[PATCH 0/4] migration: savevm testing

[PATCH 0/4] migration: savevm testing

[Fabiano Rosas]

Hi, we had a bug report that enabling multifd and attempting
savevm/loadvm crashes QEMU. This seems to have been around for many
years.

I'm adding a fix for this in the form of a capabilities check for
snapshots.

I'm also adding a couple of tests that validate migration capabilities
are properly rejected by savevm.

There is a larger discussion to be had which is whether we want to
attempt to implement every migration capability for snapshots or
should we try to convert snapshots into a regular migration or some
third option. For now I'm trying to avoid this by not touching
capabilities that don't cause a crash, but let me know your thoughts.

Thanks

CI run: https://gitlab.com/farosas/qemu/-/pipelines/1738368896

Fabiano Rosas (4):
  migration/savevm: Add a compatibility check for capabilities
  tests/qtest/migration: Extract machine type resolution
  tests/qtest/migration: Add QMP helpers for snapshot
  tests/qtest/migration: Add savevm tests

 migration/options.c                   |  26 +++++
 migration/options.h                   |   1 +
 migration/savevm.c                    |   8 ++
 tests/qtest/meson.build               |   1 +
 tests/qtest/migration-test.c          |   1 +
 tests/qtest/migration/framework.c     |  54 ++++++----
 tests/qtest/migration/framework.h     |   5 +
 tests/qtest/migration/migration-qmp.c | 120 +++++++++++++++++++++
 tests/qtest/migration/migration-qmp.h |   4 +
 tests/qtest/migration/savevm-tests.c  | 144 ++++++++++++++++++++++++++
 10 files changed, 345 insertions(+), 19 deletions(-)
 create mode 100644 tests/qtest/migration/savevm-tests.c

-- 
2.35.3

[PATCH 1/4] migration/savevm: Add a compatibility check for capabilities

[Fabiano Rosas]

It has always been possible to enable arbitrary migration capabilities
and attempt to take a snapshot of the VM with the savevm/loadvm
commands as well as their QMP counterparts
snapshot-save/snapshot-load.

Most migration capabilities are not meant to be used with snapshots
and there's a risk of crashing QEMU or producing incorrect
behavior. Ideally, every migration capability would either be
implemented for savevm or explicitly rejected.

Add a compatibility check routine and reject the snapshot command if
an incompatible capability is enabled. For now only act on the the two
that actually cause a crash: multifd and mapped-ram.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2881
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/options.c | 26 ++++++++++++++++++++++++++
 migration/options.h |  1 +
 migration/savevm.c  |  8 ++++++++
 3 files changed, 35 insertions(+)

diff --git a/migration/options.c b/migration/options.c
index b0ac2ea408..8772b98dca 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -443,11 +443,37 @@ INITIALIZE_MIGRATE_CAPS_SET(check_caps_background_snapshot,
     MIGRATION_CAPABILITY_VALIDATE_UUID,
     MIGRATION_CAPABILITY_ZERO_COPY_SEND);
 
+/* Snapshot compatibility check list */
+static const
+INITIALIZE_MIGRATE_CAPS_SET(check_caps_savevm,
+                            MIGRATION_CAPABILITY_MULTIFD,
+                            MIGRATION_CAPABILITY_MAPPED_RAM,
+);
+
 static bool migrate_incoming_started(void)
 {
     return !!migration_incoming_get_current()->transport_data;
 }
 
+bool migrate_can_snapshot(Error **errp)
+{
+    MigrationState *s = migrate_get_current();
+    int i;
+
+    for (i = 0; i < check_caps_savevm.size; i++) {
+        int incomp_cap = check_caps_savevm.caps[i];
+
+        if (s->capabilities[incomp_cap]) {
+            error_setg(errp,
+                       "Snapshots are not compatible with %s",
+                       MigrationCapability_str(incomp_cap));
+            return false;
+        }
+    }
+
+    return true;
+}
+
 /**
  * @migration_caps_check - check capability compatibility
  *
diff --git a/migration/options.h b/migration/options.h
index 762be4e641..20b71b6f2a 100644
--- a/migration/options.h
+++ b/migration/options.h
@@ -58,6 +58,7 @@ bool migrate_tls(void);
 /* capabilities helpers */
 
 bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp);
+bool migrate_can_snapshot(Error **errp);
 
 /* parameters */
 
diff --git a/migration/savevm.c b/migration/savevm.c
index ce158c3512..3be13bcfe8 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -3229,6 +3229,10 @@ bool save_snapshot(const char *name, bool overwrite, const char *vmstate,
 
     GLOBAL_STATE_CODE();
 
+    if (!migrate_can_snapshot(errp)) {
+        return false;
+    }
+
     if (migration_is_blocked(errp)) {
         return false;
     }
@@ -3413,6 +3417,10 @@ bool load_snapshot(const char *name, const char *vmstate,
     int ret;
     MigrationIncomingState *mis = migration_incoming_get_current();
 
+    if (!migrate_can_snapshot(errp)) {
+        return false;
+    }
+
     if (!bdrv_all_can_snapshot(has_devices, devices, errp)) {
         return false;
     }
-- 
2.35.3

[PATCH 2/4] tests/qtest/migration: Extract machine type resolution

[Fabiano Rosas]

Move the machine type resolution code to a separate function, along
with the machine alias, which is only used for this purpose.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/migration/framework.c | 52 +++++++++++++++++++++----------
 1 file changed, 35 insertions(+), 17 deletions(-)

diff --git a/tests/qtest/migration/framework.c b/tests/qtest/migration/framework.c
index 10e1d04b58..2311100dd6 100644
--- a/tests/qtest/migration/framework.c
+++ b/tests/qtest/migration/framework.c
@@ -207,6 +207,38 @@ static QList *migrate_start_get_qmp_capabilities(const MigrateStart *args)
     return capabilities;
 }
 
+static char *migrate_resolve_alias(const char *arch)
+{
+    const char *machine_alias;
+
+    if (g_str_equal(arch, "i386")) {
+        machine_alias = "pc";
+
+    } else if (g_str_equal(arch, "x86_64")) {
+        machine_alias = "q35";
+
+    } else if (g_str_equal(arch, "s390x")) {
+        machine_alias = "s390-ccw-virtio";
+
+    } else if (g_str_equal(arch, "ppc64")) {
+        machine_alias = "pseries";
+
+    } else if (g_str_equal(arch, "aarch64")) {
+        machine_alias = "virt";
+
+    } else {
+        g_assert_not_reached();
+    }
+
+    if (!qtest_has_machine(machine_alias)) {
+        g_autofree char *msg = g_strdup_printf("machine %s not supported", machine_alias);
+        g_test_skip(msg);
+        return NULL;
+    }
+
+    return resolve_machine_version(machine_alias, QEMU_ENV_SRC, QEMU_ENV_DST);
+}
+
 int migrate_start(QTestState **from, QTestState **to, const char *uri,
                   MigrateStart *args)
 {
@@ -220,7 +252,7 @@ int migrate_start(QTestState **from, QTestState **to, const char *uri,
     const char *kvm_opts = NULL;
     const char *arch = qtest_get_arch();
     const char *memory_size;
-    const char *machine_alias, *machine_opts = "";
+    const char *machine_opts = "";
     g_autofree char *machine = NULL;
     const char *bootpath;
     g_autoptr(QList) capabilities = migrate_start_get_qmp_capabilities(args);
@@ -241,12 +273,6 @@ int migrate_start(QTestState **from, QTestState **to, const char *uri,
 
     if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
         memory_size = "150M";
-
-        if (g_str_equal(arch, "i386")) {
-            machine_alias = "pc";
-        } else {
-            machine_alias = "q35";
-        }
         arch_opts = g_strdup_printf(
             "-drive if=none,id=d0,file=%s,format=raw "
             "-device ide-hd,drive=d0,secs=1,cyls=1,heads=1", bootpath);
@@ -254,7 +280,6 @@ int migrate_start(QTestState **from, QTestState **to, const char *uri,
         end_address = X86_TEST_MEM_END;
     } else if (g_str_equal(arch, "s390x")) {
         memory_size = "128M";
-        machine_alias = "s390-ccw-virtio";
         arch_opts = g_strdup_printf("-bios %s", bootpath);
         start_address = S390_TEST_MEM_START;
         end_address = S390_TEST_MEM_END;
@@ -262,14 +287,12 @@ int migrate_start(QTestState **from, QTestState **to, const char *uri,
         memory_size = "256M";
         start_address = PPC_TEST_MEM_START;
         end_address = PPC_TEST_MEM_END;
-        machine_alias = "pseries";
         machine_opts = "vsmt=8";
         arch_opts = g_strdup_printf(
             "-nodefaults -machine " PSERIES_DEFAULT_CAPABILITIES " "
             "-bios %s", bootpath);
     } else if (strcmp(arch, "aarch64") == 0) {
         memory_size = "150M";
-        machine_alias = "virt";
         machine_opts = "gic-version=3";
         arch_opts = g_strdup_printf("-cpu max -kernel %s", bootpath);
         start_address = ARM_TEST_MEM_START;
@@ -311,15 +334,10 @@ int migrate_start(QTestState **from, QTestState **to, const char *uri,
         kvm_opts = ",dirty-ring-size=4096";
     }
 
-    if (!qtest_has_machine(machine_alias)) {
-        g_autofree char *msg = g_strdup_printf("machine %s not supported", machine_alias);
-        g_test_skip(msg);
+    machine = migrate_resolve_alias(arch);
+    if (!machine) {
         return -1;
     }
-
-    machine = resolve_machine_version(machine_alias, QEMU_ENV_SRC,
-                                      QEMU_ENV_DST);
-
     g_test_message("Using machine type: %s", machine);
 
     cmd_source = g_strdup_printf("-accel kvm%s -accel tcg "
-- 
2.35.3

[PATCH 3/4] tests/qtest/migration: Add QMP helpers for snapshot

[Fabiano Rosas]

Add helpers to call QMP snapshot commands and monitor the snapshot
job.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/migration/migration-qmp.c | 120 ++++++++++++++++++++++++++
 tests/qtest/migration/migration-qmp.h |   4 +
 2 files changed, 124 insertions(+)

diff --git a/tests/qtest/migration/migration-qmp.c b/tests/qtest/migration/migration-qmp.c
index fb59741b2c..f5941e9473 100644
--- a/tests/qtest/migration/migration-qmp.c
+++ b/tests/qtest/migration/migration-qmp.c
@@ -518,3 +518,123 @@ void migrate_postcopy_start(QTestState *from, QTestState *to,
     wait_for_stop(from, src_state);
     qtest_qmp_eventwait(to, "RESUME");
 }
+
+static void job_status_wait(QTestState *s, const char *id, const char *target)
+{
+    QDict *response, *data;
+    const char *status, *name;
+    bool found;
+
+    do {
+        response = qtest_qmp_eventwait_ref(s, "JOB_STATUS_CHANGE");
+        data = qdict_get_qdict(response, "data");
+        g_assert(data);
+
+        name = qdict_get_str(data, "id");
+        if (g_str_equal(name, id)) {
+            status = qdict_get_str(data, "status");
+            found = (strcmp(status, target) == 0);
+        }
+
+        qobject_unref(response);
+    } while (!found);
+}
+
+static bool job_check_status(QTestState *qts, const char *id, char **msg)
+{
+    QDict *rsp, *info;
+    QList *list;
+    const QListEntry *p;
+    gchar *status;
+    const char *job_msg, *job_id;
+
+    rsp = qtest_qmp(qts, "{ 'execute': 'query-jobs' }");
+    g_assert(rsp);
+    g_assert(qdict_haskey(rsp, "return"));
+
+    list = qdict_get_qlist(rsp, "return");
+    g_assert(list);
+
+    for (p = qlist_first(list); p; p = qlist_next(p)) {
+        info = qobject_to(QDict, qlist_entry_obj(p));
+
+        g_assert(qdict_haskey(info, "id"));
+        job_id = qdict_get_str(info, "id");
+
+        if (g_str_equal(job_id, id)) {
+            break;
+        }
+    }
+
+    /* otherwise job not found */
+    g_assert(p);
+
+    g_assert(qdict_haskey(info, "status"));
+    status = g_strdup(qdict_get_str(info, "status"));
+    g_assert(g_str_equal(status, "concluded"));
+
+    if (qdict_haskey(info, "error")) {
+        job_msg = qdict_get_str(info, "error");
+
+        g_assert(msg);
+        *msg = g_strdup(job_msg);
+
+        return false;
+    }
+
+    qobject_unref(rsp);
+
+    return true;
+}
+
+static void snapshot_cmd_qmp(QTestState *qts, const char *cmd, const char *id)
+{
+    if (g_str_equal(cmd, "snapshot-delete")) {
+        qtest_qmp_assert_success(qts, "{ 'execute': %s,"
+                                 "'arguments': { "
+                                 "'job-id': %s,"
+                                 "'tag': 'my-snap',"
+                                 "'devices': [ 'disk0' ] } }",
+                                 cmd, id);
+    } else {
+        qtest_qmp_assert_success(qts, "{ 'execute': %s,"
+                                 "'arguments': { "
+                                 "'job-id': %s,"
+                                 "'tag': 'my-snap',"
+                                 "'devices': [ 'disk0' ],"
+                                 "'vmstate': 'disk0' } }",
+                                 cmd, id);
+    }
+}
+
+static bool snapshot_cmd_qmp_sync(QTestState *qts, const char *cmd, const char* id,
+                                  char **error_str)
+{
+    bool ret;
+
+    snapshot_cmd_qmp(qts, cmd, id);
+    job_status_wait(qts, id, "concluded");
+    ret = job_check_status(qts, id, error_str);
+
+    qtest_qmp_assert_success(qts,
+                             "{ 'execute': 'job-dismiss',"
+                             "'arguments': { "
+                             "'id': %s } }", id);
+    return ret;
+}
+
+bool snapshot_delete_qmp_sync(QTestState *qts, char **error_str)
+{
+    return snapshot_cmd_qmp_sync(qts, "snapshot-delete", "snapdelete0",
+                                 error_str);
+}
+
+bool snapshot_load_qmp_sync(QTestState *qts, char **error_str)
+{
+    return snapshot_cmd_qmp_sync(qts, "snapshot-load", "snapload0", error_str);
+}
+
+bool snapshot_save_qmp_sync(QTestState *qts, char **error_str)
+{
+    return snapshot_cmd_qmp_sync(qts, "snapshot-save", "snapsave0", error_str);
+}
diff --git a/tests/qtest/migration/migration-qmp.h b/tests/qtest/migration/migration-qmp.h
index faa8181d91..9033828f7d 100644
--- a/tests/qtest/migration/migration-qmp.h
+++ b/tests/qtest/migration/migration-qmp.h
@@ -44,5 +44,9 @@ void migrate_recover(QTestState *who, const char *uri);
 void migrate_cancel(QTestState *who);
 void migrate_postcopy_start(QTestState *from, QTestState *to,
                             QTestMigrationState *src_state);
+bool snapshot_delete_qmp_sync(QTestState *qts, char **error_str);
+bool snapshot_load_qmp_sync(QTestState *qts, char **error_str);
+bool snapshot_save_qmp_sync(QTestState *qts, char **error_str);
+
 
 #endif /* MIGRATION_QMP_H */
-- 
2.35.3

[PATCH 4/4] tests/qtest/migration: Add savevm tests

[Fabiano Rosas]

Add a test file for savevm tests so the snapshot functionality can be
better tested in the context of migration. There's currently issues
with migration capabilities causing crashes in QEMU when running
savevm.

Start with a couple of tests, one that simply saves and loads a
snapshot and another to check that migration capabilities don't cause
disruption of savevm.

The simple savevm/loadvm test will be redundant with some block layer
tests that are already present. The objective here is more to have an
infrastructure that can save and load snapshots from different QEMU
versions, which is convenient for tracking compatibility bugs.

I chose to not add a guest workload for this because we could in the
future run the test for all architectures without having to write
guest code (but also because the QEMU cmdline construction is way more
complex).

Both tests only run during the full set of tests.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/meson.build              |   1 +
 tests/qtest/migration-test.c         |   1 +
 tests/qtest/migration/framework.c    |   4 +-
 tests/qtest/migration/framework.h    |   5 +
 tests/qtest/migration/savevm-tests.c | 144 +++++++++++++++++++++++++++
 5 files changed, 152 insertions(+), 3 deletions(-)
 create mode 100644 tests/qtest/migration/savevm-tests.c

diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index 3136d15e0f..305b662620 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -347,6 +347,7 @@ migration_files = [files(
   'migration/misc-tests.c',
   'migration/precopy-tests.c',
   'migration/postcopy-tests.c',
+  'migration/savevm-tests.c',
 )]
 
 migration_tls_files = []
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 0893687174..b15f6d091b 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -55,6 +55,7 @@ int main(int argc, char **argv)
     migration_test_add_precopy(env);
     migration_test_add_cpr(env);
     migration_test_add_misc(env);
+    migration_test_add_savevm(env);
 
     ret = g_test_run();
 
diff --git a/tests/qtest/migration/framework.c b/tests/qtest/migration/framework.c
index 2311100dd6..42bda03693 100644
--- a/tests/qtest/migration/framework.c
+++ b/tests/qtest/migration/framework.c
@@ -28,8 +28,6 @@
 
 
 #define QEMU_VM_FILE_MAGIC 0x5145564d
-#define QEMU_ENV_SRC "QTEST_QEMU_BINARY_SRC"
-#define QEMU_ENV_DST "QTEST_QEMU_BINARY_DST"
 
 unsigned start_address;
 unsigned end_address;
@@ -207,7 +205,7 @@ static QList *migrate_start_get_qmp_capabilities(const MigrateStart *args)
     return capabilities;
 }
 
-static char *migrate_resolve_alias(const char *arch)
+char *migrate_resolve_alias(const char *arch)
 {
     const char *machine_alias;
 
diff --git a/tests/qtest/migration/framework.h b/tests/qtest/migration/framework.h
index e4a11870f6..66823267af 100644
--- a/tests/qtest/migration/framework.h
+++ b/tests/qtest/migration/framework.h
@@ -17,6 +17,9 @@
 #define FILE_TEST_OFFSET 0x1000
 #define FILE_TEST_MARKER 'X'
 
+#define QEMU_ENV_SRC "QTEST_QEMU_BINARY_SRC"
+#define QEMU_ENV_DST "QTEST_QEMU_BINARY_DST"
+
 typedef struct MigrationTestEnv {
     bool has_kvm;
     bool has_tcg;
@@ -225,6 +228,7 @@ void test_file_common(MigrateCommon *args, bool stop_src);
 void *migrate_hook_start_precopy_tcp_multifd_common(QTestState *from,
                                                     QTestState *to,
                                                     const char *method);
+char *migrate_resolve_alias(const char *arch);
 
 typedef struct QTestMigrationState QTestMigrationState;
 QTestMigrationState *get_src(void);
@@ -240,5 +244,6 @@ void migration_test_add_file(MigrationTestEnv *env);
 void migration_test_add_precopy(MigrationTestEnv *env);
 void migration_test_add_cpr(MigrationTestEnv *env);
 void migration_test_add_misc(MigrationTestEnv *env);
+void migration_test_add_savevm(MigrationTestEnv *env);
 
 #endif /* TEST_FRAMEWORK_H */
diff --git a/tests/qtest/migration/savevm-tests.c b/tests/qtest/migration/savevm-tests.c
new file mode 100644
index 0000000000..5904c4b07e
--- /dev/null
+++ b/tests/qtest/migration/savevm-tests.c
@@ -0,0 +1,144 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#include "qemu/osdep.h"
+#include "libqtest.h"
+#include "migration/framework.h"
+#include "migration/migration-qmp.h"
+#include "migration/migration-util.h"
+#include "qapi/qapi-types-migration.h"
+
+char *disk_path;
+
+static char *savevm_make_cmdline(void)
+{
+    MigrationTestEnv *env = migration_get_env();
+    g_autofree char *drive_opts = NULL;
+    g_autofree char *arch_opts = NULL;
+    g_autofree char *machine_opts = NULL;
+    g_autofree char *machine = NULL;
+
+    disk_path = g_strdup_printf("%s/qtest-savevm-%d.qcow2", g_get_tmp_dir(),
+                                getpid());
+    drive_opts = g_strdup_printf("-drive if=none,file=%s,format=qcow2,node-name=disk0 ",
+                                disk_path);
+
+    g_assert(mkimg(disk_path, "qcow2", 100));
+
+    machine = migrate_resolve_alias(env->arch);
+
+    if (machine) {
+        machine_opts = g_strdup_printf("-machine %s", machine);
+    }
+
+    return g_strdup_printf("%s %s %s",
+                           drive_opts,
+                           arch_opts ?: "",
+                           machine_opts ?: "");
+}
+
+static void teardown_savevm_test(void)
+{
+    unlink(disk_path);
+    g_free(disk_path);
+}
+
+/*
+ * Enabling capabilities before savevm/loadvm should either apply the
+ * appropriate feature or reject the command. Crashing or ignoring the
+ * capability is not acceptable. Most (all?) migration capabilities
+ * are incompatible with snapshots, but they've historically not been
+ * rejected. Since there are compatibility concerns with simply
+ * rejecting all caps, for now this test only validates that nothing
+ * crashes.
+ */
+static void test_savevm_caps(void)
+{
+    MigrationTestEnv *env = migration_get_env();
+    g_autofree char *cmdline = savevm_make_cmdline();
+    QTestState *vm;
+
+    /*
+     * Only one VM to avoid having to shutdown the machine several
+     * times to release the disks lock.
+     */
+    if (env->qemu_src || env->qemu_dst) {
+        g_test_skip("Only one QEMU binary is supported");
+        return;
+    }
+
+    vm = qtest_init(cmdline);
+
+    for (int i = 0; i < MIGRATION_CAPABILITY__MAX; i++) {
+        const char *cap = MigrationCapability_str(i);
+        g_autofree char *error_str = NULL;
+        bool ret;
+
+        switch (i) {
+        case MIGRATION_CAPABILITY_ZERO_BLOCKS:          /* deprecated */
+        case MIGRATION_CAPABILITY_ZERO_COPY_SEND:       /* requires multifd */
+        case MIGRATION_CAPABILITY_POSTCOPY_PREEMPT:     /* requires postcopy-ram */
+        case MIGRATION_CAPABILITY_SWITCHOVER_ACK:       /* requires return-path */
+        case MIGRATION_CAPABILITY_DIRTY_LIMIT:          /* requires dirty ring setup */
+        case MIGRATION_CAPABILITY_BACKGROUND_SNAPSHOT:  /* requires uffd setup */
+            continue;
+        default:
+            break;
+        }
+
+        if (getenv("QTEST_LOG")) {
+            g_test_message("%s", MigrationCapability_str(i));
+        }
+        migrate_set_capability(vm, MigrationCapability_str(i), true);
+
+        ret = snapshot_save_qmp_sync(vm, &error_str);
+
+        if (ret) {
+            g_assert(snapshot_load_qmp_sync(vm, NULL));
+            g_assert(snapshot_delete_qmp_sync(vm, NULL));
+        } else {
+            g_autofree char *msg = g_strdup_printf(
+                "Snapshots are not compatible with %s", cap);
+
+            g_assert(error_str);
+            g_assert(g_str_equal(msg, error_str));
+        }
+
+        migrate_set_capability(vm, MigrationCapability_str(i), false);
+    }
+
+    qtest_quit(vm);
+    teardown_savevm_test();
+}
+
+static void test_savevm_loadvm(void)
+{
+    g_autofree char *cmdline = savevm_make_cmdline();
+    QTestState *src, *dst;
+    bool ret;
+
+    src = qtest_init_with_env(QEMU_ENV_SRC, cmdline, true);
+
+    ret = snapshot_save_qmp_sync(src, NULL);
+    qtest_quit(src);
+
+    if (ret) {
+        dst = qtest_init_with_env(QEMU_ENV_DST, cmdline, true);
+
+        g_assert(snapshot_load_qmp_sync(dst, NULL));
+        g_assert(snapshot_delete_qmp_sync(dst, NULL));
+        qtest_quit(dst);
+    }
+
+    teardown_savevm_test();
+}
+
+void migration_test_add_savevm(MigrationTestEnv *env)
+{
+    if (!getenv("QTEST_QEMU_IMG")) {
+        g_test_message("savevm tests require QTEST_QEMU_IMG");
+        return;
+    }
+
+    migration_test_add("/migration/savevm/save-load", test_savevm_loadvm);
+    migration_test_add("/migration/savevm/capabilities", test_savevm_caps);
+}
-- 
2.35.3

Re: [PATCH 0/4] migration: savevm testing

[Fabiano Rosas]

Fabiano Rosas <farosas@suse.de> writes:

> Hi, we had a bug report that enabling multifd and attempting
> savevm/loadvm crashes QEMU. This seems to have been around for many
> years.
>
> I'm adding a fix for this in the form of a capabilities check for
> snapshots.
>
> I'm also adding a couple of tests that validate migration capabilities
> are properly rejected by savevm.
>
> There is a larger discussion to be had which is whether we want to
> attempt to implement every migration capability for snapshots or
> should we try to convert snapshots into a regular migration or some
> third option. For now I'm trying to avoid this by not touching
> capabilities that don't cause a crash, but let me know your thoughts.

Turns out, there's a patch that just arrived on the mailing list adding
mapped-ram support for savevm/loadvm, so I guess we'll have this
discussion now =)

migration: add FEATURE_SEEKABLE to QIOChannelBlock
https://lore.kernel.org/r/20250327141451.163744-3-Marco.Cavenati@eurecom.fr

>
> Thanks
>
> CI run: https://gitlab.com/farosas/qemu/-/pipelines/1738368896
>
> Fabiano Rosas (4):
>   migration/savevm: Add a compatibility check for capabilities
>   tests/qtest/migration: Extract machine type resolution
>   tests/qtest/migration: Add QMP helpers for snapshot
>   tests/qtest/migration: Add savevm tests
>
>  migration/options.c                   |  26 +++++
>  migration/options.h                   |   1 +
>  migration/savevm.c                    |   8 ++
>  tests/qtest/meson.build               |   1 +
>  tests/qtest/migration-test.c          |   1 +
>  tests/qtest/migration/framework.c     |  54 ++++++----
>  tests/qtest/migration/framework.h     |   5 +
>  tests/qtest/migration/migration-qmp.c | 120 +++++++++++++++++++++
>  tests/qtest/migration/migration-qmp.h |   4 +
>  tests/qtest/migration/savevm-tests.c  | 144 ++++++++++++++++++++++++++
>  10 files changed, 345 insertions(+), 19 deletions(-)
>  create mode 100644 tests/qtest/migration/savevm-tests.c

Re: [PATCH 1/4] migration/savevm: Add a compatibility check for capabilities

[Daniel P. Berrangé]

On Thu, Mar 27, 2025 at 11:39:31AM -0300, Fabiano Rosas wrote:
> It has always been possible to enable arbitrary migration capabilities
> and attempt to take a snapshot of the VM with the savevm/loadvm
> commands as well as their QMP counterparts
> snapshot-save/snapshot-load.
> 
> Most migration capabilities are not meant to be used with snapshots
> and there's a risk of crashing QEMU or producing incorrect
> behavior. Ideally, every migration capability would either be
> implemented for savevm or explicitly rejected.

IMHO, this a prime example of why migration config shouldn't be held
as global state, and instead passed as parameters to the commands
that need them.  The snapshot-save/load commands would then only
be able to accept what few settings are actually relevant, instead
of inheriting any/all global migration state.

> Add a compatibility check routine and reject the snapshot command if
> an incompatible capability is enabled. For now only act on the the two
> that actually cause a crash: multifd and mapped-ram.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2818

Issue is 2881 not 2818                                   ^^^^^^^

> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  migration/options.c | 26 ++++++++++++++++++++++++++
>  migration/options.h |  1 +
>  migration/savevm.c  |  8 ++++++++
>  3 files changed, 35 insertions(+)

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 1/4] migration/savevm: Add a compatibility check for capabilities

[Fabiano Rosas]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Thu, Mar 27, 2025 at 11:39:31AM -0300, Fabiano Rosas wrote:
>> It has always been possible to enable arbitrary migration capabilities
>> and attempt to take a snapshot of the VM with the savevm/loadvm
>> commands as well as their QMP counterparts
>> snapshot-save/snapshot-load.
>> 
>> Most migration capabilities are not meant to be used with snapshots
>> and there's a risk of crashing QEMU or producing incorrect
>> behavior. Ideally, every migration capability would either be
>> implemented for savevm or explicitly rejected.
>
> IMHO, this a prime example of why migration config shouldn't be held
> as global state, and instead passed as parameters to the commands
> that need them.  The snapshot-save/load commands would then only
> be able to accept what few settings are actually relevant, instead
> of inheriting any/all global migration state.
>

Right, I remember we got caught around the fact that some migration
options are needed during runtime as well... but I don't remember the
details, let try to find that thread.

>> Add a compatibility check routine and reject the snapshot command if
>> an incompatible capability is enabled. For now only act on the the two
>> that actually cause a crash: multifd and mapped-ram.
>> 
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2818
>
> Issue is 2881 not 2818                                   ^^^^^^^
>

It seem that was your own C-t =)

>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  migration/options.c | 26 ++++++++++++++++++++++++++
>>  migration/options.h |  1 +
>>  migration/savevm.c  |  8 ++++++++
>>  3 files changed, 35 insertions(+)
>
> With regards,
> Daniel

Re: [PATCH 1/4] migration/savevm: Add a compatibility check for capabilities

[Marco Cavenati]

Hello Fabiano,

First of all thanks a lot for the quick follow up to my issue!

I just want to point out that with only mapped-ram enabled (without
multifd) savevm/loadvm do not lead to a crash but just to an error
according to my (few) experiments (on upstream).

Ciao

Marco

On Thursday, March 27, 2025 15:39 CET, Fabiano Rosas <farosas@suse.de> wrote:

> It has always been possible to enable arbitrary migration capabilities
> and attempt to take a snapshot of the VM with the savevm/loadvm
> commands as well as their QMP counterparts
> snapshot-save/snapshot-load.
> 
> Most migration capabilities are not meant to be used with snapshots
> and there's a risk of crashing QEMU or producing incorrect
> behavior. Ideally, every migration capability would either be
> implemented for savevm or explicitly rejected.
> 
> Add a compatibility check routine and reject the snapshot command if
> an incompatible capability is enabled. For now only act on the the two
> that actually cause a crash: multifd and mapped-ram.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2881
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  migration/options.c | 26 ++++++++++++++++++++++++++
>  migration/options.h |  1 +
>  migration/savevm.c  |  8 ++++++++
>  3 files changed, 35 insertions(+)
> 
> diff --git a/migration/options.c b/migration/options.c
> index b0ac2ea408..8772b98dca 100644
> --- a/migration/options.c
> +++ b/migration/options.c
> @@ -443,11 +443,37 @@ INITIALIZE_MIGRATE_CAPS_SET(check_caps_background_snapshot,
>      MIGRATION_CAPABILITY_VALIDATE_UUID,
>      MIGRATION_CAPABILITY_ZERO_COPY_SEND);
>  
> +/* Snapshot compatibility check list */
> +static const
> +INITIALIZE_MIGRATE_CAPS_SET(check_caps_savevm,
> +                            MIGRATION_CAPABILITY_MULTIFD,
> +                            MIGRATION_CAPABILITY_MAPPED_RAM,
> +);
> +
>  static bool migrate_incoming_started(void)
>  {
>      return !!migration_incoming_get_current()->transport_data;
>  }
>  
> +bool migrate_can_snapshot(Error **errp)
> +{
> +    MigrationState *s = migrate_get_current();
> +    int i;
> +
> +    for (i = 0; i < check_caps_savevm.size; i++) {
> +        int incomp_cap = check_caps_savevm.caps[i];
> +
> +        if (s->capabilities[incomp_cap]) {
> +            error_setg(errp,
> +                       "Snapshots are not compatible with %s",
> +                       MigrationCapability_str(incomp_cap));
> +            return false;
> +        }
> +    }
> +
> +    return true;
> +}
> +
>  /**
>   * @migration_caps_check - check capability compatibility
>   *
> diff --git a/migration/options.h b/migration/options.h
> index 762be4e641..20b71b6f2a 100644
> --- a/migration/options.h
> +++ b/migration/options.h
> @@ -58,6 +58,7 @@ bool migrate_tls(void);
>  /* capabilities helpers */
>  
>  bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp);
> +bool migrate_can_snapshot(Error **errp);
>  
>  /* parameters */
>  
> diff --git a/migration/savevm.c b/migration/savevm.c
> index ce158c3512..3be13bcfe8 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -3229,6 +3229,10 @@ bool save_snapshot(const char *name, bool overwrite, const char *vmstate,
>  
>      GLOBAL_STATE_CODE();
>  
> +    if (!migrate_can_snapshot(errp)) {
> +        return false;
> +    }
> +
>      if (migration_is_blocked(errp)) {
>          return false;
>      }
> @@ -3413,6 +3417,10 @@ bool load_snapshot(const char *name, const char *vmstate,
>      int ret;
>      MigrationIncomingState *mis = migration_incoming_get_current();
>  
> +    if (!migrate_can_snapshot(errp)) {
> +        return false;
> +    }
> +
>      if (!bdrv_all_can_snapshot(has_devices, devices, errp)) {
>          return false;
>      }
> -- 
> 2.35.3
>

Re: [PATCH 1/4] migration/savevm: Add a compatibility check for capabilities

[Fabiano Rosas]

"Marco Cavenati" <Marco.Cavenati@eurecom.fr> writes:

> Hello Fabiano,
>
> First of all thanks a lot for the quick follow up to my issue!
>
> I just want to point out that with only mapped-ram enabled (without
> multifd) savevm/loadvm do not lead to a crash but just to an error
> according to my (few) experiments (on upstream).
>

Yes, absolutely. I used imprecise language. Thanks for the correction,
I'll explain it better in the following versions.

Re: [PATCH 1/4] migration/savevm: Add a compatibility check for capabilities

[Fabiano Rosas]

Fabiano Rosas <farosas@suse.de> writes:

> Daniel P. Berrangé <berrange@redhat.com> writes:
>
>> On Thu, Mar 27, 2025 at 11:39:31AM -0300, Fabiano Rosas wrote:
>>> It has always been possible to enable arbitrary migration capabilities
>>> and attempt to take a snapshot of the VM with the savevm/loadvm
>>> commands as well as their QMP counterparts
>>> snapshot-save/snapshot-load.
>>> 
>>> Most migration capabilities are not meant to be used with snapshots
>>> and there's a risk of crashing QEMU or producing incorrect
>>> behavior. Ideally, every migration capability would either be
>>> implemented for savevm or explicitly rejected.
>>
>> IMHO, this a prime example of why migration config shouldn't be held
>> as global state, and instead passed as parameters to the commands
>> that need them.  The snapshot-save/load commands would then only
>> be able to accept what few settings are actually relevant, instead
>> of inheriting any/all global migration state.
>>
>
> Right, I remember we got caught around the fact that some migration
> options are needed during runtime as well... but I don't remember the
> details, let try to find that thread.
>

Found it: https://lore.kernel.org/r/ZVM5xmsaE41WJYgb@redhat.com

I don't think it's *too* hard to start passing the configuration to
qmp_migrate & friends. We just need to figure out a path for the
compatibility.

I'm thiking of:

1) Unifying capabilities and parameters in a MigrationConfig
structure. We take the opportunity to fix the tls options to 'str'
instead of StrOrNull.

2) Deprecate migrate-set-capabilities. There are no capabilities
anymore.

3) Deprecate migate-set-parameters. There are no parameters
anymore. Alternatively, reuse the existing command, but have it take the
additional capabilities as optional (everything else is already
optional).

4) Depending on what we do on (3), add a new migrate-set-config command
that sets every option. All as optional. This would be nice because we
wouldn't need to worry about breaking compat on the tls options, we just
define the new command in the correct way.

5) Add a {'*config': MigrationConfig} entry to qmp_migrate and
migrate_set_{config|parameters}. Here is where I have questions, because
ideally we'd have a way to limit the migrate_set_config command to only
the options that can be set at runtime. But I can't see a way of doing
that without the duplication of the options in the QAPI .json file. I'm
inclined to allow the whole set of options and do some tracking on the
side in options.c in the migration code.

(same issue for savevm really. To allow it to (say) work with
mapped-ram, we'd need a duplicate mapped-ram entry in migration.json)

About (2) and (3). If we use this unified MigrationConfig, I can keep
the old commands working (along with the query_* variants), by defining
a compat function that converts from those commands specific format into
the new format. But then there's the question of what do we do when a
new capability/parameter comes along? Can we declare that the old
commands will not see the new data and that's it? If there's no
distinction between caps and params anymore, there isn't even a way to
decide which command to use.

>>> Add a compatibility check routine and reject the snapshot command if
>>> an incompatible capability is enabled. For now only act on the the
>>> two that actually cause a crash: multifd and mapped-ram.
>>> 
>>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2818
>>
>> Issue is 2881 not 2818                                   ^^^^^^^
>>
>
> It seem that was your own C-t =)
>
>>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>>> ---
>>>  migration/options.c | 26 ++++++++++++++++++++++++++
>>>  migration/options.h |  1 +
>>>  migration/savevm.c  |  8 ++++++++
>>>  3 files changed, 35 insertions(+)
>>
>> With regards,
>> Daniel

Re: [PATCH 1/4] migration/savevm: Add a compatibility check for capabilities

[Fabiano Rosas]

Fabiano Rosas <farosas@suse.de> writes:

+Cc Markus

context:
This series was trying to stop savevm from crashing when arbitrary
migration capabilities are enabled. Daniel brought up the previous
discussion around unifying capabilities + parameters and passing it all
via the migrate (or snapshot in this case) command arguments. I'm
looking into that now.

> Fabiano Rosas <farosas@suse.de> writes:
>
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>>
>>> On Thu, Mar 27, 2025 at 11:39:31AM -0300, Fabiano Rosas wrote:
>>>> It has always been possible to enable arbitrary migration capabilities
>>>> and attempt to take a snapshot of the VM with the savevm/loadvm
>>>> commands as well as their QMP counterparts
>>>> snapshot-save/snapshot-load.
>>>> 
>>>> Most migration capabilities are not meant to be used with snapshots
>>>> and there's a risk of crashing QEMU or producing incorrect
>>>> behavior. Ideally, every migration capability would either be
>>>> implemented for savevm or explicitly rejected.
>>>
>>> IMHO, this a prime example of why migration config shouldn't be held
>>> as global state, and instead passed as parameters to the commands
>>> that need them.  The snapshot-save/load commands would then only
>>> be able to accept what few settings are actually relevant, instead
>>> of inheriting any/all global migration state.
>>>
>>
>> Right, I remember we got caught around the fact that some migration
>> options are needed during runtime as well... but I don't remember the
>> details, let try to find that thread.
>>
>
> Found it: https://lore.kernel.org/r/ZVM5xmsaE41WJYgb@redhat.com
>
> I don't think it's *too* hard to start passing the configuration to
> qmp_migrate & friends. We just need to figure out a path for the
> compatibility.
>
> I'm thiking of:
>
> 1) Unifying capabilities and parameters in a MigrationConfig
> structure. We take the opportunity to fix the tls options to 'str'
> instead of StrOrNull.
>
> 2) Deprecate migrate-set-capabilities. There are no capabilities
> anymore.
>
> 3) Deprecate migate-set-parameters. There are no parameters
> anymore. Alternatively, reuse the existing command, but have it take the
> additional capabilities as optional (everything else is already
> optional).
>
> 4) Depending on what we do on (3), add a new migrate-set-config command
> that sets every option. All as optional. This would be nice because we
> wouldn't need to worry about breaking compat on the tls options, we just
> define the new command in the correct way.
>
> 5) Add a {'*config': MigrationConfig} entry to qmp_migrate and
> migrate_set_{config|parameters}. Here is where I have questions, because
> ideally we'd have a way to limit the migrate_set_config command to only
> the options that can be set at runtime. But I can't see a way of doing
> that without the duplication of the options in the QAPI .json file. I'm
> inclined to allow the whole set of options and do some tracking on the
> side in options.c in the migration code.
>
> (same issue for savevm really. To allow it to (say) work with
> mapped-ram, we'd need a duplicate mapped-ram entry in migration.json)
>
> About (2) and (3). If we use this unified MigrationConfig, I can keep
> the old commands working (along with the query_* variants), by defining
> a compat function that converts from those commands specific format into
> the new format. But then there's the question of what do we do when a
> new capability/parameter comes along? Can we declare that the old
> commands will not see the new data and that's it? If there's no
> distinction between caps and params anymore, there isn't even a way to
> decide which command to use.
>

Re: [PATCH 1/4] migration/savevm: Add a compatibility check for capabilities

[Fabiano Rosas]

Fabiano Rosas <farosas@suse.de> writes:

> Fabiano Rosas <farosas@suse.de> writes:
>
> +Cc Markus
>
> context:
> This series was trying to stop savevm from crashing when arbitrary
> migration capabilities are enabled. Daniel brought up the previous
> discussion around unifying capabilities + parameters and passing it all
> via the migrate (or snapshot in this case) command arguments. I'm
> looking into that now.
>
>> Fabiano Rosas <farosas@suse.de> writes:
>>
>>> Daniel P. Berrangé <berrange@redhat.com> writes:
>>>
>>>> On Thu, Mar 27, 2025 at 11:39:31AM -0300, Fabiano Rosas wrote:
>>>>> It has always been possible to enable arbitrary migration capabilities
>>>>> and attempt to take a snapshot of the VM with the savevm/loadvm
>>>>> commands as well as their QMP counterparts
>>>>> snapshot-save/snapshot-load.
>>>>> 
>>>>> Most migration capabilities are not meant to be used with snapshots
>>>>> and there's a risk of crashing QEMU or producing incorrect
>>>>> behavior. Ideally, every migration capability would either be
>>>>> implemented for savevm or explicitly rejected.
>>>>
>>>> IMHO, this a prime example of why migration config shouldn't be held
>>>> as global state, and instead passed as parameters to the commands
>>>> that need them.  The snapshot-save/load commands would then only
>>>> be able to accept what few settings are actually relevant, instead
>>>> of inheriting any/all global migration state.
>>>>
>>>
>>> Right, I remember we got caught around the fact that some migration
>>> options are needed during runtime as well... but I don't remember the
>>> details, let try to find that thread.
>>>
>>
>> Found it: https://lore.kernel.org/r/ZVM5xmsaE41WJYgb@redhat.com
>>
>> I don't think it's *too* hard to start passing the configuration to
>> qmp_migrate & friends. We just need to figure out a path for the
>> compatibility.
>>
>> I'm thiking of:
>>
>> 1) Unifying capabilities and parameters in a MigrationConfig
>> structure. We take the opportunity to fix the tls options to 'str'
>> instead of StrOrNull.
>>
>> 2) Deprecate migrate-set-capabilities. There are no capabilities
>> anymore.
>>
>> 3) Deprecate migate-set-parameters. There are no parameters
>> anymore. Alternatively, reuse the existing command, but have it take the
>> additional capabilities as optional (everything else is already
>> optional).
>>
>> 4) Depending on what we do on (3), add a new migrate-set-config command
>> that sets every option. All as optional. This would be nice because we
>> wouldn't need to worry about breaking compat on the tls options, we just
>> define the new command in the correct way.
>>
>> 5) Add a {'*config': MigrationConfig} entry to qmp_migrate and
>> migrate_set_{config|parameters}. Here is where I have questions, because
>> ideally we'd have a way to limit the migrate_set_config command to only
>> the options that can be set at runtime. But I can't see a way of doing
>> that without the duplication of the options in the QAPI .json file. I'm
>> inclined to allow the whole set of options and do some tracking on the
>> side in options.c in the migration code.
>>
>> (same issue for savevm really. To allow it to (say) work with
>> mapped-ram, we'd need a duplicate mapped-ram entry in migration.json)
>>
>> About (2) and (3). If we use this unified MigrationConfig, I can keep
>> the old commands working (along with the query_* variants), by defining
>> a compat function that converts from those commands specific format into
>> the new format. But then there's the question of what do we do when a
>> new capability/parameter comes along? Can we declare that the old
>> commands will not see the new data and that's it? If there's no
>> distinction between caps and params anymore, there isn't even a way to
>> decide which command to use.
>>

Hi all, please disregard my messages on this thread. I've posted a
series which has a cover letter that explains the situation better:

[RFC PATCH 00/13] migration: Unify capabilities and parameters
https://lore.kernel.org/r/20250411191443.22565-1-farosas@suse.de