-
Notifications
You must be signed in to change notification settings - Fork 656
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
Create system capsules crate, move kernel implementations, update boards #3992
Conversation
These trait implementations do not need to be in the core kernel.
pub flash: &'static [u8], | ||
|
||
/// The footers of the process binary (may be zero-sized), which are metadata | ||
/// about the process not covered by integrity. Used, among other things, to | ||
/// store signatures. | ||
pub(crate) footers: &'static [u8], | ||
pub footers: &'static [u8], | ||
|
||
/// Collection of pointers to the TBF header in flash. | ||
pub(crate) header: tock_tbf::types::TbfHeader, | ||
pub header: tock_tbf::types::TbfHeader, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I don't think there's necessarily anything in a read-only reference to process flash that needs to be protected, this is a pretty big change in visibility. My instinct is that it's preferred to prevent any capsule from accessing this more willy-nilly.
Having only googled this briefly, can we use the pub (in path)
syntax to restrict this to just the system capsules?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a capsule can get a ProcessBinary
, I'm not sure there's any harm in accessing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should really think of pub(crate)
as serving two possible roles:
- Protecting things that can break security (e.g. exposing a private field or allowing construction of a protected type) from other crates
- Avoiding exposing things to other crates because it would make maintaining things harder if those methods/fields are not stable.
We don't really have a general story for (2) so I'm not super worried about this. I don't think (1) applies here.
//! This file contains definitions of policies the Tock kernel can use when | ||
//! managing processes. For example, these policies control decisions such as | ||
//! whether a specific process should be restarted. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file / description is a bit weird now since it doesn't actually contain any policies anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It still defines what we need policies for.
@@ -17,7 +14,7 @@ pub struct ProcessPrinterContext { | |||
/// The overall print message is broken in to chunks so that it can be fit | |||
/// in a small buffer that is called multiple times. This tracks which byte | |||
/// we are at so we can ignore the text before and print the next bytes. | |||
offset: usize, | |||
pub offset: usize, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same visibility comment here; more motivated by this being a somewhat tightly coupled interface than any concerns regarding access.
Spelling fix Co-authored-by: Pat Pannuto <pat.pannuto@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an incredibly good direction
Pull Request Overview
Implements #3845.
The core kernel exposes several interfaces for implementations to customize the kernel. We have some default implementations, and for convenience we have kept those implementations in the core kernel. However, this code does not need to be in the kernel crate. This PR moves these implementations to a new capsule crate (
system
capsules). This moves more code to crates whereunsafe
is prohibited.Testing Strategy
travis
TODO or Help Wanted
I thought moving the schedulers would make sense too, but the scheduler trait has unsafe functions and uses internal kernel APIs.
Documentation Updated
/docs
, or no updates are required.Formatting
make prepush
.