[Qemu-devel] [PATCH for-2.5] virtio: handle non-virtio-1-capable backend

[Cornelia Huck]

If you run a qemu advertising VERSION_1 with an old kernel where
vhost did not yet support VERSION_1, you'll end up with a device
that is {modern pci|ccw revision 1} but does not advertise VERSION_1.
This is not a sensible configuration and is rejected by the Linux
guest drivers.

To fix this, add a ->post_plugged() callback invoked after features
have been queried that can handle the VERSION_1 bit being withdrawn
and change pci (only setup modern if VERSION_1 is still present) and
ccw (fall back to revision 0 if VERSION_1 is gone).

Signed-off-by: Cornelia Huck <address@hidden>
---
Michael: I think this should go into 2.5, as otherwise ccw (which
defaults to virtio-1 with 2.5) will be broken with old host kernels.
I need someone to give virtio-pci some testing, as I do not have
a proper setup for that. ccw is working fine.

Changes from initial writeup:
- Move pci-modern init to post_plugged instead of undoing [Jason]
- Add a proper patch description + signoff
- Add some comments
---
 hw/s390x/virtio-ccw.c          | 12 ++++++
 hw/virtio/virtio-bus.c         |  3 ++
 hw/virtio/virtio-pci.c         | 96 ++++++++++++++++++++++++++----------------
 include/hw/virtio/virtio-bus.h |  5 +++
 4 files changed, 80 insertions(+), 36 deletions(-)

diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index fb103b7..63da303 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -1555,6 +1555,17 @@ static void virtio_ccw_device_plugged(DeviceState *d, 
Error **errp)
                           d->hotplugged, 1);
 }
 
+static void virtio_ccw_post_plugged(DeviceState *d, Error **errp)
+{
+   VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
+   VirtIODevice *vdev = virtio_bus_get_device(&dev->bus);
+
+   if (!virtio_host_has_feature(vdev, VIRTIO_F_VERSION_1)) {
+        /* A backend didn't support modern virtio. */
+       dev->max_rev = 0;
+   }
+}
+
 static void virtio_ccw_device_unplugged(DeviceState *d)
 {
     VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
@@ -1891,6 +1902,7 @@ static void virtio_ccw_bus_class_init(ObjectClass *klass, 
void *data)
     k->save_config = virtio_ccw_save_config;
     k->load_config = virtio_ccw_load_config;
     k->device_plugged = virtio_ccw_device_plugged;
+    k->post_plugged = virtio_ccw_post_plugged;
     k->device_unplugged = virtio_ccw_device_unplugged;
 }
 
diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
index febda76..81c7cdd 100644
--- a/hw/virtio/virtio-bus.c
+++ b/hw/virtio/virtio-bus.c
@@ -56,6 +56,9 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error 
**errp)
     assert(vdc->get_features != NULL);
     vdev->host_features = vdc->get_features(vdev, vdev->host_features,
                                             errp);
+    if (klass->post_plugged != NULL) {
+        klass->post_plugged(qbus->parent, errp);
+    }
 }
 
 /* Reset the virtio_bus */
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index dd48562..af59496 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1626,7 +1626,6 @@ static void virtio_pci_device_plugged(DeviceState *d, 
Error **errp)
     VirtioBusState *bus = &proxy->bus;
     bool legacy = !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_LEGACY);
     bool modern = !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN);
-    bool modern_pio = proxy->flags & VIRTIO_PCI_FLAG_MODERN_PIO_NOTIFY;
     uint8_t *config;
     uint32_t size;
     VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
@@ -1653,6 +1652,65 @@ static void virtio_pci_device_plugged(DeviceState *d, 
Error **errp)
 
 
     if (modern) {
+        virtio_add_feature(&vdev->host_features, VIRTIO_F_VERSION_1);
+        /* Let's see if this feature bit sticks before doing further init. */
+    }
+
+    if (proxy->nvectors) {
+        int err = msix_init_exclusive_bar(&proxy->pci_dev, proxy->nvectors,
+                                          proxy->msix_bar);
+        if (err) {
+            /* Notice when a system that supports MSIx can't initialize it.  */
+            if (err != -ENOTSUP) {
+                error_report("unable to init msix vectors to %" PRIu32,
+                             proxy->nvectors);
+            }
+            proxy->nvectors = 0;
+        }
+    }
+
+    proxy->pci_dev.config_write = virtio_write_config;
+    proxy->pci_dev.config_read = virtio_read_config;
+
+    if (legacy) {
+        size = VIRTIO_PCI_REGION_SIZE(&proxy->pci_dev)
+            + virtio_bus_get_vdev_config_len(bus);
+        size = pow2ceil(size);
+
+        memory_region_init_io(&proxy->bar, OBJECT(proxy),
+                              &virtio_pci_config_ops,
+                              proxy, "virtio-pci", size);
+
+        pci_register_bar(&proxy->pci_dev, proxy->legacy_io_bar,
+                         PCI_BASE_ADDRESS_SPACE_IO, &proxy->bar);
+    }
+
+    if (!kvm_has_many_ioeventfds()) {
+        proxy->flags &= ~VIRTIO_PCI_FLAG_USE_IOEVENTFD;
+    }
+
+    virtio_add_feature(&vdev->host_features, VIRTIO_F_BAD_FEATURE);
+}
+
+static void virtio_pci_post_plugged(DeviceState *d, Error **errp)
+{
+    VirtIOPCIProxy *proxy = VIRTIO_PCI(d);
+    bool legacy = !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_LEGACY);
+    bool modern = !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN);
+    bool modern_pio = proxy->flags & VIRTIO_PCI_FLAG_MODERN_PIO_NOTIFY;
+    VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
+
+    if (modern && !virtio_host_has_feature(vdev, VIRTIO_F_VERSION_1)) {
+        /* A backend didn't support modern virtio. */
+        if (legacy) {
+            proxy->flags |= VIRTIO_PCI_FLAG_DISABLE_MODERN;
+            modern = false;
+        } else {
+            error_setg(errp, "can't fall back to legacy virtio");
+            return;
+        }
+    }
+    if (modern) {
         struct virtio_pci_cap cap = {
             .cap_len = sizeof cap,
         };
@@ -1672,7 +1730,6 @@ static void virtio_pci_device_plugged(DeviceState *d, 
Error **errp)
 
         struct virtio_pci_cfg_cap *cfg_mask;
 
-        virtio_add_feature(&vdev->host_features, VIRTIO_F_VERSION_1);
         virtio_pci_modern_regions_init(proxy);
 
         virtio_pci_modern_mem_region_map(proxy, &proxy->common, &cap);
@@ -1705,40 +1762,6 @@ static void virtio_pci_device_plugged(DeviceState *d, 
Error **errp)
         pci_set_long(cfg_mask->pci_cfg_data, ~0x0);
     }
 
-    if (proxy->nvectors) {
-        int err = msix_init_exclusive_bar(&proxy->pci_dev, proxy->nvectors,
-                                          proxy->msix_bar);
-        if (err) {
-            /* Notice when a system that supports MSIx can't initialize it.  */
-            if (err != -ENOTSUP) {
-                error_report("unable to init msix vectors to %" PRIu32,
-                             proxy->nvectors);
-            }
-            proxy->nvectors = 0;
-        }
-    }
-
-    proxy->pci_dev.config_write = virtio_write_config;
-    proxy->pci_dev.config_read = virtio_read_config;
-
-    if (legacy) {
-        size = VIRTIO_PCI_REGION_SIZE(&proxy->pci_dev)
-            + virtio_bus_get_vdev_config_len(bus);
-        size = pow2ceil(size);
-
-        memory_region_init_io(&proxy->bar, OBJECT(proxy),
-                              &virtio_pci_config_ops,
-                              proxy, "virtio-pci", size);
-
-        pci_register_bar(&proxy->pci_dev, proxy->legacy_io_bar,
-                         PCI_BASE_ADDRESS_SPACE_IO, &proxy->bar);
-    }
-
-    if (!kvm_has_many_ioeventfds()) {
-        proxy->flags &= ~VIRTIO_PCI_FLAG_USE_IOEVENTFD;
-    }
-
-    virtio_add_feature(&vdev->host_features, VIRTIO_F_BAD_FEATURE);
 }
 
 static void virtio_pci_device_unplugged(DeviceState *d)
@@ -2474,6 +2497,7 @@ static void virtio_pci_bus_class_init(ObjectClass *klass, 
void *data)
     k->set_guest_notifiers = virtio_pci_set_guest_notifiers;
     k->vmstate_change = virtio_pci_vmstate_change;
     k->device_plugged = virtio_pci_device_plugged;
+    k->post_plugged = virtio_pci_post_plugged;
     k->device_unplugged = virtio_pci_device_unplugged;
     k->query_nvectors = virtio_pci_query_nvectors;
 }
diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h
index 6c3d4cb..3f2c136 100644
--- a/include/hw/virtio/virtio-bus.h
+++ b/include/hw/virtio/virtio-bus.h
@@ -60,6 +60,11 @@ typedef struct VirtioBusClass {
      */
     void (*device_plugged)(DeviceState *d, Error **errp);
     /*
+     * Re-evaluate setup after feature bits have been validated
+     * by the device backend.
+     */
+    void (*post_plugged)(DeviceState *d, Error **errp);
+    /*
      * transport independent exit function.
      * This is called by virtio-bus just before the device is unplugged.
      */
-- 
2.3.9