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

[Harsh Prateek Bora]

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);