| Message ID | bb1ebda319fc6f74456306a4051dd6da721bb797.1691763757.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | RISCV basic exception handling implementation | expand |
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@gmail.com> > --- > 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
diff --git a/xen/arch/riscv/traps.c b/xen/arch/riscv/traps.c index c1175668b6..b096dce6c7 100644 --- a/xen/arch/riscv/traps.c +++ b/xen/arch/riscv/traps.c @@ -5,6 +5,8 @@ * RISC-V Trap handlers */ +#include <xen/bug.h> +#include <xen/errno.h> #include <xen/lib.h> #include <asm/csr.h> @@ -101,7 +103,143 @@ static void do_unexpected_trap(const struct cpu_user_regs *regs) die(); } +void show_execution_state(const struct cpu_user_regs *regs) +{ + printk("implement show_execution_state(regs)\n"); +} + +/* + * TODO: generic do_bug_frame() should be used instead of current + * implementation panic(), printk() and find_text_region() + * (virtual memory?) will be ready/merged + */ +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; + } + + /* 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(); + + 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(); + } + + 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; +} + +/* Should be used only on Xen code */ +static uint32_t read_instr(unsigned long pc) +{ + uint16_t instr16 = *(uint16_t *)pc; + + if ( guest_mode() ) + { + printk("%s should be called only on Xen code\n", __func__); + die(); + } + + if ( GET_INSN_LENGTH(instr16) == 2 ) + return instr16; + else + return *(uint32_t *)pc; +} + 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) ) + { + cpu_regs->sepc += GET_INSN_LENGTH(instr); + return; + } + } + do_unexpected_trap(cpu_regs); } diff --git a/xen/arch/riscv/xen.lds.S b/xen/arch/riscv/xen.lds.S index 3fa7db3bf9..a10e0ad87c 100644 --- a/xen/arch/riscv/xen.lds.S +++ b/xen/arch/riscv/xen.lds.S @@ -45,6 +45,16 @@ SECTIONS . = ALIGN(PAGE_SIZE); .rodata : { _srodata = .; /* Read-only data */ + /* Bug frames table */ + __start_bug_frames = .; + *(.bug_frames.0) + __stop_bug_frames_0 = .; + *(.bug_frames.1) + __stop_bug_frames_1 = .; + *(.bug_frames.2) + __stop_bug_frames_2 = .; + *(.bug_frames.3) + __stop_bug_frames_3 = .; *(.rodata) *(.rodata.*) *(.data.rel.ro)
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@gmail.com> --- 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. --- Changes in V7: - move to this patch the definition of cast_to_bug_frame() from the previous patch. - update the comment in bug.h. - update the comment above do_bug_frame(). - fix code style. - add comment to read_instr func. - add space for bug_frames in lds.S. --- Changes in V6: - Avoid LINK_TO_LOAD() as bug.h functionality expected to be used after MMU is enabled. - Change early_printk() to printk() --- Changes in V5: - Remove "#include <xen/types.h>" from <asm/bug.h> as there is no any need in it anymore - Update macros GET_INSN_LENGTH: remove UL and 'unsigned int len;' from it - Remove " include <xen/bug.h>" from risc/setup.c. it is not needed in the current version of the patch - change an argument type from vaddr_t to uint32_t for is_valid_bugaddr and introduce read_instr() to read instruction properly as the length of qinstruction can be either 32 or 16 bits. - Code style fixes - update the comments before do_bug_frame() in riscv/trap.c - Refactor is_valid_bugaddr() function. - introduce macros cast_to_bug_frame(addr) to hide casts. - use LINK_TO_LOAD() for addresses which are linker time relative. --- Changes in V4: - Updates in RISC-V's <asm/bug.h>: * Add explanatory comment about why there is only defined for 32-bits length instructions and 16/32-bits BUG_INSN_{16,32}. * Change 'unsigned long' to 'unsigned int' inside GET_INSN_LENGTH(). * Update declaration of is_valid_bugaddr(): switch return type from int to bool and the argument from 'unsigned int' to 'vaddr'. - Updates in RISC-V's traps.c: * replace /xen and /asm includes * update definition of is_valid_bugaddr():switch return type from int to bool and the argument from 'unsigned int' to 'vaddr'. Code style inside function was updated too. * do_bug_frame() refactoring: * local variables start and bug became 'const struct bug_frame' * bug_frames[] array became 'static const struct bug_frame[] = ...' * remove all casts * remove unneeded comments and add an explanatory comment that the do_bug_frame() will be switched to a generic one. * do_trap() refactoring: * read 16-bits value instead of 32-bits as compressed instruction can be used and it might happen than only 16-bits may be accessible. * code style updates * re-use instr variable instead of re-reading instruction. - Updates in setup.c: * add blank line between xen/ and asm/ includes. --- Changes in V3: - Rebase the patch "xen/riscv: introduce an implementation of macros from <asm/bug.h>" on top of patch series [introduce generic implementation of macros from bug.h] --- Changes in V2: - Remove __ in define namings - Update run_in_exception_handler() with register void *fn_ asm(__stringify(BUG_FN_REG)) = (fn); - Remove bug_instr_t type and change it's usage to uint32_t --- xen/arch/riscv/traps.c | 138 +++++++++++++++++++++++++++++++++++++++ xen/arch/riscv/xen.lds.S | 10 +++ 2 files changed, 148 insertions(+)