Re: [PATCH v2 06/25] hw/pci-host/gpex-acpi: Propagate hotplug type info from virt machine downto gpex

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

Hi Jonathan,

On 5/30/25 12:14 PM, Jonathan Cameron wrote:
> On Tue, 27 May 2025 09:40:08 +0200
> Eric Auger <eric.auger@redhat.com> wrote:
>
>> Propagate the type of pci hotplug mode downto the gpex
>> acpi code. In case machine acpi_pcihp is unset we configure
>> pci native hotplug on pci0. For expander bridges we keep
>> legacy pci native hotplug, as done on x86 q35.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> Reviewed-by: Gustavo Romero <gustavo.romero@linaro.org>
>> ---
>>  include/hw/pci-host/gpex.h | 1 +
>>  hw/arm/virt-acpi-build.c   | 1 +
>>  hw/pci-host/gpex-acpi.c    | 3 ++-
>>  3 files changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/hw/pci-host/gpex.h b/include/hw/pci-host/gpex.h
>> index 84471533af..feaf827474 100644
>> --- a/include/hw/pci-host/gpex.h
>> +++ b/include/hw/pci-host/gpex.h
>> @@ -45,6 +45,7 @@ struct GPEXConfig {
>>      MemMapEntry pio;
>>      int         irq;
>>      PCIBus      *bus;
>> +    bool        pci_native_hotplug;
>>  };
>>  
>>  typedef struct GPEXIrq GPEXIrq;
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index 7e8e0f0298..be5e00a56e 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -129,6 +129,7 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap,
>>          .ecam   = memmap[ecam_id],
>>          .irq    = irq,
>>          .bus    = vms->bus,
>> +        .pci_native_hotplug = !vms->acpi_pcihp,
>>      };
>>  
>>      if (vms->highmem_mmio) {
>> diff --git a/hw/pci-host/gpex-acpi.c b/hw/pci-host/gpex-acpi.c
>> index 1aa2d12026..f1ab30f3d5 100644
>> --- a/hw/pci-host/gpex-acpi.c
>> +++ b/hw/pci-host/gpex-acpi.c
>> @@ -204,6 +204,7 @@ void acpi_dsdt_add_gpex(Aml *scope, struct GPEXConfig *cfg)
>>              if (is_cxl) {
>>                  build_cxl_osc_method(dev);
>>              } else {
>> +                /* pxb bridges do not have ACPI PCI Hot-plug enabled */
>>                  acpi_dsdt_add_host_bridge_methods(dev, true);
> This is awkward but explains why my CXL cases weren't causing trouble.
> A mixed config is counter to the recommendation in the PCI firmware spec
>
> "It is recommended that a machine with multiple host bridge devices should
> report the same capabilities for all host bridges of the same type and also
> negotiate control of the features described in the Control Field in the
> same way for all host bridges of the same type"

Thank for pointing to this spec excerpt.

Maybe the nuance relates to "host bridges of the same type", ie. gpex
versus pxb?
refering to Igor's following reply i will leave it as is. This is
inherited from existing x86 code in
hw/i386/acpi-build.c build_dsdt():

                /* Expander bridges do not have ACPI PCI Hot-plug enabled */
                aml_append(dev, build_q35_osc_method(true));

Eric
>
> I guess if any OS isn't coping with the mix then they can request native
> hotplug.
>
>
>
>>              }
>>  
>> @@ -279,7 +280,7 @@ void acpi_dsdt_add_gpex(Aml *scope, struct GPEXConfig *cfg)
>>      }
>>      aml_append(dev, aml_name_decl("_CRS", rbuf));
>>  
>> -    acpi_dsdt_add_host_bridge_methods(dev, true);
>> +    acpi_dsdt_add_host_bridge_methods(dev, cfg->pci_native_hotplug);
>>  
>>      Aml *dev_res0 = aml_device("%s", "RES0");
>>      aml_append(dev_res0, aml_name_decl("_HID", aml_string("PNP0C02")));