[Qemu-devel] [2.5 issue] virtio-1 in virtio-net and old vhost

[Qemu-devel] [2.5 issue] virtio-1 in virtio-net and old vhost

[Cornelia Huck]

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...

Re: [Qemu-devel] [2.5 issue] virtio-1 in virtio-net and old vhost

[Cornelia Huck]

On Tue, 1 Dec 2015 11:11:08 +0100
Cornelia Huck <cornelia.huck@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.

Re: [Qemu-devel] [2.5 issue] virtio-1 in virtio-net and old vhost

[Cornelia Huck]

On Tue, 1 Dec 2015 13:10:40 +0100
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> On Tue, 1 Dec 2015 11:11:08 +0100
> Cornelia Huck <cornelia.huck@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.

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.

Re: [Qemu-devel] [2.5 issue] virtio-1 in virtio-net and old vhost

[Jason Wang]

On 12/01/2015 10:21 PM, Cornelia Huck wrote:
> On Tue, 1 Dec 2015 13:10:40 +0100
> Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
>
>> On Tue, 1 Dec 2015 11:11:08 +0100
>> Cornelia Huck <cornelia.huck@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.
> 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.

I wonder instead of rolling back in post_plugged(), maybe we could just
delay the region setups to post_plugged(). Or just call transport
specific device_plugged() after get_features() call in
virtio_bus_device_plugged(). And I'm not sure we need to handle
migration compatibility in this case.

>> 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.

Re: [Qemu-devel] [2.5 issue] virtio-1 in virtio-net and old vhost

[Cornelia Huck]

On Wed, 2 Dec 2015 13:54:09 +0800
Jason Wang <jasowang@redhat.com> wrote:

> I wonder instead of rolling back in post_plugged(), maybe we could just
> delay the region setups to post_plugged(). 

If this is the saner thing to do for pci, sure.

> Or just call transport
> specific device_plugged() after get_features() call in
> virtio_bus_device_plugged(). 

The problem is that the VERSION_1 bit is only added in the
->device_plugged() callbacks by the transport, so ->get_features() can
only be called after that. We have a dependency in both directions :(

> And I'm not sure we need to handle
> migration compatibility in this case.

The thing we would need to care about is basically the host kernel on
the target supporting less than the host kernel on the source. Do we
care about that in other contexts right now?

Re: [Qemu-devel] [2.5 issue] virtio-1 in virtio-net and old vhost

[Michael S. Tsirkin]

On Wed, Dec 02, 2015 at 11:11:28AM +0100, Cornelia Huck wrote:
> On Wed, 2 Dec 2015 13:54:09 +0800
> Jason Wang <jasowang@redhat.com> wrote:
> 
> > I wonder instead of rolling back in post_plugged(), maybe we could just
> > delay the region setups to post_plugged(). 
> 
> If this is the saner thing to do for pci, sure.
> 
> > Or just call transport
> > specific device_plugged() after get_features() call in
> > virtio_bus_device_plugged(). 
> 
> The problem is that the VERSION_1 bit is only added in the
> ->device_plugged() callbacks by the transport, so ->get_features() can
> only be called after that. We have a dependency in both directions :(
> 
> > And I'm not sure we need to handle
> > migration compatibility in this case.
> 
> The thing we would need to care about is basically the host kernel on
> the target supporting less than the host kernel on the source.

This is a fundamental problem, we have it with other
features like transport offloads. We don't have a solution for
this now. kvm has the same issue.

> Do we
> care about that in other contexts right now?

Re: [Qemu-devel] [2.5 issue] virtio-1 in virtio-net and old vhost

[Jason Wang]

On 12/02/2015 06:11 PM, Cornelia Huck wrote:
> On Wed, 2 Dec 2015 13:54:09 +0800
> Jason Wang <jasowang@redhat.com> wrote:
>
>> I wonder instead of rolling back in post_plugged(), maybe we could just
>> delay the region setups to post_plugged(). 
> If this is the saner thing to do for pci, sure.
>
>> Or just call transport
>> specific device_plugged() after get_features() call in
>> virtio_bus_device_plugged(). 
> The problem is that the VERSION_1 bit is only added in the
> ->device_plugged() callbacks by the transport, so ->get_features() can
> only be called after that. We have a dependency in both directions :(

Ok.

>
>> And I'm not sure we need to handle
>> migration compatibility in this case.
> The thing we would need to care about is basically the host kernel on
> the target supporting less than the host kernel on the source. Do we
> care about that in other contexts right now?

The problem is for pci: without this patch, guest may always see modern
bar is "disable-modern=false". But with this patch, on an old kernel
that does not support VERSION_1, even "disable-modern=false" were
specified, guest can not see modern bar anymore. Looks like a guest
visible change.

Re: [Qemu-devel] [2.5 issue] virtio-1 in virtio-net and old vhost

[Cornelia Huck]

On Fri, 4 Dec 2015 10:06:58 +0800
Jason Wang <jasowang@redhat.com> wrote:

> The problem is for pci: without this patch, guest may always see modern
> bar is "disable-modern=false". But with this patch, on an old kernel
> that does not support VERSION_1, even "disable-modern=false" were
> specified, guest can not see modern bar anymore. Looks like a guest
> visible change.

But the guest even see a modern bar if the host is not able to present
a spec-compliant device?

What we have now is a device that looks like a modern device but that
does not offer VERSION_1. Probably not spec compliant. If virtio-pci is
fixed to not present such broken devices to the guest, this is
guest-visible, yes; but only in the sense that the guest will no longer
see broken devices, but simply legacy devices if configured.

Re: [Qemu-devel] [2.5 issue] virtio-1 in virtio-net and old vhost

[Michael S. Tsirkin]

On Fri, Dec 04, 2015 at 11:15:48AM +0100, Cornelia Huck wrote:
> On Fri, 4 Dec 2015 10:06:58 +0800
> Jason Wang <jasowang@redhat.com> wrote:
> 
> > The problem is for pci: without this patch, guest may always see modern
> > bar is "disable-modern=false". But with this patch, on an old kernel
> > that does not support VERSION_1, even "disable-modern=false" were
> > specified, guest can not see modern bar anymore. Looks like a guest
> > visible change.
> 
> But the guest even see a modern bar if the host is not able to present
> a spec-compliant device?
> 
> What we have now is a device that looks like a modern device but that
> does not offer VERSION_1. Probably not spec compliant. If virtio-pci is
> fixed to not present such broken devices to the guest, this is
> guest-visible, yes; but only in the sense that the guest will no longer
> see broken devices, but simply legacy devices if configured.

I agree - we don't have to keep what's broken, broken.
If guests are known not to work, we don't worry about
how well they migrate.

-- 
MST