Repository: tock/tock (language: rust)

Issue:
[RISCV] "Non-standard" mcause bits are lost in conversion
In this code:

https://github.com/tock/tock/blob/22ab469dfe3aeaac01e3947aedabacf0a697d312/arch/riscv/src/csr/mcause.rs#L5-L19

the `reason` field is 4-bit wide, and the comment is explaining why. But there is a problem if an implementation actually needs the higher values. The issue stems from the `Interrupt::from_reason()` implementation:

https://github.com/tock/tock/blob/22ab469dfe3aeaac01e3947aedabacf0a697d312/arch/riscv/src/csr/mcause.rs#L79-L95

Since it is considering only 4 bits of `reason`, any value with `mcause.EXCCODE` higher than `15` will be misinterpreted. Here is the panic I am seeing on my run:
```
unexpected user-mode interrupt
        Kernel version unknown

---| RISC-V Machine State |---
Last cause (mcause): User software interrupt (interrupt=1, exception code=0x00000010)
Last value (mtval):  0x00000000

System register dump:
 mepc:    0x00803568    mstatus:     0x00001880
 mcycle:  0x00C11870    minstret:    0x000F0AC2
 mtvec:   0x00800100
 mstatus: 0x00001880
  uie:    false  upie:   false
  sie:    false  spie:   false
  mie:    false  mpie:   true
  spp:    false
 mie:   0x00010888   mip:   0x00010800
  usoft:  false               false
  ssoft:  false               false
  msoft:  true                false
  utimer: false               false
  stimer: false               false
  mtimer: true                false
  uext:   false               false
  sext:   false               false
  mext:   true                true
```

Specifically the line 
> Last cause (mcause): User software interrupt (interrupt=1, exception code=0x00000010)

As we can see, the actual "reason" is interrupt number `0x10` (which in my case is a LocalInterrupt0), but it is misinterpreted as User software interrupt (interrupt number `0`).

Since this is happening on the `arch::riscv` level, there is no way to fix it on the board/chip level. 
I think the bitness limit of `reason` should be removed. The unknown values will still be mapped to `Interrupt::Unknown` without breaking anything, but will also give the downstream code to handle it appropriately (but ideally, of course, it should be "extendable" somehow for the specific implementation).


Discussion / hints:

Fix patch:
diff --git a/arch/riscv/src/csr/mcause.rs b/arch/riscv/src/csr/mcause.rs
index b3dad3676a..d9a731ef4e 100644
--- a/arch/riscv/src/csr/mcause.rs
+++ b/arch/riscv/src/csr/mcause.rs
@@ -9,13 +9,6 @@ register_bitfields![usize,
         is_interrupt OFFSET(crate::XLEN - 1) NUMBITS(1) [],
         reason OFFSET(0) NUMBITS(crate::XLEN - 1) []
     ],
-    // Per the spec, implementations are allowed to use the higher bits of the
-    // interrupt/exception reason for their own purposes.  For regular parsing,
-    // we only concern ourselves with the "standard" values.
-    reason [
-        reserved OFFSET(4) NUMBITS(crate::XLEN - 5) [],
-        std OFFSET(0) NUMBITS(4) []
-    ]
 ];
 
 /// Trap Cause
@@ -53,7 +46,7 @@ pub enum Interrupt {
     UserExternal,
     SupervisorExternal,
     MachineExternal,
-    Unknown,
+    Unknown(usize),
 }
 
 /// Exception
@@ -78,8 +71,8 @@ pub enum Exception {
 
 impl Interrupt {
     fn from_reason(val: usize) -> Self {
-        let reason = LocalRegisterCopy::<usize, reason::Register>::new(val);
-        match reason.read(reason::std) {
+        let mcause = LocalRegisterCopy::<usize, mcause::Register>::new(val);
+        match mcause.read(mcause::reason) {
             0 => Interrupt::UserSoft,
             1 => Interrupt::SupervisorSoft,
             3 => Interrupt::MachineSoft,
@@ -89,15 +82,15 @@ impl Interrupt {
             8 => Interrupt::UserExternal,
             9 => Interrupt::SupervisorExternal,
             11 => Interrupt::MachineExternal,
-            _ => Interrupt::Unknown,
+            val => Interrupt::Unknown(val),
         }
     }
 }
 
 impl Exception {
     fn from_reason(val: usize) -> Self {
-        let reason = LocalRegisterCopy::<usize, reason::Register>::new(val);
-        match reason.read(reason::std) {
+        let mcause = LocalRegisterCopy::<usize, mcause::Register>::new(val);
+        match mcause.read(mcause::reason) {
             0 => Exception::InstructionMisaligned,
             1 => Exception::InstructionFault,
             2 => Exception::IllegalInstruction,
diff --git a/arch/rv32i/src/lib.rs b/arch/rv32i/src/lib.rs
index 3407695211..182b584b5f 100644
--- a/arch/rv32i/src/lib.rs
+++ b/arch/rv32i/src/lib.rs
@@ -454,7 +454,7 @@ pub unsafe fn print_mcause(mcval: csr::mcause::Trap, writer: &mut dyn Write) {
             csr::mcause::Interrupt::MachineExternal => {
                 let _ = writer.write_fmt(format_args!("Machine external interrupt"));
             }
-            csr::mcause::Interrupt::Unknown => {
+            csr::mcause::Interrupt::Unknown(_) => {
                 let _ = writer.write_fmt(format_args!("Reserved/Unknown"));
             }
         },
diff --git a/chips/e310x/src/chip.rs b/chips/e310x/src/chip.rs
index 6333ab36c1..7d97fafe6c 100644
--- a/chips/e310x/src/chip.rs
+++ b/chips/e310x/src/chip.rs
@@ -245,7 +245,7 @@ unsafe fn handle_interrupt(intr: mcause::Interrupt) {
             }
         }
 
-        mcause::Interrupt::Unknown => {
+        mcause::Interrupt::Unknown(_) => {
             panic!("interrupt of unknown cause");
         }
     }
diff --git a/chips/earlgrey/src/chip.rs b/chips/earlgrey/src/chip.rs
index f23d7a258b..8dc3a96f3a 100644
--- a/chips/earlgrey/src/chip.rs
+++ b/chips/earlgrey/src/chip.rs
@@ -390,7 +390,7 @@ unsafe fn handle_interrupt(intr: mcause::Interrupt) {
             }
         }
 
-        mcause::Interrupt::Unknown => {
+        mcause::Interrupt::Unknown(_) => {
             panic!("interrupt of unknown cause");
         }
     }
diff --git a/chips/litex_vexriscv/src/chip.rs b/chips/litex_vexriscv/src/chip.rs
index 8f6819131c..5e534d4536 100644
--- a/chips/litex_vexriscv/src/chip.rs
+++ b/chips/litex_vexriscv/src/chip.rs
@@ -166,7 +166,7 @@ unsafe fn handle_interrupt(intr: mcause::Interrupt) {
             }
         }
 
-        mcause::Interrupt::Unknown => {
+        mcause::Interrupt::Unknown(_) => {
             debug!("interrupt of unknown cause");
         }
     }
diff --git a/chips/qemu_rv32_virt_chip/src/chip.rs b/chips/qemu_rv32_virt_chip/src/chip.rs
index 3830586320..91484a7dde 100644
--- a/chips/qemu_rv32_virt_chip/src/chip.rs
+++ b/chips/qemu_rv32_virt_chip/src/chip.rs
@@ -245,7 +245,7 @@ unsafe fn handle_interrupt(intr: mcause::Interrupt) {
             }
         }
 
-        mcause::Interrupt::Unknown => {
+        mcause::Interrupt::Unknown(_) => {
             panic!("interrupt of unknown cause");
         }
     }
diff --git a/chips/veer_el2/src/chip.rs b/chips/veer_el2/src/chip.rs
index 75b9d119d9..a51f85a510 100644
--- a/chips/veer_el2/src/chip.rs
+++ b/chips/veer_el2/src/chip.rs
@@ -215,7 +215,7 @@ unsafe fn handle_interrupt(intr: mcause::Interrupt) {
             }
         }
 
-        mcause::Interrupt::Unknown => {
+        mcause::Interrupt::Unknown(_) => {
             panic!("interrupt of unknown cause");
         }
     }


Test patch: