PmpPluginOld: fix NAPOT address calculation overflow issue

[lschuermann]

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.

[lschuermann]

Notably, the PmpPlugin is not affected by this same bug, as it uses a different logic for address comparison.

A comment / question orthogonal to this change: the PmpPlugin has been introduced as it requires less resources and has a less significant impact on the timing closure compared to the PmpPluginOld, by deliberately only supporting NAPOT (nearest power of two) addressing.

However, as far as I can tell, this is not at all RISC-V privileged spec compliant. Even if spec compliance were not important for VexRiscv, it can be a rather tricky issue to debug why existing software cannot use the VexRiscv PMP. To developers, the symptoms of using such a partial PMP implementation are often indistinguishable from an entirely broken implementation, and may -- in the worst case -- silently configure the system into an insecure state.

Would it be possible to change the full-featured PmpPluginOld to be the default, and hide the PmpPlugin (perhaps renamed to PmpPluginNAPOT, to better reflect its tradeoffs) behind a flag? Perhaps we can introduce a flag pmp-addressing-modes, which by default includes all of TOR, NAPOT and NA4. Only when users explicitly indicate that they don't need TOR or NA4 addressing could we fall onto the more efficient NAPOT-only implementation.

In general, I believe that it's better for such a security-critical plugin to generate more capable but slower/expensive hardware by default, and provide users knobs to deliberately disable features / diverge from the spec if they so desire. What do you think @Dolu1990 @lindemer?

[Dolu1990]

Hi,

Would it be possible to change the full-featured PmpPluginOld to be the default, and hide the PmpPlugin (perhaps renamed to PmpPluginNAPOT, to better reflect its tradeoffs) behind a flag?

This sound reasonable, especialy "PmpPluginNapot"

Even if spec compliance were not important for VexRiscv

Full spec compliance isn't important for VexRiscv.
One issue of the RISC-V privileged spec, is that often it only see things from the ASIC perspective, where adding CSR is mostly free.
I think that non-compliance is fine as long it is clear to the user.

Perhaps we can introduce a flag pmp-addressing-modes

Do you mean it from a litex perspective ?

[lschuermann]

Do you mean it from a litex perspective ?

Oh, yes, I'm sorry. I always forget that the GenCoreDefault is shipped in the pythondata-cpu-vexriscv repository itself! I will propose those changes there then.

This sound reasonable, especialy "PmpPluginNapot"

Great! I suppose it'd be better for me to open a separate PR for that, right? I'll happily do that after this fix is merged.

[Dolu1990]

Great! I suppose it'd be better for me to open a separate PR for that, right? I'll happily do that after this fix is merged.

Sound good :)

Thanks !