iommu/arm-smmu-v3: Introduce an RCU-protected invalidation array

Both the ARM_SMMU_DOMAIN_S1 case and the SVA case use ASID, requiring ASID based invalidation commands to flush the TLB.

Define an ARM_SMMU_DOMAIN_SVA to make the SVA case clear to share the same path with the ARM_SMMU_DOMAIN_S1 case, which will be a part of the routine to build a new per-domain invalidation array.

There is no function change.

Suggested-by Jason Gunthorpe <[email protected]>

Acked-by Balbir Singh <[email protected]>

Reviewed-by Jason Gunthorpe <[email protected]>

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

---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h     | 1 +
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 1 +
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c     | 3 +++
 3 files changed, 5 insertions(+)

There will be a bit more things to free than smmu_domain itself. So keep a simple inline function in the header to share aross files.

Suggested-by Jason Gunthorpe <[email protected]>

Reviewed-by Jason Gunthorpe <[email protected]>

Acked-by Balbir Singh <[email protected]>

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-sva.c | 5 +++--
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c     | 4 ++--
 3 files changed, 10 insertions(+), 4 deletions(-)

From: Jason Gunthorpe <[email protected]>

Create a new data structure to hold an array of invalidations that need to be performed for the domain based on what masters are attached, to replace the single smmu pointer and linked list of masters in the current design.

Each array entry holds one of the invalidation actions - S1_ASID, S2_VMID, ATS or their variant with information to feed invalidation commands to HW. It is structured so that multiple SMMUs can participate in the same array, removing one key limitation of the current system.

To maximize performance, a sorted array is used as the data structure. It allows grouping SYNCs together to parallelize invalidations. For instance, it will group all the ATS entries after the ASID/VMID entry, so they will all be pushed to the PCI devices in parallel with one SYNC.

To minimize the locking cost on the invalidation fast path (reader of the invalidation array), the array is managed with RCU.

Provide a set of APIs to add/delete entries to/from an array, which cover cannot-fail attach cases, e.g. attaching to arm_smmu_blocked_domain. Also add kunit coverage for those APIs.

Signed-off-by Jason Gunthorpe <[email protected]>

Reviewed-by Jason Gunthorpe <[email protected]>

Co-developed-by Nicolin Chen <[email protected]>

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

---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  98 +++++++
 .../iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c  |  92 +++++++
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 258 ++++++++++++++++++
 3 files changed, 448 insertions(+)

From: Jason Gunthorpe <[email protected]>

Create a new data structure to hold an array of invalidations that need to be performed for the domain based on what masters are attached, to replace the single smmu pointer and linked list of masters in the current design.

Each array entry holds one of the invalidation actions - S1_ASID, S2_VMID, ATS or their variant with information to feed invalidation commands to HW. It is structured so that multiple SMMUs can participate in the same array, removing one key limitation of the current system.

To maximize performance, a sorted array is used as the data structure. It allows grouping SYNCs together to parallelize invalidations. For instance, it will group all the ATS entries after the ASID/VMID entry, so they will all be pushed to the PCI devices in parallel with one SYNC.

To minimize the locking cost on the invalidation fast path (reader of the invalidation array), the array is managed with RCU.

Provide a set of APIs to add/delete entries to/from an array, which cover cannot-fail attach cases, e.g. attaching to arm_smmu_blocked_domain. Also add kunit coverage for those APIs.

Signed-off-by Jason Gunthorpe <[email protected]>

Reviewed-by Jason Gunthorpe <[email protected]>

Co-developed-by Nicolin Chen <[email protected]>

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

---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  98 +++++++
 .../iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c  |  92 +++++++
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 258 ++++++++++++++++++
 3 files changed, 448 insertions(+)

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

index 96a23ca633cb..a9441dc9070e 100644
@@ -649,6 +649,93 @@ struct arm_smmu_cmdq_batch {
 	int				num;
 };
 
+/*
+ * The order here also determines the sequence in which commands are sent to the
+ * command queue. E.g. TLBI must be done before ATC_INV.
+ */
+enum arm_smmu_inv_type {
+	INV_TYPE_S1_ASID,
+	INV_TYPE_S2_VMID,
+	INV_TYPE_S2_VMID_S1_CLEAR,
+	INV_TYPE_ATS,
+	INV_TYPE_ATS_FULL,
+};
+
+struct arm_smmu_inv {
+	struct arm_smmu_device *smmu;

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

+static void arm_smmu_v3_invs_test(struct kunit *test)
+{
+	const int results1[2][3] = { { 1, 1, 3, }, { 1, 1, 1, }, };
+	const int results2[2][5] = { { 1, 1, 3, 4, 5, }, { 2, 1, 1, 1, 1, }, };
+	const int results3[2][3] = { { 1, 1, 3, }, { 1, 1, 1, }, };
+	const int results4[2][5] = { { 1, 1, 3, 5, 6, }, { 2, 1, 1, 1, 1, }, };
+	const int results5[2][5] = { { 1, 1, 3, 5, 6, }, { 1, 0, 0, 1, 1, }, };
+	const int results6[2][3] = { { 1, 5, 6, }, { 1, 1, 1, }, };
+	struct arm_smmu_invs *test_a, *test_b;
+
+	/* New array */
+	test_a = arm_smmu_invs_alloc(0);
+	KUNIT_EXPECT_EQ(test, test_a->num_invs, 0);
+
+	/* Test1: merge invs1 (new array) */
+	test_b = arm_smmu_invs_merge(test_a, &invs1);
+	kfree(test_a);
+	arm_smmu_v3_invs_test_verify(test, test_b, ARRAY_SIZE(results1[0]),
+				     results1[0], results1[1]);
+
+	/* Test2: merge invs2 (new array) */
+	test_a = arm_smmu_invs_merge(test_b, &invs2);
+	kfree(test_b);
+	arm_smmu_v3_invs_test_verify(test, test_a, ARRAY_SIZE(results2[0]),
+				     results2[0], results2[1]);
+
+	/* Test3: unref invs2 (same array) */
+	arm_smmu_invs_unref(test_a, &invs2, NULL);
+	arm_smmu_v3_invs_test_verify(test, test_a, ARRAY_SIZE(results3[0]),
+				     results3[0], results3[1]);
+	KUNIT_EXPECT_EQ(test, test_a->num_trashes, 0);
+
+	/* Test4: merge invs3 (new array) */
+	test_b = arm_smmu_invs_merge(test_a, &invs3);
+	kfree(test_a);
+	arm_smmu_v3_invs_test_verify(test, test_b, ARRAY_SIZE(results4[0]),
+				     results4[0], results4[1]);
+
+	/* Test5: unref invs1 (same array) */
+	arm_smmu_invs_unref(test_b, &invs1, NULL);
+	arm_smmu_v3_invs_test_verify(test, test_b, ARRAY_SIZE(results5[0]),
+				     results5[0], results5[1]);
+	KUNIT_EXPECT_EQ(test, test_b->num_trashes, 2);
+

Quoted 9 lines · Nicolin Chen

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

+struct arm_smmu_inv {
+	struct arm_smmu_device *smmu;
+	u8 type;
+	u8 size_opcode;
+	u8 nsize_opcode;
+	u32 id; /* ASID or VMID or SID */
+	union {
+		size_t pgsize; /* ARM_SMMU_FEAT_RANGE_INV */
+		u32 ssid; /* INV_TYPE_ATS */

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h struct arm_smmu_cmdq_batch { · line 675

Quoted 3 lines

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

+	};
+
+	refcount_t users; /* users=0 to mark as a trash to be purged */

The refcount_t API uses atomics with barrier semantics. Do we actually need those properties when updating the refcounts here? The ASID lock gives us pretty strong serialisation even after this patch series and we rely heavily on that.

Quoted 5 lines

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

+	/* Test6: purge test_b (new array) */
+	test_a = arm_smmu_invs_purge(test_b);
+	kfree(test_b);
+	arm_smmu_v3_invs_test_verify(test, test_a, ARRAY_SIZE(results6[0]),
+				     results6[0], results6[1]);

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c static void arm_smmu_v3_write_cd_test_sva_release(struct kunit *test) · line 656

Quoted 5 lines

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

+
+	/* Test7: unref invs3 (same array) */
+	arm_smmu_invs_unref(test_a, &invs3, NULL);
+	KUNIT_EXPECT_EQ(test, test_a->num_invs, 0);
+	KUNIT_EXPECT_EQ(test, test_a->num_trashes, 0);

Wouldn't we be better off testing num_trashes == 0 after test 6 has completed?

Quoted 4 lines

+/**
+ * arm_smmu_invs_for_each_entry - Iterate over two sorted arrays computing for
+ * arm_smmu_invs_merge() or arm_smmu_invs_unref()
+ * @invs_l: the base invalidation array

Quoted 6 lines

+ * @idx_l: a stack variable of 'size_t', to store the base array index
+ * @invs_r: the build_invs array as to_merge or to_unref
+ * @idx_r: a stack variable of 'size_t', to store the build_invs index
+ * @cmp: a stack variable of 'int', to store return value (-1, 0, or 1)
+ */
+#define arm_smmu_invs_for_each_cmp(invs_l, idx_l, invs_r, idx_r, cmp) \

nit: the kerneldoc comment doesn't match the name of this function.

Quoted 10 lines · Will Deacon

Quoted 9 lines · Nicolin Chen

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

+struct arm_smmu_inv {
+	struct arm_smmu_device *smmu;
+	u8 type;
+	u8 size_opcode;
+	u8 nsize_opcode;
+	u32 id; /* ASID or VMID or SID */
+	union {
+		size_t pgsize; /* ARM_SMMU_FEAT_RANGE_INV */
+		u32 ssid; /* INV_TYPE_ATS */

Quoted 8 lines

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h struct arm_smmu_cmdq_batch { · line 675

Quoted 3 lines

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

+	};
+
+	refcount_t users; /* users=0 to mark as a trash to be purged */

The refcount_t API uses atomics with barrier semantics. Do we actually need those properties when updating the refcounts here? The ASID lock gives us pretty strong serialisation even after this patch series and we rely heavily on that.

But we can't use that mutex in the invalidation function that might be an IRQ context?

Quoted 20 lines · Nicolin Chen

Quoted 18 lines · Will Deacon

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h struct arm_smmu_cmdq_batch { · line 675

Quoted 12 lines · Nicolin Chen

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

+struct arm_smmu_inv {
+	struct arm_smmu_device *smmu;
+	u8 type;
+	u8 size_opcode;
+	u8 nsize_opcode;
+	u32 id; /* ASID or VMID or SID */
+	union {
+		size_t pgsize; /* ARM_SMMU_FEAT_RANGE_INV */
+		u32 ssid; /* INV_TYPE_ATS */
+	};
+
+	refcount_t users; /* users=0 to mark as a trash to be purged */

The refcount_t API uses atomics with barrier semantics. Do we actually need those properties when updating the refcounts here? The ASID lock gives us pretty strong serialisation even after this patch series and we rely heavily on that.

Quoted 2 lines

But we can't use that mutex in the invalidation function that might be an IRQ context?

My question, really, is why do you need the atomic properties in this patch series? It just looks like overhead at the moment.

Quoted 21 lines · Will Deacon

Quoted 20 lines · Nicolin Chen

Quoted 18 lines · Will Deacon

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h struct arm_smmu_cmdq_batch { · line 675

Quoted 12 lines · Nicolin Chen

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

+struct arm_smmu_inv {
+	struct arm_smmu_device *smmu;
+	u8 type;
+	u8 size_opcode;
+	u8 nsize_opcode;
+	u32 id; /* ASID or VMID or SID */
+	union {
+		size_t pgsize; /* ARM_SMMU_FEAT_RANGE_INV */
+		u32 ssid; /* INV_TYPE_ATS */
+	};
+
+	refcount_t users; /* users=0 to mark as a trash to be purged */

The refcount_t API uses atomics with barrier semantics. Do we actually need those properties when updating the refcounts here? The ASID lock gives us pretty strong serialisation even after this patch series and we rely heavily on that.

Quoted 5 lines

Quoted 2 lines

But we can't use that mutex in the invalidation function that might be an IRQ context?

My question, really, is why do you need the atomic properties in this patch series? It just looks like overhead at the moment.

Hmm, shouldn't it be atomic, since..

(might be IRQ, no mutex) __arm_smmu_domain_inv_range() reads it. (mutex protected) arm_smmu_attach_dev() writes it.

..?

Quoted 28 lines · Nicolin Chen

Quoted 21 lines · Will Deacon

Quoted 20 lines · Nicolin Chen

Quoted 18 lines · Will Deacon

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h struct arm_smmu_cmdq_batch { · line 675

Quoted 12 lines · Nicolin Chen

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

+struct arm_smmu_inv {
+	struct arm_smmu_device *smmu;
+	u8 type;
+	u8 size_opcode;
+	u8 nsize_opcode;
+	u32 id; /* ASID or VMID or SID */
+	union {
+		size_t pgsize; /* ARM_SMMU_FEAT_RANGE_INV */
+		u32 ssid; /* INV_TYPE_ATS */
+	};
+
+	refcount_t users; /* users=0 to mark as a trash to be purged */

The refcount_t API uses atomics with barrier semantics. Do we actually need those properties when updating the refcounts here? The ASID lock gives us pretty strong serialisation even after this patch series and we rely heavily on that.

Quoted 5 lines

Quoted 2 lines

But we can't use that mutex in the invalidation function that might be an IRQ context?

My question, really, is why do you need the atomic properties in this patch series? It just looks like overhead at the moment.

Quoted 6 lines

Hmm, shouldn't it be atomic, since..

(might be IRQ, no mutex) __arm_smmu_domain_inv_range() reads it. (mutex protected) arm_smmu_attach_dev() writes it.

..?

It doesn't need to be a full atomic, you can use WRITE_ONCE/READ_ONCE instead for this case.

The general argument of this scheme is it doesn't matter if the invalidation side is doing extra invalidation, that is supposed to always be safe except for ATS. For ATS we hold locks and then it doesn't to be atomic because of the lock.

So I think Will has it right, just use READ_ONCE/WRITE_ONCE on a naked unsigned int.

Quoted 28 lines · Nicolin Chen

Quoted 21 lines · Will Deacon

Quoted 20 lines · Nicolin Chen

Quoted 18 lines · Will Deacon

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h struct arm_smmu_cmdq_batch { · line 675

Quoted 12 lines · Nicolin Chen

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

+struct arm_smmu_inv {
+	struct arm_smmu_device *smmu;
+	u8 type;
+	u8 size_opcode;
+	u8 nsize_opcode;
+	u32 id; /* ASID or VMID or SID */
+	union {
+		size_t pgsize; /* ARM_SMMU_FEAT_RANGE_INV */
+		u32 ssid; /* INV_TYPE_ATS */
+	};
+
+	refcount_t users; /* users=0 to mark as a trash to be purged */

The refcount_t API uses atomics with barrier semantics. Do we actually need those properties when updating the refcounts here? The ASID lock gives us pretty strong serialisation even after this patch series and we rely heavily on that.

Quoted 5 lines

Quoted 2 lines

But we can't use that mutex in the invalidation function that might be an IRQ context?

My question, really, is why do you need the atomic properties in this patch series? It just looks like overhead at the moment.

Quoted 6 lines

Hmm, shouldn't it be atomic, since..

(might be IRQ, no mutex) __arm_smmu_domain_inv_range() reads it. (mutex protected) arm_smmu_attach_dev() writes it.

..?

It doesn't need to be a full atomic, you can use WRITE_ONCE/READ_ONCE instead for this case.

The general argument of this scheme is it doesn't matter if the invalidation side is doing extra invalidation, that is supposed to always be safe except for ATS. For ATS we hold locks and then it doesn't to be atomic because of the lock.

So I think Will has it right, just use READ_ONCE/WRITE_ONCE on a naked unsigned int.

There's the usual scary ordering worries with this stuff, but they exist with refcount_read() too. We're ok because cur->users is address dependent on the read of the invalidation array pointer, which is RCU protected.

(I think!)

When a master is attached from an old domain to a new domain, it needs to build an invalidation array to delete and add the array entries from/onto the invalidation arrays of those two domains, passed via the to_merge and to_unref arguments into arm_smmu_invs_merge/unref() respectively.

Since the master->num_streams might differ across masters, a memory would have to be allocated when building an to_merge/to_unref array which might fail with -ENOMEM.

On the other hand, an attachment to arm_smmu_blocked_domain must not fail so it's the best to avoid any memory allocation in that path.

Pre-allocate a fixed size invalidation array for every master. This array will be used as a scratch to fill dynamically when building a to_merge or to_unref invs array. Sort fwspec->ids in an ascending order to fit to the arm_smmu_invs_merge() function.

Co-developed-by Jason Gunthorpe <[email protected]>

Signed-off-by Jason Gunthorpe <[email protected]>

Reviewed-by Jason Gunthorpe <[email protected]>

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

---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  8 ++++
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 41 +++++++++++++++++++--
 2 files changed, 45 insertions(+), 4 deletions(-)

Each smmu_domain now has an arm_smmu_invs that specifies the invalidation steps to perform after any change the IOPTEs. This includes supports for basic ASID/VMID, the special case for nesting, and ATC invalidations.

Introduce a new arm_smmu_domain_inv helper iterating smmu_domain->invs to convert the invalidation array to commands. Any invalidation request with no size specified means an entire flush over a range based one.

Take advantage of the sorted array to compatible batch operations together to the same SMMU. For instance, ATC invaliations for multiple SIDs can be pushed as a batch.

ATC invalidations must be completed before the driver disables ATS. Or the device is permitted to ignore any racing invalidation that would cause an SMMU timeout. The sequencing is done with a rwlock where holding the write side of the rwlock means that there are no outstanding ATC invalidations. If ATS is not used the rwlock is ignored, similar to the existing code.

Co-developed-by Jason Gunthorpe <[email protected]>

Signed-off-by Jason Gunthorpe <[email protected]>

Reviewed-by Jason Gunthorpe <[email protected]>

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

---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |   9 +
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 258 +++++++++++++++++++-
 2 files changed, 254 insertions(+), 13 deletions(-)

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

index f8dc96476c43..c3fee7f14480 100644
@@ -1086,6 +1086,15 @@ void arm_smmu_tlb_inv_range_asid(unsigned long iova, size_t size, int asid,
 int arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain,
 			    unsigned long iova, size_t size);
 
+void arm_smmu_domain_inv_range(struct arm_smmu_domain *smmu_domain,
+			       unsigned long iova, size_t size,
+			       unsigned int granule, bool leaf);
+
+static inline void arm_smmu_domain_inv(struct arm_smmu_domain *smmu_domain)
+{
+	arm_smmu_domain_inv_range(smmu_domain, 0, 0, 0, false);
+}
+
 void __arm_smmu_cmdq_skip_err(struct arm_smmu_device *smmu,
 			      struct arm_smmu_cmdq *cmdq);
 int arm_smmu_init_one_queue(struct arm_smmu_device *smmu,

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +192 -13

index fb45359680d2..6e1082e6d164 100644
@@ -2516,23 +2516,19 @@ static void arm_smmu_tlb_inv_context(void *cookie)
 	arm_smmu_atc_inv_domain(smmu_domain, 0, 0);
 }
 
-static void __arm_smmu_tlb_inv_range(struct arm_smmu_cmdq_ent *cmd,
-				     unsigned long iova, size_t size,
-				     size_t granule,
-				     struct arm_smmu_domain *smmu_domain)
+static void arm_smmu_cmdq_batch_add_range(struct arm_smmu_device *smmu,
+					  struct arm_smmu_cmdq_batch *cmds,
+					  struct arm_smmu_cmdq_ent *cmd,
+					  unsigned long iova, size_t size,
+					  size_t granule, size_t pgsize)
 {
-	struct arm_smmu_device *smmu = smmu_domain->smmu;
-	unsigned long end = iova + size, num_pages = 0, tg = 0;
+	unsigned long end = iova + size, num_pages = 0, tg = pgsize;
 	size_t inv_range = granule;
-	struct arm_smmu_cmdq_batch cmds;
 
 	if (!size)
 		return;
 
 	if (smmu->features & ARM_SMMU_FEAT_RANGE_INV) {
-		/* Get the leaf page size */
-		tg = __ffs(smmu_domain->domain.pgsize_bitmap);
-
 		num_pages = size >> tg;
 
 		/* Convert page size of 12,14,16 (log2) to 1,2,3 */

@@ -2552,8 +2548,6 @@ static void __arm_smmu_tlb_inv_range(struct arm_smmu_cmdq_ent *cmd,
 			num_pages++;
 	}
 
-	arm_smmu_cmdq_batch_init(smmu, &cmds, cmd);
-
 	while (iova < end) {
 		if (smmu->features & ARM_SMMU_FEAT_RANGE_INV) {
 			/*

@@ -2581,9 +2575,26 @@ static void __arm_smmu_tlb_inv_range(struct arm_smmu_cmdq_ent *cmd,
 		}
 
 		cmd->tlbi.addr = iova;
-		arm_smmu_cmdq_batch_add(smmu, &cmds, cmd);
+		arm_smmu_cmdq_batch_add(smmu, cmds, cmd);
 		iova += inv_range;
 	}
+}
+
+static void __arm_smmu_tlb_inv_range(struct arm_smmu_cmdq_ent *cmd,
+				     unsigned long iova, size_t size,
+				     size_t granule,
+				     struct arm_smmu_domain *smmu_domain)
+{
+	struct arm_smmu_device *smmu = smmu_domain->smmu;
+	struct arm_smmu_cmdq_batch cmds;
+	size_t pgsize;
+
+	/* Get the leaf page size */
+	pgsize = __ffs(smmu_domain->domain.pgsize_bitmap);
+
+	arm_smmu_cmdq_batch_init(smmu, &cmds, cmd);
+	arm_smmu_cmdq_batch_add_range(smmu, &cmds, cmd, iova, size, granule,
+				      pgsize);
 	arm_smmu_cmdq_batch_submit(smmu, &cmds);
 }
 

@@ -2639,6 +2650,193 @@ void arm_smmu_tlb_inv_range_asid(unsigned long iova, size_t size, int asid,
 	__arm_smmu_tlb_inv_range(&cmd, iova, size, granule, smmu_domain);
 }
 
+static bool arm_smmu_inv_size_too_big(struct arm_smmu_device *smmu, size_t size,
+				      size_t granule)
+{
+	size_t max_tlbi_ops;
+
+	/* 0 size means invalidate all */
+	if (!size || size == SIZE_MAX)
+		return true;
+
+	if (smmu->features & ARM_SMMU_FEAT_RANGE_INV)
+		return false;
+
+	/*
+	 * Borrowed from the MAX_TLBI_OPS in arch/arm64/include/asm/tlbflush.h,
+	 * this is used as a threshold to replace "size_opcode" commands with a
+	 * single "nsize_opcode" command, when SMMU doesn't implement the range
+	 * invalidation feature, where there can be too many per-granule TLBIs,
+	 * resulting in a soft lockup.
+	 */
+	max_tlbi_ops = 1 << (ilog2(granule) - 3);
+	return size >= max_tlbi_ops * granule;
+}
+
+/* Used by non INV_TYPE_ATS* invalidations */
+static void arm_smmu_inv_to_cmdq_batch(struct arm_smmu_inv *inv,
+				       struct arm_smmu_cmdq_batch *cmds,
+				       struct arm_smmu_cmdq_ent *cmd,
+				       unsigned long iova, size_t size,
+				       unsigned int granule)
+{
+	if (arm_smmu_inv_size_too_big(inv->smmu, size, granule)) {
+		cmd->opcode = inv->nsize_opcode;
+		arm_smmu_cmdq_batch_add(inv->smmu, cmds, cmd);
+		return;
+	}
+
+	cmd->opcode = inv->size_opcode;
+	arm_smmu_cmdq_batch_add_range(inv->smmu, cmds, cmd, iova, size, granule,
+				      inv->pgsize);
+}
+
+static inline bool arm_smmu_invs_end_batch(struct arm_smmu_inv *cur,
+					   struct arm_smmu_inv *next)
+{
+	/* Changing smmu means changing command queue */
+	if (cur->smmu != next->smmu)
+		return true;
+	/* The batch for S2 TLBI must be done before nested S1 ASIDs */
+	if (cur->type != INV_TYPE_S2_VMID_S1_CLEAR &&
+	    next->type == INV_TYPE_S2_VMID_S1_CLEAR)
+		return true;
+	/* ATS must be after a sync of the S1/S2 invalidations */
+	if (!arm_smmu_inv_is_ats(cur) && arm_smmu_inv_is_ats(next))
+		return true;
+	return false;
+}
+
+static void __arm_smmu_domain_inv_range(struct arm_smmu_invs *invs,
+					unsigned long iova, size_t size,
+					unsigned int granule, bool leaf)
+{
+	struct arm_smmu_cmdq_batch cmds = {};
+	struct arm_smmu_inv *cur;
+	struct arm_smmu_inv *end;
+
+	cur = invs->inv;
+	end = cur + READ_ONCE(invs->num_invs);
+	/* Skip any leading entry marked as a trash */
+	for (; cur != end; cur++)
+		if (refcount_read(&cur->users))
+			break;
+	while (cur != end) {
+		struct arm_smmu_device *smmu = cur->smmu;
+		struct arm_smmu_cmdq_ent cmd = {
+			/*
+			 * Pick size_opcode to run arm_smmu_get_cmdq(). This can
+			 * be changed to nsize_opcode, which would result in the
+			 * same CMDQ pointer.
+			 */
+			.opcode = cur->size_opcode,
+		};
+		struct arm_smmu_inv *next;
+
+		if (!cmds.num)
+			arm_smmu_cmdq_batch_init(smmu, &cmds, &cmd);
+
+		switch (cur->type) {
+		case INV_TYPE_S1_ASID:
+			cmd.tlbi.asid = cur->id;
+			cmd.tlbi.leaf = leaf;
+			arm_smmu_inv_to_cmdq_batch(cur, &cmds, &cmd, iova, size,
+						   granule);
+			break;
+		case INV_TYPE_S2_VMID:
+			cmd.tlbi.vmid = cur->id;
+			cmd.tlbi.leaf = leaf;
+			arm_smmu_inv_to_cmdq_batch(cur, &cmds, &cmd, iova, size,
+						   granule);
+			break;
+		case INV_TYPE_S2_VMID_S1_CLEAR:
+			/* CMDQ_OP_TLBI_S12_VMALL already flushed S1 entries */
+			if (arm_smmu_inv_size_too_big(cur->smmu, size, granule))
+				continue;
+			cmd.tlbi.vmid = cur->id;
+			arm_smmu_cmdq_batch_add(smmu, &cmds, &cmd);
+			break;
+		case INV_TYPE_ATS:
+			arm_smmu_atc_inv_to_cmd(cur->ssid, iova, size, &cmd);
+			cmd.atc.sid = cur->id;
+			arm_smmu_cmdq_batch_add(smmu, &cmds, &cmd);
+			break;
+		case INV_TYPE_ATS_FULL:
+			arm_smmu_atc_inv_to_cmd(IOMMU_NO_PASID, 0, 0, &cmd);
+			cmd.atc.sid = cur->id;
+			arm_smmu_cmdq_batch_add(smmu, &cmds, &cmd);
+			break;
+		default:
+			WARN_ON_ONCE(1);
+			continue;
+		}
+
+		/* Skip any trash entry in-between */
+		for (next = cur + 1; next != end; next++)
+			if (refcount_read(&next->users))
+				break;
+
+		if (cmds.num &&
+		    (next == end || arm_smmu_invs_end_batch(cur, next))) {
+			arm_smmu_cmdq_batch_submit(smmu, &cmds);
+			cmds.num = 0;
+		}
+		cur = next;
+	}
+}
+
+void arm_smmu_domain_inv_range(struct arm_smmu_domain *smmu_domain,
+			       unsigned long iova, size_t size,
+			       unsigned int granule, bool leaf)
+{
+	struct arm_smmu_invs *invs;
+
+	/*
+	 * An invalidation request must follow some IOPTE change and then load
+	 * an invalidation array. In the meantime, a domain attachment mutates
+	 * the array and then stores an STE/CD asking SMMU HW to acquire those
+	 * changed IOPTEs. In other word, these two are interdependent and can
+	 * race.
+	 *
+	 * In a race, the RCU design (with its underlying memory barriers) can
+	 * ensure the invalidation array to always get updated before loaded.
+	 *
+	 * smp_mb() is used here, paired with the smp_mb() following the array
+	 * update in a concurrent attach, to ensure:
+	 *  - HW sees the new IOPTEs if it walks after STE installation
+	 *  - Invalidation thread sees the updated array with the new ASID.
+	 *
+	 *  [CPU0]                        | [CPU1]
+	 *                                |
+	 *  change IOPTEs and TLB flush:  |
+	 *  arm_smmu_domain_inv_range() { | arm_smmu_install_new_domain_invs {
+	 *    ...                         |   rcu_assign_pointer(new_invs);
+	 *    smp_mb(); // ensure IOPTEs  |   smp_mb(); // ensure new_invs
+	 *    ...                         |   kfree_rcu(old_invs, rcu);
+	 *    // load invalidation array  | }
+	 *    invs = rcu_dereference();   | arm_smmu_install_ste_for_dev {
+	 *                                |   STE = TTB0 // read new IOPTEs
+	 */
+	smp_mb();

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

+		__arm_smmu_domain_inv_range(invs, iova, size, granule, leaf);
+		read_unlock(&invs->rwlock);
+	} else {
+		__arm_smmu_domain_inv_range(invs, iova, size, granule, leaf);
+	}
+
+	rcu_read_unlock();
+}
+
 static void arm_smmu_tlb_inv_page_nosync(struct iommu_iotlb_gather *gather,
 					 unsigned long iova, size_t granule,
 					 void *cookie)

@@ -3285,6 +3483,23 @@ arm_smmu_install_new_domain_invs(struct arm_smmu_attach_state *state)
 		return;
 
 	rcu_assign_pointer(*invst->invs_ptr, invst->new_invs);
+	/*
+	 * We are committed to updating the STE. Ensure the invalidation array
+	 * is visible to concurrent map/unmap threads, and acquire any racing
+	 * IOPTE updates.
+	 *
+	 *  [CPU0]                        | [CPU1]
+	 *                                |
+	 *  change IOPTEs and TLB flush:  |
+	 *  arm_smmu_domain_inv_range() { | arm_smmu_install_new_domain_invs {
+	 *    ...                         |   rcu_assign_pointer(new_invs);
+	 *    smp_mb(); // ensure IOPTEs  |   smp_mb(); // ensure new_invs
+	 *    ...                         |   kfree_rcu(old_invs, rcu);
+	 *    // load invalidation array  | }
+	 *    invs = rcu_dereference();   | arm_smmu_install_ste_for_dev {
+	 *                                |   STE = TTB0 // read new IOPTEs
+	 */
+	smp_mb();
 	kfree_rcu(invst->old_invs, rcu);
 }
 

@@ -3334,6 +3549,23 @@ arm_smmu_install_old_domain_invs(struct arm_smmu_attach_state *state)
 		return;
 
 	rcu_assign_pointer(*invst->invs_ptr, new_invs);
+	/*
+	 * We are committed to updating the STE. Ensure the invalidation array
+	 * is visible to concurrent map/unmap threads, and acquire any racing
+	 * IOPTE updates.
+	 *
+	 *  [CPU0]                        | [CPU1]
+	 *                                |
+	 *  change IOPTEs and TLB flush:  |
+	 *  arm_smmu_domain_inv_range() { | arm_smmu_install_old_domain_invs {
+	 *    ...                         |   rcu_assign_pointer(new_invs);

For INV_TYPE_S1_ASID, the new code loops and checks size_too_big via arm_smmu_inv_to_cmdq_batch. However, for INV_TYPE_ATS, it issues a single command for the entire range. While this matches the current driver, are we confident arm_smmu_atc_inv_to_cmd handles all massive sizes correctly without needing a similar loop or "too big" fallback?

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c void arm_smmu_tlb_inv_range_asid(unsigned long iova, size_t size, int asid, · line 2830

Quoted 6 lines · Nicolin Chen

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

+	/*
+	 * Avoid locking unless ATS is being used. No ATC invalidation can be
+	 * going on after a domain is detached.
+	 */
+	if (invs->has_ats) {
+		read_lock(&invs->rwlock);

Shouldn't these be read_lock_irqsave for all rwlock variants here? Invalidations might happen in IRQ context as well..

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c void arm_smmu_tlb_inv_range_asid(unsigned long iova, size_t size, int asid, · line 2832

Quoted 2 lines

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

+		__arm_smmu_domain_inv_range(invs, iova, size, granule, leaf);
+		read_unlock(&invs->rwlock);

Hi Pranjal,

Sorry, I missed this!

Quoted 11 lines · Pranjal Shrivastava

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c void arm_smmu_tlb_inv_range_asid(unsigned long iova, size_t size, int asid, · line 2830

Quoted 6 lines · Nicolin Chen

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

+	/*
+	 * Avoid locking unless ATS is being used. No ATC invalidation can be
+	 * going on after a domain is detached.
+	 */
+	if (invs->has_ats) {
+		read_lock(&invs->rwlock);

Shouldn't these be read_lock_irqsave for all rwlock variants here? Invalidations might happen in IRQ context as well..

Quoted 2 lines

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c void arm_smmu_tlb_inv_range_asid(unsigned long iova, size_t size, int asid, · line 2832

Quoted 2 lines

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

+		__arm_smmu_domain_inv_range(invs, iova, size, granule, leaf);
+		read_unlock(&invs->rwlock);

It was kept from the older versions where we had a trylock. Jason had an insight about this, mainly for less latency on invalidation threads.

Yet, now we have a plain locking. TBH, I can't find a good reason justifying this. And it does look a bit unsafe to me. So, I think I will just change to the _irqsave version. (Jason?)

Quoted 23 lines · Nicolin Chen

Hi Pranjal,

Sorry, I missed this!

Quoted 11 lines · Pranjal Shrivastava

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c void arm_smmu_tlb_inv_range_asid(unsigned long iova, size_t size, int asid, · line 2830

Quoted 6 lines · Nicolin Chen

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

+	/*
+	 * Avoid locking unless ATS is being used. No ATC invalidation can be
+	 * going on after a domain is detached.
+	 */
+	if (invs->has_ats) {
+		read_lock(&invs->rwlock);

Shouldn't these be read_lock_irqsave for all rwlock variants here? Invalidations might happen in IRQ context as well..

Quoted 2 lines

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c void arm_smmu_tlb_inv_range_asid(unsigned long iova, size_t size, int asid, · line 2832

Quoted 2 lines

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

+		__arm_smmu_domain_inv_range(invs, iova, size, granule, leaf);
+		read_unlock(&invs->rwlock);

It was kept from the older versions where we had a trylock. Jason had an insight about this, mainly for less latency on invalidation threads.

Quoted 3 lines

Yet, now we have a plain locking. TBH, I can't find a good reason justifying this. And it does look a bit unsafe to me. So, I think I will just change to the _irqsave version. (Jason?)

My understanding has been that this invalidation can run from an IRQ context - we permit the use of the DMA API from an interrupt handler?

I though that for rwsem the read side does not require the _irqsave, even if it is in an irq context, unless the write side runs from an IRQ.

Here the write side always runs from a process context.

So the write side will block the IRQ which ensures we don't spin during read in an IRQ.

Quoted 6 lines

My understanding has been that this invalidation can run from an IRQ context - we permit the use of the DMA API from an interrupt handler?

I though that for rwsem the read side does not require the _irqsave, even if it is in an irq context, unless the write side runs from an IRQ.

Hmm, is "rwsem" a typo? Because it's rwlock_t, which is spinlock :-/

Quoted 4 lines

Here the write side always runs from a process context.

So the write side will block the IRQ which ensures we don't spin during read in an IRQ.

And, does write_lock_irqsave() disable global IRQ or local IRQ only?

Documentation/locking/locktypes.rst mentions "local_irq_disable()"..

Quoted 8 lines · Nicolin Chen

Quoted 6 lines

My understanding has been that this invalidation can run from an IRQ context - we permit the use of the DMA API from an interrupt handler?

I though that for rwsem the read side does not require the _irqsave, even if it is in an irq context, unless the write side runs from an IRQ.

Hmm, is "rwsem" a typo? Because it's rwlock_t, which is spinlock :-/

Yeah, sorry

Quoted 7 lines

Quoted 4 lines

Here the write side always runs from a process context.

So the write side will block the IRQ which ensures we don't spin during read in an IRQ.

And, does write_lock_irqsave() disable global IRQ or local IRQ only?

Quoted 1 line

Documentation/locking/locktypes.rst mentions "local_irq_disable()"..

It will only disable the local IRQ, since it is a spin type lock an IRQ on another CPU can spin until it is unlocked.

The main issue is if this CPU takes an IRQ while the write side is locked and spins, then it will never unlock.

Quoted 21 lines · Jason Gunthorpe

Quoted 8 lines · Nicolin Chen

Quoted 6 lines

My understanding has been that this invalidation can run from an IRQ context - we permit the use of the DMA API from an interrupt handler?

I though that for rwsem the read side does not require the _irqsave, even if it is in an irq context, unless the write side runs from an IRQ.

Hmm, is "rwsem" a typo? Because it's rwlock_t, which is spinlock :-/

Yeah, sorry

Quoted 8 lines

Quoted 4 lines

Here the write side always runs from a process context.

So the write side will block the IRQ which ensures we don't spin during read in an IRQ.

And, does write_lock_irqsave() disable global IRQ or local IRQ only?

Documentation/locking/locktypes.rst mentions "local_irq_disable()"..

Quoted 5 lines

It will only disable the local IRQ, since it is a spin type lock an IRQ on another CPU can spin until it is unlocked.

The main issue is if this CPU takes an IRQ while the write side is locked and spins, then it will never unlock.

Yea, that sounds unsafe. I'll send a v11 with read_lock_irqsave().

Quoted 25 lines · Nicolin Chen

Quoted 24 lines · Jason Gunthorpe

Quoted 8 lines · Nicolin Chen

Quoted 6 lines

My understanding has been that this invalidation can run from an IRQ context - we permit the use of the DMA API from an interrupt handler?

I though that for rwsem the read side does not require the _irqsave, even if it is in an irq context, unless the write side runs from an IRQ.

Hmm, is "rwsem" a typo? Because it's rwlock_t, which is spinlock :-/

Yeah, sorry

Quoted 8 lines

Quoted 4 lines

Here the write side always runs from a process context.

So the write side will block the IRQ which ensures we don't spin during read in an IRQ.

And, does write_lock_irqsave() disable global IRQ or local IRQ only?

Documentation/locking/locktypes.rst mentions "local_irq_disable()"..

It will only disable the local IRQ, since it is a spin type lock an IRQ on another CPU can spin until it is unlocked.

Quoted 4 lines

Quoted 2 lines

The main issue is if this CPU takes an IRQ while the write side is locked and spins, then it will never unlock.

Yea, that sounds unsafe. I'll send a v11 with read_lock_irqsave().

I'm explaining why it is safe now, the write side takes the irqsave so the above can't happen.

There is no case where the read side needs to block IRQ because if the read side succeeds, an IRQ happens and tries to take another read side, it will succeed not spin.

And I don't see any non-hackable way for CONFIG_QUEUED_RWLOCKS=n unless CONFIG_PREEMPT_RT=y, which would be a different ball game that I assume SMMUv3 might not be completely compatible with.

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

+void arm_smmu_domain_inv_range(struct arm_smmu_domain *smmu_domain,
+			       unsigned long iova, size_t size,
+			       unsigned int granule, bool leaf)
+{
+	struct arm_smmu_invs *invs;
+
+	/*
+	 * An invalidation request must follow some IOPTE change and then load
+	 * an invalidation array. In the meantime, a domain attachment mutates
+	 * the array and then stores an STE/CD asking SMMU HW to acquire those
+	 * changed IOPTEs. In other word, these two are interdependent and can
+	 * race.
+	 *
+	 * In a race, the RCU design (with its underlying memory barriers) can
+	 * ensure the invalidation array to always get updated before loaded.
+	 *
+	 * smp_mb() is used here, paired with the smp_mb() following the array
+	 * update in a concurrent attach, to ensure:
+	 *  - HW sees the new IOPTEs if it walks after STE installation
+	 *  - Invalidation thread sees the updated array with the new ASID.
+	 *
+	 *  [CPU0]                        | [CPU1]
+	 *                                |
+	 *  change IOPTEs and TLB flush:  |
+	 *  arm_smmu_domain_inv_range() { | arm_smmu_install_new_domain_invs {
+	 *    ...                         |   rcu_assign_pointer(new_invs);
+	 *    smp_mb(); // ensure IOPTEs  |   smp_mb(); // ensure new_invs
+	 *    ...                         |   kfree_rcu(old_invs, rcu);
+	 *    // load invalidation array  | }
+	 *    invs = rcu_dereference();   | arm_smmu_install_ste_for_dev {
+	 *                                |   STE = TTB0 // read new IOPTEs
+	 */
+	smp_mb();
+
+	rcu_read_lock();
+	invs = rcu_dereference(smmu_domain->invs);
+
+	/*
+	 * Avoid locking unless ATS is being used. No ATC invalidation can be
+	 * going on after a domain is detached.
+	 */
+	if (invs->has_ats) {
+		read_lock(&invs->rwlock);
+		__arm_smmu_domain_inv_range(invs, iova, size, granule, leaf);
+		read_unlock(&invs->rwlock);
+	} else {
+		__arm_smmu_domain_inv_range(invs, iova, size, granule, leaf);
+	}
+
+	rcu_read_unlock();
+}
+
 static void arm_smmu_tlb_inv_page_nosync(struct iommu_iotlb_gather *gather,
 					 unsigned long iova, size_t granule,
 					 void *cookie)

@@ -3285,6 +3483,23 @@ arm_smmu_install_new_domain_invs(struct arm_smmu_attach_state *state)
 		return;
 
 	rcu_assign_pointer(*invst->invs_ptr, invst->new_invs);
+	/*
+	 * We are committed to updating the STE. Ensure the invalidation array
+	 * is visible to concurrent map/unmap threads, and acquire any racing
+	 * IOPTE updates.
+	 *
+	 *  [CPU0]                        | [CPU1]
+	 *                                |
+	 *  change IOPTEs and TLB flush:  |
+	 *  arm_smmu_domain_inv_range() { | arm_smmu_install_new_domain_invs {
+	 *    ...                         |   rcu_assign_pointer(new_invs);
+	 *    smp_mb(); // ensure IOPTEs  |   smp_mb(); // ensure new_invs
+	 *    ...                         |   kfree_rcu(old_invs, rcu);
+	 *    // load invalidation array  | }
+	 *    invs = rcu_dereference();   | arm_smmu_install_ste_for_dev {
+	 *                                |   STE = TTB0 // read new IOPTEs
+	 */
+	smp_mb();
 	kfree_rcu(invst->old_invs, rcu);
 }
 

@@ -3334,6 +3549,23 @@ arm_smmu_install_old_domain_invs(struct arm_smmu_attach_state *state)
 		return;
 
 	rcu_assign_pointer(*invst->invs_ptr, new_invs);
+	/*
+	 * We are committed to updating the STE. Ensure the invalidation array
+	 * is visible to concurrent map/unmap threads, and acquire any racing
+	 * IOPTE updates.
+	 *
+	 *  [CPU0]                        | [CPU1]
+	 *                                |

Quoted 7 lines · Nicolin Chen

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

+	/*
+	 * We are committed to updating the STE. Ensure the invalidation array
+	 * is visible to concurrent map/unmap threads, and acquire any racing
+	 * IOPTE updates.
+	 *
+	 *  [CPU0]                        | [CPU1]
+	 *                                |

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c arm_smmu_install_old_domain_invs(struct arm_smmu_attach_state *state) · line 3568

Quoted 10 lines

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

+	 *  change IOPTEs and TLB flush:  |
+	 *  arm_smmu_domain_inv_range() { | arm_smmu_install_old_domain_invs {
+	 *    ...                         |   rcu_assign_pointer(new_invs);
+	 *    smp_mb(); // ensure IOPTEs  |   smp_mb(); // ensure new_invs
+	 *    ...                         |   kfree_rcu(old_invs, rcu);
+	 *    // load invalidation array  | }
+	 *    invs = rcu_dereference();   | arm_smmu_install_ste_for_dev {
+	 *                                |   STE = TTB0 // read new IOPTEs
+	 */
+	smp_mb();

I don't think we need to duplicate this comment three times, you can just refer to the first function (e.g. "See ordering comment in arm_smmu_domain_inv_range()").

However, isn't the comment above misleading for this case? arm_smmu_install_old_domain_invs() has the sequencing the other way around on CPU 1: we should update the STE first.

Quoted 14 lines · Will Deacon

Quoted 13 lines · Nicolin Chen

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

+	/*
+	 * We are committed to updating the STE. Ensure the invalidation array
+	 * is visible to concurrent map/unmap threads, and acquire any racing
+	 * IOPTE updates.
+	 *
+	 *  [CPU0]                        | [CPU1]
+	 *                                |
+	 *  change IOPTEs and TLB flush:  |
+	 *  arm_smmu_domain_inv_range() { | arm_smmu_install_old_domain_invs {
+	 *    ...                         |   rcu_assign_pointer(new_invs);
+	 *    smp_mb(); // ensure IOPTEs  |   smp_mb(); // ensure new_invs
+	 *    ...                         |   kfree_rcu(old_invs, rcu);
+	 *    // load invalidation array  | }

Quoted 8 lines

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c void arm_smmu_tlb_inv_range_asid(unsigned long iova, size_t size, int asid, · line 2820

Quoted 4 lines

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

+	 *    invs = rcu_dereference();   | arm_smmu_install_ste_for_dev {
+	 *                                |   STE = TTB0 // read new IOPTEs
+	 */
+	smp_mb();

I don't think we need to duplicate this comment three times, you can just refer to the first function (e.g. "See ordering comment in arm_smmu_domain_inv_range()").

Quoted 3 lines

However, isn't the comment above misleading for this case? arm_smmu_install_old_domain_invs() has the sequencing the other way around on CPU 1: we should update the STE first.

I also think we probably want a dma_mb() instead of an smp_mb() for all of these examples? It won't make any practical difference but I think it helps readability given that one of the readers is the PTW.

Quoted 23 lines · Will Deacon

Quoted 7 lines · Nicolin Chen

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

+	/*
+	 * We are committed to updating the STE. Ensure the invalidation array
+	 * is visible to concurrent map/unmap threads, and acquire any racing
+	 * IOPTE updates.
+	 *
+	 *  [CPU0]                        | [CPU1]
+	 *                                |

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c arm_smmu_install_old_domain_invs(struct arm_smmu_attach_state *state) · line 3568

Quoted 10 lines

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

+	 *  change IOPTEs and TLB flush:  |
+	 *  arm_smmu_domain_inv_range() { | arm_smmu_install_old_domain_invs {
+	 *    ...                         |   rcu_assign_pointer(new_invs);
+	 *    smp_mb(); // ensure IOPTEs  |   smp_mb(); // ensure new_invs
+	 *    ...                         |   kfree_rcu(old_invs, rcu);
+	 *    // load invalidation array  | }
+	 *    invs = rcu_dereference();   | arm_smmu_install_ste_for_dev {
+	 *                                |   STE = TTB0 // read new IOPTEs
+	 */
+	smp_mb();

I don't think we need to duplicate this comment three times, you can just refer to the first function (e.g. "See ordering comment in arm_smmu_domain_inv_range()").

Quoted 3 lines

However, isn't the comment above misleading for this case? arm_smmu_install_old_domain_invs() has the sequencing the other way around on CPU 1: we should update the STE first.

I also think we probably want a dma_mb() instead of an smp_mb() for all of these examples? It won't make any practical difference but I think it helps readability given that one of the readers is the PTW.

Quoted 24 lines · Will Deacon

Quoted 23 lines · Will Deacon

Quoted 7 lines · Nicolin Chen

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

+	/*
+	 * We are committed to updating the STE. Ensure the invalidation array
+	 * is visible to concurrent map/unmap threads, and acquire any racing
+	 * IOPTE updates.
+	 *
+	 *  [CPU0]                        | [CPU1]
+	 *                                |

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c arm_smmu_install_old_domain_invs(struct arm_smmu_attach_state *state) · line 3568

Quoted 10 lines

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

+	 *  change IOPTEs and TLB flush:  |
+	 *  arm_smmu_domain_inv_range() { | arm_smmu_install_old_domain_invs {
+	 *    ...                         |   rcu_assign_pointer(new_invs);
+	 *    smp_mb(); // ensure IOPTEs  |   smp_mb(); // ensure new_invs
+	 *    ...                         |   kfree_rcu(old_invs, rcu);
+	 *    // load invalidation array  | }
+	 *    invs = rcu_dereference();   | arm_smmu_install_ste_for_dev {
+	 *                                |   STE = TTB0 // read new IOPTEs
+	 */
+	smp_mb();

I don't think we need to duplicate this comment three times, you can just refer to the first function (e.g. "See ordering comment in arm_smmu_domain_inv_range()").

Quoted 7 lines

Quoted 3 lines

However, isn't the comment above misleading for this case? arm_smmu_install_old_domain_invs() has the sequencing the other way around on CPU 1: we should update the STE first.

I also think we probably want a dma_mb() instead of an smp_mb() for all of these examples? It won't make any practical difference but I think it helps readability given that one of the readers is the PTW.

The only actual dma_wmb() is inside arm_smmu_install_ste_for_dev() after updating the STE. Adding that line explicitly would help as that is the only point where we must have the writes actually visible to the DMA HW.

The ones written here as smp_mb() are not required to be DMA ones and could all be NOP's on UP..

Quoted 33 lines · Jason Gunthorpe

Quoted 24 lines · Will Deacon

Quoted 23 lines · Will Deacon

Quoted 7 lines · Nicolin Chen

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

+	/*
+	 * We are committed to updating the STE. Ensure the invalidation array
+	 * is visible to concurrent map/unmap threads, and acquire any racing
+	 * IOPTE updates.
+	 *
+	 *  [CPU0]                        | [CPU1]
+	 *                                |

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c arm_smmu_install_old_domain_invs(struct arm_smmu_attach_state *state) · line 3568

Quoted 10 lines

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

+	 *  change IOPTEs and TLB flush:  |
+	 *  arm_smmu_domain_inv_range() { | arm_smmu_install_old_domain_invs {
+	 *    ...                         |   rcu_assign_pointer(new_invs);
+	 *    smp_mb(); // ensure IOPTEs  |   smp_mb(); // ensure new_invs
+	 *    ...                         |   kfree_rcu(old_invs, rcu);
+	 *    // load invalidation array  | }
+	 *    invs = rcu_dereference();   | arm_smmu_install_ste_for_dev {
+	 *                                |   STE = TTB0 // read new IOPTEs
+	 */
+	smp_mb();

I don't think we need to duplicate this comment three times, you can just refer to the first function (e.g. "See ordering comment in arm_smmu_domain_inv_range()").

Quoted 7 lines

Quoted 3 lines

However, isn't the comment above misleading for this case? arm_smmu_install_old_domain_invs() has the sequencing the other way around on CPU 1: we should update the STE first.

I also think we probably want a dma_mb() instead of an smp_mb() for all of these examples? It won't make any practical difference but I think it helps readability given that one of the readers is the PTW.

Quoted 7 lines

The only actual dma_wmb() is inside arm_smmu_install_ste_for_dev() after updating the STE. Adding that line explicitly would help as that is the only point where we must have the writes actually visible to the DMA HW.

The ones written here as smp_mb() are not required to be DMA ones and could all be NOP's on UP..

Hmm, I'm not sure about that.

If we've written a new (i.e. previously invalid) valid PTE to a page-table and then we install that page-table into an STE hitlessly (let's say we write the S2TTB field) then isn't there a window before we do the STE invalidation where the page-table might be accessible to the SMMU but the new PTE is still sitting in the CPU?

Quoted 42 lines · Will Deacon

Quoted 33 lines · Jason Gunthorpe

Quoted 24 lines · Will Deacon

Quoted 23 lines · Will Deacon

Quoted 7 lines · Nicolin Chen

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

+	/*
+	 * We are committed to updating the STE. Ensure the invalidation array
+	 * is visible to concurrent map/unmap threads, and acquire any racing
+	 * IOPTE updates.
+	 *
+	 *  [CPU0]                        | [CPU1]
+	 *                                |

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c arm_smmu_install_old_domain_invs(struct arm_smmu_attach_state *state) · line 3568

Quoted 10 lines

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

+	 *  change IOPTEs and TLB flush:  |
+	 *  arm_smmu_domain_inv_range() { | arm_smmu_install_old_domain_invs {
+	 *    ...                         |   rcu_assign_pointer(new_invs);
+	 *    smp_mb(); // ensure IOPTEs  |   smp_mb(); // ensure new_invs
+	 *    ...                         |   kfree_rcu(old_invs, rcu);
+	 *    // load invalidation array  | }
+	 *    invs = rcu_dereference();   | arm_smmu_install_ste_for_dev {
+	 *                                |   STE = TTB0 // read new IOPTEs
+	 */
+	smp_mb();

I don't think we need to duplicate this comment three times, you can just refer to the first function (e.g. "See ordering comment in arm_smmu_domain_inv_range()").

Quoted 7 lines

Quoted 3 lines

However, isn't the comment above misleading for this case? arm_smmu_install_old_domain_invs() has the sequencing the other way around on CPU 1: we should update the STE first.

I also think we probably want a dma_mb() instead of an smp_mb() for all of these examples? It won't make any practical difference but I think it helps readability given that one of the readers is the PTW.

Quoted 7 lines

The only actual dma_wmb() is inside arm_smmu_install_ste_for_dev() after updating the STE. Adding that line explicitly would help as that is the only point where we must have the writes actually visible to the DMA HW.

The ones written here as smp_mb() are not required to be DMA ones and could all be NOP's on UP..

Quoted 7 lines

Hmm, I'm not sure about that.

If we've written a new (i.e. previously invalid) valid PTE to a page-table and then we install that page-table into an STE hitlessly (let's say we write the S2TTB field) then isn't there a window before we do the STE invalidation where the page-table might be accessible to the SMMU but the new PTE is still sitting in the CPU?

Hmm! Yes seems like it.

However, that's seems like a general bug, if we allocate an iommu_domain and immediately hitlessly install it, then there would be no dma_wmb() for the page table memory prior to the earliest point the HW is able to read the STE.

What I wrote is is how things are intended to work, so lets fix it with this?

Quoted 17 lines · Jason Gunthorpe

Quoted 16 lines · Will Deacon

Quoted 15 lines · Jason Gunthorpe

Quoted 14 lines · Will Deacon

Quoted 13 lines · Will Deacon

Quoted 12 lines · Nicolin Chen

(quoted diff) +12 -0

+	/*
+	 * We are committed to updating the STE. Ensure the invalidation array
+	 * is visible to concurrent map/unmap threads, and acquire any racing
+	 * IOPTE updates.
+	 *
+	 *  [CPU0]                        | [CPU1]
+	 *                                |
+	 *  change IOPTEs and TLB flush:  |
+	 *  arm_smmu_domain_inv_range() { | arm_smmu_install_old_domain_invs {
+	 *    ...                         |   rcu_assign_pointer(new_invs);
+	 *    smp_mb(); // ensure IOPTEs  |   smp_mb(); // ensure new_invs
+	 *    ...                         |   kfree_rcu(old_invs, rcu);

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+	 *    // load invalidation array  | }
+	 *    invs = rcu_dereference();   | arm_smmu_install_ste_for_dev {
+	 *                                |   STE = TTB0 // read new IOPTEs
+	 */
+	smp_mb();

If we do that, can we drop the smp_mb()s from
arm_smmu_install_{old,new}_domain_invs()?

Quoted 18 lines · Will Deacon

Quoted 17 lines · Jason Gunthorpe

Quoted 16 lines · Will Deacon

Quoted 15 lines · Jason Gunthorpe

Quoted 14 lines · Will Deacon

Quoted 13 lines · Will Deacon

Quoted 12 lines · Nicolin Chen

(quoted diff) +12 -0

+	/*
+	 * We are committed to updating the STE. Ensure the invalidation array
+	 * is visible to concurrent map/unmap threads, and acquire any racing
+	 * IOPTE updates.
+	 *
+	 *  [CPU0]                        | [CPU1]
+	 *                                |
+	 *  change IOPTEs and TLB flush:  |
+	 *  arm_smmu_domain_inv_range() { | arm_smmu_install_old_domain_invs {
+	 *    ...                         |   rcu_assign_pointer(new_invs);
+	 *    smp_mb(); // ensure IOPTEs  |   smp_mb(); // ensure new_invs
+	 *    ...                         |   kfree_rcu(old_invs, rcu);

Quoted 8 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+	 *    // load invalidation array  | }
+	 *    invs = rcu_dereference();   | arm_smmu_install_ste_for_dev {
+	 *                                |   STE = TTB0 // read new IOPTEs
+	 */
+	smp_mb();

If we do that, can we drop the smp_mb()s from
arm_smmu_install_{old,new}_domain_invs()?

I suppose so, but domain attach isn't a performance path so it depends on your preference for strict pairing of barriers. Currently the two smp_mbs() are paired. Can we reliably pair smp_mb() with dma_wmb()? Are you happy with that clarity?

My view is attach isn't a performance path, so having extra barriers is fine if it helps understandability.

Quoted 8 lines · Jason Gunthorpe

Quoted 2 lines · Will Deacon

If we do that, can we drop the smp_mb()s from
arm_smmu_install_{old,new}_domain_invs()?

I suppose so, but domain attach isn't a performance path so it depends on your preference for strict pairing of barriers. Currently the two smp_mbs() are paired. Can we reliably pair smp_mb() with dma_wmb()? Are you happy with that clarity?

Yeah, I think that's ok.

Quoted 2 lines

My view is attach isn't a performance path, so having extra barriers is fine if it helps understandability.

I think that the more barriers we have, the harder the code is to understand so I would prefer just to have the smp_mb() for the write->read case in arm_smmu_domain_inv_range() along with a comment explaining why it's needed.

I think that also means that my concern about the old comments on the other patch largely disappears.

Quoted 5 lines · Will Deacon

If we've written a new (i.e. previously invalid) valid PTE to a page-table and then we install that page-table into an STE hitlessly (let's say we write the S2TTB field) then isn't there a window before we do the STE invalidation where the page-table might be accessible to the SMMU but the new PTE is still sitting in the CPU?

Hmm! Yes seems like it.

However, that's seems like a general bug, if we allocate an iommu_domain and immediately hitlessly install it, then there would be no dma_wmb() for the page table memory prior to the earliest point the HW is able to read the STE.

What I wrote is is how things are intended to work, so lets fix it with this?

(quoted diff) +0 -0

@@ -1173,6 +1173,13 @@ void arm_smmu_write_entry(struct arm_smmu_entry_writer *writer, __le64 *entry,
        __le64 unused_update[NUM_ENTRY_QWORDS];
        u8 used_qword_diff;
 

Replace the old invalidation functions with arm_smmu_domain_inv_range() in all the existing invalidation routines. And deprecate the old functions.

The new arm_smmu_domain_inv_range() handles the CMDQ_MAX_TLBI_OPS as well, so drop it in the SVA function.

Since arm_smmu_cmdq_batch_add_range() has only one caller now, and it must be given a valid size, add a WARN_ON_ONCE to catch any missed case.

Reviewed-by Jason Gunthorpe <[email protected]>

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

---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |   7 -
 .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c   |  29 +--
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 165 +-----------------
 3 files changed, 11 insertions(+), 190 deletions(-)

Quoted 9 lines · Nicolin Chen

Replace the old invalidation functions with arm_smmu_domain_inv_range() in all the existing invalidation routines. And deprecate the old functions.

The new arm_smmu_domain_inv_range() handles the CMDQ_MAX_TLBI_OPS as well, so drop it in the SVA function.

Since arm_smmu_cmdq_batch_add_range() has only one caller now, and it must be given a valid size, add a WARN_ON_ONCE to catch any missed case.

Quoted 7 lines

Reviewed-by Jason Gunthorpe <[email protected]>

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

---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |   7 -
 .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c   |  29 +--
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 165 +-----------------
 3 files changed, 11 insertions(+), 190 deletions(-)

It's one thing replacing the invalidation implementation but I think you need to update some of the old ordering comments, too. In particular, the old code relies on the dma_wmb() during cmdq insertion to order updates to in-memory structures, which includes the pgtable in non-strict mode.

I don't think any of that is true now?

Quoted 9 lines · Nicolin Chen

Replace the old invalidation functions with arm_smmu_domain_inv_range() in all the existing invalidation routines. And deprecate the old functions.

The new arm_smmu_domain_inv_range() handles the CMDQ_MAX_TLBI_OPS as well, so drop it in the SVA function.

Since arm_smmu_cmdq_batch_add_range() has only one caller now, and it must be given a valid size, add a WARN_ON_ONCE to catch any missed case.

Quoted 7 lines

Reviewed-by Jason Gunthorpe <[email protected]>

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

---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |   7 -
 .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c   |  29 +--
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 165 +-----------------
 3 files changed, 11 insertions(+), 190 deletions(-)

It's one thing replacing the invalidation implementation but I think you need to update some of the old ordering comments, too. In particular, the old code relies on the dma_wmb() during cmdq insertion to order updates to in-memory structures, which includes the pgtable in non-strict mode.

I don't think any of that is true now?

* NVIDIA is building systems with > 10 SMMU instances where > 8 are being used concurrently in a single VM. So having 8 copies of an identical S2 page table is not efficient. Instead, all vSMMU instances should check compatibility on a shared S2 iopt, to eliminate 7 copies.

Previous attempt based on the list/spinlock design:
iommu/arm-smmu-v3: Allocate vmid per vsmmu instead of s2_parent
https://lore.kernel.org/all/[email protected]/
now can adopt this invs array, avoiding adding complex lists/locks.

* The guest support for BTM requires temporarily invalidating two ASIDs for a single instance. When it renumbers ASIDs this can now be done via the invs array.

* SVA with multiple devices being used by a single process (NVIDIA today has 4-8) sequentially iterates the invalidations through all instances. This ignores the HW concurrency available in each instance. It would be nice to not spin on each sync but go forward and issue batches to other instances also. Reducing to a single SVA domain shared across instances is required to look at this.

This is on Github:
https://github.com/nicolinc/iommufd/commits/arm_smmu_invs-v9

Changelog
v9:
* Set cur->ssid correctly in arm_smmu_master_build_inv()
* Add comments for INV_TYPE_S1_ASID in arm_smmu_master_build_inv()