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

[Peter Xu <peterx@redhat.com>]

On Mon, Nov 13, 2023 at 10:33:50AM -0800, Steve Sistare wrote:
> 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.  Modify vm_stop_force_state to
> completely stop the vm if the current state is suspended, to be called for
> live migration and snapshots.
> 
> Suggested-by: Peter Xu <peterx@redhat.com>
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
>  system/cpus.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/system/cpus.c b/system/cpus.c
> index f72c4be..c772708 100644
> --- a/system/cpus.c
> +++ b/system/cpus.c
> @@ -255,6 +255,8 @@ void cpu_interrupt(CPUState *cpu, int mask)
>  static int do_vm_stop(RunState state, bool send_stop, bool force)
>  {
>      int ret = 0;
> +    bool running = runstate_is_running();
> +    bool suspended = runstate_check(RUN_STATE_SUSPENDED);
>  
>      if (qemu_in_vcpu_thread()) {
>          qemu_system_vmstop_request_prepare();
> @@ -267,10 +269,12 @@ static int do_vm_stop(RunState state, bool send_stop, bool force)
>          return 0;
>      }
>  
> -    if (runstate_is_running()) {
> +    if (running || (suspended && force)) {
>          runstate_set(state);
>          cpu_disable_ticks();

Not directly relevant, but this is weird that I just notice.

If we disable ticks before stopping vCPUs, IIUC it means vcpus can see
stall ticks.  I checked the vm_start() and indeed that one did it in the
other way round: we'll stop vCPUs before stopping the ticks.

> -        pause_all_vcpus();
> +        if (running) {
> +            pause_all_vcpus();
> +        }
>          vm_state_notify(0, state);
>          if (send_stop) {
>              qapi_event_send_stop();

IIUC the "force" is not actually needed.  It's only used when SUSPENDED,
right?

In general, considering all above, I'm wondering something like this would
be much cleaner (and dropping force)?

===8<===
static int do_vm_stop(RunState state, bool send_stop)
 {
+    bool suspended = runstate_check(RUN_STATE_SUSPENDED);
+    bool running = runstate_is_running();
     int ret = 0;
 
-    if (runstate_is_running()) {
+    /*
+     * RUNNING:   VM and vCPUs are all running
+     * SUSPENDED: VM is running, VCPUs are stopped
+     * Others:    VM and vCPUs are all stopped
+     */
+
+    /* Whether do we need to stop vCPUs? */
+    if (running) {
+        pause_all_vcpus();
+    }
+
+    /* Whether do we need to stop the VM in general? */
+    if (running || suspended) {
         runstate_set(state);
         cpu_disable_ticks();
-        pause_all_vcpus();
         vm_state_notify(0, state);
         if (send_stop) {
             qapi_event_send_stop();

-- 
Peter Xu