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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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?

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.

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

Regards,

Phil.