Set up the IO registers used to communicate between QEMU
and ACPI.
Move the create_pcie() call after the creation of the acpi
ged device since hotplug callbacks will soon be called on gpex
realize and will require the acpi pcihp state to be initialized.
The hacky thing is the root bus has not yet been created on
acpi_pcihp_init() call so it is set later after the gpex realize.
How to fix this chicken & egg issue?
Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
v1 -> v2:
- use ACPI_PCIHP_REGION_NAME
---
include/hw/arm/virt.h | 1 +
hw/arm/virt-acpi-build.c | 1 +
hw/arm/virt.c | 42 +++++++++++++++++++++++++++++++++++-----
3 files changed, 39 insertions(+), 5 deletions(-)
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index 1b2e2e1284..a4c4e3a67a 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -35,6 +35,7 @@
#include "hw/boards.h"
#include "hw/arm/boot.h"
#include "hw/arm/bsa.h"
+#include "hw/acpi/pcihp.h"
#include "hw/block/flash.h"
#include "system/kvm.h"
#include "hw/intc/arm_gicv3_common.h"
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 9d88ffc318..cd49f67d60 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -44,6 +44,7 @@
#include "hw/acpi/generic_event_device.h"
#include "hw/acpi/tpm.h"
#include "hw/acpi/hmat.h"
+#include "hw/acpi/pcihp.h"
#include "hw/pci/pcie_host.h"
#include "hw/pci/pci.h"
#include "hw/pci/pci_bus.h"
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 4aa40c8e8b..cdcff0a984 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -682,6 +682,8 @@ static inline DeviceState *create_acpi_ged(VirtMachineState *vms)
{
DeviceState *dev;
MachineState *ms = MACHINE(vms);
+ SysBusDevice *sbdev;
+
int irq = vms->irqmap[VIRT_ACPI_GED];
uint32_t event = ACPI_GED_PWR_DOWN_EVT;
@@ -693,12 +695,28 @@ static inline DeviceState *create_acpi_ged(VirtMachineState *vms)
event |= ACPI_GED_NVDIMM_HOTPLUG_EVT;
}
+ if (vms->acpi_pcihp) {
+ event |= ACPI_GED_PCI_HOTPLUG_EVT;
+ }
+
dev = qdev_new(TYPE_ACPI_GED);
qdev_prop_set_uint32(dev, "ged-event", event);
- sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+ sbdev = SYS_BUS_DEVICE(dev);
+ sysbus_realize_and_unref(sbdev, &error_fatal);
- sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, vms->memmap[VIRT_ACPI_GED].base);
- sysbus_mmio_map(SYS_BUS_DEVICE(dev), 1, vms->memmap[VIRT_PCDIMM_ACPI].base);
+ sysbus_mmio_map(sbdev, 0, vms->memmap[VIRT_ACPI_GED].base);
+ sysbus_mmio_map(sbdev, 1, vms->memmap[VIRT_PCDIMM_ACPI].base);
+ if (vms->acpi_pcihp) {
+ AcpiGedState *acpi_ged_state = ACPI_GED(dev);
+ int i;
+
+ i = sysbus_mmio_map_name(sbdev, ACPI_PCIHP_REGION_NAME,
+ vms->memmap[VIRT_ACPI_PCIHP].base);
+ assert(i >= 0);
+ acpi_pcihp_init(OBJECT(dev), &acpi_ged_state->pcihp_state,
+ vms->bus, sysbus_mmio_get_region(sbdev, i), 0);
+ acpi_ged_state->pcihp_state.use_acpi_hotplug_bridge = true;
+ }
sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, qdev_get_gpio_in(vms->gic, irq));
return dev;
@@ -1758,6 +1776,13 @@ void virt_machine_done(Notifier *notifier, void *data)
pci_bus_add_fw_cfg_extra_pci_roots(vms->fw_cfg, vms->bus,
&error_abort);
+
+ if (vms->acpi_pcihp) {
+ AcpiGedState *acpi_ged_state = ACPI_GED(vms->acpi_dev);
+
+ acpi_pcihp_reset(&acpi_ged_state->pcihp_state);
+ }
+
virt_acpi_setup(vms);
virt_build_smbios(vms);
}
@@ -2395,8 +2420,6 @@ static void machvirt_init(MachineState *machine)
create_rtc(vms);
- create_pcie(vms);
-
if (has_ged && aarch64 && firmware_loaded && virt_is_acpi_enabled(vms)) {
vms->acpi_pcihp &= !vmc->no_acpi_pcihp;
vms->acpi_dev = create_acpi_ged(vms);
@@ -2405,6 +2428,15 @@ static void machvirt_init(MachineState *machine)
create_gpio_devices(vms, VIRT_GPIO, sysmem);
}
+ create_pcie(vms);
+
+ if (vms->acpi_dev) {
+ AcpiGedState *acpi_ged_state = ACPI_GED(vms->acpi_dev);
+
+ acpi_ged_state = ACPI_GED(vms->acpi_dev);
+ acpi_ged_state->pcihp_state.root = vms->bus;
+ }
+
if (vms->secure && !vmc->no_secure_gpio) {
create_gpio_devices(vms, VIRT_SECURE_GPIO, secure_sysmem);
}
--
2.49.0On Tue, 27 May 2025 09:40:24 +0200
Eric Auger <eric.auger@redhat.com> wrote:
> Set up the IO registers used to communicate between QEMU
> and ACPI.
> Move the create_pcie() call after the creation of the acpi
> ged device since hotplug callbacks will soon be called on gpex
> realize and will require the acpi pcihp state to be initialized.
>
> The hacky thing is the root bus has not yet been created on
> acpi_pcihp_init() call so it is set later after the gpex realize.
can you elaborate on this, preferably with call expected call flows?
> How to fix this chicken & egg issue?
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>
> ---
>
> v1 -> v2:
> - use ACPI_PCIHP_REGION_NAME
> ---
> include/hw/arm/virt.h | 1 +
> hw/arm/virt-acpi-build.c | 1 +
> hw/arm/virt.c | 42 +++++++++++++++++++++++++++++++++++-----
> 3 files changed, 39 insertions(+), 5 deletions(-)
>
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index 1b2e2e1284..a4c4e3a67a 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -35,6 +35,7 @@
> #include "hw/boards.h"
> #include "hw/arm/boot.h"
> #include "hw/arm/bsa.h"
> +#include "hw/acpi/pcihp.h"
> #include "hw/block/flash.h"
> #include "system/kvm.h"
> #include "hw/intc/arm_gicv3_common.h"
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 9d88ffc318..cd49f67d60 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -44,6 +44,7 @@
> #include "hw/acpi/generic_event_device.h"
> #include "hw/acpi/tpm.h"
> #include "hw/acpi/hmat.h"
> +#include "hw/acpi/pcihp.h"
> #include "hw/pci/pcie_host.h"
> #include "hw/pci/pci.h"
> #include "hw/pci/pci_bus.h"
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 4aa40c8e8b..cdcff0a984 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -682,6 +682,8 @@ static inline DeviceState *create_acpi_ged(VirtMachineState *vms)
> {
> DeviceState *dev;
> MachineState *ms = MACHINE(vms);
> + SysBusDevice *sbdev;
> +
> int irq = vms->irqmap[VIRT_ACPI_GED];
> uint32_t event = ACPI_GED_PWR_DOWN_EVT;
>
> @@ -693,12 +695,28 @@ static inline DeviceState *create_acpi_ged(VirtMachineState *vms)
> event |= ACPI_GED_NVDIMM_HOTPLUG_EVT;
> }
>
> + if (vms->acpi_pcihp) {
> + event |= ACPI_GED_PCI_HOTPLUG_EVT;
> + }
> +
> dev = qdev_new(TYPE_ACPI_GED);
> qdev_prop_set_uint32(dev, "ged-event", event);
> - sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> + sbdev = SYS_BUS_DEVICE(dev);
> + sysbus_realize_and_unref(sbdev, &error_fatal);
>
> - sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, vms->memmap[VIRT_ACPI_GED].base);
> - sysbus_mmio_map(SYS_BUS_DEVICE(dev), 1, vms->memmap[VIRT_PCDIMM_ACPI].base);
> + sysbus_mmio_map(sbdev, 0, vms->memmap[VIRT_ACPI_GED].base);
> + sysbus_mmio_map(sbdev, 1, vms->memmap[VIRT_PCDIMM_ACPI].base);
Perhaps move out sbdev renaming into a separate patch, as it's not really related.
> + if (vms->acpi_pcihp) {
> + AcpiGedState *acpi_ged_state = ACPI_GED(dev);
> + int i;
> +
> + i = sysbus_mmio_map_name(sbdev, ACPI_PCIHP_REGION_NAME,
> + vms->memmap[VIRT_ACPI_PCIHP].base);
I don't like mix of old way (index based) above and new name based mapping,
can we use the same, please?
> + assert(i >= 0);
g_assert(sysbus_mmio_map_name...) to get more meaning-full crash
that is not compiled out.
> + acpi_pcihp_init(OBJECT(dev), &acpi_ged_state->pcihp_state,
> + vms->bus, sysbus_mmio_get_region(sbdev, i), 0);
hmm, looks broken..
region mapping must happen after acpi_pcihp_init().
if we after making sysbus_mmio_map() sane and easier to use
(which is a bit on tangent to this series).
We could feed sysbus owner device a memory map (ex: name based),
and then use [pre_]plug handlers on sysbus to map children
automatically.
That will alleviate need to do all mapping manually in every board.
(frankly speaking it deserves its own series, with tree wide cleanup).
As it is I'd use old index based approach like the rest.
(unless you feel adventurous about sysbus refactoring)
> + acpi_ged_state->pcihp_state.use_acpi_hotplug_bridge = true;
> + }
> sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, qdev_get_gpio_in(vms->gic, irq));
>
> return dev;
> @@ -1758,6 +1776,13 @@ void virt_machine_done(Notifier *notifier, void *data)
> pci_bus_add_fw_cfg_extra_pci_roots(vms->fw_cfg, vms->bus,
> &error_abort);
> +
> + if (vms->acpi_pcihp) {
> + AcpiGedState *acpi_ged_state = ACPI_GED(vms->acpi_dev);
> +
> + acpi_pcihp_reset(&acpi_ged_state->pcihp_state);
> + }
> +
> virt_acpi_setup(vms);
> virt_build_smbios(vms);
> }
> @@ -2395,8 +2420,6 @@ static void machvirt_init(MachineState *machine)
>
> create_rtc(vms);
>
> - create_pcie(vms);
> -
> if (has_ged && aarch64 && firmware_loaded && virt_is_acpi_enabled(vms)) {
> vms->acpi_pcihp &= !vmc->no_acpi_pcihp;
> vms->acpi_dev = create_acpi_ged(vms);
> @@ -2405,6 +2428,15 @@ static void machvirt_init(MachineState *machine)
> create_gpio_devices(vms, VIRT_GPIO, sysmem);
> }
>
> + create_pcie(vms);
> +
> + if (vms->acpi_dev) {
> + AcpiGedState *acpi_ged_state = ACPI_GED(vms->acpi_dev);
> +
> + acpi_ged_state = ACPI_GED(vms->acpi_dev);
> + acpi_ged_state->pcihp_state.root = vms->bus;
> + }
> +
> if (vms->secure && !vmc->no_secure_gpio) {
> create_gpio_devices(vms, VIRT_SECURE_GPIO, secure_sysmem);
> }
I don't like pulling acpi_pcihp_init()/reset (and issues it causes) into board code,
on x86 it's a part of host bridge device model.
The same should apply to GED device.
The only thing board has to do is map regions into IO space like we do
everywhere else.
with current code, may be add link<pci_bus> property to GED,
and set it before GED realize in create_acpi_ged(),
then just follow existing sysbus_mmio_map() pattern.
Hi folks,
On 5/27/25 12:56, Igor Mammedov wrote:
> On Tue, 27 May 2025 09:40:24 +0200
> Eric Auger <eric.auger@redhat.com> wrote:
>
>> Set up the IO registers used to communicate between QEMU
>> and ACPI.
>
>
>
>> Move the create_pcie() call after the creation of the acpi
>> ged device since hotplug callbacks will soon be called on gpex
>> realize and will require the acpi pcihp state to be initialized.
>>
>> The hacky thing is the root bus has not yet been created on
>> acpi_pcihp_init() call so it is set later after the gpex realize.
>
> can you elaborate on this, preferably with call expected call flows?
The way I'm understanding it is: because the GED and GPEX are created
in virt.c, so at the machine "level" (and so a machine hotplug handler is
used instead of a device hotplug handler, like it's done in PIIX4 and ICH9
controller in the x86 machines), and they need to be created in that
order because of dependecy of GPEX of GED, the flow is:
1) create_acpi_ged() -> calls acpi_pcihp_init(), which by its turn will inspect the root bus to generate the ACPI code
2) create_pcie() -> just in realize the root bus will be created
and the sequence has to be 1 -> 2, because GED will be used in the GPEX callbacks on hotplug.
So here we're following a different design from the current one in x86,
which handles the hotplug at the PCI controller "level".
Cheers,
Gustavo
>> How to fix this chicken & egg issue?
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>
>> ---
>>
>> v1 -> v2:
>> - use ACPI_PCIHP_REGION_NAME
>> ---
>> include/hw/arm/virt.h | 1 +
>> hw/arm/virt-acpi-build.c | 1 +
>> hw/arm/virt.c | 42 +++++++++++++++++++++++++++++++++++-----
>> 3 files changed, 39 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
>> index 1b2e2e1284..a4c4e3a67a 100644
>> --- a/include/hw/arm/virt.h
>> +++ b/include/hw/arm/virt.h
>> @@ -35,6 +35,7 @@
>> #include "hw/boards.h"
>> #include "hw/arm/boot.h"
>> #include "hw/arm/bsa.h"
>> +#include "hw/acpi/pcihp.h"
>> #include "hw/block/flash.h"
>> #include "system/kvm.h"
>> #include "hw/intc/arm_gicv3_common.h"
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index 9d88ffc318..cd49f67d60 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -44,6 +44,7 @@
>> #include "hw/acpi/generic_event_device.h"
>> #include "hw/acpi/tpm.h"
>> #include "hw/acpi/hmat.h"
>> +#include "hw/acpi/pcihp.h"
>> #include "hw/pci/pcie_host.h"
>> #include "hw/pci/pci.h"
>> #include "hw/pci/pci_bus.h"
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index 4aa40c8e8b..cdcff0a984 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -682,6 +682,8 @@ static inline DeviceState *create_acpi_ged(VirtMachineState *vms)
>> {
>> DeviceState *dev;
>> MachineState *ms = MACHINE(vms);
>> + SysBusDevice *sbdev;
>> +
>> int irq = vms->irqmap[VIRT_ACPI_GED];
>> uint32_t event = ACPI_GED_PWR_DOWN_EVT;
>>
>> @@ -693,12 +695,28 @@ static inline DeviceState *create_acpi_ged(VirtMachineState *vms)
>> event |= ACPI_GED_NVDIMM_HOTPLUG_EVT;
>> }
>>
>> + if (vms->acpi_pcihp) {
>> + event |= ACPI_GED_PCI_HOTPLUG_EVT;
>> + }
>> +
>> dev = qdev_new(TYPE_ACPI_GED);
>> qdev_prop_set_uint32(dev, "ged-event", event);
>> - sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>> + sbdev = SYS_BUS_DEVICE(dev);
>> + sysbus_realize_and_unref(sbdev, &error_fatal);
>>
>> - sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, vms->memmap[VIRT_ACPI_GED].base);
>> - sysbus_mmio_map(SYS_BUS_DEVICE(dev), 1, vms->memmap[VIRT_PCDIMM_ACPI].base);
>> + sysbus_mmio_map(sbdev, 0, vms->memmap[VIRT_ACPI_GED].base);
>> + sysbus_mmio_map(sbdev, 1, vms->memmap[VIRT_PCDIMM_ACPI].base);
>
>
> Perhaps move out sbdev renaming into a separate patch, as it's not really related.
>
>> + if (vms->acpi_pcihp) {
>> + AcpiGedState *acpi_ged_state = ACPI_GED(dev);
>> + int i;
>> +
>> + i = sysbus_mmio_map_name(sbdev, ACPI_PCIHP_REGION_NAME,
>> + vms->memmap[VIRT_ACPI_PCIHP].base);
>
> I don't like mix of old way (index based) above and new name based mapping,
> can we use the same, please?
>
>> + assert(i >= 0);
> g_assert(sysbus_mmio_map_name...) to get more meaning-full crash
> that is not compiled out.
>
>> + acpi_pcihp_init(OBJECT(dev), &acpi_ged_state->pcihp_state,
>> + vms->bus, sysbus_mmio_get_region(sbdev, i), 0);
>
> hmm, looks broken..
> region mapping must happen after acpi_pcihp_init().
>
> if we after making sysbus_mmio_map() sane and easier to use
> (which is a bit on tangent to this series).
> We could feed sysbus owner device a memory map (ex: name based),
> and then use [pre_]plug handlers on sysbus to map children
> automatically.
> That will alleviate need to do all mapping manually in every board.
> (frankly speaking it deserves its own series, with tree wide cleanup).
>
> As it is I'd use old index based approach like the rest.
> (unless you feel adventurous about sysbus refactoring)
>
>
>> + acpi_ged_state->pcihp_state.use_acpi_hotplug_bridge = true;
>> + }
>> sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, qdev_get_gpio_in(vms->gic, irq));
>>
>> return dev;
>> @@ -1758,6 +1776,13 @@ void virt_machine_done(Notifier *notifier, void *data)
>> pci_bus_add_fw_cfg_extra_pci_roots(vms->fw_cfg, vms->bus,
>> &error_abort);
>
>
>> +
>> + if (vms->acpi_pcihp) {
>> + AcpiGedState *acpi_ged_state = ACPI_GED(vms->acpi_dev);
>> +
>> + acpi_pcihp_reset(&acpi_ged_state->pcihp_state);
>> + }
>> +
>> virt_acpi_setup(vms);
>> virt_build_smbios(vms);
>> }
>> @@ -2395,8 +2420,6 @@ static void machvirt_init(MachineState *machine)
>>
>> create_rtc(vms);
>>
>> - create_pcie(vms);
>> -
>> if (has_ged && aarch64 && firmware_loaded && virt_is_acpi_enabled(vms)) {
>> vms->acpi_pcihp &= !vmc->no_acpi_pcihp;
>> vms->acpi_dev = create_acpi_ged(vms);
>> @@ -2405,6 +2428,15 @@ static void machvirt_init(MachineState *machine)
>> create_gpio_devices(vms, VIRT_GPIO, sysmem);
>> }
>>
>> + create_pcie(vms);
>> +
>> + if (vms->acpi_dev) {
>> + AcpiGedState *acpi_ged_state = ACPI_GED(vms->acpi_dev);
>> +
>> + acpi_ged_state = ACPI_GED(vms->acpi_dev);
>> + acpi_ged_state->pcihp_state.root = vms->bus;
>> + }
>> +
>> if (vms->secure && !vmc->no_secure_gpio) {
>> create_gpio_devices(vms, VIRT_SECURE_GPIO, secure_sysmem);
>> }
>
> I don't like pulling acpi_pcihp_init()/reset (and issues it causes) into board code,
> on x86 it's a part of host bridge device model.
> The same should apply to GED device.
>
> The only thing board has to do is map regions into IO space like we do
> everywhere else.
>
> with current code, may be add link<pci_bus> property to GED,
> and set it before GED realize in create_acpi_ged(),
> then just follow existing sysbus_mmio_map() pattern.
>
On Tue, 27 May 2025 13:44:35 -0300
Gustavo Romero <gustavo.romero@linaro.org> wrote:
> Hi folks,
>
> On 5/27/25 12:56, Igor Mammedov wrote:
> > On Tue, 27 May 2025 09:40:24 +0200
> > Eric Auger <eric.auger@redhat.com> wrote:
> >
> >> Set up the IO registers used to communicate between QEMU
> >> and ACPI.
> >
> >
> >
> >> Move the create_pcie() call after the creation of the acpi
> >> ged device since hotplug callbacks will soon be called on gpex
> >> realize and will require the acpi pcihp state to be initialized.
> >>
> >> The hacky thing is the root bus has not yet been created on
> >> acpi_pcihp_init() call so it is set later after the gpex realize.
> >
> > can you elaborate on this, preferably with call expected call flows?
>
> The way I'm understanding it is: because the GED and GPEX are created
> in virt.c, so at the machine "level" (and so a machine hotplug handler is
> used instead of a device hotplug handler, like it's done in PIIX4 and ICH9
> controller in the x86 machines), and they need to be created in that
> order because of dependecy of GPEX of GED, the flow is:
>
> 1) create_acpi_ged() -> calls acpi_pcihp_init(), which by its turn will inspect the root bus to generate the ACPI code
> 2) create_pcie() -> just in realize the root bus will be created
>
> and the sequence has to be 1 -> 2, because GED will be used in the GPEX callbacks on hotplug.
>
> So here we're following a different design from the current one in x86,
> which handles the hotplug at the PCI controller "level".
It should be fine to keep x86 design instead of making up a new one
(unless we have to).
The difference between x86 and ARM is that acpi device happens to part of
host bridge or its children. While in ARM virt, its separate devices
(GPEX & GED).
From quick glance at the code, It should be fine to keep pcihp hw as part
of GED. All it needs from host-bridge is root bus, the rest is paravirt
ACPI magic wand.
Also we typically handle hotplug for devices that have parent bus
we are using bus hotplug handler.
Only bus-less or complicated cases are orchestrated by machine.
That's what is not this series, it probably has been overlooked
since it's not in pcihp.c, so kinda not obvious.
for example see:
ich9_pm_init()
qbus_set_hotplug_handler()
where default native hotplug handler is overridden with ACPI one,
and then later on, all coldplug bridges are picked up by
acpi_pcihp_device_plug_cb() and got the same treatment.
So I'd suggest to try keeping current order with few changes
create pci
create ged (bus)
acpidev = object_new(GED)
object_property_set_link(acpidev, 'pci_root_bus', bus)
realize(acpidev)
and then in
ged_realize(dev)
acpi_pcihp_init()
qbus_set_hotplug_handler(root_bus, dev)
that's likely would do the job.
>
> Cheers,
> Gustavo
>
> >> How to fix this chicken & egg issue?
> >>
> >> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> >>
> >> ---
> >>
> >> v1 -> v2:
> >> - use ACPI_PCIHP_REGION_NAME
> >> ---
> >> include/hw/arm/virt.h | 1 +
> >> hw/arm/virt-acpi-build.c | 1 +
> >> hw/arm/virt.c | 42 +++++++++++++++++++++++++++++++++++-----
> >> 3 files changed, 39 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> >> index 1b2e2e1284..a4c4e3a67a 100644
> >> --- a/include/hw/arm/virt.h
> >> +++ b/include/hw/arm/virt.h
> >> @@ -35,6 +35,7 @@
> >> #include "hw/boards.h"
> >> #include "hw/arm/boot.h"
> >> #include "hw/arm/bsa.h"
> >> +#include "hw/acpi/pcihp.h"
> >> #include "hw/block/flash.h"
> >> #include "system/kvm.h"
> >> #include "hw/intc/arm_gicv3_common.h"
> >> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> >> index 9d88ffc318..cd49f67d60 100644
> >> --- a/hw/arm/virt-acpi-build.c
> >> +++ b/hw/arm/virt-acpi-build.c
> >> @@ -44,6 +44,7 @@
> >> #include "hw/acpi/generic_event_device.h"
> >> #include "hw/acpi/tpm.h"
> >> #include "hw/acpi/hmat.h"
> >> +#include "hw/acpi/pcihp.h"
> >> #include "hw/pci/pcie_host.h"
> >> #include "hw/pci/pci.h"
> >> #include "hw/pci/pci_bus.h"
> >> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> >> index 4aa40c8e8b..cdcff0a984 100644
> >> --- a/hw/arm/virt.c
> >> +++ b/hw/arm/virt.c
> >> @@ -682,6 +682,8 @@ static inline DeviceState *create_acpi_ged(VirtMachineState *vms)
> >> {
> >> DeviceState *dev;
> >> MachineState *ms = MACHINE(vms);
> >> + SysBusDevice *sbdev;
> >> +
> >> int irq = vms->irqmap[VIRT_ACPI_GED];
> >> uint32_t event = ACPI_GED_PWR_DOWN_EVT;
> >>
> >> @@ -693,12 +695,28 @@ static inline DeviceState *create_acpi_ged(VirtMachineState *vms)
> >> event |= ACPI_GED_NVDIMM_HOTPLUG_EVT;
> >> }
> >>
> >> + if (vms->acpi_pcihp) {
> >> + event |= ACPI_GED_PCI_HOTPLUG_EVT;
> >> + }
> >> +
> >> dev = qdev_new(TYPE_ACPI_GED);
> >> qdev_prop_set_uint32(dev, "ged-event", event);
> >> - sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> >> + sbdev = SYS_BUS_DEVICE(dev);
> >> + sysbus_realize_and_unref(sbdev, &error_fatal);
> >>
> >> - sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, vms->memmap[VIRT_ACPI_GED].base);
> >> - sysbus_mmio_map(SYS_BUS_DEVICE(dev), 1, vms->memmap[VIRT_PCDIMM_ACPI].base);
> >> + sysbus_mmio_map(sbdev, 0, vms->memmap[VIRT_ACPI_GED].base);
> >> + sysbus_mmio_map(sbdev, 1, vms->memmap[VIRT_PCDIMM_ACPI].base);
> >
> >
> > Perhaps move out sbdev renaming into a separate patch, as it's not really related.
> >
> >> + if (vms->acpi_pcihp) {
> >> + AcpiGedState *acpi_ged_state = ACPI_GED(dev);
> >> + int i;
> >> +
> >> + i = sysbus_mmio_map_name(sbdev, ACPI_PCIHP_REGION_NAME,
> >> + vms->memmap[VIRT_ACPI_PCIHP].base);
> >
> > I don't like mix of old way (index based) above and new name based mapping,
> > can we use the same, please?
> >
> >> + assert(i >= 0);
> > g_assert(sysbus_mmio_map_name...) to get more meaning-full crash
> > that is not compiled out.
> >
> >> + acpi_pcihp_init(OBJECT(dev), &acpi_ged_state->pcihp_state,
> >> + vms->bus, sysbus_mmio_get_region(sbdev, i), 0);
> >
> > hmm, looks broken..
> > region mapping must happen after acpi_pcihp_init().
> >
> > if we after making sysbus_mmio_map() sane and easier to use
> > (which is a bit on tangent to this series).
> > We could feed sysbus owner device a memory map (ex: name based),
> > and then use [pre_]plug handlers on sysbus to map children
> > automatically.
> > That will alleviate need to do all mapping manually in every board.
> > (frankly speaking it deserves its own series, with tree wide cleanup).
> >
> > As it is I'd use old index based approach like the rest.
> > (unless you feel adventurous about sysbus refactoring)
> >
> >
> >> + acpi_ged_state->pcihp_state.use_acpi_hotplug_bridge = true;
> >> + }
> >> sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, qdev_get_gpio_in(vms->gic, irq));
> >>
> >> return dev;
> >> @@ -1758,6 +1776,13 @@ void virt_machine_done(Notifier *notifier, void *data)
> >> pci_bus_add_fw_cfg_extra_pci_roots(vms->fw_cfg, vms->bus,
> >> &error_abort);
> >
> >
> >> +
> >> + if (vms->acpi_pcihp) {
> >> + AcpiGedState *acpi_ged_state = ACPI_GED(vms->acpi_dev);
> >> +
> >> + acpi_pcihp_reset(&acpi_ged_state->pcihp_state);
> >> + }
> >> +
> >> virt_acpi_setup(vms);
> >> virt_build_smbios(vms);
> >> }
> >> @@ -2395,8 +2420,6 @@ static void machvirt_init(MachineState *machine)
> >>
> >> create_rtc(vms);
> >>
> >> - create_pcie(vms);
> >> -
> >> if (has_ged && aarch64 && firmware_loaded && virt_is_acpi_enabled(vms)) {
> >> vms->acpi_pcihp &= !vmc->no_acpi_pcihp;
> >> vms->acpi_dev = create_acpi_ged(vms);
> >> @@ -2405,6 +2428,15 @@ static void machvirt_init(MachineState *machine)
> >> create_gpio_devices(vms, VIRT_GPIO, sysmem);
> >> }
> >>
> >> + create_pcie(vms);
> >> +
> >> + if (vms->acpi_dev) {
> >> + AcpiGedState *acpi_ged_state = ACPI_GED(vms->acpi_dev);
> >> +
> >> + acpi_ged_state = ACPI_GED(vms->acpi_dev);
> >> + acpi_ged_state->pcihp_state.root = vms->bus;
> >> + }
> >> +
> >> if (vms->secure && !vmc->no_secure_gpio) {
> >> create_gpio_devices(vms, VIRT_SECURE_GPIO, secure_sysmem);
> >> }
> >
> > I don't like pulling acpi_pcihp_init()/reset (and issues it causes) into board code,
> > on x86 it's a part of host bridge device model.
> > The same should apply to GED device.
> >
> > The only thing board has to do is map regions into IO space like we do
> > everywhere else.
> >
> > with current code, may be add link<pci_bus> property to GED,
> > and set it before GED realize in create_acpi_ged(),
> > then just follow existing sysbus_mmio_map() pattern.
> >
>
On 5/27/25 13:44, Gustavo Romero wrote:
> Hi folks,
>
> On 5/27/25 12:56, Igor Mammedov wrote:
>> On Tue, 27 May 2025 09:40:24 +0200
>> Eric Auger <eric.auger@redhat.com> wrote:
>>
>>> Set up the IO registers used to communicate between QEMU
>>> and ACPI.
>>
>>
>>
>>> Move the create_pcie() call after the creation of the acpi
>>> ged device since hotplug callbacks will soon be called on gpex
>>> realize and will require the acpi pcihp state to be initialized.
>>>
>>> The hacky thing is the root bus has not yet been created on
>>> acpi_pcihp_init() call so it is set later after the gpex realize.
>>
>> can you elaborate on this, preferably with call expected call flows?
>
> The way I'm understanding it is: because the GED and GPEX are created
> in virt.c, so at the machine "level" (and so a machine hotplug handler is
> used instead of a device hotplug handler, like it's done in PIIX4 and ICH9
> controller in the x86 machines), and they need to be created in that
> order because of dependecy of GPEX of GED, the flow is:
>
> 1) create_acpi_ged() -> calls acpi_pcihp_init(), which by its turn will inspect the root bus to generate the ACPI code
> 2) create_pcie() -> just in realize the root bus will be created
>
> and the sequence has to be 1 -> 2, because GED will be used in the GPEX callbacks on hotplug.
>
> So here we're following a different design from the current one in x86,
> which handles the hotplug at the PCI controller "level".
... where this chicken & egg doesn't exist, I meant.
So one option would be follow the ICH9 design, i.e.,
the call to acpi_pcihp_init() would be from a gpex_root_class_realize() (to be created)
and set in gpex_root_class_init(), like k->realize = gpex_root_class_realize.
AcpiPciHpState would then be part of GPEXRootState struct. TYPE_GPEX_ROOT_DEVICE would
also define the interfaces TYPE_HOTPLUG_HANDLER and TYPE_ACPI_DEVICE_IF, and
gpex_root_class_init() will set callbacks on these interfaces, for instance:
hc->pre_plug = gpex_device_pre_plug_cb;
hc->plug = gpex_device_plug_cb;
hc->unplug_request = gpex_device_unplug_request_cb;
hc->unplug = gpex_device_unplug_cb;
hc->is_hotpluggable_bus = gpex_is_hotpluggable_bus;
adevc->send_event = gpex_root_acpi_send_event;
Cheers,
Gustavo
>
> Cheers,
> Gustavo
>
>>> How to fix this chicken & egg issue?
>>>
>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>>
>>> ---
>>>
>>> v1 -> v2:
>>> - use ACPI_PCIHP_REGION_NAME
>>> ---
>>> include/hw/arm/virt.h | 1 +
>>> hw/arm/virt-acpi-build.c | 1 +
>>> hw/arm/virt.c | 42 +++++++++++++++++++++++++++++++++++-----
>>> 3 files changed, 39 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
>>> index 1b2e2e1284..a4c4e3a67a 100644
>>> --- a/include/hw/arm/virt.h
>>> +++ b/include/hw/arm/virt.h
>>> @@ -35,6 +35,7 @@
>>> #include "hw/boards.h"
>>> #include "hw/arm/boot.h"
>>> #include "hw/arm/bsa.h"
>>> +#include "hw/acpi/pcihp.h"
>>> #include "hw/block/flash.h"
>>> #include "system/kvm.h"
>>> #include "hw/intc/arm_gicv3_common.h"
>>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>>> index 9d88ffc318..cd49f67d60 100644
>>> --- a/hw/arm/virt-acpi-build.c
>>> +++ b/hw/arm/virt-acpi-build.c
>>> @@ -44,6 +44,7 @@
>>> #include "hw/acpi/generic_event_device.h"
>>> #include "hw/acpi/tpm.h"
>>> #include "hw/acpi/hmat.h"
>>> +#include "hw/acpi/pcihp.h"
>>> #include "hw/pci/pcie_host.h"
>>> #include "hw/pci/pci.h"
>>> #include "hw/pci/pci_bus.h"
>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>>> index 4aa40c8e8b..cdcff0a984 100644
>>> --- a/hw/arm/virt.c
>>> +++ b/hw/arm/virt.c
>>> @@ -682,6 +682,8 @@ static inline DeviceState *create_acpi_ged(VirtMachineState *vms)
>>> {
>>> DeviceState *dev;
>>> MachineState *ms = MACHINE(vms);
>>> + SysBusDevice *sbdev;
>>> +
>>> int irq = vms->irqmap[VIRT_ACPI_GED];
>>> uint32_t event = ACPI_GED_PWR_DOWN_EVT;
>>> @@ -693,12 +695,28 @@ static inline DeviceState *create_acpi_ged(VirtMachineState *vms)
>>> event |= ACPI_GED_NVDIMM_HOTPLUG_EVT;
>>> }
>>> + if (vms->acpi_pcihp) {
>>> + event |= ACPI_GED_PCI_HOTPLUG_EVT;
>>> + }
>>> +
>>> dev = qdev_new(TYPE_ACPI_GED);
>>> qdev_prop_set_uint32(dev, "ged-event", event);
>>> - sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>>> + sbdev = SYS_BUS_DEVICE(dev);
>>> + sysbus_realize_and_unref(sbdev, &error_fatal);
>>> - sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, vms->memmap[VIRT_ACPI_GED].base);
>>> - sysbus_mmio_map(SYS_BUS_DEVICE(dev), 1, vms->memmap[VIRT_PCDIMM_ACPI].base);
>>> + sysbus_mmio_map(sbdev, 0, vms->memmap[VIRT_ACPI_GED].base);
>>> + sysbus_mmio_map(sbdev, 1, vms->memmap[VIRT_PCDIMM_ACPI].base);
>>
>>
>> Perhaps move out sbdev renaming into a separate patch, as it's not really related.
>>
>>> + if (vms->acpi_pcihp) {
>>> + AcpiGedState *acpi_ged_state = ACPI_GED(dev);
>>> + int i;
>>> +
>>> + i = sysbus_mmio_map_name(sbdev, ACPI_PCIHP_REGION_NAME,
>>> + vms->memmap[VIRT_ACPI_PCIHP].base);
>>
>> I don't like mix of old way (index based) above and new name based mapping,
>> can we use the same, please?
>>
>>> + assert(i >= 0);
>> g_assert(sysbus_mmio_map_name...) to get more meaning-full crash
>> that is not compiled out.
>>
>>> + acpi_pcihp_init(OBJECT(dev), &acpi_ged_state->pcihp_state,
>>> + vms->bus, sysbus_mmio_get_region(sbdev, i), 0);
>>
>> hmm, looks broken..
>> region mapping must happen after acpi_pcihp_init().
>>
>> if we after making sysbus_mmio_map() sane and easier to use
>> (which is a bit on tangent to this series).
>> We could feed sysbus owner device a memory map (ex: name based),
>> and then use [pre_]plug handlers on sysbus to map children
>> automatically.
>> That will alleviate need to do all mapping manually in every board.
>> (frankly speaking it deserves its own series, with tree wide cleanup).
>>
>> As it is I'd use old index based approach like the rest.
>> (unless you feel adventurous about sysbus refactoring)
>>
>>
>>> + acpi_ged_state->pcihp_state.use_acpi_hotplug_bridge = true;
>>> + }
>>> sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, qdev_get_gpio_in(vms->gic, irq));
>>> return dev;
>>> @@ -1758,6 +1776,13 @@ void virt_machine_done(Notifier *notifier, void *data)
>>> pci_bus_add_fw_cfg_extra_pci_roots(vms->fw_cfg, vms->bus,
>>> &error_abort);
>>
>>
>>> +
>>> + if (vms->acpi_pcihp) {
>>> + AcpiGedState *acpi_ged_state = ACPI_GED(vms->acpi_dev);
>>> +
>>> + acpi_pcihp_reset(&acpi_ged_state->pcihp_state);
>>> + }
>>> +
>>> virt_acpi_setup(vms);
>>> virt_build_smbios(vms);
>>> }
>>> @@ -2395,8 +2420,6 @@ static void machvirt_init(MachineState *machine)
>>> create_rtc(vms);
>>> - create_pcie(vms);
>>> -
>>> if (has_ged && aarch64 && firmware_loaded && virt_is_acpi_enabled(vms)) {
>>> vms->acpi_pcihp &= !vmc->no_acpi_pcihp;
>>> vms->acpi_dev = create_acpi_ged(vms);
>>> @@ -2405,6 +2428,15 @@ static void machvirt_init(MachineState *machine)
>>> create_gpio_devices(vms, VIRT_GPIO, sysmem);
>>> }
>>> + create_pcie(vms);
>>> +
>>> + if (vms->acpi_dev) {
>>> + AcpiGedState *acpi_ged_state = ACPI_GED(vms->acpi_dev);
>>> +
>>> + acpi_ged_state = ACPI_GED(vms->acpi_dev);
>>> + acpi_ged_state->pcihp_state.root = vms->bus;
>>> + }
>>> +
>>> if (vms->secure && !vmc->no_secure_gpio) {
>>> create_gpio_devices(vms, VIRT_SECURE_GPIO, secure_sysmem);
>>> }
>>
>> I don't like pulling acpi_pcihp_init()/reset (and issues it causes) into board code,
>> on x86 it's a part of host bridge device model.
>> The same should apply to GED device.
>>
>> The only thing board has to do is map regions into IO space like we do
>> everywhere else.
>>
>> with current code, may be add link<pci_bus> property to GED,
>> and set it before GED realize in create_acpi_ged(),
>> then just follow existing sysbus_mmio_map() pattern.
>>
>
Hi Eric, On 27/5/25 09:40, Eric Auger wrote: > Set up the IO registers used to communicate between QEMU > and ACPI. > > Move the create_pcie() call after the creation of the acpi > ged device since hotplug callbacks will soon be called on gpex > realize and will require the acpi pcihp state to be initialized. > > The hacky thing is the root bus has not yet been created on > acpi_pcihp_init() call so it is set later after the gpex realize. > > How to fix this chicken & egg issue? FYI I have this question tagged since v1, I will try to look at it in some days... > > Signed-off-by: Eric Auger <eric.auger@redhat.com> > > --- > > v1 -> v2: > - use ACPI_PCIHP_REGION_NAME > --- > include/hw/arm/virt.h | 1 + > hw/arm/virt-acpi-build.c | 1 + > hw/arm/virt.c | 42 +++++++++++++++++++++++++++++++++++----- > 3 files changed, 39 insertions(+), 5 deletions(-)
© 2016 - 2026 Red Hat, Inc.