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

[John Baldwin <jhb@FreeBSD.org>]

On 8/3/23 4:01 PM, Greg Savin via Gdb-patches wrote:
> This patch adds support for vector register accessibility (via
> $v0..$v31 syntax and also "info registers vector") to native Linux
> RISC-V configurations of gdb/gdbserver.  ptrace() of head of tree
> Linux kernel makes those registers available if kernel is built with
> the appropriate config flags.  I don't have an SoC implementing RISC-V
> cores capable of running Linux and implementing RISC-V vector
> extension, in order to test this patch.  I have tried this patch on a
> VCU118 FPGA-based board configured with a proprietary bitstream
> implementing RISC-V processor(s) with RISC-V vector extension, running
> a Linux kernel that is configured for RISC-V vector extension support.
> Also tried it on a configuration of QEMU that models RISC-V processor
> w/ RISC-V vector extension, running the same Linux kernel.
> 
> This patch is offered in case equivalent functionality isn't already
> sitting on a branch at https://sourceware.org/git/binutils-gdb.git.  I
> don't see anything equivalent on current master branch.
> 
> The baseline for this patch was commit 606d863236197cc2fbf74edf589cbaf35ea15801
> of master branch of https://sourceware.org/git/binutils-gdb.git
> 
> ---
>   gdb/arch/riscv.c             | 191 ++++++++++++++++++++++++++++++++-
>   gdb/nat/riscv-linux-tdesc.c  |  68 ++++++++++++
>   gdb/nat/riscv-linux-tdesc.h  |  27 +++++
>   gdb/riscv-linux-nat.c        | 200 +++++++++++++++++++++++++++++++++++
>   gdb/riscv-linux-tdep.c       | 132 +++++++++++++++++++++++
>   gdb/riscv-tdep.c             |  49 ++++++++-
>   gdb/riscv-tdep.h             |   5 +
>   gdbserver/linux-riscv-low.cc | 110 +++++++++++++++++++
>   8 files changed, 775 insertions(+), 7 deletions(-)
> 
> diff --git a/gdb/arch/riscv.c b/gdb/arch/riscv.c
> index 6f6fcb081e8..e8dd5994bb0 100644
> --- a/gdb/arch/riscv.c
> +++ b/gdb/arch/riscv.c
> @@ -26,12 +26,30 @@
>   #include "../features/riscv/64bit-fpu.c"
>   #include "../features/riscv/rv32e-xregs.c"
>   
> +#include "opcode/riscv-opc.h"
> +
>   #ifndef GDBSERVER
>   #define STATIC_IN_GDB static
>   #else
>   #define STATIC_IN_GDB
>   #endif
>   
> +#ifdef GDBSERVER
> +/* Work around issue where trying to include riscv-tdep.h (to get access to canonical RISCV_V0_REGNUM declaration
> +   from that header) is problamtic for gdbserver build */
> +#define RISCV_V0_REGNUM 4162
> +#else
> +#include "defs.h"
> +#include "riscv-tdep.h"
> +#endif

On other architectures the regnum constants are in arch/foo.h instead, e.g.
gdb/arch/aarch64.h.  You should probably move the *REGNUM constants to
gdb/arch/riscv.h instead of this workaround.

> diff --git a/gdb/nat/riscv-linux-tdesc.h b/gdb/nat/riscv-linux-tdesc.h
> index 8e8da410265..4da9af7844c 100644
> --- a/gdb/nat/riscv-linux-tdesc.h
> +++ b/gdb/nat/riscv-linux-tdesc.h
> @@ -20,9 +20,36 @@
>   #define NAT_RISCV_LINUX_TDESC_H
>   
>   #include "arch/riscv.h"
> +#include "asm/ptrace.h"
>   
>   /* Determine XLEN and FLEN for the LWP identified by TID, and return a
>      corresponding features object.  */
>   struct riscv_gdbarch_features riscv_linux_read_features (int tid);
>   
> +#ifndef NT_RISCV_VECTOR
> +#define RISCV_MAX_VLENB (8192)
> +#define NT_RISCV_VECTOR	0x900	/* RISC-V vector registers */
> +#endif

Should probably add NT_RISCV_VECTOR to include/elf/common.h instead so
it is always defined.  You will also then want to add it in other places
under binutils (e.g. so that readelf -n gives a suitable description,
grepping for something like NT_X86_XSTATE might be helpful to find other
places to update for a new note type).

> diff --git a/gdb/riscv-linux-nat.c b/gdb/riscv-linux-nat.c
> index 8be4a5ac3e5..6bc5c66f3cc 100644
> --- a/gdb/riscv-linux-nat.c
> +++ b/gdb/riscv-linux-nat.c
> @@ -125,6 +125,152 @@ supply_fpregset_regnum (struct regcache *regcache, const prfpregset_t *fpregs,
>       }
>   }
>   
> +
> +#define FOR_V0_TO_V31(idx, buf, regcache_method) \
> +  for ((idx) = RISCV_V0_REGNUM; (idx) <= RISCV_V31_REGNUM; (idx)++, (buf) += vlenb) \
> +    regcache->regcache_method ((idx), (buf))
> +
> +#define SINGLE_REGISTER_V0_TO_V31(regnum, buf, regcache_method) \
> +  (buf) = vregs->data + vlenb * ((regnum) - RISCV_V0_REGNUM);	\
> +  regcache->regcache_method ((regnum), (buf));
> +
> +#define ALL_VECTOR_REGS_OR_SPECIFIC_VECTOR_CSR(regnum_val, buf, field, regcache_method) \
> +  if (regnum == -1 || regnum == (regnum_val))	\
> +    { \
> +      (buf) = (gdb_byte*)&vregs->vstate.field;	     \
> +      regcache->regcache_method ((regnum_val), (buf));	\
> +    }
> +
> +
> +static void
> +supply_vregset_regnum (struct regcache *regcache,
> +		       const struct __riscv_vregs *vregs, int regnum)
> +{
> +  const gdb_byte *buf;
> +  int vlenb = register_size (regcache->arch (), RISCV_V0_REGNUM);
> +  int i;
> +
> +  if (regnum == -1)
> +    {
> +      buf = vregs->data;
> +      FOR_V0_TO_V31(i, buf, raw_supply);
> +    }
> +  else if (regnum >= RISCV_V0_REGNUM && regnum <= RISCV_V31_REGNUM)
> +    {
> +      SINGLE_REGISTER_V0_TO_V31(regnum, buf, raw_supply);
> +    }
> +
> +  if (regnum == -1 || regnum == RISCV_CSR_VSTART_REGNUM)
> +    {
> +      ALL_VECTOR_REGS_OR_SPECIFIC_VECTOR_CSR(RISCV_CSR_VSTART_REGNUM, buf, vstart, raw_supply);
> +    }
> +
> +  if (regnum == -1 || regnum == RISCV_CSR_VL_REGNUM)
> +    {
> +      ALL_VECTOR_REGS_OR_SPECIFIC_VECTOR_CSR(RISCV_CSR_VL_REGNUM, buf, vl, raw_supply);
> +    }
> +
> +  if (regnum == -1 || regnum == RISCV_CSR_VTYPE_REGNUM)
> +    {
> +      ALL_VECTOR_REGS_OR_SPECIFIC_VECTOR_CSR(RISCV_CSR_VTYPE_REGNUM, buf, vtype, raw_supply);
> +    }
> +
> +  if (regnum == -1 || regnum == RISCV_CSR_VCSR_REGNUM)
> +    {
> +      ALL_VECTOR_REGS_OR_SPECIFIC_VECTOR_CSR(RISCV_CSR_VCSR_REGNUM, buf, vcsr, raw_supply);
> +    }
> +
> +  if (regnum == -1 || regnum == RISCV_CSR_VLENB_REGNUM)
> +    {
> +      /* we already have a local copy above, use that (widened for XLEN padding) */
> +      uint64_t xlen_safe_vlenb = vlenb;
> +      buf = (gdb_byte *) & xlen_safe_vlenb;
> +      regcache->raw_supply (RISCV_CSR_VLENB_REGNUM, buf);
> +    }
> +
> +  if (regnum == -1 || regnum == RISCV_CSR_VXSAT_REGNUM)
> +    {
> +      /*  this CSR is not part of vregs->vstate literally, but we can infer a value from vcsr */
> +      uint64_t vxsat = ((vregs->vstate.vcsr >> VCSR_POS_VXSAT) & VCSR_MASK_VXSAT);
> +      buf = (gdb_byte *) & vxsat;
> +      regcache->raw_supply (RISCV_CSR_VXSAT_REGNUM, buf);
> +    }
> +
> +  if (regnum == -1 || regnum == RISCV_CSR_VXRM_REGNUM)
> +    {
> +      /*  this CSR is not part of vregs->vstate literally, but we can infer a value from vcsr */
> +      uint64_t vxrm = ((vregs->vstate.vcsr >> VCSR_POS_VXRM) & VCSR_MASK_VXRM);
> +      buf = (gdb_byte *) & vxrm;
> +      regcache->raw_supply (RISCV_CSR_VXRM_REGNUM, buf);
> +    }> +}
> +
> +static void
> +fill_vregset (const struct regcache *regcache, struct __riscv_vregs *vregs,
> +	      int regnum)
> +{
> +  gdb_byte *buf;
> +  int vlenb = register_size (regcache->arch (), RISCV_V0_REGNUM);
> +  int i;
> +
> +  if (regnum == -1)
> +    {
> +      buf = vregs->data;
> +      FOR_V0_TO_V31(i, buf, raw_collect);
> +    }
> +  else if (regnum >= RISCV_V0_REGNUM && regnum <= RISCV_V31_REGNUM)
> +    {
> +      SINGLE_REGISTER_V0_TO_V31(regnum, buf, raw_collect);
> +    }
> +
> +  if (regnum == -1 || regnum == RISCV_CSR_VSTART_REGNUM)
> +    {
> +      ALL_VECTOR_REGS_OR_SPECIFIC_VECTOR_CSR(RISCV_CSR_VSTART_REGNUM, buf, vstart, raw_collect);
> +    }
> +
> +  if (regnum == -1 || regnum == RISCV_CSR_VL_REGNUM)
> +    {
> +      ALL_VECTOR_REGS_OR_SPECIFIC_VECTOR_CSR(RISCV_CSR_VL_REGNUM, buf, vl, raw_collect);
> +    }
> +
> +  if (regnum == -1 || regnum == RISCV_CSR_VTYPE_REGNUM)
> +    {
> +      ALL_VECTOR_REGS_OR_SPECIFIC_VECTOR_CSR(RISCV_CSR_VTYPE_REGNUM, buf, vtype, raw_collect);
> +    }
> +
> +  if (regnum == -1 || regnum == RISCV_CSR_VCSR_REGNUM || regnum == RISCV_CSR_VXSAT_REGNUM
> +      || regnum == RISCV_CSR_VXRM_REGNUM)
> +    {
> +      uint64_t vxsat_from_regcache;
> +      uint64_t vxrm_from_regcache;
> +
> +      ALL_VECTOR_REGS_OR_SPECIFIC_VECTOR_CSR(RISCV_CSR_VCSR_REGNUM, buf, vcsr, raw_collect);
> +
> +      if (regnum == RISCV_CSR_VXSAT_REGNUM)
> +	{
> +	  /* Overwrite VCSR with the VXSAT bit here */
> +	  buf = (gdb_byte*)&vxsat_from_regcache;
> +	  regcache->raw_collect (RISCV_CSR_VXSAT_REGNUM, buf);
> +	  vregs->vstate.vcsr &= ~((uint64_t)VCSR_MASK_VXSAT << VCSR_POS_VXSAT);
> +	  vregs->vstate.vcsr |= ((vxsat_from_regcache & VCSR_MASK_VXSAT) << VCSR_POS_VXSAT);
> +	}
> +
> +      if (regnum == RISCV_CSR_VXRM_REGNUM)
> +	{
> +	  /* Overwrite VCSR with the VXRM bit here */
> +	  buf = (gdb_byte*)&vxrm_from_regcache;
> +	  regcache->raw_collect (RISCV_CSR_VXRM_REGNUM, buf);
> +	  vregs->vstate.vcsr &= ~((uint64_t)VCSR_MASK_VXRM << VCSR_POS_VXRM);	
> +	  vregs->vstate.vcsr |= ((vxrm_from_regcache & VCSR_MASK_VXRM) << VCSR_POS_VXRM);
> +	}
> +
> +    }
> +
> +  /* VLENB register is not writable, so that's why nothing is collected here for that register */
> +
> +}
> +
> +

This might be a bit shorter to write if you use a regcache_map.  It can use a size
of 0 for the V registers which will use register_size () of those registers to determine
the size (if the register_size for a given gdbarch is always the same  as vlenb).  Something
like:

static const regcache_map_entry riscv_linux_vregmap[] =
{
     { 32, RISCV_V0_REGNUM, 0 },
     { 1, RISCV_CSR_XXX, 8 },
     ...
};

Also, it seems like the sub-registers of VCSR would be better off as psuedo
registers.  Arguably FRM and FFLAGS should be as well vs the rather unusual
hack used in riscv_supply_regset currently that's kind of a half-way pseudo
register.

-- 
John Baldwin