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

[Tushar Dave]

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@xxxxxxxxxxxxxxx
> > Signed-off-by: Tushar Dave <tdave@xxxxxxxxxx>
> > ---
> >
> > 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;
> >   		}