[PATCH 5/5] lib: sbi: add Smdbltrp ISA extension support

[Clément Léger]

On 13/09/2024 20:01, Samuel Holland wrote:
> Hi Clément,
> 
> On 2024-09-13 4:52 AM, Clément Léger wrote:
>> On 13/09/2024 01:50, Samuel Holland wrote:
>>> On 2024-09-12 7:10 AM, Clément Léger wrote:
>>>> Add support for the Smdbltrp[1] ISA extension. First thing to do is
>>>> clearing MDT on entry after setting the first MTVEC (since MDT is
>>>> reset to 1). Additionally, during trap handling, clear MDT once all
>>>> critical CSRs have been saved and in return path, restore MSTATUS before
>>>> restoring MEPC to avoid taking another trap which would clobber it.
>>>>
>>>> Link: https://github.com/riscv/riscv-double-trap/releases/download/v0.56/riscv-double-trap.pdf [1]
>>>> Signed-off-by: Clément Léger <cleger at rivosinc.com>
>>>> ---
>>>>  firmware/fw_base.S           | 29 ++++++++++++++++++++++++++---
>>>>  include/sbi/riscv_encoding.h |  2 ++
>>>>  2 files changed, 28 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/firmware/fw_base.S b/firmware/fw_base.S
>>>> index b950c0b..01d5f51 100644
>>>> --- a/firmware/fw_base.S
>>>> +++ b/firmware/fw_base.S
>>>> @@ -31,6 +31,16 @@
>>>>  	add	\__d4, \__s4, zero
>>>>  .endm
>>>>  
>>>> +.macro CLEAR_MDT tmp, have_mstatush
>>>> +	.if \have_mstatush
>>>> +	li 	\tmp, MSTATUSH_MDT
>>>> +	csrc	CSR_MSTATUSH, \tmp
>>>> +	.else
>>>> +	li 	\tmp, MSTATUS_MDT
>>>> +	csrc	CSR_MSTATUS, \tmp
>>>> +	.endif
>>>
>>> Why not put the __riscv_xlen check inside the macro? Then it doesn't need the
>>> have_mstatush parameter.
>>>
>>>> +.endm
>>>> +
>>>>  	.section .entry, "ax", %progbits
>>>>  	.align 3
>>>>  	.globl _start
>>>> @@ -91,6 +101,13 @@ _bss_zero:
>>>>  	lla	s4, _start_hang
>>>>  	csrw	CSR_MTVEC, s4
>>>>  
>>>> +	/* We are now ready to take a trap, clear MDT */
>>>> +#if __riscv_xlen > 32
>>>> +	CLEAR_MDT t0, 0
>>>> +#else
>>>> +	CLEAR_MDT t0, 1
>>>> +#endif
>>>
>>> I don't think we're really ready to take a trap here; the stack pointer is not
>>> even set yet. Also this needs to go after _start_warm so it gets run on all
>>> harts. So I think this should go after the other write to CSR_MTVEC, near the
>>> call to sbi_init().
>>
>> Actually, if we don't clear MDT, if a trap happens, it will trigger a
>> M-mode double trap. Since a temporary trap handler is set, we can let
>> the trap happen without generating a M-mode double trap (which would end
>> up in either a cpu_abort() (at least in qemu) or a NMI interrupt. I'm
>> not sure which one is better though (either hang in the temporary trap
>> handler or cpu abort.
> 
> Ah, you're right, the temporary trap handler doesn't use the stack, so this is
> OK for the boot hart. There is still a problem for other harts, or the boot hart
> after non-retentive suspend, which would jump to _start_warm and miss this code.

Yes totally and thanks for catching it. I added a call to CLEAR_MDT
right after the second time MTVEC is written as you proposed.

Thanks,

Clément

> 
> Regards,
> Samuel
>