Skip to content

fix: pmp sector walk hang on accesses at the top of the address space #61

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
cphurley82 wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix: pmp sector walk hang on accesses at the top of the address space #61

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
949b731
ci: bound PMP functional test runs with a timeout
Oct 1, 2026
be59080
fix(pmp): terminate the sector walk at the top of the address space
Oct 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 25 additions & 14 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ jobs:
name: PMP Functional Tests
runs-on: ubuntu-24.04
needs: cpp-compliance
timeout-minutes: 10
steps:
- uses: actions/checkout@v4

Expand All @@ -94,11 +95,11 @@ jobs:

- name: rv64gc_m CSR test - interp (no PMP, expect exit 2)
run: |
LD_LIBRARY_PATH=. ./riscv-sim -f pmp_csr_test --isa rv64gc_m --backend interp || rc=$?
LD_LIBRARY_PATH=. timeout 60 ./riscv-sim -f pmp_csr_test --isa rv64gc_m --backend interp || rc=$?
[ "${rc:-0}" -eq 2 ]

- name: rv64gc_mp_64 CSR test - interp (with PMP)
run: LD_LIBRARY_PATH=. ./riscv-sim -f pmp_csr_test --isa rv64gc_mp_64 --backend interp
run: LD_LIBRARY_PATH=. timeout 60 ./riscv-sim -f pmp_csr_test --isa rv64gc_mp_64 --backend interp

- name: Build PMP enforcement test firmware
run: |
Expand All @@ -108,7 +109,7 @@ jobs:
contrib/fw/pmp-enforce-test/pmp_enforce_test.S

- name: rv64gc_mp_64 enforcement test - interp
run: LD_LIBRARY_PATH=. ./riscv-sim -f pmp_enforce_test --isa rv64gc_mp_64 --backend interp
run: LD_LIBRARY_PATH=. timeout 60 ./riscv-sim -f pmp_enforce_test --isa rv64gc_mp_64 --backend interp

- name: Build PMP shift test firmware
run: |
Expand All @@ -118,7 +119,7 @@ jobs:
contrib/fw/pmp-shift-test/pmp_shift_test.S

- name: rv64gc_mp_64 shift test - interp
run: LD_LIBRARY_PATH=. ./riscv-sim -f pmp_shift_test --isa rv64gc_mp_64 --backend interp
run: LD_LIBRARY_PATH=. timeout 60 ./riscv-sim -f pmp_shift_test --isa rv64gc_mp_64 --backend interp

- name: Build PMP upper-cfg test firmware
run: |
Expand All @@ -128,7 +129,7 @@ jobs:
contrib/fw/pmp-upper-cfg-test/pmp_upper_cfg_test.S

- name: rv64gc_mp_64 upper-cfg test - interp
run: LD_LIBRARY_PATH=. ./riscv-sim -f pmp_upper_cfg_test --isa rv64gc_mp_64 --backend interp
run: LD_LIBRARY_PATH=. timeout 60 ./riscv-sim -f pmp_upper_cfg_test --isa rv64gc_mp_64 --backend interp

- name: Build PMP cfg2 test firmware
run: |
Expand All @@ -138,7 +139,7 @@ jobs:
contrib/fw/pmp-cfg2-test/pmp_cfg2_test.S

- name: rv64gc_mp_64 cfg2 test - interp
run: LD_LIBRARY_PATH=. ./riscv-sim -f pmp_cfg2_test --isa rv64gc_mp_64 --backend interp
run: LD_LIBRARY_PATH=. timeout 60 ./riscv-sim -f pmp_cfg2_test --isa rv64gc_mp_64 --backend interp

- name: Build PMP TOR test firmware
run: |
Expand All @@ -148,7 +149,7 @@ jobs:
contrib/fw/pmp-tor-test/pmp_tor_test.S

- name: rv64gc_mp_64 TOR test - interp
run: LD_LIBRARY_PATH=. ./riscv-sim -f pmp_tor_test --isa rv64gc_mp_64 --backend interp
run: LD_LIBRARY_PATH=. timeout 60 ./riscv-sim -f pmp_tor_test --isa rv64gc_mp_64 --backend interp

- name: Build PMP 64-entry pmpaddr test firmware
run: |
Expand All @@ -158,7 +159,7 @@ jobs:
contrib/fw/pmp-64entry-addr-test/pmp_64entry_addr_test.S

- name: rv64gc_mp_64 64-entry pmpaddr test - interp
run: LD_LIBRARY_PATH=. ./riscv-sim -f pmp_64entry_addr_test --isa rv64gc_mp_64 --backend interp
run: LD_LIBRARY_PATH=. timeout 60 ./riscv-sim -f pmp_64entry_addr_test --isa rv64gc_mp_64 --backend interp

- name: Build PMP 64-entry pmpcfg test firmware
run: |
Expand All @@ -168,7 +169,7 @@ jobs:
contrib/fw/pmp-64entry-cfg-test/pmp_64entry_cfg_test.S

- name: rv64gc_mp_64 64-entry pmpcfg test - interp
run: LD_LIBRARY_PATH=. ./riscv-sim -f pmp_64entry_cfg_test --isa rv64gc_mp_64 --backend interp
run: LD_LIBRARY_PATH=. timeout 60 ./riscv-sim -f pmp_64entry_cfg_test --isa rv64gc_mp_64 --backend interp

- name: Build PMP 8-entry guard test firmware (VP/S5 model)
run: |
Expand All @@ -178,7 +179,7 @@ jobs:
contrib/fw/pmp-8entry-guard-test/pmp_8entry_guard_test.S

- 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
run: LD_LIBRARY_PATH=. timeout 60 ./riscv-sim -f pmp_8entry_guard_test --isa rv64gc_mp_8 --backend interp

- name: Build PMP fetch deny test firmware
run: |
Expand All @@ -188,7 +189,7 @@ jobs:
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
run: LD_LIBRARY_PATH=. timeout 60 ./riscv-sim -f pmp_fetch_deny_test --isa rv64gc_mp_64 --backend interp

- name: Build PMP fetch straddle test firmware
run: |
Expand All @@ -198,7 +199,17 @@ jobs:
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
run: LD_LIBRARY_PATH=. timeout 60 ./riscv-sim -f pmp_fetch_straddle_test --isa rv64gc_mp_64 --backend interp

- name: Build PMP top-sector test firmware
run: |
riscv64-unknown-elf-gcc -nostdlib -march=rv64gc -mabi=lp64 -I contrib/fw \
-Wl,-Ttext=0x10000,--no-dynamic-linker \
-o pmp_top_sector_test \
contrib/fw/pmp-top-sector-test/pmp_top_sector_test.S

- name: rv64gc_mp_64 M-mode load from top 4-byte sector completes - interp
run: LD_LIBRARY_PATH=. timeout 60 ./riscv-sim -f pmp_top_sector_test --isa rv64gc_mp_64 --backend interp

- name: Build PMP lock test firmware
run: |
Expand All @@ -208,7 +219,7 @@ jobs:
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
run: LD_LIBRARY_PATH=. timeout 60 ./riscv-sim -f pmp_lock_test --isa rv64gc_mp_64 --backend interp

- name: Build PMP no-entry U-mode test firmware
run: |
Expand All @@ -218,4 +229,4 @@ jobs:
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
run: LD_LIBRARY_PATH=. timeout 60 ./riscv-sim -f pmp_noentry_umode_test --isa rv64gc_mup --backend interp
53 changes: 53 additions & 0 deletions contrib/fw/pmp-top-sector-test/pmp_top_sector_test.S
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
// PMP sector iteration must terminate when an access touches the top sector of
// the address space.
//
// pmp_check walked sectors with an inclusive address bound:
// for(cur_addr = first; cur_addr <= last_sector; cur_addr += 4)
// When last_sector is the top 4-byte sector, the increment past it wraps
// cur_addr to 0, which is again <= last_sector, so the loop never ends. The
// simulator hangs with no trap and no log.
//
// The walk only runs for entries that are enabled, so entry 0 is programmed as
// an NA4 region over guard_word, well away from the top. It is unlocked, and
// guard_word lives in .data so no instruction fetch can overlap it; the entry
// exists only to make the walk run. The load from the top sector then matches
// no entry and must fall through to the M-mode default, which allows it. The
// load then reaches the simulator's default memory, which returns arbitrary
// data for unpopulated pages without faulting, so the address does not need to
// be backed. The loaded value is not checked.
//
// Buggy: the sector walk for entry 0 never terminates -> hang (CI timeout).
// Fixed: no entry matches, the M-mode load completes -> PASS.
//
// Pass: the load completes without a trap -> j . (exit 0)
// Fail: any trap, CSR or load -> semihosting SYS_EXIT (exit 2)

#include "pmp_test_common.h"

.section .text
.globl _start
_start:
// Any trap means either PMP is unavailable or the load faulted -> FAIL
la t0, unexpected_trap
csrw mtvec, t0

// pmpaddr0 = guard_word >> 2: NA4 over one word nowhere near the top
la t1, guard_word
srli t1, t1, 2
csrw pmpaddr0, t1

// pmpcfg0 byte 0 = 0x10 = NA4, unlocked, no R/W/X
li t1, 0x10
csrw pmpcfg0, t1

li t0, -4 // 0xFFFF_FFFF_FFFF_FFFC, the top 4-byte sector
lw t1, 0(t0)
j . // the load completed -> PASS

trap_entry unexpected_trap
semihosting_fail // CSR or load trapped -> FAIL

.section .data
.align 2
guard_word:
.word 0
15 changes: 10 additions & 5 deletions src/iss/mem/pmp.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,17 +181,22 @@ template <typename PLAT, size_t NUM_ENTRIES> bool pmp<PLAT, NUM_ENTRIES>::pmp_ch
auto is_na4 = pmp_a == PMP_NA4;
reg_t mask = (pmpaddr[i] << 1) | (!is_na4);
mask = ~(mask & ~(mask + 1)) << PMP_SHIFT;
// 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
// Check every 4-byte sector the access touches: span counts bytes from the start of
// the first sector, so a misaligned access also covers its last sector. Fetches can
// be misaligned: 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.
// The bound is a byte count, not an address: an address bound cannot terminate
// when the last sector is the top of the address space, as the step past it wraps.
// The count is 64 bits wide so that it cannot wrap either, whatever len is.
auto any_match = false;
auto all_match = true;
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) {
reg_t first_sector = addr & ~(sector_size - 1);
uint64_t span = uint64_t(len) + (addr & (sector_size - 1));
for(uint64_t off = 0; off < span; off += sector_size) {
reg_t cur_addr = first_sector + off; // wraps to 0 past the top; off, not cur_addr, ends the loop
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;
Expand Down
Loading