| Message ID | 20250502132441.64723-1-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] hw/virtio: Introduce CONFIG_VIRTIO_LEGACY to disable legacy support | expand |
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é <philmd@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.bouvier@linaro.org>
On Fri, May 02, 2025 at 03:24:41PM +0200, 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. > > Devices modelling shouldn't really change depending on a property > of a CPU accessing it. > > 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. > > 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. IMHO that isn't acceptable. In order to be able to provide an upgrade path from the old binaries, we need the need the new binaries to be able to serve the same use cases & disabling virtio 0.9 support prevents that. I don't see a compelling technical reason why we can't support virtio 0.9 from this endian point. Yes may be more ugly/messy than we would like, but that's par for the course with QEMU emulating arbitrary device models. If the new binaries can't cope with messy devices then I think we are doing something wrong. With regards, Daniel
On Tue, May 06, 2025 at 09:04:50AM +0100, Daniel P. Berrangé wrote: > On Fri, May 02, 2025 at 03:24:41PM +0200, 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. > > > > Devices modelling shouldn't really change depending on a property > > of a CPU accessing it. > > > > 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. > > > > 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. > > IMHO that isn't acceptable. In order to be able to provide an > upgrade path from the old binaries, we need the need the new > binaries to be able to serve the same use cases & disabling > virtio 0.9 support prevents that. I don't see a compelling > technical reason why we can't support virtio 0.9 from this > endian point. > > Yes may be more ugly/messy than we would like, but that's par > for the course with QEMU emulating arbitrary device models. > If the new binaries can't cope with messy devices then I think > we are doing something wrong. > > With regards, > Daniel To be more specific, having a kconfig knob modifying the device without regards for machine types, means it is no longer enough to inspect the command line to figure out the compatiblity. That's a problem.
On 6/5/25 10:12, Michael S. Tsirkin wrote: > On Tue, May 06, 2025 at 09:04:50AM +0100, Daniel P. Berrangé wrote: >> On Fri, May 02, 2025 at 03:24:41PM +0200, 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. >>> >>> Devices modelling shouldn't really change depending on a property >>> of a CPU accessing it. >>> >>> 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. >>> >>> 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. >> >> IMHO that isn't acceptable. In order to be able to provide an >> upgrade path from the old binaries, we need the need the new >> binaries to be able to serve the same use cases & disabling >> virtio 0.9 support prevents that. This isn't for the single binary effort, there we are taking a lot of care to not introduce any change. This is for after it; once we have a single binary (one architecture run by an instance) we can start testing heterogeneous emulation (different architectures in the same instance). >> I don't see a compelling >> technical reason why we can't support virtio 0.9 from this >> endian point. VirtIO 0.9 needs knowledge of the vCPU architecture to get its endianness. So we need to somehow propagate that at creation time, guarantying which vCPU (or set of vCPUs) will access a virtio device. The use case I'd like to figure out is how should we model plugging a virtio 0.9 device in into a fully emulated ZynqMP machine, which has little-endian ARM cores and big endian MicroBlaze cores. Alex said this is unlikely to happen, and better is to ignore this case by not allowing virtio pre-1.0 devices in our future heterogeneous emulator. In this same thread Pierrick pointed me to the reference in the spec: '2.4.3 Legacy Interfaces: A Note on Virtqueue Endianness', "It is assumed that the host is already aware of the guest endian." I suppose this means a pre-1.0 virtio device expect to be used by a single guest OS, but it is not clear the guest OS as fixed endianness, because the code path checks vCPU endianness at runtime, so passing guest endianness as a property to pre-1.0 devices isn't really an option. Anyway I think 1/ I posted this too early, "speculating" as Pierrick noticed, and confuse the community w.r.t. "single binary" and 2/ I don' t understand legacy virtio and its endianness handling enough to figure a clever idea to keep using it heterogeneous setup, so I'll let this task to someone more familiar with that tech. >> Yes may be more ugly/messy than we would like, but that's par >> for the course with QEMU emulating arbitrary device models. >> If the new binaries can't cope with messy devices then I think >> we are doing something wrong. > > To be more specific, having a kconfig knob modifying the device > without regards for machine types, means it is no longer > enough to inspect the command line to figure out the > compatiblity. That's a problem. > OK. I won't pursue in this direction. I apologize for mentioning this topic again too early. Regards, Phil.
On Tue, May 06, 2025 at 10:55:34AM +0200, Philippe Mathieu-Daudé wrote: > On 6/5/25 10:12, Michael S. Tsirkin wrote: > > On Tue, May 06, 2025 at 09:04:50AM +0100, Daniel P. Berrangé wrote: > > > On Fri, May 02, 2025 at 03:24:41PM +0200, 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. > > > > > > > > Devices modelling shouldn't really change depending on a property > > > > of a CPU accessing it. > > > > > > > > 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. > > > > > > > > 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. > > > > > > IMHO that isn't acceptable. In order to be able to provide an > > > upgrade path from the old binaries, we need the need the new > > > binaries to be able to serve the same use cases & disabling > > > virtio 0.9 support prevents that. > > This isn't for the single binary effort, there we are taking a > lot of care to not introduce any change. > > This is for after it; once we have a single binary (one architecture > run by an instance) we can start testing heterogeneous emulation > (different architectures in the same instance). > > > > I don't see a compelling > > > technical reason why we can't support virtio 0.9 from this > > > endian point. > > VirtIO 0.9 needs knowledge of the vCPU architecture to get its > endianness. So we need to somehow propagate that at creation > time, guarantying which vCPU (or set of vCPUs) will access a > virtio device. > > The use case I'd like to figure out is how should we model > plugging a virtio 0.9 device in into a fully emulated > ZynqMP machine, which has little-endian ARM cores and big > endian MicroBlaze cores. > > Alex said this is unlikely to happen, and better is to > ignore this case by not allowing virtio pre-1.0 devices in > our future heterogeneous emulator. Indeed. I just do not think this can be done with a kconfig knob, it's a machine property. > In this same thread Pierrick pointed me to the reference in > the spec: '2.4.3 Legacy Interfaces: A Note on Virtqueue Endianness', > "It is assumed that the host is already aware of the guest endian." > > I suppose this means a pre-1.0 virtio device expect to be used by > a single guest OS, but it is not clear the guest OS as fixed > endianness, because the code path checks vCPU endianness at > runtime, so passing guest endianness as a property to pre-1.0 > devices isn't really an option. > > Anyway I think 1/ I posted this too early, "speculating" as Pierrick > noticed, and confuse the community w.r.t. "single binary" and > 2/ I don' t understand legacy virtio and its endianness handling > enough to figure a clever idea to keep using it heterogeneous > setup, so I'll let this task to someone more familiar with that tech. > > > > Yes may be more ugly/messy than we would like, but that's par > > > for the course with QEMU emulating arbitrary device models. > > > If the new binaries can't cope with messy devices then I think > > > we are doing something wrong. > > > > > To be more specific, having a kconfig knob modifying the device > > without regards for machine types, means it is no longer > > enough to inspect the command line to figure out the > > compatiblity. That's a problem. > > > > OK. I won't pursue in this direction. I apologize for mentioning > this topic again too early. > > Regards, > > Phil.
On 5/6/25 1:55 AM, Philippe Mathieu-Daudé wrote: > On 6/5/25 10:12, Michael S. Tsirkin wrote: >> On Tue, May 06, 2025 at 09:04:50AM +0100, Daniel P. Berrangé wrote: >>> On Fri, May 02, 2025 at 03:24:41PM +0200, 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. >>>> >>>> Devices modelling shouldn't really change depending on a property >>>> of a CPU accessing it. >>>> >>>> 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. >>>> >>>> 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. >>> >>> IMHO that isn't acceptable. In order to be able to provide an >>> upgrade path from the old binaries, we need the need the new >>> binaries to be able to serve the same use cases & disabling >>> virtio 0.9 support prevents that. > > This isn't for the single binary effort, there we are taking a > lot of care to not introduce any change. > > This is for after it; once we have a single binary (one architecture > run by an instance) we can start testing heterogeneous emulation > (different architectures in the same instance). > >>> I don't see a compelling >>> technical reason why we can't support virtio 0.9 from this >>> endian point. > > VirtIO 0.9 needs knowledge of the vCPU architecture to get its > endianness. So we need to somehow propagate that at creation > time, guarantying which vCPU (or set of vCPUs) will access a > virtio device. > > The use case I'd like to figure out is how should we model > plugging a virtio 0.9 device in into a fully emulated > ZynqMP machine, which has little-endian ARM cores and big > endian MicroBlaze cores. > Virtio devices are not the only one who will need to be extended to support such a scenario. What happens when a disk or network card is added to the machine? Should it be shared? Should it be mapped only for one cpu address space? Should address spaces be mixed or independent? Per cluster, per cpu, per architure? Having a concrete prototype will allow us to answer to those questions, and many others, and find solutions for the situations we meet. > Alex said this is unlikely to happen, and better is to > ignore this case by not allowing virtio pre-1.0 devices in > our future heterogeneous emulator. > > In this same thread Pierrick pointed me to the reference in > the spec: '2.4.3 Legacy Interfaces: A Note on Virtqueue Endianness', > "It is assumed that the host is already aware of the guest endian." > > I suppose this means a pre-1.0 virtio device expect to be used by > a single guest OS, but it is not clear the guest OS as fixed > endianness, because the code path checks vCPU endianness at > runtime, so passing guest endianness as a property to pre-1.0 > devices isn't really an option. > > Anyway I think 1/ I posted this too early, "speculating" as Pierrick > noticed, and confuse the community w.r.t. "single binary" and > 2/ I don' t understand legacy virtio and its endianness handling > enough to figure a clever idea to keep using it heterogeneous > setup, so I'll let this task to someone more familiar with that tech. > An important mantra will be to keep that compatible with existing command line (potentially extending it to support those new use cases, in both the existing *and* the new binary). The last thing we want to introduce is "yet another QEMU". >>> Yes may be more ugly/messy than we would like, but that's par >>> for the course with QEMU emulating arbitrary device models. >>> If the new binaries can't cope with messy devices then I think >>> we are doing something wrong. > >> >> To be more specific, having a kconfig knob modifying the device >> without regards for machine types, means it is no longer >> enough to inspect the command line to figure out the >> compatiblity. That's a problem. >> > > OK. I won't pursue in this direction. I apologize for mentioning > this topic again too early. > > Regards, > > Phil.
On 2025/05/06 17:55, Philippe Mathieu-Daudé wrote: > On 6/5/25 10:12, Michael S. Tsirkin wrote: >> On Tue, May 06, 2025 at 09:04:50AM +0100, Daniel P. Berrangé wrote: >>> On Fri, May 02, 2025 at 03:24:41PM +0200, 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. >>>> >>>> Devices modelling shouldn't really change depending on a property >>>> of a CPU accessing it. >>>> >>>> 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. >>>> >>>> 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. >>> >>> IMHO that isn't acceptable. In order to be able to provide an >>> upgrade path from the old binaries, we need the need the new >>> binaries to be able to serve the same use cases & disabling >>> virtio 0.9 support prevents that. > > This isn't for the single binary effort, there we are taking a > lot of care to not introduce any change. > > This is for after it; once we have a single binary (one architecture > run by an instance) we can start testing heterogeneous emulation > (different architectures in the same instance). > >>> I don't see a compelling >>> technical reason why we can't support virtio 0.9 from this >>> endian point. > > VirtIO 0.9 needs knowledge of the vCPU architecture to get its > endianness. So we need to somehow propagate that at creation > time, guarantying which vCPU (or set of vCPUs) will access a > virtio device. > > The use case I'd like to figure out is how should we model > plugging a virtio 0.9 device in into a fully emulated > ZynqMP machine, which has little-endian ARM cores and big > endian MicroBlaze cores. > > Alex said this is unlikely to happen, and better is to > ignore this case by not allowing virtio pre-1.0 devices in > our future heterogeneous emulator. > > In this same thread Pierrick pointed me to the reference in > the spec: '2.4.3 Legacy Interfaces: A Note on Virtqueue Endianness', > "It is assumed that the host is already aware of the guest endian." > > I suppose this means a pre-1.0 virtio device expect to be used by > a single guest OS, but it is not clear the guest OS as fixed > endianness, because the code path checks vCPU endianness at > runtime, so passing guest endianness as a property to pre-1.0 > devices isn't really an option. > > Anyway I think 1/ I posted this too early, "speculating" as Pierrick > noticed, and confuse the community w.r.t. "single binary" and > 2/ I don' t understand legacy virtio and its endianness handling > enough to figure a clever idea to keep using it heterogeneous > setup, so I'll let this task to someone more familiar with that tech. It may not be too early as QEMU may already support heterogeneous endianness. An Arm CPU have a register to switch the endianness, which depends on the current Exception level, and QEMU already implements it at least enough to switch the endianness for virtio if I read the code correctly. This means QEMU should already be able to handle cases where the endianness changes depending on: - timing (due to writes to SCLR or switches among Exception levels) or - vCPUs (as each vCPU has its own register instance). If virtio has a problem with heterogeneous endianness, I'm afraid that it matters even now for Arm emulation. In my understanding, the virtio code is written carefully enough to avoid breaking migration and I found no other obvious problems like crashes and memory leaks with heterogeneous endianness, so I can conclude that the virtio code requires no change. However, allowing to have legacy devices with heterogeneous endianness may still lead to mismatches between QEMU and user's expectations as the spec is not clear. Deprecating the combination of legacy devices with CPUs that can switch endianness (and other heterogeneous endianness scenarios in the future) may be a good idea to avoid that. Regards, Akihiko Odaki
On Thu, 8 May 2025 at 09:37, Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > > On 2025/05/06 17:55, Philippe Mathieu-Daudé wrote: > > VirtIO 0.9 needs knowledge of the vCPU architecture to get its > > endianness. So we need to somehow propagate that at creation > > time, guarantying which vCPU (or set of vCPUs) will access a > > virtio device. > It may not be too early as QEMU may already support heterogeneous > endianness. > > An Arm CPU have a register to switch the endianness, which depends on > the current Exception level, and QEMU already implements it at least > enough to switch the endianness for virtio if I read the code correctly. > > This means QEMU should already be able to handle cases where the > endianness changes depending on: > - timing (due to writes to SCLR or switches among Exception levels) or > - vCPUs (as each vCPU has its own register instance). Mmm -- the virtio code uses 'current_cpu' to get the CPU which did the device access to it, and then queries that CPU for which endianness it should be assuming the data to be. That ought to also work in a fully heterogenous setup with e.g. multiple Arm and microblaze cores, I think ? There is however also a function virtio_default_endian() which seems to get used in contexts where the device isn't being directly accessed by a CPU, such as system reset, and which looks at target_big_endian(). That I think will need attention for a heterogenous setup. (For runtime-endian change setups like Arm it doesn't matter, because when the OS resets the virtio device it will set the endianness that way. But AIUI target_big_endian() becomes kind of meaningless in the heterogenous single binary setup?) Note that in practice nobody's going to be actively flipping endianness and expecting the virtio device to keep up: the use cases are "the OS sets things up once at bootup before it touches any virtio device", and "the CPU boots up in big-endian mode (i.e. the opposite of the target_big_endian() compile time value) because the board/SoC model configures it that way". thanks -- PMM
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
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. Devices modelling shouldn't really change depending on a property of a CPU accessing it. 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. 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. Signed-off-by: Philippe Mathieu-Daudé <philmd@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(-)