Re: [PATCH-for-10.1 4/4] tcg: Convert TARGET_SUPPORTS_MTTCG to TCGCPUOps::mttcg_supported field

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 23/3/25 20:07, Richard Henderson wrote:
> On 3/21/25 08:59, Philippe Mathieu-Daudé wrote:
>> Instead of having a compile-time TARGET_SUPPORTS_MTTCG definition,
>> have each target set the 'mttcg_supported' field in the TCGCPUOps
>> structure.
>>
>> Since so far we only emulate one target architecture at a time,
>> tcg_init_machine() gets whether MTTCG is supported via the
>> &first_cpu global.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   docs/devel/multi-thread-tcg.rst          | 2 +-
>>   configs/targets/aarch64-softmmu.mak      | 1 -
>>   configs/targets/alpha-softmmu.mak        | 1 -
>>   configs/targets/arm-softmmu.mak          | 1 -
>>   configs/targets/hppa-softmmu.mak         | 1 -
>>   configs/targets/i386-softmmu.mak         | 1 -
>>   configs/targets/loongarch64-softmmu.mak  | 1 -
>>   configs/targets/microblaze-softmmu.mak   | 1 -
>>   configs/targets/microblazeel-softmmu.mak | 1 -
>>   configs/targets/mips-softmmu.mak         | 1 -
>>   configs/targets/mipsel-softmmu.mak       | 1 -
>>   configs/targets/or1k-softmmu.mak         | 1 -
>>   configs/targets/ppc64-softmmu.mak        | 1 -
>>   configs/targets/riscv32-softmmu.mak      | 1 -
>>   configs/targets/riscv64-softmmu.mak      | 1 -
>>   configs/targets/s390x-softmmu.mak        | 1 -
>>   configs/targets/sparc-softmmu.mak        | 1 -
>>   configs/targets/sparc64-softmmu.mak      | 1 -
>>   configs/targets/x86_64-softmmu.mak       | 1 -
>>   configs/targets/xtensa-softmmu.mak       | 1 -
>>   configs/targets/xtensaeb-softmmu.mak     | 1 -
>>   include/accel/tcg/cpu-ops.h              | 8 ++++++++
>>   include/exec/poison.h                    | 1 -
>>   accel/tcg/tcg-all.c                      | 7 ++-----
>>   target/alpha/cpu.c                       | 1 +
>>   target/arm/cpu.c                         | 1 +
>>   target/arm/tcg/cpu-v7m.c                 | 1 +
>>   target/avr/cpu.c                         | 1 +
>>   target/hexagon/cpu.c                     | 1 +
>>   target/hppa/cpu.c                        | 1 +
>>   target/i386/tcg/tcg-cpu.c                | 1 +
>>   target/loongarch/cpu.c                   | 1 +
>>   target/m68k/cpu.c                        | 1 +
>>   target/microblaze/cpu.c                  | 1 +
>>   target/mips/cpu.c                        | 1 +
>>   target/openrisc/cpu.c                    | 1 +
>>   target/ppc/cpu_init.c                    | 1 +
>>   target/riscv/tcg/tcg-cpu.c               | 1 +
>>   target/rx/cpu.c                          | 1 +
>>   target/s390x/cpu.c                       | 1 +
>>   target/sh4/cpu.c                         | 1 +
>>   target/sparc/cpu.c                       | 1 +
>>   target/tricore/cpu.c                     | 1 +
>>   target/xtensa/cpu.c                      | 1 +
>>   44 files changed, 31 insertions(+), 27 deletions(-)
> 
> As far as this goes,
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 
>> @@ -91,11 +92,7 @@ static int tcg_init_machine(MachineState *ms)
>>   #else
>>       unsigned max_cpus = ms->smp.max_cpus;
>>   #endif
>> -#ifdef TARGET_SUPPORTS_MTTCG
>> -    bool mttcg_supported = true;
>> -#else
>> -    bool mttcg_supported = false;
>> -#endif
>> +    bool mttcg_supported = first_cpu->cc->tcg_ops->mttcg_supported; / 
>> * FIXME avoid first_cpu */
> 
> Intersection of CPU_FOREACH?

Short term I used CPU_RESOLVING_TYPE:

     CPUClass *cc = CPU_CLASS(object_class_by_name(CPU_RESOLVING_TYPE));
     mttcg_supported = cc->tcg_ops->mttcg_supported;

I'll move to some helper.

Long term we need the intersection. I'll add a comment for now.

>> diff --git a/target/avr/cpu.c b/target/avr/cpu.c
>> index 24e52e28f44..9bc921c1cb2 100644
>> --- a/target/avr/cpu.c
>> +++ b/target/avr/cpu.c
>> @@ -216,6 +216,7 @@ static const TCGCPUOps avr_tcg_ops = {
>>       .cpu_exec_halt = avr_cpu_has_work,
>>       .tlb_fill = avr_cpu_tlb_fill,
>>       .do_interrupt = avr_cpu_do_interrupt,
>> +    .mttcg_supported = false,
>>       .guest_default_memory_order = 0,
>>   };
> 
> This may well be the only true not supported.
> I'm not sure what needs to happen to make that true.

I'd postpone that for later ;)

>> diff --git a/target/hexagon/cpu.c b/target/hexagon/cpu.c
>> index 34734b0edb0..7bcf297998f 100644
>> --- a/target/hexagon/cpu.c
>> +++ b/target/hexagon/cpu.c
>> @@ -324,6 +324,7 @@ static const TCGCPUOps hexagon_tcg_ops = {
>>       .translate_code = hexagon_translate_code,
>>       .synchronize_from_tb = hexagon_cpu_synchronize_from_tb,
>>       .restore_state_to_opc = hexagon_restore_state_to_opc,
>> +    .mttcg_supported = false,
>>       /* MTTCG not yet supported: require strict ordering */
>>       .guest_default_memory_order = TCG_MO_ALL,
> 
> We now have strict ordering, right there with TCG_MO_ALL.
> Likewise for m68k, sh4, tricore.
> I think we simply forgot to update.

Doh... I'll clean in a preliminary patch, thanks.

>> diff --git a/target/mips/cpu.c b/target/mips/cpu.c
>> index 5ddc9bbb829..eee604e9254 100644
>> --- a/target/mips/cpu.c
>> +++ b/target/mips/cpu.c
>> @@ -554,6 +554,7 @@ static const TCGCPUOps mips_tcg_ops = {
>>       .synchronize_from_tb = mips_cpu_synchronize_from_tb,
>>       .restore_state_to_opc = mips_restore_state_to_opc,
>> +    .mttcg_supported = TARGET_LONG_BITS == 32,
>>       .guest_default_memory_order = 0,
> 
> What is missing from mip64?

See:

commit a092a95547710bcbb8c168e5ec03f55fb4ab1ad7
Author: Alex Bennée <alex.bennee@linaro.org>
Date:   Mon Mar 23 16:15:09 2020 +0000

     configure: disable MTTCG for MIPS guests

     While debugging check-acceptance failures I found an instability in
     the mips64el test case. Briefly the test case:

       retry.py -n 100 -c -- ./mips64el-softmmu/qemu-system-mips64el \
         -display none -vga none -serial mon:stdio \
         -machine malta -kernel ./vmlinux-4.7.0-rc1.I6400 \
         -cpu I6400 -smp 8 -vga std \
         -append "printk.time=0 clocksource=GIC console=tty0 " \
                 "console=ttyS0 panic=-1" \
         --no-reboot

     Reports about a 9% failure rate:

       Results summary:
       0: 91 times (91.00%), avg time 5.547 (0.45 varience/0.67 deviation)
       -6: 9 times (9.00%), avg time 3.394 (0.02 varience/0.13 deviation)
       Ran command 100 times, 91 passes

     When re-run with "--accel tcg,thread=single" the instability goes
     away.

       Results summary:
       0: 100 times (100.00%), avg time 17.318 (249.76 varience/15.80 
deviation)
       Ran command 100 times, 100 passes

     Which seems to indicate there is some aspect of the MIPS MTTCG fixes
     that has been missed. Ideally we would fix that but I'm afraid I
     don't have time to investigate and am not super familiar with the
     architecture anyway. In lieu of someone tracking down the failure
     lets disable it for now.

> 
>> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
>> index 28fbbb8d3c1..ed79cc1a6b7 100644
>> --- a/target/ppc/cpu_init.c
>> +++ b/target/ppc/cpu_init.c
>> @@ -7490,6 +7490,7 @@ static const TCGCPUOps ppc_tcg_ops = {
>>     .translate_code = ppc_translate_code,
>>     .restore_state_to_opc = ppc_restore_state_to_opc,
>> +  .mttcg_supported = TARGET_LONG_BITS == 64,
>>     .guest_default_memory_order = 0,
> 
> Similarly.  I'd be surprised if ppc32 can't use mttcg, really.

Per Cédric on IRC our ppc32 implementations are single core,
so never tested for mttcg.

Regards,

Phil.