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

[Fabiano Rosas]

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

> On 15/08/2022 17:09, Fabiano Rosas wrote:
>> 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?
>
> At this point in the patch series, raising interrupts with ppc_set_irq 
> always sets CPU_INTERRUPT_HARD, so it is possible that all interrupts 
> are masked, and then ppc_pending_interrupt would return zero.
>
>> Could the function's name be
>> made a bit clearer? Maybe interrupt = ppc_next_pending_interrupt or
>> something to that effect.
>> 
>
> Maybe ppc_next_unmasked_interrupt?

Could be. Although your suggestion below of moving the pending=0 case
into ppc_hw_interrupt already clears up the confusion quite a lot.

>>> +        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.
>> 
>
> This condition would not be an error in ppc_pending_interrupt because 
> we'll call this method from other places in the following patches, like 
> ppc_set_irq. Maybe we should move it to a "case 0:" in ppc_hw_interrupt?
>

Good idea, this condition is very unlikely (impossible?) to happen so
there's no need to have it prominent in this function like this. Should
it be turned into an assert as well perhaps?

>>> +        }
>>> +        return false;
>>> +    }
>>> +
>>> +    ppc_hw_interrupt(env, pending_interrupt);
>> 
>> Some verbs would be nice. ppc_deliver_interrupt?
>
> Agreed. Should we also make processor-specific versions of this method? 
> That way, we could get rid of the calls to ppc_decr_clear_on_delivery 
> and is_book3s_arch2x.

Yes, let's see what it looks like.

What interrupts exist for a given CPU along with their implementation
specific behavior are data that's linked to the CPU/group of CPUs. In
theory we could have these decisions made at build time/CPU selection
time and just operate on the smaller subset of data at runtime.

The is_book3s_arch2x check can already be removed. After the
powerpc_excp functions were split there's nothing handling DOORI; and
SDOOR is now only handled by powerpc_books. So we would just be changing
the "exception not implemented" message from DOORI to SDOOR.

... which will probably lead to some exploration of why we are
generating exceptions in TCG code for interrupts that are never
delivered/handled.

>> 
>>> +    if (env->pending_interrupts == 0) {
>>> +        cpu_reset_interrupt(cs, CPU_INTERRUPT_HARD);
>>> +    }
>>> +    return true;
>>>   }
>>>
>>>   #endif /* !CONFIG_USER_ONLY */
>
> Thanks,
> Matheus K. Ferst
> Instituto de Pesquisas ELDORADO <http://www.eldorado.org.br/>
> Analista de Software
> Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>