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

[Mario Limonciello <superm1@kernel.org>]

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