Fix/pmp correctness - #57
Draft
cphurley82 wants to merge 11 commits into
Draft
Conversation
The block referenced symbols that no longer exist (hart_if.reg.PRIV, csr[pmpcfg0+...], bit_sub) so it could not be revived as written.
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.
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.
read_mem gated on
addr.space == traits<PLAT>::MEM || std::numeric_limits<decltype(phys_addr_t::space)>::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.
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.
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.
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_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.
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.
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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix five PMP correctness bugs, with firmware tests for each
Five defects in
src/iss/mem/pmp.h. Each fix is its own commit, paired with thefirmware test that demonstrates it, and every new test was confirmed to fail
before its fix and pass after.
read_memIMEMcomparisonpmp_checkpmp_checkwrite_pmpcfg/write_pmpaddrread_mem/write_memOne existing test changed
pmp-64entry-cfg-testprobedpmpcfg4with0x9Fand then asserted it could becleared back to zero.
0x9Fhas the lock bit set, so that assertion only heldwhile the lock was unenforced. The test exists to show that
pmpcfg4isregistered and persistent for entries 32 to 39, which
0x1Fdemonstrates equallywell without locking the entry.
New core:
rv64gc_mupEvery PMP-enabled core so far wraps
riscv_hart_m_p, wherePRIVis alwaysPRIV_M. On those harts the no-active-entry defect is invisible, becausereturn trueandreturn PRIV == PRIV_Magree.rv64gc_muppairsriscv_hart_mu_pwith PMP so U mode is reachable and the privilege-dependentpaths can be tested. Registered for both the standalone simulator and the
SystemC wrapper, following the existing variants.
Tests
Four new firmware tests, wired into the existing functional test job:
pmp-fetch-deny-testlocks an NA4 region with no X over a routine and jumpsinto it. First coverage of the fetch path and the X bit.
pmp-fetch-straddle-testplaces a 4-byte jump ataddr % 4 == 2with a denyregion over the upper sector only, so the fetch partially overlaps it.
pmp-lock-testcovers a locked config byte, a locked address register, and thelocked-TOR lower bound.
pmp-noentry-umode-testdrops to U mode with no entry programmed.The duplicated semihosting exit block, previously copied into all nine tests, is
now shared from
contrib/fw/pmp_test_common.h; each of those tests assembles tobyte-identical code. All thirteen tests plus the no-PMP negative control pass.