All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH] acpi: Add emulated sleep button
@ 2017-07-20  9:31 Stefan Fritsch
  2017-07-20 12:05 ` Eric Blake
  2017-07-20 14:59 ` Igor Mammedov
  0 siblings, 2 replies; 10+ messages in thread
From: Stefan Fritsch @ 2017-07-20  9:31 UTC (permalink / raw)
  To: qemu-devel
  Cc: Dr. David Alan Gilbert, Michael S. Tsirkin, Igor Mammedov,
	Paolo Bonzini, Richard Henderson, Eduardo Habkost, Eric Blake,
	Markus Armbruster

From: Stefan Fritsch <stefan_fritsch@genua.de>

Add an ACPI sleep button and QMP/HMP commands to trigger it.  A sleep
button is a so called "fixed hardware feature", which makes it more
suitable for putting the system to sleep than a laptop lid, for example.

The sleep button is disabled by default (Bit 5 in the FACP flags
register set and no button "device" present in SSDT/DSDT). Clearing said
bit enables it as a fixed feature device.

Signed-off-by: Stefan Fritsch <stefan_fritsch@genua.de>
---
 hmp-commands.hx         | 16 ++++++++++++++++
 hmp.c                   |  5 +++++
 hmp.h                   |  1 +
 hw/acpi/core.c          |  8 ++++++++
 hw/acpi/ich9.c          | 10 ++++++++++
 hw/acpi/piix4.c         | 12 ++++++++++++
 hw/i386/acpi-build.c    |  1 -
 include/hw/acpi/acpi.h  |  2 ++
 include/hw/acpi/ich9.h  |  1 +
 include/sysemu/sysemu.h |  2 ++
 qapi-schema.json        | 12 ++++++++++++
 qmp.c                   |  5 +++++
 tests/test-hmp.c        |  1 +
 vl.c                    | 29 +++++++++++++++++++++++++++++
 14 files changed, 104 insertions(+), 1 deletion(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 1941e19932..8ba4380883 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -638,6 +638,22 @@ Reset the system.
 ETEXI
 
     {
+        .name       = "system_sleep",
+        .args_type  = "",
+        .params     = "",
+        .help       = "send ACPI sleep event",
+        .cmd = hmp_system_sleep,
+    },
+
+STEXI
+@item system_sleep
+@findex system_sleep
+
+Push the virtual sleep button; if supported the system will enter
+an ACPI sleep state.
+ETEXI
+
+    {
         .name       = "system_powerdown",
         .args_type  = "",
         .params     = "",
diff --git a/hmp.c b/hmp.c
index bf1de747d5..b4b584016c 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1047,6 +1047,11 @@ void hmp_system_reset(Monitor *mon, const QDict *qdict)
     qmp_system_reset(NULL);
 }
 
+void hmp_system_sleep(Monitor *mon, const QDict *qdict)
+{
+    qmp_system_sleep(NULL);
+}
+
 void hmp_system_powerdown(Monitor *mon, const QDict *qdict)
 {
     qmp_system_powerdown(NULL);
diff --git a/hmp.h b/hmp.h
index 1ff455295e..15b53de9ed 100644
--- a/hmp.h
+++ b/hmp.h
@@ -45,6 +45,7 @@ void hmp_info_iothreads(Monitor *mon, const QDict *qdict);
 void hmp_quit(Monitor *mon, const QDict *qdict);
 void hmp_stop(Monitor *mon, const QDict *qdict);
 void hmp_system_reset(Monitor *mon, const QDict *qdict);
+void hmp_system_sleep(Monitor *mon, const QDict *qdict);
 void hmp_system_powerdown(Monitor *mon, const QDict *qdict);
 void hmp_cpu(Monitor *mon, const QDict *qdict);
 void hmp_memsave(Monitor *mon, const QDict *qdict);
diff --git a/hw/acpi/core.c b/hw/acpi/core.c
index 95fcac95a2..2ee64b6878 100644
--- a/hw/acpi/core.c
+++ b/hw/acpi/core.c
@@ -422,6 +422,14 @@ void acpi_pm1_evt_power_down(ACPIREGS *ar)
     }
 }
 
+void acpi_pm1_evt_sleep(ACPIREGS *ar)
+{
+    if (ar->pm1.evt.en & ACPI_BITMASK_SLEEP_BUTTON_ENABLE) {
+        ar->pm1.evt.sts |= ACPI_BITMASK_SLEEP_BUTTON_STATUS;
+        ar->tmr.update_sci(ar);
+    }
+}
+
 void acpi_pm1_evt_reset(ACPIREGS *ar)
 {
     ar->pm1.evt.sts = 0;
diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index c5d8646abc..3faeab4d2e 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -260,6 +260,14 @@ static void pm_reset(void *opaque)
     acpi_update_sci(&pm->acpi_regs, pm->irq);
 }
 
+static void pm_sleep_req(Notifier *n, void *opaque)
+{
+    ICH9LPCPMRegs *pm = container_of(n, ICH9LPCPMRegs, sleep_notifier);
+
+    acpi_pm1_evt_sleep(&pm->acpi_regs);
+}
+
+
 static void pm_powerdown_req(Notifier *n, void *opaque)
 {
     ICH9LPCPMRegs *pm = container_of(n, ICH9LPCPMRegs, powerdown_notifier);
@@ -299,6 +307,8 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
     qemu_register_reset(pm_reset, pm);
     pm->powerdown_notifier.notify = pm_powerdown_req;
     qemu_register_powerdown_notifier(&pm->powerdown_notifier);
+    pm->sleep_notifier.notify = pm_sleep_req;
+    qemu_register_sleep_notifier(&pm->sleep_notifier);
 
     legacy_acpi_cpu_hotplug_init(pci_address_space_io(lpc_pci),
         OBJECT(lpc_pci), &pm->gpe_cpu, ICH9_CPU_HOTPLUG_IO_BASE);
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index f276967365..15e20976c3 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -79,6 +79,7 @@ typedef struct PIIX4PMState {
     int smm_enabled;
     Notifier machine_ready;
     Notifier powerdown_notifier;
+    Notifier sleep_notifier;
 
     AcpiPciHpState acpi_pci_hotplug;
     bool use_acpi_pci_hotplug;
@@ -371,6 +372,15 @@ static void piix4_pm_powerdown_req(Notifier *n, void *opaque)
     acpi_pm1_evt_power_down(&s->ar);
 }
 
+static void piix4_pm_sleep_req(Notifier *n, void *opaque)
+{
+    PIIX4PMState *s = container_of(n, PIIX4PMState, sleep_notifier);
+
+    assert(s != NULL);
+    acpi_pm1_evt_sleep(&s->ar);
+}
+
+
 static void piix4_device_plug_cb(HotplugHandler *hotplug_dev,
                                  DeviceState *dev, Error **errp)
 {
@@ -535,6 +545,8 @@ static void piix4_pm_realize(PCIDevice *dev, Error **errp)
 
     s->powerdown_notifier.notify = piix4_pm_powerdown_req;
     qemu_register_powerdown_notifier(&s->powerdown_notifier);
+    s->sleep_notifier.notify = piix4_pm_sleep_req;
+    qemu_register_sleep_notifier(&s->sleep_notifier);
 
     s->machine_ready.notify = piix4_pm_machine_ready;
     qemu_add_machine_init_done_notifier(&s->machine_ready);
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 6b7bade183..06b28dacfe 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -294,7 +294,6 @@ static void fadt_setup(AcpiFadtDescriptorRev3 *fadt, AcpiPmInfo *pm)
     fadt->plvl3_lat = cpu_to_le16(0xfff); /* C3 state not supported */
     fadt->flags = cpu_to_le32((1 << ACPI_FADT_F_WBINVD) |
                               (1 << ACPI_FADT_F_PROC_C1) |
-                              (1 << ACPI_FADT_F_SLP_BUTTON) |
                               (1 << ACPI_FADT_F_RTC_S4));
     fadt->flags |= cpu_to_le32(1 << ACPI_FADT_F_USE_PLATFORM_CLOCK);
     /* APIC destination mode ("Flat Logical") has an upper limit of 8 CPUs
diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h
index 7b3d93cf0d..51cf901ef6 100644
--- a/include/hw/acpi/acpi.h
+++ b/include/hw/acpi/acpi.h
@@ -78,6 +78,7 @@
 #define ACPI_BITMASK_PM1_COMMON_ENABLED         ( \
         ACPI_BITMASK_RT_CLOCK_ENABLE        | \
         ACPI_BITMASK_POWER_BUTTON_ENABLE    | \
+        ACPI_BITMASK_SLEEP_BUTTON_ENABLE    | \
         ACPI_BITMASK_GLOBAL_LOCK_ENABLE     | \
         ACPI_BITMASK_TIMER_ENABLE)
 
@@ -148,6 +149,7 @@ void acpi_pm_tmr_reset(ACPIREGS *ar);
 /* PM1a_EVT: piix and ich9 don't implement PM1b. */
 uint16_t acpi_pm1_evt_get_sts(ACPIREGS *ar);
 void acpi_pm1_evt_power_down(ACPIREGS *ar);
+void acpi_pm1_evt_sleep(ACPIREGS *ar);
 void acpi_pm1_evt_reset(ACPIREGS *ar);
 void acpi_pm1_evt_init(ACPIREGS *ar, acpi_update_sci_fn update_sci,
                        MemoryRegion *parent);
diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
index a352c94fde..2073eec168 100644
--- a/include/hw/acpi/ich9.h
+++ b/include/hw/acpi/ich9.h
@@ -48,6 +48,7 @@ typedef struct ICH9LPCPMRegs {
 
     uint32_t pm_io_base;
     Notifier powerdown_notifier;
+    Notifier sleep_notifier;
 
     bool cpu_hotplug_legacy;
     AcpiCpuHotplug gpe_cpu;
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index b21369672a..00f54653dc 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -75,6 +75,8 @@ void qemu_register_wakeup_notifier(Notifier *notifier);
 void qemu_system_shutdown_request(ShutdownCause reason);
 void qemu_system_powerdown_request(void);
 void qemu_register_powerdown_notifier(Notifier *notifier);
+void qemu_system_sleep_request(void);
+void qemu_register_sleep_notifier(Notifier *notifier);
 void qemu_system_debug_request(void);
 void qemu_system_vmstop_request(RunState reason);
 void qemu_system_vmstop_request_prepare(void);
diff --git a/qapi-schema.json b/qapi-schema.json
index 8b015bee2e..c6ccfcd70f 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2314,6 +2314,18 @@
 { 'command': 'system_reset' }
 
 ##
+# @system_sleep:
+#
+# Requests that a guest perform a ACPI sleep transition by pushing a virtual
+# sleep button.
+#
+# Notes: A guest may or may not respond to this command.  This command
+#        returning does not indicate that a guest has accepted the request or
+#        that it has gone to sleep.
+##
+{ 'command': 'system_sleep' }
+
+##
 # @system_powerdown:
 #
 # Requests that a guest perform a powerdown operation.
diff --git a/qmp.c b/qmp.c
index b86201e349..bc1f2e3d7f 100644
--- a/qmp.c
+++ b/qmp.c
@@ -108,6 +108,11 @@ void qmp_system_reset(Error **errp)
     qemu_system_reset_request(SHUTDOWN_CAUSE_HOST_QMP);
 }
 
+void qmp_system_sleep(Error **erp)
+{
+    qemu_system_sleep_request();
+}
+
 void qmp_system_powerdown(Error **erp)
 {
     qemu_system_powerdown_request();
diff --git a/tests/test-hmp.c b/tests/test-hmp.c
index d77b3c8710..3fa850bf1e 100644
--- a/tests/test-hmp.c
+++ b/tests/test-hmp.c
@@ -67,6 +67,7 @@ static const char *hmp_cmds[] = {
     "sum 0 512",
     "x /8i 0x100",
     "xp /16x 0",
+    "system_sleep",
     NULL
 };
 
diff --git a/vl.c b/vl.c
index fb6b2efafa..6a0f847dcf 100644
--- a/vl.c
+++ b/vl.c
@@ -1608,6 +1608,7 @@ static ShutdownCause reset_requested;
 static ShutdownCause shutdown_requested;
 static int shutdown_signal;
 static pid_t shutdown_pid;
+static int sleep_requested;
 static int powerdown_requested;
 static int debug_requested;
 static int suspend_requested;
@@ -1618,6 +1619,8 @@ static NotifierList suspend_notifiers =
     NOTIFIER_LIST_INITIALIZER(suspend_notifiers);
 static NotifierList wakeup_notifiers =
     NOTIFIER_LIST_INITIALIZER(wakeup_notifiers);
+static NotifierList sleep_notifiers =
+    NOTIFIER_LIST_INITIALIZER(sleep_notifiers);
 static uint32_t wakeup_reason_mask = ~(1 << QEMU_WAKEUP_REASON_NONE);
 
 ShutdownCause qemu_shutdown_requested_get(void)
@@ -1838,6 +1841,24 @@ static void qemu_system_powerdown(void)
     notifier_list_notify(&powerdown_notifiers, NULL);
 }
 
+static void qemu_system_sleep(void)
+{
+    notifier_list_notify(&sleep_notifiers, NULL);
+}
+
+static int qemu_sleep_requested(void)
+{
+    int r = sleep_requested;
+    sleep_requested = 0;
+    return r;
+}
+
+void qemu_system_sleep_request(void)
+{
+    sleep_requested = 1;
+    qemu_notify_event();
+}
+
 void qemu_system_powerdown_request(void)
 {
     trace_qemu_system_powerdown_request();
@@ -1850,6 +1871,11 @@ void qemu_register_powerdown_notifier(Notifier *notifier)
     notifier_list_add(&powerdown_notifiers, notifier);
 }
 
+void qemu_register_sleep_notifier(Notifier *notifier)
+{
+    notifier_list_add(&sleep_notifiers, notifier);
+}
+
 void qemu_system_debug_request(void)
 {
     debug_requested = 1;
@@ -1899,6 +1925,9 @@ static bool main_loop_should_exit(void)
     if (qemu_powerdown_requested()) {
         qemu_system_powerdown();
     }
+    if (qemu_sleep_requested()) {
+        qemu_system_sleep();
+    }
     if (qemu_vmstop_requested(&r)) {
         vm_stop(r);
     }
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [PATCH] acpi: Add emulated sleep button
  2017-07-20  9:31 [Qemu-devel] [PATCH] acpi: Add emulated sleep button Stefan Fritsch
@ 2017-07-20 12:05 ` Eric Blake
  2017-07-20 12:27   ` Stefan Fritsch
  2017-07-20 14:59 ` Igor Mammedov
  1 sibling, 1 reply; 10+ messages in thread
From: Eric Blake @ 2017-07-20 12:05 UTC (permalink / raw)
  To: Stefan Fritsch, qemu-devel
  Cc: Dr. David Alan Gilbert, Michael S. Tsirkin, Igor Mammedov,
	Paolo Bonzini, Richard Henderson, Eduardo Habkost,
	Markus Armbruster

[-- Attachment #1: Type: text/plain, Size: 1679 bytes --]

On 07/20/2017 04:31 AM, Stefan Fritsch wrote:
> From: Stefan Fritsch <stefan_fritsch@genua.de>
> 
> Add an ACPI sleep button and QMP/HMP commands to trigger it.  A sleep
> button is a so called "fixed hardware feature", which makes it more
> suitable for putting the system to sleep than a laptop lid, for example.
> 
> The sleep button is disabled by default (Bit 5 in the FACP flags
> register set and no button "device" present in SSDT/DSDT). Clearing said
> bit enables it as a fixed feature device.
> 
> Signed-off-by: Stefan Fritsch <stefan_fritsch@genua.de>
> ---

Just an interface review:

> +++ b/qapi-schema.json
> @@ -2314,6 +2314,18 @@
>  { 'command': 'system_reset' }
>  
>  ##
> +# @system_sleep:
> +#
> +# Requests that a guest perform a ACPI sleep transition by pushing a virtual

s/a /an /

> +# sleep button.
> +#
> +# Notes: A guest may or may not respond to this command.  This command
> +#        returning does not indicate that a guest has accepted the request or
> +#        that it has gone to sleep.

Missing a 'Since: 2.11' line.

> +##
> +{ 'command': 'system_sleep' }

Please name this 'system-sleep' (new QMP commands should favor '-' over
'_').  Or at least document in the commit message that you are
intentionally trying to match spelling of older existing system_*
commands.  I wonder if it might be better to have a single system-acpi
command that takes an enum of which action (sleep, powerdown, ...) to
attempt, rather than a proliferation of commands per action..

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [PATCH] acpi: Add emulated sleep button
  2017-07-20 12:05 ` Eric Blake
@ 2017-07-20 12:27   ` Stefan Fritsch
  0 siblings, 0 replies; 10+ messages in thread
From: Stefan Fritsch @ 2017-07-20 12:27 UTC (permalink / raw)
  To: Eric Blake
  Cc: qemu-devel, Dr. David Alan Gilbert, Michael S. Tsirkin,
	Igor Mammedov, Paolo Bonzini, Richard Henderson, Eduardo Habkost,
	Markus Armbruster

On Thu, 20 Jul 2017, Eric Blake wrote:

> On 07/20/2017 04:31 AM, Stefan Fritsch wrote:
> > From: Stefan Fritsch <stefan_fritsch@genua.de>
> > 
> > Add an ACPI sleep button and QMP/HMP commands to trigger it.  A sleep
> > button is a so called "fixed hardware feature", which makes it more
> > suitable for putting the system to sleep than a laptop lid, for example.
> > 
> > The sleep button is disabled by default (Bit 5 in the FACP flags
> > register set and no button "device" present in SSDT/DSDT). Clearing said
> > bit enables it as a fixed feature device.
> > 
> > Signed-off-by: Stefan Fritsch <stefan_fritsch@genua.de>
> > ---
> 
> Just an interface review:
> 
> > +++ b/qapi-schema.json
> > @@ -2314,6 +2314,18 @@
> >  { 'command': 'system_reset' }
> >  
> >  ##
> > +# @system_sleep:
> > +#
> > +# Requests that a guest perform a ACPI sleep transition by pushing a virtual
> 
> s/a /an /
> 
> > +# sleep button.
> > +#
> > +# Notes: A guest may or may not respond to this command.  This command
> > +#        returning does not indicate that a guest has accepted the request or
> > +#        that it has gone to sleep.
> 
> Missing a 'Since: 2.11' line.
> 
> > +##
> > +{ 'command': 'system_sleep' }
> 
> Please name this 'system-sleep' (new QMP commands should favor '-' over
> '_').  Or at least document in the commit message that you are
> intentionally trying to match spelling of older existing system_*
> commands.  I wonder if it might be better to have a single system-acpi
> command that takes an enum of which action (sleep, powerdown, ...) to
> attempt, rather than a proliferation of commands per action..

I would prefer to match the spelling of older existing system_* commands 
and will put that in the commit message. But weould be fine with the other 
solutions, too.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [PATCH] acpi: Add emulated sleep button
  2017-07-20  9:31 [Qemu-devel] [PATCH] acpi: Add emulated sleep button Stefan Fritsch
  2017-07-20 12:05 ` Eric Blake
@ 2017-07-20 14:59 ` Igor Mammedov
  2021-08-06 20:18   ` Annie.li
  1 sibling, 1 reply; 10+ messages in thread
From: Igor Mammedov @ 2017-07-20 14:59 UTC (permalink / raw)
  To: Stefan Fritsch
  Cc: qemu-devel, Eduardo Habkost, Michael S. Tsirkin,
	Dr. David Alan Gilbert, Markus Armbruster, Paolo Bonzini,
	Richard Henderson

On Thu, 20 Jul 2017 11:31:26 +0200
Stefan Fritsch <sf@sfritsch.de> wrote:

> From: Stefan Fritsch <stefan_fritsch@genua.de>
> 
> Add an ACPI sleep button and QMP/HMP commands to trigger it.  A sleep
> button is a so called "fixed hardware feature", which makes it more
> suitable for putting the system to sleep than a laptop lid, for example.
> 
> The sleep button is disabled by default (Bit 5 in the FACP flags
> register set and no button "device" present in SSDT/DSDT). Clearing said
> bit enables it as a fixed feature device.

per spec sleep button is used for both putting system into
sleep and for waking it up.

Reusing system_wakeup 'button' to behave as per spec would
make this patch significantly smaller.

If you like idea of separate command/button better, then
I'd go generic control sleep button way instead of fixed
hardware, it would allow us to reuse most of the code in
ARM target, plus I'd avoid notifiers and use acpi device
lookup instead (see: qmp_query_acpi_ospm_status as example)


> Signed-off-by: Stefan Fritsch <stefan_fritsch@genua.de>
> ---
>  hmp-commands.hx         | 16 ++++++++++++++++
>  hmp.c                   |  5 +++++
>  hmp.h                   |  1 +
>  hw/acpi/core.c          |  8 ++++++++
>  hw/acpi/ich9.c          | 10 ++++++++++
>  hw/acpi/piix4.c         | 12 ++++++++++++
>  hw/i386/acpi-build.c    |  1 -
>  include/hw/acpi/acpi.h  |  2 ++
>  include/hw/acpi/ich9.h  |  1 +
>  include/sysemu/sysemu.h |  2 ++
>  qapi-schema.json        | 12 ++++++++++++
>  qmp.c                   |  5 +++++
>  tests/test-hmp.c        |  1 +
>  vl.c                    | 29 +++++++++++++++++++++++++++++
>  14 files changed, 104 insertions(+), 1 deletion(-)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 1941e19932..8ba4380883 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -638,6 +638,22 @@ Reset the system.
>  ETEXI
>  
>      {
> +        .name       = "system_sleep",
> +        .args_type  = "",
> +        .params     = "",
> +        .help       = "send ACPI sleep event",
> +        .cmd = hmp_system_sleep,
> +    },
> +
> +STEXI
> +@item system_sleep
> +@findex system_sleep
> +
> +Push the virtual sleep button; if supported the system will enter
> +an ACPI sleep state.
> +ETEXI
> +
> +    {
>          .name       = "system_powerdown",
>          .args_type  = "",
>          .params     = "",
> diff --git a/hmp.c b/hmp.c
> index bf1de747d5..b4b584016c 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1047,6 +1047,11 @@ void hmp_system_reset(Monitor *mon, const QDict *qdict)
>      qmp_system_reset(NULL);
>  }
>  
> +void hmp_system_sleep(Monitor *mon, const QDict *qdict)
> +{
> +    qmp_system_sleep(NULL);
> +}
> +
>  void hmp_system_powerdown(Monitor *mon, const QDict *qdict)
>  {
>      qmp_system_powerdown(NULL);
> diff --git a/hmp.h b/hmp.h
> index 1ff455295e..15b53de9ed 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -45,6 +45,7 @@ void hmp_info_iothreads(Monitor *mon, const QDict *qdict);
>  void hmp_quit(Monitor *mon, const QDict *qdict);
>  void hmp_stop(Monitor *mon, const QDict *qdict);
>  void hmp_system_reset(Monitor *mon, const QDict *qdict);
> +void hmp_system_sleep(Monitor *mon, const QDict *qdict);
>  void hmp_system_powerdown(Monitor *mon, const QDict *qdict);
>  void hmp_cpu(Monitor *mon, const QDict *qdict);
>  void hmp_memsave(Monitor *mon, const QDict *qdict);
> diff --git a/hw/acpi/core.c b/hw/acpi/core.c
> index 95fcac95a2..2ee64b6878 100644
> --- a/hw/acpi/core.c
> +++ b/hw/acpi/core.c
> @@ -422,6 +422,14 @@ void acpi_pm1_evt_power_down(ACPIREGS *ar)
>      }
>  }
>  
> +void acpi_pm1_evt_sleep(ACPIREGS *ar)
> +{
> +    if (ar->pm1.evt.en & ACPI_BITMASK_SLEEP_BUTTON_ENABLE) {
> +        ar->pm1.evt.sts |= ACPI_BITMASK_SLEEP_BUTTON_STATUS;
> +        ar->tmr.update_sci(ar);
> +    }
> +}
> +
>  void acpi_pm1_evt_reset(ACPIREGS *ar)
>  {
>      ar->pm1.evt.sts = 0;
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index c5d8646abc..3faeab4d2e 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -260,6 +260,14 @@ static void pm_reset(void *opaque)
>      acpi_update_sci(&pm->acpi_regs, pm->irq);
>  }
>  
> +static void pm_sleep_req(Notifier *n, void *opaque)
> +{
> +    ICH9LPCPMRegs *pm = container_of(n, ICH9LPCPMRegs, sleep_notifier);
> +
> +    acpi_pm1_evt_sleep(&pm->acpi_regs);
> +}
> +
> +
>  static void pm_powerdown_req(Notifier *n, void *opaque)
>  {
>      ICH9LPCPMRegs *pm = container_of(n, ICH9LPCPMRegs, powerdown_notifier);
> @@ -299,6 +307,8 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
>      qemu_register_reset(pm_reset, pm);
>      pm->powerdown_notifier.notify = pm_powerdown_req;
>      qemu_register_powerdown_notifier(&pm->powerdown_notifier);
> +    pm->sleep_notifier.notify = pm_sleep_req;
> +    qemu_register_sleep_notifier(&pm->sleep_notifier);
>  
>      legacy_acpi_cpu_hotplug_init(pci_address_space_io(lpc_pci),
>          OBJECT(lpc_pci), &pm->gpe_cpu, ICH9_CPU_HOTPLUG_IO_BASE);
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index f276967365..15e20976c3 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -79,6 +79,7 @@ typedef struct PIIX4PMState {
>      int smm_enabled;
>      Notifier machine_ready;
>      Notifier powerdown_notifier;
> +    Notifier sleep_notifier;
>  
>      AcpiPciHpState acpi_pci_hotplug;
>      bool use_acpi_pci_hotplug;
> @@ -371,6 +372,15 @@ static void piix4_pm_powerdown_req(Notifier *n, void *opaque)
>      acpi_pm1_evt_power_down(&s->ar);
>  }
>  
> +static void piix4_pm_sleep_req(Notifier *n, void *opaque)
> +{
> +    PIIX4PMState *s = container_of(n, PIIX4PMState, sleep_notifier);
> +
> +    assert(s != NULL);
> +    acpi_pm1_evt_sleep(&s->ar);
> +}
> +
> +
>  static void piix4_device_plug_cb(HotplugHandler *hotplug_dev,
>                                   DeviceState *dev, Error **errp)
>  {
> @@ -535,6 +545,8 @@ static void piix4_pm_realize(PCIDevice *dev, Error **errp)
>  
>      s->powerdown_notifier.notify = piix4_pm_powerdown_req;
>      qemu_register_powerdown_notifier(&s->powerdown_notifier);
> +    s->sleep_notifier.notify = piix4_pm_sleep_req;
> +    qemu_register_sleep_notifier(&s->sleep_notifier);
>  
>      s->machine_ready.notify = piix4_pm_machine_ready;
>      qemu_add_machine_init_done_notifier(&s->machine_ready);
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 6b7bade183..06b28dacfe 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -294,7 +294,6 @@ static void fadt_setup(AcpiFadtDescriptorRev3 *fadt, AcpiPmInfo *pm)
>      fadt->plvl3_lat = cpu_to_le16(0xfff); /* C3 state not supported */
>      fadt->flags = cpu_to_le32((1 << ACPI_FADT_F_WBINVD) |
>                                (1 << ACPI_FADT_F_PROC_C1) |
> -                              (1 << ACPI_FADT_F_SLP_BUTTON) |
>                                (1 << ACPI_FADT_F_RTC_S4));
>      fadt->flags |= cpu_to_le32(1 << ACPI_FADT_F_USE_PLATFORM_CLOCK);
>      /* APIC destination mode ("Flat Logical") has an upper limit of 8 CPUs
> diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h
> index 7b3d93cf0d..51cf901ef6 100644
> --- a/include/hw/acpi/acpi.h
> +++ b/include/hw/acpi/acpi.h
> @@ -78,6 +78,7 @@
>  #define ACPI_BITMASK_PM1_COMMON_ENABLED         ( \
>          ACPI_BITMASK_RT_CLOCK_ENABLE        | \
>          ACPI_BITMASK_POWER_BUTTON_ENABLE    | \
> +        ACPI_BITMASK_SLEEP_BUTTON_ENABLE    | \
>          ACPI_BITMASK_GLOBAL_LOCK_ENABLE     | \
>          ACPI_BITMASK_TIMER_ENABLE)
>  
> @@ -148,6 +149,7 @@ void acpi_pm_tmr_reset(ACPIREGS *ar);
>  /* PM1a_EVT: piix and ich9 don't implement PM1b. */
>  uint16_t acpi_pm1_evt_get_sts(ACPIREGS *ar);
>  void acpi_pm1_evt_power_down(ACPIREGS *ar);
> +void acpi_pm1_evt_sleep(ACPIREGS *ar);
>  void acpi_pm1_evt_reset(ACPIREGS *ar);
>  void acpi_pm1_evt_init(ACPIREGS *ar, acpi_update_sci_fn update_sci,
>                         MemoryRegion *parent);
> diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
> index a352c94fde..2073eec168 100644
> --- a/include/hw/acpi/ich9.h
> +++ b/include/hw/acpi/ich9.h
> @@ -48,6 +48,7 @@ typedef struct ICH9LPCPMRegs {
>  
>      uint32_t pm_io_base;
>      Notifier powerdown_notifier;
> +    Notifier sleep_notifier;
>  
>      bool cpu_hotplug_legacy;
>      AcpiCpuHotplug gpe_cpu;
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index b21369672a..00f54653dc 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -75,6 +75,8 @@ void qemu_register_wakeup_notifier(Notifier *notifier);
>  void qemu_system_shutdown_request(ShutdownCause reason);
>  void qemu_system_powerdown_request(void);
>  void qemu_register_powerdown_notifier(Notifier *notifier);
> +void qemu_system_sleep_request(void);
> +void qemu_register_sleep_notifier(Notifier *notifier);
>  void qemu_system_debug_request(void);
>  void qemu_system_vmstop_request(RunState reason);
>  void qemu_system_vmstop_request_prepare(void);
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 8b015bee2e..c6ccfcd70f 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2314,6 +2314,18 @@
>  { 'command': 'system_reset' }
>  
>  ##
> +# @system_sleep:
> +#
> +# Requests that a guest perform a ACPI sleep transition by pushing a virtual
> +# sleep button.
> +#
> +# Notes: A guest may or may not respond to this command.  This command
> +#        returning does not indicate that a guest has accepted the request or
> +#        that it has gone to sleep.
> +##
> +{ 'command': 'system_sleep' }
> +
> +##
>  # @system_powerdown:
>  #
>  # Requests that a guest perform a powerdown operation.
> diff --git a/qmp.c b/qmp.c
> index b86201e349..bc1f2e3d7f 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -108,6 +108,11 @@ void qmp_system_reset(Error **errp)
>      qemu_system_reset_request(SHUTDOWN_CAUSE_HOST_QMP);
>  }
>  
> +void qmp_system_sleep(Error **erp)
> +{
> +    qemu_system_sleep_request();
> +}
> +
>  void qmp_system_powerdown(Error **erp)
>  {
>      qemu_system_powerdown_request();
> diff --git a/tests/test-hmp.c b/tests/test-hmp.c
> index d77b3c8710..3fa850bf1e 100644
> --- a/tests/test-hmp.c
> +++ b/tests/test-hmp.c
> @@ -67,6 +67,7 @@ static const char *hmp_cmds[] = {
>      "sum 0 512",
>      "x /8i 0x100",
>      "xp /16x 0",
> +    "system_sleep",
>      NULL
>  };
>  
> diff --git a/vl.c b/vl.c
> index fb6b2efafa..6a0f847dcf 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1608,6 +1608,7 @@ static ShutdownCause reset_requested;
>  static ShutdownCause shutdown_requested;
>  static int shutdown_signal;
>  static pid_t shutdown_pid;
> +static int sleep_requested;
>  static int powerdown_requested;
>  static int debug_requested;
>  static int suspend_requested;
> @@ -1618,6 +1619,8 @@ static NotifierList suspend_notifiers =
>      NOTIFIER_LIST_INITIALIZER(suspend_notifiers);
>  static NotifierList wakeup_notifiers =
>      NOTIFIER_LIST_INITIALIZER(wakeup_notifiers);
> +static NotifierList sleep_notifiers =
> +    NOTIFIER_LIST_INITIALIZER(sleep_notifiers);
>  static uint32_t wakeup_reason_mask = ~(1 << QEMU_WAKEUP_REASON_NONE);
>  
>  ShutdownCause qemu_shutdown_requested_get(void)
> @@ -1838,6 +1841,24 @@ static void qemu_system_powerdown(void)
>      notifier_list_notify(&powerdown_notifiers, NULL);
>  }
>  
> +static void qemu_system_sleep(void)
> +{
> +    notifier_list_notify(&sleep_notifiers, NULL);
> +}
> +
> +static int qemu_sleep_requested(void)
> +{
> +    int r = sleep_requested;
> +    sleep_requested = 0;
> +    return r;
> +}
> +
> +void qemu_system_sleep_request(void)
> +{
> +    sleep_requested = 1;
> +    qemu_notify_event();
> +}
> +
>  void qemu_system_powerdown_request(void)
>  {
>      trace_qemu_system_powerdown_request();
> @@ -1850,6 +1871,11 @@ void qemu_register_powerdown_notifier(Notifier *notifier)
>      notifier_list_add(&powerdown_notifiers, notifier);
>  }
>  
> +void qemu_register_sleep_notifier(Notifier *notifier)
> +{
> +    notifier_list_add(&sleep_notifiers, notifier);
> +}
> +
>  void qemu_system_debug_request(void)
>  {
>      debug_requested = 1;
> @@ -1899,6 +1925,9 @@ static bool main_loop_should_exit(void)
>      if (qemu_powerdown_requested()) {
>          qemu_system_powerdown();
>      }
> +    if (qemu_sleep_requested()) {
> +        qemu_system_sleep();
> +    }
>      if (qemu_vmstop_requested(&r)) {
>          vm_stop(r);
>      }

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [PATCH] acpi: Add emulated sleep button
  2017-07-20 14:59 ` Igor Mammedov
@ 2021-08-06 20:18   ` Annie.li
  2021-09-20  7:53     ` Igor Mammedov
  0 siblings, 1 reply; 10+ messages in thread
From: Annie.li @ 2021-08-06 20:18 UTC (permalink / raw)
  To: Igor Mammedov, Stefan Fritsch
  Cc: Eduardo Habkost, Michael S. Tsirkin, Markus Armbruster,
	qemu-devel, Paolo Bonzini, Dr. David Alan Gilbert,
	Richard Henderson

Hello Igor,

This is an old patch, but it does what we need.
I am getting a little bit lost about not implementing fixed hardware
sleep button, can you please clarify? thank you!

On 7/20/2017 10:59 AM, Igor Mammedov wrote:
> On Thu, 20 Jul 2017 11:31:26 +0200
> Stefan Fritsch <sf@sfritsch.de> wrote:
>
>> From: Stefan Fritsch <stefan_fritsch@genua.de>
>>
>> Add an ACPI sleep button and QMP/HMP commands to trigger it.  A sleep
>> button is a so called "fixed hardware feature", which makes it more
>> suitable for putting the system to sleep than a laptop lid, for example.
>>
>> The sleep button is disabled by default (Bit 5 in the FACP flags
>> register set and no button "device" present in SSDT/DSDT). Clearing said
>> bit enables it as a fixed feature device.
> per spec sleep button is used for both putting system into
> sleep and for waking it up.
>
> Reusing system_wakeup 'button' to behave as per spec would
> make this patch significantly smaller.

About reusing "system_wakeup", does it mean the following?

1. when guest is in sleep state, "system_wakeup" wakes up the guest
2. when guest is running, "system_wakeup" puts the guest into sleep

"system_wakeup" sets WAK_STS and then system transitions to the
working state. Correspondingly, I suppose both SLPBTN_STS and
SLPBTN_EN need to be set for sleeping, and this is what fixed
hardware sleep button requires?

I have combined the sleep and wakeup together, share the
code between. But Xen also registers the wakeup notifier, and
this makes things more complicated if this notifier is shared
between sleep and wakeup. Or this is my misunderstanding?
please correct me if I am wrong.

> If you like idea of separate command/button better, then
> I'd go generic control sleep button way instead of fixed
> hardware, it would allow us to reuse most of the code in
> ARM target, plus I'd avoid notifiers and use acpi device
> lookup instead (see: qmp_query_acpi_ospm_status as example)
>
Do you mean the "Control Method Sleep Button" that needs to
notify OSPM by event indication 0x80?

Thanks
Annie
>> Signed-off-by: Stefan Fritsch <stefan_fritsch@genua.de>
>> ---
>>   hmp-commands.hx         | 16 ++++++++++++++++
>>   hmp.c                   |  5 +++++
>>   hmp.h                   |  1 +
>>   hw/acpi/core.c          |  8 ++++++++
>>   hw/acpi/ich9.c          | 10 ++++++++++
>>   hw/acpi/piix4.c         | 12 ++++++++++++
>>   hw/i386/acpi-build.c    |  1 -
>>   include/hw/acpi/acpi.h  |  2 ++
>>   include/hw/acpi/ich9.h  |  1 +
>>   include/sysemu/sysemu.h |  2 ++
>>   qapi-schema.json        | 12 ++++++++++++
>>   qmp.c                   |  5 +++++
>>   tests/test-hmp.c        |  1 +
>>   vl.c                    | 29 +++++++++++++++++++++++++++++
>>   14 files changed, 104 insertions(+), 1 deletion(-)
>>
>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>> index 1941e19932..8ba4380883 100644
>> --- a/hmp-commands.hx
>> +++ b/hmp-commands.hx
>> @@ -638,6 +638,22 @@ Reset the system.
>>   ETEXI
>>   
>>       {
>> +        .name       = "system_sleep",
>> +        .args_type  = "",
>> +        .params     = "",
>> +        .help       = "send ACPI sleep event",
>> +        .cmd = hmp_system_sleep,
>> +    },
>> +
>> +STEXI
>> +@item system_sleep
>> +@findex system_sleep
>> +
>> +Push the virtual sleep button; if supported the system will enter
>> +an ACPI sleep state.
>> +ETEXI
>> +
>> +    {
>>           .name       = "system_powerdown",
>>           .args_type  = "",
>>           .params     = "",
>> diff --git a/hmp.c b/hmp.c
>> index bf1de747d5..b4b584016c 100644
>> --- a/hmp.c
>> +++ b/hmp.c
>> @@ -1047,6 +1047,11 @@ void hmp_system_reset(Monitor *mon, const QDict *qdict)
>>       qmp_system_reset(NULL);
>>   }
>>   
>> +void hmp_system_sleep(Monitor *mon, const QDict *qdict)
>> +{
>> +    qmp_system_sleep(NULL);
>> +}
>> +
>>   void hmp_system_powerdown(Monitor *mon, const QDict *qdict)
>>   {
>>       qmp_system_powerdown(NULL);
>> diff --git a/hmp.h b/hmp.h
>> index 1ff455295e..15b53de9ed 100644
>> --- a/hmp.h
>> +++ b/hmp.h
>> @@ -45,6 +45,7 @@ void hmp_info_iothreads(Monitor *mon, const QDict *qdict);
>>   void hmp_quit(Monitor *mon, const QDict *qdict);
>>   void hmp_stop(Monitor *mon, const QDict *qdict);
>>   void hmp_system_reset(Monitor *mon, const QDict *qdict);
>> +void hmp_system_sleep(Monitor *mon, const QDict *qdict);
>>   void hmp_system_powerdown(Monitor *mon, const QDict *qdict);
>>   void hmp_cpu(Monitor *mon, const QDict *qdict);
>>   void hmp_memsave(Monitor *mon, const QDict *qdict);
>> diff --git a/hw/acpi/core.c b/hw/acpi/core.c
>> index 95fcac95a2..2ee64b6878 100644
>> --- a/hw/acpi/core.c
>> +++ b/hw/acpi/core.c
>> @@ -422,6 +422,14 @@ void acpi_pm1_evt_power_down(ACPIREGS *ar)
>>       }
>>   }
>>   
>> +void acpi_pm1_evt_sleep(ACPIREGS *ar)
>> +{
>> +    if (ar->pm1.evt.en & ACPI_BITMASK_SLEEP_BUTTON_ENABLE) {
>> +        ar->pm1.evt.sts |= ACPI_BITMASK_SLEEP_BUTTON_STATUS;
>> +        ar->tmr.update_sci(ar);
>> +    }
>> +}
>> +
>>   void acpi_pm1_evt_reset(ACPIREGS *ar)
>>   {
>>       ar->pm1.evt.sts = 0;
>> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
>> index c5d8646abc..3faeab4d2e 100644
>> --- a/hw/acpi/ich9.c
>> +++ b/hw/acpi/ich9.c
>> @@ -260,6 +260,14 @@ static void pm_reset(void *opaque)
>>       acpi_update_sci(&pm->acpi_regs, pm->irq);
>>   }
>>   
>> +static void pm_sleep_req(Notifier *n, void *opaque)
>> +{
>> +    ICH9LPCPMRegs *pm = container_of(n, ICH9LPCPMRegs, sleep_notifier);
>> +
>> +    acpi_pm1_evt_sleep(&pm->acpi_regs);
>> +}
>> +
>> +
>>   static void pm_powerdown_req(Notifier *n, void *opaque)
>>   {
>>       ICH9LPCPMRegs *pm = container_of(n, ICH9LPCPMRegs, powerdown_notifier);
>> @@ -299,6 +307,8 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
>>       qemu_register_reset(pm_reset, pm);
>>       pm->powerdown_notifier.notify = pm_powerdown_req;
>>       qemu_register_powerdown_notifier(&pm->powerdown_notifier);
>> +    pm->sleep_notifier.notify = pm_sleep_req;
>> +    qemu_register_sleep_notifier(&pm->sleep_notifier);
>>   
>>       legacy_acpi_cpu_hotplug_init(pci_address_space_io(lpc_pci),
>>           OBJECT(lpc_pci), &pm->gpe_cpu, ICH9_CPU_HOTPLUG_IO_BASE);
>> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
>> index f276967365..15e20976c3 100644
>> --- a/hw/acpi/piix4.c
>> +++ b/hw/acpi/piix4.c
>> @@ -79,6 +79,7 @@ typedef struct PIIX4PMState {
>>       int smm_enabled;
>>       Notifier machine_ready;
>>       Notifier powerdown_notifier;
>> +    Notifier sleep_notifier;
>>   
>>       AcpiPciHpState acpi_pci_hotplug;
>>       bool use_acpi_pci_hotplug;
>> @@ -371,6 +372,15 @@ static void piix4_pm_powerdown_req(Notifier *n, void *opaque)
>>       acpi_pm1_evt_power_down(&s->ar);
>>   }
>>   
>> +static void piix4_pm_sleep_req(Notifier *n, void *opaque)
>> +{
>> +    PIIX4PMState *s = container_of(n, PIIX4PMState, sleep_notifier);
>> +
>> +    assert(s != NULL);
>> +    acpi_pm1_evt_sleep(&s->ar);
>> +}
>> +
>> +
>>   static void piix4_device_plug_cb(HotplugHandler *hotplug_dev,
>>                                    DeviceState *dev, Error **errp)
>>   {
>> @@ -535,6 +545,8 @@ static void piix4_pm_realize(PCIDevice *dev, Error **errp)
>>   
>>       s->powerdown_notifier.notify = piix4_pm_powerdown_req;
>>       qemu_register_powerdown_notifier(&s->powerdown_notifier);
>> +    s->sleep_notifier.notify = piix4_pm_sleep_req;
>> +    qemu_register_sleep_notifier(&s->sleep_notifier);
>>   
>>       s->machine_ready.notify = piix4_pm_machine_ready;
>>       qemu_add_machine_init_done_notifier(&s->machine_ready);
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index 6b7bade183..06b28dacfe 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -294,7 +294,6 @@ static void fadt_setup(AcpiFadtDescriptorRev3 *fadt, AcpiPmInfo *pm)
>>       fadt->plvl3_lat = cpu_to_le16(0xfff); /* C3 state not supported */
>>       fadt->flags = cpu_to_le32((1 << ACPI_FADT_F_WBINVD) |
>>                                 (1 << ACPI_FADT_F_PROC_C1) |
>> -                              (1 << ACPI_FADT_F_SLP_BUTTON) |
>>                                 (1 << ACPI_FADT_F_RTC_S4));
>>       fadt->flags |= cpu_to_le32(1 << ACPI_FADT_F_USE_PLATFORM_CLOCK);
>>       /* APIC destination mode ("Flat Logical") has an upper limit of 8 CPUs
>> diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h
>> index 7b3d93cf0d..51cf901ef6 100644
>> --- a/include/hw/acpi/acpi.h
>> +++ b/include/hw/acpi/acpi.h
>> @@ -78,6 +78,7 @@
>>   #define ACPI_BITMASK_PM1_COMMON_ENABLED         ( \
>>           ACPI_BITMASK_RT_CLOCK_ENABLE        | \
>>           ACPI_BITMASK_POWER_BUTTON_ENABLE    | \
>> +        ACPI_BITMASK_SLEEP_BUTTON_ENABLE    | \
>>           ACPI_BITMASK_GLOBAL_LOCK_ENABLE     | \
>>           ACPI_BITMASK_TIMER_ENABLE)
>>   
>> @@ -148,6 +149,7 @@ void acpi_pm_tmr_reset(ACPIREGS *ar);
>>   /* PM1a_EVT: piix and ich9 don't implement PM1b. */
>>   uint16_t acpi_pm1_evt_get_sts(ACPIREGS *ar);
>>   void acpi_pm1_evt_power_down(ACPIREGS *ar);
>> +void acpi_pm1_evt_sleep(ACPIREGS *ar);
>>   void acpi_pm1_evt_reset(ACPIREGS *ar);
>>   void acpi_pm1_evt_init(ACPIREGS *ar, acpi_update_sci_fn update_sci,
>>                          MemoryRegion *parent);
>> diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
>> index a352c94fde..2073eec168 100644
>> --- a/include/hw/acpi/ich9.h
>> +++ b/include/hw/acpi/ich9.h
>> @@ -48,6 +48,7 @@ typedef struct ICH9LPCPMRegs {
>>   
>>       uint32_t pm_io_base;
>>       Notifier powerdown_notifier;
>> +    Notifier sleep_notifier;
>>   
>>       bool cpu_hotplug_legacy;
>>       AcpiCpuHotplug gpe_cpu;
>> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
>> index b21369672a..00f54653dc 100644
>> --- a/include/sysemu/sysemu.h
>> +++ b/include/sysemu/sysemu.h
>> @@ -75,6 +75,8 @@ void qemu_register_wakeup_notifier(Notifier *notifier);
>>   void qemu_system_shutdown_request(ShutdownCause reason);
>>   void qemu_system_powerdown_request(void);
>>   void qemu_register_powerdown_notifier(Notifier *notifier);
>> +void qemu_system_sleep_request(void);
>> +void qemu_register_sleep_notifier(Notifier *notifier);
>>   void qemu_system_debug_request(void);
>>   void qemu_system_vmstop_request(RunState reason);
>>   void qemu_system_vmstop_request_prepare(void);
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index 8b015bee2e..c6ccfcd70f 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -2314,6 +2314,18 @@
>>   { 'command': 'system_reset' }
>>   
>>   ##
>> +# @system_sleep:
>> +#
>> +# Requests that a guest perform a ACPI sleep transition by pushing a virtual
>> +# sleep button.
>> +#
>> +# Notes: A guest may or may not respond to this command.  This command
>> +#        returning does not indicate that a guest has accepted the request or
>> +#        that it has gone to sleep.
>> +##
>> +{ 'command': 'system_sleep' }
>> +
>> +##
>>   # @system_powerdown:
>>   #
>>   # Requests that a guest perform a powerdown operation.
>> diff --git a/qmp.c b/qmp.c
>> index b86201e349..bc1f2e3d7f 100644
>> --- a/qmp.c
>> +++ b/qmp.c
>> @@ -108,6 +108,11 @@ void qmp_system_reset(Error **errp)
>>       qemu_system_reset_request(SHUTDOWN_CAUSE_HOST_QMP);
>>   }
>>   
>> +void qmp_system_sleep(Error **erp)
>> +{
>> +    qemu_system_sleep_request();
>> +}
>> +
>>   void qmp_system_powerdown(Error **erp)
>>   {
>>       qemu_system_powerdown_request();
>> diff --git a/tests/test-hmp.c b/tests/test-hmp.c
>> index d77b3c8710..3fa850bf1e 100644
>> --- a/tests/test-hmp.c
>> +++ b/tests/test-hmp.c
>> @@ -67,6 +67,7 @@ static const char *hmp_cmds[] = {
>>       "sum 0 512",
>>       "x /8i 0x100",
>>       "xp /16x 0",
>> +    "system_sleep",
>>       NULL
>>   };
>>   
>> diff --git a/vl.c b/vl.c
>> index fb6b2efafa..6a0f847dcf 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -1608,6 +1608,7 @@ static ShutdownCause reset_requested;
>>   static ShutdownCause shutdown_requested;
>>   static int shutdown_signal;
>>   static pid_t shutdown_pid;
>> +static int sleep_requested;
>>   static int powerdown_requested;
>>   static int debug_requested;
>>   static int suspend_requested;
>> @@ -1618,6 +1619,8 @@ static NotifierList suspend_notifiers =
>>       NOTIFIER_LIST_INITIALIZER(suspend_notifiers);
>>   static NotifierList wakeup_notifiers =
>>       NOTIFIER_LIST_INITIALIZER(wakeup_notifiers);
>> +static NotifierList sleep_notifiers =
>> +    NOTIFIER_LIST_INITIALIZER(sleep_notifiers);
>>   static uint32_t wakeup_reason_mask = ~(1 << QEMU_WAKEUP_REASON_NONE);
>>   
>>   ShutdownCause qemu_shutdown_requested_get(void)
>> @@ -1838,6 +1841,24 @@ static void qemu_system_powerdown(void)
>>       notifier_list_notify(&powerdown_notifiers, NULL);
>>   }
>>   
>> +static void qemu_system_sleep(void)
>> +{
>> +    notifier_list_notify(&sleep_notifiers, NULL);
>> +}
>> +
>> +static int qemu_sleep_requested(void)
>> +{
>> +    int r = sleep_requested;
>> +    sleep_requested = 0;
>> +    return r;
>> +}
>> +
>> +void qemu_system_sleep_request(void)
>> +{
>> +    sleep_requested = 1;
>> +    qemu_notify_event();
>> +}
>> +
>>   void qemu_system_powerdown_request(void)
>>   {
>>       trace_qemu_system_powerdown_request();
>> @@ -1850,6 +1871,11 @@ void qemu_register_powerdown_notifier(Notifier *notifier)
>>       notifier_list_add(&powerdown_notifiers, notifier);
>>   }
>>   
>> +void qemu_register_sleep_notifier(Notifier *notifier)
>> +{
>> +    notifier_list_add(&sleep_notifiers, notifier);
>> +}
>> +
>>   void qemu_system_debug_request(void)
>>   {
>>       debug_requested = 1;
>> @@ -1899,6 +1925,9 @@ static bool main_loop_should_exit(void)
>>       if (qemu_powerdown_requested()) {
>>           qemu_system_powerdown();
>>       }
>> +    if (qemu_sleep_requested()) {
>> +        qemu_system_sleep();
>> +    }
>>       if (qemu_vmstop_requested(&r)) {
>>           vm_stop(r);
>>       }


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [PATCH] acpi: Add emulated sleep button
  2021-08-06 20:18   ` Annie.li
@ 2021-09-20  7:53     ` Igor Mammedov
  2023-07-07 17:43       ` Annie.li
  0 siblings, 1 reply; 10+ messages in thread
From: Igor Mammedov @ 2021-09-20  7:53 UTC (permalink / raw)
  To: Annie.li
  Cc: Eduardo Habkost, Michael S. Tsirkin, Markus Armbruster,
	qemu-devel, Paolo Bonzini, Stefan Fritsch,
	Dr. David Alan Gilbert, Richard Henderson

On Fri, 6 Aug 2021 16:18:09 -0400
"Annie.li" <annie.li@oracle.com> wrote:

> Hello Igor,
> 
> This is an old patch, but it does what we need.
> I am getting a little bit lost about not implementing fixed hardware
> sleep button, can you please clarify? thank you!
> 
> On 7/20/2017 10:59 AM, Igor Mammedov wrote:
> > On Thu, 20 Jul 2017 11:31:26 +0200
> > Stefan Fritsch <sf@sfritsch.de> wrote:
> >  
> >> From: Stefan Fritsch <stefan_fritsch@genua.de>
> >>
> >> Add an ACPI sleep button and QMP/HMP commands to trigger it.  A sleep
> >> button is a so called "fixed hardware feature", which makes it more
> >> suitable for putting the system to sleep than a laptop lid, for example.
> >>
> >> The sleep button is disabled by default (Bit 5 in the FACP flags
> >> register set and no button "device" present in SSDT/DSDT). Clearing said
> >> bit enables it as a fixed feature device.  
> > per spec sleep button is used for both putting system into
> > sleep and for waking it up.
> >
> > Reusing system_wakeup 'button' to behave as per spec would
> > make this patch significantly smaller.  
> 
> About reusing "system_wakeup", does it mean the following?
> 
> 1. when guest is in sleep state, "system_wakeup" wakes up the guest
> 2. when guest is running, "system_wakeup" puts the guest into sleep

yes,  it could be something like this


> "system_wakeup" sets WAK_STS and then system transitions to the
> working state. Correspondingly, I suppose both SLPBTN_STS and
> SLPBTN_EN need to be set for sleeping, and this is what fixed
> hardware sleep button requires?

yep
 
> I have combined the sleep and wakeup together, share the
> code between. But Xen also registers the wakeup notifier, and
> this makes things more complicated if this notifier is shared
> between sleep and wakeup. Or this is my misunderstanding?
> please correct me if I am wrong.

you'd have to fixup xen notifier to handle that
 
> > If you like idea of separate command/button better, then
> > I'd go generic control sleep button way instead of fixed
> > hardware, it would allow us to reuse most of the code in
> > ARM target, plus I'd avoid notifiers and use acpi device
> > lookup instead (see: qmp_query_acpi_ospm_status as example)
> >  
> Do you mean the "Control Method Sleep Button" that needs to
> notify OSPM by event indication 0x80?

yes, in addition to virt-arm machine it could be reused by
microvm which also uses 'reduced hardware' acpi model
(i.e. it lacks fixed hardware registers like virt-arm does)

maybe while adding button to pc/q35 you can look into adding
it to microvm and virt-arm at the same time (should be trivial
on top of pc/q35 support).

> Thanks
> Annie
> >> Signed-off-by: Stefan Fritsch <stefan_fritsch@genua.de>
> >> ---
> >>   hmp-commands.hx         | 16 ++++++++++++++++
> >>   hmp.c                   |  5 +++++
> >>   hmp.h                   |  1 +
> >>   hw/acpi/core.c          |  8 ++++++++
> >>   hw/acpi/ich9.c          | 10 ++++++++++
> >>   hw/acpi/piix4.c         | 12 ++++++++++++
> >>   hw/i386/acpi-build.c    |  1 -
> >>   include/hw/acpi/acpi.h  |  2 ++
> >>   include/hw/acpi/ich9.h  |  1 +
> >>   include/sysemu/sysemu.h |  2 ++
> >>   qapi-schema.json        | 12 ++++++++++++
> >>   qmp.c                   |  5 +++++
> >>   tests/test-hmp.c        |  1 +
> >>   vl.c                    | 29 +++++++++++++++++++++++++++++
> >>   14 files changed, 104 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/hmp-commands.hx b/hmp-commands.hx
> >> index 1941e19932..8ba4380883 100644
> >> --- a/hmp-commands.hx
> >> +++ b/hmp-commands.hx
> >> @@ -638,6 +638,22 @@ Reset the system.
> >>   ETEXI
> >>   
> >>       {
> >> +        .name       = "system_sleep",
> >> +        .args_type  = "",
> >> +        .params     = "",
> >> +        .help       = "send ACPI sleep event",
> >> +        .cmd = hmp_system_sleep,
> >> +    },
> >> +
> >> +STEXI
> >> +@item system_sleep
> >> +@findex system_sleep
> >> +
> >> +Push the virtual sleep button; if supported the system will enter
> >> +an ACPI sleep state.
> >> +ETEXI
> >> +
> >> +    {
> >>           .name       = "system_powerdown",
> >>           .args_type  = "",
> >>           .params     = "",
> >> diff --git a/hmp.c b/hmp.c
> >> index bf1de747d5..b4b584016c 100644
> >> --- a/hmp.c
> >> +++ b/hmp.c
> >> @@ -1047,6 +1047,11 @@ void hmp_system_reset(Monitor *mon, const QDict *qdict)
> >>       qmp_system_reset(NULL);
> >>   }
> >>   
> >> +void hmp_system_sleep(Monitor *mon, const QDict *qdict)
> >> +{
> >> +    qmp_system_sleep(NULL);
> >> +}
> >> +
> >>   void hmp_system_powerdown(Monitor *mon, const QDict *qdict)
> >>   {
> >>       qmp_system_powerdown(NULL);
> >> diff --git a/hmp.h b/hmp.h
> >> index 1ff455295e..15b53de9ed 100644
> >> --- a/hmp.h
> >> +++ b/hmp.h
> >> @@ -45,6 +45,7 @@ void hmp_info_iothreads(Monitor *mon, const QDict *qdict);
> >>   void hmp_quit(Monitor *mon, const QDict *qdict);
> >>   void hmp_stop(Monitor *mon, const QDict *qdict);
> >>   void hmp_system_reset(Monitor *mon, const QDict *qdict);
> >> +void hmp_system_sleep(Monitor *mon, const QDict *qdict);
> >>   void hmp_system_powerdown(Monitor *mon, const QDict *qdict);
> >>   void hmp_cpu(Monitor *mon, const QDict *qdict);
> >>   void hmp_memsave(Monitor *mon, const QDict *qdict);
> >> diff --git a/hw/acpi/core.c b/hw/acpi/core.c
> >> index 95fcac95a2..2ee64b6878 100644
> >> --- a/hw/acpi/core.c
> >> +++ b/hw/acpi/core.c
> >> @@ -422,6 +422,14 @@ void acpi_pm1_evt_power_down(ACPIREGS *ar)
> >>       }
> >>   }
> >>   
> >> +void acpi_pm1_evt_sleep(ACPIREGS *ar)
> >> +{
> >> +    if (ar->pm1.evt.en & ACPI_BITMASK_SLEEP_BUTTON_ENABLE) {
> >> +        ar->pm1.evt.sts |= ACPI_BITMASK_SLEEP_BUTTON_STATUS;
> >> +        ar->tmr.update_sci(ar);
> >> +    }
> >> +}
> >> +
> >>   void acpi_pm1_evt_reset(ACPIREGS *ar)
> >>   {
> >>       ar->pm1.evt.sts = 0;
> >> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> >> index c5d8646abc..3faeab4d2e 100644
> >> --- a/hw/acpi/ich9.c
> >> +++ b/hw/acpi/ich9.c
> >> @@ -260,6 +260,14 @@ static void pm_reset(void *opaque)
> >>       acpi_update_sci(&pm->acpi_regs, pm->irq);
> >>   }
> >>   
> >> +static void pm_sleep_req(Notifier *n, void *opaque)
> >> +{
> >> +    ICH9LPCPMRegs *pm = container_of(n, ICH9LPCPMRegs, sleep_notifier);
> >> +
> >> +    acpi_pm1_evt_sleep(&pm->acpi_regs);
> >> +}
> >> +
> >> +
> >>   static void pm_powerdown_req(Notifier *n, void *opaque)
> >>   {
> >>       ICH9LPCPMRegs *pm = container_of(n, ICH9LPCPMRegs, powerdown_notifier);
> >> @@ -299,6 +307,8 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
> >>       qemu_register_reset(pm_reset, pm);
> >>       pm->powerdown_notifier.notify = pm_powerdown_req;
> >>       qemu_register_powerdown_notifier(&pm->powerdown_notifier);
> >> +    pm->sleep_notifier.notify = pm_sleep_req;
> >> +    qemu_register_sleep_notifier(&pm->sleep_notifier);
> >>   
> >>       legacy_acpi_cpu_hotplug_init(pci_address_space_io(lpc_pci),
> >>           OBJECT(lpc_pci), &pm->gpe_cpu, ICH9_CPU_HOTPLUG_IO_BASE);
> >> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> >> index f276967365..15e20976c3 100644
> >> --- a/hw/acpi/piix4.c
> >> +++ b/hw/acpi/piix4.c
> >> @@ -79,6 +79,7 @@ typedef struct PIIX4PMState {
> >>       int smm_enabled;
> >>       Notifier machine_ready;
> >>       Notifier powerdown_notifier;
> >> +    Notifier sleep_notifier;
> >>   
> >>       AcpiPciHpState acpi_pci_hotplug;
> >>       bool use_acpi_pci_hotplug;
> >> @@ -371,6 +372,15 @@ static void piix4_pm_powerdown_req(Notifier *n, void *opaque)
> >>       acpi_pm1_evt_power_down(&s->ar);
> >>   }
> >>   
> >> +static void piix4_pm_sleep_req(Notifier *n, void *opaque)
> >> +{
> >> +    PIIX4PMState *s = container_of(n, PIIX4PMState, sleep_notifier);
> >> +
> >> +    assert(s != NULL);
> >> +    acpi_pm1_evt_sleep(&s->ar);
> >> +}
> >> +
> >> +
> >>   static void piix4_device_plug_cb(HotplugHandler *hotplug_dev,
> >>                                    DeviceState *dev, Error **errp)
> >>   {
> >> @@ -535,6 +545,8 @@ static void piix4_pm_realize(PCIDevice *dev, Error **errp)
> >>   
> >>       s->powerdown_notifier.notify = piix4_pm_powerdown_req;
> >>       qemu_register_powerdown_notifier(&s->powerdown_notifier);
> >> +    s->sleep_notifier.notify = piix4_pm_sleep_req;
> >> +    qemu_register_sleep_notifier(&s->sleep_notifier);
> >>   
> >>       s->machine_ready.notify = piix4_pm_machine_ready;
> >>       qemu_add_machine_init_done_notifier(&s->machine_ready);
> >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> >> index 6b7bade183..06b28dacfe 100644
> >> --- a/hw/i386/acpi-build.c
> >> +++ b/hw/i386/acpi-build.c
> >> @@ -294,7 +294,6 @@ static void fadt_setup(AcpiFadtDescriptorRev3 *fadt, AcpiPmInfo *pm)
> >>       fadt->plvl3_lat = cpu_to_le16(0xfff); /* C3 state not supported */
> >>       fadt->flags = cpu_to_le32((1 << ACPI_FADT_F_WBINVD) |
> >>                                 (1 << ACPI_FADT_F_PROC_C1) |
> >> -                              (1 << ACPI_FADT_F_SLP_BUTTON) |
> >>                                 (1 << ACPI_FADT_F_RTC_S4));
> >>       fadt->flags |= cpu_to_le32(1 << ACPI_FADT_F_USE_PLATFORM_CLOCK);
> >>       /* APIC destination mode ("Flat Logical") has an upper limit of 8 CPUs
> >> diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h
> >> index 7b3d93cf0d..51cf901ef6 100644
> >> --- a/include/hw/acpi/acpi.h
> >> +++ b/include/hw/acpi/acpi.h
> >> @@ -78,6 +78,7 @@
> >>   #define ACPI_BITMASK_PM1_COMMON_ENABLED         ( \
> >>           ACPI_BITMASK_RT_CLOCK_ENABLE        | \
> >>           ACPI_BITMASK_POWER_BUTTON_ENABLE    | \
> >> +        ACPI_BITMASK_SLEEP_BUTTON_ENABLE    | \
> >>           ACPI_BITMASK_GLOBAL_LOCK_ENABLE     | \
> >>           ACPI_BITMASK_TIMER_ENABLE)
> >>   
> >> @@ -148,6 +149,7 @@ void acpi_pm_tmr_reset(ACPIREGS *ar);
> >>   /* PM1a_EVT: piix and ich9 don't implement PM1b. */
> >>   uint16_t acpi_pm1_evt_get_sts(ACPIREGS *ar);
> >>   void acpi_pm1_evt_power_down(ACPIREGS *ar);
> >> +void acpi_pm1_evt_sleep(ACPIREGS *ar);
> >>   void acpi_pm1_evt_reset(ACPIREGS *ar);
> >>   void acpi_pm1_evt_init(ACPIREGS *ar, acpi_update_sci_fn update_sci,
> >>                          MemoryRegion *parent);
> >> diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
> >> index a352c94fde..2073eec168 100644
> >> --- a/include/hw/acpi/ich9.h
> >> +++ b/include/hw/acpi/ich9.h
> >> @@ -48,6 +48,7 @@ typedef struct ICH9LPCPMRegs {
> >>   
> >>       uint32_t pm_io_base;
> >>       Notifier powerdown_notifier;
> >> +    Notifier sleep_notifier;
> >>   
> >>       bool cpu_hotplug_legacy;
> >>       AcpiCpuHotplug gpe_cpu;
> >> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> >> index b21369672a..00f54653dc 100644
> >> --- a/include/sysemu/sysemu.h
> >> +++ b/include/sysemu/sysemu.h
> >> @@ -75,6 +75,8 @@ void qemu_register_wakeup_notifier(Notifier *notifier);
> >>   void qemu_system_shutdown_request(ShutdownCause reason);
> >>   void qemu_system_powerdown_request(void);
> >>   void qemu_register_powerdown_notifier(Notifier *notifier);
> >> +void qemu_system_sleep_request(void);
> >> +void qemu_register_sleep_notifier(Notifier *notifier);
> >>   void qemu_system_debug_request(void);
> >>   void qemu_system_vmstop_request(RunState reason);
> >>   void qemu_system_vmstop_request_prepare(void);
> >> diff --git a/qapi-schema.json b/qapi-schema.json
> >> index 8b015bee2e..c6ccfcd70f 100644
> >> --- a/qapi-schema.json
> >> +++ b/qapi-schema.json
> >> @@ -2314,6 +2314,18 @@
> >>   { 'command': 'system_reset' }
> >>   
> >>   ##
> >> +# @system_sleep:
> >> +#
> >> +# Requests that a guest perform a ACPI sleep transition by pushing a virtual
> >> +# sleep button.
> >> +#
> >> +# Notes: A guest may or may not respond to this command.  This command
> >> +#        returning does not indicate that a guest has accepted the request or
> >> +#        that it has gone to sleep.
> >> +##
> >> +{ 'command': 'system_sleep' }
> >> +
> >> +##
> >>   # @system_powerdown:
> >>   #
> >>   # Requests that a guest perform a powerdown operation.
> >> diff --git a/qmp.c b/qmp.c
> >> index b86201e349..bc1f2e3d7f 100644
> >> --- a/qmp.c
> >> +++ b/qmp.c
> >> @@ -108,6 +108,11 @@ void qmp_system_reset(Error **errp)
> >>       qemu_system_reset_request(SHUTDOWN_CAUSE_HOST_QMP);
> >>   }
> >>   
> >> +void qmp_system_sleep(Error **erp)
> >> +{
> >> +    qemu_system_sleep_request();
> >> +}
> >> +
> >>   void qmp_system_powerdown(Error **erp)
> >>   {
> >>       qemu_system_powerdown_request();
> >> diff --git a/tests/test-hmp.c b/tests/test-hmp.c
> >> index d77b3c8710..3fa850bf1e 100644
> >> --- a/tests/test-hmp.c
> >> +++ b/tests/test-hmp.c
> >> @@ -67,6 +67,7 @@ static const char *hmp_cmds[] = {
> >>       "sum 0 512",
> >>       "x /8i 0x100",
> >>       "xp /16x 0",
> >> +    "system_sleep",
> >>       NULL
> >>   };
> >>   
> >> diff --git a/vl.c b/vl.c
> >> index fb6b2efafa..6a0f847dcf 100644
> >> --- a/vl.c
> >> +++ b/vl.c
> >> @@ -1608,6 +1608,7 @@ static ShutdownCause reset_requested;
> >>   static ShutdownCause shutdown_requested;
> >>   static int shutdown_signal;
> >>   static pid_t shutdown_pid;
> >> +static int sleep_requested;
> >>   static int powerdown_requested;
> >>   static int debug_requested;
> >>   static int suspend_requested;
> >> @@ -1618,6 +1619,8 @@ static NotifierList suspend_notifiers =
> >>       NOTIFIER_LIST_INITIALIZER(suspend_notifiers);
> >>   static NotifierList wakeup_notifiers =
> >>       NOTIFIER_LIST_INITIALIZER(wakeup_notifiers);
> >> +static NotifierList sleep_notifiers =
> >> +    NOTIFIER_LIST_INITIALIZER(sleep_notifiers);
> >>   static uint32_t wakeup_reason_mask = ~(1 << QEMU_WAKEUP_REASON_NONE);
> >>   
> >>   ShutdownCause qemu_shutdown_requested_get(void)
> >> @@ -1838,6 +1841,24 @@ static void qemu_system_powerdown(void)
> >>       notifier_list_notify(&powerdown_notifiers, NULL);
> >>   }
> >>   
> >> +static void qemu_system_sleep(void)
> >> +{
> >> +    notifier_list_notify(&sleep_notifiers, NULL);
> >> +}
> >> +
> >> +static int qemu_sleep_requested(void)
> >> +{
> >> +    int r = sleep_requested;
> >> +    sleep_requested = 0;
> >> +    return r;
> >> +}
> >> +
> >> +void qemu_system_sleep_request(void)
> >> +{
> >> +    sleep_requested = 1;
> >> +    qemu_notify_event();
> >> +}
> >> +
> >>   void qemu_system_powerdown_request(void)
> >>   {
> >>       trace_qemu_system_powerdown_request();
> >> @@ -1850,6 +1871,11 @@ void qemu_register_powerdown_notifier(Notifier *notifier)
> >>       notifier_list_add(&powerdown_notifiers, notifier);
> >>   }
> >>   
> >> +void qemu_register_sleep_notifier(Notifier *notifier)
> >> +{
> >> +    notifier_list_add(&sleep_notifiers, notifier);
> >> +}
> >> +
> >>   void qemu_system_debug_request(void)
> >>   {
> >>       debug_requested = 1;
> >> @@ -1899,6 +1925,9 @@ static bool main_loop_should_exit(void)
> >>       if (qemu_powerdown_requested()) {
> >>           qemu_system_powerdown();
> >>       }
> >> +    if (qemu_sleep_requested()) {
> >> +        qemu_system_sleep();
> >> +    }
> >>       if (qemu_vmstop_requested(&r)) {
> >>           vm_stop(r);
> >>       }  
> 



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [PATCH] acpi: Add emulated sleep button
  2021-09-20  7:53     ` Igor Mammedov
@ 2023-07-07 17:43       ` Annie.li
  2023-07-14 14:04         ` Igor Mammedov
  0 siblings, 1 reply; 10+ messages in thread
From: Annie.li @ 2023-07-07 17:43 UTC (permalink / raw)
  To: Igor Mammedov
  Cc: Stefan Fritsch, qemu-devel, Eduardo Habkost, Michael S. Tsirkin,
	Dr. David Alan Gilbert, Markus Armbruster, Paolo Bonzini,
	Richard Henderson

Hi Igor,

Revisiting this thread and have more questions, please clarify, thank you!

On 9/20/2021 3:53 AM, Igor Mammedov wrote:
> On Fri, 6 Aug 2021 16:18:09 -0400
> "Annie.li" <annie.li@oracle.com> wrote:
>
>> Hello Igor,
>>
>> This is an old patch, but it does what we need.
>> I am getting a little bit lost about not implementing fixed hardware
>> sleep button, can you please clarify? thank you!
>>
>> On 7/20/2017 10:59 AM, Igor Mammedov wrote:
>>> On Thu, 20 Jul 2017 11:31:26 +0200
>>> Stefan Fritsch <sf@sfritsch.de> wrote:
>>>   
>>>> From: Stefan Fritsch <stefan_fritsch@genua.de>
>>>>
>>>> Add an ACPI sleep button and QMP/HMP commands to trigger it.  A sleep
>>>> button is a so called "fixed hardware feature", which makes it more
>>>> suitable for putting the system to sleep than a laptop lid, for example.
>>>>
>>>> The sleep button is disabled by default (Bit 5 in the FACP flags
>>>> register set and no button "device" present in SSDT/DSDT). Clearing said
>>>> bit enables it as a fixed feature device.
>>> per spec sleep button is used for both putting system into
>>> sleep and for waking it up.
>>>
>>> Reusing system_wakeup 'button' to behave as per spec would
>>> make this patch significantly smaller.
Current 'system_wakeup' sets the WAK_STS bit and PWRBTN_STS to wake up
the system, the system_wakeup 'button' is the power button. So(Correct me
if I am wrong) reusing the system_wakeup 'button' means reusing the power
button for sleep. See the following code of setting WAK_STS and PWRBTN_STS
for 'system_wakeup',
     case QEMU_WAKEUP_REASON_OTHER:
         /* ACPI_BITMASK_WAKE_STATUS should be set on resume.
            Pretend that resume was caused by power button */
         ar->pm1.evt.sts |=
             (ACPI_BITMASK_WAKE_STATUS | ACPI_BITMASK_POWER_BUTTON_STATUS);
         break;

Per the ACPI spec, the power button can be used in single button model, i.e.
it can be used to either shut down the system or put the system into sleep.
However, this depends on the software policy and user's setting of the 
system.
Sleep/shutdown can not be done through the power button in the same 
scenario.
If the system has configured pressing the power button to put the system 
into sleep,
system_sleep will put the system into sleep state through the power 
button, as well
as system_powerdown. Pressing the power button will not shut down the 
system.
In this case, system_powerdown has its own issue, but this is different 
story and
let's just focus on things related to system_sleep here.
>> About reusing "system_wakeup", does it mean the following?
>>
>> 1. when guest is in sleep state, "system_wakeup" wakes up the guest
>> 2. when guest is running, "system_wakeup" puts the guest into sleep
> yes,  it could be something like this
>
>
>> "system_wakeup" sets WAK_STS and then system transitions to the
>> working state. Correspondingly, I suppose both SLPBTN_STS and
>> SLPBTN_EN need to be set for sleeping, and this is what fixed
>> hardware sleep button requires?
> yep
>   
>> I have combined the sleep and wakeup together, share the
>> code between. But Xen also registers the wakeup notifier, and
>> this makes things more complicated if this notifier is shared
>> between sleep and wakeup. Or this is my misunderstanding?
>> please correct me if I am wrong.
> you'd have to fixup xen notifier to handle that
>   
>>> If you like idea of separate command/button better, then
>>> I'd go generic control sleep button way instead of fixed
>>> hardware, it would allow us to reuse most of the code in
>>> ARM target, plus I'd avoid notifiers and use acpi device
>>> lookup instead (see: qmp_query_acpi_ospm_status as example)
Implementing the generic control sleep button for x86 does align
to the generic control power button implementation in ARM, but
it doesn't align to the current fixed hardware power button for x86.
Should sleep button be implemented as generic control sleep button
to reuse code in ARM or fixed hardware button to align to the fixed
power button for x86?

Thanks
Annie
>>>   
>> Do you mean the "Control Method Sleep Button" that needs to
>> notify OSPM by event indication 0x80?
> yes, in addition to virt-arm machine it could be reused by
> microvm which also uses 'reduced hardware' acpi model
> (i.e. it lacks fixed hardware registers like virt-arm does)
>
> maybe while adding button to pc/q35 you can look into adding
> it to microvm and virt-arm at the same time (should be trivial
> on top of pc/q35 support).
>
>> Thanks
>> Annie
>>>> Signed-off-by: Stefan Fritsch <stefan_fritsch@genua.de>
>>>> ---
>>>>    hmp-commands.hx         | 16 ++++++++++++++++
>>>>    hmp.c                   |  5 +++++
>>>>    hmp.h                   |  1 +
>>>>    hw/acpi/core.c          |  8 ++++++++
>>>>    hw/acpi/ich9.c          | 10 ++++++++++
>>>>    hw/acpi/piix4.c         | 12 ++++++++++++
>>>>    hw/i386/acpi-build.c    |  1 -
>>>>    include/hw/acpi/acpi.h  |  2 ++
>>>>    include/hw/acpi/ich9.h  |  1 +
>>>>    include/sysemu/sysemu.h |  2 ++
>>>>    qapi-schema.json        | 12 ++++++++++++
>>>>    qmp.c                   |  5 +++++
>>>>    tests/test-hmp.c        |  1 +
>>>>    vl.c                    | 29 +++++++++++++++++++++++++++++
>>>>    14 files changed, 104 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>>>> index 1941e19932..8ba4380883 100644
>>>> --- a/hmp-commands.hx
>>>> +++ b/hmp-commands.hx
>>>> @@ -638,6 +638,22 @@ Reset the system.
>>>>    ETEXI
>>>>    
>>>>        {
>>>> +        .name       = "system_sleep",
>>>> +        .args_type  = "",
>>>> +        .params     = "",
>>>> +        .help       = "send ACPI sleep event",
>>>> +        .cmd = hmp_system_sleep,
>>>> +    },
>>>> +
>>>> +STEXI
>>>> +@item system_sleep
>>>> +@findex system_sleep
>>>> +
>>>> +Push the virtual sleep button; if supported the system will enter
>>>> +an ACPI sleep state.
>>>> +ETEXI
>>>> +
>>>> +    {
>>>>            .name       = "system_powerdown",
>>>>            .args_type  = "",
>>>>            .params     = "",
>>>> diff --git a/hmp.c b/hmp.c
>>>> index bf1de747d5..b4b584016c 100644
>>>> --- a/hmp.c
>>>> +++ b/hmp.c
>>>> @@ -1047,6 +1047,11 @@ void hmp_system_reset(Monitor *mon, const QDict *qdict)
>>>>        qmp_system_reset(NULL);
>>>>    }
>>>>    
>>>> +void hmp_system_sleep(Monitor *mon, const QDict *qdict)
>>>> +{
>>>> +    qmp_system_sleep(NULL);
>>>> +}
>>>> +
>>>>    void hmp_system_powerdown(Monitor *mon, const QDict *qdict)
>>>>    {
>>>>        qmp_system_powerdown(NULL);
>>>> diff --git a/hmp.h b/hmp.h
>>>> index 1ff455295e..15b53de9ed 100644
>>>> --- a/hmp.h
>>>> +++ b/hmp.h
>>>> @@ -45,6 +45,7 @@ void hmp_info_iothreads(Monitor *mon, const QDict *qdict);
>>>>    void hmp_quit(Monitor *mon, const QDict *qdict);
>>>>    void hmp_stop(Monitor *mon, const QDict *qdict);
>>>>    void hmp_system_reset(Monitor *mon, const QDict *qdict);
>>>> +void hmp_system_sleep(Monitor *mon, const QDict *qdict);
>>>>    void hmp_system_powerdown(Monitor *mon, const QDict *qdict);
>>>>    void hmp_cpu(Monitor *mon, const QDict *qdict);
>>>>    void hmp_memsave(Monitor *mon, const QDict *qdict);
>>>> diff --git a/hw/acpi/core.c b/hw/acpi/core.c
>>>> index 95fcac95a2..2ee64b6878 100644
>>>> --- a/hw/acpi/core.c
>>>> +++ b/hw/acpi/core.c
>>>> @@ -422,6 +422,14 @@ void acpi_pm1_evt_power_down(ACPIREGS *ar)
>>>>        }
>>>>    }
>>>>    
>>>> +void acpi_pm1_evt_sleep(ACPIREGS *ar)
>>>> +{
>>>> +    if (ar->pm1.evt.en & ACPI_BITMASK_SLEEP_BUTTON_ENABLE) {
>>>> +        ar->pm1.evt.sts |= ACPI_BITMASK_SLEEP_BUTTON_STATUS;
>>>> +        ar->tmr.update_sci(ar);
>>>> +    }
>>>> +}
>>>> +
>>>>    void acpi_pm1_evt_reset(ACPIREGS *ar)
>>>>    {
>>>>        ar->pm1.evt.sts = 0;
>>>> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
>>>> index c5d8646abc..3faeab4d2e 100644
>>>> --- a/hw/acpi/ich9.c
>>>> +++ b/hw/acpi/ich9.c
>>>> @@ -260,6 +260,14 @@ static void pm_reset(void *opaque)
>>>>        acpi_update_sci(&pm->acpi_regs, pm->irq);
>>>>    }
>>>>    
>>>> +static void pm_sleep_req(Notifier *n, void *opaque)
>>>> +{
>>>> +    ICH9LPCPMRegs *pm = container_of(n, ICH9LPCPMRegs, sleep_notifier);
>>>> +
>>>> +    acpi_pm1_evt_sleep(&pm->acpi_regs);
>>>> +}
>>>> +
>>>> +
>>>>    static void pm_powerdown_req(Notifier *n, void *opaque)
>>>>    {
>>>>        ICH9LPCPMRegs *pm = container_of(n, ICH9LPCPMRegs, powerdown_notifier);
>>>> @@ -299,6 +307,8 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
>>>>        qemu_register_reset(pm_reset, pm);
>>>>        pm->powerdown_notifier.notify = pm_powerdown_req;
>>>>        qemu_register_powerdown_notifier(&pm->powerdown_notifier);
>>>> +    pm->sleep_notifier.notify = pm_sleep_req;
>>>> +    qemu_register_sleep_notifier(&pm->sleep_notifier);
>>>>    
>>>>        legacy_acpi_cpu_hotplug_init(pci_address_space_io(lpc_pci),
>>>>            OBJECT(lpc_pci), &pm->gpe_cpu, ICH9_CPU_HOTPLUG_IO_BASE);
>>>> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
>>>> index f276967365..15e20976c3 100644
>>>> --- a/hw/acpi/piix4.c
>>>> +++ b/hw/acpi/piix4.c
>>>> @@ -79,6 +79,7 @@ typedef struct PIIX4PMState {
>>>>        int smm_enabled;
>>>>        Notifier machine_ready;
>>>>        Notifier powerdown_notifier;
>>>> +    Notifier sleep_notifier;
>>>>    
>>>>        AcpiPciHpState acpi_pci_hotplug;
>>>>        bool use_acpi_pci_hotplug;
>>>> @@ -371,6 +372,15 @@ static void piix4_pm_powerdown_req(Notifier *n, void *opaque)
>>>>        acpi_pm1_evt_power_down(&s->ar);
>>>>    }
>>>>    
>>>> +static void piix4_pm_sleep_req(Notifier *n, void *opaque)
>>>> +{
>>>> +    PIIX4PMState *s = container_of(n, PIIX4PMState, sleep_notifier);
>>>> +
>>>> +    assert(s != NULL);
>>>> +    acpi_pm1_evt_sleep(&s->ar);
>>>> +}
>>>> +
>>>> +
>>>>    static void piix4_device_plug_cb(HotplugHandler *hotplug_dev,
>>>>                                     DeviceState *dev, Error **errp)
>>>>    {
>>>> @@ -535,6 +545,8 @@ static void piix4_pm_realize(PCIDevice *dev, Error **errp)
>>>>    
>>>>        s->powerdown_notifier.notify = piix4_pm_powerdown_req;
>>>>        qemu_register_powerdown_notifier(&s->powerdown_notifier);
>>>> +    s->sleep_notifier.notify = piix4_pm_sleep_req;
>>>> +    qemu_register_sleep_notifier(&s->sleep_notifier);
>>>>    
>>>>        s->machine_ready.notify = piix4_pm_machine_ready;
>>>>        qemu_add_machine_init_done_notifier(&s->machine_ready);
>>>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>>>> index 6b7bade183..06b28dacfe 100644
>>>> --- a/hw/i386/acpi-build.c
>>>> +++ b/hw/i386/acpi-build.c
>>>> @@ -294,7 +294,6 @@ static void fadt_setup(AcpiFadtDescriptorRev3 *fadt, AcpiPmInfo *pm)
>>>>        fadt->plvl3_lat = cpu_to_le16(0xfff); /* C3 state not supported */
>>>>        fadt->flags = cpu_to_le32((1 << ACPI_FADT_F_WBINVD) |
>>>>                                  (1 << ACPI_FADT_F_PROC_C1) |
>>>> -                              (1 << ACPI_FADT_F_SLP_BUTTON) |
>>>>                                  (1 << ACPI_FADT_F_RTC_S4));
>>>>        fadt->flags |= cpu_to_le32(1 << ACPI_FADT_F_USE_PLATFORM_CLOCK);
>>>>        /* APIC destination mode ("Flat Logical") has an upper limit of 8 CPUs
>>>> diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h
>>>> index 7b3d93cf0d..51cf901ef6 100644
>>>> --- a/include/hw/acpi/acpi.h
>>>> +++ b/include/hw/acpi/acpi.h
>>>> @@ -78,6 +78,7 @@
>>>>    #define ACPI_BITMASK_PM1_COMMON_ENABLED         ( \
>>>>            ACPI_BITMASK_RT_CLOCK_ENABLE        | \
>>>>            ACPI_BITMASK_POWER_BUTTON_ENABLE    | \
>>>> +        ACPI_BITMASK_SLEEP_BUTTON_ENABLE    | \
>>>>            ACPI_BITMASK_GLOBAL_LOCK_ENABLE     | \
>>>>            ACPI_BITMASK_TIMER_ENABLE)
>>>>    
>>>> @@ -148,6 +149,7 @@ void acpi_pm_tmr_reset(ACPIREGS *ar);
>>>>    /* PM1a_EVT: piix and ich9 don't implement PM1b. */
>>>>    uint16_t acpi_pm1_evt_get_sts(ACPIREGS *ar);
>>>>    void acpi_pm1_evt_power_down(ACPIREGS *ar);
>>>> +void acpi_pm1_evt_sleep(ACPIREGS *ar);
>>>>    void acpi_pm1_evt_reset(ACPIREGS *ar);
>>>>    void acpi_pm1_evt_init(ACPIREGS *ar, acpi_update_sci_fn update_sci,
>>>>                           MemoryRegion *parent);
>>>> diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
>>>> index a352c94fde..2073eec168 100644
>>>> --- a/include/hw/acpi/ich9.h
>>>> +++ b/include/hw/acpi/ich9.h
>>>> @@ -48,6 +48,7 @@ typedef struct ICH9LPCPMRegs {
>>>>    
>>>>        uint32_t pm_io_base;
>>>>        Notifier powerdown_notifier;
>>>> +    Notifier sleep_notifier;
>>>>    
>>>>        bool cpu_hotplug_legacy;
>>>>        AcpiCpuHotplug gpe_cpu;
>>>> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
>>>> index b21369672a..00f54653dc 100644
>>>> --- a/include/sysemu/sysemu.h
>>>> +++ b/include/sysemu/sysemu.h
>>>> @@ -75,6 +75,8 @@ void qemu_register_wakeup_notifier(Notifier *notifier);
>>>>    void qemu_system_shutdown_request(ShutdownCause reason);
>>>>    void qemu_system_powerdown_request(void);
>>>>    void qemu_register_powerdown_notifier(Notifier *notifier);
>>>> +void qemu_system_sleep_request(void);
>>>> +void qemu_register_sleep_notifier(Notifier *notifier);
>>>>    void qemu_system_debug_request(void);
>>>>    void qemu_system_vmstop_request(RunState reason);
>>>>    void qemu_system_vmstop_request_prepare(void);
>>>> diff --git a/qapi-schema.json b/qapi-schema.json
>>>> index 8b015bee2e..c6ccfcd70f 100644
>>>> --- a/qapi-schema.json
>>>> +++ b/qapi-schema.json
>>>> @@ -2314,6 +2314,18 @@
>>>>    { 'command': 'system_reset' }
>>>>    
>>>>    ##
>>>> +# @system_sleep:
>>>> +#
>>>> +# Requests that a guest perform a ACPI sleep transition by pushing a virtual
>>>> +# sleep button.
>>>> +#
>>>> +# Notes: A guest may or may not respond to this command.  This command
>>>> +#        returning does not indicate that a guest has accepted the request or
>>>> +#        that it has gone to sleep.
>>>> +##
>>>> +{ 'command': 'system_sleep' }
>>>> +
>>>> +##
>>>>    # @system_powerdown:
>>>>    #
>>>>    # Requests that a guest perform a powerdown operation.
>>>> diff --git a/qmp.c b/qmp.c
>>>> index b86201e349..bc1f2e3d7f 100644
>>>> --- a/qmp.c
>>>> +++ b/qmp.c
>>>> @@ -108,6 +108,11 @@ void qmp_system_reset(Error **errp)
>>>>        qemu_system_reset_request(SHUTDOWN_CAUSE_HOST_QMP);
>>>>    }
>>>>    
>>>> +void qmp_system_sleep(Error **erp)
>>>> +{
>>>> +    qemu_system_sleep_request();
>>>> +}
>>>> +
>>>>    void qmp_system_powerdown(Error **erp)
>>>>    {
>>>>        qemu_system_powerdown_request();
>>>> diff --git a/tests/test-hmp.c b/tests/test-hmp.c
>>>> index d77b3c8710..3fa850bf1e 100644
>>>> --- a/tests/test-hmp.c
>>>> +++ b/tests/test-hmp.c
>>>> @@ -67,6 +67,7 @@ static const char *hmp_cmds[] = {
>>>>        "sum 0 512",
>>>>        "x /8i 0x100",
>>>>        "xp /16x 0",
>>>> +    "system_sleep",
>>>>        NULL
>>>>    };
>>>>    
>>>> diff --git a/vl.c b/vl.c
>>>> index fb6b2efafa..6a0f847dcf 100644
>>>> --- a/vl.c
>>>> +++ b/vl.c
>>>> @@ -1608,6 +1608,7 @@ static ShutdownCause reset_requested;
>>>>    static ShutdownCause shutdown_requested;
>>>>    static int shutdown_signal;
>>>>    static pid_t shutdown_pid;
>>>> +static int sleep_requested;
>>>>    static int powerdown_requested;
>>>>    static int debug_requested;
>>>>    static int suspend_requested;
>>>> @@ -1618,6 +1619,8 @@ static NotifierList suspend_notifiers =
>>>>        NOTIFIER_LIST_INITIALIZER(suspend_notifiers);
>>>>    static NotifierList wakeup_notifiers =
>>>>        NOTIFIER_LIST_INITIALIZER(wakeup_notifiers);
>>>> +static NotifierList sleep_notifiers =
>>>> +    NOTIFIER_LIST_INITIALIZER(sleep_notifiers);
>>>>    static uint32_t wakeup_reason_mask = ~(1 << QEMU_WAKEUP_REASON_NONE);
>>>>    
>>>>    ShutdownCause qemu_shutdown_requested_get(void)
>>>> @@ -1838,6 +1841,24 @@ static void qemu_system_powerdown(void)
>>>>        notifier_list_notify(&powerdown_notifiers, NULL);
>>>>    }
>>>>    
>>>> +static void qemu_system_sleep(void)
>>>> +{
>>>> +    notifier_list_notify(&sleep_notifiers, NULL);
>>>> +}
>>>> +
>>>> +static int qemu_sleep_requested(void)
>>>> +{
>>>> +    int r = sleep_requested;
>>>> +    sleep_requested = 0;
>>>> +    return r;
>>>> +}
>>>> +
>>>> +void qemu_system_sleep_request(void)
>>>> +{
>>>> +    sleep_requested = 1;
>>>> +    qemu_notify_event();
>>>> +}
>>>> +
>>>>    void qemu_system_powerdown_request(void)
>>>>    {
>>>>        trace_qemu_system_powerdown_request();
>>>> @@ -1850,6 +1871,11 @@ void qemu_register_powerdown_notifier(Notifier *notifier)
>>>>        notifier_list_add(&powerdown_notifiers, notifier);
>>>>    }
>>>>    
>>>> +void qemu_register_sleep_notifier(Notifier *notifier)
>>>> +{
>>>> +    notifier_list_add(&sleep_notifiers, notifier);
>>>> +}
>>>> +
>>>>    void qemu_system_debug_request(void)
>>>>    {
>>>>        debug_requested = 1;
>>>> @@ -1899,6 +1925,9 @@ static bool main_loop_should_exit(void)
>>>>        if (qemu_powerdown_requested()) {
>>>>            qemu_system_powerdown();
>>>>        }
>>>> +    if (qemu_sleep_requested()) {
>>>> +        qemu_system_sleep();
>>>> +    }
>>>>        if (qemu_vmstop_requested(&r)) {
>>>>            vm_stop(r);
>>>>        }
>


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [PATCH] acpi: Add emulated sleep button
  2023-07-07 17:43       ` Annie.li
@ 2023-07-14 14:04         ` Igor Mammedov
  2023-08-01 17:00           ` Annie.li
  0 siblings, 1 reply; 10+ messages in thread
From: Igor Mammedov @ 2023-07-14 14:04 UTC (permalink / raw)
  To: Annie.li
  Cc: Stefan Fritsch, qemu-devel, Eduardo Habkost, Michael S. Tsirkin,
	Dr. David Alan Gilbert, Markus Armbruster, Paolo Bonzini,
	Richard Henderson

On Fri, 7 Jul 2023 13:43:36 -0400
"Annie.li" <annie.li@oracle.com> wrote:

> Hi Igor,
> 
> Revisiting this thread and have more questions, please clarify, thank you!
> 
> On 9/20/2021 3:53 AM, Igor Mammedov wrote:
> > On Fri, 6 Aug 2021 16:18:09 -0400
> > "Annie.li" <annie.li@oracle.com> wrote:
> >  
> >> Hello Igor,
> >>
> >> This is an old patch, but it does what we need.
> >> I am getting a little bit lost about not implementing fixed hardware
> >> sleep button, can you please clarify? thank you!
> >>
> >> On 7/20/2017 10:59 AM, Igor Mammedov wrote:  
> >>> On Thu, 20 Jul 2017 11:31:26 +0200
> >>> Stefan Fritsch <sf@sfritsch.de> wrote:
> >>>     
> >>>> From: Stefan Fritsch <stefan_fritsch@genua.de>
> >>>>
> >>>> Add an ACPI sleep button and QMP/HMP commands to trigger it.  A sleep
> >>>> button is a so called "fixed hardware feature", which makes it more
> >>>> suitable for putting the system to sleep than a laptop lid, for example.
> >>>>
> >>>> The sleep button is disabled by default (Bit 5 in the FACP flags
> >>>> register set and no button "device" present in SSDT/DSDT). Clearing said
> >>>> bit enables it as a fixed feature device.  
> >>> per spec sleep button is used for both putting system into
> >>> sleep and for waking it up.
> >>>
> >>> Reusing system_wakeup 'button' to behave as per spec would
> >>> make this patch significantly smaller.  
> Current 'system_wakeup' sets the WAK_STS bit and PWRBTN_STS to wake up
> the system, the system_wakeup 'button' is the power button. So(Correct me
> if I am wrong) reusing the system_wakeup 'button' means reusing the power
> button for sleep. See the following code of setting WAK_STS and PWRBTN_STS
> for 'system_wakeup',
>      case QEMU_WAKEUP_REASON_OTHER:
>          /* ACPI_BITMASK_WAKE_STATUS should be set on resume.
>             Pretend that resume was caused by power button */
>          ar->pm1.evt.sts |=
>              (ACPI_BITMASK_WAKE_STATUS | ACPI_BITMASK_POWER_BUTTON_STATUS);

(that's quite ancient code (0bacd1300d) and I couldn't find a reason why
power button was chosen.
 https://www.mail-archive.com/kvm@vger.kernel.org/msg05742.html
that was tested with WinXP so would be wise to check if SLEEP button
works there. (though I'm not sure if we still care for XP being runnable on QEMU)
)

it doesn't have to be ACPI_BITMASK_POWER_BUTTON_STATUS,
you can convert it to sleep button by using ACPI_BITMASK_SLEEP_BUTTON_STATUS.
However you'll have to enable sleep button (which is disabled currently)
in FADT (ACPI_FADT_F_SLP_BUTTON), for guest to see the button. 

>          break;
> 
> Per the ACPI spec, the power button can be used in single button model, i.e.
> it can be used to either shut down the system or put the system into sleep.
> However, this depends on the software policy and user's setting of the 
> system.
> Sleep/shutdown can not be done through the power button in the same 
> scenario.
> If the system has configured pressing the power button to put the system 
> into sleep,
> system_sleep will put the system into sleep state through the power 
> button, as well
> as system_powerdown. Pressing the power button will not shut down the 
> system.
> In this case, system_powerdown has its own issue, but this is different 
> story and
> let's just focus on things related to system_sleep here.

regardless of what button is used OSPM is free to enable or disable it
using FOO_EN bits.

> >> About reusing "system_wakeup", does it mean the following?
> >>
> >> 1. when guest is in sleep state, "system_wakeup" wakes up the guest
> >> 2. when guest is running, "system_wakeup" puts the guest into sleep  
> > yes,  it could be something like this
> >
> >  
> >> "system_wakeup" sets WAK_STS and then system transitions to the
> >> working state. Correspondingly, I suppose both SLPBTN_STS and
> >> SLPBTN_EN need to be set for sleeping, and this is what fixed
> >> hardware sleep button requires?  
> > yep
> >     
> >> I have combined the sleep and wakeup together, share the
> >> code between. But Xen also registers the wakeup notifier, and
> >> this makes things more complicated if this notifier is shared
> >> between sleep and wakeup. Or this is my misunderstanding?
> >> please correct me if I am wrong.  
> > you'd have to fixup xen notifier to handle that
> >     
> >>> If you like idea of separate command/button better, then
> >>> I'd go generic control sleep button way instead of fixed
> >>> hardware, it would allow us to reuse most of the code in
> >>> ARM target, plus I'd avoid notifiers and use acpi device
> >>> lookup instead (see: qmp_query_acpi_ospm_status as example)
  
> Implementing the generic control sleep button for x86 does align
> to the generic control power button implementation in ARM, but
> it doesn't align to the current fixed hardware power button for x86.
> Should sleep button be implemented as generic control sleep button
> to reuse code in ARM or fixed hardware button to align to the fixed
> power button for x86?

sleep control button device was present in ACPI 1.0b so it might be
supported by x86 as well (one just need to test it with ancient OS
to find out if it were implemented. (WinXP,Vista,some ancient linux...))

> Thanks
> Annie
> >>>     
> >> Do you mean the "Control Method Sleep Button" that needs to
> >> notify OSPM by event indication 0x80?  
> > yes, in addition to virt-arm machine it could be reused by
> > microvm which also uses 'reduced hardware' acpi model
> > (i.e. it lacks fixed hardware registers like virt-arm does)
> >
> > maybe while adding button to pc/q35 you can look into adding
> > it to microvm and virt-arm at the same time (should be trivial
> > on top of pc/q35 support).
> >  
> >> Thanks
> >> Annie  
> >>>> Signed-off-by: Stefan Fritsch <stefan_fritsch@genua.de>
> >>>> ---
> >>>>    hmp-commands.hx         | 16 ++++++++++++++++
> >>>>    hmp.c                   |  5 +++++
> >>>>    hmp.h                   |  1 +
> >>>>    hw/acpi/core.c          |  8 ++++++++
> >>>>    hw/acpi/ich9.c          | 10 ++++++++++
> >>>>    hw/acpi/piix4.c         | 12 ++++++++++++
> >>>>    hw/i386/acpi-build.c    |  1 -
> >>>>    include/hw/acpi/acpi.h  |  2 ++
> >>>>    include/hw/acpi/ich9.h  |  1 +
> >>>>    include/sysemu/sysemu.h |  2 ++
> >>>>    qapi-schema.json        | 12 ++++++++++++
> >>>>    qmp.c                   |  5 +++++
> >>>>    tests/test-hmp.c        |  1 +
> >>>>    vl.c                    | 29 +++++++++++++++++++++++++++++
> >>>>    14 files changed, 104 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/hmp-commands.hx b/hmp-commands.hx
> >>>> index 1941e19932..8ba4380883 100644
> >>>> --- a/hmp-commands.hx
> >>>> +++ b/hmp-commands.hx
> >>>> @@ -638,6 +638,22 @@ Reset the system.
> >>>>    ETEXI
> >>>>    
> >>>>        {
> >>>> +        .name       = "system_sleep",
> >>>> +        .args_type  = "",
> >>>> +        .params     = "",
> >>>> +        .help       = "send ACPI sleep event",
> >>>> +        .cmd = hmp_system_sleep,
> >>>> +    },
> >>>> +
> >>>> +STEXI
> >>>> +@item system_sleep
> >>>> +@findex system_sleep
> >>>> +
> >>>> +Push the virtual sleep button; if supported the system will enter
> >>>> +an ACPI sleep state.
> >>>> +ETEXI
> >>>> +
> >>>> +    {
> >>>>            .name       = "system_powerdown",
> >>>>            .args_type  = "",
> >>>>            .params     = "",
> >>>> diff --git a/hmp.c b/hmp.c
> >>>> index bf1de747d5..b4b584016c 100644
> >>>> --- a/hmp.c
> >>>> +++ b/hmp.c
> >>>> @@ -1047,6 +1047,11 @@ void hmp_system_reset(Monitor *mon, const QDict *qdict)
> >>>>        qmp_system_reset(NULL);
> >>>>    }
> >>>>    
> >>>> +void hmp_system_sleep(Monitor *mon, const QDict *qdict)
> >>>> +{
> >>>> +    qmp_system_sleep(NULL);
> >>>> +}
> >>>> +
> >>>>    void hmp_system_powerdown(Monitor *mon, const QDict *qdict)
> >>>>    {
> >>>>        qmp_system_powerdown(NULL);
> >>>> diff --git a/hmp.h b/hmp.h
> >>>> index 1ff455295e..15b53de9ed 100644
> >>>> --- a/hmp.h
> >>>> +++ b/hmp.h
> >>>> @@ -45,6 +45,7 @@ void hmp_info_iothreads(Monitor *mon, const QDict *qdict);
> >>>>    void hmp_quit(Monitor *mon, const QDict *qdict);
> >>>>    void hmp_stop(Monitor *mon, const QDict *qdict);
> >>>>    void hmp_system_reset(Monitor *mon, const QDict *qdict);
> >>>> +void hmp_system_sleep(Monitor *mon, const QDict *qdict);
> >>>>    void hmp_system_powerdown(Monitor *mon, const QDict *qdict);
> >>>>    void hmp_cpu(Monitor *mon, const QDict *qdict);
> >>>>    void hmp_memsave(Monitor *mon, const QDict *qdict);
> >>>> diff --git a/hw/acpi/core.c b/hw/acpi/core.c
> >>>> index 95fcac95a2..2ee64b6878 100644
> >>>> --- a/hw/acpi/core.c
> >>>> +++ b/hw/acpi/core.c
> >>>> @@ -422,6 +422,14 @@ void acpi_pm1_evt_power_down(ACPIREGS *ar)
> >>>>        }
> >>>>    }
> >>>>    
> >>>> +void acpi_pm1_evt_sleep(ACPIREGS *ar)
> >>>> +{
> >>>> +    if (ar->pm1.evt.en & ACPI_BITMASK_SLEEP_BUTTON_ENABLE) {
> >>>> +        ar->pm1.evt.sts |= ACPI_BITMASK_SLEEP_BUTTON_STATUS;
> >>>> +        ar->tmr.update_sci(ar);
> >>>> +    }
> >>>> +}
> >>>> +
> >>>>    void acpi_pm1_evt_reset(ACPIREGS *ar)
> >>>>    {
> >>>>        ar->pm1.evt.sts = 0;
> >>>> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> >>>> index c5d8646abc..3faeab4d2e 100644
> >>>> --- a/hw/acpi/ich9.c
> >>>> +++ b/hw/acpi/ich9.c
> >>>> @@ -260,6 +260,14 @@ static void pm_reset(void *opaque)
> >>>>        acpi_update_sci(&pm->acpi_regs, pm->irq);
> >>>>    }
> >>>>    
> >>>> +static void pm_sleep_req(Notifier *n, void *opaque)
> >>>> +{
> >>>> +    ICH9LPCPMRegs *pm = container_of(n, ICH9LPCPMRegs, sleep_notifier);
> >>>> +
> >>>> +    acpi_pm1_evt_sleep(&pm->acpi_regs);
> >>>> +}
> >>>> +
> >>>> +
> >>>>    static void pm_powerdown_req(Notifier *n, void *opaque)
> >>>>    {
> >>>>        ICH9LPCPMRegs *pm = container_of(n, ICH9LPCPMRegs, powerdown_notifier);
> >>>> @@ -299,6 +307,8 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
> >>>>        qemu_register_reset(pm_reset, pm);
> >>>>        pm->powerdown_notifier.notify = pm_powerdown_req;
> >>>>        qemu_register_powerdown_notifier(&pm->powerdown_notifier);
> >>>> +    pm->sleep_notifier.notify = pm_sleep_req;
> >>>> +    qemu_register_sleep_notifier(&pm->sleep_notifier);
> >>>>    
> >>>>        legacy_acpi_cpu_hotplug_init(pci_address_space_io(lpc_pci),
> >>>>            OBJECT(lpc_pci), &pm->gpe_cpu, ICH9_CPU_HOTPLUG_IO_BASE);
> >>>> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> >>>> index f276967365..15e20976c3 100644
> >>>> --- a/hw/acpi/piix4.c
> >>>> +++ b/hw/acpi/piix4.c
> >>>> @@ -79,6 +79,7 @@ typedef struct PIIX4PMState {
> >>>>        int smm_enabled;
> >>>>        Notifier machine_ready;
> >>>>        Notifier powerdown_notifier;
> >>>> +    Notifier sleep_notifier;
> >>>>    
> >>>>        AcpiPciHpState acpi_pci_hotplug;
> >>>>        bool use_acpi_pci_hotplug;
> >>>> @@ -371,6 +372,15 @@ static void piix4_pm_powerdown_req(Notifier *n, void *opaque)
> >>>>        acpi_pm1_evt_power_down(&s->ar);
> >>>>    }
> >>>>    
> >>>> +static void piix4_pm_sleep_req(Notifier *n, void *opaque)
> >>>> +{
> >>>> +    PIIX4PMState *s = container_of(n, PIIX4PMState, sleep_notifier);
> >>>> +
> >>>> +    assert(s != NULL);
> >>>> +    acpi_pm1_evt_sleep(&s->ar);
> >>>> +}
> >>>> +
> >>>> +
> >>>>    static void piix4_device_plug_cb(HotplugHandler *hotplug_dev,
> >>>>                                     DeviceState *dev, Error **errp)
> >>>>    {
> >>>> @@ -535,6 +545,8 @@ static void piix4_pm_realize(PCIDevice *dev, Error **errp)
> >>>>    
> >>>>        s->powerdown_notifier.notify = piix4_pm_powerdown_req;
> >>>>        qemu_register_powerdown_notifier(&s->powerdown_notifier);
> >>>> +    s->sleep_notifier.notify = piix4_pm_sleep_req;
> >>>> +    qemu_register_sleep_notifier(&s->sleep_notifier);
> >>>>    
> >>>>        s->machine_ready.notify = piix4_pm_machine_ready;
> >>>>        qemu_add_machine_init_done_notifier(&s->machine_ready);
> >>>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> >>>> index 6b7bade183..06b28dacfe 100644
> >>>> --- a/hw/i386/acpi-build.c
> >>>> +++ b/hw/i386/acpi-build.c
> >>>> @@ -294,7 +294,6 @@ static void fadt_setup(AcpiFadtDescriptorRev3 *fadt, AcpiPmInfo *pm)
> >>>>        fadt->plvl3_lat = cpu_to_le16(0xfff); /* C3 state not supported */
> >>>>        fadt->flags = cpu_to_le32((1 << ACPI_FADT_F_WBINVD) |
> >>>>                                  (1 << ACPI_FADT_F_PROC_C1) |
> >>>> -                              (1 << ACPI_FADT_F_SLP_BUTTON) |
> >>>>                                  (1 << ACPI_FADT_F_RTC_S4));
> >>>>        fadt->flags |= cpu_to_le32(1 << ACPI_FADT_F_USE_PLATFORM_CLOCK);
> >>>>        /* APIC destination mode ("Flat Logical") has an upper limit of 8 CPUs
> >>>> diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h
> >>>> index 7b3d93cf0d..51cf901ef6 100644
> >>>> --- a/include/hw/acpi/acpi.h
> >>>> +++ b/include/hw/acpi/acpi.h
> >>>> @@ -78,6 +78,7 @@
> >>>>    #define ACPI_BITMASK_PM1_COMMON_ENABLED         ( \
> >>>>            ACPI_BITMASK_RT_CLOCK_ENABLE        | \
> >>>>            ACPI_BITMASK_POWER_BUTTON_ENABLE    | \
> >>>> +        ACPI_BITMASK_SLEEP_BUTTON_ENABLE    | \
> >>>>            ACPI_BITMASK_GLOBAL_LOCK_ENABLE     | \
> >>>>            ACPI_BITMASK_TIMER_ENABLE)
> >>>>    
> >>>> @@ -148,6 +149,7 @@ void acpi_pm_tmr_reset(ACPIREGS *ar);
> >>>>    /* PM1a_EVT: piix and ich9 don't implement PM1b. */
> >>>>    uint16_t acpi_pm1_evt_get_sts(ACPIREGS *ar);
> >>>>    void acpi_pm1_evt_power_down(ACPIREGS *ar);
> >>>> +void acpi_pm1_evt_sleep(ACPIREGS *ar);
> >>>>    void acpi_pm1_evt_reset(ACPIREGS *ar);
> >>>>    void acpi_pm1_evt_init(ACPIREGS *ar, acpi_update_sci_fn update_sci,
> >>>>                           MemoryRegion *parent);
> >>>> diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
> >>>> index a352c94fde..2073eec168 100644
> >>>> --- a/include/hw/acpi/ich9.h
> >>>> +++ b/include/hw/acpi/ich9.h
> >>>> @@ -48,6 +48,7 @@ typedef struct ICH9LPCPMRegs {
> >>>>    
> >>>>        uint32_t pm_io_base;
> >>>>        Notifier powerdown_notifier;
> >>>> +    Notifier sleep_notifier;
> >>>>    
> >>>>        bool cpu_hotplug_legacy;
> >>>>        AcpiCpuHotplug gpe_cpu;
> >>>> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> >>>> index b21369672a..00f54653dc 100644
> >>>> --- a/include/sysemu/sysemu.h
> >>>> +++ b/include/sysemu/sysemu.h
> >>>> @@ -75,6 +75,8 @@ void qemu_register_wakeup_notifier(Notifier *notifier);
> >>>>    void qemu_system_shutdown_request(ShutdownCause reason);
> >>>>    void qemu_system_powerdown_request(void);
> >>>>    void qemu_register_powerdown_notifier(Notifier *notifier);
> >>>> +void qemu_system_sleep_request(void);
> >>>> +void qemu_register_sleep_notifier(Notifier *notifier);
> >>>>    void qemu_system_debug_request(void);
> >>>>    void qemu_system_vmstop_request(RunState reason);
> >>>>    void qemu_system_vmstop_request_prepare(void);
> >>>> diff --git a/qapi-schema.json b/qapi-schema.json
> >>>> index 8b015bee2e..c6ccfcd70f 100644
> >>>> --- a/qapi-schema.json
> >>>> +++ b/qapi-schema.json
> >>>> @@ -2314,6 +2314,18 @@
> >>>>    { 'command': 'system_reset' }
> >>>>    
> >>>>    ##
> >>>> +# @system_sleep:
> >>>> +#
> >>>> +# Requests that a guest perform a ACPI sleep transition by pushing a virtual
> >>>> +# sleep button.
> >>>> +#
> >>>> +# Notes: A guest may or may not respond to this command.  This command
> >>>> +#        returning does not indicate that a guest has accepted the request or
> >>>> +#        that it has gone to sleep.
> >>>> +##
> >>>> +{ 'command': 'system_sleep' }
> >>>> +
> >>>> +##
> >>>>    # @system_powerdown:
> >>>>    #
> >>>>    # Requests that a guest perform a powerdown operation.
> >>>> diff --git a/qmp.c b/qmp.c
> >>>> index b86201e349..bc1f2e3d7f 100644
> >>>> --- a/qmp.c
> >>>> +++ b/qmp.c
> >>>> @@ -108,6 +108,11 @@ void qmp_system_reset(Error **errp)
> >>>>        qemu_system_reset_request(SHUTDOWN_CAUSE_HOST_QMP);
> >>>>    }
> >>>>    
> >>>> +void qmp_system_sleep(Error **erp)
> >>>> +{
> >>>> +    qemu_system_sleep_request();
> >>>> +}
> >>>> +
> >>>>    void qmp_system_powerdown(Error **erp)
> >>>>    {
> >>>>        qemu_system_powerdown_request();
> >>>> diff --git a/tests/test-hmp.c b/tests/test-hmp.c
> >>>> index d77b3c8710..3fa850bf1e 100644
> >>>> --- a/tests/test-hmp.c
> >>>> +++ b/tests/test-hmp.c
> >>>> @@ -67,6 +67,7 @@ static const char *hmp_cmds[] = {
> >>>>        "sum 0 512",
> >>>>        "x /8i 0x100",
> >>>>        "xp /16x 0",
> >>>> +    "system_sleep",
> >>>>        NULL
> >>>>    };
> >>>>    
> >>>> diff --git a/vl.c b/vl.c
> >>>> index fb6b2efafa..6a0f847dcf 100644
> >>>> --- a/vl.c
> >>>> +++ b/vl.c
> >>>> @@ -1608,6 +1608,7 @@ static ShutdownCause reset_requested;
> >>>>    static ShutdownCause shutdown_requested;
> >>>>    static int shutdown_signal;
> >>>>    static pid_t shutdown_pid;
> >>>> +static int sleep_requested;
> >>>>    static int powerdown_requested;
> >>>>    static int debug_requested;
> >>>>    static int suspend_requested;
> >>>> @@ -1618,6 +1619,8 @@ static NotifierList suspend_notifiers =
> >>>>        NOTIFIER_LIST_INITIALIZER(suspend_notifiers);
> >>>>    static NotifierList wakeup_notifiers =
> >>>>        NOTIFIER_LIST_INITIALIZER(wakeup_notifiers);
> >>>> +static NotifierList sleep_notifiers =
> >>>> +    NOTIFIER_LIST_INITIALIZER(sleep_notifiers);
> >>>>    static uint32_t wakeup_reason_mask = ~(1 << QEMU_WAKEUP_REASON_NONE);
> >>>>    
> >>>>    ShutdownCause qemu_shutdown_requested_get(void)
> >>>> @@ -1838,6 +1841,24 @@ static void qemu_system_powerdown(void)
> >>>>        notifier_list_notify(&powerdown_notifiers, NULL);
> >>>>    }
> >>>>    
> >>>> +static void qemu_system_sleep(void)
> >>>> +{
> >>>> +    notifier_list_notify(&sleep_notifiers, NULL);
> >>>> +}
> >>>> +
> >>>> +static int qemu_sleep_requested(void)
> >>>> +{
> >>>> +    int r = sleep_requested;
> >>>> +    sleep_requested = 0;
> >>>> +    return r;
> >>>> +}
> >>>> +
> >>>> +void qemu_system_sleep_request(void)
> >>>> +{
> >>>> +    sleep_requested = 1;
> >>>> +    qemu_notify_event();
> >>>> +}
> >>>> +
> >>>>    void qemu_system_powerdown_request(void)
> >>>>    {
> >>>>        trace_qemu_system_powerdown_request();
> >>>> @@ -1850,6 +1871,11 @@ void qemu_register_powerdown_notifier(Notifier *notifier)
> >>>>        notifier_list_add(&powerdown_notifiers, notifier);
> >>>>    }
> >>>>    
> >>>> +void qemu_register_sleep_notifier(Notifier *notifier)
> >>>> +{
> >>>> +    notifier_list_add(&sleep_notifiers, notifier);
> >>>> +}
> >>>> +
> >>>>    void qemu_system_debug_request(void)
> >>>>    {
> >>>>        debug_requested = 1;
> >>>> @@ -1899,6 +1925,9 @@ static bool main_loop_should_exit(void)
> >>>>        if (qemu_powerdown_requested()) {
> >>>>            qemu_system_powerdown();
> >>>>        }
> >>>> +    if (qemu_sleep_requested()) {
> >>>> +        qemu_system_sleep();
> >>>> +    }
> >>>>        if (qemu_vmstop_requested(&r)) {
> >>>>            vm_stop(r);
> >>>>        }  
> >  
> 



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [PATCH] acpi: Add emulated sleep button
  2023-07-14 14:04         ` Igor Mammedov
@ 2023-08-01 17:00           ` Annie.li
  2023-08-30  9:22             ` Igor Mammedov
  0 siblings, 1 reply; 10+ messages in thread
From: Annie.li @ 2023-08-01 17:00 UTC (permalink / raw)
  To: Igor Mammedov
  Cc: Stefan Fritsch, qemu-devel, Eduardo Habkost, Michael S. Tsirkin,
	Dr. David Alan Gilbert, Markus Armbruster, Paolo Bonzini,
	Richard Henderson

Hi Igor,

On 7/14/2023 10:04 AM, Igor Mammedov wrote:
> On Fri, 7 Jul 2023 13:43:36 -0400
> "Annie.li" <annie.li@oracle.com> wrote:
>
>> Hi Igor,
>>
>> Revisiting this thread and have more questions, please clarify, thank you!
>>
>> On 9/20/2021 3:53 AM, Igor Mammedov wrote:
>>> On Fri, 6 Aug 2021 16:18:09 -0400
>>> "Annie.li" <annie.li@oracle.com> wrote:
>>>   
>>>> Hello Igor,
>>>>
>>>> This is an old patch, but it does what we need.
>>>> I am getting a little bit lost about not implementing fixed hardware
>>>> sleep button, can you please clarify? thank you!
>>>>
>>>> On 7/20/2017 10:59 AM, Igor Mammedov wrote:
>>>>> On Thu, 20 Jul 2017 11:31:26 +0200
>>>>> Stefan Fritsch <sf@sfritsch.de> wrote:
>>>>>      
>>>>>> From: Stefan Fritsch <stefan_fritsch@genua.de>
>>>>>>
>>>>>> Add an ACPI sleep button and QMP/HMP commands to trigger it.  A sleep
>>>>>> button is a so called "fixed hardware feature", which makes it more
>>>>>> suitable for putting the system to sleep than a laptop lid, for example.
>>>>>>
>>>>>> The sleep button is disabled by default (Bit 5 in the FACP flags
>>>>>> register set and no button "device" present in SSDT/DSDT). Clearing said
>>>>>> bit enables it as a fixed feature device.
>>>>> per spec sleep button is used for both putting system into
>>>>> sleep and for waking it up.
>>>>>
>>>>> Reusing system_wakeup 'button' to behave as per spec would
>>>>> make this patch significantly smaller.
>> Current 'system_wakeup' sets the WAK_STS bit and PWRBTN_STS to wake up
>> the system, the system_wakeup 'button' is the power button. So(Correct me
>> if I am wrong) reusing the system_wakeup 'button' means reusing the power
>> button for sleep. See the following code of setting WAK_STS and PWRBTN_STS
>> for 'system_wakeup',
>>       case QEMU_WAKEUP_REASON_OTHER:
>>           /* ACPI_BITMASK_WAKE_STATUS should be set on resume.
>>              Pretend that resume was caused by power button */
>>           ar->pm1.evt.sts |=
>>               (ACPI_BITMASK_WAKE_STATUS | ACPI_BITMASK_POWER_BUTTON_STATUS);
> (that's quite ancient code (0bacd1300d) and I couldn't find a reason why
> power button was chosen.
>   https://urldefense.com/v3/__https://www.mail-archive.com/kvm@vger.kernel.org/msg05742.html__;!!ACWV5N9M2RV99hQ!PQp9_UyWYc4gTuTwNUiyPgE0Xwinlsi8F-J6zWOA8KRLUh0EIv68g01XQQrFzKipeZbe-vhHfpGZBb0$
> that was tested with WinXP so would be wise to check if SLEEP button
> works there. (though I'm not sure if we still care for XP being runnable on QEMU)
> )

don't have WinXP at hand to check for now...
Microsoft has ended the WinXP support since Apr 8, 2014.
If someone is still running WinXP, not sure if they care about
the sleep button.
>
> it doesn't have to be ACPI_BITMASK_POWER_BUTTON_STATUS,
> you can convert it to sleep button by using ACPI_BITMASK_SLEEP_BUTTON_STATUS.
> However you'll have to enable sleep button (which is disabled currently)
> in FADT (ACPI_FADT_F_SLP_BUTTON), for guest to see the button.
Agree
Per ACPI spec, SLPBTN_STS -

"This optional bit is set when the sleep button is pressed. In the system
working state, while SLPBTN_EN and SLPBTN_STS are both set, an
interrupt event is raised. In the sleep or soft-off states a wake event is
generated when the sleeping button is pressed and the SLPBTN_EN bit
is set."

Using ACPI_BITMASK_SLEEP_BUTTON_STATUS means qemu ends up with
supporting the sleep button.  With this, implementing the fixed
hardware sleep button(what this patch does) is one option. The interesting
topic is whether it should be implemented as General Control sleep button
or fixed hardware button.

>
>>           break;
>>
>> Per the ACPI spec, the power button can be used in single button model, i.e.
>> it can be used to either shut down the system or put the system into sleep.
>> However, this depends on the software policy and user's setting of the
>> system.
>> Sleep/shutdown can not be done through the power button in the same
>> scenario.
>> If the system has configured pressing the power button to put the system
>> into sleep,
>> system_sleep will put the system into sleep state through the power
>> button, as well
>> as system_powerdown. Pressing the power button will not shut down the
>> system.
>> In this case, system_powerdown has its own issue, but this is different
>> story and
>> let's just focus on things related to system_sleep here.
> regardless of what button is used OSPM is free to enable or disable it
> using FOO_EN bits.

Nod
The system_powerdown issue I mentioned above is -
If the guest is configured to go into sleep when the power button is
pressed, system_powerdown will trigger the guest to go into sleep state.
However, system_wakeup won't be able to wake up the guest in such case.
Looks the current qemu doesn't handle this scenario properly.
>>>> About reusing "system_wakeup", does it mean the following?
>>>>
>>>> 1. when guest is in sleep state, "system_wakeup" wakes up the guest
>>>> 2. when guest is running, "system_wakeup" puts the guest into sleep
>>> yes,  it could be something like this
>>>
>>>   
>>>> "system_wakeup" sets WAK_STS and then system transitions to the
>>>> working state. Correspondingly, I suppose both SLPBTN_STS and
>>>> SLPBTN_EN need to be set for sleeping, and this is what fixed
>>>> hardware sleep button requires?
>>> yep
>>>      
>>>> I have combined the sleep and wakeup together, share the
>>>> code between. But Xen also registers the wakeup notifier, and
>>>> this makes things more complicated if this notifier is shared
>>>> between sleep and wakeup. Or this is my misunderstanding?
>>>> please correct me if I am wrong.
>>> you'd have to fixup xen notifier to handle that
>>>      
>>>>> If you like idea of separate command/button better, then
>>>>> I'd go generic control sleep button way instead of fixed
>>>>> hardware, it would allow us to reuse most of the code in
>>>>> ARM target, plus I'd avoid notifiers and use acpi device
>>>>> lookup instead (see: qmp_query_acpi_ospm_status as example)
>    
>> Implementing the generic control sleep button for x86 does align
>> to the generic control power button implementation in ARM, but
>> it doesn't align to the current fixed hardware power button for x86.
>> Should sleep button be implemented as generic control sleep button
>> to reuse code in ARM or fixed hardware button to align to the fixed
>> power button for x86?
> sleep control button device was present in ACPI 1.0b so it might be
> supported by x86 as well (one just need to test it with ancient OS
> to find out if it were implemented. (WinXP,Vista,some ancient linux...))

If qemu needs to cover the ancient OS, isn't the fixed sleep hardware
button more appropriate?
How about qemu uses fixed sleep button for x86(just like what this
patch does), and share the sleep button for between wake-up and sleep?
The power button has been implemented as fixed hardware button for x86.

Thanks
Annie

>>>>>      
>>>> Do you mean the "Control Method Sleep Button" that needs to
>>>> notify OSPM by event indication 0x80?
>>> yes, in addition to virt-arm machine it could be reused by
>>> microvm which also uses 'reduced hardware' acpi model
>>> (i.e. it lacks fixed hardware registers like virt-arm does)
>>>
>>> maybe while adding button to pc/q35 you can look into adding
>>> it to microvm and virt-arm at the same time (should be trivial
>>> on top of pc/q35 support).
>>>   
>>>> Thanks
>>>> Annie
>>>>>> Signed-off-by: Stefan Fritsch <stefan_fritsch@genua.de>
>>>>>> ---
>>>>>>     hmp-commands.hx         | 16 ++++++++++++++++
>>>>>>     hmp.c                   |  5 +++++
>>>>>>     hmp.h                   |  1 +
>>>>>>     hw/acpi/core.c          |  8 ++++++++
>>>>>>     hw/acpi/ich9.c          | 10 ++++++++++
>>>>>>     hw/acpi/piix4.c         | 12 ++++++++++++
>>>>>>     hw/i386/acpi-build.c    |  1 -
>>>>>>     include/hw/acpi/acpi.h  |  2 ++
>>>>>>     include/hw/acpi/ich9.h  |  1 +
>>>>>>     include/sysemu/sysemu.h |  2 ++
>>>>>>     qapi-schema.json        | 12 ++++++++++++
>>>>>>     qmp.c                   |  5 +++++
>>>>>>     tests/test-hmp.c        |  1 +
>>>>>>     vl.c                    | 29 +++++++++++++++++++++++++++++
>>>>>>     14 files changed, 104 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>>>>>> index 1941e19932..8ba4380883 100644
>>>>>> --- a/hmp-commands.hx
>>>>>> +++ b/hmp-commands.hx
>>>>>> @@ -638,6 +638,22 @@ Reset the system.
>>>>>>     ETEXI
>>>>>>     
>>>>>>         {
>>>>>> +        .name       = "system_sleep",
>>>>>> +        .args_type  = "",
>>>>>> +        .params     = "",
>>>>>> +        .help       = "send ACPI sleep event",
>>>>>> +        .cmd = hmp_system_sleep,
>>>>>> +    },
>>>>>> +
>>>>>> +STEXI
>>>>>> +@item system_sleep
>>>>>> +@findex system_sleep
>>>>>> +
>>>>>> +Push the virtual sleep button; if supported the system will enter
>>>>>> +an ACPI sleep state.
>>>>>> +ETEXI
>>>>>> +
>>>>>> +    {
>>>>>>             .name       = "system_powerdown",
>>>>>>             .args_type  = "",
>>>>>>             .params     = "",
>>>>>> diff --git a/hmp.c b/hmp.c
>>>>>> index bf1de747d5..b4b584016c 100644
>>>>>> --- a/hmp.c
>>>>>> +++ b/hmp.c
>>>>>> @@ -1047,6 +1047,11 @@ void hmp_system_reset(Monitor *mon, const QDict *qdict)
>>>>>>         qmp_system_reset(NULL);
>>>>>>     }
>>>>>>     
>>>>>> +void hmp_system_sleep(Monitor *mon, const QDict *qdict)
>>>>>> +{
>>>>>> +    qmp_system_sleep(NULL);
>>>>>> +}
>>>>>> +
>>>>>>     void hmp_system_powerdown(Monitor *mon, const QDict *qdict)
>>>>>>     {
>>>>>>         qmp_system_powerdown(NULL);
>>>>>> diff --git a/hmp.h b/hmp.h
>>>>>> index 1ff455295e..15b53de9ed 100644
>>>>>> --- a/hmp.h
>>>>>> +++ b/hmp.h
>>>>>> @@ -45,6 +45,7 @@ void hmp_info_iothreads(Monitor *mon, const QDict *qdict);
>>>>>>     void hmp_quit(Monitor *mon, const QDict *qdict);
>>>>>>     void hmp_stop(Monitor *mon, const QDict *qdict);
>>>>>>     void hmp_system_reset(Monitor *mon, const QDict *qdict);
>>>>>> +void hmp_system_sleep(Monitor *mon, const QDict *qdict);
>>>>>>     void hmp_system_powerdown(Monitor *mon, const QDict *qdict);
>>>>>>     void hmp_cpu(Monitor *mon, const QDict *qdict);
>>>>>>     void hmp_memsave(Monitor *mon, const QDict *qdict);
>>>>>> diff --git a/hw/acpi/core.c b/hw/acpi/core.c
>>>>>> index 95fcac95a2..2ee64b6878 100644
>>>>>> --- a/hw/acpi/core.c
>>>>>> +++ b/hw/acpi/core.c
>>>>>> @@ -422,6 +422,14 @@ void acpi_pm1_evt_power_down(ACPIREGS *ar)
>>>>>>         }
>>>>>>     }
>>>>>>     
>>>>>> +void acpi_pm1_evt_sleep(ACPIREGS *ar)
>>>>>> +{
>>>>>> +    if (ar->pm1.evt.en & ACPI_BITMASK_SLEEP_BUTTON_ENABLE) {
>>>>>> +        ar->pm1.evt.sts |= ACPI_BITMASK_SLEEP_BUTTON_STATUS;
>>>>>> +        ar->tmr.update_sci(ar);
>>>>>> +    }
>>>>>> +}
>>>>>> +
>>>>>>     void acpi_pm1_evt_reset(ACPIREGS *ar)
>>>>>>     {
>>>>>>         ar->pm1.evt.sts = 0;
>>>>>> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
>>>>>> index c5d8646abc..3faeab4d2e 100644
>>>>>> --- a/hw/acpi/ich9.c
>>>>>> +++ b/hw/acpi/ich9.c
>>>>>> @@ -260,6 +260,14 @@ static void pm_reset(void *opaque)
>>>>>>         acpi_update_sci(&pm->acpi_regs, pm->irq);
>>>>>>     }
>>>>>>     
>>>>>> +static void pm_sleep_req(Notifier *n, void *opaque)
>>>>>> +{
>>>>>> +    ICH9LPCPMRegs *pm = container_of(n, ICH9LPCPMRegs, sleep_notifier);
>>>>>> +
>>>>>> +    acpi_pm1_evt_sleep(&pm->acpi_regs);
>>>>>> +}
>>>>>> +
>>>>>> +
>>>>>>     static void pm_powerdown_req(Notifier *n, void *opaque)
>>>>>>     {
>>>>>>         ICH9LPCPMRegs *pm = container_of(n, ICH9LPCPMRegs, powerdown_notifier);
>>>>>> @@ -299,6 +307,8 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
>>>>>>         qemu_register_reset(pm_reset, pm);
>>>>>>         pm->powerdown_notifier.notify = pm_powerdown_req;
>>>>>>         qemu_register_powerdown_notifier(&pm->powerdown_notifier);
>>>>>> +    pm->sleep_notifier.notify = pm_sleep_req;
>>>>>> +    qemu_register_sleep_notifier(&pm->sleep_notifier);
>>>>>>     
>>>>>>         legacy_acpi_cpu_hotplug_init(pci_address_space_io(lpc_pci),
>>>>>>             OBJECT(lpc_pci), &pm->gpe_cpu, ICH9_CPU_HOTPLUG_IO_BASE);
>>>>>> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
>>>>>> index f276967365..15e20976c3 100644
>>>>>> --- a/hw/acpi/piix4.c
>>>>>> +++ b/hw/acpi/piix4.c
>>>>>> @@ -79,6 +79,7 @@ typedef struct PIIX4PMState {
>>>>>>         int smm_enabled;
>>>>>>         Notifier machine_ready;
>>>>>>         Notifier powerdown_notifier;
>>>>>> +    Notifier sleep_notifier;
>>>>>>     
>>>>>>         AcpiPciHpState acpi_pci_hotplug;
>>>>>>         bool use_acpi_pci_hotplug;
>>>>>> @@ -371,6 +372,15 @@ static void piix4_pm_powerdown_req(Notifier *n, void *opaque)
>>>>>>         acpi_pm1_evt_power_down(&s->ar);
>>>>>>     }
>>>>>>     
>>>>>> +static void piix4_pm_sleep_req(Notifier *n, void *opaque)
>>>>>> +{
>>>>>> +    PIIX4PMState *s = container_of(n, PIIX4PMState, sleep_notifier);
>>>>>> +
>>>>>> +    assert(s != NULL);
>>>>>> +    acpi_pm1_evt_sleep(&s->ar);
>>>>>> +}
>>>>>> +
>>>>>> +
>>>>>>     static void piix4_device_plug_cb(HotplugHandler *hotplug_dev,
>>>>>>                                      DeviceState *dev, Error **errp)
>>>>>>     {
>>>>>> @@ -535,6 +545,8 @@ static void piix4_pm_realize(PCIDevice *dev, Error **errp)
>>>>>>     
>>>>>>         s->powerdown_notifier.notify = piix4_pm_powerdown_req;
>>>>>>         qemu_register_powerdown_notifier(&s->powerdown_notifier);
>>>>>> +    s->sleep_notifier.notify = piix4_pm_sleep_req;
>>>>>> +    qemu_register_sleep_notifier(&s->sleep_notifier);
>>>>>>     
>>>>>>         s->machine_ready.notify = piix4_pm_machine_ready;
>>>>>>         qemu_add_machine_init_done_notifier(&s->machine_ready);
>>>>>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>>>>>> index 6b7bade183..06b28dacfe 100644
>>>>>> --- a/hw/i386/acpi-build.c
>>>>>> +++ b/hw/i386/acpi-build.c
>>>>>> @@ -294,7 +294,6 @@ static void fadt_setup(AcpiFadtDescriptorRev3 *fadt, AcpiPmInfo *pm)
>>>>>>         fadt->plvl3_lat = cpu_to_le16(0xfff); /* C3 state not supported */
>>>>>>         fadt->flags = cpu_to_le32((1 << ACPI_FADT_F_WBINVD) |
>>>>>>                                   (1 << ACPI_FADT_F_PROC_C1) |
>>>>>> -                              (1 << ACPI_FADT_F_SLP_BUTTON) |
>>>>>>                                   (1 << ACPI_FADT_F_RTC_S4));
>>>>>>         fadt->flags |= cpu_to_le32(1 << ACPI_FADT_F_USE_PLATFORM_CLOCK);
>>>>>>         /* APIC destination mode ("Flat Logical") has an upper limit of 8 CPUs
>>>>>> diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h
>>>>>> index 7b3d93cf0d..51cf901ef6 100644
>>>>>> --- a/include/hw/acpi/acpi.h
>>>>>> +++ b/include/hw/acpi/acpi.h
>>>>>> @@ -78,6 +78,7 @@
>>>>>>     #define ACPI_BITMASK_PM1_COMMON_ENABLED         ( \
>>>>>>             ACPI_BITMASK_RT_CLOCK_ENABLE        | \
>>>>>>             ACPI_BITMASK_POWER_BUTTON_ENABLE    | \
>>>>>> +        ACPI_BITMASK_SLEEP_BUTTON_ENABLE    | \
>>>>>>             ACPI_BITMASK_GLOBAL_LOCK_ENABLE     | \
>>>>>>             ACPI_BITMASK_TIMER_ENABLE)
>>>>>>     
>>>>>> @@ -148,6 +149,7 @@ void acpi_pm_tmr_reset(ACPIREGS *ar);
>>>>>>     /* PM1a_EVT: piix and ich9 don't implement PM1b. */
>>>>>>     uint16_t acpi_pm1_evt_get_sts(ACPIREGS *ar);
>>>>>>     void acpi_pm1_evt_power_down(ACPIREGS *ar);
>>>>>> +void acpi_pm1_evt_sleep(ACPIREGS *ar);
>>>>>>     void acpi_pm1_evt_reset(ACPIREGS *ar);
>>>>>>     void acpi_pm1_evt_init(ACPIREGS *ar, acpi_update_sci_fn update_sci,
>>>>>>                            MemoryRegion *parent);
>>>>>> diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
>>>>>> index a352c94fde..2073eec168 100644
>>>>>> --- a/include/hw/acpi/ich9.h
>>>>>> +++ b/include/hw/acpi/ich9.h
>>>>>> @@ -48,6 +48,7 @@ typedef struct ICH9LPCPMRegs {
>>>>>>     
>>>>>>         uint32_t pm_io_base;
>>>>>>         Notifier powerdown_notifier;
>>>>>> +    Notifier sleep_notifier;
>>>>>>     
>>>>>>         bool cpu_hotplug_legacy;
>>>>>>         AcpiCpuHotplug gpe_cpu;
>>>>>> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
>>>>>> index b21369672a..00f54653dc 100644
>>>>>> --- a/include/sysemu/sysemu.h
>>>>>> +++ b/include/sysemu/sysemu.h
>>>>>> @@ -75,6 +75,8 @@ void qemu_register_wakeup_notifier(Notifier *notifier);
>>>>>>     void qemu_system_shutdown_request(ShutdownCause reason);
>>>>>>     void qemu_system_powerdown_request(void);
>>>>>>     void qemu_register_powerdown_notifier(Notifier *notifier);
>>>>>> +void qemu_system_sleep_request(void);
>>>>>> +void qemu_register_sleep_notifier(Notifier *notifier);
>>>>>>     void qemu_system_debug_request(void);
>>>>>>     void qemu_system_vmstop_request(RunState reason);
>>>>>>     void qemu_system_vmstop_request_prepare(void);
>>>>>> diff --git a/qapi-schema.json b/qapi-schema.json
>>>>>> index 8b015bee2e..c6ccfcd70f 100644
>>>>>> --- a/qapi-schema.json
>>>>>> +++ b/qapi-schema.json
>>>>>> @@ -2314,6 +2314,18 @@
>>>>>>     { 'command': 'system_reset' }
>>>>>>     
>>>>>>     ##
>>>>>> +# @system_sleep:
>>>>>> +#
>>>>>> +# Requests that a guest perform a ACPI sleep transition by pushing a virtual
>>>>>> +# sleep button.
>>>>>> +#
>>>>>> +# Notes: A guest may or may not respond to this command.  This command
>>>>>> +#        returning does not indicate that a guest has accepted the request or
>>>>>> +#        that it has gone to sleep.
>>>>>> +##
>>>>>> +{ 'command': 'system_sleep' }
>>>>>> +
>>>>>> +##
>>>>>>     # @system_powerdown:
>>>>>>     #
>>>>>>     # Requests that a guest perform a powerdown operation.
>>>>>> diff --git a/qmp.c b/qmp.c
>>>>>> index b86201e349..bc1f2e3d7f 100644
>>>>>> --- a/qmp.c
>>>>>> +++ b/qmp.c
>>>>>> @@ -108,6 +108,11 @@ void qmp_system_reset(Error **errp)
>>>>>>         qemu_system_reset_request(SHUTDOWN_CAUSE_HOST_QMP);
>>>>>>     }
>>>>>>     
>>>>>> +void qmp_system_sleep(Error **erp)
>>>>>> +{
>>>>>> +    qemu_system_sleep_request();
>>>>>> +}
>>>>>> +
>>>>>>     void qmp_system_powerdown(Error **erp)
>>>>>>     {
>>>>>>         qemu_system_powerdown_request();
>>>>>> diff --git a/tests/test-hmp.c b/tests/test-hmp.c
>>>>>> index d77b3c8710..3fa850bf1e 100644
>>>>>> --- a/tests/test-hmp.c
>>>>>> +++ b/tests/test-hmp.c
>>>>>> @@ -67,6 +67,7 @@ static const char *hmp_cmds[] = {
>>>>>>         "sum 0 512",
>>>>>>         "x /8i 0x100",
>>>>>>         "xp /16x 0",
>>>>>> +    "system_sleep",
>>>>>>         NULL
>>>>>>     };
>>>>>>     
>>>>>> diff --git a/vl.c b/vl.c
>>>>>> index fb6b2efafa..6a0f847dcf 100644
>>>>>> --- a/vl.c
>>>>>> +++ b/vl.c
>>>>>> @@ -1608,6 +1608,7 @@ static ShutdownCause reset_requested;
>>>>>>     static ShutdownCause shutdown_requested;
>>>>>>     static int shutdown_signal;
>>>>>>     static pid_t shutdown_pid;
>>>>>> +static int sleep_requested;
>>>>>>     static int powerdown_requested;
>>>>>>     static int debug_requested;
>>>>>>     static int suspend_requested;
>>>>>> @@ -1618,6 +1619,8 @@ static NotifierList suspend_notifiers =
>>>>>>         NOTIFIER_LIST_INITIALIZER(suspend_notifiers);
>>>>>>     static NotifierList wakeup_notifiers =
>>>>>>         NOTIFIER_LIST_INITIALIZER(wakeup_notifiers);
>>>>>> +static NotifierList sleep_notifiers =
>>>>>> +    NOTIFIER_LIST_INITIALIZER(sleep_notifiers);
>>>>>>     static uint32_t wakeup_reason_mask = ~(1 << QEMU_WAKEUP_REASON_NONE);
>>>>>>     
>>>>>>     ShutdownCause qemu_shutdown_requested_get(void)
>>>>>> @@ -1838,6 +1841,24 @@ static void qemu_system_powerdown(void)
>>>>>>         notifier_list_notify(&powerdown_notifiers, NULL);
>>>>>>     }
>>>>>>     
>>>>>> +static void qemu_system_sleep(void)
>>>>>> +{
>>>>>> +    notifier_list_notify(&sleep_notifiers, NULL);
>>>>>> +}
>>>>>> +
>>>>>> +static int qemu_sleep_requested(void)
>>>>>> +{
>>>>>> +    int r = sleep_requested;
>>>>>> +    sleep_requested = 0;
>>>>>> +    return r;
>>>>>> +}
>>>>>> +
>>>>>> +void qemu_system_sleep_request(void)
>>>>>> +{
>>>>>> +    sleep_requested = 1;
>>>>>> +    qemu_notify_event();
>>>>>> +}
>>>>>> +
>>>>>>     void qemu_system_powerdown_request(void)
>>>>>>     {
>>>>>>         trace_qemu_system_powerdown_request();
>>>>>> @@ -1850,6 +1871,11 @@ void qemu_register_powerdown_notifier(Notifier *notifier)
>>>>>>         notifier_list_add(&powerdown_notifiers, notifier);
>>>>>>     }
>>>>>>     
>>>>>> +void qemu_register_sleep_notifier(Notifier *notifier)
>>>>>> +{
>>>>>> +    notifier_list_add(&sleep_notifiers, notifier);
>>>>>> +}
>>>>>> +
>>>>>>     void qemu_system_debug_request(void)
>>>>>>     {
>>>>>>         debug_requested = 1;
>>>>>> @@ -1899,6 +1925,9 @@ static bool main_loop_should_exit(void)
>>>>>>         if (qemu_powerdown_requested()) {
>>>>>>             qemu_system_powerdown();
>>>>>>         }
>>>>>> +    if (qemu_sleep_requested()) {
>>>>>> +        qemu_system_sleep();
>>>>>> +    }
>>>>>>         if (qemu_vmstop_requested(&r)) {
>>>>>>             vm_stop(r);
>>>>>>         }
>>>   


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [PATCH] acpi: Add emulated sleep button
  2023-08-01 17:00           ` Annie.li
@ 2023-08-30  9:22             ` Igor Mammedov
  0 siblings, 0 replies; 10+ messages in thread
From: Igor Mammedov @ 2023-08-30  9:22 UTC (permalink / raw)
  To: Annie.li
  Cc: Stefan Fritsch, qemu-devel, Eduardo Habkost, Michael S. Tsirkin,
	Dr. David Alan Gilbert, Markus Armbruster, Paolo Bonzini,
	Richard Henderson

On Tue, 1 Aug 2023 13:00:02 -0400
"Annie.li" <annie.li@oracle.com> wrote:

> Hi Igor,
> 
> On 7/14/2023 10:04 AM, Igor Mammedov wrote:
> > On Fri, 7 Jul 2023 13:43:36 -0400
> > "Annie.li" <annie.li@oracle.com> wrote:
> >  
> >> Hi Igor,
> >>
> >> Revisiting this thread and have more questions, please clarify, thank you!
> >>
> >> On 9/20/2021 3:53 AM, Igor Mammedov wrote:  
> >>> On Fri, 6 Aug 2021 16:18:09 -0400
> >>> "Annie.li" <annie.li@oracle.com> wrote:
> >>>     
> >>>> Hello Igor,
> >>>>
> >>>> This is an old patch, but it does what we need.
> >>>> I am getting a little bit lost about not implementing fixed hardware
> >>>> sleep button, can you please clarify? thank you!
> >>>>
> >>>> On 7/20/2017 10:59 AM, Igor Mammedov wrote:  
> >>>>> On Thu, 20 Jul 2017 11:31:26 +0200
> >>>>> Stefan Fritsch <sf@sfritsch.de> wrote:
> >>>>>        
> >>>>>> From: Stefan Fritsch <stefan_fritsch@genua.de>
> >>>>>>
> >>>>>> Add an ACPI sleep button and QMP/HMP commands to trigger it.  A sleep
> >>>>>> button is a so called "fixed hardware feature", which makes it more
> >>>>>> suitable for putting the system to sleep than a laptop lid, for example.
> >>>>>>
> >>>>>> The sleep button is disabled by default (Bit 5 in the FACP flags
> >>>>>> register set and no button "device" present in SSDT/DSDT). Clearing said
> >>>>>> bit enables it as a fixed feature device.  
> >>>>> per spec sleep button is used for both putting system into
> >>>>> sleep and for waking it up.
> >>>>>
> >>>>> Reusing system_wakeup 'button' to behave as per spec would
> >>>>> make this patch significantly smaller.  
> >> Current 'system_wakeup' sets the WAK_STS bit and PWRBTN_STS to wake up
> >> the system, the system_wakeup 'button' is the power button. So(Correct me
> >> if I am wrong) reusing the system_wakeup 'button' means reusing the power
> >> button for sleep. See the following code of setting WAK_STS and PWRBTN_STS
> >> for 'system_wakeup',
> >>       case QEMU_WAKEUP_REASON_OTHER:
> >>           /* ACPI_BITMASK_WAKE_STATUS should be set on resume.
> >>              Pretend that resume was caused by power button */
> >>           ar->pm1.evt.sts |=
> >>               (ACPI_BITMASK_WAKE_STATUS | ACPI_BITMASK_POWER_BUTTON_STATUS);  
> > (that's quite ancient code (0bacd1300d) and I couldn't find a reason why
> > power button was chosen.
> >   https://urldefense.com/v3/__https://www.mail-archive.com/kvm@vger.kernel.org/msg05742.html__;!!ACWV5N9M2RV99hQ!PQp9_UyWYc4gTuTwNUiyPgE0Xwinlsi8F-J6zWOA8KRLUh0EIv68g01XQQrFzKipeZbe-vhHfpGZBb0$
> > that was tested with WinXP so would be wise to check if SLEEP button
> > works there. (though I'm not sure if we still care for XP being runnable on QEMU)
> > )  
> 
> don't have WinXP at hand to check for now...
> Microsoft has ended the WinXP support since Apr 8, 2014.
> If someone is still running WinXP, not sure if they care about
> the sleep button.

the only thing I care here is not regressing current state.
I can test WnXP and RHEL6 for you 

> >
> > it doesn't have to be ACPI_BITMASK_POWER_BUTTON_STATUS,
> > you can convert it to sleep button by using ACPI_BITMASK_SLEEP_BUTTON_STATUS.
> > However you'll have to enable sleep button (which is disabled currently)
> > in FADT (ACPI_FADT_F_SLP_BUTTON), for guest to see the button.  
> Agree
> Per ACPI spec, SLPBTN_STS -
> 
> "This optional bit is set when the sleep button is pressed. In the system
> working state, while SLPBTN_EN and SLPBTN_STS are both set, an
> interrupt event is raised. In the sleep or soft-off states a wake event is
> generated when the sleeping button is pressed and the SLPBTN_EN bit
> is set."
> 
> Using ACPI_BITMASK_SLEEP_BUTTON_STATUS means qemu ends up with
> supporting the sleep button.  With this, implementing the fixed
> hardware sleep button(what this patch does) is one option. The interesting
> topic is whether it should be implemented as General Control sleep button
> or fixed hardware button.
> 
> >  
> >>           break;
> >>
> >> Per the ACPI spec, the power button can be used in single button model, i.e.
> >> it can be used to either shut down the system or put the system into sleep.
> >> However, this depends on the software policy and user's setting of the
> >> system.
> >> Sleep/shutdown can not be done through the power button in the same
> >> scenario.
> >> If the system has configured pressing the power button to put the system
> >> into sleep,
> >> system_sleep will put the system into sleep state through the power
> >> button, as well
> >> as system_powerdown. Pressing the power button will not shut down the
> >> system.
> >> In this case, system_powerdown has its own issue, but this is different
> >> story and
> >> let's just focus on things related to system_sleep here.  
> > regardless of what button is used OSPM is free to enable or disable it
> > using FOO_EN bits.  
> 
> Nod
> The system_powerdown issue I mentioned above is -
> If the guest is configured to go into sleep when the power button is
> pressed, system_powerdown will trigger the guest to go into sleep state.
> However, system_wakeup won't be able to wake up the guest in such case.
> Looks the current qemu doesn't handle this scenario properly.
> >>>> About reusing "system_wakeup", does it mean the following?
> >>>>
> >>>> 1. when guest is in sleep state, "system_wakeup" wakes up the guest
> >>>> 2. when guest is running, "system_wakeup" puts the guest into sleep  
> >>> yes,  it could be something like this
> >>>
> >>>     
> >>>> "system_wakeup" sets WAK_STS and then system transitions to the
> >>>> working state. Correspondingly, I suppose both SLPBTN_STS and
> >>>> SLPBTN_EN need to be set for sleeping, and this is what fixed
> >>>> hardware sleep button requires?  
> >>> yep
> >>>        
> >>>> I have combined the sleep and wakeup together, share the
> >>>> code between. But Xen also registers the wakeup notifier, and
> >>>> this makes things more complicated if this notifier is shared
> >>>> between sleep and wakeup. Or this is my misunderstanding?
> >>>> please correct me if I am wrong.  
> >>> you'd have to fixup xen notifier to handle that
> >>>        
> >>>>> If you like idea of separate command/button better, then
> >>>>> I'd go generic control sleep button way instead of fixed
> >>>>> hardware, it would allow us to reuse most of the code in
> >>>>> ARM target, plus I'd avoid notifiers and use acpi device
> >>>>> lookup instead (see: qmp_query_acpi_ospm_status as example)  
> >      
> >> Implementing the generic control sleep button for x86 does align
> >> to the generic control power button implementation in ARM, but
> >> it doesn't align to the current fixed hardware power button for x86.
> >> Should sleep button be implemented as generic control sleep button
> >> to reuse code in ARM or fixed hardware button to align to the fixed
> >> power button for x86?  
> > sleep control button device was present in ACPI 1.0b so it might be
> > supported by x86 as well (one just need to test it with ancient OS
> > to find out if it were implemented. (WinXP,Vista,some ancient linux...))  
> 
> If qemu needs to cover the ancient OS, isn't the fixed sleep hardware
> button more appropriate?

As long as using control method approach doesn't break anything it
should be fine.

> How about qemu uses fixed sleep button for x86(just like what this
> patch does), and share the sleep button for between wake-up and sleep?

Fixed HW button is not supported on HW reduced platforms (arm/virt, microvm), so
I'd try control method way 1st (so it could be reused there as well)
and only if that doesn't work out would fallback to fixed hw approach.

PS:
only instead of introducing new system_sleep, I'd suggest to reuse
existing system_wakeup.


> The power button has been implemented as fixed hardware button for x86.

> Thanks
> Annie
> 
> >>>>>        
> >>>> Do you mean the "Control Method Sleep Button" that needs to
> >>>> notify OSPM by event indication 0x80?  
> >>> yes, in addition to virt-arm machine it could be reused by
> >>> microvm which also uses 'reduced hardware' acpi model
> >>> (i.e. it lacks fixed hardware registers like virt-arm does)
> >>>
> >>> maybe while adding button to pc/q35 you can look into adding
> >>> it to microvm and virt-arm at the same time (should be trivial
> >>> on top of pc/q35 support).
> >>>     
> >>>> Thanks
> >>>> Annie  
> >>>>>> Signed-off-by: Stefan Fritsch <stefan_fritsch@genua.de>
> >>>>>> ---
> >>>>>>     hmp-commands.hx         | 16 ++++++++++++++++
> >>>>>>     hmp.c                   |  5 +++++
> >>>>>>     hmp.h                   |  1 +
> >>>>>>     hw/acpi/core.c          |  8 ++++++++
> >>>>>>     hw/acpi/ich9.c          | 10 ++++++++++
> >>>>>>     hw/acpi/piix4.c         | 12 ++++++++++++
> >>>>>>     hw/i386/acpi-build.c    |  1 -
> >>>>>>     include/hw/acpi/acpi.h  |  2 ++
> >>>>>>     include/hw/acpi/ich9.h  |  1 +
> >>>>>>     include/sysemu/sysemu.h |  2 ++
> >>>>>>     qapi-schema.json        | 12 ++++++++++++
> >>>>>>     qmp.c                   |  5 +++++
> >>>>>>     tests/test-hmp.c        |  1 +
> >>>>>>     vl.c                    | 29 +++++++++++++++++++++++++++++
> >>>>>>     14 files changed, 104 insertions(+), 1 deletion(-)
> >>>>>>
> >>>>>> diff --git a/hmp-commands.hx b/hmp-commands.hx
> >>>>>> index 1941e19932..8ba4380883 100644
> >>>>>> --- a/hmp-commands.hx
> >>>>>> +++ b/hmp-commands.hx
> >>>>>> @@ -638,6 +638,22 @@ Reset the system.
> >>>>>>     ETEXI
> >>>>>>     
> >>>>>>         {
> >>>>>> +        .name       = "system_sleep",
> >>>>>> +        .args_type  = "",
> >>>>>> +        .params     = "",
> >>>>>> +        .help       = "send ACPI sleep event",
> >>>>>> +        .cmd = hmp_system_sleep,
> >>>>>> +    },
> >>>>>> +
> >>>>>> +STEXI
> >>>>>> +@item system_sleep
> >>>>>> +@findex system_sleep
> >>>>>> +
> >>>>>> +Push the virtual sleep button; if supported the system will enter
> >>>>>> +an ACPI sleep state.
> >>>>>> +ETEXI
> >>>>>> +
> >>>>>> +    {
> >>>>>>             .name       = "system_powerdown",
> >>>>>>             .args_type  = "",
> >>>>>>             .params     = "",
> >>>>>> diff --git a/hmp.c b/hmp.c
> >>>>>> index bf1de747d5..b4b584016c 100644
> >>>>>> --- a/hmp.c
> >>>>>> +++ b/hmp.c
> >>>>>> @@ -1047,6 +1047,11 @@ void hmp_system_reset(Monitor *mon, const QDict *qdict)
> >>>>>>         qmp_system_reset(NULL);
> >>>>>>     }
> >>>>>>     
> >>>>>> +void hmp_system_sleep(Monitor *mon, const QDict *qdict)
> >>>>>> +{
> >>>>>> +    qmp_system_sleep(NULL);
> >>>>>> +}
> >>>>>> +
> >>>>>>     void hmp_system_powerdown(Monitor *mon, const QDict *qdict)
> >>>>>>     {
> >>>>>>         qmp_system_powerdown(NULL);
> >>>>>> diff --git a/hmp.h b/hmp.h
> >>>>>> index 1ff455295e..15b53de9ed 100644
> >>>>>> --- a/hmp.h
> >>>>>> +++ b/hmp.h
> >>>>>> @@ -45,6 +45,7 @@ void hmp_info_iothreads(Monitor *mon, const QDict *qdict);
> >>>>>>     void hmp_quit(Monitor *mon, const QDict *qdict);
> >>>>>>     void hmp_stop(Monitor *mon, const QDict *qdict);
> >>>>>>     void hmp_system_reset(Monitor *mon, const QDict *qdict);
> >>>>>> +void hmp_system_sleep(Monitor *mon, const QDict *qdict);
> >>>>>>     void hmp_system_powerdown(Monitor *mon, const QDict *qdict);
> >>>>>>     void hmp_cpu(Monitor *mon, const QDict *qdict);
> >>>>>>     void hmp_memsave(Monitor *mon, const QDict *qdict);
> >>>>>> diff --git a/hw/acpi/core.c b/hw/acpi/core.c
> >>>>>> index 95fcac95a2..2ee64b6878 100644
> >>>>>> --- a/hw/acpi/core.c
> >>>>>> +++ b/hw/acpi/core.c
> >>>>>> @@ -422,6 +422,14 @@ void acpi_pm1_evt_power_down(ACPIREGS *ar)
> >>>>>>         }
> >>>>>>     }
> >>>>>>     
> >>>>>> +void acpi_pm1_evt_sleep(ACPIREGS *ar)
> >>>>>> +{
> >>>>>> +    if (ar->pm1.evt.en & ACPI_BITMASK_SLEEP_BUTTON_ENABLE) {
> >>>>>> +        ar->pm1.evt.sts |= ACPI_BITMASK_SLEEP_BUTTON_STATUS;
> >>>>>> +        ar->tmr.update_sci(ar);
> >>>>>> +    }
> >>>>>> +}
> >>>>>> +
> >>>>>>     void acpi_pm1_evt_reset(ACPIREGS *ar)
> >>>>>>     {
> >>>>>>         ar->pm1.evt.sts = 0;
> >>>>>> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> >>>>>> index c5d8646abc..3faeab4d2e 100644
> >>>>>> --- a/hw/acpi/ich9.c
> >>>>>> +++ b/hw/acpi/ich9.c
> >>>>>> @@ -260,6 +260,14 @@ static void pm_reset(void *opaque)
> >>>>>>         acpi_update_sci(&pm->acpi_regs, pm->irq);
> >>>>>>     }
> >>>>>>     
> >>>>>> +static void pm_sleep_req(Notifier *n, void *opaque)
> >>>>>> +{
> >>>>>> +    ICH9LPCPMRegs *pm = container_of(n, ICH9LPCPMRegs, sleep_notifier);
> >>>>>> +
> >>>>>> +    acpi_pm1_evt_sleep(&pm->acpi_regs);
> >>>>>> +}
> >>>>>> +
> >>>>>> +
> >>>>>>     static void pm_powerdown_req(Notifier *n, void *opaque)
> >>>>>>     {
> >>>>>>         ICH9LPCPMRegs *pm = container_of(n, ICH9LPCPMRegs, powerdown_notifier);
> >>>>>> @@ -299,6 +307,8 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
> >>>>>>         qemu_register_reset(pm_reset, pm);
> >>>>>>         pm->powerdown_notifier.notify = pm_powerdown_req;
> >>>>>>         qemu_register_powerdown_notifier(&pm->powerdown_notifier);
> >>>>>> +    pm->sleep_notifier.notify = pm_sleep_req;
> >>>>>> +    qemu_register_sleep_notifier(&pm->sleep_notifier);
> >>>>>>     
> >>>>>>         legacy_acpi_cpu_hotplug_init(pci_address_space_io(lpc_pci),
> >>>>>>             OBJECT(lpc_pci), &pm->gpe_cpu, ICH9_CPU_HOTPLUG_IO_BASE);
> >>>>>> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> >>>>>> index f276967365..15e20976c3 100644
> >>>>>> --- a/hw/acpi/piix4.c
> >>>>>> +++ b/hw/acpi/piix4.c
> >>>>>> @@ -79,6 +79,7 @@ typedef struct PIIX4PMState {
> >>>>>>         int smm_enabled;
> >>>>>>         Notifier machine_ready;
> >>>>>>         Notifier powerdown_notifier;
> >>>>>> +    Notifier sleep_notifier;
> >>>>>>     
> >>>>>>         AcpiPciHpState acpi_pci_hotplug;
> >>>>>>         bool use_acpi_pci_hotplug;
> >>>>>> @@ -371,6 +372,15 @@ static void piix4_pm_powerdown_req(Notifier *n, void *opaque)
> >>>>>>         acpi_pm1_evt_power_down(&s->ar);
> >>>>>>     }
> >>>>>>     
> >>>>>> +static void piix4_pm_sleep_req(Notifier *n, void *opaque)
> >>>>>> +{
> >>>>>> +    PIIX4PMState *s = container_of(n, PIIX4PMState, sleep_notifier);
> >>>>>> +
> >>>>>> +    assert(s != NULL);
> >>>>>> +    acpi_pm1_evt_sleep(&s->ar);
> >>>>>> +}
> >>>>>> +
> >>>>>> +
> >>>>>>     static void piix4_device_plug_cb(HotplugHandler *hotplug_dev,
> >>>>>>                                      DeviceState *dev, Error **errp)
> >>>>>>     {
> >>>>>> @@ -535,6 +545,8 @@ static void piix4_pm_realize(PCIDevice *dev, Error **errp)
> >>>>>>     
> >>>>>>         s->powerdown_notifier.notify = piix4_pm_powerdown_req;
> >>>>>>         qemu_register_powerdown_notifier(&s->powerdown_notifier);
> >>>>>> +    s->sleep_notifier.notify = piix4_pm_sleep_req;
> >>>>>> +    qemu_register_sleep_notifier(&s->sleep_notifier);
> >>>>>>     
> >>>>>>         s->machine_ready.notify = piix4_pm_machine_ready;
> >>>>>>         qemu_add_machine_init_done_notifier(&s->machine_ready);
> >>>>>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> >>>>>> index 6b7bade183..06b28dacfe 100644
> >>>>>> --- a/hw/i386/acpi-build.c
> >>>>>> +++ b/hw/i386/acpi-build.c
> >>>>>> @@ -294,7 +294,6 @@ static void fadt_setup(AcpiFadtDescriptorRev3 *fadt, AcpiPmInfo *pm)
> >>>>>>         fadt->plvl3_lat = cpu_to_le16(0xfff); /* C3 state not supported */
> >>>>>>         fadt->flags = cpu_to_le32((1 << ACPI_FADT_F_WBINVD) |
> >>>>>>                                   (1 << ACPI_FADT_F_PROC_C1) |
> >>>>>> -                              (1 << ACPI_FADT_F_SLP_BUTTON) |
> >>>>>>                                   (1 << ACPI_FADT_F_RTC_S4));
> >>>>>>         fadt->flags |= cpu_to_le32(1 << ACPI_FADT_F_USE_PLATFORM_CLOCK);
> >>>>>>         /* APIC destination mode ("Flat Logical") has an upper limit of 8 CPUs
> >>>>>> diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h
> >>>>>> index 7b3d93cf0d..51cf901ef6 100644
> >>>>>> --- a/include/hw/acpi/acpi.h
> >>>>>> +++ b/include/hw/acpi/acpi.h
> >>>>>> @@ -78,6 +78,7 @@
> >>>>>>     #define ACPI_BITMASK_PM1_COMMON_ENABLED         ( \
> >>>>>>             ACPI_BITMASK_RT_CLOCK_ENABLE        | \
> >>>>>>             ACPI_BITMASK_POWER_BUTTON_ENABLE    | \
> >>>>>> +        ACPI_BITMASK_SLEEP_BUTTON_ENABLE    | \
> >>>>>>             ACPI_BITMASK_GLOBAL_LOCK_ENABLE     | \
> >>>>>>             ACPI_BITMASK_TIMER_ENABLE)
> >>>>>>     
> >>>>>> @@ -148,6 +149,7 @@ void acpi_pm_tmr_reset(ACPIREGS *ar);
> >>>>>>     /* PM1a_EVT: piix and ich9 don't implement PM1b. */
> >>>>>>     uint16_t acpi_pm1_evt_get_sts(ACPIREGS *ar);
> >>>>>>     void acpi_pm1_evt_power_down(ACPIREGS *ar);
> >>>>>> +void acpi_pm1_evt_sleep(ACPIREGS *ar);
> >>>>>>     void acpi_pm1_evt_reset(ACPIREGS *ar);
> >>>>>>     void acpi_pm1_evt_init(ACPIREGS *ar, acpi_update_sci_fn update_sci,
> >>>>>>                            MemoryRegion *parent);
> >>>>>> diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
> >>>>>> index a352c94fde..2073eec168 100644
> >>>>>> --- a/include/hw/acpi/ich9.h
> >>>>>> +++ b/include/hw/acpi/ich9.h
> >>>>>> @@ -48,6 +48,7 @@ typedef struct ICH9LPCPMRegs {
> >>>>>>     
> >>>>>>         uint32_t pm_io_base;
> >>>>>>         Notifier powerdown_notifier;
> >>>>>> +    Notifier sleep_notifier;
> >>>>>>     
> >>>>>>         bool cpu_hotplug_legacy;
> >>>>>>         AcpiCpuHotplug gpe_cpu;
> >>>>>> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> >>>>>> index b21369672a..00f54653dc 100644
> >>>>>> --- a/include/sysemu/sysemu.h
> >>>>>> +++ b/include/sysemu/sysemu.h
> >>>>>> @@ -75,6 +75,8 @@ void qemu_register_wakeup_notifier(Notifier *notifier);
> >>>>>>     void qemu_system_shutdown_request(ShutdownCause reason);
> >>>>>>     void qemu_system_powerdown_request(void);
> >>>>>>     void qemu_register_powerdown_notifier(Notifier *notifier);
> >>>>>> +void qemu_system_sleep_request(void);
> >>>>>> +void qemu_register_sleep_notifier(Notifier *notifier);
> >>>>>>     void qemu_system_debug_request(void);
> >>>>>>     void qemu_system_vmstop_request(RunState reason);
> >>>>>>     void qemu_system_vmstop_request_prepare(void);
> >>>>>> diff --git a/qapi-schema.json b/qapi-schema.json
> >>>>>> index 8b015bee2e..c6ccfcd70f 100644
> >>>>>> --- a/qapi-schema.json
> >>>>>> +++ b/qapi-schema.json
> >>>>>> @@ -2314,6 +2314,18 @@
> >>>>>>     { 'command': 'system_reset' }
> >>>>>>     
> >>>>>>     ##
> >>>>>> +# @system_sleep:
> >>>>>> +#
> >>>>>> +# Requests that a guest perform a ACPI sleep transition by pushing a virtual
> >>>>>> +# sleep button.
> >>>>>> +#
> >>>>>> +# Notes: A guest may or may not respond to this command.  This command
> >>>>>> +#        returning does not indicate that a guest has accepted the request or
> >>>>>> +#        that it has gone to sleep.
> >>>>>> +##
> >>>>>> +{ 'command': 'system_sleep' }
> >>>>>> +
> >>>>>> +##
> >>>>>>     # @system_powerdown:
> >>>>>>     #
> >>>>>>     # Requests that a guest perform a powerdown operation.
> >>>>>> diff --git a/qmp.c b/qmp.c
> >>>>>> index b86201e349..bc1f2e3d7f 100644
> >>>>>> --- a/qmp.c
> >>>>>> +++ b/qmp.c
> >>>>>> @@ -108,6 +108,11 @@ void qmp_system_reset(Error **errp)
> >>>>>>         qemu_system_reset_request(SHUTDOWN_CAUSE_HOST_QMP);
> >>>>>>     }
> >>>>>>     
> >>>>>> +void qmp_system_sleep(Error **erp)
> >>>>>> +{
> >>>>>> +    qemu_system_sleep_request();
> >>>>>> +}
> >>>>>> +
> >>>>>>     void qmp_system_powerdown(Error **erp)
> >>>>>>     {
> >>>>>>         qemu_system_powerdown_request();
> >>>>>> diff --git a/tests/test-hmp.c b/tests/test-hmp.c
> >>>>>> index d77b3c8710..3fa850bf1e 100644
> >>>>>> --- a/tests/test-hmp.c
> >>>>>> +++ b/tests/test-hmp.c
> >>>>>> @@ -67,6 +67,7 @@ static const char *hmp_cmds[] = {
> >>>>>>         "sum 0 512",
> >>>>>>         "x /8i 0x100",
> >>>>>>         "xp /16x 0",
> >>>>>> +    "system_sleep",
> >>>>>>         NULL
> >>>>>>     };
> >>>>>>     
> >>>>>> diff --git a/vl.c b/vl.c
> >>>>>> index fb6b2efafa..6a0f847dcf 100644
> >>>>>> --- a/vl.c
> >>>>>> +++ b/vl.c
> >>>>>> @@ -1608,6 +1608,7 @@ static ShutdownCause reset_requested;
> >>>>>>     static ShutdownCause shutdown_requested;
> >>>>>>     static int shutdown_signal;
> >>>>>>     static pid_t shutdown_pid;
> >>>>>> +static int sleep_requested;
> >>>>>>     static int powerdown_requested;
> >>>>>>     static int debug_requested;
> >>>>>>     static int suspend_requested;
> >>>>>> @@ -1618,6 +1619,8 @@ static NotifierList suspend_notifiers =
> >>>>>>         NOTIFIER_LIST_INITIALIZER(suspend_notifiers);
> >>>>>>     static NotifierList wakeup_notifiers =
> >>>>>>         NOTIFIER_LIST_INITIALIZER(wakeup_notifiers);
> >>>>>> +static NotifierList sleep_notifiers =
> >>>>>> +    NOTIFIER_LIST_INITIALIZER(sleep_notifiers);
> >>>>>>     static uint32_t wakeup_reason_mask = ~(1 << QEMU_WAKEUP_REASON_NONE);
> >>>>>>     
> >>>>>>     ShutdownCause qemu_shutdown_requested_get(void)
> >>>>>> @@ -1838,6 +1841,24 @@ static void qemu_system_powerdown(void)
> >>>>>>         notifier_list_notify(&powerdown_notifiers, NULL);
> >>>>>>     }
> >>>>>>     
> >>>>>> +static void qemu_system_sleep(void)
> >>>>>> +{
> >>>>>> +    notifier_list_notify(&sleep_notifiers, NULL);
> >>>>>> +}
> >>>>>> +
> >>>>>> +static int qemu_sleep_requested(void)
> >>>>>> +{
> >>>>>> +    int r = sleep_requested;
> >>>>>> +    sleep_requested = 0;
> >>>>>> +    return r;
> >>>>>> +}
> >>>>>> +
> >>>>>> +void qemu_system_sleep_request(void)
> >>>>>> +{
> >>>>>> +    sleep_requested = 1;
> >>>>>> +    qemu_notify_event();
> >>>>>> +}
> >>>>>> +
> >>>>>>     void qemu_system_powerdown_request(void)
> >>>>>>     {
> >>>>>>         trace_qemu_system_powerdown_request();
> >>>>>> @@ -1850,6 +1871,11 @@ void qemu_register_powerdown_notifier(Notifier *notifier)
> >>>>>>         notifier_list_add(&powerdown_notifiers, notifier);
> >>>>>>     }
> >>>>>>     
> >>>>>> +void qemu_register_sleep_notifier(Notifier *notifier)
> >>>>>> +{
> >>>>>> +    notifier_list_add(&sleep_notifiers, notifier);
> >>>>>> +}
> >>>>>> +
> >>>>>>     void qemu_system_debug_request(void)
> >>>>>>     {
> >>>>>>         debug_requested = 1;
> >>>>>> @@ -1899,6 +1925,9 @@ static bool main_loop_should_exit(void)
> >>>>>>         if (qemu_powerdown_requested()) {
> >>>>>>             qemu_system_powerdown();
> >>>>>>         }
> >>>>>> +    if (qemu_sleep_requested()) {
> >>>>>> +        qemu_system_sleep();
> >>>>>> +    }
> >>>>>>         if (qemu_vmstop_requested(&r)) {
> >>>>>>             vm_stop(r);
> >>>>>>         }  
> >>>     
> 



^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2023-08-30  9:22 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-20  9:31 [Qemu-devel] [PATCH] acpi: Add emulated sleep button Stefan Fritsch
2017-07-20 12:05 ` Eric Blake
2017-07-20 12:27   ` Stefan Fritsch
2017-07-20 14:59 ` Igor Mammedov
2021-08-06 20:18   ` Annie.li
2021-09-20  7:53     ` Igor Mammedov
2023-07-07 17:43       ` Annie.li
2023-07-14 14:04         ` Igor Mammedov
2023-08-01 17:00           ` Annie.li
2023-08-30  9:22             ` Igor Mammedov

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.