| Message ID | 932b0fd2a211d2d837d00e1bda87a84ad680fe7b.1677233393.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | introduce generic implementation of macros from bug.h | expand |
Hi Oleksii, On 24/02/2023 11:31, Oleksii Kurochko wrote: > A large part of the content of the bug.h is repeated among all > architectures, so it was decided to create a new config > CONFIG_GENERIC_BUG_FRAME. > > The version of <bug.h> from x86 was taken as the base version. > > The patch introduces the following stuff: > * common bug.h header > * generic implementation of do_bug_frame > * new config CONFIG_GENERIC_BUG_FRAME > > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> > --- > Changes in V3: > * Add debugger_trap_fatal() to do_bug_frame(). It simplifies usage of > do_bug_frame() for x86 so making handle_bug_frame() and find_bug_frame() > not needed anymore. > * Update do_bug_frame() to return -EINVAL if something goes wrong; otherwise > id of bug_frame > * Update _ASM_BUGFRAME_TEXT to make it more portable. > * Drop unnecessary comments. > * define stub value for TRAP_invalid_op in case if wasn't defined in > arch-specific folders. > --- > Changes in V2: > - Switch to x86 implementation as generic as it is more compact > ( at least from the point of view of bug frame structure ). > - Rename CONFIG_GENERIC_DO_BUG_FRAME to CONFIG_GENERIC_BUG_FRAME. > - Change the macro bug_loc(b) to avoid the need for a cast: > #define bug_loc(b) ((unsigned long)(b) + (b)->loc_disp) > - Rename BUG_FRAME_STUFF to BUG_FRAME_STRUCT > - Make macros related to bug frame structure more generic. > - Introduce BUG_INSTR and MODIFIER to make _ASM_BUGFRAME_TEXT reusable > between x86 and RISC-V. > - Rework do_bug_frame() and introduce find_bug_frame() and handle_bug_frame() > functions to make it reusable by x86. > - code style fixes > --- > xen/common/Kconfig | 3 + > xen/common/Makefile | 1 + > xen/common/bug.c | 109 ++++++++++++++++++++++++++++++ > xen/include/xen/bug.h | 150 ++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 263 insertions(+) > create mode 100644 xen/common/bug.c > create mode 100644 xen/include/xen/bug.h > > diff --git a/xen/common/Kconfig b/xen/common/Kconfig > index f1ea3199c8..b226323537 100644 > --- a/xen/common/Kconfig > +++ b/xen/common/Kconfig > @@ -28,6 +28,9 @@ config ALTERNATIVE_CALL > config ARCH_MAP_DOMAIN_PAGE > bool > > +config GENERIC_BUG_FRAME > + bool > + > config HAS_ALTERNATIVE > bool > > diff --git a/xen/common/Makefile b/xen/common/Makefile > index bbd75b4be6..46049eac35 100644 > --- a/xen/common/Makefile > +++ b/xen/common/Makefile > @@ -1,5 +1,6 @@ > obj-$(CONFIG_ARGO) += argo.o > obj-y += bitmap.o > +obj-$(CONFIG_GENERIC_BUG_FRAME) += bug.o > obj-$(CONFIG_HYPFS_CONFIG) += config_data.o > obj-$(CONFIG_CORE_PARKING) += core_parking.o > obj-y += cpu.o > diff --git a/xen/common/bug.c b/xen/common/bug.c > new file mode 100644 > index 0000000000..f81724fc9b > --- /dev/null > +++ b/xen/common/bug.c > @@ -0,0 +1,109 @@ > +#include <xen/bug.h> > +#include <xen/debugger.h> > +#include <xen/errno.h> > +#include <xen/kernel.h> > +#include <xen/livepatch.h> > +#include <xen/string.h> > +#include <xen/types.h> > +#include <xen/virtual_region.h> > + > +#include <asm/processor.h> > + > +/* Set default value for TRAP_invalid_op as it is defined only for X86 now */ > +#ifndef TRAP_invalid_op > +#define TRAP_invalid_op 0 > +#endif It feels to me that this value should be defined in the else part in xen/debugger.h. > + > +int do_bug_frame(const struct cpu_user_regs *regs, unsigned long pc) I would suggest to document what this function is meant to return. AFAUI, it would return a negative value in case of an error otherwise the bug type. > +{ > + const struct bug_frame *bug = NULL; > + const struct virtual_region *region; > + const char *prefix = "", *filename, *predicate; > + unsigned long fixup; > + unsigned int id = BUGFRAME_NR, lineno; > + > + region = find_text_region(pc); > + if ( region ) NIT: If you invert the condition here, then you can reduce the indention by one below. > + { > + for ( id = 0; id < BUGFRAME_NR; id++ ) > + { > + const struct bug_frame *b; > + unsigned int i; You compare this against n_bugs which is a size_t. So, this wants to be a size_t. > + > + for ( i = 0, b = region->frame[id].bugs; > + i < region->frame[id].n_bugs; b++, i++ ) > + { > + if ( bug_loc(b) == pc ) > + { > + bug = b; > + goto found; > + } > + } > + } > + } > + > + found: > + if ( !bug ) > + return -EINVAL; > + > + if ( id == BUGFRAME_run_fn ) > + { > +#ifdef BUG_FN_REG > + void (*fn)(const struct cpu_user_regs *) = (void *)regs->BUG_FN_REG; AFAIU, this is necessary so Arm can use the generic do_bug_frame(). I was under the impression that RISC-V and Arm had the similar issue with %c. It seems like you managed to resolve it on RISC-V, so can we fully switch Arm to the generic implementation of bug? > +#else > + void (*fn)(const struct cpu_user_regs *) = bug_ptr(bug); > +#endif > + > + fn(regs); > + > + return id; > + } > + > + /* WARN, BUG or ASSERT: decode the filename pointer and line number. */ > + filename = bug_ptr(bug); > + if ( !is_kernel(filename) && !is_patch(filename) ) > + return -EINVAL; > + fixup = strlen(filename); > + if ( fixup > 50 ) > + { > + filename += fixup - 47; > + prefix = "..."; > + } > + lineno = bug_line(bug); > + > + switch ( id ) > + { > + case BUGFRAME_warn: > + printk("Xen WARN at %s%s:%d\n", prefix, filename, lineno); > + show_execution_state(regs); > + > + return id; > + > + case BUGFRAME_bug: > + printk("Xen BUG at %s%s:%d\n", prefix, filename, lineno); > + > + if ( debugger_trap_fatal(TRAP_invalid_op, regs) ) > + return id; > + > + show_execution_state(regs); > + panic("Xen BUG at %s%s:%d\n", prefix, filename, lineno); > + > + case BUGFRAME_assert: > + /* ASSERT: decode the predicate string pointer. */ > + predicate = bug_msg(bug); > + if ( !is_kernel(predicate) && !is_patch(predicate) ) > + predicate = "<unknown>"; > + > + printk("Assertion '%s' failed at %s%s:%d\n", > + predicate, prefix, filename, lineno); > + > + if ( debugger_trap_fatal(TRAP_invalid_op, regs) ) > + return id; > + > + show_execution_state(regs); > + panic("Assertion '%s' failed at %s%s:%d\n", > + predicate, prefix, filename, lineno); > + } > + > + return id; > +} > diff --git a/xen/include/xen/bug.h b/xen/include/xen/bug.h > new file mode 100644 > index 0000000000..4b18cfa69c > --- /dev/null > +++ b/xen/include/xen/bug.h > @@ -0,0 +1,150 @@ > +#ifndef __XEN_BUG_H__ > +#define __XEN_BUG_H__ > + > +#define BUG_DISP_WIDTH 24 > +#define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH) > +#define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH) > + > +#define BUGFRAME_run_fn 0 > +#define BUGFRAME_warn 1 > +#define BUGFRAME_bug 2 > +#define BUGFRAME_assert 3 > + > +#define BUGFRAME_NR 4 > + > +#include <asm/bug.h> > + > +#ifndef __ASSEMBLY__ > + > +#include <xen/errno.h> errno.h doesn't look to be used within this here. So is it necessary to import it? > +#include <xen/lib.h> Why is this necessary to include in the header? > +#include <xen/stringify.h> You don't seem to use __stringify in this header. So is this necessary? > + > +#ifndef BUG_FRAME_STRUCT > + > +struct bug_frame { > + signed int loc_disp:BUG_DISP_WIDTH; > + unsigned int line_hi:BUG_LINE_HI_WIDTH; > + signed int ptr_disp:BUG_DISP_WIDTH; > + unsigned int line_lo:BUG_LINE_LO_WIDTH; > + signed int msg_disp[]; > +}; > + > +#endif /* BUG_FRAME_STRUCT */ > + > +#ifndef bug_loc > +#define bug_loc(b) ((unsigned long)(b) + (b)->loc_disp) > +#endif > + > +#ifndef bug_ptr > +#define bug_ptr(b) ((const void *)(b) + (b)->ptr_disp) > +#endif > + > +#ifndef bug_line > +#define bug_line(b) (((((b)->line_hi + ((b)->loc_disp < 0)) & \ > + ((1 << BUG_LINE_HI_WIDTH) - 1)) << \ > + BUG_LINE_LO_WIDTH) + \ > + (((b)->line_lo + ((b)->ptr_disp < 0)) & \ > + ((1 << BUG_LINE_LO_WIDTH) - 1))) > +#endif > + > +#ifndef bug_msg > +#define bug_msg(b) ((const char *)(b) + (b)->msg_disp[1]) > +#endif For all the macro above, it feels wrong to me to allow an architecture to override them if the default BUG_FRAME_STRUCT. It would also feels wrong to me that if the default BUG_FRAME_STRUCT is not used to still partially rely on the generic version of the helper.\ So I would suggest to move them in the #ifndef BUG_FRAME_STRUCT and drop the #ifndef <helper>. > + > +#ifndef BUG_ASM_CONST > +#define BUG_ASM_CONST "" > +#endif This line is a bit misterious to me. Would you be able to outline why an architecture would override this? > + > +#if !defined(_ASM_BUGFRAME_TEXT) || !defined(_ASM_BUGFRAME_INFO) > + > +#define _ASM_BUGFRAME_TEXT(second_frame) \ > + ".Lbug%=:"BUG_INSTR"\n" \ > + " .pushsection .bug_frames.%"BUG_ASM_CONST"[bf_type], \"a\", %%progbits\n" \ > + " .p2align 2\n" \ > + ".Lfrm%=:\n" \ > + " .long (.Lbug%= - .Lfrm%=) + %"BUG_ASM_CONST"[bf_line_hi]\n" \ > + " .long (%"BUG_ASM_CONST"[bf_ptr] - .Lfrm%=) + %"BUG_ASM_CONST"[bf_line_lo]\n"\ > + " .if " #second_frame "\n" \ > + " .long 0, %"BUG_ASM_CONST"[bf_msg] - .Lfrm%=\n" \ > + " .endif\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) > + > +#endif /* _ASM_BUGFRAME_TEXT || _ASM_BUGFRAME_INFO */ > + > +#ifndef BUG_FRAME > + > +#define BUG_FRAME(type, line, ptr, second_frame, msg) do { \ > + BUILD_BUG_ON((line) >> (BUG_LINE_LO_WIDTH + BUG_LINE_HI_WIDTH)); \ > + BUILD_BUG_ON((type) >= BUGFRAME_NR); \ > + asm volatile ( _ASM_BUGFRAME_TEXT(second_frame) \ > + :: _ASM_BUGFRAME_INFO(type, line, ptr, msg) ); \ > +} while (0) > + > +#endif > + > +#ifndef run_in_exception_handler > + > +/* > + * TODO: untangle header dependences, break BUILD_BUG_ON() out of xen/lib.h, > + * and use a real static inline here to get proper type checking of fn(). > + */ > +#define run_in_exception_handler(fn) \ > + do { \ > + (void)((fn) == (void (*)(struct cpu_user_regs *))NULL); \ > + BUG_FRAME(BUGFRAME_run_fn, 0, fn, 0, NULL); \ > + } while ( 0 ) > + > +#endif /* run_in_exception_handler */ > + > +#ifndef WARN > +#define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, 0, NULL) > +#endif > + > +#ifndef BUG > +#define BUG() do { \ > + BUG_FRAME(BUGFRAME_bug, __LINE__, __FILE__, 0, NULL); \ > + unreachable(); \ > +} while (0) > +#endif > + > +#ifndef assert_failed > +#define assert_failed(msg) do { \ > + BUG_FRAME(BUGFRAME_assert, __LINE__, __FILE__, 1, msg); \ > + unreachable(); \ > +} while (0) > +#endif > + > +#ifdef CONFIG_GENERIC_BUG_FRAME > + > +struct cpu_user_regs; > + > +int do_bug_frame(const struct cpu_user_regs *regs, unsigned long pc); > + > +#endif /* CONFIG_GENERIC_BUG_FRAME */ > + > +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 /* __XEN_BUG_H__ */ > +/* > + * Local variables: > + * mode: C > + * c-file-style: "BSD" > + * c-basic-offset: 4 > + * indent-tabs-mode: nil > + * End: > + */ Cheers,
On 25.02.2023 17:42, Julien Grall wrote: > On 24/02/2023 11:31, Oleksii Kurochko wrote: >> --- /dev/null >> +++ b/xen/common/bug.c >> @@ -0,0 +1,109 @@ >> +#include <xen/bug.h> >> +#include <xen/debugger.h> >> +#include <xen/errno.h> >> +#include <xen/kernel.h> >> +#include <xen/livepatch.h> > +#include <xen/string.h> >> +#include <xen/types.h> >> +#include <xen/virtual_region.h> >> + >> +#include <asm/processor.h> >> + >> +/* Set default value for TRAP_invalid_op as it is defined only for X86 now */ >> +#ifndef TRAP_invalid_op >> +#define TRAP_invalid_op 0 >> +#endif > > It feels to me that this value should be defined in the else part in > xen/debugger.h. I guess with [1] it won't be as straightforward anymore ... Jan [1] https://lists.xen.org/archives/html/xen-devel/2023-02/msg01026.html
On 24.02.2023 12:31, Oleksii Kurochko wrote: > --- /dev/null > +++ b/xen/common/bug.c > @@ -0,0 +1,109 @@ > +#include <xen/bug.h> > +#include <xen/debugger.h> > +#include <xen/errno.h> > +#include <xen/kernel.h> > +#include <xen/livepatch.h> > +#include <xen/string.h> > +#include <xen/types.h> > +#include <xen/virtual_region.h> > + > +#include <asm/processor.h> > + > +/* Set default value for TRAP_invalid_op as it is defined only for X86 now */ > +#ifndef TRAP_invalid_op > +#define TRAP_invalid_op 0 > +#endif > + > +int do_bug_frame(const struct cpu_user_regs *regs, unsigned long pc) > +{ > + const struct bug_frame *bug = NULL; > + const struct virtual_region *region; > + const char *prefix = "", *filename, *predicate; > + unsigned long fixup; > + unsigned int id = BUGFRAME_NR, lineno; > + > + region = find_text_region(pc); > + if ( region ) > + { > + for ( id = 0; id < BUGFRAME_NR; id++ ) > + { > + const struct bug_frame *b; > + unsigned int i; > + > + for ( i = 0, b = region->frame[id].bugs; > + i < region->frame[id].n_bugs; b++, i++ ) > + { > + if ( bug_loc(b) == pc ) > + { > + bug = b; > + goto found; > + } > + } > + } > + } > + > + found: > + if ( !bug ) > + return -EINVAL; > + > + if ( id == BUGFRAME_run_fn ) > + { > +#ifdef BUG_FN_REG > + void (*fn)(const struct cpu_user_regs *) = (void *)regs->BUG_FN_REG; > +#else > + void (*fn)(const struct cpu_user_regs *) = bug_ptr(bug); > +#endif > + > + fn(regs); > + > + return id; > + } > + > + /* WARN, BUG or ASSERT: decode the filename pointer and line number. */ > + filename = bug_ptr(bug); > + if ( !is_kernel(filename) && !is_patch(filename) ) > + return -EINVAL; > + fixup = strlen(filename); > + if ( fixup > 50 ) > + { > + filename += fixup - 47; > + prefix = "..."; > + } > + lineno = bug_line(bug); > + > + switch ( id ) > + { > + case BUGFRAME_warn: > + printk("Xen WARN at %s%s:%d\n", prefix, filename, lineno); > + show_execution_state(regs); > + > + return id; > + > + case BUGFRAME_bug: > + printk("Xen BUG at %s%s:%d\n", prefix, filename, lineno); > + > + if ( debugger_trap_fatal(TRAP_invalid_op, regs) ) TRAP_invalid_op is, as said, about to disappear on x86 as well. I think this construct wants abstracting by another asm/bug.h provided macro (taking just regs). Jan
On Mon, 2023-02-27 at 15:23 +0100, Jan Beulich wrote: > On 24.02.2023 12:31, Oleksii Kurochko wrote: > > --- /dev/null > > +++ b/xen/common/bug.c > > @@ -0,0 +1,109 @@ > > +#include <xen/bug.h> > > +#include <xen/debugger.h> > > +#include <xen/errno.h> > > +#include <xen/kernel.h> > > +#include <xen/livepatch.h> > > +#include <xen/string.h> > > +#include <xen/types.h> > > +#include <xen/virtual_region.h> > > + > > +#include <asm/processor.h> > > + > > +/* Set default value for TRAP_invalid_op as it is defined only for > > X86 now */ > > +#ifndef TRAP_invalid_op > > +#define TRAP_invalid_op 0 > > +#endif > > + > > +int do_bug_frame(const struct cpu_user_regs *regs, unsigned long > > pc) > > +{ > > + const struct bug_frame *bug = NULL; > > + const struct virtual_region *region; > > + const char *prefix = "", *filename, *predicate; > > + unsigned long fixup; > > + unsigned int id = BUGFRAME_NR, lineno; > > + > > + region = find_text_region(pc); > > + if ( region ) > > + { > > + for ( id = 0; id < BUGFRAME_NR; id++ ) > > + { > > + const struct bug_frame *b; > > + unsigned int i; > > + > > + for ( i = 0, b = region->frame[id].bugs; > > + i < region->frame[id].n_bugs; b++, i++ ) > > + { > > + if ( bug_loc(b) == pc ) > > + { > > + bug = b; > > + goto found; > > + } > > + } > > + } > > + } > > + > > + found: > > + if ( !bug ) > > + return -EINVAL; > > + > > + if ( id == BUGFRAME_run_fn ) > > + { > > +#ifdef BUG_FN_REG > > + void (*fn)(const struct cpu_user_regs *) = (void *)regs- > > >BUG_FN_REG; > > +#else > > + void (*fn)(const struct cpu_user_regs *) = bug_ptr(bug); > > +#endif > > + > > + fn(regs); > > + > > + return id; > > + } > > + > > + /* WARN, BUG or ASSERT: decode the filename pointer and line > > number. */ > > + filename = bug_ptr(bug); > > + if ( !is_kernel(filename) && !is_patch(filename) ) > > + return -EINVAL; > > + fixup = strlen(filename); > > + if ( fixup > 50 ) > > + { > > + filename += fixup - 47; > > + prefix = "..."; > > + } > > + lineno = bug_line(bug); > > + > > + switch ( id ) > > + { > > + case BUGFRAME_warn: > > + printk("Xen WARN at %s%s:%d\n", prefix, filename, lineno); > > + show_execution_state(regs); > > + > > + return id; > > + > > + case BUGFRAME_bug: > > + printk("Xen BUG at %s%s:%d\n", prefix, filename, lineno); > > + > > + if ( debugger_trap_fatal(TRAP_invalid_op, regs) ) > > TRAP_invalid_op is, as said, about to disappear on x86 as well. I > think > this construct wants abstracting by another asm/bug.h provided macro > (taking just regs). > Thanks for the link. Nice idea to abstract 'debugger_trap_fatal(TRAP_invalid_op, regs)'. Actually we have to options here: 1. As you proposed abstract in <asm/bug.h>: x86: #define DEBUG_TRAP_FATAL(regs) debugger_trap_fatal(X86_EXC_GP, regs) ARM: #define DEBUG_TRAP_FATAL(regs) 0 RISC-V: #define DEBUG_TRAP_FATAL(regs) 0 For ARM and RISC-V it doesn't use so we can skip the check if ( DEBUG_TRAP_FATAL ). 2. Abstract only TRAP_invalid_op in <asm/bug.h> x86: #define TRAP_invalud_op X86_EXC_GP RISC-V: #define TRAP_invalid_op 0 ARN: #define TRAP_invalid_op 0 I am not sure if we have to provide real invalid opcodes for RISC-V and ARM as it looks like debug_trap_fatal() isn't used in ARM&RISC-V now. Could you please suggest which one option is better? ~ Oleksii > Jan
On 28.02.2023 11:30, Oleksii wrote: > On Mon, 2023-02-27 at 15:23 +0100, Jan Beulich wrote: >> On 24.02.2023 12:31, Oleksii Kurochko wrote: >>> --- /dev/null >>> +++ b/xen/common/bug.c >>> @@ -0,0 +1,109 @@ >>> +#include <xen/bug.h> >>> +#include <xen/debugger.h> >>> +#include <xen/errno.h> >>> +#include <xen/kernel.h> >>> +#include <xen/livepatch.h> >>> +#include <xen/string.h> >>> +#include <xen/types.h> >>> +#include <xen/virtual_region.h> >>> + >>> +#include <asm/processor.h> >>> + >>> +/* Set default value for TRAP_invalid_op as it is defined only for >>> X86 now */ >>> +#ifndef TRAP_invalid_op >>> +#define TRAP_invalid_op 0 >>> +#endif >>> + >>> +int do_bug_frame(const struct cpu_user_regs *regs, unsigned long >>> pc) >>> +{ >>> + const struct bug_frame *bug = NULL; >>> + const struct virtual_region *region; >>> + const char *prefix = "", *filename, *predicate; >>> + unsigned long fixup; >>> + unsigned int id = BUGFRAME_NR, lineno; >>> + >>> + region = find_text_region(pc); >>> + if ( region ) >>> + { >>> + for ( id = 0; id < BUGFRAME_NR; id++ ) >>> + { >>> + const struct bug_frame *b; >>> + unsigned int i; >>> + >>> + for ( i = 0, b = region->frame[id].bugs; >>> + i < region->frame[id].n_bugs; b++, i++ ) >>> + { >>> + if ( bug_loc(b) == pc ) >>> + { >>> + bug = b; >>> + goto found; >>> + } >>> + } >>> + } >>> + } >>> + >>> + found: >>> + if ( !bug ) >>> + return -EINVAL; >>> + >>> + if ( id == BUGFRAME_run_fn ) >>> + { >>> +#ifdef BUG_FN_REG >>> + void (*fn)(const struct cpu_user_regs *) = (void *)regs- >>>> BUG_FN_REG; >>> +#else >>> + void (*fn)(const struct cpu_user_regs *) = bug_ptr(bug); >>> +#endif >>> + >>> + fn(regs); >>> + >>> + return id; >>> + } >>> + >>> + /* WARN, BUG or ASSERT: decode the filename pointer and line >>> number. */ >>> + filename = bug_ptr(bug); >>> + if ( !is_kernel(filename) && !is_patch(filename) ) >>> + return -EINVAL; >>> + fixup = strlen(filename); >>> + if ( fixup > 50 ) >>> + { >>> + filename += fixup - 47; >>> + prefix = "..."; >>> + } >>> + lineno = bug_line(bug); >>> + >>> + switch ( id ) >>> + { >>> + case BUGFRAME_warn: >>> + printk("Xen WARN at %s%s:%d\n", prefix, filename, lineno); >>> + show_execution_state(regs); >>> + >>> + return id; >>> + >>> + case BUGFRAME_bug: >>> + printk("Xen BUG at %s%s:%d\n", prefix, filename, lineno); >>> + >>> + if ( debugger_trap_fatal(TRAP_invalid_op, regs) ) >> >> TRAP_invalid_op is, as said, about to disappear on x86 as well. I >> think >> this construct wants abstracting by another asm/bug.h provided macro >> (taking just regs). >> > Thanks for the link. > > Nice idea to abstract 'debugger_trap_fatal(TRAP_invalid_op, regs)'. > Actually we have to options here: > 1. As you proposed abstract in <asm/bug.h>: > x86: #define DEBUG_TRAP_FATAL(regs) debugger_trap_fatal(X86_EXC_GP, > regs) > ARM: #define DEBUG_TRAP_FATAL(regs) 0 > RISC-V: #define DEBUG_TRAP_FATAL(regs) 0 > For ARM and RISC-V it doesn't use so we can skip the check if ( > DEBUG_TRAP_FATAL ). > > 2. Abstract only TRAP_invalid_op in <asm/bug.h> > x86: #define TRAP_invalud_op X86_EXC_GP > RISC-V: #define TRAP_invalid_op 0 > ARN: #define TRAP_invalid_op 0 > > I am not sure if we have to provide real invalid opcodes for RISC-V > and ARM as it looks like debug_trap_fatal() isn't used in ARM&RISC-V > now. > > Could you please suggest which one option is better? I don't view 2 as a viable option. How an arch deals with invalid opcodes is entirely arch-specific (including the naming). As to 1 - since we want this solely for bug.c, I'd prefer if the wrapper macro's name would start with BUG_, e.g. BUG_DEBUGGER_TRAP_FATAL() or BUG_TRAP_FATAL() or just BUG_FATAL(). Further adding ARCH_ may also be wanted by other maintainers (I'm neither pro nor con there). Jan
Hi Julien, On Sat, 2023-02-25 at 16:42 +0000, Julien Grall wrote: > Hi Oleksii, > > On 24/02/2023 11:31, Oleksii Kurochko wrote: > > A large part of the content of the bug.h is repeated among all > > architectures, so it was decided to create a new config > > CONFIG_GENERIC_BUG_FRAME. > > > > The version of <bug.h> from x86 was taken as the base version. > > > > The patch introduces the following stuff: > > * common bug.h header > > * generic implementation of do_bug_frame > > * new config CONFIG_GENERIC_BUG_FRAME > > > > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> > > --- > > Changes in V3: > > * Add debugger_trap_fatal() to do_bug_frame(). It simplifies > > usage of > > do_bug_frame() for x86 so making handle_bug_frame() and > > find_bug_frame() > > not needed anymore. > > * Update do_bug_frame() to return -EINVAL if something goes > > wrong; otherwise > > id of bug_frame > > * Update _ASM_BUGFRAME_TEXT to make it more portable. > > * Drop unnecessary comments. > > * define stub value for TRAP_invalid_op in case if wasn't defined > > in > > arch-specific folders. > > --- > > Changes in V2: > > - Switch to x86 implementation as generic as it is more compact > > ( at least from the point of view of bug frame structure ). > > - Rename CONFIG_GENERIC_DO_BUG_FRAME to > > CONFIG_GENERIC_BUG_FRAME. > > - Change the macro bug_loc(b) to avoid the need for a cast: > > #define bug_loc(b) ((unsigned long)(b) + (b)->loc_disp) > > - Rename BUG_FRAME_STUFF to BUG_FRAME_STRUCT > > - Make macros related to bug frame structure more generic. > > - Introduce BUG_INSTR and MODIFIER to make _ASM_BUGFRAME_TEXT > > reusable > > between x86 and RISC-V. > > - Rework do_bug_frame() and introduce find_bug_frame() and > > handle_bug_frame() > > functions to make it reusable by x86. > > - code style fixes > > --- > > xen/common/Kconfig | 3 + > > xen/common/Makefile | 1 + > > xen/common/bug.c | 109 ++++++++++++++++++++++++++++++ > > xen/include/xen/bug.h | 150 > > ++++++++++++++++++++++++++++++++++++++++++ > > 4 files changed, 263 insertions(+) > > create mode 100644 xen/common/bug.c > > create mode 100644 xen/include/xen/bug.h > > > > diff --git a/xen/common/Kconfig b/xen/common/Kconfig > > index f1ea3199c8..b226323537 100644 > > --- a/xen/common/Kconfig > > +++ b/xen/common/Kconfig > > @@ -28,6 +28,9 @@ config ALTERNATIVE_CALL > > config ARCH_MAP_DOMAIN_PAGE > > bool > > > > +config GENERIC_BUG_FRAME > > + bool > > + > > config HAS_ALTERNATIVE > > bool > > > > diff --git a/xen/common/Makefile b/xen/common/Makefile > > index bbd75b4be6..46049eac35 100644 > > --- a/xen/common/Makefile > > +++ b/xen/common/Makefile > > @@ -1,5 +1,6 @@ > > obj-$(CONFIG_ARGO) += argo.o > > obj-y += bitmap.o > > +obj-$(CONFIG_GENERIC_BUG_FRAME) += bug.o > > obj-$(CONFIG_HYPFS_CONFIG) += config_data.o > > obj-$(CONFIG_CORE_PARKING) += core_parking.o > > obj-y += cpu.o > > diff --git a/xen/common/bug.c b/xen/common/bug.c > > new file mode 100644 > > index 0000000000..f81724fc9b > > --- /dev/null > > +++ b/xen/common/bug.c > > @@ -0,0 +1,109 @@ > > +#include <xen/bug.h> > > +#include <xen/debugger.h> > > +#include <xen/errno.h> > > +#include <xen/kernel.h> > > +#include <xen/livepatch.h> > +#include <xen/string.h> > > +#include <xen/types.h> > > +#include <xen/virtual_region.h> > > + > > +#include <asm/processor.h> > > + > > +/* Set default value for TRAP_invalid_op as it is defined only for > > X86 now */ > > +#ifndef TRAP_invalid_op > > +#define TRAP_invalid_op 0 > > +#endif > > It feels to me that this value should be defined in the else part in > xen/debugger.h. As it was disscussed in the other e-mail [1] the following will be introduced in <xen/bug.h> #ifndef BUG_DEBUGGER_TRAP_FATAL #define BUG_DEBUGGER_TRAP_FATAL(regs) 0 #endif and re-defined in <asm/bug.h>. Of course, do_bug_frame() will be updated correspondingly to use BUG_DEBUGGER_TRAP_FATAL. [1] https://lore.kernel.org/xen-devel/9b66ee51-17c3-0f8e-0fc2-4ff083952e9d@suse.com/ > > > + > > +int do_bug_frame(const struct cpu_user_regs *regs, unsigned long > > pc) > > I would suggest to document what this function is meant to return. > AFAUI, it would return a negative value in case of an error otherwise > the bug type. sure. i'll added in the next patch version. > > > +{ > > + const struct bug_frame *bug = NULL; > > + const struct virtual_region *region; > > + const char *prefix = "", *filename, *predicate; > > + unsigned long fixup; > > + unsigned int id = BUGFRAME_NR, lineno; > > + > > + region = find_text_region(pc); > > + if ( region ) > > NIT: If you invert the condition here, then you can reduce the > indention > by one below. Thanks. i'll added in the next patch version. > > > + { > > + for ( id = 0; id < BUGFRAME_NR; id++ ) > > + { > > + const struct bug_frame *b; > > + unsigned int i; > > You compare this against n_bugs which is a size_t. So, this wants to > be > a size_t. This one will be updated too. Thanks. > > > + > > + for ( i = 0, b = region->frame[id].bugs; > > + i < region->frame[id].n_bugs; b++, i++ ) > > + { > > + if ( bug_loc(b) == pc ) > > + { > > + bug = b; > > + goto found; > > + } > > + } > > + } > > + } > > + > > + found: > > + if ( !bug ) > > + return -EINVAL; > > + > > + if ( id == BUGFRAME_run_fn ) > > + { > > +#ifdef BUG_FN_REG > > + void (*fn)(const struct cpu_user_regs *) = (void *)regs- > > >BUG_FN_REG; > > AFAIU, this is necessary so Arm can use the generic do_bug_frame(). > > I was under the impression that RISC-V and Arm had the similar issue > with %c. It seems like you managed to resolve it on RISC-V, so can we > fully switch Arm to the generic implementation of bug? I tried to switch ARM to generic implementation. Here is the patch: [1] diff --git a/xen/arch/arm/include/asm/bug.h b/xen/arch/arm/include/asm/bug.h index e6cc37e1d6..ffb0f569fc 100644 --- a/xen/arch/arm/include/asm/bug.h +++ b/xen/arch/arm/include/asm/bug.h @@ -1,8 +1,6 @@ #ifndef __ARM_BUG_H__ #define __ARM_BUG_H__ -#include <xen/types.h> - #if defined(CONFIG_ARM_32) # include <asm/arm32/bug.h> #elif defined(CONFIG_ARM_64) @@ -11,63 +9,7 @@ # error "unknown ARM variant" #endif -#define BUG_FRAME_STRUCT - -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_ptr(b) ((const void *)(b) + (b)->file_disp) -#define bug_line(b) ((b)->line) -#define bug_msg(b) ((const char *)(b) + (b)->msg_disp) - -/* 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 { \ - BUILD_BUG_ON((line) >> 16); \ - BUILD_BUG_ON((type) >= BUGFRAME_NR); \ - asm ("1:"BUG_INSTR"\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 { \ - asm ("mov " __stringify(BUG_FN_REG) ", %0\n" \ - "1:"BUG_INSTR"\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) : __stringify(BUG_FN_REG) ); \ -} while (0) +#define BUG_ASM_CONST "c" #endif /* __ARM_BUG_H__ */ ... (it will be merged with patch 3 if it is OK ) And looks like we can switch ARM to generic implementation as all tests passed: https://gitlab.com/xen-project/people/olkur/xen/-/pipelines/791549396 The only issue is with yocto-arm: https://gitlab.com/xen-project/people/olkur/xen/-/pipelines/791549396/failures But I am not sure that it is because of my patch Is this enough from a verification point of view? [1] https://gitlab.com/xen-project/people/olkur/xen/-/commit/5ff7a06e1d354e1e42bde1c203f3cf05a3653ad6https://gitlab.com/xen-project/people/olkur/xen/-/commit/5ff7a06e1d354e1e42bde1c203f3cf05a3653ad6 > > > +#else > > + void (*fn)(const struct cpu_user_regs *) = bug_ptr(bug); > > +#endif > > + > > + fn(regs); > > + > > + return id; > > + } > > + > > + /* WARN, BUG or ASSERT: decode the filename pointer and line > > number. */ > > + filename = bug_ptr(bug); > > + if ( !is_kernel(filename) && !is_patch(filename) ) > > + return -EINVAL; > > + fixup = strlen(filename); > > + if ( fixup > 50 ) > > + { > > + filename += fixup - 47; > > + prefix = "..."; > > + } > > + lineno = bug_line(bug); > > + > > + switch ( id ) > > + { > > + case BUGFRAME_warn: > > + printk("Xen WARN at %s%s:%d\n", prefix, filename, lineno); > > + show_execution_state(regs); > > + > > + return id; > > + > > + case BUGFRAME_bug: > > + printk("Xen BUG at %s%s:%d\n", prefix, filename, lineno); > > + > > + if ( debugger_trap_fatal(TRAP_invalid_op, regs) ) > > + return id; > > + > > + show_execution_state(regs); > > + panic("Xen BUG at %s%s:%d\n", prefix, filename, lineno); > > + > > + case BUGFRAME_assert: > > + /* ASSERT: decode the predicate string pointer. */ > > + predicate = bug_msg(bug); > > + if ( !is_kernel(predicate) && !is_patch(predicate) ) > > + predicate = "<unknown>"; > > + > > + printk("Assertion '%s' failed at %s%s:%d\n", > > + predicate, prefix, filename, lineno); > > + > > + if ( debugger_trap_fatal(TRAP_invalid_op, regs) ) > > + return id; > > + > > + show_execution_state(regs); > > + panic("Assertion '%s' failed at %s%s:%d\n", > > + predicate, prefix, filename, lineno); > > + } > > + > > + return id; > > +} > > diff --git a/xen/include/xen/bug.h b/xen/include/xen/bug.h > > new file mode 100644 > > index 0000000000..4b18cfa69c > > --- /dev/null > > +++ b/xen/include/xen/bug.h > > @@ -0,0 +1,150 @@ > > +#ifndef __XEN_BUG_H__ > > +#define __XEN_BUG_H__ > > + > > +#define BUG_DISP_WIDTH 24 > > +#define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH) > > +#define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH) > > + > > +#define BUGFRAME_run_fn 0 > > +#define BUGFRAME_warn 1 > > +#define BUGFRAME_bug 2 > > +#define BUGFRAME_assert 3 > > + > > +#define BUGFRAME_NR 4 > > + > > +#include <asm/bug.h> > > + > > +#ifndef __ASSEMBLY__ > > + > > +#include <xen/errno.h> > > errno.h doesn't look to be used within this here. So is it necessary > to > import it? > > > +#include <xen/lib.h> > > Why is this necessary to include in the header? > > > +#include <xen/stringify.h> > > You don't seem to use __stringify in this header. So is this > necessary? The mentioned headers will be removed. They was needed when I tried to use ARM implementation as generic one. > > > + > > +#ifndef BUG_FRAME_STRUCT > > + > > +struct bug_frame { > > + signed int loc_disp:BUG_DISP_WIDTH; > > + unsigned int line_hi:BUG_LINE_HI_WIDTH; > > + signed int ptr_disp:BUG_DISP_WIDTH; > > + unsigned int line_lo:BUG_LINE_LO_WIDTH; > > + signed int msg_disp[]; > > +}; > > + > > +#endif /* BUG_FRAME_STRUCT */ > > + > > +#ifndef bug_loc > > +#define bug_loc(b) ((unsigned long)(b) + (b)->loc_disp) > > +#endif > > + > > +#ifndef bug_ptr > > +#define bug_ptr(b) ((const void *)(b) + (b)->ptr_disp) > > +#endif > > + > > +#ifndef bug_line > > +#define bug_line(b) (((((b)->line_hi + ((b)->loc_disp < 0)) > > & \ > > + ((1 << BUG_LINE_HI_WIDTH) - 1)) > > << \ > > + BUG_LINE_LO_WIDTH) > > + \ > > + (((b)->line_lo + ((b)->ptr_disp < 0)) > > & \ > > + ((1 << BUG_LINE_LO_WIDTH) - 1))) > > +#endif > > + > > +#ifndef bug_msg > > +#define bug_msg(b) ((const char *)(b) + (b)->msg_disp[1]) > > +#endif > > For all the macro above, it feels wrong to me to allow an > architecture > to override them if the default BUG_FRAME_STRUCT. > > It would also feels wrong to me that if the default BUG_FRAME_STRUCT > is > not used to still partially rely on the generic version of the > helper.\ > > So I would suggest to move them in the #ifndef BUG_FRAME_STRUCT and > drop > the #ifndef <helper>. Agree. I'll do that in the next version of the patch. Thanks. > > > + > > +#ifndef BUG_ASM_CONST > > +#define BUG_ASM_CONST "" > > +#endif > > This line is a bit misterious to me. Would you be able to outline why > an > architecture would override this? It is needed in case if compiler for an architecture doesn't have proper support of '%c' ( it is so for ARM & RISC-V ) > > > + > > +#if !defined(_ASM_BUGFRAME_TEXT) || !defined(_ASM_BUGFRAME_INFO) > > + > > +#define > > _ASM_BUGFRAME_TEXT(second_frame) > > \ > > + > > ".Lbug%=:"BUG_INSTR"\n" > > \ > > + " .pushsection .bug_frames.%"BUG_ASM_CONST"[bf_type], \"a\", > > %%progbits\n" \ > > + " .p2align > > 2\n" > > \ > > + > > ".Lfrm%=:\n" > > \ > > + " .long (.Lbug%= - .Lfrm%=) + > > %"BUG_ASM_CONST"[bf_line_hi]\n" \ > > + " .long (%"BUG_ASM_CONST"[bf_ptr] - .Lfrm%=) + > > %"BUG_ASM_CONST"[bf_line_lo]\n"\ > > + " .if " #second_frame > > "\n" \ > > + " .long 0, %"BUG_ASM_CONST"[bf_msg] - > > .Lfrm%=\n" \ > > + " > > .endif\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) > > + > > +#endif /* _ASM_BUGFRAME_TEXT || _ASM_BUGFRAME_INFO */ > > + > > +#ifndef BUG_FRAME > > + > > +#define BUG_FRAME(type, line, ptr, second_frame, msg) do > > { \ > > + BUILD_BUG_ON((line) >> (BUG_LINE_LO_WIDTH + > > BUG_LINE_HI_WIDTH)); \ > > + BUILD_BUG_ON((type) >= > > BUGFRAME_NR); \ > > + asm volatile ( > > _ASM_BUGFRAME_TEXT(second_frame) \ > > + :: _ASM_BUGFRAME_INFO(type, line, ptr, msg) > > ); \ > > +} while (0) > > + > > +#endif > > + > > +#ifndef run_in_exception_handler > > + > > +/* > > + * TODO: untangle header dependences, break BUILD_BUG_ON() out of > > xen/lib.h, > > + * and use a real static inline here to get proper type checking > > of fn(). > > + */ > > +#define run_in_exception_handler(fn) \ > > + do { \ > > + (void)((fn) == (void (*)(struct cpu_user_regs *))NULL); \ > > + BUG_FRAME(BUGFRAME_run_fn, 0, fn, 0, NULL); \ > > + } while ( 0 ) > > + > > +#endif /* run_in_exception_handler */ > > + > > +#ifndef WARN > > +#define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, 0, > > NULL) > > +#endif > > + > > +#ifndef BUG > > +#define BUG() do { \ > > + BUG_FRAME(BUGFRAME_bug, __LINE__, __FILE__, 0, NULL); \ > > + unreachable(); \ > > +} while (0) > > +#endif > > + > > +#ifndef assert_failed > > +#define assert_failed(msg) do { \ > > + BUG_FRAME(BUGFRAME_assert, __LINE__, __FILE__, 1, msg); \ > > + unreachable(); \ > > +} while (0) > > +#endif > > + > > +#ifdef CONFIG_GENERIC_BUG_FRAME > > + > > +struct cpu_user_regs; > > + > > +int do_bug_frame(const struct cpu_user_regs *regs, unsigned long > > pc); > > + > > +#endif /* CONFIG_GENERIC_BUG_FRAME */ > > + > > +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 /* __XEN_BUG_H__ */ > > +/* > > + * Local variables: > > + * mode: C > > + * c-file-style: "BSD" > > + * c-basic-offset: 4 > > + * indent-tabs-mode: nil > > + * End: > > + */ > > Cheers, > ~ Oleksii
On 28/02/2023 17:21, Oleksii wrote: > Hi Julien, Hi Oleksii, >>> + >>> + for ( i = 0, b = region->frame[id].bugs; >>> + i < region->frame[id].n_bugs; b++, i++ ) >>> + { >>> + if ( bug_loc(b) == pc ) >>> + { >>> + bug = b; >>> + goto found; >>> + } >>> + } >>> + } >>> + } >>> + >>> + found: >>> + if ( !bug ) >>> + return -EINVAL; >>> + >>> + if ( id == BUGFRAME_run_fn ) >>> + { >>> +#ifdef BUG_FN_REG >>> + void (*fn)(const struct cpu_user_regs *) = (void *)regs- >>>> BUG_FN_REG; >> >> AFAIU, this is necessary so Arm can use the generic do_bug_frame(). >> >> I was under the impression that RISC-V and Arm had the similar issue >> with %c. It seems like you managed to resolve it on RISC-V, so can we >> fully switch Arm to the generic implementation of bug? > I tried to switch ARM to generic implementation. > > Here is the patch: [1] I have replied on the other thread. >>> +#ifndef BUG_ASM_CONST >>> +#define BUG_ASM_CONST "" >>> +#endif >> >> This line is a bit misterious to me. Would you be able to outline why >> an >> architecture would override this? > It is needed in case if compiler for an architecture doesn't have > proper support of '%c' ( it is so for ARM & RISC-V ) Hmmm.... Why can't x86 use the same version? IOW what's the benefits to differ on x86? Anyway, documentation is always good to have because it helps the reader/reviewer to understand how such decision was made. Cheers,
On Tue, 2023-02-28 at 18:01 +0000, Julien Grall wrote: > On 28/02/2023 17:21, Oleksii wrote: > > Hi Julien, > > Hi Oleksii, > > > > + > > > > + for ( i = 0, b = region->frame[id].bugs; > > > > + i < region->frame[id].n_bugs; b++, i++ ) > > > > + { > > > > + if ( bug_loc(b) == pc ) > > > > + { > > > > + bug = b; > > > > + goto found; > > > > + } > > > > + } > > > > + } > > > > + } > > > > + > > > > + found: > > > > + if ( !bug ) > > > > + return -EINVAL; > > > > + > > > > + if ( id == BUGFRAME_run_fn ) > > > > + { > > > > +#ifdef BUG_FN_REG > > > > + void (*fn)(const struct cpu_user_regs *) = (void > > > > *)regs- > > > > > BUG_FN_REG; > > > > > > AFAIU, this is necessary so Arm can use the generic > > > do_bug_frame(). > > > > > > I was under the impression that RISC-V and Arm had the similar > > > issue > > > with %c. It seems like you managed to resolve it on RISC-V, so > > > can we > > > fully switch Arm to the generic implementation of bug? > > I tried to switch ARM to generic implementation. > > > > Here is the patch: [1] > > I have replied on the other thread. > > > > +#ifndef BUG_ASM_CONST > > > > +#define BUG_ASM_CONST "" > > > > +#endif > > > > > > This line is a bit misterious to me. Would you be able to outline > > > why > > > an > > > architecture would override this? > > It is needed in case if compiler for an architecture doesn't have > > proper support of '%c' ( it is so for ARM & RISC-V ) > > Hmmm.... Why can't x86 use the same version? IOW what's the benefits > to > differ on x86? We can't use '%c' for all architectures because not all compiler supports '%c' fully for all architectures. There is no any benefits. In case of x86 it is needed to delete punctuation before immediate. I mean that immediate is passed as $1 ( or # i always missed with ARM ) and to drop $ it is used %c. > > Anyway, documentation is always good to have because it helps the > reader/reviewer to understand how such decision was made. I'll add the comment then before define. > > Cheers, > ~ Oleksii
diff --git a/xen/common/Kconfig b/xen/common/Kconfig index f1ea3199c8..b226323537 100644 --- a/xen/common/Kconfig +++ b/xen/common/Kconfig @@ -28,6 +28,9 @@ config ALTERNATIVE_CALL config ARCH_MAP_DOMAIN_PAGE bool +config GENERIC_BUG_FRAME + bool + config HAS_ALTERNATIVE bool diff --git a/xen/common/Makefile b/xen/common/Makefile index bbd75b4be6..46049eac35 100644 --- a/xen/common/Makefile +++ b/xen/common/Makefile @@ -1,5 +1,6 @@ obj-$(CONFIG_ARGO) += argo.o obj-y += bitmap.o +obj-$(CONFIG_GENERIC_BUG_FRAME) += bug.o obj-$(CONFIG_HYPFS_CONFIG) += config_data.o obj-$(CONFIG_CORE_PARKING) += core_parking.o obj-y += cpu.o diff --git a/xen/common/bug.c b/xen/common/bug.c new file mode 100644 index 0000000000..f81724fc9b --- /dev/null +++ b/xen/common/bug.c @@ -0,0 +1,109 @@ +#include <xen/bug.h> +#include <xen/debugger.h> +#include <xen/errno.h> +#include <xen/kernel.h> +#include <xen/livepatch.h> +#include <xen/string.h> +#include <xen/types.h> +#include <xen/virtual_region.h> + +#include <asm/processor.h> + +/* Set default value for TRAP_invalid_op as it is defined only for X86 now */ +#ifndef TRAP_invalid_op +#define TRAP_invalid_op 0 +#endif + +int do_bug_frame(const struct cpu_user_regs *regs, unsigned long pc) +{ + const struct bug_frame *bug = NULL; + const struct virtual_region *region; + const char *prefix = "", *filename, *predicate; + unsigned long fixup; + unsigned int id = BUGFRAME_NR, lineno; + + region = find_text_region(pc); + if ( region ) + { + for ( id = 0; id < BUGFRAME_NR; id++ ) + { + const struct bug_frame *b; + unsigned int i; + + for ( i = 0, b = region->frame[id].bugs; + i < region->frame[id].n_bugs; b++, i++ ) + { + if ( bug_loc(b) == pc ) + { + bug = b; + goto found; + } + } + } + } + + found: + if ( !bug ) + return -EINVAL; + + if ( id == BUGFRAME_run_fn ) + { +#ifdef BUG_FN_REG + void (*fn)(const struct cpu_user_regs *) = (void *)regs->BUG_FN_REG; +#else + void (*fn)(const struct cpu_user_regs *) = bug_ptr(bug); +#endif + + fn(regs); + + return id; + } + + /* WARN, BUG or ASSERT: decode the filename pointer and line number. */ + filename = bug_ptr(bug); + if ( !is_kernel(filename) && !is_patch(filename) ) + return -EINVAL; + fixup = strlen(filename); + if ( fixup > 50 ) + { + filename += fixup - 47; + prefix = "..."; + } + lineno = bug_line(bug); + + switch ( id ) + { + case BUGFRAME_warn: + printk("Xen WARN at %s%s:%d\n", prefix, filename, lineno); + show_execution_state(regs); + + return id; + + case BUGFRAME_bug: + printk("Xen BUG at %s%s:%d\n", prefix, filename, lineno); + + if ( debugger_trap_fatal(TRAP_invalid_op, regs) ) + return id; + + show_execution_state(regs); + panic("Xen BUG at %s%s:%d\n", prefix, filename, lineno); + + case BUGFRAME_assert: + /* ASSERT: decode the predicate string pointer. */ + predicate = bug_msg(bug); + if ( !is_kernel(predicate) && !is_patch(predicate) ) + predicate = "<unknown>"; + + printk("Assertion '%s' failed at %s%s:%d\n", + predicate, prefix, filename, lineno); + + if ( debugger_trap_fatal(TRAP_invalid_op, regs) ) + return id; + + show_execution_state(regs); + panic("Assertion '%s' failed at %s%s:%d\n", + predicate, prefix, filename, lineno); + } + + return id; +} diff --git a/xen/include/xen/bug.h b/xen/include/xen/bug.h new file mode 100644 index 0000000000..4b18cfa69c --- /dev/null +++ b/xen/include/xen/bug.h @@ -0,0 +1,150 @@ +#ifndef __XEN_BUG_H__ +#define __XEN_BUG_H__ + +#define BUG_DISP_WIDTH 24 +#define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH) +#define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH) + +#define BUGFRAME_run_fn 0 +#define BUGFRAME_warn 1 +#define BUGFRAME_bug 2 +#define BUGFRAME_assert 3 + +#define BUGFRAME_NR 4 + +#include <asm/bug.h> + +#ifndef __ASSEMBLY__ + +#include <xen/errno.h> +#include <xen/lib.h> +#include <xen/stringify.h> + +#ifndef BUG_FRAME_STRUCT + +struct bug_frame { + signed int loc_disp:BUG_DISP_WIDTH; + unsigned int line_hi:BUG_LINE_HI_WIDTH; + signed int ptr_disp:BUG_DISP_WIDTH; + unsigned int line_lo:BUG_LINE_LO_WIDTH; + signed int msg_disp[]; +}; + +#endif /* BUG_FRAME_STRUCT */ + +#ifndef bug_loc +#define bug_loc(b) ((unsigned long)(b) + (b)->loc_disp) +#endif + +#ifndef bug_ptr +#define bug_ptr(b) ((const void *)(b) + (b)->ptr_disp) +#endif + +#ifndef bug_line +#define bug_line(b) (((((b)->line_hi + ((b)->loc_disp < 0)) & \ + ((1 << BUG_LINE_HI_WIDTH) - 1)) << \ + BUG_LINE_LO_WIDTH) + \ + (((b)->line_lo + ((b)->ptr_disp < 0)) & \ + ((1 << BUG_LINE_LO_WIDTH) - 1))) +#endif + +#ifndef bug_msg +#define bug_msg(b) ((const char *)(b) + (b)->msg_disp[1]) +#endif + +#ifndef BUG_ASM_CONST +#define BUG_ASM_CONST "" +#endif + +#if !defined(_ASM_BUGFRAME_TEXT) || !defined(_ASM_BUGFRAME_INFO) + +#define _ASM_BUGFRAME_TEXT(second_frame) \ + ".Lbug%=:"BUG_INSTR"\n" \ + " .pushsection .bug_frames.%"BUG_ASM_CONST"[bf_type], \"a\", %%progbits\n" \ + " .p2align 2\n" \ + ".Lfrm%=:\n" \ + " .long (.Lbug%= - .Lfrm%=) + %"BUG_ASM_CONST"[bf_line_hi]\n" \ + " .long (%"BUG_ASM_CONST"[bf_ptr] - .Lfrm%=) + %"BUG_ASM_CONST"[bf_line_lo]\n"\ + " .if " #second_frame "\n" \ + " .long 0, %"BUG_ASM_CONST"[bf_msg] - .Lfrm%=\n" \ + " .endif\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) + +#endif /* _ASM_BUGFRAME_TEXT || _ASM_BUGFRAME_INFO */ + +#ifndef BUG_FRAME + +#define BUG_FRAME(type, line, ptr, second_frame, msg) do { \ + BUILD_BUG_ON((line) >> (BUG_LINE_LO_WIDTH + BUG_LINE_HI_WIDTH)); \ + BUILD_BUG_ON((type) >= BUGFRAME_NR); \ + asm volatile ( _ASM_BUGFRAME_TEXT(second_frame) \ + :: _ASM_BUGFRAME_INFO(type, line, ptr, msg) ); \ +} while (0) + +#endif + +#ifndef run_in_exception_handler + +/* + * TODO: untangle header dependences, break BUILD_BUG_ON() out of xen/lib.h, + * and use a real static inline here to get proper type checking of fn(). + */ +#define run_in_exception_handler(fn) \ + do { \ + (void)((fn) == (void (*)(struct cpu_user_regs *))NULL); \ + BUG_FRAME(BUGFRAME_run_fn, 0, fn, 0, NULL); \ + } while ( 0 ) + +#endif /* run_in_exception_handler */ + +#ifndef WARN +#define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, 0, NULL) +#endif + +#ifndef BUG +#define BUG() do { \ + BUG_FRAME(BUGFRAME_bug, __LINE__, __FILE__, 0, NULL); \ + unreachable(); \ +} while (0) +#endif + +#ifndef assert_failed +#define assert_failed(msg) do { \ + BUG_FRAME(BUGFRAME_assert, __LINE__, __FILE__, 1, msg); \ + unreachable(); \ +} while (0) +#endif + +#ifdef CONFIG_GENERIC_BUG_FRAME + +struct cpu_user_regs; + +int do_bug_frame(const struct cpu_user_regs *regs, unsigned long pc); + +#endif /* CONFIG_GENERIC_BUG_FRAME */ + +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 /* __XEN_BUG_H__ */ +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * indent-tabs-mode: nil + * End: + */
A large part of the content of the bug.h is repeated among all architectures, so it was decided to create a new config CONFIG_GENERIC_BUG_FRAME. The version of <bug.h> from x86 was taken as the base version. The patch introduces the following stuff: * common bug.h header * generic implementation of do_bug_frame * new config CONFIG_GENERIC_BUG_FRAME Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- Changes in V3: * Add debugger_trap_fatal() to do_bug_frame(). It simplifies usage of do_bug_frame() for x86 so making handle_bug_frame() and find_bug_frame() not needed anymore. * Update do_bug_frame() to return -EINVAL if something goes wrong; otherwise id of bug_frame * Update _ASM_BUGFRAME_TEXT to make it more portable. * Drop unnecessary comments. * define stub value for TRAP_invalid_op in case if wasn't defined in arch-specific folders. --- Changes in V2: - Switch to x86 implementation as generic as it is more compact ( at least from the point of view of bug frame structure ). - Rename CONFIG_GENERIC_DO_BUG_FRAME to CONFIG_GENERIC_BUG_FRAME. - Change the macro bug_loc(b) to avoid the need for a cast: #define bug_loc(b) ((unsigned long)(b) + (b)->loc_disp) - Rename BUG_FRAME_STUFF to BUG_FRAME_STRUCT - Make macros related to bug frame structure more generic. - Introduce BUG_INSTR and MODIFIER to make _ASM_BUGFRAME_TEXT reusable between x86 and RISC-V. - Rework do_bug_frame() and introduce find_bug_frame() and handle_bug_frame() functions to make it reusable by x86. - code style fixes --- xen/common/Kconfig | 3 + xen/common/Makefile | 1 + xen/common/bug.c | 109 ++++++++++++++++++++++++++++++ xen/include/xen/bug.h | 150 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 263 insertions(+) create mode 100644 xen/common/bug.c create mode 100644 xen/include/xen/bug.h