[PATCH v2 0/3] lib: sbi: Fix Smrnmi resume init and optional platform callback

[Nia Su]

On Sat, Sep 12, 2026 at 1:11 AM Evgeny Voevodin
<evvoevod at tenstorrent.com> wrote:
>
> Patch 3/3:
>
> - About the mode variable: I don't think we need to keep any state just
>   to indicate how Smrnmi trap handlers were initialized, if a simple
>   check gives the same answer.
>
>   sbi_boot_print_hart() (where the Smrnmi mode is indicated) is only
>   reached in these cases:
>
>   1. No Smrnmi detected (Smrnmi mode output is suppressed based on its
>      absence)
>   2. No platform handlers installed (Default)
>   3. Platform handlers installed AND succeed (Platform)
>
>   If a platform has handlers but the initialization didn't succeed
>   (sbi_smrnmi_hart_init() returned non-zero) the hart gets hanged in
>   init_coldboot() before any part of the banner is printed
>   (sbi_init.c:299, the banner starts at sbi_init.c:440), so the Smrnmi
>   mode line is never reached.
>
>   So, I would still prefer to get rid of the Smrnmi mode variable in
>   favor of a simple on the fly check.

Understood. I agree that a failed Smrnmi handler initialization should
fail before the banner is printed. I’ll remove it.

> - About the platform flag. Allowing the NULL callback is right, it fixes
>   the panic on platforms with fixed or mtvec based vectors. My point is
>   that NULL alone does not distinguish between platforms which
>   intentionally didn't set the handlers and those which forgot to do
>   this. Both proceed with csr_set(CSR_MNSTATUS, MNSTATUS_NMIE), so for
>   the second one NMIs are enabled while the vendor vector register keeps
>   its reset value, and the first RNMI jumps to an undefined address.
>   This is broken in a way which might be hard to debug.
>
>   A flag in sbi_platform which says "this platform has nothing to
>   program" separates the two cases: no flag and no callback is then a
>   platform error. This is what we agreed on in [1], options a) and c)
>   together.

Thanks for the clarification. I think I understand the distinction now.
The flag distinguishes platforms that intentionally leave
smrnmi_handlers_init unset from platforms that accidentally omit it. I
plan to implement it as follows.

enum sbi_platform_features {
...
/* Platform uses a fixed or mtvec-based RNMI vector. */
SBI_PLATFORM_HAS_FIXED_SMRNMI_VECTOR = (1 << 2),
...
};

#define sbi_platform_has_fixed_smrnmi_vector(__p) \
((__p)->features & SBI_PLATFORM_HAS_FIXED_SMRNMI_VECTOR)

Platforms with fixed or mtvec-based vectors will set the flag in their
platform descriptor:

.features = SBI_PLATFORM_DEFAULT_FEATURES |
   SBI_PLATFORM_HAS_FIXED_SMRNMI_VECTOR,

The Smrnmi initialization will accept a NULL callback only when the flag is
set. Otherwise, it will fail before enabling NMIs:

static int sbi_smrnmi_hart_init(struct sbi_scratch *scratch)
{
...
   if (ops && ops->smrnmi_handlers_init) {
        //do ops->smrnmi_handlers_init
        ...
   } else if (!sbi_platform_has_fixed_smrnmi_vector(plat)) {
        sbi_printf("%s: platform must provide "
                         "smrnmi_handlers_init() or set "
                         "SBI_PLATFORM_HAS_FIXED_SMRNMI_VECTOR\\n",
                          __func__);
       return SBI_ENOTSUPP;
   }
...
}

Since initialization failures stop boot before the banner, the mode can
be reported from the callback presence:

if (sbi_hart_has_extension(scratch, SBI_HART_EXT_SMRNMI)) {
...
   sbi_printf("Boot HART Smrnmi Init       : %s\\n",
        (ops && ops->smrnmi_handlers_init) ?
        "Platform" : "Default");
   }
}

Please let me know if I’ve misunderstood anything or if this approach
needs any changes; otherwise, I’ll include it in the next revision.

Thanks,
Nia

>   Thanks,
>   Evgeny
>
> [1] https://lore.kernel.org/opensbi/CACpYXwCiciQzQo99=f=_wioYb3kHHTZkQm_UAi6FCXYpbJmeog@mail.gmail.com/