On 12/23/2023 12:41 AM, Markus Armbruster wrote:
> 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?
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 <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.
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 }
>>>
>>> [...]
>
