Re: [PATCH v2 rc] iommu: Skip PASID validation for devices without PASID capability

[Tushar Dave <tdave@nvidia.com>]

On 5/1/25 03:58, Vasant Hegde wrote:
> On 4/30/2025 8:24 AM, Tushar Dave wrote:
>> Generally PASID support requires ACS settings that usually create
>> single device groups, but there are some niche cases where we can get
>> multi-device groups and still have working PASID support. The primary
>> issue is that PCI switches are not required to treat PASID tagged TLPs
>> specially so appropriate ACS settings are required to route all TLPs to
>> the host bridge if PASID is going to work properly.
>>
>> pci_enable_pasid() does check that each device that will use PASID has
>> the proper ACS settings to achieve this routing.
>>
>> However, no-PASID devices can be combined with PASID capable devices
>> within the same topology using non-uniform ACS settings. In this case
>> the no-PASID devices may not have strict route to host ACS flags and
>> end up being grouped with the PASID devices.
>>
>> This configuration fails to allow use of the PASID within the iommu
>> core code which wrongly checks if the no-PASID device supports PASID.
>>
>> Fix this by ignoring no-PASID devices during the PASID validation. They
>> will never issue a PASID TLP anyhow so they can be ignored.
>>
>> Fixes: c404f55c26fc ("iommu: Validate the PASID in iommu_attach_device_pasid()")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Tushar Dave <tdave@nvidia.com>
>> ---
>>
>> changes in v2:
>> - added no-pasid check in __iommu_set_group_pasid and __iommu_remove_group_pasid
>>
>>   drivers/iommu/iommu.c | 22 ++++++++++++++++------
>>   1 file changed, 16 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 60aed01e54f2..8251b07f4022 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -3329,8 +3329,9 @@ static int __iommu_set_group_pasid(struct iommu_domain *domain,
>>   	int ret;
> 
> initialize ret to zero?

Thanks Vasant.

How about:

         for_each_group_device(group, device) {
-               ret = domain->ops->set_dev_pasid(domain, device->dev,
-                                                pasid, NULL);
-               if (ret)
-                       goto err_revert;
+               if (device->dev->iommu->max_pasids > 0) {
+                       ret = domain->ops->set_dev_pasid(domain, device->dev,
+                                                        pasid, NULL);
+                       if (ret)
+                               goto err_revert;
+               }
         }

Let me know.

-Tushar

> 
> -Vasant
> 
>>   
>>   	for_each_group_device(group, device) {
>> -		ret = domain->ops->set_dev_pasid(domain, device->dev,
>> -						 pasid, NULL);
>> +		if (device->dev->iommu->max_pasids > 0)
>> +			ret = domain->ops->set_dev_pasid(domain, device->dev,
>> +							 pasid, NULL);
>>   		if (ret)
>>   			goto err_revert;
>>   	}
>> @@ -3342,7 +3343,8 @@ static int __iommu_set_group_pasid(struct iommu_domain *domain,
>>   	for_each_group_device(group, device) {
>>   		if (device == last_gdev)
>>   			break;
>> -		iommu_remove_dev_pasid(device->dev, pasid, domain);
>> +		if (device->dev->iommu->max_pasids > 0)
>> +			iommu_remove_dev_pasid(device->dev, pasid, domain);
>>   	}
>>   	return ret;
>>   }
>> @@ -3353,8 +3355,10 @@ static void __iommu_remove_group_pasid(struct iommu_group *group,
>>   {
>>   	struct group_device *device;
>>   
>> -	for_each_group_device(group, device)
>> -		iommu_remove_dev_pasid(device->dev, pasid, domain);
>> +	for_each_group_device(group, device) {
>> +		if (device->dev->iommu->max_pasids > 0)
>> +			iommu_remove_dev_pasid(device->dev, pasid, domain);
>> +	}
>>   }
>>   
>>   /*
>> @@ -3391,7 +3395,13 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
>>   
>>   	mutex_lock(&group->mutex);
>>   	for_each_group_device(group, device) {
>> -		if (pasid >= device->dev->iommu->max_pasids) {
>> +		/*
>> +		 * Skip PASID validation for devices without PASID support
>> +		 * (max_pasids = 0). These devices cannot issue transactions
>> +		 * with PASID, so they don't affect group's PASID usage.
>> +		 */
>> +		if ((device->dev->iommu->max_pasids > 0) &&
>> +		    (pasid >= device->dev->iommu->max_pasids)) {
>>   			ret = -EINVAL;
>>   			goto out_unlock;
>>   		}
>