None of the interrupt setup code touches 'vector', so we can move it
earlier in the function. This will allow us to later move the System
Call Vectored setup that is on the top level into the
POWERPC_EXCP_SYSCALL_VECTORED code block.

This patch also moves the verification for when 'excp' does not have
an address associated with it. We now bail a little earlier when that
is the case. This should not cause any visible effects.

Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
---
 target/ppc/excp_helper.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 8b9c6bc5a8..14fd0213a0 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -352,6 +352,14 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
     }
 #endif
 
+    vector = env->excp_vectors[excp];
+    if (vector == (target_ulong)-1ULL) {
+        cpu_abort(cs, "Raised an exception without defined vector %d\n",
+                  excp);
+    }
+
+    vector |= env->excp_prefix;
+
     switch (excp) {
     case POWERPC_EXCP_NONE:
         /* Should never happen */
@@ -831,14 +839,6 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
     }
 #endif
 
-    vector = env->excp_vectors[excp];
-    if (vector == (target_ulong)-1ULL) {
-        cpu_abort(cs, "Raised an exception without defined vector %d\n",
-                  excp);
-    }
-
-    vector |= env->excp_prefix;
-
 #if defined(TARGET_PPC64)
     if (excp_model == POWERPC_EXCP_BOOKE) {
         if (env->spr[SPR_BOOKE_EPCR] & EPCR_ICM) {
-- 
2.33.1

Adding Nicholas.

On 12/20/21 19:18, Fabiano Rosas wrote:
> None of the interrupt setup code touches 'vector', so we can move it
> earlier in the function. This will allow us to later move the System
> Call Vectored setup that is on the top level into the
> POWERPC_EXCP_SYSCALL_VECTORED code block.
> 
> This patch also moves the verification for when 'excp' does not have
> an address associated with it. We now bail a little earlier when that
> is the case. This should not cause any visible effects.
> 
> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>


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

Thanks,

C.
     
> ---
>   target/ppc/excp_helper.c | 16 ++++++++--------
>   1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 8b9c6bc5a8..14fd0213a0 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -352,6 +352,14 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>       }
>   #endif
>   
> +    vector = env->excp_vectors[excp];
> +    if (vector == (target_ulong)-1ULL) {
> +        cpu_abort(cs, "Raised an exception without defined vector %d\n",
> +                  excp);
> +    }
> +
> +    vector |= env->excp_prefix;
> +
>       switch (excp) {
>       case POWERPC_EXCP_NONE:
>           /* Should never happen */
> @@ -831,14 +839,6 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>       }
>   #endif
>   
> -    vector = env->excp_vectors[excp];
> -    if (vector == (target_ulong)-1ULL) {
> -        cpu_abort(cs, "Raised an exception without defined vector %d\n",
> -                  excp);
> -    }
> -
> -    vector |= env->excp_prefix;
> -
>   #if defined(TARGET_PPC64)
>       if (excp_model == POWERPC_EXCP_BOOKE) {
>           if (env->spr[SPR_BOOKE_EPCR] & EPCR_ICM) {
> 

Excerpts from Cédric Le Goater's message of December 22, 2021 4:48 pm:
> Adding Nicholas.

These look fine by me. The whole series looks like a really nice
cleanup actually.

Thanks,
Nick

> 
> On 12/20/21 19:18, Fabiano Rosas wrote:
>> None of the interrupt setup code touches 'vector', so we can move it
>> earlier in the function. This will allow us to later move the System
>> Call Vectored setup that is on the top level into the
>> POWERPC_EXCP_SYSCALL_VECTORED code block.
>> 
>> This patch also moves the verification for when 'excp' does not have
>> an address associated with it. We now bail a little earlier when that
>> is the case. This should not cause any visible effects.
>> 
>> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
> 
> 
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> 
> Thanks,
> 
> C.
>      
>> ---
>>   target/ppc/excp_helper.c | 16 ++++++++--------
>>   1 file changed, 8 insertions(+), 8 deletions(-)
>> 
>> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
>> index 8b9c6bc5a8..14fd0213a0 100644
>> --- a/target/ppc/excp_helper.c
>> +++ b/target/ppc/excp_helper.c
>> @@ -352,6 +352,14 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>>       }
>>   #endif
>>   
>> +    vector = env->excp_vectors[excp];
>> +    if (vector == (target_ulong)-1ULL) {
>> +        cpu_abort(cs, "Raised an exception without defined vector %d\n",
>> +                  excp);
>> +    }
>> +
>> +    vector |= env->excp_prefix;
>> +
>>       switch (excp) {
>>       case POWERPC_EXCP_NONE:
>>           /* Should never happen */
>> @@ -831,14 +839,6 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>>       }
>>   #endif
>>   
>> -    vector = env->excp_vectors[excp];
>> -    if (vector == (target_ulong)-1ULL) {
>> -        cpu_abort(cs, "Raised an exception without defined vector %d\n",
>> -                  excp);
>> -    }
>> -
>> -    vector |= env->excp_prefix;
>> -
>>   #if defined(TARGET_PPC64)
>>       if (excp_model == POWERPC_EXCP_BOOKE) {
>>           if (env->spr[SPR_BOOKE_EPCR] & EPCR_ICM) {
>> 
> 
> 

On 12/20/21 10:18 AM, Fabiano Rosas wrote:
> None of the interrupt setup code touches 'vector', so we can move it
> earlier in the function. This will allow us to later move the System
> Call Vectored setup that is on the top level into the
> POWERPC_EXCP_SYSCALL_VECTORED code block.
> 
> This patch also moves the verification for when 'excp' does not have
> an address associated with it. We now bail a little earlier when that
> is the case. This should not cause any visible effects.
> 
> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
> ---
>   target/ppc/excp_helper.c | 16 ++++++++--------
>   1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 8b9c6bc5a8..14fd0213a0 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -352,6 +352,14 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>       }
>   #endif
>   
> +    vector = env->excp_vectors[excp];
> +    if (vector == (target_ulong)-1ULL) {
> +        cpu_abort(cs, "Raised an exception without defined vector %d\n",
> +                  excp);
> +    }
> +
> +    vector |= env->excp_prefix;
> +
>       switch (excp) {
>       case POWERPC_EXCP_NONE:
>           /* Should never happen */

You've moved the cpu_abort above the excp check above the early return for NONE (which 
possibly shouldn't exist) and above the excp default

         cpu_abort(cs, "Invalid PowerPC exception %d. Aborting\n", excp);

I would certainly expect invalid excp to not have a defined vector either.

I'll also note that the excp_vectors[] index is no longer bounds checked by the switch.


r~

Richard Henderson <richard.henderson@linaro.org> writes:

> On 12/20/21 10:18 AM, Fabiano Rosas wrote:
>> None of the interrupt setup code touches 'vector', so we can move it
>> earlier in the function. This will allow us to later move the System
>> Call Vectored setup that is on the top level into the
>> POWERPC_EXCP_SYSCALL_VECTORED code block.
>> 
>> This patch also moves the verification for when 'excp' does not have
>> an address associated with it. We now bail a little earlier when that
>> is the case. This should not cause any visible effects.
>> 
>> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
>> ---
>>   target/ppc/excp_helper.c | 16 ++++++++--------
>>   1 file changed, 8 insertions(+), 8 deletions(-)
>> 
>> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
>> index 8b9c6bc5a8..14fd0213a0 100644
>> --- a/target/ppc/excp_helper.c
>> +++ b/target/ppc/excp_helper.c
>> @@ -352,6 +352,14 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>>       }
>>   #endif
>>   
>> +    vector = env->excp_vectors[excp];
>> +    if (vector == (target_ulong)-1ULL) {
>> +        cpu_abort(cs, "Raised an exception without defined vector %d\n",
>> +                  excp);
>> +    }
>> +
>> +    vector |= env->excp_prefix;
>> +
>>       switch (excp) {
>>       case POWERPC_EXCP_NONE:
>>           /* Should never happen */
>
> You've moved the cpu_abort above the excp check above the early return for NONE (which 
> possibly shouldn't exist) and above the excp default

Right, I think I had this patch initially after patch 7 which moves the
NONE check ealier in the function. I'll reorganize.

>          cpu_abort(cs, "Invalid PowerPC exception %d. Aborting\n", excp);
>
> I would certainly expect invalid excp to not have a defined vector
> either.

One thing we always had that this series makes more explicit is the
distinction between what interrupts QEMU knows about and what vectors a
processor uses. I have played around with using the same data structure
to hold both, which would perhaps match the expectation more closely,
but nothing came out of it. I think there's still space after this
series for further improvement in that regard.

> I'll also note that the excp_vectors[] index is no longer bounds checked by the switch.

Ack.