misaligned accesses traps are not nmi and should be treated as normal
one using irqentry_enter()/exit(). 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,
--
2.49.0
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?
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,
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,
On Tue, Apr 22, 2025 at 09:57:12AM +0200, Clément Léger wrote:
>
>
> 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.
So, a trap that happens in kernel space while IRQs are disabled, SHOULD
really be NMI-like.
You then have a choice, make all such traps from kernel space NMI-like;
this makes it easy on the trap handler, since the context is always the
same. Mistakes are 'easy' to find.
Or,.. do funny stuff and only make it NMI like if IRQs were disabled.
Which gives inconsistent context for the handler and you'll find
yourself scratching your head at some point in the future wondering why
this one rare occasion goes BOOM.
x86 mostly does the first, any trap that can happen with IRQs disabled
is treated unconditionally as NMI like. The obvious exception is
page-fault, but that already has a from-non-preemptible-context branch
that is 'careful'.
As to unaligned traps from kernel space, I would imagine they mostly BUG
the kernel, except when there's an exception entry for that location, in
which case it might do a fixup?
Anyway, the reason these exceptions should be NMI like, is because
interrupts are not allowed to nest. Notably something like:
raw_spin_lock_irqsave(&foo);
<IRQ>
raw_spin_lock_irqsave(&foo);
...
Is an obvious problem. Exceptions that can run while IRQs are disabled,
must not use locks -- treating them as NMI-like (they are non-maskable
after all), ensures this.
On 22/04/2025 11:44, Peter Zijlstra wrote: > On Tue, Apr 22, 2025 at 09:57:12AM +0200, Clément Léger wrote: >> >> >> 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. > > So, a trap that happens in kernel space while IRQs are disabled, SHOULD > really be NMI-like. > > You then have a choice, make all such traps from kernel space NMI-like; > this makes it easy on the trap handler, since the context is always the > same. Mistakes are 'easy' to find. > > Or,.. do funny stuff and only make it NMI like if IRQs were disabled. > Which gives inconsistent context for the handler and you'll find > yourself scratching your head at some point in the future wondering why > this one rare occasion goes BOOM. Hi Peter, Yeah agreed, that might be hazardous. > > x86 mostly does the first, any trap that can happen with IRQs disabled > is treated unconditionally as NMI like. The obvious exception is > page-fault, but that already has a from-non-preemptible-context branch > that is 'careful'. > > As to unaligned traps from kernel space, I would imagine they mostly BUG > the kernel, except when there's an exception entry for that location, in > which case it might do a fixup? The misaligned access exception handling currently handles misaligned access for the kernel as well (except if explicitly disabled). > > > Anyway, the reason these exceptions should be NMI like, is because > interrupts are not allowed to nest. Notably something like: > > > raw_spin_lock_irqsave(&foo); > <IRQ> > raw_spin_lock_irqsave(&foo); > ... > > Is an obvious problem. Exceptions that can run while IRQs are disabled, > must not use locks -- treating them as NMI-like (they are non-maskable > after all), ensures this. As said in my previous reply, the misaligned handling code does not currently access any locks (when handling kernel misaligned accesses) and is self contained. That being said, that assumption might not be true in future so that might be better to take your approach and treat the misaligned access like an NMI. Thanks, Clément
On Tue, 22 Apr 2025, Clément Léger wrote:
> > x86 mostly does the first, any trap that can happen with IRQs disabled
> > is treated unconditionally as NMI like. The obvious exception is
> > page-fault, but that already has a from-non-preemptible-context branch
> > that is 'careful'.
> >
> > As to unaligned traps from kernel space, I would imagine they mostly BUG
> > the kernel, except when there's an exception entry for that location, in
> > which case it might do a fixup?
>
> The misaligned access exception handling currently handles misaligned
> access for the kernel as well (except if explicitly disabled).
It's currently not clear that a kernel mode unaligned access is indeed a
bug, as some network protocol stacks may still rely on unaligned accesses
for performance reasons for the regular case where network headers do come
out aligned[1][2].
Hopefully not in the hardirq context though, and the usual approach is to
keep interrupts disabled in the emulation path if arriving from the kernel
mode as we don't expect kernel code to be ever paged out (the same applies
to all kinds of machine instruction emulation).
References:
[1] "TCP SYNs broken in 2.3.41",
<https://marc.info/?l=linux-kernel&m=94927689929463>
[2] "Alpha: Emulate unaligned LDx_L/STx_C for data consistency",
<https://lore.kernel.org/lkml/87v7rd8h99.fsf@email.froward.int.ebiederm.org/>
HTH,
Maciej
On Thu, 1 May 2025, Maciej W. Rozycki wrote: > Hopefully not in the hardirq context though, and the usual approach is to > keep interrupts disabled in the emulation path if arriving from the kernel > mode as we don't expect kernel code to be ever paged out (the same applies > to all kinds of machine instruction emulation). s/code/data/, obviously. Maciej
© 2016 - 2026 Red Hat, Inc.