-
Notifications
You must be signed in to change notification settings - Fork 186
riscv: add the mvien + mvienh CSR
#361
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Adds the `mvien` + `mvienh` CSR to represent the `Machine Virtual Interrupt Enable` registers.
riscv/src/register/mvien.rs
Outdated
| read_write_csr_field! { | ||
| Mvien, | ||
| /// Represents the enable status of a virtual major interrupt. | ||
| interrupt: 13..=31, | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about this. Take a look at mie: https://github.com/rust-embedded/riscv/blob/master/riscv/src/register/mie.rs
As each bit is a field, we probably prefer to use something like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, for composite registers, we might want to use this:
riscv/riscv/src/register/instret.rs
Line 8 in 8499f1a
| read_composite_csr!(super::instreth::read(), read()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As each bit is a field, we probably prefer to use something like that.
Thanks, I applied the naming, and patterns from mie. It's not a 1-to-1, since some of mie fields are not valid in mvien.
I also applied the changes as fixup commits. After final review, I'll squash all the commits down to one.
Also, for composite registers, we might want to use this
Thanks for the suggestion, but I don't think this applies. Since we aren't reading the value as a usize/u64, using this will throw a compile error.
However, I did add the field helpers from Mvien to the Mvienh type. If we still want to read the raw u64 value, I guess we could use the unsafe _read()read().bits() methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about returning results to notify users about potential issues with their interrupt enum?
riscv/src/register/mvien.rs
Outdated
|
|
||
| /// Enable a specific core interrupt source. | ||
| #[inline] | ||
| pub fn enable<I: InterruptNumber>(&mut self, interrupt: I) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might prefer to return a riscv::Result<()> to notify users that the interrupt has been effectively enabled. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM, I'll switch the methods to be fallible, and document the error conditions.
riscv/src/register/mvien.rs
Outdated
|
|
||
| /// Disable a specific core interrupt source. | ||
| #[inline] | ||
| pub fn disable<I: InterruptNumber>(&mut self, interrupt: I) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
riscv/src/register/mvienh.rs
Outdated
|
|
||
| /// Enable a specific core interrupt source. | ||
| #[inline] | ||
| pub fn enable<I: InterruptNumber>(&mut self, interrupt: I) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
riscv/src/register/mvienh.rs
Outdated
|
|
||
| /// Disable a specific core interrupt source. | ||
| #[inline] | ||
| pub fn disable<I: InterruptNumber>(&mut self, interrupt: I) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
I think this is a good idea. Do you think we should also provide an example pub struct VirtualInterrupt {
SupervisorSoftware = 1,
SupervisorTimer = 5,
SupervisorExternal = 9,
Interrupt(RangedUsize<13, 63>),
}
/// SAFETY: `Interrupt` represents the virtual RISC-V interrupts
unsafe impl InterruptNumber for VirtualInterrupt {
const MAX_INTERRUPT_NUMBER: usize = Mvien::MAX_INTERRUPT;
#[inline]
fn number(self) -> usize {
match self {
Self::SupervisorSoftware => 1,
Self::SupervisorTimer => 5,
Self::SupervisorExternal => 9,
Self::Interrupt(int) => int.inner(),
}
}
#[inline]
fn from_number(value: usize) -> Result<Self> {
match value {
1 => Ok(Self::SupervisorSoftware),
5 => Ok(Self::SupervisorTimer),
9 => Ok(Self::SupervisorExternal),
_ => RangedUsize::try_from(value).map(Self::Interrupt),
}
}
}Maybe this is better left to higher-level libraries? |
Adds the
mvien+mvienhCSR to represent theMachine Virtual Interrupt Enableregisters.Related: #1, #226