Re: [RFC PATCH 06/13] target/ppc: remove embedded interrupts from ppc_pending_interrupt_p9

[Fabiano Rosas]

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

> On 15/08/2022 18:23, Fabiano Rosas wrote:
>> Matheus Ferst <matheus.ferst@eldorado.org.br> writes:
>> 
>>> Critical Input, Watchdog Timer, and Fixed Interval Timer are only
>>> defined for embedded CPUs. The Programmable Interval Timer is 40x-only.
>>>
>>> Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>
>>> ---
>>>   target/ppc/excp_helper.c | 18 ------------------
>>>   1 file changed, 18 deletions(-)
>>>
>>> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
>>> index 2ca6a917b2..42b57019ba 100644
>>> --- a/target/ppc/excp_helper.c
>>> +++ b/target/ppc/excp_helper.c
>>> @@ -1780,28 +1780,10 @@ static int ppc_pending_interrupt_p9(CPUPPCState 
>>> *env)
>>>               return PPC_INTERRUPT_EXT;
>>>           }
>>>       }
>>> -    if (FIELD_EX64(env->msr, MSR, CE)) {
>>> -        /* External critical interrupt */
>>> -        if (env->pending_interrupts & PPC_INTERRUPT_CEXT) {
>>> -            return PPC_INTERRUPT_CEXT;
>>> -        }
>>> -    }
>>>       if (async_deliver != 0) {
>>> -        /* Watchdog timer on embedded PowerPC */
>>> -        if (env->pending_interrupts & PPC_INTERRUPT_WDT) {
>>> -            return PPC_INTERRUPT_WDT;
>>> -        }
>>>           if (env->pending_interrupts & PPC_INTERRUPT_CDOORBELL) {
>>>               return PPC_INTERRUPT_CDOORBELL;
>>>           }
>> 
>> This one too.
>> 
>> And the Thermal.
>
> There are some other interrupts that I was not sure if we should remove:

I would keep them simply because that is an unrelated cleanup. Here you
are removing the ones that are not present in those CPUs at all. I think
the discussion about how/what QEMU emulates is a different one. If we
determine that they are indeed not used and that is not a mistake, we
could replace them with a placeholder comment or even some explanation
of why we don't need them.

> - PPC_INTERRUPT_PERFM doesn't seem to be used anywhere else. I guess it 
> will be used when we implement more PMU stuff, so I left it in all 
> ppc_pending_interrupt_* methods.
> - PPC_INTERRUPT_RESET was treated in cpu_has_work_POWER*, but AFAICS, 
> it's only used in ppc6xx_set_irq and ppc970_set_irq, which means it can 
> only be raised on 6xx, 7xx, 970, and POWER5+. Should we remove it too?

I'm not sure if we have an external interrupt source that affects the
CPU like that. I see that we simply call powerpc_excp to reset the CPUs
when we need it.

Cédric, any thoughts?

>> 
>>> -        /* Fixed interval timer on embedded PowerPC */
>>> -        if (env->pending_interrupts & PPC_INTERRUPT_FIT) {
>>> -            return PPC_INTERRUPT_FIT;
>>> -        }
>>> -        /* Programmable interval timer on embedded PowerPC */
>>> -        if (env->pending_interrupts & PPC_INTERRUPT_PIT) {
>>> -            return PPC_INTERRUPT_PIT;
>>> -        }
>>>           /* Decrementer exception */
>>>           if (env->pending_interrupts & PPC_INTERRUPT_DECR) {
>>>               return PPC_INTERRUPT_DECR;
>
> Tḧanks,
> 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>