Re: [PATCH 27/29] target/ppc: Move mfmsr, mtmsr[d] instructions to decodetree

[Miles Glenn <milesg@linux.ibm.com>]

Reviewed-by: Glenn Miles <milesg@linux.ibm.com>

Thanks,

Glenn

On Thu, 2026-08-20 at 19:30 +0530, Chinmay Rath wrote:
> From: Vishal Chourasia <vishalc@linux.ibm.com>
> 
> Moving the following instructions to decodetree specification:
>             mfmsr                           : X-form
>             mtmsr[d]                        : X-form
> 
> 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.
> 
> This also includes improvements from review feedback:
> - Gate MFMSR/MTMSR on PPC_MISC via TRANS_FLAGS()
> - Use UINT32_MAX instead of 0xFFFFFFFF for clarity
> 
> Signed-off-by: Vishal Chourasia <vishalc@linux.ibm.com>
> Signed-off-by: Chinmay Rath <rathc@linux.ibm.com>
> ---
>  target/ppc/insn32.decode             |  5 ++
>  target/ppc/translate.c               | 97 ----------------------------
>  target/ppc/translate/misc-impl.c.inc | 64 ++++++++++++++++++
>  3 files changed, 69 insertions(+), 97 deletions(-)
> 
> diff --git a/target/ppc/insn32.decode b/target/ppc/insn32.decode
> index b0fa85b93e..f7ab08704f 100644
> --- a/target/ppc/insn32.decode
> +++ b/target/ppc/insn32.decode
> @@ -315,6 +315,11 @@
>  &M              ra rs sh mb me rc:bool
>  @M              ...... rs:5 ra:5 sh:5 mb:5 me:5 rc:1 &M
>  
> +### MSR
> +MFMSR           011111 ..... ----- ----- 0001010011 -   @X_t
> +MTMSR           011111 ..... ---- . ----- 0010010010 -  @X_rs_l
> +MTMSRD          011111 ..... ---- . ----- 0010110010 -  @X_rs_l
> +
>  ### Fixed-Point Load Instructions
>  
>  LBZ             100010 ..... ..... ................     @D
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 9db9d1e070..9d29134fa1 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -2843,13 +2843,6 @@ static void gen_mcrxrx(DisasContext *ctx)
>  }
>  #endif
>  
> -/* mfmsr */
> -static void gen_mfmsr(DisasContext *ctx)
> -{
> -    CHK_SV(ctx);
> -    tcg_gen_mov_tl(cpu_gpr[rD(ctx->opcode)], cpu_msr);
> -}
> -
>  /* mfspr */
>  static inline void gen_op_mfspr(DisasContext *ctx)
>  {
> @@ -2923,93 +2916,6 @@ static void gen_mftb(DisasContext *ctx)
>      gen_op_mfspr(ctx);
>  }
>  
> -/* mtmsr */
> -#if defined(TARGET_PPC64)
> -static void gen_mtmsrd(DisasContext *ctx)
> -{
> -    if (unlikely(!is_book3s_arch2x(ctx))) {
> -        gen_invalid(ctx);
> -        return;
> -    }
> -
> -    CHK_SV(ctx);
> -
> -#if !defined(CONFIG_USER_ONLY)
> -    TCGv t0, t1;
> -    target_ulong mask;
> -
> -    t0 = tcg_temp_new();
> -    t1 = tcg_temp_new();
> -
> -    translator_io_start(&ctx->base);
> -
> -    if (ctx->opcode & 0x00010000) {
> -        /* L=1 form only updates EE and RI */
> -        mask = (1ULL << MSR_RI) | (1ULL << MSR_EE);
> -    } else {
> -        /* mtmsrd does not alter HV, S, ME, or LE */
> -        mask = ~((1ULL << MSR_LE) | (1ULL << MSR_ME) | (1ULL << MSR_S) |
> -                 (1ULL << MSR_HV));
> -        /*
> -         * XXX: we need to update nip before the store if we enter
> -         *      power saving mode, we will exit the loop directly from
> -         *      ppc_store_msr
> -         */
> -        gen_update_nip(ctx, ctx->base.pc_next);
> -    }
> -
> -    tcg_gen_andi_tl(t0, cpu_gpr[rS(ctx->opcode)], mask);
> -    tcg_gen_andi_tl(t1, cpu_msr, ~mask);
> -    tcg_gen_or_tl(t0, t0, t1);
> -
> -    gen_helper_store_msr(tcg_env, t0);
> -
> -    /* Must stop the translation as machine state (may have) changed */
> -    ctx->base.is_jmp = DISAS_EXIT_UPDATE;
> -#endif /* !defined(CONFIG_USER_ONLY) */
> -}
> -#endif /* defined(TARGET_PPC64) */
> -
> -static void gen_mtmsr(DisasContext *ctx)
> -{
> -    CHK_SV(ctx);
> -
> -#if !defined(CONFIG_USER_ONLY)
> -    TCGv t0, t1;
> -    target_ulong mask = 0xFFFFFFFF;
> -
> -    t0 = tcg_temp_new();
> -    t1 = tcg_temp_new();
> -
> -    translator_io_start(&ctx->base);
> -    if (ctx->opcode & 0x00010000) {
> -        /* L=1 form only updates EE and RI */
> -        mask &= (1ULL << MSR_RI) | (1ULL << MSR_EE);
> -    } else {
> -        if (likely(!(ctx->insns_flags2 & PPC2_PPE42))) {
> -            /* mtmsr does not alter S, ME, or LE */
> -            mask &= ~((1ULL << MSR_LE) | (1ULL << MSR_ME) | (1ULL << MSR_S));
> -        }
> -
> -        /*
> -         * XXX: we need to update nip before the store if we enter
> -         *      power saving mode, we will exit the loop directly from
> -         *      ppc_store_msr
> -         */
> -        gen_update_nip(ctx, ctx->base.pc_next);
> -    }
> -
> -    tcg_gen_andi_tl(t0, cpu_gpr[rS(ctx->opcode)], mask);
> -    tcg_gen_andi_tl(t1, cpu_msr, ~mask);
> -    tcg_gen_or_tl(t0, t0, t1);
> -
> -    gen_helper_store_msr(tcg_env, t0);
> -
> -    /* Must stop the translation as machine state (may have) changed */
> -    ctx->base.is_jmp = DISAS_EXIT_UPDATE;
> -#endif
> -}
> -
>  /* mtspr */
>  static void gen_mtspr(DisasContext *ctx)
>  {
> @@ -4687,15 +4593,12 @@ GEN_HANDLER(hrfid, 0x13, 0x12, 0x08, 0x03FF8001, PPC_64H),
>  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(mfmsr, 0x1F, 0x13, 0x02, 0x001FF801, PPC_MISC),
>  GEN_HANDLER(mfspr, 0x1F, 0x13, 0x0A, 0x00000001, PPC_MISC),
>  GEN_HANDLER(mftb, 0x1F, 0x13, 0x0B, 0x00000001, PPC_MFTB),
>  #if defined(TARGET_PPC64)
> -GEN_HANDLER(mtmsrd, 0x1F, 0x12, 0x05, 0x001EF801, PPC_64B),
>  GEN_HANDLER_E(setb, 0x1F, 0x00, 0x04, 0x0003F801, PPC_NONE, PPC2_ISA300),
>  GEN_HANDLER_E(mcrxrx, 0x1F, 0x00, 0x12, 0x007FF801, PPC_NONE, PPC2_ISA300),
>  #endif
> -GEN_HANDLER(mtmsr, 0x1F, 0x12, 0x04, 0x001EF801, PPC_MISC),
>  GEN_HANDLER(mtspr, 0x1F, 0x13, 0x0E, 0x00000000, PPC_MISC),
>  GEN_HANDLER(mfsr, 0x1F, 0x13, 0x12, 0x0010F801, PPC_SEGMENT),
>  GEN_HANDLER(mfsrin, 0x1F, 0x13, 0x14, 0x001F0001, PPC_SEGMENT),
> diff --git a/target/ppc/translate/misc-impl.c.inc b/target/ppc/translate/misc-impl.c.inc
> index 890fbb209e..54712f9b73 100644
> --- a/target/ppc/translate/misc-impl.c.inc
> +++ b/target/ppc/translate/misc-impl.c.inc
> @@ -161,3 +161,67 @@ static bool trans_MCRF(DisasContext *ctx, arg_MCRF *a)
>      tcg_gen_mov_i32(cpu_crf[a->bf], cpu_crf[a->bfa]);
>      return true;
>  }
> +
> +static bool do_mfmsr(DisasContext *ctx, arg_X_t *a)
> +{
> +    REQUIRE_SV(ctx);
> +    tcg_gen_mov_tl(cpu_gpr[a->rt], cpu_msr);
> +    return true;
> +}
> +TRANS_FLAGS(MISC, MFMSR, do_mfmsr)
> +
> +static bool do_mtmsr(DisasContext *ctx, arg_X_rs_l *a, bool is_mtmsrd)
> +{
> +    REQUIRE_SV(ctx);
> +#if !defined(CONFIG_USER_ONLY)
> +    TCGv t0, t1;
> +    target_ulong mask;
> +
> +    if (is_mtmsrd) {
> +        if (unlikely(!is_book3s_arch2x(ctx))) {
> +            gen_invalid(ctx);
> +            return true;
> +        }
> +        mask = ~(target_ulong)0;
> +    } else {
> +        mask = UINT32_MAX;
> +    }
> +
> +    t0 = tcg_temp_new();
> +    t1 = tcg_temp_new();
> +    translator_io_start(&ctx->base);
> +
> +    if (a->l) {
> +        /* L=1 form only updates EE and RI */
> +        mask &= (1ULL << MSR_RI) | (1ULL << MSR_EE);
> +    } else {
> +        if (is_mtmsrd) {
> +            /* mtmsrd does not alter HV, S, ME, or LE */
> +            mask &= ~((1ULL << MSR_LE) | (1ULL << MSR_ME) |
> +                      (1ULL << MSR_S)  | (1ULL << MSR_HV));
> +        } else if (likely(!(ctx->insns_flags2 & PPC2_PPE42))) {
> +            /* mtmsr does not alter S, ME, or LE */
> +            mask &= ~((1ULL << MSR_LE) | (1ULL << MSR_ME) | (1ULL << MSR_S));
> +        }
> +        /*
> +         * XXX: we need to update nip before the store if we enter
> +         *      power saving mode, we will exit the loop directly from
> +         *      ppc_store_msr
> +         */
> +        gen_update_nip(ctx, ctx->base.pc_next);
> +    }
> +
> +    tcg_gen_andi_tl(t0, cpu_gpr[a->rs], mask);
> +    tcg_gen_andi_tl(t1, cpu_msr, ~mask);
> +    tcg_gen_or_tl(t0, t0, t1);
> +
> +    gen_helper_store_msr(tcg_env, t0);
> +
> +    /* Must stop the translation as machine state (may have) changed */
> +    ctx->base.is_jmp = DISAS_EXIT_UPDATE;
> +#endif /* !CONFIG_USER_ONLY */
> +    return true;
> +}
> +
> +TRANS_FLAGS(MISC, MTMSR, do_mtmsr, false)
> +TRANS64(MTMSRD, do_mtmsr, true)