Merge #262
262: Remove `libtock_platform::allows`. r=hudson-ayers a=jrvanwhy
Tock 2.0 system calls do not need the types in `allows`. Now that I'm reworking `libtock_platform` to use Tock 2.0 system calls, we can get rid of the module.
Co-authored-by: Johnathan Van Why <jrvanwhy@google.com>
diff --git a/core/platform/src/allows/allow_readable.rs b/core/platform/src/allows/allow_readable.rs
deleted file mode 100644
index c35701b..0000000
--- a/core/platform/src/allows/allow_readable.rs
+++ /dev/null
@@ -1,18 +0,0 @@
-/// Because the kernel may modify shared memory and place any bit pattern into
-/// that memory, we can only read types out of shared memory if every bit
-/// pattern is a valid value of that type. `AllowReadable` types are safe to
-/// read out of a shared buffer.
-pub unsafe trait AllowReadable {}
-
-unsafe impl AllowReadable for i8 {}
-unsafe impl AllowReadable for i16 {}
-unsafe impl AllowReadable for i32 {}
-unsafe impl AllowReadable for i64 {}
-unsafe impl AllowReadable for i128 {}
-unsafe impl AllowReadable for isize {}
-unsafe impl AllowReadable for u8 {}
-unsafe impl AllowReadable for u16 {}
-unsafe impl AllowReadable for u32 {}
-unsafe impl AllowReadable for u64 {}
-unsafe impl AllowReadable for u128 {}
-unsafe impl AllowReadable for usize {}
diff --git a/core/platform/src/allows/allowed.rs b/core/platform/src/allows/allowed.rs
deleted file mode 100644
index 386562f..0000000
--- a/core/platform/src/allows/allowed.rs
+++ /dev/null
@@ -1,71 +0,0 @@
-/// An individual value that has been shared with the kernel using the `allow`
-/// system call.
-// Allowed's implementation does not directly use the 'b lifetime. Platform uses
-// 'b to prevent the Allowed from accessing the buffer after the buffer becomes
-// invalid.
-// Allowed requires T to be Copy due to concerns about the semantics of
-// non-copyable types in shared memory as well as concerns about unexpected
-// behavior with Drop types. See the following PR discussion for more
-// information: https://github.com/tock/libtock-rs/pull/222
-pub struct Allowed<'b, T: Copy + 'b> {
- // Safety properties:
- // 1. `buffer` remains valid and usable for the lifetime of this Allowed
- // instance.
- // 2. Read and write accesses of `buffer`'s pointee must be performed as a
- // volatile operation, as the kernel may mutate buffer's pointee at any
- // time.
- // 3. The value `buffer` points to may have an arbitrary bit pattern in
- // it, so reading from `buffer` is only safe if the type contained
- // within is AllowReadable.
- buffer: core::ptr::NonNull<T>,
-
- // We need to use the 'b lifetime in Allowed to prevent an "unused lifetime"
- // compiler error. We use it here. Note that the phantom data must be an
- // &mut rather than a shared reference in order to make Allowed invariant
- // with respect to T. Invariance is required because Allowed allows us to
- // mutate the value in buffer, and invariance is the required property to do
- // so without allowing callers to produce dangling references. This is
- // documented at https://doc.rust-lang.org/nomicon/subtyping.html.
- _phantom: core::marker::PhantomData<&'b mut T>,
-}
-
-// Allowed's API is based on that of core::cell::Cell, but removes some methods
-// that are not safe for use with shared memory.
-//
-// Internally, Allowed performs accesses to the shared memory using volatile
-// reads and writes through raw pointers. We avoid constructing references to
-// shared memory because that leads to undefined behavior (there is some
-// background on this in the following discussion:
-// https://github.com/rust-lang/unsafe-code-guidelines/issues/33). Tock runs on
-// single-threaded platforms, some of which lack atomic instructions, so we only
-// need to be able to deconflict races between the kernel (which will never
-// interrupt an instruction's execution) and this process. Therefore volatile
-// accesses are sufficient to deconflict races.
-impl<'b, T: Copy + 'b> Allowed<'b, T> {
- // Allowed can only be constructed by the Platform. It is constructed after
- // the `allow` system call, and as such must accept a raw pointer rather
- // than a reference. The caller must make sure the following are true:
- // 1. buffer points to a valid instance of type T
- // 2. There are no references to buffer's pointee
- // 3. buffer remains usable until the Allowed's lifetime has ended.
- #[allow(dead_code)] // TODO: Remove when Platform is implemented
- pub(crate) unsafe fn new(buffer: core::ptr::NonNull<T>) -> Allowed<'b, T> {
- Allowed {
- buffer,
- _phantom: core::marker::PhantomData,
- }
- }
-
- // Sets the value in the buffer.
- pub fn set(&self, value: T) {
- unsafe {
- core::ptr::write_volatile(self.buffer.as_ptr(), value);
- }
- }
-}
-
-impl<'b, T: crate::AllowReadable + Copy + 'b> Allowed<'b, T> {
- pub fn get(&self) -> T {
- unsafe { core::ptr::read_volatile(self.buffer.as_ptr()) }
- }
-}
diff --git a/core/platform/src/allows/allowed_tests.rs b/core/platform/src/allows/allowed_tests.rs
deleted file mode 100644
index 1859dd1..0000000
--- a/core/platform/src/allows/allowed_tests.rs
+++ /dev/null
@@ -1,98 +0,0 @@
-use crate::Allowed;
-use core::marker::PhantomData;
-use core::ptr::NonNull;
-
-// How do we simulate accesses to the shared buffer by the kernel?
-//
-// Well, a naive way would be to mutate the `buffer` variable directly. Because
-// Allowed accesses the memory through a *mut pointer, such a test would compile
-// and run fine with the current Rust compiler. As far as I can tell, it would
-// not hit any behavior documented as undefined at
-// https://doc.rust-lang.org/stable/reference/behavior-considered-undefined.html,
-// nor would it cause rustc to generate LLVM bitcode that encounters undefined
-// behavior.
-//
-// However, the naive approach will throw an "undefined behavior" error when run
-// under Miri (e.g. with `cargo miri test`), which uses the stacked borrows
-// model [1]. In particular, accessing the `buffer` variable directly pops the
-// SharedRW off buffer's borrow stack, which prevents Allowed from using its
-// *mut pointer to access `buffer` afterwards. It is likely that Rust will adopt
-// the stacked borrows model as its formal model for borrow validity, and in
-// that case accessing `buffer` in that manner will become undefined behavior.
-// In addition, running these tests under Miri is highly valuable, as this is
-// tricky code to get correct and an unsound API may be hard to fix.
-//
-// Instead, we explicitly refer to buffer through use of a KernelPtr that
-// simulates the pointer that `allow()` would hand to the Tock kernel. As far as
-// the stacked borrows model is concerned, accesses through the KernelPtr
-// variable behave identically to mutations performed by the kernel. This
-// pattern allows us to use `cargo miri test` to not only execute the unit
-// tests, but to test whether Allowed would encounter undefined behavior when
-// interacting with a real Tock kernel.
-//
-// [1] https://plv.mpi-sws.org/rustbelt/stacked-borrows/paper.pdf
-struct KernelPtr<'b, T: Copy + 'b> {
- ptr: NonNull<T>,
-
- // We need to consume the 'b lifetime. This is very similar to Allowed's
- // implementation.
- _phantom: PhantomData<&'b mut T>,
-}
-
-impl<'b, T: Copy + 'b> KernelPtr<'b, T> {
- // The constructor for KernelPtr; simulates allow(). Returns both the
- // Allowed instance the Platform would return and a KernelPtr the test can
- // use to simulate a kernel.
- pub fn allow(buffer: &'b mut T) -> (Allowed<'b, T>, KernelPtr<'b, T>) {
- let ptr = NonNull::new(buffer).unwrap();
- // Discard buffer *without* creating a reference to it, as would be done
- // if we called drop().
- let _ = buffer;
- // All 3 preconditions of Allowed::new are satisfied by the fact that
- // `buffer` is directly derived from a &'b mut T.
- let allowed = unsafe { Allowed::new(ptr) };
- let kernel_ptr = KernelPtr {
- ptr,
- _phantom: PhantomData,
- };
- (allowed, kernel_ptr)
- }
-
- // Replaces the value in the buffer with a new one.
- pub fn set(&self, value: T) {
- unsafe {
- core::ptr::write(self.ptr.as_ptr(), value);
- }
- }
-
- // Copies the contained value out of the buffer.
- pub fn get(&self) -> T {
- unsafe { core::ptr::read(self.ptr.as_ptr()) }
- }
-}
-
-#[test]
-fn set() {
- let mut buffer = 1;
- let (allowed, kernel_ptr) = KernelPtr::allow(&mut buffer);
- assert_eq!(kernel_ptr.get(), 1);
-
- // Simulate the kernel replacing the value in buffer.
- kernel_ptr.set(2);
- allowed.set(3);
- assert_eq!(kernel_ptr.get(), 3);
-}
-
-#[test]
-fn get() {
- let mut buffer = 1;
- let (allowed, kernel_ptr) = KernelPtr::allow(&mut buffer);
- assert_eq!(kernel_ptr.get(), 1);
-
- assert_eq!(allowed.get(), 1);
- assert_eq!(kernel_ptr.get(), 1);
-
- kernel_ptr.set(2);
- assert_eq!(allowed.get(), 2);
- assert_eq!(kernel_ptr.get(), 2);
-}
diff --git a/core/platform/src/allows/mod.rs b/core/platform/src/allows/mod.rs
deleted file mode 100644
index 8cb2158..0000000
--- a/core/platform/src/allows/mod.rs
+++ /dev/null
@@ -1,8 +0,0 @@
-mod allow_readable;
-mod allowed;
-
-pub use allow_readable::AllowReadable;
-pub use allowed::Allowed;
-
-#[cfg(test)]
-mod allowed_tests;
diff --git a/core/platform/src/lib.rs b/core/platform/src/lib.rs
index 0588d14..0a9f936 100644
--- a/core/platform/src/lib.rs
+++ b/core/platform/src/lib.rs
@@ -1,21 +1,11 @@
#![no_std]
-// TODO: Implement this crate, which will be done piece-by-piece. Platform will
-// include:
-// 1. The Allowed and AllowedSlice abstractions for sharing memory with the
-// kernel
-// 2. The PlatformApi trait and Platform implementation.
-// 3. A system call trait so that Platform works in both real Tock apps and
-// unit test environments. [DONE]
-
-mod allows;
mod async_traits;
mod error_code;
mod raw_syscalls;
mod syscalls;
mod syscalls_impl;
-pub use allows::{AllowReadable, Allowed};
pub use async_traits::{CallbackContext, FreeCallback, Locator, MethodCallback};
pub use error_code::ErrorCode;
pub use raw_syscalls::{OneArgMemop, RawSyscalls, YieldType, ZeroArgMemop};
