[coreboot-gerrit] [M] Change in coreboot[main]: Signed-off-by: Ronald G Minnich <[e-mail]>

[ron minnich (Code Review)]

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 
......................................................................

Signed-off-by: Ronald G Minnich

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);
+
 }