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

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

On 5/3/25 02:36, Pierrick Bouvier wrote:
> 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.

OK, in the next version I'll try to better explain where this series is
going in the patch description.