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

Peter Xu posted 24 patches 1 week, 5 days ago
Maintainers: Hailiang Zhang <zhanghailiang@xfusion.com>, Peter Xu <peterx@redhat.com>, Fabiano Rosas <farosas@suse.de>, Markus Armbruster <armbru@redhat.com>
[PATCH v2 19/24] migration: Refactor qemu_savevm_state_setup()
Posted by Peter Xu 1 week, 5 days ago
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()
Posted by Fabiano Rosas 1 week, 4 days ago
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()
Posted by Peter Xu 1 week, 4 days ago
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()
Posted by Fabiano Rosas 1 week, 4 days ago
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 }
>>