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

[Glenn Miles]

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