Re: [RFC PATCH] hw/virtio: Introduce CONFIG_VIRTIO_LEGACY to disable legacy support

Fri, 02 May 2025 09:40:47 -0700 

On 5/2/25 6:24 AM, Philippe Mathieu-Daudé wrote:

Legacy VirtIO devices don't have their endianness clearly defined.
QEMU infers it taking the endianness of the (target) binary, or,
when a target support switching endianness at runtime, taking the
endianness of the vCPU accessing the device.
Probably it's what you meant, but it is clearly defined by the standard [1]. It's just not fixed. 

2.4.3 Legacy Interfaces: A Note on Virtqueue Endianness
Note that when using the legacy interface, transitional devices and drivers MUST use the native endian of the guest as the endian of fields and in the virtqueue. This is opposed to little-endian for non-legacy interface as specified by this standard. It is assumed that the host is already aware of the guest endian.
[1] https://docs.oasis-open.org/virtio/virtio/v1.0/cs04/virtio-v1.0-cs04.html#x1-250003 


Devices modelling shouldn't really change depending on a property
of a CPU accessing it.



Devices concerning various targets are aware of the cpu and its properties.


For heterogeneous systems, it is simpler to break such dev <-> cpu
dependency, only allowing generic device models, with no knowledge
of CPU (or DMA controller) accesses.
At this point, we speculated it could be a problem, without really proving it. In case all accesses are done within a given vcpu thread, there is no ambiguity on the current endianness. How about we wait to expose a crash in an heterogeneous system to take a decision? 


Therefore we introduce the VIRTIO_LEGACY Kconfig key. We keep the
current default (enabled).
New binaries can set CONFIG_VIRTIO_LEGACY=n to restrict models to
the VirtIO version 1 spec.



I think it's a good change, regarding the legacy support.
However, if the only goal is to support a custom configuration for heterogeneous emulation, I think it's the wrong direction.
In essence, we are working hard right now to remove compile time configuration for various QEMU targets. So seeing a new compile time configuration to solve something looks like the opposite of what we are trying to achive. A possible alternative would be to enable virtio legacy support at runtime, based on a specific criteria. From what I remember, legacy vs modern is a property set by disable-modern=bool,disable-legacy=bool, and "modern" is the default. Thus, we can simply restrict the disable-legacy=on in case we detect it's an heterogeneous emulation scenario. 


Signed-off-by: Philippe Mathieu-Daudé <phi...@linaro.org>
---
  include/hw/virtio/virtio-access.h | 5 ++++-
  hw/virtio/virtio.c                | 8 ++++++++
  target/arm/cpu.c                  | 5 +++++
  target/ppc/cpu_init.c             | 5 +++++
  hw/virtio/Kconfig                 | 5 +++++
  5 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/include/hw/virtio/virtio-access.h 
b/include/hw/virtio/virtio-access.h
index 07aae69042a..b5b471711a6 100644
--- a/include/hw/virtio/virtio-access.h
+++ b/include/hw/virtio/virtio-access.h
@@ -20,7 +20,10 @@
  #include "hw/virtio/virtio.h"
  #include "hw/virtio/virtio-bus.h"
-#if defined(TARGET_PPC64) || defined(TARGET_ARM) 
+#include CONFIG_DEVICES
+
+#if defined(CONFIG_VIRTIO_LEGACY) && \
+    (defined(TARGET_PPC64) || defined(TARGET_ARM))
  #define LEGACY_VIRTIO_IS_BIENDIAN 1
  #endif
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c 
index 480c2e50365..659ab3cb969 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -47,6 +47,8 @@
  #include "standard-headers/linux/virtio_mem.h"
  #include "standard-headers/linux/virtio_vsock.h"
+#include CONFIG_DEVICES 
+
  /*
   * Maximum size of virtio device config space
   */
@@ -3502,6 +3504,7 @@ void virtio_init(VirtIODevice *vdev, uint16_t device_id, 
size_t config_size)
  bool virtio_legacy_allowed(VirtIODevice *vdev)
  {
      switch (vdev->device_id) {
+#ifdef CONFIG_VIRTIO_LEGACY
      case VIRTIO_ID_NET:
      case VIRTIO_ID_BLOCK:
      case VIRTIO_ID_CONSOLE:
@@ -3513,6 +3516,7 @@ bool virtio_legacy_allowed(VirtIODevice *vdev)
      case VIRTIO_ID_RPROC_SERIAL:
      case VIRTIO_ID_CAIF:
          return true;
+#endif
      default:
          return false;
      }
@@ -4014,8 +4018,10 @@ static const Property virtio_properties[] = {
      DEFINE_VIRTIO_COMMON_FEATURES(VirtIODevice, host_features),
      DEFINE_PROP_BOOL("use-started", VirtIODevice, use_started, true),
      DEFINE_PROP_BOOL("use-disabled-flag", VirtIODevice, use_disabled_flag, 
true),
+#ifdef CONFIG_VIRTIO_LEGACY
      DEFINE_PROP_BOOL("x-disable-legacy-check", VirtIODevice,
                       disable_legacy_check, false),
+#endif
  };
static int virtio_device_start_ioeventfd_impl(VirtIODevice *vdev) 
@@ -4151,7 +4157,9 @@ static void virtio_device_class_init(ObjectClass *klass, 
const void *data)
      vdc->start_ioeventfd = virtio_device_start_ioeventfd_impl;
      vdc->stop_ioeventfd = virtio_device_stop_ioeventfd_impl;
+#ifdef CONFIG_VIRTIO_LEGACY 
      vdc->legacy_features |= VIRTIO_LEGACY_FEATURES;
+#endif
  }
bool virtio_device_ioeventfd_enabled(VirtIODevice *vdev) 
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 5e951675c60..d01fcb9fd1a 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -39,6 +39,7 @@
  #if !defined(CONFIG_USER_ONLY)
  #include "hw/loader.h"
  #include "hw/boards.h"
+#include CONFIG_DEVICES
  #ifdef CONFIG_TCG
  #include "hw/intc/armv7m_nvic.h"
  #endif /* CONFIG_TCG */
@@ -1130,6 +1131,7 @@ static void arm_cpu_kvm_set_irq(void *opaque, int irq, 
int level)
  #endif
  }
+#ifdef CONFIG_VIRTIO_LEGACY 
  static bool arm_cpu_virtio_is_big_endian(CPUState *cs)
  {
      ARMCPU *cpu = ARM_CPU(cs);
@@ -1138,6 +1140,7 @@ static bool arm_cpu_virtio_is_big_endian(CPUState *cs)
      cpu_synchronize_state(cs);
      return arm_cpu_data_is_big_endian(env);
  }
+#endif
#ifdef CONFIG_TCG 
  bool arm_cpu_exec_halt(CPUState *cs)
@@ -2681,7 +2684,9 @@ static const struct SysemuCPUOps arm_sysemu_ops = {
      .asidx_from_attrs = arm_asidx_from_attrs,
      .write_elf32_note = arm_cpu_write_elf32_note,
      .write_elf64_note = arm_cpu_write_elf64_note,
+#ifdef CONFIG_VIRTIO_LEGACY
      .virtio_is_big_endian = arm_cpu_virtio_is_big_endian,
+#endif
      .legacy_vmsd = &vmstate_arm_cpu,
  };
  #endif
diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
index b0973b6df95..4b6c347bda8 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -50,6 +50,7 @@
  #include "hw/boards.h"
  #include "hw/intc/intc.h"
  #include "kvm_ppc.h"
+#include CONFIG_DEVICES
  #endif
#include "cpu_init.h" 
@@ -7352,12 +7353,14 @@ static void ppc_cpu_reset_hold(Object *obj, ResetType 
type)
#ifndef CONFIG_USER_ONLY +#ifdef CONFIG_VIRTIO_LEGACY 
  static bool ppc_cpu_is_big_endian(CPUState *cs)
  {
      cpu_synchronize_state(cs);
return !FIELD_EX64(cpu_env(cs)->msr, MSR, LE); 
  }
+#endif
static bool ppc_get_irq_stats(InterruptStatsProvider *obj, 
                                uint64_t **irq_counts, unsigned int *nb_irqs)
@@ -7470,7 +7473,9 @@ static const struct SysemuCPUOps ppc_sysemu_ops = {
      .get_phys_page_debug = ppc_cpu_get_phys_page_debug,
      .write_elf32_note = ppc32_cpu_write_elf32_note,
      .write_elf64_note = ppc64_cpu_write_elf64_note,
+#ifdef CONFIG_VIRTIO_LEGACY
      .virtio_is_big_endian = ppc_cpu_is_big_endian,
+#endif
      .legacy_vmsd = &vmstate_ppc_cpu,
  };
  #endif
diff --git a/hw/virtio/Kconfig b/hw/virtio/Kconfig
index 7648a2d68da..314185f0016 100644
--- a/hw/virtio/Kconfig
+++ b/hw/virtio/Kconfig
@@ -1,6 +1,11 @@
  config VIRTIO
      bool
+config VIRTIO_LEGACY 
+    bool
+    default y
+    depends on VIRTIO
+
  config VIRTIO_RNG
      bool
      default y
If the goal is to condition virtio_legacy, to maybe deprecate it one day, then: 
Reviewed-by: Pierrick Bouvier <pierrick.bouv...@linaro.org>

Reply via email to