Message ID | 4DF4E00D.4030101@freebsd.org |
---|---|
State | New |
Headers | show |
Am 12.06.2011 um 17:49 schrieb Nathan Whitehorn <nwhitehorn@freebsd.org>: > The mtmsr instruction is required not to modify the upper 32-bits of the machine state register, but checks the current value of MSR[SF] to decide whether to do this. This has the effect of zeroing the upper 32 bits of the MSR whenever mtmsr is executed in 64-bit mode. Unconditionally preserve the upper 32-bits in mtmsr for TARGET_PPC64. > > Signed-off-by: Nathan Whitehorn <nwhitehorn@freebsd.org> > --- > target-ppc/translate.c | 5 ++--- > 1 files changed, 2 insertions(+), 3 deletions(-) > > diff --git a/target-ppc/translate.c b/target-ppc/translate.c > index 59aef85..38d2e2e 100644 > --- a/target-ppc/translate.c > +++ b/target-ppc/translate.c > @@ -3884,18 +3884,17 @@ static void gen_mtmsr(DisasContext *ctx) > */ > gen_update_nip(ctx, ctx->nip); > #if defined(TARGET_PPC64) > - if (!ctx->sf_mode) { > - TCGv t0 = tcg_temp_new(); > - TCGv t1 = tcg_temp_new(); > - tcg_gen_andi_tl(t0, cpu_msr, 0xFFFFFFFF00000000ULL); > - tcg_gen_ext32u_tl(t1, cpu_gpr[rS(ctx->opcode)]); > - tcg_gen_or_tl(t0, t0, t1); > - tcg_temp_free(t1); > - gen_helper_store_msr(t0); > - tcg_temp_free(t0); > - } else > + TCGv t0 = tcg_temp_new(); > + TCGv t1 = tcg_temp_new(); You're declaring variables in mid-scope. Please open a new scope :). > + tcg_gen_andi_tl(t0, cpu_msr, 0xFFFFFFFF00000000ULL); > + tcg_gen_ext32u_tl(t1, cpu_gpr[rS(ctx->opcode)]); > + tcg_gen_or_tl(t0, t0, t1); While at it, this is a perfect scenario for the deposit tcg op! :) If you feel like this is too cumbersome work for such a small patch, please let me know and I'll do the changes for you :) Alex > + tcg_temp_free(t1); > + gen_helper_store_msr(t0); > + tcg_temp_free(t0); > +#else > + gen_helper_store_msr(cpu_gpr[rS(ctx->opcode)]); > #endif > - gen_helper_store_msr(cpu_gpr[rS(ctx->opcode)]); > /* Must stop the translation as machine state (may have) changed */ > /* Note that mtmsr is not always defined as context-synchronizing */ > gen_stop_exception(ctx); >
On 06/13/11 05:20, Alexander Graf wrote: > > > > Am 12.06.2011 um 17:49 schrieb Nathan Whitehorn<nwhitehorn@freebsd.org>: > >> The mtmsr instruction is required not to modify the upper 32-bits of the machine state register, but checks the current value of MSR[SF] to decide whether to do this. This has the effect of zeroing the upper 32 bits of the MSR whenever mtmsr is executed in 64-bit mode. Unconditionally preserve the upper 32-bits in mtmsr for TARGET_PPC64. >> >> Signed-off-by: Nathan Whitehorn<nwhitehorn@freebsd.org> >> --- >> target-ppc/translate.c | 5 ++--- >> 1 files changed, 2 insertions(+), 3 deletions(-) >> >> diff --git a/target-ppc/translate.c b/target-ppc/translate.c >> index 59aef85..38d2e2e 100644 >> --- a/target-ppc/translate.c >> +++ b/target-ppc/translate.c >> @@ -3884,18 +3884,17 @@ static void gen_mtmsr(DisasContext *ctx) >> */ >> gen_update_nip(ctx, ctx->nip); >> #if defined(TARGET_PPC64) >> - if (!ctx->sf_mode) { >> - TCGv t0 = tcg_temp_new(); >> - TCGv t1 = tcg_temp_new(); >> - tcg_gen_andi_tl(t0, cpu_msr, 0xFFFFFFFF00000000ULL); >> - tcg_gen_ext32u_tl(t1, cpu_gpr[rS(ctx->opcode)]); >> - tcg_gen_or_tl(t0, t0, t1); >> - tcg_temp_free(t1); >> - gen_helper_store_msr(t0); >> - tcg_temp_free(t0); >> - } else >> + TCGv t0 = tcg_temp_new(); >> + TCGv t1 = tcg_temp_new(); > You're declaring variables in mid-scope. Please open a new scope :). Does the gen_update_nip(ctx, ctx->nip); need to be first here? If not, we can just move it to the end and avoid the scoping issue. >> + tcg_gen_andi_tl(t0, cpu_msr, 0xFFFFFFFF00000000ULL); >> + tcg_gen_ext32u_tl(t1, cpu_gpr[rS(ctx->opcode)]); >> + tcg_gen_or_tl(t0, t0, t1); > While at it, this is a perfect scenario for the deposit tcg op! :) > > If you feel like this is too cumbersome work for such a small patch, please let me know and I'll do the changes for you :) I have no idea what that is, so I'd prefer you do it :) -Nathan
On 13.06.2011, at 14:52, Nathan Whitehorn wrote: > On 06/13/11 05:20, Alexander Graf wrote: >> >> >> >> Am 12.06.2011 um 17:49 schrieb Nathan Whitehorn<nwhitehorn@freebsd.org>: >> >>> The mtmsr instruction is required not to modify the upper 32-bits of the machine state register, but checks the current value of MSR[SF] to decide whether to do this. This has the effect of zeroing the upper 32 bits of the MSR whenever mtmsr is executed in 64-bit mode. Unconditionally preserve the upper 32-bits in mtmsr for TARGET_PPC64. >>> >>> Signed-off-by: Nathan Whitehorn<nwhitehorn@freebsd.org> >>> --- >>> target-ppc/translate.c | 5 ++--- >>> 1 files changed, 2 insertions(+), 3 deletions(-) >>> >>> diff --git a/target-ppc/translate.c b/target-ppc/translate.c >>> index 59aef85..38d2e2e 100644 >>> --- a/target-ppc/translate.c >>> +++ b/target-ppc/translate.c >>> @@ -3884,18 +3884,17 @@ static void gen_mtmsr(DisasContext *ctx) >>> */ >>> gen_update_nip(ctx, ctx->nip); >>> #if defined(TARGET_PPC64) >>> - if (!ctx->sf_mode) { >>> - TCGv t0 = tcg_temp_new(); >>> - TCGv t1 = tcg_temp_new(); >>> - tcg_gen_andi_tl(t0, cpu_msr, 0xFFFFFFFF00000000ULL); >>> - tcg_gen_ext32u_tl(t1, cpu_gpr[rS(ctx->opcode)]); >>> - tcg_gen_or_tl(t0, t0, t1); >>> - tcg_temp_free(t1); >>> - gen_helper_store_msr(t0); >>> - tcg_temp_free(t0); >>> - } else >>> + TCGv t0 = tcg_temp_new(); >>> + TCGv t1 = tcg_temp_new(); >> You're declaring variables in mid-scope. Please open a new scope :). > > Does the gen_update_nip(ctx, ctx->nip); need to be first here? If not, we can just move it to the end and avoid the scoping issue. It makes sure that we know the current instruction pointer in case an interrupt gets delivered for example. There's a hack to not require this for data protection interrupts, but others might. In general, we need this whenever we call a helper function that could manually raise an interrupt. helper_store_msr does this: void helper_store_msr (target_ulong val) { val = hreg_store_msr(env, val, 0); if (val != 0) { env->interrupt_request |= CPU_INTERRUPT_EXITTB; helper_raise_exception(val); } } So it needs to come before any call to gen_helper_store_msr :). >>> + tcg_gen_andi_tl(t0, cpu_msr, 0xFFFFFFFF00000000ULL); >>> + tcg_gen_ext32u_tl(t1, cpu_gpr[rS(ctx->opcode)]); >>> + tcg_gen_or_tl(t0, t0, t1); >> While at it, this is a perfect scenario for the deposit tcg op! :) >> >> If you feel like this is too cumbersome work for such a small patch, please let me know and I'll do the changes for you :) > > I have no idea what that is, so I'd prefer you do it :) It's a new tcg call that makes the above code a lot easier. Do you want to go for v3 without changing deposit then? That would be awesome :). Alex
diff --git a/target-ppc/translate.c b/target-ppc/translate.c index 59aef85..38d2e2e 100644 --- a/target-ppc/translate.c +++ b/target-ppc/translate.c @@ -3884,18 +3884,17 @@ static void gen_mtmsr(DisasContext *ctx) */ gen_update_nip(ctx, ctx->nip); #if defined(TARGET_PPC64) - if (!ctx->sf_mode) { - TCGv t0 = tcg_temp_new(); - TCGv t1 = tcg_temp_new(); - tcg_gen_andi_tl(t0, cpu_msr, 0xFFFFFFFF00000000ULL); - tcg_gen_ext32u_tl(t1, cpu_gpr[rS(ctx->opcode)]); - tcg_gen_or_tl(t0, t0, t1); - tcg_temp_free(t1); - gen_helper_store_msr(t0); - tcg_temp_free(t0); - } else + TCGv t0 = tcg_temp_new(); + TCGv t1 = tcg_temp_new(); + tcg_gen_andi_tl(t0, cpu_msr, 0xFFFFFFFF00000000ULL); + tcg_gen_ext32u_tl(t1, cpu_gpr[rS(ctx->opcode)]); + tcg_gen_or_tl(t0, t0, t1); + tcg_temp_free(t1); + gen_helper_store_msr(t0); + tcg_temp_free(t0); +#else + gen_helper_store_msr(cpu_gpr[rS(ctx->opcode)]); #endif - gen_helper_store_msr(cpu_gpr[rS(ctx->opcode)]); /* Must stop the translation as machine state (may have) changed */ /* Note that mtmsr is not always defined as
The mtmsr instruction is required not to modify the upper 32-bits of the machine state register, but checks the current value of MSR[SF] to decide whether to do this. This has the effect of zeroing the upper 32 bits of the MSR whenever mtmsr is executed in 64-bit mode. Unconditionally preserve the upper 32-bits in mtmsr for TARGET_PPC64. Signed-off-by: Nathan Whitehorn <nwhitehorn@freebsd.org> --- target-ppc/translate.c | 5 ++--- 1 files changed, 2 insertions(+), 3 deletions(-) context-synchronizing */ gen_stop_exception(ctx);