| Message ID | 4a478638449e66a76e1671db38ec29b9e3108201.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: > The following changes were made: > * Make GENERIC_BUG_FRAME mandatory for X86 > * Update asm/bug.h using generic implementation in <xen/bug.h> > * Change prototype of debugger_trap_fatal() in asm/debugger.h > to align it with generic prototype in <xen/debugger.h>. Is this part of the description stale? The patch ... > --- > xen/arch/x86/Kconfig | 1 + > xen/arch/x86/include/asm/bug.h | 73 ++---------------------------- > xen/arch/x86/traps.c | 81 +++------------------------------- > 3 files changed, 11 insertions(+), 144 deletions(-) ... doesn't touch asm/debugger.h at all. > --- a/xen/arch/x86/include/asm/bug.h > +++ b/xen/arch/x86/include/asm/bug.h > @@ -1,79 +1,12 @@ > #ifndef __X86_BUG_H__ > #define __X86_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 BUG_DEBUGGER_TRAP_FATAL(regs) debugger_trap_fatal(X86_EXC_GP,regs) Along the lines of a comment on an earlier patch, please move this ... > #ifndef __ASSEMBLY__ ... into the thus guarded section. > @@ -1166,12 +1167,9 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf, > > void do_invalid_op(struct cpu_user_regs *regs) > { > - const struct bug_frame *bug = NULL; > u8 bug_insn[2]; > - const char *prefix = "", *filename, *predicate, *eip = (char *)regs->rip; > - unsigned long fixup; > - int id = -1, lineno; > - const struct virtual_region *region; > + const char *eip = (char *)regs->rip; I realize "char" was used before, but now that this is all on its own, can this at least become "unsigned char", but yet better "void"? > @@ -1185,83 +1183,18 @@ void do_invalid_op(struct cpu_user_regs *regs) > memcmp(bug_insn, "\xf\xb", sizeof(bug_insn)) ) > goto die; > > - region = find_text_region(regs->rip); > - 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) == eip ) > - { > - bug = b; > - goto found; > - } > - } > - } > - } > - > - found: > - if ( !bug ) > + id = do_bug_frame(regs, regs->rip); > + if ( id < 0 ) > goto die; > - eip += sizeof(bug_insn); > - if ( id == BUGFRAME_run_fn ) > - { > - void (*fn)(struct cpu_user_regs *) = bug_ptr(bug); > - > - fn(regs); > - fixup_exception_return(regs, (unsigned long)eip); > - return; > - } > > - /* WARN, BUG or ASSERT: decode the filename pointer and line number. */ > - filename = bug_ptr(bug); > - if ( !is_kernel(filename) && !is_patch(filename) ) > - goto die; > - fixup = strlen(filename); > - if ( fixup > 50 ) > - { > - filename += fixup - 47; > - prefix = "..."; > - } > - lineno = bug_line(bug); > + eip += sizeof(bug_insn); > > switch ( id ) > { > + case BUGFRAME_run_fn: > case BUGFRAME_warn: > - printk("Xen WARN at %s%s:%d\n", prefix, filename, lineno); > - show_execution_state(regs); > fixup_exception_return(regs, (unsigned long)eip); > return; > - > - case BUGFRAME_bug: > - printk("Xen BUG at %s%s:%d\n", prefix, filename, lineno); > - > - if ( debugger_trap_fatal(TRAP_invalid_op, regs) ) > - return; This and ... > - show_execution_state(regs); > - panic("Xen BUG at %s%s:%d\n", prefix, filename, lineno); > - > - case BUGFRAME_assert: > - /* ASSERT: decode the predicate string pointer. */ > - predicate = bug_msg(bug); > - if ( !is_kernel(predicate) && !is_patch(predicate) ) > - predicate = "<unknown>"; > - > - printk("Assertion '%s' failed at %s%s:%d\n", > - predicate, prefix, filename, lineno); > - > - if ( debugger_trap_fatal(TRAP_invalid_op, regs) ) > - return; ... this return look to have no proper representation in the new logic; both cases fall through to ... > - show_execution_state(regs); > - panic("Assertion '%s' failed at %s%s:%d\n", > - predicate, prefix, filename, lineno); > } > > die: ... here now, which is an unwanted functional change. Jan
On Mon, 2023-03-06 at 11:36 +0100, Jan Beulich wrote: > On 03.03.2023 11:38, Oleksii Kurochko wrote: > > The following changes were made: > > * Make GENERIC_BUG_FRAME mandatory for X86 > > * Update asm/bug.h using generic implementation in <xen/bug.h> > > * Change prototype of debugger_trap_fatal() in asm/debugger.h > > to align it with generic prototype in <xen/debugger.h>. > > Is this part of the description stale? The patch ... It is stale. Updated now. > > > --- > > xen/arch/x86/Kconfig | 1 + > > xen/arch/x86/include/asm/bug.h | 73 ++---------------------------- > > xen/arch/x86/traps.c | 81 +++--------------------------- > > ---- > > 3 files changed, 11 insertions(+), 144 deletions(-) > > ... doesn't touch asm/debugger.h at all. > > > --- a/xen/arch/x86/include/asm/bug.h > > +++ b/xen/arch/x86/include/asm/bug.h > > @@ -1,79 +1,12 @@ > > #ifndef __X86_BUG_H__ > > #define __X86_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 BUG_DEBUGGER_TRAP_FATAL(regs) > > debugger_trap_fatal(X86_EXC_GP,regs) > > Along the lines of a comment on an earlier patch, please move this > ... > > > #ifndef __ASSEMBLY__ > > ... into the thus guarded section. But it was defined there before the patch series and these definies are used in "#else /* !__ASSEMBLY__ */" > > > @@ -1166,12 +1167,9 @@ void cpuid_hypervisor_leaves(const struct > > vcpu *v, uint32_t leaf, > > > > void do_invalid_op(struct cpu_user_regs *regs) > > { > > - const struct bug_frame *bug = NULL; > > u8 bug_insn[2]; > > - const char *prefix = "", *filename, *predicate, *eip = (char > > *)regs->rip; > > - unsigned long fixup; > > - int id = -1, lineno; > > - const struct virtual_region *region; > > + const char *eip = (char *)regs->rip; > > I realize "char" was used before, but now that this is all on its > own, > can this at least become "unsigned char", but yet better "void"? If we change it to "void" shouldn't it require additional casts here ( which is not nice ): eip += sizeof(bug_insn); Probably 'unsgined char' will be better. > > > @@ -1185,83 +1183,18 @@ void do_invalid_op(struct cpu_user_regs > > *regs) > > memcmp(bug_insn, "\xf\xb", sizeof(bug_insn)) ) > > goto die; > > > > - region = find_text_region(regs->rip); > > - 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) == eip ) > > - { > > - bug = b; > > - goto found; > > - } > > - } > > - } > > - } > > - > > - found: > > - if ( !bug ) > > + id = do_bug_frame(regs, regs->rip); > > + if ( id < 0 ) > > goto die; > > - eip += sizeof(bug_insn); > > - if ( id == BUGFRAME_run_fn ) > > - { > > - void (*fn)(struct cpu_user_regs *) = bug_ptr(bug); > > - > > - fn(regs); > > - fixup_exception_return(regs, (unsigned long)eip); > > - return; > > - } > > > > - /* WARN, BUG or ASSERT: decode the filename pointer and line > > number. */ > > - filename = bug_ptr(bug); > > - if ( !is_kernel(filename) && !is_patch(filename) ) > > - goto die; > > - fixup = strlen(filename); > > - if ( fixup > 50 ) > > - { > > - filename += fixup - 47; > > - prefix = "..."; > > - } > > - lineno = bug_line(bug); > > + eip += sizeof(bug_insn); > > > > switch ( id ) > > { > > + case BUGFRAME_run_fn: > > case BUGFRAME_warn: > > - printk("Xen WARN at %s%s:%d\n", prefix, filename, lineno); > > - show_execution_state(regs); > > fixup_exception_return(regs, (unsigned long)eip); > > return; > > - > > - case BUGFRAME_bug: > > - printk("Xen BUG at %s%s:%d\n", prefix, filename, lineno); > > - > > - if ( debugger_trap_fatal(TRAP_invalid_op, regs) ) > > - return; > > This and ... > > > - show_execution_state(regs); > > - panic("Xen BUG at %s%s:%d\n", prefix, filename, lineno); > > - > > - case BUGFRAME_assert: > > - /* ASSERT: decode the predicate string pointer. */ > > - predicate = bug_msg(bug); > > - if ( !is_kernel(predicate) && !is_patch(predicate) ) > > - predicate = "<unknown>"; > > - > > - printk("Assertion '%s' failed at %s%s:%d\n", > > - predicate, prefix, filename, lineno); > > - > > - if ( debugger_trap_fatal(TRAP_invalid_op, regs) ) > > - return; > > ... this return look to have no proper representation in the new > logic; both cases fall through to ... > > > - show_execution_state(regs); > > - panic("Assertion '%s' failed at %s%s:%d\n", > > - predicate, prefix, filename, lineno); > > } > > > > die: > > ... here now, which is an unwanted functional change. Oh, you are right. So then in case we have correct id it is needed to do only return: switch ( id ) { case BUGFRAME_run_fn: case BUGFRAME_warn: fixup_exception_return(regs, (unsigned long)eip); break; } return; Thanks for finding that. ~ Oleksii
On 07.03.2023 13:52, Oleksii wrote: > On Mon, 2023-03-06 at 11:36 +0100, Jan Beulich wrote: >> On 03.03.2023 11:38, Oleksii Kurochko wrote: >>> --- a/xen/arch/x86/include/asm/bug.h >>> +++ b/xen/arch/x86/include/asm/bug.h >>> @@ -1,79 +1,12 @@ >>> #ifndef __X86_BUG_H__ >>> #define __X86_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 BUG_DEBUGGER_TRAP_FATAL(regs) >>> debugger_trap_fatal(X86_EXC_GP,regs) >> >> Along the lines of a comment on an earlier patch, please move this >> ... >> >>> #ifndef __ASSEMBLY__ >> >> ... into the thus guarded section. > But it was defined there before the patch series and these definies are > used in "#else /* !__ASSEMBLY__ */" Since you use plural, maybe a misunderstanding? My comment was solely on the line you add; the removed lines are there merely as context. >>> @@ -1166,12 +1167,9 @@ void cpuid_hypervisor_leaves(const struct >>> vcpu *v, uint32_t leaf, >>> >>> void do_invalid_op(struct cpu_user_regs *regs) >>> { >>> - const struct bug_frame *bug = NULL; >>> u8 bug_insn[2]; >>> - const char *prefix = "", *filename, *predicate, *eip = (char >>> *)regs->rip; >>> - unsigned long fixup; >>> - int id = -1, lineno; >>> - const struct virtual_region *region; >>> + const char *eip = (char *)regs->rip; >> >> I realize "char" was used before, but now that this is all on its >> own, >> can this at least become "unsigned char", but yet better "void"? > If we change it to "void" shouldn't it require additional casts here ( > which is not nice ): > eip += sizeof(bug_insn); Arithmetic on pointers to void is a GNU extension which we make heavy use of all over the hypervisor. >>> switch ( id ) >>> { >>> + case BUGFRAME_run_fn: >>> case BUGFRAME_warn: >>> - printk("Xen WARN at %s%s:%d\n", prefix, filename, lineno); >>> - show_execution_state(regs); >>> fixup_exception_return(regs, (unsigned long)eip); >>> return; >>> - >>> - case BUGFRAME_bug: >>> - printk("Xen BUG at %s%s:%d\n", prefix, filename, lineno); >>> - >>> - if ( debugger_trap_fatal(TRAP_invalid_op, regs) ) >>> - return; >> >> This and ... >> >>> - show_execution_state(regs); >>> - panic("Xen BUG at %s%s:%d\n", prefix, filename, lineno); >>> - >>> - case BUGFRAME_assert: >>> - /* ASSERT: decode the predicate string pointer. */ >>> - predicate = bug_msg(bug); >>> - if ( !is_kernel(predicate) && !is_patch(predicate) ) >>> - predicate = "<unknown>"; >>> - >>> - printk("Assertion '%s' failed at %s%s:%d\n", >>> - predicate, prefix, filename, lineno); >>> - >>> - if ( debugger_trap_fatal(TRAP_invalid_op, regs) ) >>> - return; >> >> ... this return look to have no proper representation in the new >> logic; both cases fall through to ... >> >>> - show_execution_state(regs); >>> - panic("Assertion '%s' failed at %s%s:%d\n", >>> - predicate, prefix, filename, lineno); >>> } >>> >>> die: >> >> ... here now, which is an unwanted functional change. > Oh, you are right. So then in case we have correct id it is needed to > do only return: > switch ( id ) > { > case BUGFRAME_run_fn: > case BUGFRAME_warn: > fixup_exception_return(regs, (unsigned long)eip); > break; > } > > return; Except the "return" needs to remain inside the switch; unrecognized id values should still fall through to the "die" label. Jan
On Tue, 2023-03-07 at 13:59 +0100, Jan Beulich wrote: > On 07.03.2023 13:52, Oleksii wrote: > > On Mon, 2023-03-06 at 11:36 +0100, Jan Beulich wrote: > > > On 03.03.2023 11:38, Oleksii Kurochko wrote: > > > > --- a/xen/arch/x86/include/asm/bug.h > > > > +++ b/xen/arch/x86/include/asm/bug.h > > > > @@ -1,79 +1,12 @@ > > > > #ifndef __X86_BUG_H__ > > > > #define __X86_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 BUG_DEBUGGER_TRAP_FATAL(regs) > > > > debugger_trap_fatal(X86_EXC_GP,regs) > > > > > > Along the lines of a comment on an earlier patch, please move > > > this > > > ... > > > > > > > #ifndef __ASSEMBLY__ > > > > > > ... into the thus guarded section. > > But it was defined there before the patch series and these definies > > are > > used in "#else /* !__ASSEMBLY__ */" > > Since you use plural, maybe a misunderstanding? My comment was solely > on the line you add; the removed lines are there merely as context. Really. I misunderstood you. > > > > > @@ -1166,12 +1167,9 @@ void cpuid_hypervisor_leaves(const > > > > struct > > > > vcpu *v, uint32_t leaf, > > > > > > > > void do_invalid_op(struct cpu_user_regs *regs) > > > > { > > > > - const struct bug_frame *bug = NULL; > > > > u8 bug_insn[2]; > > > > - const char *prefix = "", *filename, *predicate, *eip = > > > > (char > > > > *)regs->rip; > > > > - unsigned long fixup; > > > > - int id = -1, lineno; > > > > - const struct virtual_region *region; > > > > + const char *eip = (char *)regs->rip; > > > > > > I realize "char" was used before, but now that this is all on its > > > own, > > > can this at least become "unsigned char", but yet better "void"? > > If we change it to "void" shouldn't it require additional casts > > here ( > > which is not nice ): > > eip += sizeof(bug_insn); > > Arithmetic on pointers to void is a GNU extension which we make > heavy use of all over the hypervisor. Got it. Than I'll rework it with 'void *'. > > > > > switch ( id ) > > > > { > > > > + case BUGFRAME_run_fn: > > > > case BUGFRAME_warn: > > > > - printk("Xen WARN at %s%s:%d\n", prefix, filename, > > > > lineno); > > > > - show_execution_state(regs); > > > > fixup_exception_return(regs, (unsigned long)eip); > > > > return; > > > > - > > > > - case BUGFRAME_bug: > > > > - printk("Xen BUG at %s%s:%d\n", prefix, filename, > > > > lineno); > > > > - > > > > - if ( debugger_trap_fatal(TRAP_invalid_op, regs) ) > > > > - return; > > > > > > This and ... > > > > > > > - show_execution_state(regs); > > > > - panic("Xen BUG at %s%s:%d\n", prefix, filename, > > > > lineno); > > > > - > > > > - case BUGFRAME_assert: > > > > - /* ASSERT: decode the predicate string pointer. */ > > > > - predicate = bug_msg(bug); > > > > - if ( !is_kernel(predicate) && !is_patch(predicate) ) > > > > - predicate = "<unknown>"; > > > > - > > > > - printk("Assertion '%s' failed at %s%s:%d\n", > > > > - predicate, prefix, filename, lineno); > > > > - > > > > - if ( debugger_trap_fatal(TRAP_invalid_op, regs) ) > > > > - return; > > > > > > ... this return look to have no proper representation in the new > > > logic; both cases fall through to ... > > > > > > > - show_execution_state(regs); > > > > - panic("Assertion '%s' failed at %s%s:%d\n", > > > > - predicate, prefix, filename, lineno); > > > > } > > > > > > > > die: > > > > > > ... here now, which is an unwanted functional change. > > Oh, you are right. So then in case we have correct id it is needed > > to > > do only return: > > switch ( id ) > > { > > case BUGFRAME_run_fn: > > case BUGFRAME_warn: > > fixup_exception_return(regs, (unsigned long)eip); > > break; > > } > > > > return; > > Except the "return" needs to remain inside the switch; unrecognized > id > values should still fall through to the "die" label. Yeah, it is still possible to get unrecognized id. Thanks. > ~ Oleksii
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig index 6a7825f4ba..b0ff1f3ee6 100644 --- a/xen/arch/x86/Kconfig +++ b/xen/arch/x86/Kconfig @@ -11,6 +11,7 @@ config X86 select ARCH_MAP_DOMAIN_PAGE select ARCH_SUPPORTS_INT128 select CORE_PARKING + select GENERIC_BUG_FRAME select HAS_ALTERNATIVE select HAS_COMPAT select HAS_CPUFREQ diff --git a/xen/arch/x86/include/asm/bug.h b/xen/arch/x86/include/asm/bug.h index 79133e53e7..2a8d3f84b8 100644 --- a/xen/arch/x86/include/asm/bug.h +++ b/xen/arch/x86/include/asm/bug.h @@ -1,79 +1,12 @@ #ifndef __X86_BUG_H__ #define __X86_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 BUG_DEBUGGER_TRAP_FATAL(regs) debugger_trap_fatal(X86_EXC_GP,regs) #ifndef __ASSEMBLY__ -#define 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) ((const void *)(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]) - -#define _ASM_BUGFRAME_TEXT(second_frame) \ - ".Lbug%=: ud2\n" \ - ".pushsection .bug_frames.%c[bf_type], \"a\", @progbits\n" \ - ".p2align 2\n" \ - ".Lfrm%=:\n" \ - ".long (.Lbug%= - .Lfrm%=) + %c[bf_line_hi]\n" \ - ".long (%c[bf_ptr] - .Lfrm%=) + %c[bf_line_lo]\n" \ - ".if " #second_frame "\n" \ - ".long 0, %c[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) - -#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) - - -#define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, 0, NULL) -#define BUG() do { \ - BUG_FRAME(BUGFRAME_bug, __LINE__, __FILE__, 0, NULL); \ - unreachable(); \ -} while (0) - -/* - * 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 ) - -#define assert_failed(msg) do { \ - BUG_FRAME(BUGFRAME_assert, __LINE__, __FILE__, 1, msg); \ - unreachable(); \ -} while (0) +#define BUG_INSTR "ud2" +#define BUG_ASM_CONST "c" #else /* !__ASSEMBLY__ */ diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c index cade9e12f8..7c33ebceff 100644 --- a/xen/arch/x86/traps.c +++ b/xen/arch/x86/traps.c @@ -24,6 +24,7 @@ * Gareth Hughes <gareth@valinux.com>, May 2000 */ +#include <xen/bug.h> #include <xen/init.h> #include <xen/sched.h> #include <xen/lib.h> @@ -1166,12 +1167,9 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf, void do_invalid_op(struct cpu_user_regs *regs) { - const struct bug_frame *bug = NULL; u8 bug_insn[2]; - const char *prefix = "", *filename, *predicate, *eip = (char *)regs->rip; - unsigned long fixup; - int id = -1, lineno; - const struct virtual_region *region; + const char *eip = (char *)regs->rip; + int id; if ( likely(guest_mode(regs)) ) { @@ -1185,83 +1183,18 @@ void do_invalid_op(struct cpu_user_regs *regs) memcmp(bug_insn, "\xf\xb", sizeof(bug_insn)) ) goto die; - region = find_text_region(regs->rip); - 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) == eip ) - { - bug = b; - goto found; - } - } - } - } - - found: - if ( !bug ) + id = do_bug_frame(regs, regs->rip); + if ( id < 0 ) goto die; - eip += sizeof(bug_insn); - if ( id == BUGFRAME_run_fn ) - { - void (*fn)(struct cpu_user_regs *) = bug_ptr(bug); - - fn(regs); - fixup_exception_return(regs, (unsigned long)eip); - return; - } - /* WARN, BUG or ASSERT: decode the filename pointer and line number. */ - filename = bug_ptr(bug); - if ( !is_kernel(filename) && !is_patch(filename) ) - goto die; - fixup = strlen(filename); - if ( fixup > 50 ) - { - filename += fixup - 47; - prefix = "..."; - } - lineno = bug_line(bug); + eip += sizeof(bug_insn); switch ( id ) { + case BUGFRAME_run_fn: case BUGFRAME_warn: - printk("Xen WARN at %s%s:%d\n", prefix, filename, lineno); - show_execution_state(regs); fixup_exception_return(regs, (unsigned long)eip); return; - - case BUGFRAME_bug: - printk("Xen BUG at %s%s:%d\n", prefix, filename, lineno); - - if ( debugger_trap_fatal(TRAP_invalid_op, regs) ) - return; - - show_execution_state(regs); - panic("Xen BUG at %s%s:%d\n", prefix, filename, lineno); - - case BUGFRAME_assert: - /* ASSERT: decode the predicate string pointer. */ - predicate = bug_msg(bug); - if ( !is_kernel(predicate) && !is_patch(predicate) ) - predicate = "<unknown>"; - - printk("Assertion '%s' failed at %s%s:%d\n", - predicate, prefix, filename, lineno); - - if ( debugger_trap_fatal(TRAP_invalid_op, regs) ) - return; - - show_execution_state(regs); - panic("Assertion '%s' failed at %s%s:%d\n", - predicate, prefix, filename, lineno); } die:
The following changes were made: * Make GENERIC_BUG_FRAME mandatory for X86 * Update asm/bug.h using generic implementation in <xen/bug.h> * Change prototype of debugger_trap_fatal() in asm/debugger.h to align it with generic prototype in <xen/debugger.h>. * Update do_invalid_op using generic do_bug_frame() Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- Changes in V5: * Nothing changed --- Changes in V4: * Back comment /* !__ASSEMBLY__ */ for #else case in <asm/bug.h> * Remove changes related to x86/.../asm/debuger.h as do_bug_frame() prototype was updated and cpu_user_regs isn't const any more. --- Changes in V3: * As prototype and what do_bug_frame() returns was changed so patch 3 and 4 was updated to use a new version of do_bug_frame * MODIFIER was change to BUG_ASM_CONST to align with generic implementation --- Changes in V2: * Remove all unnecessary things from <asm/bug.h> as they were introduced in <xen/bug.h>. * Define BUG_INSTR = 'ud2' and MODIFIER = 'c' ( it is needed to skip '$' when use an imidiate in x86 assembly ) * Update do_invalid_op() to re-use handle_bug_frame() and find_bug_frame() from generic implemetation of CONFIG_GENERIC_BUG_FRAME * Code style fixes. --- xen/arch/x86/Kconfig | 1 + xen/arch/x86/include/asm/bug.h | 73 ++---------------------------- xen/arch/x86/traps.c | 81 +++------------------------------- 3 files changed, 11 insertions(+), 144 deletions(-)