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};