On Thu, Nov 30, 2023 at 01:37:16PM -0800, Steve Sistare wrote: > Currently, a vm in the suspended state is not completely stopped. The VCPUs > have been paused, but the cpu clock still runs, and runstate notifiers for > the transition to stopped have not been called. This causes problems for > live migration. Stale cpu timers_state is saved to the migration stream, > causing time errors in the guest when it wakes from suspend, and state that > would have been modified by runstate notifiers is wrong. > > Modify vm_stop to completely stop the vm if the current state is suspended, > transition to RUN_STATE_PAUSED, and remember that the machine was suspended. > Modify vm_start to restore the suspended state. > > This affects all callers of vm_stop and vm_start, notably, the qapi stop and > cont commands. For example: > > (qemu) info status > VM status: paused (suspended) > > (qemu) stop > (qemu) info status > VM status: paused > > (qemu) cont > (qemu) info status > VM status: paused (suspended) > > (qemu) system_wakeup > (qemu) info status > VM status: running
So system_wakeup for a stopped (but used to be suspended) VM will fail directly, not touching vm_was_suspended. It's not mentioned here, but that behavior makes sense to me. > > Suggested-by: Peter Xu <pet...@redhat.com> > Signed-off-by: Steve Sistare <steven.sist...@oracle.com> Reviewed-by: Peter Xu <pet...@redhat.com> Since you touched qapi/, please copy maintainers too. I've copied Markus and Eric in this reply. I also have some nitpicks which may not affect the R-b, please see below. > --- > include/sysemu/runstate.h | 5 +++++ > qapi/misc.json | 10 ++++++++-- > system/cpus.c | 19 ++++++++++++++----- > system/runstate.c | 3 +++ > 4 files changed, 30 insertions(+), 7 deletions(-) > > diff --git a/include/sysemu/runstate.h b/include/sysemu/runstate.h > index f6a337b..1d6828f 100644 > --- a/include/sysemu/runstate.h > +++ b/include/sysemu/runstate.h > @@ -40,6 +40,11 @@ static inline bool shutdown_caused_by_guest(ShutdownCause > cause) > return cause >= SHUTDOWN_CAUSE_GUEST_SHUTDOWN; > } > > +static inline bool runstate_is_started(RunState state) Would runstate_has_vm_running() sound better? It is a bit awkward when saying something like "start a runstate". > +{ > + return state == RUN_STATE_RUNNING || state == RUN_STATE_SUSPENDED; > +} > + > void vm_start(void); > > /** > diff --git a/qapi/misc.json b/qapi/misc.json > index cda2eff..efb8d44 100644 > --- a/qapi/misc.json > +++ b/qapi/misc.json > @@ -134,7 +134,7 @@ > ## > # @stop: > # > -# Stop all guest VCPU execution. > +# Stop all guest VCPU and VM execution. > # > # Since: 0.14 > # > @@ -143,6 +143,9 @@ > # the guest remains paused once migration finishes, as if the -S > # option was passed on the command line. > # > +# In the "suspended" state, it will completely stop the VM and > +# cause a transition to the "paused" state. (Since 9.0) > +# > # Example: > # > # -> { "execute": "stop" } > @@ -153,7 +156,7 @@ > ## > # @cont: > # > -# Resume guest VCPU execution. > +# Resume guest VCPU and VM execution. > # > # Since: 0.14 > # > @@ -165,6 +168,9 @@ > # guest starts once migration finishes, removing the effect of the > # -S command line option if it was passed. > # > +# If the VM was previously suspended, and not been reset or woken, > +# this command will transition back to the "suspended" state. (Since 9.0) > +# > # Example: > # > # -> { "execute": "cont" } > diff --git a/system/cpus.c b/system/cpus.c > index ef7a0d3..cbc6d6d 100644 > --- a/system/cpus.c > +++ b/system/cpus.c > @@ -277,11 +277,15 @@ bool vm_get_suspended(void) > static int do_vm_stop(RunState state, bool send_stop) > { > int ret = 0; > + RunState oldstate = runstate_get(); > > - if (runstate_is_running()) { > + if (runstate_is_started(oldstate)) { > + vm_was_suspended = (oldstate == RUN_STATE_SUSPENDED); > runstate_set(state); > cpu_disable_ticks(); > - pause_all_vcpus(); > + if (oldstate == RUN_STATE_RUNNING) { > + pause_all_vcpus(); > + } > vm_state_notify(0, state); > if (send_stop) { > qapi_event_send_stop(); > @@ -736,8 +740,13 @@ int vm_prepare_start(bool step_pending, RunState state) > > void vm_start(void) > { > - if (!vm_prepare_start(false, RUN_STATE_RUNNING)) { > - resume_all_vcpus(); > + RunState state = vm_was_suspended ? RUN_STATE_SUSPENDED : > RUN_STATE_RUNNING; > + > + if (!vm_prepare_start(false, state)) { > + if (state == RUN_STATE_RUNNING) { > + resume_all_vcpus(); > + } > + vm_was_suspended = false; > } > } > > @@ -745,7 +754,7 @@ void vm_start(void) > current state is forgotten forever */ > int vm_stop_force_state(RunState state) > { > - if (runstate_is_running()) { > + if (runstate_is_started(runstate_get())) { > return vm_stop(state); > } else { > int ret; > diff --git a/system/runstate.c b/system/runstate.c > index ea9d6c2..e2fa204 100644 > --- a/system/runstate.c > +++ b/system/runstate.c > @@ -108,6 +108,7 @@ static const RunStateTransition > runstate_transitions_def[] = { > { RUN_STATE_PAUSED, RUN_STATE_POSTMIGRATE }, > { RUN_STATE_PAUSED, RUN_STATE_PRELAUNCH }, > { RUN_STATE_PAUSED, RUN_STATE_COLO}, > + { RUN_STATE_PAUSED, RUN_STATE_SUSPENDED}, > > { RUN_STATE_POSTMIGRATE, RUN_STATE_RUNNING }, > { RUN_STATE_POSTMIGRATE, RUN_STATE_FINISH_MIGRATE }, > @@ -161,6 +162,7 @@ static const RunStateTransition > runstate_transitions_def[] = { > { RUN_STATE_SUSPENDED, RUN_STATE_FINISH_MIGRATE }, > { RUN_STATE_SUSPENDED, RUN_STATE_PRELAUNCH }, > { RUN_STATE_SUSPENDED, RUN_STATE_COLO}, > + { RUN_STATE_SUSPENDED, RUN_STATE_PAUSED}, > > { RUN_STATE_WATCHDOG, RUN_STATE_RUNNING }, > { RUN_STATE_WATCHDOG, RUN_STATE_FINISH_MIGRATE }, > @@ -502,6 +504,7 @@ void qemu_system_reset(ShutdownCause reason) > qapi_event_send_reset(shutdown_caused_by_guest(reason), reason); > } > cpu_synchronize_all_post_reset(); > + vm_set_suspended(false); > } > > /* > -- > 1.8.3.1 > -- Peter Xu