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

[Niklas Schnelle <schnelle@linux.ibm.com>]

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>