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

[Andrew Jones <ajones@ventanamicro.com>]

On Sun, Dec 01, 2024 at 10:27:58AM +0000, Ben Dooks wrote:
> 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);

I don't think we want this, even ratelimited.

> +				handled = false;
> +				insn = 0;
> +			}
> +		}
> +
> +		if (insn)
> +			handled = riscv_v_first_use_handler(regs, insn);
>  
>  		local_irq_disable();
>  
> diff --git a/arch/riscv/kernel/vector.c b/arch/riscv/kernel/vector.c
> index 821818886fab..08164a9121fe 100644
> --- a/arch/riscv/kernel/vector.c
> +++ b/arch/riscv/kernel/vector.c
> @@ -168,11 +168,8 @@ bool riscv_v_vstate_ctrl_user_allowed(void)
>  }
>  EXPORT_SYMBOL_GPL(riscv_v_vstate_ctrl_user_allowed);
>  
> -bool riscv_v_first_use_handler(struct pt_regs *regs)
> +bool riscv_v_first_use_handler(struct pt_regs *regs, u32 insn)
>  {
> -	u32 __user *epc = (u32 __user *)regs->epc;
> -	u32 insn = (u32)regs->badaddr;
> -
>  	if (!has_vector())
>  		return false;
>  
> @@ -184,12 +181,6 @@ bool riscv_v_first_use_handler(struct pt_regs *regs)
>  	if (riscv_v_vstate_query(regs))
>  		return false;
>  
> -	/* Get the instruction */
> -	if (!insn) {
> -		if (__get_user(insn, epc))
> -			return false;
> -	}
> -
>  	/* Filter out non-V instructions */
>  	if (!insn_is_vector(insn))
>  		return false;
> -- 
> 2.37.2.352.g3c44437643
>

Thanks,
drew

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