From: Palmer Dabbelt <palmer@dabbelt.com>
To: macro@wdc.com
Cc: linux-riscv@lists.infradead.org, aou@eecs.berkeley.edu,
linux-kernel@vger.kernel.org, stable@vger.kernel.org,
Paul Walmsley <paul.walmsley@sifive.com>
Subject: Re: [PATCH 1/2] riscv: ptrace: Use the correct API for `fcsr' access
Date: Tue, 04 Aug 2020 19:01:01 -0700 (PDT) [thread overview]
Message-ID: <mhng-611be025-6e03-46a1-8a8a-6f6eeea04f57@palmerdabbelt-glaptop1> (raw)
In-Reply-To: <alpine.DEB.2.20.2007232213510.4462@tpp.orcam.me.uk>
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));
> }
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
next prev parent reply other threads:[~2020-08-05 2:01 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-23 23:21 [PATCH 0/2] riscv: ptrace: NT_PRFPREG regset access fixes Maciej W. Rozycki
2020-07-23 23:22 ` [PATCH 1/2] riscv: ptrace: Use the correct API for `fcsr' access Maciej W. Rozycki
2020-08-05 2:01 ` Palmer Dabbelt [this message]
2020-08-05 2:07 ` Al Viro
2020-08-05 2:20 ` Palmer Dabbelt
2020-08-05 2:48 ` Al Viro
2020-08-05 3:15 ` Palmer Dabbelt
2020-08-05 10:25 ` Maciej W. Rozycki
2020-08-05 19:48 ` Palmer Dabbelt
2020-08-19 20:00 ` Maciej W. Rozycki
2020-07-23 23:22 ` [PATCH 2/2] riscv: ptrace: Improve the style in NT_PRFPREG regset handling Maciej W. Rozycki
2020-08-05 2:01 ` Palmer Dabbelt
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=mhng-611be025-6e03-46a1-8a8a-6f6eeea04f57@palmerdabbelt-glaptop1 \
--to=palmer@dabbelt.com \
--cc=aou@eecs.berkeley.edu \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-riscv@lists.infradead.org \
--cc=macro@wdc.com \
--cc=paul.walmsley@sifive.com \
--cc=stable@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).