[Qemu-devel] [PATCH 0/3] virtio-balloon: stats vq fixes

[Qemu-devel] [PATCH 0/3] virtio-balloon: stats vq fixes

[Ladi Prosek]

This is another series that aims to fix issues with the balloon stats
queue, heavily inspired by previous patches posted by Stefan and Roman.

Stefan Hajnoczi (1):
  virtio: add virtqueue_rewind()

Ladi Prosek (2):
  virtio-balloon: discard virtqueue element on reset
  virtio-balloon: fix stats vq migration

The first patch addresses the "inuse leak" issue which was discovered
recently.

The virtqueue_rewind() implementation is reposted verbatim. However,
it is called from the set_status callback, same as in Roman's patch.
Doing it in balloon_stats_poll_cb (Stefan's patch) is potentially
problematic because the guest may push another buffer to the queue
before the timer fires, so we could still leak a virtqueue slot.
Also, we want to call the full-blown virtio_balloon_receive_stats and
not manually reconstruct only some pieces of the state. It is not
necessary to check balloon_stats_supported. Pushing buffers without
negotiating stats support would be a driver bug. The common codepath
does not check it either.

[Qemu-devel] [PATCH 1/3] virtio-balloon: discard virtqueue element on reset

[Ladi Prosek]

The one pending element is being freed but not discarded on device
reset, which causes svq->inuse to creep up, eventually hitting the
"Virtqueue size exceeded" error.

Properly discarding the element on device reset makes sure that its
buffers are unmapped and the inuse counter stays balanced.

Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Roman Kagan <rkagan@virtuozzo.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Ladi Prosek <lprosek@redhat.com>
---
 hw/virtio/virtio-balloon.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 5af429a..ad4189a 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -463,6 +463,7 @@ static void virtio_balloon_device_reset(VirtIODevice *vdev)
     VirtIOBalloon *s = VIRTIO_BALLOON(vdev);
 
     if (s->stats_vq_elem != NULL) {
+        virtqueue_discard(s->svq, s->stats_vq_elem, 0);
         g_free(s->stats_vq_elem);
         s->stats_vq_elem = NULL;
     }
-- 
2.5.5

[Qemu-devel] [PATCH 2/3] virtio: add virtqueue_rewind()

[Ladi Prosek]

From: Stefan Hajnoczi <stefanha@redhat.com>

virtqueue_discard() requires a VirtQueueElement but virtio-balloon does
not migrate its in-use element.  Introduce a new function that is
similar to virtqueue_discard() but doesn't require a VirtQueueElement.

This will allow virtio-balloon to access element again after migration
with the usual proviso that the guest may have modified the vring since
last time.

Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Roman Kagan <rkagan@virtuozzo.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Ladi Prosek <lprosek@redhat.com>
---
 hw/virtio/virtio.c         | 22 ++++++++++++++++++++++
 include/hw/virtio/virtio.h |  1 +
 2 files changed, 23 insertions(+)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 74c085c..3de6029 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -272,6 +272,28 @@ void virtqueue_discard(VirtQueue *vq, const VirtQueueElement *elem,
     virtqueue_unmap_sg(vq, elem, len);
 }
 
+/* virtqueue_rewind:
+ * @vq: The #VirtQueue
+ * @num: Number of elements to push back
+ *
+ * Pretend that elements weren't popped from the virtqueue.  The next
+ * virtqueue_pop() will refetch the oldest element.
+ *
+ * Use virtqueue_discard() instead if you have a VirtQueueElement.
+ *
+ * Returns: true on success, false if @num is greater than the number of in use
+ * elements.
+ */
+bool virtqueue_rewind(VirtQueue *vq, unsigned int num)
+{
+    if (num > vq->inuse) {
+        return false;
+    }
+    vq->last_avail_idx -= num;
+    vq->inuse -= num;
+    return true;
+}
+
 void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem,
                     unsigned int len, unsigned int idx)
 {
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index d2490c1..f05559d 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -154,6 +154,7 @@ void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem,
 void virtqueue_flush(VirtQueue *vq, unsigned int count);
 void virtqueue_discard(VirtQueue *vq, const VirtQueueElement *elem,
                        unsigned int len);
+bool virtqueue_rewind(VirtQueue *vq, unsigned int num);
 void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem,
                     unsigned int len, unsigned int idx);
 
-- 
2.5.5

[Qemu-devel] [PATCH 3/3] virtio-balloon: fix stats vq migration

[Ladi Prosek]

The statistics virtqueue is not migrated properly because virtio-balloon
does not include s->stats_vq_elem in the migration stream.

After migration the statistics virtqueue hangs because the host never
completes the last element (s->stats_vq_elem is NULL on the destination
QEMU).  Therefore the guest never submits new elements and the virtqueue
is hung.

Instead of changing the migration stream format in an incompatible way,
detect the migration case and rewind the virtqueue so the last element
can be completed.

Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Roman Kagan <rkagan@virtuozzo.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Suggested-by: Roman Kagan <rkagan@virtuozzo.com>
Signed-off-by: Ladi Prosek <lprosek@redhat.com>
---
 hw/virtio/virtio-balloon.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index ad4189a..49a2f4a 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -469,6 +469,18 @@ static void virtio_balloon_device_reset(VirtIODevice *vdev)
     }
 }
 
+static void virtio_balloon_set_status(VirtIODevice *vdev, uint8_t status)
+{
+    VirtIOBalloon *s = VIRTIO_BALLOON(vdev);
+
+    if (!s->stats_vq_elem && vdev->vm_running &&
+        (status & VIRTIO_CONFIG_S_DRIVER_OK) && virtqueue_rewind(s->svq, 1)) {
+        /* poll stats queue for the element we have discarded when the VM
+         * was stopped */
+        virtio_balloon_receive_stats(vdev, s->svq);
+    }
+}
+
 static void virtio_balloon_instance_init(Object *obj)
 {
     VirtIOBalloon *s = VIRTIO_BALLOON(obj);
@@ -506,6 +518,7 @@ static void virtio_balloon_class_init(ObjectClass *klass, void *data)
     vdc->get_features = virtio_balloon_get_features;
     vdc->save = virtio_balloon_save_device;
     vdc->load = virtio_balloon_load_device;
+    vdc->set_status = virtio_balloon_set_status;
 }
 
 static const TypeInfo virtio_balloon_info = {
-- 
2.5.5

Re: [Qemu-devel] [PATCH 1/3] virtio-balloon: discard virtqueue element on reset

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 693 bytes --]

On Wed, Sep 07, 2016 at 05:20:47PM +0200, Ladi Prosek wrote:
> The one pending element is being freed but not discarded on device
> reset, which causes svq->inuse to creep up, eventually hitting the
> "Virtqueue size exceeded" error.
> 
> Properly discarding the element on device reset makes sure that its
> buffers are unmapped and the inuse counter stays balanced.
> 
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Roman Kagan <rkagan@virtuozzo.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Ladi Prosek <lprosek@redhat.com>
> ---
>  hw/virtio/virtio-balloon.c | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 0/3] virtio-balloon: stats vq fixes

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 570 bytes --]

On Wed, Sep 07, 2016 at 05:20:46PM +0200, Ladi Prosek wrote:
> It is not
> necessary to check balloon_stats_supported. Pushing buffers without
> negotiating stats support would be a driver bug. The common codepath
> does not check it either.

This part made me nervous because QEMU is never allowed to trust the
guest - QEMU must never crash or expose information to the guest.

However this seems okay since virtqueue_rewind() on an unused virtqueue
is safe.  It will return false so we'll never actually try to access a
virtqueue that hasn't been initialized.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 3/3] virtio-balloon: fix stats vq migration

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 999 bytes --]

On Wed, Sep 07, 2016 at 05:20:49PM +0200, Ladi Prosek wrote:
> The statistics virtqueue is not migrated properly because virtio-balloon
> does not include s->stats_vq_elem in the migration stream.
> 
> After migration the statistics virtqueue hangs because the host never
> completes the last element (s->stats_vq_elem is NULL on the destination
> QEMU).  Therefore the guest never submits new elements and the virtqueue
> is hung.
> 
> Instead of changing the migration stream format in an incompatible way,
> detect the migration case and rewind the virtqueue so the last element
> can be completed.
> 
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Roman Kagan <rkagan@virtuozzo.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Suggested-by: Roman Kagan <rkagan@virtuozzo.com>
> Signed-off-by: Ladi Prosek <lprosek@redhat.com>
> ---
>  hw/virtio/virtio-balloon.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 1/3] virtio-balloon: discard virtqueue element on reset

[Roman Kagan]

On Wed, Sep 07, 2016 at 05:20:47PM +0200, Ladi Prosek wrote:
> The one pending element is being freed but not discarded on device
> reset, which causes svq->inuse to creep up, eventually hitting the
> "Virtqueue size exceeded" error.
> 
> Properly discarding the element on device reset makes sure that its
> buffers are unmapped and the inuse counter stays balanced.
> 
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Roman Kagan <rkagan@virtuozzo.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Ladi Prosek <lprosek@redhat.com>
> ---
>  hw/virtio/virtio-balloon.c | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>

Re: [Qemu-devel] [PATCH 2/3] virtio: add virtqueue_rewind()

[Roman Kagan]

On Wed, Sep 07, 2016 at 05:20:48PM +0200, Ladi Prosek wrote:
> From: Stefan Hajnoczi <stefanha@redhat.com>
> 
> virtqueue_discard() requires a VirtQueueElement but virtio-balloon does
> not migrate its in-use element.  Introduce a new function that is
> similar to virtqueue_discard() but doesn't require a VirtQueueElement.
> 
> This will allow virtio-balloon to access element again after migration
> with the usual proviso that the guest may have modified the vring since
> last time.
> 
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Roman Kagan <rkagan@virtuozzo.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Ladi Prosek <lprosek@redhat.com>
> ---
>  hw/virtio/virtio.c         | 22 ++++++++++++++++++++++
>  include/hw/virtio/virtio.h |  1 +
>  2 files changed, 23 insertions(+)

Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>

One question though:

> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 74c085c..3de6029 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -272,6 +272,28 @@ void virtqueue_discard(VirtQueue *vq, const VirtQueueElement *elem,
>      virtqueue_unmap_sg(vq, elem, len);
>  }
>  
> +/* virtqueue_rewind:
> + * @vq: The #VirtQueue
> + * @num: Number of elements to push back
> + *
> + * Pretend that elements weren't popped from the virtqueue.  The next
> + * virtqueue_pop() will refetch the oldest element.
> + *
> + * Use virtqueue_discard() instead if you have a VirtQueueElement.
> + *
> + * Returns: true on success, false if @num is greater than the number of in use
> + * elements.
> + */
> +bool virtqueue_rewind(VirtQueue *vq, unsigned int num)
> +{
> +    if (num > vq->inuse) {
> +        return false;
> +    }
> +    vq->last_avail_idx -= num;
> +    vq->inuse -= num;
> +    return true;
> +}
> +

Presumably you envision rewinding by something other than ->inuse.  Do
you have in mind a usecase for that, or is it just a matter of API
symmetry or whatnot?

Thanks,
Roman.

Re: [Qemu-devel] [PATCH 3/3] virtio-balloon: fix stats vq migration

[Roman Kagan]

On Wed, Sep 07, 2016 at 05:20:49PM +0200, Ladi Prosek wrote:
> The statistics virtqueue is not migrated properly because virtio-balloon
> does not include s->stats_vq_elem in the migration stream.
> 
> After migration the statistics virtqueue hangs because the host never
> completes the last element (s->stats_vq_elem is NULL on the destination
> QEMU).  Therefore the guest never submits new elements and the virtqueue
> is hung.
> 
> Instead of changing the migration stream format in an incompatible way,
> detect the migration case and rewind the virtqueue so the last element
> can be completed.
> 
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Roman Kagan <rkagan@virtuozzo.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Suggested-by: Roman Kagan <rkagan@virtuozzo.com>
> Signed-off-by: Ladi Prosek <lprosek@redhat.com>
> ---
>  hw/virtio/virtio-balloon.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)

Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>

Re: [Qemu-devel] [PATCH 2/3] virtio: add virtqueue_rewind()

[Ladi Prosek]

On Thu, Sep 8, 2016 at 8:44 AM, Roman Kagan <rkagan@virtuozzo.com> wrote:
> On Wed, Sep 07, 2016 at 05:20:48PM +0200, Ladi Prosek wrote:
>> From: Stefan Hajnoczi <stefanha@redhat.com>
>>
>> virtqueue_discard() requires a VirtQueueElement but virtio-balloon does
>> not migrate its in-use element.  Introduce a new function that is
>> similar to virtqueue_discard() but doesn't require a VirtQueueElement.
>>
>> This will allow virtio-balloon to access element again after migration
>> with the usual proviso that the guest may have modified the vring since
>> last time.
>>
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> Cc: Roman Kagan <rkagan@virtuozzo.com>
>> Cc: Stefan Hajnoczi <stefanha@redhat.com>
>> Signed-off-by: Ladi Prosek <lprosek@redhat.com>
>> ---
>>  hw/virtio/virtio.c         | 22 ++++++++++++++++++++++
>>  include/hw/virtio/virtio.h |  1 +
>>  2 files changed, 23 insertions(+)
>
> Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
>
> One question though:
>
>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>> index 74c085c..3de6029 100644
>> --- a/hw/virtio/virtio.c
>> +++ b/hw/virtio/virtio.c
>> @@ -272,6 +272,28 @@ void virtqueue_discard(VirtQueue *vq, const VirtQueueElement *elem,
>>      virtqueue_unmap_sg(vq, elem, len);
>>  }
>>
>> +/* virtqueue_rewind:
>> + * @vq: The #VirtQueue
>> + * @num: Number of elements to push back
>> + *
>> + * Pretend that elements weren't popped from the virtqueue.  The next
>> + * virtqueue_pop() will refetch the oldest element.
>> + *
>> + * Use virtqueue_discard() instead if you have a VirtQueueElement.
>> + *
>> + * Returns: true on success, false if @num is greater than the number of in use
>> + * elements.
>> + */
>> +bool virtqueue_rewind(VirtQueue *vq, unsigned int num)
>> +{
>> +    if (num > vq->inuse) {
>> +        return false;
>> +    }
>> +    vq->last_avail_idx -= num;
>> +    vq->inuse -= num;
>> +    return true;
>> +}
>> +
>
> Presumably you envision rewinding by something other than ->inuse.  Do
> you have in mind a usecase for that, or is it just a matter of API
> symmetry or whatnot?

It may not always be correct to rewind by ->inuse. The elements that
are in use do not necessarily have to be at the end of the available
ring. Example:

elem1 = virtqueue_pop();
elem2 = virtqueue_pop();
elem3 = virtqueue_pop();

virtqueue_push(elem2);

Now inuse == 2 but rewinding by 2 would be incorrect because elem2 has
already been pushed to the used ring.

So it is a dangerous API, which I believe is why Stefan added the num
parameter even though it is currently not needed.

Thanks,
Ladi

Re: [Qemu-devel] [PATCH 2/3] virtio: add virtqueue_rewind()

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 3036 bytes --]

On Thu, Sep 08, 2016 at 09:30:13AM +0200, Ladi Prosek wrote:
> On Thu, Sep 8, 2016 at 8:44 AM, Roman Kagan <rkagan@virtuozzo.com> wrote:
> > On Wed, Sep 07, 2016 at 05:20:48PM +0200, Ladi Prosek wrote:
> >> From: Stefan Hajnoczi <stefanha@redhat.com>
> >>
> >> virtqueue_discard() requires a VirtQueueElement but virtio-balloon does
> >> not migrate its in-use element.  Introduce a new function that is
> >> similar to virtqueue_discard() but doesn't require a VirtQueueElement.
> >>
> >> This will allow virtio-balloon to access element again after migration
> >> with the usual proviso that the guest may have modified the vring since
> >> last time.
> >>
> >> Cc: Michael S. Tsirkin <mst@redhat.com>
> >> Cc: Roman Kagan <rkagan@virtuozzo.com>
> >> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> >> Signed-off-by: Ladi Prosek <lprosek@redhat.com>
> >> ---
> >>  hw/virtio/virtio.c         | 22 ++++++++++++++++++++++
> >>  include/hw/virtio/virtio.h |  1 +
> >>  2 files changed, 23 insertions(+)
> >
> > Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
> >
> > One question though:
> >
> >> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> >> index 74c085c..3de6029 100644
> >> --- a/hw/virtio/virtio.c
> >> +++ b/hw/virtio/virtio.c
> >> @@ -272,6 +272,28 @@ void virtqueue_discard(VirtQueue *vq, const VirtQueueElement *elem,
> >>      virtqueue_unmap_sg(vq, elem, len);
> >>  }
> >>
> >> +/* virtqueue_rewind:
> >> + * @vq: The #VirtQueue
> >> + * @num: Number of elements to push back
> >> + *
> >> + * Pretend that elements weren't popped from the virtqueue.  The next
> >> + * virtqueue_pop() will refetch the oldest element.
> >> + *
> >> + * Use virtqueue_discard() instead if you have a VirtQueueElement.
> >> + *
> >> + * Returns: true on success, false if @num is greater than the number of in use
> >> + * elements.
> >> + */
> >> +bool virtqueue_rewind(VirtQueue *vq, unsigned int num)
> >> +{
> >> +    if (num > vq->inuse) {
> >> +        return false;
> >> +    }
> >> +    vq->last_avail_idx -= num;
> >> +    vq->inuse -= num;
> >> +    return true;
> >> +}
> >> +
> >
> > Presumably you envision rewinding by something other than ->inuse.  Do
> > you have in mind a usecase for that, or is it just a matter of API
> > symmetry or whatnot?
> 
> It may not always be correct to rewind by ->inuse. The elements that
> are in use do not necessarily have to be at the end of the available
> ring. Example:
> 
> elem1 = virtqueue_pop();
> elem2 = virtqueue_pop();
> elem3 = virtqueue_pop();
> 
> virtqueue_push(elem2);
> 
> Now inuse == 2 but rewinding by 2 would be incorrect because elem2 has
> already been pushed to the used ring.
> 
> So it is a dangerous API, which I believe is why Stefan added the num
> parameter even though it is currently not needed.

The function could be hard-coded to rewind just 1 element.  I wanted to
make it easy for a device to rewind the last n elements, but this
functionality isn't used.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]