| Message ID | bd2dd42c778714f25e7e98f74ff5e98eee1cd0a5.1685359848.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | RISCV basic exception handling implementation | expand |
On 29.05.2023 14:13, Oleksii Kurochko wrote: > --- a/xen/arch/riscv/include/asm/bug.h > +++ b/xen/arch/riscv/include/asm/bug.h > @@ -7,4 +7,32 @@ > #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 ) but they aren't used in Xen > + * and Linux kernel ( where these definitions were taken from ). This, at least to some degree, looks to contradict ... > + * 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 ) ... this. Plus there already is CONFIG_RISCV_ISA_C, so compressed insns can very well be used in Xen. > @@ -114,7 +116,134 @@ 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: change early_printk's function to early_printk with format > + * when s(n)printf() will be added. What is this comment about? I don't think I understand what it says needs doing. > + * Probably the TODO won't be needed as generic do_bug_frame() > + * has been introduced and current implementation will be replaced > + * with generic one when 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) While it's going to be the maintainers to judge, I continue to be unconvinced that introducing copies of common functions (also in patch 1) is a good idea. > +{ > + 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[] = { Nit: * and blank want to swap places. I would also expect another "const". > +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; > +} As long as this function is only used on Xen code, it's kind of okay. There you/we control whether code can change behind our backs. But as soon as you might use this on guest code, the double read is going to be a problem (I think; I wonder how hardware is supposed to deal with the situation: Maybe they indeed fetch in 16-bit quantities?). > --- a/xen/arch/riscv/xen.lds.S > +++ b/xen/arch/riscv/xen.lds.S > @@ -40,6 +40,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) Nit: There looks to be an off-by-one in how you indent your addition (except for the comment). Jan
On Tue, 2023-05-30 at 18:00 +0200, Jan Beulich wrote: > On 29.05.2023 14:13, Oleksii Kurochko wrote: > > --- a/xen/arch/riscv/include/asm/bug.h > > +++ b/xen/arch/riscv/include/asm/bug.h > > @@ -7,4 +7,32 @@ > > #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 ) but they aren't used > > in Xen > > + * and Linux kernel ( where these definitions were taken from ). > > This, at least to some degree, looks to contradict ... > > > + * 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 ) > > ... this. Plus there already is CONFIG_RISCV_ISA_C, so compressed > insns > can very well be used in Xen. Thanks. You are right. The comment should be updated. > > > @@ -114,7 +116,134 @@ 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: change early_printk's function to early_printk with > > format > > + * when s(n)printf() will be added. > > What is this comment about? I don't think I understand what it says > needs doing. I meant that it would be nice to introduce the second version of early_printk() function which will take 'format', as printk() does. But there is no any sense in this comment because all early_printk() in do_bug_frame() were changed to printk(). Thereby I will update the comment. > > > + * Probably the TODO won't be needed as generic do_bug_frame() > > + * has been introduced and current implementation will be replaced > > + * with generic one when 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) > > While it's going to be the maintainers to judge, I continue to be > unconvinced that introducing copies of common functions (also in > patch 1) is a good idea. Generally I agree with you but as I mentioned before and in the comment above the function do_bug_frame() the reason not to use generic implementation of do_bug_frame() now as it will require to introduce compilation of whole Xen's common code. ( there is no way to enable just necessary parts for the current one function ). I think that after this patch series I'll introduce compilation of Xen's common code and after it'll be merged do_bug_frame() can be removed. > > > +{ > > + 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[] = { > > Nit: * and blank want to swap places. I would also expect another > "const". Thanks. I'll update that. > > > +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; > > +} > > As long as this function is only used on Xen code, it's kind of okay. > There you/we control whether code can change behind our backs. But as > soon as you might use this on guest code, the double read is going to > be a problem (I think; I wonder how hardware is supposed to deal with > the situation: Maybe they indeed fetch in 16-bit quantities?). I'll check how the hardware fetches instructions. I am trying to figure out why the double-read can be a problem. It looks pretty safe to read 16 bits ( they will be available for any instruction length with the assumption that the minimal instruction length is 16 ), then check the length of the instruction, and if it is 32-bit instruction, read it as uint32_t. > > > --- a/xen/arch/riscv/xen.lds.S > > +++ b/xen/arch/riscv/xen.lds.S > > @@ -40,6 +40,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) > > Nit: There looks to be an off-by-one in how you indent your addition > (except for the comment). Thanks. One space is really absent... ~ Oleksii
On 31.05.2023 12:40, Oleksii wrote: > On Tue, 2023-05-30 at 18:00 +0200, Jan Beulich wrote: >> On 29.05.2023 14:13, 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; >>> + else >>> + return *(uint32_t *)pc; >>> +} >> >> As long as this function is only used on Xen code, it's kind of okay. >> There you/we control whether code can change behind our backs. But as >> soon as you might use this on guest code, the double read is going to >> be a problem (I think; I wonder how hardware is supposed to deal with >> the situation: Maybe they indeed fetch in 16-bit quantities?). > I'll check how the hardware fetches instructions. > > I am trying to figure out why the double-read can be a problem. It > looks pretty safe to read 16 bits ( they will be available for any > instruction length with the assumption that the minimal instruction > length is 16 ), then check the length of the instruction, and if it is > 32-bit instruction, read it as uint32_t. Simply consider what happens if a buggy or malicious entity changes the code between the two reads. And not just with the detection of "break" in mind that you use it for here. Jan
On 31.05.2023 22:06, Oleksii wrote: > On Tue, 2023-05-30 at 18:00 +0200, Jan Beulich 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; >>> + else >>> + return *(uint32_t *)pc; >>> +} >> >> As long as this function is only used on Xen code, it's kind of okay. >> There you/we control whether code can change behind our backs. But as >> soon as you might use this on guest code, the double read is going to >> be a problem > Will it be enough to add a comment that read_instr() should be used > only on Xen code? Or it is needed to introduce some lock? A comment will do for now. A lock would be problematic: It won't help when the function is used on non-Xen code, and since you use this in exception handling you may deadlock unless you carefully use a recursive lock. >> (I think; I wonder how hardware is supposed to deal with >> the situation: Maybe they indeed fetch in 16-bit quantities?). > I thought that it reads amount of bytes corresponded to i-cache size > and then the pipeline tracks whether an instruction is 16 or 32 bit. And what if an insn spans a cacheline boundary? Jan
On Thu, 2023-06-01 at 09:59 +0200, Jan Beulich wrote: > On 31.05.2023 22:06, Oleksii wrote: > > On Tue, 2023-05-30 at 18:00 +0200, Jan Beulich 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; > > > > + else > > > > + return *(uint32_t *)pc; > > > > +} > > > > > > As long as this function is only used on Xen code, it's kind of > > > okay. > > > There you/we control whether code can change behind our backs. > > > But as > > > soon as you might use this on guest code, the double read is > > > going to > > > be a problem > > Will it be enough to add a comment that read_instr() should be used > > only on Xen code? Or it is needed to introduce some lock? > > A comment will do for now. A lock would be problematic: It won't help > when the function is used on non-Xen code, and since you use this in > exception handling you may deadlock unless you carefully use a > recursive lock. Then I'll add a comment. > > > > (I think; I wonder how hardware is supposed to deal with > > > the situation: Maybe they indeed fetch in 16-bit quantities?). > > I thought that it reads amount of bytes corresponded to i-cache > > size > > and then the pipeline tracks whether an instruction is 16 or 32 > > bit. > > And what if an insn spans a cacheline boundary? I think it is CPU specific, but your original assumption ( about 16-bit fetching ) was probably right. In RISC-V ISA doc [1] I found the following in chapter 1.2: The base RISC-V ISA has fixed-length 32-bit instructions that must be naturally aligned on 32-bit boundaries. However, the standard RISC-V encoding scheme is designed to support ISA extensions with variable- length instructions, where each instruction can be any number of 16-bit instruction parcels in length, and parcels are naturally aligned on 16- bit boundaries. The standard compressed ISA extension described in Chapter 12 reduces code size by providing compressed 16-bit instructions and relaxes the alignment constraints to allow all instructions (16 bit and 32 bit) to be aligned on any 16-bit boundary to improve code density. It sounds like h/w reads 16-bit and then based on the first bits decides if it is needed to read more 16-bit parcels. [1] https://riscv.org/wp-content/uploads/2017/05/riscv-spec-v2.2.pdf
diff --git a/xen/arch/riscv/include/asm/bug.h b/xen/arch/riscv/include/asm/bug.h index e8b1e40823..bf3194443f 100644 --- a/xen/arch/riscv/include/asm/bug.h +++ b/xen/arch/riscv/include/asm/bug.h @@ -7,4 +7,32 @@ #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 ) but they aren't used in Xen + * and Linux kernel ( where these definitions were taken from ). + * + * 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 48c1059954..535fb058e1 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> @@ -114,7 +116,134 @@ 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: change early_printk's function to early_printk with format + * when s(n)printf() will be added. + * + * Probably the TODO won't be needed as generic do_bug_frame() + * has been introduced and current implementation will be replaced + * with generic one when 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; +} + +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 df71d31e17..0412493911 100644 --- a/xen/arch/riscv/xen.lds.S +++ b/xen/arch/riscv/xen.lds.S @@ -40,6 +40,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 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 | 28 +++++++ xen/arch/riscv/traps.c | 129 +++++++++++++++++++++++++++++++ xen/arch/riscv/xen.lds.S | 10 +++ 3 files changed, 167 insertions(+)