ron minnich has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/81305?usp=email )
Change subject: Signed-off-by: Ronald G Minnich rminnich@gmail.com ......................................................................
Signed-off-by: Ronald G Minnich rminnich@gmail.com
Change-Id: I3ba80ca4f1e0cd7468065b29fd0d07eeff5ec9ed clang-formatted-by: Ronald G Minnich --- M src/arch/riscv/trap_handler.c M src/arch/riscv/virtual_memory.c 2 files changed, 50 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/81305/1
diff --git a/src/arch/riscv/trap_handler.c b/src/arch/riscv/trap_handler.c index f3968bb..d66bed5 100644 --- a/src/arch/riscv/trap_handler.c +++ b/src/arch/riscv/trap_handler.c @@ -111,6 +111,41 @@
void (*trap_handler)(struct trapframe *tf) = default_trap_handler;
+/* + * ill is the bare minimum, very unsophisticated illegal instruction handler. + * It is deliberately dumb because, in the long term, we don't intend + * to use it very much. People have a way of latching on to functions like + * this and growing them without bound. The entire purpose of coreboot SBI + * is to provided the absolute minimum capability needed for modern + * RISC-V CPUs. + * Also, NOTE: C compilers are pretty smart about handling case + * statements. Do not assume, because this looks inefficient, that it is + * inefficient. For a very simple example, note that most compilers + * inline this function without being told to. + * Also, we COULD make this very general, but compilers seem to use + * only a small subset of possible variations, so that may not be + * worth it. + */ +static void ill(struct trapframe *tf) +{ + u32 insn = (u32) (tf->badvaddr); + int insn_size = 4; + + switch (insn) { + case 0xc0102773: // csrrs x14, time, x0 + tf->gpr[24] = *HLS()->time; + break; + case 0xc0102573: // csrrs x10, time, x0 + tf->gpr[20] = *HLS()->time; + break; + default: // proceed anyway. + printk(BIOS_EMERG, "Unhandled instruction: %x\n", insn); + print_trap_information(tf); + break; + } + write_csr(mepc, read_csr(mepc) + insn_size); +} + void default_trap_handler(struct trapframe *tf) { if (tf->cause & 0x8000000000000000ULL) { @@ -119,9 +154,12 @@ }
switch (tf->cause) { + case CAUSE_ILLEGAL_INSTRUCTION: + ill(tf); + return; + break; case CAUSE_MISALIGNED_FETCH: case CAUSE_FETCH_ACCESS: - case CAUSE_ILLEGAL_INSTRUCTION: case CAUSE_BREAKPOINT: case CAUSE_LOAD_ACCESS: case CAUSE_STORE_ACCESS: diff --git a/src/arch/riscv/virtual_memory.c b/src/arch/riscv/virtual_memory.c index 43e3d70..913836f 100644 --- a/src/arch/riscv/virtual_memory.c +++ b/src/arch/riscv/virtual_memory.c @@ -16,7 +16,6 @@ static int delegate = 0 | (1 << CAUSE_MISALIGNED_FETCH) | (1 << CAUSE_FETCH_ACCESS) - | (1 << CAUSE_ILLEGAL_INSTRUCTION) | (1 << CAUSE_BREAKPOINT) | (1 << CAUSE_LOAD_ACCESS) | (1 << CAUSE_STORE_ACCESS) @@ -24,10 +23,18 @@ | (1 << CAUSE_FETCH_PAGE_FAULT) | (1 << CAUSE_LOAD_PAGE_FAULT) | (1 << CAUSE_STORE_PAGE_FAULT) + /* + * This should only be enabled if we do NOT have menvcfg. + * The comment is left here to make sure nobody thinks it was + * left out by mistake. + * | (1 << CAUSE_ILLEGAL_INSTRUCTION) + */ ;
void mstatus_init(void) { + extern uintptr_t has_menvcfg; + // clear any pending timer interrupts. clear_csr(mip, MIP_STIP | MIP_SSIP);
@@ -38,10 +45,13 @@ // Delegate supervisor timer and other interrupts to supervisor mode, // if supervisor mode is supported. if (supports_extension('S')) { + if (ENV_RAMSTAGE && has_menvcfg) + delegate |= (1 << CAUSE_ILLEGAL_INSTRUCTION); set_csr(mideleg, MIP_STIP | MIP_SSIP); set_csr(medeleg, delegate); }
// Enable all user/supervisor-mode counters write_csr(mcounteren, 7); + }