x86/xsaves: Fix XSAVES known issues

Hi Folks,

I've heard through the grapevine that there's some concern that we should not be bothering to enable XSAVES because there's not a sufficient use case for it. [...]

Once we *HAVE* XSAVES support, it also opens up the possibilities for doing things like dynamic XSAVE buffer allocation. For instance, let threads that are not _using_ AVX-512 not waste the 2k of space for it.

Quoted 4 lines · Dave Hansen

Once we *HAVE* XSAVES support, it also opens up the possibilities for doing things like dynamic XSAVE buffer allocation. For instance, let threads that are not _using_ AVX-512 not waste the 2k of space for it.

If we can somehow modify exec* system call to scan the executable binary (in user space) and pass along a bitmask containing xfeatures used in the binary, and XSAVES is enabled in the kernel, we can easily save a lot of memory. The kernel only needs to allocate space for tasks that actually use xstates; most of them do not.

arch/x86/kernel/fpu/signal.c +1 -0

index 0fbf60c..09945f1 100644
@@ -130,6 +130,45 @@ static inline int copy_fpregs_to_sigframe(struct xregs_state __user *buf)
 	return err;
 }
 
+static int may_copy_fpregs_to_sigframe(void)

That's not feasible. Think of dynamic libraries or just-in-time compilers. What instruction set does /usr/bin/java use, for instance? :)

CPUID function 0x0d, sub function (i, i > 1) returns in ecx[1] the alignment requirement of component i when the compacted format is used.

If ecx[1] is 0, component i is located immediately following the preceding component. If ecx[1] is 1, component i is located on the next 64-byte boundary following the preceding component.

Quoted 2 lines · Dave Hansen

That's not feasible. Think of dynamic libraries or just-in-time compilers. What instruction set does /usr/bin/java use, for instance? :)

The java argument is true. In that case or when the bitmask is missing, we can allocate for all supported features.

arch/x86/kernel/fpu/xstate.c +26 -0

+int copyin_to_xsaves(const void *kbuf, const void __user *ubuf,
+		     struct xregs_state *xsave)
+{
+	unsigned int offset, size;
+	int i;
+	u64 xfeatures;
+
+	offset = offsetof(struct xregs_state, header);
+	size = sizeof(xfeatures);
+
+	if (kbuf)
+		memcpy(&xfeatures, kbuf + offset, size);
+	else if (__copy_from_user(&xfeatures, ubuf + offset, size))
+		return -EFAULT;
+
+	/*
+	 * Reject if the user tries to set any supervisor xstates.
+	 */
+	if (xfeatures & XFEATURE_MASK_SUPERVISOR)
+		return -EINVAL;
+
+	for (i = 0; i < XFEATURE_MAX; i++) {
+		u64 mask = ((u64)1 << i);
+
+		if ((xfeatures & mask) && xfeature_enabled(i)) {
+			void *dst = get_xsave_addr_no_check(xsave, i);

Component offset print out was incorrect for XSAVES. Correct it and move to a separate function.

The arrays xstate_offsets[] and xstate_sizes[] record XSAVE area offsets and sizes. Values for legacy components i387 and XMMs were not initialized. Fix it.

We did not handle XSAVES* instructions correctly. There were issues in converting between standard and compacted format when interfacing with user-space. These issues have been corrected.

Add a WARN_ONCE() to make it clear that XSAVES supervisor states are not yet implemented.

Quoted 3 lines · Dave Hansen

Quoted 3 lines

That's not feasible. Think of dynamic libraries or just-in-time compilers. What instruction set does /usr/bin/java use, for instance? :)

The java argument is true. In that case or when the bitmask is missing, we can allocate for all supported features.

We probably want a software X86_FEATURE_OS_XSAVES or something.

Quoted 6 lines · Yu-cheng Yu

Quoted 2 lines · Dave Hansen

That's not feasible. Think of dynamic libraries or just-in-time compilers. What instruction set does /usr/bin/java use, for instance? :)

The java argument is true. In that case or when the bitmask is missing, we can allocate for all supported features.

I actually want to see us moving in the direction of unconditionally allocating everything on process startup. If we can stop using CR0.TS entirely, I think everything will be better.

Quoted 1 line · Dave Hansen

Quoted 1 line

We probably want a software X86_FEATURE_OS_XSAVES or something.

... or a simple variable.

I think we do some instruction patching based on it, so I was just suggesting the software X86_FEATURE because it would plug in to the existing scheme easier.

Quoted 11 lines · Andy Lutomirski

Quoted 6 lines · Yu-cheng Yu

Quoted 2 lines · Dave Hansen

That's not feasible. Think of dynamic libraries or just-in-time compilers. What instruction set does /usr/bin/java use, for instance? :)

The java argument is true. In that case or when the bitmask is missing, we can allocate for all supported features.

I actually want to see us moving in the direction of unconditionally allocating everything on process startup. If we can stop using CR0.TS entirely, I think everything will be better.

We can absolutely allocate the worst-case XSAVE buffer at task startup for folks that never want to see a latency spike in the life of the app no matter what.

But I also think it would be pretty nice if 'ls' didn't pay the 2k cost to have AVX-512 state if it's not using AVX-512. We also don't have to do this with CR0.TS. We'd actually use a combination of out-of-line (not appended to task_struct) XSAVE buffers and XGETBV1 to check the size of our XSAVE buffer before we call XSAVE* and resize it when needed.

Maybe nobody will ever care enough about 2kbytes/thread, though.

Quoted 3 lines · Yu-cheng Yu

The arrays xstate_offsets[] and xstate_sizes[] record XSAVE area offsets and sizes. Values for legacy components i387 and XMMs were not initialized. Fix it.

Is this just a completeness thing or does it actually break something?

Quoted 6 lines · Dave Hansen

Quoted 3 lines · Yu-cheng Yu

The arrays xstate_offsets[] and xstate_sizes[] record XSAVE area offsets and sizes. Values for legacy components i387 and XMMs were not initialized. Fix it.

Is this just a completeness thing or does it actually break something?

PTRACE format conversion needs them.

Quoted 6 lines · Yu-cheng Yu

Quoted 5 lines · Dave Hansen

Quoted 3 lines · Yu-cheng Yu

The arrays xstate_offsets[] and xstate_sizes[] record XSAVE area offsets and sizes. Values for legacy components i387 and XMMs were not initialized. Fix it.

Quoted 3 lines

Quoted 1 line

Is this just a completeness thing or does it actually break something?

PTRACE format conversion needs them.

If you respin these, can you please note that in the changelog and/or comments?

arch/x86/kernel/fpu/xstate.c const void *get_xsave_field_ptr(int xsave_state) · line 868

Quoted 6 lines · Yu-cheng Yu

arch/x86/kernel/fpu/xstate.c +6 -0

+	for (i = 0; i < XFEATURE_MAX; i++) {
+		/*
+		 * Copy only in-use xstates.
+		 */
+		if (((header.xfeatures >> i) & 1) && xfeature_enabled(i)) {
+			void *src = get_xsave_addr_no_check(xsave, i);

How could a bit in header.xfeatures get set if it is not set in xfeature_enabled() aka xfeatures_mask aka XCR0?

If a caller tries to pass a non-enabled xfeature in, we appear to just silently drop it and return success. Is that really what we want to do or do we want to error out?

Quoted 10 lines · Dave Hansen

arch/x86/kernel/fpu/xstate.c const void *get_xsave_field_ptr(int xsave_state) · line 868

Quoted 6 lines · Yu-cheng Yu

arch/x86/kernel/fpu/xstate.c +6 -0

+	for (i = 0; i < XFEATURE_MAX; i++) {
+		/*
+		 * Copy only in-use xstates.
+		 */
+		if (((header.xfeatures >> i) & 1) && xfeature_enabled(i)) {
+			void *src = get_xsave_addr_no_check(xsave, i);

How could a bit in header.xfeatures get set if it is not set in xfeature_enabled() aka xfeatures_mask aka XCR0?

Do you mean, we should test xfeature_enabled(i) first, like,

	if (xfeature_enabled(i) && ((header.xfeatures >> i) & 1)) ?

The result will be the same, like you said, if XCR0[i] is not set, hader.xfeatures[i] cannot be set. But if XCR0[i] is set, header.xfeatures[i] can be cleared.

Quoted 8 lines

Quoted 8 lines

But I also think it would be pretty nice if 'ls' didn't pay the 2k cost to have AVX-512 state if it's not using AVX-512. We also don't have to do this with CR0.TS. We'd actually use a combination of out-of-line (not appended to task_struct) XSAVE buffers and XGETBV1 to check the size of our XSAVE buffer before we call XSAVE* and resize it when needed.

Maybe nobody will ever care enough about 2kbytes/thread, though.

I suspect we're so far about 2k/thread that no one cares.

However, what do we do if we run out when memory when trying to reallocate?

Quoted 14 lines · Yu-cheng Yu

Quoted 10 lines · Dave Hansen

arch/x86/kernel/fpu/xstate.c const void *get_xsave_field_ptr(int xsave_state) · line 868

Quoted 6 lines · Yu-cheng Yu

arch/x86/kernel/fpu/xstate.c +6 -0

+	for (i = 0; i < XFEATURE_MAX; i++) {
+		/*
+		 * Copy only in-use xstates.
+		 */
+		if (((header.xfeatures >> i) & 1) && xfeature_enabled(i)) {
+			void *src = get_xsave_addr_no_check(xsave, i);

How could a bit in header.xfeatures get set if it is not set in xfeature_enabled() aka xfeatures_mask aka XCR0?

Do you mean, we should test xfeature_enabled(i) first, like,

Quoted 5 lines

	if (xfeature_enabled(i) && ((header.xfeatures >> i) & 1)) ?

The result will be the same, like you said, if XCR0[i] is not set, hader.xfeatures[i] cannot be set. But if XCR0[i] is set, header.xfeatures[i] can be cleared.

I think the xfeature_enabled(i) is probably redundant. Does it serve any actual purpose?

Quoted 9 lines · Yu-cheng Yu

arch/x86/kernel/fpu/signal.c +1 -0

index 0fbf60c..09945f1 100644
@@ -130,6 +130,45 @@ static inline int copy_fpregs_to_sigframe(struct xregs_state __user *buf)
 	return err;
 }
 
+static int may_copy_fpregs_to_sigframe(void)

arch/x86/kernel/fpu/signal.c static inline int copy_fpregs_to_sigframe(struct xregs_state __user *buf) · line 143

Quoted 10 lines

arch/x86/kernel/fpu/signal.c +10 -0

+{
+	/*
+	 * In signal handling path, the kernel already checks if
+	 * FPU instructions have been used before it calls
+	 * copy_fpstate_to_sigframe(). We check this here again
+	 * to detect any potential mis-use and saving invalid
+	 * register values directly to a signal frame.
+	 */
+	WARN_ONCE(!current->thread.fpu.fpstate_active,
+		  "direct FPU save with no math use\n");

This is probably an OK check for this _particular_ context (since this context is all ready to copy_to_user() the fpu state). But is it good generally? Why couldn't you have a !fpstate_active thread that _was_ fpregs_active?

Such a thread _could_ do a direct XSAVE with no issues.

The reason I haven't acked this patch is that I want to be _sure_ that we've audited all of the call paths that access the XSAVE buffer to ensure that they can all either handle the XSAVES format *or* don't care for whatever reason.

Could you share the steps that you've taken to assure yourself that all of the call paths are handled and we don't have more bugs?

Quoted 18 lines · Dave Hansen

arch/x86/kernel/fpu/signal.c static inline int copy_fpregs_to_sigframe(struct xregs_state __user *buf) · line 143

Quoted 11 lines · Yu-cheng Yu

arch/x86/kernel/fpu/signal.c +11 -0

+static int may_copy_fpregs_to_sigframe(void)
+{
+	/*
+	 * In signal handling path, the kernel already checks if
+	 * FPU instructions have been used before it calls
+	 * copy_fpstate_to_sigframe(). We check this here again
+	 * to detect any potential mis-use and saving invalid
+	 * register values directly to a signal frame.
+	 */
+	WARN_ONCE(!current->thread.fpu.fpstate_active,
+		  "direct FPU save with no math use\n");

This is probably an OK check for this _particular_ context (since this context is all ready to copy_to_user() the fpu state). But is it good generally? Why couldn't you have a !fpstate_active thread that _was_ fpregs_active?

Quoted 1 line

Such a thread _could_ do a direct XSAVE with no issues.

But it won't come to this function unless fpstate_active is ture?

Quoted 5 lines · Dave Hansen

The reason I haven't acked this patch is that I want to be _sure_ that we've audited all of the call paths that access the XSAVE buffer to ensure that they can all either handle the XSAVES format *or* don't care for whatever reason.

Quoted 2 lines

Could you share the steps that you've taken to assure yourself that all of the call paths are handled and we don't have more bugs?

We tested for signal, ptrace, context switch, avx, and mpx. We also run these tests with your audit patch to detect any format mis-match. That said, I cannot be sure there are no more bugs. As you said, we want to get this feature tested in the field and find potential issues early.

But I also think it would be pretty nice if 'ls' didn't pay the 2k cost to have AVX-512 state if it's not using AVX-512. [...]

If may_copy_fpregs_to_sigframe() were called from a slightly different context, or if we change the call-site, what breaks?

In other words. if we can still "may_copy_fpregs_to_sigframe()" no matter the state of fpu.fpstate_active, then I don't think we should be checking it in may_copy_fpregs_to_sigframe().

Quoted 6 lines · Dave Hansen

If may_copy_fpregs_to_sigframe() were called from a slightly different context, or if we change the call-site, what breaks?

In other words. if we can still "may_copy_fpregs_to_sigframe()" no matter the state of fpu.fpstate_active, then I don't think we should be checking it in may_copy_fpregs_to_sigframe().

Do you mean, don't check fpu.fpstate_active here?

That's better than what we had before, but it relies entirely on testing coverage and runtime checks.

Is it too much to ask that you also take a look and audit all the places the XSAVE buffer is accessed in the kernel and ensure that they either have code to handle standard vs. compacted/supervisor or don't care for some reason?

I did such an audit once upon a time, but I think it would be a good exercise to repeat both by a second set of eyes and because some time has passed.

We can still use the compacted area handling instructions, because presumably those are the fastest and are also the most optimized ones? But I wouldn't use them to do dynamic allocation: just allocate the maximum possible FPU save area at task creation time and never again worry about that detail.

Ok?

[    0.048347] x86/fpu: Cost of: XSAVE                       insn          :   127 cycles
[    0.049134] x86/fpu: Cost of: XSAVES                      insn          :   113 cycles
[    0.048492] x86/fpu: Cost of: XRSTOR                      insn          :   120 cycles
[    0.049267] x86/fpu: Cost of: XRSTORS                     insn          :   102 cycles

Quoted 4 lines · Yu-cheng Yu

Quoted 3 lines · Dave Hansen

If may_copy_fpregs_to_sigframe() were called from a slightly different context, or if we change the call-site, what breaks?

Quoted 5 lines

Quoted 3 lines

In other words. if we can still "may_copy_fpregs_to_sigframe()" no matter the state of fpu.fpstate_active, then I don't think we should be checking it in may_copy_fpregs_to_sigframe().

Do you mean, don't check fpu.fpstate_active here?

Not really. I'm asking *why* the check is there.

Quoted 9 lines · Dave Hansen

Quoted 8 lines · Yu-cheng Yu

Quoted 6 lines · Dave Hansen

If may_copy_fpregs_to_sigframe() were called from a slightly different context, or if we change the call-site, what breaks?

In other words. if we can still "may_copy_fpregs_to_sigframe()" no matter the state of fpu.fpstate_active, then I don't think we should be checking it in may_copy_fpregs_to_sigframe().

Quoted 3 lines

Quoted 1 line

Do you mean, don't check fpu.fpstate_active here?

Not really. I'm asking *why* the check is there.

If (fpu.fpstate_active == 0), then the task does not use FPU; we don't want to save these registers, right?

Quoted 2 lines

If (fpu.fpstate_active == 0), then the task does not use FPU; we don't want to save these registers, right?

No. It's possible to have fpstate_active=0 while fpregs_active=1. Such a task uses the FPU, but just hasn't done an XSAVE* to save the register content to the fpstate buffer.

Note, this is just theoretical, and does not happen in this particular call path today.

Quoted 9 lines · Dave Hansen

Quoted 2 lines

If (fpu.fpstate_active == 0), then the task does not use FPU; we don't want to save these registers, right?

No. It's possible to have fpstate_active=0 while fpregs_active=1. Such a task uses the FPU, but just hasn't done an XSAVE* to save the register content to the fpstate buffer.

Note, this is just theoretical, and does not happen in this particular call path today.

What about...

static int may_copy_fpregs_to_sigframe(void)

{
	if (fpregs_active())
		return 1;

	WARN_ONCE(!current->thread.fpu.fpstate_active,
		  "direct FPU save with no math use\n");

	if (boot_cpu_has(X86_FEATURE_XSAVES))
		return 1;

	return 0;
} 

Quoted 6 lines · Ingo Molnar

We can still use the compacted area handling instructions, because presumably those are the fastest and are also the most optimized ones? But I wouldn't use them to do dynamic allocation: just allocate the maximum possible FPU save area at task creation time and never again worry about that detail.

Ok?

Sounds sane to me.

BTW, I hacked up your "fpu performance" to compare XSAVE vs. XSAVES:

Quoted 4 lines

[    0.048347] x86/fpu: Cost of: XSAVE                       insn          :   127 cycles
[    0.049134] x86/fpu: Cost of: XSAVES                      insn          :   113 cycles
[    0.048492] x86/fpu: Cost of: XRSTOR                      insn          :   120 cycles
[    0.049267] x86/fpu: Cost of: XRSTORS                     insn          :   102 cycles

So I guess we can add that to the list of things that XSAVES is good for.

[...] Granted, the real-world benefit is probably hard to measure because the cache residency of the XSAVE buffer isn't as good when _actually_ context switching, but this at least shows a small theoretical advantage for XSAVES.

Quoted 24 lines · Yu-cheng Yu

Quoted 9 lines · Dave Hansen

Quoted 2 lines

If (fpu.fpstate_active == 0), then the task does not use FPU; we don't want to save these registers, right?

No. It's possible to have fpstate_active=0 while fpregs_active=1. Such a task uses the FPU, but just hasn't done an XSAVE* to save the register content to the fpstate buffer.

Note, this is just theoretical, and does not happen in this particular call path today.

What about...

static int may_copy_fpregs_to_sigframe(void)

{
	if (fpregs_active())
		return 1;

	WARN_ONCE(!current->thread.fpu.fpstate_active,
		  "direct FPU save with no math use\n");

	if (boot_cpu_has(X86_FEATURE_XSAVES))
		return 1;

Quoted 2 lines

	return 0;
} 

I don't think that changes anything. We still have a check in there that has no purpose. You've changed the ordering so that the specific example that I pointed out no longer triggers it. But, the underlying issue remains.

Before Linux gets into copy_fpstate_to_sigframe(), current->thread.fpu.fpstate_active must be true. For eagerfpu, fpregs_active() must also be true. For lazyfpu, once we try to do FSAVE/FXSAVE/XSAVE, fpregs_active() will become true as well.

We should have not based on boot_cpu_has(X86_FEATURE_XSAVES) at all.

Why don't we make it simple and always copy_fpregs_to_signal_frame()? Or, only for the lazy case, i.e. !fpregs_active(), we do __copy_to_user().

Quoted 6 lines · Yu-cheng Yu

Quoted 6 lines

Before Linux gets into copy_fpstate_to_sigframe(), current->thread.fpu.fpstate_active must be true. For eagerfpu, fpregs_active() must also be true. For lazyfpu, once we try to do FSAVE/FXSAVE/XSAVE, fpregs_active() will become true as well.

Quoted 5 lines

Quoted 5 lines

We should have not based on boot_cpu_has(X86_FEATURE_XSAVES) at all.

Why don't we make it simple and always copy_fpregs_to_signal_frame()? Or, only for the lazy case, i.e. !fpregs_active(), we do __copy_to_user().

For (lazy && not XSAVES) actually!

I think we're off in the weeds here.

Please just send an updated patch for what you want to do here.

From 43134a773d23ae8bab9f158d143c5cfb76bc0e9c Mon Sep 17 00:00:00 2001

Quoted 11 lines · Dave Hansen

That's better than what we had before, but it relies entirely on testing coverage and runtime checks.

Is it too much to ask that you also take a look and audit all the places the XSAVE buffer is accessed in the kernel and ensure that they either have code to handle standard vs. compacted/supervisor or don't care for some reason?

I did such an audit once upon a time, but I think it would be a good exercise to repeat both by a second set of eyes and because some time has passed.

I think there are 12 files that can be directly impacted by XSAVES.

	arch/x86/include/asm/fpu/internal.h
	arch/x86/include/asm/fpu/types.h
	arch/x86/include/asm/fpu/xstate.h
	arch/x86/include/uapi/asm/sigcontext.h
	arch/x86/kernel/traps.c
	arch/x86/kernel/fpu/core.c
	arch/x86/kernel/fpu/init.c
	arch/x86/kernel/fpu/regset.c
	arch/x86/kernel/fpu/signal.c
	arch/x86/kernel/fpu/xstate.c
	arch/x86/kvm/x86.c
	arch/x86/mm/mpx.c

They have been reviewed from the perspective of the compacted format. Please let me know anything else.

Quoted 28 lines · Yu-cheng Yu

Quoted 11 lines · Dave Hansen

That's better than what we had before, but it relies entirely on testing coverage and runtime checks.

Is it too much to ask that you also take a look and audit all the places the XSAVE buffer is accessed in the kernel and ensure that they either have code to handle standard vs. compacted/supervisor or don't care for some reason?

I did such an audit once upon a time, but I think it would be a good exercise to repeat both by a second set of eyes and because some time has passed.

I think there are 12 files that can be directly impacted by XSAVES.

	arch/x86/include/asm/fpu/internal.h
	arch/x86/include/asm/fpu/types.h
	arch/x86/include/asm/fpu/xstate.h
	arch/x86/include/uapi/asm/sigcontext.h
	arch/x86/kernel/traps.c
	arch/x86/kernel/fpu/core.c
	arch/x86/kernel/fpu/init.c
	arch/x86/kernel/fpu/regset.c
	arch/x86/kernel/fpu/signal.c
	arch/x86/kernel/fpu/xstate.c
	arch/x86/kvm/x86.c
	arch/x86/mm/mpx.c

Quoted 2 lines

They have been reviewed from the perspective of the compacted format. Please let me know anything else.

Can you double-check that nothing has changed in mainline (or tip for that matter) since you first did these checks?

But, since we *always* call XSAVES with an instruction mask of -1 and end up with a requested feature bitmap (RFBM) equal to XCR0, I think we can do a shortcut because we'll practically *always* have an xcomp_bv==RFBM==XCR0, which means that all (present) components will always have an address.

It's my fault, but you also need to go update

fpu__xfeature_set_state()
and

	__raw_xsave_addr()

The theoretical problem is that you might ask for a __raw_xsave_addr() of a component which has been compacted out of an XSAVES buffer and thus has no address. We could work around this by doing a memmove() and moving the components "up" after the one we are trying to set in order to make space.

But, since we *always* call XSAVES with an instruction mask of -1 and end up with a requested feature bitmap (RFBM) equal to XCR0, I think we can do a shortcut because we'll practically *always* have an xcomp_bv==RFBM==XCR0, which means that all (present) components will always have an address.

So, the alternative to doing the memmove() is to add some WARN_ON_FPU() checks to enforce xcomp_bv==RFBM==XCR0 in places where we call XSAVES/XRSTORS and __raw_xsave_addr(), maybe more.