On Fri, 6 Aug 2021 16:18:09 -0400
"Annie.li" <annie...@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 <s...@sfritsch.de> wrote:
> >
> >> From: Stefan Fritsch <stefan_frit...@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_frit...@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);
> >> }
>
