On 2026/08/26 10:38 AM, Chinmay Rath wrote:
> From: Vishal Chourasia <[email protected]>
>
> Moving the following instructions to decodetree specification:
> sc, scv : SC-form
> rfi, rfid, rfscv, hrfid : XL-form
>
> This builds upon the previous work that moved mfmsr and mtmsr[d]
> instructions to decodetree.
>
> The changes were verified by validating that the tcg ops generated by
> those instructions remain the same, which were captured with the
> `-d in_asm,op` flag, and also by booting a pseries qemu guest.
>
> This also includes improvements from review feedback:
> - Rename helpers to uppercase (RFI/RFID/RFSCV/HRFID) to match ISA mnemonics
> - Add TRANS64_FLAGS() macro variants
> - Add REQUIRE_INSNS_FLAGS_NOT() check for flag exclusion
> - Specify lev field as uint8_t in SC instruction format
> - Remove redundant masking in SCV since lev is already 7-bit
> - Replace TARGET_PPC64 ifdefs with REQUIRE_64BIT() macro
> - Gate SC, SCV, RFI, RFID, RFSCV, and HRFID with appropriate flags
> - Consolidate CONFIG_USER_ONLY guards into single block
>
> Signed-off-by: Vishal Chourasia <[email protected]>
> Reviewed-by: Glenn Miles <[email protected]>
> Signed-off-by: Chinmay Rath <[email protected]>
> ---
> target/ppc/helper.h | 8 +-
> target/ppc/insn32.decode | 11 +++
> target/ppc/tcg-excp_helper.c | 8 +-
> target/ppc/translate.c | 126 ++-------------------------
> target/ppc/translate/misc-impl.c.inc | 100 +++++++++++++++++++++
> 5 files changed, 126 insertions(+), 127 deletions(-)
>
> diff --git a/target/ppc/helper.h b/target/ppc/helper.h
> index 3659408c46..f24a34cfa3 100644
> --- a/target/ppc/helper.h
> +++ b/target/ppc/helper.h
> @@ -11,7 +11,7 @@ DEF_HELPER_4(HASHCHKP, void, env, tl, tl, tl)
> #if !defined(CONFIG_USER_ONLY)
> DEF_HELPER_2(store_msr, void, env, tl)
> DEF_HELPER_1(ppc_maybe_interrupt, void, env)
> -DEF_HELPER_1(rfi, void, env)
> +DEF_HELPER_1(RFI, void, env)
> DEF_HELPER_1(40x_rfci, void, env)
> DEF_HELPER_1(rfci, void, env)
> DEF_HELPER_1(rfdi, void, env)
> @@ -19,9 +19,9 @@ DEF_HELPER_1(rfmci, void, env)
> #if defined(TARGET_PPC64)
> DEF_HELPER_2(scv, noreturn, env, i32)
> DEF_HELPER_2(PMINSN, void, env, i32)
> -DEF_HELPER_1(rfid, void, env)
> -DEF_HELPER_1(rfscv, void, env)
> -DEF_HELPER_1(hrfid, void, env)
> +DEF_HELPER_1(RFID, void, env)
> +DEF_HELPER_1(RFSCV, void, env)
> +DEF_HELPER_1(HRFID, void, env)
> DEF_HELPER_2(rfebb, void, env, tl)
> DEF_HELPER_2(store_lpcr, void, env, tl)
> DEF_HELPER_2(store_pcr, void, env, tl)
> diff --git a/target/ppc/insn32.decode b/target/ppc/insn32.decode
> index 7614655f72..9d6c4b550d 100644
> --- a/target/ppc/insn32.decode
> +++ b/target/ppc/insn32.decode
> @@ -199,6 +199,9 @@
> &X_rs_l rs l:bool
> @X_rs_l ...... rs:5 .... l:1 ..... .......... . &X_rs_l
>
> +&SC lev:uint8_t
> +@SC ...... ..... ..... .... lev:7 ... . . &SC
> +
> &X_uim5 xt uim:uint8_t
> @X_uim5 ...... ..... ..... uim:5 .......... . &X_uim5
> xt=%x_xt
>
> @@ -320,6 +323,14 @@ MFMSR 011111 ..... ----- ----- 0001010011 -
> @X_t
> MTMSR 011111 ..... ---- . ----- 0010010010 - @X_rs_l
> MTMSRD 011111 ..... ---- . ----- 0010110010 - @X_rs_l
>
> +### System Call and Return from Interrupt
> +SC 010001 ----- ----- ---- ....... --- 1 - @SC
> +SCV 010001 ----- ----- ---- ....... --- 0 1 @SC
> +RFI 010011 ----- ----- ----- 0000110010 -
> +RFID 010011 ----- ----- ----- 0000010010 -
> +RFSCV 010011 ----- ----- ----- 0001010010 -
> +HRFID 010011 ----- ----- ----- 0100010010 -
> +
> ### Fixed-Point Byte-Reverse Instructions
> BRW 011111 ..... ..... ----- 0010011011 - @X_sa
> BRD 011111 ..... ..... ----- 0010111011 - @X_sa
> diff --git a/target/ppc/tcg-excp_helper.c b/target/ppc/tcg-excp_helper.c
> index c4ffa2dfbb..d06d3f8642 100644
> --- a/target/ppc/tcg-excp_helper.c
> +++ b/target/ppc/tcg-excp_helper.c
> @@ -528,13 +528,13 @@ static void do_rfi(CPUPPCState *env, target_ulong nip,
> target_ulong msr)
> check_tlb_flush(env, false);
> }
>
> -void helper_rfi(CPUPPCState *env)
> +void helper_RFI(CPUPPCState *env)
> {
> do_rfi(env, env->spr[SPR_SRR0], env->spr[SPR_SRR1] & 0xfffffffful);
> }
>
> #ifdef TARGET_PPC64
> -void helper_rfid(CPUPPCState *env)
> +void helper_RFID(CPUPPCState *env)
> {
> /*
> * The architecture defines a number of rules for which bits can
> @@ -545,12 +545,12 @@ void helper_rfid(CPUPPCState *env)
> do_rfi(env, env->spr[SPR_SRR0], env->spr[SPR_SRR1]);
> }
>
> -void helper_rfscv(CPUPPCState *env)
> +void helper_RFSCV(CPUPPCState *env)
> {
> do_rfi(env, env->lr, env->ctr);
> }
>
> -void helper_hrfid(CPUPPCState *env)
> +void helper_HRFID(CPUPPCState *env)
> {
> do_rfi(env, env->spr[SPR_HSRR0], env->spr[SPR_HSRR1]);
> }
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 8950476be7..c6c87d3e33 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -2679,110 +2679,6 @@ static inline void gen_setlr(DisasContext *ctx,
> target_ulong nip)
> tcg_gen_movi_tl(cpu_lr, nip);
> }
>
> -/*** System linkage
> ***/
> -
> -/* rfi (supervisor only) */
> -static void gen_rfi(DisasContext *ctx)
> -{
> -#if defined(CONFIG_USER_ONLY)
> - GEN_PRIV(ctx);
> -#else
> - /*
> - * This instruction doesn't exist anymore on 64-bit server
> - * processors compliant with arch 2.x
> - */
> - if (is_book3s_arch2x(ctx)) {
> - gen_inval_exception(ctx, POWERPC_EXCP_INVAL_INVAL);
> - return;
> - }
> - /* Restore CPU state */
> - CHK_SV(ctx);
> - translator_io_start(&ctx->base);
> - gen_update_branch_history(ctx, ctx->cia, NULL, BHRB_TYPE_NORECORD);
> - gen_helper_rfi(tcg_env);
> - ctx->base.is_jmp = DISAS_EXIT;
> -#endif
> -}
> -
> -#if defined(TARGET_PPC64)
> -static void gen_rfid(DisasContext *ctx)
> -{
> -#if defined(CONFIG_USER_ONLY)
> - GEN_PRIV(ctx);
> -#else
> - /* Restore CPU state */
> - CHK_SV(ctx);
> - translator_io_start(&ctx->base);
> - gen_update_branch_history(ctx, ctx->cia, NULL, BHRB_TYPE_NORECORD);
> - gen_helper_rfid(tcg_env);
> - ctx->base.is_jmp = DISAS_EXIT;
> -#endif
> -}
> -
> -#if !defined(CONFIG_USER_ONLY)
> -static void gen_rfscv(DisasContext *ctx)
> -{
> -#if defined(CONFIG_USER_ONLY)
> - GEN_PRIV(ctx);
> -#else
> - /* Restore CPU state */
> - CHK_SV(ctx);
> - translator_io_start(&ctx->base);
> - gen_update_branch_history(ctx, ctx->cia, NULL, BHRB_TYPE_NORECORD);
> - gen_helper_rfscv(tcg_env);
> - ctx->base.is_jmp = DISAS_EXIT;
> -#endif
> -}
> -#endif
> -
> -static void gen_hrfid(DisasContext *ctx)
> -{
> -#if defined(CONFIG_USER_ONLY)
> - GEN_PRIV(ctx);
> -#else
> - /* Restore CPU state */
> - CHK_HV(ctx);
> - translator_io_start(&ctx->base);
> - gen_helper_hrfid(tcg_env);
> - ctx->base.is_jmp = DISAS_EXIT;
> -#endif
> -}
> -#endif
> -
> -/* sc */
> -#if defined(CONFIG_USER_ONLY)
> -#define POWERPC_SYSCALL POWERPC_EXCP_SYSCALL_USER
> -#else
> -#define POWERPC_SYSCALL POWERPC_EXCP_SYSCALL
> -#endif
> -static void gen_sc(DisasContext *ctx)
> -{
> - uint32_t lev;
> -
> - /*
> - * LEV is a 7-bit field, but the top 6 bits are treated as a reserved
> - * field (i.e., ignored). ISA v3.1 changes that to 5 bits, but that is
> - * for Ultravisor which TCG does not support, so just ignore the top 6.
> - */
> - lev = (ctx->opcode >> 5) & 0x1;
> - gen_exception_err(ctx, POWERPC_SYSCALL, lev);
> -}
> -
> -#if defined(TARGET_PPC64)
> -#if !defined(CONFIG_USER_ONLY)
> -static void gen_scv(DisasContext *ctx)
> -{
> - uint32_t lev = (ctx->opcode >> 5) & 0x7F;
> -
> - /* Set the PC back to the faulting instruction. */
> - gen_update_nip(ctx, ctx->cia);
> - gen_helper_scv(tcg_env, tcg_constant_i32(lev));
> -
> - ctx->base.is_jmp = DISAS_NORETURN;
> -}
> -#endif
> -#endif
> -
> /*** Trap
> ***/
>
> /* Check for unconditional traps (always or never) */
> @@ -4125,6 +4021,13 @@ static int64_t dw_compose_ea(DisasContext *ctx, int x)
> #define TRANS64(NAME, FUNC, ...) \
> static bool trans_##NAME(DisasContext *ctx, arg_##NAME *a) \
> { REQUIRE_64BIT(ctx); return FUNC(ctx, a, ##__VA_ARGS__); }
> +#define TRANS64_FLAGS(FLAGS, NAME, FUNC, ...) \
> + static bool trans_##NAME(DisasContext *ctx, arg_##NAME * a) \
> + { \
> + REQUIRE_64BIT(ctx); \
> + REQUIRE_INSNS_FLAGS(ctx, FLAGS); \
> + return FUNC(ctx, a, ##__VA_ARGS__); \
> + }
> #define TRANS64_FLAGS2(FLAGS2, NAME, FUNC, ...) \
> static bool trans_##NAME(DisasContext *ctx, arg_##NAME *a) \
> { \
> @@ -4542,21 +4445,6 @@ GEN_HANDLER(rlwnm, 0x17, 0xFF, 0xFF, 0x00000000,
> PPC_INTEGER),
> GEN_HANDLER_E(dform39, 0x39, 0xFF, 0xFF, 0x00000000, PPC_NONE, PPC2_ISA205),
> /* handles stfdp, stxsd, stxssp */
> GEN_HANDLER_E(dform3D, 0x3D, 0xFF, 0xFF, 0x00000000, PPC_NONE, PPC2_ISA205),
> -/* ISA v3.0 changed the extended opcode from 62 to 30 */
> -GEN_HANDLER(rfi, 0x13, 0x12, 0x01, 0x03FF8001, PPC_FLOW),
> -#if defined(TARGET_PPC64)
> -GEN_HANDLER(rfid, 0x13, 0x12, 0x00, 0x03FF8001, PPC_64B),
> -#if !defined(CONFIG_USER_ONLY)
> -/* Top bit of opc2 corresponds with low bit of LEV, so use two handlers */
> -GEN_HANDLER_E(scv, 0x11, 0x10, 0xFF, 0x03FFF01E, PPC_NONE, PPC2_ISA300),
> -GEN_HANDLER_E(scv, 0x11, 0x00, 0xFF, 0x03FFF01E, PPC_NONE, PPC2_ISA300),
> -GEN_HANDLER_E(rfscv, 0x13, 0x12, 0x02, 0x03FF8001, PPC_NONE, PPC2_ISA300),
> -#endif
> -GEN_HANDLER(hrfid, 0x13, 0x12, 0x08, 0x03FF8001, PPC_64H),
> -#endif
> -/* Top bit of opc2 corresponds with low bit of LEV, so use two handlers */
> -GEN_HANDLER(sc, 0x11, 0x11, 0xFF, 0x03FFF01D, PPC_FLOW),
> -GEN_HANDLER(sc, 0x11, 0x01, 0xFF, 0x03FFF01D, PPC_FLOW),
> GEN_HANDLER(mcrxr, 0x1F, 0x00, 0x10, 0x007FF801, PPC_MISC),
> GEN_HANDLER(mfspr, 0x1F, 0x13, 0x0A, 0x00000001, PPC_MISC),
> GEN_HANDLER(mftb, 0x1F, 0x13, 0x0B, 0x00000001, PPC_MFTB),
> diff --git a/target/ppc/translate/misc-impl.c.inc
> b/target/ppc/translate/misc-impl.c.inc
> index 54712f9b73..05757e8190 100644
> --- a/target/ppc/translate/misc-impl.c.inc
> +++ b/target/ppc/translate/misc-impl.c.inc
> @@ -225,3 +225,103 @@ static bool do_mtmsr(DisasContext *ctx, arg_X_rs_l *a,
> bool is_mtmsrd)
>
> TRANS_FLAGS(MISC, MTMSR, do_mtmsr, false)
> TRANS64(MTMSRD, do_mtmsr, true)
> +
> +/*
> + * System Call and Return from Interrupt Instructions
> + */
> +
> +static bool do_SC(DisasContext *ctx, arg_SC * a, uint32_t excp)
> +{
> + uint32_t lev;
> +
> + /*
> + * LEV is a 7-bit field, but the top 6 bits are treated as a reserved
> + * field (i.e., ignored). ISA v3.1 changes that to 5 bits, but that is
> + * for Ultravisor which TCG does not support, so just ignore the top 6.
> + */
> + lev = a->lev & 0x1;
> + gen_exception_err(ctx, excp, lev);
> + return true;
> +}
> +
> +static bool do_SCV(DisasContext *ctx, arg_SC *a)
> +{
> +#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
> + /* Set the PC back to the faulting instruction. */
> + gen_update_nip(ctx, ctx->cia);
> + gen_helper_scv(tcg_env, tcg_constant_i32(a->lev));
> +
> + ctx->base.is_jmp = DISAS_NORETURN;
> + return true;
> +#else
> + gen_invalid(ctx);
> + return true;
> +#endif
> +}
> +
> +/*
> + * Common helper for return-from-interrupt instructions
> + */
> +enum {
> + RFI = 0,
> + RFID = 1,
> + HRFID = 2,
> + RFSCV = 3,
> +};
These names seem shadow the decodetree-generated arg_RFI, arg_RFID
struct names and instruction mnemonics. The line TRANS_FLAGS(FLOW, RFI,
do_rfi, RFI) is particularly confusing — the trailing RFI is the enum
value 0, but it looks like the instruction name. Prefer a distinct
prefix, e.g. RFI_KIND_RFI = 0, RFI_KIND_RFID = 1, etc.
> +
> +static bool do_rfi(DisasContext *ctx, arg_RFID *a, int kind)
> +{
> +#if defined(CONFIG_USER_ONLY)
> + gen_priv_opc(ctx);
> + return true;
> +#else
> + void (*helper)(TCGv_ptr);
> +
> + switch (kind) {
> + case RFI:
> + if (is_book3s_arch2x(ctx)) {
> + gen_invalid(ctx);
> + return true;
> + }
> + REQUIRE_SV(ctx);
> + helper = gen_helper_RFI;
> + break;
> +#if defined(TARGET_PPC64)
> + case HRFID:
> + REQUIRE_HV(ctx);
> + helper = gen_helper_HRFID;
> + break;
> + case RFID:
> + REQUIRE_SV(ctx);
> + helper = gen_helper_RFID;
> + break;
> + case RFSCV:
> + REQUIRE_SV(ctx);
> + helper = gen_helper_RFSCV;
> + break;
> +#endif
> + default:
> + gen_invalid(ctx);
> + return true;
> + }
> +
> + translator_io_start(&ctx->base);
> + gen_update_branch_history(ctx, ctx->cia, NULL, BHRB_TYPE_NORECORD);
This gen_update_branch_history call sits after the switch and executes
unconditionally for all four kind values. The old gen_hrfid did not call
it.
Inside gen_update_branch_history, if ctx->has_cfar is true, cpu_cfar is
written unconditionally before the BHRB-recording check. So hrfid now
updates CFAR on every execution, where it did not before. The fix is to
guard the call:
if (kind != HRFID) {
gen_update_branch_history(ctx, ctx->cia, NULL, BHRB_TYPE_NORECORD);
}
Having said that, I think the root cause is the consolidation itself.
With four separate translators (the pattern used throughout this
series), each would contain exactly what its old gen_ function had, and
the HRFID omission would have been naturally preserved.
~Amit
> + helper(tcg_env);
> + ctx->base.is_jmp = DISAS_EXIT;
> + return true;
> +#endif
> +}
> +
> +#if defined(CONFIG_USER_ONLY)
> +TRANS_FLAGS(FLOW, SC, do_SC, POWERPC_EXCP_SYSCALL_USER)
> +#else
> +TRANS_FLAGS(FLOW, SC, do_SC, POWERPC_EXCP_SYSCALL)
> +#endif
> +
> +TRANS64_FLAGS2(ISA300, SCV, do_SCV)
> +
> +TRANS_FLAGS(FLOW, RFI, do_rfi, RFI)
> +TRANS64(RFID, do_rfi, RFID)
> +TRANS64_FLAGS(64H, HRFID, do_rfi, HRFID)
> +TRANS64_FLAGS2(ISA300, RFSCV, do_rfi, RFSCV)
> --
> 2.55.0
>
