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é <berra...@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
