[Qemu-devel] [PATCH v2 0/2] virtio-pci: Improve device plugging whith legacy backends

[Qemu-devel] [PATCH v2 0/2] virtio-pci: Improve device plugging whith legacy backends

[Maxime Coquelin]

This series makes device plugging more robust, to avoid guest to be confused
when the backend doesn't support VIRTIO_F_VERSION_1.

The problem is seen with Linux guests running mainline kernels, when backend
doesn't support the feature:
virtio_net virtio0: virtio: device uses modern interface but does not have VIRTIO_F_VERSION_1.
When it happens, the modern device probe returns -EINVAL, whereas its caller
expects -ENODEV being returned to switch to legacy device probing.

We need to make QEMU more robust to ensure the guest won't be confused,
so this series exposes modern interface only when backend support it.

It has been tested with vhost-net and vhost-user backends in client
and server modes.

Changes since v1:
-----------------
 - Make the backend feature check function specialized to only VIRTIO_1

Maxime Coquelin (2):
  virtio: Add function to check whether backend supports VIRTIO_1
  virtio-pci: Disable modern interface if backend without
    VIRTIO_F_VERSION_1

 hw/virtio/virtio-pci.c     | 15 +++++++++++++++
 hw/virtio/virtio-pci.h     |  5 +++++
 hw/virtio/virtio.c         | 13 +++++++++++++
 include/hw/virtio/virtio.h |  1 +
 4 files changed, 34 insertions(+)

-- 
2.7.4

[Qemu-devel] [PATCH v2 1/2] virtio: Add function to check whether backend supports VIRTIO_1

[Maxime Coquelin]

This patch adds virtio_test_backend_virtio_1() function to
check whether backend supports VIRTIO_F_VERSION_1 before the
negociation takes place.

Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
Cc: Marcel Apfelbaum <marcel@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: qemu-stable@nongnu.org
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 hw/virtio/virtio.c         | 13 +++++++++++++
 include/hw/virtio/virtio.h |  1 +
 2 files changed, 14 insertions(+)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 74c085c..8b30b69 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1481,6 +1481,19 @@ void virtio_vmstate_save(QEMUFile *f, void *opaque, size_t size)
     virtio_save(VIRTIO_DEVICE(opaque), f);
 }
 
+bool virtio_test_backend_virtio_1(VirtIODevice *vdev, Error **errp)
+{
+    VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
+    uint64_t feature;
+
+    virtio_add_feature(&feature, VIRTIO_F_VERSION_1);
+
+    assert(k->get_features != NULL);
+    feature = k->get_features(vdev, feature, errp);
+
+    return virtio_has_feature(feature, VIRTIO_F_VERSION_1);
+}
+
 static int virtio_set_features_nocheck(VirtIODevice *vdev, uint64_t val)
 {
     VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index d2490c1..3a31754 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -235,6 +235,7 @@ int virtio_set_status(VirtIODevice *vdev, uint8_t val);
 void virtio_reset(void *opaque);
 void virtio_update_irq(VirtIODevice *vdev);
 int virtio_set_features(VirtIODevice *vdev, uint64_t val);
+bool virtio_test_backend_virtio_1(VirtIODevice *vdev, Error **errp);
 
 /* Base devices.  */
 typedef struct VirtIOBlkConf VirtIOBlkConf;
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 1/2] virtio: Add function to check whether backend supports VIRTIO_1

[Marcel Apfelbaum]

On 09/09/2016 04:10 PM, Maxime Coquelin wrote:
> This patch adds virtio_test_backend_virtio_1() function to
> check whether backend supports VIRTIO_F_VERSION_1 before the
> negociation takes place.
>
> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> Cc: Marcel Apfelbaum <marcel@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  hw/virtio/virtio.c         | 13 +++++++++++++
>  include/hw/virtio/virtio.h |  1 +
>  2 files changed, 14 insertions(+)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 74c085c..8b30b69 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -1481,6 +1481,19 @@ void virtio_vmstate_save(QEMUFile *f, void *opaque, size_t size)
>      virtio_save(VIRTIO_DEVICE(opaque), f);
>  }
>
> +bool virtio_test_backend_virtio_1(VirtIODevice *vdev, Error **errp)
> +{
> +    VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
> +    uint64_t feature;
> +

I would set "feature = 0" even if doesn't really matter.
Anyway:

Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>

Thanks,
Marcel

> +    virtio_add_feature(&feature, VIRTIO_F_VERSION_1);
> +
> +    assert(k->get_features != NULL);
> +    feature = k->get_features(vdev, feature, errp);
> +
> +    return virtio_has_feature(feature, VIRTIO_F_VERSION_1);
> +}
> +
>  static int virtio_set_features_nocheck(VirtIODevice *vdev, uint64_t val)
>  {
>      VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index d2490c1..3a31754 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -235,6 +235,7 @@ int virtio_set_status(VirtIODevice *vdev, uint8_t val);
>  void virtio_reset(void *opaque);
>  void virtio_update_irq(VirtIODevice *vdev);
>  int virtio_set_features(VirtIODevice *vdev, uint64_t val);
> +bool virtio_test_backend_virtio_1(VirtIODevice *vdev, Error **errp);
>
>  /* Base devices.  */
>  typedef struct VirtIOBlkConf VirtIOBlkConf;
>

Re: [Qemu-devel] [PATCH v2 1/2] virtio: Add function to check whether backend supports VIRTIO_1

[Michael S. Tsirkin]

On Fri, Sep 09, 2016 at 08:00:31PM +0300, Marcel Apfelbaum wrote:
> On 09/09/2016 04:10 PM, Maxime Coquelin wrote:
> > This patch adds virtio_test_backend_virtio_1() function to
> > check whether backend supports VIRTIO_F_VERSION_1 before the
> > negociation takes place.
> > 
> > Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> > Cc: Marcel Apfelbaum <marcel@redhat.com>
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Cc: qemu-stable@nongnu.org
> > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> > ---
> >  hw/virtio/virtio.c         | 13 +++++++++++++
> >  include/hw/virtio/virtio.h |  1 +
> >  2 files changed, 14 insertions(+)
> > 
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index 74c085c..8b30b69 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -1481,6 +1481,19 @@ void virtio_vmstate_save(QEMUFile *f, void *opaque, size_t size)
> >      virtio_save(VIRTIO_DEVICE(opaque), f);
> >  }
> > 
> > +bool virtio_test_backend_virtio_1(VirtIODevice *vdev, Error **errp)
> > +{
> > +    VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
> > +    uint64_t feature;
> > +
> 
> I would set "feature = 0" even if doesn't really matter.
> Anyway:
> 
> Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
> 
> Thanks,
> Marcel

why wouldn't it matter? Looks like an uninitialized variable to me.

> > +    virtio_add_feature(&feature, VIRTIO_F_VERSION_1);
> > +
> > +    assert(k->get_features != NULL);
> > +    feature = k->get_features(vdev, feature, errp);
> > +
> > +    return virtio_has_feature(feature, VIRTIO_F_VERSION_1);
> > +}
> > +
> >  static int virtio_set_features_nocheck(VirtIODevice *vdev, uint64_t val)
> >  {
> >      VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
> > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> > index d2490c1..3a31754 100644
> > --- a/include/hw/virtio/virtio.h
> > +++ b/include/hw/virtio/virtio.h
> > @@ -235,6 +235,7 @@ int virtio_set_status(VirtIODevice *vdev, uint8_t val);
> >  void virtio_reset(void *opaque);
> >  void virtio_update_irq(VirtIODevice *vdev);
> >  int virtio_set_features(VirtIODevice *vdev, uint64_t val);
> > +bool virtio_test_backend_virtio_1(VirtIODevice *vdev, Error **errp);
> > 
> >  /* Base devices.  */
> >  typedef struct VirtIOBlkConf VirtIOBlkConf;
> > 

Re: [Qemu-devel] [PATCH v2 1/2] virtio: Add function to check whether backend supports VIRTIO_1

[Maxime Coquelin]

On 09/09/2016 07:47 PM, Michael S. Tsirkin wrote:
> On Fri, Sep 09, 2016 at 08:00:31PM +0300, Marcel Apfelbaum wrote:
>> On 09/09/2016 04:10 PM, Maxime Coquelin wrote:
>>> This patch adds virtio_test_backend_virtio_1() function to
>>> check whether backend supports VIRTIO_F_VERSION_1 before the
>>> negociation takes place.
>>>
>>> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
>>> Cc: Marcel Apfelbaum <marcel@redhat.com>
>>> Cc: Michael S. Tsirkin <mst@redhat.com>
>>> Cc: qemu-stable@nongnu.org
>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>> ---
>>>  hw/virtio/virtio.c         | 13 +++++++++++++
>>>  include/hw/virtio/virtio.h |  1 +
>>>  2 files changed, 14 insertions(+)
>>>
>>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>>> index 74c085c..8b30b69 100644
>>> --- a/hw/virtio/virtio.c
>>> +++ b/hw/virtio/virtio.c
>>> @@ -1481,6 +1481,19 @@ void virtio_vmstate_save(QEMUFile *f, void *opaque, size_t size)
>>>      virtio_save(VIRTIO_DEVICE(opaque), f);
>>>  }
>>>
>>> +bool virtio_test_backend_virtio_1(VirtIODevice *vdev, Error **errp)
>>> +{
>>> +    VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
>>> +    uint64_t feature;
>>> +
>>
>> I would set "feature = 0" even if doesn't really matter.
>> Anyway:
>>
>> Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
>>
>> Thanks,
>> Marcel
>
> why wouldn't it matter? Looks like an uninitialized variable to me.
Oh yes, you are right, it should be initialized.
Just wait confirmation from Michael on the RFC before sending a v3.

Thanks,
Maxime
>
>>> +    virtio_add_feature(&feature, VIRTIO_F_VERSION_1);
>>> +
>>> +    assert(k->get_features != NULL);
>>> +    feature = k->get_features(vdev, feature, errp);
>>> +
>>> +    return virtio_has_feature(feature, VIRTIO_F_VERSION_1);
>>> +}
>>> +
>>>  static int virtio_set_features_nocheck(VirtIODevice *vdev, uint64_t val)
>>>  {
>>>      VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
>>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
>>> index d2490c1..3a31754 100644
>>> --- a/include/hw/virtio/virtio.h
>>> +++ b/include/hw/virtio/virtio.h
>>> @@ -235,6 +235,7 @@ int virtio_set_status(VirtIODevice *vdev, uint8_t val);
>>>  void virtio_reset(void *opaque);
>>>  void virtio_update_irq(VirtIODevice *vdev);
>>>  int virtio_set_features(VirtIODevice *vdev, uint64_t val);
>>> +bool virtio_test_backend_virtio_1(VirtIODevice *vdev, Error **errp);
>>>
>>>  /* Base devices.  */
>>>  typedef struct VirtIOBlkConf VirtIOBlkConf;
>>>

Re: [Qemu-devel] [PATCH v2 1/2] virtio: Add function to check whether backend supports VIRTIO_1

[Marcel Apfelbaum]

On 09/09/2016 08:47 PM, Michael S. Tsirkin wrote:
> On Fri, Sep 09, 2016 at 08:00:31PM +0300, Marcel Apfelbaum wrote:
>> On 09/09/2016 04:10 PM, Maxime Coquelin wrote:
>>> This patch adds virtio_test_backend_virtio_1() function to
>>> check whether backend supports VIRTIO_F_VERSION_1 before the
>>> negociation takes place.
>>>
>>> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
>>> Cc: Marcel Apfelbaum <marcel@redhat.com>
>>> Cc: Michael S. Tsirkin <mst@redhat.com>
>>> Cc: qemu-stable@nongnu.org
>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>> ---
>>>  hw/virtio/virtio.c         | 13 +++++++++++++
>>>  include/hw/virtio/virtio.h |  1 +
>>>  2 files changed, 14 insertions(+)
>>>
>>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>>> index 74c085c..8b30b69 100644
>>> --- a/hw/virtio/virtio.c
>>> +++ b/hw/virtio/virtio.c
>>> @@ -1481,6 +1481,19 @@ void virtio_vmstate_save(QEMUFile *f, void *opaque, size_t size)
>>>      virtio_save(VIRTIO_DEVICE(opaque), f);
>>>  }
>>>
>>> +bool virtio_test_backend_virtio_1(VirtIODevice *vdev, Error **errp)
>>> +{
>>> +    VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
>>> +    uint64_t feature;
>>> +
>>
>> I would set "feature = 0" even if doesn't really matter.
>> Anyway:
>>
>> Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
>>
>> Thanks,
>> Marcel
>
> why wouldn't it matter? Looks like an uninitialized variable to me.

because it adds a flag, then it checks only if this specific flag is
negotiated, but uninitialized variables are never a good idea.

Thanks,
Marcel

>
>>> +    virtio_add_feature(&feature, VIRTIO_F_VERSION_1);
>>> +
>>> +    assert(k->get_features != NULL);
>>> +    feature = k->get_features(vdev, feature, errp);
>>> +
>>> +    return virtio_has_feature(feature, VIRTIO_F_VERSION_1);
>>> +}
>>> +
>>>  static int virtio_set_features_nocheck(VirtIODevice *vdev, uint64_t val)
>>>  {
>>>      VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
>>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
>>> index d2490c1..3a31754 100644
>>> --- a/include/hw/virtio/virtio.h
>>> +++ b/include/hw/virtio/virtio.h
>>> @@ -235,6 +235,7 @@ int virtio_set_status(VirtIODevice *vdev, uint8_t val);
>>>  void virtio_reset(void *opaque);
>>>  void virtio_update_irq(VirtIODevice *vdev);
>>>  int virtio_set_features(VirtIODevice *vdev, uint64_t val);
>>> +bool virtio_test_backend_virtio_1(VirtIODevice *vdev, Error **errp);
>>>
>>>  /* Base devices.  */
>>>  typedef struct VirtIOBlkConf VirtIOBlkConf;
>>>

Re: [Qemu-devel] [PATCH v2 1/2] virtio: Add function to check whether backend supports VIRTIO_1

[Cornelia Huck]

On Fri,  9 Sep 2016 15:10:06 +0200
Maxime Coquelin <maxime.coquelin@redhat.com> wrote:

> This patch adds virtio_test_backend_virtio_1() function to
> check whether backend supports VIRTIO_F_VERSION_1 before the
> negociation takes place.

s/negociation/negotiation/

> 
> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> Cc: Marcel Apfelbaum <marcel@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  hw/virtio/virtio.c         | 13 +++++++++++++
>  include/hw/virtio/virtio.h |  1 +
>  2 files changed, 14 insertions(+)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 74c085c..8b30b69 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -1481,6 +1481,19 @@ void virtio_vmstate_save(QEMUFile *f, void *opaque, size_t size)
>      virtio_save(VIRTIO_DEVICE(opaque), f);
>  }
> 
> +bool virtio_test_backend_virtio_1(VirtIODevice *vdev, Error **errp)
> +{
> +    VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
> +    uint64_t feature;

As others had requested, I'd prefer setting this to 0 as well.

With that changed:

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

Re: [Qemu-devel] [PATCH v2 1/2] virtio: Add function to check whether backend supports VIRTIO_1

[Maxime Coquelin]

On 09/12/2016 02:28 PM, Cornelia Huck wrote:
> On Fri,  9 Sep 2016 15:10:06 +0200
> Maxime Coquelin <maxime.coquelin@redhat.com> wrote:
>
>> This patch adds virtio_test_backend_virtio_1() function to
>> check whether backend supports VIRTIO_F_VERSION_1 before the
>> negociation takes place.
>
> s/negociation/negotiation/
>
>>
>> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
>> Cc: Marcel Apfelbaum <marcel@redhat.com>
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> Cc: qemu-stable@nongnu.org
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> ---
>>  hw/virtio/virtio.c         | 13 +++++++++++++
>>  include/hw/virtio/virtio.h |  1 +
>>  2 files changed, 14 insertions(+)
>>
>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>> index 74c085c..8b30b69 100644
>> --- a/hw/virtio/virtio.c
>> +++ b/hw/virtio/virtio.c
>> @@ -1481,6 +1481,19 @@ void virtio_vmstate_save(QEMUFile *f, void *opaque, size_t size)
>>      virtio_save(VIRTIO_DEVICE(opaque), f);
>>  }
>>
>> +bool virtio_test_backend_virtio_1(VirtIODevice *vdev, Error **errp)
>> +{
>> +    VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
>> +    uint64_t feature;
>
> As others had requested, I'd prefer setting this to 0 as well.
>
> With that changed:
>
> Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Thanks Cornelia,
Note that if everyone agree to have patch adding pre_plugged,
this series will not be needed anymore.

Regards,
Maxime

Re: [Qemu-devel] [PATCH v2 1/2] virtio: Add function to check whether backend supports VIRTIO_1

[Michael S. Tsirkin]

On Mon, Sep 12, 2016 at 06:42:46PM +0200, Maxime Coquelin wrote:
> 
> 
> On 09/12/2016 02:28 PM, Cornelia Huck wrote:
> > On Fri,  9 Sep 2016 15:10:06 +0200
> > Maxime Coquelin <maxime.coquelin@redhat.com> wrote:
> > 
> > > This patch adds virtio_test_backend_virtio_1() function to
> > > check whether backend supports VIRTIO_F_VERSION_1 before the
> > > negociation takes place.
> > 
> > s/negociation/negotiation/
> > 
> > > 
> > > Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> > > Cc: Marcel Apfelbaum <marcel@redhat.com>
> > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > Cc: qemu-stable@nongnu.org
> > > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> > > ---
> > >  hw/virtio/virtio.c         | 13 +++++++++++++
> > >  include/hw/virtio/virtio.h |  1 +
> > >  2 files changed, 14 insertions(+)
> > > 
> > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > > index 74c085c..8b30b69 100644
> > > --- a/hw/virtio/virtio.c
> > > +++ b/hw/virtio/virtio.c
> > > @@ -1481,6 +1481,19 @@ void virtio_vmstate_save(QEMUFile *f, void *opaque, size_t size)
> > >      virtio_save(VIRTIO_DEVICE(opaque), f);
> > >  }
> > > 
> > > +bool virtio_test_backend_virtio_1(VirtIODevice *vdev, Error **errp)
> > > +{
> > > +    VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
> > > +    uint64_t feature;
> > 
> > As others had requested, I'd prefer setting this to 0 as well.
> > 
> > With that changed:
> > 
> > Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> Thanks Cornelia,
> Note that if everyone agree to have patch adding pre_plugged,
> this series will not be needed anymore.
> 
> Regards,
> Maxime

Yes - that one needs testing for ccw and mmio.

-- 
MST

[Qemu-devel] [PATCH v2 2/2] virtio-pci: Disable modern interface if backend without VIRTIO_F_VERSION_1

[Maxime Coquelin]

This patch makes pci devices plugging more robust, by not confusing
guest with modern interface when the backend doesn't support
VIRTIO_F_VERSION_1.

Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
Cc: Marcel Apfelbaum <marcel@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: qemu-stable@nongnu.org
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 hw/virtio/virtio-pci.c | 15 +++++++++++++++
 hw/virtio/virtio-pci.h |  5 +++++
 2 files changed, 20 insertions(+)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 755f921..9e88d7b 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1581,6 +1581,21 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
     uint32_t size;
     VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
 
+    /*
+     * Virtio capabilities present without
+     * VIRTIO_F_VERSION_1 confuses guests
+     */
+    if (!virtio_test_backend_virtio_1(vdev, errp)) {
+        virtio_pci_disable_modern(proxy);
+    }
+
+    legacy = virtio_pci_legacy(proxy);
+    modern = virtio_pci_modern(proxy);
+    if (!legacy && !modern) {
+        error_setg(errp, "PCI device is neither legacy nor modern.");
+        return;
+    }
+
     config = proxy->pci_dev.config;
     if (proxy->class_code) {
         pci_config_set_class(config, proxy->class_code);
diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
index 25fbf8a..4e976b3 100644
--- a/hw/virtio/virtio-pci.h
+++ b/hw/virtio/virtio-pci.h
@@ -172,6 +172,11 @@ static inline void virtio_pci_force_virtio_1(VirtIOPCIProxy *proxy)
     proxy->disable_legacy = ON_OFF_AUTO_ON;
 }
 
+static inline void virtio_pci_disable_modern(VirtIOPCIProxy *proxy)
+{
+    proxy->disable_modern = true;
+}
+
 /*
  * virtio-scsi-pci: This extends VirtioPCIProxy.
  */
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 2/2] virtio-pci: Disable modern interface if backend without VIRTIO_F_VERSION_1

[Marcel Apfelbaum]

On 09/09/2016 04:10 PM, Maxime Coquelin wrote:
> This patch makes pci devices plugging more robust, by not confusing
> guest with modern interface when the backend doesn't support
> VIRTIO_F_VERSION_1.
>
> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> Cc: Marcel Apfelbaum <marcel@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  hw/virtio/virtio-pci.c | 15 +++++++++++++++
>  hw/virtio/virtio-pci.h |  5 +++++
>  2 files changed, 20 insertions(+)
>
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 755f921..9e88d7b 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1581,6 +1581,21 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
>      uint32_t size;
>      VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
>
> +    /*
> +     * Virtio capabilities present without
> +     * VIRTIO_F_VERSION_1 confuses guests
> +     */
> +    if (!virtio_test_backend_virtio_1(vdev, errp)) {
> +        virtio_pci_disable_modern(proxy);
> +    }
> +
> +    legacy = virtio_pci_legacy(proxy);
> +    modern = virtio_pci_modern(proxy);

I think the first initialization of the
legacy/modern variables is not necessary anymore.

Other than that it looks good to me.

Thanks,
Marcel

> +    if (!legacy && !modern) {
> +        error_setg(errp, "PCI device is neither legacy nor modern.");
> +        return;
> +    }
> +
>      config = proxy->pci_dev.config;
>      if (proxy->class_code) {
>          pci_config_set_class(config, proxy->class_code);
> diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
> index 25fbf8a..4e976b3 100644
> --- a/hw/virtio/virtio-pci.h
> +++ b/hw/virtio/virtio-pci.h
> @@ -172,6 +172,11 @@ static inline void virtio_pci_force_virtio_1(VirtIOPCIProxy *proxy)
>      proxy->disable_legacy = ON_OFF_AUTO_ON;
>  }
>
> +static inline void virtio_pci_disable_modern(VirtIOPCIProxy *proxy)
> +{
> +    proxy->disable_modern = true;
> +}
> +
>  /*
>   * virtio-scsi-pci: This extends VirtioPCIProxy.
>   */
>

Re: [Qemu-devel] [PATCH v2 2/2] virtio-pci: Disable modern interface if backend without VIRTIO_F_VERSION_1

[Michael S. Tsirkin]

On Fri, Sep 09, 2016 at 03:10:07PM +0200, Maxime Coquelin wrote:
> This patch makes pci devices plugging more robust, by not confusing
> guest with modern interface when the backend doesn't support
> VIRTIO_F_VERSION_1.
> 
> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> Cc: Marcel Apfelbaum <marcel@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  hw/virtio/virtio-pci.c | 15 +++++++++++++++
>  hw/virtio/virtio-pci.h |  5 +++++
>  2 files changed, 20 insertions(+)
> 
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 755f921..9e88d7b 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1581,6 +1581,21 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
>      uint32_t size;
>      VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
>  
> +    /*
> +     * Virtio capabilities present without
> +     * VIRTIO_F_VERSION_1 confuses guests
> +     */
> +    if (!virtio_test_backend_virtio_1(vdev, errp)) {
> +        virtio_pci_disable_modern(proxy);
> +    }
> +
> +    legacy = virtio_pci_legacy(proxy);
> +    modern = virtio_pci_modern(proxy);
> +    if (!legacy && !modern) {
> +        error_setg(errp, "PCI device is neither legacy nor modern.");
> +        return;
> +    }
> +

How does this interact with
	virtio-pci: error out when both legacy and modern modes are disabled
?
If it's the same, I'd rather pick that one and apply your
change on top.

>      config = proxy->pci_dev.config;
>      if (proxy->class_code) {
>          pci_config_set_class(config, proxy->class_code);
> diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
> index 25fbf8a..4e976b3 100644
> --- a/hw/virtio/virtio-pci.h
> +++ b/hw/virtio/virtio-pci.h
> @@ -172,6 +172,11 @@ static inline void virtio_pci_force_virtio_1(VirtIOPCIProxy *proxy)
>      proxy->disable_legacy = ON_OFF_AUTO_ON;
>  }
>  
> +static inline void virtio_pci_disable_modern(VirtIOPCIProxy *proxy)
> +{
> +    proxy->disable_modern = true;
> +}
> +
>  /*
>   * virtio-scsi-pci: This extends VirtioPCIProxy.
>   */
> -- 
> 2.7.4

Re: [Qemu-devel] [PATCH v2 2/2] virtio-pci: Disable modern interface if backend without VIRTIO_F_VERSION_1

[Maxime Coquelin]

On 09/09/2016 07:48 PM, Michael S. Tsirkin wrote:
> On Fri, Sep 09, 2016 at 03:10:07PM +0200, Maxime Coquelin wrote:
>> This patch makes pci devices plugging more robust, by not confusing
>> guest with modern interface when the backend doesn't support
>> VIRTIO_F_VERSION_1.
>>
>> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
>> Cc: Marcel Apfelbaum <marcel@redhat.com>
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> Cc: qemu-stable@nongnu.org
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> ---
>>  hw/virtio/virtio-pci.c | 15 +++++++++++++++
>>  hw/virtio/virtio-pci.h |  5 +++++
>>  2 files changed, 20 insertions(+)
>>
>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>> index 755f921..9e88d7b 100644
>> --- a/hw/virtio/virtio-pci.c
>> +++ b/hw/virtio/virtio-pci.c
>> @@ -1581,6 +1581,21 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
>>      uint32_t size;
>>      VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
>>
>> +    /*
>> +     * Virtio capabilities present without
>> +     * VIRTIO_F_VERSION_1 confuses guests
>> +     */
>> +    if (!virtio_test_backend_virtio_1(vdev, errp)) {
>> +        virtio_pci_disable_modern(proxy);
>> +    }
>> +
>> +    legacy = virtio_pci_legacy(proxy);
>> +    modern = virtio_pci_modern(proxy);
>> +    if (!legacy && !modern) {
>> +        error_setg(errp, "PCI device is neither legacy nor modern.");
>> +        return;
>> +    }
>> +
>
> How does this interact with
> 	virtio-pci: error out when both legacy and modern modes are disabled
> ?
> If it's the same, I'd rather pick that one and apply your
> change on top.
Not exactly, since with my patch modern can be disabled after realize
callback has been called.

But I will base my next revision on top of Greg's patch,
and use the same pattern as he did use.

Thanks,
Maxime