Re: [PATCH 05/12] gdb, gdbserver: Use xstate_bv for target description creation on x86.

[Thiago Jung Bauermann <thiago.bauermann@linaro.org>]

"Schimpe, Christina" <christina.schimpe@intel.com> writes:

> The XSAVE features set is organized in state components, which are a set
> of or parts of registers.  So-called XSAVE-supported features are
> organized using state-component bitmaps, each bit corresonding to a

Typo: "corresponding"

> single state component.
>
> The SDM uses the term xstate_bv for a state-component bitmap, which is
> defined as XCR0 | IA32_XSS.  The control register XCR0 only contains a
> state-component bitmap that specifies user state components, while IA32_XSS
> contains a state-component bitmap that specifies supervisor state components.
>
> Until now, XCR0 is used as input for target description creation in GDB.
> However, a following patch will add userspace support for the CET shadow
> stack feature by Intel.  The CET state is configured in IA32_XSS and consists
> of 2 state components:
> - State component 11 used for the 2 MSRs controlling user-mode
>   functionality for CET (CET_U state)
> - State component 12 used for the 3 MSRs containing shadow-stack pointers
>   for privilege levels 0-2 (CET_S state).
>
> Reading the CET shadow stack pointer register on linux requires a separate
> ptrace call using NT_X86_SHSTK.  To pass the CET shadow stack enablement
> state we would like to pass the xstate_bv value instead of xcr0 for target
> description creation.  To prepare for that, we rename the xcr0 mask
> values for target description creation to xstate_bv.  However, this
> patch doesn't add any functional changes in GDB.
>
> Future states specified in IA32_XSS such as CET will create a combined
> xstate_bv_mask including xcr0 register value and its corresponding bit in
> the state component bitmap.  This combined mask will then be used to create
> the target descriptions.
> ---
>  gdb/amd64-tdep.c                    | 14 ++++----
>  gdb/amd64-tdep.h                    |  8 +++--
>  gdb/arch/amd64-linux-tdesc.c        | 33 ++++++++---------
>  gdb/arch/amd64-linux-tdesc.h        |  7 ++--
>  gdb/arch/amd64.c                    | 15 ++++----
>  gdb/arch/amd64.h                    | 10 ++++--
>  gdb/arch/i386-linux-tdesc.c         | 29 ++++++++-------
>  gdb/arch/i386-linux-tdesc.h         |  5 +--
>  gdb/arch/i386.c                     | 15 ++++----
>  gdb/arch/i386.h                     |  8 ++++-
>  gdb/arch/x86-linux-tdesc-features.c | 55 +++++++++++++++--------------
>  gdb/arch/x86-linux-tdesc-features.h | 25 +++++++------
>  gdb/i386-tdep.c                     | 14 ++++----
>  gdb/i386-tdep.h                     |  7 ++--
>  gdb/nat/x86-linux-tdesc.c           | 18 +++++-----
>  gdb/nat/x86-linux-tdesc.h           |  7 ++--
>  gdb/x86-linux-nat.c                 | 11 ++++--
>  gdbserver/i387-fp.cc                | 40 ++++++++++-----------
>  gdbserver/linux-amd64-ipa.cc        | 10 +++---
>  gdbserver/linux-i386-ipa.cc         |  6 ++--
>  gdbserver/linux-x86-low.cc          |  9 +++--
>  gdbsupport/x86-xstate.h             |  4 ++-
>  22 files changed, 196 insertions(+), 154 deletions(-)

Just some minor suggestions in comments.

Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>

> diff --git a/gdb/arch/x86-linux-tdesc-features.c b/gdb/arch/x86-linux-tdesc-features.c
> index f6eb112f312..3f041ece6eb 100644
> --- a/gdb/arch/x86-linux-tdesc-features.c
> +++ b/gdb/arch/x86-linux-tdesc-features.c
> @@ -28,18 +28,21 @@
>
>     We want to cache target descriptions, and this is currently done in
>     three separate caches, one each for i386, amd64, and x32.  Additionally,
> -   the caching we're discussing here is Linux only, and for Linux, the only
> -   thing that has an impact on target description creation is the xcr0
> -   value.
> +   the caching we're discussing here is Linux only.  Currently for linux,

Nit: "Linux"

> +   the only thing that has an impact on target description creation are
> +   the supported features in xsave which are modelled by a xstate_bv_mask
> +   value, which has the same format than the state component bitmap.
>
>     In order to ensure the cache functions correctly we need to filter out

I think this should read "filter" rather than "filter out". I understand
the latter to mean that xstate_bv_mask feature bits that are relevant
should be ignored, which isn't what the code does.

> @@ -51,11 +54,13 @@ extern int x86_linux_amd64_tdesc_count ();
>  extern int x86_linux_x32_tdesc_count ();
>  extern int x86_linux_i386_tdesc_count ();
>
> -/* Convert an index number (as returned from x86_linux_xcr0_to_tdesc_idx)
> -   into an xcr0 value which can then be used to create a target
> -   description.  */
> +/* Convert an index number (as returned from
> +   x86_linux_xstate_bv_mask_to_tdesc_idx) into an xstate_bv_mask
> +   value which can then be used to create a target description.
> +   The return mask same format than the state component bitmap and does

Missing words: "The return mask has the same format ..."

> +   include user and supervisor state components.*/
>
> -extern uint64_t x86_linux_tdesc_idx_to_xcr0 (int idx);
> +extern uint64_t x86_linux_tdesc_idx_to_xstate_bv_mask (int idx);
>
>  #endif /* IN_PROCESS_AGENT */

--
Thiago