[PATCH v2 0/5] Re-write PPC64 PMU instruction count using TCG Ops

[PATCH v2 0/5] Re-write PPC64 PMU instruction count using TCG Ops

[Daniel Henrique Barboza]

Hi,

In this version the tedious repetition was taken away from the
code by using a helper that increments the PMCs based on specified
conditions.

As far as Avocado test goes, the performance is the same as the previous
version. All PMU-EBB kernel selftests are also passing. Basically we have
the same benefits from v1 but 150+ lines shorter.

changes from v1:
- former patches 2-6: removed
- new patch 2:
  * added inc_spr_if_cond() helper
- new patch 3:
  * add insn count for PMCs 1-4
- patch 4 (former 7):
  * use a loop to reduce code repetition when checking for counter
overflows 
- v1 link: https://lists.gnu.org/archive/html/qemu-devel/2021-12/msg03871.html


Daniel Henrique Barboza (5):
  target/ppc: introduce power8-pmu-insn-cnt.c.inc
  target/ppc/power8-pmu-insn-cnt: introduce inc_spr_if_cond()
  target/ppc/power8-pmu-insn-cnt: add PMCs1-4 insn count
  target/ppc/power8-pmu-insn-cnt: add pmu_check_overflow()
  target/ppc/power8-pmu.c: remove helper_insns_inc()

 target/ppc/helper.h                  |   2 +-
 target/ppc/power8-pmu-insn-cnt.c.inc | 213 +++++++++++++++++++++++++++
 target/ppc/power8-pmu.c              |  60 +-------
 target/ppc/translate.c               |  44 +-----
 4 files changed, 220 insertions(+), 99 deletions(-)
 create mode 100644 target/ppc/power8-pmu-insn-cnt.c.inc

-- 
2.33.1

[PATCH v2 1/5] target/ppc: introduce power8-pmu-insn-cnt.c.inc

[Daniel Henrique Barboza]

We're going to add a significant amount of TCG ops code for
instruction counting, eventually getting rid of the 'helper_insn_inc'
helper entirely.

Create a new file to avoid putting even more stuff on the already
crowded target/ppc/translate.c.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 target/ppc/power8-pmu-insn-cnt.c.inc | 54 ++++++++++++++++++++++++++++
 target/ppc/translate.c               | 44 ++---------------------
 2 files changed, 56 insertions(+), 42 deletions(-)
 create mode 100644 target/ppc/power8-pmu-insn-cnt.c.inc

diff --git a/target/ppc/power8-pmu-insn-cnt.c.inc b/target/ppc/power8-pmu-insn-cnt.c.inc
new file mode 100644
index 0000000000..6cdf2d2d88
--- /dev/null
+++ b/target/ppc/power8-pmu-insn-cnt.c.inc
@@ -0,0 +1,54 @@
+/*
+ * IBM Power8+ PMU TCG instruction count functions
+ *
+ * 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)
+static void pmu_count_insns(DisasContext *ctx)
+{
+    /*
+     * Do not bother calling the helper if the PMU isn't counting
+     * instructions.
+     */
+    if (!ctx->pmu_insn_cnt) {
+        return;
+    }
+
+ #if !defined(CONFIG_USER_ONLY)
+    /*
+     * 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
+    /*
+     * User mode can read (but not write) PMC5 and start/stop
+     * the PMU via MMCR0_FC. In this case just increment
+     * PMC5 with base.num_insns.
+     */
+    TCGv t0 = tcg_temp_new();
+
+    gen_load_spr(t0, SPR_POWER_PMC5);
+    tcg_gen_addi_tl(t0, t0, ctx->base.num_insns);
+    gen_store_spr(SPR_POWER_PMC5, t0);
+
+    tcg_temp_free(t0);
+#endif /* #if !defined(CONFIG_USER_ONLY) */
+}
+#else
+static void pmu_count_insns(DisasContext *ctx)
+{
+    return;
+}
+#endif /* #if defined(TARGET_PPC64) */
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 114456148c..44773bc6cd 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -4183,48 +4183,8 @@ static inline void gen_update_cfar(DisasContext *ctx, target_ulong nip)
 #endif
 }
 
-#if defined(TARGET_PPC64)
-static void pmu_count_insns(DisasContext *ctx)
-{
-    /*
-     * Do not bother calling the helper if the PMU isn't counting
-     * instructions.
-     */
-    if (!ctx->pmu_insn_cnt) {
-        return;
-    }
-
- #if !defined(CONFIG_USER_ONLY)
-    /*
-     * 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
-    /*
-     * User mode can read (but not write) PMC5 and start/stop
-     * the PMU via MMCR0_FC. In this case just increment
-     * PMC5 with base.num_insns.
-     */
-    TCGv t0 = tcg_temp_new();
-
-    gen_load_spr(t0, SPR_POWER_PMC5);
-    tcg_gen_addi_tl(t0, t0, ctx->base.num_insns);
-    gen_store_spr(SPR_POWER_PMC5, t0);
-
-    tcg_temp_free(t0);
-#endif /* #if !defined(CONFIG_USER_ONLY) */
-}
-#else
-static void pmu_count_insns(DisasContext *ctx)
-{
-    return;
-}
-#endif /* #if defined(TARGET_PPC64) */
+/* For pmu_count_insns */
+#include "target/ppc/power8-pmu-insn-cnt.c.inc"
 
 static inline bool use_goto_tb(DisasContext *ctx, target_ulong dest)
 {
-- 
2.33.1

[PATCH v2 2/5] target/ppc/power8-pmu-insn-cnt: introduce inc_spr_if_cond()

[Daniel Henrique Barboza]

The code that increments a PMC is repetitive: check if a given register
has a bit/mask set or cleared and increment the counter.

inc_spr_if_cond() will help deal with this repetition. This patch also
gives a sample of how the function works by incrementing PMC5, which is
supposed to be incremented only if MMCR0_FC56 is not set.

We've also removing the call from the helper since that would cause
PMC5 to be counted twice.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 target/ppc/power8-pmu-insn-cnt.c.inc | 43 ++++++++++++++++++++++------
 1 file changed, 34 insertions(+), 9 deletions(-)

diff --git a/target/ppc/power8-pmu-insn-cnt.c.inc b/target/ppc/power8-pmu-insn-cnt.c.inc
index 6cdf2d2d88..3cfb801c69 100644
--- a/target/ppc/power8-pmu-insn-cnt.c.inc
+++ b/target/ppc/power8-pmu-insn-cnt.c.inc
@@ -10,6 +10,38 @@
  * See the COPYING file in the top-level directory.
  */
 
+#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
+/*
+ * This function increments a SPR 'spr' by 'inc_val' if a given
+ * register 'reg' has a bitmask 'mask' set (cond = TCG_COND_EQ) or
+ * not set (TCG_COND_NE).
+ */
+static void inc_spr_if_cond(int reg, uint64_t mask, TCGCond cond,
+                            int spr, int inc_val)
+{
+    TCGCond exit_cond = tcg_invert_cond(cond);
+    TCGLabel *l_exit;
+    TCGv t0, t1;
+
+    l_exit = gen_new_label();
+
+    t0 = tcg_temp_new();
+    gen_load_spr(t0, reg);
+    tcg_gen_andi_tl(t0, t0, mask);
+    tcg_gen_brcondi_tl(exit_cond, t0, mask, l_exit);
+
+    t1 = tcg_temp_new();
+    gen_load_spr(t1, spr);
+    tcg_gen_addi_tl(t1, t1, inc_val);
+    gen_store_spr(spr, t1);
+
+    gen_set_label(l_exit);
+
+    tcg_temp_free(t0);
+    tcg_temp_free(t1);
+}
+#endif /* #if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY) */
+
 #if defined(TARGET_PPC64)
 static void pmu_count_insns(DisasContext *ctx)
 {
@@ -22,15 +54,8 @@ static void pmu_count_insns(DisasContext *ctx)
     }
 
  #if !defined(CONFIG_USER_ONLY)
-    /*
-     * 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));
+    inc_spr_if_cond(SPR_POWER_MMCR0, MMCR0_FC56, TCG_COND_NE,
+                    SPR_POWER_PMC5, ctx->base.num_insns);
 #else
     /*
      * User mode can read (but not write) PMC5 and start/stop
-- 
2.33.1

[PATCH v2 3/5] target/ppc/power8-pmu-insn-cnt: add PMCs1-4 insn count

[Daniel Henrique Barboza]

Use inc_spr_if_cond() to count instructions of all other PMCs that are
capable of counting instructions (all PMCs but PMC6).

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 target/ppc/power8-pmu-insn-cnt.c.inc | 62 ++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

diff --git a/target/ppc/power8-pmu-insn-cnt.c.inc b/target/ppc/power8-pmu-insn-cnt.c.inc
index 3cfb801c69..a5a0d42e71 100644
--- a/target/ppc/power8-pmu-insn-cnt.c.inc
+++ b/target/ppc/power8-pmu-insn-cnt.c.inc
@@ -11,6 +11,13 @@
  */
 
 #if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
+#define MMCR1_PMC1_INS_CNT        0x02000000
+#define MMCR1_PMC1_INS_CNT2       0xFE000000
+#define MMCR1_PMC2_INS_CNT        0x00020000
+#define MMCR1_PMC3_INS_CNT        0x00000200
+#define MMCR1_PMC4_INS_CNT        0x00000002
+#define MMCR1_PMC4_INS_LATCH_CNT  0x000000FA
+
 /*
  * This function increments a SPR 'spr' by 'inc_val' if a given
  * register 'reg' has a bitmask 'mask' set (cond = TCG_COND_EQ) or
@@ -54,8 +61,63 @@ static void pmu_count_insns(DisasContext *ctx)
     }
 
  #if !defined(CONFIG_USER_ONLY)
+    TCGv t_mmcr0, t_mmcr1, t_ctrl;
+    TCGLabel *l_skip_pmc14;
+
     inc_spr_if_cond(SPR_POWER_MMCR0, MMCR0_FC56, TCG_COND_NE,
                     SPR_POWER_PMC5, ctx->base.num_insns);
+
+    /*
+     * Skip PMC1-4 increment if:
+     * - MMCR0_FC14 is set OR
+     * - MMCR1 is cleared
+     */
+    l_skip_pmc14 = gen_new_label();
+
+    t_mmcr0 = tcg_temp_new();
+    gen_load_spr(t_mmcr0, SPR_POWER_MMCR0);
+    tcg_gen_andi_tl(t_mmcr0, t_mmcr0, MMCR0_FC14);
+    tcg_gen_brcondi_tl(TCG_COND_EQ, t_mmcr0, MMCR0_FC14, l_skip_pmc14);
+
+    t_mmcr1 = tcg_temp_new();
+    gen_load_spr(t_mmcr1, SPR_POWER_MMCR1);
+    tcg_gen_brcondi_tl(TCG_COND_EQ, t_mmcr1, 0x0, l_skip_pmc14);
+
+    /* PMC1 is incremented if PMC1SEL = 0x02 or 0xFE */
+    inc_spr_if_cond(SPR_POWER_MMCR1, MMCR1_PMC1_INS_CNT, TCG_COND_EQ,
+                    SPR_POWER_PMC1, ctx->base.num_insns);
+    inc_spr_if_cond(SPR_POWER_MMCR1, MMCR1_PMC1_INS_CNT2, TCG_COND_EQ,
+                    SPR_POWER_PMC1, ctx->base.num_insns);
+
+    /* PMC2 is incremented if PMC2SEL = 0x02 */
+    inc_spr_if_cond(SPR_POWER_MMCR1, MMCR1_PMC2_INS_CNT, TCG_COND_EQ,
+                    SPR_POWER_PMC2, ctx->base.num_insns);
+
+    /* PMC3 is incremented if PMC3SEL = 0x02 */
+    inc_spr_if_cond(SPR_POWER_MMCR1, MMCR1_PMC3_INS_CNT, TCG_COND_EQ,
+                    SPR_POWER_PMC3, ctx->base.num_insns);
+
+    /*
+     * PMC4 is incremented if PMC4SEL = 0x02 or 0xFA. 0xFA depends on the
+     * run latch (SPR_CTRL & CTRL_RUN). Check for the run latch before
+     * checking for PMC4SEL = 0xFA.
+     */
+    inc_spr_if_cond(SPR_POWER_MMCR1, MMCR1_PMC4_INS_CNT, TCG_COND_EQ,
+                    SPR_POWER_PMC4, ctx->base.num_insns);
+
+    t_ctrl = tcg_temp_new();
+    gen_load_spr(t_ctrl, SPR_CTRL);
+    tcg_gen_andi_tl(t_ctrl, t_ctrl, CTRL_RUN);
+    tcg_gen_brcondi_tl(TCG_COND_NE, t_ctrl, CTRL_RUN, l_skip_pmc14);
+
+    inc_spr_if_cond(SPR_POWER_MMCR1, MMCR1_PMC4_INS_LATCH_CNT, TCG_COND_EQ,
+                    SPR_POWER_PMC4, ctx->base.num_insns);
+
+    gen_set_label(l_skip_pmc14);
+
+    tcg_temp_free(t_mmcr0);
+    tcg_temp_free(t_mmcr1);
+    tcg_temp_free(t_ctrl);
 #else
     /*
      * User mode can read (but not write) PMC5 and start/stop
-- 
2.33.1

[PATCH v2 4/5] target/ppc/power8-pmu-insn-cnt: add pmu_check_overflow()

[Daniel Henrique Barboza]

pmu_check_overflow() will verify for overflow in the PMC1-5 counters,
firing a performance monitor alert if an overflow happened with the
proper MMCR0 bits set.

The alert is fired by using helper_pmu_overflow().

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 target/ppc/helper.h                  |  1 +
 target/ppc/power8-pmu-insn-cnt.c.inc | 76 ++++++++++++++++++++++++++++
 target/ppc/power8-pmu.c              |  8 +++
 3 files changed, 85 insertions(+)

diff --git a/target/ppc/helper.h b/target/ppc/helper.h
index fb6cac38b4..4d8193caab 100644
--- a/target/ppc/helper.h
+++ b/target/ppc/helper.h
@@ -26,6 +26,7 @@ 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)
+DEF_HELPER_1(pmu_overflow, void, env)
 #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-insn-cnt.c.inc b/target/ppc/power8-pmu-insn-cnt.c.inc
index a5a0d42e71..6e0e4e1270 100644
--- a/target/ppc/power8-pmu-insn-cnt.c.inc
+++ b/target/ppc/power8-pmu-insn-cnt.c.inc
@@ -18,6 +18,8 @@
 #define MMCR1_PMC4_INS_CNT        0x00000002
 #define MMCR1_PMC4_INS_LATCH_CNT  0x000000FA
 
+#define PMC_COUNTER_NEGATIVE_VAL  0x80000000UL
+
 /*
  * This function increments a SPR 'spr' by 'inc_val' if a given
  * register 'reg' has a bitmask 'mask' set (cond = TCG_COND_EQ) or
@@ -47,6 +49,78 @@ static void inc_spr_if_cond(int reg, uint64_t mask, TCGCond cond,
     tcg_temp_free(t0);
     tcg_temp_free(t1);
 }
+
+/*
+ * Check for overflow of PMC1-PMC5 counters and call the overflow
+ * helper in case any counter has overflown.
+ */
+static void pmu_check_overflow(DisasContext *ctx)
+{
+    TCGv t0, t1, t_pmc1, t_pmc;
+    TCGLabel *l_pmc_overflow;
+    TCGLabel *l_skip_pmc1_overflow;
+    TCGLabel *l_skip_overflow;
+    int i;
+
+    /*
+     * Check if we have overflow bits set and fire an overflow
+     * event if necessary. Skip directly to 'l_pmc_overflow'
+     * right after finding the first overflow.
+     */
+    l_pmc_overflow = gen_new_label();
+    l_skip_pmc1_overflow = gen_new_label();
+
+    t0 = tcg_temp_new();
+    gen_load_spr(t0, SPR_POWER_MMCR0);
+    tcg_gen_andi_tl(t0, t0, MMCR0_PMC1CE);
+    tcg_gen_brcondi_tl(TCG_COND_NE, t0, MMCR0_PMC1CE, l_skip_pmc1_overflow);
+
+    t_pmc1 = tcg_temp_new();
+    gen_load_spr(t_pmc1, SPR_POWER_PMC1);
+    tcg_gen_brcondi_tl(TCG_COND_GE, t_pmc1, PMC_COUNTER_NEGATIVE_VAL,
+                       l_pmc_overflow);
+
+    gen_set_label(l_skip_pmc1_overflow);
+
+    l_skip_overflow = gen_new_label();
+
+    /*
+     * At this point we're sure PMC1 didn't overflow. If MMCR0_PMCjCE
+     * isn't set we can skip everything since PMC2-5 overflow is
+     * disabled.
+     */
+    t1 = tcg_temp_new();
+    gen_load_spr(t1, SPR_POWER_MMCR0);
+    tcg_gen_andi_tl(t1, t1, MMCR0_PMCjCE);
+    tcg_gen_brcondi_tl(TCG_COND_NE, t1, MMCR0_PMCjCE, l_skip_overflow);
+
+    for (i = SPR_POWER_PMC2; i < SPR_POWER_PMC6; i++) {
+        t_pmc = tcg_temp_new();
+        gen_load_spr(t_pmc, i);
+        tcg_gen_brcondi_tl(TCG_COND_GE, t_pmc, PMC_COUNTER_NEGATIVE_VAL,
+                           l_pmc_overflow);
+        tcg_temp_free(t_pmc);
+    }
+
+    tcg_gen_br(l_skip_overflow);
+
+    gen_set_label(l_pmc_overflow);
+
+    /*
+     * The PMU overflow helper manipulates the internal PMU timer.
+     * 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_pmu_overflow(cpu_env);
+
+    gen_set_label(l_skip_overflow);
+
+    tcg_temp_free(t0);
+    tcg_temp_free(t1);
+    tcg_temp_free(t_pmc1);
+}
 #endif /* #if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY) */
 
 #if defined(TARGET_PPC64)
@@ -115,6 +189,8 @@ static void pmu_count_insns(DisasContext *ctx)
 
     gen_set_label(l_skip_pmc14);
 
+    pmu_check_overflow(ctx);
+
     tcg_temp_free(t_mmcr0);
     tcg_temp_free(t_mmcr1);
     tcg_temp_free(t_ctrl);
diff --git a/target/ppc/power8-pmu.c b/target/ppc/power8-pmu.c
index 08d1902cd5..6696c9b3ae 100644
--- a/target/ppc/power8-pmu.c
+++ b/target/ppc/power8-pmu.c
@@ -323,6 +323,14 @@ void helper_insns_inc(CPUPPCState *env, uint32_t num_insns)
     }
 }
 
+/* Helper to fire a PMC interrupt from TCG code */
+void helper_pmu_overflow(CPUPPCState *env)
+{
+    PowerPCCPU *cpu = env_archcpu(env);
+
+    fire_PMC_interrupt(cpu);
+}
+
 static void cpu_ppc_pmu_timer_cb(void *opaque)
 {
     PowerPCCPU *cpu = opaque;
-- 
2.33.1

[PATCH v2 5/5] target/ppc/power8-pmu.c: remove helper_insns_inc()

[Daniel Henrique Barboza]

After moving all the instruction counting to TCG Ops code
this helper is not needed anymore.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 target/ppc/helper.h                  |  1 -
 target/ppc/power8-pmu-insn-cnt.c.inc |  4 --
 target/ppc/power8-pmu.c              | 60 ----------------------------
 3 files changed, 65 deletions(-)

diff --git a/target/ppc/helper.h b/target/ppc/helper.h
index 4d8193caab..de80e82ebe 100644
--- a/target/ppc/helper.h
+++ b/target/ppc/helper.h
@@ -25,7 +25,6 @@ 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)
 DEF_HELPER_1(pmu_overflow, void, env)
 #endif
 DEF_HELPER_1(check_tlb_flush_local, void, env)
diff --git a/target/ppc/power8-pmu-insn-cnt.c.inc b/target/ppc/power8-pmu-insn-cnt.c.inc
index 6e0e4e1270..adb796c1c1 100644
--- a/target/ppc/power8-pmu-insn-cnt.c.inc
+++ b/target/ppc/power8-pmu-insn-cnt.c.inc
@@ -126,10 +126,6 @@ static void pmu_check_overflow(DisasContext *ctx)
 #if defined(TARGET_PPC64)
 static void pmu_count_insns(DisasContext *ctx)
 {
-    /*
-     * Do not bother calling the helper if the PMU isn't counting
-     * instructions.
-     */
     if (!ctx->pmu_insn_cnt) {
         return;
     }
diff --git a/target/ppc/power8-pmu.c b/target/ppc/power8-pmu.c
index 6696c9b3ae..bfc052b49e 100644
--- a/target/ppc/power8-pmu.c
+++ b/target/ppc/power8-pmu.c
@@ -135,52 +135,6 @@ bool pmu_insn_cnt_enabled(CPUPPCState *env)
     return false;
 }
 
-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++) {
-        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;
-        }
-
-        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)) {
-
-            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;
-        }
-    }
-
-    return overflow_triggered;
-}
-
 static void pmu_update_cycles(CPUPPCState *env)
 {
     uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
@@ -309,20 +263,6 @@ 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);
-    }
-}
-
 /* Helper to fire a PMC interrupt from TCG code */
 void helper_pmu_overflow(CPUPPCState *env)
 {
-- 
2.33.1

Re: [PATCH v2 2/5] target/ppc/power8-pmu-insn-cnt: introduce inc_spr_if_cond()

[Richard Henderson]

On 12/23/21 12:18 PM, Daniel Henrique Barboza wrote:
> The code that increments a PMC is repetitive: check if a given register
> has a bit/mask set or cleared and increment the counter.
> 
> inc_spr_if_cond() will help deal with this repetition. This patch also
> gives a sample of how the function works by incrementing PMC5, which is
> supposed to be incremented only if MMCR0_FC56 is not set.
> 
> We've also removing the call from the helper since that would cause
> PMC5 to be counted twice.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>   target/ppc/power8-pmu-insn-cnt.c.inc | 43 ++++++++++++++++++++++------
>   1 file changed, 34 insertions(+), 9 deletions(-)
> 
> diff --git a/target/ppc/power8-pmu-insn-cnt.c.inc b/target/ppc/power8-pmu-insn-cnt.c.inc
> index 6cdf2d2d88..3cfb801c69 100644
> --- a/target/ppc/power8-pmu-insn-cnt.c.inc
> +++ b/target/ppc/power8-pmu-insn-cnt.c.inc
> @@ -10,6 +10,38 @@
>    * See the COPYING file in the top-level directory.
>    */
>   
> +#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
> +/*
> + * This function increments a SPR 'spr' by 'inc_val' if a given
> + * register 'reg' has a bitmask 'mask' set (cond = TCG_COND_EQ) or
> + * not set (TCG_COND_NE).
> + */
> +static void inc_spr_if_cond(int reg, uint64_t mask, TCGCond cond,
> +                            int spr, int inc_val)
> +{
> +    TCGCond exit_cond = tcg_invert_cond(cond);
> +    TCGLabel *l_exit;
> +    TCGv t0, t1;
> +
> +    l_exit = gen_new_label();
> +
> +    t0 = tcg_temp_new();
> +    gen_load_spr(t0, reg);
> +    tcg_gen_andi_tl(t0, t0, mask);
> +    tcg_gen_brcondi_tl(exit_cond, t0, mask, l_exit);

When testing a single bit, compare against 0, not the bit.

> +    t1 = tcg_temp_new();
> +    gen_load_spr(t1, spr);
> +    tcg_gen_addi_tl(t1, t1, inc_val);
> +    gen_store_spr(spr, t1);

It will probably perform better to make this a true conditional add.
I.e.

     gen_load_spr(t0, spr);
     tcg_gen_addi_tl(t1, t0, inc);
     tcg_gen_movcond_tl(cond, t0, reg, zero, t1, t0);
     gen_store_spr(spr, t0);

> -    /*
> -     * 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);

Removing this is incorrect.

> -    gen_helper_insns_inc(cpu_env, tcg_constant_i32(ctx->base.num_insns));
> +    inc_spr_if_cond(SPR_POWER_MMCR0, MMCR0_FC56, TCG_COND_NE,
> +                    SPR_POWER_PMC5, ctx->base.num_insns);

This is non-bisectable.  You're removing support for all registers and only adding back 
PMC5.  You add them all back before the end of the series, but the middle patches do not 
behave correctly.


r~

Re: [PATCH v2 0/5] Re-write PPC64 PMU instruction count using TCG Ops

[Cédric Le Goater]

On 12/23/21 21:18, Daniel Henrique Barboza wrote:
> Hi,
> 
> In this version the tedious repetition was taken away from the
> code by using a helper that increments the PMCs based on specified
> conditions.
> 
> As far as Avocado test goes, the performance is the same as the previous
> version. All PMU-EBB kernel selftests are also passing. Basically we have
> the same benefits from v1 but 150+ lines shorter.
> 
> changes from v1:
> - former patches 2-6: removed
> - new patch 2:
>    * added inc_spr_if_cond() helper
> - new patch 3:
>    * add insn count for PMCs 1-4
> - patch 4 (former 7):
>    * use a loop to reduce code repetition when checking for counter
> overflows
> - v1 link: https://lists.gnu.org/archive/html/qemu-devel/2021-12/msg03871.html

I understand that you are going to rebase on top of Richard's patchset.
and so this series is now obsolete ?

Thanks,

C.
  

Re: [PATCH v2 0/5] Re-write PPC64 PMU instruction count using TCG Ops

[Daniel Henrique Barboza]

On 1/3/22 03:46, Cédric Le Goater wrote:
> On 12/23/21 21:18, Daniel Henrique Barboza wrote:
>> Hi,
>>
>> In this version the tedious repetition was taken away from the
>> code by using a helper that increments the PMCs based on specified
>> conditions.
>>
>> As far as Avocado test goes, the performance is the same as the previous
>> version. All PMU-EBB kernel selftests are also passing. Basically we have
>> the same benefits from v1 but 150+ lines shorter.
>>
>> changes from v1:
>> - former patches 2-6: removed
>> - new patch 2:
>>    * added inc_spr_if_cond() helper
>> - new patch 3:
>>    * add insn count for PMCs 1-4
>> - patch 4 (former 7):
>>    * use a loop to reduce code repetition when checking for counter
>> overflows
>> - v1 link: https://lists.gnu.org/archive/html/qemu-devel/2021-12/msg03871.html
> 
> I understand that you are going to rebase on top of Richard's patchset.
> and so this series is now obsolete ?

Yes it is. I'll post a new version of Richard's patchset shortly.


Daniel

> 
> Thanks,
> 
> C.
>