iommu&pci: Disable ATS during FLR resets

Per PCIe r6.3, sec 10.3.1 IMPLEMENTATION NOTE, software should disable ATS before initiating a Function Level Reset, and then ensure no invalidation requests being issued to a device when its ATS capability is disabled.

Since pci_enable_ats() and pci_disable_ats() are called by an IOMMU driver while an unsolicited FLR can happen at any time in the PCI layer, PCI code has to notify the IOMMU subsystem about the ongoing FLR. Add a pair of new IOMMU APIs that will be called by the PCI reset functions before&after the reset routines.

However, if there is a domain attachment/replacement happening during an ongoing reset, the ATS might be re-enabled between the two function calls. So the iommu_dev_reset_prepare() has to hold the group mutex to avoid this race condition, unitl iommu_dev_reset_done() is finished. Thus, these two functions are a strong pair that must be used together.

Inside the mutex, these two functions will dock all RID and PASID domains to an IOMMU_DOMAIN_BLOCKED. This would further disable ATS by two-fold: an IOMMU driver should disable ATS in its control bits (e.g. SMMU's STE.EATS) and an IOMMU driver should call pci_disable_ats() as well.

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.

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

---
  include/linux/iommu.h |  12 +++++
  drivers/iommu/iommu.c | 106 ++++++++++++++++++++++++++++++++++++++++++
  2 files changed, 118 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 +16 -0

index a4b606c591da..3c1854c5e55e 100644
@@ -3795,6 +3795,112 @@ int iommu_replace_group_handle(struct iommu_group *group,
  }
  EXPORT_SYMBOL_NS_GPL(iommu_replace_group_handle, "IOMMUFD_INTERNAL");
 
+/*
+ * Deadlock Alert
+ *
+ * Caller must use iommu_dev_reset_prepare() and iommu_dev_reset_done() together
+ * before/after the core-level reset routine, as iommu_dev_reset_prepare() holds
+ * the group->mutex that will be only released in iommu_dev_reset_done().
+ */
+int iommu_dev_reset_prepare(struct device *dev)
+{
+	struct iommu_group *group = dev->iommu_group;
+	const struct iommu_ops *ops;
+	unsigned long pasid;
+	void *entry;
+	int ret;
+
+	/* Before locking */

drivers/iommu/iommu.c +4 -0

+	if (!ops->blocked_domain) {
+		dev_warn(dev,
+			 "IOMMU driver doesn't support IOMMU_DOMAIN_BLOCKED\n");
+		return -EOPNOTSUPP;

drivers/iommu/iommu.c +33 -0

+
+	/* Device is already attached to the blocked_domain. Nothing to do */
+	if (group->domain->type == IOMMU_DOMAIN_BLOCKED)
+		return 0;
+
+	/* Dock RID domain to blocked_domain while retaining group->domain */
+	ret = __iommu_attach_device(ops->blocked_domain, dev);
+	if (ret)
+		return ret;
+
+	/* 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);
+
+	/* group->mutex is held. Caller must invoke iommu_dev_reset_done() */
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(iommu_dev_reset_prepare);
+
+/*
+ * This is the resume routine of iommu_dev_reset_prepare(). It unlocks the group
+ * mutex at end, after all RID/PASID domains are re-attached.
+ *
+ * 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;

drivers/iommu/iommu.c +13 -0

+
+	if (group->domain->type == IOMMU_DOMAIN_BLOCKED)
+		goto unlock;
+
+	/* 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));

Review

Quoted 1 line · Nicolin Chen

(quoted diff) +1 -0

+	ops = dev_iommu_ops(dev);

Should this be protected by group->mutext?

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

Quoted 8 lines

drivers/iommu/iommu.c +8 -0

+	/*
+	 * group->mutex starts
+	 *
+	 * This has to hold the group mutex until the reset is done, to prevent
+	 * any RID or PASID domain attachment/replacement, which otherwise might
+	 * re-enable the ATS during the reset cycle.
+	 */
+	mutex_lock(&group->mutex);

Is it possible that group has been freed when it reaches here?

Quoted 9 lines

drivers/iommu/iommu.c +9 -0

+void iommu_dev_reset_done(struct device *dev)
+{
+	struct iommu_group *group = dev->iommu_group;
+	const struct iommu_ops *ops;
+	unsigned long pasid;
+	void *entry;
+
+	/* Previously unlocked */
+	if (!dev_has_iommu(dev))

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

Quoted 4 lines

drivers/iommu/iommu.c +4 -0

+		return;
+	ops = dev_iommu_ops(dev);
+	if (!ops->blocked_domain)
+		return;

Should it be a WARN_ON()? As proposed, reset_prepare and reset_done must be paired. So if reset_prepare returns failure, reset_done should not be called. Or not?

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

Quoted 2 lines

drivers/iommu/iommu.c +2 -0

+	/* group->mutex held in iommu_dev_reset_prepare() continues from here */
+	WARN_ON(!lockdep_is_held(&group->mutex));

Probably iommu_group_mutex_assert() and move it up?

How about combining these two helpers? Something like,

int iommu_dev_block_dma_and_action(struct device *dev,
int (*action)(struct pci_dev *dev))

{
	prepare();
	action();
	done();
}

Quoted 4 lines · Baolu Lu

Review

Quoted 1 line · Nicolin Chen

(quoted diff) +1 -0

+	ops = dev_iommu_ops(dev);

Should this be protected by group->mutext?

Not seemingly, but should require the iommu_probe_device_lock I think.

Quoted 6 lines · Nicolin Chen

Quoted 4 lines · Baolu Lu

Review

Quoted 1 line · Nicolin Chen

(quoted diff) +1 -0

+	ops = dev_iommu_ops(dev);

Should this be protected by group->mutext?

Quoted 2 lines

Not seemingly, but should require the iommu_probe_device_lock I think.

group and ops are not permitted to change while a driver is attached..

IIRC the FLR code in PCI doesn't always ensure that (due to the sysfs paths), so yeah, this looks troubled. iommu_probe_device_lock perhaps would fix it.

Quoted 7 lines · Jason Gunthorpe

Quoted 6 lines · Nicolin Chen

Quoted 4 lines · Baolu Lu

Review

Quoted 1 line · Nicolin Chen

(quoted diff) +1 -0

+	ops = dev_iommu_ops(dev);

Should this be protected by group->mutext?

Quoted 8 lines

Quoted 2 lines

Not seemingly, but should require the iommu_probe_device_lock I think.

group and ops are not permitted to change while a driver is attached..

IIRC the FLR code in PCI doesn't always ensure that (due to the sysfs paths), so yeah, this looks troubled. iommu_probe_device_lock perhaps would fix it.

No, iommu_probe_device_lock is for protecting access to dev->iommu in the probe path until the device is definitively assigned to a group (or not). Fundamentally it defends against multiple sources triggering a probe of the same device in parallel - once the device *is* probed it is no longer relevant, and the group mutex is the right thing to protect all subsequent operations.

Quoted 17 lines · Robin Murphy

Quoted 7 lines · Jason Gunthorpe

Quoted 6 lines · Nicolin Chen

Quoted 4 lines · Baolu Lu

Review

Quoted 1 line · Nicolin Chen

(quoted diff) +1 -0

+	ops = dev_iommu_ops(dev);

Should this be protected by group->mutext?

Quoted 8 lines

Quoted 2 lines

Not seemingly, but should require the iommu_probe_device_lock I think.

group and ops are not permitted to change while a driver is attached..

IIRC the FLR code in PCI doesn't always ensure that (due to the sysfs paths), so yeah, this looks troubled. iommu_probe_device_lock perhaps would fix it.

Quoted 6 lines

No, iommu_probe_device_lock is for protecting access to dev->iommu in the probe path until the device is definitively assigned to a group (or not). Fundamentally it defends against multiple sources triggering a probe of the same device in parallel - once the device *is* probed it is no longer relevant, and the group mutex is the right thing to protect all subsequent operations.

Yes, adding iommu_probe_device_lock to iommu_deinit_device() would be gross.

but something is required to protect the load of dev->iommu_group.. dev->iommu_group->mutex can't protect itself against a race UAF on deinit.

READ_ONCE is good enough to protect from races with the probe path, no need for iommu_probe_device_lock there.

In this case need to look at the PCI sysfs for races against the iommu_release_device()/etc that is freeing the dev->iommu_group.

Maybe the sysfs is always removed before we get to release. Or maybe the PCI FLR sysfs code should hold the device_lock..

Quoted 25 lines · Jason Gunthorpe

Quoted 23 lines · Robin Murphy

Quoted 7 lines · Jason Gunthorpe

Quoted 6 lines · Nicolin Chen

Quoted 4 lines · Baolu Lu

Review

Quoted 1 line · Nicolin Chen

(quoted diff) +1 -0

+	ops = dev_iommu_ops(dev);

Should this be protected by group->mutext?

Quoted 8 lines

Quoted 2 lines

Not seemingly, but should require the iommu_probe_device_lock I think.

group and ops are not permitted to change while a driver is attached..

IIRC the FLR code in PCI doesn't always ensure that (due to the sysfs paths), so yeah, this looks troubled. iommu_probe_device_lock perhaps would fix it.

No, iommu_probe_device_lock is for protecting access to dev->iommu in the probe path until the device is definitively assigned to a group (or not). Fundamentally it defends against multiple sources triggering a probe of the same device in parallel - once the device *is* probed it is no longer relevant, and the group mutex is the right thing to protect all subsequent operations.

Quoted 6 lines

Yes, adding iommu_probe_device_lock to iommu_deinit_device() would be gross.

but something is required to protect the load of dev->iommu_group.. dev->iommu_group->mutex can't protect itself against a race UAF on deinit.

Then you do iommu_group_get/put() around it as well.

From a quick skim I suspect it's probably OK - at least device_del() gets to bus_remove_device()->device_remove_groups() well enough before the BUS_NOTIFY_REMOVED_DEVICE call that triggers iommu_release_device().

And on an unrelated thought that's just come to mind, if we ever did FLR with PASIDs enabled, presumably that's going to wipe out the PASID configuration,

so will the caller who requested the reset actually expect
the attachments at the IOMMU end to be preserved, or would they assume to start over from scratch? Seems like there's not necessarily one right answer there :/

Quoted 3 lines · Robin Murphy

From a quick skim I suspect it's probably OK - at least device_del() gets to bus_remove_device()->device_remove_groups() well enough before the BUS_NOTIFY_REMOVED_DEVICE call that triggers iommu_release_device().

Make sense, Nicolin, a well placed comment explaining this would be good

Quoted 3 lines

And on an unrelated thought that's just come to mind, if we ever did FLR with PASIDs enabled, presumably that's going to wipe out the PASID configuration,

I've always been expecting the PCI FLR code to preserve the config space that belongs the iommu subsystem. PASID, ATS, PRI, etc. Never looked into it... Nicolin??

Quoted 7 lines

Quoted 3 lines

And on an unrelated thought that's just come to mind, if we ever did FLR with PASIDs enabled, presumably that's going to wipe out the PASID configuration,

I've always been expecting the PCI FLR code to preserve the config space that belongs the iommu subsystem. PASID, ATS, PRI, etc. Never looked into it... Nicolin??

Those are the flags in struct pci_dev right?

unsigned int    ats_enabled:1;          /* Address Translation Svc */
unsigned int    pasid_enabled:1;        /* Process Address Space ID */
unsigned int    pri_enabled:1;          /* Page Request Interface */

And pci_restore_state() does the recovery of these bits.

Per PCIe r6.3, sec 10.3.1 IMPLEMENTATION NOTE, software should disable ATS before initiating a Function Level Reset.

Call iommu_dev_reset_prepare() before FLR and iommu_dev_reset_done() after, in the two FLR Functions. This will dock the device at IOMMU_DOMAIN_BLOCKED during the FLR function, which should allow the IOMMU driver to pause DMA traffic and invode pci_disable_ats() and pci_enable_ats() respectively.

Add a warning if ATS isn't disabled, in which case IOMMU driver should fix itself to disable ATS following the design in iommu_dev_reset_prepare().

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

---
  drivers/pci/pci.c | 42 ++++++++++++++++++++++++++++++++++++++----
  1 file changed, 38 insertions(+), 4 deletions(-)

drivers/pci/pci.c +3 -0

index e9448d55113b..61535435bde1 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>

@@ -4518,13 +4519,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))

Quoted 4 lines · Nicolin Chen

  int pcie_flr(struct pci_dev *dev)
  {

drivers/pci/pci.c +2 -0

+	int ret = 0;
+

drivers/pci/pci.c EXPORT_SYMBOL(pci_wait_for_pending_transaction); · line 4534

Quoted 10 lines

drivers/pci/pci.c +8 -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 Function Level Reset. So notify the iommu driver
+	 * that actually enabled ATS. Have to call it after waiting for pending
+	 * DMA transaction.
+	 */
+	if (iommu_dev_reset_prepare(&dev->dev))
+		pci_err(dev, "failed to stop IOMMU\n");

Need to abort here?

Hi all,

Per PCIe r6.3, sec 10.3.1 IMPLEMENTATION NOTE, software should disable ATS before initiating a Function Level Reset, and then ensure no invalidation requests being issued to a device when its ATS capability is disabled.

Both pci_enable_ats() and pci_disable_ats() are called by an IOMMU driver, but an unsolicited FLR can happen at any time in the PCI layer. This might result in a race between them, breaking the rules given by the PCIe Spec.

Therefore, there needs to be a sync between IOMMU and PCI subsystems, to ensure that ATS will be disabled and never gets re-enabled until the FLR finishes. Add a pair of new IOMMU helpers for PCI reset functions to call before and after the reset routines. These two helpers will temporally attach the device's RID/PASID to IOMMU_DOMAIN_BLOCKED, which should allow its IOMMU driver to pause any DMA traffic and disable ATS feature until the FLR is done.

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

Thanks
Nicolin

Nicolin Chen (2):
iommu: Introduce iommu_dev_reset_prepare() and iommu_dev_reset_done()
pci: Suspend ATS before doing FLR

  include/linux/iommu.h |  12 +++++
  drivers/iommu/iommu.c | 106 ++++++++++++++++++++++++++++++++++++++++++
  drivers/pci/pci.c     |  42 +++++++++++++++--
  3 files changed, 156 insertions(+), 4 deletions(-)

Quoted 5 lines · Nicolin Chen

Hi all,

Per PCIe r6.3, sec 10.3.1 IMPLEMENTATION NOTE, software should disable ATS before initiating a Function Level Reset, and then ensure no invalidation requests being issued to a device when its ATS capability is disabled.

Not really - what it says is that software should not expect to receive invalidate completions from a function which is in the process of being reset or powered off, and if software doesn't want to be confused by that then it should take care to wait for completion or timeout of all outstanding requests, and avoid issuing new requests, before initiating such a reset or power transition.

Quoted 7 lines

Therefore, there needs to be a sync between IOMMU and PCI subsystems, to ensure that ATS will be disabled and never gets re-enabled until the FLR finishes. Add a pair of new IOMMU helpers for PCI reset functions to call before and after the reset routines. These two helpers will temporally attach the device's RID/PASID to IOMMU_DOMAIN_BLOCKED, which should allow its IOMMU driver to pause any DMA traffic and disable ATS feature until the FLR is done.

FLR must inherently stop the function from issuing any kind of requests anyway (see 6.6.2), so "pausing" traffic at the IOMMU end seems like a non-issue.

I guess I can see how messing with the domain attachment underneath the rest of the group manages to prevent new invalidate requests from group->domain being issued to the given function, but it's pretty horrid - leaving the mutex blocked might be just about tolerable for an FLR that's supposed to take no longer than 100ms, but what if we do want to do this for suspend/resume as well?

Quoted 7 lines · Robin Murphy

Quoted 5 lines · Nicolin Chen

Hi all,

Per PCIe r6.3, sec 10.3.1 IMPLEMENTATION NOTE, software should disable ATS before initiating a Function Level Reset, and then ensure no invalidation requests being issued to a device when its ATS capability is disabled.

Quoted 6 lines

Not really - what it says is that software should not expect to receive invalidate completions from a function which is in the process of being reset or powered off, and if software doesn't want to be confused by that then it should take care to wait for completion or timeout of all outstanding requests, and avoid issuing new requests, before initiating such a reset or power transition.

The commit message can be more precise, but I agree with the conclusion that the right direction for Linux is to disable and block ATS, instead of trying to ignore completion time out events, or trying to block page table mutations. Ie do what the implementation note says..

Maybe:

PCIe permits a device to ignore ATS invalidation TLPs while it is processing FLR. This creates a problem visible to the OS where ATS invalidation commands will time out. For instance a SVA domain will have no coordination with a FLR event and can racily issue ATC invalidations into 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 the OS could arrange to ignore the timeout, 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, and we already have iommu driver support to implement something like this. Implement this approach in the iommu core.

Provide a callback from the PCI subsystem that will enclose the FLR and have the iommu core temporarily change all the domain attachments into BLOCKED. When attaching a BLOCKED domain IOMMU drivers should fence any incoming ATS queries, synchronously stop issuing new ATS invalidations, and synchronously wait for all ATS invalidations to complete. This will avoid any ATS invaliation time outs.

IOMMU drivers may also disable ATS in PCI config space, but it is not required to solve the completion timeout problem. The PCI FLR logic will put all the iommu owned config space bits back before completing.

During this period holding the group mutex will not allow new domains to be attached to prevent any new ATS invalidations.

Quoted 6 lines

I guess I can see how messing with the domain attachment underneath the rest of the group manages to prevent new invalidate requests from group->domain being issued to the given function, but it's pretty horrid - leaving the mutex blocked might be just about tolerable for an FLR that's supposed to take no longer than 100ms, but what if we do want to do this for suspend/resume as well?

I don't view this a problem for FLR, we can hold a mutex for a long time. It principally delays domain changes which are kind of nonsense to be doing concurrently with FLR in the first place..

However, for suspend, we probably want to leave a marker in the group

that the group is force-blocked and all domain attach/detach logic will only update the group tracking structures and not call into the iommu driver. When the resume happens the core will set the current group domain list to the iommu driver. No need for a long lived lock this way.

Quoted 5 lines

I don't view this a problem for FLR, we can hold a mutex for a long time. It principally delays domain changes which are kind of nonsense to be doing concurrently with FLR in the first place..

However, for suspend, we probably want to leave a marker in the group

IIUIC, the thing for suspend/resume is that it would result in a long hold of the mutex, which can be a problem?

Quoted 5 lines

that the group is force-blocked and all domain attach/detach logic will only update the group tracking structures and not call into the iommu driver. When the resume happens the core will set the current group domain list to the iommu driver. No need for a long lived lock this way.

Yea, what we don't want is driver re-enabling ATS. So, bypassing it at the core level should work. Then, iommu_dev_reset_prepare and iommu_dev_reset_done will only mutex the flag.

Quoted 7 lines · Robin Murphy

Quoted 5 lines · Nicolin Chen

Hi all,

Per PCIe r6.3, sec 10.3.1 IMPLEMENTATION NOTE, software should disable ATS before initiating a Function Level Reset, and then ensure no invalidation requests being issued to a device when its ATS capability is disabled.

Quoted 6 lines

Not really - what it says is that software should not expect to receive invalidate completions from a function which is in the process of being reset or powered off, and if software doesn't want to be confused by that then it should take care to wait for completion or timeout of all outstanding requests, and avoid issuing new requests, before initiating such a reset or power transition.

The commit message can be more precise, but I agree with the conclusion that the right direction for Linux is to disable and block ATS, instead of trying to ignore completion time out events, or trying to block page table mutations. Ie do what the implementation note says..

Maybe:

PCIe permits a device to ignore ATS invalidation TLPs while it is processing FLR. This creates a problem visible to the OS where ATS invalidation commands will time out. For instance a SVA domain will have no coordination with a FLR event and can racily issue ATC invalidations into a resetting device.

Quoted 21 lines · Jason Gunthorpe

Quoted 7 lines · Robin Murphy

Quoted 5 lines · Nicolin Chen

Hi all,

Per PCIe r6.3, sec 10.3.1 IMPLEMENTATION NOTE, software should disable ATS before initiating a Function Level Reset, and then ensure no invalidation requests being issued to a device when its ATS capability is disabled.

Quoted 6 lines

Not really - what it says is that software should not expect to receive invalidate completions from a function which is in the process of being reset or powered off, and if software doesn't want to be confused by that then it should take care to wait for completion or timeout of all outstanding requests, and avoid issuing new requests, before initiating such a reset or power transition.

The commit message can be more precise, but I agree with the conclusion that the right direction for Linux is to disable and block ATS, instead of trying to ignore completion time out events, or trying to block page table mutations. Ie do what the implementation note says..

Quoted 7 lines

Maybe:

PCIe permits a device to ignore ATS invalidation TLPs while it is processing FLR. This creates a problem visible to the OS where ATS invalidation commands will time out. For instance a SVA domain will have no coordination with a FLR event and can racily issue ATC invalidations into a resetting device.

The sec 10.3.1 implementation note mentions FLR specifically, but it seems like *any* kind of reset would be vulnerable, e.g., SBR, external PERST# assert, etc?

Quoted 21 lines · Jason Gunthorpe

Quoted 7 lines · Robin Murphy

Quoted 5 lines · Nicolin Chen

Hi all,

Per PCIe r6.3, sec 10.3.1 IMPLEMENTATION NOTE, software should disable ATS before initiating a Function Level Reset, and then ensure no invalidation requests being issued to a device when its ATS capability is disabled.

Quoted 6 lines

Not really - what it says is that software should not expect to receive invalidate completions from a function which is in the process of being reset or powered off, and if software doesn't want to be confused by that then it should take care to wait for completion or timeout of all outstanding requests, and avoid issuing new requests, before initiating such a reset or power transition.

The commit message can be more precise, but I agree with the conclusion that the right direction for Linux is to disable and block ATS, instead of trying to ignore completion time out events, or trying to block page table mutations. Ie do what the implementation note says..

Quoted 7 lines

Maybe:

PCIe permits a device to ignore ATS invalidation TLPs while it is processing FLR. This creates a problem visible to the OS where ATS invalidation commands will time out. For instance a SVA domain will have no coordination with a FLR event and can racily issue ATC invalidations into a resetting device.

The sec 10.3.1 implementation note mentions FLR specifically, but it seems like *any* kind of reset would be vulnerable, e.g., SBR, external PERST# assert, etc?