Steven Sistare <steven.sist...@oracle.com> writes:
> 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 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 <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.
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 }
>>
>> [...]
