Re: [Qemu-devel] [PATCH v9 0/3] wakeup-from-suspend and system_wakeup changes

Re: [Qemu-devel] [PATCH v9 0/3] wakeup-from-suspend and system_wakeup changes

[Daniel Henrique Barboza]

ping

On 9/18/18 3:52 PM, Daniel Henrique Barboza wrote:
> changes in v9, all proposed by Mike Roth:
> - added a new 'qemu_register_wakeup_support' to be called by the wake-up
>    implementations to register the support in vl.c (patch 1)
> - changed versions from 3.0.0 to 3.1.0 (patch 1)
> - added back the 'qemu_system_wakeup_request' call that was removed by
> mistake in the previous version (patch 3)
> - Previous series link:
> https://lists.gnu.org/archive/html/qemu-devel/2018-07/msg01675.html
>
>
> Daniel Henrique Barboza (3):
>    qmp: query-current-machine with wakeup-suspend-support
>    qga: update guest-suspend-ram and guest-suspend-hybrid descriptions
>    qmp hmp: Make system_wakeup check wake-up support and run state
>
>   hmp.c                   |  5 ++++-
>   hw/acpi/core.c          |  1 +
>   include/sysemu/sysemu.h |  2 ++
>   qapi/misc.json          | 29 ++++++++++++++++++++++++++++-
>   qga/qapi-schema.json    | 12 ++++++------
>   qmp.c                   | 10 ++++++++++
>   vl.c                    | 19 +++++++++++++++++++
>   7 files changed, 70 insertions(+), 8 deletions(-)
>

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

[Markus Armbruster]

Daniel Henrique Barboza <danielhb413@gmail.com> 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, only the ACPI machines via acpi_pm1_cnt_init has wake-up
> from suspend support.
>
> 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' that indicates if the guest
> supports wake up from suspend via system_wakeup. The machine is considered
> to implement wake-up support if a call to a new 'qemu_register_wakeup_support'
> is made during its init, as it is now being done inside acpi_pm1_cnt_init.
> This allows for any other machine type to declare wake-up support regardless
> of ACPI state or wakeup_notifiers subscription, making easier for
> newer implementations that might have its own mechanisms in the future.
>
> 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>
> ---
>  hw/acpi/core.c          |  1 +
>  include/sysemu/sysemu.h |  1 +
>  qapi/misc.json          | 24 ++++++++++++++++++++++++
>  vl.c                    | 19 +++++++++++++++++++
>  4 files changed, 45 insertions(+)
>
> diff --git a/hw/acpi/core.c b/hw/acpi/core.c
> index b8d39012cd..2dea893245 100644
> --- a/hw/acpi/core.c
> +++ b/hw/acpi/core.c
> @@ -617,6 +617,7 @@ void acpi_pm1_cnt_init(ACPIREGS *ar, MemoryRegion *parent,
>      ar->pm1.cnt.s4_val = s4_val;
>      ar->wakeup.notify = acpi_notify_wakeup;
>      qemu_register_wakeup_notifier(&ar->wakeup);
> +    qemu_register_wakeup_support();
>      memory_region_init_io(&ar->pm1.cnt.io, memory_region_owner(parent),
>                            &acpi_pm_cnt_ops, ar, "acpi-cnt", 2);
>      memory_region_add_subregion(parent, 4, &ar->pm1.cnt.io);

We might've discussed this before, not sure...  There's a second
qemu_register_wakeup_notifier() in xen-hvm.c.  By not adding
qemu_register_wakeup_support() there, you implicitly state that it
doesn't provide wakeup support.  I have no idea whether that's true.  If
it is, then I think the commit message should explain it.  We might even
want a comment.  Please advise.

> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index 8d6095d98b..0446adacc6 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -77,6 +77,7 @@ void qemu_register_suspend_notifier(Notifier *notifier);
>  void qemu_system_wakeup_request(WakeupReason reason);
>  void qemu_system_wakeup_enable(WakeupReason reason, bool enabled);
>  void qemu_register_wakeup_notifier(Notifier *notifier);
> +void qemu_register_wakeup_support(void);
>  void qemu_system_shutdown_request(ShutdownCause reason);
>  void qemu_system_powerdown_request(void);
>  void qemu_register_powerdown_notifier(Notifier *notifier);
> diff --git a/qapi/misc.json b/qapi/misc.json
> index d450cfef21..b616d385a4 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

s/target/machine/

> +#                          suspend
> +#
> +# Since: 3.1.0
> +##
> +{ 'struct': 'CurrentMachineParams',
> +  'data': { 'wakeup-suspend-support': 'bool'} }
> +
> +##
> +# @query-current-machine:
> +#
> +# Return a list of parameters of the running machine.

"a list of" suggests it returns a QAPI list.  Let's say "Return
information on the current virtual machine".

> +#
> +# Returns: CurrentMachineParams
> +#
> +# Since: 3.1.0
> +##
> +{ 'command': 'query-current-machine', 'returns': 'CurrentMachineParams' }
> +
>  ##
>  # @CpuDefinitionInfo:
>  #
> diff --git a/vl.c b/vl.c
> index 5ba06adf78..f78947f69c 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -184,6 +184,7 @@ bool boot_strict;
>  uint8_t *boot_splash_filedata;
>  size_t boot_splash_filedata_size;
>  uint8_t qemu_extra_params_fw[2];
> +bool wakeup_suspend_enabled;
>  
>  int icount_align_option;
>  
> @@ -1753,6 +1754,24 @@ void qemu_register_wakeup_notifier(Notifier *notifier)
>      notifier_list_add(&wakeup_notifiers, notifier);
>  }
>  
> +void qemu_register_wakeup_support(void)
> +{
> +    wakeup_suspend_enabled = true;
> +}
> +
> +static bool qemu_wakeup_suspend_enabled(void)
> +{
> +    return wakeup_suspend_enabled;
> +}
> +
> +CurrentMachineParams *qmp_query_current_machine(Error **errp)
> +{
> +    CurrentMachineParams *params = g_malloc0(sizeof(*params));
> +    params->wakeup_suspend_support = qemu_wakeup_suspend_enabled();
> +
> +    return params;
> +}
> +
>  void qemu_system_killed(int signal, pid_t pid)
>  {
>      shutdown_signal = signal;

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

[Daniel Henrique Barboza]

On 10/11/18 4:38 AM, Markus Armbruster wrote:
> Daniel Henrique Barboza <danielhb413@gmail.com> 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, only the ACPI machines via acpi_pm1_cnt_init has wake-up
>> from suspend support.
>>
>> 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' that indicates if the guest
>> supports wake up from suspend via system_wakeup. The machine is considered
>> to implement wake-up support if a call to a new 'qemu_register_wakeup_support'
>> is made during its init, as it is now being done inside acpi_pm1_cnt_init.
>> This allows for any other machine type to declare wake-up support regardless
>> of ACPI state or wakeup_notifiers subscription, making easier for
>> newer implementations that might have its own mechanisms in the future.
>>
>> 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>
>> ---
>>   hw/acpi/core.c          |  1 +
>>   include/sysemu/sysemu.h |  1 +
>>   qapi/misc.json          | 24 ++++++++++++++++++++++++
>>   vl.c                    | 19 +++++++++++++++++++
>>   4 files changed, 45 insertions(+)
>>
>> diff --git a/hw/acpi/core.c b/hw/acpi/core.c
>> index b8d39012cd..2dea893245 100644
>> --- a/hw/acpi/core.c
>> +++ b/hw/acpi/core.c
>> @@ -617,6 +617,7 @@ void acpi_pm1_cnt_init(ACPIREGS *ar, MemoryRegion *parent,
>>       ar->pm1.cnt.s4_val = s4_val;
>>       ar->wakeup.notify = acpi_notify_wakeup;
>>       qemu_register_wakeup_notifier(&ar->wakeup);
>> +    qemu_register_wakeup_support();
>>       memory_region_init_io(&ar->pm1.cnt.io, memory_region_owner(parent),
>>                             &acpi_pm_cnt_ops, ar, "acpi-cnt", 2);
>>       memory_region_add_subregion(parent, 4, &ar->pm1.cnt.io);
> We might've discussed this before, not sure...  There's a second
> qemu_register_wakeup_notifier() in xen-hvm.c.  By not adding
> qemu_register_wakeup_support() there, you implicitly state that it
> doesn't provide wakeup support.  I have no idea whether that's true.  If
> it is, then I think the commit message should explain it.  We might even
> want a comment.  Please advise.

Not adding the wakeup notifier at xen-hvm.c was a mistake. I am under
the assumption that everyone that register a notifier is implementing
wakeup support, which seems reasonable to me (why else would you
want to listen to that event?). Simply put, I failed to check on all 
callers of
qemu_register_wakeup_notifier().

Thanks for noticing it.


>
>> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
>> index 8d6095d98b..0446adacc6 100644
>> --- a/include/sysemu/sysemu.h
>> +++ b/include/sysemu/sysemu.h
>> @@ -77,6 +77,7 @@ void qemu_register_suspend_notifier(Notifier *notifier);
>>   void qemu_system_wakeup_request(WakeupReason reason);
>>   void qemu_system_wakeup_enable(WakeupReason reason, bool enabled);
>>   void qemu_register_wakeup_notifier(Notifier *notifier);
>> +void qemu_register_wakeup_support(void);
>>   void qemu_system_shutdown_request(ShutdownCause reason);
>>   void qemu_system_powerdown_request(void);
>>   void qemu_register_powerdown_notifier(Notifier *notifier);
>> diff --git a/qapi/misc.json b/qapi/misc.json
>> index d450cfef21..b616d385a4 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
> s/target/machine/

ok!

>
>> +#                          suspend
>> +#
>> +# Since: 3.1.0
>> +##
>> +{ 'struct': 'CurrentMachineParams',
>> +  'data': { 'wakeup-suspend-support': 'bool'} }
>> +
>> +##
>> +# @query-current-machine:
>> +#
>> +# Return a list of parameters of the running machine.
> "a list of" suggests it returns a QAPI list.  Let's say "Return
> information on the current virtual machine".

ok!

>
>> +#
>> +# Returns: CurrentMachineParams
>> +#
>> +# Since: 3.1.0
>> +##
>> +{ 'command': 'query-current-machine', 'returns': 'CurrentMachineParams' }
>> +
>>   ##
>>   # @CpuDefinitionInfo:
>>   #
>> diff --git a/vl.c b/vl.c
>> index 5ba06adf78..f78947f69c 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -184,6 +184,7 @@ bool boot_strict;
>>   uint8_t *boot_splash_filedata;
>>   size_t boot_splash_filedata_size;
>>   uint8_t qemu_extra_params_fw[2];
>> +bool wakeup_suspend_enabled;
>>   
>>   int icount_align_option;
>>   
>> @@ -1753,6 +1754,24 @@ void qemu_register_wakeup_notifier(Notifier *notifier)
>>       notifier_list_add(&wakeup_notifiers, notifier);
>>   }
>>   
>> +void qemu_register_wakeup_support(void)
>> +{
>> +    wakeup_suspend_enabled = true;
>> +}
>> +
>> +static bool qemu_wakeup_suspend_enabled(void)
>> +{
>> +    return wakeup_suspend_enabled;
>> +}
>> +
>> +CurrentMachineParams *qmp_query_current_machine(Error **errp)
>> +{
>> +    CurrentMachineParams *params = g_malloc0(sizeof(*params));
>> +    params->wakeup_suspend_support = qemu_wakeup_suspend_enabled();
>> +
>> +    return params;
>> +}
>> +
>>   void qemu_system_killed(int signal, pid_t pid)
>>   {
>>       shutdown_signal = signal;

Re: [Qemu-devel] [PATCH v9 2/3] qga: update guest-suspend-ram and guest-suspend-hybrid descriptions

[Markus Armbruster]

Daniel Henrique Barboza <danielhb413@gmail.com> writes:

> This patch updates the descriptions of 'guest-suspend-ram' and
> 'guest-suspend-hybrid' to mention that both commands relies now
> on the proper support for wake up from suspend, retrieved by the
> 'wakeup-suspend-support' attribute of the 'query-current-machine'
> QMP command.
>
> Reported-by: Balamuruhan S <bala24@linux.vnet.ibm.com>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>  qga/qapi-schema.json | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index dfbc4a5e32..7ae7e2502c 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -567,9 +567,9 @@
>  # For the best results it's strongly recommended to have the pm-utils
>  # package installed in the guest.
>  #
> -# IMPORTANT: guest-suspend-ram requires QEMU to support the 'system_wakeup'
> -# command.  Thus, it's *required* to query QEMU for the presence of the
> -# 'system_wakeup' command before issuing guest-suspend-ram.
> +# IMPORTANT: guest-suspend-ram requires working wakeup support in
> +# QEMU. You *must* check QMP command query-current-machine returns
> +# wakeup-suspend-support: true before issuing this command.
>  #
>  # This command does NOT return a response on success. There are two options
>  # to check for success:
> @@ -594,9 +594,9 @@
>  #
>  # This command requires the pm-utils package to be installed in the guest.
>  #
> -# IMPORTANT: guest-suspend-hybrid requires QEMU to support the 'system_wakeup'
> -# command.  Thus, it's *required* to query QEMU for the presence of the
> -# 'system_wakeup' command before issuing guest-suspend-hybrid.
> +# IMPORTANT: guest-suspend-hybrid requires working wakeup support in
> +# QEMU. You *must* check QMP command query-current-machine returns
> +# wakeup-suspend-support: true before issuing this command.
>  #
>  # This command does NOT return a response on success. There are two options
>  # to check for success:

The emphasis in "You *must* check" hints at all kinds of terrible
disasters if you don't, but then you're not pointing to any.

So, what can happen if you issue this command when query-current-machine
returns wakeup-suspend-support: false?

Re: [Qemu-devel] [PATCH v9 2/3] qga: update guest-suspend-ram and guest-suspend-hybrid descriptions

[Daniel Henrique Barboza]

On 10/11/18 4:40 AM, Markus Armbruster wrote:
> Daniel Henrique Barboza <danielhb413@gmail.com> writes:
>
>> This patch updates the descriptions of 'guest-suspend-ram' and
>> 'guest-suspend-hybrid' to mention that both commands relies now
>> on the proper support for wake up from suspend, retrieved by the
>> 'wakeup-suspend-support' attribute of the 'query-current-machine'
>> QMP command.
>>
>> Reported-by: Balamuruhan S <bala24@linux.vnet.ibm.com>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
>> ---
>>   qga/qapi-schema.json | 12 ++++++------
>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
>> index dfbc4a5e32..7ae7e2502c 100644
>> --- a/qga/qapi-schema.json
>> +++ b/qga/qapi-schema.json
>> @@ -567,9 +567,9 @@
>>   # For the best results it's strongly recommended to have the pm-utils
>>   # package installed in the guest.
>>   #
>> -# IMPORTANT: guest-suspend-ram requires QEMU to support the 'system_wakeup'
>> -# command.  Thus, it's *required* to query QEMU for the presence of the
>> -# 'system_wakeup' command before issuing guest-suspend-ram.
>> +# IMPORTANT: guest-suspend-ram requires working wakeup support in
>> +# QEMU. You *must* check QMP command query-current-machine returns
>> +# wakeup-suspend-support: true before issuing this command.
>>   #
>>   # This command does NOT return a response on success. There are two options
>>   # to check for success:
>> @@ -594,9 +594,9 @@
>>   #
>>   # This command requires the pm-utils package to be installed in the guest.
>>   #
>> -# IMPORTANT: guest-suspend-hybrid requires QEMU to support the 'system_wakeup'
>> -# command.  Thus, it's *required* to query QEMU for the presence of the
>> -# 'system_wakeup' command before issuing guest-suspend-hybrid.
>> +# IMPORTANT: guest-suspend-hybrid requires working wakeup support in
>> +# QEMU. You *must* check QMP command query-current-machine returns
>> +# wakeup-suspend-support: true before issuing this command.
>>   #
>>   # This command does NOT return a response on success. There are two options
>>   # to check for success:
> The emphasis in "You *must* check" hints at all kinds of terrible
> disasters if you don't, but then you're not pointing to any.
>
> So, what can happen if you issue this command when query-current-machine
> returns wakeup-suspend-support: false?

That's fair. If you don't check for flag when issuing 
guest-suspend-{ram/hybrid},
what happens is that QEMU will suspend the guest (QGA will suspend the
guest via systemctl, regardless of the guest arch) but system_wakeup 
will not
be able to wake the guest back up. You'll need to reboot the guest.

What  do you think of this new text?

+# IMPORTANT: guest-suspend-hybrid requires working wakeup support in
+# QEMU. You *must* check QMP command query-current-machine returns
+# wakeup-suspend-support: true before issuing this command. Failure in
+# doing so can result in a suspended guest that QEMU will not be able to
+# awaken, forcing the user to power cycle the guest to bring it back.

Re: [Qemu-devel] [PATCH v9 3/3] qmp hmp: Make system_wakeup check wake-up support and run state

[Markus Armbruster]

Daniel Henrique Barboza <danielhb413@gmail.com> writes:

> The qmp/hmp command 'system_wakeup' is simply a direct call to
> 'qemu_system_wakeup_request' from vl.c. This function verifies if
> runstate is SUSPENDED and if the wake up reason is valid before
> proceeding. However, no error or warning is thrown if any of those
> pre-requirements isn't met. There is no way for the caller to
> differentiate between a successful wakeup or an error state caused
> when trying to wake up a guest that wasn't suspended.
>
> This means that system_wakeup is silently failing, which can be
> considered a bug. Adding error handling isn't an API break in this
> case - applications that didn't check the result will remain broken,
> the ones that check it will have a chance to deal with it.
>
> Adding to that, the commit before previous created a new QMP API called
> query-current-machine, with a new flag called wakeup-suspend-support,
> that indicates if the guest has the capability of waking up from suspended
> state. Although such guest will never reach SUSPENDED state and erroring
> it out in this scenario would suffice, it is more informative for the user
> to differentiate between a failure because the guest isn't suspended versus
> a failure because the guest does not have support for wake up at all.
>
> All this considered, this patch changes qmp_system_wakeup to:
>
> - check if the guest has the capability to wake-up from suspend. If
> not, error out informing about the lack of support;
>
> - make the runstate verification before proceeding to call
> qemu_system_wakeup_request, firing up an error message if the user tries
> to wake up a machine that isn't in SUSPENDED state.
>
> After this patch, this is the output of system_wakeup in a guest that
> does not have wake-up from suspend support (ppc64):
>
> (qemu) system_wakeup
> wake-up from suspend is not supported by this guest
> (qemu)
>
> And this is the output of system_wakeup in a x86 guest that has the
> support but isn't suspended:
>
> (qemu) system_wakeup
> Unable to wake up: guest is not in suspended state
> (qemu)
>
> Reported-by: Balamuruhan S <bala24@linux.vnet.ibm.com>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>  hmp.c                   |  5 ++++-
>  include/sysemu/sysemu.h |  1 +
>  qapi/misc.json          |  5 ++++-
>  qmp.c                   | 10 ++++++++++
>  vl.c                    |  2 +-
>  5 files changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/hmp.c b/hmp.c
> index 4975fa56b0..e98c24782f 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1203,7 +1203,10 @@ void hmp_cont(Monitor *mon, const QDict *qdict)
>  
>  void hmp_system_wakeup(Monitor *mon, const QDict *qdict)
>  {
> -    qmp_system_wakeup(NULL);
> +    Error *err = NULL;
> +
> +    qmp_system_wakeup(&err);
> +    hmp_handle_error(mon, &err);
>  }
>  
>  void hmp_nmi(Monitor *mon, const QDict *qdict)
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index 0446adacc6..8d91c5ad5b 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -74,6 +74,7 @@ void qemu_exit_preconfig_request(void);
>  void qemu_system_reset_request(ShutdownCause reason);
>  void qemu_system_suspend_request(void);
>  void qemu_register_suspend_notifier(Notifier *notifier);
> +bool qemu_wakeup_suspend_enabled(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 b616d385a4..2f5995f45f 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -1230,7 +1230,10 @@
>  ##
>  # @system_wakeup:
>  #
> -# Wakeup guest from suspend.  Does nothing in case the guest isn't suspended.
> +# If the guest has wake-up from suspend support enabled
> +# (wakeup-suspend-support flag from query-current-machine), wakeup guest
> +# from suspend if the guest is in SUSPENDED state. Returns an error
> +# otherwise.

Perhaps a note that versions older than 3.1 neglected to report errors
would be helpful.

>  #
>  # Since:  1.1
>  #
> diff --git a/qmp.c b/qmp.c
> index e7c0a2fd60..44e619d1be 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -183,6 +183,16 @@ void qmp_cont(Error **errp)
>  
>  void qmp_system_wakeup(Error **errp)
>  {
> +    if (!qemu_wakeup_suspend_enabled()) {
> +        error_setg(errp,
> +                   "wake-up from suspend is not supported by this guest");
> +        return;
> +    }

Is this a new check?

If not, where is the condition checked before this patch?

> +    if (!runstate_check(RUN_STATE_SUSPENDED)) {
> +        error_setg(errp,
> +                   "Unable to wake up: guest is not in suspended state");
> +        return;
> +    }

Duplicates the check in qemu_system_wakeup_request().  Should that
function take an Error ** argument instead?

>      qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
>  }
>  
> diff --git a/vl.c b/vl.c
> index f78947f69c..35a33d61ba 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1759,7 +1759,7 @@ void qemu_register_wakeup_support(void)
>      wakeup_suspend_enabled = true;
>  }
>  
> -static bool qemu_wakeup_suspend_enabled(void)
> +bool qemu_wakeup_suspend_enabled(void)
>  {
>      return wakeup_suspend_enabled;
>  }

Re: [Qemu-devel] [PATCH v9 3/3] qmp hmp: Make system_wakeup check wake-up support and run state

[Daniel Henrique Barboza]

On 10/11/18 4:45 AM, Markus Armbruster wrote:
> Daniel Henrique Barboza <danielhb413@gmail.com> writes:
>
>> The qmp/hmp command 'system_wakeup' is simply a direct call to
>> 'qemu_system_wakeup_request' from vl.c. This function verifies if
>> runstate is SUSPENDED and if the wake up reason is valid before
>> proceeding. However, no error or warning is thrown if any of those
>> pre-requirements isn't met. There is no way for the caller to
>> differentiate between a successful wakeup or an error state caused
>> when trying to wake up a guest that wasn't suspended.
>>
>> This means that system_wakeup is silently failing, which can be
>> considered a bug. Adding error handling isn't an API break in this
>> case - applications that didn't check the result will remain broken,
>> the ones that check it will have a chance to deal with it.
>>
>> Adding to that, the commit before previous created a new QMP API called
>> query-current-machine, with a new flag called wakeup-suspend-support,
>> that indicates if the guest has the capability of waking up from suspended
>> state. Although such guest will never reach SUSPENDED state and erroring
>> it out in this scenario would suffice, it is more informative for the user
>> to differentiate between a failure because the guest isn't suspended versus
>> a failure because the guest does not have support for wake up at all.
>>
>> All this considered, this patch changes qmp_system_wakeup to:
>>
>> - check if the guest has the capability to wake-up from suspend. If
>> not, error out informing about the lack of support;
>>
>> - make the runstate verification before proceeding to call
>> qemu_system_wakeup_request, firing up an error message if the user tries
>> to wake up a machine that isn't in SUSPENDED state.
>>
>> After this patch, this is the output of system_wakeup in a guest that
>> does not have wake-up from suspend support (ppc64):
>>
>> (qemu) system_wakeup
>> wake-up from suspend is not supported by this guest
>> (qemu)
>>
>> And this is the output of system_wakeup in a x86 guest that has the
>> support but isn't suspended:
>>
>> (qemu) system_wakeup
>> Unable to wake up: guest is not in suspended state
>> (qemu)
>>
>> Reported-by: Balamuruhan S <bala24@linux.vnet.ibm.com>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
>>   hmp.c                   |  5 ++++-
>>   include/sysemu/sysemu.h |  1 +
>>   qapi/misc.json          |  5 ++++-
>>   qmp.c                   | 10 ++++++++++
>>   vl.c                    |  2 +-
>>   5 files changed, 20 insertions(+), 3 deletions(-)
>>
>> diff --git a/hmp.c b/hmp.c
>> index 4975fa56b0..e98c24782f 100644
>> --- a/hmp.c
>> +++ b/hmp.c
>> @@ -1203,7 +1203,10 @@ void hmp_cont(Monitor *mon, const QDict *qdict)
>>   
>>   void hmp_system_wakeup(Monitor *mon, const QDict *qdict)
>>   {
>> -    qmp_system_wakeup(NULL);
>> +    Error *err = NULL;
>> +
>> +    qmp_system_wakeup(&err);
>> +    hmp_handle_error(mon, &err);
>>   }
>>   
>>   void hmp_nmi(Monitor *mon, const QDict *qdict)
>> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
>> index 0446adacc6..8d91c5ad5b 100644
>> --- a/include/sysemu/sysemu.h
>> +++ b/include/sysemu/sysemu.h
>> @@ -74,6 +74,7 @@ void qemu_exit_preconfig_request(void);
>>   void qemu_system_reset_request(ShutdownCause reason);
>>   void qemu_system_suspend_request(void);
>>   void qemu_register_suspend_notifier(Notifier *notifier);
>> +bool qemu_wakeup_suspend_enabled(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 b616d385a4..2f5995f45f 100644
>> --- a/qapi/misc.json
>> +++ b/qapi/misc.json
>> @@ -1230,7 +1230,10 @@
>>   ##
>>   # @system_wakeup:
>>   #
>> -# Wakeup guest from suspend.  Does nothing in case the guest isn't suspended.
>> +# If the guest has wake-up from suspend support enabled
>> +# (wakeup-suspend-support flag from query-current-machine), wakeup guest
>> +# from suspend if the guest is in SUSPENDED state. Returns an error
>> +# otherwise.
> Perhaps a note that versions older than 3.1 neglected to report errors
> would be helpful.


Ok!

>
>>   #
>>   # Since:  1.1
>>   #
>> diff --git a/qmp.c b/qmp.c
>> index e7c0a2fd60..44e619d1be 100644
>> --- a/qmp.c
>> +++ b/qmp.c
>> @@ -183,6 +183,16 @@ void qmp_cont(Error **errp)
>>   
>>   void qmp_system_wakeup(Error **errp)
>>   {
>> +    if (!qemu_wakeup_suspend_enabled()) {
>> +        error_setg(errp,
>> +                   "wake-up from suspend is not supported by this guest");
>> +        return;
>> +    }
> Is this a new check?
>
> If not, where is the condition checked before this patch?

This is a new check that concerns only the QMP callers, thus I chose not to
bake it inside qemu_system_wakeup_request().


>
>> +    if (!runstate_check(RUN_STATE_SUSPENDED)) {
>> +        error_setg(errp,
>> +                   "Unable to wake up: guest is not in suspended state");
>> +        return;
>> +    }
> Duplicates the check in qemu_system_wakeup_request().  Should that
> function take an Error ** argument instead?

Yes, it is saner than adding a duplicated check. Shouldn't hurt
other callers as well. I'll respin the series with this change.

>
>>       qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
>>   }
>>   
>> diff --git a/vl.c b/vl.c
>> index f78947f69c..35a33d61ba 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -1759,7 +1759,7 @@ void qemu_register_wakeup_support(void)
>>       wakeup_suspend_enabled = true;
>>   }
>>   
>> -static bool qemu_wakeup_suspend_enabled(void)
>> +bool qemu_wakeup_suspend_enabled(void)
>>   {
>>       return wakeup_suspend_enabled;
>>   }