| Message ID | 5cd3dd5233cf41ad54ce1cd98b706085b95bfcee.1677839409.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | introduce generic implementation of macros from bug.h | expand |
On 03.03.2023 11:38, Oleksii Kurochko wrote: > --- 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 With Arm now also going to use the generic logic, do we actually need this control anymore (provided things have been proven to work on Arm for the compiler range we support there)? It looks like because of the way the series is partitioned it may be necessary transiently, but it should be possible to drop it again in a new 5th patch. > --- /dev/null > +++ b/xen/common/bug.c > @@ -0,0 +1,103 @@ > +#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> > + > +/* > + * Returns a negative value in case of an error otherwise the bug type. > + */ Nit: Style (this is a single line comment). But see also below related comment on the function's declaration. > +int do_bug_frame(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 ) > + return -EINVAL; > + > + for ( id = 0; id < BUGFRAME_NR; id++ ) > + { > + const struct bug_frame *b; > + size_t i; > + > + for ( i = 0, b = region->frame[id].bugs; > + i < region->frame[id].n_bugs; b++, i++ ) Nit: Indentation (the "i" on the 2nd line wants to align with that on the 1st one). > + { > + if ( bug_loc(b) == pc ) > + { > + bug = b; > + goto found; > + } > + } > + } > + > + found: > + if ( !bug ) > + return -EINVAL; While I'm generally unhappy with many uses of -EINVAL (it's used to indicate way too many different kinds of errors), can we at least consider using -ENOENT here instead? (I'm sorry, I should have asked for this earlier on.) > --- /dev/null > +++ b/xen/include/xen/bug.h > @@ -0,0 +1,158 @@ > +#ifndef __XEN_BUG_H__ > +#define __XEN_BUG_H__ > + > +#define BUGFRAME_run_fn 0 > +#define BUGFRAME_warn 1 > +#define BUGFRAME_bug 2 > +#define BUGFRAME_assert 3 > + > +#define BUGFRAME_NR 4 > + > +#ifndef BUG_FRAME_STRUCT This check won't help when it comes ahead of ... > +#define BUG_DISP_WIDTH 24 > +#define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH) > +#define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH) > +#endif > + > +#include <asm/bug.h> ... this. But is the #ifdef actually necessary? Or can the #define-s be moved ... > +#ifndef BUG_DEBUGGER_TRAP_FATAL > +#define BUG_DEBUGGER_TRAP_FATAL(regs) 0 > +#endif > + > +#ifndef __ASSEMBLY__ > + > +#include <xen/lib.h> > + > +#ifndef BUG_FRAME_STRUCT ... here? (I guess having them defined early, but unconditionally is better. If an arch wants to override them, they can #undef and then #define.) Above from here the "#ifndef __ASSEMBLY__" also wants to move up, to further enclose BUG_DEBUGGER_TRAP_FATAL (which is useless in assembly code). > +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[]; > +}; > + > +#define bug_loc(b) ((unsigned long)(b) + (b)->loc_disp) > + > +#define bug_ptr(b) ((const void *)(b) + (b)->ptr_disp) > + > +#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))) > + > +#define bug_msg(b) ((const char *)(b) + (b)->msg_disp[1]) > + > +#endif /* BUG_FRAME_STRUCT */ > + > +/* > + * Generic implementation has been based on x86 implementation where > + * '%c' to deal with punctation sign ($, # depending on architecture) > + * before immediate. > + * > + * Not all architecture's compilers have full support of '%c' and not for all > + * assemblers punctation sign is used before immediate. > + * Thereby it was decided to introduce BUG_ASM_CONST. > + */ I have to admit that I'm not really happy with this comment. At the very least the last sentence imo doesn't belong there. But overall how about something like "Some architectures mark immediate instruction operands in a special way. In such cases the special marking may need omitting when specifying directive operands. Allow architectures to specify a suitable modifier." > +#ifndef BUG_ASM_CONST > +#define BUG_ASM_CONST "" > +#endif > + > +#if !defined(_ASM_BUGFRAME_TEXT) || !defined(_ASM_BUGFRAME_INFO) Please don't make the conditional more complicated than necessary: Checking for just _ASM_BUGFRAME_TEXT is enough here. > +#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) Isn't this tied to BUG_FRAME_STRUCT being defined (or not)? At least the 1st BUILD_BUG_ON() looks problematic if an arch wasn't to use the generic struct: With how you have things right now BUG_LINE_{LO,HI}_WIDTH may not be defined, and the check would also be at risk of causing false positives. Perhaps it's appropriate to have a separate #ifdef (incl the distinct identifier used), but that first BUILD_BUG_ON() needs abstracting out. Also nit: Style (see ... > +#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 ) ... here). Also, considering the boolean nature, I guess we're in the process of moving such to be "} while ( false );". Also please be consistent with formatting of the two adjacent macros, both indentation-wise and where "do {" is placed. Which of the two forms you use is secondary. > +#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; > + > +/* > + * Returns a negative value in case of an error otherwise the bug type. > + */ Perhaps add "(BUGFRAME_*)", which would then also make this a properly multi-line comment (right now it's a style violation, as is the case for the function definition as pointed out above)? Jan
On 03.03.2023 11:38, Oleksii Kurochko wrote: > +int do_bug_frame(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 ) > + return -EINVAL; > + > + for ( id = 0; id < BUGFRAME_NR; id++ ) > + { > + const struct bug_frame *b; > + size_t 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 ) > + { > + void (*fn)(const struct cpu_user_regs *) = bug_ptr(bug); > + > + 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 ( BUG_DEBUGGER_TRAP_FATAL(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 ( BUG_DEBUGGER_TRAP_FATAL(regs) ) > + return id; > + > + show_execution_state(regs); > + panic("Assertion '%s' failed at %s%s:%d\n", > + predicate, prefix, filename, lineno); > + } > + > + return id; > +} This final "return" looks like almost dead code (it isn't when an unrecognized id is found). May I suggest to switch all "return id" inside the switch() block to just "break", to make this final "return" be (a) the central one and (b) more obviously a used path? Jan
On Mon, 2023-03-06 at 11:17 +0100, Jan Beulich wrote: > > On 03.03.2023 11:38, Oleksii Kurochko wrote: > > > > --- 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 > > > > With Arm now also going to use the generic logic, do we actually > > need > > this > > control anymore (provided things have been proven to work on Arm > > for > > the > > compiler range we support there)? It looks like because of the way > > the > > series is partitioned it may be necessary transiently, but it > > should > > be > > possible to drop it again in a new 5th patch. We still need it because RISC-V doesn't ready yet to use do_bug_frame() from xen/common/bug.c as it requires find_text_region(), virtual_region() and panic(). The mentioned functionality isn't ready now for RISC-V so RISC-V will use only generic things from <xen/bug.h> only for some time. Another one reason I would like to leave the config it is probably the same situation will be for future architectures. This is not something mandatory if the config isn't really necessary than it should be removed after RISC-V will be ready to migrate full on generic implementation. > > > > > > --- /dev/null > > > > +++ b/xen/common/bug.c > > > > @@ -0,0 +1,103 @@ > > > > +#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> > > > > + > > > > +/* > > > > + * Returns a negative value in case of an error otherwise the > > > > bug > > > > type. > > > > + */ > > > > Nit: Style (this is a single line comment). But see also below > > related > > comment on the function's declaration. Thanks. I'll fix that. > > > > > > +int do_bug_frame(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 ) > > > > + return -EINVAL; > > > > + > > > > + for ( id = 0; id < BUGFRAME_NR; id++ ) > > > > + { > > > > + const struct bug_frame *b; > > > > + size_t i; > > > > + > > > > + for ( i = 0, b = region->frame[id].bugs; > > > > + i < region->frame[id].n_bugs; b++, i++ ) > > > > Nit: Indentation (the "i" on the 2nd line wants to align with that > > on the 1st one). Thanks. I'll update the indentation. > > > > > > + { > > > > + if ( bug_loc(b) == pc ) > > > > + { > > > > + bug = b; > > > > + goto found; > > > > + } > > > > + } > > > > + } > > > > + > > > > + found: > > > > + if ( !bug ) > > > > + return -EINVAL; > > > > While I'm generally unhappy with many uses of -EINVAL (it's used to > > indicate way too many different kinds of errors), can we at least > > consider using -ENOENT here instead? (I'm sorry, I should have > > asked > > for this earlier on.) Sure. Done. > > > > > > --- /dev/null > > > > +++ b/xen/include/xen/bug.h > > > > @@ -0,0 +1,158 @@ > > > > +#ifndef __XEN_BUG_H__ > > > > +#define __XEN_BUG_H__ > > > > + > > > > +#define BUGFRAME_run_fn 0 > > > > +#define BUGFRAME_warn 1 > > > > +#define BUGFRAME_bug 2 > > > > +#define BUGFRAME_assert 3 > > > > + > > > > +#define BUGFRAME_NR 4 > > > > + > > > > +#ifndef BUG_FRAME_STRUCT > > > > This check won't help when it comes ahead of ... > > > > > > +#define BUG_DISP_WIDTH 24 > > > > +#define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH) > > > > +#define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH) > > > > +#endif > > > > + > > > > +#include <asm/bug.h> > > > > ... this. But is the #ifdef actually necessary? Or can the #define- > > s > > be moved ... > > > > > > +#ifndef BUG_DEBUGGER_TRAP_FATAL > > > > +#define BUG_DEBUGGER_TRAP_FATAL(regs) 0 > > > > +#endif > > > > + > > > > +#ifndef __ASSEMBLY__ > > > > + > > > > +#include <xen/lib.h> > > > > + > > > > +#ifndef BUG_FRAME_STRUCT > > > > ... here? (I guess having them defined early, but unconditionally > > is > > better. If an arch wants to override them, they can #undef and then > > #define.) We can't move it to #indef __ASSEMBLY__ because in this case x86 compilation will fail as in x86's <asm/bug.h> BUG_DISP_WIDTH, BUG_LINE_LO_WIDTH, BUG_LINE_HI_WIDTH are used in case when the header is included in assembly code. I agree that there is no any sense to have the defines under "#ifndef BUG_FRAME_STUCT" before <asm/bug.h> but it is necessary to define them before <asm/bug.h>. The defines were put under "#ifndef BUG_FRAME_STUCT" because it seems to me that logically the defines should go only with definition of 'struct bug_frame'. So it looks like the only one way we have. It is define them unconditionally before <asm/bug.h> and #undef if it will be necessary for specific architecture. As an option we can move the defines to #ifndef BUG_FRAME_STRUCT under #ifndef __ASSEMBLY__ and then define them explicitly in x86's <asm/bug.h> for case when the header is included some where in assembly code. But this option looks really weird. > > > > Above from here the "#ifndef __ASSEMBLY__" also wants to move up, > > to > > further enclose BUG_DEBUGGER_TRAP_FATAL (which is useless in > > assembly > > code). Yeah, sure. Will move up "#ifndef __ASSEMBLY__" > > > > > > +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[]; > > > > +}; > > > > + > > > > +#define bug_loc(b) ((unsigned long)(b) + (b)->loc_disp) > > > > + > > > > +#define bug_ptr(b) ((const void *)(b) + (b)->ptr_disp) > > > > + > > > > +#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))) > > > > + > > > > +#define bug_msg(b) ((const char *)(b) + (b)->msg_disp[1]) > > > > + > > > > +#endif /* BUG_FRAME_STRUCT */ > > > > + > > > > +/* > > > > + * Generic implementation has been based on x86 implementation > > > > where > > > > + * '%c' to deal with punctation sign ($, # depending on > > > > architecture) > > > > + * before immediate. > > > > + * > > > > + * Not all architecture's compilers have full support of '%c' > > > > and > > > > not for all > > > > + * assemblers punctation sign is used before immediate. > > > > + * Thereby it was decided to introduce BUG_ASM_CONST. > > > > + */ > > > > I have to admit that I'm not really happy with this comment. At the > > very > > least the last sentence imo doesn't belong there. But overall how > > about > > something like > > > > "Some architectures mark immediate instruction operands in a > > special > > way. > > In such cases the special marking may need omitting when > > specifying > > directive operands. Allow architectures to specify a suitable > > modifier." Your comment looks really better. So I'll use it. > > > > > > +#ifndef BUG_ASM_CONST > > > > +#define BUG_ASM_CONST "" > > > > +#endif > > > > + > > > > +#if !defined(_ASM_BUGFRAME_TEXT) || > > > > !defined(_ASM_BUGFRAME_INFO) > > > > Please don't make the conditional more complicated than necessary: > > Checking for just _ASM_BUGFRAME_TEXT is enough here. Sure. I'll update that. > > > > > > +#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) > > > > Isn't this tied to BUG_FRAME_STRUCT being defined (or not)? At > > least > > the 1st BUILD_BUG_ON() looks problematic if an arch wasn't to use > > the generic struct: With how you have things right now > > BUG_LINE_{LO,HI}_WIDTH may not be defined, and the check would also > > be at risk of causing false positives. Perhaps it's appropriate to > > have a separate #ifdef (incl the distinct identifier used), but > > that > > first BUILD_BUG_ON() needs abstracting out. Missed that. Thanks. I'll introduce that a separate #ifdef before BUG_FRAME: #ifndef BUILD_BUG_ON_LINE_WIDTH #define BUILD_BUG_ON_LINE_WIDTH \ BUILD_BUG_ON((line) >> (BUG_LINE_LO_WIDTH + BUG_LINE_HI_WIDTH)) #endif > > > > Also nit: Style (see ... > > > > > > +#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 ) > > > > ... here). Also, considering the boolean nature, I guess we're in > > the > > process of moving such to be "} while ( false );". > > > > Also please be consistent with formatting of the two adjacent > > macros, > > both indentation-wise and where "do {" is placed. Which of the two > > forms you use is secondary. Thanks. Done. > > > > > > +#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; > > > > + > > > > +/* > > > > + * Returns a negative value in case of an error otherwise the > > > > bug > > > > type. > > > > + */ > > > > Perhaps add "(BUGFRAME_*)", which would then also make this a > > properly > > multi-line comment (right now it's a style violation, as is the > > case > > for the function definition as pointed out above)? > > Updated. Thanks. ~ Oleksii
On Mon, 2023-03-06 at 11:41 +0100, Jan Beulich wrote: > On 03.03.2023 11:38, Oleksii Kurochko wrote: > > +int do_bug_frame(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 ) > > + return -EINVAL; > > + > > + for ( id = 0; id < BUGFRAME_NR; id++ ) > > + { > > + const struct bug_frame *b; > > + size_t 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 ) > > + { > > + void (*fn)(const struct cpu_user_regs *) = bug_ptr(bug); > > + > > + 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 ( BUG_DEBUGGER_TRAP_FATAL(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 ( BUG_DEBUGGER_TRAP_FATAL(regs) ) > > + return id; > > + > > + show_execution_state(regs); > > + panic("Assertion '%s' failed at %s%s:%d\n", > > + predicate, prefix, filename, lineno); > > + } > > + > > + return id; > > +} > > This final "return" looks like almost dead code (it isn't when an > unrecognized id is found). May I suggest to switch all "return id" > inside the switch() block to just "break", to make this final > "return" be (a) the central one and (b) more obviously a used path? > Sure It will be much better. ~ Oleksii
On 07.03.2023 12:32, Oleksii wrote: > On Mon, 2023-03-06 at 11:17 +0100, Jan Beulich wrote: >>> On 03.03.2023 11:38, Oleksii Kurochko wrote: >>>>> --- 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 >>> >>> With Arm now also going to use the generic logic, do we actually >>> need >>> this >>> control anymore (provided things have been proven to work on Arm >>> for >>> the >>> compiler range we support there)? It looks like because of the way >>> the >>> series is partitioned it may be necessary transiently, but it >>> should >>> be >>> possible to drop it again in a new 5th patch. > We still need it because RISC-V doesn't ready yet to use do_bug_frame() > from xen/common/bug.c as it requires find_text_region(), > virtual_region() and panic(). > The mentioned functionality isn't ready now for RISC-V so RISC-V will > use only generic things from <xen/bug.h> only for some time. Oh, right. So let's leave it for the time being, but consider dropping it once RISC-V is more complete. >>>>> --- /dev/null >>>>> +++ b/xen/include/xen/bug.h >>>>> @@ -0,0 +1,158 @@ >>>>> +#ifndef __XEN_BUG_H__ >>>>> +#define __XEN_BUG_H__ >>>>> + >>>>> +#define BUGFRAME_run_fn 0 >>>>> +#define BUGFRAME_warn 1 >>>>> +#define BUGFRAME_bug 2 >>>>> +#define BUGFRAME_assert 3 >>>>> + >>>>> +#define BUGFRAME_NR 4 >>>>> + >>>>> +#ifndef BUG_FRAME_STRUCT >>> >>> This check won't help when it comes ahead of ... >>> >>>>> +#define BUG_DISP_WIDTH 24 >>>>> +#define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH) >>>>> +#define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH) >>>>> +#endif >>>>> + >>>>> +#include <asm/bug.h> >>> >>> ... this. But is the #ifdef actually necessary? Or can the #define- >>> s >>> be moved ... >>> >>>>> +#ifndef BUG_DEBUGGER_TRAP_FATAL >>>>> +#define BUG_DEBUGGER_TRAP_FATAL(regs) 0 >>>>> +#endif >>>>> + >>>>> +#ifndef __ASSEMBLY__ >>>>> + >>>>> +#include <xen/lib.h> >>>>> + >>>>> +#ifndef BUG_FRAME_STRUCT >>> >>> ... here? (I guess having them defined early, but unconditionally >>> is >>> better. If an arch wants to override them, they can #undef and then >>> #define.) > We can't move it to #indef __ASSEMBLY__ because in this case x86 > compilation will fail as in x86's <asm/bug.h> BUG_DISP_WIDTH, > BUG_LINE_LO_WIDTH, BUG_LINE_HI_WIDTH are used in case when the header > is included in assembly code. Oh, of course. > I agree that there is no any sense to have the defines under "#ifndef > BUG_FRAME_STUCT" before <asm/bug.h> but it is necessary to define them > before <asm/bug.h>. The defines were put under "#ifndef > BUG_FRAME_STUCT" because it seems to me that logically the defines > should go only with definition of 'struct bug_frame'. > > So it looks like the only one way we have. It is define them > unconditionally before <asm/bug.h> and #undef if it will be necessary > for specific architecture. > As an option we can move the defines to #ifndef BUG_FRAME_STRUCT under > #ifndef __ASSEMBLY__ and then define them explicitly in x86's > <asm/bug.h> for case when the header is included some where in assembly > code. But this option looks really weird. Indeed. >>>>> +#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) >>> >>> Isn't this tied to BUG_FRAME_STRUCT being defined (or not)? At >>> least >>> the 1st BUILD_BUG_ON() looks problematic if an arch wasn't to use >>> the generic struct: With how you have things right now >>> BUG_LINE_{LO,HI}_WIDTH may not be defined, and the check would also >>> be at risk of causing false positives. Perhaps it's appropriate to >>> have a separate #ifdef (incl the distinct identifier used), but >>> that >>> first BUILD_BUG_ON() needs abstracting out. > Missed that. Thanks. > I'll introduce that a separate #ifdef before BUG_FRAME: > > #ifndef BUILD_BUG_ON_LINE_WIDTH > #define BUILD_BUG_ON_LINE_WIDTH \ > BUILD_BUG_ON((line) >> (BUG_LINE_LO_WIDTH + BUG_LINE_HI_WIDTH)) > #endif But then please make this a function-like macro. I'm also not convinced of the #ifndef - an arch using an entirely different layout should have no need for this check. So I'd rather attach the #define to what's inside #ifndef BUG_FRAME_STRUCT and have an #else there to supply a #define to <empty> alternatively. Jan
> > > > > + > > > > > +#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) > > > > > > Isn't this tied to BUG_FRAME_STRUCT being defined (or not)? At > > > least > > > the 1st BUILD_BUG_ON() looks problematic if an arch wasn't to use > > > the generic struct: With how you have things right now > > > BUG_LINE_{LO,HI}_WIDTH may not be defined, and the check would > > > also > > > be at risk of causing false positives. Perhaps it's appropriate > > > to > > > have a separate #ifdef (incl the distinct identifier used), but > > > that > > > first BUILD_BUG_ON() needs abstracting out. > Missed that. Thanks. > I'll introduce that a separate #ifdef before BUG_FRAME: > > #ifndef BUILD_BUG_ON_LINE_WIDTH > #define BUILD_BUG_ON_LINE_WIDTH \ > BUILD_BUG_ON((line) >> (BUG_LINE_LO_WIDTH + > BUG_LINE_HI_WIDTH)) > #endif I think even better will be to do in the following way: #ifndef LINE_WIDTH #define LINE_WIDTH (BUG_LINE_LO_WIDTH + BUG_LINE_HI_WIDTH) #endif #define BUG_FRAME(type, line, ptr, second_frame, msg) do { \ BUILD_BUG_ON((line) >> LINE_WIDTH); \ BUILD_BUG_ON((type) >= BUGFRAME_NR); \ asm volatile ( _ASM_BUGFRAME_TEXT(second_frame) \ :: _ASM_BUGFRAME_INFO(type, line, ptr, msg) ); \ } while (false) ~ Oleksii
On 07.03.2023 14:13, Oleksii wrote: >>>>>> + >>>>>> +#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) >>>> >>>> Isn't this tied to BUG_FRAME_STRUCT being defined (or not)? At >>>> least >>>> the 1st BUILD_BUG_ON() looks problematic if an arch wasn't to use >>>> the generic struct: With how you have things right now >>>> BUG_LINE_{LO,HI}_WIDTH may not be defined, and the check would >>>> also >>>> be at risk of causing false positives. Perhaps it's appropriate >>>> to >>>> have a separate #ifdef (incl the distinct identifier used), but >>>> that >>>> first BUILD_BUG_ON() needs abstracting out. >> Missed that. Thanks. >> I'll introduce that a separate #ifdef before BUG_FRAME: >> >> #ifndef BUILD_BUG_ON_LINE_WIDTH >> #define BUILD_BUG_ON_LINE_WIDTH \ >> BUILD_BUG_ON((line) >> (BUG_LINE_LO_WIDTH + >> BUG_LINE_HI_WIDTH)) >> #endif > I think even better will be to do in the following way: > > #ifndef LINE_WIDTH > #define LINE_WIDTH (BUG_LINE_LO_WIDTH + BUG_LINE_HI_WIDTH) > #endif > > #define BUG_FRAME(type, line, ptr, second_frame, msg) do { > \ > BUILD_BUG_ON((line) >> LINE_WIDTH); > \ > BUILD_BUG_ON((type) >= BUGFRAME_NR); > \ > asm volatile ( _ASM_BUGFRAME_TEXT(second_frame) > \ > :: _ASM_BUGFRAME_INFO(type, line, ptr, msg) ); > \ > } while (false) Not sure - that'll still require arches to define LINE_WIDTH. What if they store the line number in a 32-bit field? Then defining LINE_WIDTH to 32 would yield undefined behavior here. Jan
On Tue, 2023-03-07 at 14:16 +0100, Jan Beulich wrote: > On 07.03.2023 14:13, Oleksii wrote: > > > > > > > + > > > > > > > +#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) > > > > > > > > > > Isn't this tied to BUG_FRAME_STRUCT being defined (or not)? > > > > > At > > > > > least > > > > > the 1st BUILD_BUG_ON() looks problematic if an arch wasn't to > > > > > use > > > > > the generic struct: With how you have things right now > > > > > BUG_LINE_{LO,HI}_WIDTH may not be defined, and the check > > > > > would > > > > > also > > > > > be at risk of causing false positives. Perhaps it's > > > > > appropriate > > > > > to > > > > > have a separate #ifdef (incl the distinct identifier used), > > > > > but > > > > > that > > > > > first BUILD_BUG_ON() needs abstracting out. > > > Missed that. Thanks. > > > I'll introduce that a separate #ifdef before BUG_FRAME: > > > > > > #ifndef BUILD_BUG_ON_LINE_WIDTH > > > #define BUILD_BUG_ON_LINE_WIDTH \ > > > BUILD_BUG_ON((line) >> (BUG_LINE_LO_WIDTH + > > > BUG_LINE_HI_WIDTH)) > > > #endif > > I think even better will be to do in the following way: > > > > #ifndef LINE_WIDTH > > #define LINE_WIDTH (BUG_LINE_LO_WIDTH + BUG_LINE_HI_WIDTH) > > #endif > > > > #define BUG_FRAME(type, line, ptr, second_frame, msg) do > > { > > \ > > BUILD_BUG_ON((line) >> > > LINE_WIDTH); > > \ > > BUILD_BUG_ON((type) >= > > BUGFRAME_NR); > > \ > > asm volatile ( > > _ASM_BUGFRAME_TEXT(second_frame) > > \ > > :: _ASM_BUGFRAME_INFO(type, line, ptr, msg) > > ); > > \ > > } while (false) > > Not sure - that'll still require arches to define LINE_WIDTH. What > if they store the line number in a 32-bit field? Then defining > LINE_WIDTH to 32 would yield undefined behavior here. > It might be an issue. Than it will be better to have function-like macros mentioned in previous e-mail. Thanks ~ 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..e636edd85a --- /dev/null +++ b/xen/common/bug.c @@ -0,0 +1,103 @@ +#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> + +/* + * Returns a negative value in case of an error otherwise the bug type. + */ +int do_bug_frame(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 ) + return -EINVAL; + + for ( id = 0; id < BUGFRAME_NR; id++ ) + { + const struct bug_frame *b; + size_t 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 ) + { + void (*fn)(const struct cpu_user_regs *) = bug_ptr(bug); + + 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 ( BUG_DEBUGGER_TRAP_FATAL(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 ( BUG_DEBUGGER_TRAP_FATAL(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..722ec5c23b --- /dev/null +++ b/xen/include/xen/bug.h @@ -0,0 +1,158 @@ +#ifndef __XEN_BUG_H__ +#define __XEN_BUG_H__ + +#define BUGFRAME_run_fn 0 +#define BUGFRAME_warn 1 +#define BUGFRAME_bug 2 +#define BUGFRAME_assert 3 + +#define BUGFRAME_NR 4 + +#ifndef BUG_FRAME_STRUCT +#define BUG_DISP_WIDTH 24 +#define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH) +#define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH) +#endif + +#include <asm/bug.h> + +#ifndef BUG_DEBUGGER_TRAP_FATAL +#define BUG_DEBUGGER_TRAP_FATAL(regs) 0 +#endif + +#ifndef __ASSEMBLY__ + +#include <xen/lib.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[]; +}; + +#define bug_loc(b) ((unsigned long)(b) + (b)->loc_disp) + +#define bug_ptr(b) ((const void *)(b) + (b)->ptr_disp) + +#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))) + +#define bug_msg(b) ((const char *)(b) + (b)->msg_disp[1]) + +#endif /* BUG_FRAME_STRUCT */ + +/* + * Generic implementation has been based on x86 implementation where + * '%c' to deal with punctation sign ($, # depending on architecture) + * before immediate. + * + * Not all architecture's compilers have full support of '%c' and not for all + * assemblers punctation sign is used before immediate. + * Thereby it was decided to introduce BUG_ASM_CONST. + */ +#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; + +/* + * Returns a negative value in case of an error otherwise the bug type. + */ +int do_bug_frame(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 V5: * Remove "#ifdef BUG_FN_REG..." from generic do_bug_frame() as ARM will use generic implementation fully. --- Changes in V4: * common/bug.c: - Use BUG_DEBUGGER_TRAP_FATAL(regs) mnacros instead of debugger_trap_fatal(TRAP_invalid_op, regs) in <common/bug.c> as TRAP_invalid_op is x86-specific thereby BUG_DEBUGGER_TRAP_FATAL should be defined for each architecture. - add information about what do_bug_frame() returns. - invert the condition 'if ( region )' in do_bug_frame() to reduce the indention. - change type of variable i from 'unsigned int' to 'size_t' as it is compared with n_bugs which has type 'size_t' * xen/bug.h: - Introduce generic BUG_DEBUGGER_TRAP_FATAL(regs) mnacros which is used to deal with debugger_trap_fatal(TRAP_invalid_op, regs) where TRAP_invalid_op is x86-specific - remove '#include <xen/stringify.h>' as it doesn't need any more after switch to x86 implementation. - remove '#include <xen/errno.h>' as it isn't needed any more - move bug_*() macros inside '#ifndef BUG_FRAME_STRUCT' - add <xen/lib.h> to fix compile issue with BUILD_ON()... - Add documentation for BUG_ASM_CONST. * Update the commit message --- 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 | 103 +++++++++++++++++++++++++++ xen/include/xen/bug.h | 158 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 265 insertions(+) create mode 100644 xen/common/bug.c create mode 100644 xen/include/xen/bug.h