diff mbox series

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

Message ID 20250411204133.2955-1-annie.li@oracle.com (mailing list archive)
State New
Headers show
Series Support ACPI Control Method Sleep button | expand

Commit Message

Annie Li April 11, 2025, 8:41 p.m. UTC
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(+)

Comments

Alex Bennée April 14, 2025, 3:18 p.m. UTC | #1
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?

> +
> +    if (obj) {
> +        /* Send sleep event */
> +        acpi_send_event(DEVICE(obj), ACPI_SLEEP_STATUS);
> +    }
> +}
> +
>  static void acpi_notify_wakeup(Notifier *notifier, void *data)
>  {
>      ACPIREGS *ar = container_of(notifier, ACPIREGS, wakeup);
> diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h
> index d1a4fa2af8..64d3ff78ed 100644
> --- a/include/hw/acpi/acpi.h
> +++ b/include/hw/acpi/acpi.h
> @@ -184,6 +184,7 @@ uint32_t acpi_gpe_ioport_readb(ACPIREGS *ar, uint32_t addr);
>  
>  void acpi_send_gpe_event(ACPIREGS *ar, qemu_irq irq,
>                           AcpiEventStatusBits status);
> +void acpi_send_sleep_event(void);
>  
>  void acpi_update_sci(ACPIREGS *acpi_regs, qemu_irq irq);
>  
> diff --git a/include/hw/acpi/acpi_dev_interface.h b/include/hw/acpi/acpi_dev_interface.h
> index 68d9d15f50..1cb050cd3a 100644
> --- a/include/hw/acpi/acpi_dev_interface.h
> +++ b/include/hw/acpi/acpi_dev_interface.h
> @@ -13,6 +13,7 @@ typedef enum {
>      ACPI_NVDIMM_HOTPLUG_STATUS = 16,
>      ACPI_VMGENID_CHANGE_STATUS = 32,
>      ACPI_POWER_DOWN_STATUS = 64,
> +    ACPI_SLEEP_STATUS = 128,
>  } AcpiEventStatusBits;
>  
>  #define TYPE_ACPI_DEVICE_IF "acpi-device-interface"
Annie Li April 15, 2025, 1:24 a.m. UTC | #2
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!

Thanks

Annie

>> +
>> +    if (obj) {
>> +        /* Send sleep event */
>> +        acpi_send_event(DEVICE(obj), ACPI_SLEEP_STATUS);
>> +    }
>> +}
>> +
>>   static void acpi_notify_wakeup(Notifier *notifier, void *data)
>>   {
>>       ACPIREGS *ar = container_of(notifier, ACPIREGS, wakeup);
>> diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h
>> index d1a4fa2af8..64d3ff78ed 100644
>> --- a/include/hw/acpi/acpi.h
>> +++ b/include/hw/acpi/acpi.h
>> @@ -184,6 +184,7 @@ uint32_t acpi_gpe_ioport_readb(ACPIREGS *ar, uint32_t addr);
>>   
>>   void acpi_send_gpe_event(ACPIREGS *ar, qemu_irq irq,
>>                            AcpiEventStatusBits status);
>> +void acpi_send_sleep_event(void);
>>   
>>   void acpi_update_sci(ACPIREGS *acpi_regs, qemu_irq irq);
>>   
>> diff --git a/include/hw/acpi/acpi_dev_interface.h b/include/hw/acpi/acpi_dev_interface.h
>> index 68d9d15f50..1cb050cd3a 100644
>> --- a/include/hw/acpi/acpi_dev_interface.h
>> +++ b/include/hw/acpi/acpi_dev_interface.h
>> @@ -13,6 +13,7 @@ typedef enum {
>>       ACPI_NVDIMM_HOTPLUG_STATUS = 16,
>>       ACPI_VMGENID_CHANGE_STATUS = 32,
>>       ACPI_POWER_DOWN_STATUS = 64,
>> +    ACPI_SLEEP_STATUS = 128,
>>   } AcpiEventStatusBits;
>>   
>>   #define TYPE_ACPI_DEVICE_IF "acpi-device-interface"
Philippe Mathieu-Daudé April 15, 2025, 3:29 p.m. UTC | #3
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.

Regards,

Phil.

>>> +
>>> +    if (obj) {
>>> +        /* Send sleep event */
>>> +        acpi_send_event(DEVICE(obj), ACPI_SLEEP_STATUS);
>>> +    }
>>> +}
>>> +
>>>   static void acpi_notify_wakeup(Notifier *notifier, void *data)
>>>   {
>>>       ACPIREGS *ar = container_of(notifier, ACPIREGS, wakeup);
>>> diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h
>>> index d1a4fa2af8..64d3ff78ed 100644
>>> --- a/include/hw/acpi/acpi.h
>>> +++ b/include/hw/acpi/acpi.h
>>> @@ -184,6 +184,7 @@ uint32_t acpi_gpe_ioport_readb(ACPIREGS *ar, 
>>> uint32_t addr);
>>>   void acpi_send_gpe_event(ACPIREGS *ar, qemu_irq irq,
>>>                            AcpiEventStatusBits status);
>>> +void acpi_send_sleep_event(void);
>>>   void acpi_update_sci(ACPIREGS *acpi_regs, qemu_irq irq);
>>> diff --git a/include/hw/acpi/acpi_dev_interface.h b/include/hw/acpi/ 
>>> acpi_dev_interface.h
>>> index 68d9d15f50..1cb050cd3a 100644
>>> --- a/include/hw/acpi/acpi_dev_interface.h
>>> +++ b/include/hw/acpi/acpi_dev_interface.h
>>> @@ -13,6 +13,7 @@ typedef enum {
>>>       ACPI_NVDIMM_HOTPLUG_STATUS = 16,
>>>       ACPI_VMGENID_CHANGE_STATUS = 32,
>>>       ACPI_POWER_DOWN_STATUS = 64,
>>> +    ACPI_SLEEP_STATUS = 128,
>>>   } AcpiEventStatusBits;
>>>   #define TYPE_ACPI_DEVICE_IF "acpi-device-interface"
Annie Li April 15, 2025, 9:48 p.m. UTC | #4
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.

Thanks
Annie
>
> Regards,
>
> Phil.
>
>>>> +
>>>> +    if (obj) {
>>>> +        /* Send sleep event */
>>>> +        acpi_send_event(DEVICE(obj), ACPI_SLEEP_STATUS);
>>>> +    }
>>>> +}
>>>> +
>>>>   static void acpi_notify_wakeup(Notifier *notifier, void *data)
>>>>   {
>>>>       ACPIREGS *ar = container_of(notifier, ACPIREGS, wakeup);
>>>> diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h
>>>> index d1a4fa2af8..64d3ff78ed 100644
>>>> --- a/include/hw/acpi/acpi.h
>>>> +++ b/include/hw/acpi/acpi.h
>>>> @@ -184,6 +184,7 @@ uint32_t acpi_gpe_ioport_readb(ACPIREGS *ar, 
>>>> uint32_t addr);
>>>>   void acpi_send_gpe_event(ACPIREGS *ar, qemu_irq irq,
>>>>                            AcpiEventStatusBits status);
>>>> +void acpi_send_sleep_event(void);
>>>>   void acpi_update_sci(ACPIREGS *acpi_regs, qemu_irq irq);
>>>> diff --git a/include/hw/acpi/acpi_dev_interface.h 
>>>> b/include/hw/acpi/ acpi_dev_interface.h
>>>> index 68d9d15f50..1cb050cd3a 100644
>>>> --- a/include/hw/acpi/acpi_dev_interface.h
>>>> +++ b/include/hw/acpi/acpi_dev_interface.h
>>>> @@ -13,6 +13,7 @@ typedef enum {
>>>>       ACPI_NVDIMM_HOTPLUG_STATUS = 16,
>>>>       ACPI_VMGENID_CHANGE_STATUS = 32,
>>>>       ACPI_POWER_DOWN_STATUS = 64,
>>>> +    ACPI_SLEEP_STATUS = 128,
>>>>   } AcpiEventStatusBits;
>>>>   #define TYPE_ACPI_DEVICE_IF "acpi-device-interface"
>
Philippe Mathieu-Daudé April 16, 2025, 6:24 a.m. UTC | #5
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.

Regards,

Phil.

> 
> Thanks
> Annie
>>
>> Regards,
>>
>> Phil.
>>
>>>>> +
>>>>> +    if (obj) {
>>>>> +        /* Send sleep event */
>>>>> +        acpi_send_event(DEVICE(obj), ACPI_SLEEP_STATUS);
>>>>> +    }
>>>>> +}
>>>>> +
>>>>>   static void acpi_notify_wakeup(Notifier *notifier, void *data)
>>>>>   {
>>>>>       ACPIREGS *ar = container_of(notifier, ACPIREGS, wakeup);
>>>>> diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h
>>>>> index d1a4fa2af8..64d3ff78ed 100644
>>>>> --- a/include/hw/acpi/acpi.h
>>>>> +++ b/include/hw/acpi/acpi.h
>>>>> @@ -184,6 +184,7 @@ uint32_t acpi_gpe_ioport_readb(ACPIREGS *ar, 
>>>>> uint32_t addr);
>>>>>   void acpi_send_gpe_event(ACPIREGS *ar, qemu_irq irq,
>>>>>                            AcpiEventStatusBits status);
>>>>> +void acpi_send_sleep_event(void);
>>>>>   void acpi_update_sci(ACPIREGS *acpi_regs, qemu_irq irq);
>>>>> diff --git a/include/hw/acpi/acpi_dev_interface.h b/include/hw/ 
>>>>> acpi/ acpi_dev_interface.h
>>>>> index 68d9d15f50..1cb050cd3a 100644
>>>>> --- a/include/hw/acpi/acpi_dev_interface.h
>>>>> +++ b/include/hw/acpi/acpi_dev_interface.h
>>>>> @@ -13,6 +13,7 @@ typedef enum {
>>>>>       ACPI_NVDIMM_HOTPLUG_STATUS = 16,
>>>>>       ACPI_VMGENID_CHANGE_STATUS = 32,
>>>>>       ACPI_POWER_DOWN_STATUS = 64,
>>>>> +    ACPI_SLEEP_STATUS = 128,
>>>>>   } AcpiEventStatusBits;
>>>>>   #define TYPE_ACPI_DEVICE_IF "acpi-device-interface"
>>
diff mbox series

Patch

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);
+
+    if (obj) {
+        /* Send sleep event */
+        acpi_send_event(DEVICE(obj), ACPI_SLEEP_STATUS);
+    }
+}
+
 static void acpi_notify_wakeup(Notifier *notifier, void *data)
 {
     ACPIREGS *ar = container_of(notifier, ACPIREGS, wakeup);
diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h
index d1a4fa2af8..64d3ff78ed 100644
--- a/include/hw/acpi/acpi.h
+++ b/include/hw/acpi/acpi.h
@@ -184,6 +184,7 @@  uint32_t acpi_gpe_ioport_readb(ACPIREGS *ar, uint32_t addr);
 
 void acpi_send_gpe_event(ACPIREGS *ar, qemu_irq irq,
                          AcpiEventStatusBits status);
+void acpi_send_sleep_event(void);
 
 void acpi_update_sci(ACPIREGS *acpi_regs, qemu_irq irq);
 
diff --git a/include/hw/acpi/acpi_dev_interface.h b/include/hw/acpi/acpi_dev_interface.h
index 68d9d15f50..1cb050cd3a 100644
--- a/include/hw/acpi/acpi_dev_interface.h
+++ b/include/hw/acpi/acpi_dev_interface.h
@@ -13,6 +13,7 @@  typedef enum {
     ACPI_NVDIMM_HOTPLUG_STATUS = 16,
     ACPI_VMGENID_CHANGE_STATUS = 32,
     ACPI_POWER_DOWN_STATUS = 64,
+    ACPI_SLEEP_STATUS = 128,
 } AcpiEventStatusBits;
 
 #define TYPE_ACPI_DEVICE_IF "acpi-device-interface"