[PATCH 0/2] virtio-balloon: Free page hinting clean-ups

[PATCH 0/2] virtio-balloon: Free page hinting clean-ups

[Alexander Duyck]

This series contains a couple minor cleanups related to free page hinting.

The first patch addresses what I believe is a possible issue in which the
driver could potentially force the device out of the stop state and back
into the running state if it were to replay an earlier virtqueue element
containing the same ID it had submitted earlier.

The second patch takes care of renaming various hinting objects that were
using "reporting" in the name to try and clarify which objects are for free
page reporting and which are for free page hinting.

---

Alexander Duyck (2):
      virtio-balloon: Prevent guest from starting a report when we didn't request one
      virtio-balloon: Replace free page hinting references to 'report' with 'hint'


 hw/virtio/virtio-balloon.c         |   77 +++++++++++++++++++-----------------
 include/hw/virtio/virtio-balloon.h |   20 +++++----
 2 files changed, 50 insertions(+), 47 deletions(-)

--

[PATCH 1/2] virtio-balloon: Prevent guest from starting a report when we didn't request one

[Alexander Duyck]

From: Alexander Duyck <alexander.h.duyck@linux.intel.com>

Based on code review it appears possible for the driver to force the device
out of a stopped state when hinting by repeating the last ID it was
provided.

Prevent this by only allowing a transition to the start state when we are
in the requested state. This way the driver is only allowed to send one
descriptor that will transition the device into the start state. All others
will leave it in the stop state once it has finished.

In addition add the necessary locking to provent any potential races
between the accesses of the cmd_id and the status.

Fixes: c13c4153f76d ("virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT")
Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
---
 hw/virtio/virtio-balloon.c |   11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 10507b2a430a..7f3af266f674 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -527,7 +527,8 @@ static bool get_free_page_hints(VirtIOBalloon *dev)
             ret = false;
             goto out;
         }
-        if (id == dev->free_page_report_cmd_id) {
+        if (dev->free_page_report_status == FREE_PAGE_REPORT_S_REQUESTED &&
+            id == dev->free_page_report_cmd_id) {
             dev->free_page_report_status = FREE_PAGE_REPORT_S_START;
         } else {
             /*
@@ -592,14 +593,16 @@ static void virtio_balloon_free_page_start(VirtIOBalloon *s)
         return;
     }
 
-    if (s->free_page_report_cmd_id == UINT_MAX) {
+    qemu_mutex_lock(&s->free_page_lock);
+
+    if (s->free_page_report_cmd_id++ == UINT_MAX) {
         s->free_page_report_cmd_id =
                        VIRTIO_BALLOON_FREE_PAGE_REPORT_CMD_ID_MIN;
-    } else {
-        s->free_page_report_cmd_id++;
     }
 
     s->free_page_report_status = FREE_PAGE_REPORT_S_REQUESTED;
+    qemu_mutex_unlock(&s->free_page_lock);
+
     virtio_notify_config(vdev);
 }
 

[PATCH 2/2] virtio-balloon: Replace free page hinting references to 'report' with 'hint'

[Alexander Duyck]

From: Alexander Duyck <alexander.h.duyck@linux.intel.com>

Recently a feature named Free Page Reporting was added to the virtio
balloon. In order to avoid any confusion we should drop the use of the word
'report' when referring to Free Page Hinting. So what this patch does is go
through and replace all instances of 'report' with 'hint" when we are
referring to free page hinting.

Acked-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
---
 hw/virtio/virtio-balloon.c         |   72 ++++++++++++++++++------------------
 include/hw/virtio/virtio-balloon.h |   20 +++++-----
 2 files changed, 46 insertions(+), 46 deletions(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 7f3af266f674..eea53078a73f 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -527,22 +527,22 @@ static bool get_free_page_hints(VirtIOBalloon *dev)
             ret = false;
             goto out;
         }
-        if (dev->free_page_report_status == FREE_PAGE_REPORT_S_REQUESTED &&
-            id == dev->free_page_report_cmd_id) {
-            dev->free_page_report_status = FREE_PAGE_REPORT_S_START;
+        if (dev->free_page_hint_status == FREE_PAGE_HINT_S_REQUESTED &&
+            id == dev->free_page_hint_cmd_id) {
+            dev->free_page_hint_status = FREE_PAGE_HINT_S_START;
         } else {
             /*
              * Stop the optimization only when it has started. This
              * avoids a stale stop sign for the previous command.
              */
-            if (dev->free_page_report_status == FREE_PAGE_REPORT_S_START) {
-                dev->free_page_report_status = FREE_PAGE_REPORT_S_STOP;
+            if (dev->free_page_hint_status == FREE_PAGE_HINT_S_START) {
+                dev->free_page_hint_status = FREE_PAGE_HINT_S_STOP;
             }
         }
     }
 
     if (elem->in_num) {
-        if (dev->free_page_report_status == FREE_PAGE_REPORT_S_START) {
+        if (dev->free_page_hint_status == FREE_PAGE_HINT_S_START) {
             qemu_guest_free_page_hint(elem->in_sg[0].iov_base,
                                       elem->in_sg[0].iov_len);
         }
@@ -568,11 +568,11 @@ static void virtio_ballloon_get_free_page_hints(void *opaque)
         qemu_mutex_unlock(&dev->free_page_lock);
         virtio_notify(vdev, vq);
       /*
-       * Start to poll the vq once the reporting started. Otherwise, continue
+       * Start to poll the vq once the hinting started. Otherwise, continue
        * only when there are entries on the vq, which need to be given back.
        */
     } while (continue_to_get_hints ||
-             dev->free_page_report_status == FREE_PAGE_REPORT_S_START);
+             dev->free_page_hint_status == FREE_PAGE_HINT_S_START);
     virtio_queue_set_notification(vq, 1);
 }
 
@@ -595,12 +595,12 @@ static void virtio_balloon_free_page_start(VirtIOBalloon *s)
 
     qemu_mutex_lock(&s->free_page_lock);
 
-    if (s->free_page_report_cmd_id++ == UINT_MAX) {
-        s->free_page_report_cmd_id =
-                       VIRTIO_BALLOON_FREE_PAGE_REPORT_CMD_ID_MIN;
+    if (s->free_page_hint_cmd_id++ == UINT_MAX) {
+        s->free_page_hint_cmd_id =
+                       VIRTIO_BALLOON_FREE_PAGE_HINT_CMD_ID_MIN;
     }
 
-    s->free_page_report_status = FREE_PAGE_REPORT_S_REQUESTED;
+    s->free_page_hint_status = FREE_PAGE_HINT_S_REQUESTED;
     qemu_mutex_unlock(&s->free_page_lock);
 
     virtio_notify_config(vdev);
@@ -610,18 +610,18 @@ static void virtio_balloon_free_page_stop(VirtIOBalloon *s)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(s);
 
-    if (s->free_page_report_status != FREE_PAGE_REPORT_S_STOP) {
+    if (s->free_page_hint_status != FREE_PAGE_HINT_S_STOP) {
         /*
          * The lock also guarantees us that the
          * virtio_ballloon_get_free_page_hints exits after the
-         * free_page_report_status is set to S_STOP.
+         * free_page_hint_status is set to S_STOP.
          */
         qemu_mutex_lock(&s->free_page_lock);
         /*
-         * The guest hasn't done the reporting, so host sends a notification
-         * to the guest to actively stop the reporting.
+         * The guest isn't done hinting, so send a notification
+         * to the guest to actively stop the hinting.
          */
-        s->free_page_report_status = FREE_PAGE_REPORT_S_STOP;
+        s->free_page_hint_status = FREE_PAGE_HINT_S_STOP;
         qemu_mutex_unlock(&s->free_page_lock);
         virtio_notify_config(vdev);
     }
@@ -631,15 +631,15 @@ static void virtio_balloon_free_page_done(VirtIOBalloon *s)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(s);
 
-    s->free_page_report_status = FREE_PAGE_REPORT_S_DONE;
+    s->free_page_hint_status = FREE_PAGE_HINT_S_DONE;
     virtio_notify_config(vdev);
 }
 
 static int
-virtio_balloon_free_page_report_notify(NotifierWithReturn *n, void *data)
+virtio_balloon_free_page_hint_notify(NotifierWithReturn *n, void *data)
 {
     VirtIOBalloon *dev = container_of(n, VirtIOBalloon,
-                                      free_page_report_notify);
+                                      free_page_hint_notify);
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
     PrecopyNotifyData *pnd = data;
 
@@ -688,7 +688,7 @@ static size_t virtio_balloon_config_size(VirtIOBalloon *s)
     if (virtio_has_feature(features, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
         return offsetof(struct virtio_balloon_config, poison_val);
     }
-    return offsetof(struct virtio_balloon_config, free_page_report_cmd_id);
+    return offsetof(struct virtio_balloon_config, free_page_hint_cmd_id);
 }
 
 static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
@@ -700,14 +700,14 @@ static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
     config.actual = cpu_to_le32(dev->actual);
     config.poison_val = cpu_to_le32(dev->poison_val);
 
-    if (dev->free_page_report_status == FREE_PAGE_REPORT_S_REQUESTED) {
-        config.free_page_report_cmd_id =
-                       cpu_to_le32(dev->free_page_report_cmd_id);
-    } else if (dev->free_page_report_status == FREE_PAGE_REPORT_S_STOP) {
-        config.free_page_report_cmd_id =
+    if (dev->free_page_hint_status == FREE_PAGE_HINT_S_REQUESTED) {
+        config.free_page_hint_cmd_id =
+                       cpu_to_le32(dev->free_page_hint_cmd_id);
+    } else if (dev->free_page_hint_status == FREE_PAGE_HINT_S_STOP) {
+        config.free_page_hint_cmd_id =
                        cpu_to_le32(VIRTIO_BALLOON_CMD_ID_STOP);
-    } else if (dev->free_page_report_status == FREE_PAGE_REPORT_S_DONE) {
-        config.free_page_report_cmd_id =
+    } else if (dev->free_page_hint_status == FREE_PAGE_HINT_S_DONE) {
+        config.free_page_hint_cmd_id =
                        cpu_to_le32(VIRTIO_BALLOON_CMD_ID_DONE);
     }
 
@@ -820,14 +820,14 @@ static int virtio_balloon_post_load_device(void *opaque, int version_id)
     return 0;
 }
 
-static const VMStateDescription vmstate_virtio_balloon_free_page_report = {
+static const VMStateDescription vmstate_virtio_balloon_free_page_hint = {
     .name = "virtio-balloon-device/free-page-report",
     .version_id = 1,
     .minimum_version_id = 1,
     .needed = virtio_balloon_free_page_support,
     .fields = (VMStateField[]) {
-        VMSTATE_UINT32(free_page_report_cmd_id, VirtIOBalloon),
-        VMSTATE_UINT32(free_page_report_status, VirtIOBalloon),
+        VMSTATE_UINT32(free_page_hint_cmd_id, VirtIOBalloon),
+        VMSTATE_UINT32(free_page_hint_status, VirtIOBalloon),
         VMSTATE_END_OF_LIST()
     }
 };
@@ -854,7 +854,7 @@ static const VMStateDescription vmstate_virtio_balloon_device = {
         VMSTATE_END_OF_LIST()
     },
     .subsections = (const VMStateDescription * []) {
-        &vmstate_virtio_balloon_free_page_report,
+        &vmstate_virtio_balloon_free_page_hint,
         &vmstate_virtio_balloon_page_poison,
         NULL
     }
@@ -893,7 +893,7 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
                            VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
         s->free_page_vq = virtio_add_queue(vdev, VIRTQUEUE_MAX_SIZE,
                                            virtio_balloon_handle_free_page_vq);
-        precopy_add_notifier(&s->free_page_report_notify);
+        precopy_add_notifier(&s->free_page_hint_notify);
 
         object_ref(OBJECT(s->iothread));
         s->free_page_bh = aio_bh_new(iothread_get_aio_context(s->iothread),
@@ -917,7 +917,7 @@ static void virtio_balloon_device_unrealize(DeviceState *dev)
         qemu_bh_delete(s->free_page_bh);
         object_unref(OBJECT(s->iothread));
         virtio_balloon_free_page_stop(s);
-        precopy_remove_notifier(&s->free_page_report_notify);
+        precopy_remove_notifier(&s->free_page_hint_notify);
     }
     balloon_stats_destroy_timer(s);
     qemu_remove_balloon_handler(s);
@@ -989,8 +989,8 @@ static void virtio_balloon_instance_init(Object *obj)
 
     qemu_mutex_init(&s->free_page_lock);
     qemu_cond_init(&s->free_page_cond);
-    s->free_page_report_cmd_id = VIRTIO_BALLOON_FREE_PAGE_REPORT_CMD_ID_MIN;
-    s->free_page_report_notify.notify = virtio_balloon_free_page_report_notify;
+    s->free_page_hint_cmd_id = VIRTIO_BALLOON_FREE_PAGE_HINT_CMD_ID_MIN;
+    s->free_page_hint_notify.notify = virtio_balloon_free_page_hint_notify;
 
     object_property_add(obj, "guest-stats", "guest statistics",
                         balloon_stats_get_all, NULL, NULL, s);
diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h
index d49fef00cef2..28fd2b396087 100644
--- a/include/hw/virtio/virtio-balloon.h
+++ b/include/hw/virtio/virtio-balloon.h
@@ -23,7 +23,7 @@
 #define VIRTIO_BALLOON(obj) \
         OBJECT_CHECK(VirtIOBalloon, (obj), TYPE_VIRTIO_BALLOON)
 
-#define VIRTIO_BALLOON_FREE_PAGE_REPORT_CMD_ID_MIN 0x80000000
+#define VIRTIO_BALLOON_FREE_PAGE_HINT_CMD_ID_MIN 0x80000000
 
 typedef struct virtio_balloon_stat VirtIOBalloonStat;
 
@@ -33,20 +33,20 @@ typedef struct virtio_balloon_stat_modern {
        uint64_t val;
 } VirtIOBalloonStatModern;
 
-enum virtio_balloon_free_page_report_status {
-    FREE_PAGE_REPORT_S_STOP = 0,
-    FREE_PAGE_REPORT_S_REQUESTED = 1,
-    FREE_PAGE_REPORT_S_START = 2,
-    FREE_PAGE_REPORT_S_DONE = 3,
+enum virtio_balloon_free_page_hint_status {
+    FREE_PAGE_HINT_S_STOP = 0,
+    FREE_PAGE_HINT_S_REQUESTED = 1,
+    FREE_PAGE_HINT_S_START = 2,
+    FREE_PAGE_HINT_S_DONE = 3,
 };
 
 typedef struct VirtIOBalloon {
     VirtIODevice parent_obj;
     VirtQueue *ivq, *dvq, *svq, *free_page_vq, *reporting_vq;
-    uint32_t free_page_report_status;
+    uint32_t free_page_hint_status;
     uint32_t num_pages;
     uint32_t actual;
-    uint32_t free_page_report_cmd_id;
+    uint32_t free_page_hint_cmd_id;
     uint64_t stats[VIRTIO_BALLOON_S_NR];
     VirtQueueElement *stats_vq_elem;
     size_t stats_vq_offset;
@@ -55,7 +55,7 @@ typedef struct VirtIOBalloon {
     QEMUBH *free_page_bh;
     /*
      * Lock to synchronize threads to access the free page reporting related
-     * fields (e.g. free_page_report_status).
+     * fields (e.g. free_page_hint_status).
      */
     QemuMutex free_page_lock;
     QemuCond  free_page_cond;
@@ -64,7 +64,7 @@ typedef struct VirtIOBalloon {
      * stopped.
      */
     bool block_iothread;
-    NotifierWithReturn free_page_report_notify;
+    NotifierWithReturn free_page_hint_notify;
     int64_t stats_last_update;
     int64_t stats_poll_interval;
     uint32_t host_features;

Re: [PATCH 1/2] virtio-balloon: Prevent guest from starting a report when we didn't request one

[David Hildenbrand]

On 19.06.20 23:53, Alexander Duyck wrote:
> From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> 
> Based on code review it appears possible for the driver to force the device
> out of a stopped state when hinting by repeating the last ID it was
> provided.

Indeed, thanks for noticing.

> 
> Prevent this by only allowing a transition to the start state when we are
> in the requested state. This way the driver is only allowed to send one
> descriptor that will transition the device into the start state. All others
> will leave it in the stop state once it has finished.
> 
> In addition add the necessary locking to provent any potential races

s/provent/prevent/

> between the accesses of the cmd_id and the status.
> 
> Fixes: c13c4153f76d ("virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT")
> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> ---
>  hw/virtio/virtio-balloon.c |   11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index 10507b2a430a..7f3af266f674 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -527,7 +527,8 @@ static bool get_free_page_hints(VirtIOBalloon *dev)
>              ret = false;
>              goto out;
>          }
> -        if (id == dev->free_page_report_cmd_id) {
> +        if (dev->free_page_report_status == FREE_PAGE_REPORT_S_REQUESTED &&
> +            id == dev->free_page_report_cmd_id) {
>              dev->free_page_report_status = FREE_PAGE_REPORT_S_START;
>          } else {
>              /*

But doesn't that mean that, after the first hint, all further ones will
be discarded and we'll enter the STOP state in the else case? Or am I
missing something?

Shouldn't this be something like

if (id == dev->free_page_report_cmd_id) {
    if (dev->free_page_report_status == FREE_PAGE_REPORT_S_REQUESTED) {
        dev->free_page_report_status = FREE_PAGE_REPORT_S_START;
    }
    /* Stay in FREE_PAGE_REPORT_S_START as long as the cmd_id match .*/
} else { ...

> @@ -592,14 +593,16 @@ static void virtio_balloon_free_page_start(VirtIOBalloon *s)
>          return;
>      }
>  
> -    if (s->free_page_report_cmd_id == UINT_MAX) {
> +    qemu_mutex_lock(&s->free_page_lock);
> +
> +    if (s->free_page_report_cmd_id++ == UINT_MAX) {
>          s->free_page_report_cmd_id =
>                         VIRTIO_BALLOON_FREE_PAGE_REPORT_CMD_ID_MIN;
> -    } else {
> -        s->free_page_report_cmd_id++;
>      }

Somewhat unrelated cleanup.

>  
>      s->free_page_report_status = FREE_PAGE_REPORT_S_REQUESTED;
> +    qemu_mutex_unlock(&s->free_page_lock);
> +
>      virtio_notify_config(vdev);
>  }
>  
> 


-- 
Thanks,

David / dhildenb

Re: [PATCH 1/2] virtio-balloon: Prevent guest from starting a report when we didn't request one

[Alexander Duyck]

On Mon, Jun 22, 2020 at 1:10 AM David Hildenbrand <david@redhat.com> wrote:
>
> On 19.06.20 23:53, Alexander Duyck wrote:
> > From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> >
> > Based on code review it appears possible for the driver to force the device
> > out of a stopped state when hinting by repeating the last ID it was
> > provided.
>
> Indeed, thanks for noticing.
>
> >
> > Prevent this by only allowing a transition to the start state when we are
> > in the requested state. This way the driver is only allowed to send one
> > descriptor that will transition the device into the start state. All others
> > will leave it in the stop state once it has finished.
> >
> > In addition add the necessary locking to provent any potential races
>
> s/provent/prevent/

Thanks for catching that. I will fix the typo.

> > between the accesses of the cmd_id and the status.
> >
> > Fixes: c13c4153f76d ("virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT")
> > Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> > ---
> >  hw/virtio/virtio-balloon.c |   11 +++++++----
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> > index 10507b2a430a..7f3af266f674 100644
> > --- a/hw/virtio/virtio-balloon.c
> > +++ b/hw/virtio/virtio-balloon.c
> > @@ -527,7 +527,8 @@ static bool get_free_page_hints(VirtIOBalloon *dev)
> >              ret = false;
> >              goto out;
> >          }
> > -        if (id == dev->free_page_report_cmd_id) {
> > +        if (dev->free_page_report_status == FREE_PAGE_REPORT_S_REQUESTED &&
> > +            id == dev->free_page_report_cmd_id) {
> >              dev->free_page_report_status = FREE_PAGE_REPORT_S_START;
> >          } else {
> >              /*
>
> But doesn't that mean that, after the first hint, all further ones will
> be discarded and we'll enter the STOP state in the else case? Or am I
> missing something?
>
> Shouldn't this be something like
>
> if (id == dev->free_page_report_cmd_id) {
>     if (dev->free_page_report_status == FREE_PAGE_REPORT_S_REQUESTED) {
>         dev->free_page_report_status = FREE_PAGE_REPORT_S_START;
>     }
>     /* Stay in FREE_PAGE_REPORT_S_START as long as the cmd_id match .*/
> } else { ...

There should only be one element containing an outbuf at the start of
the report. Once that is processed we should not see the driver
sending additional outbufs unless it is sending the STOP command ID.

> > @@ -592,14 +593,16 @@ static void virtio_balloon_free_page_start(VirtIOBalloon *s)
> >          return;
> >      }
> >
> > -    if (s->free_page_report_cmd_id == UINT_MAX) {
> > +    qemu_mutex_lock(&s->free_page_lock);
> > +
> > +    if (s->free_page_report_cmd_id++ == UINT_MAX) {
> >          s->free_page_report_cmd_id =
> >                         VIRTIO_BALLOON_FREE_PAGE_REPORT_CMD_ID_MIN;
> > -    } else {
> > -        s->free_page_report_cmd_id++;
> >      }
>
> Somewhat unrelated cleanup.

Agreed. I can drop it if preferred. I just took care of it because I
was adding the lock above and below to prevent us from getting into
any wierd states where the command ID might be updated but the report
status was not.

> >
> >      s->free_page_report_status = FREE_PAGE_REPORT_S_REQUESTED;
> > +    qemu_mutex_unlock(&s->free_page_lock);
> > +
> >      virtio_notify_config(vdev);
> >  }
> >
> >
>
>
> --
> Thanks,
>
> David / dhildenb
>

Re: [PATCH 1/2] virtio-balloon: Prevent guest from starting a report when we didn't request one

[David Hildenbrand]

>>> +++ b/hw/virtio/virtio-balloon.c
>>> @@ -527,7 +527,8 @@ static bool get_free_page_hints(VirtIOBalloon *dev)
>>>              ret = false;
>>>              goto out;
>>>          }
>>> -        if (id == dev->free_page_report_cmd_id) {
>>> +        if (dev->free_page_report_status == FREE_PAGE_REPORT_S_REQUESTED &&
>>> +            id == dev->free_page_report_cmd_id) {
>>>              dev->free_page_report_status = FREE_PAGE_REPORT_S_START;
>>>          } else {
>>>              /*
>>
>> But doesn't that mean that, after the first hint, all further ones will
>> be discarded and we'll enter the STOP state in the else case? Or am I
>> missing something?
>>
>> Shouldn't this be something like
>>
>> if (id == dev->free_page_report_cmd_id) {
>>     if (dev->free_page_report_status == FREE_PAGE_REPORT_S_REQUESTED) {
>>         dev->free_page_report_status = FREE_PAGE_REPORT_S_START;
>>     }
>>     /* Stay in FREE_PAGE_REPORT_S_START as long as the cmd_id match .*/
>> } else { ...
> 
> There should only be one element containing an outbuf at the start of
> the report. Once that is processed we should not see the driver
> sending additional outbufs unless it is sending the STOP command ID.

Ok, I assume what Linux guests do is considered the correct protocol.

[...]

> 
>>> @@ -592,14 +593,16 @@ static void virtio_balloon_free_page_start(VirtIOBalloon *s)
>>>          return;
>>>      }
>>>
>>> -    if (s->free_page_report_cmd_id == UINT_MAX) {
>>> +    qemu_mutex_lock(&s->free_page_lock);
>>> +
>>> +    if (s->free_page_report_cmd_id++ == UINT_MAX) {
>>>          s->free_page_report_cmd_id =
>>>                         VIRTIO_BALLOON_FREE_PAGE_REPORT_CMD_ID_MIN;
>>> -    } else {
>>> -        s->free_page_report_cmd_id++;
>>>      }
>>
>> Somewhat unrelated cleanup.
> 
> Agreed. I can drop it if preferred. I just took care of it because I
> was adding the lock above and below to prevent us from getting into
> any wierd states where the command ID might be updated but the report
> status was not.

No hard feelings, it just makes reviewing harder, because one has to
investigate how the changes relate to the locking changes - to find out
they don't. :)

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb