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

[Tushar Dave <tdave@nvidia.com>]

On 5/7/25 17:21, Tushar Dave wrote:
> 
> 
> 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(-)
> 

Yi Liu,

Should I send v4 with the change below or we are good with v3?

-Tushar

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