This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 6/8] AARCH64 SVE: Add SVE register support


Thanks for the review!

On 13/12/2016 11:26, "Yao Qi" <qiyaoltc@gmail.com> wrote:

>On 16-12-05 12:29:22, Alan Hayward wrote:
>> diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
>> index 
>> 
>>65ca8ff2a7ff25b16e31135f4a8b11b6b013dae3..5525b0dd46a1bb641f4b60039426134
>>37
>> 202e785 100644
>> --- a/gdb/aarch64-linux-nat.c
>> +++ b/gdb/aarch64-linux-nat.c
>> @@ -49,6 +49,129 @@
>>  #define TRAP_HWBKPT 0x0004
>>  #endif
>> 
>> +#define sve_vg_from_vl(vl)	((vl) / 8)
>> +#define sve_vl_from_vg(vg)	((vg) * 8)
>
>All these macros and struct are duplicated in gdb/aarch64-linux-nat.c
>and gdb/gdbserver/linux-aarch64-low.c.  We can add a new file
>gdb/nat/aarch64-sve-linux.c and put them there.

Ok.

>
>> +
>> +/* Structures and defines taken from sigcontext.h.
>> +   These can be removed when kernel changes are fully committed.  */
>
>I am not sure we can remove them after kernel changes are fully
>committed, because sve-aware toolchain can be built with old kernel
>headers, no?

I was unsure about this. Does gdb have a “minimum” kernel version it needs
to build against? Otherwise how can any of the gdb code rely on defines
from
kernel headers. I noticed that other parts of gdb had similar
redefinitions 
of other defines.

>
>> +#ifndef SVE_SIG_ZREGS_SIZE
>> +
>> +struct sve_context {
>
>"{" should be in the next line.

All of these defines between the SVE_SIG_ZREGS_SIZE check and the
SVE_PT_SVE_ZREG_SIZE check are directly copied from the Linux kernel
header. For this reason, I purposefully didn’t want to reformat any of this
code to GNU standards or add additional comments, and I would be very wary
of changing types to use gdb defined ones (e.g. uint16 instead of __u16).
I certainly wouldn't want to change macros to functions or combine them
etc. The defines themselves are currently under review on the Linux kernel
mailing lists.


Would it be ok to put them all into a separate file and change the
comment to something like:
/* Structures and defines directly copied from ptrace.h.
   Refer to Linux kernel documentation for details.
   Eventually this section should be removed.  */


>
>> +	struct _aarch64_ctx head;
>
>indentation is wrong.
>
>> +	__u16 vl;
>> +	__u16 __reserved[3];
>
>where is __u16 defined?
>
>> +};
>> +
>> +#define SVE_VQ_MIN		1
>> +#define SVE_VQ_MAX		0x200
>
>What does this mean?
>
>> +#define SVE_SIG_CONTEXT_SIZE(vq) (SVE_SIG_REGS_OFFSET +
>> SVE_SIG_REGS_SIZE(vq))
>> +
>
>All these macros don't have comments to explain them.
>
>> +/* Structures and defines taken from ptrace.h.
>> +   These can be removed when kernel changes are fully committed.  */
>> +#ifndef SVE_PT_SVE_ZREG_SIZE
>> +
>> +struct user_sve_header {
>> +	__u32 size; /* total meaningful regset content in bytes */
>> +	__u32 max_size; /* maxmium possible size for this thread */
>> +	__u16 vl; /* current vector length */
>> +	__u16 max_vl; /* maximum possible vector length */
>> +	__u16 flags;
>> +	__u16 __reserved;
>
>We don't use __u{16,32} in GDB, use uint{16,32}_t instead.
>
>> +};
>> +
>> +#define SVE_PT_REGS_MASK		(1 << 0)
>> +#define SVE_PT_REGS_FPSIMD		0
>> +#define SVE_PT_REGS_SVE			SVE_PT_REGS_MASK
>> +
>> +#define SVE_PT_REGS_OFFSET	((sizeof (struct sve_context) + 15) / 16 *
>>16)
>> +
>> +#define SVE_PT_FPSIMD_OFFSET		SVE_PT_REGS_OFFSET
>> +
>> +#define SVE_PT_FPSIMD_SIZE(vq, flags)	(sizeof(struct
>>user_fpsimd_state))
>> +
>> +#define SVE_PT_SVE_ZREG_SIZE(vq)	SVE_SIG_ZREG_SIZE(vq)
>> +#define SVE_PT_SVE_PREG_SIZE(vq)	SVE_SIG_PREG_SIZE(vq)
>> +#define SVE_PT_SVE_FFR_SIZE(vq)		SVE_SIG_FFR_SIZE(vq)
>> +#define SVE_PT_SVE_FPSR_SIZE		sizeof(__u32)
>> +#define SVE_PT_SVE_FPCR_SIZE		sizeof(__u32)
>> +
>> +#define __SVE_SIG_TO_PT(offset) \
>> +	((offset) - SVE_SIG_REGS_OFFSET + SVE_PT_REGS_OFFSET)
>> +
>> +#define SVE_PT_SVE_OFFSET		SVE_PT_REGS_OFFSET
>> +
>> +#define SVE_PT_SVE_ZREGS_OFFSET \
>> +	__SVE_SIG_TO_PT(SVE_SIG_ZREGS_OFFSET)
>> +#define SVE_PT_SVE_ZREG_OFFSET(vq, n) \
>> +	__SVE_SIG_TO_PT(SVE_SIG_ZREG_OFFSET(vq, n))
>> +#define SVE_PT_SVE_ZREGS_SIZE(vq) \
>> +	(SVE_PT_SVE_ZREG_OFFSET(vq, SVE_NUM_ZREGS) - SVE_PT_SVE_ZREGS_OFFSET)
>> +
>> +#define SVE_PT_SVE_PREGS_OFFSET(vq) \
>> +	__SVE_SIG_TO_PT(SVE_SIG_PREGS_OFFSET(vq))
>> +#define SVE_PT_SVE_PREG_OFFSET(vq, n) \
>> +	__SVE_SIG_TO_PT(SVE_SIG_PREG_OFFSET(vq, n))
>> +#define SVE_PT_SVE_PREGS_SIZE(vq) \
>> +	(SVE_PT_SVE_PREG_OFFSET(vq, SVE_NUM_PREGS) - \
>> +		SVE_PT_SVE_PREGS_OFFSET(vq))
>> +
>> +#define SVE_PT_SVE_FFR_OFFSET(vq) \
>> +	__SVE_SIG_TO_PT(SVE_SIG_FFR_OFFSET(vq))
>> +
>> +#define SVE_PT_SVE_FPSR_OFFSET(vq) \
>> +	((SVE_PT_SVE_FFR_OFFSET(vq) + SVE_PT_SVE_FFR_SIZE(vq) + 15) / 16 * 16)
>> +#define SVE_PT_SVE_FPCR_OFFSET(vq) \
>> +	(SVE_PT_SVE_FPSR_OFFSET(vq) + SVE_PT_SVE_FPSR_SIZE)
>> +
>> +#define SVE_PT_SVE_SIZE(vq, flags)				\
>> +	((SVE_PT_SVE_FPCR_OFFSET(vq) + SVE_PT_SVE_FPCR_SIZE -	\
>> +		SVE_PT_SVE_OFFSET + 15) / 16 * 16)
>> +
>> +#define SVE_PT_SIZE(vq, flags)						\
>> +	 (((flags) & SVE_PT_REGS_MASK) == SVE_PT_REGS_SVE ?		\
>> +		  SVE_PT_SVE_OFFSET + SVE_PT_SVE_SIZE(vq, flags)	\
>> +		: SVE_PT_FPSIMD_OFFSET + SVE_PT_FPSIMD_SIZE(vq, flags))
>> +
>
>Any reason to use some many macros?  At least SVE_PT_SIZE should be
>defined as a function rather than macro.

(see previous comment for all the above)


>
>> +#endif
>> +
>>  /* Per-process data.  We don't bind this to a per-inferior registry
>>     because of targets like x86 GNU/Linux that need to keep track of
>>     processes that aren't bound to any inferior (e.g., fork children,
>> @@ -342,6 +465,266 @@ store_fpregs_to_thread (const struct regcache
>> *regcache)
>>      }
>>  }
>> 
>> +/* Fill GDB's register array with the sve register values
>> +   from the current thread.  */
>> +
>> +static void
>> +fetch_sveregs_from_thread (struct regcache *regcache, int regno)
>
>REGNO is not used.

I’ll remove. It is left over from an older version of the patch.

>
>> +{
>> +  int ret, tid, i;
>> +  struct iovec iovec;
>> +  struct gdbarch *gdbarch = get_regcache_arch (regcache);
>> +  struct user_sve_header header;
>> +  long vg, vq;
>> +  char *base;
>> +
>> +  tid = ptid_get_lwp (inferior_ptid);
>> +
>> +  /* First call ptrace to get a header.  */
>> +
>> +  iovec.iov_len = sizeof (header);
>> +  iovec.iov_base = &header;
>> +
>> +  ret = ptrace (PTRACE_GETREGSET, tid, NT_ARM_SVE, &iovec);
>> +  if (ret < 0)
>> +    perror_with_name (_("Unable to fetch sve register header"));
>> +
>> +  gdb_assert (sve_vl_valid (header.vl));
>> +  vg = sve_vg_from_vl (header.vl);
>> +  vq = sve_vq_from_vl (header.vl);
>> +  gdb_assert (SVE_PT_SIZE (vq, header.flags) == header.size);
>
>Comments are needed.

Ok.

>
>> +  regcache_raw_supply (regcache, AARCH64_SVE_VG_REGNUM, &vg);
>> +
>> +  /* Call again using the full buffer size.  */
>> +
>> +  iovec.iov_len = header.size;
>> +  iovec.iov_base = xmalloc (iovec.iov_len);
>> +  base = (char*)iovec.iov_base;
>
> = (char *) iovec.iov_base

Ok.

>
>> +
>
>Please add comments that thread may have or may not have SVE state.


Ok.

>
>> +  if (header.flags && SVE_PT_REGS_SVE)
>
>Looks (header.flags && SVE_PT_REGS_SVE) is widely used, can you add
>a macro like,
>
>#define HAS_SVE_STATE(header) header.flags && SVE_PT_REGS_SVE
>
>that helps to improve the code readability.

Ok.

>
>> +    {
>> +      ret = ptrace (PTRACE_GETREGSET, tid, NT_ARM_SVE, &iovec);
>> +      if (ret < 0)
>> +	perror_with_name (_("Unable to fetch sve registers"));
>> +
>> +      for (i = 0; i <= AARCH64_SVE_Z31_REGNUM - AARCH64_SVE_Z0_REGNUM;
>> i++)
>> +	regcache_raw_supply (regcache, AARCH64_SVE_Z0_REGNUM + i,
>> +			     base + SVE_PT_SVE_ZREG_OFFSET (vq, i));
>> +
>> +      for (i = 0; i <= AARCH64_SVE_P15_REGNUM - AARCH64_SVE_P0_REGNUM;
>> i++)
>> +	regcache_raw_supply (regcache, AARCH64_SVE_P0_REGNUM + i,
>> +			     base + SVE_PT_SVE_PREG_OFFSET (vq, i));
>> +
>> +      regcache_raw_supply (regcache, AARCH64_SVE_FFR_REGNUM,
>> +			   base + SVE_PT_SVE_FFR_OFFSET (vq));
>> +      regcache_raw_supply (regcache, AARCH64_FPSR_REGNUM,
>> +			   base + SVE_PT_SVE_FPSR_OFFSET (vq));
>> +      regcache_raw_supply (regcache, AARCH64_FPCR_REGNUM,
>> +			   base + SVE_PT_SVE_FPCR_OFFSET (vq));
>
>Could you draw an ascii diagram about the registers layout in iovec?
>If the layout is different between "thread has SVE state" and thread
>doesn't have SVE state", we need two.

There is already a diagram in the linux ptrace header - I can add that
when I copy the defines across.

>
>> +    }
>> +  else
>> +    {
>> +      /* There is no SVE state.  Read fpsimd instead.  */
>> +      char sve_reg[SVE_PT_SVE_ZREG_SIZE (vq)];
>> +      struct user_fpsimd_state *fpsimd
>> +	= (struct user_fpsimd_state *)(base + SVE_PT_FPSIMD_OFFSET);
>> +
>> +      ret = ptrace (PTRACE_GETREGSET, tid, NT_ARM_SVE, &iovec);
>
>Please add comments on why do you use NT_ARM_SVE to get fpsimd, rather
>than NT_FPREGSET.
>
>> +      if (ret < 0)
>> +	perror_with_name (_("Unable to fetch fpsimd registers via sve"));
>> +
>> +      /* Clear the SVE only registers.  */
>> +
>> +      memset (&sve_reg, 0, SVE_PT_SVE_ZREG_SIZE (vq));
>> +
>> +      for (i = 0; i <= AARCH64_SVE_P15_REGNUM - AARCH64_SVE_P0_REGNUM;
>> i++)
>> +	regcache_raw_supply (regcache, AARCH64_SVE_P0_REGNUM + i, &sve_reg);
>
>This looks wrong to me.  If there is no SVE state, the target
>description for the regcache is the one for normal aarch64 (without sve),
>we should call regcache_raw_supply for normal aarch64 V registers.
>AARCH64_SVE_P0_REGNUM is unknown to regcache.
>
>Or I suspect it is dead code because fetch_sveregs_from_thread is guarded
>by AARCH64_TDEP_HAS_SVE, and AARCH64_TDEP_HAS_SVE is false when the thread
>doesn't have SVE state?

If the kernel returns a structure for NT_ARM_SVE, but there is no SVE state
in it then this means that SVE is supported, but the current process has
not yet executed any SVE code. The SVE registers exist but the contents are
not yet defined.
As soon as the code hits an SVE instruction, then SVE state will be
created,
and the next NT_ARM_SVE call will return an SVE structure.


If NT_ARM_SVE returns a valid structure (either sve or no sve) then the gdb
target description will always have SVE registers. If there is no SVE state
in the kernel then gdb still needs to show SVE registers - the registers
still exist but there is currently no state for them.
When in this state it is also perfectly valid for the gdb user to manually
set an SVE register, which would cause gdb to then write a full SVE state
to the kernel.

>
>> +
>> +      regcache_raw_supply (regcache, AARCH64_SVE_FFR_REGNUM, &sve_reg);
>> +
>> +      /* Convert vregs from fpsimd state to zregs, ensuring additional
>> state is
>> +	 null.  */
>> +
>> +      for (i = 0; i <= AARCH64_SVE_Z31_REGNUM - AARCH64_SVE_Z0_REGNUM;
>> i++)
>> +	{
>> +	  memcpy (&sve_reg, &fpsimd->vregs[i], sizeof (__int128));
>> +	  regcache_raw_supply (regcache, AARCH64_SVE_Z0_REGNUM + i, &sve_reg);
>> +	}
>> +
>> +      regcache_raw_supply (regcache, AARCH64_FPSR_REGNUM,
>>&fpsimd->fpsr);
>> +      regcache_raw_supply (regcache, AARCH64_FPCR_REGNUM,
>>&fpsimd->fpcr);
>> +    }
>> +
>> +  free (iovec.iov_base);
>
>Xfree


Ok.

>
>> +}
>> +
>> +/* Store to the current thread the valid sve register
>> +   values in the GDB's register array.  */
>> +
>> +static void
>> +store_sveregs_to_thread (const struct regcache *regcache, int regno)
>> +{
>> +  int ret, tid, r;
>> +  struct iovec iovec;
>> +  struct gdbarch *gdbarch = get_regcache_arch (regcache);
>> +  struct user_sve_header header;
>> +  long vq;
>> +  char *sve_reg_p, *base;
>> +
>> +  tid = ptid_get_lwp (inferior_ptid);
>> +
>> +  /* First call ptrace to get a header.  */
>> +
>> +  iovec.iov_len = sizeof (header);
>> +  iovec.iov_base = &header;
>> +
>> +  ret = ptrace (PTRACE_GETREGSET, tid, NT_ARM_SVE, &iovec);
>> +  if (ret < 0)
>> +    perror_with_name (_("Unable to fetch sve register header"));
>> +
>> +  gdb_assert (sve_vl_valid (header.vl));
>> +  vq = sve_vq_from_vl (header.vl);
>> +  gdb_assert (SVE_PT_SIZE (vq, header.flags) == header.size);
>> +
>> +  /* Get the full structure.  */
>> +
>> +  if (header.flags && SVE_PT_REGS_SVE)
>> +    iovec.iov_len = header.size;
>> +  else
>> +    iovec.iov_len = SVE_PT_SIZE (vq, SVE_PT_REGS_SVE);
>
>Why do you sent iov_len this way when the thread doesn't have SVE
>state?

It needs a comment :)
Need make sure there is enough buffer state to write out a full SVE
structure. It would probably be simpler to just have the “else” case.


>
>> +  iovec.iov_base = xmalloc (iovec.iov_len);
>> +  base = (char*)iovec.iov_base;
>> +
>> +  ret = ptrace (PTRACE_GETREGSET, tid, NT_ARM_SVE, &iovec);
>> +  if (ret < 0)
>> +    perror_with_name (_("Unable to fetch sve registers before
>>storing"));
>> +
>> +  if (!(header.flags && SVE_PT_REGS_SVE))
>> +    {
>> +      /* There is no SVE state.  Try to put the registers into an
>>fpsimd
>> +	 structure, whilst checking to see if there is any actual SVE
>> +	 state.  */
>
>It is confusing to me when I read for the first time.
>
>        /* There is no SVE state in current thread.  Try to put the
>	   registers into an fpsimd structure, whilst checking to see
>	   if there is any SVE state in REGCACHE.  */
>
>Again, this function is guarded by AARCH64_TDEP_HAS_SVE, how is this
>path reachable?

As mentioned before, this is because the process has no yet executed any
SVE instructions.

>
>> +      int has_sve_state = 0;
>> +      char sve_reg[SVE_PT_SVE_ZREG_SIZE (vq)];
>
>Is it a variable length array?  We should use alloca instead.  Maybe, it
>is more clear to name it as sve_reg_zero.

VQ won’t change during the function. GCC was ok compiling this!

>
>> +      struct user_fpsimd_state *fpsimd
>> +	= (struct user_fpsimd_state *)(base + SVE_PT_FPSIMD_OFFSET);
>> +
>> +      memset (&sve_reg, 0, SVE_PT_SVE_ZREG_SIZE (vq));
>> +
>> +      /* Check if any SVE state exists in the regcache.  */
>> +
>> +      for (r = AARCH64_SVE_Z0_REGNUM; r <= AARCH64_SVE_Z31_REGNUM; r++)
>> +	{
>> +	  has_sve_state |= regcache_raw_compare (regcache, r, &sve_reg,
>> +						 sizeof (__int128));
>
>Why don't you use regcache_raw_read (regcache, r, buf), and check bits
>after 128 are zero not?  That is more clear, IMO, and avoid adding
>regcache_raw_compare.

Using regcache_raw_read I would have to copy the contents of the register
into a buffer then memcmp against sve_reg_zero.
regcache_raw_compare just does a memcmp without needing to copy.

Given I have to do this for all SVE registers (32+16+1) it seemed better
To add the new function.

>
>Further, we don't have to compare each bytes in regcache to determine
>REGCACHE has SVE state or not.  Instead, we can detect whether
>REGCACHE has P registers or not.
>

What I am doing here is for the case when kernel has not yet executed any
SVE code. If the gdb user has not manually written to any of the SVE
registers then we want to just write a fpsimd structure (because we
don’t want to “enable” sve state in the kernel - even if it is just
writing null state).
However, if the user has manually set an SVE register then we should
write SVE state back.

>
>> +	  if (has_sve_state != 0)
>> +	    break;
>> +	}
>> +
>> +      if (has_sve_state == 0)
>> +	for (r = AARCH64_SVE_P0_REGNUM; r <= AARCH64_SVE_P15_REGNUM; r++)
>> +	  {
>> +	    has_sve_state |= regcache_raw_compare (regcache, r, &sve_reg, 0);
>> +	    if (has_sve_state != 0)
>> +	      break;
>> +	  }
>> +
>> +      if (has_sve_state == 0)
>> +	  has_sve_state |= regcache_raw_compare (regcache,
>> +						 AARCH64_SVE_FFR_REGNUM,
>> +						 &sve_reg, 0);
>> +
>> +      /* If no SVE state then copy into a fpsimd structure.  */
>> +
>> +      if (has_sve_state == 0)
>> +	{
>> +	  for (r = AARCH64_SVE_Z0_REGNUM; r <= AARCH64_SVE_Z31_REGNUM; r++)
>> +	    {
>> +	      if (REG_VALID == regcache_register_status (regcache, r))
>> +		regcache_raw_collect (
>> +		  regcache, r, &fpsimd->vregs[r - AARCH64_SVE_Z0_REGNUM]);
>> +	    }
>> +
>> +	  regcache_raw_collect (regcache, AARCH64_FPSR_REGNUM, &fpsimd->fpsr);
>> +	  regcache_raw_collect (regcache, AARCH64_FPCR_REGNUM, &fpsimd->fpcr);
>> +
>> +	  ret = ptrace (PTRACE_SETREGSET, tid, NT_ARM_SVE, &iovec);
>> +
>> +	  xfree (iovec.iov_base);
>> +
>> +	  if (ret < 0)
>> +	   perror_with_name (_("Unable to store sve registers using fpsimd"));
>> +
>> +	  return;
>> +	}
>> +
>> +      /* Expand the current kernel state into an SVE set (with null
>> values for
>> +	 the P and FFR regs) and set that as the current iov_base.  */
>> +
>> +      int sve_size = SVE_PT_SIZE (vq, SVE_PT_REGS_SVE);
>> +      char *sve_buf = (char*)xmalloc (sve_size);
>
> = (gdb_byte *) xmalloc (sve_size);

Ok

>
>> +      struct user_sve_header *sve_header = (struct user_sve_header
>> *)sve_buf;
>> +
>> +      memcpy (sve_buf, iovec.iov_base, sizeof (struct
>>user_sve_header));
>> +      sve_header->size = sve_size;
>> +      sve_header->flags |= SVE_PT_REGS_SVE;
>> +
>> +      for (r = 0; r <= AARCH64_SVE_Z31_REGNUM - AARCH64_SVE_Z0_REGNUM;
>> r++)
>> +	{
>> +	  memcpy (sve_buf + SVE_PT_SVE_ZREG_OFFSET (vq, r),
>> +		  &fpsimd->vregs[r], sizeof (__int128));
>
>s/__int128/int128_t/

Ok

>
>> +	}
>> +
>> +      memcpy (sve_buf + SVE_PT_SVE_FPSR_OFFSET (vq), &fpsimd->fpsr,
>> +	      sizeof (uint32_t));
>> +      memcpy (sve_buf + SVE_PT_SVE_FPCR_OFFSET (vq), &fpsimd->fpcr,
>> +	      sizeof (uint32_t));
>> +
>> +      xfree (iovec.iov_base);
>> +      iovec.iov_base = sve_buf;
>> +      iovec.iov_len = sve_size;
>> +      base = (char*)iovec.iov_base;
>
> = (gdb_byte *) iovec.iov_base;

Ok

>
>> +    }
>> +
>> +  /* Replace the kernel values with those from regcache.  */
>> +
>> +  sve_reg_p = base + SVE_PT_SVE_ZREGS_OFFSET;
>> +  for (r = AARCH64_SVE_Z0_REGNUM; r <= AARCH64_SVE_Z31_REGNUM; r++)
>> +    {
>> +      if (REG_VALID == regcache_register_status (regcache, r))
>> +	regcache_raw_collect (regcache, r, sve_reg_p);
>> +      sve_reg_p += SVE_PT_SVE_ZREG_SIZE (vq);
>> +    }
>> +
>> +  sve_reg_p = base + SVE_PT_SVE_PREGS_OFFSET (vq);
>> +  for (r = AARCH64_SVE_P0_REGNUM; r <= AARCH64_SVE_P15_REGNUM; r++)
>> +    {
>> +      if (REG_VALID == regcache_register_status (regcache, r))
>> +	regcache_raw_collect (regcache, r, sve_reg_p);
>> +      sve_reg_p += SVE_PT_SVE_PREG_SIZE (vq);
>> +    }
>> +
>> +  if (REG_VALID == regcache_register_status (regcache,
>> AARCH64_SVE_FFR_REGNUM))
>> +    regcache_raw_collect (regcache, AARCH64_SVE_FFR_REGNUM,
>> +			  base + SVE_PT_SVE_FFR_OFFSET (vq));
>> +  if (REG_VALID == regcache_register_status (regcache,
>> AARCH64_FPSR_REGNUM))
>> +    regcache_raw_collect (regcache, AARCH64_FPSR_REGNUM,
>> +			  base + SVE_PT_SVE_FPSR_OFFSET (vq));
>> +  if (REG_VALID == regcache_register_status (regcache,
>> AARCH64_FPCR_REGNUM))
>> +    regcache_raw_collect (regcache, AARCH64_FPCR_REGNUM,
>> +			  base + SVE_PT_SVE_FPCR_OFFSET (vq));
>> +
>> +  /* Note: We do not set VL.  Changing VL on the fly is currently
>> unsupported
>> +     in Linux.  */
>> +
>> +  ret = ptrace (PTRACE_SETREGSET, tid, NT_ARM_SVE, &iovec);
>> +
>> +  xfree (iovec.iov_base);
>> +
>> +  if (ret < 0)
>> +    perror_with_name (_("Unable to store sve registers"));
>> +}
>> +
>>  /* Implement the "to_fetch_register" target_ops method.  */
>
>> 
>> +/* The SVE 'Z' and 'P' registers.  */
>> +static const char *const aarch64_sve_register_names[] =
>> +{
>> +  /* These registers must appear in consecutive RAW register number
>> +     order and they must begin with AARCH64_SVE_Z0_REGNUM! */
>> +  "z0", "z1", "z2", "z3",
>> +  "z4", "z5", "z6", "z7",
>> +  "z8", "z9", "z10", "z11",
>> +  "z12", "z13", "z14", "z15",
>> +  "z16", "z17", "z18", "z19",
>> +  "z20", "z21", "z22", "z23",
>> +  "z24", "z25", "z26", "z27",
>> +  "z28", "z29", "z30", "z31",
>> +  "fpsr", "fpcr",
>> +  "p0", "p1", "p2", "p3",
>> +  "p4", "p5", "p6", "p7",
>> +  "p8", "p9", "p10", "p11",
>> +  "p12", "p13", "p14", "p15",
>> +  "ffr", "vg"
>> +};
>> +
>>  /* AArch64 prologue cache structure.  */
>>  struct aarch64_prologue_cache
>>  {
>> @@ -1554,11 +1576,40 @@ aarch64_vnb_type (struct gdbarch *gdbarch)
>>    return tdep->vnb_type;
>>  }
>> 
>> +/* Return the type for an AdvSISD V register.  */
>> +
>> +static struct type *
>> +aarch64_vnv_type (struct gdbarch *gdbarch)
>> +{
>> +  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
>> +
>> +  if (tdep->vnv_type == NULL)
>> +    {
>> +      struct type *t;
>> +      struct type *elem;
>> +
>> +      t = arch_composite_type (gdbarch, "__gdb_builtin_type_vnv",
>> +			       TYPE_CODE_UNION);
>> +
>> +      append_composite_type_field (t, "d", aarch64_vnd_type (gdbarch));
>> +      append_composite_type_field (t, "s", aarch64_vns_type (gdbarch));
>> +      append_composite_type_field (t, "h", aarch64_vnh_type (gdbarch));
>> +      append_composite_type_field (t, "b", aarch64_vnb_type (gdbarch));
>> +      append_composite_type_field (t, "q", aarch64_vnq_type (gdbarch));
>> +
>> +      tdep->vnv_type = t;
>> +    }
>> +
>> +  return tdep->vnv_type;
>> +}
>> +
>>  /* Implement the "dwarf2_reg_to_regnum" gdbarch method.  */
>> 
>>  static int
>>  aarch64_dwarf_reg_to_regnum (struct gdbarch *gdbarch, int reg)
>>  {
>> +  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
>> +
>>    if (reg >= AARCH64_DWARF_X0 && reg <= AARCH64_DWARF_X0 + 30)
>>      return AARCH64_X0_REGNUM + reg - AARCH64_DWARF_X0;
>> 
>> @@ -1568,6 +1619,21 @@ aarch64_dwarf_reg_to_regnum (struct gdbarch
>> *gdbarch, int reg)
>>    if (reg >= AARCH64_DWARF_V0 && reg <= AARCH64_DWARF_V0 + 31)
>>      return AARCH64_V0_REGNUM + reg - AARCH64_DWARF_V0;
>> 
>> +  if (AARCH64_TDEP_HAS_SVE (tdep))
>> +    {
>
>This check is not needed, or it is wrong to do the check.
>Since there is no overlap on DWARF register numbers, we can do SVE
>registers mapping without checking AARCH64_TDEP_HAS_SVE.

Don’t think this will cause any harm, but I will remove the check.

>
>> +      if (reg == AARCH64_DWARF_SVE_VG)
>> +	return AARCH64_SVE_VG_REGNUM;
>> +
>> +      if (reg == AARCH64_DWARF_SVE_FFR)
>> +	return AARCH64_SVE_FFR_REGNUM;
>> +
>> +      if (reg >= AARCH64_DWARF_SVE_P0 && reg <= AARCH64_DWARF_SVE_P0 +
>>15)
>> +	return AARCH64_SVE_P0_REGNUM + reg - AARCH64_DWARF_SVE_P0;
>> +
>> +      if (reg >= AARCH64_DWARF_SVE_Z0 && reg <= AARCH64_DWARF_SVE_Z0 +
>>15)
>> +	return AARCH64_SVE_Z0_REGNUM + reg - AARCH64_DWARF_SVE_Z0;
>> +    }
>> +
>>    return -1;
>>  }
>> 
>> 
>> @@ -1902,6 +1968,8 @@ aarch64_gen_return_address (struct gdbarch
>>*gdbarch,
>>  static const char *
>>  aarch64_pseudo_register_name (struct gdbarch *gdbarch, int regnum)
>>  {
>> +  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
>> +
>>    static const char *const q_name[] =
>>      {
>>        "q0", "q1", "q2", "q3",
>> @@ -1979,6 +2047,25 @@ aarch64_pseudo_register_name (struct gdbarch
>> *gdbarch, int regnum)
>>    if (regnum >= AARCH64_B0_REGNUM && regnum < AARCH64_B0_REGNUM + 32)
>>      return b_name[regnum - AARCH64_B0_REGNUM];
>> 
>> +  if (AARCH64_TDEP_HAS_SVE (tdep))
>> +    {
>> +      static const char *const sve_v_name[] =
>> +	{
>> +	  "v0", "v1", "v2", "v3",
>> +	  "v4", "v5", "v6", "v7",
>> +	  "v8", "v9", "v10", "v11",
>> +	  "v12", "v13", "v14", "v15",
>> +	  "v16", "v17", "v18", "v19",
>> +	  "v20", "v21", "v22", "v23",
>> +	  "v24", "v25", "v26", "v27",
>> +	  "v28", "v29", "v30", "v31",
>> +	};
>> +
>> +      if (regnum >= AARCH64_SVE_V0_REGNUM
>> +	  && regnum < AARCH64_SVE_V0_REGNUM + 32)
>> +	return sve_v_name[regnum - AARCH64_SVE_V0_REGNUM];
>> +    }
>> +
>>    internal_error (__FILE__, __LINE__,
>>  		  _("aarch64_pseudo_register_name: bad register number %d"),
>>  		  regnum);
>> @@ -1989,6 +2076,8 @@ aarch64_pseudo_register_name (struct gdbarch
>> *gdbarch, int regnum)
>>  static struct type *
>>  aarch64_pseudo_register_type (struct gdbarch *gdbarch, int regnum)
>>  {
>> +  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
>> +
>>    regnum -= gdbarch_num_regs (gdbarch);
>> 
>>    if (regnum >= AARCH64_Q0_REGNUM && regnum < AARCH64_Q0_REGNUM + 32)
>> @@ -2006,6 +2095,10 @@ aarch64_pseudo_register_type (struct gdbarch
>> *gdbarch, int regnum)
>>    if (regnum >= AARCH64_B0_REGNUM && regnum < AARCH64_B0_REGNUM + 32)
>>      return aarch64_vnb_type (gdbarch);
>> 
>> +  if (AARCH64_TDEP_HAS_SVE (tdep) && regnum >= AARCH64_SVE_V0_REGNUM
>> +      && regnum < AARCH64_SVE_V0_REGNUM + 32)
>> +    return aarch64_vnv_type (gdbarch);
>> +
>>    internal_error (__FILE__, __LINE__,
>>  		  _("aarch64_pseudo_register_type: bad register number %d"),
>>  		  regnum);
>> @@ -2017,6 +2110,8 @@ static int
>>  aarch64_pseudo_register_reggroup_p (struct gdbarch *gdbarch, int
>>regnum,
>>  				    struct reggroup *group)
>>  {
>> +  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
>> +
>>    regnum -= gdbarch_num_regs (gdbarch);
>> 
>>    if (regnum >= AARCH64_Q0_REGNUM && regnum < AARCH64_Q0_REGNUM + 32)
>> @@ -2031,6 +2126,9 @@ aarch64_pseudo_register_reggroup_p (struct gdbarch
>> *gdbarch, int regnum,
>>      return group == all_reggroup || group == vector_reggroup;
>>    else if (regnum >= AARCH64_B0_REGNUM && regnum < AARCH64_B0_REGNUM +
>>32)
>>      return group == all_reggroup || group == vector_reggroup;
>> +  else if (AARCH64_TDEP_HAS_SVE (tdep) && regnum >=
>>AARCH64_SVE_V0_REGNUM
>> +	   && regnum < AARCH64_SVE_V0_REGNUM + 32)
>> +    return group == all_reggroup || group == vector_reggroup;
>> 
>>    return group == all_reggroup;
>>  }
>> @@ -2042,6 +2140,7 @@ aarch64_pseudo_read_value (struct gdbarch
>>*gdbarch,
>>  			   struct regcache *regcache,
>>  			   int regnum)
>>  {
>> +  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
>>    gdb_byte reg_buf[MAX_REGISTER_SIZE];
>>    struct value *result_value;
>>    gdb_byte *buf;
>> @@ -2128,6 +2227,22 @@ aarch64_pseudo_read_value (struct gdbarch
>>*gdbarch,
>>        return result_value;
>>      }
>> 
>> +  if (AARCH64_TDEP_HAS_SVE (tdep) && regnum >= AARCH64_SVE_V0_REGNUM
>> +      && regnum < AARCH64_SVE_V0_REGNUM + 32)
>> +    {
>> +      enum register_status status;
>> +      unsigned v_regnum;
>> +
>> +      v_regnum = AARCH64_V0_REGNUM + regnum - AARCH64_SVE_V0_REGNUM;
>> +      status = regcache_raw_read (regcache, v_regnum, reg_buf);
>
>It should read Z register,
>
>	 z_regnum = AARCH64_SVE_Z0_REGNUM + regnum - AARCH64_SVE_V0_REGNUM;

Yes! I will fix.

>
>> +      if (status != REG_VALID)
>> +	mark_value_bytes_unavailable (result_value, 0,
>> +				      TYPE_LENGTH (value_type (result_value)));
>> +      else
>> +	memcpy (buf, reg_buf, V_REGISTER_SIZE);
>> +      return result_value;
>> +    }
>> +
>>    gdb_assert_not_reached ("regnum out of bound");
>>  }
>> 
>> @@ -2137,6 +2252,7 @@ static void
>>  aarch64_pseudo_write (struct gdbarch *gdbarch, struct regcache
>>*regcache,
>>  		      int regnum, const gdb_byte *buf)
>>  {
>> +  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
>>    gdb_byte reg_buf[MAX_REGISTER_SIZE];
>> 
>>    /* Ensure the register buffer is zero, we want gdb writes of the
>> @@ -2201,6 +2317,18 @@ aarch64_pseudo_write (struct gdbarch *gdbarch,
>> struct regcache *regcache,
>>        return;
>>      }
>> 
>> +  if (AARCH64_TDEP_HAS_SVE (tdep) && regnum >= AARCH64_SVE_V0_REGNUM
>> +      && regnum < AARCH64_SVE_V0_REGNUM + 32)
>> +    {
>> +      /* pseudo V registers.  */
>> +      unsigned v_regnum;
>> +
>> +      v_regnum = AARCH64_V0_REGNUM + regnum - AARCH64_SVE_V0_REGNUM;
>
>Likewise.

Will fix too.


>
>> +      memcpy (reg_buf, buf, V_REGISTER_SIZE);
>> +      regcache_raw_write (regcache, v_regnum, reg_buf);
>> +      return;
>> +    }
>> +
>>    gdb_assert_not_reached ("regnum out of bound");
>>  }
>> 
>> diff --git a/gdb/gdbserver/linux-aarch64-low.c
>> b/gdb/gdbserver/linux-aarch64-low.c
>> index 
>> 
>>a4cf4746285683e1529142b0073f58b68eed9cf3..6a6fc5b22dabd2d3000300aa5212f43
>>31
>> 25ab751 100644
>> --- a/gdb/gdbserver/linux-aarch64-low.c
>> +++ b/gdb/gdbserver/linux-aarch64-low.c
>> @@ -28,6 +28,8 @@
>>  #include "elf/common.h"
>>  #include "ax.h"
>>  #include "tracepoint.h"
>> +#include "tdesc.h"
>> +#include "regdef.h"
>> 
>>  #include <signal.h>
>>  #include <sys/user.h>
>> +
>> +#define SVE_SIG_CONTEXT_SIZE(vq) (SVE_SIG_REGS_OFFSET +
>> SVE_SIG_REGS_SIZE(vq))
>> +
>
>Your mailer wrap the patch, so it can't be applied.

Not sure why this went wrong. If I can’t fix my email client then I’ll
attach the patches as well as pasting them.

>
>> +#endif
>> +
>> @@ -49,15 +174,22 @@ extern const struct target_desc *tdesc_aarch64;
>> 
>>  #define AARCH64_X_REGS_NUM 31
>>  #define AARCH64_V_REGS_NUM 32
>> +#define AARCH64_SVE_Z_REGS_NUM AARCH64_V_REGS_NUM
>> +#define AARCH64_SVE_P_REGS_NUM 16
>>  #define AARCH64_X0_REGNO    0
>>  #define AARCH64_SP_REGNO   31
>>  #define AARCH64_PC_REGNO   32
>>  #define AARCH64_CPSR_REGNO 33
>>  #define AARCH64_V0_REGNO   34
>> +#define AARCH64_SVE_Z0_REGNO AARCH64_V0_REGNO
>
>We need comments to explain why register V0 and Z0 have the same
>register number.

Ok.

>
>>  #define AARCH64_FPSR_REGNO (AARCH64_V0_REGNO + AARCH64_V_REGS_NUM)
>>  #define AARCH64_FPCR_REGNO (AARCH64_V0_REGNO + AARCH64_V_REGS_NUM + 1)
>> +#define AARCH64_SVE_P0_REGNO (AARCH64_FPCR_REGNO + 1)
>> +#define AARCH64_SVE_FFR_REGNO (AARCH64_SVE_P0_REGNO +
>> AARCH64_SVE_P_REGS_NUM)
>> +#define AARCH64_SVE_VG_REGNO (AARCH64_SVE_FFR_REGNO + 1)
>> 
>> -#define AARCH64_NUM_REGS (AARCH64_V0_REGNO + AARCH64_V_REGS_NUM + 2)
>> +#define AARCH64_NUM_REGS AARCH64_FPCR_REGNO + 1
>> +#define AARCH64_SVE_NUM_REGS AARCH64_SVE_VG_REGNO + 1
>
>These register number macros can be moved to gdb/arch/aarch64.h, so
>that it can be shared in both GDB and GDBserver.

The standard aarch64 register numbers are not shared with gdbserver.
Did you want me to move all of them?

>
>> 
>>  /* Per-process arch-specific data we want to keep.  */
>> 
>> @@ -86,6 +218,16 @@ is_64bit_tdesc (void)
>>    return register_size (regcache->tdesc, 0) == 8;
>>  }
>> 
>> +/* Return true if the Z and P registers exist.  */
>> +
>> +static int
>
>static bool

Ok.

>
>> +is_sve_tdesc (void)
>> +{
>> +  struct regcache *regcache = get_thread_regcache (current_thread, 0);
>> +
>> +  return regcache->tdesc->num_registers == AARCH64_SVE_NUM_REGS;
>
>Is it safe to rely on the number of registers?  Instead, we can find
>some special register by name to tell SVE tdesc is in use or not.

I could also check that the value of the VG register is not zero.

>
>> +}
>> +
>>  /* Implementation of linux_target_ops method "cannot_store_register".
>>*/
>> 
>>  static int
>> @@ -153,6 +295,201 @@ aarch64_store_fpregset (struct regcache *regcache,
>> const void *buf)
>>    supply_register (regcache, AARCH64_FPCR_REGNO, &regset->fpcr);
>>  }
>> 
>> +static void
>> +aarch64_fill_svefpregset (struct regcache *regcache, void *buf)
>> +{
>> +  struct user_fpsimd_state *regset = (struct user_fpsimd_state *) buf;
>> +
>> +  collect_register (regcache, AARCH64_FPSR_REGNO, &regset->fpsr);
>> +  collect_register (regcache, AARCH64_FPCR_REGNO, &regset->fpcr);
>> +}
>> +
>> +static void
>> +aarch64_store_svefpregset (struct regcache *regcache, const void *buf)
>> +{
>> +  const struct user_fpsimd_state *regset
>> +    = (const struct user_fpsimd_state *) buf;
>> +
>> +  supply_register (regcache, AARCH64_FPSR_REGNO, &regset->fpsr);
>> +  supply_register (regcache, AARCH64_FPCR_REGNO, &regset->fpcr);
>> +}
>> +
>> +/* Put the registers from regcache into linux structure buf.  */
>> +
>> +static void
>> +aarch64_fill_sveregset (struct regcache *regcache, void *buf)
>> +{
>> +  struct user_sve_header *header = (struct user_sve_header *) buf;
>> +  char *base = (char*) buf;
>> +  long vq;
>> +  int i;
>> +
>> +  /* Note: We do not set VL.  Changing VL on the fly is currently
>> unsupported
>> +     in Linux.  */
>> +  gdb_assert (sve_vl_valid (header->vl));
>> +  vq = sve_vq_from_vl (header->vl);
>> +  gdb_assert (SVE_PT_SIZE (vq, header->flags) == header->size);
>> +
>> +  if (!(header->flags && SVE_PT_REGS_SVE))
>> +    {
>> +      /* There is no SVE state, however our buffer does have enough
>>room 
>> to fit
>> +	 a full SVE state.  Try to put the registers into an fpsimd structure,
>> +	 whilst checking to see if there is any actual SVE state.  */
>> +
>> +      int has_sve_state = 0;
>> +      char sve_reg[SVE_PT_SVE_ZREG_SIZE (vq)];
>> +      struct user_fpsimd_state *fpsimd
>> +	= (struct user_fpsimd_state *)(base + SVE_PT_FPSIMD_OFFSET);
>> +
>> +      memset (&sve_reg, 0, SVE_PT_SVE_ZREG_SIZE (vq));
>> +
>> +      /* Check if any SVE state exists in the regcache.  */
>> +
>> +      for (i = 0; i < AARCH64_SVE_Z_REGS_NUM; i++)
>> +	{
>> +	  has_sve_state |= compare_register (regcache,
>> +					     AARCH64_SVE_Z0_REGNO + i, &sve_reg,
>> +					     sizeof (__int128));
>> +	  if (has_sve_state != 0)
>> +	    break;
>> +	}
>> +
>> +      if (has_sve_state == 0)
>> +	for (i = 0; i < AARCH64_SVE_P_REGS_NUM; i++)
>> +	  {
>> +	    has_sve_state |= compare_register (regcache,
>> +					       AARCH64_SVE_P0_REGNO + i,
>> +					       &sve_reg, 0);
>> +	    if (has_sve_state != 0)
>> +	      break;
>> +	  }
>> +
>> +      if (has_sve_state == 0)
>> +	  has_sve_state |= compare_register (regcache, AARCH64_SVE_FFR_REGNO,
>> +					     &sve_reg, 0);
>> +
>> +      /* If no SVE state then copy into a fpsimd structure.  */
>> +
>> +      if (has_sve_state == 0)
>> +	{
>> +	  /* These collects will overflow the size of a vreg, but that does 
>>not
>> +	     matter as we have enough room in the buffer and we know that the
>> +	     overflow will always be 0s.  */
>> +	  for (i = 0; i < AARCH64_SVE_Z_REGS_NUM; i++)
>> +	    collect_register (regcache, AARCH64_SVE_Z0_REGNO + i,
>> +			      &fpsimd->vregs[i]);
>> +
>> +	  collect_register (regcache, AARCH64_FPSR_REGNO, &fpsimd->fpsr);
>> +	  collect_register (regcache, AARCH64_FPCR_REGNO, &fpsimd->fpcr);
>> +
>> +	  return;
>> +	}
>> +
>> +      /* Otherwise, enable SVE state in the header.  */
>> +
>> +      header->flags |= SVE_PT_REGS_SVE;
>> +      header->size = SVE_PT_SIZE (vq, header->flags);
>> +      memset (base + SVE_PT_SVE_OFFSET, 0,
>> +	      SVE_PT_SVE_SIZE (vq, SVE_PT_REGS_SVE));
>> +    }
>> +
>> +  for (i = 0; i < AARCH64_SVE_Z_REGS_NUM; i++)
>> +    collect_register (regcache, AARCH64_SVE_Z0_REGNO + i,
>> +		      base + SVE_PT_SVE_ZREG_OFFSET (vq, i));
>> +
>> +  for (i = 0; i < AARCH64_SVE_P_REGS_NUM; i++)
>> +    collect_register (regcache, AARCH64_SVE_P0_REGNO + i,
>> +		      base + SVE_PT_SVE_PREG_OFFSET (vq, i));
>> +
>> +  collect_register (regcache, AARCH64_SVE_FFR_REGNO,
>> +		    base + SVE_PT_SVE_FFR_OFFSET (vq));
>> +  collect_register (regcache, AARCH64_FPSR_REGNO,
>> +		    base + SVE_PT_SVE_FPSR_OFFSET (vq));
>> +  collect_register (regcache, AARCH64_FPCR_REGNO,
>> +		    base + SVE_PT_SVE_FPCR_OFFSET (vq));
>> +}
>> +
>> +/* Put the registers from linux structure buf into regcache.  */
>> +
>> +static void
>> +aarch64_store_sveregset (struct regcache *regcache, const void *buf)
>> +{
>> +  struct user_sve_header *header = (struct user_sve_header *) buf;
>> +  char *base = (char*) buf;
>> +  long vg, vq;
>> +  int i;
>> +
>> +  gdb_assert (sve_vl_valid (header->vl));
>> +  vg = sve_vg_from_vl (header->vl);
>> +  vq = sve_vq_from_vl (header->vl);
>> +  gdb_assert (SVE_PT_SIZE (vq, header->flags) == header->size);
>> +  supply_register (regcache, AARCH64_SVE_VG_REGNO, &vg);
>> +
>> +  if (header->flags && SVE_PT_REGS_SVE)
>> +    {
>> +      for (i = 0; i < AARCH64_SVE_Z_REGS_NUM; i++)
>> +	supply_register (regcache, AARCH64_SVE_Z0_REGNO + i,
>> +			 base + SVE_PT_SVE_ZREG_OFFSET (vq, i));
>> +
>> +      for (i = 0; i < AARCH64_SVE_P_REGS_NUM; i++)
>> +	supply_register (regcache, AARCH64_SVE_P0_REGNO + i,
>> +			 base + SVE_PT_SVE_PREG_OFFSET (vq, i));
>> +
>> +      supply_register (regcache, AARCH64_SVE_FFR_REGNO,
>> +		       base + SVE_PT_SVE_FFR_OFFSET (vq));
>> +      supply_register (regcache, AARCH64_FPSR_REGNO,
>> +		       base + SVE_PT_SVE_FPSR_OFFSET (vq));
>> +      supply_register (regcache, AARCH64_FPCR_REGNO,
>> +		       base + SVE_PT_SVE_FPCR_OFFSET (vq));
>> +    }
>> +  else
>> +    {
>> +      /* There is no SVE state.  */
>> +      char sve_reg[SVE_PT_SVE_ZREG_SIZE (vq)];
>> +      struct user_fpsimd_state *fpsimd
>> +	= (struct user_fpsimd_state *)(base + SVE_PT_FPSIMD_OFFSET);
>> +
>> +      /* Clear the SVE only registers.  */
>> +
>> +      memset (&sve_reg, 0, SVE_PT_SVE_ZREG_SIZE (vq));
>> +
>> +      for (i = 0; i <= AARCH64_SVE_P_REGS_NUM; i++)
>> +	supply_register (regcache, AARCH64_SVE_P0_REGNO + i, &sve_reg);
>> +
>> +      supply_register (regcache, AARCH64_SVE_FFR_REGNO, &sve_reg);
>> +
>> +      /* Convert vregs from fpsimd state to zregs, ensuring additional 
>> state is
>> +	 null.  */
>> +
>> +      for (i = 0; i <= AARCH64_SVE_Z_REGS_NUM; i++)
>> +	{
>> +	  memcpy (&sve_reg, &fpsimd->vregs[i], sizeof (__int128));
>> +	  supply_register (regcache, AARCH64_SVE_Z0_REGNO + i, &sve_reg);
>> +	}
>> +
>> +      supply_register (regcache, AARCH64_FPSR_REGNO, &fpsimd->fpsr);
>> +      supply_register (regcache, AARCH64_FPCR_REGNO, &fpsimd->fpcr);
>> +    }
>> +}
>> +
>> +/* Given an sve header from ptrace, return the size of a full register 
>>set
>> +   (with header).  */
>> +
>> +static int
>> +aarch64_get_full_sve_size (const void *buf)
>> +{
>> +  struct user_sve_header *header = (struct user_sve_header *) buf;
>> +
>> +  gdb_assert (sve_vl_valid (header->vl));
>> +
>> +  if (header->flags && SVE_PT_REGS_SVE)
>> +    return header->size;
>> +  else
>> +    /* There is no SVE state.  If we are writing state back to the 
>>kernel 
>> then
>> +       there needs to be enough room to fit a current sized SVE state. 
>> */
>> +    return SVE_PT_SIZE (sve_vq_from_vl (header->vl), SVE_PT_REGS_SVE);
>> +}
>> +
>>  /* Enable miscellaneous debugging output.  The name is historical - it
>>     was originally used to debug LinuxThreads support.  */
>>  extern int debug_threads;
>> @@ -526,13 +863,48 @@ static struct regs_info regs_info_aarch64 =
>>      &aarch64_regsets_info,
>>    };
>> 
>> +static struct regset_info aarch64_sve_regsets[] =
>> +{
>> +  { PTRACE_GETREGSET, PTRACE_SETREGSET, NT_PRSTATUS,
>> +    sizeof (struct user_pt_regs), GENERAL_REGS,
>> +    aarch64_fill_gregset, aarch64_store_gregset },
>> +  { PTRACE_GETREGSET, PTRACE_SETREGSET, NT_FPREGSET,
>> +    sizeof (struct user_fpsimd_state), FP_REGS,
>> +    aarch64_fill_svefpregset, aarch64_store_svefpregset
>> +  },
>
>Do you need this entry?  IIUC, GDBserver can read FPSR and FPCR
>via NT_ARM_SVE.

Yes. I think this is code I forgot to remove from an older patch.

>
>> +  { PTRACE_GETREGSET, PTRACE_SETREGSET, NT_ARM_SVE,
>> +    sizeof (struct user_sve_header), EXTENDED_REGS,
>> +    aarch64_fill_sveregset, aarch64_store_sveregset,
>> +    aarch64_get_full_sve_size
>> +  },
>
>I don't like adding another function in "struct regset_info".
>Can we set regset_info.size to the max size, so that we
>don't need aarch64_get_full_sve_size.

We could - however that would mean we always allocating to the
max VG size, even for targets that do not support that max VG.
That's quite a large size.
If people are happy to always allocate this much, then ok.

>
>> +  NULL_REGSET
>> +};
>> +
>
>> @@ -546,8 +918,8 @@ aarch64_supports_tracepoints (void)
>>      return 1;
>>    else
>>      {
>> -      /* We don't support tracepoints on aarch32 now.  */
>> -      return is_64bit_tdesc ();
>> +      /* We don't support tracepoints on either aarch32 or SVE.  */
>> +      return is_64bit_tdesc () && !is_sve_tdesc ();
>
>SVE registers can't be collected in tracepoint, but we shouldn't
>prevent user using tracepoint on AArch64.  Secondly, we still can't
>disable tracepoint for SVE, because aarch64_supports_tracepoints
>is called when GDB connects to GDBserver.  At that moment, the
>process doesn't SVE state, but it may have later.
>
>IMO, we should still allow tracepoint but disable user collecting
>Z and P registers in tracepoint actions.

Ok, I can look into this.

>
>> diff --git a/gdb/gdbserver/linux-low.h b/gdb/gdbserver/linux-low.h
>> index 
>> 
>>b5ac8de5ccee13866d6b74a34d3db87412537e66..78a671fe13f689c64b64378eeafe2de
>>d3
>> e309ca7 100644
>> --- a/gdb/gdbserver/linux-low.h
>> +++ b/gdb/gdbserver/linux-low.h
>> @@ -32,6 +32,7 @@
>>  #ifdef HAVE_LINUX_REGSETS
>>  typedef void (*regset_fill_func) (struct regcache *, void *);
>>  typedef void (*regset_store_func) (struct regcache *, const void *);
>> +typedef int (*regset_get_full_size_func) (const void *);
>>  enum regset_type {
>>    GENERAL_REGS,
>>    FP_REGS,
>> @@ -53,6 +54,7 @@ struct regset_info
>>    enum regset_type type;
>>    regset_fill_func fill_function;
>>    regset_store_func store_function;
>> +  regset_get_full_size_func get_full_size_function;
>
>
>
>>  };
>> 
>>  /* Aggregation of all the supported regsets of a given
>> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
>> index 
>> 
>>602f7376c60a9b7b23496e4e0dea437e985f7c71..595144f7123c72a248dda651bd7c42b
>>04
>> 0f286a7 100644
>> --- a/gdb/gdbserver/linux-low.c
>> +++ b/gdb/gdbserver/linux-low.c
>> @@ -5490,6 +5490,38 @@ regsets_fetch_inferior_registers (struct 
>> regsets_info *regsets_info,
>>  #else
>>        res = ptrace (regset->get_request, pid, data, nt_type);
>>  #endif
>> +
>> +      if (res == 0 && regset->get_full_size_function != NULL)
>> +	{
>> +	  /* First call only obtained a partial set of data.  Use obtained 
>>data
>> +	     to get the full size and then call ptrace again.  */
>> +
>> +	  int full_size = regset->get_full_size_function (buf);
>> +
>> +	  if (full_size <= 0)
>> +	    res = -1;
>> +	  else
>> +	    {
>> +	      free (buf);
>> +	      buf = xmalloc (full_size);
>> +
>> +	      if (nt_type)
>> +		{
>> +		  iov.iov_base = buf;
>> +		  iov.iov_len = full_size;
>> +		}
>> +	      else
>> +		data = buf;
>> +
>> +#ifndef __sparc__
>> +	      res = ptrace (regset->get_request, pid,
>> +			    (PTRACE_TYPE_ARG3) (long) nt_type, data);
>> +#else
>> +	      res = ptrace (regset->get_request, pid, data, nt_type);
>> +#endif
>> +	    }
>> +	}
>> +
>>        if (res < 0)
>>  	{
>>  	  if (errno == EIO)
>> @@ -5574,6 +5606,37 @@ regsets_store_inferior_registers (struct 
>> regsets_info *regsets_info,
>>        res = ptrace (regset->get_request, pid, data, nt_type);
>>  #endif
>> 
>> +      if (res == 0 && regset->get_full_size_function != NULL)
>> +	{
>> +	  /* First call only obtained a partial set of data.  Use obtained 
>>data
>> +	     to get the full size and then call ptrace again.  */
>> +
>> +	  int full_size = regset->get_full_size_function (buf);
>> +
>> +	  if (full_size <= 0)
>> +	    res = -1;
>> +	  else
>> +	    {
>> +	      free (buf);
>> +	      buf = xmalloc (full_size);
>> +
>> +	      if (nt_type)
>> +		{
>> +		  iov.iov_base = buf;
>> +		  iov.iov_len = full_size;
>> +		}
>> +	      else
>> +		data = buf;
>> +
>> +#ifndef __sparc__
>> +	      res = ptrace (regset->get_request, pid,
>> +			    (PTRACE_TYPE_ARG3) (long) nt_type, data);
>> +#else
>> +	      res = ptrace (regset->get_request, pid, data, nt_type);
>> +#endif
>> +	    }
>> +	}
>> +
>
>I hope after we set regset_info.size to the max size, these changes above
>are not needed any more.

Correct.

>
>> diff --git a/include/elf/common.h b/include/elf/common.h
>> index 
>> 
>>da79613e6dbad54003e921c2fbe2b30b34c21595..80a4d730b5c86e8cfdb29e1163e934b
>>9f
>> 8a7b498 100644
>> --- a/include/elf/common.h
>> +++ b/include/elf/common.h
>> @@ -605,6 +605,7 @@
>>  					/*   note name must be "LINUX".  */
>>  #define NT_ARM_HW_WATCH	0x403		/* AArch hardware watchpoint registers 
>>*/
>>  					/*   note name must be "LINUX".  */
>> +#define NT_ARM_SVE	0x405		/* ARM SVE registers.  */
>>  #define NT_SIGINFO	0x53494749	/* Fields of siginfo_t.  */
>>  #define NT_FILE		0x46494c45	/* Description of mapped files.  */
>
>This change should be sent to binutils for review.

Ok.

>
>-- 
>Yao (齐尧)



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]