* Re: [Qemu-devel] [PATCH v9 0/3] wakeup-from-suspend and system_wakeup changes
[not found] <20180918185246.18109-1-danielhb413@gmail.com>
@ 2018-10-08 18:12 ` Daniel Henrique Barboza
[not found] ` <20180918185246.18109-2-danielhb413@gmail.com>
` (2 subsequent siblings)
3 siblings, 0 replies; 7+ messages in thread
From: Daniel Henrique Barboza @ 2018-10-08 18:12 UTC (permalink / raw)
To: qemu-devel; +Cc: mdroth, ehabkost, armbru, mst, imammedo
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(-)
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v9 1/3] qmp: query-current-machine with wakeup-suspend-support
[not found] ` <20180918185246.18109-2-danielhb413@gmail.com>
@ 2018-10-11 7:38 ` Markus Armbruster
2018-10-11 12:08 ` Daniel Henrique Barboza
0 siblings, 1 reply; 7+ messages in thread
From: Markus Armbruster @ 2018-10-11 7:38 UTC (permalink / raw)
To: Daniel Henrique Barboza
Cc: qemu-devel, ehabkost, mst, armbru, mdroth, imammedo
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;
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v9 2/3] qga: update guest-suspend-ram and guest-suspend-hybrid descriptions
[not found] ` <20180918185246.18109-3-danielhb413@gmail.com>
@ 2018-10-11 7:40 ` Markus Armbruster
2018-10-11 12:22 ` Daniel Henrique Barboza
0 siblings, 1 reply; 7+ messages in thread
From: Markus Armbruster @ 2018-10-11 7:40 UTC (permalink / raw)
To: Daniel Henrique Barboza; +Cc: qemu-devel, ehabkost, mst, mdroth, imammedo
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?
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v9 3/3] qmp hmp: Make system_wakeup check wake-up support and run state
[not found] ` <20180918185246.18109-4-danielhb413@gmail.com>
@ 2018-10-11 7:45 ` Markus Armbruster
2018-10-11 20:21 ` Daniel Henrique Barboza
0 siblings, 1 reply; 7+ messages in thread
From: Markus Armbruster @ 2018-10-11 7:45 UTC (permalink / raw)
To: Daniel Henrique Barboza
Cc: qemu-devel, ehabkost, mst, armbru, mdroth, imammedo
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;
> }
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v9 1/3] qmp: query-current-machine with wakeup-suspend-support
2018-10-11 7:38 ` [Qemu-devel] [PATCH v9 1/3] qmp: query-current-machine with wakeup-suspend-support Markus Armbruster
@ 2018-10-11 12:08 ` Daniel Henrique Barboza
0 siblings, 0 replies; 7+ messages in thread
From: Daniel Henrique Barboza @ 2018-10-11 12:08 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, ehabkost, mst, mdroth, imammedo
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;
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v9 2/3] qga: update guest-suspend-ram and guest-suspend-hybrid descriptions
2018-10-11 7:40 ` [Qemu-devel] [PATCH v9 2/3] qga: update guest-suspend-ram and guest-suspend-hybrid descriptions Markus Armbruster
@ 2018-10-11 12:22 ` Daniel Henrique Barboza
0 siblings, 0 replies; 7+ messages in thread
From: Daniel Henrique Barboza @ 2018-10-11 12:22 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, ehabkost, mst, mdroth, imammedo
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v9 3/3] qmp hmp: Make system_wakeup check wake-up support and run state
2018-10-11 7:45 ` [Qemu-devel] [PATCH v9 3/3] qmp hmp: Make system_wakeup check wake-up support and run state Markus Armbruster
@ 2018-10-11 20:21 ` Daniel Henrique Barboza
0 siblings, 0 replies; 7+ messages in thread
From: Daniel Henrique Barboza @ 2018-10-11 20:21 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, ehabkost, mst, mdroth, imammedo
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;
>> }
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-10-11 20:21 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20180918185246.18109-1-danielhb413@gmail.com>
2018-10-08 18:12 ` [Qemu-devel] [PATCH v9 0/3] wakeup-from-suspend and system_wakeup changes Daniel Henrique Barboza
[not found] ` <20180918185246.18109-2-danielhb413@gmail.com>
2018-10-11 7:38 ` [Qemu-devel] [PATCH v9 1/3] qmp: query-current-machine with wakeup-suspend-support Markus Armbruster
2018-10-11 12:08 ` Daniel Henrique Barboza
[not found] ` <20180918185246.18109-3-danielhb413@gmail.com>
2018-10-11 7:40 ` [Qemu-devel] [PATCH v9 2/3] qga: update guest-suspend-ram and guest-suspend-hybrid descriptions Markus Armbruster
2018-10-11 12:22 ` Daniel Henrique Barboza
[not found] ` <20180918185246.18109-4-danielhb413@gmail.com>
2018-10-11 7:45 ` [Qemu-devel] [PATCH v9 3/3] qmp hmp: Make system_wakeup check wake-up support and run state Markus Armbruster
2018-10-11 20:21 ` Daniel Henrique Barboza
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.