ron minnich has submitted this change. ( https://review.coreboot.org/c/coreboot/+/81416?usp=email )
(
7 is the latest approved patch-set. No files were changed between the latest approved patch-set and the submitted one. )Change subject: arch/riscv: remove misaligned load/store/fetch handling ......................................................................
arch/riscv: remove misaligned load/store/fetch handling
Testing on the unmatched shows the code no longer works completely correctly; Linux has taken over the handling of misalignment anyway, because handling it in firmware, with the growing complexity of the ISA and the awkward way in which it has to be handled, is more trouble than its worth.
Plus, we don't WANT misalignment handled, magically, in firmware: the cost of getting it wrong is high (as I've spent a month learning); the performance is terrible (350x slowdown; and most toolchains now know to avoid unaligned load/store on RISC-V anyway.
But, mostly, if alignment problems exist, *we need to know*, and if they're handled invisibly in firmware, we don't.
The problem with invisible handling was shown a while back in the Go toolchain: runtime had a small error, such that many misaligned load/store were happening, and it was not discovered for some time. Had a trap been directed to kernel or user on misalignment, the problem would have been known immediately, not after many months. (The error, btw, was masking the address with 3, not 7, to detect misalignment; an easy mistake!).
But, the coreboot code does not work any more any way, and it's not worth fixing. Remove it.
Tested by booting Linux to runlevel 1; before, it would hang on an alignment fault, as the alignment code was failing (somewhere).
This takes the coreboot SBI code much closer to revival.
Change-Id: I84a8d433ed2f50745686a8c109d101e8718f2a46 Signed-off-by: Ronald G Minnich rminnich@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/81416 Reviewed-by: Maximilian Brune maximilian.brune@9elements.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/arch/riscv/Makefile.mk M src/arch/riscv/include/arch/exception.h D src/arch/riscv/misaligned.c M src/arch/riscv/trap_handler.c M src/arch/riscv/virtual_memory.c 5 files changed, 4 insertions(+), 265 deletions(-)
Approvals: Maximilian Brune: Looks good to me, approved build bot (Jenkins): Verified
diff --git a/src/arch/riscv/Makefile.mk b/src/arch/riscv/Makefile.mk index 1131bd5..d5defea 100644 --- a/src/arch/riscv/Makefile.mk +++ b/src/arch/riscv/Makefile.mk @@ -52,7 +52,6 @@ all-y += trap_util.S all-y += trap_handler.c all-y += fp_asm.S -all-y += misaligned.c all-y += sbi.c all-y += mcall.c all-y += virtual_memory.c diff --git a/src/arch/riscv/include/arch/exception.h b/src/arch/riscv/include/arch/exception.h index e807a3f..2eb575e 100644 --- a/src/arch/riscv/include/arch/exception.h +++ b/src/arch/riscv/include/arch/exception.h @@ -28,6 +28,5 @@ void redirect_trap(void); void default_trap_handler(struct trapframe *tf); void handle_supervisor_call(struct trapframe *tf); -void handle_misaligned(struct trapframe *tf);
#endif diff --git a/src/arch/riscv/misaligned.c b/src/arch/riscv/misaligned.c deleted file mode 100644 index 151ed99..0000000 --- a/src/arch/riscv/misaligned.c +++ /dev/null @@ -1,260 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0-only */ - -#include <vm.h> -#include <arch/exception.h> -#include <commonlib/helpers.h> -#include <types.h> - -/* these functions are defined in src/arch/riscv/fp_asm.S */ -#if defined(__riscv_flen) -#if __riscv_flen >= 32 -extern void read_f32(int regnum, uint32_t *v); -extern void write_f32(int regnum, uint32_t *v); -#endif // __riscv_flen >= 32 -#if __riscv_flen >= 64 -extern void read_f64(int regnum, uint64_t *v); -extern void write_f64(int regnum, uint64_t *v); -#endif // __riscv_flen >= 64 -#endif // defined(__riscv_flen) - -/* This union makes it easy to read multibyte types by byte operations. */ -union endian_buf { - uint8_t b[8]; - uint16_t h[4]; - uint32_t w[2]; - uint64_t d[1]; - uintptr_t v; -}; - -/* This struct hold info of load/store instruction */ -struct memory_instruction_info { - /* opcode/mask used to identify instruction, - * (instruction_val) & mask == opcode */ - uint32_t opcode; - uint32_t mask; - /* reg_shift/reg_mask/reg_addition used to get register number - * ((instruction_val >> reg_shift) & reg_mask) + reg_addition */ - unsigned int reg_shift; - unsigned int reg_mask; - unsigned int reg_addition; - unsigned int is_fp : 1; /* mark as a float operation */ - unsigned int is_load : 1; /* mark as a load operation */ - unsigned int width : 8; /* Record the memory width of the operation */ - unsigned int sign_extend : 1; /* mark need to be sign extended */ -}; - -static struct memory_instruction_info insn_info[] = { -#if __riscv_xlen == 128 - { 0x00002000, 0x0000e003, 2, 7, 8, 0, 1, 16, 1}, // C.LQ -#else - { 0x00002000, 0x0000e003, 2, 7, 8, 1, 1, 8, 0}, // C.FLD -#endif - { 0x00004000, 0x0000e003, 2, 7, 8, 0, 1, 4, 1}, // C.LW -#if __riscv_xlen == 32 - { 0x00006000, 0x0000e003, 2, 7, 8, 1, 1, 4, 0}, // C.FLW -#else - { 0x00006000, 0x0000e003, 2, 7, 8, 0, 1, 8, 1}, // C.LD -#endif - -#if __riscv_xlen == 128 - { 0x0000a000, 0x0000e003, 2, 7, 8, 0, 0, 16, 0}, // C.SQ -#else - { 0x0000a000, 0x0000e003, 2, 7, 8, 1, 0, 8, 0}, // C.FSD -#endif - { 0x0000c000, 0x0000e003, 2, 7, 8, 0, 0, 4, 0}, // C.SW -#if __riscv_xlen == 32 - { 0x0000e000, 0x0000e003, 2, 7, 8, 1, 0, 4, 0}, // C.FSW -#else - { 0x0000e000, 0x0000e003, 2, 7, 8, 0, 0, 8, 0}, // C.SD -#endif - -#if __riscv_xlen == 128 - { 0x00002002, 0x0000e003, 7, 15, 0, 0, 1, 16, 1}, // C.LQSP -#else - { 0x00002002, 0x0000e003, 7, 15, 0, 1, 1, 8, 0}, // C.FLDSP -#endif - { 0x00004002, 0x0000e003, 7, 15, 0, 0, 1, 4, 1}, // C.LWSP -#if __riscv_xlen == 32 - { 0x00006002, 0x0000e003, 7, 15, 0, 1, 1, 4, 0}, // C.FLWSP -#else - { 0x00006002, 0x0000e003, 7, 15, 0, 0, 1, 8, 1}, // C.LDSP -#endif - -#if __riscv_xlen == 128 - { 0x0000a002, 0x0000e003, 2, 15, 0, 0, 0, 16, 0}, // C.SQSP -#else - { 0x0000a002, 0x0000e003, 2, 15, 0, 1, 0, 8, 0}, // C.FSDSP -#endif - { 0x0000c002, 0x0000e003, 2, 15, 0, 0, 0, 4, 0}, // C.SWSP -#if __riscv_xlen == 32 - { 0x0000e002, 0x0000e003, 2, 15, 0, 1, 0, 4, 0}, // C.FSWSP -#else - { 0x0000e002, 0x0000e003, 2, 15, 0, 0, 0, 8, 0}, // C.SDSP -#endif - - { 0x00000003, 0x0000707f, 7, 15, 0, 0, 1, 1, 1}, // LB - { 0x00001003, 0x0000707f, 7, 15, 0, 0, 1, 2, 1}, // LH - { 0x00002003, 0x0000707f, 7, 15, 0, 0, 1, 4, 1}, // LW -#if __riscv_xlen > 32 - { 0x00003003, 0x0000707f, 7, 15, 0, 0, 1, 8, 1}, // LD -#endif - { 0x00004003, 0x0000707f, 7, 15, 0, 0, 1, 1, 0}, // LBU - { 0x00005003, 0x0000707f, 7, 15, 0, 0, 1, 2, 0}, // LHU - { 0x00006003, 0x0000707f, 7, 15, 0, 0, 1, 4, 0}, // LWU - - { 0x00000023, 0x0000707f, 20, 15, 0, 0, 0, 1, 0}, // SB - { 0x00001023, 0x0000707f, 20, 15, 0, 0, 0, 2, 0}, // SH - { 0x00002023, 0x0000707f, 20, 15, 0, 0, 0, 4, 0}, // SW -#if __riscv_xlen > 32 - { 0x00003023, 0x0000707f, 20, 15, 0, 0, 0, 8, 0}, // SD -#endif - -#if defined(__riscv_flen) -#if __riscv_flen >= 32 - { 0x00002007, 0x0000707f, 7, 15, 0, 1, 1, 4, 0}, // FLW - { 0x00003007, 0x0000707f, 7, 15, 0, 1, 1, 8, 0}, // FLD -#endif // __riscv_flen >= 32 - -#if __riscv_flen >= 64 - { 0x00002027, 0x0000707f, 20, 15, 0, 1, 0, 4, 0}, // FSW - { 0x00003027, 0x0000707f, 20, 15, 0, 1, 0, 8, 0}, // FSD -#endif // __riscv_flen >= 64 -#endif // defined(__riscv_flen) -}; - -static struct memory_instruction_info *match_instruction(uintptr_t insn) -{ - int i; - for (i = 0; i < ARRAY_SIZE(insn_info); i++) - if ((insn_info[i].mask & insn) == insn_info[i].opcode) - return &(insn_info[i]); - return NULL; -} - -static enum cb_err fetch_16bit_instruction(uintptr_t vaddr, uintptr_t *insn, int *size) -{ - uint16_t ins = mprv_read_mxr_u16((uint16_t *)vaddr); - if (EXTRACT_FIELD(ins, 0x3) != 3) { - *insn = ins; - *size = 2; - return CB_SUCCESS; - } - return CB_ERR; -} - -static enum cb_err fetch_32bit_instruction(uintptr_t vaddr, uintptr_t *insn, int *size) -{ - uint32_t l = (uint32_t)mprv_read_mxr_u16((uint16_t *)vaddr + 0); - uint32_t h = (uint32_t)mprv_read_mxr_u16((uint16_t *)vaddr + 1); - uint32_t ins = (h << 16) | l; - if ((EXTRACT_FIELD(ins, 0x3) == 3) && - (EXTRACT_FIELD(ins, 0x1c) != 0x7)) { - *insn = ins; - *size = 4; - return CB_SUCCESS; - } - return CB_ERR; -} - -void handle_misaligned(struct trapframe *tf) -{ - uintptr_t insn = 0; - union endian_buf buff; - int insn_size = 0; - - /* try to fetch 16/32 bits instruction */ - if (fetch_16bit_instruction(tf->epc, &insn, &insn_size) < 0) { - if (fetch_32bit_instruction(tf->epc, &insn, &insn_size) < 0) { - redirect_trap(); - return; - } - } - - /* matching instruction */ - struct memory_instruction_info *match = match_instruction(insn); - - if (!match) { - redirect_trap(); - return; - } - - int regnum; - regnum = ((insn >> match->reg_shift) & match->reg_mask); - regnum = regnum + match->reg_addition; - buff.v = 0; - if (match->is_load) { - /* load operation */ - - /* reading from memory by bytes prevents misaligned - * memory access */ - for (int i = 0; i < match->width; i++) { - uint8_t *addr = (uint8_t *)(tf->badvaddr + i); - buff.b[i] = mprv_read_u8(addr); - } - - /* sign extend for signed integer loading */ - if (match->sign_extend) - if (buff.v >> (8 * match->width - 1)) - buff.v |= -1 << (8 * match->width); - - /* write to register */ - if (match->is_fp) { - int done = 0; -#if defined(__riscv_flen) -#if __riscv_flen >= 32 - /* single-precision floating-point */ - if (match->width == 4) { - write_f32(regnum, buff.w); - done = 1; - } -#endif // __riscv_flen >= 32 -#if __riscv_flen >= 64 - /* double-precision floating-point */ - if (match->width == 8) { - write_f64(regnum, buff.d); - done = 1; - } -#endif // __riscv_flen >= 64 -#endif // defined(__riscv_flen) - if (!done) - redirect_trap(); - } else { - tf->gpr[regnum] = buff.v; - } - } else { - /* store operation */ - - /* reading from register */ - if (match->is_fp) { - int done = 0; -#if defined(__riscv_flen) -#if __riscv_flen >= 32 - if (match->width == 4) { - read_f32(regnum, buff.w); - done = 1; - } -#endif // __riscv_flen >= 32 -#if __riscv_flen >= 64 - if (match->width == 8) { - read_f64(regnum, buff.d); - done = 1; - } -#endif // __riscv_flen >= 64 -#endif // defined(__riscv_flen) - if (!done) - redirect_trap(); - } else { - buff.v = tf->gpr[regnum]; - } - - /* writing to memory by bytes prevents misaligned - * memory access */ - for (int i = 0; i < match->width; i++) { - uint8_t *addr = (uint8_t *)(tf->badvaddr + i); - mprv_write_u8(addr, buff.b[i]); - } - } - - /* return to where we came from */ - write_csr(mepc, read_csr(mepc) + insn_size); -} diff --git a/src/arch/riscv/trap_handler.c b/src/arch/riscv/trap_handler.c index f3968bb..2f22ce2 100644 --- a/src/arch/riscv/trap_handler.c +++ b/src/arch/riscv/trap_handler.c @@ -119,7 +119,6 @@ }
switch (tf->cause) { - case CAUSE_MISALIGNED_FETCH: case CAUSE_FETCH_ACCESS: case CAUSE_ILLEGAL_INSTRUCTION: case CAUSE_BREAKPOINT: @@ -133,10 +132,10 @@ case CAUSE_SUPERVISOR_ECALL: handle_sbi(tf); return; + case CAUSE_MISALIGNED_FETCH: case CAUSE_MISALIGNED_LOAD: case CAUSE_MISALIGNED_STORE: print_trap_information(tf); - handle_misaligned(tf); return; default: printk(BIOS_EMERG, "================================\n"); diff --git a/src/arch/riscv/virtual_memory.c b/src/arch/riscv/virtual_memory.c index 43e3d70..ed2127c 100644 --- a/src/arch/riscv/virtual_memory.c +++ b/src/arch/riscv/virtual_memory.c @@ -13,8 +13,10 @@ * to M-mode). In practice, this variable has been a lifesaver. It is * still not quite determined which delegation might by unallowed by * the spec so for now we enumerate and set them all. */ -static int delegate = 0 +static u32 delegate = 0 | (1 << CAUSE_MISALIGNED_FETCH) + | (1 << CAUSE_MISALIGNED_STORE) + | (1 << CAUSE_MISALIGNED_LOAD) | (1 << CAUSE_FETCH_ACCESS) | (1 << CAUSE_ILLEGAL_INSTRUCTION) | (1 << CAUSE_BREAKPOINT)