qemu-ppc
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 1/4] target/ppc: Add new hflags to support BHRB


From: Glenn Miles
Subject: Re: [PATCH 1/4] target/ppc: Add new hflags to support BHRB
Date: Tue, 19 Sep 2023 16:19:12 -0500

On 2023-09-14 19:39, Nicholas Piggin wrote:
On Wed Sep 13, 2023 at 6:23 AM AEST, Glenn Miles wrote:
This commit is preparatory to the addition of Branch History
Rolling Buffer (BHRB) functionality, which is being provided
today starting with the P8 processor.

BHRB uses several SPR register fields to control whether or not
a branch instruction's address (and sometimes target address)
should be recorded.  Checking each of these fields with each
branch instruction using jitted code would lead to a significant
decrease in performance.

Therefore, it was decided that BHRB configuration bits that are
not expected to change frequently should have their state stored in
hflags so that the amount of checking done by jitted code can
be reduced.

This commit contains the changes for storing the state of the
following register fields as hflags:

        MMCR0[FCP] - Determines if BHRB recording is frozen in the
                     problem state

        MMCR0[FCPC] - A modifier for MMCR0[FCP]

        MMCRA[BHRBRD] - Disables all BHRB recording for a thread

Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com>
---
 target/ppc/cpu.h                 |  9 +++++++++
 target/ppc/cpu_init.c            |  4 ++--
 target/ppc/helper.h              |  1 +
 target/ppc/helper_regs.c         | 12 ++++++++++++
 target/ppc/machine.c             |  2 +-
 target/ppc/power8-pmu-regs.c.inc |  5 +++++
 target/ppc/power8-pmu.c          | 15 +++++++++++----
 target/ppc/power8-pmu.h          |  4 ++--
 target/ppc/spr_common.h          |  1 +
 target/ppc/translate.c           |  6 ++++++
 10 files changed, 50 insertions(+), 9 deletions(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 25fac9577a..20ae1466a5 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -439,6 +439,9 @@ FIELD(MSR, LE, MSR_LE, 1)
#define MMCR0_FC56 PPC_BIT(59) /* PMC Freeze Counters 5-6 bit */ #define MMCR0_PMC1CE PPC_BIT(48) /* MMCR0 PMC1 Condition Enabled */ #define MMCR0_PMCjCE PPC_BIT(49) /* MMCR0 PMCj Condition Enabled */
+#define MMCR0_BHRBA  PPC_BIT_NR(42)      /* BHRB Available */

It's confusing to use NR for this. Either call it MMCR0_BHRBA_NR or have the facility check in patch 3 take the bit value. I'd move it to patch 3
too.


Ok, adding NR suffix.

+#define MMCR0_FCP PPC_BIT(34) /* Freeze Counters/BHRB if PR=1 */
+#define MMCR0_FCPC   PPC_BIT(51)         /* Condition for FCP bit */
 /* MMCR0 userspace r/w mask */
 #define MMCR0_UREG_MASK (MMCR0_FC | MMCR0_PMAO | MMCR0_PMAE)
 /* MMCR2 userspace r/w mask */
@@ -451,6 +454,9 @@ FIELD(MSR, LE, MSR_LE, 1)
 #define MMCR2_UREG_MASK (MMCR2_FC1P0 | MMCR2_FC2P0 | MMCR2_FC3P0 | \
                          MMCR2_FC4P0 | MMCR2_FC5P0 | MMCR2_FC6P0)

+#define MMCRA_BHRBRD PPC_BIT(26) /* BHRB Recording Disable */
+
+
 #define MMCR1_EVT_SIZE 8
 /* extract64() does a right shift before extracting */
 #define MMCR1_PMC1SEL_START 32
@@ -703,6 +709,9 @@ enum {
     HFLAGS_PMCJCE = 17, /* MMCR0 PMCjCE bit */
     HFLAGS_PMC_OTHER = 18, /* PMC other than PMC5-6 is enabled */
     HFLAGS_INSN_CNT = 19, /* PMU instruction count enabled */
+    HFLAGS_FCPC = 20,   /* MMCR0 FCPC bit */
+    HFLAGS_FCP = 21,    /* MMCR0 FCP bit */
+    HFLAGS_BHRBRD = 22, /* MMCRA BHRBRD bit */
     HFLAGS_VSX = 23, /* MSR_VSX if cpu has VSX */
     HFLAGS_VR = 25,  /* MSR_VR if cpu has VRE */

hflags are an interesting tradeoff. You can specialise some code but
at the cost of duplicating your jit footprint, which is often the
most costly thing. The ideal hflag is one where code is not shared
between flag set/clear like PR and HV. Rarely used features is another
good one, that BHRB falls into.

But, we do want flags that carry stronger or more direct semantics
wrt code generation because you want to avoid redundant hflags values
that result in the same code generation. I might have missed something
but AFAIKS BHRB_ENABLED could be a combination of this logic (from
later patch):

+    /* ISA 3.1 adds the PMCRA[BRHBRD] and problem state checks */
+ if ((ctx->insns_flags2 & PPC2_ISA310) && (ctx->mmcra_bhrbrd || !ctx->pr)) {
+        return;
+    }
+
+    /* Check for BHRB "frozen" conditions */
+    if (ctx->mmcr0_fcpc) {
+        if (ctx->mmcr0_fcp) {
+            if ((ctx->hv) && (ctx->pr)) {
+                return;
+            }
+        } else if (!(ctx->hv) && (ctx->pr)) {
+            return;
+        }
+    } else if ((ctx->mmcr0_fcp) && (ctx->pr)) {
+        return;
+    }


Ok, Combining above logic into a single hflag.

Otherwise the patch looks good to me.

Thanks,
Nick



reply via email to

[Prev in Thread] Current Thread [Next in Thread]