[0/1] Add XSAVE layout description to Core files for debuggers to support varying XSAVE layouts

Message ID 20240314112359.50713-1-vigbalas@amd.com
Headers
Series Add XSAVE layout description to Core files for debuggers to support varying XSAVE layouts |

Message

Vignesh Balasubramanian March 14, 2024, 11:23 a.m. UTC
  This patch proposes to add an extra .note section in the corefile to dump the CPUID information of a machine. This is being done to solve the issue of tools like the debuggers having to deal with coredumps from machines with varying XSAVE layouts in spite of having the same XCR0 bits. The new proposed .note section, at this point, consists of an array of records containing the information of each extended feature that is present. This provides details about the offsets and the sizes of the various extended save state components of the machine where the application crash occurred. Requesting a review for this patch.

Some background:

The XSAVE layouts of modern AMD and Intel CPUs differ, especially since Memory Protection Keys and the AVX-512 features have been inculcated into the AMD CPUs. This is since AMD never adopted (and hence never left room in the XSAVE layout for) the Intel MPX feature. Tools like GDB had assumed a fixed XSAVE layout matching that of Intel (based on the XCR0 mask). Hence, the core dumps from AMD CPUs didn't match the known size for the XCR0 mask. This resulted in GDB and other tools not being able to access the values of the AVX-512 and PKRU registers on AMD CPUs. To solve this, an interim solution has been accepted into GDB, and is already a part of GDB 14, thanks to these series of patches : [ https://sourceware.org/pipermail/gdb-patches/2023-March/198081.html ].
But this patch series depends on heuristics based on the total XSAVE register set size and the XCR0 mask to infer the layouts of the various register blocks for core dumps, and hence, is not a foolproof mechanism to determine the layout of the XSAVE area.

Hence this new core dump note has been proposed as a more sturdy mechanism to allow GDB/LLDB and other relevant tools to determine the layout of the XSAVE area of the machine where the corefile was dumped.
The  new core dump note (which is being proposed as a per-process .note section), NT_X86_XSAVE_LAYOUT (0x205) contains an array of structures.
Each structure describes an individual extended feature containing offset, size and flags (that is obtained through CPUID instruction) in a format roughly matching the follow C structure:

struct xfeat_component {
       u32 xfeat_type;
       u32 xfeat_sz;
       u32 xfeat_off;
       u32 xfeat_flags;
};


Vignesh Balasubramanian (1):
  x86/elf: Add a new .note section containing Xfeatures information to
    x86 core files

 arch/Kconfig                   |   9 +++
 arch/powerpc/Kconfig           |   1 +
 arch/powerpc/include/asm/elf.h |   2 -
 arch/x86/Kconfig               |   1 +
 arch/x86/include/asm/elf.h     |   7 +++
 arch/x86/kernel/fpu/xstate.c   | 101 +++++++++++++++++++++++++++++++++
 include/linux/elf.h            |   2 +-
 include/uapi/linux/elf.h       |   1 +
 8 files changed, 121 insertions(+), 3 deletions(-)
  

Comments

Willgerodt, Felix March 14, 2024, 4:25 p.m. UTC | #1
> -----Original Message-----
> From: Vignesh Balasubramanian <vigbalas@amd.com>
> Sent: Donnerstag, 14. März 2024 12:23
> To: linux-kernel@vger.kernel.org; linux-toolchains@vger.kernel.org
> Cc: mpe@ellerman.id.au; npiggin@gmail.com; christophe.leroy@csgroup.eu;
> aneesh.kumar@kernel.org; naveen.n.rao@linux.ibm.com;
> ebiederm@xmission.com; keescook@chromium.org; x86@kernel.org; linuxppc-
> dev@lists.ozlabs.org; linux-mm@kvack.org; bpetkov@amd.com;
> jinisusan.george@amd.com; matz@suse.de; binutils@sourceware.org;
> jhb@FreeBSD.org; Willgerodt, Felix <felix.willgerodt@intel.com>; Vignesh
> Balasubramanian <vigbalas@amd.com>
> Subject: [PATCH 0/1] Add XSAVE layout description to Core files for debuggers to
> support varying XSAVE layouts
> 
> This patch proposes to add an extra .note section in the corefile to dump the
> CPUID information of a machine. This is being done to solve the issue of tools like
> the debuggers having to deal with coredumps from machines with varying XSAVE
> layouts in spite of having the same XCR0 bits. The new proposed .note section, at
> this point, consists of an array of records containing the information of each
> extended feature that is present. This provides details about the offsets and the
> sizes of the various extended save state components of the machine where the
> application crash occurred. Requesting a review for this patch.
> 
> Some background:
> 
> The XSAVE layouts of modern AMD and Intel CPUs differ, especially since Memory
> Protection Keys and the AVX-512 features have been inculcated into the AMD
> CPUs. This is since AMD never adopted (and hence never left room in the XSAVE
> layout for) the Intel MPX feature. Tools like GDB had assumed a fixed XSAVE
> layout matching that of Intel (based on the XCR0 mask). 

Hi,

I am a GDB developer and very much in favour of getting rid of the 
interim solution added to GDB. It doesn't scale well, as soon as we add new state
that has the same size as some existing state.

I am wondering if it wouldn't be easier for everyone if corefiles would just
contain space for all possible XSAVE components? Regardless of whether the CPU
supports it or whether or not AMD or Intel ever supported the feature.
Or if we would at least not drop some state from the middle, like in this case.

Regards,
Felix
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
  
Borislav Petkov March 14, 2024, 4:33 p.m. UTC | #2
On Thu, Mar 14, 2024 at 04:25:44PM +0000, Willgerodt, Felix wrote:
> I am wondering if it wouldn't be easier for everyone if corefiles would just
> contain space for all possible XSAVE components?

You mean we should shuffle out from the kernel 8K of AMX state even if
nothing uses it or the machine doesn't even support it?

That's silly.

Please have a look at this:

+struct xfeat_component {
+       u32 xfeat_type;
+       u32 xfeat_sz;
+       u32 xfeat_off;
+       u32 xfeat_flags;
+} __packed;

What is wrong with having a blob of such xfeat_component things
describing the XSTATE buffer and parsing it in gdb?

Thx.
  
Willgerodt, Felix March 15, 2024, 8:43 a.m. UTC | #3
> -----Original Message-----
> From: Borislav Petkov <bp@alien8.de>
> Sent: Donnerstag, 14. März 2024 17:34
> To: Willgerodt, Felix <felix.willgerodt@intel.com>
> Cc: Vignesh Balasubramanian <vigbalas@amd.com>; linux-
> kernel@vger.kernel.org; linux-toolchains@vger.kernel.org;
> mpe@ellerman.id.au; npiggin@gmail.com; christophe.leroy@csgroup.eu;
> aneesh.kumar@kernel.org; naveen.n.rao@linux.ibm.com;
> ebiederm@xmission.com; keescook@chromium.org; x86@kernel.org;
> linuxppc-dev@lists.ozlabs.org; linux-mm@kvack.org;
> jinisusan.george@amd.com; matz@suse.de; binutils@sourceware.org;
> jhb@FreeBSD.org
> Subject: Re: [PATCH 0/1] Add XSAVE layout description to Core files for
> debuggers to support varying XSAVE layouts
> 
> On Thu, Mar 14, 2024 at 04:25:44PM +0000, Willgerodt, Felix wrote:
> > I am wondering if it wouldn't be easier for everyone if corefiles would just
> > contain space for all possible XSAVE components?
> 
> You mean we should shuffle out from the kernel 8K of AMX state even if
> nothing uses it or the machine doesn't even support it?
> 
> That's silly.

I don't think it is so silly ;) Let me elaborate.

I was mostly trying to suggest an "easy fix" for the MPX issue.
I also said that we could drop features from the end of the list if the CPU
doesn't support them. Yes it is still wasteful, but kind of was the status quo
in the past afaik. And yes, if new states come after AMX it could get more wasteful.

Though in another email here Dave said that the offsets in XSAVE
are "fixed in practice". That makes me wonder. Even if we add CPUID to corefiles,
will Intel CPUs benefit? (I am not saying it isn't worth changing even if Intel CPUs don't.)
E.g. will we actually get rid of the 8k that you are complaining about?
I don't know the answer to be honest. If XSAVE offsets are "fixed in practice",
I don't see how we would benefit.

And how would you check that "nothing uses AMX", if the state exist according
to CPUID?

> Please have a look at this:
> 
> +struct xfeat_component {
> +       u32 xfeat_type;
> +       u32 xfeat_sz;
> +       u32 xfeat_off;
> +       u32 xfeat_flags;
> +} __packed;
> 
> What is wrong with having a blob of such xfeat_component things
> describing the XSTATE buffer and parsing it in gdb?

I didn't say it is wrong or that I am opposing it. In fact CPUID in
corefiles could be useful for other scenarios as well.
Though all consumers will need to add to their implementation.
I assume LLDB and GDB aren't the only consumers.

Felix
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928