[PATCH 0/9] Improve trap handling for nested traps

[PATCH 0/9] Improve trap handling for nested traps

[Anup Patel]

Nested traps will be a common when dealing with RAS error traps so
this series improves trap handling for nested traps by introducing
a linked-list based trap context chain.

These patches can also be found the trap_handling_imp_v1 branch at
https://github.com/avpatel/opensbi.git

Anup Patel (9):
  lib: sbi: Remove sbi_trap_exit() and related code
  include: sbi: Add trap_context pointer in struct sbi_scratch
  lib: sbi: Introduce trap context
  lib: sbi: Simplify parameters of misaligned and access fault handlers
  lib: sbi: Simplify parameters of sbi_illegal_insn_handler()
  lib: sbi: Remove regs paramter of sbi_irqchip_process()
  lib: sbi: Remove regs parameter from trap irq handling functions
  lib: sbi: Pass trap context pointer to sbi_ecall_handler()
  lib: sbi: Extend sbi_trap_error() to dump state in a nested trap

 firmware/fw_base.S             |  14 +--
 include/sbi/sbi_ecall.h        |   4 +-
 include/sbi/sbi_illegal_insn.h |   4 +-
 include/sbi/sbi_irqchip.h      |   5 +-
 include/sbi/sbi_scratch.h      |  14 +--
 include/sbi/sbi_trap.h         |  24 ++++-
 include/sbi/sbi_trap_ldst.h    |  12 +--
 lib/sbi/sbi_ecall.c            |   3 +-
 lib/sbi/sbi_illegal_insn.c     |  14 +--
 lib/sbi/sbi_irqchip.c          |  10 +-
 lib/sbi/sbi_trap.c             | 186 +++++++++++++++++----------------
 lib/sbi/sbi_trap_ldst.c        |  67 ++++++------
 lib/utils/irqchip/imsic.c      |   2 +-
 13 files changed, 185 insertions(+), 174 deletions(-)

-- 
2.34.1

[PATCH 1/9] lib: sbi: Remove sbi_trap_exit() and related code

[Anup Patel]

Over the years, no uses of sbi_trap_exit() have been found so remove
it and also remove related code from fw_base.S and sbi_scratch.h.

Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
 firmware/fw_base.S        | 11 -----------
 include/sbi/sbi_scratch.h | 15 +++------------
 include/sbi/sbi_trap.h    |  2 --
 lib/sbi/sbi_trap.c        | 19 -------------------
 4 files changed, 3 insertions(+), 44 deletions(-)

diff --git a/firmware/fw_base.S b/firmware/fw_base.S
index 126b067..c404d8b 100644
--- a/firmware/fw_base.S
+++ b/firmware/fw_base.S
@@ -298,9 +298,6 @@ _scratch_init:
 	/* Store hartid-to-scratch function address in scratch space */
 	lla	a4, _hartid_to_scratch
 	REG_S	a4, SBI_SCRATCH_HARTID_TO_SCRATCH_OFFSET(tp)
-	/* Store trap-exit function address in scratch space */
-	lla	a4, _trap_exit
-	REG_S	a4, SBI_SCRATCH_TRAP_EXIT_OFFSET(tp)
 	/* Clear tmp0 in scratch space */
 	REG_S	zero, SBI_SCRATCH_TMP0_OFFSET(tp)
 	/* Store firmware options in scratch space */
@@ -453,10 +450,6 @@ _start_warm:
 	srli	a5, a5, ('H' - 'A')
 	andi	a5, a5, 0x1
 	beq	a5, zero, _skip_trap_handler_rv32_hyp
-	/* Override trap exit for H-extension */
-	csrr	a5, CSR_MSCRATCH
-	lla	a4, _trap_exit_rv32_hyp
-	REG_S	a4, SBI_SCRATCH_TRAP_EXIT_OFFSET(a5)
 	lla	a4, _trap_handler_rv32_hyp
 _skip_trap_handler_rv32_hyp:
 #endif
@@ -695,7 +688,6 @@ memcmp:
 	.section .entry, "ax", %progbits
 	.align 3
 	.globl _trap_handler
-	.globl _trap_exit
 _trap_handler:
 	TRAP_SAVE_AND_SETUP_SP_T0
 
@@ -705,7 +697,6 @@ _trap_handler:
 
 	TRAP_CALL_C_ROUTINE
 
-_trap_exit:
 	TRAP_RESTORE_GENERAL_REGS_EXCEPT_A0_T0
 
 	TRAP_RESTORE_MEPC_MSTATUS 0
@@ -718,7 +709,6 @@ _trap_exit:
 	.section .entry, "ax", %progbits
 	.align 3
 	.globl _trap_handler_rv32_hyp
-	.globl _trap_exit_rv32_hyp
 _trap_handler_rv32_hyp:
 	TRAP_SAVE_AND_SETUP_SP_T0
 
@@ -728,7 +718,6 @@ _trap_handler_rv32_hyp:
 
 	TRAP_CALL_C_ROUTINE
 
-_trap_exit_rv32_hyp:
 	TRAP_RESTORE_GENERAL_REGS_EXCEPT_A0_T0
 
 	TRAP_RESTORE_MEPC_MSTATUS 1
diff --git a/include/sbi/sbi_scratch.h b/include/sbi/sbi_scratch.h
index e6a33ba..55b937f 100644
--- a/include/sbi/sbi_scratch.h
+++ b/include/sbi/sbi_scratch.h
@@ -36,14 +36,12 @@
 #define SBI_SCRATCH_PLATFORM_ADDR_OFFSET	(9 * __SIZEOF_POINTER__)
 /** Offset of hartid_to_scratch member in sbi_scratch */
 #define SBI_SCRATCH_HARTID_TO_SCRATCH_OFFSET	(10 * __SIZEOF_POINTER__)
-/** Offset of trap_exit member in sbi_scratch */
-#define SBI_SCRATCH_TRAP_EXIT_OFFSET		(11 * __SIZEOF_POINTER__)
 /** Offset of tmp0 member in sbi_scratch */
-#define SBI_SCRATCH_TMP0_OFFSET			(12 * __SIZEOF_POINTER__)
+#define SBI_SCRATCH_TMP0_OFFSET			(11 * __SIZEOF_POINTER__)
 /** Offset of options member in sbi_scratch */
-#define SBI_SCRATCH_OPTIONS_OFFSET		(13 * __SIZEOF_POINTER__)
+#define SBI_SCRATCH_OPTIONS_OFFSET		(12 * __SIZEOF_POINTER__)
 /** Offset of extra space in sbi_scratch */
-#define SBI_SCRATCH_EXTRA_SPACE_OFFSET		(14 * __SIZEOF_POINTER__)
+#define SBI_SCRATCH_EXTRA_SPACE_OFFSET		(13 * __SIZEOF_POINTER__)
 /** Maximum size of sbi_scratch (4KB) */
 #define SBI_SCRATCH_SIZE			(0x1000)
 
@@ -77,8 +75,6 @@ struct sbi_scratch {
 	unsigned long platform_addr;
 	/** Address of HART ID to sbi_scratch conversion function */
 	unsigned long hartid_to_scratch;
-	/** Address of trap exit function */
-	unsigned long trap_exit;
 	/** Temporary storage */
 	unsigned long tmp0;
 	/** Options for OpenSBI library */
@@ -129,11 +125,6 @@ _Static_assert(
 		== SBI_SCRATCH_HARTID_TO_SCRATCH_OFFSET,
 	"struct sbi_scratch definition has changed, please redefine "
 	"SBI_SCRATCH_HARTID_TO_SCRATCH_OFFSET");
-_Static_assert(
-	offsetof(struct sbi_scratch, trap_exit)
-		== SBI_SCRATCH_TRAP_EXIT_OFFSET,
-	"struct sbi_scratch definition has changed, please redefine "
-	"SBI_SCRATCH_TRAP_EXIT_OFFSET");
 _Static_assert(
 	offsetof(struct sbi_scratch, tmp0)
 		== SBI_SCRATCH_TMP0_OFFSET,
diff --git a/include/sbi/sbi_trap.h b/include/sbi/sbi_trap.h
index 2727bdb..15ccd0b 100644
--- a/include/sbi/sbi_trap.h
+++ b/include/sbi/sbi_trap.h
@@ -229,8 +229,6 @@ int sbi_trap_redirect(struct sbi_trap_regs *regs,
 
 struct sbi_trap_regs *sbi_trap_handler(struct sbi_trap_regs *regs);
 
-void __noreturn sbi_trap_exit(const struct sbi_trap_regs *regs);
-
 #endif
 
 #endif
diff --git a/lib/sbi/sbi_trap.c b/lib/sbi/sbi_trap.c
index c665013..e514066 100644
--- a/lib/sbi/sbi_trap.c
+++ b/lib/sbi/sbi_trap.c
@@ -335,22 +335,3 @@ trap_error:
 		sbi_trap_error(msg, rc, mcause, mtval, mtval2, mtinst, regs);
 	return regs;
 }
-
-typedef void (*trap_exit_t)(const struct sbi_trap_regs *regs);
-
-/**
- * Exit trap/interrupt handling
- *
- * This function is called by non-firmware code to abruptly exit
- * trap/interrupt handling and resume execution at context pointed
- * by given register state.
- *
- * @param regs pointer to register state
- */
-void __noreturn sbi_trap_exit(const struct sbi_trap_regs *regs)
-{
-	struct sbi_scratch *scratch = sbi_scratch_thishart_ptr();
-
-	((trap_exit_t)scratch->trap_exit)(regs);
-	__builtin_unreachable();
-}
-- 
2.34.1

[PATCH 1/9] lib: sbi: Remove sbi_trap_exit() and related code

[Samuel Holland]

Hi Anup,

On 2024-03-11 11:09 AM, Anup Patel wrote:
> Over the years, no uses of sbi_trap_exit() have been found so remove
> it and also remove related code from fw_base.S and sbi_scratch.h.
> 
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> ---
>  firmware/fw_base.S        | 11 -----------
>  include/sbi/sbi_scratch.h | 15 +++------------
>  include/sbi/sbi_trap.h    |  2 --
>  lib/sbi/sbi_trap.c        | 19 -------------------
>  4 files changed, 3 insertions(+), 44 deletions(-)
> 
> diff --git a/firmware/fw_base.S b/firmware/fw_base.S
> index 126b067..c404d8b 100644
> --- a/firmware/fw_base.S
> +++ b/firmware/fw_base.S
> @@ -298,9 +298,6 @@ _scratch_init:
>  	/* Store hartid-to-scratch function address in scratch space */
>  	lla	a4, _hartid_to_scratch
>  	REG_S	a4, SBI_SCRATCH_HARTID_TO_SCRATCH_OFFSET(tp)
> -	/* Store trap-exit function address in scratch space */
> -	lla	a4, _trap_exit
> -	REG_S	a4, SBI_SCRATCH_TRAP_EXIT_OFFSET(tp)
>  	/* Clear tmp0 in scratch space */
>  	REG_S	zero, SBI_SCRATCH_TMP0_OFFSET(tp)
>  	/* Store firmware options in scratch space */
> @@ -453,10 +450,6 @@ _start_warm:
>  	srli	a5, a5, ('H' - 'A')
>  	andi	a5, a5, 0x1
>  	beq	a5, zero, _skip_trap_handler_rv32_hyp
> -	/* Override trap exit for H-extension */
> -	csrr	a5, CSR_MSCRATCH
> -	lla	a4, _trap_exit_rv32_hyp
> -	REG_S	a4, SBI_SCRATCH_TRAP_EXIT_OFFSET(a5)
>  	lla	a4, _trap_handler_rv32_hyp
>  _skip_trap_handler_rv32_hyp:
>  #endif
> @@ -695,7 +688,6 @@ memcmp:
>  	.section .entry, "ax", %progbits
>  	.align 3
>  	.globl _trap_handler
> -	.globl _trap_exit
>  _trap_handler:
>  	TRAP_SAVE_AND_SETUP_SP_T0
>  
> @@ -705,7 +697,6 @@ _trap_handler:
>  
>  	TRAP_CALL_C_ROUTINE
>  
> -_trap_exit:
>  	TRAP_RESTORE_GENERAL_REGS_EXCEPT_A0_T0
>  
>  	TRAP_RESTORE_MEPC_MSTATUS 0
> @@ -718,7 +709,6 @@ _trap_exit:
>  	.section .entry, "ax", %progbits
>  	.align 3
>  	.globl _trap_handler_rv32_hyp
> -	.globl _trap_exit_rv32_hyp
>  _trap_handler_rv32_hyp:
>  	TRAP_SAVE_AND_SETUP_SP_T0
>  
> @@ -728,7 +718,6 @@ _trap_handler_rv32_hyp:
>  
>  	TRAP_CALL_C_ROUTINE
>  
> -_trap_exit_rv32_hyp:
>  	TRAP_RESTORE_GENERAL_REGS_EXCEPT_A0_T0
>  
>  	TRAP_RESTORE_MEPC_MSTATUS 1
> diff --git a/include/sbi/sbi_scratch.h b/include/sbi/sbi_scratch.h
> index e6a33ba..55b937f 100644
> --- a/include/sbi/sbi_scratch.h
> +++ b/include/sbi/sbi_scratch.h
> @@ -36,14 +36,12 @@
>  #define SBI_SCRATCH_PLATFORM_ADDR_OFFSET	(9 * __SIZEOF_POINTER__)
>  /** Offset of hartid_to_scratch member in sbi_scratch */
>  #define SBI_SCRATCH_HARTID_TO_SCRATCH_OFFSET	(10 * __SIZEOF_POINTER__)
> -/** Offset of trap_exit member in sbi_scratch */
> -#define SBI_SCRATCH_TRAP_EXIT_OFFSET		(11 * __SIZEOF_POINTER__)
>  /** Offset of tmp0 member in sbi_scratch */
> -#define SBI_SCRATCH_TMP0_OFFSET			(12 * __SIZEOF_POINTER__)
> +#define SBI_SCRATCH_TMP0_OFFSET			(11 * __SIZEOF_POINTER__)
>  /** Offset of options member in sbi_scratch */
> -#define SBI_SCRATCH_OPTIONS_OFFSET		(13 * __SIZEOF_POINTER__)
> +#define SBI_SCRATCH_OPTIONS_OFFSET		(12 * __SIZEOF_POINTER__)
>  /** Offset of extra space in sbi_scratch */
> -#define SBI_SCRATCH_EXTRA_SPACE_OFFSET		(14 * __SIZEOF_POINTER__)
> +#define SBI_SCRATCH_EXTRA_SPACE_OFFSET		(13 * __SIZEOF_POINTER__)
>  /** Maximum size of sbi_scratch (4KB) */
>  #define SBI_SCRATCH_SIZE			(0x1000)
>  
> @@ -77,8 +75,6 @@ struct sbi_scratch {
>  	unsigned long platform_addr;
>  	/** Address of HART ID to sbi_scratch conversion function */
>  	unsigned long hartid_to_scratch;
> -	/** Address of trap exit function */
> -	unsigned long trap_exit;

It would avoid some churn (and effects on `git blame`) to replace this with
padding instead of changing the offsets for one commit, but either way:

Reviewed-by: Samuel Holland <samuel.holland@sifive.com>

>  	/** Temporary storage */
>  	unsigned long tmp0;
>  	/** Options for OpenSBI library */
> @@ -129,11 +125,6 @@ _Static_assert(
>  		== SBI_SCRATCH_HARTID_TO_SCRATCH_OFFSET,
>  	"struct sbi_scratch definition has changed, please redefine "
>  	"SBI_SCRATCH_HARTID_TO_SCRATCH_OFFSET");
> -_Static_assert(
> -	offsetof(struct sbi_scratch, trap_exit)
> -		== SBI_SCRATCH_TRAP_EXIT_OFFSET,
> -	"struct sbi_scratch definition has changed, please redefine "
> -	"SBI_SCRATCH_TRAP_EXIT_OFFSET");
>  _Static_assert(
>  	offsetof(struct sbi_scratch, tmp0)
>  		== SBI_SCRATCH_TMP0_OFFSET,
> diff --git a/include/sbi/sbi_trap.h b/include/sbi/sbi_trap.h
> index 2727bdb..15ccd0b 100644
> --- a/include/sbi/sbi_trap.h
> +++ b/include/sbi/sbi_trap.h
> @@ -229,8 +229,6 @@ int sbi_trap_redirect(struct sbi_trap_regs *regs,
>  
>  struct sbi_trap_regs *sbi_trap_handler(struct sbi_trap_regs *regs);
>  
> -void __noreturn sbi_trap_exit(const struct sbi_trap_regs *regs);
> -
>  #endif
>  
>  #endif
> diff --git a/lib/sbi/sbi_trap.c b/lib/sbi/sbi_trap.c
> index c665013..e514066 100644
> --- a/lib/sbi/sbi_trap.c
> +++ b/lib/sbi/sbi_trap.c
> @@ -335,22 +335,3 @@ trap_error:
>  		sbi_trap_error(msg, rc, mcause, mtval, mtval2, mtinst, regs);
>  	return regs;
>  }
> -
> -typedef void (*trap_exit_t)(const struct sbi_trap_regs *regs);
> -
> -/**
> - * Exit trap/interrupt handling
> - *
> - * This function is called by non-firmware code to abruptly exit
> - * trap/interrupt handling and resume execution at context pointed
> - * by given register state.
> - *
> - * @param regs pointer to register state
> - */
> -void __noreturn sbi_trap_exit(const struct sbi_trap_regs *regs)
> -{
> -	struct sbi_scratch *scratch = sbi_scratch_thishart_ptr();
> -
> -	((trap_exit_t)scratch->trap_exit)(regs);
> -	__builtin_unreachable();
> -}

[PATCH 2/9] include: sbi: Add trap_context pointer in struct sbi_scratch

[Anup Patel]

To track nested traps, the struct sbi_scratch needs a pointer the
current trap context so add trap_context pointer in struct sbi_context.

Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
 firmware/fw_base.S        |  3 ++-
 include/sbi/sbi_scratch.h | 15 ++++++++++++---
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/firmware/fw_base.S b/firmware/fw_base.S
index c404d8b..539fd1d 100644
--- a/firmware/fw_base.S
+++ b/firmware/fw_base.S
@@ -298,7 +298,8 @@ _scratch_init:
 	/* Store hartid-to-scratch function address in scratch space */
 	lla	a4, _hartid_to_scratch
 	REG_S	a4, SBI_SCRATCH_HARTID_TO_SCRATCH_OFFSET(tp)
-	/* Clear tmp0 in scratch space */
+	/* Clear trap_context and tmp0 in scratch space */
+	REG_S	zero, SBI_SCRATCH_TRAP_CONTEXT_OFFSET(tp)
 	REG_S	zero, SBI_SCRATCH_TMP0_OFFSET(tp)
 	/* Store firmware options in scratch space */
 	MOV_3R	s0, a0, s1, a1, s2, a2
diff --git a/include/sbi/sbi_scratch.h b/include/sbi/sbi_scratch.h
index 55b937f..12e6a98 100644
--- a/include/sbi/sbi_scratch.h
+++ b/include/sbi/sbi_scratch.h
@@ -36,12 +36,14 @@
 #define SBI_SCRATCH_PLATFORM_ADDR_OFFSET	(9 * __SIZEOF_POINTER__)
 /** Offset of hartid_to_scratch member in sbi_scratch */
 #define SBI_SCRATCH_HARTID_TO_SCRATCH_OFFSET	(10 * __SIZEOF_POINTER__)
+/** Offset of trap_context member in sbi_scratch */
+#define SBI_SCRATCH_TRAP_CONTEXT_OFFSET		(11 * __SIZEOF_POINTER__)
 /** Offset of tmp0 member in sbi_scratch */
-#define SBI_SCRATCH_TMP0_OFFSET			(11 * __SIZEOF_POINTER__)
+#define SBI_SCRATCH_TMP0_OFFSET			(12 * __SIZEOF_POINTER__)
 /** Offset of options member in sbi_scratch */
-#define SBI_SCRATCH_OPTIONS_OFFSET		(12 * __SIZEOF_POINTER__)
+#define SBI_SCRATCH_OPTIONS_OFFSET		(13 * __SIZEOF_POINTER__)
 /** Offset of extra space in sbi_scratch */
-#define SBI_SCRATCH_EXTRA_SPACE_OFFSET		(13 * __SIZEOF_POINTER__)
+#define SBI_SCRATCH_EXTRA_SPACE_OFFSET		(14 * __SIZEOF_POINTER__)
 /** Maximum size of sbi_scratch (4KB) */
 #define SBI_SCRATCH_SIZE			(0x1000)
 
@@ -75,6 +77,8 @@ struct sbi_scratch {
 	unsigned long platform_addr;
 	/** Address of HART ID to sbi_scratch conversion function */
 	unsigned long hartid_to_scratch;
+	/** Address of current trap context */
+	unsigned long trap_context;
 	/** Temporary storage */
 	unsigned long tmp0;
 	/** Options for OpenSBI library */
@@ -125,6 +129,11 @@ _Static_assert(
 		== SBI_SCRATCH_HARTID_TO_SCRATCH_OFFSET,
 	"struct sbi_scratch definition has changed, please redefine "
 	"SBI_SCRATCH_HARTID_TO_SCRATCH_OFFSET");
+_Static_assert(
+	offsetof(struct sbi_scratch, trap_context)
+		== SBI_SCRATCH_TRAP_CONTEXT_OFFSET,
+	"struct sbi_scratch definition has changed, please redefine "
+	"SBI_SCRATCH_TRAP_CONTEXT_OFFSET");
 _Static_assert(
 	offsetof(struct sbi_scratch, tmp0)
 		== SBI_SCRATCH_TMP0_OFFSET,
-- 
2.34.1

[PATCH 3/9] lib: sbi: Introduce trap context

[Anup Patel]

Club pointers to struct sbi_trap_regs and struct sbi_trap_info a new
struct sbi_trap_context (aka trap context).

To track nested traps, the struct sbi_scratch points to the current
trap context and the trap context has pointer to pervious context of
previous trap.

Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
 include/sbi/sbi_trap.h | 22 ++++++++++++++++++++++
 lib/sbi/sbi_trap.c     | 35 ++++++++++++++++++++++-------------
 2 files changed, 44 insertions(+), 13 deletions(-)

diff --git a/include/sbi/sbi_trap.h b/include/sbi/sbi_trap.h
index 15ccd0b..513a82e 100644
--- a/include/sbi/sbi_trap.h
+++ b/include/sbi/sbi_trap.h
@@ -117,6 +117,7 @@
 #ifndef __ASSEMBLER__
 
 #include <sbi/sbi_types.h>
+#include <sbi/sbi_scratch.h>
 
 /** Representation of register state at time of trap/interrupt */
 struct sbi_trap_regs {
@@ -208,6 +209,16 @@ struct sbi_trap_info {
 	unsigned long gva;
 };
 
+/** Representation of trap context saved on stack */
+struct sbi_trap_context {
+	/** Pointer to previous trap context */
+	struct sbi_trap_context *prev_context;
+	/** Pointer to register state */
+	struct sbi_trap_regs *regs;
+	/** Pointer to trap details */
+	const struct sbi_trap_info *trap;
+};
+
 static inline unsigned long sbi_regs_gva(const struct sbi_trap_regs *regs)
 {
 	/*
@@ -227,6 +238,17 @@ static inline unsigned long sbi_regs_gva(const struct sbi_trap_regs *regs)
 int sbi_trap_redirect(struct sbi_trap_regs *regs,
 		      const struct sbi_trap_info *trap);
 
+static inline struct sbi_trap_context *sbi_trap_get_context(struct sbi_scratch *scratch)
+{
+	return (scratch) ? (void *)scratch->trap_context : NULL;
+}
+
+static inline void sbi_trap_set_context(struct sbi_scratch *scratch,
+					struct sbi_trap_context *tcntx)
+{
+	scratch->trap_context = (unsigned long)tcntx;
+}
+
 struct sbi_trap_regs *sbi_trap_handler(struct sbi_trap_regs *regs);
 
 #endif
diff --git a/lib/sbi/sbi_trap.c b/lib/sbi/sbi_trap.c
index e514066..dba267c 100644
--- a/lib/sbi/sbi_trap.c
+++ b/lib/sbi/sbi_trap.c
@@ -266,6 +266,8 @@ struct sbi_trap_regs *sbi_trap_handler(struct sbi_trap_regs *regs)
 	const char *msg = "trap handler failed";
 	ulong mcause = csr_read(CSR_MCAUSE);
 	ulong mtval = csr_read(CSR_MTVAL), mtval2 = 0, mtinst = 0;
+	struct sbi_scratch *scratch = sbi_scratch_thishart_ptr();
+	struct sbi_trap_context tcntx;
 	struct sbi_trap_info trap;
 
 	if (misa_extension('H')) {
@@ -273,25 +275,31 @@ struct sbi_trap_regs *sbi_trap_handler(struct sbi_trap_regs *regs)
 		mtinst = csr_read(CSR_MTINST);
 	}
 
+	/* Original trap_info */
+	trap.epc     = regs->mepc;
+	trap.cause   = mcause;
+	trap.tval    = mtval;
+	trap.tval2   = mtval2;
+	trap.tinst   = mtinst;
+	trap.gva     = sbi_regs_gva(regs);
+
+	/* Setup trap context */
+	tcntx.prev_context = sbi_trap_get_context(scratch);
+	tcntx.regs         = regs;
+	tcntx.trap         = &trap;
+
+	/* Update trap context pointer */
+	sbi_trap_set_context(scratch, &tcntx);
+
 	if (mcause & (1UL << (__riscv_xlen - 1))) {
 		if (sbi_hart_has_extension(sbi_scratch_thishart_ptr(),
 					   SBI_HART_EXT_SMAIA))
 			rc = sbi_trap_aia_irq(regs, mcause);
 		else
 			rc = sbi_trap_nonaia_irq(regs, mcause);
-		if (rc) {
-			msg = "unhandled local interrupt";
-			goto trap_error;
-		}
-		return regs;
+		msg = "unhandled local interrupt";
+		goto trap_done;
 	}
-	/* Original trap_info */
-	trap.epc   = regs->mepc;
-	trap.cause = mcause;
-	trap.tval  = mtval;
-	trap.tval2 = mtval2;
-	trap.tinst = mtinst;
-	trap.gva   = sbi_regs_gva(regs);
 
 	switch (mcause) {
 	case CAUSE_ILLEGAL_INSTRUCTION:
@@ -330,8 +338,9 @@ struct sbi_trap_regs *sbi_trap_handler(struct sbi_trap_regs *regs)
 		break;
 	}
 
-trap_error:
+trap_done:
 	if (rc)
 		sbi_trap_error(msg, rc, mcause, mtval, mtval2, mtinst, regs);
+	sbi_trap_set_context(scratch, tcntx.prev_context);
 	return regs;
 }
-- 
2.34.1

[PATCH 3/9] lib: sbi: Introduce trap context

[Samuel Holland]

Hi Anup,

On 2024-03-11 11:09 AM, Anup Patel wrote:
> Club pointers to struct sbi_trap_regs and struct sbi_trap_info a new
> struct sbi_trap_context (aka trap context).
> 
> To track nested traps, the struct sbi_scratch points to the current
> trap context and the trap context has pointer to pervious context of
> previous trap.
> 
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> ---
>  include/sbi/sbi_trap.h | 22 ++++++++++++++++++++++
>  lib/sbi/sbi_trap.c     | 35 ++++++++++++++++++++++-------------
>  2 files changed, 44 insertions(+), 13 deletions(-)
> 
> diff --git a/include/sbi/sbi_trap.h b/include/sbi/sbi_trap.h
> index 15ccd0b..513a82e 100644
> --- a/include/sbi/sbi_trap.h
> +++ b/include/sbi/sbi_trap.h
> @@ -117,6 +117,7 @@
>  #ifndef __ASSEMBLER__
>  
>  #include <sbi/sbi_types.h>
> +#include <sbi/sbi_scratch.h>
>  
>  /** Representation of register state at time of trap/interrupt */
>  struct sbi_trap_regs {
> @@ -208,6 +209,16 @@ struct sbi_trap_info {
>  	unsigned long gva;
>  };
>  
> +/** Representation of trap context saved on stack */
> +struct sbi_trap_context {
> +	/** Pointer to previous trap context */
> +	struct sbi_trap_context *prev_context;
> +	/** Pointer to register state */
> +	struct sbi_trap_regs *regs;
> +	/** Pointer to trap details */
> +	const struct sbi_trap_info *trap;

It would be more efficient to embed these two structures inside the context
instead of pointing to them. The assembly code wouldn't have to fill out
sbi_trap_info or prev_context even if it allocates the whole sbi_trap_context.

Regards,
Samuel

> +};
> +
>  static inline unsigned long sbi_regs_gva(const struct sbi_trap_regs *regs)
>  {
>  	/*
> @@ -227,6 +238,17 @@ static inline unsigned long sbi_regs_gva(const struct sbi_trap_regs *regs)
>  int sbi_trap_redirect(struct sbi_trap_regs *regs,
>  		      const struct sbi_trap_info *trap);
>  
> +static inline struct sbi_trap_context *sbi_trap_get_context(struct sbi_scratch *scratch)
> +{
> +	return (scratch) ? (void *)scratch->trap_context : NULL;
> +}
> +
> +static inline void sbi_trap_set_context(struct sbi_scratch *scratch,
> +					struct sbi_trap_context *tcntx)
> +{
> +	scratch->trap_context = (unsigned long)tcntx;
> +}
> +
>  struct sbi_trap_regs *sbi_trap_handler(struct sbi_trap_regs *regs);
>  
>  #endif
> diff --git a/lib/sbi/sbi_trap.c b/lib/sbi/sbi_trap.c
> index e514066..dba267c 100644
> --- a/lib/sbi/sbi_trap.c
> +++ b/lib/sbi/sbi_trap.c
> @@ -266,6 +266,8 @@ struct sbi_trap_regs *sbi_trap_handler(struct sbi_trap_regs *regs)
>  	const char *msg = "trap handler failed";
>  	ulong mcause = csr_read(CSR_MCAUSE);
>  	ulong mtval = csr_read(CSR_MTVAL), mtval2 = 0, mtinst = 0;
> +	struct sbi_scratch *scratch = sbi_scratch_thishart_ptr();
> +	struct sbi_trap_context tcntx;
>  	struct sbi_trap_info trap;
>  
>  	if (misa_extension('H')) {
> @@ -273,25 +275,31 @@ struct sbi_trap_regs *sbi_trap_handler(struct sbi_trap_regs *regs)
>  		mtinst = csr_read(CSR_MTINST);
>  	}
>  
> +	/* Original trap_info */
> +	trap.epc     = regs->mepc;
> +	trap.cause   = mcause;
> +	trap.tval    = mtval;
> +	trap.tval2   = mtval2;
> +	trap.tinst   = mtinst;
> +	trap.gva     = sbi_regs_gva(regs);
> +
> +	/* Setup trap context */
> +	tcntx.prev_context = sbi_trap_get_context(scratch);
> +	tcntx.regs         = regs;
> +	tcntx.trap         = &trap;
> +
> +	/* Update trap context pointer */
> +	sbi_trap_set_context(scratch, &tcntx);
> +
>  	if (mcause & (1UL << (__riscv_xlen - 1))) {
>  		if (sbi_hart_has_extension(sbi_scratch_thishart_ptr(),
>  					   SBI_HART_EXT_SMAIA))
>  			rc = sbi_trap_aia_irq(regs, mcause);
>  		else
>  			rc = sbi_trap_nonaia_irq(regs, mcause);
> -		if (rc) {
> -			msg = "unhandled local interrupt";
> -			goto trap_error;
> -		}
> -		return regs;
> +		msg = "unhandled local interrupt";
> +		goto trap_done;
>  	}
> -	/* Original trap_info */
> -	trap.epc   = regs->mepc;
> -	trap.cause = mcause;
> -	trap.tval  = mtval;
> -	trap.tval2 = mtval2;
> -	trap.tinst = mtinst;
> -	trap.gva   = sbi_regs_gva(regs);
>  
>  	switch (mcause) {
>  	case CAUSE_ILLEGAL_INSTRUCTION:
> @@ -330,8 +338,9 @@ struct sbi_trap_regs *sbi_trap_handler(struct sbi_trap_regs *regs)
>  		break;
>  	}
>  
> -trap_error:
> +trap_done:
>  	if (rc)
>  		sbi_trap_error(msg, rc, mcause, mtval, mtval2, mtinst, regs);
> +	sbi_trap_set_context(scratch, tcntx.prev_context);
>  	return regs;
>  }

[PATCH 3/9] lib: sbi: Introduce trap context

[Anup Patel]

On Mon, Mar 11, 2024 at 11:02?PM Samuel Holland
<samuel.holland@sifive.com> wrote:
>
> Hi Anup,
>
> On 2024-03-11 11:09 AM, Anup Patel wrote:
> > Club pointers to struct sbi_trap_regs and struct sbi_trap_info a new
> > struct sbi_trap_context (aka trap context).
> >
> > To track nested traps, the struct sbi_scratch points to the current
> > trap context and the trap context has pointer to pervious context of
> > previous trap.
> >
> > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > ---
> >  include/sbi/sbi_trap.h | 22 ++++++++++++++++++++++
> >  lib/sbi/sbi_trap.c     | 35 ++++++++++++++++++++++-------------
> >  2 files changed, 44 insertions(+), 13 deletions(-)
> >
> > diff --git a/include/sbi/sbi_trap.h b/include/sbi/sbi_trap.h
> > index 15ccd0b..513a82e 100644
> > --- a/include/sbi/sbi_trap.h
> > +++ b/include/sbi/sbi_trap.h
> > @@ -117,6 +117,7 @@
> >  #ifndef __ASSEMBLER__
> >
> >  #include <sbi/sbi_types.h>
> > +#include <sbi/sbi_scratch.h>
> >
> >  /** Representation of register state at time of trap/interrupt */
> >  struct sbi_trap_regs {
> > @@ -208,6 +209,16 @@ struct sbi_trap_info {
> >       unsigned long gva;
> >  };
> >
> > +/** Representation of trap context saved on stack */
> > +struct sbi_trap_context {
> > +     /** Pointer to previous trap context */
> > +     struct sbi_trap_context *prev_context;
> > +     /** Pointer to register state */
> > +     struct sbi_trap_regs *regs;
> > +     /** Pointer to trap details */
> > +     const struct sbi_trap_info *trap;
>
> It would be more efficient to embed these two structures inside the context
> instead of pointing to them. The assembly code wouldn't have to fill out
> sbi_trap_info or prev_context even if it allocates the whole sbi_trap_context.

Actually, I was already thinking of minimizing the pointer chasing.

Let me try to do this in the next revision.

Regards,
Anup

>
> Regards,
> Samuel
>
> > +};
> > +
> >  static inline unsigned long sbi_regs_gva(const struct sbi_trap_regs *regs)
> >  {
> >       /*
> > @@ -227,6 +238,17 @@ static inline unsigned long sbi_regs_gva(const struct sbi_trap_regs *regs)
> >  int sbi_trap_redirect(struct sbi_trap_regs *regs,
> >                     const struct sbi_trap_info *trap);
> >
> > +static inline struct sbi_trap_context *sbi_trap_get_context(struct sbi_scratch *scratch)
> > +{
> > +     return (scratch) ? (void *)scratch->trap_context : NULL;
> > +}
> > +
> > +static inline void sbi_trap_set_context(struct sbi_scratch *scratch,
> > +                                     struct sbi_trap_context *tcntx)
> > +{
> > +     scratch->trap_context = (unsigned long)tcntx;
> > +}
> > +
> >  struct sbi_trap_regs *sbi_trap_handler(struct sbi_trap_regs *regs);
> >
> >  #endif
> > diff --git a/lib/sbi/sbi_trap.c b/lib/sbi/sbi_trap.c
> > index e514066..dba267c 100644
> > --- a/lib/sbi/sbi_trap.c
> > +++ b/lib/sbi/sbi_trap.c
> > @@ -266,6 +266,8 @@ struct sbi_trap_regs *sbi_trap_handler(struct sbi_trap_regs *regs)
> >       const char *msg = "trap handler failed";
> >       ulong mcause = csr_read(CSR_MCAUSE);
> >       ulong mtval = csr_read(CSR_MTVAL), mtval2 = 0, mtinst = 0;
> > +     struct sbi_scratch *scratch = sbi_scratch_thishart_ptr();
> > +     struct sbi_trap_context tcntx;
> >       struct sbi_trap_info trap;
> >
> >       if (misa_extension('H')) {
> > @@ -273,25 +275,31 @@ struct sbi_trap_regs *sbi_trap_handler(struct sbi_trap_regs *regs)
> >               mtinst = csr_read(CSR_MTINST);
> >       }
> >
> > +     /* Original trap_info */
> > +     trap.epc     = regs->mepc;
> > +     trap.cause   = mcause;
> > +     trap.tval    = mtval;
> > +     trap.tval2   = mtval2;
> > +     trap.tinst   = mtinst;
> > +     trap.gva     = sbi_regs_gva(regs);
> > +
> > +     /* Setup trap context */
> > +     tcntx.prev_context = sbi_trap_get_context(scratch);
> > +     tcntx.regs         = regs;
> > +     tcntx.trap         = &trap;
> > +
> > +     /* Update trap context pointer */
> > +     sbi_trap_set_context(scratch, &tcntx);
> > +
> >       if (mcause & (1UL << (__riscv_xlen - 1))) {
> >               if (sbi_hart_has_extension(sbi_scratch_thishart_ptr(),
> >                                          SBI_HART_EXT_SMAIA))
> >                       rc = sbi_trap_aia_irq(regs, mcause);
> >               else
> >                       rc = sbi_trap_nonaia_irq(regs, mcause);
> > -             if (rc) {
> > -                     msg = "unhandled local interrupt";
> > -                     goto trap_error;
> > -             }
> > -             return regs;
> > +             msg = "unhandled local interrupt";
> > +             goto trap_done;
> >       }
> > -     /* Original trap_info */
> > -     trap.epc   = regs->mepc;
> > -     trap.cause = mcause;
> > -     trap.tval  = mtval;
> > -     trap.tval2 = mtval2;
> > -     trap.tinst = mtinst;
> > -     trap.gva   = sbi_regs_gva(regs);
> >
> >       switch (mcause) {
> >       case CAUSE_ILLEGAL_INSTRUCTION:
> > @@ -330,8 +338,9 @@ struct sbi_trap_regs *sbi_trap_handler(struct sbi_trap_regs *regs)
> >               break;
> >       }
> >
> > -trap_error:
> > +trap_done:
> >       if (rc)
> >               sbi_trap_error(msg, rc, mcause, mtval, mtval2, mtinst, regs);
> > +     sbi_trap_set_context(scratch, tcntx.prev_context);
> >       return regs;
> >  }
>

[PATCH 4/9] lib: sbi: Simplify parameters of misaligned and access fault handlers

[Anup Patel]

The struct sbi_trap_context already has the information needed by
misaligned load/store and access fault load/store handlers so directly
pass struct sbi_trap_context pointer to these functions.

Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
 include/sbi/sbi_trap_ldst.h | 12 +++----
 lib/sbi/sbi_trap.c          |  8 ++---
 lib/sbi/sbi_trap_ldst.c     | 67 ++++++++++++++++++-------------------
 3 files changed, 40 insertions(+), 47 deletions(-)

diff --git a/include/sbi/sbi_trap_ldst.h b/include/sbi/sbi_trap_ldst.h
index 9cab4e4..8aee316 100644
--- a/include/sbi/sbi_trap_ldst.h
+++ b/include/sbi/sbi_trap_ldst.h
@@ -20,16 +20,12 @@ union sbi_ldst_data {
 	ulong data_ulong;
 };
 
-int sbi_misaligned_load_handler(struct sbi_trap_regs *regs,
-				const struct sbi_trap_info *orig_trap);
+int sbi_misaligned_load_handler(struct sbi_trap_context *tcntx);
 
-int sbi_misaligned_store_handler(struct sbi_trap_regs *regs,
-				 const struct sbi_trap_info *orig_trap);
+int sbi_misaligned_store_handler(struct sbi_trap_context *tcntx);
 
-int sbi_load_access_handler(struct sbi_trap_regs *regs,
-			    const struct sbi_trap_info *orig_trap);
+int sbi_load_access_handler(struct sbi_trap_context *tcntx);
 
-int sbi_store_access_handler(struct sbi_trap_regs *regs,
-			     const struct sbi_trap_info *orig_trap);
+int sbi_store_access_handler(struct sbi_trap_context *tcntx);
 
 #endif
diff --git a/lib/sbi/sbi_trap.c b/lib/sbi/sbi_trap.c
index dba267c..359749f 100644
--- a/lib/sbi/sbi_trap.c
+++ b/lib/sbi/sbi_trap.c
@@ -308,12 +308,12 @@ struct sbi_trap_regs *sbi_trap_handler(struct sbi_trap_regs *regs)
 		break;
 	case CAUSE_MISALIGNED_LOAD:
 		sbi_pmu_ctr_incr_fw(SBI_PMU_FW_MISALIGNED_LOAD);
-		rc  = sbi_misaligned_load_handler(regs, &trap);
+		rc  = sbi_misaligned_load_handler(&tcntx);
 		msg = "misaligned load handler failed";
 		break;
 	case CAUSE_MISALIGNED_STORE:
 		sbi_pmu_ctr_incr_fw(SBI_PMU_FW_MISALIGNED_STORE);
-		rc  = sbi_misaligned_store_handler(regs, &trap);
+		rc  = sbi_misaligned_store_handler(&tcntx);
 		msg = "misaligned store handler failed";
 		break;
 	case CAUSE_SUPERVISOR_ECALL:
@@ -323,12 +323,12 @@ struct sbi_trap_regs *sbi_trap_handler(struct sbi_trap_regs *regs)
 		break;
 	case CAUSE_LOAD_ACCESS:
 		sbi_pmu_ctr_incr_fw(SBI_PMU_FW_ACCESS_LOAD);
-		rc  = sbi_load_access_handler(regs, &trap);
+		rc  = sbi_load_access_handler(&tcntx);
 		msg = "load fault handler failed";
 		break;
 	case CAUSE_STORE_ACCESS:
 		sbi_pmu_ctr_incr_fw(SBI_PMU_FW_ACCESS_STORE);
-		rc  = sbi_store_access_handler(regs, &trap);
+		rc  = sbi_store_access_handler(&tcntx);
 		msg = "store fault handler failed";
 		break;
 	default:
diff --git a/lib/sbi/sbi_trap_ldst.c b/lib/sbi/sbi_trap_ldst.c
index d864ad1..2694e88 100644
--- a/lib/sbi/sbi_trap_ldst.c
+++ b/lib/sbi/sbi_trap_ldst.c
@@ -12,7 +12,6 @@
 #include <sbi/riscv_fp.h>
 #include <sbi/sbi_error.h>
 #include <sbi/sbi_trap_ldst.h>
-#include <sbi/sbi_pmu.h>
 #include <sbi/sbi_trap.h>
 #include <sbi/sbi_unpriv.h>
 #include <sbi/sbi_platform.h>
@@ -23,8 +22,7 @@
  * @return rlen=success, 0=success w/o regs modification, or negative error
  */
 typedef int (*sbi_trap_ld_emulator)(int rlen, union sbi_ldst_data *out_val,
-				    struct sbi_trap_regs *regs,
-				    const struct sbi_trap_info *orig_trap);
+				    struct sbi_trap_context *tcntx);
 
 /**
  * Store emulator callback:
@@ -32,8 +30,7 @@ typedef int (*sbi_trap_ld_emulator)(int rlen, union sbi_ldst_data *out_val,
  * @return wlen=success, 0=success w/o regs modification, or negative error
  */
 typedef int (*sbi_trap_st_emulator)(int wlen, union sbi_ldst_data in_val,
-				    struct sbi_trap_regs *regs,
-				    const struct sbi_trap_info *orig_trap);
+				    struct sbi_trap_context *tcntx);
 
 static ulong sbi_misaligned_tinst_fixup(ulong orig_tinst, ulong new_tinst,
 					ulong addr_offset)
@@ -47,10 +44,11 @@ static ulong sbi_misaligned_tinst_fixup(ulong orig_tinst, ulong new_tinst,
 		return orig_tinst | (addr_offset << SH_RS1);
 }
 
-static int sbi_trap_emulate_load(struct sbi_trap_regs *regs,
-				 const struct sbi_trap_info *orig_trap,
+static int sbi_trap_emulate_load(struct sbi_trap_context *tcntx,
 				 sbi_trap_ld_emulator emu)
 {
+	const struct sbi_trap_info *orig_trap = tcntx->trap;
+	struct sbi_trap_regs *regs = tcntx->regs;
 	ulong insn, insn_len;
 	union sbi_ldst_data val = { 0 };
 	struct sbi_trap_info uptrap;
@@ -151,8 +149,7 @@ static int sbi_trap_emulate_load(struct sbi_trap_regs *regs,
 		return sbi_trap_redirect(regs, orig_trap);
 	}
 
-	rc = emu(len, &val, regs, orig_trap);
-
+	rc = emu(len, &val, tcntx);
 	if (rc <= 0)
 		return rc;
 
@@ -170,10 +167,11 @@ static int sbi_trap_emulate_load(struct sbi_trap_regs *regs,
 	return 0;
 }
 
-static int sbi_trap_emulate_store(struct sbi_trap_regs *regs,
-				  const struct sbi_trap_info *orig_trap,
+static int sbi_trap_emulate_store(struct sbi_trap_context *tcntx,
 				  sbi_trap_st_emulator emu)
 {
+	const struct sbi_trap_info *orig_trap = tcntx->trap;
+	struct sbi_trap_regs *regs = tcntx->regs;
 	ulong insn, insn_len;
 	union sbi_ldst_data val;
 	struct sbi_trap_info uptrap;
@@ -256,8 +254,7 @@ static int sbi_trap_emulate_store(struct sbi_trap_regs *regs,
 		return sbi_trap_redirect(regs, orig_trap);
 	}
 
-	rc = emu(len, val, regs, orig_trap);
-
+	rc = emu(len, val, tcntx);
 	if (rc <= 0)
 		return rc;
 
@@ -267,9 +264,10 @@ static int sbi_trap_emulate_store(struct sbi_trap_regs *regs,
 }
 
 static int sbi_misaligned_ld_emulator(int rlen, union sbi_ldst_data *out_val,
-				      struct sbi_trap_regs *regs,
-				      const struct sbi_trap_info *orig_trap)
+				      struct sbi_trap_context *tcntx)
 {
+	const struct sbi_trap_info *orig_trap = tcntx->trap;
+	struct sbi_trap_regs *regs = tcntx->regs;
 	struct sbi_trap_info uptrap;
 	int i;
 
@@ -286,17 +284,16 @@ static int sbi_misaligned_ld_emulator(int rlen, union sbi_ldst_data *out_val,
 	return rlen;
 }
 
-int sbi_misaligned_load_handler(struct sbi_trap_regs *regs,
-				const struct sbi_trap_info *orig_trap)
+int sbi_misaligned_load_handler(struct sbi_trap_context *tcntx)
 {
-	return sbi_trap_emulate_load(regs, orig_trap,
-				     sbi_misaligned_ld_emulator);
+	return sbi_trap_emulate_load(tcntx, sbi_misaligned_ld_emulator);
 }
 
 static int sbi_misaligned_st_emulator(int wlen, union sbi_ldst_data in_val,
-				      struct sbi_trap_regs *regs,
-				      const struct sbi_trap_info *orig_trap)
+				      struct sbi_trap_context *tcntx)
 {
+	const struct sbi_trap_info *orig_trap = tcntx->trap;
+	struct sbi_trap_regs *regs = tcntx->regs;
 	struct sbi_trap_info uptrap;
 	int i;
 
@@ -313,17 +310,17 @@ static int sbi_misaligned_st_emulator(int wlen, union sbi_ldst_data in_val,
 	return wlen;
 }
 
-int sbi_misaligned_store_handler(struct sbi_trap_regs *regs,
-				 const struct sbi_trap_info *orig_trap)
+int sbi_misaligned_store_handler(struct sbi_trap_context *tcntx)
 {
-	return sbi_trap_emulate_store(regs, orig_trap,
-				      sbi_misaligned_st_emulator);
+	return sbi_trap_emulate_store(tcntx, sbi_misaligned_st_emulator);
 }
 
 static int sbi_ld_access_emulator(int rlen, union sbi_ldst_data *out_val,
-				  struct sbi_trap_regs *regs,
-				  const struct sbi_trap_info *orig_trap)
+				  struct sbi_trap_context *tcntx)
 {
+	const struct sbi_trap_info *orig_trap = tcntx->trap;
+	struct sbi_trap_regs *regs = tcntx->regs;
+
 	/* If fault came from M mode, just fail */
 	if (((regs->mstatus & MSTATUS_MPP) >> MSTATUS_MPP_SHIFT) == PRV_M)
 		return SBI_EINVAL;
@@ -336,16 +333,17 @@ static int sbi_ld_access_emulator(int rlen, union sbi_ldst_data *out_val,
 	return rlen;
 }
 
-int sbi_load_access_handler(struct sbi_trap_regs *regs,
-			    const struct sbi_trap_info *orig_trap)
+int sbi_load_access_handler(struct sbi_trap_context *tcntx)
 {
-	return sbi_trap_emulate_load(regs, orig_trap, sbi_ld_access_emulator);
+	return sbi_trap_emulate_load(tcntx, sbi_ld_access_emulator);
 }
 
 static int sbi_st_access_emulator(int wlen, union sbi_ldst_data in_val,
-				  struct sbi_trap_regs *regs,
-				  const struct sbi_trap_info *orig_trap)
+				  struct sbi_trap_context *tcntx)
 {
+	const struct sbi_trap_info *orig_trap = tcntx->trap;
+	struct sbi_trap_regs *regs = tcntx->regs;
+
 	/* If fault came from M mode, just fail */
 	if (((regs->mstatus & MSTATUS_MPP) >> MSTATUS_MPP_SHIFT) == PRV_M)
 		return SBI_EINVAL;
@@ -358,8 +356,7 @@ static int sbi_st_access_emulator(int wlen, union sbi_ldst_data in_val,
 	return wlen;
 }
 
-int sbi_store_access_handler(struct sbi_trap_regs *regs,
-			     const struct sbi_trap_info *orig_trap)
+int sbi_store_access_handler(struct sbi_trap_context *tcntx)
 {
-	return sbi_trap_emulate_store(regs, orig_trap, sbi_st_access_emulator);
+	return sbi_trap_emulate_store(tcntx, sbi_st_access_emulator);
 }
-- 
2.34.1

[PATCH 5/9] lib: sbi: Simplify parameters of sbi_illegal_insn_handler()

[Anup Patel]

The struct sbi_trap_context already has the information needed by
sbi_illegal_insn_handler() so directly pass struct sbi_trap_context
pointer to this function.

Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
 include/sbi/sbi_illegal_insn.h |  4 ++--
 lib/sbi/sbi_illegal_insn.c     | 14 ++++++++------
 lib/sbi/sbi_trap.c             |  2 +-
 3 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/include/sbi/sbi_illegal_insn.h b/include/sbi/sbi_illegal_insn.h
index 0397935..7be72ac 100644
--- a/include/sbi/sbi_illegal_insn.h
+++ b/include/sbi/sbi_illegal_insn.h
@@ -12,8 +12,8 @@
 
 #include <sbi/sbi_types.h>
 
-struct sbi_trap_regs;
+struct sbi_trap_context;
 
-int sbi_illegal_insn_handler(ulong insn, struct sbi_trap_regs *regs);
+int sbi_illegal_insn_handler(struct sbi_trap_context *tcntx);
 
 #endif
diff --git a/lib/sbi/sbi_illegal_insn.c b/lib/sbi/sbi_illegal_insn.c
index 2be4757..0c8ad1e 100644
--- a/lib/sbi/sbi_illegal_insn.c
+++ b/lib/sbi/sbi_illegal_insn.c
@@ -137,8 +137,9 @@ static const illegal_insn_func illegal_insn_table[32] = {
 	truly_illegal_insn  /* 31 */
 };
 
-int sbi_illegal_insn_handler(ulong insn, struct sbi_trap_regs *regs)
+int sbi_illegal_insn_handler(struct sbi_trap_context *tcntx)
 {
+	ulong mepc, insn = tcntx->trap->tval;
 	struct sbi_trap_info uptrap;
 
 	/*
@@ -154,14 +155,15 @@ int sbi_illegal_insn_handler(ulong insn, struct sbi_trap_regs *regs)
 
 	sbi_pmu_ctr_incr_fw(SBI_PMU_FW_ILLEGAL_INSN);
 	if (unlikely((insn & 3) != 3)) {
-		insn = sbi_get_insn(regs->mepc, &uptrap);
+		mepc = tcntx->regs->mepc;
+		insn = sbi_get_insn(mepc, &uptrap);
 		if (uptrap.cause) {
-			uptrap.epc = regs->mepc;
-			return sbi_trap_redirect(regs, &uptrap);
+			uptrap.epc = mepc;
+			return sbi_trap_redirect(tcntx->regs, &uptrap);
 		}
 		if ((insn & 3) != 3)
-			return truly_illegal_insn(insn, regs);
+			return truly_illegal_insn(insn, tcntx->regs);
 	}
 
-	return illegal_insn_table[(insn & 0x7c) >> 2](insn, regs);
+	return illegal_insn_table[(insn & 0x7c) >> 2](insn, tcntx->regs);
 }
diff --git a/lib/sbi/sbi_trap.c b/lib/sbi/sbi_trap.c
index 359749f..3d300c3 100644
--- a/lib/sbi/sbi_trap.c
+++ b/lib/sbi/sbi_trap.c
@@ -303,7 +303,7 @@ struct sbi_trap_regs *sbi_trap_handler(struct sbi_trap_regs *regs)
 
 	switch (mcause) {
 	case CAUSE_ILLEGAL_INSTRUCTION:
-		rc  = sbi_illegal_insn_handler(mtval, regs);
+		rc  = sbi_illegal_insn_handler(&tcntx);
 		msg = "illegal instruction handler failed";
 		break;
 	case CAUSE_MISALIGNED_LOAD:
-- 
2.34.1

[PATCH 6/9] lib: sbi: Remove regs paramter of sbi_irqchip_process()

[Anup Patel]

The irqchip handlers will typically not need pointer to trap registers
so remove regs paramter of sbi_irqchip_process().

Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
 include/sbi/sbi_irqchip.h |  5 ++---
 lib/sbi/sbi_irqchip.c     | 10 +++++-----
 lib/sbi/sbi_trap.c        |  4 ++--
 lib/utils/irqchip/imsic.c |  2 +-
 4 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/include/sbi/sbi_irqchip.h b/include/sbi/sbi_irqchip.h
index 6acc6e3..0ed02eb 100644
--- a/include/sbi/sbi_irqchip.h
+++ b/include/sbi/sbi_irqchip.h
@@ -13,7 +13,6 @@
 #include <sbi/sbi_types.h>
 
 struct sbi_scratch;
-struct sbi_trap_regs;
 
 /**
  * Set external interrupt handling function
@@ -23,7 +22,7 @@ struct sbi_trap_regs;
  *
  * @param fn function pointer for handling external irqs
  */
-void sbi_irqchip_set_irqfn(int (*fn)(struct sbi_trap_regs *regs));
+void sbi_irqchip_set_irqfn(int (*fn)(void));
 
 /**
  * Process external interrupts
@@ -33,7 +32,7 @@ void sbi_irqchip_set_irqfn(int (*fn)(struct sbi_trap_regs *regs));
  *
  * @param regs pointer for trap registers
  */
-int sbi_irqchip_process(struct sbi_trap_regs *regs);
+int sbi_irqchip_process(void);
 
 /** Initialize interrupt controllers */
 int sbi_irqchip_init(struct sbi_scratch *scratch, bool cold_boot);
diff --git a/lib/sbi/sbi_irqchip.c b/lib/sbi/sbi_irqchip.c
index 24128be..0ae604a 100644
--- a/lib/sbi/sbi_irqchip.c
+++ b/lib/sbi/sbi_irqchip.c
@@ -10,22 +10,22 @@
 #include <sbi/sbi_irqchip.h>
 #include <sbi/sbi_platform.h>
 
-static int default_irqfn(struct sbi_trap_regs *regs)
+static int default_irqfn(void)
 {
 	return SBI_ENODEV;
 }
 
-static int (*ext_irqfn)(struct sbi_trap_regs *regs) = default_irqfn;
+static int (*ext_irqfn)(void) = default_irqfn;
 
-void sbi_irqchip_set_irqfn(int (*fn)(struct sbi_trap_regs *regs))
+void sbi_irqchip_set_irqfn(int (*fn)(void))
 {
 	if (fn)
 		ext_irqfn = fn;
 }
 
-int sbi_irqchip_process(struct sbi_trap_regs *regs)
+int sbi_irqchip_process(void)
 {
-	return ext_irqfn(regs);
+	return ext_irqfn();
 }
 
 int sbi_irqchip_init(struct sbi_scratch *scratch, bool cold_boot)
diff --git a/lib/sbi/sbi_trap.c b/lib/sbi/sbi_trap.c
index 3d300c3..e6ef6d1 100644
--- a/lib/sbi/sbi_trap.c
+++ b/lib/sbi/sbi_trap.c
@@ -209,7 +209,7 @@ static int sbi_trap_nonaia_irq(struct sbi_trap_regs *regs, ulong mcause)
 		sbi_ipi_process();
 		break;
 	case IRQ_M_EXT:
-		return sbi_irqchip_process(regs);
+		return sbi_irqchip_process();
 	default:
 		return SBI_ENOENT;
 	}
@@ -232,7 +232,7 @@ static int sbi_trap_aia_irq(struct sbi_trap_regs *regs, ulong mcause)
 			sbi_ipi_process();
 			break;
 		case IRQ_M_EXT:
-			rc = sbi_irqchip_process(regs);
+			rc = sbi_irqchip_process();
 			if (rc)
 				return rc;
 			break;
diff --git a/lib/utils/irqchip/imsic.c b/lib/utils/irqchip/imsic.c
index 36ef66c..f2a35c6 100644
--- a/lib/utils/irqchip/imsic.c
+++ b/lib/utils/irqchip/imsic.c
@@ -140,7 +140,7 @@ int imsic_get_target_file(u32 hartid)
 	return imsic_get_hart_file(scratch);
 }
 
-static int imsic_external_irqfn(struct sbi_trap_regs *regs)
+static int imsic_external_irqfn(void)
 {
 	ulong mirq;
 
-- 
2.34.1

[PATCH 7/9] lib: sbi: Remove regs parameter from trap irq handling functions

[Anup Patel]

The trap irq handling functions no longer require regs parameter
so remove it.

Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
 lib/sbi/sbi_trap.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/lib/sbi/sbi_trap.c b/lib/sbi/sbi_trap.c
index e6ef6d1..a50e466 100644
--- a/lib/sbi/sbi_trap.c
+++ b/lib/sbi/sbi_trap.c
@@ -198,9 +198,9 @@ int sbi_trap_redirect(struct sbi_trap_regs *regs,
 	return 0;
 }
 
-static int sbi_trap_nonaia_irq(struct sbi_trap_regs *regs, ulong mcause)
+static int sbi_trap_nonaia_irq(unsigned long mcause)
 {
-	mcause &= ~(1UL << (__riscv_xlen - 1));
+	mcause &= ~BIT(__riscv_xlen - 1);
 	switch (mcause) {
 	case IRQ_M_TIMER:
 		sbi_timer_process();
@@ -217,7 +217,7 @@ static int sbi_trap_nonaia_irq(struct sbi_trap_regs *regs, ulong mcause)
 	return 0;
 }
 
-static int sbi_trap_aia_irq(struct sbi_trap_regs *regs, ulong mcause)
+static int sbi_trap_aia_irq(unsigned long mcause)
 {
 	int rc;
 	unsigned long mtopi;
@@ -291,12 +291,12 @@ struct sbi_trap_regs *sbi_trap_handler(struct sbi_trap_regs *regs)
 	/* Update trap context pointer */
 	sbi_trap_set_context(scratch, &tcntx);
 
-	if (mcause & (1UL << (__riscv_xlen - 1))) {
+	if (mcause & BIT(__riscv_xlen - 1)) {
 		if (sbi_hart_has_extension(sbi_scratch_thishart_ptr(),
 					   SBI_HART_EXT_SMAIA))
-			rc = sbi_trap_aia_irq(regs, mcause);
+			rc = sbi_trap_aia_irq(mcause);
 		else
-			rc = sbi_trap_nonaia_irq(regs, mcause);
+			rc = sbi_trap_nonaia_irq(mcause);
 		msg = "unhandled local interrupt";
 		goto trap_done;
 	}
-- 
2.34.1

[PATCH 7/9] lib: sbi: Remove regs parameter from trap irq handling functions

[Samuel Holland]

Hi Anup,

On 2024-03-11 11:09 AM, Anup Patel wrote:
> The trap irq handling functions no longer require regs parameter
> so remove it.
> 
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> ---
>  lib/sbi/sbi_trap.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/sbi/sbi_trap.c b/lib/sbi/sbi_trap.c
> index e6ef6d1..a50e466 100644
> --- a/lib/sbi/sbi_trap.c
> +++ b/lib/sbi/sbi_trap.c
> @@ -198,9 +198,9 @@ int sbi_trap_redirect(struct sbi_trap_regs *regs,
>  	return 0;
>  }
>  
> -static int sbi_trap_nonaia_irq(struct sbi_trap_regs *regs, ulong mcause)
> +static int sbi_trap_nonaia_irq(unsigned long mcause)
>  {
> -	mcause &= ~(1UL << (__riscv_xlen - 1));
> +	mcause &= ~BIT(__riscv_xlen - 1);
>  	switch (mcause) {
>  	case IRQ_M_TIMER:
>  		sbi_timer_process();
> @@ -217,7 +217,7 @@ static int sbi_trap_nonaia_irq(struct sbi_trap_regs *regs, ulong mcause)
>  	return 0;
>  }
>  
> -static int sbi_trap_aia_irq(struct sbi_trap_regs *regs, ulong mcause)
> +static int sbi_trap_aia_irq(unsigned long mcause)

The mcause parameter is not used in this function either.

Regards,
Samuel

>  {
>  	int rc;
>  	unsigned long mtopi;
> @@ -291,12 +291,12 @@ struct sbi_trap_regs *sbi_trap_handler(struct sbi_trap_regs *regs)
>  	/* Update trap context pointer */
>  	sbi_trap_set_context(scratch, &tcntx);
>  
> -	if (mcause & (1UL << (__riscv_xlen - 1))) {
> +	if (mcause & BIT(__riscv_xlen - 1)) {
>  		if (sbi_hart_has_extension(sbi_scratch_thishart_ptr(),
>  					   SBI_HART_EXT_SMAIA))
> -			rc = sbi_trap_aia_irq(regs, mcause);
> +			rc = sbi_trap_aia_irq(mcause);
>  		else
> -			rc = sbi_trap_nonaia_irq(regs, mcause);
> +			rc = sbi_trap_nonaia_irq(mcause);
>  		msg = "unhandled local interrupt";
>  		goto trap_done;
>  	}

[PATCH 7/9] lib: sbi: Remove regs parameter from trap irq handling functions

[Anup Patel]

On Tue, Mar 12, 2024 at 1:17?AM Samuel Holland
<samuel.holland@sifive.com> wrote:
>
> Hi Anup,
>
> On 2024-03-11 11:09 AM, Anup Patel wrote:
> > The trap irq handling functions no longer require regs parameter
> > so remove it.
> >
> > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > ---
> >  lib/sbi/sbi_trap.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/lib/sbi/sbi_trap.c b/lib/sbi/sbi_trap.c
> > index e6ef6d1..a50e466 100644
> > --- a/lib/sbi/sbi_trap.c
> > +++ b/lib/sbi/sbi_trap.c
> > @@ -198,9 +198,9 @@ int sbi_trap_redirect(struct sbi_trap_regs *regs,
> >       return 0;
> >  }
> >
> > -static int sbi_trap_nonaia_irq(struct sbi_trap_regs *regs, ulong mcause)
> > +static int sbi_trap_nonaia_irq(unsigned long mcause)
> >  {
> > -     mcause &= ~(1UL << (__riscv_xlen - 1));
> > +     mcause &= ~BIT(__riscv_xlen - 1);
> >       switch (mcause) {
> >       case IRQ_M_TIMER:
> >               sbi_timer_process();
> > @@ -217,7 +217,7 @@ static int sbi_trap_nonaia_irq(struct sbi_trap_regs *regs, ulong mcause)
> >       return 0;
> >  }
> >
> > -static int sbi_trap_aia_irq(struct sbi_trap_regs *regs, ulong mcause)
> > +static int sbi_trap_aia_irq(unsigned long mcause)
>
> The mcause parameter is not used in this function either.

Ahh, yes. I will drop it in the next revision.

Regards,
Anup

>
> Regards,
> Samuel
>
> >  {
> >       int rc;
> >       unsigned long mtopi;
> > @@ -291,12 +291,12 @@ struct sbi_trap_regs *sbi_trap_handler(struct sbi_trap_regs *regs)
> >       /* Update trap context pointer */
> >       sbi_trap_set_context(scratch, &tcntx);
> >
> > -     if (mcause & (1UL << (__riscv_xlen - 1))) {
> > +     if (mcause & BIT(__riscv_xlen - 1)) {
> >               if (sbi_hart_has_extension(sbi_scratch_thishart_ptr(),
> >                                          SBI_HART_EXT_SMAIA))
> > -                     rc = sbi_trap_aia_irq(regs, mcause);
> > +                     rc = sbi_trap_aia_irq(mcause);
> >               else
> > -                     rc = sbi_trap_nonaia_irq(regs, mcause);
> > +                     rc = sbi_trap_nonaia_irq(mcause);
> >               msg = "unhandled local interrupt";
> >               goto trap_done;
> >       }
>

[PATCH 8/9] lib: sbi: Pass trap context pointer to sbi_ecall_handler()

[Anup Patel]

To be consistent with other trap handlers, pass trap context pointer
to sbi_ecall_handler().

Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
 include/sbi/sbi_ecall.h | 4 ++--
 lib/sbi/sbi_ecall.c     | 3 ++-
 lib/sbi/sbi_trap.c      | 2 +-
 3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/include/sbi/sbi_ecall.h b/include/sbi/sbi_ecall.h
index 0bf42d1..0b35eff 100644
--- a/include/sbi/sbi_ecall.h
+++ b/include/sbi/sbi_ecall.h
@@ -18,7 +18,7 @@
 #define SBI_OPENSBI_IMPID		1
 
 struct sbi_trap_regs;
-struct sbi_trap_info;
+struct sbi_trap_context;
 
 struct sbi_ecall_return {
 	/* Return flag to skip register update */
@@ -87,7 +87,7 @@ int sbi_ecall_register_extension(struct sbi_ecall_extension *ext);
 
 void sbi_ecall_unregister_extension(struct sbi_ecall_extension *ext);
 
-int sbi_ecall_handler(struct sbi_trap_regs *regs);
+int sbi_ecall_handler(struct sbi_trap_context *tcntx);
 
 int sbi_ecall_init(void);
 
diff --git a/lib/sbi/sbi_ecall.c b/lib/sbi/sbi_ecall.c
index 631c5dd..845e79b 100644
--- a/lib/sbi/sbi_ecall.c
+++ b/lib/sbi/sbi_ecall.c
@@ -95,9 +95,10 @@ void sbi_ecall_unregister_extension(struct sbi_ecall_extension *ext)
 		sbi_list_del_init(&ext->head);
 }
 
-int sbi_ecall_handler(struct sbi_trap_regs *regs)
+int sbi_ecall_handler(struct sbi_trap_context *tcntx)
 {
 	int ret = 0;
+	struct sbi_trap_regs *regs = tcntx->regs;
 	struct sbi_ecall_extension *ext;
 	unsigned long extension_id = regs->a7;
 	unsigned long func_id = regs->a6;
diff --git a/lib/sbi/sbi_trap.c b/lib/sbi/sbi_trap.c
index a50e466..b0fa5fd 100644
--- a/lib/sbi/sbi_trap.c
+++ b/lib/sbi/sbi_trap.c
@@ -318,7 +318,7 @@ struct sbi_trap_regs *sbi_trap_handler(struct sbi_trap_regs *regs)
 		break;
 	case CAUSE_SUPERVISOR_ECALL:
 	case CAUSE_MACHINE_ECALL:
-		rc  = sbi_ecall_handler(regs);
+		rc  = sbi_ecall_handler(&tcntx);
 		msg = "ecall handler failed";
 		break;
 	case CAUSE_LOAD_ACCESS:
-- 
2.34.1

[PATCH 9/9] lib: sbi: Extend sbi_trap_error() to dump state in a nested trap

[Anup Patel]

The sbi_trap_error() should dump state of all in-flight traps upon
failure in a nested trap so extend it accordingly.

Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
 lib/sbi/sbi_trap.c | 104 ++++++++++++++++++++++++++-------------------
 1 file changed, 60 insertions(+), 44 deletions(-)

diff --git a/lib/sbi/sbi_trap.c b/lib/sbi/sbi_trap.c
index b0fa5fd..1c9cf1f 100644
--- a/lib/sbi/sbi_trap.c
+++ b/lib/sbi/sbi_trap.c
@@ -23,54 +23,70 @@
 #include <sbi/sbi_timer.h>
 #include <sbi/sbi_trap.h>
 
-static void __noreturn sbi_trap_error(const char *msg, int rc,
-				      ulong mcause, ulong mtval, ulong mtval2,
-				      ulong mtinst, struct sbi_trap_regs *regs)
+static void sbi_trap_error_one(struct sbi_trap_context *tcntx,
+			       const char *prefix, u32 hartid, u32 depth)
 {
-	u32 hartid = current_hartid();
+	const struct sbi_trap_info *trap = tcntx->trap;
+	struct sbi_trap_regs *regs = tcntx->regs;
 
-	sbi_printf("%s: hart%d: %s (error %d)\n", __func__, hartid, msg, rc);
-	sbi_printf("%s: hart%d: mcause=0x%" PRILX " mtval=0x%" PRILX "\n",
-		   __func__, hartid, mcause, mtval);
+	sbi_printf("\n");
+	sbi_printf("%s: hart%d: trap%d: mcause=0x%" PRILX " mtval=0x%" PRILX "\n",
+		   prefix, hartid, depth, trap->cause, trap->tval);
 	if (misa_extension('H')) {
-		sbi_printf("%s: hart%d: mtval2=0x%" PRILX
+		sbi_printf("%s: hart%d: trap%d: mtval2=0x%" PRILX
 			   " mtinst=0x%" PRILX "\n",
-			   __func__, hartid, mtval2, mtinst);
+			   prefix, hartid, depth, trap->tval2, trap->tinst);
 	}
-	sbi_printf("%s: hart%d: mepc=0x%" PRILX " mstatus=0x%" PRILX "\n",
-		   __func__, hartid, regs->mepc, regs->mstatus);
-	sbi_printf("%s: hart%d: %s=0x%" PRILX " %s=0x%" PRILX "\n", __func__,
-		   hartid, "ra", regs->ra, "sp", regs->sp);
-	sbi_printf("%s: hart%d: %s=0x%" PRILX " %s=0x%" PRILX "\n", __func__,
-		   hartid, "gp", regs->gp, "tp", regs->tp);
-	sbi_printf("%s: hart%d: %s=0x%" PRILX " %s=0x%" PRILX "\n", __func__,
-		   hartid, "s0", regs->s0, "s1", regs->s1);
-	sbi_printf("%s: hart%d: %s=0x%" PRILX " %s=0x%" PRILX "\n", __func__,
-		   hartid, "a0", regs->a0, "a1", regs->a1);
-	sbi_printf("%s: hart%d: %s=0x%" PRILX " %s=0x%" PRILX "\n", __func__,
-		   hartid, "a2", regs->a2, "a3", regs->a3);
-	sbi_printf("%s: hart%d: %s=0x%" PRILX " %s=0x%" PRILX "\n", __func__,
-		   hartid, "a4", regs->a4, "a5", regs->a5);
-	sbi_printf("%s: hart%d: %s=0x%" PRILX " %s=0x%" PRILX "\n", __func__,
-		   hartid, "a6", regs->a6, "a7", regs->a7);
-	sbi_printf("%s: hart%d: %s=0x%" PRILX " %s=0x%" PRILX "\n", __func__,
-		   hartid, "s2", regs->s2, "s3", regs->s3);
-	sbi_printf("%s: hart%d: %s=0x%" PRILX " %s=0x%" PRILX "\n", __func__,
-		   hartid, "s4", regs->s4, "s5", regs->s5);
-	sbi_printf("%s: hart%d: %s=0x%" PRILX " %s=0x%" PRILX "\n", __func__,
-		   hartid, "s6", regs->s6, "s7", regs->s7);
-	sbi_printf("%s: hart%d: %s=0x%" PRILX " %s=0x%" PRILX "\n", __func__,
-		   hartid, "s8", regs->s8, "s9", regs->s9);
-	sbi_printf("%s: hart%d: %s=0x%" PRILX " %s=0x%" PRILX "\n", __func__,
-		   hartid, "s10", regs->s10, "s11", regs->s11);
-	sbi_printf("%s: hart%d: %s=0x%" PRILX " %s=0x%" PRILX "\n", __func__,
-		   hartid, "t0", regs->t0, "t1", regs->t1);
-	sbi_printf("%s: hart%d: %s=0x%" PRILX " %s=0x%" PRILX "\n", __func__,
-		   hartid, "t2", regs->t2, "t3", regs->t3);
-	sbi_printf("%s: hart%d: %s=0x%" PRILX " %s=0x%" PRILX "\n", __func__,
-		   hartid, "t4", regs->t4, "t5", regs->t5);
-	sbi_printf("%s: hart%d: %s=0x%" PRILX "\n", __func__, hartid, "t6",
-		   regs->t6);
+	sbi_printf("%s: hart%d: trap%d: mepc=0x%" PRILX " mstatus=0x%" PRILX "\n",
+		   prefix, hartid, depth, regs->mepc, regs->mstatus);
+	sbi_printf("%s: hart%d: trap%d: %s=0x%" PRILX " %s=0x%" PRILX "\n", prefix,
+		   hartid, depth, "ra", regs->ra, "sp", regs->sp);
+	sbi_printf("%s: hart%d: trap%d: %s=0x%" PRILX " %s=0x%" PRILX "\n", prefix,
+		   hartid, depth, "gp", regs->gp, "tp", regs->tp);
+	sbi_printf("%s: hart%d: trap%d: %s=0x%" PRILX " %s=0x%" PRILX "\n", prefix,
+		   hartid, depth, "s0", regs->s0, "s1", regs->s1);
+	sbi_printf("%s: hart%d: trap%d: %s=0x%" PRILX " %s=0x%" PRILX "\n", prefix,
+		   hartid, depth, "a0", regs->a0, "a1", regs->a1);
+	sbi_printf("%s: hart%d: trap%d: %s=0x%" PRILX " %s=0x%" PRILX "\n", prefix,
+		   hartid, depth, "a2", regs->a2, "a3", regs->a3);
+	sbi_printf("%s: hart%d: trap%d: %s=0x%" PRILX " %s=0x%" PRILX "\n", prefix,
+		   hartid, depth, "a4", regs->a4, "a5", regs->a5);
+	sbi_printf("%s: hart%d: trap%d: %s=0x%" PRILX " %s=0x%" PRILX "\n", prefix,
+		   hartid, depth, "a6", regs->a6, "a7", regs->a7);
+	sbi_printf("%s: hart%d: trap%d: %s=0x%" PRILX " %s=0x%" PRILX "\n", prefix,
+		   hartid, depth, "s2", regs->s2, "s3", regs->s3);
+	sbi_printf("%s: hart%d: trap%d: %s=0x%" PRILX " %s=0x%" PRILX "\n", prefix,
+		   hartid, depth, "s4", regs->s4, "s5", regs->s5);
+	sbi_printf("%s: hart%d: trap%d: %s=0x%" PRILX " %s=0x%" PRILX "\n", prefix,
+		   hartid, depth, "s6", regs->s6, "s7", regs->s7);
+	sbi_printf("%s: hart%d: trap%d: %s=0x%" PRILX " %s=0x%" PRILX "\n", prefix,
+		   hartid, depth, "s8", regs->s8, "s9", regs->s9);
+	sbi_printf("%s: hart%d: trap%d: %s=0x%" PRILX " %s=0x%" PRILX "\n", prefix,
+		   hartid, depth, "s10", regs->s10, "s11", regs->s11);
+	sbi_printf("%s: hart%d: trap%d: %s=0x%" PRILX " %s=0x%" PRILX "\n", prefix,
+		   hartid, depth, "t0", regs->t0, "t1", regs->t1);
+	sbi_printf("%s: hart%d: trap%d: %s=0x%" PRILX " %s=0x%" PRILX "\n", prefix,
+		   hartid, depth, "t2", regs->t2, "t3", regs->t3);
+	sbi_printf("%s: hart%d: trap%d: %s=0x%" PRILX " %s=0x%" PRILX "\n", prefix,
+		   hartid, depth, "t4", regs->t4, "t5", regs->t5);
+	sbi_printf("%s: hart%d: trap%d: %s=0x%" PRILX "\n", prefix,
+		   hartid, depth, "t6", regs->t6);
+}
+
+static void __noreturn sbi_trap_error(const char *msg, int rc,
+				      struct sbi_trap_context *tcntx)
+{
+	u32 depth = 0, hartid = current_hartid();
+	struct sbi_trap_context *tc;
+
+	for (tc = tcntx; tc; tc = tc->prev_context)
+		depth++;
+
+	sbi_printf("\n");
+	sbi_printf("%s: hart%d: trap%d: %s (error %d)\n", __func__,
+		   hartid, depth - 1, msg, rc);
+	for (tc = tcntx; tc; tc = tc->prev_context)
+		sbi_trap_error_one(tc, __func__, hartid, --depth);
 
 	sbi_hart_hang();
 }
@@ -340,7 +356,7 @@ struct sbi_trap_regs *sbi_trap_handler(struct sbi_trap_regs *regs)
 
 trap_done:
 	if (rc)
-		sbi_trap_error(msg, rc, mcause, mtval, mtval2, mtinst, regs);
+		sbi_trap_error(msg, rc, &tcntx);
 	sbi_trap_set_context(scratch, tcntx.prev_context);
 	return regs;
 }
-- 
2.34.1

[PATCH 0/9] Improve trap handling for nested traps

[Bo Gan]

On 3/11/24 9:09 AM, Anup Patel wrote:
> Nested traps will be a common when dealing with RAS error traps so
> this series improves trap handling for nested traps by introducing
> a linked-list based trap context chain.
> 
> These patches can also be found the trap_handling_imp_v1 branch at
> https://github.com/avpatel/opensbi.git
> 
> Anup Patel (9):
>    lib: sbi: Remove sbi_trap_exit() and related code
>    include: sbi: Add trap_context pointer in struct sbi_scratch
>    lib: sbi: Introduce trap context
>    lib: sbi: Simplify parameters of misaligned and access fault handlers
>    lib: sbi: Simplify parameters of sbi_illegal_insn_handler()
>    lib: sbi: Remove regs paramter of sbi_irqchip_process()
>    lib: sbi: Remove regs parameter from trap irq handling functions
>    lib: sbi: Pass trap context pointer to sbi_ecall_handler()
>    lib: sbi: Extend sbi_trap_error() to dump state in a nested trap
> 
>   firmware/fw_base.S             |  14 +--
>   include/sbi/sbi_ecall.h        |   4 +-
>   include/sbi/sbi_illegal_insn.h |   4 +-
>   include/sbi/sbi_irqchip.h      |   5 +-
>   include/sbi/sbi_scratch.h      |  14 +--
>   include/sbi/sbi_trap.h         |  24 ++++-
>   include/sbi/sbi_trap_ldst.h    |  12 +--
>   lib/sbi/sbi_ecall.c            |   3 +-
>   lib/sbi/sbi_illegal_insn.c     |  14 +--
>   lib/sbi/sbi_irqchip.c          |  10 +-
>   lib/sbi/sbi_trap.c             | 186 +++++++++++++++++----------------
>   lib/sbi/sbi_trap_ldst.c        |  67 ++++++------
>   lib/utils/irqchip/imsic.c      |   2 +-
>   13 files changed, 185 insertions(+), 174 deletions(-)
> 

Hi Anup,

Can you help providing an example for nested traps and the RAS extension?
The closest spec I can find (https://github.com/riscv/riscv-ssrastraps) is
still empty. I'm wondering where I can find the related documentation.

Bo

[PATCH 0/9] Improve trap handling for nested traps

[Anup Patel]

On Tue, Mar 12, 2024 at 6:31?AM Bo Gan <ganboing@gmail.com> wrote:
>
> On 3/11/24 9:09 AM, Anup Patel wrote:
> > Nested traps will be a common when dealing with RAS error traps so
> > this series improves trap handling for nested traps by introducing
> > a linked-list based trap context chain.
> >
> > These patches can also be found the trap_handling_imp_v1 branch at
> > https://github.com/avpatel/opensbi.git
> >
> > Anup Patel (9):
> >    lib: sbi: Remove sbi_trap_exit() and related code
> >    include: sbi: Add trap_context pointer in struct sbi_scratch
> >    lib: sbi: Introduce trap context
> >    lib: sbi: Simplify parameters of misaligned and access fault handlers
> >    lib: sbi: Simplify parameters of sbi_illegal_insn_handler()
> >    lib: sbi: Remove regs paramter of sbi_irqchip_process()
> >    lib: sbi: Remove regs parameter from trap irq handling functions
> >    lib: sbi: Pass trap context pointer to sbi_ecall_handler()
> >    lib: sbi: Extend sbi_trap_error() to dump state in a nested trap
> >
> >   firmware/fw_base.S             |  14 +--
> >   include/sbi/sbi_ecall.h        |   4 +-
> >   include/sbi/sbi_illegal_insn.h |   4 +-
> >   include/sbi/sbi_irqchip.h      |   5 +-
> >   include/sbi/sbi_scratch.h      |  14 +--
> >   include/sbi/sbi_trap.h         |  24 ++++-
> >   include/sbi/sbi_trap_ldst.h    |  12 +--
> >   lib/sbi/sbi_ecall.c            |   3 +-
> >   lib/sbi/sbi_illegal_insn.c     |  14 +--
> >   lib/sbi/sbi_irqchip.c          |  10 +-
> >   lib/sbi/sbi_trap.c             | 186 +++++++++++++++++----------------
> >   lib/sbi/sbi_trap_ldst.c        |  67 ++++++------
> >   lib/utils/irqchip/imsic.c      |   2 +-
> >   13 files changed, 185 insertions(+), 174 deletions(-)
> >
>
> Hi Anup,
>
> Can you help providing an example for nested traps and the RAS extension?
> The closest spec I can find (https://github.com/riscv/riscv-ssrastraps) is
> still empty. I'm wondering where I can find the related documentation.

Refer, "3.1.15. Machine Cause Register" of the draft Priv v1.13 specification.
https://github.com/riscv/riscv-isa-manual/releases/download/riscv-isa-release-157641b-2024-03-12/priv-isa-asciidoc.pdf

Regards,
Anup

[PATCH 0/9] Improve trap handling for nested traps

[Bo Gan]

On 3/11/24 8:43 PM, Anup Patel wrote:
> On Tue, Mar 12, 2024 at 6:31?AM Bo Gan <ganboing@gmail.com> wrote:
>>
>> On 3/11/24 9:09 AM, Anup Patel wrote:
>>> Nested traps will be a common when dealing with RAS error traps so
>>> this series improves trap handling for nested traps by introducing
>>> a linked-list based trap context chain.
>>>
>>> These patches can also be found the trap_handling_imp_v1 branch at
>>> https://github.com/avpatel/opensbi.git
>>>
>>> Anup Patel (9):
>>>     lib: sbi: Remove sbi_trap_exit() and related code
>>>     include: sbi: Add trap_context pointer in struct sbi_scratch
>>>     lib: sbi: Introduce trap context
>>>     lib: sbi: Simplify parameters of misaligned and access fault handlers
>>>     lib: sbi: Simplify parameters of sbi_illegal_insn_handler()
>>>     lib: sbi: Remove regs paramter of sbi_irqchip_process()
>>>     lib: sbi: Remove regs parameter from trap irq handling functions
>>>     lib: sbi: Pass trap context pointer to sbi_ecall_handler()
>>>     lib: sbi: Extend sbi_trap_error() to dump state in a nested trap
>>>
>>>    firmware/fw_base.S             |  14 +--
>>>    include/sbi/sbi_ecall.h        |   4 +-
>>>    include/sbi/sbi_illegal_insn.h |   4 +-
>>>    include/sbi/sbi_irqchip.h      |   5 +-
>>>    include/sbi/sbi_scratch.h      |  14 +--
>>>    include/sbi/sbi_trap.h         |  24 ++++-
>>>    include/sbi/sbi_trap_ldst.h    |  12 +--
>>>    lib/sbi/sbi_ecall.c            |   3 +-
>>>    lib/sbi/sbi_illegal_insn.c     |  14 +--
>>>    lib/sbi/sbi_irqchip.c          |  10 +-
>>>    lib/sbi/sbi_trap.c             | 186 +++++++++++++++++----------------
>>>    lib/sbi/sbi_trap_ldst.c        |  67 ++++++------
>>>    lib/utils/irqchip/imsic.c      |   2 +-
>>>    13 files changed, 185 insertions(+), 174 deletions(-)
>>>
>>
>> Hi Anup,
>>
>> Can you help providing an example for nested traps and the RAS extension?
>> The closest spec I can find (https://github.com/riscv/riscv-ssrastraps) is
>> still empty. I'm wondering where I can find the related documentation.
> 
> Refer, "3.1.15. Machine Cause Register" of the draft Priv v1.13 specification.
> https://github.com/riscv/riscv-isa-manual/releases/download/riscv-isa-release-157641b-2024-03-12/priv-isa-asciidoc.pdf
> 
> Regards,
> Anup
> 


Hi Anup, Thanks for the pointer. I know what nested trap is. My concern is when do
we expect such trap to happen? Does it mean for every memory access in M mode, it
might trigger a RAS fault, causing a nested trap? Or even the fault can be delivered
asynchronously? If that's the case, how can we even safely handle nested traps? E.g.,
The entry of _trap_handler doesn't look like reentrant-safe to me. Perhaps you plan
to enhance it in later patches?

Bo

[PATCH 0/9] Improve trap handling for nested traps

[Anup Patel]

On Tue, Mar 12, 2024 at 10:03?AM Bo Gan <ganboing@gmail.com> wrote:
>
> On 3/11/24 8:43 PM, Anup Patel wrote:
> > On Tue, Mar 12, 2024 at 6:31?AM Bo Gan <ganboing@gmail.com> wrote:
> >>
> >> On 3/11/24 9:09 AM, Anup Patel wrote:
> >>> Nested traps will be a common when dealing with RAS error traps so
> >>> this series improves trap handling for nested traps by introducing
> >>> a linked-list based trap context chain.
> >>>
> >>> These patches can also be found the trap_handling_imp_v1 branch at
> >>> https://github.com/avpatel/opensbi.git
> >>>
> >>> Anup Patel (9):
> >>>     lib: sbi: Remove sbi_trap_exit() and related code
> >>>     include: sbi: Add trap_context pointer in struct sbi_scratch
> >>>     lib: sbi: Introduce trap context
> >>>     lib: sbi: Simplify parameters of misaligned and access fault handlers
> >>>     lib: sbi: Simplify parameters of sbi_illegal_insn_handler()
> >>>     lib: sbi: Remove regs paramter of sbi_irqchip_process()
> >>>     lib: sbi: Remove regs parameter from trap irq handling functions
> >>>     lib: sbi: Pass trap context pointer to sbi_ecall_handler()
> >>>     lib: sbi: Extend sbi_trap_error() to dump state in a nested trap
> >>>
> >>>    firmware/fw_base.S             |  14 +--
> >>>    include/sbi/sbi_ecall.h        |   4 +-
> >>>    include/sbi/sbi_illegal_insn.h |   4 +-
> >>>    include/sbi/sbi_irqchip.h      |   5 +-
> >>>    include/sbi/sbi_scratch.h      |  14 +--
> >>>    include/sbi/sbi_trap.h         |  24 ++++-
> >>>    include/sbi/sbi_trap_ldst.h    |  12 +--
> >>>    lib/sbi/sbi_ecall.c            |   3 +-
> >>>    lib/sbi/sbi_illegal_insn.c     |  14 +--
> >>>    lib/sbi/sbi_irqchip.c          |  10 +-
> >>>    lib/sbi/sbi_trap.c             | 186 +++++++++++++++++----------------
> >>>    lib/sbi/sbi_trap_ldst.c        |  67 ++++++------
> >>>    lib/utils/irqchip/imsic.c      |   2 +-
> >>>    13 files changed, 185 insertions(+), 174 deletions(-)
> >>>
> >>
> >> Hi Anup,
> >>
> >> Can you help providing an example for nested traps and the RAS extension?
> >> The closest spec I can find (https://github.com/riscv/riscv-ssrastraps) is
> >> still empty. I'm wondering where I can find the related documentation.
> >
> > Refer, "3.1.15. Machine Cause Register" of the draft Priv v1.13 specification.
> > https://github.com/riscv/riscv-isa-manual/releases/download/riscv-isa-release-157641b-2024-03-12/priv-isa-asciidoc.pdf
> >
> > Regards,
> > Anup
> >
>
>
> Hi Anup, Thanks for the pointer. I know what nested trap is. My concern is when do
> we expect such trap to happen? Does it mean for every memory access in M mode, it
> might trigger a RAS fault, causing a nested trap? Or even the fault can be delivered
> asynchronously?

RAS error can occur at any time. A synchronous RAS errors will be taken
as an exception (mcause = 19) whereas asynchronous RAS errors will be
taken as RAS local interrupt or RAS external interrupt (through interrupt
controller).

Only RAS synchronous errors can cause nested trap.

> If that's the case, how can we even safely handle nested traps? E.g.,
> The entry of _trap_handler doesn't look like reentrant-safe to me. Perhaps you plan
> to enhance it in later patches.
>

The _trap_handler pushes the register state on stack. It already takes care
of nesting by continuing the same SP if the trap was taken while in M-mode.

If _trap_handler() did not support nesting then I would not be able to test
this series.

Refer, TRAP_SAVE_AND_SETUP_SP_T0() in fw_base.S

Regards,
Anup

[PATCH 0/9] Improve trap handling for nested traps

[Bo Gan]

On 3/11/24 10:18 PM, Anup Patel wrote:
> On Tue, Mar 12, 2024 at 10:03?AM Bo Gan <ganboing@gmail.com> wrote:
>>
>> On 3/11/24 8:43 PM, Anup Patel wrote:
>>> On Tue, Mar 12, 2024 at 6:31?AM Bo Gan <ganboing@gmail.com> wrote:
>>>>
>>>> On 3/11/24 9:09 AM, Anup Patel wrote:
>>>>> Nested traps will be a common when dealing with RAS error traps so
>>>>> this series improves trap handling for nested traps by introducing
>>>>> a linked-list based trap context chain.
>>>>>
>>>>> These patches can also be found the trap_handling_imp_v1 branch at
>>>>> https://github.com/avpatel/opensbi.git
>>>>>
>>>>> Anup Patel (9):
>>>>>      lib: sbi: Remove sbi_trap_exit() and related code
>>>>>      include: sbi: Add trap_context pointer in struct sbi_scratch
>>>>>      lib: sbi: Introduce trap context
>>>>>      lib: sbi: Simplify parameters of misaligned and access fault handlers
>>>>>      lib: sbi: Simplify parameters of sbi_illegal_insn_handler()
>>>>>      lib: sbi: Remove regs paramter of sbi_irqchip_process()
>>>>>      lib: sbi: Remove regs parameter from trap irq handling functions
>>>>>      lib: sbi: Pass trap context pointer to sbi_ecall_handler()
>>>>>      lib: sbi: Extend sbi_trap_error() to dump state in a nested trap
>>>>>
>>>>>     firmware/fw_base.S             |  14 +--
>>>>>     include/sbi/sbi_ecall.h        |   4 +-
>>>>>     include/sbi/sbi_illegal_insn.h |   4 +-
>>>>>     include/sbi/sbi_irqchip.h      |   5 +-
>>>>>     include/sbi/sbi_scratch.h      |  14 +--
>>>>>     include/sbi/sbi_trap.h         |  24 ++++-
>>>>>     include/sbi/sbi_trap_ldst.h    |  12 +--
>>>>>     lib/sbi/sbi_ecall.c            |   3 +-
>>>>>     lib/sbi/sbi_illegal_insn.c     |  14 +--
>>>>>     lib/sbi/sbi_irqchip.c          |  10 +-
>>>>>     lib/sbi/sbi_trap.c             | 186 +++++++++++++++++----------------
>>>>>     lib/sbi/sbi_trap_ldst.c        |  67 ++++++------
>>>>>     lib/utils/irqchip/imsic.c      |   2 +-
>>>>>     13 files changed, 185 insertions(+), 174 deletions(-)
>>>>>
>>>>
>>>> Hi Anup,
>>>>
>>>> Can you help providing an example for nested traps and the RAS extension?
>>>> The closest spec I can find (https://github.com/riscv/riscv-ssrastraps) is
>>>> still empty. I'm wondering where I can find the related documentation.
>>>
>>> Refer, "3.1.15. Machine Cause Register" of the draft Priv v1.13 specification.
>>> https://github.com/riscv/riscv-isa-manual/releases/download/riscv-isa-release-157641b-2024-03-12/priv-isa-asciidoc.pdf
>>>
>>> Regards,
>>> Anup
>>>
>>
>>
>> Hi Anup, Thanks for the pointer. I know what nested trap is. My concern is when do
>> we expect such trap to happen? Does it mean for every memory access in M mode, it
>> might trigger a RAS fault, causing a nested trap? Or even the fault can be delivered
>> asynchronously?
> 
> RAS error can occur at any time. A synchronous RAS errors will be taken
> as an exception (mcause = 19) whereas asynchronous RAS errors will be
> taken as RAS local interrupt or RAS external interrupt (through interrupt
> controller).
> 
> Only RAS synchronous errors can cause nested trap.
> 
>> If that's the case, how can we even safely handle nested traps? E.g.,
>> The entry of _trap_handler doesn't look like reentrant-safe to me. Perhaps you plan
>> to enhance it in later patches.
>>
> 
> The _trap_handler pushes the register state on stack. It already takes care
> of nesting by continuing the same SP if the trap was taken while in M-mode.
> 
> If _trap_handler() did not support nesting then I would not be able to test
> this series.
> 
> Refer, TRAP_SAVE_AND_SETUP_SP_T0() in fw_base.S
> 
> Regards,
> Anup
> 

I'm looking at the exact code you are pointing.

> .macro TRAP_SAVE_AND_SETUP_SP_T0
> 	/* Swap TP and MSCRATCH */
> 	csrrw	tp, CSR_MSCRATCH, tp

===> what if RAS exception gets triggered here?

> 	/* Save T0 in scratch space */
> 	REG_S	t0, SBI_SCRATCH_TMP0_OFFSET(tp)
>
  
===> Or here?

> 	/*
> 	 * Set T0 to appropriate exception stack
> 	 *
> 	 * Came_From_M_Mode = ((MSTATUS.MPP < PRV_M) ? 1 : 0) - 1;
> 	 * Exception_Stack = TP ^ (Came_From_M_Mode & (SP ^ TP))
> 	 *
> 	 * Came_From_M_Mode = 0    ==>    Exception_Stack = TP
> 	 * Came_From_M_Mode = -1   ==>    Exception_Stack = SP
> 	 */

===> Or somewhere below?

> 	csrr	t0, CSR_MSTATUS
> 	srl	t0, t0, MSTATUS_MPP_SHIFT
> 	and	t0, t0, PRV_M
.....

We only have one spare register CSR_MSCRATCH to work with before we
can work on stack. How are we suppose to do the nested trap safely?
Am i missing something?

Bo

[PATCH 0/9] Improve trap handling for nested traps

[Anup Patel]

On Tue, Mar 12, 2024 at 11:11?AM Bo Gan <ganboing@gmail.com> wrote:
>
> On 3/11/24 10:18 PM, Anup Patel wrote:
> > On Tue, Mar 12, 2024 at 10:03?AM Bo Gan <ganboing@gmail.com> wrote:
> >>
> >> On 3/11/24 8:43 PM, Anup Patel wrote:
> >>> On Tue, Mar 12, 2024 at 6:31?AM Bo Gan <ganboing@gmail.com> wrote:
> >>>>
> >>>> On 3/11/24 9:09 AM, Anup Patel wrote:
> >>>>> Nested traps will be a common when dealing with RAS error traps so
> >>>>> this series improves trap handling for nested traps by introducing
> >>>>> a linked-list based trap context chain.
> >>>>>
> >>>>> These patches can also be found the trap_handling_imp_v1 branch at
> >>>>> https://github.com/avpatel/opensbi.git
> >>>>>
> >>>>> Anup Patel (9):
> >>>>>      lib: sbi: Remove sbi_trap_exit() and related code
> >>>>>      include: sbi: Add trap_context pointer in struct sbi_scratch
> >>>>>      lib: sbi: Introduce trap context
> >>>>>      lib: sbi: Simplify parameters of misaligned and access fault handlers
> >>>>>      lib: sbi: Simplify parameters of sbi_illegal_insn_handler()
> >>>>>      lib: sbi: Remove regs paramter of sbi_irqchip_process()
> >>>>>      lib: sbi: Remove regs parameter from trap irq handling functions
> >>>>>      lib: sbi: Pass trap context pointer to sbi_ecall_handler()
> >>>>>      lib: sbi: Extend sbi_trap_error() to dump state in a nested trap
> >>>>>
> >>>>>     firmware/fw_base.S             |  14 +--
> >>>>>     include/sbi/sbi_ecall.h        |   4 +-
> >>>>>     include/sbi/sbi_illegal_insn.h |   4 +-
> >>>>>     include/sbi/sbi_irqchip.h      |   5 +-
> >>>>>     include/sbi/sbi_scratch.h      |  14 +--
> >>>>>     include/sbi/sbi_trap.h         |  24 ++++-
> >>>>>     include/sbi/sbi_trap_ldst.h    |  12 +--
> >>>>>     lib/sbi/sbi_ecall.c            |   3 +-
> >>>>>     lib/sbi/sbi_illegal_insn.c     |  14 +--
> >>>>>     lib/sbi/sbi_irqchip.c          |  10 +-
> >>>>>     lib/sbi/sbi_trap.c             | 186 +++++++++++++++++----------------
> >>>>>     lib/sbi/sbi_trap_ldst.c        |  67 ++++++------
> >>>>>     lib/utils/irqchip/imsic.c      |   2 +-
> >>>>>     13 files changed, 185 insertions(+), 174 deletions(-)
> >>>>>
> >>>>
> >>>> Hi Anup,
> >>>>
> >>>> Can you help providing an example for nested traps and the RAS extension?
> >>>> The closest spec I can find (https://github.com/riscv/riscv-ssrastraps) is
> >>>> still empty. I'm wondering where I can find the related documentation.
> >>>
> >>> Refer, "3.1.15. Machine Cause Register" of the draft Priv v1.13 specification.
> >>> https://github.com/riscv/riscv-isa-manual/releases/download/riscv-isa-release-157641b-2024-03-12/priv-isa-asciidoc.pdf
> >>>
> >>> Regards,
> >>> Anup
> >>>
> >>
> >>
> >> Hi Anup, Thanks for the pointer. I know what nested trap is. My concern is when do
> >> we expect such trap to happen? Does it mean for every memory access in M mode, it
> >> might trigger a RAS fault, causing a nested trap? Or even the fault can be delivered
> >> asynchronously?
> >
> > RAS error can occur at any time. A synchronous RAS errors will be taken
> > as an exception (mcause = 19) whereas asynchronous RAS errors will be
> > taken as RAS local interrupt or RAS external interrupt (through interrupt
> > controller).
> >
> > Only RAS synchronous errors can cause nested trap.
> >
> >> If that's the case, how can we even safely handle nested traps? E.g.,
> >> The entry of _trap_handler doesn't look like reentrant-safe to me. Perhaps you plan
> >> to enhance it in later patches.
> >>
> >
> > The _trap_handler pushes the register state on stack. It already takes care
> > of nesting by continuing the same SP if the trap was taken while in M-mode.
> >
> > If _trap_handler() did not support nesting then I would not be able to test
> > this series.
> >
> > Refer, TRAP_SAVE_AND_SETUP_SP_T0() in fw_base.S
> >
> > Regards,
> > Anup
> >
>
> I'm looking at the exact code you are pointing.
>
> > .macro TRAP_SAVE_AND_SETUP_SP_T0
> >       /* Swap TP and MSCRATCH */
> >       csrrw   tp, CSR_MSCRATCH, tp
>
> ===> what if RAS exception gets triggered here?
>
> >       /* Save T0 in scratch space */
> >       REG_S   t0, SBI_SCRATCH_TMP0_OFFSET(tp)
> >
>
> ===> Or here?
>
> >       /*
> >        * Set T0 to appropriate exception stack
> >        *
> >        * Came_From_M_Mode = ((MSTATUS.MPP < PRV_M) ? 1 : 0) - 1;
> >        * Exception_Stack = TP ^ (Came_From_M_Mode & (SP ^ TP))
> >        *
> >        * Came_From_M_Mode = 0    ==>    Exception_Stack = TP
> >        * Came_From_M_Mode = -1   ==>    Exception_Stack = SP
> >        */
>
> ===> Or somewhere below?
>
> >       csrr    t0, CSR_MSTATUS
> >       srl     t0, t0, MSTATUS_MPP_SHIFT
> >       and     t0, t0, PRV_M
> .....
>
> We only have one spare register CSR_MSCRATCH to work with before we
> can work on stack. How are we suppose to do the nested trap safely?
> Am i missing something?
>

If we get a nested trap before _trap_handler() gets a chance to save
state then nested trap handling will not work.

The _trap_handler() functionally supports nesting but it is certainly
not 100% safe due to lack of additional HW support.

The upcoming double trap extensions (particularly Smdbltrp) will
help us provide 100% safety in the low-level _trap_handler().
(Refer, https://wiki.riscv.org/display/HOME/RISC-V+Specification+Status)

Think of this series as preparatory work for using double trap
extension in OpenSBI.

Regards,
Anup

[PATCH 0/9] Improve trap handling for nested traps

[Bo Gan]

On 3/12/24 12:43 AM, Anup Patel wrote:
> On Tue, Mar 12, 2024 at 11:11?AM Bo Gan <ganboing@gmail.com> wrote:
>>
>> On 3/11/24 10:18 PM, Anup Patel wrote:
>>> On Tue, Mar 12, 2024 at 10:03?AM Bo Gan <ganboing@gmail.com> wrote:
>>>>
>>>> On 3/11/24 8:43 PM, Anup Patel wrote:
>>>>> On Tue, Mar 12, 2024 at 6:31?AM Bo Gan <ganboing@gmail.com> wrote:
>>>>>>
>>>>>> On 3/11/24 9:09 AM, Anup Patel wrote:
>>>>>>> Nested traps will be a common when dealing with RAS error traps so
>>>>>>> this series improves trap handling for nested traps by introducing
>>>>>>> a linked-list based trap context chain.
>>>>>>>
>>>>>>> These patches can also be found the trap_handling_imp_v1 branch at
>>>>>>> https://github.com/avpatel/opensbi.git
>>>>>>>
>>>>>>> Anup Patel (9):
>>>>>>>       lib: sbi: Remove sbi_trap_exit() and related code
>>>>>>>       include: sbi: Add trap_context pointer in struct sbi_scratch
>>>>>>>       lib: sbi: Introduce trap context
>>>>>>>       lib: sbi: Simplify parameters of misaligned and access fault handlers
>>>>>>>       lib: sbi: Simplify parameters of sbi_illegal_insn_handler()
>>>>>>>       lib: sbi: Remove regs paramter of sbi_irqchip_process()
>>>>>>>       lib: sbi: Remove regs parameter from trap irq handling functions
>>>>>>>       lib: sbi: Pass trap context pointer to sbi_ecall_handler()
>>>>>>>       lib: sbi: Extend sbi_trap_error() to dump state in a nested trap
>>>>>>>
>>>>>>>      firmware/fw_base.S             |  14 +--
>>>>>>>      include/sbi/sbi_ecall.h        |   4 +-
>>>>>>>      include/sbi/sbi_illegal_insn.h |   4 +-
>>>>>>>      include/sbi/sbi_irqchip.h      |   5 +-
>>>>>>>      include/sbi/sbi_scratch.h      |  14 +--
>>>>>>>      include/sbi/sbi_trap.h         |  24 ++++-
>>>>>>>      include/sbi/sbi_trap_ldst.h    |  12 +--
>>>>>>>      lib/sbi/sbi_ecall.c            |   3 +-
>>>>>>>      lib/sbi/sbi_illegal_insn.c     |  14 +--
>>>>>>>      lib/sbi/sbi_irqchip.c          |  10 +-
>>>>>>>      lib/sbi/sbi_trap.c             | 186 +++++++++++++++++----------------
>>>>>>>      lib/sbi/sbi_trap_ldst.c        |  67 ++++++------
>>>>>>>      lib/utils/irqchip/imsic.c      |   2 +-
>>>>>>>      13 files changed, 185 insertions(+), 174 deletions(-)
>>>>>>>
>>>>>>
>>>>>> Hi Anup,
>>>>>>
>>>>>> Can you help providing an example for nested traps and the RAS extension?
>>>>>> The closest spec I can find (https://github.com/riscv/riscv-ssrastraps) is
>>>>>> still empty. I'm wondering where I can find the related documentation.
>>>>>
>>>>> Refer, "3.1.15. Machine Cause Register" of the draft Priv v1.13 specification.
>>>>> https://github.com/riscv/riscv-isa-manual/releases/download/riscv-isa-release-157641b-2024-03-12/priv-isa-asciidoc.pdf
>>>>>
>>>>> Regards,
>>>>> Anup
>>>>>
>>>>
>>>>
>>>> Hi Anup, Thanks for the pointer. I know what nested trap is. My concern is when do
>>>> we expect such trap to happen? Does it mean for every memory access in M mode, it
>>>> might trigger a RAS fault, causing a nested trap? Or even the fault can be delivered
>>>> asynchronously?
>>>
>>> RAS error can occur at any time. A synchronous RAS errors will be taken
>>> as an exception (mcause = 19) whereas asynchronous RAS errors will be
>>> taken as RAS local interrupt or RAS external interrupt (through interrupt
>>> controller).
>>>
>>> Only RAS synchronous errors can cause nested trap.
>>>
>>>> If that's the case, how can we even safely handle nested traps? E.g.,
>>>> The entry of _trap_handler doesn't look like reentrant-safe to me. Perhaps you plan
>>>> to enhance it in later patches.
>>>>
>>>
>>> The _trap_handler pushes the register state on stack. It already takes care
>>> of nesting by continuing the same SP if the trap was taken while in M-mode.
>>>
>>> If _trap_handler() did not support nesting then I would not be able to test
>>> this series.
>>>
>>> Refer, TRAP_SAVE_AND_SETUP_SP_T0() in fw_base.S
>>>
>>> Regards,
>>> Anup
>>>
>>
>> I'm looking at the exact code you are pointing.
>>
>>> .macro TRAP_SAVE_AND_SETUP_SP_T0
>>>        /* Swap TP and MSCRATCH */
>>>        csrrw   tp, CSR_MSCRATCH, tp
>>
>> ===> what if RAS exception gets triggered here?
>>
>>>        /* Save T0 in scratch space */
>>>        REG_S   t0, SBI_SCRATCH_TMP0_OFFSET(tp)
>>>
>>
>> ===> Or here?
>>
>>>        /*
>>>         * Set T0 to appropriate exception stack
>>>         *
>>>         * Came_From_M_Mode = ((MSTATUS.MPP < PRV_M) ? 1 : 0) - 1;
>>>         * Exception_Stack = TP ^ (Came_From_M_Mode & (SP ^ TP))
>>>         *
>>>         * Came_From_M_Mode = 0    ==>    Exception_Stack = TP
>>>         * Came_From_M_Mode = -1   ==>    Exception_Stack = SP
>>>         */
>>
>> ===> Or somewhere below?
>>
>>>        csrr    t0, CSR_MSTATUS
>>>        srl     t0, t0, MSTATUS_MPP_SHIFT
>>>        and     t0, t0, PRV_M
>> .....
>>
>> We only have one spare register CSR_MSCRATCH to work with before we
>> can work on stack. How are we suppose to do the nested trap safely?
>> Am i missing something?
>>
> 
> If we get a nested trap before _trap_handler() gets a chance to save
> state then nested trap handling will not work.
> 
> The _trap_handler() functionally supports nesting but it is certainly
> not 100% safe due to lack of additional HW support.
> 
> The upcoming double trap extensions (particularly Smdbltrp) will
> help us provide 100% safety in the low-level _trap_handler().
> (Refer, https://wiki.riscv.org/display/HOME/RISC-V+Specification+Status)
> 
> Think of this series as preparatory work for using double trap
> extension in OpenSBI.
> 
> Regards,
> Anup
> 

Thanks Anup. This double trap extension fills the void. Now this whole
series make a lot more sense to me with all these background information.

Bo