On 2/8/22 20:20, Fabiano Rosas wrote:
Daniel Henrique Barboza <danielhb...@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 <danielhb...@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. */
