From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39418) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a3lok-0006s4-EW for qemu-devel@nongnu.org; Tue, 01 Dec 2015 09:22:07 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a3log-0002F4-DJ for qemu-devel@nongnu.org; Tue, 01 Dec 2015 09:22:06 -0500 Received: from e06smtp06.uk.ibm.com ([195.75.94.102]:34236) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a3log-0002Eq-0n for qemu-devel@nongnu.org; Tue, 01 Dec 2015 09:22:02 -0500 Received: from localhost by e06smtp06.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 1 Dec 2015 14:22:00 -0000 Received: from b06cxnps4074.portsmouth.uk.ibm.com (d06relay11.portsmouth.uk.ibm.com [9.149.109.196]) by d06dlp03.portsmouth.uk.ibm.com (Postfix) with ESMTP id DAF5B1B08067 for ; Tue, 1 Dec 2015 14:22:21 +0000 (GMT) Received: from d06av10.portsmouth.uk.ibm.com (d06av10.portsmouth.uk.ibm.com [9.149.37.251]) by b06cxnps4074.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id tB1ELtEm64946300 for ; Tue, 1 Dec 2015 14:21:56 GMT Received: from d06av10.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av10.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id tB1DLuIn012679 for ; Tue, 1 Dec 2015 06:21:57 -0700 Date: Tue, 1 Dec 2015 15:21:53 +0100 From: Cornelia Huck Message-ID: <20151201152153.1bb1f58f.cornelia.huck@de.ibm.com> In-Reply-To: <20151201131040.046ab170.cornelia.huck@de.ibm.com> References: <20151201111108.6dd85381.cornelia.huck@de.ibm.com> <20151201131040.046ab170.cornelia.huck@de.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [2.5 issue] virtio-1 in virtio-net and old vhost List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: Christian Borntraeger , Jason Wang , qemu-devel@nongnu.org On Tue, 1 Dec 2015 13:10:40 +0100 Cornelia Huck wrote: > On Tue, 1 Dec 2015 11:11:08 +0100 > Cornelia Huck 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. I've confirmed that this fixes an old host kernel + new qemu setup that exhibits the "guest displeased by missing VERSION_1" syndrome for virtio-ccw. > > 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.