Re: [Qemu-devel] [PATCH 2/4] virtio-balloon: make stats virtqueue length 1

[Roman Kagan]

On Fri, Aug 19, 2016 at 02:44:10PM +0100, Stefan Hajnoczi wrote:
> On Thu, Aug 18, 2016 at 09:27:52PM +0300, Roman Kagan wrote:
> > The protocol for virtio-balloon stats virtqueue doesn't allow more than
> > one element in the virtqueue.
> > 
> > So, instead of trying to compensate for guest misbehavior if it sends
> > new data before the slot has been released by the host, just define the
> > stats virtqueue length to 1 initially and rely on the generic virtio
> > code to handle overflows.
> > 
> > Signed-off-by: Roman Kagan <address@hidden>
> > Cc: "Michael S. Tsirkin" <address@hidden>
> > Cc: Ladi Prosek <address@hidden>
> > ---
> >  hw/virtio/virtio-balloon.c | 9 +--------
> >  1 file changed, 1 insertion(+), 8 deletions(-)
> > 
> > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> > index 5af429a..0baf4b3 100644
> > --- a/hw/virtio/virtio-balloon.c
> > +++ b/hw/virtio/virtio-balloon.c
> > @@ -262,13 +262,6 @@ static void virtio_balloon_receive_stats(VirtIODevice 
> > *vdev, VirtQueue *vq)
> >          goto out;
> >      }
> >  
> > -    if (s->stats_vq_elem != NULL) {
> > -        /* This should never happen if the driver follows the spec. */
> > -        virtqueue_push(vq, s->stats_vq_elem, 0);
> > -        virtio_notify(vdev, vq);
> > -        g_free(s->stats_vq_elem);
> > -    }
> > -
> >      s->stats_vq_elem = elem;
> >  
> >      /* Initialize the stats to get rid of any stale values.  This is only
> > @@ -443,7 +436,7 @@ static void virtio_balloon_device_realize(DeviceState 
> > *dev, Error **errp)
> >  
> >      s->ivq = virtio_add_queue(vdev, 128, virtio_balloon_handle_output);
> >      s->dvq = virtio_add_queue(vdev, 128, virtio_balloon_handle_output);
> > -    s->svq = virtio_add_queue(vdev, 128, virtio_balloon_receive_stats);
> > +    s->svq = virtio_add_queue(vdev, 1, virtio_balloon_receive_stats);
> 
> This change breaks live migration compatibility.  The device is not
> allowed to change in existing machine types.

Ideed.

> I think Ladi's fix for this bug is the way to go.  It preserves
> migration compatibility.

Agreed.

Thanks,
Roman.