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

[Daniel Henrique Barboza]

On 01/04/2018 08:00 PM, Michael Roth wrote:
> Quoting Daniel Henrique Barboza (2018-01-03 05:54:54)
> > 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).
> >
> > Signed-off-by: Daniel Henrique Barboza <address@hidden>
> Looks good to me, but I'm a little concerned that someone might
> register a wakeup notifier in the future for some reason other than
> actually supporting wakeup (perhaps as an informational thing, or as
> as part of some change to general lifecycle event tracking). I'm not
> sure it's currently worth adding an explicit flag that users need to
> set, but maybe at least some comments around
> qemu_register_wakeup_notifier() noting the potential side-effects of
> doing such a thing would be warranted.

Noted. I'll add a disclaimer in qemu_register_wakeup_notifier to
inform that the presence/absence of a notifier is being used to
determine whether an architecture supports wake up from suspend. That
way we can at least inform what will happen if someone adds
a new notifier without including the proper support.


Thanks,

Daniel
> > ---
> >   arch_init.c             | 1 +
> >   include/sysemu/sysemu.h | 1 +
> >   qapi-schema.json        | 4 +++-
> >   vl.c                    | 5 +++++
> >   4 files changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch_init.c b/arch_init.c
> > index a0b8ed6167..9c5c519d9d 100644
> > --- a/arch_init.c
> > +++ b/arch_init.c
> > @@ -109,6 +109,7 @@ TargetInfo *qmp_query_target(Error **errp)
> >       TargetInfo *info = g_malloc0(sizeof(*info));
> >
> >       info->arch = g_strdup(TARGET_NAME);
> > +    info->wakeup_suspend_support = !qemu_wakeup_notifier_is_empty();
> >
> >       return info;
> >   }
> > diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> > index 31612caf10..b15374e0b8 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-schema.json b/qapi-schema.json
> > index 5c06745c79..2f1c08fde7 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -2388,11 +2388,13 @@
> >   # Information describing the QEMU target.
> >   #
> >   # @arch: the target architecture (eg "x86_64", "i386", etc)
> > +# @wakeup-suspend-support: true if the target supports wake up from
> > +#                          suspend (since 2.12)
> >   #
> >   # Since: 1.2.0
> >   ##
> >   { 'struct': 'TargetInfo',
> > -  'data': { 'arch': 'str' } }
> > +  'data': { 'arch': 'str', 'wakeup-suspend-support': 'bool' } }
> >
> >   ##
> >   # @query-target:
> > diff --git a/vl.c b/vl.c
> > index d3a5c5d021..871c1f9bd1 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -1844,6 +1844,11 @@ void qemu_register_wakeup_notifier(Notifier *notifier)
> >       notifier_list_add(&wakeup_notifiers, notifier);
> >   }
> >
> > +bool qemu_wakeup_notifier_is_empty(void)
> > +{
> > +    return QLIST_EMPTY(&wakeup_notifiers.notifiers);
> > +}
> > +
> >   void qemu_system_killed(int signal, pid_t pid)
> >   {
> >       shutdown_signal = signal;
> > --
> > 2.13.6