| Message ID | 20260827-rnmi-trap-fixes-v1-0-df371cf92301@sifive.com |
|---|---|
| Headers | show |
| Series | lib: sbi: Fix Smrnmi/RNMI trap handling issues | expand |
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
On Tue, Sep 1, 2026 at 1:18 AM Evgeny Voevodin <evvoevod@tenstorrent.com> wrote: > > 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. Good suggestion. I’ll submit this change as a separate patch. > 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 I agree with your proposal to adopt options a) and c), and I will follow the implementation suggested in [2] Thanks, Nia
Just to be clear about the split - my suggestion was to keep the current void return type of ops->smrnmi_handlers_init in patch 1 and change it to int in a separate patch, so that a platform can indicate that Smrnmi handlers initialization went wrong. This will make backporting to v1.9 easier if needed. Is this aligned with what you were going to proceed with? Thanks, Evgeny
On Thu, Sep 3, 2026 at 1:33 AM Evgeny Voevodin <evvoevod@tenstorrent.com> wrote: > > Just to be clear about the split - my suggestion was to keep the current void > return type of ops->smrnmi_handlers_init in patch 1 and change it to int in a > separate patch, so that a platform can indicate that Smrnmi handlers > initialization went wrong. This will make backporting to v1.9 easier if needed. > Is this aligned with what you were going to proceed with? > > Thanks, > Evgeny Yes, I fully understand your suggestion, and this is also how I plan to proceed. I will split the series into separate patches: 1. Reinitialize the RNMI after non-retentive suspend/resume. 2. Change smrnmi_handlers_init from void to int. 3. Allow a NULL callback and report the RNMI vector mode at boot. This matches your proposal and should make the patches easier to backport. Thanks for the clarification. Thanks, Nia
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 --