Re: [Qemu-devel] [PATCH] virtio: Provide version-specific variants of virtio PCI devices

[Cornelia Huck]

On Fri, 12 Oct 2018 23:54:35 -0300
Eduardo Habkost <address@hidden> wrote:

> 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 this multi-purpose device
> type, 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 is 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-0.9: legacy virtio device
>   - Supports Conventional PCI buses only, because
>     it has a PIO BAR
> - virtio-*-pci-1.0-transitional: virtio-1.0 device supporting legacy drivers
>   - Supports Conventional PCI buses only, because
>     it has a PIO BAR
> - virtio-*-pci-1.0: modern-only
>   - Supports both Conventional PCI and PCI Express buses
> 
> All the types above will inherit from an abstract
> "virtio-*-pci-base" type, so existing code that doesn't care
> about the virtio version can use "virtio-*-pci-base" on type
> casts.

I get a crash when running 'make check'; might be missing a change to
virtio-input-host?

TEST: tests/device-introspect-test... (pid=28626)
  /s390x/device/introspect/list:                                       OK
  /s390x/device/introspect/list-fields:                                OK
  /s390x/device/introspect/none:                                       OK
  /s390x/device/introspect/abstract:                                   OK
  /s390x/device/introspect/abstract-interfaces:                        OK
  /s390x/device/introspect/concrete/defaults/none:                     
/home/cohuck/git/qemu/hw/virtio/virtio-pci.c:2730:virtio_host_initfn: Object 
0x5633a43a1480 is not an instance of type virtio-input-host-device-base
Broken pipe
/home/cohuck/git/qemu/tests/libqtest.c:125: kill_qemu() detected QEMU death 
from signal 6 (Aborted) (core dumped)

(...)
#2  0x00005633a2e92555 in object_dynamic_cast_assert (obj=0x5633a43a1480, 
    typename=0x5633a30a295c "virtio-input-host-device-base", 
    file=0x5633a30a2152 "/home/cohuck/git/qemu/hw/virtio/virtio-pci.c", 
    line=2730, func=0x5633a30a297a "virtio_host_initfn")
    at /home/cohuck/git/qemu/qom/object.c:702
#3  0x00005633a2e30fa3 in virtio_host_initfn (obj=0x5633a43a1480)
    at /home/cohuck/git/qemu/hw/virtio/virtio-pci.c:2730
#4  0x00005633a2e966a2 in object_init_with_type (obj=0x5633a43a1480, 
    ti=0x5633a42eb990) at /home/cohuck/git/qemu/qom/object.c:356
#5  0x00005633a2e91433 in object_initialize_with_type (data=0x5633a43a1480, 
    size=33632, type=0x5633a42eb990) at /home/cohuck/git/qemu/qom/object.c:391
#6  0x00005633a2e91db8 in object_new_with_type (type=<optimized out>)
    at /home/cohuck/git/qemu/qom/object.c:553
#7  object_new (typename=<optimized out>)
    at /home/cohuck/git/qemu/qom/object.c:563
#8  0x00005633a2dbfc49 in qmp_device_list_properties (
    typename=0x5633a437cb40 "virtio-input-host-pci-1.0-transitional", 
    errp=0x7ffd66276010) at /home/cohuck/git/qemu/qmp.c:513
(...)

> 
> 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.
> 
> Signed-off-by: Eduardo Habkost <address@hidden>
> ---
> Reference to previous discussion that originated this idea:
>   https://www.mail-archive.com/address@hidden/msg558389.html
> ---
>  hw/virtio/virtio-pci.h             |  93 +++++++++---
>  hw/display/virtio-gpu-pci.c        |   8 +-
>  hw/display/virtio-vga.c            |  11 +-
>  hw/virtio/virtio-crypto-pci.c      |   8 +-
>  hw/virtio/virtio-pci.c             | 225 +++++++++++++++++++++--------
>  tests/acceptance/virtio_version.py | 138 ++++++++++++++++++
>  6 files changed, 390 insertions(+), 93 deletions(-)
>  create mode 100644 tests/acceptance/virtio_version.py

The approach makes sense to me, but I second the suggestion to use
something like 'modern' (or 'standard'?) instead of '1.0'.

Also, before someone asks: I don't think we need something like that
for virtio-ccw, as we (a) only support fencing newer revisions, not
older ones, and (b) the implications of using different virtio
revisions are localized to virtio-ccw only and do not have further
implications as for virtio-pci.