Re: [PATCH 3/4] target/ppc: Add clrbhrb and mfbhrbe instructions

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

On 2023-09-14 20:13, Nicholas Piggin wrote:
> On Wed Sep 13, 2023 at 6:24 AM AEST, Glenn Miles wrote:
>> Add support for the clrbhrb and mfbhrbe instructions.
>> 
>> Since neither instruction is believed to be critical to
>> performance, both instructions were implemented using helper
>> functions.
>> 
>> Access to both instructions is controlled by bits in the
>> HFSCR (for privileged state) and MMCR0 (for problem state).
>> A new function, helper_mmcr0_facility_check, was added for
>> checking MMCR0[BHRBA] and raising a facility_unavailable exception
>> if required.
>> 
>> Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com>
>> ---
>>  target/ppc/cpu.h         |  1 +
>>  target/ppc/helper.h      |  4 ++++
>>  target/ppc/misc_helper.c | 43 
>> ++++++++++++++++++++++++++++++++++++++++
>>  target/ppc/translate.c   | 13 ++++++++++++
>>  4 files changed, 61 insertions(+)
>> 
>> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
>> index bda1afb700..ee81ede4ee 100644
>> --- a/target/ppc/cpu.h
>> +++ b/target/ppc/cpu.h
>> @@ -541,6 +541,7 @@ FIELD(MSR, LE, MSR_LE, 1)
>> 
>>  /* HFSCR bits */
>>  #define HFSCR_MSGP     PPC_BIT(53) /* Privileged Message Send 
>> Facilities */
>> +#define HFSCR_BHRB     PPC_BIT(59) /* BHRB Instructions */
>>  #define HFSCR_IC_MSGP  0xA
>> 
>>  #define DBCR0_ICMP (1 << 27)
>> diff --git a/target/ppc/helper.h b/target/ppc/helper.h
>> index 1a3d9a7e57..bbc32ff114 100644
>> --- a/target/ppc/helper.h
>> +++ b/target/ppc/helper.h
>> @@ -816,3 +816,7 @@ DEF_HELPER_4(DSCLIQ, void, env, fprp, fprp, i32)
>> 
>>  DEF_HELPER_1(tbegin, void, env)
>>  DEF_HELPER_FLAGS_1(fixup_thrm, TCG_CALL_NO_RWG, void, env)
>> +
>> +DEF_HELPER_1(clrbhrb, void, env)
>> +DEF_HELPER_FLAGS_2(mfbhrbe, TCG_CALL_NO_WG, i64, env, i32)
>> +
>> diff --git a/target/ppc/misc_helper.c b/target/ppc/misc_helper.c
>> index 692d058665..45abe04f66 100644
>> --- a/target/ppc/misc_helper.c
>> +++ b/target/ppc/misc_helper.c
>> @@ -139,6 +139,17 @@ void helper_fscr_facility_check(CPUPPCState *env, 
>> uint32_t bit,
>>  #endif
>>  }
>> 
>> +static void helper_mmcr0_facility_check(CPUPPCState *env, uint32_t 
>> bit,
>> +                                 uint32_t sprn, uint32_t cause)
>> +{
>> +#ifdef TARGET_PPC64
>> +    if (FIELD_EX64(env->msr, MSR, PR) &&
>> +        !(env->spr[SPR_POWER_MMCR0] & (1ULL << bit))) {
>> +        raise_fu_exception(env, bit, sprn, cause, GETPC());
>> +    }
>> +#endif
>> +}
>> +
>>  void helper_msr_facility_check(CPUPPCState *env, uint32_t bit,
>>                                 uint32_t sprn, uint32_t cause)
>>  {
>> @@ -351,3 +362,35 @@ void helper_fixup_thrm(CPUPPCState *env)
>>          env->spr[i] = v;
>>      }
>>  }
>> +
>> +void helper_clrbhrb(CPUPPCState *env)
>> +{
>> +    helper_hfscr_facility_check(env, HFSCR_BHRB, "clrbhrb", 
>> FSCR_IC_BHRB);
>> +
>> +    helper_mmcr0_facility_check(env, MMCR0_BHRBA, 0, FSCR_IC_BHRB);
> 
> Repeating the comment about MMCR0_BHRBA and PPC_BIT_NR discrepancy here
> for posterity.
> 

Added NR suffix.

>> +
>> +    memset(env->bhrb, 0, sizeof(env->bhrb));
>> +}
>> +
>> +uint64_t helper_mfbhrbe(CPUPPCState *env, uint32_t bhrbe)
>> +{
>> +    unsigned int index;
>> +
>> +    helper_hfscr_facility_check(env, HFSCR_BHRB, "mfbhrbe", 
>> FSCR_IC_BHRB);
>> +
>> +    helper_mmcr0_facility_check(env, MMCR0_BHRBA, 0, FSCR_IC_BHRB);
>> +
>> +    if ((bhrbe >= env->bhrb_num_entries) ||
>> +       (env->spr[SPR_POWER_MMCR0] & MMCR0_PMAE)) {
> 
> Nitpick, but multi line statment starts again inside the first
> parenthesis after a keyword like this.
> 

Fixed.

>> +        return 0;
>> +    }
>> +
>> +    /*
>> +     * Note: bhrb_offset is the byte offset for writing the
>> +     * next entry (over the oldest entry), which is why we
>> +     * must offset bhrbe by 1 to get to the 0th entry.
>> +     */
>> +    index = ((env->bhrb_offset / sizeof(uint64_t)) - (bhrbe + 1)) %
>> +            env->bhrb_num_entries;
>> +    return env->bhrb[index];
>> +}
>> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
>> index 7824475f54..b330871793 100644
>> --- a/target/ppc/translate.c
>> +++ b/target/ppc/translate.c
>> @@ -6549,12 +6549,25 @@ static void gen_brh(DisasContext *ctx)
>>  }
>>  #endif
>> 
>> +static void gen_clrbhrb(DisasContext *ctx)
>> +{
>> +    gen_helper_clrbhrb(cpu_env);
>> +}
>> +
>> +static void gen_mfbhrbe(DisasContext *ctx)
>> +{
>> +    TCGv_i32 bhrbe = tcg_constant_i32(_SPR(ctx->opcode));
>> +    gen_helper_mfbhrbe(cpu_gpr[rD(ctx->opcode)], cpu_env, bhrbe);
>> +}
>> +
>>  static opcode_t opcodes[] = {
>>  #if defined(TARGET_PPC64)
>>  GEN_HANDLER_E(brd, 0x1F, 0x1B, 0x05, 0x0000F801, PPC_NONE, 
>> PPC2_ISA310),
>>  GEN_HANDLER_E(brw, 0x1F, 0x1B, 0x04, 0x0000F801, PPC_NONE, 
>> PPC2_ISA310),
>>  GEN_HANDLER_E(brh, 0x1F, 0x1B, 0x06, 0x0000F801, PPC_NONE, 
>> PPC2_ISA310),
>>  #endif
>> +GEN_HANDLER_E(clrbhrb, 0x1F, 0x0E, 0x0D, 0x3FFF801, PPC_NONE, 
>> PPC2_ISA207S),
>> +GEN_HANDLER_E(mfbhrbe, 0x1F, 0x0E, 0x09, 0x0000001, PPC_NONE, 
>> PPC2_ISA207S),
> 
> How much of a pain would it be to add it as decodetree? If there is an
> addition a family of existing instrutions here it makes sense to add it
> here, for new family would be nice to use decodetree.
> 
> I think they're only supported in 64-bit ISA so it could be ifdef
> TARGET_PPC64.
> 

Ok, switched to using decodetree.

> Thanks,
> Nick
> 

Thanks for the review!

Glenn

>>  GEN_HANDLER(invalid, 0x00, 0x00, 0x00, 0xFFFFFFFF, PPC_NONE),
>>  #if defined(TARGET_PPC64)
>>  GEN_HANDLER_E(cmpeqb, 0x1F, 0x00, 0x07, 0x00600000, PPC_NONE, 
>> PPC2_ISA300),