system_safe_mode: Print process stack dump

Add the CONFIG_SYSTEM_SAFE_MODE_PRINT_STACK option for
printing the faulting process stack to the console buffer.
This must be done in safe mode because the console buffer
ignores log messages while in an ISR.

This option is on by default when system safe mode is enabled.

get_panic_stack_pointer is a new overidable function that each
architecture needs to implement to support this feature. The default
implementation returns 0, which will result in no stack being printed.

This is a workaround until coredumps are fully supported.

BUG=b:266084064
BRANCH=None
TEST=System safe mode zephyr test passes
Stack print observed on boten, guybrush, and skyrim boards.
LOW_COVERAGE_REASON=Cortex-M and NDS32 specific code cannot be tested

Change-Id: Ied78ab7e6edca9cfa97c50323d94e39a3fca0eef
Signed-off-by: Rob Barnes <robbarnes@google.com>
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/4301695
Reviewed-by: Boris Mittelberg <bmbm@google.com>

12 files changed, 247 insertions, 47 deletions

diff --git a/common/panic_output.c b/common/panic_output.c
index 87b83043f3..370f5a50b0 100644
--- a/common/panic_output.c
+++ b/common/panic_output.c
@@ -202,6 +202,12 @@ struct panic_data *test_get_panic_data_pointer(void)
 }
 #endif
 
+__overridable uint32_t get_panic_stack_pointer(const struct panic_data *pdata)
+{
+	/* Not Implemented */
+	return 0;
+}
+
 test_mockable struct panic_data *get_panic_data_write(void)
 {
 	/*
diff --git a/common/system_safe_mode.c b/common/system_safe_mode.c
index 4ea2abe4ea..889c67f799 100644
--- a/common/system_safe_mode.c
+++ b/common/system_safe_mode.c
@@ -17,6 +17,8 @@
 #include "timer.h"
 #include "watchdog.h"
 
+#define STACK_PRINT_SIZE_WORDS 32
+
 static bool in_safe_mode;
 
 static const int safe_mode_allowed_hostcmds[] = {
@@ -88,6 +90,32 @@ bool system_is_in_safe_mode(void)
 	return !!in_safe_mode;
 }
 
+/*
+ * Print contents of stack to console buffer.
+ */
+static void print_panic_stack(void)
+{
+	uint32_t sp;
+	const struct panic_data *pdata = panic_get_data();
+
+	ccprintf("\n========== Stack Contents ===========");
+	sp = get_panic_stack_pointer(pdata);
+	for (int i = 0; i < STACK_PRINT_SIZE_WORDS; i++) {
+		if (sp == 0 ||
+		    sp + sizeof(uint32_t) > CONFIG_RAM_BASE + CONFIG_RAM_SIZE) {
+			ccprintf("\nSP(%x) out of range", sp);
+			break;
+		}
+		if (i % 4 == 0)
+			ccprintf("\n%08x:", sp);
+		ccprintf(" %08x", *(uint32_t *)sp);
+		sp += sizeof(uint32_t);
+	}
+	ccprintf("\n");
+	/* Flush so dump isn't mixed with other output */
+	cflush();
+}
+
 bool command_is_allowed_in_safe_mode(int command)
 {
 	for (int i = 0; i < ARRAY_SIZE(safe_mode_allowed_hostcmds); i++)
@@ -98,6 +126,9 @@ bool command_is_allowed_in_safe_mode(int command)
 
 static void system_safe_mode_start(void)
 {
+	ccprintf("*** Post Panic System Safe Mode ***\n");
+	if (IS_ENABLED(CONFIG_SYSTEM_SAFE_MODE_PRINT_STACK))
+		print_panic_stack();
 	if (IS_ENABLED(CONFIG_HOSTCMD_EVENTS))
 		host_set_single_event(EC_HOST_EVENT_PANIC);
 }
diff --git a/core/cortex-m/panic.c b/core/cortex-m/panic.c
index eefe068931..828ec9a7e2 100644
--- a/core/cortex-m/panic.c
+++ b/core/cortex-m/panic.c
@@ -18,6 +18,9 @@
 #include "util.h"
 #include "watchdog.h"
 
+#define BASE_EXCEPTION_FRAME_SIZE_BYTES (8 * sizeof(uint32_t))
+#define FPU_EXCEPTION_FRAME_SIZE_BYTES (18 * sizeof(uint32_t))
+
 /* Whether bus fault is ignored */
 static int bus_fault_ignored;
 
@@ -75,6 +78,53 @@ static int32_t is_frame_in_handler_stack(const uint32_t exc_return)
 	return (exc_return & 0xf) == 1 || (exc_return & 0xf) == 9;
 }
 
+/*
+ * Returns the size of the exception frame.
+ *
+ * See B1.5.7 "Stack alignment on exception entry" of ARM DDI 0403D for details.
+ * In short, the exception frame size can be either 0x20, 0x24, 0x68, or 0x6c
+ * depending on FPU context and padding for 8-byte alignment.
+ */
+static uint32_t get_exception_frame_size(const struct panic_data *pdata)
+{
+	uint32_t frame_size = 0;
+
+	/* base exception frame */
+	frame_size += BASE_EXCEPTION_FRAME_SIZE_BYTES;
+
+	/* CPU uses xPSR[9] to indicate whether it padded the stack for
+	 * alignment or not.
+	 */
+	if (pdata->cm.frame[CORTEX_PANIC_FRAME_REGISTER_PSR] & BIT(9))
+		frame_size += sizeof(uint32_t);
+
+#ifdef CONFIG_FPU
+	/* CPU uses EXC_RETURN[4] to indicate whether it stored extended
+	 * frame for FPU or not.
+	 */
+	if (!(pdata->cm.regs[CORTEX_PANIC_REGISTER_LR] & BIT(4)))
+		frame_size += FPU_EXCEPTION_FRAME_SIZE_BYTES;
+#endif
+
+	return frame_size;
+}
+
+/*
+ * Returns the position of the process stack before the exception frame.
+ * It computes the size of the exception frame and adds it to psp.
+ * If the exception happened in the exception context, it returns psp as is.
+ */
+uint32_t get_panic_stack_pointer(const struct panic_data *pdata)
+{
+	uint32_t psp = pdata->cm.regs[CORTEX_PANIC_REGISTER_PSP];
+
+	if (!is_frame_in_handler_stack(
+		    pdata->cm.regs[CORTEX_PANIC_REGISTER_LR]))
+		psp += get_exception_frame_size(pdata);
+
+	return psp;
+}
+
 #ifdef CONFIG_DEBUG_EXCEPTIONS
 /* Names for each of the bits in the cfs register, starting at bit 0 */
 static const char *const cfsr_name[32] = {
@@ -165,51 +215,6 @@ static void show_fault(uint32_t cfsr, uint32_t hfsr, uint32_t dfsr)
 }
 
 /*
- * Returns the size of the exception frame.
- *
- * See B1.5.7 "Stack alignment on exception entry" of ARM DDI 0403D for details.
- * In short, the exception frame size can be either 0x20, 0x24, 0x68, or 0x6c
- * depending on FPU context and padding for 8-byte alignment.
- */
-static uint32_t get_exception_frame_size(const struct panic_data *pdata)
-{
-	uint32_t frame_size = 0;
-
-	/* base exception frame */
-	frame_size += 8 * sizeof(uint32_t);
-
-	/* CPU uses xPSR[9] to indicate whether it padded the stack for
-	 * alignment or not. */
-	if (pdata->cm.frame[CORTEX_PANIC_FRAME_REGISTER_PSR] & BIT(9))
-		frame_size += sizeof(uint32_t);
-
-#ifdef CONFIG_FPU
-	/* CPU uses EXC_RETURN[4] to indicate whether it stored extended
-	 * frame for FPU or not. */
-	if (!(pdata->cm.regs[CORTEX_PANIC_REGISTER_LR] & BIT(4)))
-		frame_size += 18 * sizeof(uint32_t);
-#endif
-
-	return frame_size;
-}
-
-/*
- * Returns the position of the process stack before the exception frame.
- * It computes the size of the exception frame and adds it to psp.
- * If the exception happened in the exception context, it returns psp as is.
- */
-static uint32_t get_process_stack_position(const struct panic_data *pdata)
-{
-	uint32_t psp = pdata->cm.regs[CORTEX_PANIC_REGISTER_PSP];
-
-	if (!is_frame_in_handler_stack(
-		    pdata->cm.regs[CORTEX_PANIC_REGISTER_LR]))
-		psp += get_exception_frame_size(pdata);
-
-	return psp;
-}
-
-/*
  * Show extra information that might be useful to understand a panic()
  *
  * We show fault register information, including the fault address registers
@@ -235,7 +240,7 @@ static void panic_show_process_stack(const struct panic_data *pdata)
 {
 	panic_printf("\n=========== Process Stack Contents ===========");
 	if (pdata->flags & PANIC_DATA_FLAG_FRAME_VALID) {
-		uint32_t psp = get_process_stack_position(pdata);
+		uint32_t psp = get_panic_stack_pointer(pdata);
 		int i;
 		for (i = 0; i < 16; i++) {
 			if (psp + sizeof(uint32_t) >
@@ -321,7 +326,8 @@ void __keep report_panic(void)
 		     pdata->cm.regs[CORTEX_PANIC_REGISTER_PSP];
 	/* If stack is valid, copy exception frame to pdata */
 	if ((sp & 3) == 0 && sp >= CONFIG_RAM_BASE &&
-	    sp <= CONFIG_RAM_BASE + CONFIG_RAM_SIZE - 8 * sizeof(uint32_t)) {
+	    sp <= CONFIG_RAM_BASE + CONFIG_RAM_SIZE -
+			    BASE_EXCEPTION_FRAME_SIZE_BYTES) {
 		const uint32_t *sregs = (const uint32_t *)sp;
 		int i;
 
diff --git a/core/nds32/panic.c b/core/nds32/panic.c
index a111286144..eafdb637ab 100644
--- a/core/nds32/panic.c
+++ b/core/nds32/panic.c
@@ -134,6 +134,14 @@ void panic_get_reason(uint32_t *reason, uint32_t *info, uint8_t *exception)
 	}
 }
 
+/**
+ * Returns the SP register
+ */
+uint32_t get_panic_stack_pointer(const struct panic_data *pdata)
+{
+	return pdata->nds_n8.regs[15];
+}
+
 static void print_panic_information(uint32_t *regs, uint32_t itype,
 				    uint32_t ipc, uint32_t ipsw)
 {
diff --git a/include/config.h b/include/config.h
index 9b4092db6d..cc95bed7f6 100644
--- a/include/config.h
+++ b/include/config.h
@@ -1762,6 +1762,11 @@
  */
 #undef CONFIG_SYSTEM_SAFE_MODE
 #define CONFIG_SYSTEM_SAFE_MODE_TIMEOUT_MSEC 2000
+/*
+ * Prints the stack of the faulting task to the console buffer in system safe
+ * mode.
+ */
+#define CONFIG_SYSTEM_SAFE_MODE_PRINT_STACK
 
 /*
  * Provide the default GPIO abstraction layer.
diff --git a/include/panic.h b/include/panic.h
index e3323e7cc7..ce357eafad 100644
--- a/include/panic.h
+++ b/include/panic.h
@@ -190,6 +190,12 @@ struct panic_data *test_get_panic_data_pointer(void);
 struct panic_data *get_panic_data_write(void);
 
 /**
+ * Return a pointer to the stack of the process that caused the panic.
+ * The implementation of this function will depend on the architecture.
+ */
+uint32_t get_panic_stack_pointer(const struct panic_data *pdata);
+
+/**
  * Chip-specific implementation for backing up panic data to persistent
  * storage. This function is used to ensure that the panic data can survive loss
  * of VCC power rail.
diff --git a/zephyr/Kconfig.system b/zephyr/Kconfig.system
index 7a92d2686f..1da0c87478 100644
--- a/zephyr/Kconfig.system
+++ b/zephyr/Kconfig.system
@@ -98,6 +98,14 @@ config PLATFORM_EC_SYSTEM_SAFE_MODE_TIMEOUT_MSEC
 	  Safe mode will force a system reset after this timeout. The system may
 	  reset sooner if all safe mode tasks are completed before the timeout.
 
+config PLATFORM_EC_SYSTEM_SAFE_MODE_PRINT_STACK
+	bool "Print stack to log after panic"
+	default y
+	depends on PLATFORM_EC_SYSTEM_SAFE_MODE
+	help
+	  Prints the stack of the faulting thread to the console buffer in
+	  system safe mode.
+
 config PLATFORM_EC_HOST_COMMAND_MEMORY_DUMP
 	bool "Enable Memory Dump Host Commands"
 	select THREAD_STACK_INFO
diff --git a/zephyr/shim/core/cortex-m/CMakeLists.txt b/zephyr/shim/core/cortex-m/CMakeLists.txt
index 030512552a..8cc60076eb 100644
--- a/zephyr/shim/core/cortex-m/CMakeLists.txt
+++ b/zephyr/shim/core/cortex-m/CMakeLists.txt
@@ -4,3 +4,4 @@
 
 zephyr_library_sources_ifdef(CONFIG_PLATFORM_EC_CONSOLE_CMD_IRQ irq_command.c)
 zephyr_library_sources_ifdef(CONFIG_MPU mpu.c)
+zephyr_library_sources(panic.c)
\ No newline at end of file
diff --git a/zephyr/shim/core/cortex-m/panic.c b/zephyr/shim/core/cortex-m/panic.c
new file mode 100644
index 0000000000..d42eec5e1f
--- /dev/null
+++ b/zephyr/shim/core/cortex-m/panic.c
@@ -0,0 +1,67 @@
+/* Copyright 2023 The ChromiumOS Authors
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+
+#include "panic.h"
+
+#define BASE_EXCEPTION_FRAME_SIZE_BYTES (8 * sizeof(uint32_t))
+#define FPU_EXCEPTION_FRAME_SIZE_BYTES (18 * sizeof(uint32_t))
+
+/*
+ * Returns non-zero if the exception frame was created on the main stack, or
+ * zero if it's on the process stack.
+ *
+ * See B1.5.8 "Exception return behavior" of ARM DDI 0403D for details.
+ */
+static int32_t is_frame_in_handler_stack(const uint32_t exc_return)
+{
+	return (exc_return & 0xf) == 1 || (exc_return & 0xf) == 9;
+}
+
+/*
+ * Returns the size of the exception frame.
+ *
+ * See B1.5.7 "Stack alignment on exception entry" of ARM DDI 0403D for details.
+ * In short, the exception frame size can be either 0x20, 0x24, 0x68, or 0x6c
+ * depending on FPU context and padding for 8-byte alignment.
+ */
+static uint32_t get_exception_frame_size(const struct panic_data *pdata)
+{
+	uint32_t frame_size = 0;
+
+	/* base exception frame */
+	frame_size += BASE_EXCEPTION_FRAME_SIZE_BYTES;
+
+	/* CPU uses xPSR[9] to indicate whether it padded the stack for
+	 * alignment or not.
+	 */
+	if (pdata->cm.frame[CORTEX_PANIC_FRAME_REGISTER_PSR] & BIT(9))
+		frame_size += sizeof(uint32_t);
+
+#ifdef CONFIG_FPU
+	/* CPU uses EXC_RETURN[4] to indicate whether it stored extended
+	 * frame for FPU or not.
+	 */
+	if (!(pdata->cm.regs[CORTEX_PANIC_REGISTER_LR] & BIT(4)))
+		frame_size += FPU_EXCEPTION_FRAME_SIZE_BYTES;
+#endif
+
+	return frame_size;
+}
+
+/*
+ * Returns the position of the process stack before the exception frame.
+ * It computes the size of the exception frame and adds it to psp.
+ * If the exception happened in the exception context, it returns psp as is.
+ */
+uint32_t get_panic_stack_pointer(const struct panic_data *pdata)
+{
+	uint32_t psp = pdata->cm.regs[CORTEX_PANIC_REGISTER_PSP];
+
+	if (!is_frame_in_handler_stack(
+		    pdata->cm.regs[CORTEX_PANIC_REGISTER_LR]))
+		psp += get_exception_frame_size(pdata);
+
+	return psp;
+}
diff --git a/zephyr/shim/include/config_chip.h b/zephyr/shim/include/config_chip.h
index 2d110bbe84..4b00a952c5 100644
--- a/zephyr/shim/include/config_chip.h
+++ b/zephyr/shim/include/config_chip.h
@@ -2980,6 +2980,11 @@ BUILD_ASSERT((DT_NUM_INST_STATUS_OKAY(mps_mp2964)) == 1,
 	CONFIG_PLATFORM_EC_SYSTEM_SAFE_MODE_TIMEOUT_MSEC
 #endif
 
+#undef CONFIG_SYSTEM_SAFE_MODE_PRINT_STACK
+#ifdef CONFIG_PLATFORM_EC_SYSTEM_SAFE_MODE_PRINT_STACK
+#define CONFIG_SYSTEM_SAFE_MODE_PRINT_STACK
+#endif
+
 #undef CONFIG_BATT_HOST_FULL_FACTOR
 #ifdef CONFIG_PLATFORM_EC_BATT_HOST_FULL_FACTOR
 #define CONFIG_BATT_HOST_FULL_FACTOR CONFIG_PLATFORM_EC_BATT_HOST_FULL_FACTOR
diff --git a/zephyr/test/system_safe_mode/prj.conf b/zephyr/test/system_safe_mode/prj.conf
index 6c520dd235..bb6c6906c8 100644
--- a/zephyr/test/system_safe_mode/prj.conf
+++ b/zephyr/test/system_safe_mode/prj.conf
@@ -9,6 +9,7 @@ CONFIG_CROS_EC=y
 CONFIG_FLASH=y
 CONFIG_SYSTEM_FAKE=y
 CONFIG_PLATFORM_EC_SYSTEM_SAFE_MODE=y
+CONFIG_PLATFORM_EC_SYSTEM_SAFE_MODE_PRINT_STACK=y
 CONFIG_TASK_HOSTCMD_THREAD_DEDICATED=y
 CONFIG_PLATFORM_EC_HOSTCMD=y
 CONFIG_ASSERT_TEST=y
diff --git a/zephyr/test/system_safe_mode/src/system_safe_mode.c b/zephyr/test/system_safe_mode/src/system_safe_mode.c
index 68c6dc4797..f75a27ec84 100644
--- a/zephyr/test/system_safe_mode/src/system_safe_mode.c
+++ b/zephyr/test/system_safe_mode/src/system_safe_mode.c
@@ -3,6 +3,7 @@
  * found in the LICENSE file.
  */
 
+#include "console.h"
 #include "ec_commands.h"
 #include "ec_tasks.h"
 #include "host_command.h"
@@ -10,11 +11,14 @@
 #include "system.h"
 #include "system_fake.h"
 #include "system_safe_mode.h"
+#include "uart.h"
 
 #include <zephyr/fff.h>
 #include <zephyr/kernel.h>
 #include <zephyr/ztest.h>
 
+#include <regex.h>
+
 FAKE_VOID_FUNC(system_reset, int);
 
 static void system_before(void *data)
@@ -23,6 +27,7 @@ static void system_before(void *data)
 	set_system_safe_mode(false);
 	get_panic_data_write()->flags = 0;
 	system_set_shrspi_image_copy(EC_IMAGE_RW);
+	shell_start(get_ec_shell());
 }
 
 static void enter_safe_mode_cb(struct k_timer *unused)
@@ -155,4 +160,55 @@ ZTEST_USER(system_safe_mode, test_panic_event_notify)
 	zassert_true(host_is_event_set(EC_HOST_EVENT_PANIC));
 }
 
+static uint32_t fake_stack[] = {
+	0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0a,
+	0x0b, 0x0c, 0x0d, 0x0e, 0x0f, 0x10, 0x11, 0x12, 0x13, 0x14, 0x15,
+	0x16, 0x17, 0x18, 0x19, 0x1a, 0x1b, 0x1c, 0x1d, 0x1e, 0x1f,
+};
+
+uint32_t get_panic_stack_pointer(const struct panic_data *pdata)
+{
+	return (uint32_t)fake_stack;
+}
+
+ZTEST_USER(system_safe_mode, test_print_stack_contents)
+{
+	char buffer[1024];
+	uint16_t write_count;
+	regex_t regex;
+
+	char *regex_str = "========== Stack Contents ===========\n"
+			  "[0-9a-f]{8}: 00000000 00000001 00000002 00000003\n"
+			  "[0-9a-f]{8}: 00000004 00000005 00000006 00000007\n"
+			  "[0-9a-f]{8}: 00000008 00000009 0000000a 0000000b\n"
+			  "[0-9a-f]{8}: 0000000c 0000000d 0000000e 0000000f\n"
+			  "[0-9a-f]{8}: 00000010 00000011 00000012 00000013\n"
+			  "[0-9a-f]{8}: 00000014 00000015 00000016 00000017\n"
+			  "[0-9a-f]{8}: 00000018 00000019 0000001a 0000001b\n"
+			  "[0-9a-f]{8}: 0000001c 0000001d 0000001e 0000001f\n";
+
+	zassert_ok(regcomp(&regex, regex_str, REG_EXTENDED));
+
+	/* Snapshot console before panic */
+	zassert_ok(uart_console_read_buffer_init(), NULL);
+
+	k_sys_fatal_error_handler(K_ERR_CPU_EXCEPTION, NULL);
+	/* Short sleep to allow hook task to run */
+	k_msleep(1);
+	zassert_true(system_is_in_safe_mode());
+
+	/* Snapshot console after panic */
+	zassert_ok(uart_console_read_buffer_init(), NULL);
+
+	zassert_ok(uart_console_read_buffer(CONSOLE_READ_RECENT, buffer,
+					    sizeof(buffer), &write_count),
+		   NULL);
+	/* Need at least 405 bytes for match */
+	zassert_true(write_count >= 405);
+
+	/* Check for expected stack print in console buffer */
+	zassert_ok(regexec(&regex, buffer, 0, NULL, 0));
+	regfree(&regex);
+}
+
 ZTEST_SUITE(system_safe_mode, NULL, NULL, system_before, NULL, NULL);