riscv: Use riscv_pac::CoreInterruptNumber in xip registers

[romancardenas]

The methods for unpending interrupts via xip registers are marked as unsafe, as per the RISCV ISA specification:

"Each individual bit in register mip may be writable or may be read-only. When bit i in mip is writable, a pending interrupt i can be cleared by writing 0 to this bit. If interrupt i can become pending but bit i in mip is read-only, the implementation must provide some other mechanism for clearing the pending interrupt.

Therefore, each platform may provide a safe abstraction of these functions for their target when applies.

[rmsyn]

LGTM

[romancardenas]

I think we might face an issue when using the Mip::clear_pending method. For example:

1. We read the mip register and store its value in Mip. Interrupt i is pending, while interrupt j is not.
2. Interrupt j triggers and now mip registers this interrupt as pending
3. We clear the interrupt i in Mip and write its value to mip. As interrupts are cleared by writing a 0 in pending interrupts, we accidentally clear interrupt j.

Perhaps it is better to remove this method and only leave the atomic version mip::clear_pending

@rmsyn any thoughts?

[rmsyn]

I agree with your reasoning above, and may even want to remove the public write(mip: Mip) implementation, though we could also just provide documentation warning about the situation you described. We could replace it with and/or add a modify-style implementation, where the user provides a closure for a read-modify-write procedure.

Something like:

impl Mip {
    /// Performs a read-modify-write procedure on the [Mip] CSR.
    ///
    /// # Example
    ///
    /// ```rust, no_run
    /// use riscv::register::Mip;
    ///
    /// Mip::modify_pending(|mip| {
    ///     if mip.is_pending(MTIMER) {
    ///         mip.clear_pending(MTIMER);
    ///     }
    ///
    ///     // ... do other mip things
    ///
    ///     // return the modified in-memory structure to write
    ///     mip
    /// });
    /// ```
    pub fn modify_pending(f: FnOnce(Self) -> Self) {
        unsafe { write(f(read())) }
    }
}

There is still the off chance that the above runs into the same issue regarding an interrupt firing while processing the closure, but the tighter timing decreases the likelihood.

We could also provide some documentation that for the most control, users should reach for the free is_pending and clear_pending functions.

The closure approach does somewhat limit how the user responds to the interrupts since they can't pass any local variables.

[romancardenas]

I set all Xip fields read-only to avoid unwanted interrupt clears. I left the registers as read_write to allow things like mip::write(Mip::from_bits(0)).

[rmsyn]

I set all Xip fields read-only to avoid unwanted interrupt clears. I left the registers as read_write to allow things like mip::write(Mip::from_bits(0)).

I don't think that really addresses the issue you brought up, since a previous value could still clear an interrupt that fired in between the read and write calls. Same with my suggested solution, the issue still exists.

Another potential solution is to manually implement write to only write back clear values (0) using a cas atomic operation. If the interrupt index is 1, and the corresponding in-memory value is 0, clear the interrupt. Same for setting interrupts, something like an XOR using cas operations. Even that doesn't fully address the issue, because the mip::read call could return a 0 for an interrupt position, an interrupt could fire switching the bit to a 1, and the write(Mip) call would clear an unhandled interrupt.

Again, the only sure way I see to resolve the issue is to force atomic operations, or some form of tracking for changes. For example, add a was_cleared boolean to each bit position that only becomes true when a change from set to clear happens. It could also be a separate XipState bitfield added only to the in-memory structs. Then, only clear interrupts when was_cleared is true for each interrupt position. I don't know, we can only hand-hold users so much, and this is still a fairly low-level library. At a certain point, it's up to the users to figure it out with some helpful guidance from our docs.

Maybe there are other solutions?

[romancardenas]

I would go for a read-only mechanism for xip registers, with unsafe fn clear_pending<I>() functions that use atomic instructions to clear interrupts. If a given target presents a more sophisticated xip register, it should be reimplemented on the PAC or in an riscv-chipid crate that reexposes parts of riscv and reimplements the remaining.

[rmsyn]

I would go for a read-only mechanism for xip registers, with unsafe fn clear_pending<I>() functions that use atomic instructions to clear interrupts. If a given target presents a more sophisticated xip register, it should be reimplemented on the PAC or in an riscv-chipid crate that reexposes parts of riscv and reimplements the remaining.

That sounds like a much simpler solution! LGTM