| Message ID | 20220815032025.2685516-2-tongtiangen@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | riscv: some refactorings realted to uaccess and extable | expand |
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
在 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 > .
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
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
在 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 > > .
在 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 > > .
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
在 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. > > .
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)
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(-)