On 12/22/2023 7:20 AM, Markus Armbruster wrote:
> Steve Sistare <steven.sist...@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 <pet...@redhat.com>
>> Signed-off-by: Steve Sistare <steven.sist...@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 }
>
> [...]
