Re: [Qemu-devel] [PATCH v8 1/3] qmp: query-current-machine with wakeup-suspend-support

[Michael Roth]

Quoting Daniel Henrique Barboza (2018-07-05 15:08:11)
> 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 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 creates a new API called query-current-machine [1], that holds
> a new flag called 'wakeup-suspend-support', which indicates if the guest
> supports wake up from suspend via system_wakeup. The guest is considered
> to implement wake-up support if:
> 
> - there is at least one subscriber for the wake up event;
> 
> and, for the PC machine type:
> 
> - ACPI is enabled.
> 
> This is the expected output of query-current-machine when running a x86
> guest:
> 
> {"execute" : "query-current-machine"}
> {"return": {"wakeup-suspend-support": true}}
> 
> Running the same x86 guest, but with the --no-acpi option:
> 
> {"execute" : "query-current-machine"}
> {"return": {"wakeup-suspend-support": false}}
> 
> This is the output when running a pseries guest:
> 
> {"execute" : "query-current-machine"}
> {"return": {"wakeup-suspend-support": false}}
> 
> 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).
> 
> [1] the decision of creating the query-current-machine API is based
> on discussions in the QEMU mailing list where it was decided that
> query-target wasn't a proper place to store the wake-up flag, neither
> was query-machines because this isn't a static property of the
> machine object. This new API can then be used to store other
> dynamic machine properties that are scattered around the code
> ATM. More info at:
> https://lists.gnu.org/archive/html/qemu-devel/2018-05/msg04235.html
> 
> Reported-by: Balamuruhan S <address@hidden>
> Signed-off-by: Daniel Henrique Barboza <address@hidden>
> ---
>  qapi/misc.json | 24 ++++++++++++++++++++++++
>  vl.c           | 30 ++++++++++++++++++++++++++++++
>  2 files changed, 54 insertions(+)
> 
> diff --git a/qapi/misc.json b/qapi/misc.json
> index 29da7856e3..266f88cb09 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -2003,6 +2003,30 @@
>  ##
>  { 'command': 'query-machines', 'returns': ['MachineInfo'] }
> 
> +##
> +# @CurrentMachineParams:
> +#
> +# Information describing the running machine parameters.
> +#
> +# @wakeup-suspend-support: true if the target supports wake up from
> +#                          suspend
> +#
> +# Since: 3.0.0

3.1.0 now (hopefully :))

> +##
> +{ 'struct': 'CurrentMachineParams',
> +  'data': { 'wakeup-suspend-support': 'bool'} }
> +
> +##
> +# @query-current-machine:
> +#
> +# Return a list of parameters of the running machine.
> +#
> +# Returns: CurrentMachineParams
> +#
> +# Since: 3.0.0
> +##
> +{ 'command': 'query-current-machine', 'returns': 'CurrentMachineParams' }
> +
>  ##
>  # @CpuDefinitionInfo:
>  #
> diff --git a/vl.c b/vl.c
> index ef6cfcec40..96ad448d57 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1747,11 +1747,41 @@ void qemu_system_wakeup_enable(WakeupReason reason, 
> bool enabled)
>      }
>  }
> 
> +/*
> + * The existence of a wake-up notifier is being checked in the function
> + * qemu_wakeup_suspend_support and it's used in the logic of the
> + * wakeup-suspend-support flag of QMP 'query-current-machine' 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.
> + */
>  void qemu_register_wakeup_notifier(Notifier *notifier)
>  {
>      notifier_list_add(&wakeup_notifiers, notifier);
>  }
> 
> +static bool qemu_wakeup_suspend_support(void)
> +{
> +    return !QLIST_EMPTY(&wakeup_notifiers.notifiers) && acpi_enabled;
> +}

I think this would need to be re-worked for any machine types that
eventually implement wakeup even though acpi_enabled == false. It was
maybe a bit hacky, but kind of nice that registering a wakeup notifier
implicitly registered support for wakeup, but now that we already have
these sorts of edge cases to consider maybe it's best to just add an
explicit qemu_register_wakeup_support() that just sets a flag in
vl.c and add call that at the appropriate locations for known supported
configurations. Maybe one call in acpi_pm1_cnt_init() might be enough?

Though I'm noticing mips malta board also calls that, would be curious
if that actually supports wake-from-suspend. If not, maybe the callers
are where we should declare support (ich9_pm_init and piix4_pm_realize
currently)?

> +
> +CurrentMachineParams *qmp_query_current_machine(Error **errp)
> +{
> +    CurrentMachineParams *params = g_malloc0(sizeof(*params));
> +    params->wakeup_suspend_support = qemu_wakeup_suspend_support();
> +
> +    return params;
> +}
> +
>  void qemu_system_killed(int signal, pid_t pid)
>  {
>      shutdown_signal = signal;
> -- 
> 2.17.1
>