Linux-RISC-V Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/2] riscv: ptrace: NT_PRFPREG regset access fixes
@ 2020-07-23 23:21 Maciej W. Rozycki
  2020-07-23 23:22 ` [PATCH 1/2] riscv: ptrace: Use the correct API for `fcsr' access Maciej W. Rozycki
  2020-07-23 23:22 ` [PATCH 2/2] riscv: ptrace: Improve the style in NT_PRFPREG regset handling Maciej W. Rozycki
  0 siblings, 2 replies; 11+ messages in thread
From: Maciej W. Rozycki @ 2020-07-23 23:21 UTC (permalink / raw)
  To: linux-riscv; +Cc: Albert Ou, Palmer Dabbelt, linux-kernel, Paul Walmsley

Hi,

 In the course of adding RISC-V/Linux support to `gdbserver' I examined 
our Linux regset code to make sure I get the userland side right and 
discovered a discrepancy between how our internal `user_regset_copyout' 
and `user_regset_copyout' API has been defined and how we call it in the 
RISC-V backend in the handling of the NT_PRFPREG regset.  Additionally I 
found that code violating our coding style.

 To address these issues I made this small patch set, addressing each of 
them separately.  See individual change descriptions for details.

 These changes are hopefully obviously correct, however to make sure I 
have verified them manually with a HiFive Unleashed board running 
`gdbserver' controlled remotely by GDB and also by running the full GDB 
testsuite in the same environment using the lp64d ABI, without and with 
the changes applied.

 Please apply.

  Maciej

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/2] riscv: ptrace: Use the correct API for `fcsr' access
  2020-07-23 23:21 [PATCH 0/2] riscv: ptrace: NT_PRFPREG regset access fixes Maciej W. Rozycki
@ 2020-07-23 23:22 ` Maciej W. Rozycki
  2020-08-05  2:01   ` Palmer Dabbelt
  2020-07-23 23:22 ` [PATCH 2/2] riscv: ptrace: Improve the style in NT_PRFPREG regset handling Maciej W. Rozycki
  1 sibling, 1 reply; 11+ messages in thread
From: Maciej W. Rozycki @ 2020-07-23 23:22 UTC (permalink / raw)
  To: linux-riscv
  Cc: Albert Ou, Palmer Dabbelt, linux-kernel, stable, Paul Walmsley

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 2/2] riscv: ptrace: Improve the style in NT_PRFPREG regset handling
  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-07-23 23:22 ` Maciej W. Rozycki
  2020-08-05  2:01   ` Palmer Dabbelt
  1 sibling, 1 reply; 11+ messages in thread
From: Maciej W. Rozycki @ 2020-07-23 23:22 UTC (permalink / raw)
  To: linux-riscv; +Cc: Albert Ou, Palmer Dabbelt, linux-kernel, Paul Walmsley

Use an auxiliary variable for the size taken by floating point general 
registers in `struct __riscv_d_ext_state' to improve the readability of 
code in the `riscv_fpr_get' and `riscv_fpr_set' handlers, by avoiding 
excessive line wrapping and extending beyond 80 columns.  Also shuffle 
local variables in the reverse Christmas tree order.  No functional 
change.

Signed-off-by: Maciej W. Rozycki <macro@wdc.com>
---
 arch/riscv/kernel/ptrace.c |   24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

linux-riscv-ptrace-fpr-style.diff
Index: linux-hv/arch/riscv/kernel/ptrace.c
===================================================================
--- linux-hv.orig/arch/riscv/kernel/ptrace.c
+++ linux-hv/arch/riscv/kernel/ptrace.c
@@ -58,18 +58,16 @@ static int riscv_fpr_get(struct task_str
 			 unsigned int pos, unsigned int count,
 			 void *kbuf, void __user *ubuf)
 {
-	int ret;
+	const size_t fgr_size = offsetof(struct __riscv_d_ext_state, fcsr);
 	struct __riscv_d_ext_state *fstate = &target->thread.fstate;
+	int ret;
 
 	ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf, &fstate->f, 0,
-				  offsetof(struct __riscv_d_ext_state, fcsr));
+				  fgr_size);
 	if (!ret) {
 		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));
+					  &fstate->fcsr, fgr_size,
+					  fgr_size + sizeof(fstate->fcsr));
 	}
 
 	return ret;
@@ -80,18 +78,16 @@ static int riscv_fpr_set(struct task_str
 			 unsigned int pos, unsigned int count,
 			 const void *kbuf, const void __user *ubuf)
 {
-	int ret;
+	const size_t fgr_size = offsetof(struct __riscv_d_ext_state, fcsr);
 	struct __riscv_d_ext_state *fstate = &target->thread.fstate;
+	int ret;
 
 	ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, &fstate->f, 0,
-				 offsetof(struct __riscv_d_ext_state, fcsr));
+				 fgr_size);
 	if (!ret) {
 		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));
+					 &fstate->fcsr, fgr_size,
+					 fgr_size + sizeof(fstate->fcsr));
 	}
 
 	return ret;

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] riscv: ptrace: Use the correct API for `fcsr' access
  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
  2020-08-05  2:07     ` Al Viro
  0 siblings, 1 reply; 11+ messages in thread
From: Palmer Dabbelt @ 2020-08-05  2:01 UTC (permalink / raw)
  To: macro; +Cc: linux-riscv, aou, linux-kernel, stable, Paul Walmsley

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] riscv: ptrace: Improve the style in NT_PRFPREG regset handling
  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
  0 siblings, 0 replies; 11+ messages in thread
From: Palmer Dabbelt @ 2020-08-05  2:01 UTC (permalink / raw)
  To: macro; +Cc: linux-riscv, aou, linux-kernel, Paul Walmsley

On Thu, 23 Jul 2020 16:22:30 PDT (-0700), macro@wdc.com wrote:
> Use an auxiliary variable for the size taken by floating point general
> registers in `struct __riscv_d_ext_state' to improve the readability of
> code in the `riscv_fpr_get' and `riscv_fpr_set' handlers, by avoiding
> excessive line wrapping and extending beyond 80 columns.  Also shuffle
> local variables in the reverse Christmas tree order.  No functional
> change.
>
> Signed-off-by: Maciej W. Rozycki <macro@wdc.com>
> ---
>  arch/riscv/kernel/ptrace.c |   24 ++++++++++--------------
>  1 file changed, 10 insertions(+), 14 deletions(-)
>
> linux-riscv-ptrace-fpr-style.diff
> Index: linux-hv/arch/riscv/kernel/ptrace.c
> ===================================================================
> --- linux-hv.orig/arch/riscv/kernel/ptrace.c
> +++ linux-hv/arch/riscv/kernel/ptrace.c
> @@ -58,18 +58,16 @@ static int riscv_fpr_get(struct task_str
>  			 unsigned int pos, unsigned int count,
>  			 void *kbuf, void __user *ubuf)
>  {
> -	int ret;
> +	const size_t fgr_size = offsetof(struct __riscv_d_ext_state, fcsr);
>  	struct __riscv_d_ext_state *fstate = &target->thread.fstate;
> +	int ret;
>
>  	ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf, &fstate->f, 0,
> -				  offsetof(struct __riscv_d_ext_state, fcsr));
> +				  fgr_size);
>  	if (!ret) {
>  		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));
> +					  &fstate->fcsr, fgr_size,
> +					  fgr_size + sizeof(fstate->fcsr));
>  	}
>
>  	return ret;
> @@ -80,18 +78,16 @@ static int riscv_fpr_set(struct task_str
>  			 unsigned int pos, unsigned int count,
>  			 const void *kbuf, const void __user *ubuf)
>  {
> -	int ret;
> +	const size_t fgr_size = offsetof(struct __riscv_d_ext_state, fcsr);
>  	struct __riscv_d_ext_state *fstate = &target->thread.fstate;
> +	int ret;
>
>  	ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, &fstate->f, 0,
> -				 offsetof(struct __riscv_d_ext_state, fcsr));
> +				 fgr_size);
>  	if (!ret) {
>  		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));
> +					 &fstate->fcsr, fgr_size,
> +					 fgr_size + sizeof(fstate->fcsr));
>  	}
>
>  	return ret;

Reviewed-by: Palmer Dabbelt <palmerdabbelt@google.com>

(Though the comments in the first patch apply here)

Thanks!

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] riscv: ptrace: Use the correct API for `fcsr' access
  2020-08-05  2:01   ` Palmer Dabbelt
@ 2020-08-05  2:07     ` Al Viro
  2020-08-05  2:20       ` Palmer Dabbelt
  0 siblings, 1 reply; 11+ messages in thread
From: Al Viro @ 2020-08-05  2:07 UTC (permalink / raw)
  To: Palmer Dabbelt
  Cc: aou, linux-kernel, stable, Paul Walmsley, macro, linux-riscv

On Tue, Aug 04, 2020 at 07:01:01PM -0700, Palmer Dabbelt wrote:

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

start_pos *is* fixed at 0 and it's going to go away, along with the
sodding user_regset_copyout() very shortly.  ->get() is simply a bad API.
See vfs.git#work.regset for replacement.  And ->put() is also going to be
taken out and shot (next cycle, most likely).

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] riscv: ptrace: Use the correct API for `fcsr' access
  2020-08-05  2:07     ` Al Viro
@ 2020-08-05  2:20       ` Palmer Dabbelt
  2020-08-05  2:48         ` Al Viro
  0 siblings, 1 reply; 11+ messages in thread
From: Palmer Dabbelt @ 2020-08-05  2:20 UTC (permalink / raw)
  To: viro; +Cc: aou, linux-kernel, stable, Paul Walmsley, macro, linux-riscv

On Tue, 04 Aug 2020 19:07:45 PDT (-0700), viro@zeniv.linux.org.uk wrote:
> On Tue, Aug 04, 2020 at 07:01:01PM -0700, Palmer Dabbelt wrote:
>
>> > 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.
>
> start_pos *is* fixed at 0 and it's going to go away, along with the
> sodding user_regset_copyout() very shortly.  ->get() is simply a bad API.
> See vfs.git#work.regset for replacement.  And ->put() is also going to be
> taken out and shot (next cycle, most likely).

I'm not sure I understand what you're saying, but given that branch replaces
all of this I guess it's best to just do nothing on our end here?

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] riscv: ptrace: Use the correct API for `fcsr' access
  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
  0 siblings, 2 replies; 11+ messages in thread
From: Al Viro @ 2020-08-05  2:48 UTC (permalink / raw)
  To: Palmer Dabbelt
  Cc: aou, linux-kernel, stable, Paul Walmsley, macro, linux-riscv

On Tue, Aug 04, 2020 at 07:20:05PM -0700, Palmer Dabbelt wrote:
> On Tue, 04 Aug 2020 19:07:45 PDT (-0700), viro@zeniv.linux.org.uk wrote:
> > On Tue, Aug 04, 2020 at 07:01:01PM -0700, Palmer Dabbelt wrote:
> > 
> > > > 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.
> > 
> > start_pos *is* fixed at 0 and it's going to go away, along with the
> > sodding user_regset_copyout() very shortly.  ->get() is simply a bad API.
> > See vfs.git#work.regset for replacement.  And ->put() is also going to be
> > taken out and shot (next cycle, most likely).
> 
> I'm not sure I understand what you're saying, but given that branch replaces
> all of this I guess it's best to just do nothing on our end here?

It doesn't replace ->put() (for now); it _does_ replace ->get() and AFAICS the
replacement is much saner:

static int riscv_fpr_get(struct task_struct *target,
                         const struct user_regset *regset,
                         struct membuf to)
{
	struct __riscv_d_ext_state *fstate = &target->thread.fstate;

	membuf_write(&to, fstate, offsetof(struct __riscv_d_ext_state, fcsr));
	membuf_store(&to, fstate->fcsr);
	return membuf_zero(&to, 4);     // explicitly pad
}

user_regset_copyout() calling conventions are atrocious and so are those of
regset ->get().  The best thing to do with both is to take them out of their
misery and be done with that.  Do you see any problems with riscv gdbserver
on current linux-next?  If not, I'd rather see that "API" simply go away...
If there are problems, I would very much prefer fixes on top of what's done
in that branch.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] riscv: ptrace: Use the correct API for `fcsr' access
  2020-08-05  2:48         ` Al Viro
@ 2020-08-05  3:15           ` Palmer Dabbelt
  2020-08-05 10:25           ` Maciej W. Rozycki
  1 sibling, 0 replies; 11+ messages in thread
From: Palmer Dabbelt @ 2020-08-05  3:15 UTC (permalink / raw)
  To: viro; +Cc: aou, linux-kernel, stable, Paul Walmsley, macro, linux-riscv

On Tue, 04 Aug 2020 19:48:07 PDT (-0700), viro@zeniv.linux.org.uk wrote:
> On Tue, Aug 04, 2020 at 07:20:05PM -0700, Palmer Dabbelt wrote:
>> On Tue, 04 Aug 2020 19:07:45 PDT (-0700), viro@zeniv.linux.org.uk wrote:
>> > On Tue, Aug 04, 2020 at 07:01:01PM -0700, Palmer Dabbelt wrote:
>> >
>> > > > 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.
>> >
>> > start_pos *is* fixed at 0 and it's going to go away, along with the
>> > sodding user_regset_copyout() very shortly.  ->get() is simply a bad API.
>> > See vfs.git#work.regset for replacement.  And ->put() is also going to be
>> > taken out and shot (next cycle, most likely).
>>
>> I'm not sure I understand what you're saying, but given that branch replaces
>> all of this I guess it's best to just do nothing on our end here?
>
> It doesn't replace ->put() (for now); it _does_ replace ->get() and AFAICS the
> replacement is much saner:
>
> static int riscv_fpr_get(struct task_struct *target,
>                          const struct user_regset *regset,
>                          struct membuf to)
> {
> 	struct __riscv_d_ext_state *fstate = &target->thread.fstate;
>
> 	membuf_write(&to, fstate, offsetof(struct __riscv_d_ext_state, fcsr));
> 	membuf_store(&to, fstate->fcsr);
> 	return membuf_zero(&to, 4);     // explicitly pad
> }
>
> user_regset_copyout() calling conventions are atrocious and so are those of
> regset ->get().  The best thing to do with both is to take them out of their
> misery and be done with that.  Do you see any problems with riscv gdbserver
> on current linux-next?  If not, I'd rather see that "API" simply go away...
> If there are problems, I would very much prefer fixes on top of what's done
> in that branch.

I guess my confusion was about "start_pos *is* fixed at 0": it certainly is
zero in the code right now, but when poking around while review the patch I
didn't see any reason that must be so.  Admittedly all I did was read the
prototype and function, so maybe I'm just missing something.  That said, if
it's all going away anyway then I don't really care either way.

As far as I can tell the patch set in question (the RISC-V one) doesn't change
any functionality.  I don't actually use GDB, but I haven't seen any issues
reported in a few years so if there is one I've missed it.

I did this ptrace stuff many years ago (IIRC it was actually my first RISC-V
Linux patch), and all I really remember is that it seemed way more complicated
than it needed to be.  I'm happy to just drop our patch set, as yours looks way
cleaner to me and if you're already planning on fixing put() then it doesn't
seem worth the churn.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] riscv: ptrace: Use the correct API for `fcsr' access
  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
  1 sibling, 1 reply; 11+ messages in thread
From: Maciej W. Rozycki @ 2020-08-05 10:25 UTC (permalink / raw)
  To: Al Viro
  Cc: aou, linux-kernel, stable, Palmer Dabbelt, Paul Walmsley, linux-riscv

On Wed, 5 Aug 2020, Al Viro wrote:

> > I'm not sure I understand what you're saying, but given that branch replaces
> > all of this I guess it's best to just do nothing on our end here?
> 
> It doesn't replace ->put() (for now); it _does_ replace ->get() and AFAICS the
> replacement is much saner:
> 
> static int riscv_fpr_get(struct task_struct *target,
>                          const struct user_regset *regset,
>                          struct membuf to)
> {
> 	struct __riscv_d_ext_state *fstate = &target->thread.fstate;
> 
> 	membuf_write(&to, fstate, offsetof(struct __riscv_d_ext_state, fcsr));
> 	membuf_store(&to, fstate->fcsr);
> 	return membuf_zero(&to, 4);     // explicitly pad
> }

 I'm glad to see the old interface go, it was cumbersome.

> user_regset_copyout() calling conventions are atrocious and so are those of
> regset ->get().  The best thing to do with both is to take them out of their
> misery and be done with that.  Do you see any problems with riscv gdbserver
> on current linux-next?  If not, I'd rather see that "API" simply go away...
> If there are problems, I would very much prefer fixes on top of what's done
> in that branch.

 I can push linux-next through regression-testing with RISC-V gdbserver 
and/or native GDB if that would help.  This is also used with core dumps, 
but honestly I don't know what state RISC-V support is in in the BFD/GDB's 
core dump interpreter, as people tend to forget about the core dump 
feature nowadays.

  Maciej

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] riscv: ptrace: Use the correct API for `fcsr' access
  2020-08-05 10:25           ` Maciej W. Rozycki
@ 2020-08-05 19:48             ` Palmer Dabbelt
  0 siblings, 0 replies; 11+ messages in thread
From: Palmer Dabbelt @ 2020-08-05 19:48 UTC (permalink / raw)
  To: macro; +Cc: aou, linux-kernel, stable, viro, Paul Walmsley, linux-riscv

On Wed, 05 Aug 2020 03:25:11 PDT (-0700), macro@wdc.com wrote:
> On Wed, 5 Aug 2020, Al Viro wrote:
>
>> > I'm not sure I understand what you're saying, but given that branch replaces
>> > all of this I guess it's best to just do nothing on our end here?
>>
>> It doesn't replace ->put() (for now); it _does_ replace ->get() and AFAICS the
>> replacement is much saner:
>>
>> static int riscv_fpr_get(struct task_struct *target,
>>                          const struct user_regset *regset,
>>                          struct membuf to)
>> {
>> 	struct __riscv_d_ext_state *fstate = &target->thread.fstate;
>>
>> 	membuf_write(&to, fstate, offsetof(struct __riscv_d_ext_state, fcsr));
>> 	membuf_store(&to, fstate->fcsr);
>> 	return membuf_zero(&to, 4);     // explicitly pad
>> }
>
>  I'm glad to see the old interface go, it was cumbersome.
>
>> user_regset_copyout() calling conventions are atrocious and so are those of
>> regset ->get().  The best thing to do with both is to take them out of their
>> misery and be done with that.  Do you see any problems with riscv gdbserver
>> on current linux-next?  If not, I'd rather see that "API" simply go away...
>> If there are problems, I would very much prefer fixes on top of what's done
>> in that branch.
>
>  I can push linux-next through regression-testing with RISC-V gdbserver
> and/or native GDB if that would help.  This is also used with core dumps,
> but honestly I don't know what state RISC-V support is in in the BFD/GDB's
> core dump interpreter, as people tend to forget about the core dump
> feature nowadays.

IIRC Andrew does GDB test suite runs sometimes natively on Linux as part of
general GDB maintiance and we don't see major issues, but I'm pretty checked
out of GDB development these days so he would know better than I do.  It's
always great to have someone test stuff, though -- and I doubt he's testing
linux-next.  It's been on my TODO list for a long time now to put together
tip-of-tree testing for the various projects but I've never gotten around to
doing it.

Oddly enough, despite not really using GDB I have used it for core dumps -- I
was writing a tool to convert commit logs to coredumps with the GDB reverse
debugging annotations, but I never got around to finishing it.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, back to index

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
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-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

Linux-RISC-V Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-riscv/0 linux-riscv/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-riscv linux-riscv/ https://lore.kernel.org/linux-riscv \
		linux-riscv@lists.infradead.org
	public-inbox-index linux-riscv

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-riscv


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git