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