Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1397394 > unrolled thread

[PATCH v5 12/13] x86/xsaves: Fix fpstate_init() for XSAVES

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v5 12/13] x86/xsaves: Fix fpstate_init() for XSAVES Yu-cheng Yu <yu-cheng.yu@intel.com> - 2016-05-09 23:00 +0200
    Re: [PATCH v5 12/13] x86/xsaves: Fix fpstate_init() for XSAVES Dave Hansen <dave.hansen@linux.intel.com> - 2016-05-10 01:50 +0200
      Re: [PATCH v5 12/13] x86/xsaves: Fix fpstate_init() for XSAVES Yu-cheng Yu <yu-cheng.yu@intel.com> - 2016-05-10 02:00 +0200
        Re: [PATCH v5 12/13] x86/xsaves: Fix fpstate_init() for XSAVES Dave Hansen <dave.hansen@linux.intel.com> - 2016-05-10 02:10 +0200

#1397394 — [PATCH v5 12/13] x86/xsaves: Fix fpstate_init() for XSAVES

When XSAVES is used, xsave.header.xcomp_bv[63] must be set.

Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
---
 arch/x86/kernel/fpu/core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index 41ab106..25e2605 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -224,6 +224,9 @@ void fpstate_init(union fpregs_state *state)
 
 	memset(state, 0, kernel_xstate_size);
 
+	if (cpu_has_xsaves)
+		state->xsave.header.xcomp_bv = (u64)1 << 63;
+
 	if (cpu_has_fxsr)
 		fpstate_init_fxstate(&state->fxsave);
 	else
-- 
1.9.1

[toc] | [next] | [standalone]

#1397521

On 05/09/2016 01:46 PM, Yu-cheng Yu wrote:
> When XSAVES is used, xsave.header.xcomp_bv[63] must be set.

So, that's not strictly correct.  XSAVES can write to a completely empty
(0'd) memory buffer.  It's only XRSTORS that _needs_ bit 63 set.  The
instruction reference is pretty clear on this point.

Oh, and if you decided to do this for some reason, please have mercy and
go make a macro for 1<<63.

Also, I don't think the kernel ever checks for this bit.  So are we
really calling XRSTORS on otherwise uninitialized xsave buffers?

[toc] | [prev] | [next] | [standalone]

#1397528

On Mon, May 09, 2016 at 04:41:31PM -0700, Dave Hansen wrote:
> On 05/09/2016 01:46 PM, Yu-cheng Yu wrote:
> > When XSAVES is used, xsave.header.xcomp_bv[63] must be set.
> 
> So, that's not strictly correct.  XSAVES can write to a completely empty
> (0'd) memory buffer.  It's only XRSTORS that _needs_ bit 63 set.  The
> instruction reference is pretty clear on this point.

You mean the comments?  I will change it to XRSTORS.

> 
> Oh, and if you decided to do this for some reason, please have mercy and
> go make a macro for 1<<63.
> 
> Also, I don't think the kernel ever checks for this bit.  So are we
> really calling XRSTORS on otherwise uninitialized xsave buffers?

Right now if we do fpstate_init(), without this patch, it will 
trigger a warning from copy_kernel_to_fxregs() when the task is
scheduled.

[toc] | [prev] | [next] | [standalone]

#1397556

On 05/09/2016 04:50 PM, Yu-cheng Yu wrote:
>> > 
>> > Oh, and if you decided to do this for some reason, please have mercy and
>> > go make a macro for 1<<63.
>> > 
>> > Also, I don't think the kernel ever checks for this bit.  So are we
>> > really calling XRSTORS on otherwise uninitialized xsave buffers?
> Right now if we do fpstate_init(), without this patch, it will 
> trigger a warning from copy_kernel_to_fxregs() when the task is
> scheduled.

Please include this in the patch description.  It's a pretty important
piece of justification.  Did you mean "copy_kernel_to_xregs()" without
the "f"?

I think we should probably also have an _explicit_ FPU_WARN_ON() in the
XRSTORS path for this.  *Both* the booting and regular ones, btw...

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web