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

[Tushar Dave <tdave@nvidia.com>]

On 5/7/25 06:59, Yi Liu wrote:
> 
> On 2025/5/6 05:15, 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 v3:
>> - addressed review comment from Vasant.
>>
>>   drivers/iommu/iommu.c | 27 +++++++++++++++++++--------
>>   1 file changed, 19 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 60aed01e54f2..636fc68a8ec0 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -3329,10 +3329,12 @@ static int __iommu_set_group_pasid(struct iommu_domain 
>> *domain,
>>       int ret;
>>       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;
>> +        }
>>       }
>>       return 0;
>> @@ -3342,7 +3344,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);
> 
> Reviewed-by: Yi Liu <yi.l.liu@intel.com>
> 
> with a nit. would it save some loc by adding the max_pasids check in
> iommu_remove_dev_pasid()?

With current code:

  drivers/iommu/iommu.c | 27 +++++++++++++++++++--------
  1 file changed, 19 insertions(+), 8 deletions(-)


If I move the pasid check in iommu_remove_dev_pasid(), it would be:

  drivers/iommu/iommu.c | 23 ++++++++++++++++-------
  1 file changed, 16 insertions(+), 7 deletions(-)


e.g.

@@ -3318,8 +3318,9 @@ static void iommu_remove_dev_pasid(struct device *dev, 
ioasid_t pasid,
         const struct iommu_ops *ops = dev_iommu_ops(dev);
         struct iommu_domain *blocked_domain = ops->blocked_domain;

-       WARN_ON(blocked_domain->ops->set_dev_pasid(blocked_domain,
-                                                  dev, pasid, domain));
+       if (dev->iommu->max_pasids > 0)
+               WARN_ON(blocked_domain->ops->set_dev_pasid(blocked_domain,
+                                                          dev, pasid, domain));
  }

  static int __iommu_set_group_pasid(struct iommu_domain *domain,
@@ -3329,10 +3330,12 @@ static int __iommu_set_group_pasid(struct iommu_domain 
*domain,
         int ret;

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

         return 0;

Last hunk remain same as before for iommu_attach_device_pasid()


Let me know.

-Tushar


> 
> 
>>       }
>>       return ret;
>>   }
>> @@ -3353,8 +3356,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 +3396,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;
>>           }
>