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.au...@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--;
>> }
>
