Re: [PATCH v4 25/32] hw/acpi/ged: Prepare the device to react to PCI hotplug events

[Eric Auger <eric.auger@redhat.com>]

Hi Jonathan,

On 6/30/25 2:49 PM, Jonathan Cameron wrote:
> On Fri, 27 Jun 2025 11:55:14 +0200
> Eric Auger <eric.auger@redhat.com> 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 occurrence 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>
> Maybe call out why you aren't following the advice in the
> docs for device_class_set_legacy_reset() to use the resettable API. 
Good point. I think I shall migrate to the new API.
>
> One other question inline about setting of the event bitmap in
> ged_realize rather than create_acpi_ged() in virt.c
>
> There is not obviously right answer to where that should be but
> what you have here seems inconsistent with existing code s
> a comment may makes sense if you leave it as it stands.
>
>> ---
>> v3 -> v4:
>> - add qbus_set_hotplug_handler
>> - root bus is not passed in acpi_pcihp_init arg
>>
>> v2 -> v3:
>> - pcihp_init and reset are put in ged code instead of machine code
>>   (Igor)
>> - Add ACPI_GED_PCI_HOTPLUG_EVT event depending on use_acpi_hotplug_bridge
>>   (Igor)
>>
>> v1 -> v2:
>> - Introduce ACPI_PCIHP_REGION_NAME
>> diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
>> index 92b931758f..fc84bfb34e 100644
>> --- a/hw/acpi/generic_event_device.c
>> +++ b/hw/acpi/generic_event_device.c
>> @@ -427,9 +437,13 @@ static void acpi_ged_realize(DeviceState *dev, Error **errp)
>>  {
>>      SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>>      AcpiGedState *s = ACPI_GED(dev);
>> +    AcpiPciHpState *pcihp_state = &s->pcihp_state;
>>      uint32_t ged_events;
>>      int i;
>>  
>> +    if (pcihp_state->use_acpi_hotplug_bridge) {
>> +        s->ged_event_bitmap |= ACPI_GED_PCI_HOTPLUG_EVT;
>> +    }
> Maybe a comment on why this belongs insider the GED code rather
> than being provided in the event_bitmap from virt.c or similar
> as is done for all the over elements of ged_event_bitmap.
>
> Particularly as you get the acpi_pcihp property in create_acpi_ged()
> in patch 29.
Actually Igor suggested to put this setting in the realize()
https://lore.kernel.org/all/20250620150648.09dab163@fedora/
instead of in create_acpi_ged(), as it was done in v3 So I followed his
guidance Eric
>
>
>>      ged_events = ctpop32(s->ged_event_bitmap);
>>  
>