[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 1/2] hw: report invalid disable-legacy|modern
From: |
Daniel P . Berrangé |
Subject: |
Re: [Qemu-devel] [PATCH v2 1/2] hw: report invalid disable-legacy|modern usage for virtio-1-only devs |
Date: |
Mon, 20 May 2019 10:56:11 +0100 |
User-agent: |
Mutt/1.11.4 (2019-03-13) |
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é <address@hidden>
> [...]
> > 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
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
[Qemu-devel] [PULL 02/37] Revert "globals: Allow global properties to be optional", Michael S. Tsirkin, 2019/05/16
[Qemu-devel] [PULL 03/37] docs: reST-ify vhost-user documentation, Michael S. Tsirkin, 2019/05/16
[Qemu-devel] [PULL 04/37] virtio: Introduce started flag to VirtioDevice, Michael S. Tsirkin, 2019/05/16