* [PATCH v8 01/10] target/ppc: introduce PMUEventType and PMU overflow timers
2021-11-25 15:08 [PATCH v8 00/10] PMU-EBB support for PPC64 TCG Daniel Henrique Barboza
@ 2021-11-25 15:08 ` Daniel Henrique Barboza
2021-11-25 15:08 ` [PATCH v8 02/10] target/ppc: PMU basic cycle count for pseries TCG Daniel Henrique Barboza
` (8 subsequent siblings)
9 siblings, 0 replies; 24+ messages in thread
From: Daniel Henrique Barboza @ 2021-11-25 15:08 UTC (permalink / raw)
To: qemu-devel
Cc: richard.henderson, Daniel Henrique Barboza, qemu-ppc, clg, david
This patch starts an IBM Power8+ compatible PMU implementation by adding
the representation of PMU events that we are going to sample,
PMUEventType. This enum represents a Perf event that is being sampled by
a specific counter 'sprn'. Events that aren't available (i.e. no event
was set in MMCR1) will be of type 'PMU_EVENT_INVALID'. Events that are
inactive due to frozen counter bits state are of type
'PMU_EVENT_INACTIVE'. Other types added in this patch are
PMU_EVENT_CYCLES and PMU_EVENT_INSTRUCTIONS. More types will be added
later on.
Let's also add the required PMU cycle overflow timers. They will be used
to trigger cycle overflows when cycle events are being sampled. This
timer will call cpu_ppc_pmu_timer_cb(), which in turn calls
fire_PMC_interrupt(). Both functions are stubs that will be implemented
later on when EBB support is added.
Two new helper files are created to host this new logic.
cpu_ppc_pmu_init() will init all overflow timers during CPU init time.
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
hw/ppc/spapr_cpu_core.c | 1 +
target/ppc/cpu.h | 16 ++++++++++++
target/ppc/cpu_init.c | 24 ++++++++++++++++++
target/ppc/meson.build | 1 +
target/ppc/power8-pmu.c | 56 +++++++++++++++++++++++++++++++++++++++++
target/ppc/power8-pmu.h | 25 ++++++++++++++++++
6 files changed, 123 insertions(+)
create mode 100644 target/ppc/power8-pmu.c
create mode 100644 target/ppc/power8-pmu.h
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 58e7341cb7..a57ba70a87 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -20,6 +20,7 @@
#include "target/ppc/kvm_ppc.h"
#include "hw/ppc/ppc.h"
#include "target/ppc/mmu-hash64.h"
+#include "target/ppc/power8-pmu.h"
#include "sysemu/numa.h"
#include "sysemu/reset.h"
#include "sysemu/hw_accel.h"
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index e946da5f3a..2ad47b06d0 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -296,6 +296,16 @@ typedef struct ppc_v3_pate_t {
uint64_t dw1;
} ppc_v3_pate_t;
+/* PMU related structs and defines */
+#define PMU_COUNTERS_NUM 6
+#define PMU_TIMERS_NUM (PMU_COUNTERS_NUM - 1) /* PMC5 doesn't count cycles */
+typedef enum {
+ PMU_EVENT_INVALID = 0,
+ PMU_EVENT_INACTIVE,
+ PMU_EVENT_CYCLES,
+ PMU_EVENT_INSTRUCTIONS,
+} PMUEventType;
+
/*****************************************************************************/
/* Machine state register bits definition */
#define MSR_SF 63 /* Sixty-four-bit mode hflags */
@@ -1191,6 +1201,12 @@ struct CPUPPCState {
uint32_t tm_vscr;
uint64_t tm_dscr;
uint64_t tm_tar;
+
+ /*
+ * Timers used to fire performance monitor alerts
+ * when counting cycles.
+ */
+ QEMUTimer *pmu_cyc_overflow_timers[PMU_TIMERS_NUM];
};
#define SET_FIT_PERIOD(a_, b_, c_, d_) \
diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
index 6695985e9b..9610e65c76 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -45,6 +45,7 @@
#include "helper_regs.h"
#include "internal.h"
#include "spr_tcg.h"
+#include "power8-pmu.h"
/* #define PPC_DEBUG_SPR */
/* #define USE_APPLE_GDB */
@@ -7377,6 +7378,20 @@ static void register_power9_mmu_sprs(CPUPPCState *env)
#endif
}
+/*
+ * Initialize PMU counter overflow timers for Power8 and
+ * newer Power chips when using TCG.
+ */
+static void init_tcg_pmu_power8(CPUPPCState *env)
+{
+#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
+ /* Init PMU overflow timers */
+ if (!kvm_enabled()) {
+ cpu_ppc_pmu_init(env);
+ }
+#endif
+}
+
static void init_proc_book3s_common(CPUPPCState *env)
{
register_ne_601_sprs(env);
@@ -7694,6 +7709,9 @@ static void init_proc_POWER8(CPUPPCState *env)
register_sdr1_sprs(env);
register_book3s_207_dbg_sprs(env);
+ /* Common TCG PMU */
+ init_tcg_pmu_power8(env);
+
/* POWER8 Specific Registers */
register_book3s_ids_sprs(env);
register_rmor_sprs(env);
@@ -7888,6 +7906,9 @@ static void init_proc_POWER9(CPUPPCState *env)
init_proc_book3s_common(env);
register_book3s_207_dbg_sprs(env);
+ /* Common TCG PMU */
+ init_tcg_pmu_power8(env);
+
/* POWER8 Specific Registers */
register_book3s_ids_sprs(env);
register_amr_sprs(env);
@@ -8104,6 +8125,9 @@ static void init_proc_POWER10(CPUPPCState *env)
init_proc_book3s_common(env);
register_book3s_207_dbg_sprs(env);
+ /* Common TCG PMU */
+ init_tcg_pmu_power8(env);
+
/* POWER8 Specific Registers */
register_book3s_ids_sprs(env);
register_amr_sprs(env);
diff --git a/target/ppc/meson.build b/target/ppc/meson.build
index b85f295703..a49a8911e0 100644
--- a/target/ppc/meson.build
+++ b/target/ppc/meson.build
@@ -51,6 +51,7 @@ ppc_softmmu_ss.add(when: 'TARGET_PPC64', if_true: files(
'mmu-book3s-v3.c',
'mmu-hash64.c',
'mmu-radix64.c',
+ 'power8-pmu.c',
))
target_arch += {'ppc': ppc_ss}
diff --git a/target/ppc/power8-pmu.c b/target/ppc/power8-pmu.c
new file mode 100644
index 0000000000..e66c152eb5
--- /dev/null
+++ b/target/ppc/power8-pmu.c
@@ -0,0 +1,56 @@
+/*
+ * PMU emulation helpers for TCG IBM POWER chips
+ *
+ * Copyright IBM Corp. 2021
+ *
+ * Authors:
+ * Daniel Henrique Barboza <danielhb413@gmail.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+
+#include "power8-pmu.h"
+#include "cpu.h"
+#include "helper_regs.h"
+#include "exec/exec-all.h"
+#include "exec/helper-proto.h"
+#include "qemu/error-report.h"
+#include "qemu/main-loop.h"
+#include "hw/ppc/ppc.h"
+
+#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
+
+static void fire_PMC_interrupt(PowerPCCPU *cpu)
+{
+ CPUPPCState *env = &cpu->env;
+
+ if (!(env->spr[SPR_POWER_MMCR0] & MMCR0_EBE)) {
+ return;
+ }
+
+ /* PMC interrupt not implemented yet */
+ return;
+}
+
+static void cpu_ppc_pmu_timer_cb(void *opaque)
+{
+ PowerPCCPU *cpu = opaque;
+
+ fire_PMC_interrupt(cpu);
+}
+
+void cpu_ppc_pmu_init(CPUPPCState *env)
+{
+ PowerPCCPU *cpu = env_archcpu(env);
+ int i;
+
+ for (i = 0; i < PMU_TIMERS_NUM; i++) {
+ env->pmu_cyc_overflow_timers[i] = timer_new_ns(QEMU_CLOCK_VIRTUAL,
+ &cpu_ppc_pmu_timer_cb,
+ cpu);
+ }
+}
+#endif /* defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY) */
diff --git a/target/ppc/power8-pmu.h b/target/ppc/power8-pmu.h
new file mode 100644
index 0000000000..49a813a443
--- /dev/null
+++ b/target/ppc/power8-pmu.h
@@ -0,0 +1,25 @@
+/*
+ * PMU emulation helpers for TCG IBM POWER chips
+ *
+ * Copyright IBM Corp. 2021
+ *
+ * Authors:
+ * Daniel Henrique Barboza <danielhb413@gmail.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef POWER8_PMU
+#define POWER8_PMU
+
+#include "qemu/osdep.h"
+#include "cpu.h"
+#include "exec/exec-all.h"
+#include "exec/helper-proto.h"
+#include "qemu/error-report.h"
+#include "qemu/main-loop.h"
+
+void cpu_ppc_pmu_init(CPUPPCState *env);
+
+#endif
--
2.31.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v8 02/10] target/ppc: PMU basic cycle count for pseries TCG
2021-11-25 15:08 [PATCH v8 00/10] PMU-EBB support for PPC64 TCG Daniel Henrique Barboza
2021-11-25 15:08 ` [PATCH v8 01/10] target/ppc: introduce PMUEventType and PMU overflow timers Daniel Henrique Barboza
@ 2021-11-25 15:08 ` Daniel Henrique Barboza
2021-11-26 2:17 ` David Gibson
2021-11-25 15:08 ` [PATCH v8 03/10] target/ppc: PMU: update counters on PMCs r/w Daniel Henrique Barboza
` (7 subsequent siblings)
9 siblings, 1 reply; 24+ messages in thread
From: Daniel Henrique Barboza @ 2021-11-25 15:08 UTC (permalink / raw)
To: qemu-devel
Cc: richard.henderson, Daniel Henrique Barboza, qemu-ppc, clg, david
This patch adds the barebones of the PMU logic by enabling cycle
counting. The overall logic goes as follows:
- MMCR0 reg initial value is set to 0x80000000 (MMCR0_FC set) to avoid
having to spin the PMU right at system init;
- to retrieve the events that are being profiled, pmc_get_event() will
check the current MMCR0 and MMCR1 value and return the appropriate
PMUEventType. For PMCs 1-4, event 0x2 is the implementation dependent
value of PMU_EVENT_INSTRUCTIONS and event 0x1E is the implementation
dependent value of PMU_EVENT_CYCLES. These events are supported by IBM
Power chips since Power8, at least, and the Linux Perf driver makes use
of these events until kernel v5.15. For PMC1, event 0xF0 is the
architected PowerISA event for cycles. Event 0xFE is the architected
PowerISA event for instructions;
- if the counter is frozen, either via the global MMCR0_FC bit or its
individual frozen counter bit, PMU_EVENT_INACTIVE is returned;
- pmu_update_cycles() will go through each counter and update the
values of all PMCs that are counting cycles. This function will be
called every time a MMCR0 update is done to keep counters values
up to date. Upcoming patches will use this function to allow the
counters to be properly updated during read/write of the PMCs
and MMCR1 writes.
Given that the base CPU frequency is fixed at 1Ghz for both powernv and
pseries clock, cycle calculation assumes that 1 nanosecond equals 1 CPU
cycle. Cycle value is then calculated by adding the elapsed time, in
nanoseconds, of the last cycle update done via pmu_update_cycles().
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
target/ppc/cpu.h | 20 +++++
target/ppc/cpu_init.c | 6 +-
target/ppc/helper.h | 1 +
target/ppc/power8-pmu-regs.c.inc | 23 +++++-
target/ppc/power8-pmu.c | 122 +++++++++++++++++++++++++++++++
target/ppc/spr_tcg.h | 1 +
6 files changed, 169 insertions(+), 4 deletions(-)
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 2ad47b06d0..9c732953f0 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -361,6 +361,9 @@ typedef enum {
#define MMCR0_FCECE PPC_BIT(38) /* FC on Enabled Cond or Event */
#define MMCR0_PMCC0 PPC_BIT(44) /* PMC Control bit 0 */
#define MMCR0_PMCC1 PPC_BIT(45) /* PMC Control bit 1 */
+#define MMCR0_PMCC PPC_BITMASK(44, 45) /* PMC Control */
+#define MMCR0_FC14 PPC_BIT(58) /* PMC Freeze Counters 1-4 bit */
+#define MMCR0_FC56 PPC_BIT(59) /* PMC Freeze Counters 5-6 bit */
/* MMCR0 userspace r/w mask */
#define MMCR0_UREG_MASK (MMCR0_FC | MMCR0_PMAO | MMCR0_PMAE)
/* MMCR2 userspace r/w mask */
@@ -373,6 +376,17 @@ typedef enum {
#define MMCR2_UREG_MASK (MMCR2_FC1P0 | MMCR2_FC2P0 | MMCR2_FC3P0 | \
MMCR2_FC4P0 | MMCR2_FC5P0 | MMCR2_FC6P0)
+#define MMCR1_EVT_SIZE 8
+/* extract64() does a right shift before extracting */
+#define MMCR1_PMC1SEL_START 32
+#define MMCR1_PMC1EVT_EXTR (64 - MMCR1_PMC1SEL_START - MMCR1_EVT_SIZE)
+#define MMCR1_PMC2SEL_START 40
+#define MMCR1_PMC2EVT_EXTR (64 - MMCR1_PMC2SEL_START - MMCR1_EVT_SIZE)
+#define MMCR1_PMC3SEL_START 48
+#define MMCR1_PMC3EVT_EXTR (64 - MMCR1_PMC3SEL_START - MMCR1_EVT_SIZE)
+#define MMCR1_PMC4SEL_START 56
+#define MMCR1_PMC4EVT_EXTR (64 - MMCR1_PMC4SEL_START - MMCR1_EVT_SIZE)
+
/* LPCR bits */
#define LPCR_VPM0 PPC_BIT(0)
#define LPCR_VPM1 PPC_BIT(1)
@@ -1207,6 +1221,12 @@ struct CPUPPCState {
* when counting cycles.
*/
QEMUTimer *pmu_cyc_overflow_timers[PMU_TIMERS_NUM];
+
+ /*
+ * PMU base time value used by the PMU to calculate
+ * running cycles.
+ */
+ uint64_t pmu_base_time;
};
#define SET_FIT_PERIOD(a_, b_, c_, d_) \
diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
index 9610e65c76..e0b6fe4057 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -6821,8 +6821,8 @@ static void register_book3s_pmu_sup_sprs(CPUPPCState *env)
{
spr_register_kvm(env, SPR_POWER_MMCR0, "MMCR0",
SPR_NOACCESS, SPR_NOACCESS,
- &spr_read_generic, &spr_write_generic,
- KVM_REG_PPC_MMCR0, 0x00000000);
+ &spr_read_generic, &spr_write_MMCR0,
+ KVM_REG_PPC_MMCR0, 0x80000000);
spr_register_kvm(env, SPR_POWER_MMCR1, "MMCR1",
SPR_NOACCESS, SPR_NOACCESS,
&spr_read_generic, &spr_write_generic,
@@ -6870,7 +6870,7 @@ static void register_book3s_pmu_user_sprs(CPUPPCState *env)
spr_register(env, SPR_POWER_UMMCR0, "UMMCR0",
&spr_read_MMCR0_ureg, &spr_write_MMCR0_ureg,
&spr_read_ureg, &spr_write_ureg,
- 0x00000000);
+ 0x80000000);
spr_register(env, SPR_POWER_UMMCR1, "UMMCR1",
&spr_read_ureg, SPR_NOACCESS,
&spr_read_ureg, &spr_write_ureg,
diff --git a/target/ppc/helper.h b/target/ppc/helper.h
index 627811cefc..ea60a7493c 100644
--- a/target/ppc/helper.h
+++ b/target/ppc/helper.h
@@ -20,6 +20,7 @@ DEF_HELPER_1(rfscv, void, env)
DEF_HELPER_1(hrfid, 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)
#endif
DEF_HELPER_1(check_tlb_flush_local, void, env)
DEF_HELPER_1(check_tlb_flush_global, void, env)
diff --git a/target/ppc/power8-pmu-regs.c.inc b/target/ppc/power8-pmu-regs.c.inc
index 7391851238..fbb8977641 100644
--- a/target/ppc/power8-pmu-regs.c.inc
+++ b/target/ppc/power8-pmu-regs.c.inc
@@ -104,6 +104,17 @@ void spr_read_MMCR0_ureg(DisasContext *ctx, int gprn, int sprn)
tcg_temp_free(t0);
}
+static void write_MMCR0_common(DisasContext *ctx, TCGv val)
+{
+ /*
+ * helper_store_mmcr0 will make clock based operations that
+ * will cause 'bad icount read' errors if we do not execute
+ * gen_icount_io_start() beforehand.
+ */
+ gen_icount_io_start(ctx);
+ gen_helper_store_mmcr0(cpu_env, val);
+}
+
void spr_write_MMCR0_ureg(DisasContext *ctx, int sprn, int gprn)
{
TCGv masked_gprn;
@@ -119,7 +130,7 @@ void spr_write_MMCR0_ureg(DisasContext *ctx, int sprn, int gprn)
*/
masked_gprn = masked_gprn_for_spr_write(gprn, SPR_POWER_MMCR0,
MMCR0_UREG_MASK);
- gen_store_spr(SPR_POWER_MMCR0, masked_gprn);
+ write_MMCR0_common(ctx, masked_gprn);
tcg_temp_free(masked_gprn);
}
@@ -219,6 +230,11 @@ void spr_write_PMC56_ureg(DisasContext *ctx, int sprn, int gprn)
/* The remaining steps are similar to PMCs 1-4 userspace write */
spr_write_PMC14_ureg(ctx, sprn, gprn);
}
+
+void spr_write_MMCR0(DisasContext *ctx, int sprn, int gprn)
+{
+ write_MMCR0_common(ctx, cpu_gpr[gprn]);
+}
#else
void spr_read_MMCR0_ureg(DisasContext *ctx, int gprn, int sprn)
{
@@ -259,4 +275,9 @@ void spr_write_PMC56_ureg(DisasContext *ctx, int sprn, int gprn)
{
spr_noaccess(ctx, gprn, sprn);
}
+
+void spr_write_MMCR0(DisasContext *ctx, int sprn, int gprn)
+{
+ spr_write_generic(ctx, sprn, gprn);
+}
#endif /* defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY) */
diff --git a/target/ppc/power8-pmu.c b/target/ppc/power8-pmu.c
index e66c152eb5..47932ded4f 100644
--- a/target/ppc/power8-pmu.c
+++ b/target/ppc/power8-pmu.c
@@ -23,6 +23,128 @@
#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
+static bool pmc_is_inactive(CPUPPCState *env, int sprn)
+{
+ if (env->spr[SPR_POWER_MMCR0] & MMCR0_FC) {
+ return true;
+ }
+
+ if (sprn < SPR_POWER_PMC5) {
+ return env->spr[SPR_POWER_MMCR0] & MMCR0_FC14;
+ }
+
+ return env->spr[SPR_POWER_MMCR0] & MMCR0_FC56;
+}
+
+/*
+ * For PMCs 1-4, IBM POWER chips has support for an implementation
+ * dependent event, 0x1E, that enables cycle counting. The Linux kernel
+ * makes extensive use of 0x1E, so let's also support it.
+ *
+ * Likewise, event 0x2 is an implementation-dependent event that IBM
+ * POWER chips implement (at least since POWER8) that is equivalent to
+ * PM_INST_CMPL. Let's support this event on PMCs 1-4 as well.
+ */
+static PMUEventType pmc_get_event(CPUPPCState *env, int sprn)
+{
+ uint8_t mmcr1_evt_extr[] = { MMCR1_PMC1EVT_EXTR, MMCR1_PMC2EVT_EXTR,
+ MMCR1_PMC3EVT_EXTR, MMCR1_PMC4EVT_EXTR };
+ PMUEventType evt_type = PMU_EVENT_INVALID;
+ uint8_t pmcsel;
+ int i;
+
+ if (pmc_is_inactive(env, sprn)) {
+ return PMU_EVENT_INACTIVE;
+ }
+
+ if (sprn == SPR_POWER_PMC5) {
+ return PMU_EVENT_INSTRUCTIONS;
+ }
+
+ if (sprn == SPR_POWER_PMC6) {
+ return PMU_EVENT_CYCLES;
+ }
+
+ i = sprn - SPR_POWER_PMC1;
+ pmcsel = extract64(env->spr[SPR_POWER_MMCR1], mmcr1_evt_extr[i],
+ MMCR1_EVT_SIZE);
+
+ switch (pmcsel) {
+ case 0x2:
+ evt_type = PMU_EVENT_INSTRUCTIONS;
+ break;
+ case 0x1E:
+ evt_type = PMU_EVENT_CYCLES;
+ break;
+ case 0xF0:
+ /*
+ * PMC1SEL = 0xF0 is the architected PowerISA v3.1
+ * event that counts cycles using PMC1.
+ */
+ if (sprn == SPR_POWER_PMC1) {
+ evt_type = PMU_EVENT_CYCLES;
+ }
+ break;
+ case 0xFE:
+ /*
+ * PMC1SEL = 0xFE is the architected PowerISA v3.1
+ * event to sample instructions using PMC1.
+ */
+ if (sprn == SPR_POWER_PMC1) {
+ evt_type = PMU_EVENT_INSTRUCTIONS;
+ }
+ break;
+ default:
+ break;
+ }
+
+ return evt_type;
+}
+
+static void pmu_update_cycles(CPUPPCState *env)
+{
+ uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+ uint64_t time_delta = now - env->pmu_base_time;
+ int sprn;
+
+ for (sprn = SPR_POWER_PMC1; sprn <= SPR_POWER_PMC6; sprn++) {
+ if (pmc_get_event(env, sprn) != PMU_EVENT_CYCLES) {
+ continue;
+ }
+
+ /*
+ * The pseries and powernv clock runs at 1Ghz, meaning
+ * that 1 nanosec equals 1 cycle.
+ */
+ env->spr[sprn] += time_delta;
+ }
+
+ /* Update base_time for future calculations */
+ env->pmu_base_time = now;
+}
+
+void helper_store_mmcr0(CPUPPCState *env, target_ulong value)
+{
+ target_ulong curr_value = env->spr[SPR_POWER_MMCR0];
+ bool curr_FC = curr_value & MMCR0_FC;
+ bool new_FC = value & MMCR0_FC;
+
+ pmu_update_cycles(env);
+
+ env->spr[SPR_POWER_MMCR0] = value;
+
+ /*
+ * MMCR0 writes can change HFLAGS_PMCCCLEAR and HFLAGS_MMCR0FC.
+ * hreg_compute_hflags() does too much stuff to be called
+ * needlessly, so check beforehand if we really need a hflags
+ * update.
+ */
+ if (((curr_value & MMCR0_PMCC) != (value & MMCR0_PMCC)) ||
+ (curr_FC != new_FC)) {
+ hreg_compute_hflags(env);
+ }
+}
+
static void fire_PMC_interrupt(PowerPCCPU *cpu)
{
CPUPPCState *env = &cpu->env;
diff --git a/target/ppc/spr_tcg.h b/target/ppc/spr_tcg.h
index 520f1ef233..eb1d0c2bf0 100644
--- a/target/ppc/spr_tcg.h
+++ b/target/ppc/spr_tcg.h
@@ -25,6 +25,7 @@
void spr_noaccess(DisasContext *ctx, int gprn, int sprn);
void spr_read_generic(DisasContext *ctx, int gprn, int sprn);
void spr_write_generic(DisasContext *ctx, int sprn, int gprn);
+void spr_write_MMCR0(DisasContext *ctx, int sprn, int gprn);
void spr_read_xer(DisasContext *ctx, int gprn, int sprn);
void spr_write_xer(DisasContext *ctx, int sprn, int gprn);
void spr_read_lr(DisasContext *ctx, int gprn, int sprn);
--
2.31.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v8 02/10] target/ppc: PMU basic cycle count for pseries TCG
2021-11-25 15:08 ` [PATCH v8 02/10] target/ppc: PMU basic cycle count for pseries TCG Daniel Henrique Barboza
@ 2021-11-26 2:17 ` David Gibson
0 siblings, 0 replies; 24+ messages in thread
From: David Gibson @ 2021-11-26 2:17 UTC (permalink / raw)
To: Daniel Henrique Barboza; +Cc: richard.henderson, qemu-ppc, qemu-devel, clg
[-- Attachment #1: Type: text/plain, Size: 12859 bytes --]
On Thu, Nov 25, 2021 at 12:08:09PM -0300, Daniel Henrique Barboza wrote:
> This patch adds the barebones of the PMU logic by enabling cycle
> counting. The overall logic goes as follows:
>
> - MMCR0 reg initial value is set to 0x80000000 (MMCR0_FC set) to avoid
> having to spin the PMU right at system init;
>
> - to retrieve the events that are being profiled, pmc_get_event() will
> check the current MMCR0 and MMCR1 value and return the appropriate
> PMUEventType. For PMCs 1-4, event 0x2 is the implementation dependent
> value of PMU_EVENT_INSTRUCTIONS and event 0x1E is the implementation
> dependent value of PMU_EVENT_CYCLES. These events are supported by IBM
> Power chips since Power8, at least, and the Linux Perf driver makes use
> of these events until kernel v5.15. For PMC1, event 0xF0 is the
> architected PowerISA event for cycles. Event 0xFE is the architected
> PowerISA event for instructions;
>
> - if the counter is frozen, either via the global MMCR0_FC bit or its
> individual frozen counter bit, PMU_EVENT_INACTIVE is returned;
>
> - pmu_update_cycles() will go through each counter and update the
> values of all PMCs that are counting cycles. This function will be
> called every time a MMCR0 update is done to keep counters values
> up to date. Upcoming patches will use this function to allow the
> counters to be properly updated during read/write of the PMCs
> and MMCR1 writes.
>
> Given that the base CPU frequency is fixed at 1Ghz for both powernv and
> pseries clock, cycle calculation assumes that 1 nanosecond equals 1 CPU
> cycle. Cycle value is then calculated by adding the elapsed time, in
> nanoseconds, of the last cycle update done via pmu_update_cycles().
>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> target/ppc/cpu.h | 20 +++++
> target/ppc/cpu_init.c | 6 +-
> target/ppc/helper.h | 1 +
> target/ppc/power8-pmu-regs.c.inc | 23 +++++-
> target/ppc/power8-pmu.c | 122 +++++++++++++++++++++++++++++++
> target/ppc/spr_tcg.h | 1 +
> 6 files changed, 169 insertions(+), 4 deletions(-)
>
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 2ad47b06d0..9c732953f0 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -361,6 +361,9 @@ typedef enum {
> #define MMCR0_FCECE PPC_BIT(38) /* FC on Enabled Cond or Event */
> #define MMCR0_PMCC0 PPC_BIT(44) /* PMC Control bit 0 */
> #define MMCR0_PMCC1 PPC_BIT(45) /* PMC Control bit 1 */
> +#define MMCR0_PMCC PPC_BITMASK(44, 45) /* PMC Control */
> +#define MMCR0_FC14 PPC_BIT(58) /* PMC Freeze Counters 1-4 bit */
> +#define MMCR0_FC56 PPC_BIT(59) /* PMC Freeze Counters 5-6 bit */
> /* MMCR0 userspace r/w mask */
> #define MMCR0_UREG_MASK (MMCR0_FC | MMCR0_PMAO | MMCR0_PMAE)
> /* MMCR2 userspace r/w mask */
> @@ -373,6 +376,17 @@ typedef enum {
> #define MMCR2_UREG_MASK (MMCR2_FC1P0 | MMCR2_FC2P0 | MMCR2_FC3P0 | \
> MMCR2_FC4P0 | MMCR2_FC5P0 | MMCR2_FC6P0)
>
> +#define MMCR1_EVT_SIZE 8
> +/* extract64() does a right shift before extracting */
> +#define MMCR1_PMC1SEL_START 32
> +#define MMCR1_PMC1EVT_EXTR (64 - MMCR1_PMC1SEL_START - MMCR1_EVT_SIZE)
> +#define MMCR1_PMC2SEL_START 40
> +#define MMCR1_PMC2EVT_EXTR (64 - MMCR1_PMC2SEL_START - MMCR1_EVT_SIZE)
> +#define MMCR1_PMC3SEL_START 48
> +#define MMCR1_PMC3EVT_EXTR (64 - MMCR1_PMC3SEL_START - MMCR1_EVT_SIZE)
> +#define MMCR1_PMC4SEL_START 56
> +#define MMCR1_PMC4EVT_EXTR (64 - MMCR1_PMC4SEL_START - MMCR1_EVT_SIZE)
> +
> /* LPCR bits */
> #define LPCR_VPM0 PPC_BIT(0)
> #define LPCR_VPM1 PPC_BIT(1)
> @@ -1207,6 +1221,12 @@ struct CPUPPCState {
> * when counting cycles.
> */
> QEMUTimer *pmu_cyc_overflow_timers[PMU_TIMERS_NUM];
> +
> + /*
> + * PMU base time value used by the PMU to calculate
> + * running cycles.
> + */
> + uint64_t pmu_base_time;
> };
>
> #define SET_FIT_PERIOD(a_, b_, c_, d_) \
> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> index 9610e65c76..e0b6fe4057 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -6821,8 +6821,8 @@ static void register_book3s_pmu_sup_sprs(CPUPPCState *env)
> {
> spr_register_kvm(env, SPR_POWER_MMCR0, "MMCR0",
> SPR_NOACCESS, SPR_NOACCESS,
> - &spr_read_generic, &spr_write_generic,
> - KVM_REG_PPC_MMCR0, 0x00000000);
> + &spr_read_generic, &spr_write_MMCR0,
> + KVM_REG_PPC_MMCR0, 0x80000000);
> spr_register_kvm(env, SPR_POWER_MMCR1, "MMCR1",
> SPR_NOACCESS, SPR_NOACCESS,
> &spr_read_generic, &spr_write_generic,
> @@ -6870,7 +6870,7 @@ static void register_book3s_pmu_user_sprs(CPUPPCState *env)
> spr_register(env, SPR_POWER_UMMCR0, "UMMCR0",
> &spr_read_MMCR0_ureg, &spr_write_MMCR0_ureg,
> &spr_read_ureg, &spr_write_ureg,
> - 0x00000000);
> + 0x80000000);
> spr_register(env, SPR_POWER_UMMCR1, "UMMCR1",
> &spr_read_ureg, SPR_NOACCESS,
> &spr_read_ureg, &spr_write_ureg,
> diff --git a/target/ppc/helper.h b/target/ppc/helper.h
> index 627811cefc..ea60a7493c 100644
> --- a/target/ppc/helper.h
> +++ b/target/ppc/helper.h
> @@ -20,6 +20,7 @@ DEF_HELPER_1(rfscv, void, env)
> DEF_HELPER_1(hrfid, 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)
> #endif
> DEF_HELPER_1(check_tlb_flush_local, void, env)
> DEF_HELPER_1(check_tlb_flush_global, void, env)
> diff --git a/target/ppc/power8-pmu-regs.c.inc b/target/ppc/power8-pmu-regs.c.inc
> index 7391851238..fbb8977641 100644
> --- a/target/ppc/power8-pmu-regs.c.inc
> +++ b/target/ppc/power8-pmu-regs.c.inc
> @@ -104,6 +104,17 @@ void spr_read_MMCR0_ureg(DisasContext *ctx, int gprn, int sprn)
> tcg_temp_free(t0);
> }
>
> +static void write_MMCR0_common(DisasContext *ctx, TCGv val)
> +{
> + /*
> + * helper_store_mmcr0 will make clock based operations that
> + * will cause 'bad icount read' errors if we do not execute
> + * gen_icount_io_start() beforehand.
> + */
> + gen_icount_io_start(ctx);
> + gen_helper_store_mmcr0(cpu_env, val);
> +}
> +
> void spr_write_MMCR0_ureg(DisasContext *ctx, int sprn, int gprn)
> {
> TCGv masked_gprn;
> @@ -119,7 +130,7 @@ void spr_write_MMCR0_ureg(DisasContext *ctx, int sprn, int gprn)
> */
> masked_gprn = masked_gprn_for_spr_write(gprn, SPR_POWER_MMCR0,
> MMCR0_UREG_MASK);
> - gen_store_spr(SPR_POWER_MMCR0, masked_gprn);
> + write_MMCR0_common(ctx, masked_gprn);
>
> tcg_temp_free(masked_gprn);
> }
> @@ -219,6 +230,11 @@ void spr_write_PMC56_ureg(DisasContext *ctx, int sprn, int gprn)
> /* The remaining steps are similar to PMCs 1-4 userspace write */
> spr_write_PMC14_ureg(ctx, sprn, gprn);
> }
> +
> +void spr_write_MMCR0(DisasContext *ctx, int sprn, int gprn)
> +{
> + write_MMCR0_common(ctx, cpu_gpr[gprn]);
> +}
> #else
> void spr_read_MMCR0_ureg(DisasContext *ctx, int gprn, int sprn)
> {
> @@ -259,4 +275,9 @@ void spr_write_PMC56_ureg(DisasContext *ctx, int sprn, int gprn)
> {
> spr_noaccess(ctx, gprn, sprn);
> }
> +
> +void spr_write_MMCR0(DisasContext *ctx, int sprn, int gprn)
> +{
> + spr_write_generic(ctx, sprn, gprn);
> +}
> #endif /* defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY) */
> diff --git a/target/ppc/power8-pmu.c b/target/ppc/power8-pmu.c
> index e66c152eb5..47932ded4f 100644
> --- a/target/ppc/power8-pmu.c
> +++ b/target/ppc/power8-pmu.c
> @@ -23,6 +23,128 @@
>
> #if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
>
> +static bool pmc_is_inactive(CPUPPCState *env, int sprn)
> +{
> + if (env->spr[SPR_POWER_MMCR0] & MMCR0_FC) {
> + return true;
> + }
> +
> + if (sprn < SPR_POWER_PMC5) {
> + return env->spr[SPR_POWER_MMCR0] & MMCR0_FC14;
> + }
> +
> + return env->spr[SPR_POWER_MMCR0] & MMCR0_FC56;
> +}
> +
> +/*
> + * For PMCs 1-4, IBM POWER chips has support for an implementation
> + * dependent event, 0x1E, that enables cycle counting. The Linux kernel
> + * makes extensive use of 0x1E, so let's also support it.
> + *
> + * Likewise, event 0x2 is an implementation-dependent event that IBM
> + * POWER chips implement (at least since POWER8) that is equivalent to
> + * PM_INST_CMPL. Let's support this event on PMCs 1-4 as well.
> + */
> +static PMUEventType pmc_get_event(CPUPPCState *env, int sprn)
> +{
> + uint8_t mmcr1_evt_extr[] = { MMCR1_PMC1EVT_EXTR, MMCR1_PMC2EVT_EXTR,
> + MMCR1_PMC3EVT_EXTR, MMCR1_PMC4EVT_EXTR };
> + PMUEventType evt_type = PMU_EVENT_INVALID;
> + uint8_t pmcsel;
> + int i;
> +
> + if (pmc_is_inactive(env, sprn)) {
> + return PMU_EVENT_INACTIVE;
> + }
> +
> + if (sprn == SPR_POWER_PMC5) {
> + return PMU_EVENT_INSTRUCTIONS;
> + }
> +
> + if (sprn == SPR_POWER_PMC6) {
> + return PMU_EVENT_CYCLES;
> + }
> +
> + i = sprn - SPR_POWER_PMC1;
> + pmcsel = extract64(env->spr[SPR_POWER_MMCR1], mmcr1_evt_extr[i],
> + MMCR1_EVT_SIZE);
> +
> + switch (pmcsel) {
> + case 0x2:
> + evt_type = PMU_EVENT_INSTRUCTIONS;
> + break;
> + case 0x1E:
> + evt_type = PMU_EVENT_CYCLES;
> + break;
> + case 0xF0:
> + /*
> + * PMC1SEL = 0xF0 is the architected PowerISA v3.1
> + * event that counts cycles using PMC1.
> + */
> + if (sprn == SPR_POWER_PMC1) {
> + evt_type = PMU_EVENT_CYCLES;
> + }
> + break;
> + case 0xFE:
> + /*
> + * PMC1SEL = 0xFE is the architected PowerISA v3.1
> + * event to sample instructions using PMC1.
> + */
> + if (sprn == SPR_POWER_PMC1) {
> + evt_type = PMU_EVENT_INSTRUCTIONS;
> + }
> + break;
> + default:
> + break;
> + }
> +
> + return evt_type;
> +}
> +
> +static void pmu_update_cycles(CPUPPCState *env)
> +{
> + uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> + uint64_t time_delta = now - env->pmu_base_time;
> + int sprn;
> +
> + for (sprn = SPR_POWER_PMC1; sprn <= SPR_POWER_PMC6; sprn++) {
> + if (pmc_get_event(env, sprn) != PMU_EVENT_CYCLES) {
> + continue;
> + }
> +
> + /*
> + * The pseries and powernv clock runs at 1Ghz, meaning
> + * that 1 nanosec equals 1 cycle.
> + */
> + env->spr[sprn] += time_delta;
> + }
> +
> + /* Update base_time for future calculations */
> + env->pmu_base_time = now;
> +}
> +
> +void helper_store_mmcr0(CPUPPCState *env, target_ulong value)
> +{
> + target_ulong curr_value = env->spr[SPR_POWER_MMCR0];
> + bool curr_FC = curr_value & MMCR0_FC;
> + bool new_FC = value & MMCR0_FC;
> +
> + pmu_update_cycles(env);
> +
> + env->spr[SPR_POWER_MMCR0] = value;
> +
> + /*
> + * MMCR0 writes can change HFLAGS_PMCCCLEAR and HFLAGS_MMCR0FC.
> + * hreg_compute_hflags() does too much stuff to be called
> + * needlessly, so check beforehand if we really need a hflags
> + * update.
> + */
> + if (((curr_value & MMCR0_PMCC) != (value & MMCR0_PMCC)) ||
> + (curr_FC != new_FC)) {
> + hreg_compute_hflags(env);
> + }
> +}
> +
> static void fire_PMC_interrupt(PowerPCCPU *cpu)
> {
> CPUPPCState *env = &cpu->env;
> diff --git a/target/ppc/spr_tcg.h b/target/ppc/spr_tcg.h
> index 520f1ef233..eb1d0c2bf0 100644
> --- a/target/ppc/spr_tcg.h
> +++ b/target/ppc/spr_tcg.h
> @@ -25,6 +25,7 @@
> void spr_noaccess(DisasContext *ctx, int gprn, int sprn);
> void spr_read_generic(DisasContext *ctx, int gprn, int sprn);
> void spr_write_generic(DisasContext *ctx, int sprn, int gprn);
> +void spr_write_MMCR0(DisasContext *ctx, int sprn, int gprn);
> void spr_read_xer(DisasContext *ctx, int gprn, int sprn);
> void spr_write_xer(DisasContext *ctx, int sprn, int gprn);
> void spr_read_lr(DisasContext *ctx, int gprn, int sprn);
--
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] 24+ messages in thread
* [PATCH v8 03/10] target/ppc: PMU: update counters on PMCs r/w
2021-11-25 15:08 [PATCH v8 00/10] PMU-EBB support for PPC64 TCG Daniel Henrique Barboza
2021-11-25 15:08 ` [PATCH v8 01/10] target/ppc: introduce PMUEventType and PMU overflow timers Daniel Henrique Barboza
2021-11-25 15:08 ` [PATCH v8 02/10] target/ppc: PMU basic cycle count for pseries TCG Daniel Henrique Barboza
@ 2021-11-25 15:08 ` Daniel Henrique Barboza
2021-11-26 2:24 ` David Gibson
2021-11-25 15:08 ` [PATCH v8 04/10] target/ppc: PMU: update counters on MMCR1 write Daniel Henrique Barboza
` (6 subsequent siblings)
9 siblings, 1 reply; 24+ messages in thread
From: Daniel Henrique Barboza @ 2021-11-25 15:08 UTC (permalink / raw)
To: qemu-devel
Cc: richard.henderson, Daniel Henrique Barboza, qemu-ppc, clg, david
Calling pmu_update_cycles() on every PMC read/write operation ensures
that the values being fetched are up to date with the current PMU state.
In theory we can get away by just trapping PMCs reads, but we're going
to trap PMC writes to deal with counter overflow logic later on. Let's
put the required wiring for that and make our lives a bit easier in the
next patches.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
target/ppc/cpu_init.c | 12 ++++++------
target/ppc/helper.h | 2 ++
target/ppc/power8-pmu-regs.c.inc | 29 +++++++++++++++++++++++++++--
target/ppc/power8-pmu.c | 14 ++++++++++++++
target/ppc/spr_tcg.h | 2 ++
5 files changed, 51 insertions(+), 8 deletions(-)
diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
index e0b6fe4057..a7f47ec322 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -6833,27 +6833,27 @@ static void register_book3s_pmu_sup_sprs(CPUPPCState *env)
KVM_REG_PPC_MMCRA, 0x00000000);
spr_register_kvm(env, SPR_POWER_PMC1, "PMC1",
SPR_NOACCESS, SPR_NOACCESS,
- &spr_read_generic, &spr_write_generic,
+ &spr_read_PMC, &spr_write_PMC,
KVM_REG_PPC_PMC1, 0x00000000);
spr_register_kvm(env, SPR_POWER_PMC2, "PMC2",
SPR_NOACCESS, SPR_NOACCESS,
- &spr_read_generic, &spr_write_generic,
+ &spr_read_PMC, &spr_write_PMC,
KVM_REG_PPC_PMC2, 0x00000000);
spr_register_kvm(env, SPR_POWER_PMC3, "PMC3",
SPR_NOACCESS, SPR_NOACCESS,
- &spr_read_generic, &spr_write_generic,
+ &spr_read_PMC, &spr_write_PMC,
KVM_REG_PPC_PMC3, 0x00000000);
spr_register_kvm(env, SPR_POWER_PMC4, "PMC4",
SPR_NOACCESS, SPR_NOACCESS,
- &spr_read_generic, &spr_write_generic,
+ &spr_read_PMC, &spr_write_PMC,
KVM_REG_PPC_PMC4, 0x00000000);
spr_register_kvm(env, SPR_POWER_PMC5, "PMC5",
SPR_NOACCESS, SPR_NOACCESS,
- &spr_read_generic, &spr_write_generic,
+ &spr_read_PMC, &spr_write_PMC,
KVM_REG_PPC_PMC5, 0x00000000);
spr_register_kvm(env, SPR_POWER_PMC6, "PMC6",
SPR_NOACCESS, SPR_NOACCESS,
- &spr_read_generic, &spr_write_generic,
+ &spr_read_PMC, &spr_write_PMC,
KVM_REG_PPC_PMC6, 0x00000000);
spr_register_kvm(env, SPR_POWER_SIAR, "SIAR",
SPR_NOACCESS, SPR_NOACCESS,
diff --git a/target/ppc/helper.h b/target/ppc/helper.h
index ea60a7493c..d7567f75b4 100644
--- a/target/ppc/helper.h
+++ b/target/ppc/helper.h
@@ -21,6 +21,8 @@ DEF_HELPER_1(hrfid, 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)
+DEF_HELPER_3(store_pmc, void, env, i32, i64)
+DEF_HELPER_2(read_pmc, tl, env, i32)
#endif
DEF_HELPER_1(check_tlb_flush_local, void, env)
DEF_HELPER_1(check_tlb_flush_global, void, env)
diff --git a/target/ppc/power8-pmu-regs.c.inc b/target/ppc/power8-pmu-regs.c.inc
index fbb8977641..f0c9cc343b 100644
--- a/target/ppc/power8-pmu-regs.c.inc
+++ b/target/ppc/power8-pmu-regs.c.inc
@@ -181,13 +181,23 @@ void spr_write_MMCR2_ureg(DisasContext *ctx, int sprn, int gprn)
tcg_temp_free(masked_gprn);
}
+void spr_read_PMC(DisasContext *ctx, int gprn, int sprn)
+{
+ TCGv_i32 t_sprn = tcg_const_i32(sprn);
+
+ gen_icount_io_start(ctx);
+ gen_helper_read_pmc(cpu_gpr[gprn], cpu_env, t_sprn);
+
+ tcg_temp_free_i32(t_sprn);
+}
+
void spr_read_PMC14_ureg(DisasContext *ctx, int gprn, int sprn)
{
if (!spr_groupA_read_allowed(ctx)) {
return;
}
- spr_read_ureg(ctx, gprn, sprn);
+ spr_read_PMC(ctx, gprn, sprn + 0x10);
}
void spr_read_PMC56_ureg(DisasContext *ctx, int gprn, int sprn)
@@ -206,13 +216,23 @@ void spr_read_PMC56_ureg(DisasContext *ctx, int gprn, int sprn)
spr_read_PMC14_ureg(ctx, gprn, sprn);
}
+void spr_write_PMC(DisasContext *ctx, int sprn, int gprn)
+{
+ TCGv_i32 t_sprn = tcg_const_i32(sprn);
+
+ gen_icount_io_start(ctx);
+ gen_helper_store_pmc(cpu_env, t_sprn, cpu_gpr[gprn]);
+
+ tcg_temp_free_i32(t_sprn);
+}
+
void spr_write_PMC14_ureg(DisasContext *ctx, int sprn, int gprn)
{
if (!spr_groupA_write_allowed(ctx)) {
return;
}
- spr_write_ureg(ctx, sprn, gprn);
+ spr_write_PMC(ctx, sprn + 0x10, gprn);
}
void spr_write_PMC56_ureg(DisasContext *ctx, int sprn, int gprn)
@@ -280,4 +300,9 @@ void spr_write_MMCR0(DisasContext *ctx, int sprn, int gprn)
{
spr_write_generic(ctx, sprn, gprn);
}
+
+void spr_write_PMC(DisasContext *ctx, int sprn, int gprn)
+{
+ spr_write_generic(ctx, sprn, gprn);
+}
#endif /* defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY) */
diff --git a/target/ppc/power8-pmu.c b/target/ppc/power8-pmu.c
index 47932ded4f..5f2623aa25 100644
--- a/target/ppc/power8-pmu.c
+++ b/target/ppc/power8-pmu.c
@@ -145,6 +145,20 @@ void helper_store_mmcr0(CPUPPCState *env, target_ulong value)
}
}
+target_ulong helper_read_pmc(CPUPPCState *env, uint32_t sprn)
+{
+ pmu_update_cycles(env);
+
+ return env->spr[sprn];
+}
+
+void helper_store_pmc(CPUPPCState *env, uint32_t sprn, uint64_t value)
+{
+ pmu_update_cycles(env);
+
+ env->spr[sprn] = value;
+}
+
static void fire_PMC_interrupt(PowerPCCPU *cpu)
{
CPUPPCState *env = &cpu->env;
diff --git a/target/ppc/spr_tcg.h b/target/ppc/spr_tcg.h
index eb1d0c2bf0..1e79a0522a 100644
--- a/target/ppc/spr_tcg.h
+++ b/target/ppc/spr_tcg.h
@@ -26,6 +26,7 @@ void spr_noaccess(DisasContext *ctx, int gprn, int sprn);
void spr_read_generic(DisasContext *ctx, int gprn, int sprn);
void spr_write_generic(DisasContext *ctx, int sprn, int gprn);
void spr_write_MMCR0(DisasContext *ctx, int sprn, int gprn);
+void spr_write_PMC(DisasContext *ctx, int sprn, int gprn);
void spr_read_xer(DisasContext *ctx, int gprn, int sprn);
void spr_write_xer(DisasContext *ctx, int sprn, int gprn);
void spr_read_lr(DisasContext *ctx, int gprn, int sprn);
@@ -35,6 +36,7 @@ void spr_write_ctr(DisasContext *ctx, int sprn, int gprn);
void spr_read_ureg(DisasContext *ctx, int gprn, int sprn);
void spr_read_MMCR0_ureg(DisasContext *ctx, int gprn, int sprn);
void spr_read_MMCR2_ureg(DisasContext *ctx, int gprn, int sprn);
+void spr_read_PMC(DisasContext *ctx, int gprn, int sprn);
void spr_read_PMC14_ureg(DisasContext *ctx, int gprn, int sprn);
void spr_read_PMC56_ureg(DisasContext *ctx, int gprn, int sprn);
void spr_read_tbl(DisasContext *ctx, int gprn, int sprn);
--
2.31.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v8 03/10] target/ppc: PMU: update counters on PMCs r/w
2021-11-25 15:08 ` [PATCH v8 03/10] target/ppc: PMU: update counters on PMCs r/w Daniel Henrique Barboza
@ 2021-11-26 2:24 ` David Gibson
0 siblings, 0 replies; 24+ messages in thread
From: David Gibson @ 2021-11-26 2:24 UTC (permalink / raw)
To: Daniel Henrique Barboza; +Cc: richard.henderson, qemu-ppc, qemu-devel, clg
[-- Attachment #1: Type: text/plain, Size: 7698 bytes --]
On Thu, Nov 25, 2021 at 12:08:10PM -0300, Daniel Henrique Barboza wrote:
> Calling pmu_update_cycles() on every PMC read/write operation ensures
> that the values being fetched are up to date with the current PMU state.
>
> In theory we can get away by just trapping PMCs reads, but we're going
> to trap PMC writes to deal with counter overflow logic later on. Let's
> put the required wiring for that and make our lives a bit easier in the
> next patches.
>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> target/ppc/cpu_init.c | 12 ++++++------
> target/ppc/helper.h | 2 ++
> target/ppc/power8-pmu-regs.c.inc | 29 +++++++++++++++++++++++++++--
> target/ppc/power8-pmu.c | 14 ++++++++++++++
> target/ppc/spr_tcg.h | 2 ++
> 5 files changed, 51 insertions(+), 8 deletions(-)
>
> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> index e0b6fe4057..a7f47ec322 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -6833,27 +6833,27 @@ static void register_book3s_pmu_sup_sprs(CPUPPCState *env)
> KVM_REG_PPC_MMCRA, 0x00000000);
> spr_register_kvm(env, SPR_POWER_PMC1, "PMC1",
> SPR_NOACCESS, SPR_NOACCESS,
> - &spr_read_generic, &spr_write_generic,
> + &spr_read_PMC, &spr_write_PMC,
> KVM_REG_PPC_PMC1, 0x00000000);
> spr_register_kvm(env, SPR_POWER_PMC2, "PMC2",
> SPR_NOACCESS, SPR_NOACCESS,
> - &spr_read_generic, &spr_write_generic,
> + &spr_read_PMC, &spr_write_PMC,
> KVM_REG_PPC_PMC2, 0x00000000);
> spr_register_kvm(env, SPR_POWER_PMC3, "PMC3",
> SPR_NOACCESS, SPR_NOACCESS,
> - &spr_read_generic, &spr_write_generic,
> + &spr_read_PMC, &spr_write_PMC,
> KVM_REG_PPC_PMC3, 0x00000000);
> spr_register_kvm(env, SPR_POWER_PMC4, "PMC4",
> SPR_NOACCESS, SPR_NOACCESS,
> - &spr_read_generic, &spr_write_generic,
> + &spr_read_PMC, &spr_write_PMC,
> KVM_REG_PPC_PMC4, 0x00000000);
> spr_register_kvm(env, SPR_POWER_PMC5, "PMC5",
> SPR_NOACCESS, SPR_NOACCESS,
> - &spr_read_generic, &spr_write_generic,
> + &spr_read_PMC, &spr_write_PMC,
> KVM_REG_PPC_PMC5, 0x00000000);
> spr_register_kvm(env, SPR_POWER_PMC6, "PMC6",
> SPR_NOACCESS, SPR_NOACCESS,
> - &spr_read_generic, &spr_write_generic,
> + &spr_read_PMC, &spr_write_PMC,
> KVM_REG_PPC_PMC6, 0x00000000);
> spr_register_kvm(env, SPR_POWER_SIAR, "SIAR",
> SPR_NOACCESS, SPR_NOACCESS,
> diff --git a/target/ppc/helper.h b/target/ppc/helper.h
> index ea60a7493c..d7567f75b4 100644
> --- a/target/ppc/helper.h
> +++ b/target/ppc/helper.h
> @@ -21,6 +21,8 @@ DEF_HELPER_1(hrfid, 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)
> +DEF_HELPER_3(store_pmc, void, env, i32, i64)
> +DEF_HELPER_2(read_pmc, tl, env, i32)
> #endif
> DEF_HELPER_1(check_tlb_flush_local, void, env)
> DEF_HELPER_1(check_tlb_flush_global, void, env)
> diff --git a/target/ppc/power8-pmu-regs.c.inc b/target/ppc/power8-pmu-regs.c.inc
> index fbb8977641..f0c9cc343b 100644
> --- a/target/ppc/power8-pmu-regs.c.inc
> +++ b/target/ppc/power8-pmu-regs.c.inc
> @@ -181,13 +181,23 @@ void spr_write_MMCR2_ureg(DisasContext *ctx, int sprn, int gprn)
> tcg_temp_free(masked_gprn);
> }
>
> +void spr_read_PMC(DisasContext *ctx, int gprn, int sprn)
> +{
> + TCGv_i32 t_sprn = tcg_const_i32(sprn);
> +
> + gen_icount_io_start(ctx);
> + gen_helper_read_pmc(cpu_gpr[gprn], cpu_env, t_sprn);
> +
> + tcg_temp_free_i32(t_sprn);
> +}
> +
> void spr_read_PMC14_ureg(DisasContext *ctx, int gprn, int sprn)
> {
> if (!spr_groupA_read_allowed(ctx)) {
> return;
> }
>
> - spr_read_ureg(ctx, gprn, sprn);
> + spr_read_PMC(ctx, gprn, sprn + 0x10);
> }
>
> void spr_read_PMC56_ureg(DisasContext *ctx, int gprn, int sprn)
> @@ -206,13 +216,23 @@ void spr_read_PMC56_ureg(DisasContext *ctx, int gprn, int sprn)
> spr_read_PMC14_ureg(ctx, gprn, sprn);
> }
>
> +void spr_write_PMC(DisasContext *ctx, int sprn, int gprn)
> +{
> + TCGv_i32 t_sprn = tcg_const_i32(sprn);
> +
> + gen_icount_io_start(ctx);
> + gen_helper_store_pmc(cpu_env, t_sprn, cpu_gpr[gprn]);
> +
> + tcg_temp_free_i32(t_sprn);
> +}
> +
> void spr_write_PMC14_ureg(DisasContext *ctx, int sprn, int gprn)
> {
> if (!spr_groupA_write_allowed(ctx)) {
> return;
> }
>
> - spr_write_ureg(ctx, sprn, gprn);
> + spr_write_PMC(ctx, sprn + 0x10, gprn);
> }
>
> void spr_write_PMC56_ureg(DisasContext *ctx, int sprn, int gprn)
> @@ -280,4 +300,9 @@ void spr_write_MMCR0(DisasContext *ctx, int sprn, int gprn)
> {
> spr_write_generic(ctx, sprn, gprn);
> }
> +
> +void spr_write_PMC(DisasContext *ctx, int sprn, int gprn)
> +{
> + spr_write_generic(ctx, sprn, gprn);
> +}
> #endif /* defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY) */
> diff --git a/target/ppc/power8-pmu.c b/target/ppc/power8-pmu.c
> index 47932ded4f..5f2623aa25 100644
> --- a/target/ppc/power8-pmu.c
> +++ b/target/ppc/power8-pmu.c
> @@ -145,6 +145,20 @@ void helper_store_mmcr0(CPUPPCState *env, target_ulong value)
> }
> }
>
> +target_ulong helper_read_pmc(CPUPPCState *env, uint32_t sprn)
> +{
> + pmu_update_cycles(env);
> +
> + return env->spr[sprn];
> +}
> +
> +void helper_store_pmc(CPUPPCState *env, uint32_t sprn, uint64_t value)
> +{
> + pmu_update_cycles(env);
> +
> + env->spr[sprn] = value;
> +}
> +
> static void fire_PMC_interrupt(PowerPCCPU *cpu)
> {
> CPUPPCState *env = &cpu->env;
> diff --git a/target/ppc/spr_tcg.h b/target/ppc/spr_tcg.h
> index eb1d0c2bf0..1e79a0522a 100644
> --- a/target/ppc/spr_tcg.h
> +++ b/target/ppc/spr_tcg.h
> @@ -26,6 +26,7 @@ void spr_noaccess(DisasContext *ctx, int gprn, int sprn);
> void spr_read_generic(DisasContext *ctx, int gprn, int sprn);
> void spr_write_generic(DisasContext *ctx, int sprn, int gprn);
> void spr_write_MMCR0(DisasContext *ctx, int sprn, int gprn);
> +void spr_write_PMC(DisasContext *ctx, int sprn, int gprn);
> void spr_read_xer(DisasContext *ctx, int gprn, int sprn);
> void spr_write_xer(DisasContext *ctx, int sprn, int gprn);
> void spr_read_lr(DisasContext *ctx, int gprn, int sprn);
> @@ -35,6 +36,7 @@ void spr_write_ctr(DisasContext *ctx, int sprn, int gprn);
> void spr_read_ureg(DisasContext *ctx, int gprn, int sprn);
> void spr_read_MMCR0_ureg(DisasContext *ctx, int gprn, int sprn);
> void spr_read_MMCR2_ureg(DisasContext *ctx, int gprn, int sprn);
> +void spr_read_PMC(DisasContext *ctx, int gprn, int sprn);
> void spr_read_PMC14_ureg(DisasContext *ctx, int gprn, int sprn);
> void spr_read_PMC56_ureg(DisasContext *ctx, int gprn, int sprn);
> void spr_read_tbl(DisasContext *ctx, int gprn, int sprn);
--
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] 24+ messages in thread
* [PATCH v8 04/10] target/ppc: PMU: update counters on MMCR1 write
2021-11-25 15:08 [PATCH v8 00/10] PMU-EBB support for PPC64 TCG Daniel Henrique Barboza
` (2 preceding siblings ...)
2021-11-25 15:08 ` [PATCH v8 03/10] target/ppc: PMU: update counters on PMCs r/w Daniel Henrique Barboza
@ 2021-11-25 15:08 ` Daniel Henrique Barboza
2021-11-26 2:24 ` David Gibson
2021-11-25 15:08 ` [PATCH v8 05/10] target/ppc: enable PMU counter overflow with cycle events Daniel Henrique Barboza
` (5 subsequent siblings)
9 siblings, 1 reply; 24+ messages in thread
From: Daniel Henrique Barboza @ 2021-11-25 15:08 UTC (permalink / raw)
To: qemu-devel
Cc: richard.henderson, Daniel Henrique Barboza, qemu-ppc, clg, david
MMCR1 determines the events to be sampled by the PMU. Updating the
counters at every MMCR1 write ensures that we're not sampling more
or less events by looking only at MMCR0 and the PMCs.
It is worth noticing that both the Book3S PowerPC PMU, and this IBM
Power8+ PMU that we're modeling, also uses MMCRA, MMCR2 and MMCR3 to
control the PMU. These three registers aren't being handled in this
initial implementation, so for now we're controlling all the PMU
aspects using MMCR0, MMCR1 and the PMCs.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
target/ppc/cpu_init.c | 2 +-
target/ppc/helper.h | 1 +
target/ppc/power8-pmu-regs.c.inc | 11 +++++++++++
target/ppc/power8-pmu.c | 7 +++++++
target/ppc/spr_tcg.h | 1 +
5 files changed, 21 insertions(+), 1 deletion(-)
diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
index a7f47ec322..2d72dde26d 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -6825,7 +6825,7 @@ static void register_book3s_pmu_sup_sprs(CPUPPCState *env)
KVM_REG_PPC_MMCR0, 0x80000000);
spr_register_kvm(env, SPR_POWER_MMCR1, "MMCR1",
SPR_NOACCESS, SPR_NOACCESS,
- &spr_read_generic, &spr_write_generic,
+ &spr_read_generic, &spr_write_MMCR1,
KVM_REG_PPC_MMCR1, 0x00000000);
spr_register_kvm(env, SPR_POWER_MMCRA, "MMCRA",
SPR_NOACCESS, SPR_NOACCESS,
diff --git a/target/ppc/helper.h b/target/ppc/helper.h
index d7567f75b4..94b4690375 100644
--- a/target/ppc/helper.h
+++ b/target/ppc/helper.h
@@ -21,6 +21,7 @@ DEF_HELPER_1(hrfid, 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)
+DEF_HELPER_2(store_mmcr1, void, env, tl)
DEF_HELPER_3(store_pmc, void, env, i32, i64)
DEF_HELPER_2(read_pmc, tl, env, i32)
#endif
diff --git a/target/ppc/power8-pmu-regs.c.inc b/target/ppc/power8-pmu-regs.c.inc
index f0c9cc343b..25b13ad564 100644
--- a/target/ppc/power8-pmu-regs.c.inc
+++ b/target/ppc/power8-pmu-regs.c.inc
@@ -255,6 +255,12 @@ void spr_write_MMCR0(DisasContext *ctx, int sprn, int gprn)
{
write_MMCR0_common(ctx, cpu_gpr[gprn]);
}
+
+void spr_write_MMCR1(DisasContext *ctx, int sprn, int gprn)
+{
+ gen_icount_io_start(ctx);
+ gen_helper_store_mmcr1(cpu_env, cpu_gpr[gprn]);
+}
#else
void spr_read_MMCR0_ureg(DisasContext *ctx, int gprn, int sprn)
{
@@ -301,6 +307,11 @@ void spr_write_MMCR0(DisasContext *ctx, int sprn, int gprn)
spr_write_generic(ctx, sprn, gprn);
}
+void spr_write_MMCR1(DisasContext *ctx, int sprn, int gprn)
+{
+ spr_write_generic(ctx, sprn, gprn);
+}
+
void spr_write_PMC(DisasContext *ctx, int sprn, int gprn)
{
spr_write_generic(ctx, sprn, gprn);
diff --git a/target/ppc/power8-pmu.c b/target/ppc/power8-pmu.c
index 5f2623aa25..acdaee7459 100644
--- a/target/ppc/power8-pmu.c
+++ b/target/ppc/power8-pmu.c
@@ -145,6 +145,13 @@ void helper_store_mmcr0(CPUPPCState *env, target_ulong value)
}
}
+void helper_store_mmcr1(CPUPPCState *env, uint64_t value)
+{
+ pmu_update_cycles(env);
+
+ env->spr[SPR_POWER_MMCR1] = value;
+}
+
target_ulong helper_read_pmc(CPUPPCState *env, uint32_t sprn)
{
pmu_update_cycles(env);
diff --git a/target/ppc/spr_tcg.h b/target/ppc/spr_tcg.h
index 1e79a0522a..1d6521eedc 100644
--- a/target/ppc/spr_tcg.h
+++ b/target/ppc/spr_tcg.h
@@ -26,6 +26,7 @@ void spr_noaccess(DisasContext *ctx, int gprn, int sprn);
void spr_read_generic(DisasContext *ctx, int gprn, int sprn);
void spr_write_generic(DisasContext *ctx, int sprn, int gprn);
void spr_write_MMCR0(DisasContext *ctx, int sprn, int gprn);
+void spr_write_MMCR1(DisasContext *ctx, int sprn, int gprn);
void spr_write_PMC(DisasContext *ctx, int sprn, int gprn);
void spr_read_xer(DisasContext *ctx, int gprn, int sprn);
void spr_write_xer(DisasContext *ctx, int sprn, int gprn);
--
2.31.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v8 04/10] target/ppc: PMU: update counters on MMCR1 write
2021-11-25 15:08 ` [PATCH v8 04/10] target/ppc: PMU: update counters on MMCR1 write Daniel Henrique Barboza
@ 2021-11-26 2:24 ` David Gibson
0 siblings, 0 replies; 24+ messages in thread
From: David Gibson @ 2021-11-26 2:24 UTC (permalink / raw)
To: Daniel Henrique Barboza; +Cc: richard.henderson, qemu-ppc, qemu-devel, clg
[-- Attachment #1: Type: text/plain, Size: 4631 bytes --]
On Thu, Nov 25, 2021 at 12:08:11PM -0300, Daniel Henrique Barboza wrote:
> MMCR1 determines the events to be sampled by the PMU. Updating the
> counters at every MMCR1 write ensures that we're not sampling more
> or less events by looking only at MMCR0 and the PMCs.
>
> It is worth noticing that both the Book3S PowerPC PMU, and this IBM
> Power8+ PMU that we're modeling, also uses MMCRA, MMCR2 and MMCR3 to
> control the PMU. These three registers aren't being handled in this
> initial implementation, so for now we're controlling all the PMU
> aspects using MMCR0, MMCR1 and the PMCs.
>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> target/ppc/cpu_init.c | 2 +-
> target/ppc/helper.h | 1 +
> target/ppc/power8-pmu-regs.c.inc | 11 +++++++++++
> target/ppc/power8-pmu.c | 7 +++++++
> target/ppc/spr_tcg.h | 1 +
> 5 files changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> index a7f47ec322..2d72dde26d 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -6825,7 +6825,7 @@ static void register_book3s_pmu_sup_sprs(CPUPPCState *env)
> KVM_REG_PPC_MMCR0, 0x80000000);
> spr_register_kvm(env, SPR_POWER_MMCR1, "MMCR1",
> SPR_NOACCESS, SPR_NOACCESS,
> - &spr_read_generic, &spr_write_generic,
> + &spr_read_generic, &spr_write_MMCR1,
> KVM_REG_PPC_MMCR1, 0x00000000);
> spr_register_kvm(env, SPR_POWER_MMCRA, "MMCRA",
> SPR_NOACCESS, SPR_NOACCESS,
> diff --git a/target/ppc/helper.h b/target/ppc/helper.h
> index d7567f75b4..94b4690375 100644
> --- a/target/ppc/helper.h
> +++ b/target/ppc/helper.h
> @@ -21,6 +21,7 @@ DEF_HELPER_1(hrfid, 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)
> +DEF_HELPER_2(store_mmcr1, void, env, tl)
> DEF_HELPER_3(store_pmc, void, env, i32, i64)
> DEF_HELPER_2(read_pmc, tl, env, i32)
> #endif
> diff --git a/target/ppc/power8-pmu-regs.c.inc b/target/ppc/power8-pmu-regs.c.inc
> index f0c9cc343b..25b13ad564 100644
> --- a/target/ppc/power8-pmu-regs.c.inc
> +++ b/target/ppc/power8-pmu-regs.c.inc
> @@ -255,6 +255,12 @@ void spr_write_MMCR0(DisasContext *ctx, int sprn, int gprn)
> {
> write_MMCR0_common(ctx, cpu_gpr[gprn]);
> }
> +
> +void spr_write_MMCR1(DisasContext *ctx, int sprn, int gprn)
> +{
> + gen_icount_io_start(ctx);
> + gen_helper_store_mmcr1(cpu_env, cpu_gpr[gprn]);
> +}
> #else
> void spr_read_MMCR0_ureg(DisasContext *ctx, int gprn, int sprn)
> {
> @@ -301,6 +307,11 @@ void spr_write_MMCR0(DisasContext *ctx, int sprn, int gprn)
> spr_write_generic(ctx, sprn, gprn);
> }
>
> +void spr_write_MMCR1(DisasContext *ctx, int sprn, int gprn)
> +{
> + spr_write_generic(ctx, sprn, gprn);
> +}
> +
> void spr_write_PMC(DisasContext *ctx, int sprn, int gprn)
> {
> spr_write_generic(ctx, sprn, gprn);
> diff --git a/target/ppc/power8-pmu.c b/target/ppc/power8-pmu.c
> index 5f2623aa25..acdaee7459 100644
> --- a/target/ppc/power8-pmu.c
> +++ b/target/ppc/power8-pmu.c
> @@ -145,6 +145,13 @@ void helper_store_mmcr0(CPUPPCState *env, target_ulong value)
> }
> }
>
> +void helper_store_mmcr1(CPUPPCState *env, uint64_t value)
> +{
> + pmu_update_cycles(env);
> +
> + env->spr[SPR_POWER_MMCR1] = value;
> +}
> +
> target_ulong helper_read_pmc(CPUPPCState *env, uint32_t sprn)
> {
> pmu_update_cycles(env);
> diff --git a/target/ppc/spr_tcg.h b/target/ppc/spr_tcg.h
> index 1e79a0522a..1d6521eedc 100644
> --- a/target/ppc/spr_tcg.h
> +++ b/target/ppc/spr_tcg.h
> @@ -26,6 +26,7 @@ void spr_noaccess(DisasContext *ctx, int gprn, int sprn);
> void spr_read_generic(DisasContext *ctx, int gprn, int sprn);
> void spr_write_generic(DisasContext *ctx, int sprn, int gprn);
> void spr_write_MMCR0(DisasContext *ctx, int sprn, int gprn);
> +void spr_write_MMCR1(DisasContext *ctx, int sprn, int gprn);
> void spr_write_PMC(DisasContext *ctx, int sprn, int gprn);
> void spr_read_xer(DisasContext *ctx, int gprn, int sprn);
> void spr_write_xer(DisasContext *ctx, int sprn, int gprn);
--
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] 24+ messages in thread
* [PATCH v8 05/10] target/ppc: enable PMU counter overflow with cycle events
2021-11-25 15:08 [PATCH v8 00/10] PMU-EBB support for PPC64 TCG Daniel Henrique Barboza
` (3 preceding siblings ...)
2021-11-25 15:08 ` [PATCH v8 04/10] target/ppc: PMU: update counters on MMCR1 write Daniel Henrique Barboza
@ 2021-11-25 15:08 ` Daniel Henrique Barboza
2021-11-29 3:41 ` David Gibson
2021-11-25 15:08 ` [PATCH v8 06/10] target/ppc: enable PMU instruction count Daniel Henrique Barboza
` (4 subsequent siblings)
9 siblings, 1 reply; 24+ messages in thread
From: Daniel Henrique Barboza @ 2021-11-25 15:08 UTC (permalink / raw)
To: qemu-devel
Cc: richard.henderson, Daniel Henrique Barboza, qemu-ppc, clg, david
The PowerISA v3.1 defines that if the proper bits are set (MMCR0_PMC1CE
for PMC1 and MMCR0_PMCjCE for the remaining PMCs), counter negative
conditions are enabled. This means that if the counter value overflows
(i.e. exceeds 0x80000000) a performance monitor alert will occur. This alert
can trigger an event-based exception (to be implemented in the next patches)
if the MMCR0_EBE bit is set.
For now, overflowing the counter when the PMC is counting cycles will
just trigger a performance monitor alert. This is done by starting the
overflow timer to expire in the moment the overflow would be occuring. The
timer will call fire_PMC_interrupt() (via cpu_ppc_pmu_timer_cb) which will
trigger the PMU alert and, if the conditions are met, an EBB exception.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
target/ppc/cpu.h | 2 ++
target/ppc/power8-pmu.c | 80 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 82 insertions(+)
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 9c732953f0..9b41b022e2 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -364,6 +364,8 @@ typedef enum {
#define MMCR0_PMCC PPC_BITMASK(44, 45) /* PMC Control */
#define MMCR0_FC14 PPC_BIT(58) /* PMC Freeze Counters 1-4 bit */
#define MMCR0_FC56 PPC_BIT(59) /* PMC Freeze Counters 5-6 bit */
+#define MMCR0_PMC1CE PPC_BIT(48) /* MMCR0 PMC1 Condition Enabled */
+#define MMCR0_PMCjCE PPC_BIT(49) /* MMCR0 PMCj Condition Enabled */
/* MMCR0 userspace r/w mask */
#define MMCR0_UREG_MASK (MMCR0_FC | MMCR0_PMAO | MMCR0_PMAE)
/* MMCR2 userspace r/w mask */
diff --git a/target/ppc/power8-pmu.c b/target/ppc/power8-pmu.c
index acdaee7459..01e0b9b8fc 100644
--- a/target/ppc/power8-pmu.c
+++ b/target/ppc/power8-pmu.c
@@ -23,6 +23,8 @@
#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
+#define PMC_COUNTER_NEGATIVE_VAL 0x80000000UL
+
static bool pmc_is_inactive(CPUPPCState *env, int sprn)
{
if (env->spr[SPR_POWER_MMCR0] & MMCR0_FC) {
@@ -36,6 +38,15 @@ static bool pmc_is_inactive(CPUPPCState *env, int sprn)
return env->spr[SPR_POWER_MMCR0] & MMCR0_FC56;
}
+static bool pmc_has_overflow_enabled(CPUPPCState *env, int sprn)
+{
+ if (sprn == SPR_POWER_PMC1) {
+ return env->spr[SPR_POWER_MMCR0] & MMCR0_PMC1CE;
+ }
+
+ return env->spr[SPR_POWER_MMCR0] & MMCR0_PMCjCE;
+}
+
/*
* For PMCs 1-4, IBM POWER chips has support for an implementation
* dependent event, 0x1E, that enables cycle counting. The Linux kernel
@@ -123,6 +134,70 @@ static void pmu_update_cycles(CPUPPCState *env)
env->pmu_base_time = now;
}
+/*
+ * Helper function to retrieve the cycle overflow timer of the
+ * 'sprn' counter. Given that PMC5 doesn't have a timer, the
+ * amount of timers is less than the total counters and the PMC6
+ * timer is the last of the array.
+ */
+static QEMUTimer *get_cyc_overflow_timer(CPUPPCState *env, int sprn)
+{
+ if (sprn == SPR_POWER_PMC5) {
+ return NULL;
+ }
+
+ if (sprn == SPR_POWER_PMC6) {
+ return env->pmu_cyc_overflow_timers[PMU_TIMERS_NUM - 1];
+ }
+
+ return env->pmu_cyc_overflow_timers[sprn - SPR_POWER_PMC1];
+}
+
+static void pmc_update_overflow_timer(CPUPPCState *env, int sprn)
+{
+ QEMUTimer *pmc_overflow_timer;
+ int64_t timeout;
+
+ /* PMC5 does not have an overflow timer */
+ if (sprn == SPR_POWER_PMC5) {
+ return;
+ }
+
+ pmc_overflow_timer = get_cyc_overflow_timer(env, sprn);
+
+ if (pmc_get_event(env, sprn) != PMU_EVENT_CYCLES ||
+ !pmc_has_overflow_enabled(env, sprn)) {
+ /* Overflow timer is not needed for this counter */
+ timer_del(pmc_overflow_timer);
+ return;
+ }
+
+ if (env->spr[sprn] >= PMC_COUNTER_NEGATIVE_VAL) {
+ timeout = 0;
+ } else {
+ timeout = PMC_COUNTER_NEGATIVE_VAL - env->spr[sprn];
+ }
+
+ /*
+ * Use timer_mod_anticipate() because an overflow timer might
+ * be already running for this PMC.
+ */
+ timer_mod_anticipate(pmc_overflow_timer, env->pmu_base_time + timeout);
+}
+
+static void pmu_update_overflow_timers(CPUPPCState *env)
+{
+ int sprn;
+
+ /*
+ * Scroll through all PMCs and start counter overflow timers for
+ * PM_CYC events, if needed.
+ */
+ for (sprn = SPR_POWER_PMC1; sprn <= SPR_POWER_PMC6; sprn++) {
+ pmc_update_overflow_timer(env, sprn);
+ }
+}
+
void helper_store_mmcr0(CPUPPCState *env, target_ulong value)
{
target_ulong curr_value = env->spr[SPR_POWER_MMCR0];
@@ -143,6 +218,9 @@ void helper_store_mmcr0(CPUPPCState *env, target_ulong value)
(curr_FC != new_FC)) {
hreg_compute_hflags(env);
}
+
+ /* Update cycle overflow timers with the current MMCR0 state */
+ pmu_update_overflow_timers(env);
}
void helper_store_mmcr1(CPUPPCState *env, uint64_t value)
@@ -164,6 +242,8 @@ void helper_store_pmc(CPUPPCState *env, uint32_t sprn, uint64_t value)
pmu_update_cycles(env);
env->spr[sprn] = value;
+
+ pmc_update_overflow_timer(env, sprn);
}
static void fire_PMC_interrupt(PowerPCCPU *cpu)
--
2.31.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v8 05/10] target/ppc: enable PMU counter overflow with cycle events
2021-11-25 15:08 ` [PATCH v8 05/10] target/ppc: enable PMU counter overflow with cycle events Daniel Henrique Barboza
@ 2021-11-29 3:41 ` David Gibson
0 siblings, 0 replies; 24+ messages in thread
From: David Gibson @ 2021-11-29 3:41 UTC (permalink / raw)
To: Daniel Henrique Barboza; +Cc: richard.henderson, qemu-ppc, qemu-devel, clg
[-- Attachment #1: Type: text/plain, Size: 6356 bytes --]
On Thu, Nov 25, 2021 at 12:08:12PM -0300, Daniel Henrique Barboza wrote:
65;6601;1c> The PowerISA v3.1 defines that if the proper bits are set (MMCR0_PMC1CE
> for PMC1 and MMCR0_PMCjCE for the remaining PMCs), counter negative
> conditions are enabled. This means that if the counter value overflows
> (i.e. exceeds 0x80000000) a performance monitor alert will occur. This alert
> can trigger an event-based exception (to be implemented in the next patches)
> if the MMCR0_EBE bit is set.
>
> For now, overflowing the counter when the PMC is counting cycles will
> just trigger a performance monitor alert. This is done by starting the
> overflow timer to expire in the moment the overflow would be occuring. The
> timer will call fire_PMC_interrupt() (via cpu_ppc_pmu_timer_cb) which will
> trigger the PMU alert and, if the conditions are met, an EBB exception.
>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
A couple of minor nits noted below, though.
> ---
> target/ppc/cpu.h | 2 ++
> target/ppc/power8-pmu.c | 80 +++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 82 insertions(+)
>
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 9c732953f0..9b41b022e2 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -364,6 +364,8 @@ typedef enum {
> #define MMCR0_PMCC PPC_BITMASK(44, 45) /* PMC Control */
> #define MMCR0_FC14 PPC_BIT(58) /* PMC Freeze Counters 1-4 bit */
> #define MMCR0_FC56 PPC_BIT(59) /* PMC Freeze Counters 5-6 bit */
> +#define MMCR0_PMC1CE PPC_BIT(48) /* MMCR0 PMC1 Condition Enabled */
> +#define MMCR0_PMCjCE PPC_BIT(49) /* MMCR0 PMCj Condition Enabled */
> /* MMCR0 userspace r/w mask */
> #define MMCR0_UREG_MASK (MMCR0_FC | MMCR0_PMAO | MMCR0_PMAE)
> /* MMCR2 userspace r/w mask */
> diff --git a/target/ppc/power8-pmu.c b/target/ppc/power8-pmu.c
> index acdaee7459..01e0b9b8fc 100644
> --- a/target/ppc/power8-pmu.c
> +++ b/target/ppc/power8-pmu.c
> @@ -23,6 +23,8 @@
>
> #if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
>
> +#define PMC_COUNTER_NEGATIVE_VAL 0x80000000UL
> +
> static bool pmc_is_inactive(CPUPPCState *env, int sprn)
> {
> if (env->spr[SPR_POWER_MMCR0] & MMCR0_FC) {
> @@ -36,6 +38,15 @@ static bool pmc_is_inactive(CPUPPCState *env, int sprn)
> return env->spr[SPR_POWER_MMCR0] & MMCR0_FC56;
> }
>
> +static bool pmc_has_overflow_enabled(CPUPPCState *env, int sprn)
> +{
> + if (sprn == SPR_POWER_PMC1) {
> + return env->spr[SPR_POWER_MMCR0] & MMCR0_PMC1CE;
> + }
> +
> + return env->spr[SPR_POWER_MMCR0] & MMCR0_PMCjCE;
> +}
> +
> /*
> * For PMCs 1-4, IBM POWER chips has support for an implementation
> * dependent event, 0x1E, that enables cycle counting. The Linux kernel
> @@ -123,6 +134,70 @@ static void pmu_update_cycles(CPUPPCState *env)
> env->pmu_base_time = now;
> }
>
> +/*
> + * Helper function to retrieve the cycle overflow timer of the
> + * 'sprn' counter. Given that PMC5 doesn't have a timer, the
> + * amount of timers is less than the total counters and the PMC6
> + * timer is the last of the array.
> + */
> +static QEMUTimer *get_cyc_overflow_timer(CPUPPCState *env, int sprn)
> +{
> + if (sprn == SPR_POWER_PMC5) {
> + return NULL;
Given that the entries in the pmu_cyc_overflow_timers are just
pointers, it would probably be slightly cheaper in terms of both time
and space to just have an always-NULL entry for PMC5, rather than
having to special case it.
> + }
> +
> + if (sprn == SPR_POWER_PMC6) {
> + return env->pmu_cyc_overflow_timers[PMU_TIMERS_NUM - 1];
> + }
> +
> + return env->pmu_cyc_overflow_timers[sprn - SPR_POWER_PMC1];
> +}
> +
> +static void pmc_update_overflow_timer(CPUPPCState *env, int sprn)
> +{
> + QEMUTimer *pmc_overflow_timer;
> + int64_t timeout;
> +
> + /* PMC5 does not have an overflow timer */
> + if (sprn == SPR_POWER_PMC5) {
> + return;
Since you've already handled the PMC5 case in
get_cyc_overflow_timer(), you could replace this handling with just an
if (!pmc_overflow_timer) {return;}
> + }
> +
> + pmc_overflow_timer = get_cyc_overflow_timer(env, sprn);
> +
> + if (pmc_get_event(env, sprn) != PMU_EVENT_CYCLES ||
> + !pmc_has_overflow_enabled(env, sprn)) {
> + /* Overflow timer is not needed for this counter */
> + timer_del(pmc_overflow_timer);
> + return;
> + }
> +
> + if (env->spr[sprn] >= PMC_COUNTER_NEGATIVE_VAL) {
> + timeout = 0;
> + } else {
> + timeout = PMC_COUNTER_NEGATIVE_VAL - env->spr[sprn];
> + }
> +
> + /*
> + * Use timer_mod_anticipate() because an overflow timer might
> + * be already running for this PMC.
> + */
> + timer_mod_anticipate(pmc_overflow_timer, env->pmu_base_time + timeout);
> +}
> +
> +static void pmu_update_overflow_timers(CPUPPCState *env)
> +{
> + int sprn;
> +
> + /*
> + * Scroll through all PMCs and start counter overflow timers for
> + * PM_CYC events, if needed.
> + */
> + for (sprn = SPR_POWER_PMC1; sprn <= SPR_POWER_PMC6; sprn++) {
> + pmc_update_overflow_timer(env, sprn);
> + }
> +}
> +
> void helper_store_mmcr0(CPUPPCState *env, target_ulong value)
> {
> target_ulong curr_value = env->spr[SPR_POWER_MMCR0];
> @@ -143,6 +218,9 @@ void helper_store_mmcr0(CPUPPCState *env, target_ulong value)
> (curr_FC != new_FC)) {
> hreg_compute_hflags(env);
> }
> +
> + /* Update cycle overflow timers with the current MMCR0 state */
> + pmu_update_overflow_timers(env);
> }
>
> void helper_store_mmcr1(CPUPPCState *env, uint64_t value)
> @@ -164,6 +242,8 @@ void helper_store_pmc(CPUPPCState *env, uint32_t sprn, uint64_t value)
> pmu_update_cycles(env);
>
> env->spr[sprn] = value;
> +
> + pmc_update_overflow_timer(env, sprn);
> }
>
> static void fire_PMC_interrupt(PowerPCCPU *cpu)
--
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] 24+ messages in thread
* [PATCH v8 06/10] target/ppc: enable PMU instruction count
2021-11-25 15:08 [PATCH v8 00/10] PMU-EBB support for PPC64 TCG Daniel Henrique Barboza
` (4 preceding siblings ...)
2021-11-25 15:08 ` [PATCH v8 05/10] target/ppc: enable PMU counter overflow with cycle events Daniel Henrique Barboza
@ 2021-11-25 15:08 ` Daniel Henrique Barboza
2021-11-29 4:36 ` David Gibson
2021-11-25 15:08 ` [PATCH v8 07/10] target/ppc/power8-pmu.c: add PM_RUN_INST_CMPL (0xFA) event Daniel Henrique Barboza
` (3 subsequent siblings)
9 siblings, 1 reply; 24+ messages in thread
From: Daniel Henrique Barboza @ 2021-11-25 15:08 UTC (permalink / raw)
To: qemu-devel
Cc: richard.henderson, Daniel Henrique Barboza, qemu-ppc, clg, david
The PMU is already counting cycles by calculating time elapsed in
nanoseconds. Counting instructions is a different matter and requires
another approach.
This patch adds the capability of counting completed instructions
(Perf event PM_INST_CMPL) by counting the amount of instructions
translated in each translation block right before exiting it.
A new pmu_count_insns() helper in translation.c was added to do that.
After verifying that the PMU is running (MMCR0_FC bit not set), call
helper_insns_inc(). This new helper from power8-pmu.c will add the
instructions to the relevant counters. It'll also be responsible for
triggering counter negative overflows as it is already being done with
cycles.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
target/ppc/cpu.h | 1 +
target/ppc/helper.h | 1 +
target/ppc/helper_regs.c | 4 +++
target/ppc/power8-pmu-regs.c.inc | 6 +++++
target/ppc/power8-pmu.c | 38 ++++++++++++++++++++++++++
target/ppc/translate.c | 46 ++++++++++++++++++++++++++++++++
6 files changed, 96 insertions(+)
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 9b41b022e2..38cd2b5c43 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -656,6 +656,7 @@ enum {
HFLAGS_PR = 14, /* MSR_PR */
HFLAGS_PMCC0 = 15, /* MMCR0 PMCC bit 0 */
HFLAGS_PMCC1 = 16, /* MMCR0 PMCC bit 1 */
+ HFLAGS_MMCR0FC = 17, /* MMCR0 FC bit */
HFLAGS_VSX = 23, /* MSR_VSX if cpu has VSX */
HFLAGS_VR = 25, /* MSR_VR if cpu has VRE */
diff --git a/target/ppc/helper.h b/target/ppc/helper.h
index 94b4690375..d8a23e054a 100644
--- a/target/ppc/helper.h
+++ b/target/ppc/helper.h
@@ -24,6 +24,7 @@ DEF_HELPER_2(store_mmcr0, void, env, tl)
DEF_HELPER_2(store_mmcr1, void, env, tl)
DEF_HELPER_3(store_pmc, void, env, i32, i64)
DEF_HELPER_2(read_pmc, tl, env, i32)
+DEF_HELPER_2(insns_inc, void, env, i32)
#endif
DEF_HELPER_1(check_tlb_flush_local, void, env)
DEF_HELPER_1(check_tlb_flush_global, void, env)
diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
index 99562edd57..875c2fdfc6 100644
--- a/target/ppc/helper_regs.c
+++ b/target/ppc/helper_regs.c
@@ -115,6 +115,10 @@ static uint32_t hreg_compute_hflags_value(CPUPPCState *env)
if (env->spr[SPR_POWER_MMCR0] & MMCR0_PMCC1) {
hflags |= 1 << HFLAGS_PMCC1;
}
+ if (env->spr[SPR_POWER_MMCR0] & MMCR0_FC) {
+ hflags |= 1 << HFLAGS_MMCR0FC;
+ }
+
#ifndef CONFIG_USER_ONLY
if (!env->has_hv_mode || (msr & (1ull << MSR_HV))) {
diff --git a/target/ppc/power8-pmu-regs.c.inc b/target/ppc/power8-pmu-regs.c.inc
index 25b13ad564..580e4e41b2 100644
--- a/target/ppc/power8-pmu-regs.c.inc
+++ b/target/ppc/power8-pmu-regs.c.inc
@@ -113,6 +113,12 @@ static void write_MMCR0_common(DisasContext *ctx, TCGv val)
*/
gen_icount_io_start(ctx);
gen_helper_store_mmcr0(cpu_env, val);
+
+ /*
+ * End the translation block because MMCR0 writes can change
+ * ctx->pmu_frozen.
+ */
+ ctx->base.is_jmp = DISAS_EXIT_UPDATE;
}
void spr_write_MMCR0_ureg(DisasContext *ctx, int sprn, int gprn)
diff --git a/target/ppc/power8-pmu.c b/target/ppc/power8-pmu.c
index 01e0b9b8fc..59d0def79d 100644
--- a/target/ppc/power8-pmu.c
+++ b/target/ppc/power8-pmu.c
@@ -112,6 +112,30 @@ static PMUEventType pmc_get_event(CPUPPCState *env, int sprn)
return evt_type;
}
+static bool pmu_increment_insns(CPUPPCState *env, uint32_t num_insns)
+{
+ bool overflow_triggered = false;
+ int sprn;
+
+ /* PMC6 never counts instructions */
+ for (sprn = SPR_POWER_PMC1; sprn <= SPR_POWER_PMC5; sprn++) {
+ if (pmc_get_event(env, sprn) != PMU_EVENT_INSTRUCTIONS) {
+ continue;
+ }
+
+ env->spr[sprn] += num_insns;
+
+ if (env->spr[sprn] >= PMC_COUNTER_NEGATIVE_VAL &&
+ pmc_has_overflow_enabled(env, sprn)) {
+
+ overflow_triggered = true;
+ env->spr[sprn] = PMC_COUNTER_NEGATIVE_VAL;
+ }
+ }
+
+ return overflow_triggered;
+}
+
static void pmu_update_cycles(CPUPPCState *env)
{
uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
@@ -258,6 +282,20 @@ static void fire_PMC_interrupt(PowerPCCPU *cpu)
return;
}
+/* This helper assumes that the PMC is running. */
+void helper_insns_inc(CPUPPCState *env, uint32_t num_insns)
+{
+ bool overflow_triggered;
+ PowerPCCPU *cpu;
+
+ overflow_triggered = pmu_increment_insns(env, num_insns);
+
+ if (overflow_triggered) {
+ cpu = env_archcpu(env);
+ fire_PMC_interrupt(cpu);
+ }
+}
+
static void cpu_ppc_pmu_timer_cb(void *opaque)
{
PowerPCCPU *cpu = opaque;
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 9960df6e18..ccc83d0603 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -177,6 +177,7 @@ struct DisasContext {
bool hr;
bool mmcr0_pmcc0;
bool mmcr0_pmcc1;
+ bool pmu_frozen;
ppc_spr_t *spr_cb; /* Needed to check rights for mfspr/mtspr */
int singlestep_enabled;
uint32_t flags;
@@ -4170,6 +4171,31 @@ static inline void gen_update_cfar(DisasContext *ctx, target_ulong nip)
#endif
}
+#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
+static void pmu_count_insns(DisasContext *ctx)
+{
+ /* Do not bother calling the helper if the PMU is frozen */
+ if (ctx->pmu_frozen) {
+ return;
+ }
+
+ /*
+ * The PMU insns_inc() helper stops the internal PMU timer if a
+ * counter overflows happens. In that case, if the guest is
+ * running with icount and we do not handle it beforehand,
+ * the helper can trigger a 'bad icount read'.
+ */
+ gen_icount_io_start(ctx);
+
+ gen_helper_insns_inc(cpu_env, tcg_constant_i32(ctx->base.num_insns));
+}
+#else
+static void pmu_count_insns(DisasContext *ctx)
+{
+ return;
+}
+#endif
+
static inline bool use_goto_tb(DisasContext *ctx, target_ulong dest)
{
return translator_use_goto_tb(&ctx->base, dest);
@@ -4180,6 +4206,14 @@ static void gen_lookup_and_goto_ptr(DisasContext *ctx)
if (unlikely(ctx->singlestep_enabled)) {
gen_debug_exception(ctx);
} else {
+ /*
+ * tcg_gen_lookup_and_goto_ptr will exit the TB if
+ * CF_NO_GOTO_PTR is set. Count insns now.
+ */
+ if (ctx->base.tb->flags & CF_NO_GOTO_PTR) {
+ pmu_count_insns(ctx);
+ }
+
tcg_gen_lookup_and_goto_ptr();
}
}
@@ -4191,6 +4225,7 @@ static void gen_goto_tb(DisasContext *ctx, int n, target_ulong dest)
dest = (uint32_t) dest;
}
if (use_goto_tb(ctx, dest)) {
+ pmu_count_insns(ctx);
tcg_gen_goto_tb(n);
tcg_gen_movi_tl(cpu_nip, dest & ~3);
tcg_gen_exit_tb(ctx->base.tb, n);
@@ -8458,6 +8493,7 @@ static void ppc_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
ctx->hr = (hflags >> HFLAGS_HR) & 1;
ctx->mmcr0_pmcc0 = (hflags >> HFLAGS_PMCC0) & 1;
ctx->mmcr0_pmcc1 = (hflags >> HFLAGS_PMCC1) & 1;
+ ctx->pmu_frozen = (hflags >> HFLAGS_MMCR0FC) & 1;
ctx->singlestep_enabled = 0;
if ((hflags >> HFLAGS_SE) & 1) {
@@ -8564,6 +8600,7 @@ static void ppc_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs)
switch (is_jmp) {
case DISAS_TOO_MANY:
if (use_goto_tb(ctx, nip)) {
+ pmu_count_insns(ctx);
tcg_gen_goto_tb(0);
gen_update_nip(ctx, nip);
tcg_gen_exit_tb(ctx->base.tb, 0);
@@ -8574,6 +8611,14 @@ static void ppc_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs)
gen_update_nip(ctx, nip);
/* fall through */
case DISAS_CHAIN:
+ /*
+ * tcg_gen_lookup_and_goto_ptr will exit the TB if
+ * CF_NO_GOTO_PTR is set. Count insns now.
+ */
+ if (ctx->base.tb->flags & CF_NO_GOTO_PTR) {
+ pmu_count_insns(ctx);
+ }
+
tcg_gen_lookup_and_goto_ptr();
break;
@@ -8581,6 +8626,7 @@ static void ppc_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs)
gen_update_nip(ctx, nip);
/* fall through */
case DISAS_EXIT:
+ pmu_count_insns(ctx);
tcg_gen_exit_tb(NULL, 0);
break;
--
2.31.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v8 06/10] target/ppc: enable PMU instruction count
2021-11-25 15:08 ` [PATCH v8 06/10] target/ppc: enable PMU instruction count Daniel Henrique Barboza
@ 2021-11-29 4:36 ` David Gibson
2021-11-30 22:24 ` Daniel Henrique Barboza
0 siblings, 1 reply; 24+ messages in thread
From: David Gibson @ 2021-11-29 4:36 UTC (permalink / raw)
To: Daniel Henrique Barboza; +Cc: richard.henderson, qemu-ppc, qemu-devel, clg
[-- Attachment #1: Type: text/plain, Size: 9936 bytes --]
On Thu, Nov 25, 2021 at 12:08:13PM -0300, Daniel Henrique Barboza wrote:
> The PMU is already counting cycles by calculating time elapsed in
> nanoseconds. Counting instructions is a different matter and requires
> another approach.
>
> This patch adds the capability of counting completed instructions
> (Perf event PM_INST_CMPL) by counting the amount of instructions
> translated in each translation block right before exiting it.
>
> A new pmu_count_insns() helper in translation.c was added to do that.
> After verifying that the PMU is running (MMCR0_FC bit not set), call
> helper_insns_inc(). This new helper from power8-pmu.c will add the
> instructions to the relevant counters. It'll also be responsible for
> triggering counter negative overflows as it is already being done with
> cycles.
>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
> target/ppc/cpu.h | 1 +
> target/ppc/helper.h | 1 +
> target/ppc/helper_regs.c | 4 +++
> target/ppc/power8-pmu-regs.c.inc | 6 +++++
> target/ppc/power8-pmu.c | 38 ++++++++++++++++++++++++++
> target/ppc/translate.c | 46 ++++++++++++++++++++++++++++++++
> 6 files changed, 96 insertions(+)
>
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 9b41b022e2..38cd2b5c43 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -656,6 +656,7 @@ enum {
> HFLAGS_PR = 14, /* MSR_PR */
> HFLAGS_PMCC0 = 15, /* MMCR0 PMCC bit 0 */
> HFLAGS_PMCC1 = 16, /* MMCR0 PMCC bit 1 */
> + HFLAGS_MMCR0FC = 17, /* MMCR0 FC bit */
Now that the event stuff is a bit more refined, you could narrow this
down to specifically marking if any counters are actively counting
instructions (not frozen by MMCR0[FC] and not frozen by
MMCR0[FC14|FC56] *and* have the right event selected).
Since I suspect the instruction counting instrumentation could be
quite expensive (helper call on every tb), that might be worthwhile.
> HFLAGS_VSX = 23, /* MSR_VSX if cpu has VSX */
> HFLAGS_VR = 25, /* MSR_VR if cpu has VRE */
>
> diff --git a/target/ppc/helper.h b/target/ppc/helper.h
> index 94b4690375..d8a23e054a 100644
> --- a/target/ppc/helper.h
> +++ b/target/ppc/helper.h
> @@ -24,6 +24,7 @@ DEF_HELPER_2(store_mmcr0, void, env, tl)
> DEF_HELPER_2(store_mmcr1, void, env, tl)
> DEF_HELPER_3(store_pmc, void, env, i32, i64)
> DEF_HELPER_2(read_pmc, tl, env, i32)
> +DEF_HELPER_2(insns_inc, void, env, i32)
> #endif
> DEF_HELPER_1(check_tlb_flush_local, void, env)
> DEF_HELPER_1(check_tlb_flush_global, void, env)
> diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
> index 99562edd57..875c2fdfc6 100644
> --- a/target/ppc/helper_regs.c
> +++ b/target/ppc/helper_regs.c
> @@ -115,6 +115,10 @@ static uint32_t hreg_compute_hflags_value(CPUPPCState *env)
> if (env->spr[SPR_POWER_MMCR0] & MMCR0_PMCC1) {
> hflags |= 1 << HFLAGS_PMCC1;
> }
> + if (env->spr[SPR_POWER_MMCR0] & MMCR0_FC) {
> + hflags |= 1 << HFLAGS_MMCR0FC;
> + }
> +
>
> #ifndef CONFIG_USER_ONLY
> if (!env->has_hv_mode || (msr & (1ull << MSR_HV))) {
> diff --git a/target/ppc/power8-pmu-regs.c.inc b/target/ppc/power8-pmu-regs.c.inc
> index 25b13ad564..580e4e41b2 100644
> --- a/target/ppc/power8-pmu-regs.c.inc
> +++ b/target/ppc/power8-pmu-regs.c.inc
> @@ -113,6 +113,12 @@ static void write_MMCR0_common(DisasContext *ctx, TCGv val)
> */
> gen_icount_io_start(ctx);
> gen_helper_store_mmcr0(cpu_env, val);
> +
> + /*
> + * End the translation block because MMCR0 writes can change
> + * ctx->pmu_frozen.
> + */
> + ctx->base.is_jmp = DISAS_EXIT_UPDATE;
> }
>
> void spr_write_MMCR0_ureg(DisasContext *ctx, int sprn, int gprn)
> diff --git a/target/ppc/power8-pmu.c b/target/ppc/power8-pmu.c
> index 01e0b9b8fc..59d0def79d 100644
> --- a/target/ppc/power8-pmu.c
> +++ b/target/ppc/power8-pmu.c
> @@ -112,6 +112,30 @@ static PMUEventType pmc_get_event(CPUPPCState *env, int sprn)
> return evt_type;
> }
>
> +static bool pmu_increment_insns(CPUPPCState *env, uint32_t num_insns)
> +{
> + bool overflow_triggered = false;
> + int sprn;
> +
> + /* PMC6 never counts instructions */
> + for (sprn = SPR_POWER_PMC1; sprn <= SPR_POWER_PMC5; sprn++) {
> + if (pmc_get_event(env, sprn) != PMU_EVENT_INSTRUCTIONS) {
> + continue;
> + }
> +
> + env->spr[sprn] += num_insns;
> +
> + if (env->spr[sprn] >= PMC_COUNTER_NEGATIVE_VAL &&
> + pmc_has_overflow_enabled(env, sprn)) {
> +
> + overflow_triggered = true;
> + env->spr[sprn] = PMC_COUNTER_NEGATIVE_VAL;
Does the hardware PMU actually guarantee that the event will happen
exactly on the overflow? Or could you count a few into the negative
zone before the event is delivered?
> + }
> + }
> +
> + return overflow_triggered;
> +}
> +
> static void pmu_update_cycles(CPUPPCState *env)
> {
> uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> @@ -258,6 +282,20 @@ static void fire_PMC_interrupt(PowerPCCPU *cpu)
> return;
> }
>
> +/* This helper assumes that the PMC is running. */
> +void helper_insns_inc(CPUPPCState *env, uint32_t num_insns)
> +{
> + bool overflow_triggered;
> + PowerPCCPU *cpu;
> +
> + overflow_triggered = pmu_increment_insns(env, num_insns);
> +
> + if (overflow_triggered) {
> + cpu = env_archcpu(env);
> + fire_PMC_interrupt(cpu);
> + }
> +}
> +
> static void cpu_ppc_pmu_timer_cb(void *opaque)
> {
> PowerPCCPU *cpu = opaque;
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 9960df6e18..ccc83d0603 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -177,6 +177,7 @@ struct DisasContext {
> bool hr;
> bool mmcr0_pmcc0;
> bool mmcr0_pmcc1;
> + bool pmu_frozen;
> ppc_spr_t *spr_cb; /* Needed to check rights for mfspr/mtspr */
> int singlestep_enabled;
> uint32_t flags;
> @@ -4170,6 +4171,31 @@ static inline void gen_update_cfar(DisasContext *ctx, target_ulong nip)
> #endif
> }
>
> +#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
Should this actually be !CONFIG_USER_ONLY? IIUC there are
circumstances where userspace could access the PMU, including
instruction counting.
> +static void pmu_count_insns(DisasContext *ctx)
> +{
> + /* Do not bother calling the helper if the PMU is frozen */
> + if (ctx->pmu_frozen) {
> + return;
> + }
> +
> + /*
> + * The PMU insns_inc() helper stops the internal PMU timer if a
> + * counter overflows happens. In that case, if the guest is
> + * running with icount and we do not handle it beforehand,
> + * the helper can trigger a 'bad icount read'.
> + */
> + gen_icount_io_start(ctx);
> +
> + gen_helper_insns_inc(cpu_env, tcg_constant_i32(ctx->base.num_insns));
> +}
> +#else
> +static void pmu_count_insns(DisasContext *ctx)
> +{
> + return;
> +}
> +#endif
> +
> static inline bool use_goto_tb(DisasContext *ctx, target_ulong dest)
> {
> return translator_use_goto_tb(&ctx->base, dest);
> @@ -4180,6 +4206,14 @@ static void gen_lookup_and_goto_ptr(DisasContext *ctx)
> if (unlikely(ctx->singlestep_enabled)) {
> gen_debug_exception(ctx);
> } else {
> + /*
> + * tcg_gen_lookup_and_goto_ptr will exit the TB if
> + * CF_NO_GOTO_PTR is set. Count insns now.
> + */
> + if (ctx->base.tb->flags & CF_NO_GOTO_PTR) {
> + pmu_count_insns(ctx);
> + }
> +
> tcg_gen_lookup_and_goto_ptr();
> }
> }
> @@ -4191,6 +4225,7 @@ static void gen_goto_tb(DisasContext *ctx, int n, target_ulong dest)
> dest = (uint32_t) dest;
> }
> if (use_goto_tb(ctx, dest)) {
> + pmu_count_insns(ctx);
> tcg_gen_goto_tb(n);
> tcg_gen_movi_tl(cpu_nip, dest & ~3);
> tcg_gen_exit_tb(ctx->base.tb, n);
> @@ -8458,6 +8493,7 @@ static void ppc_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
> ctx->hr = (hflags >> HFLAGS_HR) & 1;
> ctx->mmcr0_pmcc0 = (hflags >> HFLAGS_PMCC0) & 1;
> ctx->mmcr0_pmcc1 = (hflags >> HFLAGS_PMCC1) & 1;
> + ctx->pmu_frozen = (hflags >> HFLAGS_MMCR0FC) & 1;
>
> ctx->singlestep_enabled = 0;
> if ((hflags >> HFLAGS_SE) & 1) {
> @@ -8564,6 +8600,7 @@ static void ppc_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs)
> switch (is_jmp) {
> case DISAS_TOO_MANY:
> if (use_goto_tb(ctx, nip)) {
> + pmu_count_insns(ctx);
> tcg_gen_goto_tb(0);
> gen_update_nip(ctx, nip);
> tcg_gen_exit_tb(ctx->base.tb, 0);
> @@ -8574,6 +8611,14 @@ static void ppc_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs)
> gen_update_nip(ctx, nip);
> /* fall through */
> case DISAS_CHAIN:
> + /*
> + * tcg_gen_lookup_and_goto_ptr will exit the TB if
> + * CF_NO_GOTO_PTR is set. Count insns now.
> + */
> + if (ctx->base.tb->flags & CF_NO_GOTO_PTR) {
> + pmu_count_insns(ctx);
> + }
> +
> tcg_gen_lookup_and_goto_ptr();
> break;
>
> @@ -8581,6 +8626,7 @@ static void ppc_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs)
> gen_update_nip(ctx, nip);
> /* fall through */
> case DISAS_EXIT:
> + pmu_count_insns(ctx);
> tcg_gen_exit_tb(NULL, 0);
> break;
>
--
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] 24+ messages in thread* Re: [PATCH v8 06/10] target/ppc: enable PMU instruction count
2021-11-29 4:36 ` David Gibson
@ 2021-11-30 22:24 ` Daniel Henrique Barboza
2021-11-30 23:52 ` David Gibson
0 siblings, 1 reply; 24+ messages in thread
From: Daniel Henrique Barboza @ 2021-11-30 22:24 UTC (permalink / raw)
To: David Gibson; +Cc: richard.henderson, qemu-ppc, qemu-devel, clg
On 11/29/21 01:36, David Gibson wrote:
> On Thu, Nov 25, 2021 at 12:08:13PM -0300, Daniel Henrique Barboza wrote:
>> The PMU is already counting cycles by calculating time elapsed in
>> nanoseconds. Counting instructions is a different matter and requires
>> another approach.
>>
>> This patch adds the capability of counting completed instructions
>> (Perf event PM_INST_CMPL) by counting the amount of instructions
>> translated in each translation block right before exiting it.
>>
>> A new pmu_count_insns() helper in translation.c was added to do that.
>> After verifying that the PMU is running (MMCR0_FC bit not set), call
>> helper_insns_inc(). This new helper from power8-pmu.c will add the
>> instructions to the relevant counters. It'll also be responsible for
>> triggering counter negative overflows as it is already being done with
>> cycles.
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
>> target/ppc/cpu.h | 1 +
>> target/ppc/helper.h | 1 +
>> target/ppc/helper_regs.c | 4 +++
>> target/ppc/power8-pmu-regs.c.inc | 6 +++++
>> target/ppc/power8-pmu.c | 38 ++++++++++++++++++++++++++
>> target/ppc/translate.c | 46 ++++++++++++++++++++++++++++++++
>> 6 files changed, 96 insertions(+)
>>
>> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
>> index 9b41b022e2..38cd2b5c43 100644
>> --- a/target/ppc/cpu.h
>> +++ b/target/ppc/cpu.h
>> @@ -656,6 +656,7 @@ enum {
>> HFLAGS_PR = 14, /* MSR_PR */
>> HFLAGS_PMCC0 = 15, /* MMCR0 PMCC bit 0 */
>> HFLAGS_PMCC1 = 16, /* MMCR0 PMCC bit 1 */
>> + HFLAGS_MMCR0FC = 17, /* MMCR0 FC bit */
>
> Now that the event stuff is a bit more refined, you could narrow this
> down to specifically marking if any counters are actively counting
> instructions (not frozen by MMCR0[FC] and not frozen by
> MMCR0[FC14|FC56] *and* have the right event selected).
>
> Since I suspect the instruction counting instrumentation could be
> quite expensive (helper call on every tb), that might be worthwhile.
That was worthwhile. The performance increase is substantial with this
change, in particular with tests that exercises only cycle events.
>
>> HFLAGS_VSX = 23, /* MSR_VSX if cpu has VSX */
>> HFLAGS_VR = 25, /* MSR_VR if cpu has VRE */
>>
>> diff --git a/target/ppc/helper.h b/target/ppc/helper.h
>> index 94b4690375..d8a23e054a 100644
>> --- a/target/ppc/helper.h
>> +++ b/target/ppc/helper.h
>> @@ -24,6 +24,7 @@ DEF_HELPER_2(store_mmcr0, void, env, tl)
>> DEF_HELPER_2(store_mmcr1, void, env, tl)
>> DEF_HELPER_3(store_pmc, void, env, i32, i64)
>> DEF_HELPER_2(read_pmc, tl, env, i32)
>> +DEF_HELPER_2(insns_inc, void, env, i32)
>> #endif
>> DEF_HELPER_1(check_tlb_flush_local, void, env)
>> DEF_HELPER_1(check_tlb_flush_global, void, env)
>> diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
>> index 99562edd57..875c2fdfc6 100644
>> --- a/target/ppc/helper_regs.c
>> +++ b/target/ppc/helper_regs.c
>> @@ -115,6 +115,10 @@ static uint32_t hreg_compute_hflags_value(CPUPPCState *env)
>> if (env->spr[SPR_POWER_MMCR0] & MMCR0_PMCC1) {
>> hflags |= 1 << HFLAGS_PMCC1;
>> }
>> + if (env->spr[SPR_POWER_MMCR0] & MMCR0_FC) {
>> + hflags |= 1 << HFLAGS_MMCR0FC;
>> + }
>> +
>>
>> #ifndef CONFIG_USER_ONLY
>> if (!env->has_hv_mode || (msr & (1ull << MSR_HV))) {
>> diff --git a/target/ppc/power8-pmu-regs.c.inc b/target/ppc/power8-pmu-regs.c.inc
>> index 25b13ad564..580e4e41b2 100644
>> --- a/target/ppc/power8-pmu-regs.c.inc
>> +++ b/target/ppc/power8-pmu-regs.c.inc
>> @@ -113,6 +113,12 @@ static void write_MMCR0_common(DisasContext *ctx, TCGv val)
>> */
>> gen_icount_io_start(ctx);
>> gen_helper_store_mmcr0(cpu_env, val);
>> +
>> + /*
>> + * End the translation block because MMCR0 writes can change
>> + * ctx->pmu_frozen.
>> + */
>> + ctx->base.is_jmp = DISAS_EXIT_UPDATE;
>> }
>>
>> void spr_write_MMCR0_ureg(DisasContext *ctx, int sprn, int gprn)
>> diff --git a/target/ppc/power8-pmu.c b/target/ppc/power8-pmu.c
>> index 01e0b9b8fc..59d0def79d 100644
>> --- a/target/ppc/power8-pmu.c
>> +++ b/target/ppc/power8-pmu.c
>> @@ -112,6 +112,30 @@ static PMUEventType pmc_get_event(CPUPPCState *env, int sprn)
>> return evt_type;
>> }
>>
>> +static bool pmu_increment_insns(CPUPPCState *env, uint32_t num_insns)
>> +{
>> + bool overflow_triggered = false;
>> + int sprn;
>> +
>> + /* PMC6 never counts instructions */
>> + for (sprn = SPR_POWER_PMC1; sprn <= SPR_POWER_PMC5; sprn++) {
>> + if (pmc_get_event(env, sprn) != PMU_EVENT_INSTRUCTIONS) {
>> + continue;
>> + }
>> +
>> + env->spr[sprn] += num_insns;
>> +
>> + if (env->spr[sprn] >= PMC_COUNTER_NEGATIVE_VAL &&
>> + pmc_has_overflow_enabled(env, sprn)) {
>> +
>> + overflow_triggered = true;
>> + env->spr[sprn] = PMC_COUNTER_NEGATIVE_VAL;
>
> Does the hardware PMU actually guarantee that the event will happen
> exactly on the overflow? Or could you count a few into the negative
> zone before the event is delivered?
My understand reading the ISA and from testing with the a real PMU is that yes,
it'll guarantee that the overflow will happen when the counter reaches exactly
0x80000000.
>
>> + }
>> + }
>> +
>> + return overflow_triggered;
>> +}
>> +
>> static void pmu_update_cycles(CPUPPCState *env)
>> {
>> uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>> @@ -258,6 +282,20 @@ static void fire_PMC_interrupt(PowerPCCPU *cpu)
>> return;
>> }
>>
>> +/* This helper assumes that the PMC is running. */
>> +void helper_insns_inc(CPUPPCState *env, uint32_t num_insns)
>> +{
>> + bool overflow_triggered;
>> + PowerPCCPU *cpu;
>> +
>> + overflow_triggered = pmu_increment_insns(env, num_insns);
>> +
>> + if (overflow_triggered) {
>> + cpu = env_archcpu(env);
>> + fire_PMC_interrupt(cpu);
>> + }
>> +}
>> +
>> static void cpu_ppc_pmu_timer_cb(void *opaque)
>> {
>> PowerPCCPU *cpu = opaque;
>> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
>> index 9960df6e18..ccc83d0603 100644
>> --- a/target/ppc/translate.c
>> +++ b/target/ppc/translate.c
>> @@ -177,6 +177,7 @@ struct DisasContext {
>> bool hr;
>> bool mmcr0_pmcc0;
>> bool mmcr0_pmcc1;
>> + bool pmu_frozen;
>> ppc_spr_t *spr_cb; /* Needed to check rights for mfspr/mtspr */
>> int singlestep_enabled;
>> uint32_t flags;
>> @@ -4170,6 +4171,31 @@ static inline void gen_update_cfar(DisasContext *ctx, target_ulong nip)
>> #endif
>> }
>>
>> +#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
>
> Should this actually be !CONFIG_USER_ONLY? IIUC there are
> circumstances where userspace could access the PMU, including
> instruction counting.
The user mode will not be able to use the PMU properly because the MMCR1
reg, used to define the events to be sampled, isn't writable by userpace
under any circunstance.
Thanks,
Daniel
>
>> +static void pmu_count_insns(DisasContext *ctx)
>> +{
>> + /* Do not bother calling the helper if the PMU is frozen */
>> + if (ctx->pmu_frozen) {
>> + return;
>> + }
>> +
>> + /*
>> + * The PMU insns_inc() helper stops the internal PMU timer if a
>> + * counter overflows happens. In that case, if the guest is
>> + * running with icount and we do not handle it beforehand,
>> + * the helper can trigger a 'bad icount read'.
>> + */
>> + gen_icount_io_start(ctx);
>> +
>> + gen_helper_insns_inc(cpu_env, tcg_constant_i32(ctx->base.num_insns));
>> +}
>> +#else
>> +static void pmu_count_insns(DisasContext *ctx)
>> +{
>> + return;
>> +}
>> +#endif
>> +
>> static inline bool use_goto_tb(DisasContext *ctx, target_ulong dest)
>> {
>> return translator_use_goto_tb(&ctx->base, dest);
>> @@ -4180,6 +4206,14 @@ static void gen_lookup_and_goto_ptr(DisasContext *ctx)
>> if (unlikely(ctx->singlestep_enabled)) {
>> gen_debug_exception(ctx);
>> } else {
>> + /*
>> + * tcg_gen_lookup_and_goto_ptr will exit the TB if
>> + * CF_NO_GOTO_PTR is set. Count insns now.
>> + */
>> + if (ctx->base.tb->flags & CF_NO_GOTO_PTR) {
>> + pmu_count_insns(ctx);
>> + }
>> +
>> tcg_gen_lookup_and_goto_ptr();
>> }
>> }
>> @@ -4191,6 +4225,7 @@ static void gen_goto_tb(DisasContext *ctx, int n, target_ulong dest)
>> dest = (uint32_t) dest;
>> }
>> if (use_goto_tb(ctx, dest)) {
>> + pmu_count_insns(ctx);
>> tcg_gen_goto_tb(n);
>> tcg_gen_movi_tl(cpu_nip, dest & ~3);
>> tcg_gen_exit_tb(ctx->base.tb, n);
>> @@ -8458,6 +8493,7 @@ static void ppc_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
>> ctx->hr = (hflags >> HFLAGS_HR) & 1;
>> ctx->mmcr0_pmcc0 = (hflags >> HFLAGS_PMCC0) & 1;
>> ctx->mmcr0_pmcc1 = (hflags >> HFLAGS_PMCC1) & 1;
>> + ctx->pmu_frozen = (hflags >> HFLAGS_MMCR0FC) & 1;
>>
>> ctx->singlestep_enabled = 0;
>> if ((hflags >> HFLAGS_SE) & 1) {
>> @@ -8564,6 +8600,7 @@ static void ppc_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs)
>> switch (is_jmp) {
>> case DISAS_TOO_MANY:
>> if (use_goto_tb(ctx, nip)) {
>> + pmu_count_insns(ctx);
>> tcg_gen_goto_tb(0);
>> gen_update_nip(ctx, nip);
>> tcg_gen_exit_tb(ctx->base.tb, 0);
>> @@ -8574,6 +8611,14 @@ static void ppc_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs)
>> gen_update_nip(ctx, nip);
>> /* fall through */
>> case DISAS_CHAIN:
>> + /*
>> + * tcg_gen_lookup_and_goto_ptr will exit the TB if
>> + * CF_NO_GOTO_PTR is set. Count insns now.
>> + */
>> + if (ctx->base.tb->flags & CF_NO_GOTO_PTR) {
>> + pmu_count_insns(ctx);
>> + }
>> +
>> tcg_gen_lookup_and_goto_ptr();
>> break;
>>
>> @@ -8581,6 +8626,7 @@ static void ppc_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs)
>> gen_update_nip(ctx, nip);
>> /* fall through */
>> case DISAS_EXIT:
>> + pmu_count_insns(ctx);
>> tcg_gen_exit_tb(NULL, 0);
>> break;
>>
>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v8 06/10] target/ppc: enable PMU instruction count
2021-11-30 22:24 ` Daniel Henrique Barboza
@ 2021-11-30 23:52 ` David Gibson
2021-12-01 13:12 ` Daniel Henrique Barboza
0 siblings, 1 reply; 24+ messages in thread
From: David Gibson @ 2021-11-30 23:52 UTC (permalink / raw)
To: Daniel Henrique Barboza; +Cc: richard.henderson, qemu-ppc, qemu-devel, clg
[-- Attachment #1: Type: text/plain, Size: 8257 bytes --]
On Tue, Nov 30, 2021 at 07:24:04PM -0300, Daniel Henrique Barboza wrote:
>
>
> On 11/29/21 01:36, David Gibson wrote:
> > On Thu, Nov 25, 2021 at 12:08:13PM -0300, Daniel Henrique Barboza wrote:
> > > The PMU is already counting cycles by calculating time elapsed in
> > > nanoseconds. Counting instructions is a different matter and requires
> > > another approach.
> > >
> > > This patch adds the capability of counting completed instructions
> > > (Perf event PM_INST_CMPL) by counting the amount of instructions
> > > translated in each translation block right before exiting it.
> > >
> > > A new pmu_count_insns() helper in translation.c was added to do that.
> > > After verifying that the PMU is running (MMCR0_FC bit not set), call
> > > helper_insns_inc(). This new helper from power8-pmu.c will add the
> > > instructions to the relevant counters. It'll also be responsible for
> > > triggering counter negative overflows as it is already being done with
> > > cycles.
> > >
> > > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> > > ---
> > > target/ppc/cpu.h | 1 +
> > > target/ppc/helper.h | 1 +
> > > target/ppc/helper_regs.c | 4 +++
> > > target/ppc/power8-pmu-regs.c.inc | 6 +++++
> > > target/ppc/power8-pmu.c | 38 ++++++++++++++++++++++++++
> > > target/ppc/translate.c | 46 ++++++++++++++++++++++++++++++++
> > > 6 files changed, 96 insertions(+)
> > >
> > > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> > > index 9b41b022e2..38cd2b5c43 100644
> > > --- a/target/ppc/cpu.h
> > > +++ b/target/ppc/cpu.h
> > > @@ -656,6 +656,7 @@ enum {
> > > HFLAGS_PR = 14, /* MSR_PR */
> > > HFLAGS_PMCC0 = 15, /* MMCR0 PMCC bit 0 */
> > > HFLAGS_PMCC1 = 16, /* MMCR0 PMCC bit 1 */
> > > + HFLAGS_MMCR0FC = 17, /* MMCR0 FC bit */
> >
> > Now that the event stuff is a bit more refined, you could narrow this
> > down to specifically marking if any counters are actively counting
> > instructions (not frozen by MMCR0[FC] and not frozen by
> > MMCR0[FC14|FC56] *and* have the right event selected).
> >
> > Since I suspect the instruction counting instrumentation could be
> > quite expensive (helper call on every tb), that might be worthwhile.
>
> That was worthwhile. The performance increase is substantial with this
> change, in particular with tests that exercises only cycle events.
Good to know.
> > > HFLAGS_VSX = 23, /* MSR_VSX if cpu has VSX */
> > > HFLAGS_VR = 25, /* MSR_VR if cpu has VRE */
> > > diff --git a/target/ppc/helper.h b/target/ppc/helper.h
> > > index 94b4690375..d8a23e054a 100644
> > > --- a/target/ppc/helper.h
> > > +++ b/target/ppc/helper.h
> > > @@ -24,6 +24,7 @@ DEF_HELPER_2(store_mmcr0, void, env, tl)
> > > DEF_HELPER_2(store_mmcr1, void, env, tl)
> > > DEF_HELPER_3(store_pmc, void, env, i32, i64)
> > > DEF_HELPER_2(read_pmc, tl, env, i32)
> > > +DEF_HELPER_2(insns_inc, void, env, i32)
> > > #endif
> > > DEF_HELPER_1(check_tlb_flush_local, void, env)
> > > DEF_HELPER_1(check_tlb_flush_global, void, env)
> > > diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
> > > index 99562edd57..875c2fdfc6 100644
> > > --- a/target/ppc/helper_regs.c
> > > +++ b/target/ppc/helper_regs.c
> > > @@ -115,6 +115,10 @@ static uint32_t hreg_compute_hflags_value(CPUPPCState *env)
> > > if (env->spr[SPR_POWER_MMCR0] & MMCR0_PMCC1) {
> > > hflags |= 1 << HFLAGS_PMCC1;
> > > }
> > > + if (env->spr[SPR_POWER_MMCR0] & MMCR0_FC) {
> > > + hflags |= 1 << HFLAGS_MMCR0FC;
> > > + }
> > > +
> > > #ifndef CONFIG_USER_ONLY
> > > if (!env->has_hv_mode || (msr & (1ull << MSR_HV))) {
> > > diff --git a/target/ppc/power8-pmu-regs.c.inc b/target/ppc/power8-pmu-regs.c.inc
> > > index 25b13ad564..580e4e41b2 100644
> > > --- a/target/ppc/power8-pmu-regs.c.inc
> > > +++ b/target/ppc/power8-pmu-regs.c.inc
> > > @@ -113,6 +113,12 @@ static void write_MMCR0_common(DisasContext *ctx, TCGv val)
> > > */
> > > gen_icount_io_start(ctx);
> > > gen_helper_store_mmcr0(cpu_env, val);
> > > +
> > > + /*
> > > + * End the translation block because MMCR0 writes can change
> > > + * ctx->pmu_frozen.
> > > + */
> > > + ctx->base.is_jmp = DISAS_EXIT_UPDATE;
> > > }
> > > void spr_write_MMCR0_ureg(DisasContext *ctx, int sprn, int gprn)
> > > diff --git a/target/ppc/power8-pmu.c b/target/ppc/power8-pmu.c
> > > index 01e0b9b8fc..59d0def79d 100644
> > > --- a/target/ppc/power8-pmu.c
> > > +++ b/target/ppc/power8-pmu.c
> > > @@ -112,6 +112,30 @@ static PMUEventType pmc_get_event(CPUPPCState *env, int sprn)
> > > return evt_type;
> > > }
> > > +static bool pmu_increment_insns(CPUPPCState *env, uint32_t num_insns)
> > > +{
> > > + bool overflow_triggered = false;
> > > + int sprn;
> > > +
> > > + /* PMC6 never counts instructions */
> > > + for (sprn = SPR_POWER_PMC1; sprn <= SPR_POWER_PMC5; sprn++) {
> > > + if (pmc_get_event(env, sprn) != PMU_EVENT_INSTRUCTIONS) {
> > > + continue;
> > > + }
> > > +
> > > + env->spr[sprn] += num_insns;
> > > +
> > > + if (env->spr[sprn] >= PMC_COUNTER_NEGATIVE_VAL &&
> > > + pmc_has_overflow_enabled(env, sprn)) {
> > > +
> > > + overflow_triggered = true;
> > > + env->spr[sprn] = PMC_COUNTER_NEGATIVE_VAL;
> >
> > Does the hardware PMU actually guarantee that the event will happen
> > exactly on the overflow? Or could you count a few into the negative
> > zone before the event is delivered?
>
> My understand reading the ISA and from testing with the a real PMU is that yes,
> it'll guarantee that the overflow will happen when the counter reaches exactly
> 0x80000000.
Ok. We can't quite achieve that in TCG, which makes forcing the
counter to 0x8000000 a reasonable way of faking it. Might be worth
commenting that that's what this is, though.
> > > + }
> > > + }
> > > +
> > > + return overflow_triggered;
> > > +}
> > > +
> > > static void pmu_update_cycles(CPUPPCState *env)
> > > {
> > > uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> > > @@ -258,6 +282,20 @@ static void fire_PMC_interrupt(PowerPCCPU *cpu)
> > > return;
> > > }
> > > +/* This helper assumes that the PMC is running. */
> > > +void helper_insns_inc(CPUPPCState *env, uint32_t num_insns)
> > > +{
> > > + bool overflow_triggered;
> > > + PowerPCCPU *cpu;
> > > +
> > > + overflow_triggered = pmu_increment_insns(env, num_insns);
> > > +
> > > + if (overflow_triggered) {
> > > + cpu = env_archcpu(env);
> > > + fire_PMC_interrupt(cpu);
> > > + }
> > > +}
> > > +
> > > static void cpu_ppc_pmu_timer_cb(void *opaque)
> > > {
> > > PowerPCCPU *cpu = opaque;
> > > diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> > > index 9960df6e18..ccc83d0603 100644
> > > --- a/target/ppc/translate.c
> > > +++ b/target/ppc/translate.c
> > > @@ -177,6 +177,7 @@ struct DisasContext {
> > > bool hr;
> > > bool mmcr0_pmcc0;
> > > bool mmcr0_pmcc1;
> > > + bool pmu_frozen;
> > > ppc_spr_t *spr_cb; /* Needed to check rights for mfspr/mtspr */
> > > int singlestep_enabled;
> > > uint32_t flags;
> > > @@ -4170,6 +4171,31 @@ static inline void gen_update_cfar(DisasContext *ctx, target_ulong nip)
> > > #endif
> > > }
> > > +#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
> >
> > Should this actually be !CONFIG_USER_ONLY? IIUC there are
> > circumstances where userspace could access the PMU, including
> > instruction counting.
>
> The user mode will not be able to use the PMU properly because the MMCR1
> reg, used to define the events to be sampled, isn't writable by userpace
> under any circunstance.
Couldn't they use PMC5 without writing MMCR1?
--
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] 24+ messages in thread* Re: [PATCH v8 06/10] target/ppc: enable PMU instruction count
2021-11-30 23:52 ` David Gibson
@ 2021-12-01 13:12 ` Daniel Henrique Barboza
2021-12-02 1:23 ` David Gibson
0 siblings, 1 reply; 24+ messages in thread
From: Daniel Henrique Barboza @ 2021-12-01 13:12 UTC (permalink / raw)
To: David Gibson; +Cc: richard.henderson, qemu-ppc, qemu-devel, clg
On 11/30/21 20:52, David Gibson wrote:
> On Tue, Nov 30, 2021 at 07:24:04PM -0300, Daniel Henrique Barboza wrote:
>>
>>
>> On 11/29/21 01:36, David Gibson wrote:
>>> On Thu, Nov 25, 2021 at 12:08:13PM -0300, Daniel Henrique Barboza wrote:
>>>> The PMU is already counting cycles by calculating time elapsed in
>>>> nanoseconds. Counting instructions is a different matter and requires
>>>> another approach.
>>>>
>>>> This patch adds the capability of counting completed instructions
>>>> (Perf event PM_INST_CMPL) by counting the amount of instructions
>>>> translated in each translation block right before exiting it.
>>>>
>>>> A new pmu_count_insns() helper in translation.c was added to do that.
>>>> After verifying that the PMU is running (MMCR0_FC bit not set), call
>>>> helper_insns_inc(). This new helper from power8-pmu.c will add the
>>>> instructions to the relevant counters. It'll also be responsible for
>>>> triggering counter negative overflows as it is already being done with
>>>> cycles.
>>>>
>>>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>>> ---
>>>> target/ppc/cpu.h | 1 +
>>>> target/ppc/helper.h | 1 +
>>>> target/ppc/helper_regs.c | 4 +++
>>>> target/ppc/power8-pmu-regs.c.inc | 6 +++++
>>>> target/ppc/power8-pmu.c | 38 ++++++++++++++++++++++++++
>>>> target/ppc/translate.c | 46 ++++++++++++++++++++++++++++++++
>>>> 6 files changed, 96 insertions(+)
>>>>
>>>> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
>>>> index 9b41b022e2..38cd2b5c43 100644
>>>> --- a/target/ppc/cpu.h
>>>> +++ b/target/ppc/cpu.h
>>>> @@ -656,6 +656,7 @@ enum {
>>>> HFLAGS_PR = 14, /* MSR_PR */
>>>> HFLAGS_PMCC0 = 15, /* MMCR0 PMCC bit 0 */
>>>> HFLAGS_PMCC1 = 16, /* MMCR0 PMCC bit 1 */
>>>> + HFLAGS_MMCR0FC = 17, /* MMCR0 FC bit */
>>>
>>> Now that the event stuff is a bit more refined, you could narrow this
>>> down to specifically marking if any counters are actively counting
>>> instructions (not frozen by MMCR0[FC] and not frozen by
>>> MMCR0[FC14|FC56] *and* have the right event selected).
>>>
>>> Since I suspect the instruction counting instrumentation could be
>>> quite expensive (helper call on every tb), that might be worthwhile.
>>
>> That was worthwhile. The performance increase is substantial with this
>> change, in particular with tests that exercises only cycle events.
>
> Good to know.
>
>>>> HFLAGS_VSX = 23, /* MSR_VSX if cpu has VSX */
>>>> HFLAGS_VR = 25, /* MSR_VR if cpu has VRE */
>>>> diff --git a/target/ppc/helper.h b/target/ppc/helper.h
>>>> index 94b4690375..d8a23e054a 100644
>>>> --- a/target/ppc/helper.h
>>>> +++ b/target/ppc/helper.h
>>>> @@ -24,6 +24,7 @@ DEF_HELPER_2(store_mmcr0, void, env, tl)
>>>> DEF_HELPER_2(store_mmcr1, void, env, tl)
>>>> DEF_HELPER_3(store_pmc, void, env, i32, i64)
>>>> DEF_HELPER_2(read_pmc, tl, env, i32)
>>>> +DEF_HELPER_2(insns_inc, void, env, i32)
>>>> #endif
>>>> DEF_HELPER_1(check_tlb_flush_local, void, env)
>>>> DEF_HELPER_1(check_tlb_flush_global, void, env)
>>>> diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
>>>> index 99562edd57..875c2fdfc6 100644
>>>> --- a/target/ppc/helper_regs.c
>>>> +++ b/target/ppc/helper_regs.c
>>>> @@ -115,6 +115,10 @@ static uint32_t hreg_compute_hflags_value(CPUPPCState *env)
>>>> if (env->spr[SPR_POWER_MMCR0] & MMCR0_PMCC1) {
>>>> hflags |= 1 << HFLAGS_PMCC1;
>>>> }
>>>> + if (env->spr[SPR_POWER_MMCR0] & MMCR0_FC) {
>>>> + hflags |= 1 << HFLAGS_MMCR0FC;
>>>> + }
>>>> +
>>>> #ifndef CONFIG_USER_ONLY
>>>> if (!env->has_hv_mode || (msr & (1ull << MSR_HV))) {
>>>> diff --git a/target/ppc/power8-pmu-regs.c.inc b/target/ppc/power8-pmu-regs.c.inc
>>>> index 25b13ad564..580e4e41b2 100644
>>>> --- a/target/ppc/power8-pmu-regs.c.inc
>>>> +++ b/target/ppc/power8-pmu-regs.c.inc
>>>> @@ -113,6 +113,12 @@ static void write_MMCR0_common(DisasContext *ctx, TCGv val)
>>>> */
>>>> gen_icount_io_start(ctx);
>>>> gen_helper_store_mmcr0(cpu_env, val);
>>>> +
>>>> + /*
>>>> + * End the translation block because MMCR0 writes can change
>>>> + * ctx->pmu_frozen.
>>>> + */
>>>> + ctx->base.is_jmp = DISAS_EXIT_UPDATE;
>>>> }
>>>> void spr_write_MMCR0_ureg(DisasContext *ctx, int sprn, int gprn)
>>>> diff --git a/target/ppc/power8-pmu.c b/target/ppc/power8-pmu.c
>>>> index 01e0b9b8fc..59d0def79d 100644
>>>> --- a/target/ppc/power8-pmu.c
>>>> +++ b/target/ppc/power8-pmu.c
>>>> @@ -112,6 +112,30 @@ static PMUEventType pmc_get_event(CPUPPCState *env, int sprn)
>>>> return evt_type;
>>>> }
>>>> +static bool pmu_increment_insns(CPUPPCState *env, uint32_t num_insns)
>>>> +{
>>>> + bool overflow_triggered = false;
>>>> + int sprn;
>>>> +
>>>> + /* PMC6 never counts instructions */
>>>> + for (sprn = SPR_POWER_PMC1; sprn <= SPR_POWER_PMC5; sprn++) {
>>>> + if (pmc_get_event(env, sprn) != PMU_EVENT_INSTRUCTIONS) {
>>>> + continue;
>>>> + }
>>>> +
>>>> + env->spr[sprn] += num_insns;
>>>> +
>>>> + if (env->spr[sprn] >= PMC_COUNTER_NEGATIVE_VAL &&
>>>> + pmc_has_overflow_enabled(env, sprn)) {
>>>> +
>>>> + overflow_triggered = true;
>>>> + env->spr[sprn] = PMC_COUNTER_NEGATIVE_VAL;
>>>
>>> Does the hardware PMU actually guarantee that the event will happen
>>> exactly on the overflow? Or could you count a few into the negative
>>> zone before the event is delivered?
>>
>> My understand reading the ISA and from testing with the a real PMU is that yes,
>> it'll guarantee that the overflow will happen when the counter reaches exactly
>> 0x80000000.
>
> Ok. We can't quite achieve that in TCG, which makes forcing the
> counter to 0x8000000 a reasonable way of faking it. Might be worth
> commenting that that's what this is, though.
Fair enough.
>
>>>> + }
>>>> + }
>>>> +
>>>> + return overflow_triggered;
>>>> +}
>>>> +
>>>> static void pmu_update_cycles(CPUPPCState *env)
>>>> {
>>>> uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>>>> @@ -258,6 +282,20 @@ static void fire_PMC_interrupt(PowerPCCPU *cpu)
>>>> return;
>>>> }
>>>> +/* This helper assumes that the PMC is running. */
>>>> +void helper_insns_inc(CPUPPCState *env, uint32_t num_insns)
>>>> +{
>>>> + bool overflow_triggered;
>>>> + PowerPCCPU *cpu;
>>>> +
>>>> + overflow_triggered = pmu_increment_insns(env, num_insns);
>>>> +
>>>> + if (overflow_triggered) {
>>>> + cpu = env_archcpu(env);
>>>> + fire_PMC_interrupt(cpu);
>>>> + }
>>>> +}
>>>> +
>>>> static void cpu_ppc_pmu_timer_cb(void *opaque)
>>>> {
>>>> PowerPCCPU *cpu = opaque;
>>>> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
>>>> index 9960df6e18..ccc83d0603 100644
>>>> --- a/target/ppc/translate.c
>>>> +++ b/target/ppc/translate.c
>>>> @@ -177,6 +177,7 @@ struct DisasContext {
>>>> bool hr;
>>>> bool mmcr0_pmcc0;
>>>> bool mmcr0_pmcc1;
>>>> + bool pmu_frozen;
>>>> ppc_spr_t *spr_cb; /* Needed to check rights for mfspr/mtspr */
>>>> int singlestep_enabled;
>>>> uint32_t flags;
>>>> @@ -4170,6 +4171,31 @@ static inline void gen_update_cfar(DisasContext *ctx, target_ulong nip)
>>>> #endif
>>>> }
>>>> +#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
>>>
>>> Should this actually be !CONFIG_USER_ONLY? IIUC there are
>>> circumstances where userspace could access the PMU, including
>>> instruction counting.
>>
>> The user mode will not be able to use the PMU properly because the MMCR1
>> reg, used to define the events to be sampled, isn't writable by userpace
>> under any circunstance.
>
> Couldn't they use PMC5 without writing MMCR1?
Yeah, in theory. Problem state write access to PMCs are granted for MMCR0_PMCC
0b10 || 0b11. The PMCC bits of MMCR0 aren't user read/writable (only FC, PMAO and PMAE
bits can be r/w from userspace, all other bits are filtered out). With the default
PMCC value of 0b00 the PMCs are readable, but not writable. So in a way userspace can
start the PMU and see instruction counting in PMC5 but it wouldn't be able to set it
to a specific value and wouldn't be able to use overflows.
All that said, the change to allow PMC5 to be incremented in problem state is simple
enough so I ended up doing it.
Thanks,
Daniel
>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v8 06/10] target/ppc: enable PMU instruction count
2021-12-01 13:12 ` Daniel Henrique Barboza
@ 2021-12-02 1:23 ` David Gibson
0 siblings, 0 replies; 24+ messages in thread
From: David Gibson @ 2021-12-02 1:23 UTC (permalink / raw)
To: Daniel Henrique Barboza; +Cc: richard.henderson, qemu-ppc, qemu-devel, clg
[-- Attachment #1: Type: text/plain, Size: 6179 bytes --]
On Wed, Dec 01, 2021 at 10:12:27AM -0300, Daniel Henrique Barboza wrote:
> On 11/30/21 20:52, David Gibson wrote:
> > On Tue, Nov 30, 2021 at 07:24:04PM -0300, Daniel Henrique Barboza wrote:
[snip]
> > > > > +static bool pmu_increment_insns(CPUPPCState *env, uint32_t num_insns)
> > > > > +{
> > > > > + bool overflow_triggered = false;
> > > > > + int sprn;
> > > > > +
> > > > > + /* PMC6 never counts instructions */
> > > > > + for (sprn = SPR_POWER_PMC1; sprn <= SPR_POWER_PMC5; sprn++) {
> > > > > + if (pmc_get_event(env, sprn) != PMU_EVENT_INSTRUCTIONS) {
> > > > > + continue;
> > > > > + }
> > > > > +
> > > > > + env->spr[sprn] += num_insns;
> > > > > +
> > > > > + if (env->spr[sprn] >= PMC_COUNTER_NEGATIVE_VAL &&
> > > > > + pmc_has_overflow_enabled(env, sprn)) {
> > > > > +
> > > > > + overflow_triggered = true;
> > > > > + env->spr[sprn] = PMC_COUNTER_NEGATIVE_VAL;
> > > >
> > > > Does the hardware PMU actually guarantee that the event will happen
> > > > exactly on the overflow? Or could you count a few into the negative
> > > > zone before the event is delivered?
> > >
> > > My understand reading the ISA and from testing with the a real PMU is that yes,
> > > it'll guarantee that the overflow will happen when the counter reaches exactly
> > > 0x80000000.
> >
> > Ok. We can't quite achieve that in TCG, which makes forcing the
> > counter to 0x8000000 a reasonable way of faking it. Might be worth
> > commenting that that's what this is, though.
>
> Fair enough.
To expand on this a little, I mentioned in connection with cycle
counting that we kind of have a choice as to which illusion to
preserve. At least assuming that the VM can see a real real-time
clock, we can either preserve the illusion that cycles have the
advertised frequency, or we can preserve the illusion that
instructions take vaguely the right number of cycles to complete.
With TCG, we can't do both.
We have a somewhat similar tradeoff here: do we prioritize matching
the behaviour that the counter will be exactly 0x80000000 at the time
of an overflow interrupt, or do we prioritize matching the behaviour
that the counter is an accurate and exact count of completed
instructions. The fact that there is a tradeoff and which side we've
chosen is the key point to describe here, I think.
[In this instance, I think we can get closer to getting both sides
right. I believe there's a way in TCG to clamp the number of
instructions in a translated block - we could set that based on the
distance to overflow of the PMCs. That sounds very much not worth it
at this stage - we could look at it as a refinement later if we care]
> > > > > + }
> > > > > + }
> > > > > +
> > > > > + return overflow_triggered;
> > > > > +}
> > > > > +
> > > > > static void pmu_update_cycles(CPUPPCState *env)
> > > > > {
> > > > > uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> > > > > @@ -258,6 +282,20 @@ static void fire_PMC_interrupt(PowerPCCPU *cpu)
> > > > > return;
> > > > > }
> > > > > +/* This helper assumes that the PMC is running. */
> > > > > +void helper_insns_inc(CPUPPCState *env, uint32_t num_insns)
> > > > > +{
> > > > > + bool overflow_triggered;
> > > > > + PowerPCCPU *cpu;
> > > > > +
> > > > > + overflow_triggered = pmu_increment_insns(env, num_insns);
> > > > > +
> > > > > + if (overflow_triggered) {
> > > > > + cpu = env_archcpu(env);
> > > > > + fire_PMC_interrupt(cpu);
> > > > > + }
> > > > > +}
> > > > > +
> > > > > static void cpu_ppc_pmu_timer_cb(void *opaque)
> > > > > {
> > > > > PowerPCCPU *cpu = opaque;
> > > > > diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> > > > > index 9960df6e18..ccc83d0603 100644
> > > > > --- a/target/ppc/translate.c
> > > > > +++ b/target/ppc/translate.c
> > > > > @@ -177,6 +177,7 @@ struct DisasContext {
> > > > > bool hr;
> > > > > bool mmcr0_pmcc0;
> > > > > bool mmcr0_pmcc1;
> > > > > + bool pmu_frozen;
> > > > > ppc_spr_t *spr_cb; /* Needed to check rights for mfspr/mtspr */
> > > > > int singlestep_enabled;
> > > > > uint32_t flags;
> > > > > @@ -4170,6 +4171,31 @@ static inline void gen_update_cfar(DisasContext *ctx, target_ulong nip)
> > > > > #endif
> > > > > }
> > > > > +#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
> > > >
> > > > Should this actually be !CONFIG_USER_ONLY? IIUC there are
> > > > circumstances where userspace could access the PMU, including
> > > > instruction counting.
> > >
> > > The user mode will not be able to use the PMU properly because the MMCR1
> > > reg, used to define the events to be sampled, isn't writable by userpace
> > > under any circunstance.
> >
> > Couldn't they use PMC5 without writing MMCR1?
>
>
> Yeah, in theory. Problem state write access to PMCs are granted for MMCR0_PMCC
> 0b10 || 0b11. The PMCC bits of MMCR0 aren't user read/writable (only FC, PMAO and PMAE
> bits can be r/w from userspace, all other bits are filtered out). With the default
> PMCC value of 0b00 the PMCs are readable, but not writable. So in a way userspace can
> start the PMU and see instruction counting in PMC5 but it wouldn't be able to set it
> to a specific value and wouldn't be able to use overflows.
>
> All that said, the change to allow PMC5 to be incremented in problem state is simple
> enough so I ended up doing it.
Ok, I think that's a good idea.
You're probably right that at the moment there's no way for a pure
userspace program to access the counters like this. However, that's
only because of a pretty complex chain of restrictions. There's no
fundamental reason why we couldn't have some configuration mechanism
to execute a userland program with PMU permission.
--
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] 24+ messages in thread
* [PATCH v8 07/10] target/ppc/power8-pmu.c: add PM_RUN_INST_CMPL (0xFA) event
2021-11-25 15:08 [PATCH v8 00/10] PMU-EBB support for PPC64 TCG Daniel Henrique Barboza
` (5 preceding siblings ...)
2021-11-25 15:08 ` [PATCH v8 06/10] target/ppc: enable PMU instruction count Daniel Henrique Barboza
@ 2021-11-25 15:08 ` Daniel Henrique Barboza
2021-11-30 0:33 ` David Gibson
2021-11-25 15:08 ` [PATCH v8 08/10] PPC64/TCG: Implement 'rfebb' instruction Daniel Henrique Barboza
` (2 subsequent siblings)
9 siblings, 1 reply; 24+ messages in thread
From: Daniel Henrique Barboza @ 2021-11-25 15:08 UTC (permalink / raw)
To: qemu-devel
Cc: richard.henderson, Daniel Henrique Barboza, qemu-ppc, clg, david
PM_RUN_INST_CMPL, instructions completed with the run latch set, is
the architected PowerISA v3.1 event defined with PMC4SEL = 0xFA.
Implement it by checking for the CTRL RUN bit before incrementing the
counter. To make this work properly we also need to force a new
translation block each time SPR_CTRL is written. A small tweak in
pmu_increment_insns() is then needed to only increment this event
if the thread has the run latch.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
target/ppc/cpu.h | 4 ++++
target/ppc/cpu_init.c | 2 +-
target/ppc/power8-pmu.c | 24 ++++++++++++++++++++++--
target/ppc/spr_tcg.h | 1 +
target/ppc/translate.c | 12 ++++++++++++
5 files changed, 40 insertions(+), 3 deletions(-)
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 38cd2b5c43..993884164f 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -304,6 +304,7 @@ typedef enum {
PMU_EVENT_INACTIVE,
PMU_EVENT_CYCLES,
PMU_EVENT_INSTRUCTIONS,
+ PMU_EVENT_INSN_RUN_LATCH,
} PMUEventType;
/*****************************************************************************/
@@ -389,6 +390,9 @@ typedef enum {
#define MMCR1_PMC4SEL_START 56
#define MMCR1_PMC4EVT_EXTR (64 - MMCR1_PMC4SEL_START - MMCR1_EVT_SIZE)
+/* PMU uses CTRL_RUN to sample PM_RUN_INST_CMPL */
+#define CTRL_RUN PPC_BIT(63)
+
/* LPCR bits */
#define LPCR_VPM0 PPC_BIT(0)
#define LPCR_VPM1 PPC_BIT(1)
diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
index 2d72dde26d..ecce4c7c1e 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -6749,7 +6749,7 @@ static void register_book3s_ctrl_sprs(CPUPPCState *env)
{
spr_register(env, SPR_CTRL, "SPR_CTRL",
SPR_NOACCESS, SPR_NOACCESS,
- SPR_NOACCESS, &spr_write_generic,
+ SPR_NOACCESS, &spr_write_CTRL,
0x00000000);
spr_register(env, SPR_UCTRL, "SPR_UCTRL",
&spr_read_ureg, SPR_NOACCESS,
diff --git a/target/ppc/power8-pmu.c b/target/ppc/power8-pmu.c
index 59d0def79d..98797f0b2f 100644
--- a/target/ppc/power8-pmu.c
+++ b/target/ppc/power8-pmu.c
@@ -96,6 +96,15 @@ static PMUEventType pmc_get_event(CPUPPCState *env, int sprn)
evt_type = PMU_EVENT_CYCLES;
}
break;
+ case 0xFA:
+ /*
+ * PMC4SEL = 0xFA is the "instructions completed
+ * with run latch set" event.
+ */
+ if (sprn == SPR_POWER_PMC4) {
+ evt_type = PMU_EVENT_INSN_RUN_LATCH;
+ }
+ break;
case 0xFE:
/*
* PMC1SEL = 0xFE is the architected PowerISA v3.1
@@ -119,11 +128,22 @@ static bool pmu_increment_insns(CPUPPCState *env, uint32_t num_insns)
/* PMC6 never counts instructions */
for (sprn = SPR_POWER_PMC1; sprn <= SPR_POWER_PMC5; sprn++) {
- if (pmc_get_event(env, sprn) != PMU_EVENT_INSTRUCTIONS) {
+ PMUEventType evt_type = pmc_get_event(env, sprn);
+ bool insn_event = evt_type == PMU_EVENT_INSTRUCTIONS ||
+ evt_type == PMU_EVENT_INSN_RUN_LATCH;
+
+ if (pmc_is_inactive(env, sprn) || !insn_event) {
continue;
}
- env->spr[sprn] += num_insns;
+ if (evt_type == PMU_EVENT_INSTRUCTIONS) {
+ env->spr[sprn] += num_insns;
+ }
+
+ if (evt_type == PMU_EVENT_INSN_RUN_LATCH &&
+ env->spr[SPR_CTRL] & CTRL_RUN) {
+ env->spr[sprn] += num_insns;
+ }
if (env->spr[sprn] >= PMC_COUNTER_NEGATIVE_VAL &&
pmc_has_overflow_enabled(env, sprn)) {
diff --git a/target/ppc/spr_tcg.h b/target/ppc/spr_tcg.h
index 1d6521eedc..f98d97c0ba 100644
--- a/target/ppc/spr_tcg.h
+++ b/target/ppc/spr_tcg.h
@@ -28,6 +28,7 @@ void spr_write_generic(DisasContext *ctx, int sprn, int gprn);
void spr_write_MMCR0(DisasContext *ctx, int sprn, int gprn);
void spr_write_MMCR1(DisasContext *ctx, int sprn, int gprn);
void spr_write_PMC(DisasContext *ctx, int sprn, int gprn);
+void spr_write_CTRL(DisasContext *ctx, int sprn, int gprn);
void spr_read_xer(DisasContext *ctx, int gprn, int sprn);
void spr_write_xer(DisasContext *ctx, int sprn, int gprn);
void spr_read_lr(DisasContext *ctx, int gprn, int sprn);
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index ccc83d0603..d0e361a9d1 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -403,6 +403,18 @@ void spr_write_generic(DisasContext *ctx, int sprn, int gprn)
spr_store_dump_spr(sprn);
}
+void spr_write_CTRL(DisasContext *ctx, int sprn, int gprn)
+{
+ spr_write_generic(ctx, sprn, gprn);
+
+ /*
+ * SPR_CTRL writes must force a new translation block,
+ * allowing the PMU to calculate the run latch events with
+ * more accuracy.
+ */
+ ctx->base.is_jmp = DISAS_EXIT_UPDATE;
+}
+
#if !defined(CONFIG_USER_ONLY)
void spr_write_generic32(DisasContext *ctx, int sprn, int gprn)
{
--
2.31.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v8 07/10] target/ppc/power8-pmu.c: add PM_RUN_INST_CMPL (0xFA) event
2021-11-25 15:08 ` [PATCH v8 07/10] target/ppc/power8-pmu.c: add PM_RUN_INST_CMPL (0xFA) event Daniel Henrique Barboza
@ 2021-11-30 0:33 ` David Gibson
0 siblings, 0 replies; 24+ messages in thread
From: David Gibson @ 2021-11-30 0:33 UTC (permalink / raw)
To: Daniel Henrique Barboza; +Cc: richard.henderson, qemu-ppc, qemu-devel, clg
[-- Attachment #1: Type: text/plain, Size: 5987 bytes --]
On Thu, Nov 25, 2021 at 12:08:14PM -0300, Daniel Henrique Barboza wrote:
> PM_RUN_INST_CMPL, instructions completed with the run latch set, is
> the architected PowerISA v3.1 event defined with PMC4SEL = 0xFA.
>
> Implement it by checking for the CTRL RUN bit before incrementing the
> counter. To make this work properly we also need to force a new
> translation block each time SPR_CTRL is written. A small tweak in
> pmu_increment_insns() is then needed to only increment this event
> if the thread has the run latch.
>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Obviously, it would also be possible to treat the runlatch
instructions event like the all-instructions event but force an update
on runlatch changes. Having to incoporate CTRL into the active
counter logic as well as the other stuff seems like it might make
things messier that way overall though.
> ---
> target/ppc/cpu.h | 4 ++++
> target/ppc/cpu_init.c | 2 +-
> target/ppc/power8-pmu.c | 24 ++++++++++++++++++++++--
> target/ppc/spr_tcg.h | 1 +
> target/ppc/translate.c | 12 ++++++++++++
> 5 files changed, 40 insertions(+), 3 deletions(-)
>
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 38cd2b5c43..993884164f 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -304,6 +304,7 @@ typedef enum {
> PMU_EVENT_INACTIVE,
> PMU_EVENT_CYCLES,
> PMU_EVENT_INSTRUCTIONS,
> + PMU_EVENT_INSN_RUN_LATCH,
> } PMUEventType;
>
> /*****************************************************************************/
> @@ -389,6 +390,9 @@ typedef enum {
> #define MMCR1_PMC4SEL_START 56
> #define MMCR1_PMC4EVT_EXTR (64 - MMCR1_PMC4SEL_START - MMCR1_EVT_SIZE)
>
> +/* PMU uses CTRL_RUN to sample PM_RUN_INST_CMPL */
> +#define CTRL_RUN PPC_BIT(63)
> +
> /* LPCR bits */
> #define LPCR_VPM0 PPC_BIT(0)
> #define LPCR_VPM1 PPC_BIT(1)
> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> index 2d72dde26d..ecce4c7c1e 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -6749,7 +6749,7 @@ static void register_book3s_ctrl_sprs(CPUPPCState *env)
> {
> spr_register(env, SPR_CTRL, "SPR_CTRL",
> SPR_NOACCESS, SPR_NOACCESS,
> - SPR_NOACCESS, &spr_write_generic,
> + SPR_NOACCESS, &spr_write_CTRL,
> 0x00000000);
> spr_register(env, SPR_UCTRL, "SPR_UCTRL",
> &spr_read_ureg, SPR_NOACCESS,
> diff --git a/target/ppc/power8-pmu.c b/target/ppc/power8-pmu.c
> index 59d0def79d..98797f0b2f 100644
> --- a/target/ppc/power8-pmu.c
> +++ b/target/ppc/power8-pmu.c
> @@ -96,6 +96,15 @@ static PMUEventType pmc_get_event(CPUPPCState *env, int sprn)
> evt_type = PMU_EVENT_CYCLES;
> }
> break;
> + case 0xFA:
> + /*
> + * PMC4SEL = 0xFA is the "instructions completed
> + * with run latch set" event.
> + */
> + if (sprn == SPR_POWER_PMC4) {
> + evt_type = PMU_EVENT_INSN_RUN_LATCH;
> + }
> + break;
> case 0xFE:
> /*
> * PMC1SEL = 0xFE is the architected PowerISA v3.1
> @@ -119,11 +128,22 @@ static bool pmu_increment_insns(CPUPPCState *env, uint32_t num_insns)
>
> /* PMC6 never counts instructions */
> for (sprn = SPR_POWER_PMC1; sprn <= SPR_POWER_PMC5; sprn++) {
> - if (pmc_get_event(env, sprn) != PMU_EVENT_INSTRUCTIONS) {
> + PMUEventType evt_type = pmc_get_event(env, sprn);
> + bool insn_event = evt_type == PMU_EVENT_INSTRUCTIONS ||
> + evt_type == PMU_EVENT_INSN_RUN_LATCH;
> +
> + if (pmc_is_inactive(env, sprn) || !insn_event) {
> continue;
> }
>
> - env->spr[sprn] += num_insns;
> + if (evt_type == PMU_EVENT_INSTRUCTIONS) {
> + env->spr[sprn] += num_insns;
> + }
> +
> + if (evt_type == PMU_EVENT_INSN_RUN_LATCH &&
> + env->spr[SPR_CTRL] & CTRL_RUN) {
> + env->spr[sprn] += num_insns;
> + }
>
> if (env->spr[sprn] >= PMC_COUNTER_NEGATIVE_VAL &&
> pmc_has_overflow_enabled(env, sprn)) {
> diff --git a/target/ppc/spr_tcg.h b/target/ppc/spr_tcg.h
> index 1d6521eedc..f98d97c0ba 100644
> --- a/target/ppc/spr_tcg.h
> +++ b/target/ppc/spr_tcg.h
> @@ -28,6 +28,7 @@ void spr_write_generic(DisasContext *ctx, int sprn, int gprn);
> void spr_write_MMCR0(DisasContext *ctx, int sprn, int gprn);
> void spr_write_MMCR1(DisasContext *ctx, int sprn, int gprn);
> void spr_write_PMC(DisasContext *ctx, int sprn, int gprn);
> +void spr_write_CTRL(DisasContext *ctx, int sprn, int gprn);
> void spr_read_xer(DisasContext *ctx, int gprn, int sprn);
> void spr_write_xer(DisasContext *ctx, int sprn, int gprn);
> void spr_read_lr(DisasContext *ctx, int gprn, int sprn);
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index ccc83d0603..d0e361a9d1 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -403,6 +403,18 @@ void spr_write_generic(DisasContext *ctx, int sprn, int gprn)
> spr_store_dump_spr(sprn);
> }
>
> +void spr_write_CTRL(DisasContext *ctx, int sprn, int gprn)
> +{
> + spr_write_generic(ctx, sprn, gprn);
> +
> + /*
> + * SPR_CTRL writes must force a new translation block,
> + * allowing the PMU to calculate the run latch events with
> + * more accuracy.
> + */
> + ctx->base.is_jmp = DISAS_EXIT_UPDATE;
> +}
> +
> #if !defined(CONFIG_USER_ONLY)
> void spr_write_generic32(DisasContext *ctx, int sprn, int gprn)
> {
--
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] 24+ messages in thread
* [PATCH v8 08/10] PPC64/TCG: Implement 'rfebb' instruction
2021-11-25 15:08 [PATCH v8 00/10] PMU-EBB support for PPC64 TCG Daniel Henrique Barboza
` (6 preceding siblings ...)
2021-11-25 15:08 ` [PATCH v8 07/10] target/ppc/power8-pmu.c: add PM_RUN_INST_CMPL (0xFA) event Daniel Henrique Barboza
@ 2021-11-25 15:08 ` Daniel Henrique Barboza
2021-11-30 4:11 ` David Gibson
2021-11-25 15:08 ` [PATCH v8 09/10] target/ppc: PMU Event-Based exception support Daniel Henrique Barboza
2021-11-25 15:08 ` [PATCH v8 10/10] target/ppc/excp_helper.c: EBB handling adjustments Daniel Henrique Barboza
9 siblings, 1 reply; 24+ messages in thread
From: Daniel Henrique Barboza @ 2021-11-25 15:08 UTC (permalink / raw)
To: qemu-devel
Cc: Daniel Henrique Barboza, richard.henderson, qemu-ppc, clg,
Matheus Ferst, david
An Event-Based Branch (EBB) allows applications to change the NIA when a
event-based exception occurs. Event-based exceptions are enabled by
setting the Branch Event Status and Control Register (BESCR). If the
event-based exception is enabled when the exception occurs, an EBB
happens.
The following operations happens during an EBB:
- Global Enable (GE) bit of BESCR is set to 0;
- bits 0-61 of the Event-Based Branch Return Register (EBBRR) are set
to the the effective address of the NIA that would have executed if the EBB
didn't happen;
- Instruction fetch and execution will continue in the effective address
contained in the Event-Based Branch Handler Register (EBBHR).
The EBB Handler will process the event and then execute the Return From
Event-Based Branch (rfebb) instruction. rfebb sets BESCR_GE and then
redirects execution to the address pointed in EBBRR. This process is
described in the PowerISA v3.1, Book II, Chapter 6 [1].
This patch implements the rfebb instruction. Descriptions of all
relevant BESCR bits are also added - this patch is only using BESCR_GE,
but the next patches will use the remaining bits.
[1] https://wiki.raptorcs.com/w/images/f/f5/PowerISA_public.v3.1.pdf
Reviewed-by: Matheus Ferst <matheus.ferst@eldorado.org.br>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
target/ppc/cpu.h | 13 ++++++++++
target/ppc/excp_helper.c | 31 ++++++++++++++++++++++++
target/ppc/helper.h | 1 +
target/ppc/insn32.decode | 5 ++++
target/ppc/translate.c | 2 ++
target/ppc/translate/branch-impl.c.inc | 33 ++++++++++++++++++++++++++
6 files changed, 85 insertions(+)
create mode 100644 target/ppc/translate/branch-impl.c.inc
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 993884164f..edb4488176 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -393,6 +393,19 @@ typedef enum {
/* PMU uses CTRL_RUN to sample PM_RUN_INST_CMPL */
#define CTRL_RUN PPC_BIT(63)
+/* EBB/BESCR bits */
+/* Global Enable */
+#define BESCR_GE PPC_BIT(0)
+/* External Event-based Exception Enable */
+#define BESCR_EE PPC_BIT(30)
+/* Performance Monitor Event-based Exception Enable */
+#define BESCR_PME PPC_BIT(31)
+/* External Event-based Exception Occurred */
+#define BESCR_EEO PPC_BIT(62)
+/* Performance Monitor Event-based Exception Occurred */
+#define BESCR_PMEO PPC_BIT(63)
+#define BESCR_INVALID PPC_BITMASK(32, 33)
+
/* LPCR bits */
#define LPCR_VPM0 PPC_BIT(0)
#define LPCR_VPM1 PPC_BIT(1)
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 17607adbe4..7ead32279c 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -1250,6 +1250,37 @@ void helper_hrfid(CPUPPCState *env)
}
#endif
+#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
+void helper_rfebb(CPUPPCState *env, target_ulong s)
+{
+ target_ulong msr = env->msr;
+
+ /*
+ * Handling of BESCR bits 32:33 according to PowerISA v3.1:
+ *
+ * "If BESCR 32:33 != 0b00 the instruction is treated as if
+ * the instruction form were invalid."
+ */
+ if (env->spr[SPR_BESCR] & BESCR_INVALID) {
+ raise_exception_err(env, POWERPC_EXCP_PROGRAM,
+ POWERPC_EXCP_INVAL | POWERPC_EXCP_INVAL_INVAL);
+ }
+
+ env->nip = env->spr[SPR_EBBRR];
+
+ /* Switching to 32-bit ? Crop the nip */
+ if (!msr_is_64bit(env, msr)) {
+ env->nip = (uint32_t)env->spr[SPR_EBBRR];
+ }
+
+ if (s) {
+ env->spr[SPR_BESCR] |= BESCR_GE;
+ } else {
+ env->spr[SPR_BESCR] &= ~BESCR_GE;
+ }
+}
+#endif
+
/*****************************************************************************/
/* Embedded PowerPC specific helpers */
void helper_40x_rfci(CPUPPCState *env)
diff --git a/target/ppc/helper.h b/target/ppc/helper.h
index d8a23e054a..b0535b389b 100644
--- a/target/ppc/helper.h
+++ b/target/ppc/helper.h
@@ -18,6 +18,7 @@ DEF_HELPER_2(pminsn, void, env, i32)
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_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/insn32.decode b/target/ppc/insn32.decode
index e135b8aba4..6cad783dde 100644
--- a/target/ppc/insn32.decode
+++ b/target/ppc/insn32.decode
@@ -427,3 +427,8 @@ XXSPLTW 111100 ..... ---.. ..... 010100100 . . @XX2
## VSX Vector Load Special Value Instruction
LXVKQ 111100 ..... 11111 ..... 0101101000 . @X_uim5
+
+### rfebb
+&XL_s s:uint8_t
+@XL_s ......-------------- s:1 .......... - &XL_s
+RFEBB 010011-------------- . 0010010010 - @XL_s
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index d0e361a9d1..d643a83a51 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -7467,6 +7467,8 @@ static bool resolve_PLS_D(DisasContext *ctx, arg_D *d, arg_PLS_D *a)
#include "translate/spe-impl.c.inc"
+#include "translate/branch-impl.c.inc"
+
/* Handles lfdp, lxsd, lxssp */
static void gen_dform39(DisasContext *ctx)
{
diff --git a/target/ppc/translate/branch-impl.c.inc b/target/ppc/translate/branch-impl.c.inc
new file mode 100644
index 0000000000..29cfa11854
--- /dev/null
+++ b/target/ppc/translate/branch-impl.c.inc
@@ -0,0 +1,33 @@
+/*
+ * Power ISA decode for branch instructions
+ *
+ * Copyright IBM Corp. 2021
+ *
+ * Authors:
+ * Daniel Henrique Barboza <danielhb413@gmail.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
+
+static bool trans_RFEBB(DisasContext *ctx, arg_XL_s *arg)
+{
+ REQUIRE_INSNS_FLAGS2(ctx, ISA207S);
+
+ gen_icount_io_start(ctx);
+ gen_update_cfar(ctx, ctx->cia);
+ gen_helper_rfebb(cpu_env, cpu_gpr[arg->s]);
+
+ ctx->base.is_jmp = DISAS_CHAIN;
+
+ return true;
+}
+#else
+static bool trans_RFEBB(DisasContext *ctx, arg_XL_s *arg)
+{
+ gen_invalid(ctx);
+ return true;
+}
+#endif
--
2.31.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v8 08/10] PPC64/TCG: Implement 'rfebb' instruction
2021-11-25 15:08 ` [PATCH v8 08/10] PPC64/TCG: Implement 'rfebb' instruction Daniel Henrique Barboza
@ 2021-11-30 4:11 ` David Gibson
0 siblings, 0 replies; 24+ messages in thread
From: David Gibson @ 2021-11-30 4:11 UTC (permalink / raw)
To: Daniel Henrique Barboza
Cc: richard.henderson, Matheus Ferst, qemu-ppc, qemu-devel, clg
[-- Attachment #1: Type: text/plain, Size: 7232 bytes --]
On Thu, Nov 25, 2021 at 12:08:15PM -0300, Daniel Henrique Barboza wrote:
> An Event-Based Branch (EBB) allows applications to change the NIA when a
> event-based exception occurs. Event-based exceptions are enabled by
> setting the Branch Event Status and Control Register (BESCR). If the
> event-based exception is enabled when the exception occurs, an EBB
> happens.
>
> The following operations happens during an EBB:
>
> - Global Enable (GE) bit of BESCR is set to 0;
> - bits 0-61 of the Event-Based Branch Return Register (EBBRR) are set
> to the the effective address of the NIA that would have executed if the EBB
> didn't happen;
> - Instruction fetch and execution will continue in the effective address
> contained in the Event-Based Branch Handler Register (EBBHR).
>
> The EBB Handler will process the event and then execute the Return From
> Event-Based Branch (rfebb) instruction. rfebb sets BESCR_GE and then
> redirects execution to the address pointed in EBBRR. This process is
> described in the PowerISA v3.1, Book II, Chapter 6 [1].
>
> This patch implements the rfebb instruction. Descriptions of all
> relevant BESCR bits are also added - this patch is only using BESCR_GE,
> but the next patches will use the remaining bits.
>
> [1] https://wiki.raptorcs.com/w/images/f/f5/PowerISA_public.v3.1.pdf
>
> Reviewed-by: Matheus Ferst <matheus.ferst@eldorado.org.br>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
I'm guessing that for some applications rfebb could be a fairly hot
path, so you might want to rework this to avoid the helper. But that
can certainly be a later improvement.
> ---
> target/ppc/cpu.h | 13 ++++++++++
> target/ppc/excp_helper.c | 31 ++++++++++++++++++++++++
> target/ppc/helper.h | 1 +
> target/ppc/insn32.decode | 5 ++++
> target/ppc/translate.c | 2 ++
> target/ppc/translate/branch-impl.c.inc | 33 ++++++++++++++++++++++++++
> 6 files changed, 85 insertions(+)
> create mode 100644 target/ppc/translate/branch-impl.c.inc
>
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 993884164f..edb4488176 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -393,6 +393,19 @@ typedef enum {
> /* PMU uses CTRL_RUN to sample PM_RUN_INST_CMPL */
> #define CTRL_RUN PPC_BIT(63)
>
> +/* EBB/BESCR bits */
> +/* Global Enable */
> +#define BESCR_GE PPC_BIT(0)
> +/* External Event-based Exception Enable */
> +#define BESCR_EE PPC_BIT(30)
> +/* Performance Monitor Event-based Exception Enable */
> +#define BESCR_PME PPC_BIT(31)
> +/* External Event-based Exception Occurred */
> +#define BESCR_EEO PPC_BIT(62)
> +/* Performance Monitor Event-based Exception Occurred */
> +#define BESCR_PMEO PPC_BIT(63)
> +#define BESCR_INVALID PPC_BITMASK(32, 33)
> +
> /* LPCR bits */
> #define LPCR_VPM0 PPC_BIT(0)
> #define LPCR_VPM1 PPC_BIT(1)
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 17607adbe4..7ead32279c 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -1250,6 +1250,37 @@ void helper_hrfid(CPUPPCState *env)
> }
> #endif
>
> +#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
> +void helper_rfebb(CPUPPCState *env, target_ulong s)
> +{
> + target_ulong msr = env->msr;
> +
> + /*
> + * Handling of BESCR bits 32:33 according to PowerISA v3.1:
> + *
> + * "If BESCR 32:33 != 0b00 the instruction is treated as if
> + * the instruction form were invalid."
> + */
> + if (env->spr[SPR_BESCR] & BESCR_INVALID) {
> + raise_exception_err(env, POWERPC_EXCP_PROGRAM,
> + POWERPC_EXCP_INVAL | POWERPC_EXCP_INVAL_INVAL);
> + }
> +
> + env->nip = env->spr[SPR_EBBRR];
> +
> + /* Switching to 32-bit ? Crop the nip */
> + if (!msr_is_64bit(env, msr)) {
> + env->nip = (uint32_t)env->spr[SPR_EBBRR];
> + }
> +
> + if (s) {
> + env->spr[SPR_BESCR] |= BESCR_GE;
> + } else {
> + env->spr[SPR_BESCR] &= ~BESCR_GE;
> + }
> +}
> +#endif
> +
> /*****************************************************************************/
> /* Embedded PowerPC specific helpers */
> void helper_40x_rfci(CPUPPCState *env)
> diff --git a/target/ppc/helper.h b/target/ppc/helper.h
> index d8a23e054a..b0535b389b 100644
> --- a/target/ppc/helper.h
> +++ b/target/ppc/helper.h
> @@ -18,6 +18,7 @@ DEF_HELPER_2(pminsn, void, env, i32)
> 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_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/insn32.decode b/target/ppc/insn32.decode
> index e135b8aba4..6cad783dde 100644
> --- a/target/ppc/insn32.decode
> +++ b/target/ppc/insn32.decode
> @@ -427,3 +427,8 @@ XXSPLTW 111100 ..... ---.. ..... 010100100 . . @XX2
> ## VSX Vector Load Special Value Instruction
>
> LXVKQ 111100 ..... 11111 ..... 0101101000 . @X_uim5
> +
> +### rfebb
> +&XL_s s:uint8_t
> +@XL_s ......-------------- s:1 .......... - &XL_s
> +RFEBB 010011-------------- . 0010010010 - @XL_s
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index d0e361a9d1..d643a83a51 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -7467,6 +7467,8 @@ static bool resolve_PLS_D(DisasContext *ctx, arg_D *d, arg_PLS_D *a)
>
> #include "translate/spe-impl.c.inc"
>
> +#include "translate/branch-impl.c.inc"
> +
> /* Handles lfdp, lxsd, lxssp */
> static void gen_dform39(DisasContext *ctx)
> {
> diff --git a/target/ppc/translate/branch-impl.c.inc b/target/ppc/translate/branch-impl.c.inc
> new file mode 100644
> index 0000000000..29cfa11854
> --- /dev/null
> +++ b/target/ppc/translate/branch-impl.c.inc
> @@ -0,0 +1,33 @@
> +/*
> + * Power ISA decode for branch instructions
> + *
> + * Copyright IBM Corp. 2021
> + *
> + * Authors:
> + * Daniel Henrique Barboza <danielhb413@gmail.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
> +
> +static bool trans_RFEBB(DisasContext *ctx, arg_XL_s *arg)
> +{
> + REQUIRE_INSNS_FLAGS2(ctx, ISA207S);
> +
> + gen_icount_io_start(ctx);
> + gen_update_cfar(ctx, ctx->cia);
> + gen_helper_rfebb(cpu_env, cpu_gpr[arg->s]);
> +
> + ctx->base.is_jmp = DISAS_CHAIN;
> +
> + return true;
> +}
> +#else
> +static bool trans_RFEBB(DisasContext *ctx, arg_XL_s *arg)
> +{
> + gen_invalid(ctx);
> + return true;
> +}
> +#endif
--
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] 24+ messages in thread
* [PATCH v8 09/10] target/ppc: PMU Event-Based exception support
2021-11-25 15:08 [PATCH v8 00/10] PMU-EBB support for PPC64 TCG Daniel Henrique Barboza
` (7 preceding siblings ...)
2021-11-25 15:08 ` [PATCH v8 08/10] PPC64/TCG: Implement 'rfebb' instruction Daniel Henrique Barboza
@ 2021-11-25 15:08 ` Daniel Henrique Barboza
2021-11-30 4:15 ` David Gibson
2021-11-25 15:08 ` [PATCH v8 10/10] target/ppc/excp_helper.c: EBB handling adjustments Daniel Henrique Barboza
9 siblings, 1 reply; 24+ messages in thread
From: Daniel Henrique Barboza @ 2021-11-25 15:08 UTC (permalink / raw)
To: qemu-devel
Cc: Gustavo Romero, Gustavo Romero, Daniel Henrique Barboza,
richard.henderson, qemu-ppc, clg, david
From: Gustavo Romero <gromero@linux.ibm.com>
Following up the rfebb implementation, this patch adds the EBB exception
support that are triggered by Performance Monitor alerts. This exception
occurs when an enabled PMU condition or event happens and both MMCR0_EBE
and BESCR_PME are set.
The supported PM alerts will consist of counter negative conditions of
the PMU counters. This will be achieved by a timer mechanism that will
predict when a counter becomes negative. The PMU timer callback will set
the appropriate bits in MMCR0 and fire a PMC interrupt. The EBB
exception code will then set the appropriate BESCR bits, set the next
instruction pointer to the address pointed by the return register
(SPR_EBBRR), and redirect execution to the handler (pointed by
SPR_EBBHR).
CC: Gustavo Romero <gustavo.romero@linaro.org>
Signed-off-by: Gustavo Romero <gromero@linux.ibm.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
target/ppc/cpu.h | 5 ++++-
target/ppc/excp_helper.c | 29 +++++++++++++++++++++++++++++
target/ppc/power8-pmu.c | 35 +++++++++++++++++++++++++++++++++--
3 files changed, 66 insertions(+), 3 deletions(-)
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index edb4488176..28ae904d76 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -129,8 +129,10 @@ enum {
/* ISA 3.00 additions */
POWERPC_EXCP_HVIRT = 101,
POWERPC_EXCP_SYSCALL_VECTORED = 102, /* scv exception */
+ POWERPC_EXCP_EBB = 103, /* Event-based branch exception */
+
/* EOL */
- POWERPC_EXCP_NB = 103,
+ POWERPC_EXCP_NB = 104,
/* QEMU exceptions: special cases we want to stop translation */
POWERPC_EXCP_SYSCALL_USER = 0x203, /* System call in user mode only */
};
@@ -2453,6 +2455,7 @@ enum {
PPC_INTERRUPT_HMI, /* Hypervisor Maintenance interrupt */
PPC_INTERRUPT_HDOORBELL, /* Hypervisor Doorbell interrupt */
PPC_INTERRUPT_HVIRT, /* Hypervisor virtualization interrupt */
+ PPC_INTERRUPT_PMC, /* Hypervisor virtualization interrupt */
};
/* Processor Compatibility mask (PCR) */
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 7ead32279c..a26d266fe6 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -799,6 +799,23 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
cpu_abort(cs, "Non maskable external exception "
"is not implemented yet !\n");
break;
+ case POWERPC_EXCP_EBB: /* Event-based branch exception */
+ if ((env->spr[SPR_FSCR] & (1ull << FSCR_EBB)) &&
+ (env->spr[SPR_BESCR] & BESCR_GE) &&
+ (env->spr[SPR_BESCR] & BESCR_PME)) {
+ target_ulong nip;
+
+ env->spr[SPR_BESCR] &= ~BESCR_GE; /* Clear GE */
+ env->spr[SPR_BESCR] |= BESCR_PMEO; /* Set PMEO */
+ env->spr[SPR_EBBRR] = env->nip; /* Save NIP for rfebb insn */
+ nip = env->spr[SPR_EBBHR]; /* EBB handler */
+ powerpc_set_excp_state(cpu, nip, env->msr);
+ }
+ /*
+ * This interrupt is handled by userspace. No need
+ * to proceed.
+ */
+ return;
default:
excp_invalid:
cpu_abort(cs, "Invalid PowerPC exception %d. Aborting\n", excp);
@@ -1046,6 +1063,18 @@ static void ppc_hw_interrupt(CPUPPCState *env)
powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_THERM);
return;
}
+ /* PMC -> Event-based branch exception */
+ if (env->pending_interrupts & (1 << PPC_INTERRUPT_PMC)) {
+ /*
+ * Performance Monitor event-based exception can only
+ * occur in problem state.
+ */
+ if (msr_pr == 1) {
+ env->pending_interrupts &= ~(1 << PPC_INTERRUPT_PMC);
+ powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_EBB);
+ return;
+ }
+ }
}
if (env->resume_as_sreset) {
diff --git a/target/ppc/power8-pmu.c b/target/ppc/power8-pmu.c
index 98797f0b2f..330e0d2ae8 100644
--- a/target/ppc/power8-pmu.c
+++ b/target/ppc/power8-pmu.c
@@ -290,6 +290,15 @@ void helper_store_pmc(CPUPPCState *env, uint32_t sprn, uint64_t value)
pmc_update_overflow_timer(env, sprn);
}
+static void pmu_delete_timers(CPUPPCState *env)
+{
+ int i;
+
+ for (i = 0; i < PMU_TIMERS_NUM; i++) {
+ timer_del(env->pmu_cyc_overflow_timers[i]);
+ }
+}
+
static void fire_PMC_interrupt(PowerPCCPU *cpu)
{
CPUPPCState *env = &cpu->env;
@@ -298,8 +307,30 @@ static void fire_PMC_interrupt(PowerPCCPU *cpu)
return;
}
- /* PMC interrupt not implemented yet */
- 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 demands a new hflags compute */
+ hreg_compute_hflags(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;
+ }
+
+ /* Fire the PMC hardware exception */
+ ppc_set_irq(cpu, PPC_INTERRUPT_PMC, 1);
}
/* This helper assumes that the PMC is running. */
--
2.31.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v8 09/10] target/ppc: PMU Event-Based exception support
2021-11-25 15:08 ` [PATCH v8 09/10] target/ppc: PMU Event-Based exception support Daniel Henrique Barboza
@ 2021-11-30 4:15 ` David Gibson
0 siblings, 0 replies; 24+ messages in thread
From: David Gibson @ 2021-11-30 4:15 UTC (permalink / raw)
To: Daniel Henrique Barboza
Cc: Gustavo Romero, Gustavo Romero, richard.henderson, qemu-devel,
qemu-ppc, clg
[-- Attachment #1: Type: text/plain, Size: 6470 bytes --]
On Thu, Nov 25, 2021 at 12:08:16PM -0300, Daniel Henrique Barboza wrote:
> From: Gustavo Romero <gromero@linux.ibm.com>
>
> Following up the rfebb implementation, this patch adds the EBB exception
> support that are triggered by Performance Monitor alerts. This exception
> occurs when an enabled PMU condition or event happens and both MMCR0_EBE
> and BESCR_PME are set.
>
> The supported PM alerts will consist of counter negative conditions of
> the PMU counters. This will be achieved by a timer mechanism that will
> predict when a counter becomes negative. The PMU timer callback will set
> the appropriate bits in MMCR0 and fire a PMC interrupt. The EBB
> exception code will then set the appropriate BESCR bits, set the next
> instruction pointer to the address pointed by the return register
> (SPR_EBBRR), and redirect execution to the handler (pointed by
> SPR_EBBHR).
>
> CC: Gustavo Romero <gustavo.romero@linaro.org>
> Signed-off-by: Gustavo Romero <gromero@linux.ibm.com>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
> target/ppc/cpu.h | 5 ++++-
> target/ppc/excp_helper.c | 29 +++++++++++++++++++++++++++++
> target/ppc/power8-pmu.c | 35 +++++++++++++++++++++++++++++++++--
> 3 files changed, 66 insertions(+), 3 deletions(-)
>
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index edb4488176..28ae904d76 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -129,8 +129,10 @@ enum {
> /* ISA 3.00 additions */
> POWERPC_EXCP_HVIRT = 101,
> POWERPC_EXCP_SYSCALL_VECTORED = 102, /* scv exception */
> + POWERPC_EXCP_EBB = 103, /* Event-based branch exception */
> +
> /* EOL */
> - POWERPC_EXCP_NB = 103,
> + POWERPC_EXCP_NB = 104,
> /* QEMU exceptions: special cases we want to stop translation */
> POWERPC_EXCP_SYSCALL_USER = 0x203, /* System call in user mode only */
> };
> @@ -2453,6 +2455,7 @@ enum {
> PPC_INTERRUPT_HMI, /* Hypervisor Maintenance interrupt */
> PPC_INTERRUPT_HDOORBELL, /* Hypervisor Doorbell interrupt */
> PPC_INTERRUPT_HVIRT, /* Hypervisor virtualization interrupt */
> + PPC_INTERRUPT_PMC, /* Hypervisor virtualization interrupt */
I'm guessing the comment here should be updated.
> };
>
> /* Processor Compatibility mask (PCR) */
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 7ead32279c..a26d266fe6 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -799,6 +799,23 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
> cpu_abort(cs, "Non maskable external exception "
> "is not implemented yet !\n");
> break;
> + case POWERPC_EXCP_EBB: /* Event-based branch exception */
> + if ((env->spr[SPR_FSCR] & (1ull << FSCR_EBB)) &&
> + (env->spr[SPR_BESCR] & BESCR_GE) &&
> + (env->spr[SPR_BESCR] & BESCR_PME)) {
> + target_ulong nip;
> +
> + env->spr[SPR_BESCR] &= ~BESCR_GE; /* Clear GE */
> + env->spr[SPR_BESCR] |= BESCR_PMEO; /* Set PMEO */
> + env->spr[SPR_EBBRR] = env->nip; /* Save NIP for rfebb insn */
> + nip = env->spr[SPR_EBBHR]; /* EBB handler */
> + powerpc_set_excp_state(cpu, nip, env->msr);
> + }
> + /*
> + * This interrupt is handled by userspace. No need
> + * to proceed.
> + */
> + return;
> default:
> excp_invalid:
> cpu_abort(cs, "Invalid PowerPC exception %d. Aborting\n", excp);
> @@ -1046,6 +1063,18 @@ static void ppc_hw_interrupt(CPUPPCState *env)
> powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_THERM);
> return;
> }
> + /* PMC -> Event-based branch exception */
> + if (env->pending_interrupts & (1 << PPC_INTERRUPT_PMC)) {
> + /*
> + * Performance Monitor event-based exception can only
> + * occur in problem state.
> + */
> + if (msr_pr == 1) {
> + env->pending_interrupts &= ~(1 << PPC_INTERRUPT_PMC);
> + powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_EBB);
> + return;
> + }
> + }
> }
>
> if (env->resume_as_sreset) {
> diff --git a/target/ppc/power8-pmu.c b/target/ppc/power8-pmu.c
> index 98797f0b2f..330e0d2ae8 100644
> --- a/target/ppc/power8-pmu.c
> +++ b/target/ppc/power8-pmu.c
> @@ -290,6 +290,15 @@ void helper_store_pmc(CPUPPCState *env, uint32_t sprn, uint64_t value)
> pmc_update_overflow_timer(env, sprn);
> }
>
> +static void pmu_delete_timers(CPUPPCState *env)
> +{
> + int i;
> +
> + for (i = 0; i < PMU_TIMERS_NUM; i++) {
> + timer_del(env->pmu_cyc_overflow_timers[i]);
> + }
> +}
> +
> static void fire_PMC_interrupt(PowerPCCPU *cpu)
> {
> CPUPPCState *env = &cpu->env;
> @@ -298,8 +307,30 @@ static void fire_PMC_interrupt(PowerPCCPU *cpu)
> return;
> }
>
> - /* PMC interrupt not implemented yet */
> - 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 demands a new hflags compute */
> + hreg_compute_hflags(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;
> + }
> +
> + /* Fire the PMC hardware exception */
> + ppc_set_irq(cpu, PPC_INTERRUPT_PMC, 1);
> }
>
> /* This helper assumes that the PMC is running. */
--
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] 24+ messages in thread
* [PATCH v8 10/10] target/ppc/excp_helper.c: EBB handling adjustments
2021-11-25 15:08 [PATCH v8 00/10] PMU-EBB support for PPC64 TCG Daniel Henrique Barboza
` (8 preceding siblings ...)
2021-11-25 15:08 ` [PATCH v8 09/10] target/ppc: PMU Event-Based exception support Daniel Henrique Barboza
@ 2021-11-25 15:08 ` Daniel Henrique Barboza
2021-11-30 4:17 ` David Gibson
9 siblings, 1 reply; 24+ messages in thread
From: Daniel Henrique Barboza @ 2021-11-25 15:08 UTC (permalink / raw)
To: qemu-devel
Cc: richard.henderson, Daniel Henrique Barboza, qemu-ppc, clg, david
The current logic is only considering event-based exceptions triggered
by the performance monitor. This is true now, but we might want to add
support for external event-based exceptions in the future.
Let's make it a bit easier to do so by adding the bit logic that would
happen in case we were dealing with an external event-based exception.
While we're at it, add a few comments explaining why we're setting and
clearing BESCR bits.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
target/ppc/excp_helper.c | 45 ++++++++++++++++++++++++++++++++++------
1 file changed, 39 insertions(+), 6 deletions(-)
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index a26d266fe6..42e2fee9c8 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -801,14 +801,47 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
break;
case POWERPC_EXCP_EBB: /* Event-based branch exception */
if ((env->spr[SPR_FSCR] & (1ull << FSCR_EBB)) &&
- (env->spr[SPR_BESCR] & BESCR_GE) &&
- (env->spr[SPR_BESCR] & BESCR_PME)) {
+ (env->spr[SPR_BESCR] & BESCR_GE)) {
target_ulong nip;
- env->spr[SPR_BESCR] &= ~BESCR_GE; /* Clear GE */
- env->spr[SPR_BESCR] |= BESCR_PMEO; /* Set PMEO */
- env->spr[SPR_EBBRR] = env->nip; /* Save NIP for rfebb insn */
- nip = env->spr[SPR_EBBHR]; /* EBB handler */
+ /*
+ * If we have Performance Monitor Event-Based exception
+ * enabled (BESCR_PME) and a Performance Monitor alert
+ * occurred (MMCR0_PMAO), clear BESCR_PME and set BESCR_PMEO
+ * (Performance Monitor Event-Based Exception Occurred).
+ *
+ * Software is responsible for clearing both BESCR_PMEO and
+ * MMCR0_PMAO after the event has been handled.
+ */
+ if ((env->spr[SPR_BESCR] & BESCR_PME) &&
+ (env->spr[SPR_POWER_MMCR0] & MMCR0_PMAO)) {
+ env->spr[SPR_BESCR] &= ~BESCR_PME;
+ env->spr[SPR_BESCR] |= BESCR_PMEO;
+ }
+
+ /*
+ * In the case of External Event-Based exceptions, do a
+ * similar logic with BESCR_EE and BESCR_EEO. BESCR_EEO must
+ * also be cleared by software.
+ *
+ * PowerISA 3.1 considers that we'll not have BESCR_PMEO and
+ * BESCR_EEO set at the same time. We can check for BESCR_PMEO
+ * being not set in step above to see if this exception was
+ * trigged by an external event.
+ */
+ if (env->spr[SPR_BESCR] & BESCR_EE &&
+ !(env->spr[SPR_BESCR] & BESCR_PMEO)) {
+ env->spr[SPR_BESCR] &= ~BESCR_EE;
+ env->spr[SPR_BESCR] |= BESCR_EEO;
+ }
+
+ /*
+ * Clear BESCR_GE, save NIP for 'rfebb' and point the
+ * execution to the event handler (SPR_EBBHR) address.
+ */
+ env->spr[SPR_BESCR] &= ~BESCR_GE;
+ env->spr[SPR_EBBRR] = env->nip;
+ nip = env->spr[SPR_EBBHR];
powerpc_set_excp_state(cpu, nip, env->msr);
}
/*
--
2.31.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v8 10/10] target/ppc/excp_helper.c: EBB handling adjustments
2021-11-25 15:08 ` [PATCH v8 10/10] target/ppc/excp_helper.c: EBB handling adjustments Daniel Henrique Barboza
@ 2021-11-30 4:17 ` David Gibson
0 siblings, 0 replies; 24+ messages in thread
From: David Gibson @ 2021-11-30 4:17 UTC (permalink / raw)
To: Daniel Henrique Barboza; +Cc: richard.henderson, qemu-ppc, qemu-devel, clg
[-- Attachment #1: Type: text/plain, Size: 863 bytes --]
On Thu, Nov 25, 2021 at 12:08:17PM -0300, Daniel Henrique Barboza wrote:
> The current logic is only considering event-based exceptions triggered
> by the performance monitor. This is true now, but we might want to add
> support for external event-based exceptions in the future.
>
> Let's make it a bit easier to do so by adding the bit logic that would
> happen in case we were dealing with an external event-based exception.
>
> While we're at it, add a few comments explaining why we're setting and
> clearing BESCR bits.
>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
--
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] 24+ messages in thread