Re: [PATCH v2 4/5] lib: sbi: Create a spot to place Smrnmi detection before traps and after DT is ready

On Mon, May 4, 2026 at 10:10 PM Evgeny Voevodin
<[email protected]> wrote:
>
> Hi Anup,
>
> Two concerns about the suggested approach.
>
> 1) Enablement of NMIs before the platform has set up its NMI handler
> introduces a window of possible hang. While NMIE=1, the trap handler is
> not defined, so if an NMI arrives, the trap jumps to nowhere.
>
> The v2 series keeps NMIE=0 until smrnmi_handlers_init() has installed
> both NMIVEC and the regular trap entry, which avoids this hole.
>
> More generally: on Smrnmi-enabled platforms, there is really no way to
> use any trap-based mechanism until the platform has set its NMI handlers.
> Holding off them until after a call to the platform's
> smrnmi_handlers_init() is mandatory to prevent possible hangs.
>
> 2) About the stack canary concern with the C-side __stack_chk_guard_init().
>
> The danger pattern "canary already pushed on stack" only triggers if a
> function call which returns is currently on stack, which means
> __stack_chk_guard variable will be checked against its value on prologue.
> Neither C nor asm functions currently on stack return or use epilogue
> check of __stack_chk_guard. The full call chain to __stack_chk_guard_init()
> consists of:
>
>   asm _start
>    -> sbi_init()              __noreturn
>       -> init_coldboot()      __noreturn
>          -> __stack_chk_guard_init()    asm, no canary
>
> Even if asm side falls back to `return`, it is still `j _start_hang`
> which also doesn't check epilogue.
>
> Could you reconsider patch 4/5 with this context?
>

Sounds good but I still require some changes.

This patch needs to be split into two patches:
1) The first patch should only move sbi_platform_extensions_init()
before trap-based feature detection
2) The second patch should move Zkr based stack canary
initialization into C code. No need for preserving the existing
__stack_chk_guard_init() intead we can initialize __stack_chk_guard
variable directly from init_coldboot()

Regards,
Anup

-- 
opensbi mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/opensbi