Hi Gustavo,
On 4/17/2025 1:34 PM, Gustavo Romero wrote:
> 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"
Ack
>
>
>> 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" ?
Agree. I should have updated the comments here also, this was kept
from the existing code. Since the ACPI spec has been updated, the
comments here should be updated also.
Thanks
Annie
>
>
> 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"
>