| Message ID | 06c06dde5ee635c6d1ebf66a8cff9e7e1f4fbf5c.1674818705.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | RISCV basic exception handling implementation | expand |
On 27.01.2023 14:59, Oleksii Kurochko wrote: > The patch introduces macros: BUG(), WARN(), run_in_exception(), > assert_failed. > > The implementation uses "ebreak" instruction in combination with > diffrent bug frame tables (for each type) which contains useful > information. > > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> > --- > Changes: > - Remove __ in define namings > - Update run_in_exception_handler() with > register void *fn_ asm(__stringify(BUG_FN_REG)) = (fn); > - Remove bug_instr_t type and change it's usage to uint32_t But that's not correct - as said before, you can't assume you can access 32 bits, there maybe only a 16-bit insn at the end of a page, with nothing mapped to the VA of the subsequent page. Even more ... > + end: > + regs->sepc += GET_INSN_LENGTH(*(uint32_t *)pc); ... you obtain insn length you don't even need to read 32 bits. Jan
On 27.01.2023 14:59, Oleksii Kurochko wrote: > +int is_valid_bugaddr(uint32_t insn) > +{ > + if ((insn & INSN_LENGTH_MASK) == INSN_LENGTH_32) > + return (insn == BUG_INSN_32); > + else > + return ((insn & COMPRESSED_INSN_MASK) == BUG_INSN_16); > +} > + > void do_trap(struct cpu_user_regs *cpu_regs) > { > + register_t pc = cpu_regs->sepc; > + uint32_t instr = *(uint32_t *)pc; > + > + if (is_valid_bugaddr(instr)) > + if (!do_bug_frame(cpu_regs, pc)) return; > + > do_unexpected_trap(cpu_regs); > } One more remark, style related: Even if some of the additions you're making are temporary, it'll be better if you have everything in Xen style. That'll reduce the risk of someone copying bad style from adjacent code, and it'll also avoid people like me thinking whether to comment and request an adjustment, or whether to assume that it's temporary code and will get changed again anyway. Jan
Hi Oleksii, On 27/01/2023 13:59, Oleksii Kurochko wrote: > The patch introduces macros: BUG(), WARN(), run_in_exception(), > assert_failed. > > The implementation uses "ebreak" instruction in combination with > diffrent bug frame tables (for each type) which contains useful > information. > > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> > --- > Changes: > - Remove __ in define namings > - Update run_in_exception_handler() with > register void *fn_ asm(__stringify(BUG_FN_REG)) = (fn); > - Remove bug_instr_t type and change it's usage to uint32_t > --- > xen/arch/riscv/include/asm/bug.h | 118 ++++++++++++++++++++++++++++ > xen/arch/riscv/traps.c | 128 +++++++++++++++++++++++++++++++ > xen/arch/riscv/xen.lds.S | 10 +++ > 3 files changed, 256 insertions(+) > create mode 100644 xen/arch/riscv/include/asm/bug.h > > diff --git a/xen/arch/riscv/include/asm/bug.h b/xen/arch/riscv/include/asm/bug.h > new file mode 100644 > index 0000000000..4b15d8eba6 > --- /dev/null > +++ b/xen/arch/riscv/include/asm/bug.h > @@ -0,0 +1,118 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Copyright (C) 2012 Regents of the University of California > + * Copyright (C) 2021-2023 Vates I have to question the two copyrights here given that the majority of the code seems to be taken from the arm implementation (see arch/arm/include/asm/bug.h). With that said, we should consolidate the code rather than duplicating it on every architecture. Cheers,
On Fri, 2023-01-27 at 15:34 +0100, Jan Beulich wrote: > On 27.01.2023 14:59, Oleksii Kurochko wrote: > > The patch introduces macros: BUG(), WARN(), run_in_exception(), > > assert_failed. > > > > The implementation uses "ebreak" instruction in combination with > > diffrent bug frame tables (for each type) which contains useful > > information. > > > > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> > > --- > > Changes: > > - Remove __ in define namings > > - Update run_in_exception_handler() with > > register void *fn_ asm(__stringify(BUG_FN_REG)) = (fn); > > - Remove bug_instr_t type and change it's usage to uint32_t > > But that's not correct - as said before, you can't assume you can > access > 32 bits, there maybe only a 16-bit insn at the end of a page, with > nothing > mapped to the VA of the subsequent page. Even more ... > Agree that it will be an issue if 16-bit insn will be at the end of a page. The code is based on Linux (https://elixir.bootlin.com/linux/latest/source/arch/riscv/kernel/traps.c#L152) and it seems they might have the same issue. > > + end: > > + regs->sepc += GET_INSN_LENGTH(*(uint32_t *)pc); > > ... you obtain insn length you don't even need to read 32 bits. > It looks you are right so I'll change that in the new version of the patch series. > Jan Oleksii
Hi Julien, On Fri, 2023-01-27 at 16:02 +0000, Julien Grall wrote: > Hi Oleksii, > > On 27/01/2023 13:59, Oleksii Kurochko wrote: > > The patch introduces macros: BUG(), WARN(), run_in_exception(), > > assert_failed. > > > > The implementation uses "ebreak" instruction in combination with > > diffrent bug frame tables (for each type) which contains useful > > information. > > > > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> > > --- > > Changes: > > - Remove __ in define namings > > - Update run_in_exception_handler() with > > register void *fn_ asm(__stringify(BUG_FN_REG)) = (fn); > > - Remove bug_instr_t type and change it's usage to uint32_t > > --- > > xen/arch/riscv/include/asm/bug.h | 118 > > ++++++++++++++++++++++++++++ > > xen/arch/riscv/traps.c | 128 > > +++++++++++++++++++++++++++++++ > > xen/arch/riscv/xen.lds.S | 10 +++ > > 3 files changed, 256 insertions(+) > > create mode 100644 xen/arch/riscv/include/asm/bug.h > > > > diff --git a/xen/arch/riscv/include/asm/bug.h > > b/xen/arch/riscv/include/asm/bug.h > > new file mode 100644 > > index 0000000000..4b15d8eba6 > > --- /dev/null > > +++ b/xen/arch/riscv/include/asm/bug.h > > @@ -0,0 +1,118 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +/* > > + * Copyright (C) 2012 Regents of the University of California > > + * Copyright (C) 2021-2023 Vates > > I have to question the two copyrights here given that the majority of > the code seems to be taken from the arm implementation (see > arch/arm/include/asm/bug.h). > > With that said, we should consolidate the code rather than > duplicating > it on every architecture. > Copyrights should be removed. They were taken from the previous implementation of bug.h for RISC-V so I just forgot to remove them. It looks like we should have common bug.h for ARM and RISCV but I am not sure that I know how to do that better. Probably the code wants to be moved to xen/include/bug.h and using ifdef ARM && RISCV ... But still I am not sure that this is the best one option as at least we have different implementation for x86_64. > Cheers, > ~ Oleksii
On 30.01.23 12:35, Oleksii wrote: > Hi Julien, > On Fri, 2023-01-27 at 16:02 +0000, Julien Grall wrote: >> Hi Oleksii, >> >> On 27/01/2023 13:59, Oleksii Kurochko wrote: >>> The patch introduces macros: BUG(), WARN(), run_in_exception(), >>> assert_failed. >>> >>> The implementation uses "ebreak" instruction in combination with >>> diffrent bug frame tables (for each type) which contains useful >>> information. >>> >>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> >>> --- >>> Changes: >>> - Remove __ in define namings >>> - Update run_in_exception_handler() with >>> register void *fn_ asm(__stringify(BUG_FN_REG)) = (fn); >>> - Remove bug_instr_t type and change it's usage to uint32_t >>> --- >>> xen/arch/riscv/include/asm/bug.h | 118 >>> ++++++++++++++++++++++++++++ >>> xen/arch/riscv/traps.c | 128 >>> +++++++++++++++++++++++++++++++ >>> xen/arch/riscv/xen.lds.S | 10 +++ >>> 3 files changed, 256 insertions(+) >>> create mode 100644 xen/arch/riscv/include/asm/bug.h >>> >>> diff --git a/xen/arch/riscv/include/asm/bug.h >>> b/xen/arch/riscv/include/asm/bug.h >>> new file mode 100644 >>> index 0000000000..4b15d8eba6 >>> --- /dev/null >>> +++ b/xen/arch/riscv/include/asm/bug.h >>> @@ -0,0 +1,118 @@ >>> +/* SPDX-License-Identifier: GPL-2.0 */ >>> +/* >>> + * Copyright (C) 2012 Regents of the University of California >>> + * Copyright (C) 2021-2023 Vates >> >> I have to question the two copyrights here given that the majority of >> the code seems to be taken from the arm implementation (see >> arch/arm/include/asm/bug.h). >> >> With that said, we should consolidate the code rather than >> duplicating >> it on every architecture. >> > Copyrights should be removed. They were taken from the previous > implementation of bug.h for RISC-V so I just forgot to remove them. > > It looks like we should have common bug.h for ARM and RISCV but I am > not sure that I know how to do that better. > Probably the code wants to be moved to xen/include/bug.h and using > ifdef ARM && RISCV ... > But still I am not sure that this is the best one option as at least we > have different implementation for x86_64. There are already a lot of duplicated #defines in the Arm and x86 asm/bug.h files. I'd create xen/include/xen/bug.h including asm/bug.h first and then adding all the common stuff. In case 2 archs are sharing some #define FOO you could #define FOO in the asm/bug.h for the arch not using the common definition and do #ifndef FOO in xen/include/xen/bug.h around the variant shared by the other archs. Juergen
Hi Oleksii, On 30/01/2023 11:35, Oleksii wrote: > Hi Julien, > On Fri, 2023-01-27 at 16:02 +0000, Julien Grall wrote: >> Hi Oleksii, >> >> On 27/01/2023 13:59, Oleksii Kurochko wrote: >>> The patch introduces macros: BUG(), WARN(), run_in_exception(), >>> assert_failed. >>> >>> The implementation uses "ebreak" instruction in combination with >>> diffrent bug frame tables (for each type) which contains useful >>> information. >>> >>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> >>> --- >>> Changes: >>> - Remove __ in define namings >>> - Update run_in_exception_handler() with >>> register void *fn_ asm(__stringify(BUG_FN_REG)) = (fn); >>> - Remove bug_instr_t type and change it's usage to uint32_t >>> --- >>> xen/arch/riscv/include/asm/bug.h | 118 >>> ++++++++++++++++++++++++++++ >>> xen/arch/riscv/traps.c | 128 >>> +++++++++++++++++++++++++++++++ >>> xen/arch/riscv/xen.lds.S | 10 +++ >>> 3 files changed, 256 insertions(+) >>> create mode 100644 xen/arch/riscv/include/asm/bug.h >>> >>> diff --git a/xen/arch/riscv/include/asm/bug.h >>> b/xen/arch/riscv/include/asm/bug.h >>> new file mode 100644 >>> index 0000000000..4b15d8eba6 >>> --- /dev/null >>> +++ b/xen/arch/riscv/include/asm/bug.h >>> @@ -0,0 +1,118 @@ >>> +/* SPDX-License-Identifier: GPL-2.0 */ >>> +/* >>> + * Copyright (C) 2012 Regents of the University of California >>> + * Copyright (C) 2021-2023 Vates >> >> I have to question the two copyrights here given that the majority of >> the code seems to be taken from the arm implementation (see >> arch/arm/include/asm/bug.h). >> >> With that said, we should consolidate the code rather than >> duplicating >> it on every architecture. >> > Copyrights should be removed. They were taken from the previous > implementation of bug.h for RISC-V so I just forgot to remove them. > > It looks like we should have common bug.h for ARM and RISCV but I am > not sure that I know how to do that better. > Probably the code wants to be moved to xen/include/bug.h and using > ifdef ARM && RISCV ... Or you could introduce CONFIG_BUG_GENERIC or else, so it is easily selectable by other architecture. > But still I am not sure that this is the best one option as at least we > have different implementation for x86_64. My main concern is the maintainance effort. For every bug, we would need to fix it in two places. The risk is we may forget to fix one architecture. This is not a very ideal situation. So I think sharing the header between RISC-V and Arm (or x86, see below) is at least a must. We can do the 3rd architecture at a leisure pace. One option would be to introduce asm-generic like Linux (IIRC this was a suggestion from Andrew). This would also to share code between two of the archs. Also, from a brief look, the difference in implementation is mainly because on Arm we can't use %c (some version of GCC didn't support it). Is this also the case on RISC-V? If not, you may want to consider to use the x86 version. Cheers,
On Mon, 2023-01-30 at 22:28 +0000, Julien Grall wrote: > Hi Oleksii, > > On 30/01/2023 11:35, Oleksii wrote: > > Hi Julien, > > On Fri, 2023-01-27 at 16:02 +0000, Julien Grall wrote: > > > Hi Oleksii, > > > > > > On 27/01/2023 13:59, Oleksii Kurochko wrote: > > > > The patch introduces macros: BUG(), WARN(), run_in_exception(), > > > > assert_failed. > > > > > > > > The implementation uses "ebreak" instruction in combination > > > > with > > > > diffrent bug frame tables (for each type) which contains useful > > > > information. > > > > > > > > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> > > > > --- > > > > Changes: > > > > - Remove __ in define namings > > > > - Update run_in_exception_handler() with > > > > register void *fn_ asm(__stringify(BUG_FN_REG)) = (fn); > > > > - Remove bug_instr_t type and change it's usage to uint32_t > > > > --- > > > > xen/arch/riscv/include/asm/bug.h | 118 > > > > ++++++++++++++++++++++++++++ > > > > xen/arch/riscv/traps.c | 128 > > > > +++++++++++++++++++++++++++++++ > > > > xen/arch/riscv/xen.lds.S | 10 +++ > > > > 3 files changed, 256 insertions(+) > > > > create mode 100644 xen/arch/riscv/include/asm/bug.h > > > > > > > > diff --git a/xen/arch/riscv/include/asm/bug.h > > > > b/xen/arch/riscv/include/asm/bug.h > > > > new file mode 100644 > > > > index 0000000000..4b15d8eba6 > > > > --- /dev/null > > > > +++ b/xen/arch/riscv/include/asm/bug.h > > > > @@ -0,0 +1,118 @@ > > > > +/* SPDX-License-Identifier: GPL-2.0 */ > > > > +/* > > > > + * Copyright (C) 2012 Regents of the University of California > > > > + * Copyright (C) 2021-2023 Vates > > > > > > I have to question the two copyrights here given that the > > > majority of > > > the code seems to be taken from the arm implementation (see > > > arch/arm/include/asm/bug.h). > > > > > > With that said, we should consolidate the code rather than > > > duplicating > > > it on every architecture. > > > > > Copyrights should be removed. They were taken from the previous > > implementation of bug.h for RISC-V so I just forgot to remove them. > > > > It looks like we should have common bug.h for ARM and RISCV but I > > am > > not sure that I know how to do that better. > > Probably the code wants to be moved to xen/include/bug.h and using > > ifdef ARM && RISCV ... > > Or you could introduce CONFIG_BUG_GENERIC or else, so it is easily > selectable by other architecture. > > > But still I am not sure that this is the best one option as at > > least we > > have different implementation for x86_64. > > My main concern is the maintainance effort. For every bug, we would > need > to fix it in two places. The risk is we may forget to fix one > architecture. > This is not a very ideal situation. > > So I think sharing the header between RISC-V and Arm (or x86, see > below) > is at least a must. We can do the 3rd architecture at a leisure pace. > > One option would be to introduce asm-generic like Linux (IIRC this > was a > suggestion from Andrew). This would also to share code between two of > the archs. > > Also, from a brief look, the difference in implementation is mainly > because on Arm we can't use %c (some version of GCC didn't support > it). > Is this also the case on RISC-V? If not, you may want to consider to > use > the x86 version. > No, it shouldn't be an issue for RISC-V. I'll double check. Any way it should bug.h should be shared between archs so I am going to rework that in this patch series and sent the changes in the next version of the patch series. Thanks. ~Oleksii > Cheers, >
Hi Julien, On Mon, 2023-01-30 at 22:28 +0000, Julien Grall wrote: > Hi Oleksii, > > On 30/01/2023 11:35, Oleksii wrote: > > Hi Julien, > > On Fri, 2023-01-27 at 16:02 +0000, Julien Grall wrote: > > > Hi Oleksii, > > > > > > On 27/01/2023 13:59, Oleksii Kurochko wrote: > > > > The patch introduces macros: BUG(), WARN(), run_in_exception(), > > > > assert_failed. > > > > > > > > The implementation uses "ebreak" instruction in combination > > > > with > > > > diffrent bug frame tables (for each type) which contains useful > > > > information. > > > > > > > > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> > > > > --- > > > > Changes: > > > > - Remove __ in define namings > > > > - Update run_in_exception_handler() with > > > > register void *fn_ asm(__stringify(BUG_FN_REG)) = (fn); > > > > - Remove bug_instr_t type and change it's usage to uint32_t > > > > --- > > > > xen/arch/riscv/include/asm/bug.h | 118 > > > > ++++++++++++++++++++++++++++ > > > > xen/arch/riscv/traps.c | 128 > > > > +++++++++++++++++++++++++++++++ > > > > xen/arch/riscv/xen.lds.S | 10 +++ > > > > 3 files changed, 256 insertions(+) > > > > create mode 100644 xen/arch/riscv/include/asm/bug.h > > > > > > > > diff --git a/xen/arch/riscv/include/asm/bug.h > > > > b/xen/arch/riscv/include/asm/bug.h > > > > new file mode 100644 > > > > index 0000000000..4b15d8eba6 > > > > --- /dev/null > > > > +++ b/xen/arch/riscv/include/asm/bug.h > > > > @@ -0,0 +1,118 @@ > > > > +/* SPDX-License-Identifier: GPL-2.0 */ > > > > +/* > > > > + * Copyright (C) 2012 Regents of the University of California > > > > + * Copyright (C) 2021-2023 Vates > > > > > > I have to question the two copyrights here given that the > > > majority of > > > the code seems to be taken from the arm implementation (see > > > arch/arm/include/asm/bug.h). > > > > > > With that said, we should consolidate the code rather than > > > duplicating > > > it on every architecture. > > > > > Copyrights should be removed. They were taken from the previous > > implementation of bug.h for RISC-V so I just forgot to remove them. > > > > It looks like we should have common bug.h for ARM and RISCV but I > > am > > not sure that I know how to do that better. > > Probably the code wants to be moved to xen/include/bug.h and using > > ifdef ARM && RISCV ... > > Or you could introduce CONFIG_BUG_GENERIC or else, so it is easily > selectable by other architecture. > > > But still I am not sure that this is the best one option as at > > least we > > have different implementation for x86_64. > > My main concern is the maintainance effort. For every bug, we would > need > to fix it in two places. The risk is we may forget to fix one > architecture. > This is not a very ideal situation. > > So I think sharing the header between RISC-V and Arm (or x86, see > below) > is at least a must. We can do the 3rd architecture at a leisure pace. > > One option would be to introduce asm-generic like Linux (IIRC this > was a > suggestion from Andrew). This would also to share code between two of > the archs. > > Also, from a brief look, the difference in implementation is mainly > because on Arm we can't use %c (some version of GCC didn't support > it). > Is this also the case on RISC-V? If not, you may want to consider to > use > the x86 version. > I did several experiments related to '%c' in inline assembly for RISC-V and it seems that '%c' doesn't support all forms of the use of '%c'. I wrote the following macros and they have been compiled without any errors: ..... #define _ASM_BUGFRAME_TEXT(second_frame) \ ".Lbug%=: ebreak\n" \ ".pushsection .bug_frames.%c[bf_type], \"a\", @progbits\n" \ ".p2align 2\n" \ ".Lfrm%=:\n" \ ".long (.Lfrm%=)\n" \ ".long (0x55555555)\n" \ ".long (.Lbug%=)\n" \ ".long (0x55555555)\n" \ ".long %c[bf_line_hi]\n" \ ".long (0x55555555)\n" \ ".long %[bf_line_hi]\n" \ ".long (0x55555555)\n" \ ".long %[bf_line_lo]\n" \ ".long (0x55555555)\n" \ ".long %[bf_ptr]\n" \ ".long (0x55555555)\n" \ ".long (.Lbug%= - .Lfrm%=) + %c[bf_line_hi]\n" \ ".popsection\n" \ #define _ASM_BUGFRAME_INFO(type, line, ptr, msg) \ [bf_type] "i" (type), \ [bf_ptr] "i" (ptr), \ [bf_msg] "i" (msg), \ [bf_line_lo] "i" ((line & ((1 << BUG_LINE_LO_WIDTH) - 1)) \ << BUG_DISP_WIDTH), \ [bf_line_hi] "i" (((line) >> BUG_LINE_LO_WIDTH) << BUG_DISP_WIDTH) #define BUG_FRAME(type, line, ptr, second_frame, msg) do { \ __asm__ __volatile__ ( _ASM_BUGFRAME_TEXT(second_frame) \ :: _ASM_BUGFRAME_INFO(type, line, ptr, msg) ); \ } while (0) .... But if add ".long %c[bf_ptr]\n" then the following compilation error will occur: [ error: invalid 'asm': invalid use of '%c'. ] If you look at the dissembler of _bug_frames_... ...... 80201010: 1010 addi a2,sp,32 # .Lfrm%= 80201012: 8020 .2byte 0x8020 80201014: 5555 li a0,-11 80201016: 5555 li a0,-11 80201018: 3022 .2byte 0x3022 # .Lbug%= 8020101a: 8020 .2byte 0x8020 8020101c: 5555 li a0,-11 8020101e: 5555 li a0,-11 80201020: 0000 unimp # %c[bf_line_hi] 80201022: 0000 unimp 80201024: 5555 li a0,-11 80201026: 5555 li a0,-11 80201028: 0000 unimp # %[bf_line_hi] 8020102a: 0000 unimp 8020102c: 5555 li a0,-11 8020102e: 5555 li a0,-11 80201030: 0000 unimp # %[bf_line_lo] 80201032: 1600 addi s0,sp,800 80201034: 5555 li a0,-11 80201036: 5555 li a0,-11 80201038: 10b8 addi a4,sp,104 # %[bf_ptr] 8020103a: 8020 .2byte 0x8020 8020103c: 5555 li a0,-11 8020103e: 5555 li a0,-11 80201040: 2012 .2byte 0x2012 # (.Lbug%= - .Lfrm%=) + %c[bf_line_hi] ..... ... it looks like the error will be generated if the name in %c[name] isn't equal to 0. Another thing I noticed is that %[name] can be used instead of %c[name] for RISC-V ( i did a quick check and it works) so it is still possible to follow Intel implementation but required a redefinition of _ASM_BUGFRAME_TEXT where %c[...] won't be used. All the rest will be the same as in x86 implementation: ..... #define _ASM_BUGFRAME_TEXT(second_frame) \ ".Lbug%=: ebreak\n" \ ".pushsection .bug_frames.%[bf_type], \"a\", @progbits\n" \ ".p2align 2\n" \ ".Lfrm%=:\n" \ ".long (.Lbug%= - .Lfrm%=) + %[bf_line_hi]\n" \ ".long (%[bf_ptr] - .Lfrm%=) + %[bf_line_lo]\n" \ ".if " #second_frame "\n" \ ".long 0, %[bf_msg] - .Lfrm%=\n" \ ".endif\n" \ ".popsection\n" \ ..... One thing I would like to ask you is why it's better to follow/use x86 implementation instead of ARM? It seems that "%c[...]" has the best support only for Intel GCC and thereby ARM implementation looks more universal, doesn't it? > Cheers, > ~ Oleksii
On 01/02/2023 17:40, Oleksii wrote: > Hi Julien, Hi Oleksii, > On Mon, 2023-01-30 at 22:28 +0000, Julien Grall wrote: >> Hi Oleksii, >> >> On 30/01/2023 11:35, Oleksii wrote: >>> Hi Julien, >>> On Fri, 2023-01-27 at 16:02 +0000, Julien Grall wrote: >>>> Hi Oleksii, >>>> >>>> On 27/01/2023 13:59, Oleksii Kurochko wrote: >>>>> The patch introduces macros: BUG(), WARN(), run_in_exception(), >>>>> assert_failed. >>>>> >>>>> The implementation uses "ebreak" instruction in combination >>>>> with >>>>> diffrent bug frame tables (for each type) which contains useful >>>>> information. >>>>> >>>>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> >>>>> --- >>>>> Changes: >>>>> - Remove __ in define namings >>>>> - Update run_in_exception_handler() with >>>>> register void *fn_ asm(__stringify(BUG_FN_REG)) = (fn); >>>>> - Remove bug_instr_t type and change it's usage to uint32_t >>>>> --- >>>>> xen/arch/riscv/include/asm/bug.h | 118 >>>>> ++++++++++++++++++++++++++++ >>>>> xen/arch/riscv/traps.c | 128 >>>>> +++++++++++++++++++++++++++++++ >>>>> xen/arch/riscv/xen.lds.S | 10 +++ >>>>> 3 files changed, 256 insertions(+) >>>>> create mode 100644 xen/arch/riscv/include/asm/bug.h >>>>> >>>>> diff --git a/xen/arch/riscv/include/asm/bug.h >>>>> b/xen/arch/riscv/include/asm/bug.h >>>>> new file mode 100644 >>>>> index 0000000000..4b15d8eba6 >>>>> --- /dev/null >>>>> +++ b/xen/arch/riscv/include/asm/bug.h >>>>> @@ -0,0 +1,118 @@ >>>>> +/* SPDX-License-Identifier: GPL-2.0 */ >>>>> +/* >>>>> + * Copyright (C) 2012 Regents of the University of California >>>>> + * Copyright (C) 2021-2023 Vates >>>> >>>> I have to question the two copyrights here given that the >>>> majority of >>>> the code seems to be taken from the arm implementation (see >>>> arch/arm/include/asm/bug.h). >>>> >>>> With that said, we should consolidate the code rather than >>>> duplicating >>>> it on every architecture. >>>> >>> Copyrights should be removed. They were taken from the previous >>> implementation of bug.h for RISC-V so I just forgot to remove them. >>> >>> It looks like we should have common bug.h for ARM and RISCV but I >>> am >>> not sure that I know how to do that better. >>> Probably the code wants to be moved to xen/include/bug.h and using >>> ifdef ARM && RISCV ... >> >> Or you could introduce CONFIG_BUG_GENERIC or else, so it is easily >> selectable by other architecture. >> >>> But still I am not sure that this is the best one option as at >>> least we >>> have different implementation for x86_64. >> >> My main concern is the maintainance effort. For every bug, we would >> need >> to fix it in two places. The risk is we may forget to fix one >> architecture. >> This is not a very ideal situation. >> >> So I think sharing the header between RISC-V and Arm (or x86, see >> below) >> is at least a must. We can do the 3rd architecture at a leisure pace. >> >> One option would be to introduce asm-generic like Linux (IIRC this >> was a >> suggestion from Andrew). This would also to share code between two of >> the archs. >> >> Also, from a brief look, the difference in implementation is mainly >> because on Arm we can't use %c (some version of GCC didn't support >> it). >> Is this also the case on RISC-V? If not, you may want to consider to >> use >> the x86 version. >> > I did several experiments related to '%c' in inline assembly for RISC-V > and it seems that '%c' doesn't support all forms of the use of '%c'. Thanks for checking! > I wrote the following macros and they have been compiled without any > errors: > ..... > #define _ASM_BUGFRAME_TEXT(second_frame) \ > ".Lbug%=: ebreak\n" \ > ".pushsection .bug_frames.%c[bf_type], \"a\", @progbits\n" \ > ".p2align 2\n" \ > ".Lfrm%=:\n" \ > ".long (.Lfrm%=)\n" \ > ".long (0x55555555)\n" \ > ".long (.Lbug%=)\n" \ > ".long (0x55555555)\n" \ > ".long %c[bf_line_hi]\n" \ > ".long (0x55555555)\n" \ > ".long %[bf_line_hi]\n" \ > ".long (0x55555555)\n" \ > ".long %[bf_line_lo]\n" \ > ".long (0x55555555)\n" \ > ".long %[bf_ptr]\n" \ > ".long (0x55555555)\n" \ > ".long (.Lbug%= - .Lfrm%=) + %c[bf_line_hi]\n" \ > ".popsection\n" \ > > #define _ASM_BUGFRAME_INFO(type, line, ptr, msg) \ > [bf_type] "i" (type), \ > [bf_ptr] "i" (ptr), \ > [bf_msg] "i" (msg), \ > [bf_line_lo] "i" ((line & ((1 << BUG_LINE_LO_WIDTH) - 1)) \ > << BUG_DISP_WIDTH), \ > [bf_line_hi] "i" (((line) >> BUG_LINE_LO_WIDTH) << BUG_DISP_WIDTH) > > #define BUG_FRAME(type, line, ptr, second_frame, msg) do { \ > __asm__ __volatile__ ( _ASM_BUGFRAME_TEXT(second_frame) \ > :: _ASM_BUGFRAME_INFO(type, line, ptr, msg) ); \ > } while (0) > .... > > > But if add ".long %c[bf_ptr]\n" then the following compilation error > will occur: [ error: invalid 'asm': invalid use of '%c'. ] > > If you look at the dissembler of _bug_frames_... > ...... > 80201010: 1010 addi a2,sp,32 # .Lfrm%= > 80201012: 8020 .2byte 0x8020 > 80201014: 5555 li a0,-11 > 80201016: 5555 li a0,-11 > 80201018: 3022 .2byte 0x3022 # .Lbug%= > 8020101a: 8020 .2byte 0x8020 > 8020101c: 5555 li a0,-11 > 8020101e: 5555 li a0,-11 > 80201020: 0000 unimp # %c[bf_line_hi] > 80201022: 0000 unimp > 80201024: 5555 li a0,-11 > 80201026: 5555 li a0,-11 > 80201028: 0000 unimp # %[bf_line_hi] > 8020102a: 0000 unimp > 8020102c: 5555 li a0,-11 > 8020102e: 5555 li a0,-11 > 80201030: 0000 unimp # %[bf_line_lo] > 80201032: 1600 addi s0,sp,800 > 80201034: 5555 li a0,-11 > 80201036: 5555 li a0,-11 > 80201038: 10b8 addi a4,sp,104 # %[bf_ptr] > 8020103a: 8020 .2byte 0x8020 > 8020103c: 5555 li a0,-11 > 8020103e: 5555 li a0,-11 > 80201040: 2012 .2byte 0x2012 # (.Lbug%= - > .Lfrm%=) + %c[bf_line_hi] > ..... > ... it looks like the error will be generated if the name in %c[name] > isn't equal to 0. > > Another thing I noticed is that %[name] can be used instead of %c[name] > for RISC-V ( i did a quick check and it works) so it is still possible > to follow Intel implementation but required a redefinition of > _ASM_BUGFRAME_TEXT where %c[...] won't be used. All the rest will be > the same as in x86 implementation: > ..... > #define _ASM_BUGFRAME_TEXT(second_frame) \ > ".Lbug%=: ebreak\n" \ > ".pushsection .bug_frames.%[bf_type], \"a\", @progbits\n" \ > ".p2align 2\n" \ > ".Lfrm%=:\n" \ > ".long (.Lbug%= - .Lfrm%=) + %[bf_line_hi]\n" \ > ".long (%[bf_ptr] - .Lfrm%=) + %[bf_line_lo]\n" \ > ".if " #second_frame "\n" \ > ".long 0, %[bf_msg] - .Lfrm%=\n" \ > ".endif\n" \ > ".popsection\n" \ > ..... > > One thing I would like to ask you is why it's better to follow/use x86 > implementation instead of ARM? IMHO, the x86 version is cleaner. But... > It seems that "%c[...]" has the best support only for Intel GCC and > thereby ARM implementation looks more universal, doesn't it? ... you are right, the Arm version is more portable. Although, I do wonder how GCC managed to have a different behavior between architecture as I would have expected %c to be a generic implementation. Anyway, if you are basing on the Arm one, then you should be able to 1) move arch/arm/include/asm/bug.h in asm-generic/bug.h (or similar) 2) Rename the guard and remove arm specific code.(I am not sure from where to include arm{32, 64}/bug.h) 3) Define BUG_INSTR to ebreak on RISC-V. 4) Find a place for all the RISC-V specific header 5) Move do_bug_frame() in common/bug.c I am happy to help testing the Arm version and/or help moving the code to common. Cheers,
On 01.02.2023 23:11, Julien Grall wrote: > On 01/02/2023 17:40, Oleksii wrote: >> I wrote the following macros and they have been compiled without any >> errors: >> ..... >> #define _ASM_BUGFRAME_TEXT(second_frame) \ >> ".Lbug%=: ebreak\n" \ >> ".pushsection .bug_frames.%c[bf_type], \"a\", @progbits\n" \ >> ".p2align 2\n" \ >> ".Lfrm%=:\n" \ >> ".long (.Lfrm%=)\n" \ >> ".long (0x55555555)\n" \ >> ".long (.Lbug%=)\n" \ >> ".long (0x55555555)\n" \ >> ".long %c[bf_line_hi]\n" \ >> ".long (0x55555555)\n" \ >> ".long %[bf_line_hi]\n" \ >> ".long (0x55555555)\n" \ >> ".long %[bf_line_lo]\n" \ >> ".long (0x55555555)\n" \ >> ".long %[bf_ptr]\n" \ >> ".long (0x55555555)\n" \ >> ".long (.Lbug%= - .Lfrm%=) + %c[bf_line_hi]\n" \ >> ".popsection\n" \ >> >> #define _ASM_BUGFRAME_INFO(type, line, ptr, msg) \ >> [bf_type] "i" (type), \ >> [bf_ptr] "i" (ptr), \ >> [bf_msg] "i" (msg), \ >> [bf_line_lo] "i" ((line & ((1 << BUG_LINE_LO_WIDTH) - 1)) \ >> << BUG_DISP_WIDTH), \ >> [bf_line_hi] "i" (((line) >> BUG_LINE_LO_WIDTH) << BUG_DISP_WIDTH) >> >> #define BUG_FRAME(type, line, ptr, second_frame, msg) do { \ >> __asm__ __volatile__ ( _ASM_BUGFRAME_TEXT(second_frame) \ >> :: _ASM_BUGFRAME_INFO(type, line, ptr, msg) ); \ >> } while (0) >> .... >> >> >> But if add ".long %c[bf_ptr]\n" then the following compilation error >> will occur: [ error: invalid 'asm': invalid use of '%c'. ] >> >> If you look at the dissembler of _bug_frames_... >> ...... >> 80201010: 1010 addi a2,sp,32 # .Lfrm%= >> 80201012: 8020 .2byte 0x8020 >> 80201014: 5555 li a0,-11 >> 80201016: 5555 li a0,-11 >> 80201018: 3022 .2byte 0x3022 # .Lbug%= >> 8020101a: 8020 .2byte 0x8020 >> 8020101c: 5555 li a0,-11 >> 8020101e: 5555 li a0,-11 >> 80201020: 0000 unimp # %c[bf_line_hi] >> 80201022: 0000 unimp >> 80201024: 5555 li a0,-11 >> 80201026: 5555 li a0,-11 >> 80201028: 0000 unimp # %[bf_line_hi] >> 8020102a: 0000 unimp >> 8020102c: 5555 li a0,-11 >> 8020102e: 5555 li a0,-11 >> 80201030: 0000 unimp # %[bf_line_lo] >> 80201032: 1600 addi s0,sp,800 >> 80201034: 5555 li a0,-11 >> 80201036: 5555 li a0,-11 >> 80201038: 10b8 addi a4,sp,104 # %[bf_ptr] >> 8020103a: 8020 .2byte 0x8020 >> 8020103c: 5555 li a0,-11 >> 8020103e: 5555 li a0,-11 >> 80201040: 2012 .2byte 0x2012 # (.Lbug%= - >> .Lfrm%=) + %c[bf_line_hi] >> ..... >> ... it looks like the error will be generated if the name in %c[name] >> isn't equal to 0. >> >> Another thing I noticed is that %[name] can be used instead of %c[name] >> for RISC-V ( i did a quick check and it works) so it is still possible >> to follow Intel implementation but required a redefinition of >> _ASM_BUGFRAME_TEXT where %c[...] won't be used. All the rest will be >> the same as in x86 implementation: >> ..... >> #define _ASM_BUGFRAME_TEXT(second_frame) \ >> ".Lbug%=: ebreak\n" \ >> ".pushsection .bug_frames.%[bf_type], \"a\", @progbits\n" \ >> ".p2align 2\n" \ >> ".Lfrm%=:\n" \ >> ".long (.Lbug%= - .Lfrm%=) + %[bf_line_hi]\n" \ >> ".long (%[bf_ptr] - .Lfrm%=) + %[bf_line_lo]\n" \ >> ".if " #second_frame "\n" \ >> ".long 0, %[bf_msg] - .Lfrm%=\n" \ >> ".endif\n" \ >> ".popsection\n" \ >> ..... >> >> One thing I would like to ask you is why it's better to follow/use x86 >> implementation instead of ARM? > > IMHO, the x86 version is cleaner. But... > >> It seems that "%c[...]" has the best support only for Intel GCC and >> thereby ARM implementation looks more universal, doesn't it? > > ... you are right, the Arm version is more portable. Although, I do > wonder how GCC managed to have a different behavior between architecture > as I would have expected %c to be a generic implementation. While the implementation is common, the condition when 'c' is legitimate can be customized by arch-specific code. While all code for all of the four architectures does so, perhaps x86'es goes farther with what it permits. Jan
Hi Julien, On Wed, 2023-02-01 at 22:11 +0000, Julien Grall wrote: > > > On 01/02/2023 17:40, Oleksii wrote: > > Hi Julien, > > Hi Oleksii, > > > On Mon, 2023-01-30 at 22:28 +0000, Julien Grall wrote: > > > Hi Oleksii, > > > > > > On 30/01/2023 11:35, Oleksii wrote: > > > > Hi Julien, > > > > On Fri, 2023-01-27 at 16:02 +0000, Julien Grall wrote: > > > > > Hi Oleksii, > > > > > > > > > > On 27/01/2023 13:59, Oleksii Kurochko wrote: > > > > > > The patch introduces macros: BUG(), WARN(), > > > > > > run_in_exception(), > > > > > > assert_failed. > > > > > > > > > > > > The implementation uses "ebreak" instruction in combination > > > > > > with > > > > > > diffrent bug frame tables (for each type) which contains > > > > > > useful > > > > > > information. > > > > > > > > > > > > Signed-off-by: Oleksii Kurochko > > > > > > <oleksii.kurochko@gmail.com> > > > > > > --- > > > > > > Changes: > > > > > > - Remove __ in define namings > > > > > > - Update run_in_exception_handler() with > > > > > > register void *fn_ asm(__stringify(BUG_FN_REG)) = > > > > > > (fn); > > > > > > - Remove bug_instr_t type and change it's usage to > > > > > > uint32_t > > > > > > --- > > > > > > xen/arch/riscv/include/asm/bug.h | 118 > > > > > > ++++++++++++++++++++++++++++ > > > > > > xen/arch/riscv/traps.c | 128 > > > > > > +++++++++++++++++++++++++++++++ > > > > > > xen/arch/riscv/xen.lds.S | 10 +++ > > > > > > 3 files changed, 256 insertions(+) > > > > > > create mode 100644 xen/arch/riscv/include/asm/bug.h > > > > > > > > > > > > diff --git a/xen/arch/riscv/include/asm/bug.h > > > > > > b/xen/arch/riscv/include/asm/bug.h > > > > > > new file mode 100644 > > > > > > index 0000000000..4b15d8eba6 > > > > > > --- /dev/null > > > > > > +++ b/xen/arch/riscv/include/asm/bug.h > > > > > > @@ -0,0 +1,118 @@ > > > > > > +/* SPDX-License-Identifier: GPL-2.0 */ > > > > > > +/* > > > > > > + * Copyright (C) 2012 Regents of the University of > > > > > > California > > > > > > + * Copyright (C) 2021-2023 Vates > > > > > > > > > > I have to question the two copyrights here given that the > > > > > majority of > > > > > the code seems to be taken from the arm implementation (see > > > > > arch/arm/include/asm/bug.h). > > > > > > > > > > With that said, we should consolidate the code rather than > > > > > duplicating > > > > > it on every architecture. > > > > > > > > > Copyrights should be removed. They were taken from the previous > > > > implementation of bug.h for RISC-V so I just forgot to remove > > > > them. > > > > > > > > It looks like we should have common bug.h for ARM and RISCV but > > > > I > > > > am > > > > not sure that I know how to do that better. > > > > Probably the code wants to be moved to xen/include/bug.h and > > > > using > > > > ifdef ARM && RISCV ... > > > > > > Or you could introduce CONFIG_BUG_GENERIC or else, so it is > > > easily > > > selectable by other architecture. > > > > > > > But still I am not sure that this is the best one option as at > > > > least we > > > > have different implementation for x86_64. > > > > > > My main concern is the maintainance effort. For every bug, we > > > would > > > need > > > to fix it in two places. The risk is we may forget to fix one > > > architecture. > > > This is not a very ideal situation. > > > > > > So I think sharing the header between RISC-V and Arm (or x86, see > > > below) > > > is at least a must. We can do the 3rd architecture at a leisure > > > pace. > > > > > > One option would be to introduce asm-generic like Linux (IIRC > > > this > > > was a > > > suggestion from Andrew). This would also to share code between > > > two of > > > the archs. > > > > > > Also, from a brief look, the difference in implementation is > > > mainly > > > because on Arm we can't use %c (some version of GCC didn't > > > support > > > it). > > > Is this also the case on RISC-V? If not, you may want to consider > > > to > > > use > > > the x86 version. > > > > > I did several experiments related to '%c' in inline assembly for > > RISC-V > > and it seems that '%c' doesn't support all forms of the use of > > '%c'. > > Thanks for checking! > > > I wrote the following macros and they have been compiled without > > any > > errors: > > ..... > > #define _ASM_BUGFRAME_TEXT(second_frame) \ > > ".Lbug%=: ebreak\n" \ > > ".pushsection .bug_frames.%c[bf_type], \"a\", @progbits\n" \ > > ".p2align 2\n" \ > > ".Lfrm%=:\n" \ > > ".long (.Lfrm%=)\n" \ > > ".long (0x55555555)\n" \ > > ".long (.Lbug%=)\n" \ > > ".long (0x55555555)\n" \ > > ".long %c[bf_line_hi]\n" \ > > ".long (0x55555555)\n" \ > > ".long %[bf_line_hi]\n" \ > > ".long (0x55555555)\n" \ > > ".long %[bf_line_lo]\n" \ > > ".long (0x55555555)\n" \ > > ".long %[bf_ptr]\n" \ > > ".long (0x55555555)\n" \ > > ".long (.Lbug%= - .Lfrm%=) + %c[bf_line_hi]\n" \ > > ".popsection\n" \ > > > > #define _ASM_BUGFRAME_INFO(type, line, ptr, msg) \ > > [bf_type] "i" (type), \ > > [bf_ptr] "i" (ptr), \ > > [bf_msg] "i" (msg), \ > > [bf_line_lo] "i" ((line & ((1 << BUG_LINE_LO_WIDTH) - 1)) \ > > << BUG_DISP_WIDTH), \ > > [bf_line_hi] "i" (((line) >> BUG_LINE_LO_WIDTH) << > > BUG_DISP_WIDTH) > > > > #define BUG_FRAME(type, line, ptr, second_frame, msg) do { \ > > __asm__ __volatile__ ( _ASM_BUGFRAME_TEXT(second_frame) \ > > :: _ASM_BUGFRAME_INFO(type, line, ptr, msg) ); > > \ > > } while (0) > > .... > > > > > > But if add ".long %c[bf_ptr]\n" then the following compilation > > error > > will occur: [ error: invalid 'asm': invalid use of '%c'. ] > > > > If you look at the dissembler of _bug_frames_... > > ...... > > 80201010: 1010 addi a2,sp,32 # > > .Lfrm%= > > 80201012: 8020 .2byte 0x8020 > > 80201014: 5555 li a0,-11 > > 80201016: 5555 li a0,-11 > > 80201018: 3022 .2byte 0x3022 # .Lbug%= > > 8020101a: 8020 .2byte 0x8020 > > 8020101c: 5555 li a0,-11 > > 8020101e: 5555 li a0,-11 > > 80201020: 0000 unimp # > > %c[bf_line_hi] > > 80201022: 0000 unimp > > 80201024: 5555 li a0,-11 > > 80201026: 5555 li a0,-11 > > 80201028: 0000 unimp # > > %[bf_line_hi] > > 8020102a: 0000 unimp > > 8020102c: 5555 li a0,-11 > > 8020102e: 5555 li a0,-11 > > 80201030: 0000 unimp # > > %[bf_line_lo] > > 80201032: 1600 addi s0,sp,800 > > 80201034: 5555 li a0,-11 > > 80201036: 5555 li a0,-11 > > 80201038: 10b8 addi a4,sp,104 # > > %[bf_ptr] > > 8020103a: 8020 .2byte 0x8020 > > 8020103c: 5555 li a0,-11 > > 8020103e: 5555 li a0,-11 > > 80201040: 2012 .2byte 0x2012 # > > (.Lbug%= - > > .Lfrm%=) + %c[bf_line_hi] > > ..... > > ... it looks like the error will be generated if the name in > > %c[name] > > isn't equal to 0. > > > > Another thing I noticed is that %[name] can be used instead of > > %c[name] > > for RISC-V ( i did a quick check and it works) so it is still > > possible > > to follow Intel implementation but required a redefinition of > > _ASM_BUGFRAME_TEXT where %c[...] won't be used. All the rest will > > be > > the same as in x86 implementation: > > ..... > > #define _ASM_BUGFRAME_TEXT(second_frame) \ > > ".Lbug%=: ebreak\n" \ > > ".pushsection .bug_frames.%[bf_type], \"a\", @progbits\n" \ > > ".p2align 2\n" \ > > ".Lfrm%=:\n" \ > > ".long (.Lbug%= - .Lfrm%=) + %[bf_line_hi]\n" \ > > ".long (%[bf_ptr] - .Lfrm%=) + %[bf_line_lo]\n" \ > > ".if " #second_frame "\n" \ > > ".long 0, %[bf_msg] - .Lfrm%=\n" \ > > ".endif\n" \ > > ".popsection\n" \ > > ..... > > > > One thing I would like to ask you is why it's better to follow/use > > x86 > > implementation instead of ARM? > > IMHO, the x86 version is cleaner. But... > > > It seems that "%c[...]" has the best support only for Intel GCC and > > thereby ARM implementation looks more universal, doesn't it? > > ... you are right, the Arm version is more portable. Although, I do > wonder how GCC managed to have a different behavior between > architecture > as I would have expected %c to be a generic implementation. > > Anyway, if you are basing on the Arm one, then you should be able to > 1) move arch/arm/include/asm/bug.h in asm-generic/bug.h (or > similar) > 2) Rename the guard and remove arm specific code.(I am not sure > from > where to include arm{32, 64}/bug.h) > 3) Define BUG_INSTR to ebreak on RISC-V. > 4) Find a place for all the RISC-V specific header > 5) Move do_bug_frame() in common/bug.c > > I am happy to help testing the Arm version and/or help moving the > code > to common. > Thanks a lot for the help offered. I've started to rework bug.h stuff but faced an issue. I am trying to introduce GENERIC_BUG_FRAME config ( only for ARM now as some stuff isn't available now for RISC-V such as find_text_region(), printk() and so on... Thereby I will switch to do_bug_frame() to generic one a little bit later for RISCV ) so I added the following to Kconfig: config GENERIC_DO_BUG_FRAME bool "Generic implementation of do_bug_frame()" default y if ARM default n help ... But when I pushed the commit to GitLab all ARM randconfig jobs failed because they decided not to set GENERIC_BUG_FRAME=y. Could you please give me a suggestion how I can work around this problem? ( I thought that it would be enough to use default y but randconfig can override it ). Or is it needed to provide an empty implementation for do_bug_frame() GENERIC_BUG_FRAME=n? Also I thought about weak function as an option... Here is pipeline for generic bug frame feature and the patch ( of course not ready for upstream but at least it shows an idea ): * https://gitlab.com/xen-project/people/olkur/xen/-/pipelines/766581174 * https://gitlab.com/xen-project/people/olkur/xen/-/commit/6fc6481202852e0aa2c2cb3877f2d71ac0213511 P.S.: Probably you have some comments ( something that is unacceptable even now ) about the patch. I will happy to hear them too. Thanks in advance. > Cheers, > ~ Oleksii
On 03/02/2023 13:15, Oleksii wrote: > Hi Julien, Hi Oleksii, > On Wed, 2023-02-01 at 22:11 +0000, Julien Grall wrote: > I am trying to introduce GENERIC_BUG_FRAME config ( only for ARM now as > some stuff isn't available now for RISC-V such as find_text_region(), > printk() and so on... Thereby I will switch to do_bug_frame() to > generic one a little bit later for RISCV ) so I added the following to > Kconfig: > > config GENERIC_DO_BUG_FRAME > bool "Generic implementation of do_bug_frame()" > default y if ARM > default n > help > ... > > But when I pushed the commit to GitLab all ARM randconfig jobs failed > because they decided not to set GENERIC_BUG_FRAME=y. > Could you please give me a suggestion how I can work around this > problem? ( I thought that it would be enough to use default y but > randconfig can override it ). You don't want to allow the user to deselect GENERIC_DO_BUG_FRAME. So you want config ARM to select it: (arch/arm/Kconfig) config ARM ... select GENERIC_DO_BUG_FRAME (common/Kconfig) config GENERIC_DO_BUG_FRAME bool > Or is it needed to provide an empty implementation for do_bug_frame() > GENERIC_BUG_FRAME=n? > Also I thought about weak function as an option... > > Here is pipeline for generic bug frame feature and the patch ( of > course not ready for upstream but at least it shows an idea ): > * > https://gitlab.com/xen-project/people/olkur/xen/-/pipelines/766581174 > * > https://gitlab.com/xen-project/people/olkur/xen/-/commit/6fc6481202852e0aa2c2cb3877f2d71ac0213511 > > P.S.: Probably you have some comments ( something that is unacceptable > even now ) about the patch. I will happy to hear them too. I will try to have a look next week. Cheers,
On Fri, 2023-02-03 at 13:23 +0000, Julien Grall wrote: > > > On 03/02/2023 13:15, Oleksii wrote: > > Hi Julien, > > Hi Oleksii, > > > On Wed, 2023-02-01 at 22:11 +0000, Julien Grall wrote: > > I am trying to introduce GENERIC_BUG_FRAME config ( only for ARM > > now as > > some stuff isn't available now for RISC-V such as > > find_text_region(), > > printk() and so on... Thereby I will switch to do_bug_frame() to > > generic one a little bit later for RISCV ) so I added the following > > to > > Kconfig: > > > > config GENERIC_DO_BUG_FRAME > > bool "Generic implementation of do_bug_frame()" > > default y if ARM > > default n > > help > > ... > > > > But when I pushed the commit to GitLab all ARM randconfig jobs > > failed > > because they decided not to set GENERIC_BUG_FRAME=y. > > Could you please give me a suggestion how I can work around this > > problem? ( I thought that it would be enough to use default y but > > randconfig can override it ). > > You don't want to allow the user to deselect GENERIC_DO_BUG_FRAME. So > you want config ARM to select it: > > (arch/arm/Kconfig) > config ARM > ... > select GENERIC_DO_BUG_FRAME > > > (common/Kconfig) > config GENERIC_DO_BUG_FRAME > bool > > > Or is it needed to provide an empty implementation for > > do_bug_frame() > > GENERIC_BUG_FRAME=n? > > Also I thought about weak function as an option... > > > > Here is pipeline for generic bug frame feature and the patch ( of > > course not ready for upstream but at least it shows an idea ): > > * > > https://gitlab.com/xen-project/people/olkur/xen/-/pipelines/766581174 > > * > > https://gitlab.com/xen-project/people/olkur/xen/-/commit/6fc6481202852e0aa2c2cb3877f2d71ac0213511 > > > > P.S.: Probably you have some comments ( something that is > > unacceptable > > even now ) about the patch. I will happy to hear them too. > > I will try to have a look next week. > Thanks a lot. I think that I'll send separate patch series with generic bug.h stuff today. > Cheers, > ~ Oleksii
diff --git a/xen/arch/riscv/include/asm/bug.h b/xen/arch/riscv/include/asm/bug.h new file mode 100644 index 0000000000..4b15d8eba6 --- /dev/null +++ b/xen/arch/riscv/include/asm/bug.h @@ -0,0 +1,118 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Copyright (C) 2012 Regents of the University of California + * Copyright (C) 2021-2023 Vates + * + */ + +#ifndef _ASM_RISCV_BUG_H +#define _ASM_RISCV_BUG_H + +#include <xen/stringify.h> +#include <xen/types.h> + +#ifndef __ASSEMBLY__ + +struct bug_frame { + signed int loc_disp; /* Relative address to the bug address */ + signed int file_disp; /* Relative address to the filename */ + signed int msg_disp; /* Relative address to the predicate (for ASSERT) */ + uint16_t line; /* Line number */ + uint32_t pad0:16; /* Padding for 8-bytes align */ +}; + +#define bug_loc(b) ((const void *)(b) + (b)->loc_disp) +#define bug_file(b) ((const void *)(b) + (b)->file_disp); +#define bug_line(b) ((b)->line) +#define bug_msg(b) ((const char *)(b) + (b)->msg_disp) + +#define BUGFRAME_run_fn 0 +#define BUGFRAME_warn 1 +#define BUGFRAME_bug 2 +#define BUGFRAME_assert 3 + +#define BUGFRAME_NR 4 + +#define INSN_LENGTH_MASK _UL(0x3) +#define INSN_LENGTH_32 _UL(0x3) + +#define BUG_INSN_32 _UL(0x00100073) /* ebreak */ +#define BUG_INSN_16 _UL(0x9002) /* c.ebreak */ +#define COMPRESSED_INSN_MASK _UL(0xffff) + +#define GET_INSN_LENGTH(insn) \ +({ \ + unsigned long len; \ + len = ((insn & INSN_LENGTH_MASK) == INSN_LENGTH_32) ? \ + 4UL : 2UL; \ + len; \ +}) + +/* These are defined by the architecture */ +int is_valid_bugaddr(uint32_t addr); + +#define BUG_FN_REG t0 + +/* Many versions of GCC doesn't support the asm %c parameter which would + * be preferable to this unpleasantness. We use mergeable string + * sections to avoid multiple copies of the string appearing in the + * Xen image. BUGFRAME_run_fn needs to be handled separately. + */ +#define BUG_FRAME(type, line, file, has_msg, msg) do { \ + asm ("1:ebreak\n" \ + ".pushsection .rodata.str, \"aMS\", %progbits, 1\n" \ + "2:\t.asciz " __stringify(file) "\n" \ + "3:\n" \ + ".if " #has_msg "\n" \ + "\t.asciz " #msg "\n" \ + ".endif\n" \ + ".popsection\n" \ + ".pushsection .bug_frames." __stringify(type) ", \"a\", %progbits\n"\ + "4:\n" \ + ".p2align 2\n" \ + ".long (1b - 4b)\n" \ + ".long (2b - 4b)\n" \ + ".long (3b - 4b)\n" \ + ".hword " __stringify(line) ", 0\n" \ + ".popsection"); \ +} while (0) + +/* + * GCC will not allow to use "i" when PIE is enabled (Xen doesn't set the + * flag but instead rely on the default value from the compiler). So the + * easiest way to implement run_in_exception_handler() is to pass the to + * be called function in a fixed register. + */ +#define run_in_exception_handler(fn) do { \ + register void *fn_ asm(__stringify(BUG_FN_REG)) = (fn); \ + asm ("1:ebreak\n" \ + ".pushsection .bug_frames." __stringify(BUGFRAME_run_fn) "," \ + " \"a\", %%progbits\n" \ + "2:\n" \ + ".p2align 2\n" \ + ".long (1b - 2b)\n" \ + ".long 0, 0, 0\n" \ + ".popsection" :: "r" (fn_) ); \ +} while (0) + +#define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, 0, "") + +#define BUG() do { \ + BUG_FRAME(BUGFRAME_bug, __LINE__, __FILE__, 0, ""); \ + unreachable(); \ +} while (0) + +#define assert_failed(msg) do { \ + BUG_FRAME(BUGFRAME_assert, __LINE__, __FILE__, 1, msg); \ + unreachable(); \ +} while (0) + +extern const struct bug_frame __start_bug_frames[], + __stop_bug_frames_0[], + __stop_bug_frames_1[], + __stop_bug_frames_2[], + __stop_bug_frames_3[]; + +#endif /* !__ASSEMBLY__ */ + +#endif /* _ASM_RISCV_BUG_H */ diff --git a/xen/arch/riscv/traps.c b/xen/arch/riscv/traps.c index 31ed63e3c1..0afb8e4e42 100644 --- a/xen/arch/riscv/traps.c +++ b/xen/arch/riscv/traps.c @@ -4,6 +4,7 @@ * * RISC-V Trap handlers */ +#include <asm/bug.h> #include <asm/csr.h> #include <asm/early_printk.h> #include <asm/processor.h> @@ -97,7 +98,134 @@ static void do_unexpected_trap(const struct cpu_user_regs *regs) die(); } +void show_execution_state(const struct cpu_user_regs *regs) +{ + early_printk("implement show_execution_state(regs)\n"); +} + +int do_bug_frame(struct cpu_user_regs *regs, vaddr_t pc) +{ + struct bug_frame *start, *end; + struct bug_frame *bug = NULL; + unsigned int id = 0; + const char *filename, *predicate; + int lineno; + + unsigned long bug_frames[] = { + (unsigned long)&__start_bug_frames[0], + (unsigned long)&__stop_bug_frames_0[0], + (unsigned long)&__stop_bug_frames_1[0], + (unsigned long)&__stop_bug_frames_2[0], + (unsigned long)&__stop_bug_frames_3[0], + }; + + for ( id = 0; id < BUGFRAME_NR; id++ ) + { + start = (struct bug_frame *)bug_frames[id]; + end = (struct bug_frame *)bug_frames[id + 1]; + + while ( start != end ) + { + if ( (vaddr_t)bug_loc(start) == pc ) + { + bug = start; + goto found; + } + + start++; + } + } + + found: + if ( bug == NULL ) + return -ENOENT; + + if ( id == BUGFRAME_run_fn ) + { + void (*fn)(const struct cpu_user_regs *) = (void *)regs->BUG_FN_REG; + + fn(regs); + + goto end; + } + + /* WARN, BUG or ASSERT: decode the filename pointer and line number. */ + filename = bug_file(bug); + lineno = bug_line(bug); + + switch ( id ) + { + case BUGFRAME_warn: + /* + * TODO: change early_printk's function to early_printk with format + * when s(n)printf() will be added. + */ + early_printk("Xen WARN at "); + early_printk(filename); + early_printk(":"); + // early_printk_hnum(lineno); + + show_execution_state(regs); + + goto end; + + case BUGFRAME_bug: + /* + * TODO: change early_printk's function to early_printk with format + * when s(n)printf() will be added. + */ + early_printk("Xen BUG at "); + early_printk(filename); + early_printk(":"); + // early_printk_hnum(lineno); + + show_execution_state(regs); + early_printk("change wait_for_interrupt to panic() when common is available\n"); + die(); + + case BUGFRAME_assert: + /* ASSERT: decode the predicate string pointer. */ + predicate = bug_msg(bug); + + /* + * TODO: change early_printk's function to early_printk with format + * when s(n)printf() will be added. + */ + early_printk("Assertion \'"); + early_printk(predicate); + early_printk("\' failed at "); + early_printk(filename); + early_printk(":"); + // early_printk_hnum(lineno); + + show_execution_state(regs); + early_printk("change wait_for_interrupt to panic() when common is available\n"); + die(); + } + + return -EINVAL; + + end: + regs->sepc += GET_INSN_LENGTH(*(uint32_t *)pc); + + return 0; +} + +int is_valid_bugaddr(uint32_t insn) +{ + if ((insn & INSN_LENGTH_MASK) == INSN_LENGTH_32) + return (insn == BUG_INSN_32); + else + return ((insn & COMPRESSED_INSN_MASK) == BUG_INSN_16); +} + void do_trap(struct cpu_user_regs *cpu_regs) { + register_t pc = cpu_regs->sepc; + uint32_t instr = *(uint32_t *)pc; + + if (is_valid_bugaddr(instr)) + if (!do_bug_frame(cpu_regs, pc)) return; + do_unexpected_trap(cpu_regs); } diff --git a/xen/arch/riscv/xen.lds.S b/xen/arch/riscv/xen.lds.S index ca57cce75c..139e2d04cb 100644 --- a/xen/arch/riscv/xen.lds.S +++ b/xen/arch/riscv/xen.lds.S @@ -39,6 +39,16 @@ SECTIONS . = ALIGN(PAGE_SIZE); .rodata : { _srodata = .; /* Read-only data */ + /* Bug frames table */ + __start_bug_frames = .; + *(.bug_frames.0) + __stop_bug_frames_0 = .; + *(.bug_frames.1) + __stop_bug_frames_1 = .; + *(.bug_frames.2) + __stop_bug_frames_2 = .; + *(.bug_frames.3) + __stop_bug_frames_3 = .; *(.rodata) *(.rodata.*) *(.data.rel.ro)
The patch introduces macros: BUG(), WARN(), run_in_exception(), assert_failed. The implementation uses "ebreak" instruction in combination with diffrent bug frame tables (for each type) which contains useful information. Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- Changes: - Remove __ in define namings - Update run_in_exception_handler() with register void *fn_ asm(__stringify(BUG_FN_REG)) = (fn); - Remove bug_instr_t type and change it's usage to uint32_t --- xen/arch/riscv/include/asm/bug.h | 118 ++++++++++++++++++++++++++++ xen/arch/riscv/traps.c | 128 +++++++++++++++++++++++++++++++ xen/arch/riscv/xen.lds.S | 10 +++ 3 files changed, 256 insertions(+) create mode 100644 xen/arch/riscv/include/asm/bug.h