[PATCH -next v2 0/2]riscv: some refactorings realted to uaccess and extable

[PATCH -next v2 0/2]riscv: some refactorings realted to uaccess and extable

[Tong Tiangen]

This patchset do some refactorings related to riscv's uaccess and extable,
mainly for the usage of __get/put_user_nocheck() which not distinguish user
access and kernel access.

v1 -> v2: 
  According to Conor's suggestion, split into two logically independent
  patches.

Tong Tiangen (2):
  riscv: uaccess: rename __get/put_user_nocheck to __get/put_mem_nocheck
  riscv: extable: add new extable type EX_TYPE_KACCESS_ERR_ZERO support

 arch/riscv/include/asm/asm-extable.h |  12 ++
 arch/riscv/include/asm/uaccess.h     | 162 +++++++++++++--------------
 arch/riscv/mm/extable.c              |   1 +
 3 files changed, 94 insertions(+), 81 deletions(-)

-- 
2.25.1

[PATCH -next v2 1/2] riscv: uaccess: rename __get/put_user_nocheck to __get/put_mem_nocheck

[Tong Tiangen]

Current, The helpers __get/put_user_nocheck() is used by get/put_user() and
__get/put_kernel_nofault(), which is not always uaccess, so the name with
*user* is not appropriate.

Also rename xxx_user_xxx to xxx_mem_xx  on the call path of
__get/put_user_nocheck()

Only refactor code without any functional changes.

Signed-off-by: Tong Tiangen <tongtiangen@huawei.com>
---
 arch/riscv/include/asm/uaccess.h | 48 ++++++++++++++++----------------
 1 file changed, 24 insertions(+), 24 deletions(-)

diff --git a/arch/riscv/include/asm/uaccess.h b/arch/riscv/include/asm/uaccess.h
index 855450bed9f5..1370da055b44 100644
--- a/arch/riscv/include/asm/uaccess.h
+++ b/arch/riscv/include/asm/uaccess.h
@@ -50,7 +50,7 @@
  * call.
  */
 
-#define __get_user_asm(insn, x, ptr, err)			\
+#define __get_mem_asm(insn, x, ptr, err)			\
 do {								\
 	__typeof__(x) __x;					\
 	__asm__ __volatile__ (					\
@@ -64,12 +64,12 @@ do {								\
 } while (0)
 
 #ifdef CONFIG_64BIT
-#define __get_user_8(x, ptr, err) \
-	__get_user_asm("ld", x, ptr, err)
+#define __get_mem_8(x, ptr, err) \
+	__get_mem_asm("ld", x, ptr, err)
 #else /* !CONFIG_64BIT */
-#define __get_user_8(x, ptr, err)				\
+#define __get_mem_8(x, ptr, err)				\
 do {								\
-	u32 __user *__ptr = (u32 __user *)(ptr);		\
+	u32 *__ptr = (u32 *)(ptr);				\
 	u32 __lo, __hi;						\
 	__asm__ __volatile__ (					\
 		"1:\n"						\
@@ -88,20 +88,20 @@ do {								\
 } while (0)
 #endif /* CONFIG_64BIT */
 
-#define __get_user_nocheck(x, __gu_ptr, __gu_err)		\
+#define __get_mem_nocheck(x, __gu_ptr, __gu_err)		\
 do {								\
 	switch (sizeof(*__gu_ptr)) {				\
 	case 1:							\
-		__get_user_asm("lb", (x), __gu_ptr, __gu_err);	\
+		__get_mem_asm("lb", (x), __gu_ptr, __gu_err);	\
 		break;						\
 	case 2:							\
-		__get_user_asm("lh", (x), __gu_ptr, __gu_err);	\
+		__get_mem_asm("lh", (x), __gu_ptr, __gu_err);	\
 		break;						\
 	case 4:							\
-		__get_user_asm("lw", (x), __gu_ptr, __gu_err);	\
+		__get_mem_asm("lw", (x), __gu_ptr, __gu_err);	\
 		break;						\
 	case 8:							\
-		__get_user_8((x), __gu_ptr, __gu_err);	\
+		__get_mem_8((x), __gu_ptr, __gu_err);		\
 		break;						\
 	default:						\
 		BUILD_BUG();					\
@@ -136,7 +136,7 @@ do {								\
 	__chk_user_ptr(__gu_ptr);				\
 								\
 	__enable_user_access();					\
-	__get_user_nocheck(x, __gu_ptr, __gu_err);		\
+	__get_mem_nocheck(x, __gu_ptr, __gu_err);		\
 	__disable_user_access();				\
 								\
 	__gu_err;						\
@@ -168,7 +168,7 @@ do {								\
 		((x) = 0, -EFAULT);				\
 })
 
-#define __put_user_asm(insn, x, ptr, err)			\
+#define __put_mem_asm(insn, x, ptr, err)			\
 do {								\
 	__typeof__(*(ptr)) __x = x;				\
 	__asm__ __volatile__ (					\
@@ -181,12 +181,12 @@ do {								\
 } while (0)
 
 #ifdef CONFIG_64BIT
-#define __put_user_8(x, ptr, err) \
-	__put_user_asm("sd", x, ptr, err)
+#define __put_mem_8(x, ptr, err) \
+	__put_mem_asm("sd", x, ptr, err)
 #else /* !CONFIG_64BIT */
-#define __put_user_8(x, ptr, err)				\
+#define __put_mem_8(x, ptr, err)				\
 do {								\
-	u32 __user *__ptr = (u32 __user *)(ptr);		\
+	u32 *__ptr = (u32 *)(ptr);				\
 	u64 __x = (__typeof__((x)-(x)))(x);			\
 	__asm__ __volatile__ (					\
 		"1:\n"						\
@@ -203,20 +203,20 @@ do {								\
 } while (0)
 #endif /* CONFIG_64BIT */
 
-#define __put_user_nocheck(x, __gu_ptr, __pu_err)					\
+#define __put_mem_nocheck(x, __gu_ptr, __pu_err)		\
 do {								\
 	switch (sizeof(*__gu_ptr)) {				\
 	case 1:							\
-		__put_user_asm("sb", (x), __gu_ptr, __pu_err);	\
+		__put_mem_asm("sb", (x), __gu_ptr, __pu_err);	\
 		break;						\
 	case 2:							\
-		__put_user_asm("sh", (x), __gu_ptr, __pu_err);	\
+		__put_mem_asm("sh", (x), __gu_ptr, __pu_err);	\
 		break;						\
 	case 4:							\
-		__put_user_asm("sw", (x), __gu_ptr, __pu_err);	\
+		__put_mem_asm("sw", (x), __gu_ptr, __pu_err);	\
 		break;						\
 	case 8:							\
-		__put_user_8((x), __gu_ptr, __pu_err);	\
+		__put_mem_8((x), __gu_ptr, __pu_err);		\
 		break;						\
 	default:						\
 		BUILD_BUG();					\
@@ -253,7 +253,7 @@ do {								\
 	__chk_user_ptr(__gu_ptr);				\
 								\
 	__enable_user_access();					\
-	__put_user_nocheck(__val, __gu_ptr, __pu_err);		\
+	__put_mem_nocheck(__val, __gu_ptr, __pu_err);		\
 	__disable_user_access();				\
 								\
 	__pu_err;						\
@@ -321,7 +321,7 @@ unsigned long __must_check clear_user(void __user *to, unsigned long n)
 do {									\
 	long __kr_err;							\
 									\
-	__get_user_nocheck(*((type *)(dst)), (type *)(src), __kr_err);	\
+	__get_mem_nocheck(*((type *)(dst)), (type *)(src), __kr_err);	\
 	if (unlikely(__kr_err))						\
 		goto err_label;						\
 } while (0)
@@ -330,7 +330,7 @@ do {									\
 do {									\
 	long __kr_err;							\
 									\
-	__put_user_nocheck(*((type *)(src)), (type *)(dst), __kr_err);	\
+	__put_mem_nocheck(*((type *)(src)), (type *)(dst), __kr_err);	\
 	if (unlikely(__kr_err))						\
 		goto err_label;						\
 } while (0)
-- 
2.25.1

[PATCH -next v2 2/2] riscv: extable: add new extable type EX_TYPE_KACCESS_ERR_ZERO support

[Tong Tiangen]

Currently, The extable type EX_TYPE_UACCESS_ERR_ZERO is used by
__get/put_kernel_nofault(), but those helpers are not uaccess type, so we
add a new extable type EX_TYPE_KACCESS_ERR_ZERO which can be used by
__get/put_kernel_no_fault().

Only refactor code without any functional changes.

Signed-off-by: Tong Tiangen <tongtiangen@huawei.com>
---
 arch/riscv/include/asm/asm-extable.h |  12 ++
 arch/riscv/include/asm/uaccess.h     | 160 +++++++++++++--------------
 arch/riscv/mm/extable.c              |   1 +
 3 files changed, 93 insertions(+), 80 deletions(-)

diff --git a/arch/riscv/include/asm/asm-extable.h b/arch/riscv/include/asm/asm-extable.h
index 14be0673f5b5..73c70098a9c8 100644
--- a/arch/riscv/include/asm/asm-extable.h
+++ b/arch/riscv/include/asm/asm-extable.h
@@ -6,6 +6,7 @@
 #define EX_TYPE_FIXUP			1
 #define EX_TYPE_BPF			2
 #define EX_TYPE_UACCESS_ERR_ZERO	3
+#define EX_TYPE_KACCESS_ERR_ZERO	4
 
 #ifdef __ASSEMBLY__
 
@@ -57,9 +58,20 @@
 			    EX_DATA_REG(ZERO, zero)			\
 			  ")")
 
+#define _ASM_EXTABLE_KACCESS_ERR_ZERO(insn, fixup, err, zero)		\
+	__DEFINE_ASM_GPR_NUMS						\
+	__ASM_EXTABLE_RAW(#insn, #fixup,				\
+			  __stringify(EX_TYPE_KACCESS_ERR_ZERO),	\
+			  "("						\
+			    EX_DATA_REG(ERR, err) " | "			\
+			    EX_DATA_REG(ZERO, zero)			\
+			  ")")
+
 #define _ASM_EXTABLE_UACCESS_ERR(insn, fixup, err)			\
 	_ASM_EXTABLE_UACCESS_ERR_ZERO(insn, fixup, err, zero)
 
+#define _ASM_EXTABLE_KACCESS_ERR(insn, fixup, err)			\
+	_ASM_EXTABLE_KACCESS_ERR_ZERO(insn, fixup, err, zero)
 #endif /* __ASSEMBLY__ */
 
 #endif /* __ASM_ASM_EXTABLE_H */
diff --git a/arch/riscv/include/asm/uaccess.h b/arch/riscv/include/asm/uaccess.h
index 1370da055b44..a79af147636e 100644
--- a/arch/riscv/include/asm/uaccess.h
+++ b/arch/riscv/include/asm/uaccess.h
@@ -50,62 +50,62 @@
  * call.
  */
 
-#define __get_mem_asm(insn, x, ptr, err)			\
-do {								\
-	__typeof__(x) __x;					\
-	__asm__ __volatile__ (					\
-		"1:\n"						\
-		"	" insn " %1, %2\n"			\
-		"2:\n"						\
-		_ASM_EXTABLE_UACCESS_ERR_ZERO(1b, 2b, %0, %1)	\
-		: "+r" (err), "=&r" (__x)			\
-		: "m" (*(ptr)));				\
-	(x) = __x;						\
+#define __get_mem_asm(insn, x, ptr, err, type)				\
+do {									\
+	__typeof__(x) __x;						\
+	__asm__ __volatile__ (						\
+		"1:\n"							\
+		"	" insn " %1, %2\n"				\
+		"2:\n"							\
+		_ASM_EXTABLE_##type##ACCESS_ERR_ZERO(1b, 2b, %0, %1)	\
+		: "+r" (err), "=&r" (__x)				\
+		: "m" (*(ptr)));					\
+	(x) = __x;							\
 } while (0)
 
 #ifdef CONFIG_64BIT
-#define __get_mem_8(x, ptr, err) \
-	__get_mem_asm("ld", x, ptr, err)
+#define __get_mem_8(x, ptr, err, type) \
+	__get_mem_asm("ld", x, ptr, err, type)
 #else /* !CONFIG_64BIT */
-#define __get_mem_8(x, ptr, err)				\
-do {								\
-	u32 *__ptr = (u32 *)(ptr);				\
-	u32 __lo, __hi;						\
-	__asm__ __volatile__ (					\
-		"1:\n"						\
-		"	lw %1, %3\n"				\
-		"2:\n"						\
-		"	lw %2, %4\n"				\
-		"3:\n"						\
-		_ASM_EXTABLE_UACCESS_ERR_ZERO(1b, 3b, %0, %1)	\
-		_ASM_EXTABLE_UACCESS_ERR_ZERO(2b, 3b, %0, %1)	\
-		: "+r" (err), "=&r" (__lo), "=r" (__hi)		\
-		: "m" (__ptr[__LSW]), "m" (__ptr[__MSW]));	\
-	if (err)						\
-		__hi = 0;					\
-	(x) = (__typeof__(x))((__typeof__((x)-(x)))(		\
-		(((u64)__hi << 32) | __lo)));			\
+#define __get_mem_8(x, ptr, err, type)					\
+do {									\
+	u32 *__ptr = (u32 *)(ptr);					\
+	u32 __lo, __hi;							\
+	__asm__ __volatile__ (						\
+		"1:\n"							\
+		"	lw %1, %3\n"					\
+		"2:\n"							\
+		"	lw %2, %4\n"					\
+		"3:\n"							\
+		_ASM_EXTABLE_##type##ACCESS_ERR_ZERO(1b, 3b, %0, %1)	\
+		_ASM_EXTABLE_##type##ACCESS_ERR_ZERO(2b, 3b, %0, %1)	\
+		: "+r" (err), "=&r" (__lo), "=r" (__hi)			\
+		: "m" (__ptr[__LSW]), "m" (__ptr[__MSW]));		\
+	if (err)							\
+		__hi = 0;						\
+	(x) = (__typeof__(x))((__typeof__((x)-(x)))(			\
+		(((u64)__hi << 32) | __lo)));				\
 } while (0)
 #endif /* CONFIG_64BIT */
 
-#define __get_mem_nocheck(x, __gu_ptr, __gu_err)		\
-do {								\
-	switch (sizeof(*__gu_ptr)) {				\
-	case 1:							\
-		__get_mem_asm("lb", (x), __gu_ptr, __gu_err);	\
-		break;						\
-	case 2:							\
-		__get_mem_asm("lh", (x), __gu_ptr, __gu_err);	\
-		break;						\
-	case 4:							\
-		__get_mem_asm("lw", (x), __gu_ptr, __gu_err);	\
-		break;						\
-	case 8:							\
-		__get_mem_8((x), __gu_ptr, __gu_err);		\
-		break;						\
-	default:						\
-		BUILD_BUG();					\
-	}							\
+#define __get_mem_nocheck(x, __gu_ptr, __gu_err, type)			\
+do {									\
+	switch (sizeof(*__gu_ptr)) {					\
+	case 1:								\
+		__get_mem_asm("lb", (x), __gu_ptr, __gu_err, type);	\
+		break;							\
+	case 2:								\
+		__get_mem_asm("lh", (x), __gu_ptr, __gu_err, type);	\
+		break;							\
+	case 4:								\
+		__get_mem_asm("lw", (x), __gu_ptr, __gu_err, type);	\
+		break;							\
+	case 8:								\
+		__get_mem_8((x), __gu_ptr, __gu_err, type);		\
+		break;							\
+	default:							\
+		BUILD_BUG();						\
+	}								\
 } while (0)
 
 /**
@@ -136,7 +136,7 @@ do {								\
 	__chk_user_ptr(__gu_ptr);				\
 								\
 	__enable_user_access();					\
-	__get_mem_nocheck(x, __gu_ptr, __gu_err);		\
+	__get_mem_nocheck(x, __gu_ptr, __gu_err, U);		\
 	__disable_user_access();				\
 								\
 	__gu_err;						\
@@ -163,28 +163,28 @@ do {								\
 ({								\
 	const __typeof__(*(ptr)) __user *__p = (ptr);		\
 	might_fault();						\
-	access_ok(__p, sizeof(*__p)) ?		\
+	access_ok(__p, sizeof(*__p)) ?				\
 		__get_user((x), __p) :				\
 		((x) = 0, -EFAULT);				\
 })
 
-#define __put_mem_asm(insn, x, ptr, err)			\
+#define __put_mem_asm(insn, x, ptr, err, type)			\
 do {								\
 	__typeof__(*(ptr)) __x = x;				\
 	__asm__ __volatile__ (					\
 		"1:\n"						\
 		"	" insn " %z2, %1\n"			\
 		"2:\n"						\
-		_ASM_EXTABLE_UACCESS_ERR(1b, 2b, %0)		\
+		_ASM_EXTABLE_##type##ACCESS_ERR(1b, 2b, %0)	\
 		: "+r" (err), "=m" (*(ptr))			\
 		: "rJ" (__x));					\
 } while (0)
 
 #ifdef CONFIG_64BIT
-#define __put_mem_8(x, ptr, err) \
-	__put_mem_asm("sd", x, ptr, err)
+#define __put_mem_8(x, ptr, err, type) \
+	__put_mem_asm("sd", x, ptr, err, type)
 #else /* !CONFIG_64BIT */
-#define __put_mem_8(x, ptr, err)				\
+#define __put_mem_8(x, ptr, err, type)				\
 do {								\
 	u32 *__ptr = (u32 *)(ptr);				\
 	u64 __x = (__typeof__((x)-(x)))(x);			\
@@ -194,8 +194,8 @@ do {								\
 		"2:\n"						\
 		"	sw %z4, %2\n"				\
 		"3:\n"						\
-		_ASM_EXTABLE_UACCESS_ERR(1b, 3b, %0)		\
-		_ASM_EXTABLE_UACCESS_ERR(2b, 3b, %0)		\
+		_ASM_EXTABLE_##type##ACCESS_ERR(1b, 3b, %0)	\
+		_ASM_EXTABLE_##type##ACCESS_ERR(2b, 3b, %0)	\
 		: "+r" (err),					\
 			"=m" (__ptr[__LSW]),			\
 			"=m" (__ptr[__MSW])			\
@@ -203,24 +203,24 @@ do {								\
 } while (0)
 #endif /* CONFIG_64BIT */
 
-#define __put_mem_nocheck(x, __gu_ptr, __pu_err)		\
-do {								\
-	switch (sizeof(*__gu_ptr)) {				\
-	case 1:							\
-		__put_mem_asm("sb", (x), __gu_ptr, __pu_err);	\
-		break;						\
-	case 2:							\
-		__put_mem_asm("sh", (x), __gu_ptr, __pu_err);	\
-		break;						\
-	case 4:							\
-		__put_mem_asm("sw", (x), __gu_ptr, __pu_err);	\
-		break;						\
-	case 8:							\
-		__put_mem_8((x), __gu_ptr, __pu_err);		\
-		break;						\
-	default:						\
-		BUILD_BUG();					\
-	}							\
+#define __put_mem_nocheck(x, __gu_ptr, __pu_err, type)			\
+do {									\
+	switch (sizeof(*__gu_ptr)) {					\
+	case 1:								\
+		__put_mem_asm("sb", (x), __gu_ptr, __pu_err, type);	\
+		break;							\
+	case 2:								\
+		__put_mem_asm("sh", (x), __gu_ptr, __pu_err, type);	\
+		break;							\
+	case 4:								\
+		__put_mem_asm("sw", (x), __gu_ptr, __pu_err, type);	\
+		break;							\
+	case 8:								\
+		__put_mem_8((x), __gu_ptr, __pu_err, type);		\
+		break;							\
+	default:							\
+		BUILD_BUG();						\
+	}								\
 } while (0)
 
 /**
@@ -253,7 +253,7 @@ do {								\
 	__chk_user_ptr(__gu_ptr);				\
 								\
 	__enable_user_access();					\
-	__put_mem_nocheck(__val, __gu_ptr, __pu_err);		\
+	__put_mem_nocheck(__val, __gu_ptr, __pu_err, U);	\
 	__disable_user_access();				\
 								\
 	__pu_err;						\
@@ -279,7 +279,7 @@ do {								\
 ({								\
 	__typeof__(*(ptr)) __user *__p = (ptr);			\
 	might_fault();						\
-	access_ok(__p, sizeof(*__p)) ?		\
+	access_ok(__p, sizeof(*__p)) ?				\
 		__put_user((x), __p) :				\
 		-EFAULT;					\
 })
@@ -321,7 +321,7 @@ unsigned long __must_check clear_user(void __user *to, unsigned long n)
 do {									\
 	long __kr_err;							\
 									\
-	__get_mem_nocheck(*((type *)(dst)), (type *)(src), __kr_err);	\
+	__get_mem_nocheck(*((type *)(dst)), (type *)(src), __kr_err, K);\
 	if (unlikely(__kr_err))						\
 		goto err_label;						\
 } while (0)
@@ -330,7 +330,7 @@ do {									\
 do {									\
 	long __kr_err;							\
 									\
-	__put_mem_nocheck(*((type *)(src)), (type *)(dst), __kr_err);	\
+	__put_mem_nocheck(*((type *)(src)), (type *)(dst), __kr_err, K);\
 	if (unlikely(__kr_err))						\
 		goto err_label;						\
 } while (0)
diff --git a/arch/riscv/mm/extable.c b/arch/riscv/mm/extable.c
index 35484d830fd6..a21ad8237189 100644
--- a/arch/riscv/mm/extable.c
+++ b/arch/riscv/mm/extable.c
@@ -64,6 +64,7 @@ bool fixup_exception(struct pt_regs *regs)
 	case EX_TYPE_BPF:
 		return ex_handler_bpf(ex, regs);
 	case EX_TYPE_UACCESS_ERR_ZERO:
+	case EX_TYPE_KACCESS_ERR_ZERO:
 		return ex_handler_uaccess_err_zero(ex, regs);
 	}
 
-- 
2.25.1

Re: [PATCH -next v2 0/2]riscv: some refactorings realted to uaccess and extable

[Tong Tiangen]

Hi riscv maintainers, kindly ping...

Thanks,
Tong.

在 2022/8/15 11:20, Tong Tiangen 写道:
> This patchset do some refactorings related to riscv's uaccess and extable,
> mainly for the usage of __get/put_user_nocheck() which not distinguish user
> access and kernel access.
> 
> v1 -> v2:
>    According to Conor's suggestion, split into two logically independent
>    patches.
> 
> Tong Tiangen (2):
>    riscv: uaccess: rename __get/put_user_nocheck to __get/put_mem_nocheck
>    riscv: extable: add new extable type EX_TYPE_KACCESS_ERR_ZERO support
> 
>   arch/riscv/include/asm/asm-extable.h |  12 ++
>   arch/riscv/include/asm/uaccess.h     | 162 +++++++++++++--------------
>   arch/riscv/mm/extable.c              |   1 +
>   3 files changed, 94 insertions(+), 81 deletions(-)
> 

Re: [PATCH -next v2 0/2]riscv: some refactorings realted to uaccess and extable

[Conor.Dooley]

On 24/08/2022 07:31, Tong Tiangen wrote:
> Hi riscv maintainers, kindly ping...
> 
> Thanks,
> Tong.
> 
> 在 2022/8/15 11:20, Tong Tiangen 写道:

It's barely been more than a week, relax :)

checkpatch really does not like one of the macros you added. Please
consider whether this is valid:

ERROR: Macros with complex values should be enclosed in parentheses
#38: FILE: arch/riscv/include/asm/asm-extable.h:61:
+#define _ASM_EXTABLE_KACCESS_ERR_ZERO(insn, fixup, err, zero)          \
+       __DEFINE_ASM_GPR_NUMS                                           \
+       __ASM_EXTABLE_RAW(#insn, #fixup,                                \
+                         __stringify(EX_TYPE_KACCESS_ERR_ZERO),        \
+                         "("                                           \
+                           EX_DATA_REG(ERR, err) " | "                 \
+                           EX_DATA_REG(ZERO, zero)                     \
+                         ")")

Thanks,
Conor.

>> This patchset do some refactorings related to riscv's uaccess and extable,
>> mainly for the usage of __get/put_user_nocheck() which not distinguish user
>> access and kernel access.
>>
>> v1 -> v2:
>>    According to Conor's suggestion, split into two logically independent
>>    patches.
>>
>> Tong Tiangen (2):
>>    riscv: uaccess: rename __get/put_user_nocheck to __get/put_mem_nocheck
>>    riscv: extable: add new extable type EX_TYPE_KACCESS_ERR_ZERO support
>>
>>   arch/riscv/include/asm/asm-extable.h |  12 ++
>>   arch/riscv/include/asm/uaccess.h     | 162 +++++++++++++--------------
>>   arch/riscv/mm/extable.c              |   1 +
>>   3 files changed, 94 insertions(+), 81 deletions(-)
>>

Re: [PATCH -next v2 0/2]riscv: some refactorings realted to uaccess and extable

[Tong Tiangen]

在 2022/8/25 0:49, Conor.Dooley@microchip.com 写道:
> On 24/08/2022 07:31, Tong Tiangen wrote:
>> Hi riscv maintainers, kindly ping...
>>
>> Thanks,
>> Tong.
>>
>> 在 2022/8/15 11:20, Tong Tiangen 写道:
> 
> It's barely been more than a week, relax :)
> 
> checkpatch really does not like one of the macros you added. Please
> consider whether this is valid:
> 
> ERROR: Macros with complex values should be enclosed in parentheses
> #38: FILE: arch/riscv/include/asm/asm-extable.h:61:
> +#define _ASM_EXTABLE_KACCESS_ERR_ZERO(insn, fixup, err, zero)          \
> +       __DEFINE_ASM_GPR_NUMS                                           \
> +       __ASM_EXTABLE_RAW(#insn, #fixup,                                \
> +                         __stringify(EX_TYPE_KACCESS_ERR_ZERO),        \
> +                         "("                                           \
> +                           EX_DATA_REG(ERR, err) " | "                 \
> +                           EX_DATA_REG(ZERO, zero)                     \
> +                         ")")
> 
> Thanks,
> Conor.


Judging from the use context of this macro, there is no problem with the 
definition of this macro.

In addition, I refer to the definition of macro 
_ASM_EXTABLE_UACCESS_ERR_ZERO for the style of this macro. The 
difference is that the types used in the macro are different.

:)

Thanks,
Tong.

> 
>>> This patchset do some refactorings related to riscv's uaccess and extable,
>>> mainly for the usage of __get/put_user_nocheck() which not distinguish user
>>> access and kernel access.
>>>
>>> v1 -> v2:
>>>     According to Conor's suggestion, split into two logically independent
>>>     patches.
>>>
>>> Tong Tiangen (2):
>>>     riscv: uaccess: rename __get/put_user_nocheck to __get/put_mem_nocheck
>>>     riscv: extable: add new extable type EX_TYPE_KACCESS_ERR_ZERO support
>>>
>>>    arch/riscv/include/asm/asm-extable.h |  12 ++
>>>    arch/riscv/include/asm/uaccess.h     | 162 +++++++++++++--------------
>>>    arch/riscv/mm/extable.c              |   1 +
>>>    3 files changed, 94 insertions(+), 81 deletions(-)
>>>

Re: [PATCH -next v2 1/2] riscv: uaccess: rename __get/put_user_nocheck to __get/put_mem_nocheck

[Andrew Jones]

On Mon, Aug 15, 2022 at 03:20:24AM +0000, Tong Tiangen wrote:
> Current, The helpers __get/put_user_nocheck() is used by get/put_user() and
> __get/put_kernel_nofault(), which is not always uaccess, so the name with
> *user* is not appropriate.
> 
> Also rename xxx_user_xxx to xxx_mem_xx  on the call path of
> __get/put_user_nocheck()
> 
> Only refactor code without any functional changes.
> 
> Signed-off-by: Tong Tiangen <tongtiangen@huawei.com>
> ---
>  arch/riscv/include/asm/uaccess.h | 48 ++++++++++++++++----------------
>  1 file changed, 24 insertions(+), 24 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/uaccess.h b/arch/riscv/include/asm/uaccess.h
> index 855450bed9f5..1370da055b44 100644
> --- a/arch/riscv/include/asm/uaccess.h
> +++ b/arch/riscv/include/asm/uaccess.h
> @@ -50,7 +50,7 @@
>   * call.
>   */
>  
> -#define __get_user_asm(insn, x, ptr, err)			\
> +#define __get_mem_asm(insn, x, ptr, err)			\
>  do {								\
>  	__typeof__(x) __x;					\
>  	__asm__ __volatile__ (					\
> @@ -64,12 +64,12 @@ do {								\
>  } while (0)
>  
>  #ifdef CONFIG_64BIT
> -#define __get_user_8(x, ptr, err) \
> -	__get_user_asm("ld", x, ptr, err)
> +#define __get_mem_8(x, ptr, err) \
> +	__get_mem_asm("ld", x, ptr, err)
>  #else /* !CONFIG_64BIT */
> -#define __get_user_8(x, ptr, err)				\
> +#define __get_mem_8(x, ptr, err)				\
>  do {								\
> -	u32 __user *__ptr = (u32 __user *)(ptr);		\
> +	u32 *__ptr = (u32 *)(ptr);				\

Doesn't casting away __user reduce sparse's utility?

>  	u32 __lo, __hi;						\
>  	__asm__ __volatile__ (					\
>  		"1:\n"						\
> @@ -88,20 +88,20 @@ do {								\
>  } while (0)
>  #endif /* CONFIG_64BIT */
>  
> -#define __get_user_nocheck(x, __gu_ptr, __gu_err)		\
> +#define __get_mem_nocheck(x, __gu_ptr, __gu_err)		\

The patch replaces all get/put_user instances with get/put_mem,
but what about 'gu' and 'pu' instances (which are presumably short
for get/put_user)?

Thanks,
drew

Re: [PATCH -next v2 2/2] riscv: extable: add new extable type EX_TYPE_KACCESS_ERR_ZERO support

[Andrew Jones]

On Mon, Aug 15, 2022 at 03:20:25AM +0000, Tong Tiangen wrote:
> Currently, The extable type EX_TYPE_UACCESS_ERR_ZERO is used by
> __get/put_kernel_nofault(), but those helpers are not uaccess type, so we
> add a new extable type EX_TYPE_KACCESS_ERR_ZERO which can be used by
> __get/put_kernel_no_fault().
> 
> Only refactor code without any functional changes.

This isn't quite true. __get/put_kernel_nofault now sets a different
extable type (as the commit message says). But, nothing special seems
to be done with that, so there's effectively no functional change. Can
you please elaborate on the motivation for this change? Where will the
KACCESS type need to be distinguished from the UACCESS type?

Thanks,
drew

Re: [PATCH -next v2 1/2] riscv: uaccess: rename __get/put_user_nocheck to __get/put_mem_nocheck

[Tong Tiangen]

在 2022/8/25 18:56, Andrew Jones 写道:
> On Mon, Aug 15, 2022 at 03:20:24AM +0000, Tong Tiangen wrote:
>> Current, The helpers __get/put_user_nocheck() is used by get/put_user() and
>> __get/put_kernel_nofault(), which is not always uaccess, so the name with
>> *user* is not appropriate.
>>
>> Also rename xxx_user_xxx to xxx_mem_xx  on the call path of
>> __get/put_user_nocheck()
>>
>> Only refactor code without any functional changes.
>>
>> Signed-off-by: Tong Tiangen <tongtiangen@huawei.com>
>> ---
>>   arch/riscv/include/asm/uaccess.h | 48 ++++++++++++++++----------------
>>   1 file changed, 24 insertions(+), 24 deletions(-)
>>
>> diff --git a/arch/riscv/include/asm/uaccess.h b/arch/riscv/include/asm/uaccess.h
>> index 855450bed9f5..1370da055b44 100644
>> --- a/arch/riscv/include/asm/uaccess.h
>> +++ b/arch/riscv/include/asm/uaccess.h
>> @@ -50,7 +50,7 @@
>>    * call.
>>    */
>>   
>> -#define __get_user_asm(insn, x, ptr, err)			\
>> +#define __get_mem_asm(insn, x, ptr, err)			\
>>   do {								\
>>   	__typeof__(x) __x;					\
>>   	__asm__ __volatile__ (					\
>> @@ -64,12 +64,12 @@ do {								\
>>   } while (0)
>>   
>>   #ifdef CONFIG_64BIT
>> -#define __get_user_8(x, ptr, err) \
>> -	__get_user_asm("ld", x, ptr, err)
>> +#define __get_mem_8(x, ptr, err) \
>> +	__get_mem_asm("ld", x, ptr, err)
>>   #else /* !CONFIG_64BIT */
>> -#define __get_user_8(x, ptr, err)				\
>> +#define __get_mem_8(x, ptr, err)				\
>>   do {								\
>> -	u32 __user *__ptr = (u32 __user *)(ptr);		\
>> +	u32 *__ptr = (u32 *)(ptr);				\
> 
> Doesn't casting away __user reduce sparse's utility?

 From the call logic[1], the address passed into this macro is not 
necessarily __user. I understand that no problem will be introduced for 
sparse's utility.

In addition, there is no need to do a pointer conversion here, will be 
fixed next version.

[1] __get_kernel_nofault -> __get_mem_nocheck -> __get_mem_8
> 
>>   	u32 __lo, __hi;						\
>>   	__asm__ __volatile__ (					\
>>   		"1:\n"						\
>> @@ -88,20 +88,20 @@ do {								\
>>   } while (0)
>>   #endif /* CONFIG_64BIT */
>>   
>> -#define __get_user_nocheck(x, __gu_ptr, __gu_err)		\
>> +#define __get_mem_nocheck(x, __gu_ptr, __gu_err)		\
> 
> The patch replaces all get/put_user instances with get/put_mem,
> but what about 'gu' and 'pu' instances (which are presumably short
> for get/put_user)?

ok, missing that, It is not appropriate to use __gu_xxx，will be fixed 
next version.

Thanks,
Tong.

> 
> Thanks,
> drew
> .

Re: [PATCH -next v2 2/2] riscv: extable: add new extable type EX_TYPE_KACCESS_ERR_ZERO support

[Tong Tiangen]

在 2022/8/25 19:06, Andrew Jones 写道:
> On Mon, Aug 15, 2022 at 03:20:25AM +0000, Tong Tiangen wrote:
>> Currently, The extable type EX_TYPE_UACCESS_ERR_ZERO is used by
>> __get/put_kernel_nofault(), but those helpers are not uaccess type, so we
>> add a new extable type EX_TYPE_KACCESS_ERR_ZERO which can be used by
>> __get/put_kernel_no_fault().
>>
>> Only refactor code without any functional changes.
> 
> This isn't quite true. __get/put_kernel_nofault now sets a different
> extable type (as the commit message says). But, nothing special seems
> to be done with that, so there's effectively no functional change. Can
> you please elaborate on the motivation for this change? Where will the
> KACCESS type need to be distinguished from the UACCESS type?

The introduction of EX_TYPE_KACCESS_ERR_ZERO does not change any 
function, but makes a correct distinction in the actual type, indicating 
that there are indeed some kaccess entries in extable. I think this 
optimization is more clear and reasonable.

A few weeks ago, I did something similar on arm64[1]. I think this 
optimization can also be used on riscv.

We can do some features that are used on uaccss but not applicable on 
kaccess in the future[2].

[1] 
https://lore.kernel.org/lkml/20220621072638.1273594-2-tongtiangen@huawei.com/
[2]https://lore.kernel.org/lkml/20220812070557.1028499-4-tongtiangen@huawei.com/

Thanks,
Tong.

> 
> Thanks,
> drew
> .

Re: [PATCH -next v2 1/2] riscv: uaccess: rename __get/put_user_nocheck to __get/put_mem_nocheck

[Andrew Jones]

On Fri, Aug 26, 2022 at 02:33:47PM +0800, Tong Tiangen wrote:
> 
> 
> 在 2022/8/25 18:56, Andrew Jones 写道:
> > On Mon, Aug 15, 2022 at 03:20:24AM +0000, Tong Tiangen wrote:
> > > Current, The helpers __get/put_user_nocheck() is used by get/put_user() and
> > > __get/put_kernel_nofault(), which is not always uaccess, so the name with
> > > *user* is not appropriate.
> > > 
> > > Also rename xxx_user_xxx to xxx_mem_xx  on the call path of
> > > __get/put_user_nocheck()
> > > 
> > > Only refactor code without any functional changes.
> > > 
> > > Signed-off-by: Tong Tiangen <tongtiangen@huawei.com>
> > > ---
> > >   arch/riscv/include/asm/uaccess.h | 48 ++++++++++++++++----------------
> > >   1 file changed, 24 insertions(+), 24 deletions(-)
> > > 
> > > diff --git a/arch/riscv/include/asm/uaccess.h b/arch/riscv/include/asm/uaccess.h
> > > index 855450bed9f5..1370da055b44 100644
> > > --- a/arch/riscv/include/asm/uaccess.h
> > > +++ b/arch/riscv/include/asm/uaccess.h
> > > @@ -50,7 +50,7 @@
> > >    * call.
> > >    */
> > > -#define __get_user_asm(insn, x, ptr, err)			\
> > > +#define __get_mem_asm(insn, x, ptr, err)			\
> > >   do {								\
> > >   	__typeof__(x) __x;					\
> > >   	__asm__ __volatile__ (					\
> > > @@ -64,12 +64,12 @@ do {								\
> > >   } while (0)
> > >   #ifdef CONFIG_64BIT
> > > -#define __get_user_8(x, ptr, err) \
> > > -	__get_user_asm("ld", x, ptr, err)
> > > +#define __get_mem_8(x, ptr, err) \
> > > +	__get_mem_asm("ld", x, ptr, err)
> > >   #else /* !CONFIG_64BIT */
> > > -#define __get_user_8(x, ptr, err)				\
> > > +#define __get_mem_8(x, ptr, err)				\
> > >   do {								\
> > > -	u32 __user *__ptr = (u32 __user *)(ptr);		\
> > > +	u32 *__ptr = (u32 *)(ptr);				\
> > 
> > Doesn't casting away __user reduce sparse's utility?
> 
> From the call logic[1], the address passed into this macro is not
> necessarily __user. I understand that no problem will be introduced for
> sparse's utility.
> 
> In addition, there is no need to do a pointer conversion here, will be fixed
> next version.
> 
> [1] __get_kernel_nofault -> __get_mem_nocheck -> __get_mem_8

Yes, I understood that. My concern was for the times that the address was
__user as we'd no longer get that check for them.

Thanks,
drew

Re: [PATCH -next v2 2/2] riscv: extable: add new extable type EX_TYPE_KACCESS_ERR_ZERO support

[Andrew Jones]

On Fri, Aug 26, 2022 at 02:44:48PM +0800, Tong Tiangen wrote:
> 
> 
> 在 2022/8/25 19:06, Andrew Jones 写道:
> > On Mon, Aug 15, 2022 at 03:20:25AM +0000, Tong Tiangen wrote:
> > > Currently, The extable type EX_TYPE_UACCESS_ERR_ZERO is used by
> > > __get/put_kernel_nofault(), but those helpers are not uaccess type, so we
> > > add a new extable type EX_TYPE_KACCESS_ERR_ZERO which can be used by
> > > __get/put_kernel_no_fault().
> > > 
> > > Only refactor code without any functional changes.
> > 
> > This isn't quite true. __get/put_kernel_nofault now sets a different
> > extable type (as the commit message says). But, nothing special seems
> > to be done with that, so there's effectively no functional change. Can
> > you please elaborate on the motivation for this change? Where will the
> > KACCESS type need to be distinguished from the UACCESS type?
> 
> The introduction of EX_TYPE_KACCESS_ERR_ZERO does not change any function,
> but makes a correct distinction in the actual type, indicating that there
> are indeed some kaccess entries in extable. I think this optimization is
> more clear and reasonable.

Well, creating new types, just for new type sake, just bloats code.

> 
> A few weeks ago, I did something similar on arm64[1]. I think this
> optimization can also be used on riscv.
> 
> We can do some features that are used on uaccss but not applicable on
> kaccess in the future[2].
> 
> [1]
> https://lore.kernel.org/lkml/20220621072638.1273594-2-tongtiangen@huawei.com/
> [2]https://lore.kernel.org/lkml/20220812070557.1028499-4-tongtiangen@huawei.com/
> 

This is part of the information, but I had already found this. What's
still missing to me are the riscv patches, or at least a riscv plan, for
actually implementing something which requires kaccess and uaccess to have
distinct types.

Thanks,
drew

Re: [PATCH -next v2 1/2] riscv: uaccess: rename __get/put_user_nocheck to __get/put_mem_nocheck

[Arnd Bergmann]

On Mon, Aug 15, 2022 at 5:20 AM Tong Tiangen <tongtiangen@huawei.com> wrote:
>
> Current, The helpers __get/put_user_nocheck() is used by get/put_user() and
> __get/put_kernel_nofault(), which is not always uaccess, so the name with
> *user* is not appropriate.
>
> Also rename xxx_user_xxx to xxx_mem_xx  on the call path of
> __get/put_user_nocheck()
>
> Only refactor code without any functional changes.
>
> Signed-off-by: Tong Tiangen <tongtiangen@huawei.com>

I would prefer this not being done, it just makes riscv diverge from the
code on other architectures. While the new name does make more sense,
it ends up making it harder to refactor this across architectures in the end.

There are two important cleanups that I would like to see done in
asm/uaccess.h across architectures:

 - generalize the __get_user()/__put_user()/__get_kernel_nofault()/
  __put_kernel_nofault() wrappers to the point that architectures do not
  need to worry about the variable type stuff but instead just provide
  trivial fixed-length helpers of some sort

- change the calling conventions in a way that allows the use of the
  asm-goto-with-output method for better object code on modern
  compilers.

The x86 version already has most of this, with their
__get_user_size() macro supporting both the asm-goto label
and the error code assignment, so the generalized code should
probably be based on that approach.

       Arnd

Re: [PATCH -next v2 2/2] riscv: extable: add new extable type EX_TYPE_KACCESS_ERR_ZERO support

[Tong Tiangen]

在 2022/8/26 16:16, Andrew Jones 写道:
> On Fri, Aug 26, 2022 at 02:44:48PM +0800, Tong Tiangen wrote:
>>
>>
>> 在 2022/8/25 19:06, Andrew Jones 写道:
>>> On Mon, Aug 15, 2022 at 03:20:25AM +0000, Tong Tiangen wrote:
>>>> Currently, The extable type EX_TYPE_UACCESS_ERR_ZERO is used by
>>>> __get/put_kernel_nofault(), but those helpers are not uaccess type, so we
>>>> add a new extable type EX_TYPE_KACCESS_ERR_ZERO which can be used by
>>>> __get/put_kernel_no_fault().
>>>>
>>>> Only refactor code without any functional changes.
>>>
>>> This isn't quite true. __get/put_kernel_nofault now sets a different
>>> extable type (as the commit message says). But, nothing special seems
>>> to be done with that, so there's effectively no functional change. Can
>>> you please elaborate on the motivation for this change? Where will the
>>> KACCESS type need to be distinguished from the UACCESS type?
>>
>> The introduction of EX_TYPE_KACCESS_ERR_ZERO does not change any function,
>> but makes a correct distinction in the actual type, indicating that there
>> are indeed some kaccess entries in extable. I think this optimization is
>> more clear and reasonable.
> 
> Well, creating new types, just for new type sake, just bloats code.
> 
>>
>> A few weeks ago, I did something similar on arm64[1]. I think this
>> optimization can also be used on riscv.
>>
>> We can do some features that are used on uaccss but not applicable on
>> kaccess in the future[2].
>>
>> [1]
>> https://lore.kernel.org/lkml/20220621072638.1273594-2-tongtiangen@huawei.com/
>> [2]https://lore.kernel.org/lkml/20220812070557.1028499-4-tongtiangen@huawei.com/
>>
> 
> This is part of the information, but I had already found this. What's
> still missing to me are the riscv patches, or at least a riscv plan, for
> actually implementing something which requires kaccess and uaccess to have
> distinct types.
> 
> Thanks,
> drew

At present, there is no such plan on riscv, because it is rely on 
hardware support.
I think this patch can be merged as a small code optimization and 
without any function change.

Thanks,
Tong.

> 
> .

Re: [PATCH -next v2 1/2] riscv: uaccess: rename __get/put_user_nocheck to __get/put_mem_nocheck

[Tong Tiangen]

在 2022/8/26 15:43, Andrew Jones 写道:
> On Fri, Aug 26, 2022 at 02:33:47PM +0800, Tong Tiangen wrote:
>>
>>
>> 在 2022/8/25 18:56, Andrew Jones 写道:
>>> On Mon, Aug 15, 2022 at 03:20:24AM +0000, Tong Tiangen wrote:
>>>> Current, The helpers __get/put_user_nocheck() is used by get/put_user() and
>>>> __get/put_kernel_nofault(), which is not always uaccess, so the name with
>>>> *user* is not appropriate.
>>>>
>>>> Also rename xxx_user_xxx to xxx_mem_xx  on the call path of
>>>> __get/put_user_nocheck()
>>>>
>>>> Only refactor code without any functional changes.
>>>>
>>>> Signed-off-by: Tong Tiangen <tongtiangen@huawei.com>
>>>> ---
>>>>    arch/riscv/include/asm/uaccess.h | 48 ++++++++++++++++----------------
>>>>    1 file changed, 24 insertions(+), 24 deletions(-)
>>>>
>>>> diff --git a/arch/riscv/include/asm/uaccess.h b/arch/riscv/include/asm/uaccess.h
>>>> index 855450bed9f5..1370da055b44 100644
>>>> --- a/arch/riscv/include/asm/uaccess.h
>>>> +++ b/arch/riscv/include/asm/uaccess.h
>>>> @@ -50,7 +50,7 @@
>>>>     * call.
>>>>     */
>>>> -#define __get_user_asm(insn, x, ptr, err)			\
>>>> +#define __get_mem_asm(insn, x, ptr, err)			\
>>>>    do {								\
>>>>    	__typeof__(x) __x;					\
>>>>    	__asm__ __volatile__ (					\
>>>> @@ -64,12 +64,12 @@ do {								\
>>>>    } while (0)
>>>>    #ifdef CONFIG_64BIT
>>>> -#define __get_user_8(x, ptr, err) \
>>>> -	__get_user_asm("ld", x, ptr, err)
>>>> +#define __get_mem_8(x, ptr, err) \
>>>> +	__get_mem_asm("ld", x, ptr, err)
>>>>    #else /* !CONFIG_64BIT */
>>>> -#define __get_user_8(x, ptr, err)				\
>>>> +#define __get_mem_8(x, ptr, err)				\
>>>>    do {								\
>>>> -	u32 __user *__ptr = (u32 __user *)(ptr);		\
>>>> +	u32 *__ptr = (u32 *)(ptr);				\
>>>
>>> Doesn't casting away __user reduce sparse's utility?
>>
>>  From the call logic[1], the address passed into this macro is not
>> necessarily __user. I understand that no problem will be introduced for
>> sparse's utility.
>>
>> In addition, there is no need to do a pointer conversion here, will be fixed
>> next version.
>>
>> [1] __get_kernel_nofault -> __get_mem_nocheck -> __get_mem_8
> 
> Yes, I understood that. My concern was for the times that the address was
> __user as we'd no longer get that check for them.

Check __user ptr at __get_user() has the same effect? Is this 
understanding correct？

Thanks,
Tong.

> 
> Thanks,
> drew
> 
> .

Re: [PATCH -next v2 1/2] riscv: uaccess: rename __get/put_user_nocheck to __get/put_mem_nocheck

[Tong Tiangen]

在 2022/8/26 17:30, Arnd Bergmann 写道:
> On Mon, Aug 15, 2022 at 5:20 AM Tong Tiangen <tongtiangen@huawei.com> wrote:
>>
>> Current, The helpers __get/put_user_nocheck() is used by get/put_user() and
>> __get/put_kernel_nofault(), which is not always uaccess, so the name with
>> *user* is not appropriate.
>>
>> Also rename xxx_user_xxx to xxx_mem_xx  on the call path of
>> __get/put_user_nocheck()
>>
>> Only refactor code without any functional changes.
>>
>> Signed-off-by: Tong Tiangen <tongtiangen@huawei.com>
> 
> I would prefer this not being done, it just makes riscv diverge from the
> code on other architectures. While the new name does make more sense,
> it ends up making it harder to refactor this across architectures in the end.
> 
> There are two important cleanups that I would like to see done in
> asm/uaccess.h across architectures:
> 
>   - generalize the __get_user()/__put_user()/__get_kernel_nofault()/
>    __put_kernel_nofault() wrappers to the point that architectures do not
>    need to worry about the variable type stuff but instead just provide
>    trivial fixed-length helpers of some sort
> 
> - change the calling conventions in a way that allows the use of the
>    asm-goto-with-output method for better object code on modern
>    compilers.
> 
> The x86 version already has most of this, with their
> __get_user_size() macro supporting both the asm-goto label
> and the error code assignment, so the generalized code should
> probably be based on that approach.

I am very interested in the implementation of X86. I need to investigate 
and consider a cross architecture implementation.
However, I understand that the modification of the current patch has 
little to do with the two points mentioned above. We can optimize the 
code step by step.

Thanks,
Tong.

> 
>         Arnd
> 
> .

Re: [PATCH -next v2 1/2] riscv: uaccess: rename __get/put_user_nocheck to __get/put_mem_nocheck

[Arnd Bergmann]

On Sat, Aug 27, 2022 at 12:43 PM Tong Tiangen <tongtiangen@huawei.com> wrote:
> 在 2022/8/26 17:30, Arnd Bergmann 写道:
>
> I am very interested in the implementation of X86. I need to investigate
> and consider a cross architecture implementation.

One more point about the cross-architecture work: it generally makes
sense to do the most commonly used architectures first, usually
that would be x86, arm64 and powerpc64, followed by riscv, arm,
s390 and mips. If we can find something that the first architecture
maintainers like, everyone else can follow and you don't have to
rework all of them multiple times before getting to a consensus.

> However, I understand that the modification of the current patch has
> little to do with the two points mentioned above. We can optimize the
> code step by step.

You are correct that this has little to do with your patch, my point
was mainly that your patch is moving the code further away from
the other architectures, so it would make it harder to then do the
changes we actually want.

       Arnd

Re: [PATCH -next v2 1/2] riscv: uaccess: rename __get/put_user_nocheck to __get/put_mem_nocheck

[Tong Tiangen]

在 2022/8/27 20:49, Arnd Bergmann 写道:
> On Sat, Aug 27, 2022 at 12:43 PM Tong Tiangen <tongtiangen@huawei.com> wrote:
>> 在 2022/8/26 17:30, Arnd Bergmann 写道:
>>
>> I am very interested in the implementation of X86. I need to investigate
>> and consider a cross architecture implementation.
> 
> One more point about the cross-architecture work: it generally makes
> sense to do the most commonly used architectures first, usually
> that would be x86, arm64 and powerpc64, followed by riscv, arm,
> s390 and mips. If we can find something that the first architecture
> maintainers like, everyone else can follow and you don't have to
> rework all of them multiple times before getting to a consensus.
> 
>> However, I understand that the modification of the current patch has
>> little to do with the two points mentioned above. We can optimize the
>> code step by step.
> 
> You are correct that this has little to do with your patch, my point
> was mainly that your patch is moving the code further away from
> the other architectures, so it would make it harder to then do the
> changes we actually want.
> 
>         Arnd

I understand what you mean，it's reasonable.

Thanks,
Tong.
> 
> .

Re: [PATCH -next v2 2/2] riscv: extable: add new extable type EX_TYPE_KACCESS_ERR_ZERO support

[Palmer Dabbelt]

On Sat, 27 Aug 2022 03:39:38 PDT (-0700), tongtiangen@huawei.com wrote:
>
>
> 在 2022/8/26 16:16, Andrew Jones 写道:
>> On Fri, Aug 26, 2022 at 02:44:48PM +0800, Tong Tiangen wrote:
>>>
>>>
>>> 在 2022/8/25 19:06, Andrew Jones 写道:
>>>> On Mon, Aug 15, 2022 at 03:20:25AM +0000, Tong Tiangen wrote:
>>>>> Currently, The extable type EX_TYPE_UACCESS_ERR_ZERO is used by
>>>>> __get/put_kernel_nofault(), but those helpers are not uaccess type, so we
>>>>> add a new extable type EX_TYPE_KACCESS_ERR_ZERO which can be used by
>>>>> __get/put_kernel_no_fault().
>>>>>
>>>>> Only refactor code without any functional changes.
>>>>
>>>> This isn't quite true. __get/put_kernel_nofault now sets a different
>>>> extable type (as the commit message says). But, nothing special seems
>>>> to be done with that, so there's effectively no functional change. Can
>>>> you please elaborate on the motivation for this change? Where will the
>>>> KACCESS type need to be distinguished from the UACCESS type?
>>>
>>> The introduction of EX_TYPE_KACCESS_ERR_ZERO does not change any function,
>>> but makes a correct distinction in the actual type, indicating that there
>>> are indeed some kaccess entries in extable. I think this optimization is
>>> more clear and reasonable.
>>
>> Well, creating new types, just for new type sake, just bloats code.
>>
>>>
>>> A few weeks ago, I did something similar on arm64[1]. I think this
>>> optimization can also be used on riscv.
>>>
>>> We can do some features that are used on uaccss but not applicable on
>>> kaccess in the future[2].
>>>
>>> [1]
>>> https://lore.kernel.org/lkml/20220621072638.1273594-2-tongtiangen@huawei.com/
>>> [2]https://lore.kernel.org/lkml/20220812070557.1028499-4-tongtiangen@huawei.com/
>>>
>>
>> This is part of the information, but I had already found this. What's
>> still missing to me are the riscv patches, or at least a riscv plan, for
>> actually implementing something which requires kaccess and uaccess to have
>> distinct types.
>>
>> Thanks,
>> drew
>
> At present, there is no such plan on riscv, because it is rely on
> hardware support.
> I think this patch can be merged as a small code optimization and
> without any function change.

Generally we need some use of the code in the upstream kernel to justify 
its existence.  In this case I don't really see that: it's just another 
type that's exactly the same as the existing one, having some out of 
tree code that depends on making these types do something different 
isn't a sufficient justification.

Re: [PATCH -next v2 2/2] riscv: extable: add new extable type EX_TYPE_KACCESS_ERR_ZERO support

[Tong Tiangen]

在 2022/9/22 4:25, Palmer Dabbelt 写道:
> On Sat, 27 Aug 2022 03:39:38 PDT (-0700), tongtiangen@huawei.com wrote:
>>
>>
>> 在 2022/8/26 16:16, Andrew Jones 写道:
>>> On Fri, Aug 26, 2022 at 02:44:48PM +0800, Tong Tiangen wrote:
>>>>
>>>>
>>>> 在 2022/8/25 19:06, Andrew Jones 写道:
>>>>> On Mon, Aug 15, 2022 at 03:20:25AM +0000, Tong Tiangen wrote:
>>>>>> Currently, The extable type EX_TYPE_UACCESS_ERR_ZERO is used by
>>>>>> __get/put_kernel_nofault(), but those helpers are not uaccess 
>>>>>> type, so we
>>>>>> add a new extable type EX_TYPE_KACCESS_ERR_ZERO which can be used by
>>>>>> __get/put_kernel_no_fault().
>>>>>>
>>>>>> Only refactor code without any functional changes.
>>>>>
>>>>> This isn't quite true. __get/put_kernel_nofault now sets a different
>>>>> extable type (as the commit message says). But, nothing special seems
>>>>> to be done with that, so there's effectively no functional change. Can
>>>>> you please elaborate on the motivation for this change? Where will the
>>>>> KACCESS type need to be distinguished from the UACCESS type?
>>>>
>>>> The introduction of EX_TYPE_KACCESS_ERR_ZERO does not change any 
>>>> function,
>>>> but makes a correct distinction in the actual type, indicating that 
>>>> there
>>>> are indeed some kaccess entries in extable. I think this 
>>>> optimization is
>>>> more clear and reasonable.
>>>
>>> Well, creating new types, just for new type sake, just bloats code.
>>>
>>>>
>>>> A few weeks ago, I did something similar on arm64[1]. I think this
>>>> optimization can also be used on riscv.
>>>>
>>>> We can do some features that are used on uaccss but not applicable on
>>>> kaccess in the future[2].
>>>>
>>>> [1]
>>>> https://lore.kernel.org/lkml/20220621072638.1273594-2-tongtiangen@huawei.com/ 
>>>>
>>>> [2]https://lore.kernel.org/lkml/20220812070557.1028499-4-tongtiangen@huawei.com/ 
>>>>
>>>>
>>>
>>> This is part of the information, but I had already found this. What's
>>> still missing to me are the riscv patches, or at least a riscv plan, for
>>> actually implementing something which requires kaccess and uaccess to 
>>> have
>>> distinct types.
>>>
>>> Thanks,
>>> drew
>>
>> At present, there is no such plan on riscv, because it is rely on
>> hardware support.
>> I think this patch can be merged as a small code optimization and
>> without any function change.
> 
> Generally we need some use of the code in the upstream kernel to justify 
> its existence.  In this case I don't really see that: it's just another 
> type that's exactly the same as the existing one, having some out of 
> tree code that depends on making these types do something different 
> isn't a sufficient justification.
> .
Hi palmer:

I agree with this point very much，many thanks.

Tong.