This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 4/4] Add support for Intel PKRU register to GDB and GDBserver.
- From: Pedro Alves <palves at redhat dot com>
- To: Michael Sturm <michael dot sturm at intel dot com>, mark dot kettenis at xs4all dot nl, eliz at gnu dot org
- Cc: gdb-patches at sourceware dot org
- Date: Fri, 27 May 2016 12:27:35 +0100
- Subject: Re: [PATCH 4/4] Add support for Intel PKRU register to GDB and GDBserver.
- Authentication-results: sourceware.org; auth=none
- References: <1463143833-24399-1-git-send-email-michael dot sturm at intel dot com> <1463143833-24399-5-git-send-email-michael dot sturm at intel dot com>
On 05/13/2016 01:50 PM, Michael Sturm wrote:
> This patch adds support for the registers added by the
> Memory Protection Keys for Userspace (PKU aka PKEYs) feature.
> Native and remote debugging are covered by this patch.
>
> The XSAVE area is extended with a new state containing
> the 32-bit wide PKRU register. The new register is added to
> amd64-avx-mpx_avx512-* tdesc, thus it is renamed accordingly.
Is that really the right thing to do? What about machines that
_don't_ support pkru? Shouldn't we keep the older descriptions
for those?
> Also,
> respective xstate mask X86_XSTATE_AVX_MPX_AVX512_MASK is renamed to
> X86_XSTATE_AVX_MPX_AVX512_PKU_MASK to reflect the new feature set
> it supports.
> (X86_XSTATE_PKRU_SIZE): New makro.
> (HAS_PKRU(XCR0)): New makro.
"macro".
> (i386_linux_gregset_reg_offset): Adde PKRU register.
"Add".
> diff --git a/gdb/gdbserver/linux-amd64-ipa.c b/gdb/gdbserver/linux-amd64-ipa.c
> index 40c6995..1851b60 100644
> --- a/gdb/gdbserver/linux-amd64-ipa.c
> +++ b/gdb/gdbserver/linux-amd64-ipa.c
> @@ -185,7 +185,7 @@ get_ipa_tdesc (int idx)
> case X86_TDESC_AVX_MPX:
> return tdesc_amd64_avx_mpx_linux;
> case X86_TDESC_AVX_MPX_AVX512:
> - return tdesc_amd64_avx_mpx_avx512_linux;
> + return tdesc_amd64_avx_mpx_avx512_pku_linux;
No rename for X86_TDESC_AVX_MPX_AVX512?
> case X86_TDESC_AVX_AVX512:
> return tdesc_amd64_avx_avx512_linux;
> default:
> @@ -219,6 +219,6 @@ initialize_low_tracepoint (void)
> init_registers_amd64_avx_linux ();
> init_registers_amd64_avx_mpx_linux ();
> init_registers_amd64_mpx_linux ();
> - init_registers_amd64_avx_mpx_avx512_linux ();
> + init_registers_amd64_avx_mpx_avx512_pku_linux ();
> init_registers_amd64_avx_avx512_linux ();
> }
> diff --git a/gdb/gdbserver/linux-i386-ipa.c b/gdb/gdbserver/linux-i386-ipa.c
> index 5e00a59..5f1e09d 100644
> --- a/gdb/gdbserver/linux-i386-ipa.c
> +++ b/gdb/gdbserver/linux-i386-ipa.c
> @@ -265,7 +265,7 @@ get_ipa_tdesc (int idx)
> case X86_TDESC_AVX_AVX512:
> return tdesc_i386_avx_avx512_linux;
> case X86_TDESC_AVX_MPX_AVX512:
Ditto.
> - return tdesc_i386_avx_mpx_avx512_linux;
> + return tdesc_i386_avx_mpx_avx512_pku_linux;
> default:
> internal_error (__FILE__, __LINE__,
> "unknown ipa tdesc index: %d", idx);
> diff --git a/gdb/nat/x86-gcc-cpuid.h b/gdb/nat/x86-gcc-cpuid.h
> index 1045521..2283ea3 100644
> --- a/gdb/nat/x86-gcc-cpuid.h
> +++ b/gdb/nat/x86-gcc-cpuid.h
> @@ -84,6 +84,10 @@
> #define bit_AVX512CD (1 << 28)
> #define bit_SHA (1 << 29)
>
> +/* %ecx */
> +#define bit_PKU (1 << 3)
> +#define bit_OSPKE (1 << 4)
> +
This file is imported from gcc, and the upstream file has more bits in
in. Please instead submit a separate, preliminary patch that syncs
our copy with upstream's.
> +static inline unsigned long
> +rdpkru(void)
Space before parens.
> +{
> + unsigned int eax, edx;
> + unsigned int ecx = 0;
> + unsigned int pkru;
> +
> + asm volatile(".byte 0x0f,0x01,0xee\n\t"
Ditto.
> + : "=a" (eax), "=d" (edx)
> + : "c" (ecx));
> + pkru = eax;
> + return pkru;
> +}
> +
> +static inline void
> +wrpkru(unsigned int pkru)
Ditto.
> +{
> + 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));
Ditto.
> +}
> +
> +
> +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;
> + }
> + return 0;
> +}
> +
> +int
> +main (int argc, char **argv)
> +{
> + unsigned int wr_value = 0x12345678;
> + unsigned int rd_value = 0x0;
> +
> + if (have_pkru ())
> + {
> + wrpkru(wr_value);
Ditto.
> + asm ("nop\n\t"); /* break here 1. */
> +
> + rd_value = rdpkru();
Ditto.
> + 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..79d25fb
> --- /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."
> + return
> +}
> +
> +set comp_flags "-I${srcdir}/../nat/"
> +
> +if { [prepare_for_testing ${testfile}.exp ${testfile} ${srcfile} \
> + [list debug nowarnings additional_flags=${comp_flags}]] } {
Why "nowarnings" ?
> + 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
> + }
> +}
> +
> +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" $test_string "pkru formating"
> +
> +# 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$test_string" "read pkru register"
> +
> +set test_string ".*0x44444444.*"
> +gdb_test "print /x \$pkru = 0x44444444" "= 0x44444444" "set pkru value"
> +gdb_test "info register pkru" ".*pkru$test_string" "read value after setting value"
There doesn't seem to be much point to the "test_string" indirection;
it's used only once each time.
Thanks,
Pedro Alves