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

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

On 02/12/2024 09:16, Andrew Jones wrote:
> 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.
> 

Ok, it is a bit weird but I suppose it will end up generating a fault.



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