| Message ID | 20180627082248.14921-3-pagupta@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | virtio-rng: fix old guest kernel hang | expand |
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 >
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; }
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(-)