[PATCH v3 6/6] lib: sbi: hart: Detect and enable Smrnmi before trap-based feature detection

[Nylon Chen]

Anup Patel <anup at brainfault.org> 於 2026年5月9日週六 下午2:51寫道：

>
> On Thu, May 7, 2026 at 11:38 PM Evgeny Voevodin
> <evvoevod at tenstorrent.com> wrote:
> >
> > The location of the RNMI/E trap vectors in the Smrnmi extension is
> > implementation-defined, so platforms with vendor-specific NMI vector
> > mechanisms must install the firmware's NMI entry points themselves.
> >
> > Add an smrnmi_handlers_init() callback to sbi_platform_operations that
> > receives the firmware entry points and lets platform code install them
> > at the hardware-specific vector locations. Two pointers are passed:
> >
> >   - _trap_rnmi_handler: the dedicated RNMI entry point that saves
> >     context using the Smrnmi MN* CSRs and returns via mnret.
> >   - _trap_handler: the regular M-mode trap entry since RNME is taken
> >     as a regular M-mode trap with NMIE=0.
> >
> > When Smrnmi is present, install the platform's NMI vectors via the new
> > callback, initialize MNSCRATCH with the per-hart scratch pointer, and
> > set MNSTATUS.NMIE.
> >
> > Smrnmi-enabled platforms must register smrnmi_handlers_init; if the
> > extension is detected but no callback is registered, sbi_panic() is
> > called since enabling NMIs without handlers in place would route
> > subsequent traps into nowhere.
> >
> > Signed-off-by: Evgeny Voevodin <evvoevod at tenstorrent.com>
>
> LGTM.
>
> Reviewed-by: Anup Patel <anup at brainfault.org>
>
> Regards,
> Anup
>
> > ---
> >  include/sbi/sbi_platform.h |  4 ++++
> >  lib/sbi/sbi_hart.c         | 20 ++++++++++++++++++++
> >  2 files changed, 24 insertions(+)
> >
> > diff --git a/include/sbi/sbi_platform.h b/include/sbi/sbi_platform.h
> > index 715df499..fe382b56 100644
> > --- a/include/sbi/sbi_platform.h
> > +++ b/include/sbi/sbi_platform.h
> > @@ -150,6 +150,10 @@ struct sbi_platform_operations {
> >         /** platform specific pmp disable on current HART */
> >         void (*pmp_disable)(unsigned int n);
> >
> > +       /** platform specific Smrnmi handlers init on current HART */
> > +       void (*smrnmi_handlers_init)(void (*rnmi_handler)(void),
> > +                       void (*rnme_handler)(void));
> > +
> >         /** platform specific Smrnmi NMI handler.
> >          *  Returns SBI_SUCCESS on success, error code if NMI cannot be handled. */
> >         int (*rnmi_handler)(struct sbi_trap_context *tcntx);
> > diff --git a/lib/sbi/sbi_hart.c b/lib/sbi/sbi_hart.c
> > index 781161e5..92c602aa 100644
> > --- a/lib/sbi/sbi_hart.c
> > +++ b/lib/sbi/sbi_hart.c
> > @@ -532,6 +532,26 @@ static int hart_detect_features(struct sbi_scratch *scratch)
> >         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);
Two concerns about this block

First, sbi_panic() when smrnmi_handlers_init is NULL is too strict.
Not all platforms with Smrnmi need to program a vendor-specific NMI
vector register.
Some implementations have a fixed NMI vector address or route NMIs
through mtvec by default; for these, setting MNSCRATCH and NMIE is
sufficient and no platform callback is needed.

This can be reproduced with QEMU virt:

qemu-system-riscv64 -M virt -cpu rv64,smrnmi=on -m 256m \
      -bios fw_jump.bin -nographic -no-reboot

The firmware hangs with zero output. The panic fires before
sbi_platform_early_init() so the console is not yet initialized and
the message is silently dropped.

Adding a no-op stub to the generic platform confirms the panic is  the
sole cause -- boot proceeds normally with the stub in place.

Second, the callback returns void, so there is no way to detect
whether the platform succeeded in programming the NMI vector.
csr_set(MNSTATUS, NMIE) then runs unconditionally.
If the callback fails silently, NMIs are enabled but the hardware
vector register is left at its reset value. The first NMI would jump
to an undefined address.

Suggested fix:

/* allow NULL: platforms with fixed or mtvec-based NMI vectors
* do not need to program any vendor register */
if (ops && ops->smrnmi_handlers_init) {
        int 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);

This requires changing the callback signature from void to int.

Platforms that have nothing to program return 0, platforms that fail
return an error, and platforms with no callback at all are handled
gracefully without a panic.

> > +       }
> > +
> >  #define __check_hpm_csr(__csr, __mask)                                           \
> >         oldval = csr_read_allowed(__csr, &trap);                          \
> >         if (!trap.cause) {                                                \
> > --
> > 2.43.0
> >