On Thu, Dec 8, 2022 at 6:11 PM Björn Töpel <bjorn@xxxxxxxxxx> wrote:
>
> guoren@xxxxxxxxxx writes:
>
> The RISC-V entry.S is much more paletable after this patch! :-)
>
> Some minor things...
>
> > From: Guo Ren <guoren@xxxxxxxxxxxxxxxxx>
> >
> > This patch converts riscv to use the generic entry infrastructure from
> > kernel/entry/*. The generic entry makes maintainers' work easier and
> > codes more elegant. Here are the changes than before:
>
> s/changes than before/changes/
Okay
>
> > - More clear entry.S with handle_exception and ret_from_exception
> > - Get rid of complex custom signal implementation
> > - More readable syscall procedure
>
> Maybe reword this a bit? It's a move from assembly to C (which, is much
> more readable!).
Okay.
>
> > - Little modification on ret_from_fork & ret_from_kernel_thread
>
> What changes?
ENTRY(ret_from_fork)
+ call schedule_tail
+ move a0, sp /* pt_regs */
la ra, ret_from_exception
- tail schedule_tail
+ tail syscall_exit_to_user_mode
ENDPROC(ret_from_fork)
ENTRY(ret_from_kernel_thread)
call schedule_tail
/* Call fn(arg) */
- la ra, ret_from_exception
move a0, s1
- jr s0
+ jalr s0
+ move a0, sp /* pt_regs */
+ la ra, ret_from_exception
+ tail syscall_exit_to_user_mode
ENDPROC(ret_from_kernel_thread)
>
> > - Wrap with irqentry_enter/exit and syscall_enter/exit_from_user_mode
> > - Use the standard preemption code instead of custom
>
> > Suggested-by: Huacai Chen <chenhuacai@xxxxxxxxxx>
> > Tested-by: Yipeng Zou <zouyipeng@xxxxxxxxxx>
> > Tested-by: Jisheng Zhang <jszhang@xxxxxxxxxx>
> > Signed-off-by: Guo Ren <guoren@xxxxxxxxxxxxxxxxx>
> > Signed-off-by: Guo Ren <guoren@xxxxxxxxxx>
> > Cc: Ben Hutchings <ben@xxxxxxxxxxxxxxx>
> > ---
> > arch/riscv/Kconfig | 1 +
> > arch/riscv/include/asm/csr.h | 1 -
> > arch/riscv/include/asm/entry-common.h | 8 +
> > arch/riscv/include/asm/ptrace.h | 10 +-
> > arch/riscv/include/asm/stacktrace.h | 5 +
> > arch/riscv/include/asm/syscall.h | 6 +
> > arch/riscv/include/asm/thread_info.h | 13 +-
> > arch/riscv/kernel/entry.S | 237 ++++----------------------
> > arch/riscv/kernel/irq.c | 15 ++
> > arch/riscv/kernel/ptrace.c | 43 -----
> > arch/riscv/kernel/signal.c | 21 +--
> > arch/riscv/kernel/sys_riscv.c | 29 ++++
> > arch/riscv/kernel/traps.c | 70 ++++++--
> > arch/riscv/mm/fault.c | 16 +-
> > 14 files changed, 175 insertions(+), 300 deletions(-)
> > create mode 100644 arch/riscv/include/asm/entry-common.h
>
> [...]
>
> > diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
> > index da44fe2d0d82..69097dfffdc9 100644
> > --- a/arch/riscv/kernel/entry.S
> > +++ b/arch/riscv/kernel/entry.S
> > @@ -14,10 +14,6 @@
> > #include <asm/asm-offsets.h>
> > #include <asm/errata_list.h>
> >
> > -#if !IS_ENABLED(CONFIG_PREEMPTION)
> > -.set resume_kernel, restore_all
> > -#endif
> > -
> > ENTRY(handle_exception)
> > /*
> > * If coming from userspace, preserve the user thread pointer and load
> > @@ -106,19 +102,8 @@ _save_context:
> > .option norelax
> > la gp, __global_pointer$
> > .option pop
> > -
> > -#ifdef CONFIG_TRACE_IRQFLAGS
> > - call __trace_hardirqs_off
> > -#endif
> > -
> > -#ifdef CONFIG_CONTEXT_TRACKING_USER
> > - /* If previous state is in user mode, call user_exit_callable(). */
> > - li a0, SR_PP
> > - and a0, s1, a0
> > - bnez a0, skip_context_tracking
> > - call user_exit_callable
> > -skip_context_tracking:
> > -#endif
> > + move a0, sp /* pt_regs */
> > + la ra, ret_from_exception
>
> Not for this series, but at some point it would be nice to get rid of
> the "move" pseudoinsn in favor of "mv".
>
> [...]
>
> > diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
> > index f7fa973558bc..ee9a0ef672e9 100644
> > --- a/arch/riscv/kernel/traps.c
> > +++ b/arch/riscv/kernel/traps.c
> > @@ -17,6 +17,7 @@
> > #include <linux/module.h>
> > #include <linux/irq.h>
> > #include <linux/kexec.h>
> > +#include <linux/entry-common.h>
> >
> > #include <asm/asm-prototypes.h>
> > #include <asm/bug.h>
> > @@ -99,10 +100,19 @@ static void do_trap_error(struct pt_regs *regs, int signo, int code,
> > #else
> > #define __trap_section noinstr
> > #endif
> > -#define DO_ERROR_INFO(name, signo, code, str) \
> > -asmlinkage __visible __trap_section void name(struct pt_regs *regs) \
> > -{ \
> > - do_trap_error(regs, signo, code, regs->epc, "Oops - " str); \
> > +#define DO_ERROR_INFO(name, signo, code, str) \
> > +asmlinkage __visible __trap_section void name(struct pt_regs *regs) \
> > +{ \
> > + if (user_mode(regs)) { \
> > + irqentry_enter_from_user_mode(regs); \
> > + do_trap_error(regs, signo, code, regs->epc, "Oops - " str); \
> > + irqentry_exit_to_user_mode(regs); \
> > + } else { \
> > + irqentry_state_t irq_state = irqentry_nmi_enter(regs); \
> > + do_trap_error(regs, signo, code, regs->epc, "Oops - " str); \
> > + irqentry_nmi_exit(regs, irq_state); \
> > + } \
> > + BUG_ON(!irqs_disabled()); \
> > }
> >
> > DO_ERROR_INFO(do_trap_unknown,
> > @@ -126,18 +136,38 @@ int handle_misaligned_store(struct pt_regs *regs);
> >
> > asmlinkage void __trap_section do_trap_load_misaligned(struct pt_regs *regs)
> > {
> > - if (!handle_misaligned_load(regs))
> > - return;
> > - do_trap_error(regs, SIGBUS, BUS_ADRALN, regs->epc,
> > - "Oops - load address misaligned");
> > + if (user_mode(regs)) {
> > + irqentry_enter_from_user_mode(regs);
> > + if (handle_misaligned_load(regs))
> > + do_trap_error(regs, SIGBUS, BUS_ADRALN, regs->epc,
> > + "Oops - load address misaligned");
> > + irqentry_exit_to_user_mode(regs);
> > + } else {
> > + irqentry_state_t irq_state = irqentry_nmi_enter(regs);
>
> Please add a newline.
okay
>
> > + if (handle_misaligned_load(regs))
> > + do_trap_error(regs, SIGBUS, BUS_ADRALN, regs->epc,
> > + "Oops - load address misaligned");
> > + irqentry_nmi_exit(regs, irq_state);
> > + }
> > + BUG_ON(!irqs_disabled());
> > }
> >
> > asmlinkage void __trap_section do_trap_store_misaligned(struct pt_regs *regs)
> > {
> > - if (!handle_misaligned_store(regs))
> > - return;
> > - do_trap_error(regs, SIGBUS, BUS_ADRALN, regs->epc,
> > - "Oops - store (or AMO) address misaligned");
> > + 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 irq_state = irqentry_nmi_enter(regs);
>
> Please add a newline.
okay
>
> > + if (handle_misaligned_store(regs))
> > + do_trap_error(regs, SIGBUS, BUS_ADRALN, regs->epc,
> > + "Oops - store (or AMO) address misaligned");
> > + irqentry_nmi_exit(regs, irq_state);
> > + }
> > + BUG_ON(!irqs_disabled());
> > }
> > #endif
> > DO_ERROR_INFO(do_trap_store_fault,
> > @@ -159,7 +189,7 @@ static inline unsigned long get_break_insn_length(unsigned long pc)
> > return GET_INSN_LENGTH(insn);
> > }
> >
> > -asmlinkage __visible __trap_section void do_trap_break(struct pt_regs *regs)
> > +static void __do_trap_break(struct pt_regs *regs)
> > {
> > #ifdef CONFIG_KPROBES
> > if (kprobe_single_step_handler(regs))
> > @@ -189,6 +219,20 @@ asmlinkage __visible __trap_section void do_trap_break(struct pt_regs *regs)
> > else
> > die(regs, "Kernel BUG");
> > }
> > +
> > +asmlinkage __visible __trap_section void do_trap_break(struct pt_regs *regs)
> > +{
> > + if (user_mode(regs)) {
> > + irqentry_enter_from_user_mode(regs);
> > + __do_trap_break(regs);
> > + irqentry_exit_to_user_mode(regs);
> > + } else {
> > + irqentry_state_t irq_state = irqentry_nmi_enter(regs);
>
> Please add a newline.
okay
>
>
> Björn
--
Best Regards
Guo Ren
