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

Steve Sistare posted 12 patches 11 months, 3 weeks ago
Maintainers: Stefan Berger <stefanb@linux.vnet.ibm.com>, Gerd Hoffmann <kraxel@redhat.com>, Stefano Stabellini <sstabellini@kernel.org>, Anthony Perard <anthony.perard@citrix.com>, Paul Durrant <paul@xen.org>, Juan Quintela <quintela@redhat.com>, Peter Xu <peterx@redhat.com>, Fabiano Rosas <farosas@suse.de>, Leonardo Bras <leobras@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Eric Blake <eblake@redhat.com>, Markus Armbruster <armbru@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, Thomas Huth <thuth@redhat.com>, Laurent Vivier <lvivier@redhat.com>
There is a newer version of this series
[PATCH V7 02/12] cpus: stop vm in suspended runstate
Posted by Steve Sistare 11 months, 3 weeks ago
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;
+}
+
 void vm_start(void);
 
 /**
diff --git a/qapi/misc.json b/qapi/misc.json
index cda2eff..efb8d44 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -134,7 +134,7 @@
 ##
 # @stop:
 #
-# Stop all guest VCPU execution.
+# Stop all guest VCPU and VM execution.
 #
 # Since: 0.14
 #
@@ -143,6 +143,9 @@
 #     the guest remains paused once migration finishes, as if the -S
 #     option was passed on the command line.
 #
+#     In the "suspended" state, it will completely stop the VM and
+#     cause a transition to the "paused" state. (Since 9.0)
+#
 # Example:
 #
 # -> { "execute": "stop" }
@@ -153,7 +156,7 @@
 ##
 # @cont:
 #
-# Resume guest VCPU execution.
+# Resume guest VCPU and VM execution.
 #
 # Since: 0.14
 #
@@ -165,6 +168,9 @@
 #     guest starts once migration finishes, removing the effect of the
 #     -S command line option if it was passed.
 #
+#     If the VM was previously suspended, and not been reset or woken,
+#     this command will transition back to the "suspended" state. (Since 9.0)
+#
 # Example:
 #
 # -> { "execute": "cont" }
diff --git a/system/cpus.c b/system/cpus.c
index 9f631ab..f162435 100644
--- a/system/cpus.c
+++ b/system/cpus.c
@@ -277,11 +277,15 @@ bool vm_get_suspended(void)
 static int do_vm_stop(RunState state, bool send_stop)
 {
     int ret = 0;
+    RunState oldstate = runstate_get();
 
-    if (runstate_is_running()) {
+    if (runstate_is_live(oldstate)) {
+        vm_was_suspended = (oldstate == RUN_STATE_SUSPENDED);
         runstate_set(state);
         cpu_disable_ticks();
-        pause_all_vcpus();
+        if (oldstate == RUN_STATE_RUNNING) {
+            pause_all_vcpus();
+        }
         vm_state_notify(0, state);
         if (send_stop) {
             qapi_event_send_stop();
@@ -694,11 +698,13 @@ int vm_stop(RunState state)
 
 /**
  * Prepare for (re)starting the VM.
- * Returns -1 if the vCPUs are not to be restarted (e.g. if they are already
- * running or in case of an error condition), 0 otherwise.
+ * Returns 0 if the vCPUs should be restarted, -1 on an error condition,
+ * and 1 otherwise.
  */
 int vm_prepare_start(bool step_pending)
 {
+    int ret = vm_was_suspended ? 1 : 0;
+    RunState state = vm_was_suspended ? RUN_STATE_SUSPENDED : RUN_STATE_RUNNING;
     RunState requested;
 
     qemu_vmstop_requested(&requested);
@@ -729,9 +735,10 @@ int vm_prepare_start(bool step_pending)
     qapi_event_send_resume();
 
     cpu_enable_ticks();
-    runstate_set(RUN_STATE_RUNNING);
-    vm_state_notify(1, RUN_STATE_RUNNING);
-    return 0;
+    runstate_set(state);
+    vm_state_notify(1, state);
+    vm_was_suspended = false;
+    return ret;
 }
 
 void vm_start(void)
@@ -745,7 +752,7 @@ void vm_start(void)
    current state is forgotten forever */
 int vm_stop_force_state(RunState state)
 {
-    if (runstate_is_running()) {
+    if (runstate_is_live(runstate_get())) {
         return vm_stop(state);
     } else {
         int ret;
diff --git a/system/runstate.c b/system/runstate.c
index ea9d6c2..e2fa204 100644
--- a/system/runstate.c
+++ b/system/runstate.c
@@ -108,6 +108,7 @@ static const RunStateTransition runstate_transitions_def[] = {
     { RUN_STATE_PAUSED, RUN_STATE_POSTMIGRATE },
     { RUN_STATE_PAUSED, RUN_STATE_PRELAUNCH },
     { RUN_STATE_PAUSED, RUN_STATE_COLO},
+    { RUN_STATE_PAUSED, RUN_STATE_SUSPENDED},
 
     { RUN_STATE_POSTMIGRATE, RUN_STATE_RUNNING },
     { RUN_STATE_POSTMIGRATE, RUN_STATE_FINISH_MIGRATE },
@@ -161,6 +162,7 @@ static const RunStateTransition runstate_transitions_def[] = {
     { RUN_STATE_SUSPENDED, RUN_STATE_FINISH_MIGRATE },
     { RUN_STATE_SUSPENDED, RUN_STATE_PRELAUNCH },
     { RUN_STATE_SUSPENDED, RUN_STATE_COLO},
+    { RUN_STATE_SUSPENDED, RUN_STATE_PAUSED},
 
     { RUN_STATE_WATCHDOG, RUN_STATE_RUNNING },
     { RUN_STATE_WATCHDOG, RUN_STATE_FINISH_MIGRATE },
@@ -502,6 +504,7 @@ void qemu_system_reset(ShutdownCause reason)
         qapi_event_send_reset(shutdown_caused_by_guest(reason), reason);
     }
     cpu_synchronize_all_post_reset();
+    vm_set_suspended(false);
 }
 
 /*
-- 
1.8.3.1
Re: [PATCH V7 02/12] cpus: stop vm in suspended runstate
Posted by Steven Sistare 11 months, 3 weeks ago
Hi Markus, Eric, any comment about this change in stop/cont behavior before
it gets pulled?  There is no change since V6.

- Steve

On 12/6/2023 12:23 PM, 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;
> +}
> +
>  void vm_start(void);
>  
>  /**
> diff --git a/qapi/misc.json b/qapi/misc.json
> index cda2eff..efb8d44 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -134,7 +134,7 @@
>  ##
>  # @stop:
>  #
> -# Stop all guest VCPU execution.
> +# Stop all guest VCPU and VM execution.
>  #
>  # Since: 0.14
>  #
> @@ -143,6 +143,9 @@
>  #     the guest remains paused once migration finishes, as if the -S
>  #     option was passed on the command line.
>  #
> +#     In the "suspended" state, it will completely stop the VM and
> +#     cause a transition to the "paused" state. (Since 9.0)
> +#
>  # Example:
>  #
>  # -> { "execute": "stop" }
> @@ -153,7 +156,7 @@
>  ##
>  # @cont:
>  #
> -# Resume guest VCPU execution.
> +# Resume guest VCPU and VM execution.
>  #
>  # Since: 0.14
>  #
> @@ -165,6 +168,9 @@
>  #     guest starts once migration finishes, removing the effect of the
>  #     -S command line option if it was passed.
>  #
> +#     If the VM was previously suspended, and not been reset or woken,
> +#     this command will transition back to the "suspended" state. (Since 9.0)
> +#
>  # Example:
>  #
>  # -> { "execute": "cont" }
> diff --git a/system/cpus.c b/system/cpus.c
> index 9f631ab..f162435 100644
> --- a/system/cpus.c
> +++ b/system/cpus.c
> @@ -277,11 +277,15 @@ bool vm_get_suspended(void)
>  static int do_vm_stop(RunState state, bool send_stop)
>  {
>      int ret = 0;
> +    RunState oldstate = runstate_get();
>  
> -    if (runstate_is_running()) {
> +    if (runstate_is_live(oldstate)) {
> +        vm_was_suspended = (oldstate == RUN_STATE_SUSPENDED);
>          runstate_set(state);
>          cpu_disable_ticks();
> -        pause_all_vcpus();
> +        if (oldstate == RUN_STATE_RUNNING) {
> +            pause_all_vcpus();
> +        }
>          vm_state_notify(0, state);
>          if (send_stop) {
>              qapi_event_send_stop();
> @@ -694,11 +698,13 @@ int vm_stop(RunState state)
>  
>  /**
>   * Prepare for (re)starting the VM.
> - * Returns -1 if the vCPUs are not to be restarted (e.g. if they are already
> - * running or in case of an error condition), 0 otherwise.
> + * Returns 0 if the vCPUs should be restarted, -1 on an error condition,
> + * and 1 otherwise.
>   */
>  int vm_prepare_start(bool step_pending)
>  {
> +    int ret = vm_was_suspended ? 1 : 0;
> +    RunState state = vm_was_suspended ? RUN_STATE_SUSPENDED : RUN_STATE_RUNNING;
>      RunState requested;
>  
>      qemu_vmstop_requested(&requested);
> @@ -729,9 +735,10 @@ int vm_prepare_start(bool step_pending)
>      qapi_event_send_resume();
>  
>      cpu_enable_ticks();
> -    runstate_set(RUN_STATE_RUNNING);
> -    vm_state_notify(1, RUN_STATE_RUNNING);
> -    return 0;
> +    runstate_set(state);
> +    vm_state_notify(1, state);
> +    vm_was_suspended = false;
> +    return ret;
>  }
>  
>  void vm_start(void)
> @@ -745,7 +752,7 @@ void vm_start(void)
>     current state is forgotten forever */
>  int vm_stop_force_state(RunState state)
>  {
> -    if (runstate_is_running()) {
> +    if (runstate_is_live(runstate_get())) {
>          return vm_stop(state);
>      } else {
>          int ret;
> diff --git a/system/runstate.c b/system/runstate.c
> index ea9d6c2..e2fa204 100644
> --- a/system/runstate.c
> +++ b/system/runstate.c
> @@ -108,6 +108,7 @@ static const RunStateTransition runstate_transitions_def[] = {
>      { RUN_STATE_PAUSED, RUN_STATE_POSTMIGRATE },
>      { RUN_STATE_PAUSED, RUN_STATE_PRELAUNCH },
>      { RUN_STATE_PAUSED, RUN_STATE_COLO},
> +    { RUN_STATE_PAUSED, RUN_STATE_SUSPENDED},
>  
>      { RUN_STATE_POSTMIGRATE, RUN_STATE_RUNNING },
>      { RUN_STATE_POSTMIGRATE, RUN_STATE_FINISH_MIGRATE },
> @@ -161,6 +162,7 @@ static const RunStateTransition runstate_transitions_def[] = {
>      { RUN_STATE_SUSPENDED, RUN_STATE_FINISH_MIGRATE },
>      { RUN_STATE_SUSPENDED, RUN_STATE_PRELAUNCH },
>      { RUN_STATE_SUSPENDED, RUN_STATE_COLO},
> +    { RUN_STATE_SUSPENDED, RUN_STATE_PAUSED},
>  
>      { RUN_STATE_WATCHDOG, RUN_STATE_RUNNING },
>      { RUN_STATE_WATCHDOG, RUN_STATE_FINISH_MIGRATE },
> @@ -502,6 +504,7 @@ void qemu_system_reset(ShutdownCause reason)
>          qapi_event_send_reset(shutdown_caused_by_guest(reason), reason);
>      }
>      cpu_synchronize_all_post_reset();
> +    vm_set_suspended(false);
>  }
>  
>  /*
Re: [PATCH V7 02/12] cpus: stop vm in suspended runstate
Posted by Philippe Mathieu-Daudé 11 months, 3 weeks ago
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.
Re: [PATCH V7 02/12] cpus: stop vm in suspended runstate
Posted by Steven Sistare 11 months, 3 weeks ago
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 <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?

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);
>      }
>  }


Re: [PATCH V7 02/12] cpus: stop vm in suspended runstate
Posted by Philippe Mathieu-Daudé 11 months, 3 weeks ago
On 6/12/23 20:19, Steven Sistare wrote:
> 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 <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?
> 
> 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.

OK then, no objection, but please add a comment describing.

Thanks,

Phil.

Re: [PATCH V7 02/12] cpus: stop vm in suspended runstate
Posted by Philippe Mathieu-Daudé 11 months, 3 weeks ago
On 6/12/23 21:48, Philippe Mathieu-Daudé wrote:
> On 6/12/23 20:19, Steven Sistare wrote:
>> 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 <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?
>>
>> 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.

What about runstate_is_vcpu_clock_ticking() then?

>>> 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.
> 
> OK then, no objection, but please add a comment describing.
> 
> Thanks,
> 
> Phil.


Re: [PATCH V7 02/12] cpus: stop vm in suspended runstate
Posted by Peter Xu 11 months, 3 weeks ago
On Wed, Dec 06, 2023 at 10:09:32PM +0100, Philippe Mathieu-Daudé wrote:
> What about runstate_is_vcpu_clock_ticking() then?

The problem is vCPU clock ticks can only be one part of "VM is running"
state (no matter whether vCPUs are running or not).  I'm even thinking
whether cpu_enable_ticks() should one day be put into a vm state change
notifier instead to be even clearer that it's not special (I hope I didn't
overlook its specialty..).

Thanks,

-- 
Peter Xu