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

[Steven Sistare <steven.sistare@oracle.com>]

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