| Message ID | 1471296770-7984-1-git-send-email-mst@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Aug 15, 2016 at 11:32 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > virtio spec says that devices must not touch VQs > before DRIVER_OK is set. > > Additionally, balloon must not touch stats VQ > unless the stats feature bit has been negotiated. > > Cc: Ladi Prosek <lprosek@redhat.com> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> Looks good! Do you want me to add this to the patch that introduces virtio_balloon_vmstate_cb or will you apply it on top of it? Thanks, Ladi
On Tue, Aug 16, 2016 at 12:32:55AM +0300, Michael S. Tsirkin wrote: > virtio spec says that devices must not touch VQs > before DRIVER_OK is set. > > Additionally, balloon must not touch stats VQ > unless the stats feature bit has been negotiated. > > Cc: Ladi Prosek <lprosek@redhat.com> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > hw/virtio/virtio-balloon.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c > index 65457e9..7189260 100644 > --- a/hw/virtio/virtio-balloon.c > +++ b/hw/virtio/virtio-balloon.c > @@ -427,6 +427,7 @@ static void virtio_balloon_vmstate_cb(void *opaque, int running, > RunState state) > { > VirtIOBalloon *s = opaque; > + VirtIODevice *vdev = VIRTIO_DEVICE(s); > > if (!running) { > /* put the stats element back if the VM is not running */ > @@ -436,7 +437,8 @@ static void virtio_balloon_vmstate_cb(void *opaque, int running, > s->stats_vq_elem = NULL; > } > > - } else { > + } else if (balloon_stats_supported(s) && > + (vdev->status & VIRTIO_CONFIG_S_DRIVER_OK)) { > /* poll stats queue for the element we may have discarded > * when the VM was stopped */ > virtio_balloon_receive_stats(VIRTIO_DEVICE(s), s->svq); Another suspect case is when the feature bit was negotiated, VIRTIO_CONFIG_S_DRIVER_OK is set, but the virtqueue PFN wasn't set up. In that case virtqueue_pop() will access junk values. I'm not sure if anything terrible can happen but a check that vq->vring.desc != 0 would be nice - just like we do in virtio_queue_notify_vq(). Can you add a virtio_queue_get_addr() call to see if the PFN has been set? Stefan
On Tue, Aug 16, 2016 at 11:30:16AM +0100, Stefan Hajnoczi wrote: > On Tue, Aug 16, 2016 at 12:32:55AM +0300, Michael S. Tsirkin wrote: > > virtio spec says that devices must not touch VQs > > before DRIVER_OK is set. > > > > Additionally, balloon must not touch stats VQ > > unless the stats feature bit has been negotiated. > > > > Cc: Ladi Prosek <lprosek@redhat.com> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > --- > > hw/virtio/virtio-balloon.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c > > index 65457e9..7189260 100644 > > --- a/hw/virtio/virtio-balloon.c > > +++ b/hw/virtio/virtio-balloon.c > > @@ -427,6 +427,7 @@ static void virtio_balloon_vmstate_cb(void *opaque, int running, > > RunState state) > > { > > VirtIOBalloon *s = opaque; > > + VirtIODevice *vdev = VIRTIO_DEVICE(s); > > > > if (!running) { > > /* put the stats element back if the VM is not running */ > > @@ -436,7 +437,8 @@ static void virtio_balloon_vmstate_cb(void *opaque, int running, > > s->stats_vq_elem = NULL; > > } > > > > - } else { > > + } else if (balloon_stats_supported(s) && > > + (vdev->status & VIRTIO_CONFIG_S_DRIVER_OK)) { > > /* poll stats queue for the element we may have discarded > > * when the VM was stopped */ > > virtio_balloon_receive_stats(VIRTIO_DEVICE(s), s->svq); > > Another suspect case is when the feature bit was negotiated, > VIRTIO_CONFIG_S_DRIVER_OK is set, but the virtqueue PFN wasn't set up. > > In that case virtqueue_pop() will access junk values. > > I'm not sure if anything terrible can happen but a check that > vq->vring.desc != 0 would be nice - just like we do in > virtio_queue_notify_vq(). > > Can you add a virtio_queue_get_addr() call to see if the PFN has been > set? > > Stefan DRIVER_OK is set after VQs are initialized so I think that's enough.
diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c index 65457e9..7189260 100644 --- a/hw/virtio/virtio-balloon.c +++ b/hw/virtio/virtio-balloon.c @@ -427,6 +427,7 @@ static void virtio_balloon_vmstate_cb(void *opaque, int running, RunState state) { VirtIOBalloon *s = opaque; + VirtIODevice *vdev = VIRTIO_DEVICE(s); if (!running) { /* put the stats element back if the VM is not running */ @@ -436,7 +437,8 @@ static void virtio_balloon_vmstate_cb(void *opaque, int running, s->stats_vq_elem = NULL; } - } else { + } else if (balloon_stats_supported(s) && + (vdev->status & VIRTIO_CONFIG_S_DRIVER_OK)) { /* poll stats queue for the element we may have discarded * when the VM was stopped */ virtio_balloon_receive_stats(VIRTIO_DEVICE(s), s->svq);
virtio spec says that devices must not touch VQs before DRIVER_OK is set. Additionally, balloon must not touch stats VQ unless the stats feature bit has been negotiated. Cc: Ladi Prosek <lprosek@redhat.com> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- hw/virtio/virtio-balloon.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)