Pierrick Bouvier <[email protected]> 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é <[email protected]>
[...]
>>>> @@ -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/[email protected]/
>>
>
> 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 <[email protected]>
