Received-SPF: pass (google.com: domain of linux-kernel+bounces-77385-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <17a29a7e-b013-4568-8e21-9647969b6b6d@nvidia.com>
Date: Fri, 23 Feb 2024 02:48:24 +0530
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH V3] PCI: Add support for preserving boot configuration
Content-Language: en-US
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: bhelgaas@google.com, rafael@kernel.org, lenb@kernel.org, will@kernel.org,
 lpieralisi@kernel.org, kw@linux.com, robh@kernel.org,
 frowand.list@gmail.com, linux-pci@vger.kernel.org,
 linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org,
 linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org,
 treding@nvidia.com, jonathanh@nvidia.com, kthota@nvidia.com,
 mmaddireddy@nvidia.com, sagar.tv@gmail.com
References: <20240222170641.GA15593@bhelgaas>
From: Vidya Sagar <vidyas@nvidia.com>
In-Reply-To: <20240222170641.GA15593@bhelgaas>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?b2NFL1AxUVlPS0J2dTRiS3Aza2prUDBvQWtmZHN0bU4vZDBEckxHOGRmQnNu?=
 =?utf-8?B?MVdnNWpHb3I1VUpGQ0EwN3AyY09PbFVTTkZxc3JuV1ZvSkRUNC9pQmZpR0xl?=
 =?utf-8?B?Y24zNW0rNlFyVm9yT3prRld0Ly9NSHBva0R5TUthTDNyZ0lrYnFjOGJWNTV2?=
 =?utf-8?B?cXQyckt4d1NBUSt3NXkxUURqU0RUWDlFVmFwN1ZNL2UzRDJMOXI4YUhUb1JW?=
 =?utf-8?B?ajI1Vk5WMjJJNXdEbmhtbXhmTjFzbkU1T25JSzVCSGlkdDM4clFPdVF5M0ZN?=
 =?utf-8?B?Z0hsS2E5WGgvOGp5Ymp4NkJHMThxRTRSRmtWMWU2b0dMKzN3OStwakdHMmpj?=
 =?utf-8?B?Q3diZU10a0NRbkNlQlV2UGZDKzRMWjNtVVVrNlNRaVZUYlo5WGFxay8xeWtC?=
 =?utf-8?B?VFczTTljL1RyY1hTOC93RTJDMnNTeE5ycCtha2xwSVVIWmorK2hBdmcrbVF1?=
 =?utf-8?B?MitOQjB3ZjdBclQyaVRhbUdjbytCQWdsVERXcXkvZ1dNZy82VGFpNUFqTThM?=
 =?utf-8?B?V1ZQT2FVVkl4NzBqZzZTdEd1Mk01TnIwTTdHVGQ5WHhLcmZucWlwbHJnMWE4?=
 =?utf-8?B?TkZrQ2NsT2tmUlZ5ZldGOS9TNytiYzdnb01SbVNwRURROEpqU3lSQ2RwQ2Jl?=
 =?utf-8?B?YnphN1ovaTl6VHhXUFdHUFNuOVpwbEU4ekN0ejQ2TFVTazFmdm05ZzNFWlQ3?=
 =?utf-8?B?MENoZTAzZlFZamp1RWpOZ3FwdHpZZVNMaTRMbU9GNk51MzhnaE93azY2YVNr?=
 =?utf-8?B?Q1lFUnk5SjJ4cnZJNmg1MVZ0MFpsNnVRbFhuSWtQdGRub1ZyODlPV1NXTTZk?=
 =?utf-8?B?a1QzM0JENndPcXYrTkdHTktBNGZVcnpsQ0dZVGlBMURCVWZ0VU90cWE0c3Y5?=
 =?utf-8?B?RitNN0F0RHNlWnRNOUFmR280YTNxS05vQkVKdjcrTGdtOUFIdFptUEZwUjlJ?=
 =?utf-8?B?RExSc29YVWxiSFNoNXlaUDJJZ0FZYkd4dytqWHNaWFFsbEErd3JySjNiWEwv?=
 =?utf-8?B?d2J6RW03Q0hNdE9WUEtkQ2hHZ2JhQU9DbmFTK2tLV04zNW0vUUhSRVkrRHEw?=
 =?utf-8?B?VjY0RXM3bzlaNDRTa0RVQVBDVnNwOGRNRVFpMXJ4RGRZRTJGQVZRZTRrRUFX?=
 =?utf-8?B?UnljdVJYMzVQK1cvN1NMdzA3VEZyKzFVRjljaEVTSjFjNFlsZ2hGYmF2T0NN?=
 =?utf-8?B?V0NDd21LeHRUNGNqMHIreHFTU3k4ZUp5K2Jrb3pLSTQrblk1Z1FGdjI3TGlY?=
 =?utf-8?B?WmZ1WkI0alBrbHlsdkhOeVg1dmUrbnVHb1dmQWZWaC9zcUZHS0VHQ0RRSDBp?=
 =?utf-8?B?b25iWlcvWUVCN3BkSWtydFNOZm1VcEJYUlFxWDlMbmlHVEVXK0pDb05NYmZv?=
 =?utf-8?B?TFhiRng2MVlMQ0ZuMkdvNFRsOHVuRjdHcWlubFR1eFRuWDB0SVEvTndod0Fh?=
 =?utf-8?B?ak01N2FKczh5UHptTVQ5U2FKaWxubmxrL0xlbGhEODlaS1BWZE8yOWQzRUlC?=
 =?utf-8?B?YjlvSHdUOVpFZ0VxZ3dmTkhBRGtSUldUVU9qM2I5ZWwyMEE3TzhZaThKUkVD?=
 =?utf-8?B?QWVNNkpPSks1SXdYTS9TWkRPVGtXc1FIMU8xdTQ2QjlHS2dxcUkyY1ppNFVr?=
 =?utf-8?B?dWxhMmVrM0NNWCt6dDZYMkgxYU81TktmeXpVTHNTVzlwRU5mZmR3a3lnZE9p?=
 =?utf-8?B?TnZZQUVibE5BWmJtTnRqZzIwZ1Z0T3JaVzlkVGVHc1JCbVV5TEZvMUZ6SHJ3?=
 =?utf-8?B?ZUNQUU5sVGF0cnN0WDB6aCtyditMbEN3bTc5d1pIWEc4cDlPYVEyemorTzJU?=
 =?utf-8?B?SVYyRHQwYXpVZGUyVDJEc1N0MGJRRmZNYllHS3NiS2pZaWpvaGRJZzJpOTlO?=
 =?utf-8?B?dldtM2hmU0tOd0tLOTZVMEF5NTZhZ1dGQSszc3M1OXVTTVRKdDlnZnE1aGwr?=
 =?utf-8?B?T3liaG4yc0JpZzJWNVJhdWJGcUZvVmdxeG9yNzF1REpRNmdxWThnamdYNWx1?=
 =?utf-8?B?RzhwbDNSWFBsamdKaVBuTy9nd3ptL1Q2bnJBbmpYMURqZ0lpbW5RRGtVdDFy?=
 =?utf-8?B?SEswQ0tTNmpVdzZqVTRkeVhieWIzNWNhbll3QWw4RWJ3OTZkbmpWRjFWbjB1?=
 =?utf-8?Q?W5X0biPdj3yYJcaorayN2QuC/?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 0e84b121-2b3d-44a5-feff-08dc33ebd348
X-MS-Exchange-CrossTenant-AuthSource: PH8PR12MB6674.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 Feb 2024 21:18:33.8407
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: pe1wiy4LrfIkxSnKwkj49phmdtAo3pSUIL1x4W2FjDe7vFqwjZ7nAKw1Uwi91zCvvxmn8xJUDFR2cFHgRKfPAg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR12MB8074


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