Merge #110

110: Add critical-section 1.0 implementation, fix multicore unsoundness. r=almindor a=Dirbaio

~~Requires #109~~

This adds a [critical-section](https://github.com/rust-embedded/critical-section) implementation for single-core chips, based on disabling all interrupts.

`interrupt::free` is is unsound on multicore systems because it only disables interrupts in the
current core. For multicore chips, a chip-specific critical section implementationis needed instead. Unsoundness is fixed by not returning the `CriticalSection` token.

This is a breaking change.

This is the riscv equivalent of https://github.com/rust-embedded/cortex-m/pull/447 and https://github.com/rust-embedded/cortex-m/pull/448



Co-authored-by: Dario Nieuwenhuis <dirbaio@dirbaio.net>

.github/workflows/ci.yaml

@@ -38,6 +38,14 @@ jobs:
         run: cargo check --target riscv64imac-unknown-none-elf
       - name: Run CI script for riscv64gc-unknown-none-elf under ${{ matrix.rust }}
         run: cargo check --target riscv64gc-unknown-none-elf
+      - name: Run CI script for x86_64-unknown-linux-gnu under ${{ matrix.rust }} with critical-section-single-hart
+        run: cargo check --target x86_64-unknown-linux-gnu --features critical-section-single-hart
+      - name: Run CI script for riscv32imac-unknown-none-elf under ${{ matrix.rust }} with critical-section-single-hart
+        run: cargo check --target riscv32imac-unknown-none-elf --features critical-section-single-hart
+      - name: Run CI script for riscv64imac-unknown-none-elf under ${{ matrix.rust }} with critical-section-single-hart
+        run: cargo check --target riscv64imac-unknown-none-elf --features critical-section-single-hart
+      - name: Run CI script for riscv64gc-unknown-none-elf under ${{ matrix.rust }} with critical-section-single-hart
+        run: cargo check --target riscv64gc-unknown-none-elf --features critical-section-single-hart
 
   # On macOS and Windows, we at least make sure that the crate builds and links.
   build-other:
@@ -56,4 +64,4 @@ jobs:
           toolchain: stable
           override: true
       - name: Build crate for host OS
-        run: cargo build
+        run: cargo build --features critical-section-single-hart

CHANGELOG.md

@@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 
 ## [Unreleased]
 
+### Added
+
+- Added `critical-section-single-hart` feature which provides an implementation for the `critical_section` crate for single-hart systems, based on disabling all interrupts.
+
 ## [v0.9.0] - 2022-10-06
 
 ### Fixed

Cargo.toml

@@ -17,7 +17,10 @@ targets = [
     "riscv64imac-unknown-none-elf", "riscv64gc-unknown-none-elf",
 ]
 
+[features]
+critical-section-single-hart = ["critical-section/restore-state-bool"]
+
 [dependencies]
-bare-metal = "1.0.0"
 bit_field = "0.10.0"
+critical-section = "1.1.0"
 embedded-hal = "0.2.6"

src/critical_section.rs

@@ -0,0 +1,22 @@
+use critical_section::{set_impl, Impl, RawRestoreState};
+
+use crate::interrupt;
+use crate::register::mstatus;
+
+struct SingleHartCriticalSection;
+set_impl!(SingleHartCriticalSection);
+
+unsafe impl Impl for SingleHartCriticalSection {
+    unsafe fn acquire() -> RawRestoreState {
+        let was_active = mstatus::read().mie();
+        interrupt::disable();
+        was_active
+    }
+
+    unsafe fn release(was_active: RawRestoreState) {
+        // Only re-enable interrupts if they were enabled before the critical section.
+        if was_active {
+            interrupt::enable()
+        }
+    }
+}

src/interrupt.rs

@@ -2,9 +2,8 @@
 
 // NOTE: Adapted from cortex-m/src/interrupt.rs
 use crate::register::mstatus;
-pub use bare_metal::{CriticalSection, Mutex};
 
-/// Disables all interrupts
+/// Disables all interrupts in the current hart.
 #[inline]
 pub unsafe fn disable() {
     match () {
@@ -15,11 +14,11 @@ pub unsafe fn disable() {
     }
 }
 
-/// Enables all the interrupts
+/// Enables all the interrupts in the current hart.
 ///
 /// # Safety
 ///
-/// - Do not call this function inside an `interrupt::free` critical section
+/// - Do not call this function inside a critical section.
 #[inline]
 pub unsafe fn enable() {
     match () {
@@ -30,13 +29,18 @@ pub unsafe fn enable() {
     }
 }
 
-/// Execute closure `f` in an interrupt-free context.
+/// Execute closure `f` with interrupts disabled in the current hart.
 ///
-/// This as also known as a "critical section".
+/// This method does not synchronise multiple harts, so it is not suitable for
+/// using as a critical section. See the `critical-section` crate for a cross-platform
+/// way to enter a critical section which provides a `CriticalSection` token.
+///
+/// This crate provides an implementation for `critical-section` suitable for single-hart systems,
+/// based on disabling all interrupts. It can be enabled with the `critical-section-single-hart` feature.
 #[inline]
 pub fn free<F, R>(f: F) -> R
 where
-    F: FnOnce(&CriticalSection) -> R,
+    F: FnOnce() -> R,
 {
     let mstatus = mstatus::read();
 
@@ -45,7 +49,7 @@ where
         disable();
     }
 
-    let r = f(unsafe { &CriticalSection::new() });
+    let r = f();
 
     // If the interrupts were active before our `disable` call, then re-enable
     // them. Otherwise, keep them disabled

src/lib.rs

@@ -12,6 +12,18 @@
 //! - Access to core registers like `mstatus` or `mcause`.
 //! - Interrupt manipulation mechanisms.
 //! - Wrappers around assembly instructions like `WFI`.
+//!
+//! # Optional features
+//!
+//! ## `critical-section-single-hart`
+//!
+//! This feature enables a [`critical-section`](https://github.com/rust-embedded/critical-section)
+//! implementation suitable for single-hart targets, based on disabling interrupts globally.
+//!
+//! It is **unsound** to enable it on multi-hart targets,
+//! and may cause functional problems in systems where some interrupts must be not be disabled
+//! or critical sections are managed as part of an RTOS. In these cases, you should use
+//! a target-specific implementation instead, typically provided by a HAL or RTOS crate.
 
 #![no_std]
 
@@ -22,3 +34,13 @@ pub mod register;
 
 #[macro_use]
 mod macros;
+
+#[cfg(all(riscv, feature = "critical-section-single-hart"))]
+mod critical_section;
+
+/// Used to reexport items for use in macros. Do not use directly.
+/// Not covered by semver guarantees.
+#[doc(hidden)]
+pub mod _export {
+    pub use critical_section;
+}

src/macros.rs

@@ -6,7 +6,10 @@
 /// at most once in the whole lifetime of the program.
 ///
 /// # Note
-/// this macro is unsound on multi-core systems
+///
+/// This macro requires a `critical-section` implementation to be set. For most single-hart systems,
+/// you can enable the `critical-section-single-hart` feature for this crate. For other systems, you
+/// have to provide one from elsewhere, typically your chip's HAL crate.
 ///
 /// # Example
 ///
@@ -29,7 +32,7 @@
 #[macro_export]
 macro_rules! singleton {
     (: $ty:ty = $expr:expr) => {
-        $crate::interrupt::free(|_| {
+        $crate::_export::critical_section::with(|_| {
             static mut VAR: Option<$ty> = None;
 
             #[allow(unsafe_code)]