[PATCH v4 00/12] tests/migration-test: Allow testing older machine types

[PATCH v4 00/12] tests/migration-test: Allow testing older machine types

[Fabiano Rosas]

(please ignore v3 which was bogus, but don't miss the discussion in it
about the caveats of this approach:
https://lore.kernel.org/r/87jzrkdne2.fsf@suse.de)

This adds support for running migration-test with two different QEMU
versions to test migration compatibility. The tests automatically
choose the latest machine type supported by both QEMU versions.

changes:

- added a cleanup function for the allocated strings in the machine
  list. The suggestion of making a copy of the list won't work because
  keeping the structure around to avoid freeing memory, and caching
  the list are two separate things that only make sense together at
  qtest_get_machines(). Outside of it, code can just free memory
  normally and caching doesn't make sense because there's no QMP call;

- kept strdups for the reasons mentioned above;

- added a message when we're unable to find a common machine type
  (should never happen);

- changed the default x86_64 machine to q35;

- new patch to fix booting with the a-b bootsector on q35;

- added machine_opts for the options after the comma;

- used machine_alias and machine for the variable names;

- added support for specifying the machine type;

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

v3:
https://lore.kernel.org/r/20231018140736.3618-1-farosas@suse.de

v2:
https://lore.kernel.org/r/20231006123910.17759-1-farosas@suse.de

changes:

- introduce *_with_env variants of the relevant functions [Daniel, Juan]

- keep the requirement for having the QTEST_QEMU_BINARY always
  present. qtest_get_arch() is used extensively in the qtest_add*
  functions. It would be too much churn to pass a different binary
  into it.

- with this^ we also need to keep the requirement for using only one
  of SRC|DST. Otherwise it would be confusing to have three binaries
  listed.

- query the alias to find out the machine types [Daniel]

I haven't looked into the docker part for now. I think Daniel's
suggestion of QTEST_QEMU_BINARY_SRC='podman run ... qemu-system-foo'
looks interesting. Do we have the latest release already built in the
registry at any given point?

Thanks

v1:
https://lore.kernel.org/r/20231003141932.2367-1-farosas@suse.de

Hi, I had this WIP patch laying around that seems to fit Juan's vision
about testing older machine types. It is a very rough draft for now,
but it may be useful for kickstarting the discussion.

With this we can give the tests two different QEMU versions. The test
picks the older machine type between the two and runs the whole
migration-test suite.

We'd just need a way to provide the older build. Currently I'm doing
this by hand.

sample output:
 # Using two different QEMU binaries. Common machine type: pc-i440fx-8.1
 ...
 # Using ./qemu-system-x86_64 (v8.1.0-952-g8a940312a2-dirty) as migration source
 ...
 # Using ../build-8.1.0/qemu-system-x86_64 (v8.1.0-dirty) as migration destination

Let me know what you think.

Fabiano Rosas (12):
  tests/qtest: Allow qtest_qemu_binary to use a custom environment
    variable
  tests/qtest: Introduce qtest_init_with_env
  tests/qtest: Allow qtest_get_machines to use an alternate QEMU binary
  tests/qtest: Introduce qtest_has_machine_with_env
  tests/qtest: Introduce qtest_resolve_machine_alias
  tests/qtest/migration: Introduce find_common_machine_version
  tests/qtest/migration: Define a machine for all architectures
  tests/qtest/migration: Specify the geometry of the bootsector
  tests/qtest/migration: Set q35 as the default machine for x86_86
  tests/qtest/migration: Support more than one QEMU binary
  tests/qtest/migration: Allow user to specify a machine type
  tests/qtest: Don't print messages from query instances

 tests/qtest/libqtest.c          | 98 ++++++++++++++++++++++++++++-----
 tests/qtest/libqtest.h          | 32 +++++++++++
 tests/qtest/migration-helpers.c | 52 +++++++++++++++++
 tests/qtest/migration-helpers.h |  4 ++
 tests/qtest/migration-test.c    | 50 +++++++++++++++--
 5 files changed, 215 insertions(+), 21 deletions(-)

-- 
2.35.3

[PATCH v4 01/12] tests/qtest: Allow qtest_qemu_binary to use a custom environment variable

[Fabiano Rosas]

We're adding support for testing migration using two different QEMU
binaries. We'll provide the second binary in a new environment
variable.

Allow qtest_qemu_binary() to receive the name of the new variable. If
the new environment variable is not set, that's not an error, we use
QTEST_QEMU_BINARY as a fallback.

Reviewed-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/libqtest.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index dc7a55634c..03fa644663 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -336,10 +336,17 @@ void qtest_remove_abrt_handler(void *data)
     }
 }
 
-static const char *qtest_qemu_binary(void)
+static const char *qtest_qemu_binary(const char *var)
 {
     const char *qemu_bin;
 
+    if (var) {
+        qemu_bin = getenv(var);
+        if (qemu_bin) {
+            return qemu_bin;
+        }
+    }
+
     qemu_bin = getenv("QTEST_QEMU_BINARY");
     if (!qemu_bin) {
         fprintf(stderr, "Environment variable QTEST_QEMU_BINARY required\n");
@@ -392,7 +399,7 @@ static QTestState *G_GNUC_PRINTF(1, 2) qtest_spawn_qemu(const char *fmt, ...)
 
     va_start(ap, fmt);
     g_string_append_printf(command, CMD_EXEC "%s %s",
-                           qtest_qemu_binary(), tracearg);
+                           qtest_qemu_binary(NULL), tracearg);
     g_string_append_vprintf(command, fmt, ap);
     va_end(ap);
 
@@ -905,7 +912,7 @@ char *qtest_hmp(QTestState *s, const char *fmt, ...)
 
 const char *qtest_get_arch(void)
 {
-    const char *qemu = qtest_qemu_binary();
+    const char *qemu = qtest_qemu_binary(NULL);
     const char *end = strrchr(qemu, '-');
 
     if (!end) {
-- 
2.35.3

[PATCH v4 02/12] tests/qtest: Introduce qtest_init_with_env

[Fabiano Rosas]

Add a version of qtest_init() that takes an environment variable
containing the path of the QEMU binary. This allows tests to use more
than one QEMU binary.

If no variable is provided or the environment variable does not exist,
that is not an error. Fallback to using QTEST_QEMU_BINARY.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
Reviewed-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 tests/qtest/libqtest.c | 26 +++++++++++++++++++-------
 tests/qtest/libqtest.h | 13 +++++++++++++
 2 files changed, 32 insertions(+), 7 deletions(-)

diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index 03fa644663..9eebba8767 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -388,7 +388,8 @@ static pid_t qtest_create_process(char *cmd)
 }
 #endif /* _WIN32 */
 
-static QTestState *G_GNUC_PRINTF(1, 2) qtest_spawn_qemu(const char *fmt, ...)
+static QTestState *G_GNUC_PRINTF(2, 3) qtest_spawn_qemu(const char *qemu_bin,
+                                                        const char *fmt, ...)
 {
     va_list ap;
     QTestState *s = g_new0(QTestState, 1);
@@ -398,8 +399,7 @@ static QTestState *G_GNUC_PRINTF(1, 2) qtest_spawn_qemu(const char *fmt, ...)
     g_autoptr(GString) command = g_string_new("");
 
     va_start(ap, fmt);
-    g_string_append_printf(command, CMD_EXEC "%s %s",
-                           qtest_qemu_binary(NULL), tracearg);
+    g_string_append_printf(command, CMD_EXEC "%s %s", qemu_bin, tracearg);
     g_string_append_vprintf(command, fmt, ap);
     va_end(ap);
 
@@ -438,7 +438,8 @@ static QTestState *G_GNUC_PRINTF(1, 2) qtest_spawn_qemu(const char *fmt, ...)
     return s;
 }
 
-QTestState *qtest_init_without_qmp_handshake(const char *extra_args)
+static QTestState *qtest_init_internal(const char *qemu_bin,
+                                       const char *extra_args)
 {
     QTestState *s;
     int sock, qmpsock, i;
@@ -463,7 +464,8 @@ QTestState *qtest_init_without_qmp_handshake(const char *extra_args)
     sock = init_socket(socket_path);
     qmpsock = init_socket(qmp_socket_path);
 
-    s = qtest_spawn_qemu("-qtest unix:%s "
+    s = qtest_spawn_qemu(qemu_bin,
+                         "-qtest unix:%s "
                          "-qtest-log %s "
                          "-chardev socket,path=%s,id=char0 "
                          "-mon chardev=char0,mode=control "
@@ -516,9 +518,14 @@ QTestState *qtest_init_without_qmp_handshake(const char *extra_args)
     return s;
 }
 
-QTestState *qtest_init(const char *extra_args)
+QTestState *qtest_init_without_qmp_handshake(const char *extra_args)
 {
-    QTestState *s = qtest_init_without_qmp_handshake(extra_args);
+    return qtest_init_internal(qtest_qemu_binary(NULL), extra_args);
+}
+
+QTestState *qtest_init_with_env(const char *var, const char *extra_args)
+{
+    QTestState *s = qtest_init_internal(qtest_qemu_binary(var), extra_args);
     QDict *greeting;
 
     /* Read the QMP greeting and then do the handshake */
@@ -529,6 +536,11 @@ QTestState *qtest_init(const char *extra_args)
     return s;
 }
 
+QTestState *qtest_init(const char *extra_args)
+{
+    return qtest_init_with_env(NULL, extra_args);
+}
+
 QTestState *qtest_vinitf(const char *fmt, va_list ap)
 {
     char *args = g_strdup_vprintf(fmt, ap);
diff --git a/tests/qtest/libqtest.h b/tests/qtest/libqtest.h
index 5fe3d13466..76fc195f1c 100644
--- a/tests/qtest/libqtest.h
+++ b/tests/qtest/libqtest.h
@@ -55,6 +55,19 @@ QTestState *qtest_vinitf(const char *fmt, va_list ap) G_GNUC_PRINTF(1, 0);
  */
 QTestState *qtest_init(const char *extra_args);
 
+/**
+ * qtest_init_with_env:
+ * @var: Environment variable from where to take the QEMU binary
+ * @extra_args: Other arguments to pass to QEMU.  CAUTION: these
+ * arguments are subject to word splitting and shell evaluation.
+ *
+ * Like qtest_init(), but use a different environment variable for the
+ * QEMU binary.
+ *
+ * Returns: #QTestState instance.
+ */
+QTestState *qtest_init_with_env(const char *var, const char *extra_args);
+
 /**
  * qtest_init_without_qmp_handshake:
  * @extra_args: other arguments to pass to QEMU.  CAUTION: these
-- 
2.35.3

[PATCH v4 03/12] tests/qtest: Allow qtest_get_machines to use an alternate QEMU binary

[Fabiano Rosas]

We're adding support for using more than one QEMU binary in
tests. Modify qtest_get_machines() to take an environment variable
that contains the QEMU binary path.

Since the function keeps a cache of the machines list in the form of a
static variable, refresh it any time the environment variable changes.

Reviewed-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/libqtest.c | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index 9eebba8767..3cc7bf3076 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -1468,13 +1468,26 @@ struct MachInfo {
     char *alias;
 };
 
+static void qtest_free_machine_list(struct MachInfo *machines)
+{
+    if (machines) {
+        for (int i = 0; machines[i].name != NULL; i++) {
+            g_free(machines[i].name);
+            g_free(machines[i].alias);
+        }
+
+        g_free(machines);
+    }
+}
+
 /*
  * Returns an array with pointers to the available machine names.
  * The terminating entry has the name set to NULL.
  */
-static struct MachInfo *qtest_get_machines(void)
+static struct MachInfo *qtest_get_machines(const char *var)
 {
     static struct MachInfo *machines;
+    static char *qemu_var;
     QDict *response, *minfo;
     QList *list;
     const QListEntry *p;
@@ -1483,11 +1496,19 @@ static struct MachInfo *qtest_get_machines(void)
     QTestState *qts;
     int idx;
 
+    if (g_strcmp0(qemu_var, var)) {
+        qemu_var = g_strdup(var);
+
+        /* new qemu, clear the cache */
+        qtest_free_machine_list(machines);
+        machines = NULL;
+    }
+
     if (machines) {
         return machines;
     }
 
-    qts = qtest_init("-machine none");
+    qts = qtest_init_with_env(qemu_var, "-machine none");
     response = qtest_qmp(qts, "{ 'execute': 'query-machines' }");
     g_assert(response);
     list = qdict_get_qlist(response, "return");
@@ -1528,7 +1549,7 @@ void qtest_cb_for_every_machine(void (*cb)(const char *machine),
     struct MachInfo *machines;
     int i;
 
-    machines = qtest_get_machines();
+    machines = qtest_get_machines(NULL);
 
     for (i = 0; machines[i].name != NULL; i++) {
         /* Ignore machines that cannot be used for qtests */
@@ -1549,7 +1570,7 @@ bool qtest_has_machine(const char *machine)
     struct MachInfo *machines;
     int i;
 
-    machines = qtest_get_machines();
+    machines = qtest_get_machines(NULL);
 
     for (i = 0; machines[i].name != NULL; i++) {
         if (g_str_equal(machine, machines[i].name) ||
-- 
2.35.3

[PATCH v4 04/12] tests/qtest: Introduce qtest_has_machine_with_env

[Fabiano Rosas]

Add a variant of qtest_has_machine() that receives an environment
variable containing an alternate QEMU binary path.

Reviewed-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/libqtest.c | 9 +++++++--
 tests/qtest/libqtest.h | 9 +++++++++
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index 3cc7bf3076..603d900e7d 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -1565,12 +1565,12 @@ void qtest_cb_for_every_machine(void (*cb)(const char *machine),
     }
 }
 
-bool qtest_has_machine(const char *machine)
+bool qtest_has_machine_with_env(const char *var, const char *machine)
 {
     struct MachInfo *machines;
     int i;
 
-    machines = qtest_get_machines(NULL);
+    machines = qtest_get_machines(var);
 
     for (i = 0; machines[i].name != NULL; i++) {
         if (g_str_equal(machine, machines[i].name) ||
@@ -1582,6 +1582,11 @@ bool qtest_has_machine(const char *machine)
     return false;
 }
 
+bool qtest_has_machine(const char *machine)
+{
+    return qtest_has_machine_with_env(NULL, machine);
+}
+
 bool qtest_has_device(const char *device)
 {
     static QList *list;
diff --git a/tests/qtest/libqtest.h b/tests/qtest/libqtest.h
index 76fc195f1c..d16deb9891 100644
--- a/tests/qtest/libqtest.h
+++ b/tests/qtest/libqtest.h
@@ -930,6 +930,15 @@ void qtest_cb_for_every_machine(void (*cb)(const char *machine),
  */
 bool qtest_has_machine(const char *machine);
 
+/**
+ * qtest_has_machine_with_env:
+ * @var: Environment variable from where to take the QEMU binary
+ * @machine: The machine to look for
+ *
+ * Returns: true if the machine is available in the specified binary.
+ */
+bool qtest_has_machine_with_env(const char *var, const char *machine);
+
 /**
  * qtest_has_device:
  * @device: The device to look for
-- 
2.35.3

[PATCH v4 05/12] tests/qtest: Introduce qtest_resolve_machine_alias

[Fabiano Rosas]

The migration tests are being enhanced to test migration between
different QEMU versions. A requirement of migration is that the
machine type between source and destination matches, including the
version.

We cannot hardcode machine types in the tests because those change
with each release. QEMU provides a machine type alias that has a fixed
name, but points to the latest machine type at each release.

Add a helper to resolve the alias into the exact machine
type. E.g. "-machine pc" resolves to "pc-i440fx-8.2"

Reviewed-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/libqtest.c | 16 ++++++++++++++++
 tests/qtest/libqtest.h | 10 ++++++++++
 2 files changed, 26 insertions(+)

diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index 603d900e7d..c843c41188 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -1565,6 +1565,22 @@ void qtest_cb_for_every_machine(void (*cb)(const char *machine),
     }
 }
 
+char *qtest_resolve_machine_alias(const char *var, const char *alias)
+{
+    struct MachInfo *machines;
+    int i;
+
+    machines = qtest_get_machines(var);
+
+    for (i = 0; machines[i].name != NULL; i++) {
+        if (machines[i].alias && g_str_equal(alias, machines[i].alias)) {
+            return g_strdup(machines[i].name);
+        }
+    }
+
+    return NULL;
+}
+
 bool qtest_has_machine_with_env(const char *var, const char *machine)
 {
     struct MachInfo *machines;
diff --git a/tests/qtest/libqtest.h b/tests/qtest/libqtest.h
index d16deb9891..6e3d3525bf 100644
--- a/tests/qtest/libqtest.h
+++ b/tests/qtest/libqtest.h
@@ -922,6 +922,16 @@ void qtest_qmp_fds_assert_success(QTestState *qts, int *fds, size_t nfds,
 void qtest_cb_for_every_machine(void (*cb)(const char *machine),
                                 bool skip_old_versioned);
 
+/**
+ * qtest_resolve_machine_alias:
+ * @var: Environment variable from where to take the QEMU binary
+ * @alias: The alias to resolve
+ *
+ * Returns: the machine type corresponding to the alias if any,
+ * otherwise NULL.
+ */
+char *qtest_resolve_machine_alias(const char *var, const char *alias);
+
 /**
  * qtest_has_machine:
  * @machine: The machine to look for
-- 
2.35.3

[PATCH v4 06/12] tests/qtest/migration: Introduce find_common_machine_version

[Fabiano Rosas]

When using two different QEMU binaries for migration testing, we'll
need to find what is the machine version that will work with both
binaries. Add a helper for that.

Reviewed-by: Juan Quintela <quintela@redhat.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/migration-helpers.c | 26 ++++++++++++++++++++++++++
 tests/qtest/migration-helpers.h |  2 ++
 2 files changed, 28 insertions(+)

diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index 0c185db450..13449c1fe1 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -240,3 +240,29 @@ void wait_for_migration_fail(QTestState *from, bool allow_active)
     g_assert(qdict_get_bool(rsp_return, "running"));
     qobject_unref(rsp_return);
 }
+
+char *find_common_machine_version(const char *mtype, const char *var1,
+                                  const char *var2)
+{
+    g_autofree char *type1 = qtest_resolve_machine_alias(var1, mtype);
+    g_autofree char *type2 = qtest_resolve_machine_alias(var2, mtype);
+
+    g_assert(type1 && type2);
+
+    if (g_str_equal(type1, type2)) {
+        /* either can be used */
+        return g_strdup(type1);
+    }
+
+    if (qtest_has_machine_with_env(var2, type1)) {
+        return g_strdup(type1);
+    }
+
+    if (qtest_has_machine_with_env(var1, type2)) {
+        return g_strdup(type2);
+    }
+
+    g_test_message("No common machine version for machine type '%s' between "
+                   "binaries %s and %s", mtype, getenv(var1), getenv(var2));
+    g_assert_not_reached();
+}
diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h
index 4f51d0f8bc..d1c2351d33 100644
--- a/tests/qtest/migration-helpers.h
+++ b/tests/qtest/migration-helpers.h
@@ -43,4 +43,6 @@ void wait_for_migration_complete(QTestState *who);
 
 void wait_for_migration_fail(QTestState *from, bool allow_active);
 
+char *find_common_machine_version(const char *mtype, const char *var1,
+                                  const char *var2);
 #endif /* MIGRATION_HELPERS_H */
-- 
2.35.3

[PATCH v4 07/12] tests/qtest/migration: Define a machine for all architectures

[Fabiano Rosas]

Stop relying on defaults and select a machine explicitly for every
architecture.

This is a prerequisite for being able to select machine types for
migration using different QEMU binaries for source and destination.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/migration-test.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index e1c110537b..43d0b83771 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -743,6 +743,7 @@ static int test_migrate_start(QTestState **from, QTestState **to,
     const char *kvm_opts = NULL;
     const char *arch = qtest_get_arch();
     const char *memory_size;
+    const char *machine_alias, *machine_opts = "";
 
     if (args->use_shmem) {
         if (!g_file_test("/dev/shm", G_FILE_TEST_IS_DIR)) {
@@ -755,11 +756,13 @@ static int test_migrate_start(QTestState **from, QTestState **to,
     got_dst_resume = false;
     if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
         memory_size = "150M";
+        machine_alias = "pc";
         arch_opts = g_strdup_printf("-drive file=%s,format=raw", bootpath);
         start_address = X86_TEST_MEM_START;
         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;
@@ -771,11 +774,14 @@ static int test_migrate_start(QTestState **from, QTestState **to,
                                       "'nvramrc=hex .\" _\" begin %x %x "
                                       "do i c@ 1 + i c! 1000 +loop .\" B\" 0 "
                                       "until'", end_address, start_address);
-        arch_opts = g_strdup("-nodefaults -machine vsmt=8");
+        machine_alias = "pseries";
+        machine_opts = "vsmt=8";
+        arch_opts = g_strdup("-nodefaults");
     } else if (strcmp(arch, "aarch64") == 0) {
         memory_size = "150M";
-        arch_opts = g_strdup_printf("-machine virt,gic-version=max -cpu max "
-                                    "-kernel %s", bootpath);
+        machine_alias = "virt";
+        machine_opts = "gic-version=max";
+        arch_opts = g_strdup_printf("-cpu max -kernel %s", bootpath);
         start_address = ARM_TEST_MEM_START;
         end_address = ARM_TEST_MEM_END;
     } else {
@@ -810,11 +816,13 @@ static int test_migrate_start(QTestState **from, QTestState **to,
     }
 
     cmd_source = g_strdup_printf("-accel kvm%s -accel tcg "
+                                 "-machine %s,%s "
                                  "-name source,debug-threads=on "
                                  "-m %s "
                                  "-serial file:%s/src_serial "
                                  "%s %s %s %s %s",
                                  kvm_opts ? kvm_opts : "",
+                                 machine_alias, machine_opts,
                                  memory_size, tmpfs,
                                  arch_opts ? arch_opts : "",
                                  arch_source ? arch_source : "",
@@ -829,12 +837,14 @@ static int test_migrate_start(QTestState **from, QTestState **to,
     }
 
     cmd_target = g_strdup_printf("-accel kvm%s -accel tcg "
+                                 "-machine %s,%s "
                                  "-name target,debug-threads=on "
                                  "-m %s "
                                  "-serial file:%s/dest_serial "
                                  "-incoming %s "
                                  "%s %s %s %s %s",
                                  kvm_opts ? kvm_opts : "",
+                                 machine_alias, machine_opts,
                                  memory_size, tmpfs, uri,
                                  arch_opts ? arch_opts : "",
                                  arch_target ? arch_target : "",
-- 
2.35.3

[PATCH v4 08/12] tests/qtest/migration: Specify the geometry of the bootsector

[Fabiano Rosas]

We're about to enable the x86_64 tests to run with the q35 machine,
but that machine does not work with the program we use to dirty the
memory for the tests.

The issue is that QEMU needs to guess the geometry of the "disk" we
give to it and the guessed geometry doesn't pass the sanity checks
done by SeaBIOS. This causes SeaBIOS to interpret the geometry as if
needing a translation from LBA to CHS and SeaBIOS ends up miscomputing
the number of cylinders and aborting due to that.

The reason things work with the "pc" machine is that is uses ATA
instead of AHCI like q35 and SeaBIOS has an exception for ATA that
ends up skipping the sanity checks and ignoring translation
altogether.

Workaround this situation by specifying a geometry in the command
line.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/migration-test.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 43d0b83771..b45a389de8 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -757,7 +757,9 @@ static int test_migrate_start(QTestState **from, QTestState **to,
     if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
         memory_size = "150M";
         machine_alias = "pc";
-        arch_opts = g_strdup_printf("-drive file=%s,format=raw", bootpath);
+        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);
         start_address = X86_TEST_MEM_START;
         end_address = X86_TEST_MEM_END;
     } else if (g_str_equal(arch, "s390x")) {
-- 
2.35.3

[PATCH v4 09/12] tests/qtest/migration: Set q35 as the default machine for x86_86

[Fabiano Rosas]

Change the x86_64 to use the q35 machines in tests from now on. Keep
testing the pc macine on 32bit.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
this could affect bisecting, so I put it in separate patch to be
easier to revert
---
 tests/qtest/migration-test.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index b45a389de8..b718634b1c 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -756,7 +756,12 @@ static int test_migrate_start(QTestState **from, QTestState **to,
     got_dst_resume = false;
     if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
         memory_size = "150M";
-        machine_alias = "pc";
+
+        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);
-- 
2.35.3

[PATCH v4 10/12] tests/qtest/migration: Support more than one QEMU binary

[Fabiano Rosas]

We have strict rules around migration compatibility between different
QEMU versions but no test to validate the migration state between
different binaries.

Add infrastructure to allow running the migration tests with two
different QEMU binaries as migration source and destination.

The code now recognizes two new environment variables
QTEST_QEMU_BINARY_SRC and QTEST_QEMU_BINARY_DST. In the absence of
either of them, the test will use the QTEST_QEMU_BINARY variable. If
both are missing then the tests are run with single binary as
previously.

The machine type is selected automatically as the latest machine type
version that works with both binaries.

Usage (only one of SRC|DST is allowed):

QTEST_QEMU_BINARY_SRC=../build-8.2.0/qemu-system-x86_64 \
QTEST_QEMU_BINARY=../build-8.1.0/qemu-system-x86_64 \
./tests/qtest/migration-test

Reviewed-by: Juan Quintela <quintela@redhat.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/migration-test.c | 28 ++++++++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index b718634b1c..51f5603aac 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -71,6 +71,8 @@ static bool got_dst_resume;
 #define QEMU_VM_FILE_MAGIC 0x5145564d
 #define FILE_TEST_FILENAME "migfile"
 #define FILE_TEST_OFFSET 0x1000
+#define QEMU_ENV_SRC "QTEST_QEMU_BINARY_SRC"
+#define QEMU_ENV_DST "QTEST_QEMU_BINARY_DST"
 
 #if defined(__linux__)
 #include <sys/syscall.h>
@@ -744,6 +746,7 @@ static int test_migrate_start(QTestState **from, QTestState **to,
     const char *arch = qtest_get_arch();
     const char *memory_size;
     const char *machine_alias, *machine_opts = "";
+    g_autofree char *machine = NULL;
 
     if (args->use_shmem) {
         if (!g_file_test("/dev/shm", G_FILE_TEST_IS_DIR)) {
@@ -822,6 +825,10 @@ static int test_migrate_start(QTestState **from, QTestState **to,
         kvm_opts = ",dirty-ring-size=4096";
     }
 
+    machine = find_common_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 "
                                  "-machine %s,%s "
                                  "-name source,debug-threads=on "
@@ -829,7 +836,7 @@ static int test_migrate_start(QTestState **from, QTestState **to,
                                  "-serial file:%s/src_serial "
                                  "%s %s %s %s %s",
                                  kvm_opts ? kvm_opts : "",
-                                 machine_alias, machine_opts,
+                                 machine, machine_opts,
                                  memory_size, tmpfs,
                                  arch_opts ? arch_opts : "",
                                  arch_source ? arch_source : "",
@@ -837,7 +844,7 @@ static int test_migrate_start(QTestState **from, QTestState **to,
                                  args->opts_source ? args->opts_source : "",
                                  ignore_stderr);
     if (!args->only_target) {
-        *from = qtest_init(cmd_source);
+        *from = qtest_init_with_env(QEMU_ENV_SRC, cmd_source);
         qtest_qmp_set_event_callback(*from,
                                      migrate_watch_for_stop,
                                      &got_src_stop);
@@ -851,14 +858,14 @@ static int test_migrate_start(QTestState **from, QTestState **to,
                                  "-incoming %s "
                                  "%s %s %s %s %s",
                                  kvm_opts ? kvm_opts : "",
-                                 machine_alias, machine_opts,
+                                 machine, machine_opts,
                                  memory_size, tmpfs, uri,
                                  arch_opts ? arch_opts : "",
                                  arch_target ? arch_target : "",
                                  shmem_opts ? shmem_opts : "",
                                  args->opts_target ? args->opts_target : "",
                                  ignore_stderr);
-    *to = qtest_init(cmd_target);
+    *to = qtest_init_with_env(QEMU_ENV_DST, cmd_target);
     qtest_qmp_set_event_callback(*to,
                                  migrate_watch_for_resume,
                                  &got_dst_resume);
@@ -2989,10 +2996,23 @@ int main(int argc, char **argv)
     bool has_uffd;
     const char *arch;
     g_autoptr(GError) err = NULL;
+    const char *qemu_src = getenv(QEMU_ENV_SRC);
+    const char *qemu_dst = getenv(QEMU_ENV_DST);
     int ret;
 
     g_test_init(&argc, &argv, NULL);
 
+    /*
+     * The default QTEST_QEMU_BINARY must always be provided because
+     * that is what helpers use to query the accel type and
+     * architecture.
+     */
+    if (qemu_src && qemu_dst) {
+        g_test_message("Only one of %s, %s is allowed",
+                       QEMU_ENV_SRC, QEMU_ENV_DST);
+        exit(1);
+    }
+
     has_kvm = qtest_has_accel("kvm");
     has_tcg = qtest_has_accel("tcg");
 
-- 
2.35.3

[PATCH v4 11/12] tests/qtest/migration: Allow user to specify a machine type

[Fabiano Rosas]

Accept the QTEST_QEMU_MACHINE_TYPE environment variable to take a
machine type to use in the tests.

The full machine type is recognized (e.g. pc-q35-8.2). Aliases
(e.g. pc) are also allowed and resolve to the latest machine version
for that alias, or, if using two QEMU binaries, to the latest common
machine version between the two.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/migration-helpers.c | 26 ++++++++++++++++++++++++++
 tests/qtest/migration-helpers.h |  2 ++
 tests/qtest/migration-test.c    |  5 +++--
 3 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index 13449c1fe1..24fb7b3525 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -11,6 +11,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/ctype.h"
 #include "qapi/qmp/qjson.h"
 
 #include "migration-helpers.h"
@@ -266,3 +267,28 @@ char *find_common_machine_version(const char *mtype, const char *var1,
                    "binaries %s and %s", mtype, getenv(var1), getenv(var2));
     g_assert_not_reached();
 }
+
+char *resolve_machine_version(const char *alias, const char *var1,
+                              const char *var2)
+{
+    const char *mname = g_getenv("QTEST_QEMU_MACHINE_TYPE");
+    g_autofree char *machine_name = NULL;
+
+    if (mname) {
+        const char *dash = strrchr(mname, '-');
+        const char *dot = strrchr(mname, '.');
+
+        machine_name = g_strdup(mname);
+
+        if (dash && dot) {
+            assert(qtest_has_machine(machine_name));
+            return g_steal_pointer(&machine_name);
+        }
+        /* else: probably an alias, let it be resolved below */
+    } else {
+        /* use the hardcoded alias */
+        machine_name = g_strdup(alias);
+    }
+
+    return find_common_machine_version(machine_name, var1, var2);
+}
diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h
index d1c2351d33..e31dc85cc7 100644
--- a/tests/qtest/migration-helpers.h
+++ b/tests/qtest/migration-helpers.h
@@ -45,4 +45,6 @@ void wait_for_migration_fail(QTestState *from, bool allow_active);
 
 char *find_common_machine_version(const char *mtype, const char *var1,
                                   const char *var2);
+char *resolve_machine_version(const char *alias, const char *var1,
+                              const char *var2);
 #endif /* MIGRATION_HELPERS_H */
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 51f5603aac..e15ac3b353 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -825,8 +825,9 @@ static int test_migrate_start(QTestState **from, QTestState **to,
         kvm_opts = ",dirty-ring-size=4096";
     }
 
-    machine = find_common_machine_version(machine_alias, QEMU_ENV_SRC,
-                                          QEMU_ENV_DST);
+    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 v4 12/12] tests/qtest: Don't print messages from query instances

[Fabiano Rosas]

Now that we can query more than one binary, the "starting QEMU..."
message can get a little noisy. Mute those messages unless we're
running with --verbose.

Only affects qtest_init() calls from within libqtest. The tests
continue to output as usual.

Reviewed-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/libqtest.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index c843c41188..f33a210861 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -91,6 +91,7 @@ struct QTestState
 
 static GHookList abrt_hooks;
 static void (*sighandler_old)(int);
+static bool silence_spawn_log;
 
 static int qtest_query_target_endianness(QTestState *s);
 
@@ -405,7 +406,9 @@ static QTestState *G_GNUC_PRINTF(2, 3) qtest_spawn_qemu(const char *qemu_bin,
 
     qtest_add_abrt_handler(kill_qemu_hook_func, s);
 
-    g_test_message("starting QEMU: %s", command->str);
+    if (!silence_spawn_log) {
+        g_test_message("starting QEMU: %s", command->str);
+    }
 
 #ifndef _WIN32
     s->qemu_pid = fork();
@@ -1508,6 +1511,8 @@ static struct MachInfo *qtest_get_machines(const char *var)
         return machines;
     }
 
+    silence_spawn_log = !g_test_verbose();
+
     qts = qtest_init_with_env(qemu_var, "-machine none");
     response = qtest_qmp(qts, "{ 'execute': 'query-machines' }");
     g_assert(response);
@@ -1539,6 +1544,8 @@ static struct MachInfo *qtest_get_machines(const char *var)
     qtest_quit(qts);
     qobject_unref(response);
 
+    silence_spawn_log = false;
+
     memset(&machines[idx], 0, sizeof(struct MachInfo)); /* Terminating entry */
     return machines;
 }
-- 
2.35.3

Re: [PATCH v4 02/12] tests/qtest: Introduce qtest_init_with_env

[Peter Xu]

On Wed, Oct 18, 2023 at 04:27:31PM -0300, Fabiano Rosas wrote:
> +/**
> + * qtest_init_with_env:
> + * @var: Environment variable from where to take the QEMU binary
> + * @extra_args: Other arguments to pass to QEMU.  CAUTION: these
> + * arguments are subject to word splitting and shell evaluation.
> + *
> + * Like qtest_init(), but use a different environment variable for the
> + * QEMU binary.
> + *
> + * Returns: #QTestState instance.
> + */
> +QTestState *qtest_init_with_env(const char *var, const char *extra_args);

Another way to do is instead of passing over the env var, passing over
"char *qemu_bin" always, and take qtest_qemu_binary() as default.  Also
relevant to patch 1.  Not a big deal though, so can be done for later.

Thanks,

-- 
Peter Xu

Re: [PATCH v4 06/12] tests/qtest/migration: Introduce find_common_machine_version

[Thomas Huth]

On 18/10/2023 21.27, Fabiano Rosas wrote:
> When using two different QEMU binaries for migration testing, we'll
> need to find what is the machine version that will work with both
> binaries. Add a helper for that.
> 
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>   tests/qtest/migration-helpers.c | 26 ++++++++++++++++++++++++++
>   tests/qtest/migration-helpers.h |  2 ++
>   2 files changed, 28 insertions(+)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v4 07/12] tests/qtest/migration: Define a machine for all architectures

[Thomas Huth]

On 18/10/2023 21.27, Fabiano Rosas wrote:
> Stop relying on defaults and select a machine explicitly for every
> architecture.
> 
> This is a prerequisite for being able to select machine types for
> migration using different QEMU binaries for source and destination.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>   tests/qtest/migration-test.c | 16 +++++++++++++---
>   1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index e1c110537b..43d0b83771 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -743,6 +743,7 @@ static int test_migrate_start(QTestState **from, QTestState **to,
>       const char *kvm_opts = NULL;
>       const char *arch = qtest_get_arch();
>       const char *memory_size;
> +    const char *machine_alias, *machine_opts = "";
>   
>       if (args->use_shmem) {
>           if (!g_file_test("/dev/shm", G_FILE_TEST_IS_DIR)) {
> @@ -755,11 +756,13 @@ static int test_migrate_start(QTestState **from, QTestState **to,
>       got_dst_resume = false;
>       if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
>           memory_size = "150M";
> +        machine_alias = "pc";
>           arch_opts = g_strdup_printf("-drive file=%s,format=raw", bootpath);
>           start_address = X86_TEST_MEM_START;
>           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;
> @@ -771,11 +774,14 @@ static int test_migrate_start(QTestState **from, QTestState **to,
>                                         "'nvramrc=hex .\" _\" begin %x %x "
>                                         "do i c@ 1 + i c! 1000 +loop .\" B\" 0 "
>                                         "until'", end_address, start_address);
> -        arch_opts = g_strdup("-nodefaults -machine vsmt=8");
> +        machine_alias = "pseries";
> +        machine_opts = "vsmt=8";
> +        arch_opts = g_strdup("-nodefaults");
>       } else if (strcmp(arch, "aarch64") == 0) {
>           memory_size = "150M";
> -        arch_opts = g_strdup_printf("-machine virt,gic-version=max -cpu max "
> -                                    "-kernel %s", bootpath);
> +        machine_alias = "virt";
> +        machine_opts = "gic-version=max";
> +        arch_opts = g_strdup_printf("-cpu max -kernel %s", bootpath);
>           start_address = ARM_TEST_MEM_START;
>           end_address = ARM_TEST_MEM_END;
>       } else {
> @@ -810,11 +816,13 @@ static int test_migrate_start(QTestState **from, QTestState **to,
>       }
>   
>       cmd_source = g_strdup_printf("-accel kvm%s -accel tcg "
> +                                 "-machine %s,%s "
>                                    "-name source,debug-threads=on "
>                                    "-m %s "
>                                    "-serial file:%s/src_serial "
>                                    "%s %s %s %s %s",
>                                    kvm_opts ? kvm_opts : "",
> +                                 machine_alias, machine_opts,
>                                    memory_size, tmpfs,
>                                    arch_opts ? arch_opts : "",
>                                    arch_source ? arch_source : "",
> @@ -829,12 +837,14 @@ static int test_migrate_start(QTestState **from, QTestState **to,
>       }
>   
>       cmd_target = g_strdup_printf("-accel kvm%s -accel tcg "
> +                                 "-machine %s,%s "

If machine_opts is empty, there will be a lonely "," at the end of the 
parameter ... seems to work, but it's a little bit ugly.

Anyway:
Reviewed-by: Thomas Huth <thuth@redhat.com>


>                                    "-name target,debug-threads=on "
>                                    "-m %s "
>                                    "-serial file:%s/dest_serial "
>                                    "-incoming %s "
>                                    "%s %s %s %s %s",
>                                    kvm_opts ? kvm_opts : "",
> +                                 machine_alias, machine_opts,
>                                    memory_size, tmpfs, uri,
>                                    arch_opts ? arch_opts : "",
>                                    arch_target ? arch_target : "",

Re: [PATCH v4 08/12] tests/qtest/migration: Specify the geometry of the bootsector

[Thomas Huth]

On 18/10/2023 21.27, Fabiano Rosas wrote:
> We're about to enable the x86_64 tests to run with the q35 machine,
> but that machine does not work with the program we use to dirty the
> memory for the tests.
> 
> The issue is that QEMU needs to guess the geometry of the "disk" we
> give to it and the guessed geometry doesn't pass the sanity checks
> done by SeaBIOS. This causes SeaBIOS to interpret the geometry as if
> needing a translation from LBA to CHS and SeaBIOS ends up miscomputing
> the number of cylinders and aborting due to that.
> 
> The reason things work with the "pc" machine is that is uses ATA
> instead of AHCI like q35 and SeaBIOS has an exception for ATA that
> ends up skipping the sanity checks and ignoring translation
> altogether.
> 
> Workaround this situation by specifying a geometry in the command
> line.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>   tests/qtest/migration-test.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index 43d0b83771..b45a389de8 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -757,7 +757,9 @@ static int test_migrate_start(QTestState **from, QTestState **to,
>       if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
>           memory_size = "150M";
>           machine_alias = "pc";
> -        arch_opts = g_strdup_printf("-drive file=%s,format=raw", bootpath);
> +        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);
>           start_address = X86_TEST_MEM_START;
>           end_address = X86_TEST_MEM_END;
>       } else if (g_str_equal(arch, "s390x")) {

Acked-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v4 09/12] tests/qtest/migration: Set q35 as the default machine for x86_86

[Thomas Huth]

On 18/10/2023 21.27, Fabiano Rosas wrote:
> Change the x86_64 to use the q35 machines in tests from now on. Keep
> testing the pc macine on 32bit.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
> this could affect bisecting, so I put it in separate patch to be
> easier to revert
> ---
>   tests/qtest/migration-test.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index b45a389de8..b718634b1c 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -756,7 +756,12 @@ static int test_migrate_start(QTestState **from, QTestState **to,
>       got_dst_resume = false;
>       if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
>           memory_size = "150M";
> -        machine_alias = "pc";
> +
> +        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);

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v4 10/12] tests/qtest/migration: Support more than one QEMU binary

[Thomas Huth]

On 18/10/2023 21.27, Fabiano Rosas wrote:
> We have strict rules around migration compatibility between different
> QEMU versions but no test to validate the migration state between
> different binaries.
> 
> Add infrastructure to allow running the migration tests with two
> different QEMU binaries as migration source and destination.
> 
> The code now recognizes two new environment variables
> QTEST_QEMU_BINARY_SRC and QTEST_QEMU_BINARY_DST. In the absence of
> either of them, the test will use the QTEST_QEMU_BINARY variable. If
> both are missing then the tests are run with single binary as
> previously.
> 
> The machine type is selected automatically as the latest machine type
> version that works with both binaries.
> 
> Usage (only one of SRC|DST is allowed):
> 
> QTEST_QEMU_BINARY_SRC=../build-8.2.0/qemu-system-x86_64 \
> QTEST_QEMU_BINARY=../build-8.1.0/qemu-system-x86_64 \
> ./tests/qtest/migration-test
> 
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>   tests/qtest/migration-test.c | 28 ++++++++++++++++++++++++----
>   1 file changed, 24 insertions(+), 4 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

I wonder whether we could test this in the gitlab-CI, too, e.g. by using a 
Debian container and installing the qemu-system-x86_64 from the Debian 
distro there (since this should be close enough to an older version of an 
upstream release), then run the test with that version from Debian and the 
one that has just been compiled from the master branch? Anyway, just an 
idea, this can certainly be done later.

  Thomas

Re: [PATCH v4 11/12] tests/qtest/migration: Allow user to specify a machine type

[Thomas Huth]

On 18/10/2023 21.27, Fabiano Rosas wrote:
> Accept the QTEST_QEMU_MACHINE_TYPE environment variable to take a
> machine type to use in the tests.
> 
> The full machine type is recognized (e.g. pc-q35-8.2). Aliases
> (e.g. pc) are also allowed and resolve to the latest machine version
> for that alias, or, if using two QEMU binaries, to the latest common
> machine version between the two.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>   tests/qtest/migration-helpers.c | 26 ++++++++++++++++++++++++++
>   tests/qtest/migration-helpers.h |  2 ++
>   tests/qtest/migration-test.c    |  5 +++--
>   3 files changed, 31 insertions(+), 2 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v4 07/12] tests/qtest/migration: Define a machine for all architectures

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> On 18/10/2023 21.27, Fabiano Rosas wrote:
>> Stop relying on defaults and select a machine explicitly for every
>> architecture.
>> This is a prerequisite for being able to select machine types for
>> migration using different QEMU binaries for source and destination.
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>   tests/qtest/migration-test.c | 16 +++++++++++++---
>>   1 file changed, 13 insertions(+), 3 deletions(-)
>> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
>> index e1c110537b..43d0b83771 100644
>> --- a/tests/qtest/migration-test.c
>> +++ b/tests/qtest/migration-test.c

[...]

>> @@ -829,12 +837,14 @@ static int test_migrate_start(QTestState **from, QTestState **to,
>>       }
>>         cmd_target = g_strdup_printf("-accel kvm%s -accel tcg "
>> +                                 "-machine %s,%s "
>
> If machine_opts is empty, there will be a lonely "," at the end of the parameter ... seems to work, but it's a little bit ugly.

keyval_parse() & friends accept trailing ',' to help with keeping things
simple.

> Anyway:
> Reviewed-by: Thomas Huth <thuth@redhat.com>
>
>
>>                                    "-name target,debug-threads=on "
>>                                    "-m %s "
>>                                    "-serial file:%s/dest_serial "
>>                                    "-incoming %s "
>>                                    "%s %s %s %s %s",
>>                                    kvm_opts ? kvm_opts : "",
>> +                                 machine_alias, machine_opts,
>>                                    memory_size, tmpfs, uri,
>>                                    arch_opts ? arch_opts : "",
>>                                    arch_target ? arch_target : "",

Re: [PATCH v4 10/12] tests/qtest/migration: Support more than one QEMU binary

[Juan Quintela]

Thomas Huth <thuth@redhat.com> wrote:
> On 18/10/2023 21.27, Fabiano Rosas wrote:
>> We have strict rules around migration compatibility between different
>> QEMU versions but no test to validate the migration state between
>> different binaries.
>> Add infrastructure to allow running the migration tests with two
>> different QEMU binaries as migration source and destination.
>> The code now recognizes two new environment variables
>> QTEST_QEMU_BINARY_SRC and QTEST_QEMU_BINARY_DST. In the absence of
>> either of them, the test will use the QTEST_QEMU_BINARY variable. If
>> both are missing then the tests are run with single binary as
>> previously.
>> The machine type is selected automatically as the latest machine
>> type
>> version that works with both binaries.
>> Usage (only one of SRC|DST is allowed):
>> QTEST_QEMU_BINARY_SRC=../build-8.2.0/qemu-system-x86_64 \
>> QTEST_QEMU_BINARY=../build-8.1.0/qemu-system-x86_64 \
>> ./tests/qtest/migration-test
>> Reviewed-by: Juan Quintela <quintela@redhat.com>
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>   tests/qtest/migration-test.c | 28 ++++++++++++++++++++++++----
>>   1 file changed, 24 insertions(+), 4 deletions(-)
>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
>
> I wonder whether we could test this in the gitlab-CI, too, e.g. by
> using a Debian container and installing the qemu-system-x86_64 from
> the Debian distro there (since this should be close enough to an older
> version of an upstream release), then run the test with that version
> from Debian and the one that has just been compiled from the master
> branch? Anyway, just an idea, this can certainly be done later.

My idea would be to modify the container to create two trees:
- last released version
- upstream tip

And just use that two binaries with the upstream one handling this.

Later, Juan.

Re: [PATCH v4 07/12] tests/qtest/migration: Define a machine for all architectures

[Juan Quintela]

Fabiano Rosas <farosas@suse.de> wrote:
> Stop relying on defaults and select a machine explicitly for every
> architecture.
>
> This is a prerequisite for being able to select machine types for
> migration using different QEMU binaries for source and destination.
>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

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


I have patches to simplify this, but will rebase on top of yours.

Re: [PATCH v4 08/12] tests/qtest/migration: Specify the geometry of the bootsector

[Juan Quintela]

Fabiano Rosas <farosas@suse.de> wrote:
> We're about to enable the x86_64 tests to run with the q35 machine,
> but that machine does not work with the program we use to dirty the
> memory for the tests.
>
> The issue is that QEMU needs to guess the geometry of the "disk" we
> give to it and the guessed geometry doesn't pass the sanity checks
> done by SeaBIOS. This causes SeaBIOS to interpret the geometry as if
> needing a translation from LBA to CHS and SeaBIOS ends up miscomputing
> the number of cylinders and aborting due to that.
>
> The reason things work with the "pc" machine is that is uses ATA
> instead of AHCI like q35 and SeaBIOS has an exception for ATA that
> ends up skipping the sanity checks and ignoring translation
> altogether.
>
> Workaround this situation by specifying a geometry in the command
> line.
>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

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

Re: [PATCH v4 09/12] tests/qtest/migration: Set q35 as the default machine for x86_86

[Juan Quintela]

Fabiano Rosas <farosas@suse.de> wrote:
> Change the x86_64 to use the q35 machines in tests from now on. Keep
> testing the pc macine on 32bit.
>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

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

Re: [PATCH v4 11/12] tests/qtest/migration: Allow user to specify a machine type

[Juan Quintela]

Fabiano Rosas <farosas@suse.de> wrote:
> Accept the QTEST_QEMU_MACHINE_TYPE environment variable to take a
> machine type to use in the tests.
>
> The full machine type is recognized (e.g. pc-q35-8.2). Aliases
> (e.g. pc) are also allowed and resolve to the latest machine version
> for that alias, or, if using two QEMU binaries, to the latest common
> machine version between the two.
>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

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

Thanks.

Re: [PATCH v4 00/12] tests/migration-test: Allow testing older machine types

[Juan Quintela]

Fabiano Rosas <farosas@suse.de> wrote:
> (please ignore v3 which was bogus, but don't miss the discussion in it
> about the caveats of this approach:
> https://lore.kernel.org/r/87jzrkdne2.fsf@suse.de)

queued for next pull request.

Re: [PATCH v4 10/12] tests/qtest/migration: Support more than one QEMU binary

[Fabiano Rosas]

Thomas Huth <thuth@redhat.com> writes:

> On 18/10/2023 21.27, Fabiano Rosas wrote:
>> We have strict rules around migration compatibility between different
>> QEMU versions but no test to validate the migration state between
>> different binaries.
>> 
>> Add infrastructure to allow running the migration tests with two
>> different QEMU binaries as migration source and destination.
>> 
>> The code now recognizes two new environment variables
>> QTEST_QEMU_BINARY_SRC and QTEST_QEMU_BINARY_DST. In the absence of
>> either of them, the test will use the QTEST_QEMU_BINARY variable. If
>> both are missing then the tests are run with single binary as
>> previously.
>> 
>> The machine type is selected automatically as the latest machine type
>> version that works with both binaries.
>> 
>> Usage (only one of SRC|DST is allowed):
>> 
>> QTEST_QEMU_BINARY_SRC=../build-8.2.0/qemu-system-x86_64 \
>> QTEST_QEMU_BINARY=../build-8.1.0/qemu-system-x86_64 \
>> ./tests/qtest/migration-test
>> 
>> Reviewed-by: Juan Quintela <quintela@redhat.com>
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>   tests/qtest/migration-test.c | 28 ++++++++++++++++++++++++----
>>   1 file changed, 24 insertions(+), 4 deletions(-)
>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
>
> I wonder whether we could test this in the gitlab-CI, too, e.g. by using a 
> Debian container and installing the qemu-system-x86_64 from the Debian 
> distro there (since this should be close enough to an older version of an 
> upstream release), then run the test with that version from Debian and the 
> one that has just been compiled from the master branch? Anyway, just an 
> idea, this can certainly be done later.

Yes, something like this is the goal. It's not in this series because my
docker-fu is a bit rusty, so I didn't want to delay the qtest part.

I think taking a built-from-tree QEMU would be better than a
distro-shipped one.

I also think that we should have this disabled in CI, due to the issues
I described in the other thread. And possibly enable it with fewer
migration-test tests. I don't see the need to run *all* of the
migration-tests in this "compat testing" scheme.

Re: [PATCH v4 02/12] tests/qtest: Introduce qtest_init_with_env

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> On Wed, Oct 18, 2023 at 04:27:31PM -0300, Fabiano Rosas wrote:
>> +/**
>> + * qtest_init_with_env:
>> + * @var: Environment variable from where to take the QEMU binary
>> + * @extra_args: Other arguments to pass to QEMU.  CAUTION: these
>> + * arguments are subject to word splitting and shell evaluation.
>> + *
>> + * Like qtest_init(), but use a different environment variable for the
>> + * QEMU binary.
>> + *
>> + * Returns: #QTestState instance.
>> + */
>> +QTestState *qtest_init_with_env(const char *var, const char *extra_args);
>
> Another way to do is instead of passing over the env var, passing over
> "char *qemu_bin" always, and take qtest_qemu_binary() as default.  Also
> relevant to patch 1.  Not a big deal though, so can be done for later.

Here's an low-hanging fruit if someone's interested. =)

I'll take note and do it when I get the chance after the PR merges
otherwise.

Thanks Peter