x86/xsaves: fix overwriting between non-lazy/lazy xsaves

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 not be used until supervised state is introduced in hypervisor.
And XSTATE_XSAVES_ONLY (indicates supervised state is understood in xen)
is introduced, the use of xsaves depend on whether XSTATE_XSAVES_ONLY is set
in xcr0_accum.

Reported-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Shuai Ruan <shuai.ruan@linux.intel.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 6ec755484a13d73ae9251f525c31b639fc5a4c79..a4f6db27500f7f35e32fcaee8571711b812a8591 100644 (file)
--- 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 b7c7f424c49bfe896beef971ad65148b7700ec49..e5180efd7ce7702fcfe2b0a314482c5dbc13e014 100644 (file)
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -927,7 +927,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;
 
@@ -947,10 +947,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 9d4e5b8aaac83f2807d15f29aed9a69f0d0bd614..ad14eb35ddd0d09d672dae803ba3b785669e6e0f 100644 (file)
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1157,9 +1157,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;
@@ -4273,8 +4272,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 1a3e2771bd5970f319f415c2bb5aa0e81322988a..9c4e81a9beb52ba6f812379a4033660511983e30 100644 (file)
--- 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 6d98f56f40328c202d87195fbd79df7b2d8a55b9..8c652bc2387539a2533a2a316b8c70a1c95ca45e 100644 (file)
--- 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,14 @@ 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) )
     {
         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.
@@ -270,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) )
     {
@@ -369,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(insn) \
+        asm volatile ( "1: .byte " insn "\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,");
@@ -390,6 +402,7 @@ void xrstor(struct vcpu *v, uint64_t mask)
             XRSTOR("");
             break;
 #undef XRSTOR
+#undef _xrstor
         }
         if ( likely(faults == prev_faults) )
             break;
@@ -417,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;
@@ -434,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 a488688960ccbe15e1e9583bb854fcbd44cc120e..91d1c391b2d17be81327110ee044f0d0fb7e2a00 100644 (file)
--- 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)