Re: [PATCH V5 02/12] cpus: stop vm in suspended state

["Daniel P. Berrangé" <berrange@redhat.com>]

On Mon, Nov 20, 2023 at 04:44:50PM -0500, Peter Xu wrote:
> On Mon, Nov 20, 2023 at 03:55:54PM -0500, Steven Sistare wrote:
> > If we drop force, then all calls to vm_stop will completely stop the
> > suspended state, eg an hmp "stop" command. This causes two problems.
> > First, that is a change in user-visible behavior for something that
> > currently works,
> 
> IMHO it depends on what should be the correct behavior.  IOW, when VM is in
> SUSPENDED state and then the user sends "stop" QMP command, what should we
> expect?

I would say that from a mgmtm app POV "stop" is initiating a state
transition, from RUN_STATE_RUNNING to RUN_STATE_PAUSED and "cont"
is doing the reverse from PAUSED to RUNNING.

It is a little more complicated than that as there are some other
states like INMIGRATE that are conceptually equiv to RUNNING,
and states where the transition simply doesn't make sense.


So my question is if we're in "SUSPENDED" and someone issues "stop",
what state do we go into, and perhaps more importantly what state
do we go to in a subsequent "cont".

If you say SUSPENDED ---(stop)---> PAUSED ---(cont)---> SUSPENDED
then we create a problem, because the decision for the transition
out of PAUSED needs memory of the previous state.

> My understanding is we should expect to fully stop the VM, including the
> ticks, for example.  Keeping the ticks running even after QMP "stop"
> doesn't sound right, isn't it?

The "stop" QMP command is documented as

    "Stop all guest VCPU execution"

the devil is in the detail though, and we've not documented any detail.

Whether or not timers keep running across stop/cont I think can be
argued to be an impl detail, as long as the headline goal "vcpus
don't execute" is satisfied.

> > vs the migration code where we are fixing brokenness.
> 
> This is not a migration-only bug if above holds, IMO.
> 
> > Second, it does not quite work, because the state becomes
> > RUN_STATE_PAUSED, so the suspended state is forgotten, and the hmp "cont"
> > will try to set the running state.  I could fix that by introducing a new
> > state RUN_STATE_SUSPENDED_STOPPED, but again it is a user-visible change
> > in existing behavior.  (I even implemented that while developing, then I
> > realized it was not needed to fix the migration bugs.)
> 
> Good point.

We have added new guest states periodically. It is a user visible
change, but you could argue that it is implementing a new feature
ie the ability to "stop" a "suspended" guest, and so is justified.

S3 is so little used in virt, so I'm not surprised we're finding
long standing edge cases that have never been thought about before.

> Now with above comments, what's your thoughts on whether we should change
> the user behavior?  My answer is still a yes.
> 
> Maybe SUSPENDED should not be a RunState at all? SUSPENDED is guest visible
> behavior, while something like QMP "stop" is not guest visible.  Maybe we
> should remember it separately?

Yes, every time I look at this area I come away thinking that
the RunState enum is a mess, overloading too many different
concepts onto the same single field.

Specifically "SUSPENDED" vs "RUNNING" is a reflection of guest
state (ie whether or not the VM is in S3), but pretty much all
the others are a reflection of QEMU host state. I kind of feel
that SUSPENDED (S3) probably shouldn't have been a RunState at
all. I'd probably put guest-panicked into a separate thing too.

But we're stuck with what we have.

> It means qemu_system_suspend() could remember that in a separate field (as
> part of guest state), then when wakeup we should conditionally go back
> with/without vcpus running depending on the new "suspended" state.



With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|