[PATCH v2] iommu/amd: Allow matching ACPI HID devices without matching UIDs

[PATCH v2] iommu/amd: Allow matching ACPI HID devices without matching UIDs

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

A BIOS upgrade has changed the IVRS DTE UID for a device that no
longer matches the UID in the SSDT. In this case there is only
one ACPI device on the system with that _HID but the _UID mismatch.

IVRS:
```
[1E4h 0484 001h]               Subtable Type : F0 [Device Entry: ACPI HID Named Device]
[1E5h 0485 002h]                   Device ID : 0060
[1E7h 0487 001h] Data Setting (decoded below) : 40
                                    INITPass : 0
                                    EIntPass : 0
                                     NMIPass : 0
                                    Reserved : 0
                                 System MGMT : 0
                                  LINT0 Pass : 1
                                  LINT1 Pass : 0
[1E8h 0488 008h]                    ACPI HID : "MSFT0201"
[1F0h 0496 008h]                    ACPI CID : 0000000000000000
[1F8h 0504 001h]                  UID Format : 02
[1F9h 0505 001h]                  UID Length : 09
[1FAh 0506 009h]                         UID : "\_SB.MHSP"
```

SSDT:
```
Device (MHSP)
{
    Name (_ADR, Zero)  // _ADR: Address
    Name (_HID, "MSFT0201")  // _HID: Hardware ID
    Name (_UID, One)  // _UID: Unique ID
```

To handle this case; while enumerating ACPI devices in get_acpihid_device_id()
count the number of matching ACPI devices with a matching _HID. If there is
exactly one _HID match then accept it even if the UID doesn't match. Other
operating systems allow this, but the current IVRS spec doesn't explicitly
allow or disallow it. Output 'Firmware Bug' for this case to encourage it to
be solved in the BIOS.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v2:
 * Use FW_BUG
 * Update commit message
Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Cc: Vasant Hegde <Vasant.Hegde@amd.com>
Cc: Paul Blinzer <Paul.Blinzer@amd.com>
---
 drivers/iommu/amd/iommu.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index b19e8c0f48fa2..06f747d2f964c 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -109,7 +109,8 @@ static inline int get_acpihid_device_id(struct device *dev,
 					struct acpihid_map_entry **entry)
 {
 	struct acpi_device *adev = ACPI_COMPANION(dev);
-	struct acpihid_map_entry *p;
+	struct acpihid_map_entry *p, *p1;
+	bool fw_bug;
 
 	if (!adev)
 		return -ENODEV;
@@ -117,11 +118,26 @@ static inline int get_acpihid_device_id(struct device *dev,
 	list_for_each_entry(p, &acpihid_map, list) {
 		if (acpi_dev_hid_uid_match(adev, p->hid,
 					   p->uid[0] ? p->uid : NULL)) {
-			if (entry)
-				*entry = p;
-			return p->devid;
+			p1 = p;
+			fw_bug = false;
+			break;
+		}
+
+		/* In ase there is no UID match, but exactly one HID match */
+		if (acpi_dev_hid_match(adev, p->hid)) {
+			p1 = p;
+			fw_bug = true;
 		}
 	}
+
+	if (p1) {
+		if (fw_bug)
+			dev_err_once(dev, FW_BUG "No matching UID in ACPI tables, but found matching HID.\n");
+		if (entry)
+			*entry = p1;
+		return p1->devid;
+	}
+
 	return -EINVAL;
 }
 
-- 
2.43.0

Re: [PATCH v2] iommu/amd: Allow matching ACPI HID devices without matching UIDs

[Vasant Hegde]

Hi Mario,

On 5/5/2025 8:14 PM, Mario Limonciello wrote:
> From: Mario Limonciello <mario.limonciello@amd.com>
> 
> A BIOS upgrade has changed the IVRS DTE UID for a device that no
> longer matches the UID in the SSDT. In this case there is only
> one ACPI device on the system with that _HID but the _UID mismatch.
> 
> IVRS:
> ```
> [1E4h 0484 001h]               Subtable Type : F0 [Device Entry: ACPI HID Named Device]
> [1E5h 0485 002h]                   Device ID : 0060
> [1E7h 0487 001h] Data Setting (decoded below) : 40
>                                     INITPass : 0
>                                     EIntPass : 0
>                                      NMIPass : 0
>                                     Reserved : 0
>                                  System MGMT : 0
>                                   LINT0 Pass : 1
>                                   LINT1 Pass : 0
> [1E8h 0488 008h]                    ACPI HID : "MSFT0201"
> [1F0h 0496 008h]                    ACPI CID : 0000000000000000
> [1F8h 0504 001h]                  UID Format : 02
> [1F9h 0505 001h]                  UID Length : 09
> [1FAh 0506 009h]                         UID : "\_SB.MHSP"
> ```
> 
> SSDT:
> ```
> Device (MHSP)
> {
>     Name (_ADR, Zero)  // _ADR: Address
>     Name (_HID, "MSFT0201")  // _HID: Hardware ID
>     Name (_UID, One)  // _UID: Unique ID
> ```
> 
> To handle this case; while enumerating ACPI devices in get_acpihid_device_id()
> count the number of matching ACPI devices with a matching _HID. If there is
> exactly one _HID match then accept it even if the UID doesn't match. Other
> operating systems allow this, but the current IVRS spec doesn't explicitly
> allow or disallow it. Output 'Firmware Bug' for this case to encourage it to
> be solved in the BIOS.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> v2:
>  * Use FW_BUG
>  * Update commit message
> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Cc: Vasant Hegde <Vasant.Hegde@amd.com>
> Cc: Paul Blinzer <Paul.Blinzer@amd.com>
> ---
>  drivers/iommu/amd/iommu.c | 24 ++++++++++++++++++++----
>  1 file changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index b19e8c0f48fa2..06f747d2f964c 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -109,7 +109,8 @@ static inline int get_acpihid_device_id(struct device *dev,
>  					struct acpihid_map_entry **entry)
>  {
>  	struct acpi_device *adev = ACPI_COMPANION(dev);
> -	struct acpihid_map_entry *p;
> +	struct acpihid_map_entry *p, *p1;
> +	bool fw_bug;

Initialize these variables?

>  
>  	if (!adev)
>  		return -ENODEV;
> @@ -117,11 +118,26 @@ static inline int get_acpihid_device_id(struct device *dev,
>  	list_for_each_entry(p, &acpihid_map, list) {
>  		if (acpi_dev_hid_uid_match(adev, p->hid,
>  					   p->uid[0] ? p->uid : NULL)) {
> -			if (entry)
> -				*entry = p;
> -			return p->devid;
> +			p1 = p;
> +			fw_bug = false;
> +			break;
> +		}
> +
> +		/* In ase there is no UID match, but exactly one HID match */
> +		if (acpi_dev_hid_match(adev, p->hid)) {
> +			p1 = p;
> +			fw_bug = true;

I am not sure why you dropped counter from v1. I think we still need HID match
counter so that function returns -ENODEV if HID match counter > 1?

-Vasant

>  		}
>  	}
> +
> +	if (p1) {
> +		if (fw_bug)
> +			dev_err_once(dev, FW_BUG "No matching UID in ACPI tables, but found matching HID.\n");
> +		if (entry)
> +			*entry = p1;
> +		return p1->devid;
> +	}
> +
>  	return -EINVAL;
>  }
>  

Re: [PATCH v2] iommu/amd: Allow matching ACPI HID devices without matching UIDs

[Mario Limonciello]

On 5/12/2025 12:54 AM, Vasant Hegde wrote:
> Hi Mario,
> 
> On 5/5/2025 8:14 PM, Mario Limonciello wrote:
>> From: Mario Limonciello <mario.limonciello@amd.com>
>>
>> A BIOS upgrade has changed the IVRS DTE UID for a device that no
>> longer matches the UID in the SSDT. In this case there is only
>> one ACPI device on the system with that _HID but the _UID mismatch.
>>
>> IVRS:
>> ```
>> [1E4h 0484 001h]               Subtable Type : F0 [Device Entry: ACPI HID Named Device]
>> [1E5h 0485 002h]                   Device ID : 0060
>> [1E7h 0487 001h] Data Setting (decoded below) : 40
>>                                      INITPass : 0
>>                                      EIntPass : 0
>>                                       NMIPass : 0
>>                                      Reserved : 0
>>                                   System MGMT : 0
>>                                    LINT0 Pass : 1
>>                                    LINT1 Pass : 0
>> [1E8h 0488 008h]                    ACPI HID : "MSFT0201"
>> [1F0h 0496 008h]                    ACPI CID : 0000000000000000
>> [1F8h 0504 001h]                  UID Format : 02
>> [1F9h 0505 001h]                  UID Length : 09
>> [1FAh 0506 009h]                         UID : "\_SB.MHSP"
>> ```
>>
>> SSDT:
>> ```
>> Device (MHSP)
>> {
>>      Name (_ADR, Zero)  // _ADR: Address
>>      Name (_HID, "MSFT0201")  // _HID: Hardware ID
>>      Name (_UID, One)  // _UID: Unique ID
>> ```
>>
>> To handle this case; while enumerating ACPI devices in get_acpihid_device_id()
>> count the number of matching ACPI devices with a matching _HID. If there is
>> exactly one _HID match then accept it even if the UID doesn't match. Other
>> operating systems allow this, but the current IVRS spec doesn't explicitly
>> allow or disallow it. Output 'Firmware Bug' for this case to encourage it to
>> be solved in the BIOS.
>>
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>> v2:
>>   * Use FW_BUG
>>   * Update commit message
>> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> Cc: Vasant Hegde <Vasant.Hegde@amd.com>
>> Cc: Paul Blinzer <Paul.Blinzer@amd.com>
>> ---
>>   drivers/iommu/amd/iommu.c | 24 ++++++++++++++++++++----
>>   1 file changed, 20 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
>> index b19e8c0f48fa2..06f747d2f964c 100644
>> --- a/drivers/iommu/amd/iommu.c
>> +++ b/drivers/iommu/amd/iommu.c
>> @@ -109,7 +109,8 @@ static inline int get_acpihid_device_id(struct device *dev,
>>   					struct acpihid_map_entry **entry)
>>   {
>>   	struct acpi_device *adev = ACPI_COMPANION(dev);
>> -	struct acpihid_map_entry *p;
>> +	struct acpihid_map_entry *p, *p1;
>> +	bool fw_bug;
> 
> Initialize these variables?

Thanks - I'll make sure p1 is initialized to NULL.
> 
>>   
>>   	if (!adev)
>>   		return -ENODEV;
>> @@ -117,11 +118,26 @@ static inline int get_acpihid_device_id(struct device *dev,
>>   	list_for_each_entry(p, &acpihid_map, list) {
>>   		if (acpi_dev_hid_uid_match(adev, p->hid,
>>   					   p->uid[0] ? p->uid : NULL)) {
>> -			if (entry)
>> -				*entry = p;
>> -			return p->devid;
>> +			p1 = p;
>> +			fw_bug = false;
>> +			break;
>> +		}
>> +
>> +		/* In ase there is no UID match, but exactly one HID match */
>> +		if (acpi_dev_hid_match(adev, p->hid)) {
>> +			p1 = p;
>> +			fw_bug = true;
> 
> I am not sure why you dropped counter from v1. I think we still need HID match
> counter so that function returns -ENODEV if HID match counter > 1?
> 
> -Vasant

I was hoping to avoid more variables than necessary but be able to show 
a message.  Initially I'm tempted to add an extra case into the 
acpi_dev_hid_match() block:

if (acpi_dev_hid_match()) {
	if (fw_bug)
		return -ENODEV;
	fw_bug = true;
	p1 = p;
}

However this breaks if there are 3 HID devices because it could preclude 
a HID/UID match being the 3rd enumerated.  So I think you're right, for 
this specific case we need a counter as well to know whether to show 
fw_bug message and allow it.

I'll go back the counter used from v1.

> 
>>   		}
>>   	}
>> +
>> +	if (p1) {
>> +		if (fw_bug)
>> +			dev_err_once(dev, FW_BUG "No matching UID in ACPI tables, but found matching HID.\n");
>> +		if (entry)
>> +			*entry = p1;
>> +		return p1->devid;
>> +	}
>> +
>>   	return -EINVAL;
>>   }
>>   
>