Re: [RFC V3 PATCH 03/13] acpi: Support Control Method sleep button for x86

[Annie Li <annie.li@oracle.com>]

Hi Gustavo,

On 4/17/2025 1:30 PM, Gustavo Romero wrote:
> Hi Annie,
>
> On 4/11/25 17:34, Annie Li wrote:
>> Add Control Method Sleep button and its GPE event handler for
>> x86 platform. The GPE event handler notifies OSPM when the
>> Sleep button event is triggered.
>>
>> Signed-off-by: Annie Li <annie.li@oracle.com>
>> ---
>>   hw/i386/acpi-build.c | 22 +++++++++++++++++++++-
>>   1 file changed, 21 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index 3fffa4a332..4be3595e5a 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -40,6 +40,7 @@
>>   #include "hw/acpi/acpi_aml_interface.h"
>>   #include "hw/input/i8042.h"
>>   #include "hw/acpi/memory_hotplug.h"
>> +#include "hw/acpi/control_method_device.h"
>>   #include "system/tpm.h"
>>   #include "hw/acpi/tpm.h"
>>   #include "hw/acpi/vmgenid.h"
>> @@ -1359,7 +1360,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>>                                                        NULL);
>>       Object *q35 = 
>> object_resolve_type_unambiguous(TYPE_Q35_HOST_DEVICE, NULL);
>>       CrsRangeEntry *entry;
>> -    Aml *dsdt, *sb_scope, *scope, *dev, *method, *field, *pkg, *crs;
>> +    Aml *dsdt, *sb_scope, *scope, *dev, *method, *field, *pkg, *crs, 
>> *condition;
>>       CrsRangeSet crs_range_set;
>>       PCMachineState *pcms = PC_MACHINE(machine);
>>       PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(machine);
>> @@ -1465,6 +1466,25 @@ build_dsdt(GArray *table_data, BIOSLinker 
>> *linker,
>>       }
>>       aml_append(dsdt, scope);
>>   +    sb_scope = aml_scope("_SB");
>> +    acpi_dsdt_add_sleep_button(sb_scope);
>> +    aml_append(dsdt, sb_scope);
>> +
>> +    /*
>> +     * The event handler for the control method sleep button is 
>> generated
>> +     * for notifying OSPM[ACPI v6.5 Section 4.8.2.2.2.].
>
> Typo: Section 4.8.2.2.2.2
>
> Please use the form "... OSPM (ACPI v6.5, Section 4.8.2.2.2.2)" for 
> citations.
>
Ack
>
>> +     */
>> +    scope =  aml_scope("\\_GPE");
>> +    method = aml_method("_L07", 0, AML_NOTSERIALIZED);
>> +    condition = aml_if(aml_name("\\_SB.SLPB.SBP"));
>> +    aml_append(condition, aml_store(aml_int(1), 
>> aml_name("\\_SB.SLPB.SBP")));
>
> Why not concatenate with ACPI_SLEEP_BUTTON_DEVICE here and in the 
> conditional
> above it as it's done below for the Notify()?

Make sense.

Thanks

Annie

>
>
> Cheers,
> Gustavo
>
>> +    aml_append(condition,
>> + aml_notify(aml_name("\\_SB."ACPI_SLEEP_BUTTON_DEVICE),
>> +                                    aml_int(0x80)));
>> +    aml_append(method, condition);
>> +    aml_append(scope, method);
>> +    aml_append(dsdt, scope);
>> +
>>       if (pcmc->legacy_cpu_hotplug) {
>>           build_legacy_cpu_hotplug_aml(dsdt, machine, 
>> pm->cpu_hp_io_base);
>>       } else {
>