Support "non-standard" interrupts and exceptions

[romancardenas]

Currently, the riscv and riscv-rt crates assume that the interrupt and exception sources are the ones gathered in the RISCV ISA. However, we do not support target-specific sources. Moreover, some RISC-V targets do not follow the standard interrupt scheme (e.g., ESP32 C3). Another example is the E310x microcontroller, which does not support supervisor interrupt sources.

Proposal

Now that we have the riscv-pac trait to define interrupt and exception sources, we should be able to adapt the rest of the crates to rely on generic functions with constraints to these traits. For example, the mcause register can have something like this:

// in riscv::register::mcause 
pub use riscv_pac::{CoreInterruptNumber, ExceptionNumber};

pub enum Trap<I, E> {
    Interrupt(I),
    Exception(E),
}

impl Mcause {
    pub fn cause<I: CoreInterruptNumber, E: ExceptionNumber>(&self) -> Trap<I, E> {
        if self.is_interrupt() {
            Trap::Interrupt(I::from_number(self.code()).unwrap())
        } else {
            Trap::Exception(E::from_number(self.code()).unwrap())
        }
    }
}

By default, I and E should be the standard enumerations (as we are doing now). Still, PACs will be able to opt out of this default implementation and provide their custom interrupt/exception enumeration.

Next, we can enrich riscv-rt macros to automatically generate the necessary code for letting PACs inject their custom interrupt and exception tables. Also, in the case of vectored mode, we could generate the necessary vector table and so on.

NOTE THAT THIS IS JUST MY PROPOSAL, but I would love to hear your opinion as well as clever mechanisms to implement this neatly.

I will do a self-review to point out some to-dos that I would like to discuss with you.

Solves #146

[github-actions]

This PR is being prevented from merging because it presents one of the blocking labels: work in progress, do not merge.

[almindor]

I think this is a good approach once it's clean and tested. I only had time to skim through it but don't see anything "wrong" with it.

[romancardenas]

Changes done. Please, take a look and let me know what you think. I personally think that we can improve it considerably. I will comment on the PR with my alternative idea.

[romancardenas]

Different approach: do not use generics nor enums at all in mcause and scause

I think the current proposal might become hard to handle for developers, as they now need to always work with generic arguments. I propose the following changes:

In riscv::register::xcause

• Remove Interrupt and Exception
• Change Trap<I, E> to:

pub enum Trap {
    Interrupt(usize),
    Exception(usize),
}

• Change Xcause::cause accordingly:

impl Mcause {
    pub cause(&self) -> Trap { ...}
}

In riscv::interrupt::{machine,supervisor}

• Here will be the standard Interrupt and Exception enums with the implementation of the riscv-pac traits
• Here we can add a generic cause function that will be the one used by users or frameworks such as RTIC:

pub use riscv_pac::{CoreInterruptNumber, InterruptNumber, ExceptionNumber};

/// Trap cause
#[derive(Copy, Clone, Debug, PartialEq, Eq)]
pub enum Cause<I, E> {
    Interrupt(I),
    Exception(E),
}

/// Trap cause error
#[derive(Copy, Clone, Debug, PartialEq, Eq)]
pub enum CauseError {
    InvalidInterrupt(usize),
    InvalidException(usize),
}

pub mod machine {
    use crate::register::{mcause, Trap}

    pub fn cause<I: CoreInterruptNumber, E: ExceptionNumber>() -> Result<Cause<I, E>, CauseError> {
        match mcause::read().cause() {
            Trap::Interrupt(i) => match I::from_number(i) {
                Ok(interrupt) => Ok(interrupt),
                Err(code) => Err(CauseError::InvalidInterrupt(code)),
            },
            Trap::Exception(e) => match E::from_number(e) {
                Ok(exception) => Ok(exception),
                Err(code) => Err(CauseError::InvalidException(code)),
            }
        }
    }
}
...

Naming can be improved. For instance, Xcause could return a riscv::interrupt::Cause<usize, usize> to avoid having too many similar enums.

[romancardenas]

Thanks for the feedback @rmsyn !

I implemented my alternative approach in the riscv-pac2 branch. Please take a look and let me know what you think. Personally, I feel like it is more user friendly, and somehow it makes sense to me that the Interrupt and Exception enums are in riscv::interrupt instead of riscv::register

[rmsyn]

it makes sense to me that the Interrupt and Exception enums are in riscv::interrupt instead of riscv::register

That makes sense to me, too.

Really either one can work, because the values are read from the mcause and scause registers (which makes sense for riscv::register).

riscv::interrupt makes obvious sense because Interrupt and Exception are explicitly about interrupts.

🤷

[rmsyn]

I probably need more time to review the proc-macro stuff, but it looks good to me.

[rmsyn]

I implemented my alternative approach in the riscv-pac2 branch.

After an initial review, I do like the code organization in riscv-pac2 more. Mostly aesthetic preferences, but it does feel like it clarifies things a bit.

The unification of one Trap type is also nice from an ergonomics perspective regarding the mcause/scause registers.

[romancardenas]

So I think the new riscv version looks good (still needs some testing etc.) BUT... it's time for riscv-rt and the interrupt/exception handlers. We need to provide an out-of-the-box solution for dealing with custom interrupts and exceptions. I guess that it's time for thinking on macros that:

• declare the interrupt and exception handlers as extern "C" fn
• define __INTERRUPTS and __EXCEPTIONS arrays from the enums
• In vectored mode, creates the vector table etc.

[romancardenas]

I found some time to keep working on this. In the new commit, the following code:

#[pac_enum(unsafe PriorityNumber)]
#[derive(Clone, Copy, Debug, Eq, PartialEq)]
enum Priority {
    P0 = 0,
    P1 = 1,
    P2 = 2,
    P3 = 3,
}

expands to:

#[derive(Clone, Copy, Debug, Eq, PartialEq)]
enum Priority {
    P0 = 0,
    P1 = 1,
    P2 = 2,
    P3 = 3,
}
unsafe impl riscv_pac::PriorityNumber for Priority {
    const MAX_PRIORITY_NUMBER: u8 = 3;
    #[inline]
    fn number(self) -> u8 {
        self as _
    }
    #[inline]
    fn from_number(number: u8) -> Result<Self, u8> {
        match number {
            3 => Ok(Self::P3),
            0 => Ok(Self::P0),
            1 => Ok(Self::P1),
            2 => Ok(Self::P2),
            _ => Err(number),
        }
    }
}

On the other hand, the following code:

#[pac_enum(unsafe CoreInterruptNumber)]
#[derive(Clone, Copy, Debug, Eq, PartialEq)]
enum Interrupt {
    I1 = 1,
    I2 = 2,
    I4 = 4,
    I7 = 7,
}

Expands to:

#[derive(Clone, Copy, Debug, Eq, PartialEq)]
enum Interrupt {
    I1 = 1,
    I2 = 2,
    I4 = 4,
    I7 = 7,
}
unsafe impl riscv_pac::InterruptNumber for Interrupt {
    const MAX_INTERRUPT_NUMBER: usize = 7;
    #[inline]
    fn number(self) -> usize {
        self as _
    }
    #[inline]
    fn from_number(number: usize) -> Result<Self, usize> {
        match number {
            2 => Ok(Self::I2),
            1 => Ok(Self::I1),
            4 => Ok(Self::I4),
            7 => Ok(Self::I7),
            _ => Err(number),
        }
    }
}
unsafe impl riscv_pac::CoreInterruptNumber for Interrupt {} // it also implements the marker trait

// It declares the existence of all the interrupt handlers
extern "C" {
    fn I2();
    fn I1();
    fn I4();
    fn I7();
}

// It generates the array with handlers
#[no_mangle]
pub static __CORE_INTERRUPTS: [Option<unsafe extern "C" fn()>; 7 + 1] = [
    None,
    Some(I1),
    Some(I2),
    None,
    Some(I4),
    None,
    None,
    Some(I7),
];

// It implements the interrupt dispatching function
#[no_mangle]
unsafe extern "C" fn _dispatch_core_interrupt(code: usize) {
    extern "C" {
        fn DefaultHandler();

    }
    if code < __CORE_INTERRUPTS.len() {
        let h = &__CORE_INTERRUPTS[code];
        if let Some(handler) = h {
            handler();
        } else {
            DefaultHandler();
        }
    } else {
        DefaultHandler();
    }
}

It should work as is for direct mode. I still need to adapt this solution to vectored mode.

[romancardenas]

It already creates the interrupt vector table. However, I think we should discuss the new approach with the @rust-embedded/tools team to align svd2rust with the new structure

[rmsyn]

Do we want to add an extra feature to the riscv crate to enable the vectors feature in riscv-pac?

[romancardenas]

So far, riscv-pac only has a default feature to opt out of procedural macros (i.e., syn, proc-macro2, and quote dependencies). However, I don't know if this effort is worthy it, as in the end, you will end up using riscv-rt, which depends on procedural macros.

[romancardenas]

maybe we could do something like:

[features]
default = ["riscv-pac/default"]

[rmsyn]

Minor style nit, and a quick question about a exposing riscv-pac features in riscv.

LGTM

[romancardenas]

Exciting times! I think the riscv ecosystem will improve a lot with #222 and this PR.

I will wait until #222 is solved and merge the outcome, so we can adapt the work to the new approach.
In the meantime, I will keep updating my forks for svd2rust so we have an out-of-the-box, easy-to-apply tool to help RISC-V PACs adopt our new ecosystem.

[romancardenas]

I updated this PR to use the new features coming from #222 .
An additional change is that I moved the macros to riscv instead of riscv-pac. As now riscv-pac is fully re-exported into riscv, it makes more sense, specially when leading with svd2rust (I'm also working on that).

This branch is quite a mess. I will divide it into small PRs so it will be easier to review.

PS: Rust nightly is complaining about a bug in LLVM for inline assembly with x86 targets. I think it is a matter of time that the error message dissapears: rust-lang/rust#127935

[rmsyn]

Rust nightly is complaining about a bug in LLVM for inline assembly with x86 targets

One potential way to solve that issue before the fix makes it into a nightly release could be to feature-gate all assembly with:

#[cfg(any(target_arch = "riscv32", target_arch = "riscv64"))]
...

[romancardenas]

The thing is that it still complains for RISC-V targets :/

[rmsyn]

The thing is that it still complains for RISC-V targets :/

What about switching the label to 1b:, b1:, or something similar? Not sure about any concern for label collision.

[romancardenas]

fixed

[romancardenas]

Closing this PR in favor of #223 , which has a cleaner commit history and cooler macros for defining interrupt and exception handlers