On Tue, 27 May 2025 09:40:06 +0200
Eric Auger <eric.au...@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.au...@redhat.com>
> Reviewed-by: Gustavo Romero <gustavo.rom...@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!)
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")));
