[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/2] virtio: Set status early in VirtIODevice pa
From: |
Pankaj Gupta |
Subject: |
Re: [Qemu-devel] [PATCH 2/2] virtio: Set status early in VirtIODevice parent object |
Date: |
Wed, 27 Jun 2018 07:06:45 -0400 (EDT) |
>
> 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 <address@hidden>
> > ---
> > 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
>