[PATCH V6 0/2] xsaves bug fix

[PATCH V6 0/2] xsaves bug fix

[Shuai Ruan]

From: Shuai Ruan <shuai.ruan@intel.com>

This patchset contain two xsaves bug fix patches
1. calculate the comp_offsets base on xcomp_bv
2. fix overwriting between non-lazy/lazy xsaves

Shuai Ruan (2):
  x86/xsaves: calculate the comp_offsets base on xcomp_bv
  x86/xsaves: fix overwriting between non-lazy/lazy xsaves

 xen/arch/x86/domain.c        |   8 +--
 xen/arch/x86/domctl.c        |   6 +--
 xen/arch/x86/hvm/hvm.c       |   8 ++-
 xen/arch/x86/i387.c          |  23 +++++++--
 xen/arch/x86/xstate.c        | 116 +++++++++++++++++++++++++++----------------
 xen/include/asm-x86/xstate.h |   3 ++
 6 files changed, 99 insertions(+), 65 deletions(-)

-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH V6 1/2] x86/xsaves: calculate the comp_offsets base on xcomp_bv

[Shuai Ruan]

Previous patch using all available features calculate comp_offsets.
This is wrong.This patch fix this bug by calculating the comp_offset
based on xcomp_bv of current guest.
Also, the comp_offset should take alignment into consideration.

Signed-off-by: Shuai Ruan <shuai.ruan@linux.intel.com>
Reported-by: Jan Beulich <jbeulich@suse.com>
---
V5: Address comments from Jan:
1. use xcomp_bv to caculate comp_offsets
2. local variable/funciton parameters with no xstate_ prefix 
3. fix a bug realted with test_bit().

V4: Address comments from Jan:
1. use xstate_comp_offsets as on-stack array.

V3: Address comments from Jan:
1. fix xstate_comp_offsets used as static array problem.
2. change xstate_align from array to u64 and used as bitmap.
3. change calculating xstate_comp_offsets into three step.
        1) whether component is set in xsavearea
        2) whether component need align
        3) add xstate_size[i-1]

V2: Address comments from Jan:
1. code style fix.
2. setup_xstate_comp take xcomp_bv as param.

 xen/arch/x86/xstate.c        | 58 ++++++++++++++++++++++++++++----------------
 xen/include/asm-x86/xstate.h |  2 ++
 2 files changed, 39 insertions(+), 21 deletions(-)

diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index f649405..a7b6a04 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -26,8 +26,8 @@ u64 __read_mostly xfeature_mask;
 
 static unsigned int *__read_mostly xstate_offsets;
 unsigned int *__read_mostly xstate_sizes;
+static u64 __read_mostly xstate_align;
 static unsigned int __read_mostly xstate_features;
-static unsigned int __read_mostly xstate_comp_offsets[sizeof(xfeature_mask)*8];
 
 static uint32_t __read_mostly mxcsr_mask = 0x0000ffbf;
 
@@ -94,7 +94,7 @@ static bool_t xsave_area_compressed(const struct xsave_struct *xsave_area)
 
 static int setup_xstate_features(bool_t bsp)
 {
-    unsigned int leaf, tmp, eax, ebx;
+    unsigned int leaf, eax, ebx, ecx, edx;
 
     if ( bsp )
     {
@@ -111,57 +111,71 @@ static int setup_xstate_features(bool_t bsp)
     for ( leaf = 2; leaf < xstate_features; leaf++ )
     {
         if ( bsp )
+        {
             cpuid_count(XSTATE_CPUID, leaf, &xstate_sizes[leaf],
-                        &xstate_offsets[leaf], &tmp, &tmp);
+                        &xstate_offsets[leaf], &ecx, &edx);
+            if ( ecx & XSTATE_ALIGN64 )
+                __set_bit(leaf, &xstate_align);
+        }
         else
         {
             cpuid_count(XSTATE_CPUID, leaf, &eax,
-                        &ebx, &tmp, &tmp);
+                        &ebx, &ecx, &edx);
             BUG_ON(eax != xstate_sizes[leaf]);
             BUG_ON(ebx != xstate_offsets[leaf]);
+            BUG_ON(!(ecx & XSTATE_ALIGN64) != !test_bit(leaf, &xstate_align));
         }
     }
 
     return 0;
 }
 
-static void __init setup_xstate_comp(void)
+static void setup_xstate_comp(uint16_t *comp_offsets,
+                              const uint64_t xcomp_bv)
 {
     unsigned int i;
+    unsigned int offset;
 
     /*
      * The FP xstates and SSE xstates are legacy states. They are always
      * in the fixed offsets in the xsave area in either compacted form
      * or standard form.
      */
-    xstate_comp_offsets[0] = 0;
-    xstate_comp_offsets[1] = XSAVE_SSE_OFFSET;
+    comp_offsets[0] = 0;
+    comp_offsets[1] = XSAVE_SSE_OFFSET;
 
-    xstate_comp_offsets[2] = FXSAVE_SIZE + XSAVE_HDR_SIZE;
+    comp_offsets[2] = FXSAVE_SIZE + XSAVE_HDR_SIZE;
 
-    for ( i = 3; i < xstate_features; i++ )
+    offset = comp_offsets[2];
+    for ( i = 2; i < xstate_features; i++ )
     {
-        xstate_comp_offsets[i] = xstate_comp_offsets[i - 1] +
-                                 (((1ul << i) & xfeature_mask)
-                                  ? xstate_sizes[i - 1] : 0);
-        ASSERT(xstate_comp_offsets[i] + xstate_sizes[i] <= xsave_cntxt_size);
+        if ( (1ul << i) & xcomp_bv )
+        {
+            if ( test_bit(i, &xstate_align) )
+                offset = ROUNDUP(offset, 64);
+            comp_offsets[i] = offset;
+            offset += xstate_sizes[i];
+        }
     }
+    ASSERT(offset <= xsave_cntxt_size);
 }
 
 static void *get_xsave_addr(struct xsave_struct *xsave,
-        unsigned int xfeature_idx)
+                            const uint16_t *comp_offsets,
+                            unsigned int xfeature_idx)
 {
     if ( !((1ul << xfeature_idx) & xsave->xsave_hdr.xstate_bv) )
         return NULL;
 
-    return (void *)xsave + (xsave_area_compressed(xsave)
-            ? xstate_comp_offsets
-            : xstate_offsets)[xfeature_idx];
+    return (void *)xsave + (xsave_area_compressed(xsave) ?
+                            comp_offsets[xfeature_idx] :
+                            xstate_offsets[xfeature_idx]);
 }
 
 void expand_xsave_states(struct vcpu *v, void *dest, unsigned int size)
 {
     struct xsave_struct *xsave = v->arch.xsave_area;
+    uint16_t comp_offsets[sizeof(xfeature_mask)*8];
     u64 xstate_bv = xsave->xsave_hdr.xstate_bv;
     u64 valid;
 
@@ -172,6 +186,8 @@ void expand_xsave_states(struct vcpu *v, void *dest, unsigned int size)
     }
 
     ASSERT(xsave_area_compressed(xsave));
+    setup_xstate_comp(comp_offsets, xsave->xsave_hdr.xcomp_bv);
+
     /*
      * Copy legacy XSAVE area and XSAVE hdr area.
      */
@@ -188,7 +204,7 @@ void expand_xsave_states(struct vcpu *v, void *dest, unsigned int size)
     {
         u64 feature = valid & -valid;
         unsigned int index = fls(feature) - 1;
-        const void *src = get_xsave_addr(xsave, index);
+        const void *src = get_xsave_addr(xsave, comp_offsets, index);
 
         if ( src )
         {
@@ -203,6 +219,7 @@ void expand_xsave_states(struct vcpu *v, void *dest, unsigned int size)
 void compress_xsave_states(struct vcpu *v, const void *src, unsigned int size)
 {
     struct xsave_struct *xsave = v->arch.xsave_area;
+    uint16_t comp_offsets[sizeof(xfeature_mask)*8];
     u64 xstate_bv = ((const struct xsave_struct *)src)->xsave_hdr.xstate_bv;
     u64 valid;
 
@@ -222,6 +239,7 @@ void compress_xsave_states(struct vcpu *v, const void *src, unsigned int size)
     /* Set XSTATE_BV and XCOMP_BV.  */
     xsave->xsave_hdr.xstate_bv = xstate_bv;
     xsave->xsave_hdr.xcomp_bv = v->arch.xcr0_accum | XSTATE_COMPACTION_ENABLED;
+    setup_xstate_comp(comp_offsets, xsave->xsave_hdr.xcomp_bv);
 
     /*
      * Copy each region from the non-compacted offset to the
@@ -232,7 +250,7 @@ void compress_xsave_states(struct vcpu *v, const void *src, unsigned int size)
     {
         u64 feature = valid & -valid;
         unsigned int index = fls(feature) - 1;
-        void *dest = get_xsave_addr(xsave, index);
+        void *dest = get_xsave_addr(xsave, comp_offsets, index);
 
         if ( dest )
         {
@@ -564,8 +582,6 @@ void xstate_init(struct cpuinfo_x86 *c)
 
     if ( setup_xstate_features(bsp) && bsp )
         BUG();
-    if ( bsp && (cpu_has_xsaves || cpu_has_xsavec) )
-        setup_xstate_comp();
 }
 
 static bool_t valid_xcr0(u64 xcr0)
diff --git a/xen/include/asm-x86/xstate.h b/xen/include/asm-x86/xstate.h
index c28cea5..a488688 100644
--- a/xen/include/asm-x86/xstate.h
+++ b/xen/include/asm-x86/xstate.h
@@ -46,6 +46,8 @@
 #define XSTATE_LAZY    (XSTATE_ALL & ~XSTATE_NONLAZY)
 #define XSTATE_COMPACTION_ENABLED  (1ULL << 63)
 
+#define XSTATE_ALIGN64 (1U << 1)
+
 extern u64 xfeature_mask;
 extern unsigned int *xstate_sizes;
 
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH V6 2/2] x86/xsaves: fix overwriting between non-lazy/lazy xsaves

[Shuai Ruan]

The offset at which components xsaved by xsave[sc] are not fixed.
So when when a save with v->fpu_dirtied set is followed by one
with v->fpu_dirtied clear, non-lazy xsave[sc] may overwriting data
written by the lazy one.

The solution is when using_xsave_compact is enabled and taking xcr0_accum into
consideration, if guest has ever used XSTATE_LAZY & ~XSTATE_FP_SSE
(XSTATE_FP_SSE will be excluded beacause xsave will write XSTATE_FP_SSE
part in legacy region of xsave area which is fixed, saving XSTATE_FS_SSE
will not cause overwriting problem), vcpu_xsave_mask will return XSTATE_ALL.
Otherwise vcpu_xsave_mask will return XSTATE_NONLAZY.

This may cause overhead save on lazy states which will cause performance
impact. After doing some performance tests on xsavec and xsaveopt
(suggested by jan), the results show xsaveopt performs better than xsavec.
So hypervisor will not use xsavec anymore.

xsaves will be used until supervised state is instroduced in hypervisor.
And XSTATE_XSAVES_ONLY (indicates supervised state is understood in xen)
is instroduced, the use of xsaves depend on whether XSTATE_XSAVES_ONLY
is set in xcr0_accum.

Signed-off-by: Shuai Ruan <shuai.ruan@linux.intel.com>
Reported-by: Jan Beulich <jbeulich@suse.com>
---
v6: Address comments from Jan
1. Do all changes in XSAVE()/XRSTOR() marco definition.
2. Add code in arch_set_info_guest()/hvm_load_cpu_ctxt()
   hvm_vcpu_reset_state() to guarante xcomp_bv is zero.

v5: Address comments from Jan
1. Add XSTATE_XSAVES_ONLY and using xsaves depend on whether this bits are
   set in xcr0_accum
2. Change compress logic in compress_xsave_states() depend on
   !(v->arch.xcr0_accum & XSTATE_XSAVES_ONLY) && !xsave_area_compressed(src)).
3. XSTATE_COMPACTION_ENABLED only set in xrstor().
4. Rebase the code on
   [V4] x86/xsaves: calculate the xstate_comp_offsets base on xstate_bv
   (already sent out) For they both change same code.
   (I am not sure whether this rebase is ok or not).

v4: Address comments from Jan
1. Add synthetic CPU feature X86_FEATURE_XSAVE_COMPACT and use this feature
   indicate whether hypervisor use compact xsave area or not.
2. Fix type/grammer errors of the comment in vcpu_xsave_mask.

v3: Address comments from Jan
1. Add xsavc clean up code and disable xsaves.
2. Add comment on why certain mask should be return in vcpu_xsave_mask.

v2: Address comments from Jan
1. add performance impact and next step to do in the description.

 xen/arch/x86/domain.c        |  8 +-----
 xen/arch/x86/domctl.c        |  6 +----
 xen/arch/x86/hvm/hvm.c       |  8 +++---
 xen/arch/x86/i387.c          | 23 ++++++++++++++----
 xen/arch/x86/xstate.c        | 58 +++++++++++++++++++++++++++-----------------
 xen/include/asm-x86/xstate.h |  1 +
 6 files changed, 60 insertions(+), 44 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index a33f975..e456e62 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -947,13 +947,7 @@ int arch_set_info_guest(
         fpu_sse->fcw = FCW_DEFAULT;
         fpu_sse->mxcsr = MXCSR_DEFAULT;
     }
-    if ( cpu_has_xsaves )
-    {
-        ASSERT(v->arch.xsave_area);
-        v->arch.xsave_area->xsave_hdr.xcomp_bv = XSTATE_COMPACTION_ENABLED |
-            v->arch.xsave_area->xsave_hdr.xstate_bv;
-    }
-    else if ( v->arch.xsave_area )
+    if ( v->arch.xsave_area )
         v->arch.xsave_area->xsave_hdr.xcomp_bv = 0;
 
     if ( !compat )
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index b34a295..1a36a36 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -922,7 +922,7 @@ long arch_do_domctl(
                 ret = -EFAULT;
 
             offset += sizeof(v->arch.xcr0_accum);
-            if ( !ret && (cpu_has_xsaves || cpu_has_xsavec) )
+            if ( !ret )
             {
                 void *xsave_area;
 
@@ -942,10 +942,6 @@ long arch_do_domctl(
                      ret = -EFAULT;
                 xfree(xsave_area);
            }
-           else if ( !ret && copy_to_guest_offset(evc->buffer, offset,
-                                                  (void *)v->arch.xsave_area,
-                                                  size - 2 * sizeof(uint64_t)) )
-                ret = -EFAULT;
 
             vcpu_unpause(v);
         }
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 80d59ff..a99821a 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -2202,9 +2202,8 @@ static int hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
         xsave_area->xsave_hdr.xstate_bv = 0;
         xsave_area->fpu_sse.mxcsr = MXCSR_DEFAULT;
     }
-    if ( cpu_has_xsaves && xsave_area )
-        xsave_area->xsave_hdr.xcomp_bv = XSTATE_COMPACTION_ENABLED |
-            xsave_area->xsave_hdr.xstate_bv;
+    if ( xsave_area )
+        xsave_area->xsave_hdr.xcomp_bv = 0;
 
     v->arch.user_regs.eax = ctxt.rax;
     v->arch.user_regs.ebx = ctxt.rbx;
@@ -5581,8 +5580,7 @@ void hvm_vcpu_reset_state(struct vcpu *v, uint16_t cs, uint16_t ip)
     if ( v->arch.xsave_area )
     {
         v->arch.xsave_area->xsave_hdr.xstate_bv = XSTATE_FP;
-        v->arch.xsave_area->xsave_hdr.xcomp_bv = cpu_has_xsaves
-            ? XSTATE_COMPACTION_ENABLED | XSTATE_FP : 0;
+        v->arch.xsave_area->xsave_hdr.xcomp_bv = 0;
     }
 
     v->arch.vgc_flags = VGCF_online;
diff --git a/xen/arch/x86/i387.c b/xen/arch/x86/i387.c
index c29d0fa..2afa762 100644
--- a/xen/arch/x86/i387.c
+++ b/xen/arch/x86/i387.c
@@ -118,7 +118,24 @@ static inline uint64_t vcpu_xsave_mask(const struct vcpu *v)
     if ( v->fpu_dirtied )
         return v->arch.nonlazy_xstate_used ? XSTATE_ALL : XSTATE_LAZY;
 
-    return v->arch.nonlazy_xstate_used ? XSTATE_NONLAZY : 0;
+    ASSERT(v->arch.nonlazy_xstate_used);
+
+    /*
+     * The offsets of components which live in the extended region of
+     * compact xsave area are not fixed. Xsave area may be overwritten
+     * when a xsave with v->fpu_dirtied set is followed by one with
+     * v->fpu_dirtied clear.
+     * In such case, if hypervisor uses compact xsave area and guest
+     * has ever used lazy states (checking xcr0_accum excluding
+     * XSTATE_FP_SSE), vcpu_xsave_mask will return XSTATE_ALL. Otherwise
+     * return XSTATE_NONLAZY.
+     * XSTATE_FP_SSE may be excluded, because the offsets of XSTATE_FP_SSE
+     * (in the legacy region of xsave area) are fixed, so saving
+     * XSTATE_FP_SSE will not cause overwriting problem.
+     */
+    return (v->arch.xsave_area->xsave_hdr.xcomp_bv & XSTATE_COMPACTION_ENABLED)
+           && (v->arch.xcr0_accum & XSTATE_LAZY & ~XSTATE_FP_SSE)
+           ? XSTATE_ALL : XSTATE_NONLAZY;
 }
 
 /* Save x87 extended state */
@@ -275,11 +292,7 @@ int vcpu_init_fpu(struct vcpu *v)
         return rc;
 
     if ( v->arch.xsave_area )
-    {
         v->arch.fpu_ctxt = &v->arch.xsave_area->fpu_sse;
-        if ( cpu_has_xsaves )
-            v->arch.xsave_area->xsave_hdr.xcomp_bv = XSTATE_COMPACTION_ENABLED;
-    }
     else
     {
         BUILD_BUG_ON(__alignof(v->arch.xsave_area->fpu_sse) < 16);
diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index a7b6a04..c75292c 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -179,7 +179,7 @@ void expand_xsave_states(struct vcpu *v, void *dest, unsigned int size)
     u64 xstate_bv = xsave->xsave_hdr.xstate_bv;
     u64 valid;
 
-    if ( !cpu_has_xsaves && !cpu_has_xsavec )
+    if ( !(xsave->xsave_hdr.xcomp_bv & XSTATE_COMPACTION_ENABLED) )
     {
         memcpy(dest, xsave, size);
         return;
@@ -223,13 +223,15 @@ void compress_xsave_states(struct vcpu *v, const void *src, unsigned int size)
     u64 xstate_bv = ((const struct xsave_struct *)src)->xsave_hdr.xstate_bv;
     u64 valid;
 
-    if ( !cpu_has_xsaves && !cpu_has_xsavec )
+    ASSERT(!xsave_area_compressed(src));
+
+    if ( !(v->arch.xcr0_accum & XSTATE_XSAVES_ONLY) &&
+         !xsave_area_compressed(src) )
     {
         memcpy(xsave, src, size);
         return;
     }
 
-    ASSERT(!xsave_area_compressed(src));
     /*
      * Copy legacy XSAVE area, to avoid complications with CPUID
      * leaves 0 and 1 in the loop below.
@@ -269,15 +271,16 @@ void xsave(struct vcpu *v, uint64_t mask)
     uint32_t lmask = mask;
     unsigned int fip_width = v->domain->arch.x87_fip_width;
 #define XSAVE(pfx) \
-        alternative_io_3(".byte " pfx "0x0f,0xae,0x27\n", /* xsave */ \
-                         ".byte " pfx "0x0f,0xae,0x37\n", /* xsaveopt */ \
-                         X86_FEATURE_XSAVEOPT, \
-                         ".byte " pfx "0x0f,0xc7,0x27\n", /* xsavec */ \
-                         X86_FEATURE_XSAVEC, \
-                         ".byte " pfx "0x0f,0xc7,0x2f\n", /* xsaves */ \
-                         X86_FEATURE_XSAVES, \
-                         "=m" (*ptr), \
-                         "a" (lmask), "d" (hmask), "D" (ptr))
+        if ( v->arch.xcr0_accum & XSTATE_XSAVES_ONLY ) \
+            asm volatile ( ".byte " pfx "0x0f,0xc7,0x2f\n" /* xsaves */ \
+                           : "=m" (*ptr) \
+                           : "a" (lmask), "d" (hmask), "D" (ptr) ); \
+        else \
+            alternative_io(".byte " pfx "0x0f,0xae,0x27\n", /* xsave */ \
+                           ".byte " pfx "0x0f,0xae,0x37\n", /* xsaveopt */ \
+                           X86_FEATURE_XSAVEOPT, \
+                           "=m" (*ptr), \
+                           "a" (lmask), "d" (hmask), "D" (ptr))
 
     if ( fip_width == 8 || !(mask & XSTATE_FP) )
     {
@@ -368,19 +371,29 @@ void xrstor(struct vcpu *v, uint64_t mask)
         switch ( __builtin_expect(ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET], 8) )
         {
             BUILD_BUG_ON(sizeof(faults) != 4); /* Clang doesn't support %z in asm. */
-#define XRSTOR(pfx) \
-        alternative_io("1: .byte " pfx "0x0f,0xae,0x2f\n" \
+#define _xrstor(xrstor_ins) \
+        asm volatile ( "1: .byte "xrstor_ins"\n" \
                        "3:\n" \
                        "   .section .fixup,\"ax\"\n" \
                        "2: incl %[faults]\n" \
                        "   jmp 3b\n" \
                        "   .previous\n" \
-                       _ASM_EXTABLE(1b, 2b), \
-                       ".byte " pfx "0x0f,0xc7,0x1f\n", \
-                       X86_FEATURE_XSAVES, \
-                       ASM_OUTPUT2([mem] "+m" (*ptr), [faults] "+g" (faults)), \
-                       [lmask] "a" (lmask), [hmask] "d" (hmask), \
-                       [ptr] "D" (ptr))
+                       _ASM_EXTABLE(1b, 2b) \
+                       : [mem] "+m" (*ptr), [faults] "+g" (faults) \
+                       : [lmask] "a" (lmask), [hmask] "d" (hmask), \
+                         [ptr] "D" (ptr) )
+
+#define XRSTOR(pfx) \
+        if ( v->arch.xcr0_accum & XSTATE_XSAVES_ONLY ) \
+        { \
+            if ( unlikely(!(ptr->xsave_hdr.xcomp_bv \
+                            & XSTATE_COMPACTION_ENABLED)) ) \
+                ptr->xsave_hdr.xcomp_bv |= ptr->xsave_hdr.xstate_bv \
+                                           | XSTATE_COMPACTION_ENABLED; \
+                _xrstor(pfx "0x0f,0xc7,0x1f"); /* xrstors */ \
+        } \
+        else \
+            _xrstor(pfx "0x0f,0xae,0x2f") /* xrstor */
 
         default:
             XRSTOR("0x48,");
@@ -389,6 +402,7 @@ void xrstor(struct vcpu *v, uint64_t mask)
             XRSTOR("");
             break;
 #undef XRSTOR
+#undef _xrstor
         }
         if ( likely(faults == prev_faults) )
             break;
@@ -416,7 +430,7 @@ void xrstor(struct vcpu *v, uint64_t mask)
                   ((mask & XSTATE_YMM) &&
                    !(ptr->xsave_hdr.xcomp_bv & XSTATE_COMPACTION_ENABLED))) )
                 ptr->fpu_sse.mxcsr &= mxcsr_mask;
-            if ( cpu_has_xsaves || cpu_has_xsavec )
+            if ( v->arch.xcr0_accum & XSTATE_XSAVES_ONLY )
             {
                 ptr->xsave_hdr.xcomp_bv &= this_cpu(xcr0) | this_cpu(xss);
                 ptr->xsave_hdr.xstate_bv &= ptr->xsave_hdr.xcomp_bv;
@@ -433,7 +447,7 @@ void xrstor(struct vcpu *v, uint64_t mask)
         case 2: /* Stage 2: Reset all state. */
             ptr->fpu_sse.mxcsr = MXCSR_DEFAULT;
             ptr->xsave_hdr.xstate_bv = 0;
-            ptr->xsave_hdr.xcomp_bv = cpu_has_xsaves
+            ptr->xsave_hdr.xcomp_bv = v->arch.xcr0_accum & XSTATE_XSAVES_ONLY
                                       ? XSTATE_COMPACTION_ENABLED : 0;
             continue;
         }
diff --git a/xen/include/asm-x86/xstate.h b/xen/include/asm-x86/xstate.h
index a488688..91d1c39 100644
--- a/xen/include/asm-x86/xstate.h
+++ b/xen/include/asm-x86/xstate.h
@@ -44,6 +44,7 @@
 #define XSTATE_NONLAZY (XSTATE_LWP | XSTATE_BNDREGS | XSTATE_BNDCSR | \
                         XSTATE_PKRU)
 #define XSTATE_LAZY    (XSTATE_ALL & ~XSTATE_NONLAZY)
+#define XSTATE_XSAVES_ONLY         0
 #define XSTATE_COMPACTION_ENABLED  (1ULL << 63)
 
 #define XSTATE_ALIGN64 (1U << 1)
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH V6 1/2] x86/xsaves: calculate the comp_offsets base on xcomp_bv

[Jan Beulich]

>>> On 24.03.16 at 09:29, <shuai.ruan@linux.intel.com> wrote:
>  static void *get_xsave_addr(struct xsave_struct *xsave,
> -        unsigned int xfeature_idx)
> +                            const uint16_t *comp_offsets,
> +                            unsigned int xfeature_idx)
>  {
>      if ( !((1ul << xfeature_idx) & xsave->xsave_hdr.xstate_bv) )
>          return NULL;

While not a problem in this patch, I think this being at least
questionable now becomes more obvious: With XSAVEC and
XSAVES this field may have clear bits just because the
component was in init state. Yet I don't think this should be
fixed here - instead the consumer(s) should better deal with
NULL coming back here (i.e. expand_xsave_states() should
probably memset() the area in an "else" to the if() leading to
memcpy(), whereas compress_xsave_states() is fine as is).

That's a 3rd patch, though.

> +    return (void *)xsave + (xsave_area_compressed(xsave) ?
> +                            comp_offsets[xfeature_idx] :
> +                            xstate_offsets[xfeature_idx]);

This function doesn't ever get called when
!xsave_area_compressed(xsave), so I don't really see why this
conditional expression ever got added. Yet another follow-up
patch I would say.

The patch here looks okay to me now.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH V6 2/2] x86/xsaves: fix overwriting between non-lazy/lazy xsaves

[Jan Beulich]

>>> On 24.03.16 at 09:29, <shuai.ruan@linux.intel.com> wrote:
> The offset at which components xsaved by xsave[sc] are not fixed.
> So when when a save with v->fpu_dirtied set is followed by one
> with v->fpu_dirtied clear, non-lazy xsave[sc] may overwriting data
> written by the lazy one.
> 
> The solution is when using_xsave_compact is enabled and taking xcr0_accum into
> consideration, if guest has ever used XSTATE_LAZY & ~XSTATE_FP_SSE
> (XSTATE_FP_SSE will be excluded beacause xsave will write XSTATE_FP_SSE
> part in legacy region of xsave area which is fixed, saving XSTATE_FS_SSE
> will not cause overwriting problem), vcpu_xsave_mask will return XSTATE_ALL.
> Otherwise vcpu_xsave_mask will return XSTATE_NONLAZY.
> 
> This may cause overhead save on lazy states which will cause performance
> impact. After doing some performance tests on xsavec and xsaveopt
> (suggested by jan), the results show xsaveopt performs better than xsavec.
> So hypervisor will not use xsavec anymore.
> 
> xsaves will be used until supervised state is instroduced in hypervisor.

"xsaves will not be used ... introduced ..." I suppose?

> @@ -223,13 +223,15 @@ void compress_xsave_states(struct vcpu *v, const void *src, unsigned int size)
>      u64 xstate_bv = ((const struct xsave_struct *)src)->xsave_hdr.xstate_bv;
>      u64 valid;
>  
> -    if ( !cpu_has_xsaves && !cpu_has_xsavec )
> +    ASSERT(!xsave_area_compressed(src));
> +
> +    if ( !(v->arch.xcr0_accum & XSTATE_XSAVES_ONLY) &&
> +         !xsave_area_compressed(src) )

Considering the ASSERT(), what's this second half of the conditional
good for?

> @@ -368,19 +371,29 @@ void xrstor(struct vcpu *v, uint64_t mask)
>          switch ( __builtin_expect(ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET], 8) )
>          {
>              BUILD_BUG_ON(sizeof(faults) != 4); /* Clang doesn't support %z in asm. */
> -#define XRSTOR(pfx) \
> -        alternative_io("1: .byte " pfx "0x0f,0xae,0x2f\n" \
> +#define _xrstor(xrstor_ins) \
> +        asm volatile ( "1: .byte "xrstor_ins"\n" \

Blanks around xrstor_ins please. Also please consider naming the
macro parameter just "insn".

>                         "3:\n" \
>                         "   .section .fixup,\"ax\"\n" \
>                         "2: incl %[faults]\n" \
>                         "   jmp 3b\n" \
>                         "   .previous\n" \
> -                       _ASM_EXTABLE(1b, 2b), \
> -                       ".byte " pfx "0x0f,0xc7,0x1f\n", \
> -                       X86_FEATURE_XSAVES, \
> -                       ASM_OUTPUT2([mem] "+m" (*ptr), [faults] "+g" (faults)), \
> -                       [lmask] "a" (lmask), [hmask] "d" (hmask), \
> -                       [ptr] "D" (ptr))
> +                       _ASM_EXTABLE(1b, 2b) \
> +                       : [mem] "+m" (*ptr), [faults] "+g" (faults) \
> +                       : [lmask] "a" (lmask), [hmask] "d" (hmask), \
> +                         [ptr] "D" (ptr) )
> +
> +#define XRSTOR(pfx) \
> +        if ( v->arch.xcr0_accum & XSTATE_XSAVES_ONLY ) \
> +        { \
> +            if ( unlikely(!(ptr->xsave_hdr.xcomp_bv \
> +                            & XSTATE_COMPACTION_ENABLED)) ) \
> +                ptr->xsave_hdr.xcomp_bv |= ptr->xsave_hdr.xstate_bv \
> +                                           | XSTATE_COMPACTION_ENABLED; \

In both cases above the operator in a split line belongs on the
previous one.

> +                _xrstor(pfx "0x0f,0xc7,0x1f"); /* xrstors */ \

Indentation.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH V6 2/2] x86/xsaves: fix overwriting between non-lazy/lazy xsaves

[Shuai Ruan]

On Tue, Mar 29, 2016 at 09:00:38AM -0600, Jan Beulich wrote:
> > 
> > xsaves will be used until supervised state is instroduced in hypervisor.
> 
> "xsaves will not be used ... introduced ..." I suppose?
Thanks
> 
> > @@ -223,13 +223,15 @@ void compress_xsave_states(struct vcpu *v, const void *src, unsigned int size)
> >      u64 xstate_bv = ((const struct xsave_struct *)src)->xsave_hdr.xstate_bv;
> >      u64 valid;
> >  
> > -    if ( !cpu_has_xsaves && !cpu_has_xsavec )
> > +    ASSERT(!xsave_area_compressed(src));
> > +
> > +    if ( !(v->arch.xcr0_accum & XSTATE_XSAVES_ONLY) &&
> > +         !xsave_area_compressed(src) )
> 
> Considering the ASSERT(), what's this second half of the conditional
> good for?
> 
The check of !xsave_area_compressed(src) indicate we did check the input
data. ( The check here is useless, but just want to make this more clear.)
As it is useless, I may drop it. 
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH V6 1/2] x86/xsaves: calculate the comp_offsets base on xcomp_bv

[Shuai Ruan]

On Tue, Mar 29, 2016 at 08:35:34AM -0600, Jan Beulich wrote:
> >>> On 24.03.16 at 09:29, <shuai.ruan@linux.intel.com> wrote:
> >  static void *get_xsave_addr(struct xsave_struct *xsave,
> > -        unsigned int xfeature_idx)
> > +                            const uint16_t *comp_offsets,
> > +                            unsigned int xfeature_idx)
> >  {
> >      if ( !((1ul << xfeature_idx) & xsave->xsave_hdr.xstate_bv) )
> >          return NULL;
> 
> While not a problem in this patch, I think this being at least
> questionable now becomes more obvious: With XSAVEC and
> XSAVES this field may have clear bits just because the
> component was in init state. Yet I don't think this should be
> fixed here - instead the consumer(s) should better deal with
> NULL coming back here (i.e. expand_xsave_states() should
> probably memset() the area in an "else" to the if() leading to
> memcpy(), whereas compress_xsave_states() is fine as is).
> 
> That's a 3rd patch, though.
> 
I will add code only in expand_xsave_states() to deal with NULL 
returned by get_xsave_addr().
> > +    return (void *)xsave + (xsave_area_compressed(xsave) ?
> > +                            comp_offsets[xfeature_idx] :
> > +                            xstate_offsets[xfeature_idx]);
> 
> This function doesn't ever get called when
> !xsave_area_compressed(xsave), so I don't really see why this
> conditional expression ever got added. Yet another follow-up
> patch I would say.
I will drop this.
> 
> The patch here looks okay to me now.
> 
Thanks
> Jan
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel