Re: [PATCH v2 04/25] hw/pci-host/gpex-acpi: Add native_pci_hotplug arg to acpi_dsdt_add_pci_osc

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

Hi Jonathan,

On 5/30/25 11:27 AM, Jonathan Cameron wrote:
> On Tue, 27 May 2025 09:40:06 +0200
> Eric Auger <eric.auger@redhat.com> wrote:
>
>> Add a new argument to acpi_dsdt_add_pci_osc to be able to disable
>> native pci hotplug.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> Reviewed-by: Gustavo Romero <gustavo.romero@linaro.org>
> Hi Eric,
>
> Makes me wonder what we should do for CXL - I was expecting
> a very similar change.  Currently seems like those always
> allow native hotplug (__build_cxl_osc_method()) on x86 and
> arm64 (patches on list).
>
> Maybe that has only been working because the kernel is reading
> the PCI _OSC first. Or it's always been doing native hotplug
> an no one noticed.  A quick look at logs shows the kernel
> first gets told no, then yes as it queries the two different
> _OSC types. 
>
> Looks like I should fix that _OSC then it should be carried
> over to this as well (or if you don't mind adding a trivial
> patch to replicate this  patch for the CXL _OSC, even better!)

If you don't mind I would prefer we carry out cxl changes separately as
it may also require some test blob changes and I am not much
knowledgable on CXL yet. I would prefer stabilizing this series before
extending it.

But sure if it is relevant we can then mimic that change on cxl path.

Eric
>
> Other than that, this patch looks fine to me though I do wonder
> if we could unify this with build_q35_osc_method()?
> I'm not the best at reading AML generation code but whilst
> they are written quite differently they seem to be functionally
> very similar, more so after this patch.
>
>> ---
>>
>> rfc -> v1:
>> - updated the "Allow OS control for all 5 features" comment
>> ---
>>  hw/pci-host/gpex-acpi.c | 15 ++++++++-------
>>  1 file changed, 8 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/pci-host/gpex-acpi.c b/hw/pci-host/gpex-acpi.c
>> index 0aba47c71c..f34b7cf25e 100644
>> --- a/hw/pci-host/gpex-acpi.c
>> +++ b/hw/pci-host/gpex-acpi.c
>> @@ -50,7 +50,7 @@ static void acpi_dsdt_add_pci_route_table(Aml *dev, uint32_t irq,
>>      }
>>  }
>>  
>> -static void acpi_dsdt_add_pci_osc(Aml *dev)
>> +static void acpi_dsdt_add_pci_osc(Aml *dev, bool enable_native_pcie_hotplug)
>>  {
>>      Aml *method, *UUID, *ifctx, *ifctx1, *elsectx, *buf;
>>  
>> @@ -77,11 +77,12 @@ static void acpi_dsdt_add_pci_osc(Aml *dev)
>>      aml_append(ifctx, aml_store(aml_name("CDW3"), aml_name("CTRL")));
>>  
>>      /*
>> -     * Allow OS control for all 5 features:
>> -     * PCIeHotplug SHPCHotplug PME AER PCIeCapability.
>> +     * Allow OS control for SHPCHotplug, PME, AER, PCIeCapability,
>> +     * and PCIeHotplug depending on enable_native_pcie_hotplug
>>       */
>> -    aml_append(ifctx, aml_and(aml_name("CTRL"), aml_int(0x1F),
>> -                              aml_name("CTRL")));
>> +    aml_append(ifctx, aml_and(aml_name("CTRL"),
>> +               aml_int(0x1E | (enable_native_pcie_hotplug ? 0x1 : 0x0)),
>> +               aml_name("CTRL")));
>>  
>>      ifctx1 = aml_if(aml_lnot(aml_equal(aml_arg(1), aml_int(0x1))));
>>      aml_append(ifctx1, aml_or(aml_name("CDW1"), aml_int(0x08),
>> @@ -192,7 +193,7 @@ void acpi_dsdt_add_gpex(Aml *scope, struct GPEXConfig *cfg)
>>              if (is_cxl) {
>>                  build_cxl_osc_method(dev);
> This was the path I was expecting to change as well.
>
>>              } else {
>> -                acpi_dsdt_add_pci_osc(dev);
>> +                acpi_dsdt_add_pci_osc(dev, true);
>>              }
>>  
>>              aml_append(scope, dev);
>> @@ -267,7 +268,7 @@ void acpi_dsdt_add_gpex(Aml *scope, struct GPEXConfig *cfg)
>>      }
>>      aml_append(dev, aml_name_decl("_CRS", rbuf));
>>  
>> -    acpi_dsdt_add_pci_osc(dev);
>> +    acpi_dsdt_add_pci_osc(dev, true);
>>  
>>      Aml *dev_res0 = aml_device("%s", "RES0");
>>      aml_append(dev_res0, aml_name_decl("_HID", aml_string("PNP0C02")));