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

[Daniel Henrique Barboza <danielhb413@gmail.com>]

On 09/05/2018 09:21 PM, Michael Roth wrote:
> 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 <bala24@linux.vnet.ibm.com>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
>>   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?

I like the idea of qemu_register_wakeup_support(). This avoids
overloading the existing wakeup_notifiers with an extra assumption
like I am doing up there. New implementations will need to call
this register function to let the new API aware of the support, but
I don't see it as a huge deal.

>
> 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
>>