From 8a5a6cfba9103412a6aabc5bc496736b80c61770 Mon Sep 17 00:00:00 2001 From: Chris Hurley Date: Tue, 8 Sep 2026 15:35:39 -0700 Subject: [PATCH 01/11] refactor(pmp): drop commented-out legacy pmp_check The block referenced symbols that no longer exist (hart_if.reg.PRIV, csr[pmpcfg0+...], bit_sub) so it could not be revived as written. --- src/iss/mem/pmp.h | 31 ------------------------------- 1 file changed, 31 deletions(-) diff --git a/src/iss/mem/pmp.h b/src/iss/mem/pmp.h index b00e00b..300c321 100644 --- a/src/iss/mem/pmp.h +++ b/src/iss/mem/pmp.h @@ -189,37 +189,6 @@ template bool pmp::pmp_ch } base = tor; } - // constexpr auto pmp_num_regs = 16; - // reg_t tor_base = 0; - // auto any_active = false; - // auto lower_addr = addr >>2; - // auto upper_addr = (addr+len-1)>>2; - // for (size_t i = 0; i < pmp_num_regs; i++) { - // uint8_t cfg = csr[pmpcfg0+(i/4)]>>(i%4); - // uint8_t cfg_next = i==(pmp_num_regs-1)? 0 : csr[pmpcfg0+((i+1)/4)]>>((i+1)%4); - // auto pmpaddr = csr[pmpaddr0+i]; - // if (cfg & PMP_A) { - // any_active=true; - // auto is_tor = bit_sub<3, 2>(cfg) == PMP_TOR; - // auto is_napot = bit_sub<4, 1>(cfg) && bit_sub<3, 2>(cfg_next)!= PMP_TOR; - // if(is_napot) { - // reg_t mask = bit_sub<3, 1>(cfg)?~( pmpaddr & ~(pmpaddr + 1)): 0x3fffffff; - // auto mpmpaddr = pmpaddr & mask; - // if((lower_addr&mask) == mpmpaddr && (upper_addr&mask)==mpmpaddr) - // return (hart_if.reg.PRIV == PRIV_M && !(cfg & PMP_L)) || - // (type == access_type::READ && (cfg & PMP_R)) || - // (type == access_type::WRITE && (cfg & PMP_W)) || - // (type == access_type::FETCH && (cfg & PMP_X)); - // } else if(is_tor) { - // if(lower_addr>=tor_base && upper_addr<=pmpaddr) - // return (hart_if.reg.PRIV == PRIV_M && !(cfg & PMP_L)) || - // (type == access_type::READ && (cfg & PMP_R)) || - // (type == access_type::WRITE && (cfg & PMP_W)) || - // (type == access_type::FETCH && (cfg & PMP_X)); - // } - // } - // tor_base = pmpaddr; - // } return hart_if.PRIV == arch::PRIV_M; } From 8b7107d3aa0f18d8ca9ac7f63226b3836e10b9d1 Mon Sep 17 00:00:00 2001 From: Chris Hurley Date: Tue, 8 Sep 2026 15:36:24 -0700 Subject: [PATCH 02/11] refactor(pmp): drop unreachable debug branch in read_mem/write_mem The enclosing condition already required !is_debug(addr.access), so the inner is_debug throw could never run. Debug accesses are meant to bypass PMP entirely so a debugger can still inspect protected memory; making the throw reachable would break that, so the dead branch is removed and the intent recorded in a comment instead. --- src/iss/mem/pmp.h | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/iss/mem/pmp.h b/src/iss/mem/pmp.h index 300c321..d2fd259 100644 --- a/src/iss/mem/pmp.h +++ b/src/iss/mem/pmp.h @@ -90,10 +90,9 @@ template struct pmp : public memory_ele iss::status read_mem(const addr_t& addr, unsigned length, uint8_t* data) { assert((addr.type == iss::address_type::PHYSICAL || is_debug(addr.access)) && "Only physical addresses are expected in pmp"); + // debug accesses bypass PMP so a debugger can inspect protected memory if(likely(addr.space == arch::traits::MEM || std::numeric_limits::max()) && !pmp_check(addr.access, addr.val, length) && !is_debug(addr.access)) { - if(is_debug(addr.access)) - throw trap_access(0, addr.val); // trap is raised in privilege wrapper return iss::Err; } @@ -102,9 +101,8 @@ template struct pmp : public memory_ele iss::status write_mem(const addr_t& addr, unsigned length, uint8_t const* data) { assert((addr.type == iss::address_type::PHYSICAL || is_debug(addr.access)) && "Only physical addresses are expected in pmp"); + // debug accesses bypass PMP so a debugger can modify protected memory if(likely(addr.space == arch::traits::MEM) && !pmp_check(addr.access, addr.val, length) && !is_debug(addr.access)) { - if(is_debug(addr.access)) - throw trap_access(0, addr.val); // trap is raised in privilege wrapper, so we just return error return iss::Err; } From faae4b36c281d31235e40ede229bd6cf291fcb2d Mon Sep 17 00:00:00 2001 From: Chris Hurley Date: Tue, 8 Sep 2026 15:36:56 -0700 Subject: [PATCH 03/11] test(pmp): share the semihosting fail sequence across firmware tests All nine PMP tests carried their own copy of the SYS_EXIT block, three as a local macro and six inlined. Move it to contrib/fw/pmp_test_common.h and include it, which requires -I contrib/fw on the firmware build lines. Each test assembles to byte-identical code before and after. --- .github/workflows/ci.yml | 18 ++++++------- .../pmp_64entry_addr_test.S | 11 +++----- .../pmp_64entry_cfg_test.S | 11 +++----- .../pmp_8entry_guard_test.S | 11 +------- contrib/fw/pmp-cfg2-test/pmp_cfg2_test.S | 11 +++----- contrib/fw/pmp-csr-test/pmp_csr_test.S | 11 +++----- .../fw/pmp-enforce-test/pmp_enforce_test.S | 11 +------- contrib/fw/pmp-shift-test/pmp_shift_test.S | 11 +------- contrib/fw/pmp-tor-test/pmp_tor_test.S | 11 +++----- .../pmp-upper-cfg-test/pmp_upper_cfg_test.S | 11 +++----- contrib/fw/pmp_test_common.h | 26 +++++++++++++++++++ 11 files changed, 56 insertions(+), 87 deletions(-) create mode 100644 contrib/fw/pmp_test_common.h diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4d4b8c3..c1c2a46 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -87,7 +87,7 @@ jobs: - name: Build PMP CSR test firmware run: | - riscv64-unknown-elf-gcc -nostdlib -march=rv64gc -mabi=lp64 \ + riscv64-unknown-elf-gcc -nostdlib -march=rv64gc -mabi=lp64 -I contrib/fw \ -Wl,-Ttext=0x10000,--no-dynamic-linker \ -o pmp_csr_test \ contrib/fw/pmp-csr-test/pmp_csr_test.S @@ -102,7 +102,7 @@ jobs: - name: Build PMP enforcement test firmware run: | - riscv64-unknown-elf-gcc -nostdlib -march=rv64gc -mabi=lp64 \ + riscv64-unknown-elf-gcc -nostdlib -march=rv64gc -mabi=lp64 -I contrib/fw \ -Wl,-Ttext=0x10000,--no-dynamic-linker \ -o pmp_enforce_test \ contrib/fw/pmp-enforce-test/pmp_enforce_test.S @@ -112,7 +112,7 @@ jobs: - name: Build PMP shift test firmware run: | - riscv64-unknown-elf-gcc -nostdlib -march=rv64gc -mabi=lp64 \ + riscv64-unknown-elf-gcc -nostdlib -march=rv64gc -mabi=lp64 -I contrib/fw \ -Wl,-Ttext=0x10000,--no-dynamic-linker \ -o pmp_shift_test \ contrib/fw/pmp-shift-test/pmp_shift_test.S @@ -122,7 +122,7 @@ jobs: - name: Build PMP upper-cfg test firmware run: | - riscv64-unknown-elf-gcc -nostdlib -march=rv64gc -mabi=lp64 \ + riscv64-unknown-elf-gcc -nostdlib -march=rv64gc -mabi=lp64 -I contrib/fw \ -Wl,-Ttext=0x10000,--no-dynamic-linker \ -o pmp_upper_cfg_test \ contrib/fw/pmp-upper-cfg-test/pmp_upper_cfg_test.S @@ -132,7 +132,7 @@ jobs: - name: Build PMP cfg2 test firmware run: | - riscv64-unknown-elf-gcc -nostdlib -march=rv64gc -mabi=lp64 \ + riscv64-unknown-elf-gcc -nostdlib -march=rv64gc -mabi=lp64 -I contrib/fw \ -Wl,-Ttext=0x10000,--no-dynamic-linker \ -o pmp_cfg2_test \ contrib/fw/pmp-cfg2-test/pmp_cfg2_test.S @@ -142,7 +142,7 @@ jobs: - name: Build PMP TOR test firmware run: | - riscv64-unknown-elf-gcc -nostdlib -march=rv64gc -mabi=lp64 \ + riscv64-unknown-elf-gcc -nostdlib -march=rv64gc -mabi=lp64 -I contrib/fw \ -Wl,-Ttext=0x10000,--no-dynamic-linker \ -o pmp_tor_test \ contrib/fw/pmp-tor-test/pmp_tor_test.S @@ -152,7 +152,7 @@ jobs: - name: Build PMP 64-entry pmpaddr test firmware run: | - riscv64-unknown-elf-gcc -nostdlib -march=rv64gc -mabi=lp64 \ + riscv64-unknown-elf-gcc -nostdlib -march=rv64gc -mabi=lp64 -I contrib/fw \ -Wl,-Ttext=0x10000,--no-dynamic-linker \ -o pmp_64entry_addr_test \ contrib/fw/pmp-64entry-addr-test/pmp_64entry_addr_test.S @@ -162,7 +162,7 @@ jobs: - name: Build PMP 64-entry pmpcfg test firmware run: | - riscv64-unknown-elf-gcc -nostdlib -march=rv64gc -mabi=lp64 \ + riscv64-unknown-elf-gcc -nostdlib -march=rv64gc -mabi=lp64 -I contrib/fw \ -Wl,-Ttext=0x10000,--no-dynamic-linker \ -o pmp_64entry_cfg_test \ contrib/fw/pmp-64entry-cfg-test/pmp_64entry_cfg_test.S @@ -172,7 +172,7 @@ jobs: - name: Build PMP 8-entry guard test firmware (VP/S5 model) run: | - riscv64-unknown-elf-gcc -nostdlib -march=rv64gc -mabi=lp64 \ + riscv64-unknown-elf-gcc -nostdlib -march=rv64gc -mabi=lp64 -I contrib/fw \ -Wl,-Ttext=0x10000,--no-dynamic-linker \ -o pmp_8entry_guard_test \ contrib/fw/pmp-8entry-guard-test/pmp_8entry_guard_test.S diff --git a/contrib/fw/pmp-64entry-addr-test/pmp_64entry_addr_test.S b/contrib/fw/pmp-64entry-addr-test/pmp_64entry_addr_test.S index 64fe595..7f4e4f0 100644 --- a/contrib/fw/pmp-64entry-addr-test/pmp_64entry_addr_test.S +++ b/contrib/fw/pmp-64entry-addr-test/pmp_64entry_addr_test.S @@ -2,6 +2,8 @@ // Pass: readback matches written sentinel -> j . (exit 0) // Fail: any trap (unregistered CSR) or readback mismatch -> semihosting SYS_EXIT (exit 2) +#include "pmp_test_common.h" + .section .text .globl _start _start: @@ -16,11 +18,4 @@ _start: j . // pass fail: - li a0, 0x18 - .option push - .option norvc - slli zero, zero, 0x1f - ebreak - srai zero, zero, 7 - .option pop - j . + semihosting_fail diff --git a/contrib/fw/pmp-64entry-cfg-test/pmp_64entry_cfg_test.S b/contrib/fw/pmp-64entry-cfg-test/pmp_64entry_cfg_test.S index 0336554..1c08912 100644 --- a/contrib/fw/pmp-64entry-cfg-test/pmp_64entry_cfg_test.S +++ b/contrib/fw/pmp-64entry-cfg-test/pmp_64entry_cfg_test.S @@ -3,6 +3,8 @@ // Pass: write 0x9F -> read back 0x9F -> write 0 -> read back 0 -> j . (exit 0) // Fail: any trap or readback mismatch -> semihosting SYS_EXIT (exit 2) +#include "pmp_test_common.h" + .section .text .globl _start _start: @@ -24,11 +26,4 @@ _start: j . // pass fail: - li a0, 0x18 - .option push - .option norvc - slli zero, zero, 0x1f - ebreak - srai zero, zero, 7 - .option pop - j . + semihosting_fail diff --git a/contrib/fw/pmp-8entry-guard-test/pmp_8entry_guard_test.S b/contrib/fw/pmp-8entry-guard-test/pmp_8entry_guard_test.S index a739801..da4afcb 100644 --- a/contrib/fw/pmp-8entry-guard-test/pmp_8entry_guard_test.S +++ b/contrib/fw/pmp-8entry-guard-test/pmp_8entry_guard_test.S @@ -5,16 +5,7 @@ // Pass: phase2_trap fires (load denied) -> j . (exit 0) // Fail: CSR trap, load succeeds, or unexpected behavior -> semihosting SYS_EXIT (exit 2) -.macro semihosting_fail - li a0, 0x18 - .option push - .option norvc - slli zero, zero, 0x1f - ebreak - srai zero, zero, 7 - .option pop - j . -.endm +#include "pmp_test_common.h" .section .text .globl _start diff --git a/contrib/fw/pmp-cfg2-test/pmp_cfg2_test.S b/contrib/fw/pmp-cfg2-test/pmp_cfg2_test.S index 7a125a0..439632b 100644 --- a/contrib/fw/pmp-cfg2-test/pmp_cfg2_test.S +++ b/contrib/fw/pmp-cfg2-test/pmp_cfg2_test.S @@ -4,6 +4,8 @@ // Pass: pmpcfg2 readback == written value -> j . (exit 0) // Fail: pmpcfg2 write traps OR readback wrong -> semihosting SYS_EXIT (exit 2) +#include "pmp_test_common.h" + .section .text .globl _start _start: @@ -19,11 +21,4 @@ _start: j . // pass: ISS detects j . and exits 0 fail: - li a0, 0x18 - .option push - .option norvc - slli zero, zero, 0x1f - ebreak - srai zero, zero, 7 - .option pop - j . + semihosting_fail diff --git a/contrib/fw/pmp-csr-test/pmp_csr_test.S b/contrib/fw/pmp-csr-test/pmp_csr_test.S index e363642..ac01397 100644 --- a/contrib/fw/pmp-csr-test/pmp_csr_test.S +++ b/contrib/fw/pmp-csr-test/pmp_csr_test.S @@ -2,6 +2,8 @@ // Pass: j . detected as JUMP_TO_SELF by ISS, exits 0. // Fail: semihosting SYS_EXIT sequence, ISS exits non-zero. +#include "pmp_test_common.h" + .section .text .globl _start _start: @@ -22,11 +24,4 @@ fail: // Semihosting SYS_EXIT: the ISS checks for slli+ebreak+srai and intercepts // before dispatching to mtvec. Must use 4-byte ebreak (not c.ebreak) so the // check lands at the correct offsets (-4 and +4 from the ebreak address). - li a0, 0x18 // SYS_EXIT - .option push - .option norvc // force 4-byte ebreak (0x00100073), not 2-byte c.ebreak - slli zero, zero, 0x1f - ebreak - srai zero, zero, 7 - .option pop - j . // fallback if semihosting is not configured + semihosting_fail diff --git a/contrib/fw/pmp-enforce-test/pmp_enforce_test.S b/contrib/fw/pmp-enforce-test/pmp_enforce_test.S index 549f133..da31a0d 100644 --- a/contrib/fw/pmp-enforce-test/pmp_enforce_test.S +++ b/contrib/fw/pmp-enforce-test/pmp_enforce_test.S @@ -6,16 +6,7 @@ // Pass: PMP denies load -> load fault fires -> j . (exit 0) // Fail: any CSR trap, OR load succeeds without fault -> semihosting SYS_EXIT (exit 2) -.macro semihosting_fail - li a0, 0x18 - .option push - .option norvc - slli zero, zero, 0x1f - ebreak - srai zero, zero, 7 - .option pop - j . -.endm +#include "pmp_test_common.h" .section .text .globl _start diff --git a/contrib/fw/pmp-shift-test/pmp_shift_test.S b/contrib/fw/pmp-shift-test/pmp_shift_test.S index bbb15bb..4bfd69f 100644 --- a/contrib/fw/pmp-shift-test/pmp_shift_test.S +++ b/contrib/fw/pmp-shift-test/pmp_shift_test.S @@ -8,16 +8,7 @@ // Pass: PMP denies load -> load fault fires -> j . (exit 0) // Fail: any CSR trap, OR load succeeds without fault -> semihosting SYS_EXIT (exit 2) -.macro semihosting_fail - li a0, 0x18 - .option push - .option norvc - slli zero, zero, 0x1f - ebreak - srai zero, zero, 7 - .option pop - j . -.endm +#include "pmp_test_common.h" .section .text .globl _start diff --git a/contrib/fw/pmp-tor-test/pmp_tor_test.S b/contrib/fw/pmp-tor-test/pmp_tor_test.S index b9001a9..5c2499d 100644 --- a/contrib/fw/pmp-tor-test/pmp_tor_test.S +++ b/contrib/fw/pmp-tor-test/pmp_tor_test.S @@ -8,6 +8,8 @@ // Pass: load fault fires -> j . (exit 0) // Fail: load succeeds without fault -> semihosting SYS_EXIT (exit 2) +#include "pmp_test_common.h" + .section .text .globl _start _start: @@ -38,14 +40,7 @@ _start: // No fault fired -> bug present fail: - li a0, 0x18 - .option push - .option norvc - slli zero, zero, 0x1f - ebreak - srai zero, zero, 7 - .option pop - j . + semihosting_fail pmp_fault: j . // load denied as expected -> PASS diff --git a/contrib/fw/pmp-upper-cfg-test/pmp_upper_cfg_test.S b/contrib/fw/pmp-upper-cfg-test/pmp_upper_cfg_test.S index 7c7e524..8334724 100644 --- a/contrib/fw/pmp-upper-cfg-test/pmp_upper_cfg_test.S +++ b/contrib/fw/pmp-upper-cfg-test/pmp_upper_cfg_test.S @@ -5,6 +5,8 @@ // Pass: readback == written value -> j . (exit 0) // Fail: readback != written value -> semihosting SYS_EXIT (exit 2) +#include "pmp_test_common.h" + .section .text .globl _start _start: @@ -17,11 +19,4 @@ _start: j . // pass: ISS detects j . and exits 0 fail: - li a0, 0x18 - .option push - .option norvc - slli zero, zero, 0x1f - ebreak - srai zero, zero, 7 - .option pop - j . + semihosting_fail diff --git a/contrib/fw/pmp_test_common.h b/contrib/fw/pmp_test_common.h new file mode 100644 index 0000000..0e13eb7 --- /dev/null +++ b/contrib/fw/pmp_test_common.h @@ -0,0 +1,26 @@ +// Shared helpers for the PMP firmware tests in contrib/fw/pmp-*. +// +// Result convention used by every PMP test: +// Pass: reach a `j .` self-loop, which trips the ISS JUMP_TO_SELF finish +// condition and exits 0. +// Fail: semihosting SYS_EXIT, which makes riscv-sim exit 2. + +#ifndef PMP_TEST_COMMON_H +#define PMP_TEST_COMMON_H + +// Terminate the run as a failure via ARM-style semihosting SYS_EXIT. +// The ebreak must be the 4-byte encoding rather than c.ebreak, because the ISS +// identifies the semihosting call by the magic instructions at -4 and +4 around +// it, so norvc is required here even when the caller allows compressed code. +.macro semihosting_fail + li a0, 0x18 // SYS_EXIT + .option push + .option norvc + slli zero, zero, 0x1f + ebreak + srai zero, zero, 7 + .option pop + j . // fallback if semihosting is not configured +.endm + +#endif // PMP_TEST_COMMON_H From e2a2a892cfb891fe40ebcf3eadae63e573a28831 Mon Sep 17 00:00:00 2001 From: Chris Hurley Date: Tue, 8 Sep 2026 15:39:14 -0700 Subject: [PATCH 04/11] fix(pmp): compare against IMEM instead of an always-true space guard read_mem gated on addr.space == traits::MEM || std::numeric_limits::max() The right operand is a non-zero constant, so the whole disjunction was always true. The intent was to also admit the IMEM space, which is declared as enum mem_type_e { MEM, FENCE, RES, CSR, IMEM = std::numeric_limits<...>::max() } so the comparison against addr.space was simply dropped. Restore it. Deleting the term instead, rather than fixing the comparison, would look like a tidy-up but would stop PMP from ever checking instruction fetches, since fetch_ins issues them in the IMEM space. That silently disables X permission enforcement and no existing test notices, so add pmp-fetch-deny-test, which locks an NA4 region with no X over a routine and jumps into it. It is the first test to cover the fetch path and the X bit at all. --- .../pmp-fetch-deny-test/pmp_fetch_deny_test.S | 46 +++++++++++++++++++ src/iss/mem/pmp.h | 3 +- 2 files changed, 48 insertions(+), 1 deletion(-) create mode 100644 contrib/fw/pmp-fetch-deny-test/pmp_fetch_deny_test.S diff --git a/contrib/fw/pmp-fetch-deny-test/pmp_fetch_deny_test.S b/contrib/fw/pmp-fetch-deny-test/pmp_fetch_deny_test.S new file mode 100644 index 0000000..71feff0 --- /dev/null +++ b/contrib/fw/pmp-fetch-deny-test/pmp_fetch_deny_test.S @@ -0,0 +1,46 @@ +// PMP execute-permission enforcement on instruction fetch. +// +// Instruction fetches use the IMEM address space rather than MEM, and no other +// PMP test exercises the fetch path or the X permission bit at all. This test +// pins that behaviour so the address-space guard in pmp.h read_mem cannot be +// narrowed to MEM alone without a test failing. +// +// Entry 0 is an NA4 region covering the 4 bytes at target, locked and with no +// X permission. L=1 is required because pmp_check grants M-mode unconditionally +// when L=0. +// +// Pass: fetch of target is denied -> fault fires -> j . (exit 0) +// Fail: any CSR trap, OR the fetch succeeds -> semihosting SYS_EXIT (exit 2) + +#include "pmp_test_common.h" + +.section .text +.globl _start +_start: + // A CSR trap here means PMP is unavailable -> FAIL + la t0, csr_trap + csrw mtvec, t0 + + // pmpaddr0 = target >> 2 (NA4: covers exactly the 4 bytes at target) + la t5, target + srli t1, t5, 2 + csrw pmpaddr0, t1 + + // pmpcfg0 byte 0 = 0x90 = NA4 (0x10) | L (0x80), no R/W/X + li t1, 0x90 + csrw pmpcfg0, t1 + + la t0, pmp_fault + csrw mtvec, t0 + + jr t5 // fetch from target: PMP must deny (no X) + +csr_trap: + semihosting_fail // CSR trapped: PMP unavailable -> FAIL + +pmp_fault: + j . // fetch denied as expected -> PASS + + .align 2 // 4-byte align so the NA4 region covers this exactly +target: + semihosting_fail // executed only if the fetch was allowed -> FAIL diff --git a/src/iss/mem/pmp.h b/src/iss/mem/pmp.h index d2fd259..7861d4f 100644 --- a/src/iss/mem/pmp.h +++ b/src/iss/mem/pmp.h @@ -91,7 +91,8 @@ template struct pmp : public memory_ele iss::status read_mem(const addr_t& addr, unsigned length, uint8_t* data) { assert((addr.type == iss::address_type::PHYSICAL || is_debug(addr.access)) && "Only physical addresses are expected in pmp"); // debug accesses bypass PMP so a debugger can inspect protected memory - if(likely(addr.space == arch::traits::MEM || std::numeric_limits::max()) && + // instruction fetches arrive in the IMEM space, data reads in MEM; both are subject to PMP + if(likely(addr.space == arch::traits::MEM || addr.space == arch::traits::IMEM) && !pmp_check(addr.access, addr.val, length) && !is_debug(addr.access)) { // trap is raised in privilege wrapper return iss::Err; From 106b6ba92a5e774b839c50a9f6896ffd866cf1c5 Mon Sep 17 00:00:00 2001 From: Chris Hurley Date: Tue, 8 Sep 2026 15:40:37 -0700 Subject: [PATCH 05/11] fix(pmp): check every sector an access touches, not floor(len/4) of them The sector walk started at addr and stopped once the offset reached len, so it visited only floor(len/4) sectors. When addr is not sector aligned the last touched sector was never examined and could not contribute to the match, so an access overlapping a deny region was allowed. Iterate from the aligned-down first sector to the aligned-down last sector inclusive instead. Instruction fetch is the reachable path. Misaligned data accesses are rejected with MISALIGNED_LOAD before they reach PMP, but fetch_ins always requests 4 bytes and the fetch alignment is 2 on a compressed ISA, so a 4-byte instruction at addr%4==2 spans two sectors. pmp-fetch-straddle-test places a 4-byte jump at addr%4==2 with an NA4 deny region over the upper sector only, so the fetch partially overlaps it and must be denied. --- .../pmp_fetch_straddle_test.S | 64 +++++++++++++++++++ src/iss/mem/pmp.h | 12 +++- 2 files changed, 73 insertions(+), 3 deletions(-) create mode 100644 contrib/fw/pmp-fetch-straddle-test/pmp_fetch_straddle_test.S diff --git a/contrib/fw/pmp-fetch-straddle-test/pmp_fetch_straddle_test.S b/contrib/fw/pmp-fetch-straddle-test/pmp_fetch_straddle_test.S new file mode 100644 index 0000000..1e90eb1 --- /dev/null +++ b/contrib/fw/pmp-fetch-straddle-test/pmp_fetch_straddle_test.S @@ -0,0 +1,64 @@ +// PMP sector iteration must cover every 4-byte sector an access touches. +// +// pmp_check walked sectors with `for(offset = 0; offset < len; offset += 4)`, +// which visits only floor(len/4) sectors starting at addr. When addr is not +// 4-byte aligned the final touched sector is never examined. +// +// Instruction fetch is the reachable case: the hart rejects misaligned data +// accesses with MISALIGNED_LOAD before they reach PMP, but fetch_ins always +// requests 4 bytes and the fetch alignment is 2 on a compressed ISA, so a +// 4-byte instruction at PC%4==2 spans two sectors. +// +// Layout at straddle_base (4-byte aligned): +// base+0..base+1 c.nop sector base+0 +// base+2..base+5 4-byte j sectors base+0 and base+4 +// Entry 0 is an NA4 deny region covering only sector base+4, so the fetch at +// base+2 partially overlaps it. Partial overlap must be denied. +// +// Buggy: only sector base+0 is checked, no entry matches, the M-mode +// fall-through allows the fetch, the jump runs -> FAIL. +// Fixed: sector base+4 matches, the overlap is partial -> denied -> PASS. +// +// Pass: fetch of the straddling instruction is denied -> j . (exit 0) +// Fail: any CSR trap, OR the fetch succeeds -> semihosting SYS_EXIT (exit 2) + +#include "pmp_test_common.h" + +.section .text +.globl _start +_start: + // A CSR trap here means PMP is unavailable -> FAIL + la t0, csr_trap + csrw mtvec, t0 + + // pmpaddr0 = (straddle_base + 4) >> 2: NA4 over the upper sector only + la t5, straddle_base + addi t1, t5, 4 + srli t1, t1, 2 + csrw pmpaddr0, t1 + + // pmpcfg0 byte 0 = 0x90 = NA4 (0x10) | L (0x80), no R/W/X + li t1, 0x90 + csrw pmpcfg0, t1 + + la t0, pmp_fault + csrw mtvec, t0 + + jr t5 // enter at straddle_base, which is 4-byte aligned + +csr_trap: + semihosting_fail // CSR trapped: PMP unavailable -> FAIL + +pmp_fault: + j . // straddling fetch denied as expected -> PASS + +fetch_allowed: + semihosting_fail // the straddling fetch was allowed -> FAIL + + .align 2 // 4-byte align: straddle_base % 4 == 0 +straddle_base: + c.nop // 2 bytes, leaves PC at base+2 + .option push + .option norvc // force a 4-byte jump so the fetch spans two sectors + j fetch_allowed // occupies base+2..base+5 + .option pop diff --git a/src/iss/mem/pmp.h b/src/iss/mem/pmp.h index 7861d4f..f80ec99 100644 --- a/src/iss/mem/pmp.h +++ b/src/iss/mem/pmp.h @@ -167,11 +167,17 @@ template bool pmp::pmp_ch auto is_na4 = pmp_a == PMP_NA4; reg_t mask = (pmpaddr[i] << 1) | (!is_na4); mask = ~(mask & ~(mask + 1)) << PMP_SHIFT; - // Check each 4-byte sector of the access + // Check every 4-byte sector the access touches. Counting offsets up to len + // skips the last sector whenever addr is not sector aligned, which fetches can + // be: fetch_ins always asks for 4 bytes and the fetch alignment is 2 on a + // compressed ISA, so an instruction at addr%4==2 spans two sectors. Note this + // also inspects the 2 bytes the ISS over-reads past a compressed instruction, + // so a fetch at the very end of an executable region is denied conservatively. auto any_match = false; auto all_match = true; - for(reg_t offset = 0; offset < len; offset += 1 << PMP_SHIFT) { - reg_t cur_addr = addr + offset; + constexpr reg_t sector_size = 1 << PMP_SHIFT; + reg_t last_sector = (addr + len - 1) & ~(sector_size - 1); + for(reg_t cur_addr = addr & ~(sector_size - 1); cur_addr <= last_sector; cur_addr += sector_size) { auto napot_match = ((cur_addr ^ tor) & mask) == 0; auto tor_match = base <= cur_addr && cur_addr < tor; auto match = is_tor ? tor_match : napot_match; From 2dba29a2274167653face6be57e135c01257727b Mon Sep 17 00:00:00 2001 From: Chris Hurley Date: Tue, 8 Sep 2026 15:42:12 -0700 Subject: [PATCH 06/11] refactor(pmp): extract cfg_byte helper for packed config access The shift-and-index expression for an entry's config byte was repeated at each use. No behaviour change: every existing use masks to bits at or below 7, so narrowing the result to a byte cannot alter it. --- src/iss/mem/pmp.h | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/iss/mem/pmp.h b/src/iss/mem/pmp.h index f80ec99..6da1e80 100644 --- a/src/iss/mem/pmp.h +++ b/src/iss/mem/pmp.h @@ -126,6 +126,9 @@ template struct pmp : public memory_ele return iss::Err; } + // config byte of entry i, extracted from the cfg register that packs it + uint8_t cfg_byte(size_t i) const { return (pmpcfg[i / cfg_reg_size] >> ((i % cfg_reg_size) * 8)) & 0xff; } + iss::status read_pmpcfg(unsigned addr, reg_t& val) { if(addr >= arch::pmpcfg0 && addr < arch::pmpcfg0 + (NUM_ENTRIES / cfg_reg_size) * pmpcfg_stride) { val = pmpcfg[(addr - arch::pmpcfg0) / pmpcfg_stride]; @@ -137,10 +140,8 @@ template struct pmp : public memory_ele if(addr >= arch::pmpcfg0 && addr < arch::pmpcfg0 + (NUM_ENTRIES / cfg_reg_size) * pmpcfg_stride) { pmpcfg[(addr - arch::pmpcfg0) / pmpcfg_stride] = val & cfg_valid_mask; any_active = false; - for(size_t i = 0; i < NUM_ENTRIES; i++) { - auto cfg = pmpcfg[i / cfg_reg_size] >> ((i % cfg_reg_size) * 8); - any_active |= cfg & PMP_A; - } + for(size_t i = 0; i < NUM_ENTRIES; i++) + any_active |= cfg_byte(i) & PMP_A; return iss::Ok; } return iss::Err; @@ -160,7 +161,7 @@ template bool pmp::pmp_ch reg_t base = 0; for(size_t i = 0; i < NUM_ENTRIES; i++) { reg_t tor = pmpaddr[i] << PMP_SHIFT; - reg_t cfg = pmpcfg[i / cfg_reg_size] >> ((i % cfg_reg_size) * 8); + reg_t cfg = cfg_byte(i); if(cfg & PMP_A) { auto pmp_a = (cfg & PMP_A) >> 3; auto is_tor = pmp_a == PMP_TOR; From 1874d78587f4cf92b2d22491fdc2fccd35604fce Mon Sep 17 00:00:00 2001 From: Chris Hurley Date: Tue, 8 Sep 2026 15:44:29 -0700 Subject: [PATCH 07/11] fix(pmp): enforce the lock bit on pmpcfg and pmpaddr writes write_pmpcfg and write_pmpaddr replaced their targets unconditionally, so a locked entry could be relaxed at any time. Per the privileged spec, L=1 freezes both pmpicfg and pmpiaddr until reset, and a locked entry with A=TOR also freezes pmpaddr[i-1], which supplies its lower bound. Retain locked cfg bytes individually and drop writes to a frozen address. Both still report Ok, since such writes are ignored rather than trapping. pmp-64entry-cfg-test probed pmpcfg4 with 0x9F and then asserted it could be cleared. 0x9F has L set, so that assertion required the lock to be violated. The test exists to check that pmpcfg4 is registered and persistent for entries 32 to 39, which 0x1F covers just as well without locking the entry. --- .../pmp_64entry_cfg_test.S | 6 +- contrib/fw/pmp-lock-test/pmp_lock_test.S | 66 +++++++++++++++++++ src/iss/mem/pmp.h | 15 ++++- 3 files changed, 83 insertions(+), 4 deletions(-) create mode 100644 contrib/fw/pmp-lock-test/pmp_lock_test.S diff --git a/contrib/fw/pmp-64entry-cfg-test/pmp_64entry_cfg_test.S b/contrib/fw/pmp-64entry-cfg-test/pmp_64entry_cfg_test.S index 1c08912..b0527ac 100644 --- a/contrib/fw/pmp-64entry-cfg-test/pmp_64entry_cfg_test.S +++ b/contrib/fw/pmp-64entry-cfg-test/pmp_64entry_cfg_test.S @@ -1,6 +1,8 @@ // Verify pmpcfg4 (CSR 0x3A4, holds entries 32-39 on RV64) is accessible and persistent. // Tests 64-entry pmpcfg registration. -// Pass: write 0x9F -> read back 0x9F -> write 0 -> read back 0 -> j . (exit 0) +// The probe value deliberately leaves L (0x80) clear: locking entry 32 would freeze its +// config byte, so the clear below could not be observed. +// Pass: write 0x1F -> read back 0x1F -> write 0 -> read back 0 -> j . (exit 0) // Fail: any trap or readback mismatch -> semihosting SYS_EXIT (exit 2) #include "pmp_test_common.h" @@ -12,7 +14,7 @@ _start: csrw mtvec, t0 // Write and verify a non-zero value - li t0, 0x9F + li t0, 0x1F csrw 0x3A4, t0 // pmpcfg4 write csrr t1, 0x3A4 // pmpcfg4 read bne t0, t1, fail // must match diff --git a/contrib/fw/pmp-lock-test/pmp_lock_test.S b/contrib/fw/pmp-lock-test/pmp_lock_test.S new file mode 100644 index 0000000..75ee06a --- /dev/null +++ b/contrib/fw/pmp-lock-test/pmp_lock_test.S @@ -0,0 +1,66 @@ +// PMP lock bit (L, bit 7 of a cfg byte) must be enforced. +// +// Per the RISC-V privileged spec, once a PMP entry is locked only a reset can +// unlock it: L=1 freezes both pmpicfg and the associated pmpiaddr, and if a +// locked entry uses A=TOR it additionally freezes pmpaddr[i-1], the region's +// lower bound. write_pmpcfg and write_pmpaddr overwrote their targets +// unconditionally, so a locked configuration could be relaxed at will. +// +// The locked entries here use A=OFF (and an empty TOR range) so no region is +// ever matched. This keeps the test purely about CSR retention and removes any +// chance of the test denying its own instruction fetches. +// +// Writes to locked entries are ignored silently rather than trapping, so every +// check is a CSR write followed by a readback compare. +// +// Pass: all three locked values are retained -> j . (exit 0) +// Fail: any CSR trap, OR any locked value changed -> semihosting SYS_EXIT (exit 2) + +#include "pmp_test_common.h" + +.section .text +.globl _start +_start: + // A CSR trap here means PMP is unavailable -> FAIL + la t0, fail + csrw mtvec, t0 + + // Entry 0: set an address, then lock the entry with A=OFF so it is locked + // but inactive. + li t1, 0x1111 + csrw pmpaddr0, t1 + li t1, 0x80 // L (0x80), A=OFF + csrw pmpcfg0, t1 + csrr t2, pmpcfg0 + li t3, 0x80 + bne t2, t3, fail // lock bit did not store at all + + // A locked cfg byte must survive an attempt to clear it. + csrw pmpcfg0, zero + csrr t2, pmpcfg0 + bne t2, t3, fail + + // A locked entry's pmpaddr must survive an attempt to change it. + li t1, 0x2222 + csrw pmpaddr0, t1 + csrr t2, pmpaddr0 + li t3, 0x1111 + bne t2, t3, fail + + // Entry 3 as locked TOR freezes pmpaddr2, its lower bound, even though + // entry 2 is itself unlocked. pmpaddr3 stays 0 so the range is empty. + li t1, 0x3333 + csrw pmpaddr2, t1 + li t1, 0x88 // L (0x80) | A=TOR (0x08) + slli t1, t1, 24 // place in cfg byte 3 -> entry 3 + csrw pmpcfg0, t1 + li t1, 0x4444 + csrw pmpaddr2, t1 + csrr t2, pmpaddr2 + li t3, 0x3333 + bne t2, t3, fail + + j . // every locked value retained -> PASS + +fail: + semihosting_fail diff --git a/src/iss/mem/pmp.h b/src/iss/mem/pmp.h index 6da1e80..abcbdb9 100644 --- a/src/iss/mem/pmp.h +++ b/src/iss/mem/pmp.h @@ -120,7 +120,12 @@ template struct pmp : public memory_ele iss::status write_pmpaddr(unsigned addr, reg_t const& val) { if(addr >= arch::pmpaddr0 && addr < arch::pmpaddr0 + NUM_ENTRIES) { - pmpaddr[addr - arch::pmpaddr0] = val; + auto i = addr - arch::pmpaddr0; + // L freezes the entry's own address; a locked TOR entry additionally freezes + // the preceding address register, which supplies its lower bound + auto locked_as_tor_base = i + 1 < NUM_ENTRIES && (cfg_byte(i + 1) & PMP_L) && ((cfg_byte(i + 1) & PMP_A) >> 3) == PMP_TOR; + if(!(cfg_byte(i) & PMP_L) && !locked_as_tor_base) + pmpaddr[i] = val; return iss::Ok; } return iss::Err; @@ -138,7 +143,13 @@ template struct pmp : public memory_ele } iss::status write_pmpcfg(unsigned addr, reg_t val) { if(addr >= arch::pmpcfg0 && addr < arch::pmpcfg0 + (NUM_ENTRIES / cfg_reg_size) * pmpcfg_stride) { - pmpcfg[(addr - arch::pmpcfg0) / pmpcfg_stride] = val & cfg_valid_mask; + auto reg = (addr - arch::pmpcfg0) / pmpcfg_stride; + // a locked entry keeps its config byte until reset, so retain those bytes + reg_t locked = 0; + for(size_t b = 0; b < cfg_reg_size; b++) + if(cfg_byte(reg * cfg_reg_size + b) & PMP_L) + locked |= reg_t(0xff) << (b * 8); + pmpcfg[reg] = (pmpcfg[reg] & locked) | ((val & cfg_valid_mask) & ~locked); any_active = false; for(size_t i = 0; i < NUM_ENTRIES; i++) any_active |= cfg_byte(i) & PMP_A; From ef5c4620af8fdccbcd1eadcb6f1b0d4028374716 Mon Sep 17 00:00:00 2001 From: Chris Hurley Date: Tue, 8 Sep 2026 15:48:22 -0700 Subject: [PATCH 08/11] fix(pmp): fail S/U access when no PMP entry is enabled pmp_check short-circuited with `if(!any_active) return true;`, allowing the access at any privilege level. Per the privileged spec, when no entry matches, an M-mode access succeeds but an S/U-mode access fails as long as at least one entry is implemented. The fall-through at the end of the function already returned PRIV == PRIV_M, so the shortcut contradicted it. Return the same thing and keep the scan skipped. Every PMP core so far wraps riscv_hart_m_p, where PRIV is always PRIV_M and the old and new results coincide, so the behaviour was untestable. Add rv64gc_mup:interp, which pairs riscv_hart_mu_p with PMP, and pmp-noentry-umode-test, which drops to U mode with no entry programmed and requires the access to be denied. Also corrects a comment describing the 8-entry hart that sat above the 64-entry one. --- .../pmp_noentry_umode_test.S | 45 +++++++++++++++++++ src/iss/mem/pmp.h | 4 +- src/sysc/register_cores.cpp | 9 +++- src/vm/interp/vm_rv64gc_mp.cpp | 32 +++++++++++-- 4 files changed, 84 insertions(+), 6 deletions(-) create mode 100644 contrib/fw/pmp-noentry-umode-test/pmp_noentry_umode_test.S diff --git a/contrib/fw/pmp-noentry-umode-test/pmp_noentry_umode_test.S b/contrib/fw/pmp-noentry-umode-test/pmp_noentry_umode_test.S new file mode 100644 index 0000000..6a6443a --- /dev/null +++ b/contrib/fw/pmp-noentry-umode-test/pmp_noentry_umode_test.S @@ -0,0 +1,45 @@ +// With PMP entries implemented but none enabled, an S/U-mode access must fail. +// +// The privileged spec says that if no PMP entry matches an M-mode access the +// access succeeds, but if no entry matches an S/U-mode access while at least one +// entry is implemented, the access fails. pmp_check short-circuited on +// `if(!any_active) return true;`, which allowed the access at any privilege +// level, contradicting the fall-through at the end of the function that already +// returned PRIV == PRIV_M. +// +// This needs a hart that implements U mode: the M-only wrappers pin PRIV to +// PRIV_M, where the buggy and correct results coincide. Run with rv64gc_mup. +// +// No PMP entry is programmed here, so any_active stays false. After mret the +// first U-mode instruction fetch must already be denied. +// +// Pass: the U-mode access traps back to M mode -> j . (exit 0) +// Fail: U mode runs unrestricted -> semihosting SYS_EXIT (exit 2) + +#include "pmp_test_common.h" + +.section .text +.globl _start +_start: + // Any trap from U mode lands here and means PMP denied the access -> PASS + la t0, pmp_fault + csrw mtvec, t0 + + // mstatus.MPP = U (0) so mret drops to user mode + li t0, 3 + slli t0, t0, 11 // MPP mask, bits 12:11 + csrc mstatus, t0 + + la t0, umode_entry + csrw mepc, t0 + mret + + // Only reached if mret did not transfer control at all + semihosting_fail + +pmp_fault: + j . // U-mode access denied as expected -> PASS + +umode_entry: + // Reached only if U mode was allowed to run with no PMP entry enabled. + semihosting_fail diff --git a/src/iss/mem/pmp.h b/src/iss/mem/pmp.h index abcbdb9..680b9ba 100644 --- a/src/iss/mem/pmp.h +++ b/src/iss/mem/pmp.h @@ -167,8 +167,10 @@ template struct pmp : public memory_ele }; template bool pmp::pmp_check(access_type type, uint64_t addr, unsigned len) { + // No entry can match, so this is the no-match outcome: M mode is unrestricted while + // S/U mode fails, since entries are implemented. Matches the fall-through below. if(!any_active) - return true; + return hart_if.PRIV == arch::PRIV_M; reg_t base = 0; for(size_t i = 0; i < NUM_ENTRIES; i++) { reg_t tor = pmpaddr[i] << PMP_SHIFT; diff --git a/src/sysc/register_cores.cpp b/src/sysc/register_cores.cpp index db74446..721c7d5 100644 --- a/src/sysc/register_cores.cpp +++ b/src/sysc/register_cores.cpp @@ -50,7 +50,7 @@ namespace iss { namespace interp { using namespace sysc; -__attribute__((used)) volatile std::array riscv_init = { +__attribute__((used)) volatile std::array riscv_init = { iss_factory::instance().register_creator("rv32i_m:interp", [](unsigned gdb_port, sysc::riscv::core_complex_if* cc) -> iss_factory::base_t { auto* cpu = new core2sc_adapter>(cc); @@ -151,6 +151,13 @@ __attribute__((used)) volatile std::array riscv_init = { auto* cpu = new core2sc_adapter>(cc); return {sysc::core_ptr{cpu}, vm_ptr{create(static_cast(cpu), gdb_port)}}; }), + iss_factory::instance().register_creator("rv64gc_mup:interp", // rv64gc_mu with PMP + [](unsigned gdb_port, sysc::riscv::core_complex_if* cc) -> iss_factory::base_t { + auto* cpu = new core2sc_adapter>(cc); + cpu->memories.insert_before_last( + std::make_unique>(cpu->get_priv_if())); + return {sysc::core_ptr{cpu}, vm_ptr{create(static_cast(cpu), gdb_port)}}; + }), iss_factory::instance().register_creator("rv64gc_msu:interp", [](unsigned gdb_port, sysc::riscv::core_complex_if* cc) -> iss_factory::base_t { auto* cpu = new core2sc_adapter>(cc); diff --git a/src/vm/interp/vm_rv64gc_mp.cpp b/src/vm/interp/vm_rv64gc_mp.cpp index 86eefaf..73d50b4 100644 --- a/src/vm/interp/vm_rv64gc_mp.cpp +++ b/src/vm/interp/vm_rv64gc_mp.cpp @@ -3,6 +3,7 @@ #include #include #include +#include #include #include #include @@ -24,7 +25,7 @@ struct rv64gc_mp_hart : public arch::riscv_hart_m_p { } }; -// rv64gc_mp_8_hart: same as rv64gc_mp but with only 8 PMP entries (models SiFive S5). +// rv64gc_mp_64_hart: same wiring, with the full 64 PMP entries. struct rv64gc_mp_64_hart : public arch::riscv_hart_m_p { mem::pmp pmp_obj{this->get_priv_if()}; @@ -34,6 +35,7 @@ struct rv64gc_mp_64_hart : public arch::riscv_hart_m_p { } }; +// rv64gc_mp_8_hart: only 8 PMP entries (models SiFive S5). struct rv64gc_mp_8_hart : public arch::riscv_hart_m_p { mem::pmp pmp_obj{this->get_priv_if()}; @@ -43,9 +45,21 @@ struct rv64gc_mp_8_hart : public arch::riscv_hart_m_p { } }; +// rv64gc_mup_hart: PMP on a hart that also implements U mode, so the privilege +// dependent parts of pmp_check are reachable. The M-only wrappers pin PRIV to +// PRIV_M, which makes any S/U behaviour untestable. +struct rv64gc_mup_hart : public arch::riscv_hart_mu_p { + mem::pmp pmp_obj{this->get_priv_if()}; + + rv64gc_mup_hart() { + pmp_obj.set_next(this->default_mem.get_mem_if()); + memory = pmp_obj.get_mem_if(); + } +}; + namespace { -volatile std::array rv64gc_mp_dummy = { +volatile std::array rv64gc_mp_dummy = { core_factory::instance().register_creator("rv64gc_mp:interp", [](unsigned port, void* init_data) -> std::tuple { auto* cpu = new rv64gc_mp_hart(); @@ -66,8 +80,18 @@ volatile std::array rv64gc_mp_dummy = { } return {cpu_ptr{cpu}, vm_ptr{iss::interp::create(cpu, port, false)}}; }), - core_factory::instance().register_creator("rv64gc_mp_8:interp", [](unsigned port, void* init_data) -> std::tuple { - auto* cpu = new rv64gc_mp_8_hart(); + core_factory::instance().register_creator("rv64gc_mp_8:interp", + [](unsigned port, void* init_data) -> std::tuple { + auto* cpu = new rv64gc_mp_8_hart(); + if(init_data) { + auto* cb = + reinterpret_cast::reg_t>*>(init_data); + cpu->set_semihosting_callback(*cb); + } + return {cpu_ptr{cpu}, vm_ptr{iss::interp::create(cpu, port, false)}}; + }), + core_factory::instance().register_creator("rv64gc_mup:interp", [](unsigned port, void* init_data) -> std::tuple { + auto* cpu = new rv64gc_mup_hart(); if(init_data) { auto* cb = reinterpret_cast::reg_t>*>(init_data); cpu->set_semihosting_callback(*cb); From 90a6a987cdd4a501202248a9c634373e895ac112 Mon Sep 17 00:00:00 2001 From: Chris Hurley Date: Tue, 8 Sep 2026 15:52:06 -0700 Subject: [PATCH 09/11] ci: run the four new PMP tests in the functional test job Three of them run on rv64gc_mp_64 alongside the existing tests. The no-entry U-mode test needs rv64gc_mup, since PMP behaviour that depends on privilege level cannot be observed on an M-only hart. --- .github/workflows/ci.yml | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c1c2a46..0e9d5d7 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -179,3 +179,43 @@ jobs: - name: rv64gc_mp_8 8-entry enforcement test - interp (VP/S5 model) run: LD_LIBRARY_PATH=. ./riscv-sim -f pmp_8entry_guard_test --isa rv64gc_mp_8 --backend interp + + - name: Build PMP fetch deny test firmware + run: | + riscv64-unknown-elf-gcc -nostdlib -march=rv64gc -mabi=lp64 -I contrib/fw \ + -Wl,-Ttext=0x10000,--no-dynamic-linker \ + -o pmp_fetch_deny_test \ + contrib/fw/pmp-fetch-deny-test/pmp_fetch_deny_test.S + + - name: rv64gc_mp_64 fetch execute-permission test - interp + run: LD_LIBRARY_PATH=. ./riscv-sim -f pmp_fetch_deny_test --isa rv64gc_mp_64 --backend interp + + - name: Build PMP fetch straddle test firmware + run: | + riscv64-unknown-elf-gcc -nostdlib -march=rv64gc -mabi=lp64 -I contrib/fw \ + -Wl,-Ttext=0x10000,--no-dynamic-linker \ + -o pmp_fetch_straddle_test \ + contrib/fw/pmp-fetch-straddle-test/pmp_fetch_straddle_test.S + + - name: rv64gc_mp_64 straddling-fetch sector test - interp + run: LD_LIBRARY_PATH=. ./riscv-sim -f pmp_fetch_straddle_test --isa rv64gc_mp_64 --backend interp + + - name: Build PMP lock test firmware + run: | + riscv64-unknown-elf-gcc -nostdlib -march=rv64gc -mabi=lp64 -I contrib/fw \ + -Wl,-Ttext=0x10000,--no-dynamic-linker \ + -o pmp_lock_test \ + contrib/fw/pmp-lock-test/pmp_lock_test.S + + - name: rv64gc_mp_64 lock-bit test - interp + run: LD_LIBRARY_PATH=. ./riscv-sim -f pmp_lock_test --isa rv64gc_mp_64 --backend interp + + - name: Build PMP no-entry U-mode test firmware + run: | + riscv64-unknown-elf-gcc -nostdlib -march=rv64gc -mabi=lp64 -I contrib/fw \ + -Wl,-Ttext=0x10000,--no-dynamic-linker \ + -o pmp_noentry_umode_test \ + contrib/fw/pmp-noentry-umode-test/pmp_noentry_umode_test.S + + - name: rv64gc_mup no-entry U-mode denial test - interp + run: LD_LIBRARY_PATH=. ./riscv-sim -f pmp_noentry_umode_test --isa rv64gc_mup --backend interp From 8a74c2efa972f165ca8f5a226d272876171af04b Mon Sep 17 00:00:00 2001 From: Chris Hurley Date: Wed, 9 Sep 2026 15:58:12 -0700 Subject: [PATCH 10/11] test(pmp): align trap handlers so their bodies actually run mtvec holds the vectoring mode in its low two bits, so a handler at a 2-byte-aligned address has its address truncated and the hart vectors two bytes early. That lands inside the preceding semihosting_fail, whose trailing `j .` is the pass condition, so a misaligned handler lets a test pass without executing any of its body. Five of the thirteen tests were affected. Their verdicts were still correct, since the allowed path exits through semihosting, but any handler logic was dead. Handlers now come from a trap_entry macro that aligns them and records why. --- .../pmp_64entry_addr_test.S | 2 +- .../pmp-64entry-cfg-test/pmp_64entry_cfg_test.S | 2 +- .../pmp_8entry_guard_test.S | 4 ++-- contrib/fw/pmp-cfg2-test/pmp_cfg2_test.S | 2 +- contrib/fw/pmp-csr-test/pmp_csr_test.S | 2 +- contrib/fw/pmp-enforce-test/pmp_enforce_test.S | 4 ++-- .../pmp-fetch-deny-test/pmp_fetch_deny_test.S | 17 ++++++++++++----- .../pmp_fetch_straddle_test.S | 12 ++++++++---- contrib/fw/pmp-lock-test/pmp_lock_test.S | 2 +- .../pmp_noentry_umode_test.S | 2 +- contrib/fw/pmp-shift-test/pmp_shift_test.S | 4 ++-- contrib/fw/pmp-tor-test/pmp_tor_test.S | 4 ++-- contrib/fw/pmp_test_common.h | 10 ++++++++++ 13 files changed, 44 insertions(+), 23 deletions(-) diff --git a/contrib/fw/pmp-64entry-addr-test/pmp_64entry_addr_test.S b/contrib/fw/pmp-64entry-addr-test/pmp_64entry_addr_test.S index 7f4e4f0..670ea9d 100644 --- a/contrib/fw/pmp-64entry-addr-test/pmp_64entry_addr_test.S +++ b/contrib/fw/pmp-64entry-addr-test/pmp_64entry_addr_test.S @@ -17,5 +17,5 @@ _start: j . // pass -fail: +trap_entry fail semihosting_fail diff --git a/contrib/fw/pmp-64entry-cfg-test/pmp_64entry_cfg_test.S b/contrib/fw/pmp-64entry-cfg-test/pmp_64entry_cfg_test.S index b0527ac..545d69b 100644 --- a/contrib/fw/pmp-64entry-cfg-test/pmp_64entry_cfg_test.S +++ b/contrib/fw/pmp-64entry-cfg-test/pmp_64entry_cfg_test.S @@ -27,5 +27,5 @@ _start: j . // pass -fail: +trap_entry fail semihosting_fail diff --git a/contrib/fw/pmp-8entry-guard-test/pmp_8entry_guard_test.S b/contrib/fw/pmp-8entry-guard-test/pmp_8entry_guard_test.S index da4afcb..1bc9081 100644 --- a/contrib/fw/pmp-8entry-guard-test/pmp_8entry_guard_test.S +++ b/contrib/fw/pmp-8entry-guard-test/pmp_8entry_guard_test.S @@ -30,10 +30,10 @@ _start: semihosting_fail // no fault -> FAIL -phase1_trap: +trap_entry phase1_trap semihosting_fail // CSR trap -> FAIL -phase2_trap: +trap_entry phase2_trap j . // load denied as expected -> PASS .align 2 diff --git a/contrib/fw/pmp-cfg2-test/pmp_cfg2_test.S b/contrib/fw/pmp-cfg2-test/pmp_cfg2_test.S index 439632b..bcbe967 100644 --- a/contrib/fw/pmp-cfg2-test/pmp_cfg2_test.S +++ b/contrib/fw/pmp-cfg2-test/pmp_cfg2_test.S @@ -20,5 +20,5 @@ _start: j . // pass: ISS detects j . and exits 0 -fail: +trap_entry fail semihosting_fail diff --git a/contrib/fw/pmp-csr-test/pmp_csr_test.S b/contrib/fw/pmp-csr-test/pmp_csr_test.S index ac01397..0a603c0 100644 --- a/contrib/fw/pmp-csr-test/pmp_csr_test.S +++ b/contrib/fw/pmp-csr-test/pmp_csr_test.S @@ -20,7 +20,7 @@ _start: j . // pass: ISS detects j . and exits 0 -fail: +trap_entry fail // Semihosting SYS_EXIT: the ISS checks for slli+ebreak+srai and intercepts // before dispatching to mtvec. Must use 4-byte ebreak (not c.ebreak) so the // check lands at the correct offsets (-4 and +4 from the ebreak address). diff --git a/contrib/fw/pmp-enforce-test/pmp_enforce_test.S b/contrib/fw/pmp-enforce-test/pmp_enforce_test.S index da31a0d..0b8a62c 100644 --- a/contrib/fw/pmp-enforce-test/pmp_enforce_test.S +++ b/contrib/fw/pmp-enforce-test/pmp_enforce_test.S @@ -32,10 +32,10 @@ _start: semihosting_fail // no fault: enforcement did not fire -> FAIL -phase1_trap: +trap_entry phase1_trap semihosting_fail // CSR trapped: PMP unavailable -> FAIL -phase2_trap: +trap_entry phase2_trap j . // load denied as expected -> PASS .align 2 diff --git a/contrib/fw/pmp-fetch-deny-test/pmp_fetch_deny_test.S b/contrib/fw/pmp-fetch-deny-test/pmp_fetch_deny_test.S index 71feff0..8966ad1 100644 --- a/contrib/fw/pmp-fetch-deny-test/pmp_fetch_deny_test.S +++ b/contrib/fw/pmp-fetch-deny-test/pmp_fetch_deny_test.S @@ -9,8 +9,8 @@ // X permission. L=1 is required because pmp_check grants M-mode unconditionally // when L=0. // -// Pass: fetch of target is denied -> fault fires -> j . (exit 0) -// Fail: any CSR trap, OR the fetch succeeds -> semihosting SYS_EXIT (exit 2) +// Pass: fetch of target is denied and reports an instruction access fault -> j . (exit 0) +// Fail: any CSR trap, the fetch succeeds, or the cause is wrong -> semihosting SYS_EXIT (exit 2) #include "pmp_test_common.h" @@ -35,11 +35,18 @@ _start: jr t5 // fetch from target: PMP must deny (no X) -csr_trap: +trap_entry csr_trap semihosting_fail // CSR trapped: PMP unavailable -> FAIL -pmp_fault: - j . // fetch denied as expected -> PASS +trap_entry pmp_fault + // A denied fetch is an instruction access fault, not a load access fault. + csrr t1, mcause + li t2, 1 // RV_CAUSE_FETCH_ACCESS + bne t1, t2, fail + j . // fetch denied with the correct cause -> PASS + +fail: + semihosting_fail .align 2 // 4-byte align so the NA4 region covers this exactly target: diff --git a/contrib/fw/pmp-fetch-straddle-test/pmp_fetch_straddle_test.S b/contrib/fw/pmp-fetch-straddle-test/pmp_fetch_straddle_test.S index 1e90eb1..4a6b18c 100644 --- a/contrib/fw/pmp-fetch-straddle-test/pmp_fetch_straddle_test.S +++ b/contrib/fw/pmp-fetch-straddle-test/pmp_fetch_straddle_test.S @@ -19,8 +19,8 @@ // fall-through allows the fetch, the jump runs -> FAIL. // Fixed: sector base+4 matches, the overlap is partial -> denied -> PASS. // -// Pass: fetch of the straddling instruction is denied -> j . (exit 0) -// Fail: any CSR trap, OR the fetch succeeds -> semihosting SYS_EXIT (exit 2) +// Pass: the straddling fetch is denied and reports an instruction access fault -> j . (exit 0) +// Fail: any CSR trap, the fetch succeeds, or the cause is wrong -> semihosting SYS_EXIT (exit 2) #include "pmp_test_common.h" @@ -46,10 +46,14 @@ _start: jr t5 // enter at straddle_base, which is 4-byte aligned -csr_trap: +trap_entry csr_trap semihosting_fail // CSR trapped: PMP unavailable -> FAIL -pmp_fault: +trap_entry pmp_fault + // A denied fetch is an instruction access fault, not a load access fault. + csrr t1, mcause + li t2, 1 // RV_CAUSE_FETCH_ACCESS + bne t1, t2, fetch_allowed j . // straddling fetch denied as expected -> PASS fetch_allowed: diff --git a/contrib/fw/pmp-lock-test/pmp_lock_test.S b/contrib/fw/pmp-lock-test/pmp_lock_test.S index 75ee06a..13240c0 100644 --- a/contrib/fw/pmp-lock-test/pmp_lock_test.S +++ b/contrib/fw/pmp-lock-test/pmp_lock_test.S @@ -62,5 +62,5 @@ _start: j . // every locked value retained -> PASS -fail: +trap_entry fail semihosting_fail diff --git a/contrib/fw/pmp-noentry-umode-test/pmp_noentry_umode_test.S b/contrib/fw/pmp-noentry-umode-test/pmp_noentry_umode_test.S index 6a6443a..e5eef41 100644 --- a/contrib/fw/pmp-noentry-umode-test/pmp_noentry_umode_test.S +++ b/contrib/fw/pmp-noentry-umode-test/pmp_noentry_umode_test.S @@ -37,7 +37,7 @@ _start: // Only reached if mret did not transfer control at all semihosting_fail -pmp_fault: +trap_entry pmp_fault j . // U-mode access denied as expected -> PASS umode_entry: diff --git a/contrib/fw/pmp-shift-test/pmp_shift_test.S b/contrib/fw/pmp-shift-test/pmp_shift_test.S index 4bfd69f..2c13f3f 100644 --- a/contrib/fw/pmp-shift-test/pmp_shift_test.S +++ b/contrib/fw/pmp-shift-test/pmp_shift_test.S @@ -34,10 +34,10 @@ _start: semihosting_fail // no fault: enforcement did not fire -> FAIL -phase1_trap: +trap_entry phase1_trap semihosting_fail // CSR trapped: PMP unavailable -> FAIL -phase2_trap: +trap_entry phase2_trap j . // load denied as expected -> PASS .align 2 diff --git a/contrib/fw/pmp-tor-test/pmp_tor_test.S b/contrib/fw/pmp-tor-test/pmp_tor_test.S index 5c2499d..02de520 100644 --- a/contrib/fw/pmp-tor-test/pmp_tor_test.S +++ b/contrib/fw/pmp-tor-test/pmp_tor_test.S @@ -39,10 +39,10 @@ _start: ld t2, 0(t5) // No fault fired -> bug present -fail: +trap_entry fail semihosting_fail -pmp_fault: +trap_entry pmp_fault j . // load denied as expected -> PASS .align 3 // 8-byte align for ld diff --git a/contrib/fw/pmp_test_common.h b/contrib/fw/pmp_test_common.h index 0e13eb7..a430e95 100644 --- a/contrib/fw/pmp_test_common.h +++ b/contrib/fw/pmp_test_common.h @@ -23,4 +23,14 @@ j . // fallback if semihosting is not configured .endm +// Start a trap handler. mtvec's low two bits hold the vectoring mode, so a +// handler that is only 2-byte aligned has its address truncated and the hart +// vectors two bytes early. That lands inside a preceding semihosting_fail, whose +// trailing `j .` reads as the pass condition, so a misaligned handler makes a +// test pass without its body ever running. Always declare handlers with this. +.macro trap_entry name + .align 2 +\name\(): +.endm + #endif // PMP_TEST_COMMON_H From e7c6ecb7b89779824910a4c8087976c238ab9450 Mon Sep 17 00:00:00 2001 From: Chris Hurley Date: Wed, 9 Sep 2026 15:58:55 -0700 Subject: [PATCH 11/11] fix: report an instruction access fault for a denied fetch riscv_hart_m_p::read set RV_CAUSE_LOAD_ACCESS unconditionally when the memory chain returned an error, so a PMP-denied instruction fetch surfaced as a load access fault. RV_CAUSE_FETCH_ACCESS was consumed by the trap dispatch switch but never assigned in this wrapper. riscv_hart_mu_p and riscv_hart_msu_vp already selected the cause on is_fetch, so this was an oversight in the M-mode wrapper alone; it now matches them. The two fetch tests only asserted that some trap fired, which is why this went unnoticed; both now assert the cause. --- src/iss/arch/riscv_hart_m_p.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/iss/arch/riscv_hart_m_p.h b/src/iss/arch/riscv_hart_m_p.h index de83a35..bb7fb09 100644 --- a/src/iss/arch/riscv_hart_m_p.h +++ b/src/iss/arch/riscv_hart_m_p.h @@ -187,7 +187,8 @@ iss::status riscv_hart_m_p::read(const addr_t& a, const unsigned len } auto res = this->memory.rd_mem({address_type::PHYSICAL, a.access, a.space, a.val}, length, data); if(unlikely(res != iss::Ok && (access & access_type::DEBUG) == 0)) { - this->reg.trap_state = (1UL << 31) | traits::RV_CAUSE_LOAD_ACCESS << 16; + auto trap_id = is_fetch(a.access) ? traits::RV_CAUSE_FETCH_ACCESS : traits::RV_CAUSE_LOAD_ACCESS; + this->reg.trap_state = (1UL << 31) | trap_id << 16; this->fault_data = addr; } return res;