iommu/arm-smmu-v3: Reset PCI device upon ATC invalidate timeout

Currently, when GERROR_CMDQ_ERR occurs, the arm_smmu_cmdq_skip_err() won't do anything for the CMDQ_ERR_CERROR_ATC_INV_IDX.

When a device wasn't responsive to an ATC invalidation request, this often results in constant CMDQ errors:
unexpected global error reported (0x00000001), this could be serious CMDQ error (cons 0x0302bb84): ATC invalidate timeout unexpected global error reported (0x00000001), this could be serious CMDQ error (cons 0x0302bb88): ATC invalidate timeout unexpected global error reported (0x00000001), this could be serious CMDQ error (cons 0x0302bb8c): ATC invalidate timeout
...

An ATC invalidation timeout indicates that the device failed to respond to a protocol-critical coherency request, which means that device's internal ATS state is desynchronized from the SMMU.

Furthermore, ignoring the timeout leaves the system in an unsafe state, as the device cache may retain stale ATC entries for memory pages that the OS has already reclaimed and reassigned. This might lead to data corruption.

The only safe recovery action is to issue a PCI reset, which guarantees to flush all internal device caches and recover the device.

Read the ATC_INV command that led to the timeouts, and schedule a recovery worker to reset the device corresponding to the Stream ID. If reset fails, keep the device in the resetting/blocking domain to avoid data corruption.

Though it'd be ideal to block it immediately in the ISR, it cannot be done because an STE update would require another CFIG_STE command that couldn't finish in the context of an ISR handling a CMDQ error.

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

---
  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |   5 +
  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 131 +++++++++++++++++++-
  2 files changed, 132 insertions(+), 4 deletions(-)

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +5 -0

index 3c6d65d36164f..8789cf8294504 100644
@@ -803,6 +803,11 @@ struct arm_smmu_device {
 
  	struct rb_root			streams;
  	struct mutex			streams_mutex;
+
+	struct {
+		struct list_head	list;
+		spinlock_t		lock; /* Lock the list */
+	} atc_recovery;
  };
 
  struct arm_smmu_stream {

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +74 -0

index 4d00d796f0783..de182c27c77c4 100644
@@ -106,6 +106,8 @@ static const char * const event_class_str[] = {
  	[3] = "Reserved",
  };
 
+static struct arm_smmu_master *
+arm_smmu_find_master(struct arm_smmu_device *smmu, u32 sid);
  static int arm_smmu_alloc_cd_tables(struct arm_smmu_master *master);
 
  static void parse_driver_options(struct arm_smmu_device *smmu)

@@ -174,6 +176,13 @@ static void queue_inc_cons(struct arm_smmu_ll_queue *q)
  	q->cons = Q_OVF(q->cons) | Q_WRP(q, cons) | Q_IDX(q, cons);
  }
 
+static u32 queue_prev_cons(struct arm_smmu_ll_queue *q, u32 cons)
+{
+	u32 idx_wrp = (Q_WRP(q, cons) | Q_IDX(q, cons)) - 1;
+
+	return Q_OVF(cons) | Q_WRP(q, idx_wrp) | Q_IDX(q, idx_wrp);
+}
+
  static void queue_sync_cons_ovf(struct arm_smmu_queue *q)
  {
  	struct arm_smmu_ll_queue *llq = &q->llq;

@@ -410,6 +419,97 @@ static void arm_smmu_cmdq_build_sync_cmd(u64 *cmd, struct arm_smmu_device *smmu,
  		u64p_replace_bits(cmd, CMDQ_SYNC_0_CS_NONE, CMDQ_SYNC_0_CS);
  }
 
+/* ATC recovery upon ATC invalidation timeout */
+struct arm_smmu_atc_recovery_param {
+	struct arm_smmu_device *smmu;
+	struct pci_dev *pdev;
+	u32 sid;
+
+	struct work_struct work;
+	struct list_head node;
+};
+
+static void arm_smmu_atc_recovery_worker(struct work_struct *work)
+{
+	struct arm_smmu_atc_recovery_param *param =
+		container_of(work, struct arm_smmu_atc_recovery_param, work);
+	struct pci_dev *pdev;
+
+	scoped_guard(mutex, &param->smmu->streams_mutex) {
+		struct arm_smmu_master *master;
+
+		master = arm_smmu_find_master(param->smmu, param->sid);
+		if (!master || WARN_ON(!dev_is_pci(master->dev)))
+			goto free_param;
+		pdev = to_pci_dev(master->dev);
+		pci_dev_get(pdev);
+	}
+
+	scoped_guard(spinlock_irqsave, &param->smmu->atc_recovery.lock) {
+		struct arm_smmu_atc_recovery_param *e;
+
+		list_for_each_entry(e, &param->smmu->atc_recovery.list, node) {
+			/* Device is already being recovered */
+			if (e->pdev == pdev)
+				goto put_pdev;
+		}
+		param->pdev = pdev;
+		list_add(&param->node, &param->smmu->atc_recovery.list);
+	}
+
+	/*
+	 * Stop DMA (PCI) and block ATS (IOMMU) immediately, to prevent memory
+	 * corruption. This must take pci_dev_lock to prevent any racy unplug.
+	 *
+	 * If pci_dev_reset_iommu_prepare() fails, pci_reset_function will call
+	 * it again internally.
+	 */
+	pci_dev_lock(pdev);
+	pci_clear_master(pdev);
+	if (pci_dev_reset_iommu_prepare(pdev))
+		pci_err(pdev, "failed to block ATS!\n");
+	pci_dev_unlock(pdev);
+
+	/*
+	 * ATC timeout indicates the device has stopped responding to coherence
+	 * protocol requests. The only safe recovery is a reset to flush stale
+	 * cached translations. Note that pci_reset_function() internally calls
+	 * pci_dev_reset_iommu_prepare/done() as well and ensures to block ATS
+	 * if PCI-level reset fails.
+	 */
+	if (!pci_reset_function(pdev)) {
+		/*
+		 * If reset succeeds, set BME back. Otherwise, fence the system
+		 * from a faulty device, in which case user will have to replug
+		 * the device to invoke pci_set_master().
+		 */
+		pci_dev_lock(pdev);

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +6 -4

+	return 0;
+}
+
  void __arm_smmu_cmdq_skip_err(struct arm_smmu_device *smmu,
  			      struct arm_smmu_cmdq *cmdq)
  {

@@ -441,11 +541,10 @@ void __arm_smmu_cmdq_skip_err(struct arm_smmu_device *smmu,
  	case CMDQ_ERR_CERROR_ATC_INV_IDX:
  		/*
  		 * ATC Invalidation Completion timeout. CONS is still pointing
-		 * at the CMD_SYNC. Attempt to complete other pending commands
-		 * by repeating the CMD_SYNC, though we might well end up back
-		 * here since the ATC invalidation may still be pending.
+		 * at the CMD_SYNC. Rewind it to read the ATC_INV command.
  		 */
-		return;
+		cons = queue_prev_cons(&q->llq, cons);
+		fallthrough;
  	case CMDQ_ERR_CERROR_ILL_IDX:
  	default:
  		break;

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +17 -0

+		 */
+		WARN_ON(FIELD_GET(CMDQ_0_OP, cmd[0]) != CMDQ_OP_ATC_INV);
+
+		/*
+		 * If we failed to schedule a recovery worker, we would well end
+		 * up back here since the ATC invalidation may still be pending.
+		 * This gives us another chance to reschedule a recovery worker.
+		 */
+		arm_smmu_sched_atc_recovery(smmu,
+					    FIELD_GET(CMDQ_ATC_0_SID, cmd[0]));
+		return;
+	}
+
+	/* idx == CMDQ_ERR_CERROR_ILL_IDX */
  	dev_err(smmu->dev, "skipping command in error state:\n");
  	for (i = 0; i < ARRAY_SIZE(cmd); ++i)
  		dev_err(smmu->dev, "\t0x%016llx\n", (unsigned long long)cmd[i]);

@@ -3942,6 +4062,9 @@ static int arm_smmu_init_structures(struct arm_smmu_device *smmu)
  {
  	int ret;
 
+	INIT_LIST_HEAD(&smmu->atc_recovery.list);
+	spin_lock_init(&smmu->atc_recovery.lock);
+
  	mutex_init(&smmu->streams_mutex);
  	smmu->streams = RB_ROOT;
 

Quoted 3 lines · Nicolin Chen

Though it'd be ideal to block it immediately in the ISR, it cannot be done because an STE update would require another CFIG_STE command that couldn't finish in the context of an ISR handling a CMDQ error.

Why not? As soon as we've acked GERRORN.CMDQ_ERR, command consumption will resume and we're free to do whatever we fancy. Admittedly this probably represents more work than we *want* to be doing in the SMMU's IRQ handler (arguably even in a thread, since all the PCI housekeeping isn't really the SMMU driver's own problem), but I would say the workqueue is a definite design choice, not a functional requirement.

Quoted 6 lines

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +6 -0

+static void arm_smmu_atc_recovery_worker(struct work_struct *work)
+{
+	struct arm_smmu_atc_recovery_param *param =
+		container_of(work, struct arm_smmu_atc_recovery_param, work);
+	struct pci_dev *pdev;
+

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static void arm_smmu_cmdq_build_sync_cmd(u64 *cmd, struct arm_smmu_device *smmu, · line 441

Quoted 4 lines

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +4 -0

+	scoped_guard(mutex, &param->smmu->streams_mutex) {
+		struct arm_smmu_master *master;
+
+		master = arm_smmu_find_master(param->smmu, param->sid);

The only thing SMMU-specific about this seems to be the use of arm_smmu_find_master() to resolve the device, which could just as well be done upon submission anyway - why isn't this a generic IOMMU/IOPF mechanism?

Quoted 11 lines

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +11 -0

+static int arm_smmu_sched_atc_recovery(struct arm_smmu_device *smmu, u32 sid)
+{
+	struct arm_smmu_atc_recovery_param *param;
+
+	param = kzalloc_obj(*param, GFP_ATOMIC);
+	if (!param)
+		return -ENOMEM;
+	param->smmu = smmu;
+	param->sid = sid;
+
+	INIT_WORK(&param->work, arm_smmu_atc_recovery_worker);

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static void arm_smmu_cmdq_build_sync_cmd(u64 *cmd, struct arm_smmu_device *smmu, · line 509

Quoted 1 line

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +1 -0

+	queue_work(system_unbound_wq, &param->work);

Might it make more sense to have a single work item associated with the list_head and use the latter as an actual queue, such that the work runs until the list is empty, then here at submisison time we do the list_add() and schedule_work()? That could perhaps even be a global queue, since ATS timeouts can hardly be expected to be a highly-contended high-perfoamnce concern.

Right now it seems the list is barely doing anything - a "deduplication" mechanism that only works if multiple resets for the same device happen to have their work scheduled concurrently seems pretty ineffective.

Quoted 7 lines

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +3 -0

@@ -456,6 +555,27 @@ void __arm_smmu_cmdq_skip_err(struct arm_smmu_device *smmu,
  	 * not to touch any of the shadow cmdq state.
  	 */
  	queue_read(cmd, Q_ENT(q, cons), q->ent_dwords);
+
+	if (idx == CMDQ_ERR_CERROR_ATC_INV_IDX) {
+		/*

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c void __arm_smmu_cmdq_skip_err(struct arm_smmu_device *smmu, · line 564

Quoted 4 lines

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +4 -0

+		 * Since commands can be issued in batch making it difficult to
+		 * identify which CMDQ_OP_ATC_INV actually timed out, the driver
+		 * must ensure only CMDQ_OP_ATC_INV commands for the same device
+		 * can be batched.

But this *is* "the driver" - arm_smmu_atc_inv_domain() is literally further down this same C file, and does not do what this comment is saying it must do, so how are you expecting this to work correctly?

Quoted 11 lines · Robin Murphy

Quoted 3 lines · Nicolin Chen

Though it'd be ideal to block it immediately in the ISR, it cannot be done because an STE update would require another CFIG_STE command that couldn't finish in the context of an ISR handling a CMDQ error.

Why not? As soon as we've acked GERRORN.CMDQ_ERR, command consumption will resume and we're free to do whatever we fancy. Admittedly this probably represents more work than we *want* to be doing in the SMMU's IRQ handler (arguably even in a thread, since all the PCI housekeeping isn't really the SMMU driver's own problem), but I would say the workqueue is a definite design choice, not a functional requirement.

Hmm, you are right, after writel(gerror, ARM_SMMU_GERRORN), it would be doable.

Though SID->pdev conversion requires streams_mutex, which can't happen in ISR?

Quoted 6 lines

Quoted 6 lines

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +6 -0

+static void arm_smmu_atc_recovery_worker(struct work_struct *work)
+{
+	struct arm_smmu_atc_recovery_param *param =
+		container_of(work, struct arm_smmu_atc_recovery_param, work);
+	struct pci_dev *pdev;
+

Quoted 8 lines

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static void arm_smmu_cmdq_build_sync_cmd(u64 *cmd, struct arm_smmu_device *smmu, · line 441

Quoted 4 lines

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +4 -0

+	scoped_guard(mutex, &param->smmu->streams_mutex) {
+		struct arm_smmu_master *master;
+
+		master = arm_smmu_find_master(param->smmu, param->sid);

The only thing SMMU-specific about this seems to be the use of arm_smmu_find_master() to resolve the device, which could just as well be done upon submission anyway - why isn't this a generic IOMMU/IOPF mechanism?

You mean treating this as a page fault? That's an interesting idea.

So, the IOMMU-level fence could be done in arm_smmu_page_response().

Quoted 11 lines

Quoted 11 lines

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +11 -0

+static int arm_smmu_sched_atc_recovery(struct arm_smmu_device *smmu, u32 sid)
+{
+	struct arm_smmu_atc_recovery_param *param;
+
+	param = kzalloc_obj(*param, GFP_ATOMIC);
+	if (!param)
+		return -ENOMEM;
+	param->smmu = smmu;
+	param->sid = sid;
+
+	INIT_WORK(&param->work, arm_smmu_atc_recovery_worker);

Quoted 8 lines

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static void arm_smmu_cmdq_build_sync_cmd(u64 *cmd, struct arm_smmu_device *smmu, · line 509

Quoted 1 line

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +1 -0

+	queue_work(system_unbound_wq, &param->work);

Might it make more sense to have a single work item associated with the list_head and use the latter as an actual queue, such that the work runs until the list is empty, then here at submisison time we do the list_add() and schedule_work()? That could perhaps even be a global queue, since ATS timeouts can hardly be expected to be a highly-contended high-perfoamnce concern.

That sounds like the IOPF implementation. Maybe inventing another IOMMU_FAULT_ATC_TIMEOUT to reuse the existing infrastructure would make things cleaner.

Quoted 3 lines

Right now it seems the list is barely doing anything - a "deduplication" mechanism that only works if multiple resets for the same device happen to have their work scheduled concurrently seems pretty ineffective.

From my testing results, it does effectively block duplications, so I think it's somehow meaningful.

If I wasn't wrong, the duplicated ATC timeout might come from this exact reset thread, as it did a CMDQ_OP_CFGI_STE while the CONS was still pointing to previous CMDQ_OP_ATC_INV.

IIUIC, the IOPF queue doesn't expect duplicated PRI events. So, it might invoke the handler twice if there is a duplication. The core handling list_add() might need a deduplication.

Quoted 7 lines

Quoted 7 lines

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +3 -0

@@ -456,6 +555,27 @@ void __arm_smmu_cmdq_skip_err(struct arm_smmu_device *smmu,
  	 * not to touch any of the shadow cmdq state.
  	 */
  	queue_read(cmd, Q_ENT(q, cons), q->ent_dwords);
+
+	if (idx == CMDQ_ERR_CERROR_ATC_INV_IDX) {
+		/*

Quoted 8 lines

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c void __arm_smmu_cmdq_skip_err(struct arm_smmu_device *smmu, · line 564

Quoted 4 lines

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +4 -0

+		 * Since commands can be issued in batch making it difficult to
+		 * identify which CMDQ_OP_ATC_INV actually timed out, the driver
+		 * must ensure only CMDQ_OP_ATC_INV commands for the same device
+		 * can be batched.

But this *is* "the driver" - arm_smmu_atc_inv_domain() is literally further down this same C file, and does not do what this comment is saying it must do, so how are you expecting this to work correctly?

Oh, that's a big miss..

I imaged these changes to be based on the arm_smmu_invs series, where it could break the ATC in the batching function. Here, it should have made some change to arm_smmu_atc_inv_domain().

Thanks
Nicolin

That sounds like the IOPF implementation. Maybe inventing another IOMMU_FAULT_ATC_TIMEOUT to reuse the existing infrastructure would make things cleaner.

Quoted 3 lines · Nicolin Chen

That sounds like the IOPF implementation. Maybe inventing another IOMMU_FAULT_ATC_TIMEOUT to reuse the existing infrastructure would make things cleaner.

I think the routing is quite different, IOPF wants to route an event the domain creator, here you want to route an event to the IOMMU core then the PCIe RAS callbacks.

IDK if there is much to be reused there, especially since IOPF requires a memory allocation and ideally we should not be allocating memory to resolve this critical error condition.

Quoted 5 lines · Jason Gunthorpe

Quoted 3 lines · Nicolin Chen

That sounds like the IOPF implementation. Maybe inventing another IOMMU_FAULT_ATC_TIMEOUT to reuse the existing infrastructure would make things cleaner.

Quoted 7 lines

I think the routing is quite different, IOPF wants to route an event the domain creator, here you want to route an event to the IOMMU core then the PCIe RAS callbacks.

IDK if there is much to be reused there, especially since IOPF requires a memory allocation and ideally we should not be allocating memory to resolve this critical error condition.

Yeah, sorry, for a moment there I somehow forgot that we can expect to use ATS without PRI, so indeed tying this to IOPF wouldn't be appropriate. And given the general difficulty of trying to infer what went wrong and what to do from the CMDQ contents alone, I do like your idea of trying to return a new kind of sync failure back to arm_smmu_atc_inv_{master,domain}() so that we can take any defensive action from there, with all the information to hand. We'd just have to ensure that if a large set of ATCI commands needs to span multiple batches, every batch must contain its own sync (since if some other batch of unrelated commands could get interleaved in the middle and issue a sync that then fails due to someone else's ATC timeout, everything's likely to get confused and go wrong).

The fiddly thing then is that we might also have to be prepared to "handle" CMD_SYNC timeout by manually checking for GERRORs, in case the whole invalidation is in the context of an dma_unmap within some other device's IRQ handler, which happens to be on the same CPU where the GERROR IRQ is now pending, but can't be taken until we can complete the inv and return out of the current IRQ :/

Quoted 14 lines · Robin Murphy

Quoted 5 lines · Jason Gunthorpe

Quoted 3 lines · Nicolin Chen

That sounds like the IOPF implementation. Maybe inventing another IOMMU_FAULT_ATC_TIMEOUT to reuse the existing infrastructure would make things cleaner.

Quoted 7 lines

I think the routing is quite different, IOPF wants to route an event the domain creator, here you want to route an event to the IOMMU core then the PCIe RAS callbacks.

IDK if there is much to be reused there, especially since IOPF requires a memory allocation and ideally we should not be allocating memory to resolve this critical error condition.

Quoted 11 lines

Yeah, sorry, for a moment there I somehow forgot that we can expect to use ATS without PRI, so indeed tying this to IOPF wouldn't be appropriate. And given the general difficulty of trying to infer what went wrong and what to do from the CMDQ contents alone, I do like your idea of trying to return a new kind of sync failure back to arm_smmu_atc_inv_{master,domain}() so that we can take any defensive action from there, with all the information to hand. We'd just have to ensure that if a large set of ATCI commands needs to span multiple batches, every batch must contain its own sync (since if some other batch of unrelated commands could get interleaved in the middle and issue a sync that then fails due to someone else's ATC timeout, everything's likely to get confused and go wrong).

Yeah, that all makes sense to me.

The batching issue is scary, we definately can't allow an ATC invalidation to be pushed without a SYNC that localizes any failure to this specific thread, or we can't properly disambiguate the failures anymore.

My feeling is when the sync "fails", it can bubble up the error and we can get back to the invalidation list processor which can then see it failed to process an ATC batch and take an appropriate action.

Quoted 6 lines

The fiddly thing then is that we might also have to be prepared to "handle" CMD_SYNC timeout by manually checking for GERRORs, in case the whole invalidation is in the context of an dma_unmap within some other device's IRQ handler, which happens to be on the same CPU where the GERROR IRQ is now pending, but can't be taken until we can complete the inv and return out of the current IRQ :/

IIRC didn't the PM patches propose to add this anyhow?

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +7 -0

+	/*
+	 * ATC timeout indicates the device has stopped responding to coherence
+	 * protocol requests. The only safe recovery is a reset to flush stale
+	 * cached translations. Note that pci_reset_function() internally calls
+	 * pci_dev_reset_iommu_prepare/done() as well and ensures to block ATS
+	 * if PCI-level reset fails.
+	 */

Quoted 7 lines · Nicolin Chen

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +7 -0

+	/*
+	 * ATC timeout indicates the device has stopped responding to coherence
+	 * protocol requests. The only safe recovery is a reset to flush stale
+	 * cached translations. Note that pci_reset_function() internally calls
+	 * pci_dev_reset_iommu_prepare/done() as well and ensures to block ATS
+	 * if PCI-level reset fails.
+	 */

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static void arm_smmu_cmdq_build_sync_cmd(u64 *cmd, struct arm_smmu_device *smmu, · line 489

Quoted 10 lines

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +10 -0

+	if (!pci_reset_function(pdev)) {
+		/*
+		 * If reset succeeds, set BME back. Otherwise, fence the system
+		 * from a faulty device, in which case user will have to replug
+		 * the device to invoke pci_set_master().
+		 */
+		pci_dev_lock(pdev);
+		pci_set_master(pdev);
+		pci_dev_unlock(pdev);
+	}

I thought we talked about this, the iommu driver cannot just blindly issue a reset like this, the reset has to come from the actual device driver through the AERish mechanism. Otherwise the driver RAS is going to explode.

The smmu driver should immediately block the STE (reject translated requests) to protect the system before resuming whatever command submissio n has encountered the error.

You could delegate the STE change to the interrupted command submission to avoid doing it from a ISR, that makes alot of sense because the submission thread is already operating a cmdq so it could stick in a STE invalidation command, possibly even in place of the failed ATC command.

I think I'd break this up into smaller steps, just focus on this STE mechanism at start and have any future attach callback fix the STE.

Then we can talk about how to properly trigger the PCI RAS flow and so on.

You mean in arm_smmu_cmdq_issue_cmdlist() that issued the timed out ATC command?

So my test case was to trigger a device fault followed by an ATC command. But, I found that the ATC command submission returned 0 while only the ISR received:
CMDQ error (cons 0x03000003): ATC invalidate timeout arm_smmu_debugfs_atc_write: ATC_INV ret=0

It seems difficult to insert a CMDQ_OP_CFGI_STE in the submission thread?

Quoted 2 lines · Nicolin Chen

You mean in arm_smmu_cmdq_issue_cmdlist() that issued the timed out ATC command?

Yes, it was my off hand thought.

Quoted 6 lines

So my test case was to trigger a device fault followed by an ATC command. But, I found that the ATC command submission returned 0 while only the ISR received:
CMDQ error (cons 0x03000003): ATC invalidate timeout arm_smmu_debugfs_atc_write: ATC_INV ret=0

Quoted 2 lines

It seems difficult to insert a CMDQ_OP_CFGI_STE in the submission thread?

I didn't look, but I thought the CMDQ stops on the ATC invalidation, flags the error and the ISR NOP's the failing CMDQ entry and restarts it to resume the thread? Is that something else?

If so you could insert the STE flush instead of a NOP

Otherwise the arm_smmu_cmdq_issue_cmdlist() can just push another CMD to the queue and sync, it is obviously in a context that can do that.

But arm_smmu_cmdq_issue_cmdlist() doesn't know when to push another CMD. In my case where ATC_INV irq occurs, the return value from the arm_smmu_cmdq_poll_until_sync() in the Step 5 is 0, and prods/cons are also matched. Actually, at this point that NOP ISR has already finished.

Quoted 5 lines · Nicolin Chen

But arm_smmu_cmdq_issue_cmdlist() doesn't know when to push another CMD. In my case where ATC_INV irq occurs, the return value from the arm_smmu_cmdq_poll_until_sync() in the Step 5 is 0, and prods/cons are also matched. Actually, at this point that NOP ISR has already finished.

Yes, you'd need a sneaky way to convay the error from the ISR to the cmdlist code that didn't harm performance. Maybe we could come up with something, but if it works replacing the NOP with flush sounds fairly appealing - though can you do a single WORD edit to the STE that will block translated requests? Zero EATS?

Also, will the SMMU start spamming with blocked translation events or something that will need suppression too?

Quoted 8 lines · Jason Gunthorpe

Quoted 5 lines · Nicolin Chen

But arm_smmu_cmdq_issue_cmdlist() doesn't know when to push another CMD. In my case where ATC_INV irq occurs, the return value from the arm_smmu_cmdq_poll_until_sync() in the Step 5 is 0, and prods/cons are also matched. Actually, at this point that NOP ISR has already finished.

Quoted 5 lines

Yes, you'd need a sneaky way to convay the error from the ISR to the cmdlist code that didn't harm performance. Maybe we could come up with something, but if it works replacing the NOP with flush sounds fairly appealing - though can you do a single WORD edit to the STE that will block translated requests? Zero EATS?

Yea. I can give that a try.

Quoted 2 lines

Also, will the SMMU start spamming with blocked translation events or something that will need suppression too?

CD.R=0 can suppress fault records, but we would need to override that in every CD of the device.

Quoted 15 lines · Nicolin Chen

Quoted 13 lines · Jason Gunthorpe

Quoted 5 lines · Nicolin Chen

But arm_smmu_cmdq_issue_cmdlist() doesn't know when to push another CMD. In my case where ATC_INV irq occurs, the return value from the arm_smmu_cmdq_poll_until_sync() in the Step 5 is 0, and prods/cons are also matched. Actually, at this point that NOP ISR has already finished.

Yes, you'd need a sneaky way to convay the error from the ISR to the cmdlist code that didn't harm performance. Maybe we could come up with something, but if it works replacing the NOP with flush sounds fairly appealing - though can you do a single WORD edit to the STE that will block translated requests? Zero EATS?

Quoted 1 line

Yea. I can give that a try.

This also really needs to go after the invalidation changes because it is feasible to also edit the lockless RCU invalidation list from the ISR and disable the ATC for the failed device too.

In general, the maximum users count of an INV_TYPE_ATS would be 1. So, an unref() would be sufficient to mute it, though it'd require the unref() API to support a mismatched users counter, because the PCI reset in the WQ would block ATS, which would try to unref the removed command once again.

Quoted 5 lines · Nicolin Chen

In general, the maximum users count of an INV_TYPE_ATS would be 1. So, an unref() would be sufficient to mute it, though it'd require the unref() API to support a mismatched users counter, because the PCI reset in the WQ would block ATS, which would try to unref the removed command once again.

INV_TYPE_ATS_DISABLED could be made to work too..

Quoted 9 lines · Jason Gunthorpe

Quoted 5 lines · Nicolin Chen

In general, the maximum users count of an INV_TYPE_ATS would be 1. So, an unref() would be sufficient to mute it, though it'd require the unref() API to support a mismatched users counter, because the PCI reset in the WQ would block ATS, which would try to unref the removed command once again.

INV_TYPE_ATS_DISABLED could be made to work too..

Overriding the type.. hmm, that's brilliant.

Rechecking the unref API, I realized that even the "users" in the latest version isn't atomic anymore. I recall that we are fine to do READ_ONCE/WRITE_ONCE on an int type because invalidation array is serialized by the asid mutex, which wouldn't work either in an ISR or just in the arm_smmu_master/domain_atc_inv()?

Quoted 9 lines · Nicolin Chen

Quoted 8 lines · Jason Gunthorpe

Quoted 5 lines · Nicolin Chen

But arm_smmu_cmdq_issue_cmdlist() doesn't know when to push another CMD. In my case where ATC_INV irq occurs, the return value from the arm_smmu_cmdq_poll_until_sync() in the Step 5 is 0, and prods/cons are also matched. Actually, at this point that NOP ISR has already finished.

Quoted 7 lines

Quoted 5 lines

Yes, you'd need a sneaky way to convay the error from the ISR to the cmdlist code that didn't harm performance. Maybe we could come up with something, but if it works replacing the NOP with flush sounds fairly appealing - though can you do a single WORD edit to the STE that will block translated requests? Zero EATS?

Yea. I can give that a try.

This also really needs to go after the invalidation changes because it is feasible to also edit the lockless RCU invalidation list from the ISR and disable the ATC for the failed device too.

Quoted 5 lines

Quoted 2 lines

Also, will the SMMU start spamming with blocked translation events or something that will need suppression too?

CD.R=0 can suppress fault records, but we would need to override that in every CD of the device.

That's too much to do from ISR, but maybe we can do it from a WQ..

Jason

Quoted 28 lines · Jason Gunthorpe

Quoted 9 lines · Nicolin Chen

Quoted 8 lines · Jason Gunthorpe

Quoted 5 lines · Nicolin Chen

But arm_smmu_cmdq_issue_cmdlist() doesn't know when to push another CMD. In my case where ATC_INV irq occurs, the return value from the arm_smmu_cmdq_poll_until_sync() in the Step 5 is 0, and prods/cons are also matched. Actually, at this point that NOP ISR has already finished.

Quoted 7 lines

Quoted 5 lines

Yes, you'd need a sneaky way to convay the error from the ISR to the cmdlist code that didn't harm performance. Maybe we could come up with something, but if it works replacing the NOP with flush sounds fairly appealing - though can you do a single WORD edit to the STE that will block translated requests? Zero EATS?

Yea. I can give that a try.

This also really needs to go after the invalidation changes because it is feasible to also edit the lockless RCU invalidation list from the ISR and disable the ATC for the failed device too.

Quoted 5 lines

Quoted 2 lines

Also, will the SMMU start spamming with blocked translation events or something that will need suppression too?

CD.R=0 can suppress fault records, but we would need to override that in every CD of the device.

Quoted 2 lines

That's too much to do from ISR, but maybe we can do it from a WQ..

(Skimming through these, apologies if I'm losing context), shouldn't we do all that (marking it as an inv STE / abort STE, suppressing the faults) in the worker instead of trying to reset/recover the device?

Quoted 32 lines · Pranjal Shrivastava

Quoted 30 lines · Jason Gunthorpe

Quoted 9 lines · Nicolin Chen

Quoted 8 lines · Jason Gunthorpe

Quoted 5 lines · Nicolin Chen

But arm_smmu_cmdq_issue_cmdlist() doesn't know when to push another CMD. In my case where ATC_INV irq occurs, the return value from the arm_smmu_cmdq_poll_until_sync() in the Step 5 is 0, and prods/cons are also matched. Actually, at this point that NOP ISR has already finished.

Quoted 7 lines

Quoted 5 lines

Yes, you'd need a sneaky way to convay the error from the ISR to the cmdlist code that didn't harm performance. Maybe we could come up with something, but if it works replacing the NOP with flush sounds fairly appealing - though can you do a single WORD edit to the STE that will block translated requests? Zero EATS?

Yea. I can give that a try.

This also really needs to go after the invalidation changes because it is feasible to also edit the lockless RCU invalidation list from the ISR and disable the ATC for the failed device too.

Quoted 5 lines

Quoted 2 lines

Also, will the SMMU start spamming with blocked translation events or something that will need suppression too?

CD.R=0 can suppress fault records, but we would need to override that in every CD of the device.

That's too much to do from ISR, but maybe we can do it from a WQ..

Quoted 3 lines

(Skimming through these, apologies if I'm losing context), shouldn't we do all that (marking it as an inv STE / abort STE, suppressing the faults) in the worker instead of trying to reset/recover the device?

EATS should be unset asap to avoid memory corruption. It's best to do in the unmap() context where the page isn't reclaimed yet by the kernel.

Worker thread will be a bit late, but it is good enough for any further step.

Quoted 2 lines · Nicolin Chen

You mean in arm_smmu_cmdq_issue_cmdlist() that issued the timed out ATC command?

Yes, it was my off hand thought.

Quoted 6 lines

So my test case was to trigger a device fault followed by an ATC command. But, I found that the ATC command submission returned 0 while only the ISR received:
CMDQ error (cons 0x03000003): ATC invalidate timeout arm_smmu_debugfs_atc_write: ATC_INV ret=0

Quoted 2 lines

It seems difficult to insert a CMDQ_OP_CFGI_STE in the submission thread?

I didn't look, but I thought the CMDQ stops on the ATC invalidation, flags the error and the ISR NOP's the failing CMDQ entry and restarts it to resume the thread? Is that something else?

If so you could insert the STE flush instead of a NOP

Otherwise the arm_smmu_cmdq_issue_cmdlist() can just push another CMD to the queue and sync, it is obviously in a context that can do that.

Jason

Quoted 20 lines · Jason Gunthorpe

Quoted 2 lines · Nicolin Chen

You mean in arm_smmu_cmdq_issue_cmdlist() that issued the timed out ATC command?

Yes, it was my off hand thought.

Quoted 8 lines

So my test case was to trigger a device fault followed by an ATC command. But, I found that the ATC command submission returned 0 while only the ISR received:
CMDQ error (cons 0x03000003): ATC invalidate timeout arm_smmu_debugfs_atc_write: ATC_INV ret=0

It seems difficult to insert a CMDQ_OP_CFGI_STE in the submission thread?

I didn't look, but I thought the CMDQ stops on the ATC invalidation, flags the error and the ISR NOP's the failing CMDQ entry and restarts it to resume the thread? Is that something else?

Quoted 1 line

If so you could insert the STE flush instead of a NOP

Nope, sadly the timeout is asynchronous, and CERROR_ATC_INV_SYNC is only reported on the *next* CMD_SYNC - it can't even tell us which CMD_ATC_INV(s) had a problem.

Also there is no NOP; currently the only command rewriting we do is for CERROR_ILL, where we turn the illegal command into a CMD_SYNC.

Quoted 21 lines · Robin Murphy

Quoted 20 lines · Jason Gunthorpe

Quoted 2 lines · Nicolin Chen

You mean in arm_smmu_cmdq_issue_cmdlist() that issued the timed out ATC command?

Yes, it was my off hand thought.

Quoted 8 lines

So my test case was to trigger a device fault followed by an ATC command. But, I found that the ATC command submission returned 0 while only the ISR received:
CMDQ error (cons 0x03000003): ATC invalidate timeout arm_smmu_debugfs_atc_write: ATC_INV ret=0

It seems difficult to insert a CMDQ_OP_CFGI_STE in the submission thread?

I didn't look, but I thought the CMDQ stops on the ATC invalidation, flags the error and the ISR NOP's the failing CMDQ entry and restarts it to resume the thread? Is that something else?

Quoted 5 lines

Quoted 1 line

If so you could insert the STE flush instead of a NOP

Nope, sadly the timeout is asynchronous, and CERROR_ATC_INV_SYNC is only reported on the *next* CMD_SYNC - it can't even tell us which CMD_ATC_INV(s) had a problem.

!! That's a good point! The new invalidation code runs many ATC invalidations under one sync to optimize for SVA performance so we have no idea what devices need to be reset :(

So we really do need to signal to the issuing thread and it will have to go back and check how many ATC invalidations are under this sync and re-issue one by one to isolate the error then issue the STE change and sync. Nothing from an ISR then..

IIUIC, we would have two timeouts to identify the device(s), so we wouldn't need to give away the optimization of batching ATCI cmds?

Will letting a faulty device time out once again give it a window to corrupt the memory?

Quoted 2 lines · Nicolin Chen

IIUIC, we would have two timeouts to identify the device(s), so we wouldn't need to give away the optimization of batching ATCI cmds?

Yes we should not harm success path performance to compensate for a RAS error case.

Quoted 2 lines

Will letting a faulty device time out once again give it a window to corrupt the memory?

No, you are doing this within the flush callback so everything relying on that flush should remain held.

Quoted 7 lines · Nicolin Chen

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +7 -0

+	/*
+	 * ATC timeout indicates the device has stopped responding to coherence
+	 * protocol requests. The only safe recovery is a reset to flush stale
+	 * cached translations. Note that pci_reset_function() internally calls
+	 * pci_dev_reset_iommu_prepare/done() as well and ensures to block ATS
+	 * if PCI-level reset fails.
+	 */

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static void arm_smmu_cmdq_build_sync_cmd(u64 *cmd, struct arm_smmu_device *smmu, · line 489

Quoted 10 lines

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +10 -0

+	if (!pci_reset_function(pdev)) {
+		/*
+		 * If reset succeeds, set BME back. Otherwise, fence the system
+		 * from a faulty device, in which case user will have to replug
+		 * the device to invoke pci_set_master().
+		 */
+		pci_dev_lock(pdev);
+		pci_set_master(pdev);
+		pci_dev_unlock(pdev);
+	}

I thought we talked about this, the iommu driver cannot just blindly issue a reset like this, the reset has to come from the actual device driver through the AERish mechanism. Otherwise the driver RAS is going to explode.

The smmu driver should immediately block the STE (reject translated requests) to protect the system before resuming whatever command submissio n has encountered the error.

You could delegate the STE change to the interrupted command submission to avoid doing it from a ISR, that makes alot of sense because the submission thread is already operating a cmdq so it could stick in a STE invalidation command, possibly even in place of the failed ATC command.

I think I'd break this up into smaller steps, just focus on this STE mechanism at start and have any future attach callback fix the STE.

Then we can talk about how to properly trigger the PCI RAS flow and so on.

I believe this issue is not unique to the arm-smmu-v3 driver. Device ATC invalidation timeout is a generic challenge across all IOMMU architectures that support PCI ATS. Would it be feasible to implement a common 'fencing and recovery' mechanism in the IOMMU core so that all IOMMU drivers could benefit?

Quoted 5 lines · Baolu Lu

I believe this issue is not unique to the arm-smmu-v3 driver. Device ATC invalidation timeout is a generic challenge across all IOMMU architectures that support PCI ATS. Would it be feasible to implement a common 'fencing and recovery' mechanism in the IOMMU core so that all IOMMU drivers could benefit?

I think yes, for parts, but the driver itself has to do something deep inside it's invalidation to allow the flush to complete without exposing the system to memory corruption - meaning it has to block translated requests before completing the flush

I don't see how that can be made too generalized since we are running this flush stuff in irq and reclaim contexts, it has to be very small and targtted without memory allocation or sleeping locks.

Jason

Quoted 11 lines · Jason Gunthorpe

Quoted 5 lines · Baolu Lu

I believe this issue is not unique to the arm-smmu-v3 driver. Device ATC invalidation timeout is a generic challenge across all IOMMU architectures that support PCI ATS. Would it be feasible to implement a common 'fencing and recovery' mechanism in the IOMMU core so that all IOMMU drivers could benefit?

I think yes, for parts, but the driver itself has to do something deep inside it's invalidation to allow the flush to complete without exposing the system to memory corruption - meaning it has to block translated requests before completing the flush

Yes and currently the underlying drivers have software timeouts (AMD=100millisecond, arm-smmu-v3=1second) defined which could timeout before the actual ATC invalidation timeout occurs. Do you think maybe the timeout needs to be propagated to the caller (flush callback) so the memory/IOVA is not allocated to something else?

Or blocking translated requests for such devices should be enough?

Quoted 13 lines · Samiullah Khawaja

Quoted 11 lines · Jason Gunthorpe

Quoted 5 lines · Baolu Lu

I believe this issue is not unique to the arm-smmu-v3 driver. Device ATC invalidation timeout is a generic challenge across all IOMMU architectures that support PCI ATS. Would it be feasible to implement a common 'fencing and recovery' mechanism in the IOMMU core so that all IOMMU drivers could benefit?

I think yes, for parts, but the driver itself has to do something deep inside it's invalidation to allow the flush to complete without exposing the system to memory corruption - meaning it has to block translated requests before completing the flush

Quoted 5 lines

Yes and currently the underlying drivers have software timeouts (AMD=100millisecond, arm-smmu-v3=1second) defined which could timeout before the actual ATC invalidation timeout occurs. Do you think maybe the timeout needs to be propagated to the caller (flush callback) so the memory/IOVA is not allocated to something else?

No, definitely not, that's basically impossible, so many callers just can't handle such an idea, and you can't ever fully recover from such a thing.

Quoted 1 line

Or blocking translated requests for such devices should be enough?

Yes, we have to fence the hardware and then allow the existing SW stack to continue without any fear of UAF from the broken HW.

Fencing the HW means using the IOMMU to block translated requests.

Jason

Quoted 20 lines · Jason Gunthorpe

Quoted 13 lines · Samiullah Khawaja

Quoted 11 lines · Jason Gunthorpe

Quoted 5 lines · Baolu Lu

I believe this issue is not unique to the arm-smmu-v3 driver. Device ATC invalidation timeout is a generic challenge across all IOMMU architectures that support PCI ATS. Would it be feasible to implement a common 'fencing and recovery' mechanism in the IOMMU core so that all IOMMU drivers could benefit?

I think yes, for parts, but the driver itself has to do something deep inside it's invalidation to allow the flush to complete without exposing the system to memory corruption - meaning it has to block translated requests before completing the flush

Quoted 5 lines

Yes and currently the underlying drivers have software timeouts (AMD=100millisecond, arm-smmu-v3=1second) defined which could timeout before the actual ATC invalidation timeout occurs. Do you think maybe the timeout needs to be propagated to the caller (flush callback) so the memory/IOVA is not allocated to something else?

Quoted 4 lines

No, definitely not, that's basically impossible, so many callers just can't handle such an idea, and you can't ever fully recover from such a thing.

Agreed.

Quoted 4 lines

Quoted 1 line

Or blocking translated requests for such devices should be enough?

Yes, we have to fence the hardware and then allow the existing SW stack to continue without any fear of UAF from the broken HW.

And this applies to software timeout also I think, since both have same end result.

Quoted 26 lines · Samiullah Khawaja

Quoted 20 lines · Jason Gunthorpe

Quoted 13 lines · Samiullah Khawaja

Quoted 11 lines · Jason Gunthorpe

Quoted 5 lines · Baolu Lu

I believe this issue is not unique to the arm-smmu-v3 driver. Device ATC invalidation timeout is a generic challenge across all IOMMU architectures that support PCI ATS. Would it be feasible to implement a common 'fencing and recovery' mechanism in the IOMMU core so that all IOMMU drivers could benefit?

I think yes, for parts, but the driver itself has to do something deep inside it's invalidation to allow the flush to complete without exposing the system to memory corruption - meaning it has to block translated requests before completing the flush

Quoted 5 lines

Yes and currently the underlying drivers have software timeouts (AMD=100millisecond, arm-smmu-v3=1second) defined which could timeout before the actual ATC invalidation timeout occurs. Do you think maybe the timeout needs to be propagated to the caller (flush callback) so the memory/IOVA is not allocated to something else?

Quoted 4 lines

No, definitely not, that's basically impossible, so many callers just can't handle such an idea, and you can't ever fully recover from such a thing.

Quoted 8 lines

Agreed.

Quoted 4 lines

Quoted 1 line

Or blocking translated requests for such devices should be enough?

Yes, we have to fence the hardware and then allow the existing SW stack to continue without any fear of UAF from the broken HW.

And this applies to software timeout also I think, since both have same end result.

Any situation where the ATC flush doesn't get a positive response from the HW must fence the HW before continuing to avoid UAF bugs.

Obviously today we just succeed the flush anyhow and hope for the best, and I think that is a good starting point for VT-d. We need at least that to build anything more complex on to.

Fencing the device also has to come with a full RAS flow to eventually unfence it, so I wouldn't do it in isolation.

I would like the unfence to be done with a fresh domain attach (or re-attach I guess) that just rewrites the context entry with the correct one.

For VT-d that probably also means it will need all the domain attach fixing we've talked about as a precondition too.

Jason

But do you think doing the timeout logic without fencing would be good enough?

Currently VT-d blocks itself, until it gets an Invalidation Timeout from HW, and system ends up in a hardlockup since interrupts are disabled.

Are you concerned that if fencing is done without an RAS flow, the device might not be able to detect the failure (if it really needs ATS to work)?

I am thinking, we can do translated fence and timeout change for VT-d. And the device can use existing RAS mechanism to recover itself. This way we atleast make sure that caller of flush can reuse the memory/IOVAs without UAFs.

Quoted 2 lines · Samiullah Khawaja

But do you think doing the timeout logic without fencing would be good enough?

It is what ARM and AMD do, so I wouldn't object to it.

Quoted 8 lines

Currently VT-d blocks itself, until it gets an Invalidation Timeout from HW, and system ends up in a hardlockup since interrupts are disabled.

Are you concerned that if fencing is done without an RAS flow, the device might not be able to detect the failure (if it really needs ATS to work)?

Yes, and then the device is badly locked because nothing will fix the IOMMU fence.

VFIO might fix it if it is restarted, but other approahces like rmmod/insmod won't restore the broken device.

So I'd rather see a more complete solution before we add fencing to the iommu drivers. Minimally userspace doing a rmmod, flr, insmod should be able to restore the device.

Then auto-FLR through RAS could sit on top of that.

Quoted 4 lines

I am thinking, we can do translated fence and timeout change for VT-d. And the device can use existing RAS mechanism to recover itself. This way we atleast make sure that caller of flush can reuse the memory/IOVAs without UAFs.

Without a larger framework to unfence I think this will get devices stuck..

Jason

Quoted 6 lines · Jason Gunthorpe

Quoted 2 lines · Samiullah Khawaja

But do you think doing the timeout logic without fencing would be good enough?

It is what ARM and AMD do, so I wouldn't object to it.

I think without any back pressure to the caller, a device will be able to fill the invalidation queue with device IOTLB invalidations that get stuck until the HW timeout occurs.

Currently, when GERROR_CMDQ_ERR occurs, the arm_smmu_cmdq_skip_err() won't do anything for the CMDQ_ERR_CERROR_ATC_INV_IDX.

When a device wasn't responsive to an ATC invalidation request, this often results in constant CMDQ errors:
unexpected global error reported (0x00000001), this could be serious CMDQ error (cons 0x0302bb84): ATC invalidate timeout unexpected global error reported (0x00000001), this could be serious CMDQ error (cons 0x0302bb88): ATC invalidate timeout unexpected global error reported (0x00000001), this could be serious CMDQ error (cons 0x0302bb8c): ATC invalidate timeout
...

An ATC invalidation timeout indicates that the device failed to respond to a protocol-critical coherency request, which means that device's internal ATS state is desynchronized from the SMMU.

Furthermore, ignoring the timeout leaves the system in an unsafe state, as the device cache may retain stale ATC entries for memory pages that the OS has already reclaimed and reassigned. This might lead to data corruption.

The only safe recovery action is to issue a PCI reset, which guarantees to flush all internal device caches and recover the device.

Read the ATC_INV command that led to the timeouts, and schedule a recovery worker to reset the device corresponding to the Stream ID. If reset fails, keep the device in the resetting/blocking domain to avoid data corruption.

Though it'd be ideal to block it immediately in the ISR, it cannot be done because an STE update would require another CFIG_STE command that couldn't

finish in the context of an ISR handling a CMDQ error.

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

---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |   5 +
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 131 +++++++++++++++++++-
 2 files changed, 132 insertions(+), 4 deletions(-)

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +5 -0

index 3c6d65d36164f..8789cf8294504 100644
@@ -803,6 +803,11 @@ struct arm_smmu_device {
 
 	struct rb_root			streams;
 	struct mutex			streams_mutex;
+
+	struct {
+		struct list_head	list;
+		spinlock_t		lock; /* Lock the list */
+	} atc_recovery;
 };
 
 struct arm_smmu_stream {

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +58 -0

index 4d00d796f0783..de182c27c77c4 100644
@@ -106,6 +106,8 @@ static const char * const event_class_str[] = {
 	[3] = "Reserved",
 };
 
+static struct arm_smmu_master *
+arm_smmu_find_master(struct arm_smmu_device *smmu, u32 sid);
 static int arm_smmu_alloc_cd_tables(struct arm_smmu_master *master);
 
 static void parse_driver_options(struct arm_smmu_device *smmu)

@@ -174,6 +176,13 @@ static void queue_inc_cons(struct arm_smmu_ll_queue *q)
 	q->cons = Q_OVF(q->cons) | Q_WRP(q, cons) | Q_IDX(q, cons);
 }
 
+static u32 queue_prev_cons(struct arm_smmu_ll_queue *q, u32 cons)
+{
+	u32 idx_wrp = (Q_WRP(q, cons) | Q_IDX(q, cons)) - 1;
+
+	return Q_OVF(cons) | Q_WRP(q, idx_wrp) | Q_IDX(q, idx_wrp);
+}
+
 static void queue_sync_cons_ovf(struct arm_smmu_queue *q)
 {
 	struct arm_smmu_ll_queue *llq = &q->llq;

@@ -410,6 +419,97 @@ static void arm_smmu_cmdq_build_sync_cmd(u64 *cmd, struct arm_smmu_device *smmu,
 		u64p_replace_bits(cmd, CMDQ_SYNC_0_CS_NONE, CMDQ_SYNC_0_CS);
 }
 
+/* ATC recovery upon ATC invalidation timeout */
+struct arm_smmu_atc_recovery_param {
+	struct arm_smmu_device *smmu;
+	struct pci_dev *pdev;
+	u32 sid;
+
+	struct work_struct work;
+	struct list_head node;
+};
+
+static void arm_smmu_atc_recovery_worker(struct work_struct *work)
+{
+	struct arm_smmu_atc_recovery_param *param =
+		container_of(work, struct arm_smmu_atc_recovery_param, work);
+	struct pci_dev *pdev;
+
+	scoped_guard(mutex, &param->smmu->streams_mutex) {
+		struct arm_smmu_master *master;
+
+		master = arm_smmu_find_master(param->smmu, param->sid);
+		if (!master || WARN_ON(!dev_is_pci(master->dev)))
+			goto free_param;
+		pdev = to_pci_dev(master->dev);
+		pci_dev_get(pdev);
+	}
+
+	scoped_guard(spinlock_irqsave, &param->smmu->atc_recovery.lock) {
+		struct arm_smmu_atc_recovery_param *e;
+
+		list_for_each_entry(e, &param->smmu->atc_recovery.list, node) {
+			/* Device is already being recovered */
+			if (e->pdev == pdev)
+				goto put_pdev;
+		}
+		param->pdev = pdev;
+		list_add(&param->node, &param->smmu->atc_recovery.list);
+	}
+
+	/*
+	 * Stop DMA (PCI) and block ATS (IOMMU) immediately, to prevent memory
+	 * corruption. This must take pci_dev_lock to prevent any racy unplug.
+	 *
+	 * If pci_dev_reset_iommu_prepare() fails, pci_reset_function will call
+	 * it again internally.
+	 */
+	pci_dev_lock(pdev);
+	pci_clear_master(pdev);
+	if (pci_dev_reset_iommu_prepare(pdev))
+		pci_err(pdev, "failed to block ATS!\n");

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +26 -0

+		pci_set_master(pdev);
+		pci_dev_unlock(pdev);
+	}
+	scoped_guard(spinlock_irqsave, &param->smmu->atc_recovery.lock)
+		list_del(&param->node);
+put_pdev:
+	pci_dev_put(pdev);
+free_param:
+	kfree(param);
+}
+
+static int arm_smmu_sched_atc_recovery(struct arm_smmu_device *smmu, u32 sid)
+{
+	struct arm_smmu_atc_recovery_param *param;
+
+	param = kzalloc_obj(*param, GFP_ATOMIC);
+	if (!param)
+		return -ENOMEM;
+	param->smmu = smmu;
+	param->sid = sid;
+
+	INIT_WORK(&param->work, arm_smmu_atc_recovery_worker);
+	queue_work(system_unbound_wq, &param->work);
+	return 0;
+}
+
 void __arm_smmu_cmdq_skip_err(struct arm_smmu_device *smmu,
 			      struct arm_smmu_cmdq *cmdq)
 {

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +8 -0

+		fallthrough;
 	case CMDQ_ERR_CERROR_ILL_IDX:
 	default:
 		break;

@@ -456,6 +555,27 @@ void __arm_smmu_cmdq_skip_err(struct arm_smmu_device *smmu,
 	 * not to touch any of the shadow cmdq state.
 	 */
 	queue_read(cmd, Q_ENT(q, cons), q->ent_dwords);
+
+	if (idx == CMDQ_ERR_CERROR_ATC_INV_IDX) {
+		/*
+		 * Since commands can be issued in batch making it difficult to
+		 * identify which CMDQ_OP_ATC_INV actually timed out, the driver
+		 * must ensure only CMDQ_OP_ATC_INV commands for the same device
+		 * can be batched.

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +4 -0

+		return;
+	}
+
+	/* idx == CMDQ_ERR_CERROR_ILL_IDX */
 	dev_err(smmu->dev, "skipping command in error state:\n");
 	for (i = 0; i < ARRAY_SIZE(cmd); ++i)
 		dev_err(smmu->dev, "\t0x%016llx\n", (unsigned long long)cmd[i]);

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static void arm_smmu_cmdq_build_sync_cmd(u64 *cmd, struct arm_smmu_device *smmu, · line 480

Quoted 8 lines · Nicolin Chen

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +8 -0

+	/*
+	 * ATC timeout indicates the device has stopped responding to coherence
+	 * protocol requests. The only safe recovery is a reset to flush stale
+	 * cached translations. Note that pci_reset_function() internally calls
+	 * pci_dev_reset_iommu_prepare/done() as well and ensures to block ATS
+	 * if PCI-level reset fails.
+	 */
+	if (!pci_reset_function(pdev)) {

I'm a little uncomfortable with this, why is an IOMMU driver poking into the PCI mechanics? I agree that a reset might be the right thing to do here but we wouldn't want the IOMMU driver to trigger it.. Ideally, we'd need a mechanism that bubbles up fatal IOMMU faults to the PCI core and let it decide/perform the reset. Maybe this could mean adding another op to struct pci_error_handlers or something like that?

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static void arm_smmu_cmdq_build_sync_cmd(u64 *cmd, struct arm_smmu_device *smmu, · line 486

Quoted 6 lines

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +6 -0

+		/*
+		 * If reset succeeds, set BME back. Otherwise, fence the system
+		 * from a faulty device, in which case user will have to replug
+		 * the device to invoke pci_set_master().
+		 */
+		pci_dev_lock(pdev);

Why are we using spinlock_irqsave across the worker? Also, why does atc_recovery.lock have to be a spinlock? The workers run in process context, and I also don't see anyone else take the atc_recovery.lock?

Why does it need to be irq-safe? If this can somehow run in irq context, we also seem to be using pci_dev_lock and streams_mutex across the worker?

Mixing mutexes with spinlocks is brittle and invites "sleep-while-atomic" bugs in future refactors..

Quoted 16 lines · Pranjal Shrivastava

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static void arm_smmu_cmdq_build_sync_cmd(u64 *cmd, struct arm_smmu_device *smmu, · line 480

Quoted 8 lines · Nicolin Chen

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +8 -0

+	/*
+	 * ATC timeout indicates the device has stopped responding to coherence
+	 * protocol requests. The only safe recovery is a reset to flush stale
+	 * cached translations. Note that pci_reset_function() internally calls
+	 * pci_dev_reset_iommu_prepare/done() as well and ensures to block ATS
+	 * if PCI-level reset fails.
+	 */
+	if (!pci_reset_function(pdev)) {

I'm a little uncomfortable with this, why is an IOMMU driver poking into the PCI mechanics? I agree that a reset might be the right thing to do here but we wouldn't want the IOMMU driver to trigger it.. Ideally, we'd need a mechanism that bubbles up fatal IOMMU faults to the PCI core and let it decide/perform the reset. Maybe this could mean adding another op to struct pci_error_handlers or something like that?

Robin/Jason already had similar remarks (to most of your other comments as well). I have acked their comments, and am already reworking on these.

Quoted 10 lines

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static void arm_smmu_cmdq_build_sync_cmd(u64 *cmd, struct arm_smmu_device *smmu, · line 486

Quoted 6 lines

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +6 -0

+		/*
+		 * If reset succeeds, set BME back. Otherwise, fence the system
+		 * from a faulty device, in which case user will have to replug
+		 * the device to invoke pci_set_master().
+		 */
+		pci_dev_lock(pdev);

Why are we using spinlock_irqsave across the worker? Also, why does atc_recovery.lock have to be a spinlock? The workers run in process context, and I also don't see anyone else take the atc_recovery.lock?

I guess mutex would be okay here, since there is no other place access the linked list. Pairing a linked list with a spinlock is just a common practice..

Quoted 3 lines

Why does it need to be irq-safe? If this can somehow run in irq context, we also seem to be using pci_dev_lock and streams_mutex across the worker?

pci_dev_lock was to fence race on the PCI level. Yet, the entire BME call is probably not a good idea. So, dropping that means we won't need pci_dev_lock.

Quoted 2 lines

Mixing mutexes with spinlocks is brittle and invites "sleep-while-atomic" bugs in future refactors..

Either streams_mutex or atc_recovery.lock was scoped for only a few lines each section. Each was released before the other one was taken. Where is the "mixing" or "sleep-while-atomic" case?