To process pending requests for driver status 'VIRTIO_CONFIG_S_DRIVER_OK',
 virtio-rng 'set_status' calls 'is_guest_ready' function. This checks if 
 virtqueue is ready and status is set to 'VIRTIO_CONFIG_S_DRIVER_OK'. 

 This call is made before new status is updated in VirtIODevice parent object 
 and calling function uses old status value.

Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
---
 hw/virtio/virtio.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index d4e4d98b59..37dc59a686 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1154,10 +1154,11 @@ int virtio_set_status(VirtIODevice *vdev, uint8_t val)
             }
         }
     }
+    vdev->status = val;
+
     if (k->set_status) {
         k->set_status(vdev, val);
     }
-    vdev->status = val;
     return 0;
 }
 
-- 
2.14.3

On Wed, Jun 27, 2018 at 01:52:48PM +0530, Pankaj Gupta wrote:
>  To process pending requests for driver status 'VIRTIO_CONFIG_S_DRIVER_OK',
>  virtio-rng 'set_status' calls 'is_guest_ready' function. This checks if 
>  virtqueue is ready and status is set to 'VIRTIO_CONFIG_S_DRIVER_OK'. 
> 
>  This call is made before new status is updated in VirtIODevice parent object 
>  and calling function uses old status value.
> 
> Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
> ---
>  hw/virtio/virtio.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index d4e4d98b59..37dc59a686 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -1154,10 +1154,11 @@ int virtio_set_status(VirtIODevice *vdev, uint8_t val)
>              }
>          }
>      }
> +    vdev->status = val;
> +
>      if (k->set_status) {
>          k->set_status(vdev, val);
>      }
> -    vdev->status = val;

This breaks existing ->set_status() callbacks that rely on vdev->status
containing the old value.  Have you audited them?

For example, virtio_net_set_status -> virtio_net_vnet_endian_status uses
vdev->status!

It may be easier to modify virtio-rng.c so that the old status value
isn't used.

Stefan

> 
> On Wed, Jun 27, 2018 at 01:52:48PM +0530, Pankaj Gupta wrote:
> >  To process pending requests for driver status 'VIRTIO_CONFIG_S_DRIVER_OK',
> >  virtio-rng 'set_status' calls 'is_guest_ready' function. This checks if
> >  virtqueue is ready and status is set to 'VIRTIO_CONFIG_S_DRIVER_OK'.
> > 
> >  This call is made before new status is updated in VirtIODevice parent
> >  object
> >  and calling function uses old status value.
> > 
> > Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
> > ---
> >  hw/virtio/virtio.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index d4e4d98b59..37dc59a686 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -1154,10 +1154,11 @@ int virtio_set_status(VirtIODevice *vdev, uint8_t
> > val)
> >              }
> >          }
> >      }
> > +    vdev->status = val;
> > +
> >      if (k->set_status) {
> >          k->set_status(vdev, val);
> >      }
> > -    vdev->status = val;
> 
> This breaks existing ->set_status() callbacks that rely on vdev->status
> containing the old value.  Have you audited them?

I did not audit them all. I thought that's the way it should be base object
first get updated. But you are right, it can break things where old status is used.

> 
> For example, virtio_net_set_status -> virtio_net_vnet_endian_status uses
> vdev->status!
> 
> It may be easier to modify virtio-rng.c so that the old status value
> isn't used.

Just updating status only for virtio_rng_set_status looks okay? This will avoid
multiple copies of status as different functions are using it.

I will send a V2.

+static void virtio_rng_set_status(VirtIODevice *vdev, uint8_t status)
+{
+    VirtIORNG *vrng = VIRTIO_RNG(vdev);
+
+    if (!vdev->vm_running) {
+        return;
+    }
+    vdev->status = status;
+
+    /* Something changed, try to process buffers */
+    virtio_rng_process(vrng);
+}
+


Thanks,
Pankaj

> 
> Stefan
>