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

["Nicholas Piggin" <npiggin@gmail.com>]

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.

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

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

Thanks,
Nick

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