Skip to contentSkip to conversation

iommu: Introduce and roll out test_dev domain op

PATCH In review Nicolin Chen 4 participants 43 messages Oct 13, 2025 – Nov 7, 2025

Subsystems (21)
lore
Share & tools
lore
#0 lore raw

cover iommu: Introduce and roll out test_dev domain op

Add a new test_dev domain op for drivers to run a compatibility test prior to the actual attachment at the driver level. Any incompatible attachment will be rejected early, allowing the iommu core to postpone any concurrent attachment during a device reset state.

This is a preparatory series for new iommu_dev_reset APIs:
https://lore.kernel.org/all/[email protected]/

The change is a must to keep attach uAPI behavior consistent.

This is on github:
https://github.com/nicolinc/iommufd/tree/iommu_test_dev-v1

Nicolin Chen (20):
iommu: Lock group->mutex in iommu_deferred_attach()
iommu: Introduce a test_dev domain op and an internal helper iommu/arm-smmu-v3: Implement arm_smmu_domain_test_dev iommu/intel: Implement test_dev callbacks to domain ops iommu/amd: Implement test_dev callbacks to domain ops iommu/arm-smmu: Implement arm_smmu_test_dev iommu/qcom_iommu: Implement test_dev callbacks to domain ops iommu/riscv: Implement riscv_iommu_test_paging_domain iommu/mkt_iommu: Implement mtk_iommu_test_device iommu/apple-dart: Implement test_dev callbacks to domain ops iommu/ipmmu-vmsa: Implement ipmmu_domain_test_device iommu/sun50i-iommu: Implement sun50i_iommu_domain_test_device iommu/rockchip-iommu: Implement rk_iommu_identity_test_dev iommu/msm_iommu: Implement msm_iommu_domain_test_dev iommu/fsl_pamu_domain: Implement fsl_pamu_domain_test_device iommu/omap-iommu: Implement omap_iommu_domain_test_dev iommu/s390-iommu: Implement s390_iommu_domain_test_device iommufd/selftest: Implement mock_domain_nop_test iommu/virtio-iommu: Implement viommu_domain_test_dev iommu/tegra-smmu: Implement tegra_smmu_domain_test_dev

 drivers/iommu/amd/amd_iommu.h                 |   3 -
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |   2 +
 include/linux/iommu.h                         |  17 ++-
 drivers/iommu/amd/iommu.c                     |  27 ++--
 drivers/iommu/amd/pasid.c                     |  29 ++--
 drivers/iommu/apple-dart.c                    |  50 +++++--
 .../arm/arm-smmu-v3/arm-smmu-v3-iommufd.c     |   6 +-
 .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c   |   4 +-
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 113 ++++++++++------
 drivers/iommu/arm/arm-smmu/arm-smmu.c         | 116 +++++++++-------
 drivers/iommu/arm/arm-smmu/qcom_iommu.c       |  46 +++++--
 drivers/iommu/fsl_pamu_domain.c               |  50 +++++--
 drivers/iommu/intel/iommu.c                   |  66 ++++++----
 drivers/iommu/intel/nested.c                  |  29 ++--
 drivers/iommu/intel/svm.c                     |  11 +-
 drivers/iommu/iommu.c                         | 124 +++++++++++++-----
 drivers/iommu/iommufd/selftest.c              |  45 +++++--
 drivers/iommu/ipmmu-vmsa.c                    |  38 ++++--
 drivers/iommu/msm_iommu.c                     |  31 ++++-
 drivers/iommu/mtk_iommu.c                     |  17 ++-
 drivers/iommu/omap-iommu.c                    |  41 +++---
 drivers/iommu/riscv/iommu.c                   |  16 ++-
 drivers/iommu/rockchip-iommu.c                |  18 ++-
 drivers/iommu/s390-iommu.c                    |  22 +++-
 drivers/iommu/sun50i-iommu.c                  |  16 ++-
 drivers/iommu/tegra-smmu.c                    |  23 ++--
 drivers/iommu/virtio-iommu.c                  |  16 ++-
 27 files changed, 671 insertions(+), 305 deletions(-)
Signature · 1 line
2.43.0
#1 lore raw

PATCH 1/20 iommu: Lock group->mutex in iommu_deferred_attach()

The iommu_deferred_attach() function invokes __iommu_attach_device() while not holding the group->mutex, like other __iommu_attach_device() callers.

Though there is no pratical bug being triggered so far, it would be better to apply the same locking to this __iommu_attach_device(), since the IOMMU drivers nowaday are more aware of the group->mutex -- some of them use the iommu_group_mutex_assert() function that could be potentially in the path of an attach_dev callback function invoked by the __iommu_attach_device().

The iommu_deferred_attach() will soon need to invoke a new domain op that must be locked with __iommu_attach_device under group->mutex.

So, grab the mutex to guard __iommu_attach_device() like other callers.

---
 drivers/iommu/iommu.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)
drivers/iommu/iommu.c +10 -3
index 2ca990dfbb884..170e522b5bda4 100644
@@ -2185,10 +2185,17 @@ EXPORT_SYMBOL_GPL(iommu_attach_device);
 
 int iommu_deferred_attach(struct device *dev, struct iommu_domain *domain)
 {
-	if (dev->iommu && dev->iommu->attach_deferred)
-		return __iommu_attach_device(domain, dev, NULL);
+	/*
+	 * This is called on the dma mapping fast path so avoid locking. This is
+	 * racy, but we have an expectation that the driver will setup its DMAs
+	 * inside probe while being single threaded to avoid racing.
+	 */
+	if (!dev->iommu || !dev->iommu->attach_deferred)
+		return 0;
 
-	return 0;
+	guard(mutex)(&dev->iommu_group->mutex);
+
+	return __iommu_attach_device(domain, dev, NULL);
 }
 
 void iommu_detach_device(struct iommu_domain *domain, struct device *dev)
Signature · 1 line
2.43.0
#2 lore raw

PATCH 2/20 iommu: Introduce a test_dev domain op and an internal helper

In reply to Nicolin Chen

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()

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.

---
 include/linux/iommu.h |  17 +++++--
 drivers/iommu/iommu.c | 111 ++++++++++++++++++++++++++++++------------
 2 files changed, 93 insertions(+), 35 deletions(-)
include/linux/iommu.h +14 -3
index 801b2bd9e8d49..2ec99502dc29c 100644
@@ -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
+ *            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.
+ *            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,
drivers/iommu/iommu.c +79 -32

179 lines not on this page · raw message

Signature · 1 line
2.43.0
maintainer Reviewed-by on v1 2/20
#3 lore raw

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

Quoted 8 lines · Nicolin Chen

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.

Quoted 22 lines

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.

---
 include/linux/iommu.h |  17 +++++--
 drivers/iommu/iommu.c | 111 ++++++++++++++++++++++++++++++------------
 2 files changed, 93 insertions(+), 35 deletions(-)
include/linux/iommu.h +0 -0
index 801b2bd9e8d49..2ec99502dc29c 100644

include/linux/iommu.h struct iommu_ops { · line 718

Quoted 6 lines
include/linux/iommu.h +2 -1
@@ -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.

include/linux/iommu.h struct iommu_ops { · line 721

Quoted 3 lines
include/linux/iommu.h +3 -0
+ *            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."?

Quoted 24 lines
include/linux/iommu.h +7 -2
+ *            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

include/linux/iommu.h struct iommu_ops { · line 767

Quoted 8 lines
include/linux/iommu.h +2 -0
@@ -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 ---

include/linux/iommu.h int iommu_replace_device_pasid(struct iommu_domain *domain, · line 3667

Quoted 12 lines
include/linux/iommu.h +5 -0
@@ -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.

#4 lore raw

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

Hi Niklas,

Quoted 5 lines · Niklas Schnelle
Quoted 4 lines · Nicolin Chen

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.

Quoted 7 lines
Quoted 4 lines

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.

iommu_deferred_attach() invokes __iommu_attach_device(), so it is already included in the list :)

Quoted 7 lines
Quoted 5 lines

/**
* 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.

Ack.

Quoted 11 lines
Quoted 3 lines
+ *            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."?

Sure. I can do that. Fixing both parts, it would be:

* @test_dev: Test compatibility prior to an @attach_dev or @set_dev_pasid call.

 *            A driver callback of this op should do a thorough sanity check, to
 *            make sure a device is compatible with the domain, so the following
 *            @attach_dev and @set_dev_pasid functions would likely succeed with
 *            only the exception of temporary failures like out of memory.
Quoted 17 lines

--- snip ---

drivers/iommu/iommu.c int iommu_replace_device_pasid(struct iommu_domain *domain, · line 3667

Quoted 12 lines
drivers/iommu/iommu.c +5 -0
@@ -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.

Quoted 1 line

Thanks for the review!
Nicolin

maintainer
#5 lore raw

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

Quoted 13 lines · Nicolin Chen

Hi Niklas,

Quoted 10 lines · Niklas Schnelle
Quoted 8 lines · Nicolin Chen

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()

Quoted 5 lines
Quoted 2 lines

Should this list also include iommu_deferred_attach()? The code does include it.

iommu_deferred_attach() invokes __iommu_attach_device(), so it is already included in the list :)

Ok makes sense, though it does list __iommu_device_set_domain() separately. Either way is fine for me.

Quoted 23 lines
Quoted 8 lines
Quoted 6 lines

/**
* 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.

Ack.

Quoted 11 lines
Quoted 3 lines
+ *            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."?

Quoted 8 lines

Sure. I can do that. Fixing both parts, it would be:

* @test_dev: Test compatibility prior to an @attach_dev or @set_dev_pasid call.

 *            A driver callback of this op should do a thorough sanity check, to
 *            make sure a device is compatible with the domain, so the following
 *            @attach_dev and @set_dev_pasid functions would likely succeed with
 *            only the exception of temporary failures like out of memory.

Sounds good, thanks!

maintainer
#6 lore raw

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

In reply to Nicolin Chen
Quoted 3 lines · Nicolin Chen

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.

I'm not sure about this.. For the problem we are trying to solve this would be racy as the test would be done and the group mutex unlocked. Then later it will be re-tested and attached.

Quoted 9 lines

/**
* 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
+ *            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.
+ *            It's suggested to avoid the kernel prints in this op.

This is a general rule, even normal attach should be following it or iommufd will be spamming the log.

Quoted 22 lines

* Return:
* * 0 - success
* * EINVAL - can indicate that device and domain are incompatible due to

include/linux/iommu.h +6 -2
@@ -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

include/linux/iommu.h struct iommu_ops { · line 764

Quoted 5 lines
include/linux/iommu.h +2 -0
@@ -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);

Because of the starting remark I'm skeptical that old should be included here.

The rest looks OK

Jason

#7 lore raw

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

Quoted 9 lines · Jason Gunthorpe
Quoted 3 lines · Nicolin Chen

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.

I'm not sure about this.. For the problem we are trying to solve this would be racy as the test would be done and the group mutex unlocked. Then later it will be re-tested and attached.

Oh right, we'll have to retest in iommu_dev_reset_done(). I missed that.

Quoted 9 lines

include/linux/iommu.h struct iommu_ops { · line 764

Quoted 6 lines
include/linux/iommu.h +2 -0
@@ -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);

Because of the starting remark I'm skeptical that old should be included here.

Hmm, the followings functions sanitizes "old":
- qcom_iommu_identity_attach() drivers/iommu/arm/arm-smmu/qcom_iommu.c
- iommu_sva_set_dev_pasid() in drivers/iommu/amd/pasid.c

Thanks
Nicolin

maintainer
#8 lore raw

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

Quoted 24 lines · Nicolin Chen
Quoted 9 lines · Jason Gunthorpe
Quoted 3 lines · Nicolin Chen

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.

I'm not sure about this.. For the problem we are trying to solve this would be racy as the test would be done and the group mutex unlocked. Then later it will be re-tested and attached.

Oh right, we'll have to retest in iommu_dev_reset_done(). I missed that.

Quoted 9 lines

include/linux/iommu.h struct iommu_ops { · line 764

Quoted 6 lines
include/linux/iommu.h +2 -0
@@ -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);

Because of the starting remark I'm skeptical that old should be included here.

Quoted 2 lines

Hmm, the followings functions sanitizes "old":
- qcom_iommu_identity_attach() drivers/iommu/arm/arm-smmu/qcom_iommu.c

That shouldn't be copied over to test??

        if (domain == identity_domain || !domain)
                return 0;

That is just optimizing away the attach if it has nothing to do

        qcom_domain = to_qcom_iommu_domain(domain);
        if (WARN_ON(!qcom_domain->iommu))
                return -EINVAL;

That can't never happen

Quoted 1 line

- iommu_sva_set_dev_pasid() in drivers/iommu/amd/pasid.c

Its broken, you are not required by API to detach a domain before setting a new one. Keep it in attach, hope someone fixes this driver someday.

Jason

#9 lore raw

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

Quoted 27 lines · Jason Gunthorpe
Quoted 12 lines · Nicolin Chen
Quoted 10 lines · Jason Gunthorpe

include/linux/iommu.h struct iommu_ops { · line 764

Quoted 6 lines · Nicolin Chen
include/linux/iommu.h +2 -0
@@ -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);

Because of the starting remark I'm skeptical that old should be included here.

Quoted 2 lines

Hmm, the followings functions sanitizes "old":
- qcom_iommu_identity_attach() drivers/iommu/arm/arm-smmu/qcom_iommu.c

That shouldn't be copied over to test??

        if (domain == identity_domain || !domain)
                return 0;

That is just optimizing away the attach if it has nothing to do

        qcom_domain = to_qcom_iommu_domain(domain);
        if (WARN_ON(!qcom_domain->iommu))
                return -EINVAL;
Quoted 7 lines

That can't never happen

Quoted 1 line

- iommu_sva_set_dev_pasid() in drivers/iommu/amd/pasid.c

Its broken, you are not required by API to detach a domain before setting a new one. Keep it in attach, hope someone fixes this driver someday.

OK. I am leaving them alone. And no more old passed to test_dev.

Thanks
Nicolin

#10 lore raw

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

In reply to Nicolin Chen
Quoted 1 line · Nicolin Chen
(quoted diff) +0 -0
@@ -3479,38 +3545,19 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,

...

Quoted 4 lines
drivers/iommu/iommu.c +0 -4
-	for_each_group_device(group, device) {
-		/*
-		 * Skip PASID validation for devices without PASID support
-		 * (max_pasids = 0). These devices cannot issue transactions

drivers/iommu/iommu.c int iommu_attach_device_pasid(struct iommu_domain *domain, · line 3558

Quoted 10 lines
drivers/iommu/iommu.c +2 -8
-		 * with PASID, so they don't affect group's PASID usage.
-		 */
-		if ((device->dev->iommu->max_pasids > 0) &&
-		    (pasid >= device->dev->iommu->max_pasids)) {
-			ret = -EINVAL;
-			goto out_unlock;
-		}
-	}
+
+	ret = __iommu_domain_test_device(domain, dev, pasid, NULL);

I realized that here it needs to be under for_each_group_device, as its following __iommu_set_group_pasid() calls attach_dev() on every group_device. So, the new code should run a test_dev() on every group_device to keep the consistency.

drivers/iommu/iommu.c int iommu_replace_device_pasid(struct iommu_domain *domain, · line 3666

Quoted 11 lines
drivers/iommu/iommu.c +5 -0
@@ -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);

Needs the same fix above.

Nicolin

maintainer
#11 lore raw

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

In reply to Nicolin Chen
Quoted 24 lines

From: Nicolin Chen <[email protected]>
Sent: Monday, October 13, 2025 8:05 AM

include/linux/iommu.h +1 -1
@@ -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
+ *            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.
+ * 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

include/linux/iommu.h +0 -1
@@ -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

include/linux/iommu.h struct iommu_ops { · line 738

Quoted 10 lines
include/linux/iommu.h +6 -1
- * * 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

It might need more work to meet this requirement. e.g. after patch4 I could still spot other errors easily in the attach path:

intel_iommu_attach_device()
iopf_for_domain_set()

    intel_iommu_enable_iopf():

        if (!info->pri_enabled)
                return -ENODEV;

intel_iommu_attach_device()
dmar_domain_attach_device()

    domain_attach_iommu():
      
       curr = xa_cmpxchg(&domain->iommu_array, iommu->seq_id,
                          NULL, info, GFP_KERNEL);
        if (curr) {
                ret = xa_err(curr) ? : -EBUSY;
                goto err_clear;
        }

intel_iommu_attach_device()
dmar_domain_attach_device()

    domain_setup_first_level()
      __domain_setup_first_level()
        intel_pasid_setup_first_level():

        pte = intel_pasid_get_entry(dev, pasid);
        if (!pte) {
                spin_unlock(&iommu->lock);
                return -ENODEV;
        }

        if (pasid_pte_is_present(pte)) {
                spin_unlock(&iommu->lock);
                return -EBUSY;
        }

On the other hand, how do we communicate whatever errors returned by attach_dev in the reset_done path back to userspace? As noted above resource allocation failures could still occur in attach_dev, but userspace may think the requested attach in middle of a reset has succeeded as long as it passes the test_dev check.

Does it work better to block the attaching process upon ongoing reset and wake it up later upon reset_done to resume attach?

#12 lore raw

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

Quoted 9 lines · Kevin

It might need more work to meet this requirement. e.g. after patch4 I could still spot other errors easily in the attach path:

intel_iommu_attach_device()
iopf_for_domain_set()

    intel_iommu_enable_iopf():

        if (!info->pri_enabled)
                return -ENODEV;

Yea, I missed that.

Quoted 10 lines

intel_iommu_attach_device()
dmar_domain_attach_device()

    domain_attach_iommu():
      
       curr = xa_cmpxchg(&domain->iommu_array, iommu->seq_id,
                          NULL, info, GFP_KERNEL);
        if (curr) {
                ret = xa_err(curr) ? : -EBUSY;
                goto err_clear;
        }

There is actually an xa_load() in this function:

	curr = xa_load(&domain->iommu_array, iommu->seq_id);
	if (curr) {
		curr->refcnt++;
		kfree(info);
		return 0;
	}

	[...]

	info->refcnt	= 1;
	info->did	= num;
	info->iommu	= iommu;
	curr = xa_cmpxchg(&domain->iommu_array, iommu->seq_id,
			  NULL, info, GFP_KERNEL);
	if (curr) {
		ret = xa_err(curr) ? : -EBUSY;
		goto err_clear;
	}

It seems that this xa_cmpxchg could be just xa_store()?

Quoted 5 lines

intel_iommu_attach_device()
dmar_domain_attach_device()
domain_setup_first_level()
__domain_setup_first_level()
intel_pasid_setup_first_level():

Yea. There are a few others in the track also..

Quoted 10 lines
        pte = intel_pasid_get_entry(dev, pasid);
        if (!pte) {
                spin_unlock(&iommu->lock);
                return -ENODEV;
        }

        if (pasid_pte_is_present(pte)) {
                spin_unlock(&iommu->lock);
                return -EBUSY;
        }

Hmm, this is fenced by iommu->lock and can race with !attach_dev callbacks. It might be difficult to shift these to test_dev..

Quoted 5 lines

On the other hand, how do we communicate whatever errors returned by attach_dev in the reset_done path back to userspace? As noted above resource allocation failures could still occur in attach_dev, but userspace may think the requested attach in middle of a reset has succeeded as long as it passes the test_dev check.

That's a legit point. Jason pointed out that we would end up with some inconsistency between driver and core as well, at the SMMUv3 patch. So, this test_dev doesn't seemingly solve our problem very well..

Quoted 2 lines

Does it work better to block the attaching process upon ongoing reset and wake it up later upon reset_done to resume attach?

Yea, I think returning -EBUSY would be the simplest solution like we did in the previous version.

But the concern is that VF might not be aware of a PF reset, so it can still race an attachment, which would be -EBUSY as well. Then, if its driver doesn't retry/defer the attach, this might break it?

FWIW, I am thinking of another design based on Jason's remarks:
https://lore.kernel.org/linux-iommu/aQBopHFub8wyQh5C@Asurada-Nvidia/

So, instead of core initiating the round trip between the blocking domain and group->domain, it forwards dev_reset_prepare/done to the driver where it does a low-level attachment that wouldn't fail:
For SMMUv3, it's an STE update.
For intel_iommu, it seems to be the context table update?

Then, any concurrent would be allowed to carry on to go through all the compatibility/sanity checks as usual, but it would bypass the final step: STE or context table update.

Thanks
Nicolin

maintainer
#13 lore raw

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

Quoted 8 lines · Nicolin Chen

FWIW, I am thinking of another design based on Jason's remarks:
https://lore.kernel.org/linux-iommu/aQBopHFub8wyQh5C@Asurada-Nvidia/

So, instead of core initiating the round trip between the blocking domain and group->domain, it forwards dev_reset_prepare/done to the driver where it does a low-level attachment that wouldn't fail:
For SMMUv3, it's an STE update.
For intel_iommu, it seems to be the context table update?

Kevin, how bad do you think the UAPI issue is if we ignore it?

Jason

maintainer
#14 lore raw

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

Quoted 9 lines

From: Jason Gunthorpe <[email protected]>
Sent: Tuesday, November 4, 2025 2:54 AM

Quoted 2 lines · Nicolin Chen

FWIW, I am thinking of another design based on Jason's remarks:
https://lore.kernel.org/linux-iommu/aQBopHFub8wyQh5C@Asurada-

Nvidia/

Quoted 1 line
Quoted 8 lines
Quoted 5 lines

So, instead of core initiating the round trip between the blocking domain and group->domain, it forwards dev_reset_prepare/done to the driver where it does a low-level attachment that wouldn't fail:
For SMMUv3, it's an STE update.
For intel_iommu, it seems to be the context table update?

Kevin, how bad do you think the UAPI issue is if we ignore it?

yeah probably better to leave it. I didn't see a clean way and the value didn't justify the complexity.

Regarding to PF reset, it's a devastating operation while the vf user is operating the vf w/o any awareness. there must be certain coordination in userspace. otherwise nobody can recover the registers. Comparing to that, solving the domain attach problem is less important...

#15 lore raw

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

Quoted 18 lines · Kevin
Quoted 9 lines

From: Jason Gunthorpe <[email protected]>
Sent: Tuesday, November 4, 2025 2:54 AM

Quoted 2 lines · Nicolin Chen

FWIW, I am thinking of another design based on Jason's remarks:
https://lore.kernel.org/linux-iommu/aQBopHFub8wyQh5C@Asurada-

Nvidia/

Quoted 1 line
Quoted 8 lines
Quoted 5 lines

So, instead of core initiating the round trip between the blocking domain and group->domain, it forwards dev_reset_prepare/done to the driver where it does a low-level attachment that wouldn't fail:
For SMMUv3, it's an STE update.
For intel_iommu, it seems to be the context table update?

Kevin, how bad do you think the UAPI issue is if we ignore it?

Quoted 8 lines

yeah probably better to leave it. I didn't see a clean way and the value didn't justify the complexity.

Regarding to PF reset, it's a devastating operation while the vf user is operating the vf w/o any awareness. there must be certain coordination in userspace. otherwise nobody can recover the registers. Comparing to that, solving the domain attach problem is less important...

If I capture these correctly, we should go with a -EBUSY version:
- Reject concurrent attachments during a device reset
- Skip reset for devices having sibling group devices
- Allow PF to stop IOMMU, ignoring VFs
?

That sounds pretty much like this v4:
https://lore.kernel.org/linux-iommu/0f6021b500c74db33af8118210dd7a2b2fd31b3c.1756682135.git.nicolinc@nvidia.com/
by dropping the SRIOV concern.

Thanks
Nicolin

maintainer
#16 lore raw

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

Quoted 34 lines · Nicolin Chen
Quoted 18 lines · Kevin
Quoted 9 lines

From: Jason Gunthorpe <[email protected]>
Sent: Tuesday, November 4, 2025 2:54 AM

Quoted 2 lines · Nicolin Chen

FWIW, I am thinking of another design based on Jason's remarks:
https://lore.kernel.org/linux-iommu/aQBopHFub8wyQh5C@Asurada-

Nvidia/

Quoted 1 line
Quoted 8 lines
Quoted 5 lines

So, instead of core initiating the round trip between the blocking domain and group->domain, it forwards dev_reset_prepare/done to the driver where it does a low-level attachment that wouldn't fail:
For SMMUv3, it's an STE update.
For intel_iommu, it seems to be the context table update?

Kevin, how bad do you think the UAPI issue is if we ignore it?

Quoted 8 lines

yeah probably better to leave it. I didn't see a clean way and the value didn't justify the complexity.

Regarding to PF reset, it's a devastating operation while the vf user is operating the vf w/o any awareness. there must be certain coordination in userspace. otherwise nobody can recover the registers. Comparing to that, solving the domain attach problem is less important...

If I capture these correctly, we should go with a -EBUSY version:
- Reject concurrent attachments during a device reset
- Skip reset for devices having sibling group devices
- Allow PF to stop IOMMU, ignoring VFs
?

Quoted 3 lines

That sounds pretty much like this v4:
https://lore.kernel.org/linux-iommu/0f6021b500c74db33af8118210dd7a2b2fd31b3c.1756682135.git.nicolinc@nvidia.com/
by dropping the SRIOV concern.

It seems like the simplest answer..

I'd ignore the VFs, I think it is already really weird/dangerous to be resetting the PF while VFs have drivers bound.. Not sure there is anything we can do to make this work better.

Jason

#17 lore raw

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

Quoted 39 lines · Jason Gunthorpe
Quoted 34 lines · Nicolin Chen
Quoted 18 lines · Kevin
Quoted 9 lines

From: Jason Gunthorpe <[email protected]>
Sent: Tuesday, November 4, 2025 2:54 AM

Quoted 2 lines · Nicolin Chen

FWIW, I am thinking of another design based on Jason's remarks:
https://lore.kernel.org/linux-iommu/aQBopHFub8wyQh5C@Asurada-

Nvidia/

Quoted 1 line
Quoted 8 lines
Quoted 5 lines

So, instead of core initiating the round trip between the blocking domain and group->domain, it forwards dev_reset_prepare/done to the driver where it does a low-level attachment that wouldn't fail:
For SMMUv3, it's an STE update.
For intel_iommu, it seems to be the context table update?

Kevin, how bad do you think the UAPI issue is if we ignore it?

Quoted 8 lines

yeah probably better to leave it. I didn't see a clean way and the value didn't justify the complexity.

Regarding to PF reset, it's a devastating operation while the vf user is operating the vf w/o any awareness. there must be certain coordination in userspace. otherwise nobody can recover the registers. Comparing to that, solving the domain attach problem is less important...

If I capture these correctly, we should go with a -EBUSY version:
- Reject concurrent attachments during a device reset
- Skip reset for devices having sibling group devices
- Allow PF to stop IOMMU, ignoring VFs
?

Quoted 3 lines

That sounds pretty much like this v4:
https://lore.kernel.org/linux-iommu/0f6021b500c74db33af8118210dd7a2b2fd31b3c.1756682135.git.nicolinc@nvidia.com/
by dropping the SRIOV concern.

Quoted 5 lines

It seems like the simplest answer..

I'd ignore the VFs, I think it is already really weird/dangerous to be resetting the PF while VFs have drivers bound.. Not sure there is anything we can do to make this work better.

Ack. I've rebased that and will do some cleanup and run sanity tests before sending.

Thanks
Nicolin

#18 lore raw

PATCH 3/20 iommu/arm-smmu-v3: Implement arm_smmu_domain_test_dev

In reply to Nicolin Chen

Move sanity and compatibility tests from the attach_dev callbacks to this new test_dev callback function. The IOMMU core makes sure an attach_dev() must be invoked after a successful test_dev callback.

---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |   2 +
 .../arm/arm-smmu-v3/arm-smmu-v3-iommufd.c     |   6 +-
 .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c   |   4 +-
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 113 +++++++++++-------
 4 files changed, 74 insertions(+), 51 deletions(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +2 -0
index ae23aacc38402..acb1dbc592cf0 100644
@@ -963,6 +963,8 @@ void arm_smmu_write_cd_entry(struct arm_smmu_master *master, int ssid,
 			     struct arm_smmu_cd *cdptr,
 			     const struct arm_smmu_cd *target);
 
+int arm_smmu_domain_test_dev(struct iommu_domain *domain, struct device *dev,
+			     ioasid_t pasid, struct iommu_domain *old_domain);
 int arm_smmu_set_pasid(struct arm_smmu_master *master,
 		       struct arm_smmu_domain *smmu_domain, ioasid_t pasid,
 		       struct arm_smmu_cd *cd, struct iommu_domain *old);
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c +1 -5
index 313201a616991..a253f9c8bb290 100644
@@ -152,11 +152,6 @@ static int arm_smmu_attach_dev_nested(struct iommu_domain *domain,
 	struct arm_smmu_ste ste;
 	int ret;
 
-	if (nested_domain->vsmmu->smmu != master->smmu)
-		return -EINVAL;
-	if (arm_smmu_ssids_in_use(&master->cd_table))
-		return -EBUSY;
-
 	mutex_lock(&arm_smmu_asid_lock);
 	/*
 	 * The VM has to control the actual ATS state at the PCI device because
@@ -187,6 +182,7 @@ static void arm_smmu_domain_nested_free(struct iommu_domain *domain)
 }
 
 static const struct iommu_domain_ops arm_smmu_nested_ops = {
+	.test_dev = arm_smmu_domain_test_dev,
 	.attach_dev = arm_smmu_attach_dev_nested,
 	.free = arm_smmu_domain_nested_free,
 };
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c +1 -3
index 59a480974d80f..610d9e826c07e 100644
@@ -276,9 +276,6 @@ static int arm_smmu_sva_set_dev_pasid(struct iommu_domain *domain,
 	struct arm_smmu_cd target;
 	int ret;
 
-	if (!(master->smmu->features & ARM_SMMU_FEAT_SVA))
-		return -EOPNOTSUPP;
-
 	/* Prevent arm_smmu_mm_release from being called while we are attaching */
 	if (!mmget_not_zero(domain->mm))
 		return -EINVAL;
@@ -319,6 +316,7 @@ static void arm_smmu_sva_domain_free(struct iommu_domain *domain)
 }
 
 static const struct iommu_domain_ops arm_smmu_sva_domain_ops = {
+	.test_dev		= arm_smmu_domain_test_dev,
 	.set_dev_pasid		= arm_smmu_sva_set_dev_pasid,
 	.free			= arm_smmu_sva_domain_free
 };
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +70 -43

180 lines not on this page · raw message

Signature · 1 line
2.43.0
maintainer
#19 lore raw

Re: [PATCH v1 03/20] iommu/arm-smmu-v3: Implement arm_smmu_domain_test_dev

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static int arm_smmu_enable_iopf(struct arm_smmu_master *master, · line 2769

Quoted 10 lines · Nicolin Chen
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +0 -2
index a33fbd12a0dd9..3448e55bbcdbb 100644
@@ -2765,9 +2765,6 @@ static int arm_smmu_enable_iopf(struct arm_smmu_master *master,
 
 	iommu_group_mutex_assert(master->dev);
 
-	if (!IS_ENABLED(CONFIG_ARM_SMMU_V3_SVA))
-		return -EOPNOTSUPP;

Stuff like this is also optimizing the codegen, it shouldn't be removed.

Quoted 10 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +10 -0
+int arm_smmu_domain_test_dev(struct iommu_domain *domain, struct device *dev,
+			     ioasid_t pasid, struct iommu_domain *old_domain)
+{
+	struct arm_smmu_domain *device_domain = to_smmu_domain_devices(domain);
+	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
+
+	if (!dev_iommu_fwspec_get(dev))
+		return -ENOENT;
+
+	switch (domain->type) {

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c void arm_smmu_attach_commit(struct arm_smmu_attach_state *state) · line 3007

Quoted 10 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +10 -0
+	case IOMMU_DOMAIN_NESTED: {
+		struct arm_smmu_nested_domain *nested_domain =
+			to_smmu_nested_domain(domain);
+
+		if (WARN_ON(pasid != IOMMU_NO_PASID))
+			return -EOPNOTSUPP;
+		if (nested_domain->vsmmu->smmu != master->smmu)
+			return -EINVAL;
+		if (arm_smmu_ssids_in_use(&master->cd_table))
+			return -EBUSY;

This gives me alot of pause.. Here we are detecting if a S1 PASID is installed for a S2 attach, but to your purpose this can be made inconsistent by userspace during a FLR..

I don't see any reasonable way to mitigate this??

Which makes me wonder if we should just try to solve the simple obvious things like direct, permanent incompatability and still have some kind of recovery code to leave things in blocking if they fail to attach

Jason

#20 lore raw

Re: [PATCH v1 03/20] iommu/arm-smmu-v3: Implement arm_smmu_domain_test_dev

Quoted 7 lines · Jason Gunthorpe
Quoted 6 lines · Nicolin Chen
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +0 -0
index a33fbd12a0dd9..3448e55bbcdbb 100644
@@ -2765,9 +2765,6 @@ static int arm_smmu_enable_iopf(struct arm_smmu_master *master,
 
Quoted 7 lines

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static int arm_smmu_enable_iopf(struct arm_smmu_master *master, · line 2769

Quoted 4 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +0 -2
 	iommu_group_mutex_assert(master->dev);
 
-	if (!IS_ENABLED(CONFIG_ARM_SMMU_V3_SVA))
-		return -EOPNOTSUPP;

Stuff like this is also optimizing the codegen, it shouldn't be removed.

Okay. I assume we should just copy it to test_dev() then.

Quoted 16 lines
Quoted 16 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +16 -0
+int arm_smmu_domain_test_dev(struct iommu_domain *domain, struct device *dev,
+			     ioasid_t pasid, struct iommu_domain *old_domain)
+{
+	struct arm_smmu_domain *device_domain = to_smmu_domain_devices(domain);
+	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
+
+	if (!dev_iommu_fwspec_get(dev))
+		return -ENOENT;
+
+	switch (domain->type) {
+	case IOMMU_DOMAIN_NESTED: {
+		struct arm_smmu_nested_domain *nested_domain =
+			to_smmu_nested_domain(domain);
+
+		if (WARN_ON(pasid != IOMMU_NO_PASID))
+			return -EOPNOTSUPP;
Quoted 8 lines

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c void arm_smmu_attach_commit(struct arm_smmu_attach_state *state) · line 3007

Quoted 4 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +4 -0
+		if (nested_domain->vsmmu->smmu != master->smmu)
+			return -EINVAL;
+		if (arm_smmu_ssids_in_use(&master->cd_table))
+			return -EBUSY;

This gives me alot of pause.. Here we are detecting if a S1 PASID is installed for a S2 attach, but to your purpose this can be made inconsistent by userspace during a FLR..

Ah right, the used_ssids could mismatch with the group->domain!

Quoted 1 line

I don't see any reasonable way to mitigate this??

Right. It can't simply go through a regular attach_dev call since driver wouldn't expect any inconsistency in the core.

Driver would have to be aware of the reset state, and make a copy of the old domain's CD/STE to use for a test_dev() during a reset.

Quoted 4 lines

Which makes me wonder if we should just try to solve the simple obvious things like direct, permanent incompatability and still have some kind of recovery code to leave things in blocking if they fail to attach

I don't quite get this. Mind elaborating?

Thanks
Nicolin

maintainer
#21 lore raw

Re: [PATCH v1 03/20] iommu/arm-smmu-v3: Implement arm_smmu_domain_test_dev

Quoted 7 lines · Nicolin Chen
Quoted 1 line

I don't see any reasonable way to mitigate this??

Right. It can't simply go through a regular attach_dev call since driver wouldn't expect any inconsistency in the core.

Driver would have to be aware of the reset state, and make a copy of the old domain's CD/STE to use for a test_dev() during a reset.

It seems convoluted :\

IDK if you want to do that some kind of "attach but really don't" flag so all the tracking was kept, just the STE was forced to blocking.

Then since all the fake attaches are tracked and validated switching to a real ste shouldn't fail.

For instance SMMU could continue to build the CD table and act like the CD is active, but the STE wouldn't point to it.

Basically, this approach doesn't seem to solve all the problems, it reduces them sure, but there are still enough gaps.

So I think we either have to live with them and call it a user bug to change the domain improperly during FLR.. If it happens dump the translation into blocking and move on..

Or we have to do some kind of fake attach tracking which doesn't sound appealing..

Jason

#22 lore raw

Re: [PATCH v1 03/20] iommu/arm-smmu-v3: Implement arm_smmu_domain_test_dev

Quoted 11 lines · Jason Gunthorpe
Quoted 7 lines · Nicolin Chen
Quoted 1 line

I don't see any reasonable way to mitigate this??

Right. It can't simply go through a regular attach_dev call since driver wouldn't expect any inconsistency in the core.

Driver would have to be aware of the reset state, and make a copy of the old domain's CD/STE to use for a test_dev() during a reset.

It seems convoluted :\

Yea. Both core and driver would end up with complexities Orz.

Quoted 11 lines

IDK if you want to do that some kind of "attach but really don't" flag so all the tracking was kept, just the STE was forced to blocking.

Then since all the fake attaches are tracked and validated switching to a real ste shouldn't fail.

For instance SMMU could continue to build the CD table and act like the CD is active, but the STE wouldn't point to it.

Basically, this approach doesn't seem to solve all the problems, it reduces them sure, but there are still enough gaps.

Yea, this works for reset because the physical STE is on an ABORT state, so SW can concurrently mutate the CD as much as it wants. Yet, with a translating STE, CD table can't be mutated I suppose?

So this "attach but really don't" flag would be used by the reset case exclusively. It feels like the surgical in-driver approach that we thought about in the beginning v.s. the core-managed one that we wished for to fix all drivers.

By doing so, I think the core could simply forward the two reset callbacks to the driver where it does an STE-level suspend/resume and raises a reset flag so that any concurrent attach_dev would bypass the last STE updates until the reset_done is finished.

This, however, will require driver to implement those callbacks individually, if it cares to fix the vulnerability..

Quoted 3 lines

So I think we either have to live with them and call it a user bug to change the domain improperly during FLR.. If it happens dump the translation into blocking and move on..

But, we previously discussed that it'd not be a user bug for VFs, unless they are all getting reset by the PCI core?

Otherwise, returning -EBUSY rejecting concurrent attach would be the easiest way.

Thanks
Nicolin

#23 lore raw

PATCH 4/20 iommu/intel: Implement test_dev callbacks to domain ops

In reply to Nicolin Chen

Move sanity and compatibility tests from the attach_dev callbacks to the new test_dev callback functions. The IOMMU core makes sure an attach_dev call must be invoked after a successful test_dev call.

Following the test_dev guideline, replace dev_err with dev_dbg.

---
 drivers/iommu/intel/iommu.c  | 66 +++++++++++++++++++++---------------
 drivers/iommu/intel/nested.c | 29 +++++++++++-----
 drivers/iommu/intel/svm.c    | 11 +++---
 3 files changed, 67 insertions(+), 39 deletions(-)
drivers/iommu/intel/iommu.c +39 -27
index f0396591cd9bb..10d422bd463a2 100644
@@ -3537,6 +3537,26 @@ int paging_domain_compatible(struct iommu_domain *domain, struct device *dev)
 	return 0;
 }
 
+static int intel_iommu_test_device(struct iommu_domain *domain,
+				   struct device *dev, ioasid_t pasid,
+				   struct iommu_domain *old)
+{
+	struct device_domain_info *info = dev_iommu_priv_get(dev);
+	struct intel_iommu *iommu = info->iommu;
+
+	if (pasid != IOMMU_NO_PASID) {
+		if (WARN_ON_ONCE(!(domain->type & __IOMMU_DOMAIN_PAGING)))
+			return -EINVAL;
+		if (!pasid_supported(iommu) || dev_is_real_dma_subdevice(dev))
+			return -EOPNOTSUPP;
+		if (domain->dirty_ops)
+			return -EINVAL;
+		if (context_copied(iommu, info->bus, info->devfn))
+			return -EBUSY;
+	}
+	return paging_domain_compatible(domain, dev);
+}
+
 static int intel_iommu_attach_device(struct iommu_domain *domain,
 				     struct device *dev,
 				     struct iommu_domain *old)
@@ -3545,10 +3565,6 @@ static int intel_iommu_attach_device(struct iommu_domain *domain,
 
 	device_block_translation(dev);
 
-	ret = paging_domain_compatible(domain, dev);
-	if (ret)
-		return ret;
-
 	ret = iopf_for_domain_set(domain, dev);
 	if (ret)
 		return ret;
@@ -4151,22 +4167,6 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
 	struct dev_pasid_info *dev_pasid;
 	int ret;
 
-	if (WARN_ON_ONCE(!(domain->type & __IOMMU_DOMAIN_PAGING)))
-		return -EINVAL;
-
-	if (!pasid_supported(iommu) || dev_is_real_dma_subdevice(dev))
-		return -EOPNOTSUPP;
-
-	if (domain->dirty_ops)
-		return -EINVAL;
-
-	if (context_copied(iommu, info->bus, info->devfn))
-		return -EBUSY;
-
-	ret = paging_domain_compatible(domain, dev);
-	if (ret)
-		return ret;
-
 	dev_pasid = domain_add_dev_pasid(domain, dev, pasid);
 	if (IS_ERR(dev_pasid))
 		return PTR_ERR(dev_pasid);
@@ -4178,12 +4178,9 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
 	if (intel_domain_is_fs_paging(dmar_domain))
 		ret = domain_setup_first_level(iommu, dmar_domain,
 					       dev, pasid, old);
-	else if (intel_domain_is_ss_paging(dmar_domain))
+	else /* paging_domain_compatible() made sure it's ss_paging */
 		ret = domain_setup_second_level(iommu, dmar_domain,
 						dev, pasid, old);
-	else if (WARN_ON(true))
-		ret = -EINVAL;
-
 	if (ret)
 		goto out_unwind_iopf;
 
@@ -4403,6 +4400,21 @@ static int device_setup_pass_through(struct device *dev)
 				      context_setup_pass_through_cb, dev);
 }
 
+static int identity_domain_test_dev(struct iommu_domain *domain,
+				    struct device *dev, ioasid_t pasid,
+				    struct iommu_domain *old)
+{
+	if (pasid != IOMMU_NO_PASID) {
+		struct device_domain_info *info = dev_iommu_priv_get(dev);
+		struct intel_iommu *iommu = info->iommu;
+
+		if (!pasid_supported(iommu) || dev_is_real_dma_subdevice(dev))
+			return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
 static int identity_domain_attach_dev(struct iommu_domain *domain,
 				      struct device *dev,
 				      struct iommu_domain *old)
@@ -4440,9 +4452,6 @@ static int identity_domain_set_dev_pasid(struct iommu_domain *domain,
 	struct intel_iommu *iommu = info->iommu;
 	int ret;
 
-	if (!pasid_supported(iommu) || dev_is_real_dma_subdevice(dev))
-		return -EOPNOTSUPP;
-
 	ret = iopf_for_domain_replace(domain, old, dev);
 	if (ret)
 		return ret;
@@ -4460,12 +4469,14 @@ static int identity_domain_set_dev_pasid(struct iommu_domain *domain,
 static struct iommu_domain identity_domain = {
 	.type = IOMMU_DOMAIN_IDENTITY,
 	.ops = &(const struct iommu_domain_ops) {
+		.test_dev	= identity_domain_test_dev,
 		.attach_dev	= identity_domain_attach_dev,
 		.set_dev_pasid	= identity_domain_set_dev_pasid,
 	},
 };
 
 const struct iommu_domain_ops intel_fs_paging_domain_ops = {
+	.test_dev = intel_iommu_test_device,
 	.attach_dev = intel_iommu_attach_device,
 	.set_dev_pasid = intel_iommu_set_dev_pasid,
 	.map_pages = intel_iommu_map_pages,
@@ -4479,6 +4490,7 @@ const struct iommu_domain_ops intel_fs_paging_domain_ops = {
 };
 
 const struct iommu_domain_ops intel_ss_paging_domain_ops = {
+	.test_dev = intel_iommu_test_device,
 	.attach_dev = intel_iommu_attach_device,
 	.set_dev_pasid = intel_iommu_set_dev_pasid,
 	.map_pages = intel_iommu_map_pages,
drivers/iommu/intel/nested.c +21 -8
index 760d7aa2ade84..708b8e653d5cd 100644
@@ -18,19 +18,17 @@
 #include "iommu.h"
 #include "pasid.h"
 
-static int intel_nested_attach_dev(struct iommu_domain *domain,
-				   struct device *dev, struct iommu_domain *old)
+static int intel_nested_test_dev(struct iommu_domain *domain,
+				 struct device *dev, ioasid_t pasid,
+				 struct iommu_domain *old)
 {
 	struct device_domain_info *info = dev_iommu_priv_get(dev);
 	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
 	struct intel_iommu *iommu = info->iommu;
-	unsigned long flags;
-	int ret = 0;
-
-	device_block_translation(dev);
+	int ret;
 
 	if (iommu->agaw < dmar_domain->s2_domain->agaw) {
-		dev_err_ratelimited(dev, "Adjusted guest address width not compatible\n");
+		dev_dbg_ratelimited(dev, "Adjusted guest address width not compatible\n");
 		return -ENODEV;
 	}
 
@@ -41,10 +39,24 @@ static int intel_nested_attach_dev(struct iommu_domain *domain,
 	 */
 	ret = paging_domain_compatible(&dmar_domain->s2_domain->domain, dev);
 	if (ret) {
-		dev_err_ratelimited(dev, "s2 domain is not compatible\n");
+		dev_dbg_ratelimited(dev, "s2 domain is not compatible\n");
 		return ret;
 	}
 
+	return 0;
+}
+
+static int intel_nested_attach_dev(struct iommu_domain *domain,
+				   struct device *dev, struct iommu_domain *old)
+{
+	struct device_domain_info *info = dev_iommu_priv_get(dev);
+	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
+	struct intel_iommu *iommu = info->iommu;
+	unsigned long flags;
+	int ret = 0;
+
+	device_block_translation(dev);
+
 	ret = domain_attach_iommu(dmar_domain, iommu);
 	if (ret) {
 		dev_err_ratelimited(dev, "Failed to attach domain to iommu\n");
@@ -192,6 +204,7 @@ static int intel_nested_set_dev_pasid(struct iommu_domain *domain,
 }
 
 static const struct iommu_domain_ops intel_nested_domain_ops = {
+	.test_dev		= intel_nested_test_dev,
 	.attach_dev		= intel_nested_attach_dev,
 	.set_dev_pasid		= intel_nested_set_dev_pasid,
 	.free			= intel_nested_domain_free,
drivers/iommu/intel/svm.c +7 -4
index e147f71f91b72..5901caa4ceebc 100644
@@ -145,6 +145,12 @@ static int intel_iommu_sva_supported(struct device *dev)
 	return 0;
 }
 
+static int intel_svm_test_dev(struct iommu_domain *domain, struct device *dev,
+			      ioasid_t pasid, struct iommu_domain *old)
+{
+	return intel_iommu_sva_supported(dev);
+}
+
 static int intel_svm_set_dev_pasid(struct iommu_domain *domain,
 				   struct device *dev, ioasid_t pasid,
 				   struct iommu_domain *old)
@@ -156,10 +162,6 @@ static int intel_svm_set_dev_pasid(struct iommu_domain *domain,
 	unsigned long sflags;
 	int ret = 0;
 
-	ret = intel_iommu_sva_supported(dev);
-	if (ret)
-		return ret;
-
 	dev_pasid = domain_add_dev_pasid(domain, dev, pasid);
 	if (IS_ERR(dev_pasid))
 		return PTR_ERR(dev_pasid);
@@ -195,6 +197,7 @@ static void intel_svm_domain_free(struct iommu_domain *domain)
 }
 
 static const struct iommu_domain_ops intel_svm_domain_ops = {
+	.test_dev		= intel_svm_test_dev,
 	.set_dev_pasid		= intel_svm_set_dev_pasid,
 	.free			= intel_svm_domain_free
 };
Signature · 1 line
2.43.0
#24 lore raw

PATCH 5/20 iommu/amd: Implement test_dev callbacks to domain ops

In reply to Nicolin Chen

Move sanity and compatibility tests from the attach_dev callbacks to the new test_dev callback functions. The IOMMU core makes sure an attach_dev call must be invoked after a successful test_dev call.

Correct the errno upon malloc failure. Also, drop the function prototype of iommu_sva_set_dev_pasid() from the header and make it static, as only pasid.c uses it.

---
 drivers/iommu/amd/amd_iommu.h |  3 ---
 drivers/iommu/amd/iommu.c     | 27 +++++++++++++++++++--------
 drivers/iommu/amd/pasid.c     | 29 +++++++++++++++++++----------
 3 files changed, 38 insertions(+), 21 deletions(-)
drivers/iommu/amd/amd_iommu.h +0 -3
index 9b4b589a54b57..f99fa4da34996 100644
@@ -52,9 +52,6 @@ struct protection_domain *protection_domain_alloc(void);
 struct iommu_domain *amd_iommu_domain_alloc_sva(struct device *dev,
 						struct mm_struct *mm);
 void amd_iommu_domain_free(struct iommu_domain *dom);
-int iommu_sva_set_dev_pasid(struct iommu_domain *domain,
-			    struct device *dev, ioasid_t pasid,
-			    struct iommu_domain *old);
 void amd_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
 				struct iommu_domain *domain);
 
drivers/iommu/amd/iommu.c +19 -8
index e16ad510c8c8a..dc0406427dcf8 100644
@@ -70,6 +70,8 @@ int amd_iommu_max_glx_val = -1;
  */
 DEFINE_IDA(pdom_ids);
 
+static int amd_iommu_test_device(struct iommu_domain *dom, struct device *dev,
+				 ioasid_t pasid, struct iommu_domain *old);
 static int amd_iommu_attach_device(struct iommu_domain *dom, struct device *dev,
 				   struct iommu_domain *old);
 
@@ -2670,6 +2672,7 @@ static struct iommu_domain blocked_domain = {
 static struct protection_domain identity_domain;
 
 static const struct iommu_domain_ops identity_domain_ops = {
+	.test_dev = amd_iommu_test_device,
 	.attach_dev = amd_iommu_attach_device,
 };
 
@@ -2686,12 +2689,26 @@ void amd_iommu_init_identity_domain(void)
 	protection_domain_init(&identity_domain);
 }
 
+static int amd_iommu_test_device(struct iommu_domain *dom, struct device *dev,
+				 ioasid_t pasid, struct iommu_domain *old)
+{
+	struct amd_iommu *iommu = get_amd_iommu_from_dev(dev);
+
+	/*
+	 * Restrict to devices with compatible IOMMU hardware support
+	 * when enforcement of dirty tracking is enabled.
+	 */
+	if (dom->dirty_ops && !amd_iommu_hd_support(iommu))
+		return -EINVAL;
+
+	return 0;
+}
+
 static int amd_iommu_attach_device(struct iommu_domain *dom, struct device *dev,
 				   struct iommu_domain *old)
 {
 	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
 	struct protection_domain *domain = to_pdomain(dom);
-	struct amd_iommu *iommu = get_amd_iommu_from_dev(dev);
 	int ret;
 
 	/*
@@ -2703,13 +2720,6 @@ static int amd_iommu_attach_device(struct iommu_domain *dom, struct device *dev,
 
 	dev_data->defer_attach = false;
 
-	/*
-	 * Restrict to devices with compatible IOMMU hardware support
-	 * when enforcement of dirty tracking is enabled.
-	 */
-	if (dom->dirty_ops && !amd_iommu_hd_support(iommu))
-		return -EINVAL;
-
 	if (dev_data->domain)
 		detach_device(dev);
 
@@ -3047,6 +3057,7 @@ const struct iommu_ops amd_iommu_ops = {
 	.def_domain_type = amd_iommu_def_domain_type,
 	.page_response = amd_iommu_page_response,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
+		.test_dev	= amd_iommu_test_device,
 		.attach_dev	= amd_iommu_attach_device,
 		.map_pages	= amd_iommu_map_pages,
 		.unmap_pages	= amd_iommu_unmap_pages,
drivers/iommu/amd/pasid.c +19 -10
index 77c8e9a91cbca..474494a66dd40 100644
@@ -99,31 +99,39 @@ static const struct mmu_notifier_ops sva_mn = {
 	.release = sva_mn_release,
 };
 
-int iommu_sva_set_dev_pasid(struct iommu_domain *domain,
-			    struct device *dev, ioasid_t pasid,
-			    struct iommu_domain *old)
+static int iommu_sva_test_dev(struct iommu_domain *domain, struct device *dev,
+			      ioasid_t pasid, struct iommu_domain *old)
 {
-	struct pdom_dev_data *pdom_dev_data;
-	struct protection_domain *sva_pdom = to_pdomain(domain);
 	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
-	unsigned long flags;
-	int ret = -EINVAL;
 
 	if (old)
 		return -EOPNOTSUPP;
 
 	/* PASID zero is used for requests from the I/O device without PASID */
 	if (!is_pasid_valid(dev_data, pasid))
-		return ret;
+		return -EINVAL;
 
 	/* Make sure PASID is enabled */
 	if (!is_pasid_enabled(dev_data))
-		return ret;
+		return -EINVAL;
+
+	return 0;
+}
+
+static int iommu_sva_set_dev_pasid(struct iommu_domain *domain,
+				   struct device *dev, ioasid_t pasid,
+				   struct iommu_domain *old)
+{
+	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
+	struct protection_domain *sva_pdom = to_pdomain(domain);
+	struct pdom_dev_data *pdom_dev_data;
+	unsigned long flags;
+	int ret;
 
 	/* Add PASID to protection domain pasid list */
 	pdom_dev_data = kzalloc(sizeof(*pdom_dev_data), GFP_KERNEL);
 	if (pdom_dev_data == NULL)
-		return ret;
+		return -ENOMEM;
 
 	pdom_dev_data->pasid = pasid;
 	pdom_dev_data->dev_data = dev_data;
@@ -175,6 +183,7 @@ static void iommu_sva_domain_free(struct iommu_domain *domain)
 }
 
 static const struct iommu_domain_ops amd_sva_domain_ops = {
+	.test_dev      = iommu_sva_test_dev,
 	.set_dev_pasid = iommu_sva_set_dev_pasid,
 	.free	       = iommu_sva_domain_free
 };
Signature · 1 line
2.43.0
#25 lore raw

PATCH 6/20 iommu/arm-smmu: Implement arm_smmu_test_dev

In reply to Nicolin Chen

Move sanity and compatibility tests from the attach_dev callback to the new test_dev callback function. The IOMMU core makes sure an attach_dev call must be invoked after a successful test_dev call.

---
 drivers/iommu/arm/arm-smmu/arm-smmu.c | 116 ++++++++++++++++----------
 1 file changed, 71 insertions(+), 45 deletions(-)
drivers/iommu/arm/arm-smmu/arm-smmu.c +71 -45
index 5e690cf85ec96..5752eecc1d434 100644
@@ -672,6 +672,37 @@ static int arm_smmu_alloc_context_bank(struct arm_smmu_domain *smmu_domain,
 	return __arm_smmu_alloc_bitmap(smmu->context_map, start, smmu->num_context_banks);
 }
 
+static enum arm_smmu_context_fmt
+arm_smmu_get_context_fmt(struct arm_smmu_domain *smmu_domain)
+{
+	struct arm_smmu_device *smmu = smmu_domain->smmu;
+	struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
+	enum arm_smmu_context_fmt fmt = cfg->fmt;
+
+	/*
+	 * Choosing a suitable context format is even more fiddly. Until we
+	 * grow some way for the caller to express a preference, and/or move
+	 * the decision into the io-pgtable code where it arguably belongs,
+	 * just aim for the closest thing to the rest of the system, and hope
+	 * that the hardware isn't esoteric enough that we can't assume AArch64
+	 * support to be a superset of AArch32 support...
+	 */
+	if (smmu->features & ARM_SMMU_FEAT_FMT_AARCH32_L)
+		fmt = ARM_SMMU_CTX_FMT_AARCH32_L;
+	if (IS_ENABLED(CONFIG_IOMMU_IO_PGTABLE_ARMV7S) &&
+	    !IS_ENABLED(CONFIG_64BIT) && !IS_ENABLED(CONFIG_ARM_LPAE) &&
+	    (smmu->features & ARM_SMMU_FEAT_FMT_AARCH32_S) &&
+	    (smmu_domain->stage == ARM_SMMU_DOMAIN_S1))
+		fmt = ARM_SMMU_CTX_FMT_AARCH32_S;
+	if ((IS_ENABLED(CONFIG_64BIT) || cfg->fmt == ARM_SMMU_CTX_FMT_NONE) &&
+	    (smmu->features & (ARM_SMMU_FEAT_FMT_AARCH64_64K |
+			       ARM_SMMU_FEAT_FMT_AARCH64_16K |
+			       ARM_SMMU_FEAT_FMT_AARCH64_4K)))
+		fmt = ARM_SMMU_CTX_FMT_AARCH64;
+
+	return fmt;
+}
+
 static int arm_smmu_init_domain_context(struct arm_smmu_domain *smmu_domain,
 					struct arm_smmu_device *smmu,
 					struct device *dev)
@@ -712,31 +743,8 @@ static int arm_smmu_init_domain_context(struct arm_smmu_domain *smmu_domain,
 	if (!(smmu->features & ARM_SMMU_FEAT_TRANS_S2))
 		smmu_domain->stage = ARM_SMMU_DOMAIN_S1;
 
-	/*
-	 * Choosing a suitable context format is even more fiddly. Until we
-	 * grow some way for the caller to express a preference, and/or move
-	 * the decision into the io-pgtable code where it arguably belongs,
-	 * just aim for the closest thing to the rest of the system, and hope
-	 * that the hardware isn't esoteric enough that we can't assume AArch64
-	 * support to be a superset of AArch32 support...
-	 */
-	if (smmu->features & ARM_SMMU_FEAT_FMT_AARCH32_L)
-		cfg->fmt = ARM_SMMU_CTX_FMT_AARCH32_L;
-	if (IS_ENABLED(CONFIG_IOMMU_IO_PGTABLE_ARMV7S) &&
-	    !IS_ENABLED(CONFIG_64BIT) && !IS_ENABLED(CONFIG_ARM_LPAE) &&
-	    (smmu->features & ARM_SMMU_FEAT_FMT_AARCH32_S) &&
-	    (smmu_domain->stage == ARM_SMMU_DOMAIN_S1))
-		cfg->fmt = ARM_SMMU_CTX_FMT_AARCH32_S;
-	if ((IS_ENABLED(CONFIG_64BIT) || cfg->fmt == ARM_SMMU_CTX_FMT_NONE) &&
-	    (smmu->features & (ARM_SMMU_FEAT_FMT_AARCH64_64K |
-			       ARM_SMMU_FEAT_FMT_AARCH64_16K |
-			       ARM_SMMU_FEAT_FMT_AARCH64_4K)))
-		cfg->fmt = ARM_SMMU_CTX_FMT_AARCH64;
-
-	if (cfg->fmt == ARM_SMMU_CTX_FMT_NONE) {
-		ret = -EINVAL;
-		goto out_unlock;
-	}
+	cfg->fmt = arm_smmu_get_context_fmt(smmu_domain);
+	WARN_ON(cfg->fmt == ARM_SMMU_CTX_FMT_NONE);
 
 	switch (smmu_domain->stage) {
 	case ARM_SMMU_DOMAIN_S1:
@@ -1165,14 +1173,11 @@ static void arm_smmu_master_install_s2crs(struct arm_smmu_master_cfg *cfg,
 	}
 }
 
-static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev,
-			       struct iommu_domain *old)
+static int arm_smmu_test_dev(struct iommu_domain *domain, struct device *dev,
+			     ioasid_t pasid, struct iommu_domain *old)
 {
-	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
-	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
-	struct arm_smmu_master_cfg *cfg;
-	struct arm_smmu_device *smmu;
-	int ret;
+	struct arm_smmu_master_cfg *cfg = dev_iommu_priv_get(dev);
+	struct arm_smmu_domain *smmu_domain;
 
 	/*
 	 * FIXME: The arch/arm DMA API code tries to attach devices to its own
@@ -1181,11 +1186,40 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev,
 	 * domains, just say no (but more politely than by dereferencing NULL).
 	 * This should be at least a WARN_ON once that's sorted.
 	 */
-	cfg = dev_iommu_priv_get(dev);
 	if (!cfg)
 		return -ENODEV;
 
-	smmu = cfg->smmu;
+	if (domain == arm_smmu_ops.identity_domain ||
+	    domain == arm_smmu_ops.blocked_domain)
+		return 0;
+
+	smmu_domain = to_smmu_domain(domain);
+	scoped_guard(mutex, &smmu_domain->init_mutex) {
+		/* arm_smmu_init_domain_context() will initialize it */
+		if (!smmu_domain->smmu)
+			return 0;
+		/*
+		 * Sanity check the domain. We don't support domains across
+		 * different SMMUs.
+		 */
+		if (smmu_domain->smmu != cfg->smmu)
+			return -EINVAL;
+		if (arm_smmu_get_context_fmt(smmu_domain) ==
+		    ARM_SMMU_CTX_FMT_NONE)
+			return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev,
+			       struct iommu_domain *old)
+{
+	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
+	struct arm_smmu_master_cfg *cfg = dev_iommu_priv_get(dev);
+	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
+	struct arm_smmu_device *smmu = cfg->smmu;
+	int ret;
 
 	ret = arm_smmu_rpm_get(smmu);
 	if (ret < 0)
@@ -1196,15 +1230,6 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev,
 	if (ret < 0)
 		goto rpm_put;
 
-	/*
-	 * Sanity check the domain. We don't support domains across
-	 * different SMMUs.
-	 */
-	if (smmu_domain->smmu != smmu) {
-		ret = -EINVAL;
-		goto rpm_put;
-	}
-
 	/* Looks ok, so add the device to the domain */
 	arm_smmu_master_install_s2crs(cfg, S2CR_TYPE_TRANS,
 				      smmu_domain->cfg.cbndx, fwspec);
@@ -1221,8 +1246,6 @@ static int arm_smmu_attach_dev_type(struct device *dev,
 	struct arm_smmu_device *smmu;
 	int ret;
 
-	if (!cfg)
-		return -ENODEV;
 	smmu = cfg->smmu;
 
 	ret = arm_smmu_rpm_get(smmu);
@@ -1242,6 +1265,7 @@ static int arm_smmu_attach_dev_identity(struct iommu_domain *domain,
 }
 
 static const struct iommu_domain_ops arm_smmu_identity_ops = {
+	.test_dev = arm_smmu_test_dev,
 	.attach_dev = arm_smmu_attach_dev_identity,
 };
 
@@ -1258,6 +1282,7 @@ static int arm_smmu_attach_dev_blocked(struct iommu_domain *domain,
 }
 
 static const struct iommu_domain_ops arm_smmu_blocked_ops = {
+	.test_dev = arm_smmu_test_dev,
 	.attach_dev = arm_smmu_attach_dev_blocked,
 };
 
@@ -1647,6 +1672,7 @@ static const struct iommu_ops arm_smmu_ops = {
 	.def_domain_type	= arm_smmu_def_domain_type,
 	.owner			= THIS_MODULE,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
+		.test_dev		= arm_smmu_test_dev,
 		.attach_dev		= arm_smmu_attach_dev,
 		.map_pages		= arm_smmu_map_pages,
 		.unmap_pages		= arm_smmu_unmap_pages,
Signature · 1 line
2.43.0
#26 lore raw

PATCH 7/20 iommu/qcom_iommu: Implement test_dev callbacks to domain ops

In reply to Nicolin Chen

Move sanity and compatibility tests from the attach_dev callbacks to the new test_dev callback functions. The IOMMU core makes sure an attach_dev call must be invoked after a successful test_dev call.

Following the test_dev guideline, replace dev_err with dev_dbg.

---
 drivers/iommu/arm/arm-smmu/qcom_iommu.c | 46 ++++++++++++++++++-------
 1 file changed, 34 insertions(+), 12 deletions(-)
drivers/iommu/arm/arm-smmu/qcom_iommu.c +34 -12
index 9222a4a48bb33..53b1c279563ba 100644
@@ -359,18 +359,36 @@ static void qcom_iommu_domain_free(struct iommu_domain *domain)
 	kfree(qcom_domain);
 }
 
-static int qcom_iommu_attach_dev(struct iommu_domain *domain,
-				 struct device *dev, struct iommu_domain *old)
+static int qcom_iommu_domain_test_dev(struct iommu_domain *domain,
+				      struct device *dev, ioasid_t pasid,
+				      struct iommu_domain *old)
 {
 	struct qcom_iommu_dev *qcom_iommu = dev_iommu_priv_get(dev);
 	struct qcom_iommu_domain *qcom_domain = to_qcom_iommu_domain(domain);
-	int ret;
 
 	if (!qcom_iommu) {
-		dev_err(dev, "cannot attach to IOMMU, is it on the same bus?\n");
+		dev_dbg(dev, "cannot attach to IOMMU, is it on the same bus?\n");
 		return -ENXIO;
 	}
 
+	scoped_guard(mutex, &qcom_domain->init_mutex) {
+		/*
+		 * Sanity check the domain. We don't support domains across
+		 * different IOMMUs.
+		 */
+		if (qcom_domain->iommu && qcom_domain->iommu != qcom_iommu)
+			return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int qcom_iommu_attach_dev(struct iommu_domain *domain,
+				 struct device *dev, struct iommu_domain *old)
+{
+	struct qcom_iommu_dev *qcom_iommu = dev_iommu_priv_get(dev);
+	int ret;
+
 	/* Ensure that the domain is finalized */
 	pm_runtime_get_sync(qcom_iommu->dev);
 	ret = qcom_iommu_init_domain(domain, qcom_iommu, dev);
@@ -378,13 +396,17 @@ static int qcom_iommu_attach_dev(struct iommu_domain *domain,
 	if (ret < 0)
 		return ret;
 
-	/*
-	 * Sanity check the domain. We don't support domains across
-	 * different IOMMUs.
-	 */
-	if (qcom_domain->iommu != qcom_iommu)
-		return -EINVAL;
+	return 0;
+}
 
+static int qcom_iommu_identity_test(struct iommu_domain *identity_domain,
+				    struct device *dev, ioasid_t pasid,
+				    struct iommu_domain *old)
+{
+	if (old == identity_domain || !old)
+		return 0;
+	if (WARN_ON(!to_qcom_iommu_domain(old)->iommu))
+		return -EINVAL;
 	return 0;
 }
 
@@ -401,8 +423,6 @@ static int qcom_iommu_identity_attach(struct iommu_domain *identity_domain,
 		return 0;
 
 	qcom_domain = to_qcom_iommu_domain(old);
-	if (WARN_ON(!qcom_domain->iommu))
-		return -EINVAL;
 
 	pm_runtime_get_sync(qcom_iommu->dev);
 	for (i = 0; i < fwspec->num_ids; i++) {
@@ -418,6 +438,7 @@ static int qcom_iommu_identity_attach(struct iommu_domain *identity_domain,
 }
 
 static struct iommu_domain_ops qcom_iommu_identity_ops = {
+	.test_dev = qcom_iommu_identity_test,
 	.attach_dev = qcom_iommu_identity_attach,
 };
 
@@ -599,6 +620,7 @@ static const struct iommu_ops qcom_iommu_ops = {
 	.device_group	= generic_device_group,
 	.of_xlate	= qcom_iommu_of_xlate,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
+		.test_dev	= qcom_iommu_domain_test_dev,
 		.attach_dev	= qcom_iommu_attach_dev,
 		.map_pages	= qcom_iommu_map,
 		.unmap_pages	= qcom_iommu_unmap,
Signature · 1 line
2.43.0
#27 lore raw

PATCH 8/20 iommu/riscv: Implement riscv_iommu_test_paging_domain

In reply to Nicolin Chen

Move sanity and compatibility tests from the attach_dev callback to the new test_dev callback function. The IOMMU core makes sure an attach_dev call must be invoked after a successful test_dev call.

---
 drivers/iommu/riscv/iommu.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)
drivers/iommu/riscv/iommu.c +13 -3
index d9429097a2b51..6613ece2c9f41 100644
@@ -1320,6 +1320,18 @@ static bool riscv_iommu_pt_supported(struct riscv_iommu_device *iommu, int pgd_m
 	return false;
 }
 
+static int riscv_iommu_test_paging_domain(struct iommu_domain *iommu_domain,
+					  struct device *dev, ioasid_t pasid,
+					  struct iommu_domain *old)
+{
+	struct riscv_iommu_domain *domain = iommu_domain_to_riscv(iommu_domain);
+	struct riscv_iommu_device *iommu = dev_to_iommu(dev);
+
+	if (!riscv_iommu_pt_supported(iommu, domain->pgd_mode))
+		return -ENODEV;
+	return 0;
+}
+
 static int riscv_iommu_attach_paging_domain(struct iommu_domain *iommu_domain,
 					    struct device *dev,
 					    struct iommu_domain *old)
@@ -1329,9 +1341,6 @@ static int riscv_iommu_attach_paging_domain(struct iommu_domain *iommu_domain,
 	struct riscv_iommu_info *info = dev_iommu_priv_get(dev);
 	u64 fsc, ta;
 
-	if (!riscv_iommu_pt_supported(iommu, domain->pgd_mode))
-		return -ENODEV;
-
 	fsc = FIELD_PREP(RISCV_IOMMU_PC_FSC_MODE, domain->pgd_mode) |
 	      FIELD_PREP(RISCV_IOMMU_PC_FSC_PPN, virt_to_pfn(domain->pgd_root));
 	ta = FIELD_PREP(RISCV_IOMMU_PC_TA_PSCID, domain->pscid) |
@@ -1348,6 +1357,7 @@ static int riscv_iommu_attach_paging_domain(struct iommu_domain *iommu_domain,
 }
 
 static const struct iommu_domain_ops riscv_iommu_paging_domain_ops = {
+	.test_dev = riscv_iommu_test_paging_domain,
 	.attach_dev = riscv_iommu_attach_paging_domain,
 	.free = riscv_iommu_free_paging_domain,
 	.map_pages = riscv_iommu_map_pages,
Signature · 1 line
2.43.0
#28 lore raw

PATCH 9/20 iommu/mkt_iommu: Implement mtk_iommu_test_device

In reply to Nicolin Chen

Move sanity and compatibility tests from the attach_dev callback to the new test_dev callback function. The IOMMU core makes sure an attach_dev call must be invoked after a successful test_dev call.

---
 drivers/iommu/mtk_iommu.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)
drivers/iommu/mtk_iommu.c +15 -2
index 9747ef1644138..0cfcd0d08ae64 100644
@@ -704,6 +704,20 @@ static void mtk_iommu_domain_free(struct iommu_domain *domain)
 	kfree(to_mtk_domain(domain));
 }
 
+static int mtk_iommu_test_device(struct iommu_domain *domain,
+				 struct device *dev, ioasid_t pasid,
+				 struct iommu_domain *old)
+{
+	struct mtk_iommu_data *data = dev_iommu_priv_get(dev);
+	int region_id;
+
+	region_id = mtk_iommu_get_iova_region_id(dev, data->plat_data);
+	if (region_id < 0)
+		return region_id;
+
+	return 0;
+}
+
 static int mtk_iommu_attach_device(struct iommu_domain *domain,
 				   struct device *dev, struct iommu_domain *old)
 {
@@ -716,8 +730,6 @@ static int mtk_iommu_attach_device(struct iommu_domain *domain,
 	int ret, region_id;
 
 	region_id = mtk_iommu_get_iova_region_id(dev, data->plat_data);
-	if (region_id < 0)
-		return region_id;
 
 	bankid = mtk_iommu_get_bank_id(dev, data->plat_data);
 	mutex_lock(&dom->mutex);
@@ -1019,6 +1031,7 @@ static const struct iommu_ops mtk_iommu_ops = {
 	.get_resv_regions = mtk_iommu_get_resv_regions,
 	.owner		= THIS_MODULE,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
+		.test_dev	= mtk_iommu_test_device,
 		.attach_dev	= mtk_iommu_attach_device,
 		.map_pages	= mtk_iommu_map,
 		.unmap_pages	= mtk_iommu_unmap,
Signature · 1 line
2.43.0
#29 lore raw

PATCH 10/20 iommu/apple-dart: Implement test_dev callbacks to domain ops

In reply to Nicolin Chen

Move sanity and compatibility tests from the attach_dev callbacks to the new test_dev callback functions. The IOMMU core makes sure an attach_dev call must be invoked after a successful test_dev call.

Note the apple_dart_finalize_domain() has another caller than attach_dev so it has to duplicate the pgsize sanity too.

---
 drivers/iommu/apple-dart.c | 50 +++++++++++++++++++++++++++++---------
 1 file changed, 39 insertions(+), 11 deletions(-)
drivers/iommu/apple-dart.c +39 -11
index b5848770ef482..fb63dcb7462a7 100644
@@ -593,9 +593,6 @@ static int apple_dart_finalize_domain(struct apple_dart_domain *dart_domain,
 	int ret = 0;
 	int i, j;
 
-	if (dart->pgsize > PAGE_SIZE)
-		return -EINVAL;
-
 	mutex_lock(&dart_domain->init_lock);
 
 	if (dart_domain->finalized)
@@ -643,11 +640,6 @@ apple_dart_mod_streams(struct apple_dart_atomic_stream_map *domain_maps,
 {
 	int i, j;
 
-	for (i = 0; i < MAX_DARTS_PER_DEVICE; ++i) {
-		if (domain_maps[i].dart != master_maps[i].dart)
-			return -EINVAL;
-	}
-
 	for (i = 0; i < MAX_DARTS_PER_DEVICE; ++i) {
 		if (!domain_maps[i].dart)
 			break;
@@ -671,6 +663,29 @@ static int apple_dart_domain_add_streams(struct apple_dart_domain *domain,
 				      true);
 }
 
+static int apple_dart_test_dev_paging(struct iommu_domain *domain,
+				      struct device *dev, ioasid_t pasid,
+				      struct iommu_domain *old)
+{
+	struct apple_dart_domain *dart_domain = to_dart_domain(domain);
+	struct apple_dart_master_cfg *cfg = dev_iommu_priv_get(dev);
+	struct apple_dart *dart = cfg->stream_maps[0].dart;
+
+	if (dart->pgsize > PAGE_SIZE)
+		return -EINVAL;
+	if (dart_domain->finalized) {
+		int i;
+
+		for (i = 0; i < MAX_DARTS_PER_DEVICE; ++i) {
+			if (dart_domain->stream_maps[i].dart !=
+			    cfg->stream_maps[i].dart)
+				return -EINVAL;
+		}
+	}
+
+	return 0;
+}
+
 static int apple_dart_attach_dev_paging(struct iommu_domain *domain,
 					struct device *dev,
 					struct iommu_domain *old)
@@ -693,6 +708,17 @@ static int apple_dart_attach_dev_paging(struct iommu_domain *domain,
 	return 0;
 }
 
+static int apple_dart_test_dev_identity(struct iommu_domain *domain,
+					struct device *dev, ioasid_t pasid,
+					struct iommu_domain *old)
+{
+	struct apple_dart_master_cfg *cfg = dev_iommu_priv_get(dev);
+
+	if (!cfg->supports_bypass)
+		return -EINVAL;
+	return 0;
+}
+
 static int apple_dart_attach_dev_identity(struct iommu_domain *domain,
 					  struct device *dev,
 					  struct iommu_domain *old)
@@ -701,15 +727,13 @@ static int apple_dart_attach_dev_identity(struct iommu_domain *domain,
 	struct apple_dart_stream_map *stream_map;
 	int i;
 
-	if (!cfg->supports_bypass)
-		return -EINVAL;
-
 	for_each_stream_map(i, cfg, stream_map)
 		apple_dart_hw_enable_bypass(stream_map);
 	return 0;
 }
 
 static const struct iommu_domain_ops apple_dart_identity_ops = {
+	.test_dev = apple_dart_test_dev_identity,
 	.attach_dev = apple_dart_attach_dev_identity,
 };
 
@@ -776,8 +800,11 @@ static struct iommu_domain *apple_dart_domain_alloc_paging(struct device *dev)
 
 	if (dev) {
 		struct apple_dart_master_cfg *cfg = dev_iommu_priv_get(dev);
+		struct apple_dart *dart = cfg->stream_maps[0].dart;
 		int ret;
 
+		if (dart->pgsize > PAGE_SIZE)
+			return ERR_PTR(-EINVAL);
 		ret = apple_dart_finalize_domain(dart_domain, cfg);
 		if (ret) {
 			kfree(dart_domain);
@@ -1010,6 +1037,7 @@ static const struct iommu_ops apple_dart_iommu_ops = {
 	.get_resv_regions = apple_dart_get_resv_regions,
 	.owner = THIS_MODULE,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
+		.test_dev	= apple_dart_test_dev_paging,
 		.attach_dev	= apple_dart_attach_dev_paging,
 		.map_pages	= apple_dart_map_pages,
 		.unmap_pages	= apple_dart_unmap_pages,
Signature · 1 line
2.43.0
#30 lore raw

PATCH 11/20 iommu/ipmmu-vmsa: Implement ipmmu_domain_test_device

In reply to Nicolin Chen

Move sanity and compatibility tests from the attach_dev callback to the new test_dev callback function. The IOMMU core makes sure an attach_dev call must be invoked after a successful test_dev call.

---
 drivers/iommu/ipmmu-vmsa.c | 38 ++++++++++++++++++++++++++------------
 1 file changed, 26 insertions(+), 12 deletions(-)
drivers/iommu/ipmmu-vmsa.c +26 -12
index 6667ecc331f01..fdbb26ec6c632 100644
@@ -589,6 +589,30 @@ static void ipmmu_domain_free(struct iommu_domain *io_domain)
 	kfree(domain);
 }
 
+static int ipmmu_domain_test_device(struct iommu_domain *io_domain,
+				    struct device *dev, ioasid_t pasid,
+				    struct iommu_domain *old)
+{
+	struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain);
+	struct ipmmu_vmsa_device *mmu = to_ipmmu(dev);
+
+	if (!mmu) {
+		dev_dbg(dev, "Cannot attach to IPMMU\n");
+		return -ENXIO;
+	}
+
+	scoped_guard(mutex, &domain->mutex) {
+		/*
+		 * Something is wrong, we can't attach two devices using different
+		 * IOMMUs to the same domain.
+		 */
+		if (domain->mmu && domain->mmu != mmu)
+			return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int ipmmu_attach_device(struct iommu_domain *io_domain,
 			       struct device *dev, struct iommu_domain *old)
 {
@@ -598,11 +622,6 @@ static int ipmmu_attach_device(struct iommu_domain *io_domain,
 	unsigned int i;
 	int ret = 0;
 
-	if (!mmu) {
-		dev_err(dev, "Cannot attach to IPMMU\n");
-		return -ENXIO;
-	}
-
 	mutex_lock(&domain->mutex);
 
 	if (!domain->mmu) {
@@ -616,13 +635,7 @@ static int ipmmu_attach_device(struct iommu_domain *io_domain,
 			dev_info(dev, "Using IPMMU context %u\n",
 				 domain->context_id);
 		}
-	} else if (domain->mmu != mmu) {
-		/*
-		 * Something is wrong, we can't attach two devices using
-		 * different IOMMUs to the same domain.
-		 */
-		ret = -EINVAL;
-	} else
+	} else /* domain->mmu == mmu */
 		dev_info(dev, "Reusing IPMMU context %u\n", domain->context_id);
 
 	mutex_unlock(&domain->mutex);
@@ -885,6 +898,7 @@ static const struct iommu_ops ipmmu_ops = {
 			? generic_device_group : generic_single_device_group,
 	.of_xlate = ipmmu_of_xlate,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
+		.test_dev	= ipmmu_domain_test_device,
 		.attach_dev	= ipmmu_attach_device,
 		.map_pages	= ipmmu_map,
 		.unmap_pages	= ipmmu_unmap,
Signature · 1 line
2.43.0
#31 lore raw

PATCH 12/20 iommu/sun50i-iommu: Implement sun50i_iommu_domain_test_device

In reply to Nicolin Chen

Move sanity and compatibility tests from the attach_dev callback to the new test_dev callback function. The IOMMU core makes sure an attach_dev call must be invoked after a successful test_dev call.

---
 drivers/iommu/sun50i-iommu.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)
drivers/iommu/sun50i-iommu.c +11 -5
index d3b190be18b5a..d7517cfb260d5 100644
@@ -797,16 +797,21 @@ static struct iommu_domain sun50i_iommu_identity_domain = {
 	.ops = &sun50i_iommu_identity_ops,
 };
 
+static int sun50i_iommu_domain_test_device(struct iommu_domain *domain,
+					   struct device *dev, ioasid_t pasid,
+					   struct iommu_domain *old)
+{
+	if (!sun50i_iommu_from_dev(dev))
+		return -ENODEV;
+	return 0;
+}
+
 static int sun50i_iommu_attach_device(struct iommu_domain *domain,
 				      struct device *dev,
 				      struct iommu_domain *old)
 {
 	struct sun50i_iommu_domain *sun50i_domain = to_sun50i_domain(domain);
-	struct sun50i_iommu *iommu;
-
-	iommu = sun50i_iommu_from_dev(dev);
-	if (!iommu)
-		return -ENODEV;
+	struct sun50i_iommu *iommu = sun50i_iommu_from_dev(dev);
 
 	dev_dbg(dev, "Attaching to IOMMU domain\n");
 
@@ -851,6 +856,7 @@ static const struct iommu_ops sun50i_iommu_ops = {
 	.of_xlate	= sun50i_iommu_of_xlate,
 	.probe_device	= sun50i_iommu_probe_device,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
+		.test_dev	= sun50i_iommu_domain_test_device,
 		.attach_dev	= sun50i_iommu_attach_device,
 		.flush_iotlb_all = sun50i_iommu_flush_iotlb_all,
 		.iotlb_sync_map = sun50i_iommu_iotlb_sync_map,
Signature · 1 line
2.43.0
#32 lore raw

PATCH 13/20 iommu/rockchip-iommu: Implement rk_iommu_identity_test_dev

In reply to Nicolin Chen

Move sanity and compatibility tests from the attach_dev callback to the new test_dev callback function. The IOMMU core makes sure an attach_dev call must be invoked after a successful test_dev call.

---
 drivers/iommu/rockchip-iommu.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)
drivers/iommu/rockchip-iommu.c +12 -6
index 85f3667e797c3..89cb65f76fe52 100644
@@ -959,20 +959,25 @@ static int rk_iommu_enable(struct rk_iommu *iommu)
 	return ret;
 }
 
+static int rk_iommu_identity_test_dev(struct iommu_domain *domain,
+				      struct device *dev, ioasid_t pasid,
+				      struct iommu_domain *old)
+{
+	/* Allow 'virtual devices' (eg drm) to detach from domain */
+	if (!rk_iommu_from_dev(dev))
+		return -ENODEV;
+	return 0;
+}
+
 static int rk_iommu_identity_attach(struct iommu_domain *identity_domain,
 				    struct device *dev,
 				    struct iommu_domain *old)
 {
-	struct rk_iommu *iommu;
+	struct rk_iommu *iommu = rk_iommu_from_dev(dev);
 	struct rk_iommu_domain *rk_domain;
 	unsigned long flags;
 	int ret;
 
-	/* Allow 'virtual devices' (eg drm) to detach from domain */
-	iommu = rk_iommu_from_dev(dev);
-	if (!iommu)
-		return -ENODEV;
-
 	rk_domain = to_rk_domain(iommu->domain);
 
 	dev_dbg(dev, "Detaching from iommu domain\n");
@@ -997,6 +1002,7 @@ static int rk_iommu_identity_attach(struct iommu_domain *identity_domain,
 }
 
 static struct iommu_domain_ops rk_identity_ops = {
+	.test_dev = rk_iommu_identity_test_dev,
 	.attach_dev = rk_iommu_identity_attach,
 };
 
Signature · 1 line
2.43.0
#33 lore raw

PATCH 14/20 iommu/msm_iommu: Implement msm_iommu_domain_test_dev

In reply to Nicolin Chen

Move sanity and compatibility tests from the attach_dev callback to the new test_dev callback function. The IOMMU core makes sure an attach_dev call must be invoked after a successful test_dev call.

Following the test_dev guideline, replace -EEXIST with -EBUSY.

---
 drivers/iommu/msm_iommu.c | 31 ++++++++++++++++++++++++++-----
 1 file changed, 26 insertions(+), 5 deletions(-)
drivers/iommu/msm_iommu.c +26 -5
index 819add75a6652..b7b21c97d1bd7 100644
@@ -391,6 +391,31 @@ static struct iommu_device *msm_iommu_probe_device(struct device *dev)
 	return &iommu->iommu;
 }
 
+static int msm_iommu_domain_test_dev(struct iommu_domain *domain,
+				     struct device *dev, ioasid_t pasid,
+				     struct iommu_domain *old)
+{
+	struct msm_iommu_dev *iommu;
+
+	guard(spinlock_irqsave)(&msm_iommu_lock);
+
+	list_for_each_entry(iommu, &qcom_iommu_devices, dev_node) {
+		struct msm_iommu_ctx_dev *master = list_first_entry(
+			&iommu->ctx_list, struct msm_iommu_ctx_dev, list);
+
+		if (master->of_node != dev->of_node)
+			continue;
+		list_for_each_entry(master, &iommu->ctx_list, list) {
+			if (master->num) {
+				dev_dbg(dev, "domain already attached");
+				return -EBUSY;
+			}
+		}
+	}
+
+	return 0;
+}
+
 static int msm_iommu_attach_dev(struct iommu_domain *domain, struct device *dev,
 				struct iommu_domain *old)
 {
@@ -414,11 +439,6 @@ static int msm_iommu_attach_dev(struct iommu_domain *domain, struct device *dev,
 				goto fail;
 
 			list_for_each_entry(master, &iommu->ctx_list, list) {
-				if (master->num) {
-					dev_err(dev, "domain already attached");
-					ret = -EEXIST;
-					goto fail;
-				}
 				master->num =
 					msm_iommu_alloc_ctx(iommu->context_map,
 							    0, iommu->ncb);
@@ -695,6 +715,7 @@ static struct iommu_ops msm_iommu_ops = {
 	.device_group = generic_device_group,
 	.of_xlate = qcom_iommu_of_xlate,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
+		.test_dev	= msm_iommu_domain_test_dev,
 		.attach_dev	= msm_iommu_attach_dev,
 		.map_pages	= msm_iommu_map,
 		.unmap_pages	= msm_iommu_unmap,
Signature · 1 line
2.43.0
#34 lore raw

PATCH 15/20 iommu/fsl_pamu_domain: Implement fsl_pamu_domain_test_device

In reply to Nicolin Chen

Move sanity and compatibility tests from the attach_dev callback to the new test_dev callback function. The IOMMU core makes sure an attach_dev call must be invoked after a successful test_dev call.

---
 drivers/iommu/fsl_pamu_domain.c | 50 +++++++++++++++++++++++++--------
 1 file changed, 38 insertions(+), 12 deletions(-)
drivers/iommu/fsl_pamu_domain.c +38 -12
index 9664ef9840d2c..fbdaa74936394 100644
@@ -237,6 +237,43 @@ static int update_domain_stash(struct fsl_dma_domain *dma_domain, u32 val)
 	return ret;
 }
 
+static int fsl_pamu_domain_test_device(struct iommu_domain *domain,
+				       struct device *dev, ioasid_t pasid,
+				       struct iommu_domain *old)
+{
+	struct fsl_dma_domain *dma_domain = to_fsl_dma_domain(domain);
+	const u32 *liodn;
+	int len, i;
+
+	/* Use LIODN of the PCI controller while attaching a PCI device. */
+	if (dev_is_pci(dev)) {
+		/*
+		 * make dev point to pci controller device so we can get the
+		 * LIODN programmed by u-boot.
+		 */
+		dev = pci_bus_to_host(to_pci_dev(dev)->bus)->parent;
+	}
+
+	liodn = of_get_property(dev->of_node, "fsl,liodn", &len);
+	if (!liodn) {
+		pr_debug("missing fsl,liodn property at %pOF\n", dev->of_node);
+		return -ENODEV;
+	}
+
+	guard(spin_lock_irqsave)(&dma_domain->domain_lock);
+
+	for (i = 0; i < len / sizeof(u32); i++) {
+		/* Ensure that LIODN value is valid */
+		if (liodn[i] < PAACE_NUMBER_ENTRIES)
+			continue;
+		pr_debug("Invalid liodn %d, attach device failed for %pOF\n",
+			 liodn[i], dev->of_node);
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
 static int fsl_pamu_attach_device(struct iommu_domain *domain,
 				  struct device *dev, struct iommu_domain *old)
 {
@@ -263,21 +300,9 @@ static int fsl_pamu_attach_device(struct iommu_domain *domain,
 	}
 
 	liodn = of_get_property(dev->of_node, "fsl,liodn", &len);
-	if (!liodn) {
-		pr_debug("missing fsl,liodn property at %pOF\n", dev->of_node);
-		return -ENODEV;
-	}
 
 	spin_lock_irqsave(&dma_domain->domain_lock, flags);
 	for (i = 0; i < len / sizeof(u32); i++) {
-		/* Ensure that LIODN value is valid */
-		if (liodn[i] >= PAACE_NUMBER_ENTRIES) {
-			pr_debug("Invalid liodn %d, attach device failed for %pOF\n",
-				 liodn[i], dev->of_node);
-			ret = -ENODEV;
-			break;
-		}
-
 		attach_device(dma_domain, liodn[i], dev);
 		ret = pamu_set_liodn(dma_domain, dev, liodn[i]);
 		if (ret)
@@ -434,6 +459,7 @@ static const struct iommu_ops fsl_pamu_ops = {
 	.probe_device	= fsl_pamu_probe_device,
 	.device_group   = fsl_pamu_device_group,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
+		.test_dev	= fsl_pamu_domain_test_device,
 		.attach_dev	= fsl_pamu_attach_device,
 		.iova_to_phys	= fsl_pamu_iova_to_phys,
 		.free		= fsl_pamu_domain_free,
Signature · 1 line
2.43.0
#35 lore raw

Re: [PATCH v1 15/20] iommu/fsl_pamu_domain: Implement fsl_pamu_domain_test_device

Quoted 4 lines · Nicolin Chen
drivers/iommu/fsl_pamu_domain.c +4 -0
+static int fsl_pamu_domain_test_device(struct iommu_domain *domain,
+				       struct device *dev, ioasid_t pasid,
+				       struct iommu_domain *old)
+{

...

drivers/iommu/fsl_pamu_domain.c static int update_domain_stash(struct fsl_dma_domain *dma_domain, u32 val) · line 263

Quoted 7 lines
drivers/iommu/fsl_pamu_domain.c +7 -0
+	liodn = of_get_property(dev->of_node, "fsl,liodn", &len);
+	if (!liodn) {
+		pr_debug("missing fsl,liodn property at %pOF\n", dev->of_node);
+		return -ENODEV;
+	}
+
+	guard(spin_lock_irqsave)(&dma_domain->domain_lock);

Sorry. This should be:

	guard(spinlock_irqsave)(&dma_domain->domain_lock);

I have fixed this locally, and did the full build tests on all impacted arches.

I will send a v2 next week while collecting review comments here.

Thanks
Nicolin

#36 lore raw

PATCH 16/20 iommu/omap-iommu: Implement omap_iommu_domain_test_dev

In reply to Nicolin Chen

Move sanity and compatibility tests from the attach_dev callback to the new test_dev callback function. The IOMMU core makes sure an attach_dev call must be invoked after a successful test_dev call.

Following the test_dev guideline, replace dev_err with dev_dbg.

---
 drivers/iommu/omap-iommu.c | 41 ++++++++++++++++++++++++--------------
 1 file changed, 26 insertions(+), 15 deletions(-)
drivers/iommu/omap-iommu.c +26 -15
index 9f0057ccea573..26a7803e4144f 100644
@@ -1392,9 +1392,6 @@ static int omap_iommu_attach_init(struct device *dev,
 	int i;
 
 	odomain->num_iommus = omap_iommu_count(dev);
-	if (!odomain->num_iommus)
-		return -ENODEV;
-
 	odomain->iommus = kcalloc(odomain->num_iommus, sizeof(*iommu),
 				  GFP_ATOMIC);
 	if (!odomain->iommus)
@@ -1431,6 +1428,31 @@ static void omap_iommu_detach_fini(struct omap_iommu_domain *odomain)
 	odomain->iommus = NULL;
 }
 
+static int omap_iommu_domain_test_dev(struct iommu_domain *domain,
+				      struct device *dev, ioasid_t pasid,
+				      struct iommu_domain *old)
+{
+	struct omap_iommu_arch_data *arch_data = dev_iommu_priv_get(dev);
+	struct omap_iommu_domain *omap_domain = to_omap_domain(domain);
+
+	if (!arch_data || !arch_data->iommu_dev) {
+		dev_dbg(dev, "device doesn't have an associated iommu\n");
+		return -ENODEV;
+	}
+
+	scoped_guard(spinlock, &omap_domain->lock) {
+		/* only a single client device can be attached to a domain */
+		if (omap_domain->dev) {
+			dev_dbg(dev, "iommu domain is already attached\n");
+			return -EINVAL;
+		}
+		if (!omap_iommu_count(dev))
+			return -ENODEV;
+	}
+
+	return 0;
+}
+
 static int omap_iommu_attach_dev(struct iommu_domain *domain,
 				 struct device *dev, struct iommu_domain *old)
 {
@@ -1441,20 +1463,8 @@ static int omap_iommu_attach_dev(struct iommu_domain *domain,
 	int ret = 0;
 	int i;
 
-	if (!arch_data || !arch_data->iommu_dev) {
-		dev_err(dev, "device doesn't have an associated iommu\n");
-		return -ENODEV;
-	}
-
 	spin_lock(&omap_domain->lock);
 
-	/* only a single client device can be attached to a domain */
-	if (omap_domain->dev) {
-		dev_err(dev, "iommu domain is already attached\n");
-		ret = -EINVAL;
-		goto out;
-	}
-
 	ret = omap_iommu_attach_init(dev, omap_domain);
 	if (ret) {
 		dev_err(dev, "failed to allocate required iommu data %d\n",
@@ -1724,6 +1734,7 @@ static const struct iommu_ops omap_iommu_ops = {
 	.device_group	= generic_single_device_group,
 	.of_xlate	= omap_iommu_of_xlate,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
+		.test_dev	= omap_iommu_domain_test_dev,
 		.attach_dev	= omap_iommu_attach_dev,
 		.map_pages	= omap_iommu_map,
 		.unmap_pages	= omap_iommu_unmap,
Signature · 1 line
2.43.0
#37 lore raw

PATCH 17/20 iommu/s390-iommu: Implement s390_iommu_domain_test_device

In reply to Nicolin Chen

Move sanity and compatibility tests from the attach_dev callback to the new test_dev callback function. The IOMMU core makes sure an attach_dev call must be invoked after a successful test_dev call.

---
 drivers/iommu/s390-iommu.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)
drivers/iommu/s390-iommu.c +15 -7
index 366e47978ac07..3c6141a4a1faf 100644
@@ -694,6 +694,20 @@ static int blocking_domain_attach_device(struct iommu_domain *domain,
 	return 0;
 }
 
+static int s390_iommu_domain_test_device(struct iommu_domain *domain,
+					 struct device *dev, ioasid_t pasid,
+					 struct iommu_domain *old)
+{
+	struct zpci_dev *zdev = to_zpci_dev(dev);
+
+	if (!zdev)
+		return -ENODEV;
+	if (WARN_ON(domain->geometry.aperture_start > zdev->end_dma ||
+		    domain->geometry.aperture_end < zdev->start_dma))
+		return -EINVAL;
+	return 0;
+}
+
 static int s390_iommu_attach_device(struct iommu_domain *domain,
 				    struct device *dev,
 				    struct iommu_domain *old)
@@ -704,13 +718,6 @@ static int s390_iommu_attach_device(struct iommu_domain *domain,
 	u8 status;
 	int cc;
 
-	if (!zdev)
-		return -ENODEV;
-
-	if (WARN_ON(domain->geometry.aperture_start > zdev->end_dma ||
-		domain->geometry.aperture_end < zdev->start_dma))
-		return -EINVAL;
-
 	blocking_domain_attach_device(&blocking_domain, dev);
 
 	/* If we fail now DMA remains blocked via blocking domain */
@@ -1177,6 +1184,7 @@ static struct iommu_domain blocking_domain = {
 	.device_group = generic_device_group, \
 	.get_resv_regions = s390_iommu_get_resv_regions, \
 	.default_domain_ops = &(const struct iommu_domain_ops) { \
+		.test_dev	= s390_iommu_domain_test_device, \
 		.attach_dev	= s390_iommu_attach_device, \
 		.map_pages	= s390_iommu_map_pages, \
 		.unmap_pages	= s390_iommu_unmap_pages, \
Signature · 1 line
2.43.0
maintainer Reviewed-by on v1 17/20
#38 lore raw

Re: [PATCH v1 17/20] iommu/s390-iommu: Implement s390_iommu_domain_test_device

Quoted 49 lines · Nicolin Chen

Move sanity and compatibility tests from the attach_dev callback to the new test_dev callback function. The IOMMU core makes sure an attach_dev call must be invoked after a successful test_dev call.

---
 drivers/iommu/s390-iommu.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)
drivers/iommu/s390-iommu.c +14 -7
index 366e47978ac07..3c6141a4a1faf 100644
@@ -694,6 +694,20 @@ static int blocking_domain_attach_device(struct iommu_domain *domain,
 	return 0;
 }
 
+static int s390_iommu_domain_test_device(struct iommu_domain *domain,
+					 struct device *dev, ioasid_t pasid,
+					 struct iommu_domain *old)
+{
+	struct zpci_dev *zdev = to_zpci_dev(dev);
+
+	if (!zdev)
+		return -ENODEV;
+	if (WARN_ON(domain->geometry.aperture_start > zdev->end_dma ||
+		    domain->geometry.aperture_end < zdev->start_dma))
+		return -EINVAL;
+	return 0;
+}
+
 static int s390_iommu_attach_device(struct iommu_domain *domain,
 				    struct device *dev,
 				    struct iommu_domain *old)
@@ -704,13 +718,6 @@ static int s390_iommu_attach_device(struct iommu_domain *domain,
 	u8 status;
 	int cc;
 
-	if (!zdev)
-		return -ENODEV;
-
-	if (WARN_ON(domain->geometry.aperture_start > zdev->end_dma ||
-		domain->geometry.aperture_end < zdev->start_dma))
-		return -EINVAL;
-
 	blocking_domain_attach_device(&blocking_domain, dev);
 
 	/* If we fail now DMA remains blocked via blocking domain */

drivers/iommu/s390-iommu.c static struct iommu_domain blocking_domain = { · line 1190

Quoted 7 lines
drivers/iommu/s390-iommu.c +1 -0
@@ -1177,6 +1184,7 @@ static struct iommu_domain blocking_domain = {
 	.device_group = generic_device_group, \
 	.get_resv_regions = s390_iommu_get_resv_regions, \
 	.default_domain_ops = &(const struct iommu_domain_ops) { \
+		.test_dev	= s390_iommu_domain_test_device, \
 		.attach_dev	= s390_iommu_attach_device, \
 		.map_pages	= s390_iommu_map_pages, \
 		.unmap_pages	= s390_iommu_unmap_pages, \

Looks good to me, thanks!

#39 lore raw

PATCH 18/20 iommufd/selftest: Implement mock_domain_nop_test

In reply to Nicolin Chen

Move sanity and compatibility tests from the attach_dev callback to the new test_dev callback function. The IOMMU core makes sure an attach_dev call must be invoked after a successful test_dev call.

---
 drivers/iommu/iommufd/selftest.c | 45 +++++++++++++++++++++++++-------
 1 file changed, 36 insertions(+), 9 deletions(-)
drivers/iommu/iommufd/selftest.c +36 -9
index 5661d2da2b679..f9c871a280b03 100644
@@ -58,6 +58,7 @@ enum {
 	MOCK_PFN_HUGE_IOVA = _MOCK_PFN_START << 2,
 };
 
+static struct iopf_queue *mock_iommu_iopf_queue;
 static int mock_dev_enable_iopf(struct device *dev, struct iommu_domain *domain);
 static void mock_dev_disable_iopf(struct device *dev, struct iommu_domain *domain);
 
@@ -215,6 +216,37 @@ static inline struct selftest_obj *to_selftest_obj(struct iommufd_object *obj)
 	return container_of(obj, struct selftest_obj, obj);
 }
 
+static int mock_domain_nop_test(struct iommu_domain *domain, struct device *dev,
+				ioasid_t pasid, struct iommu_domain *old)
+{
+	struct mock_dev *mdev = to_mock_dev(dev);
+
+	if (domain->dirty_ops && (mdev->flags & MOCK_FLAGS_DEVICE_NO_DIRTY))
+		return -EINVAL;
+
+	iommu_group_mutex_assert(dev);
+	if (domain->type == IOMMU_DOMAIN_NESTED) {
+		struct mock_viommu *viommu =
+			to_mock_nested(domain)->mock_viommu;
+		unsigned long vdev_id = 0;
+		int rc;
+
+		if (viommu) {
+			rc = iommufd_viommu_get_vdev_id(&viommu->core, dev,
+							&vdev_id);
+			if (rc)
+				return rc;
+		}
+	}
+
+	if (domain->iopf_handler) {
+		if (!mock_iommu_iopf_queue)
+			return -ENODEV;
+	}
+
+	return 0;
+}
+
 static int mock_domain_nop_attach(struct iommu_domain *domain,
 				  struct device *dev, struct iommu_domain *old)
 {
@@ -223,16 +255,13 @@ static int mock_domain_nop_attach(struct iommu_domain *domain,
 	unsigned long vdev_id = 0;
 	int rc;
 
-	if (domain->dirty_ops && (mdev->flags & MOCK_FLAGS_DEVICE_NO_DIRTY))
-		return -EINVAL;
-
 	iommu_group_mutex_assert(dev);
 	if (domain->type == IOMMU_DOMAIN_NESTED) {
 		new_viommu = to_mock_nested(domain)->mock_viommu;
 		if (new_viommu) {
 			rc = iommufd_viommu_get_vdev_id(&new_viommu->core, dev,
 							&vdev_id);
-			if (rc)
+			if (WARN_ON(rc))
 				return rc;
 		}
 	}
@@ -296,6 +325,7 @@ static int mock_domain_set_dev_pasid_nop(struct iommu_domain *domain,
 }
 
 static const struct iommu_domain_ops mock_blocking_ops = {
+	.test_dev = mock_domain_nop_test,
 	.attach_dev = mock_domain_nop_attach,
 	.set_dev_pasid = mock_domain_set_dev_pasid_nop
 };
@@ -630,8 +660,6 @@ static bool mock_domain_capable(struct device *dev, enum iommu_cap cap)
 	return false;
 }
 
-static struct iopf_queue *mock_iommu_iopf_queue;
-
 static struct mock_iommu_device {
 	struct iommu_device iommu_dev;
 	struct completion complete;
@@ -658,9 +686,6 @@ static int mock_dev_enable_iopf(struct device *dev, struct iommu_domain *domain)
 	if (!domain || !domain->iopf_handler)
 		return 0;
 
-	if (!mock_iommu_iopf_queue)
-		return -ENODEV;
-
 	if (mdev->iopf_refcount) {
 		mdev->iopf_refcount++;
 		return 0;
@@ -958,6 +983,7 @@ static const struct iommu_ops mock_ops = {
 	.default_domain_ops =
 		&(struct iommu_domain_ops){
 			.free = mock_domain_free,
+			.test_dev = mock_domain_nop_test,
 			.attach_dev = mock_domain_nop_attach,
 			.map_pages = mock_domain_map_pages,
 			.unmap_pages = mock_domain_unmap_pages,
@@ -1021,6 +1047,7 @@ mock_domain_cache_invalidate_user(struct iommu_domain *domain,
 
 static struct iommu_domain_ops domain_nested_ops = {
 	.free = mock_domain_free_nested,
+	.test_dev = mock_domain_nop_test,
 	.attach_dev = mock_domain_nop_attach,
 	.cache_invalidate_user = mock_domain_cache_invalidate_user,
 	.set_dev_pasid = mock_domain_set_dev_pasid_nop,
Signature · 1 line
2.43.0
#40 lore raw

PATCH 19/20 iommu/virtio-iommu: Implement viommu_domain_test_dev

In reply to Nicolin Chen

Move sanity and compatibility tests from the attach_dev callback to the new test_dev callback function. The IOMMU core makes sure an attach_dev call must be invoked after a successful test_dev call.

---
 drivers/iommu/virtio-iommu.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)
drivers/iommu/virtio-iommu.c +13 -3
index d314fa5cd8476..766dded8fc8a6 100644
@@ -730,6 +730,18 @@ static struct iommu_domain *viommu_domain_alloc_identity(struct device *dev)
 	return domain;
 }
 
+static int viommu_domain_test_dev(struct iommu_domain *domain,
+				  struct device *dev, ioasid_t pasid,
+				  struct iommu_domain *old)
+{
+	struct viommu_endpoint *vdev = dev_iommu_priv_get(dev);
+	struct viommu_domain *vdomain = to_viommu_domain(domain);
+
+	if (vdomain->viommu != vdev->viommu)
+		return -EINVAL;
+	return 0;
+}
+
 static int viommu_attach_dev(struct iommu_domain *domain, struct device *dev,
 			     struct iommu_domain *old)
 {
@@ -738,9 +750,6 @@ static int viommu_attach_dev(struct iommu_domain *domain, struct device *dev,
 	struct viommu_endpoint *vdev = dev_iommu_priv_get(dev);
 	struct viommu_domain *vdomain = to_viommu_domain(domain);
 
-	if (vdomain->viommu != vdev->viommu)
-		return -EINVAL;
-
 	/*
 	 * In the virtio-iommu device, when attaching the endpoint to a new
 	 * domain, it is detached from the old one and, if as a result the
@@ -1099,6 +1108,7 @@ static const struct iommu_ops viommu_ops = {
 	.of_xlate		= viommu_of_xlate,
 	.owner			= THIS_MODULE,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
+		.test_dev		= viommu_domain_test_dev,
 		.attach_dev		= viommu_attach_dev,
 		.map_pages		= viommu_map_pages,
 		.unmap_pages		= viommu_unmap_pages,
Signature · 1 line
2.43.0
#41 lore raw

PATCH 20/20 iommu/tegra-smmu: Implement tegra_smmu_domain_test_dev

In reply to Nicolin Chen

Move sanity and compatibility tests from the attach_dev callback to the new test_dev callback function. The IOMMU core makes sure an attach_dev call must be invoked after a successful test_dev call.

---
 drivers/iommu/tegra-smmu.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)
drivers/iommu/tegra-smmu.c +14 -9
index 336e0a3ff41fb..cfbe67678a426 100644
@@ -489,6 +489,18 @@ static void tegra_smmu_as_unprepare(struct tegra_smmu *smmu,
 	mutex_unlock(&smmu->lock);
 }
 
+static int tegra_smmu_domain_test_dev(struct iommu_domain *domain,
+				      struct device *dev, ioasid_t pasid,
+				      struct iommu_domain *old)
+{
+	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
+
+	if (!fwspec || !fwspec->num_ids)
+		return -ENOENT;
+
+	return 0;
+}
+
 static int tegra_smmu_attach_dev(struct iommu_domain *domain,
 				 struct device *dev, struct iommu_domain *old)
 {
@@ -498,9 +510,6 @@ static int tegra_smmu_attach_dev(struct iommu_domain *domain,
 	unsigned int index;
 	int err;
 
-	if (!fwspec)
-		return -ENOENT;
-
 	for (index = 0; index < fwspec->num_ids; index++) {
 		err = tegra_smmu_as_prepare(smmu, as);
 		if (err)
@@ -509,9 +518,6 @@ static int tegra_smmu_attach_dev(struct iommu_domain *domain,
 		tegra_smmu_enable(smmu, fwspec->ids[index], as->id);
 	}
 
-	if (index == 0)
-		return -ENODEV;
-
 	return 0;
 
 disable:
@@ -532,9 +538,6 @@ static int tegra_smmu_identity_attach(struct iommu_domain *identity_domain,
 	struct tegra_smmu *smmu;
 	unsigned int index;
 
-	if (!fwspec)
-		return -ENODEV;
-
 	if (old == identity_domain || !old)
 		return 0;
 
@@ -548,6 +551,7 @@ static int tegra_smmu_identity_attach(struct iommu_domain *identity_domain,
 }
 
 static struct iommu_domain_ops tegra_smmu_identity_ops = {
+	.test_dev = tegra_smmu_domain_test_dev,
 	.attach_dev = tegra_smmu_identity_attach,
 };
 
@@ -1005,6 +1009,7 @@ static const struct iommu_ops tegra_smmu_ops = {
 	.device_group = tegra_smmu_device_group,
 	.of_xlate = tegra_smmu_of_xlate,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
+		.test_dev	= tegra_smmu_domain_test_dev,
 		.attach_dev	= tegra_smmu_attach_dev,
 		.map_pages	= tegra_smmu_map,
 		.unmap_pages	= tegra_smmu_unmap,
Signature · 1 line
2.43.0
maintainer
#42 lore raw
In reply to Nicolin Chen
Quoted 4 lines · Nicolin Chen

Add a new test_dev domain op for drivers to run a compatibility test prior to the actual attachment at the driver level. Any incompatible attachment will be rejected early, allowing the iommu core to postpone any concurrent attachment during a device reset state.

I had to go back and find the original email from kevin to understand this..

This is a preparatory series for new iommu_dev_reset APIs:
https://lore.kernel.org/all/[email protected]/

That series parks the domain attachment at BLOCKED during a device reset. To keep the uAPI this also required that any change in domain during this reset sequence is just recorded and kept in the background until the reset is finished.

This creates a weird hole where userspace could propose to attach to a domain that is incompatible with the device during FLR, have that attach queued and then ultimately have the domain attach fail when the FLR concludes.

This can be mitigated by splitting out the compatability test from the attach and having the core code check the compatability before accepting a queued domain attach.

It was felt that the subtle uAPI change warrants this rework.

Jason