Re: [RFC PATCH 03/13] target/ppc: move interrupt masking out of ppc_hw_interrupt

[Fabiano Rosas]

Matheus Ferst <matheus.ferst@eldorado.org.br> writes:

> Move the interrupt masking logic to a new method, ppc_pending_interrupt,
> and only handle the interrupt processing in ppc_hw_interrupt.
>
> Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>
> ---
>  target/ppc/excp_helper.c | 228 ++++++++++++++++++++++++---------------
>  1 file changed, 141 insertions(+), 87 deletions(-)
>
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c

<snip>

> @@ -1884,15 +1915,38 @@ bool ppc_cpu_exec_interrupt(CPUState *cs, int 
> interrupt_request)
>  {
>      PowerPCCPU *cpu = POWERPC_CPU(cs);
>      CPUPPCState *env = &cpu->env;
> +    int pending_interrupt;

I would give this a simpler name to avoid confusion with the other two
pending_interrupt terms.

>  
> -    if (interrupt_request & CPU_INTERRUPT_HARD) {
> -        ppc_hw_interrupt(env);
> -        if (env->pending_interrupts == 0) {
> -            cs->interrupt_request &= ~CPU_INTERRUPT_HARD;
> -        }
> -        return true;
> +    if ((interrupt_request & CPU_INTERRUPT_HARD) == 0) {
> +        return false;
>      }
> -    return false;
> +

It seems we're assuming that after this point we certainly have some
pending interrupt...

> +    pending_interrupt = ppc_pending_interrupt(env);
> +    if (pending_interrupt == 0) {

...but how then is this able to return 0? Could the function's name be
made a bit clearer? Maybe interrupt = ppc_next_pending_interrupt or
something to that effect.

> +        if (env->resume_as_sreset) {
> +            /*
> +             * This is a bug ! It means that has_work took us out of halt
> +             * without anything to deliver while in a PM state that requires
> +             * getting out via a 0x100
> +             *
> +             * This means we will incorrectly execute past the power 
> management
> +             * instruction instead of triggering a reset.
> +             *
> +             * It generally means a discrepancy between the wakeup 
> conditions in
> +             * the processor has_work implementation and the logic in this
> +             * function.
> +             */
> +            cpu_abort(env_cpu(env),
> +                      "Wakeup from PM state but interrupt Undelivered");

This condition is BookS only. Perhaps it would be better to move it
inside each of the processor-specific functions. And since you're
merging has_work with pending_interrupts, can't you solve that issue
earlier? Specifically the "has_work tooks us out of halt..." part.

> +        }
> +        return false;
> +    }
> +
> +    ppc_hw_interrupt(env, pending_interrupt);

Some verbs would be nice. ppc_deliver_interrupt?

> +    if (env->pending_interrupts == 0) {
> +        cpu_reset_interrupt(cs, CPU_INTERRUPT_HARD);
> +    }
> +    return true;
>  }
>  
>  #endif /* !CONFIG_USER_ONLY */