Re: [PATCH V3 00/10] fix migration of suspended runstate

[Steven Sistare]

On 8/17/2023 2:23 PM, Peter Xu wrote:
> On Mon, Aug 14, 2023 at 11:54:26AM -0700, Steve Sistare wrote:
>> Migration of a guest in the suspended runstate is broken.  The incoming
>> migration code automatically tries to wake the guest, which is wrong;
>> the guest should end migration in the same runstate it started.  Further,
>> for a restored snapshot, the automatic wakeup fails.  The runstate is
>> RUNNING, but the guest is not.  See the commit messages for the details.
> 
> Hi Steve,
> 
> I drafted two small patches to show what I meant, on top of this series.
> Before applying these two, one needs to revert patch 1 in this series.
> 
> After applied, it should also pass all three new suspend tests.  We can
> continue the discussion here based on the patches.

Your 2 patches look good.  I suggest we keep patch 1, and I squash patch 2
into the other patches.

There is one more fix needed: on the sending side, if the state is suspended,
then ticks must be disabled so the tick globals are updated before they are
written to vmstate.  Otherwise, tick starts at 0 in the receiver when
cpu_enable_ticks is called.

-------------------------------------------
diff --git a/migration/migration.c b/migration/migration.c
index b004475..89d56a8 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -63,6 +63,7 @@
 #include "sysemu/cpus.h"
 #include "yank_functions.h"
 #include "sysemu/qtest.h"
+#include "sysemu/cpu-timers.h"
 #include "options.h"
 #include "sysemu/dirtylimit.h"

@@ -2125,6 +2126,9 @@ static int postcopy_start(MigrationState *ms, Error **errp
     trace_postcopy_start_set_run();

     global_state_store();
+    if (runstate_check(RUN_STATE_SUSPENDED)) {
+        cpu_disable_ticks();
+    }
     ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
     if (ret < 0) {
         goto fail;
@@ -2333,6 +2337,9 @@ static void migration_completion(MigrationState *s)
         s->vm_old_state = runstate_get();
         global_state_store();

+        if (runstate_check(RUN_STATE_SUSPENDED)) {
+            cpu_disable_ticks();
+        }
         ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
         trace_migration_completion_vm_stop(ret);
         if (ret >= 0) {
diff --git a/migration/savevm.c b/migration/savevm.c
index 7b9c477..eff6976 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -58,6 +58,7 @@
 #include "qemu/cutils.h"
 #include "io/channel-buffer.h"
 #include "io/channel-file.h"
+#include "sysemu/cpu-timers.h"
 #include "sysemu/replay.h"
 #include "sysemu/runstate.h"
 #include "sysemu/sysemu.h"
@@ -2969,6 +2970,9 @@ bool save_snapshot(const char *name, bool overwrite, const
     saved_vm_running = runstate_is_running();

     global_state_store();
+    if (runstate_check(RUN_STATE_SUSPENDED)) {
+        cpu_disable_ticks();
+    }
     vm_stop(RUN_STATE_SAVE_VM);

     bdrv_drain_all_begin();
@@ -3037,6 +3041,8 @@ bool save_snapshot(const char *name, bool overwrite, const

     if (saved_vm_running) {
         vm_start();
+    } else if (runstate_check(RUN_STATE_SUSPENDED)) {
+        cpu_enable_ticks();
     }
     return ret == 0;
 }

-------------------------------------------

- Steve