Skip to content
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

kern: add IRQ_STATUS syscall #1661

Merged
merged 23 commits into from
Mar 18, 2024
Merged

kern: add IRQ_STATUS syscall #1661

merged 23 commits into from
Mar 18, 2024

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented Mar 15, 2024

As described in #1659, we currently lack a mechanism for a task to query
the state of the interrupts mapped to its notification set. Because only
the kernel can touch the NVIC, such a mechanism must be added to the
kernel.

This branch adds a new system call, IRQ_STATUS (syscall 13).
IRQ_STATUS takes a notification mask as an argument, and returns a set
of bit flags representing the state of the interrupts mapped to that
notification set:

  • bit 0 is set if the interrupt has been enabled by the task (by calling
    IRQ_CONTROL) and unset if the interrupt has not been enabled
  • bit 1 is set if an interrupt is currently pending for any task in the
    notification set
  • bit 2 is set if a notification has been posted to the task for any
    interrupt in the notification set

If multiple bits are set in the notification mask, and all of those
notification bits are set to interrupts, the returned IrqState
bitflags will be the boolean OR of the states of all the interrupts.

Design Notes

We also considered hanging this off of the existing IRQ_CONTROL system
call, which currently takes an argument of either 0 or 1 to indicate
whether to enable or disable interrupts. We could, potentially, have
added the ability to pass 2 as well to query IRQ status. However, that
system call currently doesn't return anything, so adding a return value
would make it spill more registers, and @cbiffle liked that the argument
to it was essentially a bool. Thus, we decided to add a new system call.

I also considered a design where `IRQ_STATUS' returns three words, which
are bitmaps of which individual notifications in the notification set
are enabled, pending, and posted, respectively. This would let us return
more information when multiple bits are set in the notification mask.
However, it has a couple of downsides:

  • We would have to spill more registers in the IRQ_STATUS syscall
    stub, a cost which is inflicted on callers who only want to look up the
    status of a single IRQ
  • Currently, the generated lookup table of IRQs can't be queried for
    individual bits, only for the whole mask. We'd need to change how we
    generate that LUT to support this. This seemed kinda annoying.

Testing

In order to test this, I've also added a new KIPC operation to trigger a
software IRQ, given a task ID and notification mask. This is a privilege
operation that can only be called by the supervisor, so I've added a
test-runner IPC to ask the test runner (which runs as supervisor) to
please do it. Incidentally, this has the nice side benefit of letting us
test that IRQs are even dispatched to tasks at all, which is probably
worth having!

Closes #1659

@hawkw hawkw force-pushed the eliza/irq-status branch 2 times, most recently from fc8dc74 to 107efb2 Compare March 15, 2024 17:17
@hawkw hawkw marked this pull request as ready for review March 18, 2024 21:53
@hawkw
Copy link
Member Author

hawkw commented Mar 18, 2024

@cbiffle I'm somewhat on the fence about this potential change --- curious about how you feel about it:

diff --git a/sys/kern/src/arch/arm_m.rs b/sys/kern/src/arch/arm_m.rs
index 278f4721..21a4d154 100644
--- a/sys/kern/src/arch/arm_m.rs
+++ b/sys/kern/src/arch/arm_m.rs
@@ -1142,20 +1142,16 @@ pub unsafe extern "C" fn DefaultHandler() {
 pub fn disable_irq(n: u32) {
     // Disable the interrupt by poking the Interrupt Clear Enable Register.
     let nvic = unsafe { &*cortex_m::peripheral::NVIC::PTR };
-    let reg_num = (n / 32) as usize;
-    let bit_mask = 1 << (n % 32);
     unsafe {
-        nvic.icer[reg_num].write(bit_mask);
+        nvic.icer[irq_reg(n)].write(irq_bit(n));
     }
 }
 
 pub fn enable_irq(n: u32) {
     // Enable the interrupt by poking the Interrupt Set Enable Register.
     let nvic = unsafe { &*cortex_m::peripheral::NVIC::PTR };
-    let reg_num = (n / 32) as usize;
-    let bit_mask = 1 << (n % 32);
     unsafe {
-        nvic.iser[reg_num].write(bit_mask);
+        nvic.iser[irq_reg(n)].write(irq_bit(n));
     }
 }
 
@@ -1165,8 +1161,8 @@ pub fn irq_status(n: u32) -> abi::IrqStatus {
     let mut status = abi::IrqStatus::empty();
 
     let nvic = unsafe { &*cortex_m::peripheral::NVIC::PTR };
-    let reg_num = (n / 32) as usize;
-    let bit_mask = 1 << (n % 32);
+    let reg_num = irq_reg(n);
+    let bit_mask = irq_bit(n);
 
     // See if the interrupt is enabled by checking the bit in the Interrupt Set
     // Enable Register.
@@ -1183,12 +1179,18 @@ pub fn irq_status(n: u32) -> abi::IrqStatus {
 
 pub fn pend_software_irq(InterruptNum(n): InterruptNum) {
     let nvic = unsafe { &*cortex_m::peripheral::NVIC::PTR };
-    let reg_num = (n / 32) as usize;
-    let bit_mask = 1 << (n % 32);
 
     // Pend the IRQ by poking the corresponding bit in the Interrupt Set Pending
     // Register (ISPR).
-    unsafe { nvic.ispr[reg_num].write(bit_mask) };
+    unsafe { nvic.ispr[irq_reg(n)].write(irq_bit(n)) };
+}
+
+fn irq_reg(irq: u32) -> usize {
+    (irq / 32) as usize
+}
+
+fn irq_bit(irq: u32) -> u32 {
+    1 << (irq % 32)
 }
 
 #[repr(u8)]

Copy link
Collaborator

@cbiffle cbiffle left a comment

Choose a reason for hiding this comment

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

This is looking good! Caught a couple things below.

sys/userlib/src/lib.rs Outdated Show resolved Hide resolved
test/test-suite/src/main.rs Outdated Show resolved Hide resolved
test/test-suite/src/main.rs Outdated Show resolved Hide resolved
test/tests-psc/app.toml Outdated Show resolved Hide resolved
@hawkw hawkw requested a review from cbiffle March 18, 2024 23:07
@cbiffle
Copy link
Collaborator

cbiffle commented Mar 18, 2024

@cbiffle I'm somewhat on the fence about this potential change --- curious about how you feel about it:

re: the interrupt number extractor functions --- if you're excited about it, by all means! But I don't think it's necessary.

@hawkw hawkw merged commit 2770ffb into master Mar 18, 2024
83 checks passed
@hawkw hawkw deleted the eliza/irq-status branch March 18, 2024 23:38
hawkw added a commit that referenced this pull request Mar 19, 2024
Using the `irq_status` system call I added in #1661, we can extend the
panic function added to `stm32xx-i2c` in #1657 to also include the state
of any IRQs currently mapped to its notification mask. This way, if an
I2C driver panics, we can also include the kernel's understanding of its
IRQ state in the dump.
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.

Need a way for a task to _check_ its interrupt state
2 participants