| Message ID | 20190215103239.28640-2-berrange@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw: provide error checking of disable-legacy/modern property usage | expand |
Hi, Sorry for taking so long to look at this more closely: On Fri, Feb 15, 2019 at 10:32:38AM +0000, Daniel P. Berrangé wrote: > A number of virtio devices (gpu, crypto, mouse, keyboard, tablet) only > support the virtio-1 (aka modern) mode. Currently if the user launches > QEMU, setting those devices to enable legacy mode, QEMU will silently > create them in modern mode, ignoring the user's (mistaken) request. > > This patch introduces proper data validation so that an attempt to > configure a virtio-1-only devices in legacy mode gets reported as an > error to the user. > > Checking this required introduction of a new field to explicitly track > what operating model is to be used for a device, separately from the > disable_modern and disable_legacy fields that record the user's > requested configuration. I'm still trying to understand why we need to add a new field. If disable_modern, disable_legacy and mode are always expected to be consistent with each other, why do we need another field? If they are not always consistent with each other, when exactly do we want them to be inconsistent, and why? > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> [...] > diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h > index bd223a6e3b..16ef4c0a3f 100644 > --- a/hw/virtio/virtio-pci.h > +++ b/hw/virtio/virtio-pci.h > @@ -15,6 +15,7 @@ > #ifndef QEMU_VIRTIO_PCI_H > #define QEMU_VIRTIO_PCI_H > > +#include "qapi/error.h" > #include "hw/pci/msi.h" > #include "hw/virtio/virtio-bus.h" > > @@ -118,6 +119,12 @@ typedef struct VirtIOPCIQueue { > uint32_t used[2]; > } VirtIOPCIQueue; > > +typedef enum { > + VIRTIO_PCI_MODE_LEGACY, > + VIRTIO_PCI_MODE_TRANSITIONAL, > + VIRTIO_PCI_MODE_MODERN, > +} VirtIOPCIMode; > + > struct VirtIOPCIProxy { > PCIDevice pci_dev; > MemoryRegion bar; > @@ -142,6 +149,7 @@ struct VirtIOPCIProxy { > bool disable_modern; > bool ignore_backend_features; > OnOffAuto disable_legacy; > + VirtIOPCIMode mode; > uint32_t class_code; > uint32_t nvectors; > uint32_t dfselect; > @@ -156,23 +164,34 @@ struct VirtIOPCIProxy { > > static inline bool virtio_pci_modern(VirtIOPCIProxy *proxy) > { > - return !proxy->disable_modern; > + return proxy->mode != VIRTIO_PCI_MODE_LEGACY; > } > > static inline bool virtio_pci_legacy(VirtIOPCIProxy *proxy) > { > - return proxy->disable_legacy == ON_OFF_AUTO_OFF; > + return proxy->mode != VIRTIO_PCI_MODE_MODERN; > } > > -static inline void virtio_pci_force_virtio_1(VirtIOPCIProxy *proxy) > +static inline bool virtio_pci_force_virtio_1(VirtIOPCIProxy *proxy, > + Error **errp) > { > - proxy->disable_modern = false; > - proxy->disable_legacy = ON_OFF_AUTO_ON; > + if (proxy->disable_legacy == ON_OFF_AUTO_OFF) { > + error_setg(errp, "Unable to set disable-legacy=off on a virtio-1.0 " > + "only device"); > + return false; > + } > + if (proxy->disable_modern == true) { > + error_setg(errp, "Unable to set disable-modern=on on a virtio-1.0 " > + "only device"); > + return false; > + } > + proxy->mode = VIRTIO_PCI_MODE_MODERN; > + return true; > } > > static inline void virtio_pci_disable_modern(VirtIOPCIProxy *proxy) > { > - proxy->disable_modern = true; > + proxy->mode = VIRTIO_PCI_MODE_LEGACY; > } > > /* > -- > 2.20.1 > >
On Fri, May 17, 2019 at 04:01:29PM -0300, Eduardo Habkost wrote: > Hi, > > Sorry for taking so long to look at this more closely: > > On Fri, Feb 15, 2019 at 10:32:38AM +0000, Daniel P. Berrangé wrote: > > A number of virtio devices (gpu, crypto, mouse, keyboard, tablet) only > > support the virtio-1 (aka modern) mode. Currently if the user launches > > QEMU, setting those devices to enable legacy mode, QEMU will silently > > create them in modern mode, ignoring the user's (mistaken) request. > > > > This patch introduces proper data validation so that an attempt to > > configure a virtio-1-only devices in legacy mode gets reported as an > > error to the user. > > > > Checking this required introduction of a new field to explicitly track > > what operating model is to be used for a device, separately from the > > disable_modern and disable_legacy fields that record the user's > > requested configuration. > > I'm still trying to understand why we need to add a new field. > > If disable_modern, disable_legacy and mode are always expected to > be consistent with each other, why do we need another field? > > If they are not always consistent with each other, when exactly > do we want them to be inconsistent, and why? The pain point is that we're using the existing variables to record two distinct pieces of information - The user's request for modern vs legacy - The PCI bus requirements for modern vs legacy The existing code would overwrite the user's setting for "disable_legacy" when deciding whether the device is in a PCI or PCIe port. This happens in virtio_pci_realize. We can only report errors with the user's requested config after the sub-classes call virtio_pci_force_virtio_1, but this doesn't happen until virtio_${subclass}_pci_release. So by the time we're able to report errors, virtio_pci_realize has already overwritten the user's disable_legacy setting, so we've lost the very piece of info we need to check to report errors with. Given the ordering of virtio_pci_realize vs the calls to virtio_pci_force_virtio_1 by subclasses, I don't see any option other than to use separate variables for the two distinct pieces of information. > > > > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > [...] > > diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h > > index bd223a6e3b..16ef4c0a3f 100644 > > --- a/hw/virtio/virtio-pci.h > > +++ b/hw/virtio/virtio-pci.h > > @@ -15,6 +15,7 @@ > > #ifndef QEMU_VIRTIO_PCI_H > > #define QEMU_VIRTIO_PCI_H > > > > +#include "qapi/error.h" > > #include "hw/pci/msi.h" > > #include "hw/virtio/virtio-bus.h" > > > > @@ -118,6 +119,12 @@ typedef struct VirtIOPCIQueue { > > uint32_t used[2]; > > } VirtIOPCIQueue; > > > > +typedef enum { > > + VIRTIO_PCI_MODE_LEGACY, > > + VIRTIO_PCI_MODE_TRANSITIONAL, > > + VIRTIO_PCI_MODE_MODERN, > > +} VirtIOPCIMode; > > + > > struct VirtIOPCIProxy { > > PCIDevice pci_dev; > > MemoryRegion bar; > > @@ -142,6 +149,7 @@ struct VirtIOPCIProxy { > > bool disable_modern; > > bool ignore_backend_features; > > OnOffAuto disable_legacy; > > + VirtIOPCIMode mode; > > uint32_t class_code; > > uint32_t nvectors; > > uint32_t dfselect; > > @@ -156,23 +164,34 @@ struct VirtIOPCIProxy { > > > > static inline bool virtio_pci_modern(VirtIOPCIProxy *proxy) > > { > > - return !proxy->disable_modern; > > + return proxy->mode != VIRTIO_PCI_MODE_LEGACY; > > } > > > > static inline bool virtio_pci_legacy(VirtIOPCIProxy *proxy) > > { > > - return proxy->disable_legacy == ON_OFF_AUTO_OFF; > > + return proxy->mode != VIRTIO_PCI_MODE_MODERN; > > } > > > > -static inline void virtio_pci_force_virtio_1(VirtIOPCIProxy *proxy) > > +static inline bool virtio_pci_force_virtio_1(VirtIOPCIProxy *proxy, > > + Error **errp) > > { > > - proxy->disable_modern = false; > > - proxy->disable_legacy = ON_OFF_AUTO_ON; > > + if (proxy->disable_legacy == ON_OFF_AUTO_OFF) { > > + error_setg(errp, "Unable to set disable-legacy=off on a virtio-1.0 " > > + "only device"); > > + return false; > > + } > > + if (proxy->disable_modern == true) { > > + error_setg(errp, "Unable to set disable-modern=on on a virtio-1.0 " > > + "only device"); > > + return false; > > + } > > + proxy->mode = VIRTIO_PCI_MODE_MODERN; > > + return true; > > } > > > > static inline void virtio_pci_disable_modern(VirtIOPCIProxy *proxy) > > { > > - proxy->disable_modern = true; > > + proxy->mode = VIRTIO_PCI_MODE_LEGACY; > > } > > > > /* > > -- > > 2.20.1 > > > > > > -- > Eduardo > Regards, Daniel
On Mon, May 20, 2019 at 10:56:11AM +0100, Daniel P. Berrangé wrote: > On Fri, May 17, 2019 at 04:01:29PM -0300, Eduardo Habkost wrote: > > Hi, > > > > Sorry for taking so long to look at this more closely: > > > > On Fri, Feb 15, 2019 at 10:32:38AM +0000, Daniel P. Berrangé wrote: > > > A number of virtio devices (gpu, crypto, mouse, keyboard, tablet) only > > > support the virtio-1 (aka modern) mode. Currently if the user launches > > > QEMU, setting those devices to enable legacy mode, QEMU will silently > > > create them in modern mode, ignoring the user's (mistaken) request. > > > > > > This patch introduces proper data validation so that an attempt to > > > configure a virtio-1-only devices in legacy mode gets reported as an > > > error to the user. > > > > > > Checking this required introduction of a new field to explicitly track > > > what operating model is to be used for a device, separately from the > > > disable_modern and disable_legacy fields that record the user's > > > requested configuration. > > > > I'm still trying to understand why we need to add a new field. > > > > If disable_modern, disable_legacy and mode are always expected to > > be consistent with each other, why do we need another field? > > > > If they are not always consistent with each other, when exactly > > do we want them to be inconsistent, and why? > > The pain point is that we're using the existing variables to record > two distinct pieces of information > > - The user's request for modern vs legacy > - The PCI bus requirements for modern vs legacy > > The existing code would overwrite the user's setting for > "disable_legacy" when deciding whether the device is in > a PCI or PCIe port. This happens in virtio_pci_realize. > > We can only report errors with the user's requested config > after the sub-classes call virtio_pci_force_virtio_1, but > this doesn't happen until virtio_${subclass}_pci_release. > > So by the time we're able to report errors, virtio_pci_realize > has already overwritten the user's disable_legacy setting, so > we've lost the very piece of info we need to check to report > errors with. Oh, that's the information I was missing. Thanks! > > Given the ordering of virtio_pci_realize vs the calls > to virtio_pci_force_virtio_1 by subclasses, I don't see any > option other than to use separate variables for the two > distinct pieces of information. We could replace the virtio_pci_force_virtio_1() calls with a new VirtioDeviceClass::virtio_1_only boolean field, to be handled by virtio_pci_realize() before overriding disable_legacy. But this can be done as a follow up patch, so: Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
On Mon, May 20, 2019 at 05:59:59PM -0300, Eduardo Habkost wrote: > On Mon, May 20, 2019 at 10:56:11AM +0100, Daniel P. Berrangé wrote: > > On Fri, May 17, 2019 at 04:01:29PM -0300, Eduardo Habkost wrote: > > > Hi, > > > > > > Sorry for taking so long to look at this more closely: > > > > > > On Fri, Feb 15, 2019 at 10:32:38AM +0000, Daniel P. Berrangé wrote: > > > > A number of virtio devices (gpu, crypto, mouse, keyboard, tablet) only > > > > support the virtio-1 (aka modern) mode. Currently if the user launches > > > > QEMU, setting those devices to enable legacy mode, QEMU will silently > > > > create them in modern mode, ignoring the user's (mistaken) request. > > > > > > > > This patch introduces proper data validation so that an attempt to > > > > configure a virtio-1-only devices in legacy mode gets reported as an > > > > error to the user. > > > > > > > > Checking this required introduction of a new field to explicitly track > > > > what operating model is to be used for a device, separately from the > > > > disable_modern and disable_legacy fields that record the user's > > > > requested configuration. > > > > > > I'm still trying to understand why we need to add a new field. > > > > > > If disable_modern, disable_legacy and mode are always expected to > > > be consistent with each other, why do we need another field? > > > > > > If they are not always consistent with each other, when exactly > > > do we want them to be inconsistent, and why? > > > > The pain point is that we're using the existing variables to record > > two distinct pieces of information > > > > - The user's request for modern vs legacy > > - The PCI bus requirements for modern vs legacy > > > > The existing code would overwrite the user's setting for > > "disable_legacy" when deciding whether the device is in > > a PCI or PCIe port. This happens in virtio_pci_realize. > > > > We can only report errors with the user's requested config > > after the sub-classes call virtio_pci_force_virtio_1, but > > this doesn't happen until virtio_${subclass}_pci_release. > > > > So by the time we're able to report errors, virtio_pci_realize > > has already overwritten the user's disable_legacy setting, so > > we've lost the very piece of info we need to check to report > > errors with. > > Oh, that's the information I was missing. Thanks! > > > > > Given the ordering of virtio_pci_realize vs the calls > > to virtio_pci_force_virtio_1 by subclasses, I don't see any > > option other than to use separate variables for the two > > distinct pieces of information. > > We could replace the virtio_pci_force_virtio_1() calls with a new > VirtioDeviceClass::virtio_1_only boolean field, to be handled by > virtio_pci_realize() before overriding disable_legacy. Yes, that would be a desirable thing todo in future to get the error checking done sooner in virtio_pci_realize() only. Regards, Daniel
diff --git a/hw/core/machine.c b/hw/core/machine.c index 077fbd182a..61fb791e6a 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -96,9 +96,26 @@ const size_t hw_compat_2_7_len = G_N_ELEMENTS(hw_compat_2_7); GlobalProperty hw_compat_2_6[] = { { "virtio-mmio", "format_transport_address", "off" }, - /* Optional because not all virtio-pci devices support legacy mode */ - { "virtio-pci", "disable-modern", "on", .optional = true }, - { "virtio-pci", "disable-legacy", "off", .optional = true }, + /* + * don't include devices which are modern-only + * ie keyboard, mouse, tablet, gpu, vga & crypto + */ + { "virtio-9p-pci", "disable-modern", "on" }, + { "virtio-9p-pci", "disable-legacy", "off" }, + { "virtio-balloon-pci", "disable-modern", "on" }, + { "virtio-balloon-pci", "disable-legacy", "off" }, + { "virtio-blk-pci", "disable-modern", "on" }, + { "virtio-blk-pci", "disable-legacy", "off" }, + { "virtio-input-host-pci", "disable-modern", "on" }, + { "virtio-input-host-pci", "disable-legacy", "off" }, + { "virtio-net-pci", "disable-modern", "on" }, + { "virtio-net-pci", "disable-legacy", "off" }, + { "virtio-rng-pci", "disable-modern", "on" }, + { "virtio-rng-pci", "disable-legacy", "off" }, + { "virtio-scsi-pci", "disable-modern", "on" }, + { "virtio-scsi-pci", "disable-legacy", "off" }, + { "virtio-serial-pci", "disable-modern", "on" }, + { "virtio-serial-pci", "disable-legacy", "off" }, }; const size_t hw_compat_2_6_len = G_N_ELEMENTS(hw_compat_2_6); diff --git a/hw/display/virtio-gpu-pci.c b/hw/display/virtio-gpu-pci.c index bdcd33c925..0bc4d9d424 100644 --- a/hw/display/virtio-gpu-pci.c +++ b/hw/display/virtio-gpu-pci.c @@ -47,7 +47,9 @@ static void virtio_gpu_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp) Error *local_error = NULL; qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus)); - virtio_pci_force_virtio_1(vpci_dev); + if (!virtio_pci_force_virtio_1(vpci_dev, errp)) { + return; + } object_property_set_bool(OBJECT(vdev), true, "realized", &local_error); if (local_error) { diff --git a/hw/display/virtio-vga.c b/hw/display/virtio-vga.c index 1e48009b74..d63fe8c345 100644 --- a/hw/display/virtio-vga.c +++ b/hw/display/virtio-vga.c @@ -145,7 +145,9 @@ static void virtio_vga_realize(VirtIOPCIProxy *vpci_dev, Error **errp) /* init virtio bits */ qdev_set_parent_bus(DEVICE(g), BUS(&vpci_dev->bus)); - virtio_pci_force_virtio_1(vpci_dev); + if (!virtio_pci_force_virtio_1(vpci_dev, errp)) { + return; + } object_property_set_bool(OBJECT(g), true, "realized", &err); if (err) { error_propagate(errp, err); diff --git a/hw/virtio/virtio-crypto-pci.c b/hw/virtio/virtio-crypto-pci.c index 90a6e0dc2e..13807e538b 100644 --- a/hw/virtio/virtio-crypto-pci.c +++ b/hw/virtio/virtio-crypto-pci.c @@ -51,7 +51,9 @@ static void virtio_crypto_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp) } qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus)); - virtio_pci_force_virtio_1(vpci_dev); + if (!virtio_pci_force_virtio_1(vpci_dev, errp)) { + return; + } object_property_set_bool(OBJECT(vdev), true, "realized", errp); object_property_set_link(OBJECT(vcrypto), OBJECT(vcrypto->vdev.conf.cryptodev), "cryptodev", diff --git a/hw/virtio/virtio-input-pci.c b/hw/virtio/virtio-input-pci.c index 2c1397842b..28477729a3 100644 --- a/hw/virtio/virtio-input-pci.c +++ b/hw/virtio/virtio-input-pci.c @@ -48,7 +48,9 @@ static void virtio_input_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp) DeviceState *vdev = DEVICE(&vinput->vdev); qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus)); - virtio_pci_force_virtio_1(vpci_dev); + if (!virtio_pci_force_virtio_1(vpci_dev, errp)) { + return; + } object_property_set_bool(OBJECT(vdev), true, "realized", errp); } diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c index e978bfe760..ec31ec6cf3 100644 --- a/hw/virtio/virtio-pci.c +++ b/hw/virtio/virtio-pci.c @@ -1721,16 +1721,22 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp) /* PCI BAR regions must be powers of 2 */ pow2ceil(proxy->notify.offset + proxy->notify.size)); - if (proxy->disable_legacy == ON_OFF_AUTO_AUTO) { - proxy->disable_legacy = pcie_port ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF; - } - - if (!virtio_pci_modern(proxy) && !virtio_pci_legacy(proxy)) { - error_setg(errp, "device cannot work as neither modern nor legacy mode" - " is enabled"); - error_append_hint(errp, "Set either disable-modern or disable-legacy" - " to off\n"); - return; + if ((proxy->disable_legacy == ON_OFF_AUTO_ON) || + ((proxy->disable_legacy == ON_OFF_AUTO_AUTO) && pcie_port)) { + if (proxy->disable_modern) { + error_setg(errp, "device cannot work as neither modern nor " + "legacy mode is enabled"); + error_append_hint(errp, "Set either disable-modern or " + "disable-legacy to off\n"); + return; + } + proxy->mode = VIRTIO_PCI_MODE_MODERN; + } else { + if (proxy->disable_modern) { + proxy->mode = VIRTIO_PCI_MODE_LEGACY; + } else { + proxy->mode = VIRTIO_PCI_MODE_TRANSITIONAL; + } } if (pcie_port && pci_is_express(pci_dev)) { diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h index bd223a6e3b..16ef4c0a3f 100644 --- a/hw/virtio/virtio-pci.h +++ b/hw/virtio/virtio-pci.h @@ -15,6 +15,7 @@ #ifndef QEMU_VIRTIO_PCI_H #define QEMU_VIRTIO_PCI_H +#include "qapi/error.h" #include "hw/pci/msi.h" #include "hw/virtio/virtio-bus.h" @@ -118,6 +119,12 @@ typedef struct VirtIOPCIQueue { uint32_t used[2]; } VirtIOPCIQueue; +typedef enum { + VIRTIO_PCI_MODE_LEGACY, + VIRTIO_PCI_MODE_TRANSITIONAL, + VIRTIO_PCI_MODE_MODERN, +} VirtIOPCIMode; + struct VirtIOPCIProxy { PCIDevice pci_dev; MemoryRegion bar; @@ -142,6 +149,7 @@ struct VirtIOPCIProxy { bool disable_modern; bool ignore_backend_features; OnOffAuto disable_legacy; + VirtIOPCIMode mode; uint32_t class_code; uint32_t nvectors; uint32_t dfselect; @@ -156,23 +164,34 @@ struct VirtIOPCIProxy { static inline bool virtio_pci_modern(VirtIOPCIProxy *proxy) { - return !proxy->disable_modern; + return proxy->mode != VIRTIO_PCI_MODE_LEGACY; } static inline bool virtio_pci_legacy(VirtIOPCIProxy *proxy) { - return proxy->disable_legacy == ON_OFF_AUTO_OFF; + return proxy->mode != VIRTIO_PCI_MODE_MODERN; } -static inline void virtio_pci_force_virtio_1(VirtIOPCIProxy *proxy) +static inline bool virtio_pci_force_virtio_1(VirtIOPCIProxy *proxy, + Error **errp) { - proxy->disable_modern = false; - proxy->disable_legacy = ON_OFF_AUTO_ON; + if (proxy->disable_legacy == ON_OFF_AUTO_OFF) { + error_setg(errp, "Unable to set disable-legacy=off on a virtio-1.0 " + "only device"); + return false; + } + if (proxy->disable_modern == true) { + error_setg(errp, "Unable to set disable-modern=on on a virtio-1.0 " + "only device"); + return false; + } + proxy->mode = VIRTIO_PCI_MODE_MODERN; + return true; } static inline void virtio_pci_disable_modern(VirtIOPCIProxy *proxy) { - proxy->disable_modern = true; + proxy->mode = VIRTIO_PCI_MODE_LEGACY; } /*
A number of virtio devices (gpu, crypto, mouse, keyboard, tablet) only support the virtio-1 (aka modern) mode. Currently if the user launches QEMU, setting those devices to enable legacy mode, QEMU will silently create them in modern mode, ignoring the user's (mistaken) request. This patch introduces proper data validation so that an attempt to configure a virtio-1-only devices in legacy mode gets reported as an error to the user. Checking this required introduction of a new field to explicitly track what operating model is to be used for a device, separately from the disable_modern and disable_legacy fields that record the user's requested configuration. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- hw/core/machine.c | 23 ++++++++++++++++++++--- hw/display/virtio-gpu-pci.c | 4 +++- hw/display/virtio-vga.c | 4 +++- hw/virtio/virtio-crypto-pci.c | 4 +++- hw/virtio/virtio-input-pci.c | 4 +++- hw/virtio/virtio-pci.c | 26 ++++++++++++++++---------- hw/virtio/virtio-pci.h | 31 +++++++++++++++++++++++++------ 7 files changed, 73 insertions(+), 23 deletions(-)