Re: [PATCH v2 2/3] riscv: traps: make insn fetch common in unknown instruction

["Björn Töpel" <bjorn@kernel.org>]

Ben Dooks <ben.dooks@codethink.co.uk> writes:

> Add the trapped instruction (insn) as the second argument to
> riscv_v_first_use_handler() from the trap handler so when we
> add more handlers we can do the fetch of the instruction just
> once.
>
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> ---
> - fixed wording of patch from rfc
> v2:
>   - fixed todo by going to illegal instruction error if get_user fails
>   - added pointer print for failed read
>   - fixed issues with rebasing onto main branch
> ---
>  arch/riscv/include/asm/vector.h |  4 ++--
>  arch/riscv/kernel/traps.c       | 14 +++++++++++++-
>  arch/riscv/kernel/vector.c      | 11 +----------
>  3 files changed, 16 insertions(+), 13 deletions(-)
>
> diff --git a/arch/riscv/include/asm/vector.h b/arch/riscv/include/asm/vector.h
> index c7c023afbacd..9ec2473c1b73 100644
> --- a/arch/riscv/include/asm/vector.h
> +++ b/arch/riscv/include/asm/vector.h
> @@ -22,7 +22,7 @@
>  extern unsigned long riscv_v_vsize;
>  int riscv_v_setup_vsize(void);
>  bool insn_is_vector(u32 insn_buf);
> -bool riscv_v_first_use_handler(struct pt_regs *regs);
> +bool riscv_v_first_use_handler(struct pt_regs *regs, u32 insn);
>  void kernel_vector_begin(void);
>  void kernel_vector_end(void);
>  void get_cpu_vector_context(void);
> @@ -270,7 +270,7 @@ struct pt_regs;
>  static inline int riscv_v_setup_vsize(void) { return -EOPNOTSUPP; }
>  static __always_inline bool has_vector(void) { return false; }
>  static __always_inline bool insn_is_vector(u32 insn_buf) { return false; }
> -static inline bool riscv_v_first_use_handler(struct pt_regs *regs) { return false; }
> +static inline bool riscv_v_first_use_handler(struct pt_regs *regs, u32 insn) { return false; }
>  static inline bool riscv_v_vstate_query(struct pt_regs *regs) { return false; }
>  static inline bool riscv_v_vstate_ctrl_user_allowed(void) { return false; }
>  #define riscv_v_vsize (0)
> diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
> index 51ebfd23e007..9662138ba45c 100644
> --- a/arch/riscv/kernel/traps.c
> +++ b/arch/riscv/kernel/traps.c
> @@ -172,11 +172,23 @@ asmlinkage __visible __trap_section void do_trap_insn_illegal(struct pt_regs *re
>  	bool handled;
>  
>  	if (user_mode(regs)) {
> +		u32 __user *epc = (u32 __user *)regs->epc;
> +		u32 insn = (u32)regs->badaddr;
> +
>  		irqentry_enter_from_user_mode(regs);
>  
>  		local_irq_enable();
>  
> -		handled = riscv_v_first_use_handler(regs);
> +		if (!insn) {
> +			if (__get_user(insn, epc)) {
> +				printk_ratelimited(KERN_ERR "traps: failed to read instruction at user %px\n", epc);
> +				handled = false;
> +				insn = 0;
> +			}
> +		}
> +
> +		if (insn)
> +			handled = riscv_v_first_use_handler(regs, insn);

Maybe it's just me, but this addition makes it a bit hard (harder!) to
read.

Maybe replace the chunk something in the lines of:

	if (!insn && __get_user(insn, epc))
		goto out;
	
	handled = riscv_v_first_use_handler(regs, insn);
out:


Björn

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv