arm64: Replace TLB invalidation preprocessor macros with C functions

In preparation for reducing our reliance on complex preprocessor macros for TLB invalidation routines, introduce a new C wrapper for by-level TLB invalidation helpers which can be used instead of the __tlbi() macro and can additionally be called from C code.

Signed-off-by Will Deacon <[email protected]>

---
 arch/arm64/include/asm/tlbflush.h | 33 ++++++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

arch/arm64/include/asm/tlbflush.h +10 -0

index aa9efee17277..1c7548ec6cb7 100644
@@ -88,6 +88,16 @@ static inline unsigned long get_trans_granule(void)
 	}
 }
 
+enum tlbi_op {
+	vae1is,
+	vae2is,
+	vale1is,
+	vale2is,
+	vaale1is,
+	ipas2e1,
+	ipas2e1is,
+};
+
 /*
  * Level-based TLBI operations.
  *

arch/arm64/include/asm/tlbflush.h +21 -1

+	case op:							\
+		__tlbi(op, arg);					\
+		break
+
+static __always_inline void __tlbi_level_op(const enum tlbi_op op, u64 arg)
+{
+	switch (op) {
+	__GEN_TLBI_OP_CASE(vae1is);
+	__GEN_TLBI_OP_CASE(vae2is);
+	__GEN_TLBI_OP_CASE(vale1is);
+	__GEN_TLBI_OP_CASE(vale2is);
+	__GEN_TLBI_OP_CASE(vaale1is);
+	__GEN_TLBI_OP_CASE(ipas2e1);
+	__GEN_TLBI_OP_CASE(ipas2e1is);
+	default:
+		BUILD_BUG();
+	}
+}
+#undef __GEN_TLBI_OP_CASE
+
 #define __tlbi_level(op, addr, level) do {				\
 	u64 arg = addr;							\
 									\

@@ -116,7 +147,7 @@ static inline unsigned long get_trans_granule(void)
 		arg |= FIELD_PREP(TLBI_TTL_MASK, ttl);			\
 	}								\
 									\
-	__tlbi(op, arg);						\
+	__tlbi_level_op(op, arg);					\
 } while(0)
 
 #define __tlbi_user_level(op, arg, level) do {				\

In preparation for reducing our reliance on complex preprocessor macros for TLB invalidation routines, introduce a new C wrapper for by-range TLB invalidation helpers which can be used instead of the __tlbi() macro and can additionally be called from C code.

Signed-off-by Will Deacon <[email protected]>

---
 arch/arm64/include/asm/tlbflush.h | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

arch/arm64/include/asm/tlbflush.h +0 -0

index 1c7548ec6cb7..4408aeebf4d5 100644

arch/arm64/include/asm/tlbflush.h +13 -1

+{
+	switch (op) {
+	__GEN_TLBI_OP_CASE(vae1is);
+	__GEN_TLBI_OP_CASE(vale1is);
+	__GEN_TLBI_OP_CASE(vaale1is);
+	__GEN_TLBI_OP_CASE(ipas2e1is);
+	default:
+		BUILD_BUG();
+	}
+}
+#undef __GEN_TLBI_OP_CASE
+
 #define __flush_tlb_range_op(op, start, pages, stride,			\
 				asid, tlb_level, tlbi_user, lpa2)	\
 do {									\

@@ -445,7 +463,7 @@ do {									\
 		if (num >= 0) {						\
 			addr = __TLBI_VADDR_RANGE(__flush_start >> shift, asid, \
 						scale, num, tlb_level);	\
-			__tlbi(r##op, addr);				\
+			__tlbi_range(op, addr);				\
 			if (tlbi_user)					\
 				__tlbi_user(r##op, addr);		\
 			__flush_start += __TLBI_RANGE_PAGES(num, scale) << PAGE_SHIFT; \

When kpti is enabled, separate ASIDs are used for userspace and kernelspace, requiring ASID-qualified TLB invalidation by virtual address to invalidate both of them.

Push the logic for invalidating the two ASIDs down into the low-level __tlbi_level_op() function based on the TLBI operation and remove the burden from the caller to handle the kpti-specific behaviour.

Signed-off-by Will Deacon <[email protected]>

---
 arch/arm64/include/asm/tlbflush.h | 45 ++++++++++++++++++-------------
 1 file changed, 26 insertions(+), 19 deletions(-)

arch/arm64/include/asm/tlbflush.h +20 -12

index 4408aeebf4d5..08e509f37b28 100644
@@ -115,17 +115,25 @@ enum tlbi_op {
 
 #define TLBI_TTL_UNKNOWN	INT_MAX
 
-#define __GEN_TLBI_OP_CASE(op)						\
+#define ___GEN_TLBI_OP_CASE(op)						\
 	case op:							\
-		__tlbi(op, arg);					\
+		__tlbi(op, arg)
+
+#define __GEN_TLBI_OP_ASID_CASE(op)					\
+	___GEN_TLBI_OP_CASE(op);					\
+		__tlbi_user(op, arg);					\
+		break
+
+#define __GEN_TLBI_OP_CASE(op)						\
+	___GEN_TLBI_OP_CASE(op);					\
 		break
 
 static __always_inline void __tlbi_level_op(const enum tlbi_op op, u64 arg)
 {
 	switch (op) {
-	__GEN_TLBI_OP_CASE(vae1is);
+	__GEN_TLBI_OP_ASID_CASE(vae1is);
 	__GEN_TLBI_OP_CASE(vae2is);
-	__GEN_TLBI_OP_CASE(vale1is);
+	__GEN_TLBI_OP_ASID_CASE(vale1is);
 	__GEN_TLBI_OP_CASE(vale2is);
 	__GEN_TLBI_OP_CASE(vaale1is);
 	__GEN_TLBI_OP_CASE(ipas2e1);

@@ -134,7 +142,8 @@ static __always_inline void __tlbi_level_op(const enum tlbi_op op, u64 arg)
 		BUILD_BUG();
 	}
 }
-#undef __GEN_TLBI_OP_CASE
+#undef __GEN_TLBI_OP_ASID_CASE
+#undef ___GEN_TLBI_OP_CASE
 
 #define __tlbi_level(op, addr, level) do {				\
 	u64 arg = addr;							\

@@ -150,11 +159,6 @@ static __always_inline void __tlbi_level_op(const enum tlbi_op op, u64 arg)
 	__tlbi_level_op(op, arg);					\
 } while(0)
 
-#define __tlbi_user_level(op, arg, level) do {				\
-	if (arm64_kernel_unmapped_at_el0())				\
-		__tlbi_level(op, (arg | USER_ASID_FLAG), level);	\
-} while (0)
-
 /*
  * This macro creates a properly formatted VA operand for the TLB RANGE. The
  * value bit assignments are:

@@ -418,22 +422,28 @@ static inline void arch_tlbbatch_flush(struct arch_tlbflush_unmap_batch *batch)
  *    operations can only span an even number of pages. We save this for last to
  *    ensure 64KB start alignment is maintained for the LPA2 case.
  */
-#define __GEN_TLBI_OP_CASE(op)						\
+#define ___GEN_TLBI_OP_CASE(op)						\
 	case op:							\
-		__tlbi(r ## op, arg);					\
+		__tlbi(r ## op, arg)
+
+#define __GEN_TLBI_OP_ASID_CASE(op)					\
+	___GEN_TLBI_OP_CASE(op);					\
+		__tlbi_user(r ## op, arg);				\
 		break

arch/arm64/include/asm/tlbflush.h +4 -5

 	default:
 		BUILD_BUG();
 	}
 }
+#undef __GEN_TLBI_OP_ASID_CASE
+#undef ___GEN_TLBI_OP_CASE
 #undef __GEN_TLBI_OP_CASE
 
 #define __flush_tlb_range_op(op, start, pages, stride,			\

@@ -452,8 +462,6 @@ do {									\
 		    (lpa2 && __flush_start != ALIGN(__flush_start, SZ_64K))) {	\
 			addr = __TLBI_VADDR(__flush_start, asid);	\
 			__tlbi_level(op, addr, tlb_level);		\
-			if (tlbi_user)					\
-				__tlbi_user_level(op, addr, tlb_level);	\
 			__flush_start += stride;			\
 			__flush_pages -= stride >> PAGE_SHIFT;		\
 			continue;					\

@@ -464,8 +472,6 @@ do {									\
 			addr = __TLBI_VADDR_RANGE(__flush_start >> shift, asid, \
 						scale, num, tlb_level);	\
 			__tlbi_range(op, addr);				\
-			if (tlbi_user)					\
-				__tlbi_user(r##op, addr);		\
 			__flush_start += __TLBI_RANGE_PAGES(num, scale) << PAGE_SHIFT; \
 			__flush_pages -= __TLBI_RANGE_PAGES(num, scale);\
 		}							\

@@ -584,6 +590,7 @@ static inline void arch_tlbbatch_add_pending(struct arch_tlbflush_unmap_batch *b
 {
 	__flush_tlb_range_nosync(mm, start, end, PAGE_SIZE, true, 3);
 }
-#endif
 
+#undef __tlbi_user
+#endif
 #endif

Quoted 84 lines · Will Deacon

When kpti is enabled, separate ASIDs are used for userspace and kernelspace, requiring ASID-qualified TLB invalidation by virtual address to invalidate both of them.

Push the logic for invalidating the two ASIDs down into the low-level __tlbi_level_op() function based on the TLBI operation and remove the burden from the caller to handle the kpti-specific behaviour.

Signed-off-by Will Deacon <[email protected]>

---
 arch/arm64/include/asm/tlbflush.h | 45 ++++++++++++++++++-------------
 1 file changed, 26 insertions(+), 19 deletions(-)

arch/arm64/include/asm/tlbflush.h +20 -12

index 4408aeebf4d5..08e509f37b28 100644
@@ -115,17 +115,25 @@ enum tlbi_op {
 
 #define TLBI_TTL_UNKNOWN	INT_MAX
 
-#define __GEN_TLBI_OP_CASE(op)						\
+#define ___GEN_TLBI_OP_CASE(op)						\
 	case op:							\
-		__tlbi(op, arg);					\
+		__tlbi(op, arg)
+
+#define __GEN_TLBI_OP_ASID_CASE(op)					\
+	___GEN_TLBI_OP_CASE(op);					\
+		__tlbi_user(op, arg);					\
+		break
+
+#define __GEN_TLBI_OP_CASE(op)						\
+	___GEN_TLBI_OP_CASE(op);					\
 		break
 
 static __always_inline void __tlbi_level_op(const enum tlbi_op op, u64 arg)
 {
 	switch (op) {
-	__GEN_TLBI_OP_CASE(vae1is);
+	__GEN_TLBI_OP_ASID_CASE(vae1is);
 	__GEN_TLBI_OP_CASE(vae2is);
-	__GEN_TLBI_OP_CASE(vale1is);
+	__GEN_TLBI_OP_ASID_CASE(vale1is);
 	__GEN_TLBI_OP_CASE(vale2is);
 	__GEN_TLBI_OP_CASE(vaale1is);
 	__GEN_TLBI_OP_CASE(ipas2e1);

@@ -134,7 +142,8 @@ static __always_inline void __tlbi_level_op(const enum tlbi_op op, u64 arg)
 		BUILD_BUG();
 	}
 }
-#undef __GEN_TLBI_OP_CASE
+#undef __GEN_TLBI_OP_ASID_CASE
+#undef ___GEN_TLBI_OP_CASE
 
 #define __tlbi_level(op, addr, level) do {				\
 	u64 arg = addr;							\

@@ -150,11 +159,6 @@ static __always_inline void __tlbi_level_op(const enum tlbi_op op, u64 arg)
 	__tlbi_level_op(op, arg);					\
 } while(0)
 
-#define __tlbi_user_level(op, arg, level) do {				\
-	if (arm64_kernel_unmapped_at_el0())				\
-		__tlbi_level(op, (arg | USER_ASID_FLAG), level);	\
-} while (0)
-
 /*
  * This macro creates a properly formatted VA operand for the TLB RANGE. The
  * value bit assignments are:

@@ -418,22 +422,28 @@ static inline void arch_tlbbatch_flush(struct arch_tlbflush_unmap_batch *batch)
  *    operations can only span an even number of pages. We save this for last to
  *    ensure 64KB start alignment is maintained for the LPA2 case.
  */
-#define __GEN_TLBI_OP_CASE(op)						\
+#define ___GEN_TLBI_OP_CASE(op)						\
 	case op:							\
-		__tlbi(r ## op, arg);					\
+		__tlbi(r ## op, arg)
+
+#define __GEN_TLBI_OP_ASID_CASE(op)					\
+	___GEN_TLBI_OP_CASE(op);					\
+		__tlbi_user(r ## op, arg);				\
 		break
 

arch/arm64/include/asm/tlbflush.h static inline void arch_tlbbatch_flush(struct arch_tlbflush_unmap_batch *batch) · line 440

Quoted 9 lines

arch/arm64/include/asm/tlbflush.h +2 -2

 static __always_inline void __tlbi_range(const enum tlbi_op op, u64 arg)
 {
 	switch (op) {
-	__GEN_TLBI_OP_CASE(vae1is);
-	__GEN_TLBI_OP_CASE(vale1is);
+	__GEN_TLBI_OP_ASID_CASE(vae1is);
+	__GEN_TLBI_OP_ASID_CASE(vale1is);
 	__GEN_TLBI_OP_CASE(vaale1is);
 	__GEN_TLBI_OP_CASE(ipas2e1is);

Bug? This 2 underscore version is still defined from the level case above. So this is no longer issuing a range-based tlbi? (i.e. you're no longer prepending the "r" here.

Quoted 37 lines

arch/arm64/include/asm/tlbflush.h +4 -5

 	default:
 		BUILD_BUG();
 	}
 }
+#undef __GEN_TLBI_OP_ASID_CASE
+#undef ___GEN_TLBI_OP_CASE
 #undef __GEN_TLBI_OP_CASE
 
 #define __flush_tlb_range_op(op, start, pages, stride,			\

@@ -452,8 +462,6 @@ do {									\
 		    (lpa2 && __flush_start != ALIGN(__flush_start, SZ_64K))) {	\
 			addr = __TLBI_VADDR(__flush_start, asid);	\
 			__tlbi_level(op, addr, tlb_level);		\
-			if (tlbi_user)					\
-				__tlbi_user_level(op, addr, tlb_level);	\
 			__flush_start += stride;			\
 			__flush_pages -= stride >> PAGE_SHIFT;		\
 			continue;					\

@@ -464,8 +472,6 @@ do {									\
 			addr = __TLBI_VADDR_RANGE(__flush_start >> shift, asid, \
 						scale, num, tlb_level);	\
 			__tlbi_range(op, addr);				\
-			if (tlbi_user)					\
-				__tlbi_user(r##op, addr);		\
 			__flush_start += __TLBI_RANGE_PAGES(num, scale) << PAGE_SHIFT; \
 			__flush_pages -= __TLBI_RANGE_PAGES(num, scale);\
 		}							\

@@ -584,6 +590,7 @@ static inline void arch_tlbbatch_add_pending(struct arch_tlbflush_unmap_batch *b
 {
 	__flush_tlb_range_nosync(mm, start, end, PAGE_SIZE, true, 3);
 }
-#endif
 
+#undef __tlbi_user
+#endif
 #endif

__tlbi_level() is just a simple macro around __tlbi_level_op(), so merge the two into a single C function. Drop the redundant comparison of 'u32 level' against 0 and tidy up the code a little while we're at it.

Signed-off-by Will Deacon <[email protected]>

---
 arch/arm64/include/asm/tlbflush.h | 25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)

arch/arm64/include/asm/tlbflush.h +0 -0

index 728b00f3e1f4..ddd77e92b268 100644

arch/arm64/include/asm/tlbflush.h +9 -14

 {
+	u64 arg = addr;
+
+	if (alternative_has_cap_unlikely(ARM64_HAS_ARMv8_4_TTL) && level <= 3) {
+		u64 ttl = level | (get_trans_granule() << 2);
+
+		arg &= ~TLBI_TTL_MASK;
+		arg |= FIELD_PREP(TLBI_TTL_MASK, ttl);
+	}
+
 	switch (op) {
 	__GEN_TLBI_OP_ASID_CASE(vae1is);
 	__GEN_TLBI_OP_CASE(vae2is);

@@ -145,20 +154,6 @@ static __always_inline void __tlbi_level_op(const enum tlbi_op op, u64 arg)
 #undef __GEN_TLBI_OP_ASID_CASE
 #undef ___GEN_TLBI_OP_CASE
 
-#define __tlbi_level(op, addr, level) do {				\
-	u64 arg = addr;							\
-									\
-	if (alternative_has_cap_unlikely(ARM64_HAS_ARMv8_4_TTL) &&	\
-	    level >= 0 && level <= 3) {					\
-		u64 ttl = level & 3;					\
-		ttl |= get_trans_granule() << 2;			\
-		arg &= ~TLBI_TTL_MASK;					\
-		arg |= FIELD_PREP(TLBI_TTL_MASK, ttl);			\
-	}								\
-									\
-	__tlbi_level_op(op, arg);					\
-} while(0)
-
 /*
  * This macro creates a properly formatted VA operand for the TLB RANGE. The
  * value bit assignments are:

Since commit e2768b798a19 ("arm64/mm: Modify range-based tlbi to decrement scale"), we don't need to clamp the 'pages' argument to fit the range for the specified 'scale' as we know that the upper bits will have been processed in a prior iteration.

Drop the clamping and simplify the __TLBI_RANGE_NUM() macro.

Signed-off-by Will Deacon <[email protected]>

---
 arch/arm64/include/asm/tlbflush.h | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

arch/arm64/include/asm/tlbflush.h +1 -5

index ddd77e92b268..a8d21e52ef3a 100644
@@ -205,11 +205,7 @@ static __always_inline void __tlbi_level(const enum tlbi_op op, u64 addr, u32 le
  * range.
  */
 #define __TLBI_RANGE_NUM(pages, scale)					\
-	({								\
-		int __pages = min((pages),				\
-				  __TLBI_RANGE_PAGES(31, (scale)));	\
-		(__pages >> (5 * (scale) + 1)) - 1;			\
-	})
+	(((pages) >> (5 * (scale) + 1)) - 1)
 
 /*
  *	TLB Invalidation

Since commit e2768b798a19 ("arm64/mm: Modify range-based tlbi to decrement scale"), we don't need to clamp the 'pages' argument to fit the range for the specified 'scale' as we know that the upper bits will have been processed in a prior iteration.

Drop the clamping and simplify the __TLBI_RANGE_NUM() macro.

Signed-off-by Will Deacon <[email protected]>

---
  arch/arm64/include/asm/tlbflush.h | 6 +-----
  1 file changed, 1 insertion(+), 5 deletions(-)

arch/arm64/include/asm/tlbflush.h +1 -5

index ddd77e92b268..a8d21e52ef3a 100644
@@ -205,11 +205,7 @@ static __always_inline void __tlbi_level(const enum tlbi_op op, u64 addr, u32 le
   * range.
   */
  #define __TLBI_RANGE_NUM(pages, scale)					\
-	({								\
-		int __pages = min((pages),				\
-				  __TLBI_RANGE_PAGES(31, (scale)));	\
-		(__pages >> (5 * (scale) + 1)) - 1;			\
-	})
+	(((pages) >> (5 * (scale) + 1)) - 1)
 
  /*
   *	TLB Invalidation

The __TLBI_VADDR_RANGE() macro is only used in one place and isn't something that's generally useful outside of the low-level range invalidation gubbins.

Inline __TLBI_VADDR_RANGE() into the __tlbi_range() function so that the macro can be removed entirely.

Signed-off-by Will Deacon <[email protected]>

---
 arch/arm64/include/asm/tlbflush.h | 32 +++++++++++++------------------
 1 file changed, 13 insertions(+), 19 deletions(-)

arch/arm64/include/asm/tlbflush.h +0 -13

index 434b9fdb340a..8618a85d5cd3 100644
@@ -185,19 +185,6 @@ static inline void __tlbi_level(const enum tlbi_op op, u64 addr, u32 level)
 #define TLBIR_TTL_MASK		GENMASK_ULL(38, 37)
 #define TLBIR_BADDR_MASK	GENMASK_ULL(36,  0)
 
-#define __TLBI_VADDR_RANGE(baddr, asid, scale, num, ttl)		\
-	({								\
-		unsigned long __ta = 0;					\
-		unsigned long __ttl = (ttl >= 1 && ttl <= 3) ? ttl : 0;	\
-		__ta |= FIELD_PREP(TLBIR_BADDR_MASK, baddr);		\
-		__ta |= FIELD_PREP(TLBIR_TTL_MASK, __ttl);		\
-		__ta |= FIELD_PREP(TLBIR_NUM_MASK, num);		\
-		__ta |= FIELD_PREP(TLBIR_SCALE_MASK, scale);		\
-		__ta |= FIELD_PREP(TLBIR_TG_MASK, get_trans_granule());	\
-		__ta |= FIELD_PREP(TLBIR_ASID_MASK, asid);		\
-		__ta;							\
-	})
-
 /* These macros are used by the TLBI RANGE feature. */
 #define __TLBI_RANGE_PAGES(num, scale)	\
 	((unsigned long)((num) + 1) << (5 * (scale) + 1))

arch/arm64/include/asm/tlbflush.h +10 -5

 {
+	u64 arg = 0;
+
+	arg |= FIELD_PREP(TLBIR_BADDR_MASK, addr >> (lpa2 ? 16 : PAGE_SHIFT));
+	arg |= FIELD_PREP(TLBIR_TTL_MASK, level > 3 ? 0 : level);
+	arg |= FIELD_PREP(TLBIR_NUM_MASK, num);
+	arg |= FIELD_PREP(TLBIR_SCALE_MASK, scale);
+	arg |= FIELD_PREP(TLBIR_TG_MASK, get_trans_granule());
+	arg |= FIELD_PREP(TLBIR_ASID_MASK, asid);
+
 	switch (op) {
 	__GEN_TLBI_OP_ASID_CASE(vae1is);
 	__GEN_TLBI_OP_ASID_CASE(vale1is);

@@ -448,8 +446,6 @@ do {									\
 	typeof(pages) __flush_pages = pages;				\
 	int num = 0;							\
 	int scale = 3;							\
-	int shift = lpa2 ? 16 : PAGE_SHIFT;				\
-	unsigned long addr;						\
 									\
 	while (__flush_pages > 0) {					\
 		if (!system_supports_tlb_range() ||			\

@@ -463,9 +459,7 @@ do {									\
 									\
 		num = __TLBI_RANGE_NUM(__flush_pages, scale);		\
 		if (num >= 0) {						\
-			addr = __TLBI_VADDR_RANGE(__flush_start >> shift, asid, \
-						scale, num, tlb_level);	\
-			__tlbi_range(op, addr);				\
+			__tlbi_range(op, __flush_start, asid, scale, num, tlb_level, lpa2); \
 			__flush_start += __TLBI_RANGE_PAGES(num, scale) << PAGE_SHIFT; \
 			__flush_pages -= __TLBI_RANGE_PAGES(num, scale);\
 		}							\

__flush_tlb_range_limit_excess() is unnecessarily complicated:

- It takes a 'start', 'end' and 'pages' argument, whereas it only needs 'pages' (which the caller has computed from the other two arguments!).

- It erroneously compares 'pages' with MAX_TLBI_RANGE_PAGES when the system doesn't support range-based invalidation but the range to be invalidated would result in fewer than MAX_DVM_OPS invalidations.

Simplify the function so that it no longer takes the 'start' and 'end' arguments and only considers the MAX_TLBI_RANGE_PAGES threshold on systems that implement range-based invalidation.

Signed-off-by Will Deacon <[email protected]>

---
 arch/arm64/include/asm/tlbflush.h | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

arch/arm64/include/asm/tlbflush.h +2 -7

index 8618a85d5cd3..2541863721af 100644
@@ -470,21 +470,13 @@ do {									\
 #define __flush_s2_tlb_range_op(op, start, pages, stride, tlb_level) \
 	__flush_tlb_range_op(op, start, pages, stride, 0, tlb_level, kvm_lpa2_is_enabled());
 
-static inline bool __flush_tlb_range_limit_excess(unsigned long start,
-		unsigned long end, unsigned long pages, unsigned long stride)
+static inline bool __flush_tlb_range_limit_excess(unsigned long pages,
+						  unsigned long stride)
 {
-	/*
-	 * When the system does not support TLB range based flush
-	 * operation, (MAX_DVM_OPS - 1) pages can be handled. But
-	 * with TLB range based operation, MAX_TLBI_RANGE_PAGES
-	 * pages can be handled.

arch/arm64/include/asm/tlbflush.h +2 -2

 static inline void __flush_tlb_range_nosync(struct mm_struct *mm,

@@ -498,7 +490,7 @@ static inline void __flush_tlb_range_nosync(struct mm_struct *mm,
 	end = round_up(end, stride);
 	pages = (end - start) >> PAGE_SHIFT;
 
-	if (__flush_tlb_range_limit_excess(start, end, pages, stride)) {
+	if (__flush_tlb_range_limit_excess(pages, stride)) {
 		flush_tlb_mm(mm);
 		return;
 	}

@@ -547,7 +539,7 @@ static inline void flush_tlb_kernel_range(unsigned long start, unsigned long end
 	end = round_up(end, stride);
 	pages = (end - start) >> PAGE_SHIFT;
 
-	if (__flush_tlb_range_limit_excess(start, end, pages, stride)) {
+	if (__flush_tlb_range_limit_excess(pages, stride)) {
 		flush_tlb_all();
 		return;
 	}

Quoted 10 lines · Will Deacon

__flush_tlb_range_limit_excess() is unnecessarily complicated:

- It takes a 'start', 'end' and 'pages' argument, whereas it only needs 'pages' (which the caller has computed from the other two arguments!).

- It erroneously compares 'pages' with MAX_TLBI_RANGE_PAGES when the system doesn't support range-based invalidation but the range to be invalidated would result in fewer than MAX_DVM_OPS invalidations.

Quoted 5 lines

Simplify the function so that it no longer takes the 'start' and 'end' arguments and only considers the MAX_TLBI_RANGE_PAGES threshold on systems that implement range-based invalidation.

Signed-off-by Will Deacon <[email protected]>

Does this warrant a Fixes: tag?

Quoted 29 lines

---
  arch/arm64/include/asm/tlbflush.h | 20 ++++++--------------
  1 file changed, 6 insertions(+), 14 deletions(-)

arch/arm64/include/asm/tlbflush.h +3 -11

index 8618a85d5cd3..2541863721af 100644
@@ -470,21 +470,13 @@ do {									\
  #define __flush_s2_tlb_range_op(op, start, pages, stride, tlb_level) \
  	__flush_tlb_range_op(op, start, pages, stride, 0, tlb_level, kvm_lpa2_is_enabled());
 
-static inline bool __flush_tlb_range_limit_excess(unsigned long start,
-		unsigned long end, unsigned long pages, unsigned long stride)
+static inline bool __flush_tlb_range_limit_excess(unsigned long pages,
+						  unsigned long stride)
  {
-	/*
-	 * When the system does not support TLB range based flush
-	 * operation, (MAX_DVM_OPS - 1) pages can be handled. But
-	 * with TLB range based operation, MAX_TLBI_RANGE_PAGES
-	 * pages can be handled.
-	 */
-	if ((!system_supports_tlb_range() &&
-	     (end - start) >= (MAX_DVM_OPS * stride)) ||
-	    pages > MAX_TLBI_RANGE_PAGES)
+	if (system_supports_tlb_range() && pages > MAX_TLBI_RANGE_PAGES)
  		return true;
 

arch/arm64/include/asm/tlbflush.h do { \ · line 480

Quoted 3 lines

arch/arm64/include/asm/tlbflush.h +1 -1

-	return false;
+	return pages >= (MAX_DVM_OPS * stride) >> PAGE_SHIFT;
  }

I'm still not sure I totally get this... Aren't these really 2 separate concepts? MAX_TLBI_RANGE_PAGES is the max amount of VA that can be handled by a single tlbi-by-range (and due to implementation, the largest range that can be handled by the loop in __flush_tlb_range_op()). Whereas MAX_DVM_OPS is the max number of tlbi instrcutions you want to issue with the PTL held? Perhaps it is better to split these out; For the range case, calculate the number of ops you actually need and compare with MAX_DVM_OPS?

Quoted 22 lines

arch/arm64/include/asm/tlbflush.h +2 -2

  static inline void __flush_tlb_range_nosync(struct mm_struct *mm,

@@ -498,7 +490,7 @@ static inline void __flush_tlb_range_nosync(struct mm_struct *mm,
  	end = round_up(end, stride);
  	pages = (end - start) >> PAGE_SHIFT;
 
-	if (__flush_tlb_range_limit_excess(start, end, pages, stride)) {
+	if (__flush_tlb_range_limit_excess(pages, stride)) {
  		flush_tlb_mm(mm);
  		return;
  	}

@@ -547,7 +539,7 @@ static inline void flush_tlb_kernel_range(unsigned long start, unsigned long end
  	end = round_up(end, stride);
  	pages = (end - start) >> PAGE_SHIFT;
 
-	if (__flush_tlb_range_limit_excess(start, end, pages, stride)) {
+	if (__flush_tlb_range_limit_excess(pages, stride)) {
  		flush_tlb_all();
  		return;
  	}

The __flush_tlb_range_op() macro is horrible and has been a previous source of bugs thanks to multiple expansions of its arguments (see commit f7edb07ad7c6 ("Fix mmu notifiers for range-based invalidates")).

Rewrite the thing in C.

Quoted 6 lines · Will Deacon

The __flush_tlb_range_op() macro is horrible and has been a previous source of bugs thanks to multiple expansions of its arguments (see commit f7edb07ad7c6 ("Fix mmu notifiers for range-based invalidates")).

Rewrite the thing in C.

So I do think this is better than the old case, but I think you could go one step further...

arch/arm64/include/asm/tlbflush.h static __always_inline void __tlbi_range(const enum tlbi_op op, u64 addr, · line 445

Quoted 4 lines

arch/arm64/include/asm/tlbflush.h +4 -0

+static __always_inline void __flush_tlb_range_op(const enum tlbi_op op,
+                                                u64 start, size_t pages,
+                                                u64 stride, u16 asid,
+                                                u32 level, bool lpa2)

If you replaced that "enum tlbi_op op" with two function pointers instead (for "the invalidate range" and "invalidate one" cases respectively), I think you could make this code much more obvious.

And exactly like how you depend on that 'op' value being constant-folded because all the different levels are inline functions, the same thing ends up happening with function pointers where inlining will result in a constant function pointer becoming just a static call (and in turn inlined as well).

And then the actual *callers* would use the "look up op" thing, but I suspect that in many cases those could then be in turn also simplified to not use that op-number at all, but just end up using the op-name.

I didn't try to actually create that series - and I think you'd want to do it in multiple steps just to make each individual step small and obvious - but I think it would end up looking nicer.

The __flush_tlb_range_op() macro is horrible and has been a previous

source of bugs thanks to multiple expansions of its arguments (see commit f7edb07ad7c6 ("Fix mmu notifiers for range-based invalidates")).

Rewrite the thing in C.

Suggested-by Linus Torvalds <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---
 arch/arm64/include/asm/tlbflush.h | 63 +++++++++++++++++--------------
 1 file changed, 34 insertions(+), 29 deletions(-)

arch/arm64/include/asm/tlbflush.h +34 -29

index 2541863721af..ee69efdc12ab 100644
@@ -376,12 +376,12 @@ static inline void arch_tlbbatch_flush(struct arch_tlbflush_unmap_batch *batch)
 /*
  * __flush_tlb_range_op - Perform TLBI operation upon a range
  *
- * @op:	TLBI instruction that operates on a range (has 'r' prefix)
+ * @op:		TLBI instruction that operates on a range
  * @start:	The start address of the range
  * @pages:	Range as the number of pages from 'start'
  * @stride:	Flush granularity
  * @asid:	The ASID of the task (0 for IPA instructions)
- * @tlb_level:	Translation Table level hint, if known
+ * @level:	Translation Table level hint, if known
  * @lpa2:	If 'true', the lpa2 scheme is used as set out below
  *
  * When the CPU does not support TLB range operations, flush the TLB

@@ -439,33 +439,38 @@ static __always_inline void __tlbi_range(const enum tlbi_op op, u64 addr,
 #undef ___GEN_TLBI_OP_CASE
 #undef __GEN_TLBI_OP_CASE
 
-#define __flush_tlb_range_op(op, start, pages, stride,			\
-				asid, tlb_level, lpa2)			\
-do {									\
-	typeof(start) __flush_start = start;				\
-	typeof(pages) __flush_pages = pages;				\
-	int num = 0;							\
-	int scale = 3;							\
-									\
-	while (__flush_pages > 0) {					\
-		if (!system_supports_tlb_range() ||			\
-		    __flush_pages == 1 ||				\
-		    (lpa2 && __flush_start != ALIGN(__flush_start, SZ_64K))) {	\
-			__tlbi_level_asid(op, __flush_start, tlb_level, asid);	\
-			__flush_start += stride;			\
-			__flush_pages -= stride >> PAGE_SHIFT;		\
-			continue;					\
-		}							\
-									\
-		num = __TLBI_RANGE_NUM(__flush_pages, scale);		\
-		if (num >= 0) {						\
-			__tlbi_range(op, __flush_start, asid, scale, num, tlb_level, lpa2); \
-			__flush_start += __TLBI_RANGE_PAGES(num, scale) << PAGE_SHIFT; \
-			__flush_pages -= __TLBI_RANGE_PAGES(num, scale);\
-		}							\
-		scale--;						\
-	}								\
-} while (0)
+static __always_inline void __flush_tlb_range_op(const enum tlbi_op op,
+						 u64 start, size_t pages,
+						 u64 stride, u16 asid,
+						 u32 level, bool lpa2)
+{
+	u64 addr = start, end = start + pages * PAGE_SIZE;
+	int scale = 3;
+
+	while (addr != end) {
+		int num;
+
+		pages = (end - addr) >> PAGE_SHIFT;
+
+		if (!system_supports_tlb_range() || pages == 1)
+			goto invalidate_one;
+
+		if (lpa2 && !IS_ALIGNED(addr, SZ_64K))
+			goto invalidate_one;
+
+		num = __TLBI_RANGE_NUM(pages, scale);
+		if (num >= 0) {
+			__tlbi_range(op, addr, asid, scale, num, level, lpa2);
+			addr += __TLBI_RANGE_PAGES(num, scale) << PAGE_SHIFT;
+		}
+
+		scale--;
+		continue;
+invalidate_one:
+		__tlbi_level_asid(op, addr, level, asid);
+		addr += stride;
+	}
+}
 
 #define __flush_s2_tlb_range_op(op, start, pages, stride, tlb_level) \
 	__flush_tlb_range_op(op, start, pages, stride, 0, tlb_level, kvm_lpa2_is_enabled());

Hi all,

I cooked this series following a complaint from Linus back in March about our range-based TLB invalidation macro after we fixed an over-invalidation bug thanks to incorrect handling of its arguments:

https://lore.kernel.org/all/CAHk-=wgiX0q0WCL+SFwVCYtG7JR3=2Rshse-5J3AO2Y4AgT7Jw@mail.gmail.com/

Once I started trying to rework the range macro into a C function, I spotted a few other opportunities for cleanup and so I've ended up with this series.

Testing and feedback welcome.

Cheers,

Will

Cc Ard Biesheuvel <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Ryan Roberts <[email protected]>

Cc Mark Rutland <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Oliver Upton <[email protected]>

Cc Marc Zyngier <[email protected]>

--->8

Will Deacon (10):
arm64: mm: Introduce a C wrapper for by-level TLB invalidation helpers
arm64: mm: Introduce a C wrapper for by-range TLB invalidation helpers
arm64: mm: Implicitly invalidate user ASID based on TLBI operation
arm64: mm: Remove unused 'tlbi_user' argument from

    __flush_tlb_range_op()

arm64: mm: Re-implement the __tlbi_level macro in C
arm64: mm: Simplify __TLBI_RANGE_NUM() macro
arm64: mm: Push __TLBI_VADDR() into __tlbi_level()
arm64: mm: Inline __TLBI_VADDR_RANGE() into __tlbi_range()
arm64: mm: Simplify __flush_tlb_range_limit_excess()
arm64: mm: Re-implement the __flush_tlb_range_op macro in C

 arch/arm64/include/asm/tlbflush.h | 230 ++++++++++++++++++------------
 arch/arm64/kernel/sys_compat.c    |   2 +-
 arch/arm64/kvm/hyp/nvhe/mm.c      |   2 +-
 arch/arm64/kvm/hyp/pgtable.c      |   4 +-
 4 files changed, 140 insertions(+), 98 deletions(-)

Quoted 6 lines · Will Deacon

Hi all,

I cooked this series following a complaint from Linus back in March about our range-based TLB invalidation macro after we fixed an over-invalidation bug thanks to incorrect handling of its arguments:

Quoted 7 lines

https://lore.kernel.org/all/CAHk-=wgiX0q0WCL+SFwVCYtG7JR3=2Rshse-5J3AO2Y4AgT7Jw@mail.gmail.com/

Once I started trying to rework the range macro into a C function, I spotted a few other opportunities for cleanup and so I've ended up with this series.

Testing and feedback welcome.

Hi Will,

Did this go anywhere/are you planning to post a new version? There are a couple of other cleanups I'd like to do in this area, and also I'd like to add tlbip support. But I don't want to spend time on it if the baseline is going to drastically change.

If you're not planning a v2, perhaps I could have a go then extend for the extras I want to do?

Thanks,
Ryan

Quoted 34 lines

Cheers,

Will

Cc Ard Biesheuvel <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Ryan Roberts <[email protected]>

Cc Mark Rutland <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Oliver Upton <[email protected]>

Cc Marc Zyngier <[email protected]>

--->8

Will Deacon (10):
arm64: mm: Introduce a C wrapper for by-level TLB invalidation helpers
arm64: mm: Introduce a C wrapper for by-range TLB invalidation helpers
arm64: mm: Implicitly invalidate user ASID based on TLBI operation
arm64: mm: Remove unused 'tlbi_user' argument from

    __flush_tlb_range_op()

arm64: mm: Re-implement the __tlbi_level macro in C
arm64: mm: Simplify __TLBI_RANGE_NUM() macro
arm64: mm: Push __TLBI_VADDR() into __tlbi_level()
arm64: mm: Inline __TLBI_VADDR_RANGE() into __tlbi_range()
arm64: mm: Simplify __flush_tlb_range_limit_excess()
arm64: mm: Re-implement the __flush_tlb_range_op macro in C

 arch/arm64/include/asm/tlbflush.h | 230 ++++++++++++++++++------------
 arch/arm64/kernel/sys_compat.c    |   2 +-
 arch/arm64/kvm/hyp/nvhe/mm.c      |   2 +-
 arch/arm64/kvm/hyp/pgtable.c      |   4 +-
 4 files changed, 140 insertions(+), 98 deletions(-)

Quoted 20 lines · Ryan Roberts

Quoted 11 lines · Will Deacon

I cooked this series following a complaint from Linus back in March about our range-based TLB invalidation macro after we fixed an over-invalidation bug thanks to incorrect handling of its arguments:

https://lore.kernel.org/all/CAHk-=wgiX0q0WCL+SFwVCYtG7JR3=2Rshse-5J3AO2Y4AgT7Jw@mail.gmail.com/

Once I started trying to rework the range macro into a C function, I spotted a few other opportunities for cleanup and so I've ended up with this series.

Testing and feedback welcome.

Hi Will,

Did this go anywhere/are you planning to post a new version? There are a couple of other cleanups I'd like to do in this area, and also I'd like to add tlbip support. But I don't want to spend time on it if the baseline is going to drastically change.

Quoted 2 lines

If you're not planning a v2, perhaps I could have a go then extend for the extras I want to do?

Since I didn't hear back, I've reworked the series to include Linus's suggestion and am currently testing. Plan to post against -rc1.