Messages in this thread Patch in this message | | | Date | Tue, 04 Aug 2020 19:01:01 -0700 (PDT) | Subject | Re: [PATCH 1/2] riscv: ptrace: Use the correct API for `fcsr' access | From | Palmer Dabbelt <> |
| |
On Thu, 23 Jul 2020 16:22:15 PDT (-0700), macro@wdc.com wrote: > Adjust the calls to `user_regset_copyout' and `user_regset_copyin' in > `riscv_fpr_get' and `riscv_fpr_set' respectively so as to use @start_pos > and @end_pos according to API documentation in <linux/regset.h>, that is > to point at the beginning and the end respectively of the data chunk to > be copied. Update @data accordingly, also for the first call, to make > it clear which structure member is accessed. > > We currently have @start_pos fixed at 0 across all calls, which works as > a result of the implementation, in particular because we have no padding > between the FP general registers and the FP control and status register, > but appears not to have been the intent of the API and is not what other > ports do, requiring one to study the copy handlers to understand what is > going on here. > > Signed-off-by: Maciej W. Rozycki <macro@wdc.com> > Fixes: b8c8a9590e4f ("RISC-V: Add FP register ptrace support for gdb.") > Cc: stable@vger.kernel.org # 4.20+ > --- > arch/riscv/kernel/ptrace.c | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > linux-riscv-ptrace-fcsr.diff > Index: linux-hv/arch/riscv/kernel/ptrace.c > =================================================================== > --- linux-hv.orig/arch/riscv/kernel/ptrace.c > +++ linux-hv/arch/riscv/kernel/ptrace.c > @@ -61,10 +61,13 @@ static int riscv_fpr_get(struct task_str > int ret; > struct __riscv_d_ext_state *fstate = &target->thread.fstate; > > - ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf, fstate, 0, > + ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf, &fstate->f, 0, > offsetof(struct __riscv_d_ext_state, fcsr));
As far as I can tell the current code works correctly, it just requires knowledge of the layout of __riscv_d_ext_state to determine that it functions correctly. This new code still requires that knowledge: the first blob copies the F registers, but only works if the CSR is after the registers. If we fix both of those the code seems easier to read, but I don't think splitting the difference helps any.
So I guess what I'm saying is: maybe that second line should be changed to something like "ARRAY_SIZE(fstate->f)"?
> if (!ret) { > - ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf, fstate, 0, > + ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf, > + &fstate->fcsr, > + offsetof(struct __riscv_d_ext_state, > + fcsr), > offsetof(struct __riscv_d_ext_state, fcsr) + > sizeof(fstate->fcsr)); > } > @@ -80,10 +83,13 @@ static int riscv_fpr_set(struct task_str > int ret; > struct __riscv_d_ext_state *fstate = &target->thread.fstate; > > - ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, fstate, 0, > + ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, &fstate->f, 0, > offsetof(struct __riscv_d_ext_state, fcsr)); > if (!ret) { > - ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, fstate, 0, > + ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, > + &fstate->fcsr, > + offsetof(struct __riscv_d_ext_state, > + fcsr), > offsetof(struct __riscv_d_ext_state, fcsr) + > sizeof(fstate->fcsr)); > }
| |