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>
---
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)));
+ /* 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);
}
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")));
--
2.49.0On 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?
> }
>
> 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")));
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")));
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.
Done
Thanks
Eric
>
> 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?
>
>> }
>>
>> 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")));
On Fri, 30 May 2025 11:02:27 +0100
Jonathan Cameron <Jonathan.Cameron@huawei.com> 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.
>
[...]
unless CXL is root host bridge, current _DSM shouldn't be added to it.
read on comment below.
> > @@ -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.
It's fine for each PXB to have it's own _OSC.
Also current incarnation of ACPI pcihp doesn't support PXBs at all,
it would be wrong to enable the on PXBs.
Thus I'd avoid touching CXL related code paths from this series.
I'm working on extending ACPI pcihp to PXBs
(for the same reason as Eric does for arm/virt, i.e. enable acpi-index support there).
I can add CXL bits then if there is a need/demand for that in CXL land.
[...]
On Fri, 30 May 2025 14:05:16 +0200
Igor Mammedov <imammedo@redhat.com> wrote:
> On Fri, 30 May 2025 11:02:27 +0100
> Jonathan Cameron <Jonathan.Cameron@huawei.com> 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.
> >
> [...]
> unless CXL is root host bridge, current _DSM shouldn't be added to it.
> read on comment below.
I'm not clear how this is different from pxb-pcie where we do have
the _DSM. Both are pretending to be real host bridges.
>
> > > @@ -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.
>
> It's fine for each PXB to have it's own _OSC.
> Also current incarnation of ACPI pcihp doesn't support PXBs at all,
> it would be wrong to enable the on PXBs.
>
> Thus I'd avoid touching CXL related code paths from this series.
>
> I'm working on extending ACPI pcihp to PXBs
> (for the same reason as Eric does for arm/virt, i.e. enable acpi-index support there).
> I can add CXL bits then if there is a need/demand for that in CXL land.
Ok. My original motivation for _DSM on CXL was function 5 to stop Linux messing up
the reenumeration which I know has been rejected upstream for a bunch of
compatibility reasons. Anyhow, that's a future problem.
Thanks,
Jonathan
>
> [...]
>
>
On Fri, 30 May 2025 16:00:19 +0100
Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
> On Fri, 30 May 2025 14:05:16 +0200
> Igor Mammedov <imammedo@redhat.com> wrote:
>
> > On Fri, 30 May 2025 11:02:27 +0100
> > Jonathan Cameron <Jonathan.Cameron@huawei.com> 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.
> > >
> > [...]
> > unless CXL is root host bridge, current _DSM shouldn't be added to it.
> > read on comment below.
>
> I'm not clear how this is different from pxb-pcie where we do have
> the _DSM. Both are pretending to be real host bridges.
there is some space for _OSC consolidation, but it's not realy related to pcihp,
so I'd rather do it as a separate series on top.
current PCI _DSM and _CXL one (build_cxl_dsm_method) are implementing
different namespaces, there is not much to consolidate there.
If later on CXL would need E5C937D0-3553-4D7A-9117-EA4D19C3434D,
then, I'd say do it at that time and use that moment as an opportunity
to consolidate.
> >
> > > > @@ -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.
> >
> > It's fine for each PXB to have it's own _OSC.
> > Also current incarnation of ACPI pcihp doesn't support PXBs at all,
> > it would be wrong to enable the on PXBs.
> >
> > Thus I'd avoid touching CXL related code paths from this series.
> >
> > I'm working on extending ACPI pcihp to PXBs
> > (for the same reason as Eric does for arm/virt, i.e. enable acpi-index support there).
> > I can add CXL bits then if there is a need/demand for that in CXL land.
>
> Ok. My original motivation for _DSM on CXL was function 5 to stop Linux messing up
> the reenumeration which I know has been rejected upstream for a bunch of
> compatibility reasons. Anyhow, that's a future problem.
yep, let's worry about merging _DSMs when that future comes.
>
> Thanks,
>
> Jonathan
>
> >
> > [...]
> >
> >
>
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>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> ---
> 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)));
> + /* 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);
> }
>
> 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")));
© 2016 - 2026 Red Hat, Inc.