From 9baba6d11fd621302592a8a87892cb3c6520e32b Mon Sep 17 00:00:00 2001
From: Leon Schuermann <leon@is.currently.online>
Date: Fri, 3 Nov 2023 09:11:42 -0400
Subject: [PATCH] PmpPluginOld: fix NAPOT address calculation overflow issue

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]: https://github.com/tock/tock/pull/3597
---
 .../scala/vexriscv/plugin/PmpPluginOld.scala  | 35 ++++++++++++++++---
 1 file changed, 30 insertions(+), 5 deletions(-)

diff --git a/src/main/scala/vexriscv/plugin/PmpPluginOld.scala b/src/main/scala/vexriscv/plugin/PmpPluginOld.scala
index 0426902..2f0f6d1 100644
--- a/src/main/scala/vexriscv/plugin/PmpPluginOld.scala
+++ b/src/main/scala/vexriscv/plugin/PmpPluginOld.scala
@@ -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) {
@@ -114,9 +136,12 @@ 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
 
   // PMP changes take effect two clock cycles after the initial CSR write (i.e.,
   // settings propagate from csr -> state -> region).
@@ -135,7 +160,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