[PATCH v1 00/13] migrate/ram: Fix resizing RAM blocks while migrating

[PATCH v1 00/13] migrate/ram: Fix resizing RAM blocks while migrating

[David Hildenbrand]

This is the follow up of
    "[PATCH RFC] memory: Don't allow to resize RAM while migrating" [1]

This series contains some (slightly modified) patches also contained in:
    "[PATCH v2 fixed 00/16] Ram blocks with resizable anonymous allocations
     under POSIX" [2]
That series will be based on this series. The last patch (#13) in this
series could be moved to the other series, but I decided to include it in
here for now (similar context).

I realized that resizing RAM blocks while the guest is being migrated
(precopy: resize while still running on the source, postcopy: resize
 while already running on the target) is buggy. In case of precopy, we
can simply cancel migration. Postcopy handling is more involved. Resizing
can currently happen during a guest reboot, triggered by ACPI rebuilds.

Along with the fixes, some cleanups.

[1] https://lkml.kernel.org/r/20200213172016.196609-1-david@redhat.com
[2] https://lkml.kernel.org/r/20200212134254.11073-1-david@redhat.com

David Hildenbrand (13):
  util: vfio-helpers: Factor out and fix processing of existing ram
    blocks
  stubs/ram-block: Remove stubs that are no longer needed
  numa: Teach ram block notifiers about resizeable ram blocks
  numa: Make all callbacks of ram block notifiers optional
  migrate/ram: Handle RAM block resizes during precopy
  migrate/ram: Discard new RAM when growing RAM blocks and the VM is
    stopped
  migrate/ram: Get rid of "place_source" in ram_load_postcopy()
  migrate/ram: Simplify host page handling in ram_load_postcopy()
  migrate/ram: Consolidate variable reset after placement in
    ram_load_postcopy()
  migrate/ram: Handle RAM block resizes during postcopy
  migrate/multifd: Print used_length of memory block
  migrate/ram: Use offset_in_ramblock() in range checks
  migrate/ram: Tolerate partially changed mappings in postcopy code

 exec.c                     |  23 +++++-
 hw/core/numa.c             |  41 ++++++++++-
 hw/i386/xen/xen-mapcache.c |   7 +-
 include/exec/cpu-common.h  |   1 +
 include/exec/memory.h      |  10 ++-
 include/exec/ramblock.h    |   9 +++
 include/exec/ramlist.h     |  13 +++-
 migration/migration.c      |   9 ++-
 migration/migration.h      |   1 +
 migration/multifd.c        |   2 +-
 migration/postcopy-ram.c   |  58 ++++++++++++---
 migration/ram.c            | 147 +++++++++++++++++++++++++++----------
 stubs/ram-block.c          |  20 -----
 target/i386/hax-mem.c      |   5 +-
 target/i386/sev.c          |  18 +++--
 util/vfio-helpers.c        |  41 ++++-------
 16 files changed, 280 insertions(+), 125 deletions(-)

-- 
2.24.1

[PATCH v1 01/13] util: vfio-helpers: Factor out and fix processing of existing ram blocks

[David Hildenbrand]

Factor it out into common code when a new notifier is registered, just
as done with the memory region notifier. This allows us to have the
logic about how to process existing ram blocks at a central place (which
will be extended soon).

Just like when adding a new ram block, we have to register the max_length
for now. We don't have a way to get notified about resizes yet, and some
memory would not be mapped when growing the ram block.

Note: Currently, ram blocks are only "fake resized". All memory
(max_length) is accessible.

We can get rid of a bunch of functions in stubs/ram-block.c . Print the
warning from inside qemu_vfio_ram_block_added().

Cc: Richard Henderson <rth@twiddle.net>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
Cc: Alex Williamson <alex.williamson@redhat.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Cc: Peter Xu <peterx@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 exec.c                    |  5 +++++
 hw/core/numa.c            | 14 ++++++++++++++
 include/exec/cpu-common.h |  1 +
 util/vfio-helpers.c       | 28 +++++++---------------------
 4 files changed, 27 insertions(+), 21 deletions(-)

diff --git a/exec.c b/exec.c
index 8e9cc3b47c..dfd43d27c6 100644
--- a/exec.c
+++ b/exec.c
@@ -2016,6 +2016,11 @@ ram_addr_t qemu_ram_get_used_length(RAMBlock *rb)
     return rb->used_length;
 }
 
+ram_addr_t qemu_ram_get_max_length(RAMBlock *rb)
+{
+    return rb->max_length;
+}
+
 bool qemu_ram_is_shared(RAMBlock *rb)
 {
     return rb->flags & RAM_SHARED;
diff --git a/hw/core/numa.c b/hw/core/numa.c
index 0d1b4be76a..6599c69e05 100644
--- a/hw/core/numa.c
+++ b/hw/core/numa.c
@@ -899,9 +899,23 @@ void query_numa_node_mem(NumaNodeMem node_mem[], MachineState *ms)
     }
 }
 
+static int ram_block_notify_add_single(RAMBlock *rb, void *opaque)
+{
+    const ram_addr_t max_size = qemu_ram_get_max_length(rb);
+    void *host = qemu_ram_get_host_addr(rb);
+    RAMBlockNotifier *notifier = opaque;
+
+    if (host) {
+        notifier->ram_block_added(notifier, host, max_size);
+    }
+    return 0;
+}
+
 void ram_block_notifier_add(RAMBlockNotifier *n)
 {
     QLIST_INSERT_HEAD(&ram_list.ramblock_notifiers, n, next);
+    /* Notify about all existing ram blocks. */
+    qemu_ram_foreach_block(ram_block_notify_add_single, n);
 }
 
 void ram_block_notifier_remove(RAMBlockNotifier *n)
diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
index 81753bbb34..9760ac9068 100644
--- a/include/exec/cpu-common.h
+++ b/include/exec/cpu-common.h
@@ -59,6 +59,7 @@ const char *qemu_ram_get_idstr(RAMBlock *rb);
 void *qemu_ram_get_host_addr(RAMBlock *rb);
 ram_addr_t qemu_ram_get_offset(RAMBlock *rb);
 ram_addr_t qemu_ram_get_used_length(RAMBlock *rb);
+ram_addr_t qemu_ram_get_max_length(RAMBlock *rb);
 bool qemu_ram_is_shared(RAMBlock *rb);
 bool qemu_ram_is_uf_zeroable(RAMBlock *rb);
 void qemu_ram_set_uf_zeroable(RAMBlock *rb);
diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
index 813f7ec564..71e02e7f35 100644
--- a/util/vfio-helpers.c
+++ b/util/vfio-helpers.c
@@ -376,8 +376,13 @@ static void qemu_vfio_ram_block_added(RAMBlockNotifier *n,
                                       void *host, size_t size)
 {
     QEMUVFIOState *s = container_of(n, QEMUVFIOState, ram_notifier);
+    int ret;
+
     trace_qemu_vfio_ram_block_added(s, host, size);
-    qemu_vfio_dma_map(s, host, size, false, NULL);
+    ret = qemu_vfio_dma_map(s, host, size, false, NULL);
+    if (ret) {
+        error_report("qemu_vfio_dma_map(%p, %zu) failed: %d", host, size, ret);
+    }
 }
 
 static void qemu_vfio_ram_block_removed(RAMBlockNotifier *n,
@@ -390,33 +395,14 @@ static void qemu_vfio_ram_block_removed(RAMBlockNotifier *n,
     }
 }
 
-static int qemu_vfio_init_ramblock(RAMBlock *rb, void *opaque)
-{
-    void *host_addr = qemu_ram_get_host_addr(rb);
-    ram_addr_t length = qemu_ram_get_used_length(rb);
-    int ret;
-    QEMUVFIOState *s = opaque;
-
-    if (!host_addr) {
-        return 0;
-    }
-    ret = qemu_vfio_dma_map(s, host_addr, length, false, NULL);
-    if (ret) {
-        fprintf(stderr, "qemu_vfio_init_ramblock: failed %p %" PRId64 "\n",
-                host_addr, (uint64_t)length);
-    }
-    return 0;
-}
-
 static void qemu_vfio_open_common(QEMUVFIOState *s)
 {
     qemu_mutex_init(&s->lock);
     s->ram_notifier.ram_block_added = qemu_vfio_ram_block_added;
     s->ram_notifier.ram_block_removed = qemu_vfio_ram_block_removed;
-    ram_block_notifier_add(&s->ram_notifier);
     s->low_water_mark = QEMU_VFIO_IOVA_MIN;
     s->high_water_mark = QEMU_VFIO_IOVA_MAX;
-    qemu_ram_foreach_block(qemu_vfio_init_ramblock, s);
+    ram_block_notifier_add(&s->ram_notifier);
 }
 
 /**
-- 
2.24.1

Re: [PATCH v1 01/13] util: vfio-helpers: Factor out and fix processing of existing ram blocks

[Peter Xu]

On Wed, Feb 19, 2020 at 05:17:13PM +0100, David Hildenbrand wrote:
> Factor it out into common code when a new notifier is registered, just
> as done with the memory region notifier. This allows us to have the
> logic about how to process existing ram blocks at a central place (which
> will be extended soon).
> 
> Just like when adding a new ram block, we have to register the max_length
> for now. We don't have a way to get notified about resizes yet, and some
> memory would not be mapped when growing the ram block.
> 
> Note: Currently, ram blocks are only "fake resized". All memory
> (max_length) is accessible.
> 
> We can get rid of a bunch of functions in stubs/ram-block.c . Print the
> warning from inside qemu_vfio_ram_block_added().
> 
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Cc: Peter Xu <peterx@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

[PATCH v1 02/13] stubs/ram-block: Remove stubs that are no longer needed

[David Hildenbrand]

Current code no longer needs these stubs to compile. Let's just remove
them.

Cc: Richard Henderson <rth@twiddle.net>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: Peter Xu <peterx@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 stubs/ram-block.c | 20 --------------------
 1 file changed, 20 deletions(-)

diff --git a/stubs/ram-block.c b/stubs/ram-block.c
index 73c0a3ee08..10855b52dd 100644
--- a/stubs/ram-block.c
+++ b/stubs/ram-block.c
@@ -2,21 +2,6 @@
 #include "exec/ramlist.h"
 #include "exec/cpu-common.h"
 
-void *qemu_ram_get_host_addr(RAMBlock *rb)
-{
-    return 0;
-}
-
-ram_addr_t qemu_ram_get_offset(RAMBlock *rb)
-{
-    return 0;
-}
-
-ram_addr_t qemu_ram_get_used_length(RAMBlock *rb)
-{
-    return 0;
-}
-
 void ram_block_notifier_add(RAMBlockNotifier *n)
 {
 }
@@ -24,8 +9,3 @@ void ram_block_notifier_add(RAMBlockNotifier *n)
 void ram_block_notifier_remove(RAMBlockNotifier *n)
 {
 }
-
-int qemu_ram_foreach_block(RAMBlockIterFunc func, void *opaque)
-{
-    return 0;
-}
-- 
2.24.1

Re: [PATCH v1 02/13] stubs/ram-block: Remove stubs that are no longer needed

[Peter Xu]

On Wed, Feb 19, 2020 at 05:17:14PM +0100, David Hildenbrand wrote:
> Current code no longer needs these stubs to compile. Let's just remove
> them.
> 
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Cc: Peter Xu <peterx@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

[PATCH v1 03/13] numa: Teach ram block notifiers about resizeable ram blocks

[David Hildenbrand]

Ram block notifiers are currently not aware of resizes. Especially to
handle resizes during migration, but also to implement actually resizeable
ram blocks (make everything between used_length and max_length
inaccessible), we want to teach ram block notifiers about resizeable
ram.

Introduce the basic infrastructure but keep using max_size in the
existing notifiers. Supply the max_size when adding and removing ram
blocks. Also, notify on resizes.

Acked-by: Paul Durrant <paul@xen.org>
Cc: Richard Henderson <rth@twiddle.net>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Paul Durrant <paul@xen.org>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: xen-devel@lists.xenproject.org
Cc: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 exec.c                     | 13 +++++++++++--
 hw/core/numa.c             | 22 +++++++++++++++++-----
 hw/i386/xen/xen-mapcache.c |  7 ++++---
 include/exec/ramlist.h     | 13 +++++++++----
 target/i386/hax-mem.c      |  5 +++--
 target/i386/sev.c          | 18 ++++++++++--------
 util/vfio-helpers.c        | 17 +++++++++--------
 7 files changed, 63 insertions(+), 32 deletions(-)

diff --git a/exec.c b/exec.c
index dfd43d27c6..b75250e773 100644
--- a/exec.c
+++ b/exec.c
@@ -2129,6 +2129,8 @@ static int memory_try_enable_merging(void *addr, size_t len)
  */
 int qemu_ram_resize(RAMBlock *block, ram_addr_t newsize, Error **errp)
 {
+    const ram_addr_t oldsize = block->used_length;
+
     assert(block);
 
     newsize = HOST_PAGE_ALIGN(newsize);
@@ -2153,6 +2155,11 @@ int qemu_ram_resize(RAMBlock *block, ram_addr_t newsize, Error **errp)
         return -EINVAL;
     }
 
+    /* Notify before modifying the ram block and touching the bitmaps. */
+    if (block->host) {
+        ram_block_notify_resize(block->host, oldsize, newsize);
+    }
+
     cpu_physical_memory_clear_dirty_range(block->offset, block->used_length);
     block->used_length = newsize;
     cpu_physical_memory_set_dirty_range(block->offset, block->used_length,
@@ -2312,7 +2319,8 @@ static void ram_block_add(RAMBlock *new_block, Error **errp, bool shared)
         qemu_madvise(new_block->host, new_block->max_length, QEMU_MADV_HUGEPAGE);
         /* MADV_DONTFORK is also needed by KVM in absence of synchronous MMU */
         qemu_madvise(new_block->host, new_block->max_length, QEMU_MADV_DONTFORK);
-        ram_block_notify_add(new_block->host, new_block->max_length);
+        ram_block_notify_add(new_block->host, new_block->used_length,
+                             new_block->max_length);
     }
 }
 
@@ -2492,7 +2500,8 @@ void qemu_ram_free(RAMBlock *block)
     }
 
     if (block->host) {
-        ram_block_notify_remove(block->host, block->max_length);
+        ram_block_notify_remove(block->host, block->used_length,
+                                block->max_length);
     }
 
     qemu_mutex_lock_ramlist();
diff --git a/hw/core/numa.c b/hw/core/numa.c
index 6599c69e05..e28ad24fcd 100644
--- a/hw/core/numa.c
+++ b/hw/core/numa.c
@@ -902,11 +902,12 @@ void query_numa_node_mem(NumaNodeMem node_mem[], MachineState *ms)
 static int ram_block_notify_add_single(RAMBlock *rb, void *opaque)
 {
     const ram_addr_t max_size = qemu_ram_get_max_length(rb);
+    const ram_addr_t size = qemu_ram_get_used_length(rb);
     void *host = qemu_ram_get_host_addr(rb);
     RAMBlockNotifier *notifier = opaque;
 
     if (host) {
-        notifier->ram_block_added(notifier, host, max_size);
+        notifier->ram_block_added(notifier, host, size, max_size);
     }
     return 0;
 }
@@ -923,20 +924,31 @@ void ram_block_notifier_remove(RAMBlockNotifier *n)
     QLIST_REMOVE(n, next);
 }
 
-void ram_block_notify_add(void *host, size_t size)
+void ram_block_notify_add(void *host, size_t size, size_t max_size)
 {
     RAMBlockNotifier *notifier;
 
     QLIST_FOREACH(notifier, &ram_list.ramblock_notifiers, next) {
-        notifier->ram_block_added(notifier, host, size);
+        notifier->ram_block_added(notifier, host, size, max_size);
     }
 }
 
-void ram_block_notify_remove(void *host, size_t size)
+void ram_block_notify_remove(void *host, size_t size, size_t max_size)
 {
     RAMBlockNotifier *notifier;
 
     QLIST_FOREACH(notifier, &ram_list.ramblock_notifiers, next) {
-        notifier->ram_block_removed(notifier, host, size);
+        notifier->ram_block_removed(notifier, host, size, max_size);
+    }
+}
+
+void ram_block_notify_resize(void *host, size_t old_size, size_t new_size)
+{
+    RAMBlockNotifier *notifier;
+
+    QLIST_FOREACH(notifier, &ram_list.ramblock_notifiers, next) {
+        if (notifier->ram_block_resized) {
+            notifier->ram_block_resized(notifier, host, old_size, new_size);
+        }
     }
 }
diff --git a/hw/i386/xen/xen-mapcache.c b/hw/i386/xen/xen-mapcache.c
index 5b120ed44b..d6dcea65d1 100644
--- a/hw/i386/xen/xen-mapcache.c
+++ b/hw/i386/xen/xen-mapcache.c
@@ -169,7 +169,8 @@ static void xen_remap_bucket(MapCacheEntry *entry,
 
     if (entry->vaddr_base != NULL) {
         if (!(entry->flags & XEN_MAPCACHE_ENTRY_DUMMY)) {
-            ram_block_notify_remove(entry->vaddr_base, entry->size);
+            ram_block_notify_remove(entry->vaddr_base, entry->size,
+                                    entry->size);
         }
         if (munmap(entry->vaddr_base, entry->size) != 0) {
             perror("unmap fails");
@@ -211,7 +212,7 @@ static void xen_remap_bucket(MapCacheEntry *entry,
     }
 
     if (!(entry->flags & XEN_MAPCACHE_ENTRY_DUMMY)) {
-        ram_block_notify_add(vaddr_base, size);
+        ram_block_notify_add(vaddr_base, size, size);
     }
 
     entry->vaddr_base = vaddr_base;
@@ -452,7 +453,7 @@ static void xen_invalidate_map_cache_entry_unlocked(uint8_t *buffer)
     }
 
     pentry->next = entry->next;
-    ram_block_notify_remove(entry->vaddr_base, entry->size);
+    ram_block_notify_remove(entry->vaddr_base, entry->size, entry->size);
     if (munmap(entry->vaddr_base, entry->size) != 0) {
         perror("unmap fails");
         exit(-1);
diff --git a/include/exec/ramlist.h b/include/exec/ramlist.h
index bc4faa1b00..293c0ddabe 100644
--- a/include/exec/ramlist.h
+++ b/include/exec/ramlist.h
@@ -65,15 +65,20 @@ void qemu_mutex_lock_ramlist(void);
 void qemu_mutex_unlock_ramlist(void);
 
 struct RAMBlockNotifier {
-    void (*ram_block_added)(RAMBlockNotifier *n, void *host, size_t size);
-    void (*ram_block_removed)(RAMBlockNotifier *n, void *host, size_t size);
+    void (*ram_block_added)(RAMBlockNotifier *n, void *host, size_t size,
+                            size_t max_size);
+    void (*ram_block_removed)(RAMBlockNotifier *n, void *host, size_t size,
+                              size_t max_size);
+    void (*ram_block_resized)(RAMBlockNotifier *n, void *host, size_t old_size,
+                              size_t new_size);
     QLIST_ENTRY(RAMBlockNotifier) next;
 };
 
 void ram_block_notifier_add(RAMBlockNotifier *n);
 void ram_block_notifier_remove(RAMBlockNotifier *n);
-void ram_block_notify_add(void *host, size_t size);
-void ram_block_notify_remove(void *host, size_t size);
+void ram_block_notify_add(void *host, size_t size, size_t max_size);
+void ram_block_notify_remove(void *host, size_t size, size_t max_size);
+void ram_block_notify_resize(void *host, size_t old_size, size_t new_size);
 
 void ram_block_dump(Monitor *mon);
 
diff --git a/target/i386/hax-mem.c b/target/i386/hax-mem.c
index 6bb5a24917..454d7fb212 100644
--- a/target/i386/hax-mem.c
+++ b/target/i386/hax-mem.c
@@ -293,7 +293,8 @@ static MemoryListener hax_memory_listener = {
     .priority = 10,
 };
 
-static void hax_ram_block_added(RAMBlockNotifier *n, void *host, size_t size)
+static void hax_ram_block_added(RAMBlockNotifier *n, void *host, size_t size,
+                                size_t max_size)
 {
     /*
      * We must register each RAM block with the HAXM kernel module, or
@@ -304,7 +305,7 @@ static void hax_ram_block_added(RAMBlockNotifier *n, void *host, size_t size)
      * host physical pages for the RAM block as part of this registration
      * process, hence the name hax_populate_ram().
      */
-    if (hax_populate_ram((uint64_t)(uintptr_t)host, size) < 0) {
+    if (hax_populate_ram((uint64_t)(uintptr_t)host, max_size) < 0) {
         fprintf(stderr, "HAX failed to populate RAM\n");
         abort();
     }
diff --git a/target/i386/sev.c b/target/i386/sev.c
index 024bb24e51..6b4cee24a2 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -129,7 +129,8 @@ sev_set_guest_state(SevState new_state)
 }
 
 static void
-sev_ram_block_added(RAMBlockNotifier *n, void *host, size_t size)
+sev_ram_block_added(RAMBlockNotifier *n, void *host, size_t size,
+                    size_t max_size)
 {
     int r;
     struct kvm_enc_region range;
@@ -146,19 +147,20 @@ sev_ram_block_added(RAMBlockNotifier *n, void *host, size_t size)
     }
 
     range.addr = (__u64)(unsigned long)host;
-    range.size = size;
+    range.size = max_size;
 
-    trace_kvm_memcrypt_register_region(host, size);
+    trace_kvm_memcrypt_register_region(host, max_size);
     r = kvm_vm_ioctl(kvm_state, KVM_MEMORY_ENCRYPT_REG_REGION, &range);
     if (r) {
         error_report("%s: failed to register region (%p+%#zx) error '%s'",
-                     __func__, host, size, strerror(errno));
+                     __func__, host, max_size, strerror(errno));
         exit(1);
     }
 }
 
 static void
-sev_ram_block_removed(RAMBlockNotifier *n, void *host, size_t size)
+sev_ram_block_removed(RAMBlockNotifier *n, void *host, size_t size,
+                      size_t max_size)
 {
     int r;
     struct kvm_enc_region range;
@@ -175,13 +177,13 @@ sev_ram_block_removed(RAMBlockNotifier *n, void *host, size_t size)
     }
 
     range.addr = (__u64)(unsigned long)host;
-    range.size = size;
+    range.size = max_size;
 
-    trace_kvm_memcrypt_unregister_region(host, size);
+    trace_kvm_memcrypt_unregister_region(host, max_size);
     r = kvm_vm_ioctl(kvm_state, KVM_MEMORY_ENCRYPT_UNREG_REGION, &range);
     if (r) {
         error_report("%s: failed to unregister region (%p+%#zx)",
-                     __func__, host, size);
+                     __func__, host, max_size);
     }
 }
 
diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
index 71e02e7f35..61a0cf6e12 100644
--- a/util/vfio-helpers.c
+++ b/util/vfio-helpers.c
@@ -372,25 +372,26 @@ fail_container:
     return ret;
 }
 
-static void qemu_vfio_ram_block_added(RAMBlockNotifier *n,
-                                      void *host, size_t size)
+static void qemu_vfio_ram_block_added(RAMBlockNotifier *n, void *host,
+                                      size_t size, size_t max_size)
 {
     QEMUVFIOState *s = container_of(n, QEMUVFIOState, ram_notifier);
     int ret;
 
-    trace_qemu_vfio_ram_block_added(s, host, size);
-    ret = qemu_vfio_dma_map(s, host, size, false, NULL);
+    trace_qemu_vfio_ram_block_added(s, host, max_size);
+    ret = qemu_vfio_dma_map(s, host, max_size, false, NULL);
     if (ret) {
-        error_report("qemu_vfio_dma_map(%p, %zu) failed: %d", host, size, ret);
+        error_report("qemu_vfio_dma_map(%p, %zu) failed: %d", host,
+                     max_size, ret);
     }
 }
 
-static void qemu_vfio_ram_block_removed(RAMBlockNotifier *n,
-                                        void *host, size_t size)
+static void qemu_vfio_ram_block_removed(RAMBlockNotifier *n, void *host,
+                                        size_t size, size_t max_size)
 {
     QEMUVFIOState *s = container_of(n, QEMUVFIOState, ram_notifier);
     if (host) {
-        trace_qemu_vfio_ram_block_removed(s, host, size);
+        trace_qemu_vfio_ram_block_removed(s, host, max_size);
         qemu_vfio_dma_unmap(s, host);
     }
 }
-- 
2.24.1

Re: [PATCH v1 03/13] numa: Teach ram block notifiers about resizeable ram blocks

[Peter Xu]

On Wed, Feb 19, 2020 at 05:17:15PM +0100, David Hildenbrand wrote:
> Ram block notifiers are currently not aware of resizes. Especially to
> handle resizes during migration, but also to implement actually resizeable
> ram blocks (make everything between used_length and max_length
> inaccessible), we want to teach ram block notifiers about resizeable
> ram.
> 
> Introduce the basic infrastructure but keep using max_size in the
> existing notifiers. Supply the max_size when adding and removing ram
> blocks. Also, notify on resizes.
> 
> Acked-by: Paul Durrant <paul@xen.org>
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Paul Durrant <paul@xen.org>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: xen-devel@lists.xenproject.org
> Cc: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

[PATCH v1 04/13] numa: Make all callbacks of ram block notifiers optional

[David Hildenbrand]

Let's make add/remove optional. We want to introduce a RAM block
notifier for RAM migration, that's only interested in resizes.

Cc: Richard Henderson <rth@twiddle.net>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
Cc: Peter Xu <peterx@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/core/numa.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/hw/core/numa.c b/hw/core/numa.c
index e28ad24fcd..4270b268c8 100644
--- a/hw/core/numa.c
+++ b/hw/core/numa.c
@@ -915,8 +915,11 @@ static int ram_block_notify_add_single(RAMBlock *rb, void *opaque)
 void ram_block_notifier_add(RAMBlockNotifier *n)
 {
     QLIST_INSERT_HEAD(&ram_list.ramblock_notifiers, n, next);
+
     /* Notify about all existing ram blocks. */
-    qemu_ram_foreach_block(ram_block_notify_add_single, n);
+    if (n->ram_block_added) {
+        qemu_ram_foreach_block(ram_block_notify_add_single, n);
+    }
 }
 
 void ram_block_notifier_remove(RAMBlockNotifier *n)
@@ -929,7 +932,9 @@ void ram_block_notify_add(void *host, size_t size, size_t max_size)
     RAMBlockNotifier *notifier;
 
     QLIST_FOREACH(notifier, &ram_list.ramblock_notifiers, next) {
-        notifier->ram_block_added(notifier, host, size, max_size);
+        if (notifier->ram_block_added) {
+            notifier->ram_block_added(notifier, host, size, max_size);
+        }
     }
 }
 
@@ -938,7 +943,9 @@ void ram_block_notify_remove(void *host, size_t size, size_t max_size)
     RAMBlockNotifier *notifier;
 
     QLIST_FOREACH(notifier, &ram_list.ramblock_notifiers, next) {
-        notifier->ram_block_removed(notifier, host, size, max_size);
+        if (notifier->ram_block_removed) {
+            notifier->ram_block_removed(notifier, host, size, max_size);
+        }
     }
 }
 
-- 
2.24.1

Re: [PATCH v1 04/13] numa: Make all callbacks of ram block notifiers optional

[Peter Xu]

On Wed, Feb 19, 2020 at 05:17:16PM +0100, David Hildenbrand wrote:
> Let's make add/remove optional. We want to introduce a RAM block
> notifier for RAM migration, that's only interested in resizes.
> 
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> Cc: Peter Xu <peterx@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

[PATCH v1 05/13] migrate/ram: Handle RAM block resizes during precopy

[David Hildenbrand]

Resizing while migrating is dangerous and does not work as expected.
The whole migration code works on the usable_length of ram blocks and does
not expect this to change at random points in time.

In the case of precopy, the ram block size must not change on the source,
after syncing the RAM block list in ram_save_setup(), so as long as the
guest is still running on the source.

Resizing can be trigger *after* (but not during) a reset in
ACPI code by the guest
- hw/arm/virt-acpi-build.c:acpi_ram_update()
- hw/i386/acpi-build.c:acpi_ram_update()

Use the ram block notifier to get notified about resizes. Let's simply
cancel migration and indicate the reason. We'll continue running on the
source. No harm done.

Update the documentation. Postcopy will be handled separately.

Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
Cc: Juan Quintela <quintela@redhat.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Richard Henderson <richard.henderson@linaro.org>
Cc: Shannon Zhao <shannon.zhao@linaro.org>
Cc: Alex Bennée <alex.bennee@linaro.org>
Cc: Peter Xu <peterx@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 exec.c                |  5 +++--
 include/exec/memory.h | 10 ++++++----
 migration/migration.c |  9 +++++++--
 migration/migration.h |  1 +
 migration/ram.c       | 41 +++++++++++++++++++++++++++++++++++++++++
 5 files changed, 58 insertions(+), 8 deletions(-)

diff --git a/exec.c b/exec.c
index b75250e773..8b015821d6 100644
--- a/exec.c
+++ b/exec.c
@@ -2120,8 +2120,9 @@ static int memory_try_enable_merging(void *addr, size_t len)
     return qemu_madvise(addr, len, QEMU_MADV_MERGEABLE);
 }
 
-/* Only legal before guest might have detected the memory size: e.g. on
- * incoming migration, or right after reset.
+/*
+ * Resizing RAM while migrating can result in the migration being canceled.
+ * Care has to be taken if the guest might have already detected the memory.
  *
  * As memory core doesn't know how is memory accessed, it is up to
  * resize callback to update device state and/or add assertions to detect
diff --git a/include/exec/memory.h b/include/exec/memory.h
index e85b7de99a..de111347e8 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -113,7 +113,7 @@ typedef struct IOMMUNotifier IOMMUNotifier;
 #define RAM_SHARED     (1 << 1)
 
 /* Only a portion of RAM (used_length) is actually used, and migrated.
- * This used_length size can change across reboots.
+ * Resizing RAM while migrating can result in the migration being canceled.
  */
 #define RAM_RESIZEABLE (1 << 2)
 
@@ -843,7 +843,9 @@ void memory_region_init_ram_shared_nomigrate(MemoryRegion *mr,
  *                                     RAM.  Accesses into the region will
  *                                     modify memory directly.  Only an initial
  *                                     portion of this RAM is actually used.
- *                                     The used size can change across reboots.
+ *                                     Changing the size while migrating
+ *                                     can result in the migration being
+ *                                     canceled.
  *
  * @mr: the #MemoryRegion to be initialized.
  * @owner: the object that tracks the region's reference count
@@ -1464,8 +1466,8 @@ void *memory_region_get_ram_ptr(MemoryRegion *mr);
 
 /* memory_region_ram_resize: Resize a RAM region.
  *
- * Only legal before guest might have detected the memory size: e.g. on
- * incoming migration, or right after reset.
+ * Resizing RAM while migrating can result in the migration being canceled.
+ * Care has to be taken if the guest might have already detected the memory.
  *
  * @mr: a memory region created with @memory_region_init_resizeable_ram.
  * @newsize: the new size the region
diff --git a/migration/migration.c b/migration/migration.c
index 8fb68795dc..ac9751dbe5 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -175,13 +175,18 @@ void migration_object_init(void)
     }
 }
 
+void migration_cancel(void)
+{
+    migrate_fd_cancel(current_migration);
+}
+
 void migration_shutdown(void)
 {
     /*
      * Cancel the current migration - that will (eventually)
      * stop the migration using this structure
      */
-    migrate_fd_cancel(current_migration);
+    migration_cancel();
     object_unref(OBJECT(current_migration));
 }
 
@@ -2019,7 +2024,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
 
 void qmp_migrate_cancel(Error **errp)
 {
-    migrate_fd_cancel(migrate_get_current());
+    migration_cancel();
 }
 
 void qmp_migrate_continue(MigrationStatus state, Error **errp)
diff --git a/migration/migration.h b/migration/migration.h
index 8473ddfc88..79fd74afa5 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -343,5 +343,6 @@ int foreach_not_ignored_block(RAMBlockIterFunc func, void *opaque);
 void migration_make_urgent_request(void);
 void migration_consume_urgent_request(void);
 bool migration_rate_limit(void);
+void migration_cancel(void);
 
 #endif
diff --git a/migration/ram.c b/migration/ram.c
index ed23ed1c7c..57f32011a3 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -52,6 +52,7 @@
 #include "migration/colo.h"
 #include "block.h"
 #include "sysemu/sysemu.h"
+#include "sysemu/runstate.h"
 #include "savevm.h"
 #include "qemu/iov.h"
 #include "multifd.h"
@@ -3710,8 +3711,48 @@ static SaveVMHandlers savevm_ram_handlers = {
     .resume_prepare = ram_resume_prepare,
 };
 
+static void ram_mig_ram_block_resized(RAMBlockNotifier *n, void *host,
+                                      size_t old_size, size_t new_size)
+{
+    ram_addr_t offset;
+    Error *err = NULL;
+    RAMBlock *rb = qemu_ram_block_from_host(host, false, &offset);
+
+    if (ramblock_is_ignored(rb)) {
+        return;
+    }
+
+    /*
+     * Some resizes are triggered on the migration target by precopy code,
+     * when synchronizing RAM block sizes. In these cases, the VM is not
+     * running and migration is not idle. We have to ignore these resizes,
+     * as we only care about resizes during precopy on the migration source.
+     * This handler is always registered, so ignore when migration is idle.
+     */
+    if (migration_is_idle() || !runstate_is_running() ||
+        postcopy_is_running()) {
+        return;
+    }
+
+    /*
+     * Precopy code cannot deal with the size of ram blocks changing at
+     * random points in time. We're still running on the source, abort
+     * the migration and continue running here. Make sure to wait until
+     * migration was canceled.
+     */
+    error_setg(&err, "RAM block '%s' resized during precopy.", rb->idstr);
+    migrate_set_error(migrate_get_current(), err);
+    error_free(err);
+    migration_cancel();
+}
+
+static RAMBlockNotifier ram_mig_ram_notifier = {
+    .ram_block_resized = ram_mig_ram_block_resized,
+};
+
 void ram_mig_init(void)
 {
     qemu_mutex_init(&XBZRLE.lock);
     register_savevm_live("ram", 0, 4, &savevm_ram_handlers, &ram_state);
+    ram_block_notifier_add(&ram_mig_ram_notifier);
 }
-- 
2.24.1

Re: [PATCH v1 05/13] migrate/ram: Handle RAM block resizes during precopy

[David Hildenbrand]

On 19.02.20 17:17, David Hildenbrand wrote:
> Resizing while migrating is dangerous and does not work as expected.
> The whole migration code works on the usable_length of ram blocks and does
> not expect this to change at random points in time.
> 
> In the case of precopy, the ram block size must not change on the source,
> after syncing the RAM block list in ram_save_setup(), so as long as the
> guest is still running on the source.
> 
> Resizing can be trigger *after* (but not during) a reset in
> ACPI code by the guest
> - hw/arm/virt-acpi-build.c:acpi_ram_update()
> - hw/i386/acpi-build.c:acpi_ram_update()
> 
> Use the ram block notifier to get notified about resizes. Let's simply
> cancel migration and indicate the reason. We'll continue running on the
> source. No harm done.
> 
> Update the documentation. Postcopy will be handled separately.
> 
> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> Cc: Juan Quintela <quintela@redhat.com>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Richard Henderson <richard.henderson@linaro.org>
> Cc: Shannon Zhao <shannon.zhao@linaro.org>
> Cc: Alex Bennée <alex.bennee@linaro.org>
> Cc: Peter Xu <peterx@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  exec.c                |  5 +++--
>  include/exec/memory.h | 10 ++++++----
>  migration/migration.c |  9 +++++++--
>  migration/migration.h |  1 +
>  migration/ram.c       | 41 +++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 58 insertions(+), 8 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index b75250e773..8b015821d6 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2120,8 +2120,9 @@ static int memory_try_enable_merging(void *addr, size_t len)
>      return qemu_madvise(addr, len, QEMU_MADV_MERGEABLE);
>  }
>  
> -/* Only legal before guest might have detected the memory size: e.g. on
> - * incoming migration, or right after reset.
> +/*
> + * Resizing RAM while migrating can result in the migration being canceled.
> + * Care has to be taken if the guest might have already detected the memory.
>   *
>   * As memory core doesn't know how is memory accessed, it is up to
>   * resize callback to update device state and/or add assertions to detect
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index e85b7de99a..de111347e8 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -113,7 +113,7 @@ typedef struct IOMMUNotifier IOMMUNotifier;
>  #define RAM_SHARED     (1 << 1)
>  
>  /* Only a portion of RAM (used_length) is actually used, and migrated.
> - * This used_length size can change across reboots.
> + * Resizing RAM while migrating can result in the migration being canceled.
>   */
>  #define RAM_RESIZEABLE (1 << 2)
>  
> @@ -843,7 +843,9 @@ void memory_region_init_ram_shared_nomigrate(MemoryRegion *mr,
>   *                                     RAM.  Accesses into the region will
>   *                                     modify memory directly.  Only an initial
>   *                                     portion of this RAM is actually used.
> - *                                     The used size can change across reboots.
> + *                                     Changing the size while migrating
> + *                                     can result in the migration being
> + *                                     canceled.
>   *
>   * @mr: the #MemoryRegion to be initialized.
>   * @owner: the object that tracks the region's reference count
> @@ -1464,8 +1466,8 @@ void *memory_region_get_ram_ptr(MemoryRegion *mr);
>  
>  /* memory_region_ram_resize: Resize a RAM region.
>   *
> - * Only legal before guest might have detected the memory size: e.g. on
> - * incoming migration, or right after reset.
> + * Resizing RAM while migrating can result in the migration being canceled.
> + * Care has to be taken if the guest might have already detected the memory.
>   *
>   * @mr: a memory region created with @memory_region_init_resizeable_ram.
>   * @newsize: the new size the region
> diff --git a/migration/migration.c b/migration/migration.c
> index 8fb68795dc..ac9751dbe5 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -175,13 +175,18 @@ void migration_object_init(void)
>      }
>  }
>  
> +void migration_cancel(void)
> +{
> +    migrate_fd_cancel(current_migration);
> +}
> +
>  void migration_shutdown(void)
>  {
>      /*
>       * Cancel the current migration - that will (eventually)
>       * stop the migration using this structure
>       */
> -    migrate_fd_cancel(current_migration);
> +    migration_cancel();
>      object_unref(OBJECT(current_migration));
>  }
>  
> @@ -2019,7 +2024,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>  
>  void qmp_migrate_cancel(Error **errp)
>  {
> -    migrate_fd_cancel(migrate_get_current());
> +    migration_cancel();
>  }
>  
>  void qmp_migrate_continue(MigrationStatus state, Error **errp)
> diff --git a/migration/migration.h b/migration/migration.h
> index 8473ddfc88..79fd74afa5 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -343,5 +343,6 @@ int foreach_not_ignored_block(RAMBlockIterFunc func, void *opaque);
>  void migration_make_urgent_request(void);
>  void migration_consume_urgent_request(void);
>  bool migration_rate_limit(void);
> +void migration_cancel(void);
>  
>  #endif
> diff --git a/migration/ram.c b/migration/ram.c
> index ed23ed1c7c..57f32011a3 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -52,6 +52,7 @@
>  #include "migration/colo.h"
>  #include "block.h"
>  #include "sysemu/sysemu.h"
> +#include "sysemu/runstate.h"
>  #include "savevm.h"
>  #include "qemu/iov.h"
>  #include "multifd.h"
> @@ -3710,8 +3711,48 @@ static SaveVMHandlers savevm_ram_handlers = {
>      .resume_prepare = ram_resume_prepare,
>  };
>  
> +static void ram_mig_ram_block_resized(RAMBlockNotifier *n, void *host,
> +                                      size_t old_size, size_t new_size)
> +{
> +    ram_addr_t offset;
> +    Error *err = NULL;
> +    RAMBlock *rb = qemu_ram_block_from_host(host, false, &offset);
> +
> +    if (ramblock_is_ignored(rb)) {
> +        return;
> +    }
> +
> +    /*
> +     * Some resizes are triggered on the migration target by precopy code,
> +     * when synchronizing RAM block sizes. In these cases, the VM is not
> +     * running and migration is not idle. We have to ignore these resizes,
> +     * as we only care about resizes during precopy on the migration source.
> +     * This handler is always registered, so ignore when migration is idle.
> +     */
> +    if (migration_is_idle() || !runstate_is_running() ||
> +        postcopy_is_running()) {
> +        return;
> +    }
> +
> +    /*
> +     * Precopy code cannot deal with the size of ram blocks changing at
> +     * random points in time. We're still running on the source, abort
> +     * the migration and continue running here. Make sure to wait until
> +     * migration was canceled.
> +     */
> +    error_setg(&err, "RAM block '%s' resized during precopy.", rb->idstr);
> +    migrate_set_error(migrate_get_current(), err);
> +    error_free(err);
> +    migration_cancel();
> +}
> +
> +static RAMBlockNotifier ram_mig_ram_notifier = {
> +    .ram_block_resized = ram_mig_ram_block_resized,
> +};
> +
>  void ram_mig_init(void)
>  {
>      qemu_mutex_init(&XBZRLE.lock);
>      register_savevm_live("ram", 0, 4, &savevm_ram_handlers, &ram_state);
> +    ram_block_notifier_add(&ram_mig_ram_notifier);
>  }
> 

So, this seems to work very reliably when triggering a resize of a RAM
block during system reset (using my virtio-mem prototype):

(qemu) info migrate
globals:
store-global-state: on
only-migratable: off
send-configuration: on
send-section-footer: on
decompress-error-check: on
clear-bitmap-shift: 18
Migration status: cancelled
total time: 0 milliseconds


And from QEMU

qemu-system-x86_64: RAM block '0000:00:03.0/mem1' resized during precopy.

-- 
Thanks,

David / dhildenb

Re: [PATCH v1 05/13] migrate/ram: Handle RAM block resizes during precopy

[Peter Xu]

On Thu, Feb 20, 2020 at 04:16:02PM +0100, David Hildenbrand wrote:
> On 19.02.20 17:17, David Hildenbrand wrote:
> > Resizing while migrating is dangerous and does not work as expected.
> > The whole migration code works on the usable_length of ram blocks and does
> > not expect this to change at random points in time.
> > 
> > In the case of precopy, the ram block size must not change on the source,
> > after syncing the RAM block list in ram_save_setup(), so as long as the
> > guest is still running on the source.
> > 
> > Resizing can be trigger *after* (but not during) a reset in
> > ACPI code by the guest
> > - hw/arm/virt-acpi-build.c:acpi_ram_update()
> > - hw/i386/acpi-build.c:acpi_ram_update()
> > 
> > Use the ram block notifier to get notified about resizes. Let's simply
> > cancel migration and indicate the reason. We'll continue running on the
> > source. No harm done.
> > 
> > Update the documentation. Postcopy will be handled separately.
> > 
> > Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > Cc: Juan Quintela <quintela@redhat.com>
> > Cc: Eduardo Habkost <ehabkost@redhat.com>
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: Igor Mammedov <imammedo@redhat.com>
> > Cc: "Michael S. Tsirkin" <mst@redhat.com>
> > Cc: Richard Henderson <richard.henderson@linaro.org>
> > Cc: Shannon Zhao <shannon.zhao@linaro.org>
> > Cc: Alex Bennée <alex.bennee@linaro.org>
> > Cc: Peter Xu <peterx@redhat.com>
> > Signed-off-by: David Hildenbrand <david@redhat.com>
> > ---
> >  exec.c                |  5 +++--
> >  include/exec/memory.h | 10 ++++++----
> >  migration/migration.c |  9 +++++++--
> >  migration/migration.h |  1 +
> >  migration/ram.c       | 41 +++++++++++++++++++++++++++++++++++++++++
> >  5 files changed, 58 insertions(+), 8 deletions(-)
> > 
> > diff --git a/exec.c b/exec.c
> > index b75250e773..8b015821d6 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -2120,8 +2120,9 @@ static int memory_try_enable_merging(void *addr, size_t len)
> >      return qemu_madvise(addr, len, QEMU_MADV_MERGEABLE);
> >  }
> >  
> > -/* Only legal before guest might have detected the memory size: e.g. on
> > - * incoming migration, or right after reset.
> > +/*
> > + * Resizing RAM while migrating can result in the migration being canceled.
> > + * Care has to be taken if the guest might have already detected the memory.
> >   *
> >   * As memory core doesn't know how is memory accessed, it is up to
> >   * resize callback to update device state and/or add assertions to detect
> > diff --git a/include/exec/memory.h b/include/exec/memory.h
> > index e85b7de99a..de111347e8 100644
> > --- a/include/exec/memory.h
> > +++ b/include/exec/memory.h
> > @@ -113,7 +113,7 @@ typedef struct IOMMUNotifier IOMMUNotifier;
> >  #define RAM_SHARED     (1 << 1)
> >  
> >  /* Only a portion of RAM (used_length) is actually used, and migrated.
> > - * This used_length size can change across reboots.
> > + * Resizing RAM while migrating can result in the migration being canceled.
> >   */
> >  #define RAM_RESIZEABLE (1 << 2)
> >  
> > @@ -843,7 +843,9 @@ void memory_region_init_ram_shared_nomigrate(MemoryRegion *mr,
> >   *                                     RAM.  Accesses into the region will
> >   *                                     modify memory directly.  Only an initial
> >   *                                     portion of this RAM is actually used.
> > - *                                     The used size can change across reboots.
> > + *                                     Changing the size while migrating
> > + *                                     can result in the migration being
> > + *                                     canceled.
> >   *
> >   * @mr: the #MemoryRegion to be initialized.
> >   * @owner: the object that tracks the region's reference count
> > @@ -1464,8 +1466,8 @@ void *memory_region_get_ram_ptr(MemoryRegion *mr);
> >  
> >  /* memory_region_ram_resize: Resize a RAM region.
> >   *
> > - * Only legal before guest might have detected the memory size: e.g. on
> > - * incoming migration, or right after reset.
> > + * Resizing RAM while migrating can result in the migration being canceled.
> > + * Care has to be taken if the guest might have already detected the memory.
> >   *
> >   * @mr: a memory region created with @memory_region_init_resizeable_ram.
> >   * @newsize: the new size the region
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 8fb68795dc..ac9751dbe5 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -175,13 +175,18 @@ void migration_object_init(void)
> >      }
> >  }
> >  
> > +void migration_cancel(void)
> > +{
> > +    migrate_fd_cancel(current_migration);
> > +}
> > +
> >  void migration_shutdown(void)
> >  {
> >      /*
> >       * Cancel the current migration - that will (eventually)
> >       * stop the migration using this structure
> >       */
> > -    migrate_fd_cancel(current_migration);
> > +    migration_cancel();
> >      object_unref(OBJECT(current_migration));
> >  }
> >  
> > @@ -2019,7 +2024,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
> >  
> >  void qmp_migrate_cancel(Error **errp)
> >  {
> > -    migrate_fd_cancel(migrate_get_current());
> > +    migration_cancel();
> >  }
> >  
> >  void qmp_migrate_continue(MigrationStatus state, Error **errp)
> > diff --git a/migration/migration.h b/migration/migration.h
> > index 8473ddfc88..79fd74afa5 100644
> > --- a/migration/migration.h
> > +++ b/migration/migration.h
> > @@ -343,5 +343,6 @@ int foreach_not_ignored_block(RAMBlockIterFunc func, void *opaque);
> >  void migration_make_urgent_request(void);
> >  void migration_consume_urgent_request(void);
> >  bool migration_rate_limit(void);
> > +void migration_cancel(void);
> >  
> >  #endif
> > diff --git a/migration/ram.c b/migration/ram.c
> > index ed23ed1c7c..57f32011a3 100644
> > --- a/migration/ram.c
> > +++ b/migration/ram.c
> > @@ -52,6 +52,7 @@
> >  #include "migration/colo.h"
> >  #include "block.h"
> >  #include "sysemu/sysemu.h"
> > +#include "sysemu/runstate.h"
> >  #include "savevm.h"
> >  #include "qemu/iov.h"
> >  #include "multifd.h"
> > @@ -3710,8 +3711,48 @@ static SaveVMHandlers savevm_ram_handlers = {
> >      .resume_prepare = ram_resume_prepare,
> >  };
> >  
> > +static void ram_mig_ram_block_resized(RAMBlockNotifier *n, void *host,
> > +                                      size_t old_size, size_t new_size)
> > +{
> > +    ram_addr_t offset;
> > +    Error *err = NULL;
> > +    RAMBlock *rb = qemu_ram_block_from_host(host, false, &offset);
> > +
> > +    if (ramblock_is_ignored(rb)) {
> > +        return;
> > +    }
> > +
> > +    /*
> > +     * Some resizes are triggered on the migration target by precopy code,
> > +     * when synchronizing RAM block sizes. In these cases, the VM is not
> > +     * running and migration is not idle. We have to ignore these resizes,
> > +     * as we only care about resizes during precopy on the migration source.
> > +     * This handler is always registered, so ignore when migration is idle.
> > +     */
> > +    if (migration_is_idle() || !runstate_is_running() ||

So I noticed that I mis-misread the code after chat with Dave...

migration_is_idle() should only return false if on the source and only
if during migration.  Destination should still return true for that
(destination VM reads state from MigrationIncomingState.state
instead).

With that, I think we can drop the confusing !runstate_is_running()
check because migration_is_idle() will cover that (and touch up the
comment too)?

> > +        postcopy_is_running()) {
> > +        return;
> > +    }
> > +
> > +    /*
> > +     * Precopy code cannot deal with the size of ram blocks changing at
> > +     * random points in time. We're still running on the source, abort
> > +     * the migration and continue running here. Make sure to wait until
> > +     * migration was canceled.
> > +     */
> > +    error_setg(&err, "RAM block '%s' resized during precopy.", rb->idstr);
> > +    migrate_set_error(migrate_get_current(), err);
> > +    error_free(err);
> > +    migration_cancel();
> > +}
> > +
> > +static RAMBlockNotifier ram_mig_ram_notifier = {
> > +    .ram_block_resized = ram_mig_ram_block_resized,
> > +};
> > +
> >  void ram_mig_init(void)
> >  {
> >      qemu_mutex_init(&XBZRLE.lock);
> >      register_savevm_live("ram", 0, 4, &savevm_ram_handlers, &ram_state);
> > +    ram_block_notifier_add(&ram_mig_ram_notifier);
> >  }
> > 
> 
> So, this seems to work very reliably when triggering a resize of a RAM
> block during system reset (using my virtio-mem prototype):
> 
> (qemu) info migrate
> globals:
> store-global-state: on
> only-migratable: off
> send-configuration: on
> send-section-footer: on
> decompress-error-check: on
> clear-bitmap-shift: 18
> Migration status: cancelled
> total time: 0 milliseconds
> 
> 
> And from QEMU
> 
> qemu-system-x86_64: RAM block '0000:00:03.0/mem1' resized during precopy.

Good news!

Thanks,

-- 
Peter Xu

Re: [PATCH v1 05/13] migrate/ram: Handle RAM block resizes during precopy

[David Hildenbrand]

On 20.02.20 21:17, Peter Xu wrote:
> On Thu, Feb 20, 2020 at 04:16:02PM +0100, David Hildenbrand wrote:
>> On 19.02.20 17:17, David Hildenbrand wrote:
>>> Resizing while migrating is dangerous and does not work as expected.
>>> The whole migration code works on the usable_length of ram blocks and does
>>> not expect this to change at random points in time.
>>>
>>> In the case of precopy, the ram block size must not change on the source,
>>> after syncing the RAM block list in ram_save_setup(), so as long as the
>>> guest is still running on the source.
>>>
>>> Resizing can be trigger *after* (but not during) a reset in
>>> ACPI code by the guest
>>> - hw/arm/virt-acpi-build.c:acpi_ram_update()
>>> - hw/i386/acpi-build.c:acpi_ram_update()
>>>
>>> Use the ram block notifier to get notified about resizes. Let's simply
>>> cancel migration and indicate the reason. We'll continue running on the
>>> source. No harm done.
>>>
>>> Update the documentation. Postcopy will be handled separately.
>>>
>>> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>>> Cc: Juan Quintela <quintela@redhat.com>
>>> Cc: Eduardo Habkost <ehabkost@redhat.com>
>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>> Cc: Igor Mammedov <imammedo@redhat.com>
>>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>>> Cc: Richard Henderson <richard.henderson@linaro.org>
>>> Cc: Shannon Zhao <shannon.zhao@linaro.org>
>>> Cc: Alex Bennée <alex.bennee@linaro.org>
>>> Cc: Peter Xu <peterx@redhat.com>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>>  exec.c                |  5 +++--
>>>  include/exec/memory.h | 10 ++++++----
>>>  migration/migration.c |  9 +++++++--
>>>  migration/migration.h |  1 +
>>>  migration/ram.c       | 41 +++++++++++++++++++++++++++++++++++++++++
>>>  5 files changed, 58 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/exec.c b/exec.c
>>> index b75250e773..8b015821d6 100644
>>> --- a/exec.c
>>> +++ b/exec.c
>>> @@ -2120,8 +2120,9 @@ static int memory_try_enable_merging(void *addr, size_t len)
>>>      return qemu_madvise(addr, len, QEMU_MADV_MERGEABLE);
>>>  }
>>>  
>>> -/* Only legal before guest might have detected the memory size: e.g. on
>>> - * incoming migration, or right after reset.
>>> +/*
>>> + * Resizing RAM while migrating can result in the migration being canceled.
>>> + * Care has to be taken if the guest might have already detected the memory.
>>>   *
>>>   * As memory core doesn't know how is memory accessed, it is up to
>>>   * resize callback to update device state and/or add assertions to detect
>>> diff --git a/include/exec/memory.h b/include/exec/memory.h
>>> index e85b7de99a..de111347e8 100644
>>> --- a/include/exec/memory.h
>>> +++ b/include/exec/memory.h
>>> @@ -113,7 +113,7 @@ typedef struct IOMMUNotifier IOMMUNotifier;
>>>  #define RAM_SHARED     (1 << 1)
>>>  
>>>  /* Only a portion of RAM (used_length) is actually used, and migrated.
>>> - * This used_length size can change across reboots.
>>> + * Resizing RAM while migrating can result in the migration being canceled.
>>>   */
>>>  #define RAM_RESIZEABLE (1 << 2)
>>>  
>>> @@ -843,7 +843,9 @@ void memory_region_init_ram_shared_nomigrate(MemoryRegion *mr,
>>>   *                                     RAM.  Accesses into the region will
>>>   *                                     modify memory directly.  Only an initial
>>>   *                                     portion of this RAM is actually used.
>>> - *                                     The used size can change across reboots.
>>> + *                                     Changing the size while migrating
>>> + *                                     can result in the migration being
>>> + *                                     canceled.
>>>   *
>>>   * @mr: the #MemoryRegion to be initialized.
>>>   * @owner: the object that tracks the region's reference count
>>> @@ -1464,8 +1466,8 @@ void *memory_region_get_ram_ptr(MemoryRegion *mr);
>>>  
>>>  /* memory_region_ram_resize: Resize a RAM region.
>>>   *
>>> - * Only legal before guest might have detected the memory size: e.g. on
>>> - * incoming migration, or right after reset.
>>> + * Resizing RAM while migrating can result in the migration being canceled.
>>> + * Care has to be taken if the guest might have already detected the memory.
>>>   *
>>>   * @mr: a memory region created with @memory_region_init_resizeable_ram.
>>>   * @newsize: the new size the region
>>> diff --git a/migration/migration.c b/migration/migration.c
>>> index 8fb68795dc..ac9751dbe5 100644
>>> --- a/migration/migration.c
>>> +++ b/migration/migration.c
>>> @@ -175,13 +175,18 @@ void migration_object_init(void)
>>>      }
>>>  }
>>>  
>>> +void migration_cancel(void)
>>> +{
>>> +    migrate_fd_cancel(current_migration);
>>> +}
>>> +
>>>  void migration_shutdown(void)
>>>  {
>>>      /*
>>>       * Cancel the current migration - that will (eventually)
>>>       * stop the migration using this structure
>>>       */
>>> -    migrate_fd_cancel(current_migration);
>>> +    migration_cancel();
>>>      object_unref(OBJECT(current_migration));
>>>  }
>>>  
>>> @@ -2019,7 +2024,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>>>  
>>>  void qmp_migrate_cancel(Error **errp)
>>>  {
>>> -    migrate_fd_cancel(migrate_get_current());
>>> +    migration_cancel();
>>>  }
>>>  
>>>  void qmp_migrate_continue(MigrationStatus state, Error **errp)
>>> diff --git a/migration/migration.h b/migration/migration.h
>>> index 8473ddfc88..79fd74afa5 100644
>>> --- a/migration/migration.h
>>> +++ b/migration/migration.h
>>> @@ -343,5 +343,6 @@ int foreach_not_ignored_block(RAMBlockIterFunc func, void *opaque);
>>>  void migration_make_urgent_request(void);
>>>  void migration_consume_urgent_request(void);
>>>  bool migration_rate_limit(void);
>>> +void migration_cancel(void);
>>>  
>>>  #endif
>>> diff --git a/migration/ram.c b/migration/ram.c
>>> index ed23ed1c7c..57f32011a3 100644
>>> --- a/migration/ram.c
>>> +++ b/migration/ram.c
>>> @@ -52,6 +52,7 @@
>>>  #include "migration/colo.h"
>>>  #include "block.h"
>>>  #include "sysemu/sysemu.h"
>>> +#include "sysemu/runstate.h"
>>>  #include "savevm.h"
>>>  #include "qemu/iov.h"
>>>  #include "multifd.h"
>>> @@ -3710,8 +3711,48 @@ static SaveVMHandlers savevm_ram_handlers = {
>>>      .resume_prepare = ram_resume_prepare,
>>>  };
>>>  
>>> +static void ram_mig_ram_block_resized(RAMBlockNotifier *n, void *host,
>>> +                                      size_t old_size, size_t new_size)
>>> +{
>>> +    ram_addr_t offset;
>>> +    Error *err = NULL;
>>> +    RAMBlock *rb = qemu_ram_block_from_host(host, false, &offset);
>>> +
>>> +    if (ramblock_is_ignored(rb)) {
>>> +        return;
>>> +    }
>>> +
>>> +    /*
>>> +     * Some resizes are triggered on the migration target by precopy code,
>>> +     * when synchronizing RAM block sizes. In these cases, the VM is not
>>> +     * running and migration is not idle. We have to ignore these resizes,
>>> +     * as we only care about resizes during precopy on the migration source.
>>> +     * This handler is always registered, so ignore when migration is idle.
>>> +     */
>>> +    if (migration_is_idle() || !runstate_is_running() ||
> 
> So I noticed that I mis-misread the code after chat with Dave...
> 
> migration_is_idle() should only return false if on the source and only
> if during migration.  Destination should still return true for that
> (destination VM reads state from MigrationIncomingState.state
> instead).
> 
> With that, I think we can drop the confusing !runstate_is_running()
> check because migration_is_idle() will cover that (and touch up the
> comment too)?

So, we want to cancel migration whenever we are on the source and we are
migrating (postcopy). Resizing will only happen while the VM is running
on the source once we're migrating.

So you're saying that

if (migration_is_idle() || postcopy_is_running()) {
	return:
}

is enough. Will migration_is_idle() always return true on the
destination (I remember something different, but might be *I* misread
the code)?

Then this would distill down to

if (migration_is_idle()) {
	return:
}

Which would be nice.

-- 
Thanks,

David / dhildenb

Re: [PATCH v1 05/13] migrate/ram: Handle RAM block resizes during precopy

[David Hildenbrand]

On 21.02.20 10:18, David Hildenbrand wrote:
> On 20.02.20 21:17, Peter Xu wrote:
>> On Thu, Feb 20, 2020 at 04:16:02PM +0100, David Hildenbrand wrote:
>>> On 19.02.20 17:17, David Hildenbrand wrote:
>>>> Resizing while migrating is dangerous and does not work as expected.
>>>> The whole migration code works on the usable_length of ram blocks and does
>>>> not expect this to change at random points in time.
>>>>
>>>> In the case of precopy, the ram block size must not change on the source,
>>>> after syncing the RAM block list in ram_save_setup(), so as long as the
>>>> guest is still running on the source.
>>>>
>>>> Resizing can be trigger *after* (but not during) a reset in
>>>> ACPI code by the guest
>>>> - hw/arm/virt-acpi-build.c:acpi_ram_update()
>>>> - hw/i386/acpi-build.c:acpi_ram_update()
>>>>
>>>> Use the ram block notifier to get notified about resizes. Let's simply
>>>> cancel migration and indicate the reason. We'll continue running on the
>>>> source. No harm done.
>>>>
>>>> Update the documentation. Postcopy will be handled separately.
>>>>
>>>> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>>>> Cc: Juan Quintela <quintela@redhat.com>
>>>> Cc: Eduardo Habkost <ehabkost@redhat.com>
>>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>>> Cc: Igor Mammedov <imammedo@redhat.com>
>>>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>>>> Cc: Richard Henderson <richard.henderson@linaro.org>
>>>> Cc: Shannon Zhao <shannon.zhao@linaro.org>
>>>> Cc: Alex Bennée <alex.bennee@linaro.org>
>>>> Cc: Peter Xu <peterx@redhat.com>
>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>> ---
>>>>  exec.c                |  5 +++--
>>>>  include/exec/memory.h | 10 ++++++----
>>>>  migration/migration.c |  9 +++++++--
>>>>  migration/migration.h |  1 +
>>>>  migration/ram.c       | 41 +++++++++++++++++++++++++++++++++++++++++
>>>>  5 files changed, 58 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/exec.c b/exec.c
>>>> index b75250e773..8b015821d6 100644
>>>> --- a/exec.c
>>>> +++ b/exec.c
>>>> @@ -2120,8 +2120,9 @@ static int memory_try_enable_merging(void *addr, size_t len)
>>>>      return qemu_madvise(addr, len, QEMU_MADV_MERGEABLE);
>>>>  }
>>>>  
>>>> -/* Only legal before guest might have detected the memory size: e.g. on
>>>> - * incoming migration, or right after reset.
>>>> +/*
>>>> + * Resizing RAM while migrating can result in the migration being canceled.
>>>> + * Care has to be taken if the guest might have already detected the memory.
>>>>   *
>>>>   * As memory core doesn't know how is memory accessed, it is up to
>>>>   * resize callback to update device state and/or add assertions to detect
>>>> diff --git a/include/exec/memory.h b/include/exec/memory.h
>>>> index e85b7de99a..de111347e8 100644
>>>> --- a/include/exec/memory.h
>>>> +++ b/include/exec/memory.h
>>>> @@ -113,7 +113,7 @@ typedef struct IOMMUNotifier IOMMUNotifier;
>>>>  #define RAM_SHARED     (1 << 1)
>>>>  
>>>>  /* Only a portion of RAM (used_length) is actually used, and migrated.
>>>> - * This used_length size can change across reboots.
>>>> + * Resizing RAM while migrating can result in the migration being canceled.
>>>>   */
>>>>  #define RAM_RESIZEABLE (1 << 2)
>>>>  
>>>> @@ -843,7 +843,9 @@ void memory_region_init_ram_shared_nomigrate(MemoryRegion *mr,
>>>>   *                                     RAM.  Accesses into the region will
>>>>   *                                     modify memory directly.  Only an initial
>>>>   *                                     portion of this RAM is actually used.
>>>> - *                                     The used size can change across reboots.
>>>> + *                                     Changing the size while migrating
>>>> + *                                     can result in the migration being
>>>> + *                                     canceled.
>>>>   *
>>>>   * @mr: the #MemoryRegion to be initialized.
>>>>   * @owner: the object that tracks the region's reference count
>>>> @@ -1464,8 +1466,8 @@ void *memory_region_get_ram_ptr(MemoryRegion *mr);
>>>>  
>>>>  /* memory_region_ram_resize: Resize a RAM region.
>>>>   *
>>>> - * Only legal before guest might have detected the memory size: e.g. on
>>>> - * incoming migration, or right after reset.
>>>> + * Resizing RAM while migrating can result in the migration being canceled.
>>>> + * Care has to be taken if the guest might have already detected the memory.
>>>>   *
>>>>   * @mr: a memory region created with @memory_region_init_resizeable_ram.
>>>>   * @newsize: the new size the region
>>>> diff --git a/migration/migration.c b/migration/migration.c
>>>> index 8fb68795dc..ac9751dbe5 100644
>>>> --- a/migration/migration.c
>>>> +++ b/migration/migration.c
>>>> @@ -175,13 +175,18 @@ void migration_object_init(void)
>>>>      }
>>>>  }
>>>>  
>>>> +void migration_cancel(void)
>>>> +{
>>>> +    migrate_fd_cancel(current_migration);
>>>> +}
>>>> +
>>>>  void migration_shutdown(void)
>>>>  {
>>>>      /*
>>>>       * Cancel the current migration - that will (eventually)
>>>>       * stop the migration using this structure
>>>>       */
>>>> -    migrate_fd_cancel(current_migration);
>>>> +    migration_cancel();
>>>>      object_unref(OBJECT(current_migration));
>>>>  }
>>>>  
>>>> @@ -2019,7 +2024,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>>>>  
>>>>  void qmp_migrate_cancel(Error **errp)
>>>>  {
>>>> -    migrate_fd_cancel(migrate_get_current());
>>>> +    migration_cancel();
>>>>  }
>>>>  
>>>>  void qmp_migrate_continue(MigrationStatus state, Error **errp)
>>>> diff --git a/migration/migration.h b/migration/migration.h
>>>> index 8473ddfc88..79fd74afa5 100644
>>>> --- a/migration/migration.h
>>>> +++ b/migration/migration.h
>>>> @@ -343,5 +343,6 @@ int foreach_not_ignored_block(RAMBlockIterFunc func, void *opaque);
>>>>  void migration_make_urgent_request(void);
>>>>  void migration_consume_urgent_request(void);
>>>>  bool migration_rate_limit(void);
>>>> +void migration_cancel(void);
>>>>  
>>>>  #endif
>>>> diff --git a/migration/ram.c b/migration/ram.c
>>>> index ed23ed1c7c..57f32011a3 100644
>>>> --- a/migration/ram.c
>>>> +++ b/migration/ram.c
>>>> @@ -52,6 +52,7 @@
>>>>  #include "migration/colo.h"
>>>>  #include "block.h"
>>>>  #include "sysemu/sysemu.h"
>>>> +#include "sysemu/runstate.h"
>>>>  #include "savevm.h"
>>>>  #include "qemu/iov.h"
>>>>  #include "multifd.h"
>>>> @@ -3710,8 +3711,48 @@ static SaveVMHandlers savevm_ram_handlers = {
>>>>      .resume_prepare = ram_resume_prepare,
>>>>  };
>>>>  
>>>> +static void ram_mig_ram_block_resized(RAMBlockNotifier *n, void *host,
>>>> +                                      size_t old_size, size_t new_size)
>>>> +{
>>>> +    ram_addr_t offset;
>>>> +    Error *err = NULL;
>>>> +    RAMBlock *rb = qemu_ram_block_from_host(host, false, &offset);
>>>> +
>>>> +    if (ramblock_is_ignored(rb)) {
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    /*
>>>> +     * Some resizes are triggered on the migration target by precopy code,
>>>> +     * when synchronizing RAM block sizes. In these cases, the VM is not
>>>> +     * running and migration is not idle. We have to ignore these resizes,
>>>> +     * as we only care about resizes during precopy on the migration source.
>>>> +     * This handler is always registered, so ignore when migration is idle.
>>>> +     */
>>>> +    if (migration_is_idle() || !runstate_is_running() ||
>>
>> So I noticed that I mis-misread the code after chat with Dave...
>>
>> migration_is_idle() should only return false if on the source and only
>> if during migration.  Destination should still return true for that
>> (destination VM reads state from MigrationIncomingState.state
>> instead).
>>
>> With that, I think we can drop the confusing !runstate_is_running()
>> check because migration_is_idle() will cover that (and touch up the
>> comment too)?
> 
> So, we want to cancel migration whenever we are on the source and we are
> migrating (postcopy). Resizing will only happen while the VM is running
> on the source once we're migrating.
> 
> So you're saying that
> 
> if (migration_is_idle() || postcopy_is_running()) {
> 	return:
> }
> 
> is enough. Will migration_is_idle() always return true on the
> destination (I remember something different, but might be *I* misread
> the code)?
> 
> Then this would distill down to
> 
> if (migration_is_idle()) {
> 	return:
> }
> 
> Which would be nice.

... but looking at process_incoming_migration_co(), we'll set the state
to MIGRATION_STATUS_ACTIVE right at the start.


So dropping postcopy_is_running() would be wrong and dropping
!runstate_is_running() would be wrong as well right now.

What we actually want is

if (!migration_outgoing()) {
	return;
}

Any experts?

-- 
Thanks,

David / dhildenb

Re: [PATCH v1 05/13] migrate/ram: Handle RAM block resizes during precopy

[David Hildenbrand]

On 21.02.20 11:10, David Hildenbrand wrote:
> On 21.02.20 10:18, David Hildenbrand wrote:
>> On 20.02.20 21:17, Peter Xu wrote:
>>> On Thu, Feb 20, 2020 at 04:16:02PM +0100, David Hildenbrand wrote:
>>>> On 19.02.20 17:17, David Hildenbrand wrote:
>>>>> Resizing while migrating is dangerous and does not work as expected.
>>>>> The whole migration code works on the usable_length of ram blocks and does
>>>>> not expect this to change at random points in time.
>>>>>
>>>>> In the case of precopy, the ram block size must not change on the source,
>>>>> after syncing the RAM block list in ram_save_setup(), so as long as the
>>>>> guest is still running on the source.
>>>>>
>>>>> Resizing can be trigger *after* (but not during) a reset in
>>>>> ACPI code by the guest
>>>>> - hw/arm/virt-acpi-build.c:acpi_ram_update()
>>>>> - hw/i386/acpi-build.c:acpi_ram_update()
>>>>>
>>>>> Use the ram block notifier to get notified about resizes. Let's simply
>>>>> cancel migration and indicate the reason. We'll continue running on the
>>>>> source. No harm done.
>>>>>
>>>>> Update the documentation. Postcopy will be handled separately.
>>>>>
>>>>> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>>>>> Cc: Juan Quintela <quintela@redhat.com>
>>>>> Cc: Eduardo Habkost <ehabkost@redhat.com>
>>>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>>>> Cc: Igor Mammedov <imammedo@redhat.com>
>>>>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>>>>> Cc: Richard Henderson <richard.henderson@linaro.org>
>>>>> Cc: Shannon Zhao <shannon.zhao@linaro.org>
>>>>> Cc: Alex Bennée <alex.bennee@linaro.org>
>>>>> Cc: Peter Xu <peterx@redhat.com>
>>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>>> ---
>>>>>  exec.c                |  5 +++--
>>>>>  include/exec/memory.h | 10 ++++++----
>>>>>  migration/migration.c |  9 +++++++--
>>>>>  migration/migration.h |  1 +
>>>>>  migration/ram.c       | 41 +++++++++++++++++++++++++++++++++++++++++
>>>>>  5 files changed, 58 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/exec.c b/exec.c
>>>>> index b75250e773..8b015821d6 100644
>>>>> --- a/exec.c
>>>>> +++ b/exec.c
>>>>> @@ -2120,8 +2120,9 @@ static int memory_try_enable_merging(void *addr, size_t len)
>>>>>      return qemu_madvise(addr, len, QEMU_MADV_MERGEABLE);
>>>>>  }
>>>>>  
>>>>> -/* Only legal before guest might have detected the memory size: e.g. on
>>>>> - * incoming migration, or right after reset.
>>>>> +/*
>>>>> + * Resizing RAM while migrating can result in the migration being canceled.
>>>>> + * Care has to be taken if the guest might have already detected the memory.
>>>>>   *
>>>>>   * As memory core doesn't know how is memory accessed, it is up to
>>>>>   * resize callback to update device state and/or add assertions to detect
>>>>> diff --git a/include/exec/memory.h b/include/exec/memory.h
>>>>> index e85b7de99a..de111347e8 100644
>>>>> --- a/include/exec/memory.h
>>>>> +++ b/include/exec/memory.h
>>>>> @@ -113,7 +113,7 @@ typedef struct IOMMUNotifier IOMMUNotifier;
>>>>>  #define RAM_SHARED     (1 << 1)
>>>>>  
>>>>>  /* Only a portion of RAM (used_length) is actually used, and migrated.
>>>>> - * This used_length size can change across reboots.
>>>>> + * Resizing RAM while migrating can result in the migration being canceled.
>>>>>   */
>>>>>  #define RAM_RESIZEABLE (1 << 2)
>>>>>  
>>>>> @@ -843,7 +843,9 @@ void memory_region_init_ram_shared_nomigrate(MemoryRegion *mr,
>>>>>   *                                     RAM.  Accesses into the region will
>>>>>   *                                     modify memory directly.  Only an initial
>>>>>   *                                     portion of this RAM is actually used.
>>>>> - *                                     The used size can change across reboots.
>>>>> + *                                     Changing the size while migrating
>>>>> + *                                     can result in the migration being
>>>>> + *                                     canceled.
>>>>>   *
>>>>>   * @mr: the #MemoryRegion to be initialized.
>>>>>   * @owner: the object that tracks the region's reference count
>>>>> @@ -1464,8 +1466,8 @@ void *memory_region_get_ram_ptr(MemoryRegion *mr);
>>>>>  
>>>>>  /* memory_region_ram_resize: Resize a RAM region.
>>>>>   *
>>>>> - * Only legal before guest might have detected the memory size: e.g. on
>>>>> - * incoming migration, or right after reset.
>>>>> + * Resizing RAM while migrating can result in the migration being canceled.
>>>>> + * Care has to be taken if the guest might have already detected the memory.
>>>>>   *
>>>>>   * @mr: a memory region created with @memory_region_init_resizeable_ram.
>>>>>   * @newsize: the new size the region
>>>>> diff --git a/migration/migration.c b/migration/migration.c
>>>>> index 8fb68795dc..ac9751dbe5 100644
>>>>> --- a/migration/migration.c
>>>>> +++ b/migration/migration.c
>>>>> @@ -175,13 +175,18 @@ void migration_object_init(void)
>>>>>      }
>>>>>  }
>>>>>  
>>>>> +void migration_cancel(void)
>>>>> +{
>>>>> +    migrate_fd_cancel(current_migration);
>>>>> +}
>>>>> +
>>>>>  void migration_shutdown(void)
>>>>>  {
>>>>>      /*
>>>>>       * Cancel the current migration - that will (eventually)
>>>>>       * stop the migration using this structure
>>>>>       */
>>>>> -    migrate_fd_cancel(current_migration);
>>>>> +    migration_cancel();
>>>>>      object_unref(OBJECT(current_migration));
>>>>>  }
>>>>>  
>>>>> @@ -2019,7 +2024,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>>>>>  
>>>>>  void qmp_migrate_cancel(Error **errp)
>>>>>  {
>>>>> -    migrate_fd_cancel(migrate_get_current());
>>>>> +    migration_cancel();
>>>>>  }
>>>>>  
>>>>>  void qmp_migrate_continue(MigrationStatus state, Error **errp)
>>>>> diff --git a/migration/migration.h b/migration/migration.h
>>>>> index 8473ddfc88..79fd74afa5 100644
>>>>> --- a/migration/migration.h
>>>>> +++ b/migration/migration.h
>>>>> @@ -343,5 +343,6 @@ int foreach_not_ignored_block(RAMBlockIterFunc func, void *opaque);
>>>>>  void migration_make_urgent_request(void);
>>>>>  void migration_consume_urgent_request(void);
>>>>>  bool migration_rate_limit(void);
>>>>> +void migration_cancel(void);
>>>>>  
>>>>>  #endif
>>>>> diff --git a/migration/ram.c b/migration/ram.c
>>>>> index ed23ed1c7c..57f32011a3 100644
>>>>> --- a/migration/ram.c
>>>>> +++ b/migration/ram.c
>>>>> @@ -52,6 +52,7 @@
>>>>>  #include "migration/colo.h"
>>>>>  #include "block.h"
>>>>>  #include "sysemu/sysemu.h"
>>>>> +#include "sysemu/runstate.h"
>>>>>  #include "savevm.h"
>>>>>  #include "qemu/iov.h"
>>>>>  #include "multifd.h"
>>>>> @@ -3710,8 +3711,48 @@ static SaveVMHandlers savevm_ram_handlers = {
>>>>>      .resume_prepare = ram_resume_prepare,
>>>>>  };
>>>>>  
>>>>> +static void ram_mig_ram_block_resized(RAMBlockNotifier *n, void *host,
>>>>> +                                      size_t old_size, size_t new_size)
>>>>> +{
>>>>> +    ram_addr_t offset;
>>>>> +    Error *err = NULL;
>>>>> +    RAMBlock *rb = qemu_ram_block_from_host(host, false, &offset);
>>>>> +
>>>>> +    if (ramblock_is_ignored(rb)) {
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>>> +    /*
>>>>> +     * Some resizes are triggered on the migration target by precopy code,
>>>>> +     * when synchronizing RAM block sizes. In these cases, the VM is not
>>>>> +     * running and migration is not idle. We have to ignore these resizes,
>>>>> +     * as we only care about resizes during precopy on the migration source.
>>>>> +     * This handler is always registered, so ignore when migration is idle.
>>>>> +     */
>>>>> +    if (migration_is_idle() || !runstate_is_running() ||
>>>
>>> So I noticed that I mis-misread the code after chat with Dave...
>>>
>>> migration_is_idle() should only return false if on the source and only
>>> if during migration.  Destination should still return true for that
>>> (destination VM reads state from MigrationIncomingState.state
>>> instead).
>>>
>>> With that, I think we can drop the confusing !runstate_is_running()
>>> check because migration_is_idle() will cover that (and touch up the
>>> comment too)?
>>
>> So, we want to cancel migration whenever we are on the source and we are
>> migrating (postcopy). Resizing will only happen while the VM is running
>> on the source once we're migrating.
>>
>> So you're saying that
>>
>> if (migration_is_idle() || postcopy_is_running()) {
>> 	return:
>> }
>>
>> is enough. Will migration_is_idle() always return true on the
>> destination (I remember something different, but might be *I* misread
>> the code)?
>>
>> Then this would distill down to
>>
>> if (migration_is_idle()) {
>> 	return:
>> }
>>
>> Which would be nice.
> 
> ... but looking at process_incoming_migration_co(), we'll set the state
> to MIGRATION_STATUS_ACTIVE right at the start.
> 
> 
> So dropping postcopy_is_running() would be wrong and dropping
> !runstate_is_running() would be wrong as well right now.
> 
> What we actually want is
> 
> if (!migration_outgoing()) {
> 	return;
> }
> 
> Any experts?
> 

Lol, man this code is confusing.

So, migration_is_idle() really only checks current_migration, not
current_incoming.

I didn't expect to be knees deep in migration code at this point ...

if (migration_is_idle()) {
	return:
}

works. Thanks!

-- 
Thanks,

David / dhildenb

Re: [PATCH v1 05/13] migrate/ram: Handle RAM block resizes during precopy

[Peter Xu]

On Fri, Feb 21, 2020 at 11:19:58AM +0100, David Hildenbrand wrote:

[...]

> Lol, man this code is confusing.

(Yes in many places it is...)

> 
> So, migration_is_idle() really only checks current_migration, not
> current_incoming.
> 
> I didn't expect to be knees deep in migration code at this point ...
> 
> if (migration_is_idle()) {
> 	return:
> }

Yes this seems to work too for postcopy, though may worth add a
comment showing the fact...  Thanks,

-- 
Peter Xu

Re: [PATCH v1 05/13] migrate/ram: Handle RAM block resizes during precopy

[Dr. David Alan Gilbert]

* David Hildenbrand (david@redhat.com) wrote:
> Resizing while migrating is dangerous and does not work as expected.
> The whole migration code works on the usable_length of ram blocks and does
> not expect this to change at random points in time.
> 
> In the case of precopy, the ram block size must not change on the source,
> after syncing the RAM block list in ram_save_setup(), so as long as the
> guest is still running on the source.
> 
> Resizing can be trigger *after* (but not during) a reset in
> ACPI code by the guest
> - hw/arm/virt-acpi-build.c:acpi_ram_update()
> - hw/i386/acpi-build.c:acpi_ram_update()
> 
> Use the ram block notifier to get notified about resizes. Let's simply
> cancel migration and indicate the reason. We'll continue running on the
> source. No harm done.
> 
> Update the documentation. Postcopy will be handled separately.
> 
> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> Cc: Juan Quintela <quintela@redhat.com>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Richard Henderson <richard.henderson@linaro.org>
> Cc: Shannon Zhao <shannon.zhao@linaro.org>
> Cc: Alex Bennée <alex.bennee@linaro.org>
> Cc: Peter Xu <peterx@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  exec.c                |  5 +++--
>  include/exec/memory.h | 10 ++++++----
>  migration/migration.c |  9 +++++++--
>  migration/migration.h |  1 +
>  migration/ram.c       | 41 +++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 58 insertions(+), 8 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index b75250e773..8b015821d6 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2120,8 +2120,9 @@ static int memory_try_enable_merging(void *addr, size_t len)
>      return qemu_madvise(addr, len, QEMU_MADV_MERGEABLE);
>  }
>  
> -/* Only legal before guest might have detected the memory size: e.g. on
> - * incoming migration, or right after reset.
> +/*
> + * Resizing RAM while migrating can result in the migration being canceled.
> + * Care has to be taken if the guest might have already detected the memory.
>   *
>   * As memory core doesn't know how is memory accessed, it is up to
>   * resize callback to update device state and/or add assertions to detect
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index e85b7de99a..de111347e8 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -113,7 +113,7 @@ typedef struct IOMMUNotifier IOMMUNotifier;
>  #define RAM_SHARED     (1 << 1)
>  
>  /* Only a portion of RAM (used_length) is actually used, and migrated.
> - * This used_length size can change across reboots.
> + * Resizing RAM while migrating can result in the migration being canceled.
>   */
>  #define RAM_RESIZEABLE (1 << 2)
>  
> @@ -843,7 +843,9 @@ void memory_region_init_ram_shared_nomigrate(MemoryRegion *mr,
>   *                                     RAM.  Accesses into the region will
>   *                                     modify memory directly.  Only an initial
>   *                                     portion of this RAM is actually used.
> - *                                     The used size can change across reboots.
> + *                                     Changing the size while migrating
> + *                                     can result in the migration being
> + *                                     canceled.
>   *
>   * @mr: the #MemoryRegion to be initialized.
>   * @owner: the object that tracks the region's reference count
> @@ -1464,8 +1466,8 @@ void *memory_region_get_ram_ptr(MemoryRegion *mr);
>  
>  /* memory_region_ram_resize: Resize a RAM region.
>   *
> - * Only legal before guest might have detected the memory size: e.g. on
> - * incoming migration, or right after reset.
> + * Resizing RAM while migrating can result in the migration being canceled.
> + * Care has to be taken if the guest might have already detected the memory.
>   *
>   * @mr: a memory region created with @memory_region_init_resizeable_ram.
>   * @newsize: the new size the region
> diff --git a/migration/migration.c b/migration/migration.c
> index 8fb68795dc..ac9751dbe5 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -175,13 +175,18 @@ void migration_object_init(void)
>      }
>  }
>  
> +void migration_cancel(void)
> +{
> +    migrate_fd_cancel(current_migration);
> +}
> +
>  void migration_shutdown(void)
>  {
>      /*
>       * Cancel the current migration - that will (eventually)
>       * stop the migration using this structure
>       */
> -    migrate_fd_cancel(current_migration);
> +    migration_cancel();
>      object_unref(OBJECT(current_migration));
>  }
>  
> @@ -2019,7 +2024,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>  
>  void qmp_migrate_cancel(Error **errp)
>  {
> -    migrate_fd_cancel(migrate_get_current());
> +    migration_cancel();
>  }
>  
>  void qmp_migrate_continue(MigrationStatus state, Error **errp)
> diff --git a/migration/migration.h b/migration/migration.h
> index 8473ddfc88..79fd74afa5 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -343,5 +343,6 @@ int foreach_not_ignored_block(RAMBlockIterFunc func, void *opaque);
>  void migration_make_urgent_request(void);
>  void migration_consume_urgent_request(void);
>  bool migration_rate_limit(void);
> +void migration_cancel(void);
>  
>  #endif
> diff --git a/migration/ram.c b/migration/ram.c
> index ed23ed1c7c..57f32011a3 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -52,6 +52,7 @@
>  #include "migration/colo.h"
>  #include "block.h"
>  #include "sysemu/sysemu.h"
> +#include "sysemu/runstate.h"
>  #include "savevm.h"
>  #include "qemu/iov.h"
>  #include "multifd.h"
> @@ -3710,8 +3711,48 @@ static SaveVMHandlers savevm_ram_handlers = {
>      .resume_prepare = ram_resume_prepare,
>  };
>  
> +static void ram_mig_ram_block_resized(RAMBlockNotifier *n, void *host,
> +                                      size_t old_size, size_t new_size)
> +{
> +    ram_addr_t offset;
> +    Error *err = NULL;
> +    RAMBlock *rb = qemu_ram_block_from_host(host, false, &offset);
> +
> +    if (ramblock_is_ignored(rb)) {
> +        return;
> +    }
> +
> +    /*
> +     * Some resizes are triggered on the migration target by precopy code,
> +     * when synchronizing RAM block sizes. In these cases, the VM is not
> +     * running and migration is not idle. We have to ignore these resizes,
> +     * as we only care about resizes during precopy on the migration source.
> +     * This handler is always registered, so ignore when migration is idle.
> +     */
> +    if (migration_is_idle() || !runstate_is_running() ||
> +        postcopy_is_running()) {
> +        return;
> +    }
> +
> +    /*
> +     * Precopy code cannot deal with the size of ram blocks changing at
> +     * random points in time. We're still running on the source, abort
> +     * the migration and continue running here. Make sure to wait until
> +     * migration was canceled.
> +     */
> +    error_setg(&err, "RAM block '%s' resized during precopy.", rb->idstr);
> +    migrate_set_error(migrate_get_current(), err);
> +    error_free(err);
> +    migration_cancel();
> +}
> +
> +static RAMBlockNotifier ram_mig_ram_notifier = {
> +    .ram_block_resized = ram_mig_ram_block_resized,
> +};
> +
>  void ram_mig_init(void)
>  {
>      qemu_mutex_init(&XBZRLE.lock);
>      register_savevm_live("ram", 0, 4, &savevm_ram_handlers, &ram_state);
> +    ram_block_notifier_add(&ram_mig_ram_notifier);

Can we avoid the question of the 'is_idle' checks by doing this
registration in save_setup/load_setup and unregistering in
save_cleanup/load_cleanup?

That means if we land in the handler we know we're in either an incoming
or outgoing migration and then you just have to check which?

Dave

>  }
> -- 
> 2.24.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v1 05/13] migrate/ram: Handle RAM block resizes during precopy

[David Hildenbrand]

On 21.02.20 16:14, Dr. David Alan Gilbert wrote:
> * David Hildenbrand (david@redhat.com) wrote:
>> Resizing while migrating is dangerous and does not work as expected.
>> The whole migration code works on the usable_length of ram blocks and does
>> not expect this to change at random points in time.
>>
>> In the case of precopy, the ram block size must not change on the source,
>> after syncing the RAM block list in ram_save_setup(), so as long as the
>> guest is still running on the source.
>>
>> Resizing can be trigger *after* (but not during) a reset in
>> ACPI code by the guest
>> - hw/arm/virt-acpi-build.c:acpi_ram_update()
>> - hw/i386/acpi-build.c:acpi_ram_update()
>>
>> Use the ram block notifier to get notified about resizes. Let's simply
>> cancel migration and indicate the reason. We'll continue running on the
>> source. No harm done.
>>
>> Update the documentation. Postcopy will be handled separately.
>>
>> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>> Cc: Juan Quintela <quintela@redhat.com>
>> Cc: Eduardo Habkost <ehabkost@redhat.com>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Igor Mammedov <imammedo@redhat.com>
>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>> Cc: Richard Henderson <richard.henderson@linaro.org>
>> Cc: Shannon Zhao <shannon.zhao@linaro.org>
>> Cc: Alex Bennée <alex.bennee@linaro.org>
>> Cc: Peter Xu <peterx@redhat.com>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  exec.c                |  5 +++--
>>  include/exec/memory.h | 10 ++++++----
>>  migration/migration.c |  9 +++++++--
>>  migration/migration.h |  1 +
>>  migration/ram.c       | 41 +++++++++++++++++++++++++++++++++++++++++
>>  5 files changed, 58 insertions(+), 8 deletions(-)
>>
>> diff --git a/exec.c b/exec.c
>> index b75250e773..8b015821d6 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -2120,8 +2120,9 @@ static int memory_try_enable_merging(void *addr, size_t len)
>>      return qemu_madvise(addr, len, QEMU_MADV_MERGEABLE);
>>  }
>>  
>> -/* Only legal before guest might have detected the memory size: e.g. on
>> - * incoming migration, or right after reset.
>> +/*
>> + * Resizing RAM while migrating can result in the migration being canceled.
>> + * Care has to be taken if the guest might have already detected the memory.
>>   *
>>   * As memory core doesn't know how is memory accessed, it is up to
>>   * resize callback to update device state and/or add assertions to detect
>> diff --git a/include/exec/memory.h b/include/exec/memory.h
>> index e85b7de99a..de111347e8 100644
>> --- a/include/exec/memory.h
>> +++ b/include/exec/memory.h
>> @@ -113,7 +113,7 @@ typedef struct IOMMUNotifier IOMMUNotifier;
>>  #define RAM_SHARED     (1 << 1)
>>  
>>  /* Only a portion of RAM (used_length) is actually used, and migrated.
>> - * This used_length size can change across reboots.
>> + * Resizing RAM while migrating can result in the migration being canceled.
>>   */
>>  #define RAM_RESIZEABLE (1 << 2)
>>  
>> @@ -843,7 +843,9 @@ void memory_region_init_ram_shared_nomigrate(MemoryRegion *mr,
>>   *                                     RAM.  Accesses into the region will
>>   *                                     modify memory directly.  Only an initial
>>   *                                     portion of this RAM is actually used.
>> - *                                     The used size can change across reboots.
>> + *                                     Changing the size while migrating
>> + *                                     can result in the migration being
>> + *                                     canceled.
>>   *
>>   * @mr: the #MemoryRegion to be initialized.
>>   * @owner: the object that tracks the region's reference count
>> @@ -1464,8 +1466,8 @@ void *memory_region_get_ram_ptr(MemoryRegion *mr);
>>  
>>  /* memory_region_ram_resize: Resize a RAM region.
>>   *
>> - * Only legal before guest might have detected the memory size: e.g. on
>> - * incoming migration, or right after reset.
>> + * Resizing RAM while migrating can result in the migration being canceled.
>> + * Care has to be taken if the guest might have already detected the memory.
>>   *
>>   * @mr: a memory region created with @memory_region_init_resizeable_ram.
>>   * @newsize: the new size the region
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 8fb68795dc..ac9751dbe5 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -175,13 +175,18 @@ void migration_object_init(void)
>>      }
>>  }
>>  
>> +void migration_cancel(void)
>> +{
>> +    migrate_fd_cancel(current_migration);
>> +}
>> +
>>  void migration_shutdown(void)
>>  {
>>      /*
>>       * Cancel the current migration - that will (eventually)
>>       * stop the migration using this structure
>>       */
>> -    migrate_fd_cancel(current_migration);
>> +    migration_cancel();
>>      object_unref(OBJECT(current_migration));
>>  }
>>  
>> @@ -2019,7 +2024,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>>  
>>  void qmp_migrate_cancel(Error **errp)
>>  {
>> -    migrate_fd_cancel(migrate_get_current());
>> +    migration_cancel();
>>  }
>>  
>>  void qmp_migrate_continue(MigrationStatus state, Error **errp)
>> diff --git a/migration/migration.h b/migration/migration.h
>> index 8473ddfc88..79fd74afa5 100644
>> --- a/migration/migration.h
>> +++ b/migration/migration.h
>> @@ -343,5 +343,6 @@ int foreach_not_ignored_block(RAMBlockIterFunc func, void *opaque);
>>  void migration_make_urgent_request(void);
>>  void migration_consume_urgent_request(void);
>>  bool migration_rate_limit(void);
>> +void migration_cancel(void);
>>  
>>  #endif
>> diff --git a/migration/ram.c b/migration/ram.c
>> index ed23ed1c7c..57f32011a3 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -52,6 +52,7 @@
>>  #include "migration/colo.h"
>>  #include "block.h"
>>  #include "sysemu/sysemu.h"
>> +#include "sysemu/runstate.h"
>>  #include "savevm.h"
>>  #include "qemu/iov.h"
>>  #include "multifd.h"
>> @@ -3710,8 +3711,48 @@ static SaveVMHandlers savevm_ram_handlers = {
>>      .resume_prepare = ram_resume_prepare,
>>  };
>>  
>> +static void ram_mig_ram_block_resized(RAMBlockNotifier *n, void *host,
>> +                                      size_t old_size, size_t new_size)
>> +{
>> +    ram_addr_t offset;
>> +    Error *err = NULL;
>> +    RAMBlock *rb = qemu_ram_block_from_host(host, false, &offset);
>> +
>> +    if (ramblock_is_ignored(rb)) {
>> +        return;
>> +    }
>> +
>> +    /*
>> +     * Some resizes are triggered on the migration target by precopy code,
>> +     * when synchronizing RAM block sizes. In these cases, the VM is not
>> +     * running and migration is not idle. We have to ignore these resizes,
>> +     * as we only care about resizes during precopy on the migration source.
>> +     * This handler is always registered, so ignore when migration is idle.
>> +     */
>> +    if (migration_is_idle() || !runstate_is_running() ||
>> +        postcopy_is_running()) {
>> +        return;
>> +    }
>> +
>> +    /*
>> +     * Precopy code cannot deal with the size of ram blocks changing at
>> +     * random points in time. We're still running on the source, abort
>> +     * the migration and continue running here. Make sure to wait until
>> +     * migration was canceled.
>> +     */
>> +    error_setg(&err, "RAM block '%s' resized during precopy.", rb->idstr);
>> +    migrate_set_error(migrate_get_current(), err);
>> +    error_free(err);
>> +    migration_cancel();
>> +}
>> +
>> +static RAMBlockNotifier ram_mig_ram_notifier = {
>> +    .ram_block_resized = ram_mig_ram_block_resized,
>> +};
>> +
>>  void ram_mig_init(void)
>>  {
>>      qemu_mutex_init(&XBZRLE.lock);
>>      register_savevm_live("ram", 0, 4, &savevm_ram_handlers, &ram_state);
>> +    ram_block_notifier_add(&ram_mig_ram_notifier);
> 
> Can we avoid the question of the 'is_idle' checks by doing this
> registration in save_setup/load_setup and unregistering in
> save_cleanup/load_cleanup?

Well, I figured it out now :)

migration_is_idle() is enough to handle it in this patch.

> 
> That means if we land in the handler we know we're in either an incoming
> or outgoing migration and then you just have to check which?

Can save/load race with other QEMU code that might register/unregister
notifiers? I want to avoid having to introduce locking just for that.

-- 
Thanks,

David / dhildenb

Re: [PATCH v1 05/13] migrate/ram: Handle RAM block resizes during precopy

[Dr. David Alan Gilbert]

* David Hildenbrand (david@redhat.com) wrote:
> On 21.02.20 16:14, Dr. David Alan Gilbert wrote:
> > * David Hildenbrand (david@redhat.com) wrote:
> >> Resizing while migrating is dangerous and does not work as expected.
> >> The whole migration code works on the usable_length of ram blocks and does
> >> not expect this to change at random points in time.
> >>
> >> In the case of precopy, the ram block size must not change on the source,
> >> after syncing the RAM block list in ram_save_setup(), so as long as the
> >> guest is still running on the source.
> >>
> >> Resizing can be trigger *after* (but not during) a reset in
> >> ACPI code by the guest
> >> - hw/arm/virt-acpi-build.c:acpi_ram_update()
> >> - hw/i386/acpi-build.c:acpi_ram_update()
> >>
> >> Use the ram block notifier to get notified about resizes. Let's simply
> >> cancel migration and indicate the reason. We'll continue running on the
> >> source. No harm done.
> >>
> >> Update the documentation. Postcopy will be handled separately.
> >>
> >> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >> Cc: Juan Quintela <quintela@redhat.com>
> >> Cc: Eduardo Habkost <ehabkost@redhat.com>
> >> Cc: Paolo Bonzini <pbonzini@redhat.com>
> >> Cc: Igor Mammedov <imammedo@redhat.com>
> >> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> >> Cc: Richard Henderson <richard.henderson@linaro.org>
> >> Cc: Shannon Zhao <shannon.zhao@linaro.org>
> >> Cc: Alex Bennée <alex.bennee@linaro.org>
> >> Cc: Peter Xu <peterx@redhat.com>
> >> Signed-off-by: David Hildenbrand <david@redhat.com>
> >> ---
> >>  exec.c                |  5 +++--
> >>  include/exec/memory.h | 10 ++++++----
> >>  migration/migration.c |  9 +++++++--
> >>  migration/migration.h |  1 +
> >>  migration/ram.c       | 41 +++++++++++++++++++++++++++++++++++++++++
> >>  5 files changed, 58 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/exec.c b/exec.c
> >> index b75250e773..8b015821d6 100644
> >> --- a/exec.c
> >> +++ b/exec.c
> >> @@ -2120,8 +2120,9 @@ static int memory_try_enable_merging(void *addr, size_t len)
> >>      return qemu_madvise(addr, len, QEMU_MADV_MERGEABLE);
> >>  }
> >>  
> >> -/* Only legal before guest might have detected the memory size: e.g. on
> >> - * incoming migration, or right after reset.
> >> +/*
> >> + * Resizing RAM while migrating can result in the migration being canceled.
> >> + * Care has to be taken if the guest might have already detected the memory.
> >>   *
> >>   * As memory core doesn't know how is memory accessed, it is up to
> >>   * resize callback to update device state and/or add assertions to detect
> >> diff --git a/include/exec/memory.h b/include/exec/memory.h
> >> index e85b7de99a..de111347e8 100644
> >> --- a/include/exec/memory.h
> >> +++ b/include/exec/memory.h
> >> @@ -113,7 +113,7 @@ typedef struct IOMMUNotifier IOMMUNotifier;
> >>  #define RAM_SHARED     (1 << 1)
> >>  
> >>  /* Only a portion of RAM (used_length) is actually used, and migrated.
> >> - * This used_length size can change across reboots.
> >> + * Resizing RAM while migrating can result in the migration being canceled.
> >>   */
> >>  #define RAM_RESIZEABLE (1 << 2)
> >>  
> >> @@ -843,7 +843,9 @@ void memory_region_init_ram_shared_nomigrate(MemoryRegion *mr,
> >>   *                                     RAM.  Accesses into the region will
> >>   *                                     modify memory directly.  Only an initial
> >>   *                                     portion of this RAM is actually used.
> >> - *                                     The used size can change across reboots.
> >> + *                                     Changing the size while migrating
> >> + *                                     can result in the migration being
> >> + *                                     canceled.
> >>   *
> >>   * @mr: the #MemoryRegion to be initialized.
> >>   * @owner: the object that tracks the region's reference count
> >> @@ -1464,8 +1466,8 @@ void *memory_region_get_ram_ptr(MemoryRegion *mr);
> >>  
> >>  /* memory_region_ram_resize: Resize a RAM region.
> >>   *
> >> - * Only legal before guest might have detected the memory size: e.g. on
> >> - * incoming migration, or right after reset.
> >> + * Resizing RAM while migrating can result in the migration being canceled.
> >> + * Care has to be taken if the guest might have already detected the memory.
> >>   *
> >>   * @mr: a memory region created with @memory_region_init_resizeable_ram.
> >>   * @newsize: the new size the region
> >> diff --git a/migration/migration.c b/migration/migration.c
> >> index 8fb68795dc..ac9751dbe5 100644
> >> --- a/migration/migration.c
> >> +++ b/migration/migration.c
> >> @@ -175,13 +175,18 @@ void migration_object_init(void)
> >>      }
> >>  }
> >>  
> >> +void migration_cancel(void)
> >> +{
> >> +    migrate_fd_cancel(current_migration);
> >> +}
> >> +
> >>  void migration_shutdown(void)
> >>  {
> >>      /*
> >>       * Cancel the current migration - that will (eventually)
> >>       * stop the migration using this structure
> >>       */
> >> -    migrate_fd_cancel(current_migration);
> >> +    migration_cancel();
> >>      object_unref(OBJECT(current_migration));
> >>  }
> >>  
> >> @@ -2019,7 +2024,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
> >>  
> >>  void qmp_migrate_cancel(Error **errp)
> >>  {
> >> -    migrate_fd_cancel(migrate_get_current());
> >> +    migration_cancel();
> >>  }
> >>  
> >>  void qmp_migrate_continue(MigrationStatus state, Error **errp)
> >> diff --git a/migration/migration.h b/migration/migration.h
> >> index 8473ddfc88..79fd74afa5 100644
> >> --- a/migration/migration.h
> >> +++ b/migration/migration.h
> >> @@ -343,5 +343,6 @@ int foreach_not_ignored_block(RAMBlockIterFunc func, void *opaque);
> >>  void migration_make_urgent_request(void);
> >>  void migration_consume_urgent_request(void);
> >>  bool migration_rate_limit(void);
> >> +void migration_cancel(void);
> >>  
> >>  #endif
> >> diff --git a/migration/ram.c b/migration/ram.c
> >> index ed23ed1c7c..57f32011a3 100644
> >> --- a/migration/ram.c
> >> +++ b/migration/ram.c
> >> @@ -52,6 +52,7 @@
> >>  #include "migration/colo.h"
> >>  #include "block.h"
> >>  #include "sysemu/sysemu.h"
> >> +#include "sysemu/runstate.h"
> >>  #include "savevm.h"
> >>  #include "qemu/iov.h"
> >>  #include "multifd.h"
> >> @@ -3710,8 +3711,48 @@ static SaveVMHandlers savevm_ram_handlers = {
> >>      .resume_prepare = ram_resume_prepare,
> >>  };
> >>  
> >> +static void ram_mig_ram_block_resized(RAMBlockNotifier *n, void *host,
> >> +                                      size_t old_size, size_t new_size)
> >> +{
> >> +    ram_addr_t offset;
> >> +    Error *err = NULL;
> >> +    RAMBlock *rb = qemu_ram_block_from_host(host, false, &offset);
> >> +
> >> +    if (ramblock_is_ignored(rb)) {
> >> +        return;
> >> +    }
> >> +
> >> +    /*
> >> +     * Some resizes are triggered on the migration target by precopy code,
> >> +     * when synchronizing RAM block sizes. In these cases, the VM is not
> >> +     * running and migration is not idle. We have to ignore these resizes,
> >> +     * as we only care about resizes during precopy on the migration source.
> >> +     * This handler is always registered, so ignore when migration is idle.
> >> +     */
> >> +    if (migration_is_idle() || !runstate_is_running() ||
> >> +        postcopy_is_running()) {
> >> +        return;
> >> +    }
> >> +
> >> +    /*
> >> +     * Precopy code cannot deal with the size of ram blocks changing at
> >> +     * random points in time. We're still running on the source, abort
> >> +     * the migration and continue running here. Make sure to wait until
> >> +     * migration was canceled.
> >> +     */
> >> +    error_setg(&err, "RAM block '%s' resized during precopy.", rb->idstr);
> >> +    migrate_set_error(migrate_get_current(), err);
> >> +    error_free(err);
> >> +    migration_cancel();
> >> +}
> >> +
> >> +static RAMBlockNotifier ram_mig_ram_notifier = {
> >> +    .ram_block_resized = ram_mig_ram_block_resized,
> >> +};
> >> +
> >>  void ram_mig_init(void)
> >>  {
> >>      qemu_mutex_init(&XBZRLE.lock);
> >>      register_savevm_live("ram", 0, 4, &savevm_ram_handlers, &ram_state);
> >> +    ram_block_notifier_add(&ram_mig_ram_notifier);
> > 
> > Can we avoid the question of the 'is_idle' checks by doing this
> > registration in save_setup/load_setup and unregistering in
> > save_cleanup/load_cleanup?
> 
> Well, I figured it out now :)
> 
> migration_is_idle() is enough to handle it in this patch.
> 
> > 
> > That means if we land in the handler we know we're in either an incoming
> > or outgoing migration and then you just have to check which?
> 
> Can save/load race with other QEMU code that might register/unregister
> notifiers? I want to avoid having to introduce locking just for that.

<looks at code> It looks like it can; qemu_savevm_state_setup
is called from near the top of migration_thread, so I don't
think it's holding locks at that point; and all the existing notifier
changes on this notifier seem to be done from either init or vfio_open
which I guess is under the big lock (?)

Dave


> -- 
> Thanks,
> 
> David / dhildenb
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v1 05/13] migrate/ram: Handle RAM block resizes during precopy

[David Hildenbrand]

On 21.02.20 16:40, Dr. David Alan Gilbert wrote:
> * David Hildenbrand (david@redhat.com) wrote:
>> On 21.02.20 16:14, Dr. David Alan Gilbert wrote:
>>> * David Hildenbrand (david@redhat.com) wrote:
>>>> Resizing while migrating is dangerous and does not work as expected.
>>>> The whole migration code works on the usable_length of ram blocks and does
>>>> not expect this to change at random points in time.
>>>>
>>>> In the case of precopy, the ram block size must not change on the source,
>>>> after syncing the RAM block list in ram_save_setup(), so as long as the
>>>> guest is still running on the source.
>>>>
>>>> Resizing can be trigger *after* (but not during) a reset in
>>>> ACPI code by the guest
>>>> - hw/arm/virt-acpi-build.c:acpi_ram_update()
>>>> - hw/i386/acpi-build.c:acpi_ram_update()
>>>>
>>>> Use the ram block notifier to get notified about resizes. Let's simply
>>>> cancel migration and indicate the reason. We'll continue running on the
>>>> source. No harm done.
>>>>
>>>> Update the documentation. Postcopy will be handled separately.
>>>>
>>>> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>>>> Cc: Juan Quintela <quintela@redhat.com>
>>>> Cc: Eduardo Habkost <ehabkost@redhat.com>
>>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>>> Cc: Igor Mammedov <imammedo@redhat.com>
>>>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>>>> Cc: Richard Henderson <richard.henderson@linaro.org>
>>>> Cc: Shannon Zhao <shannon.zhao@linaro.org>
>>>> Cc: Alex Bennée <alex.bennee@linaro.org>
>>>> Cc: Peter Xu <peterx@redhat.com>
>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>> ---
>>>>  exec.c                |  5 +++--
>>>>  include/exec/memory.h | 10 ++++++----
>>>>  migration/migration.c |  9 +++++++--
>>>>  migration/migration.h |  1 +
>>>>  migration/ram.c       | 41 +++++++++++++++++++++++++++++++++++++++++
>>>>  5 files changed, 58 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/exec.c b/exec.c
>>>> index b75250e773..8b015821d6 100644
>>>> --- a/exec.c
>>>> +++ b/exec.c
>>>> @@ -2120,8 +2120,9 @@ static int memory_try_enable_merging(void *addr, size_t len)
>>>>      return qemu_madvise(addr, len, QEMU_MADV_MERGEABLE);
>>>>  }
>>>>  
>>>> -/* Only legal before guest might have detected the memory size: e.g. on
>>>> - * incoming migration, or right after reset.
>>>> +/*
>>>> + * Resizing RAM while migrating can result in the migration being canceled.
>>>> + * Care has to be taken if the guest might have already detected the memory.
>>>>   *
>>>>   * As memory core doesn't know how is memory accessed, it is up to
>>>>   * resize callback to update device state and/or add assertions to detect
>>>> diff --git a/include/exec/memory.h b/include/exec/memory.h
>>>> index e85b7de99a..de111347e8 100644
>>>> --- a/include/exec/memory.h
>>>> +++ b/include/exec/memory.h
>>>> @@ -113,7 +113,7 @@ typedef struct IOMMUNotifier IOMMUNotifier;
>>>>  #define RAM_SHARED     (1 << 1)
>>>>  
>>>>  /* Only a portion of RAM (used_length) is actually used, and migrated.
>>>> - * This used_length size can change across reboots.
>>>> + * Resizing RAM while migrating can result in the migration being canceled.
>>>>   */
>>>>  #define RAM_RESIZEABLE (1 << 2)
>>>>  
>>>> @@ -843,7 +843,9 @@ void memory_region_init_ram_shared_nomigrate(MemoryRegion *mr,
>>>>   *                                     RAM.  Accesses into the region will
>>>>   *                                     modify memory directly.  Only an initial
>>>>   *                                     portion of this RAM is actually used.
>>>> - *                                     The used size can change across reboots.
>>>> + *                                     Changing the size while migrating
>>>> + *                                     can result in the migration being
>>>> + *                                     canceled.
>>>>   *
>>>>   * @mr: the #MemoryRegion to be initialized.
>>>>   * @owner: the object that tracks the region's reference count
>>>> @@ -1464,8 +1466,8 @@ void *memory_region_get_ram_ptr(MemoryRegion *mr);
>>>>  
>>>>  /* memory_region_ram_resize: Resize a RAM region.
>>>>   *
>>>> - * Only legal before guest might have detected the memory size: e.g. on
>>>> - * incoming migration, or right after reset.
>>>> + * Resizing RAM while migrating can result in the migration being canceled.
>>>> + * Care has to be taken if the guest might have already detected the memory.
>>>>   *
>>>>   * @mr: a memory region created with @memory_region_init_resizeable_ram.
>>>>   * @newsize: the new size the region
>>>> diff --git a/migration/migration.c b/migration/migration.c
>>>> index 8fb68795dc..ac9751dbe5 100644
>>>> --- a/migration/migration.c
>>>> +++ b/migration/migration.c
>>>> @@ -175,13 +175,18 @@ void migration_object_init(void)
>>>>      }
>>>>  }
>>>>  
>>>> +void migration_cancel(void)
>>>> +{
>>>> +    migrate_fd_cancel(current_migration);
>>>> +}
>>>> +
>>>>  void migration_shutdown(void)
>>>>  {
>>>>      /*
>>>>       * Cancel the current migration - that will (eventually)
>>>>       * stop the migration using this structure
>>>>       */
>>>> -    migrate_fd_cancel(current_migration);
>>>> +    migration_cancel();
>>>>      object_unref(OBJECT(current_migration));
>>>>  }
>>>>  
>>>> @@ -2019,7 +2024,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>>>>  
>>>>  void qmp_migrate_cancel(Error **errp)
>>>>  {
>>>> -    migrate_fd_cancel(migrate_get_current());
>>>> +    migration_cancel();
>>>>  }
>>>>  
>>>>  void qmp_migrate_continue(MigrationStatus state, Error **errp)
>>>> diff --git a/migration/migration.h b/migration/migration.h
>>>> index 8473ddfc88..79fd74afa5 100644
>>>> --- a/migration/migration.h
>>>> +++ b/migration/migration.h
>>>> @@ -343,5 +343,6 @@ int foreach_not_ignored_block(RAMBlockIterFunc func, void *opaque);
>>>>  void migration_make_urgent_request(void);
>>>>  void migration_consume_urgent_request(void);
>>>>  bool migration_rate_limit(void);
>>>> +void migration_cancel(void);
>>>>  
>>>>  #endif
>>>> diff --git a/migration/ram.c b/migration/ram.c
>>>> index ed23ed1c7c..57f32011a3 100644
>>>> --- a/migration/ram.c
>>>> +++ b/migration/ram.c
>>>> @@ -52,6 +52,7 @@
>>>>  #include "migration/colo.h"
>>>>  #include "block.h"
>>>>  #include "sysemu/sysemu.h"
>>>> +#include "sysemu/runstate.h"
>>>>  #include "savevm.h"
>>>>  #include "qemu/iov.h"
>>>>  #include "multifd.h"
>>>> @@ -3710,8 +3711,48 @@ static SaveVMHandlers savevm_ram_handlers = {
>>>>      .resume_prepare = ram_resume_prepare,
>>>>  };
>>>>  
>>>> +static void ram_mig_ram_block_resized(RAMBlockNotifier *n, void *host,
>>>> +                                      size_t old_size, size_t new_size)
>>>> +{
>>>> +    ram_addr_t offset;
>>>> +    Error *err = NULL;
>>>> +    RAMBlock *rb = qemu_ram_block_from_host(host, false, &offset);
>>>> +
>>>> +    if (ramblock_is_ignored(rb)) {
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    /*
>>>> +     * Some resizes are triggered on the migration target by precopy code,
>>>> +     * when synchronizing RAM block sizes. In these cases, the VM is not
>>>> +     * running and migration is not idle. We have to ignore these resizes,
>>>> +     * as we only care about resizes during precopy on the migration source.
>>>> +     * This handler is always registered, so ignore when migration is idle.
>>>> +     */
>>>> +    if (migration_is_idle() || !runstate_is_running() ||
>>>> +        postcopy_is_running()) {
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    /*
>>>> +     * Precopy code cannot deal with the size of ram blocks changing at
>>>> +     * random points in time. We're still running on the source, abort
>>>> +     * the migration and continue running here. Make sure to wait until
>>>> +     * migration was canceled.
>>>> +     */
>>>> +    error_setg(&err, "RAM block '%s' resized during precopy.", rb->idstr);
>>>> +    migrate_set_error(migrate_get_current(), err);
>>>> +    error_free(err);
>>>> +    migration_cancel();
>>>> +}
>>>> +
>>>> +static RAMBlockNotifier ram_mig_ram_notifier = {
>>>> +    .ram_block_resized = ram_mig_ram_block_resized,
>>>> +};
>>>> +
>>>>  void ram_mig_init(void)
>>>>  {
>>>>      qemu_mutex_init(&XBZRLE.lock);
>>>>      register_savevm_live("ram", 0, 4, &savevm_ram_handlers, &ram_state);
>>>> +    ram_block_notifier_add(&ram_mig_ram_notifier);
>>>
>>> Can we avoid the question of the 'is_idle' checks by doing this
>>> registration in save_setup/load_setup and unregistering in
>>> save_cleanup/load_cleanup?
>>
>> Well, I figured it out now :)
>>
>> migration_is_idle() is enough to handle it in this patch.
>>
>>>
>>> That means if we land in the handler we know we're in either an incoming
>>> or outgoing migration and then you just have to check which?
>>
>> Can save/load race with other QEMU code that might register/unregister
>> notifiers? I want to avoid having to introduce locking just for that.
> 
> <looks at code> It looks like it can; qemu_savevm_state_setup
> is called from near the top of migration_thread, so I don't
> think it's holding locks at that point; and all the existing notifier
> changes on this notifier seem to be done from either init or vfio_open
> which I guess is under the big lock (?)

Yes, same as actual resizes AFAIK (which could otherwise also race with
(un)registration when traversing the list). So I'd prefer to not
register the notifier dynamically for now. (at least makes it easier for
me :) )


-- 
Thanks,

David / dhildenb

[PATCH v1 06/13] migrate/ram: Discard new RAM when growing RAM blocks and the VM is stopped

[David Hildenbrand]

In case we grow our RAM after ram_postcopy_incoming_init() (e.g., when
synchronizing the RAM block state with the migration source), the resized
part would not get discarded. Let's perform that when being notified
about a resize while postcopy has been advised and the guest is not
running yet.

Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
Cc: Juan Quintela <quintela@redhat.com>
Cc: Peter Xu <peterx@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 migration/ram.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/migration/ram.c b/migration/ram.c
index 57f32011a3..cbd54947fb 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -3722,6 +3722,25 @@ static void ram_mig_ram_block_resized(RAMBlockNotifier *n, void *host,
         return;
     }
 
+    /*
+     * Especially at the start of precopy on the migration target, before
+     * starting postcopy, we synchronize the RAM block sizes. Let's make sure
+     * that any resizes before starting the guest are properly handled by
+     * postcopy. Note: All other postcopy handling (e.g., registering handlers,
+     * disabling THP) happens after all resizes (e.g., during precopy) were
+     * performed.
+     */
+    if (postcopy_is_advised() && !runstate_is_running()) {
+        /* Update what ram_postcopy_incoming_init()->init_range() does. */
+        if (old_size < new_size) {
+            if (ram_discard_range(rb->idstr, old_size, new_size - old_size)) {
+                error_report("RAM block '%s' discard of resized RAM failed",
+                             rb->idstr);
+            }
+        }
+        return;
+    }
+
     /*
      * Some resizes are triggered on the migration target by precopy code,
      * when synchronizing RAM block sizes. In these cases, the VM is not
-- 
2.24.1

Re: [PATCH v1 06/13] migrate/ram: Discard new RAM when growing RAM blocks and the VM is stopped

[David Hildenbrand]

On 19.02.20 17:17, David Hildenbrand wrote:
> In case we grow our RAM after ram_postcopy_incoming_init() (e.g., when
> synchronizing the RAM block state with the migration source), the resized
> part would not get discarded. Let's perform that when being notified
> about a resize while postcopy has been advised and the guest is not
> running yet.
> 
> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> Cc: Juan Quintela <quintela@redhat.com>
> Cc: Peter Xu <peterx@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  migration/ram.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index 57f32011a3..cbd54947fb 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -3722,6 +3722,25 @@ static void ram_mig_ram_block_resized(RAMBlockNotifier *n, void *host,
>          return;
>      }
>  
> +    /*
> +     * Especially at the start of precopy on the migration target, before
> +     * starting postcopy, we synchronize the RAM block sizes. Let's make sure
> +     * that any resizes before starting the guest are properly handled by
> +     * postcopy. Note: All other postcopy handling (e.g., registering handlers,
> +     * disabling THP) happens after all resizes (e.g., during precopy) were
> +     * performed.
> +     */

I think it would be clearer to do a

ps >= POSTCOPY_INCOMING_ADVISE && ps < POSTCOPY_INCOMING_RUNNING

We really only want to do something when psotcopy has been advised but
the guest is not running yet.

Will look into that as I find ways to actually test this :)

-- 
Thanks,

David / dhildenb

Re: [PATCH v1 06/13] migrate/ram: Discard new RAM when growing RAM blocks and the VM is stopped

[Dr. David Alan Gilbert]

* David Hildenbrand (david@redhat.com) wrote:
> On 19.02.20 17:17, David Hildenbrand wrote:
> > In case we grow our RAM after ram_postcopy_incoming_init() (e.g., when
> > synchronizing the RAM block state with the migration source), the resized
> > part would not get discarded. Let's perform that when being notified
> > about a resize while postcopy has been advised and the guest is not
> > running yet.
> > 
> > Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > Cc: Juan Quintela <quintela@redhat.com>
> > Cc: Peter Xu <peterx@redhat.com>
> > Signed-off-by: David Hildenbrand <david@redhat.com>
> > ---
> >  migration/ram.c | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> > 
> > diff --git a/migration/ram.c b/migration/ram.c
> > index 57f32011a3..cbd54947fb 100644
> > --- a/migration/ram.c
> > +++ b/migration/ram.c
> > @@ -3722,6 +3722,25 @@ static void ram_mig_ram_block_resized(RAMBlockNotifier *n, void *host,
> >          return;
> >      }
> >  
> > +    /*
> > +     * Especially at the start of precopy on the migration target, before
> > +     * starting postcopy, we synchronize the RAM block sizes. Let's make sure
> > +     * that any resizes before starting the guest are properly handled by
> > +     * postcopy. Note: All other postcopy handling (e.g., registering handlers,
> > +     * disabling THP) happens after all resizes (e.g., during precopy) were
> > +     * performed.
> > +     */
> 
> I think it would be clearer to do a
> 
> ps >= POSTCOPY_INCOMING_ADVISE && ps < POSTCOPY_INCOMING_RUNNING
> 
> We really only want to do something when psotcopy has been advised but
> the guest is not running yet.
> 
> Will look into that as I find ways to actually test this :)

Should that be < POSTCOPY_INCOMING_LISTENING - i.e. before the
userfaultfd has been enabled on the region?

Dave

> 
> -- 
> Thanks,
> 
> David / dhildenb
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v1 06/13] migrate/ram: Discard new RAM when growing RAM blocks and the VM is stopped

[David Hildenbrand]

On 21.02.20 16:51, Dr. David Alan Gilbert wrote:
> * David Hildenbrand (david@redhat.com) wrote:
>> On 19.02.20 17:17, David Hildenbrand wrote:
>>> In case we grow our RAM after ram_postcopy_incoming_init() (e.g., when
>>> synchronizing the RAM block state with the migration source), the resized
>>> part would not get discarded. Let's perform that when being notified
>>> about a resize while postcopy has been advised and the guest is not
>>> running yet.
>>>
>>> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>>> Cc: Juan Quintela <quintela@redhat.com>
>>> Cc: Peter Xu <peterx@redhat.com>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>>  migration/ram.c | 19 +++++++++++++++++++
>>>  1 file changed, 19 insertions(+)
>>>
>>> diff --git a/migration/ram.c b/migration/ram.c
>>> index 57f32011a3..cbd54947fb 100644
>>> --- a/migration/ram.c
>>> +++ b/migration/ram.c
>>> @@ -3722,6 +3722,25 @@ static void ram_mig_ram_block_resized(RAMBlockNotifier *n, void *host,
>>>          return;
>>>      }
>>>  
>>> +    /*
>>> +     * Especially at the start of precopy on the migration target, before
>>> +     * starting postcopy, we synchronize the RAM block sizes. Let's make sure
>>> +     * that any resizes before starting the guest are properly handled by
>>> +     * postcopy. Note: All other postcopy handling (e.g., registering handlers,
>>> +     * disabling THP) happens after all resizes (e.g., during precopy) were
>>> +     * performed.
>>> +     */
>>
>> I think it would be clearer to do a
>>
>> ps >= POSTCOPY_INCOMING_ADVISE && ps < POSTCOPY_INCOMING_RUNNING
>>
>> We really only want to do something when psotcopy has been advised but
>> the guest is not running yet.
>>
>> Will look into that as I find ways to actually test this :)
> 
> Should that be < POSTCOPY_INCOMING_LISTENING - i.e. before the
> userfaultfd has been enabled on the region?
> 

We can be even stricter. I have in my current patch:

diff --git a/migration/ram.c b/migration/ram.c
index 39c7d1c4a6..1ccb40970e 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -3714,6 +3714,7 @@ static SaveVMHandlers savevm_ram_handlers = {
 static void ram_mig_ram_block_resized(RAMBlockNotifier *n, void *host,
                                       size_t old_size, size_t new_size)
 {
+    PostcopyState ps = postcopy_state_get();
     ram_addr_t offset;
     Error *err = NULL;
     RAMBlock *rb = qemu_ram_block_from_host(host, false, &offset);
@@ -3734,6 +3735,34 @@ static void ram_mig_ram_block_resized(RAMBlockNotifier *n, void *host,
         error_free(err);
         migration_cancel();
     }
+
+    switch (ps) {
+    case POSTCOPY_INCOMING_ADVISE:
+        /*
+         * Update what ram_postcopy_incoming_init()->init_range() does at the
+         * time postcopy was advised. Syncing RAM blocks with the source will
+         * result in RAM resizes.
+         */
+        if (old_size < new_size) {
+            if (ram_discard_range(rb->idstr, old_size, new_size - old_size)) {
+                error_report("RAM block '%s' discard of resized RAM failed",
+                             rb->idstr);
+            }
+        }
+        break;
+    case POSTCOPY_INCOMING_NONE:
+    case POSTCOPY_INCOMING_RUNNING:
+    case POSTCOPY_INCOMING_END:
+        /*
+         * Once our guest is running, postcopy does no longer care about
+         * resizes. When growing, the new memory was not available on the
+         * source, no handler needed.
+         */
+        break;
+    default:
+        error_report("Unexpected RAM resize during postcopy state: %d", ps);
+        exit(-1);
+    }
 }



-- 
Thanks,

David / dhildenb

[PATCH v1 07/13] migrate/ram: Get rid of "place_source" in ram_load_postcopy()

[David Hildenbrand]

It's always the same value.

Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
Cc: Juan Quintela <quintela@redhat.com>
Cc: Peter Xu <peterx@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 migration/ram.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index cbd54947fb..75014717f6 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -3119,7 +3119,6 @@ static int ram_load_postcopy(QEMUFile *f)
         ram_addr_t addr;
         void *host = NULL;
         void *page_buffer = NULL;
-        void *place_source = NULL;
         RAMBlock *block = NULL;
         uint8_t ch;
         int len;
@@ -3188,7 +3187,6 @@ static int ram_load_postcopy(QEMUFile *f)
                 place_needed = true;
                 target_pages = 0;
             }
-            place_source = postcopy_host_page;
         }
 
         switch (flags & ~RAM_SAVE_FLAG_CONTINUE) {
@@ -3220,7 +3218,7 @@ static int ram_load_postcopy(QEMUFile *f)
                  * buffer to make sure the buffer is valid when
                  * placing the page.
                  */
-                qemu_get_buffer_in_place(f, (uint8_t **)&place_source,
+                qemu_get_buffer_in_place(f, (uint8_t **)&postcopy_host_page,
                                          TARGET_PAGE_SIZE);
             }
             break;
@@ -3265,8 +3263,8 @@ static int ram_load_postcopy(QEMUFile *f)
                 ret = postcopy_place_page_zero(mis, place_dest,
                                                block);
             } else {
-                ret = postcopy_place_page(mis, place_dest,
-                                          place_source, block);
+                ret = postcopy_place_page(mis, place_dest, postcopy_host_page,
+                                          block);
             }
         }
     }
-- 
2.24.1

Re: [PATCH v1 07/13] migrate/ram: Get rid of "place_source" in ram_load_postcopy()

[Peter Xu]

On Wed, Feb 19, 2020 at 05:17:19PM +0100, David Hildenbrand wrote:
> It's always the same value.

I guess not, because...

> 
> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> Cc: Juan Quintela <quintela@redhat.com>
> Cc: Peter Xu <peterx@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  migration/ram.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index cbd54947fb..75014717f6 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -3119,7 +3119,6 @@ static int ram_load_postcopy(QEMUFile *f)
>          ram_addr_t addr;
>          void *host = NULL;
>          void *page_buffer = NULL;
> -        void *place_source = NULL;
>          RAMBlock *block = NULL;
>          uint8_t ch;
>          int len;
> @@ -3188,7 +3187,6 @@ static int ram_load_postcopy(QEMUFile *f)
>                  place_needed = true;
>                  target_pages = 0;
>              }
> -            place_source = postcopy_host_page;
>          }
>  
>          switch (flags & ~RAM_SAVE_FLAG_CONTINUE) {
> @@ -3220,7 +3218,7 @@ static int ram_load_postcopy(QEMUFile *f)
>                   * buffer to make sure the buffer is valid when
>                   * placing the page.
>                   */
> -                qemu_get_buffer_in_place(f, (uint8_t **)&place_source,

... it can be modified inside the call.

I feel like this patch could even fail the QEMU unit test.  It would
be good to mention what tests have been carried out in the cover
letter or with RFC tag if no test is done yet.

For a series like this, I'll try at least the unit tests and smoke on
both precopy and postcopy.  The resizing test would be even better but
seems untrivial, so maybe optional.

Thanks,

> +                qemu_get_buffer_in_place(f, (uint8_t **)&postcopy_host_page,
>                                           TARGET_PAGE_SIZE);
>              }
>              break;
> @@ -3265,8 +3263,8 @@ static int ram_load_postcopy(QEMUFile *f)
>                  ret = postcopy_place_page_zero(mis, place_dest,
>                                                 block);
>              } else {
> -                ret = postcopy_place_page(mis, place_dest,
> -                                          place_source, block);
> +                ret = postcopy_place_page(mis, place_dest, postcopy_host_page,
> +                                          block);
>              }
>          }
>      }
> -- 
> 2.24.1
> 

-- 
Peter Xu

Re: [PATCH v1 07/13] migrate/ram: Get rid of "place_source" in ram_load_postcopy()

[Peter Xu]

On Wed, Feb 19, 2020 at 03:47:30PM -0500, Peter Xu wrote:
> On Wed, Feb 19, 2020 at 05:17:19PM +0100, David Hildenbrand wrote:
> > It's always the same value.
> 
> I guess not, because...
> 
> > 
> > Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > Cc: Juan Quintela <quintela@redhat.com>
> > Cc: Peter Xu <peterx@redhat.com>
> > Signed-off-by: David Hildenbrand <david@redhat.com>
> > ---
> >  migration/ram.c | 8 +++-----
> >  1 file changed, 3 insertions(+), 5 deletions(-)
> > 
> > diff --git a/migration/ram.c b/migration/ram.c
> > index cbd54947fb..75014717f6 100644
> > --- a/migration/ram.c
> > +++ b/migration/ram.c
> > @@ -3119,7 +3119,6 @@ static int ram_load_postcopy(QEMUFile *f)
> >          ram_addr_t addr;
> >          void *host = NULL;
> >          void *page_buffer = NULL;
> > -        void *place_source = NULL;
> >          RAMBlock *block = NULL;
> >          uint8_t ch;
> >          int len;
> > @@ -3188,7 +3187,6 @@ static int ram_load_postcopy(QEMUFile *f)
> >                  place_needed = true;
> >                  target_pages = 0;
> >              }
> > -            place_source = postcopy_host_page;
> >          }
> >  
> >          switch (flags & ~RAM_SAVE_FLAG_CONTINUE) {
> > @@ -3220,7 +3218,7 @@ static int ram_load_postcopy(QEMUFile *f)
> >                   * buffer to make sure the buffer is valid when
> >                   * placing the page.
> >                   */
> > -                qemu_get_buffer_in_place(f, (uint8_t **)&place_source,
> 
> ... it can be modified inside the call.
> 
> I feel like this patch could even fail the QEMU unit test.  It would
> be good to mention what tests have been carried out in the cover
> letter or with RFC tag if no test is done yet.
> 
> For a series like this, I'll try at least the unit tests and smoke on
> both precopy and postcopy.  The resizing test would be even better but
> seems untrivial, so maybe optional.

For resizing test, an easy way (I can think of) is to temporarily
remove the size check below in your test branch:

diff --git a/exec.c b/exec.c
index 8e9cc3b47c..17dc660281 100644
--- a/exec.c
+++ b/exec.c
@@ -2128,10 +2128,6 @@ int qemu_ram_resize(RAMBlock *block, ram_addr_t newsize, Error **errp)
 
     newsize = HOST_PAGE_ALIGN(newsize);
 
-    if (block->used_length == newsize) {
-        return 0;
-    }
-
     if (!(block->flags & RAM_RESIZEABLE)) {
         error_setg_errno(errp, EINVAL,
                          "Length mismatch: %s: 0x" RAM_ADDR_FMT

Then reboot the guest during migration to see whether precopy can be
cancelled smoothly. And same to postcopy.  Thanks,

-- 
Peter Xu

Re: [PATCH v1 07/13] migrate/ram: Get rid of "place_source" in ram_load_postcopy()

[David Hildenbrand]

On 19.02.20 21:55, Peter Xu wrote:
> On Wed, Feb 19, 2020 at 03:47:30PM -0500, Peter Xu wrote:
>> On Wed, Feb 19, 2020 at 05:17:19PM +0100, David Hildenbrand wrote:
>>> It's always the same value.
>>
>> I guess not, because...
>>
>>>
>>> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>>> Cc: Juan Quintela <quintela@redhat.com>
>>> Cc: Peter Xu <peterx@redhat.com>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>>  migration/ram.c | 8 +++-----
>>>  1 file changed, 3 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/migration/ram.c b/migration/ram.c
>>> index cbd54947fb..75014717f6 100644
>>> --- a/migration/ram.c
>>> +++ b/migration/ram.c
>>> @@ -3119,7 +3119,6 @@ static int ram_load_postcopy(QEMUFile *f)
>>>          ram_addr_t addr;
>>>          void *host = NULL;
>>>          void *page_buffer = NULL;
>>> -        void *place_source = NULL;
>>>          RAMBlock *block = NULL;
>>>          uint8_t ch;
>>>          int len;
>>> @@ -3188,7 +3187,6 @@ static int ram_load_postcopy(QEMUFile *f)
>>>                  place_needed = true;
>>>                  target_pages = 0;
>>>              }
>>> -            place_source = postcopy_host_page;
>>>          }
>>>  
>>>          switch (flags & ~RAM_SAVE_FLAG_CONTINUE) {
>>> @@ -3220,7 +3218,7 @@ static int ram_load_postcopy(QEMUFile *f)
>>>                   * buffer to make sure the buffer is valid when
>>>                   * placing the page.
>>>                   */
>>> -                qemu_get_buffer_in_place(f, (uint8_t **)&place_source,
>>
>> ... it can be modified inside the call.
>>
>> I feel like this patch could even fail the QEMU unit test.  It would
>> be good to mention what tests have been carried out in the cover
>> letter or with RFC tag if no test is done yet.
>>
>> For a series like this, I'll try at least the unit tests and smoke on
>> both precopy and postcopy.  The resizing test would be even better but
>> seems untrivial, so maybe optional.
> 
> For resizing test, an easy way (I can think of) is to temporarily
> remove the size check below in your test branch:

Yeah, it's especially hard to have a reliable test one can materialize.
I played with manual tests like this myself.

I'm thinking about testing this with a device that can trigger resizes
on demand, e.g., virtio-mem, for now on my private branch. But I'd
really like to have some test one can automate at one point ... however,
there seems to be no easy way to achieve that right now.

Thanks!

-- 
Thanks,

David / dhildenb

Re: [PATCH v1 07/13] migrate/ram: Get rid of "place_source" in ram_load_postcopy()

[Peter Xu]

On Thu, Feb 20, 2020 at 02:22:48PM +0100, David Hildenbrand wrote:
> > For resizing test, an easy way (I can think of) is to temporarily
> > remove the size check below in your test branch:
> 
> Yeah, it's especially hard to have a reliable test one can materialize.
> I played with manual tests like this myself.

Great!

> 
> I'm thinking about testing this with a device that can trigger resizes
> on demand, e.g., virtio-mem, for now on my private branch. But I'd
> really like to have some test one can automate at one point ... however,
> there seems to be no easy way to achieve that right now.

Yes I totally agree.

Thanks,

-- 
Peter Xu

Re: [PATCH v1 07/13] migrate/ram: Get rid of "place_source" in ram_load_postcopy()

[David Hildenbrand]

> Am 19.02.2020 um 21:47 schrieb Peter Xu <peterx@redhat.com>:
> 
> On Wed, Feb 19, 2020 at 05:17:19PM +0100, David Hildenbrand wrote:
>> It's always the same value.
> 
> I guess not, because...
> 
>> 
>> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>> Cc: Juan Quintela <quintela@redhat.com>
>> Cc: Peter Xu <peterx@redhat.com>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>> migration/ram.c | 8 +++-----
>> 1 file changed, 3 insertions(+), 5 deletions(-)
>> 
>> diff --git a/migration/ram.c b/migration/ram.c
>> index cbd54947fb..75014717f6 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -3119,7 +3119,6 @@ static int ram_load_postcopy(QEMUFile *f)
>>         ram_addr_t addr;
>>         void *host = NULL;
>>         void *page_buffer = NULL;
>> -        void *place_source = NULL;
>>         RAMBlock *block = NULL;
>>         uint8_t ch;
>>         int len;
>> @@ -3188,7 +3187,6 @@ static int ram_load_postcopy(QEMUFile *f)
>>                 place_needed = true;
>>                 target_pages = 0;
>>             }
>> -            place_source = postcopy_host_page;
>>         }
>> 
>>         switch (flags & ~RAM_SAVE_FLAG_CONTINUE) {
>> @@ -3220,7 +3218,7 @@ static int ram_load_postcopy(QEMUFile *f)
>>                  * buffer to make sure the buffer is valid when
>>                  * placing the page.
>>                  */
>> -                qemu_get_buffer_in_place(f, (uint8_t **)&place_source,
> 
> ... it can be modified inside the call.

Very right, will drop this patch! Thanks!

> 
> I feel like this patch could even fail the QEMU unit test.  It would
> be good to mention what tests have been carried out in the cover
> letter or with RFC tag if no test is done yet.

I test all code I share. This survives „make check“. I assume all tests send small pages where „matches_target_page_size==true“, so the tests did not catch this.

I even spent the last day getting avocado-vt to work and ran multiple (obviously not all) migration tests, including postcopy, so your suggestions have already been considered ...

Could have mentioned that in the cover letter, yes.

Re: [PATCH v1 07/13] migrate/ram: Get rid of "place_source" in ram_load_postcopy()

[Dr. David Alan Gilbert]

* David Hildenbrand (david@redhat.com) wrote:
> 
> 
> > Am 19.02.2020 um 21:47 schrieb Peter Xu <peterx@redhat.com>:
> > 
> > On Wed, Feb 19, 2020 at 05:17:19PM +0100, David Hildenbrand wrote:
> >> It's always the same value.
> > 
> > I guess not, because...
> > 
> >> 
> >> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >> Cc: Juan Quintela <quintela@redhat.com>
> >> Cc: Peter Xu <peterx@redhat.com>
> >> Signed-off-by: David Hildenbrand <david@redhat.com>
> >> ---
> >> migration/ram.c | 8 +++-----
> >> 1 file changed, 3 insertions(+), 5 deletions(-)
> >> 
> >> diff --git a/migration/ram.c b/migration/ram.c
> >> index cbd54947fb..75014717f6 100644
> >> --- a/migration/ram.c
> >> +++ b/migration/ram.c
> >> @@ -3119,7 +3119,6 @@ static int ram_load_postcopy(QEMUFile *f)
> >>         ram_addr_t addr;
> >>         void *host = NULL;
> >>         void *page_buffer = NULL;
> >> -        void *place_source = NULL;
> >>         RAMBlock *block = NULL;
> >>         uint8_t ch;
> >>         int len;
> >> @@ -3188,7 +3187,6 @@ static int ram_load_postcopy(QEMUFile *f)
> >>                 place_needed = true;
> >>                 target_pages = 0;
> >>             }
> >> -            place_source = postcopy_host_page;
> >>         }
> >> 
> >>         switch (flags & ~RAM_SAVE_FLAG_CONTINUE) {
> >> @@ -3220,7 +3218,7 @@ static int ram_load_postcopy(QEMUFile *f)
> >>                  * buffer to make sure the buffer is valid when
> >>                  * placing the page.
> >>                  */
> >> -                qemu_get_buffer_in_place(f, (uint8_t **)&place_source,
> > 
> > ... it can be modified inside the call.
> 
> Very right, will drop this patch! Thanks!
> 
> > 
> > I feel like this patch could even fail the QEMU unit test.  It would
> > be good to mention what tests have been carried out in the cover
> > letter or with RFC tag if no test is done yet.
> 
> I test all code I share. This survives „make check“. I assume all tests send small pages where „matches_target_page_size==true“, so the tests did not catch this.
> 
> I even spent the last day getting avocado-vt to work and ran multiple (obviously not all) migration tests, including postcopy, so your suggestions have already been considered ...

A test on Power or aarch might catch this one; where they normally
have larger pages.

Dave

> Could have mentioned that in the cover letter, yes.
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[PATCH v1 08/13] migrate/ram: Simplify host page handling in ram_load_postcopy()

[David Hildenbrand]

Add two new helper functions. This will in come handy once we want to
handle ram block resizes while postcopy is active.

Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
Cc: Juan Quintela <quintela@redhat.com>
Cc: Peter Xu <peterx@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 migration/ram.c | 52 ++++++++++++++++++++++++++++---------------------
 1 file changed, 30 insertions(+), 22 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 75014717f6..80a4e4a9ea 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2734,6 +2734,20 @@ static inline void *host_from_ram_block_offset(RAMBlock *block,
     return block->host + offset;
 }
 
+static void *host_page_from_ram_block_offset(RAMBlock *block,
+                                             ram_addr_t offset)
+{
+    /* Note: Explicitly no check against offset_in_ramblock(). */
+    return (void *)QEMU_ALIGN_DOWN((uintptr_t)block->host + offset,
+                                   block->page_size);
+}
+
+static ram_addr_t host_page_offset_from_ram_block_offset(RAMBlock *block,
+                                                         ram_addr_t offset)
+{
+    return ((uintptr_t)block->host + offset) & (block->page_size - 1);
+}
+
 static inline void *colo_cache_from_block_offset(RAMBlock *block,
                                                  ram_addr_t offset)
 {
@@ -3111,13 +3125,12 @@ static int ram_load_postcopy(QEMUFile *f)
     MigrationIncomingState *mis = migration_incoming_get_current();
     /* Temporary page that is later 'placed' */
     void *postcopy_host_page = mis->postcopy_tmp_page;
-    void *this_host = NULL;
+    void *host_page = NULL;
     bool all_zero = false;
     int target_pages = 0;
 
     while (!ret && !(flags & RAM_SAVE_FLAG_EOS)) {
         ram_addr_t addr;
-        void *host = NULL;
         void *page_buffer = NULL;
         RAMBlock *block = NULL;
         uint8_t ch;
@@ -3142,9 +3155,12 @@ static int ram_load_postcopy(QEMUFile *f)
         if (flags & (RAM_SAVE_FLAG_ZERO | RAM_SAVE_FLAG_PAGE |
                      RAM_SAVE_FLAG_COMPRESS_PAGE)) {
             block = ram_block_from_stream(f, flags);
+            if (!block) {
+                ret = -EINVAL;
+                break;
+            }
 
-            host = host_from_ram_block_offset(block, addr);
-            if (!host) {
+            if (!offset_in_ramblock(block, addr)) {
                 error_report("Illegal RAM offset " RAM_ADDR_FMT, addr);
                 ret = -EINVAL;
                 break;
@@ -3162,21 +3178,18 @@ static int ram_load_postcopy(QEMUFile *f)
              * of a host page in one chunk.
              */
             page_buffer = postcopy_host_page +
-                          ((uintptr_t)host & (block->page_size - 1));
+                          host_page_offset_from_ram_block_offset(block, addr);
             /* If all TP are zero then we can optimise the place */
             if (target_pages == 1) {
                 all_zero = true;
-                this_host = (void *)QEMU_ALIGN_DOWN((uintptr_t)host,
-                                                    block->page_size);
-            } else {
+                host_page = host_page_from_ram_block_offset(block, addr);
+            } else if (host_page != host_page_from_ram_block_offset(block,
+                                                                    addr)) {
                 /* not the 1st TP within the HP */
-                if (QEMU_ALIGN_DOWN((uintptr_t)host, block->page_size) !=
-                    (uintptr_t)this_host) {
-                    error_report("Non-same host page %p/%p",
-                                  host, this_host);
-                    ret = -EINVAL;
-                    break;
-                }
+                error_report("Non-same host page %p/%p", host_page,
+                             host_page_from_ram_block_offset(block, addr));
+                ret = -EINVAL;
+                break;
             }
 
             /*
@@ -3255,15 +3268,10 @@ static int ram_load_postcopy(QEMUFile *f)
         }
 
         if (!ret && place_needed) {
-            /* This gets called at the last target page in the host page */
-            void *place_dest = (void *)QEMU_ALIGN_DOWN((uintptr_t)host,
-                                                       block->page_size);
-
             if (all_zero) {
-                ret = postcopy_place_page_zero(mis, place_dest,
-                                               block);
+                ret = postcopy_place_page_zero(mis, host_page, block);
             } else {
-                ret = postcopy_place_page(mis, place_dest, postcopy_host_page,
+                ret = postcopy_place_page(mis, host_page, postcopy_host_page,
                                           block);
             }
         }
-- 
2.24.1

[PATCH v1 09/13] migrate/ram: Consolidate variable reset after placement in ram_load_postcopy()

[David Hildenbrand]

Let's consolidate resetting the variables.

Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
Cc: Juan Quintela <quintela@redhat.com>
Cc: Peter Xu <peterx@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 migration/ram.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 80a4e4a9ea..ab1f5534cf 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -3126,7 +3126,7 @@ static int ram_load_postcopy(QEMUFile *f)
     /* Temporary page that is later 'placed' */
     void *postcopy_host_page = mis->postcopy_tmp_page;
     void *host_page = NULL;
-    bool all_zero = false;
+    bool all_zero = true;
     int target_pages = 0;
 
     while (!ret && !(flags & RAM_SAVE_FLAG_EOS)) {
@@ -3151,7 +3151,6 @@ static int ram_load_postcopy(QEMUFile *f)
         addr &= TARGET_PAGE_MASK;
 
         trace_ram_load_postcopy_loop((uint64_t)addr, flags);
-        place_needed = false;
         if (flags & (RAM_SAVE_FLAG_ZERO | RAM_SAVE_FLAG_PAGE |
                      RAM_SAVE_FLAG_COMPRESS_PAGE)) {
             block = ram_block_from_stream(f, flags);
@@ -3179,9 +3178,7 @@ static int ram_load_postcopy(QEMUFile *f)
              */
             page_buffer = postcopy_host_page +
                           host_page_offset_from_ram_block_offset(block, addr);
-            /* If all TP are zero then we can optimise the place */
             if (target_pages == 1) {
-                all_zero = true;
                 host_page = host_page_from_ram_block_offset(block, addr);
             } else if (host_page != host_page_from_ram_block_offset(block,
                                                                     addr)) {
@@ -3198,7 +3195,6 @@ static int ram_load_postcopy(QEMUFile *f)
              */
             if (target_pages == (block->page_size / TARGET_PAGE_SIZE)) {
                 place_needed = true;
-                target_pages = 0;
             }
         }
 
@@ -3274,6 +3270,10 @@ static int ram_load_postcopy(QEMUFile *f)
                 ret = postcopy_place_page(mis, host_page, postcopy_host_page,
                                           block);
             }
+            place_needed = false;
+            target_pages = 0;
+            /* Assume we have a zero page until we detect something different */
+            all_zero = true;
         }
     }
 
-- 
2.24.1

[PATCH v1 10/13] migrate/ram: Handle RAM block resizes during postcopy

[David Hildenbrand]

Resizing while migrating is dangerous and does not work as expected.
The whole migration code works on the usable_length of ram blocks and does
not expect this to change at random points in time.

In the case of postcopy, relying on used_length is racy as soon as the
guest is running. Also, when used_length changes we might leave the
uffd handler registered for some memory regions, reject valid pages
when migrating and fail when sending the recv bitmap to the source.

Resizing can be trigger *after* (but not during) a reset in
ACPI code by the guest
- hw/arm/virt-acpi-build.c:acpi_ram_update()
- hw/i386/acpi-build.c:acpi_ram_update()

Let's remember the original used_length in a separate variable and
use it in relevant postcopy code. Make sure to update it when we resize
during precopy, when synchronizing the RAM block sizes with the source.

Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
Cc: Juan Quintela <quintela@redhat.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Richard Henderson <richard.henderson@linaro.org>
Cc: Shannon Zhao <shannon.zhao@linaro.org>
Cc: Alex Bennée <alex.bennee@linaro.org>
Cc: Peter Xu <peterx@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 include/exec/ramblock.h  |  9 +++++++++
 migration/postcopy-ram.c | 15 ++++++++++++---
 migration/ram.c          | 11 +++++++++--
 3 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/include/exec/ramblock.h b/include/exec/ramblock.h
index 07d50864d8..0e9e9b346b 100644
--- a/include/exec/ramblock.h
+++ b/include/exec/ramblock.h
@@ -59,6 +59,15 @@ struct RAMBlock {
      */
     unsigned long *clear_bmap;
     uint8_t clear_bmap_shift;
+
+    /*
+     * RAM block used_length before the guest started running while postcopy
+     * was active. Once the guest is running, used_length can change. Used to
+     * register/unregister uffd handlers and as the size of the recv bitmap.
+     * Receiving any page beyond this length will bail out, as it could not have
+     * been valid on the source.
+     */
+    ram_addr_t postcopy_length;
 };
 #endif
 #endif
diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index a36402722b..c68caf4e42 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -17,6 +17,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/rcu.h"
 #include "exec/target_page.h"
 #include "migration.h"
 #include "qemu-file.h"
@@ -31,6 +32,7 @@
 #include "qemu/error-report.h"
 #include "trace.h"
 #include "hw/boards.h"
+#include "exec/ramblock.h"
 
 /* Arbitrary limit on size of each discard command,
  * keeps them around ~200 bytes
@@ -456,6 +458,13 @@ static int init_range(RAMBlock *rb, void *opaque)
     ram_addr_t length = qemu_ram_get_used_length(rb);
     trace_postcopy_init_range(block_name, host_addr, offset, length);
 
+    /*
+     * Save the used_length before running the guest. In case we have to
+     * resize RAM blocks when syncing RAM block sizes from the source during
+     * precopy, we'll update it manually via the ram block notifier.
+     */
+    rb->postcopy_length = length;
+
     /*
      * We need the whole of RAM to be truly empty for postcopy, so things
      * like ROMs and any data tables built during init must be zero'd
@@ -478,7 +487,7 @@ static int cleanup_range(RAMBlock *rb, void *opaque)
     const char *block_name = qemu_ram_get_idstr(rb);
     void *host_addr = qemu_ram_get_host_addr(rb);
     ram_addr_t offset = qemu_ram_get_offset(rb);
-    ram_addr_t length = qemu_ram_get_used_length(rb);
+    ram_addr_t length = rb->postcopy_length;
     MigrationIncomingState *mis = opaque;
     struct uffdio_range range_struct;
     trace_postcopy_cleanup_range(block_name, host_addr, offset, length);
@@ -600,7 +609,7 @@ static int nhp_range(RAMBlock *rb, void *opaque)
     const char *block_name = qemu_ram_get_idstr(rb);
     void *host_addr = qemu_ram_get_host_addr(rb);
     ram_addr_t offset = qemu_ram_get_offset(rb);
-    ram_addr_t length = qemu_ram_get_used_length(rb);
+    ram_addr_t length = rb->postcopy_length;
     trace_postcopy_nhp_range(block_name, host_addr, offset, length);
 
     /*
@@ -644,7 +653,7 @@ static int ram_block_enable_notify(RAMBlock *rb, void *opaque)
     struct uffdio_register reg_struct;
 
     reg_struct.range.start = (uintptr_t)qemu_ram_get_host_addr(rb);
-    reg_struct.range.len = qemu_ram_get_used_length(rb);
+    reg_struct.range.len = rb->postcopy_length;
     reg_struct.mode = UFFDIO_REGISTER_MODE_MISSING;
 
     /* Now tell our userfault_fd that it's responsible for this area */
diff --git a/migration/ram.c b/migration/ram.c
index ab1f5534cf..6d1dcb362c 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -244,7 +244,7 @@ int64_t ramblock_recv_bitmap_send(QEMUFile *file,
         return -1;
     }
 
-    nbits = block->used_length >> TARGET_PAGE_BITS;
+    nbits = block->postcopy_length >> TARGET_PAGE_BITS;
 
     /*
      * Make sure the tmp bitmap buffer is big enough, e.g., on 32bit
@@ -3159,7 +3159,13 @@ static int ram_load_postcopy(QEMUFile *f)
                 break;
             }
 
-            if (!offset_in_ramblock(block, addr)) {
+            /*
+             * Relying on used_length is racy and can result in false positives.
+             * We might place pages beyond used_length in case RAM was shrunk
+             * while in postcopy, which is fine - trying to place via
+             * UFFDIO_COPY/UFFDIO_ZEROPAGE will never segfault.
+             */
+            if (!block->host || addr >= block->postcopy_length) {
                 error_report("Illegal RAM offset " RAM_ADDR_FMT, addr);
                 ret = -EINVAL;
                 break;
@@ -3744,6 +3750,7 @@ static void ram_mig_ram_block_resized(RAMBlockNotifier *n, void *host,
                              rb->idstr);
             }
         }
+        rb->postcopy_length = new_size;
         return;
     }
 
-- 
2.24.1

Re: [PATCH v1 10/13] migrate/ram: Handle RAM block resizes during postcopy

[Peter Xu]

On Wed, Feb 19, 2020 at 05:17:22PM +0100, David Hildenbrand wrote:
> Resizing while migrating is dangerous and does not work as expected.
> The whole migration code works on the usable_length of ram blocks and does
> not expect this to change at random points in time.
> 
> In the case of postcopy, relying on used_length is racy as soon as the
> guest is running. Also, when used_length changes we might leave the
> uffd handler registered for some memory regions, reject valid pages
> when migrating and fail when sending the recv bitmap to the source.
> 
> Resizing can be trigger *after* (but not during) a reset in
> ACPI code by the guest
> - hw/arm/virt-acpi-build.c:acpi_ram_update()
> - hw/i386/acpi-build.c:acpi_ram_update()
> 
> Let's remember the original used_length in a separate variable and
> use it in relevant postcopy code. Make sure to update it when we resize
> during precopy, when synchronizing the RAM block sizes with the source.
> 
> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> Cc: Juan Quintela <quintela@redhat.com>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Richard Henderson <richard.henderson@linaro.org>
> Cc: Shannon Zhao <shannon.zhao@linaro.org>
> Cc: Alex Bennée <alex.bennee@linaro.org>
> Cc: Peter Xu <peterx@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  include/exec/ramblock.h  |  9 +++++++++
>  migration/postcopy-ram.c | 15 ++++++++++++---
>  migration/ram.c          | 11 +++++++++--
>  3 files changed, 30 insertions(+), 5 deletions(-)
> 
> diff --git a/include/exec/ramblock.h b/include/exec/ramblock.h
> index 07d50864d8..0e9e9b346b 100644
> --- a/include/exec/ramblock.h
> +++ b/include/exec/ramblock.h
> @@ -59,6 +59,15 @@ struct RAMBlock {
>       */
>      unsigned long *clear_bmap;
>      uint8_t clear_bmap_shift;
> +
> +    /*
> +     * RAM block used_length before the guest started running while postcopy
> +     * was active. Once the guest is running, used_length can change. Used to
> +     * register/unregister uffd handlers and as the size of the recv bitmap.
> +     * Receiving any page beyond this length will bail out, as it could not have
> +     * been valid on the source.
> +     */
> +    ram_addr_t postcopy_length;
>  };
>  #endif
>  #endif
> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> index a36402722b..c68caf4e42 100644
> --- a/migration/postcopy-ram.c
> +++ b/migration/postcopy-ram.c
> @@ -17,6 +17,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qemu/rcu.h"
>  #include "exec/target_page.h"
>  #include "migration.h"
>  #include "qemu-file.h"
> @@ -31,6 +32,7 @@
>  #include "qemu/error-report.h"
>  #include "trace.h"
>  #include "hw/boards.h"
> +#include "exec/ramblock.h"
>  
>  /* Arbitrary limit on size of each discard command,
>   * keeps them around ~200 bytes
> @@ -456,6 +458,13 @@ static int init_range(RAMBlock *rb, void *opaque)
>      ram_addr_t length = qemu_ram_get_used_length(rb);
>      trace_postcopy_init_range(block_name, host_addr, offset, length);
>  
> +    /*
> +     * Save the used_length before running the guest. In case we have to
> +     * resize RAM blocks when syncing RAM block sizes from the source during
> +     * precopy, we'll update it manually via the ram block notifier.
> +     */
> +    rb->postcopy_length = length;
> +
>      /*
>       * We need the whole of RAM to be truly empty for postcopy, so things
>       * like ROMs and any data tables built during init must be zero'd
> @@ -478,7 +487,7 @@ static int cleanup_range(RAMBlock *rb, void *opaque)
>      const char *block_name = qemu_ram_get_idstr(rb);
>      void *host_addr = qemu_ram_get_host_addr(rb);
>      ram_addr_t offset = qemu_ram_get_offset(rb);
> -    ram_addr_t length = qemu_ram_get_used_length(rb);
> +    ram_addr_t length = rb->postcopy_length;
>      MigrationIncomingState *mis = opaque;
>      struct uffdio_range range_struct;
>      trace_postcopy_cleanup_range(block_name, host_addr, offset, length);
> @@ -600,7 +609,7 @@ static int nhp_range(RAMBlock *rb, void *opaque)
>      const char *block_name = qemu_ram_get_idstr(rb);
>      void *host_addr = qemu_ram_get_host_addr(rb);
>      ram_addr_t offset = qemu_ram_get_offset(rb);
> -    ram_addr_t length = qemu_ram_get_used_length(rb);
> +    ram_addr_t length = rb->postcopy_length;
>      trace_postcopy_nhp_range(block_name, host_addr, offset, length);
>  
>      /*
> @@ -644,7 +653,7 @@ static int ram_block_enable_notify(RAMBlock *rb, void *opaque)
>      struct uffdio_register reg_struct;
>  
>      reg_struct.range.start = (uintptr_t)qemu_ram_get_host_addr(rb);
> -    reg_struct.range.len = qemu_ram_get_used_length(rb);
> +    reg_struct.range.len = rb->postcopy_length;
>      reg_struct.mode = UFFDIO_REGISTER_MODE_MISSING;
>  
>      /* Now tell our userfault_fd that it's responsible for this area */
> diff --git a/migration/ram.c b/migration/ram.c
> index ab1f5534cf..6d1dcb362c 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -244,7 +244,7 @@ int64_t ramblock_recv_bitmap_send(QEMUFile *file,
>          return -1;
>      }
>  
> -    nbits = block->used_length >> TARGET_PAGE_BITS;
> +    nbits = block->postcopy_length >> TARGET_PAGE_BITS;
>  
>      /*
>       * Make sure the tmp bitmap buffer is big enough, e.g., on 32bit
> @@ -3159,7 +3159,13 @@ static int ram_load_postcopy(QEMUFile *f)
>                  break;
>              }
>  
> -            if (!offset_in_ramblock(block, addr)) {
> +            /*
> +             * Relying on used_length is racy and can result in false positives.
> +             * We might place pages beyond used_length in case RAM was shrunk
> +             * while in postcopy, which is fine - trying to place via
> +             * UFFDIO_COPY/UFFDIO_ZEROPAGE will never segfault.
> +             */
> +            if (!block->host || addr >= block->postcopy_length) {
>                  error_report("Illegal RAM offset " RAM_ADDR_FMT, addr);
>                  ret = -EINVAL;
>                  break;
> @@ -3744,6 +3750,7 @@ static void ram_mig_ram_block_resized(RAMBlockNotifier *n, void *host,
>                               rb->idstr);
>              }
>          }
> +        rb->postcopy_length = new_size;

With this change, postcopy_length will be the same as used_length?

I thought you wanted to cache that value when starting the postcopy
phase so postcopy_length should be constant after set once.  Did I
misunderstood?

Thanks,

>          return;
>      }
>  
> -- 
> 2.24.1
> 

-- 
Peter Xu

Re: [PATCH v1 10/13] migrate/ram: Handle RAM block resizes during postcopy

[David Hildenbrand]

On 20.02.20 21:54, Peter Xu wrote:
> On Wed, Feb 19, 2020 at 05:17:22PM +0100, David Hildenbrand wrote:
>> Resizing while migrating is dangerous and does not work as expected.
>> The whole migration code works on the usable_length of ram blocks and does
>> not expect this to change at random points in time.
>>
>> In the case of postcopy, relying on used_length is racy as soon as the
>> guest is running. Also, when used_length changes we might leave the
>> uffd handler registered for some memory regions, reject valid pages
>> when migrating and fail when sending the recv bitmap to the source.
>>
>> Resizing can be trigger *after* (but not during) a reset in
>> ACPI code by the guest
>> - hw/arm/virt-acpi-build.c:acpi_ram_update()
>> - hw/i386/acpi-build.c:acpi_ram_update()
>>
>> Let's remember the original used_length in a separate variable and
>> use it in relevant postcopy code. Make sure to update it when we resize
>> during precopy, when synchronizing the RAM block sizes with the source.
>>
>> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>> Cc: Juan Quintela <quintela@redhat.com>
>> Cc: Eduardo Habkost <ehabkost@redhat.com>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Igor Mammedov <imammedo@redhat.com>
>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>> Cc: Richard Henderson <richard.henderson@linaro.org>
>> Cc: Shannon Zhao <shannon.zhao@linaro.org>
>> Cc: Alex Bennée <alex.bennee@linaro.org>
>> Cc: Peter Xu <peterx@redhat.com>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  include/exec/ramblock.h  |  9 +++++++++
>>  migration/postcopy-ram.c | 15 ++++++++++++---
>>  migration/ram.c          | 11 +++++++++--
>>  3 files changed, 30 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/exec/ramblock.h b/include/exec/ramblock.h
>> index 07d50864d8..0e9e9b346b 100644
>> --- a/include/exec/ramblock.h
>> +++ b/include/exec/ramblock.h
>> @@ -59,6 +59,15 @@ struct RAMBlock {
>>       */
>>      unsigned long *clear_bmap;
>>      uint8_t clear_bmap_shift;
>> +
>> +    /*
>> +     * RAM block used_length before the guest started running while postcopy
>> +     * was active. Once the guest is running, used_length can change. Used to
>> +     * register/unregister uffd handlers and as the size of the recv bitmap.
>> +     * Receiving any page beyond this length will bail out, as it could not have
>> +     * been valid on the source.
>> +     */
>> +    ram_addr_t postcopy_length;
>>  };
>>  #endif
>>  #endif
>> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
>> index a36402722b..c68caf4e42 100644
>> --- a/migration/postcopy-ram.c
>> +++ b/migration/postcopy-ram.c
>> @@ -17,6 +17,7 @@
>>   */
>>  
>>  #include "qemu/osdep.h"
>> +#include "qemu/rcu.h"
>>  #include "exec/target_page.h"
>>  #include "migration.h"
>>  #include "qemu-file.h"
>> @@ -31,6 +32,7 @@
>>  #include "qemu/error-report.h"
>>  #include "trace.h"
>>  #include "hw/boards.h"
>> +#include "exec/ramblock.h"
>>  
>>  /* Arbitrary limit on size of each discard command,
>>   * keeps them around ~200 bytes
>> @@ -456,6 +458,13 @@ static int init_range(RAMBlock *rb, void *opaque)
>>      ram_addr_t length = qemu_ram_get_used_length(rb);
>>      trace_postcopy_init_range(block_name, host_addr, offset, length);
>>  
>> +    /*
>> +     * Save the used_length before running the guest. In case we have to
>> +     * resize RAM blocks when syncing RAM block sizes from the source during
>> +     * precopy, we'll update it manually via the ram block notifier.
>> +     */
>> +    rb->postcopy_length = length;
>> +
>>      /*
>>       * We need the whole of RAM to be truly empty for postcopy, so things
>>       * like ROMs and any data tables built during init must be zero'd
>> @@ -478,7 +487,7 @@ static int cleanup_range(RAMBlock *rb, void *opaque)
>>      const char *block_name = qemu_ram_get_idstr(rb);
>>      void *host_addr = qemu_ram_get_host_addr(rb);
>>      ram_addr_t offset = qemu_ram_get_offset(rb);
>> -    ram_addr_t length = qemu_ram_get_used_length(rb);
>> +    ram_addr_t length = rb->postcopy_length;
>>      MigrationIncomingState *mis = opaque;
>>      struct uffdio_range range_struct;
>>      trace_postcopy_cleanup_range(block_name, host_addr, offset, length);
>> @@ -600,7 +609,7 @@ static int nhp_range(RAMBlock *rb, void *opaque)
>>      const char *block_name = qemu_ram_get_idstr(rb);
>>      void *host_addr = qemu_ram_get_host_addr(rb);
>>      ram_addr_t offset = qemu_ram_get_offset(rb);
>> -    ram_addr_t length = qemu_ram_get_used_length(rb);
>> +    ram_addr_t length = rb->postcopy_length;
>>      trace_postcopy_nhp_range(block_name, host_addr, offset, length);
>>  
>>      /*
>> @@ -644,7 +653,7 @@ static int ram_block_enable_notify(RAMBlock *rb, void *opaque)
>>      struct uffdio_register reg_struct;
>>  
>>      reg_struct.range.start = (uintptr_t)qemu_ram_get_host_addr(rb);
>> -    reg_struct.range.len = qemu_ram_get_used_length(rb);
>> +    reg_struct.range.len = rb->postcopy_length;
>>      reg_struct.mode = UFFDIO_REGISTER_MODE_MISSING;
>>  
>>      /* Now tell our userfault_fd that it's responsible for this area */
>> diff --git a/migration/ram.c b/migration/ram.c
>> index ab1f5534cf..6d1dcb362c 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -244,7 +244,7 @@ int64_t ramblock_recv_bitmap_send(QEMUFile *file,
>>          return -1;
>>      }
>>  
>> -    nbits = block->used_length >> TARGET_PAGE_BITS;
>> +    nbits = block->postcopy_length >> TARGET_PAGE_BITS;
>>  
>>      /*
>>       * Make sure the tmp bitmap buffer is big enough, e.g., on 32bit
>> @@ -3159,7 +3159,13 @@ static int ram_load_postcopy(QEMUFile *f)
>>                  break;
>>              }
>>  
>> -            if (!offset_in_ramblock(block, addr)) {
>> +            /*
>> +             * Relying on used_length is racy and can result in false positives.
>> +             * We might place pages beyond used_length in case RAM was shrunk
>> +             * while in postcopy, which is fine - trying to place via
>> +             * UFFDIO_COPY/UFFDIO_ZEROPAGE will never segfault.
>> +             */
>> +            if (!block->host || addr >= block->postcopy_length) {
>>                  error_report("Illegal RAM offset " RAM_ADDR_FMT, addr);
>>                  ret = -EINVAL;
>>                  break;
>> @@ -3744,6 +3750,7 @@ static void ram_mig_ram_block_resized(RAMBlockNotifier *n, void *host,
>>                               rb->idstr);
>>              }
>>          }
>> +        rb->postcopy_length = new_size;
> 
> With this change, postcopy_length will be the same as used_length?
> 
> I thought you wanted to cache that value when starting the postcopy
> phase so postcopy_length should be constant after set once.  Did I
> misunderstood?

So, my understanding on the migration target:

1. Source VM started and initialized.
- Theoretically we could have resizes here already in the future (e.g.,
  virtio-mem). Right now, not.
2. Precopy data is loaded
- RAM block sizes will be synchronized to the source.
3. Guest starts running
- Any RAM block resize will differ to the source.


For postcopy, we have to "freeze" the RAM block size after 2, but before
3. So rb->postcopy_length will always correspond to used_length on the
migration source.

Interestingly, userfaultfd handlers etc. are registered just before 3,
before any "resizes of interest" were handled.

So, postcopy_length will match used_length until we start our guest and
do resizes (shrink/grow) that are out of sync with our source.

Makes sense?

-- 
Thanks,

David / dhildenb

Re: [PATCH v1 10/13] migrate/ram: Handle RAM block resizes during postcopy

[David Hildenbrand]

On 21.02.20 09:35, David Hildenbrand wrote:
> On 20.02.20 21:54, Peter Xu wrote:
>> On Wed, Feb 19, 2020 at 05:17:22PM +0100, David Hildenbrand wrote:
>>> Resizing while migrating is dangerous and does not work as expected.
>>> The whole migration code works on the usable_length of ram blocks and does
>>> not expect this to change at random points in time.
>>>
>>> In the case of postcopy, relying on used_length is racy as soon as the
>>> guest is running. Also, when used_length changes we might leave the
>>> uffd handler registered for some memory regions, reject valid pages
>>> when migrating and fail when sending the recv bitmap to the source.
>>>
>>> Resizing can be trigger *after* (but not during) a reset in
>>> ACPI code by the guest
>>> - hw/arm/virt-acpi-build.c:acpi_ram_update()
>>> - hw/i386/acpi-build.c:acpi_ram_update()
>>>
>>> Let's remember the original used_length in a separate variable and
>>> use it in relevant postcopy code. Make sure to update it when we resize
>>> during precopy, when synchronizing the RAM block sizes with the source.
>>>
>>> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>>> Cc: Juan Quintela <quintela@redhat.com>
>>> Cc: Eduardo Habkost <ehabkost@redhat.com>
>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>> Cc: Igor Mammedov <imammedo@redhat.com>
>>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>>> Cc: Richard Henderson <richard.henderson@linaro.org>
>>> Cc: Shannon Zhao <shannon.zhao@linaro.org>
>>> Cc: Alex Bennée <alex.bennee@linaro.org>
>>> Cc: Peter Xu <peterx@redhat.com>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>>  include/exec/ramblock.h  |  9 +++++++++
>>>  migration/postcopy-ram.c | 15 ++++++++++++---
>>>  migration/ram.c          | 11 +++++++++--
>>>  3 files changed, 30 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/include/exec/ramblock.h b/include/exec/ramblock.h
>>> index 07d50864d8..0e9e9b346b 100644
>>> --- a/include/exec/ramblock.h
>>> +++ b/include/exec/ramblock.h
>>> @@ -59,6 +59,15 @@ struct RAMBlock {
>>>       */
>>>      unsigned long *clear_bmap;
>>>      uint8_t clear_bmap_shift;
>>> +
>>> +    /*
>>> +     * RAM block used_length before the guest started running while postcopy
>>> +     * was active. Once the guest is running, used_length can change. Used to
>>> +     * register/unregister uffd handlers and as the size of the recv bitmap.
>>> +     * Receiving any page beyond this length will bail out, as it could not have
>>> +     * been valid on the source.
>>> +     */
>>> +    ram_addr_t postcopy_length;
>>>  };
>>>  #endif
>>>  #endif
>>> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
>>> index a36402722b..c68caf4e42 100644
>>> --- a/migration/postcopy-ram.c
>>> +++ b/migration/postcopy-ram.c
>>> @@ -17,6 +17,7 @@
>>>   */
>>>  
>>>  #include "qemu/osdep.h"
>>> +#include "qemu/rcu.h"
>>>  #include "exec/target_page.h"
>>>  #include "migration.h"
>>>  #include "qemu-file.h"
>>> @@ -31,6 +32,7 @@
>>>  #include "qemu/error-report.h"
>>>  #include "trace.h"
>>>  #include "hw/boards.h"
>>> +#include "exec/ramblock.h"
>>>  
>>>  /* Arbitrary limit on size of each discard command,
>>>   * keeps them around ~200 bytes
>>> @@ -456,6 +458,13 @@ static int init_range(RAMBlock *rb, void *opaque)
>>>      ram_addr_t length = qemu_ram_get_used_length(rb);
>>>      trace_postcopy_init_range(block_name, host_addr, offset, length);
>>>  
>>> +    /*
>>> +     * Save the used_length before running the guest. In case we have to
>>> +     * resize RAM blocks when syncing RAM block sizes from the source during
>>> +     * precopy, we'll update it manually via the ram block notifier.
>>> +     */
>>> +    rb->postcopy_length = length;
>>> +
>>>      /*
>>>       * We need the whole of RAM to be truly empty for postcopy, so things
>>>       * like ROMs and any data tables built during init must be zero'd
>>> @@ -478,7 +487,7 @@ static int cleanup_range(RAMBlock *rb, void *opaque)
>>>      const char *block_name = qemu_ram_get_idstr(rb);
>>>      void *host_addr = qemu_ram_get_host_addr(rb);
>>>      ram_addr_t offset = qemu_ram_get_offset(rb);
>>> -    ram_addr_t length = qemu_ram_get_used_length(rb);
>>> +    ram_addr_t length = rb->postcopy_length;
>>>      MigrationIncomingState *mis = opaque;
>>>      struct uffdio_range range_struct;
>>>      trace_postcopy_cleanup_range(block_name, host_addr, offset, length);
>>> @@ -600,7 +609,7 @@ static int nhp_range(RAMBlock *rb, void *opaque)
>>>      const char *block_name = qemu_ram_get_idstr(rb);
>>>      void *host_addr = qemu_ram_get_host_addr(rb);
>>>      ram_addr_t offset = qemu_ram_get_offset(rb);
>>> -    ram_addr_t length = qemu_ram_get_used_length(rb);
>>> +    ram_addr_t length = rb->postcopy_length;
>>>      trace_postcopy_nhp_range(block_name, host_addr, offset, length);
>>>  
>>>      /*
>>> @@ -644,7 +653,7 @@ static int ram_block_enable_notify(RAMBlock *rb, void *opaque)
>>>      struct uffdio_register reg_struct;
>>>  
>>>      reg_struct.range.start = (uintptr_t)qemu_ram_get_host_addr(rb);
>>> -    reg_struct.range.len = qemu_ram_get_used_length(rb);
>>> +    reg_struct.range.len = rb->postcopy_length;
>>>      reg_struct.mode = UFFDIO_REGISTER_MODE_MISSING;
>>>  
>>>      /* Now tell our userfault_fd that it's responsible for this area */
>>> diff --git a/migration/ram.c b/migration/ram.c
>>> index ab1f5534cf..6d1dcb362c 100644
>>> --- a/migration/ram.c
>>> +++ b/migration/ram.c
>>> @@ -244,7 +244,7 @@ int64_t ramblock_recv_bitmap_send(QEMUFile *file,
>>>          return -1;
>>>      }
>>>  
>>> -    nbits = block->used_length >> TARGET_PAGE_BITS;
>>> +    nbits = block->postcopy_length >> TARGET_PAGE_BITS;
>>>  
>>>      /*
>>>       * Make sure the tmp bitmap buffer is big enough, e.g., on 32bit
>>> @@ -3159,7 +3159,13 @@ static int ram_load_postcopy(QEMUFile *f)
>>>                  break;
>>>              }
>>>  
>>> -            if (!offset_in_ramblock(block, addr)) {
>>> +            /*
>>> +             * Relying on used_length is racy and can result in false positives.
>>> +             * We might place pages beyond used_length in case RAM was shrunk
>>> +             * while in postcopy, which is fine - trying to place via
>>> +             * UFFDIO_COPY/UFFDIO_ZEROPAGE will never segfault.
>>> +             */
>>> +            if (!block->host || addr >= block->postcopy_length) {
>>>                  error_report("Illegal RAM offset " RAM_ADDR_FMT, addr);
>>>                  ret = -EINVAL;
>>>                  break;
>>> @@ -3744,6 +3750,7 @@ static void ram_mig_ram_block_resized(RAMBlockNotifier *n, void *host,
>>>                               rb->idstr);
>>>              }
>>>          }
>>> +        rb->postcopy_length = new_size;
>>
>> With this change, postcopy_length will be the same as used_length?
>>
>> I thought you wanted to cache that value when starting the postcopy
>> phase so postcopy_length should be constant after set once.  Did I
>> misunderstood?
> 
> So, my understanding on the migration target:
> 
> 1. Source VM started and initialized.

"Destination VM", sorry.


-- 
Thanks,

David / dhildenb

[PATCH v1 11/13] migrate/multifd: Print used_length of memory block

[David Hildenbrand]

We actually want to print the used_length, against which we check.

Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
Cc: Juan Quintela <quintela@redhat.com>
Cc: Peter Xu <peterx@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 migration/multifd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/migration/multifd.c b/migration/multifd.c
index b3e8ae9bcc..dd9e88c5f1 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -222,7 +222,7 @@ static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
         if (offset > (block->used_length - qemu_target_page_size())) {
             error_setg(errp, "multifd: offset too long %" PRIu64
                        " (max " RAM_ADDR_FMT ")",
-                       offset, block->max_length);
+                       offset, block->used_length);
             return -1;
         }
         p->pages->iov[i].iov_base = block->host + offset;
-- 
2.24.1

[PATCH v1 12/13] migrate/ram: Use offset_in_ramblock() in range checks

[David Hildenbrand]

We never read or write beyond the used_length of memory blocks when
migrating. Make this clearer by using offset_in_ramblock() consistently.

Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
Cc: Juan Quintela <quintela@redhat.com>
Cc: Peter Xu <peterx@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 migration/ram.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 6d1dcb362c..891dcd687a 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1309,8 +1309,8 @@ static bool find_dirty_block(RAMState *rs, PageSearchStatus *pss, bool *again)
         *again = false;
         return false;
     }
-    if ((((ram_addr_t)pss->page) << TARGET_PAGE_BITS)
-        >= pss->block->used_length) {
+    if (!offset_in_ramblock(pss->block,
+                            ((ram_addr_t)pss->page) << TARGET_PAGE_BITS)) {
         /* Didn't find anything in this RAM Block */
         pss->page = 0;
         pss->block = QLIST_NEXT_RCU(pss->block, next);
@@ -1514,7 +1514,7 @@ int ram_save_queue_pages(const char *rbname, ram_addr_t start, ram_addr_t len)
         rs->last_req_rb = ramblock;
     }
     trace_ram_save_queue_pages(ramblock->idstr, start, len);
-    if (start+len > ramblock->used_length) {
+    if (!offset_in_ramblock(ramblock, start + len - 1)) {
         error_report("%s request overrun start=" RAM_ADDR_FMT " len="
                      RAM_ADDR_FMT " blocklen=" RAM_ADDR_FMT,
                      __func__, start, len, ramblock->used_length);
@@ -3323,8 +3323,8 @@ static void colo_flush_ram_cache(void)
         while (block) {
             offset = migration_bitmap_find_dirty(ram_state, block, offset);
 
-            if (((ram_addr_t)offset) << TARGET_PAGE_BITS
-                >= block->used_length) {
+            if (!offset_in_ramblock(block,
+                                    ((ram_addr_t)offset) << TARGET_PAGE_BITS)) {
                 offset = 0;
                 block = QLIST_NEXT_RCU(block, next);
             } else {
-- 
2.24.1

[PATCH v1 13/13] migrate/ram: Tolerate partially changed mappings in postcopy code

[David Hildenbrand]

When we partially change mappings (e.g., mmap over parts of an existing
mmap) where we have a userfaultfd handler registered, the handler will
implicitly be unregistered from the parts that changed. This is e.g., the
case when doing a qemu_ram_remap(), but is also a preparation for RAM
blocks with resizable allocations and we're shrinking RAM blocks.

When the mapping is changed and the handler is removed, any waiters are
woken up. Trying to place pages will fail. We can simply ignore erors
due to that when placing pages - as the mapping changed on the migration
destination, also the content is stale. E.g., after shrinking a RAM
block, nobody should be using that memory. After doing a
qemu_ram_remap(), the old memory is expected to have vanished.

Let's tolerate such errors (but still warn for now) when placing pages.
Also, add a comment why unregistering will continue to work even though
the mapping might have changed.

Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
Cc: Juan Quintela <quintela@redhat.com>
Cc: Peter Xu <peterx@redhat.com>
Cc: Andrea Arcangeli <aarcange@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 migration/postcopy-ram.c | 43 ++++++++++++++++++++++++++++++++++------
 1 file changed, 37 insertions(+), 6 deletions(-)

diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index c68caf4e42..df9d27c004 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -506,6 +506,13 @@ static int cleanup_range(RAMBlock *rb, void *opaque)
     range_struct.start = (uintptr_t)host_addr;
     range_struct.len = length;
 
+    /*
+     * In case the mapping was partially changed since we enabled userfault
+     * (esp. when whrinking RAM blocks and we have resizable allocations, or
+     * via qemu_ram_remap()), the userfaultfd handler was already removed for
+     * the mappings that changed. Unregistering will, however, still work and
+     * ignore mappings without a registered handler.
+     */
     if (ioctl(mis->userfault_fd, UFFDIO_UNREGISTER, &range_struct)) {
         error_report("%s: userfault unregister %s", __func__, strerror(errno));
 
@@ -1239,10 +1246,28 @@ int postcopy_place_page(MigrationIncomingState *mis, void *host, void *from,
      */
     if (qemu_ufd_copy_ioctl(mis->userfault_fd, host, from, pagesize, rb)) {
         int e = errno;
-        error_report("%s: %s copy host: %p from: %p (size: %zd)",
-                     __func__, strerror(e), host, from, pagesize);
 
-        return -e;
+        /*
+         * When the mapping gets partially changed before we try to place a page
+         * (esp. when whrinking RAM blocks and we have resizable allocations, or
+         * via qemu_ram_remap()), the userfaultfd handler will be removed and
+         * placing pages will fail. In that case, any waiter was already woken
+         * up when the mapping was changed. We can safely ignore this, as
+         * mappings that change once we're running on the destination imply
+         * that memory of these mappings vanishes. Let's still print a warning
+         * for now.
+         *
+         * Old kernels report EINVAL, new kernels report ENOENT.
+         */
+        if (e == ENOENT || e == EINVAL) {
+            warn_report("%s: %s copy host: %p from: %p (size: %zd)",
+                        __func__, strerror(e), host, from, pagesize);
+        } else {
+            error_report("%s: %s copy host: %p from: %p (size: %zd)",
+                         __func__, strerror(e), host, from, pagesize);
+
+            return -e;
+        }
     }
 
     trace_postcopy_place_page(host);
@@ -1266,10 +1291,16 @@ int postcopy_place_page_zero(MigrationIncomingState *mis, void *host,
     if (qemu_ram_is_uf_zeroable(rb)) {
         if (qemu_ufd_copy_ioctl(mis->userfault_fd, host, NULL, pagesize, rb)) {
             int e = errno;
-            error_report("%s: %s zero host: %p",
-                         __func__, strerror(e), host);
 
-            return -e;
+            /* See the comment in postcopy_place_page() */
+            if (e == ENOENT || e == EINVAL) {
+                warn_report("%s: %s zero host: %p", __func__, strerror(e),
+                            host);
+            } else {
+                error_report("%s: %s zero host: %p", __func__, strerror(e),
+                             host);
+                return -e;
+            }
         }
         return postcopy_notify_shared_wake(rb,
                                            qemu_ram_block_host_offset(rb,
-- 
2.24.1

Re: [PATCH v1 13/13] migrate/ram: Tolerate partially changed mappings in postcopy code

[David Hildenbrand]

On 19.02.20 17:17, David Hildenbrand wrote:
> When we partially change mappings (e.g., mmap over parts of an existing
> mmap) where we have a userfaultfd handler registered, the handler will
> implicitly be unregistered from the parts that changed. This is e.g., the
> case when doing a qemu_ram_remap(), but is also a preparation for RAM
> blocks with resizable allocations and we're shrinking RAM blocks.
> 
> When the mapping is changed and the handler is removed, any waiters are
> woken up. Trying to place pages will fail. We can simply ignore erors
> due to that when placing pages - as the mapping changed on the migration
> destination, also the content is stale. E.g., after shrinking a RAM
> block, nobody should be using that memory. After doing a
> qemu_ram_remap(), the old memory is expected to have vanished.
> 
> Let's tolerate such errors (but still warn for now) when placing pages.
> Also, add a comment why unregistering will continue to work even though
> the mapping might have changed.
> 
> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> Cc: Juan Quintela <quintela@redhat.com>
> Cc: Peter Xu <peterx@redhat.com>
> Cc: Andrea Arcangeli <aarcange@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  migration/postcopy-ram.c | 43 ++++++++++++++++++++++++++++++++++------
>  1 file changed, 37 insertions(+), 6 deletions(-)
> 
> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> index c68caf4e42..df9d27c004 100644
> --- a/migration/postcopy-ram.c
> +++ b/migration/postcopy-ram.c
> @@ -506,6 +506,13 @@ static int cleanup_range(RAMBlock *rb, void *opaque)
>      range_struct.start = (uintptr_t)host_addr;
>      range_struct.len = length;
>  
> +    /*
> +     * In case the mapping was partially changed since we enabled userfault
> +     * (esp. when whrinking RAM blocks and we have resizable allocations, or
> +     * via qemu_ram_remap()), the userfaultfd handler was already removed for
> +     * the mappings that changed. Unregistering will, however, still work and
> +     * ignore mappings without a registered handler.
> +     */
>      if (ioctl(mis->userfault_fd, UFFDIO_UNREGISTER, &range_struct)) {
>          error_report("%s: userfault unregister %s", __func__, strerror(errno));
>  
> @@ -1239,10 +1246,28 @@ int postcopy_place_page(MigrationIncomingState *mis, void *host, void *from,
>       */
>      if (qemu_ufd_copy_ioctl(mis->userfault_fd, host, from, pagesize, rb)) {
>          int e = errno;
> -        error_report("%s: %s copy host: %p from: %p (size: %zd)",
> -                     __func__, strerror(e), host, from, pagesize);
>  
> -        return -e;
> +        /*
> +         * When the mapping gets partially changed before we try to place a page
> +         * (esp. when whrinking RAM blocks and we have resizable allocations, or
> +         * via qemu_ram_remap()), the userfaultfd handler will be removed and
> +         * placing pages will fail. In that case, any waiter was already woken
> +         * up when the mapping was changed. We can safely ignore this, as
> +         * mappings that change once we're running on the destination imply
> +         * that memory of these mappings vanishes. Let's still print a warning
> +         * for now.
> +         *

After talking to Andrea, on mapping changes, no waiter will be woken up
automatically. We have to do an UFFDIO_WAKE, which even works when there
is no longer a handler registered for that reason. Interesting stuff :)

-- 
Thanks,

David / dhildenb

Re: [PATCH v1 13/13] migrate/ram: Tolerate partially changed mappings in postcopy code

[Peter Xu]

On Thu, Feb 20, 2020 at 12:24:42PM +0100, David Hildenbrand wrote:
> On 19.02.20 17:17, David Hildenbrand wrote:
> > When we partially change mappings (e.g., mmap over parts of an existing
> > mmap) where we have a userfaultfd handler registered, the handler will
> > implicitly be unregistered from the parts that changed. This is e.g., the
> > case when doing a qemu_ram_remap(), but is also a preparation for RAM
> > blocks with resizable allocations and we're shrinking RAM blocks.
> > 
> > When the mapping is changed and the handler is removed, any waiters are
> > woken up. Trying to place pages will fail. We can simply ignore erors
> > due to that when placing pages - as the mapping changed on the migration
> > destination, also the content is stale. E.g., after shrinking a RAM
> > block, nobody should be using that memory. After doing a
> > qemu_ram_remap(), the old memory is expected to have vanished.
> > 
> > Let's tolerate such errors (but still warn for now) when placing pages.
> > Also, add a comment why unregistering will continue to work even though
> > the mapping might have changed.
> > 
> > Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > Cc: Juan Quintela <quintela@redhat.com>
> > Cc: Peter Xu <peterx@redhat.com>
> > Cc: Andrea Arcangeli <aarcange@redhat.com>
> > Signed-off-by: David Hildenbrand <david@redhat.com>
> > ---
> >  migration/postcopy-ram.c | 43 ++++++++++++++++++++++++++++++++++------
> >  1 file changed, 37 insertions(+), 6 deletions(-)
> > 
> > diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> > index c68caf4e42..df9d27c004 100644
> > --- a/migration/postcopy-ram.c
> > +++ b/migration/postcopy-ram.c
> > @@ -506,6 +506,13 @@ static int cleanup_range(RAMBlock *rb, void *opaque)
> >      range_struct.start = (uintptr_t)host_addr;
> >      range_struct.len = length;
> >  
> > +    /*
> > +     * In case the mapping was partially changed since we enabled userfault
> > +     * (esp. when whrinking RAM blocks and we have resizable allocations, or
> > +     * via qemu_ram_remap()), the userfaultfd handler was already removed for
> > +     * the mappings that changed. Unregistering will, however, still work and
> > +     * ignore mappings without a registered handler.
> > +     */
> >      if (ioctl(mis->userfault_fd, UFFDIO_UNREGISTER, &range_struct)) {
> >          error_report("%s: userfault unregister %s", __func__, strerror(errno));
> >  
> > @@ -1239,10 +1246,28 @@ int postcopy_place_page(MigrationIncomingState *mis, void *host, void *from,
> >       */
> >      if (qemu_ufd_copy_ioctl(mis->userfault_fd, host, from, pagesize, rb)) {
> >          int e = errno;
> > -        error_report("%s: %s copy host: %p from: %p (size: %zd)",
> > -                     __func__, strerror(e), host, from, pagesize);
> >  
> > -        return -e;
> > +        /*
> > +         * When the mapping gets partially changed before we try to place a page
> > +         * (esp. when whrinking RAM blocks and we have resizable allocations, or
> > +         * via qemu_ram_remap()), the userfaultfd handler will be removed and
> > +         * placing pages will fail. In that case, any waiter was already woken
> > +         * up when the mapping was changed. We can safely ignore this, as
> > +         * mappings that change once we're running on the destination imply
> > +         * that memory of these mappings vanishes. Let's still print a warning
> > +         * for now.
> > +         *
> 
> After talking to Andrea, on mapping changes, no waiter will be woken up
> automatically. We have to do an UFFDIO_WAKE, which even works when there
> is no longer a handler registered for that reason. Interesting stuff :)

Yes actually it makes sense. :)

Though I do think it should hardly happen, otherwise even if it's
waked up it'll still try to access that GPA and KVM will be confused
on that and exit because no memslot was setup for that.  Then I think
it's a fatal VM error.  In other words, I feel like the resizing
should be blocked somehow by that stall vcpu too (e.g., even if we
want to reboot a Linux guest, it'll sync between vcpus, and same to
bootstraping).

Btw, I feel like we cannot always depend on the fact that userfaultfd
will dissapear itself if the VMA is unmapped, because even it's true
it'll only be true for shrinking of memories.  How about extending
memory in the future?  So IIUC if we want to really fix this, we
probably need to take care of uffd register and unregister of changed
memory regions, which AFAIUI can be done inside your newly introduced
resize hook...

We probably need to take care of other things that might be related to
ramblock resizing too in the same notifier.  One I can think of is to
realloc the ramblock.receivedmap otherwise we could have some bit
cleared forever for shrinking memories (which logically when migration
finishes that bitmap should be all set).

Thanks,

-- 
Peter Xu

Re: [PATCH v1 13/13] migrate/ram: Tolerate partially changed mappings in postcopy code

[David Hildenbrand]

On 20.02.20 22:07, Peter Xu wrote:
> On Thu, Feb 20, 2020 at 12:24:42PM +0100, David Hildenbrand wrote:
>> On 19.02.20 17:17, David Hildenbrand wrote:
>>> When we partially change mappings (e.g., mmap over parts of an existing
>>> mmap) where we have a userfaultfd handler registered, the handler will
>>> implicitly be unregistered from the parts that changed. This is e.g., the
>>> case when doing a qemu_ram_remap(), but is also a preparation for RAM
>>> blocks with resizable allocations and we're shrinking RAM blocks.
>>>
>>> When the mapping is changed and the handler is removed, any waiters are
>>> woken up. Trying to place pages will fail. We can simply ignore erors
>>> due to that when placing pages - as the mapping changed on the migration
>>> destination, also the content is stale. E.g., after shrinking a RAM
>>> block, nobody should be using that memory. After doing a
>>> qemu_ram_remap(), the old memory is expected to have vanished.
>>>
>>> Let's tolerate such errors (but still warn for now) when placing pages.
>>> Also, add a comment why unregistering will continue to work even though
>>> the mapping might have changed.
>>>
>>> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>>> Cc: Juan Quintela <quintela@redhat.com>
>>> Cc: Peter Xu <peterx@redhat.com>
>>> Cc: Andrea Arcangeli <aarcange@redhat.com>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>>  migration/postcopy-ram.c | 43 ++++++++++++++++++++++++++++++++++------
>>>  1 file changed, 37 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
>>> index c68caf4e42..df9d27c004 100644
>>> --- a/migration/postcopy-ram.c
>>> +++ b/migration/postcopy-ram.c
>>> @@ -506,6 +506,13 @@ static int cleanup_range(RAMBlock *rb, void *opaque)
>>>      range_struct.start = (uintptr_t)host_addr;
>>>      range_struct.len = length;
>>>  
>>> +    /*
>>> +     * In case the mapping was partially changed since we enabled userfault
>>> +     * (esp. when whrinking RAM blocks and we have resizable allocations, or
>>> +     * via qemu_ram_remap()), the userfaultfd handler was already removed for
>>> +     * the mappings that changed. Unregistering will, however, still work and
>>> +     * ignore mappings without a registered handler.
>>> +     */
>>>      if (ioctl(mis->userfault_fd, UFFDIO_UNREGISTER, &range_struct)) {
>>>          error_report("%s: userfault unregister %s", __func__, strerror(errno));
>>>  
>>> @@ -1239,10 +1246,28 @@ int postcopy_place_page(MigrationIncomingState *mis, void *host, void *from,
>>>       */
>>>      if (qemu_ufd_copy_ioctl(mis->userfault_fd, host, from, pagesize, rb)) {
>>>          int e = errno;
>>> -        error_report("%s: %s copy host: %p from: %p (size: %zd)",
>>> -                     __func__, strerror(e), host, from, pagesize);
>>>  
>>> -        return -e;
>>> +        /*
>>> +         * When the mapping gets partially changed before we try to place a page
>>> +         * (esp. when whrinking RAM blocks and we have resizable allocations, or
>>> +         * via qemu_ram_remap()), the userfaultfd handler will be removed and
>>> +         * placing pages will fail. In that case, any waiter was already woken
>>> +         * up when the mapping was changed. We can safely ignore this, as
>>> +         * mappings that change once we're running on the destination imply
>>> +         * that memory of these mappings vanishes. Let's still print a warning
>>> +         * for now.
>>> +         *
>>
>> After talking to Andrea, on mapping changes, no waiter will be woken up
>> automatically. We have to do an UFFDIO_WAKE, which even works when there
>> is no longer a handler registered for that reason. Interesting stuff :)
> 
> Yes actually it makes sense. :)
> 
> Though I do think it should hardly happen, otherwise even if it's
> waked up it'll still try to access that GPA and KVM will be confused
> on that and exit because no memslot was setup for that.  Then I think
> it's a fatal VM error.  In other words, I feel like the resizing
> should be blocked somehow by that stall vcpu too (e.g., even if we
> want to reboot a Linux guest, it'll sync between vcpus, and same to
> bootstraping).

I am actually not concerned about KVM. More about some QEMU thread that
accesses bad RAM block state. With resizable allocations that thread
would get a segfault - here it could happen that it simply waits
forever. I guess a segfault would be better to debug than some thread
being stuck waiting for a uffd.

I do agree that this might be very rare (IOW, a BUG in the code :) ).
And we might skip the wakeup for now. But we should set the page
received in the bitmap, even if placement failed due to changed mappings.

Resizes are properly synchronized with KVM suing memory listeners. E.g.,
with resizable allocations, we will only shrink the RAM block *after*
the kvm slots where modified. (resizing kvm slots while VCPUs are in KVM
is a different problem to solve on my side :) )

> 
> Btw, I feel like we cannot always depend on the fact that userfaultfd
> will dissapear itself if the VMA is unmapped, because even it's true
> it'll only be true for shrinking of memories.  How about extending
> memory in the future?  So IIUC if we want to really fix this, we

As far as I understood, growing works as designed.

1. Destination VM started and initialized.
2. Precopy data is loaded
- RAM block sizes will be synchronized to the source.
3. Guest starts running
- Any RAM block resize will differ to the source.

userfaultfd handlers will be registered just before 3. Growing in 3
means that we are bigger than the RAM block on the source. There is
nothing to migrate -> no usefaultfd handler (it would even be bad to
register one).

> probably need to take care of uffd register and unregister of changed
> memory regions, which AFAIUI can be done inside your newly introduced
> resize hook...

We have to be careful, because once we resize while the guest is
running, any resizes race with precopy code. Having that said, it should
not be necessary as far as I understand.
> 
> We probably need to take care of other things that might be related to
> ramblock resizing too in the same notifier.  One I can think of is to
> realloc the ramblock.receivedmap otherwise we could have some bit
> cleared forever for shrinking memories (which logically when migration
> finishes that bitmap should be all set).

Realloc is not needed (as long as we keep allocating for max_length),
but eventually simply setting all bits of pages that are now outside of
used_length as received (which can be done atomically AFAIK, which is nice).

Having that said, I am currently trying to test the postcopy stuff with
virtio-mem and resizable allocations. So stay tuned :)

Thanks a lot for your valuable feedback!

-- 
Thanks,

David / dhildenb

Re: [PATCH v1 00/13] migrate/ram: Fix resizing RAM blocks while migrating

[David Hildenbrand]

On 19.02.20 17:17, David Hildenbrand wrote:
> This is the follow up of
>     "[PATCH RFC] memory: Don't allow to resize RAM while migrating" [1]
> 
> This series contains some (slightly modified) patches also contained in:
>     "[PATCH v2 fixed 00/16] Ram blocks with resizable anonymous allocations
>      under POSIX" [2]
> That series will be based on this series. The last patch (#13) in this
> series could be moved to the other series, but I decided to include it in
> here for now (similar context).
> 
> I realized that resizing RAM blocks while the guest is being migrated
> (precopy: resize while still running on the source, postcopy: resize
>  while already running on the target) is buggy. In case of precopy, we
> can simply cancel migration. Postcopy handling is more involved. Resizing
> can currently happen during a guest reboot, triggered by ACPI rebuilds.
> 
> Along with the fixes, some cleanups.
> 
> [1] https://lkml.kernel.org/r/20200213172016.196609-1-david@redhat.com
> [2] https://lkml.kernel.org/r/20200212134254.11073-1-david@redhat.com
> 

I'm working on some reworks/cleanups and some testing (with virtio-mem
because there it's easy to trigger resizes). So whoever wants to have a
look, better to wait for the updated series :)

-- 
Thanks,

David / dhildenb