[V8 0/4] add xsaves/xrstors support

[V8 0/4] add xsaves/xrstors support

[Shuai Ruan]

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

Changes in v8:
* Address comments form Jan/Andrew:
* Add optimisation in set_msr_xss in patch1.
* Change from black listing feature to white listing in pv_cpuid in patch2.
* Add MSR_IA32_XSS save/restore support in patch2.
* Fix some code errors and coding style errors.

Changes in v7:
* Address comments form Jan/Andrew:
* Move macro XSTATE_FIXUP into patch3.
* Change lazy write to xss_msr in patch1.
* Drop inner set of brackets and stray cast in patch2.
* Move the condition and memcpy() wouldn't into the new called functions in patch2.
* Rebase patch4 base on Andrew's"tools/libxc: Improve efficiency of xc_cpuid_apply_policy()".
* For no XSS features are currently supported.For leaf 2...63,ecx remain zero at 
  the moment in patch4.

Changes in v6:
* Address comments from Jan.
* Rebase the code based on Andrew'patch "xen/x86: Record xsave features in c->x86_capabilities".
* Re-split the patch to avoid building errors. Move some func definitions from patch1 to patch2.
* Change func name load/save_xsave_states to compress/expand_xsave_states in patch2.  
* Add macro to handle redundancy in xrstor.
* Fix some code errors and coding style errors.
* Add some descriptions in commit message.

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             |  10 +-
 xen/arch/x86/domain.c                  |   7 +
 xen/arch/x86/domctl.c                  |  31 +++-
 xen/arch/x86/hvm/hvm.c                 |  51 ++++++-
 xen/arch/x86/hvm/vmx/vmcs.c            |   5 +-
 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                  | 267 +++++++++++++++++++++++++++++----
 xen/include/asm-x86/hvm/vcpu.h         |   1 +
 xen/include/asm-x86/hvm/vmx/vmcs.h     |   4 +
 xen/include/asm-x86/hvm/vmx/vmx.h      |   2 +
 xen/include/asm-x86/msr-index.h        |   2 +
 xen/include/asm-x86/xstate.h           |  15 +-
 xen/include/public/arch-x86/hvm/save.h |   1 +
 15 files changed, 377 insertions(+), 50 deletions(-)

-- 
1.9.1

[V8 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>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/xstate.c           | 19 +++++++++++++++++++
 xen/include/asm-x86/hvm/vcpu.h  |  1 +
 xen/include/asm-x86/msr-index.h |  2 ++
 xen/include/asm-x86/xstate.h    | 12 ++++++++++--
 4 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index 9ddff90..add8c55 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -24,6 +24,9 @@ static u32 __read_mostly xsave_cntxt_size;
 /* A 64-bit bitmask of the XSAVE/XRSTOR features supported by processor. */
 u64 __read_mostly xfeature_mask;
 
+/* Cached xss for fast read */
+static DEFINE_PER_CPU(uint64_t, xss);
+
 /* Cached xcr0 for fast read */
 static DEFINE_PER_CPU(uint64_t, xcr0);
 
@@ -60,6 +63,22 @@ uint64_t get_xcr0(void)
     return this_cpu(xcr0);
 }
 
+void set_msr_xss(u64 xss)
+{
+    u64 *this_xss = &this_cpu(xss);
+
+    if ( *this_xss != xss )
+    {
+        wrmsrl(MSR_IA32_XSS, xss);
+        *this_xss = xss;
+    }
+}
+
+uint64_t get_msr_xss(void)
+{
+    return this_cpu(xss);
+}
+
 void xsave(struct vcpu *v, uint64_t mask)
 {
     struct xsave_struct *ptr = v->arch.xsave_area;
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 65c1d02..b8ad93c 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 f0d8f0b..b95a5b5 100644
--- a/xen/include/asm-x86/xstate.h
+++ b/xen/include/asm-x86/xstate.h
@@ -19,8 +19,12 @@
 
 #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 XSTATE_AREA_MIN_SIZE      (512 + 64)  /* FP/SSE + XSAVE.HEADER */
+#define FXSAVE_SIZE               512
+#define XSAVE_HDR_OFFSET          FXSAVE_SIZE
+#define XSTATE_AREA_MIN_SIZE      (FXSAVE_SIZE + XSAVE_HDR_SIZE)
 
 #define XSTATE_FP      (1ULL << 0)
 #define XSTATE_SSE     (1ULL << 1)
@@ -38,6 +42,7 @@
 #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;
 
@@ -68,7 +73,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 */
@@ -78,6 +84,8 @@ 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);
-- 
1.9.1

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

[Shuai Ruan]

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

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

Also, pv guest will not support xsaves/xrstors.

Signed-off-by: Shuai Ruan <shuai.ruan@linux.intel.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/domain.c                  |   7 +
 xen/arch/x86/domctl.c                  |  31 ++++-
 xen/arch/x86/hvm/hvm.c                 |  24 +++-
 xen/arch/x86/i387.c                    |   4 +
 xen/arch/x86/traps.c                   |   7 +-
 xen/arch/x86/xstate.c                  | 248 ++++++++++++++++++++++++++++-----
 xen/include/asm-x86/xstate.h           |   2 +
 xen/include/public/arch-x86/hvm/save.h |   1 +
 8 files changed, 279 insertions(+), 45 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index fe3be30..108d4f8 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -883,7 +883,12 @@ int arch_set_info_guest(
     {
         memcpy(v->arch.fpu_ctxt, &c.nat->fpu_ctxt, sizeof(c.nat->fpu_ctxt));
         if ( v->arch.xsave_area )
+        {
              v->arch.xsave_area->xsave_hdr.xstate_bv = XSTATE_FP_SSE;
+             if ( cpu_has_xsaves || cpu_has_xsavec )
+                  v->arch.xsave_area->xsave_hdr.xcomp_bv = XSTATE_FP_SSE |
+                                                           XSTATE_COMPACTION_ENABLED;
+        }
     }
 
     if ( !compat )
@@ -1568,6 +1573,8 @@ static void __context_switch(void)
             if ( xcr0 != get_xcr0() && !set_xcr0(xcr0) )
                 BUG();
         }
+        if ( cpu_has_xsaves && has_hvm_container_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 0f6fdb9..551dde2 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -897,9 +897,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) )
+            {
+                void *xsave_area;
+
+                xsave_area = xmalloc_bytes(size);
+                if ( !xsave_area )
+                {
+                    ret = -ENOMEM;
+                    vcpu_unpause(v);
+                    goto vcpuextstate_out;
+                }
+
+                expand_xsave_states(v, xsave_area,
+                                    size - 2 * sizeof(uint64_t));
+
+                if ( 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);
@@ -955,8 +976,8 @@ 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));
+                compress_xsave_states(v, _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 3fa2280..0140d34 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1735,6 +1735,7 @@ static int hvm_save_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
         hvm_funcs.save_cpu_ctxt(v, &ctxt);
 
         ctxt.msr_tsc_aux = hvm_msr_tsc_aux(v);
+        ctxt.msr_xss = v->arch.hvm_vcpu.msr_xss;
 
         hvm_get_segment_register(v, x86_seg_idtr, &seg);
         ctxt.idtr_limit = seg.limit;
@@ -2025,6 +2026,11 @@ static int hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
 
     v->arch.hvm_vcpu.msr_tsc_aux = ctxt.msr_tsc_aux;
 
+    if ( cpu_has_xsaves )
+        v->arch.hvm_vcpu.msr_xss = ctxt.msr_xss;
+    else
+        v->arch.hvm_vcpu.msr_xss = 0;
+
     seg.limit = ctxt.idtr_limit;
     seg.base = ctxt.idtr_base;
     hvm_set_segment_register(v, x86_seg_idtr, &seg);
@@ -2088,6 +2094,9 @@ static int hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
 
         memcpy(v->arch.xsave_area, ctxt.fpu_regs, sizeof(ctxt.fpu_regs));
         xsave_area->xsave_hdr.xstate_bv = XSTATE_FP_SSE;
+        if ( cpu_has_xsaves || cpu_has_xsavec )
+            xsave_area->xsave_hdr.xcomp_bv = XSTATE_FP_SSE |
+                                             XSTATE_COMPACTION_ENABLED;
     }
     else
         memcpy(v->arch.fpu_ctxt, ctxt.fpu_regs, sizeof(ctxt.fpu_regs));
@@ -2157,8 +2166,8 @@ 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));
+        expand_xsave_states(v, &ctxt->save_area,
+                            size - offsetof(typeof(*ctxt), save_area));
     }
 
     return 0;
@@ -2257,10 +2266,10 @@ 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));
 
+    compress_xsave_states(v, &ctxt->save_area,
+                          min(desc->length, size) -
+                          offsetof(struct hvm_hw_cpu_xsave,save_area));
     return 0;
 }
 
@@ -5409,7 +5418,12 @@ void hvm_vcpu_reset_state(struct vcpu *v, uint16_t cs, uint16_t ip)
     fpu_ctxt->fcw = FCW_RESET;
     fpu_ctxt->mxcsr = MXCSR_DEFAULT;
     if ( v->arch.xsave_area )
+    {
         v->arch.xsave_area->xsave_hdr.xstate_bv = XSTATE_FP;
+        if ( cpu_has_xsaves || cpu_has_xsavec )
+            v->arch.xsave_area->xsave_hdr.xcomp_bv = XSTATE_FP |
+                                                     XSTATE_COMPACTION_ENABLED;
+    }
 
     v->arch.vgc_flags = VGCF_online;
     memset(&v->arch.user_regs, 0, sizeof(v->arch.user_regs));
diff --git a/xen/arch/x86/i387.c b/xen/arch/x86/i387.c
index 66b51cb..b661d39 100644
--- a/xen/arch/x86/i387.c
+++ b/xen/arch/x86/i387.c
@@ -282,7 +282,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 8093535..42449b1 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -935,9 +935,10 @@ void pv_cpuid(struct cpu_user_regs *regs)
             goto unsupported;
         if ( regs->_ecx == 1 )
         {
-            a &= boot_cpu_data.x86_capability[X86_FEATURE_XSAVEOPT / 32];
-            if ( !cpu_has_xsaves )
-                b = c = d = 0;
+            a &= cpufeat_mask(X86_FEATURE_XSAVEOPT) |
+                 cpufeat_mask(X86_FEATURE_XSAVEC) |
+                 (cpu_has_xgetbv1 ? cpufeat_mask(X86_FEATURE_XGETBV1) : 0);
+            b = c = d = 0;
         }
         break;
 
diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index add8c55..ce65c11 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -23,6 +23,10 @@ static u32 __read_mostly xsave_cntxt_size;
 
 /* A 64-bit bitmask of the XSAVE/XRSTOR features supported by processor. */
 u64 __read_mostly xfeature_mask;
+static unsigned int * __read_mostly xstate_offsets;
+static 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 xss for fast read */
 static DEFINE_PER_CPU(uint64_t, xss);
@@ -79,6 +83,164 @@ uint64_t get_msr_xss(void)
     return this_cpu(xss);
 }
 
+static bool_t xsave_area_compressed(const struct xsave_struct *xsave_area)
+{
+     return xsave_area && (xsave_area->xsave_hdr.xcomp_bv
+                           & XSTATE_COMPACTION_ENABLED);
+}
+
+static int setup_xstate_features(bool_t bsp)
+{
+    unsigned int leaf, tmp, eax, ebx;
+
+    if ( bsp )
+    {
+        xstate_features = fls(xfeature_mask);
+        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 __init 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] + xstate_sizes[i] <= xsave_cntxt_size);
+    }
+}
+
+static void *get_xsave_addr(void *xsave, unsigned int xfeature_idx)
+{
+    if ( !((1ul << xfeature_idx) & xfeature_mask) )
+        return NULL;
+
+    return xsave + xstate_comp_offsets[xfeature_idx];
+}
+
+void expand_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;
+
+    if ( !cpu_has_xsaves && !cpu_has_xsavec )
+    {
+        memcpy(dest, xsave, size);
+        return;
+    }
+
+    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);
+
+    ((struct xsave_struct *)dest)->xsave_hdr.xstate_bv = xstate_bv;
+    ((struct xsave_struct *)dest)->xsave_hdr.xcomp_bv =  0;
+
+    /*
+     * 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;
+        unsigned int index = fls(feature) - 1;
+        const 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 compress_xsave_states(struct vcpu *v, const void *src, unsigned int size)
+{
+    struct xsave_struct *xsave = v->arch.xsave_area;
+    u64 xstate_bv = ((const struct xsave_struct *)src)->xsave_hdr.xstate_bv;
+    u64 valid;
+
+    if ( !cpu_has_xsaves && !cpu_has_xsavec )
+    {
+        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.
+     */
+    memcpy(xsave, src, FXSAVE_SIZE);
+
+    /* Set XSTATE_BV and XCOMP_BV.  */
+    xsave->xsave_hdr.xstate_bv = xstate_bv;
+    xsave->xsave_hdr.xcomp_bv = v->arch.xcr0_accum | XSTATE_COMPACTION_ENABLED;
+
+    /*
+     * 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;
+        unsigned int index = fls(feature) - 1;
+        void *dest = get_xsave_addr(xsave, index);
+
+        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;
@@ -91,7 +253,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
@@ -144,7 +314,15 @@ void xsave(struct vcpu *v, uint64_t mask)
     }
     else
     {
-        if ( cpu_has_xsaveopt )
+        if ( cpu_has_xsaves )
+            asm volatile ( ".byte 0x0f,0xc7,0x2f"
+                           : "=m" (*ptr)
+                           : "a" (lmask), "d" (hmask), "D" (ptr) );
+        else if ( cpu_has_xsavec )
+            asm volatile ( ".byte 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) );
@@ -158,6 +336,20 @@ void xsave(struct vcpu *v, uint64_t mask)
         ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET] = word_size;
 }
 
+#define XSTATE_FIXUP ".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"
+
 void xrstor(struct vcpu *v, uint64_t mask)
 {
     uint32_t hmask = mask >> 32;
@@ -187,39 +379,24 @@ 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"
+                           XSTATE_FIXUP );
+        else
+            asm volatile ( "1: .byte 0x48,0x0f,0xae,0x2f\n"
+                           XSTATE_FIXUP );
+            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 0x0f,0xc7,0x1f\n"
+                           XSTATE_FIXUP );
+        else
+            asm volatile ( "1: .byte 0x0f,0xae,0x2f\n"
+                           XSTATE_FIXUP );
         break;
     }
 }
+#undef XSTATE_FIXUP
 
 bool_t xsave_enabled(const struct vcpu *v)
 {
@@ -343,11 +520,18 @@ void xstate_init(struct cpuinfo_x86 *c)
 
     /* Mask out features not currently understood by Xen. */
     eax &= (cpufeat_mask(X86_FEATURE_XSAVEOPT) |
-            cpufeat_mask(X86_FEATURE_XSAVEC));
+            cpufeat_mask(X86_FEATURE_XSAVEC) |
+            cpufeat_mask(X86_FEATURE_XGETBV1) |
+            cpufeat_mask(X86_FEATURE_XSAVES));
 
     c->x86_capability[X86_FEATURE_XSAVEOPT / 32] = eax;
 
     BUG_ON(eax != boot_cpu_data.x86_capability[X86_FEATURE_XSAVEOPT / 32]);
+
+    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 b95a5b5..414cc99 100644
--- a/xen/include/asm-x86/xstate.h
+++ b/xen/include/asm-x86/xstate.h
@@ -91,6 +91,8 @@ 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 expand_xsave_states(struct vcpu *v, void *dest, unsigned int size);
+void compress_xsave_states(struct vcpu *v, const void *src, unsigned int size);
 
 /* extended state init and cleanup functions */
 void xstate_free_save_area(struct vcpu *v);
diff --git a/xen/include/public/arch-x86/hvm/save.h b/xen/include/public/arch-x86/hvm/save.h
index efb0b62..baff602 100644
--- a/xen/include/public/arch-x86/hvm/save.h
+++ b/xen/include/public/arch-x86/hvm/save.h
@@ -140,6 +140,7 @@ struct hvm_hw_cpu {
     uint64_t msr_syscall_mask;
     uint64_t msr_efer;
     uint64_t msr_tsc_aux;
+    uint64_t msr_xss;
 
     /* guest's idea of what rdtsc() would return */
     uint64_t tsc;
-- 
1.9.1

[V8 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.

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

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 0140d34..5f1d993 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4561,6 +4561,20 @@ void hvm_cpuid(unsigned int input, unsigned int *eax, unsigned int *ebx,
                     *ebx = _eax + _ebx;
             }
         }
+        if ( count == 1 )
+        {
+            if ( cpu_has_xsaves && cpu_has_vmx_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) )
+                            *ebx += xstate_sizes[sub_leaf];
+            }
+            else
+                *ebx = *ecx = *edx = 0;
+        }
         break;
 
     case 0x80000001:
@@ -4660,6 +4674,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;
@@ -4792,6 +4812,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 3592a88..7185d55 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -240,7 +240,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;
@@ -1249,6 +1250,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);
 
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index bbec0e8..5d723e8 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2852,6 +2852,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;
@@ -3423,6 +3435,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 ce65c11..d2d2993 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -23,8 +23,8 @@ static u32 __read_mostly xsave_cntxt_size;
 
 /* A 64-bit bitmask of the XSAVE/XRSTOR features supported by processor. */
 u64 __read_mostly xfeature_mask;
-static unsigned int * __read_mostly xstate_offsets;
-static unsigned int * __read_mostly xstate_sizes;
+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];
 
diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h b/xen/include/asm-x86/hvm/vmx/vmcs.h
index f1126d4..79c2c58 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
@@ -291,6 +292,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 +368,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 2ed62f9..cb66925 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 414cc99..3de88bd 100644
--- a/xen/include/asm-x86/xstate.h
+++ b/xen/include/asm-x86/xstate.h
@@ -45,6 +45,7 @@
 #define XSTATE_COMPACTION_ENABLED  (1ULL << 63)
 
 extern u64 xfeature_mask;
+extern unsigned int *xstate_offsets, *xstate_sizes;
 
 /* extended state save area */
 struct __packed __attribute__((aligned (64))) xsave_struct
-- 
1.9.1

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

[Shuai Ruan]

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

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.

But as no XSS festures are currently supported, even in HVM guests,
for leaf 2...63, ecx should be zero at the moment.

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

diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
index 031c848..8882c01 100644
--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
@@ -282,6 +282,9 @@ static void intel_xc_cpuid_policy(xc_interface *xch,
 }
 
 #define XSAVEOPT        (1 << 0)
+#define XSAVEC          (1 << 1)
+#define XGETBV1         (1 << 2)
+#define XSAVES          (1 << 3)
 /* Configure extended state enumeration leaves (0x0000000D for xsave) */
 static void xc_cpuid_config_xsave(xc_interface *xch,
                                   const struct cpuid_domain_info *info,
@@ -318,8 +321,11 @@ static void xc_cpuid_config_xsave(xc_interface *xch,
         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);
+        if ( !info->hvm )
+            regs[0] &= ~XSAVES;
+        regs[2] &= info->xfeature_mask;
+        regs[3] = 0;
         break;
     case 2 ... 63: /* sub-leaves */
         if ( !(info->xfeature_mask & (1ULL << input[1])) )
-- 
1.9.1

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

[Jan Beulich]

>>> On 23.10.15 at 11:48, <shuai.ruan@linux.intel.com> wrote:
> This patch uses xsaves/xrstors/xsavec instead of xsaveopt/xrstor
> to perform the xsave_area switching so that xen itself
> can benefit from them when available.
> 
> For xsaves/xrstors/xsavec only use compact format. Add format conversion
> support when perform guest os migration.
> 
> Also, pv guest will not support xsaves/xrstors.
> 
> Signed-off-by: Shuai Ruan <shuai.ruan@linux.intel.com>
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

There were actual bugs fixed from v7 to v8, plus there's a public
interface change, so retaining this tag is wrong.

> @@ -2025,6 +2026,11 @@ static int hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
>  
>      v->arch.hvm_vcpu.msr_tsc_aux = ctxt.msr_tsc_aux;
>  
> +    if ( cpu_has_xsaves )
> +        v->arch.hvm_vcpu.msr_xss = ctxt.msr_xss;
> +    else
> +        v->arch.hvm_vcpu.msr_xss = 0;

Cases like this call for use of ?:.

> @@ -2257,10 +2266,10 @@ 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));
>  
> +    compress_xsave_states(v, &ctxt->save_area,
> +                          min(desc->length, size) -
> +                          offsetof(struct hvm_hw_cpu_xsave,save_area));
>      return 0;

This change now misplaces the blank line. Please - a little more care
when comments on formatting were already given.

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

Why the cpu_has_xgetbv1 dependency? If you want to do it this
way, it will get unreadable with two or three more features. Why
don't you simply and with the known mask on top of the and with
the capability flags that was already there?

And actually I realize I may have misguided you: xstate_init()
already makes sure boot_cpu_data.x86_capability[] doesn't
contain any unsupported flags, so keeping just the masking
that's already there should be enough.

> +void expand_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;
> +
> +    if ( !cpu_has_xsaves && !cpu_has_xsavec )
> +    {
> +        memcpy(dest, xsave, size);
> +        return;
> +    }
> +
> +    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);
> +
> +    ((struct xsave_struct *)dest)->xsave_hdr.xstate_bv = xstate_bv;
> +    ((struct xsave_struct *)dest)->xsave_hdr.xcomp_bv =  0;

Wouldn't it be more logical to also memcpy() the header? Which
would do away with at least one of these ugly casts, would
allow folding with the memcpy() done right before, _and_ would
eliminate an (apparent or real I'm not sure without looking in
more detail) information leak (the remainder of the header).

> +    /*
> +     * 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;
> +        unsigned int index = fls(feature) - 1;
> +        const 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;
> +    }
> +
> +}

Stray blank line.

> @@ -187,39 +379,24 @@ 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"
> +                           XSTATE_FIXUP );
> +        else
> +            asm volatile ( "1: .byte 0x48,0x0f,0xae,0x2f\n"
> +                           XSTATE_FIXUP );
> +            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 0x0f,0xc7,0x1f\n"
> +                           XSTATE_FIXUP );
> +        else
> +            asm volatile ( "1: .byte 0x0f,0xae,0x2f\n"
> +                           XSTATE_FIXUP );

I wonder whether at least for the restore side alternative asm
wouldn't result in better readable code and at the same time in
a smaller patch.

> @@ -343,11 +520,18 @@ void xstate_init(struct cpuinfo_x86 *c)
>  
>      /* Mask out features not currently understood by Xen. */
>      eax &= (cpufeat_mask(X86_FEATURE_XSAVEOPT) |
> -            cpufeat_mask(X86_FEATURE_XSAVEC));
> +            cpufeat_mask(X86_FEATURE_XSAVEC) |
> +            cpufeat_mask(X86_FEATURE_XGETBV1) |
> +            cpufeat_mask(X86_FEATURE_XSAVES));
>  
>      c->x86_capability[X86_FEATURE_XSAVEOPT / 32] = eax;
>  
>      BUG_ON(eax != boot_cpu_data.x86_capability[X86_FEATURE_XSAVEOPT / 32]);
> +
> +    if ( setup_xstate_features(bsp) )
> +        BUG();

BUG()? On the BSP maybe, but APs should simply fail being
brought up instead of bringing down the whole system.

> --- a/xen/include/public/arch-x86/hvm/save.h
> +++ b/xen/include/public/arch-x86/hvm/save.h
> @@ -140,6 +140,7 @@ struct hvm_hw_cpu {
>      uint64_t msr_syscall_mask;
>      uint64_t msr_efer;
>      uint64_t msr_tsc_aux;
> +    uint64_t msr_xss;

You can't extend a public interface structure like this. New MSRs
shouldn't be saved/restored this way anyway - that's what we
have struct hvm_msr for.

Jan

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

[Jan Beulich]

>>> On 23.10.15 at 11:48, <shuai.ruan@linux.intel.com> wrote:
> --- a/xen/arch/x86/xstate.c
> +++ b/xen/arch/x86/xstate.c
> @@ -23,8 +23,8 @@ static u32 __read_mostly xsave_cntxt_size;
>  
>  /* A 64-bit bitmask of the XSAVE/XRSTOR features supported by processor. */
>  u64 __read_mostly xfeature_mask;
> -static unsigned int * __read_mostly xstate_offsets;
> -static unsigned int * __read_mostly xstate_sizes;
> +unsigned int * __read_mostly xstate_offsets;
> +unsigned int * __read_mostly xstate_sizes;

I said before that without xstate_offsets being used anywhere outside
this file it shouldn't be made non-static.

Also please note that the saving/restoring of XSS really belongs
here instead of in patch 2.

Jan

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

[Shuai Ruan]

On Mon, Oct 26, 2015 at 08:03:55AM -0600, Jan Beulich wrote:
> >>> On 23.10.15 at 11:48, <shuai.ruan@linux.intel.com> wrote:
> > Signed-off-by: Shuai Ruan <shuai.ruan@linux.intel.com>
> > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> There were actual bugs fixed from v7 to v8, plus there's a public
> interface change, so retaining this tag is wrong.
> 
Ok
> > -            a &= boot_cpu_data.x86_capability[X86_FEATURE_XSAVEOPT / 32];
> > -            if ( !cpu_has_xsaves )
> > -                b = c = d = 0;
> > +            a &= cpufeat_mask(X86_FEATURE_XSAVEOPT) |
> > +                 cpufeat_mask(X86_FEATURE_XSAVEC) |
> > +                 (cpu_has_xgetbv1 ? cpufeat_mask(X86_FEATURE_XGETBV1) : 0);
> 
> Why the cpu_has_xgetbv1 dependency? If you want to do it this
> way, it will get unreadable with two or three more features. Why
> don't you simply and with the known mask on top of the and with
> the capability flags that was already there?
> 
> And actually I realize I may have misguided you: xstate_init()
> already makes sure boot_cpu_data.x86_capability[] doesn't
> contain any unsupported flags, so keeping just the masking
> that's already there should be enough.
> 
In this patch in xstate_init I have add xsaves understood by xen. This
boot_cpu_data.x86_capability[] contain support for xsaves. And in my
previous patch V7 I use 
"a &= (boot_cpu_data.x86_capability[X86_FEATURE_XSAVEOPT / 32] & ~cpufeat_mask(X86_FEATURE_XSAVES))"
to mask out xsaves for pv guest. You think this should use white listing
way. So will the way I used in V7  ok ?

> > +void expand_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;
> > +
> > +    if ( !cpu_has_xsaves && !cpu_has_xsavec )
> > +    {
> > +        memcpy(dest, xsave, size);
> > +        return;
> > +    }
> > +
> > +    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);
> > +
> > +    ((struct xsave_struct *)dest)->xsave_hdr.xstate_bv = xstate_bv;
> > +    ((struct xsave_struct *)dest)->xsave_hdr.xcomp_bv =  0;
> 
> Wouldn't it be more logical to also memcpy() the header? Which
> would do away with at least one of these ugly casts, would
> allow folding with the memcpy() done right before, _and_ would
> eliminate an (apparent or real I'm not sure without looking in
> more detail) information leak (the remainder of the header).
> 
For machine with no-xsaves support, xcomp_bv should be zero or it will cause
GP fault. So we can not just memcpy the header when perform save.

Thanks 
Shuai
> 

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

[Jan Beulich]

>>> On 27.10.15 at 02:06, <shuai.ruan@linux.intel.com> wrote:
> On Mon, Oct 26, 2015 at 08:03:55AM -0600, Jan Beulich wrote:
>> >>> On 23.10.15 at 11:48, <shuai.ruan@linux.intel.com> wrote:
>> > -            a &= boot_cpu_data.x86_capability[X86_FEATURE_XSAVEOPT / 32];
>> > -            if ( !cpu_has_xsaves )
>> > -                b = c = d = 0;
>> > +            a &= cpufeat_mask(X86_FEATURE_XSAVEOPT) |
>> > +                 cpufeat_mask(X86_FEATURE_XSAVEC) |
>> > +                 (cpu_has_xgetbv1 ? cpufeat_mask(X86_FEATURE_XGETBV1) : 0);
>> 
>> Why the cpu_has_xgetbv1 dependency? If you want to do it this
>> way, it will get unreadable with two or three more features. Why
>> don't you simply and with the known mask on top of the and with
>> the capability flags that was already there?
>> 
>> And actually I realize I may have misguided you: xstate_init()
>> already makes sure boot_cpu_data.x86_capability[] doesn't
>> contain any unsupported flags, so keeping just the masking
>> that's already there should be enough.
>> 
> In this patch in xstate_init I have add xsaves understood by xen. This
> boot_cpu_data.x86_capability[] contain support for xsaves. And in my
> previous patch V7 I use 
> "a &= (boot_cpu_data.x86_capability[X86_FEATURE_XSAVEOPT / 32] & 
> ~cpufeat_mask(X86_FEATURE_XSAVES))"
> to mask out xsaves for pv guest. You think this should use white listing
> way.

Oh, right, you mean to hide more features than those Xen supports.

> So will the way I used in V7  ok ?

Conceptionally yes, but the first paragraph of my reply still applies,
i.e. the use of cpu_has_xgetbv1 should go away.

>> > +void expand_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;
>> > +
>> > +    if ( !cpu_has_xsaves && !cpu_has_xsavec )
>> > +    {
>> > +        memcpy(dest, xsave, size);
>> > +        return;
>> > +    }
>> > +
>> > +    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);
>> > +
>> > +    ((struct xsave_struct *)dest)->xsave_hdr.xstate_bv = xstate_bv;
>> > +    ((struct xsave_struct *)dest)->xsave_hdr.xcomp_bv =  0;
>> 
>> Wouldn't it be more logical to also memcpy() the header? Which
>> would do away with at least one of these ugly casts, would
>> allow folding with the memcpy() done right before, _and_ would
>> eliminate an (apparent or real I'm not sure without looking in
>> more detail) information leak (the remainder of the header).
>> 
> For machine with no-xsaves support, xcomp_bv should be zero or it will cause
> GP fault. So we can not just memcpy the header when perform save.

See how I limited my reply by saying "at least one", which I added
after realizing that fact? IOW I'd expect you to use memcpy() and
then only zero that one field. Or you'd memset() the whole header
to zero, and then only store xstate_bv. As said, otherwise it looks
like you're adding an information leak.

Jan

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

[Shuai Ruan]

On Tue, Oct 27, 2015 at 02:13:03AM -0600, Jan Beulich wrote:
> >> > +
> >> > +    ((struct xsave_struct *)dest)->xsave_hdr.xstate_bv = xstate_bv;
> >> > +    ((struct xsave_struct *)dest)->xsave_hdr.xcomp_bv =  0;
> >> 
> >> Wouldn't it be more logical to also memcpy() the header? Which
> >> would do away with at least one of these ugly casts, would
> >> allow folding with the memcpy() done right before, _and_ would
> >> eliminate an (apparent or real I'm not sure without looking in
> >> more detail) information leak (the remainder of the header).
> >> 
> > For machine with no-xsaves support, xcomp_bv should be zero or it will cause
> > GP fault. So we can not just memcpy the header when perform save.
> 
> See how I limited my reply by saying "at least one", which I added
> after realizing that fact? IOW I'd expect you to use memcpy() and
> then only zero that one field. Or you'd memset() the whole header
> to zero, and then only store xstate_bv. As said, otherwise it looks
> like you're adding an information leak.
> 
> Jan
Ok And sorry for misunderstanding your meaning.
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 

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

[Shuai Ruan]

On Mon, Oct 26, 2015 at 08:03:55AM -0600, Jan Beulich wrote:
> >>> On 23.10.15 at 11:48, <shuai.ruan@linux.intel.com> wrote:
> > This patch uses xsaves/xrstors/xsavec instead of xsaveopt/xrstor
> > to perform the xsave_area switching so that xen itself
> > can benefit from them when available.
> > 
> > For xsaves/xrstors/xsavec only use compact format. Add format conversion
> > support when perform guest os migration.
> > 
> > Also, pv guest will not support xsaves/xrstors.
> > @@ -343,11 +520,18 @@ void xstate_init(struct cpuinfo_x86 *c)
> >  
> >      /* Mask out features not currently understood by Xen. */
> >      eax &= (cpufeat_mask(X86_FEATURE_XSAVEOPT) |
> > -            cpufeat_mask(X86_FEATURE_XSAVEC));
> > +            cpufeat_mask(X86_FEATURE_XSAVEC) |
> > +            cpufeat_mask(X86_FEATURE_XGETBV1) |
> > +            cpufeat_mask(X86_FEATURE_XSAVES));
> >  
> >      c->x86_capability[X86_FEATURE_XSAVEOPT / 32] = eax;
> >  
> >      BUG_ON(eax != boot_cpu_data.x86_capability[X86_FEATURE_XSAVEOPT / 32]);
> > +
> > +    if ( setup_xstate_features(bsp) )
> > +        BUG();
> 
> BUG()? On the BSP maybe, but APs should simply fail being
> brought up instead of bringing down the whole system.
> 
For APs, setup_xsate_features will never return error. Just BSP can
return error as -ENOMEM.

> > --- a/xen/include/public/arch-x86/hvm/save.h
> > +++ b/xen/include/public/arch-x86/hvm/save.h
> > @@ -140,6 +140,7 @@ struct hvm_hw_cpu {
> >      uint64_t msr_syscall_mask;
> >      uint64_t msr_efer;
> >      uint64_t msr_tsc_aux;
> > +    uint64_t msr_xss;
> 
> You can't extend a public interface structure like this. New MSRs
> shouldn't be saved/restored this way anyway - that's what we
> have struct hvm_msr for.
Yes. I will use the exist function "hvm_save_cpu_msrs" to save this msr. I intend to add save msr logic before "ASSERT(ctxt->count <= msr_count_max);" in hvm_save_cpu_msrs. Is that Ok ?

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

[Jan Beulich]

>>> On 29.10.15 at 08:58, <shuai.ruan@linux.intel.com> wrote:
> On Mon, Oct 26, 2015 at 08:03:55AM -0600, Jan Beulich wrote:
>> >>> On 23.10.15 at 11:48, <shuai.ruan@linux.intel.com> wrote:
>> > This patch uses xsaves/xrstors/xsavec instead of xsaveopt/xrstor
>> > to perform the xsave_area switching so that xen itself
>> > can benefit from them when available.
>> > 
>> > For xsaves/xrstors/xsavec only use compact format. Add format conversion
>> > support when perform guest os migration.
>> > 
>> > Also, pv guest will not support xsaves/xrstors.
>> > @@ -343,11 +520,18 @@ void xstate_init(struct cpuinfo_x86 *c)
>> >  
>> >      /* Mask out features not currently understood by Xen. */
>> >      eax &= (cpufeat_mask(X86_FEATURE_XSAVEOPT) |
>> > -            cpufeat_mask(X86_FEATURE_XSAVEC));
>> > +            cpufeat_mask(X86_FEATURE_XSAVEC) |
>> > +            cpufeat_mask(X86_FEATURE_XGETBV1) |
>> > +            cpufeat_mask(X86_FEATURE_XSAVES));
>> >  
>> >      c->x86_capability[X86_FEATURE_XSAVEOPT / 32] = eax;
>> >  
>> >      BUG_ON(eax != boot_cpu_data.x86_capability[X86_FEATURE_XSAVEOPT / 32]);
>> > +
>> > +    if ( setup_xstate_features(bsp) )
>> > +        BUG();
>> 
>> BUG()? On the BSP maybe, but APs should simply fail being
>> brought up instead of bringing down the whole system.
>> 
> For APs, setup_xsate_features will never return error. Just BSP can
> return error as -ENOMEM.

This may be the case now, but will whoever ends up editing the
function remember to audit the code here?

>> > --- a/xen/include/public/arch-x86/hvm/save.h
>> > +++ b/xen/include/public/arch-x86/hvm/save.h
>> > @@ -140,6 +140,7 @@ struct hvm_hw_cpu {
>> >      uint64_t msr_syscall_mask;
>> >      uint64_t msr_efer;
>> >      uint64_t msr_tsc_aux;
>> > +    uint64_t msr_xss;
>> 
>> You can't extend a public interface structure like this. New MSRs
>> shouldn't be saved/restored this way anyway - that's what we
>> have struct hvm_msr for.
> Yes. I will use the exist function "hvm_save_cpu_msrs" to save this msr. I 
> intend to add save msr logic before "ASSERT(ctxt->count <= msr_count_max);" in 
> hvm_save_cpu_msrs. Is that Ok ?

No, the code belongs in vmx_save_msr() (and its sibling functions).

Jan

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

[Shuai Ruan]

On Thu, Oct 29, 2015 at 02:59:38AM -0600, Jan Beulich wrote:
> >>> On 29.10.15 at 08:58, <shuai.ruan@linux.intel.com> wrote:
> > On Mon, Oct 26, 2015 at 08:03:55AM -0600, Jan Beulich wrote:
> >> >>> On 23.10.15 at 11:48, <shuai.ruan@linux.intel.com> wrote:
> >> > This patch uses xsaves/xrstors/xsavec instead of xsaveopt/xrstor
> >> > to perform the xsave_area switching so that xen itself
> >> > can benefit from them when available.
> >> > 
> >> > For xsaves/xrstors/xsavec only use compact format. Add format conversion
> >> > support when perform guest os migration.
> >> > 
> >> > Also, pv guest will not support xsaves/xrstors.
> >> > @@ -343,11 +520,18 @@ void xstate_init(struct cpuinfo_x86 *c)
> >> >  
> >> >      /* Mask out features not currently understood by Xen. */
> >> >      eax &= (cpufeat_mask(X86_FEATURE_XSAVEOPT) |
> >> > -            cpufeat_mask(X86_FEATURE_XSAVEC));
> >> > +            cpufeat_mask(X86_FEATURE_XSAVEC) |
> >> > +            cpufeat_mask(X86_FEATURE_XGETBV1) |
> >> > +            cpufeat_mask(X86_FEATURE_XSAVES));
> >> >  
> >> >      c->x86_capability[X86_FEATURE_XSAVEOPT / 32] = eax;
> >> >  
> >> >      BUG_ON(eax != boot_cpu_data.x86_capability[X86_FEATURE_XSAVEOPT / 32]);
> >> > +
> >> > +    if ( setup_xstate_features(bsp) )
> >> > +        BUG();
> >> 
> >> BUG()? On the BSP maybe, but APs should simply fail being
> >> brought up instead of bringing down the whole system.
> >> 
> > For APs, setup_xsate_features will never return error. Just BSP can
> > return error as -ENOMEM.
> 
> This may be the case now, but will whoever ends up editing the
> function remember to audit the code here?
> 
Ok.
> >> > --- a/xen/include/public/arch-x86/hvm/save.h
> >> > +++ b/xen/include/public/arch-x86/hvm/save.h
> >> > @@ -140,6 +140,7 @@ struct hvm_hw_cpu {
> >> >      uint64_t msr_syscall_mask;
> >> >      uint64_t msr_efer;
> >> >      uint64_t msr_tsc_aux;
> >> > +    uint64_t msr_xss;
> >> 
> >> You can't extend a public interface structure like this. New MSRs
> >> shouldn't be saved/restored this way anyway - that's what we
> >> have struct hvm_msr for.
> > Yes. I will use the exist function "hvm_save_cpu_msrs" to save this msr. I 
> > intend to add save msr logic before "ASSERT(ctxt->count <= msr_count_max);" in 
> > hvm_save_cpu_msrs. Is that Ok ?
> 
> No, the code belongs in vmx_save_msr() (and its sibling functions).
> 
Ok.
For there is no new area added in vmcs for xss_msr, I will use
"
    if ( cpu_has_xsaves)
    {
        ctxt->msr[ctxt->count].val = v->arch.hvm_vcpu.msr_xss;
        if ( ctxt->msr[ctxt->count].val )
             ctxt->msr[ctxt->count++].index = MSR_IA32_XSS;
    }
" to save xss_msr. Is it ok to add the save logic between "vmx_vmcs_enter(v);" and "vmx_vmcs_exit(v);" ? Or just add the save logic after "vmx_vmcs_exit(v);" ?

Thanks 


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

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

[Jan Beulich]

>>> On 29.10.15 at 10:47, <shuai.ruan@linux.intel.com> wrote:
> On Thu, Oct 29, 2015 at 02:59:38AM -0600, Jan Beulich wrote:
>> >>> On 29.10.15 at 08:58, <shuai.ruan@linux.intel.com> wrote:
>> > Yes. I will use the exist function "hvm_save_cpu_msrs" to save this msr. I 
>> > intend to add save msr logic before "ASSERT(ctxt->count <= msr_count_max);" in 
> 
>> > hvm_save_cpu_msrs. Is that Ok ?
>> 
>> No, the code belongs in vmx_save_msr() (and its sibling functions).
>> 
> Ok.
> For there is no new area added in vmcs for xss_msr, I will use
> "
>     if ( cpu_has_xsaves)
>     {
>         ctxt->msr[ctxt->count].val = v->arch.hvm_vcpu.msr_xss;
>         if ( ctxt->msr[ctxt->count].val )
>              ctxt->msr[ctxt->count++].index = MSR_IA32_XSS;
>     }
> " to save xss_msr. Is it ok to add the save logic between 
> "vmx_vmcs_enter(v);" and "vmx_vmcs_exit(v);" ? Or just add the save logic 
> after "vmx_vmcs_exit(v);" ?

I think it'd be fine either way, but obviously the VMX maintainers
will have the final say.

Jan

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

[Andrew Cooper]

On 29/10/15 09:47, Shuai Ruan wrote:
> On Thu, Oct 29, 2015 at 02:59:38AM -0600, Jan Beulich wrote:
>>>>> On 29.10.15 at 08:58, <shuai.ruan@linux.intel.com> wrote:
>>> On Mon, Oct 26, 2015 at 08:03:55AM -0600, Jan Beulich wrote:
>>>>>>> On 23.10.15 at 11:48, <shuai.ruan@linux.intel.com> wrote:
>>>>> This patch uses xsaves/xrstors/xsavec instead of xsaveopt/xrstor
>>>>> to perform the xsave_area switching so that xen itself
>>>>> can benefit from them when available.
>>>>>
>>>>> For xsaves/xrstors/xsavec only use compact format. Add format conversion
>>>>> support when perform guest os migration.
>>>>>
>>>>> Also, pv guest will not support xsaves/xrstors.
>>>>> @@ -343,11 +520,18 @@ void xstate_init(struct cpuinfo_x86 *c)
>>>>>  
>>>>>      /* Mask out features not currently understood by Xen. */
>>>>>      eax &= (cpufeat_mask(X86_FEATURE_XSAVEOPT) |
>>>>> -            cpufeat_mask(X86_FEATURE_XSAVEC));
>>>>> +            cpufeat_mask(X86_FEATURE_XSAVEC) |
>>>>> +            cpufeat_mask(X86_FEATURE_XGETBV1) |
>>>>> +            cpufeat_mask(X86_FEATURE_XSAVES));
>>>>>  
>>>>>      c->x86_capability[X86_FEATURE_XSAVEOPT / 32] = eax;
>>>>>  
>>>>>      BUG_ON(eax != boot_cpu_data.x86_capability[X86_FEATURE_XSAVEOPT / 32]);
>>>>> +
>>>>> +    if ( setup_xstate_features(bsp) )
>>>>> +        BUG();
>>>> BUG()? On the BSP maybe, but APs should simply fail being
>>>> brought up instead of bringing down the whole system.
>>>>
>>> For APs, setup_xsate_features will never return error. Just BSP can
>>> return error as -ENOMEM.
>> This may be the case now, but will whoever ends up editing the
>> function remember to audit the code here?
>>
> Ok.
>>>>> --- a/xen/include/public/arch-x86/hvm/save.h
>>>>> +++ b/xen/include/public/arch-x86/hvm/save.h
>>>>> @@ -140,6 +140,7 @@ struct hvm_hw_cpu {
>>>>>      uint64_t msr_syscall_mask;
>>>>>      uint64_t msr_efer;
>>>>>      uint64_t msr_tsc_aux;
>>>>> +    uint64_t msr_xss;
>>>> You can't extend a public interface structure like this. New MSRs
>>>> shouldn't be saved/restored this way anyway - that's what we
>>>> have struct hvm_msr for.
>>> Yes. I will use the exist function "hvm_save_cpu_msrs" to save this msr. I 
>>> intend to add save msr logic before "ASSERT(ctxt->count <= msr_count_max);" in 
>>> hvm_save_cpu_msrs. Is that Ok ?
>> No, the code belongs in vmx_save_msr() (and its sibling functions).
>>
> Ok.
> For there is no new area added in vmcs for xss_msr, I will use
> "
>     if ( cpu_has_xsaves)
>     {
>         ctxt->msr[ctxt->count].val = v->arch.hvm_vcpu.msr_xss;
>         if ( ctxt->msr[ctxt->count].val )
>              ctxt->msr[ctxt->count++].index = MSR_IA32_XSS;
>     }
> " to save xss_msr. Is it ok to add the save logic between "vmx_vmcs_enter(v);" and "vmx_vmcs_exit(v);" ? Or just add the save logic after "vmx_vmcs_exit(v);" ?

That looks ok (after fixing the whitespace issue in the if)

As it doesn't rely on the vmcs, it would be better to be outside the
enter/exit pair so as to prevent needless holdup of another cpu trying
to get at this vmcs.

You also need to modify vmx_init_msr() to indicate that the maximum
possible count of MSRs has increased.

~Andrew