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 v2 5/5] Add support for Intel PKRU register to GDB and GDBserver.


Hello Luis,

thanks for your feedback. I've made some changes based on your remarks. I will send a new revision
of the patch series shortly.

Regarding your question on the 2688 constant definition, as for the other xsave feature data structures, there is no #define statement for that number. I think it is out of the scope of this patch series to fix all the definitions. But I agree that it may make sense to change this should there be a general rework of how xstate is handled in GDB.

Thanks and Regards,
Michael

On 02/12/2016 02:59, Luis Machado wrote:
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.

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928


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