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

[Yi Liu <yi.l.liu@intel.com>]

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()?


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

-- 
Regards,
Yi Liu