| Message ID | 20220815032025.2685516-3-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: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
在 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 > .
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
在 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. > > .
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.
在 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.
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); }
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(-)