Cédric Le Goater <c...@kaod.org> writes:
> On 4/14/20 1:11 PM, Nicholas Piggin wrote: >> If mtmsr L=1 sets MSR[EE] while there is a maskable exception pending, >> it does not cause an interrupt. This causes the test case to hang: >> >> https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Dppc_2019-2D10_msg00826.html&d=DwIDAg&c=jf_iaSHvJObTbx-siA1ZOg&r=XHJsZhhuWSw9713Fp0ciew&m=TQfi_v-8XYgz7MiMDAZ_CjkyalSh9-EXhQ3oDesUm74&s=pFoesXbioVBh5wCuzEnzwgfze6X7e-a9unkfUgsRwiw&e= >> >> <snip> > I was expecting more changes but this looks fine. > > Reviewed-by: Cédric Le Goater <c...@kaod.org> > >> Cc: qemu-sta...@nongnu.org >> Reported-by: Anton Blanchard <an...@ozlabs.org> >> Reported-by: Nathan Chancellor <natechancel...@gmail.com> >> Tested-by: Nathan Chancellor <natechancel...@gmail.com> >> Signed-off-by: Nicholas Piggin <npig...@gmail.com> > > I gave it a try on PowerNV, pseries and mac99. All good. > > Tested-by: Cédric Le Goater <c...@kaod.org> > > I don't know how we could include tests in QEMU such as the one Anton > sent. These are good exercisers for our exception model. It certainly looks possible. The ideal would be a fresh boot.S for ppc64 that supported: a) some sort of serial output b) a way to exit the test case with a result For ARM we use semihosting, for x86 we use plain ioport and an ACPI exit. See the tests in: tests/tcg/[x86_64/aarch64]/system/boot.S and the Makefile.softmmu-target files under tests/tcg. > > Thanks, > > C. > >> --- >> Thanks very much to Nathan for reporting and testing it, I added his >> Tested-by tag despite a more polished patch, as the the basics are >> still the same (and still fixes his test case here). >> >> This bug possibly goes back to early v2.04 / mtmsrd L=1 support around >> 2007, and the code has been changed several times since then so may >> require some backporting. >> >> 32-bit / mtmsr untested at the moment, I don't have an environment >> handy. >> >> >> target/ppc/translate.c | 46 +++++++++++++++++++++++++----------------- >> 1 file changed, 27 insertions(+), 19 deletions(-) >> >> diff --git a/target/ppc/translate.c b/target/ppc/translate.c >> index b207fb5386..9959259dba 100644 >> --- a/target/ppc/translate.c >> +++ b/target/ppc/translate.c >> @@ -4361,30 +4361,34 @@ static void gen_mtmsrd(DisasContext *ctx) >> CHK_SV; >> >> #if !defined(CONFIG_USER_ONLY) >> + if (tb_cflags(ctx->base.tb) & CF_USE_ICOUNT) { >> + gen_io_start(); >> + } >> if (ctx->opcode & 0x00010000) { >> - /* Special form that does not need any synchronisation */ >> + /* 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(cpu_msr, cpu_msr, >> + tcg_gen_andi_tl(t1, cpu_msr, >> ~(target_ulong)((1 << MSR_RI) | (1 << MSR_EE))); >> - tcg_gen_or_tl(cpu_msr, cpu_msr, t0); >> + tcg_gen_or_tl(t1, t1, t0); >> + >> + gen_helper_store_msr(cpu_env, t1); >> tcg_temp_free(t0); >> + tcg_temp_free(t1); >> + >> } else { >> /* >> * 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 >> */ >> - if (tb_cflags(ctx->base.tb) & CF_USE_ICOUNT) { >> - gen_io_start(); >> - } >> gen_update_nip(ctx, ctx->base.pc_next); >> gen_helper_store_msr(cpu_env, 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); >> } >> + /* Must stop the translation as machine state (may have) changed */ >> + gen_stop_exception(ctx); >> #endif /* !defined(CONFIG_USER_ONLY) */ >> } >> #endif /* defined(TARGET_PPC64) */ >> @@ -4394,15 +4398,23 @@ static void gen_mtmsr(DisasContext *ctx) >> CHK_SV; >> >> #if !defined(CONFIG_USER_ONLY) >> - if (ctx->opcode & 0x00010000) { >> - /* Special form that does not need any synchronisation */ >> + if (tb_cflags(ctx->base.tb) & CF_USE_ICOUNT) { >> + gen_io_start(); >> + } >> + 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(cpu_msr, cpu_msr, >> + tcg_gen_andi_tl(t1, cpu_msr, >> ~(target_ulong)((1 << MSR_RI) | (1 << MSR_EE))); >> - tcg_gen_or_tl(cpu_msr, cpu_msr, t0); >> + tcg_gen_or_tl(t1, t1, t0); >> + >> + gen_helper_store_msr(cpu_env, t1); >> tcg_temp_free(t0); >> + tcg_temp_free(t1); >> + >> } else { >> TCGv msr = tcg_temp_new(); >> >> @@ -4411,9 +4423,6 @@ static void gen_mtmsr(DisasContext *ctx) >> * power saving mode, we will exit the loop directly from >> * ppc_store_msr >> */ >> - if (tb_cflags(ctx->base.tb) & CF_USE_ICOUNT) { >> - gen_io_start(); >> - } >> 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); >> @@ -4422,10 +4431,9 @@ static void gen_mtmsr(DisasContext *ctx) >> #endif >> gen_helper_store_msr(cpu_env, msr); >> tcg_temp_free(msr); >> - /* Must stop the translation as machine state (may have) changed */ >> - /* Note that mtmsr is not always defined as context-synchronizing */ >> - gen_stop_exception(ctx); >> } >> + /* Must stop the translation as machine state (may have) changed */ >> + gen_stop_exception(ctx); >> #endif >> } >> >> -- Alex Bennée