[PATCH 07/10] x86/xsaves: Fix init_fpstate.header.xcomp_bv

[PATCH 07/10] x86/xsaves: Fix init_fpstate.header.xcomp_bv

[Yu-cheng Yu]

In setup_init_fpu_buf(), we use XRSTOR/XRSTORS with xfeatures (xstate_bv)
of 0x0 to effectively set all xstate components to init values. We then
execute XSAVE/XSAVES on the same buffer to save back init values.

This actually does not apply to XSAVES as XSAVES uses optimization. After
init, all components are in INIT and not MODIFIED state and will not be
saved. There is no need of setting xcomp_bv except for bit 63 to indicate
a compacted format.

Currently, init_fpstate is used in two ways:

(1) With copy_kernel_to_xregs(): because header.xfeatures is zero, the
    content is not actually read;

(2) With memcpy(): to sanitize a standard-format xsave area; the buffer is
    setup as expected.

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

diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 13829e1..4087261 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -371,10 +371,10 @@ static void __init setup_init_fpu_buf(void)
 	print_xstate_features();
 
 	if (cpu_has_xsaves)
-		init_fpstate.xsave.header.xcomp_bv = (u64)1 << 63 | xfeatures_mask;
+		init_fpstate.xsave.header.xcomp_bv = (u64)1 << 63;
 
 	/*
-	 * Init all the features state with header_bv being 0x0
+	 * Init all the features state with header.xfeatures being 0x0
 	 */
 	copy_kernel_to_xregs_booting(&init_fpstate.xsave);
 
-- 
1.9.1

Re: [PATCH 07/10] x86/xsaves: Fix init_fpstate.header.xcomp_bv

[Dave Hansen]

On 02/22/2016 10:59 AM, Yu-cheng Yu wrote:
> This actually does not apply to XSAVES as XSAVES uses optimization. After
> init, all components are in INIT and not MODIFIED state and will not be
> saved. There is no need of setting xcomp_bv except for bit 63 to indicate
> a compacted format.

I look at it this way: xcomp_bv tells you the format of the buffer
xstate_bv tells you which components of the buffer are valid (not in
init state).

As it stands, every kernel xcomp_bv value should be identical, since the
kernel buffer is always in the same state and XSAVES is always called
with the same Requested Feature BitMap (RFBM).

While I don't think this patch _hurts_ anything, I think it might be a
bit confusing.  Wouldn't it just be easier if *EVERY* xcomp_bv value was
the same?

Re: [PATCH 07/10] x86/xsaves: Fix init_fpstate.header.xcomp_bv

[Yu-cheng Yu]

On Mon, Feb 22, 2016 at 11:55:50AM -0800, Dave Hansen wrote:
> On 02/22/2016 10:59 AM, Yu-cheng Yu wrote:
> > This actually does not apply to XSAVES as XSAVES uses optimization. After
> > init, all components are in INIT and not MODIFIED state and will not be
> > saved. There is no need of setting xcomp_bv except for bit 63 to indicate
> > a compacted format.
> 
> I look at it this way: xcomp_bv tells you the format of the buffer
> xstate_bv tells you which components of the buffer are valid (not in
> init state).
> 
> As it stands, every kernel xcomp_bv value should be identical, since the
> kernel buffer is always in the same state and XSAVES is always called
> with the same Requested Feature BitMap (RFBM).
> 
> While I don't think this patch _hurts_ anything, I think it might be a
> bit confusing.  Wouldn't it just be easier if *EVERY* xcomp_bv value was
> the same?

Agree. This patch can be skipped if it turns out not needed.

--Yu-cheng