Re: [PATCH 1/3] rust: Introduce irq module

[Benno Lossin]

On 26.07.24 00:27, Lyude Paul wrote:
> This introduces a module for dealing with interrupt-disabled contexts,
> including the ability to enable and disable interrupts
> (with_irqs_disabled()) - along with the ability to annotate functions as
> expecting that IRQs are already disabled on the local CPU.
> 
> Signed-off-by: Lyude Paul <lyude@xxxxxxxxxx>
> ---
>  rust/helpers.c     | 14 +++++++++
>  rust/kernel/irq.rs | 74 ++++++++++++++++++++++++++++++++++++++++++++++
>  rust/kernel/lib.rs |  1 +
>  3 files changed, 89 insertions(+)
>  create mode 100644 rust/kernel/irq.rs
> 
> diff --git a/rust/helpers.c b/rust/helpers.c
> index 87ed0a5b60990..12ac32de820b5 100644
> --- a/rust/helpers.c
> +++ b/rust/helpers.c
> @@ -69,6 +69,20 @@ void rust_helper_spin_unlock(spinlock_t *lock)
>  }
>  EXPORT_SYMBOL_GPL(rust_helper_spin_unlock);
> 
> +unsigned long rust_helper_local_irq_save(void) {
> +	unsigned long flags;
> +
> +	local_irq_save(flags);
> +
> +	return flags;
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_local_irq_save);
> +
> +void rust_helper_local_irq_restore(unsigned long flags) {
> +	local_irq_restore(flags);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_local_irq_restore);
> +
>  void rust_helper_init_wait(struct wait_queue_entry *wq_entry)
>  {
>  	init_wait(wq_entry);
> diff --git a/rust/kernel/irq.rs b/rust/kernel/irq.rs
> new file mode 100644
> index 0000000000000..8a540bd6123f7
> --- /dev/null
> +++ b/rust/kernel/irq.rs
> @@ -0,0 +1,74 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Interrupt controls
> +//!
> +//! This module allows Rust code to control processor interrupts. [`with_irqs_disabled()`] may be
> +//! used for nested disables of interrupts, whereas [`IrqDisabled`] can be used for annotating code
> +//! that requires that interrupts already be disabled.
> +
> +use bindings;
> +use core::marker::*;
> +
> +/// A guarantee that IRQs are disabled on this CPU
> +///
> +/// An [`IrqDisabled`] represents a guarantee that interrupts will remain disabled on the current CPU
> +/// until the lifetime of the object ends. However, it does not disable or enable interrupts on its
> +/// own - see [`with_irqs_disabled()`] for that.
> +///
> +/// This object has no cost at runtime (TODO: …except if whatever kernel compile-time option that
> +/// would assert IRQs are enabled or not is enabled - in which case we should actually verify that
> +/// they're enabled).
> +///
> +/// # Examples
> +///
> +/// If you want to ensure that a function may only be invoked within contexts where interrupts are
> +/// disabled, you can do so by requiring that a reference to this type be passed. You can also
> +/// create this type using unsafe code in order to indicate that it's known that interrupts are
> +/// already disabled on this CPU
> +///
> +/// ```
> +/// use kernel::irq::{IrqDisabled, disable_irqs};
> +///
> +/// // Requiring interrupts be disabled to call a function
> +/// fn dont_interrupt_me(_irq: &IrqDisabled<'_>) { }

I would expect the function to take `IrqDisabled` by value instead of by
reference.

> +///
> +/// // Disabling interrupts. They'll be re-enabled once this closure completes.
> +/// disable_irqs(|irq| dont_interrupt_me(&irq));

Because then you don't need a borrow (`&`) here.

> +/// ```
> +pub struct IrqDisabled<'a>(PhantomData<&'a ()>);

You would also need to `#[derive(Clone, Copy)]` and since we're at it, I
would also add `Debug, Ord, Eq, PartialOrd, PartialEq, Hash`.
The last ones are important if we want to have structs that can only
exist while IRQs are disabled. I don't know if that makes sense, but I
think it's fine to add the derives now.

Another thing, I am wondering if we want this to be invariant over the
lifetime, I don't have a good reason, but I still think we should
consider it.

> +
> +impl<'a> IrqDisabled<'a> {
> +    /// Create a new [`IrqDisabled`] without disabling interrupts
> +    ///
> +    /// If debug assertions are enabled, this function will check that interrupts are disabled.
> +    /// Otherwise, it has no cost at runtime.

I don't see a check in the function below.

> +    ///
> +    /// # Safety
> +    ///
> +    /// This function must only be called in contexts where it is already known that interrupts have
> +    /// been disabled for the current CPU, as the user is making a promise that they will remain
> +    /// disabled at least until this [`IrqDisabled`] is dropped.
> +    pub unsafe fn new() -> Self {
> +        Self(PhantomData)
> +    }

What about adding a function here (taking `self` or `&self`, it doesn't
matter if you derived `Copy`) that checks if IRQs are disabled when
debug assertions are on?

> +}
> +
> +/// Run the closure `cb` with interrupts disabled on the local CPU.
> +///
> +/// Interrupts will be re-enabled once the closure returns. If interrupts were already disabled on
> +/// this CPU, this is a no-op.
> +#[inline]
> +pub fn with_irqs_disabled<T, F>(cb: F) -> T
> +where
> +    F: FnOnce(IrqDisabled<'_>) -> T,
> +{
> +    // SAFETY: FFI call with no special requirements

I vaguely remember that there were some problems with sleeping in IRQ
disabled contexts, is that me just misremembering (eg confusing it with
atomic context), or do we need to watch out for that?
Because if that is the case, then we would need to use klint.

> +    let flags = unsafe { bindings::local_irq_save() };
> +
> +    let ret = cb(IrqDisabled(PhantomData));
> +
> +    // SAFETY: `flags` comes from our previous call to local_irq_save
> +    unsafe { bindings::local_irq_restore(flags) };

Just to make sure, this function only enables interrupts, if they were
enabled before the call to `local_irq_save` above, right?

---
Cheers,
Benno

> +
> +    ret
> +}
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index e6b7d3a80bbce..37835ccd51087 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -36,6 +36,7 @@
>  pub mod firmware;
>  pub mod init;
>  pub mod ioctl;
> +pub mod irq;
>  #[cfg(CONFIG_KUNIT)]
>  pub mod kunit;
>  #[cfg(CONFIG_NET)]
> --
> 2.45.2
>