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

[Jason Gunthorpe <jgg@nvidia.com>]

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?

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

(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 DTE code looks OK

Jason