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 #1126

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@cmcavity
Copy link
Contributor

cmcavity commented Jul 18, 2018

Pull Request Overview

This introduces an updated MPU interface that is no longer Cortex-M4 specific. The setup_mpu function in process.rs is refactored to make use of this interface, and the interface is implemented for the Cortex-M4. The Cortex-M4 MPU implementation is also moved over to the new register interface.

This PR represents a first step in a plan to create an architecture-agnostic interface to isolating process memory (e.g. using the CortexM's memory protection unit). During the design process, we realized that, at a high level there seems to be a trade-off between an interface that is simple to implement (for each architecture) and use (for process.rs) and one that is efficient for a variety of types of MPUs, not just the Cortex-M.

There's a thread on Tock Dev with discussion of some design considerations.

This step represents a first attempt to optimize the former. In particular, it requires very minimal changes to process.rs, and the Cortex-M implementation is relatively simple.

This will serve as a benchmark to measure future designs against: how much more complicated does an efficiency-friendly interface make implementations? is it worth it? etc.

Testing Strategy

Ran the mpu_stack_growth test app.

TODO or Help Wanted

@dverhaert and I are currently working on a second version of this interface that, among other things, reduces the overhead during context switches by disentangling allocation of regions from switching to a region configuration (thus removing allocation from the hot path in setup_mpu).

Documentation Updated

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

Formatting

  • Ran make formatall.
@bradjc
Copy link
Contributor

bradjc left a comment

I know this isn't the final version, but some comments that can be rolled in if the code is still around in the newer version.

pub ctrl: ReadWrite<u32, Control::Register>,
pub rnr: ReadWrite<u32, RegionNumber::Register>,
pub rbar: ReadWrite<u32, RegionBaseAddress::Register>,
pub rasr: ReadWrite<u32, RegionAttributes::Register>,

This comment has been minimized.

@bradjc

bradjc Jul 19, 2018

Contributor

I'd hate to lose those nice comments, even if they are a little redundant with the new register interface.

This comment has been minimized.

@dverhaert

dverhaert Jul 25, 2018

Contributor

We moved all these comments over to the register bitfields and modified them. Just the tables in the comments have been removed, because the functions of different registers have become clear because of the new interface

// Access Access
NoAccess, // -- --
PrivilegedOnly, // V --
Full, // V V

This comment has been minimized.

@bradjc

bradjc Jul 19, 2018

Contributor

What does "V" mean?

This comment has been minimized.

@alevy

alevy Jul 19, 2018

Member

I think that's a checkmark.

pub struct Region {
base_address: u32,
attributes: u32,
start: usize,

This comment has been minimized.

@bradjc

bradjc Jul 19, 2018

Contributor

Return to base_address, and this could probably be a *const u8.

This comment has been minimized.

@dverhaert

dverhaert Jul 26, 2018

Contributor

We'll need to be manipulating these addresses a lot, and use the unsafe offset function every time. Is there any way to do this without using unsafe?

}

/// Noop implementation of MPU trait
/// No-op implementation of MPU trait

This comment has been minimized.

@bradjc

bradjc Jul 19, 2018

Contributor

I think this can be integrated into the trait directly (like how Driver is).

This comment has been minimized.

@cmcavity

cmcavity Jul 19, 2018

Contributor

Don't we need to explicitly implement it for () since there are chips that use that as their MPU?

This comment has been minimized.

@bradjc

bradjc Jul 19, 2018

Contributor

Is it possible for the chips to do impl Mpu for () without defining any functions? I'm not sure if that would work in rust or not.

This comment has been minimized.

@alevy

alevy Jul 19, 2018

Member

There are maybe two questions:

  1. Can you define an instance of a trait with methods without defining those methods: yes, if all methods have default implemetations.

  2. Can the chip define an instance for unit (()): no. You can only define instances of traits in the module where the trait is defined, or the module where the type for which you want an instance is defined. Other forms are called orphan instances and they can create all sorts of safety issues so Rust doesn't allow them ATM (not just Rust, Haskell also disallowed them by default).

access: AccessPermission,
) -> Option<Region>;
/// Returns the number of supported MPU regions.
fn num_supported_regions(&self) -> u32;

This comment has been minimized.

@bradjc

bradjc Jul 19, 2018

Contributor

Make this fn number_supported_regions(&self) -> usize;

@phil-levis
Copy link
Collaborator

phil-levis left a comment

I assume the plan is to transition this PR to the interface we've discussed on tock-dev?

@cmcavity

This comment has been minimized.

Copy link
Contributor

cmcavity commented Jul 19, 2018

@phil-levis We are currently working on the tock-dev version on another branch, and we were planning on submitting a second pull request for that version when completed.

@alevy

This comment has been minimized.

Copy link
Member

alevy commented Jul 19, 2018

@phil-levis I'm personally open to both.

Aside from reviewing in detail, I think this represents a significant improvement over our currently hyper Cortex-M specific interface.

Importantly, it allows @cmcavity to implement the interface for the MK66 and finally make the Teensy a first-class supported board (which has basically been blocking on not having a supported MPU). It also serves as a benchmark to actually evaluate the design changes proposed on the tock-dev thread, as well as alternate approaches.

Meanwhile, making the rest of the kernel architecture agnostic is chugging along.

On the other hand, if the feeling is that this interface is bad in a way that a future version near to come will strictly improve upon, it also seems perfectly fine to have this PR sitting around just as a marker of progress for the rest of us.

@phil-levis

This comment has been minimized.

Copy link
Collaborator

phil-levis commented Jul 27, 2018

I think we want to only change the MPU interface once; if I were supporting an out-of-main MCU with a different MPU, I might be willing to implement it once if it meant I could then get protected processes, but would be pissed and frustrated if I had to do it twice in the space of a few months.

@bradjc bradjc referenced this pull request Jul 31, 2018

Closed

Tracking: Make Tock Architecture Agnostic #985

9 of 9 tasks complete

@dverhaert dverhaert referenced this pull request Aug 22, 2018

Merged

New MPU Interface #1159

2 of 5 tasks complete

@cmcavity cmcavity closed this Aug 22, 2018

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment