Re: [PATCH v2 05/25] hw/pci-host/gpex-acpi: Split host bridge OSC and DSM generation

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

Hi Jonathan

On 5/30/25 12:02 PM, Jonathan Cameron wrote:
> On Tue, 27 May 2025 09:40:07 +0200
> Eric Auger <eric.auger@redhat.com> wrote:
>
>> acpi_dsdt_add_pci_osc() name is confusing as it gives the impression
>> it appends the _OSC method but in fact it also appends the _DSM method
>> for the host bridge. Let's split the function into two separate ones
>> and let them return the method Aml pointer instead. This matches the
>> way it is done on x86 (build_q35_osc_method). In a subsequent patch
>> we will replace the gpex method by the q35 implementation that will
>> become shared between ARM and x86.
>>
>> acpi_dsdt_add_host_bridge_methods is a new top helper that generates
>> both the _OSC and _DSM methods.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> Reviewed-by: Gustavo Romero <gustavo.romero@linaro.org>
> Makes complete sense. I've had local equivalent of this on the CXL
> tree for a while as without it we don't register the _DSM for the
> CXL path (and we should).  However, can you modify it a little to
> make that easier for me?  Basically make sure the _DSM is registered
> for the CXL path as well.
>
> One other comment inline.
>
>
>> ---
>>  hw/pci-host/gpex-acpi.c | 31 +++++++++++++++++++++----------
>>  1 file changed, 21 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/pci-host/gpex-acpi.c b/hw/pci-host/gpex-acpi.c
>> index f34b7cf25e..1aa2d12026 100644
>> --- a/hw/pci-host/gpex-acpi.c
>> +++ b/hw/pci-host/gpex-acpi.c
>> @@ -50,13 +50,10 @@ static void acpi_dsdt_add_pci_route_table(Aml *dev, uint32_t irq,
>>      }
>>  }
>>  
>> -static void acpi_dsdt_add_pci_osc(Aml *dev, bool enable_native_pcie_hotplug)
>> +static Aml *build_host_bridge_osc(bool enable_native_pcie_hotplug)
>>  {
>> -    Aml *method, *UUID, *ifctx, *ifctx1, *elsectx, *buf;
>> +    Aml *method, *UUID, *ifctx, *ifctx1, *elsectx;
>>  
>> -    /* Declare an _OSC (OS Control Handoff) method */
>> -    aml_append(dev, aml_name_decl("SUPP", aml_int(0)));
>> -    aml_append(dev, aml_name_decl("CTRL", aml_int(0)));
>>      method = aml_method("_OSC", 4, AML_NOTSERIALIZED);
>>      aml_append(method,
>>          aml_create_dword_field(aml_arg(3), aml_int(0), "CDW1"));
>> @@ -103,9 +100,13 @@ static void acpi_dsdt_add_pci_osc(Aml *dev, bool enable_native_pcie_hotplug)
>>                                 aml_name("CDW1")));
>>      aml_append(elsectx, aml_return(aml_arg(3)));
>>      aml_append(method, elsectx);
>> -    aml_append(dev, method);
>> +    return method;
>> +}
>>  
>> -    method = aml_method("_DSM", 4, AML_NOTSERIALIZED);
>> +static Aml *build_host_bridge_dsm(void)
>> +{
>> +    Aml *method = aml_method("_DSM", 4, AML_NOTSERIALIZED);
>> +    Aml *UUID, *ifctx, *ifctx1, *buf;
>>  
>>      /* PCI Firmware Specification 3.0
>>       * 4.6.1. _DSM for PCI Express Slot Information
>> @@ -124,7 +125,17 @@ static void acpi_dsdt_add_pci_osc(Aml *dev, bool enable_native_pcie_hotplug)
>>      byte_list[0] = 0;
>>      buf = aml_buffer(1, byte_list);
>>      aml_append(method, aml_return(buf));
>> -    aml_append(dev, method);
>> +    return method;
>> +}
>> +
>> +static void acpi_dsdt_add_host_bridge_methods(Aml *dev,
>> +                                              bool enable_native_pcie_hotplug)
>> +{
>> +    aml_append(dev, aml_name_decl("SUPP", aml_int(0)));
>> +    aml_append(dev, aml_name_decl("CTRL", aml_int(0)));
> These two declarations seem to be very much part of the _OSC build though not
> within the the method.  I 'think' you get left with them later with no users.
> So move them into the osc build here and they will naturally go away when
> you move to the generic code.
>
> They end up unused in the DSDT at the end of the series.
>
> I ran a quick GPEX + pxb-pcie test and we do get the odd mix that the OSC for
> the GPEX say no native hotplug but the OSC for the PXB allows it.
>
>> +    /* Declare an _OSC (OS Control Handoff) method */
>> +    aml_append(dev, build_host_bridge_osc(enable_native_pcie_hotplug));
>> +    aml_append(dev, build_host_bridge_dsm());
>>  }
>>  
>>  void acpi_dsdt_add_gpex(Aml *scope, struct GPEXConfig *cfg)
>> @@ -193,7 +204,7 @@ void acpi_dsdt_add_gpex(Aml *scope, struct GPEXConfig *cfg)
>>              if (is_cxl) {
>>                  build_cxl_osc_method(dev);
>>              } else {
>> -                acpi_dsdt_add_pci_osc(dev, true);
>> +                acpi_dsdt_add_host_bridge_methods(dev, true);
> Can you either drop the use of the wrapper for the DSM part here and call
> it unconditionally (for cxl and PCIe cases) or add an extra call to
> aml_append(dev, build_host_bridge_dsm()) for the is_cxl path?

Given the following discussion between you and Igor, I understand we can
keep the code as is for now. cxl alignment would be done later and also
pxb support might be reworked in near future.

Eric
>
>>              }
>>  
>>              aml_append(scope, dev);
>> @@ -268,7 +279,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, true);
>> +    acpi_dsdt_add_host_bridge_methods(dev, true);
>>  
>>      Aml *dev_res0 = aml_device("%s", "RES0");
>>      aml_append(dev_res0, aml_name_decl("_HID", aml_string("PNP0C02")));