Hi Catalin,

On 2020/7/10 1:36, Catalin Marinas wrote:

On Thu, Jul 09, 2020 at 05:10:54PM +0800, Zhenyu Ye wrote:

quoted

 #define __tlbi_level(op, addr, level) do {				\
 	u64 arg = addr;							\
 									\
 	if (cpus_have_const_cap(ARM64_HAS_ARMv8_4_TTL) &&		\
+	    !cpus_have_const_cap(ARM64_HAS_TLBI_RANGE) &&		\
 	    level) {							\
 		u64 ttl = level & 3;					\
-									\
-		switch (PAGE_SIZE) {					\
-		case SZ_4K:						\
-			ttl |= TLBI_TTL_TG_4K << 2;			\
-			break;						\
-		case SZ_16K:						\
-			ttl |= TLBI_TTL_TG_16K << 2;			\
-			break;						\
-		case SZ_64K:						\
-			ttl |= TLBI_TTL_TG_64K << 2;			\
-			break;						\
-		}							\
-									\
+		ttl |= get_trans_granule() << 2;			\
 		arg &= ~TLBI_TTL_MASK;					\
 		arg |= FIELD_PREP(TLBI_TTL_MASK, ttl);			\
 	}								\

I think checking for !ARM64_HAS_TLBI_RANGE here is incorrect. I can see
why you attempted this since the range and classic ops have a different
position for the level but now you are not passing the TTL at all for
the classic TLBI. It's also inconsistent to have the range ops get the
level in the addr argument while the classic ops added in the
__tlbi_level macro.

You are right, this is really a serious problem.  But this can be avoided
after removing the check for ARM64_HAS_TLBI_RANGE and dropping the
__tlbi_last_level.
Just call __tlbi() and __tlbi_user() when doing range ops.

I'd rather have two sets of macros, __tlbi_level and __tlbi_range_level,
called depending on whether you use classic or range ops.

Then we have to add __tlbi_user_range_level, too. And if we move the num
and scale out of __TLBI_VADDR_RANGE, the __TLBI_VADDR_RANGE macro will make
little sense (addr and asid also can be moved out).

__TLBI_VADDR macro is defined to create a properly formatted VA operand for
the TLBI, then how about add the level to __TLBI_VADDR, just like:

	#define __TLBI_VADDR(addr, asid, level)				\
	({								\
		unsigned long __ta = (addr) >> 12;			\
		__ta &= GENMASK_ULL(43, 0);				\
		__ta |= (unsigned long)(asid) << 48;			\
		if (cpus_have_const_cap(ARM64_HAS_ARMv8_4_TTL)) {	\
			u64 ttl = get_trans_granule() << 2 + level & 3;	\
			__ta |= ttl << 44;				\
		}							\
		__ta;							\
	})

Then we should make sure __TLBI_VADDR is used for all TLBI operands. But
the related code has changed a lot in this merge window, so I perfer to
do this in the future, after all below be merged:

git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git kvm-arm64/el2-obj-v4.1
git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git kvm-arm64/pre-nv-5.9
git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/tlbi

Currently, keep the range ops get the level in the addr argument, the classic
ops added the level in the __tlbi_level macro.

quoted

@@ -108,6 +119,49 @@
 		__tlbi_level(op, (arg | USER_ASID_FLAG), level);	\
 } while (0)
 
+#define __tlbi_last_level(op1, op2, arg, last_level, tlb_level) do {	\
+	if (last_level)	{						\
+		__tlbi_level(op1, arg, tlb_level);			\
+		__tlbi_user_level(op1, arg, tlb_level);			\
+	} else {							\
+		__tlbi_level(op2, arg, tlb_level);			\
+		__tlbi_user_level(op2, arg, tlb_level);			\
+	}								\
+} while (0)

And you could drop this altogether. I know it's slightly more lines of
code but keeping it expanded in __flush_tlb_range() would be clearer.

Thanks,
Zhenyu


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel