From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 53408 invoked by alias); 6 Dec 2016 10:54:24 -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 53386 invoked by uid 89); 6 Dec 2016 10:54:23 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-4.6 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_NONE,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy=1896, 189,6, corp, H*RU:HELO X-HELO: mga05.intel.com Received: from mga05.intel.com (HELO mga05.intel.com) (192.55.52.43) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 06 Dec 2016 10:54:12 +0000 Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga105.fm.intel.com with ESMTP; 06 Dec 2016 02:54:11 -0800 X-ExtLoop1: 1 Received: from msturm-mobl2.ger.corp.intel.com (HELO [172.28.205.49]) ([172.28.205.49]) by fmsmga006.fm.intel.com with ESMTP; 06 Dec 2016 02:54:09 -0800 Subject: Re: [PATCH v2 5/5] Add support for Intel PKRU register to GDB and GDBserver. To: Luis Machado , mark.kettenis@xs4all.nl, palves@redhat.com, eliz@gnu.org References: <1480599538-30543-1-git-send-email-michael.sturm@intel.com> <1480599538-30543-6-git-send-email-michael.sturm@intel.com> Cc: gdb-patches@sourceware.org From: "Sturm, Michael" Message-ID: <584698D0.90003@intel.com> Date: Tue, 06 Dec 2016 10:54:00 -0000 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.5.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="windows-1252"; format="flowed" Content-Transfer-Encoding: quoted-printable X-SW-Source: 2016-12/txt/msg00150.txt.bz2 Hello Luis, thanks for your feedback. I've made some changes based on your remarks.=20 I will send a new revision of the patch series shortly. Regarding your question on the 2688 constant definition, as for the=20 other xsave feature data structures, there is no #define statement for that number. I think it is out of the=20 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=20 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=20 >> register is >> + added by the Memory Protection Keys for Userspace feature which=20 >> 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 =3D 16; >> static const int num_avx512_zmmh_high_registers =3D 16; >> static const int num_avx512_ymmh_registers =3D 16; >> static const int num_avx512_xmm_registers =3D 16; >> +static const int num_pkeys_registers =3D 1; >> >> /* Note: These functions preserve the reserved bits in control=20 >> 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=20 >> 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=20 >> 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 =3D I386_K0_REGNUM + 7, >> I386_ZMM0H_REGNUM, /* %zmm0h */ >> - I386_ZMM7H_REGNUM =3D I386_ZMM0H_REGNUM + 7 >> + I386_ZMM7H_REGNUM =3D 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=20 >> *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[] =3D >> #define XSAVE_AVX512_ZMM_H_ADDR(tdep, xsave, regnum) \ >> (xsave + xsave_avx512_zmm_h_offset[regnum - I387_ZMM0H_REGNUM=20 >> (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[] =3D >> +{ >> +2688 + 0 * 8 /* %pkru (64 bits in XSTATE, 32-bit actually=20 >> used by > > Is the constant 2688 something we already #define-ed? > >> diff --git a/gdb/testsuite/gdb.arch/i386-pkru.c=20 >> 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=20 >> . */ >> + >> +#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 =3D 0; >> + unsigned int pkru; >> + >> + asm volatile (".byte 0x0f,0x01,0xee\n\t" >> + : "=3Da" (eax), "=3Dd" (edx) >> + : "c" (ecx)); >> + pkru =3D eax; >> + return pkru; >> +} >> + >> +static inline void >> +wrpkru (unsigned int pkru) >> +{ >> + unsigned int eax =3D pkru; >> + unsigned int ecx =3D 0; >> + unsigned int edx =3D 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) =3D=3D bit_OSXSAVE) >> + { >> + if (__get_cpuid_max (0, NULL) < 7) >> + return 0; >> + >> + __cpuid_count (7, 0, eax, ebx, ecx, edx); >> + >> + if ((ecx & bit_PKU) =3D=3D 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 =3D 0x12345678; >> + unsigned int rd_value =3D 0x0; >> + >> + if (have_pkru ()) >> + { >> + wrpkru (wr_value); >> + asm ("nop\n\t"); /* break here 1. */ >> + >> + rd_value =3D rdpkru (); >> + asm ("nop\n\t"); /* break here 2. */ >> + } >> + return 0; >> +} >> diff --git a/gdb/testsuite/gdb.arch/i386-pkru.exp=20 >> 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=3D${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 ".. =3D 1\r\n$gdb_prompt $" { >> + pass $test >> + set supports_pkru 1 >> + } >> + -re ".. =3D 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=20 >> register" >> + >> +# set test_string ".*0x44444444.*" >> +gdb_test "print /x \$pkru =3D 0x44444444" "=3D 0x44444444" "set pkru va= lue" >> +gdb_test "info register pkru" ".*pkru.*0x44444444.*" "read value=20 >> 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=20 > 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