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 <[email protected]>
>> Signed-off-by: Steve Sistare <[email protected]>
>> Reviewed-by: Peter Xu <[email protected]>
>> ---
>> 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);
> }
> }
