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         4542    346    0 2268
 microbit    examples/serial       2993    391    8 2264
 wioterminal examples/pininterrupt 8275   1741  148 7496

main_test.go

@@ -468,15 +468,6 @@ func runPlatTests(options compileopts.Options, tests []string, t *testing.T) {
 				continue
 			}
 		}
-		if options.Target == "riscv-qemu" {
-			switch name {
-			case "finalizerinvariants.go":
-				// The finalizer code stops or fails on multicore RISC-V.
-				// See https://github.com/tinygo-org/tinygo/issues/5679
-				continue
-			}
-		}
-
 		name := name // redefine to avoid race condition
 		t.Run(name, func(t *testing.T) {
 			t.Parallel()

src/internal/task/task_stack.go

@@ -76,9 +76,3 @@ func start(fn uintptr, args unsafe.Pointer, stackSize uintptr) {
 	t.state.initialize(fn, args, stackSize)
 	scheduleTask(t)
 }
-
-// OnSystemStack returns whether the caller is running on the system stack.
-func OnSystemStack() bool {
-	// If there is not an active goroutine, then this must be running on the system stack.
-	return Current() == nil
-}

src/internal/task/task_stack_onsystemstack.go

@@ -0,0 +1,9 @@
+//go:build (scheduler.tasks || scheduler.cores) && !tinygo.riscv
+
+package task
+
+// OnSystemStack returns whether the caller is running on the system stack.
+func OnSystemStack() bool {
+	// If there is no active goroutine, this must be the system stack.
+	return Current() == nil
+}

src/internal/task/task_stack_tinygoriscv.S

@@ -21,13 +21,21 @@ tinygo_startTask:
     // After return, exit this goroutine. This is a tail call.
     tail  tinygo_task_exit
 
-.section .text.tinygo_swapTask
+.section .text.tinygo_swapTaskRISC
+.global  tinygo_swapTaskRISC
+.type    tinygo_swapTaskRISC, %function
+tinygo_swapTaskRISC:
 .global  tinygo_swapTask
 .type    tinygo_swapTask, %function
-tinygo_swapTask:
+.set     tinygo_swapTask, tinygo_swapTaskRISC
+
     // This function gets the following parameters:
     //   a0 = newStack uintptr
     //   a1 = oldStack *uintptr
+    //   a2 = stack pointer to clear after switching, or nil
+
+    // Keep the prior hart state in t0. It is not part of the task context.
+    csrrci t0, mstatus, 8
 
     // Push all callee-saved registers.
     addi sp, sp, -52
@@ -51,6 +59,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 +80,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,22 +56,29 @@ 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)
 }
 
+//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 {
 	return *runtime_systemStackPtr()
 }
 
+// OnSystemStack returns whether the caller is running on the system stack.
+func OnSystemStack() bool {
+	return SystemStack() == 0
+}
+
 //export tinygo_task_current
 func tinygo_task_current() unsafe.Pointer {
 	return unsafe.Pointer(Current())

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,24 @@ func handleInterrupt() {
 		hartID := currentCPU()
 		switch code {
 		case riscv.MachineSoftwareInterrupt:
+			// Clear the interrupt before checking state so a new request stays pending.
+			// See RISC-V Unprivileged ISA, section 2.7.
+			aclintMSWI.MSIP[hartID].Set(0)
+			riscv.Asm("fence")
 			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)
+			// schedulerLock prevents this flag from being set before the
+			// checkpoint is saved.
+			if schedulerWakePending[hartID].Swap(0) != 0 && checkpoint.Saved() {
 				riscv.MCAUSE.Set(0)
 				checkpoint.Jump()
 			}
@@ -132,6 +140,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,17 +156,11 @@ 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")
-	}
-	aclintMSWI.MSIP[hartID].Set(0)
+	gcWaitForSignal(hartID)
 
 	// Scan the stack(s) of this core.
 	scanCurrentStack()
@@ -163,10 +173,7 @@ 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")
-	}
-	aclintMSWI.MSIP[hartID].Set(0)
+	gcWaitForSignal(hartID)
 
 	// Restore MIE bits.
 	riscv.MIE.Set(savedMIE)
@@ -175,6 +182,24 @@ func gcInterruptHandler(hartID uint32) {
 	gcScanState.Add(1)
 }
 
+func gcWaitForSignal(hartID uint32) {
+	for gcSignalWait[hartID].Load() == 0 {
+		// Clear unrelated wakeups before checking state to avoid losing a signal.
+		// See RISC-V Unprivileged ISA, section 2.7.
+		aclintMSWI.MSIP[hartID].Set(0)
+		riscv.Asm("fence")
+		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)
+	riscv.Asm("fence")
+}
+
 //go:extern _stack_top
 var stack0TopSymbol [0]byte
 
@@ -391,7 +416,7 @@ func startSecondaryCores() {
 	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 +428,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 +477,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 +524,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 +558,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()
 }