1. Patchew
  2. QEMU
  3. [RFC 00/24] APCI PCI Hotplug support on ARM
  4. View patch

[RFC 18/24] hw/acpi/ged: Prepare the device to react to PCI hotplug events

Eric Auger posted 24 patches 9 months, 2 weeks ago
[RFC 18/24] hw/acpi/ged: Prepare the device to react to PCI hotplug events
Posted by Eric Auger 9 months, 2 weeks ago 
QEMU will notify the OS about PCI hotplug/hotunplug events through
GED interrupts. Let the GED device handle a new PCI hotplug event.
On its occurence it calls the \\_SB.PCI0.PCNT method with the BLCK
mutex held.

The GED device uses a dedicated MMIO region that will be mapped
by the machine code.

At this point the GED still does not support PCI device hotplug in
its TYPE_HOTPLUG_HANDLER implementation. This will come in a
subsequent patch

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 include/hw/acpi/generic_event_device.h |  5 +++++
 include/hw/acpi/pcihp.h                |  2 ++
 hw/acpi/generic_event_device.c         | 14 ++++++++++++++
 3 files changed, 21 insertions(+)

diff --git a/include/hw/acpi/generic_event_device.h b/include/hw/acpi/generic_event_device.h
index d2dac87b4a..28be6c0582 100644
--- a/include/hw/acpi/generic_event_device.h
+++ b/include/hw/acpi/generic_event_device.h
@@ -63,6 +63,7 @@
 #include "hw/acpi/memory_hotplug.h"
 #include "hw/acpi/ghes.h"
 #include "hw/acpi/cpu.h"
+#include "hw/acpi/pcihp.h"
 #include "qom/object.h"
 
 #define ACPI_POWER_BUTTON_DEVICE "PWRB"
@@ -101,6 +102,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(AcpiGedState, ACPI_GED)
 #define ACPI_GED_PWR_DOWN_EVT      0x2
 #define ACPI_GED_NVDIMM_HOTPLUG_EVT 0x4
 #define ACPI_GED_CPU_HOTPLUG_EVT    0x8
+#define ACPI_GED_PCI_HOTPLUG_EVT    0x10
 
 typedef struct GEDState {
     MemoryRegion evt;
@@ -114,6 +116,9 @@ struct AcpiGedState {
     MemoryRegion container_memhp;
     CPUHotplugState cpuhp_state;
     MemoryRegion container_cpuhp;
+    AcpiPciHpState pcihp_state;
+    MemoryRegion container_pcihp;
+
     GEDState ged_state;
     uint32_t ged_event_bitmap;
     qemu_irq irq;
diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
index 451ef74284..abb6e14549 100644
--- a/include/hw/acpi/pcihp.h
+++ b/include/hw/acpi/pcihp.h
@@ -38,6 +38,8 @@
 #define ACPI_PCIHP_SEJ_BASE 0x8
 #define ACPI_PCIHP_BNMR_BASE 0x10
 
+#define ACPI_PCI_HOTPLUG_REG_LEN 0x14
+
 typedef struct AcpiPciHpPciStatus {
     uint32_t up;
     uint32_t down;
diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
index 7b2d582fff..0fd8baba97 100644
--- a/hw/acpi/generic_event_device.c
+++ b/hw/acpi/generic_event_device.c
@@ -26,6 +26,7 @@ static const uint32_t ged_supported_events[] = {
     ACPI_GED_PWR_DOWN_EVT,
     ACPI_GED_NVDIMM_HOTPLUG_EVT,
     ACPI_GED_CPU_HOTPLUG_EVT,
+    ACPI_GED_PCI_HOTPLUG_EVT,
 };
 
 /*
@@ -121,6 +122,12 @@ void build_ged_aml(Aml *table, const char *name, HotplugHandler *hotplug_dev,
                            aml_notify(aml_name("\\_SB.NVDR"),
                                       aml_int(0x80)));
                 break;
+            case ACPI_GED_PCI_HOTPLUG_EVT:
+                aml_append(if_ctx,
+                           aml_acquire(aml_name("\\_SB.PCI0.BLCK"), 0xFFFF));
+                aml_append(if_ctx, aml_call0("\\_SB.PCI0.PCNT"));
+                aml_append(if_ctx, aml_release(aml_name("\\_SB.PCI0.BLCK")));
+                break;
             default:
                 /*
                  * Please make sure all the events in ged_supported_events[]
@@ -299,6 +306,8 @@ static void acpi_ged_send_event(AcpiDeviceIf *adev, AcpiEventStatusBits ev)
         sel = ACPI_GED_NVDIMM_HOTPLUG_EVT;
     } else if (ev & ACPI_CPU_HOTPLUG_STATUS) {
         sel = ACPI_GED_CPU_HOTPLUG_EVT;
+    } else if (ev & ACPI_PCI_HOTPLUG_STATUS) {
+        sel = ACPI_GED_PCI_HOTPLUG_EVT;
     } else {
         /* Unknown event. Return without generating interrupt. */
         warn_report("GED: Unsupported event %d. No irq injected", ev);
@@ -428,6 +437,11 @@ static void acpi_ged_realize(DeviceState *dev, Error **errp)
             cpu_hotplug_hw_init(&s->container_cpuhp, OBJECT(dev),
                                 &s->cpuhp_state, 0);
             break;
+        case ACPI_GED_PCI_HOTPLUG_EVT:
+            memory_region_init(&s->container_pcihp, OBJECT(dev),
+                                "pcihp container",
+                                ACPI_PCI_HOTPLUG_REG_LEN);
+            sysbus_init_mmio(sbd, &s->container_pcihp);
         }
         ged_events--;
     }
-- 
2.49.0
Re: [RFC 18/24] hw/acpi/ged: Prepare the device to react to PCI hotplug events
Posted by Gustavo Romero 9 months ago 
Hi Eric,

On 4/28/25 07:25, Eric Auger wrote:
> QEMU will notify the OS about PCI hotplug/hotunplug events through
> GED interrupts. Let the GED device handle a new PCI hotplug event.
> On its occurence it calls the \\_SB.PCI0.PCNT method with the BLCK

nit: occurrence -^


> mutex held.
> 
> The GED device uses a dedicated MMIO region that will be mapped
> by the machine code.
> 
> At this point the GED still does not support PCI device hotplug in
> its TYPE_HOTPLUG_HANDLER implementation. This will come in a
> subsequent patch

nit: period missing ^


> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>   include/hw/acpi/generic_event_device.h |  5 +++++
>   include/hw/acpi/pcihp.h                |  2 ++
>   hw/acpi/generic_event_device.c         | 14 ++++++++++++++
>   3 files changed, 21 insertions(+)
> 
> diff --git a/include/hw/acpi/generic_event_device.h b/include/hw/acpi/generic_event_device.h
> index d2dac87b4a..28be6c0582 100644
> --- a/include/hw/acpi/generic_event_device.h
> +++ b/include/hw/acpi/generic_event_device.h
> @@ -63,6 +63,7 @@
>   #include "hw/acpi/memory_hotplug.h"
>   #include "hw/acpi/ghes.h"
>   #include "hw/acpi/cpu.h"
> +#include "hw/acpi/pcihp.h"
>   #include "qom/object.h"
>   
>   #define ACPI_POWER_BUTTON_DEVICE "PWRB"
> @@ -101,6 +102,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(AcpiGedState, ACPI_GED)
>   #define ACPI_GED_PWR_DOWN_EVT      0x2
>   #define ACPI_GED_NVDIMM_HOTPLUG_EVT 0x4
>   #define ACPI_GED_CPU_HOTPLUG_EVT    0x8
> +#define ACPI_GED_PCI_HOTPLUG_EVT    0x10
>   
>   typedef struct GEDState {
>       MemoryRegion evt;
> @@ -114,6 +116,9 @@ struct AcpiGedState {
>       MemoryRegion container_memhp;
>       CPUHotplugState cpuhp_state;
>       MemoryRegion container_cpuhp;
> +    AcpiPciHpState pcihp_state;
> +    MemoryRegion container_pcihp;
> +
>       GEDState ged_state;
>       uint32_t ged_event_bitmap;
>       qemu_irq irq;
> diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
> index 451ef74284..abb6e14549 100644
> --- a/include/hw/acpi/pcihp.h
> +++ b/include/hw/acpi/pcihp.h
> @@ -38,6 +38,8 @@
>   #define ACPI_PCIHP_SEJ_BASE 0x8
>   #define ACPI_PCIHP_BNMR_BASE 0x10
>   
> +#define ACPI_PCI_HOTPLUG_REG_LEN 0x14

This is wrong, no? It should be 0x18, i.e. 24 bytes or 6 registers of 4 bytes.

I think it's better to move the defines in pcihp.c into pcihp.h and
use them to define ACPI_PCI_HOTPLUG_REG_LEN so the values are consistent, like:

diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
index 82b8ea2811..3ab8b9de94 100644
--- a/hw/acpi/pcihp.c
+++ b/hw/acpi/pcihp.c
@@ -44,14 +44,6 @@
  #include "qobject/qnum.h"
  #include "trace.h"
  
-#define ACPI_PCIHP_SIZE 0x0018
-#define PCI_UP_BASE 0x0000
-#define PCI_DOWN_BASE 0x0004
-#define PCI_EJ_BASE 0x0008
-#define PCI_RMV_BASE 0x000c
-#define PCI_SEL_BASE 0x0010
-#define PCI_AIDX_BASE 0x0014
-
  typedef struct AcpiPciHpFind {
      int bsel;
      PCIBus *bus;
diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
index abb6e14549..6b79adcaef 100644
--- a/include/hw/acpi/pcihp.h
+++ b/include/hw/acpi/pcihp.h
@@ -38,7 +38,15 @@
  #define ACPI_PCIHP_SEJ_BASE 0x8
  #define ACPI_PCIHP_BNMR_BASE 0x10
  
-#define ACPI_PCI_HOTPLUG_REG_LEN 0x14
+#define ACPI_PCIHP_SIZE 0x0018
+#define PCI_UP_BASE 0x0000
+#define PCI_DOWN_BASE 0x0004
+#define PCI_EJ_BASE 0x0008
+#define PCI_RMV_BASE 0x000c
+#define PCI_SEL_BASE 0x0010
+#define PCI_AIDX_BASE 0x0014
+
+#define ACPI_PCI_HOTPLUG_REG_LEN ACPI_PCIHP_SIZE
  
  typedef struct AcpiPciHpPciStatus {
      uint32_t up;


or even use only one of them (ACPI_PCI_HOTPLUG_REG_LEN, ACPI_PCIHP_SIZE, or
another new name that would fit better in both contexts).


Cheers,
Gustavo

> +
>   typedef struct AcpiPciHpPciStatus {
>       uint32_t up;
>       uint32_t down;
> diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
> index 7b2d582fff..0fd8baba97 100644
> --- a/hw/acpi/generic_event_device.c
> +++ b/hw/acpi/generic_event_device.c
> @@ -26,6 +26,7 @@ static const uint32_t ged_supported_events[] = {
>       ACPI_GED_PWR_DOWN_EVT,
>       ACPI_GED_NVDIMM_HOTPLUG_EVT,
>       ACPI_GED_CPU_HOTPLUG_EVT,
> +    ACPI_GED_PCI_HOTPLUG_EVT,
>   };
>   
>   /*
> @@ -121,6 +122,12 @@ void build_ged_aml(Aml *table, const char *name, HotplugHandler *hotplug_dev,
>                              aml_notify(aml_name("\\_SB.NVDR"),
>                                         aml_int(0x80)));
>                   break;
> +            case ACPI_GED_PCI_HOTPLUG_EVT:
> +                aml_append(if_ctx,
> +                           aml_acquire(aml_name("\\_SB.PCI0.BLCK"), 0xFFFF));
> +                aml_append(if_ctx, aml_call0("\\_SB.PCI0.PCNT"));
> +                aml_append(if_ctx, aml_release(aml_name("\\_SB.PCI0.BLCK")));
> +                break;
>               default:
>                   /*
>                    * Please make sure all the events in ged_supported_events[]
> @@ -299,6 +306,8 @@ static void acpi_ged_send_event(AcpiDeviceIf *adev, AcpiEventStatusBits ev)
>           sel = ACPI_GED_NVDIMM_HOTPLUG_EVT;
>       } else if (ev & ACPI_CPU_HOTPLUG_STATUS) {
>           sel = ACPI_GED_CPU_HOTPLUG_EVT;
> +    } else if (ev & ACPI_PCI_HOTPLUG_STATUS) {
> +        sel = ACPI_GED_PCI_HOTPLUG_EVT;
>       } else {
>           /* Unknown event. Return without generating interrupt. */
>           warn_report("GED: Unsupported event %d. No irq injected", ev);
> @@ -428,6 +437,11 @@ static void acpi_ged_realize(DeviceState *dev, Error **errp)
>               cpu_hotplug_hw_init(&s->container_cpuhp, OBJECT(dev),
>                                   &s->cpuhp_state, 0);
>               break;
> +        case ACPI_GED_PCI_HOTPLUG_EVT:
> +            memory_region_init(&s->container_pcihp, OBJECT(dev),
> +                                "pcihp container",
> +                                ACPI_PCI_HOTPLUG_REG_LEN);
> +            sysbus_init_mmio(sbd, &s->container_pcihp);
>           }
>           ged_events--;
>       }
Re: [RFC 18/24] hw/acpi/ged: Prepare the device to react to PCI hotplug events
Posted by Eric Auger 9 months ago 
Hi Gustavo,

On 5/14/25 6:20 PM, Gustavo Romero wrote:
> Hi Eric,
>
> On 4/28/25 07:25, Eric Auger wrote:
>> QEMU will notify the OS about PCI hotplug/hotunplug events through
>> GED interrupts. Let the GED device handle a new PCI hotplug event.
>> On its occurence it calls the \\_SB.PCI0.PCNT method with the BLCK
>
> nit: occurrence -^
>
>
>> mutex held.
>>
>> The GED device uses a dedicated MMIO region that will be mapped
>> by the machine code.
>>
>> At this point the GED still does not support PCI device hotplug in
>> its TYPE_HOTPLUG_HANDLER implementation. This will come in a
>> subsequent patch
>
> nit: period missing ^
>
>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>   include/hw/acpi/generic_event_device.h |  5 +++++
>>   include/hw/acpi/pcihp.h                |  2 ++
>>   hw/acpi/generic_event_device.c         | 14 ++++++++++++++
>>   3 files changed, 21 insertions(+)
>>
>> diff --git a/include/hw/acpi/generic_event_device.h
>> b/include/hw/acpi/generic_event_device.h
>> index d2dac87b4a..28be6c0582 100644
>> --- a/include/hw/acpi/generic_event_device.h
>> +++ b/include/hw/acpi/generic_event_device.h
>> @@ -63,6 +63,7 @@
>>   #include "hw/acpi/memory_hotplug.h"
>>   #include "hw/acpi/ghes.h"
>>   #include "hw/acpi/cpu.h"
>> +#include "hw/acpi/pcihp.h"
>>   #include "qom/object.h"
>>     #define ACPI_POWER_BUTTON_DEVICE "PWRB"
>> @@ -101,6 +102,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(AcpiGedState, ACPI_GED)
>>   #define ACPI_GED_PWR_DOWN_EVT      0x2
>>   #define ACPI_GED_NVDIMM_HOTPLUG_EVT 0x4
>>   #define ACPI_GED_CPU_HOTPLUG_EVT    0x8
>> +#define ACPI_GED_PCI_HOTPLUG_EVT    0x10
>>     typedef struct GEDState {
>>       MemoryRegion evt;
>> @@ -114,6 +116,9 @@ struct AcpiGedState {
>>       MemoryRegion container_memhp;
>>       CPUHotplugState cpuhp_state;
>>       MemoryRegion container_cpuhp;
>> +    AcpiPciHpState pcihp_state;
>> +    MemoryRegion container_pcihp;
>> +
>>       GEDState ged_state;
>>       uint32_t ged_event_bitmap;
>>       qemu_irq irq;
>> diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
>> index 451ef74284..abb6e14549 100644
>> --- a/include/hw/acpi/pcihp.h
>> +++ b/include/hw/acpi/pcihp.h
>> @@ -38,6 +38,8 @@
>>   #define ACPI_PCIHP_SEJ_BASE 0x8
>>   #define ACPI_PCIHP_BNMR_BASE 0x10
>>   +#define ACPI_PCI_HOTPLUG_REG_LEN 0x14
>
> This is wrong, no? It should be 0x18, i.e. 24 bytes or 6 registers of
> 4 bytes.
yes this is wrong. you are totally right. I am about to send a new
version where this is fixed, sorry. I noticed that when testing the acpi
index modality which did not work with the RFC series.

Now I am using ACPI_PCIHP_SIZE.

Cheers

Eric
>
> I think it's better to move the defines in pcihp.c into pcihp.h and
> use them to define ACPI_PCI_HOTPLUG_REG_LEN so the values are
> consistent, like:
>
> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> index 82b8ea2811..3ab8b9de94 100644
> --- a/hw/acpi/pcihp.c
> +++ b/hw/acpi/pcihp.c
> @@ -44,14 +44,6 @@
>  #include "qobject/qnum.h"
>  #include "trace.h"
>  
> -#define ACPI_PCIHP_SIZE 0x0018
> -#define PCI_UP_BASE 0x0000
> -#define PCI_DOWN_BASE 0x0004
> -#define PCI_EJ_BASE 0x0008
> -#define PCI_RMV_BASE 0x000c
> -#define PCI_SEL_BASE 0x0010
> -#define PCI_AIDX_BASE 0x0014
> -
>  typedef struct AcpiPciHpFind {
>      int bsel;
>      PCIBus *bus;
> diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
> index abb6e14549..6b79adcaef 100644
> --- a/include/hw/acpi/pcihp.h
> +++ b/include/hw/acpi/pcihp.h
> @@ -38,7 +38,15 @@
>  #define ACPI_PCIHP_SEJ_BASE 0x8
>  #define ACPI_PCIHP_BNMR_BASE 0x10
>  
> -#define ACPI_PCI_HOTPLUG_REG_LEN 0x14
> +#define ACPI_PCIHP_SIZE 0x0018
> +#define PCI_UP_BASE 0x0000
> +#define PCI_DOWN_BASE 0x0004
> +#define PCI_EJ_BASE 0x0008
> +#define PCI_RMV_BASE 0x000c
> +#define PCI_SEL_BASE 0x0010
> +#define PCI_AIDX_BASE 0x0014
> +
> +#define ACPI_PCI_HOTPLUG_REG_LEN ACPI_PCIHP_SIZE
>  
>  typedef struct AcpiPciHpPciStatus {
>      uint32_t up;
>
>
> or even use only one of them (ACPI_PCI_HOTPLUG_REG_LEN,
> ACPI_PCIHP_SIZE, or
> another new name that would fit better in both contexts).
>
>
> Cheers,
> Gustavo
>
>> +
>>   typedef struct AcpiPciHpPciStatus {
>>       uint32_t up;
>>       uint32_t down;
>> diff --git a/hw/acpi/generic_event_device.c
>> b/hw/acpi/generic_event_device.c
>> index 7b2d582fff..0fd8baba97 100644
>> --- a/hw/acpi/generic_event_device.c
>> +++ b/hw/acpi/generic_event_device.c
>> @@ -26,6 +26,7 @@ static const uint32_t ged_supported_events[] = {
>>       ACPI_GED_PWR_DOWN_EVT,
>>       ACPI_GED_NVDIMM_HOTPLUG_EVT,
>>       ACPI_GED_CPU_HOTPLUG_EVT,
>> +    ACPI_GED_PCI_HOTPLUG_EVT,
>>   };
>>     /*
>> @@ -121,6 +122,12 @@ void build_ged_aml(Aml *table, const char *name,
>> HotplugHandler *hotplug_dev,
>>                              aml_notify(aml_name("\\_SB.NVDR"),
>>                                         aml_int(0x80)));
>>                   break;
>> +            case ACPI_GED_PCI_HOTPLUG_EVT:
>> +                aml_append(if_ctx,
>> +                           aml_acquire(aml_name("\\_SB.PCI0.BLCK"),
>> 0xFFFF));
>> +                aml_append(if_ctx, aml_call0("\\_SB.PCI0.PCNT"));
>> +                aml_append(if_ctx,
>> aml_release(aml_name("\\_SB.PCI0.BLCK")));
>> +                break;
>>               default:
>>                   /*
>>                    * Please make sure all the events in
>> ged_supported_events[]
>> @@ -299,6 +306,8 @@ static void acpi_ged_send_event(AcpiDeviceIf
>> *adev, AcpiEventStatusBits ev)
>>           sel = ACPI_GED_NVDIMM_HOTPLUG_EVT;
>>       } else if (ev & ACPI_CPU_HOTPLUG_STATUS) {
>>           sel = ACPI_GED_CPU_HOTPLUG_EVT;
>> +    } else if (ev & ACPI_PCI_HOTPLUG_STATUS) {
>> +        sel = ACPI_GED_PCI_HOTPLUG_EVT;
>>       } else {
>>           /* Unknown event. Return without generating interrupt. */
>>           warn_report("GED: Unsupported event %d. No irq injected", ev);
>> @@ -428,6 +437,11 @@ static void acpi_ged_realize(DeviceState *dev,
>> Error **errp)
>>               cpu_hotplug_hw_init(&s->container_cpuhp, OBJECT(dev),
>>                                   &s->cpuhp_state, 0);
>>               break;
>> +        case ACPI_GED_PCI_HOTPLUG_EVT:
>> +            memory_region_init(&s->container_pcihp, OBJECT(dev),
>> +                                "pcihp container",
>> +                                ACPI_PCI_HOTPLUG_REG_LEN);
>> +            sysbus_init_mmio(sbd, &s->container_pcihp);
>>           }
>>           ged_events--;
>>       }
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.