public inbox for linux-riscv@lists.infradead.org
 help / color / mirror / Atom feed
* Re: [PATCH] RISC-V: support for vector register accesses via ptrace() in RISC-V Linux native
       [not found]   ` <CADdv1FqjLPZ-eWOKPv0uZxF-u-SYjn0WJGr3KWW9H06-O0L35w@mail.gmail.com>
@ 2023-08-09 23:09     ` Maciej W. Rozycki
  2023-08-10 10:35       ` Andy Chiu
  0 siblings, 1 reply; 10+ messages in thread
From: Maciej W. Rozycki @ 2023-08-09 23:09 UTC (permalink / raw)
  To: Greg Savin, Greentime Hu, Andy Chiu
  Cc: linux-riscv, gdb-patches, Andrew Burgess

On Wed, 9 Aug 2023, Greg Savin wrote:

> The SIGILL guard is being used as a wrapper around determination of the
> VLENB CSR, which is not part of the ptrace() payload for vector registers,
> at least as it exists at head-of-tree Linux kernel.   GDB or gdbserver
> needs to know VLENB in order to construct the architectural feature
> metadata that reports an accurate width for the vector registers.  If not
> for the VLENB determination specifically, and the lack of this information
> via ptrace(), then there would be no motivation for executing a vector
> instruction directly.  It's a workaround, basically.  I guess I could
> inquire in Linux kernel land regarding whether the NT_RISCV_VECTOR ptrace()
> payload could be enhanced to provide VLENB.

 I think the kernel interface needs to be clarified first, before we can 
proceed with the tools side.

 I can see the vector state is carried in a REGSET_V regset, which in turn 
corresponds to an NT_RISCV_VECTOR core file note.  I can see that besides 
the vector data registers only the VSTART, VL, VTYPE, and VCSR vector CSRs
are provided in that regset, and that vector data registers are assigned 
a contiguous space of (32 * RISCV_MAX_VLENB) bytes rather than individual 
slots.

 So how are we supposed to determine the width of the vector registers 
recorded in a core file?  I'd say the RISC-V/Linux kernel regset API is 
incomplete.

 A complete API has to provide `ptrace' and core file access to all the 
relevant registers (vector registers in this case) that can be accessed by 
machine instructions by the debuggee.  That includes read-only registers, 
writes to which via `ptrace' will of course be ignored.  If a register is 
a shadow only and can be reconstructed from another, canonical register 
(e.g. VXRM vs VCSR) then the shadow register can (and best be) omitted of 
course.  Additional artificial OS registers may also have to be provided 
that reflect the relevant privileged state made available to the debuggee 
at run time by OS calls such as prctl(2); this for example might be a mode 
setting which affects the hardware interpretation of a register set that 
debug tools may need to take into account or the person debugging may want 
to check or modify (e.g. REGSET_FP_MODE in the MIPS/Linux port).

 I've added the authors of the Linux kernel code and the RISC-V/Linux 
mailing list to the list of recipients.  Am I missing anything here?

  Maciej

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

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

* Re: [PATCH] RISC-V: support for vector register accesses via ptrace() in RISC-V Linux native
  2023-08-09 23:09     ` [PATCH] RISC-V: support for vector register accesses via ptrace() in RISC-V Linux native Maciej W. Rozycki
@ 2023-08-10 10:35       ` Andy Chiu
  2023-08-10 11:40         ` Maciej W. Rozycki
  0 siblings, 1 reply; 10+ messages in thread
From: Andy Chiu @ 2023-08-10 10:35 UTC (permalink / raw)
  To: Maciej W. Rozycki
  Cc: Greg Savin, Greentime Hu, linux-riscv, gdb-patches,
	Andrew Burgess

On Thu, Aug 10, 2023 at 12:09:17AM +0100, Maciej W. Rozycki wrote:
> On Wed, 9 Aug 2023, Greg Savin wrote:
> 
> > The SIGILL guard is being used as a wrapper around determination of the
> > VLENB CSR, which is not part of the ptrace() payload for vector registers,
> > at least as it exists at head-of-tree Linux kernel.   GDB or gdbserver
> > needs to know VLENB in order to construct the architectural feature
> > metadata that reports an accurate width for the vector registers.  If not
> > for the VLENB determination specifically, and the lack of this information
> > via ptrace(), then there would be no motivation for executing a vector
> > instruction directly.  It's a workaround, basically.  I guess I could
> > inquire in Linux kernel land regarding whether the NT_RISCV_VECTOR ptrace()
> > payload could be enhanced to provide VLENB.
> 
>  I think the kernel interface needs to be clarified first, before we can 
> proceed with the tools side.
> 
>  I can see the vector state is carried in a REGSET_V regset, which in turn 
> corresponds to an NT_RISCV_VECTOR core file note.  I can see that besides 
> the vector data registers only the VSTART, VL, VTYPE, and VCSR vector CSRs
> are provided in that regset, and that vector data registers are assigned 
> a contiguous space of (32 * RISCV_MAX_VLENB) bytes rather than individual 
> slots.
> 
>  So how are we supposed to determine the width of the vector registers 
> recorded in a core file?  I'd say the RISC-V/Linux kernel regset API is 
> incomplete.

Does it make sense to you if we encapsulate this with a hwprobe syscall?
e.g provide a hwprobe entry to get system's VLENB. We will have to
increase and rearrange the buffer for NT_RISCV_VECTOR if we want to use
ptrace as the entry point for this purpose. I am not very sure if it'd be
too late to do though.

> 
>  A complete API has to provide `ptrace' and core file access to all the 
> relevant registers (vector registers in this case) that can be accessed by 
> machine instructions by the debuggee.  That includes read-only registers, 
> writes to which via `ptrace' will of course be ignored.  If a register is 
> a shadow only and can be reconstructed from another, canonical register 
> (e.g. VXRM vs VCSR) then the shadow register can (and best be) omitted of 
> course.  Additional artificial OS registers may also have to be provided 
> that reflect the relevant privileged state made available to the debuggee 
> at run time by OS calls such as prctl(2); this for example might be a mode 
> setting which affects the hardware interpretation of a register set that 
> debug tools may need to take into account or the person debugging may want 
> to check or modify (e.g. REGSET_FP_MODE in the MIPS/Linux port).
> 
>  I've added the authors of the Linux kernel code and the RISC-V/Linux 
> mailing list to the list of recipients.  Am I missing anything here?
> 
>   Maciej

Andy

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

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

* Re: [PATCH] RISC-V: support for vector register accesses via ptrace() in RISC-V Linux native
  2023-08-10 10:35       ` Andy Chiu
@ 2023-08-10 11:40         ` Maciej W. Rozycki
  2023-08-10 13:55           ` Maciej W. Rozycki
  2023-08-10 14:05           ` Andy Chiu
  0 siblings, 2 replies; 10+ messages in thread
From: Maciej W. Rozycki @ 2023-08-10 11:40 UTC (permalink / raw)
  To: Andy Chiu
  Cc: Greg Savin, Greentime Hu, linux-riscv, gdb-patches,
	Andrew Burgess

On Thu, 10 Aug 2023, Andy Chiu wrote:

> > > The SIGILL guard is being used as a wrapper around determination of the
> > > VLENB CSR, which is not part of the ptrace() payload for vector registers,
> > > at least as it exists at head-of-tree Linux kernel.   GDB or gdbserver
> > > needs to know VLENB in order to construct the architectural feature
> > > metadata that reports an accurate width for the vector registers.  If not
> > > for the VLENB determination specifically, and the lack of this information
> > > via ptrace(), then there would be no motivation for executing a vector
> > > instruction directly.  It's a workaround, basically.  I guess I could
> > > inquire in Linux kernel land regarding whether the NT_RISCV_VECTOR ptrace()
> > > payload could be enhanced to provide VLENB.
> > 
> >  I think the kernel interface needs to be clarified first, before we can 
> > proceed with the tools side.
> > 
> >  I can see the vector state is carried in a REGSET_V regset, which in turn 
> > corresponds to an NT_RISCV_VECTOR core file note.  I can see that besides 
> > the vector data registers only the VSTART, VL, VTYPE, and VCSR vector CSRs
> > are provided in that regset, and that vector data registers are assigned 
> > a contiguous space of (32 * RISCV_MAX_VLENB) bytes rather than individual 
> > slots.
> > 
> >  So how are we supposed to determine the width of the vector registers 
> > recorded in a core file?  I'd say the RISC-V/Linux kernel regset API is 
> > incomplete.
> 
> Does it make sense to you if we encapsulate this with a hwprobe syscall?
> e.g provide a hwprobe entry to get system's VLENB. We will have to
> increase and rearrange the buffer for NT_RISCV_VECTOR if we want to use
> ptrace as the entry point for this purpose. I am not very sure if it'd be
> too late to do though.

 No, how do you expect it to work with a core dump (that can be examined 
on a different system, or with a cross-debugger)?  You need to change the 
API I'm afraid; it's unusable anyway.  It's a pity the toolchain community 
wasn't consulted if you weren't sure how to design the interface.  Better 
yet it would have been to implement the GDB side before the kernel part 
has been committed.

  Maciej

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

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

* Re: [PATCH] RISC-V: support for vector register accesses via ptrace() in RISC-V Linux native
  2023-08-10 11:40         ` Maciej W. Rozycki
@ 2023-08-10 13:55           ` Maciej W. Rozycki
  2023-08-10 17:23             ` Andy Chiu
  2023-08-10 14:05           ` Andy Chiu
  1 sibling, 1 reply; 10+ messages in thread
From: Maciej W. Rozycki @ 2023-08-10 13:55 UTC (permalink / raw)
  To: Andy Chiu
  Cc: Greg Savin, Greentime Hu, Oleg Nesterov, Paul Walmsley,
	Palmer Dabbelt, Albert Ou, linux-riscv, gdb-patches,
	Andrew Burgess

On Thu, 10 Aug 2023, Maciej W. Rozycki wrote:

> > Does it make sense to you if we encapsulate this with a hwprobe syscall?
> > e.g provide a hwprobe entry to get system's VLENB. We will have to
> > increase and rearrange the buffer for NT_RISCV_VECTOR if we want to use
> > ptrace as the entry point for this purpose. I am not very sure if it'd be
> > too late to do though.
> 
>  No, how do you expect it to work with a core dump (that can be examined 
> on a different system, or with a cross-debugger)?  You need to change the 
> API I'm afraid; it's unusable anyway.  It's a pity the toolchain community 
> wasn't consulted if you weren't sure how to design the interface.  Better 
> yet it would have been to implement the GDB side before the kernel part 
> has been committed.

 NB since this stuff went in with v6.5-rc1 and v6.5 hasn't been released 
you can still back out the problematic change as no one is expected to use 
RC stuff in production.  Alternatively you can redefine NT_RISCV_VECTOR 
for a corrected ABI, but I think it shouldn't be necessary.  You just need 
to act quickly as I guess there may be 1-2 further v6.5 RCs only and you 
have to get with that to Linus right away.  We can have a release or two 
without NT_RISCV_VECTOR support for the otherwise included vector stuff, 
it shouldn't be a big deal.  There just won't be support for the debug 
API.

 CC-ing Linux ptrace/RISC-V maintainers now to bring their attention.

  Maciej

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

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

* Re: [PATCH] RISC-V: support for vector register accesses via ptrace() in RISC-V Linux native
  2023-08-10 11:40         ` Maciej W. Rozycki
  2023-08-10 13:55           ` Maciej W. Rozycki
@ 2023-08-10 14:05           ` Andy Chiu
  2023-08-10 20:51             ` Maciej W. Rozycki
  1 sibling, 1 reply; 10+ messages in thread
From: Andy Chiu @ 2023-08-10 14:05 UTC (permalink / raw)
  To: Maciej W. Rozycki
  Cc: Greg Savin, Greentime Hu, linux-riscv, gdb-patches,
	Andrew Burgess

Hi Maciej,

On Thu, Aug 10, 2023 at 12:40:12PM +0100, Maciej W. Rozycki wrote:
> On Thu, 10 Aug 2023, Andy Chiu wrote:
> 
> > > > The SIGILL guard is being used as a wrapper around determination of the
> > > > VLENB CSR, which is not part of the ptrace() payload for vector registers,
> > > > at least as it exists at head-of-tree Linux kernel.   GDB or gdbserver
> > > > needs to know VLENB in order to construct the architectural feature
> > > > metadata that reports an accurate width for the vector registers.  If not
> > > > for the VLENB determination specifically, and the lack of this information
> > > > via ptrace(), then there would be no motivation for executing a vector
> > > > instruction directly.  It's a workaround, basically.  I guess I could
> > > > inquire in Linux kernel land regarding whether the NT_RISCV_VECTOR ptrace()
> > > > payload could be enhanced to provide VLENB.
> > > 
> > >  I think the kernel interface needs to be clarified first, before we can 
> > > proceed with the tools side.
> > > 
> > >  I can see the vector state is carried in a REGSET_V regset, which in turn 
> > > corresponds to an NT_RISCV_VECTOR core file note.  I can see that besides 
> > > the vector data registers only the VSTART, VL, VTYPE, and VCSR vector CSRs
> > > are provided in that regset, and that vector data registers are assigned 
> > > a contiguous space of (32 * RISCV_MAX_VLENB) bytes rather than individual 
> > > slots.
> > > 
> > >  So how are we supposed to determine the width of the vector registers 
> > > recorded in a core file?  I'd say the RISC-V/Linux kernel regset API is 
> > > incomplete.
> > 
> > Does it make sense to you if we encapsulate this with a hwprobe syscall?
> > e.g provide a hwprobe entry to get system's VLENB. We will have to
> > increase and rearrange the buffer for NT_RISCV_VECTOR if we want to use
> > ptrace as the entry point for this purpose. I am not very sure if it'd be
> > too late to do though.
> 
>  No, how do you expect it to work with a core dump (that can be examined 
> on a different system, or with a cross-debugger)?  You need to change the 
> API I'm afraid; it's unusable anyway.  It's a pity the toolchain community 
> wasn't consulted if you weren't sure how to design the interface.  Better 
> yet it would have been to implement the GDB side before the kernel part 
> has been committed.

Conor just reminded me that we may still have a chance to get it right
since 6.5 has not been released yet. I will send a fix patch to address
this issue once the discussion settle down. After looking into some
code, I think it is possbile to steal the unused space in datap and
change the uapi with something like this:

diff --git a/arch/riscv/include/uapi/asm/ptrace.h b/arch/riscv/include/uapi/asm/ptrace.h
index e17c550986a6..ba6ddf4f9dc9 100644
--- a/arch/riscv/include/uapi/asm/ptrace.h
+++ b/arch/riscv/include/uapi/asm/ptrace.h
@@ -97,14 +97,17 @@ struct __riscv_v_ext_state {
 	unsigned long vl;
 	unsigned long vtype;
 	unsigned long vcsr;
-	void *datap;
+	union {
+		void *datap;
+		unsigned long vlenb;
+	};
 	/*
 	 * In signal handler, datap will be set a correct user stack offset
 	 * and vector registers will be copied to the address of datap
 	 * pointer.
 	 *
-	 * In ptrace syscall, datap will be set to zero and the vector
-	 * registers will be copied to the address right after this
+	 * In ptrace syscall, the space for datap will be set to vlenb and the
+	 * vector registers will be copied to the address right after this
 	 * structure.
 	 */
 };

Now ptrace will have the knowlege of vlen to parse V rsgisters. And this
will not cause any size change to the original data structure that is
shared by both signal and ptrace because vlenb is XLEN, which has the
same size as a pointer in both ilp32/lp64.

> 
>   Maciej

Thanks,
Andy

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

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

* Re: [PATCH] RISC-V: support for vector register accesses via ptrace() in RISC-V Linux native
  2023-08-10 13:55           ` Maciej W. Rozycki
@ 2023-08-10 17:23             ` Andy Chiu
  2023-08-10 21:08               ` Palmer Dabbelt
  2023-08-10 21:21               ` Maciej W. Rozycki
  0 siblings, 2 replies; 10+ messages in thread
From: Andy Chiu @ 2023-08-10 17:23 UTC (permalink / raw)
  To: Maciej W. Rozycki
  Cc: Greg Savin, Greentime Hu, Oleg Nesterov, Paul Walmsley,
	Palmer Dabbelt, Albert Ou, linux-riscv, gdb-patches,
	Andrew Burgess

On Thu, Aug 10, 2023 at 9:55 PM Maciej W. Rozycki <macro@orcam.me.uk> wrote:
>
> On Thu, 10 Aug 2023, Maciej W. Rozycki wrote:
>
> > > Does it make sense to you if we encapsulate this with a hwprobe syscall?
> > > e.g provide a hwprobe entry to get system's VLENB. We will have to
> > > increase and rearrange the buffer for NT_RISCV_VECTOR if we want to use
> > > ptrace as the entry point for this purpose. I am not very sure if it'd be
> > > too late to do though.
> >
> >  No, how do you expect it to work with a core dump (that can be examined
> > on a different system, or with a cross-debugger)?  You need to change the
> > API I'm afraid; it's unusable anyway.  It's a pity the toolchain community
> > wasn't consulted if you weren't sure how to design the interface.  Better
> > yet it would have been to implement the GDB side before the kernel part
> > has been committed.

I just took some look into the code and here is what I came up with.
Actually, you know VLENB in a core dump file. The size of
NT_RISCV_VECTOR in a core dump file just equals sizeof(struct
__riscv_v_ext_state), which is 40B, plus VLENB * 32. So, the debugger
can actually calculate VLENB and resolve placement of V registers by
subtracting 40 from the size of NT_RISCV_VECTOR in a core dump file.

On the other hand, ptrace is not so lucky. The kernel will return the
min of either user specified size or the maximum Vector size. It is
still safe if we consider SMP with the same VLENB across cores though,
which is an assumption made on Linux. We just need a way to get VLENB
on the system.

>
>  NB since this stuff went in with v6.5-rc1 and v6.5 hasn't been released
> you can still back out the problematic change as no one is expected to use
> RC stuff in production.  Alternatively you can redefine NT_RISCV_VECTOR
> for a corrected ABI, but I think it shouldn't be necessary.  You just need
> to act quickly as I guess there may be 1-2 further v6.5 RCs only and you
> have to get with that to Linus right away.  We can have a release or two
> without NT_RISCV_VECTOR support for the otherwise included vector stuff,
> it shouldn't be a big deal.  There just won't be support for the debug
> API.
>
>  CC-ing Linux ptrace/RISC-V maintainers now to bring their attention.
>
>   Maciej

Thanks,
Andy

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

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

* Re: [PATCH] RISC-V: support for vector register accesses via ptrace() in RISC-V Linux native
  2023-08-10 14:05           ` Andy Chiu
@ 2023-08-10 20:51             ` Maciej W. Rozycki
  0 siblings, 0 replies; 10+ messages in thread
From: Maciej W. Rozycki @ 2023-08-10 20:51 UTC (permalink / raw)
  To: Andy Chiu
  Cc: Greg Savin, Greentime Hu, linux-riscv, gdb-patches,
	Andrew Burgess

Hi Andy,

> > > Does it make sense to you if we encapsulate this with a hwprobe syscall?
> > > e.g provide a hwprobe entry to get system's VLENB. We will have to
> > > increase and rearrange the buffer for NT_RISCV_VECTOR if we want to use
> > > ptrace as the entry point for this purpose. I am not very sure if it'd be
> > > too late to do though.
> > 
> >  No, how do you expect it to work with a core dump (that can be examined 
> > on a different system, or with a cross-debugger)?  You need to change the 
> > API I'm afraid; it's unusable anyway.  It's a pity the toolchain community 
> > wasn't consulted if you weren't sure how to design the interface.  Better 
> > yet it would have been to implement the GDB side before the kernel part 
> > has been committed.
> 
> Conor just reminded me that we may still have a chance to get it right
> since 6.5 has not been released yet. I will send a fix patch to address
> this issue once the discussion settle down. After looking into some
> code, I think it is possbile to steal the unused space in datap and
> change the uapi with something like this:
> 
> diff --git a/arch/riscv/include/uapi/asm/ptrace.h b/arch/riscv/include/uapi/asm/ptrace.h
> index e17c550986a6..ba6ddf4f9dc9 100644
> --- a/arch/riscv/include/uapi/asm/ptrace.h
> +++ b/arch/riscv/include/uapi/asm/ptrace.h
> @@ -97,14 +97,17 @@ struct __riscv_v_ext_state {
>  	unsigned long vl;
>  	unsigned long vtype;
>  	unsigned long vcsr;
> -	void *datap;
> +	union {
> +		void *datap;
> +		unsigned long vlenb;
> +	};
>  	/*
>  	 * In signal handler, datap will be set a correct user stack offset
>  	 * and vector registers will be copied to the address of datap
>  	 * pointer.
>  	 *
> -	 * In ptrace syscall, datap will be set to zero and the vector
> -	 * registers will be copied to the address right after this
> +	 * In ptrace syscall, the space for datap will be set to vlenb and the
> +	 * vector registers will be copied to the address right after this
>  	 * structure.
>  	 */
>  };
> 
> Now ptrace will have the knowlege of vlen to parse V rsgisters. And this
> will not cause any size change to the original data structure that is
> shared by both signal and ptrace because vlenb is XLEN, which has the
> same size as a pointer in both ilp32/lp64.

 Barring details such as field naming (perhaps `vregp' rather than opaque 
`datap'?), or whether we want to have a union embedded such as above or 
distinct UAPI data types for the two use cases I think your proposal for 
the updated contents makes sense to me, thanks.

  Maciej

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

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

* Re: [PATCH] RISC-V: support for vector register accesses via ptrace() in RISC-V Linux native
  2023-08-10 17:23             ` Andy Chiu
@ 2023-08-10 21:08               ` Palmer Dabbelt
  2023-08-10 21:21               ` Maciej W. Rozycki
  1 sibling, 0 replies; 10+ messages in thread
From: Palmer Dabbelt @ 2023-08-10 21:08 UTC (permalink / raw)
  To: andy.chiu
  Cc: macro, greg.savin, greentime.hu, oleg, Paul Walmsley, aou,
	linux-riscv, gdb-patches, andrew.burgess

On Thu, 10 Aug 2023 10:23:34 PDT (-0700), andy.chiu@sifive.com wrote:
> On Thu, Aug 10, 2023 at 9:55 PM Maciej W. Rozycki <macro@orcam.me.uk> wrote:
>>
>> On Thu, 10 Aug 2023, Maciej W. Rozycki wrote:
>>
>> > > Does it make sense to you if we encapsulate this with a hwprobe syscall?
>> > > e.g provide a hwprobe entry to get system's VLENB. We will have to
>> > > increase and rearrange the buffer for NT_RISCV_VECTOR if we want to use
>> > > ptrace as the entry point for this purpose. I am not very sure if it'd be
>> > > too late to do though.
>> >
>> >  No, how do you expect it to work with a core dump (that can be examined
>> > on a different system, or with a cross-debugger)?  You need to change the
>> > API I'm afraid; it's unusable anyway.  It's a pity the toolchain community
>> > wasn't consulted if you weren't sure how to design the interface.  Better
>> > yet it would have been to implement the GDB side before the kernel part
>> > has been committed.
>
> I just took some look into the code and here is what I came up with.
> Actually, you know VLENB in a core dump file. The size of
> NT_RISCV_VECTOR in a core dump file just equals sizeof(struct
> __riscv_v_ext_state), which is 40B, plus VLENB * 32. So, the debugger
> can actually calculate VLENB and resolve placement of V registers by
> subtracting 40 from the size of NT_RISCV_VECTOR in a core dump file.
>
> On the other hand, ptrace is not so lucky. The kernel will return the
> min of either user specified size or the maximum Vector size. It is
> still safe if we consider SMP with the same VLENB across cores though,
> which is an assumption made on Linux. We just need a way to get VLENB
> on the system.
>
>>
>>  NB since this stuff went in with v6.5-rc1 and v6.5 hasn't been released
>> you can still back out the problematic change as no one is expected to use
>> RC stuff in production.  Alternatively you can redefine NT_RISCV_VECTOR
>> for a corrected ABI, but I think it shouldn't be necessary.  You just need
>> to act quickly as I guess there may be 1-2 further v6.5 RCs only and you
>> have to get with that to Linus right away.  We can have a release or two
>> without NT_RISCV_VECTOR support for the otherwise included vector stuff,
>> it shouldn't be a big deal.  There just won't be support for the debug
>> API.

IMO that's the way to go: given that we're still finding breakagaes this 
late in the cycle it's likely we've got others.  Like Maciej said, we 
should have gotten the GDB stuff in along with the Linux stuff to find 
the problems.

So let's just remove the ptrace() and core dump support for vector, it's 
not been released so it's not stable uABI yet.  We'll just get it right 
before committing it, that can be as simple as just one more release.

>>
>>  CC-ing Linux ptrace/RISC-V maintainers now to bring their attention.
>>
>>   Maciej
>
> Thanks,
> Andy

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

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

* Re: [PATCH] RISC-V: support for vector register accesses via ptrace() in RISC-V Linux native
  2023-08-10 17:23             ` Andy Chiu
  2023-08-10 21:08               ` Palmer Dabbelt
@ 2023-08-10 21:21               ` Maciej W. Rozycki
  2023-08-11 11:28                 ` Andy Chiu
  1 sibling, 1 reply; 10+ messages in thread
From: Maciej W. Rozycki @ 2023-08-10 21:21 UTC (permalink / raw)
  To: Andy Chiu
  Cc: Greg Savin, Greentime Hu, Oleg Nesterov, Paul Walmsley,
	Palmer Dabbelt, Albert Ou, linux-riscv, gdb-patches,
	Andrew Burgess

On Fri, 11 Aug 2023, Andy Chiu wrote:

> > >  No, how do you expect it to work with a core dump (that can be examined
> > > on a different system, or with a cross-debugger)?  You need to change the
> > > API I'm afraid; it's unusable anyway.  It's a pity the toolchain community
> > > wasn't consulted if you weren't sure how to design the interface.  Better
> > > yet it would have been to implement the GDB side before the kernel part
> > > has been committed.
> 
> I just took some look into the code and here is what I came up with.
> Actually, you know VLENB in a core dump file. The size of
> NT_RISCV_VECTOR in a core dump file just equals sizeof(struct
> __riscv_v_ext_state), which is 40B, plus VLENB * 32. So, the debugger
> can actually calculate VLENB and resolve placement of V registers by
> subtracting 40 from the size of NT_RISCV_VECTOR in a core dump file.

 Fair enough, I didn't dive into Linux code deeply enough to figure out 
that the size of an NT_RISCV_VECTOR core file note is indeed dynamically 
calculated.  Most notes are of a fixed size, but we also have generic 
support for variable-size ones in GDB, so handling this case should be 
reasonably straightforward.

 OTOH VLENB is a program-visible register, so I think it will best be 
provided explicitly regardless rather than having to be reconstructed from 
the size of the note; I would find that awkward.

 NB I have been a bit concerned about the unusually huge allocation size 
of 256KiB+ for the register buffer required for ptrace(2), but I guess 
we'll have to live with it, because any solution that makes it dynamic 
would also complicate the interface.  At least we won't waste filesystem 
space for any extraneous allocation in core dumps.

  Maciej

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

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

* Re: [PATCH] RISC-V: support for vector register accesses via ptrace() in RISC-V Linux native
  2023-08-10 21:21               ` Maciej W. Rozycki
@ 2023-08-11 11:28                 ` Andy Chiu
  0 siblings, 0 replies; 10+ messages in thread
From: Andy Chiu @ 2023-08-11 11:28 UTC (permalink / raw)
  To: Maciej W. Rozycki
  Cc: Greg Savin, Greentime Hu, Oleg Nesterov, Paul Walmsley,
	Palmer Dabbelt, Albert Ou, linux-riscv, gdb-patches,
	Andrew Burgess

On Fri, Aug 11, 2023 at 5:21 AM Maciej W. Rozycki <macro@orcam.me.uk> wrote:
>
> On Fri, 11 Aug 2023, Andy Chiu wrote:
>
> > > >  No, how do you expect it to work with a core dump (that can be examined
> > > > on a different system, or with a cross-debugger)?  You need to change the
> > > > API I'm afraid; it's unusable anyway.  It's a pity the toolchain community
> > > > wasn't consulted if you weren't sure how to design the interface.  Better
> > > > yet it would have been to implement the GDB side before the kernel part
> > > > has been committed.
> >
> > I just took some look into the code and here is what I came up with.
> > Actually, you know VLENB in a core dump file. The size of
> > NT_RISCV_VECTOR in a core dump file just equals sizeof(struct
> > __riscv_v_ext_state), which is 40B, plus VLENB * 32. So, the debugger
> > can actually calculate VLENB and resolve placement of V registers by
> > subtracting 40 from the size of NT_RISCV_VECTOR in a core dump file.
>
>  Fair enough, I didn't dive into Linux code deeply enough to figure out
> that the size of an NT_RISCV_VECTOR core file note is indeed dynamically
> calculated.  Most notes are of a fixed size, but we also have generic
> support for variable-size ones in GDB, so handling this case should be
> reasonably straightforward.
>
>  OTOH VLENB is a program-visible register, so I think it will best be
> provided explicitly regardless rather than having to be reconstructed from
> the size of the note; I would find that awkward.

Agreed.

>
>  NB I have been a bit concerned about the unusually huge allocation size
> of 256KiB+ for the register buffer required for ptrace(2), but I guess
> we'll have to live with it, because any solution that makes it dynamic
> would also complicate the interface.  At least we won't waste filesystem
> space for any extraneous allocation in core dumps.

It is possible to mitigate this consideration with the proposed
solution[1], by calling the ptrace twice. First we make a ptrace call
to obtain VLENB in struct __riscv_v_ext_state by setting the argument
iov.len = sizeof(struct __riscv_v_ext_state). Then, we can allocate a
buffer based on the result of the previous ptrace to get the full
Vector registers dump.

>
>   Maciej

[1]: https://sourceware.org/pipermail/gdb-patches/2023-August/201507.html

Andy

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

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

end of thread, other threads:[~2023-08-11 11:28 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <20230803230110.904724-1-greg.savin@sifive.com>
     [not found] ` <alpine.DEB.2.21.2308091008500.25915@angie.orcam.me.uk>
     [not found]   ` <CADdv1FqjLPZ-eWOKPv0uZxF-u-SYjn0WJGr3KWW9H06-O0L35w@mail.gmail.com>
2023-08-09 23:09     ` [PATCH] RISC-V: support for vector register accesses via ptrace() in RISC-V Linux native Maciej W. Rozycki
2023-08-10 10:35       ` Andy Chiu
2023-08-10 11:40         ` Maciej W. Rozycki
2023-08-10 13:55           ` Maciej W. Rozycki
2023-08-10 17:23             ` Andy Chiu
2023-08-10 21:08               ` Palmer Dabbelt
2023-08-10 21:21               ` Maciej W. Rozycki
2023-08-11 11:28                 ` Andy Chiu
2023-08-10 14:05           ` Andy Chiu
2023-08-10 20:51             ` Maciej W. Rozycki

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox