Re: [RFC PATCH 03/11] system: Introduce QemuArchBit enum

[Pierrick Bouvier <pierrick.bouvier@linaro.org>]

On 3/4/25 17:31, Philippe Mathieu-Daudé wrote:
> On 5/3/25 02:23, Pierrick Bouvier wrote:
>> On 3/4/25 16:52, Philippe Mathieu-Daudé wrote:
>>> Declare QEMU_ARCH_BIT_$target as QemuArchBit enum.
>>> Use them to declare QEMU_ARCH_$target bitmasks.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>    meson.build                |  4 +--
>>>    include/system/arch_init.h | 65 +++++++++++++++++++++++++-------------
>>>    system/arch_init.c         |  2 +-
>>>    3 files changed, 46 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/meson.build b/meson.build
>>> index 0a2c61d2bfa..1ab02a5d48d 100644
>>> --- a/meson.build
>>> +++ b/meson.build
>>> @@ -3357,8 +3357,8 @@ foreach target : target_dirs
>>>          config_target_data.set(k, v)
>>>        endif
>>>      endforeach
>>> -  config_target_data.set('QEMU_ARCH',
>>> -                         'QEMU_ARCH_' +
>>> config_target['TARGET_BASE_ARCH'].to_upper())
>>> +  config_target_data.set('QEMU_ARCH_BIT',
>>> +                         'QEMU_ARCH_BIT_' +
>>> config_target['TARGET_BASE_ARCH'].to_upper())
>>>      config_target_h += {target: configure_file(output: target + '-
>>> config-target.h',
>>>                                                   configuration:
>>> config_target_data)}
>>> diff --git a/include/system/arch_init.h b/include/system/arch_init.h
>>> index d8b77440487..06e5527ec88 100644
>>> --- a/include/system/arch_init.h
>>> +++ b/include/system/arch_init.h
>>> @@ -1,29 +1,50 @@
>>>    #ifndef QEMU_ARCH_INIT_H
>>>    #define QEMU_ARCH_INIT_H
>>> +#include "qemu/bitops.h"
>>> -enum {
>>> -    QEMU_ARCH_ALL = -1,
>>> -    QEMU_ARCH_ALPHA = (1 << 0),
>>> -    QEMU_ARCH_ARM = (1 << 1),
>>> -    QEMU_ARCH_I386 = (1 << 3),
>>> -    QEMU_ARCH_M68K = (1 << 4),
>>> -    QEMU_ARCH_MICROBLAZE = (1 << 6),
>>> -    QEMU_ARCH_MIPS = (1 << 7),
>>> -    QEMU_ARCH_PPC = (1 << 8),
>>> -    QEMU_ARCH_S390X = (1 << 9),
>>> -    QEMU_ARCH_SH4 = (1 << 10),
>>> -    QEMU_ARCH_SPARC = (1 << 11),
>>> -    QEMU_ARCH_XTENSA = (1 << 12),
>>> -    QEMU_ARCH_OPENRISC = (1 << 13),
>>> -    QEMU_ARCH_TRICORE = (1 << 16),
>>> -    QEMU_ARCH_HPPA = (1 << 18),
>>> -    QEMU_ARCH_RISCV = (1 << 19),
>>> -    QEMU_ARCH_RX = (1 << 20),
>>> -    QEMU_ARCH_AVR = (1 << 21),
>>> -    QEMU_ARCH_HEXAGON = (1 << 22),
>>> -    QEMU_ARCH_LOONGARCH = (1 << 23),
>>> -};
>>> +typedef enum QemuArchBit {
>>> +    QEMU_ARCH_BIT_ALPHA         = 0,
>>> +    QEMU_ARCH_BIT_ARM           = 1,
>>> +    QEMU_ARCH_BIT_I386          = 3,
>>> +    QEMU_ARCH_BIT_M68K          = 4,
>>> +    QEMU_ARCH_BIT_MICROBLAZE    = 6,
>>> +    QEMU_ARCH_BIT_MIPS          = 7,
>>> +    QEMU_ARCH_BIT_PPC           = 8,
>>> +    QEMU_ARCH_BIT_S390X         = 9,
>>> +    QEMU_ARCH_BIT_SH4           = 10,
>>> +    QEMU_ARCH_BIT_SPARC         = 11,
>>> +    QEMU_ARCH_BIT_XTENSA        = 12,
>>> +    QEMU_ARCH_BIT_OPENRISC      = 13,
>>> +    QEMU_ARCH_BIT_TRICORE       = 16,
>>> +    QEMU_ARCH_BIT_HPPA          = 18,
>>> +    QEMU_ARCH_BIT_RISCV         = 19,
>>> +    QEMU_ARCH_BIT_RX            = 20,
>>> +    QEMU_ARCH_BIT_AVR           = 21,
>>> +    QEMU_ARCH_BIT_HEXAGON       = 22,
>>> +    QEMU_ARCH_BIT_LOONGARCH     = 23,
>>> +} QemuArchBit;
>>> +
>>> +#define QEMU_ARCH_ALPHA         BIT(QEMU_ARCH_BIT_ALPHA)
>>> +#define QEMU_ARCH_ARM           BIT(QEMU_ARCH_BIT_ARM)
>>> +#define QEMU_ARCH_I386          BIT(QEMU_ARCH_BIT_I386)
>>> +#define QEMU_ARCH_M68K          BIT(QEMU_ARCH_BIT_M68K)
>>> +#define QEMU_ARCH_MICROBLAZE    BIT(QEMU_ARCH_BIT_MICROBLAZE)
>>> +#define QEMU_ARCH_MIPS          BIT(QEMU_ARCH_BIT_MIPS)
>>> +#define QEMU_ARCH_PPC           BIT(QEMU_ARCH_BIT_PPC)
>>> +#define QEMU_ARCH_S390X         BIT(QEMU_ARCH_BIT_S390X)
>>> +#define QEMU_ARCH_SH4           BIT(QEMU_ARCH_BIT_SH4)
>>> +#define QEMU_ARCH_SPARC         BIT(QEMU_ARCH_BIT_SPARC)
>>> +#define QEMU_ARCH_XTENSA        BIT(QEMU_ARCH_BIT_XTENSA)
>>> +#define QEMU_ARCH_OPENRISC      BIT(QEMU_ARCH_BIT_OPENRISC)
>>> +#define QEMU_ARCH_TRICORE       BIT(QEMU_ARCH_BIT_TRICORE)
>>> +#define QEMU_ARCH_HPPA          BIT(QEMU_ARCH_BIT_HPPA)
>>> +#define QEMU_ARCH_RISCV         BIT(QEMU_ARCH_BIT_RISCV)
>>> +#define QEMU_ARCH_RX            BIT(QEMU_ARCH_BIT_RX)
>>> +#define QEMU_ARCH_AVR           BIT(QEMU_ARCH_BIT_AVR)
>>> +#define QEMU_ARCH_HEXAGON       BIT(QEMU_ARCH_BIT_HEXAGON)
>>> +#define QEMU_ARCH_LOONGARCH     BIT(QEMU_ARCH_BIT_LOONGARCH)
>>> +#define QEMU_ARCH_ALL           -1
>>>    extern const uint32_t arch_type;
>>
>> What are we gaining by having a "bit" oriented enum, vs simple values
>> that can be compared too?
> 
> I'm not sure what you are asking here, these definitions are heavily
> used in qemu-options.hx, and I don't plan to change them anytime soon.
> 
> For the single binary I'll try to keep the command line interface with
> no change. For heterogeneous binary I plan to start with no CLI so I'm
> not really considering them.
> 

My bad, I thought we were introducing something new here.
Yes, let's stick to a minimal change.

>> As well, it would make sense to add subvariants (aarch64, x86_64, little
>> and big endian variants for some arch), so all of them are present and
>> can be queried easily.
> 
> IIUC what you are referring, this is planned for another interface, but
> not this series which is focused on introducing qemu_arch_name() and
> removing TARGET_NAME. While you don't see any improvement in duplicated
> target files as of this series, some reduction should happen in the next
> step which remove TARGET_BIG_ENDIAN uses in hw/.
> 

Yes, sounds good. It's just a bit hard to track the changes between the 
cleanup of existing files, and introducing a new API to query this 
information at run time.

>>> diff --git a/system/arch_init.c b/system/arch_init.c
>>> index b9147af93cb..fedbb18e2cc 100644
>>> --- a/system/arch_init.c
>>> +++ b/system/arch_init.c
>>> @@ -24,4 +24,4 @@
>>>    #include "qemu/osdep.h"
>>>    #include "system/arch_init.h"
>>> -const uint32_t arch_type = QEMU_ARCH;
>>> +const uint32_t arch_type = BIT(QEMU_ARCH_BIT);
>>
>