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

[Björn Töpel]

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/

>  - 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!).

>  - Little modification on ret_from_fork & ret_from_kernel_thread

What changes?

>  - 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.

> +		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.

> +		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.


Björn