Re: [PATCH] target/ppc: Filter mtmsr[d] input before setting MSR

[David Gibson]

On Fri, Oct 15, 2021 at 03:19:40PM -0300, matheus.ferst@eldorado.org.br wrote:
> From: Matheus Ferst <matheus.ferst@eldorado.org.br>
> 
> PowerISA says that mtmsr[d] "does not alter MSR[HV], MSR[S], MSR[ME], or
> MSR[LE]", but the current code only filters the GPR-provided value if
> L=1. This behavior caused some problems in FreeBSD, and a build option
> was added to work around the issue [1], but it seems that the bug was
> not reported in launchpad/gitlab. This patch address the issue in qemu,
> so the option on FreeBSD should no longer be required.
> 
> [1] 
> https://cgit.freebsd.org/src/commit/?id=4efb1ca7d2a44cfb33d7f9e18bd92f8d68dcfee0
> 
> Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>

Applied to ppc-for-6.2, thanks.

> ---
>  target/ppc/cpu.h       |  1 +
>  target/ppc/translate.c | 73 +++++++++++++++++++++++-------------------
>  2 files changed, 41 insertions(+), 33 deletions(-)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index baa4e7c34d..e94e82297b 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -314,6 +314,7 @@ typedef struct ppc_v3_pate_t {
>  #define MSR_AP   23 /* Access privilege state on 602                  hflags 
> */
>  #define MSR_VSX  23 /* Vector Scalar Extension (ISA 2.06 and later) x hflags 
> */
>  #define MSR_SA   22 /* Supervisor access mode on 602                  hflags 
> */
> +#define MSR_S    22 /* Secure state                                          
> */
>  #define MSR_KEY  19 /* key bit on 603e                                       
> */
>  #define MSR_POW  18 /* Power management                                      
> */
>  #define MSR_TGPR 17 /* TGPR usage on 602/603                        x        
> */
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index b985e9e55b..a5ebe03e2a 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -4940,32 +4940,40 @@ static void gen_mtmsrd(DisasContext *ctx)
>      CHK_SV;
>  
>  #if !defined(CONFIG_USER_ONLY)
> +    TCGv t0, t1;
> +    target_ulong mask;
> +
> +    t0 = tcg_temp_new();
> +    t1 = tcg_temp_new();
> +
>      gen_icount_io_start(ctx);
> +
>      if (ctx->opcode & 0x00010000) {
>          /* L=1 form only updates EE and RI */
> -        TCGv t0 = tcg_temp_new();
> -        TCGv t1 = tcg_temp_new();
> -        tcg_gen_andi_tl(t0, cpu_gpr[rS(ctx->opcode)],
> -                        (1 << MSR_RI) | (1 << MSR_EE));
> -        tcg_gen_andi_tl(t1, cpu_msr,
> -                        ~(target_ulong)((1 << MSR_RI) | (1 << MSR_EE)));
> -        tcg_gen_or_tl(t1, t1, t0);
> -
> -        gen_helper_store_msr(cpu_env, t1);
> -        tcg_temp_free(t0);
> -        tcg_temp_free(t1);
> -
> +        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);
> -        gen_helper_store_msr(cpu_env, cpu_gpr[rS(ctx->opcode)]);
>      }
> +
> +    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(cpu_env, t0);
> +
>      /* Must stop the translation as machine state (may have) changed */
>      ctx->base.is_jmp = DISAS_EXIT_UPDATE;
> +
> +    tcg_temp_free(t0);
> +    tcg_temp_free(t1);
>  #endif /* !defined(CONFIG_USER_ONLY) */
>  }
>  #endif /* defined(TARGET_PPC64) */
> @@ -4975,23 +4983,19 @@ static void gen_mtmsr(DisasContext *ctx)
>      CHK_SV;
>  
>  #if !defined(CONFIG_USER_ONLY)
> +    TCGv t0, t1;
> +    target_ulong mask = 0xFFFFFFFF;
> +
> +    t0 = tcg_temp_new();
> +    t1 = tcg_temp_new();
> +
>      gen_icount_io_start(ctx);
>      if (ctx->opcode & 0x00010000) {
>          /* L=1 form only updates EE and RI */
> -        TCGv t0 = tcg_temp_new();
> -        TCGv t1 = tcg_temp_new();
> -        tcg_gen_andi_tl(t0, cpu_gpr[rS(ctx->opcode)],
> -                        (1 << MSR_RI) | (1 << MSR_EE));
> -        tcg_gen_andi_tl(t1, cpu_msr,
> -                        ~(target_ulong)((1 << MSR_RI) | (1 << MSR_EE)));
> -        tcg_gen_or_tl(t1, t1, t0);
> -
> -        gen_helper_store_msr(cpu_env, t1);
> -        tcg_temp_free(t0);
> -        tcg_temp_free(t1);
> -
> +        mask &= (1ULL << MSR_RI) | (1ULL << MSR_EE);
>      } else {
> -        TCGv msr = tcg_temp_new();
> +        /* 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
> @@ -4999,16 +5003,19 @@ static void gen_mtmsr(DisasContext *ctx)
>           *      ppc_store_msr
>           */
>          gen_update_nip(ctx, ctx->base.pc_next);
> -#if defined(TARGET_PPC64)
> -        tcg_gen_deposit_tl(msr, cpu_msr, cpu_gpr[rS(ctx->opcode)], 0, 32);
> -#else
> -        tcg_gen_mov_tl(msr, cpu_gpr[rS(ctx->opcode)]);
> -#endif
> -        gen_helper_store_msr(cpu_env, msr);
> -        tcg_temp_free(msr);
>      }
> +
> +    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(cpu_env, t0);
> +
>      /* Must stop the translation as machine state (may have) changed */
>      ctx->base.is_jmp = DISAS_EXIT_UPDATE;
> +
> +    tcg_temp_free(t0);
> +    tcg_temp_free(t1);
>  #endif
>  }
>  

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature