[PATCH V7 0/3] xsaves bug fix

[PATCH V7 0/3] xsaves bug fix

[Shuai Ruan]

This patch set fix xsaves related bugs.

Shuai Ruan (3):
  x86/xsaves: fix overwriting between non-lazy/lazy xsaves
  x86/xsaves: fix two remained issues
  x86/xsaves: ebx may return wrong value using CPUID eax=0xdh,ecx =1

 xen/arch/x86/domain.c        |  8 +----
 xen/arch/x86/domctl.c        |  6 +---
 xen/arch/x86/hvm/hvm.c       | 14 +++++----
 xen/arch/x86/i387.c          | 23 +++++++++++----
 xen/arch/x86/traps.c         | 12 ++++++++
 xen/arch/x86/xstate.c        | 69 +++++++++++++++++++++++++-------------------
 xen/include/asm-x86/xstate.h |  2 ++
 7 files changed, 82 insertions(+), 52 deletions(-)

-- 
1.9.1


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

[PATCH V7 1/3] 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 not be used until supervised state is introduced 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>
---
v7: Address comments form Jan
1. drop !xsave_area_compressed(src) in compress_xsave_states
2. fix coding style issues.
3. fix errors in description.

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        | 57 +++++++++++++++++++++++++++-----------------
 xen/include/asm-x86/xstate.h |  1 +
 6 files changed, 59 insertions(+), 44 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 6ec7554..a4f6db2 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 b7c7f42..e5180ef 100644
--- 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 611470e..5aef3cb 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -2204,9 +2204,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;
@@ -5583,8 +5582,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 1a3e277..9c4e81a 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 6d98f56..8c652bc 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,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 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

[PATCH V7 2/3] x86/xsaves: fix two remained issues

[Shuai Ruan]

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

1. get_xsave_addr() will only be called when
xsave_area_compressed(xsave) is true. So drop the
conditional expression.

2. expand_xsave_states() will memset the area when
get NULL from get_xsave_addr().

Signed-off-by: Shuai Ruan <shuai.ruan@intel.com>
---
 xen/arch/x86/xstate.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index 8c652bc..f4ea54d 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -164,12 +164,8 @@ static void *get_xsave_addr(struct xsave_struct *xsave,
                             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) ?
-                            comp_offsets[xfeature_idx] :
-                            xstate_offsets[xfeature_idx]);
+    return (1ul << xfeature_idx) & xsave->xsave_hdr.xstate_bv ?
+           (void *)xsave + comp_offsets[xfeature_idx] : NULL;
 }
 
 void expand_xsave_states(struct vcpu *v, void *dest, unsigned int size)
@@ -211,6 +207,8 @@ void expand_xsave_states(struct vcpu *v, void *dest, unsigned int size)
             ASSERT((xstate_offsets[index] + xstate_sizes[index]) <= size);
             memcpy(dest + xstate_offsets[index], src, xstate_sizes[index]);
         }
+        else
+            memset(dest + xstate_offsets[index], 0, xstate_sizes[index]);
 
         valid &= ~feature;
     }
-- 
1.9.1


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

[PATCH V7 3/3] x86/xsaves: ebx may return wrong value using CPUID eax=0xdh, ecx =1

[Shuai Ruan]

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

Refer to SDM 13.4.3 Extended Region of an XSAVE Area. The value return
by ecx[1] with cpuid function 0xdh and sub-fucntion i (i>1) indicates
the alignment of the state component i when the compacted format of the
extended region of an xsave area is used.

So when hvm/pv guest using CPUID eax=0xdh,ecx=1 to get the size of area
used for compacted format, we need to take alignment into consideration.

tools side is fixed by
"tools/libxc: Calculate xstate cpuid leaf from guest information"
by Andrew Cooper

Signed-off-by: Shuai Ruan <shuai.ruan@intel.com>
---
v2: Address comments by Jan:
1. take alignment into consideration in pv_cpuid.
2. fix coding style issues

 xen/arch/x86/hvm/hvm.c       |  6 +++++-
 xen/arch/x86/traps.c         | 12 ++++++++++++
 xen/arch/x86/xstate.c        |  2 +-
 xen/include/asm-x86/xstate.h |  1 +
 4 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 5aef3cb..c6cd4fb 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4743,14 +4743,18 @@ void hvm_cpuid(unsigned int input, unsigned int *eax, unsigned int *ebx,
         }
         if ( count == 1 )
         {
-            if ( cpu_has_xsaves && cpu_has_vmx_xsaves )
+            if ( (cpu_has_xsaves && cpu_has_vmx_xsaves) || cpu_has_xsavec )
             {
                 *ebx = XSTATE_AREA_MIN_SIZE;
                 if ( v->arch.xcr0 | v->arch.hvm_vcpu.msr_xss )
                     for ( sub_leaf = 2; sub_leaf < 63; sub_leaf++ )
                         if ( (v->arch.xcr0 | v->arch.hvm_vcpu.msr_xss) &
                              (1ULL << sub_leaf) )
+                        {
+                            if ( test_bit(sub_leaf, &xstate_align) )
+                                *ebx = ROUNDUP(*ebx, 64);
                             *ebx += xstate_sizes[sub_leaf];
+                        }
             }
             else
                 *ebx = *ecx = *edx = 0;
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 6fbb1cf..8694da6 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1020,6 +1020,18 @@ void pv_cpuid(struct cpu_user_regs *regs)
             a &= (boot_cpu_data.x86_capability[cpufeat_word(X86_FEATURE_XSAVEOPT)] &
                   ~cpufeat_mask(X86_FEATURE_XSAVES));
             b = c = d = 0;
+            if ( cpu_has_xsavec )
+            {
+                b = XSTATE_AREA_MIN_SIZE;
+                if ( curr->arch.xcr0 )
+                    for( subleaf = 2; subleaf < 63; subleaf++ )
+                        if ( (1ULL << subleaf) & curr->arch.xcr0 )
+                        {
+                            if ( test_bit(subleaf, &xstate_align) )
+                                b = ROUNDUP(b, 64);
+                            b += xstate_sizes[subleaf];
+                        }
+            }
             break;
         }
         break;
diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index f4ea54d..850b778 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -26,7 +26,7 @@ u64 __read_mostly xfeature_mask;
 
 static unsigned int *__read_mostly xstate_offsets;
 unsigned int *__read_mostly xstate_sizes;
-static u64 __read_mostly xstate_align;
+u64 __read_mostly xstate_align;
 static unsigned int __read_mostly xstate_features;
 
 static uint32_t __read_mostly mxcsr_mask = 0x0000ffbf;
diff --git a/xen/include/asm-x86/xstate.h b/xen/include/asm-x86/xstate.h
index 91d1c39..535443a 100644
--- a/xen/include/asm-x86/xstate.h
+++ b/xen/include/asm-x86/xstate.h
@@ -50,6 +50,7 @@
 #define XSTATE_ALIGN64 (1U << 1)
 
 extern u64 xfeature_mask;
+extern u64 xstate_align;
 extern unsigned int *xstate_sizes;
 
 /* extended state save area */
-- 
1.9.1


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

Re: [PATCH V7 1/3] x86/xsaves: fix overwriting between non-lazy/lazy xsaves

[Jan Beulich]

>>> On 31.03.16 at 10:57, <shuai.ruan@linux.intel.com> wrote:
> xsaves will not be used until supervised state is introduced 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

There's still a spelling mistake here, despite me having pointed it out
before (you fixed one instance, but not the other). This could be
dealt with upon commit, though.

> is set in xcr0_accum.

Btw, I think this shouldn't be a #define, as it can - afaict - be derived
from CPUID output. But this can easily be a follow-up patch, even one
that doesn't make it into 4.7.

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

Reviewed-by: Jan Beulich <jbeulich@suse.com>




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

Re: [PATCH V7 2/3] x86/xsaves: fix two remained issues

[Jan Beulich]

>>> On 31.03.16 at 10:57, <shuai.ruan@linux.intel.com> wrote:

Considering this isn't the last patch in the series, the subject isn't
really nice (apart from being mis-spelled). If you e.g. replaced
"remained" by "miscellaneous", I wouldn't insist on splitting.

> 1. get_xsave_addr() will only be called when
> xsave_area_compressed(xsave) is true. So drop the
> conditional expression.
> 
> 2. expand_xsave_states() will memset the area when
> get NULL from get_xsave_addr().

Reported-by: ...

> Signed-off-by: Shuai Ruan <shuai.ruan@intel.com>
> 
>  xen/arch/x86/xstate.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
> index 8c652bc..f4ea54d 100644
> --- a/xen/arch/x86/xstate.c
> +++ b/xen/arch/x86/xstate.c
> @@ -164,12 +164,8 @@ static void *get_xsave_addr(struct xsave_struct *xsave,
>                              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) ?
> -                            comp_offsets[xfeature_idx] :
> -                            xstate_offsets[xfeature_idx]);
> +    return (1ul << xfeature_idx) & xsave->xsave_hdr.xstate_bv ?
> +           (void *)xsave + comp_offsets[xfeature_idx] : NULL;
>  }

I would really have expected an ASSERT() to get added as
(documenting) replacement.

Jan


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

Re: [PATCH V7 1/3] x86/xsaves: fix overwriting between non-lazy/lazy xsaves

[Shuai Ruan]

On Mon, Apr 04, 2016 at 09:51:56AM -0600, Jan Beulich wrote:
> >>> On 31.03.16 at 10:57, <shuai.ruan@linux.intel.com> wrote:
> > xsaves will not be used until supervised state is introduced 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
> 
> There's still a spelling mistake here, despite me having pointed it out
> before (you fixed one instance, but not the other). This could be
> dealt with upon commit, though.
Oh . "instroduced" :(
> 
> > is set in xcr0_accum.
> 
> Btw, I think this shouldn't be a #define, as it can - afaict - be derived
> from CPUID output. 
Ok.
>But this can easily be a follow-up patch, even one
> that doesn't make it into 4.7.
I do not understand your meaning clearly.
Do you mean the follow-up patch ( XSTATE_XSAVES_ONLY derived from cpuid)
will not into 4.7 ? If so, when is best/proper time to send out the
follow-up patch ? I am not sure whether add the follow-up patch in this 
patchset or in a sperate patch which one is ok ?
In either case I will keep working on this.
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
Thanks. I will send out V8 soon.
> 
> 
> 
> _______________________________________________
> 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

Re: [PATCH V7 1/3] x86/xsaves: fix overwriting between non-lazy/lazy xsaves

[Jan Beulich]

>>> On 05.04.16 at 07:30, <shuai.ruan@linux.intel.com> wrote:
> On Mon, Apr 04, 2016 at 09:51:56AM -0600, Jan Beulich wrote:
>> Btw, I think this shouldn't be a #define, as it can - afaict - be derived
>> from CPUID output. 
> Ok.
>>But this can easily be a follow-up patch, even one
>> that doesn't make it into 4.7.
> I do not understand your meaning clearly.
> Do you mean the follow-up patch ( XSTATE_XSAVES_ONLY derived from cpuid)
> will not into 4.7 ?

You're aware that we're past the submission deadline for 4.7?

> If so, when is best/proper time to send out the
> follow-up patch ?

There's no strict rule for this, just that you shouldn't expect your
change to go in prior to the 4.7 tree getting branched. Of course
it'd be easier if you submitted around or after branching time.

> I am not sure whether add the follow-up patch in this 
> patchset or in a sperate patch which one is ok ?

A later follow-up one would be preferred.

Jan


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

Re: [PATCH V7 1/3] x86/xsaves: fix overwriting between non-lazy/lazy xsaves

[Shuai Ruan]

On Tue, Apr 05, 2016 at 01:17:34AM -0600, Jan Beulich wrote:
> >>> On 05.04.16 at 07:30, <shuai.ruan@linux.intel.com> wrote:
> > On Mon, Apr 04, 2016 at 09:51:56AM -0600, Jan Beulich wrote:
> > Do you mean the follow-up patch ( XSTATE_XSAVES_ONLY derived from cpuid)
> > will not into 4.7 ?
> 
> You're aware that we're past the submission deadline for 4.7?
> 
Oh, yeah, we're past the deadline.
> > If so, when is best/proper time to send out the
> > follow-up patch ?
> 
> There's no strict rule for this, just that you shouldn't expect your
> change to go in prior to the 4.7 tree getting branched. Of course
> it'd be easier if you submitted around or after branching time.
> 
> > I am not sure whether add the follow-up patch in this 
> > patchset or in a sperate patch which one is ok ?
> 
> A later follow-up one would be preferred.
> 
I will send the follow-up patch in a proper time.
Thanks 

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

Re: [PATCH V7 3/3] x86/xsaves: ebx may return wrong value using CPUID eax=0xdh, ecx =1

[Jan Beulich]

>>> On 31.03.16 at 10:57, <shuai.ruan@linux.intel.com> wrote:
> Refer to SDM 13.4.3 Extended Region of an XSAVE Area. The value return

No section numbers please - they tend to change.

> by ecx[1] with cpuid function 0xdh and sub-fucntion i (i>1) indicates

Either "0xd" or "0dh". And "function".

> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -1020,6 +1020,18 @@ void pv_cpuid(struct cpu_user_regs *regs)
>              a &= (boot_cpu_data.x86_capability[cpufeat_word(X86_FEATURE_XSAVEOPT)] &
>                    ~cpufeat_mask(X86_FEATURE_XSAVES));
>              b = c = d = 0;
> +            if ( cpu_has_xsavec )
> +            {
> +                b = XSTATE_AREA_MIN_SIZE;

Is this really correct namely when curr->arch.xcr0 == 0? If not, the
if() below should perhaps be combined with the if() above (and then
the same would apply to hvm_cpuid()).

> +                if ( curr->arch.xcr0 )
> +                    for( subleaf = 2; subleaf < 63; subleaf++ )
> +                        if ( (1ULL << subleaf) & curr->arch.xcr0 )

The first if() is redundant with this second one. If you really
mean to avoid the loop, then please also only check bits
2..62 in the first if().

Jan


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

Re: [PATCH V7 3/3] x86/xsaves: ebx may return wrong value using CPUID eax=0xdh, ecx =1

[Shuai Ruan]

On Tue, Apr 05, 2016 at 02:31:40AM -0600, Jan Beulich wrote:
> >>> On 31.03.16 at 10:57, <shuai.ruan@linux.intel.com> wrote:
> > Refer to SDM 13.4.3 Extended Region of an XSAVE Area. The value return
> 
> No section numbers please - they tend to change.
> 
> > by ecx[1] with cpuid function 0xdh and sub-fucntion i (i>1) indicates
> 
> Either "0xd" or "0dh". And "function".
> 
> > --- a/xen/arch/x86/traps.c
> > +++ b/xen/arch/x86/traps.c
> > @@ -1020,6 +1020,18 @@ void pv_cpuid(struct cpu_user_regs *regs)
> >              a &= (boot_cpu_data.x86_capability[cpufeat_word(X86_FEATURE_XSAVEOPT)] &
> >                    ~cpufeat_mask(X86_FEATURE_XSAVES));
> >              b = c = d = 0;
> > +            if ( cpu_has_xsavec )
> > +            {
> > +                b = XSTATE_AREA_MIN_SIZE;
> 
> Is this really correct namely when curr->arch.xcr0 == 0? If not, the
> if() below should perhaps be combined with the if() above (and then
> the same would apply to hvm_cpuid()).
> 
> > +                if ( curr->arch.xcr0 )
> > +                    for( subleaf = 2; subleaf < 63; subleaf++ )
> > +                        if ( (1ULL << subleaf) & curr->arch.xcr0 )
> 
> The first if() is redundant with this second one. If you really
> mean to avoid the loop, then please also only check bits
> 2..62 in the first if().
> 
Ok for all comments above.

Another question is whether we should add this in pv_cpuid() or not.
(which we have discussed in the previous thread).

Refer to SDM Volume 1 
"13.2 ENUMERATION OF CPU SUPPORT FOR XSAVE INSTRUCTIONS AND XSAVE-
SUPPORTED FEATURES"
— CPUID function 0DH, sub-function 1.
...
"EBX enumerates the size (in bytes) required by the XSAVES instruction
 for an XSAVE area containing all
 the state components corresponding to bits currently set in XCR0 |
 IA32_XSS."

From the descriptions above, EBX only be used when XSAVES is enabled.
So I think we should not deal with pv_cpuid() here. 
Any comments ?

> 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

Re: [PATCH V7 3/3] x86/xsaves: ebx may return wrong value using CPUID eax=0xdh, ecx =1

[Jan Beulich]

>>> Shuai Ruan <shuai.ruan@linux.intel.com> 04/06/16 8:59 AM >>>
>Another question is whether we should add this in pv_cpuid() or not.
>(which we have discussed in the previous thread).
>
>Refer to SDM Volume 1 
>"13.2 ENUMERATION OF CPU SUPPORT FOR XSAVE INSTRUCTIONS AND XSAVE-
>SUPPORTED FEATURES"
>— CPUID function 0DH, sub-function 1.
>...
>"EBX enumerates the size (in bytes) required by the XSAVES instruction
 >for an XSAVE area containing all
 >the state components corresponding to bits currently set in XCR0 |
 >IA32_XSS."
>
>From the descriptions above, EBX only be used when XSAVES is enabled.
>So I think we should not deal with pv_cpuid() here. 

If it's mandated to be XSAVES only - yes. But why are you asking such a question?
It was you who proposed the change to pv_cpuid(), and it is you who should be
understanding the specifics of the various pieces of CPUID output related to XSAVE
better than me.

Jan


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

Re: [PATCH V7 1/3] x86/xsaves: fix overwriting between non-lazy/lazy xsaves

[Jan Beulich]

>>> On 31.03.16 at 10:57, <shuai.ruan@linux.intel.com> wrote:
> +#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; \

From v5 to v6 this changed from just = to |=, without any
explanation, and without me really noticing - why? Weren't
the other changes done specifically to guarantee xcomp_bv
to be zero up to this point? In which case I'd prefer to make
this obvious/explicit, by using = and perhaps an ASSERT()
here. (I have a patch ready, but I'd like to understand if
there was a reason for this change that I don't see.)

Jan


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

Re: [PATCH V7 1/3] x86/xsaves: fix overwriting between non-lazy/lazy xsaves

[Shuai Ruan]

On Mon, Apr 25, 2016 at 12:51:44AM -0600, Jan Beulich wrote:
> >>> On 31.03.16 at 10:57, <shuai.ruan@linux.intel.com> wrote:
> > +#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; \
> 
> From v5 to v6 this changed from just = to |=, without any
> explanation, and without me really noticing - why? Weren't
> the other changes done specifically to guarantee xcomp_bv
> to be zero up to this point? In which case I'd prefer to make
> this obvious/explicit, by using = and perhaps an ASSERT()
> here. (I have a patch ready, but I'd like to understand if
> there was a reason for this change that I don't see.)
> 
> Jan
Using "=" is better. xcomp_bv can be guarantee to be zero to this
point.

Thanks

> 
> 
> _______________________________________________
> 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

Re: [PATCH V7 1/3] x86/xsaves: fix overwriting between non-lazy/lazy xsaves

[Jan Beulich]

>>> On 29.04.16 at 03:36, <shuai.ruan@linux.intel.com> wrote:
> On Mon, Apr 25, 2016 at 12:51:44AM -0600, Jan Beulich wrote:
>> >>> On 31.03.16 at 10:57, <shuai.ruan@linux.intel.com> wrote:
>> > +#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; \
>> 
>> From v5 to v6 this changed from just = to |=, without any
>> explanation, and without me really noticing - why? Weren't
>> the other changes done specifically to guarantee xcomp_bv
>> to be zero up to this point? In which case I'd prefer to make
>> this obvious/explicit, by using = and perhaps an ASSERT()
>> here. (I have a patch ready, but I'd like to understand if
>> there was a reason for this change that I don't see.)
> 
> Using "=" is better. xcomp_bv can be guarantee to be zero to this
> point.

Thanks. I already have a patch, which I'll submit after 4.7 got
branched off.

Jan


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