On Sun, 2025-10-12 at 17:04 -0700, Nicolin Chen wrote: > Add a new test_dev domain op for driver to test the compatibility between > a domain and a device at the driver level, before calling into the actual > attachment/replacement of a domain. Support pasid for set_dev_pasid call. > > Move existing core-level compatibility tests to a helper function. Invoke > it prior to: > * __iommu_attach_device() or its wrapper __iommu_device_set_domain() > * __iommu_set_group_pasid()
Should this list also include iommu_deferred_attach()? The code does include it. > > And keep them within the group->mutex, so drivers can simply move all the > sanity and compatibility tests from their attach_dev callbacks to the new > test_dev callbacks without concerning about a race condition. > > This may be a public API someday for VFIO/IOMMUFD to run a list of attach > tests without doing any actual attachment, which may result in a list of > failed tests. So encourage drivers to avoid printks to prevent kernel log > spam. > > Suggested-by: Jason Gunthorpe <[email protected]> > Signed-off-by: Nicolin Chen <[email protected]> > --- > include/linux/iommu.h | 17 +++++-- > drivers/iommu/iommu.c | 111 ++++++++++++++++++++++++++++++------------ > 2 files changed, 93 insertions(+), 35 deletions(-) > > diff --git a/include/linux/iommu.h b/include/linux/iommu.h > index 801b2bd9e8d49..2ec99502dc29c 100644 > --- a/include/linux/iommu.h > +++ b/include/linux/iommu.h > @@ -714,7 +714,12 @@ struct iommu_ops { > > /** > * struct iommu_domain_ops - domain specific operations > - * @attach_dev: attach an iommu domain to a device > + * @test_dev: Test compatibility prior to an @attach_dev or @set_dev_pasid > call. > + * A driver-level callback of this op should do a thorough > sanity, to You're missing the word "check" above. > + * make sure a device is compatible with the domain. So the > following > + * @attach_dev and @set_dev_pasid functions would likely succeed > with > + * only one exception due to a temporary failure like out of > memory. Nit: "… only one exception …" / "… like out of memory …" this sounds a bit odd to me because on the one hand it's one exception but then also a group (temporary failures). Maybe better: "… would likely succeed with only the exception of temporary failures like out of memory."? > + * It's suggested to avoid the kernel prints in this op. > * Return: > * * 0 - success > * * EINVAL - can indicate that device and domain are incompatible due to > @@ -722,11 +727,15 @@ struct iommu_ops { > * driver shouldn't log an error, since it is legitimate for a > * caller to test reuse of existing domains. Otherwise, it may > * still represent some other fundamental problem > - * * ENOMEM - out of memory > - * * ENOSPC - non-ENOMEM type of resource allocation failures > * * EBUSY - device is attached to a domain and cannot be changed > * * ENODEV - device specific errors, not able to be attached > * * <others> - treated as ENODEV by the caller. Use is discouraged > + * @attach_dev: attach an iommu domain to a device > + * Return: > + * * 0 - success > + * * ENOMEM - out of memory > + * * ENOSPC - non-ENOMEM type of resource allocation failures > + * * <others> - Use is discouraged > * @set_dev_pasid: set or replace an iommu domain to a pasid of device. The > pasid of > * the device should be left in the old config in error case. > * @map_pages: map a physically contiguous set of pages of the same size to > @@ -751,6 +760,8 @@ struct iommu_ops { > * @free: Release the domain after use. > */ > struct iommu_domain_ops { > + int (*test_dev)(struct iommu_domain *domain, struct device *dev, > + ioasid_t pasid, struct iommu_domain *old); > int (*attach_dev)(struct iommu_domain *domain, struct device *dev, > struct iommu_domain *old); > int (*set_dev_pasid)(struct iommu_domain *domain, struct device *dev, > --- snip --- > @@ -3615,6 +3657,11 @@ int iommu_replace_device_pasid(struct iommu_domain > *domain, > ret = 0; > > if (curr_domain != domain) { > + ret = __iommu_domain_test_device(domain, dev, pasid, > + curr_domain); > + if (ret) > + goto out_unlock; > + > ret = __iommu_set_group_pasid(domain, group, > pasid, curr_domain); > if (ret) Apart from the comment and commit description nits mentioned above this looks good to me. Reviewed-by: Niklas Schnelle <[email protected]>
