| Message ID | 1701380247-340457-4-git-send-email-steven.sistare@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fix migration of suspended runstate | expand |
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 <peterx@redhat.com> > Signed-off-by: Steve Sistare <steven.sistare@oracle.com> Reviewed-by: Peter Xu <peterx@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 >
On 11/30/2023 5:10 PM, Peter Xu wrote: > 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. Right. I'll add that example above. >> Suggested-by: Peter Xu <peterx@redhat.com> >> Signed-off-by: Steve Sistare <steven.sistare@oracle.com> > > Reviewed-by: Peter Xu <peterx@redhat.com> > > Since you touched qapi/, please copy maintainers too. I've copied Markus > and Eric in this reply. My bad, thanks for the cc. > 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". I have been searching for the perfect name for this accessor. IMO using "running" in this accessor is confusing because it applies to both the running and suspended state. So, I invented a new aggregate state called started. vm_start transitions the machine to a started state. How about runstate_was_started? It works well at both start and stop call sites: void vm_resume(RunState state) if (runstate_was_started(state)) { vm_start(); int vm_stop_force_state(RunState state) if (runstate_was_started(runstate_get())) { return vm_stop(state); - Steve >> +{ >> + return state == RUN_STATE_RUNNING || state == RUN_STATE_SUSPENDED; >> +} >> +
On Fri, Dec 01, 2023 at 12:11:32PM -0500, Steven Sistare wrote: > >> 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". > > I have been searching for the perfect name for this accessor. > IMO using "running" in this accessor is confusing because it applies to both > the running and suspended state. So, I invented a new aggregate state called > started. vm_start transitions the machine to a started state. > > How about runstate_was_started? It works well at both start and stop call sites: > > void vm_resume(RunState state) > if (runstate_was_started(state)) { This one looks fine, but... > vm_start(); > > int vm_stop_force_state(RunState state) > if (runstate_was_started(runstate_get())) { .. this one makes the past tense not looking good. > return vm_stop(state); How about runstate_is_alive()? So far the best I can come up with. :) Even if you prefer "started", I'd vote for not using past tense, hence runstate_is_started(). Thanks,
On 12/4/2023 11:35 AM, Peter Xu wrote: > On Fri, Dec 01, 2023 at 12:11:32PM -0500, Steven Sistare wrote: >>>> 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". >> >> I have been searching for the perfect name for this accessor. >> IMO using "running" in this accessor is confusing because it applies to both >> the running and suspended state. So, I invented a new aggregate state called >> started. vm_start transitions the machine to a started state. >> >> How about runstate_was_started? It works well at both start and stop call sites: >> >> void vm_resume(RunState state) >> if (runstate_was_started(state)) { > > This one looks fine, but... > >> vm_start(); >> >> int vm_stop_force_state(RunState state) >> if (runstate_was_started(runstate_get())) { > > .. this one makes the past tense not looking good. > >> return vm_stop(state); > > How about runstate_is_alive()? So far the best I can come up with. :) > > Even if you prefer "started", I'd vote for not using past tense, hence > runstate_is_started(). runstate_is_live also occurred to me. I'll use that. - Steve
Steve Sistare <steven.sistare@oracle.com> writes: > 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. Can you explain this to me in terms of the @current_run_state state machine? Like Before the patch, trigger X in state Y goes to state Z. Afterwards, it goes to ... > 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 > > Suggested-by: Peter Xu <peterx@redhat.com> > Signed-off-by: Steve Sistare <steven.sistare@oracle.com> > --- > 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) > +{ > + 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. Doesn't "stop all VM execution" imply "stop all guest vCPU execution"? > # > # Since: 0.14 > # > @@ -143,6 +143,9 @@ # Notes: This function will succeed even if the guest is already in # the stopped state. In "inmigrate" state, it will ensure that > # 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) > +# What user-observable (with query-status) state transitions are possible here? > # 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 @@ # Returns: If successful, nothing # # Notes: This command will succeed if the guest is currently running. # It will also succeed if the guest is in the "inmigrate" state; # in this case, the effect of the command is to make sure the > # 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) Long line. What user-observable state transitions are possible here? > +# > # Example: > # > # -> { "execute": "cont" } Should we update documentation of query-status, too? ## # @StatusInfo: # # Information about VCPU run state # # @running: true if all VCPUs are runnable, false if not runnable # # @singlestep: true if using TCG with one guest instruction per # translation block # # @status: the virtual machine @RunState # # Features: # # @deprecated: Member 'singlestep' is deprecated (with no # replacement). # # Since: 0.14 # # Notes: @singlestep is enabled on the command line with '-accel # tcg,one-insn-per-tb=on', or with the HMP 'one-insn-per-tb' # command. ## { 'struct': 'StatusInfo', 'data': {'running': 'bool', 'singlestep': { 'type': 'bool', 'features': [ 'deprecated' ]}, 'status': 'RunState'} } ## # @query-status: # # Query the run status of all VCPUs # # Returns: @StatusInfo reflecting all VCPUs # # Since: 0.14 # # Example: # # -> { "execute": "query-status" } # <- { "return": { "running": true, # "singlestep": false, # "status": "running" } } ## { 'command': 'query-status', 'returns': 'StatusInfo', 'allow-preconfig': true } > 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); > } > > /*
On 12/22/2023 7:20 AM, Markus Armbruster wrote: > Steve Sistare <steven.sistare@oracle.com> writes: > >> 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. > > Can you explain this to me in terms of the @current_run_state state > machine? Like > > Before the patch, trigger X in state Y goes to state Z. > Afterwards, it goes to ... Old behavior: RUN_STATE_SUSPENDED --> stop --> RUN_STATE_SUSPENDED New behavior: RUN_STATE_SUSPENDED --> stop --> RUN_STATE_PAUSED RUN_STATE_PAUSED --> cont --> RUN_STATE_SUSPENDED >> 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 >> >> Suggested-by: Peter Xu <peterx@redhat.com> >> Signed-off-by: Steve Sistare <steven.sistare@oracle.com> >> --- >> 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) >> +{ >> + 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. > > Doesn't "stop all VM execution" imply "stop all guest vCPU execution"? Agreed, so we simply have: # @stop: # Stop guest VM execution. # @cont: # Resume guest VM execution. >> # >> # Since: 0.14 >> # >> @@ -143,6 +143,9 @@ > # Notes: This function will succeed even if the guest is already in > # the stopped state. In "inmigrate" state, it will ensure that >> # 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) >> +# > > What user-observable (with query-status) state transitions are possible > here? {"status": "suspended", "singlestep": false, "running": false} --> stop --> {"status": "paused", "singlestep": false, "running": false} >> # 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 @@ > # Returns: If successful, nothing > # > # Notes: This command will succeed if the guest is currently running. > # It will also succeed if the guest is in the "inmigrate" state; > # in this case, the effect of the command is to make sure the >> # 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) > > Long line. It fits in 80 columns, but perhaps this looks nicer: # If the VM was previously suspended, and not been reset or woken, # this command will transition back to the "suspended" state. # (Since 9.0) > What user-observable state transitions are possible here? {"status": "paused", "singlestep": false, "running": false} --> cont --> {"status": "suspended", "singlestep": false, "running": false} >> +# >> # Example: >> # >> # -> { "execute": "cont" } > > Should we update documentation of query-status, too? IMO no. The new behavior changes the status/RunState field only, and the domain of values does not change, only the transitions caused by the commands described here. - Steve > ## > # @StatusInfo: > # > # Information about VCPU run state > # > # @running: true if all VCPUs are runnable, false if not runnable > # > # @singlestep: true if using TCG with one guest instruction per > # translation block > # > # @status: the virtual machine @RunState > # > # Features: > # > # @deprecated: Member 'singlestep' is deprecated (with no > # replacement). > # > # Since: 0.14 > # > # Notes: @singlestep is enabled on the command line with '-accel > # tcg,one-insn-per-tb=on', or with the HMP 'one-insn-per-tb' > # command. > ## > { 'struct': 'StatusInfo', > 'data': {'running': 'bool', > 'singlestep': { 'type': 'bool', 'features': [ 'deprecated' ]}, > 'status': 'RunState'} } > > ## > # @query-status: > # > # Query the run status of all VCPUs > # > # Returns: @StatusInfo reflecting all VCPUs > # > # Since: 0.14 > # > # Example: > # > # -> { "execute": "query-status" } > # <- { "return": { "running": true, > # "singlestep": false, > # "status": "running" } } > ## > { 'command': 'query-status', 'returns': 'StatusInfo', > 'allow-preconfig': true } > > [...]
Steven Sistare <steven.sistare@oracle.com> writes: > On 12/22/2023 7:20 AM, Markus Armbruster wrote: >> Steve Sistare <steven.sistare@oracle.com> writes: >> >>> 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. >> >> Can you explain this to me in terms of the @current_run_state state >> machine? Like >> >> Before the patch, trigger X in state Y goes to state Z. >> Afterwards, it goes to ... > > Old behavior: > RUN_STATE_SUSPENDED --> stop --> RUN_STATE_SUSPENDED > > New behavior: > RUN_STATE_SUSPENDED --> stop --> RUN_STATE_PAUSED > RUN_STATE_PAUSED --> cont --> RUN_STATE_SUSPENDED This clarifies things quite a bit for me. Maybe work it into the commit message? >>> 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 >>> >>> Suggested-by: Peter Xu <peterx@redhat.com> >>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com> >>> --- >>> 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) >>> +{ >>> + 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. >> >> Doesn't "stop all VM execution" imply "stop all guest vCPU execution"? > > Agreed, so we simply have: > > # @stop: > # Stop guest VM execution. > > # @cont: > # Resume guest VM execution. Yes, please. >>> # >>> # Since: 0.14 >>> # >>> @@ -143,6 +143,9 @@ >> # Notes: This function will succeed even if the guest is already in >> # the stopped state. In "inmigrate" state, it will ensure that >>> # 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) >>> +# >> >> What user-observable (with query-status) state transitions are possible >> here? > > {"status": "suspended", "singlestep": false, "running": false} > --> stop --> > {"status": "paused", "singlestep": false, "running": false} > >>> # 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 @@ >> # Returns: If successful, nothing >> # >> # Notes: This command will succeed if the guest is currently running. >> # It will also succeed if the guest is in the "inmigrate" state; >> # in this case, the effect of the command is to make sure the >>> # 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) >> >> Long line. > > It fits in 80 columns, but perhaps this looks nicer: > > # If the VM was previously suspended, and not been reset or woken, > # this command will transition back to the "suspended" state. > # (Since 9.0) It does :) docs/devel/qapi-code-gen.rst section "Documentation markup": For legibility, wrap text paragraphs so every line is at most 70 characters long. >> What user-observable state transitions are possible here? > > {"status": "paused", "singlestep": false, "running": false} > --> cont --> > {"status": "suspended", "singlestep": false, "running": false} > >>> +# >>> # Example: >>> # >>> # -> { "execute": "cont" } >> >> Should we update documentation of query-status, too? > > IMO no. The new behavior changes the status/RunState field only, and the > domain of values does not change, only the transitions caused by the commands > described here. I see. But if we change the stop's and cont's description from "guest VCPU execution" to "guest VM execution", maybe we want to change query-status's from "Information about VCPU run state" to "Information about VM run state. > - Steve > >> ## >> # @StatusInfo: >> # >> # Information about VCPU run state >> # >> # @running: true if all VCPUs are runnable, false if not runnable >> # >> # @singlestep: true if using TCG with one guest instruction per >> # translation block >> # >> # @status: the virtual machine @RunState >> # >> # Features: >> # >> # @deprecated: Member 'singlestep' is deprecated (with no >> # replacement). >> # >> # Since: 0.14 >> # >> # Notes: @singlestep is enabled on the command line with '-accel >> # tcg,one-insn-per-tb=on', or with the HMP 'one-insn-per-tb' >> # command. >> ## >> { 'struct': 'StatusInfo', >> 'data': {'running': 'bool', >> 'singlestep': { 'type': 'bool', 'features': [ 'deprecated' ]}, >> 'status': 'RunState'} } >> >> ## >> # @query-status: >> # >> # Query the run status of all VCPUs >> # >> # Returns: @StatusInfo reflecting all VCPUs >> # >> # Since: 0.14 >> # >> # Example: >> # >> # -> { "execute": "query-status" } >> # <- { "return": { "running": true, >> # "singlestep": false, >> # "status": "running" } } >> ## >> { 'command': 'query-status', 'returns': 'StatusInfo', >> 'allow-preconfig': true } >> >> [...]
Steven, The discussions seem to still apply to the latest. Do you plan to post a new version, with everything covered? Thanks,
On 1/3/2024 8:09 AM, Peter Xu wrote: > Steven, > > The discussions seem to still apply to the latest. Do you plan to post a > new version, with everything covered? Yes, today, thanks - steve
On 12/23/2023 12:41 AM, Markus Armbruster wrote: > Steven Sistare <steven.sistare@oracle.com> writes: > >> On 12/22/2023 7:20 AM, Markus Armbruster wrote: >>> Steve Sistare <steven.sistare@oracle.com> writes: >>> >>>> 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. >>> >>> Can you explain this to me in terms of the @current_run_state state >>> machine? Like >>> >>> Before the patch, trigger X in state Y goes to state Z. >>> Afterwards, it goes to ... >> >> Old behavior: >> RUN_STATE_SUSPENDED --> stop --> RUN_STATE_SUSPENDED >> >> New behavior: >> RUN_STATE_SUSPENDED --> stop --> RUN_STATE_PAUSED >> RUN_STATE_PAUSED --> cont --> RUN_STATE_SUSPENDED > > This clarifies things quite a bit for me. Maybe work it into the commit > message? Will do. >>>> 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 >>>> >>>> Suggested-by: Peter Xu <peterx@redhat.com> >>>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com> >>>> --- >>>> 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) >>>> +{ >>>> + 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. >>> >>> Doesn't "stop all VM execution" imply "stop all guest vCPU execution"? >> >> Agreed, so we simply have: >> >> # @stop: >> # Stop guest VM execution. >> >> # @cont: >> # Resume guest VM execution. > > Yes, please. Will do. >>>> # >>>> # Since: 0.14 >>>> # >>>> @@ -143,6 +143,9 @@ >>> # Notes: This function will succeed even if the guest is already in >>> # the stopped state. In "inmigrate" state, it will ensure that >>>> # 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) >>>> +# >>> >>> What user-observable (with query-status) state transitions are possible >>> here? >> >> {"status": "suspended", "singlestep": false, "running": false} >> --> stop --> >> {"status": "paused", "singlestep": false, "running": false} >> >>>> # 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 @@ >>> # Returns: If successful, nothing >>> # >>> # Notes: This command will succeed if the guest is currently running. >>> # It will also succeed if the guest is in the "inmigrate" state; >>> # in this case, the effect of the command is to make sure the >>>> # 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) >>> >>> Long line. >> >> It fits in 80 columns, but perhaps this looks nicer: >> >> # If the VM was previously suspended, and not been reset or woken, >> # this command will transition back to the "suspended" state. >> # (Since 9.0) > > It does :) > > docs/devel/qapi-code-gen.rst section "Documentation markup": > > For legibility, wrap text paragraphs so every line is at most 70 > characters long. Will do, thanks for the reference. >>> What user-observable state transitions are possible here? >> >> {"status": "paused", "singlestep": false, "running": false} >> --> cont --> >> {"status": "suspended", "singlestep": false, "running": false} >> >>>> +# >>>> # Example: >>>> # >>>> # -> { "execute": "cont" } >>> >>> Should we update documentation of query-status, too? >> >> IMO no. The new behavior changes the status/RunState field only, and the >> domain of values does not change, only the transitions caused by the commands >> described here. > > I see. > > But if we change the stop's and cont's description from "guest VCPU > execution" to "guest VM execution", maybe we want to change > query-status's from "Information about VCPU run state" to "Information > about VM run state. Makes sense: # @StatusInfo: # -# Information about VCPU run state +# Information about VM run state # @query-status: # -# Query the run status of all VCPUs +# Query the run status of the VM # -# Returns: @StatusInfo reflecting all VCPUs +# Returns: @StatusInfo reflecting the VM With these changes, can I add your Acked-by to the commit? - Steve >>> ## >>> # @StatusInfo: >>> # >>> # Information about VCPU run state >>> # >>> # @running: true if all VCPUs are runnable, false if not runnable >>> # >>> # @singlestep: true if using TCG with one guest instruction per >>> # translation block >>> # >>> # @status: the virtual machine @RunState >>> # >>> # Features: >>> # >>> # @deprecated: Member 'singlestep' is deprecated (with no >>> # replacement). >>> # >>> # Since: 0.14 >>> # >>> # Notes: @singlestep is enabled on the command line with '-accel >>> # tcg,one-insn-per-tb=on', or with the HMP 'one-insn-per-tb' >>> # command. >>> ## >>> { 'struct': 'StatusInfo', >>> 'data': {'running': 'bool', >>> 'singlestep': { 'type': 'bool', 'features': [ 'deprecated' ]}, >>> 'status': 'RunState'} } >>> >>> ## >>> # @query-status: >>> # >>> # Query the run status of all VCPUs >>> # >>> # Returns: @StatusInfo reflecting all VCPUs >>> # >>> # Since: 0.14 >>> # >>> # Example: >>> # >>> # -> { "execute": "query-status" } >>> # <- { "return": { "running": true, >>> # "singlestep": false, >>> # "status": "running" } } >>> ## >>> { 'command': 'query-status', 'returns': 'StatusInfo', >>> 'allow-preconfig': true } >>> >>> [...] >
Steven Sistare <steven.sistare@oracle.com> writes:
[...]
> With these changes, can I add your Acked-by to the commit?
I'm afraid I lost context over the break. Suggest you post v7, and I
provide my Acked-by there. Likely easier for me.
Happy new year!
[...]
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) +{ + 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); } /*
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 Suggested-by: Peter Xu <peterx@redhat.com> Signed-off-by: Steve Sistare <steven.sistare@oracle.com> --- include/sysemu/runstate.h | 5 +++++ qapi/misc.json | 10 ++++++++-- system/cpus.c | 19 ++++++++++++++----- system/runstate.c | 3 +++ 4 files changed, 30 insertions(+), 7 deletions(-)