Re: [RFC V3 PATCH 05/13] acpi: Send the GPE event of suspend and wakeup for x86

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

Hi Annie,

On 4/16/25 03:24, Philippe Mathieu-Daudé wrote:
> On 15/4/25 23:48, Annie Li wrote:
>>
>> On 4/15/2025 11:29 AM, Philippe Mathieu-Daudé wrote:
>>> Hi Annie,
>>>
>>> On 15/4/25 03:24, Annie Li wrote:
>>>>
>>>> On 4/14/2025 11:18 AM, Alex Bennée wrote:
>>>>> Annie Li <annie.li@oracle.com> writes:
>>>>>
>>>>>> The GPE event is triggered to notify x86 guest to suppend
>>>>>> itself. The function acpi_send_sleep_event will also
>>>>>> trigger GED events on HW-reduced systems where ACPI GED
>>>>>> sleep event is supported.
>>>>>>
>>>>>> Signed-off-by: Annie Li <annie.li@oracle.com>
>>>>>> ---
>>>>>>   hw/acpi/core.c                       | 10 ++++++++++
>>>>>>   include/hw/acpi/acpi.h               |  1 +
>>>>>>   include/hw/acpi/acpi_dev_interface.h |  1 +
>>>>>>   3 files changed, 12 insertions(+)
>>>>>>
>>>>>> diff --git a/hw/acpi/core.c b/hw/acpi/core.c
>>>>>> index 58f8964e13..00a9d226f0 100644
>>>>>> --- a/hw/acpi/core.c
>>>>>> +++ b/hw/acpi/core.c
>>>>>> @@ -359,6 +359,16 @@ int acpi_get_slic_oem(AcpiSlicOem *oem)
>>>>>>       return -1;
>>>>>>   }
>>>>>> +void acpi_send_sleep_event(void)
>>>>>> +{
>>>>>> +    Object *obj = object_resolve_path_type("", TYPE_ACPI_DEVICE_IF,
>>>>>> NULL);
>>>>> Is it a fair assumption there will only ever be one QOM object that
>>>>> provides the TYPE_ACPI_DEVICE_IF interface on a system?
>>>>
>>>> I supposed it was, but I might be wrong(seeing some classes have the same interface). Please correct me if I've missed something, thank you!
>>>
>>>     /**
>>>      * object_resolve_path_type:
>>>      * @path: the path to resolve
>>>      * @typename: the type to look for.
>>>      * @ambiguous: (out) (optional): location to store whether the
>>>      *             lookup failed because it was ambiguous, or %NULL.
>>>      *             Set to %false on success.
>>>
>>> Since you use ambiguous=NULL, your code will only set %obj if there
>>> is exactly ONE device implementing the ACPI_DEVICE interface created.
>>>
>>> So far IIUC nothing forbids creating multiple ones, so if you expect
>>> only one, you should add code to handle the "2 or more" case. Or at
>>> least add a comment.
>> Actually, there is only one QOM object here.
>> There are 3 classes involves with TYPE_ACPI_DEVICE_IF interface.
>> PC, Q35, GED.
>> For x86 system, the PC or Q35 machine doesn't use GED event, instead,
>> it sends the GPE event.
>> For microvm/ARM/virt system, GED device is used, its own TYPE_ACPI_DEVICE_IF
>> is involved.
>> All these objects do not exist at the same time, so it is safe to assume only one QOM
>> object exists here.
> 
> I agree this is the case as of today, but I'm not sure about tomorrow.
> AFAICT nothing prohibit instanciating more than 1 type implementing
> TYPE_ACPI_DEVICE_IF.
> 
> Anyway we are just trying to be more cautious here; up to the
> maintainer.

Annie, very soon probably the GPEX device will use the TYPE_ACPI_DEVICE_IF as well.
GPEX is used in Arm machines (like "virt") and due to the support for ACPI PCI hotplug,
which needs to be routed through the PCIe controller's hotplug handlers and the ACPI pcihp
device before the hotplug event is finally delivered to the OS via the GED device, it also
will use this ACPI interface.

I can think of something when adding the ACPI PCI hotplug support on machine with GPEX,
but even for future cases, could you please add at least a check for the ambiguity and
assert for no ambiguity here?

I suspect the only case we can have an ambiguity is when we have a device that
needs to do something more complex and then deliver an event via GED, so specifically
here for the sleep button I think it's ok to, in case of ambiguity, select the GED
device by default.


Cheers,
Gustavo