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

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

[-- Attachment #1: Type: text/plain, Size: 7685 bytes --]

Hi Andrew,

Thanks a lot for the review.

>> The XSAVE features set is organized in state components, which are a set
>> of or parts of registers.  So-called XSAVE-supported features are

> Maybe here you mean: "..., which are a set of, or parts of, registers."
> If not, then this sentence doesn't make sense to me

I agree, this sentence is a bit confusing. I think I would prefer to write it out actually.
Is the following more understandable?
"The XSAVE function set is organized in state components, which are a set of registers
or parts of registers."

>> The SDM uses the term xstate_bv for a state-component bitmap, which is

> Would it be possible to define what SDM it please.

Do you mean writing it out to "Intel Software Developer’s Manual" ?

> I'd like to ask about the use of 'mask' in this variable name.  We used
> to pass in an xcr0 value, which was then combined with various masks to
> extract the state bits we were interested in.

> You've renamed the new variable as a mask, but continue to combine it
> with the same masks (e.g. X86_XSTATE_AVX), which makes me suspect the
> variable is not a mask at all.

It's not a mask at all. I honestly cannot tell why I decided to call this xstate_bv
instead of xstate_bv_mask when I wrote this patch...

> Now, I can see where the confusion might have come from.  Almost every
> call to amd64_target_description passes in a *_MASK constant.  But
> that's OK, given these are bit sets, then the MASK constants actually
> define valid values.

> I am aware that this sounds like a trivial complaint over a variable
> name, but I think calling this a mask is pretty confusing (at least to
> me), so unless I'm not understanding this, then I think it is worth
> renaming this.

> The use of `mask` for the value is present throughout this patch, not
> just this one function.

I agree and am glad for your feedback here.
The current naming is confusing and I will rename it to xstate_bv.

Christina

________________________________
From: Andrew Burgess <aburgess@redhat.com>
Sent: Monday, July 14, 2025 3:52 PM
To: Schimpe, Christina <christina.schimpe@intel.com>; gdb-patches@sourceware.org <gdb-patches@sourceware.org>
Cc: thiago.bauermann@linaro.org <thiago.bauermann@linaro.org>; luis.machado@arm.com <luis.machado@arm.com>
Subject: Re: [PATCH v5 05/12] gdb, gdbserver: Use xstate_bv for target description creation on x86.

Christina Schimpe <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

Maybe here you mean: "..., which are a set of, or parts of, registers."
If not, then this sentence doesn't make sense to me.

> organized using state-component bitmaps, each bit corresponding to a
> single state component.
>
> The SDM uses the term xstate_bv for a state-component bitmap, which is

Would it be possible to define what SDM it please.

> 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.
>
> Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
> Reviewed-By: Luis Machado <luis.machado@arm.com>
> ---
>  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 | 59 +++++++++++++++--------------
>  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, 198 insertions(+), 156 deletions(-)
>
> diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
> index 82dd1e07cf3..04539dd288a 100644
> --- a/gdb/amd64-tdep.c
> +++ b/gdb/amd64-tdep.c
> @@ -3551,23 +3551,23 @@ amd64_x32_none_init_abi (gdbarch_info info, gdbarch *arch)
>                      amd64_target_description (X86_XSTATE_SSE_MASK, true));
>  }
>
> -/* Return the target description for a specified XSAVE feature mask.  */
> +/* See amd64-tdep.h.  */
>
>  const struct target_desc *
> -amd64_target_description (uint64_t xcr0, bool segments)
> +amd64_target_description (uint64_t xstate_bv_mask, bool segments)

I'd like to ask about the use of 'mask' in this variable name.  We used
to pass in an xcr0 value, which was then combined with various masks to
extract the state bits we were interested in.

You've renamed the new variable as a mask, but continue to combine it
with the same masks (e.g. X86_XSTATE_AVX), which makes me suspect the
variable is not a mask at all.

Now, I can see where the confusion might have come from.  Almost every
call to amd64_target_description passes in a *_MASK constant.  But
that's OK, given these are bit sets, then the MASK constants actually
define valid values.

I am aware that this sounds like a trivial complaint over a variable
name, but I think calling this a mask is pretty confusing (at least to
me), so unless I'm not understanding this, then I think it is worth
renaming this.

The use of `mask` for the value is present throughout this patch, not
just this one function.

Thanks,
Andrew

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

[-- Attachment #2: Type: text/html, Size: 15859 bytes --]