zephyr: object: Improve docs about safety

Add comments to the object module describing the safety goals, and how
we achieve them.

Signed-off-by: David Brown <[email protected]>

Author: d3zd3z

zephyr/src/object.rs

@@ -13,37 +13,94 @@
 //! There are also kernel objects that are synthesized as part of the build.  Most notably, there
 //! are ones generated by the device tree.
 //!
-//! There are some funny rules about references and mutable references to memory that is
-//! inaccessible.  Basically, it is never valid to have a reference to something that isn't
-//! accessible.  However, we can have `*mut ktype` or `*const ktype`.  In Rust, having multiple
-//! `*mut` pointers live is undefined behavior.  Zephyr makes extensive use of shared mutable
-//! pointers (or sometimes immutable).  We will not dereference these in Rust code, but merely pass
-//! them to APIs in Zephyr that require them.
+//! ## Safety
 //!
-//! Most kernel objects use mutable pointers, and it makes sense to require the wrapper structure
-//! to be mutable to these accesses.  There a few cases, mostly generated ones that live in
-//! read-only memory, notably device instances, that need const pointers.  These will be
-//! represented by a separate wrapper.
+//! Zephyr has traditionally not focused on safety.  Early versions of project goals, in fact,
+//! emphasized performance and small code size as priorities over runtime checking of safety.  Over
+//! the years, this focus has changed a bit, and Zephyr does contain some additional checking, some
+//! of which is optional.
 //!
-//! # Initialization tracking
+//! Zephyr is still constrained at compile time to checks that can be performed with the limits
+//! of the C language.  With Rust, we have a much greater ability to enforce many aspects of safety
+//! at compile time.  However, there is some complexity to doing this at the interface between the C
+//! world and Rust.
 //!
-//! The Kconfig `CONFIG_RUST_CHECK_KOBJ_INIT` enabled extra checking in Rust-based kernel objects.
-//! This will result in a panic if the objects are used before the underlying object has been
-//! initialized.  The initialization must happen through the `StaticKernelObject::init_help`
-//! method.
+//! There are two types of kernel objects we deal with.  There are kernel objects that are allocated
+//! by C code (often auto-generated) that should be accessible to Rust.  These are mostly `struct
+//! device` values, and will be handled in a devices module.  The other type are objects that
+//! application code wishes to declare statically, and use from Rust code.  That is the
+//! responsibility of this module.  (There will also be support for more dynamic management of
+//! kernel objects, but this will be handled later).
 //!
-//! TODO: Document how the wrappers work once we figure out how to implement them.
+//! Static kernel objects in Zephyr are declared as C top-level variables (where the keyword static
+//! means something different).  It is the responsibility of the calling code to initialize these
+//! items, make sure they are only initialized once, and to ensure that sharing of the object is
+//! handled properly.  All of these are concerns we can handle in Rust.
+//!
+//! To handle initialization, we pair each kernel object with a single atomic value, whose zero
+//! value indicates [`KOBJ_UNINITIALIZED`].  There are a few instances of values that can be placed
+//! into uninitialized memory in a C declaration that will need to be zero initialized as a Rust
+//! static.  The case of thread stacks is handled as a special case, where the initialization
+//! tracking is kept separate so that the stack can still be placed in initialized memory.
+//!
+//! This state goes through two more values as the item is initialized, one indicating the
+//! initialization is happening, and another indicating it has finished.
+//!
+//! For each kernel object, there will be two types.  One, having a name of the form StaticThing,
+//! and the other having the form Thing.  The StaticThing will be used in a static declaration.
+//! There is a [`kobj_define!`] macro that matches declarations of these values and adds the
+//! necessary linker declarations to place these in the correct linker sections.  This is the
+//! equivalent of the set of macros in C, such as `K_SEM_DEFINE`.
+//!
+//! This StaticThing will have a single method [`init_once`] which accepts a single argument of a
+//! type defined by the object.  For most objects, it will just be an empty tuple `()`, but it can
+//! be whatever initializer is needed for that type by Zephyr.  Semaphores, for example, take the
+//! initial value and the limit.  Threads take as an initializer the stack to be used.
+//!
+//! This `init_once` will initialize the Zephyr object and return the `Thing` item that will have
+//! the methods on it to use the object.  Attributes such as `Sync`, and `Clone` will be defined
+//! appropriately so as to match the semantics of the underlying Zephyr kernel object.  Generally
+//! this `Thing` type will simply be a container for a direct pointer, and thus using and storing
+//! these will have the same characteristics as it would from C.
+//!
+//! Rust has numerous strict rules about mutable references, namely that it is not safe to have more
+//! than one mutable reference.  The language does allow multiple `*mut ktype` references, and their
+//! safety depends on the semantics of what is pointed to.  In the case of Zephyr, some of these are
+//! intentionally thread safe (for example, things like `k_sem` which have the purpose of
+//! synchronizing between threads).  Others are not, and that is mirrored in Rust by whether or not
+//! `Clone` and/or `Sync` are implemented.  Please see the documentation of individual entities for
+//! details for that object.
+//!
+//! In general, methods on `Thing` will require `&mut self` if there is any state to manage.  Those
+//! that are built around synchronization primitives, however, will generally use `&self`.  In
+//! general, objects that implement `Clone` will use `&self` because there would be no benefit to
+//! mutable self when the object could be cloned.
+//!
+//! [`kobj_define!`]: crate::kobj_define
+//! [`init_once`]: StaticKernelObject::init_once
 
 use core::{cell::UnsafeCell, mem};
 
 use crate::sync::atomic::{AtomicUsize, Ordering};
 
+// The kernel object itself must be wrapped in `UnsafeCell` in Rust.  This does several thing, but
+// the primary feature that we want to declare to the Rust compiler is that this item has "interior
+// mutability".  One impact will be that the default linker section will be writable, even though
+// the object will not be declared as mutable.  It also affects the compiler as it will avoid things
+// like aliasing and such on the data, as it will know that it is potentially mutable.  In our case,
+// the mutations happen from C code, so this is less important than the data being placed in the
+// proper section.  Many will have the link section overridden by the `kobj_define` macro.
+
 /// A kernel object represented statically in Rust code.
 ///
 /// These should not be declared directly by the user, as they generally need linker decorations to
 /// be properly registered in Zephyr as kernel objects.  The object has the underlying Zephyr type
 /// T, and the wrapper type W.
 ///
+/// Kernel objects will have their `StaticThing` implemented as `StaticKernelObject<kobj>` where
+/// `kobj` is the type of the underlying Zephyr object.  `Thing` will usually be a struct with a
+/// single field, which is a `*mut kobj`.
+///
 /// TODO: Can we avoid the public fields with a const new method?
 ///
 /// TODO: Handling const-defined alignment for these.
@@ -56,9 +113,23 @@ pub struct StaticKernelObject<T> {
     pub init: AtomicUsize,
 }
 
-/// Each can be wrapped appropriately.  The wrapped type is the instance that holds the raw pointer.
+/// Define the Wrapping of a kernel object.
+///
+/// This trait defines the association between a static kernel object and the two associated Rust
+/// types: `StaticThing` and `Thing`.  In the general case: there should be:
+/// ```
+/// impl Wrapped for StaticKernelObject<kobj> {
+///     type T = Thing,
+///     type I = (),
+///     fn get_wrapped(&self, args: Self::I) -> Self::T {
+///         let ptr = self.value.get();
+///         // Initizlie the kobj using ptr and possible the args.
+///         Thing { ptr }
+///     }
+/// }
+/// ```
 pub trait Wrapped {
-    /// The wrapped type.  This is what `take()` on the StaticKernelObject will return after
+    /// The wrapped type.  This is what `init_once()` on the StaticKernelObject will return after
     /// initialization.
     type T;
 
@@ -88,7 +159,8 @@ where
     StaticKernelObject<T>: Wrapped,
 {
     /// Construct an empty of these objects, with the zephyr data zero-filled.  This is safe in the
-    /// sense that Zephyr we track the initialization, and they start in the uninitialized state.
+    /// sense that Zephyr we track the initialization, they start in the uninitialized state, and
+    /// the zero value of the initialize atomic indicates that it is uninitialized.
     pub const fn new() -> StaticKernelObject<T> {
         StaticKernelObject {
             value: unsafe { mem::zeroed() },
@@ -100,6 +172,8 @@ where
     ///
     /// Will return a single wrapped instance of this object.  This will invoke the initialization,
     /// and return `Some<Wrapped>` for the wrapped containment type.
+    ///
+    /// If it is called an additional time, it will return None.
     pub fn init_once(&self, args: <Self as Wrapped>::I) -> Option<<Self as Wrapped>::T> {
         if let Err(_) = self.init.compare_exchange(
             KOBJ_UNINITIALIZED,
@@ -225,6 +299,13 @@ macro_rules! _kobj_stack {
         }
     };
 
+    // This initializer needs to have the elements of the array initialized to fixed elements of the
+    // `RealStaticThreadStack`.  Unfortunately, methods such as [`each_ref`] on the array are not
+    // const and can't be used in a static initializer.  We could use a recursive macro definition
+    // to perform the initialization, but this would require the array size to only be an integer
+    // literal (constants aren't calculated until after macro expansion).  It may also be possible
+    // to write a constructor for the array as a const fn, which would greatly simplify the
+    // initialization here.
     ($v:vis, $name: ident, $size:expr, $asize:expr) => {
         compile_error!("TODO: Stack initializer array");
     }