[Qemu-devel] [PATCH 0/4] virtio-balloon: assorted fixes

[Qemu-devel] [PATCH 0/4] virtio-balloon: assorted fixes

[Roman Kagan]

This patchset addresses a few problems discovered when analyzing aborts
of (an older version of) QEMU with backported commit
afd9096eb1882f23929f5b5c177898ed231bac66 "virtio: error out if guest
exceeds virtqueue size".  Those problems are present in master, too,
except that they don't trigger an abort and are thus not as easy to
notice.


Roman Kagan (4):
  virtio: assert on ->inuse underflow
  virtio-balloon: make stats virtqueue length 1
  virtio-balloon: don't restart stats timer in callback
  virtio-balloon: keep collecting stats on save/restore

Cc: "Michael S. Tsirkin" <mst@redhat.com>

 hw/virtio/virtio-balloon.c         | 49 +++++++++++++++++++++-----------------
 hw/virtio/virtio.c                 |  3 ++-
 include/hw/virtio/virtio-balloon.h |  1 +
 3 files changed, 30 insertions(+), 23 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/4] virtio: assert on ->inuse underflow

[Roman Kagan]

Make sure that ->inuse counter on virtqueue never goes negative.

This complements commit afd9096eb1882f23929f5b5c177898ed231bac66,
"virtio: error out if guest exceeds virtqueue size", which, due to
signed ->inuse comparison against unsigned ->vring.num, manifested a bug
in virtio-balloon where virtqueue_push() was called before the matching
virtqueu_pop(). [That problem will be addressed in followup patches].

Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Stefan Hajnoczi <stefanha@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 15ee3a7..7a57857 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -92,7 +92,7 @@ struct VirtQueue
 
     uint16_t queue_index;
 
-    int inuse;
+    unsigned int inuse;
 
     uint16_t vector;
     VirtIOHandleOutput handle_output;
@@ -290,6 +290,7 @@ void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem,
 void virtqueue_flush(VirtQueue *vq, unsigned int count)
 {
     uint16_t old, new;
+    assert(vq->inuse >= count);
     /* Make sure buffer is written before we update index. */
     smp_wmb();
     trace_virtqueue_flush(vq, count);
-- 
2.7.4

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

[Roman Kagan]

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 <rkagan@virtuozzo.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Ladi Prosek <lprosek@redhat.com>
---
 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);
 
     reset_stats(s);
 }
-- 
2.7.4

[Qemu-devel] [PATCH 3/4] virtio-balloon: don't restart stats timer in callback

[Roman Kagan]

There's no need to restart the stats timer in its callback.  If the
callback happens to run when there's nothing to do just do nothing and
return.

The timer is armed either in receive handler or initially when
periodic stats collection is enabled via QMP.

While at this, observe that the presence of ->stats_vq_elem is enough to
indicate there's work to do here, and drop the check for the stats
feature.

Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
---
 hw/virtio/virtio-balloon.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 0baf4b3..b56fecd 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -68,12 +68,6 @@ static inline void reset_stats(VirtIOBalloon *dev)
     for (i = 0; i < VIRTIO_BALLOON_S_NR; dev->stats[i++] = -1);
 }
 
-static bool balloon_stats_supported(const VirtIOBalloon *s)
-{
-    VirtIODevice *vdev = VIRTIO_DEVICE(s);
-    return virtio_vdev_has_feature(vdev, VIRTIO_BALLOON_F_STATS_VQ);
-}
-
 static bool balloon_stats_enabled(const VirtIOBalloon *s)
 {
     return s->stats_poll_interval > 0;
@@ -99,9 +93,10 @@ static void balloon_stats_poll_cb(void *opaque)
     VirtIOBalloon *s = opaque;
     VirtIODevice *vdev = VIRTIO_DEVICE(s);
 
-    if (s->stats_vq_elem == NULL || !balloon_stats_supported(s)) {
-        /* re-schedule */
-        balloon_stats_change_timer(s, s->stats_poll_interval);
+    if (!s->stats_vq_elem) {
+        /* The guest hasn't sent the stats yet (either not enabled or we came
+         * too early), nothing to do.  Once the guest starts sending stats the
+         * timer will get armed in receive handler */
         return;
     }
 
-- 
2.7.4

[Qemu-devel] [PATCH 4/4] virtio-balloon: keep collecting stats on save/restore

[Roman Kagan]

Upon save/restore virtio-balloon stats acquisition stops.  The reason is
that the fact that the (only) virtqueue element is being used by QEMU is
not recorded anywhere on save, so upon restore it's not released to the
guest, making further progress impossible.

Saving the information about the used element would introduce unjustified
vmstate incompatibility.

So instead just make sure the element is pushed before save, leaving the
ball on the guest side.  For that, add vm state change handler to
virtio-ballon which would take care of pushing the element if there is
one.

Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
---
 hw/virtio/virtio-balloon.c         | 27 ++++++++++++++++++++++-----
 include/hw/virtio/virtio-balloon.h |  1 +
 2 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index b56fecd..66a926a 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -88,10 +88,19 @@ static void balloon_stats_change_timer(VirtIOBalloon *s, int64_t secs)
     timer_mod(s->stats_timer, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + secs * 1000);
 }
 
+static void balloon_stats_push_elem(VirtIOBalloon *s)
+{
+    VirtIODevice *vdev = VIRTIO_DEVICE(s);
+
+    virtqueue_push(s->svq, s->stats_vq_elem, s->stats_vq_offset);
+    virtio_notify(vdev, s->svq);
+    g_free(s->stats_vq_elem);
+    s->stats_vq_elem = NULL;
+}
+
 static void balloon_stats_poll_cb(void *opaque)
 {
     VirtIOBalloon *s = opaque;
-    VirtIODevice *vdev = VIRTIO_DEVICE(s);
 
     if (!s->stats_vq_elem) {
         /* The guest hasn't sent the stats yet (either not enabled or we came
@@ -100,10 +109,7 @@ static void balloon_stats_poll_cb(void *opaque)
         return;
     }
 
-    virtqueue_push(s->svq, s->stats_vq_elem, s->stats_vq_offset);
-    virtio_notify(vdev, s->svq);
-    g_free(s->stats_vq_elem);
-    s->stats_vq_elem = NULL;
+    balloon_stats_push_elem(s);
 }
 
 static void balloon_stats_get_all(Object *obj, Visitor *v, const char *name,
@@ -411,6 +417,15 @@ static int virtio_balloon_load_device(VirtIODevice *vdev, QEMUFile *f,
     return 0;
 }
 
+static void balloon_vm_state_change(void *opaque, int running, RunState state)
+{
+    VirtIOBalloon *s = opaque;
+
+    if (!running && s->stats_vq_elem) {
+        balloon_stats_push_elem(s);
+    }
+}
+
 static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
@@ -433,6 +448,7 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
     s->dvq = virtio_add_queue(vdev, 128, virtio_balloon_handle_output);
     s->svq = virtio_add_queue(vdev, 1, virtio_balloon_receive_stats);
 
+    s->change = qemu_add_vm_change_state_handler(balloon_vm_state_change, s);
     reset_stats(s);
 }
 
@@ -441,6 +457,7 @@ static void virtio_balloon_device_unrealize(DeviceState *dev, Error **errp)
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
     VirtIOBalloon *s = VIRTIO_BALLOON(dev);
 
+    qemu_del_vm_change_state_handler(s->change);
     balloon_stats_destroy_timer(s);
     qemu_remove_balloon_handler(s);
     virtio_cleanup(vdev);
diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h
index 1ea13bd..d72ff7f 100644
--- a/include/hw/virtio/virtio-balloon.h
+++ b/include/hw/virtio/virtio-balloon.h
@@ -43,6 +43,7 @@ typedef struct VirtIOBalloon {
     int64_t stats_last_update;
     int64_t stats_poll_interval;
     uint32_t host_features;
+    VMChangeStateEntry *change;
 } VirtIOBalloon;
 
 #endif
-- 
2.7.4

Re: [Qemu-devel] [PATCH 4/4] virtio-balloon: keep collecting stats on save/restore

[Ladi Prosek]

On Thu, Aug 18, 2016 at 8:27 PM, Roman Kagan <rkagan@virtuozzo.com> wrote:
> Upon save/restore virtio-balloon stats acquisition stops.  The reason is
> that the fact that the (only) virtqueue element is being used by QEMU is
> not recorded anywhere on save, so upon restore it's not released to the
> guest, making further progress impossible.
>
> Saving the information about the used element would introduce unjustified
> vmstate incompatibility.
>
> So instead just make sure the element is pushed before save, leaving the
> ball on the guest side.  For that, add vm state change handler to
> virtio-ballon which would take care of pushing the element if there is
> one.

Please take a look at:
https://lists.gnu.org/archive/html/qemu-devel/2016-08/msg01038.html

virtqueue_discard looks like a better choice than virtqueue_push.
There's no need to cause churn on the guest side and receive an extra
set of stats in between the regular timer callbacks. Also in that
thread see the note about stats_vq_offset which is misused and can be
killed.

Thanks,
Ladi

> Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> ---
>  hw/virtio/virtio-balloon.c         | 27 ++++++++++++++++++++++-----
>  include/hw/virtio/virtio-balloon.h |  1 +
>  2 files changed, 23 insertions(+), 5 deletions(-)
>
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index b56fecd..66a926a 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -88,10 +88,19 @@ static void balloon_stats_change_timer(VirtIOBalloon *s, int64_t secs)
>      timer_mod(s->stats_timer, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + secs * 1000);
>  }
>
> +static void balloon_stats_push_elem(VirtIOBalloon *s)
> +{
> +    VirtIODevice *vdev = VIRTIO_DEVICE(s);
> +
> +    virtqueue_push(s->svq, s->stats_vq_elem, s->stats_vq_offset);
> +    virtio_notify(vdev, s->svq);
> +    g_free(s->stats_vq_elem);
> +    s->stats_vq_elem = NULL;
> +}
> +
>  static void balloon_stats_poll_cb(void *opaque)
>  {
>      VirtIOBalloon *s = opaque;
> -    VirtIODevice *vdev = VIRTIO_DEVICE(s);
>
>      if (!s->stats_vq_elem) {
>          /* The guest hasn't sent the stats yet (either not enabled or we came
> @@ -100,10 +109,7 @@ static void balloon_stats_poll_cb(void *opaque)
>          return;
>      }
>
> -    virtqueue_push(s->svq, s->stats_vq_elem, s->stats_vq_offset);
> -    virtio_notify(vdev, s->svq);
> -    g_free(s->stats_vq_elem);
> -    s->stats_vq_elem = NULL;
> +    balloon_stats_push_elem(s);
>  }
>
>  static void balloon_stats_get_all(Object *obj, Visitor *v, const char *name,
> @@ -411,6 +417,15 @@ static int virtio_balloon_load_device(VirtIODevice *vdev, QEMUFile *f,
>      return 0;
>  }
>
> +static void balloon_vm_state_change(void *opaque, int running, RunState state)
> +{
> +    VirtIOBalloon *s = opaque;
> +
> +    if (!running && s->stats_vq_elem) {
> +        balloon_stats_push_elem(s);
> +    }
> +}
> +
>  static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
>  {
>      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> @@ -433,6 +448,7 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
>      s->dvq = virtio_add_queue(vdev, 128, virtio_balloon_handle_output);
>      s->svq = virtio_add_queue(vdev, 1, virtio_balloon_receive_stats);
>
> +    s->change = qemu_add_vm_change_state_handler(balloon_vm_state_change, s);
>      reset_stats(s);
>  }
>
> @@ -441,6 +457,7 @@ static void virtio_balloon_device_unrealize(DeviceState *dev, Error **errp)
>      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>      VirtIOBalloon *s = VIRTIO_BALLOON(dev);
>
> +    qemu_del_vm_change_state_handler(s->change);
>      balloon_stats_destroy_timer(s);
>      qemu_remove_balloon_handler(s);
>      virtio_cleanup(vdev);
> diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h
> index 1ea13bd..d72ff7f 100644
> --- a/include/hw/virtio/virtio-balloon.h
> +++ b/include/hw/virtio/virtio-balloon.h
> @@ -43,6 +43,7 @@ typedef struct VirtIOBalloon {
>      int64_t stats_last_update;
>      int64_t stats_poll_interval;
>      uint32_t host_features;
> +    VMChangeStateEntry *change;
>  } VirtIOBalloon;
>
>  #endif
> --
> 2.7.4
>
>

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

[Ladi Prosek]

On Thu, Aug 18, 2016 at 8:27 PM, Roman Kagan <rkagan@virtuozzo.com> 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.

Looks good to me, one nit inline.

> Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Ladi Prosek <lprosek@redhat.com>
> ---
>  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);
> -    }
> -

Worth adding an assert?

assert(s->stats_vq_elem == NULL); // enforced by one-element stats virtqueue

>      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);
>
>      reset_stats(s);
>  }
> --
> 2.7.4
>

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

[Roman Kagan]

On Fri, Aug 19, 2016 at 12:05:29PM +0200, Ladi Prosek wrote:
> On Thu, Aug 18, 2016 at 8:27 PM, Roman Kagan <rkagan@virtuozzo.com> 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.
> 
> Looks good to me, one nit inline.
> 
> > Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> > Cc: "Michael S. Tsirkin" <mst@redhat.com>
> > Cc: Ladi Prosek <lprosek@redhat.com>
> > ---
> >  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);
> > -    }
> > -
> 
> Worth adding an assert?
> 
> assert(s->stats_vq_elem == NULL); // enforced by one-element stats virtqueue

Yep, makes perfect sense.

Thanks,
Roman.

Re: [Qemu-devel] [PATCH 4/4] virtio-balloon: keep collecting stats on save/restore

[Roman Kagan]

On Fri, Aug 19, 2016 at 11:57:44AM +0200, Ladi Prosek wrote:
> On Thu, Aug 18, 2016 at 8:27 PM, Roman Kagan <rkagan@virtuozzo.com> wrote:
> > Upon save/restore virtio-balloon stats acquisition stops.  The reason is
> > that the fact that the (only) virtqueue element is being used by QEMU is
> > not recorded anywhere on save, so upon restore it's not released to the
> > guest, making further progress impossible.
> >
> > Saving the information about the used element would introduce unjustified
> > vmstate incompatibility.
> >
> > So instead just make sure the element is pushed before save, leaving the
> > ball on the guest side.  For that, add vm state change handler to
> > virtio-ballon which would take care of pushing the element if there is
> > one.
> 
> Please take a look at:
> https://lists.gnu.org/archive/html/qemu-devel/2016-08/msg01038.html

Thanks for the pointer, I wish I did a better maillist search before
submitting.

> virtqueue_discard looks like a better choice than virtqueue_push.
> There's no need to cause churn on the guest side and receive an extra
> set of stats in between the regular timer callbacks.

I forgot about virtqueue_discard; however, I still tend to slightly
prefer the push version as it IMO makes things easier to follow: the
pumping of stats is always initiated by the guest (both initially on
guest driver load and upon restore), and ->stats_vq_elem being non-NULL
is enough to know that it needs pushing (both in the timer callback and
in vm state change handler).

> Also in that thread see the note about stats_vq_offset which is
> misused and can be killed.

Indeed.

Thanks,
Roman.

> > Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> > Cc: "Michael S. Tsirkin" <mst@redhat.com>
> > ---
> >  hw/virtio/virtio-balloon.c         | 27 ++++++++++++++++++++++-----
> >  include/hw/virtio/virtio-balloon.h |  1 +
> >  2 files changed, 23 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> > index b56fecd..66a926a 100644
> > --- a/hw/virtio/virtio-balloon.c
> > +++ b/hw/virtio/virtio-balloon.c
> > @@ -88,10 +88,19 @@ static void balloon_stats_change_timer(VirtIOBalloon *s, int64_t secs)
> >      timer_mod(s->stats_timer, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + secs * 1000);
> >  }
> >
> > +static void balloon_stats_push_elem(VirtIOBalloon *s)
> > +{
> > +    VirtIODevice *vdev = VIRTIO_DEVICE(s);
> > +
> > +    virtqueue_push(s->svq, s->stats_vq_elem, s->stats_vq_offset);
> > +    virtio_notify(vdev, s->svq);
> > +    g_free(s->stats_vq_elem);
> > +    s->stats_vq_elem = NULL;
> > +}
> > +
> >  static void balloon_stats_poll_cb(void *opaque)
> >  {
> >      VirtIOBalloon *s = opaque;
> > -    VirtIODevice *vdev = VIRTIO_DEVICE(s);
> >
> >      if (!s->stats_vq_elem) {
> >          /* The guest hasn't sent the stats yet (either not enabled or we came
> > @@ -100,10 +109,7 @@ static void balloon_stats_poll_cb(void *opaque)
> >          return;
> >      }
> >
> > -    virtqueue_push(s->svq, s->stats_vq_elem, s->stats_vq_offset);
> > -    virtio_notify(vdev, s->svq);
> > -    g_free(s->stats_vq_elem);
> > -    s->stats_vq_elem = NULL;
> > +    balloon_stats_push_elem(s);
> >  }
> >
> >  static void balloon_stats_get_all(Object *obj, Visitor *v, const char *name,
> > @@ -411,6 +417,15 @@ static int virtio_balloon_load_device(VirtIODevice *vdev, QEMUFile *f,
> >      return 0;
> >  }
> >
> > +static void balloon_vm_state_change(void *opaque, int running, RunState state)
> > +{
> > +    VirtIOBalloon *s = opaque;
> > +
> > +    if (!running && s->stats_vq_elem) {
> > +        balloon_stats_push_elem(s);
> > +    }
> > +}
> > +
> >  static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
> >  {
> >      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> > @@ -433,6 +448,7 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
> >      s->dvq = virtio_add_queue(vdev, 128, virtio_balloon_handle_output);
> >      s->svq = virtio_add_queue(vdev, 1, virtio_balloon_receive_stats);
> >
> > +    s->change = qemu_add_vm_change_state_handler(balloon_vm_state_change, s);
> >      reset_stats(s);
> >  }
> >
> > @@ -441,6 +457,7 @@ static void virtio_balloon_device_unrealize(DeviceState *dev, Error **errp)
> >      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> >      VirtIOBalloon *s = VIRTIO_BALLOON(dev);
> >
> > +    qemu_del_vm_change_state_handler(s->change);
> >      balloon_stats_destroy_timer(s);
> >      qemu_remove_balloon_handler(s);
> >      virtio_cleanup(vdev);
> > diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h
> > index 1ea13bd..d72ff7f 100644
> > --- a/include/hw/virtio/virtio-balloon.h
> > +++ b/include/hw/virtio/virtio-balloon.h
> > @@ -43,6 +43,7 @@ typedef struct VirtIOBalloon {
> >      int64_t stats_last_update;
> >      int64_t stats_poll_interval;
> >      uint32_t host_features;
> > +    VMChangeStateEntry *change;
> >  } VirtIOBalloon;
> >
> >  #endif
> > --
> > 2.7.4
> >
> >

Re: [Qemu-devel] [PATCH 4/4] virtio-balloon: keep collecting stats on save/restore

[Ladi Prosek]

On Fri, Aug 19, 2016 at 1:37 PM, Roman Kagan <rkagan@virtuozzo.com> wrote:
> On Fri, Aug 19, 2016 at 11:57:44AM +0200, Ladi Prosek wrote:
>> On Thu, Aug 18, 2016 at 8:27 PM, Roman Kagan <rkagan@virtuozzo.com> wrote:
>> > Upon save/restore virtio-balloon stats acquisition stops.  The reason is
>> > that the fact that the (only) virtqueue element is being used by QEMU is
>> > not recorded anywhere on save, so upon restore it's not released to the
>> > guest, making further progress impossible.
>> >
>> > Saving the information about the used element would introduce unjustified
>> > vmstate incompatibility.
>> >
>> > So instead just make sure the element is pushed before save, leaving the
>> > ball on the guest side.  For that, add vm state change handler to
>> > virtio-ballon which would take care of pushing the element if there is
>> > one.
>>
>> Please take a look at:
>> https://lists.gnu.org/archive/html/qemu-devel/2016-08/msg01038.html
>
> Thanks for the pointer, I wish I did a better maillist search before
> submitting.
>
>> virtqueue_discard looks like a better choice than virtqueue_push.
>> There's no need to cause churn on the guest side and receive an extra
>> set of stats in between the regular timer callbacks.
>
> I forgot about virtqueue_discard; however, I still tend to slightly
> prefer the push version as it IMO makes things easier to follow: the
> pumping of stats is always initiated by the guest (both initially on
> guest driver load and upon restore), and ->stats_vq_elem being non-NULL
> is enough to know that it needs pushing (both in the timer callback and
> in vm state change handler).

I think I agree, it is nicer this way. virtqueue_discard would add
another state of the system (element needs to be popped without first
getting notified by the guest) which is not necessary. Thanks!

>> Also in that thread see the note about stats_vq_offset which is
>> misused and can be killed.
>
> Indeed.
>
> Thanks,
> Roman.
>
>> > Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
>> > Cc: "Michael S. Tsirkin" <mst@redhat.com>
>> > ---
>> >  hw/virtio/virtio-balloon.c         | 27 ++++++++++++++++++++++-----
>> >  include/hw/virtio/virtio-balloon.h |  1 +
>> >  2 files changed, 23 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
>> > index b56fecd..66a926a 100644
>> > --- a/hw/virtio/virtio-balloon.c
>> > +++ b/hw/virtio/virtio-balloon.c
>> > @@ -88,10 +88,19 @@ static void balloon_stats_change_timer(VirtIOBalloon *s, int64_t secs)
>> >      timer_mod(s->stats_timer, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + secs * 1000);
>> >  }
>> >
>> > +static void balloon_stats_push_elem(VirtIOBalloon *s)
>> > +{
>> > +    VirtIODevice *vdev = VIRTIO_DEVICE(s);
>> > +
>> > +    virtqueue_push(s->svq, s->stats_vq_elem, s->stats_vq_offset);
>> > +    virtio_notify(vdev, s->svq);
>> > +    g_free(s->stats_vq_elem);
>> > +    s->stats_vq_elem = NULL;
>> > +}
>> > +
>> >  static void balloon_stats_poll_cb(void *opaque)
>> >  {
>> >      VirtIOBalloon *s = opaque;
>> > -    VirtIODevice *vdev = VIRTIO_DEVICE(s);
>> >
>> >      if (!s->stats_vq_elem) {
>> >          /* The guest hasn't sent the stats yet (either not enabled or we came
>> > @@ -100,10 +109,7 @@ static void balloon_stats_poll_cb(void *opaque)
>> >          return;
>> >      }
>> >
>> > -    virtqueue_push(s->svq, s->stats_vq_elem, s->stats_vq_offset);
>> > -    virtio_notify(vdev, s->svq);
>> > -    g_free(s->stats_vq_elem);
>> > -    s->stats_vq_elem = NULL;
>> > +    balloon_stats_push_elem(s);
>> >  }
>> >
>> >  static void balloon_stats_get_all(Object *obj, Visitor *v, const char *name,
>> > @@ -411,6 +417,15 @@ static int virtio_balloon_load_device(VirtIODevice *vdev, QEMUFile *f,
>> >      return 0;
>> >  }
>> >
>> > +static void balloon_vm_state_change(void *opaque, int running, RunState state)
>> > +{
>> > +    VirtIOBalloon *s = opaque;
>> > +
>> > +    if (!running && s->stats_vq_elem) {
>> > +        balloon_stats_push_elem(s);
>> > +    }
>> > +}
>> > +
>> >  static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
>> >  {
>> >      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>> > @@ -433,6 +448,7 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
>> >      s->dvq = virtio_add_queue(vdev, 128, virtio_balloon_handle_output);
>> >      s->svq = virtio_add_queue(vdev, 1, virtio_balloon_receive_stats);
>> >
>> > +    s->change = qemu_add_vm_change_state_handler(balloon_vm_state_change, s);
>> >      reset_stats(s);
>> >  }
>> >
>> > @@ -441,6 +457,7 @@ static void virtio_balloon_device_unrealize(DeviceState *dev, Error **errp)
>> >      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>> >      VirtIOBalloon *s = VIRTIO_BALLOON(dev);
>> >
>> > +    qemu_del_vm_change_state_handler(s->change);
>> >      balloon_stats_destroy_timer(s);
>> >      qemu_remove_balloon_handler(s);
>> >      virtio_cleanup(vdev);
>> > diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h
>> > index 1ea13bd..d72ff7f 100644
>> > --- a/include/hw/virtio/virtio-balloon.h
>> > +++ b/include/hw/virtio/virtio-balloon.h
>> > @@ -43,6 +43,7 @@ typedef struct VirtIOBalloon {
>> >      int64_t stats_last_update;
>> >      int64_t stats_poll_interval;
>> >      uint32_t host_features;
>> > +    VMChangeStateEntry *change;
>> >  } VirtIOBalloon;
>> >
>> >  #endif
>> > --
>> > 2.7.4
>> >
>> >

Re: [Qemu-devel] [PATCH 0/4] virtio-balloon: assorted fixes

[Stefan Hajnoczi]

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

On Thu, Aug 18, 2016 at 09:27:50PM +0300, Roman Kagan wrote:
> This patchset addresses a few problems discovered when analyzing aborts
> of (an older version of) QEMU with backported commit
> afd9096eb1882f23929f5b5c177898ed231bac66 "virtio: error out if guest
> exceeds virtqueue size".  Those problems are present in master, too,
> except that they don't trigger an abort and are thus not as easy to
> notice.
> 
> 
> Roman Kagan (4):
>   virtio: assert on ->inuse underflow
>   virtio-balloon: make stats virtqueue length 1
>   virtio-balloon: don't restart stats timer in callback
>   virtio-balloon: keep collecting stats on save/restore
> 
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> 
>  hw/virtio/virtio-balloon.c         | 49 +++++++++++++++++++++-----------------
>  hw/virtio/virtio.c                 |  3 ++-
>  include/hw/virtio/virtio-balloon.h |  1 +
>  3 files changed, 30 insertions(+), 23 deletions(-)

Please see these related patches on the mailing list:

1. [PATCH v3] balloon: Fix failure of updating guest memory status

   This addresses the virtio-balloon stats virtqueue hang after
   migration.

2. [PATCH 0/2] virtio: fix VirtQueue->inuse field

   Fixes to update virtio.c in code paths where it currently becomes
   inconsistent.

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

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

[Stefan Hajnoczi]

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

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 <rkagan@virtuozzo.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Ladi Prosek <lprosek@redhat.com>
> ---
>  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.

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

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

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 <rkagan@virtuozzo.com>
> > Cc: "Michael S. Tsirkin" <mst@redhat.com>
> > Cc: Ladi Prosek <lprosek@redhat.com>
> > ---
> >  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.

Re: [Qemu-devel] [PATCH 4/4] virtio-balloon: keep collecting stats on save/restore

[Roman Kagan]

On Fri, Aug 19, 2016 at 02:08:50PM +0200, Ladi Prosek wrote:
> On Fri, Aug 19, 2016 at 1:37 PM, Roman Kagan <rkagan@virtuozzo.com> wrote:
> > On Fri, Aug 19, 2016 at 11:57:44AM +0200, Ladi Prosek wrote:
> >> On Thu, Aug 18, 2016 at 8:27 PM, Roman Kagan <rkagan@virtuozzo.com> wrote:
> >> > Upon save/restore virtio-balloon stats acquisition stops.  The reason is
> >> > that the fact that the (only) virtqueue element is being used by QEMU is
> >> > not recorded anywhere on save, so upon restore it's not released to the
> >> > guest, making further progress impossible.
> >> >
> >> > Saving the information about the used element would introduce unjustified
> >> > vmstate incompatibility.
> >> >
> >> > So instead just make sure the element is pushed before save, leaving the
> >> > ball on the guest side.  For that, add vm state change handler to
> >> > virtio-ballon which would take care of pushing the element if there is
> >> > one.
> >>
> >> Please take a look at:
> >> https://lists.gnu.org/archive/html/qemu-devel/2016-08/msg01038.html
> >
> > Thanks for the pointer, I wish I did a better maillist search before
> > submitting.
> >
> >> virtqueue_discard looks like a better choice than virtqueue_push.
> >> There's no need to cause churn on the guest side and receive an extra
> >> set of stats in between the regular timer callbacks.
> >
> > I forgot about virtqueue_discard; however, I still tend to slightly
> > prefer the push version as it IMO makes things easier to follow: the
> > pumping of stats is always initiated by the guest (both initially on
> > guest driver load and upon restore), and ->stats_vq_elem being non-NULL
> > is enough to know that it needs pushing (both in the timer callback and
> > in vm state change handler).
> 
> I think I agree, it is nicer this way. virtqueue_discard would add
> another state of the system (element needs to be popped without first
> getting notified by the guest) which is not necessary. Thanks!

On a second thought (induced by Stefan's comment), my patch doesn't fix
the problem for loading the state saved by unfixed QEMU, while yours
does.  

So please disregrard this series, and let's move on with yours.

Thanks,
Roman.

Re: [Qemu-devel] [PATCH 4/4] virtio-balloon: keep collecting stats on save/restore

[Ladi Prosek]

On Fri, Aug 19, 2016 at 4:45 PM, Roman Kagan <rkagan@virtuozzo.com> wrote:
> On Fri, Aug 19, 2016 at 02:08:50PM +0200, Ladi Prosek wrote:
>> On Fri, Aug 19, 2016 at 1:37 PM, Roman Kagan <rkagan@virtuozzo.com> wrote:
>> > On Fri, Aug 19, 2016 at 11:57:44AM +0200, Ladi Prosek wrote:
>> >> On Thu, Aug 18, 2016 at 8:27 PM, Roman Kagan <rkagan@virtuozzo.com> wrote:
>> >> > Upon save/restore virtio-balloon stats acquisition stops.  The reason is
>> >> > that the fact that the (only) virtqueue element is being used by QEMU is
>> >> > not recorded anywhere on save, so upon restore it's not released to the
>> >> > guest, making further progress impossible.
>> >> >
>> >> > Saving the information about the used element would introduce unjustified
>> >> > vmstate incompatibility.
>> >> >
>> >> > So instead just make sure the element is pushed before save, leaving the
>> >> > ball on the guest side.  For that, add vm state change handler to
>> >> > virtio-ballon which would take care of pushing the element if there is
>> >> > one.
>> >>
>> >> Please take a look at:
>> >> https://lists.gnu.org/archive/html/qemu-devel/2016-08/msg01038.html
>> >
>> > Thanks for the pointer, I wish I did a better maillist search before
>> > submitting.
>> >
>> >> virtqueue_discard looks like a better choice than virtqueue_push.
>> >> There's no need to cause churn on the guest side and receive an extra
>> >> set of stats in between the regular timer callbacks.
>> >
>> > I forgot about virtqueue_discard; however, I still tend to slightly
>> > prefer the push version as it IMO makes things easier to follow: the
>> > pumping of stats is always initiated by the guest (both initially on
>> > guest driver load and upon restore), and ->stats_vq_elem being non-NULL
>> > is enough to know that it needs pushing (both in the timer callback and
>> > in vm state change handler).
>>
>> I think I agree, it is nicer this way. virtqueue_discard would add
>> another state of the system (element needs to be popped without first
>> getting notified by the guest) which is not necessary. Thanks!
>
> On a second thought (induced by Stefan's comment), my patch doesn't fix
> the problem for loading the state saved by unfixed QEMU, while yours
> does.

Interesting - I don't think that either patch fixes the issue when
only the loading side is fixed. But your patch handles the opposite
case where only the saving side is fixed, while mine doesn't. With
your patch the state that gets saved is a valid state today (the
timing would have to be right but it is possible to get it with
today's QEMU). With my patch I'm adding a new state that the loading
QEMU must understand, kind of what I alluded to in the previous email.

I hope I'm not missing something. This is where a whiteboard would be handy :)

> So please disregrard this series, and let's move on with yours.
>
> Thanks,
> Roman.