From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21097 invoked by alias); 2 Dec 2016 02:00:00 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 20896 invoked by uid 89); 2 Dec 2016 01:59:59 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: relay1.mentorg.com Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 02 Dec 2016 01:59:49 +0000 Received: from svr-orw-mbx-03.mgc.mentorg.com ([147.34.90.203]) by relay1.mentorg.com with esmtp id 1cCd8d-0001sj-HX from Luis_Gustavo@mentor.com ; Thu, 01 Dec 2016 17:59:47 -0800 Received: from [172.30.5.15] (147.34.91.1) by svr-orw-mbx-03.mgc.mentorg.com (147.34.90.203) with Microsoft SMTP Server (TLS) id 15.0.1210.3; Thu, 1 Dec 2016 17:59:44 -0800 Subject: Re: [PATCH v2 5/5] Add support for Intel PKRU register to GDB and GDBserver. References: <1480599538-30543-1-git-send-email-michael.sturm@intel.com> <1480599538-30543-6-git-send-email-michael.sturm@intel.com> To: Michael Sturm , , , CC: From: Luis Machado Reply-To: Luis Machado Message-ID: Date: Fri, 02 Dec 2016 02:00:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <1480599538-30543-6-git-send-email-michael.sturm@intel.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit X-ClientProxiedBy: svr-orw-mbx-01.mgc.mentorg.com (147.34.90.201) To svr-orw-mbx-03.mgc.mentorg.com (147.34.90.203) X-IsSubscribed: yes X-SW-Source: 2016-12/txt/msg00064.txt.bz2 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. > + > + 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 . */ > + > +#include > +#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. > +# > +# 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 . > + > +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.