| Message ID | 20220528065056.1034168-3-tongtiangen@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: add machine check safe support | expand |
On Sat, May 28, 2022 at 06:50:50AM +0000, Tong Tiangen wrote: > Currnetly, the extable type used by __arch_copy_from/to_user() is > EX_TYPE_FIXUP. In fact, It is more clearly to use meaningful > EX_TYPE_UACCESS_*. > > Suggested-by: Mark Rutland <mark.rutland@arm.com> > Signed-off-by: Tong Tiangen <tongtiangen@huawei.com> > --- > arch/arm64/include/asm/asm-extable.h | 8 ++++++++ > arch/arm64/include/asm/asm-uaccess.h | 12 ++++++------ > 2 files changed, 14 insertions(+), 6 deletions(-) > > diff --git a/arch/arm64/include/asm/asm-extable.h b/arch/arm64/include/asm/asm-extable.h > index 56ebe183e78b..9c94ac1f082c 100644 > --- a/arch/arm64/include/asm/asm-extable.h > +++ b/arch/arm64/include/asm/asm-extable.h > @@ -28,6 +28,14 @@ > __ASM_EXTABLE_RAW(\insn, \fixup, EX_TYPE_FIXUP, 0) > .endm > > +/* > + * Create an exception table entry for uaccess `insn`, which will branch to `fixup` > + * when an unhandled fault is taken. > + * ex->data = ~0 means both reg_err and reg_zero is set to wzr(x31). > + */ > + .macro _asm_extable_uaccess, insn, fixup > + __ASM_EXTABLE_RAW(\insn, \fixup, EX_TYPE_UACCESS_ERR_ZERO, ~0) > + .endm I'm not too keen on using `~0` here, since that also sets other bits in the data field, and its somewhat opaque. How painful is it to generate the data fields as with the C version of this macro, so that we can pass in wzr explciitly for the two sub-fields? Other than that, this looks good to me. Thanks, Mark. > /* > * Create an exception table entry for `insn` if `fixup` is provided. Otherwise > * do nothing. > diff --git a/arch/arm64/include/asm/asm-uaccess.h b/arch/arm64/include/asm/asm-uaccess.h > index 0557af834e03..75b211c98dea 100644 > --- a/arch/arm64/include/asm/asm-uaccess.h > +++ b/arch/arm64/include/asm/asm-uaccess.h > @@ -61,7 +61,7 @@ alternative_else_nop_endif > > #define USER(l, x...) \ > 9999: x; \ > - _asm_extable 9999b, l > + _asm_extable_uaccess 9999b, l > > /* > * Generate the assembly for LDTR/STTR with exception table entries. > @@ -73,8 +73,8 @@ alternative_else_nop_endif > 8889: ldtr \reg2, [\addr, #8]; > add \addr, \addr, \post_inc; > > - _asm_extable 8888b,\l; > - _asm_extable 8889b,\l; > + _asm_extable_uaccess 8888b, \l; > + _asm_extable_uaccess 8889b, \l; > .endm > > .macro user_stp l, reg1, reg2, addr, post_inc > @@ -82,14 +82,14 @@ alternative_else_nop_endif > 8889: sttr \reg2, [\addr, #8]; > add \addr, \addr, \post_inc; > > - _asm_extable 8888b,\l; > - _asm_extable 8889b,\l; > + _asm_extable_uaccess 8888b,\l; > + _asm_extable_uaccess 8889b,\l; > .endm > > .macro user_ldst l, inst, reg, addr, post_inc > 8888: \inst \reg, [\addr]; > add \addr, \addr, \post_inc; > > - _asm_extable 8888b,\l; > + _asm_extable_uaccess 8888b, \l; > .endm > #endif > -- > 2.25.1 >
在 2022/6/17 16:24, Mark Rutland 写道: > On Sat, May 28, 2022 at 06:50:50AM +0000, Tong Tiangen wrote: >> Currnetly, the extable type used by __arch_copy_from/to_user() is >> EX_TYPE_FIXUP. In fact, It is more clearly to use meaningful >> EX_TYPE_UACCESS_*. >> >> Suggested-by: Mark Rutland <mark.rutland@arm.com> >> Signed-off-by: Tong Tiangen <tongtiangen@huawei.com> >> --- >> arch/arm64/include/asm/asm-extable.h | 8 ++++++++ >> arch/arm64/include/asm/asm-uaccess.h | 12 ++++++------ >> 2 files changed, 14 insertions(+), 6 deletions(-) >> >> diff --git a/arch/arm64/include/asm/asm-extable.h b/arch/arm64/include/asm/asm-extable.h >> index 56ebe183e78b..9c94ac1f082c 100644 >> --- a/arch/arm64/include/asm/asm-extable.h >> +++ b/arch/arm64/include/asm/asm-extable.h >> @@ -28,6 +28,14 @@ >> __ASM_EXTABLE_RAW(\insn, \fixup, EX_TYPE_FIXUP, 0) >> .endm >> >> +/* >> + * Create an exception table entry for uaccess `insn`, which will branch to `fixup` >> + * when an unhandled fault is taken. >> + * ex->data = ~0 means both reg_err and reg_zero is set to wzr(x31). >> + */ >> + .macro _asm_extable_uaccess, insn, fixup >> + __ASM_EXTABLE_RAW(\insn, \fixup, EX_TYPE_UACCESS_ERR_ZERO, ~0) >> + .endm > > I'm not too keen on using `~0` here, since that also sets other bits in the > data field, and its somewhat opaque. > > How painful is it to generate the data fields as with the C version of this > macro, so that we can pass in wzr explciitly for the two sub-fields? > > Other than that, this looks good to me. > > Thanks, > Mark. ok, will fix next version. Thanks, Tong. > >> /* >> * Create an exception table entry for `insn` if `fixup` is provided. Otherwise >> * do nothing. >> diff --git a/arch/arm64/include/asm/asm-uaccess.h b/arch/arm64/include/asm/asm-uaccess.h >> index 0557af834e03..75b211c98dea 100644 >> --- a/arch/arm64/include/asm/asm-uaccess.h >> +++ b/arch/arm64/include/asm/asm-uaccess.h >> @@ -61,7 +61,7 @@ alternative_else_nop_endif >> >> #define USER(l, x...) \ >> 9999: x; \ >> - _asm_extable 9999b, l >> + _asm_extable_uaccess 9999b, l >> >> /* >> * Generate the assembly for LDTR/STTR with exception table entries. >> @@ -73,8 +73,8 @@ alternative_else_nop_endif >> 8889: ldtr \reg2, [\addr, #8]; >> add \addr, \addr, \post_inc; >> >> - _asm_extable 8888b,\l; >> - _asm_extable 8889b,\l; >> + _asm_extable_uaccess 8888b, \l; >> + _asm_extable_uaccess 8889b, \l; >> .endm >> >> .macro user_stp l, reg1, reg2, addr, post_inc >> @@ -82,14 +82,14 @@ alternative_else_nop_endif >> 8889: sttr \reg2, [\addr, #8]; >> add \addr, \addr, \post_inc; >> >> - _asm_extable 8888b,\l; >> - _asm_extable 8889b,\l; >> + _asm_extable_uaccess 8888b,\l; >> + _asm_extable_uaccess 8889b,\l; >> .endm >> >> .macro user_ldst l, inst, reg, addr, post_inc >> 8888: \inst \reg, [\addr]; >> add \addr, \addr, \post_inc; >> >> - _asm_extable 8888b,\l; >> + _asm_extable_uaccess 8888b, \l; >> .endm >> #endif >> -- >> 2.25.1 >> > .
在 2022/6/18 11:26, Tong Tiangen 写道: > > > 在 2022/6/17 16:24, Mark Rutland 写道: >> On Sat, May 28, 2022 at 06:50:50AM +0000, Tong Tiangen wrote: >>> Currnetly, the extable type used by __arch_copy_from/to_user() is >>> EX_TYPE_FIXUP. In fact, It is more clearly to use meaningful >>> EX_TYPE_UACCESS_*. >>> >>> Suggested-by: Mark Rutland <mark.rutland@arm.com> >>> Signed-off-by: Tong Tiangen <tongtiangen@huawei.com> >>> --- >>> arch/arm64/include/asm/asm-extable.h | 8 ++++++++ >>> arch/arm64/include/asm/asm-uaccess.h | 12 ++++++------ >>> 2 files changed, 14 insertions(+), 6 deletions(-) >>> >>> diff --git a/arch/arm64/include/asm/asm-extable.h >>> b/arch/arm64/include/asm/asm-extable.h >>> index 56ebe183e78b..9c94ac1f082c 100644 >>> --- a/arch/arm64/include/asm/asm-extable.h >>> +++ b/arch/arm64/include/asm/asm-extable.h >>> @@ -28,6 +28,14 @@ >>> __ASM_EXTABLE_RAW(\insn, \fixup, EX_TYPE_FIXUP, 0) >>> .endm >>> +/* >>> + * Create an exception table entry for uaccess `insn`, which will >>> branch to `fixup` >>> + * when an unhandled fault is taken. >>> + * ex->data = ~0 means both reg_err and reg_zero is set to wzr(x31). >>> + */ >>> + .macro _asm_extable_uaccess, insn, fixup >>> + __ASM_EXTABLE_RAW(\insn, \fixup, EX_TYPE_UACCESS_ERR_ZERO, ~0) >>> + .endm >> >> I'm not too keen on using `~0` here, since that also sets other bits >> in the >> data field, and its somewhat opaque. >> >> How painful is it to generate the data fields as with the C version of >> this >> macro, so that we can pass in wzr explciitly for the two sub-fields? >> >> Other than that, this looks good to me. >> >> Thanks, >> Mark. > > ok, will fix next version. > > Thanks, > Tong. I tried to using data filelds as with C version, but here assembly code we can not using operator such as << and |, if we use lsl and orr instructions, the gpr will be occupied. So how about using 0x3ff directly here? it means err register and zero register both set to x31. Thanks, Tong. > >> >>> /* >>> * Create an exception table entry for `insn` if `fixup` is >>> provided. Otherwise >>> * do nothing. >>> diff --git a/arch/arm64/include/asm/asm-uaccess.h >>> b/arch/arm64/include/asm/asm-uaccess.h >>> index 0557af834e03..75b211c98dea 100644 >>> --- a/arch/arm64/include/asm/asm-uaccess.h >>> +++ b/arch/arm64/include/asm/asm-uaccess.h >>> @@ -61,7 +61,7 @@ alternative_else_nop_endif >>> #define USER(l, x...) \ >>> 9999: x; \ >>> - _asm_extable 9999b, l >>> + _asm_extable_uaccess 9999b, l >>> /* >>> * Generate the assembly for LDTR/STTR with exception table entries. >>> @@ -73,8 +73,8 @@ alternative_else_nop_endif >>> 8889: ldtr \reg2, [\addr, #8]; >>> add \addr, \addr, \post_inc; >>> - _asm_extable 8888b,\l; >>> - _asm_extable 8889b,\l; >>> + _asm_extable_uaccess 8888b, \l; >>> + _asm_extable_uaccess 8889b, \l; >>> .endm >>> .macro user_stp l, reg1, reg2, addr, post_inc >>> @@ -82,14 +82,14 @@ alternative_else_nop_endif >>> 8889: sttr \reg2, [\addr, #8]; >>> add \addr, \addr, \post_inc; >>> - _asm_extable 8888b,\l; >>> - _asm_extable 8889b,\l; >>> + _asm_extable_uaccess 8888b,\l; >>> + _asm_extable_uaccess 8889b,\l; >>> .endm >>> .macro user_ldst l, inst, reg, addr, post_inc >>> 8888: \inst \reg, [\addr]; >>> add \addr, \addr, \post_inc; >>> - _asm_extable 8888b,\l; >>> + _asm_extable_uaccess 8888b, \l; >>> .endm >>> #endif >>> -- >>> 2.25.1 >>> >> . > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Sat, Jun 18, 2022 at 04:42:06PM +0800, Tong Tiangen wrote: > > > > diff --git a/arch/arm64/include/asm/asm-extable.h > > > > b/arch/arm64/include/asm/asm-extable.h > > > > index 56ebe183e78b..9c94ac1f082c 100644 > > > > --- a/arch/arm64/include/asm/asm-extable.h > > > > +++ b/arch/arm64/include/asm/asm-extable.h > > > > @@ -28,6 +28,14 @@ > > > > __ASM_EXTABLE_RAW(\insn, \fixup, EX_TYPE_FIXUP, 0) > > > > .endm > > > > +/* > > > > + * Create an exception table entry for uaccess `insn`, which > > > > will branch to `fixup` > > > > + * when an unhandled fault is taken. > > > > + * ex->data = ~0 means both reg_err and reg_zero is set to wzr(x31). > > > > + */ > > > > + .macro _asm_extable_uaccess, insn, fixup > > > > + __ASM_EXTABLE_RAW(\insn, \fixup, EX_TYPE_UACCESS_ERR_ZERO, ~0) > > > > + .endm > > > > > > I'm not too keen on using `~0` here, since that also sets other bits > > > in the > > > data field, and its somewhat opaque. > > > > > > How painful is it to generate the data fields as with the C version > > > of this > > > macro, so that we can pass in wzr explciitly for the two sub-fields? > > > > > > Other than that, this looks good to me. > > > > > > Thanks, > > > Mark. > > > > ok, will fix next version. > > > > Thanks, > > Tong. > > I tried to using data filelds as with C version, but here assembly code we > can not using operator such as << and |, if we use lsl and orr instructions, > the gpr will be occupied. > > So how about using 0x3ff directly here? it means err register and zero > register both set to x31. I had a go at implementing this, and it seems simple enough. Please see: https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/log/?h=arm64/extable/asm-uaccess Mark.
在 2022/6/18 20:40, Mark Rutland 写道: > On Sat, Jun 18, 2022 at 04:42:06PM +0800, Tong Tiangen wrote: >>>>> diff --git a/arch/arm64/include/asm/asm-extable.h >>>>> b/arch/arm64/include/asm/asm-extable.h >>>>> index 56ebe183e78b..9c94ac1f082c 100644 >>>>> --- a/arch/arm64/include/asm/asm-extable.h >>>>> +++ b/arch/arm64/include/asm/asm-extable.h >>>>> @@ -28,6 +28,14 @@ >>>>> __ASM_EXTABLE_RAW(\insn, \fixup, EX_TYPE_FIXUP, 0) >>>>> .endm >>>>> +/* >>>>> + * Create an exception table entry for uaccess `insn`, which >>>>> will branch to `fixup` >>>>> + * when an unhandled fault is taken. >>>>> + * ex->data = ~0 means both reg_err and reg_zero is set to wzr(x31). >>>>> + */ >>>>> + .macro _asm_extable_uaccess, insn, fixup >>>>> + __ASM_EXTABLE_RAW(\insn, \fixup, EX_TYPE_UACCESS_ERR_ZERO, ~0) >>>>> + .endm >>>> >>>> I'm not too keen on using `~0` here, since that also sets other bits >>>> in the >>>> data field, and its somewhat opaque. >>>> >>>> How painful is it to generate the data fields as with the C version >>>> of this >>>> macro, so that we can pass in wzr explciitly for the two sub-fields? >>>> >>>> Other than that, this looks good to me. >>>> >>>> Thanks, >>>> Mark. >>> >>> ok, will fix next version. >>> >>> Thanks, >>> Tong. >> >> I tried to using data filelds as with C version, but here assembly code we >> can not using operator such as << and |, if we use lsl and orr instructions, >> the gpr will be occupied. >> >> So how about using 0x3ff directly here? it means err register and zero >> register both set to x31. > > I had a go at implementing this, and it seems simple enough. Please see: > > https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/log/?h=arm64/extable/asm-uaccess > I made the following modifications, and the other parts are based on your implementation: arch/arm64/include/asm/asm-extable.h [...] .macro _asm_extable_uaccess, insn, fixup _ASM_EXTABLE_UACCESS(\insn, \fixup) .endm [...] The following errors are reported during compilation: [...] arch/arm64/lib/clear_user.S:45: Error: invalid operands (*ABS* and *UND* sections) for `<<' [...] "<<" is invalid operands in assembly, is there something wrong with me? Thanks, Tong. > Mark. > > .
On Mon, Jun 20, 2022 at 10:59:12AM +0800, Tong Tiangen wrote: > 在 2022/6/18 20:40, Mark Rutland 写道: > > On Sat, Jun 18, 2022 at 04:42:06PM +0800, Tong Tiangen wrote: > > > > > > diff --git a/arch/arm64/include/asm/asm-extable.h > > > > > > b/arch/arm64/include/asm/asm-extable.h > > > > > > index 56ebe183e78b..9c94ac1f082c 100644 > > > > > > --- a/arch/arm64/include/asm/asm-extable.h > > > > > > +++ b/arch/arm64/include/asm/asm-extable.h > > > > > > @@ -28,6 +28,14 @@ > > > > > > __ASM_EXTABLE_RAW(\insn, \fixup, EX_TYPE_FIXUP, 0) > > > > > > .endm > > > > > > +/* > > > > > > + * Create an exception table entry for uaccess `insn`, which > > > > > > will branch to `fixup` > > > > > > + * when an unhandled fault is taken. > > > > > > + * ex->data = ~0 means both reg_err and reg_zero is set to wzr(x31). > > > > > > + */ > > > > > > + .macro _asm_extable_uaccess, insn, fixup > > > > > > + __ASM_EXTABLE_RAW(\insn, \fixup, EX_TYPE_UACCESS_ERR_ZERO, ~0) > > > > > > + .endm > > > > > > > > > > I'm not too keen on using `~0` here, since that also sets other bits > > > > > in the > > > > > data field, and its somewhat opaque. > > > > > > > > > > How painful is it to generate the data fields as with the C version > > > > > of this > > > > > macro, so that we can pass in wzr explciitly for the two sub-fields? > > > > > > > > > > Other than that, this looks good to me. > > > > > > > > > > Thanks, > > > > > Mark. > > > > > > > > ok, will fix next version. > > > > > > > > Thanks, > > > > Tong. > > > > > > I tried to using data filelds as with C version, but here assembly code we > > > can not using operator such as << and |, if we use lsl and orr instructions, > > > the gpr will be occupied. > > > > > > So how about using 0x3ff directly here? it means err register and zero > > > register both set to x31. > > > > I had a go at implementing this, and it seems simple enough. Please see: > > > > https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/log/?h=arm64/extable/asm-uaccess > > > > I made the following modifications, and the other parts are based on your > implementation: > > arch/arm64/include/asm/asm-extable.h > [...] > .macro _asm_extable_uaccess, insn, fixup > _ASM_EXTABLE_UACCESS(\insn, \fixup) > .endm > [...] I also made this same change locally when testing, and building with GCC 11.1.0 or LLVM 14.0.0 I am not seeing any problem when building, and the result is as expected: | [mark@lakrids:~/src/linux]% usekorg 11.1.0 make ARCH=arm64 CROSS_COMPILE=aarch64-linux- defconfig | *** Default configuration is based on 'defconfig' | # | # No change to .config | # | [mark@lakrids:~/src/linux]% usekorg 11.1.0 make ARCH=arm64 CROSS_COMPILE=aarch64-linux- -j50 arch/arm64/lib/ | CALL scripts/atomic/check-atomics.sh | CC arch/arm64/kernel/asm-offsets.s | CALL scripts/checksyscalls.sh | AS arch/arm64/kernel/vdso/note.o | AS arch/arm64/kernel/vdso/sigreturn.o | LD arch/arm64/kernel/vdso/vdso.so.dbg | VDSOSYM include/generated/vdso-offsets.h | OBJCOPY arch/arm64/kernel/vdso/vdso.so | make[2]: Nothing to be done for 'arch/arm64/lib/'. | AS arch/arm64/lib/clear_page.o | AS arch/arm64/lib/clear_user.o | AS arch/arm64/lib/copy_from_user.o | AS arch/arm64/lib/copy_page.o | AS arch/arm64/lib/copy_to_user.o | CC arch/arm64/lib/csum.o | CC arch/arm64/lib/delay.o | AS arch/arm64/lib/memchr.o | AS arch/arm64/lib/memcmp.o | AS arch/arm64/lib/memcpy.o | AS arch/arm64/lib/memset.o | AS arch/arm64/lib/strchr.o | AS arch/arm64/lib/strcmp.o | AS arch/arm64/lib/strlen.o | AS arch/arm64/lib/strncmp.o | AS arch/arm64/lib/strnlen.o | AS arch/arm64/lib/strrchr.o | AS arch/arm64/lib/tishift.o | AS arch/arm64/lib/crc32.o | AS arch/arm64/lib/mte.o | CC [M] arch/arm64/lib/xor-neon.o | AR arch/arm64/lib/built-in.a | AR arch/arm64/lib/lib.a | [mark@lakrids:~/src/linux]% usekorg 12.1.0 aarch64-linux-objdump -j __ex_table -D arch/arm64/lib/clear_user.o | | arch/arm64/lib/clear_user.o: file format elf64-littleaarch64 | | | Disassembly of section __ex_table: | | 0000000000000000 <__ex_table>: | ... | 8: 03ff0003 .inst 0x03ff0003 ; undefined | ... | 14: 03ff0003 .inst 0x03ff0003 ; undefined | ... | 20: 03ff0003 .inst 0x03ff0003 ; undefined | ... | 2c: 03ff0003 .inst 0x03ff0003 ; undefined | ... | 38: 03ff0003 .inst 0x03ff0003 ; undefined | ... | 44: 03ff0003 .inst 0x03ff0003 ; undefined > The following errors are reported during compilation: > [...] > arch/arm64/lib/clear_user.S:45: Error: invalid operands (*ABS* and *UND* > sections) for `<<' > [...] As above, I'm not seeing this. This suggests that the EX_DATA_REG() macro is going wrong somehow. Assuming the operand types correspond to the LHS and RHS of the expression, this would mean the GPR number is defined, but the REG value is not, and I can't currently see how that can happen. > "<<" is invalid operands in assembly, is there something wrong with me? At the moment I can only assume there is a local problem. I'd suspect a typo somewhere, but maybe you have a toolchain which behaves differently? Thanks, Mark.
在 2022/6/20 17:10, Mark Rutland 写道: > On Mon, Jun 20, 2022 at 10:59:12AM +0800, Tong Tiangen wrote: >> 在 2022/6/18 20:40, Mark Rutland 写道: >>> On Sat, Jun 18, 2022 at 04:42:06PM +0800, Tong Tiangen wrote: >>>>>>> diff --git a/arch/arm64/include/asm/asm-extable.h >>>>>>> b/arch/arm64/include/asm/asm-extable.h >>>>>>> index 56ebe183e78b..9c94ac1f082c 100644 >>>>>>> --- a/arch/arm64/include/asm/asm-extable.h >>>>>>> +++ b/arch/arm64/include/asm/asm-extable.h >>>>>>> @@ -28,6 +28,14 @@ >>>>>>> __ASM_EXTABLE_RAW(\insn, \fixup, EX_TYPE_FIXUP, 0) >>>>>>> .endm >>>>>>> +/* >>>>>>> + * Create an exception table entry for uaccess `insn`, which >>>>>>> will branch to `fixup` >>>>>>> + * when an unhandled fault is taken. >>>>>>> + * ex->data = ~0 means both reg_err and reg_zero is set to wzr(x31). >>>>>>> + */ >>>>>>> + .macro _asm_extable_uaccess, insn, fixup >>>>>>> + __ASM_EXTABLE_RAW(\insn, \fixup, EX_TYPE_UACCESS_ERR_ZERO, ~0) >>>>>>> + .endm >>>>>> >>>>>> I'm not too keen on using `~0` here, since that also sets other bits >>>>>> in the >>>>>> data field, and its somewhat opaque. >>>>>> >>>>>> How painful is it to generate the data fields as with the C version >>>>>> of this >>>>>> macro, so that we can pass in wzr explciitly for the two sub-fields? >>>>>> >>>>>> Other than that, this looks good to me. >>>>>> >>>>>> Thanks, >>>>>> Mark. >>>>> >>>>> ok, will fix next version. >>>>> >>>>> Thanks, >>>>> Tong. >>>> >>>> I tried to using data filelds as with C version, but here assembly code we >>>> can not using operator such as << and |, if we use lsl and orr instructions, >>>> the gpr will be occupied. >>>> >>>> So how about using 0x3ff directly here? it means err register and zero >>>> register both set to x31. >>> >>> I had a go at implementing this, and it seems simple enough. Please see: >>> >>> https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/log/?h=arm64/extable/asm-uaccess >>> >> >> I made the following modifications, and the other parts are based on your >> implementation: >> >> arch/arm64/include/asm/asm-extable.h >> [...] >> .macro _asm_extable_uaccess, insn, fixup >> _ASM_EXTABLE_UACCESS(\insn, \fixup) >> .endm >> [...] > > I also made this same change locally when testing, and building with GCC 11.1.0 > or LLVM 14.0.0 I am not seeing any problem when building, and the result is as > expected: > > | [mark@lakrids:~/src/linux]% usekorg 11.1.0 make ARCH=arm64 CROSS_COMPILE=aarch64-linux- defconfig > | *** Default configuration is based on 'defconfig' > | # > | # No change to .config > | # > | [mark@lakrids:~/src/linux]% usekorg 11.1.0 make ARCH=arm64 CROSS_COMPILE=aarch64-linux- -j50 arch/arm64/lib/ > | CALL scripts/atomic/check-atomics.sh > | CC arch/arm64/kernel/asm-offsets.s > | CALL scripts/checksyscalls.sh > | AS arch/arm64/kernel/vdso/note.o > | AS arch/arm64/kernel/vdso/sigreturn.o > | LD arch/arm64/kernel/vdso/vdso.so.dbg > | VDSOSYM include/generated/vdso-offsets.h > | OBJCOPY arch/arm64/kernel/vdso/vdso.so > | make[2]: Nothing to be done for 'arch/arm64/lib/'. > | AS arch/arm64/lib/clear_page.o > | AS arch/arm64/lib/clear_user.o > | AS arch/arm64/lib/copy_from_user.o > | AS arch/arm64/lib/copy_page.o > | AS arch/arm64/lib/copy_to_user.o > | CC arch/arm64/lib/csum.o > | CC arch/arm64/lib/delay.o > | AS arch/arm64/lib/memchr.o > | AS arch/arm64/lib/memcmp.o > | AS arch/arm64/lib/memcpy.o > | AS arch/arm64/lib/memset.o > | AS arch/arm64/lib/strchr.o > | AS arch/arm64/lib/strcmp.o > | AS arch/arm64/lib/strlen.o > | AS arch/arm64/lib/strncmp.o > | AS arch/arm64/lib/strnlen.o > | AS arch/arm64/lib/strrchr.o > | AS arch/arm64/lib/tishift.o > | AS arch/arm64/lib/crc32.o > | AS arch/arm64/lib/mte.o > | CC [M] arch/arm64/lib/xor-neon.o > | AR arch/arm64/lib/built-in.a > | AR arch/arm64/lib/lib.a > | [mark@lakrids:~/src/linux]% usekorg 12.1.0 aarch64-linux-objdump -j __ex_table -D arch/arm64/lib/clear_user.o > | > | arch/arm64/lib/clear_user.o: file format elf64-littleaarch64 > | > | > | Disassembly of section __ex_table: > | > | 0000000000000000 <__ex_table>: > | ... > | 8: 03ff0003 .inst 0x03ff0003 ; undefined > | ... > | 14: 03ff0003 .inst 0x03ff0003 ; undefined > | ... > | 20: 03ff0003 .inst 0x03ff0003 ; undefined > | ... > | 2c: 03ff0003 .inst 0x03ff0003 ; undefined > | ... > | 38: 03ff0003 .inst 0x03ff0003 ; undefined > | ... > | 44: 03ff0003 .inst 0x03ff0003 ; undefined > >> The following errors are reported during compilation: >> [...] >> arch/arm64/lib/clear_user.S:45: Error: invalid operands (*ABS* and *UND* >> sections) for `<<' >> [...] > > As above, I'm not seeing this. > > This suggests that the EX_DATA_REG() macro is going wrong somehow. Assuming the > operand types correspond to the LHS and RHS of the expression, this would mean > the GPR number is defined, but the REG value is not, and I can't currently see > how that can happen. > >> "<<" is invalid operands in assembly, is there something wrong with me? > > At the moment I can only assume there is a local problem. I'd suspect a typo > somewhere, but maybe you have a toolchain which behaves differently? > > Thanks, > Mark. My gcc version is 9.4.0, let me try your gcc version first. Thanks, Tong. > .
在 2022/6/20 17:10, Mark Rutland 写道: > On Mon, Jun 20, 2022 at 10:59:12AM +0800, Tong Tiangen wrote: >> 在 2022/6/18 20:40, Mark Rutland 写道: >>> On Sat, Jun 18, 2022 at 04:42:06PM +0800, Tong Tiangen wrote: >>>>>>> diff --git a/arch/arm64/include/asm/asm-extable.h >>>>>>> b/arch/arm64/include/asm/asm-extable.h >>>>>>> index 56ebe183e78b..9c94ac1f082c 100644 >>>>>>> --- a/arch/arm64/include/asm/asm-extable.h >>>>>>> +++ b/arch/arm64/include/asm/asm-extable.h >>>>>>> @@ -28,6 +28,14 @@ >>>>>>> __ASM_EXTABLE_RAW(\insn, \fixup, EX_TYPE_FIXUP, 0) >>>>>>> .endm >>>>>>> +/* >>>>>>> + * Create an exception table entry for uaccess `insn`, which >>>>>>> will branch to `fixup` >>>>>>> + * when an unhandled fault is taken. >>>>>>> + * ex->data = ~0 means both reg_err and reg_zero is set to wzr(x31). >>>>>>> + */ >>>>>>> + .macro _asm_extable_uaccess, insn, fixup >>>>>>> + __ASM_EXTABLE_RAW(\insn, \fixup, EX_TYPE_UACCESS_ERR_ZERO, ~0) >>>>>>> + .endm >>>>>> >>>>>> I'm not too keen on using `~0` here, since that also sets other bits >>>>>> in the >>>>>> data field, and its somewhat opaque. >>>>>> >>>>>> How painful is it to generate the data fields as with the C version >>>>>> of this >>>>>> macro, so that we can pass in wzr explciitly for the two sub-fields? >>>>>> >>>>>> Other than that, this looks good to me. >>>>>> >>>>>> Thanks, >>>>>> Mark. >>>>> >>>>> ok, will fix next version. >>>>> >>>>> Thanks, >>>>> Tong. >>>> >>>> I tried to using data filelds as with C version, but here assembly code we >>>> can not using operator such as << and |, if we use lsl and orr instructions, >>>> the gpr will be occupied. >>>> >>>> So how about using 0x3ff directly here? it means err register and zero >>>> register both set to x31. >>> >>> I had a go at implementing this, and it seems simple enough. Please see: >>> >>> https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/log/?h=arm64/extable/asm-uaccess >>> >> >> I made the following modifications, and the other parts are based on your >> implementation: >> >> arch/arm64/include/asm/asm-extable.h >> [...] >> .macro _asm_extable_uaccess, insn, fixup >> _ASM_EXTABLE_UACCESS(\insn, \fixup) >> .endm >> [...] > > I also made this same change locally when testing, and building with GCC 11.1.0 > or LLVM 14.0.0 I am not seeing any problem when building, and the result is as > expected: > > | [mark@lakrids:~/src/linux]% usekorg 11.1.0 make ARCH=arm64 CROSS_COMPILE=aarch64-linux- defconfig > | *** Default configuration is based on 'defconfig' > | # > | # No change to .config > | # > | [mark@lakrids:~/src/linux]% usekorg 11.1.0 make ARCH=arm64 CROSS_COMPILE=aarch64-linux- -j50 arch/arm64/lib/ > | CALL scripts/atomic/check-atomics.sh > | CC arch/arm64/kernel/asm-offsets.s > | CALL scripts/checksyscalls.sh > | AS arch/arm64/kernel/vdso/note.o > | AS arch/arm64/kernel/vdso/sigreturn.o > | LD arch/arm64/kernel/vdso/vdso.so.dbg > | VDSOSYM include/generated/vdso-offsets.h > | OBJCOPY arch/arm64/kernel/vdso/vdso.so > | make[2]: Nothing to be done for 'arch/arm64/lib/'. > | AS arch/arm64/lib/clear_page.o > | AS arch/arm64/lib/clear_user.o > | AS arch/arm64/lib/copy_from_user.o > | AS arch/arm64/lib/copy_page.o > | AS arch/arm64/lib/copy_to_user.o > | CC arch/arm64/lib/csum.o > | CC arch/arm64/lib/delay.o > | AS arch/arm64/lib/memchr.o > | AS arch/arm64/lib/memcmp.o > | AS arch/arm64/lib/memcpy.o > | AS arch/arm64/lib/memset.o > | AS arch/arm64/lib/strchr.o > | AS arch/arm64/lib/strcmp.o > | AS arch/arm64/lib/strlen.o > | AS arch/arm64/lib/strncmp.o > | AS arch/arm64/lib/strnlen.o > | AS arch/arm64/lib/strrchr.o > | AS arch/arm64/lib/tishift.o > | AS arch/arm64/lib/crc32.o > | AS arch/arm64/lib/mte.o > | CC [M] arch/arm64/lib/xor-neon.o > | AR arch/arm64/lib/built-in.a > | AR arch/arm64/lib/lib.a > | [mark@lakrids:~/src/linux]% usekorg 12.1.0 aarch64-linux-objdump -j __ex_table -D arch/arm64/lib/clear_user.o > | > | arch/arm64/lib/clear_user.o: file format elf64-littleaarch64 > | > | > | Disassembly of section __ex_table: > | > | 0000000000000000 <__ex_table>: > | ... > | 8: 03ff0003 .inst 0x03ff0003 ; undefined > | ... > | 14: 03ff0003 .inst 0x03ff0003 ; undefined > | ... > | 20: 03ff0003 .inst 0x03ff0003 ; undefined > | ... > | 2c: 03ff0003 .inst 0x03ff0003 ; undefined > | ... > | 38: 03ff0003 .inst 0x03ff0003 ; undefined > | ... > | 44: 03ff0003 .inst 0x03ff0003 ; undefined > >> The following errors are reported during compilation: >> [...] >> arch/arm64/lib/clear_user.S:45: Error: invalid operands (*ABS* and *UND* >> sections) for `<<' >> [...] > > As above, I'm not seeing this. > > This suggests that the EX_DATA_REG() macro is going wrong somehow. Assuming the > operand types correspond to the LHS and RHS of the expression, this would mean > the GPR number is defined, but the REG value is not, and I can't currently see > how that can happen. > >> "<<" is invalid operands in assembly, is there something wrong with me? > > At the moment I can only assume there is a local problem. I'd suspect a typo > somewhere, but maybe you have a toolchain which behaves differently? > > Thanks, > Mark. > . Now I can compile success, both versions 9.4.0 and 11.2.0. I should have made a mistake. There is no problem using your implementation. I will send a new version these days. Thans, Tong.
On Mon, Jun 20, 2022 at 10:13:41PM +0800, Tong Tiangen wrote: > > > 在 2022/6/20 17:10, Mark Rutland 写道: > > On Mon, Jun 20, 2022 at 10:59:12AM +0800, Tong Tiangen wrote: > > > 在 2022/6/18 20:40, Mark Rutland 写道: > > > The following errors are reported during compilation: > > > [...] > > > arch/arm64/lib/clear_user.S:45: Error: invalid operands (*ABS* and *UND* > > > sections) for `<<' > > > [...] > > > > As above, I'm not seeing this. > > > > This suggests that the EX_DATA_REG() macro is going wrong somehow. Assuming the > > operand types correspond to the LHS and RHS of the expression, this would mean > > the GPR number is defined, but the REG value is not, and I can't currently see > > how that can happen. > Now I can compile success, both versions 9.4.0 and 11.2.0. > > I should have made a mistake. There is no problem using your implementation. > I will send a new version these days. No problem; thanks for confirming! Mark.
diff --git a/arch/arm64/include/asm/asm-extable.h b/arch/arm64/include/asm/asm-extable.h index 56ebe183e78b..9c94ac1f082c 100644 --- a/arch/arm64/include/asm/asm-extable.h +++ b/arch/arm64/include/asm/asm-extable.h @@ -28,6 +28,14 @@ __ASM_EXTABLE_RAW(\insn, \fixup, EX_TYPE_FIXUP, 0) .endm +/* + * Create an exception table entry for uaccess `insn`, which will branch to `fixup` + * when an unhandled fault is taken. + * ex->data = ~0 means both reg_err and reg_zero is set to wzr(x31). + */ + .macro _asm_extable_uaccess, insn, fixup + __ASM_EXTABLE_RAW(\insn, \fixup, EX_TYPE_UACCESS_ERR_ZERO, ~0) + .endm /* * Create an exception table entry for `insn` if `fixup` is provided. Otherwise * do nothing. diff --git a/arch/arm64/include/asm/asm-uaccess.h b/arch/arm64/include/asm/asm-uaccess.h index 0557af834e03..75b211c98dea 100644 --- a/arch/arm64/include/asm/asm-uaccess.h +++ b/arch/arm64/include/asm/asm-uaccess.h @@ -61,7 +61,7 @@ alternative_else_nop_endif #define USER(l, x...) \ 9999: x; \ - _asm_extable 9999b, l + _asm_extable_uaccess 9999b, l /* * Generate the assembly for LDTR/STTR with exception table entries. @@ -73,8 +73,8 @@ alternative_else_nop_endif 8889: ldtr \reg2, [\addr, #8]; add \addr, \addr, \post_inc; - _asm_extable 8888b,\l; - _asm_extable 8889b,\l; + _asm_extable_uaccess 8888b, \l; + _asm_extable_uaccess 8889b, \l; .endm .macro user_stp l, reg1, reg2, addr, post_inc @@ -82,14 +82,14 @@ alternative_else_nop_endif 8889: sttr \reg2, [\addr, #8]; add \addr, \addr, \post_inc; - _asm_extable 8888b,\l; - _asm_extable 8889b,\l; + _asm_extable_uaccess 8888b,\l; + _asm_extable_uaccess 8889b,\l; .endm .macro user_ldst l, inst, reg, addr, post_inc 8888: \inst \reg, [\addr]; add \addr, \addr, \post_inc; - _asm_extable 8888b,\l; + _asm_extable_uaccess 8888b, \l; .endm #endif
Currnetly, the extable type used by __arch_copy_from/to_user() is EX_TYPE_FIXUP. In fact, It is more clearly to use meaningful EX_TYPE_UACCESS_*. Suggested-by: Mark Rutland <mark.rutland@arm.com> Signed-off-by: Tong Tiangen <tongtiangen@huawei.com> --- arch/arm64/include/asm/asm-extable.h | 8 ++++++++ arch/arm64/include/asm/asm-uaccess.h | 12 ++++++------ 2 files changed, 14 insertions(+), 6 deletions(-)