* [PATCH -next v2 0/2]riscv: some refactorings realted to uaccess and extable @ 2022-08-15 3:20 Tong Tiangen 2022-08-15 3:20 ` [PATCH -next v2 1/2] riscv: uaccess: rename __get/put_user_nocheck to __get/put_mem_nocheck Tong Tiangen ` (2 more replies) 0 siblings, 3 replies; 20+ messages in thread From: Tong Tiangen @ 2022-08-15 3:20 UTC (permalink / raw) To: Paul Walmsley, Palmer Dabbelt, Palmer Dabbelt, Albert Ou, Conor.Dooley Cc: linux-riscv, linux-kernel, Tong Tiangen, wangkefeng.wang, Guohanjun 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH -next v2 1/2] riscv: uaccess: rename __get/put_user_nocheck to __get/put_mem_nocheck 2022-08-15 3:20 [PATCH -next v2 0/2]riscv: some refactorings realted to uaccess and extable Tong Tiangen @ 2022-08-15 3:20 ` Tong Tiangen 2022-08-25 10:56 ` Andrew Jones 2022-08-26 9:30 ` Arnd Bergmann 2022-08-15 3:20 ` [PATCH -next v2 2/2] riscv: extable: add new extable type EX_TYPE_KACCESS_ERR_ZERO support Tong Tiangen 2022-08-24 6:31 ` [PATCH -next v2 0/2]riscv: some refactorings realted to uaccess and extable Tong Tiangen 2 siblings, 2 replies; 20+ messages in thread From: Tong Tiangen @ 2022-08-15 3:20 UTC (permalink / raw) To: Paul Walmsley, Palmer Dabbelt, Palmer Dabbelt, Albert Ou, Conor.Dooley Cc: linux-riscv, linux-kernel, Tong Tiangen, wangkefeng.wang, Guohanjun 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH -next v2 1/2] riscv: uaccess: rename __get/put_user_nocheck to __get/put_mem_nocheck 2022-08-15 3:20 ` [PATCH -next v2 1/2] riscv: uaccess: rename __get/put_user_nocheck to __get/put_mem_nocheck Tong Tiangen @ 2022-08-25 10:56 ` Andrew Jones 2022-08-26 6:33 ` Tong Tiangen 2022-08-26 9:30 ` Arnd Bergmann 1 sibling, 1 reply; 20+ messages in thread From: Andrew Jones @ 2022-08-25 10:56 UTC (permalink / raw) To: Tong Tiangen Cc: Paul Walmsley, Palmer Dabbelt, Palmer Dabbelt, Albert Ou, Conor.Dooley, linux-riscv, linux-kernel, wangkefeng.wang, Guohanjun 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH -next v2 1/2] riscv: uaccess: rename __get/put_user_nocheck to __get/put_mem_nocheck 2022-08-25 10:56 ` Andrew Jones @ 2022-08-26 6:33 ` Tong Tiangen 2022-08-26 7:43 ` Andrew Jones 0 siblings, 1 reply; 20+ messages in thread From: Tong Tiangen @ 2022-08-26 6:33 UTC (permalink / raw) To: Andrew Jones Cc: Paul Walmsley, Palmer Dabbelt, Palmer Dabbelt, Albert Ou, Conor.Dooley, linux-riscv, linux-kernel, wangkefeng.wang, Guohanjun 在 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 > . ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH -next v2 1/2] riscv: uaccess: rename __get/put_user_nocheck to __get/put_mem_nocheck 2022-08-26 6:33 ` Tong Tiangen @ 2022-08-26 7:43 ` Andrew Jones 2022-08-27 10:39 ` Tong Tiangen 0 siblings, 1 reply; 20+ messages in thread From: Andrew Jones @ 2022-08-26 7:43 UTC (permalink / raw) To: Tong Tiangen Cc: Paul Walmsley, Palmer Dabbelt, Palmer Dabbelt, Albert Ou, Conor.Dooley, linux-riscv, linux-kernel, wangkefeng.wang, Guohanjun 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH -next v2 1/2] riscv: uaccess: rename __get/put_user_nocheck to __get/put_mem_nocheck 2022-08-26 7:43 ` Andrew Jones @ 2022-08-27 10:39 ` Tong Tiangen 0 siblings, 0 replies; 20+ messages in thread From: Tong Tiangen @ 2022-08-27 10:39 UTC (permalink / raw) To: Andrew Jones Cc: Paul Walmsley, Palmer Dabbelt, Palmer Dabbelt, Albert Ou, Conor.Dooley, linux-riscv, linux-kernel, wangkefeng.wang, Guohanjun 在 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 > > . ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH -next v2 1/2] riscv: uaccess: rename __get/put_user_nocheck to __get/put_mem_nocheck 2022-08-15 3:20 ` [PATCH -next v2 1/2] riscv: uaccess: rename __get/put_user_nocheck to __get/put_mem_nocheck Tong Tiangen 2022-08-25 10:56 ` Andrew Jones @ 2022-08-26 9:30 ` Arnd Bergmann 2022-08-27 10:43 ` Tong Tiangen 1 sibling, 1 reply; 20+ messages in thread From: Arnd Bergmann @ 2022-08-26 9:30 UTC (permalink / raw) To: Tong Tiangen Cc: Paul Walmsley, Palmer Dabbelt, Palmer Dabbelt, Albert Ou, Conor.Dooley, linux-riscv, linux-kernel, wangkefeng.wang, Guohanjun 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH -next v2 1/2] riscv: uaccess: rename __get/put_user_nocheck to __get/put_mem_nocheck 2022-08-26 9:30 ` Arnd Bergmann @ 2022-08-27 10:43 ` Tong Tiangen 2022-08-27 12:49 ` Arnd Bergmann 0 siblings, 1 reply; 20+ messages in thread From: Tong Tiangen @ 2022-08-27 10:43 UTC (permalink / raw) To: Arnd Bergmann Cc: Paul Walmsley, Palmer Dabbelt, Palmer Dabbelt, Albert Ou, Conor.Dooley, linux-riscv, linux-kernel, wangkefeng.wang, Guohanjun 在 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 > > . ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH -next v2 1/2] riscv: uaccess: rename __get/put_user_nocheck to __get/put_mem_nocheck 2022-08-27 10:43 ` Tong Tiangen @ 2022-08-27 12:49 ` Arnd Bergmann 2022-08-29 1:26 ` Tong Tiangen 0 siblings, 1 reply; 20+ messages in thread From: Arnd Bergmann @ 2022-08-27 12:49 UTC (permalink / raw) To: Tong Tiangen Cc: Arnd Bergmann, Paul Walmsley, Palmer Dabbelt, Palmer Dabbelt, Albert Ou, Conor.Dooley, linux-riscv, linux-kernel, wangkefeng.wang, Guohanjun 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH -next v2 1/2] riscv: uaccess: rename __get/put_user_nocheck to __get/put_mem_nocheck 2022-08-27 12:49 ` Arnd Bergmann @ 2022-08-29 1:26 ` Tong Tiangen 0 siblings, 0 replies; 20+ messages in thread From: Tong Tiangen @ 2022-08-29 1:26 UTC (permalink / raw) To: Arnd Bergmann Cc: Paul Walmsley, Palmer Dabbelt, Palmer Dabbelt, Albert Ou, Conor.Dooley, linux-riscv, linux-kernel, wangkefeng.wang, Guohanjun 在 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. > > . ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH -next v2 2/2] riscv: extable: add new extable type EX_TYPE_KACCESS_ERR_ZERO support 2022-08-15 3:20 [PATCH -next v2 0/2]riscv: some refactorings realted to uaccess and extable Tong Tiangen 2022-08-15 3:20 ` [PATCH -next v2 1/2] riscv: uaccess: rename __get/put_user_nocheck to __get/put_mem_nocheck Tong Tiangen @ 2022-08-15 3:20 ` Tong Tiangen 2022-08-25 11:06 ` Andrew Jones 2022-08-24 6:31 ` [PATCH -next v2 0/2]riscv: some refactorings realted to uaccess and extable Tong Tiangen 2 siblings, 1 reply; 20+ messages in thread From: Tong Tiangen @ 2022-08-15 3:20 UTC (permalink / raw) To: Paul Walmsley, Palmer Dabbelt, Palmer Dabbelt, Albert Ou, Conor.Dooley Cc: linux-riscv, linux-kernel, Tong Tiangen, wangkefeng.wang, Guohanjun 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH -next v2 2/2] riscv: extable: add new extable type EX_TYPE_KACCESS_ERR_ZERO support 2022-08-15 3:20 ` [PATCH -next v2 2/2] riscv: extable: add new extable type EX_TYPE_KACCESS_ERR_ZERO support Tong Tiangen @ 2022-08-25 11:06 ` Andrew Jones 2022-08-26 6:44 ` Tong Tiangen 0 siblings, 1 reply; 20+ messages in thread From: Andrew Jones @ 2022-08-25 11:06 UTC (permalink / raw) To: Tong Tiangen Cc: Paul Walmsley, Palmer Dabbelt, Palmer Dabbelt, Albert Ou, Conor.Dooley, linux-riscv, linux-kernel, wangkefeng.wang, Guohanjun 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH -next v2 2/2] riscv: extable: add new extable type EX_TYPE_KACCESS_ERR_ZERO support 2022-08-25 11:06 ` Andrew Jones @ 2022-08-26 6:44 ` Tong Tiangen 2022-08-26 8:16 ` Andrew Jones 0 siblings, 1 reply; 20+ messages in thread From: Tong Tiangen @ 2022-08-26 6:44 UTC (permalink / raw) To: Andrew Jones Cc: Paul Walmsley, Palmer Dabbelt, Palmer Dabbelt, Albert Ou, Conor.Dooley, linux-riscv, linux-kernel, wangkefeng.wang, Guohanjun 在 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 > . ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH -next v2 2/2] riscv: extable: add new extable type EX_TYPE_KACCESS_ERR_ZERO support 2022-08-26 6:44 ` Tong Tiangen @ 2022-08-26 8:16 ` Andrew Jones 2022-08-27 10:39 ` Tong Tiangen 0 siblings, 1 reply; 20+ messages in thread From: Andrew Jones @ 2022-08-26 8:16 UTC (permalink / raw) To: Tong Tiangen Cc: Paul Walmsley, Palmer Dabbelt, Palmer Dabbelt, Albert Ou, Conor.Dooley, linux-riscv, linux-kernel, wangkefeng.wang, Guohanjun 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH -next v2 2/2] riscv: extable: add new extable type EX_TYPE_KACCESS_ERR_ZERO support 2022-08-26 8:16 ` Andrew Jones @ 2022-08-27 10:39 ` Tong Tiangen 2022-09-21 20:25 ` Palmer Dabbelt 0 siblings, 1 reply; 20+ messages in thread From: Tong Tiangen @ 2022-08-27 10:39 UTC (permalink / raw) To: Andrew Jones Cc: Paul Walmsley, Palmer Dabbelt, Palmer Dabbelt, Albert Ou, Conor.Dooley, linux-riscv, linux-kernel, wangkefeng.wang, Guohanjun 在 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. > > . ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH -next v2 2/2] riscv: extable: add new extable type EX_TYPE_KACCESS_ERR_ZERO support 2022-08-27 10:39 ` Tong Tiangen @ 2022-09-21 20:25 ` Palmer Dabbelt 2022-10-21 12:23 ` Tong Tiangen 0 siblings, 1 reply; 20+ messages in thread From: Palmer Dabbelt @ 2022-09-21 20:25 UTC (permalink / raw) To: tongtiangen Cc: ajones, Paul Walmsley, aou, Conor.Dooley, linux-riscv, linux-kernel, wangkefeng.wang, guohanjun 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. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH -next v2 2/2] riscv: extable: add new extable type EX_TYPE_KACCESS_ERR_ZERO support 2022-09-21 20:25 ` Palmer Dabbelt @ 2022-10-21 12:23 ` Tong Tiangen 0 siblings, 0 replies; 20+ messages in thread From: Tong Tiangen @ 2022-10-21 12:23 UTC (permalink / raw) To: Palmer Dabbelt Cc: ajones, Paul Walmsley, aou, Conor.Dooley, linux-riscv, linux-kernel, wangkefeng.wang, guohanjun 在 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. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH -next v2 0/2]riscv: some refactorings realted to uaccess and extable 2022-08-15 3:20 [PATCH -next v2 0/2]riscv: some refactorings realted to uaccess and extable Tong Tiangen 2022-08-15 3:20 ` [PATCH -next v2 1/2] riscv: uaccess: rename __get/put_user_nocheck to __get/put_mem_nocheck Tong Tiangen 2022-08-15 3:20 ` [PATCH -next v2 2/2] riscv: extable: add new extable type EX_TYPE_KACCESS_ERR_ZERO support Tong Tiangen @ 2022-08-24 6:31 ` Tong Tiangen 2022-08-24 16:49 ` Conor.Dooley 2 siblings, 1 reply; 20+ messages in thread From: Tong Tiangen @ 2022-08-24 6:31 UTC (permalink / raw) To: Paul Walmsley, Palmer Dabbelt, Palmer Dabbelt, Albert Ou, Conor.Dooley Cc: linux-riscv, linux-kernel, wangkefeng.wang, Guohanjun 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(-) > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH -next v2 0/2]riscv: some refactorings realted to uaccess and extable 2022-08-24 6:31 ` [PATCH -next v2 0/2]riscv: some refactorings realted to uaccess and extable Tong Tiangen @ 2022-08-24 16:49 ` Conor.Dooley 2022-08-25 3:04 ` Tong Tiangen 0 siblings, 1 reply; 20+ messages in thread From: Conor.Dooley @ 2022-08-24 16:49 UTC (permalink / raw) To: tongtiangen, paul.walmsley, palmer, palmer, aou, Conor.Dooley Cc: linux-riscv, linux-kernel, wangkefeng.wang, guohanjun 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(-) >> ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH -next v2 0/2]riscv: some refactorings realted to uaccess and extable 2022-08-24 16:49 ` Conor.Dooley @ 2022-08-25 3:04 ` Tong Tiangen 0 siblings, 0 replies; 20+ messages in thread From: Tong Tiangen @ 2022-08-25 3:04 UTC (permalink / raw) To: Conor.Dooley, paul.walmsley, palmer, palmer, aou Cc: linux-riscv, linux-kernel, wangkefeng.wang, guohanjun 在 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(-) >>> ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2022-10-21 12:23 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-08-15 3:20 [PATCH -next v2 0/2]riscv: some refactorings realted to uaccess and extable Tong Tiangen 2022-08-15 3:20 ` [PATCH -next v2 1/2] riscv: uaccess: rename __get/put_user_nocheck to __get/put_mem_nocheck Tong Tiangen 2022-08-25 10:56 ` Andrew Jones 2022-08-26 6:33 ` Tong Tiangen 2022-08-26 7:43 ` Andrew Jones 2022-08-27 10:39 ` Tong Tiangen 2022-08-26 9:30 ` Arnd Bergmann 2022-08-27 10:43 ` Tong Tiangen 2022-08-27 12:49 ` Arnd Bergmann 2022-08-29 1:26 ` Tong Tiangen 2022-08-15 3:20 ` [PATCH -next v2 2/2] riscv: extable: add new extable type EX_TYPE_KACCESS_ERR_ZERO support Tong Tiangen 2022-08-25 11:06 ` Andrew Jones 2022-08-26 6:44 ` Tong Tiangen 2022-08-26 8:16 ` Andrew Jones 2022-08-27 10:39 ` Tong Tiangen 2022-09-21 20:25 ` Palmer Dabbelt 2022-10-21 12:23 ` Tong Tiangen 2022-08-24 6:31 ` [PATCH -next v2 0/2]riscv: some refactorings realted to uaccess and extable Tong Tiangen 2022-08-24 16:49 ` Conor.Dooley 2022-08-25 3:04 ` Tong Tiangen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).