Re: [PATCH V6 03/14] cpus: stop vm in suspended runstate

[Markus Armbruster <armbru@redhat.com>]

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 }
>> 
>> [...]