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

[Ben Dooks <ben.dooks@codethink.co.uk>]

On 13/11/2024 09:17, 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
> ---
>   arch/riscv/include/asm/vector.h |  4 ++--
>   arch/riscv/kernel/traps.c       | 11 ++++++++++-
>   arch/riscv/kernel/vector.c      | 11 +----------
>   3 files changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/vector.h b/arch/riscv/include/asm/vector.h
> index be7d309cca8a..c9f0b02cd975 100644
> --- a/arch/riscv/include/asm/vector.h
> +++ b/arch/riscv/include/asm/vector.h
> @@ -21,7 +21,7 @@
>   
>   extern unsigned long riscv_v_vsize;
>   int riscv_v_setup_vsize(void);
> -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);
> @@ -268,7 +268,7 @@ struct pt_regs;
>   
>   static inline int riscv_v_setup_vsize(void) { return -EOPNOTSUPP; }
>   static __always_inline bool has_vector(void) { 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..1c3fab272fd1 100644
> --- a/arch/riscv/kernel/traps.c
> +++ b/arch/riscv/kernel/traps.c
> @@ -172,11 +172,20 @@ 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)) {
> +				/* todo */
> +			}

grr, of course as soon as it is sent I notice the /todo/ here

Not sure if we should print something if __get_user() fails for
what /should/ be an good instruction addrss.... should probably
at-least bail out of do_trap_insn_illegal() if not also print a
warning.

> +		}
> +
> +		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 682b3feee451..b852648cb8d5 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;


-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

https://www.codethink.co.uk/privacy.html

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