Merge pull request #43 from sysprog21/trap-handling

Fix incorrect nested trap handling in RISC-V HAL

Author: jserv

Documentation/hal-riscv-context-switch.md

@@ -48,8 +48,9 @@ typedef uint32_t jmp_buf[17];
 The `hal_context_save` function captures complete task state including both execution context and processor state.
 The function saves all callee-saved registers as required by the RISC-V ABI,
 plus essential pointers (gp, tp, sp, ra).
-For processor state, it performs sophisticated interrupt state reconstruction and ensures that tasks resume with correct interrupt state,
-maintaining system responsiveness and preventing interrupt state corruption.
+For processor state, it saves `mstatus` as-is, preserving the exact processor state at the time of the context switch.
+The RISC-V hardware automatically manages the `mstatus.MIE` and `mstatus.MPIE` stack during trap entry and `MRET`,
+ensuring correct interrupt state restoration per the RISC-V Privileged Specification.
 
 ### 2. Select Next Task
 The scheduler, invoked via `dispatcher()` during machine timer interrupts,
@@ -69,19 +70,30 @@ This ordering ensures that interrupt state and privilege mode are correctly esta
 
 ## Processor State Management
 
-### Interrupt State Reconstruction
-The HAL context switching routines include sophisticated interrupt state management that handles the complexities of RISC-V interrupt processing:
+### Hardware-Managed Interrupt State
+The HAL context switching routines follow the RISC-V Privileged Specification for interrupt state management,
+allowing hardware to automatically manage the interrupt enable stack:
 
-During Timer Interrupts:
-- `mstatus.MIE` is automatically cleared by hardware when entering the trap
-- `mstatus.MPIE` preserves the previous interrupt enable state
-- HAL functions reconstruct the original interrupt state from `MPIE`
-- This ensures consistent interrupt behavior across context switches
+During Trap Entry (Hardware Automatic per RISC-V Spec §3.1.6.1):
+- `mstatus.MPIE ← mstatus.MIE` (preserve interrupt enable state)
+- `mstatus.MIE ← 0` (disable interrupts during trap handling)
+- `mstatus.MPP ← current_privilege` (preserve privilege mode)
+
+During MRET (Hardware Automatic):
+- `mstatus.MIE ← mstatus.MPIE` (restore interrupt enable state)
+- `mstatus.MPIE ← 1` (reset to default enabled)
+- `privilege ← mstatus.MPP` (return to saved privilege)
+
+HAL Context Switch Behavior:
+- `hal_context_save` saves `mstatus` exactly as observed (no manual bit manipulation)
+- `hal_context_restore` restores `mstatus` exactly as saved
+- Hardware manages the `MIE`/`MPIE` stack automatically during nested traps
+- This ensures spec-compliant behavior and correct interrupt state across all scenarios
 
 State Preservation:
-- Each task maintains its own interrupt enable state
+- Each task maintains its own complete `mstatus` value
 - Context switches preserve privilege mode (Machine mode for kernel tasks)
-- Interrupt state is reconstructed accurately for reliable task resumption
+- Nested interrupts are handled correctly by hardware's automatic state stacking
 
 ### Task Initialization
 New tasks are initialized with proper processor state:

arch/riscv/hal.c

@@ -147,10 +147,10 @@ static inline uint64_t mtimecmp_r(void)
 /* Safely write to the 64-bit 'mtimecmp' register on a 32-bit architecture.
  * A direct write of 'lo' then 'hi' could trigger a spurious interrupt if the
  * timer happens to cross the new 'lo' value before 'hi' is updated.
- * To prevent this, we first set the low word to an impassable value (all 1s),
- * then set the high word, and finally set the correct low word. This ensures
- * the full 64-bit compare value becomes active atomically from the timer's
- * perspective.
+ * To prevent this, the implementation first sets the low word to an impassable
+ * value (all 1s), then sets the high word, and finally sets the correct low
+ * word. This ensures the full 64-bit compare value becomes active atomically
+ * from the timer's perspective.
  */
 static inline void mtimecmp_w(uint64_t val)
 {
@@ -326,7 +326,7 @@ void hal_interrupt_tick(void)
 
     /* The task's entry point is still in RA, so this is its very first run */
     if ((uint32_t) task->entry == task->context[CONTEXT_RA])
-        _ei(); /* Enable global interrupts now that we are in a task */
+        _ei(); /* Enable global interrupts now that execution is in a task */
 }
 
 /* Context Switching */
@@ -438,16 +438,16 @@ int32_t hal_context_save(jmp_buf env)
         "sw  tp,  13*4(%0)\n"
         "sw  sp,  14*4(%0)\n"
         "sw  ra,  15*4(%0)\n"
-        /* Save mstatus with interrupt state reconstruction. During timer
-         * interrupts, mstatus.MIE is cleared, so we reconstruct the pre-trap
-         * state from MPIE for consistent interrupt context preservation.
+        /* Save mstatus as-is. According to RISC-V spec section 3.1.6.1,
+         * mstatus.mie and mstatus.mpie are automatically managed by hardware:
+         * - On trap entry: mpie <- mie, mie <- 0
+         * - On MRET: mie <- mpie, mpie <- 1
+         * The implementation saves the current state without manual bit
+         * manipulation, allowing hardware to correctly manage the interrupt
+         * enable stack.
          */
-        "csrr t0, mstatus\n" /* Read current mstatus (MIE=0 in trap) */
-        "andi t1, t0, ~8\n"  /* Clear MIE bit first */
-        "srli t2, t0, 4\n"   /* Get MPIE bit to position 3 */
-        "andi t2, t2, 8\n"   /* Isolate bit 3 */
-        "or   t1, t1, t2\n"  /* Combine cleared MIE with reconstructed bit */
-        "sw   t1, 16*4(%0)\n"
+        "csrr t0, mstatus\n"
+        "sw   t0, 16*4(%0)\n"
         /* By convention, the initial call returns 0 */
         "li a0, 0\n"
         :