[PATCH v3 00/15] PMU-EBB support for PPC64 TCG

[PATCH v3 00/15] PMU-EBB support for PPC64 TCG

[Daniel Henrique Barboza]

Hi,

This version contains changes suggested by David and Matheus. No big
design changes were made.


Changes from v2:
- former patch 1: merged into patch 2 (former 3)
- patches 1 and 2 (former 2 and 3):
  * no more intermediate write reg functions that will replaced shortly after
  * do not name specialized write callbacks with '_generic'
  * put the read/write MMCR0 ureg bits in a macro
- patch 3 (former 4): 
  * do not call 'helper_store_mmcr0' in spr_write_MMCR0_ureg
- patch 4 (former 5):
  * use extract64 when extracting MMCR1 events
- patch 5 (former 6):
  * MMCR0_FC is now represented in a DisasContext flag to avoid poking into
  registers before calling the instruction count helper
- patch 6 (former 7):
  * writing SPR_CTRL now forces a new translation block
- patch 8 (former 9):
  * rename device_tree rfebb format and argument
  * crop EBBRR when !msr_is_64bit
  * put the helper inside an "#if defined(TARGET_PPPC64)"
  * use gen_invalid() when CONFIG_USER_ONLY
- v2 link: https://lists.gnu.org/archive/html/qemu-devel/2021-08/msg04062.html


Daniel Henrique Barboza (13):
  target/ppc: add user write access control for PMU SPRs
  target/ppc: PMU basic cycle count for pseries TCG
  target/ppc/power8_pmu.c: enable PMC1-PMC4 events
  target/ppc: PMU: add instruction counting
  target/ppc/power8_pmu.c: add PM_RUN_INST_CMPL (0xFA) event
  target/ppc/power8_pmu.c: add PMC14/PMC56 counter freeze bits
  PPC64/TCG: Implement 'rfebb' instruction
  target/ppc/excp_helper.c: EBB handling adjustments
  target/ppc/power8_pmu.c: enable PMC1 counter negative overflow
  target/ppc/power8_pmu.c: cycles overflow with all PMCs
  target/ppc: PMU: insns counter negative overflow support
  target/ppc/translate: PMU: handle setting of PMCs while running
  target/ppc/power8_pmu.c: handle overflow bits when PMU is running

Gustavo Romero (2):
  target/ppc: add user read functions for MMCR0 and MMCR2
  target/ppc: PMU Event-Based exception support

 hw/ppc/spapr_cpu_core.c                |   6 +
 target/ppc/cpu.h                       |  61 +++-
 target/ppc/cpu_init.c                  |  38 +-
 target/ppc/excp_helper.c               |  92 +++++
 target/ppc/helper.h                    |   4 +
 target/ppc/helper_regs.c               |   6 +
 target/ppc/insn32.decode               |   5 +
 target/ppc/meson.build                 |   1 +
 target/ppc/power8_pmu.c                | 470 +++++++++++++++++++++++++
 target/ppc/power8_pmu.h                |  25 ++
 target/ppc/spr_tcg.h                   |   9 +-
 target/ppc/translate.c                 | 214 ++++++++++-
 target/ppc/translate/branch-impl.c.inc |  33 ++
 13 files changed, 942 insertions(+), 22 deletions(-)
 create mode 100644 target/ppc/power8_pmu.c
 create mode 100644 target/ppc/power8_pmu.h
 create mode 100644 target/ppc/translate/branch-impl.c.inc

-- 
2.31.1

[PATCH v3 01/15] target/ppc: add user read functions for MMCR0 and MMCR2

[Daniel Henrique Barboza]

From: Gustavo Romero <gromero@linux.ibm.com>

We're going to add PMU support for TCG PPC64 chips, based on IBM POWER8+
emulation and following PowerISA v3.1.

Let's start by handling the user read of UMMCR0 and UMMCR2. According to
PowerISA 3.1 these registers omit some of its bits from userspace.

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       | 10 ++++++++++
 target/ppc/cpu_init.c  |  4 ++--
 target/ppc/spr_tcg.h   |  2 ++
 target/ppc/translate.c | 37 +++++++++++++++++++++++++++++++++++++
 4 files changed, 51 insertions(+), 2 deletions(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 500205229c..f68bb8d8aa 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -342,6 +342,16 @@ typedef struct ppc_v3_pate_t {
 #define MSR_RI   1  /* Recoverable interrupt                        1        */
 #define MSR_LE   0  /* Little-endian mode                           1 hflags */
 
+/* PMU bits */
+#define MMCR0_FC    PPC_BIT(32)         /* Freeze Counters  */
+#define MMCR0_PMAO  PPC_BIT(56)         /* Perf Monitor Alert Ocurred */
+#define MMCR0_PMAE  PPC_BIT(37)         /* Perf Monitor Alert Enable */
+#define MMCR0_EBE   PPC_BIT(43)         /* Perf Monitor EBB Enable */
+#define MMCR0_FCECE PPC_BIT(38)         /* FC on Enabled Cond or Event */
+#define MMCR0_PMCC  PPC_BITMASK(44, 45) /* PMC Control */
+/* MMCR0 userspace r/w mask */
+#define MMCR0_UREG_MASK (MMCR0_FC | MMCR0_PMAO | MMCR0_PMAE)
+
 /* 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 ad7abc6041..9efc6c2d87 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -6867,7 +6867,7 @@ static void register_book3s_pmu_sup_sprs(CPUPPCState *env)
 static void register_book3s_pmu_user_sprs(CPUPPCState *env)
 {
     spr_register(env, SPR_POWER_UMMCR0, "UMMCR0",
-                 &spr_read_ureg, SPR_NOACCESS,
+                 &spr_read_MMCR0_ureg, SPR_NOACCESS,
                  &spr_read_ureg, &spr_write_ureg,
                  0x00000000);
     spr_register(env, SPR_POWER_UMMCR1, "UMMCR1",
@@ -6975,7 +6975,7 @@ static void register_power8_pmu_sup_sprs(CPUPPCState *env)
 static void register_power8_pmu_user_sprs(CPUPPCState *env)
 {
     spr_register(env, SPR_POWER_UMMCR2, "UMMCR2",
-                 &spr_read_ureg, SPR_NOACCESS,
+                 &spr_read_MMCR2_ureg, SPR_NOACCESS,
                  &spr_read_ureg, &spr_write_ureg,
                  0x00000000);
     spr_register(env, SPR_POWER_USIER, "USIER",
diff --git a/target/ppc/spr_tcg.h b/target/ppc/spr_tcg.h
index 0be5f347d5..30cb6c3fdc 100644
--- a/target/ppc/spr_tcg.h
+++ b/target/ppc/spr_tcg.h
@@ -32,6 +32,8 @@ void spr_write_lr(DisasContext *ctx, int sprn, int gprn);
 void spr_read_ctr(DisasContext *ctx, int gprn, int sprn);
 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_tbl(DisasContext *ctx, int gprn, int sprn);
 void spr_read_tbu(DisasContext *ctx, int gprn, int sprn);
 void spr_read_atbl(DisasContext *ctx, int gprn, int sprn);
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 171b216e17..b2ead144d1 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -519,6 +519,43 @@ void spr_read_ureg(DisasContext *ctx, int gprn, int sprn)
     gen_load_spr(cpu_gpr[gprn], sprn + 0x10);
 }
 
+void spr_read_MMCR0_ureg(DisasContext *ctx, int gprn, int sprn)
+{
+    TCGv t0 = tcg_temp_new();
+
+    /*
+     * Filter out all bits but FC, PMAO, and PMAE, according
+     * to ISA v3.1, in 10.4.4 Monitor Mode Control Register 0,
+     * fourth paragraph.
+     */
+    gen_load_spr(t0, SPR_POWER_MMCR0);
+    tcg_gen_andi_tl(t0, t0, MMCR0_UREG_MASK);
+    tcg_gen_mov_tl(cpu_gpr[gprn], t0);
+
+    tcg_temp_free(t0);
+}
+
+void spr_read_MMCR2_ureg(DisasContext *ctx, int gprn, int sprn)
+{
+    TCGv t0 = tcg_temp_new();
+
+    /*
+     * On read, filter out all bits that are not FCnP0 bits.
+     * When MMCR0[PMCC] is set to 0b10 or 0b11, providing
+     * problem state programs read/write access to MMCR2,
+     * only the FCnP0 bits can be accessed. All other bits are
+     * not changed when mtspr is executed in problem state, and
+     * all other bits return 0s when mfspr is executed in problem
+     * state, according to ISA v3.1, section 10.4.6 Monitor Mode
+     * Control Register 2, p. 1316, third paragraph.
+     */
+    gen_load_spr(t0, SPR_POWER_MMCR2);
+    tcg_gen_andi_tl(t0, t0, 0x4020100804020000UL);
+    tcg_gen_mov_tl(cpu_gpr[gprn], t0);
+
+    tcg_temp_free(t0);
+}
+
 #if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
 void spr_write_ureg(DisasContext *ctx, int sprn, int gprn)
 {
-- 
2.31.1

[PATCH v3 02/15] target/ppc: add user write access control for PMU SPRs

[Daniel Henrique Barboza]

The PMU needs to enable writing of its uregs to userspace, otherwise
Perf applications will not able to setup the counters correctly. This
patch enables user space writing of all PMU uregs.

MMCR0 is a special case because its userspace writing access is controlled
by MMCR0_PMCC bits. There are 4 configurations available (0b00, 0b01,
0b10 and 0b11) but for our purposes here we're handling only
MMCR0_PMCC = 0b00. In this case, if userspace tries to write MMCR0, a
hypervisor emulation assistance interrupt occurs.

This is being done by adding HFLAGS_PMCCCLEAR to hflags. This flag
indicates if MMCR0_PMCC is cleared (0b00), and a new 'pmcc_clear' flag in
DisasContext allow us to use it in spr_write_MMCR0_ureg().

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 target/ppc/cpu.h         |  1 +
 target/ppc/cpu_init.c    | 18 +++++++-------
 target/ppc/helper_regs.c |  3 +++
 target/ppc/spr_tcg.h     |  3 ++-
 target/ppc/translate.c   | 53 +++++++++++++++++++++++++++++++++++++++-
 5 files changed, 67 insertions(+), 11 deletions(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index f68bb8d8aa..8dfbb62022 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -616,6 +616,7 @@ enum {
     HFLAGS_SE = 10,  /* MSR_SE -- from elsewhere on embedded ppc */
     HFLAGS_FP = 13,  /* MSR_FP */
     HFLAGS_PR = 14,  /* MSR_PR */
+    HFLAGS_PMCCCLEAR = 15, /* PMU MMCR0 PMCC equal to 0b00 */
     HFLAGS_VSX = 23, /* MSR_VSX if cpu has VSX */
     HFLAGS_VR = 25,  /* MSR_VR if cpu has VRE */
 
diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
index 9efc6c2d87..bb5ea04c61 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -6867,7 +6867,7 @@ static void register_book3s_pmu_sup_sprs(CPUPPCState *env)
 static void register_book3s_pmu_user_sprs(CPUPPCState *env)
 {
     spr_register(env, SPR_POWER_UMMCR0, "UMMCR0",
-                 &spr_read_MMCR0_ureg, SPR_NOACCESS,
+                 &spr_read_MMCR0_ureg, &spr_write_MMCR0_ureg,
                  &spr_read_ureg, &spr_write_ureg,
                  0x00000000);
     spr_register(env, SPR_POWER_UMMCR1, "UMMCR1",
@@ -6875,31 +6875,31 @@ static void register_book3s_pmu_user_sprs(CPUPPCState *env)
                  &spr_read_ureg, &spr_write_ureg,
                  0x00000000);
     spr_register(env, SPR_POWER_UMMCRA, "UMMCRA",
-                 &spr_read_ureg, SPR_NOACCESS,
+                 &spr_read_ureg, &spr_write_ureg,
                  &spr_read_ureg, &spr_write_ureg,
                  0x00000000);
     spr_register(env, SPR_POWER_UPMC1, "UPMC1",
-                 &spr_read_ureg, SPR_NOACCESS,
+                 &spr_read_ureg, &spr_write_ureg,
                  &spr_read_ureg, &spr_write_ureg,
                  0x00000000);
     spr_register(env, SPR_POWER_UPMC2, "UPMC2",
-                 &spr_read_ureg, SPR_NOACCESS,
+                 &spr_read_ureg, &spr_write_ureg,
                  &spr_read_ureg, &spr_write_ureg,
                  0x00000000);
     spr_register(env, SPR_POWER_UPMC3, "UPMC3",
-                 &spr_read_ureg, SPR_NOACCESS,
+                 &spr_read_ureg, &spr_write_ureg,
                  &spr_read_ureg, &spr_write_ureg,
                  0x00000000);
     spr_register(env, SPR_POWER_UPMC4, "UPMC4",
-                 &spr_read_ureg, SPR_NOACCESS,
+                 &spr_read_ureg, &spr_write_ureg,
                  &spr_read_ureg, &spr_write_ureg,
                  0x00000000);
     spr_register(env, SPR_POWER_UPMC5, "UPMC5",
-                 &spr_read_ureg, SPR_NOACCESS,
+                 &spr_read_ureg, &spr_write_ureg,
                  &spr_read_ureg, &spr_write_ureg,
                  0x00000000);
     spr_register(env, SPR_POWER_UPMC6, "UPMC6",
-                 &spr_read_ureg, SPR_NOACCESS,
+                 &spr_read_ureg, &spr_write_ureg,
                  &spr_read_ureg, &spr_write_ureg,
                  0x00000000);
     spr_register(env, SPR_POWER_USIAR, "USIAR",
@@ -6975,7 +6975,7 @@ static void register_power8_pmu_sup_sprs(CPUPPCState *env)
 static void register_power8_pmu_user_sprs(CPUPPCState *env)
 {
     spr_register(env, SPR_POWER_UMMCR2, "UMMCR2",
-                 &spr_read_MMCR2_ureg, SPR_NOACCESS,
+                 &spr_read_MMCR2_ureg, &spr_write_ureg,
                  &spr_read_ureg, &spr_write_ureg,
                  0x00000000);
     spr_register(env, SPR_POWER_USIER, "USIER",
diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
index 405450d863..4c1d9575ac 100644
--- a/target/ppc/helper_regs.c
+++ b/target/ppc/helper_regs.c
@@ -106,6 +106,9 @@ static uint32_t hreg_compute_hflags_value(CPUPPCState *env)
     if (env->spr[SPR_LPCR] & LPCR_GTSE) {
         hflags |= 1 << HFLAGS_GTSE;
     }
+    if (((env->spr[SPR_POWER_MMCR0] & MMCR0_PMCC) >> 18) == 0) {
+        hflags |= 1 << HFLAGS_PMCCCLEAR;
+    }
 
 #ifndef CONFIG_USER_ONLY
     if (!env->has_hv_mode || (msr & (1ull << MSR_HV))) {
diff --git a/target/ppc/spr_tcg.h b/target/ppc/spr_tcg.h
index 30cb6c3fdc..094466a2b2 100644
--- a/target/ppc/spr_tcg.h
+++ b/target/ppc/spr_tcg.h
@@ -42,6 +42,8 @@ void spr_read_601_rtcl(DisasContext *ctx, int gprn, int sprn);
 void spr_read_601_rtcu(DisasContext *ctx, int gprn, int sprn);
 void spr_read_spefscr(DisasContext *ctx, int gprn, int sprn);
 void spr_write_spefscr(DisasContext *ctx, int sprn, int gprn);
+void spr_write_ureg(DisasContext *ctx, int sprn, int gprn);
+void spr_write_MMCR0_ureg(DisasContext *ctx, int sprn, int gprn);
 
 #ifndef CONFIG_USER_ONLY
 void spr_write_generic32(DisasContext *ctx, int sprn, int gprn);
@@ -96,7 +98,6 @@ void spr_read_mas73(DisasContext *ctx, int gprn, int sprn);
 #ifdef TARGET_PPC64
 void spr_read_cfar(DisasContext *ctx, int gprn, int sprn);
 void spr_write_cfar(DisasContext *ctx, int sprn, int gprn);
-void spr_write_ureg(DisasContext *ctx, int sprn, int gprn);
 void spr_read_purr(DisasContext *ctx, int gprn, int sprn);
 void spr_write_purr(DisasContext *ctx, int sprn, int gprn);
 void spr_read_hdecr(DisasContext *ctx, int gprn, int sprn);
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index b2ead144d1..0babde3131 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -175,6 +175,7 @@ struct DisasContext {
     bool spe_enabled;
     bool tm_enabled;
     bool gtse;
+    bool pmcc_clear;
     ppc_spr_t *spr_cb; /* Needed to check rights for mfspr/mtspr */
     int singlestep_enabled;
     uint32_t flags;
@@ -561,7 +562,56 @@ void spr_write_ureg(DisasContext *ctx, int sprn, int gprn)
 {
     gen_store_spr(sprn + 0x10, cpu_gpr[gprn]);
 }
-#endif
+
+void spr_write_MMCR0_ureg(DisasContext *ctx, int sprn, int gprn)
+{
+    TCGv t0, t1;
+
+    /*
+     * For group A PMU sprs, if PMCC = 0b00, PowerISA v3.1
+     * dictates that:
+     *
+     * "If an attempt is made to write to an SPR in group A in
+     * problem state, a Hypervisor Emulation Assistance
+     * interrupt will occur."
+     *
+     * MMCR0 is a Group A SPR and can't be written by userspace
+     * if PMCC = 0b00.
+     */
+    if (ctx->pmcc_clear) {
+        gen_hvpriv_exception(ctx, POWERPC_EXCP_INVAL_SPR);
+        return;
+    }
+
+    t0 = tcg_temp_new();
+    t1 = tcg_temp_new();
+
+    /*
+     * Filter out all bits but FC, PMAO, and PMAE, according
+     * to ISA v3.1, in 10.4.4 Monitor Mode Control Register 0,
+     * fourth paragraph.
+     */
+    tcg_gen_andi_tl(t0, cpu_gpr[gprn], MMCR0_UREG_MASK);
+    gen_load_spr(t1, SPR_POWER_MMCR0);
+    tcg_gen_andi_tl(t1, t1, ~(MMCR0_UREG_MASK));
+    /* Keep all other bits intact */
+    tcg_gen_or_tl(t1, t1, t0);
+    gen_store_spr(SPR_POWER_MMCR0, t1);
+
+    tcg_temp_free(t0);
+    tcg_temp_free(t1);
+}
+#else
+void spr_write_ureg(DisasContext *ctx, int sprn, int gprn)
+{
+    spr_noaccess(ctx, gprn, sprn);
+}
+
+void spr_write_MMCR0_ureg(DisasContext *ctx, int sprn, int gprn)
+{
+    spr_noaccess(ctx, gprn, sprn);
+}
+#endif /* defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY) */
 
 /* SPR common to all non-embedded PowerPC */
 /* DECR */
@@ -8576,6 +8626,7 @@ static void ppc_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
     ctx->vsx_enabled = (hflags >> HFLAGS_VSX) & 1;
     ctx->tm_enabled = (hflags >> HFLAGS_TM) & 1;
     ctx->gtse = (hflags >> HFLAGS_GTSE) & 1;
+    ctx->pmcc_clear = (hflags >> HFLAGS_PMCCCLEAR) & 1;
 
     ctx->singlestep_enabled = 0;
     if ((hflags >> HFLAGS_SE) & 1) {
-- 
2.31.1

[PATCH v3 03/15] target/ppc: PMU basic cycle count for pseries TCG

[Daniel Henrique Barboza]

This patch adds the barebones of the PMU logic by enabling cycle
counting, done via the performance monitor counter 6. The overall logic
goes as follows:

- a helper is added to control the PMU state on each MMCR0 write. This
allows for the PMU to start/stop as the frozen counter bit (MMCR0_FC)
is cleared or set;

- MMCR0 reg initial value is set to 0x80000000 (MMCR0_FC set) to avoid
having to spin the PMU right at system init;

- the intended usage is to freeze the counters by setting MMCR0_FC, do
any additional setting of events to be counted via MMCR1 (not
implemented yet) and enable the PMU by zeroing MMCR0_FC. Software must
freeze counters to read the results - on the fly reading of the PMCs
will return the starting value of each one.

Since there will be more PMU exclusive code to be added next, put the
PMU logic in its own helper to keep all in the same place. The name of
the new helper file, power8_pmu.c, is an indicative that the PMU logic
has been tested with the IBM POWER chip family, POWER8 being the oldest
version tested. This doesn't mean that this PMU logic will break with
any other PPC64 chip that implements Book3s, but since we can't assert
that this PMU will work with all available Book3s emulated processors
we're choosing to be explicit.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 target/ppc/cpu.h        |  6 ++++
 target/ppc/cpu_init.c   |  6 ++--
 target/ppc/helper.h     |  1 +
 target/ppc/meson.build  |  1 +
 target/ppc/power8_pmu.c | 74 +++++++++++++++++++++++++++++++++++++++++
 target/ppc/spr_tcg.h    |  1 +
 target/ppc/translate.c  | 23 ++++++++++++-
 7 files changed, 108 insertions(+), 4 deletions(-)
 create mode 100644 target/ppc/power8_pmu.c

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 8dfbb62022..a9b31736af 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1178,6 +1178,12 @@ struct CPUPPCState {
     uint32_t tm_vscr;
     uint64_t tm_dscr;
     uint64_t tm_tar;
+
+    /*
+     * 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 bb5ea04c61..07c79745ba 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -6820,8 +6820,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,
@@ -6869,7 +6869,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 4076aa281e..5122632784 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/meson.build b/target/ppc/meson.build
index b85f295703..278ce07da9 100644
--- a/target/ppc/meson.build
+++ b/target/ppc/meson.build
@@ -14,6 +14,7 @@ ppc_ss.add(when: 'CONFIG_TCG', if_true: files(
   'int_helper.c',
   'mem_helper.c',
   'misc_helper.c',
+  'power8_pmu.c',
   'timebase_helper.c',
   'translate.c',
 ))
diff --git a/target/ppc/power8_pmu.c b/target/ppc/power8_pmu.c
new file mode 100644
index 0000000000..47de38a99e
--- /dev/null
+++ b/target/ppc/power8_pmu.c
@@ -0,0 +1,74 @@
+/*
+ * 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 "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"
+
+#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
+
+static void update_PMC_PM_CYC(CPUPPCState *env, int sprn,
+                              uint64_t time_delta)
+{
+    /*
+     * The pseries and pvn clock runs at 1Ghz, meaning that
+     * 1 nanosec equals 1 cycle.
+     */
+    env->spr[sprn] += time_delta;
+}
+
+static void update_cycles_PMCs(CPUPPCState *env)
+{
+    uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+    uint64_t time_delta = now - env->pmu_base_time;
+
+    update_PMC_PM_CYC(env, SPR_POWER_PMC6, time_delta);
+}
+
+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;
+
+    env->spr[SPR_POWER_MMCR0] = value;
+
+    /* MMCR0 writes can change HFLAGS_PMCCCLEAR */
+    if ((curr_value & MMCR0_PMCC) != (value & MMCR0_PMCC)) {
+        hreg_compute_hflags(env);
+    }
+
+    /*
+     * In an frozen count (FC) bit change:
+     *
+     * - if PMCs were running (curr_FC = false) and we're freezing
+     * them (new_FC = true), save the PMCs values in the registers.
+     *
+     * - if PMCs were frozen (curr_FC = true) and we're activating
+     * them (new_FC = false), set the new base_time for future cycle
+     * calculations.
+     */
+    if (curr_FC != new_FC) {
+        if (!curr_FC) {
+            update_cycles_PMCs(env);
+        } else {
+            env->pmu_base_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+        }
+    }
+}
+
+#endif /* defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY) */
diff --git a/target/ppc/spr_tcg.h b/target/ppc/spr_tcg.h
index 094466a2b2..51fbc081de 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);
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 0babde3131..c3e2e3d329 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -401,6 +401,24 @@ void spr_write_generic(DisasContext *ctx, int sprn, int gprn)
     spr_store_dump_spr(sprn);
 }
 
+#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
+void spr_write_MMCR0(DisasContext *ctx, int sprn, int gprn)
+{
+    /*
+     * 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, cpu_gpr[gprn]);
+}
+#else
+void spr_write_MMCR0(DisasContext *ctx, int sprn, int gprn)
+{
+    spr_write_generic(ctx, sprn, gprn);
+}
+#endif
+
 #if !defined(CONFIG_USER_ONLY)
 void spr_write_generic32(DisasContext *ctx, int sprn, int gprn)
 {
@@ -596,7 +614,10 @@ void spr_write_MMCR0_ureg(DisasContext *ctx, int sprn, int gprn)
     tcg_gen_andi_tl(t1, t1, ~(MMCR0_UREG_MASK));
     /* Keep all other bits intact */
     tcg_gen_or_tl(t1, t1, t0);
-    gen_store_spr(SPR_POWER_MMCR0, t1);
+
+    /* Overwrite cpu_gpr[gprn] and use spr_write_MMCR0() */
+    tcg_gen_mov_tl(cpu_gpr[gprn], t1);
+    spr_write_MMCR0(ctx, sprn + 0x10, gprn);
 
     tcg_temp_free(t0);
     tcg_temp_free(t1);
-- 
2.31.1

[PATCH v3 04/15] target/ppc/power8_pmu.c: enable PMC1-PMC4 events

[Daniel Henrique Barboza]

This patch enable all PMCs but PMC5 to count cycles. To do that we
need to implement MMCR1 bits where the event are stored, retrieve
them, see if the PMC was configured with a PM_CYC event, and
calculate cycles if that's the case.

PowerISA v3.1 defines the following conditions to count cycles:

- PMC1 set with the event 0xF0;
- PMC6, which always count cycles

However, the PowerISA also defines a range of 'implementation dependent'
events that the chip can use in the 0x01-0xBF range. Turns out that IBM
POWER chips implements some non-ISA events, and the Linux kernel makes uses
of them. For instance, 0x1E is an implementation specific event that
counts cycles in PMCs 1-4 that the kernel uses. Let's also support 0x1E
to count cycles to allow for existing kernels to behave properly with the
PMU.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 target/ppc/cpu.h        | 11 +++++++++
 target/ppc/power8_pmu.c | 52 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 63 insertions(+)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index a9b31736af..74698a3600 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -352,6 +352,17 @@ typedef struct ppc_v3_pate_t {
 /* MMCR0 userspace r/w mask */
 #define MMCR0_UREG_MASK (MMCR0_FC | MMCR0_PMAO | MMCR0_PMAE)
 
+#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)
diff --git a/target/ppc/power8_pmu.c b/target/ppc/power8_pmu.c
index 47de38a99e..3f7b305f4f 100644
--- a/target/ppc/power8_pmu.c
+++ b/target/ppc/power8_pmu.c
@@ -31,10 +31,62 @@ static void update_PMC_PM_CYC(CPUPPCState *env, int sprn,
     env->spr[sprn] += time_delta;
 }
 
+static void update_programmable_PMC_reg(CPUPPCState *env, int sprn,
+                                        uint64_t time_delta)
+{
+    uint8_t event, evt_extr;
+
+    switch (sprn) {
+    case SPR_POWER_PMC1:
+        evt_extr = MMCR1_PMC1EVT_EXTR;
+        break;
+    case SPR_POWER_PMC2:
+        evt_extr = MMCR1_PMC2EVT_EXTR;
+        break;
+    case SPR_POWER_PMC3:
+        evt_extr = MMCR1_PMC3EVT_EXTR;
+        break;
+    case SPR_POWER_PMC4:
+        evt_extr = MMCR1_PMC4EVT_EXTR;
+        break;
+    default:
+        return;
+    }
+
+    event = extract64(env->spr[SPR_POWER_MMCR1], evt_extr, MMCR1_EVT_SIZE);
+
+    /*
+     * MMCR0_PMC1SEL = 0xF0 is the architected PowerISA v3.1 event
+     * that counts cycles using PMC1.
+     *
+     * IBM POWER chips also has support for an implementation dependent
+     * event, 0x1E, that enables cycle counting on PMCs 1-4. The
+     * Linux kernel makes extensive use of 0x1E, so let's also support
+     * it.
+     */
+    switch (event) {
+    case 0xF0:
+        if (sprn == SPR_POWER_PMC1) {
+            update_PMC_PM_CYC(env, sprn, time_delta);
+        }
+        break;
+    case 0x1E:
+        update_PMC_PM_CYC(env, sprn, time_delta);
+        break;
+    default:
+        return;
+    }
+}
+
 static void update_cycles_PMCs(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_PMC5; sprn++) {
+        update_programmable_PMC_reg(env, sprn, time_delta);
+    }
 
     update_PMC_PM_CYC(env, SPR_POWER_PMC6, time_delta);
 }
-- 
2.31.1

[PATCH v3 05/15] target/ppc: PMU: add instruction counting

[Daniel Henrique Barboza]

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 later on.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 target/ppc/cpu.h         |  1 +
 target/ppc/helper.h      |  1 +
 target/ppc/helper_regs.c |  3 ++
 target/ppc/power8_pmu.c  | 70 ++++++++++++++++++++++++++++++++++++----
 target/ppc/translate.c   | 46 ++++++++++++++++++++++++++
 5 files changed, 114 insertions(+), 7 deletions(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 74698a3600..4d4886ac74 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -628,6 +628,7 @@ enum {
     HFLAGS_FP = 13,  /* MSR_FP */
     HFLAGS_PR = 14,  /* MSR_PR */
     HFLAGS_PMCCCLEAR = 15, /* PMU MMCR0 PMCC equal to 0b00 */
+    HFLAGS_MMCR0FC = 16, /* 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 5122632784..47dbbe6da1 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(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 4c1d9575ac..27d139edd8 100644
--- a/target/ppc/helper_regs.c
+++ b/target/ppc/helper_regs.c
@@ -109,6 +109,9 @@ static uint32_t hreg_compute_hflags_value(CPUPPCState *env)
     if (((env->spr[SPR_POWER_MMCR0] & MMCR0_PMCC) >> 18) == 0) {
         hflags |= 1 << HFLAGS_PMCCCLEAR;
     }
+    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.c b/target/ppc/power8_pmu.c
index 3f7b305f4f..9769c0ff35 100644
--- a/target/ppc/power8_pmu.c
+++ b/target/ppc/power8_pmu.c
@@ -31,10 +31,13 @@ static void update_PMC_PM_CYC(CPUPPCState *env, int sprn,
     env->spr[sprn] += time_delta;
 }
 
-static void update_programmable_PMC_reg(CPUPPCState *env, int sprn,
-                                        uint64_t time_delta)
+static uint8_t get_PMC_event(CPUPPCState *env, int sprn)
 {
-    uint8_t event, evt_extr;
+    uint8_t evt_extr = 0;
+
+    if (env->spr[SPR_POWER_MMCR1] == 0) {
+        return 0;
+    }
 
     switch (sprn) {
     case SPR_POWER_PMC1:
@@ -50,10 +53,16 @@ static void update_programmable_PMC_reg(CPUPPCState *env, int sprn,
         evt_extr = MMCR1_PMC4EVT_EXTR;
         break;
     default:
-        return;
+        return 0;
     }
 
-    event = extract64(env->spr[SPR_POWER_MMCR1], evt_extr, MMCR1_EVT_SIZE);
+    return extract64(env->spr[SPR_POWER_MMCR1], evt_extr, MMCR1_EVT_SIZE);
+}
+
+static void update_programmable_PMC_reg(CPUPPCState *env, int sprn,
+                                        uint64_t time_delta)
+{
+    uint8_t event = get_PMC_event(env, sprn);
 
     /*
      * MMCR0_PMC1SEL = 0xF0 is the architected PowerISA v3.1 event
@@ -99,8 +108,9 @@ void helper_store_mmcr0(CPUPPCState *env, target_ulong value)
 
     env->spr[SPR_POWER_MMCR0] = value;
 
-    /* MMCR0 writes can change HFLAGS_PMCCCLEAR */
-    if ((curr_value & MMCR0_PMCC) != (value & MMCR0_PMCC)) {
+    /* MMCR0 writes can change HFLAGS_PMCCCLEAR and HFLAGS_MMCR0FC */
+    if (((curr_value & MMCR0_PMCC) != (value & MMCR0_PMCC)) ||
+        (curr_FC != new_FC)) {
         hreg_compute_hflags(env);
     }
 
@@ -123,4 +133,50 @@ void helper_store_mmcr0(CPUPPCState *env, target_ulong value)
     }
 }
 
+static bool pmc_counting_insns(CPUPPCState *env, int sprn)
+{
+    bool ret = false;
+    uint8_t event;
+
+    if (sprn == SPR_POWER_PMC5) {
+        return true;
+    }
+
+    event = get_PMC_event(env, sprn);
+
+    /*
+     * 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
+     * all programmable PMCs.
+     *
+     * Event 0xFE is the PowerISA v3.1 architected event to
+     * sample PM_INST_CMPL using PMC1.
+     */
+    switch (sprn) {
+    case SPR_POWER_PMC1:
+        return event == 0x2 || event == 0xFE;
+    case SPR_POWER_PMC2:
+    case SPR_POWER_PMC3:
+    case SPR_POWER_PMC4:
+        return event == 0x2;
+    default:
+        break;
+    }
+
+    return ret;
+}
+
+/* This helper assumes that the PMC is running. */
+void helper_insns_inc(CPUPPCState *env, uint32_t num_insns)
+{
+    int sprn;
+
+    for (sprn = SPR_POWER_PMC1; sprn <= SPR_POWER_PMC5; sprn++) {
+        if (pmc_counting_insns(env, sprn)) {
+            env->spr[sprn] += num_insns;
+        }
+    }
+}
+
 #endif /* defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY) */
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index c3e2e3d329..b7235a2be0 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -176,6 +176,7 @@ struct DisasContext {
     bool tm_enabled;
     bool gtse;
     bool pmcc_clear;
+    bool pmu_frozen;
     ppc_spr_t *spr_cb; /* Needed to check rights for mfspr/mtspr */
     int singlestep_enabled;
     uint32_t flags;
@@ -411,6 +412,12 @@ void spr_write_MMCR0(DisasContext *ctx, int sprn, int gprn)
      */
     gen_icount_io_start(ctx);
     gen_helper_store_mmcr0(cpu_env, cpu_gpr[gprn]);
+
+    /*
+     * End the translation block because MMCR0 writes can change
+     * ctx->pmu_frozen.
+     */
+    ctx->base.is_jmp = DISAS_EXIT_UPDATE;
 }
 #else
 void spr_write_MMCR0(DisasContext *ctx, int sprn, int gprn)
@@ -4407,6 +4414,22 @@ 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;
+    }
+
+    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);
@@ -4421,9 +4444,17 @@ static void gen_lookup_and_goto_ptr(DisasContext *ctx)
         } else if (sse & (CPU_SINGLE_STEP | CPU_BRANCH_STEP)) {
             gen_helper_raise_exception(cpu_env, tcg_constant_i32(gen_prep_dbgex(ctx)));
         } else {
+            pmu_count_insns(ctx);
             tcg_gen_exit_tb(NULL, 0);
         }
     } 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();
     }
 }
@@ -4435,6 +4466,8 @@ 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);
@@ -8648,6 +8681,7 @@ static void ppc_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
     ctx->tm_enabled = (hflags >> HFLAGS_TM) & 1;
     ctx->gtse = (hflags >> HFLAGS_GTSE) & 1;
     ctx->pmcc_clear = (hflags >> HFLAGS_PMCCCLEAR) & 1;
+    ctx->pmu_frozen = (hflags >> HFLAGS_MMCR0FC) & 1;
 
     ctx->singlestep_enabled = 0;
     if ((hflags >> HFLAGS_SE) & 1) {
@@ -8767,6 +8801,8 @@ 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);
@@ -8777,6 +8813,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;
 
@@ -8784,6 +8828,8 @@ 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

[PATCH v3 06/15] target/ppc/power8_pmu.c: add PM_RUN_INST_CMPL (0xFA) event

[Daniel Henrique Barboza]

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.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 target/ppc/cpu.h        |  3 +++
 target/ppc/cpu_init.c   |  2 +-
 target/ppc/power8_pmu.c | 27 ++++++++++++++++++++-------
 target/ppc/spr_tcg.h    |  1 +
 target/ppc/translate.c  | 12 ++++++++++++
 5 files changed, 37 insertions(+), 8 deletions(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 4d4886ac74..76b462c3c8 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -363,6 +363,9 @@ typedef struct ppc_v3_pate_t {
 #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 07c79745ba..0013cba5ff 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -6748,7 +6748,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 9769c0ff35..f584480fde 100644
--- a/target/ppc/power8_pmu.c
+++ b/target/ppc/power8_pmu.c
@@ -133,17 +133,15 @@ void helper_store_mmcr0(CPUPPCState *env, target_ulong value)
     }
 }
 
-static bool pmc_counting_insns(CPUPPCState *env, int sprn)
+static bool pmc_counting_insns(CPUPPCState *env, int sprn,
+                               uint8_t event)
 {
     bool ret = false;
-    uint8_t event;
 
     if (sprn == SPR_POWER_PMC5) {
         return true;
     }
 
-    event = get_PMC_event(env, sprn);
-
     /*
      * Event 0x2 is an implementation-dependent event that IBM
      * POWER chips implement (at least since POWER8) that is
@@ -158,8 +156,15 @@ static bool pmc_counting_insns(CPUPPCState *env, int sprn)
         return event == 0x2 || event == 0xFE;
     case SPR_POWER_PMC2:
     case SPR_POWER_PMC3:
-    case SPR_POWER_PMC4:
         return event == 0x2;
+    case SPR_POWER_PMC4:
+        /*
+         * Event 0xFA is the "instructions completed with run latch
+         * set" event. Consider it as instruction counting event.
+         * The caller is responsible for handling it separately
+         * from PM_INST_CMPL.
+         */
+        return event == 0x2 || event == 0xFA;
     default:
         break;
     }
@@ -173,8 +178,16 @@ void helper_insns_inc(CPUPPCState *env, uint32_t num_insns)
     int sprn;
 
     for (sprn = SPR_POWER_PMC1; sprn <= SPR_POWER_PMC5; sprn++) {
-        if (pmc_counting_insns(env, sprn)) {
-            env->spr[sprn] += num_insns;
+        uint8_t event = get_PMC_event(env, sprn);
+
+        if (pmc_counting_insns(env, sprn, event)) {
+            if (sprn == SPR_POWER_PMC4 && event == 0xFA) {
+                if (env->spr[SPR_CTRL] & CTRL_RUN) {
+                    env->spr[SPR_POWER_PMC4] += num_insns;
+                }
+            } else {
+                env->spr[sprn] += num_insns;
+            }
         }
     }
 }
diff --git a/target/ppc/spr_tcg.h b/target/ppc/spr_tcg.h
index 51fbc081de..5e6ed36eb1 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_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 b7235a2be0..866b1d2b34 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -402,6 +402,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);
+
+    /*
+     * Write in SPR_CTRL 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(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
 void spr_write_MMCR0(DisasContext *ctx, int sprn, int gprn)
 {
-- 
2.31.1

[PATCH v3 07/15] target/ppc/power8_pmu.c: add PMC14/PMC56 counter freeze bits

[Daniel Henrique Barboza]

We're missing two counter freeze bits that are used to further control
how the PMCs behaves: MMCR0_FC14 and MMCR0_FC56. These bits can frozen
PMCs separately: MMCR0_FC14 freezes PMCs 1 to 4 and MMCR0_FC56 freezes
PMCs 5 and 6.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 target/ppc/cpu.h        |  2 ++
 target/ppc/power8_pmu.c | 25 ++++++++++++++++++++++---
 2 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 76b462c3c8..93f4a46827 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -351,6 +351,8 @@ typedef struct ppc_v3_pate_t {
 #define MMCR0_PMCC  PPC_BITMASK(44, 45) /* PMC Control */
 /* MMCR0 userspace r/w mask */
 #define MMCR0_UREG_MASK (MMCR0_FC | MMCR0_PMAO | MMCR0_PMAE)
+#define MMCR0_FC14 PPC_BIT(58) /* MMCR0 Freeze Counters 1-4 bit */
+#define MMCR0_FC56 PPC_BIT(59) /* MMCR0 Freeze Counters 5-6 bit */
 
 #define MMCR1_EVT_SIZE 8
 /* extract64() does a right shift before extracting */
diff --git a/target/ppc/power8_pmu.c b/target/ppc/power8_pmu.c
index f584480fde..dd58f57f52 100644
--- a/target/ppc/power8_pmu.c
+++ b/target/ppc/power8_pmu.c
@@ -59,6 +59,15 @@ static uint8_t get_PMC_event(CPUPPCState *env, int sprn)
     return extract64(env->spr[SPR_POWER_MMCR1], evt_extr, MMCR1_EVT_SIZE);
 }
 
+static bool pmc_is_running(CPUPPCState *env, int sprn)
+{
+    if (sprn < SPR_POWER_PMC5) {
+        return !(env->spr[SPR_POWER_MMCR0] & MMCR0_FC14);
+    }
+
+    return !(env->spr[SPR_POWER_MMCR0] & MMCR0_FC56);
+}
+
 static void update_programmable_PMC_reg(CPUPPCState *env, int sprn,
                                         uint64_t time_delta)
 {
@@ -91,13 +100,19 @@ static void update_cycles_PMCs(CPUPPCState *env)
 {
     uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
     uint64_t time_delta = now - env->pmu_base_time;
+    bool PMC14_running = !(env->spr[SPR_POWER_MMCR0] & MMCR0_FC14);
+    bool PMC6_running = !(env->spr[SPR_POWER_MMCR0] & MMCR0_FC56);
     int sprn;
 
-    for (sprn = SPR_POWER_PMC1; sprn < SPR_POWER_PMC5; sprn++) {
-        update_programmable_PMC_reg(env, sprn, time_delta);
+    if (PMC14_running) {
+        for (sprn = SPR_POWER_PMC1; sprn < SPR_POWER_PMC5; sprn++) {
+            update_programmable_PMC_reg(env, sprn, time_delta);
+        }
     }
 
-    update_PMC_PM_CYC(env, SPR_POWER_PMC6, time_delta);
+    if (PMC6_running) {
+        update_PMC_PM_CYC(env, SPR_POWER_PMC6, time_delta);
+    }
 }
 
 void helper_store_mmcr0(CPUPPCState *env, target_ulong value)
@@ -138,6 +153,10 @@ static bool pmc_counting_insns(CPUPPCState *env, int sprn,
 {
     bool ret = false;
 
+    if (!pmc_is_running(env, sprn)) {
+        return false;
+    }
+
     if (sprn == SPR_POWER_PMC5) {
         return true;
     }
-- 
2.31.1

[PATCH v3 08/15] PPC64/TCG: Implement 'rfebb' instruction

[Daniel Henrique Barboza]

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

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 93f4a46827..26624508fa 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -368,6 +368,19 @@ typedef struct ppc_v3_pate_t {
 /* 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 7b6ac16eef..22f9835383 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -1281,6 +1281,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 47dbbe6da1..91a86992a5 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 9fd8d6b817..deb7374ea4 100644
--- a/target/ppc/insn32.decode
+++ b/target/ppc/insn32.decode
@@ -124,3 +124,8 @@ SETNBCR         011111 ..... ..... ----- 0111100000 -   @X_bi
 ## Vector Bit Manipulation Instruction
 
 VCFUGED         000100 ..... ..... ..... 10101001101    @VX
+
+### 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 866b1d2b34..7a3104ecf9 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -7636,6 +7636,8 @@ static int times_4(DisasContext *ctx, int x)
 
 #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..9c991d9abb
--- /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_RFEBB *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_RFEBB *arg)
+{
+    gen_invalid(ctx);
+    return true;
+}
+#endif
-- 
2.31.1

[PATCH v3 09/15] target/ppc: PMU Event-Based exception support

[Daniel Henrique Barboza]

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).

This patch sets the basic structure of interrupts and timers. The
following patches will add the counter negative logic for the registers.

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>
---
 hw/ppc/spapr_cpu_core.c  |  6 ++++++
 target/ppc/cpu.h         | 12 +++++++++++-
 target/ppc/excp_helper.c | 28 +++++++++++++++++++++++++++
 target/ppc/power8_pmu.c  | 41 ++++++++++++++++++++++++++++++++++++++++
 target/ppc/power8_pmu.h  | 25 ++++++++++++++++++++++++
 5 files changed, 111 insertions(+), 1 deletion(-)
 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 4f316a6f9d..c7a342c4aa 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"
@@ -266,6 +267,11 @@ static bool spapr_realize_vcpu(PowerPCCPU *cpu, SpaprMachineState *spapr,
         return false;
     }
 
+    /* Init PMU interrupt timer (TCG only) */
+    if (!kvm_enabled()) {
+        cpu_ppc_pmu_timer_init(env);
+    }
+
     if (!sc->pre_3_0_migration) {
         vmstate_register(NULL, cs->cpu_index, &vmstate_spapr_cpu_state,
                          cpu->machine_data);
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 26624508fa..780eab6f92 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -129,8 +129,9 @@ 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      */
 };
@@ -1053,6 +1054,8 @@ struct ppc_radix_page_info {
 #define PPC_CPU_OPCODES_LEN          0x40
 #define PPC_CPU_INDIRECT_OPCODES_LEN 0x20
 
+#define PMU_TIMERS_LEN 5
+
 struct CPUPPCState {
     /* Most commonly used resources during translated code execution first */
     target_ulong gpr[32];  /* general purpose registers */
@@ -1214,6 +1217,12 @@ struct CPUPPCState {
      * running cycles.
      */
     uint64_t pmu_base_time;
+
+    /*
+     * Timers used to fire performance monitor alerts and
+     * interrupts. All PMCs but PMC5 has a timer.
+     */
+    QEMUTimer *pmu_intr_timers[PMU_TIMERS_LEN];
 };
 
 #define SET_FIT_PERIOD(a_, b_, c_, d_)          \
@@ -2430,6 +2439,7 @@ enum {
     PPC_INTERRUPT_HMI,            /* Hypervisor Maintenance interrupt    */
     PPC_INTERRUPT_HDOORBELL,      /* Hypervisor Doorbell interrupt        */
     PPC_INTERRUPT_HVIRT,          /* Hypervisor virtualization interrupt  */
+    PPC_INTERRUPT_PMC,            /* Performance Monitor Counter interrupt */
 };
 
 /* Processor Compatibility mask (PCR) */
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 22f9835383..d20447f171 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -821,6 +821,22 @@ 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_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);
@@ -1068,6 +1084,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 dd58f57f52..aa5df6f51f 100644
--- a/target/ppc/power8_pmu.c
+++ b/target/ppc/power8_pmu.c
@@ -12,12 +12,14 @@
 
 #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)
 
@@ -115,6 +117,45 @@ static void update_cycles_PMCs(CPUPPCState *env)
     }
 }
 
+static void cpu_ppc_pmu_timer_cb(void *opaque)
+{
+    PowerPCCPU *cpu = opaque;
+    CPUPPCState *env = &cpu->env;
+    uint64_t mmcr0;
+
+    mmcr0 = env->spr[SPR_POWER_MMCR0];
+    if (env->spr[SPR_POWER_MMCR0] & MMCR0_EBE) {
+        /* freeeze counters if needed */
+        if (mmcr0 & MMCR0_FCECE) {
+            mmcr0 &= ~MMCR0_FCECE;
+            mmcr0 |= MMCR0_FC;
+        }
+
+        /* Clear PMAE and set PMAO */
+        if (mmcr0 & MMCR0_PMAE) {
+            mmcr0 &= ~MMCR0_PMAE;
+            mmcr0 |= MMCR0_PMAO;
+        }
+        env->spr[SPR_POWER_MMCR0] = mmcr0;
+
+        /* Fire the PMC hardware exception */
+        ppc_set_irq(cpu, PPC_INTERRUPT_PMC, 1);
+    }
+}
+
+void cpu_ppc_pmu_timer_init(CPUPPCState *env)
+{
+    PowerPCCPU *cpu = env_archcpu(env);
+    QEMUTimer *timer;
+    int i;
+
+    for (i = 0; i < PMU_TIMERS_LEN; i++) {
+        timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, &cpu_ppc_pmu_timer_cb,
+                             cpu);
+        env->pmu_intr_timers[i] = timer;
+    }
+}
+
 void helper_store_mmcr0(CPUPPCState *env, target_ulong value)
 {
     target_ulong curr_value = env->spr[SPR_POWER_MMCR0];
diff --git a/target/ppc/power8_pmu.h b/target/ppc/power8_pmu.h
new file mode 100644
index 0000000000..34a9d0e8a2
--- /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 PMU_BOOK3S_HELPER
+#define PMU_BOOK3S_HELPER
+
+#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_timer_init(CPUPPCState *env);
+
+#endif
-- 
2.31.1

[PATCH v3 10/15] target/ppc/excp_helper.c: EBB handling adjustments

[Daniel Henrique Barboza]

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 d20447f171..6d16c986af 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -822,14 +822,47 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
                   "is not implemented yet !\n");
         break;
     case POWERPC_EXCP_EBB:       /* Event-based branch exception             */
-        if ((env->spr[SPR_BESCR] & BESCR_GE) &&
-            (env->spr[SPR_BESCR] & BESCR_PME)) {
+        if (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

[PATCH v3 11/15] target/ppc/power8_pmu.c: enable PMC1 counter negative overflow

[Daniel Henrique Barboza]

This patch starts the counter negative EBB support by enabling PMC1
counter negative overflow when PMC1 is counting cycles.

A counter negative overflow happens when a performance monitor counter
reaches the value 0x80000000. When that happens, if counter negative
condition events are enabled in that counter, a performance monitor
alert is triggered. For PMC1, this condition is enabled by MMCR0_PMC1CE.

Cycle counting is done by calculating elapsed time between the time
the PMU started to run and when the PMU is shut down. Our clock is
fixed in 1Ghz, so 1 cycle equals 1 nanoseconds. The same idea is used to
predict a counter negative overflow: calculate the amount of nanoseconds
for the timer to reach 0x80000000, start a timer with it and trigger the
performance monitor alert. If event-based exceptions are enabled (bit
MMCR0_EBE), we'll go ahead and fire a PPC_INTERRUPT_PMC.

A new function 'start_cycle_count_session' was added to encapsulate the
most common steps of cycle calculation: redefine base time and start
overflow timers. This will avoid code repetition in the next patches.

Counter overflow for the remaining counters will be added shortly.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 target/ppc/cpu.h        |  1 +
 target/ppc/power8_pmu.c | 99 +++++++++++++++++++++++++++++++++--------
 2 files changed, 82 insertions(+), 18 deletions(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 780eab6f92..ba93b30ae2 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -354,6 +354,7 @@ typedef struct ppc_v3_pate_t {
 #define MMCR0_UREG_MASK (MMCR0_FC | MMCR0_PMAO | MMCR0_PMAE)
 #define MMCR0_FC14 PPC_BIT(58) /* MMCR0 Freeze Counters 1-4 bit */
 #define MMCR0_FC56 PPC_BIT(59) /* MMCR0 Freeze Counters 5-6 bit */
+#define MMCR0_PMC1CE PPC_BIT(48) /* MMCR0 PMC1 Condition Enabled */
 
 #define MMCR1_EVT_SIZE 8
 /* extract64() does a right shift before extracting */
diff --git a/target/ppc/power8_pmu.c b/target/ppc/power8_pmu.c
index aa5df6f51f..b2224d363a 100644
--- a/target/ppc/power8_pmu.c
+++ b/target/ppc/power8_pmu.c
@@ -33,6 +33,8 @@ static void update_PMC_PM_CYC(CPUPPCState *env, int sprn,
     env->spr[sprn] += time_delta;
 }
 
+#define COUNTER_NEGATIVE_VAL 0x80000000
+
 static uint8_t get_PMC_event(CPUPPCState *env, int sprn)
 {
     uint8_t evt_extr = 0;
@@ -117,30 +119,91 @@ static void update_cycles_PMCs(CPUPPCState *env)
     }
 }
 
+static int64_t get_CYC_timeout(CPUPPCState *env, int sprn)
+{
+    int64_t remaining_cyc;
+
+    if (env->spr[sprn] >= COUNTER_NEGATIVE_VAL) {
+        return 0;
+    }
+
+    remaining_cyc = COUNTER_NEGATIVE_VAL - env->spr[sprn];
+    return remaining_cyc;
+}
+
+static bool counter_negative_cond_enabled(uint64_t mmcr0)
+{
+    return mmcr0 & MMCR0_PMC1CE;
+}
+
+/*
+ * A cycle count session consists of the basic operations we
+ * need to do to support PM_CYC events: redefine a new base_time
+ * to be used to calculate PMC values and start overflow timers.
+ */
+static void start_cycle_count_session(CPUPPCState *env)
+{
+    uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+    uint64_t timeout;
+
+    env->pmu_base_time = now;
+
+    /*
+     * Always delete existing overflow timers when starting a
+     * new cycle counting session.
+     */
+    timer_del(env->pmu_intr_timers[0]);
+
+    if (!counter_negative_cond_enabled(env->spr[SPR_POWER_MMCR0])) {
+        return;
+    }
+
+    if (!pmc_is_running(env, SPR_POWER_PMC1)) {
+        return;
+    }
+
+    if (!(env->spr[SPR_POWER_MMCR0] & MMCR0_PMC1CE)) {
+        return;
+    }
+
+    switch (get_PMC_event(env, SPR_POWER_PMC1)) {
+    case 0xF0:
+    case 0x1E:
+        timeout = get_CYC_timeout(env, SPR_POWER_PMC1);
+        break;
+    default:
+        return;
+    }
+
+    timer_mod(env->pmu_intr_timers[0], now + timeout);
+}
+
 static void cpu_ppc_pmu_timer_cb(void *opaque)
 {
     PowerPCCPU *cpu = opaque;
     CPUPPCState *env = &cpu->env;
-    uint64_t mmcr0;
-
-    mmcr0 = env->spr[SPR_POWER_MMCR0];
-    if (env->spr[SPR_POWER_MMCR0] & MMCR0_EBE) {
-        /* freeeze counters if needed */
-        if (mmcr0 & MMCR0_FCECE) {
-            mmcr0 &= ~MMCR0_FCECE;
-            mmcr0 |= MMCR0_FC;
-        }
 
-        /* Clear PMAE and set PMAO */
-        if (mmcr0 & MMCR0_PMAE) {
-            mmcr0 &= ~MMCR0_PMAE;
-            mmcr0 |= MMCR0_PMAO;
-        }
-        env->spr[SPR_POWER_MMCR0] = mmcr0;
+    if (!(env->spr[SPR_POWER_MMCR0] & MMCR0_EBE)) {
+        return;
+    }
 
-        /* Fire the PMC hardware exception */
-        ppc_set_irq(cpu, PPC_INTERRUPT_PMC, 1);
+    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);
+    }
+
+    update_cycles_PMCs(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);
 }
 
 void cpu_ppc_pmu_timer_init(CPUPPCState *env)
@@ -184,7 +247,7 @@ void helper_store_mmcr0(CPUPPCState *env, target_ulong value)
         if (!curr_FC) {
             update_cycles_PMCs(env);
         } else {
-            env->pmu_base_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+            start_cycle_count_session(env);
         }
     }
 }
-- 
2.31.1

[PATCH v3 12/15] target/ppc/power8_pmu.c: cycles overflow with all PMCs

[Daniel Henrique Barboza]

All performance monitor counters can trigger a counter negative
condition if the proper MMCR0 bits are set. This patch does that
for all PMCs that can count cycles by doing the following:

- pmc_counter_negative_enabled() will check whether a given PMC is
eligible to trigger the counter negative alert;

- get_counter_neg_timeout() will return the timeout for the counter
negative condition for a given PMC, or -1 if the PMC is not able to
trigger this alert;

- the existing counter_negative_cond_enabled() now must consider the
counter negative bit for PMCs 2-6, MMCR0_PMCjCE;

- start_cycle_count_session() will start overflow timers for all eligible
PMCs.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 target/ppc/cpu.h        |   1 +
 target/ppc/power8_pmu.c | 116 ++++++++++++++++++++++++++++++++++------
 2 files changed, 100 insertions(+), 17 deletions(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index ba93b30ae2..02177e584e 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -355,6 +355,7 @@ typedef struct ppc_v3_pate_t {
 #define MMCR0_FC14 PPC_BIT(58) /* MMCR0 Freeze Counters 1-4 bit */
 #define MMCR0_FC56 PPC_BIT(59) /* MMCR0 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 */
 
 #define MMCR1_EVT_SIZE 8
 /* extract64() does a right shift before extracting */
diff --git a/target/ppc/power8_pmu.c b/target/ppc/power8_pmu.c
index b2224d363a..9125ba29ae 100644
--- a/target/ppc/power8_pmu.c
+++ b/target/ppc/power8_pmu.c
@@ -131,9 +131,81 @@ static int64_t get_CYC_timeout(CPUPPCState *env, int sprn)
     return remaining_cyc;
 }
 
+static bool pmc_counter_negative_enabled(CPUPPCState *env, int sprn)
+{
+    if (!pmc_is_running(env, sprn)) {
+        return false;
+    }
+
+    switch (sprn) {
+    case SPR_POWER_PMC1:
+        return env->spr[SPR_POWER_MMCR0] & MMCR0_PMC1CE;
+
+    case SPR_POWER_PMC2:
+    case SPR_POWER_PMC3:
+    case SPR_POWER_PMC4:
+    case SPR_POWER_PMC5:
+    case SPR_POWER_PMC6:
+        return env->spr[SPR_POWER_MMCR0] & MMCR0_PMCjCE;
+
+    default:
+        break;
+    }
+
+    return false;
+}
+
+static int64_t get_counter_neg_timeout(CPUPPCState *env, int sprn)
+{
+    int64_t timeout = -1;
+
+    if (!pmc_counter_negative_enabled(env, sprn)) {
+        return -1;
+    }
+
+    if (env->spr[sprn] >= COUNTER_NEGATIVE_VAL) {
+        return 0;
+    }
+
+    switch (sprn) {
+    case SPR_POWER_PMC1:
+    case SPR_POWER_PMC2:
+    case SPR_POWER_PMC3:
+    case SPR_POWER_PMC4:
+        switch (get_PMC_event(env, sprn)) {
+        case 0xF0:
+            if (sprn == SPR_POWER_PMC1) {
+                timeout = get_CYC_timeout(env, sprn);
+            }
+            break;
+        case 0x1E:
+            timeout = get_CYC_timeout(env, sprn);
+            break;
+        }
+
+        break;
+    case SPR_POWER_PMC6:
+        timeout = get_CYC_timeout(env, sprn);
+        break;
+    default:
+        break;
+    }
+
+    return timeout;
+}
+
 static bool counter_negative_cond_enabled(uint64_t mmcr0)
 {
-    return mmcr0 & MMCR0_PMC1CE;
+    return mmcr0 & (MMCR0_PMC1CE | MMCR0_PMCjCE);
+}
+
+static void pmu_delete_timers(CPUPPCState *env)
+{
+    int i;
+
+    for (i = 0; i < PMU_TIMERS_LEN; i++) {
+        timer_del(env->pmu_intr_timers[i]);
+    }
 }
 
 /*
@@ -144,7 +216,8 @@ static bool counter_negative_cond_enabled(uint64_t mmcr0)
 static void start_cycle_count_session(CPUPPCState *env)
 {
     uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
-    uint64_t timeout;
+    int64_t timeout;
+    int i;
 
     env->pmu_base_time = now;
 
@@ -152,30 +225,32 @@ static void start_cycle_count_session(CPUPPCState *env)
      * Always delete existing overflow timers when starting a
      * new cycle counting session.
      */
-    timer_del(env->pmu_intr_timers[0]);
+    pmu_delete_timers(env);
 
     if (!counter_negative_cond_enabled(env->spr[SPR_POWER_MMCR0])) {
         return;
     }
 
-    if (!pmc_is_running(env, SPR_POWER_PMC1)) {
-        return;
-    }
+    /*
+     * Scroll through all programmable PMCs start counter overflow
+     * timers for PM_CYC events, if needed.
+     */
+    for (i = SPR_POWER_PMC1; i < SPR_POWER_PMC5; i++) {
+        timeout = get_counter_neg_timeout(env, i);
 
-    if (!(env->spr[SPR_POWER_MMCR0] & MMCR0_PMC1CE)) {
-        return;
-    }
+        if (timeout == -1) {
+            continue;
+        }
 
-    switch (get_PMC_event(env, SPR_POWER_PMC1)) {
-    case 0xF0:
-    case 0x1E:
-        timeout = get_CYC_timeout(env, SPR_POWER_PMC1);
-        break;
-    default:
-        return;
+        timer_mod(env->pmu_intr_timers[i - SPR_POWER_PMC1],
+                                       now + timeout);
     }
 
-    timer_mod(env->pmu_intr_timers[0], now + timeout);
+    /* Check for counter neg timeout in PMC6 */
+    timeout = get_counter_neg_timeout(env, SPR_POWER_PMC6);
+    if (timeout != -1) {
+        timer_mod(env->pmu_intr_timers[PMU_TIMERS_LEN - 1], now + timeout);
+    }
 }
 
 static void cpu_ppc_pmu_timer_cb(void *opaque)
@@ -193,6 +268,13 @@ static void cpu_ppc_pmu_timer_cb(void *opaque)
 
         /* 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);
     }
 
     update_cycles_PMCs(env);
-- 
2.31.1

[PATCH v3 13/15] target/ppc: PMU: insns counter negative overflow support

[Daniel Henrique Barboza]

Enabling counter negative overflow for the PMCs that are counting
instructions is simpler than when counting cycles. Instruction
counting is done via helper_insns_inc(), which is called every
time a TB ends.

Firing a performance monitor alert due to a counter negative overflow
in this case is a matter of checking if the counter value is over
0x80000000 each time the counters are incremented and, if counter negative
events are enabled for that specific counter, trigger the PM alert.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 target/ppc/power8_pmu.c | 23 +++++++++++++++++++++--
 target/ppc/translate.c  |  8 ++++++++
 2 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/target/ppc/power8_pmu.c b/target/ppc/power8_pmu.c
index 9125ba29ae..edece140aa 100644
--- a/target/ppc/power8_pmu.c
+++ b/target/ppc/power8_pmu.c
@@ -253,9 +253,8 @@ static void start_cycle_count_session(CPUPPCState *env)
     }
 }
 
-static void cpu_ppc_pmu_timer_cb(void *opaque)
+static void fire_PMC_interrupt(PowerPCCPU *cpu)
 {
-    PowerPCCPU *cpu = opaque;
     CPUPPCState *env = &cpu->env;
 
     if (!(env->spr[SPR_POWER_MMCR0] & MMCR0_EBE)) {
@@ -288,6 +287,13 @@ static void cpu_ppc_pmu_timer_cb(void *opaque)
     ppc_set_irq(cpu, PPC_INTERRUPT_PMC, 1);
 }
 
+static void cpu_ppc_pmu_timer_cb(void *opaque)
+{
+    PowerPCCPU *cpu = opaque;
+
+    fire_PMC_interrupt(cpu);
+}
+
 void cpu_ppc_pmu_timer_init(CPUPPCState *env)
 {
     PowerPCCPU *cpu = env_archcpu(env);
@@ -380,6 +386,8 @@ static bool pmc_counting_insns(CPUPPCState *env, int sprn,
 /* This helper assumes that the PMC is running. */
 void helper_insns_inc(CPUPPCState *env, uint32_t num_insns)
 {
+    bool overflow_triggered = false;
+    PowerPCCPU *cpu;
     int sprn;
 
     for (sprn = SPR_POWER_PMC1; sprn <= SPR_POWER_PMC5; sprn++) {
@@ -393,8 +401,19 @@ void helper_insns_inc(CPUPPCState *env, uint32_t num_insns)
             } else {
                 env->spr[sprn] += num_insns;
             }
+
+            if (env->spr[sprn] >= COUNTER_NEGATIVE_VAL &&
+                pmc_counter_negative_enabled(env, sprn)) {
+                overflow_triggered = true;
+                env->spr[sprn] = COUNTER_NEGATIVE_VAL;
+            }
         }
     }
+
+    if (overflow_triggered) {
+        cpu = env_archcpu(env);
+        fire_PMC_interrupt(cpu);
+    }
 }
 
 #endif /* defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY) */
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 7a3104ecf9..c23ae2479c 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -4434,6 +4434,14 @@ static void pmu_count_insns(DisasContext *ctx)
         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
-- 
2.31.1

[PATCH v3 14/15] target/ppc/translate: PMU: handle setting of PMCs while running

[Daniel Henrique Barboza]

The initial PMU support were made under the assumption that the counters
would be set before running the PMU and read after either freezing the
PMU manually or via a performance monitor alert.

Turns out that some EBB powerpc kernel tests set the counters after
unfreezing the counters. Setting a PMC value when the PMU is running
means that, at that moment, the baseline for calculating cycle
events needs to be updated. Updating this baseline means that we need
to update all the PMCs with their actual value at that moment. Any
xisting counter negative timer needs to be discarded an a new one,
with the updated values, must be set again.

This patch does that via a new 'helper_store_pmc()' that is called in
the mtspr() callbacks of PMU counters. With this change, EBB powerpc kernel
tests such as 'no_handler_test' are now passing.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 target/ppc/cpu_init.c   | 24 ++++++++++++------------
 target/ppc/helper.h     |  1 +
 target/ppc/power8_pmu.c | 27 +++++++++++++++++++++++++++
 target/ppc/spr_tcg.h    |  2 ++
 target/ppc/translate.c  | 35 +++++++++++++++++++++++++++++++++++
 5 files changed, 77 insertions(+), 12 deletions(-)

diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
index 0013cba5ff..ee300d5931 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -6832,27 +6832,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_generic, &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_generic, &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_generic, &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_generic, &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_generic, &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_generic, &spr_write_PMC,
                      KVM_REG_PPC_PMC6, 0x00000000);
     spr_register_kvm(env, SPR_POWER_SIAR, "SIAR",
                      SPR_NOACCESS, SPR_NOACCESS,
@@ -6879,27 +6879,27 @@ static void register_book3s_pmu_user_sprs(CPUPPCState *env)
                  &spr_read_ureg, &spr_write_ureg,
                  0x00000000);
     spr_register(env, SPR_POWER_UPMC1, "UPMC1",
-                 &spr_read_ureg, &spr_write_ureg,
+                 &spr_read_ureg, &spr_write_PMC_ureg,
                  &spr_read_ureg, &spr_write_ureg,
                  0x00000000);
     spr_register(env, SPR_POWER_UPMC2, "UPMC2",
-                 &spr_read_ureg, &spr_write_ureg,
+                 &spr_read_ureg, &spr_write_PMC_ureg,
                  &spr_read_ureg, &spr_write_ureg,
                  0x00000000);
     spr_register(env, SPR_POWER_UPMC3, "UPMC3",
-                 &spr_read_ureg, &spr_write_ureg,
+                 &spr_read_ureg, &spr_write_PMC_ureg,
                  &spr_read_ureg, &spr_write_ureg,
                  0x00000000);
     spr_register(env, SPR_POWER_UPMC4, "UPMC4",
-                 &spr_read_ureg, &spr_write_ureg,
+                 &spr_read_ureg, &spr_write_PMC_ureg,
                  &spr_read_ureg, &spr_write_ureg,
                  0x00000000);
     spr_register(env, SPR_POWER_UPMC5, "UPMC5",
-                 &spr_read_ureg, &spr_write_ureg,
+                 &spr_read_ureg, &spr_write_PMC_ureg,
                  &spr_read_ureg, &spr_write_ureg,
                  0x00000000);
     spr_register(env, SPR_POWER_UPMC6, "UPMC6",
-                 &spr_read_ureg, &spr_write_ureg,
+                 &spr_read_ureg, &spr_write_PMC_ureg,
                  &spr_read_ureg, &spr_write_ureg,
                  0x00000000);
     spr_register(env, SPR_POWER_USIAR, "USIAR",
diff --git a/target/ppc/helper.h b/target/ppc/helper.h
index 91a86992a5..52cb62b9e1 100644
--- a/target/ppc/helper.h
+++ b/target/ppc/helper.h
@@ -23,6 +23,7 @@ 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(insns_inc, void, env, i32)
+DEF_HELPER_3(store_pmc, void, env, i32, i64)
 #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.c b/target/ppc/power8_pmu.c
index edece140aa..9707f6e3cf 100644
--- a/target/ppc/power8_pmu.c
+++ b/target/ppc/power8_pmu.c
@@ -117,6 +117,14 @@ static void update_cycles_PMCs(CPUPPCState *env)
     if (PMC6_running) {
         update_PMC_PM_CYC(env, SPR_POWER_PMC6, time_delta);
     }
+
+    /*
+     * Update base_time for future calculations if we updated
+     * the PMCs while the PMU was running.
+     */
+    if (!(env->spr[SPR_POWER_MMCR0] & MMCR0_FC)) {
+        env->pmu_base_time = now;
+    }
 }
 
 static int64_t get_CYC_timeout(CPUPPCState *env, int sprn)
@@ -416,4 +424,23 @@ void helper_insns_inc(CPUPPCState *env, uint32_t num_insns)
     }
 }
 
+void helper_store_pmc(CPUPPCState *env, uint32_t sprn, uint64_t value)
+{
+    bool pmu_frozen = env->spr[SPR_POWER_MMCR0] & MMCR0_FC;
+
+    if (pmu_frozen) {
+        env->spr[sprn] = value;
+        return;
+    }
+
+    /*
+     * Update counters with the events counted so far, define
+     * the new value of the PMC and start a new cycle count
+     * session.
+     */
+    update_cycles_PMCs(env);
+    env->spr[sprn] = value;
+    start_cycle_count_session(env);
+}
+
 #endif /* defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY) */
diff --git a/target/ppc/spr_tcg.h b/target/ppc/spr_tcg.h
index 5e6ed36eb1..ced714bd09 100644
--- a/target/ppc/spr_tcg.h
+++ b/target/ppc/spr_tcg.h
@@ -27,6 +27,7 @@ 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_CTRL(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);
@@ -46,6 +47,7 @@ void spr_read_spefscr(DisasContext *ctx, int gprn, int sprn);
 void spr_write_spefscr(DisasContext *ctx, int sprn, int gprn);
 void spr_write_ureg(DisasContext *ctx, int sprn, int gprn);
 void spr_write_MMCR0_ureg(DisasContext *ctx, int sprn, int gprn);
+void spr_write_PMC_ureg(DisasContext *ctx, int sprn, int gprn);
 
 #ifndef CONFIG_USER_ONLY
 void spr_write_generic32(DisasContext *ctx, int sprn, int gprn);
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index c23ae2479c..2c5c14b4f1 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -431,13 +431,29 @@ void spr_write_MMCR0(DisasContext *ctx, int sprn, int gprn)
      */
     ctx->base.is_jmp = DISAS_EXIT_UPDATE;
 }
+
+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);
+}
 #else
 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
 
+
+
 #if !defined(CONFIG_USER_ONLY)
 void spr_write_generic32(DisasContext *ctx, int sprn, int gprn)
 {
@@ -641,6 +657,20 @@ void spr_write_MMCR0_ureg(DisasContext *ctx, int sprn, int gprn)
     tcg_temp_free(t0);
     tcg_temp_free(t1);
 }
+
+void spr_write_PMC_ureg(DisasContext *ctx, int sprn, int gprn)
+{
+    /*
+     * All PMCs belongs to Group A SPRs and can't be written by
+     * userspace if PMCC = 0b00.
+     */
+    if (ctx->pmcc_clear) {
+        gen_hvpriv_exception(ctx, POWERPC_EXCP_INVAL_SPR);
+        return;
+    }
+
+    spr_write_PMC(ctx, sprn + 0x10, gprn);
+}
 #else
 void spr_write_ureg(DisasContext *ctx, int sprn, int gprn)
 {
@@ -651,6 +681,11 @@ void spr_write_MMCR0_ureg(DisasContext *ctx, int sprn, int gprn)
 {
     spr_noaccess(ctx, gprn, sprn);
 }
+
+void spr_write_PMC_ureg(DisasContext *ctx, int sprn, int gprn)
+{
+    spr_noaccess(ctx, gprn, sprn);
+}
 #endif /* defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY) */
 
 /* SPR common to all non-embedded PowerPC */
-- 
2.31.1

[PATCH v3 15/15] target/ppc/power8_pmu.c: handle overflow bits when PMU is running

[Daniel Henrique Barboza]

Up until this moment we were assuming that the counter negative
enabled bits, PMC1CE and PMCjCE, would never be changed when the
PMU is already started.

Turns out that there is no such restriction in the PowerISA v3.1,
and software can enable/disable overflow conditions of the counters
at any time.

To support this scenario, track the overflow bits state when a
write in MMCR0 is made in which the run state of the PMU (MMCR0_FC
bit) didn't change and, if some overflow bit were changed in the
middle of a cycle count session, restart it.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 target/ppc/power8_pmu.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/target/ppc/power8_pmu.c b/target/ppc/power8_pmu.c
index 9707f6e3cf..c9b096f0de 100644
--- a/target/ppc/power8_pmu.c
+++ b/target/ppc/power8_pmu.c
@@ -345,6 +345,30 @@ void helper_store_mmcr0(CPUPPCState *env, target_ulong value)
         } else {
             start_cycle_count_session(env);
         }
+    } else {
+        /*
+         * No change in MMCR0_FC state, but if the PMU is running and
+         * a change in the counter negative overflow bits is made,
+         * we need to restart a new cycle count session to restart
+         * the appropriate overflow timers.
+         */
+        if (curr_FC) {
+            return;
+        }
+
+        bool pmc1ce_curr = curr_value & MMCR0_PMC1CE;
+        bool pmc1ce_new  = value & MMCR0_PMC1CE;
+        bool pmcjce_curr = curr_value & MMCR0_PMCjCE;
+        bool pmcjce_new  = value & MMCR0_PMCjCE;
+
+        if (pmc1ce_curr == pmc1ce_new && pmcjce_curr == pmcjce_new) {
+            return;
+        }
+
+        /* Update the counter with the events counted so far */
+        update_cycles_PMCs(env);
+
+        start_cycle_count_session(env);
     }
 }
 
-- 
2.31.1

Re: [PATCH v3 01/15] target/ppc: add user read functions for MMCR0 and MMCR2

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1892 bytes --]

On Fri, Sep 03, 2021 at 05:31:02PM -0300, Daniel Henrique Barboza wrote:
> From: Gustavo Romero <gromero@linux.ibm.com>
> 
> We're going to add PMU support for TCG PPC64 chips, based on IBM POWER8+
> emulation and following PowerISA v3.1.
> 
> Let's start by handling the user read of UMMCR0 and UMMCR2. According to
> PowerISA 3.1 these registers omit some of its bits from userspace.
> 
> 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>

LGTM except for one nit...

[snip]
> +void spr_read_MMCR2_ureg(DisasContext *ctx, int gprn, int sprn)
> +{
> +    TCGv t0 = tcg_temp_new();
> +
> +    /*
> +     * On read, filter out all bits that are not FCnP0 bits.
> +     * When MMCR0[PMCC] is set to 0b10 or 0b11, providing
> +     * problem state programs read/write access to MMCR2,
> +     * only the FCnP0 bits can be accessed. All other bits are
> +     * not changed when mtspr is executed in problem state, and
> +     * all other bits return 0s when mfspr is executed in problem
> +     * state, according to ISA v3.1, section 10.4.6 Monitor Mode
> +     * Control Register 2, p. 1316, third paragraph.
> +     */
> +    gen_load_spr(t0, SPR_POWER_MMCR2);
> +    tcg_gen_andi_tl(t0, t0, 0x4020100804020000UL);

A #define for this mask... and #defines with meaningful names for the
various bits it includes would be nice.

> +    tcg_gen_mov_tl(cpu_gpr[gprn], t0);
> +
> +    tcg_temp_free(t0);
> +}
> +
>  #if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
>  void spr_write_ureg(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 --]

Re: [PATCH v3 02/15] target/ppc: add user write access control for PMU SPRs

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 9639 bytes --]

On Fri, Sep 03, 2021 at 05:31:03PM -0300, Daniel Henrique Barboza wrote:
> The PMU needs to enable writing of its uregs to userspace, otherwise
> Perf applications will not able to setup the counters correctly. This
> patch enables user space writing of all PMU uregs.
> 
> MMCR0 is a special case because its userspace writing access is controlled
> by MMCR0_PMCC bits. There are 4 configurations available (0b00, 0b01,
> 0b10 and 0b11) but for our purposes here we're handling only
> MMCR0_PMCC = 0b00. In this case, if userspace tries to write MMCR0, a
> hypervisor emulation assistance interrupt occurs.
> 
> This is being done by adding HFLAGS_PMCCCLEAR to hflags. This flag
> indicates if MMCR0_PMCC is cleared (0b00), and a new 'pmcc_clear' flag in
> DisasContext allow us to use it in spr_write_MMCR0_ureg().
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>  target/ppc/cpu.h         |  1 +
>  target/ppc/cpu_init.c    | 18 +++++++-------
>  target/ppc/helper_regs.c |  3 +++
>  target/ppc/spr_tcg.h     |  3 ++-
>  target/ppc/translate.c   | 53 +++++++++++++++++++++++++++++++++++++++-
>  5 files changed, 67 insertions(+), 11 deletions(-)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index f68bb8d8aa..8dfbb62022 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -616,6 +616,7 @@ enum {
>      HFLAGS_SE = 10,  /* MSR_SE -- from elsewhere on embedded ppc */
>      HFLAGS_FP = 13,  /* MSR_FP */
>      HFLAGS_PR = 14,  /* MSR_PR */
> +    HFLAGS_PMCCCLEAR = 15, /* PMU MMCR0 PMCC equal to 0b00 */
>      HFLAGS_VSX = 23, /* MSR_VSX if cpu has VSX */
>      HFLAGS_VR = 25,  /* MSR_VR if cpu has VRE */
>  
> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> index 9efc6c2d87..bb5ea04c61 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -6867,7 +6867,7 @@ static void register_book3s_pmu_sup_sprs(CPUPPCState *env)
>  static void register_book3s_pmu_user_sprs(CPUPPCState *env)
>  {
>      spr_register(env, SPR_POWER_UMMCR0, "UMMCR0",
> -                 &spr_read_MMCR0_ureg, SPR_NOACCESS,
> +                 &spr_read_MMCR0_ureg, &spr_write_MMCR0_ureg,
>                   &spr_read_ureg, &spr_write_ureg,
>                   0x00000000);
>      spr_register(env, SPR_POWER_UMMCR1, "UMMCR1",
> @@ -6875,31 +6875,31 @@ static void register_book3s_pmu_user_sprs(CPUPPCState *env)
>                   &spr_read_ureg, &spr_write_ureg,
>                   0x00000000);
>      spr_register(env, SPR_POWER_UMMCRA, "UMMCRA",
> -                 &spr_read_ureg, SPR_NOACCESS,
> +                 &spr_read_ureg, &spr_write_ureg,
>                   &spr_read_ureg, &spr_write_ureg,
>                   0x00000000);
>      spr_register(env, SPR_POWER_UPMC1, "UPMC1",
> -                 &spr_read_ureg, SPR_NOACCESS,
> +                 &spr_read_ureg, &spr_write_ureg,

Surely this can't be write.  AFAICT spr_write_ureg() will
unconditionally allow full userspace write access.  That can't be
right - otherwise the OS could never safely use the PMU for itself.

>                   &spr_read_ureg, &spr_write_ureg,
>                   0x00000000);
>      spr_register(env, SPR_POWER_UPMC2, "UPMC2",
> -                 &spr_read_ureg, SPR_NOACCESS,
> +                 &spr_read_ureg, &spr_write_ureg,
>                   &spr_read_ureg, &spr_write_ureg,
>                   0x00000000);
>      spr_register(env, SPR_POWER_UPMC3, "UPMC3",
> -                 &spr_read_ureg, SPR_NOACCESS,
> +                 &spr_read_ureg, &spr_write_ureg,
>                   &spr_read_ureg, &spr_write_ureg,
>                   0x00000000);
>      spr_register(env, SPR_POWER_UPMC4, "UPMC4",
> -                 &spr_read_ureg, SPR_NOACCESS,
> +                 &spr_read_ureg, &spr_write_ureg,
>                   &spr_read_ureg, &spr_write_ureg,
>                   0x00000000);
>      spr_register(env, SPR_POWER_UPMC5, "UPMC5",
> -                 &spr_read_ureg, SPR_NOACCESS,
> +                 &spr_read_ureg, &spr_write_ureg,
>                   &spr_read_ureg, &spr_write_ureg,
>                   0x00000000);
>      spr_register(env, SPR_POWER_UPMC6, "UPMC6",
> -                 &spr_read_ureg, SPR_NOACCESS,
> +                 &spr_read_ureg, &spr_write_ureg,
>                   &spr_read_ureg, &spr_write_ureg,
>                   0x00000000);
>      spr_register(env, SPR_POWER_USIAR, "USIAR",
> @@ -6975,7 +6975,7 @@ static void register_power8_pmu_sup_sprs(CPUPPCState *env)
>  static void register_power8_pmu_user_sprs(CPUPPCState *env)
>  {
>      spr_register(env, SPR_POWER_UMMCR2, "UMMCR2",
> -                 &spr_read_MMCR2_ureg, SPR_NOACCESS,
> +                 &spr_read_MMCR2_ureg, &spr_write_ureg,
>                   &spr_read_ureg, &spr_write_ureg,
>                   0x00000000);
>      spr_register(env, SPR_POWER_USIER, "USIER",
> diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
> index 405450d863..4c1d9575ac 100644
> --- a/target/ppc/helper_regs.c
> +++ b/target/ppc/helper_regs.c
> @@ -106,6 +106,9 @@ static uint32_t hreg_compute_hflags_value(CPUPPCState *env)
>      if (env->spr[SPR_LPCR] & LPCR_GTSE) {
>          hflags |= 1 << HFLAGS_GTSE;
>      }
> +    if (((env->spr[SPR_POWER_MMCR0] & MMCR0_PMCC) >> 18) == 0) {
> +        hflags |= 1 << HFLAGS_PMCCCLEAR;
> +    }
>  
>  #ifndef CONFIG_USER_ONLY
>      if (!env->has_hv_mode || (msr & (1ull << MSR_HV))) {
> diff --git a/target/ppc/spr_tcg.h b/target/ppc/spr_tcg.h
> index 30cb6c3fdc..094466a2b2 100644
> --- a/target/ppc/spr_tcg.h
> +++ b/target/ppc/spr_tcg.h
> @@ -42,6 +42,8 @@ void spr_read_601_rtcl(DisasContext *ctx, int gprn, int sprn);
>  void spr_read_601_rtcu(DisasContext *ctx, int gprn, int sprn);
>  void spr_read_spefscr(DisasContext *ctx, int gprn, int sprn);
>  void spr_write_spefscr(DisasContext *ctx, int sprn, int gprn);
> +void spr_write_ureg(DisasContext *ctx, int sprn, int gprn);
> +void spr_write_MMCR0_ureg(DisasContext *ctx, int sprn, int gprn);
>  
>  #ifndef CONFIG_USER_ONLY
>  void spr_write_generic32(DisasContext *ctx, int sprn, int gprn);
> @@ -96,7 +98,6 @@ void spr_read_mas73(DisasContext *ctx, int gprn, int sprn);
>  #ifdef TARGET_PPC64
>  void spr_read_cfar(DisasContext *ctx, int gprn, int sprn);
>  void spr_write_cfar(DisasContext *ctx, int sprn, int gprn);
> -void spr_write_ureg(DisasContext *ctx, int sprn, int gprn);
>  void spr_read_purr(DisasContext *ctx, int gprn, int sprn);
>  void spr_write_purr(DisasContext *ctx, int sprn, int gprn);
>  void spr_read_hdecr(DisasContext *ctx, int gprn, int sprn);
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index b2ead144d1..0babde3131 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -175,6 +175,7 @@ struct DisasContext {
>      bool spe_enabled;
>      bool tm_enabled;
>      bool gtse;
> +    bool pmcc_clear;
>      ppc_spr_t *spr_cb; /* Needed to check rights for mfspr/mtspr */
>      int singlestep_enabled;
>      uint32_t flags;
> @@ -561,7 +562,56 @@ void spr_write_ureg(DisasContext *ctx, int sprn, int gprn)
>  {
>      gen_store_spr(sprn + 0x10, cpu_gpr[gprn]);
>  }
> -#endif
> +
> +void spr_write_MMCR0_ureg(DisasContext *ctx, int sprn, int gprn)


Could you put this def in the PMU specific file, rather than the
enormous translate.c?

> +{
> +    TCGv t0, t1;
> +
> +    /*
> +     * For group A PMU sprs, if PMCC = 0b00, PowerISA v3.1
> +     * dictates that:
> +     *
> +     * "If an attempt is made to write to an SPR in group A in
> +     * problem state, a Hypervisor Emulation Assistance
> +     * interrupt will occur."
> +     *
> +     * MMCR0 is a Group A SPR and can't be written by userspace
> +     * if PMCC = 0b00.
> +     */
> +    if (ctx->pmcc_clear) {
> +        gen_hvpriv_exception(ctx, POWERPC_EXCP_INVAL_SPR);
> +        return;
> +    }
> +
> +    t0 = tcg_temp_new();
> +    t1 = tcg_temp_new();
> +
> +    /*
> +     * Filter out all bits but FC, PMAO, and PMAE, according
> +     * to ISA v3.1, in 10.4.4 Monitor Mode Control Register 0,
> +     * fourth paragraph.
> +     */
> +    tcg_gen_andi_tl(t0, cpu_gpr[gprn], MMCR0_UREG_MASK);
> +    gen_load_spr(t1, SPR_POWER_MMCR0);
> +    tcg_gen_andi_tl(t1, t1, ~(MMCR0_UREG_MASK));
> +    /* Keep all other bits intact */
> +    tcg_gen_or_tl(t1, t1, t0);
> +    gen_store_spr(SPR_POWER_MMCR0, t1);
> +
> +    tcg_temp_free(t0);
> +    tcg_temp_free(t1);
> +}
> +#else
> +void spr_write_ureg(DisasContext *ctx, int sprn, int gprn)

Why do you need another definition of spr_write_ureg() here?

> +{
> +    spr_noaccess(ctx, gprn, sprn);
> +}
> +
> +void spr_write_MMCR0_ureg(DisasContext *ctx, int sprn, int gprn)
> +{
> +    spr_noaccess(ctx, gprn, sprn);
> +}
> +#endif /* defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY) */
>  
>  /* SPR common to all non-embedded PowerPC */
>  /* DECR */
> @@ -8576,6 +8626,7 @@ static void ppc_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
>      ctx->vsx_enabled = (hflags >> HFLAGS_VSX) & 1;
>      ctx->tm_enabled = (hflags >> HFLAGS_TM) & 1;
>      ctx->gtse = (hflags >> HFLAGS_GTSE) & 1;
> +    ctx->pmcc_clear = (hflags >> HFLAGS_PMCCCLEAR) & 1;
>  
>      ctx->singlestep_enabled = 0;
>      if ((hflags >> HFLAGS_SE) & 1) {

-- 
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 --]

Re: [PATCH v3 03/15] target/ppc: PMU basic cycle count for pseries TCG

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2002 bytes --]

On Fri, Sep 03, 2021 at 05:31:04PM -0300, Daniel Henrique Barboza wrote:
> This patch adds the barebones of the PMU logic by enabling cycle
> counting, done via the performance monitor counter 6. The overall logic
> goes as follows:
> 
> - a helper is added to control the PMU state on each MMCR0 write. This
> allows for the PMU to start/stop as the frozen counter bit (MMCR0_FC)
> is cleared or set;
> 
> - MMCR0 reg initial value is set to 0x80000000 (MMCR0_FC set) to avoid
> having to spin the PMU right at system init;
> 
> - the intended usage is to freeze the counters by setting MMCR0_FC, do
> any additional setting of events to be counted via MMCR1 (not
> implemented yet) and enable the PMU by zeroing MMCR0_FC. Software must
> freeze counters to read the results - on the fly reading of the PMCs
> will return the starting value of each one.
> 
> Since there will be more PMU exclusive code to be added next, put the
> PMU logic in its own helper to keep all in the same place. The name of
> the new helper file, power8_pmu.c, is an indicative that the PMU logic
> has been tested with the IBM POWER chip family, POWER8 being the oldest
> version tested. This doesn't mean that this PMU logic will break with
> any other PPC64 chip that implements Book3s, but since we can't assert
> that this PMU will work with all available Book3s emulated processors
> we're choosing to be explicit.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

LGTM, except for one nit:
> +#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
> +
> +static void update_PMC_PM_CYC(CPUPPCState *env, int sprn,
> +                              uint64_t time_delta)
> +{
> +    /*
> +     * The pseries and pvn clock runs at 1Ghz, meaning that

s/pvn/pnv/ ?


-- 
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 --]

Re: [PATCH v3 04/15] target/ppc/power8_pmu.c: enable PMC1-PMC4 events

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4502 bytes --]

On Fri, Sep 03, 2021 at 05:31:05PM -0300, Daniel Henrique Barboza wrote:
65;6402;1c> This patch enable all PMCs but PMC5 to count cycles. To do that we
> need to implement MMCR1 bits where the event are stored, retrieve
> them, see if the PMC was configured with a PM_CYC event, and
> calculate cycles if that's the case.
> 
> PowerISA v3.1 defines the following conditions to count cycles:
> 
> - PMC1 set with the event 0xF0;
> - PMC6, which always count cycles
> 
> However, the PowerISA also defines a range of 'implementation dependent'
> events that the chip can use in the 0x01-0xBF range. Turns out that IBM
> POWER chips implements some non-ISA events, and the Linux kernel makes uses
> of them. For instance, 0x1E is an implementation specific event that
> counts cycles in PMCs 1-4 that the kernel uses. Let's also support 0x1E
> to count cycles to allow for existing kernels to behave properly with the
> PMU.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  target/ppc/cpu.h        | 11 +++++++++
>  target/ppc/power8_pmu.c | 52 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 63 insertions(+)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index a9b31736af..74698a3600 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -352,6 +352,17 @@ typedef struct ppc_v3_pate_t {
>  /* MMCR0 userspace r/w mask */
>  #define MMCR0_UREG_MASK (MMCR0_FC | MMCR0_PMAO | MMCR0_PMAE)
>  
> +#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)
> diff --git a/target/ppc/power8_pmu.c b/target/ppc/power8_pmu.c
> index 47de38a99e..3f7b305f4f 100644
> --- a/target/ppc/power8_pmu.c
> +++ b/target/ppc/power8_pmu.c
> @@ -31,10 +31,62 @@ static void update_PMC_PM_CYC(CPUPPCState *env, int sprn,
>      env->spr[sprn] += time_delta;
>  }
>  
> +static void update_programmable_PMC_reg(CPUPPCState *env, int sprn,
> +                                        uint64_t time_delta)
> +{
> +    uint8_t event, evt_extr;
> +
> +    switch (sprn) {
> +    case SPR_POWER_PMC1:
> +        evt_extr = MMCR1_PMC1EVT_EXTR;
> +        break;
> +    case SPR_POWER_PMC2:
> +        evt_extr = MMCR1_PMC2EVT_EXTR;
> +        break;
> +    case SPR_POWER_PMC3:
> +        evt_extr = MMCR1_PMC3EVT_EXTR;
> +        break;
> +    case SPR_POWER_PMC4:
> +        evt_extr = MMCR1_PMC4EVT_EXTR;
> +        break;
> +    default:
> +        return;
> +    }
> +
> +    event = extract64(env->spr[SPR_POWER_MMCR1], evt_extr, MMCR1_EVT_SIZE);
> +
> +    /*
> +     * MMCR0_PMC1SEL = 0xF0 is the architected PowerISA v3.1 event
> +     * that counts cycles using PMC1.
> +     *
> +     * IBM POWER chips also has support for an implementation dependent
> +     * event, 0x1E, that enables cycle counting on PMCs 1-4. The
> +     * Linux kernel makes extensive use of 0x1E, so let's also support
> +     * it.
> +     */
> +    switch (event) {
> +    case 0xF0:
> +        if (sprn == SPR_POWER_PMC1) {
> +            update_PMC_PM_CYC(env, sprn, time_delta);
> +        }
> +        break;
> +    case 0x1E:
> +        update_PMC_PM_CYC(env, sprn, time_delta);
> +        break;
> +    default:
> +        return;
> +    }
> +}
> +
>  static void update_cycles_PMCs(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_PMC5; sprn++) {
> +        update_programmable_PMC_reg(env, sprn, time_delta);
> +    }
>  
>      update_PMC_PM_CYC(env, SPR_POWER_PMC6, time_delta);
>  }

-- 
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 --]

Re: [PATCH v3 05/15] target/ppc: PMU: add instruction counting

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 10923 bytes --]

On Fri, Sep 03, 2021 at 05:31:06PM -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 later on.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>  target/ppc/cpu.h         |  1 +
>  target/ppc/helper.h      |  1 +
>  target/ppc/helper_regs.c |  3 ++
>  target/ppc/power8_pmu.c  | 70 ++++++++++++++++++++++++++++++++++++----
>  target/ppc/translate.c   | 46 ++++++++++++++++++++++++++
>  5 files changed, 114 insertions(+), 7 deletions(-)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 74698a3600..4d4886ac74 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -628,6 +628,7 @@ enum {
>      HFLAGS_FP = 13,  /* MSR_FP */
>      HFLAGS_PR = 14,  /* MSR_PR */
>      HFLAGS_PMCCCLEAR = 15, /* PMU MMCR0 PMCC equal to 0b00 */
> +    HFLAGS_MMCR0FC = 16, /* 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 5122632784..47dbbe6da1 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(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 4c1d9575ac..27d139edd8 100644
> --- a/target/ppc/helper_regs.c
> +++ b/target/ppc/helper_regs.c
> @@ -109,6 +109,9 @@ static uint32_t hreg_compute_hflags_value(CPUPPCState *env)
>      if (((env->spr[SPR_POWER_MMCR0] & MMCR0_PMCC) >> 18) == 0) {
>          hflags |= 1 << HFLAGS_PMCCCLEAR;
>      }
> +    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.c b/target/ppc/power8_pmu.c
> index 3f7b305f4f..9769c0ff35 100644
> --- a/target/ppc/power8_pmu.c
> +++ b/target/ppc/power8_pmu.c
> @@ -31,10 +31,13 @@ static void update_PMC_PM_CYC(CPUPPCState *env, int sprn,
>      env->spr[sprn] += time_delta;
>  }
>  
> -static void update_programmable_PMC_reg(CPUPPCState *env, int sprn,
> -                                        uint64_t time_delta)
> +static uint8_t get_PMC_event(CPUPPCState *env, int sprn)

I like the idea of splitting out a helper to get the selected event
(might even make sense to move that to the earlier patch).  What would
be even nicer is if it also included handling of the fact that some
events are specific to particular PMCs (like 0xF0 for PMC1).  That
means that all the event selection logic will be here, rather than
having to check the PMC number again in the caller.  Obviously to do
that you'll need some special "bad event" return value, which might
mean changing the return type.

>  {
> -    uint8_t event, evt_extr;
> +    uint8_t evt_extr = 0;
> +
> +    if (env->spr[SPR_POWER_MMCR1] == 0) {
> +        return 0;
> +    }
>  
>      switch (sprn) {
>      case SPR_POWER_PMC1:
> @@ -50,10 +53,16 @@ static void update_programmable_PMC_reg(CPUPPCState *env, int sprn,
>          evt_extr = MMCR1_PMC4EVT_EXTR;
>          break;
>      default:
> -        return;
> +        return 0;
>      }
>  
> -    event = extract64(env->spr[SPR_POWER_MMCR1], evt_extr, MMCR1_EVT_SIZE);
> +    return extract64(env->spr[SPR_POWER_MMCR1], evt_extr, MMCR1_EVT_SIZE);
> +}
> +
> +static void update_programmable_PMC_reg(CPUPPCState *env, int sprn,
> +                                        uint64_t time_delta)
> +{
> +    uint8_t event = get_PMC_event(env, sprn);
>  
>      /*
>       * MMCR0_PMC1SEL = 0xF0 is the architected PowerISA v3.1 event
> @@ -99,8 +108,9 @@ void helper_store_mmcr0(CPUPPCState *env, target_ulong value)
>  
>      env->spr[SPR_POWER_MMCR0] = value;
>  
> -    /* MMCR0 writes can change HFLAGS_PMCCCLEAR */
> -    if ((curr_value & MMCR0_PMCC) != (value & MMCR0_PMCC)) {
> +    /* MMCR0 writes can change HFLAGS_PMCCCLEAR and HFLAGS_MMCR0FC */
> +    if (((curr_value & MMCR0_PMCC) != (value & MMCR0_PMCC)) ||
> +        (curr_FC != new_FC)) {
>          hreg_compute_hflags(env);
>      }
>  
> @@ -123,4 +133,50 @@ void helper_store_mmcr0(CPUPPCState *env, target_ulong value)
>      }
>  }
>  
> +static bool pmc_counting_insns(CPUPPCState *env, int sprn)
> +{
> +    bool ret = false;
> +    uint8_t event;
> +
> +    if (sprn == SPR_POWER_PMC5) {
> +        return true;
> +    }
> +
> +    event = get_PMC_event(env, sprn);
> +
> +    /*
> +     * 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
> +     * all programmable PMCs.
> +     *
> +     * Event 0xFE is the PowerISA v3.1 architected event to
> +     * sample PM_INST_CMPL using PMC1.
> +     */
> +    switch (sprn) {
> +    case SPR_POWER_PMC1:
> +        return event == 0x2 || event == 0xFE;
> +    case SPR_POWER_PMC2:
> +    case SPR_POWER_PMC3:
> +    case SPR_POWER_PMC4:
> +        return event == 0x2;
> +    default:
> +        break;
> +    }
> +
> +    return ret;
> +}
> +
> +/* This helper assumes that the PMC is running. */
> +void helper_insns_inc(CPUPPCState *env, uint32_t num_insns)
> +{
> +    int sprn;
> +
> +    for (sprn = SPR_POWER_PMC1; sprn <= SPR_POWER_PMC5; sprn++) {
> +        if (pmc_counting_insns(env, sprn)) {
> +            env->spr[sprn] += num_insns;
> +        }
> +    }
> +}
> +
>  #endif /* defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY) */
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index c3e2e3d329..b7235a2be0 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -176,6 +176,7 @@ struct DisasContext {
>      bool tm_enabled;
>      bool gtse;
>      bool pmcc_clear;
> +    bool pmu_frozen;
>      ppc_spr_t *spr_cb; /* Needed to check rights for mfspr/mtspr */
>      int singlestep_enabled;
>      uint32_t flags;
> @@ -411,6 +412,12 @@ void spr_write_MMCR0(DisasContext *ctx, int sprn, int gprn)
>       */
>      gen_icount_io_start(ctx);
>      gen_helper_store_mmcr0(cpu_env, cpu_gpr[gprn]);
> +
> +    /*
> +     * End the translation block because MMCR0 writes can change
> +     * ctx->pmu_frozen.
> +     */
> +    ctx->base.is_jmp = DISAS_EXIT_UPDATE;
>  }
>  #else
>  void spr_write_MMCR0(DisasContext *ctx, int sprn, int gprn)
> @@ -4407,6 +4414,22 @@ 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;
> +    }
> +
> +    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);
> @@ -4421,9 +4444,17 @@ static void gen_lookup_and_goto_ptr(DisasContext *ctx)
>          } else if (sse & (CPU_SINGLE_STEP | CPU_BRANCH_STEP)) {
>              gen_helper_raise_exception(cpu_env, tcg_constant_i32(gen_prep_dbgex(ctx)));
>          } else {
> +            pmu_count_insns(ctx);
>              tcg_gen_exit_tb(NULL, 0);
>          }
>      } 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();
>      }
>  }
> @@ -4435,6 +4466,8 @@ 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);
> @@ -8648,6 +8681,7 @@ static void ppc_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
>      ctx->tm_enabled = (hflags >> HFLAGS_TM) & 1;
>      ctx->gtse = (hflags >> HFLAGS_GTSE) & 1;
>      ctx->pmcc_clear = (hflags >> HFLAGS_PMCCCLEAR) & 1;
> +    ctx->pmu_frozen = (hflags >> HFLAGS_MMCR0FC) & 1;
>  
>      ctx->singlestep_enabled = 0;
>      if ((hflags >> HFLAGS_SE) & 1) {
> @@ -8767,6 +8801,8 @@ 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);
> @@ -8777,6 +8813,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;
>  
> @@ -8784,6 +8828,8 @@ 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 --]

Re: [PATCH v3 08/15] PPC64/TCG: Implement 'rfebb' instruction

[Matheus K. Ferst]

On 03/09/2021 17:31, 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
> 
> 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 93f4a46827..26624508fa 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -368,6 +368,19 @@ typedef struct ppc_v3_pate_t {
>   /* 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 7b6ac16eef..22f9835383 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -1281,6 +1281,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 47dbbe6da1..91a86992a5 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 9fd8d6b817..deb7374ea4 100644
> --- a/target/ppc/insn32.decode
> +++ b/target/ppc/insn32.decode
> @@ -124,3 +124,8 @@ SETNBCR         011111 ..... ..... ----- 0111100000 -   @X_bi
>   ## Vector Bit Manipulation Instruction
> 
>   VCFUGED         000100 ..... ..... ..... 10101001101    @VX
> +
> +### rfebb
> +&XL_s           s:uint8_t
> +@XL_s           ......-------------- s:1 .......... -   &XL_s
> +RFEBB           010011-------------- .   0010010010 -   @XL_s

nit: Since the arg_fmt is now XL_s...

> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 866b1d2b34..7a3104ecf9 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -7636,6 +7636,8 @@ static int times_4(DisasContext *ctx, int x)
> 
>   #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..9c991d9abb
> --- /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_RFEBB *arg)

I think it's a bit more readable to use arg_XL_s instead of arg_RFEBB. 
Anyway,

Reviewed-by: Matheus Ferst <matheus.ferst@eldorado.org.br>

> +{
> +    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_RFEBB *arg)
> +{
> +    gen_invalid(ctx);
> +    return true;
> +}
> +#endif
> --
> 2.31.1
> 


-- 
Matheus K. Ferst
Instituto de Pesquisas ELDORADO <http://www.eldorado.org.br/>
Analista de Software Júnior
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>

Re: [PATCH v3 05/15] target/ppc: PMU: add instruction counting

[Daniel Henrique Barboza]

On 9/6/21 22:57, David Gibson wrote:
> On Fri, Sep 03, 2021 at 05:31:06PM -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 later on.
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
>>   target/ppc/cpu.h         |  1 +
>>   target/ppc/helper.h      |  1 +
>>   target/ppc/helper_regs.c |  3 ++
>>   target/ppc/power8_pmu.c  | 70 ++++++++++++++++++++++++++++++++++++----
>>   target/ppc/translate.c   | 46 ++++++++++++++++++++++++++
>>   5 files changed, 114 insertions(+), 7 deletions(-)
>>
>> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
>> index 74698a3600..4d4886ac74 100644
>> --- a/target/ppc/cpu.h
>> +++ b/target/ppc/cpu.h
>> @@ -628,6 +628,7 @@ enum {
>>       HFLAGS_FP = 13,  /* MSR_FP */
>>       HFLAGS_PR = 14,  /* MSR_PR */
>>       HFLAGS_PMCCCLEAR = 15, /* PMU MMCR0 PMCC equal to 0b00 */
>> +    HFLAGS_MMCR0FC = 16, /* 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 5122632784..47dbbe6da1 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(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 4c1d9575ac..27d139edd8 100644
>> --- a/target/ppc/helper_regs.c
>> +++ b/target/ppc/helper_regs.c
>> @@ -109,6 +109,9 @@ static uint32_t hreg_compute_hflags_value(CPUPPCState *env)
>>       if (((env->spr[SPR_POWER_MMCR0] & MMCR0_PMCC) >> 18) == 0) {
>>           hflags |= 1 << HFLAGS_PMCCCLEAR;
>>       }
>> +    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.c b/target/ppc/power8_pmu.c
>> index 3f7b305f4f..9769c0ff35 100644
>> --- a/target/ppc/power8_pmu.c
>> +++ b/target/ppc/power8_pmu.c
>> @@ -31,10 +31,13 @@ static void update_PMC_PM_CYC(CPUPPCState *env, int sprn,
>>       env->spr[sprn] += time_delta;
>>   }
>>   
>> -static void update_programmable_PMC_reg(CPUPPCState *env, int sprn,
>> -                                        uint64_t time_delta)
>> +static uint8_t get_PMC_event(CPUPPCState *env, int sprn)
> 
> I like the idea of splitting out a helper to get the selected event
> (might even make sense to move that to the earlier patch).  What would
> be even nicer is if it also included handling of the fact that some
> events are specific to particular PMCs (like 0xF0 for PMC1).  That
> means that all the event selection logic will be here, rather than
> having to check the PMC number again in the caller.  Obviously to do
> that you'll need some special "bad event" return value, which might
> mean changing the return type.

The initial idea of this function was to be a simple event extractor
returning 0 if MMCR1 is blank. In the end of the series there are 3 callers
of this function that will execute a specific action based on the event
returned by it.

I suppose that we can use the return value 0 as a 'bad value' based on the
PMC events we're going to support, but that will not prevent the callers
from doing a 'switch()' like logic, rechecking the PMC, to see which action
is supposed to be taken.

IIUC, your idea would require an additional layer of abstraction, e.g. a
PMCEvent object, that would tie together PMC + event. Then get_PMC_event()
would return a PMCEvent object and the caller wouldn't need to re-check the
PMC again.

I'll see how hard it would be to introduce this new concept in this existing
series. If it ends up being too much rework I'll suggest to do this in a
follow-up.



Thanks,


Daniel

> 
>>   {
>> -    uint8_t event, evt_extr;
>> +    uint8_t evt_extr = 0;
>> +
>> +    if (env->spr[SPR_POWER_MMCR1] == 0) {
>> +        return 0;
>> +    }
>>   
>>       switch (sprn) {
>>       case SPR_POWER_PMC1:
>> @@ -50,10 +53,16 @@ static void update_programmable_PMC_reg(CPUPPCState *env, int sprn,
>>           evt_extr = MMCR1_PMC4EVT_EXTR;
>>           break;
>>       default:
>> -        return;
>> +        return 0;
>>       }
>>   
>> -    event = extract64(env->spr[SPR_POWER_MMCR1], evt_extr, MMCR1_EVT_SIZE);
>> +    return extract64(env->spr[SPR_POWER_MMCR1], evt_extr, MMCR1_EVT_SIZE);
>> +}
>> +
>> +static void update_programmable_PMC_reg(CPUPPCState *env, int sprn,
>> +                                        uint64_t time_delta)
>> +{
>> +    uint8_t event = get_PMC_event(env, sprn);
>>   
>>       /*
>>        * MMCR0_PMC1SEL = 0xF0 is the architected PowerISA v3.1 event
>> @@ -99,8 +108,9 @@ void helper_store_mmcr0(CPUPPCState *env, target_ulong value)
>>   
>>       env->spr[SPR_POWER_MMCR0] = value;
>>   
>> -    /* MMCR0 writes can change HFLAGS_PMCCCLEAR */
>> -    if ((curr_value & MMCR0_PMCC) != (value & MMCR0_PMCC)) {
>> +    /* MMCR0 writes can change HFLAGS_PMCCCLEAR and HFLAGS_MMCR0FC */
>> +    if (((curr_value & MMCR0_PMCC) != (value & MMCR0_PMCC)) ||
>> +        (curr_FC != new_FC)) {
>>           hreg_compute_hflags(env);
>>       }
>>   
>> @@ -123,4 +133,50 @@ void helper_store_mmcr0(CPUPPCState *env, target_ulong value)
>>       }
>>   }
>>   
>> +static bool pmc_counting_insns(CPUPPCState *env, int sprn)
>> +{
>> +    bool ret = false;
>> +    uint8_t event;
>> +
>> +    if (sprn == SPR_POWER_PMC5) {
>> +        return true;
>> +    }
>> +
>> +    event = get_PMC_event(env, sprn);
>> +
>> +    /*
>> +     * 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
>> +     * all programmable PMCs.
>> +     *
>> +     * Event 0xFE is the PowerISA v3.1 architected event to
>> +     * sample PM_INST_CMPL using PMC1.
>> +     */
>> +    switch (sprn) {
>> +    case SPR_POWER_PMC1:
>> +        return event == 0x2 || event == 0xFE;
>> +    case SPR_POWER_PMC2:
>> +    case SPR_POWER_PMC3:
>> +    case SPR_POWER_PMC4:
>> +        return event == 0x2;
>> +    default:
>> +        break;
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>> +/* This helper assumes that the PMC is running. */
>> +void helper_insns_inc(CPUPPCState *env, uint32_t num_insns)
>> +{
>> +    int sprn;
>> +
>> +    for (sprn = SPR_POWER_PMC1; sprn <= SPR_POWER_PMC5; sprn++) {
>> +        if (pmc_counting_insns(env, sprn)) {
>> +            env->spr[sprn] += num_insns;
>> +        }
>> +    }
>> +}
>> +
>>   #endif /* defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY) */
>> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
>> index c3e2e3d329..b7235a2be0 100644
>> --- a/target/ppc/translate.c
>> +++ b/target/ppc/translate.c
>> @@ -176,6 +176,7 @@ struct DisasContext {
>>       bool tm_enabled;
>>       bool gtse;
>>       bool pmcc_clear;
>> +    bool pmu_frozen;
>>       ppc_spr_t *spr_cb; /* Needed to check rights for mfspr/mtspr */
>>       int singlestep_enabled;
>>       uint32_t flags;
>> @@ -411,6 +412,12 @@ void spr_write_MMCR0(DisasContext *ctx, int sprn, int gprn)
>>        */
>>       gen_icount_io_start(ctx);
>>       gen_helper_store_mmcr0(cpu_env, cpu_gpr[gprn]);
>> +
>> +    /*
>> +     * End the translation block because MMCR0 writes can change
>> +     * ctx->pmu_frozen.
>> +     */
>> +    ctx->base.is_jmp = DISAS_EXIT_UPDATE;
>>   }
>>   #else
>>   void spr_write_MMCR0(DisasContext *ctx, int sprn, int gprn)
>> @@ -4407,6 +4414,22 @@ 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;
>> +    }
>> +
>> +    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);
>> @@ -4421,9 +4444,17 @@ static void gen_lookup_and_goto_ptr(DisasContext *ctx)
>>           } else if (sse & (CPU_SINGLE_STEP | CPU_BRANCH_STEP)) {
>>               gen_helper_raise_exception(cpu_env, tcg_constant_i32(gen_prep_dbgex(ctx)));
>>           } else {
>> +            pmu_count_insns(ctx);
>>               tcg_gen_exit_tb(NULL, 0);
>>           }
>>       } 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();
>>       }
>>   }
>> @@ -4435,6 +4466,8 @@ 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);
>> @@ -8648,6 +8681,7 @@ static void ppc_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
>>       ctx->tm_enabled = (hflags >> HFLAGS_TM) & 1;
>>       ctx->gtse = (hflags >> HFLAGS_GTSE) & 1;
>>       ctx->pmcc_clear = (hflags >> HFLAGS_PMCCCLEAR) & 1;
>> +    ctx->pmu_frozen = (hflags >> HFLAGS_MMCR0FC) & 1;
>>   
>>       ctx->singlestep_enabled = 0;
>>       if ((hflags >> HFLAGS_SE) & 1) {
>> @@ -8767,6 +8801,8 @@ 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);
>> @@ -8777,6 +8813,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;
>>   
>> @@ -8784,6 +8828,8 @@ 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;
>>   
> 

Re: [PATCH v3 01/15] target/ppc: add user read functions for MMCR0 and MMCR2

[Matheus K. Ferst]

On 03/09/2021 17:31, Daniel Henrique Barboza wrote:
> [E-MAIL EXTERNO] Não clique em links ou abra anexos, a menos que você possa confirmar o remetente e saber que o conteúdo é seguro. Em caso de e-mail suspeito entre imediatamente em contato com o DTI.
> 
> From: Gustavo Romero <gromero@linux.ibm.com>
> 
> We're going to add PMU support for TCG PPC64 chips, based on IBM POWER8+
> emulation and following PowerISA v3.1.
> 
> Let's start by handling the user read of UMMCR0 and UMMCR2. According to
> PowerISA 3.1 these registers omit some of its bits from userspace.
> 
> 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       | 10 ++++++++++
>   target/ppc/cpu_init.c  |  4 ++--
>   target/ppc/spr_tcg.h   |  2 ++
>   target/ppc/translate.c | 37 +++++++++++++++++++++++++++++++++++++
>   4 files changed, 51 insertions(+), 2 deletions(-)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 500205229c..f68bb8d8aa 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -342,6 +342,16 @@ typedef struct ppc_v3_pate_t {
>   #define MSR_RI   1  /* Recoverable interrupt                        1        */
>   #define MSR_LE   0  /* Little-endian mode                           1 hflags */
> 
> +/* PMU bits */
> +#define MMCR0_FC    PPC_BIT(32)         /* Freeze Counters  */
> +#define MMCR0_PMAO  PPC_BIT(56)         /* Perf Monitor Alert Ocurred */
> +#define MMCR0_PMAE  PPC_BIT(37)         /* Perf Monitor Alert Enable */
> +#define MMCR0_EBE   PPC_BIT(43)         /* Perf Monitor EBB Enable */
> +#define MMCR0_FCECE PPC_BIT(38)         /* FC on Enabled Cond or Event */
> +#define MMCR0_PMCC  PPC_BITMASK(44, 45) /* PMC Control */
> +/* MMCR0 userspace r/w mask */
> +#define MMCR0_UREG_MASK (MMCR0_FC | MMCR0_PMAO | MMCR0_PMAE)
> +
>   /* 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 ad7abc6041..9efc6c2d87 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -6867,7 +6867,7 @@ static void register_book3s_pmu_sup_sprs(CPUPPCState *env)
>   static void register_book3s_pmu_user_sprs(CPUPPCState *env)
>   {
>       spr_register(env, SPR_POWER_UMMCR0, "UMMCR0",
> -                 &spr_read_ureg, SPR_NOACCESS,
> +                 &spr_read_MMCR0_ureg, SPR_NOACCESS,
>                    &spr_read_ureg, &spr_write_ureg,
>                    0x00000000);
>       spr_register(env, SPR_POWER_UMMCR1, "UMMCR1",
> @@ -6975,7 +6975,7 @@ static void register_power8_pmu_sup_sprs(CPUPPCState *env)
>   static void register_power8_pmu_user_sprs(CPUPPCState *env)
>   {
>       spr_register(env, SPR_POWER_UMMCR2, "UMMCR2",
> -                 &spr_read_ureg, SPR_NOACCESS,
> +                 &spr_read_MMCR2_ureg, SPR_NOACCESS,
>                    &spr_read_ureg, &spr_write_ureg,
>                    0x00000000);
>       spr_register(env, SPR_POWER_USIER, "USIER",
> diff --git a/target/ppc/spr_tcg.h b/target/ppc/spr_tcg.h
> index 0be5f347d5..30cb6c3fdc 100644
> --- a/target/ppc/spr_tcg.h
> +++ b/target/ppc/spr_tcg.h
> @@ -32,6 +32,8 @@ void spr_write_lr(DisasContext *ctx, int sprn, int gprn);
>   void spr_read_ctr(DisasContext *ctx, int gprn, int sprn);
>   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_tbl(DisasContext *ctx, int gprn, int sprn);
>   void spr_read_tbu(DisasContext *ctx, int gprn, int sprn);
>   void spr_read_atbl(DisasContext *ctx, int gprn, int sprn);
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 171b216e17..b2ead144d1 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -519,6 +519,43 @@ void spr_read_ureg(DisasContext *ctx, int gprn, int sprn)
>       gen_load_spr(cpu_gpr[gprn], sprn + 0x10);
>   }
> 
> +void spr_read_MMCR0_ureg(DisasContext *ctx, int gprn, int sprn)
> +{
> +    TCGv t0 = tcg_temp_new();
> +
> +    /*
> +     * Filter out all bits but FC, PMAO, and PMAE, according
> +     * to ISA v3.1, in 10.4.4 Monitor Mode Control Register 0,
> +     * fourth paragraph.
> +     */
> +    gen_load_spr(t0, SPR_POWER_MMCR0);
> +    tcg_gen_andi_tl(t0, t0, MMCR0_UREG_MASK);
> +    tcg_gen_mov_tl(cpu_gpr[gprn], t0);

 From the other patches, it seems that the focus is in the MMCR0[PMCC] = 
0b00 case, but I would note that the PMCC field description says that 
when MMCR0[PMCC] = 0b01, "Group A is not allowed to be read or written 
in problem state." If this case doesn't matter for this initial 
implementation, it'd be nice to leave a comment (XXX/TODO/etc.) saying 
that it's not handled. Otherwise, I think we'll need a helper or add 
both PMCC bits to hflags.

> +
> +    tcg_temp_free(t0);
> +}
> +
> +void spr_read_MMCR2_ureg(DisasContext *ctx, int gprn, int sprn)
> +{
> +    TCGv t0 = tcg_temp_new();
> +
> +    /*
> +     * On read, filter out all bits that are not FCnP0 bits.
> +     * When MMCR0[PMCC] is set to 0b10 or 0b11, providing
> +     * problem state programs read/write access to MMCR2,
> +     * only the FCnP0 bits can be accessed. All other bits are
> +     * not changed when mtspr is executed in problem state, and
> +     * all other bits return 0s when mfspr is executed in problem
> +     * state, according to ISA v3.1, section 10.4.6 Monitor Mode
> +     * Control Register 2, p. 1316, third paragraph.
> +     */
> +    gen_load_spr(t0, SPR_POWER_MMCR2);
> +    tcg_gen_andi_tl(t0, t0, 0x4020100804020000UL);
> +    tcg_gen_mov_tl(cpu_gpr[gprn], t0);
> +
> +    tcg_temp_free(t0);
> +}
> +
>   #if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
>   void spr_write_ureg(DisasContext *ctx, int sprn, int gprn)
>   {
> --
> 2.31.1
> 


-- 
Matheus K. Ferst
Instituto de Pesquisas ELDORADO <http://www.eldorado.org.br/>
Analista de Software Júnior
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>

Re: [PATCH v3 03/15] target/ppc: PMU basic cycle count for pseries TCG

[Matheus K. Ferst]

On 03/09/2021 17:31, Daniel Henrique Barboza wrote:
> [E-MAIL EXTERNO] Não clique em links ou abra anexos, a menos que você possa confirmar o remetente e saber que o conteúdo é seguro. Em caso de e-mail suspeito entre imediatamente em contato com o DTI.
> 
> This patch adds the barebones of the PMU logic by enabling cycle
> counting, done via the performance monitor counter 6. The overall logic
> goes as follows:
> 
> - a helper is added to control the PMU state on each MMCR0 write. This
> allows for the PMU to start/stop as the frozen counter bit (MMCR0_FC)
> is cleared or set;
> 
> - MMCR0 reg initial value is set to 0x80000000 (MMCR0_FC set) to avoid
> having to spin the PMU right at system init;
> 
> - the intended usage is to freeze the counters by setting MMCR0_FC, do
> any additional setting of events to be counted via MMCR1 (not
> implemented yet) and enable the PMU by zeroing MMCR0_FC. Software must
> freeze counters to read the results - on the fly reading of the PMCs
> will return the starting value of each one.
> 
> Since there will be more PMU exclusive code to be added next, put the
> PMU logic in its own helper to keep all in the same place. The name of
> the new helper file, power8_pmu.c, is an indicative that the PMU logic
> has been tested with the IBM POWER chip family, POWER8 being the oldest
> version tested. This doesn't mean that this PMU logic will break with
> any other PPC64 chip that implements Book3s, but since we can't assert
> that this PMU will work with all available Book3s emulated processors
> we're choosing to be explicit.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---

<snip>

> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 0babde3131..c3e2e3d329 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -401,6 +401,24 @@ void spr_write_generic(DisasContext *ctx, int sprn, int gprn)
>       spr_store_dump_spr(sprn);
>   }
> 
> +#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
> +void spr_write_MMCR0(DisasContext *ctx, int sprn, int gprn)
> +{
> +    /*
> +     * 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, cpu_gpr[gprn]);
> +}
> +#else
> +void spr_write_MMCR0(DisasContext *ctx, int sprn, int gprn)
> +{
> +    spr_write_generic(ctx, sprn, gprn);
> +}
> +#endif
> +
>   #if !defined(CONFIG_USER_ONLY)
>   void spr_write_generic32(DisasContext *ctx, int sprn, int gprn)
>   {
> @@ -596,7 +614,10 @@ void spr_write_MMCR0_ureg(DisasContext *ctx, int sprn, int gprn)
>       tcg_gen_andi_tl(t1, t1, ~(MMCR0_UREG_MASK));
>       /* Keep all other bits intact */
>       tcg_gen_or_tl(t1, t1, t0);
> -    gen_store_spr(SPR_POWER_MMCR0, t1);
> +
> +    /* Overwrite cpu_gpr[gprn] and use spr_write_MMCR0() */
> +    tcg_gen_mov_tl(cpu_gpr[gprn], t1);
> +    spr_write_MMCR0(ctx, sprn + 0x10, gprn);

IIUC, this makes writing to MMCR0 change the GPR value and expose the 
unfiltered content of the SPR to problem state. It might be better to 
call the helper directly or create another method that takes a TCGv as 
an argument and call it from spr_write_MMCR0_ureg and spr_write_MMCR0.

> 
>       tcg_temp_free(t0);
>       tcg_temp_free(t1);
> --
> 2.31.1
> 


-- 
Matheus K. Ferst
Instituto de Pesquisas ELDORADO <http://www.eldorado.org.br/>
Analista de Software Júnior
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>

Re: [PATCH v3 08/15] PPC64/TCG: Implement 'rfebb' instruction

[Daniel Henrique Barboza]

On 9/9/21 08:47, Matheus K. Ferst wrote:
> On 03/09/2021 17:31, 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
>>
>> 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 93f4a46827..26624508fa 100644
>> --- a/target/ppc/cpu.h
>> +++ b/target/ppc/cpu.h
>> @@ -368,6 +368,19 @@ typedef struct ppc_v3_pate_t {
>>   /* 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 7b6ac16eef..22f9835383 100644
>> --- a/target/ppc/excp_helper.c
>> +++ b/target/ppc/excp_helper.c
>> @@ -1281,6 +1281,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 47dbbe6da1..91a86992a5 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 9fd8d6b817..deb7374ea4 100644
>> --- a/target/ppc/insn32.decode
>> +++ b/target/ppc/insn32.decode
>> @@ -124,3 +124,8 @@ SETNBCR         011111 ..... ..... ----- 0111100000 -   @X_bi
>>   ## Vector Bit Manipulation Instruction
>>
>>   VCFUGED         000100 ..... ..... ..... 10101001101    @VX
>> +
>> +### rfebb
>> +&XL_s           s:uint8_t
>> +@XL_s           ......-------------- s:1 .......... -   &XL_s
>> +RFEBB           010011-------------- .   0010010010 -   @XL_s
> 
> nit: Since the arg_fmt is now XL_s...
> 
>> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
>> index 866b1d2b34..7a3104ecf9 100644
>> --- a/target/ppc/translate.c
>> +++ b/target/ppc/translate.c
>> @@ -7636,6 +7636,8 @@ static int times_4(DisasContext *ctx, int x)
>>
>>   #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..9c991d9abb
>> --- /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_RFEBB *arg)
> 
> I think it's a bit more readable to use arg_XL_s instead of arg_RFEBB. Anyway,


I forgot to rename the function argument together with the decode tree arg.

I'll rename it in the next version.

> 
> Reviewed-by: Matheus Ferst <matheus.ferst@eldorado.org.br>


Thanks!


Daniel


> 
>> +{
>> +    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_RFEBB *arg)
>> +{
>> +    gen_invalid(ctx);
>> +    return true;
>> +}
>> +#endif
>> -- 
>> 2.31.1
>>
> 
> 

Re: [PATCH v3 01/15] target/ppc: add user read functions for MMCR0 and MMCR2

[Daniel Henrique Barboza]

On 9/22/21 08:23, Matheus K. Ferst wrote:
> On 03/09/2021 17:31, Daniel Henrique Barboza wrote:
>> [E-MAIL EXTERNO] Não clique em links ou abra anexos, a menos que você possa confirmar o remetente e saber que o conteúdo é seguro. Em caso de e-mail suspeito entre imediatamente em contato com o DTI.
>>
>> From: Gustavo Romero <gromero@linux.ibm.com>
>>
>> We're going to add PMU support for TCG PPC64 chips, based on IBM POWER8+
>> emulation and following PowerISA v3.1.
>>
>> Let's start by handling the user read of UMMCR0 and UMMCR2. According to
>> PowerISA 3.1 these registers omit some of its bits from userspace.
>>
>> 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       | 10 ++++++++++
>>   target/ppc/cpu_init.c  |  4 ++--
>>   target/ppc/spr_tcg.h   |  2 ++
>>   target/ppc/translate.c | 37 +++++++++++++++++++++++++++++++++++++
>>   4 files changed, 51 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
>> index 500205229c..f68bb8d8aa 100644
>> --- a/target/ppc/cpu.h
>> +++ b/target/ppc/cpu.h
>> @@ -342,6 +342,16 @@ typedef struct ppc_v3_pate_t {
>>   #define MSR_RI   1  /* Recoverable interrupt                        1        */
>>   #define MSR_LE   0  /* Little-endian mode                           1 hflags */
>>
>> +/* PMU bits */
>> +#define MMCR0_FC    PPC_BIT(32)         /* Freeze Counters  */
>> +#define MMCR0_PMAO  PPC_BIT(56)         /* Perf Monitor Alert Ocurred */
>> +#define MMCR0_PMAE  PPC_BIT(37)         /* Perf Monitor Alert Enable */
>> +#define MMCR0_EBE   PPC_BIT(43)         /* Perf Monitor EBB Enable */
>> +#define MMCR0_FCECE PPC_BIT(38)         /* FC on Enabled Cond or Event */
>> +#define MMCR0_PMCC  PPC_BITMASK(44, 45) /* PMC Control */
>> +/* MMCR0 userspace r/w mask */
>> +#define MMCR0_UREG_MASK (MMCR0_FC | MMCR0_PMAO | MMCR0_PMAE)
>> +
>>   /* 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 ad7abc6041..9efc6c2d87 100644
>> --- a/target/ppc/cpu_init.c
>> +++ b/target/ppc/cpu_init.c
>> @@ -6867,7 +6867,7 @@ static void register_book3s_pmu_sup_sprs(CPUPPCState *env)
>>   static void register_book3s_pmu_user_sprs(CPUPPCState *env)
>>   {
>>       spr_register(env, SPR_POWER_UMMCR0, "UMMCR0",
>> -                 &spr_read_ureg, SPR_NOACCESS,
>> +                 &spr_read_MMCR0_ureg, SPR_NOACCESS,
>>                    &spr_read_ureg, &spr_write_ureg,
>>                    0x00000000);
>>       spr_register(env, SPR_POWER_UMMCR1, "UMMCR1",
>> @@ -6975,7 +6975,7 @@ static void register_power8_pmu_sup_sprs(CPUPPCState *env)
>>   static void register_power8_pmu_user_sprs(CPUPPCState *env)
>>   {
>>       spr_register(env, SPR_POWER_UMMCR2, "UMMCR2",
>> -                 &spr_read_ureg, SPR_NOACCESS,
>> +                 &spr_read_MMCR2_ureg, SPR_NOACCESS,
>>                    &spr_read_ureg, &spr_write_ureg,
>>                    0x00000000);
>>       spr_register(env, SPR_POWER_USIER, "USIER",
>> diff --git a/target/ppc/spr_tcg.h b/target/ppc/spr_tcg.h
>> index 0be5f347d5..30cb6c3fdc 100644
>> --- a/target/ppc/spr_tcg.h
>> +++ b/target/ppc/spr_tcg.h
>> @@ -32,6 +32,8 @@ void spr_write_lr(DisasContext *ctx, int sprn, int gprn);
>>   void spr_read_ctr(DisasContext *ctx, int gprn, int sprn);
>>   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_tbl(DisasContext *ctx, int gprn, int sprn);
>>   void spr_read_tbu(DisasContext *ctx, int gprn, int sprn);
>>   void spr_read_atbl(DisasContext *ctx, int gprn, int sprn);
>> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
>> index 171b216e17..b2ead144d1 100644
>> --- a/target/ppc/translate.c
>> +++ b/target/ppc/translate.c
>> @@ -519,6 +519,43 @@ void spr_read_ureg(DisasContext *ctx, int gprn, int sprn)
>>       gen_load_spr(cpu_gpr[gprn], sprn + 0x10);
>>   }
>>
>> +void spr_read_MMCR0_ureg(DisasContext *ctx, int gprn, int sprn)
>> +{
>> +    TCGv t0 = tcg_temp_new();
>> +
>> +    /*
>> +     * Filter out all bits but FC, PMAO, and PMAE, according
>> +     * to ISA v3.1, in 10.4.4 Monitor Mode Control Register 0,
>> +     * fourth paragraph.
>> +     */
>> +    gen_load_spr(t0, SPR_POWER_MMCR0);
>> +    tcg_gen_andi_tl(t0, t0, MMCR0_UREG_MASK);
>> +    tcg_gen_mov_tl(cpu_gpr[gprn], t0);
> 
>  From the other patches, it seems that the focus is in the MMCR0[PMCC] = 0b00 case, but I would note that the PMCC field description says that when MMCR0[PMCC] = 0b01, "Group A is not allowed to be read or written in problem state." If this case doesn't matter for this initial implementation, it'd be nice to leave a comment (XXX/TODO/etc.) saying that it's not handled. Otherwise, I think we'll need a helper or add both PMCC bits to hflags.

This is correct. The reason why PMCC = 0b00 is being handled is because a PMU
kernel selftest exercises this case expecting write failures.

The other 3 cases (all other three PMCC values) aren't being considered. For
instance, the case you just mentioned would require not just MMCR0, but all
the PMCs and MMCR2 to also have problem state read/write forbidden. PMCC 0b11
would remove PMC5 and PMC6 from the Performance Monitor altogether. All of
this is described in the ISA but it's not being used by the PMU/EBB test
case I've been using.

I'll take your advice and add a comment in the code/commit message explaining the
reasoning why PMCC 0b00 is the only case being handled. We can add additional
PMCC controls in a follow-up work of this implementation.


Thanks,


Daniel

> 
>> +
>> +    tcg_temp_free(t0);
>> +}
>> +
>> +void spr_read_MMCR2_ureg(DisasContext *ctx, int gprn, int sprn)
>> +{
>> +    TCGv t0 = tcg_temp_new();
>> +
>> +    /*
>> +     * On read, filter out all bits that are not FCnP0 bits.
>> +     * When MMCR0[PMCC] is set to 0b10 or 0b11, providing
>> +     * problem state programs read/write access to MMCR2,
>> +     * only the FCnP0 bits can be accessed. All other bits are
>> +     * not changed when mtspr is executed in problem state, and
>> +     * all other bits return 0s when mfspr is executed in problem
>> +     * state, according to ISA v3.1, section 10.4.6 Monitor Mode
>> +     * Control Register 2, p. 1316, third paragraph.
>> +     */
>> +    gen_load_spr(t0, SPR_POWER_MMCR2);
>> +    tcg_gen_andi_tl(t0, t0, 0x4020100804020000UL);
>> +    tcg_gen_mov_tl(cpu_gpr[gprn], t0);
>> +
>> +    tcg_temp_free(t0);
>> +}
>> +
>>   #if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
>>   void spr_write_ureg(DisasContext *ctx, int sprn, int gprn)
>>   {
>> -- 
>> 2.31.1
>>
> 
> 

Re: [PATCH v3 02/15] target/ppc: add user write access control for PMU SPRs

[Daniel Henrique Barboza]

On 9/6/21 22:38, David Gibson wrote:
> On Fri, Sep 03, 2021 at 05:31:03PM -0300, Daniel Henrique Barboza wrote:
>> The PMU needs to enable writing of its uregs to userspace, otherwise
>> Perf applications will not able to setup the counters correctly. This
>> patch enables user space writing of all PMU uregs.
>>
>> MMCR0 is a special case because its userspace writing access is controlled
>> by MMCR0_PMCC bits. There are 4 configurations available (0b00, 0b01,
>> 0b10 and 0b11) but for our purposes here we're handling only
>> MMCR0_PMCC = 0b00. In this case, if userspace tries to write MMCR0, a
>> hypervisor emulation assistance interrupt occurs.
>>
>> This is being done by adding HFLAGS_PMCCCLEAR to hflags. This flag
>> indicates if MMCR0_PMCC is cleared (0b00), and a new 'pmcc_clear' flag in
>> DisasContext allow us to use it in spr_write_MMCR0_ureg().
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
>>   target/ppc/cpu.h         |  1 +
>>   target/ppc/cpu_init.c    | 18 +++++++-------
>>   target/ppc/helper_regs.c |  3 +++
>>   target/ppc/spr_tcg.h     |  3 ++-
>>   target/ppc/translate.c   | 53 +++++++++++++++++++++++++++++++++++++++-
>>   5 files changed, 67 insertions(+), 11 deletions(-)
>>
>> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
>> index f68bb8d8aa..8dfbb62022 100644
>> --- a/target/ppc/cpu.h
>> +++ b/target/ppc/cpu.h
>> @@ -616,6 +616,7 @@ enum {
>>       HFLAGS_SE = 10,  /* MSR_SE -- from elsewhere on embedded ppc */
>>       HFLAGS_FP = 13,  /* MSR_FP */
>>       HFLAGS_PR = 14,  /* MSR_PR */
>> +    HFLAGS_PMCCCLEAR = 15, /* PMU MMCR0 PMCC equal to 0b00 */
>>       HFLAGS_VSX = 23, /* MSR_VSX if cpu has VSX */
>>       HFLAGS_VR = 25,  /* MSR_VR if cpu has VRE */
>>   
>> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
>> index 9efc6c2d87..bb5ea04c61 100644
>> --- a/target/ppc/cpu_init.c
>> +++ b/target/ppc/cpu_init.c
>> @@ -6867,7 +6867,7 @@ static void register_book3s_pmu_sup_sprs(CPUPPCState *env)
>>   static void register_book3s_pmu_user_sprs(CPUPPCState *env)
>>   {
>>       spr_register(env, SPR_POWER_UMMCR0, "UMMCR0",
>> -                 &spr_read_MMCR0_ureg, SPR_NOACCESS,
>> +                 &spr_read_MMCR0_ureg, &spr_write_MMCR0_ureg,
>>                    &spr_read_ureg, &spr_write_ureg,
>>                    0x00000000);
>>       spr_register(env, SPR_POWER_UMMCR1, "UMMCR1",
>> @@ -6875,31 +6875,31 @@ static void register_book3s_pmu_user_sprs(CPUPPCState *env)
>>                    &spr_read_ureg, &spr_write_ureg,
>>                    0x00000000);
>>       spr_register(env, SPR_POWER_UMMCRA, "UMMCRA",
>> -                 &spr_read_ureg, SPR_NOACCESS,
>> +                 &spr_read_ureg, &spr_write_ureg,
>>                    &spr_read_ureg, &spr_write_ureg,
>>                    0x00000000);
>>       spr_register(env, SPR_POWER_UPMC1, "UPMC1",
>> -                 &spr_read_ureg, SPR_NOACCESS,
>> +                 &spr_read_ureg, &spr_write_ureg,
> 
> Surely this can't be write.  AFAICT spr_write_ureg() will
> unconditionally allow full userspace write access.  That can't be
> right - otherwise the OS could never safely use the PMU for itself.

My assumption here was that the user mode SPRs (UMMCR* and UPMC*) were created to
allow userspace read/write of PMU regs, while the regular regs (MMCR* and PMC*)
are the supermode privileged SPRs that can't be written by userspace. At least this
is my understanding from reading commit fd51ff6328e3d98158 that introduced these
userspace PMC regs.

The reason why these are marked as SPR_NOACCESS is because we didn't bothered
writing into them from userspace because we had no PMU logic to work with.


> 
>>                    &spr_read_ureg, &spr_write_ureg,
>>                    0x00000000);
>>       spr_register(env, SPR_POWER_UPMC2, "UPMC2",
>> -                 &spr_read_ureg, SPR_NOACCESS,
>> +                 &spr_read_ureg, &spr_write_ureg,
>>                    &spr_read_ureg, &spr_write_ureg,
>>                    0x00000000);
>>       spr_register(env, SPR_POWER_UPMC3, "UPMC3",
>> -                 &spr_read_ureg, SPR_NOACCESS,
>> +                 &spr_read_ureg, &spr_write_ureg,
>>                    &spr_read_ureg, &spr_write_ureg,
>>                    0x00000000);
>>       spr_register(env, SPR_POWER_UPMC4, "UPMC4",
>> -                 &spr_read_ureg, SPR_NOACCESS,
>> +                 &spr_read_ureg, &spr_write_ureg,
>>                    &spr_read_ureg, &spr_write_ureg,
>>                    0x00000000);
>>       spr_register(env, SPR_POWER_UPMC5, "UPMC5",
>> -                 &spr_read_ureg, SPR_NOACCESS,
>> +                 &spr_read_ureg, &spr_write_ureg,
>>                    &spr_read_ureg, &spr_write_ureg,
>>                    0x00000000);
>>       spr_register(env, SPR_POWER_UPMC6, "UPMC6",
>> -                 &spr_read_ureg, SPR_NOACCESS,
>> +                 &spr_read_ureg, &spr_write_ureg,
>>                    &spr_read_ureg, &spr_write_ureg,
>>                    0x00000000);
>>       spr_register(env, SPR_POWER_USIAR, "USIAR",
>> @@ -6975,7 +6975,7 @@ static void register_power8_pmu_sup_sprs(CPUPPCState *env)
>>   static void register_power8_pmu_user_sprs(CPUPPCState *env)
>>   {
>>       spr_register(env, SPR_POWER_UMMCR2, "UMMCR2",
>> -                 &spr_read_MMCR2_ureg, SPR_NOACCESS,
>> +                 &spr_read_MMCR2_ureg, &spr_write_ureg,
>>                    &spr_read_ureg, &spr_write_ureg,
>>                    0x00000000);
>>       spr_register(env, SPR_POWER_USIER, "USIER",
>> diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
>> index 405450d863..4c1d9575ac 100644
>> --- a/target/ppc/helper_regs.c
>> +++ b/target/ppc/helper_regs.c
>> @@ -106,6 +106,9 @@ static uint32_t hreg_compute_hflags_value(CPUPPCState *env)
>>       if (env->spr[SPR_LPCR] & LPCR_GTSE) {
>>           hflags |= 1 << HFLAGS_GTSE;
>>       }
>> +    if (((env->spr[SPR_POWER_MMCR0] & MMCR0_PMCC) >> 18) == 0) {
>> +        hflags |= 1 << HFLAGS_PMCCCLEAR;
>> +    }
>>   
>>   #ifndef CONFIG_USER_ONLY
>>       if (!env->has_hv_mode || (msr & (1ull << MSR_HV))) {
>> diff --git a/target/ppc/spr_tcg.h b/target/ppc/spr_tcg.h
>> index 30cb6c3fdc..094466a2b2 100644
>> --- a/target/ppc/spr_tcg.h
>> +++ b/target/ppc/spr_tcg.h
>> @@ -42,6 +42,8 @@ void spr_read_601_rtcl(DisasContext *ctx, int gprn, int sprn);
>>   void spr_read_601_rtcu(DisasContext *ctx, int gprn, int sprn);
>>   void spr_read_spefscr(DisasContext *ctx, int gprn, int sprn);
>>   void spr_write_spefscr(DisasContext *ctx, int sprn, int gprn);
>> +void spr_write_ureg(DisasContext *ctx, int sprn, int gprn);
>> +void spr_write_MMCR0_ureg(DisasContext *ctx, int sprn, int gprn);
>>   
>>   #ifndef CONFIG_USER_ONLY
>>   void spr_write_generic32(DisasContext *ctx, int sprn, int gprn);
>> @@ -96,7 +98,6 @@ void spr_read_mas73(DisasContext *ctx, int gprn, int sprn);
>>   #ifdef TARGET_PPC64
>>   void spr_read_cfar(DisasContext *ctx, int gprn, int sprn);
>>   void spr_write_cfar(DisasContext *ctx, int sprn, int gprn);
>> -void spr_write_ureg(DisasContext *ctx, int sprn, int gprn);
>>   void spr_read_purr(DisasContext *ctx, int gprn, int sprn);
>>   void spr_write_purr(DisasContext *ctx, int sprn, int gprn);
>>   void spr_read_hdecr(DisasContext *ctx, int gprn, int sprn);
>> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
>> index b2ead144d1..0babde3131 100644
>> --- a/target/ppc/translate.c
>> +++ b/target/ppc/translate.c
>> @@ -175,6 +175,7 @@ struct DisasContext {
>>       bool spe_enabled;
>>       bool tm_enabled;
>>       bool gtse;
>> +    bool pmcc_clear;
>>       ppc_spr_t *spr_cb; /* Needed to check rights for mfspr/mtspr */
>>       int singlestep_enabled;
>>       uint32_t flags;
>> @@ -561,7 +562,56 @@ void spr_write_ureg(DisasContext *ctx, int sprn, int gprn)
>>   {
>>       gen_store_spr(sprn + 0x10, cpu_gpr[gprn]);
>>   }
>> -#endif
>> +
>> +void spr_write_MMCR0_ureg(DisasContext *ctx, int sprn, int gprn)
> 
> 
> Could you put this def in the PMU specific file, rather than the
> enormous translate.c?

Moving into the existing power8_pmu.c helper is annoying because, being a helper file,
there is no access to the whole DisasContext declaration (that is open coded in
translate.c), and other internal translate.c data like cpu_grp[].

What I was able to do is create a new file in the target/ppc/translate/ dir,
power8-pmu-regs.c.impl, and moved all these declarations over there. At very least we're
not overloading translate.c.

Eldorado, is that ok with you guys? I'm aware that this dir was holding new
decode-tree insns implementations but, in this case, it would hold old format
spr_read/spr_write code.





> 
>> +{
>> +    TCGv t0, t1;
>> +
>> +    /*
>> +     * For group A PMU sprs, if PMCC = 0b00, PowerISA v3.1
>> +     * dictates that:
>> +     *
>> +     * "If an attempt is made to write to an SPR in group A in
>> +     * problem state, a Hypervisor Emulation Assistance
>> +     * interrupt will occur."
>> +     *
>> +     * MMCR0 is a Group A SPR and can't be written by userspace
>> +     * if PMCC = 0b00.
>> +     */
>> +    if (ctx->pmcc_clear) {
>> +        gen_hvpriv_exception(ctx, POWERPC_EXCP_INVAL_SPR);
>> +        return;
>> +    }
>> +
>> +    t0 = tcg_temp_new();
>> +    t1 = tcg_temp_new();
>> +
>> +    /*
>> +     * Filter out all bits but FC, PMAO, and PMAE, according
>> +     * to ISA v3.1, in 10.4.4 Monitor Mode Control Register 0,
>> +     * fourth paragraph.
>> +     */
>> +    tcg_gen_andi_tl(t0, cpu_gpr[gprn], MMCR0_UREG_MASK);
>> +    gen_load_spr(t1, SPR_POWER_MMCR0);
>> +    tcg_gen_andi_tl(t1, t1, ~(MMCR0_UREG_MASK));
>> +    /* Keep all other bits intact */
>> +    tcg_gen_or_tl(t1, t1, t0);
>> +    gen_store_spr(SPR_POWER_MMCR0, t1);
>> +
>> +    tcg_temp_free(t0);
>> +    tcg_temp_free(t1);
>> +}
>> +#else
>> +void spr_write_ureg(DisasContext *ctx, int sprn, int gprn)
> 
> Why do you need another definition of spr_write_ureg() here?

That's an ooopsie.



Thanks,


Daniel

> 
>> +{
>> +    spr_noaccess(ctx, gprn, sprn);
>> +}
>> +
>> +void spr_write_MMCR0_ureg(DisasContext *ctx, int sprn, int gprn)
>> +{
>> +    spr_noaccess(ctx, gprn, sprn);
>> +}
>> +#endif /* defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY) */
>>   
>>   /* SPR common to all non-embedded PowerPC */
>>   /* DECR */
>> @@ -8576,6 +8626,7 @@ static void ppc_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
>>       ctx->vsx_enabled = (hflags >> HFLAGS_VSX) & 1;
>>       ctx->tm_enabled = (hflags >> HFLAGS_TM) & 1;
>>       ctx->gtse = (hflags >> HFLAGS_GTSE) & 1;
>> +    ctx->pmcc_clear = (hflags >> HFLAGS_PMCCCLEAR) & 1;
>>   
>>       ctx->singlestep_enabled = 0;
>>       if ((hflags >> HFLAGS_SE) & 1) {
> 

Re: [PATCH v3 03/15] target/ppc: PMU basic cycle count for pseries TCG

[Daniel Henrique Barboza]

On 9/22/21 08:24, Matheus K. Ferst wrote:
> On 03/09/2021 17:31, Daniel Henrique Barboza wrote:
>> [E-MAIL EXTERNO] Não clique em links ou abra anexos, a menos que você possa confirmar o remetente e saber que o conteúdo é seguro. Em caso de e-mail suspeito entre imediatamente em contato com o DTI.
>>
>> This patch adds the barebones of the PMU logic by enabling cycle
>> counting, done via the performance monitor counter 6. The overall logic
>> goes as follows:
>>
>> - a helper is added to control the PMU state on each MMCR0 write. This
>> allows for the PMU to start/stop as the frozen counter bit (MMCR0_FC)
>> is cleared or set;
>>
>> - MMCR0 reg initial value is set to 0x80000000 (MMCR0_FC set) to avoid
>> having to spin the PMU right at system init;
>>
>> - the intended usage is to freeze the counters by setting MMCR0_FC, do
>> any additional setting of events to be counted via MMCR1 (not
>> implemented yet) and enable the PMU by zeroing MMCR0_FC. Software must
>> freeze counters to read the results - on the fly reading of the PMCs
>> will return the starting value of each one.
>>
>> Since there will be more PMU exclusive code to be added next, put the
>> PMU logic in its own helper to keep all in the same place. The name of
>> the new helper file, power8_pmu.c, is an indicative that the PMU logic
>> has been tested with the IBM POWER chip family, POWER8 being the oldest
>> version tested. This doesn't mean that this PMU logic will break with
>> any other PPC64 chip that implements Book3s, but since we can't assert
>> that this PMU will work with all available Book3s emulated processors
>> we're choosing to be explicit.
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
> 
> <snip>
> 
>> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
>> index 0babde3131..c3e2e3d329 100644
>> --- a/target/ppc/translate.c
>> +++ b/target/ppc/translate.c
>> @@ -401,6 +401,24 @@ void spr_write_generic(DisasContext *ctx, int sprn, int gprn)
>>       spr_store_dump_spr(sprn);
>>   }
>>
>> +#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
>> +void spr_write_MMCR0(DisasContext *ctx, int sprn, int gprn)
>> +{
>> +    /*
>> +     * 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, cpu_gpr[gprn]);
>> +}
>> +#else
>> +void spr_write_MMCR0(DisasContext *ctx, int sprn, int gprn)
>> +{
>> +    spr_write_generic(ctx, sprn, gprn);
>> +}
>> +#endif
>> +
>>   #if !defined(CONFIG_USER_ONLY)
>>   void spr_write_generic32(DisasContext *ctx, int sprn, int gprn)
>>   {
>> @@ -596,7 +614,10 @@ void spr_write_MMCR0_ureg(DisasContext *ctx, int sprn, int gprn)
>>       tcg_gen_andi_tl(t1, t1, ~(MMCR0_UREG_MASK));
>>       /* Keep all other bits intact */
>>       tcg_gen_or_tl(t1, t1, t0);
>> -    gen_store_spr(SPR_POWER_MMCR0, t1);
>> +
>> +    /* Overwrite cpu_gpr[gprn] and use spr_write_MMCR0() */
>> +    tcg_gen_mov_tl(cpu_gpr[gprn], t1);
>> +    spr_write_MMCR0(ctx, sprn + 0x10, gprn);
> 
> IIUC, this makes writing to MMCR0 change the GPR value and expose the unfiltered content of the SPR to problem state. It might be better to call the helper directly or create another method that takes a TCGv as an argument and call it from spr_write_MMCR0_ureg and spr_write_MMCR0.

I'm overwriting cpu_gpr[gprn] with t1, which is filtered by MMCR0_REG_MASK
right before, to re-use spr_write_MMCR0() since its API requires a gprn
index. The reason I'm re-using spr_write_MMCR0() here is to avoid code repetition
in spr_write_MMCR0_ureg(), which would need to repeat the same steps as
spr_write_MMCR0 (calling icount_io_start(), calling the helper, and then setting
DISAS_EXIT_UPDATE in a later patch).

The idea behind is that all PMU user_write() functions works the same as its
privileged counterparts but with some form of filtering done beforehand. Note
that this is kind of true in the previous patch as well - gen_store_spr() is
similar to the privileged function MMCR0 was using (spr_write_generic()) with
the exception of an optional qemu_log().

Maybe I should've made this clear in the previous patch, using spr_write_generic()
and overwriting cpu_gpr[gprn] with the filtered t1 content back there.

Speaking of which, since t1 is being filtered by MMCR0_REG_MASK before being used to
overwrite cpu_gpr[gprn], I'm not sure how this is exposing unfiltered content to
problem state. Can you elaborate?



Thanks,


Daniel

> 
>>
>>       tcg_temp_free(t0);
>>       tcg_temp_free(t1);
>> -- 
>> 2.31.1
>>
> 
> 

Re: [PATCH v3 03/15] target/ppc: PMU basic cycle count for pseries TCG

[Matheus K. Ferst]

On 24/09/2021 11:41, Daniel Henrique Barboza wrote:
> On 9/22/21 08:24, Matheus K. Ferst wrote:
>> On 03/09/2021 17:31, Daniel Henrique Barboza wrote:
>>> [E-MAIL EXTERNO] Não clique em links ou abra anexos, a menos que você 
>>> possa confirmar o remetente e saber que o conteúdo é seguro. Em caso 
>>> de e-mail suspeito entre imediatamente em contato com o DTI.
>>>
>>> This patch adds the barebones of the PMU logic by enabling cycle
>>> counting, done via the performance monitor counter 6. The overall logic
>>> goes as follows:
>>>
>>> - a helper is added to control the PMU state on each MMCR0 write. This
>>> allows for the PMU to start/stop as the frozen counter bit (MMCR0_FC)
>>> is cleared or set;
>>>
>>> - MMCR0 reg initial value is set to 0x80000000 (MMCR0_FC set) to avoid
>>> having to spin the PMU right at system init;
>>>
>>> - the intended usage is to freeze the counters by setting MMCR0_FC, do
>>> any additional setting of events to be counted via MMCR1 (not
>>> implemented yet) and enable the PMU by zeroing MMCR0_FC. Software must
>>> freeze counters to read the results - on the fly reading of the PMCs
>>> will return the starting value of each one.
>>>
>>> Since there will be more PMU exclusive code to be added next, put the
>>> PMU logic in its own helper to keep all in the same place. The name of
>>> the new helper file, power8_pmu.c, is an indicative that the PMU logic
>>> has been tested with the IBM POWER chip family, POWER8 being the oldest
>>> version tested. This doesn't mean that this PMU logic will break with
>>> any other PPC64 chip that implements Book3s, but since we can't assert
>>> that this PMU will work with all available Book3s emulated processors
>>> we're choosing to be explicit.
>>>
>>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>> ---
>>
>> <snip>
>>
>>> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
>>> index 0babde3131..c3e2e3d329 100644
>>> --- a/target/ppc/translate.c
>>> +++ b/target/ppc/translate.c
>>> @@ -401,6 +401,24 @@ void spr_write_generic(DisasContext *ctx, int 
>>> sprn, int gprn)
>>>       spr_store_dump_spr(sprn);
>>>   }
>>>
>>> +#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
>>> +void spr_write_MMCR0(DisasContext *ctx, int sprn, int gprn)
>>> +{
>>> +    /*
>>> +     * 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, cpu_gpr[gprn]);
>>> +}
>>> +#else
>>> +void spr_write_MMCR0(DisasContext *ctx, int sprn, int gprn)
>>> +{
>>> +    spr_write_generic(ctx, sprn, gprn);
>>> +}
>>> +#endif
>>> +
>>>   #if !defined(CONFIG_USER_ONLY)
>>>   void spr_write_generic32(DisasContext *ctx, int sprn, int gprn)
>>>   {
>>> @@ -596,7 +614,10 @@ void spr_write_MMCR0_ureg(DisasContext *ctx, int 
>>> sprn, int gprn)
>>>       tcg_gen_andi_tl(t1, t1, ~(MMCR0_UREG_MASK));
>>>       /* Keep all other bits intact */
>>>       tcg_gen_or_tl(t1, t1, t0);
>>> -    gen_store_spr(SPR_POWER_MMCR0, t1);
>>> +
>>> +    /* Overwrite cpu_gpr[gprn] and use spr_write_MMCR0() */
>>> +    tcg_gen_mov_tl(cpu_gpr[gprn], t1);
>>> +    spr_write_MMCR0(ctx, sprn + 0x10, gprn);
>>
>> IIUC, this makes writing to MMCR0 change the GPR value and expose the 
>> unfiltered content of the SPR to problem state. It might be better to 
>> call the helper directly or create another method that takes a TCGv as 
>> an argument and call it from spr_write_MMCR0_ureg and spr_write_MMCR0.
> 
> I'm overwriting cpu_gpr[gprn] with t1, which is filtered by MMCR0_REG_MASK
> right before, to re-use spr_write_MMCR0() since its API requires a gprn
> index. The reason I'm re-using spr_write_MMCR0() here is to avoid code 
> repetition
> in spr_write_MMCR0_ureg(), which would need to repeat the same steps as
> spr_write_MMCR0 (calling icount_io_start(), calling the helper, and then 
> setting
> DISAS_EXIT_UPDATE in a later patch).
> 
> The idea behind is that all PMU user_write() functions works the same as 
> its
> privileged counterparts but with some form of filtering done beforehand. 
> Note
> that this is kind of true in the previous patch as well - 
> gen_store_spr() is
> similar to the privileged function MMCR0 was using (spr_write_generic()) 
> with
> the exception of an optional qemu_log().
> 
> Maybe I should've made this clear in the previous patch, using 
> spr_write_generic()
> and overwriting cpu_gpr[gprn] with the filtered t1 content back there.
> 
> Speaking of which, since t1 is being filtered by MMCR0_REG_MASK before 
> being used to
> overwrite cpu_gpr[gprn], I'm not sure how this is exposing unfiltered 
> content to
> problem state. Can you elaborate?

Suppose MMCR0 has the value 0x80000001 (FC and FCH) and problem state 
executes an mtspr with the value 0x4000000 (unset FC and set PMAE) in 
the GPR. The proposed code will do the following:

 > tcg_gen_andi_tl(t0, cpu_gpr[gprn], MMCR0_UREG_MASK);

t0 = GPR & MMCR0_UREG_MASK = 0x4000000 & 0x84000080 = 0x4000000

 > gen_load_spr(t1, SPR_POWER_MMCR0);

t1 = MMCR0 = 0x80000001

 > tcg_gen_andi_tl(t1, t1, ~(MMCR0_UREG_MASK));

t1 = t1 & ~MMCR0_UREG_MASK = 0x80000001 & ~0x84000080 = 0x1

 > tcg_gen_or_tl(t1, t1, t0);

t1 = t1 | t0 = 0x4000000 | 0x1 = 0x4000001

 > tcg_gen_mov_tl(cpu_gpr[gprn], t1);

GPR = 0x4000001

Now problem state knows that FCH is set.

-- 
Matheus K. Ferst
Instituto de Pesquisas ELDORADO <http://www.eldorado.org.br/>
Analista de Software Júnior
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>

Re: [PATCH v3 03/15] target/ppc: PMU basic cycle count for pseries TCG

[Daniel Henrique Barboza]

On 9/24/21 15:34, Matheus K. Ferst wrote:
> On 24/09/2021 11:41, Daniel Henrique Barboza wrote:
>> On 9/22/21 08:24, Matheus K. Ferst wrote:
>>> On 03/09/2021 17:31, Daniel Henrique Barboza wrote:
>>>> [E-MAIL EXTERNO] Não clique em links ou abra anexos, a menos que você possa confirmar o remetente e saber que o conteúdo é seguro. Em caso de e-mail suspeito entre imediatamente em contato com o DTI.
>>>>
>>>> This patch adds the barebones of the PMU logic by enabling cycle
>>>> counting, done via the performance monitor counter 6. The overall logic
>>>> goes as follows:
>>>>
>>>> - a helper is added to control the PMU state on each MMCR0 write. This
>>>> allows for the PMU to start/stop as the frozen counter bit (MMCR0_FC)
>>>> is cleared or set;
>>>>
>>>> - MMCR0 reg initial value is set to 0x80000000 (MMCR0_FC set) to avoid
>>>> having to spin the PMU right at system init;
>>>>
>>>> - the intended usage is to freeze the counters by setting MMCR0_FC, do
>>>> any additional setting of events to be counted via MMCR1 (not
>>>> implemented yet) and enable the PMU by zeroing MMCR0_FC. Software must
>>>> freeze counters to read the results - on the fly reading of the PMCs
>>>> will return the starting value of each one.
>>>>
>>>> Since there will be more PMU exclusive code to be added next, put the
>>>> PMU logic in its own helper to keep all in the same place. The name of
>>>> the new helper file, power8_pmu.c, is an indicative that the PMU logic
>>>> has been tested with the IBM POWER chip family, POWER8 being the oldest
>>>> version tested. This doesn't mean that this PMU logic will break with
>>>> any other PPC64 chip that implements Book3s, but since we can't assert
>>>> that this PMU will work with all available Book3s emulated processors
>>>> we're choosing to be explicit.
>>>>
>>>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>>> ---
>>>
>>> <snip>
>>>
>>>> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
>>>> index 0babde3131..c3e2e3d329 100644
>>>> --- a/target/ppc/translate.c
>>>> +++ b/target/ppc/translate.c
>>>> @@ -401,6 +401,24 @@ void spr_write_generic(DisasContext *ctx, int sprn, int gprn)
>>>>       spr_store_dump_spr(sprn);
>>>>   }
>>>>
>>>> +#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
>>>> +void spr_write_MMCR0(DisasContext *ctx, int sprn, int gprn)
>>>> +{
>>>> +    /*
>>>> +     * 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, cpu_gpr[gprn]);
>>>> +}
>>>> +#else
>>>> +void spr_write_MMCR0(DisasContext *ctx, int sprn, int gprn)
>>>> +{
>>>> +    spr_write_generic(ctx, sprn, gprn);
>>>> +}
>>>> +#endif
>>>> +
>>>>   #if !defined(CONFIG_USER_ONLY)
>>>>   void spr_write_generic32(DisasContext *ctx, int sprn, int gprn)
>>>>   {
>>>> @@ -596,7 +614,10 @@ void spr_write_MMCR0_ureg(DisasContext *ctx, int sprn, int gprn)
>>>>       tcg_gen_andi_tl(t1, t1, ~(MMCR0_UREG_MASK));
>>>>       /* Keep all other bits intact */
>>>>       tcg_gen_or_tl(t1, t1, t0);
>>>> -    gen_store_spr(SPR_POWER_MMCR0, t1);
>>>> +
>>>> +    /* Overwrite cpu_gpr[gprn] and use spr_write_MMCR0() */
>>>> +    tcg_gen_mov_tl(cpu_gpr[gprn], t1);
>>>> +    spr_write_MMCR0(ctx, sprn + 0x10, gprn);
>>>
>>> IIUC, this makes writing to MMCR0 change the GPR value and expose the unfiltered content of the SPR to problem state. It might be better to call the helper directly or create another method that takes a TCGv as an argument and call it from spr_write_MMCR0_ureg and spr_write_MMCR0.
>>
>> I'm overwriting cpu_gpr[gprn] with t1, which is filtered by MMCR0_REG_MASK
>> right before, to re-use spr_write_MMCR0() since its API requires a gprn
>> index. The reason I'm re-using spr_write_MMCR0() here is to avoid code repetition
>> in spr_write_MMCR0_ureg(), which would need to repeat the same steps as
>> spr_write_MMCR0 (calling icount_io_start(), calling the helper, and then setting
>> DISAS_EXIT_UPDATE in a later patch).
>>
>> The idea behind is that all PMU user_write() functions works the same as its
>> privileged counterparts but with some form of filtering done beforehand. Note
>> that this is kind of true in the previous patch as well - gen_store_spr() is
>> similar to the privileged function MMCR0 was using (spr_write_generic()) with
>> the exception of an optional qemu_log().
>>
>> Maybe I should've made this clear in the previous patch, using spr_write_generic()
>> and overwriting cpu_gpr[gprn] with the filtered t1 content back there.
>>
>> Speaking of which, since t1 is being filtered by MMCR0_REG_MASK before being used to
>> overwrite cpu_gpr[gprn], I'm not sure how this is exposing unfiltered content to
>> problem state. Can you elaborate?
> 
> Suppose MMCR0 has the value 0x80000001 (FC and FCH) and problem state executes an mtspr with the value 0x4000000 (unset FC and set PMAE) in the GPR. The proposed code will do the following:
> 
>  > tcg_gen_andi_tl(t0, cpu_gpr[gprn], MMCR0_UREG_MASK);
> 
> t0 = GPR & MMCR0_UREG_MASK = 0x4000000 & 0x84000080 = 0x4000000
> 
>  > gen_load_spr(t1, SPR_POWER_MMCR0);
> 
> t1 = MMCR0 = 0x80000001
> 
>  > tcg_gen_andi_tl(t1, t1, ~(MMCR0_UREG_MASK));
> 
> t1 = t1 & ~MMCR0_UREG_MASK = 0x80000001 & ~0x84000080 = 0x1
> 
>  > tcg_gen_or_tl(t1, t1, t0);
> 
> t1 = t1 | t0 = 0x4000000 | 0x1 = 0x4000001
> 
>  > tcg_gen_mov_tl(cpu_gpr[gprn], t1);
> 
> GPR = 0x4000001
> 
> Now problem state knows that FCH is set.

I see. The problem is that overwriting the GPR is exposing bits outside
of the MMCR0_UREG_MASK via GPR itself, something that wasn't happening
in the previous patch because the filtering logic wasn't visible via
userspace.

Thanks for clarifying. I'll fix it in the next version, probably by adding a
common 'write_MMCR0' method that receives a TCGv and that can be shared
for both privileged and user write() callbacks, like you suggested in your
previous reply.



Thanks,


Daniel


> 

Re: [PATCH v3 05/15] target/ppc: PMU: add instruction counting

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 13309 bytes --]

On Tue, Sep 21, 2021 at 06:11:24PM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 9/6/21 22:57, David Gibson wrote:
> > On Fri, Sep 03, 2021 at 05:31:06PM -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 later on.
> > > 
> > > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> > > ---
> > >   target/ppc/cpu.h         |  1 +
> > >   target/ppc/helper.h      |  1 +
> > >   target/ppc/helper_regs.c |  3 ++
> > >   target/ppc/power8_pmu.c  | 70 ++++++++++++++++++++++++++++++++++++----
> > >   target/ppc/translate.c   | 46 ++++++++++++++++++++++++++
> > >   5 files changed, 114 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> > > index 74698a3600..4d4886ac74 100644
> > > --- a/target/ppc/cpu.h
> > > +++ b/target/ppc/cpu.h
> > > @@ -628,6 +628,7 @@ enum {
> > >       HFLAGS_FP = 13,  /* MSR_FP */
> > >       HFLAGS_PR = 14,  /* MSR_PR */
> > >       HFLAGS_PMCCCLEAR = 15, /* PMU MMCR0 PMCC equal to 0b00 */
> > > +    HFLAGS_MMCR0FC = 16, /* 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 5122632784..47dbbe6da1 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(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 4c1d9575ac..27d139edd8 100644
> > > --- a/target/ppc/helper_regs.c
> > > +++ b/target/ppc/helper_regs.c
> > > @@ -109,6 +109,9 @@ static uint32_t hreg_compute_hflags_value(CPUPPCState *env)
> > >       if (((env->spr[SPR_POWER_MMCR0] & MMCR0_PMCC) >> 18) == 0) {
> > >           hflags |= 1 << HFLAGS_PMCCCLEAR;
> > >       }
> > > +    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.c b/target/ppc/power8_pmu.c
> > > index 3f7b305f4f..9769c0ff35 100644
> > > --- a/target/ppc/power8_pmu.c
> > > +++ b/target/ppc/power8_pmu.c
> > > @@ -31,10 +31,13 @@ static void update_PMC_PM_CYC(CPUPPCState *env, int sprn,
> > >       env->spr[sprn] += time_delta;
> > >   }
> > > -static void update_programmable_PMC_reg(CPUPPCState *env, int sprn,
> > > -                                        uint64_t time_delta)
> > > +static uint8_t get_PMC_event(CPUPPCState *env, int sprn)
> > 
> > I like the idea of splitting out a helper to get the selected event
> > (might even make sense to move that to the earlier patch).  What would
> > be even nicer is if it also included handling of the fact that some
> > events are specific to particular PMCs (like 0xF0 for PMC1).  That
> > means that all the event selection logic will be here, rather than
> > having to check the PMC number again in the caller.  Obviously to do
> > that you'll need some special "bad event" return value, which might
> > mean changing the return type.
> 
> The initial idea of this function was to be a simple event extractor
> returning 0 if MMCR1 is blank. In the end of the series there are 3 callers
> of this function that will execute a specific action based on the event
> returned by it.
> 
> I suppose that we can use the return value 0 as a 'bad value' based on the
> PMC events we're going to support, but that will not prevent the callers
> from doing a 'switch()' like logic, rechecking the PMC, to see which action
> is supposed to be taken.
> 
> IIUC, your idea would require an additional layer of abstraction, e.g. a
> PMCEvent object, that would tie together PMC + event. Then get_PMC_event()
> would return a PMCEvent object and the caller wouldn't need to re-check the
> PMC again.

More-or-less, yes.  I don't think the "object" would need to be
anything more than an enum though: "cycles", "instructions" or "invalid".

> I'll see how hard it would be to introduce this new concept in this existing
> series. If it ends up being too much rework I'll suggest to do this in a
> follow-up.
> 
> 
> 
> Thanks,
> 
> 
> Daniel
> 
> > 
> > >   {
> > > -    uint8_t event, evt_extr;
> > > +    uint8_t evt_extr = 0;
> > > +
> > > +    if (env->spr[SPR_POWER_MMCR1] == 0) {
> > > +        return 0;
> > > +    }
> > >       switch (sprn) {
> > >       case SPR_POWER_PMC1:
> > > @@ -50,10 +53,16 @@ static void update_programmable_PMC_reg(CPUPPCState *env, int sprn,
> > >           evt_extr = MMCR1_PMC4EVT_EXTR;
> > >           break;
> > >       default:
> > > -        return;
> > > +        return 0;
> > >       }
> > > -    event = extract64(env->spr[SPR_POWER_MMCR1], evt_extr, MMCR1_EVT_SIZE);
> > > +    return extract64(env->spr[SPR_POWER_MMCR1], evt_extr, MMCR1_EVT_SIZE);
> > > +}
> > > +
> > > +static void update_programmable_PMC_reg(CPUPPCState *env, int sprn,
> > > +                                        uint64_t time_delta)
> > > +{
> > > +    uint8_t event = get_PMC_event(env, sprn);
> > >       /*
> > >        * MMCR0_PMC1SEL = 0xF0 is the architected PowerISA v3.1 event
> > > @@ -99,8 +108,9 @@ void helper_store_mmcr0(CPUPPCState *env, target_ulong value)
> > >       env->spr[SPR_POWER_MMCR0] = value;
> > > -    /* MMCR0 writes can change HFLAGS_PMCCCLEAR */
> > > -    if ((curr_value & MMCR0_PMCC) != (value & MMCR0_PMCC)) {
> > > +    /* MMCR0 writes can change HFLAGS_PMCCCLEAR and HFLAGS_MMCR0FC */
> > > +    if (((curr_value & MMCR0_PMCC) != (value & MMCR0_PMCC)) ||
> > > +        (curr_FC != new_FC)) {
> > >           hreg_compute_hflags(env);
> > >       }
> > > @@ -123,4 +133,50 @@ void helper_store_mmcr0(CPUPPCState *env, target_ulong value)
> > >       }
> > >   }
> > > +static bool pmc_counting_insns(CPUPPCState *env, int sprn)
> > > +{
> > > +    bool ret = false;
> > > +    uint8_t event;
> > > +
> > > +    if (sprn == SPR_POWER_PMC5) {
> > > +        return true;
> > > +    }
> > > +
> > > +    event = get_PMC_event(env, sprn);
> > > +
> > > +    /*
> > > +     * 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
> > > +     * all programmable PMCs.
> > > +     *
> > > +     * Event 0xFE is the PowerISA v3.1 architected event to
> > > +     * sample PM_INST_CMPL using PMC1.
> > > +     */
> > > +    switch (sprn) {
> > > +    case SPR_POWER_PMC1:
> > > +        return event == 0x2 || event == 0xFE;
> > > +    case SPR_POWER_PMC2:
> > > +    case SPR_POWER_PMC3:
> > > +    case SPR_POWER_PMC4:
> > > +        return event == 0x2;
> > > +    default:
> > > +        break;
> > > +    }
> > > +
> > > +    return ret;
> > > +}
> > > +
> > > +/* This helper assumes that the PMC is running. */
> > > +void helper_insns_inc(CPUPPCState *env, uint32_t num_insns)
> > > +{
> > > +    int sprn;
> > > +
> > > +    for (sprn = SPR_POWER_PMC1; sprn <= SPR_POWER_PMC5; sprn++) {
> > > +        if (pmc_counting_insns(env, sprn)) {
> > > +            env->spr[sprn] += num_insns;
> > > +        }
> > > +    }
> > > +}
> > > +
> > >   #endif /* defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY) */
> > > diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> > > index c3e2e3d329..b7235a2be0 100644
> > > --- a/target/ppc/translate.c
> > > +++ b/target/ppc/translate.c
> > > @@ -176,6 +176,7 @@ struct DisasContext {
> > >       bool tm_enabled;
> > >       bool gtse;
> > >       bool pmcc_clear;
> > > +    bool pmu_frozen;
> > >       ppc_spr_t *spr_cb; /* Needed to check rights for mfspr/mtspr */
> > >       int singlestep_enabled;
> > >       uint32_t flags;
> > > @@ -411,6 +412,12 @@ void spr_write_MMCR0(DisasContext *ctx, int sprn, int gprn)
> > >        */
> > >       gen_icount_io_start(ctx);
> > >       gen_helper_store_mmcr0(cpu_env, cpu_gpr[gprn]);
> > > +
> > > +    /*
> > > +     * End the translation block because MMCR0 writes can change
> > > +     * ctx->pmu_frozen.
> > > +     */
> > > +    ctx->base.is_jmp = DISAS_EXIT_UPDATE;
> > >   }
> > >   #else
> > >   void spr_write_MMCR0(DisasContext *ctx, int sprn, int gprn)
> > > @@ -4407,6 +4414,22 @@ 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;
> > > +    }
> > > +
> > > +    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);
> > > @@ -4421,9 +4444,17 @@ static void gen_lookup_and_goto_ptr(DisasContext *ctx)
> > >           } else if (sse & (CPU_SINGLE_STEP | CPU_BRANCH_STEP)) {
> > >               gen_helper_raise_exception(cpu_env, tcg_constant_i32(gen_prep_dbgex(ctx)));
> > >           } else {
> > > +            pmu_count_insns(ctx);
> > >               tcg_gen_exit_tb(NULL, 0);
> > >           }
> > >       } 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();
> > >       }
> > >   }
> > > @@ -4435,6 +4466,8 @@ 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);
> > > @@ -8648,6 +8681,7 @@ static void ppc_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
> > >       ctx->tm_enabled = (hflags >> HFLAGS_TM) & 1;
> > >       ctx->gtse = (hflags >> HFLAGS_GTSE) & 1;
> > >       ctx->pmcc_clear = (hflags >> HFLAGS_PMCCCLEAR) & 1;
> > > +    ctx->pmu_frozen = (hflags >> HFLAGS_MMCR0FC) & 1;
> > >       ctx->singlestep_enabled = 0;
> > >       if ((hflags >> HFLAGS_SE) & 1) {
> > > @@ -8767,6 +8801,8 @@ 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);
> > > @@ -8777,6 +8813,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;
> > > @@ -8784,6 +8828,8 @@ 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 --]

Re: [PATCH v3 03/15] target/ppc: PMU basic cycle count for pseries TCG

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 7185 bytes --]

On Fri, Sep 24, 2021 at 04:05:37PM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 9/24/21 15:34, Matheus K. Ferst wrote:
> > On 24/09/2021 11:41, Daniel Henrique Barboza wrote:
> > > On 9/22/21 08:24, Matheus K. Ferst wrote:
> > > > On 03/09/2021 17:31, Daniel Henrique Barboza wrote:
> > > > > [E-MAIL EXTERNO] Não clique em links ou abra anexos, a menos que você possa confirmar o remetente e saber que o conteúdo é seguro. Em caso de e-mail suspeito entre imediatamente em contato com o DTI.
> > > > > 
> > > > > This patch adds the barebones of the PMU logic by enabling cycle
> > > > > counting, done via the performance monitor counter 6. The overall logic
> > > > > goes as follows:
> > > > > 
> > > > > - a helper is added to control the PMU state on each MMCR0 write. This
> > > > > allows for the PMU to start/stop as the frozen counter bit (MMCR0_FC)
> > > > > is cleared or set;
> > > > > 
> > > > > - MMCR0 reg initial value is set to 0x80000000 (MMCR0_FC set) to avoid
> > > > > having to spin the PMU right at system init;
> > > > > 
> > > > > - the intended usage is to freeze the counters by setting MMCR0_FC, do
> > > > > any additional setting of events to be counted via MMCR1 (not
> > > > > implemented yet) and enable the PMU by zeroing MMCR0_FC. Software must
> > > > > freeze counters to read the results - on the fly reading of the PMCs
> > > > > will return the starting value of each one.
> > > > > 
> > > > > Since there will be more PMU exclusive code to be added next, put the
> > > > > PMU logic in its own helper to keep all in the same place. The name of
> > > > > the new helper file, power8_pmu.c, is an indicative that the PMU logic
> > > > > has been tested with the IBM POWER chip family, POWER8 being the oldest
> > > > > version tested. This doesn't mean that this PMU logic will break with
> > > > > any other PPC64 chip that implements Book3s, but since we can't assert
> > > > > that this PMU will work with all available Book3s emulated processors
> > > > > we're choosing to be explicit.
> > > > > 
> > > > > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> > > > > ---
> > > > 
> > > > <snip>
> > > > 
> > > > > diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> > > > > index 0babde3131..c3e2e3d329 100644
> > > > > --- a/target/ppc/translate.c
> > > > > +++ b/target/ppc/translate.c
> > > > > @@ -401,6 +401,24 @@ void spr_write_generic(DisasContext *ctx, int sprn, int gprn)
> > > > >       spr_store_dump_spr(sprn);
> > > > >   }
> > > > > 
> > > > > +#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
> > > > > +void spr_write_MMCR0(DisasContext *ctx, int sprn, int gprn)
> > > > > +{
> > > > > +    /*
> > > > > +     * 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, cpu_gpr[gprn]);
> > > > > +}
> > > > > +#else
> > > > > +void spr_write_MMCR0(DisasContext *ctx, int sprn, int gprn)
> > > > > +{
> > > > > +    spr_write_generic(ctx, sprn, gprn);
> > > > > +}
> > > > > +#endif
> > > > > +
> > > > >   #if !defined(CONFIG_USER_ONLY)
> > > > >   void spr_write_generic32(DisasContext *ctx, int sprn, int gprn)
> > > > >   {
> > > > > @@ -596,7 +614,10 @@ void spr_write_MMCR0_ureg(DisasContext *ctx, int sprn, int gprn)
> > > > >       tcg_gen_andi_tl(t1, t1, ~(MMCR0_UREG_MASK));
> > > > >       /* Keep all other bits intact */
> > > > >       tcg_gen_or_tl(t1, t1, t0);
> > > > > -    gen_store_spr(SPR_POWER_MMCR0, t1);
> > > > > +
> > > > > +    /* Overwrite cpu_gpr[gprn] and use spr_write_MMCR0() */
> > > > > +    tcg_gen_mov_tl(cpu_gpr[gprn], t1);
> > > > > +    spr_write_MMCR0(ctx, sprn + 0x10, gprn);
> > > > 
> > > > IIUC, this makes writing to MMCR0 change the GPR value and expose the unfiltered content of the SPR to problem state. It might be better to call the helper directly or create another method that takes a TCGv as an argument and call it from spr_write_MMCR0_ureg and spr_write_MMCR0.
> > > 
> > > I'm overwriting cpu_gpr[gprn] with t1, which is filtered by MMCR0_REG_MASK
> > > right before, to re-use spr_write_MMCR0() since its API requires a gprn
> > > index. The reason I'm re-using spr_write_MMCR0() here is to avoid code repetition
> > > in spr_write_MMCR0_ureg(), which would need to repeat the same steps as
> > > spr_write_MMCR0 (calling icount_io_start(), calling the helper, and then setting
> > > DISAS_EXIT_UPDATE in a later patch).
> > > 
> > > The idea behind is that all PMU user_write() functions works the same as its
> > > privileged counterparts but with some form of filtering done beforehand. Note
> > > that this is kind of true in the previous patch as well - gen_store_spr() is
> > > similar to the privileged function MMCR0 was using (spr_write_generic()) with
> > > the exception of an optional qemu_log().
> > > 
> > > Maybe I should've made this clear in the previous patch, using spr_write_generic()
> > > and overwriting cpu_gpr[gprn] with the filtered t1 content back there.
> > > 
> > > Speaking of which, since t1 is being filtered by MMCR0_REG_MASK before being used to
> > > overwrite cpu_gpr[gprn], I'm not sure how this is exposing unfiltered content to
> > > problem state. Can you elaborate?
> > 
> > Suppose MMCR0 has the value 0x80000001 (FC and FCH) and problem state executes an mtspr with the value 0x4000000 (unset FC and set PMAE) in the GPR. The proposed code will do the following:
> > 
> >  > tcg_gen_andi_tl(t0, cpu_gpr[gprn], MMCR0_UREG_MASK);
> > 
> > t0 = GPR & MMCR0_UREG_MASK = 0x4000000 & 0x84000080 = 0x4000000
> > 
> >  > gen_load_spr(t1, SPR_POWER_MMCR0);
> > 
> > t1 = MMCR0 = 0x80000001
> > 
> >  > tcg_gen_andi_tl(t1, t1, ~(MMCR0_UREG_MASK));
> > 
> > t1 = t1 & ~MMCR0_UREG_MASK = 0x80000001 & ~0x84000080 = 0x1
> > 
> >  > tcg_gen_or_tl(t1, t1, t0);
> > 
> > t1 = t1 | t0 = 0x4000000 | 0x1 = 0x4000001
> > 
> >  > tcg_gen_mov_tl(cpu_gpr[gprn], t1);
> > 
> > GPR = 0x4000001
> > 
> > Now problem state knows that FCH is set.

Nice catch Matheus.

> I see. The problem is that overwriting the GPR is exposing bits outside
> of the MMCR0_UREG_MASK via GPR itself, something that wasn't happening
> in the previous patch because the filtering logic wasn't visible via
> userspace.

Right.  Note that even if it wasn't exposing privileged bits, I don't
think changing the GPR value would be correct behaviour, although I
suspect it would be unlikely to cause problems in practice.

> Thanks for clarifying. I'll fix it in the next version, probably by adding a
> common 'write_MMCR0' method that receives a TCGv and that can be shared
> for both privileged and user write() callbacks, like you suggested in your
> previous reply.

-- 
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 --]

Re: [PATCH v3 02/15] target/ppc: add user write access control for PMU SPRs

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 6070 bytes --]

On Thu, Sep 23, 2021 at 11:39:14AM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 9/6/21 22:38, David Gibson wrote:
> > On Fri, Sep 03, 2021 at 05:31:03PM -0300, Daniel Henrique Barboza wrote:
> > > The PMU needs to enable writing of its uregs to userspace, otherwise
> > > Perf applications will not able to setup the counters correctly. This
> > > patch enables user space writing of all PMU uregs.
> > > 
> > > MMCR0 is a special case because its userspace writing access is controlled
> > > by MMCR0_PMCC bits. There are 4 configurations available (0b00, 0b01,
> > > 0b10 and 0b11) but for our purposes here we're handling only
> > > MMCR0_PMCC = 0b00. In this case, if userspace tries to write MMCR0, a
> > > hypervisor emulation assistance interrupt occurs.
> > > 
> > > This is being done by adding HFLAGS_PMCCCLEAR to hflags. This flag
> > > indicates if MMCR0_PMCC is cleared (0b00), and a new 'pmcc_clear' flag in
> > > DisasContext allow us to use it in spr_write_MMCR0_ureg().
> > > 
> > > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> > > ---
> > >   target/ppc/cpu.h         |  1 +
> > >   target/ppc/cpu_init.c    | 18 +++++++-------
> > >   target/ppc/helper_regs.c |  3 +++
> > >   target/ppc/spr_tcg.h     |  3 ++-
> > >   target/ppc/translate.c   | 53 +++++++++++++++++++++++++++++++++++++++-
> > >   5 files changed, 67 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> > > index f68bb8d8aa..8dfbb62022 100644
> > > --- a/target/ppc/cpu.h
> > > +++ b/target/ppc/cpu.h
> > > @@ -616,6 +616,7 @@ enum {
> > >       HFLAGS_SE = 10,  /* MSR_SE -- from elsewhere on embedded ppc */
> > >       HFLAGS_FP = 13,  /* MSR_FP */
> > >       HFLAGS_PR = 14,  /* MSR_PR */
> > > +    HFLAGS_PMCCCLEAR = 15, /* PMU MMCR0 PMCC equal to 0b00 */
> > >       HFLAGS_VSX = 23, /* MSR_VSX if cpu has VSX */
> > >       HFLAGS_VR = 25,  /* MSR_VR if cpu has VRE */
> > > diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> > > index 9efc6c2d87..bb5ea04c61 100644
> > > --- a/target/ppc/cpu_init.c
> > > +++ b/target/ppc/cpu_init.c
> > > @@ -6867,7 +6867,7 @@ static void register_book3s_pmu_sup_sprs(CPUPPCState *env)
> > >   static void register_book3s_pmu_user_sprs(CPUPPCState *env)
> > >   {
> > >       spr_register(env, SPR_POWER_UMMCR0, "UMMCR0",
> > > -                 &spr_read_MMCR0_ureg, SPR_NOACCESS,
> > > +                 &spr_read_MMCR0_ureg, &spr_write_MMCR0_ureg,
> > >                    &spr_read_ureg, &spr_write_ureg,
> > >                    0x00000000);
> > >       spr_register(env, SPR_POWER_UMMCR1, "UMMCR1",
> > > @@ -6875,31 +6875,31 @@ static void register_book3s_pmu_user_sprs(CPUPPCState *env)
> > >                    &spr_read_ureg, &spr_write_ureg,
> > >                    0x00000000);
> > >       spr_register(env, SPR_POWER_UMMCRA, "UMMCRA",
> > > -                 &spr_read_ureg, SPR_NOACCESS,
> > > +                 &spr_read_ureg, &spr_write_ureg,
> > >                    &spr_read_ureg, &spr_write_ureg,
> > >                    0x00000000);
> > >       spr_register(env, SPR_POWER_UPMC1, "UPMC1",
> > > -                 &spr_read_ureg, SPR_NOACCESS,
> > > +                 &spr_read_ureg, &spr_write_ureg,
> > 
> > Surely this can't be write.  AFAICT spr_write_ureg() will
> > unconditionally allow full userspace write access.  That can't be
> > right - otherwise the OS could never safely use the PMU for itself.
> 
> My assumption here was that the user mode SPRs (UMMCR* and UPMC*) were created to
> allow userspace read/write of PMU regs, while the regular regs (MMCR* and PMC*)
> are the supermode privileged SPRs that can't be written by userspace. At least this
> is my understanding from reading commit fd51ff6328e3d98158 that introduced these
> userspace PMC regs.

Sure, but my point is that these registers are only userspace
accessible under certain conditions, IIUC.  spr_write_ureg() doesn't
test for those conditions, so it will *always* allow write access.

> The reason why these are marked as SPR_NOACCESS is because we didn't bothered
> writing into them from userspace because we had no PMU logic to work
> with.

[snip]
> > > diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> > > index b2ead144d1..0babde3131 100644
> > > --- a/target/ppc/translate.c
> > > +++ b/target/ppc/translate.c
> > > @@ -175,6 +175,7 @@ struct DisasContext {
> > >       bool spe_enabled;
> > >       bool tm_enabled;
> > >       bool gtse;
> > > +    bool pmcc_clear;
> > >       ppc_spr_t *spr_cb; /* Needed to check rights for mfspr/mtspr */
> > >       int singlestep_enabled;
> > >       uint32_t flags;
> > > @@ -561,7 +562,56 @@ void spr_write_ureg(DisasContext *ctx, int sprn, int gprn)
> > >   {
> > >       gen_store_spr(sprn + 0x10, cpu_gpr[gprn]);
> > >   }
> > > -#endif
> > > +
> > > +void spr_write_MMCR0_ureg(DisasContext *ctx, int sprn, int gprn)
> > 
> > 
> > Could you put this def in the PMU specific file, rather than the
> > enormous translate.c?
> 
> Moving into the existing power8_pmu.c helper is annoying because, being a helper file,
> there is no access to the whole DisasContext declaration (that is open coded in
> translate.c), and other internal translate.c data like cpu_grp[].

Ah, right.  We should probably make that easier someday, but it's not
reasonbly in scope for this series.

> What I was able to do is create a new file in the target/ppc/translate/ dir,
> power8-pmu-regs.c.impl, and moved all these declarations over there. At very least we're
> not overloading translate.c.

Ah, nice.

> Eldorado, is that ok with you guys? I'm aware that this dir was holding new
> decode-tree insns implementations but, in this case, it would hold old format
> spr_read/spr_write code.

-- 
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 --]

Re: [PATCH v3 02/15] target/ppc: add user write access control for PMU SPRs

[Daniel Henrique Barboza]

On 9/27/21 02:08, David Gibson wrote:
> On Thu, Sep 23, 2021 at 11:39:14AM -0300, Daniel Henrique Barboza wrote:
>>
>>
>> On 9/6/21 22:38, David Gibson wrote:
>>> On Fri, Sep 03, 2021 at 05:31:03PM -0300, Daniel Henrique Barboza wrote:
>>>> The PMU needs to enable writing of its uregs to userspace, otherwise
>>>> Perf applications will not able to setup the counters correctly. This
>>>> patch enables user space writing of all PMU uregs.
>>>>
>>>> MMCR0 is a special case because its userspace writing access is controlled
>>>> by MMCR0_PMCC bits. There are 4 configurations available (0b00, 0b01,
>>>> 0b10 and 0b11) but for our purposes here we're handling only
>>>> MMCR0_PMCC = 0b00. In this case, if userspace tries to write MMCR0, a
>>>> hypervisor emulation assistance interrupt occurs.
>>>>
>>>> This is being done by adding HFLAGS_PMCCCLEAR to hflags. This flag
>>>> indicates if MMCR0_PMCC is cleared (0b00), and a new 'pmcc_clear' flag in
>>>> DisasContext allow us to use it in spr_write_MMCR0_ureg().
>>>>
>>>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>>> ---
>>>>    target/ppc/cpu.h         |  1 +
>>>>    target/ppc/cpu_init.c    | 18 +++++++-------
>>>>    target/ppc/helper_regs.c |  3 +++
>>>>    target/ppc/spr_tcg.h     |  3 ++-
>>>>    target/ppc/translate.c   | 53 +++++++++++++++++++++++++++++++++++++++-
>>>>    5 files changed, 67 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
>>>> index f68bb8d8aa..8dfbb62022 100644
>>>> --- a/target/ppc/cpu.h
>>>> +++ b/target/ppc/cpu.h
>>>> @@ -616,6 +616,7 @@ enum {
>>>>        HFLAGS_SE = 10,  /* MSR_SE -- from elsewhere on embedded ppc */
>>>>        HFLAGS_FP = 13,  /* MSR_FP */
>>>>        HFLAGS_PR = 14,  /* MSR_PR */
>>>> +    HFLAGS_PMCCCLEAR = 15, /* PMU MMCR0 PMCC equal to 0b00 */
>>>>        HFLAGS_VSX = 23, /* MSR_VSX if cpu has VSX */
>>>>        HFLAGS_VR = 25,  /* MSR_VR if cpu has VRE */
>>>> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
>>>> index 9efc6c2d87..bb5ea04c61 100644
>>>> --- a/target/ppc/cpu_init.c
>>>> +++ b/target/ppc/cpu_init.c
>>>> @@ -6867,7 +6867,7 @@ static void register_book3s_pmu_sup_sprs(CPUPPCState *env)
>>>>    static void register_book3s_pmu_user_sprs(CPUPPCState *env)
>>>>    {
>>>>        spr_register(env, SPR_POWER_UMMCR0, "UMMCR0",
>>>> -                 &spr_read_MMCR0_ureg, SPR_NOACCESS,
>>>> +                 &spr_read_MMCR0_ureg, &spr_write_MMCR0_ureg,
>>>>                     &spr_read_ureg, &spr_write_ureg,
>>>>                     0x00000000);
>>>>        spr_register(env, SPR_POWER_UMMCR1, "UMMCR1",
>>>> @@ -6875,31 +6875,31 @@ static void register_book3s_pmu_user_sprs(CPUPPCState *env)
>>>>                     &spr_read_ureg, &spr_write_ureg,
>>>>                     0x00000000);
>>>>        spr_register(env, SPR_POWER_UMMCRA, "UMMCRA",
>>>> -                 &spr_read_ureg, SPR_NOACCESS,
>>>> +                 &spr_read_ureg, &spr_write_ureg,
>>>>                     &spr_read_ureg, &spr_write_ureg,
>>>>                     0x00000000);
>>>>        spr_register(env, SPR_POWER_UPMC1, "UPMC1",
>>>> -                 &spr_read_ureg, SPR_NOACCESS,
>>>> +                 &spr_read_ureg, &spr_write_ureg,
>>>
>>> Surely this can't be write.  AFAICT spr_write_ureg() will
>>> unconditionally allow full userspace write access.  That can't be
>>> right - otherwise the OS could never safely use the PMU for itself.
>>
>> My assumption here was that the user mode SPRs (UMMCR* and UPMC*) were created to
>> allow userspace read/write of PMU regs, while the regular regs (MMCR* and PMC*)
>> are the supermode privileged SPRs that can't be written by userspace. At least this
>> is my understanding from reading commit fd51ff6328e3d98158 that introduced these
>> userspace PMC regs.
> 
> Sure, but my point is that these registers are only userspace
> accessible under certain conditions, IIUC.  spr_write_ureg() doesn't
> test for those conditions, so it will *always* allow write access.


Got it.

I guess I'll end up biting the bullet and exposing both PMCC bits and adding
proper read/write access controls for the callbacks we need. This is somewhat
out of scope of my original goal with this series, but I guess we'll all better
off by doing it right now.

I'll add all the read/write ureg functions I'll need in the first patches (the PMC
write callback functions are on the patch 14, for instance). That will, hopefully,
making it easier to review the rest of the series by going through all the access
control and read/write callbacks early on.

Thanks,


Daniel

> 
>> The reason why these are marked as SPR_NOACCESS is because we didn't bothered
>> writing into them from userspace because we had no PMU logic to work
>> with.
> 
> [snip]
>>>> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
>>>> index b2ead144d1..0babde3131 100644
>>>> --- a/target/ppc/translate.c
>>>> +++ b/target/ppc/translate.c
>>>> @@ -175,6 +175,7 @@ struct DisasContext {
>>>>        bool spe_enabled;
>>>>        bool tm_enabled;
>>>>        bool gtse;
>>>> +    bool pmcc_clear;
>>>>        ppc_spr_t *spr_cb; /* Needed to check rights for mfspr/mtspr */
>>>>        int singlestep_enabled;
>>>>        uint32_t flags;
>>>> @@ -561,7 +562,56 @@ void spr_write_ureg(DisasContext *ctx, int sprn, int gprn)
>>>>    {
>>>>        gen_store_spr(sprn + 0x10, cpu_gpr[gprn]);
>>>>    }
>>>> -#endif
>>>> +
>>>> +void spr_write_MMCR0_ureg(DisasContext *ctx, int sprn, int gprn)
>>>
>>>
>>> Could you put this def in the PMU specific file, rather than the
>>> enormous translate.c?
>>
>> Moving into the existing power8_pmu.c helper is annoying because, being a helper file,
>> there is no access to the whole DisasContext declaration (that is open coded in
>> translate.c), and other internal translate.c data like cpu_grp[].
> 
> Ah, right.  We should probably make that easier someday, but it's not
> reasonbly in scope for this series.
> 
>> What I was able to do is create a new file in the target/ppc/translate/ dir,
>> power8-pmu-regs.c.impl, and moved all these declarations over there. At very least we're
>> not overloading translate.c.
> 
> Ah, nice.
> 
>> Eldorado, is that ok with you guys? I'm aware that this dir was holding new
>> decode-tree insns implementations but, in this case, it would hold old format
>> spr_read/spr_write code.
> 

Re: [PATCH v3 02/15] target/ppc: add user write access control for PMU SPRs

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 5456 bytes --]

On Mon, Sep 27, 2021 at 08:05:22PM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 9/27/21 02:08, David Gibson wrote:
> > On Thu, Sep 23, 2021 at 11:39:14AM -0300, Daniel Henrique Barboza wrote:
> > > 
> > > 
> > > On 9/6/21 22:38, David Gibson wrote:
> > > > On Fri, Sep 03, 2021 at 05:31:03PM -0300, Daniel Henrique Barboza wrote:
> > > > > The PMU needs to enable writing of its uregs to userspace, otherwise
> > > > > Perf applications will not able to setup the counters correctly. This
> > > > > patch enables user space writing of all PMU uregs.
> > > > > 
> > > > > MMCR0 is a special case because its userspace writing access is controlled
> > > > > by MMCR0_PMCC bits. There are 4 configurations available (0b00, 0b01,
> > > > > 0b10 and 0b11) but for our purposes here we're handling only
> > > > > MMCR0_PMCC = 0b00. In this case, if userspace tries to write MMCR0, a
> > > > > hypervisor emulation assistance interrupt occurs.
> > > > > 
> > > > > This is being done by adding HFLAGS_PMCCCLEAR to hflags. This flag
> > > > > indicates if MMCR0_PMCC is cleared (0b00), and a new 'pmcc_clear' flag in
> > > > > DisasContext allow us to use it in spr_write_MMCR0_ureg().
> > > > > 
> > > > > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> > > > > ---
> > > > >    target/ppc/cpu.h         |  1 +
> > > > >    target/ppc/cpu_init.c    | 18 +++++++-------
> > > > >    target/ppc/helper_regs.c |  3 +++
> > > > >    target/ppc/spr_tcg.h     |  3 ++-
> > > > >    target/ppc/translate.c   | 53 +++++++++++++++++++++++++++++++++++++++-
> > > > >    5 files changed, 67 insertions(+), 11 deletions(-)
> > > > > 
> > > > > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> > > > > index f68bb8d8aa..8dfbb62022 100644
> > > > > --- a/target/ppc/cpu.h
> > > > > +++ b/target/ppc/cpu.h
> > > > > @@ -616,6 +616,7 @@ enum {
> > > > >        HFLAGS_SE = 10,  /* MSR_SE -- from elsewhere on embedded ppc */
> > > > >        HFLAGS_FP = 13,  /* MSR_FP */
> > > > >        HFLAGS_PR = 14,  /* MSR_PR */
> > > > > +    HFLAGS_PMCCCLEAR = 15, /* PMU MMCR0 PMCC equal to 0b00 */
> > > > >        HFLAGS_VSX = 23, /* MSR_VSX if cpu has VSX */
> > > > >        HFLAGS_VR = 25,  /* MSR_VR if cpu has VRE */
> > > > > diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> > > > > index 9efc6c2d87..bb5ea04c61 100644
> > > > > --- a/target/ppc/cpu_init.c
> > > > > +++ b/target/ppc/cpu_init.c
> > > > > @@ -6867,7 +6867,7 @@ static void register_book3s_pmu_sup_sprs(CPUPPCState *env)
> > > > >    static void register_book3s_pmu_user_sprs(CPUPPCState *env)
> > > > >    {
> > > > >        spr_register(env, SPR_POWER_UMMCR0, "UMMCR0",
> > > > > -                 &spr_read_MMCR0_ureg, SPR_NOACCESS,
> > > > > +                 &spr_read_MMCR0_ureg, &spr_write_MMCR0_ureg,
> > > > >                     &spr_read_ureg, &spr_write_ureg,
> > > > >                     0x00000000);
> > > > >        spr_register(env, SPR_POWER_UMMCR1, "UMMCR1",
> > > > > @@ -6875,31 +6875,31 @@ static void register_book3s_pmu_user_sprs(CPUPPCState *env)
> > > > >                     &spr_read_ureg, &spr_write_ureg,
> > > > >                     0x00000000);
> > > > >        spr_register(env, SPR_POWER_UMMCRA, "UMMCRA",
> > > > > -                 &spr_read_ureg, SPR_NOACCESS,
> > > > > +                 &spr_read_ureg, &spr_write_ureg,
> > > > >                     &spr_read_ureg, &spr_write_ureg,
> > > > >                     0x00000000);
> > > > >        spr_register(env, SPR_POWER_UPMC1, "UPMC1",
> > > > > -                 &spr_read_ureg, SPR_NOACCESS,
> > > > > +                 &spr_read_ureg, &spr_write_ureg,
> > > > 
> > > > Surely this can't be write.  AFAICT spr_write_ureg() will
> > > > unconditionally allow full userspace write access.  That can't be
> > > > right - otherwise the OS could never safely use the PMU for itself.
> > > 
> > > My assumption here was that the user mode SPRs (UMMCR* and UPMC*) were created to
> > > allow userspace read/write of PMU regs, while the regular regs (MMCR* and PMC*)
> > > are the supermode privileged SPRs that can't be written by userspace. At least this
> > > is my understanding from reading commit fd51ff6328e3d98158 that introduced these
> > > userspace PMC regs.
> > 
> > Sure, but my point is that these registers are only userspace
> > accessible under certain conditions, IIUC.  spr_write_ureg() doesn't
> > test for those conditions, so it will *always* allow write access.
> 
> 
> Got it.
> 
> I guess I'll end up biting the bullet and exposing both PMCC bits and adding
> proper read/write access controls for the callbacks we need. This is somewhat
> out of scope of my original goal with this series, but I guess we'll all better
> off by doing it right now.

Yeah, sorry to divert you from the EBB stuff, but I don't want to
merge PMU support with glaring flaws.

> I'll add all the read/write ureg functions I'll need in the first patches (the PMC
> write callback functions are on the patch 14, for instance). That will, hopefully,
> making it easier to review the rest of the series by going through all the access
> control and read/write callbacks early on.

That sounds good.

-- 
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 --]