Re: [Qemu-devel] [PATCH for-4.0 v4 2/2] virtio: Provide version-specific variants of virtio PCI devices

[Cornelia Huck <cohuck@redhat.com>]

On Thu, 3 Jan 2019 23:27:08 -0500
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Jan 03, 2019 at 04:32:06PM -0200, Eduardo Habkost wrote:
> > On Thu, Jan 03, 2019 at 11:48:37AM +0100, Cornelia Huck wrote:  
> > > On Thu, 3 Jan 2019 11:14:44 +0100
> > > Thomas Huth <thuth@redhat.com> wrote:
> > >   
> > > > On 2019-01-03 10:38, Thomas Huth wrote:  
> > > > > On 2018-12-05 20:57, Eduardo Habkost wrote:    
> > > > >> Many of the current virtio-*-pci device types actually represent
> > > > >> 3 different types of devices:
> > > > >> * virtio 1.0 non-transitional devices
> > > > >> * virtio 1.0 transitional devices
> > > > >> * virtio 0.9 ("legacy device" in virtio 1.0 terminology)
> > > > >>
> > > > >> That would be just an annoyance if it didn't break our device/bus
> > > > >> compatibility QMP interfaces.  With these multi-purpose device
> > > > >> types, there's no way to tell management software that
> > > > >> transitional devices and legacy devices require a Conventional
> > > > >> PCI bus.
> > > > >>
> > > > >> The multi-purpose device types would also prevent us from telling
> > > > >> management software what's the PCI vendor/device ID for them,
> > > > >> because their PCI IDs change at runtime depending on the bus
> > > > >> where they were plugged.
> > > > >>
> > > > >> This patch adds separate device types for each of those virtio
> > > > >> device flavors:
> > > > >>
> > > > >> - virtio-*-pci: the existing multi-purpose device types
> > > > >>   - Configurable using `disable-legacy` and `disable-modern`
> > > > >>     properties
> > > > >>   - Legacy driver support is automatically enabled/disabled
> > > > >>     depending on the bus where it is plugged
> > > > >>   - Supports Conventional PCI and PCI Express buses
> > > > >>     (but Conventional PCI is incompatible with
> > > > >>     disable-legacy=off)
> > > > >>   - Changes PCI vendor/device IDs at runtime
> > > > >> - virtio-*-pci-transitional: virtio-1.0 device supporting legacy drivers
> > > > >>   - Supports Conventional PCI buses only, because
> > > > >>     it has a PIO BAR
> > > > >> - virtio-*-pci-non-transitional: modern-only
> > > > >>   - Supports both Conventional PCI and PCI Express buses
> > > > >>
> > > > >> The existing TYPE_* macros for these types will point to an
> > > > >> abstract base type, so existing casts in the code will keep
> > > > >> working for all variants.
> > > > >>
> > > > >> A simple test script (tests/acceptance/virtio_version.py) is
> > > > >> included, to check if the new device types are equivalent to
> > > > >> using the `disable-legacy` and `disable-modern` options.
> > > > >>
> > > > >> Acked-by: Andrea Bolognani <abologna@redhat.com>
> > > > >> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> > > > >> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>    
> > > > > 
> > > > >  Hi Eduardo,
> > > > > 
> > > > > with these new devices, I can trigger an abort on s390x:
> > > > > 
> > > > > $ qemu-system-s390x -M s390-ccw-virtio-2.5 -monitor stdio -no-shutdown
> > > > > QEMU 3.1.50 monitor - type 'help' for more information
> > > > > (qemu) device_add vhost-scsi-pci-non-transitional
> > > > > qemu-system-s390x: hw/core/qdev-properties.c:1236:
> > > > > qdev_prop_set_globals: Assertion `prop->user_provided' failed.
> > > > > Aborted (core dumped)    
> > > > FWIW, it happens with x86, too:
> > > > 
> > > > $ qemu-system-x86_64 -M pc-i440fx-2.6 -monitor stdio
> > > > QEMU 3.1.50 monitor - type 'help' for more information
> > > > (qemu) device_add vhost-scsi-pci-non-transitional
> > > > qemu-system-x86_64: hw/core/qdev-properties.c:1236:
> > > > qdev_prop_set_globals: Assertion `prop->user_provided' failed.
> > > > Aborted (core dumped)
> > > > 
> > > > Only machine types newer than 2.7 seem to be OK.  
> > > 
> > > It also fails for vhost-scsi-pci-transitional on 2.6 and older machines.
> > > 
> > > The problem is that HW_COMPAT_2_6 tries to set the disable_modern
> > > property, which the version-specific variants of virtio-pci do not have.
> > > 
> > > Not sure how to fix this. The version-specific devices cannot really
> > > work on the compat machine.  
> > 
> > Oops.
> > 
> > My patch broke the assumption that every virtio-pci subclass is a
> > generic legacy/modern device.  The good news is that HW_COMPAT_2_6
> > needs to affect only devices that existed in QEMU
> > 2.6 (not every subclass of virtio-pci).
> > 
> > I see 3 possible ways to address this:
> > 
> > 1) Making disable-legacy and disable-modern available on all
> > virtio-pci subclasses again.
> > 
> > This is simple to implement, but I would like to avoid that.  I'd
> > prefer to keep the legacy/modern logic complexity restricted to
> > the old generic virtio devices.  
> 
> As it's only for compat, we can probably use an "x-" property.

I'm not sure that makes it much better, though.

> > 2) Creating a virtio-pci-generic or virtio-pci-hybrid interface
> > type, add it to generic_type_info at virtio_pci_types_register(),
> > and use it on HW_COMPAT_2_6 instead of "virtio-pci".
> > 
> > This is simple to implement, but I'm wary.  I'm already bothered
> > by the complexity of our PCI and virtio type hierarchies.  We
> > have multiple overlapping sets of virtio-pci devices (depending
> > which way you look), and specifying exactly which set we want to
> > affect is tricky.  

I think the various virtio-pci types are already confusing enough
without an extra interface type.

> I'm not sure I understand this one. Would this be required
> anyway if we added a new feature to both transitional and modern
> devices?

I thought new features would need to be added to the base class anyway?

> 
> > 
> > 3) Replacing "virtio-pci" on HW_COMPAT_2_6 with an explicit list
> > of concrete virtio-pci types that existed when QEMU 2.6 was
> > released.  This will make HW_COMPAT_2_6 grow, but reduce
> > complexity of the whole system.  I'm inclined to implement this
> > solution.

This solution looks best to me as well.