Skip to content

Commit

Permalink
PmpPluginOld: fix NAPOT address calculation overflow issue
Browse files Browse the repository at this point in the history
Because pmpaddrX registers are defined to encode the address'
[XLEN + 2 downto 2] bits, the length of a NAPOT region is defined
through the most significant 0 bit in a pmpaddrX register (which in
the case of ~0 is the 33rd non-existant "virtual" bit), and the
VexRiscv PmpOld plugin represents the addresses covered by a region as
[start; end) (bounded inclusively below and exclusively above), the
start and end address registers need to be XLEN + 4 bit wide to avoid
overflows.

If such an overflow occurs, it may be that the region does not cover
any address, an issue uncovered in the Tock LiteX + VexRiscv CI during
a PMP infrastructure redesign in the Tock OS [1].

This commit has been tested on Tock's redesigned PMP infrastructure,
and by inspecting all of the intermediate signals in the PMP address
calculation through a Verilator trace file. It works correctly for
various NAPOT and TOR addresses, and I made sure that the edge cases
of pmpaddrX = [0x00000000, 0x7FFFFFFF, 0xFFFFFFFF] are all handled.

[1]: tock/tock#3597
  • Loading branch information
lschuermann committed Nov 3, 2023
1 parent b6f6120 commit cae3a56
Showing 1 changed file with 34 additions and 5 deletions.
39 changes: 34 additions & 5 deletions src/main/scala/vexriscv/plugin/PmpPluginOld.scala
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,29 @@ case class PmpRegister(previous : PmpRegister) extends Area {
// Computed PMP region bounds
val region = new Area {
val valid, locked = Bool
val start, end = UInt(32 bits)

// The calculated start & end addresses can overflow xlen by 4 bit:
//
// - 2 bit, as the pmpaddrX registers are defined as to encode
// [XLEN + 2 downto 2] addresses.
//
// - 2 bit, as for NAPOT the most significant 0 bit encodes the region
// length, with this bit included in the range!
//
// This means that (for xlen == 32 bit)
//
// pmpcfg(X / 4)(X % 4) = NAPOT
// pmpaddrX = 0xFFFFFFFF
//
// will expand to
//
// start (inclusive): 0x000000000 << 2
// end (exclusive): 0x200000000 << 2
//
// hence requiring xlen + 2 + 2 bit to represent the exclusive end
// address. This could be optimized by using a saturating add, or making the
// end address exclusive.
val start, end = UInt(36 bits)
}

when(~state.l) {
Expand All @@ -114,9 +136,16 @@ case class PmpRegister(previous : PmpRegister) extends Area {
}
}

val shifted = state.addr |<< 2
val mask = state.addr & ~(state.addr + 1)
val masked = (state.addr & ~mask) |<< 2
// Extend state.addr to 36 bits, to avoid these computations overflowing (as
// explained above):
val extended_addr = (B"00" ## state.addr.asBits).asUInt
val shifted = extended_addr << 2
val mask = extended_addr ^ (extended_addr + 1)
val masked = (extended_addr & ~mask) << 2

region.shifted := shifted
region.mask := mask
region.masked := masked

// PMP changes take effect two clock cycles after the initial CSR write (i.e.,
// settings propagate from csr -> state -> region).
Expand All @@ -135,7 +164,7 @@ case class PmpRegister(previous : PmpRegister) extends Area {
}
is(NAPOT) {
region.start := masked
region.end := masked + ((mask + 1) |<< 3)
region.end := masked + ((mask + 1) << 2)
}
default {
region.start := 0
Expand Down

0 comments on commit cae3a56

Please sign in to comment.