* 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
[parent not found: <20180918185246.18109-2-danielhb413@gmail.com>]
* 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 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
[parent not found: <20180918185246.18109-3-danielhb413@gmail.com>]
* 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 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
[parent not found: <20180918185246.18109-4-danielhb413@gmail.com>]
* 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 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.