Re: [Qemu-devel] [PATCH v6 1/2] qmp: adding 'wakeup-suspend-support' in query-target

[Markus Armbruster]

Daniel Henrique Barboza <address@hidden> writes:

> On 05/15/2018 12:45 PM, Markus Armbruster wrote:
>> Daniel Henrique Barboza <address@hidden> writes:
>>
>>> When issuing the qmp/hmp 'system_wakeup' command, what happens in a
>>> nutshell is:
>>>
>>> - qmp_system_wakeup_request set runstate to RUNNING, sets a wakeup_reason
>>> and notify the event
>>> - in the main_loop, all vcpus are paused, a system reset is issued, all
>>> subscribers of wakeup_notifiers receives a notification, vcpus are then
>>> resumed and the wake up QAPI event is fired
>>>
>>> Note that this procedure alone doesn't ensure that the guest will awake
>>> from SUSPENDED state - the subscribers of the wake up event must take
>>> action to resume the guest, otherwise the guest will simply reboot.
>>>
>>> At this moment there are only two subscribers of the wake up event: one
>>> in hw/acpi/core.c and another one in hw/i386/xen/xen-hvm.c. This means
>>> that system_wakeup does not work as intended with other architectures.
>>>
>>> However, only the presence of 'system_wakeup' is required for QGA to
>>> support 'guest-suspend-ram' and 'guest-suspend-hybrid' at this moment.
>>> This means that the user/management will expect to suspend the guest using
>>> one of those suspend commands and then resume execution using system_wakeup,
>>> regardless of the support offered in system_wakeup in the first place.
>>>
>>> This patch adds a new flag called 'wakeup-suspend-support' in TargetInfo
>>> that allows the caller to query if the guest supports wake up from
>>> suspend via system_wakeup. It goes over the subscribers of the wake up
>>> event and, if it's empty, it assumes that the guest does not support
>>> wake up from suspend (and thus, pm-suspend itself).
>>>
>>> This is the expected output of query-target when running a x86 guest:
>>>
>>> {"execute" : "query-target"}
>>> {"return": {"arch": "x86_64", "wakeup-suspend-support": true}}
>>>
>>> This is the output when running a pseries guest:
>>>
>>> {"execute" : "query-target"}
>>> {"return": {"arch": "ppc64", "wakeup-suspend-support": false}}
>>>
>>> Given that the TargetInfo structure is read-only, adding a new flag to
>>> it is backwards compatible. There is no need to deprecate the old
>>> TargetInfo format.
>>>
>>> With this extra tool, management can avoid situations where a guest
>>> that does not have proper suspend/wake capabilities ends up in
>>> inconsistent state (e.g.
>>> https://github.com/open-power-host-os/qemu/issues/31).
>>>
>>> Reported-by: Balamuruhan S <address@hidden>
>>> Signed-off-by: Daniel Henrique Barboza <address@hidden>
>>> ---
>>>   arch_init.c             |  1 +
>>>   include/sysemu/sysemu.h |  1 +
>>>   qapi/misc.json          |  4 +++-
>>>   vl.c                    | 21 +++++++++++++++++++++
>>>   4 files changed, 26 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch_init.c b/arch_init.c
>>> index 9597218ced..67bdf27528 100644
>>> --- a/arch_init.c
>>> +++ b/arch_init.c
>>> @@ -115,6 +115,7 @@ TargetInfo *qmp_query_target(Error **errp)
>>>         info->arch = qapi_enum_parse(&SysEmuTarget_lookup,
>>> TARGET_NAME, -1,
>>>                                    &error_abort);
>>> +    info->wakeup_suspend_support = !qemu_wakeup_notifier_is_empty();
>> Huh?  Hmm, see "hack" below.
>>
>>>         return info;
>>>   }
>>> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
>>> index 544ab77a2b..fbe2a3373e 100644
>>> --- a/include/sysemu/sysemu.h
>>> +++ b/include/sysemu/sysemu.h
>>> @@ -69,6 +69,7 @@ typedef enum WakeupReason {
>>>   void qemu_system_reset_request(ShutdownCause reason);
>>>   void qemu_system_suspend_request(void);
>>>   void qemu_register_suspend_notifier(Notifier *notifier);
>>> +bool qemu_wakeup_notifier_is_empty(void);
>>>   void qemu_system_wakeup_request(WakeupReason reason);
>>>   void qemu_system_wakeup_enable(WakeupReason reason, bool enabled);
>>>   void qemu_register_wakeup_notifier(Notifier *notifier);
>>> diff --git a/qapi/misc.json b/qapi/misc.json
>>> index f5988cc0b5..a385d897ae 100644
>>> --- a/qapi/misc.json
>>> +++ b/qapi/misc.json
>>> @@ -2484,11 +2484,13 @@
>>>   # Information describing the QEMU target.
>>>   #
>>>   # @arch: the target architecture
>>> +# @wakeup-suspend-support: true if the target supports wake up from
>>> +#                          suspend (since 2.13)
>>>   #
>>>   # Since: 1.2.0
>>>   ##
>>>   { 'struct': 'TargetInfo',
>>> -  'data': { 'arch': 'SysEmuTarget' } }
>>> +  'data': { 'arch': 'SysEmuTarget', 'wakeup-suspend-support': 'bool' } }
>>>     ##
>>>   # @query-target:
>> Does the documentation of system_wakeup need fixing?
>>
>>     ##
>>     # @system_wakeup:
>>     #
>>     # Wakeup guest from suspend.  Does nothing in case the guest isn't 
>> suspended.
>>     #
>>     # Since:  1.1
>>     #
>>     # Returns:  nothing.
>>     #
>>     # Example:
>>     #
>>     # -> { "execute": "system_wakeup" }
>>     # <- { "return": {} }
>>     #
>>     ##
>>     { 'command': 'system_wakeup' }
>>
>> I figure we better explain right here what the command does, both for
>> wakeup-suspend-support: false and true.
>
> Hmm, I've re-sent a patch that changes a bit the behavior of system_wakeup
> yesterday. The command should now fail with an error if the VM isn't in
> SUSPENDED state. However, I failed to update this documentation
> in that patch.
>
> What if I resend that system_wakeup patch with the updated
> documentation such
> as:
>
>
>    ##
>    # @system_wakeup:
>    #
>    # Wakeup guest from suspend.  Throws an error in case the guest isn't 
> suspended.
>    #
>    # Since:  1.1
>    #
>    # Returns:  nothing if succeed
>    #
>
>
> And then I believe we don't need to update this doc again - if the
> guest isn't suspended,
>  system_wakeup will still fire up an error.

What about a guest that is suspended under a QEMU that isn't capable of
waking it?

Shouldn't system_wakeup's documentation point to query-target's
wakeup-suspend-support?

> In fact, I could have added that patch in this series too since it
> kind of relates with these
> changes as well. Let me know if you think it helps and I'll respin it
> together with this
> series.

Respinning in a single series is the simplest way to ensure the patches
get committed in a sane order.  I'd appreciate that.

[...]