target/ppc/cpu.h | 1 + target/ppc/translate.c | 73 +++++++++++++++++++++++------------------- 2 files changed, 41 insertions(+), 33 deletions(-)
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>
---
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
}
--
2.25.1
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
© 2016 - 2024 Red Hat, Inc.