SMMUv3 CMD_SYNC optimisation

As an IRQ, the CMD_SYNC interrupt is not particularly useful, not least because we often need to wait for sync completion within someone else's IRQ handler anyway. However, when the SMMU is both coherent and supports MSIs, we can have a lot more fun by not using it as an interrupt at all. Following the example suggested in the architecture and using a write targeting normal memory, we can let callers wait on a status variable outside the lock instead of having to stall the entire queue or even touch MMIO registers. Since multiple sync commands are guaranteed to complete in order, a simple incrementing sequence count is all we need to unambiguously support any realistic number of overlapping waiters.

Signed-off-by Robin Murphy <[email protected]>

---

v2: Remove redundant 'bool msi' command member, other cosmetic tweaks

 drivers/iommu/arm-smmu-v3.c | 47 +++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 45 insertions(+), 2 deletions(-)

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

index f066725298cd..311f482b93d5 100644
@@ -377,7 +377,16 @@
 
 #define CMDQ_SYNC_0_CS_SHIFT		12
 #define CMDQ_SYNC_0_CS_NONE		(0UL << CMDQ_SYNC_0_CS_SHIFT)
+#define CMDQ_SYNC_0_CS_IRQ		(1UL << CMDQ_SYNC_0_CS_SHIFT)
 #define CMDQ_SYNC_0_CS_SEV		(2UL << CMDQ_SYNC_0_CS_SHIFT)
+#define CMDQ_SYNC_0_MSH_SHIFT		22
+#define CMDQ_SYNC_0_MSH_ISH		(3UL << CMDQ_SYNC_0_MSH_SHIFT)
+#define CMDQ_SYNC_0_MSIATTR_SHIFT	24
+#define CMDQ_SYNC_0_MSIATTR_OIWB	(0xfUL << CMDQ_SYNC_0_MSIATTR_SHIFT)
+#define CMDQ_SYNC_0_MSIDATA_SHIFT	32
+#define CMDQ_SYNC_0_MSIDATA_MASK	0xffffffffUL
+#define CMDQ_SYNC_1_MSIADDR_SHIFT	0
+#define CMDQ_SYNC_1_MSIADDR_MASK	0xffffffffffffcUL
 
 /* Event queue */
 #define EVTQ_ENT_DWORDS			4

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

 #define MSI_IOVA_BASE			0x8000000
 #define MSI_IOVA_LENGTH			0x100000

@@ -504,6 +514,10 @@ struct arm_smmu_cmdq_ent {
 		} pri;
 
 		#define CMDQ_OP_CMD_SYNC	0x46
+		struct {
+			u32			msidata;
+			u64			msiaddr;
+		} sync;
 	};
 };
 

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

+
 	unsigned long			ias; /* IPA */
 	unsigned long			oas; /* PA */
 	unsigned long			pgsize_bitmap;

@@ -878,7 +895,13 @@ static int arm_smmu_cmdq_build_cmd(u64 *cmd, struct arm_smmu_cmdq_ent *ent)
 		}
 		break;
 	case CMDQ_OP_CMD_SYNC:
-		cmd[0] |= CMDQ_SYNC_0_CS_SEV;
+		if (ent->sync.msiaddr)
+			cmd[0] |= CMDQ_SYNC_0_CS_IRQ;
+		else
+			cmd[0] |= CMDQ_SYNC_0_CS_SEV;
+		cmd[0] |= CMDQ_SYNC_0_MSH_ISH | CMDQ_SYNC_0_MSIATTR_OIWB;
+		cmd[0] |= (u64)ent->sync.msidata << CMDQ_SYNC_0_MSIDATA_SHIFT;
+		cmd[1] |= ent->sync.msiaddr & CMDQ_SYNC_1_MSIADDR_MASK;
 		break;
 	default:
 		return -ENOENT;

@@ -964,21 +987,40 @@ static void arm_smmu_cmdq_issue_cmd(struct arm_smmu_device *smmu,
 	spin_unlock_irqrestore(&smmu->cmdq.lock, flags);

Even without the MSI trick, we can still do a lot better than hogging the entire queue while it drains. All we actually need to do for the necessary guarantee of completion is wait for our particular command to have been consumed, and as long as we keep track of where it is there is no need to block other CPUs from adding further commands in the meantime. There is one theoretical (but incredibly unlikely) edge case to avoid, where cons has wrapped twice to still appear 'behind' the sync position - this is easily disambiguated by adding a generation count to the queue to indicate when prod wraps, since cons cannot wrap twice without prod having wrapped at least once.

Signed-off-by Robin Murphy <[email protected]>

---

v2: New

 drivers/iommu/arm-smmu-v3.c | 81 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 58 insertions(+), 23 deletions(-)

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

index 311f482b93d5..f5c5da553803 100644
@@ -417,7 +417,6 @@
 
 /* High-level queue structures */
 #define ARM_SMMU_POLL_TIMEOUT_US	100
-#define ARM_SMMU_CMDQ_DRAIN_TIMEOUT_US	1000000 /* 1s! */
 #define ARM_SMMU_SYNC_TIMEOUT_US	1000000 /* 1s! */
 
 #define MSI_IOVA_BASE			0x8000000

@@ -540,6 +539,7 @@ struct arm_smmu_queue {
 struct arm_smmu_cmdq {
 	struct arm_smmu_queue		q;
 	spinlock_t			lock;
+	int				generation;
 };
 
 struct arm_smmu_evtq {

@@ -770,21 +770,12 @@ static void queue_inc_prod(struct arm_smmu_queue *q)
 	writel(q->prod, q->prod_reg);
 }
 
-/*
- * Wait for the SMMU to consume items. If drain is true, wait until the queue
- * is empty. Otherwise, wait until there is at least one free slot.
- */
-static int queue_poll_cons(struct arm_smmu_queue *q, bool drain, bool wfe)
+/* Wait for the SMMU to consume items, until there is at least one free slot */
+static int queue_poll_cons(struct arm_smmu_queue *q, bool wfe)
 {
-	ktime_t timeout;
-	unsigned int delay = 1;
+	ktime_t timeout = ktime_add_us(ktime_get(), ARM_SMMU_POLL_TIMEOUT_US);
 
-	/* Wait longer if it's queue drain */
-	timeout = ktime_add_us(ktime_get(), drain ?
-					    ARM_SMMU_CMDQ_DRAIN_TIMEOUT_US :
-					    ARM_SMMU_POLL_TIMEOUT_US);
-
-	while (queue_sync_cons(q), (drain ? !queue_empty(q) : queue_full(q))) {
+	while (queue_sync_cons(q), queue_full(q)) {
 		if (ktime_compare(ktime_get(), timeout) > 0)
 			return -ETIMEDOUT;
 

@@ -792,8 +783,7 @@ static int queue_poll_cons(struct arm_smmu_queue *q, bool drain, bool wfe)
 			wfe();
 		} else {
 			cpu_relax();
-			udelay(delay);
-			delay *= 2;
+			udelay(1);
 		}
 	}
 

@@ -959,15 +949,20 @@ static void arm_smmu_cmdq_skip_err(struct arm_smmu_device *smmu)
 	queue_write(Q_ENT(q, cons), cmd, q->ent_dwords);
 }
 
-static void arm_smmu_cmdq_insert_cmd(struct arm_smmu_device *smmu, u64 *cmd)
+static u32 arm_smmu_cmdq_insert_cmd(struct arm_smmu_device *smmu, u64 *cmd)
 {
 	struct arm_smmu_queue *q = &smmu->cmdq.q;

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

+
+	return q->prod;
 }
 
 static void arm_smmu_cmdq_issue_cmd(struct arm_smmu_device *smmu,

@@ -997,15 +992,54 @@ static int arm_smmu_sync_poll_msi(struct arm_smmu_device *smmu, u32 sync_idx)
 	return (int)(val - sync_idx) < 0 ? -ETIMEDOUT : 0;
 }
 
+static int arm_smmu_sync_poll_cons(struct arm_smmu_device *smmu, u32 sync_idx,
+				   int sync_gen)
+{
+	ktime_t timeout = ktime_add_us(ktime_get(), ARM_SMMU_SYNC_TIMEOUT_US);
+	struct arm_smmu_queue *q = &smmu->cmdq.q;
+	bool wfe = !!(smmu->features & ARM_SMMU_FEAT_SEV);
+	unsigned int delay = 1;
+

Since Nate reported a reasonable performance boost from the out-of-line MSI polling in v1 [1], I've now implemented the equivalent for cons polling as well - that has been boot-tested on D05 with some trivial I/O and at least doesn't seem to lock up or explode. There's also a little cosmetic tweaking to make the patches a bit cleaner as a series.

Robin.

[1] https://www.mail-archive.com/iommu at lists.linux-foundation.org/msg19657.html

Robin Murphy (5):
iommu/arm-smmu-v3: Specialise CMD_SYNC handling iommu/arm-smmu-v3: Forget about cmdq-sync interrupt iommu/arm-smmu-v3: Use CMD_SYNC completion interrupt iommu/arm-smmu-v3: Poll for CMD_SYNC outside cmdq lock iommu/arm-smmu-v3: Use burst-polling for sync completion

Hi Robin,

This mostly looks good. Just a few comments below.

Quoted 38 lines · Robin Murphy

As an IRQ, the CMD_SYNC interrupt is not particularly useful, not least because we often need to wait for sync completion within someone else's IRQ handler anyway. However, when the SMMU is both coherent and supports MSIs, we can have a lot more fun by not using it as an interrupt at all. Following the example suggested in the architecture and using a write targeting normal memory, we can let callers wait on a status variable outside the lock instead of having to stall the entire queue or even touch MMIO registers. Since multiple sync commands are guaranteed to complete in order, a simple incrementing sequence count is all we need to unambiguously support any realistic number of overlapping waiters.

Signed-off-by Robin Murphy <[email protected]>

---

v2: Remove redundant 'bool msi' command member, other cosmetic tweaks

 drivers/iommu/arm-smmu-v3.c | 47 +++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 45 insertions(+), 2 deletions(-)

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

index f066725298cd..311f482b93d5 100644
@@ -377,7 +377,16 @@
 
 #define CMDQ_SYNC_0_CS_SHIFT		12
 #define CMDQ_SYNC_0_CS_NONE		(0UL << CMDQ_SYNC_0_CS_SHIFT)
+#define CMDQ_SYNC_0_CS_IRQ		(1UL << CMDQ_SYNC_0_CS_SHIFT)
 #define CMDQ_SYNC_0_CS_SEV		(2UL << CMDQ_SYNC_0_CS_SHIFT)
+#define CMDQ_SYNC_0_MSH_SHIFT		22
+#define CMDQ_SYNC_0_MSH_ISH		(3UL << CMDQ_SYNC_0_MSH_SHIFT)
+#define CMDQ_SYNC_0_MSIATTR_SHIFT	24
+#define CMDQ_SYNC_0_MSIATTR_OIWB	(0xfUL << CMDQ_SYNC_0_MSIATTR_SHIFT)
+#define CMDQ_SYNC_0_MSIDATA_SHIFT	32
+#define CMDQ_SYNC_0_MSIDATA_MASK	0xffffffffUL
+#define CMDQ_SYNC_1_MSIADDR_SHIFT	0
+#define CMDQ_SYNC_1_MSIADDR_MASK	0xffffffffffffcUL
 

drivers/iommu/arm-smmu-v3.c line 421

Quoted 7 lines

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

 /* Event queue */
 #define EVTQ_ENT_DWORDS			4

@@ -409,6 +418,7 @@
 /* High-level queue structures */
 #define ARM_SMMU_POLL_TIMEOUT_US	100
 #define ARM_SMMU_CMDQ_DRAIN_TIMEOUT_US	1000000 /* 1s! */
+#define ARM_SMMU_SYNC_TIMEOUT_US	1000000 /* 1s! */

We only ever do this when waiting for the queue to drain, so may as well just reuse the drain timeout.

Quoted 17 lines

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

 #define MSI_IOVA_BASE			0x8000000
 #define MSI_IOVA_LENGTH			0x100000

@@ -504,6 +514,10 @@ struct arm_smmu_cmdq_ent {
 		} pri;
 
 		#define CMDQ_OP_CMD_SYNC	0x46
+		struct {
+			u32			msidata;
+			u64			msiaddr;
+		} sync;
 	};
 };
 

@@ -617,6 +631,9 @@ struct arm_smmu_device {
 	int				gerr_irq;
 	int				combined_irq;
 

drivers/iommu/arm-smmu-v3.c struct arm_smmu_device { · line 635

Quoted 2 lines

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

+	atomic_t			sync_nr;
+	u32				sync_count;

It's probably worth sticking these in separate cachelines so we don't get spurious wakeups when sync_nr is incremented. (yes, I know it should be the ERG, but that can be unreasonably huge!).

Quoted 23 lines

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

+
 	unsigned long			ias; /* IPA */
 	unsigned long			oas; /* PA */
 	unsigned long			pgsize_bitmap;

@@ -878,7 +895,13 @@ static int arm_smmu_cmdq_build_cmd(u64 *cmd, struct arm_smmu_cmdq_ent *ent)
 		}
 		break;
 	case CMDQ_OP_CMD_SYNC:
-		cmd[0] |= CMDQ_SYNC_0_CS_SEV;
+		if (ent->sync.msiaddr)
+			cmd[0] |= CMDQ_SYNC_0_CS_IRQ;
+		else
+			cmd[0] |= CMDQ_SYNC_0_CS_SEV;
+		cmd[0] |= CMDQ_SYNC_0_MSH_ISH | CMDQ_SYNC_0_MSIATTR_OIWB;
+		cmd[0] |= (u64)ent->sync.msidata << CMDQ_SYNC_0_MSIDATA_SHIFT;
+		cmd[1] |= ent->sync.msiaddr & CMDQ_SYNC_1_MSIADDR_MASK;
 		break;
 	default:
 		return -ENOENT;

@@ -964,21 +987,40 @@ static void arm_smmu_cmdq_issue_cmd(struct arm_smmu_device *smmu,
 	spin_unlock_irqrestore(&smmu->cmdq.lock, flags);
 }
 

drivers/iommu/arm-smmu-v3.c static void arm_smmu_cmdq_issue_cmd(struct arm_smmu_device *smmu, · line 997

Quoted 8 lines

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

+static int arm_smmu_sync_poll_msi(struct arm_smmu_device *smmu, u32 sync_idx)
+{
+	ktime_t timeout = ktime_add_us(ktime_get(), ARM_SMMU_SYNC_TIMEOUT_US);
+	u32 val = smp_cond_load_acquire(&smmu->sync_count,
+					(int)(VAL - sync_idx) >= 0 ||
+					!ktime_before(ktime_get(), timeout));
+
+	return (int)(val - sync_idx) < 0 ? -ETIMEDOUT : 0;

There are some theoretical overflow issues here which I don't think will ever occur in practice, but deserve at least a comment to explain why.

Quoted 4 lines

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

+}
+
 static void arm_smmu_cmdq_issue_sync(struct arm_smmu_device *smmu)
 {

drivers/iommu/arm-smmu-v3.c static void arm_smmu_cmdq_issue_cmd(struct arm_smmu_device *smmu, · line 1006

Quoted 5 lines

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

 	u64 cmd[CMDQ_ENT_DWORDS];
 	unsigned long flags;
 	bool wfe = !!(smmu->features & ARM_SMMU_FEAT_SEV);
+	bool msi = (smmu->features & ARM_SMMU_FEAT_MSI) &&
+		   (smmu->features & ARM_SMMU_FEAT_COHERENCY);

I don't think this is sufficient for the case where we fail to setup MSIs and fall back on legacy IRQs.

drivers/iommu/arm-smmu-v3.c static void arm_smmu_cmdq_issue_cmd(struct arm_smmu_device *smmu, · line 1011

Quoted 5 lines

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

 	struct arm_smmu_cmdq_ent ent = { .opcode = CMDQ_OP_CMD_SYNC };
 	int ret;
 
+	if (msi) {
+		ent.sync.msidata = atomic_inc_return(&smmu->sync_nr);

I don't think you need barrier semantics here.

Quoted 11 lines

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

+		ent.sync.msiaddr = virt_to_phys(&smmu->sync_count);
+	}
 	arm_smmu_cmdq_build_cmd(cmd, &ent);
 
 	spin_lock_irqsave(&smmu->cmdq.lock, flags);
 	arm_smmu_cmdq_insert_cmd(smmu, cmd);
-	ret = queue_poll_cons(&smmu->cmdq.q, true, wfe);
+	if (!msi)
+		ret = queue_poll_cons(&smmu->cmdq.q, true, wfe);
 	spin_unlock_irqrestore(&smmu->cmdq.lock, flags);
 

drivers/iommu/arm-smmu-v3.c static void arm_smmu_cmdq_issue_cmd(struct arm_smmu_device *smmu, · line 1023

Quoted 2 lines

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

+	if (msi)
+		ret = arm_smmu_sync_poll_msi(smmu, ent.sync.msidata);

This looks like the queue polling should be wrapped up in a function that returns with the spinlock released.

Hi Robin,

Some of my comments on patch 3 are addressed here, but I'm really struggling to convince myself that this algorithm is correct. My preference would be to leave the code as it is for SMMUs that don't implement MSIs, but comments below anyway because it's an interesting idea.

From scrounging up boot logs I can see that neither the Cavium nor

Quoted 85 lines

Even without the MSI trick, we can still do a lot better than hogging the entire queue while it drains. All we actually need to do for the necessary guarantee of completion is wait for our particular command to have been consumed, and as long as we keep track of where it is there is no need to block other CPUs from adding further commands in the meantime. There is one theoretical (but incredibly unlikely) edge case to avoid, where cons has wrapped twice to still appear 'behind' the sync position - this is easily disambiguated by adding a generation count to the queue to indicate when prod wraps, since cons cannot wrap twice without prod having wrapped at least once.

Signed-off-by Robin Murphy <[email protected]>

---

v2: New

 drivers/iommu/arm-smmu-v3.c | 81 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 58 insertions(+), 23 deletions(-)

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

index 311f482b93d5..f5c5da553803 100644
@@ -417,7 +417,6 @@
 
 /* High-level queue structures */
 #define ARM_SMMU_POLL_TIMEOUT_US	100
-#define ARM_SMMU_CMDQ_DRAIN_TIMEOUT_US	1000000 /* 1s! */
 #define ARM_SMMU_SYNC_TIMEOUT_US	1000000 /* 1s! */
 
 #define MSI_IOVA_BASE			0x8000000

@@ -540,6 +539,7 @@ struct arm_smmu_queue {
 struct arm_smmu_cmdq {
 	struct arm_smmu_queue		q;
 	spinlock_t			lock;
+	int				generation;
 };
 
 struct arm_smmu_evtq {

@@ -770,21 +770,12 @@ static void queue_inc_prod(struct arm_smmu_queue *q)
 	writel(q->prod, q->prod_reg);
 }
 
-/*
- * Wait for the SMMU to consume items. If drain is true, wait until the queue
- * is empty. Otherwise, wait until there is at least one free slot.
- */
-static int queue_poll_cons(struct arm_smmu_queue *q, bool drain, bool wfe)
+/* Wait for the SMMU to consume items, until there is at least one free slot */
+static int queue_poll_cons(struct arm_smmu_queue *q, bool wfe)
 {
-	ktime_t timeout;
-	unsigned int delay = 1;
+	ktime_t timeout = ktime_add_us(ktime_get(), ARM_SMMU_POLL_TIMEOUT_US);
 
-	/* Wait longer if it's queue drain */
-	timeout = ktime_add_us(ktime_get(), drain ?
-					    ARM_SMMU_CMDQ_DRAIN_TIMEOUT_US :
-					    ARM_SMMU_POLL_TIMEOUT_US);
-
-	while (queue_sync_cons(q), (drain ? !queue_empty(q) : queue_full(q))) {
+	while (queue_sync_cons(q), queue_full(q)) {
 		if (ktime_compare(ktime_get(), timeout) > 0)
 			return -ETIMEDOUT;
 

@@ -792,8 +783,7 @@ static int queue_poll_cons(struct arm_smmu_queue *q, bool drain, bool wfe)
 			wfe();
 		} else {
 			cpu_relax();
-			udelay(delay);
-			delay *= 2;
+			udelay(1);
 		}
 	}
 

@@ -959,15 +949,20 @@ static void arm_smmu_cmdq_skip_err(struct arm_smmu_device *smmu)
 	queue_write(Q_ENT(q, cons), cmd, q->ent_dwords);
 }
 
-static void arm_smmu_cmdq_insert_cmd(struct arm_smmu_device *smmu, u64 *cmd)
+static u32 arm_smmu_cmdq_insert_cmd(struct arm_smmu_device *smmu, u64 *cmd)
 {
 	struct arm_smmu_queue *q = &smmu->cmdq.q;
 	bool wfe = !!(smmu->features & ARM_SMMU_FEAT_SEV);
 

drivers/iommu/arm-smmu-v3.c static void arm_smmu_cmdq_skip_err(struct arm_smmu_device *smmu) · line 963

Quoted 8 lines

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

 	while (queue_insert_raw(q, cmd) == -ENOSPC) {
-		if (queue_poll_cons(q, false, wfe))
+		if (queue_poll_cons(q, wfe))
 			dev_err_ratelimited(smmu->dev, "CMDQ timeout\n");
 	}
+
+	if (Q_IDX(q, q->prod) == 0)
+		smmu->cmdq.generation++;

The readers of generation are using READ_ONCE, so you're missing something here.

Quoted 22 lines

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

+
+	return q->prod;
 }
 
 static void arm_smmu_cmdq_issue_cmd(struct arm_smmu_device *smmu,

@@ -997,15 +992,54 @@ static int arm_smmu_sync_poll_msi(struct arm_smmu_device *smmu, u32 sync_idx)
 	return (int)(val - sync_idx) < 0 ? -ETIMEDOUT : 0;
 }
 
+static int arm_smmu_sync_poll_cons(struct arm_smmu_device *smmu, u32 sync_idx,
+				   int sync_gen)
+{
+	ktime_t timeout = ktime_add_us(ktime_get(), ARM_SMMU_SYNC_TIMEOUT_US);
+	struct arm_smmu_queue *q = &smmu->cmdq.q;
+	bool wfe = !!(smmu->features & ARM_SMMU_FEAT_SEV);
+	unsigned int delay = 1;
+
+	do {
+		queue_sync_cons(q);
+		/*
+		 * If we see updates quickly enough, cons has passed sync_idx,
+		 * but not yet wrapped. At worst, cons might have actually

drivers/iommu/arm-smmu-v3.c static int arm_smmu_sync_poll_msi(struct arm_smmu_device *smmu, u32 sync_idx) · line 1012

Quoted 5 lines

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

+		 * wrapped an even number of times, but that still guarantees
+		 * the original sync must have been consumed.
+		 */
+		if (Q_IDX(q, q->cons) >= Q_IDX(q, sync_idx) &&
+		    Q_WRP(q, sync_idx) == Q_WRP(q, q->cons))

Can you move this into a separate function please, like we have for queue_full and queue_empty?

drivers/iommu/arm-smmu-v3.c static int arm_smmu_sync_poll_msi(struct arm_smmu_device *smmu, u32 sync_idx) · line 1020

Quoted 8 lines

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

+			return 0;
+		/*
+		 * Otherwise, cons may have passed sync_idx and wrapped one or
+		 * more times to appear behind it again, but in that case prod
+		 * must also be one or more generations ahead.
+		 */
+		if (Q_IDX(q, q->cons) < Q_IDX(q, sync_idx) &&
+		    READ_ONCE(smmu->cmdq.generation) != sync_gen)

There's another daft overflow case here which deserves a comment (and even if it *did* happen, we'll recover gracefully).

drivers/iommu/arm-smmu-v3.c static int arm_smmu_sync_poll_msi(struct arm_smmu_device *smmu, u32 sync_idx) · line 1024

Quoted 4 lines

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

+			return 0;
+
+		if (wfe) {
+			wfe();

This is a bit scary... if we miss a generation update, just due to store propagation latency (maybe it's buffered by the updater), I think we can end up going into wfe when there's not an event pending. Using xchg everywhere might work, but there's still a race on having somebody update generation. The ordering here just looks generally suspicious to me because you have the generation writer in arm_smmu_cmdq_insert_cmd effectively doing:

Write prod
Write generation

and the reader in arm_smmu_sync_poll_cons doing:

Read cons
Read generation

so I can't see anything that gives you order to guarantee that the generation is seen to be up-to-date.

Hi Robin,

Quoted 10 lines · Robin Murphy

Since Nate reported a reasonable performance boost from the out-of-line MSI polling in v1 [1], I've now implemented the equivalent for cons polling as well - that has been boot-tested on D05 with some trivial I/O and at least doesn't seem to lock up or explode. There's also a little cosmetic tweaking to make the patches a bit cleaner as a series.

Robin.

[1] https://www.mail-archive.com/iommu at lists.linux-foundation.org/msg19657.html

Quoted 6 lines

Robin Murphy (5):
iommu/arm-smmu-v3: Specialise CMD_SYNC handling iommu/arm-smmu-v3: Forget about cmdq-sync interrupt iommu/arm-smmu-v3: Use CMD_SYNC completion interrupt iommu/arm-smmu-v3: Poll for CMD_SYNC outside cmdq lock iommu/arm-smmu-v3: Use burst-polling for sync completion

What's this final mythical patch about? I don't see it in the series.

Anyway, the first two patches look fine to me, but this doesn't apply on top of my iommu/devel branch so they will need rebasing.

Quoted 8 lines · Robin Murphy

Since Nate reported a reasonable performance boost from the out-of-line MSI polling in v1 [1], I've now implemented the equivalent for cons polling as well - that has been boot-tested on D05 with some trivial I/O and at least doesn't seem to lock up or explode. There's also a little cosmetic tweaking to make the patches a bit cleaner as a series.

Robin.

Quoted 8 lines

[1] https://www.mail-archive.com/iommu at lists.linux-foundation.org/msg19657.html

Robin Murphy (5):
iommu/arm-smmu-v3: Specialise CMD_SYNC handling iommu/arm-smmu-v3: Forget about cmdq-sync interrupt iommu/arm-smmu-v3: Use CMD_SYNC completion interrupt iommu/arm-smmu-v3: Poll for CMD_SYNC outside cmdq lock iommu/arm-smmu-v3: Use burst-polling for sync completion

What's this final mythical patch about? I don't see it in the series.

Anyway, the first two patches look fine to me, but this doesn't apply on top of my iommu/devel branch so they will need rebasing.