| Message ID | 20241201102759.221176-3-ben.dooks@codethink.co.uk (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/3] riscv: ptrace: add regs_set_register() | expand |
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
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 <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
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); 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;
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(-)