Re: [PATCH 2/8] Function for reading the Aarch64 SVE vector length.

[Simon Marchi <simon dot marchi at ericsson dot com>]

Hi Alan,

LGTM, with some nits.

On 2018-05-11 06:52 AM, Alan Hayward wrote:
> Add a method for reading the SVE vector length using ptrace. This returns
> 0 for systems without SVE support.
> 
> Note the defines taken from Linux kernel headers in aarch64-sve-linux-ptrace.h.
> See the covering email for details about this.

Using ptrace to read the SVE registers (and the SVE register length) will work even
for kernels that didn't have these macros?

> There are multiple ways of expressing the vector length. Thankfully these are
> all wll defined. I've added convertors for going from one to the other.
> Hopefully this will help to prevent future confusion.
Copyright 2017 -> 2018 in the new files.

> 
> 2018-05-11  Alan Hayward  <alan.hayward@arm.com>
> 
> gdb/
> 	* Makefile.in: Add new header.
> 	* gdb/arch/aarch64.h (sve_vg_from_vl): New macro.
> 	(sve_vl_from_vg): Likewise.
> 	(sve_vq_from_vl): Likewise.
> 	(sve_vl_from_vq): Likewise.
> 	(sve_vq_from_vg): Likewise.
> 	(sve_vg_from_vq): Likewise.
> 	* configure.nat: Add new c file.
> 	* nat/aarch64-sve-linux-ptrace.c: New file.
> 	* nat/aarch64-sve-linux-ptrace.h: New file.
> 
> gdbserver/
> 	* configure.srv: Add new c/h file.
> ---
>  gdb/Makefile.in                    |  1 +
>  gdb/arch/aarch64.h                 | 17 +++++++++
>  gdb/configure.nat                  |  2 +-
>  gdb/gdbserver/configure.srv        |  1 +
>  gdb/nat/aarch64-sve-linux-ptrace.c | 52 +++++++++++++++++++++++++++
>  gdb/nat/aarch64-sve-linux-ptrace.h | 73 ++++++++++++++++++++++++++++++++++++++
>  6 files changed, 145 insertions(+), 1 deletion(-)
>  create mode 100644 gdb/nat/aarch64-sve-linux-ptrace.c
>  create mode 100644 gdb/nat/aarch64-sve-linux-ptrace.h
> 
> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
> index 87d74a7703..64042d1bd1 100644
> --- a/gdb/Makefile.in
> +++ b/gdb/Makefile.in
> @@ -1478,6 +1478,7 @@ HFILES_NO_SRCDIR = \
>  	mi/mi-parse.h \
>  	nat/aarch64-linux.h \
>  	nat/aarch64-linux-hw-point.h \
> +	nat/aarch64-sve-linux-ptrace.h \
>  	nat/amd64-linux-siginfo.h \
>  	nat/gdb_ptrace.h \
>  	nat/gdb_thread_db.h \
> diff --git a/gdb/arch/aarch64.h b/gdb/arch/aarch64.h
> index 1846e04163..af0b157c51 100644
> --- a/gdb/arch/aarch64.h
> +++ b/gdb/arch/aarch64.h
> @@ -48,6 +48,23 @@ enum aarch64_regnum
>  #define AARCH64_V_REGS_NUM 32
>  #define AARCH64_NUM_REGS AARCH64_FPCR_REGNUM + 1
>  
> +/* There are a number of ways of expressing the current SVE vector size:
> +
> +   VL : Vector Length.
> +	The number of bytes in an SVE Z register.
> +   VQ : Vector Quotient.
> +	The number of 128bit chunks in an SVE Z register.
> +   VG : Vector Gradient.
> +	The number of 64bit chunks in an SVE Z register.  */
> +
> +#define sve_vg_from_vl(vl)	((vl) / 8)
> +#define sve_vl_from_vg(vg)	((vg) * 8)
> +#define sve_vq_from_vl(vl)	((vl) / 0x10)
> +#define sve_vl_from_vq(vq)	((vq) * 0x10)
> +#define sve_vq_from_vg(vg)	(sve_vq_from_vl (sve_vl_from_vg (vg)))
> +#define sve_vg_from_vq(vq)	(sve_vg_from_vl (sve_vl_from_vq (vq)))
> +
> +
>  /* Maximum supported VQ value.  Increase if required.  */
>  #define AARCH64_MAX_SVE_VQ  16
>  
> diff --git a/gdb/configure.nat b/gdb/configure.nat
> index 6b0f44fede..d7d79adaca 100644
> --- a/gdb/configure.nat
> +++ b/gdb/configure.nat
> @@ -228,7 +228,7 @@ case ${gdb_host} in
>  	    aarch64)
>  		#  Host: AArch64 based machine running GNU/Linux
>  		NATDEPFILES="${NATDEPFILES} aarch64-linux-nat.o \
> -		aarch32-linux-nat.o aarch64-linux-hw-point.o aarch64-linux.o"
> +		aarch32-linux-nat.o aarch64-linux-hw-point.o aarch64-linux.o aarch64-sve-linux-ptrace.o"

Please wrap to 80 columns.

>  		;;
>  	    arm)
>  		# Host: ARM based machine running GNU/Linux
> diff --git a/gdb/gdbserver/configure.srv b/gdb/gdbserver/configure.srv
> index ffeefb9b92..8bf0dcc650 100644
> --- a/gdb/gdbserver/configure.srv
> +++ b/gdb/gdbserver/configure.srv
> @@ -54,6 +54,7 @@ case "${target}" in
>  			srv_tgtobj="$srv_tgtobj arch/aarch64-insn.o"
>  			srv_tgtobj="$srv_tgtobj arch/aarch64.o"
>  			srv_tgtobj="$srv_tgtobj linux-aarch64-tdesc.o"
> +			srv_tgtobj="$srv_tgtobj aarch64-sve-linux-ptrace.o"
>  			srv_tgtobj="${srv_tgtobj} $srv_linux_obj"
>  			srv_linux_regsets=yes
>  			srv_linux_thread_db=yes
> diff --git a/gdb/nat/aarch64-sve-linux-ptrace.c b/gdb/nat/aarch64-sve-linux-ptrace.c
> new file mode 100644
> index 0000000000..9381786fda
> --- /dev/null
> +++ b/gdb/nat/aarch64-sve-linux-ptrace.c
> @@ -0,0 +1,52 @@
> +/* Common target dependent for AArch64 systems.
> +
> +   Copyright (C) 2017 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#include <sys/utsname.h>
> +#include <sys/uio.h>
> +#include "common-defs.h"
> +#include "elf/external.h"
> +#include "elf/common.h"
> +#include "aarch64-sve-linux-ptrace.h"
> +#include "arch/aarch64.h"
> +
> +/* 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).  */

Here, put a reference to the .h as usual.

> +
> +unsigned long
> +aarch64_sve_get_vq (int tid)
> +{
> +  struct iovec iovec;
> +  struct user_sve_header header;
> +
> +  iovec.iov_len = sizeof (header);
> +  iovec.iov_base = &header;
> +
> +  /* Ptrace gives the vector length in bytes.  Convert it to VQ, the number of
> +     128bit chunks in a Z register.  We use VQ because 128bits is the minimum
> +     a Z register can increase in size.  */
> +
> +  if (ptrace (PTRACE_GETREGSET, tid, NT_ARM_SVE, &iovec) < 0)
> +    /* SVE is not supported.  */
> +    return 0;

Add braces here.

> +
> +  long vq = sve_vq_from_vl (header.vl);
> +  gdb_assert (sve_vl_valid (header.vl));

We should avoid gdb_assert for bad input values (including what we receive from the
kernel).  Could we display a warning and return 0?

> +
> +  return vq;
> +}
> diff --git a/gdb/nat/aarch64-sve-linux-ptrace.h b/gdb/nat/aarch64-sve-linux-ptrace.h
> new file mode 100644
> index 0000000000..b318150ac1
> --- /dev/null
> +++ b/gdb/nat/aarch64-sve-linux-ptrace.h
> @@ -0,0 +1,73 @@
> +/* Common target dependent for AArch64 systems.
> +
> +   Copyright (C) 2017 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#ifndef AARCH64_SVE_LINUX_PTRACE_H
> +#define AARCH64_SVE_LINUX_PTRACE_H
> +
> +/* Where indicated, this file contains defines and macros lifted directly from
> +   the Linux kernel headers, with no modification.
> +   Refer to Linux kernel documentation for details.  */
> +
> +#include <asm/sigcontext.h>
> +#include <sys/utsname.h>
> +#include <sys/ptrace.h>
> +#include <asm/ptrace.h>
> +
> +/* 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).  */

zeo -> zero.

> +extern unsigned long aarch64_sve_get_vq (int tid);
> +
> +/* Structures and defines taken from sigcontext.h.  */
> +
> +#ifndef SVE_SIG_ZREGS_SIZE
> +
> +#define SVE_VQ_BYTES		16	/* number of bytes per quadword */
> +
> +#define SVE_VQ_MIN		1
> +#define SVE_VQ_MAX		512
> +
> +#define SVE_VL_MIN		(SVE_VQ_MIN * SVE_VQ_BYTES)
> +#define SVE_VL_MAX		(SVE_VQ_MAX * SVE_VQ_BYTES)
> +
> +#define SVE_NUM_ZREGS		32
> +#define SVE_NUM_PREGS		16
> +
> +#define sve_vl_valid(vl) \
> +	((vl) % SVE_VQ_BYTES == 0 && (vl) >= SVE_VL_MIN && (vl) <= SVE_VL_MAX)
> +
> +#endif /* SVE_SIG_ZREGS_SIZE.  */
> +
> +
> +/* Structures and defines taken from ptrace.h.  */
> +
> +#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;
> +};
> +
> +#endif /* SVE_PT_SVE_ZREG_SIZE.  */
> +
> +#endif /* aarch64-sve-linux-ptrace.h */
>