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

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

[Greg Savin]

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
+
+static int
+create_feature_riscv_vector_from_features (struct target_desc *result,
+					   long regnum,
+					   const struct riscv_gdbarch_features
+					   features);
+
+
 /* See arch/riscv.h.  */
 
 STATIC_IN_GDB target_desc_up
@@ -84,15 +102,180 @@ riscv_create_target_description (const struct riscv_gdbarch_features features)
   else if (features.flen == 8)
     regnum = create_feature_riscv_64bit_fpu (tdesc.get (), regnum);
 
-  /* Currently GDB only supports vector features coming from remote
-     targets.  We don't support creating vector features on native targets
-     (yet).  */
   if (features.vlen != 0)
-    error (_("unable to create vector feature"));
+    regnum =
+      create_feature_riscv_vector_from_features (tdesc.get (),
+						 RISCV_V0_REGNUM, features);
 
   return tdesc;
 }
 
+
+
+/* Usually, these target_desc instances are static for an architecture, and expressable
+   in XML format, but this is a special case where length of a RISC-V vector register
+   is not architecturally fixed to a constant (the maximuim width is a defined constant,
+   but it's nice to tailor a target description the actual VLENB) */
+static int
+create_feature_riscv_vector_from_features (struct target_desc *result,
+					   long regnum,
+					   const struct riscv_gdbarch_features
+					   features)
+{
+  struct tdesc_feature *feature;
+  unsigned long bitsize;
+
+  feature = tdesc_create_feature (result, "org.gnu.gdb.riscv.vector");
+  tdesc_type *element_type;
+
+  /* if VLENB is present (which we know it is present if execution reaches this function),
+     then we know by definition that it is at least 4 bytes wide */
+  
+  element_type = tdesc_named_type (feature, "uint8");
+  tdesc_create_vector (feature, "bytes", element_type, features.vlen);
+
+  element_type = tdesc_named_type (feature, "uint16");
+  tdesc_create_vector (feature, "shorts", element_type, features.vlen / 2);
+
+  element_type = tdesc_named_type (feature, "uint32");
+  tdesc_create_vector (feature, "words", element_type, features.vlen / 4);
+
+  /* Need VLENB value checks for element chunks larger than 4 bytes */
+  
+  if (features.vlen >= 8)
+    {
+      element_type = tdesc_named_type (feature, "uint64");
+      tdesc_create_vector (feature, "longs", element_type, features.vlen / 8);
+    }
+
+  /* QEMU and OpenOCD include the quads width in their target descriptions, so we're
+     following that precedent, even if it's not particularly useful in practice, yet */
+  
+  if (features.vlen >= 16)
+    {
+      element_type = tdesc_named_type (feature, "uint128");
+      tdesc_create_vector (feature, "quads", element_type,
+			   features.vlen / 16);
+    }
+
+  tdesc_type_with_fields *type_with_fields;
+  type_with_fields = tdesc_create_union (feature, "riscv_vector");
+  tdesc_type *field_type;
+
+  if (features.vlen >= 16)
+    {
+      field_type = tdesc_named_type (feature, "quads");
+      tdesc_add_field (type_with_fields, "q", field_type);
+    }
+  if (features.vlen >= 8)
+    {
+      field_type = tdesc_named_type (feature, "longs");
+      tdesc_add_field (type_with_fields, "l", field_type);
+    }
+
+  /* Again, we know vlenb is >= 4, so no if guards needed for words/shorts/bytes */
+  
+  field_type = tdesc_named_type (feature, "words");
+  tdesc_add_field (type_with_fields, "w", field_type);
+  
+  field_type = tdesc_named_type (feature, "shorts");
+  tdesc_add_field (type_with_fields, "s", field_type);
+  
+  field_type = tdesc_named_type (feature, "bytes");
+  tdesc_add_field (type_with_fields, "b", field_type);
+
+  /* Using magic numbers for regnum parameter of these CSRs.  Magic numbers aren't ever ideal,
+     but didn't find a clear alternative that compiles successfully in both the gdb and gdbserver
+     build steps.  A mitigating factor is that these numbers
+     should be stable because they are based on constituent values that should also be stable:
+     RISCV_FIRST_CSR_REGNUM (a fixed constant) added to the respective CSR numbers from RISC-V     
+     specifications.  Also there is some precedent for magic numbers; the *.xml files in features/riscv/
+     use magic numbers to refer to floating point CSRs.
+
+     Also, the init_target_desc function in gdbserver expects all these registers to be ordered
+     in increasing order of "GDB internals" register number, with CSRs before vN registers and in relative numeric order
+     ascending.  DWARF register numbers don't seem to follow that pattern, and it seems to be necessary to use the GDB
+     regnums in order for things to work on both native gdb and gdbserver.
+   */
+  tdesc_create_reg (feature, "vstart", 73, 1, NULL, features.xlen * 8, "int");
+  tdesc_create_reg (feature, "vxsat", 74, 1, NULL, features.xlen * 8, "int");
+  tdesc_create_reg (feature, "vxrm", 75, 1, NULL, features.xlen * 8, "int");  
+  tdesc_create_reg (feature, "vcsr", 80, 1, NULL, features.xlen * 8, "int");
+  tdesc_create_reg (feature, "vl", 3169, 1, NULL, features.xlen * 8, "int");
+  tdesc_create_reg (feature, "vtype", 3170, 1, NULL, features.xlen * 8, "int");
+  tdesc_create_reg (feature, "vlenb", 3171, 1, NULL, features.xlen * 8, "int");
+
+  bitsize = features.vlen * 8;
+  tdesc_create_reg (feature, "v0", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v1", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v2", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v3", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v4", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v5", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v6", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v7", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v8", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v9", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v10", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v11", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v12", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v13", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v14", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v15", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v16", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v17", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v18", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v19", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v20", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v21", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v22", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v23", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v24", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v25", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v26", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v27", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v28", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v29", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v30", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v31", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+
+
+  return regnum;
+}
+
+
 #ifndef GDBSERVER
 
 /* Wrapper used by std::unordered_map to generate hash for feature set.  */
diff --git a/gdb/nat/riscv-linux-tdesc.c b/gdb/nat/riscv-linux-tdesc.c
index d676233cc31..51d89108575 100644
--- a/gdb/nat/riscv-linux-tdesc.c
+++ b/gdb/nat/riscv-linux-tdesc.c
@@ -23,14 +23,18 @@
 #include "elf/common.h"
 #include "nat/gdb_ptrace.h"
 #include "nat/riscv-linux-tdesc.h"
+#include "gdbsupport/gdb_setjmp.h"
 
 #include <sys/uio.h>
+#include <signal.h>
 
 /* Work around glibc header breakage causing ELF_NFPREG not to be usable.  */
 #ifndef NFPREG
 # define NFPREG 33
 #endif
 
+static unsigned long safe_read_vlenb ();
+
 /* See nat/riscv-linux-tdesc.h.  */
 
 struct riscv_gdbarch_features
@@ -79,5 +83,69 @@ riscv_linux_read_features (int tid)
       break;
     }
 
+  features.vlen = safe_read_vlenb ();
+
   return features;
 }
+
+static SIGJMP_BUF sigill_guard_jmp_buf;
+
+static void
+sigill_guard (int sig)
+{
+  /* this will gets us back to caller deeper in the call stack, with an indication that
+     an illegal instruction condition was encountered */
+  SIGLONGJMP (sigill_guard_jmp_buf, -1);
+
+  /* control won't get here */
+}
+
+
+
+static unsigned long
+safe_read_vlenb ()
+{
+  /* Surrounding the attempt here to read VLENB CSR to have a signal handler set up
+     to trap illegal instruction condition (SIGILL), and if a trap happens during this call,
+     get control back within this function and return 0 in that case.
+   */
+  unsigned long vlenb = 0;
+  struct sigaction our_action = { 0 };
+  struct sigaction original_action;
+  int sysresult;
+
+
+  our_action.sa_handler = sigill_guard;
+
+  sysresult = sigaction (SIGILL, &our_action, &original_action);
+  if (sysresult != 0)
+    {
+      perror
+	("Error installing temporary SIGILL handler in safe_read_vlenb()");
+    }
+
+  if (SIGSETJMP (sigill_guard_jmp_buf, 1) == 0)
+    {
+    asm ("csrr %0, vlenb":"=r" (vlenb));
+    }
+  else
+    {
+      /* Must've generated an illegal instruction condition; we'll figure this means
+         no vector unit is present */
+      vlenb = 0;
+    }
+
+
+  if (sysresult == 0)
+    {
+      /* re-install former handler */
+      sysresult = sigaction (SIGILL, &original_action, NULL);
+      if (sysresult != 0)
+	{
+	  perror
+	    ("Error re-installing original SIGILL handler in safe_read_vlenb()");
+	}
+
+    }
+  return vlenb;
+}
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
+
+/* Some branches and/or commits of linux kernel named this "struct __riscv_v_state",
+   and later it was changed to "struct __riscv_v_ext_state",
+   so using a macro to stand-in for that struct type to make it easier to modify
+   in a single place, if compiling against one of those older Linux kernel commits */
+#ifndef RISCV_VECTOR_STATE_T
+#define RISCV_VECTOR_STATE_T struct __riscv_v_ext_state
+#endif
+
+/* Struct for use in ptrace() calls for vector CSRs/registers */
+struct __riscv_vregs
+{
+  RISCV_VECTOR_STATE_T vstate;
+  gdb_byte data[RISCV_MAX_VLENB * 32];	/* data will arrive packed, VLENB bytes per element, not necessarily RISCV_MAX_VLENB bytes per element */
+};
+
+#define VCSR_MASK_VXSAT 0x1
+#define VCSR_POS_VXSAT 0
+#define VCSR_MASK_VXRM 0x3
+#define VCSR_POS_VXRM 1
+
+
 #endif /* NAT_RISCV_LINUX_TDESC_H */
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 */
+
+}
+
+
 /* Copy all floating point registers from regset FPREGS into REGCACHE.  */
 
 void
@@ -252,6 +398,31 @@ riscv_linux_nat_target::fetch_registers (struct regcache *regcache, int regnum)
 	supply_fpregset_regnum (regcache, &regs, regnum);
     }
 
+  /* if Linux kernel was not configured to support RISC-V vectors, then
+     the ptrace call will return -1, and we just won't get vector registers,
+     but in that case it wouldn't be an error that needs user attention.
+   */
+  if ((regnum >= RISCV_V0_REGNUM && regnum <= RISCV_V31_REGNUM)
+      || (regnum == RISCV_CSR_VSTART_REGNUM)
+      || (regnum == RISCV_CSR_VL_REGNUM)
+      || (regnum == RISCV_CSR_VTYPE_REGNUM)
+      || (regnum == RISCV_CSR_VCSR_REGNUM)
+      || (regnum == RISCV_CSR_VLENB_REGNUM)
+      || (regnum == RISCV_CSR_VXSAT_REGNUM)
+      || (regnum == RISCV_CSR_VXRM_REGNUM)
+      || (regnum == -1))
+    {
+      struct iovec iov;
+      struct __riscv_vregs vregs;
+
+      iov.iov_base = &vregs;
+      iov.iov_len = sizeof (vregs);
+
+      if (ptrace (PTRACE_GETREGSET, tid, NT_RISCV_VECTOR,
+		  (PTRACE_TYPE_ARG3) & iov) == 0)
+	supply_vregset_regnum (regcache, &vregs, regnum);
+    }
+
   if ((regnum == RISCV_CSR_MISA_REGNUM)
       || (regnum == -1))
     {
@@ -321,6 +492,35 @@ riscv_linux_nat_target::store_registers (struct regcache *regcache, int regnum)
 	}
     }
 
+  /* VLENB isn't writable, so we'll skip considering that one, if it's being
+     specified alone */
+  if ((regnum >= RISCV_V0_REGNUM && regnum <= RISCV_V31_REGNUM)
+      || (regnum == RISCV_CSR_VSTART_REGNUM)
+      || (regnum == RISCV_CSR_VL_REGNUM)
+      || (regnum == RISCV_CSR_VTYPE_REGNUM)
+      || (regnum == RISCV_CSR_VCSR_REGNUM)
+      || (regnum == RISCV_CSR_VXSAT_REGNUM)
+      || (regnum == RISCV_CSR_VXRM_REGNUM)
+      || (regnum == -1))
+    {
+      struct iovec iov;
+      struct __riscv_vregs vregs;
+
+      iov.iov_base = &vregs;
+      iov.iov_len = sizeof (vregs);
+
+      if (ptrace (PTRACE_GETREGSET, tid, NT_RISCV_VECTOR,
+		  (PTRACE_TYPE_ARG3) & iov) == 0)
+	{
+	  fill_vregset (regcache, &vregs, regnum);
+
+	  if (ptrace (PTRACE_SETREGSET, tid, NT_RISCV_VECTOR,
+		      (PTRACE_TYPE_ARG3) & iov) == -1)
+	    perror_with_name (_("Couldn't set vector registers"));
+	}
+    }
+
+
   /* Access to CSRs has potential security issues, don't support them for
      now.  */
 }
diff --git a/gdb/riscv-linux-tdep.c b/gdb/riscv-linux-tdep.c
index 292d7a4ef7c..e2b5e5cf4b4 100644
--- a/gdb/riscv-linux-tdep.c
+++ b/gdb/riscv-linux-tdep.c
@@ -32,6 +32,10 @@
 
 #define RISCV_NR_rt_sigreturn 139
 
+/* Magic number written to the head.magic field of struct __sc_riscv_v_state that kernel
+   places in the reserved area of struct sigcontext.  Comes from <asm/sigcontext.h> */
+#define RVV_MAGIC 0x53465457
+
 /* Define the general register mapping.  The kernel puts the PC at offset 0,
    gdb puts it at offset 32.  Register x0 is always 0 and can be ignored.
    Registers x1 to x31 are in the same place.  */
@@ -120,8 +124,122 @@ static const struct tramp_frame riscv_linux_sigframe = {
      mcontext_t uc_mcontext;
    }; */
 
+
+
+/* riscv_linux_vector_sigframe_header_check() returns an answer to the question
+   "is there a RISC-V Vector header at this memory location"? */
+
+static bool
+riscv_linux_vector_sigframe_header_check (frame_info_ptr this_frame,
+					  int vlen, int xlen,
+					  CORE_ADDR regs_base)
+{
+  uint32_t rvv_magic;
+  uint32_t rvv_size;
+  bool info_good = false;
+
+  /* If vector information is available, then we should see this structure at this address:
+     struct __riscv_ctx_hdr {
+     __u32 magic;  (RVV_MAGIC).
+     __u32 size;   (size of struct __sc_riscv_v_state + vector register data size (32*VLENB))
+     } head;
+   */
+
+  rvv_magic =
+    get_frame_memory_unsigned (this_frame, regs_base, sizeof (rvv_magic));
+  regs_base += sizeof (rvv_magic);
+  rvv_size =
+    get_frame_memory_unsigned (this_frame, regs_base, sizeof (rvv_magic));
+  regs_base += sizeof (rvv_size);
+
+
+  info_good = (rvv_magic == RVV_MAGIC);
+  if (!info_good)
+    {
+      /* Not an error, because kernels can be configured without CONFIG_VECTOR, but worth noting if frame debug
+         setting is turned on */
+      if (frame_debug)
+	frame_debug_printf
+	  ("Did not find RISC-V vector information in ucontext (kernel not built with CONFIG_VECTOR?)");
+
+      return false;
+    }
+
+  if (frame_debug)
+    {
+      uint32_t expected_rvv_size;
+
+      frame_debug_printf
+	("Located RISC-V vector information in signal frame ucontext (info size %u)",
+	 rvv_size);
+
+      /* sanity check the reported size; should be sizeof(uint32_t) + sizeof(uint32_t) + 5 * XLENB + 32 * vlen */
+      expected_rvv_size = sizeof (uint32_t) /* magic */  +
+	sizeof (uint32_t) /* size */  +
+	5 * xlen /* vstart, vl, vtype, vcsr, and datap */  +
+	32 * vlen;		/* v0..v31 values */
+
+      if (rvv_size != expected_rvv_size)
+	{
+	  /* It doesn't seem like this should be a hard error, but it'd be good to make it visible if frame debug
+	     setting is turned on */
+	  frame_debug_printf
+	    ("Size in RISC-V vector information header in ucontext differs from the expected %u",
+	     expected_rvv_size);
+	}
+    }
+
+  return info_good;
+}
+
+static CORE_ADDR
+riscv_linux_sigframe_vector_init (frame_info_ptr this_frame,
+				  struct trad_frame_cache *this_cache,
+				  CORE_ADDR regs_base, int xlen, int vlen)
+{
+  int vfieldidx;		/* index of "unsigned long" members in __riscv_v_ext_state */
+  CORE_ADDR p_datap;
+  CORE_ADDR datap;		/* dereferenced value of void *datap that points to v0..v31 */
+
+  /* vstart, vl, vtype, vcsr, and datap are XLEN sized fields (unsigned long) from this point */
+  vfieldidx = 0;
+  trad_frame_set_reg_addr (this_cache, RISCV_CSR_VSTART_REGNUM,
+			   regs_base + (vfieldidx * xlen));
+  vfieldidx++;
+  trad_frame_set_reg_addr (this_cache, RISCV_CSR_VL_REGNUM,
+			   regs_base + (vfieldidx * xlen));
+
+  vfieldidx++;
+  trad_frame_set_reg_addr (this_cache, RISCV_CSR_VTYPE_REGNUM,
+			   regs_base + (vfieldidx * xlen));
+
+  vfieldidx++;
+  trad_frame_set_reg_addr (this_cache, RISCV_CSR_VCSR_REGNUM,
+			   regs_base + (vfieldidx * xlen));
+
+  /* for the datap member, there is one level of memory indirection to get the address of
+     the block of values for v0..v31 */
+  vfieldidx++;
+  p_datap = regs_base + (vfieldidx * xlen);
+  datap = get_frame_memory_unsigned (this_frame, p_datap, xlen);
+  regs_base = datap;
+  for (int i = 0; i < 32; i++)
+    {
+      trad_frame_set_reg_addr (this_cache, RISCV_V0_REGNUM + i,
+			       regs_base + (i * vlen));
+    }
+  regs_base += 32 * vlen;
+
+  return regs_base;
+}
+
+
 #define SIGFRAME_SIGINFO_SIZE		128
 #define UCONTEXT_MCONTEXT_OFFSET	176
+#define MCONTEXT_VECTOR_OFFSET		784	/* offset of struct mcontext's __reserved field,
+						   which is where the struct __sc_riscv_v_state is overlaid */
+#define RISCV_CONTEXT_HEADER_SIZE	8	/* size of struct __riscv_ctx_hdr {__u32 magic;  __u32 size; } */
+
 
 static void
 riscv_linux_sigframe_init (const struct tramp_frame *self,
@@ -132,6 +250,7 @@ riscv_linux_sigframe_init (const struct tramp_frame *self,
   struct gdbarch *gdbarch = get_frame_arch (this_frame);
   int xlen = riscv_isa_xlen (gdbarch);
   int flen = riscv_isa_flen (gdbarch);
+  int vlen = riscv_isa_vlen (gdbarch);
   CORE_ADDR frame_sp = get_frame_sp (this_frame);
   CORE_ADDR mcontext_base;
   CORE_ADDR regs_base;
@@ -155,6 +274,19 @@ riscv_linux_sigframe_init (const struct tramp_frame *self,
   regs_base += 32 * flen;
   trad_frame_set_reg_addr (this_cache, RISCV_CSR_FCSR_REGNUM, regs_base);
 
+  /* Handle the vector registers, if present. */
+  if (vlen > 0)
+    {
+      regs_base = mcontext_base + MCONTEXT_VECTOR_OFFSET;
+      if (riscv_linux_vector_sigframe_header_check
+	  (this_frame, vlen, xlen, regs_base))
+	{
+	  regs_base += RISCV_CONTEXT_HEADER_SIZE;	/* advance past the header */
+	  riscv_linux_sigframe_vector_init (this_frame, this_cache, regs_base,
+					    xlen, vlen);
+	}
+    }
+
   /* Choice of the bottom of the sigframe is somewhat arbitrary.  */
   trad_frame_set_id (this_cache, frame_id_build (frame_sp, func));
 }
diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
index ae18eb64452..8714b750017 100644
--- a/gdb/riscv-tdep.c
+++ b/gdb/riscv-tdep.c
@@ -47,6 +47,7 @@
 #include "remote.h"
 #include "target-descriptions.h"
 #include "dwarf2/frame.h"
+#include "dwarf2/expr.h"
 #include "user-regs.h"
 #include "valprint.h"
 #include "gdbsupport/common-defs.h"
@@ -650,6 +651,14 @@ struct riscv_vector_feature : public riscv_register_feature
       { RISCV_V0_REGNUM + 29, { "v29" } },
       { RISCV_V0_REGNUM + 30, { "v30" } },
       { RISCV_V0_REGNUM + 31, { "v31" } },
+      /* vector CSRs */
+      { RISCV_CSR_VSTART_REGNUM, { "vstart" } },
+      { RISCV_CSR_VXSAT_REGNUM, { "vxsat" } },
+      { RISCV_CSR_VXRM_REGNUM, { "vxrm" } },
+      { RISCV_CSR_VL_REGNUM, { "vl" } },
+      { RISCV_CSR_VTYPE_REGNUM, { "vtype" } },
+      { RISCV_CSR_VCSR_REGNUM, { "vcsr" } },
+      { RISCV_CSR_VLENB_REGNUM, { "vlenb" } },
     };
   }
 
@@ -681,10 +690,16 @@ struct riscv_vector_feature : public riscv_register_feature
 	return true;
       }
 
-    /* Check all of the vector registers are present.  */
+    /* Check all of the vector registers are present.  We also
+       check that the vector CSRs are present too, though if these
+       are missing this is not fatal.  */
     for (const auto &reg : m_registers)
       {
-	if (!reg.check (tdesc_data, feature_vector, true, aliases))
+	bool found = reg.check (tdesc_data, feature_vector, true, aliases);
+	
+	bool is_ctrl_reg_p = !(reg.regnum >= RISCV_V0_REGNUM && reg.regnum <= RISCV_V31_REGNUM);
+
+	if (!found && !is_ctrl_reg_p)
 	  return false;
       }
 
@@ -694,6 +709,12 @@ struct riscv_vector_feature : public riscv_register_feature
     int vector_bitsize = -1;
     for (const auto &reg : m_registers)
       {
+
+	bool is_ctrl_reg_p = !(reg.regnum >= RISCV_V0_REGNUM && reg.regnum <= RISCV_V31_REGNUM);	
+
+	if (is_ctrl_reg_p)
+	  continue;
+
 	int reg_bitsize = -1;
 	for (const char *name : reg.names)
 	  {
@@ -804,6 +825,16 @@ riscv_abi_embedded (struct gdbarch *gdbarch)
   return tdep->abi_features.embedded;
 }
 
+/* See riscv-tdep.h.  */
+
+int
+riscv_isa_vlen (struct gdbarch *gdbarch)
+{
+  riscv_gdbarch_tdep *tdep = gdbarch_tdep<riscv_gdbarch_tdep> (gdbarch);
+  return tdep->isa_features.vlen;
+}
+
+
 /* Return true if the target for GDBARCH has floating point hardware.  */
 
 static bool
@@ -1454,7 +1485,19 @@ riscv_register_reggroup_p (struct gdbarch  *gdbarch, int regnum,
       return 0;
     }
   else if (reggroup == vector_reggroup)
-    return (regnum >= RISCV_V0_REGNUM && regnum <= RISCV_V31_REGNUM);
+    {
+      if (regnum >= RISCV_V0_REGNUM && regnum <= RISCV_V31_REGNUM)
+	return 1;
+      if (regnum == RISCV_CSR_VSTART_REGNUM
+	  || regnum == RISCV_CSR_VXSAT_REGNUM
+	  || regnum == RISCV_CSR_VXRM_REGNUM
+	  || regnum == RISCV_CSR_VL_REGNUM
+	  || regnum == RISCV_CSR_VTYPE_REGNUM
+	  || regnum == RISCV_CSR_VCSR_REGNUM
+	  || regnum == RISCV_CSR_VLENB_REGNUM)
+	return 1;
+      return 0;
+    }
   else
     return 0;
 }
diff --git a/gdb/riscv-tdep.h b/gdb/riscv-tdep.h
index 4c3afb08e07..b183c58c7da 100644
--- a/gdb/riscv-tdep.h
+++ b/gdb/riscv-tdep.h
@@ -150,6 +150,11 @@ extern int riscv_abi_flen (struct gdbarch *gdbarch);
    argument registers.  */
 extern bool riscv_abi_embedded (struct gdbarch *gdbarch);
 
+/* Return the width in bytes of the hardware vector registers for
+   GDBARCH.  If this architecture has no vector registers, then
+   return 0.  */
+extern int riscv_isa_vlen (struct gdbarch *gdbarch);
+
 /* Single step based on where the current instruction will take us.  */
 extern std::vector<CORE_ADDR> riscv_software_single_step
   (struct regcache *regcache);
diff --git a/gdbserver/linux-riscv-low.cc b/gdbserver/linux-riscv-low.cc
index 129bc3b138b..169fa988c06 100644
--- a/gdbserver/linux-riscv-low.cc
+++ b/gdbserver/linux-riscv-low.cc
@@ -158,6 +158,113 @@ riscv_store_fpregset (struct regcache *regcache, const void *buf)
   supply_register_by_name (regcache, "fcsr", regbuf);
 }
 
+/* Collect vector registers from REGCACHE into BUF.  */
+
+static void
+riscv_fill_vregset (struct regcache *regcache, void *buf)
+{
+  const struct target_desc *tdesc = regcache->tdesc;
+  int regno = find_regno (tdesc, "v0");
+  int vlenb = register_size (regcache->tdesc, regno);
+  uint64_t u64_vlenb = vlenb;	/* pad to max XLEN for buffer conversion */
+  uint64_t u64_vxsat = 0;
+  uint64_t u64_vxrm = 0;
+  uint64_t u64_vcsr = 0;
+  gdb_byte *regbuf;
+  int i;
+
+  /* Since vxsat and equivalent bits in vcsr are aliases (and same for vxrm), we have a dilemma.
+     For this gdb -> gdbserver topology, if the aliased pairs have values that disagree, then
+     which value should take precedence?  We don't know which alias was most
+     recently assigned.  We're just getting a block of register values including vxsat, vxrm,
+     and vcsr.  We have to impose some kind of rule for predictable resolution to resolve any inconsistency.
+     For now, let's say that vxsat and vxrm take precedence, and those values will be applied to the
+     corresponding fields in vcsr.  Reconcile these 3 interdependent registers now:
+  */
+  regbuf = (gdb_byte *) & u64_vcsr;
+  collect_register_by_name (regcache, "vcsr", regbuf);
+  regbuf = (gdb_byte *) & u64_vxsat;
+  collect_register_by_name (regcache, "vxsat", regbuf);
+  regbuf = (gdb_byte *) & u64_vxrm;
+  collect_register_by_name (regcache, "vxrm", regbuf);
+  
+  u64_vcsr &= ~((uint64_t)VCSR_MASK_VXSAT << VCSR_POS_VXSAT);
+  u64_vcsr |= ((u64_vxsat & VCSR_MASK_VXSAT) << VCSR_POS_VXSAT);
+  u64_vcsr &= ~((uint64_t)VCSR_MASK_VXRM << VCSR_POS_VXRM);	  
+  u64_vcsr |= ((u64_vxrm & VCSR_MASK_VXRM) << VCSR_POS_VXRM);
+
+  /* Replace the original vcsr value with the "cooked" value */
+  regbuf = (gdb_byte *) & u64_vcsr;  
+  supply_register_by_name (regcache, "vcsr", regbuf);
+
+  /* Now stage the ptrace buffer (it'll receive the cooked vcsr value) */
+
+  regbuf = (gdb_byte *) buf + offsetof (struct __riscv_vregs, vstate.vstart);
+  collect_register_by_name (regcache, "vstart", regbuf);
+  regbuf = (gdb_byte *) buf + offsetof (struct __riscv_vregs, vstate.vl);
+  collect_register_by_name (regcache, "vl", regbuf);
+  regbuf = (gdb_byte *) buf + offsetof (struct __riscv_vregs, vstate.vtype);
+  collect_register_by_name (regcache, "vtype", regbuf);
+  regbuf = (gdb_byte *) buf + offsetof (struct __riscv_vregs, vstate.vcsr);
+  collect_register_by_name (regcache, "vcsr", regbuf);
+  regbuf = (gdb_byte *) & u64_vlenb;
+  collect_register_by_name (regcache, "vlenb", regbuf);
+
+
+  regbuf = (gdb_byte *) buf + offsetof (struct __riscv_vregs, data);
+  for (i = 0; i < 32; i++, regbuf += vlenb)
+    collect_register (regcache, regno + i, regbuf);
+}
+
+/* Supply vector registers from BUF into REGCACHE.  */
+
+static void
+riscv_store_vregset (struct regcache *regcache, const void *buf)
+{
+  const struct target_desc *tdesc = regcache->tdesc;
+  int regno = find_regno (tdesc, "v0");
+  int vlenb = register_size (regcache->tdesc, regno);
+  uint64_t u64_vlenb = vlenb;	/* pad to max XLEN for buffer conversion */
+  uint64_t vcsr;
+  uint64_t vxsat;
+  uint64_t vxrm;  
+  const gdb_byte *regbuf;
+  int i;
+
+  regbuf =
+    (const gdb_byte *) buf + offsetof (struct __riscv_vregs, vstate.vstart);
+  supply_register_by_name (regcache, "vstart", regbuf);
+  regbuf =
+    (const gdb_byte *) buf + offsetof (struct __riscv_vregs, vstate.vl);
+  supply_register_by_name (regcache, "vl", regbuf);
+  regbuf =
+    (const gdb_byte *) buf + offsetof (struct __riscv_vregs, vstate.vtype);
+  supply_register_by_name (regcache, "vtype", regbuf);
+  regbuf =
+    (const gdb_byte *) buf + offsetof (struct __riscv_vregs, vstate.vcsr);
+  supply_register_by_name (regcache, "vcsr", regbuf);
+  /* also store off a non-byte-wise copy of vcsr, to derive values for vxsat and vxrm */
+  vcsr = *(uint64_t*)regbuf;
+  /* vlenb isn't part of vstate, but we have already inferred its value by running code on this
+     hart, and we're assuming homogeneous VLENB if it's an SMP system */
+  regbuf = (gdb_byte *) & u64_vlenb;
+  supply_register_by_name (regcache, "vlenb", regbuf);
+
+  /* vxsat and vxrm, are not part of vstate, so we have to extract from VCSR
+     value */
+  vxsat = ((vcsr >> VCSR_POS_VXSAT) & VCSR_MASK_VXSAT);  
+  regbuf = (gdb_byte *) &vxsat;
+  supply_register_by_name (regcache, "vxsat", regbuf);
+  vxrm = ((vcsr >> VCSR_POS_VXRM) & VCSR_MASK_VXRM);  
+  regbuf = (gdb_byte *) &vxrm;
+  supply_register_by_name (regcache, "vxrm", regbuf);
+
+  /* v0..v31 */
+  regbuf = (const gdb_byte *) buf + offsetof (struct __riscv_vregs, data);
+  for (i = 0; i < 32; i++, regbuf += vlenb)
+    supply_register (regcache, regno + i, regbuf);
+}
+
 /* RISC-V/Linux regsets.  FPRs are optional and come in different sizes,
    so define multiple regsets for them marking them all as OPTIONAL_REGS
    rather than FP_REGS, so that "regsets_fetch_inferior_registers" picks
@@ -175,6 +282,9 @@ static struct regset_info riscv_regsets[] = {
   { PTRACE_GETREGSET, PTRACE_SETREGSET, NT_FPREGSET,
     sizeof (struct __riscv_mc_f_ext_state), OPTIONAL_REGS,
     riscv_fill_fpregset, riscv_store_fpregset },
+  { PTRACE_GETREGSET, PTRACE_SETREGSET, NT_RISCV_VECTOR,
+    sizeof (struct __riscv_vregs), OPTIONAL_REGS,
+    riscv_fill_vregset, riscv_store_vregset },
   NULL_REGSET
 };
 
-- 
2.25.1

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

[John Baldwin]

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

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

[Greg Savin]

A v2 re-spin of the original patch.

Now using regcache_map_entry to encode the vector register buffer format.

Regarding moving NT_RISCV_VECTOR to binutils common.h, I'll be submitting that in
a separate patch.  Evidently the Linux kernel headers are using 0x900 to
refer to NT_RISCV_VECTOR, but in binutils/common.h, 0x900 is being used
to refer to NT_RISCV_CSR.  I'll submit a patch that proposes re-numbering
NT_RISCV_CSR to 0x901, and that declares NT_RISCV_VECTOR with value 0x900,
to match the Linux kernel headers.

Regarding representing VXRM and VXSAT as pseudo-registers, I see the rationale
for that in the native debug via ptrace() case, because VXRM and VXSAT are not
included as part of the vector register transfer payload in ptrace().  However,
in the architecture of RISCV vector extension, VXRM and VXSAT do in fact exist
as CSRs in their own right, and for non-native debug (e.g. JTAG-based bare metal
via OpenOCD's gdbstub) those registers may well be accessed directly rather than
via VCSR.  Because VXRM and VXSAT are architecturally spec'ed as CSRs in their
own right, I'm wary of modeling them as pseudo-registers because I'd be concerned
about unintended consequences for non-native RISC-V debug configurations.


---
 gdb/arch/riscv.c             | 191 ++++++++++++++++++++++++++++++++++-
 gdb/nat/riscv-linux-tdesc.c  |  68 +++++++++++++
 gdb/nat/riscv-linux-tdesc.h  |  27 +++++
 gdb/riscv-linux-nat.c        | 162 +++++++++++++++++++++++++++++
 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, 737 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
+
+static int
+create_feature_riscv_vector_from_features (struct target_desc *result,
+					   long regnum,
+					   const struct riscv_gdbarch_features
+					   features);
+
+
 /* See arch/riscv.h.  */
 
 STATIC_IN_GDB target_desc_up
@@ -84,15 +102,180 @@ riscv_create_target_description (const struct riscv_gdbarch_features features)
   else if (features.flen == 8)
     regnum = create_feature_riscv_64bit_fpu (tdesc.get (), regnum);
 
-  /* Currently GDB only supports vector features coming from remote
-     targets.  We don't support creating vector features on native targets
-     (yet).  */
   if (features.vlen != 0)
-    error (_("unable to create vector feature"));
+    regnum =
+      create_feature_riscv_vector_from_features (tdesc.get (),
+						 RISCV_V0_REGNUM, features);
 
   return tdesc;
 }
 
+
+
+/* Usually, these target_desc instances are static for an architecture, and expressable
+   in XML format, but this is a special case where length of a RISC-V vector register
+   is not architecturally fixed to a constant (the maximuim width is a defined constant,
+   but it's nice to tailor a target description the actual VLENB) */
+static int
+create_feature_riscv_vector_from_features (struct target_desc *result,
+					   long regnum,
+					   const struct riscv_gdbarch_features
+					   features)
+{
+  struct tdesc_feature *feature;
+  unsigned long bitsize;
+
+  feature = tdesc_create_feature (result, "org.gnu.gdb.riscv.vector");
+  tdesc_type *element_type;
+
+  /* if VLENB is present (which we know it is present if execution reaches this function),
+     then we know by definition that it is at least 4 bytes wide */
+  
+  element_type = tdesc_named_type (feature, "uint8");
+  tdesc_create_vector (feature, "bytes", element_type, features.vlen);
+
+  element_type = tdesc_named_type (feature, "uint16");
+  tdesc_create_vector (feature, "shorts", element_type, features.vlen / 2);
+
+  element_type = tdesc_named_type (feature, "uint32");
+  tdesc_create_vector (feature, "words", element_type, features.vlen / 4);
+
+  /* Need VLENB value checks for element chunks larger than 4 bytes */
+  
+  if (features.vlen >= 8)
+    {
+      element_type = tdesc_named_type (feature, "uint64");
+      tdesc_create_vector (feature, "longs", element_type, features.vlen / 8);
+    }
+
+  /* QEMU and OpenOCD include the quads width in their target descriptions, so we're
+     following that precedent, even if it's not particularly useful in practice, yet */
+  
+  if (features.vlen >= 16)
+    {
+      element_type = tdesc_named_type (feature, "uint128");
+      tdesc_create_vector (feature, "quads", element_type,
+			   features.vlen / 16);
+    }
+
+  tdesc_type_with_fields *type_with_fields;
+  type_with_fields = tdesc_create_union (feature, "riscv_vector");
+  tdesc_type *field_type;
+
+  if (features.vlen >= 16)
+    {
+      field_type = tdesc_named_type (feature, "quads");
+      tdesc_add_field (type_with_fields, "q", field_type);
+    }
+  if (features.vlen >= 8)
+    {
+      field_type = tdesc_named_type (feature, "longs");
+      tdesc_add_field (type_with_fields, "l", field_type);
+    }
+
+  /* Again, we know vlenb is >= 4, so no if guards needed for words/shorts/bytes */
+  
+  field_type = tdesc_named_type (feature, "words");
+  tdesc_add_field (type_with_fields, "w", field_type);
+  
+  field_type = tdesc_named_type (feature, "shorts");
+  tdesc_add_field (type_with_fields, "s", field_type);
+  
+  field_type = tdesc_named_type (feature, "bytes");
+  tdesc_add_field (type_with_fields, "b", field_type);
+
+  /* Using magic numbers for regnum parameter of these CSRs.  Magic numbers aren't ever ideal,
+     but didn't find a clear alternative that compiles successfully in both the gdb and gdbserver
+     build steps.  A mitigating factor is that these numbers
+     should be stable because they are based on constituent values that should also be stable:
+     RISCV_FIRST_CSR_REGNUM (a fixed constant) added to the respective CSR numbers from RISC-V     
+     specifications.  Also there is some precedent for magic numbers; the *.xml files in features/riscv/
+     use magic numbers to refer to floating point CSRs.
+
+     Also, the init_target_desc function in gdbserver expects all these registers to be ordered
+     in increasing order of "GDB internals" register number, with CSRs before vN registers and in relative numeric order
+     ascending.  DWARF register numbers don't seem to follow that pattern, and it seems to be necessary to use the GDB
+     regnums in order for things to work on both native gdb and gdbserver.
+   */
+  tdesc_create_reg (feature, "vstart", 73, 1, NULL, features.xlen * 8, "int");
+  tdesc_create_reg (feature, "vxsat", 74, 1, NULL, features.xlen * 8, "int");
+  tdesc_create_reg (feature, "vxrm", 75, 1, NULL, features.xlen * 8, "int");  
+  tdesc_create_reg (feature, "vcsr", 80, 1, NULL, features.xlen * 8, "int");
+  tdesc_create_reg (feature, "vl", 3169, 1, NULL, features.xlen * 8, "int");
+  tdesc_create_reg (feature, "vtype", 3170, 1, NULL, features.xlen * 8, "int");
+  tdesc_create_reg (feature, "vlenb", 3171, 1, NULL, features.xlen * 8, "int");
+
+  bitsize = features.vlen * 8;
+  tdesc_create_reg (feature, "v0", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v1", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v2", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v3", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v4", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v5", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v6", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v7", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v8", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v9", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v10", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v11", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v12", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v13", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v14", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v15", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v16", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v17", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v18", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v19", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v20", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v21", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v22", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v23", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v24", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v25", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v26", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v27", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v28", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v29", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v30", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+  tdesc_create_reg (feature, "v31", regnum++, 1, NULL, bitsize,
+		    "riscv_vector");
+
+
+  return regnum;
+}
+
+
 #ifndef GDBSERVER
 
 /* Wrapper used by std::unordered_map to generate hash for feature set.  */
diff --git a/gdb/nat/riscv-linux-tdesc.c b/gdb/nat/riscv-linux-tdesc.c
index d676233cc31..51d89108575 100644
--- a/gdb/nat/riscv-linux-tdesc.c
+++ b/gdb/nat/riscv-linux-tdesc.c
@@ -23,14 +23,18 @@
 #include "elf/common.h"
 #include "nat/gdb_ptrace.h"
 #include "nat/riscv-linux-tdesc.h"
+#include "gdbsupport/gdb_setjmp.h"
 
 #include <sys/uio.h>
+#include <signal.h>
 
 /* Work around glibc header breakage causing ELF_NFPREG not to be usable.  */
 #ifndef NFPREG
 # define NFPREG 33
 #endif
 
+static unsigned long safe_read_vlenb ();
+
 /* See nat/riscv-linux-tdesc.h.  */
 
 struct riscv_gdbarch_features
@@ -79,5 +83,69 @@ riscv_linux_read_features (int tid)
       break;
     }
 
+  features.vlen = safe_read_vlenb ();
+
   return features;
 }
+
+static SIGJMP_BUF sigill_guard_jmp_buf;
+
+static void
+sigill_guard (int sig)
+{
+  /* this will gets us back to caller deeper in the call stack, with an indication that
+     an illegal instruction condition was encountered */
+  SIGLONGJMP (sigill_guard_jmp_buf, -1);
+
+  /* control won't get here */
+}
+
+
+
+static unsigned long
+safe_read_vlenb ()
+{
+  /* Surrounding the attempt here to read VLENB CSR to have a signal handler set up
+     to trap illegal instruction condition (SIGILL), and if a trap happens during this call,
+     get control back within this function and return 0 in that case.
+   */
+  unsigned long vlenb = 0;
+  struct sigaction our_action = { 0 };
+  struct sigaction original_action;
+  int sysresult;
+
+
+  our_action.sa_handler = sigill_guard;
+
+  sysresult = sigaction (SIGILL, &our_action, &original_action);
+  if (sysresult != 0)
+    {
+      perror
+	("Error installing temporary SIGILL handler in safe_read_vlenb()");
+    }
+
+  if (SIGSETJMP (sigill_guard_jmp_buf, 1) == 0)
+    {
+    asm ("csrr %0, vlenb":"=r" (vlenb));
+    }
+  else
+    {
+      /* Must've generated an illegal instruction condition; we'll figure this means
+         no vector unit is present */
+      vlenb = 0;
+    }
+
+
+  if (sysresult == 0)
+    {
+      /* re-install former handler */
+      sysresult = sigaction (SIGILL, &original_action, NULL);
+      if (sysresult != 0)
+	{
+	  perror
+	    ("Error re-installing original SIGILL handler in safe_read_vlenb()");
+	}
+
+    }
+  return vlenb;
+}
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
+
+/* Some branches and/or commits of linux kernel named this "struct __riscv_v_state",
+   and later it was changed to "struct __riscv_v_ext_state",
+   so using a macro to stand-in for that struct type to make it easier to modify
+   in a single place, if compiling against one of those older Linux kernel commits */
+#ifndef RISCV_VECTOR_STATE_T
+#define RISCV_VECTOR_STATE_T struct __riscv_v_ext_state
+#endif
+
+/* Struct for use in ptrace() calls for vector CSRs/registers */
+struct __riscv_vregs
+{
+  RISCV_VECTOR_STATE_T vstate;
+  gdb_byte data[RISCV_MAX_VLENB * 32];	/* data will arrive packed, VLENB bytes per element, not necessarily RISCV_MAX_VLENB bytes per element */
+};
+
+#define VCSR_MASK_VXSAT 0x1
+#define VCSR_POS_VXSAT 0
+#define VCSR_MASK_VXRM 0x3
+#define VCSR_POS_VXRM 1
+
+
 #endif /* NAT_RISCV_LINUX_TDESC_H */
diff --git a/gdb/riscv-linux-nat.c b/gdb/riscv-linux-nat.c
index 8be4a5ac3e5..38fdd0ac5e0 100644
--- a/gdb/riscv-linux-nat.c
+++ b/gdb/riscv-linux-nat.c
@@ -22,6 +22,7 @@
 #include "linux-nat.h"
 #include "riscv-tdep.h"
 #include "inferior.h"
+#include "regset.h"
 
 #include "elf/common.h"
 
@@ -125,6 +126,113 @@ supply_fpregset_regnum (struct regcache *regcache, const prfpregset_t *fpregs,
     }
 }
 
+#define MEMBER_SIZE(type, member) sizeof(((type *)0)->member)
+
+static const regcache_map_entry riscv_linux_vregmap[] =
+{
+  { 1, RISCV_CSR_VSTART_REGNUM, MEMBER_SIZE(struct __riscv_vregs, vstate.vstart) },
+  { 1, RISCV_CSR_VL_REGNUM, MEMBER_SIZE(struct __riscv_vregs, vstate.vl) },
+  { 1, RISCV_CSR_VTYPE_REGNUM, MEMBER_SIZE(struct __riscv_vregs, vstate.vtype) },
+  { 1, RISCV_CSR_VCSR_REGNUM, MEMBER_SIZE(struct __riscv_vregs, vstate.vcsr) },
+  /* struct __riscv_vregs member "datap" is a pointer that doesn't correspond
+     to a register value.  In the context of ptrace(), member is always zero,
+     with V0..V31 values inline after that.  So, skipping datap */
+  { 1, REGCACHE_MAP_SKIP, MEMBER_SIZE(struct __riscv_vregs, vstate.datap) },
+  /* Here's V0..V31.  Specifying 0 as size leads to a call to register_size()
+     for size determination */
+  { 32, RISCV_V0_REGNUM, 0 },
+  { 0 },  /* count==0 represents termination of entries */
+};
+
+/* Define the vector register regset.  */
+
+static const struct regset riscv_linux_vregset =
+{
+  riscv_linux_vregmap,
+  regcache_supply_regset /* Other RISC-V regsets use riscv_supply_regset here; not sure that'd be correct for this case */,
+  regcache_collect_regset
+};
+
+
+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);
+
+  regcache_supply_regset (&riscv_linux_vregset, regcache, regnum, vregs, sizeof(*vregs));  
+
+  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;
+
+  regcache_collect_regset (&riscv_linux_vregset, regcache, regnum, vregs, sizeof(*vregs));    
+
+  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;      
+
+      if ( ! (regnum == -1 || regnum == RISCV_CSR_VCSR_REGNUM) )
+	{
+	  // we don't already have the VCSR value, from the earlier regcache_collect_regset call, so let's get it now
+	  regcache_collect_regset (&riscv_linux_vregset, regcache, RISCV_CSR_VCSR_REGNUM, vregs, sizeof(*vregs));    	  
+	}
+
+      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 */
+
+}
+
+
 /* Copy all floating point registers from regset FPREGS into REGCACHE.  */
 
 void
@@ -252,6 +360,31 @@ riscv_linux_nat_target::fetch_registers (struct regcache *regcache, int regnum)
 	supply_fpregset_regnum (regcache, &regs, regnum);
     }
 
+  /* if Linux kernel was not configured to support RISC-V vectors, then
+     the ptrace call will return -1, and we just won't get vector registers,
+     but in that case it wouldn't be an error that needs user attention.
+   */
+  if ((regnum >= RISCV_V0_REGNUM && regnum <= RISCV_V31_REGNUM)
+      || (regnum == RISCV_CSR_VSTART_REGNUM)
+      || (regnum == RISCV_CSR_VL_REGNUM)
+      || (regnum == RISCV_CSR_VTYPE_REGNUM)
+      || (regnum == RISCV_CSR_VCSR_REGNUM)
+      || (regnum == RISCV_CSR_VLENB_REGNUM)
+      || (regnum == RISCV_CSR_VXSAT_REGNUM)
+      || (regnum == RISCV_CSR_VXRM_REGNUM)
+      || (regnum == -1))
+    {
+      struct iovec iov;
+      struct __riscv_vregs vregs;
+
+      iov.iov_base = &vregs;
+      iov.iov_len = sizeof (vregs);
+
+      if (ptrace (PTRACE_GETREGSET, tid, NT_RISCV_VECTOR,
+		  (PTRACE_TYPE_ARG3) & iov) == 0)
+	supply_vregset_regnum (regcache, &vregs, regnum);
+    }
+
   if ((regnum == RISCV_CSR_MISA_REGNUM)
       || (regnum == -1))
     {
@@ -321,6 +454,35 @@ riscv_linux_nat_target::store_registers (struct regcache *regcache, int regnum)
 	}
     }
 
+  /* VLENB isn't writable, so we'll skip considering that one, if it's being
+     specified alone */
+  if ((regnum >= RISCV_V0_REGNUM && regnum <= RISCV_V31_REGNUM)
+      || (regnum == RISCV_CSR_VSTART_REGNUM)
+      || (regnum == RISCV_CSR_VL_REGNUM)
+      || (regnum == RISCV_CSR_VTYPE_REGNUM)
+      || (regnum == RISCV_CSR_VCSR_REGNUM)
+      || (regnum == RISCV_CSR_VXSAT_REGNUM)
+      || (regnum == RISCV_CSR_VXRM_REGNUM)
+      || (regnum == -1))
+    {
+      struct iovec iov;
+      struct __riscv_vregs vregs;
+
+      iov.iov_base = &vregs;
+      iov.iov_len = sizeof (vregs);
+
+      if (ptrace (PTRACE_GETREGSET, tid, NT_RISCV_VECTOR,
+		  (PTRACE_TYPE_ARG3) & iov) == 0)
+	{
+	  fill_vregset (regcache, &vregs, regnum);
+
+	  if (ptrace (PTRACE_SETREGSET, tid, NT_RISCV_VECTOR,
+		      (PTRACE_TYPE_ARG3) & iov) == -1)
+	    perror_with_name (_("Couldn't set vector registers"));
+	}
+    }
+
+
   /* Access to CSRs has potential security issues, don't support them for
      now.  */
 }
diff --git a/gdb/riscv-linux-tdep.c b/gdb/riscv-linux-tdep.c
index 292d7a4ef7c..e2b5e5cf4b4 100644
--- a/gdb/riscv-linux-tdep.c
+++ b/gdb/riscv-linux-tdep.c
@@ -32,6 +32,10 @@
 
 #define RISCV_NR_rt_sigreturn 139
 
+/* Magic number written to the head.magic field of struct __sc_riscv_v_state that kernel
+   places in the reserved area of struct sigcontext.  Comes from <asm/sigcontext.h> */
+#define RVV_MAGIC 0x53465457
+
 /* Define the general register mapping.  The kernel puts the PC at offset 0,
    gdb puts it at offset 32.  Register x0 is always 0 and can be ignored.
    Registers x1 to x31 are in the same place.  */
@@ -120,8 +124,122 @@ static const struct tramp_frame riscv_linux_sigframe = {
      mcontext_t uc_mcontext;
    }; */
 
+
+
+/* riscv_linux_vector_sigframe_header_check() returns an answer to the question
+   "is there a RISC-V Vector header at this memory location"? */
+
+static bool
+riscv_linux_vector_sigframe_header_check (frame_info_ptr this_frame,
+					  int vlen, int xlen,
+					  CORE_ADDR regs_base)
+{
+  uint32_t rvv_magic;
+  uint32_t rvv_size;
+  bool info_good = false;
+
+  /* If vector information is available, then we should see this structure at this address:
+     struct __riscv_ctx_hdr {
+     __u32 magic;  (RVV_MAGIC).
+     __u32 size;   (size of struct __sc_riscv_v_state + vector register data size (32*VLENB))
+     } head;
+   */
+
+  rvv_magic =
+    get_frame_memory_unsigned (this_frame, regs_base, sizeof (rvv_magic));
+  regs_base += sizeof (rvv_magic);
+  rvv_size =
+    get_frame_memory_unsigned (this_frame, regs_base, sizeof (rvv_magic));
+  regs_base += sizeof (rvv_size);
+
+
+  info_good = (rvv_magic == RVV_MAGIC);
+  if (!info_good)
+    {
+      /* Not an error, because kernels can be configured without CONFIG_VECTOR, but worth noting if frame debug
+         setting is turned on */
+      if (frame_debug)
+	frame_debug_printf
+	  ("Did not find RISC-V vector information in ucontext (kernel not built with CONFIG_VECTOR?)");
+
+      return false;
+    }
+
+  if (frame_debug)
+    {
+      uint32_t expected_rvv_size;
+
+      frame_debug_printf
+	("Located RISC-V vector information in signal frame ucontext (info size %u)",
+	 rvv_size);
+
+      /* sanity check the reported size; should be sizeof(uint32_t) + sizeof(uint32_t) + 5 * XLENB + 32 * vlen */
+      expected_rvv_size = sizeof (uint32_t) /* magic */  +
+	sizeof (uint32_t) /* size */  +
+	5 * xlen /* vstart, vl, vtype, vcsr, and datap */  +
+	32 * vlen;		/* v0..v31 values */
+
+      if (rvv_size != expected_rvv_size)
+	{
+	  /* It doesn't seem like this should be a hard error, but it'd be good to make it visible if frame debug
+	     setting is turned on */
+	  frame_debug_printf
+	    ("Size in RISC-V vector information header in ucontext differs from the expected %u",
+	     expected_rvv_size);
+	}
+    }
+
+  return info_good;
+}
+
+static CORE_ADDR
+riscv_linux_sigframe_vector_init (frame_info_ptr this_frame,
+				  struct trad_frame_cache *this_cache,
+				  CORE_ADDR regs_base, int xlen, int vlen)
+{
+  int vfieldidx;		/* index of "unsigned long" members in __riscv_v_ext_state */
+  CORE_ADDR p_datap;
+  CORE_ADDR datap;		/* dereferenced value of void *datap that points to v0..v31 */
+
+  /* vstart, vl, vtype, vcsr, and datap are XLEN sized fields (unsigned long) from this point */
+  vfieldidx = 0;
+  trad_frame_set_reg_addr (this_cache, RISCV_CSR_VSTART_REGNUM,
+			   regs_base + (vfieldidx * xlen));
+  vfieldidx++;
+  trad_frame_set_reg_addr (this_cache, RISCV_CSR_VL_REGNUM,
+			   regs_base + (vfieldidx * xlen));
+
+  vfieldidx++;
+  trad_frame_set_reg_addr (this_cache, RISCV_CSR_VTYPE_REGNUM,
+			   regs_base + (vfieldidx * xlen));
+
+  vfieldidx++;
+  trad_frame_set_reg_addr (this_cache, RISCV_CSR_VCSR_REGNUM,
+			   regs_base + (vfieldidx * xlen));
+
+  /* for the datap member, there is one level of memory indirection to get the address of
+     the block of values for v0..v31 */
+  vfieldidx++;
+  p_datap = regs_base + (vfieldidx * xlen);
+  datap = get_frame_memory_unsigned (this_frame, p_datap, xlen);
+  regs_base = datap;
+  for (int i = 0; i < 32; i++)
+    {
+      trad_frame_set_reg_addr (this_cache, RISCV_V0_REGNUM + i,
+			       regs_base + (i * vlen));
+    }
+  regs_base += 32 * vlen;
+
+  return regs_base;
+}
+
+
 #define SIGFRAME_SIGINFO_SIZE		128
 #define UCONTEXT_MCONTEXT_OFFSET	176
+#define MCONTEXT_VECTOR_OFFSET		784	/* offset of struct mcontext's __reserved field,
+						   which is where the struct __sc_riscv_v_state is overlaid */
+#define RISCV_CONTEXT_HEADER_SIZE	8	/* size of struct __riscv_ctx_hdr {__u32 magic;  __u32 size; } */
+
 
 static void
 riscv_linux_sigframe_init (const struct tramp_frame *self,
@@ -132,6 +250,7 @@ riscv_linux_sigframe_init (const struct tramp_frame *self,
   struct gdbarch *gdbarch = get_frame_arch (this_frame);
   int xlen = riscv_isa_xlen (gdbarch);
   int flen = riscv_isa_flen (gdbarch);
+  int vlen = riscv_isa_vlen (gdbarch);
   CORE_ADDR frame_sp = get_frame_sp (this_frame);
   CORE_ADDR mcontext_base;
   CORE_ADDR regs_base;
@@ -155,6 +274,19 @@ riscv_linux_sigframe_init (const struct tramp_frame *self,
   regs_base += 32 * flen;
   trad_frame_set_reg_addr (this_cache, RISCV_CSR_FCSR_REGNUM, regs_base);
 
+  /* Handle the vector registers, if present. */
+  if (vlen > 0)
+    {
+      regs_base = mcontext_base + MCONTEXT_VECTOR_OFFSET;
+      if (riscv_linux_vector_sigframe_header_check
+	  (this_frame, vlen, xlen, regs_base))
+	{
+	  regs_base += RISCV_CONTEXT_HEADER_SIZE;	/* advance past the header */
+	  riscv_linux_sigframe_vector_init (this_frame, this_cache, regs_base,
+					    xlen, vlen);
+	}
+    }
+
   /* Choice of the bottom of the sigframe is somewhat arbitrary.  */
   trad_frame_set_id (this_cache, frame_id_build (frame_sp, func));
 }
diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
index ae18eb64452..8714b750017 100644
--- a/gdb/riscv-tdep.c
+++ b/gdb/riscv-tdep.c
@@ -47,6 +47,7 @@
 #include "remote.h"
 #include "target-descriptions.h"
 #include "dwarf2/frame.h"
+#include "dwarf2/expr.h"
 #include "user-regs.h"
 #include "valprint.h"
 #include "gdbsupport/common-defs.h"
@@ -650,6 +651,14 @@ struct riscv_vector_feature : public riscv_register_feature
       { RISCV_V0_REGNUM + 29, { "v29" } },
       { RISCV_V0_REGNUM + 30, { "v30" } },
       { RISCV_V0_REGNUM + 31, { "v31" } },
+      /* vector CSRs */
+      { RISCV_CSR_VSTART_REGNUM, { "vstart" } },
+      { RISCV_CSR_VXSAT_REGNUM, { "vxsat" } },
+      { RISCV_CSR_VXRM_REGNUM, { "vxrm" } },
+      { RISCV_CSR_VL_REGNUM, { "vl" } },
+      { RISCV_CSR_VTYPE_REGNUM, { "vtype" } },
+      { RISCV_CSR_VCSR_REGNUM, { "vcsr" } },
+      { RISCV_CSR_VLENB_REGNUM, { "vlenb" } },
     };
   }
 
@@ -681,10 +690,16 @@ struct riscv_vector_feature : public riscv_register_feature
 	return true;
       }
 
-    /* Check all of the vector registers are present.  */
+    /* Check all of the vector registers are present.  We also
+       check that the vector CSRs are present too, though if these
+       are missing this is not fatal.  */
     for (const auto &reg : m_registers)
       {
-	if (!reg.check (tdesc_data, feature_vector, true, aliases))
+	bool found = reg.check (tdesc_data, feature_vector, true, aliases);
+	
+	bool is_ctrl_reg_p = !(reg.regnum >= RISCV_V0_REGNUM && reg.regnum <= RISCV_V31_REGNUM);
+
+	if (!found && !is_ctrl_reg_p)
 	  return false;
       }
 
@@ -694,6 +709,12 @@ struct riscv_vector_feature : public riscv_register_feature
     int vector_bitsize = -1;
     for (const auto &reg : m_registers)
       {
+
+	bool is_ctrl_reg_p = !(reg.regnum >= RISCV_V0_REGNUM && reg.regnum <= RISCV_V31_REGNUM);	
+
+	if (is_ctrl_reg_p)
+	  continue;
+
 	int reg_bitsize = -1;
 	for (const char *name : reg.names)
 	  {
@@ -804,6 +825,16 @@ riscv_abi_embedded (struct gdbarch *gdbarch)
   return tdep->abi_features.embedded;
 }
 
+/* See riscv-tdep.h.  */
+
+int
+riscv_isa_vlen (struct gdbarch *gdbarch)
+{
+  riscv_gdbarch_tdep *tdep = gdbarch_tdep<riscv_gdbarch_tdep> (gdbarch);
+  return tdep->isa_features.vlen;
+}
+
+
 /* Return true if the target for GDBARCH has floating point hardware.  */
 
 static bool
@@ -1454,7 +1485,19 @@ riscv_register_reggroup_p (struct gdbarch  *gdbarch, int regnum,
       return 0;
     }
   else if (reggroup == vector_reggroup)
-    return (regnum >= RISCV_V0_REGNUM && regnum <= RISCV_V31_REGNUM);
+    {
+      if (regnum >= RISCV_V0_REGNUM && regnum <= RISCV_V31_REGNUM)
+	return 1;
+      if (regnum == RISCV_CSR_VSTART_REGNUM
+	  || regnum == RISCV_CSR_VXSAT_REGNUM
+	  || regnum == RISCV_CSR_VXRM_REGNUM
+	  || regnum == RISCV_CSR_VL_REGNUM
+	  || regnum == RISCV_CSR_VTYPE_REGNUM
+	  || regnum == RISCV_CSR_VCSR_REGNUM
+	  || regnum == RISCV_CSR_VLENB_REGNUM)
+	return 1;
+      return 0;
+    }
   else
     return 0;
 }
diff --git a/gdb/riscv-tdep.h b/gdb/riscv-tdep.h
index 4c3afb08e07..b183c58c7da 100644
--- a/gdb/riscv-tdep.h
+++ b/gdb/riscv-tdep.h
@@ -150,6 +150,11 @@ extern int riscv_abi_flen (struct gdbarch *gdbarch);
    argument registers.  */
 extern bool riscv_abi_embedded (struct gdbarch *gdbarch);
 
+/* Return the width in bytes of the hardware vector registers for
+   GDBARCH.  If this architecture has no vector registers, then
+   return 0.  */
+extern int riscv_isa_vlen (struct gdbarch *gdbarch);
+
 /* Single step based on where the current instruction will take us.  */
 extern std::vector<CORE_ADDR> riscv_software_single_step
   (struct regcache *regcache);
diff --git a/gdbserver/linux-riscv-low.cc b/gdbserver/linux-riscv-low.cc
index 129bc3b138b..169fa988c06 100644
--- a/gdbserver/linux-riscv-low.cc
+++ b/gdbserver/linux-riscv-low.cc
@@ -158,6 +158,113 @@ riscv_store_fpregset (struct regcache *regcache, const void *buf)
   supply_register_by_name (regcache, "fcsr", regbuf);
 }
 
+/* Collect vector registers from REGCACHE into BUF.  */
+
+static void
+riscv_fill_vregset (struct regcache *regcache, void *buf)
+{
+  const struct target_desc *tdesc = regcache->tdesc;
+  int regno = find_regno (tdesc, "v0");
+  int vlenb = register_size (regcache->tdesc, regno);
+  uint64_t u64_vlenb = vlenb;	/* pad to max XLEN for buffer conversion */
+  uint64_t u64_vxsat = 0;
+  uint64_t u64_vxrm = 0;
+  uint64_t u64_vcsr = 0;
+  gdb_byte *regbuf;
+  int i;
+
+  /* Since vxsat and equivalent bits in vcsr are aliases (and same for vxrm), we have a dilemma.
+     For this gdb -> gdbserver topology, if the aliased pairs have values that disagree, then
+     which value should take precedence?  We don't know which alias was most
+     recently assigned.  We're just getting a block of register values including vxsat, vxrm,
+     and vcsr.  We have to impose some kind of rule for predictable resolution to resolve any inconsistency.
+     For now, let's say that vxsat and vxrm take precedence, and those values will be applied to the
+     corresponding fields in vcsr.  Reconcile these 3 interdependent registers now:
+  */
+  regbuf = (gdb_byte *) & u64_vcsr;
+  collect_register_by_name (regcache, "vcsr", regbuf);
+  regbuf = (gdb_byte *) & u64_vxsat;
+  collect_register_by_name (regcache, "vxsat", regbuf);
+  regbuf = (gdb_byte *) & u64_vxrm;
+  collect_register_by_name (regcache, "vxrm", regbuf);
+  
+  u64_vcsr &= ~((uint64_t)VCSR_MASK_VXSAT << VCSR_POS_VXSAT);
+  u64_vcsr |= ((u64_vxsat & VCSR_MASK_VXSAT) << VCSR_POS_VXSAT);
+  u64_vcsr &= ~((uint64_t)VCSR_MASK_VXRM << VCSR_POS_VXRM);	  
+  u64_vcsr |= ((u64_vxrm & VCSR_MASK_VXRM) << VCSR_POS_VXRM);
+
+  /* Replace the original vcsr value with the "cooked" value */
+  regbuf = (gdb_byte *) & u64_vcsr;  
+  supply_register_by_name (regcache, "vcsr", regbuf);
+
+  /* Now stage the ptrace buffer (it'll receive the cooked vcsr value) */
+
+  regbuf = (gdb_byte *) buf + offsetof (struct __riscv_vregs, vstate.vstart);
+  collect_register_by_name (regcache, "vstart", regbuf);
+  regbuf = (gdb_byte *) buf + offsetof (struct __riscv_vregs, vstate.vl);
+  collect_register_by_name (regcache, "vl", regbuf);
+  regbuf = (gdb_byte *) buf + offsetof (struct __riscv_vregs, vstate.vtype);
+  collect_register_by_name (regcache, "vtype", regbuf);
+  regbuf = (gdb_byte *) buf + offsetof (struct __riscv_vregs, vstate.vcsr);
+  collect_register_by_name (regcache, "vcsr", regbuf);
+  regbuf = (gdb_byte *) & u64_vlenb;
+  collect_register_by_name (regcache, "vlenb", regbuf);
+
+
+  regbuf = (gdb_byte *) buf + offsetof (struct __riscv_vregs, data);
+  for (i = 0; i < 32; i++, regbuf += vlenb)
+    collect_register (regcache, regno + i, regbuf);
+}
+
+/* Supply vector registers from BUF into REGCACHE.  */
+
+static void
+riscv_store_vregset (struct regcache *regcache, const void *buf)
+{
+  const struct target_desc *tdesc = regcache->tdesc;
+  int regno = find_regno (tdesc, "v0");
+  int vlenb = register_size (regcache->tdesc, regno);
+  uint64_t u64_vlenb = vlenb;	/* pad to max XLEN for buffer conversion */
+  uint64_t vcsr;
+  uint64_t vxsat;
+  uint64_t vxrm;  
+  const gdb_byte *regbuf;
+  int i;
+
+  regbuf =
+    (const gdb_byte *) buf + offsetof (struct __riscv_vregs, vstate.vstart);
+  supply_register_by_name (regcache, "vstart", regbuf);
+  regbuf =
+    (const gdb_byte *) buf + offsetof (struct __riscv_vregs, vstate.vl);
+  supply_register_by_name (regcache, "vl", regbuf);
+  regbuf =
+    (const gdb_byte *) buf + offsetof (struct __riscv_vregs, vstate.vtype);
+  supply_register_by_name (regcache, "vtype", regbuf);
+  regbuf =
+    (const gdb_byte *) buf + offsetof (struct __riscv_vregs, vstate.vcsr);
+  supply_register_by_name (regcache, "vcsr", regbuf);
+  /* also store off a non-byte-wise copy of vcsr, to derive values for vxsat and vxrm */
+  vcsr = *(uint64_t*)regbuf;
+  /* vlenb isn't part of vstate, but we have already inferred its value by running code on this
+     hart, and we're assuming homogeneous VLENB if it's an SMP system */
+  regbuf = (gdb_byte *) & u64_vlenb;
+  supply_register_by_name (regcache, "vlenb", regbuf);
+
+  /* vxsat and vxrm, are not part of vstate, so we have to extract from VCSR
+     value */
+  vxsat = ((vcsr >> VCSR_POS_VXSAT) & VCSR_MASK_VXSAT);  
+  regbuf = (gdb_byte *) &vxsat;
+  supply_register_by_name (regcache, "vxsat", regbuf);
+  vxrm = ((vcsr >> VCSR_POS_VXRM) & VCSR_MASK_VXRM);  
+  regbuf = (gdb_byte *) &vxrm;
+  supply_register_by_name (regcache, "vxrm", regbuf);
+
+  /* v0..v31 */
+  regbuf = (const gdb_byte *) buf + offsetof (struct __riscv_vregs, data);
+  for (i = 0; i < 32; i++, regbuf += vlenb)
+    supply_register (regcache, regno + i, regbuf);
+}
+
 /* RISC-V/Linux regsets.  FPRs are optional and come in different sizes,
    so define multiple regsets for them marking them all as OPTIONAL_REGS
    rather than FP_REGS, so that "regsets_fetch_inferior_registers" picks
@@ -175,6 +282,9 @@ static struct regset_info riscv_regsets[] = {
   { PTRACE_GETREGSET, PTRACE_SETREGSET, NT_FPREGSET,
     sizeof (struct __riscv_mc_f_ext_state), OPTIONAL_REGS,
     riscv_fill_fpregset, riscv_store_fpregset },
+  { PTRACE_GETREGSET, PTRACE_SETREGSET, NT_RISCV_VECTOR,
+    sizeof (struct __riscv_vregs), OPTIONAL_REGS,
+    riscv_fill_vregset, riscv_store_vregset },
   NULL_REGSET
 };
 
-- 
2.25.1

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

[Andrew Burgess]

Thanks for working on this.

Based on some of the other emails, I guess kernel support for some of
this is still WIP, so I guess there's not "easy" way to test this?  Does
QEMU have vector support yet?

Given the other emails in this chain, I guess there's a V3 for this
series coming soon.  But I still have some thoughts, see inline below.

Greg Savin via Gdb-patches <gdb-patches@sourceware.org> writes:

> A v2 re-spin of the original patch.
>
> Now using regcache_map_entry to encode the vector register buffer format.
>
> Regarding moving NT_RISCV_VECTOR to binutils common.h, I'll be submitting that in
> a separate patch.  Evidently the Linux kernel headers are using 0x900 to
> refer to NT_RISCV_VECTOR, but in binutils/common.h, 0x900 is being used
> to refer to NT_RISCV_CSR.  I'll submit a patch that proposes re-numbering
> NT_RISCV_CSR to 0x901, and that declares NT_RISCV_VECTOR with value 0x900,
> to match the Linux kernel headers.
>
> Regarding representing VXRM and VXSAT as pseudo-registers, I see the rationale
> for that in the native debug via ptrace() case, because VXRM and VXSAT are not
> included as part of the vector register transfer payload in ptrace().  However,
> in the architecture of RISCV vector extension, VXRM and VXSAT do in fact exist
> as CSRs in their own right, and for non-native debug (e.g. JTAG-based bare metal
> via OpenOCD's gdbstub) those registers may well be accessed directly rather than
> via VCSR.  Because VXRM and VXSAT are architecturally spec'ed as CSRs in their
> own right, I'm wary of modeling them as pseudo-registers because I'd be concerned
> about unintended consequences for non-native RISC-V debug
> configurations.
>
>
> ---
>  gdb/arch/riscv.c             | 191 ++++++++++++++++++++++++++++++++++-
>  gdb/nat/riscv-linux-tdesc.c  |  68 +++++++++++++
>  gdb/nat/riscv-linux-tdesc.h  |  27 +++++
>  gdb/riscv-linux-nat.c        | 162 +++++++++++++++++++++++++++++
>  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, 737 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 */

Throughout this patch comments don't end in a '.'.  And there should be
two spaces at the end, with the trailing '*/' on the same line as the
last line of the comment:

  /* Comments should look like this.  */

I'll not point out every places that needs fixing, but there's a few.

> +#define RISCV_V0_REGNUM 4162   
> +#else
> +#include "defs.h"
> +#include "riscv-tdep.h"
> +#endif

I think John's suggestion for moving the register number enum is the
right way to go here.

> +
> +static int
> +create_feature_riscv_vector_from_features (struct target_desc *result,
> +					   long regnum,
> +					   const struct riscv_gdbarch_features
> +					   features);
> +
> +
>  /* See arch/riscv.h.  */
>  
>  STATIC_IN_GDB target_desc_up
> @@ -84,15 +102,180 @@ riscv_create_target_description (const struct riscv_gdbarch_features features)
>    else if (features.flen == 8)
>      regnum = create_feature_riscv_64bit_fpu (tdesc.get (), regnum);
>  
> -  /* Currently GDB only supports vector features coming from remote
> -     targets.  We don't support creating vector features on native targets
> -     (yet).  */
>    if (features.vlen != 0)
> -    error (_("unable to create vector feature"));
> +    regnum =
> +      create_feature_riscv_vector_from_features (tdesc.get (),
> +						 RISCV_V0_REGNUM, features);
>  
>    return tdesc;
>  }
>  
> +
> +
> +/* Usually, these target_desc instances are static for an architecture, and expressable
> +   in XML format, but this is a special case where length of a RISC-V vector register
> +   is not architecturally fixed to a constant (the maximuim width is a defined constant,
> +   but it's nice to tailor a target description the actual VLENB) */
> +static int
> +create_feature_riscv_vector_from_features (struct target_desc *result,
> +					   long regnum,
> +					   const struct riscv_gdbarch_features
> +					   features)
> +{
> +  struct tdesc_feature *feature;
> +  unsigned long bitsize;
> +
> +  feature = tdesc_create_feature (result, "org.gnu.gdb.riscv.vector");
> +  tdesc_type *element_type;
> +
> +  /* if VLENB is present (which we know it is present if execution reaches this function),
> +     then we know by definition that it is at least 4 bytes wide */
> +  
> +  element_type = tdesc_named_type (feature, "uint8");
> +  tdesc_create_vector (feature, "bytes", element_type, features.vlen);
> +
> +  element_type = tdesc_named_type (feature, "uint16");
> +  tdesc_create_vector (feature, "shorts", element_type, features.vlen / 2);
> +
> +  element_type = tdesc_named_type (feature, "uint32");
> +  tdesc_create_vector (feature, "words", element_type, features.vlen / 4);
> +
> +  /* Need VLENB value checks for element chunks larger than 4 bytes */
> +  
> +  if (features.vlen >= 8)
> +    {
> +      element_type = tdesc_named_type (feature, "uint64");
> +      tdesc_create_vector (feature, "longs", element_type, features.vlen / 8);
> +    }
> +
> +  /* QEMU and OpenOCD include the quads width in their target descriptions, so we're
> +     following that precedent, even if it's not particularly useful in practice, yet */
> +  
> +  if (features.vlen >= 16)
> +    {
> +      element_type = tdesc_named_type (feature, "uint128");
> +      tdesc_create_vector (feature, "quads", element_type,
> +			   features.vlen / 16);
> +    }
> +
> +  tdesc_type_with_fields *type_with_fields;
> +  type_with_fields = tdesc_create_union (feature, "riscv_vector");
> +  tdesc_type *field_type;
> +
> +  if (features.vlen >= 16)
> +    {
> +      field_type = tdesc_named_type (feature, "quads");
> +      tdesc_add_field (type_with_fields, "q", field_type);
> +    }
> +  if (features.vlen >= 8)
> +    {
> +      field_type = tdesc_named_type (feature, "longs");
> +      tdesc_add_field (type_with_fields, "l", field_type);
> +    }
> +
> +  /* Again, we know vlenb is >= 4, so no if guards needed for words/shorts/bytes */
> +  
> +  field_type = tdesc_named_type (feature, "words");
> +  tdesc_add_field (type_with_fields, "w", field_type);
> +  
> +  field_type = tdesc_named_type (feature, "shorts");
> +  tdesc_add_field (type_with_fields, "s", field_type);
> +  
> +  field_type = tdesc_named_type (feature, "bytes");
> +  tdesc_add_field (type_with_fields, "b", field_type);
> +
> +  /* Using magic numbers for regnum parameter of these CSRs.  Magic numbers aren't ever ideal,
> +     but didn't find a clear alternative that compiles successfully in both the gdb and gdbserver
> +     build steps.

If you move the register number enum, then couldn't you replace these
magic numbers with the enum names?

Also the lines throughout this comment are far too long.  Keep lines
under 80 characters where possible please.  There are other places where
your comments are too wide.

>                      A mitigating factor is that these numbers
> +     should be stable because they are based on constituent values that should also be stable:
> +     RISCV_FIRST_CSR_REGNUM (a fixed constant) added to the respective CSR numbers from RISC-V     
> +     specifications.  Also there is some precedent for magic numbers; the *.xml files in features/riscv/
> +     use magic numbers to refer to floating point CSRs.

There's a comment at the head of those XML files that explains the
reasoning for the hard-coded numbers: backward compatibility.
Specifically, older QEMU releases didn't send an XML description for
RISC-V, and instead had an assumed register numbering.

If we were brave we could possibly drop that fixed numbering now, as
QEMU has used XML for some years.  The risk would be there might be
others out there who still use the fixed numbering, so until there's a
compelling reason, I don't see a need to drop the existing hard-coded
numbering, but I'd rather not add any more.

> +
> +     Also, the init_target_desc function in gdbserver expects all these registers to be ordered
> +     in increasing order of "GDB internals" register number, with CSRs before vN registers and in relative numeric order
> +     ascending.

I'd like to understand more about this.  gdbserver does expect
ascending register numbering, but in what way is it tied to GDB's
internal numbering?  The x-reg/f-reg register read/write code seems to
be agnostic to the exact register numbering.

>                   DWARF register numbers don't seem to follow that pattern, and it seems to be necessary to use the GDB
> +     regnums in order for things to work on both native gdb and gdbserver.

I'd really like to understand more about this failure as "seems to be
necessary" just feels like "we don't 100% understand what's going on
here".

> +   */

> +  tdesc_create_reg (feature, "vstart", 73, 1, NULL, features.xlen * 8, "int");
> +  tdesc_create_reg (feature, "vxsat", 74, 1, NULL, features.xlen * 8, "int");
> +  tdesc_create_reg (feature, "vxrm", 75, 1, NULL, features.xlen * 8, "int");  
> +  tdesc_create_reg (feature, "vcsr", 80, 1, NULL, features.xlen * 8, "int");
> +  tdesc_create_reg (feature, "vl", 3169, 1, NULL, features.xlen * 8, "int");
> +  tdesc_create_reg (feature, "vtype", 3170, 1, NULL, features.xlen * 8, "int");
> +  tdesc_create_reg (feature, "vlenb", 3171, 1, NULL, features.xlen * 8, "int");
> +
> +  bitsize = features.vlen * 8;
> +  tdesc_create_reg (feature, "v0", regnum++, 1, NULL, bitsize,
> +		    "riscv_vector");
> +  tdesc_create_reg (feature, "v1", regnum++, 1, NULL, bitsize,
> +		    "riscv_vector");
> +  tdesc_create_reg (feature, "v2", regnum++, 1, NULL, bitsize,
> +		    "riscv_vector");
> +  tdesc_create_reg (feature, "v3", regnum++, 1, NULL, bitsize,
> +		    "riscv_vector");
> +  tdesc_create_reg (feature, "v4", regnum++, 1, NULL, bitsize,
> +		    "riscv_vector");
> +  tdesc_create_reg (feature, "v5", regnum++, 1, NULL, bitsize,
> +		    "riscv_vector");
> +  tdesc_create_reg (feature, "v6", regnum++, 1, NULL, bitsize,
> +		    "riscv_vector");
> +  tdesc_create_reg (feature, "v7", regnum++, 1, NULL, bitsize,
> +		    "riscv_vector");
> +  tdesc_create_reg (feature, "v8", regnum++, 1, NULL, bitsize,
> +		    "riscv_vector");
> +  tdesc_create_reg (feature, "v9", regnum++, 1, NULL, bitsize,
> +		    "riscv_vector");
> +  tdesc_create_reg (feature, "v10", regnum++, 1, NULL, bitsize,
> +		    "riscv_vector");
> +  tdesc_create_reg (feature, "v11", regnum++, 1, NULL, bitsize,
> +		    "riscv_vector");
> +  tdesc_create_reg (feature, "v12", regnum++, 1, NULL, bitsize,
> +		    "riscv_vector");
> +  tdesc_create_reg (feature, "v13", regnum++, 1, NULL, bitsize,
> +		    "riscv_vector");
> +  tdesc_create_reg (feature, "v14", regnum++, 1, NULL, bitsize,
> +		    "riscv_vector");
> +  tdesc_create_reg (feature, "v15", regnum++, 1, NULL, bitsize,
> +		    "riscv_vector");
> +  tdesc_create_reg (feature, "v16", regnum++, 1, NULL, bitsize,
> +		    "riscv_vector");
> +  tdesc_create_reg (feature, "v17", regnum++, 1, NULL, bitsize,
> +		    "riscv_vector");
> +  tdesc_create_reg (feature, "v18", regnum++, 1, NULL, bitsize,
> +		    "riscv_vector");
> +  tdesc_create_reg (feature, "v19", regnum++, 1, NULL, bitsize,
> +		    "riscv_vector");
> +  tdesc_create_reg (feature, "v20", regnum++, 1, NULL, bitsize,
> +		    "riscv_vector");
> +  tdesc_create_reg (feature, "v21", regnum++, 1, NULL, bitsize,
> +		    "riscv_vector");
> +  tdesc_create_reg (feature, "v22", regnum++, 1, NULL, bitsize,
> +		    "riscv_vector");
> +  tdesc_create_reg (feature, "v23", regnum++, 1, NULL, bitsize,
> +		    "riscv_vector");
> +  tdesc_create_reg (feature, "v24", regnum++, 1, NULL, bitsize,
> +		    "riscv_vector");
> +  tdesc_create_reg (feature, "v25", regnum++, 1, NULL, bitsize,
> +		    "riscv_vector");
> +  tdesc_create_reg (feature, "v26", regnum++, 1, NULL, bitsize,
> +		    "riscv_vector");
> +  tdesc_create_reg (feature, "v27", regnum++, 1, NULL, bitsize,
> +		    "riscv_vector");
> +  tdesc_create_reg (feature, "v28", regnum++, 1, NULL, bitsize,
> +		    "riscv_vector");
> +  tdesc_create_reg (feature, "v29", regnum++, 1, NULL, bitsize,
> +		    "riscv_vector");
> +  tdesc_create_reg (feature, "v30", regnum++, 1, NULL, bitsize,
> +		    "riscv_vector");
> +  tdesc_create_reg (feature, "v31", regnum++, 1, NULL, bitsize,
> +		    "riscv_vector");
> +
> +
> +  return regnum;
> +}
> +
> +
>  #ifndef GDBSERVER
>  
>  /* Wrapper used by std::unordered_map to generate hash for feature set.  */
> diff --git a/gdb/nat/riscv-linux-tdesc.c b/gdb/nat/riscv-linux-tdesc.c
> index d676233cc31..51d89108575 100644
> --- a/gdb/nat/riscv-linux-tdesc.c
> +++ b/gdb/nat/riscv-linux-tdesc.c
> @@ -23,14 +23,18 @@
>  #include "elf/common.h"
>  #include "nat/gdb_ptrace.h"
>  #include "nat/riscv-linux-tdesc.h"
> +#include "gdbsupport/gdb_setjmp.h"
>  
>  #include <sys/uio.h>
> +#include <signal.h>
>  
>  /* Work around glibc header breakage causing ELF_NFPREG not to be usable.  */
>  #ifndef NFPREG
>  # define NFPREG 33
>  #endif
>  
> +static unsigned long safe_read_vlenb ();

This function should have a comment.

> +
>  /* See nat/riscv-linux-tdesc.h.  */
>  
>  struct riscv_gdbarch_features
> @@ -79,5 +83,69 @@ riscv_linux_read_features (int tid)
>        break;
>      }
>  
> +  features.vlen = safe_read_vlenb ();
> +
>    return features;
>  }
> +
> +static SIGJMP_BUF sigill_guard_jmp_buf;
> +
> +static void
> +sigill_guard (int sig)
> +{
> +  /* this will gets us back to caller deeper in the call stack, with an indication that
> +     an illegal instruction condition was encountered */
> +  SIGLONGJMP (sigill_guard_jmp_buf, -1);
> +
> +  /* control won't get here */
> +}

I think based on the other thread of this email chain, the plan is to
extend the ptrace API so this sigill stuff will not be needed -- I'll
not bother looking at any of this then.

> +
> +
> +
> +static unsigned long
> +safe_read_vlenb ()
> +{
> +  /* Surrounding the attempt here to read VLENB CSR to have a signal handler set up
> +     to trap illegal instruction condition (SIGILL), and if a trap happens during this call,
> +     get control back within this function and return 0 in that case.
> +   */
> +  unsigned long vlenb = 0;
> +  struct sigaction our_action = { 0 };
> +  struct sigaction original_action;
> +  int sysresult;
> +
> +
> +  our_action.sa_handler = sigill_guard;
> +
> +  sysresult = sigaction (SIGILL, &our_action, &original_action);
> +  if (sysresult != 0)
> +    {
> +      perror
> +	("Error installing temporary SIGILL handler in safe_read_vlenb()");
> +    }
> +
> +  if (SIGSETJMP (sigill_guard_jmp_buf, 1) == 0)
> +    {
> +    asm ("csrr %0, vlenb":"=r" (vlenb));
> +    }
> +  else
> +    {
> +      /* Must've generated an illegal instruction condition; we'll figure this means
> +         no vector unit is present */
> +      vlenb = 0;
> +    }
> +
> +
> +  if (sysresult == 0)
> +    {
> +      /* re-install former handler */
> +      sysresult = sigaction (SIGILL, &original_action, NULL);
> +      if (sysresult != 0)
> +	{
> +	  perror
> +	    ("Error re-installing original SIGILL handler in safe_read_vlenb()");
> +	}
> +
> +    }
> +  return vlenb;
> +}
> 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 */

Isn't this defined in an include file somewhere?  Or maybe that was a
separate patch?  Anyway, I think this shouldn't be here.

> +#endif
> +
> +/* Some branches and/or commits of linux kernel named this "struct __riscv_v_state",
> +   and later it was changed to "struct __riscv_v_ext_state",
> +   so using a macro to stand-in for that struct type to make it easier to modify
> +   in a single place, if compiling against one of those older Linux kernel commits */
> +#ifndef RISCV_VECTOR_STATE_T
> +#define RISCV_VECTOR_STATE_T struct __riscv_v_ext_state
> +#endif
> +
> +/* Struct for use in ptrace() calls for vector CSRs/registers */
> +struct __riscv_vregs
> +{
> +  RISCV_VECTOR_STATE_T vstate;
> +  gdb_byte data[RISCV_MAX_VLENB * 32];	/* data will arrive packed, VLENB bytes per element, not necessarily RISCV_MAX_VLENB bytes per element */
> +};
> +
> +#define VCSR_MASK_VXSAT 0x1
> +#define VCSR_POS_VXSAT 0
> +#define VCSR_MASK_VXRM 0x3
> +#define VCSR_POS_VXRM 1
> +
> +
>  #endif /* NAT_RISCV_LINUX_TDESC_H */
> diff --git a/gdb/riscv-linux-nat.c b/gdb/riscv-linux-nat.c
> index 8be4a5ac3e5..38fdd0ac5e0 100644
> --- a/gdb/riscv-linux-nat.c
> +++ b/gdb/riscv-linux-nat.c
> @@ -22,6 +22,7 @@
>  #include "linux-nat.h"
>  #include "riscv-tdep.h"
>  #include "inferior.h"
> +#include "regset.h"
>  
>  #include "elf/common.h"
>  
> @@ -125,6 +126,113 @@ supply_fpregset_regnum (struct regcache *regcache, const prfpregset_t *fpregs,
>      }
>  }
>  
> +#define MEMBER_SIZE(type, member) sizeof(((type *)0)->member)

The spacing for the macro expansion should be:

  sizeof (((type *) 0)->member)

> +
> +static const regcache_map_entry riscv_linux_vregmap[] =
> +{
> +  { 1, RISCV_CSR_VSTART_REGNUM, MEMBER_SIZE(struct __riscv_vregs, vstate.vstart) },
> +  { 1, RISCV_CSR_VL_REGNUM, MEMBER_SIZE(struct __riscv_vregs, vstate.vl) },
> +  { 1, RISCV_CSR_VTYPE_REGNUM, MEMBER_SIZE(struct __riscv_vregs, vstate.vtype) },
> +  { 1, RISCV_CSR_VCSR_REGNUM, MEMBER_SIZE(struct __riscv_vregs, vstate.vcsr) },
> +  /* struct __riscv_vregs member "datap" is a pointer that doesn't correspond
> +     to a register value.  In the context of ptrace(), member is always zero,
> +     with V0..V31 values inline after that.  So, skipping datap */
> +  { 1, REGCACHE_MAP_SKIP, MEMBER_SIZE(struct __riscv_vregs, vstate.datap) },
> +  /* Here's V0..V31.  Specifying 0 as size leads to a call to register_size()
> +     for size determination */
> +  { 32, RISCV_V0_REGNUM, 0 },
> +  { 0 },  /* count==0 represents termination of entries */
> +};
> +
> +/* Define the vector register regset.  */
> +
> +static const struct regset riscv_linux_vregset =
> +{
> +  riscv_linux_vregmap,
> +  regcache_supply_regset /* Other RISC-V regsets use riscv_supply_regset here; not sure that'd be correct for this case */,
> +  regcache_collect_regset
> +};
> +
> +
> +static void
> +supply_vregset_regnum (struct regcache *regcache,
> +		       const struct __riscv_vregs *vregs, int regnum)

Every function should have a header comment before it.

> +{
> +  const gdb_byte *buf;
> +  int vlenb = register_size (regcache->arch (), RISCV_V0_REGNUM);
> +
> +  regcache_supply_regset (&riscv_linux_vregset, regcache, regnum, vregs, sizeof(*vregs));  
> +
> +  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;
> +
> +  regcache_collect_regset (&riscv_linux_vregset, regcache, regnum, vregs, sizeof(*vregs));    
> +
> +  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;      
> +
> +      if ( ! (regnum == -1 || regnum == RISCV_CSR_VCSR_REGNUM) )
> +	{
> +	  // we don't already have the VCSR value, from the earlier regcache_collect_regset call, so let's get it now

GDB doesn't use // for comments.

> +	  regcache_collect_regset (&riscv_linux_vregset, regcache, RISCV_CSR_VCSR_REGNUM, vregs, sizeof(*vregs));    	  
> +	}
> +
> +      if (regnum == RISCV_CSR_VXSAT_REGNUM)
> +	{
> +	  /* Overwrite VCSR with the VXSAT bit here */
> +	  buf = (gdb_byte*)&vxsat_from_regcache;

As BUF isn't going to outlive this scope, better to write:

  gdb_byte *buf = (gdb_byte *) &vxsat_from_regcache;

Notice two extra spaces I've added to comply with GDB style.

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

Space after '(uint64_t) '.

> +	  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 */
> +
> +}
> +
> +
>  /* Copy all floating point registers from regset FPREGS into REGCACHE.  */
>  
>  void
> @@ -252,6 +360,31 @@ riscv_linux_nat_target::fetch_registers (struct regcache *regcache, int regnum)
>  	supply_fpregset_regnum (regcache, &regs, regnum);
>      }
>  
> +  /* if Linux kernel was not configured to support RISC-V vectors, then
> +     the ptrace call will return -1, and we just won't get vector registers,
> +     but in that case it wouldn't be an error that needs user attention.
> +   */
> +  if ((regnum >= RISCV_V0_REGNUM && regnum <= RISCV_V31_REGNUM)
> +      || (regnum == RISCV_CSR_VSTART_REGNUM)
> +      || (regnum == RISCV_CSR_VL_REGNUM)
> +      || (regnum == RISCV_CSR_VTYPE_REGNUM)
> +      || (regnum == RISCV_CSR_VCSR_REGNUM)
> +      || (regnum == RISCV_CSR_VLENB_REGNUM)
> +      || (regnum == RISCV_CSR_VXSAT_REGNUM)
> +      || (regnum == RISCV_CSR_VXRM_REGNUM)
> +      || (regnum == -1))
> +    {
> +      struct iovec iov;
> +      struct __riscv_vregs vregs;
> +
> +      iov.iov_base = &vregs;
> +      iov.iov_len = sizeof (vregs);
> +
> +      if (ptrace (PTRACE_GETREGSET, tid, NT_RISCV_VECTOR,
> +		  (PTRACE_TYPE_ARG3) & iov) == 0)
> +	supply_vregset_regnum (regcache, &vregs, regnum);
> +    }
> +
>    if ((regnum == RISCV_CSR_MISA_REGNUM)
>        || (regnum == -1))
>      {
> @@ -321,6 +454,35 @@ riscv_linux_nat_target::store_registers (struct regcache *regcache, int regnum)
>  	}
>      }
>  
> +  /* VLENB isn't writable, so we'll skip considering that one, if it's being
> +     specified alone */
> +  if ((regnum >= RISCV_V0_REGNUM && regnum <= RISCV_V31_REGNUM)
> +      || (regnum == RISCV_CSR_VSTART_REGNUM)
> +      || (regnum == RISCV_CSR_VL_REGNUM)
> +      || (regnum == RISCV_CSR_VTYPE_REGNUM)
> +      || (regnum == RISCV_CSR_VCSR_REGNUM)
> +      || (regnum == RISCV_CSR_VXSAT_REGNUM)
> +      || (regnum == RISCV_CSR_VXRM_REGNUM)
> +      || (regnum == -1))
> +    {
> +      struct iovec iov;
> +      struct __riscv_vregs vregs;
> +
> +      iov.iov_base = &vregs;
> +      iov.iov_len = sizeof (vregs);
> +
> +      if (ptrace (PTRACE_GETREGSET, tid, NT_RISCV_VECTOR,
> +		  (PTRACE_TYPE_ARG3) & iov) == 0)
> +	{
> +	  fill_vregset (regcache, &vregs, regnum);
> +
> +	  if (ptrace (PTRACE_SETREGSET, tid, NT_RISCV_VECTOR,
> +		      (PTRACE_TYPE_ARG3) & iov) == -1)
> +	    perror_with_name (_("Couldn't set vector registers"));
> +	}
> +    }
> +
> +
>    /* Access to CSRs has potential security issues, don't support them for
>       now.  */
>  }
> diff --git a/gdb/riscv-linux-tdep.c b/gdb/riscv-linux-tdep.c
> index 292d7a4ef7c..e2b5e5cf4b4 100644
> --- a/gdb/riscv-linux-tdep.c
> +++ b/gdb/riscv-linux-tdep.c
> @@ -32,6 +32,10 @@
>  
>  #define RISCV_NR_rt_sigreturn 139
>  
> +/* Magic number written to the head.magic field of struct __sc_riscv_v_state that kernel
> +   places in the reserved area of struct sigcontext.  Comes from <asm/sigcontext.h> */
> +#define RVV_MAGIC 0x53465457
> +
>  /* Define the general register mapping.  The kernel puts the PC at offset 0,
>     gdb puts it at offset 32.  Register x0 is always 0 and can be ignored.
>     Registers x1 to x31 are in the same place.  */
> @@ -120,8 +124,122 @@ static const struct tramp_frame riscv_linux_sigframe = {
>       mcontext_t uc_mcontext;
>     }; */
>  
> +
> +
> +/* riscv_linux_vector_sigframe_header_check() returns an answer to the question
> +   "is there a RISC-V Vector header at this memory location"? */
> +

These comments can be assumed to apply to the immediately following
function, and should reference the arguments, so something like:

  /* Read .... from address REGS_BASE and return true if ...., otherwise
     return false.  THIS_FRAME is used for the architecture and
     byte-order when reading memory.  VLEN and XLEN are the v-register
     and x-register sizes in bytes(?) and are used for validation.  */

> +static bool
> +riscv_linux_vector_sigframe_header_check (frame_info_ptr this_frame,
> +					  int vlen, int xlen,
> +					  CORE_ADDR regs_base)
> +{
> +  uint32_t rvv_magic;
> +  uint32_t rvv_size;
> +  bool info_good = false;
> +
> +  /* If vector information is available, then we should see this structure at this address:
> +     struct __riscv_ctx_hdr {
> +     __u32 magic;  (RVV_MAGIC).
> +     __u32 size;   (size of struct __sc_riscv_v_state + vector register data size (32*VLENB))
> +     } head;
> +   */
> +
> +  rvv_magic =
> +    get_frame_memory_unsigned (this_frame, regs_base, sizeof (rvv_magic));
> +  regs_base += sizeof (rvv_magic);
> +  rvv_size =
> +    get_frame_memory_unsigned (this_frame, regs_base, sizeof (rvv_magic));
> +  regs_base += sizeof (rvv_size);
> +
> +
> +  info_good = (rvv_magic == RVV_MAGIC);
> +  if (!info_good)
> +    {
> +      /* Not an error, because kernels can be configured without CONFIG_VECTOR, but worth noting if frame debug
> +         setting is turned on */
> +      if (frame_debug)
> +	frame_debug_printf
> +	  ("Did not find RISC-V vector information in ucontext (kernel not built with CONFIG_VECTOR?)");

You don't need the 'if (frame_debug)' here, frame_debug_printf has that
built-in.

> +
> +      return false;
> +    }
> +
> +  if (frame_debug)
> +    {
> +      uint32_t expected_rvv_size;
> +
> +      frame_debug_printf
> +	("Located RISC-V vector information in signal frame ucontext (info size %u)",
> +	 rvv_size);
> +
> +      /* sanity check the reported size; should be sizeof(uint32_t) + sizeof(uint32_t) + 5 * XLENB + 32 * vlen */
> +      expected_rvv_size = sizeof (uint32_t) /* magic */  +
> +	sizeof (uint32_t) /* size */  +
> +	5 * xlen /* vstart, vl, vtype, vcsr, and datap */  +
> +	32 * vlen;		/* v0..v31 values */
> +
> +      if (rvv_size != expected_rvv_size)
> +	{
> +	  /* It doesn't seem like this should be a hard error, but it'd be good to make it visible if frame debug
> +	     setting is turned on */
> +	  frame_debug_printf
> +	    ("Size in RISC-V vector information header in ucontext differs from the expected %u",
> +	     expected_rvv_size);
> +	}

If this shouldn't be a hard error then maybe it should still be a
warning?  Most users aren't going to turn on debug output, but it feels
like, if they got this warning, and then something didn't work as
expected, the user is more likely to start asking the right questions.

> +    }
> +
> +  return info_good;
> +}
> +
> +static CORE_ADDR
> +riscv_linux_sigframe_vector_init (frame_info_ptr this_frame,
> +				  struct trad_frame_cache *this_cache,
> +				  CORE_ADDR regs_base, int xlen, int vlen)
> +{
> +  int vfieldidx;		/* index of "unsigned long" members in __riscv_v_ext_state */
> +  CORE_ADDR p_datap;
> +  CORE_ADDR datap;		/* dereferenced value of void *datap that points to v0..v31 */

GDB doesn't place variable declarations at the top of the scope like
this any more.  Where possible variables should be declared at the point
they are first used.

Comments are better placed on a line before the declaration or first use
as, like here, trying to place them inline just results in long lines.

> +
> +  /* vstart, vl, vtype, vcsr, and datap are XLEN sized fields (unsigned long) from this point */
> +  vfieldidx = 0;
> +  trad_frame_set_reg_addr (this_cache, RISCV_CSR_VSTART_REGNUM,
> +			   regs_base + (vfieldidx * xlen));
> +  vfieldidx++;
> +  trad_frame_set_reg_addr (this_cache, RISCV_CSR_VL_REGNUM,
> +			   regs_base + (vfieldidx * xlen));
> +
> +  vfieldidx++;
> +  trad_frame_set_reg_addr (this_cache, RISCV_CSR_VTYPE_REGNUM,
> +			   regs_base + (vfieldidx * xlen));
> +
> +  vfieldidx++;
> +  trad_frame_set_reg_addr (this_cache, RISCV_CSR_VCSR_REGNUM,
> +			   regs_base + (vfieldidx * xlen));
> +
> +  /* for the datap member, there is one level of memory indirection to get the address of
> +     the block of values for v0..v31 */
> +  vfieldidx++;
> +  p_datap = regs_base + (vfieldidx * xlen);
> +  datap = get_frame_memory_unsigned (this_frame, p_datap, xlen);
> +  regs_base = datap;
> +  for (int i = 0; i < 32; i++)
> +    {
> +      trad_frame_set_reg_addr (this_cache, RISCV_V0_REGNUM + i,
> +			       regs_base + (i * vlen));
> +    }
> +  regs_base += 32 * vlen;
> +
> +  return regs_base;
> +}
> +
> +
>  #define SIGFRAME_SIGINFO_SIZE		128
>  #define UCONTEXT_MCONTEXT_OFFSET	176
> +#define MCONTEXT_VECTOR_OFFSET		784	/* offset of struct mcontext's __reserved field,
> +						   which is where the struct __sc_riscv_v_state is overlaid */
> +#define RISCV_CONTEXT_HEADER_SIZE	8	/* size of struct __riscv_ctx_hdr {__u32 magic;  __u32 size; } */
> +
>  
>  static void
>  riscv_linux_sigframe_init (const struct tramp_frame *self,
> @@ -132,6 +250,7 @@ riscv_linux_sigframe_init (const struct tramp_frame *self,
>    struct gdbarch *gdbarch = get_frame_arch (this_frame);
>    int xlen = riscv_isa_xlen (gdbarch);
>    int flen = riscv_isa_flen (gdbarch);
> +  int vlen = riscv_isa_vlen (gdbarch);
>    CORE_ADDR frame_sp = get_frame_sp (this_frame);
>    CORE_ADDR mcontext_base;
>    CORE_ADDR regs_base;
> @@ -155,6 +274,19 @@ riscv_linux_sigframe_init (const struct tramp_frame *self,
>    regs_base += 32 * flen;
>    trad_frame_set_reg_addr (this_cache, RISCV_CSR_FCSR_REGNUM, regs_base);
>  
> +  /* Handle the vector registers, if present. */
> +  if (vlen > 0)
> +    {
> +      regs_base = mcontext_base + MCONTEXT_VECTOR_OFFSET;
> +      if (riscv_linux_vector_sigframe_header_check
> +	  (this_frame, vlen, xlen, regs_base))
> +	{
> +	  regs_base += RISCV_CONTEXT_HEADER_SIZE;	/* advance past the header */
> +	  riscv_linux_sigframe_vector_init (this_frame, this_cache, regs_base,
> +					    xlen, vlen);
> +	}
> +    }
> +
>    /* Choice of the bottom of the sigframe is somewhat arbitrary.  */
>    trad_frame_set_id (this_cache, frame_id_build (frame_sp, func));
>  }
> diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
> index ae18eb64452..8714b750017 100644
> --- a/gdb/riscv-tdep.c
> +++ b/gdb/riscv-tdep.c
> @@ -47,6 +47,7 @@
>  #include "remote.h"
>  #include "target-descriptions.h"
>  #include "dwarf2/frame.h"
> +#include "dwarf2/expr.h"
>  #include "user-regs.h"
>  #include "valprint.h"
>  #include "gdbsupport/common-defs.h"
> @@ -650,6 +651,14 @@ struct riscv_vector_feature : public riscv_register_feature
>        { RISCV_V0_REGNUM + 29, { "v29" } },
>        { RISCV_V0_REGNUM + 30, { "v30" } },
>        { RISCV_V0_REGNUM + 31, { "v31" } },
> +      /* vector CSRs */
> +      { RISCV_CSR_VSTART_REGNUM, { "vstart" } },
> +      { RISCV_CSR_VXSAT_REGNUM, { "vxsat" } },
> +      { RISCV_CSR_VXRM_REGNUM, { "vxrm" } },
> +      { RISCV_CSR_VL_REGNUM, { "vl" } },
> +      { RISCV_CSR_VTYPE_REGNUM, { "vtype" } },
> +      { RISCV_CSR_VCSR_REGNUM, { "vcsr" } },
> +      { RISCV_CSR_VLENB_REGNUM, { "vlenb" } },

I think we should be careful adding these here.  Yes, the fpu feature
does allow for some CSRs, but with hindsight I don't think I would have
done it that way, instead I would just require targets to supply the csr
feature.

As a result of having this potential dual location for the FP CSRs I've
seen targets that supply the CSRs in both locations (which is wrong, but
GDB seems to manage with), and GDB has to have some special handling for
those CSRs because we can't know where to expect them.

Anyway, all the tdesc features are documented in the manual, so,
whatever we decide, I think this patch needs a docs update that
mentions these CSRs.  I would suggest:

  The @samp{org.gnu.gdb.riscv.vector} feature is optional.  If present,
  it should contain registers @samp{v0} through @samp{v31}, all of which
  must be the same size.  All vector related CSRs should be placed into
  the @samp{org.gnu.gdb.riscv.csr} feature.

But if you have a compelling argument for why the CSRs should live in
the vector feature, you'll need to update the docs accordingly.  You
should also review the docs for the csr feature -- depending on what
changes you make, you might also need to update that text too.

>      };
>    }
>  
> @@ -681,10 +690,16 @@ struct riscv_vector_feature : public riscv_register_feature
>  	return true;
>        }
>  
> -    /* Check all of the vector registers are present.  */
> +    /* Check all of the vector registers are present.  We also
> +       check that the vector CSRs are present too, though if these
> +       are missing this is not fatal.  */
>      for (const auto &reg : m_registers)
>        {
> -	if (!reg.check (tdesc_data, feature_vector, true, aliases))
> +	bool found = reg.check (tdesc_data, feature_vector, true, aliases);
> +	
> +	bool is_ctrl_reg_p = !(reg.regnum >= RISCV_V0_REGNUM && reg.regnum <= RISCV_V31_REGNUM);
> +
> +	if (!found && !is_ctrl_reg_p)
>  	  return false;
>        }
>  
> @@ -694,6 +709,12 @@ struct riscv_vector_feature : public riscv_register_feature
>      int vector_bitsize = -1;
>      for (const auto &reg : m_registers)
>        {
> +
> +	bool is_ctrl_reg_p = !(reg.regnum >= RISCV_V0_REGNUM && reg.regnum <= RISCV_V31_REGNUM);	
> +
> +	if (is_ctrl_reg_p)
> +	  continue;
> +
>  	int reg_bitsize = -1;
>  	for (const char *name : reg.names)
>  	  {
> @@ -804,6 +825,16 @@ riscv_abi_embedded (struct gdbarch *gdbarch)
>    return tdep->abi_features.embedded;
>  }
>  
> +/* See riscv-tdep.h.  */
> +
> +int
> +riscv_isa_vlen (struct gdbarch *gdbarch)
> +{
> +  riscv_gdbarch_tdep *tdep = gdbarch_tdep<riscv_gdbarch_tdep> (gdbarch);
> +  return tdep->isa_features.vlen;
> +}
> +
> +
>  /* Return true if the target for GDBARCH has floating point hardware.  */
>  
>  static bool
> @@ -1454,7 +1485,19 @@ riscv_register_reggroup_p (struct gdbarch  *gdbarch, int regnum,
>        return 0;
>      }
>    else if (reggroup == vector_reggroup)
> -    return (regnum >= RISCV_V0_REGNUM && regnum <= RISCV_V31_REGNUM);
> +    {
> +      if (regnum >= RISCV_V0_REGNUM && regnum <= RISCV_V31_REGNUM)
> +	return 1;
> +      if (regnum == RISCV_CSR_VSTART_REGNUM
> +	  || regnum == RISCV_CSR_VXSAT_REGNUM
> +	  || regnum == RISCV_CSR_VXRM_REGNUM
> +	  || regnum == RISCV_CSR_VL_REGNUM
> +	  || regnum == RISCV_CSR_VTYPE_REGNUM
> +	  || regnum == RISCV_CSR_VCSR_REGNUM
> +	  || regnum == RISCV_CSR_VLENB_REGNUM)
> +	return 1;
> +      return 0;
> +    }
>    else
>      return 0;
>  }
> diff --git a/gdb/riscv-tdep.h b/gdb/riscv-tdep.h
> index 4c3afb08e07..b183c58c7da 100644
> --- a/gdb/riscv-tdep.h
> +++ b/gdb/riscv-tdep.h
> @@ -150,6 +150,11 @@ extern int riscv_abi_flen (struct gdbarch *gdbarch);
>     argument registers.  */
>  extern bool riscv_abi_embedded (struct gdbarch *gdbarch);
>  
> +/* Return the width in bytes of the hardware vector registers for
> +   GDBARCH.  If this architecture has no vector registers, then
> +   return 0.  */
> +extern int riscv_isa_vlen (struct gdbarch *gdbarch);
> +
>  /* Single step based on where the current instruction will take us.  */
>  extern std::vector<CORE_ADDR> riscv_software_single_step
>    (struct regcache *regcache);
> diff --git a/gdbserver/linux-riscv-low.cc b/gdbserver/linux-riscv-low.cc
> index 129bc3b138b..169fa988c06 100644
> --- a/gdbserver/linux-riscv-low.cc
> +++ b/gdbserver/linux-riscv-low.cc
> @@ -158,6 +158,113 @@ riscv_store_fpregset (struct regcache *regcache, const void *buf)
>    supply_register_by_name (regcache, "fcsr", regbuf);
>  }
>  
> +/* Collect vector registers from REGCACHE into BUF.  */
> +
> +static void
> +riscv_fill_vregset (struct regcache *regcache, void *buf)
> +{
> +  const struct target_desc *tdesc = regcache->tdesc;
> +  int regno = find_regno (tdesc, "v0");
> +  int vlenb = register_size (regcache->tdesc, regno);
> +  uint64_t u64_vlenb = vlenb;	/* pad to max XLEN for buffer conversion */
> +  uint64_t u64_vxsat = 0;
> +  uint64_t u64_vxrm = 0;
> +  uint64_t u64_vcsr = 0;
> +  gdb_byte *regbuf;
> +  int i;

At least some of these should be moved inline below.

> +
> +  /* Since vxsat and equivalent bits in vcsr are aliases (and same for vxrm), we have a dilemma.
> +     For this gdb -> gdbserver topology, if the aliased pairs have values that disagree, then
> +     which value should take precedence?  We don't know which alias was most
> +     recently assigned.  We're just getting a block of register values including vxsat, vxrm,
> +     and vcsr.  We have to impose some kind of rule for predictable resolution to resolve any inconsistency.
> +     For now, let's say that vxsat and vxrm take precedence, and those values will be applied to the
> +     corresponding fields in vcsr.  Reconcile these 3 interdependent registers now:
> +  */
> +  regbuf = (gdb_byte *) & u64_vcsr;
> +  collect_register_by_name (regcache, "vcsr", regbuf);
> +  regbuf = (gdb_byte *) & u64_vxsat;
> +  collect_register_by_name (regcache, "vxsat", regbuf);
> +  regbuf = (gdb_byte *) & u64_vxrm;
> +  collect_register_by_name (regcache, "vxrm", regbuf);
> +  
> +  u64_vcsr &= ~((uint64_t)VCSR_MASK_VXSAT << VCSR_POS_VXSAT);
> +  u64_vcsr |= ((u64_vxsat & VCSR_MASK_VXSAT) << VCSR_POS_VXSAT);
> +  u64_vcsr &= ~((uint64_t)VCSR_MASK_VXRM << VCSR_POS_VXRM);	  
> +  u64_vcsr |= ((u64_vxrm & VCSR_MASK_VXRM) << VCSR_POS_VXRM);

Space after the type in a cast.

> +
> +  /* Replace the original vcsr value with the "cooked" value */
> +  regbuf = (gdb_byte *) & u64_vcsr;  
> +  supply_register_by_name (regcache, "vcsr", regbuf);
> +
> +  /* Now stage the ptrace buffer (it'll receive the cooked vcsr value) */
> +
> +  regbuf = (gdb_byte *) buf + offsetof (struct __riscv_vregs, vstate.vstart);
> +  collect_register_by_name (regcache, "vstart", regbuf);
> +  regbuf = (gdb_byte *) buf + offsetof (struct __riscv_vregs, vstate.vl);
> +  collect_register_by_name (regcache, "vl", regbuf);
> +  regbuf = (gdb_byte *) buf + offsetof (struct __riscv_vregs, vstate.vtype);
> +  collect_register_by_name (regcache, "vtype", regbuf);
> +  regbuf = (gdb_byte *) buf + offsetof (struct __riscv_vregs, vstate.vcsr);
> +  collect_register_by_name (regcache, "vcsr", regbuf);
> +  regbuf = (gdb_byte *) & u64_vlenb;
> +  collect_register_by_name (regcache, "vlenb", regbuf);
> +
> +
> +  regbuf = (gdb_byte *) buf + offsetof (struct __riscv_vregs, data);
> +  for (i = 0; i < 32; i++, regbuf += vlenb)
> +    collect_register (regcache, regno + i, regbuf);
> +}
> +
> +/* Supply vector registers from BUF into REGCACHE.  */
> +
> +static void
> +riscv_store_vregset (struct regcache *regcache, const void *buf)
> +{
> +  const struct target_desc *tdesc = regcache->tdesc;
> +  int regno = find_regno (tdesc, "v0");
> +  int vlenb = register_size (regcache->tdesc, regno);
> +  uint64_t u64_vlenb = vlenb;	/* pad to max XLEN for buffer conversion */
> +  uint64_t vcsr;
> +  uint64_t vxsat;
> +  uint64_t vxrm;  
> +  const gdb_byte *regbuf;
> +  int i;
> +
> +  regbuf =
> +    (const gdb_byte *) buf + offsetof (struct __riscv_vregs, vstate.vstart);
> +  supply_register_by_name (regcache, "vstart", regbuf);
> +  regbuf =
> +    (const gdb_byte *) buf + offsetof (struct __riscv_vregs, vstate.vl);
> +  supply_register_by_name (regcache, "vl", regbuf);
> +  regbuf =
> +    (const gdb_byte *) buf + offsetof (struct __riscv_vregs, vstate.vtype);
> +  supply_register_by_name (regcache, "vtype", regbuf);
> +  regbuf =
> +    (const gdb_byte *) buf + offsetof (struct __riscv_vregs, vstate.vcsr);
> +  supply_register_by_name (regcache, "vcsr", regbuf);
> +  /* also store off a non-byte-wise copy of vcsr, to derive values for vxsat and vxrm */
> +  vcsr = *(uint64_t*)regbuf;
> +  /* vlenb isn't part of vstate, but we have already inferred its value by running code on this
> +     hart, and we're assuming homogeneous VLENB if it's an SMP system */
> +  regbuf = (gdb_byte *) & u64_vlenb;
> +  supply_register_by_name (regcache, "vlenb", regbuf);
> +
> +  /* vxsat and vxrm, are not part of vstate, so we have to extract from VCSR
> +     value */
> +  vxsat = ((vcsr >> VCSR_POS_VXSAT) & VCSR_MASK_VXSAT);  
> +  regbuf = (gdb_byte *) &vxsat;
> +  supply_register_by_name (regcache, "vxsat", regbuf);
> +  vxrm = ((vcsr >> VCSR_POS_VXRM) & VCSR_MASK_VXRM);  
> +  regbuf = (gdb_byte *) &vxrm;
> +  supply_register_by_name (regcache, "vxrm", regbuf);
> +
> +  /* v0..v31 */
> +  regbuf = (const gdb_byte *) buf + offsetof (struct __riscv_vregs, data);
> +  for (i = 0; i < 32; i++, regbuf += vlenb)
> +    supply_register (regcache, regno + i, regbuf);
> +}
> +
>  /* RISC-V/Linux regsets.  FPRs are optional and come in different sizes,
>     so define multiple regsets for them marking them all as OPTIONAL_REGS
>     rather than FP_REGS, so that "regsets_fetch_inferior_registers" picks
> @@ -175,6 +282,9 @@ static struct regset_info riscv_regsets[] = {
>    { PTRACE_GETREGSET, PTRACE_SETREGSET, NT_FPREGSET,
>      sizeof (struct __riscv_mc_f_ext_state), OPTIONAL_REGS,
>      riscv_fill_fpregset, riscv_store_fpregset },
> +  { PTRACE_GETREGSET, PTRACE_SETREGSET, NT_RISCV_VECTOR,
> +    sizeof (struct __riscv_vregs), OPTIONAL_REGS,
> +    riscv_fill_vregset, riscv_store_vregset },
>    NULL_REGSET
>  };
>  
> -- 
> 2.25.1

Thanks,
Andrew

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

[Greg Savin]

[-- Attachment #1: Type: text/plain, Size: 724 bytes --]

> Based on some of the other emails, I guess kernel support for some of
> this is still WIP, so I guess there's not "easy" way to test this?  Does
> QEMU have vector support yet?
>
> I've used a QEMU build that models RISC-V vectors, and tried out this GDB
patch interactively (mostly via "info reg vector" but also accesses through
dollar sign syntax with specific register values, reading and writing).
Though the QEMU I use for that is from a non-upstream fork, and I'm not
sure if upstream QEMU has all the capabilities for running RISCV-V vector
configurations yet.  I'll ask the people who provided the QEMU build I've
been using.

Thanks Andrew, I'll take a closer look at your other review comments on
this.

--Greg

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

[Maciej W. Rozycki]

On Thu, 3 Aug 2023, Greg Savin via Gdb-patches wrote:

> +static unsigned long
> +safe_read_vlenb ()
> +{
> +  /* Surrounding the attempt here to read VLENB CSR to have a signal handler set up
> +     to trap illegal instruction condition (SIGILL), and if a trap happens during this call,
> +     get control back within this function and return 0 in that case.
> +   */
> +  unsigned long vlenb = 0;
> +  struct sigaction our_action = { 0 };
> +  struct sigaction original_action;
> +  int sysresult;
> +
> +
> +  our_action.sa_handler = sigill_guard;
> +
> +  sysresult = sigaction (SIGILL, &our_action, &original_action);
> +  if (sysresult != 0)
> +    {
> +      perror
> +	("Error installing temporary SIGILL handler in safe_read_vlenb()");
> +    }
> +
> +  if (SIGSETJMP (sigill_guard_jmp_buf, 1) == 0)
> +    {
> +    asm ("csrr %0, vlenb":"=r" (vlenb));
> +    }
> +  else
> +    {
> +      /* Must've generated an illegal instruction condition; we'll figure this means
> +         no vector unit is present */
> +      vlenb = 0;
> +    }

 I find it weird that you trap SIGILL and try to execute a vector 
instruction in the debugger to determine whether `ptrace' can be used to 
access the vector state in the debuggee.  Why?

 The usual way is to try to use `ptrace' itself to determine whether the 
OS has support for it in the first place and then can access the vector 
state.  You can then return the contents of the register retrieved if 
successful.

  Maciej

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

[Greg Savin]

[-- Attachment #1: Type: text/plain, Size: 2445 bytes --]

Hi Maciej,

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.

Regards,
Greg


On Wed, Aug 9, 2023 at 2:21 AM Maciej W. Rozycki <macro@orcam.me.uk> wrote:

> On Thu, 3 Aug 2023, Greg Savin via Gdb-patches wrote:
>
> > +static unsigned long
> > +safe_read_vlenb ()
> > +{
> > +  /* Surrounding the attempt here to read VLENB CSR to have a signal
> handler set up
> > +     to trap illegal instruction condition (SIGILL), and if a trap
> happens during this call,
> > +     get control back within this function and return 0 in that case.
> > +   */
> > +  unsigned long vlenb = 0;
> > +  struct sigaction our_action = { 0 };
> > +  struct sigaction original_action;
> > +  int sysresult;
> > +
> > +
> > +  our_action.sa_handler = sigill_guard;
> > +
> > +  sysresult = sigaction (SIGILL, &our_action, &original_action);
> > +  if (sysresult != 0)
> > +    {
> > +      perror
> > +     ("Error installing temporary SIGILL handler in safe_read_vlenb()");
> > +    }
> > +
> > +  if (SIGSETJMP (sigill_guard_jmp_buf, 1) == 0)
> > +    {
> > +    asm ("csrr %0, vlenb":"=r" (vlenb));
> > +    }
> > +  else
> > +    {
> > +      /* Must've generated an illegal instruction condition; we'll
> figure this means
> > +         no vector unit is present */
> > +      vlenb = 0;
> > +    }
>
>  I find it weird that you trap SIGILL and try to execute a vector
> instruction in the debugger to determine whether `ptrace' can be used to
> access the vector state in the debuggee.  Why?
>
>  The usual way is to try to use `ptrace' itself to determine whether the
> OS has support for it in the first place and then can access the vector
> state.  You can then return the contents of the register retrieved if
> successful.
>
>   Maciej
>

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

[Maciej W. Rozycki]

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

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

[Andy Chiu]

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

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

[Maciej W. Rozycki]

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

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

[Maciej W. Rozycki]

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

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

[Andy Chiu]

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

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

[Palmer Dabbelt]

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

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

[Maciej W. Rozycki]

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

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

[Andy Chiu]

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

Re: [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

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

[Maciej W. Rozycki]

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