linux-kernel - Re: [PATCH 1/3] PCI: Call PCI ACPI _DSM with consistent revision argument

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <73628abc-dbb8-4dac-b83d-a78462b327b8@amd.com>
Date:   Tue, 5 Dec 2023 14:12:54 -0600
From:   Mario Limonciello <mario.limonciello@....com>
To:     Bjorn Helgaas <helgaas@...nel.org>
Cc:     Bjorn Helgaas <bhelgaas@...gle.com>,
        "Rafael J . Wysocki" <rjw@...ysocki.net>,
        "open list:PCI SUBSYSTEM" <linux-pci@...r.kernel.org>,
        linux-acpi@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] PCI: Call PCI ACPI _DSM with consistent revision
 argument

On 11/30/2023 16:29, Bjorn Helgaas wrote:
> On Fri, Nov 10, 2023 at 12:55:01PM -0600, Mario Limonciello wrote:
>> The PCI ACPI _DSM is called across multiple places in the PCI core
>> with different arguments for the revision.
>>
>> The PCI firmware specification specifies that this is an incorrect
>> behavior.
>> "OSPM must invoke all Functions other than Function 0 with the
>>   same Revision ID value"
> 
> This patch passes the same or a larger Revision ID than before, so
> everything should work the same because the spec requires backwards
> compatibility.  But it's conceivable that it could break on firmware
> that does the revision check incorrectly.
> 
> Is this fixing a problem other than the spec compliance issue?

It was just a spec compliance issue I noticed when implementing the 
other two patches.

> 
> I agree the PCI FW spec says this.  It was added in r3.3 by the ECN at
> https://members.pcisig.com/wg/Firmware/document/previewpdf/13988, but
> I don't quite understand that ECN.
> 
> ACPI r6.5, sec 9.1.1, doesn't include the "must invoke all Functions
> with same Revision ID" language, and the ASL example there clearly
> treats revisions higher than those implemented by firmware as valid,
> although new Functions added by those higher revisions are obviously
> not supported.
> 
> PCI FW also says OSPM should not use a fixed Revision ID, but should
> start with the highest known revision and "successively invoke
> Function 0 with decremented values of Revision ID until system
> firmware returns a value indicating support for more than Function 0"
> (added by the same ECN), and I don't think Linux does this part.
> 
> So I think the fixed "pci_acpi_dsm_rev" value as in your patch works
> fine with the ACPI ASL example, but it doesn't track the "successively
> decrement" part of PCI FW.  I don't know the reason for that part of
> the ECN.
> 

Do you think it's better to respin to take this into account and be more 
stringent or "do nothing"?

> Unrelated to this patch, I think it's a bug that Linux fails to invoke
> Function 0 in a few cases, e.g., DSM_PCI_PRESERVE_BOOT_CONFIG,
> DSM_PCI_POWER_ON_RESET_DELAY, and DSM_PCI_DEVICE_READINESS_DURATIONS.
> 
> Per spec, OSPM must invoke Function 0 to learn which other Functions
> are supported.  It's not explicitly stated, but I think this is
> required because a supported non-zero Function may return "any data
> object", so there's no return value that would mean "this Function
> Index is not supported."
> 

> Bjorn

What are your thoughts on the other two patches in the series?
Should they wait for a consumer or prepare the API to match the spec.

> 
>> Link: https://members.pcisig.com/wg/PCI-SIG/document/15350
>>        PCI Firmware specification 3.3, section 4.6
>> Signed-off-by: Mario Limonciello <mario.limonciello@....com>
>> ---
>>   drivers/acpi/pci_root.c  |  3 ++-
>>   drivers/pci/pci-acpi.c   |  6 ++++--
>>   drivers/pci/pci-label.c  |  4 ++--
>>   drivers/pci/pcie/edr.c   | 13 +++++++------
>>   include/linux/pci-acpi.h |  1 +
>>   5 files changed, 16 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
>> index 58b89b8d950e..bca2270a93d4 100644
>> --- a/drivers/acpi/pci_root.c
>> +++ b/drivers/acpi/pci_root.c
>> @@ -1055,7 +1055,8 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
>>   	 * exists and returns 0, we must preserve any PCI resource
>>   	 * assignments made by firmware for this host bridge.
>>   	 */
>> -	obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(bus->bridge), &pci_acpi_dsm_guid, 1,
>> +	obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(bus->bridge),
>> +				      &pci_acpi_dsm_guid, pci_acpi_dsm_rev,
>>   				      DSM_PCI_PRESERVE_BOOT_CONFIG, NULL, ACPI_TYPE_INTEGER);
>>   	if (obj && obj->integer.value == 0)
>>   		host_bridge->preserve_config = 1;
>> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
>> index 004575091596..bea72e807817 100644
>> --- a/drivers/pci/pci-acpi.c
>> +++ b/drivers/pci/pci-acpi.c
>> @@ -28,6 +28,7 @@
>>   const guid_t pci_acpi_dsm_guid =
>>   	GUID_INIT(0xe5c937d0, 0x3553, 0x4d7a,
>>   		  0x91, 0x17, 0xea, 0x4d, 0x19, 0xc3, 0x43, 0x4d);
>> +const int pci_acpi_dsm_rev = 5;
>>
>>   #if defined(CONFIG_PCI_QUIRKS) && defined(CONFIG_ARM64)
>>   static int acpi_get_rc_addr(struct acpi_device *adev, struct resource *res)
>> @@ -1215,7 +1216,8 @@ void acpi_pci_add_bus(struct pci_bus *bus)
>>   	if (!pci_is_root_bus(bus))
>>   		return;
>>   
>> -	obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(bus->bridge), &pci_acpi_dsm_guid, 3,
>> +	obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(bus->bridge),
>> +				      &pci_acpi_dsm_guid, pci_acpi_dsm_rev,
>>   				      DSM_PCI_POWER_ON_RESET_DELAY, NULL, ACPI_TYPE_INTEGER);
>>   	if (!obj)
>>   		return;
>> @@ -1376,7 +1378,7 @@ static void pci_acpi_optimize_delay(struct pci_dev *pdev,
>>   	if (bridge->ignore_reset_delay)
>>   		pdev->d3cold_delay = 0;
>>   
>> -	obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, 3,
>> +	obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, pci_acpi_dsm_rev,
>>   				      DSM_PCI_DEVICE_READINESS_DURATIONS, NULL,
>>   				      ACPI_TYPE_PACKAGE);
>>   	if (!obj)
>> diff --git a/drivers/pci/pci-label.c b/drivers/pci/pci-label.c
>> index 0c6446519640..91bdd04029f0 100644
>> --- a/drivers/pci/pci-label.c
>> +++ b/drivers/pci/pci-label.c
>> @@ -41,7 +41,7 @@ static bool device_has_acpi_name(struct device *dev)
>>   	if (!handle)
>>   		return false;
>>   
>> -	return acpi_check_dsm(handle, &pci_acpi_dsm_guid, 0x2,
>> +	return acpi_check_dsm(handle, &pci_acpi_dsm_guid, pci_acpi_dsm_rev,
>>   			      1 << DSM_PCI_DEVICE_NAME);
>>   #else
>>   	return false;
>> @@ -162,7 +162,7 @@ static int dsm_get_label(struct device *dev, char *buf,
>>   	if (!handle)
>>   		return -1;
>>   
>> -	obj = acpi_evaluate_dsm(handle, &pci_acpi_dsm_guid, 0x2,
>> +	obj = acpi_evaluate_dsm(handle, &pci_acpi_dsm_guid, pci_acpi_dsm_rev,
>>   				DSM_PCI_DEVICE_NAME, NULL);
>>   	if (!obj)
>>   		return -1;
>> diff --git a/drivers/pci/pcie/edr.c b/drivers/pci/pcie/edr.c
>> index 5f4914d313a1..ab6a50201124 100644
>> --- a/drivers/pci/pcie/edr.c
>> +++ b/drivers/pci/pcie/edr.c
>> @@ -35,7 +35,7 @@ static int acpi_enable_dpc(struct pci_dev *pdev)
>>   	 * Behavior when calling unsupported _DSM functions is undefined,
>>   	 * so check whether EDR_PORT_DPC_ENABLE_DSM is supported.
>>   	 */
>> -	if (!acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid, 5,
>> +	if (!acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid, pci_acpi_dsm_rev,
>>   			    1ULL << EDR_PORT_DPC_ENABLE_DSM))
>>   		return 0;
>>   
>> @@ -51,8 +51,9 @@ static int acpi_enable_dpc(struct pci_dev *pdev)
>>   	 * Firmware Specification r3.2, sec 4.6.12, EDR_PORT_DPC_ENABLE_DSM is
>>   	 * optional.  Return success if it's not implemented.
>>   	 */
>> -	obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid, 5,
>> -				EDR_PORT_DPC_ENABLE_DSM, &argv4);
>> +	obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid,
>> +				pci_acpi_dsm_rev, EDR_PORT_DPC_ENABLE_DSM,
>> +				&argv4);
>>   	if (!obj)
>>   		return 0;
>>   
>> @@ -88,12 +89,12 @@ static struct pci_dev *acpi_dpc_port_get(struct pci_dev *pdev)
>>   	 * Behavior when calling unsupported _DSM functions is undefined,
>>   	 * so check whether EDR_PORT_DPC_ENABLE_DSM is supported.
>>   	 */
>> -	if (!acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid, 5,
>> +	if (!acpi_check_dsm(adev->handle, &pci_acpi_dsm_guid, pci_acpi_dsm_rev,
>>   			    1ULL << EDR_PORT_LOCATE_DSM))
>>   		return pci_dev_get(pdev);
>>   
>> -	obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid, 5,
>> -				EDR_PORT_LOCATE_DSM, NULL);
>> +	obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid,
>> +				pci_acpi_dsm_rev, EDR_PORT_LOCATE_DSM, NULL);
>>   	if (!obj)
>>   		return pci_dev_get(pdev);
>>   
>> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
>> index 078225b514d4..7966ef8f14b3 100644
>> --- a/include/linux/pci-acpi.h
>> +++ b/include/linux/pci-acpi.h
>> @@ -115,6 +115,7 @@ static inline void acpiphp_check_host_bridge(struct acpi_device *adev) { }
>>   #endif
>>   
>>   extern const guid_t pci_acpi_dsm_guid;
>> +extern const int pci_acpi_dsm_rev;
>>   
>>   /* _DSM Definitions for PCI */
>>   #define DSM_PCI_PRESERVE_BOOT_CONFIG		0x05
>> -- 
>> 2.34.1
>>