[PATCH v5 9/9] target/ppc: Remove interrupt handler wrapper functions

BALATON Zoltan posted 9 patches 10 months, 1 week ago
Maintainers: Nicholas Piggin <npiggin@gmail.com>, Daniel Henrique Barboza <danielhb413@gmail.com>, "Cédric Le Goater" <clg@kaod.org>
There is a newer version of this series
[PATCH v5 9/9] target/ppc: Remove interrupt handler wrapper functions
Posted by BALATON Zoltan 10 months, 1 week ago
These wrappers call out to handle POWER7 and newer in separate
functions but reduce to the generic case when TARGET_PPC64 is not
defined. It is easy enough to include the switch in the beginning of
the generic functions to branch out to the specific functions and get
rid of these wrappers. This avoids one indirection and entitely
compiles out the switch without TARGET_PPC64.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 target/ppc/excp_helper.c | 70 ++++++++++++++++++----------------------
 1 file changed, 31 insertions(+), 39 deletions(-)

diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 5124c3e6b5..de51627c4c 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -1921,8 +1921,21 @@ static int p9_next_unmasked_interrupt(CPUPPCState *env)
 }
 #endif /* TARGET_PPC64 */
 
-static int ppc_next_unmasked_interrupt_generic(CPUPPCState *env)
+static int ppc_next_unmasked_interrupt(CPUPPCState *env)
 {
+#ifdef TARGET_PPC64
+    switch (env->excp_model) {
+    case POWERPC_EXCP_POWER7:
+        return p7_next_unmasked_interrupt(env);
+    case POWERPC_EXCP_POWER8:
+        return p8_next_unmasked_interrupt(env);
+    case POWERPC_EXCP_POWER9:
+    case POWERPC_EXCP_POWER10:
+        return p9_next_unmasked_interrupt(env);
+    default:
+        break;
+    }
+#endif
     bool async_deliver;
 
     /* External reset */
@@ -2033,23 +2046,6 @@ static int ppc_next_unmasked_interrupt_generic(CPUPPCState *env)
     return 0;
 }
 
-static int ppc_next_unmasked_interrupt(CPUPPCState *env)
-{
-    switch (env->excp_model) {
-#ifdef TARGET_PPC64
-    case POWERPC_EXCP_POWER7:
-        return p7_next_unmasked_interrupt(env);
-    case POWERPC_EXCP_POWER8:
-        return p8_next_unmasked_interrupt(env);
-    case POWERPC_EXCP_POWER9:
-    case POWERPC_EXCP_POWER10:
-        return p9_next_unmasked_interrupt(env);
-#endif
-    default:
-        return ppc_next_unmasked_interrupt_generic(env);
-    }
-}
-
 /*
  * Sets CPU_INTERRUPT_HARD if there is at least one unmasked interrupt to be
  * delivered and clears CPU_INTERRUPT_HARD otherwise.
@@ -2279,8 +2275,24 @@ static void p9_deliver_interrupt(CPUPPCState *env, int interrupt)
 }
 #endif /* TARGET_PPC64 */
 
-static void ppc_deliver_interrupt_generic(CPUPPCState *env, int interrupt)
+static void ppc_deliver_interrupt(CPUPPCState *env, int interrupt)
 {
+#ifdef TARGET_PPC64
+    switch (env->excp_model) {
+    case POWERPC_EXCP_POWER7:
+        p7_deliver_interrupt(env, interrupt);
+        return;
+    case POWERPC_EXCP_POWER8:
+        p8_deliver_interrupt(env, interrupt);
+        return;
+    case POWERPC_EXCP_POWER9:
+    case POWERPC_EXCP_POWER10:
+        p9_deliver_interrupt(env, interrupt);
+        return;
+    default:
+        break;
+    }
+#endif
     PowerPCCPU *cpu = env_archcpu(env);
 
     switch (interrupt) {
@@ -2383,26 +2395,6 @@ static void ppc_deliver_interrupt_generic(CPUPPCState *env, int interrupt)
     }
 }
 
-static void ppc_deliver_interrupt(CPUPPCState *env, int interrupt)
-{
-    switch (env->excp_model) {
-#ifdef TARGET_PPC64
-    case POWERPC_EXCP_POWER7:
-        p7_deliver_interrupt(env, interrupt);
-        break;
-    case POWERPC_EXCP_POWER8:
-        p8_deliver_interrupt(env, interrupt);
-        break;
-    case POWERPC_EXCP_POWER9:
-    case POWERPC_EXCP_POWER10:
-        p9_deliver_interrupt(env, interrupt);
-        break;
-#endif
-    default:
-        ppc_deliver_interrupt_generic(env, interrupt);
-    }
-}
-
 void ppc_cpu_do_system_reset(CPUState *cs)
 {
     PowerPCCPU *cpu = POWERPC_CPU(cs);
-- 
2.30.9
Re: [PATCH v5 9/9] target/ppc: Remove interrupt handler wrapper functions
Posted by Harsh Prateek Bora 9 months, 1 week ago

On 1/19/24 03:31, BALATON Zoltan wrote:
> These wrappers call out to handle POWER7 and newer in separate
> functions but reduce to the generic case when TARGET_PPC64 is not
> defined. It is easy enough to include the switch in the beginning of
> the generic functions to branch out to the specific functions and get
> rid of these wrappers. This avoids one indirection and entitely

s/entitely/entirely

> compiles out the switch without TARGET_PPC64.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>   target/ppc/excp_helper.c | 70 ++++++++++++++++++----------------------
>   1 file changed, 31 insertions(+), 39 deletions(-)
> 
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 5124c3e6b5..de51627c4c 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -1921,8 +1921,21 @@ static int p9_next_unmasked_interrupt(CPUPPCState *env)
>   }
>   #endif /* TARGET_PPC64 */
>   
> -static int ppc_next_unmasked_interrupt_generic(CPUPPCState *env)
> +static int ppc_next_unmasked_interrupt(CPUPPCState *env)
>   {
> +#ifdef TARGET_PPC64
> +    switch (env->excp_model) {
> +    case POWERPC_EXCP_POWER7:
> +        return p7_next_unmasked_interrupt(env);
> +    case POWERPC_EXCP_POWER8:
> +        return p8_next_unmasked_interrupt(env);
> +    case POWERPC_EXCP_POWER9:
> +    case POWERPC_EXCP_POWER10:
> +        return p9_next_unmasked_interrupt(env);
> +    default:
> +        break;
> +    }
> +#endif
>       bool async_deliver;
>   
>       /* External reset */
> @@ -2033,23 +2046,6 @@ static int ppc_next_unmasked_interrupt_generic(CPUPPCState *env)
>       return 0;
>   }
>   
> -static int ppc_next_unmasked_interrupt(CPUPPCState *env)
> -{
> -    switch (env->excp_model) {
> -#ifdef TARGET_PPC64
> -    case POWERPC_EXCP_POWER7:
> -        return p7_next_unmasked_interrupt(env);
> -    case POWERPC_EXCP_POWER8:
> -        return p8_next_unmasked_interrupt(env);
> -    case POWERPC_EXCP_POWER9:
> -    case POWERPC_EXCP_POWER10:
> -        return p9_next_unmasked_interrupt(env);
> -#endif
> -    default:
> -        return ppc_next_unmasked_interrupt_generic(env);
> -    }
> -}
> -
>   /*
>    * Sets CPU_INTERRUPT_HARD if there is at least one unmasked interrupt to be
>    * delivered and clears CPU_INTERRUPT_HARD otherwise.
> @@ -2279,8 +2275,24 @@ static void p9_deliver_interrupt(CPUPPCState *env, int interrupt)
>   }
>   #endif /* TARGET_PPC64 */
>   
> -static void ppc_deliver_interrupt_generic(CPUPPCState *env, int interrupt)
> +static void ppc_deliver_interrupt(CPUPPCState *env, int interrupt)
>   {
> +#ifdef TARGET_PPC64
> +    switch (env->excp_model) {
> +    case POWERPC_EXCP_POWER7:
> +        p7_deliver_interrupt(env, interrupt);
> +        return;
> +    case POWERPC_EXCP_POWER8:
> +        p8_deliver_interrupt(env, interrupt);
> +        return;
> +    case POWERPC_EXCP_POWER9:
> +    case POWERPC_EXCP_POWER10:
> +        p9_deliver_interrupt(env, interrupt);
> +        return;

These return statements could be clubbed with the function call itself.

With the suggested fixes,
Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>

> +    default:
> +        break;
> +    }
> +#endif
>       PowerPCCPU *cpu = env_archcpu(env);
>   
>       switch (interrupt) {
> @@ -2383,26 +2395,6 @@ static void ppc_deliver_interrupt_generic(CPUPPCState *env, int interrupt)
>       }
>   }
>   
> -static void ppc_deliver_interrupt(CPUPPCState *env, int interrupt)
> -{
> -    switch (env->excp_model) {
> -#ifdef TARGET_PPC64
> -    case POWERPC_EXCP_POWER7:
> -        p7_deliver_interrupt(env, interrupt);
> -        break;
> -    case POWERPC_EXCP_POWER8:
> -        p8_deliver_interrupt(env, interrupt);
> -        break;
> -    case POWERPC_EXCP_POWER9:
> -    case POWERPC_EXCP_POWER10:
> -        p9_deliver_interrupt(env, interrupt);
> -        break;
> -#endif
> -    default:
> -        ppc_deliver_interrupt_generic(env, interrupt);
> -    }
> -}
> -
>   void ppc_cpu_do_system_reset(CPUState *cs)
>   {
>       PowerPCCPU *cpu = POWERPC_CPU(cs);
Re: [PATCH v5 9/9] target/ppc: Remove interrupt handler wrapper functions
Posted by Harsh Prateek Bora 9 months, 1 week ago

On 1/19/24 03:31, BALATON Zoltan wrote:
> These wrappers call out to handle POWER7 and newer in separate
> functions but reduce to the generic case when TARGET_PPC64 is not
> defined. It is easy enough to include the switch in the beginning of
> the generic functions to branch out to the specific functions and get
> rid of these wrappers. This avoids one indirection and entitely

s/entitely/entirely

> compiles out the switch without TARGET_PPC64.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>   target/ppc/excp_helper.c | 70 ++++++++++++++++++----------------------
>   1 file changed, 31 insertions(+), 39 deletions(-)
> 
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 5124c3e6b5..de51627c4c 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -1921,8 +1921,21 @@ static int p9_next_unmasked_interrupt(CPUPPCState *env)
>   }
>   #endif /* TARGET_PPC64 */
>   
> -static int ppc_next_unmasked_interrupt_generic(CPUPPCState *env)
> +static int ppc_next_unmasked_interrupt(CPUPPCState *env)
>   {
> +#ifdef TARGET_PPC64
> +    switch (env->excp_model) {
> +    case POWERPC_EXCP_POWER7:
> +        return p7_next_unmasked_interrupt(env);
> +    case POWERPC_EXCP_POWER8:
> +        return p8_next_unmasked_interrupt(env);
> +    case POWERPC_EXCP_POWER9:
> +    case POWERPC_EXCP_POWER10:
> +        return p9_next_unmasked_interrupt(env);
> +    default:
> +        break;
> +    }
> +#endif
>       bool async_deliver;
>   
>       /* External reset */
> @@ -2033,23 +2046,6 @@ static int ppc_next_unmasked_interrupt_generic(CPUPPCState *env)
>       return 0;
>   }
>   
> -static int ppc_next_unmasked_interrupt(CPUPPCState *env)
> -{
> -    switch (env->excp_model) {
> -#ifdef TARGET_PPC64
> -    case POWERPC_EXCP_POWER7:
> -        return p7_next_unmasked_interrupt(env);
> -    case POWERPC_EXCP_POWER8:
> -        return p8_next_unmasked_interrupt(env);
> -    case POWERPC_EXCP_POWER9:
> -    case POWERPC_EXCP_POWER10:
> -        return p9_next_unmasked_interrupt(env);
> -#endif
> -    default:
> -        return ppc_next_unmasked_interrupt_generic(env);
> -    }
> -}
> -
>   /*
>    * Sets CPU_INTERRUPT_HARD if there is at least one unmasked interrupt to be
>    * delivered and clears CPU_INTERRUPT_HARD otherwise.
> @@ -2279,8 +2275,24 @@ static void p9_deliver_interrupt(CPUPPCState *env, int interrupt)
>   }
>   #endif /* TARGET_PPC64 */
>   
> -static void ppc_deliver_interrupt_generic(CPUPPCState *env, int interrupt)
> +static void ppc_deliver_interrupt(CPUPPCState *env, int interrupt)
>   {
> +#ifdef TARGET_PPC64
> +    switch (env->excp_model) {
> +    case POWERPC_EXCP_POWER7:
> +        p7_deliver_interrupt(env, interrupt);
> +        return;
> +    case POWERPC_EXCP_POWER8:
> +        p8_deliver_interrupt(env, interrupt);
> +        return;
> +    case POWERPC_EXCP_POWER9:
> +    case POWERPC_EXCP_POWER10:
> +        p9_deliver_interrupt(env, interrupt);
> +        return;

These return statements could be clubbed with the function call itself.

With the suggested fixes,
Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>

> +    default:
> +        break;
> +    }
> +#endif
>       PowerPCCPU *cpu = env_archcpu(env);
>   
>       switch (interrupt) {
> @@ -2383,26 +2395,6 @@ static void ppc_deliver_interrupt_generic(CPUPPCState *env, int interrupt)
>       }
>   }
>   
> -static void ppc_deliver_interrupt(CPUPPCState *env, int interrupt)
> -{
> -    switch (env->excp_model) {
> -#ifdef TARGET_PPC64
> -    case POWERPC_EXCP_POWER7:
> -        p7_deliver_interrupt(env, interrupt);
> -        break;
> -    case POWERPC_EXCP_POWER8:
> -        p8_deliver_interrupt(env, interrupt);
> -        break;
> -    case POWERPC_EXCP_POWER9:
> -    case POWERPC_EXCP_POWER10:
> -        p9_deliver_interrupt(env, interrupt);
> -        break;
> -#endif
> -    default:
> -        ppc_deliver_interrupt_generic(env, interrupt);
> -    }
> -}
> -
>   void ppc_cpu_do_system_reset(CPUState *cs)
>   {
>       PowerPCCPU *cpu = POWERPC_CPU(cs);
Re: [PATCH v5 9/9] target/ppc: Remove interrupt handler wrapper functions
Posted by BALATON Zoltan 9 months, 1 week ago
On Thu, 22 Feb 2024, Harsh Prateek Bora wrote:
> On 1/19/24 03:31, BALATON Zoltan wrote:
>> These wrappers call out to handle POWER7 and newer in separate
>> functions but reduce to the generic case when TARGET_PPC64 is not
>> defined. It is easy enough to include the switch in the beginning of
>> the generic functions to branch out to the specific functions and get
>> rid of these wrappers. This avoids one indirection and entitely
>
> s/entitely/entirely
>
>> compiles out the switch without TARGET_PPC64.
>> 
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>   target/ppc/excp_helper.c | 70 ++++++++++++++++++----------------------
>>   1 file changed, 31 insertions(+), 39 deletions(-)
>> 
>> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
>> index 5124c3e6b5..de51627c4c 100644
>> --- a/target/ppc/excp_helper.c
>> +++ b/target/ppc/excp_helper.c
>> @@ -1921,8 +1921,21 @@ static int p9_next_unmasked_interrupt(CPUPPCState 
>> *env)
>>   }
>>   #endif /* TARGET_PPC64 */
>>   -static int ppc_next_unmasked_interrupt_generic(CPUPPCState *env)
>> +static int ppc_next_unmasked_interrupt(CPUPPCState *env)
>>   {
>> +#ifdef TARGET_PPC64
>> +    switch (env->excp_model) {
>> +    case POWERPC_EXCP_POWER7:
>> +        return p7_next_unmasked_interrupt(env);
>> +    case POWERPC_EXCP_POWER8:
>> +        return p8_next_unmasked_interrupt(env);
>> +    case POWERPC_EXCP_POWER9:
>> +    case POWERPC_EXCP_POWER10:
>> +        return p9_next_unmasked_interrupt(env);
>> +    default:
>> +        break;
>> +    }
>> +#endif
>>       bool async_deliver;
>>         /* External reset */
>> @@ -2033,23 +2046,6 @@ static int 
>> ppc_next_unmasked_interrupt_generic(CPUPPCState *env)
>>       return 0;
>>   }
>>   -static int ppc_next_unmasked_interrupt(CPUPPCState *env)
>> -{
>> -    switch (env->excp_model) {
>> -#ifdef TARGET_PPC64
>> -    case POWERPC_EXCP_POWER7:
>> -        return p7_next_unmasked_interrupt(env);
>> -    case POWERPC_EXCP_POWER8:
>> -        return p8_next_unmasked_interrupt(env);
>> -    case POWERPC_EXCP_POWER9:
>> -    case POWERPC_EXCP_POWER10:
>> -        return p9_next_unmasked_interrupt(env);
>> -#endif
>> -    default:
>> -        return ppc_next_unmasked_interrupt_generic(env);
>> -    }
>> -}
>> -
>>   /*
>>    * Sets CPU_INTERRUPT_HARD if there is at least one unmasked interrupt to 
>> be
>>    * delivered and clears CPU_INTERRUPT_HARD otherwise.
>> @@ -2279,8 +2275,24 @@ static void p9_deliver_interrupt(CPUPPCState *env, 
>> int interrupt)
>>   }
>>   #endif /* TARGET_PPC64 */
>>   -static void ppc_deliver_interrupt_generic(CPUPPCState *env, int 
>> interrupt)
>> +static void ppc_deliver_interrupt(CPUPPCState *env, int interrupt)
>>   {
>> +#ifdef TARGET_PPC64
>> +    switch (env->excp_model) {
>> +    case POWERPC_EXCP_POWER7:
>> +        p7_deliver_interrupt(env, interrupt);
>> +        return;
>> +    case POWERPC_EXCP_POWER8:
>> +        p8_deliver_interrupt(env, interrupt);
>> +        return;
>> +    case POWERPC_EXCP_POWER9:
>> +    case POWERPC_EXCP_POWER10:
>> +        p9_deliver_interrupt(env, interrupt);
>> +        return;
>
> These return statements could be clubbed with the function call itself.

These return void so I thought it's cleaner this way but it appears to 
work the way you suggest too so I'll change it then.

> With the suggested fixes,
> Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>

Thanks for reviewing these. I'll send an updated version today.

Regards,
BALATON Zoltan

>> +    default:
>> +        break;
>> +    }
>> +#endif
>>       PowerPCCPU *cpu = env_archcpu(env);
>>         switch (interrupt) {
>> @@ -2383,26 +2395,6 @@ static void 
>> ppc_deliver_interrupt_generic(CPUPPCState *env, int interrupt)
>>       }
>>   }
>>   -static void ppc_deliver_interrupt(CPUPPCState *env, int interrupt)
>> -{
>> -    switch (env->excp_model) {
>> -#ifdef TARGET_PPC64
>> -    case POWERPC_EXCP_POWER7:
>> -        p7_deliver_interrupt(env, interrupt);
>> -        break;
>> -    case POWERPC_EXCP_POWER8:
>> -        p8_deliver_interrupt(env, interrupt);
>> -        break;
>> -    case POWERPC_EXCP_POWER9:
>> -    case POWERPC_EXCP_POWER10:
>> -        p9_deliver_interrupt(env, interrupt);
>> -        break;
>> -#endif
>> -    default:
>> -        ppc_deliver_interrupt_generic(env, interrupt);
>> -    }
>> -}
>> -
>>   void ppc_cpu_do_system_reset(CPUState *cs)
>>   {
>>       PowerPCCPU *cpu = POWERPC_CPU(cs);
>
>