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 <ehabk...@redhat.com> -- Eduardo