| Message ID | 20231205002356.1239-1-annie.li@oracle.com |
|---|---|
| State | New |
| Headers | show |
| Series | Support ACPI Control Method Sleep button | expand |
Hi Annie, On 5/12/23 01:23, Annie Li wrote: > Following hmp/qmp commands are implemented for pressing virtual > sleep button, > > hmp: system_sleep > qmp: { "execute": "system_sleep" } > > These commands put the guest into suspend or other power states > depending on the power settings inside the guest. > > Signed-off-by: Annie Li <annie.li@oracle.com> > --- > hmp-commands.hx | 14 ++++++++++++++ > hw/core/machine-hmp-cmds.c | 5 +++++ > hw/core/machine-qmp-cmds.c | 9 +++++++++ > include/monitor/hmp.h | 1 + > qapi/machine.json | 18 ++++++++++++++++++ > qapi/pragma.json | 1 + > 6 files changed, 48 insertions(+) > index b6d634b30d..3ac69df92f 100644 > --- a/qapi/machine.json > +++ b/qapi/machine.json > @@ -297,6 +297,24 @@ > ## > { 'command': 'system_reset' } > > +## > +# @system_sleep: @since 9.0 > +# > +# 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. > +# > +# Example: > +# > +# -> { "execute": "system_sleep" } > +# <- { "return": {} } > +# > +## > +{ 'command': 'system_sleep' } Do we want a 'mode' argument, starting here with control_method? > + > ## > # @system_powerdown: > #
Hi Philippe, On 12/5/2023 4:44 AM, Philippe Mathieu-Daudé wrote: > Hi Annie, > > On 5/12/23 01:23, Annie Li wrote: >> Following hmp/qmp commands are implemented for pressing virtual >> sleep button, >> >> hmp: system_sleep >> qmp: { "execute": "system_sleep" } >> >> These commands put the guest into suspend or other power states >> depending on the power settings inside the guest. >> >> Signed-off-by: Annie Li <annie.li@oracle.com> >> --- >> hmp-commands.hx | 14 ++++++++++++++ >> hw/core/machine-hmp-cmds.c | 5 +++++ >> hw/core/machine-qmp-cmds.c | 9 +++++++++ >> include/monitor/hmp.h | 1 + >> qapi/machine.json | 18 ++++++++++++++++++ >> qapi/pragma.json | 1 + >> 6 files changed, 48 insertions(+) > > >> index b6d634b30d..3ac69df92f 100644 >> --- a/qapi/machine.json >> +++ b/qapi/machine.json >> @@ -297,6 +297,24 @@ >> ## >> { 'command': 'system_reset' } >> +## >> +# @system_sleep: > > @since 9.0 Nod > >> +# >> +# 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. >> +# >> +# Example: >> +# >> +# -> { "execute": "system_sleep" } >> +# <- { "return": {} } >> +# >> +## >> +{ 'command': 'system_sleep' } > > Do we want a 'mode' argument, starting here with control_method? As what has been discussed previously in the following thread, https://lore.kernel.org/all/20210920095316.2dd133be@redhat.com/T/#mfe24f89778020deeacfe45083f3eea3cf9f55961 the Control Method Sleep button can be shared among various architectures. It is very likely that there will be one type of sleep button(Control Method) implemented, so the extra argument isn't necessary. Thanks Annie > >> + >> ## >> # @system_powerdown: >> #
You neglected to cc: QAPI schema maintainers. I found it by chance. Next time :) Annie Li <annie.li@oracle.com> writes: > Following hmp/qmp commands are implemented for pressing virtual > sleep button, > > hmp: system_sleep > qmp: { "execute": "system_sleep" } > > These commands put the guest into suspend or other power states > depending on the power settings inside the guest. How is this related to system_wakeup? > Signed-off-by: Annie Li <annie.li@oracle.com> > --- > hmp-commands.hx | 14 ++++++++++++++ > hw/core/machine-hmp-cmds.c | 5 +++++ > hw/core/machine-qmp-cmds.c | 9 +++++++++ > include/monitor/hmp.h | 1 + > qapi/machine.json | 18 ++++++++++++++++++ > qapi/pragma.json | 1 + > 6 files changed, 48 insertions(+) > > diff --git a/hmp-commands.hx b/hmp-commands.hx > index 765349ed14..bd01e49ec5 100644 > --- a/hmp-commands.hx > +++ b/hmp-commands.hx > @@ -652,6 +652,20 @@ SRST > whether profiling is on or off. > ERST > > + { > + .name = "system_sleep", > + .args_type = "", > + .params = "", > + .help = "send ACPI sleep event", Suggest "push the virtual sleep button", because it's easier to understand, and consistent with the documentation below. > + .cmd = hmp_system_sleep, > + }, > + > +SRST > +``system_sleep`` > + Push the virtual sleep button; if supported the system will enter > + an ACPI sleep state. > +ERST > + > { > .name = "system_reset", > .args_type = "", > diff --git a/hw/core/machine-hmp-cmds.c b/hw/core/machine-hmp-cmds.c > index a6ff6a4875..641a365e3e 100644 > --- a/hw/core/machine-hmp-cmds.c > +++ b/hw/core/machine-hmp-cmds.c > @@ -185,6 +185,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/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c > index 3860a50c3b..9f1e636c90 100644 > --- a/hw/core/machine-qmp-cmds.c > +++ b/hw/core/machine-qmp-cmds.c > @@ -257,6 +257,15 @@ void qmp_system_reset(Error **errp) > qemu_system_reset_request(SHUTDOWN_CAUSE_HOST_QMP_SYSTEM_RESET); > } > > +void qmp_system_sleep(Error **errp) > +{ > + if (!qemu_wakeup_suspend_enabled()) { > + error_setg(errp, > + "suspend from running is not supported by this guest"); > + return; > + } This can't be right: it either fails or does nothing. I guess you're leaving the "do something" part to later patches. That's okay, but it needs a TODO comment here, and a prominent mention in the commit message. > +} > + > void qmp_system_powerdown(Error **errp) > { > qemu_system_powerdown_request(); > diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h > index 13f9a2dedb..d72a3b775c 100644 > --- a/include/monitor/hmp.h > +++ b/include/monitor/hmp.h > @@ -45,6 +45,7 @@ void hmp_quit(Monitor *mon, const QDict *qdict); > void hmp_stop(Monitor *mon, const QDict *qdict); > void hmp_sync_profile(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_exit_preconfig(Monitor *mon, const QDict *qdict); > void hmp_announce_self(Monitor *mon, const QDict *qdict); > diff --git a/qapi/machine.json b/qapi/machine.json > index b6d634b30d..3ac69df92f 100644 > --- a/qapi/machine.json > +++ b/qapi/machine.json > @@ -297,6 +297,24 @@ > ## > { 'command': 'system_reset' } > > +## > +# @system_sleep: > +# > +# Requests that a guest perform a ACPI sleep transition by pushing a virtual > +# sleep button. Imperative mood, please: "Request that ..." I think "the guest" would be better. Limit line length to 70, please. > +# > +# 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. Please format like # 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. > +# > +# Example: > +# > +# -> { "execute": "system_sleep" } > +# <- { "return": {} } > +# > +## > +{ 'command': 'system_sleep' } > + > ## > # @system_powerdown: > # > diff --git a/qapi/pragma.json b/qapi/pragma.json > index 0aa4eeddd3..ef15229854 100644 > --- a/qapi/pragma.json > +++ b/qapi/pragma.json > @@ -23,6 +23,7 @@ > 'set_password', > 'system_powerdown', > 'system_reset', > + 'system_sleep', > 'system_wakeup' ], > # Commands allowed to return a non-dictionary > 'command-returns-exceptions': [ I figure you spell system_sleep with '_' instead of '-' for consistency with existing system_FOO commands. That's okay, but I recommend to point it out in the commit message.
Hi Markus, On 12/5/2023 3:34 PM, Markus Armbruster wrote: > You neglected to cc: QAPI schema maintainers. I found it by chance. > Next time :) Yep, should have cc to the maintainers. > > Annie Li <annie.li@oracle.com> writes: > >> Following hmp/qmp commands are implemented for pressing virtual >> sleep button, >> >> hmp: system_sleep >> qmp: { "execute": "system_sleep" } >> >> These commands put the guest into suspend or other power states >> depending on the power settings inside the guest. > How is this related to system_wakeup? Both 'system_sleep' and 'system_wakeup' trigger the event to notify the guest OSPM the sleep button has been pressed. 'system_wakeup' triggers wake up notification when the guest is in suspend state. > >> Signed-off-by: Annie Li <annie.li@oracle.com> >> --- >> hmp-commands.hx | 14 ++++++++++++++ >> hw/core/machine-hmp-cmds.c | 5 +++++ >> hw/core/machine-qmp-cmds.c | 9 +++++++++ >> include/monitor/hmp.h | 1 + >> qapi/machine.json | 18 ++++++++++++++++++ >> qapi/pragma.json | 1 + >> 6 files changed, 48 insertions(+) >> >> diff --git a/hmp-commands.hx b/hmp-commands.hx >> index 765349ed14..bd01e49ec5 100644 >> --- a/hmp-commands.hx >> +++ b/hmp-commands.hx >> @@ -652,6 +652,20 @@ SRST >> whether profiling is on or off. >> ERST >> >> + { >> + .name = "system_sleep", >> + .args_type = "", >> + .params = "", >> + .help = "send ACPI sleep event", > Suggest "push the virtual sleep button", because it's easier to > understand, and consistent with the documentation below. ACK > >> + .cmd = hmp_system_sleep, >> + }, >> + >> +SRST >> +``system_sleep`` >> + Push the virtual sleep button; if supported the system will enter >> + an ACPI sleep state. >> +ERST >> + >> { >> .name = "system_reset", >> .args_type = "", >> diff --git a/hw/core/machine-hmp-cmds.c b/hw/core/machine-hmp-cmds.c >> index a6ff6a4875..641a365e3e 100644 >> --- a/hw/core/machine-hmp-cmds.c >> +++ b/hw/core/machine-hmp-cmds.c >> @@ -185,6 +185,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/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c >> index 3860a50c3b..9f1e636c90 100644 >> --- a/hw/core/machine-qmp-cmds.c >> +++ b/hw/core/machine-qmp-cmds.c >> @@ -257,6 +257,15 @@ void qmp_system_reset(Error **errp) >> qemu_system_reset_request(SHUTDOWN_CAUSE_HOST_QMP_SYSTEM_RESET); >> } >> >> +void qmp_system_sleep(Error **errp) >> +{ >> + if (!qemu_wakeup_suspend_enabled()) { >> + error_setg(errp, >> + "suspend from running is not supported by this guest"); >> + return; >> + } > This can't be right: it either fails or does nothing. > > I guess you're leaving the "do something" part to later patches. Right. This patch focuses on hmp/qmp, more implementations are in patch 6. > That's > okay, but it needs a TODO comment here, and a prominent mention in the > commit message. Good point > >> +} >> + >> void qmp_system_powerdown(Error **errp) >> { >> qemu_system_powerdown_request(); >> diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h >> index 13f9a2dedb..d72a3b775c 100644 >> --- a/include/monitor/hmp.h >> +++ b/include/monitor/hmp.h >> @@ -45,6 +45,7 @@ void hmp_quit(Monitor *mon, const QDict *qdict); >> void hmp_stop(Monitor *mon, const QDict *qdict); >> void hmp_sync_profile(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_exit_preconfig(Monitor *mon, const QDict *qdict); >> void hmp_announce_self(Monitor *mon, const QDict *qdict); >> diff --git a/qapi/machine.json b/qapi/machine.json >> index b6d634b30d..3ac69df92f 100644 >> --- a/qapi/machine.json >> +++ b/qapi/machine.json >> @@ -297,6 +297,24 @@ >> ## >> { 'command': 'system_reset' } >> >> +## >> +# @system_sleep: >> +# >> +# Requests that a guest perform a ACPI sleep transition by pushing a virtual >> +# sleep button. > Imperative mood, please: "Request that ..." > > I think "the guest" would be better. > > Limit line length to 70, please. ACK > >> +# >> +# 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. > Please format like > > # 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. > ACK >> +# >> +# Example: >> +# >> +# -> { "execute": "system_sleep" } >> +# <- { "return": {} } >> +# >> +## >> +{ 'command': 'system_sleep' } >> + >> ## >> # @system_powerdown: >> # >> diff --git a/qapi/pragma.json b/qapi/pragma.json >> index 0aa4eeddd3..ef15229854 100644 >> --- a/qapi/pragma.json >> +++ b/qapi/pragma.json >> @@ -23,6 +23,7 @@ >> 'set_password', >> 'system_powerdown', >> 'system_reset', >> + 'system_sleep', >> 'system_wakeup' ], >> # Commands allowed to return a non-dictionary >> 'command-returns-exceptions': [ > I figure you spell system_sleep with '_' instead of '-' for consistency > with existing system_FOO commands. Yep > That's okay, but I recommend to > point it out in the commit message. Will do. Thanks Annie
"Annie.li" <annie.li@oracle.com> writes: > Hi Markus, > > On 12/5/2023 3:34 PM, Markus Armbruster wrote: >> You neglected to cc: QAPI schema maintainers. I found it by chance. >> Next time :) > Yep, should have cc to the maintainers. >> >> Annie Li <annie.li@oracle.com> writes: >> >>> Following hmp/qmp commands are implemented for pressing virtual >>> sleep button, >>> >>> hmp: system_sleep >>> qmp: { "execute": "system_sleep" } >>> >>> These commands put the guest into suspend or other power states >>> depending on the power settings inside the guest. >> >> How is this related to system_wakeup? > > Both 'system_sleep' and 'system_wakeup' trigger the event to notify the > guest OSPM the sleep button has been pressed. 'system_wakeup' triggers > wake up notification when the guest is in suspend state. Thanks. Would it make sens to work this into the QAPI schema doc comments somehow? [...]
On 12/22/2023 7:37 AM, Markus Armbruster wrote: > "Annie.li" <annie.li@oracle.com> writes: > >> Hi Markus, >> >> On 12/5/2023 3:34 PM, Markus Armbruster wrote: >>> You neglected to cc: QAPI schema maintainers. I found it by chance. >>> Next time :) >> Yep, should have cc to the maintainers. >>> Annie Li <annie.li@oracle.com> writes: >>> >>>> Following hmp/qmp commands are implemented for pressing virtual >>>> sleep button, >>>> >>>> hmp: system_sleep >>>> qmp: { "execute": "system_sleep" } >>>> >>>> These commands put the guest into suspend or other power states >>>> depending on the power settings inside the guest. >>> How is this related to system_wakeup? >> Both 'system_sleep' and 'system_wakeup' trigger the event to notify the >> guest OSPM the sleep button has been pressed. 'system_wakeup' triggers >> wake up notification when the guest is in suspend state. > Thanks. Would it make sens to work this into the QAPI schema doc > comments somehow? Sure. It totally makes sense. Annie
diff --git a/hmp-commands.hx b/hmp-commands.hx index 765349ed14..bd01e49ec5 100644 --- a/hmp-commands.hx +++ b/hmp-commands.hx @@ -652,6 +652,20 @@ SRST whether profiling is on or off. ERST + { + .name = "system_sleep", + .args_type = "", + .params = "", + .help = "send ACPI sleep event", + .cmd = hmp_system_sleep, + }, + +SRST +``system_sleep`` + Push the virtual sleep button; if supported the system will enter + an ACPI sleep state. +ERST + { .name = "system_reset", .args_type = "", diff --git a/hw/core/machine-hmp-cmds.c b/hw/core/machine-hmp-cmds.c index a6ff6a4875..641a365e3e 100644 --- a/hw/core/machine-hmp-cmds.c +++ b/hw/core/machine-hmp-cmds.c @@ -185,6 +185,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/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c index 3860a50c3b..9f1e636c90 100644 --- a/hw/core/machine-qmp-cmds.c +++ b/hw/core/machine-qmp-cmds.c @@ -257,6 +257,15 @@ void qmp_system_reset(Error **errp) qemu_system_reset_request(SHUTDOWN_CAUSE_HOST_QMP_SYSTEM_RESET); } +void qmp_system_sleep(Error **errp) +{ + if (!qemu_wakeup_suspend_enabled()) { + error_setg(errp, + "suspend from running is not supported by this guest"); + return; + } +} + void qmp_system_powerdown(Error **errp) { qemu_system_powerdown_request(); diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h index 13f9a2dedb..d72a3b775c 100644 --- a/include/monitor/hmp.h +++ b/include/monitor/hmp.h @@ -45,6 +45,7 @@ void hmp_quit(Monitor *mon, const QDict *qdict); void hmp_stop(Monitor *mon, const QDict *qdict); void hmp_sync_profile(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_exit_preconfig(Monitor *mon, const QDict *qdict); void hmp_announce_self(Monitor *mon, const QDict *qdict); diff --git a/qapi/machine.json b/qapi/machine.json index b6d634b30d..3ac69df92f 100644 --- a/qapi/machine.json +++ b/qapi/machine.json @@ -297,6 +297,24 @@ ## { '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. +# +# Example: +# +# -> { "execute": "system_sleep" } +# <- { "return": {} } +# +## +{ 'command': 'system_sleep' } + ## # @system_powerdown: # diff --git a/qapi/pragma.json b/qapi/pragma.json index 0aa4eeddd3..ef15229854 100644 --- a/qapi/pragma.json +++ b/qapi/pragma.json @@ -23,6 +23,7 @@ 'set_password', 'system_powerdown', 'system_reset', + 'system_sleep', 'system_wakeup' ], # Commands allowed to return a non-dictionary 'command-returns-exceptions': [
Following hmp/qmp commands are implemented for pressing virtual sleep button, hmp: system_sleep qmp: { "execute": "system_sleep" } These commands put the guest into suspend or other power states depending on the power settings inside the guest. Signed-off-by: Annie Li <annie.li@oracle.com> --- hmp-commands.hx | 14 ++++++++++++++ hw/core/machine-hmp-cmds.c | 5 +++++ hw/core/machine-qmp-cmds.c | 9 +++++++++ include/monitor/hmp.h | 1 + qapi/machine.json | 18 ++++++++++++++++++ qapi/pragma.json | 1 + 6 files changed, 48 insertions(+)