Re: [RFC V3 PATCH 07/13] microvm: support control method sleep button

[Gustavo Romero <gustavo.romero@linaro.org>]

Hi Annie,

On 4/11/25 17:42, Annie Li wrote:
> Add the support of control method sleep button and System
> S3 Sleeping State for microvm.

I would say "... of ACPI Control Method Sleeping Button ...¨,
the important part being "ACPI" to make clear what's the
context of the support. Because such a device requires
also some plumbing in QEMU code to really be "supported".

For the title, maybe smth like: "microvm: Add ACPI sleeping button device"


> Signed-off-by: Annie Li <annie.li@oracle.com>
> ---
>   hw/i386/acpi-microvm.c                 | 11 +++++++++++
>   include/hw/acpi/generic_event_device.h |  1 +
>   2 files changed, 12 insertions(+)
> 
> diff --git a/hw/i386/acpi-microvm.c b/hw/i386/acpi-microvm.c
> index 279da6b4aa..57c45ea327 100644
> --- a/hw/i386/acpi-microvm.c
> +++ b/hw/i386/acpi-microvm.c
> @@ -32,6 +32,7 @@
>   #include "hw/acpi/generic_event_device.h"
>   #include "hw/acpi/utils.h"
>   #include "hw/acpi/erst.h"
> +#include "hw/acpi/control_method_device.h"
>   #include "hw/i386/fw_cfg.h"
>   #include "hw/i386/microvm.h"
>   #include "hw/pci/pci.h"
> @@ -123,12 +124,22 @@ build_dsdt_microvm(GArray *table_data, BIOSLinker *linker,
>       build_ged_aml(sb_scope, GED_DEVICE, x86ms->acpi_dev,
>                     GED_MMIO_IRQ, AML_SYSTEM_MEMORY, GED_MMIO_BASE);
>       acpi_dsdt_add_power_button(sb_scope);
> +    acpi_dsdt_add_sleep_button(sb_scope);
>       acpi_dsdt_add_virtio(sb_scope, mms);
>       acpi_dsdt_add_xhci(sb_scope, mms);
>       acpi_dsdt_add_pci(sb_scope, mms);
>       aml_append(dsdt, sb_scope);
>   
>       /* ACPI 5.0: Table 7-209 System State Package */

Should this be "Table 7-205"? Or even, why not:

"ACPI 6.5, Table 7.11: System State Package" ?


Cheers,
Gustavo

> +    scope = aml_scope("\\");
> +    pkg = aml_package(4);
> +    aml_append(pkg, aml_int(ACPI_GED_SLP_TYP_S3));
> +    aml_append(pkg, aml_int(0)); /* ignored */
> +    aml_append(pkg, aml_int(0)); /* reserved */
> +    aml_append(pkg, aml_int(0)); /* reserved */
> +    aml_append(scope, aml_name_decl("_S3", pkg));
> +    aml_append(dsdt, scope);
> +
>       scope = aml_scope("\\");
>       pkg = aml_package(4);
>       aml_append(pkg, aml_int(ACPI_GED_SLP_TYP_S5));
> diff --git a/include/hw/acpi/generic_event_device.h b/include/hw/acpi/generic_event_device.h
> index d2dac87b4a..28c5785863 100644
> --- a/include/hw/acpi/generic_event_device.h
> +++ b/include/hw/acpi/generic_event_device.h
> @@ -85,6 +85,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(AcpiGedState, ACPI_GED)
>   #define ACPI_GED_SLP_TYP_POS       0x2   /* SLP_TYPx Bit Offset */
>   #define ACPI_GED_SLP_TYP_MASK      0x07  /* SLP_TYPx 3-bit mask */
>   #define ACPI_GED_SLP_TYP_S5        0x05  /* System _S5 State (Soft Off) */
> +#define ACPI_GED_SLP_TYP_S3        0x03  /* System _S3 State (Sleeping State) */
>   #define ACPI_GED_SLP_EN            0x20  /* SLP_EN write-only bit */
>   
>   #define GED_DEVICE      "GED"