Re: [PATCH v3 1/7] iommu: Prevent pasid attach if no ops->remove_dev_pasid

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

On 2024/11/7 17:33, Tian, Kevin wrote:
>> From: Jason Gunthorpe <jgg@nvidia.com>
>> Sent: Tuesday, November 5, 2024 11:42 PM
>>
>> On Mon, Nov 04, 2024 at 05:20:27AM -0800, Yi Liu wrote:
>>> driver should implement both set_dev_pasid and remove_dev_pasid op,
>> otherwise
>>> it is a problem how to detach pasid. In reality, it is impossible that an
>>> iommu driver implements set_dev_pasid() but no remove_dev_pasid() op.
>> However,
>>> it is better to check it.
>>>
>>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>>> ---
>>>   drivers/iommu/iommu.c | 4 +++-
>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> I was wondering if we really needed this, but it does make the patches
>> a bit easier to understand
>>
>> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
>>
> 
> me too. btw when Vasant's series introduces the PASID flag to
> domain above checks can be moved to the domain alloc time
> then here just needs to check whether the domain allows
> pasid attach.

hmmm. Are we sure all the caller of iommu_attach_device_pasid() will
allocate domain with this flag? Besides iommufd, idxd driver would
also call iommu_attach_device_pasid(), and it uses the DMA domain of
the device. I suppose this domain is allocated with the pasid flag.

Given that Vasant's series has been queued and this series is based
on that. It might make sense to drop this patch. If the pasid capable
domain can be allocated successfully, the iommu driver already indicates
it has the ability to support set_dev_pasid and the undo op.

> for now,
> 
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> 
> and a nit - there is another reference to dev_iommu_ops in this
> function:
> 
> 	if (!dev_has_iommu(dev) || dev_iommu_ops(dev) != domain->owner ||
> 
> could be replaced with ops too.

If we decide to keep this patch, I'll add it.

-- 
Regards,
Yi Liu