[PATCH v2 00/24] migration: small cleanup series

[PATCH v2 00/24] migration: small cleanup series

[Peter Xu]

CI: https://gitlab.com/peterx/qemu/-/pipelines/2289719627

v1: https://lore.kernel.org/r/20260121223336.3381912-1-peterx@redhat.com

This is the v2 of the small cleanup series.  It used to be only for COLO,
but now for some other things too around dumping vmstates.

The small series growed a little bit, because I wanted to try Fabiano's
request to deduplicate qemu_save_device_state() and precopy's similar
version to dump non-iterable devices.

Changelog skipped, as too many things added.  Old patches should almost the
same with R-bs collected, though.  I still touched up some small things,
though (where I dropped the R-bs).

This should pass the new COLO unit test too, I hope I didn't overlook
something once more. Please anyone let me know if it fails.

Comments welcomed, thanks.

Peter Xu (24):
  migration: Introduce qemu_savevm_send_* helpers
  migration: Use qemu_savevm_send_header() in qemu_save_device_state()
  migration: Remove one migration_in_colo_state() occurance
  migration/savevm: Remove SaveStateEntry.is_ram
  migration/colo: Unwrap qemu_savevm_live_state()
  migration: Remove call to send switchover start event in colo/savevm
  colo: Forbid VM resume during checkpointing
  migration/colo: Use the RAM iterable helper directly
  migration/colo: Document qemu_fflush(fb)
  migration: Drop iterable_only in qemu_savevm_state_complete_precopy
  migration: Drop qemu_file_set_error() when save non-iterable fails
  migration/colo: Send device states without copying buffer
  migration/postcopy: Send device states without copying buffer
  migration: Introduce qemu_savevm_state_end()
  migration: Provide helper for save vm description
  migration: Split qemu_savevm_state_complete_precopy_non_iterable()
  migration: qemu_savevm_state_complete_precopy() take MigrationState*
  migration: Cleanup error propagates in qemu_savevm_state_setup()
  migration: Refactor qemu_savevm_state_setup()
  migration: Introduce qemu_savevm_state_active()
  migration/bg-snapshot: Cleanup error paths
  migration: Make qemu_savevm_state_non_iterable() take errp
  migration: Simplify qemu_save_device_state()
  migration/colo/xen: Use generic helpers in qemu_save_device_state()

 migration/savevm.h    |  18 +--
 migration/colo.c      |  16 ++-
 migration/migration.c |  48 +++----
 migration/savevm.c    | 312 ++++++++++++++++++++++--------------------
 monitor/qmp-cmds.c    |   3 +
 5 files changed, 213 insertions(+), 184 deletions(-)

-- 
2.50.1

[PATCH v2 01/24] migration: Introduce qemu_savevm_send_* helpers

[Peter Xu]

Split qemu_savevm_state_header() into two parts.   This paves way for a
reuse elsewhere.

Reviewed-by: Fabiano Rosas <farosas@suse.de>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/savevm.h |  1 +
 migration/savevm.c | 58 +++++++++++++++++++++++++++-------------------
 2 files changed, 35 insertions(+), 24 deletions(-)

diff --git a/migration/savevm.h b/migration/savevm.h
index 125a2507b7..5d815af742 100644
--- a/migration/savevm.h
+++ b/migration/savevm.h
@@ -37,6 +37,7 @@ int qemu_savevm_state_prepare(Error **errp);
 int qemu_savevm_state_setup(QEMUFile *f, Error **errp);
 bool qemu_savevm_state_guest_unplug_pending(void);
 int qemu_savevm_state_resume_prepare(MigrationState *s);
+void qemu_savevm_send_header(QEMUFile *f);
 void qemu_savevm_state_header(QEMUFile *f);
 int qemu_savevm_state_iterate(QEMUFile *f, bool postcopy);
 void qemu_savevm_state_cleanup(void);
diff --git a/migration/savevm.c b/migration/savevm.c
index 3dc812a7bb..e26656cca3 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1282,38 +1282,48 @@ void qemu_savevm_non_migratable_list(strList **reasons)
     }
 }
 
-void qemu_savevm_state_header(QEMUFile *f)
+void qemu_savevm_send_header(QEMUFile *f)
 {
-    MigrationState *s = migrate_get_current();
-    JSONWriter *vmdesc = s->vmdesc;
-    Error *local_err = NULL;
-
     trace_savevm_state_header();
     qemu_put_be32(f, QEMU_VM_FILE_MAGIC);
     qemu_put_be32(f, QEMU_VM_FILE_VERSION);
+}
 
-    if (s->send_configuration) {
-        qemu_put_byte(f, QEMU_VM_CONFIGURATION);
+static void qemu_savevm_send_configuration(MigrationState *s, QEMUFile *f)
+{
+    JSONWriter *vmdesc = s->vmdesc;
+    Error *local_err = NULL;
 
-        if (vmdesc) {
-            /*
-             * This starts the main json object and is paired with the
-             * json_writer_end_object in
-             * qemu_savevm_state_complete_precopy_non_iterable
-             */
-            json_writer_start_object(vmdesc, NULL);
-            json_writer_start_object(vmdesc, "configuration");
-        }
+    qemu_put_byte(f, QEMU_VM_CONFIGURATION);
 
-        vmstate_save_state(f, &vmstate_configuration, &savevm_state,
-                           vmdesc, &local_err);
-        if (local_err) {
-            error_report_err(local_err);
-        }
+    if (vmdesc) {
+        /*
+         * This starts the main json object and is paired with the
+         * json_writer_end_object in
+         * qemu_savevm_state_complete_precopy_non_iterable
+         */
+        json_writer_start_object(vmdesc, NULL);
+        json_writer_start_object(vmdesc, "configuration");
+    }
 
-        if (vmdesc) {
-            json_writer_end_object(vmdesc);
-        }
+    vmstate_save_state(f, &vmstate_configuration, &savevm_state,
+                       vmdesc, &local_err);
+    if (local_err) {
+        error_report_err(local_err);
+    }
+
+    if (vmdesc) {
+        json_writer_end_object(vmdesc);
+    }
+}
+
+void qemu_savevm_state_header(QEMUFile *f)
+{
+    MigrationState *s = migrate_get_current();
+
+    qemu_savevm_send_header(f);
+    if (s->send_configuration) {
+        qemu_savevm_send_configuration(s, f);
     }
 }
 
-- 
2.50.1

[PATCH v2 02/24] migration: Use qemu_savevm_send_header() in qemu_save_device_state()

[Peter Xu]

Reduces duplication of the other path where we also send the same header.

Reviewed-by: Fabiano Rosas <farosas@suse.de>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/savevm.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index e26656cca3..64bf445c98 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1872,8 +1872,7 @@ int qemu_save_device_state(QEMUFile *f)
     SaveStateEntry *se;
 
     if (!migration_in_colo_state()) {
-        qemu_put_be32(f, QEMU_VM_FILE_MAGIC);
-        qemu_put_be32(f, QEMU_VM_FILE_VERSION);
+        qemu_savevm_send_header(f);
     }
     cpu_synchronize_all_states();
 
-- 
2.50.1

[PATCH v2 03/24] migration: Remove one migration_in_colo_state() occurance

[Peter Xu]

Move the send header operation directly into Xen's QMP command, as COLO
doesn't need it.

Reviewed-by: Fabiano Rosas <farosas@suse.de>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/savevm.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index 64bf445c98..61e873d90c 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1871,9 +1871,6 @@ int qemu_save_device_state(QEMUFile *f)
     Error *local_err = NULL;
     SaveStateEntry *se;
 
-    if (!migration_in_colo_state()) {
-        qemu_savevm_send_header(f);
-    }
     cpu_synchronize_all_states();
 
     QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
@@ -3335,6 +3332,7 @@ void qmp_xen_save_devices_state(const char *filename, bool has_live, bool live,
     qio_channel_set_name(QIO_CHANNEL(ioc), "migration-xen-save-state");
     f = qemu_file_new_output(QIO_CHANNEL(ioc));
     object_unref(OBJECT(ioc));
+    qemu_savevm_send_header(f);
     ret = qemu_save_device_state(f);
     if (ret < 0 || qemu_fclose(f) < 0) {
         error_setg(errp, "saving Xen device state failed");
-- 
2.50.1

[PATCH v2 04/24] migration/savevm: Remove SaveStateEntry.is_ram

[Peter Xu]

It's neither accurate nor necessary.  Use a proper helper to detect if it's
an iterable savevm state entry instead.

Reviewed-by: Fabiano Rosas <farosas@suse.de>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/savevm.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index 61e873d90c..f1cd8c913d 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -249,7 +249,6 @@ typedef struct SaveStateEntry {
     const VMStateDescription *vmsd;
     void *opaque;
     CompatEntry *compat;
-    int is_ram;
 } SaveStateEntry;
 
 typedef struct SaveState {
@@ -816,10 +815,6 @@ int register_savevm_live(const char *idstr,
     se->ops = ops;
     se->opaque = opaque;
     se->vmsd = NULL;
-    /* if this is a live_savem then set is_ram */
-    if (ops->save_setup != NULL) {
-        se->is_ram = 1;
-    }
 
     pstrcat(se->idstr, sizeof(se->idstr), idstr);
 
@@ -1866,6 +1861,12 @@ void qemu_savevm_live_state(QEMUFile *f)
     qemu_put_byte(f, QEMU_VM_EOF);
 }
 
+/* Is a save state entry iterable (e.g. RAM)? */
+static bool qemu_savevm_se_iterable(SaveStateEntry *se)
+{
+    return se->ops && se->ops->save_setup;
+}
+
 int qemu_save_device_state(QEMUFile *f)
 {
     Error *local_err = NULL;
@@ -1876,7 +1877,7 @@ int qemu_save_device_state(QEMUFile *f)
     QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
         int ret;
 
-        if (se->is_ram) {
+        if (qemu_savevm_se_iterable(se)) {
             continue;
         }
         ret = vmstate_save(f, se, NULL, &local_err);
@@ -2648,7 +2649,7 @@ qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type, Error **errp)
     se->load_section_id = section_id;
 
     /* Validate if it is a device's state */
-    if (xen_enabled() && se->is_ram) {
+    if (xen_enabled() && qemu_savevm_se_iterable(se)) {
         error_setg(errp, "loadvm: %s RAM loading not allowed on Xen", idstr);
         return -EINVAL;
     }
-- 
2.50.1

[PATCH v2 05/24] migration/colo: Unwrap qemu_savevm_live_state()

[Peter Xu]

It's only used in COLO path and only contains two calls.  Unwrap the
function.  It paves way for further reduce special COLO paths on sync.

Reviewed-by: Fabiano Rosas <farosas@suse.de>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/savevm.h | 1 -
 migration/colo.c   | 3 ++-
 migration/savevm.c | 7 -------
 3 files changed, 2 insertions(+), 9 deletions(-)

diff --git a/migration/savevm.h b/migration/savevm.h
index 5d815af742..528607f09e 100644
--- a/migration/savevm.h
+++ b/migration/savevm.h
@@ -64,7 +64,6 @@ void qemu_savevm_send_postcopy_ram_discard(QEMUFile *f, const char *name,
                                            uint64_t *start_list,
                                            uint64_t *length_list);
 void qemu_savevm_send_colo_enable(QEMUFile *f);
-void qemu_savevm_live_state(QEMUFile *f);
 int qemu_save_device_state(QEMUFile *f);
 
 int qemu_loadvm_state(QEMUFile *f, Error **errp);
diff --git a/migration/colo.c b/migration/colo.c
index db783f6fa7..e05736ecf0 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -471,7 +471,8 @@ static int colo_do_checkpoint_transaction(MigrationState *s,
      * TODO: We may need a timeout mechanism to prevent COLO process
      * to be blocked here.
      */
-    qemu_savevm_live_state(s->to_dst_file);
+    qemu_savevm_state_complete_precopy(s->to_dst_file, true);
+    qemu_put_byte(s->to_dst_file, QEMU_VM_EOF);
 
     qemu_fflush(fb);
 
diff --git a/migration/savevm.c b/migration/savevm.c
index f1cd8c913d..529cf310e0 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1854,13 +1854,6 @@ cleanup:
     return ret;
 }
 
-void qemu_savevm_live_state(QEMUFile *f)
-{
-    /* save QEMU_VM_SECTION_END section */
-    qemu_savevm_state_complete_precopy(f, true);
-    qemu_put_byte(f, QEMU_VM_EOF);
-}
-
 /* Is a save state entry iterable (e.g. RAM)? */
 static bool qemu_savevm_se_iterable(SaveStateEntry *se)
 {
-- 
2.50.1

[PATCH v2 06/24] migration: Remove call to send switchover start event in colo/savevm

[Peter Xu]

COLO (in case of periodically checkpointing) already have switchover
happened before hand.  This switchover_start feature never applies to COLO.

Savevm for snapshot doesn't have switchover phase and VM is stopped for the
whole process.

Remove both.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/colo.c   | 2 --
 migration/savevm.c | 1 -
 2 files changed, 3 deletions(-)

diff --git a/migration/colo.c b/migration/colo.c
index e05736ecf0..c344943173 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -453,8 +453,6 @@ static int colo_do_checkpoint_transaction(MigrationState *s,
         goto out;
     }
 
-    qemu_savevm_maybe_send_switchover_start(s->to_dst_file);
-
     /* Note: device state is saved into buffer */
     ret = qemu_save_device_state(fb);
 
diff --git a/migration/savevm.c b/migration/savevm.c
index 529cf310e0..d41be3a4a2 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1830,7 +1830,6 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp)
 
     ret = qemu_file_get_error(f);
     if (ret == 0) {
-        qemu_savevm_maybe_send_switchover_start(f);
         qemu_savevm_state_complete_precopy(f, false);
         ret = qemu_file_get_error(f);
     }
-- 
2.50.1

Re: [PATCH v2 06/24] migration: Remove call to send switchover start event in colo/savevm

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> COLO (in case of periodically checkpointing) already have switchover
> happened before hand.  This switchover_start feature never applies to COLO.
>
> Savevm for snapshot doesn't have switchover phase and VM is stopped for the
> whole process.
>
> Remove both.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

[PATCH v2 07/24] colo: Forbid VM resume during checkpointing

[Peter Xu]

COLO will stop the VM during each checkpoint on either PVM or SVM.

Accidentally resuming the VM during the window might be fatal because it
may cause the RAM and devices state to misalign, corrupting the checkpoint.

Hence forbid VM resume during the process.

Reviewed-by: Fabiano Rosas <farosas@suse.de>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 monitor/qmp-cmds.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index 1ca44fbd72..0c409c27dc 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -84,6 +84,9 @@ void qmp_cont(Error **errp)
     } else if (runstate_check(RUN_STATE_FINISH_MIGRATE)) {
         error_setg(errp, "Migration is not finalized yet");
         return;
+    } else if (runstate_check(RUN_STATE_COLO)) {
+        error_setg(errp, "COLO checkpoint in progress");
+        return;
     }
 
     for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
-- 
2.50.1

[PATCH v2 08/24] migration/colo: Use the RAM iterable helper directly

[Peter Xu]

qemu_savevm_state_complete_precopy() has a weird parameter called
"iterable_only".  It's needed because COLO saves device states in advance.

To make dropping that weird parameter easier, let COLO directly use the RAM
iterator helper instead, which should make the code easier to read too.

Reviewed-by: Fabiano Rosas <farosas@suse.de>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/colo.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/migration/colo.c b/migration/colo.c
index c344943173..f92803dd29 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -469,7 +469,7 @@ static int colo_do_checkpoint_transaction(MigrationState *s,
      * TODO: We may need a timeout mechanism to prevent COLO process
      * to be blocked here.
      */
-    qemu_savevm_state_complete_precopy(s->to_dst_file, true);
+    qemu_savevm_state_complete_precopy_iterable(s->to_dst_file, false);
     qemu_put_byte(s->to_dst_file, QEMU_VM_EOF);
 
     qemu_fflush(fb);
-- 
2.50.1

[PATCH v2 09/24] migration/colo: Document qemu_fflush(fb)

[Peter Xu]

COLO caches all device states in a buffer channel `fb'.  Add some comments
explaining the flush, that (1) it's the `fb' not the main channel, (2) on
what it updates.

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

diff --git a/migration/colo.c b/migration/colo.c
index f92803dd29..1b94e0f0ee 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -472,12 +472,14 @@ static int colo_do_checkpoint_transaction(MigrationState *s,
     qemu_savevm_state_complete_precopy_iterable(s->to_dst_file, false);
     qemu_put_byte(s->to_dst_file, QEMU_VM_EOF);
 
-    qemu_fflush(fb);
-
     /*
      * We need the size of the VMstate data in Secondary side,
      * With which we can decide how much data should be read.
+     *
+     * Flush the qemufile cache to make sure both bioc->usage and
+     * bioc->data contains the latest info.
      */
+    qemu_fflush(fb);
     colo_send_message_value(s->to_dst_file, COLO_MESSAGE_VMSTATE_SIZE,
                             bioc->usage, &local_err);
     if (local_err) {
-- 
2.50.1

Re: [PATCH v2 09/24] migration/colo: Document qemu_fflush(fb)

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> COLO caches all device states in a buffer channel `fb'.  Add some comments
> explaining the flush, that (1) it's the `fb' not the main channel, (2) on
> what it updates.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

[PATCH v2 10/24] migration: Drop iterable_only in qemu_savevm_state_complete_precopy

[Peter Xu]

Now after removing the special case in COLO, we can drop this parameter.

Reviewed-by: Fabiano Rosas <farosas@suse.de>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/savevm.h    |  2 +-
 migration/migration.c |  2 +-
 migration/savevm.c    | 12 +++++-------
 3 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/migration/savevm.h b/migration/savevm.h
index 528607f09e..ea01ca63ec 100644
--- a/migration/savevm.h
+++ b/migration/savevm.h
@@ -42,7 +42,7 @@ void qemu_savevm_state_header(QEMUFile *f);
 int qemu_savevm_state_iterate(QEMUFile *f, bool postcopy);
 void qemu_savevm_state_cleanup(void);
 void qemu_savevm_state_complete_postcopy(QEMUFile *f);
-int qemu_savevm_state_complete_precopy(QEMUFile *f, bool iterable_only);
+int qemu_savevm_state_complete_precopy(QEMUFile *f);
 void qemu_savevm_state_pending_exact(uint64_t *must_precopy,
                                      uint64_t *can_postcopy);
 void qemu_savevm_state_pending_estimate(uint64_t *must_precopy,
diff --git a/migration/migration.c b/migration/migration.c
index b103a82fc0..e3f1cc7b2e 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2752,7 +2752,7 @@ static int migration_completion_precopy(MigrationState *s)
         goto out_unlock;
     }
 
-    ret = qemu_savevm_state_complete_precopy(s->to_dst_file, false);
+    ret = qemu_savevm_state_complete_precopy(s->to_dst_file);
 out_unlock:
     bql_unlock();
     return ret;
diff --git a/migration/savevm.c b/migration/savevm.c
index d41be3a4a2..da9a60c73f 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1717,7 +1717,7 @@ int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f,
     return 0;
 }
 
-int qemu_savevm_state_complete_precopy(QEMUFile *f, bool iterable_only)
+int qemu_savevm_state_complete_precopy(QEMUFile *f)
 {
     int ret;
 
@@ -1726,11 +1726,9 @@ int qemu_savevm_state_complete_precopy(QEMUFile *f, bool iterable_only)
         return ret;
     }
 
-    if (!iterable_only) {
-        ret = qemu_savevm_state_complete_precopy_non_iterable(f, false);
-        if (ret) {
-            return ret;
-        }
+    ret = qemu_savevm_state_complete_precopy_non_iterable(f, false);
+    if (ret) {
+        return ret;
     }
 
     return qemu_fflush(f);
@@ -1830,7 +1828,7 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp)
 
     ret = qemu_file_get_error(f);
     if (ret == 0) {
-        qemu_savevm_state_complete_precopy(f, false);
+        qemu_savevm_state_complete_precopy(f);
         ret = qemu_file_get_error(f);
     }
     if (ret != 0) {
-- 
2.50.1

[PATCH v2 11/24] migration: Drop qemu_file_set_error() when save non-iterable fails

[Peter Xu]

All users of qemu_savevm_state_complete_precopy_non_iterable() process
return values.  There's no need to set error on qemufile (which we likely
should remove gradually across the tree).  Remove it for possible code
dedup to happen later.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/savevm.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index da9a60c73f..9d2109718a 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1688,7 +1688,6 @@ int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f,
         if (ret) {
             migrate_error_propagate(ms, error_copy(local_err));
             error_report_err(local_err);
-            qemu_file_set_error(f, ret);
             return ret;
         }
 
-- 
2.50.1

Re: [PATCH v2 11/24] migration: Drop qemu_file_set_error() when save non-iterable fails

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> All users of qemu_savevm_state_complete_precopy_non_iterable() process
> return values.  There's no need to set error on qemufile (which we likely
> should remove gradually across the tree).  Remove it for possible code
> dedup to happen later.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/savevm.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/migration/savevm.c b/migration/savevm.c
> index da9a60c73f..9d2109718a 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1688,7 +1688,6 @@ int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f,
>          if (ret) {
>              migrate_error_propagate(ms, error_copy(local_err));
>              error_report_err(local_err);
> -            qemu_file_set_error(f, ret);
>              return ret;
>          }

Reviewed-by: Fabiano Rosas <farosas@suse.de>

[PATCH v2 12/24] migration/colo: Send device states without copying buffer

[Peter Xu]

We can safely use the async version of put buffer here because the qemufile
will be flushed right away.

Suggested-by: Fabiano Rosas <farosas@suse.de>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/colo.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/migration/colo.c b/migration/colo.c
index 1b94e0f0ee..0b1a58cd8f 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -486,7 +486,8 @@ static int colo_do_checkpoint_transaction(MigrationState *s,
         goto out;
     }
 
-    qemu_put_buffer(s->to_dst_file, bioc->data, bioc->usage);
+    /* We can use async put because flush happens right away */
+    qemu_put_buffer_async(s->to_dst_file, bioc->data, bioc->usage, false);
     ret = qemu_fflush(s->to_dst_file);
     if (ret < 0) {
         goto out;
-- 
2.50.1

Re: [PATCH v2 12/24] migration/colo: Send device states without copying buffer

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> We can safely use the async version of put buffer here because the qemufile
> will be flushed right away.
>
> Suggested-by: Fabiano Rosas <farosas@suse.de>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/colo.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/migration/colo.c b/migration/colo.c
> index 1b94e0f0ee..0b1a58cd8f 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -486,7 +486,8 @@ static int colo_do_checkpoint_transaction(MigrationState *s,
>          goto out;
>      }
>  
> -    qemu_put_buffer(s->to_dst_file, bioc->data, bioc->usage);
> +    /* We can use async put because flush happens right away */
> +    qemu_put_buffer_async(s->to_dst_file, bioc->data, bioc->usage, false);
>      ret = qemu_fflush(s->to_dst_file);
>      if (ret < 0) {
>          goto out;

Reviewed-by: Fabiano Rosas <farosas@suse.de>

[PATCH v2 13/24] migration/postcopy: Send device states without copying buffer

[Peter Xu]

Put buffer can be async as long as the flush happens before the buffer will
be recycled / reused.  Do it for postcopy package data.  Quick measurement
shows a small VM the time to push / flush the package shrinks from 91us to
38us.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/savevm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index 9d2109718a..d41e89228d 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1136,7 +1136,8 @@ int qemu_savevm_send_packaged(QEMUFile *f, const uint8_t *buf, size_t len)
     trace_qemu_savevm_send_packaged();
     qemu_savevm_command_send(f, MIG_CMD_PACKAGED, 4, (uint8_t *)&tmp);
 
-    qemu_put_buffer(f, buf, len);
+    /* We can use async put because the qemufile will be flushed right away */
+    qemu_put_buffer_async(f, buf, len, false);
     qemu_fflush(f);
 
     return 0;
-- 
2.50.1

Re: [PATCH v2 13/24] migration/postcopy: Send device states without copying buffer

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> Put buffer can be async as long as the flush happens before the buffer will
> be recycled / reused.  Do it for postcopy package data.  Quick measurement
> shows a small VM the time to push / flush the package shrinks from 91us to
> 38us.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/savevm.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 9d2109718a..d41e89228d 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1136,7 +1136,8 @@ int qemu_savevm_send_packaged(QEMUFile *f, const uint8_t *buf, size_t len)
>      trace_qemu_savevm_send_packaged();
>      qemu_savevm_command_send(f, MIG_CMD_PACKAGED, 4, (uint8_t *)&tmp);
>  
> -    qemu_put_buffer(f, buf, len);
> +    /* We can use async put because the qemufile will be flushed right away */
> +    qemu_put_buffer_async(f, buf, len, false);
>      qemu_fflush(f);
>  
>      return 0;

Reviewed-by: Fabiano Rosas <farosas@suse.de>

[PATCH v2 14/24] migration: Introduce qemu_savevm_state_end()

[Peter Xu]

Introduce a helper to end a migration stream.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/savevm.h |  1 +
 migration/colo.c   |  2 +-
 migration/savevm.c | 12 +++++++++---
 3 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/migration/savevm.h b/migration/savevm.h
index ea01ca63ec..d0596d1d62 100644
--- a/migration/savevm.h
+++ b/migration/savevm.h
@@ -49,6 +49,7 @@ void qemu_savevm_state_pending_estimate(uint64_t *must_precopy,
                                         uint64_t *can_postcopy);
 int qemu_savevm_state_complete_precopy_iterable(QEMUFile *f, bool in_postcopy);
 bool qemu_savevm_state_postcopy_prepare(QEMUFile *f, Error **errp);
+void qemu_savevm_state_end(QEMUFile *f);
 void qemu_savevm_send_ping(QEMUFile *f, uint32_t value);
 void qemu_savevm_send_open_return_path(QEMUFile *f);
 int qemu_savevm_send_packaged(QEMUFile *f, const uint8_t *buf, size_t len);
diff --git a/migration/colo.c b/migration/colo.c
index 0b1a58cd8f..db804b25a9 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -470,7 +470,7 @@ static int colo_do_checkpoint_transaction(MigrationState *s,
      * to be blocked here.
      */
     qemu_savevm_state_complete_precopy_iterable(s->to_dst_file, false);
-    qemu_put_byte(s->to_dst_file, QEMU_VM_EOF);
+    qemu_savevm_state_end(s->to_dst_file);
 
     /*
      * We need the size of the VMstate data in Secondary side,
diff --git a/migration/savevm.c b/migration/savevm.c
index d41e89228d..a787691352 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1065,6 +1065,12 @@ static int vmstate_save(QEMUFile *f, SaveStateEntry *se, JSONWriter *vmdesc,
     }
     return 0;
 }
+
+void qemu_savevm_state_end(QEMUFile *f)
+{
+    qemu_put_byte(f, QEMU_VM_EOF);
+}
+
 /**
  * qemu_savevm_command_send: Send a 'QEMU_VM_COMMAND' type element with the
  *                           command and associated data.
@@ -1555,7 +1561,7 @@ void qemu_savevm_state_complete_postcopy(QEMUFile *f)
         }
     }
 
-    qemu_put_byte(f, QEMU_VM_EOF);
+    qemu_savevm_state_end(f);
     qemu_fflush(f);
 }
 
@@ -1699,7 +1705,7 @@ int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f,
 
     if (!in_postcopy) {
         /* Postcopy stream will still be going */
-        qemu_put_byte(f, QEMU_VM_EOF);
+        qemu_savevm_state_end(f);
 
         if (vmdesc) {
             json_writer_end_array(vmdesc);
@@ -1879,7 +1885,7 @@ int qemu_save_device_state(QEMUFile *f)
         }
     }
 
-    qemu_put_byte(f, QEMU_VM_EOF);
+    qemu_savevm_state_end(f);
 
     return qemu_file_get_error(f);
 }
-- 
2.50.1

Re: [PATCH v2 14/24] migration: Introduce qemu_savevm_state_end()

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> Introduce a helper to end a migration stream.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/savevm.h |  1 +
>  migration/colo.c   |  2 +-
>  migration/savevm.c | 12 +++++++++---
>  3 files changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/migration/savevm.h b/migration/savevm.h
> index ea01ca63ec..d0596d1d62 100644
> --- a/migration/savevm.h
> +++ b/migration/savevm.h
> @@ -49,6 +49,7 @@ void qemu_savevm_state_pending_estimate(uint64_t *must_precopy,
>                                          uint64_t *can_postcopy);
>  int qemu_savevm_state_complete_precopy_iterable(QEMUFile *f, bool in_postcopy);
>  bool qemu_savevm_state_postcopy_prepare(QEMUFile *f, Error **errp);
> +void qemu_savevm_state_end(QEMUFile *f);
>  void qemu_savevm_send_ping(QEMUFile *f, uint32_t value);
>  void qemu_savevm_send_open_return_path(QEMUFile *f);
>  int qemu_savevm_send_packaged(QEMUFile *f, const uint8_t *buf, size_t len);
> diff --git a/migration/colo.c b/migration/colo.c
> index 0b1a58cd8f..db804b25a9 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -470,7 +470,7 @@ static int colo_do_checkpoint_transaction(MigrationState *s,
>       * to be blocked here.
>       */
>      qemu_savevm_state_complete_precopy_iterable(s->to_dst_file, false);
> -    qemu_put_byte(s->to_dst_file, QEMU_VM_EOF);
> +    qemu_savevm_state_end(s->to_dst_file);
>  
>      /*
>       * We need the size of the VMstate data in Secondary side,
> diff --git a/migration/savevm.c b/migration/savevm.c
> index d41e89228d..a787691352 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1065,6 +1065,12 @@ static int vmstate_save(QEMUFile *f, SaveStateEntry *se, JSONWriter *vmdesc,
>      }
>      return 0;
>  }
> +
> +void qemu_savevm_state_end(QEMUFile *f)
> +{
> +    qemu_put_byte(f, QEMU_VM_EOF);
> +}
> +
>  /**
>   * qemu_savevm_command_send: Send a 'QEMU_VM_COMMAND' type element with the
>   *                           command and associated data.
> @@ -1555,7 +1561,7 @@ void qemu_savevm_state_complete_postcopy(QEMUFile *f)
>          }
>      }
>  
> -    qemu_put_byte(f, QEMU_VM_EOF);
> +    qemu_savevm_state_end(f);
>      qemu_fflush(f);
>  }
>  
> @@ -1699,7 +1705,7 @@ int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f,
>  
>      if (!in_postcopy) {
>          /* Postcopy stream will still be going */
> -        qemu_put_byte(f, QEMU_VM_EOF);
> +        qemu_savevm_state_end(f);
>  
>          if (vmdesc) {
>              json_writer_end_array(vmdesc);
> @@ -1879,7 +1885,7 @@ int qemu_save_device_state(QEMUFile *f)
>          }
>      }
>  
> -    qemu_put_byte(f, QEMU_VM_EOF);
> +    qemu_savevm_state_end(f);
>  
>      return qemu_file_get_error(f);
>  }

Reviewed-by: Fabiano Rosas <farosas@suse.de>

[PATCH v2 15/24] migration: Provide helper for save vm description

[Peter Xu]

Provide two smaller helpers to dump the vm desc.  Preparing to move it out
and generalize device state dump.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/savevm.h |  1 +
 migration/savevm.c | 35 +++++++++++++++++++++++------------
 2 files changed, 24 insertions(+), 12 deletions(-)

diff --git a/migration/savevm.h b/migration/savevm.h
index d0596d1d62..f957f851ef 100644
--- a/migration/savevm.h
+++ b/migration/savevm.h
@@ -50,6 +50,7 @@ void qemu_savevm_state_pending_estimate(uint64_t *must_precopy,
 int qemu_savevm_state_complete_precopy_iterable(QEMUFile *f, bool in_postcopy);
 bool qemu_savevm_state_postcopy_prepare(QEMUFile *f, Error **errp);
 void qemu_savevm_state_end(QEMUFile *f);
+void qemu_savevm_state_end_precopy(MigrationState *s, QEMUFile *f);
 void qemu_savevm_send_ping(QEMUFile *f, uint32_t value);
 void qemu_savevm_send_open_return_path(QEMUFile *f);
 int qemu_savevm_send_packaged(QEMUFile *f, const uint8_t *buf, size_t len);
diff --git a/migration/savevm.c b/migration/savevm.c
index a787691352..41560b97a4 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1669,13 +1669,34 @@ ret_fail_abort_threads:
     return -1;
 }
 
+static void qemu_savevm_state_vm_desc(MigrationState *s, QEMUFile *f)
+{
+    JSONWriter *vmdesc = s->vmdesc;
+    int vmdesc_len;
+
+    if (vmdesc) {
+        json_writer_end_array(vmdesc);
+        json_writer_end_object(vmdesc);
+        vmdesc_len = strlen(json_writer_get(vmdesc));
+
+        qemu_put_byte(f, QEMU_VM_VMDESCRIPTION);
+        qemu_put_be32(f, vmdesc_len);
+        qemu_put_buffer(f, (uint8_t *)json_writer_get(vmdesc), vmdesc_len);
+    }
+}
+
+void qemu_savevm_state_end_precopy(MigrationState *s, QEMUFile *f)
+{
+    qemu_savevm_state_end(f);
+    qemu_savevm_state_vm_desc(s, f);
+}
+
 int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f,
                                                     bool in_postcopy)
 {
     MigrationState *ms = migrate_get_current();
     int64_t start_ts_each, end_ts_each;
     JSONWriter *vmdesc = ms->vmdesc;
-    int vmdesc_len;
     SaveStateEntry *se;
     Error *local_err = NULL;
     int ret;
@@ -1705,17 +1726,7 @@ int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f,
 
     if (!in_postcopy) {
         /* Postcopy stream will still be going */
-        qemu_savevm_state_end(f);
-
-        if (vmdesc) {
-            json_writer_end_array(vmdesc);
-            json_writer_end_object(vmdesc);
-            vmdesc_len = strlen(json_writer_get(vmdesc));
-
-            qemu_put_byte(f, QEMU_VM_VMDESCRIPTION);
-            qemu_put_be32(f, vmdesc_len);
-            qemu_put_buffer(f, (uint8_t *)json_writer_get(vmdesc), vmdesc_len);
-        }
+        qemu_savevm_state_end_precopy(ms, f);
     }
 
     trace_vmstate_downtime_checkpoint("src-non-iterable-saved");
-- 
2.50.1

Re: [PATCH v2 15/24] migration: Provide helper for save vm description

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> Provide two smaller helpers to dump the vm desc.  Preparing to move it out
> and generalize device state dump.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/savevm.h |  1 +
>  migration/savevm.c | 35 +++++++++++++++++++++++------------
>  2 files changed, 24 insertions(+), 12 deletions(-)
>
> diff --git a/migration/savevm.h b/migration/savevm.h
> index d0596d1d62..f957f851ef 100644
> --- a/migration/savevm.h
> +++ b/migration/savevm.h
> @@ -50,6 +50,7 @@ void qemu_savevm_state_pending_estimate(uint64_t *must_precopy,
>  int qemu_savevm_state_complete_precopy_iterable(QEMUFile *f, bool in_postcopy);
>  bool qemu_savevm_state_postcopy_prepare(QEMUFile *f, Error **errp);
>  void qemu_savevm_state_end(QEMUFile *f);
> +void qemu_savevm_state_end_precopy(MigrationState *s, QEMUFile *f);
>  void qemu_savevm_send_ping(QEMUFile *f, uint32_t value);
>  void qemu_savevm_send_open_return_path(QEMUFile *f);
>  int qemu_savevm_send_packaged(QEMUFile *f, const uint8_t *buf, size_t len);
> diff --git a/migration/savevm.c b/migration/savevm.c
> index a787691352..41560b97a4 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1669,13 +1669,34 @@ ret_fail_abort_threads:
>      return -1;
>  }
>  
> +static void qemu_savevm_state_vm_desc(MigrationState *s, QEMUFile *f)
> +{
> +    JSONWriter *vmdesc = s->vmdesc;
> +    int vmdesc_len;
> +
> +    if (vmdesc) {
> +        json_writer_end_array(vmdesc);
> +        json_writer_end_object(vmdesc);
> +        vmdesc_len = strlen(json_writer_get(vmdesc));
> +
> +        qemu_put_byte(f, QEMU_VM_VMDESCRIPTION);
> +        qemu_put_be32(f, vmdesc_len);
> +        qemu_put_buffer(f, (uint8_t *)json_writer_get(vmdesc), vmdesc_len);
> +    }
> +}
> +
> +void qemu_savevm_state_end_precopy(MigrationState *s, QEMUFile *f)
> +{
> +    qemu_savevm_state_end(f);
> +    qemu_savevm_state_vm_desc(s, f);
> +}
> +
>  int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f,
>                                                      bool in_postcopy)
>  {
>      MigrationState *ms = migrate_get_current();
>      int64_t start_ts_each, end_ts_each;
>      JSONWriter *vmdesc = ms->vmdesc;
> -    int vmdesc_len;
>      SaveStateEntry *se;
>      Error *local_err = NULL;
>      int ret;
> @@ -1705,17 +1726,7 @@ int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f,
>  
>      if (!in_postcopy) {
>          /* Postcopy stream will still be going */
> -        qemu_savevm_state_end(f);
> -
> -        if (vmdesc) {
> -            json_writer_end_array(vmdesc);
> -            json_writer_end_object(vmdesc);
> -            vmdesc_len = strlen(json_writer_get(vmdesc));
> -
> -            qemu_put_byte(f, QEMU_VM_VMDESCRIPTION);
> -            qemu_put_be32(f, vmdesc_len);
> -            qemu_put_buffer(f, (uint8_t *)json_writer_get(vmdesc), vmdesc_len);
> -        }
> +        qemu_savevm_state_end_precopy(ms, f);
>      }
>  
>      trace_vmstate_downtime_checkpoint("src-non-iterable-saved");

Reviewed-by: Fabiano Rosas <farosas@suse.de>

[PATCH v2 16/24] migration: Split qemu_savevm_state_complete_precopy_non_iterable()

[Peter Xu]

Split the function, making itself to be the helper to dump all non-iterable
device states (early_vmsd excluded).  Move the precopy end logic out to the
two callers that need it.

With it, we can remove the in_postcopy parameter.  Meanwhile, renaming the
function to be qemu_savevm_state_non_iterable(): we don't need the keyword
"complete" because non-iterable doesn't iterate anyway, and we don't need
precopy because we moved precopy specialties out.

NOTE: this patch introduced one new migrate_get_current() user; will be
removed in follow up patch.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/savevm.h    |  3 +--
 migration/migration.c |  7 +++++--
 migration/savevm.c    | 12 ++++--------
 3 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/migration/savevm.h b/migration/savevm.h
index f957f851ef..57b96133d5 100644
--- a/migration/savevm.h
+++ b/migration/savevm.h
@@ -74,8 +74,7 @@ int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis,
                            Error **errp);
 int qemu_load_device_state(QEMUFile *f, Error **errp);
 int qemu_loadvm_approve_switchover(void);
-int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f,
-        bool in_postcopy);
+int qemu_savevm_state_non_iterable(QEMUFile *f);
 
 bool qemu_loadvm_load_state_buffer(const char *idstr, uint32_t instance_id,
                                    char *buf, size_t len, Error **errp);
diff --git a/migration/migration.c b/migration/migration.c
index e3f1cc7b2e..c6e54d2a3f 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2545,7 +2545,7 @@ static int postcopy_start(MigrationState *ms, Error **errp)
      */
     qemu_savevm_send_postcopy_listen(fb);
 
-    ret = qemu_savevm_state_complete_precopy_non_iterable(fb, true);
+    ret = qemu_savevm_state_non_iterable(fb);
     if (ret) {
         error_setg(errp, "Postcopy save non-iterable device states failed");
         goto fail_closefb;
@@ -3678,9 +3678,12 @@ static void *bg_migration_thread(void *opaque)
         goto fail;
     }
 
-    if (qemu_savevm_state_complete_precopy_non_iterable(fb, false)) {
+    if (qemu_savevm_state_non_iterable(fb)) {
         goto fail;
     }
+
+    qemu_savevm_state_end_precopy(s, fb);
+
     /*
      * Since we are going to get non-iterable state data directly
      * from s->bioc->data, explicit flush is needed here.
diff --git a/migration/savevm.c b/migration/savevm.c
index 41560b97a4..e1918d4f38 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1691,8 +1691,7 @@ void qemu_savevm_state_end_precopy(MigrationState *s, QEMUFile *f)
     qemu_savevm_state_vm_desc(s, f);
 }
 
-int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f,
-                                                    bool in_postcopy)
+int qemu_savevm_state_non_iterable(QEMUFile *f)
 {
     MigrationState *ms = migrate_get_current();
     int64_t start_ts_each, end_ts_each;
@@ -1724,11 +1723,6 @@ int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f,
                                     end_ts_each - start_ts_each);
     }
 
-    if (!in_postcopy) {
-        /* Postcopy stream will still be going */
-        qemu_savevm_state_end_precopy(ms, f);
-    }
-
     trace_vmstate_downtime_checkpoint("src-non-iterable-saved");
 
     return 0;
@@ -1743,11 +1737,13 @@ int qemu_savevm_state_complete_precopy(QEMUFile *f)
         return ret;
     }
 
-    ret = qemu_savevm_state_complete_precopy_non_iterable(f, false);
+    ret = qemu_savevm_state_non_iterable(f);
     if (ret) {
         return ret;
     }
 
+    qemu_savevm_state_end_precopy(migrate_get_current(), f);
+
     return qemu_fflush(f);
 }
 
-- 
2.50.1

Re: [PATCH v2 16/24] migration: Split qemu_savevm_state_complete_precopy_non_iterable()

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> Split the function, making itself to be the helper to dump all non-iterable
> device states (early_vmsd excluded).  Move the precopy end logic out to the
> two callers that need it.
>
> With it, we can remove the in_postcopy parameter.  Meanwhile, renaming the
> function to be qemu_savevm_state_non_iterable(): we don't need the keyword
> "complete" because non-iterable doesn't iterate anyway, and we don't need
> precopy because we moved precopy specialties out.
>
> NOTE: this patch introduced one new migrate_get_current() user; will be
> removed in follow up patch.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/savevm.h    |  3 +--
>  migration/migration.c |  7 +++++--
>  migration/savevm.c    | 12 ++++--------
>  3 files changed, 10 insertions(+), 12 deletions(-)
>
> diff --git a/migration/savevm.h b/migration/savevm.h
> index f957f851ef..57b96133d5 100644
> --- a/migration/savevm.h
> +++ b/migration/savevm.h
> @@ -74,8 +74,7 @@ int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis,
>                             Error **errp);
>  int qemu_load_device_state(QEMUFile *f, Error **errp);
>  int qemu_loadvm_approve_switchover(void);
> -int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f,
> -        bool in_postcopy);
> +int qemu_savevm_state_non_iterable(QEMUFile *f);
>  
>  bool qemu_loadvm_load_state_buffer(const char *idstr, uint32_t instance_id,
>                                     char *buf, size_t len, Error **errp);
> diff --git a/migration/migration.c b/migration/migration.c
> index e3f1cc7b2e..c6e54d2a3f 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2545,7 +2545,7 @@ static int postcopy_start(MigrationState *ms, Error **errp)
>       */
>      qemu_savevm_send_postcopy_listen(fb);
>  
> -    ret = qemu_savevm_state_complete_precopy_non_iterable(fb, true);
> +    ret = qemu_savevm_state_non_iterable(fb);
>      if (ret) {
>          error_setg(errp, "Postcopy save non-iterable device states failed");
>          goto fail_closefb;
> @@ -3678,9 +3678,12 @@ static void *bg_migration_thread(void *opaque)
>          goto fail;
>      }
>  
> -    if (qemu_savevm_state_complete_precopy_non_iterable(fb, false)) {
> +    if (qemu_savevm_state_non_iterable(fb)) {
>          goto fail;
>      }
> +
> +    qemu_savevm_state_end_precopy(s, fb);
> +
>      /*
>       * Since we are going to get non-iterable state data directly
>       * from s->bioc->data, explicit flush is needed here.
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 41560b97a4..e1918d4f38 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1691,8 +1691,7 @@ void qemu_savevm_state_end_precopy(MigrationState *s, QEMUFile *f)
>      qemu_savevm_state_vm_desc(s, f);
>  }
>  
> -int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f,
> -                                                    bool in_postcopy)
> +int qemu_savevm_state_non_iterable(QEMUFile *f)
>  {
>      MigrationState *ms = migrate_get_current();
>      int64_t start_ts_each, end_ts_each;
> @@ -1724,11 +1723,6 @@ int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f,
>                                      end_ts_each - start_ts_each);
>      }
>  
> -    if (!in_postcopy) {
> -        /* Postcopy stream will still be going */
> -        qemu_savevm_state_end_precopy(ms, f);
> -    }
> -
>      trace_vmstate_downtime_checkpoint("src-non-iterable-saved");
>  
>      return 0;
> @@ -1743,11 +1737,13 @@ int qemu_savevm_state_complete_precopy(QEMUFile *f)
>          return ret;
>      }
>  
> -    ret = qemu_savevm_state_complete_precopy_non_iterable(f, false);
> +    ret = qemu_savevm_state_non_iterable(f);
>      if (ret) {
>          return ret;
>      }
>  
> +    qemu_savevm_state_end_precopy(migrate_get_current(), f);
> +
>      return qemu_fflush(f);
>  }

Reviewed-by: Fabiano Rosas <farosas@suse.de>

[PATCH v2 17/24] migration: qemu_savevm_state_complete_precopy() take MigrationState*

[Peter Xu]

Make it pass in MigrationState* instead of s->to_dst_file, so as to drop
the internal migrate_get_current().

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/savevm.h    | 2 +-
 migration/migration.c | 2 +-
 migration/savevm.c    | 7 ++++---
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/migration/savevm.h b/migration/savevm.h
index 57b96133d5..bded5e2a6c 100644
--- a/migration/savevm.h
+++ b/migration/savevm.h
@@ -42,7 +42,7 @@ void qemu_savevm_state_header(QEMUFile *f);
 int qemu_savevm_state_iterate(QEMUFile *f, bool postcopy);
 void qemu_savevm_state_cleanup(void);
 void qemu_savevm_state_complete_postcopy(QEMUFile *f);
-int qemu_savevm_state_complete_precopy(QEMUFile *f);
+int qemu_savevm_state_complete_precopy(MigrationState *s);
 void qemu_savevm_state_pending_exact(uint64_t *must_precopy,
                                      uint64_t *can_postcopy);
 void qemu_savevm_state_pending_estimate(uint64_t *must_precopy,
diff --git a/migration/migration.c b/migration/migration.c
index c6e54d2a3f..b8c85496f8 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2752,7 +2752,7 @@ static int migration_completion_precopy(MigrationState *s)
         goto out_unlock;
     }
 
-    ret = qemu_savevm_state_complete_precopy(s->to_dst_file);
+    ret = qemu_savevm_state_complete_precopy(s);
 out_unlock:
     bql_unlock();
     return ret;
diff --git a/migration/savevm.c b/migration/savevm.c
index e1918d4f38..830d8e5988 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1728,8 +1728,9 @@ int qemu_savevm_state_non_iterable(QEMUFile *f)
     return 0;
 }
 
-int qemu_savevm_state_complete_precopy(QEMUFile *f)
+int qemu_savevm_state_complete_precopy(MigrationState *s)
 {
+    QEMUFile *f = s->to_dst_file;
     int ret;
 
     ret = qemu_savevm_state_complete_precopy_iterable(f, false);
@@ -1742,7 +1743,7 @@ int qemu_savevm_state_complete_precopy(QEMUFile *f)
         return ret;
     }
 
-    qemu_savevm_state_end_precopy(migrate_get_current(), f);
+    qemu_savevm_state_end_precopy(s, f);
 
     return qemu_fflush(f);
 }
@@ -1841,7 +1842,7 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp)
 
     ret = qemu_file_get_error(f);
     if (ret == 0) {
-        qemu_savevm_state_complete_precopy(f);
+        qemu_savevm_state_complete_precopy(ms);
         ret = qemu_file_get_error(f);
     }
     if (ret != 0) {
-- 
2.50.1

Re: [PATCH v2 17/24] migration: qemu_savevm_state_complete_precopy() take MigrationState*

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> Make it pass in MigrationState* instead of s->to_dst_file, so as to drop
> the internal migrate_get_current().
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/savevm.h    | 2 +-
>  migration/migration.c | 2 +-
>  migration/savevm.c    | 7 ++++---
>  3 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/migration/savevm.h b/migration/savevm.h
> index 57b96133d5..bded5e2a6c 100644
> --- a/migration/savevm.h
> +++ b/migration/savevm.h
> @@ -42,7 +42,7 @@ void qemu_savevm_state_header(QEMUFile *f);
>  int qemu_savevm_state_iterate(QEMUFile *f, bool postcopy);
>  void qemu_savevm_state_cleanup(void);
>  void qemu_savevm_state_complete_postcopy(QEMUFile *f);
> -int qemu_savevm_state_complete_precopy(QEMUFile *f);
> +int qemu_savevm_state_complete_precopy(MigrationState *s);
>  void qemu_savevm_state_pending_exact(uint64_t *must_precopy,
>                                       uint64_t *can_postcopy);
>  void qemu_savevm_state_pending_estimate(uint64_t *must_precopy,
> diff --git a/migration/migration.c b/migration/migration.c
> index c6e54d2a3f..b8c85496f8 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2752,7 +2752,7 @@ static int migration_completion_precopy(MigrationState *s)
>          goto out_unlock;
>      }
>  
> -    ret = qemu_savevm_state_complete_precopy(s->to_dst_file);
> +    ret = qemu_savevm_state_complete_precopy(s);
>  out_unlock:
>      bql_unlock();
>      return ret;
> diff --git a/migration/savevm.c b/migration/savevm.c
> index e1918d4f38..830d8e5988 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1728,8 +1728,9 @@ int qemu_savevm_state_non_iterable(QEMUFile *f)
>      return 0;
>  }
>  
> -int qemu_savevm_state_complete_precopy(QEMUFile *f)
> +int qemu_savevm_state_complete_precopy(MigrationState *s)
>  {
> +    QEMUFile *f = s->to_dst_file;
>      int ret;
>  
>      ret = qemu_savevm_state_complete_precopy_iterable(f, false);
> @@ -1742,7 +1743,7 @@ int qemu_savevm_state_complete_precopy(QEMUFile *f)
>          return ret;
>      }
>  
> -    qemu_savevm_state_end_precopy(migrate_get_current(), f);
> +    qemu_savevm_state_end_precopy(s, f);
>  
>      return qemu_fflush(f);
>  }
> @@ -1841,7 +1842,7 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp)
>  
>      ret = qemu_file_get_error(f);
>      if (ret == 0) {
> -        qemu_savevm_state_complete_precopy(f);
> +        qemu_savevm_state_complete_precopy(ms);
>          ret = qemu_file_get_error(f);
>      }
>      if (ret != 0) {

Reviewed-by: Fabiano Rosas <farosas@suse.de>

[PATCH v2 18/24] migration: Cleanup error propagates in qemu_savevm_state_setup()

[Peter Xu]

We did two unnecessary error propagations in qemu_savevm_state_setup(), on
either propagate it to MigrationState*, or set qemufile with error.

Error propagation is not needed because:

  - Two live migration callers ([bg_]migration_thread) will propagate error
    if this function returned with an error.

  - Save snapshot (qemu_savevm_state) doesn't need to persist error; it got
    returned directly from save_snapshot().

QEMUFile set error is not needed because the callers always check for
errors explicitly.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/savevm.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index 830d8e5988..0683a103c8 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1385,8 +1385,6 @@ int qemu_savevm_state_setup(QEMUFile *f, Error **errp)
         if (se->vmsd && se->vmsd->early_setup) {
             ret = vmstate_save(f, se, vmdesc, errp);
             if (ret) {
-                migrate_error_propagate(ms, error_copy(*errp));
-                qemu_file_set_error(f, ret);
                 break;
             }
             continue;
@@ -1405,7 +1403,6 @@ int qemu_savevm_state_setup(QEMUFile *f, Error **errp)
         ret = se->ops->save_setup(f, se->opaque, errp);
         save_section_footer(f, se);
         if (ret < 0) {
-            qemu_file_set_error(f, ret);
             break;
         }
     }
-- 
2.50.1

Re: [PATCH v2 18/24] migration: Cleanup error propagates in qemu_savevm_state_setup()

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> We did two unnecessary error propagations in qemu_savevm_state_setup(), on
> either propagate it to MigrationState*, or set qemufile with error.
>
> Error propagation is not needed because:
>
>   - Two live migration callers ([bg_]migration_thread) will propagate error
>     if this function returned with an error.
>
>   - Save snapshot (qemu_savevm_state) doesn't need to persist error; it got
>     returned directly from save_snapshot().
>
> QEMUFile set error is not needed because the callers always check for
> errors explicitly.
>

Nice

Reviewed-by: Fabiano Rosas <farosas@suse.de>

> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/savevm.c | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 830d8e5988..0683a103c8 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1385,8 +1385,6 @@ int qemu_savevm_state_setup(QEMUFile *f, Error **errp)
>          if (se->vmsd && se->vmsd->early_setup) {
>              ret = vmstate_save(f, se, vmdesc, errp);
>              if (ret) {
> -                migrate_error_propagate(ms, error_copy(*errp));
> -                qemu_file_set_error(f, ret);
>                  break;
>              }
>              continue;
> @@ -1405,7 +1403,6 @@ int qemu_savevm_state_setup(QEMUFile *f, Error **errp)
>          ret = se->ops->save_setup(f, se->opaque, errp);
>          save_section_footer(f, se);
>          if (ret < 0) {
> -            qemu_file_set_error(f, ret);
>              break;
>          }
>      }

[PATCH v2 19/24] migration: Refactor qemu_savevm_state_setup()

[Peter Xu]

Split it into two smaller chunks:

  - Dump of early_setup VMSDs
  - Dump of save_setup() sections

They're mutual exclusive, hence we can run two loops and do them
sequentially.  This will cause migration thread to loop one more time, but
it should be fine when migration just started and only do it once.  It's
needed because we will need to reuse the early_vmsd helper later to
deduplicate code elsewhere.

QEMU almost sticks with qemu_savevm_state_XXX() to represent the dump of
vmstates's section XXX.  With that in mind, this patch renamed the original
qemu_savevm_state_setup() to qemu_savevm_state_do_setup() instead.

So after this patch:

  - qemu_savevm_state_non_iterable_early() dumps early_vmsds only,
  - qemu_savevm_state_setup() dumps save_setup() sections only,
  - qemu_savevm_state_do_setup() does all things needed during setup
    phase (including migration SETUP notifies)

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/savevm.h    |  6 +++--
 migration/migration.c |  4 ++--
 migration/savevm.c    | 56 ++++++++++++++++++++++++++++++-------------
 3 files changed, 46 insertions(+), 20 deletions(-)

diff --git a/migration/savevm.h b/migration/savevm.h
index bded5e2a6c..f2750eca09 100644
--- a/migration/savevm.h
+++ b/migration/savevm.h
@@ -34,7 +34,7 @@
 bool qemu_savevm_state_blocked(Error **errp);
 void qemu_savevm_non_migratable_list(strList **reasons);
 int qemu_savevm_state_prepare(Error **errp);
-int qemu_savevm_state_setup(QEMUFile *f, Error **errp);
+int qemu_savevm_state_do_setup(QEMUFile *f, Error **errp);
 bool qemu_savevm_state_guest_unplug_pending(void);
 int qemu_savevm_state_resume_prepare(MigrationState *s);
 void qemu_savevm_send_header(QEMUFile *f);
@@ -75,7 +75,9 @@ int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis,
 int qemu_load_device_state(QEMUFile *f, Error **errp);
 int qemu_loadvm_approve_switchover(void);
 int qemu_savevm_state_non_iterable(QEMUFile *f);
-
+int qemu_savevm_state_non_iterable_early(QEMUFile *f,
+                                         JSONWriter *vmdesc,
+                                         Error **errp);
 bool qemu_loadvm_load_state_buffer(const char *idstr, uint32_t instance_id,
                                    char *buf, size_t len, Error **errp);
 
diff --git a/migration/migration.c b/migration/migration.c
index b8c85496f8..2cbeebc9ba 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -3528,7 +3528,7 @@ static void *migration_thread(void *opaque)
     }
 
     bql_lock();
-    ret = qemu_savevm_state_setup(s->to_dst_file, &local_err);
+    ret = qemu_savevm_state_do_setup(s->to_dst_file, &local_err);
     bql_unlock();
 
     qemu_savevm_wait_unplug(s, MIGRATION_STATUS_SETUP,
@@ -3651,7 +3651,7 @@ static void *bg_migration_thread(void *opaque)
 
     bql_lock();
     qemu_savevm_state_header(s->to_dst_file);
-    ret = qemu_savevm_state_setup(s->to_dst_file, &local_err);
+    ret = qemu_savevm_state_do_setup(s->to_dst_file, &local_err);
     bql_unlock();
 
     qemu_savevm_wait_unplug(s, MIGRATION_STATUS_SETUP,
diff --git a/migration/savevm.c b/migration/savevm.c
index 0683a103c8..b04a21ffc9 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1367,29 +1367,33 @@ int qemu_savevm_state_prepare(Error **errp)
     return 0;
 }
 
-int qemu_savevm_state_setup(QEMUFile *f, Error **errp)
+int qemu_savevm_state_non_iterable_early(QEMUFile *f,
+                                         JSONWriter *vmdesc,
+                                         Error **errp)
 {
-    ERRP_GUARD();
-    MigrationState *ms = migrate_get_current();
-    JSONWriter *vmdesc = ms->vmdesc;
     SaveStateEntry *se;
-    int ret = 0;
-
-    if (vmdesc) {
-        json_writer_int64(vmdesc, "page_size", qemu_target_page_size());
-        json_writer_start_array(vmdesc, "devices");
-    }
+    int ret;
 
-    trace_savevm_state_setup();
     QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
         if (se->vmsd && se->vmsd->early_setup) {
             ret = vmstate_save(f, se, vmdesc, errp);
             if (ret) {
-                break;
+                return ret;
             }
-            continue;
         }
+    }
+
+    return 0;
+}
+
+static int qemu_savevm_state_setup(QEMUFile *f, Error **errp)
+{
+    SaveStateEntry *se;
+    int ret;
+
+    trace_savevm_state_setup();
 
+    QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
         if (!se->ops || !se->ops->save_setup) {
             continue;
         }
@@ -1399,14 +1403,34 @@ int qemu_savevm_state_setup(QEMUFile *f, Error **errp)
             }
         }
         save_section_header(f, se, QEMU_VM_SECTION_START);
-
         ret = se->ops->save_setup(f, se->opaque, errp);
         save_section_footer(f, se);
         if (ret < 0) {
-            break;
+            return ret;
         }
     }
 
+    return 0;
+}
+
+int qemu_savevm_state_do_setup(QEMUFile *f, Error **errp)
+{
+    ERRP_GUARD();
+    MigrationState *ms = migrate_get_current();
+    JSONWriter *vmdesc = ms->vmdesc;
+    int ret;
+
+    if (vmdesc) {
+        json_writer_int64(vmdesc, "page_size", qemu_target_page_size());
+        json_writer_start_array(vmdesc, "devices");
+    }
+
+    ret = qemu_savevm_state_non_iterable_early(f, vmdesc, errp);
+    if (ret) {
+        return ret;
+    }
+
+    ret = qemu_savevm_state_setup(f, errp);
     if (ret) {
         return ret;
     }
@@ -1826,7 +1850,7 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp)
     ms->to_dst_file = f;
 
     qemu_savevm_state_header(f);
-    ret = qemu_savevm_state_setup(f, errp);
+    ret = qemu_savevm_state_do_setup(f, errp);
     if (ret) {
         goto cleanup;
     }
-- 
2.50.1

Re: [PATCH v2 19/24] migration: Refactor qemu_savevm_state_setup()

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> Split it into two smaller chunks:
>
>   - Dump of early_setup VMSDs
>   - Dump of save_setup() sections
>
> They're mutual exclusive, hence we can run two loops and do them
> sequentially.  This will cause migration thread to loop one more time, but
> it should be fine when migration just started and only do it once.  It's
> needed because we will need to reuse the early_vmsd helper later to
> deduplicate code elsewhere.
>
> QEMU almost sticks with qemu_savevm_state_XXX() to represent the dump of
> vmstates's section XXX.
> With that in mind, this patch renamed the original
> qemu_savevm_state_setup() to qemu_savevm_state_do_setup() instead.
>
> So after this patch:
>
>   - qemu_savevm_state_non_iterable_early() dumps early_vmsds only,
>   - qemu_savevm_state_setup() dumps save_setup() sections only,
>   - qemu_savevm_state_do_setup() does all things needed during setup
>     phase (including migration SETUP notifies)
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

Not for this series, but I think we could try to standardize the naming
in savevm.c a bit. Here's a list of functions from savevm.c plus the
main function called by them. You'll see that there's room from
improvement.

Removing this "qemu" prefix and choosing when to put "state" in the name
would already be a win.

Also maybe making more clear what calls vmstate code and what deals with
se->ops only.

---
qemu_loadvm_state_setup                  { se->ops->load_setup }
qemu_savevm_state_active                 { se->ops->is_active }
qemu_savevm_state_blocked                { se->vmsd->unmigratable }
qemu_savevm_state_cleanup                { se->ops->save_cleanup }
qemu_savevm_state_guest_unplug_pending   { se->vmsd->dev_unplug_pending }
qemu_savevm_state_iterate                { se->ops->save_live_iterate }
qemu_savevm_state_non_iterable_early     { se->vmsd->early_setup; vmstate_save }
qemu_savevm_state_pending_estimate       { se->ops->state_pending_estimate }
qemu_savevm_state_pending_exact          { se->ops->state_pending_exact }
qemu_savevm_state_postcopy_prepare       { se->ops->save_postcopy_prepare }
qemu_savevm_state_prepare                { se->ops->save_prepare }
qemu_savevm_state_resume_prepare         { se->ops->resume_prepare }
loadvm_postcopy_handle_switchover_start  { se->ops->switchover_start }
qemu_loadvm_load_state_buffer            { se->ops->load_state_buffer }
qemu_loadvm_state_switchover_ack_needed  { se->ops->switchover_ack_needed }
qemu_savevm_complete                     { se->ops->save_complete }
qemu_savevm_complete_exists              { se->ops->save_complete }
qemu_savevm_non_migratable_list          { se->vmsd->unmigratable }
qemu_savevm_se_iterable                  { se->ops->save_setup }
save_state_priority                      { se->vmsd->priority }
vmstate_load                             { se->ops->load_state }
vmstate_save_old_style                   { se->ops->save_state }

loadvm_handle_cmd_packaged                  { qemu_loadvm_state_main }
qemu_load_device_state                      { qemu_loadvm_state_main }
qemu_savevm_maybe_send_switchover_start     { qemu_savevm_send_switchover_start }
qemu_savevm_send_*                          { qemu_savevm_command_send }
qemu_savevm_state_complete_postcopy         { qemu_savevm_complete }
qemu_savevm_state_complete_precopy_iterable { qemu_savevm_complete }
qemu_savevm_state_end_precopy               { qemu_savevm_state_end }
qemu_savevm_state_header                    { qemu_savevm_send_header }

qemu_savevm_state_do_setup              { qemu_savevm_state_non_iterable_early;
                                          qemu_savevm_state_setup }

qemu_loadvm_state                       { qemu_loadvm_state_header;
                                          qemu_loadvm_state_setup;
                                          qemu_loadvm_state_switchover_ack_needed;
                                          qemu_loadvm_state_main;
                                          qemu_loadvm_thread_pool_wait }

qemu_loadvm_state_main                  { qemu_loadvm_section_start_full;
                                          qemu_loadvm_section_part_end;
                                          loadvm_process_command }

qemu_savevm_state                       { qemu_savevm_state_header;
                                          qemu_savevm_state_do_setup;
                                          qemu_savevm_state_iterate;
                                          qemu_savevm_state_complete_precopy;
                                          qemu_savevm_state_cleanup }

qemu_savevm_state_complete_precopy      { qemu_savevm_state_complete_precopy_iterable;
                                          qemu_savevm_state_non_iterable;
                                          qemu_savevm_state_end_precopy }

qemu_save_device_state                  { qemu_savevm_state_non_iterable_early;
                                          qemu_savevm_state_non_iterable;
                                          qemu_savevm_state_end }

loadvm_handle_recv_bitmap           { migrate_send_rp_recv_bitmap }
loadvm_postcopy_handle_advise       { ram_postcopy_incoming_init }
loadvm_postcopy_handle_listen       { postcopy_incoming_setup }
loadvm_postcopy_handle_resume       { mis->postcopy_pause_sem_fault }
loadvm_postcopy_handle_run          { loadvm_postcopy_handle_run_bh }
loadvm_postcopy_handle_run_bh       { vm_start }
loadvm_postcopy_ram_handle_discard  { postcopy_ram_prepare_discard }
loadvm_process_command              { switch (cmd) }
loadvm_process_enable_colo          { colo_init_ram_cache }
qemu_loadvm_approve_switchover      { migrate_send_rp_switchover_ack }
qemu_loadvm_load_thread             { data->function }
qemu_loadvm_section_part_end        { vmstate_load }
qemu_loadvm_section_start_full      { vmstate_load }
qemu_loadvm_start_load_thread       { thread_pool_submit }
qemu_loadvm_state_cleanup           { se->ops->load_cleanup }
qemu_loadvm_state_header            { QEMU_VM_FILE_MAGIC }
qemu_loadvm_thread_pool_create      { thread_pool_new }
qemu_loadvm_thread_pool_destroy     { thread_pool_free }
qemu_loadvm_thread_pool_wait        { thread_pool_wait }
qemu_savevm_command_send            { QEMU_VM_COMMAND }
qemu_savevm_send_colo_enable        { MIG_CMD_ENABLE_COLO }
qemu_savevm_send_configuration      { vmstate_save_state(&vmstate_configuration) }
qemu_savevm_send_header             { QEMU_VM_FILE_MAGIC }
qemu_savevm_state_end               { QEMU_VM_EOF }
qemu_savevm_state_non_iterable      { vmstate_save }
qemu_savevm_state_setup             { se->ops->save_setup }
qemu_savevm_state_vm_desc           { QEMU_VM_VMDESCRIPTION }
register_savevm_live                { savevm_state_handler_insert }
save_section_footer                 { QEMU_VM_SECTION_FOOTER }
save_section_header                 { section_type }
savevm_state_handler_insert         { QTAILQ_INSERT_TAIL }
savevm_state_handler_remove         { QTAILQ_REMOVE }
unregister_savevm                   { savevm_state_handler_remove }
vmstate_register_with_alias_id      { savevm_state_handler_insert }
vmstate_save                        { vmstate_save_old_style; vmstate_save_state }
vmstate_unregister                  { savevm_state_handler_remove }

Re: [PATCH v2 19/24] migration: Refactor qemu_savevm_state_setup()

[Peter Xu]

On Wed, Jan 28, 2026 at 04:35:32PM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > Split it into two smaller chunks:
> >
> >   - Dump of early_setup VMSDs
> >   - Dump of save_setup() sections
> >
> > They're mutual exclusive, hence we can run two loops and do them
> > sequentially.  This will cause migration thread to loop one more time, but
> > it should be fine when migration just started and only do it once.  It's
> > needed because we will need to reuse the early_vmsd helper later to
> > deduplicate code elsewhere.
> >
> > QEMU almost sticks with qemu_savevm_state_XXX() to represent the dump of
> > vmstates's section XXX.
> > With that in mind, this patch renamed the original
> > qemu_savevm_state_setup() to qemu_savevm_state_do_setup() instead.
> >
> > So after this patch:
> >
> >   - qemu_savevm_state_non_iterable_early() dumps early_vmsds only,
> >   - qemu_savevm_state_setup() dumps save_setup() sections only,
> >   - qemu_savevm_state_do_setup() does all things needed during setup
> >     phase (including migration SETUP notifies)
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> 
> Reviewed-by: Fabiano Rosas <farosas@suse.de>
> 
> Not for this series, but I think we could try to standardize the naming
> in savevm.c a bit. Here's a list of functions from savevm.c plus the
> main function called by them. You'll see that there's room from
> improvement.
> 
> Removing this "qemu" prefix and choosing when to put "state" in the name
> would already be a win.

True.

IMHO if we choose it again we can use prefix vmstate_.  The current
savevm_state_ is pretty vague on its own when loadvm needs to use it.  Then
it can be vmstate_save_* on save side, and vmstate_load_* on load side (and
vmstate_XXX) when valid on both.

> 
> Also maybe making more clear what calls vmstate code and what deals with
> se->ops only.

Yes we can try, but there'll be some tricky ones, inline below (assuming we
can shorten the prefix to "vmstate_").

> 
> ---
> qemu_loadvm_state_setup                  { se->ops->load_setup }

Taking this one as example.

We could rename this to vmstate_ops_load_setup(), however "load_setup" is
actually a concept that is higher than the impl.  IOW it'll be also
reasonable if we want to add a load_setup() hook to VMSDs some day.  Maybe
still good to rename it to vmstate_load_setup().

> qemu_savevm_state_active                 { se->ops->is_active }

Similarly, here I'd think vmstate_active() (it applies to both save/load)
better than vmstate_ops_active() because active describes the vmstate
instance, and it's fine one day we want to introduce VMSD.active.

...

> qemu_savevm_state_blocked                { se->vmsd->unmigratable }
> qemu_savevm_state_cleanup                { se->ops->save_cleanup }
> qemu_savevm_state_guest_unplug_pending   { se->vmsd->dev_unplug_pending }
> qemu_savevm_state_iterate                { se->ops->save_live_iterate }
> qemu_savevm_state_non_iterable_early     { se->vmsd->early_setup; vmstate_save }
> qemu_savevm_state_pending_estimate       { se->ops->state_pending_estimate }
> qemu_savevm_state_pending_exact          { se->ops->state_pending_exact }
> qemu_savevm_state_postcopy_prepare       { se->ops->save_postcopy_prepare }
> qemu_savevm_state_prepare                { se->ops->save_prepare }
> qemu_savevm_state_resume_prepare         { se->ops->resume_prepare }
> loadvm_postcopy_handle_switchover_start  { se->ops->switchover_start }
> qemu_loadvm_load_state_buffer            { se->ops->load_state_buffer }
> qemu_loadvm_state_switchover_ack_needed  { se->ops->switchover_ack_needed }
> qemu_savevm_complete                     { se->ops->save_complete }
> qemu_savevm_complete_exists              { se->ops->save_complete }
> qemu_savevm_non_migratable_list          { se->vmsd->unmigratable }
> qemu_savevm_se_iterable                  { se->ops->save_setup }
> save_state_priority                      { se->vmsd->priority }
> vmstate_load                             { se->ops->load_state }

This one (and vmstate_save()) is special because it handles both impls
(VMSD and OPS).  The current names are actually suitable.

> vmstate_save_old_style                   { se->ops->save_state }
> 
> loadvm_handle_cmd_packaged                  { qemu_loadvm_state_main }
> qemu_load_device_state                      { qemu_loadvm_state_main }
> qemu_savevm_maybe_send_switchover_start     { qemu_savevm_send_switchover_start }
> qemu_savevm_send_*                          { qemu_savevm_command_send }
> qemu_savevm_state_complete_postcopy         { qemu_savevm_complete }
> qemu_savevm_state_complete_precopy_iterable { qemu_savevm_complete }
> qemu_savevm_state_end_precopy               { qemu_savevm_state_end }
> qemu_savevm_state_header                    { qemu_savevm_send_header }

...

> 
> qemu_savevm_state_do_setup              { qemu_savevm_state_non_iterable_early;
>                                           qemu_savevm_state_setup }
> 
> qemu_loadvm_state                       { qemu_loadvm_state_header;
>                                           qemu_loadvm_state_setup;
>                                           qemu_loadvm_state_switchover_ack_needed;
>                                           qemu_loadvm_state_main;
>                                           qemu_loadvm_thread_pool_wait }
> 
> qemu_loadvm_state_main                  { qemu_loadvm_section_start_full;
>                                           qemu_loadvm_section_part_end;
>                                           loadvm_process_command }
> 
> qemu_savevm_state                       { qemu_savevm_state_header;
>                                           qemu_savevm_state_do_setup;
>                                           qemu_savevm_state_iterate;
>                                           qemu_savevm_state_complete_precopy;
>                                           qemu_savevm_state_cleanup }
> 
> qemu_savevm_state_complete_precopy      { qemu_savevm_state_complete_precopy_iterable;
>                                           qemu_savevm_state_non_iterable;
>                                           qemu_savevm_state_end_precopy }
> 
> qemu_save_device_state                  { qemu_savevm_state_non_iterable_early;
>                                           qemu_savevm_state_non_iterable;
>                                           qemu_savevm_state_end }

Most of above are special functions higher than vmstate alone, hence IMHO
not vmstate_*() APIs.  We can name it whatever way we like, or leave them
be.

> 
> loadvm_handle_recv_bitmap           { migrate_send_rp_recv_bitmap }
> loadvm_postcopy_handle_advise       { ram_postcopy_incoming_init }
> loadvm_postcopy_handle_listen       { postcopy_incoming_setup }
> loadvm_postcopy_handle_resume       { mis->postcopy_pause_sem_fault }
> loadvm_postcopy_handle_run          { loadvm_postcopy_handle_run_bh }
> loadvm_postcopy_handle_run_bh       { vm_start }
> loadvm_postcopy_ram_handle_discard  { postcopy_ram_prepare_discard }
> loadvm_process_command              { switch (cmd) }
> loadvm_process_enable_colo          { colo_init_ram_cache }

These are almost not vmstate relevant but about MIG_CMD*.  We can likely
leave them alone.  Currently the loadvm_ prefix actually sounds not too
bad.

...

[will skip most of below; we could start with vmstate API first]

> qemu_loadvm_approve_switchover      { migrate_send_rp_switchover_ack }
> qemu_loadvm_load_thread             { data->function }
> qemu_loadvm_section_part_end        { vmstate_load }
> qemu_loadvm_section_start_full      { vmstate_load }
> qemu_loadvm_start_load_thread       { thread_pool_submit }
> qemu_loadvm_state_cleanup           { se->ops->load_cleanup }
> qemu_loadvm_state_header            { QEMU_VM_FILE_MAGIC }
> qemu_loadvm_thread_pool_create      { thread_pool_new }
> qemu_loadvm_thread_pool_destroy     { thread_pool_free }
> qemu_loadvm_thread_pool_wait        { thread_pool_wait }
> qemu_savevm_command_send            { QEMU_VM_COMMAND }
> qemu_savevm_send_colo_enable        { MIG_CMD_ENABLE_COLO }
> qemu_savevm_send_configuration      { vmstate_save_state(&vmstate_configuration) }
> qemu_savevm_send_header             { QEMU_VM_FILE_MAGIC }
> qemu_savevm_state_end               { QEMU_VM_EOF }
> qemu_savevm_state_non_iterable      { vmstate_save }
> qemu_savevm_state_setup             { se->ops->save_setup }
> qemu_savevm_state_vm_desc           { QEMU_VM_VMDESCRIPTION }
> register_savevm_live                { savevm_state_handler_insert }
> save_section_footer                 { QEMU_VM_SECTION_FOOTER }
> save_section_header                 { section_type }
> savevm_state_handler_insert         { QTAILQ_INSERT_TAIL }
> savevm_state_handler_remove         { QTAILQ_REMOVE }
> unregister_savevm                   { savevm_state_handler_remove }
> vmstate_register_with_alias_id      { savevm_state_handler_insert }
> vmstate_save                        { vmstate_save_old_style; vmstate_save_state }
> vmstate_unregister                  { savevm_state_handler_remove }
> 

-- 
Peter Xu

Re: [PATCH v2 19/24] migration: Refactor qemu_savevm_state_setup()

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> On Wed, Jan 28, 2026 at 04:35:32PM -0300, Fabiano Rosas wrote:
>> Peter Xu <peterx@redhat.com> writes:
>> 
>> > Split it into two smaller chunks:
>> >
>> >   - Dump of early_setup VMSDs
>> >   - Dump of save_setup() sections
>> >
>> > They're mutual exclusive, hence we can run two loops and do them
>> > sequentially.  This will cause migration thread to loop one more time, but
>> > it should be fine when migration just started and only do it once.  It's
>> > needed because we will need to reuse the early_vmsd helper later to
>> > deduplicate code elsewhere.
>> >
>> > QEMU almost sticks with qemu_savevm_state_XXX() to represent the dump of
>> > vmstates's section XXX.
>> > With that in mind, this patch renamed the original
>> > qemu_savevm_state_setup() to qemu_savevm_state_do_setup() instead.
>> >
>> > So after this patch:
>> >
>> >   - qemu_savevm_state_non_iterable_early() dumps early_vmsds only,
>> >   - qemu_savevm_state_setup() dumps save_setup() sections only,
>> >   - qemu_savevm_state_do_setup() does all things needed during setup
>> >     phase (including migration SETUP notifies)
>> >
>> > Signed-off-by: Peter Xu <peterx@redhat.com>
>> 
>> Reviewed-by: Fabiano Rosas <farosas@suse.de>
>> 
>> Not for this series, but I think we could try to standardize the naming
>> in savevm.c a bit. Here's a list of functions from savevm.c plus the
>> main function called by them. You'll see that there's room from
>> improvement.
>> 
>> Removing this "qemu" prefix and choosing when to put "state" in the name
>> would already be a win.
>
> True.
>
> IMHO if we choose it again we can use prefix vmstate_.  The current
> savevm_state_ is pretty vague on its own when loadvm needs to use it.  Then
> it can be vmstate_save_* on save side, and vmstate_load_* on load side (and
> vmstate_XXX) when valid on both.
>

It's slightly annoying that vmstate is also used in the macros and
virtually every single VMStateDescription all over QEMU. I wish we had a
separate name for the implementation. But vmstate_ is definitely better
than qemu_savevm_state_.

Or perhaps something like migstate, migvm, etc.

I think we could see great benefit from a small-ish change doing:
s/qemu_savevm_state_/new_name_/ and s/SaveStateEntry/NewNameEntry/

Because then it becomes clear there's a high level thing that deals with
SaveVMHandlers and VMStateDescription which are two different ways of
migrating state.

>> 
>> Also maybe making more clear what calls vmstate code and what deals with
>> se->ops only.
>
> Yes we can try, but there'll be some tricky ones, inline below (assuming we
> can shorten the prefix to "vmstate_").
>

Yeah, I see.

>> 
>> ---
>> qemu_loadvm_state_setup                  { se->ops->load_setup }
>
> Taking this one as example.
>
> We could rename this to vmstate_ops_load_setup(), however "load_setup" is
> actually a concept that is higher than the impl.  IOW it'll be also
> reasonable if we want to add a load_setup() hook to VMSDs some day.  Maybe
> still good to rename it to vmstate_load_setup().
>
>> qemu_savevm_state_active                 { se->ops->is_active }
>
> Similarly, here I'd think vmstate_active() (it applies to both save/load)
> better than vmstate_ops_active() because active describes the vmstate
> instance, and it's fine one day we want to introduce VMSD.active.
>
> ...
>
>> qemu_savevm_state_blocked                { se->vmsd->unmigratable }
>> qemu_savevm_state_cleanup                { se->ops->save_cleanup }
>> qemu_savevm_state_guest_unplug_pending   { se->vmsd->dev_unplug_pending }
>> qemu_savevm_state_iterate                { se->ops->save_live_iterate }
>> qemu_savevm_state_non_iterable_early     { se->vmsd->early_setup; vmstate_save }
>> qemu_savevm_state_pending_estimate       { se->ops->state_pending_estimate }
>> qemu_savevm_state_pending_exact          { se->ops->state_pending_exact }
>> qemu_savevm_state_postcopy_prepare       { se->ops->save_postcopy_prepare }
>> qemu_savevm_state_prepare                { se->ops->save_prepare }
>> qemu_savevm_state_resume_prepare         { se->ops->resume_prepare }
>> loadvm_postcopy_handle_switchover_start  { se->ops->switchover_start }
>> qemu_loadvm_load_state_buffer            { se->ops->load_state_buffer }
>> qemu_loadvm_state_switchover_ack_needed  { se->ops->switchover_ack_needed }
>> qemu_savevm_complete                     { se->ops->save_complete }
>> qemu_savevm_complete_exists              { se->ops->save_complete }
>> qemu_savevm_non_migratable_list          { se->vmsd->unmigratable }
>> qemu_savevm_se_iterable                  { se->ops->save_setup }
>> save_state_priority                      { se->vmsd->priority }
>> vmstate_load                             { se->ops->load_state }
>
> This one (and vmstate_save()) is special because it handles both impls
> (VMSD and OPS).  The current names are actually suitable.
>
>> vmstate_save_old_style                   { se->ops->save_state }
>> 
>> loadvm_handle_cmd_packaged                  { qemu_loadvm_state_main }
>> qemu_load_device_state                      { qemu_loadvm_state_main }
>> qemu_savevm_maybe_send_switchover_start     { qemu_savevm_send_switchover_start }
>> qemu_savevm_send_*                          { qemu_savevm_command_send }
>> qemu_savevm_state_complete_postcopy         { qemu_savevm_complete }
>> qemu_savevm_state_complete_precopy_iterable { qemu_savevm_complete }
>> qemu_savevm_state_end_precopy               { qemu_savevm_state_end }
>> qemu_savevm_state_header                    { qemu_savevm_send_header }
>
> ...
>
>> 
>> qemu_savevm_state_do_setup              { qemu_savevm_state_non_iterable_early;
>>                                           qemu_savevm_state_setup }
>> 
>> qemu_loadvm_state                       { qemu_loadvm_state_header;
>>                                           qemu_loadvm_state_setup;
>>                                           qemu_loadvm_state_switchover_ack_needed;
>>                                           qemu_loadvm_state_main;
>>                                           qemu_loadvm_thread_pool_wait }
>> 
>> qemu_loadvm_state_main                  { qemu_loadvm_section_start_full;
>>                                           qemu_loadvm_section_part_end;
>>                                           loadvm_process_command }
>> 
>> qemu_savevm_state                       { qemu_savevm_state_header;
>>                                           qemu_savevm_state_do_setup;
>>                                           qemu_savevm_state_iterate;
>>                                           qemu_savevm_state_complete_precopy;
>>                                           qemu_savevm_state_cleanup }
>> 
>> qemu_savevm_state_complete_precopy      { qemu_savevm_state_complete_precopy_iterable;
>>                                           qemu_savevm_state_non_iterable;
>>                                           qemu_savevm_state_end_precopy }
>> 
>> qemu_save_device_state                  { qemu_savevm_state_non_iterable_early;
>>                                           qemu_savevm_state_non_iterable;
>>                                           qemu_savevm_state_end }
>
> Most of above are special functions higher than vmstate alone, hence IMHO
> not vmstate_*() APIs.  We can name it whatever way we like, or leave them
> be.
>

They definitely need a better name, it's only a question of "when".

>> 
>> loadvm_handle_recv_bitmap           { migrate_send_rp_recv_bitmap }
>> loadvm_postcopy_handle_advise       { ram_postcopy_incoming_init }
>> loadvm_postcopy_handle_listen       { postcopy_incoming_setup }
>> loadvm_postcopy_handle_resume       { mis->postcopy_pause_sem_fault }
>> loadvm_postcopy_handle_run          { loadvm_postcopy_handle_run_bh }
>> loadvm_postcopy_handle_run_bh       { vm_start }
>> loadvm_postcopy_ram_handle_discard  { postcopy_ram_prepare_discard }
>> loadvm_process_command              { switch (cmd) }
>> loadvm_process_enable_colo          { colo_init_ram_cache }
>
> These are almost not vmstate relevant but about MIG_CMD*.  We can likely
> leave them alone.  Currently the loadvm_ prefix actually sounds not too
> bad.
>

I agree.

> ...
>
> [will skip most of below; we could start with vmstate API first]
>
>> qemu_loadvm_approve_switchover      { migrate_send_rp_switchover_ack }
>> qemu_loadvm_load_thread             { data->function }
>> qemu_loadvm_section_part_end        { vmstate_load }
>> qemu_loadvm_section_start_full      { vmstate_load }
>> qemu_loadvm_start_load_thread       { thread_pool_submit }
>> qemu_loadvm_state_cleanup           { se->ops->load_cleanup }
>> qemu_loadvm_state_header            { QEMU_VM_FILE_MAGIC }
>> qemu_loadvm_thread_pool_create      { thread_pool_new }
>> qemu_loadvm_thread_pool_destroy     { thread_pool_free }
>> qemu_loadvm_thread_pool_wait        { thread_pool_wait }
>> qemu_savevm_command_send            { QEMU_VM_COMMAND }
>> qemu_savevm_send_colo_enable        { MIG_CMD_ENABLE_COLO }
>> qemu_savevm_send_configuration      { vmstate_save_state(&vmstate_configuration) }
>> qemu_savevm_send_header             { QEMU_VM_FILE_MAGIC }
>> qemu_savevm_state_end               { QEMU_VM_EOF }
>> qemu_savevm_state_non_iterable      { vmstate_save }
>> qemu_savevm_state_setup             { se->ops->save_setup }
>> qemu_savevm_state_vm_desc           { QEMU_VM_VMDESCRIPTION }
>> register_savevm_live                { savevm_state_handler_insert }
>> save_section_footer                 { QEMU_VM_SECTION_FOOTER }
>> save_section_header                 { section_type }
>> savevm_state_handler_insert         { QTAILQ_INSERT_TAIL }
>> savevm_state_handler_remove         { QTAILQ_REMOVE }
>> unregister_savevm                   { savevm_state_handler_remove }
>> vmstate_register_with_alias_id      { savevm_state_handler_insert }
>> vmstate_save                        { vmstate_save_old_style; vmstate_save_state }
>> vmstate_unregister                  { savevm_state_handler_remove }
>> 

[PATCH v2 20/24] migration: Introduce qemu_savevm_state_active()

[Peter Xu]

Introduce this helper to detect if a SaveStateEntry is active.

Note that this helper can actually also be used in loadvm paths, but let's
stick with this name for now because we still use SaveStateEntry for the
shared structure that both savevm/loadvm uses, where this name still suites.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/savevm.c | 63 ++++++++++++++++++++--------------------------
 1 file changed, 27 insertions(+), 36 deletions(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index b04a21ffc9..c16951b532 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1071,6 +1071,16 @@ void qemu_savevm_state_end(QEMUFile *f)
     qemu_put_byte(f, QEMU_VM_EOF);
 }
 
+static inline bool qemu_savevm_state_active(SaveStateEntry *se)
+{
+    /* When no is_active() hook, always treat it as ACTIVE */
+    if (!se->ops->is_active) {
+        return true;
+    }
+
+    return se->ops->is_active(se->opaque);
+}
+
 /**
  * qemu_savevm_command_send: Send a 'QEMU_VM_COMMAND' type element with the
  *                           command and associated data.
@@ -1352,12 +1362,9 @@ int qemu_savevm_state_prepare(Error **errp)
         if (!se->ops || !se->ops->save_prepare) {
             continue;
         }
-        if (se->ops->is_active) {
-            if (!se->ops->is_active(se->opaque)) {
-                continue;
-            }
+        if (!qemu_savevm_state_active(se)) {
+            continue;
         }
-
         ret = se->ops->save_prepare(se->opaque, errp);
         if (ret < 0) {
             return ret;
@@ -1397,10 +1404,8 @@ static int qemu_savevm_state_setup(QEMUFile *f, Error **errp)
         if (!se->ops || !se->ops->save_setup) {
             continue;
         }
-        if (se->ops->is_active) {
-            if (!se->ops->is_active(se->opaque)) {
-                continue;
-            }
+        if (!qemu_savevm_state_active(se)) {
+            continue;
         }
         save_section_header(f, se, QEMU_VM_SECTION_START);
         ret = se->ops->save_setup(f, se->opaque, errp);
@@ -1450,10 +1455,8 @@ int qemu_savevm_state_resume_prepare(MigrationState *s)
         if (!se->ops || !se->ops->resume_prepare) {
             continue;
         }
-        if (se->ops->is_active) {
-            if (!se->ops->is_active(se->opaque)) {
-                continue;
-            }
+        if (!qemu_savevm_state_active(se)) {
+            continue;
         }
         ret = se->ops->resume_prepare(s, se->opaque);
         if (ret < 0) {
@@ -1481,8 +1484,7 @@ int qemu_savevm_state_iterate(QEMUFile *f, bool postcopy)
         if (!se->ops || !se->ops->save_live_iterate) {
             continue;
         }
-        if (se->ops->is_active &&
-            !se->ops->is_active(se->opaque)) {
+        if (!qemu_savevm_state_active(se)) {
             continue;
         }
         if (se->ops->is_active_iterate &&
@@ -1543,10 +1545,8 @@ static int qemu_savevm_complete(SaveStateEntry *se, QEMUFile *f)
 {
     int ret;
 
-    if (se->ops->is_active) {
-        if (!se->ops->is_active(se->opaque)) {
-            return 0;
-        }
+    if (!qemu_savevm_state_active(se)) {
+        return 0;
     }
 
     trace_savevm_section_start(se->idstr, se->section_id);
@@ -1596,10 +1596,8 @@ bool qemu_savevm_state_postcopy_prepare(QEMUFile *f, Error **errp)
             continue;
         }
 
-        if (se->ops->is_active) {
-            if (!se->ops->is_active(se->opaque)) {
-                continue;
-            }
+        if (!qemu_savevm_state_active(se)) {
+            continue;
         }
 
         trace_savevm_section_start(se->idstr, se->section_id);
@@ -1785,10 +1783,8 @@ void qemu_savevm_state_pending_estimate(uint64_t *must_precopy,
         if (!se->ops || !se->ops->state_pending_estimate) {
             continue;
         }
-        if (se->ops->is_active) {
-            if (!se->ops->is_active(se->opaque)) {
-                continue;
-            }
+        if (!qemu_savevm_state_active(se)) {
+            continue;
         }
         se->ops->state_pending_estimate(se->opaque, must_precopy, can_postcopy);
     }
@@ -1806,10 +1802,8 @@ void qemu_savevm_state_pending_exact(uint64_t *must_precopy,
         if (!se->ops || !se->ops->state_pending_exact) {
             continue;
         }
-        if (se->ops->is_active) {
-            if (!se->ops->is_active(se->opaque)) {
-                continue;
-            }
+        if (!qemu_savevm_state_active(se)) {
+            continue;
         }
         se->ops->state_pending_exact(se->opaque, must_precopy, can_postcopy);
     }
@@ -2829,12 +2823,9 @@ static int qemu_loadvm_state_setup(QEMUFile *f, Error **errp)
         if (!se->ops || !se->ops->load_setup) {
             continue;
         }
-        if (se->ops->is_active) {
-            if (!se->ops->is_active(se->opaque)) {
-                continue;
-            }
+        if (!qemu_savevm_state_active(se)) {
+            continue;
         }
-
         ret = se->ops->load_setup(f, se->opaque, errp);
         if (ret < 0) {
             error_prepend(errp, "Load state of device %s failed: ",
-- 
2.50.1

Re: [PATCH v2 20/24] migration: Introduce qemu_savevm_state_active()

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> Introduce this helper to detect if a SaveStateEntry is active.
>
> Note that this helper can actually also be used in loadvm paths, but let's
> stick with this name for now because we still use SaveStateEntry for the
> shared structure that both savevm/loadvm uses, where this name still suites.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/savevm.c | 63 ++++++++++++++++++++--------------------------
>  1 file changed, 27 insertions(+), 36 deletions(-)
>
> diff --git a/migration/savevm.c b/migration/savevm.c
> index b04a21ffc9..c16951b532 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1071,6 +1071,16 @@ void qemu_savevm_state_end(QEMUFile *f)
>      qemu_put_byte(f, QEMU_VM_EOF);
>  }
>  
> +static inline bool qemu_savevm_state_active(SaveStateEntry *se)
> +{
> +    /* When no is_active() hook, always treat it as ACTIVE */
> +    if (!se->ops->is_active) {
> +        return true;
> +    }
> +
> +    return se->ops->is_active(se->opaque);
> +}
> +
>  /**
>   * qemu_savevm_command_send: Send a 'QEMU_VM_COMMAND' type element with the
>   *                           command and associated data.
> @@ -1352,12 +1362,9 @@ int qemu_savevm_state_prepare(Error **errp)
>          if (!se->ops || !se->ops->save_prepare) {
>              continue;
>          }
> -        if (se->ops->is_active) {
> -            if (!se->ops->is_active(se->opaque)) {
> -                continue;
> -            }
> +        if (!qemu_savevm_state_active(se)) {
> +            continue;
>          }
> -
>          ret = se->ops->save_prepare(se->opaque, errp);
>          if (ret < 0) {
>              return ret;
> @@ -1397,10 +1404,8 @@ static int qemu_savevm_state_setup(QEMUFile *f, Error **errp)
>          if (!se->ops || !se->ops->save_setup) {
>              continue;
>          }
> -        if (se->ops->is_active) {
> -            if (!se->ops->is_active(se->opaque)) {
> -                continue;
> -            }
> +        if (!qemu_savevm_state_active(se)) {
> +            continue;
>          }
>          save_section_header(f, se, QEMU_VM_SECTION_START);
>          ret = se->ops->save_setup(f, se->opaque, errp);
> @@ -1450,10 +1455,8 @@ int qemu_savevm_state_resume_prepare(MigrationState *s)
>          if (!se->ops || !se->ops->resume_prepare) {
>              continue;
>          }
> -        if (se->ops->is_active) {
> -            if (!se->ops->is_active(se->opaque)) {
> -                continue;
> -            }
> +        if (!qemu_savevm_state_active(se)) {
> +            continue;
>          }
>          ret = se->ops->resume_prepare(s, se->opaque);
>          if (ret < 0) {
> @@ -1481,8 +1484,7 @@ int qemu_savevm_state_iterate(QEMUFile *f, bool postcopy)
>          if (!se->ops || !se->ops->save_live_iterate) {
>              continue;
>          }
> -        if (se->ops->is_active &&
> -            !se->ops->is_active(se->opaque)) {
> +        if (!qemu_savevm_state_active(se)) {
>              continue;
>          }
>          if (se->ops->is_active_iterate &&
> @@ -1543,10 +1545,8 @@ static int qemu_savevm_complete(SaveStateEntry *se, QEMUFile *f)
>  {
>      int ret;
>  
> -    if (se->ops->is_active) {
> -        if (!se->ops->is_active(se->opaque)) {
> -            return 0;
> -        }
> +    if (!qemu_savevm_state_active(se)) {
> +        return 0;
>      }
>  
>      trace_savevm_section_start(se->idstr, se->section_id);
> @@ -1596,10 +1596,8 @@ bool qemu_savevm_state_postcopy_prepare(QEMUFile *f, Error **errp)
>              continue;
>          }
>  
> -        if (se->ops->is_active) {
> -            if (!se->ops->is_active(se->opaque)) {
> -                continue;
> -            }
> +        if (!qemu_savevm_state_active(se)) {
> +            continue;
>          }
>  
>          trace_savevm_section_start(se->idstr, se->section_id);
> @@ -1785,10 +1783,8 @@ void qemu_savevm_state_pending_estimate(uint64_t *must_precopy,
>          if (!se->ops || !se->ops->state_pending_estimate) {
>              continue;
>          }
> -        if (se->ops->is_active) {
> -            if (!se->ops->is_active(se->opaque)) {
> -                continue;
> -            }
> +        if (!qemu_savevm_state_active(se)) {
> +            continue;
>          }
>          se->ops->state_pending_estimate(se->opaque, must_precopy, can_postcopy);
>      }
> @@ -1806,10 +1802,8 @@ void qemu_savevm_state_pending_exact(uint64_t *must_precopy,
>          if (!se->ops || !se->ops->state_pending_exact) {
>              continue;
>          }
> -        if (se->ops->is_active) {
> -            if (!se->ops->is_active(se->opaque)) {
> -                continue;
> -            }
> +        if (!qemu_savevm_state_active(se)) {
> +            continue;
>          }
>          se->ops->state_pending_exact(se->opaque, must_precopy, can_postcopy);
>      }
> @@ -2829,12 +2823,9 @@ static int qemu_loadvm_state_setup(QEMUFile *f, Error **errp)
>          if (!se->ops || !se->ops->load_setup) {
>              continue;
>          }
> -        if (se->ops->is_active) {
> -            if (!se->ops->is_active(se->opaque)) {
> -                continue;
> -            }
> +        if (!qemu_savevm_state_active(se)) {
> +            continue;
>          }
> -
>          ret = se->ops->load_setup(f, se->opaque, errp);
>          if (ret < 0) {
>              error_prepend(errp, "Load state of device %s failed: ",

Reviewed-by: Fabiano Rosas <farosas@suse.de>

[PATCH v2 21/24] migration/bg-snapshot: Cleanup error paths

[Peter Xu]

Cleanup bg_migration_thread() function on error handling.  First of all,
early_fail is almost only used to say if BQL is taken.  Since we already
have separate jumping labels, we don't really need it, hence removed.

Also, since local_err is around, making sure every failure path will set a
proper error string for the failure, then propagate to MigrationState.error.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.c | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 2cbeebc9ba..ddf6faab46 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -3616,7 +3616,6 @@ static void *bg_migration_thread(void *opaque)
     int64_t setup_start;
     MigThrError thr_error;
     QEMUFile *fb;
-    bool early_fail = true;
     Error *local_err = NULL;
     int ret;
 
@@ -3662,10 +3661,7 @@ static void *bg_migration_thread(void *opaque)
      * devices to unplug. This to preserve migration state transitions.
      */
     if (ret) {
-        migrate_error_propagate(s, local_err);
-        migrate_set_state(&s->state, MIGRATION_STATUS_ACTIVE,
-                          MIGRATION_STATUS_FAILED);
-        goto fail_setup;
+        goto fail;
     }
 
     s->setup_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) - setup_start;
@@ -3675,11 +3671,13 @@ static void *bg_migration_thread(void *opaque)
     bql_lock();
 
     if (migration_stop_vm(s, RUN_STATE_PAUSED)) {
-        goto fail;
+        error_setg(&local_err, "Failed to stop the VM");
+        goto fail_with_bql;
     }
 
     if (qemu_savevm_state_non_iterable(fb)) {
-        goto fail;
+        error_setg(&local_err, "Failed to save non-iterable devices");
+        goto fail_with_bql;
     }
 
     qemu_savevm_state_end_precopy(s, fb);
@@ -3692,9 +3690,9 @@ static void *bg_migration_thread(void *opaque)
 
     /* Now initialize UFFD context and start tracking RAM writes */
     if (ram_write_tracking_start()) {
-        goto fail;
+        error_setg(&local_err, "Failed to start write tracking");
+        goto fail_with_bql;
     }
-    early_fail = false;
 
     /*
      * Start VM from BH handler to avoid write-fault lock here.
@@ -3726,21 +3724,22 @@ static void *bg_migration_thread(void *opaque)
     }
 
     trace_migration_thread_after_loop();
+    goto done;
+
+fail_with_bql:
+    bql_unlock();
 
 fail:
-    if (early_fail) {
-        migrate_set_state(&s->state, MIGRATION_STATUS_ACTIVE,
-                MIGRATION_STATUS_FAILED);
-        bql_unlock();
-    }
+    /* local_err is guaranteed to be set when reaching here */
+    migrate_error_propagate(s, local_err);
+    migrate_set_state(&s->state, MIGRATION_STATUS_ACTIVE,
+                      MIGRATION_STATUS_FAILED);
 
-fail_setup:
+done:
     bg_migration_iteration_finish(s);
-
     qemu_fclose(fb);
     object_unref(OBJECT(s));
     rcu_unregister_thread();
-
     return NULL;
 }
 
-- 
2.50.1

Re: [PATCH v2 21/24] migration/bg-snapshot: Cleanup error paths

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> Cleanup bg_migration_thread() function on error handling.  First of all,
> early_fail is almost only used to say if BQL is taken.  Since we already
> have separate jumping labels, we don't really need it, hence removed.
>
> Also, since local_err is around, making sure every failure path will set a
> proper error string for the failure, then propagate to MigrationState.error.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

[PATCH v2 22/24] migration: Make qemu_savevm_state_non_iterable() take errp

[Peter Xu]

Let the function report errors to upper layers.  Out of three current
users, two of them already process the errors, except one outlier,
qemu_savevm_state_complete_precopy(), where we do it manually for now with
a comment for TODO.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/savevm.h    |  2 +-
 migration/migration.c |  8 ++++----
 migration/savevm.c    | 13 +++++++------
 3 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/migration/savevm.h b/migration/savevm.h
index f2750eca09..6a589b2990 100644
--- a/migration/savevm.h
+++ b/migration/savevm.h
@@ -74,7 +74,7 @@ int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis,
                            Error **errp);
 int qemu_load_device_state(QEMUFile *f, Error **errp);
 int qemu_loadvm_approve_switchover(void);
-int qemu_savevm_state_non_iterable(QEMUFile *f);
+int qemu_savevm_state_non_iterable(QEMUFile *f, Error **errp);
 int qemu_savevm_state_non_iterable_early(QEMUFile *f,
                                          JSONWriter *vmdesc,
                                          Error **errp);
diff --git a/migration/migration.c b/migration/migration.c
index ddf6faab46..22cfa5f19f 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2545,9 +2545,9 @@ static int postcopy_start(MigrationState *ms, Error **errp)
      */
     qemu_savevm_send_postcopy_listen(fb);
 
-    ret = qemu_savevm_state_non_iterable(fb);
+    ret = qemu_savevm_state_non_iterable(fb, errp);
     if (ret) {
-        error_setg(errp, "Postcopy save non-iterable device states failed");
+        error_prepend(errp, "Postcopy save non-iterable states failed: ");
         goto fail_closefb;
     }
 
@@ -3675,8 +3675,8 @@ static void *bg_migration_thread(void *opaque)
         goto fail_with_bql;
     }
 
-    if (qemu_savevm_state_non_iterable(fb)) {
-        error_setg(&local_err, "Failed to save non-iterable devices");
+    if (qemu_savevm_state_non_iterable(fb, &local_err)) {
+        error_prepend(&local_err, "Failed to save non-iterable devices");
         goto fail_with_bql;
     }
 
diff --git a/migration/savevm.c b/migration/savevm.c
index c16951b532..130b9764a7 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1710,13 +1710,12 @@ void qemu_savevm_state_end_precopy(MigrationState *s, QEMUFile *f)
     qemu_savevm_state_vm_desc(s, f);
 }
 
-int qemu_savevm_state_non_iterable(QEMUFile *f)
+int qemu_savevm_state_non_iterable(QEMUFile *f, Error **errp)
 {
     MigrationState *ms = migrate_get_current();
     int64_t start_ts_each, end_ts_each;
     JSONWriter *vmdesc = ms->vmdesc;
     SaveStateEntry *se;
-    Error *local_err = NULL;
     int ret;
 
     /* Making sure cpu states are synchronized before saving non-iterable */
@@ -1730,10 +1729,8 @@ int qemu_savevm_state_non_iterable(QEMUFile *f)
 
         start_ts_each = qemu_clock_get_us(QEMU_CLOCK_REALTIME);
 
-        ret = vmstate_save(f, se, vmdesc, &local_err);
+        ret = vmstate_save(f, se, vmdesc, errp);
         if (ret) {
-            migrate_error_propagate(ms, error_copy(local_err));
-            error_report_err(local_err);
             return ret;
         }
 
@@ -1750,6 +1747,7 @@ int qemu_savevm_state_non_iterable(QEMUFile *f)
 int qemu_savevm_state_complete_precopy(MigrationState *s)
 {
     QEMUFile *f = s->to_dst_file;
+    Error *local_err = NULL;
     int ret;
 
     ret = qemu_savevm_state_complete_precopy_iterable(f, false);
@@ -1757,8 +1755,11 @@ int qemu_savevm_state_complete_precopy(MigrationState *s)
         return ret;
     }
 
-    ret = qemu_savevm_state_non_iterable(f);
+    /* TODO: pass error upper */
+    ret = qemu_savevm_state_non_iterable(f, &local_err);
     if (ret) {
+        migrate_error_propagate(s, error_copy(local_err));
+        error_report_err(local_err);
         return ret;
     }
 
-- 
2.50.1

Re: [PATCH v2 22/24] migration: Make qemu_savevm_state_non_iterable() take errp

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> Let the function report errors to upper layers.  Out of three current
> users, two of them already process the errors, except one outlier,
> qemu_savevm_state_complete_precopy(), where we do it manually for now with
> a comment for TODO.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/savevm.h    |  2 +-
>  migration/migration.c |  8 ++++----
>  migration/savevm.c    | 13 +++++++------
>  3 files changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/migration/savevm.h b/migration/savevm.h
> index f2750eca09..6a589b2990 100644
> --- a/migration/savevm.h
> +++ b/migration/savevm.h
> @@ -74,7 +74,7 @@ int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis,
>                             Error **errp);
>  int qemu_load_device_state(QEMUFile *f, Error **errp);
>  int qemu_loadvm_approve_switchover(void);
> -int qemu_savevm_state_non_iterable(QEMUFile *f);
> +int qemu_savevm_state_non_iterable(QEMUFile *f, Error **errp);
>  int qemu_savevm_state_non_iterable_early(QEMUFile *f,
>                                           JSONWriter *vmdesc,
>                                           Error **errp);
> diff --git a/migration/migration.c b/migration/migration.c
> index ddf6faab46..22cfa5f19f 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2545,9 +2545,9 @@ static int postcopy_start(MigrationState *ms, Error **errp)
>       */
>      qemu_savevm_send_postcopy_listen(fb);
>  
> -    ret = qemu_savevm_state_non_iterable(fb);
> +    ret = qemu_savevm_state_non_iterable(fb, errp);
>      if (ret) {
> -        error_setg(errp, "Postcopy save non-iterable device states failed");
> +        error_prepend(errp, "Postcopy save non-iterable states failed: ");
>          goto fail_closefb;
>      }
>  
> @@ -3675,8 +3675,8 @@ static void *bg_migration_thread(void *opaque)
>          goto fail_with_bql;
>      }
>  
> -    if (qemu_savevm_state_non_iterable(fb)) {
> -        error_setg(&local_err, "Failed to save non-iterable devices");
> +    if (qemu_savevm_state_non_iterable(fb, &local_err)) {
> +        error_prepend(&local_err, "Failed to save non-iterable devices");

No space or punctuation at the end?

>          goto fail_with_bql;
>      }
>  
> diff --git a/migration/savevm.c b/migration/savevm.c
> index c16951b532..130b9764a7 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1710,13 +1710,12 @@ void qemu_savevm_state_end_precopy(MigrationState *s, QEMUFile *f)
>      qemu_savevm_state_vm_desc(s, f);
>  }
>  
> -int qemu_savevm_state_non_iterable(QEMUFile *f)
> +int qemu_savevm_state_non_iterable(QEMUFile *f, Error **errp)
>  {
>      MigrationState *ms = migrate_get_current();
>      int64_t start_ts_each, end_ts_each;
>      JSONWriter *vmdesc = ms->vmdesc;
>      SaveStateEntry *se;
> -    Error *local_err = NULL;
>      int ret;
>  
>      /* Making sure cpu states are synchronized before saving non-iterable */
> @@ -1730,10 +1729,8 @@ int qemu_savevm_state_non_iterable(QEMUFile *f)
>  
>          start_ts_each = qemu_clock_get_us(QEMU_CLOCK_REALTIME);
>  
> -        ret = vmstate_save(f, se, vmdesc, &local_err);
> +        ret = vmstate_save(f, se, vmdesc, errp);
>          if (ret) {
> -            migrate_error_propagate(ms, error_copy(local_err));
> -            error_report_err(local_err);
>              return ret;
>          }
>  
> @@ -1750,6 +1747,7 @@ int qemu_savevm_state_non_iterable(QEMUFile *f)
>  int qemu_savevm_state_complete_precopy(MigrationState *s)
>  {
>      QEMUFile *f = s->to_dst_file;
> +    Error *local_err = NULL;
>      int ret;
>  
>      ret = qemu_savevm_state_complete_precopy_iterable(f, false);
> @@ -1757,8 +1755,11 @@ int qemu_savevm_state_complete_precopy(MigrationState *s)
>          return ret;
>      }
>  
> -    ret = qemu_savevm_state_non_iterable(f);
> +    /* TODO: pass error upper */
> +    ret = qemu_savevm_state_non_iterable(f, &local_err);
>      if (ret) {
> +        migrate_error_propagate(s, error_copy(local_err));
> +        error_report_err(local_err);
>          return ret;
>      }

[PATCH v2 23/24] migration: Simplify qemu_save_device_state()

[Peter Xu]

This function is used by both COLO and Xen.  Simplify it with two changes:

- Remove checks on qemu_savevm_se_iterable(): this is not needed as
  vmstate_save() also checks for "save_state() || vmsd" instead.  Here,
  save_setup() (or say, iterable states) should be mutual exclusive to
  "save_state() || vmsd" [*].

- Remove migrate_error_propagate(): both of the users are not using live
  migration framework, but raw vmstate operations.  Error propagation is
  not needed for query-migrate persistence.

[*] One tricky user is VFIO, who provided _both_ save_state() and
save_setup().  However VFIO mustn't have been used in these paths or it
means both COLO and Xen have ignored VFIO data instead (that is,
qemu_savevm_se_iterable() will return true for VFIO). Hence, this change is
safe.

Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Paul Durrant <paul@xen.org>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/savevm.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index 130b9764a7..b29272db3b 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1897,13 +1897,8 @@ int qemu_save_device_state(QEMUFile *f)
     QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
         int ret;
 
-        if (qemu_savevm_se_iterable(se)) {
-            continue;
-        }
         ret = vmstate_save(f, se, NULL, &local_err);
         if (ret) {
-            migrate_error_propagate(migrate_get_current(),
-                                    error_copy(local_err));
             error_report_err(local_err);
             return ret;
         }
-- 
2.50.1

Re: [PATCH v2 23/24] migration: Simplify qemu_save_device_state()

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> This function is used by both COLO and Xen.  Simplify it with two changes:
>
> - Remove checks on qemu_savevm_se_iterable(): this is not needed as
>   vmstate_save() also checks for "save_state() || vmsd" instead.  Here,
>   save_setup() (or say, iterable states) should be mutual exclusive to
>   "save_state() || vmsd" [*].
>
> - Remove migrate_error_propagate(): both of the users are not using live
>   migration framework, but raw vmstate operations.  Error propagation is
>   not needed for query-migrate persistence.
>

s/not needed/only needed/

I can fixup if no repost.

> [*] One tricky user is VFIO, who provided _both_ save_state() and
> save_setup().  However VFIO mustn't have been used in these paths or it
> means both COLO and Xen have ignored VFIO data instead (that is,
> qemu_savevm_se_iterable() will return true for VFIO). Hence, this change is
> safe.
>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Paul Durrant <paul@xen.org>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

> ---
>  migration/savevm.c | 5 -----
>  1 file changed, 5 deletions(-)
>
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 130b9764a7..b29272db3b 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1897,13 +1897,8 @@ int qemu_save_device_state(QEMUFile *f)
>      QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
>          int ret;
>  
> -        if (qemu_savevm_se_iterable(se)) {
> -            continue;
> -        }
>          ret = vmstate_save(f, se, NULL, &local_err);
>          if (ret) {
> -            migrate_error_propagate(migrate_get_current(),
> -                                    error_copy(local_err));
>              error_report_err(local_err);
>              return ret;
>          }

Re: [PATCH v2 23/24] migration: Simplify qemu_save_device_state()

[Peter Xu]

On Wed, Jan 28, 2026 at 04:55:17PM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > This function is used by both COLO and Xen.  Simplify it with two changes:
> >
> > - Remove checks on qemu_savevm_se_iterable(): this is not needed as
> >   vmstate_save() also checks for "save_state() || vmsd" instead.  Here,
> >   save_setup() (or say, iterable states) should be mutual exclusive to
> >   "save_state() || vmsd" [*].
> >
> > - Remove migrate_error_propagate(): both of the users are not using live
> >   migration framework, but raw vmstate operations.  Error propagation is
> >   not needed for query-migrate persistence.
> >
> 
> s/not needed/only needed/
> 
> I can fixup if no repost.

I'll fix both issues, including the one in the previous patch.

I'll wait for your review on the last patch to complete to see if I should
repost.

> 
> > [*] One tricky user is VFIO, who provided _both_ save_state() and
> > save_setup().  However VFIO mustn't have been used in these paths or it
> > means both COLO and Xen have ignored VFIO data instead (that is,
> > qemu_savevm_se_iterable() will return true for VFIO). Hence, this change is
> > safe.
> >
> > Cc: David Woodhouse <dwmw2@infradead.org>
> > Cc: Paul Durrant <paul@xen.org>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> 
> Reviewed-by: Fabiano Rosas <farosas@suse.de>

Thanks.

-- 
Peter Xu

[PATCH v2 24/24] migration/colo/xen: Use generic helpers in qemu_save_device_state()

[Peter Xu]

Use qemu_savevm_state_non_iterable*() helpers for saving device states,
rather than walking the vmstate handlers on its own.

Non-iterables can be either early_setup devices, or otherwise.

Note that QEMU only has one early_setup device currently, which is
virtio-mem, and I highly doubt if it is used in either COLO or Xen users..
However this step is still better needed to provide full coverage of all
non-iterable vmstates.

When at it, allow it to report errors.

Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Paul Durrant <paul@xen.org>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/savevm.h |  3 +--
 migration/colo.c   |  2 +-
 migration/savevm.c | 30 +++++++++++++++---------------
 3 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/migration/savevm.h b/migration/savevm.h
index 6a589b2990..2ba0881f3b 100644
--- a/migration/savevm.h
+++ b/migration/savevm.h
@@ -66,8 +66,7 @@ void qemu_savevm_send_postcopy_ram_discard(QEMUFile *f, const char *name,
                                            uint64_t *start_list,
                                            uint64_t *length_list);
 void qemu_savevm_send_colo_enable(QEMUFile *f);
-int qemu_save_device_state(QEMUFile *f);
-
+int qemu_save_device_state(QEMUFile *f, Error **errp);
 int qemu_loadvm_state(QEMUFile *f, Error **errp);
 void qemu_loadvm_state_cleanup(MigrationIncomingState *mis);
 int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis,
diff --git a/migration/colo.c b/migration/colo.c
index db804b25a9..f7a5bd3619 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -454,7 +454,7 @@ static int colo_do_checkpoint_transaction(MigrationState *s,
     }
 
     /* Note: device state is saved into buffer */
-    ret = qemu_save_device_state(fb);
+    ret = qemu_save_device_state(fb, &local_err);
 
     bql_unlock();
     if (ret < 0) {
diff --git a/migration/savevm.c b/migration/savevm.c
index b29272db3b..3a16c467b2 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1887,26 +1887,24 @@ static bool qemu_savevm_se_iterable(SaveStateEntry *se)
     return se->ops && se->ops->save_setup;
 }
 
-int qemu_save_device_state(QEMUFile *f)
+int qemu_save_device_state(QEMUFile *f, Error **errp)
 {
-    Error *local_err = NULL;
-    SaveStateEntry *se;
-
-    cpu_synchronize_all_states();
+    int ret;
 
-    QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
-        int ret;
+    /* Both COLO and Xen never use vmdesc, hence NULL. */
+    ret = qemu_savevm_state_non_iterable_early(f, NULL, errp);
+    if (ret) {
+        return ret;
+    }
 
-        ret = vmstate_save(f, se, NULL, &local_err);
-        if (ret) {
-            error_report_err(local_err);
-            return ret;
-        }
+    ret = qemu_savevm_state_non_iterable(f, errp);
+    if (ret) {
+        return ret;
     }
 
     qemu_savevm_state_end(f);
 
-    return qemu_file_get_error(f);
+    return 0;
 }
 
 static SaveStateEntry *find_se(const char *idstr, uint32_t instance_id)
@@ -3346,9 +3344,11 @@ void qmp_xen_save_devices_state(const char *filename, bool has_live, bool live,
     f = qemu_file_new_output(QIO_CHANNEL(ioc));
     object_unref(OBJECT(ioc));
     qemu_savevm_send_header(f);
-    ret = qemu_save_device_state(f);
+    ret = qemu_save_device_state(f, errp);
     if (ret < 0 || qemu_fclose(f) < 0) {
-        error_setg(errp, "saving Xen device state failed");
+        if (*errp == NULL) {
+            error_setg(errp, "saving Xen device state failed");
+        }
     } else {
         /* libxl calls the QMP command "stop" before calling
          * "xen-save-devices-state" and in case of migration failure, libxl
-- 
2.50.1

Re: [PATCH v2 24/24] migration/colo/xen: Use generic helpers in qemu_save_device_state()

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> Use qemu_savevm_state_non_iterable*() helpers for saving device states,
> rather than walking the vmstate handlers on its own.
>
> Non-iterables can be either early_setup devices, or otherwise.
>
> Note that QEMU only has one early_setup device currently, which is
> virtio-mem, and I highly doubt if it is used in either COLO or Xen users..
> However this step is still better needed to provide full coverage of all
> non-iterable vmstates.
>
> When at it, allow it to report errors.
>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Paul Durrant <paul@xen.org>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

Re: [PATCH v2 00/24] migration: small cleanup series

[Lukas Straub]

[-- Attachment #1: Type: text/plain, Size: 2779 bytes --]

On Tue, 27 Jan 2026 13:52:30 -0500
Peter Xu <peterx@redhat.com> wrote:

> CI: https://gitlab.com/peterx/qemu/-/pipelines/2289719627
> 
> v1: https://lore.kernel.org/r/20260121223336.3381912-1-peterx@redhat.com
> 
> This is the v2 of the small cleanup series.  It used to be only for COLO,
> but now for some other things too around dumping vmstates.
> 
> The small series growed a little bit, because I wanted to try Fabiano's
> request to deduplicate qemu_save_device_state() and precopy's similar
> version to dump non-iterable devices.
> 
> Changelog skipped, as too many things added.  Old patches should almost the
> same with R-bs collected, though.  I still touched up some small things,
> though (where I dropped the R-bs).
> 
> This should pass the new COLO unit test too, I hope I didn't overlook
> something once more. Please anyone let me know if it fails.
> 
> Comments welcomed, thanks.

This now works well in my testing:

Tested-by: Lukas Straub <lukasstraub2@web.de>

> 
> Peter Xu (24):
>   migration: Introduce qemu_savevm_send_* helpers
>   migration: Use qemu_savevm_send_header() in qemu_save_device_state()
>   migration: Remove one migration_in_colo_state() occurance
>   migration/savevm: Remove SaveStateEntry.is_ram
>   migration/colo: Unwrap qemu_savevm_live_state()
>   migration: Remove call to send switchover start event in colo/savevm
>   colo: Forbid VM resume during checkpointing
>   migration/colo: Use the RAM iterable helper directly
>   migration/colo: Document qemu_fflush(fb)
>   migration: Drop iterable_only in qemu_savevm_state_complete_precopy
>   migration: Drop qemu_file_set_error() when save non-iterable fails
>   migration/colo: Send device states without copying buffer
>   migration/postcopy: Send device states without copying buffer
>   migration: Introduce qemu_savevm_state_end()
>   migration: Provide helper for save vm description
>   migration: Split qemu_savevm_state_complete_precopy_non_iterable()
>   migration: qemu_savevm_state_complete_precopy() take MigrationState*
>   migration: Cleanup error propagates in qemu_savevm_state_setup()
>   migration: Refactor qemu_savevm_state_setup()
>   migration: Introduce qemu_savevm_state_active()
>   migration/bg-snapshot: Cleanup error paths
>   migration: Make qemu_savevm_state_non_iterable() take errp
>   migration: Simplify qemu_save_device_state()
>   migration/colo/xen: Use generic helpers in qemu_save_device_state()
> 
>  migration/savevm.h    |  18 +--
>  migration/colo.c      |  16 ++-
>  migration/migration.c |  48 +++----
>  migration/savevm.c    | 312 ++++++++++++++++++++++--------------------
>  monitor/qmp-cmds.c    |   3 +
>  5 files changed, 213 insertions(+), 184 deletions(-)
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]