Re: [PATCH v3 29/37] target/ppc: Move system call and rfi instructions to decodetree

[Amit Machhiwal <amachhiw@linux.ibm.com>]

On 2026/08/27 06:59 PM, Chinmay Rath wrote:
> From: Vishal Chourasia <Vishal.Chourasia2@ibm.com>
> 
> 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 <vishalc@linux.ibm.com>
> Reviewed-by: Glenn Miles <milesg@linux.ibm.com>
> Signed-off-by: Chinmay Rath <rathc@linux.ibm.com>
> ---
>  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 | 102 ++++++++++++++++++++++
>  5 files changed, 128 insertions(+), 127 deletions(-)
> 
> diff --git a/target/ppc/helper.h b/target/ppc/helper.h
> index ec3350fee8..80d6616bab 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 7bbe1f8838..6563dc4f64 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 20d3e7c1ab..f46e2167ea 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) */
> @@ -4155,6 +4051,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) \

Nit: Similar to what I mentioned below.

> +    {                                                           \
> +        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) \
>      {                                                          \
> @@ -4529,21 +4432,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..26c82a5a7f 100644
> --- a/target/ppc/translate/misc-impl.c.inc
> +++ b/target/ppc/translate/misc-impl.c.inc
> @@ -225,3 +225,105 @@ 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)

Nit: space before a — should be arg_SC *a per QEMU coding style (pointer
attached to the type, not the variable name). This was present in v2 and
wasn't caught earlier.

> +{
> +    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_KIND_RFI = 0,
> +   RFI_KIND_RFID = 1,
> +   RFI_KIND_HRFID = 2,
> +   RFI_KIND_RFSCV = 3,
> +};
> +
> +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) {

On the broader design point I raised in v2 — preferring four separate
translators over the consolidated do_rfi(kind) approach: I still think
separate translators would be structurally cleaner. The RFI family has
genuinely distinct privilege semantics (REQUIRE_SV vs REQUIRE_HV), a
is_book3s_arch2x check for one variant only, and the BHRB omission for
HRFID — all of which require guards that must be remembered to apply
correctly when the function is modified in future.

That said, the RFI_KIND_HRFID guard is correctly in place in v3, and I
am okay with this approach if you prefer to keep it. The key correctness
issue is resolved.

With the two pointer-spacing nits fixed (@Harsh, if there's no respin,
could you please take care of this while applying the patch?):

Reviewed-by: Amit Machhiwal <amachhiw@linux.ibm.com>

> +    case RFI_KIND_RFI:
> +        if (is_book3s_arch2x(ctx)) {
> +            gen_invalid(ctx);
> +            return true;
> +        }
> +        REQUIRE_SV(ctx);
> +        helper = gen_helper_RFI;
> +        break;
> +#if defined(TARGET_PPC64)
> +    case RFI_KIND_HRFID:
> +        REQUIRE_HV(ctx);
> +        helper = gen_helper_HRFID;
> +        break;
> +    case RFI_KIND_RFID:
> +        REQUIRE_SV(ctx);
> +        helper = gen_helper_RFID;
> +        break;
> +    case RFI_KIND_RFSCV:
> +        REQUIRE_SV(ctx);
> +        helper = gen_helper_RFSCV;
> +        break;
> +#endif
> +    default:
> +        gen_invalid(ctx);
> +        return true;
> +    }
> +
> +    translator_io_start(&ctx->base);
> +    if (kind != RFI_KIND_HRFID) {
> +        gen_update_branch_history(ctx, ctx->cia, NULL, BHRB_TYPE_NORECORD);
> +    }
> +    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_KIND_RFI)
> +TRANS64(RFID, do_rfi, RFI_KIND_RFID)
> +TRANS64_FLAGS(64H, HRFID, do_rfi, RFI_KIND_HRFID)
> +TRANS64_FLAGS2(ISA300, RFSCV, do_rfi, RFI_KIND_RFSCV)
> -- 
> 2.55.0
>