* [PATCH v10 0/3] PMU-EBB support for PPC64 TCG
@ 2022-02-08 19:48 Daniel Henrique Barboza
2022-02-08 19:48 ` [PATCH v10 1/3] target/ppc: fix indent of function parameters Daniel Henrique Barboza
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Daniel Henrique Barboza @ 2022-02-08 19:48 UTC (permalink / raw)
To: qemu-devel; +Cc: Daniel Henrique Barboza, qemu-ppc, clg, david
Hi,
This small series finalizes the pending PMU-EBB support for PPC64 TCG.
In theory this would be a re-send of patches 09 and 10 of the v9, but
those patches were so off the mark with the recent exception changes
that I ended up discarding them and doing from the start.
Patch 1 is a trivial indent fix. I've read a lot of code in excp_helper.c
and this mis-indent was being annoying.
Patch 2 is the last bits of the PMU specific code before triggering
EBBs. I chose to do it separately to not mix PMU specific code and
EBB logic.
Patch 3 is the re-implementation of what patches 09+10 of the v9 were
doing back then. The most significant change is that we're now gating
the exception before reaching powerpc_excp_books().
v9 link: https://lists.gnu.org/archive/html/qemu-devel/2021-12/msg00073.html
Daniel Henrique Barboza (3):
target/ppc: fix indent of function parameters
target/ppc: finalize pre-EBB PMU logic
target/ppc: EBB exception implementation
target/ppc/excp_helper.c | 59 +++++++++++++++++++++++++++++++++++-----
target/ppc/helper.h | 1 +
target/ppc/power8-pmu.c | 48 +++++++++++++++++++++++++++++---
3 files changed, 97 insertions(+), 11 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v10 1/3] target/ppc: fix indent of function parameters
2022-02-08 19:48 [PATCH v10 0/3] PMU-EBB support for PPC64 TCG Daniel Henrique Barboza
@ 2022-02-08 19:48 ` Daniel Henrique Barboza
2022-02-10 2:20 ` David Gibson
2022-02-08 19:48 ` [PATCH v10 2/3] target/ppc: finalize pre-EBB PMU logic Daniel Henrique Barboza
2022-02-08 19:48 ` [PATCH v10 3/3] target/ppc: EBB exception implementation Daniel Henrique Barboza
2 siblings, 1 reply; 7+ messages in thread
From: Daniel Henrique Barboza @ 2022-02-08 19:48 UTC (permalink / raw)
To: qemu-devel; +Cc: Daniel Henrique Barboza, qemu-ppc, clg, david
Fix indentation of powerpc_set_excp_state() and ppc_excp_apply_ail()
parameters.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
target/ppc/excp_helper.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index c107953dec..8a49a4ab90 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -265,9 +265,9 @@ static int powerpc_reset_wakeup(CPUState *cs, CPUPPCState *env, int excp,
* +--------------------------------------------------------------------+
*/
static void ppc_excp_apply_ail(PowerPCCPU *cpu, int excp_model, int excp,
- target_ulong msr,
- target_ulong *new_msr,
- target_ulong *vector)
+ target_ulong msr,
+ target_ulong *new_msr,
+ target_ulong *vector)
{
#if defined(TARGET_PPC64)
CPUPPCState *env = &cpu->env;
@@ -362,7 +362,7 @@ static void ppc_excp_apply_ail(PowerPCCPU *cpu, int excp_model, int excp,
}
static void powerpc_set_excp_state(PowerPCCPU *cpu,
- target_ulong vector, target_ulong msr)
+ target_ulong vector, target_ulong msr)
{
CPUState *cs = CPU(cpu);
CPUPPCState *env = &cpu->env;
--
2.34.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v10 2/3] target/ppc: finalize pre-EBB PMU logic
2022-02-08 19:48 [PATCH v10 0/3] PMU-EBB support for PPC64 TCG Daniel Henrique Barboza
2022-02-08 19:48 ` [PATCH v10 1/3] target/ppc: fix indent of function parameters Daniel Henrique Barboza
@ 2022-02-08 19:48 ` Daniel Henrique Barboza
2022-02-08 19:48 ` [PATCH v10 3/3] target/ppc: EBB exception implementation Daniel Henrique Barboza
2 siblings, 0 replies; 7+ messages in thread
From: Daniel Henrique Barboza @ 2022-02-08 19:48 UTC (permalink / raw)
To: qemu-devel; +Cc: Daniel Henrique Barboza, qemu-ppc, clg, david
There are still PMU exclusive bits to handle in fire_PMC_interrupt()
before implementing the EBB support. Let's finalize it now to avoid
dealing with PMU and EBB logic at the same time in the next patches.
fire_PMC_interrupt() will fire an Performance Monitor alert depending on
MMCR0_PMAE. If we are required to freeze the timers (MMCR0_FCECE) we'll
also need to update summaries and delete the existing overflow timers.
In all cases we're going to update the cycle counters.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
target/ppc/power8-pmu.c | 36 ++++++++++++++++++++++++++++++++++--
1 file changed, 34 insertions(+), 2 deletions(-)
diff --git a/target/ppc/power8-pmu.c b/target/ppc/power8-pmu.c
index 236e8e66e9..d245663158 100644
--- a/target/ppc/power8-pmu.c
+++ b/target/ppc/power8-pmu.c
@@ -222,6 +222,20 @@ static void pmu_update_overflow_timers(CPUPPCState *env)
}
}
+static void pmu_delete_timers(CPUPPCState *env)
+{
+ QEMUTimer *pmc_overflow_timer;
+ int sprn;
+
+ for (sprn = SPR_POWER_PMC1; sprn <= SPR_POWER_PMC6; sprn++) {
+ pmc_overflow_timer = get_cyc_overflow_timer(env, sprn);
+
+ if (pmc_overflow_timer) {
+ timer_del(pmc_overflow_timer);
+ }
+ }
+}
+
void helper_store_mmcr0(CPUPPCState *env, target_ulong value)
{
bool hflags_pmcc0 = (value & MMCR0_PMCC0) != 0;
@@ -271,8 +285,26 @@ static void fire_PMC_interrupt(PowerPCCPU *cpu)
{
CPUPPCState *env = &cpu->env;
- if (!(env->spr[SPR_POWER_MMCR0] & MMCR0_EBE)) {
- return;
+ pmu_update_cycles(env);
+
+ if (env->spr[SPR_POWER_MMCR0] & MMCR0_FCECE) {
+ env->spr[SPR_POWER_MMCR0] &= ~MMCR0_FCECE;
+ env->spr[SPR_POWER_MMCR0] |= MMCR0_FC;
+
+ /* Changing MMCR0_FC requires a new HFLAGS_INSN_CNT calc */
+ pmu_update_summaries(env);
+
+ /*
+ * Delete all pending timers if we need to freeze
+ * the PMC. We'll restart them when the PMC starts
+ * running again.
+ */
+ pmu_delete_timers(env);
+ }
+
+ if (env->spr[SPR_POWER_MMCR0] & MMCR0_PMAE) {
+ env->spr[SPR_POWER_MMCR0] &= ~MMCR0_PMAE;
+ env->spr[SPR_POWER_MMCR0] |= MMCR0_PMAO;
}
/* PMC interrupt not implemented yet */
--
2.34.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v10 3/3] target/ppc: EBB exception implementation
2022-02-08 19:48 [PATCH v10 0/3] PMU-EBB support for PPC64 TCG Daniel Henrique Barboza
2022-02-08 19:48 ` [PATCH v10 1/3] target/ppc: fix indent of function parameters Daniel Henrique Barboza
2022-02-08 19:48 ` [PATCH v10 2/3] target/ppc: finalize pre-EBB PMU logic Daniel Henrique Barboza
@ 2022-02-08 19:48 ` Daniel Henrique Barboza
2022-02-08 23:20 ` Fabiano Rosas
2 siblings, 1 reply; 7+ messages in thread
From: Daniel Henrique Barboza @ 2022-02-08 19:48 UTC (permalink / raw)
To: qemu-devel; +Cc: Daniel Henrique Barboza, qemu-ppc, clg, david
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>
---
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 */
+ 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);
+ }
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. */
--
2.34.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v10 3/3] target/ppc: EBB exception implementation
2022-02-08 19:48 ` [PATCH v10 3/3] target/ppc: EBB exception implementation Daniel Henrique Barboza
@ 2022-02-08 23:20 ` Fabiano Rosas
2022-02-09 12:40 ` Daniel Henrique Barboza
0 siblings, 1 reply; 7+ messages in thread
From: Fabiano Rosas @ 2022-02-08 23:20 UTC (permalink / raw)
To: Daniel Henrique Barboza, qemu-devel
Cc: Daniel Henrique Barboza, qemu-ppc, clg, david
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.
> + 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?
> 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. */
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v10 3/3] target/ppc: EBB exception implementation
2022-02-08 23:20 ` Fabiano Rosas
@ 2022-02-09 12:40 ` Daniel Henrique Barboza
0 siblings, 0 replies; 7+ messages in thread
From: Daniel Henrique Barboza @ 2022-02-09 12:40 UTC (permalink / raw)
To: Fabiano Rosas, qemu-devel; +Cc: qemu-ppc, clg, david
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. */
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v10 1/3] target/ppc: fix indent of function parameters
2022-02-08 19:48 ` [PATCH v10 1/3] target/ppc: fix indent of function parameters Daniel Henrique Barboza
@ 2022-02-10 2:20 ` David Gibson
0 siblings, 0 replies; 7+ messages in thread
From: David Gibson @ 2022-02-10 2:20 UTC (permalink / raw)
To: Daniel Henrique Barboza; +Cc: qemu-ppc, qemu-devel, clg
[-- Attachment #1: Type: text/plain, Size: 1864 bytes --]
On Tue, Feb 08, 2022 at 04:48:36PM -0300, Daniel Henrique Barboza wrote:
> Fix indentation of powerpc_set_excp_state() and ppc_excp_apply_ail()
> parameters.
>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> target/ppc/excp_helper.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index c107953dec..8a49a4ab90 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -265,9 +265,9 @@ static int powerpc_reset_wakeup(CPUState *cs, CPUPPCState *env, int excp,
> * +--------------------------------------------------------------------+
> */
> static void ppc_excp_apply_ail(PowerPCCPU *cpu, int excp_model, int excp,
> - target_ulong msr,
> - target_ulong *new_msr,
> - target_ulong *vector)
> + target_ulong msr,
> + target_ulong *new_msr,
> + target_ulong *vector)
> {
> #if defined(TARGET_PPC64)
> CPUPPCState *env = &cpu->env;
> @@ -362,7 +362,7 @@ static void ppc_excp_apply_ail(PowerPCCPU *cpu, int excp_model, int excp,
> }
>
> static void powerpc_set_excp_state(PowerPCCPU *cpu,
> - target_ulong vector, target_ulong msr)
> + target_ulong vector, target_ulong msr)
> {
> CPUState *cs = CPU(cpu);
> CPUPPCState *env = &cpu->env;
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-02-10 4:27 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-08 19:48 [PATCH v10 0/3] PMU-EBB support for PPC64 TCG Daniel Henrique Barboza
2022-02-08 19:48 ` [PATCH v10 1/3] target/ppc: fix indent of function parameters Daniel Henrique Barboza
2022-02-10 2:20 ` David Gibson
2022-02-08 19:48 ` [PATCH v10 2/3] target/ppc: finalize pre-EBB PMU logic Daniel Henrique Barboza
2022-02-08 19:48 ` [PATCH v10 3/3] target/ppc: EBB exception implementation Daniel Henrique Barboza
2022-02-08 23:20 ` Fabiano Rosas
2022-02-09 12:40 ` Daniel Henrique Barboza
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.