| Message ID | aa0d3704921f5ec04b66c8aa935638a64018c50b.1676909088.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | introduce generic implementation of macros from bug.h | expand |
On 20.02.2023 17:40, 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 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. Hmm, below I see it's really just "MODIFIER". I see two issues with this: For one the name is too generic for something which cannot be #undef-ed after use inside the header. And then it also doesn't really say what is being modified. While ending up longer, how about BUG_ASM_CONST or alike? I also think this should default to something if not overridden by an arch. Perhaps simply to an expansion to nothing (at which point you won't need to define it for RISC-V, aiui). > --- /dev/null > +++ b/xen/common/bug.c > @@ -0,0 +1,113 @@ > +#include <xen/bug.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> Is this really needed here? > +const struct bug_frame* find_bug_frame(unsigned long pc, unsigned int *id) Is this function going to be needed outside of this CU? IOW why is it not static? Also, nit: Left most star wants changing places with the adjacent blank. > +{ > + const struct bug_frame *bug = NULL; > + const struct virtual_region *region; > + > + 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; While in the original code the goto is kind of warranted, it isn't really here imo. You can simply "return b" here and ... > + } > + } > + } > + } > + > + found: > + return bug; ... "return NULL" here. That'll allow the function scope "bug" to go away, at which point the inner scope "b" can become "bug". > +} > + > +int handle_bug_frame(const struct cpu_user_regs *regs, > + const struct bug_frame *bug, > + unsigned int id) Nit: Indentation is off by one here. Also same question regarding the lack of static here. > +{ > + const char *prefix = "", *filename, *predicate; > + unsigned long fixup; > + unsigned int lineno; > + > + if ( id == BUGFRAME_run_fn ) > + { > +#ifdef ARM Who or what defines ARM? Anyway, seeing ... > + void (*fn)(const struct cpu_user_regs *) = (void *)regs->BUG_FN_REG; ... this, wouldn't it be better (and independent of the specific arch) if you checked for BUG_FN_REG being defined? Another (#ifdef-free) variant would be to have bug_ptr() take a 2nd argument and then uniformly use ... > +#else > + void (*fn)(const struct cpu_user_regs *) = bug_ptr(bug); ... this, slightly altered to void (*fn)(const struct cpu_user_regs *) = bug_ptr(bug, regs); > +#endif > + > + fn(regs); > + return 0; > + } > + > + /* 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 0; > + > + case BUGFRAME_bug: > + printk("Xen BUG at %s%s:%d\n", prefix, filename, lineno); > + > + 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) ) > + predicate = "<unknown>"; > + > + printk("Assertion '%s' failed at %s%s:%d\n", > + predicate, prefix, filename, lineno); > + > + show_execution_state(regs); > + panic("Assertion '%s' failed at %s%s:%d\n", > + predicate, prefix, filename, lineno); > + } > + > + return -EINVAL; > +} > + > +int do_bug_frame(const struct cpu_user_regs *regs, unsigned long pc) > +{ > + const struct bug_frame *bug = NULL; Nit: pointless initializer. You could of course have the assignment below become the initializer here. > + unsigned int id; > + > + bug = find_bug_frame(pc, &id); > + if (!bug) Nit: Style (missing blanks). > --- /dev/null > +++ b/xen/include/xen/bug.h > @@ -0,0 +1,161 @@ > +#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 > + > +#ifndef __ASSEMBLY__ > + > +#include <xen/errno.h> > +#include <xen/lib.h> > +#include <xen/stringify.h> > +#include <xen/types.h> > + > +#include <asm/bug.h> Any reason this cannot live ahead of the #ifndef, eliminating the need for an #else further down? > +#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 /* bug_loc */ For short #if / #ifdef I don't think such comments are necessary on the #else or #endif; personally I consider such to hamper readability. > +#ifndef bug_ptr > +#define bug_ptr(b) ((const void *)(b) + (b)->ptr_disp) > +#endif /* bug_ptr */ > + > +#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 /* bug_line */ > + > +#ifndef bug_msg > +#define bug_msg(b) ((const char *)(b) + (b)->msg_disp[1]) > +#endif /* bug_msg */ > + > +#ifndef _ASM_BUGFRAME_TEXT > + > +#define _ASM_BUGFRAME_TEXT(second_frame) \ > + ".Lbug%=:"BUG_INSTR"\n" \ > + ".pushsection .bug_frames.%"MODIFIER"[bf_type], \"a\", @progbits\n" \ You may want to use %progbits here right away, as being the more portable form. > + ".p2align 2\n" \ > + ".Lfrm%=:\n" \ > + ".long (.Lbug%= - .Lfrm%=) + %"MODIFIER"[bf_line_hi]\n" \ > + ".long (%"MODIFIER"[bf_ptr] - .Lfrm%=) + %"MODIFIER"[bf_line_lo]\n" \ > + ".if " #second_frame "\n" \ > + ".long 0, %"MODIFIER"[bf_msg] - .Lfrm%=\n" \ > + ".endif\n" \ > + ".popsection\n" I think I said so in reply to an earlier version already: The moment assembly code moves to a common header, it should be adjusted to be as portable as possible. In particular directives should never start at the beginning of a line, while labels always should. (Whether .long is actually portable is another question, which I'll be happy to leave aside for now.) Also nit (style): The line continuation characters want to all line up. > +#endif /* _ASM_BUGFRAME_TEXT */ > + > +#ifndef _ASM_BUGFRAME_INFO I don't think these two make sense for an arch to define independently. INFO absolutely has to match TEXT, so I think an arch should always define (or not) both together. > +#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_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(). > + */ I realize you only copy this comment, but I'm having a hard time seeing the connection to BUILD_BUG_ON() here. Would be nice if the comment was "generalized" in a form that actually can be understood. Andrew? > +#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 /* WARN */ No real need for the comment here; you also have none below for e.g. BUG(). Jan
Hello Jan, On Wed, 2023-02-22 at 13:46 +0100, Jan Beulich wrote: > On 20.02.2023 17:40, 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 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. > > Hmm, below I see it's really just "MODIFIER". I see two issues with > this: > For one the name is too generic for something which cannot be #undef- > ed > after use inside the header. And then it also doesn't really say what > is > being modified. While ending up longer, how about BUG_ASM_CONST or > alike? BUG_ASM_CONST will be much better. > > I also think this should default to something if not overridden by an > arch. Perhaps simply to an expansion to nothing (at which point you > won't need to define it for RISC-V, aiui). > Agree. Initially, I thought about that too but couldn't estimate how well the modifier '%c' is supported, deciding that each architecture has to define it. But we can make it equal to nothing (at least for 2 architectures ) it doesn't have the same support as in x86. > > --- /dev/null > > +++ b/xen/common/bug.c > > @@ -0,0 +1,113 @@ > > +#include <xen/bug.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> > > Is this really needed here? Yes, it is. Function show_execution_state() is declared in this header for all architectures and is used by handle_bug_frame(). > > > +const struct bug_frame* find_bug_frame(unsigned long pc, unsigned > > int *id) > > Is this function going to be needed outside of this CU? IOW why is it > not > static? > It's not static because there is not possible to use do_bug_frame() as is for x86 as x86 has some additional checks for some cases which aren't in generic implementation: [1] https://gitlab.com/xen-project/people/olkur/xen/-/blob/staging/xen/arch/x86/traps.c#L1217 [2] https://gitlab.com/xen-project/people/olkur/xen/-/blob/staging/xen/arch/x86/traps.c#L1238 [3] https://gitlab.com/xen-project/people/olkur/xen/-/blob/staging/xen/arch/x86/traps.c#L1259 Probably to make generic do_bug_frame() more re-usable for x86 we can implement as stubs fixup_exception_return() and debugger_trap_fatal() under #ifndef X86 ... #endif inside common/bug.c. Could you please share your thoughts about that? > Also, nit: Left most star wants changing places with the adjacent > blank. Thanks. I'll update in the next version of the patch series. > > > +{ > > + const struct bug_frame *bug = NULL; > > + const struct virtual_region *region; > > + > > + 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; > > While in the original code the goto is kind of warranted, it isn't > really > here imo. You can simply "return b" here and ... > > > + } > > + } > > + } > > + } > > + > > + found: > > + return bug; > > ... "return NULL" here. That'll allow the function scope "bug" to go > away, > at which point the inner scope "b" can become "bug". Agree, missed that when decided to move that part of the code to separate function. > > > +} > > + > > +int handle_bug_frame(const struct cpu_user_regs *regs, > > + const struct bug_frame *bug, > > + unsigned int id) > > Nit: Indentation is off by one here. Also same question regarding the > lack > of static here. Regarding static an answer is the same as with previous one static question. > > > +{ > > + const char *prefix = "", *filename, *predicate; > > + unsigned long fixup; > > + unsigned int lineno; > > + > > + if ( id == BUGFRAME_run_fn ) > > + { > > +#ifdef ARM > > Who or what defines ARM? Anyway, seeing ... it is defined by default in Kconfig: https://gitlab.com/xen-project/people/olkur/xen/-/blob/staging/xen/arch/arm/Kconfig#L13 > > > + void (*fn)(const struct cpu_user_regs *) = (void *)regs- > > >BUG_FN_REG; > > ... this, wouldn't it be better (and independent of the specific > arch) if > you checked for BUG_FN_REG being defined? If I understand Kconfig correctly than there is no significant difference what check. But probably BUG_FN_REG would be a little bit better if someone will decide to change a way how pointer to function will be passed in case of ARM than we will get compilation error and so won't miss to fix the line in do_bug_frame(). > > Another (#ifdef-free) variant would be to have bug_ptr() take a 2nd > argument > and then uniformly use ... > > > +#else > > + void (*fn)(const struct cpu_user_regs *) = bug_ptr(bug); > > ... this, slightly altered to > > void (*fn)(const struct cpu_user_regs *) = bug_ptr(bug, > regs); Probably this one option will be the most universal and we have to stick to it. > > > +#endif > > + > > + fn(regs); > > + return 0; > > + } > > + > > + /* 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 0; > > + > > + case BUGFRAME_bug: > > + printk("Xen BUG at %s%s:%d\n", prefix, filename, lineno); > > + > > + 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) ) > > + predicate = "<unknown>"; > > + > > + printk("Assertion '%s' failed at %s%s:%d\n", > > + predicate, prefix, filename, lineno); > > + > > + show_execution_state(regs); > > + panic("Assertion '%s' failed at %s%s:%d\n", > > + predicate, prefix, filename, lineno); > > + } > > + > > + return -EINVAL; > > +} > > + > > +int do_bug_frame(const struct cpu_user_regs *regs, unsigned long > > pc) > > +{ > > + const struct bug_frame *bug = NULL; > > Nit: pointless initializer. You could of course have the assignment > below > become the initializer here. Thanks. I'll update it the next version of the patch. > > > + unsigned int id; > > + > > + bug = find_bug_frame(pc, &id); > > + if (!bug) > > Nit: Style (missing blanks). Thanks. I'll update it the next version of the patch. > > > --- /dev/null > > +++ b/xen/include/xen/bug.h > > @@ -0,0 +1,161 @@ > > +#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 > > + > > +#ifndef __ASSEMBLY__ > > + > > +#include <xen/errno.h> > > +#include <xen/lib.h> > > +#include <xen/stringify.h> > > +#include <xen/types.h> > > + > > +#include <asm/bug.h> > > Any reason this cannot live ahead of the #ifndef, eliminating the > need for > an #else further down? It should be fine. Probably I had some issues during the initial stage of making bug.h more generic... > > > +#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 /* bug_loc */ > > For short #if / #ifdef I don't think such comments are necessary on > the > #else or #endif; personally I consider such to hamper readability. Thanks. I'll take it into account. > > > +#ifndef bug_ptr > > +#define bug_ptr(b) ((const void *)(b) + (b)->ptr_disp) > > +#endif /* bug_ptr */ > > + > > +#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 /* bug_line */ > > + > > +#ifndef bug_msg > > +#define bug_msg(b) ((const char *)(b) + (b)->msg_disp[1]) > > +#endif /* bug_msg */ > > + > > +#ifndef _ASM_BUGFRAME_TEXT > > + > > +#define > > _ASM_BUGFRAME_TEXT(second_frame) > > \ > > + > > ".Lbug%=:"BUG_INSTR"\n" > > \ > > + ".pushsection .bug_frames.%"MODIFIER"[bf_type], \"a\", > > @progbits\n" \ > > You may want to use %progbits here right away, as being the more > portable > form. Sure. Thanks. I'll take that into account. > > > + ".p2align > > 2\n" \ > > + > > ".Lfrm%=:\n" > > \ > > + ".long (.Lbug%= - .Lfrm%=) + > > %"MODIFIER"[bf_line_hi]\n" \ > > + ".long (%"MODIFIER"[bf_ptr] - .Lfrm%=) + > > %"MODIFIER"[bf_line_lo]\n" \ > > + ".if " #second_frame > > "\n" \ > > + ".long 0, %"MODIFIER"[bf_msg] - > > .Lfrm%=\n" \ > > + > > ".endif\n" > > \ > > + ".popsection\n" > > I think I said so in reply to an earlier version already: The moment > assembly code moves to a common header, it should be adjusted to be > as > portable as possible. In particular directives should never start at > the > beginning of a line, while labels always should. (Whether .long is > actually portable is another question, which I'll be happy to leave > aside for now.) I am not sure that I understand about which one directive we are talking about... Are we talking about .{push/pop}section and .p2align? Probably you can show me an example in Xen or other project? > > Also nit (style): The line continuation characters want to all line > up. > > > +#endif /* _ASM_BUGFRAME_TEXT */ > > + > > +#ifndef _ASM_BUGFRAME_INFO > > I don't think these two make sense for an arch to define > independently. > INFO absolutely has to match TEXT, so I think an arch should always > define (or not) both together. You are right. I'll take into account that. > > > +#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_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(). > > + */ > > I realize you only copy this comment, but I'm having a hard time > seeing > the connection to BUILD_BUG_ON() here. Would be nice if the comment > was > "generalized" in a form that actually can be understood. Andrew? > > > +#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 /* WARN */ > > No real need for the comment here; you also have none below for e.g. > BUG(). Thanks. > > Jan ~ Oleksii
On 22.02.2023 17:16, Oleksii wrote: > On Wed, 2023-02-22 at 13:46 +0100, Jan Beulich wrote: >> On 20.02.2023 17:40, Oleksii Kurochko wrote: >>> --- /dev/null >>> +++ b/xen/common/bug.c >>> @@ -0,0 +1,113 @@ >>> +#include <xen/bug.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> >> >> Is this really needed here? > Yes, it is. > > Function show_execution_state() is declared in this header for all > architectures and is used by handle_bug_frame(). Ugly, but yes, you're right. >>> +const struct bug_frame* find_bug_frame(unsigned long pc, unsigned >>> int *id) >> >> Is this function going to be needed outside of this CU? IOW why is it >> not >> static? >> > It's not static because there is not possible to use do_bug_frame() as > is for x86 as x86 has some additional checks for some cases which > aren't in generic implementation: > [1] > https://gitlab.com/xen-project/people/olkur/xen/-/blob/staging/xen/arch/x86/traps.c#L1217 > [2] > https://gitlab.com/xen-project/people/olkur/xen/-/blob/staging/xen/arch/x86/traps.c#L1238 > [3] > https://gitlab.com/xen-project/people/olkur/xen/-/blob/staging/xen/arch/x86/traps.c#L1259 > > Probably to make generic do_bug_frame() more re-usable for x86 we can > implement as stubs fixup_exception_return() and debugger_trap_fatal() > under #ifndef X86 ... #endif inside common/bug.c. > > Could you please share your thoughts about that? Isn't all that's needed a suitable return value from the function, for the caller to take appropriate further action? (Maybe even that isn't really necessary.) That said, debugger_trap_fatal() imo may sensibly be generalized, and hence could be left in common code. Arm may simply stub this to nothing then for the time being. >>> +{ >>> + const char *prefix = "", *filename, *predicate; >>> + unsigned long fixup; >>> + unsigned int lineno; >>> + >>> + if ( id == BUGFRAME_run_fn ) >>> + { >>> +#ifdef ARM >> >> Who or what defines ARM? Anyway, seeing ... > it is defined by default in Kconfig: > https://gitlab.com/xen-project/people/olkur/xen/-/blob/staging/xen/arch/arm/Kconfig#L13 That'll be CONFIG_ARM then in uses in C code. >>> + void (*fn)(const struct cpu_user_regs *) = (void *)regs- >>>> BUG_FN_REG; >> >> ... this, wouldn't it be better (and independent of the specific >> arch) if >> you checked for BUG_FN_REG being defined? > If I understand Kconfig correctly than there is no significant > difference what check. But probably BUG_FN_REG would be a little bit > better if someone will decide to change a way how pointer to function > will be passed in case of ARM than we will get compilation error and so > won't miss to fix the line in do_bug_frame(). Indeed - #ifdef like this one generally want to check for the precise aspect in question, without making assumptions about something implying something else. (Pretty certainly there are exceptions to this rule, but it holds here.) >>> + ".p2align >>> 2\n" \ >>> + >>> ".Lfrm%=:\n" >>> \ >>> + ".long (.Lbug%= - .Lfrm%=) + >>> %"MODIFIER"[bf_line_hi]\n" \ >>> + ".long (%"MODIFIER"[bf_ptr] - .Lfrm%=) + >>> %"MODIFIER"[bf_line_lo]\n" \ >>> + ".if " #second_frame >>> "\n" \ >>> + ".long 0, %"MODIFIER"[bf_msg] - >>> .Lfrm%=\n" \ >>> + >>> ".endif\n" >>> \ >>> + ".popsection\n" >> >> I think I said so in reply to an earlier version already: The moment >> assembly code moves to a common header, it should be adjusted to be >> as >> portable as possible. In particular directives should never start at >> the >> beginning of a line, while labels always should. (Whether .long is >> actually portable is another question, which I'll be happy to leave >> aside for now.) > I am not sure that I understand about which one directive we are > talking about... Are we talking about .{push/pop}section and .p2align? > Probably you can show me an example in Xen or other project? I'm talking about all directives here. Fundamentally assembly language source lines are like this (assuming colons follow labels): [<label>:|<blank>][<directive>|<insn>] Both parts can optionally be empty, but if the right one isn't, then the left one also shouldn't be (and hence minimally a blank is needed; commonly it would be a tab). Directives, unlike insns, start with a dot in most dialects. Labels can also start with a dot, but are disambiguated by the colon after them (yet more strict placement of items is therefore required when labels are not followed by colons - there are such dialects -, which is why it is generally a good idea to follow that simple formatting rule). Also, ftaod, - <insn> covers both actual insns as well as assembler macro invocations, - there can of course be multiple labels on a single line (iirc this requires that colons follow labels). As to examples: I'm afraid I'm unaware of arch-independent assembly code anywhere in Xen so far. Jan
On Thu, 2023-02-23 at 11:11 +0100, Jan Beulich wrote: > On 22.02.2023 17:16, Oleksii wrote: > > On Wed, 2023-02-22 at 13:46 +0100, Jan Beulich wrote: > > > On 20.02.2023 17:40, Oleksii Kurochko wrote: > > > > --- /dev/null > > > > +++ b/xen/common/bug.c > > > > @@ -0,0 +1,113 @@ > > > > +#include <xen/bug.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> > > > > > > Is this really needed here? > > Yes, it is. > > > > Function show_execution_state() is declared in this header for all > > architectures and is used by handle_bug_frame(). > > Ugly, but yes, you're right. > > > > > +const struct bug_frame* find_bug_frame(unsigned long pc, > > > > unsigned > > > > int *id) > > > > > > Is this function going to be needed outside of this CU? IOW why > > > is it > > > not > > > static? > > > > > It's not static because there is not possible to use do_bug_frame() > > as > > is for x86 as x86 has some additional checks for some cases which > > aren't in generic implementation: > > [1] > > https://gitlab.com/xen-project/people/olkur/xen/-/blob/staging/xen/arch/x86/traps.c#L1217 > > [2] > > https://gitlab.com/xen-project/people/olkur/xen/-/blob/staging/xen/arch/x86/traps.c#L1238 > > [3] > > https://gitlab.com/xen-project/people/olkur/xen/-/blob/staging/xen/arch/x86/traps.c#L1259 > > > > Probably to make generic do_bug_frame() more re-usable for x86 we > > can > > implement as stubs fixup_exception_return() and > > debugger_trap_fatal() > > under #ifndef X86 ... #endif inside common/bug.c. > > > > Could you please share your thoughts about that? > > Isn't all that's needed a suitable return value from the function, > for the caller to take appropriate further action? (Maybe even that > isn't really necessary.) > > That said, debugger_trap_fatal() imo may sensibly be generalized, > and hence could be left in common code. Arm may simply stub this to > nothing then for the time being. I checked the source code I found that debugger_trap_fatal() is generalized: https://gitlab.com/xen-project/people/olkur/xen/-/blob/staging/xen/include/xen/debugger.h So we can use in generic version of do_bug_frame(). About fixup_exception_return() you are right as we can it handle(call) by the caller so it's needed only to return a suitable return value for do_bug_frame(). > > > > > +{ > > > > + const char *prefix = "", *filename, *predicate; > > > > + unsigned long fixup; > > > > + unsigned int lineno; > > > > + > > > > + if ( id == BUGFRAME_run_fn ) > > > > + { > > > > +#ifdef ARM > > > > > > Who or what defines ARM? Anyway, seeing ... > > it is defined by default in Kconfig: > > https://gitlab.com/xen-project/people/olkur/xen/-/blob/staging/xen/arch/arm/Kconfig#L13 > > That'll be CONFIG_ARM then in uses in C code. Ahh, yeah. I am always missing CONFIG_... > > > > > + void (*fn)(const struct cpu_user_regs *) = (void > > > > *)regs- > > > > > BUG_FN_REG; > > > > > > ... this, wouldn't it be better (and independent of the specific > > > arch) if > > > you checked for BUG_FN_REG being defined? > > If I understand Kconfig correctly than there is no significant > > difference what check. But probably BUG_FN_REG would be a little > > bit > > better if someone will decide to change a way how pointer to > > function > > will be passed in case of ARM than we will get compilation error > > and so > > won't miss to fix the line in do_bug_frame(). > > Indeed - #ifdef like this one generally want to check for the precise > aspect in question, without making assumptions about something > implying > something else. (Pretty certainly there are exceptions to this rule, > but it holds here.) Thanks for the explanation. > > > > > + ".p2align > > > > 2\n" \ > > > > + > > > > ".Lfrm%=:\n" > > > > > > > > \ > > > > + ".long (.Lbug%= - .Lfrm%=) + > > > > %"MODIFIER"[bf_line_hi]\n" \ > > > > + ".long (%"MODIFIER"[bf_ptr] - .Lfrm%=) + > > > > %"MODIFIER"[bf_line_lo]\n" \ > > > > + ".if " #second_frame > > > > "\n" \ > > > > + ".long 0, %"MODIFIER"[bf_msg] - > > > > .Lfrm%=\n" \ > > > > + > > > > ".endif\n" > > > > > > > > \ > > > > + ".popsection\n" > > > > > > I think I said so in reply to an earlier version already: The > > > moment > > > assembly code moves to a common header, it should be adjusted to > > > be > > > as > > > portable as possible. In particular directives should never start > > > at > > > the > > > beginning of a line, while labels always should. (Whether .long > > > is > > > actually portable is another question, which I'll be happy to > > > leave > > > aside for now.) > > I am not sure that I understand about which one directive we are > > talking about... Are we talking about .{push/pop}section and > > .p2align? > > Probably you can show me an example in Xen or other project? > > I'm talking about all directives here. Fundamentally assembly > language > source lines are like this (assuming colons follow labels): > > [<label>:|<blank>][<directive>|<insn>] > > Both parts can optionally be empty, but if the right one isn't, then > the left one also shouldn't be (and hence minimally a blank is > needed; > commonly it would be a tab). Directives, unlike insns, start with a > dot in most dialects. Labels can also start with a dot, but are > disambiguated by the colon after them (yet more strict placement of > items is therefore required when labels are not followed by colons - > there are such dialects -, which is why it is generally a good idea > to follow that simple formatting rule). Also, ftaod, > - <insn> covers both actual insns as well as assembler macro > invocations, > - there can of course be multiple labels on a single line (iirc this > requires that colons follow labels). > > As to examples: I'm afraid I'm unaware of arch-independent assembly > code anywhere in Xen so far. Thank you very much for the explanation. > > Jan ~ Oleksii
> > > + void (*fn)(const struct cpu_user_regs *) = (void *)regs- > > >BUG_FN_REG; > > ... this, wouldn't it be better (and independent of the specific > arch) if > you checked for BUG_FN_REG being defined? > > Another (#ifdef-free) variant would be to have bug_ptr() take a 2nd > argument > and then uniformly use ... > > > +#else > > + void (*fn)(const struct cpu_user_regs *) = bug_ptr(bug); > > ... this, slightly altered to > > void (*fn)(const struct cpu_user_regs *) = bug_ptr(bug, > regs); I think that I will go with BUG_FN_REG instead of changing bug_ptr()'s arguments as bug_ptr() is used below to get file name so it won't be clear what bug_ptr() should return either an address of file name or regs->BUG_FN_REG. > > > +#endif > > + > > + fn(regs); > > + return 0; > > + } > > + > > + /* WARN, BUG or ASSERT: decode the filename pointer and line > > number. */ > > + filename = bug_ptr(bug); > > + if ( !is_kernel(filename) && !is_patch(filename) ) > ~ Oleksii
On 23.02.2023 14:16, Oleksii wrote: >> >>> + void (*fn)(const struct cpu_user_regs *) = (void *)regs- >>>> BUG_FN_REG; >> >> ... this, wouldn't it be better (and independent of the specific >> arch) if >> you checked for BUG_FN_REG being defined? >> >> Another (#ifdef-free) variant would be to have bug_ptr() take a 2nd >> argument >> and then uniformly use ... >> >>> +#else >>> + void (*fn)(const struct cpu_user_regs *) = bug_ptr(bug); >> >> ... this, slightly altered to >> >> void (*fn)(const struct cpu_user_regs *) = bug_ptr(bug, >> regs); > I think that I will go with BUG_FN_REG instead of changing bug_ptr()'s > arguments as bug_ptr() is used below to get file name so it won't be > clear what bug_ptr() should return either an address of file name or > regs->BUG_FN_REG. Oh, indeed - I'm sorry that I didn't pay attention to ... >>> +#endif >>> + >>> + fn(regs); >>> + return 0; >>> + } >>> + >>> + /* WARN, BUG or ASSERT: decode the filename pointer and line >>> number. */ >>> + filename = bug_ptr(bug); >>> + if ( !is_kernel(filename) && !is_patch(filename) ) ... this 2nd use. Jan
On 20.02.2023 17:40, Oleksii Kurochko wrote: > --- /dev/null > +++ b/xen/include/xen/bug.h > @@ -0,0 +1,161 @@ > +#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 > + > +#ifndef __ASSEMBLY__ > + > +#include <xen/errno.h> > +#include <xen/lib.h> > +#include <xen/stringify.h> > +#include <xen/types.h> > + > +#include <asm/bug.h> Looking at patch 2 onwards I came to wonder: How does that work without you first removing stuff from the respective asm/bug.h (or adding suitable #define-s to limit what gets defined below)? From all I can tell the compiler should object to ... > +#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[]; > +}; ... this, as asm/bug.h will have declared such a struct already. The #define-s further down may not be a problem if what they expand to matches in both places, but that's then still a latent trap to fall into. Jan
On Thu, 2023-02-23 at 14:32 +0100, Jan Beulich wrote: > On 20.02.2023 17:40, Oleksii Kurochko wrote: > > --- /dev/null > > +++ b/xen/include/xen/bug.h > > @@ -0,0 +1,161 @@ > > +#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 > > + > > +#ifndef __ASSEMBLY__ > > + > > +#include <xen/errno.h> > > +#include <xen/lib.h> > > +#include <xen/stringify.h> > > +#include <xen/types.h> > > + > > +#include <asm/bug.h> > > Looking at patch 2 onwards I came to wonder: How does that work > without > you first removing stuff from the respective asm/bug.h (or adding > suitable #define-s to limit what gets defined below)? From all I can > tell the compiler should object to ... > > > +#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[]; > > +}; > > ... this, as asm/bug.h will have declared such a struct already. The > #define-s further down may not be a problem if what they expand to > matches in both places, but that's then still a latent trap to fall > into. My fault. It doesn't work. I checked only RISC-V arch before and didn't start all the test for patch 2... So yeah, in patch 2 should be updated asm/bug.h headers with define BUG_FRAME_STRUCT and remove all common parts [ BUG_DISP_WIDTH, BUG_LINE_LO_WIDTH, BUG_LINE_HI_WIDTH, BUGFRAME_run_fn, BUGFRAME_warn, BUGFRAME_bug, BUGFRAME_assert, BUGFRAME_NR, {__start|__stop}_bug_frames{ | _{0-3} }[], ]. Thanks for noticing that. > > Jan ~ 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..5eab3ebb53 --- /dev/null +++ b/xen/common/bug.c @@ -0,0 +1,113 @@ +#include <xen/bug.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> + +const struct bug_frame* find_bug_frame(unsigned long pc, unsigned int *id) +{ + const struct bug_frame *bug = NULL; + const struct virtual_region *region; + + 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: + return bug; +} + +int handle_bug_frame(const struct cpu_user_regs *regs, + const struct bug_frame *bug, + unsigned int id) +{ + const char *prefix = "", *filename, *predicate; + unsigned long fixup; + unsigned int lineno; + + if ( id == BUGFRAME_run_fn ) + { +#ifdef ARM + 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 0; + } + + /* 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 0; + + case BUGFRAME_bug: + printk("Xen BUG at %s%s:%d\n", prefix, filename, lineno); + + 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) ) + predicate = "<unknown>"; + + printk("Assertion '%s' failed at %s%s:%d\n", + predicate, prefix, filename, lineno); + + show_execution_state(regs); + panic("Assertion '%s' failed at %s%s:%d\n", + predicate, prefix, filename, lineno); + } + + return -EINVAL; +} + +int do_bug_frame(const struct cpu_user_regs *regs, unsigned long pc) +{ + const struct bug_frame *bug = NULL; + unsigned int id; + + bug = find_bug_frame(pc, &id); + if (!bug) + return -ENOENT; + + return handle_bug_frame(regs, bug, id); +} + diff --git a/xen/include/xen/bug.h b/xen/include/xen/bug.h new file mode 100644 index 0000000000..4aedebeb18 --- /dev/null +++ b/xen/include/xen/bug.h @@ -0,0 +1,161 @@ +#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 + +#ifndef __ASSEMBLY__ + +#include <xen/errno.h> +#include <xen/lib.h> +#include <xen/stringify.h> +#include <xen/types.h> + +#include <asm/bug.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 /* bug_loc */ + +#ifndef bug_ptr +#define bug_ptr(b) ((const void *)(b) + (b)->ptr_disp) +#endif /* bug_ptr */ + +#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 /* bug_line */ + +#ifndef bug_msg +#define bug_msg(b) ((const char *)(b) + (b)->msg_disp[1]) +#endif /* bug_msg */ + +#ifndef _ASM_BUGFRAME_TEXT + +#define _ASM_BUGFRAME_TEXT(second_frame) \ + ".Lbug%=:"BUG_INSTR"\n" \ + ".pushsection .bug_frames.%"MODIFIER"[bf_type], \"a\", @progbits\n" \ + ".p2align 2\n" \ + ".Lfrm%=:\n" \ + ".long (.Lbug%= - .Lfrm%=) + %"MODIFIER"[bf_line_hi]\n" \ + ".long (%"MODIFIER"[bf_ptr] - .Lfrm%=) + %"MODIFIER"[bf_line_lo]\n" \ + ".if " #second_frame "\n" \ + ".long 0, %"MODIFIER"[bf_msg] - .Lfrm%=\n" \ + ".endif\n" \ + ".popsection\n" + +#endif /* _ASM_BUGFRAME_TEXT */ + +#ifndef _ASM_BUGFRAME_INFO + +#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_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 /* WARN */ + +#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; + +const struct bug_frame* find_bug_frame(unsigned long pc, unsigned int *id); + +int handle_bug_frame(const struct cpu_user_regs *regs, + const struct bug_frame *bug, + unsigned int id); + +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[]; + +#else /* !__ASSEMBLY__ */ + +#include <asm/bug.h> + +#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 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 | 113 +++++++++++++++++++++++++++++ xen/include/xen/bug.h | 161 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 278 insertions(+) create mode 100644 xen/common/bug.c create mode 100644 xen/include/xen/bug.h