[PATCH 0/2] lib: sbi: Fix Smrnmi/RNMI trap handling issues

[PATCH 0/2] lib: sbi: Fix Smrnmi/RNMI trap handling issues

[Nia Su]

This series fixes two issues in the Smrnmi extension and RNMI trap
handling code:

Patch 1 moves the Smrnmi CSR setup into a dedicated
sbi_smrnmi_hart_init() helper so it also runs on the non-retentive
suspend resume path (sbi_hart_reinit()), and relaxes
smrnmi_handlers_init to allow a NULL callback and to report failure
via an int return value instead of void.

Patch 2 fixes sbi_trap_rnmi_handler(), which never updates
prev_context and can inherit a stale value from an earlier trap
that reused the same M-mode exception-stack slot. This can make
sbi_trap_error() loop indefinitely instead of printing diagnostics.
The fix maintains the trap-context pointer the same way
sbi_trap_handler() does, so nested contexts always link correctly.

Signed-off-by: Nia Su <nia.su@sifive.com>
---
Nia Su (1):
      lib: sbi: Fix stale prev_context in RNMI handler

Nylon Chen (1):
      lib: sbi: Fix Smrnmi init and non-retentive suspend handling

 include/sbi/sbi_hart.h     |  1 +
 include/sbi/sbi_platform.h |  2 +-
 lib/sbi/sbi_hart.c         | 55 +++++++++++++++++++++++++++++-----------------
 lib/sbi/sbi_trap.c         | 10 +++++++--
 4 files changed, 45 insertions(+), 23 deletions(-)
---
base-commit: 4e79fd7de59f1b2899092c1a84ce68c8ebc68f93
change-id: 20260826-rnmi-trap-fixes-91347b53b0dd

--


-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

[PATCH 1/2] lib: sbi: Fix Smrnmi init and non-retentive suspend handling

[Nia Su]

From: Nylon Chen <nylon.chen@sifive.com>

Two fixes for the Smrnmi extension infrastructure:

1. Extract Smrnmi CSR setup into sbi_smrnmi_hart_init() and call it
   from both hart_detect_features() (early, before trap-based probing)
   and sbi_hart_reinit() (covers non-retentive suspend resume via
   init_warm_resume()).  This removes the need for save/restore of
   CSR_MNSCRATCH and MNSTATUS in sbi_hsm_data.

2. Change smrnmi_handlers_init callback return type from void to int.
   Allow NULL callback for platforms that do not need to program a
   vendor-specific NMI vector register.  Only set MNSTATUS.NMIE after
   the callback succeeds.

Fixes: 2d211fe6f9d5 ("lib: sbi: hart: Detect and enable Smrnmi before trap-based feature detection")

Signed-off-by: Nylon Chen <nylon.chen@sifive.com>
Co-developed-by: Nia Su <nia.su@sifive.com>
Signed-off-by: Nia Su <nia.su@sifive.com>
---
 include/sbi/sbi_hart.h     |  1 +
 include/sbi/sbi_platform.h |  2 +-
 lib/sbi/sbi_hart.c         | 55 +++++++++++++++++++++++++++++-----------------
 3 files changed, 37 insertions(+), 21 deletions(-)

diff --git a/include/sbi/sbi_hart.h b/include/sbi/sbi_hart.h
index 543393bba41526017cd95596348e26eaa897850a..03f12717572f402d77e4e78f8d29c93a5fa0343d 100644
--- a/include/sbi/sbi_hart.h
+++ b/include/sbi/sbi_hart.h
@@ -132,6 +132,7 @@ extern unsigned long hart_features_offset;
 
 struct sbi_scratch;
 
+int sbi_smrnmi_hart_init(struct sbi_scratch *scratch);
 int sbi_hart_reinit(struct sbi_scratch *scratch);
 int sbi_hart_init(struct sbi_scratch *scratch, bool cold_boot);
 
diff --git a/include/sbi/sbi_platform.h b/include/sbi/sbi_platform.h
index 1e9a23c1252558879c683633ad000828fbfa8f3f..d55805defafb0a7935d0d95653aefcc622078f69 100644
--- a/include/sbi/sbi_platform.h
+++ b/include/sbi/sbi_platform.h
@@ -155,7 +155,7 @@ struct sbi_platform_operations {
 	void (*pmp_disable)(unsigned int n);
 
 	/** platform specific Smrnmi handlers init on current HART */
-	void (*smrnmi_handlers_init)(void (*rnmi_handler)(void),
+	int (*smrnmi_handlers_init)(void (*rnmi_handler)(void),
 			void (*rnme_handler)(void));
 
 	/** platform specific Smrnmi NMI handler.
diff --git a/lib/sbi/sbi_hart.c b/lib/sbi/sbi_hart.c
index bee8855772d8bfe452a1f9f086ece2a7ab8bbe6b..287d7ba7c4e110d77b3cb68929a6309e15ccd7fe 100644
--- a/lib/sbi/sbi_hart.c
+++ b/lib/sbi/sbi_hart.c
@@ -466,6 +466,37 @@ static int hart_mhpm_get_allowed_bits(void)
 	return num_bits;
 }
 
+int sbi_smrnmi_hart_init(struct sbi_scratch *scratch)
+{
+	extern void _trap_rnmi_handler(void);
+	extern void _trap_handler(void);
+	const struct sbi_platform *plat;
+	const struct sbi_platform_operations *ops;
+	int ret;
+
+	if (!sbi_hart_has_extension(scratch, SBI_HART_EXT_SMRNMI))
+		return 0;
+
+	plat = sbi_platform_thishart_ptr();
+	ops  = plat ? sbi_platform_ops(plat) : NULL;
+
+	/*
+	 * Platforms with fixed or mtvec-based NMI vectors need no
+	 * vendor register programming; NULL callback is valid.
+	 */
+	if (ops && ops->smrnmi_handlers_init) {
+		ret = ops->smrnmi_handlers_init(_trap_rnmi_handler,
+						_trap_handler);
+		if (ret)
+			return ret;
+	}
+
+	csr_write(CSR_MNSCRATCH, scratch);
+	csr_set(CSR_MNSTATUS, MNSTATUS_NMIE);
+
+	return 0;
+}
+
 static int hart_detect_features(struct sbi_scratch *scratch, bool cold_boot)
 {
 	struct sbi_trap_info trap = {0};
@@ -487,25 +518,9 @@ static int hart_detect_features(struct sbi_scratch *scratch, bool cold_boot)
 	if (rc)
 		return rc;
 
-	if (sbi_hart_has_extension(scratch, SBI_HART_EXT_SMRNMI)) {
-		const struct sbi_platform *plat = sbi_platform_thishart_ptr();
-		const struct sbi_platform_operations *ops = sbi_platform_ops(plat);
-		extern void _trap_rnmi_handler(void);
-		extern void _trap_handler(void);
-
-		if (!ops || !ops->smrnmi_handlers_init)
-			sbi_panic("Smrnmi detected, but platform lacks smrnmi_handlers_init callback\n");
-
-		/* Reuse _trap_handler for the RNME slot since RNME is taken
-		 * as a regular M-mode trap with NMIE=0. */
-		ops->smrnmi_handlers_init(_trap_rnmi_handler, _trap_handler);
-
-		/* Initialize MNSCRATCH for the RNMI handler */
-		csr_write(CSR_MNSCRATCH, scratch);
-
-		/* Enable NMIs */
-		csr_set(CSR_MNSTATUS, MNSTATUS_NMIE);
-	}
+	rc = sbi_smrnmi_hart_init(scratch);
+	if (rc)
+		return rc;
 
 #define __check_hpm_csr(__csr, __mask) 					  \
 	oldval = csr_read_allowed(__csr, &trap);			  \
@@ -698,7 +713,7 @@ int sbi_hart_reinit(struct sbi_scratch *scratch)
 	if (rc)
 		return rc;
 
-	return 0;
+	return sbi_smrnmi_hart_init(scratch);
 }
 
 int sbi_hart_init(struct sbi_scratch *scratch, bool cold_boot)

-- 
2.43.7


-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

[PATCH 2/2] lib: sbi: Fix stale prev_context in RNMI handler

[Nia Su]

Before an RNMI is taken, an earlier SBI trap may have already used
the same slot on the M-mode exception stack to save its trap context.
When the RNMI later comes in, it reuses that same stack slot for its
own trap context, but sbi_trap_rnmi_handler() never links prev_context
to the previously active trap context, so it still holds the value
left there by that earlier trap context.

sbi_trap_error() then walks this chain with
`for (tc = tcntx; tc; tc = tc->prev_context) depth++;` to count the
depth before printing anything. If this stale prev_context happens
to point back to tcntx itself, the loop never terminates, hanging
the hart before any diagnostics are even printed.

Update sbi_trap_rnmi_handler() so that sbi_scratch points to the
new trap context on entry, and points back to the previous context
on exit, the same way sbi_trap_handler() does. This serves two
purposes: it makes tcntx->prev_context always point to the correct
previous trap context (or NULL when there isn't one) instead of
stale data, fixing the hang described above; and it lets an
exception taken while already inside RNMI handling correctly chain
back to the RNMI's trap context, instead of the RNMI context being
silently dropped from the chain and never printed. Together, these
let sbi_trap_error() print every nested trap context in the chain,
including the RNMI's own state.

Fixes: 8cdb5b1023df ("firmware: Add RNMI handler infrastructure")
Suggested-by: Nick Hu <nick.hu@sifive.com>
Signed-off-by: Nia Su <nia.su@sifive.com>
---
 lib/sbi/sbi_trap.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/lib/sbi/sbi_trap.c b/lib/sbi/sbi_trap.c
index 16774fd2f73059c1268bfb9f43fdf8e9c460c0b6..e694188c25b7b1715df660206af0a501290389fc 100644
--- a/lib/sbi/sbi_trap.c
+++ b/lib/sbi/sbi_trap.c
@@ -394,9 +394,14 @@ trap_done:
 struct sbi_trap_context *sbi_trap_rnmi_handler(struct sbi_trap_context *tcntx)
 {
 	int rc;
+	struct sbi_scratch *scratch = sbi_scratch_thishart_ptr();
 	const struct sbi_platform *plat = sbi_platform_thishart_ptr();
 	const struct sbi_platform_operations *ops = sbi_platform_ops(plat);
 
+	/* Update trap context pointer so nested traps chain correctly */
+	tcntx->prev_context = sbi_trap_get_context(scratch);
+	sbi_trap_set_context(scratch, tcntx);
+
 	/* Call platform-specific NMI handler if registered */
 	if (ops && ops->rnmi_handler) {
 		rc = ops->rnmi_handler(tcntx);
@@ -404,14 +409,15 @@ struct sbi_trap_context *sbi_trap_rnmi_handler(struct sbi_trap_context *tcntx)
 			/* Platform handler failed to handle NMI */
 			sbi_trap_error("platform NMI handler failed", rc, tcntx);
 		}
-		return tcntx;
+		goto done;
 	}
 
 	/* No platform handler - treat as unhandled NMI */
 	sbi_trap_error("unhandled NMI (no platform rnmi_handler)",
 		       SBI_ENOTSUPP, tcntx);
 
-	/* Never returns */
+done:
+	sbi_trap_set_context(scratch, tcntx->prev_context);
 	return tcntx;
 }
 

-- 
2.43.7


-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

Re: [PATCH 0/2] lib: sbi: Fix Smrnmi/RNMI trap handling issues

[Evgeny Voevodin]

I tried this patch set on artificial setup on Whisper SW system simulator and it
seems to solve issues discussed in [1].

But I'd suggest to split this series to separate changes structurally and make
backporting easier if needed:

  1. Restore the Smrnmi context on non-retentive resume: extract
     sbi_smrnmi_hart_init() and call it from sbi_hart_reinit(). This is the
     actual fix and it is self contained, so anyone on a v1.9 based tree can
     pick just this one.

  2. The sbi_trap.c fix for the clobbered trap context (your patch 2/2). It
     touches a different file and has its own Fixes tag, so it does not
     depend on the rest.

  3. Change smrnmi_handlers_init from void to int, but keep the panic when
     the platform does not provide the callback. Today a callback which
     fails has no way to report it and NMIE is set anyway, so this is a fix
     on its own.

  4. Allow the NULL callback. This one changes what a platform is allowed to
     skip, so I would prefer to discuss it separately: as was mentioned
     earlier in the previous email thread [2], it would be nice to have a
     level of control of a situation when platform forgot to initialize the
     callback rather than just silently set NMIE and proceed. I can see these
     distinct options to handle this:

  a) Boot time output: based on a flag just print whether Smrnmi vectors are
     initialized by platform explicitly or not

         Smrnmi Vectors       : NULL     <-- or "Platform"

  b) Warning print when Smrnmi is present and no callback is registered

  c) Explicit opt out: platform which doesn't need to program vectors
     explicitly sets a corresponding flag in sbi_platform to indicate this.
     Without this flag set and when handlers are missing we panic like now,
     and when the flag is set we go ahead and proceed to setting the NMIE bit.

   I personally would go with a) and c) as this allows full control of what's
   going on and catches errors early. What do you think?

[1] https://lore.kernel.org/opensbi/CAHh=Yk_Sbd1MEH8tVshAOFQptEVKm0QZv+D7bJwMC80qvarVew@mail.gmail.com/
[2] https://lore.kernel.org/opensbi/DIIOKVJ7SVHG.115EOM7LY36I1@tenstorrent.com/

  Thanks,
  Evgeny

-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi