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