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

John Baldwin
2023-08-04T00:22:10Z

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