Hi Jonathan On 6/30/25 2:53 PM, Jonathan Cameron wrote: > On Fri, 27 Jun 2025 11:55:18 +0200 > Eric Auger <[email protected]> wrote: > >> Set up the IO registers used to communicate between QEMU >> and ACPI. >> >> Signed-off-by: Eric Auger <[email protected]> > Follow on comment inline. Otherwise LGTM > > Reviewed-by: Jonathan Cameron <[email protected]> thanks! > >> --- >> v2 -> v3: >> - remove acpi_ged_state->pcihp_state.use_acpi_hotplug_bridge = true; >> - use sysbus_mmio_map_name for all regs (Igor) >> - create_pcie left at its original place >> >> v1 -> v2: >> - use ACPI_PCIHP_REGION_NAME >> --- >> hw/arm/virt.c | 13 +++++++++++++ >> 1 file changed, 13 insertions(+) >> >> diff --git a/hw/arm/virt.c b/hw/arm/virt.c >> index 878c567354..d8706ef9c8 100644 >> --- a/hw/arm/virt.c >> +++ b/hw/arm/virt.c >> @@ -686,6 +686,7 @@ static inline DeviceState >> *create_acpi_ged(VirtMachineState *vms) >> SysBusDevice *sbdev; >> int irq = vms->irqmap[VIRT_ACPI_GED]; >> uint32_t event = ACPI_GED_PWR_DOWN_EVT; >> + bool acpi_pcihp; >> >> if (ms->ram_slots) { >> event |= ACPI_GED_MEM_HOTPLUG_EVT; >> @@ -704,6 +705,18 @@ static inline DeviceState >> *create_acpi_ged(VirtMachineState *vms) >> sysbus_mmio_map_name(sbdev, TYPE_ACPI_GED, >> vms->memmap[VIRT_ACPI_GED].base); >> sysbus_mmio_map_name(sbdev, ACPI_MEMHP_REGION_NAME, >> vms->memmap[VIRT_PCDIMM_ACPI].base); >> + >> + acpi_pcihp = object_property_get_bool(OBJECT(dev), >> + ACPI_PM_PROP_ACPI_PCIHP_BRIDGE, >> NULL); >> + >> + if (acpi_pcihp) { > As mentioned in earlier patch review, with this code you now have means to > set the event > bitmap as done in other cases. Maybe just do that here as well? see my previous reply
Hope this clarifies Eric > >> + int pcihp_region_index; >> + >> + pcihp_region_index = sysbus_mmio_map_name(sbdev, >> ACPI_PCIHP_REGION_NAME, >> + >> vms->memmap[VIRT_ACPI_PCIHP].base); >> + assert(pcihp_region_index >= 0); >> + } >> + >> sysbus_connect_irq(sbdev, 0, qdev_get_gpio_in(vms->gic, irq)); >> >> return dev;
