| Message ID | 20240222124110.2681455-1-vidyas@nvidia.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | [V3] PCI: Add support for preserving boot configuration | expand |
On Thu, Feb 22, 2024 at 06:11:10PM +0530, Vidya Sagar wrote: > Add support for preserving the boot configuration done by the > platform firmware per host bridge basis, based on the presence of > 'linux,pci-probe-only' property in the respective PCIe host bridge > device-tree node. It also unifies the ACPI and DT based boot flows > in this regard. > +/** > + * of_pci_bridge_check_probe_only - Return true if the boot configuration > + * needs to be preserved I don't like the "check_probe_only" name because it's a boolean function but the name doesn't tell me what a true/false return value means. Something like "preserve_resources" would be better. If you want "probe_only", even removing the "check" would help. > + * @node: Device tree node with the domain information. > + * > + * This function looks for "linux,pci-probe-only" property for a given > + * PCIe controller's node and returns true if found. Having this property > + * for a PCIe controller ensures that the kernel doesn't re-enumerate and > + * reconfigure the BAR resources that are already done by the platform firmware. This is generic PCI, not PCIe-specific (also in commit log and comment below). I think "enumeration" specifically refers to discovering what devices are present, and the kernel always does that, so drop that part. Reconfiguring BARs and bridge windows is what we want to prevent. > + * NOTE: The scope of "linux,pci-probe-only" defined within a PCIe bridge device > + * is limited to the hierarchy under that particular bridge device. whereas > + * the scope of "linux,pci-probe-only" defined within chosen node is > + * system wide. > + * > + * Return: true if the property exists false otherwise. > + */ > +bool of_pci_bridge_check_probe_only(struct device_node *node) > +{ > + return of_property_read_bool(node, "linux,pci-probe-only"); > +} > +EXPORT_SYMBOL_GPL(of_pci_bridge_check_probe_only); Why does this need to be exported for modules and exposed via include/linux/pci.h? > +static void pci_check_config_preserve(struct pci_host_bridge *host_bridge) > +{ > + if (&host_bridge->dev) { Checking &host_bridge->dev doesn't seem like the right way to determine whether this is an ACPI host bridge. > + union acpi_object *obj; > + > + /* > + * Evaluate the "PCI Boot Configuration" _DSM Function. If it > + * exists and returns 0, we must preserve any PCI resource > + * assignments made by firmware for this host bridge. > + */ > + obj = acpi_evaluate_dsm(ACPI_HANDLE(&host_bridge->dev), &pci_acpi_dsm_guid, 1, > + DSM_PCI_PRESERVE_BOOT_CONFIG, NULL); > + if (obj && obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 0) > + host_bridge->preserve_config = 1; > + ACPI_FREE(obj); > + }
On 22-02-2024 22:36, Bjorn Helgaas wrote: > External email: Use caution opening links or attachments > > > On Thu, Feb 22, 2024 at 06:11:10PM +0530, Vidya Sagar wrote: >> Add support for preserving the boot configuration done by the >> platform firmware per host bridge basis, based on the presence of >> 'linux,pci-probe-only' property in the respective PCIe host bridge >> device-tree node. It also unifies the ACPI and DT based boot flows >> in this regard. >> +/** >> + * of_pci_bridge_check_probe_only - Return true if the boot configuration >> + * needs to be preserved > I don't like the "check_probe_only" name because it's a boolean > function but the name doesn't tell me what a true/false return value > means. Something like "preserve_resources" would be better. If you > want "probe_only", even removing the "check" would help. I'll change it in the next patch. >> + * @node: Device tree node with the domain information. >> + * >> + * This function looks for "linux,pci-probe-only" property for a given >> + * PCIe controller's node and returns true if found. Having this property >> + * for a PCIe controller ensures that the kernel doesn't re-enumerate and >> + * reconfigure the BAR resources that are already done by the platform firmware. > This is generic PCI, not PCIe-specific (also in commit log and comment > below). > > I think "enumeration" specifically refers to discovering what devices > are present, and the kernel always does that, so drop that part. > Reconfiguring BARs and bridge windows is what we want to prevent. Agree and I'll address it in the next patch. >> + * NOTE: The scope of "linux,pci-probe-only" defined within a PCIe bridge device >> + * is limited to the hierarchy under that particular bridge device. whereas >> + * the scope of "linux,pci-probe-only" defined within chosen node is >> + * system wide. >> + * >> + * Return: true if the property exists false otherwise. >> + */ >> +bool of_pci_bridge_check_probe_only(struct device_node *node) >> +{ >> + return of_property_read_bool(node, "linux,pci-probe-only"); >> +} >> +EXPORT_SYMBOL_GPL(of_pci_bridge_check_probe_only); > Why does this need to be exported for modules and exposed via > include/linux/pci.h? On a second through, I think it is not required to be exported. I'll address this also in the next patch. >> +static void pci_check_config_preserve(struct pci_host_bridge *host_bridge) >> +{ >> + if (&host_bridge->dev) { > Checking &host_bridge->dev doesn't seem like the right way to > determine whether this is an ACPI host bridge. Honestly, I couldn't find a clear way to differentiate between an ACPI based host bridge and a DT based host bridge. Hence, the current code tries to get the information using both ways and since a system can only be either ACPI or DT based, but one at a time, preserve_config will be set only once (assuming the system wants it to be set). Let me know if there is a better approach for this? I was looking at the way 'external_facing' gets set in both the boot flows and I see that there is no common place for it and the respective flows have their functions separately i.e. pci_acpi_set_external_facing() for ACPI and pci_set_bus_of_node() for DT. Thanks, Vidya Sagar >> + union acpi_object *obj; >> + >> + /* >> + * Evaluate the "PCI Boot Configuration" _DSM Function. If it >> + * exists and returns 0, we must preserve any PCI resource >> + * assignments made by firmware for this host bridge. >> + */ >> + obj = acpi_evaluate_dsm(ACPI_HANDLE(&host_bridge->dev), &pci_acpi_dsm_guid, 1, >> + DSM_PCI_PRESERVE_BOOT_CONFIG, NULL); >> + if (obj && obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 0) >> + host_bridge->preserve_config = 1; >> + ACPI_FREE(obj); >> + }
On Fri, Feb 23, 2024 at 02:48:24AM +0530, Vidya Sagar wrote: > On 22-02-2024 22:36, Bjorn Helgaas wrote: > > On Thu, Feb 22, 2024 at 06:11:10PM +0530, Vidya Sagar wrote: > > > + if (&host_bridge->dev) { > > Checking &host_bridge->dev doesn't seem like the right way to > > determine whether this is an ACPI host bridge. BTW, I think this condition is *always* true, since it's testing the address of a member of a struct. > Honestly, I couldn't find a clear way to differentiate between an > ACPI based host bridge and a DT based host bridge. Hence, the > current code tries to get the information using both ways and since > a system can only be either ACPI or DT based, but one at a time, > preserve_config will be set only once (assuming the system wants it > to be set). Let me know if there is a better approach for this? I'm not sure ACPI and DT will always be mutually exclusive; I think we're headed toward some combinations, e.g., https://lore.kernel.org/linux-pci/1692120000-46900-1-git-send-email-lizhi.hou@amd.com/ But I think "if (ACPI_HANDLE(&host_bridge->dev))" would work. Bjorn
diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c index 84030804a763..ddc2b3e89111 100644 --- a/drivers/acpi/pci_root.c +++ b/drivers/acpi/pci_root.c @@ -1008,7 +1008,6 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root, int node = acpi_get_node(device->handle); struct pci_bus *bus; struct pci_host_bridge *host_bridge; - union acpi_object *obj; info->root = root; info->bridge = device; @@ -1050,17 +1049,6 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root, if (!(root->osc_ext_control_set & OSC_CXL_ERROR_REPORTING_CONTROL)) host_bridge->native_cxl_error = 0; - /* - * Evaluate the "PCI Boot Configuration" _DSM Function. If it - * exists and returns 0, we must preserve any PCI resource - * assignments made by firmware for this host bridge. - */ - obj = acpi_evaluate_dsm(ACPI_HANDLE(bus->bridge), &pci_acpi_dsm_guid, 1, - DSM_PCI_PRESERVE_BOOT_CONFIG, NULL); - if (obj && obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 0) - host_bridge->preserve_config = 1; - ACPI_FREE(obj); - acpi_dev_power_up_children_with_adr(device); pci_scan_child_bus(bus); diff --git a/drivers/pci/controller/pci-host-common.c b/drivers/pci/controller/pci-host-common.c index 6be3266cd7b5..e2602e38ae45 100644 --- a/drivers/pci/controller/pci-host-common.c +++ b/drivers/pci/controller/pci-host-common.c @@ -73,10 +73,6 @@ int pci_host_common_probe(struct platform_device *pdev) if (IS_ERR(cfg)) return PTR_ERR(cfg); - /* Do not reassign resources if probe only */ - if (!pci_has_flag(PCI_PROBE_ONLY)) - pci_add_flags(PCI_REASSIGN_ALL_BUS); - bridge->sysdata = cfg; bridge->ops = (struct pci_ops *)&ops->pci_ops; bridge->msi_domain = true; diff --git a/drivers/pci/of.c b/drivers/pci/of.c index 51e3dd0ea5ab..7b553dd83587 100644 --- a/drivers/pci/of.c +++ b/drivers/pci/of.c @@ -258,6 +258,28 @@ void of_pci_check_probe_only(void) } EXPORT_SYMBOL_GPL(of_pci_check_probe_only); +/** + * of_pci_bridge_check_probe_only - Return true if the boot configuration + * needs to be preserved + * @node: Device tree node with the domain information. + * + * This function looks for "linux,pci-probe-only" property for a given + * PCIe controller's node and returns true if found. Having this property + * for a PCIe controller ensures that the kernel doesn't re-enumerate and + * reconfigure the BAR resources that are already done by the platform firmware. + * NOTE: The scope of "linux,pci-probe-only" defined within a PCIe bridge device + * is limited to the hierarchy under that particular bridge device. whereas + * the scope of "linux,pci-probe-only" defined within chosen node is + * system wide. + * + * Return: true if the property exists false otherwise. + */ +bool of_pci_bridge_check_probe_only(struct device_node *node) +{ + return of_property_read_bool(node, "linux,pci-probe-only"); +} +EXPORT_SYMBOL_GPL(of_pci_bridge_check_probe_only); + /** * devm_of_pci_get_host_bridge_resources() - Resource-managed parsing of PCI * host bridge resources from DT diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 795534589b98..d62d1f151ba9 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -15,6 +15,7 @@ #include <linux/cpumask.h> #include <linux/aer.h> #include <linux/acpi.h> +#include <linux/pci-acpi.h> #include <linux/hypervisor.h> #include <linux/irqdomain.h> #include <linux/pm_runtime.h> @@ -877,6 +878,28 @@ static void pci_set_bus_msi_domain(struct pci_bus *bus) dev_set_msi_domain(&bus->dev, d); } +static void pci_check_config_preserve(struct pci_host_bridge *host_bridge) +{ + if (&host_bridge->dev) { + union acpi_object *obj; + + /* + * Evaluate the "PCI Boot Configuration" _DSM Function. If it + * exists and returns 0, we must preserve any PCI resource + * assignments made by firmware for this host bridge. + */ + obj = acpi_evaluate_dsm(ACPI_HANDLE(&host_bridge->dev), &pci_acpi_dsm_guid, 1, + DSM_PCI_PRESERVE_BOOT_CONFIG, NULL); + if (obj && obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 0) + host_bridge->preserve_config = 1; + ACPI_FREE(obj); + } + + if (host_bridge->dev.parent && host_bridge->dev.parent->of_node) + host_bridge->preserve_config = + of_pci_bridge_check_probe_only(host_bridge->dev.parent->of_node); +} + static int pci_register_host_bridge(struct pci_host_bridge *bridge) { struct device *parent = bridge->dev.parent; @@ -971,6 +994,9 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge) if (nr_node_ids > 1 && pcibus_to_node(bus) == NUMA_NO_NODE) dev_warn(&bus->dev, "Unknown NUMA node; performance will be reduced\n"); + /* Check if the boot configuration by FW needs to be preserved */ + pci_check_config_preserve(bridge); + /* Coalesce contiguous windows */ resource_list_for_each_entry_safe(window, n, &resources) { if (list_is_last(&window->node, &resources)) @@ -3080,20 +3106,18 @@ int pci_host_probe(struct pci_host_bridge *bridge) bus = bridge->bus; + /* If we must preserve the resource configuration, claim now */ + if (pci_has_flag(PCI_PROBE_ONLY) || bridge->preserve_config) + pci_bus_claim_resources(bus); + /* - * We insert PCI resources into the iomem_resource and - * ioport_resource trees in either pci_bus_claim_resources() - * or pci_bus_assign_resources(). + * Assign whatever was left unassigned. If we didn't claim above, + * this will reassign everything. */ - if (pci_has_flag(PCI_PROBE_ONLY)) { - pci_bus_claim_resources(bus); - } else { - pci_bus_size_bridges(bus); - pci_bus_assign_resources(bus); + pci_assign_unassigned_root_bus_resources(bus); - list_for_each_entry(child, &bus->children, node) - pcie_bus_configure_settings(child); - } + list_for_each_entry(child, &bus->children, node) + pcie_bus_configure_settings(child); pci_bus_add_devices(bus); return 0; diff --git a/include/linux/of_pci.h b/include/linux/of_pci.h index 29658c0ee71f..9e045de3be44 100644 --- a/include/linux/of_pci.h +++ b/include/linux/of_pci.h @@ -13,6 +13,7 @@ struct device_node *of_pci_find_child_device(struct device_node *parent, unsigned int devfn); int of_pci_get_devfn(struct device_node *np); void of_pci_check_probe_only(void); +bool of_pci_bridge_check_probe_only(struct device_node *node); #else static inline struct device_node *of_pci_find_child_device(struct device_node *parent, unsigned int devfn) @@ -26,6 +27,11 @@ static inline int of_pci_get_devfn(struct device_node *np) } static inline void of_pci_check_probe_only(void) { } + +static inline bool of_pci_bridge_check_probe_only(struct device_node *node) +{ + return false; +} #endif #if IS_ENABLED(CONFIG_OF_IRQ)
Add support for preserving the boot configuration done by the platform firmware per host bridge basis, based on the presence of 'linux,pci-probe-only' property in the respective PCIe host bridge device-tree node. It also unifies the ACPI and DT based boot flows in this regard. Signed-off-by: Vidya Sagar <vidyas@nvidia.com> --- V3: * Unified ACPI and DT flows as part of addressing Bjorn's review comments V2: * Addressed issues reported by kernel test robot <lkp@intel.com> drivers/acpi/pci_root.c | 12 ------- drivers/pci/controller/pci-host-common.c | 4 --- drivers/pci/of.c | 22 ++++++++++++ drivers/pci/probe.c | 46 ++++++++++++++++++------ include/linux/of_pci.h | 6 ++++ 5 files changed, 63 insertions(+), 27 deletions(-)