[PATCH v1 0/3] virtio-balloon: free page hinting fixes

[PATCH v1 0/3] virtio-balloon: free page hinting fixes

[David Hildenbrand]

Some fixes for VIRTIO_BALLOON_F_FREE_PAGE_HINT. First issue was reported by
Alexander Bulekov [1], the other ones were discovered by me when digging
into the details.

[1] https://lkml.kernel.org/r/20200511044121.eihns2tdimdzgi4i@mozz.bu.edu

David Hildenbrand (3):
  virtio-balloon: fix free page hinting without an iothread
  virtio-balloon: fix free page hinting check on unrealize
  virtio-balloon: unref the iothread when unrealizing

 hw/virtio/virtio-balloon.c | 38 ++++++++++++++++++++------------------
 1 file changed, 20 insertions(+), 18 deletions(-)

-- 
2.25.4

[PATCH v1 1/3] virtio-balloon: fix free page hinting without an iothread

[David Hildenbrand]

In case we don't have an iothread, we mark the feature as abscent but
still add the queue. 'free_page_bh' remains set to NULL.

qemu-system-i386 \
        -M microvm \
        -nographic \
        -device virtio-balloon-device,free-page-hint=true \
        -nographic \
        -display none \
        -monitor none \
        -serial none \
        -qtest stdio

Doing a "write 0xc0000e30 0x24
0x030000000300000003000000030000000300000003000000030000000300000003000000"

We will trigger a SEGFAULT. Let's move the check and bail out.

While at it, move the static initializations to instance_initialize().

Fixes: c13c4153f76d ("virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT")
Reported-by: Alexander Bulekov <alxndr@bu.edu>
Cc: Wei Wang <wei.w.wang@intel.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Cc: Alexander Duyck <alexander.duyck@gmail.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/virtio/virtio-balloon.c | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 065cd450f1..dc3b1067ab 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -789,6 +789,13 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
         return;
     }
 
+    if (virtio_has_feature(s->host_features,
+        VIRTIO_BALLOON_F_FREE_PAGE_HINT) && !s->iothread) {
+        error_setg(errp, "'free-page-hint' requires 'iothread' to be set");
+        virtio_cleanup(vdev);
+        return;
+    }
+
     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);
@@ -797,24 +804,11 @@ 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);
-        s->free_page_report_status = FREE_PAGE_REPORT_S_STOP;
-        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;
         precopy_add_notifier(&s->free_page_report_notify);
-        if (s->iothread) {
-            object_ref(OBJECT(s->iothread));
-            s->free_page_bh = aio_bh_new(iothread_get_aio_context(s->iothread),
-                                       virtio_ballloon_get_free_page_hints, s);
-            qemu_mutex_init(&s->free_page_lock);
-            qemu_cond_init(&s->free_page_cond);
-            s->block_iothread = false;
-        } else {
-            /* Simply disable this feature if the iothread wasn't created. */
-            s->host_features &= ~(1 << VIRTIO_BALLOON_F_FREE_PAGE_HINT);
-            virtio_error(vdev, "iothread is missing");
-        }
+
+        object_ref(OBJECT(s->iothread));
+        s->free_page_bh = aio_bh_new(iothread_get_aio_context(s->iothread),
+                                     virtio_ballloon_get_free_page_hints, s);
     }
     reset_stats(s);
 }
@@ -892,6 +886,13 @@ static void virtio_balloon_instance_init(Object *obj)
 {
     VirtIOBalloon *s = VIRTIO_BALLOON(obj);
 
+    qemu_mutex_init(&s->free_page_lock);
+    qemu_cond_init(&s->free_page_cond);
+    s->free_page_report_status = FREE_PAGE_REPORT_S_STOP;
+    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->block_iothread = false;
+
     object_property_add(obj, "guest-stats", "guest statistics",
                         balloon_stats_get_all, NULL, NULL, s);
 
-- 
2.25.4

[PATCH v1 2/3] virtio-balloon: fix free page hinting check on unrealize

[David Hildenbrand]

Checking against guest features is wrong. We allocated data structures
based on host features. We can rely on "free_page_bh" as an indicator
whether to un-do stuff instead.

Fixes: c13c4153f76d ("virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT")
Cc: Wei Wang <wei.w.wang@intel.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Cc: Alexander Duyck <alexander.duyck@gmail.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/virtio/virtio-balloon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index dc3b1067ab..a4fcf2d777 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -818,7 +818,7 @@ static void virtio_balloon_device_unrealize(DeviceState *dev)
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
     VirtIOBalloon *s = VIRTIO_BALLOON(dev);
 
-    if (virtio_balloon_free_page_support(s)) {
+    if (s->free_page_bh) {
         qemu_bh_delete(s->free_page_bh);
         virtio_balloon_free_page_stop(s);
         precopy_remove_notifier(&s->free_page_report_notify);
-- 
2.25.4

[PATCH v1 3/3] virtio-balloon: unref the iothread when unrealizing

[David Hildenbrand]

We took a reference when realizing, so let's drop that reference when
unrealizing.

Fixes: c13c4153f76d ("virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT")
Cc: Wei Wang <wei.w.wang@intel.com>
Cc: Alexander Duyck <alexander.duyck@gmail.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: David Hildenbrand <david@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 a4fcf2d777..3f8fc50be0 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -820,6 +820,7 @@ static void virtio_balloon_device_unrealize(DeviceState *dev)
 
     if (s->free_page_bh) {
         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);
     }
-- 
2.25.4

Re: [PATCH v1 1/3] virtio-balloon: fix free page hinting without an iothread

[Philippe Mathieu-Daudé]

On 5/18/20 10:37 AM, David Hildenbrand wrote:
> In case we don't have an iothread, we mark the feature as abscent but
> still add the queue. 'free_page_bh' remains set to NULL.
> 
> qemu-system-i386 \
>          -M microvm \
>          -nographic \
>          -device virtio-balloon-device,free-page-hint=true \
>          -nographic \
>          -display none \
>          -monitor none \
>          -serial none \
>          -qtest stdio
> 
> Doing a "write 0xc0000e30 0x24
> 0x030000000300000003000000030000000300000003000000030000000300000003000000"
> 
> We will trigger a SEGFAULT. Let's move the check and bail out.
> 
> While at it, move the static initializations to instance_initialize().
> 
> Fixes: c13c4153f76d ("virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT")
> Reported-by: Alexander Bulekov <alxndr@bu.edu>
> Cc: Wei Wang <wei.w.wang@intel.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Cc: Alexander Duyck <alexander.duyck@gmail.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   hw/virtio/virtio-balloon.c | 35 ++++++++++++++++++-----------------
>   1 file changed, 18 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index 065cd450f1..dc3b1067ab 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -789,6 +789,13 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
>           return;
>       }
>   
> +    if (virtio_has_feature(s->host_features,
> +        VIRTIO_BALLOON_F_FREE_PAGE_HINT) && !s->iothread) {
> +        error_setg(errp, "'free-page-hint' requires 'iothread' to be set");
> +        virtio_cleanup(vdev);
> +        return;

This is exactly what I had in mind!

> +    }
> +
>       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);
> @@ -797,24 +804,11 @@ 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);
> -        s->free_page_report_status = FREE_PAGE_REPORT_S_STOP;
> -        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;
>           precopy_add_notifier(&s->free_page_report_notify);
> -        if (s->iothread) {
> -            object_ref(OBJECT(s->iothread));
> -            s->free_page_bh = aio_bh_new(iothread_get_aio_context(s->iothread),
> -                                       virtio_ballloon_get_free_page_hints, s);
> -            qemu_mutex_init(&s->free_page_lock);
> -            qemu_cond_init(&s->free_page_cond);
> -            s->block_iothread = false;
> -        } else {
> -            /* Simply disable this feature if the iothread wasn't created. */
> -            s->host_features &= ~(1 << VIRTIO_BALLOON_F_FREE_PAGE_HINT);
> -            virtio_error(vdev, "iothread is missing");
> -        }
> +
> +        object_ref(OBJECT(s->iothread));
> +        s->free_page_bh = aio_bh_new(iothread_get_aio_context(s->iothread),
> +                                     virtio_ballloon_get_free_page_hints, s);
>       }
>       reset_stats(s);
>   }
> @@ -892,6 +886,13 @@ static void virtio_balloon_instance_init(Object *obj)
>   {
>       VirtIOBalloon *s = VIRTIO_BALLOON(obj);
>   
> +    qemu_mutex_init(&s->free_page_lock);
> +    qemu_cond_init(&s->free_page_cond);
> +    s->free_page_report_status = FREE_PAGE_REPORT_S_STOP;
> +    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->block_iothread = false;

This part is even cleaner.

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> +
>       object_property_add(obj, "guest-stats", "guest statistics",
>                           balloon_stats_get_all, NULL, NULL, s);
>   
> 

Re: [PATCH v1 2/3] virtio-balloon: fix free page hinting check on unrealize

[Philippe Mathieu-Daudé]

On 5/18/20 10:37 AM, David Hildenbrand wrote:
> Checking against guest features is wrong. We allocated data structures
> based on host features. We can rely on "free_page_bh" as an indicator
> whether to un-do stuff instead.
> 
> Fixes: c13c4153f76d ("virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT")
> Cc: Wei Wang <wei.w.wang@intel.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Cc: Alexander Duyck <alexander.duyck@gmail.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   hw/virtio/virtio-balloon.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index dc3b1067ab..a4fcf2d777 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -818,7 +818,7 @@ static void virtio_balloon_device_unrealize(DeviceState *dev)
>       VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>       VirtIOBalloon *s = VIRTIO_BALLOON(dev);
>   
> -    if (virtio_balloon_free_page_support(s)) {
> +    if (s->free_page_bh) {
>           qemu_bh_delete(s->free_page_bh);
>           virtio_balloon_free_page_stop(s);
>           precopy_remove_notifier(&s->free_page_report_notify);
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH v1 3/3] virtio-balloon: unref the iothread when unrealizing

[Philippe Mathieu-Daudé]

On 5/18/20 10:37 AM, David Hildenbrand wrote:
> We took a reference when realizing, so let's drop that reference when
> unrealizing.
> 
> Fixes: c13c4153f76d ("virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT")
> Cc: Wei Wang <wei.w.wang@intel.com>
> Cc: Alexander Duyck <alexander.duyck@gmail.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Signed-off-by: David Hildenbrand <david@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 a4fcf2d777..3f8fc50be0 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -820,6 +820,7 @@ static void virtio_balloon_device_unrealize(DeviceState *dev)
>   
>       if (s->free_page_bh) {
>           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);
>       }
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH v1 0/3] virtio-balloon: free page hinting fixes

[Philippe Mathieu-Daudé]

On 5/18/20 10:37 AM, David Hildenbrand wrote:
> Some fixes for VIRTIO_BALLOON_F_FREE_PAGE_HINT. First issue was reported by
> Alexander Bulekov [1], the other ones were discovered by me when digging
> into the details.
> 
> [1] https://lkml.kernel.org/r/20200511044121.eihns2tdimdzgi4i@mozz.bu.edu
> 
> David Hildenbrand (3):
>    virtio-balloon: fix free page hinting without an iothread
>    virtio-balloon: fix free page hinting check on unrealize
>    virtio-balloon: unref the iothread when unrealizing
> 
>   hw/virtio/virtio-balloon.c | 38 ++++++++++++++++++++------------------
>   1 file changed, 20 insertions(+), 18 deletions(-)
> 

All this series deserves a 'Cc: qemu-stable@nongnu.org' tag, right?

Re: [PATCH v1 0/3] virtio-balloon: free page hinting fixes

[David Hildenbrand]

On 18.05.20 11:20, Philippe Mathieu-Daudé wrote:
> On 5/18/20 10:37 AM, David Hildenbrand wrote:
>> Some fixes for VIRTIO_BALLOON_F_FREE_PAGE_HINT. First issue was reported by
>> Alexander Bulekov [1], the other ones were discovered by me when digging
>> into the details.
>>
>> [1] https://lkml.kernel.org/r/20200511044121.eihns2tdimdzgi4i@mozz.bu.edu
>>
>> David Hildenbrand (3):
>>    virtio-balloon: fix free page hinting without an iothread
>>    virtio-balloon: fix free page hinting check on unrealize
>>    virtio-balloon: unref the iothread when unrealizing
>>
>>   hw/virtio/virtio-balloon.c | 38 ++++++++++++++++++++------------------
>>   1 file changed, 20 insertions(+), 18 deletions(-)
>>
> 
> All this series deserves a 'Cc: qemu-stable@nongnu.org' tag, right?

I was asking myself the same question. People usually don't unplug the
balloon device, but yes, #2 and #3 can be triggered by the user. Most
probably yes, cc stable for all.

-- 
Thanks,

David / dhildenb

Re: [PATCH v1 1/3] virtio-balloon: fix free page hinting without an iothread

[Alexander Duyck]

On Mon, May 18, 2020 at 1:37 AM David Hildenbrand <david@redhat.com> wrote:
>
> In case we don't have an iothread, we mark the feature as abscent but
> still add the queue. 'free_page_bh' remains set to NULL.
>
> qemu-system-i386 \
>         -M microvm \
>         -nographic \
>         -device virtio-balloon-device,free-page-hint=true \
>         -nographic \
>         -display none \
>         -monitor none \
>         -serial none \
>         -qtest stdio
>
> Doing a "write 0xc0000e30 0x24
> 0x030000000300000003000000030000000300000003000000030000000300000003000000"
>
> We will trigger a SEGFAULT. Let's move the check and bail out.
>
> While at it, move the static initializations to instance_initialize().
>
> Fixes: c13c4153f76d ("virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT")
> Reported-by: Alexander Bulekov <alxndr@bu.edu>
> Cc: Wei Wang <wei.w.wang@intel.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Cc: Alexander Duyck <alexander.duyck@gmail.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/virtio/virtio-balloon.c | 35 ++++++++++++++++++-----------------
>  1 file changed, 18 insertions(+), 17 deletions(-)
>
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index 065cd450f1..dc3b1067ab 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -789,6 +789,13 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
>          return;
>      }
>
> +    if (virtio_has_feature(s->host_features,
> +        VIRTIO_BALLOON_F_FREE_PAGE_HINT) && !s->iothread) {
> +        error_setg(errp, "'free-page-hint' requires 'iothread' to be set");
> +        virtio_cleanup(vdev);
> +        return;
> +    }
> +
>      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);
> @@ -797,24 +804,11 @@ 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);
> -        s->free_page_report_status = FREE_PAGE_REPORT_S_STOP;
> -        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;
>          precopy_add_notifier(&s->free_page_report_notify);
> -        if (s->iothread) {
> -            object_ref(OBJECT(s->iothread));
> -            s->free_page_bh = aio_bh_new(iothread_get_aio_context(s->iothread),
> -                                       virtio_ballloon_get_free_page_hints, s);
> -            qemu_mutex_init(&s->free_page_lock);
> -            qemu_cond_init(&s->free_page_cond);
> -            s->block_iothread = false;
> -        } else {
> -            /* Simply disable this feature if the iothread wasn't created. */
> -            s->host_features &= ~(1 << VIRTIO_BALLOON_F_FREE_PAGE_HINT);
> -            virtio_error(vdev, "iothread is missing");
> -        }
> +
> +        object_ref(OBJECT(s->iothread));
> +        s->free_page_bh = aio_bh_new(iothread_get_aio_context(s->iothread),
> +                                     virtio_ballloon_get_free_page_hints, s);
>      }
>      reset_stats(s);
>  }
> @@ -892,6 +886,13 @@ static void virtio_balloon_instance_init(Object *obj)
>  {
>      VirtIOBalloon *s = VIRTIO_BALLOON(obj);
>
> +    qemu_mutex_init(&s->free_page_lock);
> +    qemu_cond_init(&s->free_page_cond);
> +    s->free_page_report_status = FREE_PAGE_REPORT_S_STOP;
> +    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->block_iothread = false;
> +
>      object_property_add(obj, "guest-stats", "guest statistics",
>                          balloon_stats_get_all, NULL, NULL, s);
>

So the one nit I have is that I am not sure you need to bother with
initializing block_iothread since it should already be initialized to
false/0 shouldn't it? Otherwise this all looks good to me.

Reviewed-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>

Re: [PATCH v1 2/3] virtio-balloon: fix free page hinting check on unrealize

[Alexander Duyck]

On Mon, May 18, 2020 at 1:37 AM David Hildenbrand <david@redhat.com> wrote:
>
> Checking against guest features is wrong. We allocated data structures
> based on host features. We can rely on "free_page_bh" as an indicator
> whether to un-do stuff instead.
>
> Fixes: c13c4153f76d ("virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT")
> Cc: Wei Wang <wei.w.wang@intel.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Cc: Alexander Duyck <alexander.duyck@gmail.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/virtio/virtio-balloon.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index dc3b1067ab..a4fcf2d777 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -818,7 +818,7 @@ static void virtio_balloon_device_unrealize(DeviceState *dev)
>      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>      VirtIOBalloon *s = VIRTIO_BALLOON(dev);
>
> -    if (virtio_balloon_free_page_support(s)) {
> +    if (s->free_page_bh) {
>          qemu_bh_delete(s->free_page_bh);
>          virtio_balloon_free_page_stop(s);
>          precopy_remove_notifier(&s->free_page_report_notify);

Would it make sense to apply the same change to
virtio_balloon_device_reset and virtio_balloon_set_status? At least in
the case of virtio_balloon_set_status it seems like you could possibly
exploit it somehow as clearing the feature in the guest will prevent
the toggling of the block_iothread value.

Reviewed-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>

Re: [PATCH v1 1/3] virtio-balloon: fix free page hinting without an iothread

[David Hildenbrand]

On 18.05.20 17:01, Alexander Duyck wrote:
> On Mon, May 18, 2020 at 1:37 AM David Hildenbrand <david@redhat.com> wrote:
>>
>> In case we don't have an iothread, we mark the feature as abscent but
>> still add the queue. 'free_page_bh' remains set to NULL.
>>
>> qemu-system-i386 \
>>         -M microvm \
>>         -nographic \
>>         -device virtio-balloon-device,free-page-hint=true \
>>         -nographic \
>>         -display none \
>>         -monitor none \
>>         -serial none \
>>         -qtest stdio
>>
>> Doing a "write 0xc0000e30 0x24
>> 0x030000000300000003000000030000000300000003000000030000000300000003000000"
>>
>> We will trigger a SEGFAULT. Let's move the check and bail out.
>>
>> While at it, move the static initializations to instance_initialize().
>>
>> Fixes: c13c4153f76d ("virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT")
>> Reported-by: Alexander Bulekov <alxndr@bu.edu>
>> Cc: Wei Wang <wei.w.wang@intel.com>
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
>> Cc: Alexander Duyck <alexander.duyck@gmail.com>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  hw/virtio/virtio-balloon.c | 35 ++++++++++++++++++-----------------
>>  1 file changed, 18 insertions(+), 17 deletions(-)
>>
>> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
>> index 065cd450f1..dc3b1067ab 100644
>> --- a/hw/virtio/virtio-balloon.c
>> +++ b/hw/virtio/virtio-balloon.c
>> @@ -789,6 +789,13 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
>>          return;
>>      }
>>
>> +    if (virtio_has_feature(s->host_features,
>> +        VIRTIO_BALLOON_F_FREE_PAGE_HINT) && !s->iothread) {
>> +        error_setg(errp, "'free-page-hint' requires 'iothread' to be set");
>> +        virtio_cleanup(vdev);
>> +        return;
>> +    }
>> +
>>      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);
>> @@ -797,24 +804,11 @@ 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);
>> -        s->free_page_report_status = FREE_PAGE_REPORT_S_STOP;
>> -        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;
>>          precopy_add_notifier(&s->free_page_report_notify);
>> -        if (s->iothread) {
>> -            object_ref(OBJECT(s->iothread));
>> -            s->free_page_bh = aio_bh_new(iothread_get_aio_context(s->iothread),
>> -                                       virtio_ballloon_get_free_page_hints, s);
>> -            qemu_mutex_init(&s->free_page_lock);
>> -            qemu_cond_init(&s->free_page_cond);
>> -            s->block_iothread = false;
>> -        } else {
>> -            /* Simply disable this feature if the iothread wasn't created. */
>> -            s->host_features &= ~(1 << VIRTIO_BALLOON_F_FREE_PAGE_HINT);
>> -            virtio_error(vdev, "iothread is missing");
>> -        }
>> +
>> +        object_ref(OBJECT(s->iothread));
>> +        s->free_page_bh = aio_bh_new(iothread_get_aio_context(s->iothread),
>> +                                     virtio_ballloon_get_free_page_hints, s);
>>      }
>>      reset_stats(s);
>>  }
>> @@ -892,6 +886,13 @@ static void virtio_balloon_instance_init(Object *obj)
>>  {
>>      VirtIOBalloon *s = VIRTIO_BALLOON(obj);
>>
>> +    qemu_mutex_init(&s->free_page_lock);
>> +    qemu_cond_init(&s->free_page_cond);
>> +    s->free_page_report_status = FREE_PAGE_REPORT_S_STOP;
>> +    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->block_iothread = false;
>> +
>>      object_property_add(obj, "guest-stats", "guest statistics",
>>                          balloon_stats_get_all, NULL, NULL, s);
>>
> 
> So the one nit I have is that I am not sure you need to bother with
> initializing block_iothread since it should already be initialized to
> false/0 shouldn't it? Otherwise this all looks good to me.

Yes, and as "FREE_PAGE_REPORT_S_STOP = 0", that's implicitly set as
well. Both can go IMHO.

> 
> Reviewed-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> 

Thanks!

-- 
Thanks,

David / dhildenb

Re: [PATCH v1 3/3] virtio-balloon: unref the iothread when unrealizing

[Alexander Duyck]

On Mon, May 18, 2020 at 1:37 AM David Hildenbrand <david@redhat.com> wrote:
>
> We took a reference when realizing, so let's drop that reference when
> unrealizing.
>
> Fixes: c13c4153f76d ("virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT")
> Cc: Wei Wang <wei.w.wang@intel.com>
> Cc: Alexander Duyck <alexander.duyck@gmail.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Signed-off-by: David Hildenbrand <david@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 a4fcf2d777..3f8fc50be0 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -820,6 +820,7 @@ static void virtio_balloon_device_unrealize(DeviceState *dev)
>
>      if (s->free_page_bh) {
>          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);
>      }

I'm not entirely sure about this order of operations. It seems like it
would make more sense to remove the notifier, stop the hinting, delete
the bh, and then release the IO thread.

Re: [PATCH v1 3/3] virtio-balloon: unref the iothread when unrealizing

[David Hildenbrand]

On 18.05.20 17:35, Alexander Duyck wrote:
> On Mon, May 18, 2020 at 1:37 AM David Hildenbrand <david@redhat.com> wrote:
>>
>> We took a reference when realizing, so let's drop that reference when
>> unrealizing.
>>
>> Fixes: c13c4153f76d ("virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT")
>> Cc: Wei Wang <wei.w.wang@intel.com>
>> Cc: Alexander Duyck <alexander.duyck@gmail.com>
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
>> Signed-off-by: David Hildenbrand <david@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 a4fcf2d777..3f8fc50be0 100644
>> --- a/hw/virtio/virtio-balloon.c
>> +++ b/hw/virtio/virtio-balloon.c
>> @@ -820,6 +820,7 @@ static void virtio_balloon_device_unrealize(DeviceState *dev)
>>
>>      if (s->free_page_bh) {
>>          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);
>>      }
> 
> I'm not entirely sure about this order of operations. It seems like it
> would make more sense to remove the notifier, stop the hinting, delete
> the bh, and then release the IO thread.

This is the reverse order of the steps in
virtio_balloon_device_realize(). And I guess it should be fine. The
notifier cannot really be active/trigger while we are removing devices
(cannot happen with concurrent migration). After qemu_bh_delete(), the
iothread is effectively unused.

I am unsure about many things regarding free page hinting (e.g., if the
virtio_balloon_free_page_stop() is of any use while we are ripping out
the device and it will be gone in a second).

-- 
Thanks,

David / dhildenb

Re: [PATCH v1 2/3] virtio-balloon: fix free page hinting check on unrealize

[David Hildenbrand]

On 18.05.20 17:20, Alexander Duyck wrote:
> On Mon, May 18, 2020 at 1:37 AM David Hildenbrand <david@redhat.com> wrote:
>>
>> Checking against guest features is wrong. We allocated data structures
>> based on host features. We can rely on "free_page_bh" as an indicator
>> whether to un-do stuff instead.
>>
>> Fixes: c13c4153f76d ("virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT")
>> Cc: Wei Wang <wei.w.wang@intel.com>
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
>> Cc: Alexander Duyck <alexander.duyck@gmail.com>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  hw/virtio/virtio-balloon.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
>> index dc3b1067ab..a4fcf2d777 100644
>> --- a/hw/virtio/virtio-balloon.c
>> +++ b/hw/virtio/virtio-balloon.c
>> @@ -818,7 +818,7 @@ static void virtio_balloon_device_unrealize(DeviceState *dev)
>>      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>>      VirtIOBalloon *s = VIRTIO_BALLOON(dev);
>>
>> -    if (virtio_balloon_free_page_support(s)) {
>> +    if (s->free_page_bh) {
>>          qemu_bh_delete(s->free_page_bh);
>>          virtio_balloon_free_page_stop(s);
>>          precopy_remove_notifier(&s->free_page_report_notify);
> 
> Would it make sense to apply the same change to
> virtio_balloon_device_reset and virtio_balloon_set_status? At least in

Good question ...

> the case of virtio_balloon_set_status it seems like you could possibly
> exploit it somehow as clearing the feature in the guest will prevent
> the toggling of the block_iothread value.

The guest cannot change features at random points in time. However, what
happens if we are stopped and trigger a system_reset? The the vdev
features are stale ... I'll play with it, see if this is required, and
add a separate patch if necessary!

> 
> Reviewed-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> 

Thanks!

-- 
Thanks,

David / dhildenb

Re: [PATCH v1 3/3] virtio-balloon: unref the iothread when unrealizing

[Philippe Mathieu-Daudé]

+Stefan/Paolo in case they have a better clue.

On 5/18/20 5:41 PM, David Hildenbrand wrote:
> On 18.05.20 17:35, Alexander Duyck wrote:
>> On Mon, May 18, 2020 at 1:37 AM David Hildenbrand <david@redhat.com> wrote:
>>>
>>> We took a reference when realizing, so let's drop that reference when
>>> unrealizing.
>>>
>>> Fixes: c13c4153f76d ("virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT")
>>> Cc: Wei Wang <wei.w.wang@intel.com>
>>> Cc: Alexander Duyck <alexander.duyck@gmail.com>
>>> Cc: Michael S. Tsirkin <mst@redhat.com>
>>> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
>>> Signed-off-by: David Hildenbrand <david@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 a4fcf2d777..3f8fc50be0 100644
>>> --- a/hw/virtio/virtio-balloon.c
>>> +++ b/hw/virtio/virtio-balloon.c
>>> @@ -820,6 +820,7 @@ static void virtio_balloon_device_unrealize(DeviceState *dev)
>>>
>>>       if (s->free_page_bh) {
>>>           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);
>>>       }
>>
>> I'm not entirely sure about this order of operations. It seems like it
>> would make more sense to remove the notifier, stop the hinting, delete
>> the bh, and then release the IO thread.
> 
> This is the reverse order of the steps in
> virtio_balloon_device_realize(). And I guess it should be fine. The
> notifier cannot really be active/trigger while we are removing devices
> (cannot happen with concurrent migration). After qemu_bh_delete(), the
> iothread is effectively unused.
> 
> I am unsure about many things regarding free page hinting (e.g., if the
> virtio_balloon_free_page_stop() is of any use while we are ripping out
> the device and it will be gone in a second).
> 

Re: [PATCH v1 3/3] virtio-balloon: unref the iothread when unrealizing

[Alexander Duyck]

On Mon, May 18, 2020 at 8:42 AM David Hildenbrand <david@redhat.com> wrote:
>
> On 18.05.20 17:35, Alexander Duyck wrote:
> > On Mon, May 18, 2020 at 1:37 AM David Hildenbrand <david@redhat.com> wrote:
> >>
> >> We took a reference when realizing, so let's drop that reference when
> >> unrealizing.
> >>
> >> Fixes: c13c4153f76d ("virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT")
> >> Cc: Wei Wang <wei.w.wang@intel.com>
> >> Cc: Alexander Duyck <alexander.duyck@gmail.com>
> >> Cc: Michael S. Tsirkin <mst@redhat.com>
> >> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> >> Signed-off-by: David Hildenbrand <david@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 a4fcf2d777..3f8fc50be0 100644
> >> --- a/hw/virtio/virtio-balloon.c
> >> +++ b/hw/virtio/virtio-balloon.c
> >> @@ -820,6 +820,7 @@ static void virtio_balloon_device_unrealize(DeviceState *dev)
> >>
> >>      if (s->free_page_bh) {
> >>          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);
> >>      }
> >
> > I'm not entirely sure about this order of operations. It seems like it
> > would make more sense to remove the notifier, stop the hinting, delete
> > the bh, and then release the IO thread.
>
> This is the reverse order of the steps in
> virtio_balloon_device_realize(). And I guess it should be fine. The
> notifier cannot really be active/trigger while we are removing devices
> (cannot happen with concurrent migration). After qemu_bh_delete(), the
> iothread is effectively unused.
>
> I am unsure about many things regarding free page hinting (e.g., if the
> virtio_balloon_free_page_stop() is of any use while we are ripping out
> the device and it will be gone in a second).

Agreed. This is probably fine as is.

Reviewed-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>