Re: [PATCH V3] PCI: Add support for preserving boot configuration

[Vidya Sagar <vidyas@nvidia.com>]

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);
>> +     }