Device initialization has an extra step in VIRTIO 1.0.  The FEATURES_OK
status bit is set to indicate that feature negotiation has completed.
The driver then reads the status register again to check that the device
agrees with the final features.

Implement this step as part of qvirtio_set_features() instead of
introducing a separate function.  This way all existing code works
without modifications.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/libqos/virtio.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/tests/libqos/virtio.c b/tests/libqos/virtio.c
index 2593996c98..57fa79373b 100644
--- a/tests/libqos/virtio.c
+++ b/tests/libqos/virtio.c
@@ -52,6 +52,19 @@ void qvirtio_set_features(QVirtioDevice *d, uint64_t features)
 {
     d->features = features;
     d->bus->set_features(d, features);
+
+    /*
+     * This could be a separate function for drivers that want to access
+     * configuration space before setting FEATURES_OK, but no existing users
+     * need that and it's less code for callers if this is done implicitly.
+    */
+    if (features & (1ull << VIRTIO_F_VERSION_1)) {
+        uint8_t status = d->bus->get_status(d) |
+                         VIRTIO_CONFIG_S_FEATURES_OK;
+
+        d->bus->set_status(d, status);
+        g_assert_cmphex(d->bus->get_status(d), ==, status);
+    }
 }
 
 QVirtQueue *qvirtqueue_setup(QVirtioDevice *d,
@@ -88,9 +101,10 @@ void qvirtio_set_driver(QVirtioDevice *d)
 
 void qvirtio_set_driver_ok(QVirtioDevice *d)
 {
-    d->bus->set_status(d, d->bus->get_status(d) | VIRTIO_CONFIG_S_DRIVER_OK);
-    g_assert_cmphex(d->bus->get_status(d), ==, VIRTIO_CONFIG_S_DRIVER_OK |
-                    VIRTIO_CONFIG_S_DRIVER | VIRTIO_CONFIG_S_ACKNOWLEDGE);
+    uint8_t status = d->bus->get_status(d) | VIRTIO_CONFIG_S_DRIVER_OK;
+
+    d->bus->set_status(d, status);
+    g_assert_cmphex(d->bus->get_status(d), ==, status);
 }
 
 void qvirtio_wait_queue_isr(QTestState *qts, QVirtioDevice *d,
-- 
2.21.0

On 19/10/2019 08.38, Stefan Hajnoczi wrote:
> Device initialization has an extra step in VIRTIO 1.0.  The FEATURES_OK
> status bit is set to indicate that feature negotiation has completed.
> The driver then reads the status register again to check that the device
> agrees with the final features.
> 
> Implement this step as part of qvirtio_set_features() instead of
> introducing a separate function.  This way all existing code works
> without modifications.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  tests/libqos/virtio.c | 20 +++++++++++++++++---
>  1 file changed, 17 insertions(+), 3 deletions(-)
[...]
> @@ -88,9 +101,10 @@ void qvirtio_set_driver(QVirtioDevice *d)
>  
>  void qvirtio_set_driver_ok(QVirtioDevice *d)
>  {
> -    d->bus->set_status(d, d->bus->get_status(d) | VIRTIO_CONFIG_S_DRIVER_OK);
> -    g_assert_cmphex(d->bus->get_status(d), ==, VIRTIO_CONFIG_S_DRIVER_OK |
> -                    VIRTIO_CONFIG_S_DRIVER | VIRTIO_CONFIG_S_ACKNOWLEDGE);
> +    uint8_t status = d->bus->get_status(d) | VIRTIO_CONFIG_S_DRIVER_OK;
> +
> +    d->bus->set_status(d, status);
> +    g_assert_cmphex(d->bus->get_status(d), ==, status);
>  }

The changes to qvirtio_set_driver_ok() are not mentioned in the patch
description. Please either mention them there, or move this to a
separate patch.

 Thanks,
  Thomas

On Mon, Oct 21, 2019 at 02:23:15PM +0200, Thomas Huth wrote:
> On 19/10/2019 08.38, Stefan Hajnoczi wrote:
> > Device initialization has an extra step in VIRTIO 1.0.  The FEATURES_OK
> > status bit is set to indicate that feature negotiation has completed.
> > The driver then reads the status register again to check that the device
> > agrees with the final features.
> > 
> > Implement this step as part of qvirtio_set_features() instead of
> > introducing a separate function.  This way all existing code works
> > without modifications.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  tests/libqos/virtio.c | 20 +++++++++++++++++---
> >  1 file changed, 17 insertions(+), 3 deletions(-)
> [...]
> > @@ -88,9 +101,10 @@ void qvirtio_set_driver(QVirtioDevice *d)
> >  
> >  void qvirtio_set_driver_ok(QVirtioDevice *d)
> >  {
> > -    d->bus->set_status(d, d->bus->get_status(d) | VIRTIO_CONFIG_S_DRIVER_OK);
> > -    g_assert_cmphex(d->bus->get_status(d), ==, VIRTIO_CONFIG_S_DRIVER_OK |
> > -                    VIRTIO_CONFIG_S_DRIVER | VIRTIO_CONFIG_S_ACKNOWLEDGE);
> > +    uint8_t status = d->bus->get_status(d) | VIRTIO_CONFIG_S_DRIVER_OK;
> > +
> > +    d->bus->set_status(d, status);
> > +    g_assert_cmphex(d->bus->get_status(d), ==, status);
> >  }
> 
> The changes to qvirtio_set_driver_ok() are not mentioned in the patch
> description. Please either mention them there, or move this to a
> separate patch.

Will fix in v4.

We can no longer hardcode the status register value for DRIVER_OK since
it now depends on whether VIRTIO 1.0 is used or not.

Here is an alternative that is smaller and makes the VIRTIO 1.0
dependency obvious:

  -    g_assert_cmphex(d->bus->get_status(d), ==, VIRTIO_CONFIG_S_DRIVER_OK |
  -                    VIRTIO_CONFIG_S_DRIVER | VIRTIO_CONFIG_S_ACKNOWLEDGE);
  +    g_assert_cmphex(d->bus->get_status(d), ==, VIRTIO_CONFIG_S_DRIVER_OK |
  +                    VIRTIO_CONFIG_S_DRIVER | VIRTIO_CONFIG_S_ACKNOWLEDGE |
  +                    (d->features & (1ull << VIRTIO_F_VERSION_1) ?
  +                     VIRTIO_CONFIG_S_FEATURES_OK : 0));

Stefan