Re: [PATCH v7 06/10] iommu/amd: Modify set_dte_entry() to use 256-bit DTE helpers

["Suthikulpanit, Suravee" <suravee.suthikulpanit@amd.com>]

On 10/31/2024 7:53 PM, Jason Gunthorpe wrote:
> On Thu, Oct 31, 2024 at 09:16:20AM +0000, Suravee Suthikulpanit wrote:
>> @@ -1148,6 +1146,28 @@ static bool copy_device_table(void)
>>   	return true;
>>   }
>>   
>> +struct dev_table_entry *amd_iommu_get_ivhd_dte_flags(u16 devid)
>> +{
>> +	u16 f = 0, l = 0xFFFF;
>> +	struct ivhd_dte_flags *e;
>> +	struct dev_table_entry *dte = NULL;
>> +
>> +	for_each_ivhd_dte_flags(e) {
> 
> Maybe the list head should be on the iommu? I don't know how long it
> would be if it is worthwhile.
> 
>> +		/*
>> +		 * Need to go through the whole list to find the smallest range,
>> +		 * which contains the devid. Then store it in f and l variables.
>> +		 */
>> +		if ((e->devid_first >= devid) && (e->devid_last <= devid)) {
>> +			if (f < e->devid_first)
>> +				f = e->devid_first;
>> +			if (e->devid_last < l)
>> +				l = e->devid_last;
>> +			dte = &(e->dte);
>> +		}
> 
> f and l are never used, why calculate them?
> 
> Isn't (e->devid_first >= devid) not the right way to check if devid
> falls within a range?

Actually, I missed one line. I intended to do:

+         if ((e->devid_first >= devid) && (e->devid_last <= devid) &&
+             (f <= e->devid_first) && (e->devid_last <= l)) {

The IVHD device entry can be defined for a range of devices (range 
entry) or for a selected device (dev entry). So, a particular devid 
could fall into both a range entry and a dev entry. For example:

AMD-Vi: device: 0000:00:00.2 cap: 0040 flags: 30 info 0000
AMD-Vi:   DEV_SELECT_RANGE_START       devid: 0000:00:00.3 flags: 0x0
AMD-Vi:   DEV_RANGE_END                devid: 0000:1f:1f.6
AMD-Vi:   DEV_SPECIAL(HPET[0])         devid: 0000:00:14.0, flags: 0x0
AMD-Vi:   DEV_SPECIAL(IOAPIC[128])     devid: 0000:00:14.0, flags: 0xd7
AMD-Vi:   DEV_SPECIAL(IOAPIC[242])     devid: 0000:00:00.1, flags: 0x0
AMD-Vi:   DEV_ACPI_HID(AMDI0020[ID00]) devid: 0000:00:14.5, flags: 0x40
AMD-Vi:   DEV_ACPI_HID(AMDI0020[ID01]) devid: 0000:00:14.5, flags: 0x40
AMD-Vi:   DEV_ACPI_HID(AMDI0020[ID02]) devid: 0000:00:14.5, flags: 0x40
AMD-Vi:   DEV_ACPI_HID(AMDI0020[ID03]) devid: 0000:00:14.5, flags: 0x40
AMD-Vi:   DEV_ACPI_HID(AMDI0095[ID00]) devid: 0000:00:00.4, flags: 0x30

Note that the logic only store the entry w/ flags != 0.

For devid 0000:00:14.0, we want to get the dev entry

     DEV_SPECIAL(IOAPIC[128])     devid: 0000:00:14.0, flags: 0xd7

> Based on the comment it seems like you want something like this??
> 
> struct dev_table_entry *amd_iommu_get_ivhd_dte_flags(u16 devid)
> {
> 	struct dev_table_entry *dte = NULL;
> 	unsigned int best_len = UINT_MAX;
> 	struct ivhd_dte_flags *e;
> 
> 	for_each_ivhd_dte_flags(e) {
> 		/*
> 		 * Need to go through the whole list to find the smallest range,
> 		 * which contains the devid. Then store it in f and l variables.
> 		 */
> 		if ((e->devid_first <= devid) && (e->devid_last >= devid)) {
> 			unsigned int len = e->devid_last - e->devid_first;
> 
> 			if (len < best_len) {
> 				dte = &(e->dte);
> 				best_len = len;
> 			}
> 		}
> 	}
> 	return dte;
> }

This logic would work also. Thanks.

> (and it would be smart to sort the linked list by size, but again I
> don't know how big it is if it is worthwile complexity)

The list should not be too long (only a few entries). So, I think it 
would be okay to just keep one list for the whole system.

However, we would need to also add logic to check PCI segment ID, since 
the device table is per PCI segment.

I will update and send out V8.

Thanks,
Suravee