| 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; ... }
Pierrick Bouvier <pierrick.bouvier@linaro.org> writes: [...] > 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: qemu_api_parse()? Do you mean qapi_enum_parse()? > - 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; Effectively duplicates generated enum SysEmuTarget. Can you explain why that's useful? > > 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; > ... > }
On 4/21/25 23:04, Markus Armbruster wrote: > Pierrick Bouvier <pierrick.bouvier@linaro.org> writes: > > [...] > >> 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: > > qemu_api_parse()? Do you mean qapi_enum_parse()? > Yes, sorry for the typo. >> - 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; > > Effectively duplicates generated enum SysEmuTarget. Can you explain why > that's useful? > In terms of code, it doesn't change anything. we could reuse SysEmuTarget. I just think it's more clear to have the enum defined next to the structure using it, compared to have this buried in generated code that can't be grepped easily. (git grep SYS_EMU returns nothing, so people have to remember it's converted from a Json, and that naming is different). IMHO, DRY principle should not always be followed blindly when it hurts code readability. That said, my editor is able to find the generated definition as well, so I don't really mind reusing SysEmuTarget if we think the code readability is not worth the duplication. However, I think it's a problem to compare strings to find this, when it can be set at compile time, in a file that will have to stay target specific anyway. >> >> 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; >> ... >> } >
Pierrick Bouvier <pierrick.bouvier@linaro.org> writes: > On 4/21/25 23:04, Markus Armbruster wrote: >> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes: >> [...] >> >>> 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: >> >> qemu_api_parse()? Do you mean qapi_enum_parse()? > > Yes, sorry for the typo. > >>> - 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; >> >> Effectively duplicates generated enum SysEmuTarget. Can you explain why >> that's useful? > > In terms of code, it doesn't change anything. we could reuse SysEmuTarget. > I just think it's more clear to have the enum defined next to the structure using it, compared to have this buried in generated code that can't be grepped easily. > (git grep SYS_EMU returns nothing, so people have to remember it's converted from a Json, and that naming is different). Yes, that's unfortunate, but we don't duplicate other QAPI enums because of it. > IMHO, DRY principle should not always be followed blindly when it hurts code readability. Treating DRY as dogma would be foolish. > That said, my editor is able to find the generated definition as well, so I don't really mind reusing SysEmuTarget if we think the code readability is not worth the duplication. It's not just the duplication, it's also the conversion between the two enums. > However, I think it's a problem to compare strings to find this, when it can be set at compile time, in a file that will have to stay target specific anyway. Converting from string to enum at first practical opportunity makes the code simpler more often than not. [...]
On 4/22/25 11:24, Markus Armbruster wrote: >>> Effectively duplicates generated enum SysEmuTarget. Can you explain why >>> that's useful? >> >> In terms of code, it doesn't change anything. we could reuse SysEmuTarget. >> I just think it's more clear to have the enum defined next to the structure using it, compared to have this buried in generated code that can't be grepped easily. >> (git grep SYS_EMU returns nothing, so people have to remember it's converted from a Json, and that naming is different). > > Yes, that's unfortunate, but we don't duplicate other QAPI enums because > of it. > Fine for me if it's the consensus we already have in QEMU, no one mentioned it until your answer. >> IMHO, DRY principle should not always be followed blindly when it hurts code readability. > > Treating DRY as dogma would be foolish. > I agree, alas, it tends to be treated as such, even unintentionally, when people spend too much time on the same code/file. >> That said, my editor is able to find the generated definition as well, so I don't really mind reusing SysEmuTarget if we think the code readability is not worth the duplication. > > It's not just the duplication, it's also the conversion between the two > enums. > >> However, I think it's a problem to compare strings to find this, when it can be set at compile time, in a file that will have to stay target specific anyway. > > Converting from string to enum at first practical opportunity makes the > code simpler more often than not. > Sure, it's not what is done in the v4 anymore, so it's fine. > > [...] >
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(+)