Re: [PATCH v10 3/3] target/ppc: EBB exception implementation

[Daniel Henrique Barboza]

On 2/8/22 20:20, Fabiano Rosas wrote:
> Daniel Henrique Barboza <danielhb413@gmail.com> writes:
>
> > This patch adds the EBB exception support that are triggered by
> > Performance Monitor alerts. This happens when a Performance Monitor
> > alert occurs and MMCR0_EBE, BESCR_PME and BESCR_GE are set.
> >
> > A 'ebb_excp_enabled' helper is called at the end of fire_PMC_interrupt()
> > to fire the EBB exception, checking for FSCR and HFSCR support
> > beforehand.
> >
> > In ppc_hw_interrupt() the generated EBB exception will be taken only if
> > running in problem state and with BESCR_GE set. The check for BESCR_GE
> > bit in this step is needed to avoid race conditions where we take an
> > EBB, while the previous EBB is still inflight (BESCR_GE cleared), and
> > SPR_EBBHR is not set yet. In this case we'll branch to env->nip = 0 and the
> > guest will crash. The Linux kernel selftest 'lost_exception_test' is an
> > example where this racing will occur.
> >
> > The code in powerpc_excp_books() is the default EBB handling described
> > in the PowerISA v3.1: clear BESCR_GE, set BESCR_PMEO, save env->nip in
> > SPR_EBBRR and redirect the execution to the address pointed by
> > SPR_EBBHR. The already implemented 'rbebb' instruction is then able to
> > return from the EBB by retrieving the NIP in SPR_EBBRR.
> >
> > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>
> Now that BookS code is separate from the other CPUs, let me leave this
> here:
>
> Why do we have "interrupts" before "exceptions"? As in ppc_hw_interrupt
>   calling powerpc_excp.
>
> If anyone has a consistent mental model on how this that they could
> share I'd appreciate it.
>
> Now onto the patch:
> > ---
> >   target/ppc/excp_helper.c | 51 +++++++++++++++++++++++++++++++++++++---
> >   target/ppc/helper.h      |  1 +
> >   target/ppc/power8-pmu.c  | 12 ++++++++--
> >   3 files changed, 59 insertions(+), 5 deletions(-)
> >
> > diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> > index 8a49a4ab90..2a95cec39e 100644
> > --- a/target/ppc/excp_helper.c
> > +++ b/target/ppc/excp_helper.c
> > @@ -19,6 +19,7 @@
> >   #include "qemu/osdep.h"
> >   #include "qemu/main-loop.h"
> >   #include "cpu.h"
> > +#include "hw/ppc/ppc.h"
> >   #include "exec/exec-all.h"
> >   #include "internal.h"
> >   #include "helper_regs.h"
> > @@ -990,8 +991,22 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
> >           new_msr |= (target_ulong)MSR_HVB;
> >           new_msr |= env->msr & ((target_ulong)1 << MSR_RI);
> >           break;
> > -    case POWERPC_EXCP_THERM:     /* Thermal interrupt                        */
> >       case POWERPC_EXCP_PERFM:     /* Embedded performance monitor interrupt   
> > */
>
> We would need some way to tell apart EBB from other Performance Monitor
> interrupts here. Unless you want to leave that to the next person that
> looks at Performance Monitor interrupts.

I don't mind doing that, but bear in mind that EBB isn't an official 
interrupt/exception
vector in the ISA.

> > +        env->spr[SPR_BESCR] &= ~BESCR_GE;
> > +        env->spr[SPR_BESCR] |= BESCR_PMEO;
> > +
> > +        /*
> > +         * Save NIP for rfebb insn in SPR_EBBRR. Next nip is
> > +         * stored in the EBB Handler SPR_EBBHR.
> > +         */
> > +        env->spr[SPR_EBBRR] = env->nip;
> > +        powerpc_set_excp_state(cpu, env->spr[SPR_EBBHR], env->msr);
> > +
> > +        /*
> > +         * This exception is handled in userspace. No need to proceed.
> > +         */
> > +        return;
> > +    case POWERPC_EXCP_THERM:     /* Thermal interrupt                        */
> >       case POWERPC_EXCP_VPUA:      /* Vector assist exception                  
> > */
> >       case POWERPC_EXCP_MAINT:     /* Maintenance exception                    
> > */
> >       case POWERPC_EXCP_SDOOR:     /* Doorbell interrupt                       
> > */
> > @@ -1671,8 +1686,14 @@ static void ppc_hw_interrupt(CPUPPCState *env)
> >               return;
> >           }
> >           if (env->pending_interrupts & (1 << PPC_INTERRUPT_PERFM)) {
> > -            env->pending_interrupts &= ~(1 << PPC_INTERRUPT_PERFM);
> > -            powerpc_excp(cpu, POWERPC_EXCP_PERFM);
> > +            /*
> > +             * PERFM EBB must be taken in problem state and
> > +             * with BESCR_GE set.
> > +             */
> > +            if (msr_pr == 1 && env->spr[SPR_BESCR] & BESCR_GE) {
> > +                env->pending_interrupts &= ~(1 << PPC_INTERRUPT_PERFM);
> > +                powerpc_excp(cpu, POWERPC_EXCP_PERFM);
> > +            }
>
> This is masking other Performance Interrupts (for all CPUs). Can we move
> these checks into the helper?

Probably. I'll try it out.


Daniel

> >               return;
> >           }
> >           /* Thermal interrupt */
> > @@ -1915,6 +1936,30 @@ void helper_rfebb(CPUPPCState *env, target_ulong s)
> >           env->spr[SPR_BESCR] &= ~BESCR_GE;
> >       }
> >   }
> > +
> > +void helper_ebb_perfm_int(CPUPPCState *env)
> > +{
> > +    PowerPCCPU *cpu = env_archcpu(env);
> > +
> > +    /*
> > +     * FSCR_EBB and FSCR_IC_EBB are the same bits used with
> > +     * HFSCR.
> > +     */
> > +    helper_fscr_facility_check(env, FSCR_EBB, 0, FSCR_IC_EBB);
> > +    helper_hfscr_facility_check(env, FSCR_EBB, "EBB", FSCR_IC_EBB);
> > +
> > +    /*
> > +     * Setting "env->pending_interrupts |= 1 << PPC_INTERRUPT_PERFM"
> > +     * instead of calling "ppc_set_irq()"" works in most cases, but under
> > +     * certain race conditions (e.g. lost_exception_test EBB kernel
> > +     * selftest) this hits an assert when dealing with the BQL:
> > +     *
> > +     * tcg_handle_interrupt: assertion failed: (qemu_mutex_iothread_locked())
> > +     *
> > +     * We ended up using ppc_set_irq() because it handles the BQL.
> > +     */
> > +    ppc_set_irq(cpu, PPC_INTERRUPT_PERFM, 1);
> > +}
> >   #endif
> >   
> >   /*****************************************************************************/
> > diff --git a/target/ppc/helper.h b/target/ppc/helper.h
> > index f2e5060910..bb26da6176 100644
> > --- a/target/ppc/helper.h
> > +++ b/target/ppc/helper.h
> > @@ -19,6 +19,7 @@ DEF_HELPER_1(rfid, void, env)
> >   DEF_HELPER_1(rfscv, void, env)
> >   DEF_HELPER_1(hrfid, void, env)
> >   DEF_HELPER_2(rfebb, void, env, tl)
> > +DEF_HELPER_1(ebb_perfm_int, void, env)
> >   DEF_HELPER_2(store_lpcr, void, env, tl)
> >   DEF_HELPER_2(store_pcr, void, env, tl)
> >   DEF_HELPER_2(store_mmcr0, void, env, tl)
> > diff --git a/target/ppc/power8-pmu.c b/target/ppc/power8-pmu.c
> > index d245663158..41409e609f 100644
> > --- a/target/ppc/power8-pmu.c
> > +++ b/target/ppc/power8-pmu.c
> > @@ -281,6 +281,13 @@ void helper_store_pmc(CPUPPCState *env, uint32_t sprn, 
> > uint64_t value)
> >       pmc_update_overflow_timer(env, sprn);
> >   }
> >   
> > +static bool ebb_excp_enabled(CPUPPCState *env)
> > +{
> > +    return env->spr[SPR_POWER_MMCR0] & MMCR0_EBE &&
> > +           env->spr[SPR_BESCR] & BESCR_PME &&
> > +           env->spr[SPR_BESCR] & BESCR_GE;
> > +}
> > +
> >   static void fire_PMC_interrupt(PowerPCCPU *cpu)
> >   {
> >       CPUPPCState *env = &cpu->env;
> > @@ -307,8 +314,9 @@ static void fire_PMC_interrupt(PowerPCCPU *cpu)
> >           env->spr[SPR_POWER_MMCR0] |= MMCR0_PMAO;
> >       }
> >   
> > -    /* PMC interrupt not implemented yet */
> > -    return;
> > +    if (ebb_excp_enabled(env)) {
> > +        helper_ebb_perfm_int(env);
> > +    }
> >   }
> >   
> >   /* This helper assumes that the PMC is running. */