* [PATCH v2 0/5] Reorg ppc64 pmu insn counting
@ 2022-01-03 18:53 Daniel Henrique Barboza
2022-01-03 18:53 ` [PATCH v2 1/5] target/ppc: Cache per-pmc insn and cycle count settings Daniel Henrique Barboza
` (4 more replies)
0 siblings, 5 replies; 10+ messages in thread
From: Daniel Henrique Barboza @ 2022-01-03 18:53 UTC (permalink / raw)
To: qemu-devel
Cc: richard.henderson, Daniel Henrique Barboza, qemu-ppc, clg, david
Hi,
This is the v2 of Richard's work sent in [1]. The initial implementation
presented some issues with the event-based branch kernel tests that I
fixed in this new version. This code is now passing all EBB PPC64
tests, it makes Avocado happy and it's all contained in the C helper.
Last patch is an improvement that became natural to do after seeing
how Richard updates env->hflags in pmu_update_summaries().
Avocado test performance:
(1/1) tests/avocado/boot_linux_console.py:BootLinuxConsole.test_ppc_powernv8: PASS (32.63 s)
(1/1) tests/avocado/boot_linux_console.py:BootLinuxConsole.test_ppc_powernv9: PASS (34.16 s)
Changes from v1:
- patch 1:
* fixed a couple of minor that was causing test failures
- patch 2 and 3: unchanged
- patch 4 (new):
* clear HFLAGS_INSN_CNT if MMCR0_FC is set
- patch 5 (new):
* avoid calling hreg_compute_hflags()
- v1 link: https://lists.gnu.org/archive/html/qemu-devel/2021-12/msg04013.html
[1] https://lists.gnu.org/archive/html/qemu-devel/2021-12/msg04013.html
Daniel Henrique Barboza (2):
target/ppc: keep ins_cnt/cyc_cnt cleared if MMCR0_FC is set
target/ppc: do not call hreg_compute_hflags() in helper_store_mmcr0()
Richard Henderson (3):
target/ppc: Cache per-pmc insn and cycle count settings
target/ppc: Rewrite pmu_increment_insns
target/ppc: Use env->pnc_cyc_cnt
target/ppc/cpu.h | 3 +
target/ppc/cpu_init.c | 1 +
target/ppc/helper_regs.c | 2 +-
target/ppc/machine.c | 2 +
target/ppc/power8-pmu.c | 238 +++++++++++++++++----------------------
target/ppc/power8-pmu.h | 14 +--
6 files changed, 117 insertions(+), 143 deletions(-)
--
2.33.1
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 1/5] target/ppc: Cache per-pmc insn and cycle count settings
2022-01-03 18:53 [PATCH v2 0/5] Reorg ppc64 pmu insn counting Daniel Henrique Barboza
@ 2022-01-03 18:53 ` Daniel Henrique Barboza
2022-01-03 21:26 ` Richard Henderson
2022-01-03 18:53 ` [PATCH v2 2/5] target/ppc: Rewrite pmu_increment_insns Daniel Henrique Barboza
` (3 subsequent siblings)
4 siblings, 1 reply; 10+ messages in thread
From: Daniel Henrique Barboza @ 2022-01-03 18:53 UTC (permalink / raw)
To: qemu-devel
Cc: richard.henderson, Daniel Henrique Barboza, qemu-ppc, clg, david
From: Richard Henderson <richard.henderson@linaro.org>
This is the combination of frozen bit and counter type, on a per
counter basis. So far this is only used by HFLAGS_INSN_CNT, but
will be used more later.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
[danielhb: fixed PMC4 cyc_cnt shift and insn run latch code]
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
target/ppc/cpu.h | 3 +++
target/ppc/cpu_init.c | 1 +
target/ppc/helper_regs.c | 2 +-
target/ppc/machine.c | 2 ++
target/ppc/power8-pmu.c | 53 +++++++++++++++++++++++++++++++---------
target/ppc/power8-pmu.h | 14 +++++------
6 files changed, 54 insertions(+), 21 deletions(-)
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index fc66c3561d..a297a52168 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1144,6 +1144,9 @@ struct CPUPPCState {
/* Other registers */
target_ulong spr[1024]; /* special purpose registers */
ppc_spr_t spr_cb[1024];
+ /* Composite status for PMC[1-5] enabled and counting insns or cycles. */
+ uint8_t pmc_ins_cnt;
+ uint8_t pmc_cyc_cnt;
/* Vector status and control register, minus VSCR_SAT */
uint32_t vscr;
/* VSX registers (including FP and AVR) */
diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
index 06ef15cd9e..63f9babfee 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -8313,6 +8313,7 @@ static void ppc_cpu_reset(DeviceState *dev)
#endif /* CONFIG_TCG */
#endif
+ pmu_update_summaries(env);
hreg_compute_hflags(env);
env->reserve_addr = (target_ulong)-1ULL;
/* Be sure no exception or interrupt is pending */
diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
index b847928842..8671b7bb69 100644
--- a/target/ppc/helper_regs.c
+++ b/target/ppc/helper_regs.c
@@ -123,7 +123,7 @@ static uint32_t hreg_compute_hflags_value(CPUPPCState *env)
}
#if defined(TARGET_PPC64)
- if (pmu_insn_cnt_enabled(env)) {
+ if (env->pmc_ins_cnt) {
hflags |= 1 << HFLAGS_INSN_CNT;
}
#endif
diff --git a/target/ppc/machine.c b/target/ppc/machine.c
index 93972df58e..756d8de5d8 100644
--- a/target/ppc/machine.c
+++ b/target/ppc/machine.c
@@ -8,6 +8,7 @@
#include "qapi/error.h"
#include "qemu/main-loop.h"
#include "kvm_ppc.h"
+#include "power8-pmu.h"
static void post_load_update_msr(CPUPPCState *env)
{
@@ -19,6 +20,7 @@ static void post_load_update_msr(CPUPPCState *env)
*/
env->msr ^= env->msr_mask & ~((1ULL << MSR_TGPR) | MSR_HVB);
ppc_store_msr(env, msr);
+ pmu_update_summaries(env);
}
static int cpu_load_old(QEMUFile *f, void *opaque, int version_id)
diff --git a/target/ppc/power8-pmu.c b/target/ppc/power8-pmu.c
index 08d1902cd5..4fce6e8de8 100644
--- a/target/ppc/power8-pmu.c
+++ b/target/ppc/power8-pmu.c
@@ -11,8 +11,6 @@
*/
#include "qemu/osdep.h"
-
-#include "power8-pmu.h"
#include "cpu.h"
#include "helper_regs.h"
#include "exec/exec-all.h"
@@ -20,6 +18,7 @@
#include "qemu/error-report.h"
#include "qemu/main-loop.h"
#include "hw/ppc/ppc.h"
+#include "power8-pmu.h"
#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
@@ -121,18 +120,47 @@ static PMUEventType pmc_get_event(CPUPPCState *env, int sprn)
return evt_type;
}
-bool pmu_insn_cnt_enabled(CPUPPCState *env)
+void pmu_update_summaries(CPUPPCState *env)
{
- int sprn;
-
- for (sprn = SPR_POWER_PMC1; sprn <= SPR_POWER_PMC5; sprn++) {
- if (pmc_get_event(env, sprn) == PMU_EVENT_INSTRUCTIONS ||
- pmc_get_event(env, sprn) == PMU_EVENT_INSN_RUN_LATCH) {
- return true;
+ target_ulong mmcr0 = env->spr[SPR_POWER_MMCR0];
+ target_ulong mmcr1 = env->spr[SPR_POWER_MMCR1];
+ int ins_cnt = 0;
+ int cyc_cnt = 0;
+
+ if (!(mmcr0 & MMCR0_FC14) && mmcr1 != 0) {
+ target_ulong sel;
+
+ sel = extract64(mmcr1, MMCR1_PMC1EVT_EXTR, MMCR1_EVT_SIZE);
+ switch (sel) {
+ case 0x02:
+ case 0xfe:
+ ins_cnt |= 1 << 1;
+ break;
+ case 0x1e:
+ case 0xf0:
+ cyc_cnt |= 1 << 1;
+ break;
}
+
+ sel = extract64(mmcr1, MMCR1_PMC2EVT_EXTR, MMCR1_EVT_SIZE);
+ ins_cnt |= (sel == 0x02) << 2;
+ cyc_cnt |= (sel == 0x1e) << 2;
+
+ sel = extract64(mmcr1, MMCR1_PMC3EVT_EXTR, MMCR1_EVT_SIZE);
+ ins_cnt |= (sel == 0x02) << 3;
+ cyc_cnt |= (sel == 0x1e) << 3;
+
+ sel = extract64(mmcr1, MMCR1_PMC4EVT_EXTR, MMCR1_EVT_SIZE);
+ ins_cnt |= ((sel == 0xfa) || (sel == 0x2)) << 4;
+ cyc_cnt |= (sel == 0x1e) << 4;
}
- return false;
+ ins_cnt |= !(mmcr0 & MMCR0_FC56) << 5;
+ cyc_cnt |= !(mmcr0 & MMCR0_FC56) << 6;
+
+ env->pmc_ins_cnt = ins_cnt;
+ env->pmc_cyc_cnt = cyc_cnt;
+ env->hflags = deposit32(env->hflags, HFLAGS_INSN_CNT, 1, ins_cnt != 0);
}
static bool pmu_increment_insns(CPUPPCState *env, uint32_t num_insns)
@@ -264,8 +292,9 @@ void helper_store_mmcr0(CPUPPCState *env, target_ulong value)
env->spr[SPR_POWER_MMCR0] = value;
- /* MMCR0 writes can change HFLAGS_PMCCCLEAR and HFLAGS_INSN_CNT */
+ /* MMCR0 writes can change HFLAGS_PMCC[01] and HFLAGS_INSN_CNT */
hreg_compute_hflags(env);
+ pmu_update_summaries(env);
/* Update cycle overflow timers with the current MMCR0 state */
pmu_update_overflow_timers(env);
@@ -278,7 +307,7 @@ void helper_store_mmcr1(CPUPPCState *env, uint64_t value)
env->spr[SPR_POWER_MMCR1] = value;
/* MMCR1 writes can change HFLAGS_INSN_CNT */
- hreg_compute_hflags(env);
+ pmu_update_summaries(env);
}
target_ulong helper_read_pmc(CPUPPCState *env, uint32_t sprn)
diff --git a/target/ppc/power8-pmu.h b/target/ppc/power8-pmu.h
index 3ee4b4cda5..a839199561 100644
--- a/target/ppc/power8-pmu.h
+++ b/target/ppc/power8-pmu.h
@@ -13,14 +13,12 @@
#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);
-bool pmu_insn_cnt_enabled(CPUPPCState *env);
+
+#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
+void pmu_update_summaries(CPUPPCState *env);
+#else
+static inline void pmu_update_summaries(CPUPPCState *env) { }
+#endif
#endif
--
2.33.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v2 2/5] target/ppc: Rewrite pmu_increment_insns
2022-01-03 18:53 [PATCH v2 0/5] Reorg ppc64 pmu insn counting Daniel Henrique Barboza
2022-01-03 18:53 ` [PATCH v2 1/5] target/ppc: Cache per-pmc insn and cycle count settings Daniel Henrique Barboza
@ 2022-01-03 18:53 ` Daniel Henrique Barboza
2022-01-03 18:53 ` [PATCH v2 3/5] target/ppc: Use env->pnc_cyc_cnt Daniel Henrique Barboza
` (2 subsequent siblings)
4 siblings, 0 replies; 10+ messages in thread
From: Daniel Henrique Barboza @ 2022-01-03 18:53 UTC (permalink / raw)
To: qemu-devel; +Cc: richard.henderson, qemu-ppc, clg, david
From: Richard Henderson <richard.henderson@linaro.org>
Use the cached pmc_ins_cnt value. Unroll the loop over the
different PMC counters. Treat the PMC4 run-latch specially.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
target/ppc/power8-pmu.c | 78 ++++++++++++++++++++++++++---------------
1 file changed, 49 insertions(+), 29 deletions(-)
diff --git a/target/ppc/power8-pmu.c b/target/ppc/power8-pmu.c
index 4fce6e8de8..8f01934c15 100644
--- a/target/ppc/power8-pmu.c
+++ b/target/ppc/power8-pmu.c
@@ -165,45 +165,65 @@ void pmu_update_summaries(CPUPPCState *env)
static bool pmu_increment_insns(CPUPPCState *env, uint32_t num_insns)
{
+ target_ulong mmcr0 = env->spr[SPR_POWER_MMCR0];
+ unsigned ins_cnt = env->pmc_ins_cnt;
bool overflow_triggered = false;
- int sprn;
-
- /* PMC6 never counts instructions */
- for (sprn = SPR_POWER_PMC1; sprn <= SPR_POWER_PMC5; sprn++) {
- 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;
+ target_ulong tmp;
+
+ if (unlikely(ins_cnt & 0x1e)) {
+ if (ins_cnt & (1 << 1)) {
+ tmp = env->spr[SPR_POWER_PMC1];
+ tmp += num_insns;
+ if (tmp >= PMC_COUNTER_NEGATIVE_VAL && (mmcr0 & MMCR0_PMC1CE)) {
+ tmp = PMC_COUNTER_NEGATIVE_VAL;
+ overflow_triggered = true;
+ }
+ env->spr[SPR_POWER_PMC1] = tmp;
}
- if (evt_type == PMU_EVENT_INSTRUCTIONS) {
- env->spr[sprn] += num_insns;
+ if (ins_cnt & (1 << 2)) {
+ tmp = env->spr[SPR_POWER_PMC2];
+ tmp += num_insns;
+ if (tmp >= PMC_COUNTER_NEGATIVE_VAL && (mmcr0 & MMCR0_PMCjCE)) {
+ tmp = PMC_COUNTER_NEGATIVE_VAL;
+ overflow_triggered = true;
+ }
+ env->spr[SPR_POWER_PMC2] = tmp;
}
- if (evt_type == PMU_EVENT_INSN_RUN_LATCH &&
- env->spr[SPR_CTRL] & CTRL_RUN) {
- env->spr[sprn] += num_insns;
+ if (ins_cnt & (1 << 3)) {
+ tmp = env->spr[SPR_POWER_PMC3];
+ tmp += num_insns;
+ if (tmp >= PMC_COUNTER_NEGATIVE_VAL && (mmcr0 & MMCR0_PMCjCE)) {
+ tmp = PMC_COUNTER_NEGATIVE_VAL;
+ overflow_triggered = true;
+ }
+ env->spr[SPR_POWER_PMC3] = tmp;
}
- if (env->spr[sprn] >= PMC_COUNTER_NEGATIVE_VAL &&
- pmc_has_overflow_enabled(env, sprn)) {
+ if (ins_cnt & (1 << 4)) {
+ target_ulong mmcr1 = env->spr[SPR_POWER_MMCR1];
+ int sel = extract64(mmcr1, MMCR1_PMC4EVT_EXTR, MMCR1_EVT_SIZE);
+ if (sel == 0x02 || (env->spr[SPR_CTRL] & CTRL_RUN)) {
+ tmp = env->spr[SPR_POWER_PMC4];
+ tmp += num_insns;
+ if (tmp >= PMC_COUNTER_NEGATIVE_VAL && (mmcr0 & MMCR0_PMCjCE)) {
+ tmp = PMC_COUNTER_NEGATIVE_VAL;
+ overflow_triggered = true;
+ }
+ env->spr[SPR_POWER_PMC4] = tmp;
+ }
+ }
+ }
+ if (ins_cnt & (1 << 5)) {
+ tmp = env->spr[SPR_POWER_PMC5];
+ tmp += num_insns;
+ if (tmp >= PMC_COUNTER_NEGATIVE_VAL && (mmcr0 & MMCR0_PMCjCE)) {
+ tmp = PMC_COUNTER_NEGATIVE_VAL;
overflow_triggered = true;
-
- /*
- * The real PMU will always trigger a counter overflow with
- * PMC_COUNTER_NEGATIVE_VAL. We don't have an easy way to
- * do that since we're counting block of instructions at
- * the end of each translation block, and we're probably
- * passing this value at this point.
- *
- * Let's write PMC_COUNTER_NEGATIVE_VAL to the overflowed
- * counter to simulate what the real hardware would do.
- */
- env->spr[sprn] = PMC_COUNTER_NEGATIVE_VAL;
}
+ env->spr[SPR_POWER_PMC5] = tmp;
}
return overflow_triggered;
--
2.33.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v2 3/5] target/ppc: Use env->pnc_cyc_cnt
2022-01-03 18:53 [PATCH v2 0/5] Reorg ppc64 pmu insn counting Daniel Henrique Barboza
2022-01-03 18:53 ` [PATCH v2 1/5] target/ppc: Cache per-pmc insn and cycle count settings Daniel Henrique Barboza
2022-01-03 18:53 ` [PATCH v2 2/5] target/ppc: Rewrite pmu_increment_insns Daniel Henrique Barboza
@ 2022-01-03 18:53 ` Daniel Henrique Barboza
2022-01-03 18:53 ` [PATCH v2 4/5] target/ppc: keep ins_cnt/cyc_cnt cleared if MMCR0_FC is set Daniel Henrique Barboza
2022-01-03 18:53 ` [PATCH v2 5/5] target/ppc: do not call hreg_compute_hflags() in helper_store_mmcr0() Daniel Henrique Barboza
4 siblings, 0 replies; 10+ messages in thread
From: Daniel Henrique Barboza @ 2022-01-03 18:53 UTC (permalink / raw)
To: qemu-devel; +Cc: richard.henderson, qemu-ppc, clg, david
From: Richard Henderson <richard.henderson@linaro.org>
Use the cached pmc_cyc_cnt value in pmu_update_cycles
and pmc_update_overflow_timer. This leaves pmc_get_event
and pmc_is_inactive unused, so remove them.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
target/ppc/power8-pmu.c | 107 ++++------------------------------------
1 file changed, 9 insertions(+), 98 deletions(-)
diff --git a/target/ppc/power8-pmu.c b/target/ppc/power8-pmu.c
index 8f01934c15..7fc7d91109 100644
--- a/target/ppc/power8-pmu.c
+++ b/target/ppc/power8-pmu.c
@@ -24,19 +24,6 @@
#define PMC_COUNTER_NEGATIVE_VAL 0x80000000UL
-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;
-}
-
static bool pmc_has_overflow_enabled(CPUPPCState *env, int sprn)
{
if (sprn == SPR_POWER_PMC1) {
@@ -46,80 +33,6 @@ static bool pmc_has_overflow_enabled(CPUPPCState *env, int sprn)
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
- * 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 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
- * event to sample instructions using PMC1.
- */
- if (sprn == SPR_POWER_PMC1) {
- evt_type = PMU_EVENT_INSTRUCTIONS;
- }
- break;
- default:
- break;
- }
-
- return evt_type;
-}
-
void pmu_update_summaries(CPUPPCState *env)
{
target_ulong mmcr0 = env->spr[SPR_POWER_MMCR0];
@@ -233,18 +146,16 @@ 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;
+ int sprn, cyc_cnt = env->pmc_cyc_cnt;
for (sprn = SPR_POWER_PMC1; sprn <= SPR_POWER_PMC6; sprn++) {
- if (pmc_get_event(env, sprn) != PMU_EVENT_CYCLES) {
- continue;
+ if (cyc_cnt & (1 << (sprn - SPR_POWER_PMC1 + 1))) {
+ /*
+ * The pseries and powernv clock runs at 1Ghz, meaning
+ * that 1 nanosec equals 1 cycle.
+ */
+ env->spr[sprn] += time_delta;
}
-
- /*
- * 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 */
@@ -273,7 +184,7 @@ static void pmc_update_overflow_timer(CPUPPCState *env, int sprn)
return;
}
- if (pmc_get_event(env, sprn) != PMU_EVENT_CYCLES ||
+ if (!(env->pmc_cyc_cnt & (1 << (sprn - SPR_POWER_PMC1 + 1))) ||
!pmc_has_overflow_enabled(env, sprn)) {
/* Overflow timer is not needed for this counter */
timer_del(pmc_overflow_timer);
@@ -281,7 +192,7 @@ static void pmc_update_overflow_timer(CPUPPCState *env, int sprn)
}
if (env->spr[sprn] >= PMC_COUNTER_NEGATIVE_VAL) {
- timeout = 0;
+ timeout = 0;
} else {
timeout = PMC_COUNTER_NEGATIVE_VAL - env->spr[sprn];
}
--
2.33.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v2 4/5] target/ppc: keep ins_cnt/cyc_cnt cleared if MMCR0_FC is set
2022-01-03 18:53 [PATCH v2 0/5] Reorg ppc64 pmu insn counting Daniel Henrique Barboza
` (2 preceding siblings ...)
2022-01-03 18:53 ` [PATCH v2 3/5] target/ppc: Use env->pnc_cyc_cnt Daniel Henrique Barboza
@ 2022-01-03 18:53 ` Daniel Henrique Barboza
2022-01-03 21:38 ` Richard Henderson
2022-01-03 18:53 ` [PATCH v2 5/5] target/ppc: do not call hreg_compute_hflags() in helper_store_mmcr0() Daniel Henrique Barboza
4 siblings, 1 reply; 10+ messages in thread
From: Daniel Henrique Barboza @ 2022-01-03 18:53 UTC (permalink / raw)
To: qemu-devel
Cc: richard.henderson, Daniel Henrique Barboza, qemu-ppc, clg, david
pmu_update_summaries() is not considering the case where the PMU can be
turned off (i.e. stop counting all events) if MMCR0_FC is set,
regardless of the other frozen counter bits state. This use case was
covered in the late pmc_get_event(), via the also gone pmc_is_inactive(),
that would return an invalid event if MMCR0_FC was set.
This use case is exercised by the back_to_back_ebbs_test Linux kernel
selftests [1]. As it is today, after enabling EBB exceptions, the test
will report an additional event-based branch being taken and will fail.
Other tests, such as cycles_test.c, will report additional cycles being
calculated in the counters because we're not freezing the PMU quick
enough.
Fix pmu_update_summaries() by keeping env->ins_cnt and env->cyc_cnt
cleared when MMCR0_FC is set.
[1] tools/testing/selftests/powerpc/pmu/ebb/back_to_back_ebbs_test.c
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
target/ppc/power8-pmu.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/target/ppc/power8-pmu.c b/target/ppc/power8-pmu.c
index 7fc7d91109..73713ca2a3 100644
--- a/target/ppc/power8-pmu.c
+++ b/target/ppc/power8-pmu.c
@@ -40,6 +40,10 @@ void pmu_update_summaries(CPUPPCState *env)
int ins_cnt = 0;
int cyc_cnt = 0;
+ if (mmcr0 & MMCR0_FC) {
+ goto hflags_calc;
+ }
+
if (!(mmcr0 & MMCR0_FC14) && mmcr1 != 0) {
target_ulong sel;
@@ -71,6 +75,7 @@ void pmu_update_summaries(CPUPPCState *env)
ins_cnt |= !(mmcr0 & MMCR0_FC56) << 5;
cyc_cnt |= !(mmcr0 & MMCR0_FC56) << 6;
+ hflags_calc:
env->pmc_ins_cnt = ins_cnt;
env->pmc_cyc_cnt = cyc_cnt;
env->hflags = deposit32(env->hflags, HFLAGS_INSN_CNT, 1, ins_cnt != 0);
--
2.33.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v2 5/5] target/ppc: do not call hreg_compute_hflags() in helper_store_mmcr0()
2022-01-03 18:53 [PATCH v2 0/5] Reorg ppc64 pmu insn counting Daniel Henrique Barboza
` (3 preceding siblings ...)
2022-01-03 18:53 ` [PATCH v2 4/5] target/ppc: keep ins_cnt/cyc_cnt cleared if MMCR0_FC is set Daniel Henrique Barboza
@ 2022-01-03 18:53 ` Daniel Henrique Barboza
2022-01-03 21:40 ` Richard Henderson
4 siblings, 1 reply; 10+ messages in thread
From: Daniel Henrique Barboza @ 2022-01-03 18:53 UTC (permalink / raw)
To: qemu-devel
Cc: richard.henderson, Daniel Henrique Barboza, qemu-ppc, clg, david
MMCR0 writes will change only MMCR0 bits which are used to calculate
HFLAGS_PMCC0, HFLAGS_PMCC1 and HFLAGS_INSN_CNT hflags. No other machine
register will be changed during this operation. This means that
hreg_compute_hflags() is overkill for what we need to do.
pmu_update_summaries() is already updating HFLAGS_INSN_CNT without
calling hreg_compure_hflags(). Let's do the same for the other 2 MMCR0
hflags.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
target/ppc/power8-pmu.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/target/ppc/power8-pmu.c b/target/ppc/power8-pmu.c
index 73713ca2a3..69342413bd 100644
--- a/target/ppc/power8-pmu.c
+++ b/target/ppc/power8-pmu.c
@@ -224,12 +224,17 @@ static void pmu_update_overflow_timers(CPUPPCState *env)
void helper_store_mmcr0(CPUPPCState *env, target_ulong value)
{
+ uint32_t hflags_pmcc0 = (value & MMCR0_PMCC0) != 0;
+ uint32_t hflags_pmcc1 = (value & MMCR0_PMCC1) != 0;
+
pmu_update_cycles(env);
env->spr[SPR_POWER_MMCR0] = value;
/* MMCR0 writes can change HFLAGS_PMCC[01] and HFLAGS_INSN_CNT */
- hreg_compute_hflags(env);
+ env->hflags = deposit32(env->hflags, HFLAGS_PMCC0, 1, hflags_pmcc0);
+ env->hflags = deposit32(env->hflags, HFLAGS_PMCC1, 1, hflags_pmcc1);
+
pmu_update_summaries(env);
/* Update cycle overflow timers with the current MMCR0 state */
--
2.33.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/5] target/ppc: Cache per-pmc insn and cycle count settings
2022-01-03 18:53 ` [PATCH v2 1/5] target/ppc: Cache per-pmc insn and cycle count settings Daniel Henrique Barboza
@ 2022-01-03 21:26 ` Richard Henderson
0 siblings, 0 replies; 10+ messages in thread
From: Richard Henderson @ 2022-01-03 21:26 UTC (permalink / raw)
To: Daniel Henrique Barboza, qemu-devel; +Cc: qemu-ppc, clg, david
On 1/3/22 10:53 AM, Daniel Henrique Barboza wrote:
> + /* Composite status for PMC[1-5] enabled and counting insns or cycles. */
> + uint8_t pmc_ins_cnt;
> + uint8_t pmc_cyc_cnt;
I should have updated the comment to 1-6 when I added cyc_cnt.
> + sel = extract64(mmcr1, MMCR1_PMC4EVT_EXTR, MMCR1_EVT_SIZE);
> + ins_cnt |= ((sel == 0xfa) || (sel == 0x2)) << 4;
Ah, thanks for fixing my typo.
r~
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 4/5] target/ppc: keep ins_cnt/cyc_cnt cleared if MMCR0_FC is set
2022-01-03 18:53 ` [PATCH v2 4/5] target/ppc: keep ins_cnt/cyc_cnt cleared if MMCR0_FC is set Daniel Henrique Barboza
@ 2022-01-03 21:38 ` Richard Henderson
2022-01-03 21:50 ` Daniel Henrique Barboza
0 siblings, 1 reply; 10+ messages in thread
From: Richard Henderson @ 2022-01-03 21:38 UTC (permalink / raw)
To: Daniel Henrique Barboza, qemu-devel; +Cc: qemu-ppc, clg, david
On 1/3/22 10:53 AM, Daniel Henrique Barboza wrote:
> pmu_update_summaries() is not considering the case where the PMU can be
> turned off (i.e. stop counting all events) if MMCR0_FC is set,
> regardless of the other frozen counter bits state. This use case was
> covered in the late pmc_get_event(), via the also gone pmc_is_inactive(),
> that would return an invalid event if MMCR0_FC was set.
>
> This use case is exercised by the back_to_back_ebbs_test Linux kernel
> selftests [1]. As it is today, after enabling EBB exceptions, the test
> will report an additional event-based branch being taken and will fail.
> Other tests, such as cycles_test.c, will report additional cycles being
> calculated in the counters because we're not freezing the PMU quick
> enough.
>
> Fix pmu_update_summaries() by keeping env->ins_cnt and env->cyc_cnt
> cleared when MMCR0_FC is set.
>
> [1] tools/testing/selftests/powerpc/pmu/ebb/back_to_back_ebbs_test.c
>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
> target/ppc/power8-pmu.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/target/ppc/power8-pmu.c b/target/ppc/power8-pmu.c
> index 7fc7d91109..73713ca2a3 100644
> --- a/target/ppc/power8-pmu.c
> +++ b/target/ppc/power8-pmu.c
> @@ -40,6 +40,10 @@ void pmu_update_summaries(CPUPPCState *env)
> int ins_cnt = 0;
> int cyc_cnt = 0;
>
> + if (mmcr0 & MMCR0_FC) {
> + goto hflags_calc;
> + }
> +
> if (!(mmcr0 & MMCR0_FC14) && mmcr1 != 0) {
> target_ulong sel;
>
> @@ -71,6 +75,7 @@ void pmu_update_summaries(CPUPPCState *env)
> ins_cnt |= !(mmcr0 & MMCR0_FC56) << 5;
> cyc_cnt |= !(mmcr0 & MMCR0_FC56) << 6;
>
> + hflags_calc:
Good catch, but should be folded into patch 1 to avoid bisection breakage.
r~
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 5/5] target/ppc: do not call hreg_compute_hflags() in helper_store_mmcr0()
2022-01-03 18:53 ` [PATCH v2 5/5] target/ppc: do not call hreg_compute_hflags() in helper_store_mmcr0() Daniel Henrique Barboza
@ 2022-01-03 21:40 ` Richard Henderson
0 siblings, 0 replies; 10+ messages in thread
From: Richard Henderson @ 2022-01-03 21:40 UTC (permalink / raw)
To: Daniel Henrique Barboza, qemu-devel; +Cc: qemu-ppc, clg, david
On 1/3/22 10:53 AM, Daniel Henrique Barboza wrote:
> MMCR0 writes will change only MMCR0 bits which are used to calculate
> HFLAGS_PMCC0, HFLAGS_PMCC1 and HFLAGS_INSN_CNT hflags. No other machine
> register will be changed during this operation. This means that
> hreg_compute_hflags() is overkill for what we need to do.
>
> pmu_update_summaries() is already updating HFLAGS_INSN_CNT without
> calling hreg_compure_hflags(). Let's do the same for the other 2 MMCR0
> hflags.
>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
> target/ppc/power8-pmu.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/target/ppc/power8-pmu.c b/target/ppc/power8-pmu.c
> index 73713ca2a3..69342413bd 100644
> --- a/target/ppc/power8-pmu.c
> +++ b/target/ppc/power8-pmu.c
> @@ -224,12 +224,17 @@ static void pmu_update_overflow_timers(CPUPPCState *env)
>
> void helper_store_mmcr0(CPUPPCState *env, target_ulong value)
> {
> + uint32_t hflags_pmcc0 = (value & MMCR0_PMCC0) != 0;
> + uint32_t hflags_pmcc1 = (value & MMCR0_PMCC1) != 0;
Could use bool here. Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 4/5] target/ppc: keep ins_cnt/cyc_cnt cleared if MMCR0_FC is set
2022-01-03 21:38 ` Richard Henderson
@ 2022-01-03 21:50 ` Daniel Henrique Barboza
0 siblings, 0 replies; 10+ messages in thread
From: Daniel Henrique Barboza @ 2022-01-03 21:50 UTC (permalink / raw)
To: Richard Henderson, qemu-devel; +Cc: qemu-ppc, clg, david
On 1/3/22 18:38, Richard Henderson wrote:
> On 1/3/22 10:53 AM, Daniel Henrique Barboza wrote:
>> pmu_update_summaries() is not considering the case where the PMU can be
>> turned off (i.e. stop counting all events) if MMCR0_FC is set,
>> regardless of the other frozen counter bits state. This use case was
>> covered in the late pmc_get_event(), via the also gone pmc_is_inactive(),
>> that would return an invalid event if MMCR0_FC was set.
>>
>> This use case is exercised by the back_to_back_ebbs_test Linux kernel
>> selftests [1]. As it is today, after enabling EBB exceptions, the test
>> will report an additional event-based branch being taken and will fail.
>> Other tests, such as cycles_test.c, will report additional cycles being
>> calculated in the counters because we're not freezing the PMU quick
>> enough.
>>
>> Fix pmu_update_summaries() by keeping env->ins_cnt and env->cyc_cnt
>> cleared when MMCR0_FC is set.
>>
>> [1] tools/testing/selftests/powerpc/pmu/ebb/back_to_back_ebbs_test.c
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
>> target/ppc/power8-pmu.c | 5 +++++
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/target/ppc/power8-pmu.c b/target/ppc/power8-pmu.c
>> index 7fc7d91109..73713ca2a3 100644
>> --- a/target/ppc/power8-pmu.c
>> +++ b/target/ppc/power8-pmu.c
>> @@ -40,6 +40,10 @@ void pmu_update_summaries(CPUPPCState *env)
>> int ins_cnt = 0;
>> int cyc_cnt = 0;
>> + if (mmcr0 & MMCR0_FC) {
>> + goto hflags_calc;
>> + }
>> +
>> if (!(mmcr0 & MMCR0_FC14) && mmcr1 != 0) {
>> target_ulong sel;
>> @@ -71,6 +75,7 @@ void pmu_update_summaries(CPUPPCState *env)
>> ins_cnt |= !(mmcr0 & MMCR0_FC56) << 5;
>> cyc_cnt |= !(mmcr0 & MMCR0_FC56) << 6;
>> + hflags_calc:
>
> Good catch, but should be folded into patch 1 to avoid bisection breakage.
Fair point. Given that you have a suggestion in patch 5 as well I'll send a v3.
Thanks,
Daniel
>
>
> r~
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-01-03 21:51 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-03 18:53 [PATCH v2 0/5] Reorg ppc64 pmu insn counting Daniel Henrique Barboza
2022-01-03 18:53 ` [PATCH v2 1/5] target/ppc: Cache per-pmc insn and cycle count settings Daniel Henrique Barboza
2022-01-03 21:26 ` Richard Henderson
2022-01-03 18:53 ` [PATCH v2 2/5] target/ppc: Rewrite pmu_increment_insns Daniel Henrique Barboza
2022-01-03 18:53 ` [PATCH v2 3/5] target/ppc: Use env->pnc_cyc_cnt Daniel Henrique Barboza
2022-01-03 18:53 ` [PATCH v2 4/5] target/ppc: keep ins_cnt/cyc_cnt cleared if MMCR0_FC is set Daniel Henrique Barboza
2022-01-03 21:38 ` Richard Henderson
2022-01-03 21:50 ` Daniel Henrique Barboza
2022-01-03 18:53 ` [PATCH v2 5/5] target/ppc: do not call hreg_compute_hflags() in helper_store_mmcr0() Daniel Henrique Barboza
2022-01-03 21:40 ` Richard Henderson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.