[PATCH v1 2/2] virtio-balloon: disallow postcopy with VIRTIO_BALLOON_F_FREE_PAGE_HINT

[David Hildenbrand]

Postcopy never worked properly with 'free-page-hint=on', as there are
at least two issues:

1) With postcopy, the guest will never receive a VIRTIO_BALLOON_CMD_ID_DONE
   and consequently won't release free pages back to the OS once
   migration finishes.

   The issue is that for postcopy, we won't do a final bitmap sync while
   the guest is stopped on the source and
   virtio_balloon_free_page_hint_notify() will only call
   virtio_balloon_free_page_done() on the source during
   PRECOPY_NOTIFY_CLEANUP, after the VM state was already migrated to
   the destination.

2) Once the VM touches a page on the destination that has been excluded
   from migration on the source via qemu_guest_free_page_hint() while
   postcopy is active, that thread will stall until postcopy finishes
   and all threads are woken up. (with older Linux kernels that won't
   retry faults when woken up via userfaultfd, we might actually get a
   SEGFAULT)

   The issue is that the source will refuse to migrate any pages that
   are not marked as dirty in the dirty bmap -- for example, because the
   page might just have been sent. Consequently, the faulting thread will
   stall, waiting for the page to be migrated -- which could take quite
   a while and result in guest OS issues.

While we could fix 1), for example, by calling
virtio_balloon_free_page_done() via pre_save callbacks of the
vmstate, 2) is mostly impossible to fix without additional tracking,
such that we can actually identify these hinted pages and handle
them accordingly.

As it never worked properly, let's disable it via the postcopy notifier on
the destination. Trying to set "migrate_set_capability postcopy-ram on"
on the destination now results in "virtio-balloon: 'free-page-hint' does
not support postcopy Error: Postcopy is not supported".

Note 1: We could let qemu_guest_free_page_hint() mark postcopy
        as broken once actually clearing bits on the source. However, it's
        harder to realize as we can race with users starting postcopy
        and we cannot produce an expressive error message easily.

Note 2: virtio-mem has similar issues, however, access to "unplugged"
        memory by the guest is very rare and we would have to be very
        lucky for it to happen during migration. The spec states
        "The driver SHOULD NOT read from unplugged memory blocks ..."
        and "The driver MUST NOT write to unplugged memory blocks".
        virtio-mem will move away from virtio_balloon_free_page_done()
        soon and handle this case explicitly on the destination.

Fixes: c13c4153f76d ("virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT")
Cc: qemu-stable@nongnu.org
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>
Cc: Juan Quintela <quintela@redhat.com>
Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
Cc: Peter Xu <peterx@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/virtio/virtio-balloon.c         | 26 ++++++++++++++++++++++++++
 include/hw/virtio/virtio-balloon.h |  1 +
 2 files changed, 27 insertions(+)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 4b5d9e5e50..d0c9dc677c 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -30,6 +30,7 @@
 #include "trace.h"
 #include "qemu/error-report.h"
 #include "migration/misc.h"
+#include "migration/postcopy-ram.h"
 
 #include "hw/virtio/virtio-bus.h"
 #include "hw/virtio/virtio-access.h"
@@ -692,6 +693,28 @@ virtio_balloon_free_page_hint_notify(NotifierWithReturn 
*n, void *data)
     return 0;
 }
 
+
+static int virtio_balloon_postcopy_notify(NotifierWithReturn *n, void *opaque)
+{
+    VirtIOBalloon *dev = container_of(n, VirtIOBalloon, postcopy_notifier);
+    PostcopyNotifyData *pnd = opaque;
+
+    /* We register the notifier only with 'free-page-hint=on' for now. */
+    g_assert(virtio_has_feature(dev->host_features,
+                                VIRTIO_BALLOON_F_FREE_PAGE_HINT));
+
+    /*
+     * Pages hinted via qemu_guest_free_page_hint() are cleared from the dirty
+     * bitmap and will not get migrated, especially also not when the postcopy
+     * destination starts using them and requests migration from the source; 
the
+     * faulting thread will stall until postcopy migration finishes and
+     * all threads are woken up.
+     */
+    error_setg(pnd->errp,
+               "virtio-balloon: 'free-page-hint' does not support postcopy");
+    return -ENOENT;
+}
+
 static size_t virtio_balloon_config_size(VirtIOBalloon *s)
 {
     uint64_t features = s->host_features;
@@ -911,6 +934,7 @@ static void virtio_balloon_device_realize(DeviceState *dev, 
Error **errp)
         s->free_page_vq = virtio_add_queue(vdev, VIRTQUEUE_MAX_SIZE,
                                            virtio_balloon_handle_free_page_vq);
         precopy_add_notifier(&s->free_page_hint_notify);
+        postcopy_add_notifier(&s->postcopy_notifier);
 
         object_ref(OBJECT(s->iothread));
         s->free_page_bh = aio_bh_new(iothread_get_aio_context(s->iothread),
@@ -935,6 +959,7 @@ static void virtio_balloon_device_unrealize(DeviceState 
*dev)
         object_unref(OBJECT(s->iothread));
         virtio_balloon_free_page_stop(s);
         precopy_remove_notifier(&s->free_page_hint_notify);
+        postcopy_remove_notifier(&s->postcopy_notifier);
     }
     balloon_stats_destroy_timer(s);
     qemu_remove_balloon_handler(s);
@@ -1008,6 +1033,7 @@ static void virtio_balloon_instance_init(Object *obj)
     qemu_cond_init(&s->free_page_cond);
     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;
+    s->postcopy_notifier.notify = virtio_balloon_postcopy_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 5139cf8ab6..d0d5b793b9 100644
--- a/include/hw/virtio/virtio-balloon.h
+++ b/include/hw/virtio/virtio-balloon.h
@@ -65,6 +65,7 @@ struct VirtIOBalloon {
      */
     bool block_iothread;
     NotifierWithReturn free_page_hint_notify;
+    NotifierWithReturn postcopy_notifier;
     int64_t stats_last_update;
     int64_t stats_poll_interval;
     uint32_t host_features;
-- 
2.31.1