Disable ATS via iommu during PCI resets

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

Grab the lock to protect it like others who call __iommu_attach_device() as it will need to access dev->iommu_group.

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

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

This will need to check a per gdev property, since the dev pointer cannot store any private iommu flag for the iommu code to use. Thus, pass in the gdev pointer instead.

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

---
 drivers/iommu/iommu.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

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, the ATS might be re-enabled between the two function calls. Introduce a new pending_reset flag in group_device to defer an attachment during a reset, allowing iommu core to cache the target domains in the SW level but bypassing the driver. The iommu_dev_reset_done() will re-attach these soft-attached domains via __iommu_attach_device/set_group_pasid().

Notes:
- This only works for IOMMU drivers that implemented ops->blocked_domain correctly with pci_disable_ats().

blocked_domain is used? This might not be feasible because ops- blocked_domain might possibly be attached to a PASID of a device, while

- This only works for IOMMU drivers that will not issue ATS invalidation

    requests to the device, after it's docked at ops->blocked_domain.

Driver should fix itself to align with the aforementioned notes.

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

---
  include/linux/iommu.h |  12 ++++
  drivers/iommu/iommu.c | 158 ++++++++++++++++++++++++++++++++++++++++++
  2 files changed, 170 insertions(+)

include/linux/iommu.h +12 -0

index 156732807994..a17161b8625a 100644
@@ -1123,6 +1123,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);
 

@@ -1407,6 +1410,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 +41 -0

index bd3deedcd2de..14bfeaa9ac29 100644
@@ -71,12 +71,29 @@ struct group_device {
  	struct list_head list;
  	struct device *dev;
  	char *name;
+	bool pending_reset : 1;
  };
 
  /* Iterate over each struct group_device in a struct iommu_group */
  #define for_each_group_device(group, pos) \
  	list_for_each_entry(pos, &(group)->devices, list)
 
+/* Callers must hold the dev->iommu_group->mutex */
+static struct group_device *device_to_group_device(struct device *dev)
+{
+	struct iommu_group *group = dev->iommu_group;
+	struct group_device *gdev;
+
+	lockdep_assert_held(&group->mutex);
+
+	/* gdev must be in the list */
+	for_each_group_device(group, gdev) {
+		if (gdev->dev == dev)
+			break;
+	}
+	return gdev;
+}
+
  struct iommu_group_attribute {
  	struct attribute attr;
  	ssize_t (*show)(struct iommu_group *group, char *buf);

@@ -2155,8 +2172,17 @@ int iommu_deferred_attach(struct device *dev, struct iommu_domain *domain)
  	int ret = 0;
 
  	mutex_lock(&group->mutex);
+
+	/*
+	 * There is a racy attach while the device is resetting. Defer it until
+	 * the iommu_dev_reset_done() that attaches the device to group->domain.
+	 */
+	if (device_to_group_device(dev)->pending_reset)
+		goto unlock;
+
  	if (dev->iommu && dev->iommu->attach_deferred)
  		ret = __iommu_attach_device(domain, dev);
+unlock:
  	mutex_unlock(&group->mutex);
  	return ret;
  }

@@ -2295,6 +2321,13 @@ static int __iommu_device_set_domain(struct iommu_group *group,
  		dev->iommu->attach_deferred = 0;
  	}
 
+	/*
+	 * There is a racy attach while the device is resetting. Defer it until
+	 * the iommu_dev_reset_done() that attaches the device to group->domain.
+	 */
+	if (gdev->pending_reset)
+		return 0;
+
  	ret = __iommu_attach_device(new_domain, dev);
  	if (ret) {
  		/*

@@ -3378,6 +3411,13 @@ static int __iommu_set_group_pasid(struct iommu_domain *domain,
  	int ret;
 
  	for_each_group_device(group, device) {
+		/*
+		 * There is a racy attach while the device is resetting. Defer
+		 * it until the iommu_dev_reset_done() that attaches the device
+		 * to group->domain.
+		 */
+		if (device->pending_reset)
+			continue;
  		if (device->dev->iommu->max_pasids > 0) {
  			ret = domain->ops->set_dev_pasid(domain, device->dev,
  							 pasid, old);

@@ -3799,6 +3839,124 @@ int iommu_replace_group_handle(struct iommu_group *group,
  }
  EXPORT_SYMBOL_NS_GPL(iommu_replace_group_handle, "IOMMUFD_INTERNAL");
 
+/*

drivers/iommu/iommu.c +27 -0

+int iommu_dev_reset_prepare(struct device *dev)
+{
+	const struct iommu_ops *ops;
+	struct iommu_group *group;
+	unsigned long pasid;
+	void *entry;
+	int ret = 0;
+
+	if (!dev_has_iommu(dev))
+		return 0;
+
+	if (dev->iommu->require_direct) {
+		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;
+	}
+
+	/* group will be put in iommu_dev_reset_done() */
+	group = iommu_group_get(dev);
+
+	/* Caller ensures no racy iommu_release_device(), so this won't UAF */
+	mutex_lock(&group->mutex);
+
+	ops = dev_iommu_ops(dev);
+	if (!ops->blocked_domain) {
+		dev_warn(dev,

blocked_domain" instead of "group->domain->type ==

drivers/iommu/iommu.c +41 -0

+
+	/* Dock RID domain to blocked_domain while retaining group->domain */
+	ret = __iommu_attach_device(ops->blocked_domain, dev);
+	if (ret)
+		goto unlock;
+
+	/* Dock PASID domains to blocked_domain while retaining pasid_array */
+	xa_lock(&group->pasid_array);
+	xa_for_each_start(&group->pasid_array, pasid, entry, 1)
+		iommu_remove_dev_pasid(dev, pasid,
+				       pasid_array_entry_to_domain(entry));
+	xa_unlock(&group->pasid_array);
+
+unlock:
+	mutex_unlock(&group->mutex);
+	if (ret)
+		iommu_group_put(group);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(iommu_dev_reset_prepare);
+
+/*
+ * Pair with a previous iommu_dev_reset_prepare() that was successfully returned
+ *
+ * Note that, although unlikely, there is a risk that re-attaching domains might
+ * fail due to some unexpected happening like OOM.
+ */
+void iommu_dev_reset_done(struct device *dev)
+{
+	struct iommu_group *group = dev->iommu_group;
+	const struct iommu_ops *ops;
+	struct group_device *gdev;
+	unsigned long pasid;
+	void *entry;
+
+	if (!dev_has_iommu(dev))
+		return;
+
+	mutex_lock(&group->mutex);
+
+	gdev = device_to_group_device(dev);

drivers/iommu/iommu.c +12 -0

+
+	/* Shift RID domain back to group->domain */
+	WARN_ON(__iommu_attach_device(group->domain, dev));
+
+	/* Shift PASID domains back to domains retained in pasid_array */
+	xa_lock(&group->pasid_array);
+	xa_for_each_start(&group->pasid_array, pasid, entry, 1)
+		WARN_ON(__iommu_set_group_pasid(
+			pasid_array_entry_to_domain(entry), group, pasid,
+			ops->blocked_domain));
+	xa_unlock(&group->pasid_array);
+

Does this mean the IOMMU driver should disable ATS when ops-

Quoted 2 lines

blocked_domain is used? This might not be feasible because ops- blocked_domain might possibly be attached to a PASID of a device,

while other PASIDs still use ATS for functionality.

Quoted 3 lines

- This only works for IOMMU drivers that will not issue ATS invalidation

    requests to the device, after it's docked at ops->blocked_domain.

Driver should fix itself to align with the aforementioned notes.

My understanding of the requirements for the iommu drivers is: when all PASIDs are docked in the blocking DMA state, the IOMMU driver should:

- Flush all outstanding ATS invalidation requests;

- Stop issuing any further ATS invalidations;

- Configure the hardware to reject further ATS translation requests.

Quoted 34 lines

drivers/iommu/iommu.c +34 -0

+int iommu_dev_reset_prepare(struct device *dev)
+{
+	const struct iommu_ops *ops;
+	struct iommu_group *group;
+	unsigned long pasid;
+	void *entry;
+	int ret = 0;
+
+	if (!dev_has_iommu(dev))
+		return 0;
+
+	if (dev->iommu->require_direct) {
+		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;
+	}
+
+	/* group will be put in iommu_dev_reset_done() */
+	group = iommu_group_get(dev);
+
+	/* Caller ensures no racy iommu_release_device(), so this won't UAF */
+	mutex_lock(&group->mutex);
+
+	ops = dev_iommu_ops(dev);
+	if (!ops->blocked_domain) {
+		dev_warn(dev,
+			 "IOMMU driver doesn't support IOMMU_DOMAIN_BLOCKED\n");
+		ret = -EOPNOTSUPP;
+		goto unlock;
+	}
+
+	device_to_group_device(dev)->pending_reset = true;
+

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

Quoted 3 lines

drivers/iommu/iommu.c +3 -0

+	/* Device is already attached to the blocked_domain. Nothing to do */
+	if (group->domain->type == IOMMU_DOMAIN_BLOCKED)
+		goto unlock;

"group->domain->type == IOMMU_DOMAIN_BLOCKED" means that IOMMU_NO_PASID is docked in the blocking DMA state, but it doesn't imply that other PASIDs are also in the blocking DMA state. Therefore, we might still need the following lines to handle other PASIDs.

On the other hand, perhaps we should use "group->domain == ops-

Quoted 1 line

blocked_domain" instead of "group->domain->type ==

IOMMU_DOMAIN_BLOCKED" to make the code consistent with the commit message.

Quoted 4 lines · Baolu Lu

Does this mean the IOMMU driver should disable ATS when ops-

Quoted 2 lines

blocked_domain is used? This might not be feasible because ops- blocked_domain might possibly be attached to a PASID of a device,

while other PASIDs still use ATS for functionality.

No.. The above should be setting everything, including PASIDs to the blocked domain.

The driver doesn't have to disable ATS at the device, but ARM does.

Quoted 8 lines

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

Quoted 3 lines

drivers/iommu/iommu.c +3 -0

+	/* Device is already attached to the blocked_domain. Nothing to do */
+	if (group->domain->type == IOMMU_DOMAIN_BLOCKED)
+		goto unlock;

"group->domain->type == IOMMU_DOMAIN_BLOCKED" means that IOMMU_NO_PASID is docked in the blocking DMA state, but it doesn't imply that other PASIDs are also in the blocking DMA state. Therefore, we might still need the following lines to handle other PASIDs.

Yes, we always have to check the xarray.

Quoted 4 lines

On the other hand, perhaps we should use "group->domain == ops-

Quoted 1 line

blocked_domain" instead of "group->domain->type ==

IOMMU_DOMAIN_BLOCKED" to make the code consistent with the commit message.

ops->blocked_domain is not good, we support devices without static blocking domain. But yes, using DOMAIN_BLOCKED is not greap, there is a group->blocked_domain that should be used and will dynamicaly create an empty paging domain if needed.

Quoted 7 lines · Jason Gunthorpe

Quoted 4 lines · Baolu Lu

Does this mean the IOMMU driver should disable ATS when ops-

Quoted 2 lines

blocked_domain is used? This might not be feasible because ops- blocked_domain might possibly be attached to a PASID of a device,

while other PASIDs still use ATS for functionality.

Quoted 4 lines

No.. The above should be setting everything, including PASIDs to the blocked domain.

The driver doesn't have to disable ATS at the device, but ARM does.

Oh, the code is expecting a pci_disable_ats() call, as the next patch will check if ats is disabled on the PCI side..

If that's the case, we'd have to leave the ATS enabled but only trust that iommu driver won't issue any new ATS invalidation?

Quoted 4 lines

ops->blocked_domain is not good, we support devices without static blocking domain. But yes, using DOMAIN_BLOCKED is not greap, there is a group->blocked_domain that should be used and will dynamicaly create an empty paging domain if needed.

You mean we should use the group->blocking_domain, even if it was allocated to be a paging domain as the driver doesn't understand a IOMMU_DOMAIN_BLOCKED yet?

- This only works for IOMMU drivers that implemented ops->blocked_domain correctly with pci_disable_ats().

- This only works for IOMMU drivers that will not issue ATS invalidation requests to the device, after it's docked at ops->blocked_domain.

drivers/iommu/iommu.c +2 -0

@@ -2155,8 +2172,17 @@ int iommu_deferred_attach(struct device *dev, struct iommu_domain *domain)
 	int ret = 0;
 
 	mutex_lock(&group->mutex);
+
+	/*

drivers/iommu/iommu.c +7 -0

+int iommu_dev_reset_prepare(struct device *dev)
+{
+	const struct iommu_ops *ops;
+	struct iommu_group *group;
+	unsigned long pasid;
+	void *entry;
+	int ret = 0;

Quoted 2 lines · Nicolin Chen

- This only works for IOMMU drivers that implemented ops->blocked_domain correctly with pci_disable_ats().

As was in the thread, it works for everyone. Even if we install an empty paging domain for blocking that still will stop the ATS invalidations from being issued. ATS remains on but this is not a problem.

Quoted 6 lines

drivers/iommu/iommu.c +2 -0

@@ -2155,8 +2172,17 @@ int iommu_deferred_attach(struct device *dev, struct iommu_domain *domain)
 	int ret = 0;
 
 	mutex_lock(&group->mutex);
+
+	/*

drivers/iommu/iommu.c int iommu_deferred_attach(struct device *dev, struct iommu_domain *domain) · line 2186

Quoted 10 lines

drivers/iommu/iommu.c +7 -0

+	 * There is a racy attach while the device is resetting. Defer it until
+	 * the iommu_dev_reset_done() that attaches the device to group->domain.
+	 */
+	if (device_to_group_device(dev)->pending_reset)
+		goto unlock;
+
 	if (dev->iommu && dev->iommu->attach_deferred)
 		ret = __iommu_attach_device(domain, dev);
+unlock:
 	mutex_unlock(&group->mutex);

Actually looking at this some more maybe write it like:

/*
* 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 still single threaded to avoid racing. */
if (dev->iommu && !READ_ONCE(dev->iommu->attach_deferred))

   return 0;

guard(mutex)(&group->mutex);

if (device_to_group_device(dev)->pending_reset)

    return 0;

if (!dev->iommu->attach_deferred)
return 0;

return __iommu_attach_device(domain, dev);

And of course it is already quite crazy to be doing FLR during a device probe so this is not a realistic scenario.

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

Quoted 6 lines

drivers/iommu/iommu.c +6 -0

+	if (dev->iommu->require_direct) {
+		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;
+	}

I don't think we can do this. eg on ARM all devices have RMRs inside VMs so this will completely break FLR inside a vm???

Either ignore this condition with the rational that we are about to reset it so it doesn't matter, or we need to establish a new paging domain for isolation purposes that has the RMR setup.

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

Quoted 5 lines

drivers/iommu/iommu.c +5 -0

+	if (ret)
+		goto unlock;
+
+	/* Dock PASID domains to blocked_domain while retaining pasid_array */
+	xa_lock(&group->pasid_array);

Not sure we need this lock? The group mutex already prevents mutation of the xa list and I dont' think it is allowed to call iommu_remove_dev_pasid() in an atomic context.

Quoted 3 lines

    return 0;

guard(mutex)(&group->mutex);

I recall Baolu mentioned that Joerg might not like the guard style so I am keeping mutex_lock/unlock().

Quoted 5 lines · Nicolin Chen

Quoted 3 lines

    return 0;

guard(mutex)(&group->mutex);

I recall Baolu mentioned that Joerg might not like the guard style so I am keeping mutex_lock/unlock().

You may be misremembering or mixing something up. I didn't see Joerg express that opinion. :-)

My understanding is that cleanup.h could be used in new or refactored code, but people don't like converting existing lock/unlock mechanisms for no real benefit.

Quoted 6 lines

/*
* 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 still single threaded to avoid racing. */
if (dev->iommu && !READ_ONCE(dev->iommu->attach_deferred))

This triggers a build error as attach_deferred is a bit-field. So I am changing it from "u32 attach_deferred:1" to "bool" for this.

And, to keep the original logic, I think it should be:

	if (!dev->iommu || !READ_ONCE(dev->iommu->attach_deferred))

Quoted 2 lines

And of course it is already quite crazy to be doing FLR during a device probe so this is not a realistic scenario.

Hmm, I am not sure about that, as I see iommu_deferred_attach() get mostly invoked by a dma_alloc() or even a dma_map(). So, this might not be confined to a device probe?

Quoted 3 lines

Either ignore this condition with the rational that we are about to reset it so it doesn't matter, or we need to establish a new paging domain for isolation purposes that has the RMR setup.

Ah, you are right. ARM MSI in a VM uses RMR and sets this.

But does it also raise a question that a VM having RMR can't use the blocked_domain, as __iommu_device_set_domain() has the exact same check rejecting blocked_domain? Not sure if there would be some unintended consequnce though...

Quoted 7 lines · Nicolin Chen

Quoted 6 lines

/*
* 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 still single threaded to avoid racing. */
if (dev->iommu && !READ_ONCE(dev->iommu->attach_deferred))

Quoted 2 lines

This triggers a build error as attach_deferred is a bit-field. So I am changing it from "u32 attach_deferred:1" to "bool" for this.

Bleck, that seems undesirable.

Quoted 2 lines

And, to keep the original logic, I think it should be:

	if (!dev->iommu || !READ_ONCE(dev->iommu->attach_deferred))

That doesn't seem right, if there is no iommu by the time a driver is probed there never will be an iommu and this device should be running in direct mode only.

Quoted 6 lines

Quoted 2 lines

And of course it is already quite crazy to be doing FLR during a device probe so this is not a realistic scenario.

Hmm, I am not sure about that, as I see iommu_deferred_attach() get mostly invoked by a dma_alloc() or even a dma_map(). So, this might not be confined to a device probe?

Once you do deferred_attach the first time it is done and won't have any further impact. So long as the dev->iommu->attach_deferred guards any changes to domains it is unlikely to be racing with FLR.

Quoted 6 lines

Quoted 3 lines

Either ignore this condition with the rational that we are about to reset it so it doesn't matter, or we need to establish a new paging domain for isolation purposes that has the RMR setup.

Ah, you are right. ARM MSI in a VM uses RMR and sets this.

Quoted 4 lines

But does it also raise a question that a VM having RMR can't use the blocked_domain, as __iommu_device_set_domain() has the exact same check rejecting blocked_domain? Not sure if there would be some unintended consequnce though...

Sounds like it needs some sorting out.. For the purposes of FLR I think the blocked domain is OK, so maybe just move some of those checks around?

Quoted 8 lines · Jason Gunthorpe

Quoted 7 lines · Nicolin Chen

Quoted 6 lines

/*
* 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 still single threaded to avoid racing. */
if (dev->iommu && !READ_ONCE(dev->iommu->attach_deferred))

Quoted 4 lines

Quoted 2 lines

This triggers a build error as attach_deferred is a bit-field. So I am changing it from "u32 attach_deferred:1" to "bool" for this.

Bleck, that seems undesirable.

But inevitable for READ_ONCE :(

Quoted 6 lines

Quoted 2 lines

And, to keep the original logic, I think it should be:

	if (!dev->iommu || !READ_ONCE(dev->iommu->attach_deferred))

That doesn't seem right, if there is no iommu by the time a driver is probed there never will be an iommu and this device should be running in direct mode only.

Well, the current function does:

	if (dev->iommu && dev->iommu->attach_deferred)
		return __iommu_attach_device(domain, dev);
	return 0;

So, matching to that logic, it would be:

	if (!dev->iommu || !dev->iommu->attach_deferred)
		return 0;
	return __iommu_attach_device(domain, dev);

then add guard(mutex).

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

When a device is resetting, any new domain attachment should be deferred, until the reset is finished. The iommu callback will log the

However, if there is a domain attachment/replacement happening during an ongoing reset, the ATS might be re-enabled between the two function calls. Introduce a new pending_reset flag in iommu_group to defer any attachment during a reset, allowing iommu core to cache the target domains in the SW level but bypassing the driver. The iommu_dev_reset_done() will re-attach these soft-attached domains via __iommu_attach_device/set_group_pasid().

Notes:
- This only works for IOMMU drivers that implemented ops->blocked_domain correctly with pci_disable_ats().
- This only works for IOMMU drivers that will not issue ATS invalidation requests to the device, after it's docked at ops->blocked_domain. Driver should fix itself to align with the aforementioned notes.

This is on Github:
https://github.com/nicolinc/iommufd/commits/iommu_dev_reset-rfcv2

Changelog
v2
* [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 (4):
iommu: Lock group->mutex in iommu_deferred_attach
iommu: Pass in gdev to __iommu_device_set_domain
iommu: Introduce iommu_dev_reset_prepare() and iommu_dev_reset_done()
pci: Suspend iommu function prior to resetting a device

  include/linux/iommu.h  |  12 +++
  drivers/iommu/iommu.c  | 180 ++++++++++++++++++++++++++++++++++++++---
  drivers/pci/pci-acpi.c |  21 ++++-
  drivers/pci/pci.c      |  84 +++++++++++++++++--
  drivers/pci/quirks.c   |  27 ++++++-
  5 files changed, 307 insertions(+), 17 deletions(-)

Quoted 14 lines · Nicolin Chen

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.

Quoted 6 lines

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.

This approach seems effective for reset operations initiated through software interface functions, but how would we handle those triggered by hardware mechanisms? For example, resets caused by PCIe DPC mechanisms, device firmware, or manual hot-plug operations?

Quoted 22 lines · Ethan Zhao

Quoted 14 lines · Nicolin Chen

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.

Quoted 6 lines

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.

Quoted 4 lines

This approach seems effective for reset operations initiated through software interface functions, but how would we handle those triggered by hardware mechanisms? For example, resets caused by PCIe DPC mechanisms, device firmware, or manual hot-plug operations?

That's a good point. But I am not sure what SW can do about those.

IIUIC, DPC resets PCI at the HW level, SW only gets a notification after the HW reset finishes. So, during this HW reset, iommu might issue ATC invalidations (resulting in invalidation timeout noises) since at the SW level the device is still actively attached to an IOMMU instance. Right?

Nicolin

At least, we can do some attempt in DPC and Hot-plug driver, and then push the hardware specification update to provide pre-reset notification for DPC & hotplug. does it make sense ?

Quoted 3 lines · Ethan Zhao

At least, we can do some attempt in DPC and Hot-plug driver, and then push the hardware specification update to provide pre-reset notification for DPC & hotplug. does it make sense ?

I think DPC is a different case..

If we get a DPC we should also push the iommu into blocking, disable ATS and abandon any outstanding ATC invalidations as part of recovering from the DPC. Once everythings is cleaned up we can set the

iommu back up again and allow the driver to recover the device.

I think the current series is a good step along that path, but we'd also need to improve the drivers to handle abandonding/aborting the ATC invalidations.

IMHO DPC and SW initiated reset are separate projects.

Jason

Quoted 7 lines · Jason Gunthorpe

Quoted 3 lines · Ethan Zhao

At least, we can do some attempt in DPC and Hot-plug driver, and then push the hardware specification update to provide pre-reset notification for DPC & hotplug. does it make sense ?

I think DPC is a different case..

More complex and practical case.

Quoted 3 lines

If we get a DPC we should also push the iommu into blocking, disable ATS and abandon any outstanding ATC invalidations as part of recovering from the DPC. Once everythings is cleaned up we can set the

Yup, even pure software resets, there might be ATC invalidation pending
(in software queue or HW queue).

Quoted 9 lines · Ethan Zhao

Quoted 6 lines · Jason Gunthorpe

Quoted 3 lines · Ethan Zhao

At least, we can do some attempt in DPC and Hot-plug driver, and then push the hardware specification update to provide pre-reset notification for DPC & hotplug. does it make sense ?

Quoted 2 lines

Quoted 1 line

I think DPC is a different case..

More complex and practical case.

I'm not sure about that, we do FLRs all the time as a normal part of VFIO and VMM operations. DPC is pretty rare, IMHO.

Quoted 5 lines

Quoted 3 lines

If we get a DPC we should also push the iommu into blocking, disable ATS and abandon any outstanding ATC invalidations as part of recovering from the DPC. Once everythings is cleaned up we can set the

Yup, even pure software resets, there might be ATC invalidation pending
(in software queue or HW queue).

The design of this patch series will require the iommu driver to wait for the in-flight ATC invalidations during the blocking domain

invalidations when the FLR is triggered.

We have been talking about DPC internally, and I think it will need a related, but different flow since DPC can unavoidably trigger ATC invalidation timeouts/failures and we must sensibly handle them in the

Jason

Quoted 5 lines

invalidations when the FLR is triggered.

We have been talking about DPC internally, and I think it will need a related, but different flow since DPC can unavoidably trigger ATC invalidation timeouts/failures and we must sensibly handle them in the

There is race window for software to handle.
And for DPC containing data corruption as priority, seems not rational to issue notification to software and then do resetting. alternative way might be async modal support in iommu ATC invalidation path ?