Re: [PATCH -next V10 04/10] riscv: entry: Convert to generic entry

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Kernel]     [Kernel Newbies]     [x86 Platform Driver]     [Netdev]     [Linux Wireless]     [Netfilter]     [Bugtraq]     [Linux Filesystems]     [Yosemite Discussion]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Device Mapper]

  Powered by Linux