Merge #2064

2064: pmp: add PMP struct r=hudson-ayers a=bradjc

### Pull Request Overview

The PMP wasn't working on the arty because the configure function wasn't actually iterating the PMP regions. This separates out the PMP from the config, and makes a lot more sense to me. And it works on the arty-e21 board.






### Testing Strategy

This pull request was tested by running on arty e21.


### TODO or Help Wanted

n/a


### Documentation Updated

- [x] Updated the relevant files in `/docs`, or no updates are required.

### Formatting

- [x] Ran `make prepush`.


Co-authored-by: Brad Campbell <bradjc5@gmail.com>

arch/rv32i/src/pmp.rs

@@ -56,6 +56,22 @@ register_bitfields![u8,
     ]
 ];
 
+/// Main PMP struct.
+pub struct PMP {
+    /// The application that the MPU was last configured for. Used (along with
+    /// the `is_dirty` flag) to determine if MPU can skip writing the
+    /// configuration to hardware.
+    last_configured_for: MapCell<AppId>,
+}
+
+impl PMP {
+    pub const unsafe fn new() -> PMP {
+        PMP {
+            last_configured_for: MapCell::empty(),
+        }
+    }
+}
+
 /// Struct storing configuration for a RISC-V PMP region.
 #[derive(Copy, Clone)]
 pub struct PMPRegion {
@@ -136,37 +152,28 @@ impl PMPRegion {
     }
 }
 
-pub trait PMPConfigType: Default + Copy + Clone + Sized {}
-
 /// Struct storing region configuration for RISCV PMP.
-pub struct PMPConfig<N: PMPConfigType> {
+pub struct PMPConfig {
     /// Array of PMP regions. Each region requires two physical entries.
-    regions: N,
+    regions: [Option<PMPRegion>; $x / 2],
     /// Indicates if the configuration has changed since the last time it was
     /// written to hardware.
     is_dirty: Cell<bool>,
-    /// The application that the MPU was last configured for. Used (along with
-    /// the `is_dirty` flag) to determine if MPU can skip writing the
-    /// configuration to hardware.
-    last_configured_for: MapCell<AppId>,
     /// Which region index is used for app memory (if it has been configured).
     app_memory_region: OptionalCell<usize>,
 }
 
-impl PMPConfigType for [Option<PMPRegion>; $x / 2] {}
-
-impl Default for PMPConfig<[Option<PMPRegion>; $x / 2]> {
+impl Default for PMPConfig {
     fn default() -> Self {
         PMPConfig {
             regions: [None; $x / 2],
             is_dirty: Cell::new(true),
-            last_configured_for: MapCell::empty(),
             app_memory_region: OptionalCell::empty(),
         }
     }
 }
 
-impl fmt::Display for PMPConfig<[Option<PMPRegion>; $x / 2]> {
+impl fmt::Display for PMPConfig {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         writeln!(f, "PMP regions:")?;
         for (n, region) in self.regions.iter().enumerate() {
@@ -179,7 +186,7 @@ impl fmt::Display for PMPConfig<[Option<PMPRegion>; $x / 2]> {
     }
 }
 
-impl PMPConfig<[Option<PMPRegion>; $x / 2]> {
+impl PMPConfig {
     fn unused_region_number(&self) -> Option<usize> {
         for (number, region) in self.regions.iter().enumerate() {
             if self.app_memory_region.contains(&number) {
@@ -234,18 +241,15 @@ impl PMPConfig<[Option<PMPRegion>; $x / 2]> {
     }
 }
 
-impl kernel::mpu::MPU for PMPConfig<[Option<PMPRegion>; $x / 2]> {
-    type MpuConfig = PMPConfig<[Option<PMPRegion>; $x / 2]>;
+impl kernel::mpu::MPU for PMP {
+    type MpuConfig = PMPConfig;
 
     fn enable_mpu(&self) {}
 
     fn disable_mpu(&self) {
-        // The length of `self.regions` here refers to the number of memory
-        // slices we can protect with the PMP. Each slice requires two PMP
-        // entries to protect, so this is half of the number physical hardware
-        // PMP configuration entries. Therefore, we double the number of regions
-        // to clear all the relevant `pmpcfg` entries.
-        for x in 0..(self.regions.len() * 2) {
+        // We want to disable all of the hardware entries, so we use `$x` here,
+        // and not `$x / 2`.
+        for x in 0..$x {
             match x % 4 {
                 0 => {
                     csr::CSR.pmpcfg[x / 4].modify(
@@ -300,7 +304,7 @@ impl kernel::mpu::MPU for PMPConfig<[Option<PMPRegion>; $x / 2]> {
     }
 
     fn number_total_regions(&self) -> usize {
-        self.regions.len()
+        $x / 2
     }
 
     fn allocate_region(
@@ -464,7 +468,7 @@ impl kernel::mpu::MPU for PMPConfig<[Option<PMPRegion>; $x / 2]> {
         // Skip PMP configuration if it is already configured for this app and the MPU
         // configuration of this app has not changed.
         if !last_configured_for_this_app || config.is_dirty.get() {
-            for (x, region) in self.regions.iter().enumerate() {
+            for (x, region) in config.regions.iter().enumerate() {
                 match region {
                     Some(r) => {
                         let cfg_val = r.cfg.value as u32;
@@ -514,6 +518,6 @@ impl kernel::mpu::MPU for PMPConfig<[Option<PMPRegion>; $x / 2]> {
             self.last_configured_for.put(*app_id);
         }
     }
-        }
-    };
+    }
+};
 }

chips/arty_e21_chip/src/chip.rs

@@ -14,7 +14,7 @@ extern "C" {
 }
 
 pub struct ArtyExx {
-    pmp: pmp::PMPConfig<[Option<pmp::PMPRegion>; 2]>,
+    pmp: pmp::PMP,
     userspace_kernel_boundary: rv32i::syscall::SysCall,
     clic: rv32i::clic::Clic,
 }
@@ -27,7 +27,7 @@ impl ArtyExx {
         let in_use_interrupts: u64 = 0x1FFFF0080;
 
         ArtyExx {
-            pmp: pmp::PMPConfig::default(),
+            pmp: pmp::PMP::new(),
             userspace_kernel_boundary: rv32i::syscall::SysCall::new(),
             clic: rv32i::clic::Clic::new(in_use_interrupts),
         }
@@ -106,7 +106,7 @@ impl ArtyExx {
 }
 
 impl kernel::Chip for ArtyExx {
-    type MPU = pmp::PMPConfig<[Option<pmp::PMPRegion>; 2]>;
+    type MPU = pmp::PMP;
     type UserspaceKernelBoundary = rv32i::syscall::SysCall;
     type SchedulerTimer = ();
     type WatchDog = ();

chips/e310x/src/chip.rs

@@ -19,15 +19,15 @@ PMPConfigMacro!(8);
 
 pub struct E310x<A: 'static + Alarm<'static>> {
     userspace_kernel_boundary: rv32i::syscall::SysCall,
-    pmp: PMPConfig<[Option<PMPRegion>; 4]>,
+    pmp: PMP,
     scheduler_timer: kernel::VirtualSchedulerTimer<A>,
 }
 
 impl<A: 'static + Alarm<'static>> E310x<A> {
     pub unsafe fn new(alarm: &'static A) -> Self {
         Self {
             userspace_kernel_boundary: rv32i::syscall::SysCall::new(),
-            pmp: PMPConfig::default(),
+            pmp: PMP::new(),
             scheduler_timer: kernel::VirtualSchedulerTimer::new(alarm),
         }
     }
@@ -54,7 +54,7 @@ impl<A: 'static + Alarm<'static>> E310x<A> {
 }
 
 impl<A: 'static + Alarm<'static>> kernel::Chip for E310x<A> {
-    type MPU = PMPConfig<[Option<PMPRegion>; 4]>;
+    type MPU = PMP;
     type UserspaceKernelBoundary = rv32i::syscall::SysCall;
     type SchedulerTimer = kernel::VirtualSchedulerTimer<A>;
     type WatchDog = ();

chips/earlgrey/src/chip.rs

@@ -27,15 +27,15 @@ pub const CHIP_FREQ: u32 = CONFIG.chip_freq;
 
 pub struct EarlGrey<A: 'static + Alarm<'static>> {
     userspace_kernel_boundary: SysCall,
-    pmp: PMPConfig<[Option<PMPRegion>; 2]>,
+    pmp: PMP,
     scheduler_timer: kernel::VirtualSchedulerTimer<A>,
 }
 
 impl<A: 'static + Alarm<'static>> EarlGrey<A> {
     pub unsafe fn new(alarm: &'static A) -> Self {
         Self {
             userspace_kernel_boundary: SysCall::new(),
-            pmp: PMPConfig::default(),
+            pmp: PMP::new(),
             scheduler_timer: kernel::VirtualSchedulerTimer::new(alarm),
         }
     }
@@ -110,7 +110,7 @@ impl<A: 'static + Alarm<'static>> EarlGrey<A> {
 }
 
 impl<A: 'static + Alarm<'static>> kernel::Chip for EarlGrey<A> {
-    type MPU = PMPConfig<[Option<PMPRegion>; 2]>;
+    type MPU = PMP;
     type UserspaceKernelBoundary = SysCall;
     type SchedulerTimer = kernel::VirtualSchedulerTimer<A>;
     type WatchDog = ();