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

[Qemu-devel] [PATCH 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.

Maxime Coquelin (2):
  virtio: Add backend feature testing functionnality
  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         | 14 ++++++++++++++
 include/hw/virtio/virtio.h |  2 ++
 4 files changed, 36 insertions(+)

-- 
2.7.4

[Qemu-devel] [PATCH 1/2] virtio: Add backend feature testing functionnality

[Maxime Coquelin]

This patch adds virtio_test_backend_feature() function to
enable checking a backend feature before the negociation
takes place.

It may be used, for example, to check whether the backend
supports VIRTIO_F_VERSION_1 before enabling modern
capabilities.

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         | 14 ++++++++++++++
 include/hw/virtio/virtio.h |  2 ++
 2 files changed, 16 insertions(+)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 74c085c..7ab91a1 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1481,6 +1481,20 @@ void virtio_vmstate_save(QEMUFile *f, void *opaque, size_t size)
     virtio_save(VIRTIO_DEVICE(opaque), f);
 }
 
+bool virtio_test_backend_feature(VirtIODevice *vdev,
+                                 unsigned int fbit, Error **errp)
+{
+    VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
+    uint64_t feature;
+
+    virtio_add_feature(&feature, fbit);
+
+    assert(k->get_features != NULL);
+    feature = k->get_features(vdev, feature, errp);
+
+    return virtio_has_feature(feature, fbit);
+}
+
 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..5fb74c8 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -235,6 +235,8 @@ 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_feature(VirtIODevice *vdev,
+                                 unsigned int fbit, Error **errp);
 
 /* Base devices.  */
 typedef struct VirtIOBlkConf VirtIOBlkConf;
-- 
2.7.4

[Qemu-devel] [PATCH 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: 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..0e5d59c 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_feature(vdev, VIRTIO_F_VERSION_1, 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 1/2] virtio: Add backend feature testing functionnality

[Cornelia Huck]

On Fri,  9 Sep 2016 12:14:31 +0200
Maxime Coquelin <maxime.coquelin@redhat.com> wrote:

> This patch adds virtio_test_backend_feature() function to
> enable checking a backend feature before the negociation
> takes place.
> 
> It may be used, for example, to check whether the backend
> supports VIRTIO_F_VERSION_1 before enabling modern
> capabilities.
> 
> 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         | 14 ++++++++++++++
>  include/hw/virtio/virtio.h |  2 ++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 74c085c..7ab91a1 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -1481,6 +1481,20 @@ void virtio_vmstate_save(QEMUFile *f, void *opaque, size_t size)
>      virtio_save(VIRTIO_DEVICE(opaque), f);
>  }
> 
> +bool virtio_test_backend_feature(VirtIODevice *vdev,
> +                                 unsigned int fbit, Error **errp)
> +{
> +    VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
> +    uint64_t feature;
> +
> +    virtio_add_feature(&feature, fbit);
> +
> +    assert(k->get_features != NULL);
> +    feature = k->get_features(vdev, feature, errp);
> +
> +    return virtio_has_feature(feature, fbit);
> +}
> +
>  static int virtio_set_features_nocheck(VirtIODevice *vdev, uint64_t val)
>  {
>      VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);

What happens if you want to test for features that depend upon each
other? The backend may support your feature, but it may withdraw the
feature bit if a dependency is not fullfilled.

You'll probably want to run validation on the whole feature set; but
that is hard if you're too early in the setup process.

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

[Cornelia Huck]

On Fri,  9 Sep 2016 12:14:32 +0200
Maxime Coquelin <maxime.coquelin@redhat.com> 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: 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(+)

Note that 11380b361 ("virtio: handle non-virtio-1-capable backend for
ccw") fixes this issue for ccw via the introduction of a
->post_plugged() callback. Unfortunately, we did not find a good way to
make it work for pci back then.

Two comments:
- ->post_plugged() handles the dependencies (as noticed for your first
patch) - and this is due to it being called after the plugging is
already done.
- I don't really like pci and ccw being too different. We have probably
more flexibility with the handling for ccw, so I could probably convert
ccw to the same mechanism that pci uses. Maybe there are other uses for
->post_plugged()?

Re: [Qemu-devel] [PATCH 1/2] virtio: Add backend feature testing functionnality

[Marcel Apfelbaum]

On 09/09/2016 01:33 PM, Cornelia Huck wrote:
> On Fri,  9 Sep 2016 12:14:31 +0200
> Maxime Coquelin <maxime.coquelin@redhat.com> wrote:
>
>> This patch adds virtio_test_backend_feature() function to
>> enable checking a backend feature before the negociation
>> takes place.
>>
>> It may be used, for example, to check whether the backend
>> supports VIRTIO_F_VERSION_1 before enabling modern
>> capabilities.
>>
>> 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         | 14 ++++++++++++++
>>  include/hw/virtio/virtio.h |  2 ++
>>  2 files changed, 16 insertions(+)
>>
>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>> index 74c085c..7ab91a1 100644
>> --- a/hw/virtio/virtio.c
>> +++ b/hw/virtio/virtio.c
>> @@ -1481,6 +1481,20 @@ void virtio_vmstate_save(QEMUFile *f, void *opaque, size_t size)
>>      virtio_save(VIRTIO_DEVICE(opaque), f);
>>  }
>>
>> +bool virtio_test_backend_feature(VirtIODevice *vdev,
>> +                                 unsigned int fbit, Error **errp)
>> +{
>> +    VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
>> +    uint64_t feature;
>> +
>> +    virtio_add_feature(&feature, fbit);
>> +
>> +    assert(k->get_features != NULL);
>> +    feature = k->get_features(vdev, feature, errp);
>> +
>> +    return virtio_has_feature(feature, fbit);
>> +}
>> +
>>  static int virtio_set_features_nocheck(VirtIODevice *vdev, uint64_t val)
>>  {
>>      VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
>
> What happens if you want to test for features that depend upon each
> other? The backend may support your feature, but it may withdraw the
> feature bit if a dependency is not fullfilled.
>
> You'll probably want to run validation on the whole feature set; but
> that is hard if you're too early in the setup process.
>

While I agree with the feature dependency issue , would the negation be ok?
What I mean is: if the backend does not support feature X, no
matter what the depending features are, it will still not support it after the negotiation.

Changing the function to virtio_backend_unsupported_feature(x) would be better?

Thanks,
Marcel

Re: [Qemu-devel] [PATCH 1/2] virtio: Add backend feature testing functionnality

[Cornelia Huck]

On Fri, 9 Sep 2016 13:48:00 +0300
Marcel Apfelbaum <marcel@redhat.com> wrote:

> On 09/09/2016 01:33 PM, Cornelia Huck wrote:
> > On Fri,  9 Sep 2016 12:14:31 +0200
> > Maxime Coquelin <maxime.coquelin@redhat.com> wrote:
> >
> >> This patch adds virtio_test_backend_feature() function to
> >> enable checking a backend feature before the negociation
> >> takes place.
> >>
> >> It may be used, for example, to check whether the backend
> >> supports VIRTIO_F_VERSION_1 before enabling modern
> >> capabilities.
> >>
> >> 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         | 14 ++++++++++++++
> >>  include/hw/virtio/virtio.h |  2 ++
> >>  2 files changed, 16 insertions(+)
> >>
> >> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> >> index 74c085c..7ab91a1 100644
> >> --- a/hw/virtio/virtio.c
> >> +++ b/hw/virtio/virtio.c
> >> @@ -1481,6 +1481,20 @@ void virtio_vmstate_save(QEMUFile *f, void *opaque, size_t size)
> >>      virtio_save(VIRTIO_DEVICE(opaque), f);
> >>  }
> >>
> >> +bool virtio_test_backend_feature(VirtIODevice *vdev,
> >> +                                 unsigned int fbit, Error **errp)
> >> +{
> >> +    VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
> >> +    uint64_t feature;
> >> +
> >> +    virtio_add_feature(&feature, fbit);
> >> +
> >> +    assert(k->get_features != NULL);
> >> +    feature = k->get_features(vdev, feature, errp);
> >> +
> >> +    return virtio_has_feature(feature, fbit);
> >> +}
> >> +
> >>  static int virtio_set_features_nocheck(VirtIODevice *vdev, uint64_t val)
> >>  {
> >>      VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
> >
> > What happens if you want to test for features that depend upon each
> > other? The backend may support your feature, but it may withdraw the
> > feature bit if a dependency is not fullfilled.
> >
> > You'll probably want to run validation on the whole feature set; but
> > that is hard if you're too early in the setup process.
> >
> 
> While I agree with the feature dependency issue , would the negation be ok?
> What I mean is: if the backend does not support feature X, no
> matter what the depending features are, it will still not support it after the negotiation.
> 
> Changing the function to virtio_backend_unsupported_feature(x) would be better?

I think yes, although that would mean we need a new query function that
pokes through all the layers, no?

Re: [Qemu-devel] [PATCH 1/2] virtio: Add backend feature testing functionnality

[Marcel Apfelbaum]

On 09/09/2016 01:55 PM, Cornelia Huck wrote:
> On Fri, 9 Sep 2016 13:48:00 +0300
> Marcel Apfelbaum <marcel@redhat.com> wrote:
>
>> On 09/09/2016 01:33 PM, Cornelia Huck wrote:
>>> On Fri,  9 Sep 2016 12:14:31 +0200
>>> Maxime Coquelin <maxime.coquelin@redhat.com> wrote:
>>>
>>>> This patch adds virtio_test_backend_feature() function to
>>>> enable checking a backend feature before the negociation
>>>> takes place.
>>>>
>>>> It may be used, for example, to check whether the backend
>>>> supports VIRTIO_F_VERSION_1 before enabling modern
>>>> capabilities.
>>>>
>>>> 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         | 14 ++++++++++++++
>>>>  include/hw/virtio/virtio.h |  2 ++
>>>>  2 files changed, 16 insertions(+)
>>>>
>>>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>>>> index 74c085c..7ab91a1 100644
>>>> --- a/hw/virtio/virtio.c
>>>> +++ b/hw/virtio/virtio.c
>>>> @@ -1481,6 +1481,20 @@ void virtio_vmstate_save(QEMUFile *f, void *opaque, size_t size)
>>>>      virtio_save(VIRTIO_DEVICE(opaque), f);
>>>>  }
>>>>
>>>> +bool virtio_test_backend_feature(VirtIODevice *vdev,
>>>> +                                 unsigned int fbit, Error **errp)
>>>> +{
>>>> +    VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
>>>> +    uint64_t feature;
>>>> +
>>>> +    virtio_add_feature(&feature, fbit);
>>>> +
>>>> +    assert(k->get_features != NULL);
>>>> +    feature = k->get_features(vdev, feature, errp);
>>>> +
>>>> +    return virtio_has_feature(feature, fbit);
>>>> +}
>>>> +
>>>>  static int virtio_set_features_nocheck(VirtIODevice *vdev, uint64_t val)
>>>>  {
>>>>      VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
>>>
>>> What happens if you want to test for features that depend upon each
>>> other? The backend may support your feature, but it may withdraw the
>>> feature bit if a dependency is not fullfilled.
>>>
>>> You'll probably want to run validation on the whole feature set; but
>>> that is hard if you're too early in the setup process.
>>>
>>
>> While I agree with the feature dependency issue , would the negation be ok?
>> What I mean is: if the backend does not support feature X, no
>> matter what the depending features are, it will still not support it after the negotiation.
>>
>> Changing the function to virtio_backend_unsupported_feature(x) would be better?
>
> I think yes, although that would mean we need a new query function that
> pokes through all the layers, no?
>

I was thinking to keep the same function proposed by Maxime and change it to negate things:

/*
  * A missing feature before all negotiations finished will still be missing at the end.
  */
bool virtio_test_backend_unsupported_feature(VirtIODevice *vdev,
                                              unsigned int fbit, Error **errp)
{
     VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
     uint64_t feature;

     virtio_add_feature(&feature, fbit);

     assert(k->get_features != NULL);
     feature = k->get_features(vdev, feature, errp);

     return !virtio_has_feature(feature, fbit);
}

We only check if the feature was not there from the start.

Thanks,
Marcel

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

[Marcel Apfelbaum]

On 09/09/2016 01:40 PM, Cornelia Huck wrote:
> On Fri,  9 Sep 2016 12:14:32 +0200
> Maxime Coquelin <maxime.coquelin@redhat.com> 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: 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(+)
>
> Note that 11380b361 ("virtio: handle non-virtio-1-capable backend for
> ccw") fixes this issue for ccw via the introduction of a
> ->post_plugged() callback. Unfortunately, we did not find a good way to
> make it work for pci back then.

It seems that for ccw is enough to rewind dev->rev_max,
sadly for pci we need to rewind a lot of settings/resources.

Thanks,
Marcel

>
> Two comments:
> - ->post_plugged() handles the dependencies (as noticed for your first
> patch) - and this is due to it being called after the plugging is
> already done.
> - I don't really like pci and ccw being too different. We have probably
> more flexibility with the handling for ccw, so I could probably convert
> ccw to the same mechanism that pci uses. Maybe there are other uses for
> ->post_plugged()?
>

Re: [Qemu-devel] [PATCH 1/2] virtio: Add backend feature testing functionnality

[Cornelia Huck]

On Fri, 9 Sep 2016 14:02:17 +0300
Marcel Apfelbaum <marcel@redhat.com> wrote:

> I was thinking to keep the same function proposed by Maxime and change it to negate things:
> 
> /*
>   * A missing feature before all negotiations finished will still be missing at the end.
>   */
> bool virtio_test_backend_unsupported_feature(VirtIODevice *vdev,
>                                               unsigned int fbit, Error **errp)
> {
>      VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
>      uint64_t feature;
> 
>      virtio_add_feature(&feature, fbit);
> 
>      assert(k->get_features != NULL);
>      feature = k->get_features(vdev, feature, errp);
> 
>      return !virtio_has_feature(feature, fbit);
> }
> 
> We only check if the feature was not there from the start.

I think you'll still end up with the same problem (overindicating
unsupportedness), as you start out with an otherwise empty feature
set, causing features with dependencies to be removed. I fear that
anything starting with an incomplete feature list will have that
problem.

Maybe it would be better to limit this to the one bit we currently want
to test (VERSION_1)? We know the semantics of that one. Less general,
but also less headaches.

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

[Cornelia Huck]

On Fri, 9 Sep 2016 14:04:55 +0300
Marcel Apfelbaum <marcel@redhat.com> wrote:

> On 09/09/2016 01:40 PM, Cornelia Huck wrote:
> > On Fri,  9 Sep 2016 12:14:32 +0200
> > Maxime Coquelin <maxime.coquelin@redhat.com> 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: 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(+)
> >
> > Note that 11380b361 ("virtio: handle non-virtio-1-capable backend for
> > ccw") fixes this issue for ccw via the introduction of a
> > ->post_plugged() callback. Unfortunately, we did not find a good way to
> > make it work for pci back then.
> 
> It seems that for ccw is enough to rewind dev->rev_max,
> sadly for pci we need to rewind a lot of settings/resources.

Yes, that what I meant with 'more flexibility for ccw'.

> 
> Thanks,
> Marcel
> 
> >
> > Two comments:
> > - ->post_plugged() handles the dependencies (as noticed for your first
> > patch) - and this is due to it being called after the plugging is
> > already done.
> > - I don't really like pci and ccw being too different. We have probably
> > more flexibility with the handling for ccw, so I could probably convert
> > ccw to the same mechanism that pci uses. Maybe there are other uses for
> > ->post_plugged()?
> >
> 

Re: [Qemu-devel] [PATCH 1/2] virtio: Add backend feature testing functionnality

[Maxime Coquelin]

On 09/09/2016 01:20 PM, Cornelia Huck wrote:
> On Fri, 9 Sep 2016 14:02:17 +0300
> Marcel Apfelbaum <marcel@redhat.com> wrote:
>
>> I was thinking to keep the same function proposed by Maxime and change it to negate things:
>>
>> /*
>>   * A missing feature before all negotiations finished will still be missing at the end.
>>   */
>> bool virtio_test_backend_unsupported_feature(VirtIODevice *vdev,
>>                                               unsigned int fbit, Error **errp)
>> {
>>      VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
>>      uint64_t feature;
>>
>>      virtio_add_feature(&feature, fbit);
>>
>>      assert(k->get_features != NULL);
>>      feature = k->get_features(vdev, feature, errp);
>>
>>      return !virtio_has_feature(feature, fbit);
>> }
>>
>> We only check if the feature was not there from the start.
>
> I think you'll still end up with the same problem (overindicating
> unsupportedness), as you start out with an otherwise empty feature
> set, causing features with dependencies to be removed. I fear that
> anything starting with an incomplete feature list will have that
> problem.
>
> Maybe it would be better to limit this to the one bit we currently want
> to test (VERSION_1)? We know the semantics of that one. Less general,
> but also less headaches.

Yes, that could be the solution.
I agree that making it too generic might create confusion
with some features.

Thanks,
Maxime

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

[Maxime Coquelin]

On 09/09/2016 01:20 PM, Cornelia Huck wrote:
> On Fri, 9 Sep 2016 14:04:55 +0300
> Marcel Apfelbaum <marcel@redhat.com> wrote:
>
>> On 09/09/2016 01:40 PM, Cornelia Huck wrote:
>>> On Fri,  9 Sep 2016 12:14:32 +0200
>>> Maxime Coquelin <maxime.coquelin@redhat.com> 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: 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(+)
>>>
>>> Note that 11380b361 ("virtio: handle non-virtio-1-capable backend for
>>> ccw") fixes this issue for ccw via the introduction of a
>>> ->post_plugged() callback. Unfortunately, we did not find a good way to
>>> make it work for pci back then.
>>
>> It seems that for ccw is enough to rewind dev->rev_max,
>> sadly for pci we need to rewind a lot of settings/resources.
>
> Yes, that what I meant with 'more flexibility for ccw'.
Maybe we could replace post_plugged with a pre_plugged approach?

In ->pre_plugged(), cww and pci would specify which features it can
support using virtio_add_feature().
Then we could call get_features() before ->device_plugged().

Doing this, both ccw and pci would have the needed information without
having to rewind any settings.

Does that make sense?

But for now, I think it would be better to merge something in the spirit
of this series (taking into account to remarks).
Indeed, I think we want this fixed in stable, but the above proposal
would be too huge for stable.

Thanks,
Maxime

Re: [Qemu-devel] [PATCH 1/2] virtio: Add backend feature testing functionnality

[Marcel Apfelbaum]

On 09/09/2016 02:36 PM, Maxime Coquelin wrote:
>
>
> On 09/09/2016 01:20 PM, Cornelia Huck wrote:
>> On Fri, 9 Sep 2016 14:02:17 +0300
>> Marcel Apfelbaum <marcel@redhat.com> wrote:
>>
>>> I was thinking to keep the same function proposed by Maxime and change it to negate things:
>>>
>>> /*
>>>   * A missing feature before all negotiations finished will still be missing at the end.
>>>   */
>>> bool virtio_test_backend_unsupported_feature(VirtIODevice *vdev,
>>>                                               unsigned int fbit, Error **errp)
>>> {
>>>      VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
>>>      uint64_t feature;
>>>
>>>      virtio_add_feature(&feature, fbit);
>>>
>>>      assert(k->get_features != NULL);
>>>      feature = k->get_features(vdev, feature, errp);
>>>
>>>      return !virtio_has_feature(feature, fbit);
>>> }
>>>
>>> We only check if the feature was not there from the start.
>>
>> I think you'll still end up with the same problem (overindicating
>> unsupportedness), as you start out with an otherwise empty feature
>> set, causing features with dependencies to be removed. I fear that
>> anything starting with an incomplete feature list will have that
>> problem.
>>
>> Maybe it would be better to limit this to the one bit we currently want
>> to test (VERSION_1)? We know the semantics of that one. Less general,
>> but also less headaches.
>
> Yes, that could be the solution.
> I agree that making it too generic might create confusion
> with some features.
>

Sounds good to me. Let's go with it and we'll re-think it if
we'll find other feature negotiation issues later.

Thanks,
Marcel


> Thanks,
> Maxime
>

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

[Cornelia Huck]

On Fri, 9 Sep 2016 13:44:35 +0200
Maxime Coquelin <maxime.coquelin@redhat.com> wrote:

> On 09/09/2016 01:20 PM, Cornelia Huck wrote:
> > On Fri, 9 Sep 2016 14:04:55 +0300
> > Marcel Apfelbaum <marcel@redhat.com> wrote:
> >
> >> On 09/09/2016 01:40 PM, Cornelia Huck wrote:
> >>> On Fri,  9 Sep 2016 12:14:32 +0200
> >>> Maxime Coquelin <maxime.coquelin@redhat.com> 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: 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(+)
> >>>
> >>> Note that 11380b361 ("virtio: handle non-virtio-1-capable backend for
> >>> ccw") fixes this issue for ccw via the introduction of a
> >>> ->post_plugged() callback. Unfortunately, we did not find a good way to
> >>> make it work for pci back then.
> >>
> >> It seems that for ccw is enough to rewind dev->rev_max,
> >> sadly for pci we need to rewind a lot of settings/resources.
> >
> > Yes, that what I meant with 'more flexibility for ccw'.
> Maybe we could replace post_plugged with a pre_plugged approach?
> 
> In ->pre_plugged(), cww and pci would specify which features it can
> support using virtio_add_feature().
> Then we could call get_features() before ->device_plugged().

I think that would work for ccw (haven't looked at pci).

> 
> Doing this, both ccw and pci would have the needed information without
> having to rewind any settings.
> 
> Does that make sense?
> 
> But for now, I think it would be better to merge something in the spirit
> of this series (taking into account to remarks).
> Indeed, I think we want this fixed in stable, but the above proposal
> would be too huge for stable.

A 'just check for VERSION_1' approach would probably be best for stable.

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

[Maxime Coquelin]

On 09/09/2016 01:49 PM, Cornelia Huck wrote:
> On Fri, 9 Sep 2016 13:44:35 +0200
> Maxime Coquelin <maxime.coquelin@redhat.com> wrote:
>
>> On 09/09/2016 01:20 PM, Cornelia Huck wrote:
>>> On Fri, 9 Sep 2016 14:04:55 +0300
>>> Marcel Apfelbaum <marcel@redhat.com> wrote:
>>>
>>>> On 09/09/2016 01:40 PM, Cornelia Huck wrote:
>>>>> On Fri,  9 Sep 2016 12:14:32 +0200
>>>>> Maxime Coquelin <maxime.coquelin@redhat.com> 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: 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(+)
>>>>>
>>>>> Note that 11380b361 ("virtio: handle non-virtio-1-capable backend for
>>>>> ccw") fixes this issue for ccw via the introduction of a
>>>>> ->post_plugged() callback. Unfortunately, we did not find a good way to
>>>>> make it work for pci back then.
>>>>
>>>> It seems that for ccw is enough to rewind dev->rev_max,
>>>> sadly for pci we need to rewind a lot of settings/resources.
>>>
>>> Yes, that what I meant with 'more flexibility for ccw'.
>> Maybe we could replace post_plugged with a pre_plugged approach?
>>
>> In ->pre_plugged(), cww and pci would specify which features it can
>> support using virtio_add_feature().
>> Then we could call get_features() before ->device_plugged().
>
> I think that would work for ccw (haven't looked at pci).
Good, once quick fix accepted, I'll try this solution.

>
>>
>> Doing this, both ccw and pci would have the needed information without
>> having to rewind any settings.
>>
>> Does that make sense?
>>
>> But for now, I think it would be better to merge something in the spirit
>> of this series (taking into account to remarks).
>> Indeed, I think we want this fixed in stable, but the above proposal
>> would be too huge for stable.
>
> A 'just check for VERSION_1' approach would probably be best for stable.
>
Ok, thanks.
I will send a v2 replacing the generic function with a VERISON_1
specfic:
bool virtio_test_backend_virtio_1(VirtIODevice *vdev, Error **errp);

Maxime

[Qemu-devel] [RFC] virtio-bus: Plug devices after features are negotiated

[Maxime Coquelin]

Currently, devices are plugged before features are negotiated.
If the backend doesn't support VIRTIO_F_VERSION_1, the transport
need to rewind some settings.

This is the case for both PCI and CCW.
For CCW, a post_plugged callback had been introduced, where
max_rev field is just updated if VIRTIO_F_VERSION_1 is not
supported by the backend.
For PCI, implementing the post_plugged would be much more
complicated, so the current fix consists in checking whether
the backend supports VIRTIO_F_VERSION_1 in the backend.

This patch propose to replace existing solutions with a common
approach by negociating features before ->device_plugged() is
called. A pre_plugged callback is introduced so that the
transports can set their supported features.

Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
Cc: Marcel Apfelbaum <marcel@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
Cornelia,

This is what I proposed earlier today.

I have tested it successfully with PCI.
Does it sound good to you? Would you have a try with CCW?

Note that this is based on my earlier series which fixes PCI.

Thanks,
Maxime
---
 hw/s390x/virtio-ccw.c          | 30 +++++++++++++++---------------
 hw/virtio/virtio-bus.c         |  9 +++++----
 hw/virtio/virtio-pci.c         | 18 ++++++++++++++----
 include/hw/virtio/virtio-bus.h | 10 +++++-----
 4 files changed, 39 insertions(+), 28 deletions(-)

diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index a554a24..e10a88d 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -1261,6 +1261,16 @@ static int virtio_ccw_load_config(DeviceState *d, QEMUFile *f)
     return 0;
 }
 
+static void virtio_ccw_pre_plugged(DeviceState *d, Error **errp)
+{
+   VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
+   VirtIODevice *vdev = virtio_bus_get_device(&dev->bus);
+
+    if (dev->max_rev >= 1) {
+        virtio_add_feature(&vdev->host_features, VIRTIO_F_VERSION_1);
+    }
+}
+
 /* This is called by virtio-bus just after the device is plugged. */
 static void virtio_ccw_device_plugged(DeviceState *d, Error **errp)
 {
@@ -1270,6 +1280,10 @@ static void virtio_ccw_device_plugged(DeviceState *d, Error **errp)
     SubchDev *sch = ccw_dev->sch;
     int n = virtio_get_num_queues(vdev);
 
+    if (!virtio_has_feature(vdev->host_features, VIRTIO_F_VERSION_1)) {
+        dev->max_rev = 0;
+    }
+
     if (virtio_get_num_queues(vdev) > VIRTIO_CCW_QUEUE_MAX) {
         error_setg(errp, "The number of virtqueues %d "
                    "exceeds ccw limit %d", n,
@@ -1283,25 +1297,11 @@ static void virtio_ccw_device_plugged(DeviceState *d, Error **errp)
 
     sch->id.cu_model = virtio_bus_get_vdev_id(&dev->bus);
 
-    if (dev->max_rev >= 1) {
-        virtio_add_feature(&vdev->host_features, VIRTIO_F_VERSION_1);
-    }
 
     css_generate_sch_crws(sch->cssid, sch->ssid, sch->schid,
                           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)) {
-        /* A backend didn't support modern virtio. */
-       dev->max_rev = 0;
-   }
-}
-
 static void virtio_ccw_device_unplugged(DeviceState *d)
 {
     VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
@@ -1593,8 +1593,8 @@ static void virtio_ccw_bus_class_init(ObjectClass *klass, void *data)
     k->load_queue = virtio_ccw_load_queue;
     k->save_config = virtio_ccw_save_config;
     k->load_config = virtio_ccw_load_config;
+    k->pre_plugged = virtio_ccw_pre_plugged;
     k->device_plugged = virtio_ccw_device_plugged;
-    k->post_plugged = virtio_ccw_post_plugged;
     k->device_unplugged = virtio_ccw_device_unplugged;
     k->ioeventfd_started = virtio_ccw_ioeventfd_started;
     k->ioeventfd_set_started = virtio_ccw_ioeventfd_set_started;
diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
index a85b7c8..9d2111f 100644
--- a/hw/virtio/virtio-bus.c
+++ b/hw/virtio/virtio-bus.c
@@ -49,16 +49,17 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp)
 
     DPRINTF("%s: plug device.\n", qbus->name);
 
-    if (klass->device_plugged != NULL) {
-        klass->device_plugged(qbus->parent, errp);
+    if (klass->pre_plugged != NULL) {
+        klass->pre_plugged(qbus->parent, errp);
     }
 
     /* Get the features of the plugged device. */
     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);
+
+    if (klass->device_plugged != NULL) {
+        klass->device_plugged(qbus->parent, errp);
     }
 }
 
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 9e88d7b..07e6c60 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1569,6 +1569,18 @@ static void virtio_pci_modern_io_region_unmap(VirtIOPCIProxy *proxy,
                                 &region->mr);
 }
 
+static void virtio_pci_pre_plugged(DeviceState *d, Error **errp)
+{
+    VirtIOPCIProxy *proxy = VIRTIO_PCI(d);
+    VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
+
+    if (virtio_pci_modern(proxy)) {
+        virtio_add_feature(&vdev->host_features, VIRTIO_F_VERSION_1);
+    }
+
+    virtio_add_feature(&vdev->host_features, VIRTIO_F_BAD_FEATURE);
+}
+
 /* This is called by virtio-bus just after the device is plugged. */
 static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
 {
@@ -1585,7 +1597,7 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
      * Virtio capabilities present without
      * VIRTIO_F_VERSION_1 confuses guests
      */
-    if (!virtio_test_backend_virtio_1(vdev, errp)) {
+    if (!virtio_has_feature(vdev->host_features, VIRTIO_F_VERSION_1)) {
         virtio_pci_disable_modern(proxy);
     }
 
@@ -1637,7 +1649,6 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
 
         struct virtio_pci_cfg_cap *cfg_mask;
 
-        virtio_add_feature(&vdev->host_features, VIRTIO_F_VERSION_1);
         virtio_pci_modern_regions_init(proxy);
 
         virtio_pci_modern_mem_region_map(proxy, &proxy->common, &cap);
@@ -1702,8 +1713,6 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
     if (!kvm_has_many_ioeventfds()) {
         proxy->flags &= ~VIRTIO_PCI_FLAG_USE_IOEVENTFD;
     }
-
-    virtio_add_feature(&vdev->host_features, VIRTIO_F_BAD_FEATURE);
 }
 
 static void virtio_pci_device_unplugged(DeviceState *d)
@@ -2459,6 +2468,7 @@ static void virtio_pci_bus_class_init(ObjectClass *klass, void *data)
     k->query_guest_notifiers = virtio_pci_query_guest_notifiers;
     k->set_guest_notifiers = virtio_pci_set_guest_notifiers;
     k->vmstate_change = virtio_pci_vmstate_change;
+    k->pre_plugged = virtio_pci_pre_plugged;
     k->device_plugged = virtio_pci_device_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 f3e5ef3..24caa0a 100644
--- a/include/hw/virtio/virtio-bus.h
+++ b/include/hw/virtio/virtio-bus.h
@@ -54,16 +54,16 @@ typedef struct VirtioBusClass {
     int (*set_guest_notifiers)(DeviceState *d, int nvqs, bool assign);
     void (*vmstate_change)(DeviceState *d, bool running);
     /*
+     * Expose the features the transport layer supports before
+     * the negotiation takes place.
+     */
+    void (*pre_plugged)(DeviceState *d, Error **errp);
+    /*
      * transport independent init function.
      * This is called by virtio-bus just after the device is plugged.
      */
     void (*device_plugged)(DeviceState *d, Error **errp);
     /*
-     * Re-evaluate setup after feature bits have been validated
-     * by the device backend.
-     */
-    void (*post_plugged)(DeviceState *d, Error **errp);
-    /*
      * transport independent exit function.
      * This is called by virtio-bus just before the device is unplugged.
      */
-- 
2.7.4

Re: [Qemu-devel] [RFC] virtio-bus: Plug devices after features are negotiated

[Michael S. Tsirkin]

On Fri, Sep 09, 2016 at 04:14:59PM +0200, Maxime Coquelin wrote:
> Currently, devices are plugged before features are negotiated.
> If the backend doesn't support VIRTIO_F_VERSION_1, the transport
> need to rewind some settings.
> 
> This is the case for both PCI and CCW.
> For CCW, a post_plugged callback had been introduced, where
> max_rev field is just updated if VIRTIO_F_VERSION_1 is not
> supported by the backend.
> For PCI, implementing the post_plugged would be much more
> complicated, so the current fix consists in checking whether
> the backend supports VIRTIO_F_VERSION_1 in the backend.
> 
> This patch propose to replace existing solutions with a common
> approach by negociating features before ->device_plugged() is
> called. A pre_plugged callback is introduced so that the
> transports can set their supported features.
> 
> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> Cc: Marcel Apfelbaum <marcel@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>


Acked-by: Michael S. Tsirkin <mst@redhat.com>

Looks good - can you pls test and post a non-RFC?

> ---
> Cornelia,
> 
> This is what I proposed earlier today.
> 
> I have tested it successfully with PCI.
> Does it sound good to you? Would you have a try with CCW?
> 
> Note that this is based on my earlier series which fixes PCI.
> 
> Thanks,
> Maxime
> ---
>  hw/s390x/virtio-ccw.c          | 30 +++++++++++++++---------------
>  hw/virtio/virtio-bus.c         |  9 +++++----
>  hw/virtio/virtio-pci.c         | 18 ++++++++++++++----
>  include/hw/virtio/virtio-bus.h | 10 +++++-----
>  4 files changed, 39 insertions(+), 28 deletions(-)
> 
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index a554a24..e10a88d 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -1261,6 +1261,16 @@ static int virtio_ccw_load_config(DeviceState *d, QEMUFile *f)
>      return 0;
>  }
>  
> +static void virtio_ccw_pre_plugged(DeviceState *d, Error **errp)
> +{
> +   VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
> +   VirtIODevice *vdev = virtio_bus_get_device(&dev->bus);
> +
> +    if (dev->max_rev >= 1) {
> +        virtio_add_feature(&vdev->host_features, VIRTIO_F_VERSION_1);
> +    }
> +}
> +
>  /* This is called by virtio-bus just after the device is plugged. */
>  static void virtio_ccw_device_plugged(DeviceState *d, Error **errp)
>  {
> @@ -1270,6 +1280,10 @@ static void virtio_ccw_device_plugged(DeviceState *d, Error **errp)
>      SubchDev *sch = ccw_dev->sch;
>      int n = virtio_get_num_queues(vdev);
>  
> +    if (!virtio_has_feature(vdev->host_features, VIRTIO_F_VERSION_1)) {
> +        dev->max_rev = 0;
> +    }
> +
>      if (virtio_get_num_queues(vdev) > VIRTIO_CCW_QUEUE_MAX) {
>          error_setg(errp, "The number of virtqueues %d "
>                     "exceeds ccw limit %d", n,
> @@ -1283,25 +1297,11 @@ static void virtio_ccw_device_plugged(DeviceState *d, Error **errp)
>  
>      sch->id.cu_model = virtio_bus_get_vdev_id(&dev->bus);
>  
> -    if (dev->max_rev >= 1) {
> -        virtio_add_feature(&vdev->host_features, VIRTIO_F_VERSION_1);
> -    }
>  
>      css_generate_sch_crws(sch->cssid, sch->ssid, sch->schid,
>                            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)) {
> -        /* A backend didn't support modern virtio. */
> -       dev->max_rev = 0;
> -   }
> -}
> -
>  static void virtio_ccw_device_unplugged(DeviceState *d)
>  {
>      VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
> @@ -1593,8 +1593,8 @@ static void virtio_ccw_bus_class_init(ObjectClass *klass, void *data)
>      k->load_queue = virtio_ccw_load_queue;
>      k->save_config = virtio_ccw_save_config;
>      k->load_config = virtio_ccw_load_config;
> +    k->pre_plugged = virtio_ccw_pre_plugged;
>      k->device_plugged = virtio_ccw_device_plugged;
> -    k->post_plugged = virtio_ccw_post_plugged;
>      k->device_unplugged = virtio_ccw_device_unplugged;
>      k->ioeventfd_started = virtio_ccw_ioeventfd_started;
>      k->ioeventfd_set_started = virtio_ccw_ioeventfd_set_started;
> diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
> index a85b7c8..9d2111f 100644
> --- a/hw/virtio/virtio-bus.c
> +++ b/hw/virtio/virtio-bus.c
> @@ -49,16 +49,17 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp)
>  
>      DPRINTF("%s: plug device.\n", qbus->name);
>  
> -    if (klass->device_plugged != NULL) {
> -        klass->device_plugged(qbus->parent, errp);
> +    if (klass->pre_plugged != NULL) {
> +        klass->pre_plugged(qbus->parent, errp);
>      }
>  
>      /* Get the features of the plugged device. */
>      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);
> +
> +    if (klass->device_plugged != NULL) {
> +        klass->device_plugged(qbus->parent, errp);
>      }
>  }
>  
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 9e88d7b..07e6c60 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1569,6 +1569,18 @@ static void virtio_pci_modern_io_region_unmap(VirtIOPCIProxy *proxy,
>                                  &region->mr);
>  }
>  
> +static void virtio_pci_pre_plugged(DeviceState *d, Error **errp)
> +{
> +    VirtIOPCIProxy *proxy = VIRTIO_PCI(d);
> +    VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
> +
> +    if (virtio_pci_modern(proxy)) {
> +        virtio_add_feature(&vdev->host_features, VIRTIO_F_VERSION_1);
> +    }
> +
> +    virtio_add_feature(&vdev->host_features, VIRTIO_F_BAD_FEATURE);
> +}
> +
>  /* This is called by virtio-bus just after the device is plugged. */
>  static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
>  {
> @@ -1585,7 +1597,7 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
>       * Virtio capabilities present without
>       * VIRTIO_F_VERSION_1 confuses guests
>       */
> -    if (!virtio_test_backend_virtio_1(vdev, errp)) {
> +    if (!virtio_has_feature(vdev->host_features, VIRTIO_F_VERSION_1)) {
>          virtio_pci_disable_modern(proxy);
>      }
>  
> @@ -1637,7 +1649,6 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
>  
>          struct virtio_pci_cfg_cap *cfg_mask;
>  
> -        virtio_add_feature(&vdev->host_features, VIRTIO_F_VERSION_1);
>          virtio_pci_modern_regions_init(proxy);
>  
>          virtio_pci_modern_mem_region_map(proxy, &proxy->common, &cap);
> @@ -1702,8 +1713,6 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
>      if (!kvm_has_many_ioeventfds()) {
>          proxy->flags &= ~VIRTIO_PCI_FLAG_USE_IOEVENTFD;
>      }
> -
> -    virtio_add_feature(&vdev->host_features, VIRTIO_F_BAD_FEATURE);
>  }
>  
>  static void virtio_pci_device_unplugged(DeviceState *d)
> @@ -2459,6 +2468,7 @@ static void virtio_pci_bus_class_init(ObjectClass *klass, void *data)
>      k->query_guest_notifiers = virtio_pci_query_guest_notifiers;
>      k->set_guest_notifiers = virtio_pci_set_guest_notifiers;
>      k->vmstate_change = virtio_pci_vmstate_change;
> +    k->pre_plugged = virtio_pci_pre_plugged;
>      k->device_plugged = virtio_pci_device_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 f3e5ef3..24caa0a 100644
> --- a/include/hw/virtio/virtio-bus.h
> +++ b/include/hw/virtio/virtio-bus.h
> @@ -54,16 +54,16 @@ typedef struct VirtioBusClass {
>      int (*set_guest_notifiers)(DeviceState *d, int nvqs, bool assign);
>      void (*vmstate_change)(DeviceState *d, bool running);
>      /*
> +     * Expose the features the transport layer supports before
> +     * the negotiation takes place.
> +     */
> +    void (*pre_plugged)(DeviceState *d, Error **errp);
> +    /*
>       * transport independent init function.
>       * This is called by virtio-bus just after the device is plugged.
>       */
>      void (*device_plugged)(DeviceState *d, Error **errp);
>      /*
> -     * Re-evaluate setup after feature bits have been validated
> -     * by the device backend.
> -     */
> -    void (*post_plugged)(DeviceState *d, Error **errp);
> -    /*
>       * transport independent exit function.
>       * This is called by virtio-bus just before the device is unplugged.
>       */
> -- 
> 2.7.4

Re: [Qemu-devel] [RFC] virtio-bus: Plug devices after features are negotiated

[Maxime Coquelin]

On 09/09/2016 05:39 PM, Michael S. Tsirkin wrote:
> On Fri, Sep 09, 2016 at 04:14:59PM +0200, Maxime Coquelin wrote:
>> > Currently, devices are plugged before features are negotiated.
>> > If the backend doesn't support VIRTIO_F_VERSION_1, the transport
>> > need to rewind some settings.
>> >
>> > This is the case for both PCI and CCW.
>> > For CCW, a post_plugged callback had been introduced, where
>> > max_rev field is just updated if VIRTIO_F_VERSION_1 is not
>> > supported by the backend.
>> > For PCI, implementing the post_plugged would be much more
>> > complicated, so the current fix consists in checking whether
>> > the backend supports VIRTIO_F_VERSION_1 in the backend.
>> >
>> > This patch propose to replace existing solutions with a common
>> > approach by negociating features before ->device_plugged() is
>> > called. A pre_plugged callback is introduced so that the
>> > transports can set their supported features.
>> >
>> > Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
>> > Cc: Marcel Apfelbaum <marcel@redhat.com>
>> > Cc: Michael S. Tsirkin <mst@redhat.com>
>> > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
>
> Looks good - can you pls test and post a non-RFC?
>

Do you mean having this also in stable, or keeping
it based on top of former pci-only patch?

I already tested it for PCI, but cannot test it for CCW.

Thanks,
Maxime

Re: [Qemu-devel] [RFC] virtio-bus: Plug devices after features are negotiated

[Michael S. Tsirkin]

On Fri, Sep 09, 2016 at 06:16:27PM +0200, Maxime Coquelin wrote:
> 
> 
> On 09/09/2016 05:39 PM, Michael S. Tsirkin wrote:
> > On Fri, Sep 09, 2016 at 04:14:59PM +0200, Maxime Coquelin wrote:
> > > > Currently, devices are plugged before features are negotiated.
> > > > If the backend doesn't support VIRTIO_F_VERSION_1, the transport
> > > > need to rewind some settings.
> > > >
> > > > This is the case for both PCI and CCW.
> > > > For CCW, a post_plugged callback had been introduced, where
> > > > max_rev field is just updated if VIRTIO_F_VERSION_1 is not
> > > > supported by the backend.
> > > > For PCI, implementing the post_plugged would be much more
> > > > complicated, so the current fix consists in checking whether
> > > > the backend supports VIRTIO_F_VERSION_1 in the backend.
> > > >
> > > > This patch propose to replace existing solutions with a common
> > > > approach by negociating features before ->device_plugged() is
> > > > called. A pre_plugged callback is introduced so that the
> > > > transports can set their supported features.
> > > >
> > > > Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> > > > Cc: Marcel Apfelbaum <marcel@redhat.com>
> > > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> > 
> > Acked-by: Michael S. Tsirkin <mst@redhat.com>
> > 
> > Looks good - can you pls test and post a non-RFC?
> > 
> 
> Do you mean having this also in stable, or keeping
> it based on top of former pci-only patch?
> 
> I already tested it for PCI, but cannot test it for CCW.
> 
> Thanks,
> Maxime

Once you repost, ask for help testing.

-- 
MST