Re: [PATCH v2 5/5] Add support for Intel PKRU register to GDB and GDBserver.

[Luis Machado <lgustavo@codesourcery.com>]

On 12/01/2016 07:38 AM, Michael Sturm wrote:
> diff --git a/gdb/NEWS b/gdb/NEWS
> index a597405..d37c477 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -3,10 +3,16 @@
>
>  *** Changes since GDB 7.12
>
> +* GDB now supports access to the PKU register on GNU/Linux. The register is
> +  added by the Memory Protection Keys for Userspace feature which will be
> +  available in future Intel CPUs.
> +
>  * Building GDB and GDBserver now requires a C++11 compiler.
>
>    For example, GCC 4.8 or later.
>
> +* GDB and GDBserver now require building with a C++ compiler.
> +

Spurious change? Maybe a merge error.


>  # Linux object files.  This is so we don't have to repeat
> diff --git a/gdb/gdbserver/i387-fp.c b/gdb/gdbserver/i387-fp.c
> index d0b0736..6f188d2 100644
> --- a/gdb/gdbserver/i387-fp.c
> +++ b/gdb/gdbserver/i387-fp.c
> @@ -27,6 +27,7 @@ static const int num_avx512_zmmh_low_registers = 16;
>  static const int num_avx512_zmmh_high_registers = 16;
>  static const int num_avx512_ymmh_registers = 16;
>  static const int num_avx512_xmm_registers = 16;
> +static const int num_pkeys_registers = 1;
>
>  /* Note: These functions preserve the reserved bits in control registers.
>     However, gdbserver promptly throws away that information.  */
> @@ -136,6 +137,10 @@ struct i387_xsave {
>
>    /* Space for 16 512-bit zmm16-31 values.  */
>    unsigned char zmmh_high_space[1024];
> +
> +  /* Space for 1 32-bit PKRU register. The HW XSTATE size for this feature is

Two spaces after period.

> diff --git a/gdb/i386-tdep.h b/gdb/i386-tdep.h
> index 672a6cf..c0963b6 100644
> --- a/gdb/i386-tdep.h
> +++ b/gdb/i386-tdep.h
> @@ -189,6 +189,15 @@ struct gdbarch_tdep
>    /* YMM16-31 register names.  Only used for tdesc_numbered_register.  */
>    const char **ymm_avx512_register_names;
>
> +  /* Number of PKEYS registers  */

Period at the end of sentence here ...

> +  int num_pkeys_regs;
> +
> +  /* Register number for PKRU register  */

... here ...
> +  int pkru_regnum;
> +
> +  /* PKEYS register names  */

and here.

> +  const char **pkeys_register_names;
> +
>    /* Target description.  */
>    const struct target_desc *tdesc;
>
> @@ -284,7 +293,8 @@ enum i386_regnum
>    I386_K0_REGNUM,		/* %k0 */
>    I386_K7_REGNUM = I386_K0_REGNUM + 7,
>    I386_ZMM0H_REGNUM,		/* %zmm0h */
> -  I386_ZMM7H_REGNUM = I386_ZMM0H_REGNUM + 7
> +  I386_ZMM7H_REGNUM = I386_ZMM0H_REGNUM + 7,
> +  I386_PKRU_REGNUM
>  };
>
>  /* Register numbers of RECORD_REGMAP.  */
> @@ -324,6 +334,7 @@ enum record_i386_regnum
>  #define I386_AVX_NUM_REGS	(I386_YMM7H_REGNUM + 1)
>  #define I386_MPX_NUM_REGS	(I386_BNDSTATUS_REGNUM + 1)
>  #define I386_AVX512_NUM_REGS	(I386_ZMM7H_REGNUM + 1)
> +#define I386_PKEYS_NUM_REGS	(I386_PKRU_REGNUM + 1)
>
>  /* Size of the largest register.  */
>  #define I386_MAX_REGISTER_SIZE	64
> @@ -345,6 +356,7 @@ extern int i386_bnd_regnum_p (struct gdbarch *gdbarch, int regnum);
>  extern int i386_k_regnum_p (struct gdbarch *gdbarch, int regnum);
>  extern int i386_zmm_regnum_p (struct gdbarch *gdbarch, int regnum);
>  extern int i386_zmmh_regnum_p (struct gdbarch *gdbarch, int regnum);
> +extern int i386_pkru_regnum_p (struct gdbarch *gdbarch, int regnum);
>
>  extern const char *i386_pseudo_register_name (struct gdbarch *gdbarch,
>  					      int regnum);
> diff --git a/gdb/i387-tdep.c b/gdb/i387-tdep.c
> index ef3a631..dcc8f08 100644
> --- a/gdb/i387-tdep.c
> +++ b/gdb/i387-tdep.c
> @@ -888,6 +888,19 @@ static int xsave_avx512_zmm_h_offset[] =
>  #define XSAVE_AVX512_ZMM_H_ADDR(tdep, xsave, regnum) \
>    (xsave + xsave_avx512_zmm_h_offset[regnum - I387_ZMM0H_REGNUM (tdep)])
>
> +/* At xsave_pkeys_offset[REGNUM] you find the offset to the location
> +   of the PKRU register data structure used by the "xsave"
> +   instruction where GDB register REGNUM is stored.  */
> +
> +static int xsave_pkeys_offset[] =
> +{
> +2688 + 0 * 8		/* %pkru (64 bits in XSTATE, 32-bit actually used by

Is the constant 2688 something we already #define-ed?

> diff --git a/gdb/testsuite/gdb.arch/i386-pkru.c b/gdb/testsuite/gdb.arch/i386-pkru.c
> new file mode 100644
> index 0000000..dd8b8f7
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/i386-pkru.c
> @@ -0,0 +1,95 @@
> +/* Test program for PKEYS registers.
> +
> +   Copyright 2015-2016 Free Software Foundation, Inc.
> +
> +   Contributed by Intel Corp. <michael.sturm@intel.com>
> +
> +   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 <stdio.h>
> +#include "x86-cpuid.h"
> +
> +#ifndef NOINLINE
> +#define NOINLINE __attribute__ ((noinline))
> +#endif
> +
> +unsigned int have_pkru (void) NOINLINE;
> +
> +static inline unsigned long
> +rdpkru (void)
> +{
> +  unsigned int eax, edx;
> +  unsigned int ecx = 0;
> +  unsigned int pkru;
> +
> +  asm volatile (".byte 0x0f,0x01,0xee\n\t"
> +               : "=a" (eax), "=d" (edx)
> +               : "c" (ecx));
> +  pkru = eax;
> +  return pkru;
> +}
> +
> +static inline void
> +wrpkru (unsigned int pkru)
> +{
> +  unsigned int eax = pkru;
> +  unsigned int ecx = 0;
> +  unsigned int edx = 0;
> +
> +  asm volatile (".byte 0x0f,0x01,0xef\n\t"
> +               : : "a" (eax), "c" (ecx), "d" (edx));
> +}
> +
> +

Spurious newline.

> +unsigned int NOINLINE
> +have_pkru (void)
> +{
> +  unsigned int eax, ebx, ecx, edx;
> +
> +  if (!__get_cpuid (1, &eax, &ebx, &ecx, &edx))
> +    return 0;
> +
> +  if ((ecx & bit_OSXSAVE) == bit_OSXSAVE)
> +    {
> +      if (__get_cpuid_max (0, NULL) < 7)
> +	return 0;
> +
> +      __cpuid_count (7, 0, eax, ebx, ecx, edx);
> +
> +      if ((ecx & bit_PKU) == bit_PKU)
> +	return 1;
> +      else
> +	return 0;

No need to return 0 here. We can just let it continue and return 0 below?

> +    }
> +  return 0;
> +}
> +
> +int
> +main (int argc, char **argv)
> +{
> +  unsigned int wr_value = 0x12345678;
> +  unsigned int rd_value = 0x0;
> +
> +  if (have_pkru ())
> +    {
> +      wrpkru (wr_value);
> +      asm ("nop\n\t");	/* break here 1.  */
> +
> +      rd_value = rdpkru ();
> +      asm ("nop\n\t");	/* break here 2.  */
> +    }
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.arch/i386-pkru.exp b/gdb/testsuite/gdb.arch/i386-pkru.exp
> new file mode 100644
> index 0000000..f5e3da5
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/i386-pkru.exp
> @@ -0,0 +1,73 @@
> +# Copyright 2015-2016 Free Software Foundation, Inc.
> +#
> +# Contributed by Intel Corp. <michael.sturm@intel.com>
> +#
> +# 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/>.
> +
> +standard_testfile
> +
> +if { ![istarget i?86-*-*] && ![istarget x86_64-*-* ] } {
> +    verbose "Skipping PKEYS tests."

Drop the verbose call and ...

unsupported "skipping x86-specific tests"

or

unsupported "skipping x86 PKEYS tests"

or some other meaningful message.

> +    return
> +}
> +
> +set comp_flags "-I${srcdir}/../nat/"
> +
> +if { [prepare_for_testing ${testfile}.exp ${testfile} ${srcfile} \
> +     [list debug additional_flags=${comp_flags}]] } {

Add 'untested "failed to compile"'

> +    return -1
> +}
> +
> +if ![runto_main] {
> +    untested "could not run to main"
> +    return -1
> +}
> +
> +set supports_pkru 0
> +set test "probe PKRU support"
> +gdb_test_multiple "print have_pkru()" $test {
> +    -re ".. = 1\r\n$gdb_prompt $" {
> +        pass $test
> +        set supports_pkru 1
> +    }
> +    -re ".. = 0\r\n$gdb_prompt $" {
> +        pass $test
> +    }
> +}

I don't think we need to check for $gdb_prompt explicitly here.

> +
> +if { !$supports_pkru } {
> +    unsupported "processor does not support protection key feature."
> +    return
> +}
> +
> +# Test pkru register at startup
> +# set test_string "0"
> +
> +gdb_test "print \$pkru" 0 "pkru formating"
> +

"print pkru register" instead of "pkru formatting"?

> +# Read values from pseudo registers.
> +gdb_breakpoint [ gdb_get_line_number "break here 1" ]
> +gdb_continue_to_breakpoint "break here 1" ".*break here 1.*"
> +
> +# set test_string ".*0x12345678.*"
> +gdb_test "info register pkru" ".*pkru.*0x12345678.*" "read pkru register"
> +
> +# set test_string ".*0x44444444.*"
> +gdb_test "print /x \$pkru = 0x44444444" "= 0x44444444" "set pkru value"
> +gdb_test "info register pkru" ".*pkru.*0x44444444.*" "read value after setting value"
> +
> +gdb_breakpoint [ gdb_get_line_number "break here 2" ]
> +gdb_continue_to_breakpoint "break here 2" ".*break here 2.*"
> +
> +gdb_test "print /x rd_value" ".*" "program variable after reading pkru"

print variable instead of program variable? I may be missing some 
context here.