From 02ec04aeb4c3dfba4b677123dad95bacc501b6ac Mon Sep 17 00:00:00 2001 From: Stephan Walter Date: Mon, 20 Jan 2025 21:12:13 +0100 Subject: [PATCH 1/6] Optimize critical_section: use MSR instruction To avoid branches and unnecessary code in the acquire and release functions, simply write back the original value to the PRIMASK register using the MSR instruction. This changes the critical-section dependency to use `u32` as the `RawRestoreState` type instead of `bool`. The `register::primask` module now defines a `struct(pub u32)` instead of an `enum`, and additionally a `write` function for use with the critical section. --- cortex-m/Cargo.toml | 2 +- cortex-m/src/critical_section.rs | 16 +++++++--------- cortex-m/src/register/primask.rs | 27 ++++++++++++--------------- testsuite/src/main.rs | 21 +++++++++++++++++++++ 4 files changed, 41 insertions(+), 25 deletions(-) diff --git a/cortex-m/Cargo.toml b/cortex-m/Cargo.toml index d90dc376..0c317c64 100644 --- a/cortex-m/Cargo.toml +++ b/cortex-m/Cargo.toml @@ -33,7 +33,7 @@ cm7 = [] cm7-r0p1 = ["cm7"] linker-plugin-lto = [] std = [] -critical-section-single-core = ["critical-section/restore-state-bool"] +critical-section-single-core = ["critical-section/restore-state-u32"] [package.metadata.docs.rs] targets = [ diff --git a/cortex-m/src/critical_section.rs b/cortex-m/src/critical_section.rs index ecf0681f..7a30d16e 100644 --- a/cortex-m/src/critical_section.rs +++ b/cortex-m/src/critical_section.rs @@ -1,3 +1,4 @@ +use core::sync::atomic::{compiler_fence, Ordering}; use critical_section::{set_impl, Impl, RawRestoreState}; use crate::interrupt; @@ -8,18 +9,15 @@ set_impl!(SingleCoreCriticalSection); unsafe impl Impl for SingleCoreCriticalSection { unsafe fn acquire() -> RawRestoreState { - let was_active = primask::read().is_active(); + let restore_state = primask::read(); // NOTE: Fence guarantees are provided by interrupt::disable(), which performs a `compiler_fence(SeqCst)`. interrupt::disable(); - was_active + restore_state.0 } - unsafe fn release(was_active: RawRestoreState) { - // Only re-enable interrupts if they were enabled before the critical section. - if was_active { - // NOTE: Fence guarantees are provided by interrupt::enable(), which performs a - // `compiler_fence(SeqCst)`. - interrupt::enable() - } + unsafe fn release(restore_state: RawRestoreState) { + // Ensure no preceeding memory accesses are reordered to after interrupts are enabled. + compiler_fence(Ordering::SeqCst); + primask::write(restore_state); } } diff --git a/cortex-m/src/register/primask.rs b/cortex-m/src/register/primask.rs index e95276fa..988d1ff8 100644 --- a/cortex-m/src/register/primask.rs +++ b/cortex-m/src/register/primask.rs @@ -3,26 +3,20 @@ #[cfg(cortex_m)] use core::arch::asm; -/// All exceptions with configurable priority are ... -#[derive(Clone, Copy, Debug, Eq, PartialEq)] -pub enum Primask { - /// Active - Active, - /// Inactive - Inactive, -} +/// Priority mask register +pub struct Primask(pub u32); impl Primask { /// All exceptions with configurable priority are active #[inline] pub fn is_active(self) -> bool { - self == Primask::Active + !self.is_inactive() } /// All exceptions with configurable priority are inactive #[inline] pub fn is_inactive(self) -> bool { - self == Primask::Inactive + self.0 & (1 << 0) == (1 << 0) } } @@ -32,9 +26,12 @@ impl Primask { pub fn read() -> Primask { let r: u32; unsafe { asm!("mrs {}, PRIMASK", out(reg) r, options(nomem, nostack, preserves_flags)) }; - if r & (1 << 0) == (1 << 0) { - Primask::Inactive - } else { - Primask::Active - } + Primask(r) +} + +/// Writes the CPU register +#[cfg(cortex_m)] +#[inline] +pub fn write(r: u32) { + unsafe { asm!("msr PRIMASK, {}", in(reg) r, options(nomem, nostack, preserves_flags)) }; } diff --git a/testsuite/src/main.rs b/testsuite/src/main.rs index f6718b36..569e1540 100644 --- a/testsuite/src/main.rs +++ b/testsuite/src/main.rs @@ -2,6 +2,7 @@ #![no_std] extern crate cortex_m_rt; +use core::sync::atomic::{AtomicBool, Ordering}; #[cfg(target_env = "")] // appease clippy #[panic_handler] @@ -11,8 +12,16 @@ fn panic(info: &core::panic::PanicInfo) -> ! { minitest::fail() } +static CRITICAL_SECTION_FLAG: AtomicBool = AtomicBool::new(false); + +#[cortex_m_rt::exception] +fn PendSV() { + CRITICAL_SECTION_FLAG.store(true, Ordering::SeqCst); +} + #[minitest::tests] mod tests { + use crate::{Ordering, CRITICAL_SECTION_FLAG}; use minitest::log; #[init] @@ -51,4 +60,16 @@ mod tests { assert!(!p.DWT.has_cycle_counter()); } } + + #[test] + fn critical_section_nesting() { + critical_section::with(|_| { + critical_section::with(|_| { + cortex_m::peripheral::SCB::set_pendsv(); + assert!(!CRITICAL_SECTION_FLAG.load(Ordering::SeqCst)); + }); + assert!(!CRITICAL_SECTION_FLAG.load(Ordering::SeqCst)); + }); + assert!(CRITICAL_SECTION_FLAG.load(Ordering::SeqCst)); + } } From d5591d00c7a288659f27ebd4dbfc51a5dfc477d1 Mon Sep 17 00:00:00 2001 From: Stephan Walter Date: Wed, 22 Jan 2025 11:37:40 +0100 Subject: [PATCH 2/6] also for interrupt::free; revert register::primask API changes --- cortex-m/src/critical_section.rs | 13 ++++++----- cortex-m/src/interrupt.rs | 11 ++++----- cortex-m/src/register/primask.rs | 40 +++++++++++++++++++++++++------- testsuite/src/main.rs | 26 ++++++++++++++++----- 4 files changed, 64 insertions(+), 26 deletions(-) diff --git a/cortex-m/src/critical_section.rs b/cortex-m/src/critical_section.rs index 7a30d16e..6bedfffa 100644 --- a/cortex-m/src/critical_section.rs +++ b/cortex-m/src/critical_section.rs @@ -1,4 +1,3 @@ -use core::sync::atomic::{compiler_fence, Ordering}; use critical_section::{set_impl, Impl, RawRestoreState}; use crate::interrupt; @@ -9,15 +8,17 @@ set_impl!(SingleCoreCriticalSection); unsafe impl Impl for SingleCoreCriticalSection { unsafe fn acquire() -> RawRestoreState { - let restore_state = primask::read(); + // Backup previous state of PRIMASK register. We access the entire register directly as a + // u32 instead of using the primask::read() function to minimize the number of processor + // cycles during which interrupts are disabled. + let restore_state = primask::read_raw(); // NOTE: Fence guarantees are provided by interrupt::disable(), which performs a `compiler_fence(SeqCst)`. interrupt::disable(); - restore_state.0 + restore_state } unsafe fn release(restore_state: RawRestoreState) { - // Ensure no preceeding memory accesses are reordered to after interrupts are enabled. - compiler_fence(Ordering::SeqCst); - primask::write(restore_state); + // NOTE: Fence guarantees are provided by primask::write_raw(), which performs a `compiler_fence(SeqCst)`. + primask::write_raw(restore_state); } } diff --git a/cortex-m/src/interrupt.rs b/cortex-m/src/interrupt.rs index 6b8f9e96..c8914f60 100644 --- a/cortex-m/src/interrupt.rs +++ b/cortex-m/src/interrupt.rs @@ -66,18 +66,17 @@ pub fn free(f: F) -> R where F: FnOnce() -> R, { - let primask = crate::register::primask::read(); + // Backup previous state of PRIMASK register. We access the entire register directly as a + // u32 instead of using the primask::read() function to minimize the number of processor + // cycles during which interrupts are disabled. + let primask = crate::register::primask::read_raw(); // disable interrupts disable(); let r = f(); - // If the interrupts were active before our `disable` call, then re-enable - // them. Otherwise, keep them disabled - if primask.is_active() { - unsafe { enable() } - } + crate::register::primask::write_raw(primask); r } diff --git a/cortex-m/src/register/primask.rs b/cortex-m/src/register/primask.rs index 988d1ff8..89178de9 100644 --- a/cortex-m/src/register/primask.rs +++ b/cortex-m/src/register/primask.rs @@ -2,36 +2,60 @@ #[cfg(cortex_m)] use core::arch::asm; +use core::sync::atomic::{compiler_fence, Ordering}; -/// Priority mask register -pub struct Primask(pub u32); +/// All exceptions with configurable priority are ... +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub enum Primask { + /// Active + Active, + /// Inactive + Inactive, +} impl Primask { /// All exceptions with configurable priority are active #[inline] pub fn is_active(self) -> bool { - !self.is_inactive() + self == Primask::Active } /// All exceptions with configurable priority are inactive #[inline] pub fn is_inactive(self) -> bool { - self.0 & (1 << 0) == (1 << 0) + self == Primask::Inactive } } -/// Reads the CPU register +/// Reads the prioritizable interrupt mask #[cfg(cortex_m)] #[inline] pub fn read() -> Primask { + if read_raw() & (1 << 0) == (1 << 0) { + Primask::Inactive + } else { + Primask::Active + } +} + +/// Reads the entire PRIMASK register +/// Note that bits [31:1] are reserved and UNK (Unknown) +#[cfg(cortex_m)] +#[inline] +pub fn read_raw() -> u32 { let r: u32; unsafe { asm!("mrs {}, PRIMASK", out(reg) r, options(nomem, nostack, preserves_flags)) }; - Primask(r) + r } -/// Writes the CPU register +/// Writes the entire PRIMASK register +/// Note that bits [31:1] are reserved and SBZP (Should-Be-Zero-or-Preserved) #[cfg(cortex_m)] #[inline] -pub fn write(r: u32) { +pub fn write_raw(r: u32) { + // Ensure no preceeding memory accesses are reordered to after interrupts are possibly enabled. + compiler_fence(Ordering::SeqCst); unsafe { asm!("msr PRIMASK, {}", in(reg) r, options(nomem, nostack, preserves_flags)) }; + // Ensure no subsequent memory accesses are reordered to before interrupts are possibly disabled. + compiler_fence(Ordering::SeqCst); } diff --git a/testsuite/src/main.rs b/testsuite/src/main.rs index 569e1540..d742c160 100644 --- a/testsuite/src/main.rs +++ b/testsuite/src/main.rs @@ -12,16 +12,16 @@ fn panic(info: &core::panic::PanicInfo) -> ! { minitest::fail() } -static CRITICAL_SECTION_FLAG: AtomicBool = AtomicBool::new(false); +static EXCEPTION_FLAG: AtomicBool = AtomicBool::new(false); #[cortex_m_rt::exception] fn PendSV() { - CRITICAL_SECTION_FLAG.store(true, Ordering::SeqCst); + EXCEPTION_FLAG.store(true, Ordering::SeqCst); } #[minitest::tests] mod tests { - use crate::{Ordering, CRITICAL_SECTION_FLAG}; + use crate::{Ordering, EXCEPTION_FLAG}; use minitest::log; #[init] @@ -63,13 +63,27 @@ mod tests { #[test] fn critical_section_nesting() { + EXCEPTION_FLAG.store(false, Ordering::SeqCst); critical_section::with(|_| { critical_section::with(|_| { cortex_m::peripheral::SCB::set_pendsv(); - assert!(!CRITICAL_SECTION_FLAG.load(Ordering::SeqCst)); + assert!(!EXCEPTION_FLAG.load(Ordering::SeqCst)); }); - assert!(!CRITICAL_SECTION_FLAG.load(Ordering::SeqCst)); + assert!(!EXCEPTION_FLAG.load(Ordering::SeqCst)); }); - assert!(CRITICAL_SECTION_FLAG.load(Ordering::SeqCst)); + assert!(EXCEPTION_FLAG.load(Ordering::SeqCst)); + } + + #[test] + fn interrupt_free_nesting() { + EXCEPTION_FLAG.store(false, Ordering::SeqCst); + cortex_m::interrupt::free(|| { + cortex_m::interrupt::free(|| { + cortex_m::peripheral::SCB::set_pendsv(); + assert!(!EXCEPTION_FLAG.load(Ordering::SeqCst)); + }); + assert!(!EXCEPTION_FLAG.load(Ordering::SeqCst)); + }); + assert!(EXCEPTION_FLAG.load(Ordering::SeqCst)); } } From 7f99b9a506a9196addcf641baabc762c0659bc53 Mon Sep 17 00:00:00 2001 From: Stephan Walter Date: Wed, 22 Jan 2025 14:07:03 +0100 Subject: [PATCH 3/6] appease clippy --- cortex-m/src/register/primask.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/cortex-m/src/register/primask.rs b/cortex-m/src/register/primask.rs index 89178de9..85b71bc4 100644 --- a/cortex-m/src/register/primask.rs +++ b/cortex-m/src/register/primask.rs @@ -2,6 +2,7 @@ #[cfg(cortex_m)] use core::arch::asm; +#[cfg(cortex_m)] use core::sync::atomic::{compiler_fence, Ordering}; /// All exceptions with configurable priority are ... From 78f7968a0c86004423538b4ed9aa75737b19b021 Mon Sep 17 00:00:00 2001 From: Stephan Walter Date: Wed, 22 Jan 2025 22:26:12 +0100 Subject: [PATCH 4/6] Add LTO hint to `critical-section-single-core` docs --- cortex-m/src/lib.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/cortex-m/src/lib.rs b/cortex-m/src/lib.rs index b4ae0157..b0ca35c9 100644 --- a/cortex-m/src/lib.rs +++ b/cortex-m/src/lib.rs @@ -19,6 +19,11 @@ //! 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. //! +//! The critical section has been optimized to block interrupts for as few cycles as possible, +//! but -- due to `critical-section` implementation details -- incurs branches in a normal build +//! configuration. For minimal interrupt latency, you can achieve inlining by enabling +//! [linker-plugin-based LTO](https://doc.rust-lang.org/rustc/linker-plugin-lto.html). +//! //! ## `cm7-r0p1` //! //! This feature enables workarounds for errata found on Cortex-M7 chips with revision r0p1. Some From 8279b63e0991424b2e6ae2784476d50bba818d0b Mon Sep 17 00:00:00 2001 From: Stephan Walter Date: Thu, 23 Jan 2025 11:41:24 +0100 Subject: [PATCH 5/6] Review: write_raw() must be unsafe --- cortex-m/src/interrupt.rs | 4 +++- cortex-m/src/register/primask.rs | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/cortex-m/src/interrupt.rs b/cortex-m/src/interrupt.rs index c8914f60..aa792201 100644 --- a/cortex-m/src/interrupt.rs +++ b/cortex-m/src/interrupt.rs @@ -76,7 +76,9 @@ where let r = f(); - crate::register::primask::write_raw(primask); + unsafe { + crate::register::primask::write_raw(primask); + } r } diff --git a/cortex-m/src/register/primask.rs b/cortex-m/src/register/primask.rs index 85b71bc4..166962e8 100644 --- a/cortex-m/src/register/primask.rs +++ b/cortex-m/src/register/primask.rs @@ -53,7 +53,7 @@ pub fn read_raw() -> u32 { /// Note that bits [31:1] are reserved and SBZP (Should-Be-Zero-or-Preserved) #[cfg(cortex_m)] #[inline] -pub fn write_raw(r: u32) { +pub unsafe fn write_raw(r: u32) { // Ensure no preceeding memory accesses are reordered to after interrupts are possibly enabled. compiler_fence(Ordering::SeqCst); unsafe { asm!("msr PRIMASK, {}", in(reg) r, options(nomem, nostack, preserves_flags)) }; From e451dc3058ee99ff63834514d2260a4274f4801f Mon Sep 17 00:00:00 2001 From: Stephan Walter Date: Fri, 24 Jan 2025 17:55:38 +0100 Subject: [PATCH 6/6] Added safety comment to write_raw() --- cortex-m/src/register/primask.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/cortex-m/src/register/primask.rs b/cortex-m/src/register/primask.rs index 166962e8..58b8c287 100644 --- a/cortex-m/src/register/primask.rs +++ b/cortex-m/src/register/primask.rs @@ -51,6 +51,13 @@ pub fn read_raw() -> u32 { /// Writes the entire PRIMASK register /// Note that bits [31:1] are reserved and SBZP (Should-Be-Zero-or-Preserved) +/// +/// # Safety +/// +/// This method is unsafe as other unsafe code may rely on interrupts remaining disabled, for +/// example during a critical section, and being able to safely re-enable them would lead to +/// undefined behaviour. Do not call this function in a context where interrupts are expected to +/// remain disabled -- for example, in the midst of a critical section or `interrupt::free()` call. #[cfg(cortex_m)] #[inline] pub unsafe fn write_raw(r: u32) {