[PATCH v2 0/5] Reorg ppc64 pmu insn counting

[PATCH v2 0/5] Reorg ppc64 pmu insn counting

[Daniel Henrique Barboza]

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

[PATCH v2 1/5] target/ppc: Cache per-pmc insn and cycle count settings

[Daniel Henrique Barboza]

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

Re: [PATCH v2 1/5] target/ppc: Cache per-pmc insn and cycle count settings

[Richard Henderson]

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~

[PATCH v2 2/5] target/ppc: Rewrite pmu_increment_insns

[Daniel Henrique Barboza]

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

[PATCH v2 3/5] target/ppc: Use env->pnc_cyc_cnt

[Daniel Henrique Barboza]

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

[PATCH v2 4/5] target/ppc: keep ins_cnt/cyc_cnt cleared if MMCR0_FC is set

[Daniel Henrique Barboza]

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

Re: [PATCH v2 4/5] target/ppc: keep ins_cnt/cyc_cnt cleared if MMCR0_FC is set

[Richard Henderson]

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~

Re: [PATCH v2 4/5] target/ppc: keep ins_cnt/cyc_cnt cleared if MMCR0_FC is set

[Daniel Henrique Barboza]

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~

[PATCH v2 5/5] target/ppc: do not call hreg_compute_hflags() in helper_store_mmcr0()

[Daniel Henrique Barboza]

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

Re: [PATCH v2 5/5] target/ppc: do not call hreg_compute_hflags() in helper_store_mmcr0()

[Richard Henderson]

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~