[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v3 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME
On 28.02.2023 11:30, Oleksii wrote: > On Mon, 2023-02-27 at 15:23 +0100, Jan Beulich wrote: >> On 24.02.2023 12:31, Oleksii Kurochko wrote: >>> --- /dev/null >>> +++ b/xen/common/bug.c >>> @@ -0,0 +1,109 @@ >>> +#include <xen/bug.h> >>> +#include <xen/debugger.h> >>> +#include <xen/errno.h> >>> +#include <xen/kernel.h> >>> +#include <xen/livepatch.h> >>> +#include <xen/string.h> >>> +#include <xen/types.h> >>> +#include <xen/virtual_region.h> >>> + >>> +#include <asm/processor.h> >>> + >>> +/* Set default value for TRAP_invalid_op as it is defined only for >>> X86 now */ >>> +#ifndef TRAP_invalid_op >>> +#define TRAP_invalid_op 0 >>> +#endif >>> + >>> +int do_bug_frame(const struct cpu_user_regs *regs, unsigned long >>> pc) >>> +{ >>> + const struct bug_frame *bug = NULL; >>> + const struct virtual_region *region; >>> + const char *prefix = "", *filename, *predicate; >>> + unsigned long fixup; >>> + unsigned int id = BUGFRAME_NR, lineno; >>> + >>> + region = find_text_region(pc); >>> + if ( region ) >>> + { >>> + for ( id = 0; id < BUGFRAME_NR; id++ ) >>> + { >>> + const struct bug_frame *b; >>> + unsigned int i; >>> + >>> + for ( i = 0, b = region->frame[id].bugs; >>> + i < region->frame[id].n_bugs; b++, i++ ) >>> + { >>> + if ( bug_loc(b) == pc ) >>> + { >>> + bug = b; >>> + goto found; >>> + } >>> + } >>> + } >>> + } >>> + >>> + found: >>> + if ( !bug ) >>> + return -EINVAL; >>> + >>> + if ( id == BUGFRAME_run_fn ) >>> + { >>> +#ifdef BUG_FN_REG >>> + void (*fn)(const struct cpu_user_regs *) = (void *)regs- >>>> BUG_FN_REG; >>> +#else >>> + void (*fn)(const struct cpu_user_regs *) = bug_ptr(bug); >>> +#endif >>> + >>> + fn(regs); >>> + >>> + return id; >>> + } >>> + >>> + /* WARN, BUG or ASSERT: decode the filename pointer and line >>> number. */ >>> + filename = bug_ptr(bug); >>> + if ( !is_kernel(filename) && !is_patch(filename) ) >>> + return -EINVAL; >>> + fixup = strlen(filename); >>> + if ( fixup > 50 ) >>> + { >>> + filename += fixup - 47; >>> + prefix = "..."; >>> + } >>> + lineno = bug_line(bug); >>> + >>> + switch ( id ) >>> + { >>> + case BUGFRAME_warn: >>> + printk("Xen WARN at %s%s:%d\n", prefix, filename, lineno); >>> + show_execution_state(regs); >>> + >>> + return id; >>> + >>> + case BUGFRAME_bug: >>> + printk("Xen BUG at %s%s:%d\n", prefix, filename, lineno); >>> + >>> + if ( debugger_trap_fatal(TRAP_invalid_op, regs) ) >> >> TRAP_invalid_op is, as said, about to disappear on x86 as well. I >> think >> this construct wants abstracting by another asm/bug.h provided macro >> (taking just regs). >> > Thanks for the link. > > Nice idea to abstract 'debugger_trap_fatal(TRAP_invalid_op, regs)'. > Actually we have to options here: > 1. As you proposed abstract in <asm/bug.h>: > x86: #define DEBUG_TRAP_FATAL(regs) debugger_trap_fatal(X86_EXC_GP, > regs) > ARM: #define DEBUG_TRAP_FATAL(regs) 0 > RISC-V: #define DEBUG_TRAP_FATAL(regs) 0 > For ARM and RISC-V it doesn't use so we can skip the check if ( > DEBUG_TRAP_FATAL ). > > 2. Abstract only TRAP_invalid_op in <asm/bug.h> > x86: #define TRAP_invalud_op X86_EXC_GP > RISC-V: #define TRAP_invalid_op 0 > ARN: #define TRAP_invalid_op 0 > > I am not sure if we have to provide real invalid opcodes for RISC-V > and ARM as it looks like debug_trap_fatal() isn't used in ARM&RISC-V > now. > > Could you please suggest which one option is better? I don't view 2 as a viable option. How an arch deals with invalid opcodes is entirely arch-specific (including the naming). As to 1 - since we want this solely for bug.c, I'd prefer if the wrapper macro's name would start with BUG_, e.g. BUG_DEBUGGER_TRAP_FATAL() or BUG_TRAP_FATAL() or just BUG_FATAL(). Further adding ARCH_ may also be wanted by other maintainers (I'm neither pro nor con there). Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |