RE: [PATCH v3 00/13] Handle variable XSAVE layouts

["George, Jini Susan" <JiniSusan.George@amd.com>]

[Public]

Thanks again for doing this, John, Felix and Aleksandar. I went through the changes and tested on a few AMD CPUs of differing xsave layouts. As of now these changes seem to cover the current AMD CPU implementations. Going forward, if additional AMD specific layouts have to be added, we would do it.

The changes look good to me (I am no approver, though) for the most part. A few minor comments:

Nit: in gdb/i386-tdep.h, it might be good to add a comment for the following line  (line # 148) for consistency with the rest of the fields.
  x86_xsave_layout xsave_layout;

Nit: You might want to modify the comments preceding the modified  xsave_*_offset[] definitions in gdb/i387-tdep.c to reflect that the offset to the locations are at (tdep)->xsave_layout.*_offset + xsave_*_offset[REGNUM].

Nit: Line 771 in gdb/i387-tdep.c. Might make sense to remove the "HI16_ZMM_area +" part.

I also agree with your proposed layout of the corefile note for the xsave layout.

Thanks,
Jini.

>>-----Original Message-----
>>From: John Baldwin <jhb@FreeBSD.org>
>>Sent: Wednesday, May 4, 2022 2:35 AM
>>To: gdb-patches@sourceware.org
>>Cc: Willgerodt, Felix <felix.willgerodt@intel.com>; George, Jini Susan
>><JiniSusan.George@amd.com>
>>Subject: [PATCH v3 00/13] Handle variable XSAVE layouts
>>
>>[CAUTION: External Email]
>>
>>Changes since V2:
>>
>>- Pulled in some of the changes from Intel's branch Felix pointed me
>>  at, in particular gdbserver support.  However, relative to that
>>  branch these patches make the following changes:
>>
>>  - The i387_* structs and class remain in gdbserver/i387-fp.cc
>>    rather than moving to gdbsupport/.
>>
>>  - Rather than invoking cpuid each time an XSAVE area is parsed,
>>    the x86_xsave_layout structure is used to hold offsets and
>>    CPUID is only invoked the first time NT_X86_XSTATE is probed
>>    with the offsets cached for later use.
>>
>>  - I did not update the ChangeLog bits of these log messages, but
>>    probably they can be dropped for the Intel commits as GDB
>>    commits generally do not include these now?
>>
>>- Added Linux support both for gdbarches and the x86 native targets.
>>  I wasn't sure if PT_GETREGSET on Linux provides a way to query the
>>  size of the XSAVE register set (on FreeBSD PT_GETREGSET returns
>>  the register set's size in iov_len of the passed-in iovec if the
>>  original iovec has a NULL pointer and zero length), so I used
>>  cpuid leaf 0xd subleaf 0x0 to query the size of the register set
>>  for the native targets as well as for the Linux gdbserver support.
>>
>>Note that this still depends on the size and xcr0 mask heuristic for Linux and
>>FreeBSD core dumps to determine the layout (and I have not added any
>>additional layouts as I wasn't sure if Jini was intending to add additional AMD-
>>specific layouts).  I'd kind of like to land this series before doing a followup to
>>flesh out a new core dump note.
>>
>>I think for the new core dump note what I would propose is a simple array of
>>CPUID results for sub-leaves of the 0xd leaf (as a NT_X86_XSTATE_CPUID or the
>>like) where each entry in the array contained the subleaf as well as eax, ebx, ecx,
>>and edx results.  This note might even eventually permit handling "compact"
>>XSTATE in future core dumps rather than only "standard".
>>
>>I have tested this on both an AMD Ryzen 9 5900X and Intel Core i7-8650U on
>>FreeBSD/amd64 as well as on a Linux/x86-64 VM on the Intel system.  I also
>>tested FreeBSD/i386 in a VM on the AMD system.
>>
>>Aleksandar Paunovic (2):
>>  gdbserver: Refactor the legacy region within the xsave struct
>>  gdbserver: Read offsets of the XSAVE extended region via CPUID
>>
>>John Baldwin (11):
>>  x86: Add an x86_xsave_layout structure to handle variable XSAVE
>>    layouts.
>>  core: Support fetching TARGET_OBJECT_X86_XSAVE_LAYOUT from
>>    architectures.
>>  nat/x86-cpuid.h: Add x86_cpuid_count wrapper around __get_cpuid_count.
>>  x86 nat: Add helper functions to save the XSAVE layout for the host.
>>  gdb: Update x86 FreeBSD architectures to support XSAVE layouts.
>>  gdb: Support XSAVE layouts for the current host in the FreeBSD x86
>>    targets.
>>  gdb: Update x86 Linux architectures to support XSAVE layouts.
>>  gdb: Support XSAVE layouts for the current host in the Linux x86
>>    targets.
>>  gdb: Use x86_xstate_layout to parse the XSAVE extended state area.
>>  gdbserver: Add a function to set the XSAVE mask and size.
>>  x86: Remove X86_XSTATE_SIZE and related constants.
>>
>> gdb/amd64-fbsd-nat.c       |  40 +--
>> gdb/amd64-fbsd-tdep.c      |   8 +-
>> gdb/amd64-linux-nat.c      |   6 +-
>> gdb/amd64-linux-tdep.c     |   8 +-
>> gdb/configure.nat          |   8 +-
>> gdb/corelow.c              |  22 ++
>> gdb/gdbarch-components.py  |  13 +
>> gdb/gdbarch-gen.h          |  10 +
>> gdb/gdbarch.c              |  32 +++
>> gdb/i386-fbsd-nat.c        |  39 +--
>> gdb/i386-fbsd-tdep.c       |  37 ++-
>> gdb/i386-fbsd-tdep.h       |   6 +
>> gdb/i386-linux-nat.c       |   8 +-
>> gdb/i386-linux-tdep.c      |  34 ++-
>> gdb/i386-linux-tdep.h      |   6 +
>> gdb/i386-tdep.c            |  36 ++-
>> gdb/i386-tdep.h            |   3 +
>> gdb/i387-tdep.c            | 493 ++++++++++++++++++++++++-------------
>> gdb/i387-tdep.h            |   8 +
>> gdb/nat/x86-cpuid.h        |  27 ++
>> gdb/nat/x86-xstate.c       |  65 +++++
>> gdb/nat/x86-xstate.h       |  35 +++
>> gdb/target.h               |   2 +
>> gdb/x86-fbsd-nat.c         |  51 ++++
>> gdb/x86-fbsd-nat.h         |  29 ++-
>> gdb/x86-linux-nat.c        |  33 +++
>> gdb/x86-linux-nat.h        |  11 +
>> gdbserver/configure.srv    |  12 +-
>> gdbserver/i387-fp.cc       | 312 ++++++++++++++---------
>> gdbserver/i387-fp.h        |   2 +-
>> gdbserver/linux-x86-low.cc |  10 +-
>> gdbsupport/x86-xstate.h    |  69 ++++--
>> 32 files changed, 1067 insertions(+), 408 deletions(-)  create mode 100644
>>gdb/nat/x86-xstate.c  create mode 100644 gdb/nat/x86-xstate.h
>>
>>--
>>2.34.1