Disable ATS via iommu during PCI resets
Subsystems (26)
v4 cover Disable ATS via iommu during PCI resets
Hi all,
PCIe permits a device to ignore ATS invalidation TLPs, while processing a reset. This creates a problem visible to the OS where an ATS invalidation command will time out: e.g. an SVA domain will have no coordination with a reset event and can racily issue ATS invalidations to a resetting device.
The OS should do something to mitigate this as we do not want production systems to be reporting critical ATS failures, especially in a hypervisor environment. Broadly, OS could arrange to ignore the timeouts, block page table mutations to prevent invalidations, or disable and block ATS.
The PCIe spec in sec 10.3.1 IMPLEMENTATION NOTE recommends to disable and block ATS before initiating a Function Level Reset. It also mentions that other reset methods could have the same vulnerability as well.
Provide a callback from the PCI subsystem that will enclose the reset and have the iommu core temporarily change domains to group->blocking_domain, so IOMMU drivers would fence any incoming ATS queries, synchronously stop issuing new ATS invalidations, and wait for existing ATS invalidations to complete. Doing this can avoid any ATS invaliation timeouts.
When a device is resetting, any new domain attachment has to be rejected, until the reset is finished, to prevent ATS activity from being activated between the two callback functions. Introduce a new pending_reset flag to reject a concurrent __iommu_attach_device/set_group_pasid().
Finally, apply these iommu_dev_reset_prepare/done() functions in the PCI reset functions.
Note that this series doesn't work well for a resetting alias device or a SRIOV PF, so skip these two corner cases. There is nothing we can do for alias devices since they share the same RID. For SRIOV PF, its VFs would need to be blocked as well, and new dryrun attach_dev/set_group_pasid ops will be required to allow compatible domain to be cached concurrently.
Some future followups after this series:
- A pair of dryrun testing ops for attach_dev/set_dev_pasid to make sure no incompatible attempt will be given to attach_dev/set_dev_pasid().
- Stage all VFs to the blocked domain as well, if their PF is resetting.
- Clean up all unlocked iommu_get_domain_for_dev() call for UAF concern. Replace with safer alternative APIs.
This is on Github:
https://github.com/nicolinc/iommufd/commits/iommu_dev_reset-v4
Changelog
v4
* Add Reviewed-by from Baolu
* [iommu] Use guard(mutex)
* [iommu] Update kdocs for typos and revisings
* [iommu] Skip two corner cases (alias and SRIOV)
* [iommu] Rework attach_dev to pass in old domain pointer
* [iommu] Reject concurrent attach_dev/set_dev_pasid for compatibility concern
* [smmuv3] Drop the old_domain depedency in its release_dev callback
* [pci] Add pci_reset_iommu_prepare/_done() wrappers checking ATS cap
v3
https://lore.kernel.org/all/[email protected]/
* Add Reviewed-by from Jason
* [iommu] Add a fast return in iommu_deferred_attach()
* [iommu] Update kdocs, inline comments, and commit logs
* [iommu] Use group->blocking_domain v.s. ops->blocked_domain
* [iommu] Drop require_direct, iommu_group_get(), and xa_lock()
* [iommu] Set the pending_reset flag after RID/PASID domain setups
* [iommu] Do not bypass PASID domains when RID domain is already the blocking_domain
* [iommu] Add iommu_get_domain_for_dev_locked to correctly return the blocking_domain
v2
https://lore.kernel.org/all/[email protected]/
* [iommu] Update kdocs, inline comments, and commit logs
* [iommu] Replace long-holding group->mutex with a pending_reset flag
* [pci] Abort reset routines if iommu_dev_reset_prepare() fails
* [pci] Apply the same vulnerability fix to other reset functions
v1
https://lore.kernel.org/all/[email protected]/
Thanks
Nicolin
Nicolin Chen (7):
iommu/arm-smmu-v3: Add release_domain to attach prior to release_dev()
iommu: Lock group->mutex in iommu_deferred_attach()
iommu: Pass in gdev to __iommu_device_set_domain
iommu: Pass in old domain to attach_dev callback functions
iommu: Add iommu_get_domain_for_dev_locked() helper
iommu: Introduce iommu_dev_reset_prepare() and iommu_dev_reset_done()
pci: Suspend iommu function prior to resetting a device
drivers/pci/pci.h | 2 + include/linux/iommu.h | 16 +- drivers/iommu/amd/iommu.c | 11 +- drivers/iommu/apple-dart.c | 9 +- .../arm/arm-smmu-v3/arm-smmu-v3-iommufd.c | 5 +- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 66 +++-- drivers/iommu/arm/arm-smmu/arm-smmu.c | 9 +- drivers/iommu/arm/arm-smmu/qcom_iommu.c | 11 +- drivers/iommu/dma-iommu.c | 2 +- drivers/iommu/exynos-iommu.c | 6 +- drivers/iommu/fsl_pamu_domain.c | 12 +- drivers/iommu/intel/iommu.c | 10 +- drivers/iommu/intel/nested.c | 2 +- drivers/iommu/iommu.c | 256 +++++++++++++++++- drivers/iommu/iommufd/selftest.c | 2 +- drivers/iommu/ipmmu-vmsa.c | 10 +- drivers/iommu/msm_iommu.c | 8 +- drivers/iommu/mtk_iommu.c | 8 +- drivers/iommu/mtk_iommu_v1.c | 7 +- drivers/iommu/omap-iommu.c | 12 +- drivers/iommu/riscv/iommu.c | 9 +- drivers/iommu/rockchip-iommu.c | 20 +- drivers/iommu/s390-iommu.c | 9 +- drivers/iommu/sprd-iommu.c | 3 +- drivers/iommu/sun50i-iommu.c | 8 +- drivers/iommu/tegra-smmu.c | 10 +- drivers/iommu/virtio-iommu.c | 6 +- drivers/pci/pci-acpi.c | 12 +- drivers/pci/pci.c | 68 ++++- drivers/pci/quirks.c | 18 +- 30 files changed, 509 insertions(+), 118 deletions(-)
Signature · 1 line
2.43.0
PATCH v4 1/7 iommu/arm-smmu-v3: Add release_domain to attach prior to release_dev()
The iommu_get_domain_for_dev() helper will be reworked to check a per-gdv flag, so it will need to hold the group->mutex. This will give trouble to existing attach_dev callback functions that call the helper for currently attached old domains, since group->mutex is already held in an attach_dev context.
To address this, step one is to pass in the attached "old" domain pointer to the attach_dev op, similar to set_dev_pasid op.
However, the release_dev op is tricky in the iommu core, because it could be invoked when the group isn't allocated, i.e. no way of guarateeing the group->mutex being held. Thus, it would not be able to do any attachment in the release_dev callback function, arm_smmu_release_device() here.
Add a release_domain, moving the attach from arm_smmu_release_device() to the iommu_deinit_device() in the core, so that arm_smmu_release_device() will not need to worry about the group->mutex.
--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 34 ++++++++++++++++----- 1 file changed, 26 insertions(+), 8 deletions(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +26 -8
@@ -3291,6 +3291,31 @@ static struct iommu_domain arm_smmu_blocked_domain = { .ops = &arm_smmu_blocked_ops, }; +static int arm_smmu_attach_dev_release (struct iommu_domain *domain, +struct device *dev) +{ +struct arm_smmu_master *master =dev_iommu_priv_get (dev); + +WARN_ON (master->iopf_refcount); + +/* Put the STE back to what arm_smmu_init_strtab() sets */ +if (dev->iommu->require_direct) +arm_smmu_attach_dev_identity (&arm_smmu_identity_domain, dev); +else +arm_smmu_attach_dev_blocked (&arm_smmu_blocked_domain, dev); + +return 0 ; +} + +static const struct iommu_domain_ops arm_smmu_release_ops = { + .attach_dev = arm_smmu_attach_dev_release, +}; + +static struct iommu_domain arm_smmu_release_domain = { + .type =IOMMU_DOMAIN_BLOCKED , + .ops = &arm_smmu_release_ops, +}; +static struct iommu_domain *arm_smmu_domain_alloc_paging_flags (struct device *dev,u32 flags,const struct iommu_user_data *user_data)
@@ -3580,14 +3605,6 @@ static void arm_smmu_release_device(struct device *dev) {struct arm_smmu_master *master =dev_iommu_priv_get (dev); -WARN_ON (master->iopf_refcount); - -/* Put the STE back to what arm_smmu_init_strtab() sets */ -if (dev->iommu->require_direct) -arm_smmu_attach_dev_identity (&arm_smmu_identity_domain, dev); -else -arm_smmu_attach_dev_blocked (&arm_smmu_blocked_domain, dev); -arm_smmu_disable_pasid (master);arm_smmu_remove_master (master);if (arm_smmu_cdtab_allocated (&master->cd_table))
@@ -3678,6 +3695,7 @@ static int arm_smmu_def_domain_type(struct device *dev)static const struct iommu_ops arm_smmu_ops = { .identity_domain = &arm_smmu_identity_domain, .blocked_domain = &arm_smmu_blocked_domain, + .release_domain = &arm_smmu_release_domain, .capable = arm_smmu_capable, .hw_info = arm_smmu_hw_info, .domain_alloc_sva = arm_smmu_sva_domain_alloc,
Signature · 1 line
2.43.0
Tian, Kevin
maintainerRE: [PATCH v4 1/7] iommu/arm-smmu-v3: Add release_domain to attach prior to release_dev()
Quoted 17 lines
From: Nicolin Chen <[email protected]>
Sent: Monday, September 1, 2025 7:32 AMdrivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +10
-0+static int arm_smmu_attach_dev_release (struct iommu_domain *domain, +struct device *dev) +{ +struct arm_smmu_master *master =dev_iommu_priv_get (dev); + +WARN_ON (master->iopf_refcount); + +/* Put the STE back to what arm_smmu_init_strtab() sets */ +if (dev->iommu->require_direct) +arm_smmu_attach_dev_identity(&arm_smmu_identity_domain,
dev);(quoted diff) +2
-0+ else +
Quoted 2 lines
arm_smmu_attach_dev_blocked(&arm_smmu_blocked_domain,
dev);
it's a bit confusing that a BLOCKED domain type could turn to the identity mode, though this movement doesn't change the original behavior.
Quoted 13 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +13
-0+ +return 0 ; +} + +static const struct iommu_domain_ops arm_smmu_release_ops = { + .attach_dev = arm_smmu_attach_dev_release, +}; + +static struct iommu_domain arm_smmu_release_domain = { + .type =IOMMU_DOMAIN_BLOCKED , + .ops = &arm_smmu_release_ops, +}; +
Jason Gunthorpe
maintainerRe: [PATCH v4 1/7] iommu/arm-smmu-v3: Add release_domain to attach prior to release_dev()
Quoted 9 lines · Kevin
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static struct iommu_domain arm_smmu_blocked_domain = { · line 3299
Quoted 9 lines
From: Nicolin Chen <[email protected]>
Sent: Monday, September 1, 2025 7:32 AMdrivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +6
-0+static int arm_smmu_attach_dev_release (struct iommu_domain *domain, +struct device *dev) +{ +struct arm_smmu_master *master =dev_iommu_priv_get (dev); + +WARN_ON (master->iopf_refcount);
This doesn't look right anymore..
Now that iopf is managed automatically it technically doesn't go to zero until the attaches below:
Quoted 10 lines
Quoted 10 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +4
-0+ +/* Put the STE back to what arm_smmu_init_strtab() sets */ +if (dev->iommu->require_direct) +arm_smmu_attach_dev_identity(&arm_smmu_identity_domain,
dev);(quoted diff) +2
-0+ else +arm_smmu_attach_dev_blocked(&arm_smmu_blocked_domain,
dev);
And I'd argue the attaches internally should have the assertion. If no pasids and blocked/identity the iopf == 0.
Quoted 3 lines
it's a bit confusing that a BLOCKED domain type could turn to the identity mode, though this movement doesn't change the original behavior.
That isn't what is happening here..
If dev->iommu->require_direct is set we prevent attaching BLOCKING domains entirely:
if (dev->iommu->require_direct &&
(new_domain->type == IOMMU_DOMAIN_BLOCKED ||
new_domain == group->blocking_domain)) {
dev_warn(dev,
"Firmware has requested this device have a 1:1 IOMMU mapping, rejecting configuring the device without a 1:1 mapping. Contact your platform vendor.\n");
return -EINVAL;
}So in most sane cases the above will never convert BLOCKING to IDENTITY. What it is doing is preserving the RMRs...
Also, I don't think this should be in the smmu driver, every driver should have this same logic, it is part of the definition of RMR Let's put it in the core code:
if (!dev->iommu->attach_deferred && ops->release_domain) {
struct iommu_domain *release_domain = ops->release_domain;
/*
* If the device requires direct mappings then it should not
* be parked on a BLOCKED domain during release as that would
* break the direct mappings.
*/
if (dev->iommu->require_direct && ops->identity_domain &&
release_domain == ops->blocked_domain)
release_domain = ops->identity_domain;
release_domain->ops->attach_dev(release_domain, dev);
}Jason
Re: [PATCH v4 1/7] iommu/arm-smmu-v3: Add release_domain to attach prior to release_dev()
Quoted 13 lines · Jason Gunthorpe
Quoted 9 lines · Kevin
Review
Quoted 9 lines
From: Nicolin Chen <[email protected]>
Sent: Monday, September 1, 2025 7:32 AM(quoted diff) +6
-0+static int arm_smmu_attach_dev_release(struct iommu_domain *domain, + struct device *dev) +{ + struct arm_smmu_master *master = dev_iommu_priv_get(dev); + + WARN_ON(master->iopf_refcount);This doesn't look right anymore..
Quoted 2 lines
Now that iopf is managed automatically it technically doesn't go to zero until the attaches below:
I will leave this WARN_ON in the arm_smmu_release_device(), while having a release_domain to call arm_smmu_attach_dev_blocked():
-----------------------------------------------------------------
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +11 -6
@@ -3291,6 +3291,16 @@ static struct iommu_domain arm_smmu_blocked_domain = { .ops = &arm_smmu_blocked_ops, }; +/* Same as arm_smmu_blocked_ops but less set_dev_pasid */ +static const struct iommu_domain_ops arm_smmu_release_ops = { + .attach_dev = arm_smmu_attach_dev_blocked, +}; + +static struct iommu_domain arm_smmu_release_domain = { + .type =IOMMU_DOMAIN_BLOCKED , + .ops = &arm_smmu_release_ops, +}; +static struct iommu_domain *arm_smmu_domain_alloc_paging_flags (struct device *dev,u32 flags,const struct iommu_user_data *user_data)
@@ -3582,12 +3592,6 @@ static void arm_smmu_release_device(struct device *dev)WARN_ON (master->iopf_refcount); -/* Put the STE back to what arm_smmu_init_strtab() sets */ -if (dev->iommu->require_direct) -arm_smmu_attach_dev_identity (&arm_smmu_identity_domain, dev); -else -arm_smmu_attach_dev_blocked (&arm_smmu_blocked_domain, dev); -arm_smmu_disable_pasid (master);arm_smmu_remove_master (master);if (arm_smmu_cdtab_allocated (&master->cd_table))
@@ -3678,6 +3682,7 @@ static int arm_smmu_def_domain_type(struct device *dev)static const struct iommu_ops arm_smmu_ops = { .identity_domain = &arm_smmu_identity_domain, .blocked_domain = &arm_smmu_blocked_domain, + .release_domain = &arm_smmu_release_domain, .capable = arm_smmu_capable, .hw_info = arm_smmu_hw_info, .domain_alloc_sva = arm_smmu_sva_domain_alloc,
-----------------------------------------------------------------
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
(quoted diff) +4
-0+ + /* Put the STE back to what arm_smmu_init_strtab() sets */ + if (dev->iommu->require_direct) +arm_smmu_attach_dev_identity(&arm_smmu_identity_domain,
Quoted 8 lines
Quoted 5 lines
Quoted 5 lines
dev);
(quoted diff) +2
-0+ else +arm_smmu_attach_dev_blocked(&arm_smmu_blocked_domain,
dev);And I'd argue the attaches internally should have the assertion. If no pasids and blocked/identity the iopf == 0.
Ack. I will try a separate SMMU patch from this series.
Quoted 3 lines
Also, I don't think this should be in the smmu driver, every driver should have this same logic, it is part of the definition of RMR Let's put it in the core code:
Ack. Adding this patch prior to the SMMU release_domain:
-----------------------------------------------------------------
From: Jason Gunthorpe <[email protected]>
Date: Fri, 19 Sep 2025 22:26:45 +0000
Subject: [PATCH] iommu: Use identity_domain as release_domain for
require_direct
If dev->iommu->require_direct is set, the core prevent attaching a BLOCKED domains entirely in __iommu_device_set_domain():
if (dev->iommu->require_direct &&
(new_domain->type == IOMMU_DOMAIN_BLOCKED ||
new_domain == group->blocking_domain)) {
dev_warn(dev, "....");
return -EINVAL;
}Thus, in most sane cases, the above will never convert BLOCKED to IDENTITY in order to preserve the RMRs (direct mappings).
A similar situation would happen to the release_domain: while driver might have set it to a BLOCKED domain, replace it with an IDENTITY for RMRs.
--- drivers/iommu/iommu.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)
drivers/iommu/iommu.c +14 -2
@@ -516,8 +516,20 @@ static void iommu_deinit_device(struct device *dev)* Regardless, if a delayed attach never occurred, then the release * should still avoid touching any hardware configuration either. */ -if (!dev->iommu->attach_deferred && ops->release_domain) - ops->release_domain->ops->attach_dev (ops->release_domain, dev); +if (!dev->iommu->attach_deferred && ops->release_domain) { +struct iommu_domain *release_domain = ops->release_domain; + +/* +* If the device requires direct mappings then it should not +* be parked on a BLOCKED domain during release as that would +* break the direct mappings. +*/ +if (dev->iommu->require_direct && ops->identity_domain && + release_domain == ops->blocked_domain) + release_domain = ops->identity_domain; + + release_domain->ops->attach_dev (release_domain, dev); + }if (ops->release_device) ops->release_device (dev);
Signature · 6 lines
2.43.0 ----------------------------------------------------------------- Thanks Nicolin
Jason Gunthorpe
maintainerRe: [PATCH v4 1/7] iommu/arm-smmu-v3: Add release_domain to attach prior to release_dev()
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static struct iommu_domain arm_smmu_blocked_domain = { · line 3297
Quoted 4 lines · Nicolin Chen
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +4
-0+/* Same as arm_smmu_blocked_ops but less set_dev_pasid */ +static const struct iommu_domain_ops arm_smmu_release_ops = { + .attach_dev = arm_smmu_attach_dev_blocked, +};
don't worry about set_dev_pasid for the release domain, it is never called anyhow.
The intention is to just use identity or blocked domains as is.
Jason
Quoted 16 lines
-----------------------------------------------------------------
From: Jason Gunthorpe <[email protected]>
Date: Fri, 19 Sep 2025 22:26:45 +0000
Subject: [PATCH] iommu: Useidentity_domainasrelease_domainfor
require_directIf
dev->iommu->require_directis set, the core prevent attaching a BLOCKED domains entirely in__iommu_device_set_domain():if (dev->iommu->require_direct && (new_domain->type == IOMMU_DOMAIN_BLOCKED || new_domain == group->blocking_domain)) { dev_warn(dev, "...."); return -EINVAL; }
Quoted 5 lines
Thus, in most sane cases, the above will never convert BLOCKED to IDENTITY in order to preserve the RMRs (direct mappings).
A similar situation would happen to the
release_domain: while driver might have set it to a BLOCKED domain, replace it with an IDENTITY for RMRs.
How about
[PATCH] iommu: Generic support for RMRs during device release
Generally an IOMMU driver should leave the translation as BLOCKED until the translation entry is probed onto a struct device. When the struct device is removed the translation should be put back to BLOCKED.
Drivers that are able to work like this can set their release_domain to their blocking domain and the core code handles this work.
The exception is if the device has a IOMMU_RESV_DIRECT region. In this case the OS should continuously allow translation for the given range. The core code generally prevents using a BLOCKED domain with this device.
Continue this logic for the device release and hoist some open coding from drivers. If the device has dev->iommu->require_direct and the driver is using a BLOCKED release_domain override this to IDENTITY to preserve the semantics.
The only remaining required driver code for IOMMU_RESV_DIRECT should preset an IDENTITY translation during early IOMMU startup for those devices.
Jason
Re: [PATCH v4 1/7] iommu/arm-smmu-v3: Add release_domain to attach prior to release_dev()
Quoted 10 lines · Jason Gunthorpe
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static struct iommu_domain arm_smmu_blocked_domain = { · line 3297
Quoted 4 lines · Nicolin Chen
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +4
-0+/* Same as arm_smmu_blocked_ops but less set_dev_pasid */ +static const struct iommu_domain_ops arm_smmu_release_ops = { + .attach_dev = arm_smmu_attach_dev_blocked, +};don't worry about
set_dev_pasidfor the release domain, it is never called anyhow.The intention is to just use identity or blocked domains as is.
Ah, I see AMD separate release_domain from blocked_domain..
OK. Will just set it to blocked_domain. And will update the commit message as well.
Thanks
Nicolin
Jason Gunthorpe
maintainerRe: [PATCH v4 1/7] iommu/arm-smmu-v3: Add release_domain to attach prior to release_dev()
Quoted 10 lines · Nicolin Chen
Quoted 9 lines · Jason Gunthorpe
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static struct iommu_domain arm_smmu_blocked_domain = { · line 3297
Quoted 4 lines · Nicolin Chen
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +4
-0+/* Same as arm_smmu_blocked_ops but less set_dev_pasid */ +static const struct iommu_domain_ops arm_smmu_release_ops = { + .attach_dev = arm_smmu_attach_dev_blocked, +};don't worry about
set_dev_pasidfor the release domain, it is never called anyhow.
Quoted 3 lines
Quoted 1 line
The intention is to just use identity or blocked domains as is.
Ah, I see AMD separate
release_domainfromblocked_domain..
Send a patch for AMD since you found it?
Jason
Re: [PATCH v4 1/7] iommu/arm-smmu-v3: Add release_domain to attach prior to release_dev()
Quoted 11 lines · Jason Gunthorpe
Quoted 10 lines · Nicolin Chen
Quoted 9 lines · Jason Gunthorpe
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static struct iommu_domain arm_smmu_blocked_domain = { · line 3297
Quoted 4 lines · Nicolin Chen
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +4
-0+/* Same as arm_smmu_blocked_ops but less set_dev_pasid */ +static const struct iommu_domain_ops arm_smmu_release_ops = { + .attach_dev = arm_smmu_attach_dev_blocked, +};don't worry about
set_dev_pasidfor the release domain, it is never called anyhow.
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
The intention is to just use identity or blocked domains as is.
Ah, I see AMD separate
release_domainfromblocked_domain..Send a patch for AMD since you found it?
Yea, sure.
Nicolin
PATCH v4 2/7 iommu: Lock group->mutex in iommu_deferred_attach()
In reply to Nicolin ChenThe 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 verify a new flag stored in the struct group_device. To iterate the gdev list, the group->mutex should be held for this matter too.
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
@@ -2144,10 +2144,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); +/* +* 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); }void iommu_detach_device (struct iommu_domain *domain,struct device *dev)
Signature · 1 line
2.43.0
RE: [PATCH v4 2/7] iommu: Lock group->mutex in iommu_deferred_attach()
Quoted 15 lines
From: Nicolin Chen <[email protected]>
Sent: Monday, September 1, 2025 7:32 AMThe
iommu_deferred_attach()function invokes__iommu_attach_device()while
not holding thegroup->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 thegroup->mutex-- some of them use theiommu_group_mutex_assert()function that could be potentially in the path of anattach_devcallback function invoked by the__iommu_attach_device().
Quoted 8 lines
The
iommu_deferred_attach()will soon need to verify a new flag stored in the structgroup_device. To iterate the gdev list, thegroup->mutexshould be held for this matter too.So, grab the mutex to guard
__iommu_attach_device()like other callers.Reviewed-by Jason Gunthorpe <[email protected]>Signed-off-by Nicolin Chen <[email protected]>
PATCH v4 3/7 iommu: Pass in gdev to __iommu_device_set_domain
In reply to Nicolin ChenThe device under the reset will be attached to a blocked domain, while not updating the group->domain pointer. So there needs to be a per-device flag to indicate the reset state, for other iommu core functions to check so as not to shift the attached domain during the reset state.
The regular device pointer can't store any private iommu flag. So the flag has to be in the gdev structure.
Pass in the gdev pointer instead to the functions that will check that per device flag.
--- drivers/iommu/iommu.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
drivers/iommu/iommu.c +6 -6
@@ -112,7 +112,7 @@ enum { };static int __iommu_device_set_domain (struct iommu_group *group, -struct device *dev, +struct group_device *gdev,struct iommu_domain *new_domain,unsigned int flags); static int __iommu_group_set_domain_internal(struct iommu_group *group,
@@ -602,7 +602,7 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_listif (group->default_domain)iommu_create_device_direct_mappings (group->default_domain, dev);if (group->domain) { - ret =__iommu_device_set_domain (group, dev, group->domain,0 ); + ret =__iommu_device_set_domain (group, gdev, group->domain,0 );if (ret) goto err_remove_gdev; }else if (!group->default_domain && !group_list) {
@@ -2263,10 +2263,11 @@ int iommu_attach_group(struct iommu_domain *domain, struct iommu_group *group)EXPORT_SYMBOL_GPL (iommu_attach_group);static int __iommu_device_set_domain (struct iommu_group *group, -struct device *dev, +struct group_device *gdev,struct iommu_domain *new_domain,unsigned int flags) { +struct device *dev = gdev->dev;int ret;/*
@@ -2346,8 +2347,7 @@ static int __iommu_group_set_domain_internal(struct iommu_group *group,*/ result =0 ;for_each_group_device (group, gdev) { - ret =__iommu_device_set_domain (group, gdev->dev, new_domain, - flags); + ret =__iommu_device_set_domain (group, gdev, new_domain, flags);if (ret) { result = ret;/*
@@ -2379,7 +2379,7 @@ static int __iommu_group_set_domain_internal(struct iommu_group *group,*/ if (group->domain)WARN_ON (__iommu_device_set_domain ( - group, gdev->dev, group->domain, + group, gdev, group->domain,IOMMU_SET_DOMAIN_MUST_SUCCEED ));if (gdev == last_gdev)break ;
Signature · 1 line
2.43.0
PATCH v4 4/7 iommu: Pass in old domain to attach_dev callback functions
In reply to Nicolin ChenThe IOMMU core attaches each device to a default domain on probe(). Then, every new "attach" operation has a fundamental meaning of two-fold:
- detach from its currently attached (old) domain
- attach to a given new domain
Modern IOMMU drivers following this pattern usually want to clean up the things related to the old domain, so they call iommu_get_domain_for_dev() to fetch the old domain.
Pass in the old domain pointer from the core to drivers, aligning with the set_dev_pasid op that passes in already.
Ensure all low-level attach fcuntions in the core can forward the correct old domain pointer. Thus, rework those functions as well.
--- include/linux/iommu.h | 3 +- drivers/iommu/amd/iommu.c | 11 ++++--- drivers/iommu/apple-dart.c | 9 +++-- .../arm/arm-smmu-v3/arm-smmu-v3-iommufd.c | 5 +-- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 33 ++++++++++++------- drivers/iommu/arm/arm-smmu/arm-smmu.c | 9 +++-- drivers/iommu/arm/arm-smmu/qcom_iommu.c | 11 ++++--- drivers/iommu/exynos-iommu.c | 6 ++-- drivers/iommu/fsl_pamu_domain.c | 12 +++---- drivers/iommu/intel/iommu.c | 10 ++++-- drivers/iommu/intel/nested.c | 2 +- drivers/iommu/iommu.c | 26 +++++++++------ drivers/iommu/iommufd/selftest.c | 2 +- drivers/iommu/ipmmu-vmsa.c | 10 +++--- drivers/iommu/msm_iommu.c | 8 ++--- drivers/iommu/mtk_iommu.c | 8 ++--- drivers/iommu/mtk_iommu_v1.c | 7 ++-- drivers/iommu/omap-iommu.c | 12 +++---- drivers/iommu/riscv/iommu.c | 9 +++-- drivers/iommu/rockchip-iommu.c | 20 ++++++++--- drivers/iommu/s390-iommu.c | 9 +++-- drivers/iommu/sprd-iommu.c | 3 +- drivers/iommu/sun50i-iommu.c | 8 +++-- drivers/iommu/tegra-smmu.c | 10 +++--- drivers/iommu/virtio-iommu.c | 6 ++-- 25 files changed, 152 insertions(+), 97 deletions(-)
include/linux/iommu.h +2 -1
@@ -751,7 +751,8 @@ struct iommu_ops {* @free: Release the domain after use. */ struct iommu_domain_ops { -int (*attach_dev)(struct iommu_domain *domain,struct device *dev); +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,ioasid_t pasid,struct iommu_domain *old);
drivers/iommu/amd/iommu.c +6 -5
@@ -69,8 +69,8 @@ int amd_iommu_max_glx_val = -1;*/ DEFINE_IDA (pdom_ids); -static int amd_iommu_attach_device (struct iommu_domain *dom, -struct device *dev); +static int amd_iommu_attach_device (struct iommu_domain *dom,struct device *dev, +struct iommu_domain *old);static void set_dte_entry (struct amd_iommu *iommu,struct iommu_dev_data *dev_data);
@@ -2634,7 +2634,8 @@ void amd_iommu_domain_free(struct iommu_domain *dom) }static int blocked_domain_attach_device (struct iommu_domain *domain, -struct device *dev) +struct device *dev, +struct iommu_domain *old) {struct iommu_dev_data *dev_data =dev_iommu_priv_get (dev);
@@ -2692,8 +2693,8 @@ static struct iommu_domain release_domain = { } }; -static int amd_iommu_attach_device (struct iommu_domain *dom, -struct device *dev) +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);
drivers/iommu/apple-dart.c +6 -3
@@ -660,7 +660,8 @@ static int apple_dart_domain_add_streams(struct apple_dart_domain *domain, }static int apple_dart_attach_dev_paging (struct iommu_domain *domain, -struct device *dev) +struct device *dev, +struct iommu_domain *old) {int ret, i;struct apple_dart_stream_map *stream_map;
@@ -681,7 +682,8 @@ static int apple_dart_attach_dev_paging(struct iommu_domain *domain, }static int apple_dart_attach_dev_identity (struct iommu_domain *domain, -struct device *dev) +struct device *dev, +struct iommu_domain *old) {struct apple_dart_master_cfg *cfg =dev_iommu_priv_get (dev);struct apple_dart_stream_map *stream_map;
@@ -705,7 +707,8 @@ static struct iommu_domain apple_dart_identity_domain = { };static int apple_dart_attach_dev_blocked (struct iommu_domain *domain, -struct device *dev) +struct device *dev, +struct iommu_domain *old) {struct apple_dart_master_cfg *cfg =dev_iommu_priv_get (dev);struct apple_dart_stream_map *stream_map;
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c +3 -2
@@ -138,14 +138,15 @@ void arm_smmu_master_clear_vmaster(struct arm_smmu_master *master) }static int arm_smmu_attach_dev_nested (struct iommu_domain *domain, -struct device *dev) +struct device *dev, +struct iommu_domain *old_domain) {struct arm_smmu_nested_domain *nested_domain =to_smmu_nested_domain (domain);struct arm_smmu_master *master =dev_iommu_priv_get (dev);struct arm_smmu_attach_state state = { .master = master, - .old_domain =iommu_get_domain_for_dev (dev), + .old_domain = old_domain, .ssid =IOMMU_NO_PASID , };struct arm_smmu_ste ste;
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +21 -12
@@ -3002,7 +3002,8 @@ void arm_smmu_attach_commit(struct arm_smmu_attach_state *state)arm_smmu_remove_master_domain (master, state->old_domain, state->ssid); } -static int arm_smmu_attach_dev (struct iommu_domain *domain,struct device *dev) +static int arm_smmu_attach_dev (struct iommu_domain *domain,struct device *dev, +struct iommu_domain *old_domain) {int ret =0 ;struct arm_smmu_ste target;
@@ -3010,7 +3011,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)struct arm_smmu_device *smmu;struct arm_smmu_domain *smmu_domain =to_smmu_domain (domain);struct arm_smmu_attach_state state = { - .old_domain =iommu_get_domain_for_dev (dev), + .old_domain = old_domain, .ssid =IOMMU_NO_PASID , };struct arm_smmu_master *master;
@@ -3186,7 +3187,7 @@ static int arm_smmu_blocking_set_dev_pasid(struct iommu_domain *new_domain,/* * When the last user of the CD table goes away downgrade the STE back -* to a non-cd_table one. +* to a non-cd_table one, by re-attaching its sid_domain. */ if (!arm_smmu_ssids_in_use (&master->cd_table)) { struct iommu_domain *sid_domain =
@@ -3194,12 +3195,14 @@ static int arm_smmu_blocking_set_dev_pasid(struct iommu_domain *new_domain,if (sid_domain->type ==IOMMU_DOMAIN_IDENTITY || sid_domain->type ==IOMMU_DOMAIN_BLOCKED ) - sid_domain->ops->attach_dev (sid_domain, dev); + sid_domain->ops->attach_dev (sid_domain, dev, + sid_domain); }return 0 ; }static void arm_smmu_attach_dev_ste (struct iommu_domain *domain, +struct iommu_domain *old_domain,struct device *dev,struct arm_smmu_ste *ste,unsigned int s1dss)
@@ -3207,7 +3210,7 @@ static void arm_smmu_attach_dev_ste(struct iommu_domain *domain,struct arm_smmu_master *master =dev_iommu_priv_get (dev);struct arm_smmu_attach_state state = { .master = master, - .old_domain =iommu_get_domain_for_dev (dev), + .old_domain = old_domain, .ssid =IOMMU_NO_PASID , };
@@ -3248,14 +3251,16 @@ static void arm_smmu_attach_dev_ste(struct iommu_domain *domain, }static int arm_smmu_attach_dev_identity (struct iommu_domain *domain, -struct device *dev) +struct device *dev, +struct iommu_domain *old_domain) {struct arm_smmu_ste ste;struct arm_smmu_master *master =dev_iommu_priv_get (dev);arm_smmu_master_clear_vmaster (master);arm_smmu_make_bypass_ste (master->smmu, &ste); -arm_smmu_attach_dev_ste (domain, dev, &ste,STRTAB_STE_1_S1DSS_BYPASS ); +arm_smmu_attach_dev_ste (domain, old_domain, dev, &ste, +STRTAB_STE_1_S1DSS_BYPASS );return 0 ; }
@@ -3269,14 +3274,15 @@ static struct iommu_domain arm_smmu_identity_domain = { };static int arm_smmu_attach_dev_blocked (struct iommu_domain *domain, -struct device *dev) +struct device *dev, +struct iommu_domain *old_domain) {struct arm_smmu_ste ste;struct arm_smmu_master *master =dev_iommu_priv_get (dev);arm_smmu_master_clear_vmaster (master);arm_smmu_make_abort_ste (&ste); -arm_smmu_attach_dev_ste (domain, dev, &ste, +arm_smmu_attach_dev_ste (domain, old_domain, dev, &ste,STRTAB_STE_1_S1DSS_TERMINATE );return 0 ; }
@@ -3292,7 +3298,8 @@ static struct iommu_domain arm_smmu_blocked_domain = { };static int arm_smmu_attach_dev_release (struct iommu_domain *domain, -struct device *dev) +struct device *dev, +struct iommu_domain *old_domain) {struct arm_smmu_master *master =dev_iommu_priv_get (dev);
@@ -3300,9 +3307,11 @@ static int arm_smmu_attach_dev_release(struct iommu_domain *domain,/* Put the STE back to what arm_smmu_init_strtab() sets */ if (dev->iommu->require_direct) -arm_smmu_attach_dev_identity (&arm_smmu_identity_domain, dev); +arm_smmu_attach_dev_identity (&arm_smmu_identity_domain, dev, + old_domain);else -arm_smmu_attach_dev_blocked (&arm_smmu_blocked_domain, dev); +arm_smmu_attach_dev_blocked (&arm_smmu_blocked_domain, dev, + old_domain);return 0 ; }
drivers/iommu/arm/arm-smmu/arm-smmu.c +6 -3
@@ -1165,7 +1165,8 @@ 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) +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 iommu_fwspec *fwspec =dev_iommu_fwspec_get (dev);
@@ -1234,7 +1235,8 @@ static int arm_smmu_attach_dev_type(struct device *dev, }static int arm_smmu_attach_dev_identity (struct iommu_domain *domain, -struct device *dev) +struct device *dev, +struct iommu_domain *old) {return arm_smmu_attach_dev_type (dev,S2CR_TYPE_BYPASS ); }
@@ -1249,7 +1251,8 @@ static struct iommu_domain arm_smmu_identity_domain = { };static int arm_smmu_attach_dev_blocked (struct iommu_domain *domain, -struct device *dev) +struct device *dev, +struct iommu_domain *old) {return arm_smmu_attach_dev_type (dev,S2CR_TYPE_FAULT ); }
drivers/iommu/arm/arm-smmu/qcom_iommu.c +6 -5
@@ -359,7 +359,8 @@ 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) +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);struct qcom_iommu_domain *qcom_domain =to_qcom_iommu_domain (domain);
@@ -388,18 +389,18 @@ static int qcom_iommu_attach_dev(struct iommu_domain *domain, struct device *dev }static int qcom_iommu_identity_attach (struct iommu_domain *identity_domain, -struct device *dev) +struct device *dev, +struct iommu_domain *old) { -struct iommu_domain *domain =iommu_get_domain_for_dev (dev);struct qcom_iommu_domain *qcom_domain;struct iommu_fwspec *fwspec =dev_iommu_fwspec_get (dev);struct qcom_iommu_dev *qcom_iommu =dev_iommu_priv_get (dev);unsigned int i; -if (domain == identity_domain || !domain) +if (old == identity_domain || !old)return 0 ; - qcom_domain =to_qcom_iommu_domain (domain); + qcom_domain =to_qcom_iommu_domain (old);if (WARN_ON (!qcom_domain->iommu))return -EINVAL ;
drivers/iommu/exynos-iommu.c +4 -2
@@ -984,7 +984,8 @@ static void exynos_iommu_domain_free(struct iommu_domain *iommu_domain) }static int exynos_iommu_identity_attach (struct iommu_domain *identity_domain, -struct device *dev) +struct device *dev, +struct iommu_domain *old) {struct exynos_iommu_owner *owner =dev_iommu_priv_get (dev);struct exynos_iommu_domain *domain;
@@ -1035,7 +1036,8 @@ static struct iommu_domain exynos_identity_domain = { };static int exynos_iommu_attach_device (struct iommu_domain *iommu_domain, -struct device *dev) +struct device *dev, +struct iommu_domain *old) {struct exynos_iommu_domain *domain =to_exynos_domain (iommu_domain);struct exynos_iommu_owner *owner =dev_iommu_priv_get (dev);
drivers/iommu/fsl_pamu_domain.c +6 -6
@@ -238,7 +238,7 @@ static int update_domain_stash(struct fsl_dma_domain *dma_domain, u32 val) }static int fsl_pamu_attach_device (struct iommu_domain *domain, -struct device *dev) +struct device *dev,struct iommu_domain *old) {struct fsl_dma_domain *dma_domain =to_fsl_dma_domain (domain);unsigned long flags;
@@ -298,9 +298,9 @@ static int fsl_pamu_attach_device(struct iommu_domain *domain,* switches to what looks like BLOCKING. */ static int fsl_pamu_platform_attach (struct iommu_domain *platform_domain, -struct device *dev) +struct device *dev, +struct iommu_domain *old) { -struct iommu_domain *domain =iommu_get_domain_for_dev (dev);struct fsl_dma_domain *dma_domain;const u32 *prop;int len;
@@ -311,11 +311,11 @@ static int fsl_pamu_platform_attach(struct iommu_domain *platform_domain,* Hack to keep things working as they always have, only leaving an * UNMANAGED domain makes it BLOCKING. */ -if (domain == platform_domain || !domain || - domain->type !=IOMMU_DOMAIN_UNMANAGED ) +if (old == platform_domain || !old || + old->type !=IOMMU_DOMAIN_UNMANAGED )return 0 ; - dma_domain =to_fsl_dma_domain (domain); + dma_domain =to_fsl_dma_domain (old);/* * Use LIODN of the PCI controller while detaching a
drivers/iommu/intel/iommu.c +7 -3
@@ -3225,7 +3225,8 @@ void device_block_translation(struct device *dev) }static int blocking_domain_attach_dev (struct iommu_domain *domain, -struct device *dev) +struct device *dev, +struct iommu_domain *old) {struct device_domain_info *info =dev_iommu_priv_get (dev);
@@ -3532,7 +3533,8 @@ int paging_domain_compatible(struct iommu_domain *domain, struct device *dev) }static int intel_iommu_attach_device (struct iommu_domain *domain, -struct device *dev) +struct device *dev, +struct iommu_domain *old) {int ret;
@@ -4396,7 +4398,9 @@ static int device_setup_pass_through(struct device *dev) context_setup_pass_through_cb, dev); } -static int identity_domain_attach_dev (struct iommu_domain *domain,struct device *dev) +static int identity_domain_attach_dev (struct iommu_domain *domain, +struct device *dev, +struct iommu_domain *old) {struct device_domain_info *info =dev_iommu_priv_get (dev);struct intel_iommu *iommu = info->iommu;
drivers/iommu/intel/nested.c +1 -1
@@ -19,7 +19,7 @@#include "pasid.h" static int intel_nested_attach_dev (struct iommu_domain *domain, -struct device *dev) +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);
drivers/iommu/iommu.c +16 -10
@@ -100,7 +100,7 @@ static int iommu_bus_notifier(struct notifier_block *nb, unsigned long action, void *data);static void iommu_release_device (struct device *dev);static int __iommu_attach_device (struct iommu_domain *domain, -struct device *dev); +struct device *dev,struct iommu_domain *old);static int __iommu_attach_group (struct iommu_domain *domain,struct iommu_group *group); static struct iommu_domain *__iommu_paging_domain_alloc_flags(struct device *dev,
@@ -114,6 +114,7 @@ enum {static int __iommu_device_set_domain (struct iommu_group *group,struct group_device *gdev,struct iommu_domain *new_domain, +struct iommu_domain *old_domain,unsigned int flags); static int __iommu_group_set_domain_internal(struct iommu_group *group, struct iommu_domain *new_domain,
@@ -517,7 +518,8 @@ static void iommu_deinit_device(struct device *dev)* should still avoid touching any hardware configuration either. */ if (!dev->iommu->attach_deferred && ops->release_domain) - ops->release_domain->ops->attach_dev (ops->release_domain, dev); + ops->release_domain->ops->attach_dev (ops->release_domain, dev, + group->domain);if (ops->release_device) ops->release_device (dev);
@@ -602,7 +604,8 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_listif (group->default_domain)iommu_create_device_direct_mappings (group->default_domain, dev);if (group->domain) { - ret =__iommu_device_set_domain (group, gdev, group->domain,0 ); + ret =__iommu_device_set_domain (group, gdev, group->domain, +NULL ,0 );if (ret) goto err_remove_gdev; }else if (!group->default_domain && !group_list) {
@@ -2089,14 +2092,14 @@ static void __iommu_group_set_core_domain(struct iommu_group *group) }static int __iommu_attach_device (struct iommu_domain *domain, -struct device *dev) +struct device *dev,struct iommu_domain *old) {int ret;if (unlikely (domain->ops->attach_dev ==NULL ))return -ENODEV ; - ret = domain->ops->attach_dev (domain, dev); + ret = domain->ops->attach_dev (domain, dev, old);if (ret)return ret; dev->iommu->attach_deferred =0 ;
@@ -2154,7 +2157,7 @@ int iommu_deferred_attach(struct device *dev, struct iommu_domain *domain)guard (mutex)(&dev->iommu_group->mutex); -return __iommu_attach_device (domain, dev); +return __iommu_attach_device (domain, dev,NULL ); }void iommu_detach_device (struct iommu_domain *domain,struct device *dev)
@@ -2265,6 +2268,7 @@ EXPORT_SYMBOL_GPL(iommu_attach_group);static int __iommu_device_set_domain (struct iommu_group *group,struct group_device *gdev,struct iommu_domain *new_domain, +struct iommu_domain *old_domain,unsigned int flags) {struct device *dev = gdev->dev;
@@ -2291,7 +2295,7 @@ static int __iommu_device_set_domain(struct iommu_group *group, dev->iommu->attach_deferred =0 ; } - ret =__iommu_attach_device (new_domain, dev); + ret =__iommu_attach_device (new_domain, dev, old_domain);if (ret) {/* * If we have a blocking domain then try to attach that in hopes
@@ -2301,7 +2305,8 @@ static int __iommu_device_set_domain(struct iommu_group *group,if ((flags &IOMMU_SET_DOMAIN_MUST_SUCCEED ) && group->blocking_domain && group->blocking_domain != new_domain) -__iommu_attach_device (group->blocking_domain, dev); +__iommu_attach_device (group->blocking_domain, dev, + old_domain);return ret; }return 0 ;
@@ -2347,7 +2352,8 @@ static int __iommu_group_set_domain_internal(struct iommu_group *group,*/ result =0 ;for_each_group_device (group, gdev) { - ret =__iommu_device_set_domain (group, gdev, new_domain, flags); + ret =__iommu_device_set_domain (group, gdev, new_domain, + group->domain, flags);if (ret) { result = ret;/*
@@ -2379,7 +2385,7 @@ static int __iommu_group_set_domain_internal(struct iommu_group *group,*/ if (group->domain)WARN_ON (__iommu_device_set_domain ( - group, gdev, group->domain, + group, gdev, group->domain, new_domain,IOMMU_SET_DOMAIN_MUST_SUCCEED ));if (gdev == last_gdev)break ;
drivers/iommu/iommufd/selftest.c +1 -1
@@ -216,7 +216,7 @@ static inline struct selftest_obj *to_selftest_obj(struct iommufd_object *obj) }static int mock_domain_nop_attach (struct iommu_domain *domain, -struct device *dev) +struct device *dev,struct iommu_domain *old) {struct mock_dev *mdev =to_mock_dev (dev);struct mock_viommu *new_viommu =NULL ;
drivers/iommu/ipmmu-vmsa.c +5 -5
@@ -590,7 +590,7 @@ static void ipmmu_domain_free(struct iommu_domain *io_domain) }static int ipmmu_attach_device (struct iommu_domain *io_domain, -struct device *dev) +struct device *dev,struct iommu_domain *old) {struct iommu_fwspec *fwspec =dev_iommu_fwspec_get (dev);struct ipmmu_vmsa_device *mmu =to_ipmmu (dev);
@@ -637,17 +637,17 @@ static int ipmmu_attach_device(struct iommu_domain *io_domain, }static int ipmmu_iommu_identity_attach (struct iommu_domain *identity_domain, -struct device *dev) +struct device *dev, +struct iommu_domain *old) { -struct iommu_domain *io_domain =iommu_get_domain_for_dev (dev);struct iommu_fwspec *fwspec =dev_iommu_fwspec_get (dev);struct ipmmu_vmsa_domain *domain;unsigned int i; -if (io_domain == identity_domain || !io_domain) +if (old == identity_domain || !old)return 0 ; - domain =to_vmsa_domain (io_domain); + domain =to_vmsa_domain (old);for (i =0 ; i < fwspec->num_ids; ++i)ipmmu_utlb_disable (domain, fwspec->ids[i]);
drivers/iommu/msm_iommu.c +4 -4
@@ -441,19 +441,19 @@ static int msm_iommu_attach_dev(struct iommu_domain *domain, struct device *dev) }static int msm_iommu_identity_attach (struct iommu_domain *identity_domain, -struct device *dev) +struct device *dev, +struct iommu_domain *old) { -struct iommu_domain *domain =iommu_get_domain_for_dev (dev);struct msm_priv *priv;unsigned long flags;struct msm_iommu_dev *iommu;struct msm_iommu_ctx_dev *master;int ret =0 ; -if (domain == identity_domain || !domain) +if (old == identity_domain || !old)return 0 ; - priv =to_msm_priv (domain); + priv =to_msm_priv (old);free_io_pgtable_ops (priv->iop);spin_lock_irqsave (&msm_iommu_lock, flags);
drivers/iommu/mtk_iommu.c +4 -4
@@ -705,7 +705,7 @@ static void mtk_iommu_domain_free(struct iommu_domain *domain) }static int mtk_iommu_attach_device (struct iommu_domain *domain, -struct device *dev) +struct device *dev,struct iommu_domain *old) {struct mtk_iommu_data *data =dev_iommu_priv_get (dev), *frstdata;struct mtk_iommu_domain *dom =to_mtk_domain (domain);
@@ -773,12 +773,12 @@ static int mtk_iommu_attach_device(struct iommu_domain *domain, }static int mtk_iommu_identity_attach (struct iommu_domain *identity_domain, -struct device *dev) +struct device *dev, +struct iommu_domain *old) { -struct iommu_domain *domain =iommu_get_domain_for_dev (dev);struct mtk_iommu_data *data =dev_iommu_priv_get (dev); -if (domain == identity_domain || !domain) +if (old == identity_domain || !old)return 0 ;mtk_iommu_config (data, dev, false,0 );
drivers/iommu/mtk_iommu_v1.c +5 -2
@@ -303,7 +303,9 @@ static void mtk_iommu_v1_domain_free(struct iommu_domain *domain)kfree (to_mtk_domain (domain)); } -static int mtk_iommu_v1_attach_device (struct iommu_domain *domain,struct device *dev) +static int mtk_iommu_v1_attach_device (struct iommu_domain *domain, +struct device *dev, +struct iommu_domain *old) {struct mtk_iommu_v1_data *data =dev_iommu_priv_get (dev);struct mtk_iommu_v1_domain *dom =to_mtk_domain (domain);
@@ -329,7 +331,8 @@ static int mtk_iommu_v1_attach_device(struct iommu_domain *domain, struct device }static int mtk_iommu_v1_identity_attach (struct iommu_domain *identity_domain, -struct device *dev) +struct device *dev, +struct iommu_domain *old) {struct mtk_iommu_v1_data *data =dev_iommu_priv_get (dev);
drivers/iommu/omap-iommu.c +6 -6
@@ -1431,8 +1431,8 @@ static void omap_iommu_detach_fini(struct omap_iommu_domain *odomain) odomain->iommus =NULL ; } -static int -omap_iommu_attach_dev (struct iommu_domain *domain,struct device *dev) +static int omap_iommu_attach_dev (struct iommu_domain *domain, +struct device *dev,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);
@@ -1536,15 +1536,15 @@ static void _omap_iommu_detach_dev(struct omap_iommu_domain *omap_domain, }static int omap_iommu_identity_attach (struct iommu_domain *identity_domain, -struct device *dev) +struct device *dev, +struct iommu_domain *old) { -struct iommu_domain *domain =iommu_get_domain_for_dev (dev);struct omap_iommu_domain *omap_domain; -if (domain == identity_domain || !domain) +if (old == identity_domain || !old)return 0 ; - omap_domain =to_omap_domain (domain); + omap_domain =to_omap_domain (old);spin_lock (&omap_domain->lock);_omap_iommu_detach_dev (omap_domain, dev);spin_unlock (&omap_domain->lock);
drivers/iommu/riscv/iommu.c +6 -3
@@ -1319,7 +1319,8 @@ static bool riscv_iommu_pt_supported(struct riscv_iommu_device *iommu, int pgd_m }static int riscv_iommu_attach_paging_domain (struct iommu_domain *iommu_domain, -struct device *dev) +struct device *dev, +struct iommu_domain *old) {struct riscv_iommu_domain *domain =iommu_domain_to_riscv (iommu_domain);struct riscv_iommu_device *iommu =dev_to_iommu (dev);
@@ -1424,7 +1425,8 @@ static struct iommu_domain *riscv_iommu_alloc_paging_domain(struct device *dev) }static int riscv_iommu_attach_blocking_domain (struct iommu_domain *iommu_domain, -struct device *dev) +struct device *dev, +struct iommu_domain *old) {struct riscv_iommu_device *iommu =dev_to_iommu (dev);struct riscv_iommu_info *info =dev_iommu_priv_get (dev);
@@ -1445,7 +1447,8 @@ static struct iommu_domain riscv_iommu_blocking_domain = { };static int riscv_iommu_attach_identity_domain (struct iommu_domain *iommu_domain, -struct device *dev) +struct device *dev, +struct iommu_domain *old) {struct riscv_iommu_device *iommu =dev_to_iommu (dev);struct riscv_iommu_info *info =dev_iommu_priv_get (dev);
drivers/iommu/rockchip-iommu.c +15 -5
@@ -960,7 +960,8 @@ static int rk_iommu_enable(struct rk_iommu *iommu) }static int rk_iommu_identity_attach (struct iommu_domain *identity_domain, -struct device *dev) +struct device *dev, +struct iommu_domain *old) {struct rk_iommu *iommu;struct rk_iommu_domain *rk_domain;
@@ -1005,7 +1006,7 @@ static struct iommu_domain rk_identity_domain = { };static int rk_iommu_attach_device (struct iommu_domain *domain, -struct device *dev) +struct device *dev,struct iommu_domain *old) {struct rk_iommu *iommu;struct rk_iommu_domain *rk_domain =to_rk_domain (domain);
@@ -1026,7 +1027,7 @@ static int rk_iommu_attach_device(struct iommu_domain *domain,if (iommu->domain == domain)return 0 ; - ret =rk_iommu_identity_attach (&rk_identity_domain, dev); + ret =rk_iommu_identity_attach (&rk_identity_domain, dev, old);if (ret)return ret;
@@ -1041,8 +1042,17 @@ static int rk_iommu_attach_device(struct iommu_domain *domain,return 0 ; ret =rk_iommu_enable (iommu); -if (ret) -WARN_ON (rk_iommu_identity_attach (&rk_identity_domain, dev)); +if (ret) { +/* +* Note rk_iommu_identity_attach() might fail before physically +* attaching the dev to iommu->domain, in which case the actual +* old domain for this revert should be rk_identity_domain v.s. +* iommu->domain. Since rk_iommu_identity_attach() does not care +* about the old domain argument for now, this is not a problem. +*/ +WARN_ON (rk_iommu_identity_attach (&rk_identity_domain, dev, + iommu->domain)); + }pm_runtime_put (iommu->dev);
drivers/iommu/s390-iommu.c +6 -3
@@ -653,7 +653,8 @@ int zpci_iommu_register_ioat(struct zpci_dev *zdev, u8 *status) }static int blocking_domain_attach_device (struct iommu_domain *domain, -struct device *dev) +struct device *dev, +struct iommu_domain *old) {struct zpci_dev *zdev =to_zpci_dev (dev);struct s390_domain *s390_domain;
@@ -677,7 +678,8 @@ static int blocking_domain_attach_device(struct iommu_domain *domain, }static int s390_iommu_attach_device (struct iommu_domain *domain, -struct device *dev) +struct device *dev, +struct iommu_domain *old) {struct s390_domain *s390_domain =to_s390_domain (domain);struct zpci_dev *zdev =to_zpci_dev (dev);
@@ -1113,7 +1115,8 @@ static int __init s390_iommu_init(void)subsys_initcall (s390_iommu_init);static int s390_attach_dev_identity (struct iommu_domain *domain, -struct device *dev) +struct device *dev, +struct iommu_domain *old) {struct zpci_dev *zdev =to_zpci_dev (dev);u8 status;
drivers/iommu/sprd-iommu.c +2 -1
@@ -247,7 +247,8 @@ static void sprd_iommu_domain_free(struct iommu_domain *domain) }static int sprd_iommu_attach_device (struct iommu_domain *domain, -struct device *dev) +struct device *dev, +struct iommu_domain *old) {struct sprd_iommu_device *sdev =dev_iommu_priv_get (dev);struct sprd_iommu_domain *dom =to_sprd_domain (domain);
drivers/iommu/sun50i-iommu.c +5 -3
@@ -771,7 +771,8 @@ static void sun50i_iommu_detach_domain(struct sun50i_iommu *iommu, }static int sun50i_iommu_identity_attach (struct iommu_domain *identity_domain, -struct device *dev) +struct device *dev, +struct iommu_domain *old) {struct sun50i_iommu *iommu =dev_iommu_priv_get (dev);struct sun50i_iommu_domain *sun50i_domain;
@@ -797,7 +798,8 @@ static struct iommu_domain sun50i_iommu_identity_domain = { };static int sun50i_iommu_attach_device (struct iommu_domain *domain, -struct device *dev) +struct device *dev, +struct iommu_domain *old) {struct sun50i_iommu_domain *sun50i_domain =to_sun50i_domain (domain);struct sun50i_iommu *iommu;
@@ -813,7 +815,7 @@ static int sun50i_iommu_attach_device(struct iommu_domain *domain,if (iommu->domain == domain)return 0 ; -sun50i_iommu_identity_attach (&sun50i_iommu_identity_domain, dev); +sun50i_iommu_identity_attach (&sun50i_iommu_identity_domain, dev, old);sun50i_iommu_attach_domain (iommu, sun50i_domain);
drivers/iommu/tegra-smmu.c +5 -5
@@ -490,7 +490,7 @@ static void tegra_smmu_as_unprepare(struct tegra_smmu *smmu, }static int tegra_smmu_attach_dev (struct iommu_domain *domain, -struct device *dev) +struct device *dev,struct iommu_domain *old) {struct iommu_fwspec *fwspec =dev_iommu_fwspec_get (dev);struct tegra_smmu *smmu =dev_iommu_priv_get (dev);
@@ -524,9 +524,9 @@ static int tegra_smmu_attach_dev(struct iommu_domain *domain, }static int tegra_smmu_identity_attach (struct iommu_domain *identity_domain, -struct device *dev) +struct device *dev, +struct iommu_domain *old) { -struct iommu_domain *domain =iommu_get_domain_for_dev (dev);struct iommu_fwspec *fwspec =dev_iommu_fwspec_get (dev);struct tegra_smmu_as *as;struct tegra_smmu *smmu;
@@ -535,10 +535,10 @@ static int tegra_smmu_identity_attach(struct iommu_domain *identity_domain,if (!fwspec)return -ENODEV ; -if (domain == identity_domain || !domain) +if (old == identity_domain || !old)return 0 ; - as =to_smmu_as (domain); + as =to_smmu_as (old); smmu = as->smmu;for (index =0 ; index < fwspec->num_ids; index++) {tegra_smmu_disable (smmu, fwspec->ids[index], as->id);
drivers/iommu/virtio-iommu.c +4 -2
@@ -730,7 +730,8 @@ static struct iommu_domain *viommu_domain_alloc_identity(struct device *dev)return domain; } -static int viommu_attach_dev (struct iommu_domain *domain,struct device *dev) +static int viommu_attach_dev (struct iommu_domain *domain,struct device *dev, +struct iommu_domain *old) {int ret =0 ;struct virtio_iommu_req_attach req;
@@ -781,7 +782,8 @@ static int viommu_attach_dev(struct iommu_domain *domain, struct device *dev) }static int viommu_attach_identity_domain (struct iommu_domain *domain, -struct device *dev) +struct device *dev, +struct iommu_domain *old) {int ret =0 ;struct virtio_iommu_req_attach req;
Signature · 1 line
2.43.0
RE: [PATCH v4 4/7] iommu: Pass in old domain to attach_dev callback functions
Quoted 12 lines
From: Nicolin Chen <[email protected]>
Sent: Monday, September 1, 2025 7:32 AMThe IOMMU core attaches each device to a default domain on
probe(). Then, every new "attach" operation has a fundamental meaning of two-fold:
- detach from its currently attached (old) domain
- attach to a given new domainModern IOMMU drivers following this pattern usually want to clean up the things related to the old domain, so they call
iommu_get_domain_for_dev()to fetch the old domain.
Quoted 8 lines
Pass in the old domain pointer from the core to drivers, aligning with the
set_dev_pasidop that passes in already.Ensure all low-level attach fcuntions in the core can forward the correct old domain pointer. Thus, rework those functions as well.
Suggested-by Jason Gunthorpe <[email protected]>Signed-off-by Nicolin Chen <[email protected]>
Re: [PATCH v4 4/7] iommu: Pass in old domain to attach_dev callback functions
In reply to Nicolin ChenQuoted 11 lines · Nicolin Chen
drivers/iommu/iommu.c +2
-1@@ -2347,7 +2352,8 @@ static int __iommu_group_set_domain_internal(struct iommu_group *group,*/ result =0 ;for_each_group_device (group, gdev) { - ret =__iommu_device_set_domain (group, gdev, new_domain, flags); + ret =__iommu_device_set_domain (group, gdev, new_domain, + group->domain, flags);if (ret) { result = ret;/*
drivers/iommu/iommu.c static int __iommu_group_set_domain_internal(struct iommu_group *group, · line 2391
Quoted 8 lines
drivers/iommu/iommu.c +1
-1@@ -2379,7 +2385,7 @@ static int __iommu_group_set_domain_internal(struct iommu_group *group,*/ if (group->domain)WARN_ON (__iommu_device_set_domain ( - group, gdev, group->domain, + group, gdev, group->domain, new_domain,IOMMU_SET_DOMAIN_MUST_SUCCEED ));if (gdev == last_gdev)break ;
This last_gdev is the device failing __iommu_device_set_domain() previously, so it should not be reverted from the new_domain to the group->domain, as it has never left the group->domain.
This isn't a problem without this patch, because the revert will be simply a re-attach (from group->domain to group->domain).
I'm adding a new patch to the top of the series in v5, though it doesn't worth being a bug fix IMHO.
Thanks
Nicolin
Re: [PATCH v4 4/7] iommu: Pass in old domain to attach_dev callback functions
In reply to Nicolin ChenQuoted 36 lines · Nicolin Chen
The IOMMU core attaches each device to a default domain on
probe(). Then, every new "attach" operation has a fundamental meaning of two-fold:
- detach from its currently attached (old) domain
- attach to a given new domainModern IOMMU drivers following this pattern usually want to clean up the things related to the old domain, so they call
iommu_get_domain_for_dev()to fetch the old domain.Pass in the old domain pointer from the core to drivers, aligning with the
set_dev_pasidop that passes in already.Ensure all low-level attach fcuntions in the core can forward the correct old domain pointer. Thus, rework those functions as well.
Suggested-by Jason Gunthorpe <[email protected]>Signed-off-by Nicolin Chen <[email protected]>--- include/linux/iommu.h | 3 +- drivers/iommu/amd/iommu.c | 11 ++++--- drivers/iommu/apple-dart.c | 9 +++-- .../arm/arm-smmu-v3/arm-smmu-v3-iommufd.c | 5 +-- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 33 ++++++++++++------- drivers/iommu/arm/arm-smmu/arm-smmu.c | 9 +++-- drivers/iommu/arm/arm-smmu/qcom_iommu.c | 11 ++++--- drivers/iommu/exynos-iommu.c | 6 ++-- drivers/iommu/fsl_pamu_domain.c | 12 +++---- drivers/iommu/intel/iommu.c | 10 ++++-- drivers/iommu/intel/nested.c | 2 +- drivers/iommu/iommu.c | 26 +++++++++------ drivers/iommu/iommufd/selftest.c | 2 +- drivers/iommu/ipmmu-vmsa.c | 10 +++--- drivers/iommu/msm_iommu.c | 8 ++--- drivers/iommu/mtk_iommu.c | 8 ++--- drivers/iommu/mtk_iommu_v1.c | 7 ++-- drivers/iommu/omap-iommu.c | 12 +++----
Quoted 8 lines
drivers/iommu/riscv/iommu.c | 9 +++-- drivers/iommu/rockchip-iommu.c | 20 ++++++++--- drivers/iommu/s390-iommu.c | 9 +++-- drivers/iommu/sprd-iommu.c | 3 +- drivers/iommu/sun50i-iommu.c | 8 +++-- drivers/iommu/tegra-smmu.c | 10 +++--- drivers/iommu/virtio-iommu.c | 6 ++-- 25 files changed, 152 insertions(+), 97 deletions(-)
I've split things like this into more patches before, but this actually isn't too bad, so I wouldn't push for it.
A series version would be:
- add a new op 'attach_dev2' or whatever
- Convert all drivers that just change the signature
- Convert drivers that have a trivial iommu_get_domain_for_dev()
- N patches to convert more complex drivers one by one
- Remove old op attach_dev
Again, looks Ok as-is:
Jason
Re: [PATCH v4 4/7] iommu: Pass in old domain to attach_dev callback functions
Quoted 2 lines · Jason Gunthorpe
I've split things like this into more patches before, but this actually isn't too bad, so I wouldn't push for it.
Thanks. I will keep the flow in mind.
For working more on the new test attach op, I'd probably keep it as-is.
Quoted 6 lines
A series version would be:
- add a new op 'attach_dev2' or whatever
- Convert all drivers that just change the signature
- Convert drivers that have a trivialiommu_get_domain_for_dev()
- N patches to convert more complex drivers one by one
- Remove old opattach_dev
Does this mean that drivers would live with the 'attach_dev2' op renamed from 'attach_dev'? Or the last could rename it back?
Nicolin
Jason Gunthorpe
maintainerRe: [PATCH v4 4/7] iommu: Pass in old domain to attach_dev callback functions
Quoted 16 lines · Nicolin Chen
Quoted 2 lines · Jason Gunthorpe
I've split things like this into more patches before, but this actually isn't too bad, so I wouldn't push for it.
Thanks. I will keep the flow in mind.
For working more on the new test attach op, I'd probably keep it as-is.
Quoted 6 lines
A series version would be:
- add a new op 'attach_dev2' or whatever
- Convert all drivers that just change the signature
- Convert drivers that have a trivialiommu_get_domain_for_dev()
- N patches to convert more complex drivers one by one
- Remove old opattach_dev
Quoted 2 lines
Does this mean that drivers would live with the '
attach_dev2' op renamed from 'attach_dev'? Or the last could rename it back?
Could rename back, often better to select a name that doesn't need a rename back :)
Jason
PATCH v4 5/7 iommu: Add iommu_get_domain_for_dev_locked() helper
In reply to Nicolin ChenThere is a need to stage a PCI device that's under a reset to temporally the blocked domain (i.e. detach it from its previously attached domain), and then to reattach it back to its previous domain (i.e. detach it from the blocked domain) after reset.
During the reset stage, there can be races from other attach/detachment. To solve this, a per-gdev reset flag will be introduced so that all the attach functions will reject any concurrent attach_dev callbacks.
So, iommu_get_domain_for_dev() function always returns the group->domain that needs to be changed to the blocked domain by checking the per-gdev flag, for which iommu_get_domain_for_dev() must hold the group->mutex.
On the other hand, caller like the SMMUv3 driver invoke it in one of its set_dev_pasid functions where the group->mutex is held, while some other callers like non-IOMMU drivers invoke it outside IOMMU callback functions so the group->mutex is not held. Apparently, this makes it difficult to add the lock to the existing iommu_get_domain_for_dev().
Introduce a new iommu_get_domain_for_dev_locked() helper to be used in a a context that is already under the protection of the group->mutex.
Add a lockdep_assert_not_held to the existing iommu_get_domain_for_dev() to note that it would be only used outside the group->mutex.
--- include/linux/iommu.h | 1 + drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 5 +++-- drivers/iommu/dma-iommu.c | 2 +- drivers/iommu/iommu.c | 14 ++++++++++++++ 4 files changed, 19 insertions(+), 3 deletions(-)
include/linux/iommu.h +1 -0
@@ -910,6 +910,7 @@ extern int iommu_attach_device(struct iommu_domain *domain,extern void iommu_detach_device (struct iommu_domain *domain,struct device *dev);extern struct iommu_domain *iommu_get_domain_for_dev (struct device *dev); +struct iommu_domain *iommu_get_domain_for_dev_locked (struct device *dev);extern struct iommu_domain *iommu_get_dma_domain (struct device *dev);extern int iommu_map (struct iommu_domain *domain,unsigned long iova,phys_addr_t paddr,size_t size,int prot,gfp_t gfp);
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +3 -2
@@ -3125,7 +3125,8 @@ 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) { - struct iommu_domain *sid_domain = iommu_get_domain_for_dev(master->dev); + struct iommu_domain *sid_domain = +iommu_get_domain_for_dev_locked (master->dev); struct arm_smmu_attach_state state = { .master = master, .ssid = pasid,
@@ -3191,7 +3192,7 @@ static int arm_smmu_blocking_set_dev_pasid(struct iommu_domain *new_domain,*/ if (!arm_smmu_ssids_in_use (&master->cd_table)) {struct iommu_domain *sid_domain = -iommu_get_domain_for_dev (master->dev); +iommu_get_domain_for_dev_locked (master->dev);if (sid_domain->type ==IOMMU_DOMAIN_IDENTITY || sid_domain->type ==IOMMU_DOMAIN_BLOCKED )
drivers/iommu/dma-iommu.c +1 -1
@@ -2097,7 +2097,7 @@ EXPORT_SYMBOL_GPL(dma_iova_destroy);void iommu_setup_dma_ops (struct device *dev) { -struct iommu_domain *domain =iommu_get_domain_for_dev (dev); +struct iommu_domain *domain =iommu_get_domain_for_dev_locked (dev);if (dev_is_pci (dev)) dev->iommu->pci_32bit_workaround = !iommu_dma_forcedac;
drivers/iommu/iommu.c +14 -0
@@ -2179,6 +2179,7 @@ void iommu_detach_device(struct iommu_domain *domain, struct device *dev) }EXPORT_SYMBOL_GPL (iommu_detach_device); +/* Caller must be a general/external function that isn't an IOMMU callback */ struct iommu_domain *iommu_get_domain_for_dev (struct device *dev) {/* Caller must be a probed driver on dev */
@@ -2187,10 +2188,23 @@ struct iommu_domain *iommu_get_domain_for_dev(struct device *dev)if (!group)return NULL ; +lockdep_assert_not_held (&group->mutex); +return group->domain; }EXPORT_SYMBOL_GPL (iommu_get_domain_for_dev); +/* Caller must be an IOMMU callback/internal function that holds group->mutex */ +struct iommu_domain *iommu_get_domain_for_dev_locked (struct device *dev) +{ +struct iommu_group *group = dev->iommu_group; + +lockdep_assert_held (&group->mutex); + +return group->domain; +} +EXPORT_SYMBOL_GPL (iommu_get_domain_for_dev_locked); +/* * For IOMMU_DOMAIN_DMA implementations which already provide their own * guarantees that the group and its default domain are valid and correct.
Signature · 1 line
2.43.0
RE: [PATCH v4 5/7] iommu: Add iommu_get_domain_for_dev_locked() helper
Quoted 8 lines
From: Nicolin Chen <[email protected]>
Sent: Monday, September 1, 2025 7:32 AM+/* Caller must be a general/external function that isn't an IOMMU callback */
structiommu_domain*iommu_get_domain_for_dev(struct device *dev) {
'general function' is not easy to get its meaning. just keep 'external'?
Re: [PATCH v4 5/7] iommu: Add iommu_get_domain_for_dev_locked() helper
Quoted 10 lines · Kevin
Quoted 8 lines
From: Nicolin Chen <[email protected]>
Sent: Monday, September 1, 2025 7:32 AM+/* Caller must be a general/external function that isn't an IOMMU callback */
structiommu_domain*iommu_get_domain_for_dev(struct device *dev) {'general function' is not easy to get its meaning. just keep 'external'?
This is still one internal function use this:drivers/iommu/dma-iommu.c:1785: iommu_deferred_attach(dev, iommu_get_domain_for_dev(dev)))
So, it's not completely "external" :-/
Thanks
Nicolin
Jason Gunthorpe
maintainerRe: [PATCH v4 5/7] iommu: Add iommu_get_domain_for_dev_locked() helper
In reply to Nicolin Chendrivers/iommu/dma-iommu.c EXPORT_SYMBOL_GPL(dma_iova_destroy); · line 2100
Quoted 10 lines · Nicolin Chen
drivers/iommu/dma-iommu.c +1
-1@@ -2097,7 +2097,7 @@ EXPORT_SYMBOL_GPL(dma_iova_destroy);void iommu_setup_dma_ops (struct device *dev) { -struct iommu_domain *domain =iommu_get_domain_for_dev (dev); +struct iommu_domain *domain =iommu_get_domain_for_dev_locked (dev);
Lets have another patch to tidy this. This function can only be called on the default_domain. We can trivally pass it in. In all three cases the default domain was just attached to the device.
drivers/iommu/iommu.c +3 -3
@@ -620,7 +620,7 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list }if (group->default_domain) -iommu_setup_dma_ops (dev); +iommu_setup_dma_ops (dev, group->default_domain);mutex_unlock (&group->mutex);
@@ -1908,7 +1908,7 @@ static int bus_iommu_probe(const struct bus_type *bus)return ret; }for_each_group_device (group, gdev) -iommu_setup_dma_ops (gdev->dev); +iommu_setup_dma_ops (gdev->dev, group->default_domain);mutex_unlock (&group->mutex);/*
@@ -3104,7 +3104,7 @@ static ssize_t iommu_group_store_type(struct iommu_group *group,/* Make sure dma_ops is appropriatley set */ for_each_group_device (group, gdev) -iommu_setup_dma_ops (gdev->dev); +iommu_setup_dma_ops (gdev->dev, group->default_domain); out_unlock:mutex_unlock (&group->mutex);
drivers/iommu/dma-iommu.c EXPORT_SYMBOL_GPL(dma_iova_destroy); · line 2102
Quoted 2 lines
drivers/iommu/dma-iommu.c +1
-0+/* Caller must be a general/external function that isn't an IOMMU callback */ struct iommu_domain *iommu_get_domain_for_dev (struct device *dev)
Maybe a kdoc?
/**
* iommu_get_domain_for_dev() - Return the DMA API domain pointer
* @dev - Device to query
*
* This function can be called within a driver bound to dev. The returned
* pointer is valid for the lifetime of the bound driver.
*
* It should not be called by drivers with driver_managed_dma = true. */
struct iommu_domain *iommu_get_domain_for_dev(struct device *dev)
I really wanted to say this should just always return the default_domain, but it looks like host1x_client_iommu_detach() is the only place outside the iommu drivers that would be unhappy with that.
Jason
Re: [PATCH v4 5/7] iommu: Add iommu_get_domain_for_dev_locked() helper
Quoted 7 lines · Jason Gunthorpe
Quoted 6 lines · Nicolin Chen
drivers/iommu/dma-iommu.c +0
-0@@ -2097,7 +2097,7 @@ EXPORT_SYMBOL_GPL(dma_iova_destroy);
Quoted 8 lines
drivers/iommu/dma-iommu.c EXPORT_SYMBOL_GPL(dma_iova_destroy); · line 2100
Quoted 4 lines
drivers/iommu/dma-iommu.c +1
-1void iommu_setup_dma_ops (struct device *dev) { -struct iommu_domain *domain =iommu_get_domain_for_dev (dev); +struct iommu_domain *domain =iommu_get_domain_for_dev_locked (dev);Lets have another patch to tidy this. This function can only be called on the
default_domain. We can trivally pass it in. In all three cases the default domain was just attached to the device.
Will replace with your patch.
Quoted 5 lines
drivers/iommu/dma-iommu.c EXPORT_SYMBOL_GPL(dma_iova_destroy); · line 2102
Quoted 2 lines
drivers/iommu/dma-iommu.c +1
-0+/* Caller must be a general/external function that isn't an IOMMU callback */ struct iommu_domain *iommu_get_domain_for_dev (struct device *dev)Maybe a kdoc?
Quoted 10 lines
/**
*iommu_get_domain_for_dev()- Return the DMA API domain pointer
* @dev - Device to query
*
* This function can be called within a driver bound to dev. The returned
* pointer is valid for the lifetime of the bound driver.
*
* It should not be called by drivers withdriver_managed_dma= true. */
structiommu_domain*iommu_get_domain_for_dev(struct device *dev)
Will add this.
Quoted 3 lines
I really wanted to say this should just always return the
default_domain, but it looks likehost1x_client_iommu_detach()is the only place outside the iommu drivers that would be unhappy with that.
I suppose any external user that allocates/attaches a domain has a risk of potentially using this helper, in which case group->domain may not be default_domain?
Thanks
Nicolin
Jason Gunthorpe
maintainerRe: [PATCH v4 5/7] iommu: Add iommu_get_domain_for_dev_locked() helper
Quoted 7 lines · Nicolin Chen
Quoted 3 lines
I really wanted to say this should just always return the
default_domain, but it looks likehost1x_client_iommu_detach()is the only place outside the iommu drivers that would be unhappy with that.I suppose any external user that allocates/attaches a domain has a risk of potentially using this helper, in which case
group->domainmay not bedefault_domain?
Yes, host1x seemed to be the only place that had that intersection.
Jason
Re: [PATCH v4 5/7] iommu: Add iommu_get_domain_for_dev_locked() helper
Quoted 10 lines · Jason Gunthorpe
Quoted 7 lines · Nicolin Chen
Quoted 3 lines
I really wanted to say this should just always return the
default_domain, but it looks likehost1x_client_iommu_detach()is the only place outside the iommu drivers that would be unhappy with that.I suppose any external user that allocates/attaches a domain has a risk of potentially using this helper, in which case
group->domainmay not bedefault_domain?Yes, host1x seemed to be the only place that had that intersection.
I meant, we cannot prevent others from calling it like host1x does?
Even if we have those replacement APIs that we discussed last time, such a driver might still want its own domain v.s. default_domain, for pgsize or other use?
Thanks
Nicolin
Jason Gunthorpe
maintainerRe: [PATCH v4 5/7] iommu: Add iommu_get_domain_for_dev_locked() helper
Quoted 10 lines · Nicolin Chen
Quoted 9 lines · Jason Gunthorpe
Quoted 7 lines · Nicolin Chen
Quoted 3 lines
I really wanted to say this should just always return the
default_domain, but it looks likehost1x_client_iommu_detach()is the only place outside the iommu drivers that would be unhappy with that.I suppose any external user that allocates/attaches a domain has a risk of potentially using this helper, in which case
group->domainmay not bedefault_domain?
Quoted 7 lines
Quoted 1 line
Yes, host1x seemed to be the only place that had that intersection.
I meant, we cannot prevent others from calling it like host1x does?
Even if we have those replacement APIs that we discussed last time, such a driver might still want its own domain v.s.
default_domain, for pgsize or other use?
If a driver allocates its own domain and attaches it then it should be able to track it and not need to ask the iommu subsystem to give it back to them :)
Jason
PATCH v4 6/7 iommu: Introduce iommu_dev_reset_prepare() and iommu_dev_reset_done()
In reply to Nicolin ChenPCIe permits a device to ignore ATS invalidation TLPs, while processing a reset. This creates a problem visible to the OS where an ATS invalidation command will time out. E.g. an SVA domain will have no coordination with a reset event and can racily issue ATS invalidations to a resetting device.
The OS should do something to mitigate this as we do not want production systems to be reporting critical ATS failures, especially in a hypervisor environment. Broadly, OS could arrange to ignore the timeouts, block page table mutations to prevent invalidations, or disable and block ATS.
The PCIe spec in sec 10.3.1 IMPLEMENTATION NOTE recommends to disable and block ATS before initiating a Function Level Reset. It also mentions that other reset methods could have the same vulnerability as well.
Provide a callback from the PCI subsystem that will enclose the reset and have the iommu core temporarily change all the attached domain to BLOCKED. After attaching a BLOCKED domain, IOMMU hardware would fence any incoming ATS queries. And IOMMU drivers should also synchronously stop issuing new ATS invalidations and wait for all ATS invalidations to complete. This can avoid any ATS invaliation timeouts.
However, if there is a domain attachment/replacement happening during an ongoing reset, ATS routines may be re-activated between the two function calls. So, introduce a new pending_reset flag in group_device, and reject any concurrent attach_dev/set_dev_pasid call during a reset for a concern of compatibility failure.
There are two corner cases that won't work:
1. Alias devices that share the same RID
Blocking one device also blocks the other alias devices that might not want a reset. Given that it's very rare for an alias device to support ATS, simply skip the blocking routine.
2. SRIOV devices that its PF is resetting while its VF isn't. Both PF and VF should block RID and PASIDs. But, since VF is not aware of the reset, it is difficult to block it and reject concurrent attach calls, because it's not logically reasonable to reject a VF attachment due to a resetting PF unless the VF is resetting too. To address this, we won't be able to reject any concurrent attachment as simple as this patch does; instead we will need two new compatibility testing ops for attach_dev/set_dev_pasid to allowing caching a compatible attach. This itself, however, would be a big series. So, for now, skip the blocking routine for PF devices, and leave a note.
--- include/linux/iommu.h | 12 +++ drivers/iommu/iommu.c | 199 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 211 insertions(+)
include/linux/iommu.h +12 -0
@@ -1169,6 +1169,9 @@ void dev_iommu_priv_set(struct device *dev, void *priv);extern struct mutex iommu_probe_device_lock;int iommu_probe_device (struct device *dev); +int iommu_dev_reset_prepare (struct device *dev); +void iommu_dev_reset_done (struct device *dev); +int iommu_device_use_default_domain (struct device *dev);void iommu_device_unuse_default_domain (struct device *dev);
@@ -1453,6 +1456,15 @@ static inline int iommu_fwspec_add_ids(struct device *dev, u32 *ids,return -ENODEV ; } +static inline int iommu_dev_reset_prepare (struct device *dev) +{ +return 0 ; +} + +static inline void iommu_dev_reset_done (struct device *dev) +{ +} +static inline struct iommu_fwspec *dev_iommu_fwspec_get (struct device *dev) {return NULL ;
drivers/iommu/iommu.c +199 -0
241 lines not on this page · raw message
Signature · 1 line
2.43.0
Tian, Kevin
maintainerRE: [PATCH v4 6/7] iommu: Introduce iommu_dev_reset_prepare() and iommu_dev_reset_done()
Quoted 33 lines
From: Nicolin Chen <[email protected]>
Sent: Monday, September 1, 2025 7:32 AMPCIe permits a device to ignore ATS invalidation TLPs, while processing a reset. This creates a problem visible to the OS where an ATS invalidation command will time out. E.g. an SVA domain will have no coordination with a reset event and can racily issue ATS invalidations to a resetting device.
The OS should do something to mitigate this as we do not want production systems to be reporting critical ATS failures, especially in a hypervisor environment. Broadly, OS could arrange to ignore the timeouts, block page table mutations to prevent invalidations, or disable and block ATS.
The PCIe spec in sec 10.3.1 IMPLEMENTATION NOTE recommends to disable and
block ATS before initiating a Function Level Reset. It also mentions that other reset methods could have the same vulnerability as well.Provide a callback from the PCI subsystem that will enclose the reset and have the iommu core temporarily change all the attached domain to BLOCKED.
After attaching a BLOCKED domain, IOMMU hardware would fence any incoming
ATS queries. And IOMMU drivers should also synchronously stop issuing new ATS invalidations and wait for all ATS invalidations to complete. This can avoid any ATS invaliation timeouts.However, if there is a domain attachment/replacement happening during an ongoing reset, ATS routines may be re-activated between the two function calls. So, introduce a new
pending_resetflag ingroup_device, and reject any concurrent attach_dev/set_dev_pasid call during a reset for a concern of compatibility failure.
Quoted 5 lines
There are two corner cases that won't work:
1. Alias devices that share the same RID
Blocking one device also blocks the other alias devices that might not want a reset. Given that it's very rare for an alias device to support ATS, simply skip the blocking routine.
it also applies to the devices in the same iommu group. While one device is being reset, all other devices in the group cannot change the domain. This needs to be documented in the attach uAPI.
Quoted 12 lines
2. SRIOV devices that its PF is resetting while its VF isn't. Both PF and VF should block RID and PASIDs. But, since VF is not aware of the reset, it is difficult to block it and reject concurrent attach calls, because it's not logically reasonable to reject a VF attachment due to a resetting PF unless the VF is resetting too. To address this, we won't be able to reject any concurrent attachment as simple as this patch does; instead we will need two new compatibility testing ops for attach_dev/set_dev_pasid to allowing caching a compatible attach. This itself, however, would be a big series. So, for now, skip the blocking routine for PF devices, and leave a note.
given it impacts uAPI:
- now attach/replace can be done anytime
- with this series attach/replace is rejected when a device is being reset
- later with compat testing ops attach/replace can be done again at any time
we should be cautious here, especially if this series goes into 6.18 (likely the next LTS version) the interim behavior change may last long. yes we discussed that no know usage would want to do attach/replace while a device is being reset, but I wonder whether we should instead wait for a full solution to avoid unnecessary uAPI change back-and-forth...
Thanks
Kevin
Jason Gunthorpe
maintainerRe: [PATCH v4 6/7] iommu: Introduce iommu_dev_reset_prepare() and iommu_dev_reset_done()
Quoted 9 lines · Kevin
Quoted 5 lines
There are two corner cases that won't work:
1. Alias devices that share the same RID
Blocking one device also blocks the other alias devices that might not want a reset. Given that it's very rare for an alias device to support ATS, simply skip the blocking routine.it also applies to the devices in the same iommu group. While one device is being reset, all other devices in the group cannot change the domain. This needs to be documented in the attach uAPI.
I think we should just exclude multi-device groups for the first version of this.
We really need to fixup multi-device groups to distinguish between cases where the group is there for aliases and we must have consistent domains across the whole group and cases where the group is there just for representing isolation and different domains are fine.
We are now having workloads where this difference matters, ATS reset is one case, but I'm interesting in seeing iommufd able to control domains per-device within a group. We have some HW that wants this.
Jason
Re: [PATCH v4 6/7] iommu: Introduce iommu_dev_reset_prepare() and iommu_dev_reset_done()
In reply to Tian, KevinQuoted 7 lines · Kevin
given it impacts uAPI:
- now attach/replace can be done anytime
- with this series attach/replace is rejected when a device is being reset
- later with compat testing ops attach/replace can be done again at any time
Quoted 5 lines
we should be cautious here, especially if this series goes into 6.18 (likely the next LTS version) the interim behavior change may last long. yes we discussed that no know usage would want to do attach/replace while a device is being reset, but I wonder whether we should instead wait for a full solution to avoid unnecessary uAPI change back-and-forth...
That's a very fair point. To keep the uAPI consistent:
I will split the series, making a smaller preparatory one, as a part-1 for release_domain and old_domain.
Part-2 will roll out a can_attach op across drivers. And part-3 should have the final reset patches.
Thanks
Nicolin
PATCH v4 7/7 pci: Suspend iommu function prior to resetting a device
In reply to Nicolin ChenPCIe permits a device to ignore ATS invalidation TLPs, while processing a reset. This creates a problem visible to the OS where an ATS invalidation command will time out: e.g. an SVA domain will have no coordination with a reset event and can racily issue ATS invalidations to a resetting device.
The PCIe spec in sec 10.3.1 IMPLEMENTATION NOTE recommends to disable and block ATS before initiating a Function Level Reset. It also mentions that other reset methods could have the same vulnerability as well.
Now iommu_dev_reset_prepare/done() helpers are introduced for this matter. Use them in all the existing reset functions, which will attach the device to an IOMMU_DOMAIN_BLOCKED during a reset, so as to allow IOMMU driver to:
- invoke pci_disable_ats() and pci_enable_ats(), if necessary
- wait for all ATS invalidations to complete
- stop issuing new ATS invalidations
- fence any incoming ATS queries
--- drivers/pci/pci.h | 2 ++ drivers/pci/pci-acpi.c | 12 ++++++-- drivers/pci/pci.c | 68 ++++++++++++++++++++++++++++++++++++++---- drivers/pci/quirks.c | 18 ++++++++++- 4 files changed, 92 insertions(+), 8 deletions(-)
drivers/pci/pci.h +2 -0
@@ -106,6 +106,8 @@ void pci_init_reset_methods(struct pci_dev *dev);int pci_bridge_secondary_bus_reset (struct pci_dev *dev);int pci_bus_error_reset (struct pci_dev *dev);int __pci_reset_bus (struct pci_bus *bus); +int pci_reset_iommu_prepare (struct pci_dev *dev); +void pci_reset_iommu_done (struct pci_dev *dev);struct pci_cap_saved_data {u16 cap_nr;
drivers/pci/pci-acpi.c +10 -2
@@ -969,6 +969,7 @@ void pci_set_acpi_fwnode(struct pci_dev *dev)int pci_dev_acpi_reset (struct pci_dev *dev,bool probe) {acpi_handle handle =ACPI_HANDLE (&dev->dev); +int ret =0 ;if (!handle || !acpi_has_method (handle,"_RST" ))return -ENOTTY ;
@@ -976,12 +977,19 @@ int pci_dev_acpi_reset(struct pci_dev *dev, bool probe)if (probe)return 0 ; + ret =pci_reset_iommu_prepare (dev); +if (ret) { +pci_err (dev,"failed to stop IOMMU\n" ); +return ret; + } +if (ACPI_FAILURE (acpi_evaluate_object (handle,"_RST" ,NULL ,NULL ))) {pci_warn (dev,"ACPI _RST failed\n" ); -return -ENOTTY ; + ret = -ENOTTY ; } -return 0 ; +pci_reset_iommu_done (dev); +return ret; }bool acpi_pci_power_manageable (struct pci_dev *dev)
drivers/pci/pci.c +63 -5
155 lines not on this page · raw message
drivers/pci/quirks.c +17 -1
@@ -4225,6 +4225,22 @@ static const struct pci_dev_reset_methods pci_dev_reset_methods[] = { { 0 } }; +static int __pci_dev_specific_reset (struct pci_dev *dev,bool probe, +const struct pci_dev_reset_methods *i) +{ +int ret; + + ret =pci_reset_iommu_prepare (dev); +if (ret) { +pci_err (dev,"failed to stop IOMMU\n" ); +return ret; + } + + ret = i->reset (dev, probe); +pci_reset_iommu_done (dev); +return ret; +} +/* * These device-specific reset methods are here rather than in a driver * because when a host assigns a device to a guest VM, the host may need
@@ -4239,7 +4255,7 @@ int pci_dev_specific_reset(struct pci_dev *dev, bool probe) i->vendor == (u16)PCI_ANY_ID) && (i->device == dev->device || i->device == (u16)PCI_ANY_ID)) - return i->reset(dev, probe); +return __pci_dev_specific_reset (dev, probe, i); }return -ENOTTY ;
Signature · 1 line
2.43.0