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 <mil...@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),