[PATCH v2 5/5] Add support for Intel PKRU register to GDB and GDBserver.
Sturm, Michael
michael.sturm@intel.com
Tue Dec 6 10:54:00 GMT 2016
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
More information about the Gdb-patches
mailing list