Re: [PATCH V7 05/29] vl: start on wakeup request

[Steven Sistare]

On 2/24/2022 1:51 PM, Dr. David Alan Gilbert wrote:
> * Steve Sistare (steven.sistare@oracle.com) wrote:
>> If qemu starts and loads a VM in the suspended state, then a later wakeup
>> request will set the state to running, which is not sufficient to initialize
>> the vm, as vm_start was never called during this invocation of qemu.  See
>> qemu_system_wakeup_request().
>>
>> Define the start_on_wakeup_requested() hook to cause vm_start() to be called
>> when processing the wakeup request.
>>
>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>> ---
>>  include/sysemu/runstate.h |  1 +
>>  softmmu/runstate.c        | 17 ++++++++++++++++-
>>  2 files changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/sysemu/runstate.h b/include/sysemu/runstate.h
>> index a535691..b655c7b 100644
>> --- a/include/sysemu/runstate.h
>> +++ b/include/sysemu/runstate.h
>> @@ -51,6 +51,7 @@ void qemu_system_reset_request(ShutdownCause reason);
>>  void qemu_system_suspend_request(void);
>>  void qemu_register_suspend_notifier(Notifier *notifier);
>>  bool qemu_wakeup_suspend_enabled(void);
>> +void qemu_system_start_on_wakeup_request(void);
>>  void qemu_system_wakeup_request(WakeupReason reason, Error **errp);
>>  void qemu_system_wakeup_enable(WakeupReason reason, bool enabled);
>>  void qemu_register_wakeup_notifier(Notifier *notifier);
>> diff --git a/softmmu/runstate.c b/softmmu/runstate.c
>> index 10d9b73..3d344c9 100644
>> --- a/softmmu/runstate.c
>> +++ b/softmmu/runstate.c
>> @@ -115,6 +115,8 @@ static const RunStateTransition 
>> runstate_transitions_def[] = {
>>      { RUN_STATE_PRELAUNCH, RUN_STATE_RUNNING },
>>      { RUN_STATE_PRELAUNCH, RUN_STATE_FINISH_MIGRATE },
>>      { RUN_STATE_PRELAUNCH, RUN_STATE_INMIGRATE },
>> +    { RUN_STATE_PRELAUNCH, RUN_STATE_SUSPENDED },
>> +    { RUN_STATE_PRELAUNCH, RUN_STATE_PAUSED },
> 
> This seems separate? 

The RUN_STATE_SUSPENDED line is required for start on wake to work after qemu 
restarts.

> Is this the bit that allows you to load the VM into suspended?

Yes.

> But I note you're allowing PAUSED or SUSPENDED here, but the wake up
> code only handles suspended - is that expected?

The RUN_STATE_PAUSED line does not belong here. I will move it to  "cpr: reboot 
mode".

- Steve

>>      { RUN_STATE_FINISH_MIGRATE, RUN_STATE_RUNNING },
>>      { RUN_STATE_FINISH_MIGRATE, RUN_STATE_PAUSED },
>> @@ -335,6 +337,7 @@ void vm_state_notify(bool running, RunState state)
>>      }
>>  }
>>  
>> +static bool start_on_wakeup_requested;
>>  static ShutdownCause reset_requested;
>>  static ShutdownCause shutdown_requested;
>>  static int shutdown_signal;
>> @@ -562,6 +565,11 @@ void qemu_register_suspend_notifier(Notifier *notifier)
>>      notifier_list_add(&suspend_notifiers, notifier);
>>  }
>>  
>> +void qemu_system_start_on_wakeup_request(void)
>> +{
>> +    start_on_wakeup_requested = true;
>> +}
> 
> Markus: Is this OK, or should this actually be another runstate
> (PRELAUNCH_SUSPENDED??? or the like??) - is there an interaction here
> with the commandline change ideas for a build-the-guest at runtime?
> 
> Dave
> 
>>  void qemu_system_wakeup_request(WakeupReason reason, Error **errp)
>>  {
>>      trace_system_wakeup_request(reason);
>> @@ -574,7 +582,14 @@ void qemu_system_wakeup_request(WakeupReason reason, 
>> Error **errp)
>>      if (!(wakeup_reason_mask & (1 << reason))) {
>>          return;
>>      }
>> -    runstate_set(RUN_STATE_RUNNING);
>> +
>> +    if (start_on_wakeup_requested) {
>> +        start_on_wakeup_requested = false;
>> +        vm_start();
>> +    } else {
>> +        runstate_set(RUN_STATE_RUNNING);
>> +    }
>> +
>>      wakeup_reason = reason;
>>      qemu_notify_event();
>>  }
>> -- 
>> 1.8.3.1
>>
>>