[V5 0/4] add xsaves/xrstors support

[V5 0/4] add xsaves/xrstors support

[Shuai Ruan]

Changes in v5:
*Address comments from Andrew/Jan,mainly:
*Add lazy writes of the xss_msr.
*Add xsave_area check when save/restore guest.
*Add xsavec support.
*Use word 2 in xen/include/asm-x86/cpufeature.h.
*Fix some code errors.

Changes in v4:
* Address comments from Andrew, mainly:
* No xsaves suporting for pv guest.
* Using xstate_sizes instead of domain_cpuid in hvm_cupid in patch 3.
* Add support for format translation when perform pv guest migration in patch 2. 
* Fix some code errors.

Changes in v3:
* Address comments from Jan/Konrad
* Change the bits of eax/ebx/ecx/edx passed to guest in patch 4.
* Add code to verify whether host supports xsaves or not in patch 1.

Changes in v2:
* Address comments from Andrew/chao/Jan, mainly:
* Add details information of xsaves/xrstors feature.
* Test migration between xsaves-support machine and none-xsaves-support machine 
* Remove XGETBV1/XSAVEC/XSAVEOPT out of 'else' in patch 3.
* Change macro name XGETBV to XGETBV1 in patch 4.

This patchset enable xsaves/xrstors feature which will be available on 
Intel Skylake and later platform. Like xsave/xrstor, xsaves/xrstors 
feature will save and load processor state from a region of memory called 
XSAVE area. While unlike xsave/xrstor, xsaves/xrstors:

a) use the compacted format only for the extended region 
   of the XSAVE area which saves memory for you;
b) can operate on supervisor state components so the feature
   is prerequisite to support new supervisor state components;
c) execute only if CPL=0. 

Detail hardware spec can be found in chapter 13 (section 13.11 13.12) of the 
Intel SDM [1].

patch1: add basic definition/function to support xsaves
patch2: add xsaves/xrstors support for xen
patch3-4: add xsaves/xrstors support for hvm guest


[1] Intel SDM (http://www.intel.com/content/dam/www/public/us/en/documents/manuals/64-ia-32-architectures-software-developer-manual-325462.pdf)


Shuai Ruan (4):
  x86/xsaves: add basic definitions/helpers to support xsaves
  x86/xsaves: enable xsaves/xrstors/xsavec in xen
  x86/xsaves: enable xsaves/xrstors for hvm guest
  libxc: expose xsaves/xgetbv1/xsavec to hvm guest

 tools/libxc/xc_cpuid_x86.c         |  14 +-
 xen/arch/x86/domain.c              |   3 +
 xen/arch/x86/domctl.c              |  38 ++++-
 xen/arch/x86/hvm/hvm.c             |  51 ++++++-
 xen/arch/x86/hvm/vmx/vmcs.c        |   6 +-
 xen/arch/x86/hvm/vmx/vmx.c         |  20 +++
 xen/arch/x86/i387.c                |   4 +
 xen/arch/x86/traps.c               |   7 +-
 xen/arch/x86/xstate.c              | 304 ++++++++++++++++++++++++++++++++-----
 xen/include/asm-x86/cpufeature.h   |   6 +-
 xen/include/asm-x86/hvm/vcpu.h     |   1 +
 xen/include/asm-x86/hvm/vmx/vmcs.h |   6 +
 xen/include/asm-x86/hvm/vmx/vmx.h  |   2 +
 xen/include/asm-x86/msr-index.h    |   2 +
 xen/include/asm-x86/xstate.h       |  19 ++-
 15 files changed, 415 insertions(+), 68 deletions(-)

-- 
1.9.1

[V5 1/4] x86/xsaves: add basic definitions/helpers to support xsaves

[Shuai Ruan]

This patch add basic definitions/helpers which will be used in
later patches.

Signed-off-by: Shuai Ruan <shuai.ruan@linux.intel.com>
---
 xen/arch/x86/xstate.c            | 168 +++++++++++++++++++++++++++++++++++++++
 xen/include/asm-x86/cpufeature.h |   6 +-
 xen/include/asm-x86/hvm/vcpu.h   |   1 +
 xen/include/asm-x86/msr-index.h  |   2 +
 xen/include/asm-x86/xstate.h     |  14 +++-
 5 files changed, 189 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index d5f5e3b..ff03b31 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -29,12 +29,21 @@ static u32 __read_mostly xsave_cntxt_size;
 /* A 64-bit bitmask of the XSAVE/XRSTOR features supported by processor. */
 u64 __read_mostly xfeature_mask;
 
+unsigned int * __read_mostly xstate_offsets;
+unsigned int * __read_mostly xstate_sizes;
+static unsigned int __read_mostly xstate_features;
+static unsigned int __read_mostly xstate_comp_offsets[sizeof(xfeature_mask)*8];
+
+/* Cached msr_xss for fast read */
+static DEFINE_PER_CPU(uint64_t, msr_xss);
+
 /* Cached xcr0 for fast read */
 static DEFINE_PER_CPU(uint64_t, xcr0);
 
 /* Because XCR0 is cached for each CPU, xsetbv() is not exposed. Users should 
  * use set_xcr0() instead.
  */
+
 static inline bool_t xsetbv(u32 index, u64 xfeatures)
 {
     u32 hi = xfeatures >> 32;
@@ -65,6 +74,165 @@ uint64_t get_xcr0(void)
     return this_cpu(xcr0);
 }
 
+void set_msr_xss(u64 xss)
+{
+    wrmsrl(MSR_IA32_XSS, xss);
+    this_cpu(msr_xss) = xss;
+}
+
+uint64_t get_msr_xss(void)
+{
+    return this_cpu(msr_xss);
+}
+
+bool_t xsave_area_compressed(const struct xsave_struct *xsave_area)
+{
+    if ( xsave_area && (xsave_area->xsave_hdr.xcomp_bv
+                          & XSTATE_COMPACTION_ENABLED))
+	    return 1;
+    return 0;
+}
+
+static int setup_xstate_features(bool_t bsp)
+{
+    unsigned int leaf, tmp, eax, ebx;
+
+    if ( bsp )
+        xstate_features = fls(xfeature_mask);
+
+    if ( bsp )
+    {
+        xstate_offsets = xzalloc_array(unsigned int, xstate_features);
+        if( !xstate_offsets )
+            return -ENOMEM;
+
+        xstate_sizes = xzalloc_array(unsigned int, xstate_features);
+        if( !xstate_sizes )
+            return -ENOMEM;
+    }
+
+    for (leaf = 2; leaf < xstate_features; leaf++)
+    {
+        if( bsp )
+            cpuid_count(XSTATE_CPUID, leaf, &xstate_sizes[leaf],
+                        &xstate_offsets[leaf], &tmp, &tmp);
+        else
+        {
+             cpuid_count(XSTATE_CPUID, leaf, &eax,
+                        &ebx, &tmp, &tmp);
+             BUG_ON(eax != xstate_sizes[leaf]);
+             BUG_ON(ebx != xstate_offsets[leaf]);
+        }
+     }
+     return 0;
+}
+
+static void setup_xstate_comp(void)
+{
+    unsigned int i;
+
+    /*
+     * 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;
+
+    xstate_comp_offsets[2] = FXSAVE_SIZE + XSAVE_HDR_SIZE;
+
+    for (i = 3; 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] <= xsave_cntxt_size);
+    }
+}
+
+static void *get_xsave_addr(struct xsave_struct *xsave, unsigned int xfeature_idx)
+{
+    if ( !((1ul << xfeature_idx) & xfeature_mask) )
+        return NULL;
+
+    return (void *)xsave + xstate_comp_offsets[xfeature_idx];
+}
+
+void save_xsave_states(struct vcpu *v, void *dest, unsigned int size)
+{
+    struct xsave_struct *xsave = v->arch.xsave_area;
+    u64 xstate_bv = xsave->xsave_hdr.xstate_bv;
+    u64 valid;
+
+    ASSERT(xsave_area_compressed(xsave));
+    /*
+     * Copy legacy XSAVE area, to avoid complications with CPUID
+     * leaves 0 and 1 in the loop below.
+     */
+    memcpy(dest, xsave, FXSAVE_SIZE);
+
+    /* Set XSTATE_BV */
+    *(u64 *)(dest + XSAVE_HDR_OFFSET) = xstate_bv;
+
+    /*
+     * Copy each region from the possibly compacted offset to the
+     * non-compacted offset.
+     */
+    valid = xstate_bv & ~XSTATE_FP_SSE;
+    while ( valid )
+    {
+        u64 feature = valid & -valid;
+        int index = fls(feature) - 1;
+        void *src = get_xsave_addr(xsave, index);
+
+        if ( src )
+        {
+            ASSERT((xstate_offsets[index] + xstate_sizes[index]) <= size);
+            memcpy(dest + xstate_offsets[index], src, xstate_sizes[index]);
+        }
+
+        valid -= feature;
+    }
+
+}
+
+void load_xsave_states(struct vcpu *v, const void *src, unsigned int size)
+{
+    struct xsave_struct *xsave = v->arch.xsave_area;
+    u64 xstate_bv = *(u64 *)(src + XSAVE_HDR_OFFSET);
+    u64 valid;
+
+    ASSERT(!xsave_area_compressed((struct xsave_struct *)src));
+    /*
+     * Copy legacy XSAVE area, to avoid complications with CPUID
+     * leaves 0 and 1 in the loop below.
+     */
+    memcpy(xsave, src, FXSAVE_SIZE);
+
+    /* Set XSTATE_BV and possibly XCOMP_BV.  */
+    xsave->xsave_hdr.xstate_bv = xstate_bv;
+    xsave->xsave_hdr.xcomp_bv = v->arch.xcr0_accum | XSTATE_COMPACTION_ENABLED;
+
+    /*
+     * Copy each region from the non-compacted offset to the
+     * possibly compacted offset.
+     */
+    valid = xstate_bv & ~XSTATE_FP_SSE;
+    while ( valid )
+    {
+        u64 feature = valid & -valid;
+        int index = fls(feature) - 1;
+        void *dest = get_xsave_addr(xsave, feature);
+
+        if ( dest )
+        {
+            ASSERT((xstate_offsets[index] + xstate_sizes[index]) <= size);
+            memcpy(dest, src + xstate_offsets[index], xstate_sizes[index]);
+        }
+
+        valid -= feature;
+    }
+}
+
 void xsave(struct vcpu *v, uint64_t mask)
 {
     struct xsave_struct *ptr = v->arch.xsave_area;
diff --git a/xen/include/asm-x86/cpufeature.h b/xen/include/asm-x86/cpufeature.h
index 9a01563..ea60176 100644
--- a/xen/include/asm-x86/cpufeature.h
+++ b/xen/include/asm-x86/cpufeature.h
@@ -57,7 +57,11 @@
 #define X86_FEATURE_3DNOWEXT	(1*32+30) /* AMD 3DNow! extensions */
 #define X86_FEATURE_3DNOW	(1*32+31) /* 3DNow! */
 
-/* *** Available for re-use ***, word 2 */
+/* Intel-defined CPU features, CPUID level 0x0000000d (eax), word 2 */
+#define X86_FEATURE_XSAVEOPT	(2*32+0)
+#define X86_FEATURE_XSAVEC	(2*32+1)
+#define X86_FEATURE_XGETBV1	(2*32+2)
+#define X86_FEATURE_XSAVES	(2*32+3)
 
 /* Other features, Linux-defined mapping, word 3 */
 /* This range is used for feature bits which conflict or are synthesized */
diff --git a/xen/include/asm-x86/hvm/vcpu.h b/xen/include/asm-x86/hvm/vcpu.h
index f553814..de81f8a 100644
--- a/xen/include/asm-x86/hvm/vcpu.h
+++ b/xen/include/asm-x86/hvm/vcpu.h
@@ -173,6 +173,7 @@ struct hvm_vcpu {
 
     u32                 msr_tsc_aux;
     u64                 msr_tsc_adjust;
+    u64                 msr_xss;
 
     union {
         struct arch_vmx_struct vmx;
diff --git a/xen/include/asm-x86/msr-index.h b/xen/include/asm-x86/msr-index.h
index e9c4723..4e5b31f 100644
--- a/xen/include/asm-x86/msr-index.h
+++ b/xen/include/asm-x86/msr-index.h
@@ -58,6 +58,8 @@
 
 #define MSR_IA32_BNDCFGS		0x00000D90
 
+#define MSR_IA32_XSS			0x00000da0
+
 #define MSR_MTRRfix64K_00000		0x00000250
 #define MSR_MTRRfix16K_80000		0x00000258
 #define MSR_MTRRfix16K_A0000		0x00000259
diff --git a/xen/include/asm-x86/xstate.h b/xen/include/asm-x86/xstate.h
index 4c690db..a256525 100644
--- a/xen/include/asm-x86/xstate.h
+++ b/xen/include/asm-x86/xstate.h
@@ -22,7 +22,11 @@
 
 #define XCR_XFEATURE_ENABLED_MASK 0x00000000  /* index of XCR0 */
 
+#define XSAVE_HDR_SIZE            64
+#define XSAVE_SSE_OFFSET          160
 #define XSTATE_YMM_SIZE           256
+#define FXSAVE_SIZE               512
+#define XSAVE_HDR_OFFSET          FXSAVE_SIZE
 #define XSTATE_AREA_MIN_SIZE      (512 + 64)  /* FP/SSE + XSAVE.HEADER */
 
 #define XSTATE_FP      (1ULL << 0)
@@ -41,9 +45,11 @@
 #define XSTATE_ALL     (~(1ULL << 63))
 #define XSTATE_NONLAZY (XSTATE_LWP | XSTATE_BNDREGS | XSTATE_BNDCSR)
 #define XSTATE_LAZY    (XSTATE_ALL & ~XSTATE_NONLAZY)
+#define XSTATE_COMPACTION_ENABLED  (1ULL << 63)
 
 extern u64 xfeature_mask;
 extern bool_t cpu_has_xsaves, cpu_has_xgetbv1;
+extern unsigned int *xstate_offsets, *xstate_sizes;
 
 /* extended state save area */
 struct __packed __attribute__((aligned (64))) xsave_struct
@@ -72,7 +78,8 @@ struct __packed __attribute__((aligned (64))) xsave_struct
 
     struct {
         u64 xstate_bv;
-        u64 reserved[7];
+        u64 xcomp_bv;
+        u64 reserved[6];
     } xsave_hdr;                             /* The 64-byte header */
 
     struct { char x[XSTATE_YMM_SIZE]; } ymm; /* YMM */
@@ -82,11 +89,16 @@ struct __packed __attribute__((aligned (64))) xsave_struct
 /* extended state operations */
 bool_t __must_check set_xcr0(u64 xfeatures);
 uint64_t get_xcr0(void);
+void set_msr_xss(u64 xss);
+uint64_t get_msr_xss(void);
 void xsave(struct vcpu *v, uint64_t mask);
 void xrstor(struct vcpu *v, uint64_t mask);
 bool_t xsave_enabled(const struct vcpu *v);
 int __must_check validate_xstate(u64 xcr0, u64 xcr0_accum, u64 xstate_bv);
 int __must_check handle_xsetbv(u32 index, u64 new_bv);
+void save_xsave_states(struct vcpu *v, void *dest, unsigned int size);
+void load_xsave_states(struct vcpu *v, const void *src, unsigned int size);
+bool_t xsave_area_compressed(const struct xsave_struct *xsave_area);
 
 /* extended state init and cleanup functions */
 void xstate_free_save_area(struct vcpu *v);
-- 
1.9.1

[V5 2/4] x86/xsaves: enable xsaves/xrstors/xsavec in xen

[Shuai Ruan]

This patch uses xsaves/xrstors instead of xsaveopt/xrstor
to perform the xsave_area switching so that xen itself
can benefit from them when available.

For xsaves/xrstors only use compact format. Add format conversion
support when perform guest os migration.

Signed-off-by: Shuai Ruan <shuai.ruan@linux.intel.com>
---
 xen/arch/x86/domain.c        |   3 +
 xen/arch/x86/domctl.c        |  38 +++++++++++--
 xen/arch/x86/hvm/hvm.c       |  21 +++++--
 xen/arch/x86/i387.c          |   4 ++
 xen/arch/x86/traps.c         |   7 +--
 xen/arch/x86/xstate.c        | 132 ++++++++++++++++++++++++++++++-------------
 xen/include/asm-x86/xstate.h |   4 --
 7 files changed, 151 insertions(+), 58 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 045f6ff..b25094b 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1529,6 +1529,9 @@ static void __context_switch(void)
             if ( xcr0 != get_xcr0() && !set_xcr0(xcr0) )
                 BUG();
         }
+        if ( cpu_has_xsaves )
+            if ( is_hvm_vcpu(n) )
+                set_msr_xss(n->arch.hvm_vcpu.msr_xss);
         vcpu_restore_fpu_eager(n);
         n->arch.ctxt_switch_to(n);
     }
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index bf62a88..e2cd0d4 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -867,7 +867,7 @@ long arch_do_domctl(
         if ( domctl->cmd == XEN_DOMCTL_getvcpuextstate )
         {
             unsigned int size;
-
+            void * xsave_area;
             ret = 0;
             vcpu_pause(v);
 
@@ -896,9 +896,30 @@ long arch_do_domctl(
                 ret = -EFAULT;
 
             offset += sizeof(v->arch.xcr0_accum);
-            if ( !ret && copy_to_guest_offset(evc->buffer, offset,
-                                              (void *)v->arch.xsave_area,
-                                              size - 2 * sizeof(uint64_t)) )
+
+            if ( !ret && (cpu_has_xsaves || cpu_has_xsavec) &&
+                 xsave_area_compressed(v->arch.xsave_area) )
+            {
+                xsave_area = xmalloc_bytes(size);
+                if ( !xsave_area )
+                {
+                    ret = -ENOMEM;
+                    vcpu_unpause(v);
+                    goto vcpuextstate_out;
+                }
+
+                save_xsave_states(v, xsave_area,
+                                  evc->size - 2*sizeof(uint64_t));
+
+                if ( !ret && copy_to_guest_offset(evc->buffer, offset,
+					          xsave_area, size -
+		                                  2 * sizeof(uint64_t)) )
+                     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);
@@ -954,8 +975,13 @@ long arch_do_domctl(
                 v->arch.xcr0_accum = _xcr0_accum;
                 if ( _xcr0_accum & XSTATE_NONLAZY )
                     v->arch.nonlazy_xstate_used = 1;
-                memcpy(v->arch.xsave_area, _xsave_area,
-                       evc->size - 2 * sizeof(uint64_t));
+                if ( (cpu_has_xsaves || cpu_has_xsavec) &&
+		     !xsave_area_compressed(_xsave_area) )
+                    load_xsave_states(v, _xsave_area,
+                                      evc->size - 2*sizeof(uint64_t));
+                else
+                    memcpy(v->arch.xsave_area, (void *)_xsave_area,
+                           evc->size - 2 * sizeof(uint64_t));
                 vcpu_unpause(v);
             }
             else
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 615fa89..ad0a53b 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -2148,8 +2148,13 @@ static int hvm_save_cpu_xsave_states(struct domain *d, hvm_domain_context_t *h)
         ctxt->xfeature_mask = xfeature_mask;
         ctxt->xcr0 = v->arch.xcr0;
         ctxt->xcr0_accum = v->arch.xcr0_accum;
-        memcpy(&ctxt->save_area, v->arch.xsave_area,
-               size - offsetof(struct hvm_hw_cpu_xsave, save_area));
+	if ( (cpu_has_xsaves || cpu_has_xsavec) &&
+             (xsave_area_compressed(v->arch.xsave_area)) )
+            save_xsave_states(v, &ctxt->save_area,
+                              size - offsetof(typeof(*ctxt), save_area));
+        else
+            memcpy(&ctxt->save_area, v->arch.xsave_area,
+                   size - offsetof(struct hvm_hw_cpu_xsave, save_area));
     }
 
     return 0;
@@ -2248,9 +2253,15 @@ static int hvm_load_cpu_xsave_states(struct domain *d, hvm_domain_context_t *h)
     v->arch.xcr0_accum = ctxt->xcr0_accum;
     if ( ctxt->xcr0_accum & XSTATE_NONLAZY )
         v->arch.nonlazy_xstate_used = 1;
-    memcpy(v->arch.xsave_area, &ctxt->save_area,
-           min(desc->length, size) - offsetof(struct hvm_hw_cpu_xsave,
-           save_area));
+    if ( (cpu_has_xsaves || cpu_has_xsavec) &&
+          !xsave_area_compressed((struct xsave_struct *)&ctxt->save_area) )
+        load_xsave_states(v, &ctxt->save_area,
+                          min(desc->length, size) -
+                          offsetof(struct hvm_hw_cpu_xsave,save_area));
+    else
+        memcpy(v->arch.xsave_area, &ctxt->save_area,
+               min(desc->length, size) - offsetof(struct hvm_hw_cpu_xsave,
+               save_area));
 
     return 0;
 }
diff --git a/xen/arch/x86/i387.c b/xen/arch/x86/i387.c
index 14f2a79..736197f 100644
--- a/xen/arch/x86/i387.c
+++ b/xen/arch/x86/i387.c
@@ -309,7 +309,11 @@ 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 || cpu_has_xsavec )
+            v->arch.xsave_area->xsave_hdr.xcomp_bv |= XSTATE_COMPACTION_ENABLED;
+    }
     else
     {
         v->arch.fpu_ctxt = _xzalloc(sizeof(v->arch.xsave_area->fpu_sse), 16);
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 9f5a6c6..e3a84c5 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -935,10 +935,9 @@ void pv_cpuid(struct cpu_user_regs *regs)
             goto unsupported;
         if ( regs->_ecx == 1 )
         {
-            a &= XSTATE_FEATURE_XSAVEOPT |
-                 XSTATE_FEATURE_XSAVEC |
-                 (cpu_has_xgetbv1 ? XSTATE_FEATURE_XGETBV1 : 0) |
-                 (cpu_has_xsaves ? XSTATE_FEATURE_XSAVES : 0);
+            a &= cpufeat_mask(X86_FEATURE_XSAVEOPT) |
+                 cpufeat_mask(X86_FEATURE_XSAVEC) |
+                 (cpu_has_xgetbv1 ? cpufeat_mask(X86_FEATURE_XGETBV1) : 0);
             if ( !cpu_has_xsaves )
                 b = c = d = 0;
         }
diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index ff03b31..ae59a60 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -245,7 +245,15 @@ void xsave(struct vcpu *v, uint64_t mask)
         typeof(ptr->fpu_sse.fip.sel) fcs = ptr->fpu_sse.fip.sel;
         typeof(ptr->fpu_sse.fdp.sel) fds = ptr->fpu_sse.fdp.sel;
 
-        if ( cpu_has_xsaveopt )
+        if ( cpu_has_xsaves )
+            asm volatile ( ".byte 0x48,0x0f,0xc7,0x2f"
+                           : "=m" (*ptr)
+                           : "a" (lmask), "d" (hmask), "D" (ptr) );
+	else if ( cpu_has_xsavec )
+            asm volatile ( ".byte 0x48,0x0f,0xc7,0x27"
+                           : "=m" (*ptr)
+                           : "a" (lmask), "d" (hmask), "D" (ptr) );
+        else if ( cpu_has_xsaveopt )
         {
             /*
              * xsaveopt may not write the FPU portion even when the respective
@@ -298,7 +306,15 @@ void xsave(struct vcpu *v, uint64_t mask)
     }
     else
     {
-        if ( cpu_has_xsaveopt )
+        if ( cpu_has_xsaves )
+            asm volatile ( ".byte 0x48,0x0f,0xc7,0x2f"
+                           : "=m" (*ptr)
+                           : "a" (lmask), "d" (hmask), "D" (ptr) );
+	else if ( cpu_has_xsavec )
+            asm volatile ( ".byte 0x48,0x0f,0xc7,0x27"
+                           : "=m" (*ptr)
+                           : "a" (lmask), "d" (hmask), "D" (ptr) );
+        else if ( cpu_has_xsaveopt )
             asm volatile ( ".byte 0x0f,0xae,0x37"
                            : "=m" (*ptr)
                            : "a" (lmask), "d" (hmask), "D" (ptr) );
@@ -341,36 +357,68 @@ void xrstor(struct vcpu *v, uint64_t mask)
     switch ( __builtin_expect(ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET], 8) )
     {
     default:
-        asm volatile ( "1: .byte 0x48,0x0f,0xae,0x2f\n"
-                       ".section .fixup,\"ax\"      \n"
-                       "2: mov %5,%%ecx             \n"
-                       "   xor %1,%1                \n"
-                       "   rep stosb                \n"
-                       "   lea %2,%0                \n"
-                       "   mov %3,%1                \n"
-                       "   jmp 1b                   \n"
-                       ".previous                   \n"
-                       _ASM_EXTABLE(1b, 2b)
-                       : "+&D" (ptr), "+&a" (lmask)
-                       : "m" (*ptr), "g" (lmask), "d" (hmask),
-                         "m" (xsave_cntxt_size)
-                       : "ecx" );
-        break;
+        if ( cpu_has_xsaves )
+            asm volatile ( "1: .byte 0x48,0x0f,0xc7,0x1f\n"
+                           ".section .fixup,\"ax\"      \n"
+                           "2: mov %5,%%ecx             \n"
+                           "   xor %1,%1                \n"
+                           "   rep stosb                \n"
+                           "   lea %2,%0                \n"
+                           "   mov %3,%1                \n"
+                           "   jmp 1b                   \n"
+                           ".previous                   \n"
+                           _ASM_EXTABLE(1b, 2b)
+                           : "+&D" (ptr), "+&a" (lmask)
+                           : "m" (*ptr), "g" (lmask), "d" (hmask),
+                             "m" (xsave_cntxt_size)
+                           : "ecx" );
+	else
+            asm volatile ( "1: .byte 0x48,0x0f,0xae,0x2f\n"
+                           ".section .fixup,\"ax\"      \n"
+                           "2: mov %5,%%ecx             \n"
+                           "   xor %1,%1                \n"
+                           "   rep stosb                \n"
+                           "   lea %2,%0                \n"
+                           "   mov %3,%1                \n"
+                           "   jmp 1b                   \n"
+                           ".previous                   \n"
+                           _ASM_EXTABLE(1b, 2b)
+                           : "+&D" (ptr), "+&a" (lmask)
+                           : "m" (*ptr), "g" (lmask), "d" (hmask),
+                             "m" (xsave_cntxt_size)
+                           : "ecx" );
+            break;
     case 4: case 2:
-        asm volatile ( "1: .byte 0x0f,0xae,0x2f\n"
-                       ".section .fixup,\"ax\" \n"
-                       "2: mov %5,%%ecx        \n"
-                       "   xor %1,%1           \n"
-                       "   rep stosb           \n"
-                       "   lea %2,%0           \n"
-                       "   mov %3,%1           \n"
-                       "   jmp 1b              \n"
-                       ".previous              \n"
-                       _ASM_EXTABLE(1b, 2b)
-                       : "+&D" (ptr), "+&a" (lmask)
-                       : "m" (*ptr), "g" (lmask), "d" (hmask),
-                         "m" (xsave_cntxt_size)
-                       : "ecx" );
+        if ( cpu_has_xsaves )
+            asm volatile ( "1: .byte 0x48,0x0f,0xc7,0x1f\n"
+                           ".section .fixup,\"ax\"      \n"
+                           "2: mov %5,%%ecx             \n"
+                           "   xor %1,%1                \n"
+                           "   rep stosb                \n"
+                           "   lea %2,%0                \n"
+                           "   mov %3,%1                \n"
+                           "   jmp 1b                   \n"
+                           ".previous                   \n"
+                           _ASM_EXTABLE(1b, 2b)
+                           : "+&D" (ptr), "+&a" (lmask)
+                           : "m" (*ptr), "g" (lmask), "d" (hmask),
+                             "m" (xsave_cntxt_size)
+                           : "ecx" );
+	else
+            asm volatile ( "1: .byte 0x0f,0xae,0x2f\n"
+                           ".section .fixup,\"ax\" \n"
+                           "2: mov %5,%%ecx        \n"
+                           "   xor %1,%1           \n"
+                           "   rep stosb           \n"
+                           "   lea %2,%0           \n"
+                           "   mov %3,%1           \n"
+                           "   jmp 1b              \n"
+                           ".previous              \n"
+                           _ASM_EXTABLE(1b, 2b)
+                           : "+&D" (ptr), "+&a" (lmask)
+                           : "m" (*ptr), "g" (lmask), "d" (hmask),
+                             "m" (xsave_cntxt_size)
+                           : "ecx" );
         break;
     }
 }
@@ -495,18 +543,24 @@ void xstate_init(bool_t bsp)
     cpuid_count(XSTATE_CPUID, 1, &eax, &ebx, &ecx, &edx);
     if ( bsp )
     {
-        cpu_has_xsaveopt = !!(eax & XSTATE_FEATURE_XSAVEOPT);
-        cpu_has_xsavec = !!(eax & XSTATE_FEATURE_XSAVEC);
-        /* XXX cpu_has_xgetbv1 = !!(eax & XSTATE_FEATURE_XGETBV1); */
-        /* XXX cpu_has_xsaves = !!(eax & XSTATE_FEATURE_XSAVES); */
+        cpu_has_xsaveopt = !!(eax & cpufeat_mask(X86_FEATURE_XSAVEOPT));
+        cpu_has_xsavec = !!(eax & cpufeat_mask(X86_FEATURE_XSAVEC));
+        cpu_has_xgetbv1 = !!(eax & cpufeat_mask(X86_FEATURE_XGETBV1));
+        cpu_has_xsaves = !!(eax & cpufeat_mask(X86_FEATURE_XSAVES));
     }
     else
     {
-        BUG_ON(!cpu_has_xsaveopt != !(eax & XSTATE_FEATURE_XSAVEOPT));
-        BUG_ON(!cpu_has_xsavec != !(eax & XSTATE_FEATURE_XSAVEC));
-        /* XXX BUG_ON(!cpu_has_xgetbv1 != !(eax & XSTATE_FEATURE_XGETBV1)); */
-        /* XXX BUG_ON(!cpu_has_xsaves != !(eax & XSTATE_FEATURE_XSAVES)); */
+        BUG_ON(!cpu_has_xsaveopt != !(eax & cpufeat_mask(X86_FEATURE_XSAVEOPT)));
+        BUG_ON(!cpu_has_xsavec != !(eax & cpufeat_mask(X86_FEATURE_XSAVEC)));
+        BUG_ON(!cpu_has_xgetbv1 != !(eax & cpufeat_mask(X86_FEATURE_XGETBV1)));
+        BUG_ON(!cpu_has_xsaves != !(eax & cpufeat_mask(X86_FEATURE_XSAVES)));
     }
+
+    if( setup_xstate_features(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 a256525..715f096 100644
--- a/xen/include/asm-x86/xstate.h
+++ b/xen/include/asm-x86/xstate.h
@@ -15,10 +15,6 @@
 #define MXCSR_DEFAULT             0x1f80
 
 #define XSTATE_CPUID              0x0000000d
-#define XSTATE_FEATURE_XSAVEOPT   (1 << 0)    /* sub-leaf 1, eax[bit 0] */
-#define XSTATE_FEATURE_XSAVEC     (1 << 1)    /* sub-leaf 1, eax[bit 1] */
-#define XSTATE_FEATURE_XGETBV1    (1 << 2)    /* sub-leaf 1, eax[bit 2] */
-#define XSTATE_FEATURE_XSAVES     (1 << 3)    /* sub-leaf 1, eax[bit 3] */
 
 #define XCR_XFEATURE_ENABLED_MASK 0x00000000  /* index of XCR0 */
 
-- 
1.9.1

[V5 3/4] x86/xsaves: enable xsaves/xrstors for hvm guest

[Shuai Ruan]

This patch enables xsaves for hvm guest, includes:
1.handle xsaves vmcs init and vmexit.
2.add logic to write/read the XSS msr.

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Shuai Ruan <shuai.ruan@linux.intel.com>
---
 xen/arch/x86/hvm/hvm.c             | 30 ++++++++++++++++++++++++++++++
 xen/arch/x86/hvm/vmx/vmcs.c        |  6 ++++--
 xen/arch/x86/hvm/vmx/vmx.c         | 20 ++++++++++++++++++++
 xen/arch/x86/xstate.c              |  4 ++--
 xen/include/asm-x86/hvm/vmx/vmcs.h |  6 ++++++
 xen/include/asm-x86/hvm/vmx/vmx.h  |  2 ++
 xen/include/asm-x86/xstate.h       |  1 +
 7 files changed, 65 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index ad0a53b..ad2d572 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4550,6 +4550,23 @@ void hvm_cpuid(unsigned int input, unsigned int *eax, unsigned int *ebx,
                     *ebx = _eax + _ebx;
             }
         }
+        if ( count == 1 )
+        {
+            if ( cpu_has_xsaves )
+            {
+                *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)) )
+                            continue;
+                        *ebx += xstate_sizes[sub_leaf];
+                    }
+            }
+            else
+                *ebx = *ecx = *edx = 0;
+        }
         break;
 
     case 0x80000001:
@@ -4649,6 +4666,12 @@ int hvm_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
         *msr_content = v->arch.hvm_vcpu.guest_efer;
         break;
 
+    case MSR_IA32_XSS:
+        if ( !cpu_has_xsaves )
+            goto gp_fault;
+        *msr_content = v->arch.hvm_vcpu.msr_xss;
+        break;
+
     case MSR_IA32_TSC:
         *msr_content = _hvm_rdtsc_intercept();
         break;
@@ -4781,6 +4804,13 @@ int hvm_msr_write_intercept(unsigned int msr, uint64_t msr_content,
            return X86EMUL_EXCEPTION;
         break;
 
+    case MSR_IA32_XSS:
+	 /* No XSS features currently supported for guests. */
+        if ( !cpu_has_xsaves || msr_content != 0 )
+            goto gp_fault;
+        v->arch.hvm_vcpu.msr_xss = msr_content;
+        break;
+
     case MSR_IA32_TSC:
         hvm_set_guest_tsc(v, msr_content);
         break;
diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index a0a97e7..258cf17 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -236,7 +236,8 @@ static int vmx_init_vmcs_config(void)
                SECONDARY_EXEC_PAUSE_LOOP_EXITING |
                SECONDARY_EXEC_ENABLE_INVPCID |
                SECONDARY_EXEC_ENABLE_VM_FUNCTIONS |
-               SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS);
+               SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS |
+               SECONDARY_EXEC_XSAVES);
         rdmsrl(MSR_IA32_VMX_MISC, _vmx_misc_cap);
         if ( _vmx_misc_cap & VMX_MISC_VMWRITE_ALL )
             opt |= SECONDARY_EXEC_ENABLE_VMCS_SHADOWING;
@@ -1238,7 +1239,8 @@ static int construct_vmcs(struct vcpu *v)
         __vmwrite(HOST_PAT, host_pat);
         __vmwrite(GUEST_PAT, guest_pat);
     }
-
+    if ( cpu_has_vmx_xsaves )
+        __vmwrite(XSS_EXIT_BITMAP, 0);
     vmx_vmcs_exit(v);
 
     /* PVH: paging mode is updated by arch_set_info_guest(). */
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 2582cdd..b07e1d2 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2833,6 +2833,18 @@ static void vmx_idtv_reinject(unsigned long idtv_info)
     }
 }
 
+static void vmx_handle_xsaves(void)
+{
+    gdprintk(XENLOG_ERR, "xsaves should not cause vmexit\n");
+    domain_crash(current->domain);
+}
+
+static void vmx_handle_xrstors(void)
+{
+    gdprintk(XENLOG_ERR, "xrstors should not cause vmexit\n");
+    domain_crash(current->domain);
+}
+
 static int vmx_handle_apic_write(void)
 {
     unsigned long exit_qualification;
@@ -3404,6 +3416,14 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
         vmx_vcpu_flush_pml_buffer(v);
         break;
 
+    case EXIT_REASON_XSAVES:
+        vmx_handle_xsaves();
+        break;
+
+    case EXIT_REASON_XRSTORS:
+        vmx_handle_xrstors();
+        break;
+
     case EXIT_REASON_ACCESS_GDTR_OR_IDTR:
     case EXIT_REASON_ACCESS_LDTR_OR_TR:
     case EXIT_REASON_VMX_PREEMPTION_TIMER_EXPIRED:
diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index ae59a60..5940acd 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -14,8 +14,8 @@
 #include <asm/xstate.h>
 #include <asm/asm_defns.h>
 
-static bool_t __read_mostly cpu_has_xsaveopt;
-static bool_t __read_mostly cpu_has_xsavec;
+bool_t __read_mostly cpu_has_xsaveopt;
+bool_t __read_mostly cpu_has_xsavec;
 bool_t __read_mostly cpu_has_xgetbv1;
 bool_t __read_mostly cpu_has_xsaves;
 
diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h b/xen/include/asm-x86/hvm/vmx/vmcs.h
index f1126d4..81918e1 100644
--- a/xen/include/asm-x86/hvm/vmx/vmcs.h
+++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
@@ -225,6 +225,7 @@ extern u32 vmx_vmentry_control;
 #define SECONDARY_EXEC_ENABLE_VMCS_SHADOWING    0x00004000
 #define SECONDARY_EXEC_ENABLE_PML               0x00020000
 #define SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS   0x00040000
+#define SECONDARY_EXEC_XSAVES                   0x00100000
 extern u32 vmx_secondary_exec_control;
 
 #define VMX_EPT_EXEC_ONLY_SUPPORTED             0x00000001
@@ -240,6 +241,8 @@ extern u32 vmx_secondary_exec_control;
 
 #define VMX_MISC_VMWRITE_ALL                    0x20000000
 
+#define VMX_XSS_EXIT_BITMAP                     0
+
 #define VMX_VPID_INVVPID_INSTRUCTION                        0x100000000ULL
 #define VMX_VPID_INVVPID_INDIVIDUAL_ADDR                    0x10000000000ULL
 #define VMX_VPID_INVVPID_SINGLE_CONTEXT                     0x20000000000ULL
@@ -291,6 +294,8 @@ extern u32 vmx_secondary_exec_control;
     (vmx_secondary_exec_control & SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS)
 #define cpu_has_vmx_pml \
     (vmx_secondary_exec_control & SECONDARY_EXEC_ENABLE_PML)
+#define cpu_has_vmx_xsaves \
+    (vmx_secondary_exec_control & SECONDARY_EXEC_XSAVES)
 
 #define VMCS_RID_TYPE_MASK              0x80000000
 
@@ -365,6 +370,7 @@ enum vmcs_field {
     VMREAD_BITMAP                   = 0x00002026,
     VMWRITE_BITMAP                  = 0x00002028,
     VIRT_EXCEPTION_INFO             = 0x0000202a,
+    XSS_EXIT_BITMAP                 = 0x0000202c,
     GUEST_PHYSICAL_ADDRESS          = 0x00002400,
     VMCS_LINK_POINTER               = 0x00002800,
     GUEST_IA32_DEBUGCTL             = 0x00002802,
diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h b/xen/include/asm-x86/hvm/vmx/vmx.h
index 3fbfa44..bfc2368 100644
--- a/xen/include/asm-x86/hvm/vmx/vmx.h
+++ b/xen/include/asm-x86/hvm/vmx/vmx.h
@@ -188,6 +188,8 @@ static inline unsigned long pi_get_pir(struct pi_desc *pi_desc, int group)
 #define EXIT_REASON_INVPCID             58
 #define EXIT_REASON_VMFUNC              59
 #define EXIT_REASON_PML_FULL            62
+#define EXIT_REASON_XSAVES              63
+#define EXIT_REASON_XRSTORS             64
 
 /*
  * Interruption-information format
diff --git a/xen/include/asm-x86/xstate.h b/xen/include/asm-x86/xstate.h
index 715f096..4b2aa5f 100644
--- a/xen/include/asm-x86/xstate.h
+++ b/xen/include/asm-x86/xstate.h
@@ -45,6 +45,7 @@
 
 extern u64 xfeature_mask;
 extern bool_t cpu_has_xsaves, cpu_has_xgetbv1;
+extern bool_t cpu_has_xsavec, cpu_has_xsaveopt;
 extern unsigned int *xstate_offsets, *xstate_sizes;
 
 /* extended state save area */
-- 
1.9.1

[V5 4/4] libxc: expose xsaves/xgetbv1/xsavec to hvm guest

[Shuai Ruan]

This patch exposes xsaves/xgetbv1/xsavec to hvm guest.
The reserved bits of eax/ebx/ecx/edx must be cleaned up
when call cpuid(0dh) with leaf 1 or 2..63.

According to the spec the following bits must be reserved:
For leaf 1, bits 03-04/08-31 of ecx is reserved. Edx is reserved.
For leaf 2...63, bits 01-31 of ecx is reserved. Edx is reserved.

Acked-by: Ian Campbell <ian.campbell@citrix.com>
Signed-off-by: Shuai Ruan <shuai.ruan@linux.intel.com>
---
 tools/libxc/xc_cpuid_x86.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
index e146a3e..538d356 100644
--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
@@ -210,6 +210,10 @@ static void intel_xc_cpuid_policy(
 }
 
 #define XSAVEOPT        (1 << 0)
+#define XSAVEC          (1 << 1)
+#define XGETBV1         (1 << 2)
+#define XSAVES          (1 << 3)
+#define XSS_SUPPORT     (1 << 0)
 /* Configure extended state enumeration leaves (0x0000000D for xsave) */
 static void xc_cpuid_config_xsave(
     xc_interface *xch, domid_t domid, uint64_t xfeature_mask,
@@ -246,8 +250,9 @@ static void xc_cpuid_config_xsave(
         regs[1] = 512 + 64; /* FP/SSE + XSAVE.HEADER */
         break;
     case 1: /* leaf 1 */
-        regs[0] &= XSAVEOPT;
-        regs[1] = regs[2] = regs[3] = 0;
+        regs[0] &= (XSAVEOPT | XSAVEC | XGETBV1 | XSAVES);
+        regs[2] &= xfeature_mask;
+        regs[3] = 0;
         break;
     case 2 ... 63: /* sub-leaves */
         if ( !(xfeature_mask & (1ULL << input[1])) )
@@ -255,8 +260,9 @@ static void xc_cpuid_config_xsave(
             regs[0] = regs[1] = regs[2] = regs[3] = 0;
             break;
         }
-        /* Don't touch EAX, EBX. Also cleanup ECX and EDX */
-        regs[2] = regs[3] = 0;
+        /* Don't touch EAX, EBX. Also cleanup EDX. Cleanup bits 01-32 of ECX*/
+        regs[2] &= XSS_SUPPORT;
+        regs[3] = 0;
         break;
     }
 }
-- 
1.9.1

Re: [V5 4/4] libxc: expose xsaves/xgetbv1/xsavec to hvm guest

[Jan Beulich]

>>> On 21.09.15 at 13:34, <shuai.ruan@linux.intel.com> wrote:
> @@ -255,8 +260,9 @@ static void xc_cpuid_config_xsave(
>              regs[0] = regs[1] = regs[2] = regs[3] = 0;
>              break;
>          }
> -        /* Don't touch EAX, EBX. Also cleanup ECX and EDX */
> -        regs[2] = regs[3] = 0;
> +        /* Don't touch EAX, EBX. Also cleanup EDX. Cleanup bits 01-32 of ECX*/

This comment is off by one, and it will become stale the moment
XSS_SUPPORT gets added to. Better to write this in a more
generic way.

Jan

> +        regs[2] &= XSS_SUPPORT;
> +        regs[3] = 0;
>          break;
>      }
>  }

Re: [V5 1/4] x86/xsaves: add basic definitions/helpers to support xsaves

[Jan Beulich]

>>> On 21.09.15 at 13:33, <shuai.ruan@linux.intel.com> wrote:
> This patch add basic definitions/helpers which will be used in
> later patches.
> 
> Signed-off-by: Shuai Ruan <shuai.ruan@linux.intel.com>
> ---

Missing brief description of changes in v5 here.

> --- a/xen/arch/x86/xstate.c
> +++ b/xen/arch/x86/xstate.c
> @@ -29,12 +29,21 @@ static u32 __read_mostly xsave_cntxt_size;
>  /* A 64-bit bitmask of the XSAVE/XRSTOR features supported by processor. */
>  u64 __read_mostly xfeature_mask;
>  
> +unsigned int * __read_mostly xstate_offsets;
> +unsigned int * __read_mostly xstate_sizes;
> +static unsigned int __read_mostly xstate_features;
> +static unsigned int __read_mostly 
> xstate_comp_offsets[sizeof(xfeature_mask)*8];
> +
> +/* Cached msr_xss for fast read */
> +static DEFINE_PER_CPU(uint64_t, msr_xss);

Any reason not to call this just xss?

>  /* Cached xcr0 for fast read */
>  static DEFINE_PER_CPU(uint64_t, xcr0);
>  
>  /* Because XCR0 is cached for each CPU, xsetbv() is not exposed. Users should 
>   * use set_xcr0() instead.
>   */
> +
>  static inline bool_t xsetbv(u32 index, u64 xfeatures)

Stray addition of a blank line.

> +bool_t xsave_area_compressed(const struct xsave_struct *xsave_area)
> +{
> +    if ( xsave_area && (xsave_area->xsave_hdr.xcomp_bv
> +                          & XSTATE_COMPACTION_ENABLED))
> +	    return 1;
> +    return 0;
> +}

The body of the function could a single return statement.

> +static int setup_xstate_features(bool_t bsp)
> +{
> +    unsigned int leaf, tmp, eax, ebx;
> +
> +    if ( bsp )
> +        xstate_features = fls(xfeature_mask);
> +
> +    if ( bsp )

The two if()s should be combined.

> +    {
> +        xstate_offsets = xzalloc_array(unsigned int, xstate_features);
> +        if( !xstate_offsets )
> +            return -ENOMEM;
> +
> +        xstate_sizes = xzalloc_array(unsigned int, xstate_features);
> +        if( !xstate_sizes )
> +            return -ENOMEM;
> +    }
> +
> +    for (leaf = 2; leaf < xstate_features; leaf++)

Coding style.

> +    {
> +        if( bsp )

Again.

> +            cpuid_count(XSTATE_CPUID, leaf, &xstate_sizes[leaf],
> +                        &xstate_offsets[leaf], &tmp, &tmp);
> +        else
> +        {
> +             cpuid_count(XSTATE_CPUID, leaf, &eax,
> +                        &ebx, &tmp, &tmp);
> +             BUG_ON(eax != xstate_sizes[leaf]);
> +             BUG_ON(ebx != xstate_offsets[leaf]);
> +        }
> +     }
> +     return 0;

Blank line before final return statement please.

> +}
> +
> +static void setup_xstate_comp(void)
> +{
> +    unsigned int i;
> +
> +    /*
> +     * 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;
> +
> +    xstate_comp_offsets[2] = FXSAVE_SIZE + XSAVE_HDR_SIZE;
> +
> +    for (i = 3; i < xstate_features; i++)

Coding style again.

> +    {
> +        xstate_comp_offsets[i] = xstate_comp_offsets[i-1] + (((1ul << i)
> +                                 & xfeature_mask) ? xstate_sizes[i-1] : 0);

Indentation should match up with the parentheses. Which likely
means you'll want to break the line after the +.

> +        ASSERT(xstate_comp_offsets[i] <= xsave_cntxt_size);

How about the last component's offset + size exceeding
xsave_cntxt_size?

Also just considering what the function does, it looks like it ought to
be __init.

> +void save_xsave_states(struct vcpu *v, void *dest, unsigned int size)

Iirc it was suggested before that the function's name isn't adequate:
There's no saving of anything here. You're expanding already saved
state.

> +{
> +    struct xsave_struct *xsave = v->arch.xsave_area;

const?

> +    u64 xstate_bv = xsave->xsave_hdr.xstate_bv;
> +    u64 valid;
> +
> +    ASSERT(xsave_area_compressed(xsave));
> +    /*
> +     * Copy legacy XSAVE area, to avoid complications with CPUID
> +     * leaves 0 and 1 in the loop below.
> +     */
> +    memcpy(dest, xsave, FXSAVE_SIZE);
> +
> +    /* Set XSTATE_BV */
> +    *(u64 *)(dest + XSAVE_HDR_OFFSET) = xstate_bv;

Can't you avoid fragile casts like this by using a proper structure
type for dest?

> +    /*
> +     * Copy each region from the possibly compacted offset to the
> +     * non-compacted offset.
> +     */
> +    valid = xstate_bv & ~XSTATE_FP_SSE;
> +    while ( valid )
> +    {
> +        u64 feature = valid & -valid;
> +        int index = fls(feature) - 1;

unsigned int

> +        void *src = get_xsave_addr(xsave, index);

const

> +        if ( src )
> +        {
> +            ASSERT((xstate_offsets[index] + xstate_sizes[index]) <= size);
> +            memcpy(dest + xstate_offsets[index], src, xstate_sizes[index]);
> +        }
> +
> +        valid -= feature;

While correct this way, I would consider "&= ~feature" more natural.

> +    }
> +
> +}
> +
> +void load_xsave_states(struct vcpu *v, const void *src, unsigned int size)

Similar comments as above apply to this function.

> +    /* Set XSTATE_BV and possibly XCOMP_BV.  */
> +    xsave->xsave_hdr.xstate_bv = xstate_bv;
> +    xsave->xsave_hdr.xcomp_bv = v->arch.xcr0_accum | XSTATE_COMPACTION_ENABLED;

Why does the comment say "possibly" when it's done unconditionally?

Also, as a general comment - both functions get added without any
user, but they place constraints on the values they may be passed
(the src/dest pointers must not overlap the v->arch.xsave_area),
which is ugly to validate without seeing the intended callers. Perhaps,
especially if you want to keep the patch splitting as it is, adding
respective ASSERT()s would make this more explicit.

Jan

Re: [V5 2/4] x86/xsaves: enable xsaves/xrstors/xsavec in xen

[Jan Beulich]

>>> On 21.09.15 at 13:33, <shuai.ruan@linux.intel.com> wrote:
> This patch uses xsaves/xrstors instead of xsaveopt/xrstor
> to perform the xsave_area switching so that xen itself
> can benefit from them when available.
> 
> For xsaves/xrstors only use compact format. Add format conversion
> support when perform guest os migration.

Assuming the hardware has XSAVEC but no XSAVES support - why
wouldn't we want use XSAVEC in that case?

> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -1529,6 +1529,9 @@ static void __context_switch(void)
>              if ( xcr0 != get_xcr0() && !set_xcr0(xcr0) )
>                  BUG();
>          }
> +        if ( cpu_has_xsaves )
> +            if ( is_hvm_vcpu(n) )
> +                set_msr_xss(n->arch.hvm_vcpu.msr_xss);

The two if()s should be combined.

> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -867,7 +867,7 @@ long arch_do_domctl(
>          if ( domctl->cmd == XEN_DOMCTL_getvcpuextstate )
>          {
>              unsigned int size;
> -
> +            void * xsave_area;
>              ret = 0;

Stray removal of a blank line. Stray blank after *.

> @@ -896,9 +896,30 @@ long arch_do_domctl(
>                  ret = -EFAULT;
>  
>              offset += sizeof(v->arch.xcr0_accum);
> -            if ( !ret && copy_to_guest_offset(evc->buffer, offset,
> -                                              (void *)v->arch.xsave_area,
> -                                              size - 2 * sizeof(uint64_t)) )
> +
> +            if ( !ret && (cpu_has_xsaves || cpu_has_xsavec) &&

Ah, you actually use it, but don't say so in the description.

> +                 xsave_area_compressed(v->arch.xsave_area) )
> +            {
> +                xsave_area = xmalloc_bytes(size);

Perhaps this variable would better be declared here anyway.

> +                if ( !xsave_area )
> +                {
> +                    ret = -ENOMEM;
> +                    vcpu_unpause(v);
> +                    goto vcpuextstate_out;

This is unfortunate - it would be better if you could avoid failing
the request here.

> +                }
> +
> +                save_xsave_states(v, xsave_area,
> +                                  evc->size - 2*sizeof(uint64_t));

Blanks around * please.

> +
> +                if ( !ret && copy_to_guest_offset(evc->buffer, offset,

There's no way for ret to be non-zero at this point.

> +					          xsave_area, size -

Hard tabs (more elsewhere).

> @@ -954,8 +975,13 @@ long arch_do_domctl(
>                  v->arch.xcr0_accum = _xcr0_accum;
>                  if ( _xcr0_accum & XSTATE_NONLAZY )
>                      v->arch.nonlazy_xstate_used = 1;
> -                memcpy(v->arch.xsave_area, _xsave_area,
> -                       evc->size - 2 * sizeof(uint64_t));
> +                if ( (cpu_has_xsaves || cpu_has_xsavec) &&
> +		     !xsave_area_compressed(_xsave_area) )

Is it intended to support compact input here? Where would such
come from? And if so, don't you need to validate the input (e.g.
being a certain size)?

> @@ -2248,9 +2253,15 @@ static int hvm_load_cpu_xsave_states(struct domain *d, hvm_domain_context_t *h)
>      v->arch.xcr0_accum = ctxt->xcr0_accum;
>      if ( ctxt->xcr0_accum & XSTATE_NONLAZY )
>          v->arch.nonlazy_xstate_used = 1;
> -    memcpy(v->arch.xsave_area, &ctxt->save_area,
> -           min(desc->length, size) - offsetof(struct hvm_hw_cpu_xsave,
> -           save_area));
> +    if ( (cpu_has_xsaves || cpu_has_xsavec) &&
> +          !xsave_area_compressed((struct xsave_struct *)&ctxt->save_area) )

Same questions here.

> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -935,10 +935,9 @@ void pv_cpuid(struct cpu_user_regs *regs)
>              goto unsupported;
>          if ( regs->_ecx == 1 )
>          {
> -            a &= XSTATE_FEATURE_XSAVEOPT |
> -                 XSTATE_FEATURE_XSAVEC |
> -                 (cpu_has_xgetbv1 ? XSTATE_FEATURE_XGETBV1 : 0) |
> -                 (cpu_has_xsaves ? XSTATE_FEATURE_XSAVES : 0);
> +            a &= cpufeat_mask(X86_FEATURE_XSAVEOPT) |
> +                 cpufeat_mask(X86_FEATURE_XSAVEC) |
> +                 (cpu_has_xgetbv1 ? cpufeat_mask(X86_FEATURE_XGETBV1) : 0);

I think you indeed mean to drop xsaves here, but then you should
say so (i.e. PV unsupported) in the commit message

>              if ( !cpu_has_xsaves )
>                  b = c = d = 0;

And it would seem that this then needs to become unconditional?

> @@ -341,36 +357,68 @@ void xrstor(struct vcpu *v, uint64_t mask)
>      switch ( __builtin_expect(ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET], 8) )
>      {
>      default:
> -        asm volatile ( "1: .byte 0x48,0x0f,0xae,0x2f\n"
> -                       ".section .fixup,\"ax\"      \n"
> -                       "2: mov %5,%%ecx             \n"
> -                       "   xor %1,%1                \n"
> -                       "   rep stosb                \n"
> -                       "   lea %2,%0                \n"
> -                       "   mov %3,%1                \n"
> -                       "   jmp 1b                   \n"
> -                       ".previous                   \n"
> -                       _ASM_EXTABLE(1b, 2b)
> -                       : "+&D" (ptr), "+&a" (lmask)
> -                       : "m" (*ptr), "g" (lmask), "d" (hmask),
> -                         "m" (xsave_cntxt_size)
> -                       : "ecx" );
> -        break;
> +        if ( cpu_has_xsaves )
> +            asm volatile ( "1: .byte 0x48,0x0f,0xc7,0x1f\n"
> +                           ".section .fixup,\"ax\"      \n"
> +                           "2: mov %5,%%ecx             \n"
> +                           "   xor %1,%1                \n"
> +                           "   rep stosb                \n"
> +                           "   lea %2,%0                \n"
> +                           "   mov %3,%1                \n"
> +                           "   jmp 1b                   \n"
> +                           ".previous                   \n"
> +                           _ASM_EXTABLE(1b, 2b)
> +                           : "+&D" (ptr), "+&a" (lmask)
> +                           : "m" (*ptr), "g" (lmask), "d" (hmask),
> +                             "m" (xsave_cntxt_size)
> +                           : "ecx" );
> +	else
> +            asm volatile ( "1: .byte 0x48,0x0f,0xae,0x2f\n"
> +                           ".section .fixup,\"ax\"      \n"
> +                           "2: mov %5,%%ecx             \n"
> +                           "   xor %1,%1                \n"
> +                           "   rep stosb                \n"
> +                           "   lea %2,%0                \n"
> +                           "   mov %3,%1                \n"
> +                           "   jmp 1b                   \n"
> +                           ".previous                   \n"
> +                           _ASM_EXTABLE(1b, 2b)
> +                           : "+&D" (ptr), "+&a" (lmask)
> +                           : "m" (*ptr), "g" (lmask), "d" (hmask),
> +                             "m" (xsave_cntxt_size)
> +                           : "ecx" );
> +            break;
>      case 4: case 2:
> -        asm volatile ( "1: .byte 0x0f,0xae,0x2f\n"
> -                       ".section .fixup,\"ax\" \n"
> -                       "2: mov %5,%%ecx        \n"
> -                       "   xor %1,%1           \n"
> -                       "   rep stosb           \n"
> -                       "   lea %2,%0           \n"
> -                       "   mov %3,%1           \n"
> -                       "   jmp 1b              \n"
> -                       ".previous              \n"
> -                       _ASM_EXTABLE(1b, 2b)
> -                       : "+&D" (ptr), "+&a" (lmask)
> -                       : "m" (*ptr), "g" (lmask), "d" (hmask),
> -                         "m" (xsave_cntxt_size)
> -                       : "ecx" );
> +        if ( cpu_has_xsaves )
> +            asm volatile ( "1: .byte 0x48,0x0f,0xc7,0x1f\n"
> +                           ".section .fixup,\"ax\"      \n"
> +                           "2: mov %5,%%ecx             \n"
> +                           "   xor %1,%1                \n"
> +                           "   rep stosb                \n"
> +                           "   lea %2,%0                \n"
> +                           "   mov %3,%1                \n"
> +                           "   jmp 1b                   \n"
> +                           ".previous                   \n"
> +                           _ASM_EXTABLE(1b, 2b)
> +                           : "+&D" (ptr), "+&a" (lmask)
> +                           : "m" (*ptr), "g" (lmask), "d" (hmask),
> +                             "m" (xsave_cntxt_size)
> +                           : "ecx" );
> +	else
> +            asm volatile ( "1: .byte 0x0f,0xae,0x2f\n"
> +                           ".section .fixup,\"ax\" \n"
> +                           "2: mov %5,%%ecx        \n"
> +                           "   xor %1,%1           \n"
> +                           "   rep stosb           \n"
> +                           "   lea %2,%0           \n"
> +                           "   mov %3,%1           \n"
> +                           "   jmp 1b              \n"
> +                           ".previous              \n"
> +                           _ASM_EXTABLE(1b, 2b)
> +                           : "+&D" (ptr), "+&a" (lmask)
> +                           : "m" (*ptr), "g" (lmask), "d" (hmask),
> +                             "m" (xsave_cntxt_size)
> +                           : "ecx" );

The amount of redundancy calls for some helper macros.

> @@ -495,18 +543,24 @@ void xstate_init(bool_t bsp)
>      cpuid_count(XSTATE_CPUID, 1, &eax, &ebx, &ecx, &edx);
>      if ( bsp )
>      {
> -        cpu_has_xsaveopt = !!(eax & XSTATE_FEATURE_XSAVEOPT);
> -        cpu_has_xsavec = !!(eax & XSTATE_FEATURE_XSAVEC);
> -        /* XXX cpu_has_xgetbv1 = !!(eax & XSTATE_FEATURE_XGETBV1); */
> -        /* XXX cpu_has_xsaves = !!(eax & XSTATE_FEATURE_XSAVES); */
> +        cpu_has_xsaveopt = !!(eax & cpufeat_mask(X86_FEATURE_XSAVEOPT));
> +        cpu_has_xsavec = !!(eax & cpufeat_mask(X86_FEATURE_XSAVEC));
> +        cpu_has_xgetbv1 = !!(eax & cpufeat_mask(X86_FEATURE_XGETBV1));
> +        cpu_has_xsaves = !!(eax & cpufeat_mask(X86_FEATURE_XSAVES));
>      }
>      else
>      {
> -        BUG_ON(!cpu_has_xsaveopt != !(eax & XSTATE_FEATURE_XSAVEOPT));
> -        BUG_ON(!cpu_has_xsavec != !(eax & XSTATE_FEATURE_XSAVEC));
> -        /* XXX BUG_ON(!cpu_has_xgetbv1 != !(eax & XSTATE_FEATURE_XGETBV1)); */
> -        /* XXX BUG_ON(!cpu_has_xsaves != !(eax & XSTATE_FEATURE_XSAVES)); */
> +        BUG_ON(!cpu_has_xsaveopt != !(eax & cpufeat_mask(X86_FEATURE_XSAVEOPT)));
> +        BUG_ON(!cpu_has_xsavec != !(eax & cpufeat_mask(X86_FEATURE_XSAVEC)));
> +        BUG_ON(!cpu_has_xgetbv1 != !(eax & cpufeat_mask(X86_FEATURE_XGETBV1)));
> +        BUG_ON(!cpu_has_xsaves != !(eax & cpufeat_mask(X86_FEATURE_XSAVES)));
>      }
> +
> +    if( setup_xstate_features(bsp) )
> +        BUG();
> +
> +    if ( bsp && (cpu_has_xsaves || cpu_has_xsavec) )
> +        setup_xstate_comp();

Implying that the function should be annotated __init in patch 1. With
it being static there without having a caller, it also means things wouldn't
build with just patch 1 applied. This needs to be addressed.

Jan

Re: [V5 3/4] x86/xsaves: enable xsaves/xrstors for hvm guest

[Jan Beulich]

>>> On 21.09.15 at 13:34, <shuai.ruan@linux.intel.com> wrote:
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4550,6 +4550,23 @@ void hvm_cpuid(unsigned int input, unsigned int *eax, unsigned int *ebx,
>                      *ebx = _eax + _ebx;
>              }
>          }
> +        if ( count == 1 )
> +        {
> +            if ( cpu_has_xsaves )

Doesn't this also depend on the respective VMX capability (which
of course you shouldn't check directly here)?

> +            {
> +                *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)) )

Indentation.

> +                            continue;

Please invert the condition and drop the continue.

> @@ -4781,6 +4804,13 @@ int hvm_msr_write_intercept(unsigned int msr, uint64_t msr_content,
>             return X86EMUL_EXCEPTION;
>          break;
>  
> +    case MSR_IA32_XSS:
> +	 /* No XSS features currently supported for guests. */

Hard tab. Also - is the patch of much use then?

> @@ -1238,7 +1239,8 @@ static int construct_vmcs(struct vcpu *v)
>          __vmwrite(HOST_PAT, host_pat);
>          __vmwrite(GUEST_PAT, guest_pat);
>      }
> -
> +    if ( cpu_has_vmx_xsaves )
> +        __vmwrite(XSS_EXIT_BITMAP, 0);
>      vmx_vmcs_exit(v);

Instead of removing a blank line I'd rather see you add another one
before the call to vmx_vmcs_exit().

> --- a/xen/arch/x86/xstate.c
> +++ b/xen/arch/x86/xstate.c
> @@ -14,8 +14,8 @@
>  #include <asm/xstate.h>
>  #include <asm/asm_defns.h>
>  
> -static bool_t __read_mostly cpu_has_xsaveopt;
> -static bool_t __read_mostly cpu_has_xsavec;
> +bool_t __read_mostly cpu_has_xsaveopt;
> +bool_t __read_mostly cpu_has_xsavec;

Why? (But iirc this will go away anyway once re-based on Andrew's
changes.)

> --- a/xen/include/asm-x86/hvm/vmx/vmcs.h
> +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
> @@ -225,6 +225,7 @@ extern u32 vmx_vmentry_control;
>  #define SECONDARY_EXEC_ENABLE_VMCS_SHADOWING    0x00004000
>  #define SECONDARY_EXEC_ENABLE_PML               0x00020000
>  #define SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS   0x00040000
> +#define SECONDARY_EXEC_XSAVES                   0x00100000
>  extern u32 vmx_secondary_exec_control;
>  
>  #define VMX_EPT_EXEC_ONLY_SUPPORTED             0x00000001
> @@ -240,6 +241,8 @@ extern u32 vmx_secondary_exec_control;
>  
>  #define VMX_MISC_VMWRITE_ALL                    0x20000000
>  
> +#define VMX_XSS_EXIT_BITMAP                     0

What use is this addition without it having any user? (And without
any user judging whether this is a meaningful definition is also
impossible.)

Jan

Re: [V5 2/4] x86/xsaves: enable xsaves/xrstors/xsavec in xen

[Shuai Ruan]

On Mon, Sep 28, 2015 at 10:03:12AM -0600, Jan Beulich wrote:
> >>> On 21.09.15 at 13:33, <shuai.ruan@linux.intel.com> wrote:
> > @@ -954,8 +975,13 @@ long arch_do_domctl(
> >                  v->arch.xcr0_accum = _xcr0_accum;
> >                  if ( _xcr0_accum & XSTATE_NONLAZY )
> >                      v->arch.nonlazy_xstate_used = 1;
> > -                memcpy(v->arch.xsave_area, _xsave_area,
> > -                       evc->size - 2 * sizeof(uint64_t));
> > +                if ( (cpu_has_xsaves || cpu_has_xsavec) &&
> > +		     !xsave_area_compressed(_xsave_area) )
> 
> Is it intended to support compact input here? Where would such
> come from? And if so, don't you need to validate the input (e.g.
> being a certain size)?
> 
It is not intended to support compact input here.Just add some check here (According to Andrew suggestion).

Thanks 

Re: [V5 2/4] x86/xsaves: enable xsaves/xrstors/xsavec in xen

[Jan Beulich]

>>> On 09.10.15 at 07:49, <shuai.ruan@linux.intel.com> wrote:
> On Mon, Sep 28, 2015 at 10:03:12AM -0600, Jan Beulich wrote:
>> >>> On 21.09.15 at 13:33, <shuai.ruan@linux.intel.com> wrote:
>> > @@ -954,8 +975,13 @@ long arch_do_domctl(
>> >                  v->arch.xcr0_accum = _xcr0_accum;
>> >                  if ( _xcr0_accum & XSTATE_NONLAZY )
>> >                      v->arch.nonlazy_xstate_used = 1;
>> > -                memcpy(v->arch.xsave_area, _xsave_area,
>> > -                       evc->size - 2 * sizeof(uint64_t));
>> > +                if ( (cpu_has_xsaves || cpu_has_xsavec) &&
>> > +		     !xsave_area_compressed(_xsave_area) )
>> 
>> Is it intended to support compact input here? Where would such
>> come from? And if so, don't you need to validate the input (e.g.
>> being a certain size)?
>> 
> It is not intended to support compact input here.Just add some check here 
> (According to Andrew suggestion).

I think your reply only refers to the cpu_has_xsavec conditional,
but the question really was about the construct as a whole.

Jan

Re: [V5 2/4] x86/xsaves: enable xsaves/xrstors/xsavec in xen

[Shuai Ruan]

On Fri, Oct 09, 2015 at 01:13:11AM -0600, Jan Beulich wrote:
> >>> On 09.10.15 at 07:49, <shuai.ruan@linux.intel.com> wrote:
> > On Mon, Sep 28, 2015 at 10:03:12AM -0600, Jan Beulich wrote:
> >> >>> On 21.09.15 at 13:33, <shuai.ruan@linux.intel.com> wrote:
> >> > @@ -954,8 +975,13 @@ long arch_do_domctl(
> >> >                  v->arch.xcr0_accum = _xcr0_accum;
> >> >                  if ( _xcr0_accum & XSTATE_NONLAZY )
> >> >                      v->arch.nonlazy_xstate_used = 1;
> >> > -                memcpy(v->arch.xsave_area, _xsave_area,
> >> > -                       evc->size - 2 * sizeof(uint64_t));
> >> > +                if ( (cpu_has_xsaves || cpu_has_xsavec) &&
> >> > +		     !xsave_area_compressed(_xsave_area) )
> >> 
> >> Is it intended to support compact input here? Where would such
> >> come from? And if so, don't you need to validate the input (e.g.
> >> being a certain size)?
> >> 
> > It is not intended to support compact input here.Just add some check here 
> > (According to Andrew suggestion).
> 
> I think your reply only refers to the cpu_has_xsavec conditional,
> but the question really was about the construct as a whole.
> 
> Jan
> 
There is no such case that input is compact.
"!xsave_area_compressed(_xsave_area)" here just add some check to ensure
_xsave_area is uncompact. It seem that it cause confusion. If so , I will deletesuch check. Is it Ok ?
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 

Re: [V5 1/4] x86/xsaves: add basic definitions/helpers to support xsaves

[Shuai Ruan]

On Mon, Sep 28, 2015 at 03:01:50AM -0600, Jan Beulich wrote:
> >>> On 21.09.15 at 13:33, <shuai.ruan@linux.intel.com> wrote:
> > This patch add basic definitions/helpers which will be used in
> > later patches.
> > 
> > Signed-off-by: Shuai Ruan <shuai.ruan@linux.intel.com>
> > ---
> 
> 
> > +void save_xsave_states(struct vcpu *v, void *dest, unsigned int size)
> 
> Iirc it was suggested before that the function's name isn't adequate:
> There's no saving of anything here. You're expanding already saved
> state.
> 
Is void expand_xsave_states(struct vcpu *v , void *dest, unsigned int size);
Ok ?
> > +
> > +void load_xsave_states(struct vcpu *v, const void *src, unsigned int size)
> 
> Similar comments as above apply to this function.
> 
Is void compress_xsave_states(struct vcpu *v , void *dest, unsigned int size);
Ok ?
> 
> Jan
> 
Thanks 
Shuai

Re: [V5 1/4] x86/xsaves: add basic definitions/helpers to support xsaves

[Jan Beulich]

>>> On 09.10.15 at 10:17, <shuai.ruan@linux.intel.com> wrote:
> On Mon, Sep 28, 2015 at 03:01:50AM -0600, Jan Beulich wrote:
>> >>> On 21.09.15 at 13:33, <shuai.ruan@linux.intel.com> wrote:
>> > +void save_xsave_states(struct vcpu *v, void *dest, unsigned int size)
>> 
>> Iirc it was suggested before that the function's name isn't adequate:
>> There's no saving of anything here. You're expanding already saved
>> state.
>> 
> Is void expand_xsave_states(struct vcpu *v , void *dest, unsigned int size);
> Ok ?
>> > +
>> > +void load_xsave_states(struct vcpu *v, const void *src, unsigned int size)
>> 
>> Similar comments as above apply to this function.
>> 
> Is void compress_xsave_states(struct vcpu *v , void *dest, unsigned int 
> size);
> Ok ?

Afaic - yes to both.

Jan

Re: [V5 2/4] x86/xsaves: enable xsaves/xrstors/xsavec in xen

[Jan Beulich]

>>> On 09.10.15 at 10:14, <shuai.ruan@linux.intel.com> wrote:
> On Fri, Oct 09, 2015 at 01:13:11AM -0600, Jan Beulich wrote:
>> >>> On 09.10.15 at 07:49, <shuai.ruan@linux.intel.com> wrote:
>> > On Mon, Sep 28, 2015 at 10:03:12AM -0600, Jan Beulich wrote:
>> >> >>> On 21.09.15 at 13:33, <shuai.ruan@linux.intel.com> wrote:
>> >> > @@ -954,8 +975,13 @@ long arch_do_domctl(
>> >> >                  v->arch.xcr0_accum = _xcr0_accum;
>> >> >                  if ( _xcr0_accum & XSTATE_NONLAZY )
>> >> >                      v->arch.nonlazy_xstate_used = 1;
>> >> > -                memcpy(v->arch.xsave_area, _xsave_area,
>> >> > -                       evc->size - 2 * sizeof(uint64_t));
>> >> > +                if ( (cpu_has_xsaves || cpu_has_xsavec) &&
>> >> > +		     !xsave_area_compressed(_xsave_area) )
>> >> 
>> >> Is it intended to support compact input here? Where would such
>> >> come from? And if so, don't you need to validate the input (e.g.
>> >> being a certain size)?
>> >> 
>> > It is not intended to support compact input here.Just add some check here 
>> > (According to Andrew suggestion).
>> 
>> I think your reply only refers to the cpu_has_xsavec conditional,
>> but the question really was about the construct as a whole.
>> 
> There is no such case that input is compact.
> "!xsave_area_compressed(_xsave_area)" here just add some check to ensure
> _xsave_area is uncompact.

How is there not? The if() above has an else branch, so you're
taking care of both cases.

> It seem that it cause confusion. If so , I will 
> deletesuch check. Is it Ok ?

That depends on why Andrew had asked for it. If the input can't
be compressed, check it's not and bail otherwise, instead of giving
the impression of handling the case.

Jan