Re: [Qemu-devel] [PATCH v2 1/2] hw: report invalid disable-legacy|modern usage for virtio-1-only devs

[Daniel P . Berrangé]

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
-- 
|: 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 :|