Skip to content

Commit

Permalink
Merge #2021
Browse files Browse the repository at this point in the history 
2021: arch/rv32i: Don't sort PMP regions when we configure the MPU r=ppannuto a=alistair23

### Pull Request Overview

Previously we would sort the PMP regions everytime we configured the
MPU. This was wasteful in terms of instructions but also wasted stack
space, probably because we cloned the PMPConfig regions.

This patch changes the sorting to only happen after a change is made to
the regions. This should save the number of times we have to sort,
remove sorting out of the critical path and saves a lot of stack space.

See below for the stack changes:

    ### Release build ###
    ## Before this patch ##
    80     _ZN68_$LT$rv32i..pmp..PMPConfig$u20$as$u20$kernel..platform..mpu..MPU$GT$15allocate_region17hb3b05a589563feedE
    0     _ZN68_$LT$rv32i..pmp..PMPConfig$u20$as$u20$kernel..platform..mpu..MPU$GT$24update_app_memory_region17hd6078caa7928d4d8E
    592     _ZN68_$LT$rv32i..pmp..PMPConfig$u20$as$u20$kernel..platform..mpu..MPU$GT$13configure_mpu17h5967285bd6ec5881E

    ## After this patch ##
    80     _ZN68_$LT$rv32i..pmp..PMPConfig$u20$as$u20$kernel..platform..mpu..MPU$GT$15allocate_region17hb3b05a589563feedE
    16     _ZN68_$LT$rv32i..pmp..PMPConfig$u20$as$u20$kernel..platform..mpu..MPU$GT$24update_app_memory_region17hd6078caa7928d4d8E
    80     _ZN68_$LT$rv32i..pmp..PMPConfig$u20$as$u20$kernel..platform..mpu..MPU$GT$13configure_mpu17h5967285bd6ec5881E

    ### Debug build ###
    ## Before this patch ##
    80     _ZN68_$LT$rv32i..pmp..PMPConfig$u20$as$u20$kernel..platform..mpu..MPU$GT$15allocate_region17h272c094b5b64d63bE
    96     _ZN68_$LT$rv32i..pmp..PMPConfig$u20$as$u20$kernel..platform..mpu..MPU$GT$26allocate_app_memory_region17h56b3decc4c649f8cE
    32     _ZN68_$LT$rv32i..pmp..PMPConfig$u20$as$u20$kernel..platform..mpu..MPU$GT$24update_app_memory_region17h55a48db93ba6d713E
    592     _ZN68_$LT$rv32i..pmp..PMPConfig$u20$as$u20$kernel..platform..mpu..MPU$GT$13configure_mpu17h32d25c65ffe73910E

    ## After this patch ##
    80     _ZN68_$LT$rv32i..pmp..PMPConfig$u20$as$u20$kernel..platform..mpu..MPU$GT$15allocate_region17h272c094b5b64d63bE
    96     _ZN68_$LT$rv32i..pmp..PMPConfig$u20$as$u20$kernel..platform..mpu..MPU$GT$26allocate_app_memory_region17h56b3decc4c649f8cE
    32     _ZN68_$LT$rv32i..pmp..PMPConfig$u20$as$u20$kernel..platform..mpu..MPU$GT$24update_app_memory_region17h55a48db93ba6d713E
    80     _ZN68_$LT$rv32i..pmp..PMPConfig$u20$as$u20$kernel..platform..mpu..MPU$GT$13configure_mpu17h32d25c65ffe73910E

NOTE: All above numbers already include: #2020

I haven't compared instruction diffs, but I'm assuming we reduce the
number of instructions run on app swap as well.

### Testing Strategy

Tested running apps on QEMU OpenTitan.

### TODO or Help Wanted

### Documentation Updated

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

### Formatting

- [X] Ran `make prepush`.


Co-authored-by: Alistair Francis <alistair.francis@wdc.com>
  • Loading branch information
@bors @alistair23
bors[bot] and alistair23 committed Jul 20, 2020
2 parents d11598f + bed1b92 commit b99234b
Showing 1 changed file with 66 additions and 21 deletions.
87 changes: 66 additions & 21 deletions arch/rv32i/src/pmp.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
use core::cell::Cell;
use core::cmp;
use core::fmt;
use kernel::common::cells::OptionalCell;

use crate::csr;
use kernel;
Expand Down Expand Up @@ -124,10 +125,9 @@ pub struct PMPConfig {
/// 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>,
app_region: OptionalCell<usize>,
}

const APP_MEMORY_REGION_NUM: usize = 0;

impl Default for PMPConfig {
/// number of regions on the arty chip
fn default() -> PMPConfig {
Expand All @@ -136,6 +136,7 @@ impl Default for PMPConfig {
total_regions: 8,
is_dirty: Cell::new(true),
last_configured_for: MapCell::empty(),
app_region: OptionalCell::empty(),
}
}
}
Expand Down Expand Up @@ -169,12 +170,13 @@ impl PMPConfig {

is_dirty: Cell::new(true),
last_configured_for: MapCell::empty(),
app_region: OptionalCell::empty(),
}
}

fn unused_region_number(&self) -> Option<usize> {
for (number, region) in self.regions.iter().enumerate() {
if number == APP_MEMORY_REGION_NUM {
if self.app_region.contains(&number) {
continue;
}
if region.is_none() {
Expand All @@ -185,6 +187,47 @@ impl PMPConfig {
}
None
}

fn sort_regions(&mut self) {
// Get the app region address
let app_addres = if self.app_region.is_some() {
Some(
self.regions[self.app_region.unwrap_or(0)]
.unwrap()
.location
.0,
)
} else {
None
};

// Sort the regions
self.regions.sort_unstable_by(|a, b| {
let (a_start, _a_size) = match a {
Some(region) => (region.location().0 as usize, region.location().1),
None => (0xFFFF_FFFF, 0xFFFF_FFFF),
};
let (b_start, _b_size) = match b {
Some(region) => (region.location().0 as usize, region.location().1),
None => (0xFFFF_FFFF, 0xFFFF_FFFF),
};
a_start.cmp(&b_start)
});

// Update the app region after the sort
if app_addres.is_some() {
for (i, region) in self.regions.iter().enumerate() {
match region {
Some(reg) => {
if reg.location.0 == app_addres.unwrap() {
self.app_region.set(i);
}
}
None => {}
}
}
}
}
}

impl kernel::mpu::MPU for PMPConfig {
Expand Down Expand Up @@ -304,6 +347,8 @@ impl kernel::mpu::MPU for PMPConfig {
config.regions[region_num] = Some(region);
config.is_dirty.set(true);

config.sort_regions();

Some(mpu::Region::new(start as *const u8, size))
}

Expand All @@ -329,6 +374,12 @@ impl kernel::mpu::MPU for PMPConfig {
}
}

let region_num = if config.app_region.is_some() {
config.app_region.unwrap_or(0)
} else {
config.unused_region_number()?
};

// Make sure there is enough memory for app memory and kernel memory.
let memory_size = cmp::max(
min_memory_size,
Expand All @@ -355,9 +406,13 @@ impl kernel::mpu::MPU for PMPConfig {

let region = PMPRegion::new(region_start as *const u8, region_size, permissions);

config.regions[APP_MEMORY_REGION_NUM] = Some(region);
config.regions[region_num] = Some(region);
config.is_dirty.set(true);

config.app_region.set(region_num);

config.sort_regions();

Some((region_start as *const u8, region_size))
}

Expand All @@ -368,7 +423,9 @@ impl kernel::mpu::MPU for PMPConfig {
permissions: mpu::Permissions,
config: &mut Self::MpuConfig,
) -> Result<(), ()> {
let (region_start, region_size) = match config.regions[APP_MEMORY_REGION_NUM] {
let region_num = config.app_region.unwrap_or(0);

let (region_start, region_size) = match config.regions[region_num] {
Some(region) => region.location(),
None => {
// Error: Process tried to update app memory MPU region before it was created.
Expand All @@ -386,9 +443,11 @@ impl kernel::mpu::MPU for PMPConfig {

let region = PMPRegion::new(region_start as *const u8, region_size, permissions);

config.regions[APP_MEMORY_REGION_NUM] = Some(region);
config.regions[region_num] = Some(region);
config.is_dirty.set(true);

config.sort_regions();

Ok(())
}

Expand All @@ -401,22 +460,8 @@ impl kernel::mpu::MPU for PMPConfig {
// 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() {
// Sort the regions before configuring PMP in TOR mode.
let mut regions_sorted = config.regions.clone();
regions_sorted.sort_unstable_by(|a, b| {
let (a_start, _a_size) = match a {
Some(region) => (region.location().0 as usize, region.location().1),
None => (0xFFFF_FFFF, 0xFFFF_FFFF),
};
let (b_start, _b_size) = match b {
Some(region) => (region.location().0 as usize, region.location().1),
None => (0xFFFF_FFFF, 0xFFFF_FFFF),
};
a_start.cmp(&b_start)
});

for x in 0..self.total_regions {
let region = regions_sorted[x];
let region = config.regions[x];
match region {
Some(r) => {
let cfg_val = r.cfg.value as u32;
Expand Down

0 comments on commit b99234b

Please sign in to comment.