Re: [PATCH v1 02/20] iommu: Introduce a test_dev domain op and an internal helper

Niklas Schnelle
2025-10-13T09:54:40Z

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 <jgg@nvidia.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
> 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 <schnelle@linux.ibm.com>