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

Tracking: Make Tock Architecture Agnostic #985

Closed
9 tasks done
bradjc opened this issue Jun 12, 2018 · 9 comments
Closed
9 tasks done

Tracking: Make Tock Architecture Agnostic #985

bradjc opened this issue Jun 12, 2018 · 9 comments
Labels
release-blocker Issue or PR that must be resolved before the next release rfc Issue designed for discussion and to solicit feedback. tracking

Comments

@bradjc
Copy link
Contributor

bradjc commented Jun 12, 2018

Since we only currently support Cortex-M class devices it hasn't been a problem to have cortex-m specific things in the kernel crate. However, our longer terms goals require that the kernel crate be architecture agnositic.

@bradjc bradjc added this to Critical in Tock Rolling Release Jun 12, 2018
@alevy
Copy link
Member

alevy commented Jun 12, 2018

/cc @dverhaert

@bradjc bradjc mentioned this issue Jun 25, 2018
2 tasks
@bradjc bradjc added the rfc Issue designed for discussion and to solicit feedback. label Jun 27, 2018
@bradjc
Copy link
Contributor Author

bradjc commented Jun 27, 2018

To get this going I took a first pass stab at what an interface might look like that an architecture needs to implement to support a syscall interface between processes and the kernel:

/// This trait must be implemented by the architecture of the chip Tock is
/// running on. It allows the kernel to manage processes in an
/// architecture-agnostic manner.
pub trait SyscallInterface {
/// Allows the kernel to query the architecture to see if a syscall occurred
/// for the currently running process.
fn get_syscall_fired(&self) -> bool;
/// Get the syscall that the process called.
fn get_syscall_number(&self, stack_pointer: *const u8) -> Option<Syscall>;
/// Get the four u32 values that the process can pass with the syscall.
fn get_syscall_data(&self, stack_pointer: *const u8) -> (u32, u32, u32, u32);
/// Replace the last stack frame with the new function call. This function
/// is what should be executed when the process is resumed.
fn replace_function_call(&self, stack_pointer: *const u8, callback: FunctionCall);
/// Context switch to a specific process.
fn switch_to_process(&self, stack_pointer: *const u8) -> *mut u8;
}

Please comment or add commits as needed. My goal was not to copy the exact interface we are using now. There is also this issue of the stored_regs that I'm not sure how to handle.

@ppannuto
Copy link
Member

ppannuto commented Jun 28, 2018

This motivated me to resurrect a branch I had lying around towards a Tock native port ( https://github.com/tock/tock/tree/native-port ) -- its fairly immature (currently it'll get as far as segfaulting when it tries to panic from the first of many unimplemented!'s that it hits), but it compiles a native binary! Which is a start.

[Update: Now it'll make it as far a unimplemented! when the kernel tries to go to sleep]

I'll try to see what implementing this interface for a non-MCU case looks like, which hopefully will be a good stress test of its flexibility.

@bradjc
Copy link
Contributor Author

bradjc commented Jun 29, 2018

I got started on this on the https://github.com/tock/tock/tree/arch-agnostic2 branch that builds off of the change to add a Kernel struct.

From what I posted before:

  • I merged the APP_FAULT and SYSCALL_FIRED global usize values and instead used two bits of a single variable. This makes the logic cleaner.
  • I had to add a set_syscall_return_value() function to the interface.

@bradjc
Copy link
Contributor Author

bradjc commented Jul 6, 2018

Each process needs a way to keep around architecture-dependent state when that process is not running (for cortex-m we save a bunch of registers). This means the Process struct must be templated across architectures, but doing that makes the static PROCS array pointer hard (impossible?) to define. Therefore, this is blocked on removing the PROCS static variable in process.rs.

@bradjc bradjc changed the title Tracking: Remove cortex-m specific code from kernel crate Tracking: Make Tock Architecture Agnostic Jul 6, 2018
@bradjc
Copy link
Contributor Author

bradjc commented Jul 11, 2018

Templating Process across architectures means process looks like:

pub struct Process<'a, S: SyscallInterface> { ... }

and since the Kernel struct has to hold it, that looks like:

/// Main object for the kernel. Each board will need to create one.
pub struct Kernel<S: SyscallInterface> {
    /// How many "to-do" items exist at any given time. These include
    /// outstanding callbacks and processes in the Running state.
    work: Cell<usize>,
    /// This holds a pointer to the static array of Process pointers.
    processes: &'static [Option<&'static mut Process<'static, S>>],
}

but now anything that holds a kernel pointer must also have that template, which is fine except that AppId or Callback needs a &kernel so that a capsule can schedule a callback for a process. Right now that means AppId looks like:

/// Userspace app identifier.
#[derive(Clone, Copy)]
pub struct AppId<S> {
    kernel: &'static Kernel<S>,
    idx: usize,
}

The issue becomes that AppId is in the Driver interface. Which means that Driver would also need to be templated as well, and at that point it seems kind of ridiculous. Why should every capsule that wants to provide a Driver interface also be templated across architectures?

Is there a way to avoid this? Maybe some rust trick or some way to structure this that isn't popping into my head?

@phil-levis
Copy link
Contributor

phil-levis commented Jul 11, 2018

Each process needs a way to keep around architecture-dependent state when that process is not running (for cortex-m we save a bunch of registers). This means the Process struct must be templated across architectures, but doing that makes the static PROCS array pointer hard (impossible?) to define.

This is also true for MPUs -- each process will need to store state on how to restore the MPU when it's switched in. That state is chip-specific (e.g., the state kept for the K66 MPU is different than the standard CortexM MPU).

@bradjc bradjc mentioned this issue Jul 30, 2018
2 tasks
@bradjc bradjc added the release-blocker Issue or PR that must be resolved before the next release label Aug 10, 2018
bors bot added a commit that referenced this issue Aug 13, 2018
1113: Move architecture-dependent syscall code to arch/cortex-m r=bradjc a=bradjc

### Pull Request Overview

This pull request moves all of the code that is cortex-m syscall specific out of process.rs and into the cortex-m crate. This is part of #985.

It does this by:
- Making a new `SyscallInterface` trait that is implemented in the cortex-m crate. Each process has a reference to the implementing struct.
- Making a new `ProcessType` trait that all process types implement (right now we only have one, `Process` in process.rs). This allows the `Process` struct to be templated over the `SyscallInterface` type while the `processes` array in the `Kernel` struct keeps an array of `ProcessType` implementations:

    ```rust
    Kernel {
        processes: &'static [Option<&'static process::ProcessType>]
    }
    ```

    which means the template parameter stays only in the process.rs file and does not spread to all of Tock.
    
The panic! prints in process.rs are partially commented out because they relied on many architecture-specific fields. Also the `SCB_REGISTERS` global static variable still exists. Moving those to the cortex-m crate is forthcoming in a different PR.

This is blocked on #1111.

### Testing Strategy

Hail on Hail, but needs more.


### TODO or Help Wanted
n/a


### Documentation Updated

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

### Formatting

- [x] Ran `make formatall`.


Co-authored-by: Brad Campbell <bradjc5@gmail.com>
Co-authored-by: Pat Pannuto <pat.pannuto@gmail.com>
@dverhaert dverhaert mentioned this issue Aug 22, 2018
5 tasks
@bradjc
Copy link
Contributor Author

bradjc commented Sep 28, 2018

Should we move the KernelUserlandBoundary into the Chip? We would end up with something like:

pub trait Chip {
type MPU: mpu::MPU;
type UserspaceKernelBoundary: syscall::UserspaceKernelBoundary;
type SysTick: systick::SysTick;
fn service_pending_interrupts(&self);
fn has_pending_interrupts(&self) -> bool;
fn mpu(&self) -> &Self::MPU;
fn systick(&self) -> &Self::SysTick;
fn userspace_kernel_boundary(&self) -> &Self::UserspaceKernelBoundary;
fn sleep(&self);
unsafe fn atomic<F, R>(&self, f: F) -> R
where
F: FnOnce() -> R;
}

I think this is cleaner, as there is really no reason the board file should have to pick a chip and an arch.

This board file then looks like:

kernel::procs::load_processes(
board_kernel,
chip,
&_sapps as *const u8,
&mut APP_MEMORY,
&mut PROCESSES,
FAULT_RESPONSE,
&process_management_capability,
);
board_kernel.kernel_loop(&hail, chip, Some(&hail.ipc), &main_loop_capability);
}

The full implementation is here: https://github.com/tock/tock/compare/new-mpu-new-chip?expand=1

bors bot added a commit that referenced this issue Oct 17, 2018
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>
@bradjc
Copy link
Contributor Author

bradjc commented Oct 31, 2018

Ok! For the first pass this is done. We have to actually try to use Tock on a new architecture to really evaluate this, but all of the goals for this issue have been met.

@bradjc bradjc closed this as completed Oct 31, 2018
Tock Rolling Release automation moved this from Critical to Done Oct 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-blocker Issue or PR that must be resolved before the next release rfc Issue designed for discussion and to solicit feedback. tracking
Projects
No open projects
Development

No branches or pull requests

4 participants