Re: [RFC PATCH v3 13/14] qemu/target_info: Add target_aarch64() helper

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

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.