Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions hal-core/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,4 @@ thiserror = { workspace = true }
maitake-sync = { path = "../maitake-sync", default-features = false }
mycelium-util = { path = "../util" }
embedded-graphics-core = { version = "0.3", optional = true }
portable-atomic = "1.2"
23 changes: 23 additions & 0 deletions hal-core/src/interrupt.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
use core::fmt;

pub mod ctx;
mod lock;
pub use self::ctx::Context;
pub use self::lock::IrqRawMutex;

Check failure on line 6 in hal-core/src/interrupt.rs

View workflow job for this annotation

GitHub Actions / cargo check (host)

error[E0432]: unresolved import `self::lock::IrqRawMutex` --> hal-core/src/interrupt.rs:6:9 | 6 | pub use self::lock::IrqRawMutex; | ^^^^^^^^^^^^----------- | | | | | help: a similar name exists in the module: `RawMutex` | no `IrqRawMutex` in `interrupt::lock`

Check failure on line 6 in hal-core/src/interrupt.rs

View workflow job for this annotation

GitHub Actions / clippy

error[E0432]: unresolved import `self::lock::IrqRawMutex` --> hal-core/src/interrupt.rs:6:9 | 6 | pub use self::lock::IrqRawMutex; | ^^^^^^^^^^^^----------- | | | | | help: a similar name exists in the module: `RawMutex` | no `IrqRawMutex` in `interrupt::lock`

Check failure on line 6 in hal-core/src/interrupt.rs

View workflow job for this annotation

GitHub Actions / docs

error[E0432]: unresolved import `self::lock::IrqRawMutex` --> hal-core/src/interrupt.rs:6:9 | 6 | pub use self::lock::IrqRawMutex; | ^^^^^^^^^^^^----------- | | | | | help: a similar name exists in the module: `RawMutex` | no `IrqRawMutex` in `interrupt::lock`
Comment on lines 5 to +6
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/IrqRawMutex/IrqSpinlock?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol


/// An interrupt controller for a platform.
pub trait Control {
Expand Down Expand Up @@ -40,6 +42,27 @@
}
}

pub trait MaskInterrupt<V> {
/// Mask the interrupt on vector `V`.
unsafe fn mask_irq(&self, vector: V);

/// Unmask the interrupt on vector `V`.
unsafe fn unmask_irq(&self, vector: V);
}

impl<T, V> MaskInterrupt<V> for &T
where
T: MaskInterrupt<V>,
{
unsafe fn mask_irq(&self, vector: V) {
(*self).mask_irq(vector);
}

unsafe fn unmask_irq(&self, vector: V) {
(*self).unmask_irq(vector);
}
}

pub trait Handlers<R: fmt::Debug + fmt::Display> {
fn page_fault<C>(cx: C)
where
Expand Down
116 changes: 116 additions & 0 deletions hal-core/src/interrupt/lock.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
use super::MaskInterrupt;
use maitake_sync::{blocking::RawMutex, spin::Spinlock};
use portable_atomic::{AtomicBool, Ordering};

/// A spinlock that also provides mutual exclusion against the interrupt handler
/// for a particular interrupt vector.
Comment on lines +5 to +6
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's particularly important that acquiring IrqSpinlock means you can modify the vector however you want, but it does not ensure the interrupt handler itself is not also running. i don't think we need that kind of exclusivity much (at all?) but it might be worth being more precise here.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, you mean that the ISR might have already been entered on another thread when the interrupt gets masked, so you don't actually have mutual exclusion from the ISR, just from new calls to the ISR? yeah. hmm. i actually think we probably do need to handle that somehow, which is pretty annoying...

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the obvious solution would be to have an atomic counter for each interrupt vector that's incremented when a handler is entered and decremented when it returns, and make the caller do this sequence:

  1. acquire spinlock (mutual exclusion against other non-ISR code)
  2. mask IRQ (mutual exclusion against new ISRs being entered)
  3. spin until ISR count is 0 (to wait for currently executing ISRs to finish)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... as long as you're not acquiring the lock while in the ISR... which seems like a bug to me

personally i think it's fine to leave mutual exclusion with the ISR running anywhere as an exercise for the reader as long as the docs are clear that that's how the responsibilities go. everything else seems like a question of how far out of your way you want to go to say "this is a bug, stop doing that"?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... as long as you're not acquiring the lock while in the ISR... which seems like a bug to me

yeah, the docs for this thing will (eventually) state that the lock API is not intended to be used in ISRs and trying to acquire it inside an ISR will deadlock.

personally i think it's fine to leave mutual exclusion with the ISR running anywhere as an exercise for the reader as long as the docs are clear that that's how the responsibilities go. everything else seems like a question of how far out of your way you want to go to say "this is a bug, stop doing that"?

i think we actually do need to worry about this. the reason for having an IRQ lock type at all is to be able to share non-atomic mutable state between the ISR and non-ISR code in a driver. we don't know at what point the interrupt handler function in the ISR will access the shared state; it might touch it at any point before returning from the ISR. so, we really cannot let non-ISR code access the state inside the lock unless we are sure that no ISRs are running and no new ISRs will be entered as long as the lock is held. luckily i think the ISR entry counter thingy is not too terrible...

///
/// While this spinlock is locked, the interrupt vector is masked, preventing
/// the interrupt handler from running. The interrupt is unmasked when the
/// spinlock is unlocked.
///
/// This type requires exclusive control over the masking and unmasking of that
/// interrupt vector.
#[derive(Debug)]
pub struct IrqSpinlock<I, V> {
lock: Spinlock,
ctrl: I,
vector: V,
}

pub struct IrqSpinlockTable<V, const VECTORS: usize> {
locks: [AtomicBool; VECTORS],
_v: core::marker::PhantomData<fn(V)>,
}

#[derive(Debug, Clone, PartialEq, Eq)]
pub enum ClaimLockError {
AlreadyClaimed,
NoSuchIrq,
}

impl<I, V> IrqSpinlock<I, V>
where
I: MaskInterrupt<V>,
V: Copy,
{
/// # Safety
///
/// Constructing an `IrqSpinlock` for a given interrupt vector requires
/// exclusive control over the masking and unmasking of that interrupt
/// vector.
pub unsafe fn new(ctrl: I, vector: V) -> Self {
Self {
lock: Spinlock::new(),
ctrl,
vector,
}
}
}

unsafe impl<I, V> RawMutex for IrqSpinlock<I, V>
where
I: MaskInterrupt<V>,
V: Copy,
{
type GuardMarker = ();

fn lock(&self) {
self.lock.lock();
unsafe {
// Safety: having locked the spinlock means we have exclusive access
// to that vector.
self.ctrl.mask_irq(self.vector)
};
}

fn try_lock(&self) -> bool {
if !self.lock.try_lock() {
return false;
}

unsafe {
// Safety: having locked the spinlock means we have exclusive access
// to that vector.
self.ctrl.mask_irq(self.vector)
};
true
}

unsafe fn unlock(&self) {
// Safety: the contract of `RawMutex::unlock` requires that the caller
// ensure that the mutex is locked, so this implies that we have
// exclusive access to that vector in the interrupt controller.
self.ctrl.unmask_irq(self.vector);
self.lock.unlock();
}

/// Returns `true` if the mutex is currently locked.
fn is_locked(&self) -> bool {
self.lock.is_locked()
}
}

impl<V, const VECTORS: usize> IrqSpinlockTable<V, VECTORS>
where
V: Into<usize> + Copy,
{
pub fn claim_lock<I>(&self, vector: V, ctrl: I) -> Result<IrqSpinlock<I, V>, ClaimLockError>
where
I: MaskInterrupt<V>,
{
let claimed = self
.locks
.get(vector.into())
.ok_or(ClaimLockError::NoSuchIrq)?;
claimed
.compare_exchange(true, false, Ordering::AcqRel, Ordering::Acquire)
.map_err(|_| ClaimLockError::AlreadyClaimed)?;
Ok(unsafe {
// Safety: we have just claimed the vector, so unless the interrupt
// controller hands out access to it through other mechanisms, we
// have exclusive access to it.
IrqSpinlock::new(ctrl, vector)
})
}
}
12 changes: 6 additions & 6 deletions src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,13 @@ mod alloc {
mycotest::decl_test! {
fn alloc_big() {
use alloc::vec::Vec;
let mut v = Vec::new();

for i in 0..2048 {
v.push(i);
}
// let mut v = Vec::new();
panic!("lol");
// for i in 0..2048 {
// v.push(i);
// }

tracing::info!(vec = ?v);
// tracing::info!(vec = ?v);
}
}
}
Expand Down
Loading