| Message ID | 9fdda7716faf412f1e2cdf9a990c98e64c4b21f3.1691063432.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | RISCV basic exception handling implementation | expand |
On 03.08.2023 14:05, Oleksii Kurochko wrote: > --- 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> > @@ -12,6 +14,8 @@ > #include <asm/processor.h> > #include <asm/traps.h> > > +#define cast_to_bug_frame(addr) ((const struct bug_frame *)(addr)) > + > /* > * Initialize the trap handling. > * > @@ -101,7 +105,131 @@ 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 = 0; Pointless initializer. > + const char *filename, *predicate; > + int lineno; > + > + static const struct bug_frame *bug_frames[] = { You likely want another const here. > + &__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 = cast_to_bug_frame(bug_frames[id]); > + end = cast_to_bug_frame(bug_frames[id + 1]); Why these casts (and then even hidden in a macro)? The array elements look to already be of appropriate type. > + 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_bugaddr(uint32_t insn) > +{ > + return insn == BUG_INSN_32 || > + (insn & COMPRESSED_INSN_MASK) == BUG_INSN_16; > +} Why "addr" in the name when this takes an insn as argument? > +/* Should be used only in Xen code ? */ What is this question about? With ... > +static uint32_t read_instr(unsigned long pc) > +{ > + uint16_t instr16 = *(uint16_t *)pc; > + > + if ( GET_INSN_LENGTH(instr16) == 2 ) > + return (uint32_t)instr16; (I don't think this cast is needed.) > + else > + return *(uint32_t *)pc; > +} ... there still being a double read here, do you perhaps mean to make a statement (that this code isn't safe to use on guest code)? > void do_trap(struct cpu_user_regs *cpu_regs) > { > + register_t pc = cpu_regs->sepc; > + uint32_t instr = read_instr(pc); > + > + if ( is_valid_bugaddr(instr) ) > + { > + if ( !do_bug_frame(cpu_regs, pc) ) > + { > + cpu_regs->sepc += GET_INSN_LENGTH(instr); > + return; > + } > + } > + > do_unexpected_trap(cpu_regs); > }
On Mon, 2023-08-07 at 15:29 +0200, Jan Beulich wrote: > On 03.08.2023 14:05, Oleksii Kurochko wrote: > > --- 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> > > @@ -12,6 +14,8 @@ > > #include <asm/processor.h> > > #include <asm/traps.h> > > > > +#define cast_to_bug_frame(addr) ((const struct bug_frame *)(addr)) > > + > > /* > > * Initialize the trap handling. > > * > > @@ -101,7 +105,131 @@ 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 = 0; > > Pointless initializer. Agree. Thanks. I'll remove it. > > > + const char *filename, *predicate; > > + int lineno; > > + > > + static const struct bug_frame *bug_frames[] = { > > You likely want another const here. Yes, I will add it. > > > + &__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 = cast_to_bug_frame(bug_frames[id]); > > + end = cast_to_bug_frame(bug_frames[id + 1]); > > Why these casts (and then even hidden in a macro)? The array elements > look to already be of appropriate type. There is no any sense for these casts. It looks like that before bug_frames array has a different type. > > > + 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_bugaddr(uint32_t insn) > > +{ > > + return insn == BUG_INSN_32 || > > + (insn & COMPRESSED_INSN_MASK) == BUG_INSN_16; > > +} > > Why "addr" in the name when this takes an insn as argument? In the earliest patch series it was an address. But now it should be changed. Thanks. > > > +/* Should be used only in Xen code ? */ > > What is this question about? With ... I meant that it's not safe to use in guest code. > > > +static uint32_t read_instr(unsigned long pc) > > +{ > > + uint16_t instr16 = *(uint16_t *)pc; > > + > > + if ( GET_INSN_LENGTH(instr16) == 2 ) > > + return (uint32_t)instr16; > > (I don't think this cast is needed.) > > > + else > > + return *(uint32_t *)pc; > > +} > > ... there still being a double read here, do you perhaps mean to > make a statement (that this code isn't safe to use on guest code)? I wonder if it'll be safe to read 16 bytes at a time then we won't have double read ( if you meant that first 16 bytes are read twice ): static uint32_t read_instr(unsigned long pc) { uint16_t instr16 = *(uint16_t *)pc; if ( GET_INSN_LENGTH(instr16) == 2 ) return (uint32_t)instr16; else{ // return *(uint32_t *)pc; uint16_t next_16 = *((uint16_t *)pc + 1); return ((uint32_t)instr16 << sizeof(instr16)) + next_16; } } > > > void do_trap(struct cpu_user_regs *cpu_regs) > > { > > + register_t pc = cpu_regs->sepc; > > + uint32_t instr = read_instr(pc); > > + > > + if ( is_valid_bugaddr(instr) ) > > + { > > + if ( !do_bug_frame(cpu_regs, pc) ) > > + { > > + cpu_regs->sepc += GET_INSN_LENGTH(instr); > > + return; > > + } > > + } > > + > > do_unexpected_trap(cpu_regs); > > } > ~ Oleksii
On 08.08.2023 10:48, Oleksii wrote: > On Mon, 2023-08-07 at 15:29 +0200, Jan Beulich wrote: >> On 03.08.2023 14:05, Oleksii Kurochko wrote: >>> +static uint32_t read_instr(unsigned long pc) >>> +{ >>> + uint16_t instr16 = *(uint16_t *)pc; >>> + >>> + if ( GET_INSN_LENGTH(instr16) == 2 ) >>> + return (uint32_t)instr16; >> >> (I don't think this cast is needed.) >> >>> + else >>> + return *(uint32_t *)pc; >>> +} >> >> ... there still being a double read here, do you perhaps mean to >> make a statement (that this code isn't safe to use on guest code)? > I wonder if it'll be safe to read 16 bytes at a time then we won't have > double read ( if you meant that first 16 bytes are read twice ): > > static uint32_t read_instr(unsigned long pc) > { > uint16_t instr16 = *(uint16_t *)pc; > > if ( GET_INSN_LENGTH(instr16) == 2 ) > return (uint32_t)instr16; > else{ > // return *(uint32_t *)pc; > > uint16_t next_16 = *((uint16_t *)pc + 1); > return ((uint32_t)instr16 << sizeof(instr16)) + next_16; > } > } Whether this is safe for guest code depends further on what underlying mappings there are. Surely you can't simply cast a guest add (coming in as "unsigned long pc") to a hypervisor address. So as it stands the function can only ever be used on Xen code anyway. Jan
diff --git a/xen/arch/riscv/include/asm/bug.h b/xen/arch/riscv/include/asm/bug.h index e8b1e40823..f5ff96140f 100644 --- a/xen/arch/riscv/include/asm/bug.h +++ b/xen/arch/riscv/include/asm/bug.h @@ -7,4 +7,31 @@ #ifndef _ASM_RISCV_BUG_H #define _ASM_RISCV_BUG_H +#ifndef __ASSEMBLY__ + +#define BUG_INSTR "ebreak" + +/* + * The base instruction set has a fixed length of 32-bit naturally aligned + * instructions. + * + * There are extensions of variable length ( where each instruction can be + * any number of 16-bit parcels in length ). + * + * Compressed ISA is used now where the instruction length is 16 bit and + * 'ebreak' instruction, in this case, can be either 16 or 32 bit ( + * depending on if compressed ISA is used or not ) + */ +#define INSN_LENGTH_MASK _UL(0x3) +#define INSN_LENGTH_32 _UL(0x3) + +#define BUG_INSN_32 _UL(0x00100073) /* ebreak */ +#define BUG_INSN_16 _UL(0x9002) /* c.ebreak */ +#define COMPRESSED_INSN_MASK _UL(0xffff) + +#define GET_INSN_LENGTH(insn) \ + (((insn) & INSN_LENGTH_MASK) == INSN_LENGTH_32 ? 4 : 2) \ + +#endif /* !__ASSEMBLY__ */ + #endif /* _ASM_RISCV_BUG_H */ diff --git a/xen/arch/riscv/traps.c b/xen/arch/riscv/traps.c index 3e08ec44f7..439c098c22 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> @@ -12,6 +14,8 @@ #include <asm/processor.h> #include <asm/traps.h> +#define cast_to_bug_frame(addr) ((const struct bug_frame *)(addr)) + /* * Initialize the trap handling. * @@ -101,7 +105,131 @@ 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 = 0; + const char *filename, *predicate; + int lineno; + + static const struct bug_frame *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 = cast_to_bug_frame(bug_frames[id]); + end = cast_to_bug_frame(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_bugaddr(uint32_t insn) +{ + return insn == BUG_INSN_32 || + (insn & COMPRESSED_INSN_MASK) == BUG_INSN_16; +} + +/* Should be used only in Xen code ? */ +static uint32_t read_instr(unsigned long pc) +{ + uint16_t instr16 = *(uint16_t *)pc; + + if ( GET_INSN_LENGTH(instr16) == 2 ) + return (uint32_t)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_bugaddr(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 macros: BUG(), WARN(), run_in_exception(), assert_failed. To be precise, the macros from generic bug implementation (<xen/bug.h>) will be used. The implementation uses "ebreak" instruction in combination with diffrent bug frame tables (for each type) which contains useful information. Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- 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/include/asm/bug.h | 27 +++++++ xen/arch/riscv/traps.c | 128 +++++++++++++++++++++++++++++++ xen/arch/riscv/xen.lds.S | 10 +++ 3 files changed, 165 insertions(+)