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

[Glenn Miles <milesg@linux.vnet.ibm.com>]

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