Re: [RFC PATCH 10/11] qemu: Introduce qemu_arch_name() helper

["Daniel P. Berrangé" <berrange@redhat.com>]

On Wed, Mar 05, 2025 at 01:52:24AM +0100, Philippe Mathieu-Daudé wrote:
> Introduce a generic helper to get the target name of a QemuArchBit.
> (This will be used for single / heterogeneous binaries).
> Use it in target_name(), removing the last use of the TARGET_NAME
> definition.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  include/qemu/arch_info.h |  2 ++
>  arch_info-target.c       | 34 +++++++++++++++++++++++++++++++++-
>  2 files changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git a/include/qemu/arch_info.h b/include/qemu/arch_info.h
> index 613dc2037db..7e3192f590f 100644
> --- a/include/qemu/arch_info.h
> +++ b/include/qemu/arch_info.h
> @@ -46,6 +46,8 @@ typedef enum QemuArchBit {
>  #define QEMU_ARCH_LOONGARCH     BIT(QEMU_ARCH_BIT_LOONGARCH)
>  #define QEMU_ARCH_ALL           -1
>  
> +const char *qemu_arch_name(QemuArchBit qemu_arch_bit);
> +
>  const char *target_name(void);
>  
>  bool qemu_arch_available(unsigned qemu_arch_mask);
> diff --git a/arch_info-target.c b/arch_info-target.c
> index 61007415b30..9b19fe8d56d 100644
> --- a/arch_info-target.c
> +++ b/arch_info-target.c
> @@ -24,9 +24,41 @@
>  #include "qemu/osdep.h"
>  #include "qemu/arch_info.h"
>  
> +const char *qemu_arch_name(QemuArchBit qemu_arch_bit)
> +{
> +    static const char *legacy_target_names[] = {
> +        [QEMU_ARCH_ALPHA] = "alpha",

All the others you've used QEMU_ARCH_BIT except for this. Yes, it happens
to have the same value either way, but it still looks wrong.

> +        [QEMU_ARCH_BIT_ARM] = TARGET_LONG_BITS == 32 ? "arm" : "aarch64",
> +        [QEMU_ARCH_BIT_AVR] = "avr",
> +        [QEMU_ARCH_BIT_HEXAGON] = "hexagon",
> +        [QEMU_ARCH_BIT_HPPA] = "hppa",
> +        [QEMU_ARCH_BIT_I386] = TARGET_LONG_BITS == 32 ? "i386" : "x86_64",
> +        [QEMU_ARCH_BIT_LOONGARCH] = "loongarch64",
> +        [QEMU_ARCH_BIT_M68K] = "m68k",
> +        [QEMU_ARCH_BIT_MICROBLAZE] = TARGET_BIG_ENDIAN ? "microblaze"
> +                                                       : "microblazeel",
> +        [QEMU_ARCH_BIT_MIPS] = TARGET_BIG_ENDIAN
> +                             ? (TARGET_LONG_BITS == 32 ? "mips" : "mips64")
> +                             : (TARGET_LONG_BITS == 32 ? "mipsel" : "mips64el"),
> +        [QEMU_ARCH_BIT_OPENRISC] = "or1k",
> +        [QEMU_ARCH_BIT_PPC] = TARGET_LONG_BITS == 32 ? "ppc" : "ppc64",
> +        [QEMU_ARCH_BIT_RISCV] = TARGET_LONG_BITS == 32 ? "riscv32" : "riscv64",
> +        [QEMU_ARCH_BIT_RX] = "rx",
> +        [QEMU_ARCH_BIT_S390X] = "s390x",
> +        [QEMU_ARCH_BIT_SH4] = TARGET_BIG_ENDIAN ? "sh4eb" : "sh4",
> +        [QEMU_ARCH_BIT_SPARC] = TARGET_LONG_BITS == 32 ? "sparc" : "sparc64",
> +        [QEMU_ARCH_BIT_TRICORE] = "tricore",
> +        [QEMU_ARCH_BIT_XTENSA] = TARGET_BIG_ENDIAN ? "xtensaeb" : "xtensa",

Why do we need to give arches different names based on endian/bits, as
opposed to a fixed int -> name mapping. What's the intended  usage of
this ?

Since you're calling this a legacy_target_names, should the method also
be call qemu_legacy_arch_name() rather than qemu_arch_name - I would
have naively expected qemu_arch_name to be a plain mapping.

> +    };
> +
> +    assert(qemu_arch_bit < ARRAY_SIZE(legacy_target_names));
> +    assert(legacy_target_names[qemu_arch_bit]);
> +    return legacy_target_names[qemu_arch_bit];
> +}
> +
>  const char *target_name(void)
>  {
> -    return TARGET_NAME;
> +    return qemu_arch_name(QEMU_ARCH_BIT);
>  }
>  
>  bool qemu_arch_available(unsigned qemu_arch_mask)
> -- 
> 2.47.1
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|