diff mbox series

[v9,10/10] target/ppc/excp_helper.c: EBB handling adjustments

Message ID 20211201151734.654994-11-danielhb413@gmail.com
State Changes Requested
Headers show
Series PMU-EBB support for PPC64 TCG | expand

Commit Message

Daniel Henrique Barboza Dec. 1, 2021, 3:17 p.m. UTC 
The current logic is only considering event-based exceptions triggered
by the performance monitor. This is true now, but we might want to add
support for external event-based exceptions in the future.

Let's make it a bit easier to do so by adding the bit logic that would
happen in case we were dealing with an external event-based exception.

While we're at it, add a few comments explaining why we're setting and
clearing BESCR bits.

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 target/ppc/excp_helper.c | 45 ++++++++++++++++++++++++++++++++++------
 1 file changed, 39 insertions(+), 6 deletions(-)

Comments

David Gibson Dec. 9, 2021, 1:52 a.m. UTC | #1 
On Wed, Dec 01, 2021 at 12:17:34PM -0300, Daniel Henrique Barboza wrote:
> The current logic is only considering event-based exceptions triggered
> by the performance monitor. This is true now, but we might want to add
> support for external event-based exceptions in the future.
> 
> Let's make it a bit easier to do so by adding the bit logic that would
> happen in case we were dealing with an external event-based exception.
> 
> While we're at it, add a few comments explaining why we're setting and
> clearing BESCR bits.
> 
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

Still looks fine, but I'm not seeing a particularly strong reason to
keep this split from the previous patch.

> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>  target/ppc/excp_helper.c | 45 ++++++++++++++++++++++++++++++++++------
>  1 file changed, 39 insertions(+), 6 deletions(-)
> 
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index a26d266fe6..42e2fee9c8 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -801,14 +801,47 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>          break;
>      case POWERPC_EXCP_EBB:       /* Event-based branch exception             */
>          if ((env->spr[SPR_FSCR] & (1ull << FSCR_EBB)) &&
> -            (env->spr[SPR_BESCR] & BESCR_GE) &&
> -            (env->spr[SPR_BESCR] & BESCR_PME)) {
> +            (env->spr[SPR_BESCR] & BESCR_GE)) {
>              target_ulong nip;
>  
> -            env->spr[SPR_BESCR] &= ~BESCR_GE;   /* Clear GE */
> -            env->spr[SPR_BESCR] |= BESCR_PMEO;  /* Set PMEO */
> -            env->spr[SPR_EBBRR] = env->nip;     /* Save NIP for rfebb insn */
> -            nip = env->spr[SPR_EBBHR];          /* EBB handler */
> +            /*
> +             * If we have Performance Monitor Event-Based exception
> +             * enabled (BESCR_PME) and a Performance Monitor alert
> +             * occurred (MMCR0_PMAO), clear BESCR_PME and set BESCR_PMEO
> +             * (Performance Monitor Event-Based Exception Occurred).
> +             *
> +             * Software is responsible for clearing both BESCR_PMEO and
> +             * MMCR0_PMAO after the event has been handled.
> +             */
> +            if ((env->spr[SPR_BESCR] & BESCR_PME) &&
> +                (env->spr[SPR_POWER_MMCR0] & MMCR0_PMAO)) {
> +                env->spr[SPR_BESCR] &= ~BESCR_PME;
> +                env->spr[SPR_BESCR] |= BESCR_PMEO;
> +            }
> +
> +            /*
> +             * In the case of External Event-Based exceptions, do a
> +             * similar logic with BESCR_EE and BESCR_EEO. BESCR_EEO must
> +             * also be cleared by software.
> +             *
> +             * PowerISA 3.1 considers that we'll not have BESCR_PMEO and
> +             * BESCR_EEO set at the same time. We can check for BESCR_PMEO
> +             * being not set in step above to see if this exception was
> +             * trigged by an external event.
> +             */
> +            if (env->spr[SPR_BESCR] & BESCR_EE &&
> +                !(env->spr[SPR_BESCR] & BESCR_PMEO)) {
> +                env->spr[SPR_BESCR] &= ~BESCR_EE;
> +                env->spr[SPR_BESCR] |= BESCR_EEO;
> +            }
> +
> +            /*
> +             * Clear BESCR_GE, save NIP for 'rfebb' and point the
> +             * execution to the event handler (SPR_EBBHR) address.
> +             */
> +            env->spr[SPR_BESCR] &= ~BESCR_GE;
> +            env->spr[SPR_EBBRR] = env->nip;
> +            nip = env->spr[SPR_EBBHR];
>              powerpc_set_excp_state(cpu, nip, env->msr);
>          }
>          /*
Daniel Henrique Barboza Dec. 14, 2021, 8:05 p.m. UTC | #2 
On 12/8/21 22:52, David Gibson wrote:
> On Wed, Dec 01, 2021 at 12:17:34PM -0300, Daniel Henrique Barboza wrote:
>> The current logic is only considering event-based exceptions triggered
>> by the performance monitor. This is true now, but we might want to add
>> support for external event-based exceptions in the future.
>>
>> Let's make it a bit easier to do so by adding the bit logic that would
>> happen in case we were dealing with an external event-based exception.
>>
>> While we're at it, add a few comments explaining why we're setting and
>> clearing BESCR bits.
>>
>> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> 
> Still looks fine, but I'm not seeing a particularly strong reason to
> keep this split from the previous patch.
>

Fair enough. I'll squash this patch with the previous one.



Thanks,


Daniel

  
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
>>   target/ppc/excp_helper.c | 45 ++++++++++++++++++++++++++++++++++------
>>   1 file changed, 39 insertions(+), 6 deletions(-)
>>
>> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
>> index a26d266fe6..42e2fee9c8 100644
>> --- a/target/ppc/excp_helper.c
>> +++ b/target/ppc/excp_helper.c
>> @@ -801,14 +801,47 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>>           break;
>>       case POWERPC_EXCP_EBB:       /* Event-based branch exception             */
>>           if ((env->spr[SPR_FSCR] & (1ull << FSCR_EBB)) &&
>> -            (env->spr[SPR_BESCR] & BESCR_GE) &&
>> -            (env->spr[SPR_BESCR] & BESCR_PME)) {
>> +            (env->spr[SPR_BESCR] & BESCR_GE)) {
>>               target_ulong nip;
>>   
>> -            env->spr[SPR_BESCR] &= ~BESCR_GE;   /* Clear GE */
>> -            env->spr[SPR_BESCR] |= BESCR_PMEO;  /* Set PMEO */
>> -            env->spr[SPR_EBBRR] = env->nip;     /* Save NIP for rfebb insn */
>> -            nip = env->spr[SPR_EBBHR];          /* EBB handler */
>> +            /*
>> +             * If we have Performance Monitor Event-Based exception
>> +             * enabled (BESCR_PME) and a Performance Monitor alert
>> +             * occurred (MMCR0_PMAO), clear BESCR_PME and set BESCR_PMEO
>> +             * (Performance Monitor Event-Based Exception Occurred).
>> +             *
>> +             * Software is responsible for clearing both BESCR_PMEO and
>> +             * MMCR0_PMAO after the event has been handled.
>> +             */
>> +            if ((env->spr[SPR_BESCR] & BESCR_PME) &&
>> +                (env->spr[SPR_POWER_MMCR0] & MMCR0_PMAO)) {
>> +                env->spr[SPR_BESCR] &= ~BESCR_PME;
>> +                env->spr[SPR_BESCR] |= BESCR_PMEO;
>> +            }
>> +
>> +            /*
>> +             * In the case of External Event-Based exceptions, do a
>> +             * similar logic with BESCR_EE and BESCR_EEO. BESCR_EEO must
>> +             * also be cleared by software.
>> +             *
>> +             * PowerISA 3.1 considers that we'll not have BESCR_PMEO and
>> +             * BESCR_EEO set at the same time. We can check for BESCR_PMEO
>> +             * being not set in step above to see if this exception was
>> +             * trigged by an external event.
>> +             */
>> +            if (env->spr[SPR_BESCR] & BESCR_EE &&
>> +                !(env->spr[SPR_BESCR] & BESCR_PMEO)) {
>> +                env->spr[SPR_BESCR] &= ~BESCR_EE;
>> +                env->spr[SPR_BESCR] |= BESCR_EEO;
>> +            }
>> +
>> +            /*
>> +             * Clear BESCR_GE, save NIP for 'rfebb' and point the
>> +             * execution to the event handler (SPR_EBBHR) address.
>> +             */
>> +            env->spr[SPR_BESCR] &= ~BESCR_GE;
>> +            env->spr[SPR_EBBRR] = env->nip;
>> +            nip = env->spr[SPR_EBBHR];
>>               powerpc_set_excp_state(cpu, nip, env->msr);
>>           }
>>           /*
>
diff mbox series

Patch

diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index a26d266fe6..42e2fee9c8 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -801,14 +801,47 @@  static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
         break;
     case POWERPC_EXCP_EBB:       /* Event-based branch exception             */
         if ((env->spr[SPR_FSCR] & (1ull << FSCR_EBB)) &&
-            (env->spr[SPR_BESCR] & BESCR_GE) &&
-            (env->spr[SPR_BESCR] & BESCR_PME)) {
+            (env->spr[SPR_BESCR] & BESCR_GE)) {
             target_ulong nip;
 
-            env->spr[SPR_BESCR] &= ~BESCR_GE;   /* Clear GE */
-            env->spr[SPR_BESCR] |= BESCR_PMEO;  /* Set PMEO */
-            env->spr[SPR_EBBRR] = env->nip;     /* Save NIP for rfebb insn */
-            nip = env->spr[SPR_EBBHR];          /* EBB handler */
+            /*
+             * If we have Performance Monitor Event-Based exception
+             * enabled (BESCR_PME) and a Performance Monitor alert
+             * occurred (MMCR0_PMAO), clear BESCR_PME and set BESCR_PMEO
+             * (Performance Monitor Event-Based Exception Occurred).
+             *
+             * Software is responsible for clearing both BESCR_PMEO and
+             * MMCR0_PMAO after the event has been handled.
+             */
+            if ((env->spr[SPR_BESCR] & BESCR_PME) &&
+                (env->spr[SPR_POWER_MMCR0] & MMCR0_PMAO)) {
+                env->spr[SPR_BESCR] &= ~BESCR_PME;
+                env->spr[SPR_BESCR] |= BESCR_PMEO;
+            }
+
+            /*
+             * In the case of External Event-Based exceptions, do a
+             * similar logic with BESCR_EE and BESCR_EEO. BESCR_EEO must
+             * also be cleared by software.
+             *
+             * PowerISA 3.1 considers that we'll not have BESCR_PMEO and
+             * BESCR_EEO set at the same time. We can check for BESCR_PMEO
+             * being not set in step above to see if this exception was
+             * trigged by an external event.
+             */
+            if (env->spr[SPR_BESCR] & BESCR_EE &&
+                !(env->spr[SPR_BESCR] & BESCR_PMEO)) {
+                env->spr[SPR_BESCR] &= ~BESCR_EE;
+                env->spr[SPR_BESCR] |= BESCR_EEO;
+            }
+
+            /*
+             * Clear BESCR_GE, save NIP for 'rfebb' and point the
+             * execution to the event handler (SPR_EBBHR) address.
+             */
+            env->spr[SPR_BESCR] &= ~BESCR_GE;
+            env->spr[SPR_EBBRR] = env->nip;
+            nip = env->spr[SPR_EBBHR];
             powerpc_set_excp_state(cpu, nip, env->msr);
         }
         /*