On Tue, 1 Dec 2015 11:11:08 +0100 Cornelia Huck <cornelia.h...@de.ibm.com> wrote:
> Some of our test folks tried to run a recent-ish qemu (nearly 2.5) > combined with an old host kernel (and a virtio-1 capable guest). > > In that setup, we had the transport (in that case, virtio-ccw) > advertise VERSION_1 as it is a revision 1 device. However, the old > vhost driver did not support virtio-1 and therefore cleared the > VERSION_1 bit. In the end, qemu did not offer VERSION_1 to the guest > for a revision 1 device, which the guest treats as a fatal error. > > It looks to me as if virtio-pci has the same problem: The kernel will > detect a modern device as by the I/O layout and then barf at the > missing VERSION_1 feature bit. > > We _could_ make this missing VERSION_1 bit non-fatal in the guest, but > that does not fix guests that are already out there. > > The problem is that the transport cannot know whether the VERSION_1 bit > will be pulled from under it later during device setup: This is only > done in the ->get_features() callback when virtio-net will handle the > features supported by vhost. > > I'm currently lacking a good idea on how to fix this, but I think that > is an issue that should be dealt with for 2.5... What about the following (completely untested)? Have the transport verify that VERSION_1 is still present after get_features. Should do for virtio-ccw, but I'm not sure whether virtio-pci can be unrolled in that way. diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c index fb103b7..87ecbc1 100644 --- a/hw/s390x/virtio-ccw.c +++ b/hw/s390x/virtio-ccw.c @@ -1555,6 +1555,16 @@ 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)) { + dev->max_rev = 0; + } +} + static void virtio_ccw_device_unplugged(DeviceState *d) { VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d); @@ -1891,6 +1901,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..06e449c 100644 --- a/hw/virtio/virtio-pci.c +++ b/hw/virtio/virtio-pci.c @@ -1741,6 +1741,30 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp) 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)) { + if (legacy) { + virtio_pci_modern_mem_region_unmap(proxy, &proxy->common); + virtio_pci_modern_mem_region_unmap(proxy, &proxy->isr); + virtio_pci_modern_mem_region_unmap(proxy, &proxy->device); + virtio_pci_modern_mem_region_unmap(proxy, &proxy->notify); + if (modern_pio) { + virtio_pci_modern_io_region_unmap(proxy, &proxy->notify_pio); + } + proxy->flags |= VIRTIO_PCI_FLAG_DISABLE_MODERN; + } else { + error_setg(errp, "can't fall back to legacy virtio"); + } + } +} + static void virtio_pci_device_unplugged(DeviceState *d) { VirtIOPCIProxy *proxy = VIRTIO_PCI(d); @@ -2474,6 +2498,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..ff0a3b0 100644 --- a/include/hw/virtio/virtio-bus.h +++ b/include/hw/virtio/virtio-bus.h @@ -59,6 +59,7 @@ typedef struct VirtioBusClass { * This is called by virtio-bus just after the device is plugged. */ void (*device_plugged)(DeviceState *d, Error **errp); + void (*post_plugged)(DeviceState *d, Error **errp); /* * transport independent exit function. * This is called by virtio-bus just before the device is unplugged.