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

[Nia Su]

On Fri, Sep 11, 2026 at 2:58 AM Evgeny Voevodin
<evvoevod at tenstorrent.com> wrote:
>
> Patch 1/3, two things.
>
> - I'd put the sbi_smrnmi_hart_init() call in the beginning of
>   sbi_hart_reinit() to initialize trap handlers early.
>
> - Worth noting that after this change we have sbi_smrnmi_hart_init()
>   called twice on cold boot, one from hart_detect_features() and another
>   from sbi_hart_reinit() at the end of sbi_hart_init(). As long as
>   platform's smrnmi context initialization allows it, this doesn't bite.
>   Please mention it where smrnmi_handlers_init is declared in
>   sbi_platform.h, so a platform knows the callback can be called more
>   than once on the same HART.

Looks good, I'll adopt the above.

> Patch 3/3, two things.
>
> - Global variable is racy because every hart writes smrnmi_init_mode and
>   boot hart reads it. How about make the banner stateless:
>
>       const struct sbi_platform *plat = sbi_platform_thishart_ptr();
>
>       if (sbi_hart_has_extension(scratch, SBI_HART_EXT_SMRNMI))
>               sbi_printf("Boot HART Smrnmi Init       : %s\n",
>                          plat && sbi_platform_ops(plat)->smrnmi_handlers_init ?
>                          "Platform" : "Default");
>
>   also this is lightweight and clearly describes the status. Then
>   smrnmi_init_mode, the enum and sbi_hart_get_smrnmi_status_str() are
>   not needed.

Checking sbi_platform_ops(plat)->smrnmi_handlers_init only tells us a
callback exists, not that it succeeded. The current code only marks
"Platform" after it actually returns success. To keep that without a racy
global, I'd store the state per-HART instead:

struct sbi_hart_features {
      ...
      enum sbi_hart_smrnmi_init_mode smrnmi_init_mode;
};

static int sbi_smrnmi_hart_init(struct sbi_scratch *scratch)
{
      struct sbi_hart_features *hfeatures = sbi_hart_features_ptr(scratch);
      ...
      if (ops && ops->smrnmi_handlers_init) {
              ret = ops->smrnmi_handlers_init(...);
              if (ret)
                      return ret;
              hfeatures->smrnmi_init_mode = SBI_HART_SMRNMI_INIT_PLATFORM;
      } else {
              hfeatures->smrnmi_init_mode = SBI_HART_SMRNMI_INIT_DEFAULT;
      }
      ...
      return 0;
}

> - Also, I didn't see a platform flag to indicate that it doesn't need to
>   initialize Smrnmi handlers so that boot code can proceed without
>   smrnmi_handlers installed. Did you plan to add it?

A separate flag isn't needed here. With this patch,
smrnmi_handlers_init == NULL already serves as that signal.
sbi_smrnmi_hart_init() no longer panics on a missing callback,
it just falls back to the platform's fixed/mtvec NMI vector and
continues boot

Thanks,
Nia

>   Thanks,
>   Evgeny