Skip to content

Conversation

@romancardenas
Copy link
Contributor

@romancardenas romancardenas commented Jul 15, 2025

This PR adds new methods and functions for the mie CSR. The idea is to use the riscv_pac::CoreInterruptNumber trait to allow enabling and disabling target-specific interrupt sources (i.e., extending our API to non-standard interrupt sources).

Closes #314

@romancardenas romancardenas requested a review from a team as a code owner July 15, 2025 06:34
Copy link
Contributor

@rmsyn rmsyn left a comment

Choose a reason for hiding this comment

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

Some suggestions for code re-use, otherwise LGTM.

@romancardenas
Copy link
Contributor Author

Ooops! I pushed a few more changes. Can you please review the PR again? I will address your comments after your new review!

Copy link
Contributor

@rmsyn rmsyn left a comment

Choose a reason for hiding this comment

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

Small change requests for code-reuse. Otherwise, LGTM.

rmsyn
rmsyn previously approved these changes Jul 16, 2025
@romancardenas
Copy link
Contributor Author

Before merging, I have some doubts about the naming:

What do you think about having fn riscv::register::mie::enable<I: CoreInterruptNumber>() and fn riscv::interrupt::enable_interrupt<I: CoreInterruptNumber>()`? Or do you prefer both functions to have the same name?

Additionally, should I include riscv::interrupt::is_interrupt_enabled<I>()? Maybe we don't want this kind of functions exposed in riscv::interrupt

@rmsyn
Copy link
Contributor

rmsyn commented Jul 18, 2025

What do you think about having fn riscv::register::mie::enable<I: CoreInterruptNumber>() and fn riscv::interrupt::enable_interrupt<I: CoreInterruptNumber>()`?

Either one is fine, I don't have strong opinions either way. enable is shorter, enable_interrupt is more explicit (though maybe redundant).

Or do you prefer both functions to have the same name?

Whatever name you choose, I would personally prefer consistent naming. Up to you, though.

Additionally, should I include riscv::interrupt::is_interrupt_enabled? Maybe we don't want this kind of functions exposed in riscv::interrupt

I think it would be good to include it. The same type of user that would want the other free functions would probably also prefer this one to be available.

@romancardenas
Copy link
Contributor Author

I changed it to mie::enable because I found mie::enable_interrupt repetitive (machine interrupt enable :: enable interrupt...). I also added the interrupt::is_interrupt_enabled function.

Another philosophical question... Should mie::enable be unsafe? We are enabling an interrupt source, yes, but interrupts are effectively active in mstatus with mstatus::set_mie(). Usual critical section implementations manipulate the status register, not the mie register.

We can either leave both mie::enable and mstatus::set_mie unsafe or assume that enabling interrupt sources in mie is safe and unsafety comes from mstatus.

In any case, if you prefer, we can discuss this in a future PR

@rmsyn
Copy link
Contributor

rmsyn commented Jul 20, 2025

Should mie::enable be unsafe?

I think, for the same reasons enabling mie in mstatus is unsafe, enabling individual interrupt sources in mie should also be unsafe.

We have no way of knowing that all interrupt sources on all platforms are safe to enable. We also know that some interrupt sources are unsafe/unsound to enable in some contexts.

In my opinion, the above means that we should mark it / leave it marked unsafe, and describe the invariants users need to consider / maintain. Another point is that we cannot, without a very convoluted setup, determine mstatus::mie settings at runtime to know when it is safe vs. unsafe to enable interrupt sources in the mie CSR. This would probably be better left to higher-level libraries to provide those kind of abstractions.

@romancardenas romancardenas added this pull request to the merge queue Jul 21, 2025
Merged via the queue into master with commit c8c0bba Jul 21, 2025
139 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

riscv: Enable setting external interrupt bits in mie

2 participants