[V3,01/10] vl: start on wakeup request

Commit Message

Steven Sistare Aug. 14, 2023, 6:54 p.m. UTC

If qemu starts and loads a VM in the suspended state, then a later wakeup
request directly sets the state to running.  This skips vm_start() and its
initialization steps, which is fatal for the guest.  See
qemu_system_wakeup_request(), and qemu_system_wakeup() in
main_loop_should_exit().

Remember if vm_start has been called.  If not, then call vm_start from
qemu_system_wakeup_request.

Signed-off-by: Steve Sistare <[e-mail]>
Reviewed-by: Fabiano Rosas <[e-mail]>
---
 include/sysemu/runstate.h |  1 +
 softmmu/cpus.c            | 12 ++++++++++++
 softmmu/runstate.c        |  2 +-
 3 files changed, 14 insertions(+), 1 deletion(-)

Comments

Peter Xu Aug. 17, 2023, 6:27 p.m. UTC | #1

On Mon, Aug 14, 2023 at 11:54:27AM -0700, Steve Sistare wrote:
> +void vm_wakeup(void)
> +{
> +    if (!vm_started) {
> +        vm_start();

(irrelevant of the global var that I wanted to remove..)

Calling vm_start() is wrong here, IMHO.

I think we need to notify everyone on the wakeup before really waking up
the vcpus:

        notifier_list_notify(&wakeup_notifiers, &wakeup_reason);

There's resume_all_vcpus() after that.  I don't know the side effect of
resuming vcpus without such notifications, at least some acpi fields do not
seem to be updated so the vcpu can see stale values (acpi_notify_wakeup()).

> +    } else {
> +        runstate_set(RUN_STATE_RUNNING);
>      }
>  }

Steven Sistare Aug. 24, 2023, 8:54 p.m. UTC | #2

On 8/17/2023 2:27 PM, Peter Xu wrote:
> On Mon, Aug 14, 2023 at 11:54:27AM -0700, Steve Sistare wrote:
>> +void vm_wakeup(void)
>> +{
>> +    if (!vm_started) {
>> +        vm_start();
> 
> (irrelevant of the global var that I wanted to remove..)
> 
> Calling vm_start() is wrong here, IMHO.
> 
> I think we need to notify everyone on the wakeup before really waking up
> the vcpus:
> 
>         notifier_list_notify(&wakeup_notifiers, &wakeup_reason);
> 
> There's resume_all_vcpus() after that.  I don't know the side effect of
> resuming vcpus without such notifications, at least some acpi fields do not
> seem to be updated so the vcpu can see stale values (acpi_notify_wakeup()).

Agreed, good catch.

- Steve

>> +    } else {
>> +        runstate_set(RUN_STATE_RUNNING);
>>      }
>>  }
>

Patch

diff --git a/include/sysemu/runstate.h b/include/sysemu/runstate.h
index 7beb29c..42ddf83 100644
--- a/include/sysemu/runstate.h
+++ b/include/sysemu/runstate.h
@@ -34,6 +34,7 @@  static inline bool shutdown_caused_by_guest(ShutdownCause cause)
 }
 
 void vm_start(void);
+void vm_wakeup(void);
 
 /**
  * vm_prepare_start: Prepare for starting/resuming the VM
diff --git a/softmmu/cpus.c b/softmmu/cpus.c
index fed20ff..fa9e5ba 100644
--- a/softmmu/cpus.c
+++ b/softmmu/cpus.c
@@ -66,6 +66,7 @@ 
 #endif /* CONFIG_LINUX */
 
 static QemuMutex qemu_global_mutex;
+static bool vm_started;
 
 /*
  * The chosen accelerator is supposed to register this.
@@ -264,6 +265,7 @@  static int do_vm_stop(RunState state, bool send_stop)
         if (send_stop) {
             qapi_event_send_stop();
         }
+        vm_started = false;
     }
 
     bdrv_drain_all();
@@ -722,6 +724,16 @@  void vm_start(void)
 {
     if (!vm_prepare_start(false)) {
         resume_all_vcpus();
+        vm_started = true;
+    }
+}
+
+void vm_wakeup(void)
+{
+    if (!vm_started) {
+        vm_start();
+    } else {
+        runstate_set(RUN_STATE_RUNNING);
     }
 }
 
diff --git a/softmmu/runstate.c b/softmmu/runstate.c
index f3bd862..95c6ae7 100644
--- a/softmmu/runstate.c
+++ b/softmmu/runstate.c
@@ -580,7 +580,7 @@  void qemu_system_wakeup_request(WakeupReason reason, Error **errp)
     if (!(wakeup_reason_mask & (1 << reason))) {
         return;
     }
-    runstate_set(RUN_STATE_RUNNING);
+    vm_wakeup();
     wakeup_reason = reason;
     qemu_notify_event();
 }