Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New MPU Interface #1159

Merged
merged 19 commits into from Oct 17, 2018

Conversation

@dverhaert
Copy link
Contributor

commented Aug 22, 2018

Pull Request Overview

This pull request introduces an updated MPU interface that is no longer Cortex-M4 specific. It follows up on #1126, being part of a plan to make Tock architecture-agnostic (#985).

An overview of the key differences:

  1. The interface now has two Tock-specific functions for allocating and updating the MPU region for the app-owned portion of its RAM.
  2. The interface now only supports specifying user permissions for regions (instead of also specifying supervisor permissions). Memory locations not within any allocated region are required to be inaccessible in user mode, and accessible in supervisor mode. A related change is that we no longer allocate an MPU region for the grant memory in process.rs.
  3. Instead of reallocating all the MPU regions every context switch, regions are now only allocated (computed by the MPU) when they are created or updated. Configuration of the MPU registers is now the only thing that happens during context switches.

The main reasoning behind the first decision is that the app break grows over the lifetime of the process. As such, we have found it infeasible for the client to request an MPU region that is suitable for this purpose through a generic region request. The reasoning behind the second decision is that allowing simultaneous specification of both user and supervisor permissions necessitates exposing a large matrix of options that no single MPU is able to support. When decoupled, we are left with a small, logical set of user permissions that we can reasonably expect all MPUs to be able to support. Furthermore, it is more consistent with our current practice of only enabling the MPU for processes.

The new interface allows removal of code from process.rs that assumed the presence of the Cortex-M MPU, and in particular its constraints and features. For instance, in the Cortex-M, the size of an MPU region must to be a power of two and aligned to the start address, and in the case of region overlap, the permissions of the overlapped segment are those of the region with the highest region number.

For further background and discussion, see the relevant thread on tock-dev. If something remains unclear, feel free to leave a comment below.

Testing Strategy

This pull request was tested by running the mpu_stack_growth app in libtock-c, as well as a few others: one that walks over a process's RAM and flash regions, and several that try to make invalid accesses outside of these regions.

TODO or Help Wanted

  • Fix _start in userland. @alevy is working on a branch in libtock-c that fixes _start so that processes never access memory past their app break. This pull request is blocking on that branch being merged.
  • Loading processes. Ideally we would load processes in decreasing order of RAM size on boards that use the Cortex-M MPU. Doing so would eliminate the possibility of external fragmentation between processes on such boards.
  • Size of the mpu_regions array. This field in struct Process is currently hard-coded to be 6 in size because the Cortex-M supports 8 regions (and two of these are reserved for the flash and RAM regions). This is the last architecture-specific code remaining in process.rs. It currently seems infeasible to make this size chip-specific, seeing as Rust doesn't seem to support this yet.

Documentation Updated

  • Updated the relevant files in /docs, or no updates are required.

Formatting

  • Ran make formatall.
New mpu interface
Co-authored-by: Danilo Verhaert <daniloverhaert@gmail.com>
@phil-levis

This comment has been minimized.

Copy link
Collaborator

commented Aug 22, 2018

Couple of question:

  1. Can you walk through the intended call sequence for the API? Reading the API and Process::create, it sounds like allocate_region is intended for flash, while allocate_app_ region is intended for RAM. Or is allocate_region the general purpose form, and allocate_app_region is a special purpose form intended for process memory layout?
  2. It would be useful to say how they are intended to be used and their constraints. E.g., would it be valid for a process to invoke allocate_app_region twice?
  3. The notion of a "parent" region implies there is an MPU region for it, but this seems to instead just be a block of unallocated address space?

I personally don't like how now every MPU implementation is specifically bound to the current process memory layout; I think it is much better, in terms of maintenance and debugging, to separate the concerns between laying out memory in the manner that Tock expects and allocating regions. But I realize that (better in the long term) approaches is much more subtle and trick to get right, and so this approach is fine. We can always decompose the two later if experience shows it is needed (the current MPU trait becomes chip-independent, on top of a chip-specific implementation of another, lower-level trait).

@cmcavity

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2018

Yes, allocate_region is the more general function (for flash, IPC, etc.), and allocate_app_memory_region is specifically for setting up process memory in RAM and allocating the MPU region for the app-owned part. It is not valid for a process to invoke allocate_app_memory_region more than once: the implementation must return an error in this case. Regarding the name "parent", yes we meant it to be "block of unallocated address space" -- do you think this name is ambiguous?

@phil-levis

This comment has been minimized.

Copy link
Collaborator

commented Aug 22, 2018

cmcavity added some commits Aug 23, 2018

Update mpu interface and cortex-m implementation
Added documentation and slightly changed fn signatures in interface,
added region overlap checks and refactored in cortex-m implementation
arch/cortex-m4/src/mpu.rs Show resolved Hide resolved
kernel/src/process.rs Outdated Show resolved Hide resolved
kernel/src/process.rs Show resolved Hide resolved
unallocated_memory_start: *const u8,
unallocated_memory_size: usize,
min_region_size: usize,
) -> Option<(*const u8, usize)>;

This comment has been minimized.

Copy link
@bradjc

bradjc Aug 29, 2018

Contributor

I think it makes sense to add something like

struct MpuRegion {
  start_address: *const u8,
  length_bytes: usize,
}

to be used in the return value to provide a helping hint on what the tuple is. This would also help with the mpu regions array in the process struct.

chips/cc26x2/src/chip.rs Outdated Show resolved Hide resolved

@cmcavity cmcavity force-pushed the dverhaert:mpu-interface-merge branch from 6a6a235 to 9f66bd2 Sep 18, 2018

chips/cc26x2/src/chip.rs Outdated Show resolved Hide resolved
@bradjc

This comment has been minimized.

Copy link
Contributor

commented Sep 28, 2018

Also I just tried this pr on hail and the app (blink) immediately crashed. I did a little looking, and when I commented out subregion usage blink and c_hello work again.

@bradjc bradjc referenced this pull request Sep 28, 2018
2 of 2 tasks complete
@dverhaert

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2018

Also I just tried this pr on hail and the app (blink) immediately crashed. I did a little looking, and when I commented out subregion usage blink and c_hello work again.

This is actually due to a bug in userland, where processes access memory past their app break in _start (one of the TODOs). Amit has fixed this in the fix_start_order branch of libtock-c, so if you run in that branch of libtock-c you shouldn't encounter it.

@bradjc

This comment has been minimized.

Copy link
Contributor

commented Sep 28, 2018

This is actually due to a bug in userland, where processes access memory past their app break in _start (one of the TODOs). Amit has fixed this in the fix_start_order branch of libtock-c, so if you run in that branch of libtock-c you shouldn't encounter it.

Gotcha. What would it take to make current/old apps not fault? I ask because we had this idea that apps compiled at 1.0 should still work, and now they won't even though technically they shouldn't have worked before either...

@phil-levis

This comment has been minimized.

Copy link
Collaborator

commented Sep 28, 2018

Hoo boy. Let's not go down the Windows approach of masking fatal bugs in old applications...

@bradjc bradjc referenced this pull request Sep 28, 2018
9 of 9 tasks complete
@dverhaert

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2018

This is actually due to a bug in userland, where processes access memory past their app break in _start (one of the TODOs). Amit has fixed this in the fix_start_order branch of libtock-c, so if you run in that branch of libtock-c you shouldn't encounter it.

Gotcha. What would it take to make current/old apps not fault? I ask because we had this idea that apps compiled at 1.0 should still work, and now they won't even though technically they shouldn't have worked before either...

In the new Cortex-M MPU implementation, we create a process's RAM as one MPU region and use subregions to subdivide this between the PAM and grant, where an enabled subregion represents access for a process. For the unused memory that is between the PAM and grant, we chose to disable subregions. I've just tested changing this to enabled subregions, and this would prevent 1.0 apps from faulting.

@bradjc

This comment has been minimized.

Copy link
Contributor

commented Oct 3, 2018

Ok now that #1171 is merged, do you want to try updating this PR? a2c3347 should be a good starting point

@cmcavity cmcavity force-pushed the dverhaert:mpu-interface-merge branch from d137586 to 7350a07 Oct 6, 2018

match process.add_mpu_region(self.ptr() as *const u8, self.len(), self.len()) {
Some(_) => true,
None => false,
}

This comment has been minimized.

Copy link
@bradjc

bradjc Oct 9, 2018

Contributor

I think this can just be process.add_mpu_region(self.ptr() as *const u8, self.len(), self.len()).is_ok(), but I'm not completely sure.

config: &mut Self::MpuConfig,
) -> Option<(*const u8, usize)> {
let memory_size = {
if min_memory_size < initial_app_memory_size + initial_kernel_memory_size {

This comment has been minimized.

Copy link
@niklasad1

niklasad1 Oct 9, 2018

Member

simplify this code to, let memory_size = core::cmp::max(min_memory_size, initial_app_memory_size + initial_kernel_memory_size)

})
} else {
// Memory base not aligned to memory size
let region_num = match config.unused_region_number() {

This comment has been minimized.

Copy link
@niklasad1

niklasad1 Oct 9, 2018

Member

let region_num = config.unused_region_number()?;

// except for the bits whose index lie within
// [min_subregion, max_subregion]
//
// Note: Rust ranges are minimum inclusive, maximum exclusive, hence

This comment has been minimized.

Copy link
@niklasad1

niklasad1 Oct 10, 2018

Member

I don't understand this,

Can you explain why you can't use Rust's exclusive range i.e, min_subregion..=max_subregion?

// max_subregion + 1.
let mask =
(min_subregion..(max_subregion + 1)).fold(!0, |res, i| res & !(1 << i)) & 0xff;

This comment has been minimized.

Copy link
@niklasad1

niklasad1 Oct 10, 2018

Member

I find this code very hard to read.
Why are you bitwise and_ing the fold result with 0xff? (seems needless to me)

I think you can me this a little be more readable by something like:

let mask = (start..end+1).fold(u32::max_value(), |res, i| {
   // clear bit-by-bit (1 ^ 1 ==  0)    
   res ^ (1 << i)
});

regs.region_base_address.set(region.base_address());
let subregion_mask = (0..num_subregions_used).fold(!0, |res, i| res & !(1 << i)) & 0xff;

This comment has been minimized.

Copy link
@niklasad1

niklasad1 Oct 10, 2018

Member

Really, create a function that calculates subregion mask. Third occurrence of this!

@niklasad1
Copy link
Member

left a comment

Looks generally good, some minor things to address 👍

cmcavity and others added some commits Oct 12, 2018

@bradjc
Copy link
Contributor

left a comment

Ok take a look at the commit I added: 1e60298

It removes debug and panic statements, and relies on the return value. We probably aren't being expressive enough with our return values, but that is a problem for another day.

With those changes I'm happy with this PR.

@niklasad1

This comment has been minimized.

Copy link
Member

commented on kernel/src/process.rs in 1e60298 Oct 17, 2018

would prefer and_then here instead of map_or(None)

@niklasad1

This comment has been minimized.

Copy link
Member

commented on kernel/src/process.rs in 1e60298 Oct 17, 2018

same here, prefer and_then

@niklasad1

This comment has been minimized.

Copy link
Member

commented on kernel/src/process.rs in 1e60298 Oct 17, 2018

would prefer:

self.mpu_config
  .ok_or(Err)
  .and_then() { ....}

This comment has been minimized.

Copy link
Contributor Author

replied Oct 17, 2018

That doesn't work, it doesn't seem that ok_or is defined for MapCell.

@bradjc bradjc dismissed stale reviews from niklasad1 and themself via f170fb4 Oct 17, 2018

@bradjc

bradjc approved these changes Oct 17, 2018

@bradjc

This comment has been minimized.

Copy link
Contributor

commented Oct 17, 2018

bors r+

bors bot added a commit that referenced this pull request Oct 17, 2018

Merge #1159
1159: New MPU Interface r=bradjc a=dverhaert

### Pull Request Overview

This pull request introduces an updated MPU interface that is no longer Cortex-M4 specific. It follows up on #1126, being part of a plan to make Tock architecture-agnostic (#985). 

An overview of the key differences:    

1. The interface now has two Tock-specific functions for allocating and updating the MPU region for the app-owned portion of its RAM.
2. The interface now only supports specifying user permissions for regions (instead of also specifying supervisor permissions). Memory locations not within any allocated region are required to be inaccessible in user mode, and accessible in supervisor mode. A related change is that we no longer allocate an MPU region for the grant memory in process.rs.
3. Instead of reallocating all the MPU regions every context switch, regions are now only allocated (computed by the MPU) when they are created or updated. Configuration of the MPU registers is now the only thing that happens during context switches.

The main reasoning behind the first decision is that the app break grows over the lifetime of the process. As such, we have found it infeasible for the client to request an MPU region that is suitable for this purpose through a generic region request. The reasoning behind the second decision is that allowing simultaneous specification of both user and supervisor permissions necessitates exposing a large matrix of options that no single MPU is able to support. When decoupled, we are left with a small, logical set of user permissions that we can reasonably expect all MPUs to be able to support. Furthermore, it is more consistent with our current practice of only enabling the MPU for processes.

The new interface allows removal of code from process.rs that assumed the presence of the Cortex-M MPU, and in particular its constraints and features. For instance, in the Cortex-M, the size of an MPU region must to be a power of two and aligned to the start address, and in the case of region overlap, the permissions of the overlapped segment are those of the region with the highest region number.

For further background and discussion, see the relevant [thread on tock-dev](https://groups.google.com/forum/#!topic/tock-dev/Fduxk_0xvUE). If something remains unclear, feel free to leave a comment below.

### Testing Strategy

This pull request was tested by running the `mpu_stack_growth` app in libtock-c, as well as a few others: one that walks over a process's RAM and flash regions, and several that try to make invalid accesses outside of these regions. 

### TODO or Help Wanted

- [ ] **Fix `_start` in userland**. @alevy is working on a [branch](https://github.com/dverhaert/libtock-c/tree/fix_start_order) in libtock-c that fixes `_start` so that processes never access memory past their app break. This pull request is blocking on that branch being merged. 
- [ ] **Loading processes**. Ideally we would load processes in decreasing order of RAM size on boards that use the Cortex-M MPU. Doing so would eliminate the possibility of external fragmentation between processes on such boards.
- [ ] **Size of the `mpu_regions` array**. This field in `struct Process` is currently hard-coded to be 6 in size because the Cortex-M supports 8 regions (and two of these are reserved for the flash and RAM regions). This is the last architecture-specific code remaining in process.rs. It currently seems infeasible to make this size chip-specific, seeing as [Rust doesn't seem to support this yet](rust-lang/rust#43408).

### Documentation Updated

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

### Formatting

- [x] Ran `make formatall`.


Co-authored-by: Conor McAvity <cmcavity@protonmail.com>
Co-authored-by: Brad Campbell <bradjc5@gmail.com>
Co-authored-by: Danilo Verhaert <daniloverhaert@gmail.com>
@bors

This comment has been minimized.

Copy link
Contributor

commented Oct 17, 2018

@bors bors bot merged commit f170fb4 into tock:master Oct 17, 2018

3 checks passed

bors Build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.