Re: [PATCH 3/6] riscv: convert bottom half of exception handling to C

[Deepak Gupta <debug@rivosinc.com>]

On Fri, Jun 21, 2024 at 07:56:56AM +0800, Jisheng Zhang wrote:
>On Thu, Jun 20, 2024 at 10:06:15AM +0200, Clément Léger wrote:
>>
>>
>> On 20/06/2024 02:02, Cyril Bur wrote:
>> > On Thu, Jun 20, 2024 at 3:04 AM Deepak Gupta <debug@rivosinc.com> wrote:
>> >>
>> >> On Mon, Jun 17, 2024 at 01:05:50AM +0800, Jisheng Zhang wrote:
>> >>> For readability, maintainability and future scalability, convert the
>> >>> bottom half of the exception handling to C.
>> >>>
>> >>> Mostly the assembly code is converted to C in a relatively
>> >>> straightforward manner.
>> >>>
>> >>> However, there are two modifications I need to mention:
>> >>>
>> >>> 1. the CSR_CAUSE reg reading and saving is moved to the C code
>> >>> because we need the cause to dispatch the exception handling,
>> >>> if we keep the cause reading and saving, we either pass it to
>> >>> do_traps() via. 2nd param or get it from pt_regs which an extra
>> >>> memory load is needed, I don't like any of the two solutions becase
>> >>> the exception handling sits in hot code path, every instruction
>> >>> matters.
>> >>
>> >> CC: Clement.
>> >>
>> >> I think its better to save away cause in pt_regs prior to calling
>> >> `do_traps`. Once control is transferred to C code in `do_traps`,
>> >> another trap can happen. It's a problem anyways today without CPU support.
>> >>
>> >> Although with Ssdbltrp [1] extension and it kernel support [2] for it,
>> >> I expect asm code would clear up `SDT` bit in mstatus. Whenever `Ssdbltrp` lands,
>
>Hi Deepak, Clément,
>
>Currently, SR_IE bit is is set(setting means enable irq) in c, could the
>'SDT' bit be cleared in c as well when Ssdbltrp lands?

SDT is placed in sstatus CSR. So yes its possible to clear it in C in `do_traps`.
Although then you (and any future developer) will have to pay extra attention to this
function because this function can be nested depending on when SDT is cleared or not.
Maintainence (and introductions of error) wise it doesn't look ideal.

If we keep read of `cause` in asm code and pass it as parameter to `do_traps`, it
cleanly defines the boundary of which functions can be nested and which can't. It
helps features like SSE [1, 2] (which expect nesting of events and had to be creative)
to implement cleaner logic.

[1] https://lists.riscv.org/g/tech-prs/message/515
[2] https://lpc.events/event/17/contributions/1479/attachments/1243/2526/SSE_Plumbers.pdf

>
>Thanks
>> >> I think `do_traps` should expect nesting of traps and thus cause should be saved
>> >> away before it gets control so that safely traps can be nested.
>>
>> Hi,
>>
>> Indeed, every register that is "unique" to a trap and than can be
>> overwritten by a second trap should be saved before reenabling them when
>> using Ssdbltrp. So that would be nice to preserve that.
>>
>> >>
>> >
>> > Is a possible solution to do both options Jisheng suggested? Save the
>> > cause before
>> > calling do_traps but also pass it via second param?
>>
>> I guess so if it fits your performance requirements.
>>
>> Thanks,
>>
>> Clément
>>
>> >
>> >> [1] - https://github.com/riscv/riscv-double-trap/releases/download/v1.0-rc1/riscv-double-trap.pdf
>> >> [2] - https://lore.kernel.org/all/20240418133916.1442471-1-cleger@rivosinc.com/
>> >>
>> >>>
>> >>> 2.To cope with SIFIVE_CIP_453 errata, it looks like we don't need
>> >>> alternative mechanism any more after the asm->c convertion. Just
>> >>> replace the excp_vect_table two entries.
>> >>>
>> >>> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
>> >>
>> >> _______________________________________________
>> >> linux-riscv mailing list
>> >> linux-riscv@lists.infradead.org
>> >> http://lists.infradead.org/mailman/listinfo/linux-riscv