[PATCH v2 0/4] migration/hostmem: Allow to fail early for postcopy on specific fs type

[PATCH v2 0/4] migration/hostmem: Allow to fail early for postcopy on specific fs type

[Peter Xu]

v2:
- Replace the hostmem patch to use qemu_fd_getfs() instead [David]
- Define enum instead of using string for fs type [Dan]
- One more patch added to deliver error to QMP response (rather than
  keeping some of it in STDERR), so the response should contain richer
  information and looks prettier.

Postcopy can fail in a weird way when guest mem is put onto a random file:

https://bugzilla.redhat.com/show_bug.cgi?id=2057267

It's because we only check userfault privilege on dest QEMU but don't check
memory types.  We do so only until the UFFDIO_REGISTER right after we
switch to postcopy live migration from precopy but it could be too late.

This series tries to make it fail early by checking ramblock fs type if
backed by a memory-backend-file.

Now when it happens it'll fail the dest QEMU from the start:

./qemu-system-x86_64 \
        -global migration.x-postcopy-ram=on \
        -incoming defer \
        -object memory-backend-file,id=mem,size=128M,mem-path=$memfile \
        -machine memory-backend=mem

qemu-system-x86_64: Postcopy is not supported: Host backend files need to be TMPFS or HUGETLBFS only

It will also fail e.g. QMP migrate-set-capabilities properly:

{ "execute": "migrate-set-capabilities" , "arguments": { "capabilities": [ { "capability": "postcopy-ram", "state": true } ] } }
{"error": {"class": "GenericError", "desc": "Postcopy is not supported: Host backend files need to be TMPFS or HUGETLBFS only"}}

Please have a look, thanks.

Peter Xu (4):
  util/mmap-alloc: qemu_fd_getfs()
  vl.c: Create late backends before migration object
  migration/postcopy: Detect file system on dest host
  migration: Allow postcopy_ram_supported_by_host() to report err

 include/qemu/mmap-alloc.h |  7 +++
 migration/migration.c     |  9 ++--
 migration/postcopy-ram.c  | 89 +++++++++++++++++++++++++++------------
 migration/postcopy-ram.h  |  3 +-
 migration/savevm.c        |  3 +-
 softmmu/vl.c              |  9 +++-
 util/mmap-alloc.c         | 28 ++++++++++++
 7 files changed, 111 insertions(+), 37 deletions(-)

-- 
2.39.1

[PATCH v2 1/4] util/mmap-alloc: qemu_fd_getfs()

[Peter Xu]

This new helper fetches file system type for a fd.  Only Linux is
implemented so far.  Currently only tmpfs and hugetlbfs is defined, but it
can grow per need.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/qemu/mmap-alloc.h |  7 +++++++
 util/mmap-alloc.c         | 28 ++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/include/qemu/mmap-alloc.h b/include/qemu/mmap-alloc.h
index 2825e231a7..8344daaa03 100644
--- a/include/qemu/mmap-alloc.h
+++ b/include/qemu/mmap-alloc.h
@@ -1,8 +1,15 @@
 #ifndef QEMU_MMAP_ALLOC_H
 #define QEMU_MMAP_ALLOC_H
 
+typedef enum {
+    QEMU_FS_TYPE_UNKNOWN = 0,
+    QEMU_FS_TYPE_TMPFS,
+    QEMU_FS_TYPE_HUGETLBFS,
+    QEMU_FS_TYPE_NUM,
+} QemuFsType;
 
 size_t qemu_fd_getpagesize(int fd);
+QemuFsType qemu_fd_getfs(int fd);
 
 /**
  * qemu_ram_mmap: mmap anonymous memory, the specified file or device.
diff --git a/util/mmap-alloc.c b/util/mmap-alloc.c
index 5ed7d29183..ed14f9c64d 100644
--- a/util/mmap-alloc.c
+++ b/util/mmap-alloc.c
@@ -27,8 +27,36 @@
 
 #ifdef CONFIG_LINUX
 #include <sys/vfs.h>
+#include <linux/magic.h>
 #endif
 
+QemuFsType qemu_fd_getfs(int fd)
+{
+#ifdef CONFIG_LINUX
+    struct statfs fs;
+    int ret;
+
+    if (fd < 0) {
+        return QEMU_FS_TYPE_UNKNOWN;
+    }
+
+    do {
+        ret = fstatfs(fd, &fs);
+    } while (ret != 0 && errno == EINTR);
+
+    switch (fs.f_type) {
+    case TMPFS_MAGIC:
+        return QEMU_FS_TYPE_TMPFS;
+    case HUGETLBFS_MAGIC:
+        return QEMU_FS_TYPE_HUGETLBFS;
+    default:
+        return QEMU_FS_TYPE_UNKNOWN;
+    }
+#else
+    return QEMU_FS_TYPE_UNKNOWN;
+#endif
+}
+
 size_t qemu_fd_getpagesize(int fd)
 {
 #ifdef CONFIG_LINUX
-- 
2.39.1

[PATCH v2 2/4] vl.c: Create late backends before migration object

[Peter Xu]

The migration object may want to check against different types of memory
when initialized.  Delay the creation to be after late backends.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 softmmu/vl.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/softmmu/vl.c b/softmmu/vl.c
index ea20b23e4c..ad394b402f 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -3583,14 +3583,19 @@ void qemu_init(int argc, char **argv)
                      machine_class->name, machine_class->deprecation_reason);
     }
 
+    /*
+     * Create backends before creating migration objects, so that it can
+     * check against compatibilities on the backend memories (e.g. postcopy
+     * over memory-backend-file objects).
+     */
+    qemu_create_late_backends();
+
     /*
      * Note: creates a QOM object, must run only after global and
      * compat properties have been set up.
      */
     migration_object_init();
 
-    qemu_create_late_backends();
-
     /* parse features once if machine provides default cpu_type */
     current_machine->cpu_type = machine_class->default_cpu_type;
     if (cpu_option) {
-- 
2.39.1

[PATCH v2 3/4] migration/postcopy: Detect file system on dest host

[Peter Xu]

Postcopy requires the memory support userfaultfd to work.  Right now we
check it but it's a bit too late (when switching to postcopy migration).

Do that early right at enabling of postcopy.

Note that this is still only a best effort because ramblocks can be
dynamically created.  We can add check in hostmem creations and fail if
postcopy enabled, but maybe that's too aggressive.

Still, we have chance to fail the most obvious where we know there's an
existing unsupported ramblock.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/postcopy-ram.c | 34 ++++++++++++++++++++++++++++++----
 1 file changed, 30 insertions(+), 4 deletions(-)

diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index 93f39f8e06..bbb8af61ae 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -36,6 +36,7 @@
 #include "yank_functions.h"
 #include "tls.h"
 #include "qemu/userfaultfd.h"
+#include "qemu/mmap-alloc.h"
 
 /* Arbitrary limit on size of each discard command,
  * keeps them around ~200 bytes
@@ -336,11 +337,12 @@ static bool ufd_check_and_apply(int ufd, MigrationIncomingState *mis)
 
 /* Callback from postcopy_ram_supported_by_host block iterator.
  */
-static int test_ramblock_postcopiable(RAMBlock *rb, void *opaque)
+static int test_ramblock_postcopiable(RAMBlock *rb)
 {
     const char *block_name = qemu_ram_get_idstr(rb);
     ram_addr_t length = qemu_ram_get_used_length(rb);
     size_t pagesize = qemu_ram_pagesize(rb);
+    QemuFsType fs;
 
     if (length % pagesize) {
         error_report("Postcopy requires RAM blocks to be a page size multiple,"
@@ -348,6 +350,15 @@ static int test_ramblock_postcopiable(RAMBlock *rb, void *opaque)
                      "page size of 0x%zx", block_name, length, pagesize);
         return 1;
     }
+
+    if (rb->fd >= 0) {
+        fs = qemu_fd_getfs(rb->fd);
+        if (fs != QEMU_FS_TYPE_TMPFS && fs != QEMU_FS_TYPE_HUGETLBFS) {
+            error_report("Host backend files need to be TMPFS or HUGETLBFS only");
+            return 1;
+        }
+    }
+
     return 0;
 }
 
@@ -366,6 +377,7 @@ bool postcopy_ram_supported_by_host(MigrationIncomingState *mis)
     struct uffdio_range range_struct;
     uint64_t feature_mask;
     Error *local_err = NULL;
+    RAMBlock *block;
 
     if (qemu_target_page_size() > pagesize) {
         error_report("Target page size bigger than host page size");
@@ -390,9 +402,23 @@ bool postcopy_ram_supported_by_host(MigrationIncomingState *mis)
         goto out;
     }
 
-    /* We don't support postcopy with shared RAM yet */
-    if (foreach_not_ignored_block(test_ramblock_postcopiable, NULL)) {
-        goto out;
+    /*
+     * We don't support postcopy with some type of ramblocks.
+     *
+     * NOTE: we explicitly ignored ramblock_is_ignored() instead we checked
+     * all possible ramblocks.  This is because this function can be called
+     * when creating the migration object, during the phase RAM_MIGRATABLE
+     * is not even properly set for all the ramblocks.
+     *
+     * A side effect of this is we'll also check against RAM_SHARED
+     * ramblocks even if migrate_ignore_shared() is set (in which case
+     * we'll never migrate RAM_SHARED at all), but normally this shouldn't
+     * affect in reality, or we can revisit.
+     */
+    RAMBLOCK_FOREACH(block) {
+        if (test_ramblock_postcopiable(block)) {
+            goto out;
+        }
     }
 
     /*
-- 
2.39.1

[PATCH v2 4/4] migration: Allow postcopy_ram_supported_by_host() to report err

[Peter Xu]

Instead of print it to STDERR, bring the error upwards so that it can be
reported via QMP responses.

E.g.:

{ "execute": "migrate-set-capabilities" ,
  "arguments": { "capabilities":
  [ { "capability": "postcopy-ram", "state": true } ] } }

{ "error":
  { "class": "GenericError",
    "desc": "Postcopy is not supported: Host backend files need to be TMPFS
    or HUGETLBFS only" } }

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.c    |  9 +++---
 migration/postcopy-ram.c | 61 ++++++++++++++++++++++------------------
 migration/postcopy-ram.h |  3 +-
 migration/savevm.c       |  3 +-
 4 files changed, 42 insertions(+), 34 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index bda4789193..ac15fa6092 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1313,6 +1313,7 @@ static bool migrate_caps_check(bool *cap_list,
     MigrationCapabilityStatusList *cap;
     bool old_postcopy_cap;
     MigrationIncomingState *mis = migration_incoming_get_current();
+    Error *local_err = NULL;
 
     old_postcopy_cap = cap_list[MIGRATION_CAPABILITY_POSTCOPY_RAM];
 
@@ -1344,11 +1345,9 @@ static bool migrate_caps_check(bool *cap_list,
          * special support.
          */
         if (!old_postcopy_cap && runstate_check(RUN_STATE_INMIGRATE) &&
-            !postcopy_ram_supported_by_host(mis)) {
-            /* postcopy_ram_supported_by_host will have emitted a more
-             * detailed message
-             */
-            error_setg(errp, "Postcopy is not supported");
+            !postcopy_ram_supported_by_host(mis, &local_err)) {
+            error_propagate_prepend(errp, local_err,
+                                    "Postcopy is not supported: ");
             return false;
         }
 
diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index bbb8af61ae..0713ddeeef 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -282,11 +282,14 @@ static bool request_ufd_features(int ufd, uint64_t features)
     return true;
 }
 
-static bool ufd_check_and_apply(int ufd, MigrationIncomingState *mis)
+static bool ufd_check_and_apply(int ufd, MigrationIncomingState *mis,
+                                Error **errp)
 {
     uint64_t asked_features = 0;
     static uint64_t supported_features;
 
+    assert(errp);
+
     /*
      * it's not possible to
      * request UFFD_API twice per one fd
@@ -294,7 +297,7 @@ static bool ufd_check_and_apply(int ufd, MigrationIncomingState *mis)
      */
     if (!supported_features) {
         if (!receive_ufd_features(&supported_features)) {
-            error_report("%s failed", __func__);
+            error_setg(errp, "Userfault feature detection failed");
             return false;
         }
     }
@@ -316,8 +319,7 @@ static bool ufd_check_and_apply(int ufd, MigrationIncomingState *mis)
      * userfault file descriptor
      */
     if (!request_ufd_features(ufd, asked_features)) {
-        error_report("%s failed: features %" PRIu64, __func__,
-                     asked_features);
+        error_setg(errp, "Failed features %" PRIu64, asked_features);
         return false;
     }
 
@@ -328,7 +330,8 @@ static bool ufd_check_and_apply(int ufd, MigrationIncomingState *mis)
         have_hp = supported_features & UFFD_FEATURE_MISSING_HUGETLBFS;
 #endif
         if (!have_hp) {
-            error_report("Userfault on this host does not support huge pages");
+            error_setg(errp,
+                       "Userfault on this host does not support huge pages");
             return false;
         }
     }
@@ -337,7 +340,7 @@ static bool ufd_check_and_apply(int ufd, MigrationIncomingState *mis)
 
 /* Callback from postcopy_ram_supported_by_host block iterator.
  */
-static int test_ramblock_postcopiable(RAMBlock *rb)
+static int test_ramblock_postcopiable(RAMBlock *rb, Error **errp)
 {
     const char *block_name = qemu_ram_get_idstr(rb);
     ram_addr_t length = qemu_ram_get_used_length(rb);
@@ -345,16 +348,18 @@ static int test_ramblock_postcopiable(RAMBlock *rb)
     QemuFsType fs;
 
     if (length % pagesize) {
-        error_report("Postcopy requires RAM blocks to be a page size multiple,"
-                     " block %s is 0x" RAM_ADDR_FMT " bytes with a "
-                     "page size of 0x%zx", block_name, length, pagesize);
+        error_setg(errp,
+                   "Postcopy requires RAM blocks to be a page size multiple,"
+                   " block %s is 0x" RAM_ADDR_FMT " bytes with a "
+                   "page size of 0x%zx", block_name, length, pagesize);
         return 1;
     }
 
     if (rb->fd >= 0) {
         fs = qemu_fd_getfs(rb->fd);
         if (fs != QEMU_FS_TYPE_TMPFS && fs != QEMU_FS_TYPE_HUGETLBFS) {
-            error_report("Host backend files need to be TMPFS or HUGETLBFS only");
+            error_setg(errp,
+                       "Host backend files need to be TMPFS or HUGETLBFS only");
             return 1;
         }
     }
@@ -367,7 +372,8 @@ static int test_ramblock_postcopiable(RAMBlock *rb)
  * normally fine since if the postcopy succeeds it gets turned back on at the
  * end.
  */
-bool postcopy_ram_supported_by_host(MigrationIncomingState *mis)
+bool postcopy_ram_supported_by_host(MigrationIncomingState *mis,
+                                    Error **errp)
 {
     long pagesize = qemu_real_host_page_size();
     int ufd = -1;
@@ -376,29 +382,28 @@ bool postcopy_ram_supported_by_host(MigrationIncomingState *mis)
     struct uffdio_register reg_struct;
     struct uffdio_range range_struct;
     uint64_t feature_mask;
-    Error *local_err = NULL;
     RAMBlock *block;
 
+    assert(errp);
+
     if (qemu_target_page_size() > pagesize) {
-        error_report("Target page size bigger than host page size");
+        error_setg(errp, "Target page size bigger than host page size");
         goto out;
     }
 
     ufd = uffd_open(O_CLOEXEC);
     if (ufd == -1) {
-        error_report("%s: userfaultfd not available: %s", __func__,
-                     strerror(errno));
+        error_setg(errp, "Userfaultfd not available: %s", strerror(errno));
         goto out;
     }
 
     /* Give devices a chance to object */
-    if (postcopy_notify(POSTCOPY_NOTIFY_PROBE, &local_err)) {
-        error_report_err(local_err);
+    if (postcopy_notify(POSTCOPY_NOTIFY_PROBE, errp)) {
         goto out;
     }
 
     /* Version and features check */
-    if (!ufd_check_and_apply(ufd, mis)) {
+    if (!ufd_check_and_apply(ufd, mis, errp)) {
         goto out;
     }
 
@@ -416,7 +421,7 @@ bool postcopy_ram_supported_by_host(MigrationIncomingState *mis)
      * affect in reality, or we can revisit.
      */
     RAMBLOCK_FOREACH(block) {
-        if (test_ramblock_postcopiable(block)) {
+        if (test_ramblock_postcopiable(block, errp)) {
             goto out;
         }
     }
@@ -426,7 +431,7 @@ bool postcopy_ram_supported_by_host(MigrationIncomingState *mis)
      * it was enabled.
      */
     if (munlockall()) {
-        error_report("%s: munlockall: %s", __func__,  strerror(errno));
+        error_setg(errp, "munlockall() failed: %s", strerror(errno));
         goto out;
     }
 
@@ -438,8 +443,7 @@ bool postcopy_ram_supported_by_host(MigrationIncomingState *mis)
     testarea = mmap(NULL, pagesize, PROT_READ | PROT_WRITE, MAP_PRIVATE |
                                     MAP_ANONYMOUS, -1, 0);
     if (testarea == MAP_FAILED) {
-        error_report("%s: Failed to map test area: %s", __func__,
-                     strerror(errno));
+        error_setg(errp, "Failed to map test area: %s", strerror(errno));
         goto out;
     }
     g_assert(QEMU_PTR_IS_ALIGNED(testarea, pagesize));
@@ -449,14 +453,14 @@ bool postcopy_ram_supported_by_host(MigrationIncomingState *mis)
     reg_struct.mode = UFFDIO_REGISTER_MODE_MISSING;
 
     if (ioctl(ufd, UFFDIO_REGISTER, &reg_struct)) {
-        error_report("%s userfault register: %s", __func__, strerror(errno));
+        error_setg(errp, "UFFDIO_REGISTER failed: %s", strerror(errno));
         goto out;
     }
 
     range_struct.start = (uintptr_t)testarea;
     range_struct.len = pagesize;
     if (ioctl(ufd, UFFDIO_UNREGISTER, &range_struct)) {
-        error_report("%s userfault unregister: %s", __func__, strerror(errno));
+        error_setg(errp, "UFFDIO_UNREGISTER failed: %s", strerror(errno));
         goto out;
     }
 
@@ -464,8 +468,8 @@ bool postcopy_ram_supported_by_host(MigrationIncomingState *mis)
                    (__u64)1 << _UFFDIO_COPY |
                    (__u64)1 << _UFFDIO_ZEROPAGE;
     if ((reg_struct.ioctls & feature_mask) != feature_mask) {
-        error_report("Missing userfault map features: %" PRIx64,
-                     (uint64_t)(~reg_struct.ioctls & feature_mask));
+        error_setg(errp, "Missing userfault map features: %" PRIx64,
+                   (uint64_t)(~reg_struct.ioctls & feature_mask));
         goto out;
     }
 
@@ -1187,6 +1191,8 @@ static int postcopy_temp_pages_setup(MigrationIncomingState *mis)
 
 int postcopy_ram_incoming_setup(MigrationIncomingState *mis)
 {
+    Error *local_err = NULL;
+
     /* Open the fd for the kernel to give us userfaults */
     mis->userfault_fd = uffd_open(O_CLOEXEC | O_NONBLOCK);
     if (mis->userfault_fd == -1) {
@@ -1199,7 +1205,8 @@ int postcopy_ram_incoming_setup(MigrationIncomingState *mis)
      * Although the host check already tested the API, we need to
      * do the check again as an ABI handshake on the new fd.
      */
-    if (!ufd_check_and_apply(mis->userfault_fd, mis)) {
+    if (!ufd_check_and_apply(mis->userfault_fd, mis, &local_err)) {
+        error_report_err(local_err);
         return -1;
     }
 
diff --git a/migration/postcopy-ram.h b/migration/postcopy-ram.h
index b4867a32d5..442ab89752 100644
--- a/migration/postcopy-ram.h
+++ b/migration/postcopy-ram.h
@@ -14,7 +14,8 @@
 #define QEMU_POSTCOPY_RAM_H
 
 /* Return true if the host supports everything we need to do postcopy-ram */
-bool postcopy_ram_supported_by_host(MigrationIncomingState *mis);
+bool postcopy_ram_supported_by_host(MigrationIncomingState *mis,
+                                    Error **errp);
 
 /*
  * Make all of RAM sensitive to accesses to areas that haven't yet been written
diff --git a/migration/savevm.c b/migration/savevm.c
index aa54a67fda..0d61ab6c19 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1752,7 +1752,8 @@ static int loadvm_postcopy_handle_advise(MigrationIncomingState *mis,
         return -EINVAL;
     }
 
-    if (!postcopy_ram_supported_by_host(mis)) {
+    if (!postcopy_ram_supported_by_host(mis, &local_err)) {
+        error_report_err(local_err);
         postcopy_state_set(POSTCOPY_INCOMING_NONE);
         return -1;
     }
-- 
2.39.1

Re: [PATCH v2 1/4] util/mmap-alloc: qemu_fd_getfs()

[David Hildenbrand]

On 19.04.23 18:17, Peter Xu wrote:
> This new helper fetches file system type for a fd.  Only Linux is
> implemented so far.  Currently only tmpfs and hugetlbfs is defined, but it
> can grow per need.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   include/qemu/mmap-alloc.h |  7 +++++++
>   util/mmap-alloc.c         | 28 ++++++++++++++++++++++++++++
>   2 files changed, 35 insertions(+)
> 
> diff --git a/include/qemu/mmap-alloc.h b/include/qemu/mmap-alloc.h
> index 2825e231a7..8344daaa03 100644
> --- a/include/qemu/mmap-alloc.h
> +++ b/include/qemu/mmap-alloc.h
> @@ -1,8 +1,15 @@
>   #ifndef QEMU_MMAP_ALLOC_H
>   #define QEMU_MMAP_ALLOC_H
>   
> +typedef enum {
> +    QEMU_FS_TYPE_UNKNOWN = 0,
> +    QEMU_FS_TYPE_TMPFS,
> +    QEMU_FS_TYPE_HUGETLBFS,
> +    QEMU_FS_TYPE_NUM,
> +} QemuFsType;
>   
>   size_t qemu_fd_getpagesize(int fd);
> +QemuFsType qemu_fd_getfs(int fd);
>   
>   /**
>    * qemu_ram_mmap: mmap anonymous memory, the specified file or device.
> diff --git a/util/mmap-alloc.c b/util/mmap-alloc.c
> index 5ed7d29183..ed14f9c64d 100644
> --- a/util/mmap-alloc.c
> +++ b/util/mmap-alloc.c
> @@ -27,8 +27,36 @@
>   
>   #ifdef CONFIG_LINUX
>   #include <sys/vfs.h>
> +#include <linux/magic.h>
>   #endif
>   
> +QemuFsType qemu_fd_getfs(int fd)
> +{
> +#ifdef CONFIG_LINUX
> +    struct statfs fs;
> +    int ret;
> +
> +    if (fd < 0) {
> +        return QEMU_FS_TYPE_UNKNOWN;
> +    }
> +
> +    do {
> +        ret = fstatfs(fd, &fs);
> +    } while (ret != 0 && errno == EINTR);
> +
> +    switch (fs.f_type) {
> +    case TMPFS_MAGIC:
> +        return QEMU_FS_TYPE_TMPFS;
> +    case HUGETLBFS_MAGIC:
> +        return QEMU_FS_TYPE_HUGETLBFS;
> +    default:
> +        return QEMU_FS_TYPE_UNKNOWN;
> +    }
> +#else
> +    return QEMU_FS_TYPE_UNKNOWN;
> +#endif
> +}

I guess you could do

#ifdef CONFIG_LINUX
...
     default:
         break
     }
#endif
     return QEMU_FS_TYPE_UNKNOWN;

To avoid duplicating the default handling. Whatever you prefer:


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

-- 
Thanks,

David / dhildenb

Re: [PATCH v2 2/4] vl.c: Create late backends before migration object

[David Hildenbrand]

On 19.04.23 18:17, Peter Xu wrote:
> The migration object may want to check against different types of memory
> when initialized.  Delay the creation to be after late backends.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   softmmu/vl.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index ea20b23e4c..ad394b402f 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -3583,14 +3583,19 @@ void qemu_init(int argc, char **argv)
>                        machine_class->name, machine_class->deprecation_reason);
>       }
>   
> +    /*
> +     * Create backends before creating migration objects, so that it can
> +     * check against compatibilities on the backend memories (e.g. postcopy
> +     * over memory-backend-file objects).
> +     */
> +    qemu_create_late_backends();
> +
>       /*
>        * Note: creates a QOM object, must run only after global and
>        * compat properties have been set up.
>        */
>       migration_object_init();
>   
> -    qemu_create_late_backends();
> -
>       /* parse features once if machine provides default cpu_type */
>       current_machine->cpu_type = machine_class->default_cpu_type;
>       if (cpu_option) {

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

-- 
Thanks,

David / dhildenb

Re: [PATCH v2 1/4] util/mmap-alloc: qemu_fd_getfs()

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> This new helper fetches file system type for a fd.  Only Linux is
> implemented so far.  Currently only tmpfs and hugetlbfs is defined, but it

s/is/are/

> can grow per need.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

...

> +#include <linux/magic.h>

...

> +    case TMPFS_MAGIC:

...

> +    case HUGETLBFS_MAGIC:

Everybody loves magic, right? O:-)

Re: [PATCH v2 2/4] vl.c: Create late backends before migration object

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> The migration object may want to check against different types of memory
> when initialized.  Delay the creation to be after late backends.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

We are moving late to early, not the best name O:-)

Re: [PATCH v2 3/4] migration/postcopy: Detect file system on dest host

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> Postcopy requires the memory support userfaultfd to work.  Right now we
> check it but it's a bit too late (when switching to postcopy migration).
>
> Do that early right at enabling of postcopy.
>
> Note that this is still only a best effort because ramblocks can be
> dynamically created.  We can add check in hostmem creations and fail if
> postcopy enabled, but maybe that's too aggressive.
>
> Still, we have chance to fail the most obvious where we know there's an
> existing unsupported ramblock.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

> -static int test_ramblock_postcopiable(RAMBlock *rb, void *opaque)
> +static int test_ramblock_postcopiable(RAMBlock *rb)

This should return a bool, right?
Notice that it was already there, just noticing.

>  {
>      const char *block_name = qemu_ram_get_idstr(rb);
>      ram_addr_t length = qemu_ram_get_used_length(rb);
>      size_t pagesize = qemu_ram_pagesize(rb);
> +    QemuFsType fs;

You can move the variable definition to the only block that it is used.

>      if (length % pagesize) {
>          error_report("Postcopy requires RAM blocks to be a page size multiple,"
> @@ -348,6 +350,15 @@ static int test_ramblock_postcopiable(RAMBlock *rb, void *opaque)
>                       "page size of 0x%zx", block_name, length, pagesize);
>          return 1;
>      }
> +
> +    if (rb->fd >= 0) {
> +        fs = qemu_fd_getfs(rb->fd);

Minor nit: Seeing it in use, I wonder if it is clearer to name the function:

qemu_fd_get_filesystem(fd)

> +        if (fs != QEMU_FS_TYPE_TMPFS && fs != QEMU_FS_TYPE_HUGETLBFS) {
> +            error_report("Host backend files need to be TMPFS or HUGETLBFS only");

I think that the "only" is not needed on that error message.

> +            return 1;
> +        }
> +    }
> +
>      return 0;
>  }
>  
> @@ -366,6 +377,7 @@ bool postcopy_ram_supported_by_host(MigrationIncomingState *mis)
>      struct uffdio_range range_struct;
>      uint64_t feature_mask;
>      Error *local_err = NULL;
> +    RAMBlock *block;
>  
>      if (qemu_target_page_size() > pagesize) {
>          error_report("Target page size bigger than host page size");
> @@ -390,9 +402,23 @@ bool postcopy_ram_supported_by_host(MigrationIncomingState *mis)
>          goto out;
>      }
>  
> -    /* We don't support postcopy with shared RAM yet */
> -    if (foreach_not_ignored_block(test_ramblock_postcopiable, NULL)) {
> -        goto out;
> +    /*
> +     * We don't support postcopy with some type of ramblocks.
> +     *
> +     * NOTE: we explicitly ignored ramblock_is_ignored() instead we checked
> +     * all possible ramblocks.  This is because this function can be called
> +     * when creating the migration object, during the phase RAM_MIGRATABLE
> +     * is not even properly set for all the ramblocks.
> +     *
> +     * A side effect of this is we'll also check against RAM_SHARED
> +     * ramblocks even if migrate_ignore_shared() is set (in which case
> +     * we'll never migrate RAM_SHARED at all), but normally this shouldn't
> +     * affect in reality, or we can revisit.
> +     */

I think we can tweak the English of that two paragraphs.

> +    RAMBLOCK_FOREACH(block) {
> +        if (test_ramblock_postcopiable(block)) {
> +            goto out;
> +        }
>      }
>  
>      /*

In the big scheme of things, patch is ok for me.

Later, Juan.

Re: [PATCH v2 4/4] migration: Allow postcopy_ram_supported_by_host() to report err

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> Instead of print it to STDERR, bring the error upwards so that it can be
> reported via QMP responses.
>
> E.g.:
>
> { "execute": "migrate-set-capabilities" ,
>   "arguments": { "capabilities":
>   [ { "capability": "postcopy-ram", "state": true } ] } }
>
> { "error":
>   { "class": "GenericError",
>     "desc": "Postcopy is not supported: Host backend files need to be TMPFS
>     or HUGETLBFS only" } }
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/migration.c    |  9 +++---
>  migration/postcopy-ram.c | 61 ++++++++++++++++++++++------------------
>  migration/postcopy-ram.h |  3 +-
>  migration/savevm.c       |  3 +-
>  4 files changed, 42 insertions(+), 34 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index bda4789193..ac15fa6092 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1313,6 +1313,7 @@ static bool migrate_caps_check(bool *cap_list,
>      MigrationCapabilityStatusList *cap;
>      bool old_postcopy_cap;
>      MigrationIncomingState *mis = migration_incoming_get_current();
> +    Error *local_err = NULL;

This variable can be declared in the only block that uses it.

> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> index bbb8af61ae..0713ddeeef 100644
> --- a/migration/postcopy-ram.c
> +++ b/migration/postcopy-ram.c
> @@ -282,11 +282,14 @@ static bool request_ufd_features(int ufd, uint64_t features)
>      return true;
>  }
>  
> -static bool ufd_check_and_apply(int ufd, MigrationIncomingState *mis)
> +static bool ufd_check_and_apply(int ufd, MigrationIncomingState *mis,
> +                                Error **errp)
>  {
>      uint64_t asked_features = 0;
>      static uint64_t supported_features;
>  
> +    assert(errp);
> +

Is this right?  My impression was that you have to live with errp being NULL.

error_setg() knows how to handle it being NULL:

error_setg() -> error_setg_internal() -> error_setv()

static void error_setv(Error **errp,
                       const char *src, int line, const char *func,
                       ErrorClass err_class, const char *fmt, va_list ap,
                       const char *suffix)
{
    ....
    if (errp == NULL) {
        return;
    }


> -static int test_ramblock_postcopiable(RAMBlock *rb)
> +static int test_ramblock_postcopiable(RAMBlock *rb, Error **errp)

In patch 3 you do this other change:

-static int test_ramblock_postcopiable(RAMBlock *rb, void *opaque)
+static int test_ramblock_postcopiable(RAMBlock *rb)

Can you do with a single change?

The idea of the patch is right.

Later, Juan.

Re: [PATCH v2 4/4] migration: Allow postcopy_ram_supported_by_host() to report err

[Peter Xu]

On Wed, Apr 19, 2023 at 09:51:24PM +0200, Juan Quintela wrote:
> Peter Xu <peterx@redhat.com> wrote:
> > Instead of print it to STDERR, bring the error upwards so that it can be
> > reported via QMP responses.
> >
> > E.g.:
> >
> > { "execute": "migrate-set-capabilities" ,
> >   "arguments": { "capabilities":
> >   [ { "capability": "postcopy-ram", "state": true } ] } }
> >
> > { "error":
> >   { "class": "GenericError",
> >     "desc": "Postcopy is not supported: Host backend files need to be TMPFS
> >     or HUGETLBFS only" } }
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  migration/migration.c    |  9 +++---
> >  migration/postcopy-ram.c | 61 ++++++++++++++++++++++------------------
> >  migration/postcopy-ram.h |  3 +-
> >  migration/savevm.c       |  3 +-
> >  4 files changed, 42 insertions(+), 34 deletions(-)
> >
> > diff --git a/migration/migration.c b/migration/migration.c
> > index bda4789193..ac15fa6092 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -1313,6 +1313,7 @@ static bool migrate_caps_check(bool *cap_list,
> >      MigrationCapabilityStatusList *cap;
> >      bool old_postcopy_cap;
> >      MigrationIncomingState *mis = migration_incoming_get_current();
> > +    Error *local_err = NULL;
> 
> This variable can be declared in the only block that uses it.

Sure, though I just remembered I can use ERRP_GUARD(), hence I'll go with
that.

> 
> > diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> > index bbb8af61ae..0713ddeeef 100644
> > --- a/migration/postcopy-ram.c
> > +++ b/migration/postcopy-ram.c
> > @@ -282,11 +282,14 @@ static bool request_ufd_features(int ufd, uint64_t features)
> >      return true;
> >  }
> >  
> > -static bool ufd_check_and_apply(int ufd, MigrationIncomingState *mis)
> > +static bool ufd_check_and_apply(int ufd, MigrationIncomingState *mis,
> > +                                Error **errp)
> >  {
> >      uint64_t asked_features = 0;
> >      static uint64_t supported_features;
> >  
> > +    assert(errp);
> > +
> 
> Is this right?  My impression was that you have to live with errp being NULL.

Right it knows, I just wanted to make sure error msg got captured, but we
don't really need to worrry here, all callers are in this case I believe.

Let me also switch to ERRP_GUARD() just to still avoid error_abort being
passed in, so we can still abort with a full message.

> 
> error_setg() knows how to handle it being NULL:
> 
> error_setg() -> error_setg_internal() -> error_setv()
> 
> static void error_setv(Error **errp,
>                        const char *src, int line, const char *func,
>                        ErrorClass err_class, const char *fmt, va_list ap,
>                        const char *suffix)
> {
>     ....
>     if (errp == NULL) {
>         return;
>     }
> 
> 
> > -static int test_ramblock_postcopiable(RAMBlock *rb)
> > +static int test_ramblock_postcopiable(RAMBlock *rb, Error **errp)
> 
> In patch 3 you do this other change:
> 
> -static int test_ramblock_postcopiable(RAMBlock *rb, void *opaque)
> +static int test_ramblock_postcopiable(RAMBlock *rb)
> 
> Can you do with a single change?
> 
> The idea of the patch is right.

I saw that patch 3 already merged.  I'll just spin this one, thanks!

-- 
Peter Xu