[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v8 5/6] xen/riscv: implement do_bug_frame()
On 11.08.2023 16:32, Oleksii Kurochko wrote: > The patch introduces the temporary implementation of do_bug_frame(). > When common code is merged, CONFIG_GENERIC_BUG_FRAME should > be used instead. > > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx> > --- > Changes in V8: > - remove Pointless initializer of id. > - make bug_frames[] array constant. > - remove cast_to_bug_frame(addr). > - rename is_valig_bugaddr to is_valid_bug_insn(). > - add check that read_instr is used only on xen code > - update the commit message. Looks largely okay to me now, yet still a couple of remarks / suggestions: > +int do_bug_frame(const struct cpu_user_regs *regs, vaddr_t pc) > +{ > + const struct bug_frame *start, *end; > + const struct bug_frame *bug = NULL; > + unsigned int id; > + const char *filename, *predicate; > + int lineno; > + > + static const struct bug_frame * const bug_frames[] = { > + &__start_bug_frames[0], > + &__stop_bug_frames_0[0], > + &__stop_bug_frames_1[0], > + &__stop_bug_frames_2[0], > + &__stop_bug_frames_3[0], > + }; > + > + for ( id = 0; id < BUGFRAME_NR; id++ ) > + { > + start = bug_frames[id]; > + end = bug_frames[id + 1]; > + > + while ( start != end ) > + { > + if ( (vaddr_t)bug_loc(start) == pc ) > + { > + bug = start; > + goto found; > + } > + > + start++; > + } > + } > + > + found: > + if ( bug == NULL ) > + return -ENOENT; > + > + if ( id == BUGFRAME_run_fn ) > + { > + void (*fn)(const struct cpu_user_regs *) = bug_ptr(bug); > + > + fn(regs); > + > + goto end; I'd like to suggest that you avoid "goto" when at the label there's just a single, simple statement (like return in the case here). > + } > + > + /* WARN, BUG or ASSERT: decode the filename pointer and line number. */ > + filename = bug_ptr(bug); > + lineno = bug_line(bug); > + > + switch ( id ) > + { > + case BUGFRAME_warn: > + printk("Xen WARN at %s:%d\n", filename, lineno); > + > + show_execution_state(regs); > + > + goto end; > + > + case BUGFRAME_bug: > + printk("Xen BUG at %s:%d\n", filename, lineno); > + > + show_execution_state(regs); > + > + printk("change wait_for_interrupt to panic() when common is > available\n"); > + die(); The log message here ... > + case BUGFRAME_assert: > + /* ASSERT: decode the predicate string pointer. */ > + predicate = bug_msg(bug); > + > + printk("Assertion %s failed at %s:%d\n", predicate, filename, > lineno); > + > + show_execution_state(regs); > + > + printk("change wait_for_interrupt to panic() when common is > available\n"); > + die(); ... and here doesn't really fit with the following code. There's no wait_for_interrupt() in sight. > + } > + > + return -EINVAL; > + > + end: > + return 0; > +} > + > +static bool is_valid_bug_insn(uint32_t insn) > +{ > + return insn == BUG_INSN_32 || > + (insn & COMPRESSED_INSN_MASK) == BUG_INSN_16; > +} > + > +/* There are no guests for now so the func has to return alwasys false */ > +static inline bool guest_mode(void) > +{ > + return false; > +} I think this makes little sense to have, and ... > +/* Should be used only on Xen code */ ... just the comment is good enough. First and foremost I don't think a function with name and purpose of the above can get away without any parameters. Furthermore, as said before, ... > +static uint32_t read_instr(unsigned long pc) > +{ > + uint16_t instr16 = *(uint16_t *)pc; ... such casting can't be legitimate for guest addresses anyway; you would need to map guest memory instead. What you might do ... > + if ( guest_mode() ) > + { > + printk("%s should be called only on Xen code\n", __func__); > + die(); > + } ... instead of this, as some minimalistic checking, is ASSERT(is_kernel_text(pc + 1) || is_kernel_inittext(pc + 1)); > + if ( GET_INSN_LENGTH(instr16) == 2 ) > + return instr16; > + else > + return *(uint32_t *)pc; and then if ( GET_INSN_LENGTH(instr16) == 2 ) return instr16; ASSERT(is_kernel_text(pc + 3) || is_kernel_inittext(pc + 3)); return *(uint32_t *)pc; Yet then again asserting in exception handlers is of limited use only, as then you risk recursive exceptions. > void do_trap(struct cpu_user_regs *cpu_regs) > { > + register_t pc = cpu_regs->sepc; > + uint32_t instr = read_instr(pc); > + > + if ( is_valid_bug_insn(instr) ) > + { > + if ( !do_bug_frame(cpu_regs, pc) ) Two if()-s like these typically want folding into one. Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |