From 65db20e7e79eb9b718eda592f416804cc0d8b2a7 Mon Sep 17 00:00:00 2001 From: Dario Nieuwenhuis Date: Thu, 13 Oct 2022 01:35:56 +0200 Subject: [PATCH 1/2] Add implementation for critical-section 1.0 for single-hart chips. --- .github/workflows/ci.yaml | 10 +++++++++- CHANGELOG.md | 4 ++++ Cargo.toml | 4 ++++ src/critical_section.rs | 22 ++++++++++++++++++++++ src/lib.rs | 15 +++++++++++++++ 5 files changed, 54 insertions(+), 1 deletion(-) create mode 100644 src/critical_section.rs diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 70ebc99c..378e761d 100644 --- a/.github/workflows/ci.yaml +++ b/.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 diff --git a/CHANGELOG.md b/CHANGELOG.md index d08afa25..4f5c2537 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/Cargo.toml b/Cargo.toml index 43317ff0..46781db5 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -17,7 +17,11 @@ 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" diff --git a/src/critical_section.rs b/src/critical_section.rs new file mode 100644 index 00000000..623c6efa --- /dev/null +++ b/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() + } + } +} diff --git a/src/lib.rs b/src/lib.rs index f3e58f3c..06bfda6e 100644 --- a/src/lib.rs +++ b/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,6 @@ pub mod register; #[macro_use] mod macros; + +#[cfg(all(riscv, feature = "critical-section-single-hart"))] +mod critical_section; From caec77731c598d2f7dca854cc1ed2b326c3dcb8a Mon Sep 17 00:00:00 2001 From: Dario Nieuwenhuis Date: Thu, 13 Oct 2022 01:36:58 +0200 Subject: [PATCH 2/2] Fix interrupt::free() unsoundness on multi-hart systems. This is unsound on multi-hart because it only disables interrupts in the current hart. For multi-hart chips, a chip-specific critical section implementation is needed instead. Unsoundness is fixed by not returning the `CriticalSection` token. This is a breaking change. --- Cargo.toml | 1 - src/interrupt.rs | 20 ++++++++++++-------- src/lib.rs | 7 +++++++ src/macros.rs | 7 +++++-- 4 files changed, 24 insertions(+), 11 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 46781db5..29e7f961 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -21,7 +21,6 @@ targets = [ 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" diff --git a/src/interrupt.rs b/src/interrupt.rs index 14fc5d96..ee564efc 100644 --- a/src/interrupt.rs +++ b/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: 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 diff --git a/src/lib.rs b/src/lib.rs index 06bfda6e..7071403f 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -37,3 +37,10 @@ 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; +} diff --git a/src/macros.rs b/src/macros.rs index 9600b3cc..012063a4 100644 --- a/src/macros.rs +++ b/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)]