Re: [EXTERNAL] [PATCH] target/ppc: Fix mtmsr(d) L=1 variant that loses interrupts

[Alex Bennée]

Cédric Le Goater <address@hidden> 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 <address@hidden>
>
>> Cc: address@hidden
>> Reported-by: Anton Blanchard <address@hidden>
>> Reported-by: Nathan Chancellor <address@hidden>
>> Tested-by: Nathan Chancellor <address@hidden>
>> Signed-off-by: Nicholas Piggin <address@hidden>
>
> I gave it a try on PowerNV, pseries and mac99. All good.
>
> Tested-by: Cédric Le Goater <address@hidden>
>
> 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