[v2] target/ppc: Fix BookE debug interrupt generation

[PATCH v2] target/ppc: Fix BookE debug interrupt generation

Posted by Bin Meng 3 years, 9 months ago

From: Bin Meng <bin.meng@windriver.com>

Per E500 core reference manual [1], chapter 8.4.4 "Branch Taken Debug
Event" and chapter 8.4.5 "Instruction Complete Debug Event":

  "A branch taken debug event occurs if both MSR[DE] and DBCR0[BRT]
  are set ... Branch taken debug events are not recognized if MSR[DE]
  is cleared when the branch instruction executes."

  "An instruction complete debug event occurs when any instruction
  completes execution so long as MSR[DE] and DBCR0[ICMP] are both
  set ... Instruction complete debug events are not recognized if
  MSR[DE] is cleared at the time of the instruction execution."

Current codes do not check MSR.DE bit before setting HFLAGS_SE and
HFLAGS_BE flag, which would cause the immediate debug interrupt to
be generated, e.g.: when DBCR0.ICMP bit is set by guest software
and MSR.DE is not set.

[1] https://www.nxp.com/docs/en/reference-manual/E500CORERM.pdf

Signed-off-by: Bin Meng <bin.meng@windriver.com>
---

Changes in v2:
- update commit message to use E500CORERM instead of PowerISA 2.07

 target/ppc/helper_regs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
index 9a691d6833..77bc57415c 100644
--- a/target/ppc/helper_regs.c
+++ b/target/ppc/helper_regs.c
@@ -63,10 +63,10 @@ static uint32_t hreg_compute_hflags_value(CPUPPCState *env)
 
     if (ppc_flags & POWERPC_FLAG_DE) {
         target_ulong dbcr0 = env->spr[SPR_BOOKE_DBCR0];
-        if (dbcr0 & DBCR0_ICMP) {
+        if ((dbcr0 & DBCR0_ICMP) && msr_de) {
             hflags |= 1 << HFLAGS_SE;
         }
-        if (dbcr0 & DBCR0_BRT) {
+        if ((dbcr0 & DBCR0_BRT) && msr_de) {
             hflags |= 1 << HFLAGS_BE;
         }
     } else {
-- 
2.25.1

Re: [PATCH v2] target/ppc: Fix BookE debug interrupt generation

Posted by Daniel Henrique Barboza 3 years, 9 months ago

Queued in gitlab.com/danielhb/qemu/tree/ppc-next. Thanks,


Daniel

On 4/20/22 22:17, Bin Meng wrote:
> From: Bin Meng <bin.meng@windriver.com>
> 
> Per E500 core reference manual [1], chapter 8.4.4 "Branch Taken Debug
> Event" and chapter 8.4.5 "Instruction Complete Debug Event":
> 
>    "A branch taken debug event occurs if both MSR[DE] and DBCR0[BRT]
>    are set ... Branch taken debug events are not recognized if MSR[DE]
>    is cleared when the branch instruction executes."
> 
>    "An instruction complete debug event occurs when any instruction
>    completes execution so long as MSR[DE] and DBCR0[ICMP] are both
>    set ... Instruction complete debug events are not recognized if
>    MSR[DE] is cleared at the time of the instruction execution."
> 
> Current codes do not check MSR.DE bit before setting HFLAGS_SE and
> HFLAGS_BE flag, which would cause the immediate debug interrupt to
> be generated, e.g.: when DBCR0.ICMP bit is set by guest software
> and MSR.DE is not set.
> 
> [1] https://www.nxp.com/docs/en/reference-manual/E500CORERM.pdf
> 
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> ---
> 
> Changes in v2:
> - update commit message to use E500CORERM instead of PowerISA 2.07
> 
>   target/ppc/helper_regs.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
> index 9a691d6833..77bc57415c 100644
> --- a/target/ppc/helper_regs.c
> +++ b/target/ppc/helper_regs.c
> @@ -63,10 +63,10 @@ static uint32_t hreg_compute_hflags_value(CPUPPCState *env)
>   
>       if (ppc_flags & POWERPC_FLAG_DE) {
>           target_ulong dbcr0 = env->spr[SPR_BOOKE_DBCR0];
> -        if (dbcr0 & DBCR0_ICMP) {
> +        if ((dbcr0 & DBCR0_ICMP) && msr_de) {
>               hflags |= 1 << HFLAGS_SE;
>           }
> -        if (dbcr0 & DBCR0_BRT) {
> +        if ((dbcr0 & DBCR0_BRT) && msr_de) {
>               hflags |= 1 << HFLAGS_BE;
>           }
>       } else {

Re: [PATCH v2] target/ppc: Fix BookE debug interrupt generation

Posted by Lucas Mateus Martins Araujo e Castro 3 years, 9 months ago

On 20/04/2022 22:17, Bin Meng wrote:
> From: Bin Meng<bin.meng@windriver.com>
>
> Per E500 core reference manual [1], chapter 8.4.4 "Branch Taken Debug
> Event" and chapter 8.4.5 "Instruction Complete Debug Event":
>
>    "A branch taken debug event occurs if both MSR[DE] and DBCR0[BRT]
>    are set ... Branch taken debug events are not recognized if MSR[DE]
>    is cleared when the branch instruction executes."
>
>    "An instruction complete debug event occurs when any instruction
>    completes execution so long as MSR[DE] and DBCR0[ICMP] are both
>    set ... Instruction complete debug events are not recognized if
>    MSR[DE] is cleared at the time of the instruction execution."
>
> Current codes do not check MSR.DE bit before setting HFLAGS_SE and
> HFLAGS_BE flag, which would cause the immediate debug interrupt to
> be generated, e.g.: when DBCR0.ICMP bit is set by guest software
> and MSR.DE is not set.
>
> [1]https://www.nxp.com/docs/en/reference-manual/E500CORERM.pdf
>
> Signed-off-by: Bin Meng<bin.meng@windriver.com>
> ---
>
> Changes in v2:
> - update commit message to use E500CORERM instead of PowerISA 2.07
>
>   target/ppc/helper_regs.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
> index 9a691d6833..77bc57415c 100644
> --- a/target/ppc/helper_regs.c
> +++ b/target/ppc/helper_regs.c
> @@ -63,10 +63,10 @@ static uint32_t hreg_compute_hflags_value(CPUPPCState *env)
>
>       if (ppc_flags & POWERPC_FLAG_DE) {
>           target_ulong dbcr0 = env->spr[SPR_BOOKE_DBCR0];
> -        if (dbcr0 & DBCR0_ICMP) {
> +        if ((dbcr0 & DBCR0_ICMP) && msr_de) {
There was a discussion some time ago that was better to avoid hidden 
uses of *env, so it may be better to change msr_de to ((env->msr >> 
MSR_DE) & 1) or to (env->msr & BIT_ULL(MSR_DE))
>               hflags |= 1 << HFLAGS_SE;
>           }
> -        if (dbcr0 & DBCR0_BRT) {
> +        if ((dbcr0 & DBCR0_BRT) && msr_de) {
Here as well
>               hflags |= 1 << HFLAGS_BE;
>           }
>       } else {
> --
> 2.25.1
>
>
Apart from that,
Reviewed-by: Lucas Mateus Castro <lucas.araujo@eldorado.org.br>
-- 
Lucas Mateus M. Araujo e Castro
Instituto de Pesquisas ELDORADO 
<https://www.eldorado.org.br/?utm_campaign=assinatura_de_e-mail&utm_medium=email&utm_source=RD+Station>
Departamento Computação Embarcada
Analista de Software Trainee
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>

Re: [PATCH v2] target/ppc: Fix BookE debug interrupt generation

Posted by Cédric Le Goater 3 years, 9 months ago

On 4/25/22 16:16, Lucas Mateus Martins Araujo e Castro wrote:
> 
> On 20/04/2022 22:17, Bin Meng wrote:
>> From: Bin Meng<bin.meng@windriver.com>
>>
>> Per E500 core reference manual [1], chapter 8.4.4 "Branch Taken Debug
>> Event" and chapter 8.4.5 "Instruction Complete Debug Event":
>>
>>    "A branch taken debug event occurs if both MSR[DE] and DBCR0[BRT]
>>    are set ... Branch taken debug events are not recognized if MSR[DE]
>>    is cleared when the branch instruction executes."
>>
>>    "An instruction complete debug event occurs when any instruction
>>    completes execution so long as MSR[DE] and DBCR0[ICMP] are both
>>    set ... Instruction complete debug events are not recognized if
>>    MSR[DE] is cleared at the time of the instruction execution."
>>
>> Current codes do not check MSR.DE bit before setting HFLAGS_SE and
>> HFLAGS_BE flag, which would cause the immediate debug interrupt to
>> be generated, e.g.: when DBCR0.ICMP bit is set by guest software
>> and MSR.DE is not set.
>>
>> [1]https://www.nxp.com/docs/en/reference-manual/E500CORERM.pdf
>>
>> Signed-off-by: Bin Meng<bin.meng@windriver.com>
>> ---
>>
>> Changes in v2:
>> - update commit message to use E500CORERM instead of PowerISA 2.07
>>
>>   target/ppc/helper_regs.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
>> index 9a691d6833..77bc57415c 100644
>> --- a/target/ppc/helper_regs.c
>> +++ b/target/ppc/helper_regs.c
>> @@ -63,10 +63,10 @@ static uint32_t hreg_compute_hflags_value(CPUPPCState *env)
>>
>>       if (ppc_flags & POWERPC_FLAG_DE) {
>>           target_ulong dbcr0 = env->spr[SPR_BOOKE_DBCR0];
>> -        if (dbcr0 & DBCR0_ICMP) {
>> +        if ((dbcr0 & DBCR0_ICMP) && msr_de) {
> There was a discussion some time ago that was better to avoid hidden uses of *env, so it may be better to change msr_de to ((env->msr >> MSR_DE) & 1) or to (env->msr & BIT_ULL(MSR_DE))

I think the "target/ppc: Remove hidden usages of *env" patchset :

   https://patchwork.ozlabs.org/project/qemu-ppc/list/?series=296440

will take some time to merge and there will certainly be followups.

This patch can go first.

Thanks,

C.


>>               hflags |= 1 << HFLAGS_SE;
>>           }
>> -        if (dbcr0 & DBCR0_BRT) {
>> +        if ((dbcr0 & DBCR0_BRT) && msr_de) {
> Here as well
>>               hflags |= 1 << HFLAGS_BE;
>>           }
>>       } else {
>> --
>> 2.25.1
>>
>>
> Apart from that,
> Reviewed-by: Lucas Mateus Castro <lucas.araujo@eldorado.org.br>
> -- 
> Lucas Mateus M. Araujo e Castro
> Instituto de Pesquisas ELDORADO <https://www.eldorado.org.br/?utm_campaign=assinatura_de_e-mail&utm_medium=email&utm_source=RD+Station>
> Departamento Computação Embarcada
> Analista de Software Trainee
> Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>

Re: [PATCH v2] target/ppc: Fix BookE debug interrupt generation

Posted by Fabiano Rosas 3 years, 9 months ago

Bin Meng <bmeng.cn@gmail.com> writes:

> From: Bin Meng <bin.meng@windriver.com>
>
> Per E500 core reference manual [1], chapter 8.4.4 "Branch Taken Debug
> Event" and chapter 8.4.5 "Instruction Complete Debug Event":
>
>   "A branch taken debug event occurs if both MSR[DE] and DBCR0[BRT]
>   are set ... Branch taken debug events are not recognized if MSR[DE]
>   is cleared when the branch instruction executes."
>
>   "An instruction complete debug event occurs when any instruction
>   completes execution so long as MSR[DE] and DBCR0[ICMP] are both
>   set ... Instruction complete debug events are not recognized if
>   MSR[DE] is cleared at the time of the instruction execution."
>
> Current codes do not check MSR.DE bit before setting HFLAGS_SE and
> HFLAGS_BE flag, which would cause the immediate debug interrupt to
> be generated, e.g.: when DBCR0.ICMP bit is set by guest software
> and MSR.DE is not set.
>
> [1] https://www.nxp.com/docs/en/reference-manual/E500CORERM.pdf
>
> Signed-off-by: Bin Meng <bin.meng@windriver.com>

Reviewed-by: Fabiano Rosas <farosas@linux.ibm.com>

Re: [PATCH v2] target/ppc: Fix BookE debug interrupt generation

Posted by Cédric Le Goater 3 years, 9 months ago

On 4/21/22 03:17, Bin Meng wrote:
> From: Bin Meng <bin.meng@windriver.com>
> 
> Per E500 core reference manual [1], chapter 8.4.4 "Branch Taken Debug
> Event" and chapter 8.4.5 "Instruction Complete Debug Event":
> 
>    "A branch taken debug event occurs if both MSR[DE] and DBCR0[BRT]
>    are set ... Branch taken debug events are not recognized if MSR[DE]
>    is cleared when the branch instruction executes."
> 
>    "An instruction complete debug event occurs when any instruction
>    completes execution so long as MSR[DE] and DBCR0[ICMP] are both
>    set ... Instruction complete debug events are not recognized if
>    MSR[DE] is cleared at the time of the instruction execution."
> 
> Current codes do not check MSR.DE bit before setting HFLAGS_SE and
> HFLAGS_BE flag, which would cause the immediate debug interrupt to
> be generated, e.g.: when DBCR0.ICMP bit is set by guest software
> and MSR.DE is not set.
> 
> [1] https://www.nxp.com/docs/en/reference-manual/E500CORERM.pdf
> 
> Signed-off-by: Bin Meng <bin.meng@windriver.com>

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.

> ---
> 
> Changes in v2:
> - update commit message to use E500CORERM instead of PowerISA 2.07
> 
>   target/ppc/helper_regs.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
> index 9a691d6833..77bc57415c 100644
> --- a/target/ppc/helper_regs.c
> +++ b/target/ppc/helper_regs.c
> @@ -63,10 +63,10 @@ static uint32_t hreg_compute_hflags_value(CPUPPCState *env)
>   
>       if (ppc_flags & POWERPC_FLAG_DE) {
>           target_ulong dbcr0 = env->spr[SPR_BOOKE_DBCR0];
> -        if (dbcr0 & DBCR0_ICMP) {
> +        if ((dbcr0 & DBCR0_ICMP) && msr_de) {
>               hflags |= 1 << HFLAGS_SE;
>           }
> -        if (dbcr0 & DBCR0_BRT) {
> +        if ((dbcr0 & DBCR0_BRT) && msr_de) {
>               hflags |= 1 << HFLAGS_BE;
>           }
>       } else {