[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v6 1/2] qmp: adding 'wakeup-suspend-support' in
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v6 1/2] qmp: adding 'wakeup-suspend-support' in query-target |
Date: |
Tue, 15 May 2018 17:45:54 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/25.3 (gnu/linux) |
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.
> diff --git a/vl.c b/vl.c
> index 3b39bbd7a8..ab6bd7e090 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1832,11 +1832,32 @@ void qemu_system_wakeup_enable(WakeupReason reason,
> bool enabled)
> }
> }
>
> +/* The existence of a wake-up notifier is being checked in the function
Please wing both ends of this comment:
/*
* The existence of a wake-up notifier is being checked in the function
> + * qemu_wakeup_notifier_is_empty and it's used in the logic of the
> + * wakeup-suspend-support flag of QMP 'query-target' command. The idea
> + * of this flag is to indicate whether the guest supports wake-up from
> + * suspend (via system_wakeup QMP/HMP call for example), warning the user
> + * that the guest can't handle both wake-up from suspend and the suspend
> + * itself via QGA guest-suspend-ram and guest-suspend-hybrid (if it
> + * can't wake up, it can't be suspended safely).
> + *
> + * An assumption is made by the wakeup-suspend-support flag that only the
> + * guests that can go to RUN_STATE_SUSPENDED and wake up properly would
> + * be interested in this wakeup_notifier. Adding a wakeup_notifier for
> + * any other reason will break the logic of the wakeup-suspend-support
> + * flag and can lead to user/management confusion about the suspend/wake-up
> + * support of the guest.
> + */
The assumption is a bit of a hack, but I don't have better ideas.
Thanks for commenting it clearly. However, ...
> 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;
... I'd recommend to rename qemu_wakeup_notifier_is_empty() to
qemu_wakeup_suspend_support(), because then the hack is a bit more
isolated. With the current name, it spills into qmp_query_target(),
which doesn't have such a helpful comment.