* [PATCH v2 01/10] lib: sbi: Remove epc from struct sbi_trap_info
2024-03-12 10:27 [PATCH v2 00/10] Improve trap handling for nested traps Anup Patel
@ 2024-03-12 10:27 ` Anup Patel
2024-03-12 10:27 ` [PATCH v2 02/10] lib: sbi: Remove sbi_trap_exit() and related code Anup Patel
` (8 subsequent siblings)
9 siblings, 0 replies; 30+ messages in thread
From: Anup Patel @ 2024-03-12 10:27 UTC (permalink / raw)
To: opensbi
From: Samuel Holland <samuel.holland@sifive.com>
In the only places this value is used, it duplicates mepc from
struct sbi_trap_regs.
Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
Reviewed-by: Anup Patel <anup@brainfault.org>
---
include/sbi/sbi_trap.h | 16 ++++++----------
lib/sbi/sbi_ecall_legacy.c | 4 ----
lib/sbi/sbi_expected_trap.S | 4 ----
lib/sbi/sbi_illegal_insn.c | 2 --
lib/sbi/sbi_trap.c | 5 ++---
lib/sbi/sbi_trap_ldst.c | 4 ----
6 files changed, 8 insertions(+), 27 deletions(-)
diff --git a/include/sbi/sbi_trap.h b/include/sbi/sbi_trap.h
index 2727bdb..3757694 100644
--- a/include/sbi/sbi_trap.h
+++ b/include/sbi/sbi_trap.h
@@ -87,20 +87,18 @@
/** Last member index in sbi_trap_regs */
#define SBI_TRAP_REGS_last 35
-/** Index of epc member in sbi_trap_info */
-#define SBI_TRAP_INFO_epc 0
/** Index of cause member in sbi_trap_info */
-#define SBI_TRAP_INFO_cause 1
+#define SBI_TRAP_INFO_cause 0
/** Index of tval member in sbi_trap_info */
-#define SBI_TRAP_INFO_tval 2
+#define SBI_TRAP_INFO_tval 1
/** Index of tval2 member in sbi_trap_info */
-#define SBI_TRAP_INFO_tval2 3
+#define SBI_TRAP_INFO_tval2 2
/** Index of tinst member in sbi_trap_info */
-#define SBI_TRAP_INFO_tinst 4
+#define SBI_TRAP_INFO_tinst 3
/** Index of gva member in sbi_trap_info */
-#define SBI_TRAP_INFO_gva 5
+#define SBI_TRAP_INFO_gva 4
/** Last member index in sbi_trap_info */
-#define SBI_TRAP_INFO_last 6
+#define SBI_TRAP_INFO_last 5
/* clang-format on */
@@ -194,8 +192,6 @@ struct sbi_trap_regs {
/** Representation of trap details */
struct sbi_trap_info {
- /** epc Trap program counter */
- unsigned long epc;
/** cause Trap exception cause */
unsigned long cause;
/** tval Trap value */
diff --git a/lib/sbi/sbi_ecall_legacy.c b/lib/sbi/sbi_ecall_legacy.c
index 14913c9..9a1ae1e 100644
--- a/lib/sbi/sbi_ecall_legacy.c
+++ b/lib/sbi/sbi_ecall_legacy.c
@@ -74,7 +74,6 @@ static int sbi_ecall_legacy_handler(unsigned long extid, unsigned long funcid,
&hmask, &trap)) {
ret = sbi_ipi_send_smode(hmask, 0);
} else {
- trap.epc = regs->mepc;
sbi_trap_redirect(regs, &trap);
out->skip_regs_update = true;
}
@@ -86,7 +85,6 @@ static int sbi_ecall_legacy_handler(unsigned long extid, unsigned long funcid,
SBI_TLB_FENCE_I, source_hart);
ret = sbi_tlb_request(hmask, 0, &tlb_info);
} else {
- trap.epc = regs->mepc;
sbi_trap_redirect(regs, &trap);
out->skip_regs_update = true;
}
@@ -98,7 +96,6 @@ static int sbi_ecall_legacy_handler(unsigned long extid, unsigned long funcid,
SBI_TLB_SFENCE_VMA, source_hart);
ret = sbi_tlb_request(hmask, 0, &tlb_info);
} else {
- trap.epc = regs->mepc;
sbi_trap_redirect(regs, &trap);
out->skip_regs_update = true;
}
@@ -112,7 +109,6 @@ static int sbi_ecall_legacy_handler(unsigned long extid, unsigned long funcid,
source_hart);
ret = sbi_tlb_request(hmask, 0, &tlb_info);
} else {
- trap.epc = regs->mepc;
sbi_trap_redirect(regs, &trap);
out->skip_regs_update = true;
}
diff --git a/lib/sbi/sbi_expected_trap.S b/lib/sbi/sbi_expected_trap.S
index 1f2d6b9..99dede5 100644
--- a/lib/sbi/sbi_expected_trap.S
+++ b/lib/sbi/sbi_expected_trap.S
@@ -23,8 +23,6 @@
.global __sbi_expected_trap
__sbi_expected_trap:
/* Without H-extension so, MTVAL2 and MTINST CSRs and GVA not available */
- csrr a4, CSR_MEPC
- REG_S a4, SBI_TRAP_INFO_OFFSET(epc)(a3)
csrr a4, CSR_MCAUSE
REG_S a4, SBI_TRAP_INFO_OFFSET(cause)(a3)
csrr a4, CSR_MTVAL
@@ -41,8 +39,6 @@ __sbi_expected_trap:
.global __sbi_expected_trap_hext
__sbi_expected_trap_hext:
/* With H-extension so, MTVAL2 and MTINST CSRs and GVA available */
- csrr a4, CSR_MEPC
- REG_S a4, SBI_TRAP_INFO_OFFSET(epc)(a3)
csrr a4, CSR_MCAUSE
REG_S a4, SBI_TRAP_INFO_OFFSET(cause)(a3)
csrr a4, CSR_MTVAL
diff --git a/lib/sbi/sbi_illegal_insn.c b/lib/sbi/sbi_illegal_insn.c
index 2be4757..dd0b3c1 100644
--- a/lib/sbi/sbi_illegal_insn.c
+++ b/lib/sbi/sbi_illegal_insn.c
@@ -25,7 +25,6 @@ static int truly_illegal_insn(ulong insn, struct sbi_trap_regs *regs)
{
struct sbi_trap_info trap;
- trap.epc = regs->mepc;
trap.cause = CAUSE_ILLEGAL_INSTRUCTION;
trap.tval = insn;
trap.tval2 = 0;
@@ -156,7 +155,6 @@ int sbi_illegal_insn_handler(ulong insn, struct sbi_trap_regs *regs)
if (unlikely((insn & 3) != 3)) {
insn = sbi_get_insn(regs->mepc, &uptrap);
if (uptrap.cause) {
- uptrap.epc = regs->mepc;
return sbi_trap_redirect(regs, &uptrap);
}
if ((insn & 3) != 3)
diff --git a/lib/sbi/sbi_trap.c b/lib/sbi/sbi_trap.c
index c665013..c9db73f 100644
--- a/lib/sbi/sbi_trap.c
+++ b/lib/sbi/sbi_trap.c
@@ -140,7 +140,7 @@ int sbi_trap_redirect(struct sbi_trap_regs *regs,
if (next_virt) {
/* Update VS-mode exception info */
csr_write(CSR_VSTVAL, trap->tval);
- csr_write(CSR_VSEPC, trap->epc);
+ csr_write(CSR_VSEPC, regs->mepc);
csr_write(CSR_VSCAUSE, trap->cause);
/* Set MEPC to VS-mode exception vector base */
@@ -171,7 +171,7 @@ int sbi_trap_redirect(struct sbi_trap_regs *regs,
} else {
/* Update S-mode exception info */
csr_write(CSR_STVAL, trap->tval);
- csr_write(CSR_SEPC, trap->epc);
+ csr_write(CSR_SEPC, regs->mepc);
csr_write(CSR_SCAUSE, trap->cause);
/* Set MEPC to S-mode exception vector base */
@@ -286,7 +286,6 @@ struct sbi_trap_regs *sbi_trap_handler(struct sbi_trap_regs *regs)
return regs;
}
/* Original trap_info */
- trap.epc = regs->mepc;
trap.cause = mcause;
trap.tval = mtval;
trap.tval2 = mtval2;
diff --git a/lib/sbi/sbi_trap_ldst.c b/lib/sbi/sbi_trap_ldst.c
index d864ad1..5a0537b 100644
--- a/lib/sbi/sbi_trap_ldst.c
+++ b/lib/sbi/sbi_trap_ldst.c
@@ -70,7 +70,6 @@ static int sbi_trap_emulate_load(struct sbi_trap_regs *regs,
*/
insn = sbi_get_insn(regs->mepc, &uptrap);
if (uptrap.cause) {
- uptrap.epc = regs->mepc;
return sbi_trap_redirect(regs, &uptrap);
}
insn_len = INSN_LEN(insn);
@@ -193,7 +192,6 @@ static int sbi_trap_emulate_store(struct sbi_trap_regs *regs,
*/
insn = sbi_get_insn(regs->mepc, &uptrap);
if (uptrap.cause) {
- uptrap.epc = regs->mepc;
return sbi_trap_redirect(regs, &uptrap);
}
insn_len = INSN_LEN(insn);
@@ -277,7 +275,6 @@ static int sbi_misaligned_ld_emulator(int rlen, union sbi_ldst_data *out_val,
out_val->data_bytes[i] =
sbi_load_u8((void *)(orig_trap->tval + i), &uptrap);
if (uptrap.cause) {
- uptrap.epc = regs->mepc;
uptrap.tinst = sbi_misaligned_tinst_fixup(
orig_trap->tinst, uptrap.tinst, i);
return sbi_trap_redirect(regs, &uptrap);
@@ -304,7 +301,6 @@ static int sbi_misaligned_st_emulator(int wlen, union sbi_ldst_data in_val,
sbi_store_u8((void *)(orig_trap->tval + i),
in_val.data_bytes[i], &uptrap);
if (uptrap.cause) {
- uptrap.epc = regs->mepc;
uptrap.tinst = sbi_misaligned_tinst_fixup(
orig_trap->tinst, uptrap.tinst, i);
return sbi_trap_redirect(regs, &uptrap);
--
2.34.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH v2 02/10] lib: sbi: Remove sbi_trap_exit() and related code
2024-03-12 10:27 [PATCH v2 00/10] Improve trap handling for nested traps Anup Patel
2024-03-12 10:27 ` [PATCH v2 01/10] lib: sbi: Remove epc from struct sbi_trap_info Anup Patel
@ 2024-03-12 10:27 ` Anup Patel
2024-03-12 10:27 ` [PATCH v2 03/10] include: sbi: Add trap_context pointer in struct sbi_scratch Anup Patel
` (7 subsequent siblings)
9 siblings, 0 replies; 30+ messages in thread
From: Anup Patel @ 2024-03-12 10:27 UTC (permalink / raw)
To: opensbi
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>
Reviewed-by: Samuel Holland <samuel.holland@sifive.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 3757694..a6032ab 100644
--- a/include/sbi/sbi_trap.h
+++ b/include/sbi/sbi_trap.h
@@ -225,8 +225,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 c9db73f..b059c4b 100644
--- a/lib/sbi/sbi_trap.c
+++ b/lib/sbi/sbi_trap.c
@@ -334,22 +334,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
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH v2 03/10] include: sbi: Add trap_context pointer in struct sbi_scratch
2024-03-12 10:27 [PATCH v2 00/10] Improve trap handling for nested traps Anup Patel
2024-03-12 10:27 ` [PATCH v2 01/10] lib: sbi: Remove epc from struct sbi_trap_info Anup Patel
2024-03-12 10:27 ` [PATCH v2 02/10] lib: sbi: Remove sbi_trap_exit() and related code Anup Patel
@ 2024-03-12 10:27 ` Anup Patel
2024-03-12 16:55 ` Samuel Holland
2024-03-12 10:27 ` [PATCH v2 04/10] lib: sbi: Introduce trap context Anup Patel
` (6 subsequent siblings)
9 siblings, 1 reply; 30+ messages in thread
From: Anup Patel @ 2024-03-12 10:27 UTC (permalink / raw)
To: opensbi
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
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH v2 04/10] lib: sbi: Introduce trap context
2024-03-12 10:27 [PATCH v2 00/10] Improve trap handling for nested traps Anup Patel
` (2 preceding siblings ...)
2024-03-12 10:27 ` [PATCH v2 03/10] include: sbi: Add trap_context pointer in struct sbi_scratch Anup Patel
@ 2024-03-12 10:27 ` Anup Patel
2024-03-12 16:55 ` Samuel Holland
2024-03-13 4:56 ` Xiang W
2024-03-12 10:27 ` [PATCH v2 05/10] lib: sbi: Simplify parameters of misaligned and access fault handlers Anup Patel
` (5 subsequent siblings)
9 siblings, 2 replies; 30+ messages in thread
From: Anup Patel @ 2024-03-12 10:27 UTC (permalink / raw)
To: opensbi
Club the struct sbi_trap_regs and struct sbi_trap_info a new
struct sbi_trap_context (aka trap context) which must be saved
by low-level trap handler before calling sbi_trap_handler().
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>
---
firmware/fw_base.S | 62 ++++++++++++++++++++++++++++++++++--------
include/sbi/sbi_trap.h | 29 +++++++++++++++++++-
lib/sbi/sbi_trap.c | 58 +++++++++++++++++----------------------
3 files changed, 103 insertions(+), 46 deletions(-)
diff --git a/firmware/fw_base.S b/firmware/fw_base.S
index 539fd1d..967cca5 100644
--- a/firmware/fw_base.S
+++ b/firmware/fw_base.S
@@ -446,14 +446,12 @@ _start_warm:
/* Setup trap handler */
lla a4, _trap_handler
-#if __riscv_xlen == 32
csrr a5, CSR_MISA
srli a5, a5, ('H' - 'A')
andi a5, a5, 0x1
- beq a5, zero, _skip_trap_handler_rv32_hyp
- lla a4, _trap_handler_rv32_hyp
-_skip_trap_handler_rv32_hyp:
-#endif
+ beq a5, zero, _skip_trap_handler_hyp
+ lla a4, _trap_handler_hyp
+_skip_trap_handler_hyp:
csrw CSR_MTVEC, a4
/* Initialize SBI runtime */
@@ -564,10 +562,10 @@ memcmp:
xor t0, tp, t0
/* Save original SP on exception stack */
- REG_S sp, (SBI_TRAP_REGS_OFFSET(sp) - SBI_TRAP_REGS_SIZE)(t0)
+ REG_S sp, (SBI_TRAP_REGS_OFFSET(sp) - SBI_TRAP_CONTEXT_SIZE)(t0)
- /* Set SP to exception stack and make room for trap registers */
- add sp, t0, -(SBI_TRAP_REGS_SIZE)
+ /* Set SP to exception stack and make room for trap context */
+ add sp, t0, -(SBI_TRAP_CONTEXT_SIZE)
/* Restore T0 from scratch space */
REG_L t0, SBI_SCRATCH_TMP0_OFFSET(tp)
@@ -627,6 +625,32 @@ memcmp:
REG_S t6, SBI_TRAP_REGS_OFFSET(t6)(sp)
.endm
+.macro TRAP_SAVE_INFO have_mstatush have_h_extension
+ csrr t0, CSR_MCAUSE
+ REG_S t0, (SBI_TRAP_REGS_SIZE + SBI_TRAP_INFO_OFFSET(cause))(sp)
+ csrr t0, CSR_MTVAL
+ REG_S t0, (SBI_TRAP_REGS_SIZE + SBI_TRAP_INFO_OFFSET(tval))(sp)
+.if \have_h_extension
+ csrr t0, CSR_MTVAL2
+ REG_S t0, (SBI_TRAP_REGS_SIZE + SBI_TRAP_INFO_OFFSET(tval2))(sp)
+ csrr t0, CSR_MTINST
+ REG_S t0, (SBI_TRAP_REGS_SIZE + SBI_TRAP_INFO_OFFSET(tinst))(sp)
+ .if \have_mstatush
+ csrr t0, CSR_MSTATUSH
+ srli t0, t0, MSTATUSH_GVA_SHIFT
+ .else
+ csrr t0, CSR_MSTATUS
+ srli t0, t0, MSTATUS_GVA_SHIFT
+ .endif
+ and t0, t0, 0x1
+.else
+ REG_S zero, (SBI_TRAP_REGS_SIZE + SBI_TRAP_INFO_OFFSET(tval2))(sp)
+ REG_S zero, (SBI_TRAP_REGS_SIZE + SBI_TRAP_INFO_OFFSET(tinst))(sp)
+ add t0, zero, zero
+.endif
+ REG_S t0, (SBI_TRAP_REGS_SIZE + SBI_TRAP_INFO_OFFSET(gva))(sp)
+.endm
+
.macro TRAP_CALL_C_ROUTINE
/* Call C routine */
add a0, sp, zero
@@ -696,6 +720,8 @@ _trap_handler:
TRAP_SAVE_GENERAL_REGS_EXCEPT_SP_T0
+ TRAP_SAVE_INFO 0 0
+
TRAP_CALL_C_ROUTINE
TRAP_RESTORE_GENERAL_REGS_EXCEPT_A0_T0
@@ -706,27 +732,39 @@ _trap_handler:
mret
-#if __riscv_xlen == 32
.section .entry, "ax", %progbits
.align 3
- .globl _trap_handler_rv32_hyp
-_trap_handler_rv32_hyp:
+ .globl _trap_handler_hyp
+_trap_handler_hyp:
TRAP_SAVE_AND_SETUP_SP_T0
+#if __riscv_xlen == 32
TRAP_SAVE_MEPC_MSTATUS 1
+#else
+ TRAP_SAVE_MEPC_MSTATUS 0
+#endif
TRAP_SAVE_GENERAL_REGS_EXCEPT_SP_T0
+#if __riscv_xlen == 32
+ TRAP_SAVE_INFO 1 1
+#else
+ TRAP_SAVE_INFO 0 1
+#endif
+
TRAP_CALL_C_ROUTINE
TRAP_RESTORE_GENERAL_REGS_EXCEPT_A0_T0
+#if __riscv_xlen == 32
TRAP_RESTORE_MEPC_MSTATUS 1
+#else
+ TRAP_RESTORE_MEPC_MSTATUS 0
+#endif
TRAP_RESTORE_A0_T0
mret
-#endif
.section .entry, "ax", %progbits
.align 3
diff --git a/include/sbi/sbi_trap.h b/include/sbi/sbi_trap.h
index a6032ab..f7c170e 100644
--- a/include/sbi/sbi_trap.h
+++ b/include/sbi/sbi_trap.h
@@ -112,9 +112,15 @@
/** Size (in bytes) of sbi_trap_info */
#define SBI_TRAP_INFO_SIZE SBI_TRAP_INFO_OFFSET(last)
+/** Size (in bytes) of sbi_trap_context */
+#define SBI_TRAP_CONTEXT_SIZE (SBI_TRAP_REGS_SIZE + \
+ SBI_TRAP_INFO_SIZE + \
+ __SIZEOF_POINTER__)
+
#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 {
@@ -204,6 +210,16 @@ struct sbi_trap_info {
unsigned long gva;
};
+/** Representation of trap context saved on stack */
+struct sbi_trap_context {
+ /** Register state */
+ struct sbi_trap_regs regs;
+ /** Trap details */
+ struct sbi_trap_info trap;
+ /** Pointer to previous trap context */
+ struct sbi_trap_context *prev_context;
+};
+
static inline unsigned long sbi_regs_gva(const struct sbi_trap_regs *regs)
{
/*
@@ -223,7 +239,18 @@ 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);
-struct sbi_trap_regs *sbi_trap_handler(struct sbi_trap_regs *regs);
+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_context *sbi_trap_handler(struct sbi_trap_context *tcntx);
#endif
diff --git a/lib/sbi/sbi_trap.c b/lib/sbi/sbi_trap.c
index b059c4b..4dbda06 100644
--- a/lib/sbi/sbi_trap.c
+++ b/lib/sbi/sbi_trap.c
@@ -24,18 +24,18 @@
#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)
+ const struct sbi_trap_info *trap,
+ const struct sbi_trap_regs *regs)
{
u32 hartid = current_hartid();
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);
+ __func__, hartid, trap->cause, trap->tval);
if (misa_extension('H')) {
sbi_printf("%s: hart%d: mtval2=0x%" PRILX
" mtinst=0x%" PRILX "\n",
- __func__, hartid, mtval2, mtinst);
+ __func__, hartid, trap->tval2, trap->tinst);
}
sbi_printf("%s: hart%d: mepc=0x%" PRILX " mstatus=0x%" PRILX "\n",
__func__, hartid, regs->mepc, regs->mstatus);
@@ -258,20 +258,20 @@ static int sbi_trap_aia_irq(struct sbi_trap_regs *regs, ulong mcause)
* 6. Stack pointer (SP) is setup for current HART
* 7. Interrupts are disabled in MSTATUS CSR
*
- * @param regs pointer to register state
+ * @param tcntx pointer to trap context
*/
-struct sbi_trap_regs *sbi_trap_handler(struct sbi_trap_regs *regs)
+struct sbi_trap_context *sbi_trap_handler(struct sbi_trap_context *tcntx)
{
int rc = SBI_ENOTSUPP;
const char *msg = "trap handler failed";
- ulong mcause = csr_read(CSR_MCAUSE);
- ulong mtval = csr_read(CSR_MTVAL), mtval2 = 0, mtinst = 0;
- struct sbi_trap_info trap;
+ struct sbi_scratch *scratch = sbi_scratch_thishart_ptr();
+ const struct sbi_trap_info *trap = &tcntx->trap;
+ struct sbi_trap_regs *regs = &tcntx->regs;
+ ulong mcause = tcntx->trap.cause;
- if (misa_extension('H')) {
- mtval2 = csr_read(CSR_MTVAL2);
- mtinst = csr_read(CSR_MTINST);
- }
+ /* Update trap context pointer */
+ tcntx->prev_context = sbi_trap_get_context(scratch);
+ sbi_trap_set_context(scratch, tcntx);
if (mcause & (1UL << (__riscv_xlen - 1))) {
if (sbi_hart_has_extension(sbi_scratch_thishart_ptr(),
@@ -279,32 +279,23 @@ struct sbi_trap_regs *sbi_trap_handler(struct sbi_trap_regs *regs)
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.cause = mcause;
- trap.tval = mtval;
- trap.tval2 = mtval2;
- trap.tinst = mtinst;
- trap.gva = sbi_regs_gva(regs);
switch (mcause) {
case CAUSE_ILLEGAL_INSTRUCTION:
- rc = sbi_illegal_insn_handler(mtval, regs);
+ rc = sbi_illegal_insn_handler(tcntx->trap.tval, regs);
msg = "illegal instruction handler failed";
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(regs, trap);
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(regs, trap);
msg = "misaligned store handler failed";
break;
case CAUSE_SUPERVISOR_ECALL:
@@ -314,23 +305,24 @@ 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(regs, trap);
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(regs, trap);
msg = "store fault handler failed";
break;
default:
/* If the trap came from S or U mode, redirect it there */
msg = "trap redirect failed";
- rc = sbi_trap_redirect(regs, &trap);
+ rc = sbi_trap_redirect(regs, trap);
break;
}
-trap_error:
+trap_done:
if (rc)
- sbi_trap_error(msg, rc, mcause, mtval, mtval2, mtinst, regs);
- return regs;
+ sbi_trap_error(msg, rc, trap, regs);
+ sbi_trap_set_context(scratch, tcntx->prev_context);
+ return tcntx;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH v2 04/10] lib: sbi: Introduce trap context
2024-03-12 10:27 ` [PATCH v2 04/10] lib: sbi: Introduce trap context Anup Patel
@ 2024-03-12 16:55 ` Samuel Holland
2024-03-13 4:56 ` Xiang W
1 sibling, 0 replies; 30+ messages in thread
From: Samuel Holland @ 2024-03-12 16:55 UTC (permalink / raw)
To: opensbi
On 2024-03-12 5:27 AM, Anup Patel wrote:
> Club the struct sbi_trap_regs and struct sbi_trap_info a new
> struct sbi_trap_context (aka trap context) which must be saved
> by low-level trap handler before calling sbi_trap_handler().
>
> 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>
> ---
> firmware/fw_base.S | 62 ++++++++++++++++++++++++++++++++++--------
> include/sbi/sbi_trap.h | 29 +++++++++++++++++++-
> lib/sbi/sbi_trap.c | 58 +++++++++++++++++----------------------
> 3 files changed, 103 insertions(+), 46 deletions(-)
>
> diff --git a/firmware/fw_base.S b/firmware/fw_base.S
> index 539fd1d..967cca5 100644
> --- a/firmware/fw_base.S
> +++ b/firmware/fw_base.S
> @@ -446,14 +446,12 @@ _start_warm:
>
> /* Setup trap handler */
> lla a4, _trap_handler
> -#if __riscv_xlen == 32
> csrr a5, CSR_MISA
> srli a5, a5, ('H' - 'A')
> andi a5, a5, 0x1
> - beq a5, zero, _skip_trap_handler_rv32_hyp
> - lla a4, _trap_handler_rv32_hyp
> -_skip_trap_handler_rv32_hyp:
> -#endif
> + beq a5, zero, _skip_trap_handler_hyp
> + lla a4, _trap_handler_hyp
> +_skip_trap_handler_hyp:
> csrw CSR_MTVEC, a4
>
> /* Initialize SBI runtime */
> @@ -564,10 +562,10 @@ memcmp:
> xor t0, tp, t0
>
> /* Save original SP on exception stack */
> - REG_S sp, (SBI_TRAP_REGS_OFFSET(sp) - SBI_TRAP_REGS_SIZE)(t0)
> + REG_S sp, (SBI_TRAP_REGS_OFFSET(sp) - SBI_TRAP_CONTEXT_SIZE)(t0)
>
> - /* Set SP to exception stack and make room for trap registers */
> - add sp, t0, -(SBI_TRAP_REGS_SIZE)
> + /* Set SP to exception stack and make room for trap context */
> + add sp, t0, -(SBI_TRAP_CONTEXT_SIZE)
The ABI requires that the stack is aligned to 16 bytes. We already weren't doing
this, so maybe it doesn't matter.
>
> /* Restore T0 from scratch space */
> REG_L t0, SBI_SCRATCH_TMP0_OFFSET(tp)
> @@ -627,6 +625,32 @@ memcmp:
> REG_S t6, SBI_TRAP_REGS_OFFSET(t6)(sp)
> .endm
>
> +.macro TRAP_SAVE_INFO have_mstatush have_h_extension
> + csrr t0, CSR_MCAUSE
> + REG_S t0, (SBI_TRAP_REGS_SIZE + SBI_TRAP_INFO_OFFSET(cause))(sp)
> + csrr t0, CSR_MTVAL
> + REG_S t0, (SBI_TRAP_REGS_SIZE + SBI_TRAP_INFO_OFFSET(tval))(sp)
> +.if \have_h_extension
> + csrr t0, CSR_MTVAL2
> + REG_S t0, (SBI_TRAP_REGS_SIZE + SBI_TRAP_INFO_OFFSET(tval2))(sp)
> + csrr t0, CSR_MTINST
> + REG_S t0, (SBI_TRAP_REGS_SIZE + SBI_TRAP_INFO_OFFSET(tinst))(sp)
> + .if \have_mstatush
> + csrr t0, CSR_MSTATUSH
> + srli t0, t0, MSTATUSH_GVA_SHIFT
> + .else
> + csrr t0, CSR_MSTATUS
> + srli t0, t0, MSTATUS_GVA_SHIFT
> + .endif
> + and t0, t0, 0x1
> +.else
> + REG_S zero, (SBI_TRAP_REGS_SIZE + SBI_TRAP_INFO_OFFSET(tval2))(sp)
> + REG_S zero, (SBI_TRAP_REGS_SIZE + SBI_TRAP_INFO_OFFSET(tinst))(sp)
> + add t0, zero, zero
Why not use `li` here?
In any case, the changes LGTM, so:
Reviewed-by: Samuel Holland <samuel.holland@sifive.com>
> +.endif
> + REG_S t0, (SBI_TRAP_REGS_SIZE + SBI_TRAP_INFO_OFFSET(gva))(sp)
> +.endm
> +
> .macro TRAP_CALL_C_ROUTINE
> /* Call C routine */
> add a0, sp, zero
> @@ -696,6 +720,8 @@ _trap_handler:
>
> TRAP_SAVE_GENERAL_REGS_EXCEPT_SP_T0
>
> + TRAP_SAVE_INFO 0 0
> +
> TRAP_CALL_C_ROUTINE
>
> TRAP_RESTORE_GENERAL_REGS_EXCEPT_A0_T0
> @@ -706,27 +732,39 @@ _trap_handler:
>
> mret
>
> -#if __riscv_xlen == 32
> .section .entry, "ax", %progbits
> .align 3
> - .globl _trap_handler_rv32_hyp
> -_trap_handler_rv32_hyp:
> + .globl _trap_handler_hyp
> +_trap_handler_hyp:
> TRAP_SAVE_AND_SETUP_SP_T0
>
> +#if __riscv_xlen == 32
> TRAP_SAVE_MEPC_MSTATUS 1
> +#else
> + TRAP_SAVE_MEPC_MSTATUS 0
> +#endif
>
> TRAP_SAVE_GENERAL_REGS_EXCEPT_SP_T0
>
> +#if __riscv_xlen == 32
> + TRAP_SAVE_INFO 1 1
> +#else
> + TRAP_SAVE_INFO 0 1
> +#endif
> +
> TRAP_CALL_C_ROUTINE
>
> TRAP_RESTORE_GENERAL_REGS_EXCEPT_A0_T0
>
> +#if __riscv_xlen == 32
> TRAP_RESTORE_MEPC_MSTATUS 1
> +#else
> + TRAP_RESTORE_MEPC_MSTATUS 0
> +#endif
>
> TRAP_RESTORE_A0_T0
>
> mret
> -#endif
>
> .section .entry, "ax", %progbits
> .align 3
> diff --git a/include/sbi/sbi_trap.h b/include/sbi/sbi_trap.h
> index a6032ab..f7c170e 100644
> --- a/include/sbi/sbi_trap.h
> +++ b/include/sbi/sbi_trap.h
> @@ -112,9 +112,15 @@
> /** Size (in bytes) of sbi_trap_info */
> #define SBI_TRAP_INFO_SIZE SBI_TRAP_INFO_OFFSET(last)
>
> +/** Size (in bytes) of sbi_trap_context */
> +#define SBI_TRAP_CONTEXT_SIZE (SBI_TRAP_REGS_SIZE + \
> + SBI_TRAP_INFO_SIZE + \
> + __SIZEOF_POINTER__)
> +
> #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 {
> @@ -204,6 +210,16 @@ struct sbi_trap_info {
> unsigned long gva;
> };
>
> +/** Representation of trap context saved on stack */
> +struct sbi_trap_context {
> + /** Register state */
> + struct sbi_trap_regs regs;
> + /** Trap details */
> + struct sbi_trap_info trap;
> + /** Pointer to previous trap context */
> + struct sbi_trap_context *prev_context;
> +};
> +
> static inline unsigned long sbi_regs_gva(const struct sbi_trap_regs *regs)
> {
> /*
> @@ -223,7 +239,18 @@ 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);
>
> -struct sbi_trap_regs *sbi_trap_handler(struct sbi_trap_regs *regs);
> +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_context *sbi_trap_handler(struct sbi_trap_context *tcntx);
>
> #endif
>
> diff --git a/lib/sbi/sbi_trap.c b/lib/sbi/sbi_trap.c
> index b059c4b..4dbda06 100644
> --- a/lib/sbi/sbi_trap.c
> +++ b/lib/sbi/sbi_trap.c
> @@ -24,18 +24,18 @@
> #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)
> + const struct sbi_trap_info *trap,
> + const struct sbi_trap_regs *regs)
> {
> u32 hartid = current_hartid();
>
> 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);
> + __func__, hartid, trap->cause, trap->tval);
> if (misa_extension('H')) {
> sbi_printf("%s: hart%d: mtval2=0x%" PRILX
> " mtinst=0x%" PRILX "\n",
> - __func__, hartid, mtval2, mtinst);
> + __func__, hartid, trap->tval2, trap->tinst);
> }
> sbi_printf("%s: hart%d: mepc=0x%" PRILX " mstatus=0x%" PRILX "\n",
> __func__, hartid, regs->mepc, regs->mstatus);
> @@ -258,20 +258,20 @@ static int sbi_trap_aia_irq(struct sbi_trap_regs *regs, ulong mcause)
> * 6. Stack pointer (SP) is setup for current HART
> * 7. Interrupts are disabled in MSTATUS CSR
> *
> - * @param regs pointer to register state
> + * @param tcntx pointer to trap context
> */
> -struct sbi_trap_regs *sbi_trap_handler(struct sbi_trap_regs *regs)
> +struct sbi_trap_context *sbi_trap_handler(struct sbi_trap_context *tcntx)
> {
> int rc = SBI_ENOTSUPP;
> const char *msg = "trap handler failed";
> - ulong mcause = csr_read(CSR_MCAUSE);
> - ulong mtval = csr_read(CSR_MTVAL), mtval2 = 0, mtinst = 0;
> - struct sbi_trap_info trap;
> + struct sbi_scratch *scratch = sbi_scratch_thishart_ptr();
> + const struct sbi_trap_info *trap = &tcntx->trap;
> + struct sbi_trap_regs *regs = &tcntx->regs;
> + ulong mcause = tcntx->trap.cause;
>
> - if (misa_extension('H')) {
> - mtval2 = csr_read(CSR_MTVAL2);
> - mtinst = csr_read(CSR_MTINST);
> - }
> + /* Update trap context pointer */
> + tcntx->prev_context = sbi_trap_get_context(scratch);
> + sbi_trap_set_context(scratch, tcntx);
>
> if (mcause & (1UL << (__riscv_xlen - 1))) {
> if (sbi_hart_has_extension(sbi_scratch_thishart_ptr(),
> @@ -279,32 +279,23 @@ struct sbi_trap_regs *sbi_trap_handler(struct sbi_trap_regs *regs)
> 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.cause = mcause;
> - trap.tval = mtval;
> - trap.tval2 = mtval2;
> - trap.tinst = mtinst;
> - trap.gva = sbi_regs_gva(regs);
>
> switch (mcause) {
> case CAUSE_ILLEGAL_INSTRUCTION:
> - rc = sbi_illegal_insn_handler(mtval, regs);
> + rc = sbi_illegal_insn_handler(tcntx->trap.tval, regs);
> msg = "illegal instruction handler failed";
> 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(regs, trap);
> 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(regs, trap);
> msg = "misaligned store handler failed";
> break;
> case CAUSE_SUPERVISOR_ECALL:
> @@ -314,23 +305,24 @@ 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(regs, trap);
> 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(regs, trap);
> msg = "store fault handler failed";
> break;
> default:
> /* If the trap came from S or U mode, redirect it there */
> msg = "trap redirect failed";
> - rc = sbi_trap_redirect(regs, &trap);
> + rc = sbi_trap_redirect(regs, trap);
> break;
> }
>
> -trap_error:
> +trap_done:
> if (rc)
> - sbi_trap_error(msg, rc, mcause, mtval, mtval2, mtinst, regs);
> - return regs;
> + sbi_trap_error(msg, rc, trap, regs);
> + sbi_trap_set_context(scratch, tcntx->prev_context);
> + return tcntx;
> }
^ permalink raw reply [flat|nested] 30+ messages in thread* [PATCH v2 04/10] lib: sbi: Introduce trap context
2024-03-12 10:27 ` [PATCH v2 04/10] lib: sbi: Introduce trap context Anup Patel
2024-03-12 16:55 ` Samuel Holland
@ 2024-03-13 4:56 ` Xiang W
2024-03-17 11:23 ` Anup Patel
1 sibling, 1 reply; 30+ messages in thread
From: Xiang W @ 2024-03-13 4:56 UTC (permalink / raw)
To: opensbi
? 2024-03-12???? 15:57 +0530?Anup Patel???
> Club the struct sbi_trap_regs and struct sbi_trap_info a new
> struct sbi_trap_context (aka trap context) which must be saved
> by low-level trap handler before calling sbi_trap_handler().
>
> 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>
> ---
> ?firmware/fw_base.S???? | 62 ++++++++++++++++++++++++++++++++++--------
> ?include/sbi/sbi_trap.h | 29 +++++++++++++++++++-
> ?lib/sbi/sbi_trap.c???? | 58 +++++++++++++++++----------------------
> ?3 files changed, 103 insertions(+), 46 deletions(-)
>
> diff --git a/firmware/fw_base.S b/firmware/fw_base.S
> index 539fd1d..967cca5 100644
> --- a/firmware/fw_base.S
> +++ b/firmware/fw_base.S
> @@ -446,14 +446,12 @@ _start_warm:
> ?
> ? /* Setup trap handler */
> ? lla a4, _trap_handler
> -#if __riscv_xlen == 32
> ? csrr a5, CSR_MISA
> ? srli a5, a5, ('H' - 'A')
> ? andi a5, a5, 0x1
> - beq a5, zero, _skip_trap_handler_rv32_hyp
> - lla a4, _trap_handler_rv32_hyp
> -_skip_trap_handler_rv32_hyp:
> -#endif
> + beq a5, zero, _skip_trap_handler_hyp
> + lla a4, _trap_handler_hyp
> +_skip_trap_handler_hyp:
> ? csrw CSR_MTVEC, a4
> ?
> ? /* Initialize SBI runtime */
> @@ -564,10 +562,10 @@ memcmp:
> ? xor t0, tp, t0
> ?
> ? /* Save original SP on exception stack */
> - REG_S sp, (SBI_TRAP_REGS_OFFSET(sp) - SBI_TRAP_REGS_SIZE)(t0)
> + REG_S sp, (SBI_TRAP_REGS_OFFSET(sp) - SBI_TRAP_CONTEXT_SIZE)(t0)
> ?
> - /* Set SP to exception stack and make room for trap registers */
> - add sp, t0, -(SBI_TRAP_REGS_SIZE)
> + /* Set SP to exception stack and make room for trap context */
> + add sp, t0, -(SBI_TRAP_CONTEXT_SIZE)
> ?
> ? /* Restore T0 from scratch space */
> ? REG_L t0, SBI_SCRATCH_TMP0_OFFSET(tp)
> @@ -627,6 +625,32 @@ memcmp:
> ? REG_S t6, SBI_TRAP_REGS_OFFSET(t6)(sp)
> ?.endm
> ?
> +.macro TRAP_SAVE_INFO have_mstatush have_h_extension
> + csrr t0, CSR_MCAUSE
> + REG_S t0, (SBI_TRAP_REGS_SIZE + SBI_TRAP_INFO_OFFSET(cause))(sp)
> + csrr t0, CSR_MTVAL
> + REG_S t0, (SBI_TRAP_REGS_SIZE + SBI_TRAP_INFO_OFFSET(tval))(sp)
> +.if \have_h_extension
> + csrr t0, CSR_MTVAL2
> + REG_S t0, (SBI_TRAP_REGS_SIZE + SBI_TRAP_INFO_OFFSET(tval2))(sp)
> + csrr t0, CSR_MTINST
> + REG_S t0, (SBI_TRAP_REGS_SIZE + SBI_TRAP_INFO_OFFSET(tinst))(sp)
> + .if \have_mstatush
> + csrr t0, CSR_MSTATUSH
> + srli t0, t0, MSTATUSH_GVA_SHIFT
> + .else
> + csrr t0, CSR_MSTATUS
> + srli t0, t0, MSTATUS_GVA_SHIFT
> + .endif
> + and t0, t0, 0x1
> +.else
> + REG_S zero, (SBI_TRAP_REGS_SIZE + SBI_TRAP_INFO_OFFSET(tval2))(sp)
> + REG_S zero, (SBI_TRAP_REGS_SIZE + SBI_TRAP_INFO_OFFSET(tinst))(sp)
> + add t0, zero, zero
> +.endif
> + REG_S t0, (SBI_TRAP_REGS_SIZE + SBI_TRAP_INFO_OFFSET(gva))(sp)
> +.endm
> +
> ?.macro TRAP_CALL_C_ROUTINE
> ? /* Call C routine */
> ? add a0, sp, zero
> @@ -696,6 +720,8 @@ _trap_handler:
> ?
> ? TRAP_SAVE_GENERAL_REGS_EXCEPT_SP_T0
> ?
> + TRAP_SAVE_INFO 0 0
> +
> ? TRAP_CALL_C_ROUTINE
> ?
> ? TRAP_RESTORE_GENERAL_REGS_EXCEPT_A0_T0
> @@ -706,27 +732,39 @@ _trap_handler:
> ?
> ? mret
> ?
> -#if __riscv_xlen == 32
> ? .section .entry, "ax", %progbits
> ? .align 3
> - .globl _trap_handler_rv32_hyp
> -_trap_handler_rv32_hyp:
> + .globl _trap_handler_hyp
> +_trap_handler_hyp:
> ? TRAP_SAVE_AND_SETUP_SP_T0
> ?
> +#if __riscv_xlen == 32
> ? TRAP_SAVE_MEPC_MSTATUS 1
> +#else
> + TRAP_SAVE_MEPC_MSTATUS 0
> +#endif
> ?
> ? TRAP_SAVE_GENERAL_REGS_EXCEPT_SP_T0
> ?
> +#if __riscv_xlen == 32
> + TRAP_SAVE_INFO 1 1
> +#else
> + TRAP_SAVE_INFO 0 1
> +#endif
> +
> ? TRAP_CALL_C_ROUTINE
> ?
> ? TRAP_RESTORE_GENERAL_REGS_EXCEPT_A0_T0
> ?
> +#if __riscv_xlen == 32
> ? TRAP_RESTORE_MEPC_MSTATUS 1
> +#else
> + TRAP_RESTORE_MEPC_MSTATUS 0
> +#endif
> ?
> ? TRAP_RESTORE_A0_T0
> ?
> ? mret
> -#endif
> ?
> ? .section .entry, "ax", %progbits
> ? .align 3
> diff --git a/include/sbi/sbi_trap.h b/include/sbi/sbi_trap.h
> index a6032ab..f7c170e 100644
> --- a/include/sbi/sbi_trap.h
> +++ b/include/sbi/sbi_trap.h
> @@ -112,9 +112,15 @@
> ?/** Size (in bytes) of sbi_trap_info */
> ?#define SBI_TRAP_INFO_SIZE SBI_TRAP_INFO_OFFSET(last)
> ?
> +/** Size (in bytes) of sbi_trap_context */
> +#define SBI_TRAP_CONTEXT_SIZE (SBI_TRAP_REGS_SIZE + \
> + ?????? SBI_TRAP_INFO_SIZE + \
> + ?????? __SIZEOF_POINTER__)
> +
> ?#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 {
> @@ -204,6 +210,16 @@ struct sbi_trap_info {
> ? unsigned long gva;
> ?};
> ?
> +/** Representation of trap context saved on stack */
> +struct sbi_trap_context {
> + /** Register state */
> + struct sbi_trap_regs regs;
> + /** Trap details */
> + struct sbi_trap_info trap;
> + /** Pointer to previous trap context */
> + struct sbi_trap_context *prev_context;
> +};
> +
> ?static inline unsigned long sbi_regs_gva(const struct sbi_trap_regs *regs)
> ?{
> ? /*
> @@ -223,7 +239,18 @@ 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);
> ?
> -struct sbi_trap_regs *sbi_trap_handler(struct sbi_trap_regs *regs);
> +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_context *sbi_trap_handler(struct sbi_trap_context *tcntx);
> ?
> ?#endif
> ?
> diff --git a/lib/sbi/sbi_trap.c b/lib/sbi/sbi_trap.c
> index b059c4b..4dbda06 100644
> --- a/lib/sbi/sbi_trap.c
> +++ b/lib/sbi/sbi_trap.c
> @@ -24,18 +24,18 @@
> ?#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)
> + ????? const struct sbi_trap_info *trap,
> + ????? const struct sbi_trap_regs *regs)
> ?{
> ? u32 hartid = current_hartid();
> ?
> ? 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);
> + ?? __func__, hartid, trap->cause, trap->tval);
> ? if (misa_extension('H')) {
> ? sbi_printf("%s: hart%d: mtval2=0x%" PRILX
> ? ?? " mtinst=0x%" PRILX "\n",
> - ?? __func__, hartid, mtval2, mtinst);
> + ?? __func__, hartid, trap->tval2, trap->tinst);
> ? }
> ? sbi_printf("%s: hart%d: mepc=0x%" PRILX " mstatus=0x%" PRILX "\n",
> ? ?? __func__, hartid, regs->mepc, regs->mstatus);
> @@ -258,20 +258,20 @@ static int sbi_trap_aia_irq(struct sbi_trap_regs *regs, ulong mcause)
> ? * 6. Stack pointer (SP) is setup for current HART
> ? * 7. Interrupts are disabled in MSTATUS CSR
> ? *
> - * @param regs pointer to register state
> + * @param tcntx pointer to trap context
> ? */
> -struct sbi_trap_regs *sbi_trap_handler(struct sbi_trap_regs *regs)
> +struct sbi_trap_context *sbi_trap_handler(struct sbi_trap_context *tcntx)
> ?{
> ? int rc = SBI_ENOTSUPP;
> ? const char *msg = "trap handler failed";
> - ulong mcause = csr_read(CSR_MCAUSE);
> - ulong mtval = csr_read(CSR_MTVAL), mtval2 = 0, mtinst = 0;
> - struct sbi_trap_info trap;
> + struct sbi_scratch *scratch = sbi_scratch_thishart_ptr();
> + const struct sbi_trap_info *trap = &tcntx->trap;
> + struct sbi_trap_regs *regs = &tcntx->regs;
> + ulong mcause = tcntx->trap.cause;
> ?
> - if (misa_extension('H')) {
> - mtval2 = csr_read(CSR_MTVAL2);
> - mtinst = csr_read(CSR_MTINST);
> - }
> + /* Update trap context pointer */
> + tcntx->prev_context = sbi_trap_get_context(scratch);
If the exception comes from S mode, tcntx will point to the same memory address.
At this time prev_context will be the same as tcntx and needs to be avoided.
Regards,
Xiang W
> + sbi_trap_set_context(scratch, tcntx);
> ?
> ? if (mcause & (1UL << (__riscv_xlen - 1))) {
> ? if (sbi_hart_has_extension(sbi_scratch_thishart_ptr(),
> @@ -279,32 +279,23 @@ struct sbi_trap_regs *sbi_trap_handler(struct sbi_trap_regs *regs)
> ? 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.cause = mcause;
> - trap.tval? = mtval;
> - trap.tval2 = mtval2;
> - trap.tinst = mtinst;
> - trap.gva?? = sbi_regs_gva(regs);
> ?
> ? switch (mcause) {
> ? case CAUSE_ILLEGAL_INSTRUCTION:
> - rc? = sbi_illegal_insn_handler(mtval, regs);
> + rc? = sbi_illegal_insn_handler(tcntx->trap.tval, regs);
> ? msg = "illegal instruction handler failed";
> ? 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(regs, trap);
> ? 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(regs, trap);
> ? msg = "misaligned store handler failed";
> ? break;
> ? case CAUSE_SUPERVISOR_ECALL:
> @@ -314,23 +305,24 @@ 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(regs, trap);
> ? 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(regs, trap);
> ? msg = "store fault handler failed";
> ? break;
> ? default:
> ? /* If the trap came from S or U mode, redirect it there */
> ? msg = "trap redirect failed";
> - rc? = sbi_trap_redirect(regs, &trap);
> + rc? = sbi_trap_redirect(regs, trap);
> ? break;
> ? }
> ?
> -trap_error:
> +trap_done:
> ? if (rc)
> - sbi_trap_error(msg, rc, mcause, mtval, mtval2, mtinst, regs);
> - return regs;
> + sbi_trap_error(msg, rc, trap, regs);
> + sbi_trap_set_context(scratch, tcntx->prev_context);
> + return tcntx;
> ?}
> --
> 2.34.1
>
>
^ permalink raw reply [flat|nested] 30+ messages in thread* [PATCH v2 04/10] lib: sbi: Introduce trap context
2024-03-13 4:56 ` Xiang W
@ 2024-03-17 11:23 ` Anup Patel
0 siblings, 0 replies; 30+ messages in thread
From: Anup Patel @ 2024-03-17 11:23 UTC (permalink / raw)
To: opensbi
On Wed, Mar 13, 2024 at 10:28?AM Xiang W <wxjstz@126.com> wrote:
>
> ? 2024-03-12???? 15:57 +0530?Anup Patel???
> > Club the struct sbi_trap_regs and struct sbi_trap_info a new
> > struct sbi_trap_context (aka trap context) which must be saved
> > by low-level trap handler before calling sbi_trap_handler().
> >
> > 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>
> > ---
> > firmware/fw_base.S | 62 ++++++++++++++++++++++++++++++++++--------
> > include/sbi/sbi_trap.h | 29 +++++++++++++++++++-
> > lib/sbi/sbi_trap.c | 58 +++++++++++++++++----------------------
> > 3 files changed, 103 insertions(+), 46 deletions(-)
> >
> > diff --git a/firmware/fw_base.S b/firmware/fw_base.S
> > index 539fd1d..967cca5 100644
> > --- a/firmware/fw_base.S
> > +++ b/firmware/fw_base.S
> > @@ -446,14 +446,12 @@ _start_warm:
> >
> > /* Setup trap handler */
> > lla a4, _trap_handler
> > -#if __riscv_xlen == 32
> > csrr a5, CSR_MISA
> > srli a5, a5, ('H' - 'A')
> > andi a5, a5, 0x1
> > - beq a5, zero, _skip_trap_handler_rv32_hyp
> > - lla a4, _trap_handler_rv32_hyp
> > -_skip_trap_handler_rv32_hyp:
> > -#endif
> > + beq a5, zero, _skip_trap_handler_hyp
> > + lla a4, _trap_handler_hyp
> > +_skip_trap_handler_hyp:
> > csrw CSR_MTVEC, a4
> >
> > /* Initialize SBI runtime */
> > @@ -564,10 +562,10 @@ memcmp:
> > xor t0, tp, t0
> >
> > /* Save original SP on exception stack */
> > - REG_S sp, (SBI_TRAP_REGS_OFFSET(sp) - SBI_TRAP_REGS_SIZE)(t0)
> > + REG_S sp, (SBI_TRAP_REGS_OFFSET(sp) - SBI_TRAP_CONTEXT_SIZE)(t0)
> >
> > - /* Set SP to exception stack and make room for trap registers */
> > - add sp, t0, -(SBI_TRAP_REGS_SIZE)
> > + /* Set SP to exception stack and make room for trap context */
> > + add sp, t0, -(SBI_TRAP_CONTEXT_SIZE)
> >
> > /* Restore T0 from scratch space */
> > REG_L t0, SBI_SCRATCH_TMP0_OFFSET(tp)
> > @@ -627,6 +625,32 @@ memcmp:
> > REG_S t6, SBI_TRAP_REGS_OFFSET(t6)(sp)
> > .endm
> >
> > +.macro TRAP_SAVE_INFO have_mstatush have_h_extension
> > + csrr t0, CSR_MCAUSE
> > + REG_S t0, (SBI_TRAP_REGS_SIZE + SBI_TRAP_INFO_OFFSET(cause))(sp)
> > + csrr t0, CSR_MTVAL
> > + REG_S t0, (SBI_TRAP_REGS_SIZE + SBI_TRAP_INFO_OFFSET(tval))(sp)
> > +.if \have_h_extension
> > + csrr t0, CSR_MTVAL2
> > + REG_S t0, (SBI_TRAP_REGS_SIZE + SBI_TRAP_INFO_OFFSET(tval2))(sp)
> > + csrr t0, CSR_MTINST
> > + REG_S t0, (SBI_TRAP_REGS_SIZE + SBI_TRAP_INFO_OFFSET(tinst))(sp)
> > + .if \have_mstatush
> > + csrr t0, CSR_MSTATUSH
> > + srli t0, t0, MSTATUSH_GVA_SHIFT
> > + .else
> > + csrr t0, CSR_MSTATUS
> > + srli t0, t0, MSTATUS_GVA_SHIFT
> > + .endif
> > + and t0, t0, 0x1
> > +.else
> > + REG_S zero, (SBI_TRAP_REGS_SIZE + SBI_TRAP_INFO_OFFSET(tval2))(sp)
> > + REG_S zero, (SBI_TRAP_REGS_SIZE + SBI_TRAP_INFO_OFFSET(tinst))(sp)
> > + add t0, zero, zero
> > +.endif
> > + REG_S t0, (SBI_TRAP_REGS_SIZE + SBI_TRAP_INFO_OFFSET(gva))(sp)
> > +.endm
> > +
> > .macro TRAP_CALL_C_ROUTINE
> > /* Call C routine */
> > add a0, sp, zero
> > @@ -696,6 +720,8 @@ _trap_handler:
> >
> > TRAP_SAVE_GENERAL_REGS_EXCEPT_SP_T0
> >
> > + TRAP_SAVE_INFO 0 0
> > +
> > TRAP_CALL_C_ROUTINE
> >
> > TRAP_RESTORE_GENERAL_REGS_EXCEPT_A0_T0
> > @@ -706,27 +732,39 @@ _trap_handler:
> >
> > mret
> >
> > -#if __riscv_xlen == 32
> > .section .entry, "ax", %progbits
> > .align 3
> > - .globl _trap_handler_rv32_hyp
> > -_trap_handler_rv32_hyp:
> > + .globl _trap_handler_hyp
> > +_trap_handler_hyp:
> > TRAP_SAVE_AND_SETUP_SP_T0
> >
> > +#if __riscv_xlen == 32
> > TRAP_SAVE_MEPC_MSTATUS 1
> > +#else
> > + TRAP_SAVE_MEPC_MSTATUS 0
> > +#endif
> >
> > TRAP_SAVE_GENERAL_REGS_EXCEPT_SP_T0
> >
> > +#if __riscv_xlen == 32
> > + TRAP_SAVE_INFO 1 1
> > +#else
> > + TRAP_SAVE_INFO 0 1
> > +#endif
> > +
> > TRAP_CALL_C_ROUTINE
> >
> > TRAP_RESTORE_GENERAL_REGS_EXCEPT_A0_T0
> >
> > +#if __riscv_xlen == 32
> > TRAP_RESTORE_MEPC_MSTATUS 1
> > +#else
> > + TRAP_RESTORE_MEPC_MSTATUS 0
> > +#endif
> >
> > TRAP_RESTORE_A0_T0
> >
> > mret
> > -#endif
> >
> > .section .entry, "ax", %progbits
> > .align 3
> > diff --git a/include/sbi/sbi_trap.h b/include/sbi/sbi_trap.h
> > index a6032ab..f7c170e 100644
> > --- a/include/sbi/sbi_trap.h
> > +++ b/include/sbi/sbi_trap.h
> > @@ -112,9 +112,15 @@
> > /** Size (in bytes) of sbi_trap_info */
> > #define SBI_TRAP_INFO_SIZE SBI_TRAP_INFO_OFFSET(last)
> >
> > +/** Size (in bytes) of sbi_trap_context */
> > +#define SBI_TRAP_CONTEXT_SIZE (SBI_TRAP_REGS_SIZE + \
> > + SBI_TRAP_INFO_SIZE + \
> > + __SIZEOF_POINTER__)
> > +
> > #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 {
> > @@ -204,6 +210,16 @@ struct sbi_trap_info {
> > unsigned long gva;
> > };
> >
> > +/** Representation of trap context saved on stack */
> > +struct sbi_trap_context {
> > + /** Register state */
> > + struct sbi_trap_regs regs;
> > + /** Trap details */
> > + struct sbi_trap_info trap;
> > + /** Pointer to previous trap context */
> > + struct sbi_trap_context *prev_context;
> > +};
> > +
> > static inline unsigned long sbi_regs_gva(const struct sbi_trap_regs *regs)
> > {
> > /*
> > @@ -223,7 +239,18 @@ 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);
> >
> > -struct sbi_trap_regs *sbi_trap_handler(struct sbi_trap_regs *regs);
> > +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_context *sbi_trap_handler(struct sbi_trap_context *tcntx);
> >
> > #endif
> >
> > diff --git a/lib/sbi/sbi_trap.c b/lib/sbi/sbi_trap.c
> > index b059c4b..4dbda06 100644
> > --- a/lib/sbi/sbi_trap.c
> > +++ b/lib/sbi/sbi_trap.c
> > @@ -24,18 +24,18 @@
> > #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)
> > + const struct sbi_trap_info *trap,
> > + const struct sbi_trap_regs *regs)
> > {
> > u32 hartid = current_hartid();
> >
> > 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);
> > + __func__, hartid, trap->cause, trap->tval);
> > if (misa_extension('H')) {
> > sbi_printf("%s: hart%d: mtval2=0x%" PRILX
> > " mtinst=0x%" PRILX "\n",
> > - __func__, hartid, mtval2, mtinst);
> > + __func__, hartid, trap->tval2, trap->tinst);
> > }
> > sbi_printf("%s: hart%d: mepc=0x%" PRILX " mstatus=0x%" PRILX "\n",
> > __func__, hartid, regs->mepc, regs->mstatus);
> > @@ -258,20 +258,20 @@ static int sbi_trap_aia_irq(struct sbi_trap_regs *regs, ulong mcause)
> > * 6. Stack pointer (SP) is setup for current HART
> > * 7. Interrupts are disabled in MSTATUS CSR
> > *
> > - * @param regs pointer to register state
> > + * @param tcntx pointer to trap context
> > */
> > -struct sbi_trap_regs *sbi_trap_handler(struct sbi_trap_regs *regs)
> > +struct sbi_trap_context *sbi_trap_handler(struct sbi_trap_context *tcntx)
> > {
> > int rc = SBI_ENOTSUPP;
> > const char *msg = "trap handler failed";
> > - ulong mcause = csr_read(CSR_MCAUSE);
> > - ulong mtval = csr_read(CSR_MTVAL), mtval2 = 0, mtinst = 0;
> > - struct sbi_trap_info trap;
> > + struct sbi_scratch *scratch = sbi_scratch_thishart_ptr();
> > + const struct sbi_trap_info *trap = &tcntx->trap;
> > + struct sbi_trap_regs *regs = &tcntx->regs;
> > + ulong mcause = tcntx->trap.cause;
> >
> > - if (misa_extension('H')) {
> > - mtval2 = csr_read(CSR_MTVAL2);
> > - mtinst = csr_read(CSR_MTINST);
> > - }
> > + /* Update trap context pointer */
> > + tcntx->prev_context = sbi_trap_get_context(scratch);
> If the exception comes from S mode, tcntx will point to the same memory address.
> At this time prev_context will be the same as tcntx and needs to be avoided.
When we come from S/U-mode, the trap context pointer on scratch is NULL
hence prev_context will be NULL as well.
Also, when we go back to S/U-mode, the trap context pointer on scratch is
set to prev_context again (i.e. NULL).
Regards,
Anup
>
> Regards,
> Xiang W
> > + sbi_trap_set_context(scratch, tcntx);
> >
> > if (mcause & (1UL << (__riscv_xlen - 1))) {
> > if (sbi_hart_has_extension(sbi_scratch_thishart_ptr(),
> > @@ -279,32 +279,23 @@ struct sbi_trap_regs *sbi_trap_handler(struct sbi_trap_regs *regs)
> > 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.cause = mcause;
> > - trap.tval = mtval;
> > - trap.tval2 = mtval2;
> > - trap.tinst = mtinst;
> > - trap.gva = sbi_regs_gva(regs);
> >
> > switch (mcause) {
> > case CAUSE_ILLEGAL_INSTRUCTION:
> > - rc = sbi_illegal_insn_handler(mtval, regs);
> > + rc = sbi_illegal_insn_handler(tcntx->trap.tval, regs);
> > msg = "illegal instruction handler failed";
> > 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(regs, trap);
> > 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(regs, trap);
> > msg = "misaligned store handler failed";
> > break;
> > case CAUSE_SUPERVISOR_ECALL:
> > @@ -314,23 +305,24 @@ 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(regs, trap);
> > 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(regs, trap);
> > msg = "store fault handler failed";
> > break;
> > default:
> > /* If the trap came from S or U mode, redirect it there */
> > msg = "trap redirect failed";
> > - rc = sbi_trap_redirect(regs, &trap);
> > + rc = sbi_trap_redirect(regs, trap);
> > break;
> > }
> >
> > -trap_error:
> > +trap_done:
> > if (rc)
> > - sbi_trap_error(msg, rc, mcause, mtval, mtval2, mtinst, regs);
> > - return regs;
> > + sbi_trap_error(msg, rc, trap, regs);
> > + sbi_trap_set_context(scratch, tcntx->prev_context);
> > + return tcntx;
> > }
> > --
> > 2.34.1
> >
> >
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 05/10] lib: sbi: Simplify parameters of misaligned and access fault handlers
2024-03-12 10:27 [PATCH v2 00/10] Improve trap handling for nested traps Anup Patel
` (3 preceding siblings ...)
2024-03-12 10:27 ` [PATCH v2 04/10] lib: sbi: Introduce trap context Anup Patel
@ 2024-03-12 10:27 ` Anup Patel
2024-03-12 16:55 ` Samuel Holland
2024-03-13 13:45 ` Clément Léger
2024-03-12 10:28 ` [PATCH v2 06/10] lib: sbi: Simplify parameters of sbi_illegal_insn_handler() Anup Patel
` (4 subsequent siblings)
9 siblings, 2 replies; 30+ messages in thread
From: Anup Patel @ 2024-03-12 10:27 UTC (permalink / raw)
To: opensbi
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 4dbda06..a2afb0a 100644
--- a/lib/sbi/sbi_trap.c
+++ b/lib/sbi/sbi_trap.c
@@ -290,12 +290,12 @@ struct sbi_trap_context *sbi_trap_handler(struct sbi_trap_context *tcntx)
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:
@@ -305,12 +305,12 @@ struct sbi_trap_context *sbi_trap_handler(struct sbi_trap_context *tcntx)
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 5a0537b..f9c0241 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;
@@ -150,8 +148,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;
@@ -169,10 +166,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;
@@ -254,8 +252,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;
@@ -265,9 +262,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;
@@ -283,17 +281,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;
@@ -309,17 +306,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;
@@ -332,16 +329,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;
@@ -354,8 +352,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
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH v2 05/10] lib: sbi: Simplify parameters of misaligned and access fault handlers
2024-03-12 10:27 ` [PATCH v2 05/10] lib: sbi: Simplify parameters of misaligned and access fault handlers Anup Patel
@ 2024-03-12 16:55 ` Samuel Holland
2024-03-13 13:45 ` Clément Léger
1 sibling, 0 replies; 30+ messages in thread
From: Samuel Holland @ 2024-03-12 16:55 UTC (permalink / raw)
To: opensbi
On 2024-03-12 5:27 AM, Anup Patel wrote:
> 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(-)
Reviewed-by: Samuel Holland <samuel.holland@sifive.com>
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 05/10] lib: sbi: Simplify parameters of misaligned and access fault handlers
2024-03-12 10:27 ` [PATCH v2 05/10] lib: sbi: Simplify parameters of misaligned and access fault handlers Anup Patel
2024-03-12 16:55 ` Samuel Holland
@ 2024-03-13 13:45 ` Clément Léger
1 sibling, 0 replies; 30+ messages in thread
From: Clément Léger @ 2024-03-13 13:45 UTC (permalink / raw)
To: opensbi
On 12/03/2024 11:27, Anup Patel wrote:
> 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 4dbda06..a2afb0a 100644
> --- a/lib/sbi/sbi_trap.c
> +++ b/lib/sbi/sbi_trap.c
> @@ -290,12 +290,12 @@ struct sbi_trap_context *sbi_trap_handler(struct sbi_trap_context *tcntx)
> 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:
> @@ -305,12 +305,12 @@ struct sbi_trap_context *sbi_trap_handler(struct sbi_trap_context *tcntx)
> 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 5a0537b..f9c0241 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;
> @@ -150,8 +148,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;
>
> @@ -169,10 +166,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;
> @@ -254,8 +252,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;
>
> @@ -265,9 +262,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;
>
> @@ -283,17 +281,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;
>
> @@ -309,17 +306,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;
> @@ -332,16 +329,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;
> @@ -354,8 +352,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);
> }
LGTM:
Reviewed-by: Cl?ment L?ger <cleger@rivosinc.com>
Thanks,
Cl?ment
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 06/10] lib: sbi: Simplify parameters of sbi_illegal_insn_handler()
2024-03-12 10:27 [PATCH v2 00/10] Improve trap handling for nested traps Anup Patel
` (4 preceding siblings ...)
2024-03-12 10:27 ` [PATCH v2 05/10] lib: sbi: Simplify parameters of misaligned and access fault handlers Anup Patel
@ 2024-03-12 10:28 ` Anup Patel
2024-03-12 16:56 ` Samuel Holland
2024-03-13 13:42 ` Clément Léger
2024-03-12 10:28 ` [PATCH v2 07/10] lib: sbi: Remove regs paramter of sbi_irqchip_process() Anup Patel
` (3 subsequent siblings)
9 siblings, 2 replies; 30+ messages in thread
From: Anup Patel @ 2024-03-12 10:28 UTC (permalink / raw)
To: opensbi
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, 10 insertions(+), 10 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 dd0b3c1..29c5d74 100644
--- a/lib/sbi/sbi_illegal_insn.c
+++ b/lib/sbi/sbi_illegal_insn.c
@@ -136,8 +136,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 insn = tcntx->trap.tval;
struct sbi_trap_info uptrap;
/*
@@ -153,13 +154,12 @@ 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);
- if (uptrap.cause) {
- return sbi_trap_redirect(regs, &uptrap);
- }
+ insn = sbi_get_insn(tcntx->regs.mepc, &uptrap);
+ if (uptrap.cause)
+ 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 a2afb0a..4e691df 100644
--- a/lib/sbi/sbi_trap.c
+++ b/lib/sbi/sbi_trap.c
@@ -285,7 +285,7 @@ struct sbi_trap_context *sbi_trap_handler(struct sbi_trap_context *tcntx)
switch (mcause) {
case CAUSE_ILLEGAL_INSTRUCTION:
- rc = sbi_illegal_insn_handler(tcntx->trap.tval, regs);
+ rc = sbi_illegal_insn_handler(tcntx);
msg = "illegal instruction handler failed";
break;
case CAUSE_MISALIGNED_LOAD:
--
2.34.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH v2 06/10] lib: sbi: Simplify parameters of sbi_illegal_insn_handler()
2024-03-12 10:28 ` [PATCH v2 06/10] lib: sbi: Simplify parameters of sbi_illegal_insn_handler() Anup Patel
@ 2024-03-12 16:56 ` Samuel Holland
2024-03-13 13:42 ` Clément Léger
1 sibling, 0 replies; 30+ messages in thread
From: Samuel Holland @ 2024-03-12 16:56 UTC (permalink / raw)
To: opensbi
On 2024-03-12 5:28 AM, Anup Patel wrote:
> 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, 10 insertions(+), 10 deletions(-)
Reviewed-by: Samuel Holland <samuel.holland@sifive.com>
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 06/10] lib: sbi: Simplify parameters of sbi_illegal_insn_handler()
2024-03-12 10:28 ` [PATCH v2 06/10] lib: sbi: Simplify parameters of sbi_illegal_insn_handler() Anup Patel
2024-03-12 16:56 ` Samuel Holland
@ 2024-03-13 13:42 ` Clément Léger
2024-03-17 11:28 ` Anup Patel
1 sibling, 1 reply; 30+ messages in thread
From: Clément Léger @ 2024-03-13 13:42 UTC (permalink / raw)
To: opensbi
On 12/03/2024 11:28, Anup Patel wrote:
> 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, 10 insertions(+), 10 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 dd0b3c1..29c5d74 100644
> --- a/lib/sbi/sbi_illegal_insn.c
> +++ b/lib/sbi/sbi_illegal_insn.c
> @@ -136,8 +136,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 insn = tcntx->trap.tval;
Maybe add some intermediate variable:
struct sbi_trap_regs *regs = &tcntx->regs;
So you won't have to modify anything below.
Regards,
Cl?ment
> struct sbi_trap_info uptrap;
>
> /*
> @@ -153,13 +154,12 @@ 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);
> - if (uptrap.cause) {
> - return sbi_trap_redirect(regs, &uptrap);
> - }
> + insn = sbi_get_insn(tcntx->regs.mepc, &uptrap);
> + if (uptrap.cause)
> + 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 a2afb0a..4e691df 100644
> --- a/lib/sbi/sbi_trap.c
> +++ b/lib/sbi/sbi_trap.c
> @@ -285,7 +285,7 @@ struct sbi_trap_context *sbi_trap_handler(struct sbi_trap_context *tcntx)
>
> switch (mcause) {
> case CAUSE_ILLEGAL_INSTRUCTION:
> - rc = sbi_illegal_insn_handler(tcntx->trap.tval, regs);
> + rc = sbi_illegal_insn_handler(tcntx);
> msg = "illegal instruction handler failed";
> break;
> case CAUSE_MISALIGNED_LOAD:
^ permalink raw reply [flat|nested] 30+ messages in thread* [PATCH v2 06/10] lib: sbi: Simplify parameters of sbi_illegal_insn_handler()
2024-03-13 13:42 ` Clément Léger
@ 2024-03-17 11:28 ` Anup Patel
0 siblings, 0 replies; 30+ messages in thread
From: Anup Patel @ 2024-03-17 11:28 UTC (permalink / raw)
To: opensbi
On Wed, Mar 13, 2024 at 7:12?PM Cl?ment L?ger <cleger@rivosinc.com> wrote:
>
>
>
> On 12/03/2024 11:28, Anup Patel wrote:
> > 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, 10 insertions(+), 10 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 dd0b3c1..29c5d74 100644
> > --- a/lib/sbi/sbi_illegal_insn.c
> > +++ b/lib/sbi/sbi_illegal_insn.c
> > @@ -136,8 +136,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 insn = tcntx->trap.tval;
>
> Maybe add some intermediate variable:
>
> struct sbi_trap_regs *regs = &tcntx->regs;
>
> So you won't have to modify anything below.
OKay, I will update.
Regards,
Anup
>
> Regards,
>
> Cl?ment
>
> > struct sbi_trap_info uptrap;
> >
> > /*
> > @@ -153,13 +154,12 @@ 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);
> > - if (uptrap.cause) {
> > - return sbi_trap_redirect(regs, &uptrap);
> > - }
> > + insn = sbi_get_insn(tcntx->regs.mepc, &uptrap);
> > + if (uptrap.cause)
> > + 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 a2afb0a..4e691df 100644
> > --- a/lib/sbi/sbi_trap.c
> > +++ b/lib/sbi/sbi_trap.c
> > @@ -285,7 +285,7 @@ struct sbi_trap_context *sbi_trap_handler(struct sbi_trap_context *tcntx)
> >
> > switch (mcause) {
> > case CAUSE_ILLEGAL_INSTRUCTION:
> > - rc = sbi_illegal_insn_handler(tcntx->trap.tval, regs);
> > + rc = sbi_illegal_insn_handler(tcntx);
> > msg = "illegal instruction handler failed";
> > break;
> > case CAUSE_MISALIGNED_LOAD:
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 07/10] lib: sbi: Remove regs paramter of sbi_irqchip_process()
2024-03-12 10:27 [PATCH v2 00/10] Improve trap handling for nested traps Anup Patel
` (5 preceding siblings ...)
2024-03-12 10:28 ` [PATCH v2 06/10] lib: sbi: Simplify parameters of sbi_illegal_insn_handler() Anup Patel
@ 2024-03-12 10:28 ` Anup Patel
2024-03-12 16:56 ` Samuel Holland
2024-03-13 13:39 ` Clément Léger
2024-03-12 10:28 ` [PATCH v2 08/10] lib: sbi: Remove regs parameter from trap irq handling functions Anup Patel
` (2 subsequent siblings)
9 siblings, 2 replies; 30+ messages in thread
From: Anup Patel @ 2024-03-12 10:28 UTC (permalink / raw)
To: opensbi
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 4e691df..72b1788 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
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH v2 07/10] lib: sbi: Remove regs paramter of sbi_irqchip_process()
2024-03-12 10:28 ` [PATCH v2 07/10] lib: sbi: Remove regs paramter of sbi_irqchip_process() Anup Patel
@ 2024-03-12 16:56 ` Samuel Holland
2024-03-13 13:39 ` Clément Léger
1 sibling, 0 replies; 30+ messages in thread
From: Samuel Holland @ 2024-03-12 16:56 UTC (permalink / raw)
To: opensbi
On 2024-03-12 5:28 AM, Anup Patel wrote:
> 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(-)
Reviewed-by: Samuel Holland <samuel.holland@sifive.com>
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 07/10] lib: sbi: Remove regs paramter of sbi_irqchip_process()
2024-03-12 10:28 ` [PATCH v2 07/10] lib: sbi: Remove regs paramter of sbi_irqchip_process() Anup Patel
2024-03-12 16:56 ` Samuel Holland
@ 2024-03-13 13:39 ` Clément Léger
2024-03-17 11:30 ` Anup Patel
1 sibling, 1 reply; 30+ messages in thread
From: Clément Léger @ 2024-03-13 13:39 UTC (permalink / raw)
To: opensbi
On 12/03/2024 11:28, Anup Patel wrote:
> The irqchip handlers will typically not need pointer to trap registers
> so remove regs paramter of sbi_irqchip_process().
Hi Anup,
Typo: "paramter" -> "parameter"
>
> 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 4e691df..72b1788 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;
>
Other than that, LGTM:
Reviewed-by: Cl?ment L?ger <cleger@rivosinc.com>
^ permalink raw reply [flat|nested] 30+ messages in thread* [PATCH v2 07/10] lib: sbi: Remove regs paramter of sbi_irqchip_process()
2024-03-13 13:39 ` Clément Léger
@ 2024-03-17 11:30 ` Anup Patel
0 siblings, 0 replies; 30+ messages in thread
From: Anup Patel @ 2024-03-17 11:30 UTC (permalink / raw)
To: opensbi
On Wed, Mar 13, 2024 at 7:09?PM Cl?ment L?ger <cleger@rivosinc.com> wrote:
>
>
>
> On 12/03/2024 11:28, Anup Patel wrote:
> > The irqchip handlers will typically not need pointer to trap registers
> > so remove regs paramter of sbi_irqchip_process().
>
> Hi Anup,
>
> Typo: "paramter" -> "parameter"
Okay, I will update.
Regards,
Anup
>
> >
> > 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 4e691df..72b1788 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;
> >
>
> Other than that, LGTM:
>
> Reviewed-by: Cl?ment L?ger <cleger@rivosinc.com>
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 08/10] lib: sbi: Remove regs parameter from trap irq handling functions
2024-03-12 10:27 [PATCH v2 00/10] Improve trap handling for nested traps Anup Patel
` (6 preceding siblings ...)
2024-03-12 10:28 ` [PATCH v2 07/10] lib: sbi: Remove regs paramter of sbi_irqchip_process() Anup Patel
@ 2024-03-12 10:28 ` Anup Patel
2024-03-12 16:56 ` Samuel Holland
2024-03-13 13:51 ` Clément Léger
2024-03-12 10:28 ` [PATCH v2 09/10] lib: sbi: Pass trap context pointer to sbi_ecall_handler() Anup Patel
2024-03-12 10:28 ` [PATCH v2 10/10] lib: sbi: Extend sbi_trap_error() to dump state in a nested trap Anup Patel
9 siblings, 2 replies; 30+ messages in thread
From: Anup Patel @ 2024-03-12 10:28 UTC (permalink / raw)
To: opensbi
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 | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/lib/sbi/sbi_trap.c b/lib/sbi/sbi_trap.c
index 72b1788..ebf454d 100644
--- a/lib/sbi/sbi_trap.c
+++ b/lib/sbi/sbi_trap.c
@@ -198,10 +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 irq)
{
- mcause &= ~(1UL << (__riscv_xlen - 1));
- switch (mcause) {
+ switch (irq) {
case IRQ_M_TIMER:
sbi_timer_process();
break;
@@ -217,7 +216,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(void)
{
int rc;
unsigned long mtopi;
@@ -273,12 +272,12 @@ struct sbi_trap_context *sbi_trap_handler(struct sbi_trap_context *tcntx)
tcntx->prev_context = sbi_trap_get_context(scratch);
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();
else
- rc = sbi_trap_nonaia_irq(regs, mcause);
+ rc = sbi_trap_nonaia_irq(mcause & ~BIT(__riscv_xlen - 1));
msg = "unhandled local interrupt";
goto trap_done;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH v2 08/10] lib: sbi: Remove regs parameter from trap irq handling functions
2024-03-12 10:28 ` [PATCH v2 08/10] lib: sbi: Remove regs parameter from trap irq handling functions Anup Patel
@ 2024-03-12 16:56 ` Samuel Holland
2024-03-13 13:51 ` Clément Léger
1 sibling, 0 replies; 30+ messages in thread
From: Samuel Holland @ 2024-03-12 16:56 UTC (permalink / raw)
To: opensbi
On 2024-03-12 5:28 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 | 13 ++++++-------
> 1 file changed, 6 insertions(+), 7 deletions(-)
Reviewed-by: Samuel Holland <samuel.holland@sifive.com>
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 08/10] lib: sbi: Remove regs parameter from trap irq handling functions
2024-03-12 10:28 ` [PATCH v2 08/10] lib: sbi: Remove regs parameter from trap irq handling functions Anup Patel
2024-03-12 16:56 ` Samuel Holland
@ 2024-03-13 13:51 ` Clément Léger
2024-03-17 11:36 ` Anup Patel
1 sibling, 1 reply; 30+ messages in thread
From: Clément Léger @ 2024-03-13 13:51 UTC (permalink / raw)
To: opensbi
On 12/03/2024 11:28, 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 | 13 ++++++-------
> 1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/lib/sbi/sbi_trap.c b/lib/sbi/sbi_trap.c
> index 72b1788..ebf454d 100644
> --- a/lib/sbi/sbi_trap.c
> +++ b/lib/sbi/sbi_trap.c
> @@ -198,10 +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 irq)
> {
> - mcause &= ~(1UL << (__riscv_xlen - 1));
> - switch (mcause) {
> + switch (irq) {
> case IRQ_M_TIMER:
> sbi_timer_process();
> break;
> @@ -217,7 +216,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(void)
> {
> int rc;
> unsigned long mtopi;
> @@ -273,12 +272,12 @@ struct sbi_trap_context *sbi_trap_handler(struct sbi_trap_context *tcntx)
> tcntx->prev_context = sbi_trap_get_context(scratch);
> sbi_trap_set_context(scratch, tcntx);
>
> - if (mcause & (1UL << (__riscv_xlen - 1))) {
> + if (mcause & BIT(__riscv_xlen - 1)) {
Hi Anup,
You could probably add a define for BIT(__riscv_xlen - 1) such as
MCAUSE_IRQ_MASK.
Thanks,
Cl?ment
> 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();
> else
> - rc = sbi_trap_nonaia_irq(regs, mcause);
> + rc = sbi_trap_nonaia_irq(mcause & ~BIT(__riscv_xlen - 1));
> msg = "unhandled local interrupt";
> goto trap_done;
> }
^ permalink raw reply [flat|nested] 30+ messages in thread* [PATCH v2 08/10] lib: sbi: Remove regs parameter from trap irq handling functions
2024-03-13 13:51 ` Clément Léger
@ 2024-03-17 11:36 ` Anup Patel
0 siblings, 0 replies; 30+ messages in thread
From: Anup Patel @ 2024-03-17 11:36 UTC (permalink / raw)
To: opensbi
On Wed, Mar 13, 2024 at 7:21?PM Cl?ment L?ger <cleger@rivosinc.com> wrote:
>
>
>
> On 12/03/2024 11:28, 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 | 13 ++++++-------
> > 1 file changed, 6 insertions(+), 7 deletions(-)
> >
> > diff --git a/lib/sbi/sbi_trap.c b/lib/sbi/sbi_trap.c
> > index 72b1788..ebf454d 100644
> > --- a/lib/sbi/sbi_trap.c
> > +++ b/lib/sbi/sbi_trap.c
> > @@ -198,10 +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 irq)
> > {
> > - mcause &= ~(1UL << (__riscv_xlen - 1));
> > - switch (mcause) {
> > + switch (irq) {
> > case IRQ_M_TIMER:
> > sbi_timer_process();
> > break;
> > @@ -217,7 +216,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(void)
> > {
> > int rc;
> > unsigned long mtopi;
> > @@ -273,12 +272,12 @@ struct sbi_trap_context *sbi_trap_handler(struct sbi_trap_context *tcntx)
> > tcntx->prev_context = sbi_trap_get_context(scratch);
> > sbi_trap_set_context(scratch, tcntx);
> >
> > - if (mcause & (1UL << (__riscv_xlen - 1))) {
> > + if (mcause & BIT(__riscv_xlen - 1)) {
>
> Hi Anup,
>
> You could probably add a define for BIT(__riscv_xlen - 1) such as
> MCAUSE_IRQ_MASK.
Okay, I will update.
Regards,
Anup
>
> Thanks,
>
> Cl?ment
>
> > 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();
> > else
> > - rc = sbi_trap_nonaia_irq(regs, mcause);
> > + rc = sbi_trap_nonaia_irq(mcause & ~BIT(__riscv_xlen - 1));
> > msg = "unhandled local interrupt";
> > goto trap_done;
> > }
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 09/10] lib: sbi: Pass trap context pointer to sbi_ecall_handler()
2024-03-12 10:27 [PATCH v2 00/10] Improve trap handling for nested traps Anup Patel
` (7 preceding siblings ...)
2024-03-12 10:28 ` [PATCH v2 08/10] lib: sbi: Remove regs parameter from trap irq handling functions Anup Patel
@ 2024-03-12 10:28 ` Anup Patel
2024-03-12 16:56 ` Samuel Holland
2024-03-13 13:47 ` Clément Léger
2024-03-12 10:28 ` [PATCH v2 10/10] lib: sbi: Extend sbi_trap_error() to dump state in a nested trap Anup Patel
9 siblings, 2 replies; 30+ messages in thread
From: Anup Patel @ 2024-03-12 10:28 UTC (permalink / raw)
To: opensbi
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..d4fc58c 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 ebf454d..0b35d1a 100644
--- a/lib/sbi/sbi_trap.c
+++ b/lib/sbi/sbi_trap.c
@@ -299,7 +299,7 @@ struct sbi_trap_context *sbi_trap_handler(struct sbi_trap_context *tcntx)
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
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH v2 09/10] lib: sbi: Pass trap context pointer to sbi_ecall_handler()
2024-03-12 10:28 ` [PATCH v2 09/10] lib: sbi: Pass trap context pointer to sbi_ecall_handler() Anup Patel
@ 2024-03-12 16:56 ` Samuel Holland
2024-03-13 13:47 ` Clément Léger
1 sibling, 0 replies; 30+ messages in thread
From: Samuel Holland @ 2024-03-12 16:56 UTC (permalink / raw)
To: opensbi
On 2024-03-12 5:28 AM, Anup Patel wrote:
> 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(-)
Reviewed-by: Samuel Holland <samuel.holland@sifive.com>
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 09/10] lib: sbi: Pass trap context pointer to sbi_ecall_handler()
2024-03-12 10:28 ` [PATCH v2 09/10] lib: sbi: Pass trap context pointer to sbi_ecall_handler() Anup Patel
2024-03-12 16:56 ` Samuel Holland
@ 2024-03-13 13:47 ` Clément Léger
1 sibling, 0 replies; 30+ messages in thread
From: Clément Léger @ 2024-03-13 13:47 UTC (permalink / raw)
To: opensbi
On 12/03/2024 11:28, Anup Patel wrote:
> 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..d4fc58c 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 ebf454d..0b35d1a 100644
> --- a/lib/sbi/sbi_trap.c
> +++ b/lib/sbi/sbi_trap.c
> @@ -299,7 +299,7 @@ struct sbi_trap_context *sbi_trap_handler(struct sbi_trap_context *tcntx)
> 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:
Reviewed-by: Cl?ment L?ger <cleger@rivosinc.com>
Thanks,
Cl?ment
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 10/10] lib: sbi: Extend sbi_trap_error() to dump state in a nested trap
2024-03-12 10:27 [PATCH v2 00/10] Improve trap handling for nested traps Anup Patel
` (8 preceding siblings ...)
2024-03-12 10:28 ` [PATCH v2 09/10] lib: sbi: Pass trap context pointer to sbi_ecall_handler() Anup Patel
@ 2024-03-12 10:28 ` Anup Patel
2024-03-12 17:21 ` Samuel Holland
9 siblings, 1 reply; 30+ messages in thread
From: Anup Patel @ 2024-03-12 10:28 UTC (permalink / raw)
To: opensbi
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 0b35d1a..83598a4 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,
- const struct sbi_trap_info *trap,
- const 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, trap->cause, trap->tval);
+ 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, trap->tval2, trap->tinst);
+ 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();
}
@@ -321,7 +337,7 @@ struct sbi_trap_context *sbi_trap_handler(struct sbi_trap_context *tcntx)
trap_done:
if (rc)
- sbi_trap_error(msg, rc, trap, regs);
+ sbi_trap_error(msg, rc, tcntx);
sbi_trap_set_context(scratch, tcntx->prev_context);
return tcntx;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH v2 10/10] lib: sbi: Extend sbi_trap_error() to dump state in a nested trap
2024-03-12 10:28 ` [PATCH v2 10/10] lib: sbi: Extend sbi_trap_error() to dump state in a nested trap Anup Patel
@ 2024-03-12 17:21 ` Samuel Holland
2024-03-17 11:55 ` Anup Patel
0 siblings, 1 reply; 30+ messages in thread
From: Samuel Holland @ 2024-03-12 17:21 UTC (permalink / raw)
To: opensbi
On 2024-03-12 5:28 AM, Anup Patel wrote:
> 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 0b35d1a..83598a4 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,
> - const struct sbi_trap_info *trap,
> - const 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;
It looks like you are constifying these where possible; the whole tcntx can be
const here and in sbi_trap_error().
>
> - 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, trap->cause, trap->tval);
> + 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, trap->tval2, trap->tinst);
> + 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);
All of the CSR printf lines should use the same format string as the GPRs below.
This saves 120 bytes of .rodata at the cost of 32 bytes of .text.
For the series:
Tested-by: Samuel Holland <samuel.holland@sifive.com>
including by sticking an ebreak in the middle of the load/store emulation code.
Regards,
Samuel
> + 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();
> }
> @@ -321,7 +337,7 @@ struct sbi_trap_context *sbi_trap_handler(struct sbi_trap_context *tcntx)
>
> trap_done:
> if (rc)
> - sbi_trap_error(msg, rc, trap, regs);
> + sbi_trap_error(msg, rc, tcntx);
> sbi_trap_set_context(scratch, tcntx->prev_context);
> return tcntx;
> }
^ permalink raw reply [flat|nested] 30+ messages in thread* [PATCH v2 10/10] lib: sbi: Extend sbi_trap_error() to dump state in a nested trap
2024-03-12 17:21 ` Samuel Holland
@ 2024-03-17 11:55 ` Anup Patel
0 siblings, 0 replies; 30+ messages in thread
From: Anup Patel @ 2024-03-17 11:55 UTC (permalink / raw)
To: opensbi
On Tue, Mar 12, 2024 at 10:51?PM Samuel Holland
<samuel.holland@sifive.com> wrote:
>
> On 2024-03-12 5:28 AM, Anup Patel wrote:
> > 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 0b35d1a..83598a4 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,
> > - const struct sbi_trap_info *trap,
> > - const 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;
>
> It looks like you are constifying these where possible; the whole tcntx can be
> const here and in sbi_trap_error().
Sure, I will update.
>
> >
> > - 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, trap->cause, trap->tval);
> > + 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, trap->tval2, trap->tinst);
> > + 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);
>
> All of the CSR printf lines should use the same format string as the GPRs below.
> This saves 120 bytes of .rodata at the cost of 32 bytes of .text.
Sure, I will update.
>
> For the series:
>
> Tested-by: Samuel Holland <samuel.holland@sifive.com>
>
> including by sticking an ebreak in the middle of the load/store emulation code.
Thanks for testing.
Regards,
Anup
>
> Regards,
> Samuel
>
> > + 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();
> > }
> > @@ -321,7 +337,7 @@ struct sbi_trap_context *sbi_trap_handler(struct sbi_trap_context *tcntx)
> >
> > trap_done:
> > if (rc)
> > - sbi_trap_error(msg, rc, trap, regs);
> > + sbi_trap_error(msg, rc, tcntx);
> > sbi_trap_set_context(scratch, tcntx->prev_context);
> > return tcntx;
> > }
>
^ permalink raw reply [flat|nested] 30+ messages in thread