Merge pull request #576 from sw/cs-msr

Optimize critical_section: use MSR instruction

Author: adamgreig

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 = [

cortex-m/src/critical_section.rs

@@ -8,18 +8,17 @@ set_impl!(SingleCoreCriticalSection);
 
 unsafe impl Impl for SingleCoreCriticalSection {
     unsafe fn acquire() -> RawRestoreState {
-        let was_active = primask::read().is_active();
+        // 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();
-        was_active
+        restore_state
     }
 
-    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) {
+        // NOTE: Fence guarantees are provided by primask::write_raw(), which performs a `compiler_fence(SeqCst)`.
+        primask::write_raw(restore_state);
     }
 }

cortex-m/src/interrupt.rs

@@ -66,17 +66,18 @@ pub fn free<F, R>(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() }
+    unsafe {
+        crate::register::primask::write_raw(primask);
     }
 
     r

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

cortex-m/src/register/primask.rs

@@ -2,6 +2,8 @@
 
 #[cfg(cortex_m)]
 use core::arch::asm;
+#[cfg(cortex_m)]
+use core::sync::atomic::{compiler_fence, Ordering};
 
 /// All exceptions with configurable priority are ...
 #[derive(Clone, Copy, Debug, Eq, PartialEq)]
@@ -26,15 +28,42 @@ impl Primask {
     }
 }
 
-/// Reads the CPU register
+/// Reads the prioritizable interrupt mask
 #[cfg(cortex_m)]
 #[inline]
 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) {
+    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)) };
+    r
+}
+
+/// 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) {
+    // 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);
+}

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 EXCEPTION_FLAG: AtomicBool = AtomicBool::new(false);
+
+#[cortex_m_rt::exception]
+fn PendSV() {
+    EXCEPTION_FLAG.store(true, Ordering::SeqCst);
+}
+
 #[minitest::tests]
 mod tests {
+    use crate::{Ordering, EXCEPTION_FLAG};
     use minitest::log;
 
     #[init]
@@ -51,4 +60,30 @@ mod tests {
             assert!(!p.DWT.has_cycle_counter());
         }
     }
+
+    #[test]
+    fn critical_section_nesting() {
+        EXCEPTION_FLAG.store(false, Ordering::SeqCst);
+        critical_section::with(|_| {
+            critical_section::with(|_| {
+                cortex_m::peripheral::SCB::set_pendsv();
+                assert!(!EXCEPTION_FLAG.load(Ordering::SeqCst));
+            });
+            assert!(!EXCEPTION_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));
+    }
 }