[PATCH] RISC-V: support for vector register accesses via ptrace() in RISC-V Linux native

[Andy Chiu]

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