Re: [RFC PATCH v2 11/29] target/ppc: add power-saving interrupt masking logic to p9_next_unmasked_interrupt

[Fabiano Rosas]

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

> On 30/09/2022 15:38, Fabiano Rosas wrote:
>> Matheus Ferst <matheus.ferst@eldorado.org.br> writes:
>> 
>>> Export p9_interrupt_powersave and use it in p9_next_unmasked_interrupt.
>>>
>>> Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>
>>> ---
>>> Temporarily putting the prototype in internal.h for lack of a better place,
>>> we will un-export p9_interrupt_powersave in future patches.
>>> ---
>>>   target/ppc/cpu_init.c    |  2 +-
>>>   target/ppc/excp_helper.c | 46 ++++++++++++++++++++++++++++------------
>>>   target/ppc/internal.h    |  4 ++++
>>>   3 files changed, 38 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
>>> index 1f8f6c6ef2..7889158c52 100644
>>> --- a/target/ppc/cpu_init.c
>>> +++ b/target/ppc/cpu_init.c
>>> @@ -6351,7 +6351,7 @@ static bool ppc_pvr_match_power9(PowerPCCPUClass 
>>> *pcc, uint32_t pvr, bool best)
>>>       return false;
>>>   }
>>>
>>> -static int p9_interrupt_powersave(CPUPPCState *env)
>>> +int p9_interrupt_powersave(CPUPPCState *env)
>>>   {
>>>       /* External Exception */
>>>       if ((env->pending_interrupts & PPC_INTERRUPT_EXT) &&
>>> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
>>> index 67e73f30ab..5a0d2c11a2 100644
>>> --- a/target/ppc/excp_helper.c
>>> +++ b/target/ppc/excp_helper.c
>>> @@ -1686,28 +1686,39 @@ void ppc_cpu_do_interrupt(CPUState *cs)
>>>
>>>   static int p9_next_unmasked_interrupt(CPUPPCState *env)
>>>   {
>>> -    bool async_deliver;
>>> +    PowerPCCPU *cpu = env_archcpu(env);
>>> +    CPUState *cs = CPU(cpu);
>>> +    /* Ignore MSR[EE] when coming out of some power management states */
>>> +    bool msr_ee = FIELD_EX64(env->msr, MSR, EE) || env->resume_as_sreset;
>>>
>>>       assert((env->pending_interrupts & P9_UNUSED_INTERRUPTS) == 0);
>>>
>>> +    if (cs->halted) {
>>> +        if (env->spr[SPR_PSSCR] & PSSCR_EC) {
>>> +            /*
>>> +             * When PSSCR[EC] is set, LPCR[PECE] controls which interrupts 
>>> can
>>> +             * wakeup the processor
>>> +             */
>>> +            return p9_interrupt_powersave(env);
>>> +        } else {
>>> +            /*
>>> +             * When it's clear, any system-caused exception exits 
>>> power-saving
>>> +             * mode, even the ones that gate on MSR[EE].
>>> +             */
>>> +            msr_ee = true;
>>> +        }
>>> +    }
>>> +
>>>       /* Machine check exception */
>>>       if (env->pending_interrupts & PPC_INTERRUPT_MCK) {
>>>           return PPC_INTERRUPT_MCK;
>>>       }
>>>
>>> -    /*
>>> -     * For interrupts that gate on MSR:EE, we need to do something a
>>> -     * bit more subtle, as we need to let them through even when EE is
>>> -     * clear when coming out of some power management states (in order
>>> -     * for them to become a 0x100).
>>> -     */
>>> -    async_deliver = FIELD_EX64(env->msr, MSR, EE) || env->resume_as_sreset;
>>> -
>> 
>> You could simplify the code below if you bail early here when !msr_ee.
>> 
>
> The next interrupts have checks in the form
>
> if (MSR[EE] && some_condition) || (!MSR[HV] && some_other_condition)
>
> so we cannot return yet. We could check twice for these interrupts, e.g.

Ah, ok. Let's leave like it is then.

>
> if ((env->pending_interrupts & PPC_INTERRUPT_EXT) &&
>      (env->has_hv_mode && !FIELD_EX64_HV(env->msr) && !lpes0)) {
>      return PPC_INTERRUPT_EXT;
> }
>
> /* ... */
>
> if (!msr_ee) {
>      return 0;
> }
>
> /* ... */
>
> if ((env->pending_interrupts & PPC_INTERRUPT_EXT) &&
>      !(heic && FIELD_EX64_HV(env->msr) && !FIELD_EX64(env->msr, MSR, PR))) {
>      return PPC_INTERRUPT_EXT;
> }
>
> But I'm not sure if it'd be better.
>
>>>       /* Hypervisor decrementer exception */
>>>       if (env->pending_interrupts & PPC_INTERRUPT_HDECR) {
>>>           /* LPCR will be clear when not supported so this will work */
>>>           bool hdice = !!(env->spr[SPR_LPCR] & LPCR_HDICE);
>>> -        if ((async_deliver || !FIELD_EX64_HV(env->msr)) && hdice) {
>>> +        if ((msr_ee || !FIELD_EX64_HV(env->msr)) && hdice) {
>>>               /* HDEC clears on delivery */
>>>               return PPC_INTERRUPT_HDECR;
>>>           }
>>> @@ -1717,7 +1728,7 @@ static int p9_next_unmasked_interrupt(CPUPPCState 
>>> *env)
>>>       if (env->pending_interrupts & PPC_INTERRUPT_HVIRT) {
>>>           /* LPCR will be clear when not supported so this will work */
>>>           bool hvice = !!(env->spr[SPR_LPCR] & LPCR_HVICE);
>>> -        if ((async_deliver || !FIELD_EX64_HV(env->msr)) && hvice) {
>>> +        if ((msr_ee || !FIELD_EX64_HV(env->msr)) && hvice) {
>>>               return PPC_INTERRUPT_HVIRT;
>>>           }
>>>       }
>>> @@ -1727,13 +1738,13 @@ static int p9_next_unmasked_interrupt(CPUPPCState 
>>> *env)
>>>           bool lpes0 = !!(env->spr[SPR_LPCR] & LPCR_LPES0);
>>>           bool heic = !!(env->spr[SPR_LPCR] & LPCR_HEIC);
>>>           /* HEIC blocks delivery to the hypervisor */
>>> -        if ((async_deliver && !(heic && FIELD_EX64_HV(env->msr) &&
>>> +        if ((msr_ee && !(heic && FIELD_EX64_HV(env->msr) &&
>>>               !FIELD_EX64(env->msr, MSR, PR))) ||
>>>               (env->has_hv_mode && !FIELD_EX64_HV(env->msr) && !lpes0)) {
>>>               return PPC_INTERRUPT_EXT;
>>>           }
>>>       }
>>> -    if (async_deliver != 0) {
>>> +    if (msr_ee != 0) {
>>>           /* Decrementer exception */
>>>           if (env->pending_interrupts & PPC_INTERRUPT_DECR) {
>>>               return PPC_INTERRUPT_DECR;
>>> @@ -1895,6 +1906,15 @@ static void p9_deliver_interrupt(CPUPPCState *env, 
>>> int interrupt)
>>>       PowerPCCPU *cpu = env_archcpu(env);
>>>       CPUState *cs = env_cpu(env);
>>>
>>> +    if (cs->halted && !(env->spr[SPR_PSSCR] & PSSCR_EC) &&
>>> +        !FIELD_EX64(env->msr, MSR, EE)) {
>>> +        /*
>>> +         * A pending interrupt took us out of power-saving, but MSR[EE] 
>>> says
>>> +         * that we should return to NIP+4 instead of delivering it.
>>> +         */
>>> +        return;
>> 
>> How will the NIP be advanced in this case?
>> 
>
> It's already incremented by the translation code. ppc_tr_translate_insn 
> increments ctx->base.pc_next before calling decode_{insn{32,64},legacy}, 
> and methods that put the CPU to sleep will use gen_exception_nip with 
> this value as the last argument.
>
>>> +    }
>>> +
>>>       switch (interrupt) {
>>>       case PPC_INTERRUPT_MCK: /* Machine check exception */
>>>           env->pending_interrupts &= ~PPC_INTERRUPT_MCK;
>>> diff --git a/target/ppc/internal.h b/target/ppc/internal.h
>>> index 337a362205..41e79adfdb 100644
>>> --- a/target/ppc/internal.h
>>> +++ b/target/ppc/internal.h
>>> @@ -306,4 +306,8 @@ static inline int ger_pack_masks(int pmsk, int ymsk, 
>>> int xmsk)
>>>       return msk;
>>>   }
>>>
>>> +#if defined(TARGET_PPC64)
>>> +int p9_interrupt_powersave(CPUPPCState *env);
>>> +#endif
>>> +
>>>   #endif /* PPC_INTERNAL_H */
>
> 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>