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

["Schimpe, Christina" <christina.schimpe@intel.com>]

Hi Thiago,

Please see my comments below.

> -----Original Message-----
> From: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
> Sent: Thursday, February 6, 2025 4:10 AM
> To: Schimpe, Christina <christina.schimpe@intel.com>
> Cc: gdb-patches@sourceware.org
> Subject: Re: [PATCH 05/12] gdb, gdbserver: Use xstate_bv for target description
> creation on x86.
> 
> 
> "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"

Will fix.

> > 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"

Will fix.

> > +   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.

I think you are right.
I could even make it a separate generic patch since "to filter out" was used 
before already. But I think it's also ok, to just fix it with that patch.

> > @@ -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 ..."

Will fix this and all your comments above for the next version. 
Also I'll add your Reviewed-by tag.

Thanks!
Christina
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928