Merge #44

44: interrupt: Various improvements r=taiki-e a=taiki-e - Support RISC-V supervisor mode under `portable_atomic_s_mode` cfg > On RISC-V without A-extension, this generates code for machine-mode (M-mode) by default. If you pass the `--cfg portable_atomic_s_mode` together, this generates code for supervisor-mode (S-mode). In particular, `qemu-system-riscv*` uses [OpenSBI](https://github.com/riscv-software-src/opensbi) as the default firmware. - Support disabling FIQs on pre-v6 ARM under `portable_atomic_disable_fiq` cfg > On pre-v6 ARM, this disables only IRQs by default. For many systems (e.g., GBA) this is enough. If the system need to disable both IRQs and FIQs, you need to pass the `--cfg portable_atomic_disable_fiq` together. - Interrupt-related documentation improvements - [README.md](https://github.com/taiki-e/portable-atomic/blob/18961fab6716c3fe45e89cbf2abf46c6a10fad21/README.md#optional-cfg) - [src/imp/interrupt/README.md](https://github.com/taiki-e/portable-atomic/blob/18961fab6716c3fe45e89cbf2abf46c6a10fad21/src/imp/interrupt/README.md) - Defer mask until just before branch > This does not change the code generation, but in the actual generated code the mask is deferred until just before the branch, like this. Since there has been some misleading discussion about this in the past, we will use code that more closely matches the generated code. For MSP430 and AVR, it will be done in #40. Co-authored-by: Taiki Endo <[email protected]>
taiki-e · Oct 19, 2022 · c62df36 · c62df36
2 parents c4a6bc1 + d4ccac2
commit c62df36
Show file tree

Hide file tree

Showing 8 changed files with 142 additions and 37 deletions.
diff --git a/README.md b/README.md
@@ -94,18 +94,22 @@ See [this list](https://github.com/taiki-e/portable-atomic/issues/10#issuecommen
   This cfg is `unsafe`, and note the following safety requirements:
   - Enabling this cfg for multi-core systems is always **unsound**.
   - This uses privileged instructions to disable interrupts, so it usually doesn't work on unprivileged mode.
-    Enabling this cfg in an environment where privileged instructions are not available is also usually considered **unsound**, although the details are system-dependent.
-  - On pre-v6 ARM, this currently disables only IRQs.
-    Enabling this cfg in an environment where FIQs must also be disabled is also considered **unsound**.
+    Enabling this cfg in an environment where privileged instructions are not available, or if the instructions used are not sufficient to disable interrupts in the system, it is also usually considered **unsound**, although the details are system-dependent.
 
-  This is intentionally not an optional feature. (If this is an optional feature, dependencies can implicitly enable the feature, resulting in the use of unsound code without the end-user being aware of it.)
+    The following are known cases:
+    - On pre-v6 ARM, this disables only IRQs by default. For many systems (e.g., GBA) this is enough. If the system need to disable both IRQs and FIQs, you need to pass the `--cfg portable_atomic_disable_fiq` together.
+    - On RISC-V without A-extension, this generates code for machine-mode (M-mode) by default. If you pass the `--cfg portable_atomic_s_mode` together, this generates code for supervisor-mode (S-mode). In particular, `qemu-system-riscv*` uses [OpenSBI](https://github.com/riscv-software-src/opensbi) as the default firmware.
 
-  Enabling this cfg for targets that have atomic CAS will result in a compile error.
+    See also [the `interrupt` module's readme](https://github.com/taiki-e/portable-atomic/blob/HEAD/src/imp/interrupt/README.md).
+
+  This is intentionally not an optional feature. (If this is an optional feature, dependencies can implicitly enable the feature, resulting in the use of unsound code without the end-user being aware of it.)
 
   ARMv6-M (thumbv6m), pre-v6 ARM (e.g., thumbv4t, thumbv5te), RISC-V without A-extension are currently supported. See [#33] for support of multi-core systems.
 
   Since all MSP430 and AVR are single-core, we always provide atomic CAS for them without this cfg.
 
+  Enabling this cfg for targets that have atomic CAS will result in a compile error.
+
   Feel free to submit an issue if your target is not supported yet.
 
 ## Related Projects

diff --git a/src/imp/interrupt/README.md b/src/imp/interrupt/README.md
@@ -0,0 +1,21 @@
+# Implementation of disabling interrupts
+
+This module is used to provide atomic CAS for targets where atomic CAS is not available in the standard library.
+
+- On MSP430 and AVR, they are always single-core, so this module is always used.
+- On ARMv6-M (thumbv6m), pre-v6 ARM (e.g., thumbv4t, thumbv5te), RISC-V without A-extension, they could be multi-core, so this module is used when `--cfg portable_atomic_unsafe_assume_single_core` is enabled.
+
+The implementation uses privileged instructions to disable interrupts, so it usually doesn't work on unprivileged mode.
+Enabling this cfg in an environment where privileged instructions are not available, or if the instructions used are not sufficient to disable interrupts in the system, it is also usually considered **unsound**, although the details are system-dependent.
+
+For some targets, the implementation can be changed by explicitly enabling cfg.
+
+- On ARMv6-M, this disables interrupts by modifying the PRIMASK register.
+- On pre-v6 ARM, this disables interrupts by modifying the I (IRQ mask) bit of the CPSR.
+- On pre-v6 ARM with `--cfg portable_atomic_disable_fiq`, this disables interrupts by modifying the I (IRQ mask) bit and F (FIQ mask) bit of the CPSR.
+- On RISC-V (without A-extension), this disables interrupts by modifying the MIE (Machine Interrupt Enable) bit of the `mstatus` register.
+- On RISC-V (without A-extension) with `--cfg portable_atomic_s_mode`, this disables interrupts by modifying the SIE (Supervisor Interrupt Enable) bit of the `sstatus` register.
+- On MSP430, this disables interrupts by modifying the GIE (Global Interrupt Enable) bit of the status register (SR).
+- On AVR, this disables interrupts by modifying the I (Global Interrupt Enable) bit of the status register (SREG).
+
+Feel free to submit an issue if your target is not supported yet.
diff --git a/src/imp/interrupt/armv4t.rs b/src/imp/interrupt/armv4t.rs
@@ -1,8 +1,24 @@
 // Refs: https://developer.arm.com/documentation/ddi0406/cb/System-Level-Architecture/The-System-Level-Programmers--Model/ARM-processor-modes-and-ARM-core-registers/Program-Status-Registers--PSRs-?lang=en#CIHJBHJA
+//
+// Generated asm:
+// - armv5te https://godbolt.org/z/6oK9Ef7bv
 
 #[cfg(not(portable_atomic_no_asm))]
 use core::arch::asm;
 
+#[cfg(not(portable_atomic_disable_fiq))]
+macro_rules! if_disable_fiq {
+    ($tt:tt) => {
+        ""
+    };
+}
+#[cfg(portable_atomic_disable_fiq)]
+macro_rules! if_disable_fiq {
+    ($tt:tt) => {
+        $tt
+    };
+}
+
 #[derive(Clone, Copy)]
 pub(super) struct State(u32);
 
@@ -16,12 +32,13 @@ pub(super) fn disable() -> State {
     unsafe {
         // Do not use `nomem` and `readonly` because prevent subsequent memory accesses from being reordered before interrupts are disabled.
         asm!(
-            "mrs {0}, cpsr",
-            // We disable only IRQs. See also https://github.com/taiki-e/portable-atomic/pull/28#issuecomment-1214146912.
-            "orr {1}, {0}, 0x80", // I (IRQ mask) bit (1 << 7)
-            "msr cpsr_c, {1}",
-            out(reg) cpsr,
-            out(reg) _,
+            "mrs {prev}, cpsr",
+            "orr {new}, {prev}, 0x80", // I (IRQ mask) bit (1 << 7)
+            // We disable only IRQs by default. See also https://github.com/taiki-e/portable-atomic/pull/28#issuecomment-1214146912.
+            if_disable_fiq!("orr {new}, {new}, 0x40"), // F (FIQ mask) bit (1 << 6)
+            "msr cpsr_c, {new}",
+            prev = out(reg) cpsr,
+            new = out(reg) _,
             options(nostack, preserves_flags),
         );
     }
@@ -31,11 +48,11 @@ pub(super) fn disable() -> State {
 /// Restores the previous interrupt state.
 #[inline]
 #[instruction_set(arm::a32)]
-pub(super) unsafe fn restore(State(prev): State) {
+pub(super) unsafe fn restore(State(cpsr): State) {
     // SAFETY: the caller must guarantee that the state was retrieved by the previous `disable`,
     unsafe {
         // Do not use `nomem` and `readonly` because prevent preceding memory accesses from being reordered after interrupts are enabled.
-        asm!("msr cpsr_c, {0}", in(reg) prev, options(nostack));
+        asm!("msr cpsr_c, {0}", in(reg) cpsr, options(nostack));
     }
 }
 

diff --git a/src/imp/interrupt/armv6m.rs b/src/imp/interrupt/armv6m.rs
@@ -1,16 +1,19 @@
 // Adapted from https://github.com/rust-embedded/cortex-m.
+//
+// Generated asm:
+// - armv6-m https://godbolt.org/z/oMjd4h3Tr
 
 #[cfg(not(portable_atomic_no_asm))]
 use core::arch::asm;
 
 pub(super) use core::sync::atomic;
 
 #[derive(Clone, Copy)]
-pub(super) struct WasEnabled(bool);
+pub(super) struct State(u32);
 
 /// Disables interrupts and returns the previous interrupt state.
 #[inline]
-pub(super) fn disable() -> WasEnabled {
+pub(super) fn disable() -> State {
     let r: u32;
     // SAFETY: reading the priority mask register and disabling interrupts are safe.
     // (see module-level comments of interrupt/mod.rs on the safety of using privileged instructions)
@@ -23,13 +26,13 @@ pub(super) fn disable() -> WasEnabled {
             options(nostack, preserves_flags),
         );
     }
-    WasEnabled(r & 0x1 == 0)
+    State(r)
 }
 
 /// Restores the previous interrupt state.
 #[inline]
-pub(super) unsafe fn restore(WasEnabled(was_enabled): WasEnabled) {
-    if was_enabled {
+pub(super) unsafe fn restore(State(r): State) {
+    if r & 0x1 == 0 {
         // SAFETY: the caller must guarantee that the state was retrieved by the previous `disable`,
         // and we've checked that interrupts were enabled before disabling interrupts.
         unsafe {

diff --git a/src/imp/interrupt/mod.rs b/src/imp/interrupt/mod.rs
@@ -17,6 +17,8 @@
 // interrupts [^avr2] in atomic ops by default, is considered the latter.
 // MSP430 as well.
 //
+// See also README.md of this module.
+//
 // [^avr1]: https://github.com/llvm/llvm-project/blob/llvmorg-15.0.0/llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp#L1008
 // [^avr2]: https://github.com/llvm/llvm-project/blob/llvmorg-15.0.0/llvm/test/CodeGen/AVR/atomics/load16.ll#L5
 

diff --git a/src/imp/interrupt/riscv.rs b/src/imp/interrupt/riscv.rs
@@ -1,41 +1,73 @@
-// Based on asm generated for functions of interrupt module of https://github.com/rust-embedded/riscv.
+// Refs:
+// - https://five-embeddev.com/riscv-isa-manual/latest/machine.html#machine-status-registers-mstatus-and-mstatush
+// - https://five-embeddev.com/riscv-isa-manual/latest/supervisor.html#sstatus
+//
+// Generated asm:
+// - riscv64gc https://godbolt.org/z/TnPvPa4c4
 
 #[cfg(not(portable_atomic_no_asm))]
 use core::arch::asm;
 
 pub(super) use super::super::riscv as atomic;
 
+// Status register
+#[cfg(not(portable_atomic_s_mode))]
+macro_rules! status {
+    () => {
+        "mstatus"
+    };
+}
+#[cfg(portable_atomic_s_mode)]
+macro_rules! status {
+    () => {
+        "sstatus"
+    };
+}
+
+// MIE (Machine Interrupt Enable) bit (1 << 3)
+#[cfg(not(portable_atomic_s_mode))]
+const MASK: usize = 0x8;
+#[cfg(not(portable_atomic_s_mode))]
+macro_rules! mask {
+    () => {
+        "0x8"
+    };
+}
+// SIE (Supervisor Interrupt Enable) bit (1 << 1)
+#[cfg(portable_atomic_s_mode)]
+const MASK: usize = 0x2;
+#[cfg(portable_atomic_s_mode)]
+macro_rules! mask {
+    () => {
+        "0x2"
+    };
+}
+
 #[derive(Clone, Copy)]
-pub(super) struct WasEnabled(bool);
+pub(super) struct State(usize);
 
 /// Disables interrupts and returns the previous interrupt state.
 #[inline]
-pub(super) fn disable() -> WasEnabled {
+pub(super) fn disable() -> State {
     let r: usize;
     // SAFETY: reading mstatus and disabling interrupts is safe.
     // (see module-level comments of interrupt/mod.rs on the safety of using privileged instructions)
     unsafe {
         // Do not use `nomem` and `readonly` because prevent subsequent memory accesses from being reordered before interrupts are disabled.
-        asm!(
-            "csrr {0}, mstatus",
-            "csrci mstatus, 0x8",
-            out(reg) r,
-            options(nostack, preserves_flags),
-        );
+        asm!(concat!("csrrci {0}, ", status!(), ", ", mask!()), out(reg) r, options(nostack, preserves_flags));
     }
-    // MIE (Machine Interrupt Enable) bit (1 << 3)
-    WasEnabled(r & 0x8 != 0)
+    State(r)
 }
 
 /// Restores the previous interrupt state.
 #[inline]
-pub(super) unsafe fn restore(WasEnabled(was_enabled): WasEnabled) {
-    if was_enabled {
+pub(super) unsafe fn restore(State(r): State) {
+    if r & MASK != 0 {
         // SAFETY: the caller must guarantee that the state was retrieved by the previous `disable`,
         // and we've checked that interrupts were enabled before disabling interrupts.
         unsafe {
             // Do not use `nomem` and `readonly` because prevent preceding memory accesses from being reordered after interrupts are enabled.
-            asm!("csrsi mstatus, 0x8", options(nostack, preserves_flags));
+            asm!(concat!("csrsi ", status!(), ", ", mask!()), options(nostack, preserves_flags));
         }
     }
 }
diff --git a/src/lib.rs b/src/lib.rs
@@ -86,18 +86,22 @@ See [this list](https://github.com/taiki-e/portable-atomic/issues/10#issuecommen
   This cfg is `unsafe`, and note the following safety requirements:
   - Enabling this cfg for multi-core systems is always **unsound**.
   - This uses privileged instructions to disable interrupts, so it usually doesn't work on unprivileged mode.
-    Enabling this cfg in an environment where privileged instructions are not available is also usually considered **unsound**, although the details are system-dependent.
-  - On pre-v6 ARM, this currently disables only IRQs.
-    Enabling this cfg in an environment where FIQs must also be disabled is also considered **unsound**.
+    Enabling this cfg in an environment where privileged instructions are not available, or if the instructions used are not sufficient to disable interrupts in the system, it is also usually considered **unsound**, although the details are system-dependent.
 
-  This is intentionally not an optional feature. (If this is an optional feature, dependencies can implicitly enable the feature, resulting in the use of unsound code without the end-user being aware of it.)
+    The following are known cases:
+    - On pre-v6 ARM, this disables only IRQs by default. For many systems (e.g., GBA) this is enough. If the system need to disable both IRQs and FIQs, you need to pass the `--cfg portable_atomic_disable_fiq` together.
+    - On RISC-V without A-extension, this generates code for machine-mode (M-mode) by default. If you pass the `--cfg portable_atomic_s_mode` together, this generates code for supervisor-mode (S-mode). In particular, `qemu-system-riscv*` uses [OpenSBI](https://github.com/riscv-software-src/opensbi) as the default firmware.
 
-  Enabling this cfg for targets that have atomic CAS will result in a compile error.
+    See also [the `interrupt` module's readme](https://github.com/taiki-e/portable-atomic/blob/HEAD/src/imp/interrupt/README.md).
+
+  This is intentionally not an optional feature. (If this is an optional feature, dependencies can implicitly enable the feature, resulting in the use of unsound code without the end-user being aware of it.)
 
   ARMv6-M (thumbv6m), pre-v6 ARM (e.g., thumbv4t, thumbv5te), RISC-V without A-extension are currently supported. See [#33] for support of multi-core systems.
 
   Since all MSP430 and AVR are single-core, we always provide atomic CAS for them without this cfg.
 
+  Enabling this cfg for targets that have atomic CAS will result in a compile error.
+
   Feel free to submit an issue if your target is not supported yet.
 
 ## Related Projects

diff --git a/tools/build.sh b/tools/build.sh
@@ -78,6 +78,8 @@ default_targets=(
 known_cfgs=(
     docsrs
     portable_atomic_unsafe_assume_single_core
+    portable_atomic_s_mode
+    portable_atomic_disable_fiq
 )
 
 x() {
@@ -194,6 +196,16 @@ build() {
                         *)
                             RUSTFLAGS="${target_rustflags} --cfg portable_atomic_unsafe_assume_single_core" \
                                 x cargo "${args[@]}" --feature-powerset --manifest-path tests/api-test/Cargo.toml "$@"
+                            case "${target}" in
+                                thumbv[4-5]t* | armv[4-5]t*)
+                                    RUSTFLAGS="${target_rustflags} --cfg portable_atomic_unsafe_assume_single_core --cfg portable_atomic_disable_fiq" \
+                                        x cargo "${args[@]}" --feature-powerset --manifest-path tests/api-test/Cargo.toml "$@"
+                                    ;;
+                                riscv*)
+                                    RUSTFLAGS="${target_rustflags} --cfg portable_atomic_unsafe_assume_single_core --cfg portable_atomic_s_mode" \
+                                        x cargo "${args[@]}" --feature-powerset --manifest-path tests/api-test/Cargo.toml "$@"
+                                    ;;
+                            esac
                             ;;
                     esac
                 else
@@ -222,6 +234,16 @@ build() {
                     *)
                         RUSTFLAGS="${target_rustflags} --cfg portable_atomic_unsafe_assume_single_core" \
                             x cargo "${args[@]}" --target-dir target/assume-single-core "$@"
+                        case "${target}" in
+                            thumbv[4-5]t* | armv[4-5]t*)
+                                RUSTFLAGS="${target_rustflags} --cfg portable_atomic_unsafe_assume_single_core --cfg portable_atomic_disable_fiq" \
+                                    x cargo "${args[@]}" --target-dir target/assume-single-core-disable-fiq "$@"
+                                ;;
+                            riscv*)
+                                RUSTFLAGS="${target_rustflags} --cfg portable_atomic_unsafe_assume_single_core --cfg portable_atomic_s_mode" \
+                                    x cargo "${args[@]}" --target-dir target/assume-single-core-s-mode "$@"
+                                ;;
+                        esac
                         # portable-atomic-util uses atomic CAS, so doesn't work on
                         # this target without portable_atomic_unsafe_assume_single_core cfg.
                         args+=(--exclude portable-atomic-util)