Thread (43 messages) 43 messages, 4 authors, 2025-11-07

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

From: Nicolin Chen <hidden>
Date: 2025-10-13 17:22:42
Also in: asahi, linux-arm-msm, linux-iommu, linux-mediatek, linux-patches, linux-riscv, linux-rockchip, linux-s390, linux-sunxi, linux-tegra, lkml, virtualization

Hi Niklas,

On Mon, Oct 13, 2025 at 11:53:55AM +0200, Niklas Schnelle wrote:
On Sun, 2025-10-12 at 17:04 -0700, Nicolin Chen wrote:
quoted
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.
iommu_deferred_attach() invokes __iommu_attach_device(), so it is
already included in the list :)
quoted
 /**
  * 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
+ *            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 hunk ↗ jump to hunk
--- snip ---
quoted
@@ -3615,6 +3657,11 @@ int iommu_replace_device_pasid(struct iommu_domain *domain,
 	ret = 0;
 
 	if (curr_domain != domain) {
+		ret = __iommu_domain_test_device(domain, dev, pasid,
+						 curr_domain);
+		if (ret)
+			goto out_unlock;
+
 		ret = __iommu_set_group_pasid(domain, group,
 					      pasid, curr_domain);
 		if (ret)
Apart from the comment and commit description nits mentioned above this
looks good to me.

Reviewed-by: Niklas Schnelle <schnelle@linux.ibm.com>
Thanks for the review!
Nicolin
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help