| Message ID | 20250418172908.25147-14-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | single-binary: Make hw/arm/ common | expand |
On 4/18/25 10:29, Philippe Mathieu-Daudé wrote: > Add a helper to distinct the binary is targetting > Aarch64 or not. > > Start with a dump strcmp() implementation, leaving > room for future optimizations. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > include/qemu/target_info.h | 7 +++++++ > target_info.c | 5 +++++ > 2 files changed, 12 insertions(+) > > diff --git a/include/qemu/target_info.h b/include/qemu/target_info.h > index c67b97d66f3..9b7575ce632 100644 > --- a/include/qemu/target_info.h > +++ b/include/qemu/target_info.h > @@ -24,4 +24,11 @@ const char *target_name(void); > */ > const char *target_machine_typename(void); > > +/** > + * target_aarch64: > + * > + * Returns whether the target architecture is Aarch64. > + */ > +bool target_aarch64(void); > + > #endif > diff --git a/target_info.c b/target_info.c > index 1de4334ecc5..87dd1d51778 100644 > --- a/target_info.c > +++ b/target_info.c > @@ -19,3 +19,8 @@ const char *target_machine_typename(void) > { > return target_info()->machine_typename; > } > + > +bool target_aarch64(void) > +{ > + return !strcmp(target_name(), "aarch64"); I don't think doing strcmp is a good move here, even temporarily. A short term solution is making target_info.c target specific, and use: return TARGET_AARCH64; The long term solution, is to have a create target_current() that returns an enum, and target_aarch64() would become: return target_current() == {ENUM}_AARCH64. We just need to find a good name for {enum} which is not Target, since it's a poisoned identifier. This way, we can easily convert the simple #ifdef TARGET_AARCH64 by if target_aarch64(), and more complicated combinations by a switch on target_current(). For a first version, I think that the first solution is enough.
On 19/4/25 03:09, Pierrick Bouvier wrote: > On 4/18/25 10:29, Philippe Mathieu-Daudé wrote: >> Add a helper to distinct the binary is targetting >> Aarch64 or not. >> >> Start with a dump strcmp() implementation, leaving >> room for future optimizations. >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> --- >> include/qemu/target_info.h | 7 +++++++ >> target_info.c | 5 +++++ >> 2 files changed, 12 insertions(+) >> >> diff --git a/include/qemu/target_info.h b/include/qemu/target_info.h >> index c67b97d66f3..9b7575ce632 100644 >> --- a/include/qemu/target_info.h >> +++ b/include/qemu/target_info.h >> @@ -24,4 +24,11 @@ const char *target_name(void); >> */ >> const char *target_machine_typename(void); >> +/** >> + * target_aarch64: >> + * >> + * Returns whether the target architecture is Aarch64. >> + */ >> +bool target_aarch64(void); >> + >> #endif >> diff --git a/target_info.c b/target_info.c >> index 1de4334ecc5..87dd1d51778 100644 >> --- a/target_info.c >> +++ b/target_info.c >> @@ -19,3 +19,8 @@ const char *target_machine_typename(void) >> { >> return target_info()->machine_typename; >> } >> + >> +bool target_aarch64(void) >> +{ >> + return !strcmp(target_name(), "aarch64"); > > I don't think doing strcmp is a good move here, even temporarily. > > A short term solution is making target_info.c target specific, and use: > return TARGET_AARCH64; IIUC as https://lore.kernel.org/qemu-devel/20231122183048.17150-3-philmd@linaro.org/? > The long term solution, is to have a create target_current() that > returns an enum, and target_aarch64() would become: > return target_current() == {ENUM}_AARCH64. We just need to find a good > name for {enum} which is not Target, since it's a poisoned identifier. > > This way, we can easily convert the simple > #ifdef TARGET_AARCH64 by if target_aarch64(), > and more complicated combinations by a switch on target_current(). This was https://lore.kernel.org/qemu-devel/20250403234914.9154-4-philmd@linaro.org/, which was useful for the virtio-mem patch: -- >8 -- diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c index c7968ee0c61..b5d62411b3e 100644 --- a/hw/virtio/virtio-mem.c +++ b/hw/virtio/virtio-mem.c @@ -17,2 +17,3 @@ #include "qemu/units.h" +#include "qemu/target_info.h" #include "system/numa.h" @@ -35,9 +36,17 @@ static const VMStateDescription vmstate_virtio_mem_device_early; -/* - * We only had legacy x86 guests that did not support - * VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE. Other targets don't have legacy guests. - */ -#if defined(TARGET_X86_64) || defined(TARGET_I386) -#define VIRTIO_MEM_HAS_LEGACY_GUESTS -#endif +static bool virtio_mem_has_legacy_guests(void) +{ + /* + * We only had legacy x86 guests that did not support + * VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE. Other targets don't have + * legacy guests. + */ + switch (target_system_arch()) { + case SYS_EMU_TARGET_I386: + case SYS_EMU_TARGET_X86_64: + return true; + default: + return false; + } +} @@ -145,3 +154,2 @@ static uint64_t virtio_mem_default_block_size(RAMBlock *rb) -#if defined(VIRTIO_MEM_HAS_LEGACY_GUESTS) static bool virtio_mem_has_shared_zeropage(RAMBlock *rb) @@ -156,3 +164,2 @@ static bool virtio_mem_has_shared_zeropage(RAMBlock *rb) } -#endif /* VIRTIO_MEM_HAS_LEGACY_GUESTS */ @@ -999,24 +1006,26 @@ static void virtio_mem_device_realize(DeviceState *dev, Error **errp) -#if defined(VIRTIO_MEM_HAS_LEGACY_GUESTS) - switch (vmem->unplugged_inaccessible) { - case ON_OFF_AUTO_AUTO: - if (virtio_mem_has_shared_zeropage(rb)) { - vmem->unplugged_inaccessible = ON_OFF_AUTO_OFF; - } else { - vmem->unplugged_inaccessible = ON_OFF_AUTO_ON; + if (virtio_mem_has_legacy_guests()) { + switch (vmem->unplugged_inaccessible) { + case ON_OFF_AUTO_AUTO: + if (virtio_mem_has_shared_zeropage(rb)) { + vmem->unplugged_inaccessible = ON_OFF_AUTO_OFF; + } else { + vmem->unplugged_inaccessible = ON_OFF_AUTO_ON; + } + break; + case ON_OFF_AUTO_OFF: + if (!virtio_mem_has_shared_zeropage(rb)) { + warn_report("'%s' property set to 'off' with a memdev that does" + " not support the shared zeropage.", + VIRTIO_MEM_UNPLUGGED_INACCESSIBLE_PROP); + } + break; + default: + break; } - break; - case ON_OFF_AUTO_OFF: - if (!virtio_mem_has_shared_zeropage(rb)) { - warn_report("'%s' property set to 'off' with a memdev that does" - " not support the shared zeropage.", - VIRTIO_MEM_UNPLUGGED_INACCESSIBLE_PROP); - } - break; - default: - break; + } else if (vmem->unplugged_inaccessible != ON_OFF_AUTO_ON) { + error_setg(errp, "guest requires property '%s' to be 'on'", + VIRTIO_MEM_UNPLUGGED_INACCESSIBLE_PROP); + return; } -#else /* VIRTIO_MEM_HAS_LEGACY_GUESTS */ - vmem->unplugged_inaccessible = ON_OFF_AUTO_ON; -#endif /* VIRTIO_MEM_HAS_LEGACY_GUESTS */ @@ -1713,6 +1722,4 @@ static const Property virtio_mem_properties[] = { TYPE_MEMORY_BACKEND, HostMemoryBackend *), -#if defined(VIRTIO_MEM_HAS_LEGACY_GUESTS) DEFINE_PROP_ON_OFF_AUTO(VIRTIO_MEM_UNPLUGGED_INACCESSIBLE_PROP, VirtIOMEM, unplugged_inaccessible, ON_OFF_AUTO_ON), -#endif DEFINE_PROP_BOOL(VIRTIO_MEM_EARLY_MIGRATION_PROP, VirtIOMEM, --- but I thought either you didn't like the approach or it was too early to propose for the API, so I went back to strcmp. > > For a first version, I think that the first solution is enough.
On 4/19/25 05:54, Philippe Mathieu-Daudé wrote: >> I don't think doing strcmp is a good move here, even temporarily. >> >> A short term solution is making target_info.c target specific, and use: >> return TARGET_AARCH64; > > IIUC as > https://lore.kernel.org/qemu-devel/20231122183048.17150-3-philmd@linaro.org/? > Yes, but simply named target_aarch64() instead of target_aarch64_available(), to mimic the existing TARGET_AARCH64. >> The long term solution, is to have a create target_current() that >> returns an enum, and target_aarch64() would become: >> return target_current() == {ENUM}_AARCH64. We just need to find a good >> name for {enum} which is not Target, since it's a poisoned identifier. >> >> This way, we can easily convert the simple >> #ifdef TARGET_AARCH64 by if target_aarch64(), >> and more complicated combinations by a switch on target_current(). > > This was > https://lore.kernel.org/qemu-devel/20250403234914.9154-4-philmd@linaro.org/, > which was useful for the virtio-mem patch: > > -- >8 -- > diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c > index c7968ee0c61..b5d62411b3e 100644 > --- a/hw/virtio/virtio-mem.c > +++ b/hw/virtio/virtio-mem.c > @@ -17,2 +17,3 @@ > #include "qemu/units.h" > +#include "qemu/target_info.h" > #include "system/numa.h" > @@ -35,9 +36,17 @@ static const VMStateDescription > vmstate_virtio_mem_device_early; > > -/* > - * We only had legacy x86 guests that did not support > - * VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE. Other targets don't have legacy > guests. > - */ > -#if defined(TARGET_X86_64) || defined(TARGET_I386) > -#define VIRTIO_MEM_HAS_LEGACY_GUESTS > -#endif > +static bool virtio_mem_has_legacy_guests(void) > +{ > + /* > + * We only had legacy x86 guests that did not support > + * VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE. Other targets don't have > + * legacy guests. > + */ > + switch (target_system_arch()) { > + case SYS_EMU_TARGET_I386: > + case SYS_EMU_TARGET_X86_64: > + return true; > + default: > + return false; > + } > +} > > @@ -145,3 +154,2 @@ static uint64_t > virtio_mem_default_block_size(RAMBlock *rb) > > -#if defined(VIRTIO_MEM_HAS_LEGACY_GUESTS) > static bool virtio_mem_has_shared_zeropage(RAMBlock *rb) > @@ -156,3 +164,2 @@ static bool virtio_mem_has_shared_zeropage(RAMBlock *rb) > } > -#endif /* VIRTIO_MEM_HAS_LEGACY_GUESTS */ > > @@ -999,24 +1006,26 @@ static void virtio_mem_device_realize(DeviceState > *dev, Error **errp) > > -#if defined(VIRTIO_MEM_HAS_LEGACY_GUESTS) > - switch (vmem->unplugged_inaccessible) { > - case ON_OFF_AUTO_AUTO: > - if (virtio_mem_has_shared_zeropage(rb)) { > - vmem->unplugged_inaccessible = ON_OFF_AUTO_OFF; > - } else { > - vmem->unplugged_inaccessible = ON_OFF_AUTO_ON; > + if (virtio_mem_has_legacy_guests()) { > + switch (vmem->unplugged_inaccessible) { > + case ON_OFF_AUTO_AUTO: > + if (virtio_mem_has_shared_zeropage(rb)) { > + vmem->unplugged_inaccessible = ON_OFF_AUTO_OFF; > + } else { > + vmem->unplugged_inaccessible = ON_OFF_AUTO_ON; > + } > + break; > + case ON_OFF_AUTO_OFF: > + if (!virtio_mem_has_shared_zeropage(rb)) { > + warn_report("'%s' property set to 'off' with a memdev > that does" > + " not support the shared zeropage.", > + VIRTIO_MEM_UNPLUGGED_INACCESSIBLE_PROP); > + } > + break; > + default: > + break; > } > - break; > - case ON_OFF_AUTO_OFF: > - if (!virtio_mem_has_shared_zeropage(rb)) { > - warn_report("'%s' property set to 'off' with a memdev that > does" > - " not support the shared zeropage.", > - VIRTIO_MEM_UNPLUGGED_INACCESSIBLE_PROP); > - } > - break; > - default: > - break; > + } else if (vmem->unplugged_inaccessible != ON_OFF_AUTO_ON) { > + error_setg(errp, "guest requires property '%s' to be 'on'", > + VIRTIO_MEM_UNPLUGGED_INACCESSIBLE_PROP); > + return; > } > -#else /* VIRTIO_MEM_HAS_LEGACY_GUESTS */ > - vmem->unplugged_inaccessible = ON_OFF_AUTO_ON; > -#endif /* VIRTIO_MEM_HAS_LEGACY_GUESTS */ > > @@ -1713,6 +1722,4 @@ static const Property virtio_mem_properties[] = { > TYPE_MEMORY_BACKEND, HostMemoryBackend *), > -#if defined(VIRTIO_MEM_HAS_LEGACY_GUESTS) > DEFINE_PROP_ON_OFF_AUTO(VIRTIO_MEM_UNPLUGGED_INACCESSIBLE_PROP, > VirtIOMEM, > unplugged_inaccessible, ON_OFF_AUTO_ON), > -#endif > DEFINE_PROP_BOOL(VIRTIO_MEM_EARLY_MIGRATION_PROP, VirtIOMEM, > --- > > but I thought either you didn't like the approach or it was too early > to propose for the API, so I went back to strcmp. > At this point, I would like to focus on having a first version of TargetInfo API, and not reviewing any other changes, as things may be modified, and they would need to be reviewed again. It's hard to follow the same abstraction done multiple times in multiple series. Regarding your proposal for target_system_arch(), I understand that you tried to reuse the existing SysEmuTarget, which was a good intention. However, I don't think we should use any string compare for this (which qemu_api_parse does). It has several flaws: - The most important one: it can fail (what if -1 is returned?). Enums can be guaranteed and exhaustive at compile time. - It's slower than having the current arch directly known at compile time. As well, since SysEmuTarget is a generated enum, it makes it much harder to follow code IMHO. QAPI requires those things to be defined from a json file for external usage, but it's not a good reason for being forced to use it in all the codebase as the only possible abstraction. To have something fast and infallible, we can adopt this solution: In target_info.h: /* Named TargetArch to not clash with poisoned TARGET_X */ typedef enum TargetArch { TARGET_ARCH_AARCH64, TARGET_ARCH_ALPHA, TARGET_ARCH_ARM, TARGET_ARCH_AVR, TARGET_ARCH_HPPA, TARGET_ARCH_I386, TARGET_ARCH_LOONGARCH64, TARGET_ARCH_M68K, TARGET_ARCH_MICROBLAZE, TARGET_ARCH_MICROBLAZEEL, TARGET_ARCH_MIPS, TARGET_ARCH_MIPS64, TARGET_ARCH_MIPS64EL, TARGET_ARCH_MIPSEL, TARGET_ARCH_OR1K, TARGET_ARCH_PPC, TARGET_ARCH_PPC64, TARGET_ARCH_RISCV32, TARGET_ARCH_RISCV64, TARGET_ARCH_RX, TARGET_ARCH_S390X, TARGET_ARCH_SH4, TARGET_ARCH_SH4EB, TARGET_ARCH_SPARC, TARGET_ARCH_SPARC64, TARGET_ARCH_TRICORE, TARGET_ARCH_X86_64, TARGET_ARCH_XTENSA, TARGET_ARCH_XTENSAEB, } TargetArch; typedef struct TargetInfo { ... TargetArch target_arch; ... } static inline target_arch() { return target_info()->target_arch; } static inline target_aarch64() { return target_arch() == TARGET_ARCH_AARCH64; } In target_info-stub.c: #ifdef TARGET_AARCH64 # define TARGET_ARCH TARGET_ARCH_AARCH64 #elif TARGET_ARCH_ALPHA # define TARGET_ARCH TARGET_ARCH_ALPHA ... #endif static const TargetInfo target_info_stub = { ... .target_arch = TARGET_ARCH; ... }
diff --git a/include/qemu/target_info.h b/include/qemu/target_info.h index c67b97d66f3..9b7575ce632 100644 --- a/include/qemu/target_info.h +++ b/include/qemu/target_info.h @@ -24,4 +24,11 @@ const char *target_name(void); */ const char *target_machine_typename(void); +/** + * target_aarch64: + * + * Returns whether the target architecture is Aarch64. + */ +bool target_aarch64(void); + #endif diff --git a/target_info.c b/target_info.c index 1de4334ecc5..87dd1d51778 100644 --- a/target_info.c +++ b/target_info.c @@ -19,3 +19,8 @@ const char *target_machine_typename(void) { return target_info()->machine_typename; } + +bool target_aarch64(void) +{ + return !strcmp(target_name(), "aarch64"); +}
Add a helper to distinct the binary is targetting Aarch64 or not. Start with a dump strcmp() implementation, leaving room for future optimizations. Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- include/qemu/target_info.h | 7 +++++++ target_info.c | 5 +++++ 2 files changed, 12 insertions(+)