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

[Vasant Hegde <vasant.hegde@amd.com>]

On 5/2/2025 2:30 AM, Tushar Dave wrote:
> 
> 
> 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.

Looks good.

-Vasant