Re: [PATCH V7 02/12] cpus: stop vm in suspended runstate

[Steven Sistare]

On 12/6/2023 1:45 PM, Philippe Mathieu-Daudé wrote:
> Hi Steve,
> 
> On 6/12/23 18:23, Steve Sistare wrote:
>> 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.
>>
>> 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) system_wakeup
>>      Error: Unable to wake up: guest is not in suspended state
>>
>>      (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>
>> Reviewed-by: Peter Xu <peterx@redhat.com>
>> ---
>>   include/sysemu/runstate.h |  5 +++++
>>   qapi/misc.json            | 10 ++++++++--
>>   system/cpus.c             | 23 +++++++++++++++--------
>>   system/runstate.c         |  3 +++
>>   4 files changed, 31 insertions(+), 10 deletions(-)
>>
>> diff --git a/include/sysemu/runstate.h b/include/sysemu/runstate.h
>> index 88a67e2..867e53c 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_live(RunState state)
>> +{
>> +    return state == RUN_STATE_RUNNING || state == RUN_STATE_SUSPENDED;
>> +}
> 
> Not being familiar with (live) migration, from a generic vCPU PoV
> I don't get what "runstate_is_live" means. Can we add a comment
> explaining what this helper is for?

Sure.  "live" means the cpu clock is ticking, and the runstate notifiers think
we are running.  It is everything that is enabled in vm_start except for 
starting
the vcpus.

> Since this is a migration particular case, maybe we can be verbose
> in vm_resume() and keep runstate_is_live() -- eventually undocumented
> -- in migration/migration.c.

runstate_is_live is about cpu and vm state, not migration state (the "live" is 
not 
live migration), and is used in multiple places in cpus code and elsewhere, so 
I would 
like to keep it in runstate.h.  It has a specific meaning, and it is useful to 
search 
for it to see who handles "liveness", and distinguish it from code that checks 
the
running and suspended states for other reasons.

- Steve

>  void vm_resume(RunState state)
>  {
>      switch (state) {
>      case RUN_STATE_RUNNING:
>      case RUN_STATE_SUSPENDED:
>          vm_start();
>          break;
>      default:
>          runstate_set(state);
>      }
>  }