qmp: 'wakeup-suspend-support' in query-target

When issuing the qmp/hmp 'system_wakeup' command, what happens in a nutshell is:

- qmp_system_wakeup_request set runstate to RUNNING, sets a wakeup_reason and notify the event
- in the main_loop, all vcpus are paused, a system reset is issued, all subscribers of wakeup_notifiers receives a notification, vcpus are then resumed and the wake up QAPI event is fired

Note that this procedure alone doesn't ensure that the guest will awake from SUSPENDED state - the subscribers of the wake up event must take action to resume the guest, otherwise the guest will simply reboot.

At this moment there are only two subscribers of the wake up event: one in hw/acpi/core.c and another one in hw/i386/xen/xen-hvm.c. This means that system_wakeup does not work as intended with other architectures.

However, only the presence of 'system_wakeup' is required for QGA to support 'guest-suspend-ram' and 'guest-suspend-hybrid' at this moment. This means that the user/management will expect to suspend the guest using one of those suspend commands and then resume execution using system_wakeup, regardless of the support offered in system_wakeup in the first place.

This patch adds a new flag called 'wakeup-suspend-support' in TargetInfo that allows the caller to query if the guest supports wake up from suspend via system_wakeup. It goes over the subscribers of the wake up event and, if it's empty, it assumes that the guest does not support wake up from suspend (and thus, pm-suspend itself).

This is the expected output of query-target when running a x86 guest:

{"execute" : "query-target"}
{"return": {"arch": "x86_64", "wakeup-suspend-support": true}}

This is the output when running a pseries guest:

{"execute" : "query-target"}
{"return": {"arch": "ppc64", "wakeup-suspend-support": false}}

Given that the TargetInfo structure is read-only, adding a new flag to it is backwards compatible. There is no need to deprecate the old TargetInfo format.

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

Reported-by Balamuruhan S <[email protected]>

Signed-off-by Daniel Henrique Barboza <[email protected]>

---
 arch_init.c             |  1 +
 include/sysemu/sysemu.h |  1 +
 qapi/misc.json          |  4 +++-
 vl.c                    | 21 +++++++++++++++++++++
 4 files changed, 26 insertions(+), 1 deletion(-)

arch_init.c +0 -0

index 9597218ced..67bdf27528 100644

arch_init.c +0 -0

     return info;
 }

include/sysemu/sysemu.h +1 -0

index 544ab77a2b..fbe2a3373e 100644
@@ -69,6 +69,7 @@ typedef enum WakeupReason {
 void qemu_system_reset_request(ShutdownCause reason);
 void qemu_system_suspend_request(void);
 void qemu_register_suspend_notifier(Notifier *notifier);
+bool qemu_wakeup_notifier_is_empty(void);
 void qemu_system_wakeup_request(WakeupReason reason);
 void qemu_system_wakeup_enable(WakeupReason reason, bool enabled);
 void qemu_register_wakeup_notifier(Notifier *notifier);

qapi/misc.json +1 -0

index f5988cc0b5..a385d897ae 100644
@@ -2484,11 +2484,13 @@
 # Information describing the QEMU target.
 #
 # @arch: the target architecture
+# @wakeup-suspend-support: true if the target supports wake up from

vl.c +5 -0

+ * qemu_wakeup_notifier_is_empty and it's used in the logic of the
+ * wakeup-suspend-support flag of QMP 'query-target' command. The idea
+ * of this flag is to indicate whether the guest supports wake-up from
+ * suspend (via system_wakeup QMP/HMP call for example), warning the user
+ * that the guest can't handle both wake-up from suspend and the suspend

Quoted 62 lines · Daniel Henrique Barboza

When issuing the qmp/hmp 'system_wakeup' command, what happens in a nutshell is:

- qmp_system_wakeup_request set runstate to RUNNING, sets a wakeup_reason and notify the event
- in the main_loop, all vcpus are paused, a system reset is issued, all subscribers of wakeup_notifiers receives a notification, vcpus are then resumed and the wake up QAPI event is fired

Note that this procedure alone doesn't ensure that the guest will awake from SUSPENDED state - the subscribers of the wake up event must take action to resume the guest, otherwise the guest will simply reboot.

At this moment there are only two subscribers of the wake up event: one in hw/acpi/core.c and another one in hw/i386/xen/xen-hvm.c. This means that system_wakeup does not work as intended with other architectures.

However, only the presence of 'system_wakeup' is required for QGA to support 'guest-suspend-ram' and 'guest-suspend-hybrid' at this moment. This means that the user/management will expect to suspend the guest using one of those suspend commands and then resume execution using system_wakeup, regardless of the support offered in system_wakeup in the first place.

This patch adds a new flag called 'wakeup-suspend-support' in TargetInfo that allows the caller to query if the guest supports wake up from suspend via system_wakeup. It goes over the subscribers of the wake up event and, if it's empty, it assumes that the guest does not support wake up from suspend (and thus, pm-suspend itself).

This is the expected output of query-target when running a x86 guest:

{"execute" : "query-target"}
{"return": {"arch": "x86_64", "wakeup-suspend-support": true}}

This is the output when running a pseries guest:

{"execute" : "query-target"}
{"return": {"arch": "ppc64", "wakeup-suspend-support": false}}

Given that the TargetInfo structure is read-only, adding a new flag to it is backwards compatible. There is no need to deprecate the old TargetInfo format.

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

Reported-by Balamuruhan S <[email protected]>

Signed-off-by Daniel Henrique Barboza <[email protected]>

---
  arch_init.c             |  1 +
  include/sysemu/sysemu.h |  1 +
  qapi/misc.json          |  4 +++-
  vl.c                    | 21 +++++++++++++++++++++
  4 files changed, 26 insertions(+), 1 deletion(-)

arch_init.c +0 -0

index 9597218ced..67bdf27528 100644

arch_init.c TargetInfo *qmp_query_target(Error **errp) · line 118

Quoted 4 lines

arch_init.c +1 -0

@@ -115,6 +115,7 @@ TargetInfo *qmp_query_target(Error **errp)
 
      info->arch = qapi_enum_parse(&SysEmuTarget_lookup, TARGET_NAME, -1,
                                   &error_abort);
+    info->wakeup_suspend_support = !qemu_wakeup_notifier_is_empty();

Huh? Hmm, see "hack" below.

Quoted 24 lines

arch_init.c +0 -0

      return info;
  }

include/sysemu/sysemu.h +1 -0

index 544ab77a2b..fbe2a3373e 100644
@@ -69,6 +69,7 @@ typedef enum WakeupReason {
  void qemu_system_reset_request(ShutdownCause reason);
  void qemu_system_suspend_request(void);
  void qemu_register_suspend_notifier(Notifier *notifier);
+bool qemu_wakeup_notifier_is_empty(void);
  void qemu_system_wakeup_request(WakeupReason reason);
  void qemu_system_wakeup_enable(WakeupReason reason, bool enabled);
  void qemu_register_wakeup_notifier(Notifier *notifier);

qapi/misc.json +1 -0

index f5988cc0b5..a385d897ae 100644
@@ -2484,11 +2484,13 @@
  # Information describing the QEMU target.
  #
  # @arch: the target architecture
+# @wakeup-suspend-support: true if the target supports wake up from

qapi/misc.json line 2496

Quoted 10 lines

qapi/misc.json +2 -1

+#                          suspend (since 2.13)
  #
  # Since: 1.2.0
  ##
  { 'struct': 'TargetInfo',
-  'data': { 'arch': 'SysEmuTarget' } }
+  'data': { 'arch': 'SysEmuTarget', 'wakeup-suspend-support': 'bool' } }
 
  ##
  # @query-target:

Does the documentation of system_wakeup need fixing?

    ##
    # @system_wakeup:
    #
    # Wakeup guest from suspend.  Does nothing in case the guest isn't suspended.
    #
    # Since:  1.1
    #
    # Returns:  nothing.
    #
    # Example:
    #
    # -> { "execute": "system_wakeup" }
    # <- { "return": {} }
    #
    ##
    { 'command': 'system_wakeup' }

I figure we better explain right here what the command does, both for
wakeup-suspend-support: false and true.

Quoted 8 lines

vl.c +0 -0

index 3b39bbd7a8..ab6bd7e090 100644
@@ -1832,11 +1832,32 @@ void qemu_system_wakeup_enable(WakeupReason reason, bool enabled)
      }
  }
 

vl.c void qemu_system_wakeup_enable(WakeupReason reason, bool enabled) · line 1835

Quoted 1 line

vl.c +1 -0

+/* The existence of a wake-up notifier is being checked in the function

Please wing both ends of this comment:

/*
* The existence of a wake-up notifier is being checked in the function

Quoted 6 lines

vl.c +5 -0

+ * qemu_wakeup_notifier_is_empty and it's used in the logic of the
+ * wakeup-suspend-support flag of QMP 'query-target' command. The idea
+ * of this flag is to indicate whether the guest supports wake-up from
+ * suspend (via system_wakeup QMP/HMP call for example), warning the user
+ * that the guest can't handle both wake-up from suspend and the suspend

vl.c void qemu_system_wakeup_enable(WakeupReason reason, bool enabled) · line 1850

Quoted 10 lines

vl.c +10 -0

+ * itself via QGA guest-suspend-ram and guest-suspend-hybrid (if it
+ * can't wake up, it can't be suspended safely).
+ *
+ * An assumption is made by the wakeup-suspend-support flag that only the
+ * guests that can go to RUN_STATE_SUSPENDED and wake up properly would
+ * be interested in this wakeup_notifier. Adding a wakeup_notifier for
+ * any other reason will break the logic of the wakeup-suspend-support
+ * flag and can lead to user/management confusion about the suspend/wake-up
+ * support of the guest.
+ */

The assumption is a bit of a hack, but I don't have better ideas. Thanks for commenting it clearly. However, ...

Quoted 5 lines

vl.c +0 -0

  void qemu_register_wakeup_notifier(Notifier *notifier)
  {
      notifier_list_add(&wakeup_notifiers, notifier);
  }
 

vl.c void qemu_system_wakeup_enable(WakeupReason reason, bool enabled) · line 1863

Quoted 8 lines

vl.c +5 -0

+bool qemu_wakeup_notifier_is_empty(void)
+{
+    return QLIST_EMPTY(&wakeup_notifiers.notifiers);
+}
+
  void qemu_system_killed(int signal, pid_t pid)
  {
      shutdown_signal = signal;

... I'd recommend to rename qemu_wakeup_notifier_is_empty() to qemu_wakeup_suspend_support(), because then the hack is a bit more isolated. With the current name, it spills into qmp_query_target(), which doesn't have such a helpful comment.

Quoted 121 lines · Markus Armbruster

Quoted 62 lines · Daniel Henrique Barboza

When issuing the qmp/hmp 'system_wakeup' command, what happens in a nutshell is:

- qmp_system_wakeup_request set runstate to RUNNING, sets a wakeup_reason and notify the event
- in the main_loop, all vcpus are paused, a system reset is issued, all subscribers of wakeup_notifiers receives a notification, vcpus are then resumed and the wake up QAPI event is fired

Note that this procedure alone doesn't ensure that the guest will awake from SUSPENDED state - the subscribers of the wake up event must take action to resume the guest, otherwise the guest will simply reboot.

At this moment there are only two subscribers of the wake up event: one in hw/acpi/core.c and another one in hw/i386/xen/xen-hvm.c. This means that system_wakeup does not work as intended with other architectures.

However, only the presence of 'system_wakeup' is required for QGA to support 'guest-suspend-ram' and 'guest-suspend-hybrid' at this moment. This means that the user/management will expect to suspend the guest using one of those suspend commands and then resume execution using system_wakeup, regardless of the support offered in system_wakeup in the first place.

This patch adds a new flag called 'wakeup-suspend-support' in TargetInfo that allows the caller to query if the guest supports wake up from suspend via system_wakeup. It goes over the subscribers of the wake up event and, if it's empty, it assumes that the guest does not support wake up from suspend (and thus, pm-suspend itself).

This is the expected output of query-target when running a x86 guest:

{"execute" : "query-target"}
{"return": {"arch": "x86_64", "wakeup-suspend-support": true}}

This is the output when running a pseries guest:

{"execute" : "query-target"}
{"return": {"arch": "ppc64", "wakeup-suspend-support": false}}

Given that the TargetInfo structure is read-only, adding a new flag to it is backwards compatible. There is no need to deprecate the old TargetInfo format.

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

Reported-by Balamuruhan S <[email protected]>

Signed-off-by Daniel Henrique Barboza <[email protected]>

---
  arch_init.c             |  1 +
  include/sysemu/sysemu.h |  1 +
  qapi/misc.json          |  4 +++-
  vl.c                    | 21 +++++++++++++++++++++
  4 files changed, 26 insertions(+), 1 deletion(-)

arch_init.c +0 -0

index 9597218ced..67bdf27528 100644

arch_init.c TargetInfo *qmp_query_target(Error **errp) · line 118

Quoted 4 lines

arch_init.c +1 -0

@@ -115,6 +115,7 @@ TargetInfo *qmp_query_target(Error **errp)
        info->arch = qapi_enum_parse(&SysEmuTarget_lookup,
 TARGET_NAME, -1,
                                   &error_abort);
+    info->wakeup_suspend_support = !qemu_wakeup_notifier_is_empty();

Huh? Hmm, see "hack" below.

Quoted 22 lines

arch_init.c +0 -0

        return info;
  }

include/sysemu/sysemu.h +1 -0

index 544ab77a2b..fbe2a3373e 100644
@@ -69,6 +69,7 @@ typedef enum WakeupReason {
  void qemu_system_reset_request(ShutdownCause reason);
  void qemu_system_suspend_request(void);
  void qemu_register_suspend_notifier(Notifier *notifier);
+bool qemu_wakeup_notifier_is_empty(void);
  void qemu_system_wakeup_request(WakeupReason reason);
  void qemu_system_wakeup_enable(WakeupReason reason, bool enabled);
  void qemu_register_wakeup_notifier(Notifier *notifier);

qapi/misc.json +0 -0

index f5988cc0b5..a385d897ae 100644
@@ -2484,11 +2484,13 @@
  # Information describing the QEMU target.
  #
  # @arch: the target architecture

qapi/misc.json line 2495

Quoted 10 lines

qapi/misc.json +3 -1

+# @wakeup-suspend-support: true if the target supports wake up from
+#                          suspend (since 2.13)
  #
  # Since: 1.2.0
  ##
  { 'struct': 'TargetInfo',
-  'data': { 'arch': 'SysEmuTarget' } }
+  'data': { 'arch': 'SysEmuTarget', 'wakeup-suspend-support': 'bool' } }
    ##
  # @query-target:

Does the documentation of system_wakeup need fixing?

    ##
    # @system_wakeup:
    #
    # Wakeup guest from suspend.  Does nothing in case the guest isn't suspended.
    #
    # Since:  1.1
    #
    # Returns:  nothing.
    #
    # Example:
    #
    # -> { "execute": "system_wakeup" }
    # <- { "return": {} }
    #
    ##
    { 'command': 'system_wakeup' }

Quoted 2 lines

I figure we better explain right here what the command does, both for
wakeup-suspend-support: false and true.

Hmm, I've re-sent a patch that changes a bit the behavior of system_wakeup yesterday. The command should now fail with an error if the VM isn't in SUSPENDED state. However, I failed to update this documentation in that patch.

What if I resend that system_wakeup patch with the updated documentation such
as:

   ##
   # @system_wakeup:
   #
   # Wakeup guest from suspend.  Throws an error in case the guest isn't suspended.
   #
   # Since:  1.1
   #
   # Returns:  nothing if succeed
   #

And then I believe we don't need to update this doc again - if the guest isn't suspended,
 system_wakeup will still fire up an error.

In fact, I could have added that patch in this series too since it kind of relates with these
changes as well. Let me know if you think it helps and I'll respin it together with this
series.

Quoted 145 lines · Daniel Henrique Barboza

Quoted 121 lines · Markus Armbruster

Quoted 62 lines · Daniel Henrique Barboza

When issuing the qmp/hmp 'system_wakeup' command, what happens in a nutshell is:

- qmp_system_wakeup_request set runstate to RUNNING, sets a wakeup_reason and notify the event
- in the main_loop, all vcpus are paused, a system reset is issued, all subscribers of wakeup_notifiers receives a notification, vcpus are then resumed and the wake up QAPI event is fired

Note that this procedure alone doesn't ensure that the guest will awake from SUSPENDED state - the subscribers of the wake up event must take action to resume the guest, otherwise the guest will simply reboot.

At this moment there are only two subscribers of the wake up event: one in hw/acpi/core.c and another one in hw/i386/xen/xen-hvm.c. This means that system_wakeup does not work as intended with other architectures.

However, only the presence of 'system_wakeup' is required for QGA to support 'guest-suspend-ram' and 'guest-suspend-hybrid' at this moment. This means that the user/management will expect to suspend the guest using one of those suspend commands and then resume execution using system_wakeup, regardless of the support offered in system_wakeup in the first place.

This patch adds a new flag called 'wakeup-suspend-support' in TargetInfo that allows the caller to query if the guest supports wake up from suspend via system_wakeup. It goes over the subscribers of the wake up event and, if it's empty, it assumes that the guest does not support wake up from suspend (and thus, pm-suspend itself).

This is the expected output of query-target when running a x86 guest:

{"execute" : "query-target"}
{"return": {"arch": "x86_64", "wakeup-suspend-support": true}}

This is the output when running a pseries guest:

{"execute" : "query-target"}
{"return": {"arch": "ppc64", "wakeup-suspend-support": false}}

Given that the TargetInfo structure is read-only, adding a new flag to it is backwards compatible. There is no need to deprecate the old TargetInfo format.

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

Reported-by Balamuruhan S <[email protected]>

Signed-off-by Daniel Henrique Barboza <[email protected]>

---
   arch_init.c             |  1 +
   include/sysemu/sysemu.h |  1 +
   qapi/misc.json          |  4 +++-
   vl.c                    | 21 +++++++++++++++++++++
   4 files changed, 26 insertions(+), 1 deletion(-)

arch_init.c +0 -0

index 9597218ced..67bdf27528 100644

arch_init.c TargetInfo *qmp_query_target(Error **errp) · line 118

Quoted 4 lines

arch_init.c +1 -0

@@ -115,6 +115,7 @@ TargetInfo *qmp_query_target(Error **errp)
         info->arch = qapi_enum_parse(&SysEmuTarget_lookup,
 TARGET_NAME, -1,
                                    &error_abort);
+    info->wakeup_suspend_support = !qemu_wakeup_notifier_is_empty();

Huh? Hmm, see "hack" below.

Quoted 22 lines

arch_init.c +0 -0

         return info;
   }

include/sysemu/sysemu.h +1 -0

index 544ab77a2b..fbe2a3373e 100644
@@ -69,6 +69,7 @@ typedef enum WakeupReason {
   void qemu_system_reset_request(ShutdownCause reason);
   void qemu_system_suspend_request(void);
   void qemu_register_suspend_notifier(Notifier *notifier);
+bool qemu_wakeup_notifier_is_empty(void);
   void qemu_system_wakeup_request(WakeupReason reason);
   void qemu_system_wakeup_enable(WakeupReason reason, bool enabled);
   void qemu_register_wakeup_notifier(Notifier *notifier);

qapi/misc.json +0 -0

index f5988cc0b5..a385d897ae 100644
@@ -2484,11 +2484,13 @@
   # Information describing the QEMU target.
   #
   # @arch: the target architecture

qapi/misc.json line 2495

Quoted 10 lines

qapi/misc.json +3 -1

+# @wakeup-suspend-support: true if the target supports wake up from
+#                          suspend (since 2.13)
   #
   # Since: 1.2.0
   ##
   { 'struct': 'TargetInfo',
-  'data': { 'arch': 'SysEmuTarget' } }
+  'data': { 'arch': 'SysEmuTarget', 'wakeup-suspend-support': 'bool' } }
     ##
   # @query-target:

Does the documentation of system_wakeup need fixing?

     ##
     # @system_wakeup:
     #
     # Wakeup guest from suspend.  Does nothing in case the guest isn't suspended.
     #
     # Since:  1.1
     #
     # Returns:  nothing.
     #
     # Example:
     #
     # -> { "execute": "system_wakeup" }
     # <- { "return": {} }
     #
     ##
     { 'command': 'system_wakeup' }

Quoted 2 lines

I figure we better explain right here what the command does, both for
wakeup-suspend-support: false and true.

Hmm, I've re-sent a patch that changes a bit the behavior of system_wakeup yesterday. The command should now fail with an error if the VM isn't in SUSPENDED state. However, I failed to update this documentation in that patch.

What if I resend that system_wakeup patch with the updated documentation such
as:

    ##
    # @system_wakeup:
    #
    # Wakeup guest from suspend.  Throws an error in case the guest isn't suspended.
    #
    # Since:  1.1
    #
    # Returns:  nothing if succeed
    #

Quoted 3 lines

And then I believe we don't need to update this doc again - if the guest isn't suspended,
 system_wakeup will still fire up an error.

What about a guest that is suspended under a QEMU that isn't capable of waking it?

Shouldn't system_wakeup's documentation point to query-target's wakeup-suspend-support?

Quoted 6 lines

In fact, I could have added that patch in this series too since it kind of relates with these
changes as well. Let me know if you think it helps and I'll respin it together with this
series.

Respinning in a single series is the simplest way to ensure the patches get committed in a sane order. I'd appreciate that.

[...]

This patch updates the descriptions of 'guest-suspend-ram' and 'guest-suspend-hybrid' to mention that both commands relies now on the existence of 'system_wakeup' and also on the proper support for wake up from suspend, retrieved by the 'wakeup-suspend-support' attribute of the 'query-target' QMP command.

Reported-by Balamuruhan S <[email protected]>

Signed-off-by Daniel Henrique Barboza <[email protected]>

Reviewed-by Michael Roth <[email protected]>

---
 qga/qapi-schema.json | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

qga/qapi-schema.json +0 -0

index 17884c7c70..e3fb8adfce 100644

qga/qapi-schema.json +0 -0

 #
 # This command does NOT return a response on success. There are two options
 # to check for success:

@@ -593,8 +596,11 @@
 # This command requires the pm-utils package to be installed in the guest.
 #
 # IMPORTANT: guest-suspend-hybrid requires QEMU to support the 'system_wakeup'

Quoted 18 lines · Daniel Henrique Barboza

This patch updates the descriptions of 'guest-suspend-ram' and 'guest-suspend-hybrid' to mention that both commands relies now on the existence of 'system_wakeup' and also on the proper support for wake up from suspend, retrieved by the 'wakeup-suspend-support' attribute of the 'query-target' QMP command.

Reported-by Balamuruhan S <[email protected]>

Signed-off-by Daniel Henrique Barboza <[email protected]>

Reviewed-by Michael Roth <[email protected]>

---
  qga/qapi-schema.json | 14 ++++++++++----
  1 file changed, 10 insertions(+), 4 deletions(-)

qga/qapi-schema.json +0 -0

index 17884c7c70..e3fb8adfce 100644

qga/qapi-schema.json line 573

Quoted 10 lines

qga/qapi-schema.json +5 -2

@@ -566,8 +566,11 @@
  # 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.
+# command and the guest to support wake up from suspend. Thus, it's
+# *required* to query QEMU for the presence of the 'system_wakeup' command
+# and to verify that wake up from suspend is enabled by checking the
+# 'wakeup-suspend-support' flag of 'query-target' QMP command, before issuing
+# guest-suspend-ram.

Isn't checking for presence of system_wakeup redundant?

When query-target tells us "system_wakeup works" by returning
wakeup-suspend-support: true, we surely have system_wakeup (or else query-target would be lying to us).

When it returns wakeup-suspend-support: false, it doesn't matter whether we have system_wakeup.

Unless I'm wrong, we can simplify this to something like

# IMPORTANT: guest-suspend-ram requires working wakeup support in # QEMU. You *must* check QMP command query-target returns # wakeup-suspend-support: true before issuing this command.

Quoted 7 lines

qga/qapi-schema.json +0 -0

  #
  # This command does NOT return a response on success. There are two options
  # to check for success:

@@ -593,8 +596,11 @@
  # This command requires the pm-utils package to be installed in the guest.
  #
  # IMPORTANT: guest-suspend-hybrid requires QEMU to support the 'system_wakeup'

qga/qapi-schema.json line 606

Quoted 10 lines

qga/qapi-schema.json +5 -2

-# command.  Thus, it's *required* to query QEMU for the presence of the
-# 'system_wakeup' command before issuing guest-suspend-hybrid.
+# command and the guest to support wake up from suspend. Thus, it's
+# *required* to query QEMU for the presence of the 'system_wakeup' command
+# and to verify that wake up from suspend is enabled by checking the
+# 'wakeup-suspend-support' flag of 'query-target' QMP command, before issuing
+# guest-suspend-hybrid.
  #
  # This command does NOT return a response on success. There are two options
  # to check for success:

Likewise.

Quoted 36 lines · Markus Armbruster

Quoted 18 lines · Daniel Henrique Barboza

This patch updates the descriptions of 'guest-suspend-ram' and 'guest-suspend-hybrid' to mention that both commands relies now on the existence of 'system_wakeup' and also on the proper support for wake up from suspend, retrieved by the 'wakeup-suspend-support' attribute of the 'query-target' QMP command.

Reported-by Balamuruhan S <[email protected]>

Signed-off-by Daniel Henrique Barboza <[email protected]>

Reviewed-by Michael Roth <[email protected]>

---
  qga/qapi-schema.json | 14 ++++++++++----
  1 file changed, 10 insertions(+), 4 deletions(-)

qga/qapi-schema.json +0 -0

index 17884c7c70..e3fb8adfce 100644

qga/qapi-schema.json line 573

Quoted 10 lines

qga/qapi-schema.json +5 -2

@@ -566,8 +566,11 @@
  # 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.
+# command and the guest to support wake up from suspend. Thus, it's
+# *required* to query QEMU for the presence of the 'system_wakeup' command
+# and to verify that wake up from suspend is enabled by checking the
+# 'wakeup-suspend-support' flag of 'query-target' QMP command, before issuing
+# guest-suspend-ram.

Isn't checking for presence of system_wakeup redundant?

When query-target tells us "system_wakeup works" by returning
wakeup-suspend-support: true, we surely have system_wakeup (or else query-target would be lying to us).

Quoted 8 lines

When it returns wakeup-suspend-support: false, it doesn't matter whether we have system_wakeup.

Unless I'm wrong, we can simplify this to something like

# IMPORTANT: guest-suspend-ram requires working wakeup support in # QEMU. You *must* check QMP command query-target returns # wakeup-suspend-support: true before issuing this command.

It is worth noticing that this API isn't checking for the existence of system_wakeup. It is checking whether there are notifiers added in the wakeup_notifiers QLIST.

However, I think we can simplify the text as you suggested because that part seems outdated anyway. Is there any relevant scenario where system_wakeup will not be present?