Re: [PATCH 1/5] riscv: misaligned: factorize trap handling

["Clément Léger" <cleger@rivosinc.com>]

On 21/04/2025 09:06, Alexandre Ghiti wrote:
> Hi Clément,
> 
> 
> On 14/04/2025 14:34, Clément Léger wrote:
>> misaligned accesses traps are not nmi and should be treated as normal
>> one using irqentry_enter()/exit().
> 
> 
> All the traps that come from kernel mode are treated as nmi as it was
> suggested by Peter here: https://lore.kernel.org/linux-riscv/
> Yyhv4UUXuSfvMOw+@hirez.programming.kicks-ass.net/
> 
> I don't know the differences between irq_nmi_entry/exit() and irq_entry/
> exit(), so is that still correct to now treat the kernel traps as non-nmi?

Hi Alex,

Actually, this discussion was raised on a previous series [1] by Maciej
which replied that we should actually reenable interrupt depending on
the state that was interrupted. Looking at other architecture/code, it
seems like treating misaligned accesses as NMI is probably not the right
way. For instance, loongarch treats them as normal IRQ using a
irqentry_enter()/exit() and reenabling IRQS if possible.

Moreover, it looks like to me that misaligned access traps are similar
(in the way they can be handled) to how the page fault are handled in
RISC-V. It uses irqentry_enter()/exit() and reenables interrupts if
needed. Additionally, the misaligned access handling does will not take
any locks if handling a misaligned access taken while in kernel so even
if we interrupt a kernel critical section that does not have interrupts
disable, we are good to go. But my understanding might be wrong as well
and I might be missing specific cases :)

Thanks,

Clément

https://lore.kernel.org/lkml/alpine.DEB.2.21.2501070143250.18889@angie.orcam.me.uk/
[1]

> 
> Thanks,
> 
> Alex
> 
> 
>> Since both load/store and user/kernel
>> should use almost the same path and that we are going to add some code
>> around that, factorize it.
>>
>> Signed-off-by: Clément Léger<cleger@rivosinc.com>
>> ---
>>   arch/riscv/kernel/traps.c | 49 ++++++++++++++++-----------------------
>>   1 file changed, 20 insertions(+), 29 deletions(-)
>>
>> diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
>> index 8ff8e8b36524..55d9f3450398 100644
>> --- a/arch/riscv/kernel/traps.c
>> +++ b/arch/riscv/kernel/traps.c
>> @@ -198,47 +198,38 @@ asmlinkage __visible __trap_section void
>> do_trap_insn_illegal(struct pt_regs *re
>>   DO_ERROR_INFO(do_trap_load_fault,
>>       SIGSEGV, SEGV_ACCERR, "load access fault");
>>   -asmlinkage __visible __trap_section void
>> do_trap_load_misaligned(struct pt_regs *regs)
>> +enum misaligned_access_type {
>> +    MISALIGNED_STORE,
>> +    MISALIGNED_LOAD,
>> +};
>> +
>> +static void do_trap_misaligned(struct pt_regs *regs, enum
>> misaligned_access_type type)
>>   {
>> -    if (user_mode(regs)) {
>> -        irqentry_enter_from_user_mode(regs);
>> +    irqentry_state_t state = irqentry_enter(regs);
>>   +    if (type ==  MISALIGNED_LOAD) {
>>           if (handle_misaligned_load(regs))
>>               do_trap_error(regs, SIGBUS, BUS_ADRALN, regs->epc,
>> -                  "Oops - load address misaligned");
>> -
>> -        irqentry_exit_to_user_mode(regs);
>> +                      "Oops - load address misaligned");
>>       } else {
>> -        irqentry_state_t state = irqentry_nmi_enter(regs);
>> -
>> -        if (handle_misaligned_load(regs))
>> +        if (handle_misaligned_store(regs))
>>               do_trap_error(regs, SIGBUS, BUS_ADRALN, regs->epc,
>> -                  "Oops - load address misaligned");
>> -
>> -        irqentry_nmi_exit(regs, state);
>> +                      "Oops - store (or AMO) address misaligned");
>>       }
>> +
>> +    irqentry_exit(regs, state);
>>   }
>>   -asmlinkage __visible __trap_section void
>> do_trap_store_misaligned(struct pt_regs *regs)
>> +asmlinkage __visible __trap_section void
>> do_trap_load_misaligned(struct pt_regs *regs)
>>   {
>> -    if (user_mode(regs)) {
>> -        irqentry_enter_from_user_mode(regs);
>> -
>> -        if (handle_misaligned_store(regs))
>> -            do_trap_error(regs, SIGBUS, BUS_ADRALN, regs->epc,
>> -                "Oops - store (or AMO) address misaligned");
>> -
>> -        irqentry_exit_to_user_mode(regs);
>> -    } else {
>> -        irqentry_state_t state = irqentry_nmi_enter(regs);
>> -
>> -        if (handle_misaligned_store(regs))
>> -            do_trap_error(regs, SIGBUS, BUS_ADRALN, regs->epc,
>> -                "Oops - store (or AMO) address misaligned");
>> +    do_trap_misaligned(regs, MISALIGNED_LOAD);
>> +}
>>   -        irqentry_nmi_exit(regs, state);
>> -    }
>> +asmlinkage __visible __trap_section void
>> do_trap_store_misaligned(struct pt_regs *regs)
>> +{
>> +    do_trap_misaligned(regs, MISALIGNED_STORE);
>>   }
>> +
>>   DO_ERROR_INFO(do_trap_store_fault,
>>       SIGSEGV, SEGV_ACCERR, "store (or AMO) access fault");
>>   DO_ERROR_INFO(do_trap_ecall_s,