arch: riscv: streamline fatal handling code

arch/riscv/Kconfig

@@ -301,14 +301,6 @@ config RISCV_HART_MASK
 	  i.e. 128, 129, ..(0x80, 8x81, ..), this can be configured to 63 (0x7f)
 	  such that we can extract the bits that start from 0.
 
-config EXTRA_EXCEPTION_INFO
-	bool "Collect extra exception info"
-	depends on EXCEPTION_DEBUG
-	help
-	  This option enables the collection of extra information, such as
-	  register state, when a fault occurs. This information can be useful
-	  to collect for post-mortem analysis and debug of issues.
-
 config RISCV_PMP
 	bool "RISC-V PMP Support"
 	select THREAD_STACK_INFO

arch/riscv/core/fatal.c

@@ -80,12 +80,7 @@ const char *z_riscv_mcause_str(unsigned long cause)
 FUNC_NORETURN void z_riscv_fatal_error(unsigned int reason,
 				       const struct arch_esf *esf)
 {
-	z_riscv_fatal_error_csf(reason, esf, NULL);
-}
-
-FUNC_NORETURN void z_riscv_fatal_error_csf(unsigned int reason, const struct arch_esf *esf,
-					   const _callee_saved_t *csf)
-{
+	__maybe_unused _callee_saved_t *csf = NULL;
 	unsigned long mcause;
 
 	__asm__ volatile("csrr %0, mcause" : "=r" (mcause));
@@ -122,6 +117,8 @@ FUNC_NORETURN void z_riscv_fatal_error_csf(unsigned int reason, const struct arc
 		LOG_ERR("   mepc: " PR_REG, esf->mepc);
 		LOG_ERR("mstatus: " PR_REG, esf->mstatus);
 		LOG_ERR("");
+
+		csf = esf->csf;
 	}
 
 	if (csf != NULL) {
@@ -248,6 +245,12 @@ void z_impl_user_fault(unsigned int reason)
 {
 	struct arch_esf *oops_esf = _current->syscall_frame;
 
+#ifdef CONFIG_EXCEPTION_DEBUG
+	if (oops_esf != NULL) {
+		oops_esf->csf = NULL;
+	}
+#endif /* CONFIG_EXCEPTION_DEBUG */
+
 	if (((_current->base.user_options & K_USER) != 0) &&
 		reason != K_ERR_STACK_CHK_FAIL) {
 		reason = K_ERR_KERNEL_OOPS;

arch/riscv/core/isr.S

@@ -56,6 +56,20 @@
 	RV_I(	sr s9, ___callee_saved_t_s9_OFFSET(sp)		);\
 	RV_I(	sr s10, ___callee_saved_t_s10_OFFSET(sp)	);\
 	RV_I(	sr s11, ___callee_saved_t_s11_OFFSET(sp)	)
+
+/**
+ * Convenience macro to store the caller-saved registers (CSF) on stack,
+ * and link it to the `struct arch_esf`.
+ *  1. Restore the `s0` we saved early in ISR entry so it shows up properly in the CSF.
+ *  2. Allocate space for CSF on current thread stack
+ *  3. Save CSF to the allocated stack
+ *  4. Link the saved CSF to the `struct arch_esf`
+ */
+#define STORE_CSF_IN_ESF(_esf)                                                                     \
+	lr s0, __struct_arch_esf_s0_OFFSET(sp)		;                                          \
+	addi sp, sp, -__callee_saved_t_SIZEOF		;                                          \
+	STORE_CALLEE_SAVED()				;                                          \
+	sr sp __struct_arch_esf_csf_OFFSET(_esf)
 #endif /* CONFIG_EXCEPTION_DEBUG */
 
 	.macro get_current_cpu dst
@@ -78,12 +92,7 @@ GTEXT(__soc_save_context)
 GTEXT(__soc_restore_context)
 #endif /* CONFIG_RISCV_SOC_CONTEXT_SAVE */
 
-#ifdef CONFIG_EXCEPTION_DEBUG
-GTEXT(z_riscv_fatal_error_csf)
-#else
 GTEXT(z_riscv_fatal_error)
-#endif /* CONFIG_EXCEPTION_DEBUG */
-
 GTEXT(z_get_next_switch_handle)
 GTEXT(z_riscv_switch)
 GTEXT(z_riscv_thread_start)
@@ -388,8 +397,18 @@ no_fp:	/* increment _current->arch.exception_depth */
 	 * no_reschedule to restore stack.
 	 */
 	mv a0, sp
+#ifdef CONFIG_EXCEPTION_DEBUG
+	STORE_CSF_IN_ESF(a0)		;
+
+	call z_riscv_fault
+	/* Restore SP if z_riscv_fault returns before jumping to no_reschedule */
+	addi sp, sp, __callee_saved_t_SIZEOF
+
+	j no_reschedule
+#else
 	la ra, no_reschedule
 	tail z_riscv_fault
+#endif /* CONFIG_EXCEPTION_DEBUG */
 
 is_kernel_syscall:
 	/*
@@ -459,28 +478,9 @@ do_fault:
 1:	mv a1, sp
 
 #ifdef CONFIG_EXCEPTION_DEBUG
-	/*
-	 * Restore the s0 we saved early in ISR entry
-	 * so it shows up properly in the CSF.
-	 */
-	lr s0, __struct_arch_esf_s0_OFFSET(sp)
-
-	/* Allocate space for caller-saved registers on current thread stack */
-	addi sp, sp, -__callee_saved_t_SIZEOF
-
-	/* Save callee-saved registers to be passed as 3rd arg */
-	STORE_CALLEE_SAVED()		;
-	mv a2, sp
-
-#ifdef CONFIG_EXTRA_EXCEPTION_INFO
-	/* Store csf's addr into esf (a1 still holds the pointer to the esf at this point) */
-	sr a2 __struct_arch_esf_csf_OFFSET(a1)
-#endif /* CONFIG_EXTRA_EXCEPTION_INFO */
-
-	tail z_riscv_fatal_error_csf
-#else
-	tail z_riscv_fatal_error
+	STORE_CSF_IN_ESF(a1)		;
 #endif /* CONFIG_EXCEPTION_DEBUG */
+	tail z_riscv_fatal_error
 
 #if defined(CONFIG_IRQ_OFFLOAD)
 do_irq_offload:

arch/riscv/core/offsets/offsets.c

@@ -122,10 +122,6 @@ GEN_OFFSET_STRUCT(arch_esf, s0);
 GEN_OFFSET_STRUCT(arch_esf, sp);
 #endif
 
-#ifdef CONFIG_EXTRA_EXCEPTION_INFO
-GEN_OFFSET_STRUCT(arch_esf, csf);
-#endif /* CONFIG_EXTRA_EXCEPTION_INFO */
-
 #if defined(CONFIG_RISCV_SOC_CONTEXT_SAVE)
 GEN_OFFSET_STRUCT(arch_esf, soc_context);
 #endif
@@ -136,6 +132,7 @@ GEN_SOC_OFFSET_SYMS();
 GEN_ABSOLUTE_SYM(__struct_arch_esf_SIZEOF, sizeof(struct arch_esf));
 
 #ifdef CONFIG_EXCEPTION_DEBUG
+GEN_OFFSET_STRUCT(arch_esf, csf);
 GEN_ABSOLUTE_SYM(__callee_saved_t_SIZEOF, ROUND_UP(sizeof(_callee_saved_t), ARCH_STACK_PTR_ALIGN));
 #endif /* CONFIG_EXCEPTION_DEBUG */
 

arch/riscv/include/kernel_arch_func.h

@@ -74,13 +74,9 @@ arch_switch(void *switch_to, void **switched_from)
 #endif
 }
 
-/* Thin wrapper around z_riscv_fatal_error_csf */
 FUNC_NORETURN void z_riscv_fatal_error(unsigned int reason,
 				       const struct arch_esf *esf);
 
-FUNC_NORETURN void z_riscv_fatal_error_csf(unsigned int reason, const struct arch_esf *esf,
-					   const _callee_saved_t *csf);
-
 static inline bool arch_is_in_isr(void)
 {
 #ifdef CONFIG_SMP

doc/releases/migration-guide-4.2.rst

@@ -760,3 +760,9 @@ Architectures
   of vector table in RAM.
 * Renamed :kconfig:option:`CONFIG_DEBUG_INFO` to :kconfig:option:`CONFIG_X86_DEBUG_INFO` to
   better reflect its purpose. This option is now only available for x86 architecture.
+
+* RISCV
+
+  * :kconfig:option:`CONFIG_EXTRA_EXCEPTION_INFO` has been removed, the ``*csf`` pointer will be
+    available in the ``struct arch_esf`` when :kconfig:option:`CONFIG_EXCEPTION_DEBUG` is
+    enabled.

include/zephyr/arch/riscv/exception.h

@@ -45,11 +45,11 @@ struct soc_esf {
 };
 #endif
 
-#ifdef CONFIG_EXTRA_EXCEPTION_INFO
+#ifdef CONFIG_EXCEPTION_DEBUG
 /* Forward declaration */
 struct _callee_saved;
 typedef struct _callee_saved _callee_saved_t;
-#endif /* CONFIG_EXTRA_EXCEPTION_INFO */
+#endif /* CONFIG_EXCEPTION_DEBUG */
 
 #if defined(CONFIG_RISCV_SOC_HAS_ISR_STACKING)
 	SOC_ISR_STACKING_ESF_DECLARE;
@@ -91,9 +91,9 @@ struct arch_esf {
 	unsigned long sp;		/* preserved (user or kernel) stack pointer */
 #endif
 
-#ifdef CONFIG_EXTRA_EXCEPTION_INFO
+#ifdef CONFIG_EXCEPTION_DEBUG
 	_callee_saved_t *csf;		/* pointer to callee-saved-registers */
-#endif /* CONFIG_EXTRA_EXCEPTION_INFO */
+#endif /* CONFIG_EXCEPTION_DEBUG */
 
 #ifdef CONFIG_RISCV_SOC_CONTEXT_SAVE
 	struct soc_esf soc_context;

soc/nordic/common/vpr/soc_isr_stacking.h

@@ -14,13 +14,20 @@
 
 #define VPR_CPU DT_INST(0, nordic_vpr)
 
+#ifdef CONFIG_EXCEPTION_DEBUG
+#define ESF_CSF _callee_saved_t *csf
+#else
+#define ESF_CSF
+#endif /* CONFIG_EXCEPTION_DEBUG */
+
 #if DT_PROP(VPR_CPU, nordic_bus_width) == 64
 
 #define SOC_ISR_STACKING_ESF_DECLARE                                                               \
 	struct arch_esf {                                                                          \
 		unsigned long s0;                                                                  \
 		unsigned long mstatus;                                                             \
 		struct soc_esf soc_context;                                                        \
+		ESF_CSF;                                                                           \
                                                                                                    \
 		unsigned long t2;                                                                  \
 		unsigned long ra;                                                                  \
@@ -43,6 +50,7 @@
 		unsigned long s0;                                                                  \
 		unsigned long mstatus;                                                             \
 		struct soc_esf soc_context;                                                        \
+		ESF_CSF;                                                                           \
                                                                                                    \
 		unsigned long ra;                                                                  \
 		unsigned long t2;                                                                  \

tests/arch/riscv/fatal/CMakeLists.txt

@@ -4,5 +4,5 @@ cmake_minimum_required(VERSION 3.20.0)
 find_package(Zephyr REQUIRED HINTS $ENV{ZEPHYR_BASE})
 project(riscv_fatal)
 
-FILE(GLOB app_sources src/*.c)
+FILE(GLOB app_sources src/*.S)
 target_sources(app PRIVATE ${app_sources})

tests/arch/riscv/fatal/Kconfig

@@ -0,0 +1,20 @@
+# Copyright (c) 2024 Meta Platforms
+# SPDX-License-Identifier: Apache-2.0
+
+choice
+	prompt "Fatal type"
+	default TEST_RISCV_FATAL_PANIC
+
+config TEST_RISCV_FATAL_PANIC
+	bool "Panic induced fault"
+	help
+	  Tests the error handling via `z_riscv_fatal_error()`
+
+config TEST_RISCV_FATAL_ILLEGAL_INSTRUCTION
+	bool "Illegal instruction induced fault"
+	help
+	  Tests the error handling via `_Fault()`
+
+endchoice
+
+source "Kconfig.zephyr"

tests/arch/riscv/fatal/src/main.S

@@ -0,0 +1,83 @@
+/*
+ * Copyright (c) 2025 Meta Platforms.
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+#include <zephyr/toolchain.h>
+#include <zephyr/linker/sections.h>
+
+/**
+ * Load up a bunch of known values into registers
+ * and expect them to show up in the core dump.
+ * Value is register ABI name kinda spelled out,
+ * followed by zeros to pad to 32 bits,
+ * followed by FF00, followed by hex number of the register,
+ * follwed by the "hex-coded-decimal" number of the register.
+ */
+
+GTEXT(main)
+SECTION_FUNC(TEXT, main)
+
+	li ra, 0xDADA0000FF000101
+
+	/* SP is skipped because it can messes stuff up */
+
+#ifndef CONFIG_RISCV_GP
+	li gp, 0xDADA0000FF000101
+#endif
+
+#ifndef CONFIG_THREAD_LOCAL_STORAGE
+	li tp, 0xE2E20000FF000404
+#endif /* CONFIG_THREAD_LOCAL_STORAGE */
+
+#ifndef CONFIG_TEST_RISCV_FATAL_PANIC
+	/* We will load `0` (RV_ECALL_RUNTIME_EXCEPT) to `t0` */
+	li t0, 0xD0FF0000FF000505
+#endif /* CONFIG_TEST_RISCV_FATAL_PANIC */
+	li t1, 0xD1FF0000FF000606
+	li t2, 0xD2FF0000FF000707
+
+#ifndef CONFIG_FRAME_POINTER
+	li s0, 0xC0FF0000FF000808
+#endif /* CONFIG_FRAME_POINTER */
+
+	li s1, 0xC1FF0000FF000909
+
+	li a0, 0xA0FF0000FF000A10
+	li a1, 0xA1FF0000FF000B11
+	li a2, 0xA2FF0000FF000C12
+	li a3, 0xA3FF0000FF000D13
+	li a4, 0xA4FF0000FF000E14
+	li a5, 0xA5FF0000FF000F15
+
+#ifndef CONFIG_RISCV_ISA_RV32E
+	li a6, 0xA6FF0000FF001016
+	li a7, 0xA7FF0000FF001117
+
+	li s2, 0xC2FF0000FF001218
+	li s3, 0xC3FF0000FF001319
+	li s4, 0xC4FF0000FF001420
+	li s5, 0xC5FF0000FF001521
+	li s6, 0xC6FF0000FF001622
+	li s7, 0xC7FF0000FF001723
+	li s8, 0xC8FF0000FF001824
+	li s9, 0xC9FF0000FF001925
+	li s10, 0xC10FF000FF001A26
+	li s11, 0xC11FF000FF001B27
+
+	li t3, 0xD3FF0000FF001C28
+	li t4, 0xD4FF0000FF001D29
+	li t5, 0xD5FF0000FF001E30
+	li t6, 0xD6FF0000FF001F31
+#endif /* CONFIG_RISCV_ISA_RV32E */
+
+#ifdef CONFIG_TEST_RISCV_FATAL_PANIC
+	li a0, 4 /* K_ERR_KERNEL_PANIC */
+	li t0, 0 /* RV_ECALL_RUNTIME_EXCEPT */
+	ecall
+
+#else /* CONFIG_TEST_RISCV_FATAL_ILLEGAL_INSTRUCTION */
+	.insn 2, 0
+	.insn 2, 0
+#endif /* CONFIG_TEST_RISCV_FATAL_PANIC */