runtime: fix multicore RISC-V GC synchronization

Separate scheduler wakeups, GC pause requests, and GC phase signals so
software interrupts cannot consume requests for another subsystem.

Make RISC-V task stack switches atomic with respect to interrupts and
derive GC stack selection from saved system stack state. This prevents
the collector from scanning a nil or incorrect stack during a switch.

Include mtval in exception reports to identify invalid access addresses.

Fixes 5679

Author: jakebailey

builder/testdata/binary-size.txt

@@ -1,4 +1,4 @@
 target      package               code rodata data  bss
-hifive1b    examples/echo         4526    346    0 2268
+hifive1b    examples/echo         4558    346    0 2268
 microbit    examples/serial       2993    391    8 2264
 wioterminal examples/pininterrupt 8309   1739  148 7496

src/internal/task/task_stack_tinygoriscv.S

@@ -21,13 +21,17 @@ tinygo_startTask:
     // After return, exit this goroutine. This is a tail call.
     tail  tinygo_task_exit
 
-.section .text.tinygo_swapTask
-.global  tinygo_swapTask
-.type    tinygo_swapTask, %function
-tinygo_swapTask:
+.section .text.tinygo_swapTaskRISC
+.global  tinygo_swapTaskRISC
+.type    tinygo_swapTaskRISC, %function
+tinygo_swapTaskRISC:
     // This function gets the following parameters:
     //   a0 = newStack uintptr
     //   a1 = oldStack *uintptr
+    //   a2 = stack pointer to clear after switching, or nil
+
+    // Prevent an interrupt from observing a half-completed stack switch.
+    csrrci t0, mstatus, 8
 
     // Push all callee-saved registers.
     addi sp, sp, -52
@@ -51,6 +55,11 @@ tinygo_swapTask:
     // Switch to the new stack pointer.
     mv sp,  a0
 
+    // Clear the saved system stack only after switching back to it.
+    beqz a2, 1f
+    sw zero, 0(a2)
+1:
+
     // Pop all saved registers from this new stack.
     lw ra,  48(sp)
     lw s11, 44(sp)
@@ -67,5 +76,11 @@ tinygo_swapTask:
     lw s0,    (sp)
     addi sp, sp, 52
 
+    // Restore the interrupt-enable state.
+    andi t0, t0, 8
+    beqz t0, 2f
+    csrsi mstatus, 8
+2:
+
     // Return into the task.
     ret

src/internal/task/task_stack_tinygoriscv.go

@@ -56,16 +56,18 @@ func (s *state) archInit(r *calleeSavedRegs, fn uintptr, args unsafe.Pointer) {
 }
 
 func (s *state) resume() {
-	swapTask(s.sp, runtime_systemStackPtr())
+	swapTaskRISC(s.sp, runtime_systemStackPtr(), nil)
 }
 
 func (s *state) pause() {
 	systemStackPtr := runtime_systemStackPtr()
 	newStack := *systemStackPtr
-	*systemStackPtr = 0
-	swapTask(newStack, &s.sp)
+	swapTaskRISC(newStack, &s.sp, systemStackPtr)
 }
 
+//go:export tinygo_swapTaskRISC
+func swapTaskRISC(oldStack uintptr, newStack, clearStack *uintptr)
+
 // SystemStack returns the system stack pointer when called from a task stack.
 // When called from the system stack, it returns 0.
 func SystemStack() uintptr {

src/runtime/gc_stack_cores.go

@@ -20,7 +20,7 @@ func gcMarkReachable() {
 	if !secondaryCoresStarted {
 		// Scan the stack(s) of the current core.
 		scanCurrentStack()
-		if !task.OnSystemStack() {
+		if !gcOnSystemStack() {
 			// Mark system stack.
 			markRoots(task.SystemStack(), stackTop)
 		}
@@ -51,7 +51,7 @@ func gcMarkReachable() {
 
 	// Scan the stack(s) of the current core.
 	scanCurrentStack()
-	if !task.OnSystemStack() {
+	if !gcOnSystemStack() {
 		// Mark system stack.
 		markRoots(task.SystemStack(), coreStackTop(core))
 	}
@@ -86,7 +86,7 @@ func scanstack(sp uintptr) {
 	// Mark the current stack.
 	// This function is called by scanCurrentStack, after pushing all registers
 	// onto the stack.
-	if task.OnSystemStack() {
+	if gcOnSystemStack() {
 		// This is the system stack.
 		// Scan all words on the stack.
 		markRoots(sp, coreStackTop(currentCPU()))

src/runtime/runtime_rp2.go

@@ -15,6 +15,10 @@ import (
 const numCPU = 2
 const numSpinlocks = 32
 
+func gcOnSystemStack() bool {
+	return task.OnSystemStack()
+}
+
 // machineTicks is provided by package machine.
 func machineTicks() uint64
 

src/runtime/runtime_tinygoriscv_qemu.go

@@ -56,7 +56,7 @@ func main() {
 		// should exit immediately.
 		// Signal hart 0 to exit.
 		exitCodePlusOne.Store(0 + 1) // exit code 0
-		aclintMSWI.MSIP[0].Set(1)
+		signalHart(0)
 
 		// Unlock the scheduler to be sure. Shouldn't be needed.
 		schedulerLock.Unlock()
@@ -96,16 +96,21 @@ func handleInterrupt() {
 		hartID := currentCPU()
 		switch code {
 		case riscv.MachineSoftwareInterrupt:
+			// Acknowledge the interrupt before checking its consumers. A new
+			// request that arrives after this point must remain pending.
+			aclintMSWI.MSIP[hartID].Set(0)
 			if exitCodePlusOne.Load() != 0 {
 				exitNow(exitCodePlusOne.Load() - 1)
 			}
-			if gcScanState.Load() != 0 {
+			if gcPauseRequest[hartID].Swap(0) != 0 {
 				// The GC needs to run.
 				gcInterruptHandler(hartID)
 			}
+			if exitCodePlusOne.Load() != 0 {
+				exitNow(exitCodePlusOne.Load() - 1)
+			}
 			checkpoint := &schedulerWaitCheckpoints[hartID]
-			if checkpoint.Saved() {
-				aclintMSWI.MSIP[hartID].Set(0)
+			if schedulerWakePending[hartID].Swap(0) != 0 && checkpoint.Saved() {
 				riscv.MCAUSE.Set(0)
 				checkpoint.Jump()
 			}
@@ -132,6 +137,14 @@ func handleInterrupt() {
 	riscv.MCAUSE.Set(0)
 }
 
+var (
+	// State used to request a GC pause on each hart.
+	gcPauseRequest [numCPU]atomic.Uint32
+
+	// State used to signal the next GC phase to each paused hart.
+	gcSignalWait [numCPU]atomic.Uint32
+)
+
 // The GC interrupted this core for the stop-the-world phase.
 // This function handles that, and only returns after the stop-the-world phase
 // ended.
@@ -140,21 +153,30 @@ func gcInterruptHandler(hartID uint32) {
 	savedMIE := riscv.MIE.Get()
 	riscv.MIE.Set(riscv.MIE_MSIE)
 
-	// Disable this interrupt (to be enabled again soon).
-	aclintMSWI.MSIP[hartID].Set(0)
-
 	// Let the GC know we're ready.
 	gcScanState.Add(1)
 
 	// Wait until we get a signal to start scanning.
-	for riscv.MIP.Get()&riscv.MIP_MSIP == 0 {
-		riscv.Asm("wfi")
+	for gcSignalWait[hartID].Load() == 0 {
+		if hartID == 0 && exitCodePlusOne.Load() != 0 {
+			exitNow(exitCodePlusOne.Load() - 1)
+		}
+		// Clear unrelated scheduler wakeups before waiting again. Check pending
+		// requests after the clear so a concurrent signal is not lost.
+		aclintMSWI.MSIP[hartID].Set(0)
+		if hartID == 0 && exitCodePlusOne.Load() != 0 {
+			exitNow(exitCodePlusOne.Load() - 1)
+		}
+		if gcSignalWait[hartID].Load() == 0 {
+			riscv.Asm("wfi")
+		}
 	}
+	gcSignalWait[hartID].Store(0)
 	aclintMSWI.MSIP[hartID].Set(0)
 
 	// Scan the stack(s) of this core.
 	scanCurrentStack()
-	if !task.OnSystemStack() {
+	if !gcOnSystemStack() {
 		// Mark system stack.
 		markRoots(task.SystemStack(), coreStackTop(hartID))
 	}
@@ -163,9 +185,19 @@ func gcInterruptHandler(hartID uint32) {
 	gcScanState.Store(1)
 
 	// Wait until we get a signal that the stop-the-world phase has ended.
-	for riscv.MIP.Get()&riscv.MIP_MSIP == 0 {
-		riscv.Asm("wfi")
+	for gcSignalWait[hartID].Load() == 0 {
+		if hartID == 0 && exitCodePlusOne.Load() != 0 {
+			exitNow(exitCodePlusOne.Load() - 1)
+		}
+		aclintMSWI.MSIP[hartID].Set(0)
+		if hartID == 0 && exitCodePlusOne.Load() != 0 {
+			exitNow(exitCodePlusOne.Load() - 1)
+		}
+		if gcSignalWait[hartID].Load() == 0 {
+			riscv.Asm("wfi")
+		}
 	}
+	gcSignalWait[hartID].Store(0)
 	aclintMSWI.MSIP[hartID].Set(0)
 
 	// Restore MIE bits.
@@ -386,12 +418,16 @@ func currentCPU() uint32 {
 	return uint32(riscv.MHARTID.Get())
 }
 
+func gcOnSystemStack() bool {
+	return task.SystemStack() == 0
+}
+
 func startSecondaryCores() {
 	// Start all the other cores besides hart 0.
 	for hart := 1; hart < numCPU; hart++ {
 		// Signal the given hart it is ready to start using a software
 		// interrupt.
-		aclintMSWI.MSIP[hart].Set(1)
+		signalHart(uint32(hart))
 	}
 }
 
@@ -403,6 +439,9 @@ var sleepingHarts uint8
 // Checkpoints for cores waiting for runnable tasks.
 var schedulerWaitCheckpoints [numCPU]interrupt.Checkpoint
 
+// State used to distinguish scheduler wakeups from other software interrupts.
+var schedulerWakePending [numCPU]atomic.Uint32
+
 // Put the scheduler to sleep, since there are no tasks to run.
 // This will unlock the scheduler lock, and must be called with the scheduler
 // lock held.
@@ -449,21 +488,32 @@ func schedulerWake() {
 
 	if hart < 8 {
 		// There is a sleeping hart. Wake it.
-		sleepingHarts &^= 1 << hart  // clear the bit
-		aclintMSWI.MSIP[hart].Set(1) // send software interrupt
+		// Clear the sleeping bit before sending the wakeup.
+		sleepingHarts &^= 1 << hart
+		schedulerWakePending[hart].Store(1)
+		signalHart(uint32(hart))
 	}
 }
 
 // Pause the given core by sending it an interrupt.
 func gcPauseCore(core uint32) {
-	aclintMSWI.MSIP[core].Set(1) // send software interrupt
+	gcPauseRequest[core].Store(1)
+	signalHart(core)
 }
 
 // Signal the given core that it can resume one step.
 // This is called twice after gcPauseCore: the first time to scan the stack of
 // the core, and the second time to end the stop-the-world phase.
 func gcSignalCore(core uint32) {
-	aclintMSWI.MSIP[core].Set(1) // send software interrupt
+	gcSignalWait[core].Store(1)
+	signalHart(core)
+}
+
+func signalHart(hart uint32) {
+	// Order state writes before the interrupt notification.
+	// See RISC-V Unprivileged ISA, section 2.7.
+	riscv.Asm("fence")
+	aclintMSWI.MSIP[hart].Set(1)
 }
 
 func abort() {
@@ -485,7 +535,7 @@ func exit(code int) {
 	if currentCPU() != 0 {
 		// Signal hart 0 to exit.
 		exitCodePlusOne.Store(uint32(code) + 1)
-		aclintMSWI.MSIP[0].Set(1)
+		signalHart(0)
 
 		// Wait for the interrupt to happen. This should happen immediately.
 		for {
@@ -519,6 +569,6 @@ func exitNow(code uint32) {
 func handleException(code uint) {
 	// For a list of exception codes, see:
 	// https://content.riscv.org/wp-content/uploads/2019/08/riscv-privileged-20190608-1.pdf#page=49
-	print("fatal error: exception with mcause=", code, " pc=", riscv.MEPC.Get(), " hart=", uint(riscv.MHARTID.Get()), "\r\n")
+	print("fatal error: exception with mcause=", code, " pc=", riscv.MEPC.Get(), " mtval=", riscv.MTVAL.Get(), " hart=", uint(riscv.MHARTID.Get()), "\r\n")
 	abort()
 }