[8/8] Ptrace support for Aarch64 SVE

Message ID 20180511105256.27388-9-alan.hayward@arm.com
State New, archived
Headers

Commit Message

Alan Hayward May 11, 2018, 10:52 a.m. UTC
  Add support for reading and writing registers for Aarch64 SVE.
I've made this functionality common as it will be required for
gdbserver when gdbsever sve support is added.

Given that gdbserver does not yet call this function, I am
happy to remove the regcache commonise functions from the
previous patch. However, this would result in code in nat/
that does not compile for gdbserver. I wanted to avoid that.

We need to support the cases where the kernel only gives us a
fpsimd structure. This occurs when there is no active SVE state
in the kernel (for example, after starting a new process).

As per the covering email description, I've included large chunks
of linux kernel headers within an ifdef. Formatting of these macros
remains identical to the Kernel versions (ie not adapted to GNU style).

Added checks to make sure the vector length has not changed whilst
the process is running.

2018-05-11  Alan Hayward  <alan.hayward@arm.com>

gdb/
	* aarch64-linux-nat.c (fetch_sveregs_from_thread): New function.
	(store_sveregs_to_thread): Likewise.
	(aarch64_linux_fetch_inferior_registers): Check for SVE.
	(aarch64_linux_store_inferior_registers): Likewise.
	* nat/aarch64-sve-linux-ptrace.c (aarch64_sve_get_sveregs): New
	function.
	(aarch64_sve_regs_copy_to_regcache): Likewise.
	(aarch64_sve_regs_copy_from_regcache): Likewise.
	* nat/aarch64-sve-linux-ptrace.h (aarch64_sve_get_sveregs): New
	declaration.
	(aarch64_sve_regs_copy_to_regcache): Likewise.
	(aarch64_sve_regs_copy_from_regcache): Likewise.
	(sve_context): Structure from Linux headers.
	(SVE_SIG_ZREGS_SIZE): Define from Linux headers.
	(SVE_SIG_ZREG_SIZE): Likewise.
	(SVE_SIG_PREG_SIZE): Likewise.
	(SVE_SIG_FFR_SIZE): Likewise.
	(SVE_SIG_REGS_OFFSET): Likewise.
	(SVE_SIG_ZREGS_OFFSET): Likewise.
	(SVE_SIG_ZREG_OFFSET): Likewise.
	(SVE_SIG_ZREGS_SIZE): Likewise.
	(SVE_SIG_PREGS_OFFSET): Likewise.
	(SVE_SIG_PREG_OFFSET): Likewise.
	(SVE_SIG_PREGS_SIZE): Likewise.
	(SVE_SIG_FFR_OFFSET): Likewise.
	(SVE_SIG_REGS_SIZE): Likewise.
	(SVE_SIG_CONTEXT_SIZE): Likewise.
	(SVE_PT_REGS_MASK): Likewise.
	(SVE_PT_REGS_FPSIMD): Likewise.
	(SVE_PT_REGS_SVE): Likewise.
	(SVE_PT_VL_INHERIT): Likewise.
	(SVE_PT_VL_ONEXEC): Likewise.
	(SVE_PT_REGS_OFFSET): Likewise.
	(SVE_PT_FPSIMD_OFFSET): Likewise.
	(SVE_PT_FPSIMD_SIZE): Likewise.
	(SVE_PT_SVE_ZREG_SIZE): Likewise.
	(SVE_PT_SVE_PREG_SIZE): Likewise.
	(SVE_PT_SVE_FFR_SIZE): Likewise.
	(SVE_PT_SVE_FPSR_SIZE): Likewise.
	(SVE_PT_SVE_FPCR_SIZE): Likewise.
	(__SVE_SIG_TO_PT): Likewise.
	(SVE_PT_SVE_OFFSET): Likewise.
	(SVE_PT_SVE_ZREGS_OFFSET): Likewise.
	(SVE_PT_SVE_ZREG_OFFSET): Likewise.
	(SVE_PT_SVE_ZREGS_SIZE): Likewise.
	(SVE_PT_SVE_PREGS_OFFSET): Likewise.
	(SVE_PT_SVE_PREG_OFFSET): Likewise.
	(SVE_PT_SVE_PREGS_SIZE): Likewise.
	(SVE_PT_SVE_FFR_OFFSET): Likewise.
	(SVE_PT_SVE_FPSR_OFFSET): Likewise.
	(SVE_PT_SVE_FPCR_OFFSET): Likewise.
	(SVE_PT_SVE_SIZE): Likewise.
	(SVE_PT_SIZE): Likewise.
	(HAS_SVE_STATE): New define.

gdbserver
	* Makefile.in: Add aarch64-sve-linux-ptrace.c.
---
 gdb/aarch64-linux-nat.c            |  57 +++++++-
 gdb/gdbserver/Makefile.in          |   1 +
 gdb/nat/aarch64-sve-linux-ptrace.c | 263 +++++++++++++++++++++++++++++++++++++
 gdb/nat/aarch64-sve-linux-ptrace.h | 120 +++++++++++++++++
 4 files changed, 439 insertions(+), 2 deletions(-)
  

Comments

Simon Marchi May 31, 2018, 1:22 p.m. UTC | #1
Hi Alan,

Since there is a good number of macros copied from the Linux kernel, it might be
a good idea to isolate in their own file.  It would also be good to identify
precisely which file they come from (the path relative to the root of the linux
repo) and the git commit you used.

Also, since the copied code is rather large and non-trivial, does it pose copyright
problems?  If we want to include it, maybe that separate file should not state that
the copyright is owned by the FSF, since it's not the case?  Maybe others have
experience with this kind of things, or maybe we should get an advice from the FSF directly.

> diff --git a/gdb/nat/aarch64-sve-linux-ptrace.c b/gdb/nat/aarch64-sve-linux-ptrace.c
> index 9381786fda..84c7a41f40 100644
> --- a/gdb/nat/aarch64-sve-linux-ptrace.c
> +++ b/gdb/nat/aarch64-sve-linux-ptrace.c
> @@ -25,6 +25,13 @@
>  #include "aarch64-sve-linux-ptrace.h"
>  #include "arch/aarch64.h"
>  
> +#ifndef GDBSERVER
> +#include "defs.h"
> +#endif
> +#include "regcache.h"

Hmm we try not add any more "#ifdef GDBSERVER" in the common code.

https://sourceware.org/gdb/wiki/Common#Header_files_in_common_code_.28defs.h_vs_server.h.2C_etc..29

Instead, we should try defining a common interface (probably in common/common-regcache.h?) that the
common code will use, and that regcaches from GDB and GDBserver will implement.

> +
> +static bool vq_change_warned = false;
> +
>  /* Read VQ for the given tid using ptrace.  If SVE is not supported then zero
>     is returned (on a system that supports SVE, then VQ cannot be zeo).  */
>  
> @@ -50,3 +57,259 @@ aarch64_sve_get_vq (int tid)
>  
>    return vq;
>  }
> +
> +/* Read the current SVE register set using ptrace, allocating space as
> +   required.  */

Put a reference to the .h here.

Since this returns allocated memory, could we return an RAII object?  Either
std::vector, std::unique_ptr or gdb::unique_xmalloc_ptr.

> +
> +gdb_byte *
> +aarch64_sve_get_sveregs (int tid)
> +{
> +  int ret;
> +  struct iovec iovec;
> +  struct user_sve_header header;
> +  long vq = aarch64_sve_get_vq (tid);
> +
> +  if (vq == 0)
> +    perror_with_name (_("Unable to fetch sve register header"));
> +
> +  /* A ptrace call with NT_ARM_SVE will return a header followed by either a
> +     dump of all the SVE and FP registers, or an fpsimd structure (identical to
> +     the one returned by NT_FPREGSET) if the kernel has not yet executed any
> +     SVE code.  Make sure we allocate enough space for a full SVE dump.  */
> +
> +  iovec.iov_len = SVE_PT_SIZE (vq, SVE_PT_REGS_SVE);
> +  iovec.iov_base = xmalloc (iovec.iov_len);
> +
> +  ret = ptrace (PTRACE_GETREGSET, tid, NT_ARM_SVE, &iovec);
> +  if (ret < 0)
> +    perror_with_name (_("Unable to fetch sve registers"));
> +
> +  return (gdb_byte *) iovec.iov_base;
> +}
> +
> +/* Put the registers from linux structure buf into regcache.  */
> +
> +void
> +aarch64_sve_regs_copy_to_regcache (struct regcache *regcache, const void *buf)
> +{
> +  char *base = (char*) buf;
> +  int i;
> +  struct user_sve_header *header = (struct user_sve_header *) buf;
> +  long vq, vg_regcache;
> +
> +  vq = sve_vq_from_vl (header->vl);
> +
> +  /* Sanity check the data in the header.  */
> +  gdb_assert (sve_vl_valid (header->vl));
> +  gdb_assert (SVE_PT_SIZE (vq, header->flags) == header->size);

Again, we shouldn't use gdb_assert here, since this validates external input.

> +
> +  regcache->raw_collect (AARCH64_SVE_VG_REGNUM, &vg_regcache);

When fetching registers, won't it be usually to fill a shiny new, empty regcache?
In that case, won't it always fall into the "if" branch?

In any case, should we check the status of the VG register to make sure it's REG_VALID
before we try to collect it?

> +  if (vg_regcache == 0)
> +    {
> +      /* VG has not been set.  */
> +      vg_regcache = sve_vg_from_vl (header->vl);
> +      regcache->raw_supply (AARCH64_SVE_VG_REGNUM, &vg_regcache);
> +    }
> +  else if (vg_regcache != sve_vg_from_vl (header->vl) && !vq_change_warned)
> +    {
> +      /* Vector length on the running process has changed.  GDB currently does
> +	 not support this and will result in GDB showing incorrect partially
> +	 incorrect data for the vector registers.  Warn once and continue.  We
> +	 do not expect many programs to exhibit this behaviour.  To fix this
> +	 we need to spot the change earlier and generate a new target
> +	 descriptor.  */
> +      warning (_("Vector length has changed (%ld to %d). "
> +		 "Vector registers may show incorrect data."),

Perhaps mention "SVE vector length has changed..."?  Otherwise the user may wonder
what vectors we are talking about.

> +		 vg_regcache, sve_vg_from_vl (header->vl));
> +      vq_change_warned = true;
> +    }
> +
> +  if (HAS_SVE_STATE (*header))
> +    {
> +      /* The register dump contains a set of SVE registers.  */
> +
> +      for (i = 0; i < AARCH64_SVE_Z_REGS_NUM; i++)
> +	regcache->raw_supply (AARCH64_SVE_Z0_REGNUM + i,
> +		    base + SVE_PT_SVE_ZREG_OFFSET (vq, i));
> +
> +      for (i = 0; i < AARCH64_SVE_P_REGS_NUM; i++)
> +	regcache->raw_supply (AARCH64_SVE_P0_REGNUM + i,
> +		    base + SVE_PT_SVE_PREG_OFFSET (vq, i));
> +
> +      regcache->raw_supply (AARCH64_SVE_FFR_REGNUM,
> +		  base + SVE_PT_SVE_FFR_OFFSET (vq));
> +      regcache->raw_supply (AARCH64_FPSR_REGNUM,
> +		  base + SVE_PT_SVE_FPSR_OFFSET (vq));
> +      regcache->raw_supply (AARCH64_FPCR_REGNUM,
> +		  base + SVE_PT_SVE_FPCR_OFFSET (vq));

Align the second line with the first argument.  Here's an example, though
using spaces instead of tabs to make sure (hopefully) the mail clients display it
correctly.

        regcache->raw_supply (AARCH64_SVE_Z0_REGNUM + i,
                              base + SVE_PT_SVE_ZREG_OFFSET (vq, i));

There are many instances throughout this file.

Simon
  
Alan Hayward May 31, 2018, 2:46 p.m. UTC | #2
> On 31 May 2018, at 14:22, Simon Marchi <simon.marchi@ericsson.com> wrote:

> 

> Hi Alan,

> 

> Since there is a good number of macros copied from the Linux kernel, it might be

> a good idea to isolate in their own file.  It would also be good to identify

> precisely which file they come from (the path relative to the root of the linux

> repo) and the git commit you used.

> 

> Also, since the copied code is rather large and non-trivial, does it pose copyright

> problems?  If we want to include it, maybe that separate file should not state that

> the copyright is owned by the FSF, since it's not the case?  Maybe others have

> experience with this kind of things, or maybe we should get an advice from the FSF directly.


A new file makes sense (especially with regards to copyright). However, the plan was
to eventually remove the macros when "enough" time has passed - keeping everything
in aarch64-sve-linux-ptrace.h made it simpler than removing files.

Is there a minimum kernel version that gdb requires to link against in order to build?
The relevant defines are in 4.15. The kernel is now on 4.16.9.

I’m guessing this must have come up before when other targets added new kernel features?
I didn’t want to suddenly destroy everyone’s aarch64/targetall build trees without
warning.


>> diff --git a/gdb/nat/aarch64-sve-linux-ptrace.c b/gdb/nat/aarch64-sve-linux-ptrace.c

>> index 9381786fda..84c7a41f40 100644

>> --- a/gdb/nat/aarch64-sve-linux-ptrace.c

>> +++ b/gdb/nat/aarch64-sve-linux-ptrace.c

>> @@ -25,6 +25,13 @@

>> #include "aarch64-sve-linux-ptrace.h"

>> #include "arch/aarch64.h"

>> 

>> +#ifndef GDBSERVER

>> +#include "defs.h"

>> +#endif

>> +#include "regcache.h"

> 

> Hmm we try not add any more "#ifdef GDBSERVER" in the common code.

> 

> https://sourceware.org/gdb/wiki/Common#Header_files_in_common_code_.28defs.h_vs_server.h.2C_etc..29

> 

> Instead, we should try defining a common interface (probably in common/common-regcache.h?) that the

> common code will use, and that regcaches from GDB and GDBserver will implement.


I tried using common-defs.h, but gdb/regcache.h requires defines from
defs.h - RequireLongest and maybe others.
Putting defs.h at the top of gdb/regcache.h then broke in a weird way.
A lot of fiddling later, and I hadn’t found a way to make it work.

Creating common/common-regcache.h gets a bit odd because, the functions
I need for gdbserver (raw_supply, raw_collect and get_register_status)
on gdb come from:


class reg_buffer
...
  enum register_status get_register_status (int regnum) const;
...


class readable_regcache : public reg_buffer
...


class detached_regcache : public readable_regcache
...
  void raw_supply (int regnum, const void *buf);
...


class regcache : public detached_regcache
...
  void raw_collect (int regnum, void *buf) const;
...


I don’t think that this would work:
class regcache : public detached_regcache, common_regcache


> 

>> +

>> +static bool vq_change_warned = false;

>> +

>> /* Read VQ for the given tid using ptrace.  If SVE is not supported then zero

>>    is returned (on a system that supports SVE, then VQ cannot be zeo).  */

>> 

>> @@ -50,3 +57,259 @@ aarch64_sve_get_vq (int tid)

>> 

>>   return vq;

>> }

>> +

>> +/* Read the current SVE register set using ptrace, allocating space as

>> +   required.  */

> 

> Put a reference to the .h here.

> 

> Since this returns allocated memory, could we return an RAII object?  Either

> std::vector, std::unique_ptr or gdb::unique_xmalloc_ptr.


Yes, I’ll update with one of those. Is there any documentation explaining
when to use which of these?

> 

>> +

>> +gdb_byte *

>> +aarch64_sve_get_sveregs (int tid)

>> +{

>> +  int ret;

>> +  struct iovec iovec;

>> +  struct user_sve_header header;

>> +  long vq = aarch64_sve_get_vq (tid);

>> +

>> +  if (vq == 0)

>> +    perror_with_name (_("Unable to fetch sve register header"));

>> +

>> +  /* A ptrace call with NT_ARM_SVE will return a header followed by either a

>> +     dump of all the SVE and FP registers, or an fpsimd structure (identical to

>> +     the one returned by NT_FPREGSET) if the kernel has not yet executed any

>> +     SVE code.  Make sure we allocate enough space for a full SVE dump.  */

>> +

>> +  iovec.iov_len = SVE_PT_SIZE (vq, SVE_PT_REGS_SVE);

>> +  iovec.iov_base = xmalloc (iovec.iov_len);

>> +

>> +  ret = ptrace (PTRACE_GETREGSET, tid, NT_ARM_SVE, &iovec);

>> +  if (ret < 0)

>> +    perror_with_name (_("Unable to fetch sve registers"));

>> +

>> +  return (gdb_byte *) iovec.iov_base;

>> +}

>> +

>> +/* Put the registers from linux structure buf into regcache.  */

>> +

>> +void

>> +aarch64_sve_regs_copy_to_regcache (struct regcache *regcache, const void *buf)

>> +{

>> +  char *base = (char*) buf;

>> +  int i;

>> +  struct user_sve_header *header = (struct user_sve_header *) buf;

>> +  long vq, vg_regcache;

>> +

>> +  vq = sve_vq_from_vl (header->vl);

>> +

>> +  /* Sanity check the data in the header.  */

>> +  gdb_assert (sve_vl_valid (header->vl));

>> +  gdb_assert (SVE_PT_SIZE (vq, header->flags) == header->size);

> 

> Again, we shouldn't use gdb_assert here, since this validates external input.


Ok.

> 

>> +

>> +  regcache->raw_collect (AARCH64_SVE_VG_REGNUM, &vg_regcache);

> 

> When fetching registers, won't it be usually to fill a shiny new, empty regcache?

> In that case, won't it always fall into the "if" branch?

> 

> In any case, should we check the status of the VG register to make sure it's REG_VALID

> before we try to collect it?


I thought the same regcache was used each time registers needed reading?
I’ll double check this.
Either way, yes, should probably check REG_VALID

> 

>> +  if (vg_regcache == 0)

>> +    {

>> +      /* VG has not been set.  */

>> +      vg_regcache = sve_vg_from_vl (header->vl);

>> +      regcache->raw_supply (AARCH64_SVE_VG_REGNUM, &vg_regcache);

>> +    }

>> +  else if (vg_regcache != sve_vg_from_vl (header->vl) && !vq_change_warned)

>> +    {

>> +      /* Vector length on the running process has changed.  GDB currently does

>> +	 not support this and will result in GDB showing incorrect partially

>> +	 incorrect data for the vector registers.  Warn once and continue.  We

>> +	 do not expect many programs to exhibit this behaviour.  To fix this

>> +	 we need to spot the change earlier and generate a new target

>> +	 descriptor.  */

>> +      warning (_("Vector length has changed (%ld to %d). "

>> +		 "Vector registers may show incorrect data."),

> 

> Perhaps mention "SVE vector length has changed..."?  Otherwise the user may wonder

> what vectors we are talking about.


Agreed.

> 

>> +		 vg_regcache, sve_vg_from_vl (header->vl));

>> +      vq_change_warned = true;

>> +    }

>> +

>> +  if (HAS_SVE_STATE (*header))

>> +    {

>> +      /* The register dump contains a set of SVE registers.  */

>> +

>> +      for (i = 0; i < AARCH64_SVE_Z_REGS_NUM; i++)

>> +	regcache->raw_supply (AARCH64_SVE_Z0_REGNUM + i,

>> +		    base + SVE_PT_SVE_ZREG_OFFSET (vq, i));

>> +

>> +      for (i = 0; i < AARCH64_SVE_P_REGS_NUM; i++)

>> +	regcache->raw_supply (AARCH64_SVE_P0_REGNUM + i,

>> +		    base + SVE_PT_SVE_PREG_OFFSET (vq, i));

>> +

>> +      regcache->raw_supply (AARCH64_SVE_FFR_REGNUM,

>> +		  base + SVE_PT_SVE_FFR_OFFSET (vq));

>> +      regcache->raw_supply (AARCH64_FPSR_REGNUM,

>> +		  base + SVE_PT_SVE_FPSR_OFFSET (vq));

>> +      regcache->raw_supply (AARCH64_FPCR_REGNUM,

>> +		  base + SVE_PT_SVE_FPCR_OFFSET (vq));

> 

> Align the second line with the first argument.  Here's an example, though

> using spaces instead of tabs to make sure (hopefully) the mail clients display it

> correctly.

> 

>        regcache->raw_supply (AARCH64_SVE_Z0_REGNUM + i,

>                              base + SVE_PT_SVE_ZREG_OFFSET (vq, i));

> 

> There are many instances throughout this file.

> 


I think that’s leftover from when I changed from "raw_supply (regcache,”
to “regcache->raw_supply (“.
I’ll fix it up.


Alan.
  
Simon Marchi May 31, 2018, 7:38 p.m. UTC | #3
On 2018-05-11 06:52 AM, Alan Hayward wrote:
> diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
> index 1270b7e0c0..bc570ade6d 100644
> --- a/gdb/aarch64-linux-nat.c
> +++ b/gdb/aarch64-linux-nat.c
> @@ -388,19 +388,65 @@ 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)
> +{
> +  gdb_byte *base = aarch64_sve_get_sveregs (ptid_get_lwp (inferior_ptid));
> +  aarch64_sve_regs_copy_to_regcache (regcache, base);
> +  xfree (base);
> +}
> +
> +/* Store to the current thread the valid sve register
> +   values in the GDB's register array.  */
> +
> +static void
> +store_sveregs_to_thread (struct regcache *regcache)
> +{
> +  gdb_byte *base;
> +  int ret, tid;
> +  struct iovec iovec;
> +
> +  tid = ptid_get_lwp (inferior_ptid);

I've just noticed these two uses of inferior_ptid.  I think we should get the ptid
from the regcache instead.

Simon
  
Simon Marchi June 1, 2018, 3:16 p.m. UTC | #4
On 2018-05-31 10:46 AM, Alan Hayward wrote:
> 
> 
>> On 31 May 2018, at 14:22, Simon Marchi <simon.marchi@ericsson.com> wrote:
>>
>> Hi Alan,
>>
>> Since there is a good number of macros copied from the Linux kernel, it might be
>> a good idea to isolate in their own file.  It would also be good to identify
>> precisely which file they come from (the path relative to the root of the linux
>> repo) and the git commit you used.
>>
>> Also, since the copied code is rather large and non-trivial, does it pose copyright
>> problems?  If we want to include it, maybe that separate file should not state that
>> the copyright is owned by the FSF, since it's not the case?  Maybe others have
>> experience with this kind of things, or maybe we should get an advice from the FSF directly.
> 
> A new file makes sense (especially with regards to copyright). However, the plan was
> to eventually remove the macros when "enough" time has passed - keeping everything
> in aarch64-sve-linux-ptrace.h made it simpler than removing files.

How come?  Removing a file is not complicated.

> Is there a minimum kernel version that gdb requires to link against in order to build?
> The relevant defines are in 4.15. The kernel is now on 4.16.9.
>
> I’m guessing this must have come up before when other targets added new kernel features?
> I didn’t want to suddenly destroy everyone’s aarch64/targetall build trees without
> warning.

I don't think there's a specific Linux kernel version we require to build GDB, but it's
good to support as far back as possible with a reasonable effort.  Since they have only
been added very recently, I would keep our copy of the macros for a while, if it's not
in the way of anything.

>>> diff --git a/gdb/nat/aarch64-sve-linux-ptrace.c b/gdb/nat/aarch64-sve-linux-ptrace.c
>>> index 9381786fda..84c7a41f40 100644
>>> --- a/gdb/nat/aarch64-sve-linux-ptrace.c
>>> +++ b/gdb/nat/aarch64-sve-linux-ptrace.c
>>> @@ -25,6 +25,13 @@
>>> #include "aarch64-sve-linux-ptrace.h"
>>> #include "arch/aarch64.h"
>>>
>>> +#ifndef GDBSERVER
>>> +#include "defs.h"
>>> +#endif
>>> +#include "regcache.h"
>>
>> Hmm we try not add any more "#ifdef GDBSERVER" in the common code.
>>
>> https://sourceware.org/gdb/wiki/Common#Header_files_in_common_code_.28defs.h_vs_server.h.2C_etc..29
>>
>> Instead, we should try defining a common interface (probably in common/common-regcache.h?) that the
>> common code will use, and that regcaches from GDB and GDBserver will implement.
> 
> I tried using common-defs.h, but gdb/regcache.h requires defines from
> defs.h - RequireLongest and maybe others.
> Putting defs.h at the top of gdb/regcache.h then broke in a weird way.
> A lot of fiddling later, and I hadn’t found a way to make it work.
> 
> Creating common/common-regcache.h gets a bit odd because, the functions
> I need for gdbserver (raw_supply, raw_collect and get_register_status)
> on gdb come from:
> 
> 
> class reg_buffer
> ...
>   enum register_status get_register_status (int regnum) const;
> ...
> 
> 
> class readable_regcache : public reg_buffer
> ...
> 
> 
> class detached_regcache : public readable_regcache
> ...
>   void raw_supply (int regnum, const void *buf);
> ...
> 
> 
> class regcache : public detached_regcache
> ...
>   void raw_collect (int regnum, void *buf) const;
> ...
> 
> 
> I don’t think that this would work:
> class regcache : public detached_regcache, common_regcache

I did some quick hacking and it seems to work to have this in common/common-regcache.h

struct reg_buffer_common
{
  virtual ~reg_buffer_common () = default;
  virtual void raw_supply (int regnum, const void *buf) = 0;
  virtual void raw_collect (int regnum, void *buf) const = 0;
  virtual bool raw_compare (int regnum, const void *buf, int offset) const = 0;
  virtual register_status get_register_status (int regnum) const = 0;
};

and make your code in nat/ take a pointer to reg_buffer_common.  gdb's reg_buffer
and gdbserver's regcache should inherit from reg_buffer_common.  I  built
it on other regcache refactor patches I have in the pipeline, so maybe some other
changes in there are needed, maybe not.  I pushed the result on this branch, it's
a bit raw but it should be enough to show the idea.

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=shortlog;h=refs/heads/users/simark/regcache-for-alan

>> Since this returns allocated memory, could we return an RAII object?  Either
>> std::vector, std::unique_ptr or gdb::unique_xmalloc_ptr.
> 
> Yes, I’ll update with one of those. Is there any documentation explaining
> when to use which of these?

I don't think so, if you think it would be relevant we could add it to the wiki.

In short:

std::vector: Allocates and manages its own memory.  This is good when we allocate
the space ourselves in GDB, and is especially good for buffers that need to change
size over time.  By default, an std::vector<gdb_byte> would zero-out the buffer
on allocation.  To avoid that when we don't need it, we have a specialization,
gdb::byte_vector, that leaves the memory uninitialized.  You could

gdb::byte_vector buf (iovec.iov_len);
iovec.iov_base = buf.data ();

...

return buf;

std::unique_ptr: It is used to wrap a pointer to something that has been allocated
with "new", it will free it with "delete".  You could do

std::unique_ptr<gdb_byte> buf (new gdb_byte[iovec.iov_len]);
iovec.iov_base = buf.get ()

...

return buf;

gdb::unique_xmalloc_ptr: It is like std::unique_ptr, but frees the memory using
xfree.  It is useful to gradually add some RAII to GDB code that uses xmalloc.

For new code, I would either use std::vector or std::unique_ptr.

I just noticed that in the code in the current patch leaks the allocated memory
if ptrace fails and we call perror_with_name.  Using an object that manages
memory will fix that.

>> When fetching registers, won't it be usually to fill a shiny new, empty regcache?
>> In that case, won't it always fall into the "if" branch?
>>
>> In any case, should we check the status of the VG register to make sure it's REG_VALID
>> before we try to collect it?
> 
> I thought the same regcache was used each time registers needed reading?
> I’ll double check this.
> Either way, yes, should probably check REG_VALID

Hmm I'm not sure either.  Indeed it would make sense that we re-use the same regcache.
Thanks for double-checking.

Thanks,

Simon
  
Alan Hayward June 4, 2018, 3:49 p.m. UTC | #5
>>>> diff --git a/gdb/nat/aarch64-sve-linux-ptrace.c b/gdb/nat/aarch64-sve-linux-ptrace.c

>>>> index 9381786fda..84c7a41f40 100644

>>>> --- a/gdb/nat/aarch64-sve-linux-ptrace.c

>>>> +++ b/gdb/nat/aarch64-sve-linux-ptrace.c

>>>> @@ -25,6 +25,13 @@

>>>> #include "aarch64-sve-linux-ptrace.h"

>>>> #include "arch/aarch64.h"

>>>> 

>>>> +#ifndef GDBSERVER

>>>> +#include "defs.h"

>>>> +#endif

>>>> +#include "regcache.h"

>>> 

>>> Hmm we try not add any more "#ifdef GDBSERVER" in the common code.

>>> 

>>> https://sourceware.org/gdb/wiki/Common#Header_files_in_common_code_.28defs.h_vs_server.h.2C_etc..29

>>> 

>>> Instead, we should try defining a common interface (probably in common/common-regcache.h?) that the

>>> common code will use, and that regcaches from GDB and GDBserver will implement.

>> 

>> I tried using common-defs.h, but gdb/regcache.h requires defines from

>> defs.h - RequireLongest and maybe others.

>> Putting defs.h at the top of gdb/regcache.h then broke in a weird way.

>> A lot of fiddling later, and I hadn’t found a way to make it work.

>> 

>> Creating common/common-regcache.h gets a bit odd because, the functions

>> I need for gdbserver (raw_supply, raw_collect and get_register_status)

>> on gdb come from:

>> 

>> 

>> class reg_buffer

>> ...

>>  enum register_status get_register_status (int regnum) const;

>> ...

>> 

>> 

>> class readable_regcache : public reg_buffer

>> ...

>> 

>> 

>> class detached_regcache : public readable_regcache

>> ...

>>  void raw_supply (int regnum, const void *buf);

>> ...

>> 

>> 

>> class regcache : public detached_regcache

>> ...

>>  void raw_collect (int regnum, void *buf) const;

>> ...

>> 

>> 

>> I don’t think that this would work:

>> class regcache : public detached_regcache, common_regcache

> 

> I did some quick hacking and it seems to work to have this in common/common-regcache.h

> 

> struct reg_buffer_common

> {

>  virtual ~reg_buffer_common () = default;

>  virtual void raw_supply (int regnum, const void *buf) = 0;

>  virtual void raw_collect (int regnum, void *buf) const = 0;

>  virtual bool raw_compare (int regnum, const void *buf, int offset) const = 0;

>  virtual register_status get_register_status (int regnum) const = 0;

> };

> 

> and make your code in nat/ take a pointer to reg_buffer_common.  gdb's reg_buffer

> and gdbserver's regcache should inherit from reg_buffer_common.  I  built

> it on other regcache refactor patches I have in the pipeline, so maybe some other

> changes in there are needed, maybe not.  I pushed the result on this branch, it's

> a bit raw but it should be enough to show the idea.

> 

> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=shortlog;h=refs/heads/users/simark/regcache-for-alan


Thanks for trying this out, however I tried adding your changes
into my build, but that results in the following:

../../src/binutils-gdb/gdb/linux-fork.c: In function ‘void fork_save_infrun_state(fork_info*, int)’:
../../src/binutils-gdb/gdb/linux-fork.c:298:75: error: invalid new-expression of abstract class type ‘readonly_detached_regcache’
   fp->savedregs = new readonly_detached_regcache (*get_current_regcache ());
                                                                           ^
In file included from ../../src/binutils-gdb/gdb/gdbarch.h:70:0,
                 from ../../src/binutils-gdb/gdb/defs.h:531,
                 from ../../src/binutils-gdb/gdb/linux-fork.c:20:
../../src/binutils-gdb/gdb/regcache.h:367:7: note:   because the following virtual functions are pure within ‘readonly_detached_regcache’:
 class readonly_detached_regcache : public readable_regcache
       ^
In file included from ../../src/binutils-gdb/gdb/regcache.h:23:0,
                 from ../../src/binutils-gdb/gdb/gdbarch.h:70,
                 from ../../src/binutils-gdb/gdb/defs.h:531,
                 from ../../src/binutils-gdb/gdb/linux-fork.c:20:
../../src/binutils-gdb/gdb/common/common-regcache.h:68:16: note: 	virtual void reg_buffer_common::raw_supply(int, const void*)
   virtual void raw_supply (int regnum, const void *buf) = 0;
                ^
../../src/binutils-gdb/gdb/common/common-regcache.h:69:16: note: 	virtual void reg_buffer_common::raw_collect(int, void*) const
   virtual void raw_collect (int regnum, void *buf) const = 0;


And then similar errors trying to create a register_dump_reg_buffer.


I could then add the following to readonly_detached_regcache:

  void raw_supply (int regnum, const void *buf) override
  { gdb_assert(false); }

  void raw_collect (int regnum, void *buf) const override
  { gdb_assert(false); }

...Or even make the methods in reg_buffer_common have a
{ gdb_assert(false); } body.

But that feels wrong, as it’s breaking the nice regcache abstractions.

Any thoughts?

Alan.
  

Patch

diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
index 1270b7e0c0..bc570ade6d 100644
--- a/gdb/aarch64-linux-nat.c
+++ b/gdb/aarch64-linux-nat.c
@@ -388,19 +388,65 @@  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)
+{
+  gdb_byte *base = aarch64_sve_get_sveregs (ptid_get_lwp (inferior_ptid));
+  aarch64_sve_regs_copy_to_regcache (regcache, base);
+  xfree (base);
+}
+
+/* Store to the current thread the valid sve register
+   values in the GDB's register array.  */
+
+static void
+store_sveregs_to_thread (struct regcache *regcache)
+{
+  gdb_byte *base;
+  int ret, tid;
+  struct iovec iovec;
+
+  tid = ptid_get_lwp (inferior_ptid);
+
+  /* Obtain a dump of SVE registers from ptrace.  */
+  base = aarch64_sve_get_sveregs (tid);
+
+  /* Overwrite with regcache state.  */
+  aarch64_sve_regs_copy_from_regcache (regcache, base);
+
+  /* Write back to the kernel.  */
+  iovec.iov_base = base;
+  iovec.iov_len = ((struct user_sve_header *) base)->size;
+  ret = ptrace (PTRACE_SETREGSET, tid, NT_ARM_SVE, &iovec);
+  xfree (base);
+
+  if (ret < 0)
+    perror_with_name (_("Unable to store sve registers"));
+}
+
 /* Implement the "fetch_registers" target_ops method.  */
 
 void
 aarch64_linux_nat_target::fetch_registers (struct regcache *regcache,
 					   int regno)
 {
+  struct gdbarch_tdep *tdep = gdbarch_tdep (regcache->arch ());
+
   if (regno == -1)
     {
       fetch_gregs_from_thread (regcache);
-      fetch_fpregs_from_thread (regcache);
+      if (tdep->has_sve ())
+	fetch_sveregs_from_thread (regcache);
+      else
+	fetch_fpregs_from_thread (regcache);
     }
   else if (regno < AARCH64_V0_REGNUM)
     fetch_gregs_from_thread (regcache);
+  else if (tdep->has_sve ())
+    fetch_sveregs_from_thread (regcache);
   else
     fetch_fpregs_from_thread (regcache);
 }
@@ -411,13 +457,20 @@  void
 aarch64_linux_nat_target::store_registers (struct regcache *regcache,
 					   int regno)
 {
+  struct gdbarch_tdep *tdep = gdbarch_tdep (regcache->arch ());
+
   if (regno == -1)
     {
       store_gregs_to_thread (regcache);
-      store_fpregs_to_thread (regcache);
+      if (tdep->has_sve ())
+	store_sveregs_to_thread (regcache);
+      else
+	store_fpregs_to_thread (regcache);
     }
   else if (regno < AARCH64_V0_REGNUM)
     store_gregs_to_thread (regcache);
+  else if (tdep->has_sve ())
+    store_sveregs_to_thread (regcache);
   else
     store_fpregs_to_thread (regcache);
 }
diff --git a/gdb/gdbserver/Makefile.in b/gdb/gdbserver/Makefile.in
index c377378809..22ff19a2d2 100644
--- a/gdb/gdbserver/Makefile.in
+++ b/gdb/gdbserver/Makefile.in
@@ -218,6 +218,7 @@  SFILES = \
 	$(srcdir)/common/tdesc.c \
 	$(srcdir)/common/vec.c \
 	$(srcdir)/common/xml-utils.c \
+	$(srcdir)/nat/aarch64-sve-linux-ptrace.c \
 	$(srcdir)/nat/linux-btrace.c \
 	$(srcdir)/nat/linux-namespaces.c \
 	$(srcdir)/nat/linux-osdata.c \
diff --git a/gdb/nat/aarch64-sve-linux-ptrace.c b/gdb/nat/aarch64-sve-linux-ptrace.c
index 9381786fda..84c7a41f40 100644
--- a/gdb/nat/aarch64-sve-linux-ptrace.c
+++ b/gdb/nat/aarch64-sve-linux-ptrace.c
@@ -25,6 +25,13 @@ 
 #include "aarch64-sve-linux-ptrace.h"
 #include "arch/aarch64.h"
 
+#ifndef GDBSERVER
+#include "defs.h"
+#endif
+#include "regcache.h"
+
+static bool vq_change_warned = false;
+
 /* Read VQ for the given tid using ptrace.  If SVE is not supported then zero
    is returned (on a system that supports SVE, then VQ cannot be zeo).  */
 
@@ -50,3 +57,259 @@  aarch64_sve_get_vq (int tid)
 
   return vq;
 }
+
+/* Read the current SVE register set using ptrace, allocating space as
+   required.  */
+
+gdb_byte *
+aarch64_sve_get_sveregs (int tid)
+{
+  int ret;
+  struct iovec iovec;
+  struct user_sve_header header;
+  long vq = aarch64_sve_get_vq (tid);
+
+  if (vq == 0)
+    perror_with_name (_("Unable to fetch sve register header"));
+
+  /* A ptrace call with NT_ARM_SVE will return a header followed by either a
+     dump of all the SVE and FP registers, or an fpsimd structure (identical to
+     the one returned by NT_FPREGSET) if the kernel has not yet executed any
+     SVE code.  Make sure we allocate enough space for a full SVE dump.  */
+
+  iovec.iov_len = SVE_PT_SIZE (vq, SVE_PT_REGS_SVE);
+  iovec.iov_base = xmalloc (iovec.iov_len);
+
+  ret = ptrace (PTRACE_GETREGSET, tid, NT_ARM_SVE, &iovec);
+  if (ret < 0)
+    perror_with_name (_("Unable to fetch sve registers"));
+
+  return (gdb_byte *) iovec.iov_base;
+}
+
+/* Put the registers from linux structure buf into regcache.  */
+
+void
+aarch64_sve_regs_copy_to_regcache (struct regcache *regcache, const void *buf)
+{
+  char *base = (char*) buf;
+  int i;
+  struct user_sve_header *header = (struct user_sve_header *) buf;
+  long vq, vg_regcache;
+
+  vq = sve_vq_from_vl (header->vl);
+
+  /* Sanity check the data in the header.  */
+  gdb_assert (sve_vl_valid (header->vl));
+  gdb_assert (SVE_PT_SIZE (vq, header->flags) == header->size);
+
+  regcache->raw_collect (AARCH64_SVE_VG_REGNUM, &vg_regcache);
+  if (vg_regcache == 0)
+    {
+      /* VG has not been set.  */
+      vg_regcache = sve_vg_from_vl (header->vl);
+      regcache->raw_supply (AARCH64_SVE_VG_REGNUM, &vg_regcache);
+    }
+  else if (vg_regcache != sve_vg_from_vl (header->vl) && !vq_change_warned)
+    {
+      /* Vector length on the running process has changed.  GDB currently does
+	 not support this and will result in GDB showing incorrect partially
+	 incorrect data for the vector registers.  Warn once and continue.  We
+	 do not expect many programs to exhibit this behaviour.  To fix this
+	 we need to spot the change earlier and generate a new target
+	 descriptor.  */
+      warning (_("Vector length has changed (%ld to %d). "
+		 "Vector registers may show incorrect data."),
+		 vg_regcache, sve_vg_from_vl (header->vl));
+      vq_change_warned = true;
+    }
+
+  if (HAS_SVE_STATE (*header))
+    {
+      /* The register dump contains a set of SVE registers.  */
+
+      for (i = 0; i < AARCH64_SVE_Z_REGS_NUM; i++)
+	regcache->raw_supply (AARCH64_SVE_Z0_REGNUM + i,
+		    base + SVE_PT_SVE_ZREG_OFFSET (vq, i));
+
+      for (i = 0; i < AARCH64_SVE_P_REGS_NUM; i++)
+	regcache->raw_supply (AARCH64_SVE_P0_REGNUM + i,
+		    base + SVE_PT_SVE_PREG_OFFSET (vq, i));
+
+      regcache->raw_supply (AARCH64_SVE_FFR_REGNUM,
+		  base + SVE_PT_SVE_FFR_OFFSET (vq));
+      regcache->raw_supply (AARCH64_FPSR_REGNUM,
+		  base + SVE_PT_SVE_FPSR_OFFSET (vq));
+      regcache->raw_supply (AARCH64_FPCR_REGNUM,
+		  base + SVE_PT_SVE_FPCR_OFFSET (vq));
+    }
+  else
+    {
+      /* There is no SVE state yet - the register dump contains a fpsimd
+	 structure instead.  These registers still exist in the hardware, but
+	 the kernel has not yet initialised them, and so they will be null.  */
+
+      char *zero_reg = (char *) alloca (SVE_PT_SVE_ZREG_SIZE (vq));
+      struct user_fpsimd_state *fpsimd
+	= (struct user_fpsimd_state *)(base + SVE_PT_FPSIMD_OFFSET);
+
+      /* Copy across the V registers from fpsimd structure to the Z registers,
+	 ensuring the non overlapping state is set to null.  */
+
+      memset (zero_reg, 0, SVE_PT_SVE_ZREG_SIZE (vq));
+
+      for (i = 0; i <= AARCH64_SVE_Z_REGS_NUM; i++)
+	{
+	  memcpy (zero_reg, &fpsimd->vregs[i], sizeof (__int128_t));
+	  regcache->raw_supply (AARCH64_SVE_Z0_REGNUM + i, zero_reg);
+	}
+
+      regcache->raw_supply (AARCH64_FPSR_REGNUM, &fpsimd->fpsr);
+      regcache->raw_supply (AARCH64_FPCR_REGNUM, &fpsimd->fpcr);
+
+      /* Clear the SVE only registers.  */
+
+      for (i = 0; i <= AARCH64_SVE_P_REGS_NUM; i++)
+	regcache->raw_supply (AARCH64_SVE_P0_REGNUM + i, zero_reg);
+
+      regcache->raw_supply (AARCH64_SVE_FFR_REGNUM, zero_reg);
+    }
+}
+
+
+/* Put the registers from regcache into linux structure buf.  */
+
+void
+aarch64_sve_regs_copy_from_regcache (struct regcache *regcache, void *buf)
+{
+  struct user_sve_header *header = (struct user_sve_header *) buf;
+  char *base = (char*) buf;
+  long vq, vg_regcache;
+  int i;
+
+  vq = sve_vq_from_vl (header->vl);
+
+  /* Sanity check the data in the header.  */
+  gdb_assert (sve_vl_valid (header->vl));
+  gdb_assert (SVE_PT_SIZE (vq, header->flags) == header->size);
+
+  regcache->raw_collect (AARCH64_SVE_VG_REGNUM, &vg_regcache);
+  if (vg_regcache != 0 && vg_regcache != sve_vg_from_vl (header->vl))
+    /* Vector length on the running process has changed.  GDB currently does
+       not support this and will result in GDB writing invalid data back to the
+       vector registers.  Error and exit.  We do not expect many programs to
+       exhibit this behaviour.  To fix this we need to spot the change earlier
+       and generate a new target descriptor.  */
+    error (_("Vector length has changed. Cannot write back registers."));
+
+  if (!HAS_SVE_STATE (*header))
+    {
+      /* There is no SVE state yet - the register dump contains a fpsimd
+	 structure instead.  Where possible we want to write the regcache data
+	 back to the kernel using the fpsimd structure.  However, if we cannot
+	 then we'll need to reformat the fpsimd into a full SVE structure,
+	 resulting in the initialization of SVE state written back to the
+	 kernel, which is why we try to avoid it.  */
+
+      int has_sve_state = 0;
+      char *zero_reg = (char *) alloca (SVE_PT_SVE_ZREG_SIZE (vq));
+      struct user_fpsimd_state *fpsimd
+	= (struct user_fpsimd_state *)(base + SVE_PT_FPSIMD_OFFSET);
+
+      memset (zero_reg, 0, SVE_PT_SVE_ZREG_SIZE (vq));
+
+      /* Check in the regcache if any of the Z registers are set after the
+	 first 128 bits, or if any of the other SVE registers are set.  */
+
+      for (i = 0; i < AARCH64_SVE_Z_REGS_NUM; i++)
+	{
+	  has_sve_state |= regcache->raw_compare (AARCH64_SVE_Z0_REGNUM + i,
+					zero_reg, sizeof (__int128_t));
+	  if (has_sve_state != 0)
+	    break;
+	}
+
+      if (has_sve_state == 0)
+	for (i = 0; i < AARCH64_SVE_P_REGS_NUM; i++)
+	  {
+	    has_sve_state |= regcache->raw_compare (AARCH64_SVE_P0_REGNUM + i,
+					  zero_reg, 0);
+	    if (has_sve_state != 0)
+	      break;
+	  }
+
+      if (has_sve_state == 0)
+	  has_sve_state |= regcache->raw_compare (AARCH64_SVE_FFR_REGNUM,
+					zero_reg, 0);
+
+      /* If no SVE state exists, then use the existing fpsimd structure to
+	 write out state and return.  */
+
+      if (has_sve_state == 0)
+	{
+	  /* The collects of the Z registers will overflow the size of a vreg.
+	     There is enough space in the structure to allow for this, but we
+	     cannot overflow into the next register as we might not be
+	     collecting every register.  */
+
+	  for (i = 0; i < AARCH64_SVE_Z_REGS_NUM; i++)
+	    {
+	      if (REG_VALID == regcache->get_register_status (
+					   AARCH64_SVE_Z0_REGNUM + i))
+		{
+		  regcache->raw_collect (AARCH64_SVE_Z0_REGNUM + i, zero_reg);
+		  memcpy (&fpsimd->vregs[i], zero_reg, sizeof (__int128_t));
+		}
+	    }
+
+	  if (REG_VALID == regcache->get_register_status (AARCH64_FPSR_REGNUM))
+	    regcache->raw_collect (AARCH64_FPSR_REGNUM, &fpsimd->fpsr);
+	  if (REG_VALID == regcache->get_register_status (AARCH64_FPCR_REGNUM))
+	    regcache->raw_collect ( AARCH64_FPCR_REGNUM, &fpsimd->fpcr);
+
+	  return;
+	}
+
+      /* Otherwise, reformat the fpsimd structure into a full SVE set, by
+	 expanding the V registers (working backwards so we don't splat
+	 registers before they are copied) and using null for everything else.
+	 Note that enough space for a full SVE dump was originally allocated
+	 for base.  */
+
+      header->flags |= SVE_PT_REGS_SVE;
+      header->size = SVE_PT_SIZE (vq, SVE_PT_REGS_SVE);
+
+      memcpy (base + SVE_PT_SVE_FPSR_OFFSET (vq), &fpsimd->fpsr,
+	      sizeof (uint32_t));
+      memcpy (base + SVE_PT_SVE_FPCR_OFFSET (vq), &fpsimd->fpcr,
+	      sizeof (uint32_t));
+
+      for (i = AARCH64_SVE_Z_REGS_NUM; i >= 0 ; i--)
+	{
+	  memcpy (base + SVE_PT_SVE_ZREG_OFFSET (vq, i), &fpsimd->vregs[i],
+		  sizeof (__int128_t));
+	}
+    }
+
+  /* Replace the kernel values with those from regcache.  */
+
+  for (i = 0; i < AARCH64_SVE_Z_REGS_NUM; i++)
+    if (REG_VALID == regcache->get_register_status (AARCH64_SVE_Z0_REGNUM + i))
+      regcache->raw_collect (AARCH64_SVE_Z0_REGNUM + i,
+		   base + SVE_PT_SVE_ZREG_OFFSET (vq, i));
+
+  for (i = 0; i < AARCH64_SVE_P_REGS_NUM; i++)
+    if (REG_VALID == regcache->get_register_status (AARCH64_SVE_P0_REGNUM + i))
+      regcache->raw_collect (AARCH64_SVE_P0_REGNUM + i,
+		   base + SVE_PT_SVE_PREG_OFFSET (vq, i));
+
+  if (REG_VALID == regcache->get_register_status (AARCH64_SVE_FFR_REGNUM))
+    regcache->raw_collect (AARCH64_SVE_FFR_REGNUM,
+		 base + SVE_PT_SVE_FFR_OFFSET (vq));
+  if (REG_VALID == regcache->get_register_status (AARCH64_FPSR_REGNUM))
+    regcache->raw_collect (AARCH64_FPSR_REGNUM,
+		 base + SVE_PT_SVE_FPSR_OFFSET (vq));
+  if (REG_VALID == regcache->get_register_status (AARCH64_FPCR_REGNUM))
+    regcache->raw_collect (AARCH64_FPCR_REGNUM,
+		 base + SVE_PT_SVE_FPCR_OFFSET (vq));
+}
diff --git a/gdb/nat/aarch64-sve-linux-ptrace.h b/gdb/nat/aarch64-sve-linux-ptrace.h
index b318150ac1..eff90ea2ea 100644
--- a/gdb/nat/aarch64-sve-linux-ptrace.h
+++ b/gdb/nat/aarch64-sve-linux-ptrace.h
@@ -34,10 +34,31 @@ 
 
 extern unsigned long aarch64_sve_get_vq (int tid);
 
+/* Read the current SVE register set using ptrace, allocating space as
+   required.  */
+
+extern gdb_byte *aarch64_sve_get_sveregs (int tid);
+
+/* Put the registers from linux structure buf into regcache.  */
+
+extern void aarch64_sve_regs_copy_to_regcache (struct regcache *regcache,
+					       const void *buf);
+
+/* Put the registers from regcache into linux structure buf.  */
+
+extern void aarch64_sve_regs_copy_from_regcache (struct regcache *regcache,
+						 void *buf);
+
 /* Structures and defines taken from sigcontext.h.  */
 
 #ifndef SVE_SIG_ZREGS_SIZE
 
+struct sve_context {
+   struct _aarch64_ctx head;
+   __u16 vl;
+   __u16 __reserved[3];
+};
+
 #define SVE_VQ_BYTES		16	/* number of bytes per quadword */
 
 #define SVE_VQ_MIN		1
@@ -52,6 +73,35 @@  extern unsigned long aarch64_sve_get_vq (int tid);
 #define sve_vl_valid(vl) \
 	((vl) % SVE_VQ_BYTES == 0 && (vl) >= SVE_VL_MIN && (vl) <= SVE_VL_MAX)
 
+#define SVE_SIG_ZREG_SIZE(vq) ((__u32)(vq) * SVE_VQ_BYTES)
+#define SVE_SIG_PREG_SIZE(vq) ((__u32)(vq) * (SVE_VQ_BYTES / 8))
+#define SVE_SIG_FFR_SIZE(vq)  SVE_SIG_PREG_SIZE(vq)
+
+#define SVE_SIG_REGS_OFFSET               \
+   ((sizeof(struct sve_context) + (SVE_VQ_BYTES - 1)) \
+      / SVE_VQ_BYTES * SVE_VQ_BYTES)
+
+#define SVE_SIG_ZREGS_OFFSET  SVE_SIG_REGS_OFFSET
+#define SVE_SIG_ZREG_OFFSET(vq, n) \
+   (SVE_SIG_ZREGS_OFFSET + SVE_SIG_ZREG_SIZE(vq) * (n))
+#define SVE_SIG_ZREGS_SIZE(vq) \
+   (SVE_SIG_ZREG_OFFSET(vq, SVE_NUM_ZREGS) - SVE_SIG_ZREGS_OFFSET)
+
+#define SVE_SIG_PREGS_OFFSET(vq) \
+   (SVE_SIG_ZREGS_OFFSET + SVE_SIG_ZREGS_SIZE(vq))
+#define SVE_SIG_PREG_OFFSET(vq, n) \
+   (SVE_SIG_PREGS_OFFSET(vq) + SVE_SIG_PREG_SIZE(vq) * (n))
+#define SVE_SIG_PREGS_SIZE(vq) \
+   (SVE_SIG_PREG_OFFSET(vq, SVE_NUM_PREGS) - SVE_SIG_PREGS_OFFSET(vq))
+
+#define SVE_SIG_FFR_OFFSET(vq) \
+   (SVE_SIG_PREGS_OFFSET(vq) + SVE_SIG_PREGS_SIZE(vq))
+
+#define SVE_SIG_REGS_SIZE(vq) \
+   (SVE_SIG_FFR_OFFSET(vq) + SVE_SIG_FFR_SIZE(vq) - SVE_SIG_REGS_OFFSET)
+
+#define SVE_SIG_CONTEXT_SIZE(vq) (SVE_SIG_REGS_OFFSET + SVE_SIG_REGS_SIZE(vq))
+
 #endif /* SVE_SIG_ZREGS_SIZE.  */
 
 
@@ -68,6 +118,76 @@  struct user_sve_header {
 	__u16 __reserved;
 };
 
+
+#define SVE_PT_REGS_MASK      1
+
+#define SVE_PT_REGS_FPSIMD    0
+#define SVE_PT_REGS_SVE       SVE_PT_REGS_MASK
+
+#define SVE_PT_VL_INHERIT     (PR_SVE_VL_INHERIT >> 16)
+#define SVE_PT_VL_ONEXEC      (PR_SVE_SET_VL_ONEXEC >> 16)
+
+#define SVE_PT_REGS_OFFSET             \
+   ((sizeof(struct sve_context) + (SVE_VQ_BYTES - 1)) \
+      / SVE_VQ_BYTES * SVE_VQ_BYTES)
+
+#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) +  \
+         (SVE_VQ_BYTES - 1))        \
+      / SVE_VQ_BYTES * SVE_VQ_BYTES)
+#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 + (SVE_VQ_BYTES - 1)) \
+      / SVE_VQ_BYTES * SVE_VQ_BYTES)
+
+#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))
+
 #endif /* SVE_PT_SVE_ZREG_SIZE.  */
 
+
+/* Indicates whether a SVE ptrace header is followed by SVE registers or a
+   fpsimd structure.  */
+
+#define HAS_SVE_STATE(header) ((header).flags && SVE_PT_REGS_SVE)
+
+
 #endif /* aarch64-sve-linux-ptrace.h */