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-phi...@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-phi...@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;
...
}
