Re: [PATCH] system: Convert qemu_arch_available() to TargetInfo API

[Markus Armbruster <armbru@redhat.com>]

Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:

> On 1/8/26 8:32 AM, Philippe Mathieu-Daudé wrote:
>> On 8/1/26 01:36, Pierrick Bouvier wrote:
>>> On 1/7/26 10:10 AM, Philippe Mathieu-Daudé wrote:
>>>> Get the base arch_mask from the current SysEmuTarget,
>>>> making qemu_arch_available() target-agnostic.
>>>>
>>>> We don't need the per-target QEMU_ARCH definition anymore,
>>>> remove it.
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[...]

>>>> @@ -33,6 +34,46 @@ SysEmuTarget target_arch(void)
>>>>        return arch;
>>>>    }
>>>> +bool qemu_arch_available(unsigned qemu_arch_mask)
>>>> +{
>>>> +    static const unsigned base_arch_mask[SYS_EMU_TARGET__MAX] = {
>>>> +        [SYS_EMU_TARGET_AARCH64]        = QEMU_ARCH_ARM,
>>>> +        [SYS_EMU_TARGET_ALPHA]          = QEMU_ARCH_ALPHA,
>>>> +        [SYS_EMU_TARGET_ARM]            = QEMU_ARCH_ARM,
>>>> +        [SYS_EMU_TARGET_AVR]            = QEMU_ARCH_AVR,
>>>> +        /*
>>>> +        [SYS_EMU_TARGET_HEXAGON]        = QEMU_ARCH_HEXAGON,
>>>> +        */
>>>> +        [SYS_EMU_TARGET_HPPA]           = QEMU_ARCH_HPPA,
>>>> +        [SYS_EMU_TARGET_I386]           = QEMU_ARCH_I386,
>>>> +        [SYS_EMU_TARGET_LOONGARCH64]    = QEMU_ARCH_LOONGARCH,
>>>> +        [SYS_EMU_TARGET_M68K]           = QEMU_ARCH_M68K,
>>>> +        [SYS_EMU_TARGET_MICROBLAZE]     = QEMU_ARCH_MICROBLAZE,
>>>> +        [SYS_EMU_TARGET_MICROBLAZEEL]   = QEMU_ARCH_MICROBLAZE,
>>>> +        [SYS_EMU_TARGET_MIPS]           = QEMU_ARCH_MIPS,
>>>> +        [SYS_EMU_TARGET_MIPS64]         = QEMU_ARCH_MIPS,
>>>> +        [SYS_EMU_TARGET_MIPS64EL]       = QEMU_ARCH_MIPS,
>>>> +        [SYS_EMU_TARGET_MIPSEL]         = QEMU_ARCH_MIPS,
>>>> +        [SYS_EMU_TARGET_OR1K]           = QEMU_ARCH_OPENRISC,
>>>> +        [SYS_EMU_TARGET_PPC]            = QEMU_ARCH_PPC,
>>>> +        [SYS_EMU_TARGET_PPC64]          = QEMU_ARCH_PPC,
>>>> +        [SYS_EMU_TARGET_RISCV32]        = QEMU_ARCH_RISCV,
>>>> +        [SYS_EMU_TARGET_RISCV64]        = QEMU_ARCH_RISCV,
>>>> +        [SYS_EMU_TARGET_RX]             = QEMU_ARCH_RX,
>>>> +        [SYS_EMU_TARGET_S390X]          = QEMU_ARCH_S390X,
>>>> +        [SYS_EMU_TARGET_SH4]            = QEMU_ARCH_SH4,
>>>> +        [SYS_EMU_TARGET_SH4EB]          = QEMU_ARCH_SH4,
>>>> +        [SYS_EMU_TARGET_SPARC]          = QEMU_ARCH_SPARC,
>>>> +        [SYS_EMU_TARGET_SPARC64]        = QEMU_ARCH_SPARC,
>>>> +        [SYS_EMU_TARGET_TRICORE]        = QEMU_ARCH_TRICORE,
>>>> +        [SYS_EMU_TARGET_X86_64]         = QEMU_ARCH_I386,
>>>> +        [SYS_EMU_TARGET_XTENSA]         = QEMU_ARCH_XTENSA,
>>>> +        [SYS_EMU_TARGET_XTENSAEB]       = QEMU_ARCH_XTENSA,
>>>> +    };
>>>> +
>>>
>>> Just a remark: is that worth having the "static const" array approach
>>> when we could have a proper switch than can be checked for
>>> exhaustiveness with compiler warnings instead?
>>
>> I thought 40 LoC would be simpler to review than 80, and it was
>> simpler to generate the template in bash.

I find the array solution much easier to grasp at a glance.  The
initializer shows the mapping concisely.  With a switch, there's noise
around the mapping, and the fact that all all switch cases do nothing
but assign a constant to the same variable is not obvious at a glance.

>> Besides, we don't use compiler warnings for enum (such -Wswitch)
>> due to QAPI:
>> https://lore.kernel.org/qemu-devel/20230315112811.22355-1-philmd@linaro.org/
>> 
>
> Reading the thread above, the only mention I find is the 3rd commit that precisely change definition because -Wswitch is enabled with clang.
>
> And it's not only a clang thing, gcc has it in Wall also [1].
>
> I don't mind the array approach, but maybe just add a *static* assert making sure (SYS_EMU_TARGET__MAX-1 == SYS_EMU_TARGET_XTENSAEB) so this file will break as soon as there is a new target added. It's simple and the next developer who won't have to debug this will thank you.

Won't help when new enum values are added in the middle.  We keep them
alphabetically sorted...

We could use a simple run-time assertion:

    arch_mask = qemu_arch_mask & base_arch_mask[target_arch()];
    assert(arch_mask);
    return arch_mask;

Works because the QEMU_ARCH_FOO are all non-zero.

Pick your poison.

> [1] https://gcc.gnu.org/onlinedocs/gcc-14.2.0/gcc/Warning-Options.html
>
>>>
>>> Beyond that,
>>> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>