Disable ATS via iommu during PCI resets

The iommu_deferred_attach() is a runtime asynchronous function called by iommu-dma function, which could race against other attach functions if it accesses something in the dev->iommu_group.

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]>

---
  drivers/iommu/iommu.c | 13 ++++++++++---
  1 file changed, 10 insertions(+), 3 deletions(-)

drivers/iommu/iommu.c +0 -0

index 060ebe330ee16..1e0116bce0762 100644

From: Baolu Lu <[email protected]>
Sent: Friday, August 15, 2025 1:10 PM

drivers/iommu/iommu.c EXPORT_SYMBOL_GPL(iommu_attach_device);

Quoted 4 lines · Nicolin Chen

drivers/iommu/iommu.c +3 -0

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

Why not making it like this,

int iommu_deferred_attach(struct device *dev, struct iommu_domain *domain)

{
         /* Caller must be a probed driver on dev */
         if (!dev->iommu_group)
                 return 0;

         guard(mutex)(&dev->iommu_group->mutex);
         if (!dev->iommu->attach_deferred)
                 return 0;

         return __iommu_attach_device(domain, dev);
}

From: Nicolin Chen <[email protected]>
Sent: Tuesday, August 12, 2025 6:59 AM

The iommu_deferred_attach() is a runtime asynchronous function called by iommu-dma function, which could race against other attach functions if it accesses something in the dev->iommu_group.

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]>

---
 drivers/iommu/iommu.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

drivers/iommu/iommu.c +3 -2

index 060ebe330ee16..1e0116bce0762 100644
@@ -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

drivers/iommu/iommu.c +3 -1

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

Quoted 6 lines

From: Nicolin Chen <[email protected]>
Sent: Tuesday, August 12, 2025 6:59 AM

The iommu_deferred_attach() is a runtime asynchronous function called by iommu-dma function, which could race against other attach functions if it accesses something in the dev->iommu_group.

Is there a real racing scenario being observed or more theoretical?

If the former may need a Fix tag.

Quoted 7 lines

int iommu_deferred_attach(struct device *dev, struct iommu_domain
*domain)
{

drivers/iommu/iommu.c +2 -2

-	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

drivers/iommu/iommu.c EXPORT_SYMBOL_GPL(iommu_attach_device); · line 2153

Quoted 6 lines

(quoted diff) +1 -0

+	 * racy, but we have an expectation that the driver will setup its

DMAs

drivers/iommu/iommu.c +4 -0

+	 * inside probe while being single threaded to avoid racing.
+	 */
+	if (!dev->iommu || !dev->iommu->attach_deferred)
+		return 0;

Is there any way to detect a driver breaking the expectation?

Quoted 6 lines

From: Nicolin Chen <[email protected]>
Sent: Tuesday, August 12, 2025 6:59 AM

The iommu_deferred_attach() is a runtime asynchronous function called by iommu-dma function, which could race against other attach functions if it accesses something in the dev->iommu_group.

Is there a real racing scenario being observed or more theoretical?

Quoted 7 lines · Kevin

Quoted 6 lines

From: Nicolin Chen <[email protected]>
Sent: Tuesday, August 12, 2025 6:59 AM

The iommu_deferred_attach() is a runtime asynchronous function called by iommu-dma function, which could race against other attach functions if it accesses something in the dev->iommu_group.

Quoted 1 line

Is there a real racing scenario being observed or more theoretical?

I think the commit message should explain the actual reason this is being done, which AFAICT because the new lockdeps added in following patches will fail on this path otherwise.

Quoted 10 lines · Jason Gunthorpe

Quoted 8 lines · Kevin

Quoted 6 lines

From: Nicolin Chen <[email protected]>
Sent: Tuesday, August 12, 2025 6:59 AM

The iommu_deferred_attach() is a runtime asynchronous function called by iommu-dma function, which could race against other attach functions if it accesses something in the dev->iommu_group.

Is there a real racing scenario being observed or more theoretical?

Quoted 3 lines

I think the commit message should explain the actual reason this is being done, which AFAICT because the new lockdeps added in following patches will fail on this path otherwise.

Hmm, I can mention that. But I think that's just a part of the reason. It still doesn't seem correct to invoke an attach_dev function without the lock since iommu_group_mutex_assert() may be used in the path?

Quoted 11 lines · Nicolin Chen

Quoted 10 lines · Jason Gunthorpe

Quoted 8 lines · Kevin

Quoted 6 lines

From: Nicolin Chen <[email protected]>
Sent: Tuesday, August 12, 2025 6:59 AM

The iommu_deferred_attach() is a runtime asynchronous function called by iommu-dma function, which could race against other attach functions if it accesses something in the dev->iommu_group.

Is there a real racing scenario being observed or more theoretical?

Quoted 8 lines

Quoted 3 lines

I think the commit message should explain the actual reason this is being done, which AFAICT because the new lockdeps added in following patches will fail on this path otherwise.

Hmm, I can mention that. But I think that's just a part of the reason. It still doesn't seem correct to invoke an attach_dev function without the lock since iommu_group_mutex_assert() may be used in the path?

Last time this was brought up there was a bit of an argument that it couldn't happen in parallel with anything anyhow so it doesn't technically need locking. But I think we should not make such arguments and be strict about our locking. It is too hard to understand the system correctness otherwise.

The 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 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.

Reviewed-by Jason Gunthorpe<[email protected]>

Signed-off-by Nicolin Chen<[email protected]>

From: Nicolin Chen <[email protected]>
Sent: Tuesday, August 12, 2025 6:59 AM

The 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 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.

Reviewed-by Jason Gunthorpe <[email protected]>

Signed-off-by Nicolin Chen <[email protected]>

There is a need to attach 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 bypass the driver-level attach_dev callbacks, but only update the group->domain pointer. The reset recovery procedure will attach directly to the cached pointer so things will be back to normal.

On the other hand, the iommu_get_domain_for_dev() API always returns the group->domain pointer, and several IOMMMU drivers call this API in their attach_dev callback functions to get the currently attached domain for a device, which will be broken for the recovery case mentioned above:
1. core asks the driver to attach dev from blocked to group->domain
2. driver attaches dev from group->domain to group->domain

So, iommu_get_domain_for_dev() should check the gdev flag and return the blocked domain if the flag is set. But the caller of this API could hold the group->mutex already or not, making it difficult to add the lock.

Introduce a new iommu_get_domain_for_dev_locked() helper to be used by those drivers in a context that is already under the protection of the group->mutex, e.g. those attach_dev callback functions. And roll out the new helper to all the existing IOMMU drivers.

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.

Signed-off-by Nicolin Chen<[email protected]>

On 8/12/25 06:59, Nicolin Chen wrote:
Given that iommu_group->mutex is transparent to the iommu driver, how about

/*
* Called only by iommu drivers in the callback context where
* group->mutex has already been held by the core.
*/
struct iommu_domain *iommu_get_domain_for_dev_internal(struct device *dev)

{
	...
	lockdep_assert_held(&group->mutex);
	...
}

Given that iommu_group->mutex is transparent to the iommu driver, how about

Quoted 2 lines · Baolu Lu

Given that iommu_group->mutex is transparent to the iommu driver, how about

It is not actually transparent, alot of drivers are implicitly assuming that the core single threads their struct device for alot of the ops callbacks and we exposed the lockdep test to the drivers to help the document this.

From: Nicolin Chen <[email protected]>
Sent: Tuesday, August 12, 2025 6:59 AM

There is a need to attach 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 bypass the driver-level attach_dev callbacks, but only update the group->domain pointer. The reset recovery procedure will attach directly to the cached pointer so things will be back to normal.

On the other hand, the iommu_get_domain_for_dev() API always returns the group->domain pointer, and several IOMMMU drivers call this API in their attach_dev callback functions to get the currently attached domain for a device, which will be broken for the recovery case mentioned above:
1. core asks the driver to attach dev from blocked to group->domain
2. driver attaches dev from group->domain to group->domain

So, iommu_get_domain_for_dev() should check the gdev flag and return the blocked domain if the flag is set. But the caller of this API could hold the group->mutex already or not, making it difficult to add the lock.

Introduce a new iommu_get_domain_for_dev_locked() helper to be used by those drivers in a context that is already under the protection of the group->mutex, e.g. those attach_dev callback functions. And roll out the new helper to all the existing IOMMU drivers.

+/* Caller can be any general/external function that isn't an IOMMU callback */
struct iommu_domain *iommu_get_domain_for_dev(struct device *dev)

Quoted 8 lines

From: Nicolin Chen <[email protected]>
Sent: Tuesday, August 12, 2025 6:59 AM
On the other hand, the iommu_get_domain_for_dev() API always returns the group->domain pointer, and several IOMMMU drivers call this API in their attach_dev callback functions to get the currently attached domain for a device, which will be broken for the recovery case mentioned above:
1. core asks the driver to attach dev from blocked to group->domain
2. driver attaches dev from group->domain to group->domain

the 2nd bullet implies that a driver may skip the operation by noting that old domain is same as the new one?

Quoted 8 lines

So, iommu_get_domain_for_dev() should check the gdev flag and return the blocked domain if the flag is set. But the caller of this API could hold the group->mutex already or not, making it difficult to add the lock.

Introduce a new iommu_get_domain_for_dev_locked() helper to be used by those drivers in a context that is already under the protection of the group->mutex, e.g. those attach_dev callback functions. And roll out the new helper to all the existing IOMMU drivers.

iommu_get_domain_for_dev() is also called outside of attach_dev callback functions, e.g. malidp_get_pgsize_bitmap(). and the returned info according to the attached domain might be saved in static structures, e.g.:

	ms->mmu_prefetch_pgsize = malidp_get_pgsize_bitmap(mp);

would that cause weird issues when blocking domain is returned though one may not expect reset to happen at that point?

From: Nicolin Chen <[email protected]>
Sent: Saturday, August 16, 2025 2:45 AM

Quoted 3 lines · Kevin

Quoted 3 lines

From: Nicolin Chen <[email protected]>
Sent: Tuesday, August 12, 2025 6:59 AM
So, iommu_get_domain_for_dev() should check the gdev flag and return

the

Quoted 4 lines

Quoted 4 lines

blocked domain if the flag is set. But the caller of this API could hold the group->mutex already or not, making it difficult to add the lock.

Introduce a new iommu_get_domain_for_dev_locked() helper to be used

by

Quoted 11 lines

Quoted 3 lines

those drivers in a context that is already under the protection of the group->mutex, e.g. those attach_dev callback functions. And roll out the new helper to all the existing IOMMU drivers.

iommu_get_domain_for_dev() is also called outside of attach_dev callback functions, e.g. malidp_get_pgsize_bitmap(). and the returned info according to the attached domain might be saved in static structures, e.g.:

	ms->mmu_prefetch_pgsize = malidp_get_pgsize_bitmap(mp);

Quoted 2 lines

would that cause weird issues when blocking domain is returned though one may not expect reset to happen at that point?

We aren't changing the iommu_get_domain_for_dev(). So, it should be used exclusively for functions that are outside group->mutex, like this one, i.e. they should still get the group->domain v.s. the blocked domain.

Quoted 24 lines

From: Nicolin Chen <[email protected]>
Sent: Saturday, August 16, 2025 2:45 AM

Quoted 3 lines · Kevin

Quoted 3 lines

From: Nicolin Chen <[email protected]>
Sent: Tuesday, August 12, 2025 6:59 AM
So, iommu_get_domain_for_dev() should check the gdev flag and return

the

Quoted 4 lines

Quoted 4 lines

blocked domain if the flag is set. But the caller of this API could hold the group->mutex already or not, making it difficult to add the lock.

Introduce a new iommu_get_domain_for_dev_locked() helper to be used

by

Quoted 11 lines

Quoted 3 lines

those drivers in a context that is already under the protection of the group->mutex, e.g. those attach_dev callback functions. And roll out the new helper to all the existing IOMMU drivers.

iommu_get_domain_for_dev() is also called outside of attach_dev callback functions, e.g. malidp_get_pgsize_bitmap(). and the returned info according to the attached domain might be saved in static structures, e.g.:

	ms->mmu_prefetch_pgsize = malidp_get_pgsize_bitmap(mp);

Quoted 8 lines

Quoted 2 lines

would that cause weird issues when blocking domain is returned though one may not expect reset to happen at that point?

We aren't changing the iommu_get_domain_for_dev(). So, it should be used exclusively for functions that are outside group->mutex, like this one, i.e. they should still get the group->domain v.s. the blocked domain.

Usually the difference between func() and func_locked() is only about whether the caller holds a lock. If they mean to return different domains, may need better naming (though I don't have a good option now)...

Quoted 4 lines · Kevin

Usually the difference between func() and func_locked() is only about whether the caller holds a lock. If they mean to return different domains, may need better naming (though I don't have a good option now)...

Yea, naming is tricky here :)

Perhaps it could follow a different pattern:

    iommu_dev_get_domain_for_driver(struct device *dev);

With a simple note highlighting to be used by IOMMU drivers in the iommu callback functions like attach_dev.

Introduce a new iommu_get_domain_for_dev_locked() helper to be used by those drivers in a context that is already under the protection of the group->mutex, e.g. those attach_dev callback functions. And roll out the new helper to all the existing IOMMU drivers.

Quoted 4 lines · Nicolin Chen

Introduce a new iommu_get_domain_for_dev_locked() helper to be used by those drivers in a context that is already under the protection of the group->mutex, e.g. those attach_dev callback functions. And roll out the new helper to all the existing IOMMU drivers.

I still don't much care for this, I think it would be better to approach this problem by passing the old domain to the driver callback:

drivers/iommu/arm/arm-smmu/qcom_iommu.c static int qcom_iommu_attach_dev(struct iommu_domain *domain, struct device *dev · line 393

Quoted 6 lines

drivers/iommu/arm/arm-smmu/qcom_iommu.c +1 -1

@@ -390,7 +390,7 @@ 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 iommu_domain *domain = iommu_get_domain_for_dev(dev);
+	struct iommu_domain *domain = iommu_get_domain_for_dev_locked(dev);

Because this is a very common pattern in drivers.

Once that is done we can see what calls to iommu_get_domain_for_dev() are even left,

arguably we should be trying to eliminate this badly locked thing...

Quoted 4 lines

Because this is a very common pattern in drivers.

Once that is done we can see what calls to iommu_get_domain_for_dev() are even left,

... I found that in SMMUv3 driver, iommu_get_domain_for_dev() is used to get the RID domain for an SVA domain:
arm_smmu_set_pasid() arm_smmu_blocking_set_dev_pasid()

These two are already given an "old" (SVA) domain pointer, FWIW.

So, we may change to passing in the old domain as you suggested, yet we still have to fix the iommu_get_domain_for_dev() in order to reflect the RID domain correctly for the driver that calls it (or even potentially) in some group->mutex locked context where the RID domain might not be naturally passed in.

Quoted 2 lines

arguably we should be trying to eliminate this badly locked thing...

Any suggestion?

Quoted 12 lines · Nicolin Chen

Quoted 4 lines

Because this is a very common pattern in drivers.

Once that is done we can see what calls to iommu_get_domain_for_dev() are even left,

... I found that in SMMUv3 driver, iommu_get_domain_for_dev() is used to get the RID domain for an SVA domain:
arm_smmu_set_pasid() arm_smmu_blocking_set_dev_pasid()

These two are already given an "old" (SVA) domain pointer, FWIW.

Quoted 5 lines

So, we may change to passing in the old domain as you suggested, yet we still have to fix the iommu_get_domain_for_dev() in order to reflect the RID domain correctly for the driver that calls it (or even potentially) in some group->mutex locked context where the RID domain might not be naturally passed in.

It could probably be avoided by keeping track of more information in the master, but also it is not so bad to use a _locked version here.

Quoted 4 lines

Quoted 2 lines

arguably we should be trying to eliminate this badly locked thing...

Any suggestion?

Bit by bit.. I counted 58 by grep

Changing attach will get rid of alot of them

Then there is stuff like this:

        domain = iommu_get_domain_for_dev(emu->card->dev);
        if (!domain || domain->type == IOMMU_DOMAIN_IDENTITY)
                return;

Which should be more like
if (iommu_get_translation_mode(dev) == IDENTITY)

With sensible internal locking

So that is another bunch. Not sure what will be left after.

Not saying to do all that here, just prefer we move toward that direction.

Yes, I've thought about that. The concern is that some other place someday may want to use iommu_get_domain_for_dev() in similar cases but would find that it doesn't work. So it would have to duplicate the domain pointer in its "master" structure.

Overall, having a _locked version feels cleaner to me.

Quoted 1 line

With sensible internal locking

Hmm, I feel this iommu_get_translation_mode() is somewhat the same as the current iommu_get_domain_for_dev(). It would just return the group->domain->type v.s. group->domain, right?

This doesn't have any UAF concern though.

Quoted 1 line

So that is another bunch. Not sure what will be left after.

I recall that some of the drivers manages their own domains, e.g.
drivers/gpu/drm/tegra/drm.c

So, they would want more out of the domain pointer than just type.

Quoted 6 lines · Nicolin Chen

Yes, I've thought about that. The concern is that some other place someday may want to use iommu_get_domain_for_dev() in similar cases but would find that it doesn't work. So it would have to duplicate the domain pointer in its "master" structure.

Overall, having a _locked version feels cleaner to me.

We probably need the locked version, but it just shouldn't be called very much..

Quoted 7 lines

Quoted 1 line

With sensible internal locking

Hmm, I feel this iommu_get_translation_mode() is somewhat the same as the current iommu_get_domain_for_dev(). It would just return the group->domain->type v.s. group->domain, right?

This doesn't have any UAF concern though.

Yes, no UAF concern is the point

Quoted 6 lines

Quoted 1 line

So that is another bunch. Not sure what will be left after.

I recall that some of the drivers manages their own domains, e.g.
drivers/gpu/drm/tegra/drm.c

So, they would want more out of the domain pointer than just type.

This looks like it wants an 'is currently attached' operation

Yet, I also see some other cases that cannot be helped with the type function. Just listing a few:

1) domain matching (and type)
drivers/gpu/drm/tegra/drm.c:965: if (domain && domain->type != IOMMU_DOMAIN_IDENTITY &&
drivers/gpu/drm/tegra/drm.c:966: domain != tegra->domain)
drivers/gpu/drm/tegra/drm.c-967- return 0;

2) page size
drivers/gpu/drm/arm/malidp_planes.c:307: mmu_dom = iommu_get_domain_for_dev(mp->base.dev->dev);
drivers/gpu/drm/arm/malidp_planes.c-308- if (mmu_dom)
drivers/gpu/drm/arm/malidp_planes.c-309- return mmu_dom->pgsize_bitmap;

3) iommu_iova_to_phys
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c:2597: dom = iommu_get_domain_for_dev(adev->dev);
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c-2598-
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c-2599- while (size) {
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c-2600- phys_addr_t addr = *pos & PAGE_MASK;
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c-2601- loff_t off = *pos & ~PAGE_MASK;

drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c-2602- size_t bytes = PAGE_SIZE - off;
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c-2603- unsigned long pfn;
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c-2604- struct page *p;
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c-2605- void *ptr;
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c-2606-
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c-2607- bytes = min(bytes, size);
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c-2608-
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c:2609: addr = dom ? iommu_iova_to_phys(dom, addr) : addr;

4) map/unmap
drivers/net/ipa/ipa_mem.c:465: domain = iommu_get_domain_for_dev(dev); drivers/net/ipa/ipa_mem.c-466- if (!domain) {
drivers/net/ipa/ipa_mem.c-467- dev_err(dev, "no IOMMU domain found for IMEM\n");
drivers/net/ipa/ipa_mem.c-468- return -EINVAL;
drivers/net/ipa/ipa_mem.c-469- } drivers/net/ipa/ipa_mem.c-470-

drivers/net/ipa/ipa_mem.c-471-  /* Align the address down and the size up to page boundaries */
drivers/net/ipa/ipa_mem.c-472-  phys = addr & PAGE_MASK;
drivers/net/ipa/ipa_mem.c-473-  size = PAGE_ALIGN(size + addr - phys);
drivers/net/ipa/ipa_mem.c-474-  iova = phys;    /* We just want a direct mapping */

drivers/net/ipa/ipa_mem.c-475-
drivers/net/ipa/ipa_mem.c-476- ret = iommu_map(domain, iova, phys, size, IOMMU_READ | IOMMU_WRITE,
...
drivers/net/ipa/ipa_mem.c:495: domain = iommu_get_domain_for_dev(dev); drivers/net/ipa/ipa_mem.c-496- if (domain) {
drivers/net/ipa/ipa_mem.c-497- size_t size;
drivers/net/ipa/ipa_mem.c-498-
drivers/net/ipa/ipa_mem.c-499- size = iommu_unmap(domain, ipa->imem_iova, ipa->imem_size);

Quoted 2 lines · Nicolin Chen

Yet, I also see some other cases that cannot be helped with the type function. Just listing a few:

Probably several query functions are needed that can be lock safe

Quoted 4 lines

1) domain matching (and type)
drivers/gpu/drm/tegra/drm.c:965: if (domain && domain->type != IOMMU_DOMAIN_IDENTITY &&
drivers/gpu/drm/tegra/drm.c:966: domain != tegra->domain)
drivers/gpu/drm/tegra/drm.c-967- return 0;

is attached

Quoted 11 lines

4) map/unmap
drivers/net/ipa/ipa_mem.c:465: domain = iommu_get_domain_for_dev(dev); drivers/net/ipa/ipa_mem.c-466- if (!domain) {
drivers/net/ipa/ipa_mem.c-467- dev_err(dev, "no IOMMU domain found for IMEM\n");
drivers/net/ipa/ipa_mem.c-468- return -EINVAL;
drivers/net/ipa/ipa_mem.c-469- } drivers/net/ipa/ipa_mem.c-470-

drivers/net/ipa/ipa_mem.c-471-  /* Align the address down and the size up to page boundaries */
drivers/net/ipa/ipa_mem.c-472-  phys = addr & PAGE_MASK;
drivers/net/ipa/ipa_mem.c-473-  size = PAGE_ALIGN(size + addr - phys);
drivers/net/ipa/ipa_mem.c-474-  iova = phys;    /* We just want a direct mapping */

Quoted 8 lines

drivers/net/ipa/ipa_mem.c-475-
drivers/net/ipa/ipa_mem.c-476- ret = iommu_map(domain, iova, phys, size, IOMMU_READ | IOMMU_WRITE,
...
drivers/net/ipa/ipa_mem.c:495: domain = iommu_get_domain_for_dev(dev); drivers/net/ipa/ipa_mem.c-496- if (domain) {
drivers/net/ipa/ipa_mem.c-497- size_t size;
drivers/net/ipa/ipa_mem.c-498-
drivers/net/ipa/ipa_mem.c-499- size = iommu_unmap(domain, ipa->imem_iova, ipa->imem_size);

Broken! Illegal to call iommu_map on a DMA API domain.

This is exactly the sort of abuse I would like to see made imposible :(

If it really needs something like this then it needs a proper dma api interface to do it and properly reserve the iova from the allocator.

I should have pasted the full piece:
drivers/gpu/drm/tegra/drm.c-960- /*
drivers/gpu/drm/tegra/drm.c:961: * If the host1x client is already attached to an IOMMU domain that is
drivers/gpu/drm/tegra/drm.c-962- * not the shared IOMMU domain, don't try to attach it to a different
drivers/gpu/drm/tegra/drm.c-963- * domain. This allows using the IOMMU-backed DMA API.
drivers/gpu/drm/tegra/drm.c-964- */
drivers/gpu/drm/tegra/drm.c-965- if (domain && domain->type != IOMMU_DOMAIN_IDENTITY &&
drivers/gpu/drm/tegra/drm.c-966- domain != tegra->domain)

So, the check is two-fold:
1) is attached
2) is the shared IOMMU domain (tegra->domain?)

Overall, I feel this would be a big project yet arguably for a low reward..

Quoted 9 lines · Nicolin Chen

I should have pasted the full piece:
drivers/gpu/drm/tegra/drm.c-960- /*
drivers/gpu/drm/tegra/drm.c:961: * If the host1x client is already attached to an IOMMU domain that is
drivers/gpu/drm/tegra/drm.c-962- * not the shared IOMMU domain, don't try to attach it to a different
drivers/gpu/drm/tegra/drm.c-963- * domain. This allows using the IOMMU-backed DMA API.
drivers/gpu/drm/tegra/drm.c-964- */
drivers/gpu/drm/tegra/drm.c-965- if (domain && domain->type != IOMMU_DOMAIN_IDENTITY &&
drivers/gpu/drm/tegra/drm.c-966- domain != tegra->domain)

Quoted 3 lines

So, the check is two-fold:
1) is attached
2) is the shared IOMMU domain (tegra->domain?)

Yea

iommu_is_domain_currently_attached(dev, tegra->domain)

Quoted 2 lines

Overall, I feel this would be a big project yet arguably for a low reward..

Indeed, we can drop a FIXME comment there and leave it as the last user or something perhaps

From: Nicolin Chen <[email protected]>
Sent: Tuesday, August 19, 2025 1:23 AM

... I found that in SMMUv3 driver, iommu_get_domain_for_dev() is used to get the RID domain for an SVA domain:
arm_smmu_set_pasid() arm_smmu_blocking_set_dev_pasid()

These two are already given an "old" (SVA) domain pointer, FWIW.

So, we may change to passing in the old domain as you suggested, yet we still have to fix the iommu_get_domain_for_dev() in order to reflect the RID domain correctly for the driver that calls it (or even potentially) in some group->mutex locked context where the RID domain might not be naturally passed in.

Quoted 8 lines

From: Nicolin Chen <[email protected]>
Sent: Tuesday, August 19, 2025 1:23 AM

... I found that in SMMUv3 driver, iommu_get_domain_for_dev() is used to get the RID domain for an SVA domain:
arm_smmu_set_pasid() arm_smmu_blocking_set_dev_pasid()

Quoted 8 lines

These two are already given an "old" (SVA) domain pointer, FWIW.

So, we may change to passing in the old domain as you suggested, yet we still have to fix the iommu_get_domain_for_dev() in order to reflect the RID domain correctly for the driver that calls it (or even potentially) in some group->mutex locked context where the RID domain might not be naturally passed in.

Out of curiosity.

arm_smmu_blocking_set_dev_pasid()

	/*                              
	 * When the last user of the CD table goes away downgrade the STE back
	 * to a non-cd_table one.
	 */
	if (!arm_smmu_ssids_in_use(&master->cd_table)) {
		struct iommu_domain *sid_domain =
			iommu_get_domain_for_dev(master->dev);

		if (sid_domain->type == IOMMU_DOMAIN_IDENTITY ||
		   sid_domain->type == IOMMU_DOMAIN_BLOCKED)
			sid_domain->ops->attach_dev(sid_domain, dev); 
	}

why cannot downgrade apply to the case where the RID is attached to a DMA domain?

From: Jason Gunthorpe <[email protected]>
Sent: Thursday, August 21, 2025 9:14 PM

Quoted 17 lines · Kevin

Quoted 8 lines

From: Nicolin Chen <[email protected]>
Sent: Tuesday, August 19, 2025 1:23 AM

... I found that in SMMUv3 driver, iommu_get_domain_for_dev() is used to get the RID domain for an SVA domain:
arm_smmu_set_pasid() arm_smmu_blocking_set_dev_pasid()

Quoted 8 lines

These two are already given an "old" (SVA) domain pointer, FWIW.

So, we may change to passing in the old domain as you suggested, yet we still have to fix the iommu_get_domain_for_dev() in order to reflect the RID domain correctly for the driver that calls it (or even potentially) in some group->mutex locked context where the RID domain might not be naturally passed in.

Quoted 6 lines

Out of curiosity.

arm_smmu_blocking_set_dev_pasid()

	/*
	 * When the last user of the CD table goes away downgrade the STE

back

Quoted 11 lines

	 * to a non-cd_table one.
	 */
	if (!arm_smmu_ssids_in_use(&master->cd_table)) {
		struct iommu_domain *sid_domain =
			iommu_get_domain_for_dev(master->dev);

		if (sid_domain->type == IOMMU_DOMAIN_IDENTITY ||
		   sid_domain->type == IOMMU_DOMAIN_BLOCKED)
			sid_domain->ops->attach_dev(sid_domain, dev);
	}

Quoted 2 lines

why cannot downgrade apply to the case where the RID is attached to a DMA domain?

If the RID is a PAGING domain then it must be a S1 paging domain and there is
no downgrade possible.

It is impossible for the RID to be a S2 paging domain while ssids are in use.

Thanks for the background. btw is it technically impossible or just not worth of the extra software complexity for no value?

e.g. if maintaining two page tables (S1/S2 formats) with exact same mappings, does SMMU allow smooth transition between two modes w/o breaking in-fly DMAs?

but probably keeping two page tables in-sync in transition is already a problem w/o proper locking in map/unmap...

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 all the attached domain to BLOCKED. After attaching a BLOCKED domain, IOMMU drivers should fence any incoming

ATS queries, 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 to defer an attachment during a reset, allowing iommu core to cache target domains in the SW level while bypassing the driver. The iommu_dev_reset_done() will re-attach these soft-attached domains, once the device reset is finished.

Signed-off-by Nicolin Chen<[email protected]>

Quoted 3 lines · Nicolin Chen

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 drivers should fence any incoming

Nit, my understanding is that it's not the "IOMMU drivers" but the "IOMMU hardware" that fences any further incoming translation requests, right?

From: Nicolin Chen <[email protected]>
Sent: Tuesday, August 12, 2025 6:59 AM

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 to defer an attachment during a reset, allowing iommu core to cache target domains in the SW level while bypassing the driver. The iommu_dev_reset_done() will re-attach these soft-attached domains, once the device reset is finished.

Quoted 3 lines

From: Nicolin Chen <[email protected]>
Sent: Tuesday, August 12, 2025 6:59 AM

[...]

Quoted 6 lines

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 to defer an attachment during a reset, allowing iommu core to cache target domains in the SW level while bypassing the driver. The iommu_dev_reset_done() will re-attach these soft-attached domains, once the device reset is finished.

attach could fail e.g. when device and domain are incompatible. This deferred attach (until reset done) makes compatibility check impossible in the resetting window, given the driver attach_dev callback is not called in the normal attach path.

Worse..

drivers/iommu/iommu.c int iommu_replace_group_handle(struct iommu_group *group, · line 3960

Quoted 3 lines

drivers/iommu/iommu.c +3 -0

+	/* Shift RID domain back to group->domain */
+	if (group->domain != group->blocking_domain)
+		WARN_ON(__iommu_attach_device(group->domain, dev));

..means that an user could trigger WARN_ON() easily if he knows an attach would fail.

IMHO we may just fail attach request in the resetting window then WARN_ON above makes sense as it's shifting back to a domain which was originally attached to the resetting device.

Not sure any valid case where one would want to attach/replace domain for a resetting device...

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

Signed-off-by Nicolin Chen <[email protected]>

---
  drivers/pci/pci-acpi.c | 17 +++++++++--
  drivers/pci/pci.c      | 68 ++++++++++++++++++++++++++++++++++++++----
  drivers/pci/quirks.c   | 23 +++++++++++++-
  3 files changed, 100 insertions(+), 8 deletions(-)

drivers/pci/pci-acpi.c +15 -2

index ddb25960ea47d..adaf46422c05d 100644
@@ -9,6 +9,7 @@
 
  #include <linux/delay.h>
  #include <linux/init.h>
+#include <linux/iommu.h>
  #include <linux/irqdomain.h>
  #include <linux/pci.h>
  #include <linux/msi.h>

@@ -969,6 +970,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 +978,23 @@ int pci_dev_acpi_reset(struct pci_dev *dev, bool probe)
  	if (probe)
  		return 0;
 
+	/*
+	 * Per PCIe r6.3, sec 10.3.1 IMPLEMENTATION NOTE, software disables ATS
+	 * before initiating a reset. Notify the iommu driver that enabled ATS.
+	 */
+	ret = iommu_dev_reset_prepare(&dev->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;
+	iommu_dev_reset_done(&dev->dev);
+	return ret;
  }
 
  bool acpi_pci_power_manageable(struct pci_dev *dev)

drivers/pci/pci.c +2 -0

index b0f4d98036cdd..d6d87e22d81b3 100644
@@ -13,6 +13,7 @@
  #include <linux/delay.h>
  #include <linux/dmi.h>
  #include <linux/init.h>
+#include <linux/iommu.h>
  #include <linux/msi.h>
  #include <linux/of.h>
  #include <linux/pci.h>

@@ -4529,13 +4530,26 @@ EXPORT_SYMBOL(pci_wait_for_pending_transaction);
   */
  int pcie_flr(struct pci_dev *dev)
  {
+	int ret = 0;

drivers/pci/pci.c +53 -5

+		pci_err(dev, "failed to stop IOMMU\n");
+		return ret;
+	}
+
  	pcie_capability_set_word(dev, PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_BCR_FLR);
 
  	if (dev->imm_ready)
-		return 0;
+		goto done;
 
  	/*
  	 * Per PCIe r4.0, sec 6.6.2, a device must complete an FLR within

@@ -4544,7 +4558,11 @@ int pcie_flr(struct pci_dev *dev)
  	 */
  	msleep(100);
 
-	return pci_dev_wait(dev, "FLR", PCIE_RESET_READY_POLL_MS);
+	ret = pci_dev_wait(dev, "FLR", PCIE_RESET_READY_POLL_MS);
+
+done:
+	iommu_dev_reset_done(&dev->dev);
+	return ret;
  }
  EXPORT_SYMBOL_GPL(pcie_flr);
 

@@ -4572,6 +4590,7 @@ EXPORT_SYMBOL_GPL(pcie_reset_flr);
 
  static int pci_af_flr(struct pci_dev *dev, bool probe)
  {
+	int ret = 0;
  	int pos;
  	u8 cap;
 

@@ -4598,10 +4617,21 @@ static int pci_af_flr(struct pci_dev *dev, bool probe)
  				 PCI_AF_STATUS_TP << 8))
  		pci_err(dev, "timed out waiting for pending transaction; performing AF function level reset anyway\n");
 
+	/*
+	 * Per PCIe r6.3, sec 10.3.1 IMPLEMENTATION NOTE, software disables ATS
+	 * before initiating a reset. Notify the iommu driver that enabled ATS.
+	 * Have to call it after waiting for pending DMA transaction.
+	 */
+	ret = iommu_dev_reset_prepare(&dev->dev);
+	if (ret) {
+		pci_err(dev, "failed to stop IOMMU\n");
+		return ret;
+	}
+
  	pci_write_config_byte(dev, pos + PCI_AF_CTRL, PCI_AF_CTRL_FLR);
 
  	if (dev->imm_ready)
-		return 0;
+		goto done;
 
  	/*
  	 * Per Advanced Capabilities for Conventional PCI ECN, 13 April 2006,

@@ -4611,7 +4641,11 @@ static int pci_af_flr(struct pci_dev *dev, bool probe)
  	 */
  	msleep(100);
 
-	return pci_dev_wait(dev, "AF_FLR", PCIE_RESET_READY_POLL_MS);
+	ret = pci_dev_wait(dev, "AF_FLR", PCIE_RESET_READY_POLL_MS);
+
+done:
+	iommu_dev_reset_done(&dev->dev);
+	return ret;
  }
 
  /**

@@ -4632,6 +4666,7 @@ static int pci_af_flr(struct pci_dev *dev, bool probe)
  static int pci_pm_reset(struct pci_dev *dev, bool probe)
  {
  	u16 csr;
+	int ret;
 
  	if (!dev->pm_cap || dev->dev_flags & PCI_DEV_FLAGS_NO_PM_RESET)
  		return -ENOTTY;

@@ -4646,6 +4681,16 @@ static int pci_pm_reset(struct pci_dev *dev, bool probe)
  	if (dev->current_state != PCI_D0)
  		return -EINVAL;
 
+	/*
+	 * Per PCIe r6.3, sec 10.3.1 IMPLEMENTATION NOTE, software disables ATS
+	 * before initiating a reset. Notify the iommu driver that enabled ATS.
+	 */
+	ret = iommu_dev_reset_prepare(&dev->dev);
+	if (ret) {
+		pci_err(dev, "failed to stop IOMMU\n");
+		return ret;
+	}
+
  	csr &= ~PCI_PM_CTRL_STATE_MASK;
  	csr |= PCI_D3hot;
  	pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, csr);

@@ -4656,7 +4701,9 @@ static int pci_pm_reset(struct pci_dev *dev, bool probe)
  	pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, csr);
  	pci_dev_d3_sleep(dev);
 
-	return pci_dev_wait(dev, "PM D3hot->D0", PCIE_RESET_READY_POLL_MS);
+	ret = pci_dev_wait(dev, "PM D3hot->D0", PCIE_RESET_READY_POLL_MS);
+	iommu_dev_reset_done(&dev->dev);
+	return ret;
  }
 
  /**

@@ -5111,6 +5158,16 @@ static int cxl_reset_bus_function(struct pci_dev *dev, bool probe)
  	if (rc)
  		return -ENOTTY;
 
+	/*
+	 * Per PCIe r6.3, sec 10.3.1 IMPLEMENTATION NOTE, software disables ATS
+	 * before initiating a reset. Notify the iommu driver that enabled ATS.
+	 */
+	rc = iommu_dev_reset_prepare(&dev->dev);
+	if (rc) {
+		pci_err(dev, "failed to stop IOMMU\n");
+		return rc;
+	}
+
  	if (reg & PCI_DVSEC_CXL_PORT_CTL_UNMASK_SBR) {
  		val = reg;
  	} else {

@@ -5125,6 +5182,7 @@ static int cxl_reset_bus_function(struct pci_dev *dev, bool probe)
  		pci_write_config_word(bridge, dvsec + PCI_DVSEC_CXL_PORT_CTL,
  				      reg);
 
+	iommu_dev_reset_done(&dev->dev);
  	return rc;
  }
 

drivers/pci/quirks.c +22 -1

index d97335a401930..6157c6c02bdb0 100644
@@ -21,6 +21,7 @@
  #include <linux/pci.h>
  #include <linux/isa-dma.h> /* isa_dma_bridge_buggy */
  #include <linux/init.h>
+#include <linux/iommu.h>
  #include <linux/delay.h>
  #include <linux/acpi.h>
  #include <linux/dmi.h>

@@ -4225,6 +4226,26 @@ 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;
+
+	/*
+	 * Per PCIe r6.3, sec 10.3.1 IMPLEMENTATION NOTE, software disables ATS
+	 * before initiating a reset. Notify the iommu driver that enabled ATS.
+	 */
+	ret = iommu_dev_reset_prepare(&dev->dev);
+	if (ret) {
+		pci_err(dev, "failed to stop IOMMU\n");
+		return ret;
+	}
+
+	ret = i->reset(dev, probe);
+	iommu_dev_reset_done(&dev->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 +4260,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;

Quoted 13 lines · Nicolin Chen

drivers/pci/pci.c +7 -0

@@ -4529,13 +4530,26 @@ EXPORT_SYMBOL(pci_wait_for_pending_transaction);
   */
  int pcie_flr(struct pci_dev *dev)
  {
+	int ret = 0;
+
  	if (!pci_wait_for_pending_transaction(dev))
  		pci_err(dev, "timed out waiting for pending transaction; performing function level reset anyway\n");
+	/*
+	 * Per PCIe r6.3, sec 10.3.1 IMPLEMENTATION NOTE, software disables ATS
+	 * before initiating a reset. Notify the iommu driver that enabled ATS.
+	 * Have to call it after waiting for pending DMA transaction.
+	 */

Review EXPORT_SYMBOL(pci_wait_for_pending_transaction); · line 4542

Quoted 1 line

(quoted diff) +1 -0

+	ret = iommu_dev_reset_prepare(&dev->dev);

If we dont' consider the association between IOMMU and devices in FLR(), it can be understood that more complex processing logic resides outside this function. However, if the FLR() function already synchironizes and handles the association with IOMMU like this (disabling ATS by attaching device to blocking domain), then how would the following scenarios behave ?

1. Reset one of PCIe alias devices.

2. Reset PF when its VFs are actvie.

Quoted 14 lines · Ethan Zhao

Quoted 13 lines · Nicolin Chen

drivers/pci/pci.c +7 -0

@@ -4529,13 +4530,26 @@ EXPORT_SYMBOL(pci_wait_for_pending_transaction);
    */
   int pcie_flr(struct pci_dev *dev)
   {
+	int ret = 0;
+
   	if (!pci_wait_for_pending_transaction(dev))
   		pci_err(dev, "timed out waiting for pending transaction; performing function level reset anyway\n");
+	/*
+	 * Per PCIe r6.3, sec 10.3.1 IMPLEMENTATION NOTE, software disables ATS
+	 * before initiating a reset. Notify the iommu driver that enabled ATS.
+	 * Have to call it after waiting for pending DMA transaction.
+	 */

Quoted 8 lines

Review EXPORT_SYMBOL(pci_wait_for_pending_transaction); · line 4542

Quoted 1 line

(quoted diff) +1 -0

+	ret = iommu_dev_reset_prepare(&dev->dev);

If we dont' consider the association between IOMMU and devices in FLR(), it can be understood that more complex processing logic resides outside this function. However, if the FLR() function already synchironizes and handles the association with IOMMU like this (disabling ATS by attaching device to blocking domain), then how would the following scenarios behave ?

That's a good point. The iommu-level reset is per struct device. So, basically it'll match with the FLR per pci_dev. Yet, the RID isolation between siblings might be a concern:

Quoted 1 line

1. Reset one of PCIe alias devices.

IIRC, an alias device might have:

a) one pci_dev; multiple RIDs

In this case, neither FLR nor IOMMU isolates between RIDs. So, both FLR and IOMMU blocking will reset all RIDs. There should be no issue resulted from the IOMMU blocking.

b) multiple pci_devs; single RID

In this case, FLR only resets one device, while the IOMMU- level reset will block the entire RID (i.e. all devices), since they share the single translation tunnel. This could break the siblings, if they aren't also being reset along.

Quoted 1 line

2. Reset PF when its VFs are actvie.

c) multiple pci_devs with their own RIDs

In this case, either FLR or IOMMU only resets the PF. That being said, VFs might be affected since PF is resetting? If there is an issue, I don't see it coming from the IOMMU- level reset..

d
Thus, case b might be breaking. So, perhaps we should add a few conditions when calling iommu_dev_reset_prepare/done():
+ Make sure that the pci_dev has ATS capability
+ Make sure no sibling pci_dev(s) sharing the same RID
+ Any others?

Thanks
Nicolin

Quoted 6 lines · Nicolin Chen

b) multiple pci_devs; single RID

In this case, FLR only resets one device, while the IOMMU- level reset will block the entire RID (i.e. all devices), since they share the single translation tunnel. This could break the siblings, if they aren't also being reset along.

Yup, such alias devices might not have ATS cap. because of they are PCI devices or they share the RID(BDF), so checking ATS cap condition might be useful here to skip the prepare()/done()

Quoted 8 lines

Quoted 1 line

2. Reset PF when its VFs are actvie.

c) multiple pci_devs with their own RIDs

In this case, either FLR or IOMMU only resets the PF. That being said, VFs might be affected since PF is resetting? If there is an issue, I don't see it coming from the IOMMU- level reset..

Each of the PF and its VFs has it owns RID(BDF), but the VFs' life depends on the living of PF, resetting PF, means all its VFs are lost.

There is no processing logic about PF and its VFs in FLR() yet. my understanding the upper layer callers should consider the complexity of such case.

While we introducing the connection of IOMMU & device in FLR(), seems we brought some of the logic from the outside to the inside part.

One method might we don't handle PF either by explicit checking its VF configuration existing to skip prepare()/done() ? till we have much clearer handling logic about it.

c) multiple pci_devs with their own RIDs

In this case, either FLR or IOMMU only resets the PF. That being said, VFs might be affected since PF is resetting? If there is an issue, I don't see it coming from the IOMMU- level reset..

Quoted 6 lines · Nicolin Chen

c) multiple pci_devs with their own RIDs

In this case, either FLR or IOMMU only resets the PF. That being said, VFs might be affected since PF is resetting? If there is an issue, I don't see it coming from the IOMMU- level reset..

It would still allow the ATS issue from the VF side. The VF could be pushing an invalidation during the PF reset that will get clobbered.

I haven't fully checked but I think Linux doesn't really (easially?) allow resetting a PF while a VF is present...

Arguably if the PF is reset the VFs should have their translations blocked too.

Quoted 8 lines · Jason Gunthorpe

Quoted 6 lines · Nicolin Chen

c) multiple pci_devs with their own RIDs

In this case, either FLR or IOMMU only resets the PF. That being said, VFs might be affected since PF is resetting? If there is an issue, I don't see it coming from the IOMMU- level reset..

Quoted 5 lines

It would still allow the ATS issue from the VF side. The VF could be pushing an invalidation during the PF reset that will get clobbered.

I haven't fully checked but I think Linux doesn't really (easially?) allow resetting a PF while a VF is present...

Hmm, what if the PF encountered some fault? Does Linux have a choice not to reset PF?

Quoted 2 lines

Arguably if the PF is reset the VFs should have their translations blocked too.

Yea, that sounds plausible to me. But, prior to that (an IOMMU-level reset), should VFs be first reset at the PCI level?

Quoted 15 lines · Nicolin Chen

Quoted 13 lines · Jason Gunthorpe

Quoted 6 lines · Nicolin Chen

c) multiple pci_devs with their own RIDs

In this case, either FLR or IOMMU only resets the PF. That being said, VFs might be affected since PF is resetting? If there is an issue, I don't see it coming from the IOMMU- level reset..

It would still allow the ATS issue from the VF side. The VF could be pushing an invalidation during the PF reset that will get clobbered.

I haven't fully checked but I think Linux doesn't really (easially?) allow resetting a PF while a VF is present...

Quoted 2 lines

Hmm, what if the PF encountered some fault? Does Linux have a choice not to reset PF?

Upon more reflect I guess outside VFIO I seem to remember the SRIOV reset to the PFs will clobber the VFs too and then restore the SRIOV configuration in config space to bring them back.

Quoted 5 lines

Quoted 2 lines

Arguably if the PF is reset the VFs should have their translations blocked too.

Yea, that sounds plausible to me. But, prior to that (an IOMMU-level reset), should VFs be first reset at the PCI level?

PF reset of a SRIOV PF disables the VFs and effectively resets them already.

But reaching out to mangle the translation of the VFs means you do have to take care not to disrupt anything else the VF owning driver is doing since it is fully unaware of this. Ie it could be reattaching to something else concurrently.

It feels like we need a no-fail re-attach operation, or at least an unlikely-to-fail one. I recall years ago we tried a can_attach op to test the compatibility but it didn't get merged. Maybe we'd need it so that a concurrent attach can test compatibility, allowing the re-attach in iommu_dev_reset_done() to more likely succeed.

Quoted 5 lines · Nicolin Chen

It feels like we need a no-fail re-attach operation, or at least an unlikely-to-fail one. I recall years ago we tried a can_attach op to test the compatibility but it didn't get merged. Maybe we'd need it so that a concurrent attach can test compatibility, allowing the re-attach in iommu_dev_reset_done() to more likely succeed.

This is probably the cleanest option to split these things

Quoted 8 lines · Jason Gunthorpe

Quoted 5 lines · Nicolin Chen

It feels like we need a no-fail re-attach operation, or at least an unlikely-to-fail one. I recall years ago we tried a can_attach op to test the compatibility but it didn't get merged. Maybe we'd need it so that a concurrent attach can test compatibility, allowing the re-attach in iommu_dev_reset_done() to more likely succeed.

Quoted 1 line

This is probably the cleanest option to split these things

Yea, that could avoid failing a concurrent attach_dev during FLR unless the dryrun fails, helping non-SRIOV cases too.

So, next version could have some new preparatory patches:
- Pass in old domain to attach_dev
- Add a can_attach_dev op

Quoted 14 lines · Nicolin Chen

Quoted 9 lines · Jason Gunthorpe

Quoted 5 lines · Nicolin Chen

It feels like we need a no-fail re-attach operation, or at least an unlikely-to-fail one. I recall years ago we tried a can_attach op to test the compatibility but it didn't get merged. Maybe we'd need it so that a concurrent attach can test compatibility, allowing the re-attach in iommu_dev_reset_done() to more likely succeed.

This is probably the cleanest option to split these things

Yea, that could avoid failing a concurrent attach_dev during FLR unless the dryrun fails, helping non-SRIOV cases too.

Quoted 3 lines

So, next version could have some new preparatory patches:
- Pass in old domain to attach_dev
- Add a can_attach_dev op

I wouldn't make this more complicated, just focus on the signal device case here then we move on from there

Just adding can_attach_dev is big series on its own