[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 3/4] target/ppc: Add clrbhrb and mfbhrbe instructions
From: |
Nicholas Piggin |
Subject: |
Re: [PATCH 3/4] target/ppc: Add clrbhrb and mfbhrbe instructions |
Date: |
Fri, 15 Sep 2023 11:13:07 +1000 |
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),