On 11/30/2023 5:10 PM, Peter Xu wrote:
> On Thu, Nov 30, 2023 at 01:37:16PM -0800, 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) cont
>> (qemu) info status
>> VM status: paused (suspended)
>>
>> (qemu) system_wakeup
>> (qemu) info status
>> VM status: running
>
> So system_wakeup for a stopped (but used to be suspended) VM will fail
> directly, not touching vm_was_suspended. It's not mentioned here, but that
> behavior makes sense to me.
Right. I'll add that example above.
>> Suggested-by: Peter Xu <pet...@redhat.com>
>> Signed-off-by: Steve Sistare <steven.sist...@oracle.com>
>
> Reviewed-by: Peter Xu <pet...@redhat.com>
>
> Since you touched qapi/, please copy maintainers too. I've copied Markus
> and Eric in this reply.
My bad, thanks for the cc.
> I also have some nitpicks which may not affect the R-b, please see below.
>
>> ---
>> 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)
>
> Would runstate_has_vm_running() sound better? It is a bit awkward when
> saying something like "start a runstate".
I have been searching for the perfect name for this accessor.
IMO using "running" in this accessor is confusing because it applies to both
the running and suspended state. So, I invented a new aggregate state called
started. vm_start transitions the machine to a started state.
How about runstate_was_started? It works well at both start and stop call
sites:
void vm_resume(RunState state)
if (runstate_was_started(state)) {
vm_start();
int vm_stop_force_state(RunState state)
if (runstate_was_started(runstate_get())) {
return vm_stop(state);
- Steve
>> +{
>> + return state == RUN_STATE_RUNNING || state == RUN_STATE_SUSPENDED;
>> +}
>> +
