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

Move architecture-dependent syscall code to arch/cortex-m #1113

Merged
merged 11 commits into from Aug 13, 2018

Conversation

Projects
None yet
7 participants
@bradjc
Copy link
Contributor

bradjc commented Jul 13, 2018

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:

    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

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

Formatting

  • Ran make formatall.

@bradjc bradjc referenced this pull request Jul 13, 2018

Closed

Tracking: Make Tock Architecture Agnostic #985

9 of 9 tasks complete

bradjc added a commit that referenced this pull request Jul 13, 2018

@bradjc bradjc force-pushed the arch-agnostic5 branch from 8b22ec7 to 2e3cb15 Jul 13, 2018

@bradjc

This comment has been minimized.

Copy link
Contributor

bradjc commented Jul 13, 2018

I just updated the branch with two fixes:

  1. It now stores yield_pc per process and not per chip (not sure why I got that wrong the first time).
  2. It goes back to APP_FAULT and SYSCALL_FIRED flags that are set in the svc_handler and hardfault_handler functions. When I tried to combine them for some reason the svc_handler would overwrite the variable that the hardfault handler had set, causing app faults to get missed.
@alevy
Copy link
Member

alevy left a comment

I think this is really pushing us towards having architecture specific implementations of Process, but this is a big improvement for now anyway.

/// architecture-agnostic manner.
pub trait SyscallInterface {
/// Some architecture-specific struct containing per-process state that must
/// be kept while the process is not running.

This comment has been minimized.

@alevy

alevy Jul 13, 2018

Member

Maybe add something like: "e.g., CPU registers"

///
/// An implementor of this function is free to reset any state that was
/// needed to gather this information when this function is called.
fn get_context_switch_reason(&self) -> ContextSwitchReason;

This comment has been minimized.

@alevy

alevy Jul 13, 2018

Member

Could we get away with getting rid of the self argument in these methods? Can we imagine cases where the implementation would use some state in self?

fn get_context_switch_reason(&self) -> ContextSwitchReason;

/// Get the syscall that the process called.
fn get_syscall_number(&self, stack_pointer: *const usize) -> Option<Syscall>;

This comment has been minimized.

@alevy

alevy Jul 13, 2018

Member

These methods should probably all be marked unsafe. Fundamentally the expose unsafe interfaces. For example, this one allows the caller to read arbitrary memory.

This comment has been minimized.

@alevy

alevy Jul 13, 2018

Member

What about extending the Syscall enum to include arguments in each of the variants (see below) and combining this method with get_syscall_data:

pub enum Syscall {
     YIELD,
     SUBSCRIBE((usize, usize, usize, usize)), // these could potentially be even more typed, but for another time
     COMMAND((usize, usize, usize, usize)),
     ALLOW((usize, usize, usize, usize)),
     MEMOP((usize, usize, usize, usize)),
}

fn get_syscall(&self, stack_pointer: *const usize) -> Option<Syscall>;

This comment has been minimized.

@bradjc

bradjc Jul 14, 2018

Contributor

Do you have a preference of the ( version versus the { version? (I don't know what rust calls the two versions of enum args.)

&self,
stack_pointer: *const usize,
state: &mut Self::StoredState,
) -> *mut usize;

This comment has been minimized.

@alevy

alevy Jul 13, 2018

Member

Maybe this should return a StoredState (perhaps as a tuple along with the stack pointer)?

This comment has been minimized.

@bradjc

bradjc Jul 14, 2018

Contributor

So the implementation would take a reference and return it?

&self,
stack_pointer: *const usize,
state: &mut Self::StoredState,
) -> *mut usize;

This comment has been minimized.

@alevy

alevy Jul 13, 2018

Member

Ditto with return StoredState in a tuple here

// #[allow(private_no_mangle_statics)]
#[no_mangle]
#[used]
pub static mut SYSCALL_FIRED: usize = 0;

This comment has been minimized.

@alevy

alevy Jul 13, 2018

Member

does this need to be public?

This comment has been minimized.

@bradjc

bradjc Jul 14, 2018

Contributor

It's public because it's used in the cortex-m4 crate but defined in the cortex-m crate. I'm not sure if it is strictly required.

/// fault handler was called.
#[no_mangle]
#[used]
pub static mut APP_FAULT: usize = 0;

This comment has been minimized.

@alevy

alevy Jul 13, 2018

Member

ditto about being public?

@bradjc

This comment has been minimized.

Copy link
Contributor

bradjc commented Jul 14, 2018

Made some changes, but I'm not sure about removing &self and the StoredState changes.

bradjc added a commit that referenced this pull request Jul 14, 2018

@ppannuto ppannuto added the blocked label Jul 18, 2018

@bradjc bradjc force-pushed the arch-agnostic5 branch from 4e8f1c5 to 5002f59 Jul 20, 2018

@bradjc

This comment has been minimized.

Copy link
Contributor

bradjc commented Jul 24, 2018

I added a commit that updates docs, but just now checked the box.

@bradjc bradjc force-pushed the arch-agnostic5 branch from d73e3dc to d1f9cb9 Jul 26, 2018

@bradjc bradjc removed the blocked label Jul 26, 2018

@bradjc

This comment has been minimized.

Copy link
Contributor

bradjc commented Jul 26, 2018

Please review!

kernel: &'static Kernel,
syscall: &'static S,

This comment has been minimized.

@alevy

alevy Jul 26, 2018

Member

Does this actually need to be a reference? If not, we can avoid the annoying static_init! of a zero-sized value in all the board configurations.

This comment has been minimized.

@bradjc

bradjc Jul 26, 2018

Contributor

Take a look at the commit I just added. It just removes the static_init!() call with no other changes.

@brghena brghena referenced this pull request Jul 30, 2018

Merged

Kernel: add comments #1121

2 of 2 tasks complete
@brghena
Copy link
Contributor

brghena left a comment

Some comments for you. This looks like a good change overall. I have not tested it on hardware yet.

ldr r0, =APP_FAULT
"ldr r0, =APP_FAULT
mov r1, #1 /* Fault */

This comment has been minimized.

@brghena

brghena Jul 30, 2018

Contributor

Where are we actually writing to the SYSCALL_FIRED variable?

This comment has been minimized.

@bradjc

bradjc Jul 30, 2018

Contributor

I'm not sure what you mean.

This comment has been minimized.

@brghena

brghena Jul 30, 2018

Contributor

The code removed here used to write to SYSCALL_FIRED. We still read the variable to see if there was a syscall, but a glance through code left me unsure of where SYSCALL_FIRED is actually written to now.

This comment has been minimized.

@bradjc

bradjc Jul 30, 2018

Contributor

Ah, same place we were before: https://github.com/tock/tock/pull/1113/files/a6c2fe1c10227dba72dbe63499f90f8d800d559a#diff-3c4f4f49e6d8b6e9ec96c45e8c737637R146

I think marking that a syscall happened in the fault handler was a hack before.

This comment has been minimized.

@brghena

brghena Jul 30, 2018

Contributor

Okay, that makes a lot of sense now. It was just a hack for routing the failure presumably.

/// Why the process stopped executing and execution returned to the kernel.
pub enum ContextSwitchReason {
/// Process exceeded its timeslice, otherwise catch-all.
Other,

This comment has been minimized.

@brghena

brghena Jul 30, 2018

Contributor

Is Other ever used for reasons apart from timeslices expiring? It seems like we should name it for its purpose TimesliceExpired (or similar). If there really are other possible reasons, we should have a catch-all for them apart from timeslices.

This comment has been minimized.

@bradjc

bradjc Jul 30, 2018

Contributor

I don't know.

This comment has been minimized.

@ppannuto

ppannuto Aug 2, 2018

Member

+1 for no Other

This comment has been minimized.

@alevy

alevy Aug 2, 2018

Member

I agree with replacing Other with something timeslice specific.

From an API design perspective, I think it doesn't cost anything to add specific reasons later, since the implementer of UserspaceKernelBoundary only produces values of this type, and doesn't pattern match. So if some architecture might have some other reason to interrupt the process at some point later, we can add that reason to the enum with no changes needed to previous implementations.

This comment has been minimized.

@bradjc

bradjc Aug 2, 2018

Contributor

How does a timeslice expiration manifest? Put another way, does the implementer of get_and_reset_context_switch_reason() know that the reason the process stopped was because of the timeslice expiration? This is not clear in the original implementation, and why Other was created.

This comment has been minimized.

@alevy

alevy Aug 2, 2018

Member

You're right...

the systick timer fires, which is also in the arch crate. That logic, as you say, is basically split now between the arch crate (handling the systick interrupt and setting a global flag shared with the kernel crate), and the kernel crate reading the global flag and deciding---yep, this process exceeded it's timeslice.

This comment has been minimized.

@bradjc

bradjc Aug 2, 2018

Contributor

Is there a global flag? Or is it the absence of a global flag being set that indicates it was the systick firing?

///
/// An implementor of this function is free to reset any state that was
/// needed to gather this information when this function is called.
unsafe fn get_context_switch_reason(&self) -> ContextSwitchReason;

This comment has been minimized.

@brghena

brghena Jul 30, 2018

Contributor

I don't know what I want this to be named, but something better. get_context_switch_reason doesn't imply that this function seems to also clean up a switched process and can only be called once. Maybe something like cleanup_switched_process(&self) -> ContextSwitchReason? Especially because, for comparison, get_syscall can be called as many times as you please with no side effects.

This comment has been minimized.

@bradjc

bradjc Jul 30, 2018

Contributor

What about get_and_reset_context_switch_reason()?

Ok(res)
},
)
let ctr_ptr = process.grant_ptr(self.grant_num) as *mut *mut T;

This comment has been minimized.

@brghena

brghena Jul 30, 2018

Contributor

Could you add a comment to the top of this block that clarifies that you're either getting a pointer to the grant, or else allocating it.

/// Add a stack frame with the new function call. This function
/// is what should be executed when the process is resumed. Returns the new
/// stack pointer.
unsafe fn replace_function_call(

This comment has been minimized.

@brghena

brghena Jul 30, 2018

Contributor

Does this really replace at all?

This comment has been minimized.

@bradjc

bradjc Jul 30, 2018

Contributor

Oops this should be fixed. At one point I was trying to merge it with pop_syscall_stack but that ended up being too radical a change.

@@ -365,8 +458,8 @@ impl Process<'a> {
let init_fn = app_flash_address
.offset(self.header.get_init_function_offset() as isize)
as usize;
self.yield_pc.set(init_fn);
self.psr.set(0x01000000);
// self.yield_pc.set(init_fn);

This comment has been minimized.

@brghena

brghena Jul 30, 2018

Contributor

Leftover removal from testing??

12,
// self.lr(),
// self.pc(),
// self.xpsr(),

This comment has been minimized.

@brghena

brghena Jul 30, 2018

Contributor

I see that this will all likely get fixed in #1115. Should that PR and this one be merged? Without those changes, this is a pretty bad regression for Tock debugging.

This comment has been minimized.

@bradjc

bradjc Jul 30, 2018

Contributor

It absolutely gets fixed.

pub r1: usize,
pub r2: usize,
pub r3: usize,
pub pc: usize,

This comment has been minimized.

@brghena

brghena Jul 30, 2018

Contributor

The concept of a function call accepting arguments as register0, ... is probably not universal. Maybe rename these to argument0, argument1, ... which is their purpose anyways. And pc could be called functionAddress or similar.

This comment has been minimized.

@ppannuto

ppannuto Jul 31, 2018

Member

While I agree that arugmentN is a clearer name, I actually looked into this the first time this came around -- more on the bend of understanding if 4 arguments was a reasonable expectation as we look at other architectures. Turns out that syscall argument passing via registers is fairly universal, and ARM is actually the odd fellow out with 4 as a lowball number of supported arguments.

This comment has been minimized.

@bradjc

bradjc Jul 31, 2018

Contributor

So, leave it?

This comment has been minimized.

@ppannuto

ppannuto Aug 2, 2018

Member

I think argumentN is a better name, my comment was intended to address Branden's "... is probably not universal"

This comment has been minimized.

@alevy

alevy Aug 2, 2018

Member

yeah, argumentN is better. a comment could say something like ("These will typically be passed in the first few registers, but it's architecture specific").

bradjc added a commit that referenced this pull request Jul 31, 2018

@bradjc

This comment has been minimized.

Copy link
Contributor

bradjc commented Aug 1, 2018

The new interfaces are:

The SyscallInterface that handles all of the architecture-specific operations of switching between userspace and the kernel:

tock/kernel/src/syscall.rs

Lines 60 to 108 in 3e315ca

/// 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 {
/// Some architecture-specific struct containing per-process state that must
/// be kept while the process is not running. For example, for keeping CPU
/// registers that aren't stored on the stack.
type StoredState: Default;
/// Allows the kernel to query to see why the process stopped running. This
/// function can only be called once to get the last state of the process
/// and why the process context switched back to the kernel.
///
/// An implementor of this function is free to reset any state that was
/// needed to gather this information when this function is called.
unsafe fn get_and_reset_context_switch_reason(&self) -> ContextSwitchReason;
/// Get the syscall that the process called with the appropriate arguments.
unsafe fn get_syscall(&self, stack_pointer: *const usize) -> Option<Syscall>;
/// Set the return value the process should see when it begins executing
/// again after the syscall.
unsafe fn set_syscall_return_value(&self, stack_pointer: *const usize, return_value: isize);
/// Remove the last stack frame from the process and return the new stack
/// pointer location.
unsafe fn pop_syscall_stack(
&self,
stack_pointer: *const usize,
state: &mut Self::StoredState,
) -> *mut usize;
/// Add a stack frame with the new function call. This function
/// is what should be executed when the process is resumed. Returns the new
/// stack pointer.
unsafe fn push_function_call(
&self,
stack_pointer: *const usize,
callback: process::FunctionCall,
state: &Self::StoredState,
) -> *mut usize;
/// Context switch to a specific process.
unsafe fn switch_to_process(
&self,
stack_pointer: *const usize,
state: &mut Self::StoredState,
) -> *mut usize;
}

and the ProcessType interface that all process implementations must provide (we currently only have one: Process):

tock/kernel/src/process.rs

Lines 72 to 194 in 3e315ca

/// This trait is implemented by process structs.
pub trait ProcessType {
/// Queue a callback for the process. This will be added to a per-process
/// buffer and passed to the process by the scheduler.
fn schedule(&self, callback: FunctionCall) -> bool;
/// Queue an IPC operation for this process.
fn schedule_ipc(&self, from: AppId, cb_type: IPCType);
/// Remove the scheduled operation from the front of the queue.
fn unschedule(&self) -> Option<Task>;
/// Returns the current state of the process. Common states are "running" or
/// "yielded".
fn current_state(&self) -> State;
/// Move this process from the running state to the yield state.
fn yield_state(&self);
/// Put this process in the fault state. This will trigger the
/// `FaultResponse` for this process to occur.
unsafe fn fault_state(&self);
/// Get the name of the process. Used for IPC.
fn get_package_name(&self) -> &[u8];
// memop operations
/// Change the location of the program break.
fn brk(&self, new_break: *const u8) -> Result<*const u8, Error>;
/// Change the location of the program break and return the previous break
/// address.
fn sbrk(&self, increment: isize) -> Result<*const u8, Error>;
/// The start address of allocated RAM for this process.
fn mem_start(&self) -> *const u8;
/// The first address after the end of the allocated RAM for this process.
fn mem_end(&self) -> *const u8;
/// The start address of the flash region allocated for this process.
fn flash_start(&self) -> *const u8;
/// The first address after the end of the flash region allocated for this
/// process.
fn flash_end(&self) -> *const u8;
/// The lowest address of the grant region for the process.
fn kernel_memory_break(&self) -> *const u8;
/// How many writeable flash regions defined in the TBF header for this
/// process.
fn number_writeable_flash_regions(&self) -> usize;
/// Get the offset from the beginning of flash and the size of the defined
/// writeable flash region.
fn get_writeable_flash_region(&self, region_index: usize) -> (u32, u32);
/// Debug function to update the kernel on where the stack starts for this
/// process. Processes are not required to call this through the memop
/// system call, but it aids in debugging the process.
fn update_stack_start_pointer(&self, stack_pointer: *const u8);
/// Debug function to update the kernel on where the process heap starts.
/// Also optional.
fn update_heap_start_pointer(&self, heap_pointer: *const u8);
// additional memop like functions
/// Check if the buffer address and size is contained within the memory
/// owned by this process. This isn't quite the same as the memory allocated
/// to the process as this does not include the grant region which is owned
/// by the kernel.
fn in_app_owned_memory(&self, buf_start_addr: *const u8, size: usize) -> bool;
/// Get the first address of process's flash that isn't protected by the
/// kernel. The protected range of flash contains the TBF header and
/// potentially other state the kernel is storing on behalf of the process,
/// and cannot be edited by the process.
fn flash_non_protected_start(&self) -> *const u8;
// mpu
fn setup_mpu(&self, mpu: &mpu::MPU);
fn add_mpu_region(&self, base: *const u8, size: u32) -> bool;
// grants
/// Create new memory in the grant region.
unsafe fn alloc(&self, size: usize) -> Option<&mut [u8]>;
unsafe fn free(&self, _: *mut u8);
/// Get a pointer to the grant pointer for this grant number.
unsafe fn grant_ptr(&self, grant_num: usize) -> *mut *mut u8;
// functions for processes that are architecture specific
/// Get why this process stopped running.
unsafe fn get_and_reset_context_switch_reason(&self) -> syscall::ContextSwitchReason;
/// Get the syscall that the process called.
unsafe fn get_syscall(&self) -> Option<Syscall>;
/// Set the return value the process should see when it begins executing
/// again after the syscall.
unsafe fn set_syscall_return_value(&self, return_value: isize);
/// Remove the last stack frame from the process.
unsafe fn pop_syscall_stack(&self);
/// Replace the last stack frame with the new function call. This function
/// is what should be executed when the process is resumed.
unsafe fn push_function_call(&self, callback: FunctionCall);
/// Context switch to a specific process.
unsafe fn switch_to(&self);
unsafe fn fault_str(&self, writer: &mut Write);
unsafe fn statistics_str(&self, writer: &mut Write);
}

@phil-levis
Copy link
Collaborator

phil-levis left a comment

Overall, I like the trait -- it's very clean. My one major concern is with how it signals errors to callers. Since the functions are unsafe we want them to do lots of error checking and then inform a caller when they fail.

&self,
stack_pointer: *const usize,
state: &mut Self::StoredState,
) -> *mut usize;

This comment has been minimized.

@phil-levis

phil-levis Aug 1, 2018

Collaborator

What if there isn't a stack frame? E.g., suppose there is one frame and you call this twice. I'd suggest either returning some kind of a Result or some other way to denote whether the returned stack pointer location is valid.

This comment has been minimized.

@bradjc

bradjc Aug 1, 2018

Contributor

I don't know? Is there a way for the implementer to know if the pointer is a valid stack pointer?

The existing code trusts that the stack pointer is a valid stack pointer with a stack frame for a given process, although that code is all implemented in process.rs.

I assumed the contract has to be that stack_pointer is valid, and if it isn't then any bugs are the caller's fault.

This comment has been minimized.

@phil-levis

phil-levis Aug 1, 2018

Collaborator

I think it's OK to assume that the stack pointer passed in is valid (otherwise it's just random memory, e.g., if you accidentally shifted the stack pointer forward one word). I was wondering more about the stack pointer that's returned. I.e., what if the stack frame passed in is the last valid stack frame?

This comment has been minimized.

@bradjc

bradjc Aug 1, 2018

Contributor

If the only stack frame is the the syscall, then the returned stack pointer is valid, but the stack is empty. Calling pop_syscall_stack() again would be invalid, but I don't think that the implementer can detect that it is invalid.

stack_pointer: *const usize,
callback: process::FunctionCall,
state: &Self::StoredState,
) -> *mut usize;

This comment has been minimized.

@phil-levis

phil-levis Aug 1, 2018

Collaborator

Same here -- what happens if you try to push past the end of the stack?

This comment has been minimized.

@brghena

brghena Aug 1, 2018

Contributor

Is there a way we can check that we're still within the bounds of process memory before writing to the stack pointer? If we aren't, then the response can be to put the process into a fault state.

This comment has been minimized.

@bradjc

bradjc Aug 1, 2018

Contributor

Right now, pop and push are the same size, so we currently are fine.

But in general, it is possible that push is a larger frame than what was popped, or that pop isn't required, or we want to push multiple frames for some reason.

What about adding an argument remaining_app_stack_memory to the function that would then enable the implementer to detect an overflow?

&self,
stack_pointer: *const usize,
state: &mut Self::StoredState,
) -> *mut usize;

This comment has been minimized.

@phil-levis

phil-levis Aug 1, 2018

Collaborator

Are there failure cases here? If so, then it should be possible to signal so in the return value.

/// 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 {

This comment has been minimized.

@phil-levis

phil-levis Aug 1, 2018

Collaborator

I'd suggest a different name. To me, SyscallInterface would imply what currently Driver is. This is really about function calls across the user/kernel boundary. So I might call it UserKernelBoundary or something similar. But I'm find with SyscallInterface if you want to keep it. Although when typed in a sans serif font llI reads funny.

bradjc added some commits Jul 13, 2018

kernel: move arch-specific syscall to cortex-m
By making a `ProcessType` trait that the `Process` struct implements,
the `Process` struct is now templated on a `SyscallInterface` trait
which implements the architecture specific operations for the syscall
interface. This means that process.rs is now generic with respect to the
architecture.
kernel: improve function names
also add comments
kernel: improve names and return values
- update names and comments in UserlandKernelBoundary
- Allow `push_function_call` to fail if there is no stack space left
@bradjc

This comment has been minimized.

Copy link
Contributor

bradjc commented Aug 5, 2018

Ok numerous things have been updated. Notably:

  • get_and_reset_context_switch_reason() is gone. The reason the process stopped running is now returned from the switch_to function.
  • The ProcessType trait functions have been cleaned up a little.
  • FunctionCall has argumentN instead of rN.
  • The code that handles processes stopping because of timeslice expiration has been made consistent with syscalls and faults.
  • Other is no longer a syscall switch reason. Instead arch/ defaults to the process had a Fault if it stops running for an unexpected reason.
  • Pushing a frame onto the process stack can fail if there is not enough process memory to add the frame.

Here are the two key traits (updated):

tock/kernel/src/syscall.rs

Lines 60 to 117 in 708f2ed

/// This trait must be implemented by the architecture of the chip Tock is
/// running on. It allows the kernel to manage switching to and from processes
/// in an architecture-agnostic manner.
pub trait UserspaceKernelBoundary {
/// Some architecture-specific struct containing per-process state that must
/// be kept while the process is not running. For example, for keeping CPU
/// registers that aren't stored on the stack.
type StoredState: Default;
/// Get the syscall that the process called with the appropriate arguments.
unsafe fn get_syscall(&self, stack_pointer: *const usize) -> Option<Syscall>;
/// Set the return value the process should see when it begins executing
/// again after the syscall.
unsafe fn set_syscall_return_value(&self, stack_pointer: *const usize, return_value: isize);
/// Remove the last stack frame from the process and return the new stack
/// pointer location.
///
/// This function assumes that `stack_pointer` is valid and at the end of
/// the process stack, that there is at least one stack frame on the
/// stack, and that that frame is the syscall.
unsafe fn pop_syscall_stack_frame(
&self,
stack_pointer: *const usize,
state: &mut Self::StoredState,
) -> *mut usize;
/// Add a stack frame with the new function call. This function
/// is what should be executed when the process is resumed.
///
/// `remaining_stack_memory` is the number of bytes below the
/// `stack_pointer` that is allocated for the process. This value is checked
/// by the implementer to ensure that there is room for this stack frame
/// without overflowing the stack.
///
/// Returns `Ok` with the new stack pointer after adding the stack frame if
/// there was room for the stack frame, and an error with where the stack
/// would have ended up if the function call had been added otherwise.
unsafe fn push_function_call(
&self,
stack_pointer: *const usize,
remaining_stack_memory: usize,
callback: process::FunctionCall,
state: &Self::StoredState,
) -> Result<*mut usize, *mut usize>;
/// Context switch to a specific process.
///
/// This returns a tuple:
/// - The new stack pointer address of the process.
/// - Why the process stopped executing and switched back to the kernel.
unsafe fn switch_to_process(
&self,
stack_pointer: *const usize,
state: &mut Self::StoredState,
) -> (*mut usize, ContextSwitchReason);
}

tock/kernel/src/process.rs

Lines 72 to 197 in 708f2ed

/// This trait is implemented by process structs.
pub trait ProcessType {
/// Queue a `Task` for the process. This will be added to a per-process
/// buffer and executed by the scheduler. `Task`s are some function the app
/// should run, for example a callback or an IPC call.
///
/// This function returns `true` if the `Task` was successfully enqueued,
/// and `false` otherwise. This is represented as a simple `bool` because
/// this is passed to the capsule that tried to schedule the `Task`.
fn enqueue_task(&self, task: Task) -> bool;
/// Remove the scheduled operation from the front of the queue and return it
/// to be handled by the scheduler.
///
/// If there are no `Task`s in the queue for this process this will return
/// `None`.
fn dequeue_task(&self) -> Option<Task>;
/// Returns the current state the process is in. Common states are "running"
/// or "yielded".
fn get_state(&self) -> State;
/// Move this process from the running state to the yielded state.
fn set_yielded_state(&self);
/// Put this process in the fault state. This will trigger the
/// `FaultResponse` for this process to occur.
unsafe fn set_fault_state(&self);
/// Get the name of the process. Used for IPC.
fn get_process_name(&self) -> &[u8];
// memop operations
/// Change the location of the program break.
fn brk(&self, new_break: *const u8) -> Result<*const u8, Error>;
/// Change the location of the program break and return the previous break
/// address.
fn sbrk(&self, increment: isize) -> Result<*const u8, Error>;
/// The start address of allocated RAM for this process.
fn mem_start(&self) -> *const u8;
/// The first address after the end of the allocated RAM for this process.
fn mem_end(&self) -> *const u8;
/// The start address of the flash region allocated for this process.
fn flash_start(&self) -> *const u8;
/// The first address after the end of the flash region allocated for this
/// process.
fn flash_end(&self) -> *const u8;
/// The lowest address of the grant region for the process.
fn kernel_memory_break(&self) -> *const u8;
/// How many writeable flash regions defined in the TBF header for this
/// process.
fn number_writeable_flash_regions(&self) -> usize;
/// Get the offset from the beginning of flash and the size of the defined
/// writeable flash region.
fn get_writeable_flash_region(&self, region_index: usize) -> (u32, u32);
/// Debug function to update the kernel on where the stack starts for this
/// process. Processes are not required to call this through the memop
/// system call, but it aids in debugging the process.
fn update_stack_start_pointer(&self, stack_pointer: *const u8);
/// Debug function to update the kernel on where the process heap starts.
/// Also optional.
fn update_heap_start_pointer(&self, heap_pointer: *const u8);
// additional memop like functions
/// Check if the buffer address and size is contained within the memory
/// owned by this process. This isn't quite the same as the memory allocated
/// to the process as this does not include the grant region which is owned
/// by the kernel.
fn in_app_owned_memory(&self, buf_start_addr: *const u8, size: usize) -> bool;
/// Get the first address of process's flash that isn't protected by the
/// kernel. The protected range of flash contains the TBF header and
/// potentially other state the kernel is storing on behalf of the process,
/// and cannot be edited by the process.
fn flash_non_protected_start(&self) -> *const u8;
// mpu
fn setup_mpu(&self, mpu: &mpu::MPU);
fn add_mpu_region(&self, base: *const u8, size: u32) -> bool;
// grants
/// Create new memory in the grant region.
unsafe fn alloc(&self, size: usize) -> Option<&mut [u8]>;
unsafe fn free(&self, _: *mut u8);
/// Get a pointer to the grant pointer for this grant number.
unsafe fn grant_ptr(&self, grant_num: usize) -> *mut *mut u8;
// functions for processes that are architecture specific
/// Get the syscall that the process called.
unsafe fn get_syscall(&self) -> Option<Syscall>;
/// Set the return value the process should see when it begins executing
/// again after the syscall.
unsafe fn set_syscall_return_value(&self, return_value: isize);
/// Remove the last stack frame from the process.
unsafe fn pop_syscall_stack_frame(&self);
/// Replace the last stack frame with the new function call. This function
/// is what should be executed when the process is resumed.
unsafe fn push_function_call(&self, callback: FunctionCall);
/// Context switch to a specific process.
unsafe fn switch_to(&self) -> Option<syscall::ContextSwitchReason>;
unsafe fn fault_str(&self, writer: &mut Write);
unsafe fn statistics_str(&self, writer: &mut Write);
}

@dverhaert

This comment has been minimized.

Copy link
Contributor

dverhaert commented Aug 6, 2018

I like these changes! Merging this soon would help out a lot for the MPU because we're blocking on the use of trait objects in the processes array.

@bradjc

This comment has been minimized.

Copy link
Contributor

bradjc commented Aug 9, 2018

@alevy

alevy approved these changes Aug 10, 2018

Copy link
Member

alevy left a comment

I'm in favor of merging this ASAP. It basically doesn't add any new problems with how Process is structured. It uncovers a bunch of existing issues I was always unhappy with (I implemented it, so it's my own damn fault). But assuming this is a milestone for the next release, it seems simpler to merge this and iterate on the Process trait/struct/boundary.

Meanwhile this lets @cmcavity and @dverhaert get on with their MPU trait work.

@bradjc

This comment has been minimized.

Copy link
Contributor

bradjc commented Aug 11, 2018

@hudson-ayers hudson-ayers self-assigned this Aug 11, 2018

@hudson-ayers hudson-ayers self-requested a review Aug 11, 2018

@bradjc bradjc referenced this pull request Aug 12, 2018

Merged

kernel: remove TBF header v1 #1150

2 of 2 tasks complete

bradjc added a commit that referenced this pull request Aug 12, 2018

@bradjc

This comment has been minimized.

Copy link
Contributor

bradjc commented Aug 13, 2018

@bradjc

This comment has been minimized.

Copy link
Contributor

bradjc commented Aug 13, 2018

bors r+

bors bot added a commit that referenced this pull request Aug 13, 2018

Merge #1113
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>
@bors

This comment has been minimized.

Copy link
Contributor

bors bot commented Aug 13, 2018

@bors bors bot merged commit 841d531 into master Aug 13, 2018

4 checks passed

bors Build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details

@bors bors bot deleted the arch-agnostic5 branch Aug 13, 2018

bors bot added a commit that referenced this pull request Aug 16, 2018

Merge #1115
1115: Move architecture-dependent debugging print code to arch/cortex-m r=bradjc a=bradjc

### Pull Request Overview

This pull finishes up #1113 by moving the `SCB_REGISTERS` variable to the cortex-m crate and all of the printing code that is architecture specific to the cortex-m crate.

I believe this is the last step to having process.rs be architecture agnostic.

~~Blocked on #1113.~~


### Testing Strategy

This pull request was tested by running crash dummy and hail on hail and seeing the panic output.


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

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

Merge #1150
1150: kernel: remove TBF header v1 r=bradjc a=bradjc

V1 of the TBF header has been deprecated for a while now, and there is very little chance that any boards still have apps with the old tbf header version.

This also removes the PIC options field that is not supported by the kernel, not standardized, and not something we want the kernel to have to support in the future. It is also, as far as we know, unused.

~~Blocked because #1113 _needs_ to be merged.~~

Fixes #1147.


### Testing Strategy

This pull request was tested by compiling.


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

@hudson-ayers hudson-ayers referenced this pull request Aug 22, 2018

Merged

IP Userland #1134

2 of 2 tasks complete

@cmcavity cmcavity referenced this pull request Aug 22, 2018

Closed

Process faults #1160

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment