[Patch 2/4] Refining Xsave/Xrestore support

[Patch 2/4] Refining Xsave/Xrestore support

[Haitao Shan]

[-- Attachment #1: Type: text/plain, Size: 77 bytes --]

Hi, Keir,

This is patch #2, which adds PV guest Xsave support.

Shan Haitao

[-- Attachment #2: pv-xsave.patch --]
[-- Type: application/octet-stream, Size: 11552 bytes --]

Adding Xsave/Xrestore support for PV guests.

Signed-off-by:	Shan Haitao <haitao.shan@intel.com>
		Han Weidong <weidong.han@intel.com>

diff -r 9374460cf0ca tools/libxc/xc_cpuid_x86.c
--- a/tools/libxc/xc_cpuid_x86.c	Wed Oct 27 20:49:22 2010 +0800
+++ b/tools/libxc/xc_cpuid_x86.c	Wed Oct 27 21:49:54 2010 +0800
@@ -323,7 +323,6 @@ static void xc_cpuid_pv_policy(
         clear_bit(X86_FEATURE_XTPR, regs[2]);
         clear_bit(X86_FEATURE_PDCM, regs[2]);
         clear_bit(X86_FEATURE_DCA, regs[2]);
-        clear_bit(X86_FEATURE_XSAVE, regs[2]);
         set_bit(X86_FEATURE_HYPERVISOR, regs[2]);
         break;
     case 0x80000001:
diff -r 9374460cf0ca xen/arch/x86/domain.c
--- a/xen/arch/x86/domain.c	Wed Oct 27 20:49:22 2010 +0800
+++ b/xen/arch/x86/domain.c	Wed Oct 27 21:49:54 2010 +0800
@@ -343,6 +343,19 @@ int vcpu_initialise(struct vcpu *v)
 
     paging_vcpu_init(v);
 
+    if ( cpu_has_xsave )
+    {
+        /* XSAVE/XRSTOR requires the save area be 64-byte-boundary aligned. */
+        void *xsave_area = _xmalloc(xsave_cntxt_size, 64);
+        if ( xsave_area == NULL )
+            return -ENOMEM;
+
+        xsave_init_save_area(xsave_area);
+        v->arch.xsave_area = xsave_area;
+        v->arch.xcr0 = XSTATE_FP_SSE;
+        v->arch.xcr0_accum = XSTATE_FP_SSE;
+    }
+
     if ( is_hvm_domain(d) )
     {
         if ( (rc = hvm_vcpu_initialise(v)) != 0 )
@@ -384,6 +397,8 @@ void vcpu_destroy(struct vcpu *v)
     if ( is_pv_32on64_vcpu(v) )
         release_compat_l4(v);
 
+    xfree(v->arch.xsave_area);
+
     if ( is_hvm_vcpu(v) )
         hvm_vcpu_destroy(v);
 }
@@ -592,6 +607,8 @@ unsigned long pv_guest_cr4_fixup(const s
         hv_cr4_mask &= ~X86_CR4_DE;
     if ( cpu_has_fsgsbase && !is_pv_32bit_domain(v->domain) )
         hv_cr4_mask &= ~X86_CR4_FSGSBASE;
+    if ( cpu_has_xsave )
+        hv_cr4_mask &= ~X86_CR4_OSXSAVE;
 
     if ( (guest_cr4 & hv_cr4_mask) != (hv_cr4 & hv_cr4_mask) )
         gdprintk(XENLOG_WARNING,
@@ -1367,6 +1384,8 @@ static void __context_switch(void)
         memcpy(stack_regs,
                &n->arch.guest_context.user_regs,
                CTXT_SWITCH_STACK_BYTES);
+        if ( cpu_has_xsave )
+            set_xcr0(n->arch.xcr0);
         n->arch.ctxt_switch_to(n);
     }
 
diff -r 9374460cf0ca xen/arch/x86/hvm/hvm.c
--- a/xen/arch/x86/hvm/hvm.c	Wed Oct 27 20:49:22 2010 +0800
+++ b/xen/arch/x86/hvm/hvm.c	Wed Oct 27 21:49:54 2010 +0800
@@ -805,18 +805,6 @@ int hvm_vcpu_initialise(struct vcpu *v)
 
     hvm_asid_flush_vcpu(v);
 
-    if ( cpu_has_xsave )
-    {
-        /* XSAVE/XRSTOR requires the save area be 64-byte-boundary aligned. */
-        void *xsave_area = _xmalloc(xsave_cntxt_size, 64);
-        if ( xsave_area == NULL )
-            return -ENOMEM;
-
-        xsave_init_save_area(xsave_area);
-        v->arch.hvm_vcpu.xsave_area = xsave_area;
-        v->arch.hvm_vcpu.xcr0 = XSTATE_FP_SSE;
-    }
-
     if ( (rc = vlapic_init(v)) != 0 )
         goto fail1;
 
@@ -879,7 +867,6 @@ void hvm_vcpu_destroy(struct vcpu *v)
     hvm_vcpu_cacheattr_destroy(v);
     vlapic_destroy(v);
     hvm_funcs.vcpu_destroy(v);
-    xfree(v->arch.hvm_vcpu.xsave_area);
 
     /* Event channel is already freed by evtchn_destroy(). */
     /*free_xen_event_channel(v, v->arch.hvm_vcpu.xen_port);*/
diff -r 9374460cf0ca xen/arch/x86/hvm/vmx/vmx.c
--- a/xen/arch/x86/hvm/vmx/vmx.c	Wed Oct 27 20:49:22 2010 +0800
+++ b/xen/arch/x86/hvm/vmx/vmx.c	Wed Oct 27 21:49:54 2010 +0800
@@ -652,10 +652,7 @@ static void vmx_ctxt_switch_to(struct vc
     struct domain *d = v->domain;
     unsigned long old_cr4 = read_cr4(), new_cr4 = mmu_cr4_features;
 
-    /* HOST_CR4 in VMCS is always mmu_cr4_features and
-     * CR4_OSXSAVE(if supported). Sync CR4 now. */
-    if ( cpu_has_xsave )
-        new_cr4 |= X86_CR4_OSXSAVE;
+    /* HOST_CR4 in VMCS is always mmu_cr4_features. Sync CR4 now. */
     if ( old_cr4 != new_cr4 )
         write_cr4(new_cr4);
 
@@ -2215,7 +2212,8 @@ static int vmx_handle_xsetbv(u64 new_bv)
     if ( (xfeature_mask & XSTATE_YMM & new_bv) && !(new_bv & XSTATE_SSE) )
         goto err;
 
-    v->arch.hvm_vcpu.xcr0 = new_bv;
+    v->arch.xcr0 = new_bv;
+    v->arch.xcr0_accum |= new_bv;
     set_xcr0(new_bv);
     return 0;
 err:
diff -r 9374460cf0ca xen/arch/x86/i387.c
--- a/xen/arch/x86/i387.c	Wed Oct 27 20:49:22 2010 +0800
+++ b/xen/arch/x86/i387.c	Wed Oct 27 21:49:54 2010 +0800
@@ -33,9 +33,14 @@ void save_init_fpu(struct vcpu *v)
     if ( cr0 & X86_CR0_TS )
         clts();
 
-    if ( cpu_has_xsave && is_hvm_vcpu(v) )
+    if ( cpu_has_xsave )
     {
+        /* XCR0 normally represents what guest OS set. In case of Xen itself,
+         * we set all accumulated feature mask before doing save/restore.
+         */
+        set_xcr0(v->arch.xcr0_accum);
         xsave(v);
+        set_xcr0(v->arch.xcr0);
     }
     else if ( cpu_has_fxsr )
     {
@@ -171,13 +176,11 @@ void xsave_init(void)
     BUG_ON(ecx < min_size);
 
     /*
-     * We will only enable the features we know for hvm guest. Here we use
-     * set/clear CR4_OSXSAVE and re-run cpuid to get xsave_cntxt_size.
+     * Set CR4_OSXSAVE and run "cpuid" to get xsave_cntxt_size.
      */
     set_in_cr4(X86_CR4_OSXSAVE);
     set_xcr0(eax & XCNTXT_MASK);
     cpuid_count(XSTATE_CPUID, 0, &eax, &ebx, &ecx, &edx);
-    clear_in_cr4(X86_CR4_OSXSAVE);
 
     if ( cpu == 0 )
     {
diff -r 9374460cf0ca xen/arch/x86/traps.c
--- a/xen/arch/x86/traps.c	Wed Oct 27 20:49:22 2010 +0800
+++ b/xen/arch/x86/traps.c	Wed Oct 27 21:49:54 2010 +0800
@@ -795,7 +795,6 @@ static void pv_cpuid(struct cpu_user_reg
         __clear_bit(X86_FEATURE_XTPR % 32, &c);
         __clear_bit(X86_FEATURE_PDCM % 32, &c);
         __clear_bit(X86_FEATURE_DCA % 32, &c);
-        __clear_bit(X86_FEATURE_XSAVE % 32, &c);
         if ( !cpu_has_apic )
            __clear_bit(X86_FEATURE_X2APIC % 32, &c);
         __set_bit(X86_FEATURE_HYPERVISOR % 32, &c);
@@ -2051,13 +2050,34 @@ static int emulate_privileged_op(struct 
         goto fail;
     switch ( opcode )
     {
-    case 0x1: /* RDTSCP */
-        if ( (v->arch.guest_context.ctrlreg[4] & X86_CR4_TSD) &&
-             !guest_kernel_mode(v, regs) )
+    case 0x1: /* RDTSCP and XSETBV */
+        switch ( insn_fetch(u8, code_base, eip, code_limit) )
+        {
+        case 0xf9: /* RDTSCP */
+            if ( (v->arch.guest_context.ctrlreg[4] & X86_CR4_TSD) &&
+                 !guest_kernel_mode(v, regs) )
+                goto fail;
+            pv_soft_rdtsc(v, regs, 1);
+            break;
+        case 0xd1: /* XSETBV */
+        {
+            u64 new_xfeature = regs->eax | ((u64)regs->edx << 32);
+            if ( !guest_kernel_mode(v, regs)
+            /* Currently only XCR0 is implemented */
+                || regs->ecx != XCR_XFEATURE_ENABLED_MASK
+            /* bit 0 of XCR0 must be set */
+                || !(new_xfeature & XSTATE_FP)
+            /* Reserved bit must not be set */
+                || (new_xfeature & ~xfeature_mask) )
+                goto fail;
+            v->arch.xcr0 = new_xfeature;
+            v->arch.xcr0_accum |= new_xfeature;
+            set_xcr0(new_xfeature);
+            break;
+        }
+        default:
             goto fail;
-        if ( insn_fetch(u8, code_base, eip, code_limit) != 0xf9 )
-            goto fail;
-        pv_soft_rdtsc(v, regs, 1);
+        }
         break;
 
     case 0x06: /* CLTS */
diff -r 9374460cf0ca xen/include/asm-x86/domain.h
--- a/xen/include/asm-x86/domain.h	Wed Oct 27 20:49:22 2010 +0800
+++ b/xen/include/asm-x86/domain.h	Wed Oct 27 21:49:54 2010 +0800
@@ -400,6 +400,23 @@ struct arch_vcpu
     pagetable_t monitor_table;          /* (MFN) hypervisor PT (for HVM) */
     unsigned long cr3;                  /* (MA) value to install in HW CR3 */
 
+    /*
+     * The save area for Processor Extended States and the bitmask of the
+     * XSAVE/XRSTOR features. They are used by: 1) when a vcpu (which has
+     * dirtied FPU/SSE) is scheduled out we XSAVE the states here; 2) in
+     * #NM handler, we XRSTOR the states we XSAVE-ed;
+     */
+    void *xsave_area;
+    uint64_t xcr0;
+    /* Accumulated eXtended features mask for using XSAVE/XRESTORE by Xen
+     * itself, as we can never know whether guest OS depends on content
+     * preservation whenever guest OS clears one feature flag (for example,
+     * temporarily).
+     * However, processor should not be able to touch eXtended states before
+     * it explicitly enables it via xcr0.
+     */
+    uint64_t xcr0_accum;
+
     /* Current LDT details. */
     unsigned long shadow_ldt_mapcnt;
     spinlock_t shadow_ldt_lock;
@@ -435,10 +452,11 @@ unsigned long pv_guest_cr4_fixup(const s
 #define pv_guest_cr4_to_real_cr4(v)                         \
     (((v)->arch.guest_context.ctrlreg[4]                    \
       | (mmu_cr4_features & (X86_CR4_PGE | X86_CR4_PSE))    \
-      | ((v)->domain->arch.vtsc ? X86_CR4_TSD : 0))         \
+      | ((v)->domain->arch.vtsc ? X86_CR4_TSD : 0)         \
+      | ((cpu_has_xsave)? X86_CR4_OSXSAVE : 0))              \
       & ~X86_CR4_DE)
 #define real_cr4_to_pv_guest_cr4(c) \
-    ((c) & ~(X86_CR4_PGE | X86_CR4_PSE | X86_CR4_TSD | X86_CR4_OSXSAVE))
+    ((c) & ~(X86_CR4_PGE | X86_CR4_PSE | X86_CR4_TSD))
 
 void domain_cpuid(struct domain *d,
                   unsigned int  input,
diff -r 9374460cf0ca xen/include/asm-x86/hvm/vcpu.h
--- a/xen/include/asm-x86/hvm/vcpu.h	Wed Oct 27 20:49:22 2010 +0800
+++ b/xen/include/asm-x86/hvm/vcpu.h	Wed Oct 27 21:49:54 2010 +0800
@@ -49,15 +49,6 @@ struct hvm_vcpu {
      */
     unsigned long       hw_cr[5];
 
-    /*
-     * The save area for Processor Extended States and the bitmask of the
-     * XSAVE/XRSTOR features. They are used by: 1) when a vcpu (which has
-     * dirtied FPU/SSE) is scheduled out we XSAVE the states here; 2) in
-     * #NM handler, we XRSTOR the states we XSAVE-ed;
-     */
-    void *xsave_area;
-    uint64_t xcr0;
-
     struct vlapic       vlapic;
     s64                 cache_tsc_offset;
     u64                 guest_time;
diff -r 9374460cf0ca xen/include/asm-x86/i387.h
--- a/xen/include/asm-x86/i387.h	Wed Oct 27 20:49:22 2010 +0800
+++ b/xen/include/asm-x86/i387.h	Wed Oct 27 21:49:54 2010 +0800
@@ -67,7 +67,7 @@ static inline void xsave(struct vcpu *v)
 {
     struct xsave_struct *ptr;
 
-    ptr =(struct xsave_struct *)v->arch.hvm_vcpu.xsave_area;
+    ptr =(struct xsave_struct *)v->arch.xsave_area;
 
     asm volatile (".byte " REX_PREFIX "0x0f,0xae,0x27"
         :
@@ -79,7 +79,7 @@ static inline void xrstor(struct vcpu *v
 {
     struct xsave_struct *ptr;
 
-    ptr =(struct xsave_struct *)v->arch.hvm_vcpu.xsave_area;
+    ptr =(struct xsave_struct *)v->arch.xsave_area;
 
     asm volatile (".byte " REX_PREFIX "0x0f,0xae,0x2f"
         :
@@ -108,14 +108,18 @@ static inline void setup_fpu(struct vcpu
     if ( !v->fpu_dirtied )
     {
         v->fpu_dirtied = 1;
-        if ( cpu_has_xsave && is_hvm_vcpu(v) )
+        if ( cpu_has_xsave )
         {
             if ( !v->fpu_initialised )
                 v->fpu_initialised = 1;
 
-            set_xcr0(v->arch.hvm_vcpu.xcr0 | XSTATE_FP_SSE);
+            /* XCR0 normally represents what guest OS set. In case of Xen
+             * itself, we set all supported feature mask before doing
+             * save/restore.
+             */
+            set_xcr0(v->arch.xcr0_accum);
             xrstor(v);
-            set_xcr0(v->arch.hvm_vcpu.xcr0);
+            set_xcr0(v->arch.xcr0);
         }
         else
         {

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

Re: [Patch 2/4] Refining Xsave/Xrestore support

[Jan Beulich]

>@@ -1367,6 +1384,8 @@ static void __context_switch(void)
>         memcpy(stack_regs,
>                &n->arch.guest_context.user_regs,
>                CTXT_SWITCH_STACK_BYTES);
>+        if ( cpu_has_xsave )
>+            set_xcr0(n->arch.xcr0);

How slow is a write to xcr0? I.e. is it worth checking whether
current xcr0 matches the to be written value?

Jan

Re: [Patch 2/4] Refining Xsave/Xrestore support

[Jeremy Fitzhardinge]

 On 10/27/2010 12:04 AM, Haitao Shan wrote:
> Hi, Keir,
>
> This is patch #2, which adds PV guest Xsave support.

How does a PV guest know whether Xsave support is available?  Previous
versions of Xen left the xsave cpu feature flag set even though xsave
wasn't usable by the domain, so I had to forceably mask it from the
cpuid features within the domain.  Given that a PV domain can't rely on
X86_FEATURE_XSAVE, how can it tell that the feature is actually usable?

Thanks,
    J

Re: [Patch 2/4] Refining Xsave/Xrestore support

[Haitao Shan]

I don't know but I suppose this won't be a costing instruction at
lease no larger than writing to other CRx registers. But agreed that a
comparison to minimize the cost would be great. I can add that.

Thanks for pointing out.

Shan Haitao

2010/10/27 Jan Beulich <JBeulich@novell.com>:
>>@@ -1367,6 +1384,8 @@ static void __context_switch(void)
>>         memcpy(stack_regs,
>>                &n->arch.guest_context.user_regs,
>>                CTXT_SWITCH_STACK_BYTES);
>>+        if ( cpu_has_xsave )
>>+            set_xcr0(n->arch.xcr0);
>
> How slow is a write to xcr0? I.e. is it worth checking whether
> current xcr0 matches the to be written value?
>
> Jan
>
>

Re: [Patch 2/4] Refining Xsave/Xrestore support

[Haitao Shan]

Hi, Jeremy,

My approach is based an old PV-OPS kernel source. Kernel tries to set
CR4.OSXSAVE and read it back to determine whether Xsave is actually
available.
Could you still use that?

Shan Haitao

2010/10/28 Jeremy Fitzhardinge <jeremy@goop.org>:
>  On 10/27/2010 12:04 AM, Haitao Shan wrote:
>> Hi, Keir,
>>
>> This is patch #2, which adds PV guest Xsave support.
>
> How does a PV guest know whether Xsave support is available?  Previous
> versions of Xen left the xsave cpu feature flag set even though xsave
> wasn't usable by the domain, so I had to forceably mask it from the
> cpuid features within the domain.  Given that a PV domain can't rely on
> X86_FEATURE_XSAVE, how can it tell that the feature is actually usable?
>
> Thanks,
>    J
>

Re: [Patch 2/4] Refining Xsave/Xrestore support

[Haitao Shan]

[-- Attachment #1: Type: text/plain, Size: 510 bytes --]

Hi, Jan,

This is the updated patch#2. Thanks.

Shan Haitao

2010/10/27 Jan Beulich <JBeulich@novell.com>:
>>@@ -1367,6 +1384,8 @@ static void __context_switch(void)
>>         memcpy(stack_regs,
>>                &n->arch.guest_context.user_regs,
>>                CTXT_SWITCH_STACK_BYTES);
>>+        if ( cpu_has_xsave )
>>+            set_xcr0(n->arch.xcr0);
>
> How slow is a write to xcr0? I.e. is it worth checking whether
> current xcr0 matches the to be written value?
>
> Jan
>
>

[-- Attachment #2: pv-xsave.patch --]
[-- Type: application/octet-stream, Size: 11448 bytes --]

diff -r daf19351f958 tools/libxc/xc_cpuid_x86.c
--- a/tools/libxc/xc_cpuid_x86.c	Thu Oct 28 16:07:51 2010 +0800
+++ b/tools/libxc/xc_cpuid_x86.c	Thu Oct 28 19:21:08 2010 +0800
@@ -323,7 +323,6 @@ static void xc_cpuid_pv_policy(
         clear_bit(X86_FEATURE_XTPR, regs[2]);
         clear_bit(X86_FEATURE_PDCM, regs[2]);
         clear_bit(X86_FEATURE_DCA, regs[2]);
-        clear_bit(X86_FEATURE_XSAVE, regs[2]);
         set_bit(X86_FEATURE_HYPERVISOR, regs[2]);
         break;
     case 0x80000001:
diff -r daf19351f958 xen/arch/x86/domain.c
--- a/xen/arch/x86/domain.c	Thu Oct 28 16:07:51 2010 +0800
+++ b/xen/arch/x86/domain.c	Thu Oct 28 19:21:08 2010 +0800
@@ -343,6 +343,19 @@ int vcpu_initialise(struct vcpu *v)
 
     paging_vcpu_init(v);
 
+    if ( cpu_has_xsave )
+    {
+        /* XSAVE/XRSTOR requires the save area be 64-byte-boundary aligned. */
+        void *xsave_area = _xmalloc(xsave_cntxt_size, 64);
+        if ( xsave_area == NULL )
+            return -ENOMEM;
+
+        xsave_init_save_area(xsave_area);
+        v->arch.xsave_area = xsave_area;
+        v->arch.xcr0 = XSTATE_FP_SSE;
+        v->arch.xcr0_accum = XSTATE_FP_SSE;
+    }
+
     if ( is_hvm_domain(d) )
     {
         if ( (rc = hvm_vcpu_initialise(v)) != 0 )
@@ -384,6 +397,8 @@ void vcpu_destroy(struct vcpu *v)
     if ( is_pv_32on64_vcpu(v) )
         release_compat_l4(v);
 
+    xfree(v->arch.xsave_area);
+
     if ( is_hvm_vcpu(v) )
         hvm_vcpu_destroy(v);
 }
@@ -592,6 +607,8 @@ unsigned long pv_guest_cr4_fixup(const s
         hv_cr4_mask &= ~X86_CR4_DE;
     if ( cpu_has_fsgsbase && !is_pv_32bit_domain(v->domain) )
         hv_cr4_mask &= ~X86_CR4_FSGSBASE;
+    if ( cpu_has_xsave )
+        hv_cr4_mask &= ~X86_CR4_OSXSAVE;
 
     if ( (guest_cr4 & hv_cr4_mask) != (hv_cr4 & hv_cr4_mask) )
         gdprintk(XENLOG_WARNING,
@@ -1367,6 +1384,8 @@ static void __context_switch(void)
         memcpy(stack_regs,
                &n->arch.guest_context.user_regs,
                CTXT_SWITCH_STACK_BYTES);
+        if ( cpu_has_xsave && p->arch.xcr0 != n->arch.xcr0 )
+            set_xcr0(n->arch.xcr0);
         n->arch.ctxt_switch_to(n);
     }
 
diff -r daf19351f958 xen/arch/x86/hvm/hvm.c
--- a/xen/arch/x86/hvm/hvm.c	Thu Oct 28 16:07:51 2010 +0800
+++ b/xen/arch/x86/hvm/hvm.c	Thu Oct 28 19:21:08 2010 +0800
@@ -805,18 +805,6 @@ int hvm_vcpu_initialise(struct vcpu *v)
 
     hvm_asid_flush_vcpu(v);
 
-    if ( cpu_has_xsave )
-    {
-        /* XSAVE/XRSTOR requires the save area be 64-byte-boundary aligned. */
-        void *xsave_area = _xmalloc(xsave_cntxt_size, 64);
-        if ( xsave_area == NULL )
-            return -ENOMEM;
-
-        xsave_init_save_area(xsave_area);
-        v->arch.hvm_vcpu.xsave_area = xsave_area;
-        v->arch.hvm_vcpu.xcr0 = XSTATE_FP_SSE;
-    }
-
     if ( (rc = vlapic_init(v)) != 0 )
         goto fail1;
 
@@ -879,7 +867,6 @@ void hvm_vcpu_destroy(struct vcpu *v)
     hvm_vcpu_cacheattr_destroy(v);
     vlapic_destroy(v);
     hvm_funcs.vcpu_destroy(v);
-    xfree(v->arch.hvm_vcpu.xsave_area);
 
     /* Event channel is already freed by evtchn_destroy(). */
     /*free_xen_event_channel(v, v->arch.hvm_vcpu.xen_port);*/
diff -r daf19351f958 xen/arch/x86/hvm/vmx/vmx.c
--- a/xen/arch/x86/hvm/vmx/vmx.c	Thu Oct 28 16:07:51 2010 +0800
+++ b/xen/arch/x86/hvm/vmx/vmx.c	Thu Oct 28 19:21:08 2010 +0800
@@ -652,10 +652,7 @@ static void vmx_ctxt_switch_to(struct vc
     struct domain *d = v->domain;
     unsigned long old_cr4 = read_cr4(), new_cr4 = mmu_cr4_features;
 
-    /* HOST_CR4 in VMCS is always mmu_cr4_features and
-     * CR4_OSXSAVE(if supported). Sync CR4 now. */
-    if ( cpu_has_xsave )
-        new_cr4 |= X86_CR4_OSXSAVE;
+    /* HOST_CR4 in VMCS is always mmu_cr4_features. Sync CR4 now. */
     if ( old_cr4 != new_cr4 )
         write_cr4(new_cr4);
 
@@ -2215,7 +2212,8 @@ static int vmx_handle_xsetbv(u64 new_bv)
     if ( (xfeature_mask & XSTATE_YMM & new_bv) && !(new_bv & XSTATE_SSE) )
         goto err;
 
-    v->arch.hvm_vcpu.xcr0 = new_bv;
+    v->arch.xcr0 = new_bv;
+    v->arch.xcr0_accum |= new_bv;
     set_xcr0(new_bv);
     return 0;
 err:
diff -r daf19351f958 xen/arch/x86/i387.c
--- a/xen/arch/x86/i387.c	Thu Oct 28 16:07:51 2010 +0800
+++ b/xen/arch/x86/i387.c	Thu Oct 28 19:21:08 2010 +0800
@@ -33,9 +33,14 @@ void save_init_fpu(struct vcpu *v)
     if ( cr0 & X86_CR0_TS )
         clts();
 
-    if ( cpu_has_xsave && is_hvm_vcpu(v) )
+    if ( cpu_has_xsave )
     {
+        /* XCR0 normally represents what guest OS set. In case of Xen itself,
+         * we set all accumulated feature mask before doing save/restore.
+         */
+        set_xcr0(v->arch.xcr0_accum);
         xsave(v);
+        set_xcr0(v->arch.xcr0);
     }
     else if ( cpu_has_fxsr )
     {
@@ -171,13 +176,11 @@ void xsave_init(void)
     BUG_ON(ecx < min_size);
 
     /*
-     * We will only enable the features we know for hvm guest. Here we use
-     * set/clear CR4_OSXSAVE and re-run cpuid to get xsave_cntxt_size.
+     * Set CR4_OSXSAVE and run "cpuid" to get xsave_cntxt_size.
      */
     set_in_cr4(X86_CR4_OSXSAVE);
     set_xcr0(eax & XCNTXT_MASK);
     cpuid_count(XSTATE_CPUID, 0, &eax, &ebx, &ecx, &edx);
-    clear_in_cr4(X86_CR4_OSXSAVE);
 
     if ( cpu == 0 )
     {
diff -r daf19351f958 xen/arch/x86/traps.c
--- a/xen/arch/x86/traps.c	Thu Oct 28 16:07:51 2010 +0800
+++ b/xen/arch/x86/traps.c	Thu Oct 28 19:21:08 2010 +0800
@@ -795,7 +795,6 @@ static void pv_cpuid(struct cpu_user_reg
         __clear_bit(X86_FEATURE_XTPR % 32, &c);
         __clear_bit(X86_FEATURE_PDCM % 32, &c);
         __clear_bit(X86_FEATURE_DCA % 32, &c);
-        __clear_bit(X86_FEATURE_XSAVE % 32, &c);
         if ( !cpu_has_apic )
            __clear_bit(X86_FEATURE_X2APIC % 32, &c);
         __set_bit(X86_FEATURE_HYPERVISOR % 32, &c);
@@ -2051,13 +2050,34 @@ static int emulate_privileged_op(struct 
         goto fail;
     switch ( opcode )
     {
-    case 0x1: /* RDTSCP */
-        if ( (v->arch.guest_context.ctrlreg[4] & X86_CR4_TSD) &&
-             !guest_kernel_mode(v, regs) )
+    case 0x1: /* RDTSCP and XSETBV */
+        switch ( insn_fetch(u8, code_base, eip, code_limit) )
+        {
+        case 0xf9: /* RDTSCP */
+            if ( (v->arch.guest_context.ctrlreg[4] & X86_CR4_TSD) &&
+                 !guest_kernel_mode(v, regs) )
+                goto fail;
+            pv_soft_rdtsc(v, regs, 1);
+            break;
+        case 0xd1: /* XSETBV */
+        {
+            u64 new_xfeature = regs->eax | ((u64)regs->edx << 32);
+            if ( !guest_kernel_mode(v, regs)
+            /* Currently only XCR0 is implemented */
+                || regs->ecx != XCR_XFEATURE_ENABLED_MASK
+            /* bit 0 of XCR0 must be set */
+                || !(new_xfeature & XSTATE_FP)
+            /* Reserved bit must not be set */
+                || (new_xfeature & ~xfeature_mask) )
+                goto fail;
+            v->arch.xcr0 = new_xfeature;
+            v->arch.xcr0_accum |= new_xfeature;
+            set_xcr0(new_xfeature);
+            break;
+        }
+        default:
             goto fail;
-        if ( insn_fetch(u8, code_base, eip, code_limit) != 0xf9 )
-            goto fail;
-        pv_soft_rdtsc(v, regs, 1);
+        }
         break;
 
     case 0x06: /* CLTS */
diff -r daf19351f958 xen/include/asm-x86/domain.h
--- a/xen/include/asm-x86/domain.h	Thu Oct 28 16:07:51 2010 +0800
+++ b/xen/include/asm-x86/domain.h	Thu Oct 28 19:21:08 2010 +0800
@@ -400,6 +400,23 @@ struct arch_vcpu
     pagetable_t monitor_table;          /* (MFN) hypervisor PT (for HVM) */
     unsigned long cr3;                  /* (MA) value to install in HW CR3 */
 
+    /*
+     * The save area for Processor Extended States and the bitmask of the
+     * XSAVE/XRSTOR features. They are used by: 1) when a vcpu (which has
+     * dirtied FPU/SSE) is scheduled out we XSAVE the states here; 2) in
+     * #NM handler, we XRSTOR the states we XSAVE-ed;
+     */
+    void *xsave_area;
+    uint64_t xcr0;
+    /* Accumulated eXtended features mask for using XSAVE/XRESTORE by Xen
+     * itself, as we can never know whether guest OS depends on content
+     * preservation whenever guest OS clears one feature flag (for example,
+     * temporarily).
+     * However, processor should not be able to touch eXtended states before
+     * it explicitly enables it via xcr0.
+     */
+    uint64_t xcr0_accum;
+
     /* Current LDT details. */
     unsigned long shadow_ldt_mapcnt;
     spinlock_t shadow_ldt_lock;
@@ -435,10 +452,11 @@ unsigned long pv_guest_cr4_fixup(const s
 #define pv_guest_cr4_to_real_cr4(v)                         \
     (((v)->arch.guest_context.ctrlreg[4]                    \
       | (mmu_cr4_features & (X86_CR4_PGE | X86_CR4_PSE))    \
-      | ((v)->domain->arch.vtsc ? X86_CR4_TSD : 0))         \
+      | ((v)->domain->arch.vtsc ? X86_CR4_TSD : 0)         \
+      | ((cpu_has_xsave)? X86_CR4_OSXSAVE : 0))              \
       & ~X86_CR4_DE)
 #define real_cr4_to_pv_guest_cr4(c) \
-    ((c) & ~(X86_CR4_PGE | X86_CR4_PSE | X86_CR4_TSD | X86_CR4_OSXSAVE))
+    ((c) & ~(X86_CR4_PGE | X86_CR4_PSE | X86_CR4_TSD))
 
 void domain_cpuid(struct domain *d,
                   unsigned int  input,
diff -r daf19351f958 xen/include/asm-x86/hvm/vcpu.h
--- a/xen/include/asm-x86/hvm/vcpu.h	Thu Oct 28 16:07:51 2010 +0800
+++ b/xen/include/asm-x86/hvm/vcpu.h	Thu Oct 28 19:21:08 2010 +0800
@@ -49,15 +49,6 @@ struct hvm_vcpu {
      */
     unsigned long       hw_cr[5];
 
-    /*
-     * The save area for Processor Extended States and the bitmask of the
-     * XSAVE/XRSTOR features. They are used by: 1) when a vcpu (which has
-     * dirtied FPU/SSE) is scheduled out we XSAVE the states here; 2) in
-     * #NM handler, we XRSTOR the states we XSAVE-ed;
-     */
-    void *xsave_area;
-    uint64_t xcr0;
-
     struct vlapic       vlapic;
     s64                 cache_tsc_offset;
     u64                 guest_time;
diff -r daf19351f958 xen/include/asm-x86/i387.h
--- a/xen/include/asm-x86/i387.h	Thu Oct 28 16:07:51 2010 +0800
+++ b/xen/include/asm-x86/i387.h	Thu Oct 28 19:21:08 2010 +0800
@@ -67,7 +67,7 @@ static inline void xsave(struct vcpu *v)
 {
     struct xsave_struct *ptr;
 
-    ptr =(struct xsave_struct *)v->arch.hvm_vcpu.xsave_area;
+    ptr =(struct xsave_struct *)v->arch.xsave_area;
 
     asm volatile (".byte " REX_PREFIX "0x0f,0xae,0x27"
         :
@@ -79,7 +79,7 @@ static inline void xrstor(struct vcpu *v
 {
     struct xsave_struct *ptr;
 
-    ptr =(struct xsave_struct *)v->arch.hvm_vcpu.xsave_area;
+    ptr =(struct xsave_struct *)v->arch.xsave_area;
 
     asm volatile (".byte " REX_PREFIX "0x0f,0xae,0x2f"
         :
@@ -108,14 +108,18 @@ static inline void setup_fpu(struct vcpu
     if ( !v->fpu_dirtied )
     {
         v->fpu_dirtied = 1;
-        if ( cpu_has_xsave && is_hvm_vcpu(v) )
+        if ( cpu_has_xsave )
         {
             if ( !v->fpu_initialised )
                 v->fpu_initialised = 1;
 
-            set_xcr0(v->arch.hvm_vcpu.xcr0 | XSTATE_FP_SSE);
+            /* XCR0 normally represents what guest OS set. In case of Xen
+             * itself, we set all supported feature mask before doing
+             * save/restore.
+             */
+            set_xcr0(v->arch.xcr0_accum);
             xrstor(v);
-            set_xcr0(v->arch.hvm_vcpu.xcr0);
+            set_xcr0(v->arch.xcr0);
         }
         else
         {

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

Re: [Patch 2/4] Refining Xsave/Xrestore support

[Jan Beulich]

>>> On 28.10.10 at 06:58, Haitao Shan <maillists.shan@gmail.com> wrote:
> This is the updated patch#2. Thanks.

Sorry, but this is worse than not checking at all: You didn't consider
the idle vCPU case here, and hence you may end up having more
features enabled in xcr0 for a guest than it should have.

Also I only now noticed that you're leaking the xsave_area allocation
in vcpu_initialize() if hvm_vcpu_initialise() fails.

Jan

Re: [Patch 2/4] Refining Xsave/Xrestore support

[Jan Beulich]

>>> On 28.10.10 at 04:57, Haitao Shan <maillists.shan@gmail.com> wrote:
> My approach is based an old PV-OPS kernel source. Kernel tries to set
> CR4.OSXSAVE and read it back to determine whether Xsave is actually
> available.

Hmm, in our kernels we check the OSXSAVE feature bit instead,
expecting (just like for FXSAVE) the hypervisor to enable the bit
in CR4 when it supports the feature.

I think a mechanism needs to be found in the hypervisor that
accommodates both current pv-ops and current forward port
kernels' behaviors.

Jan

> 2010/10/28 Jeremy Fitzhardinge <jeremy@goop.org>:
>>  On 10/27/2010 12:04 AM, Haitao Shan wrote:
>>> Hi, Keir,
>>>
>>> This is patch #2, which adds PV guest Xsave support.
>>
>> How does a PV guest know whether Xsave support is available?  Previous
>> versions of Xen left the xsave cpu feature flag set even though xsave
>> wasn't usable by the domain, so I had to forceably mask it from the
>> cpuid features within the domain.  Given that a PV domain can't rely on
>> X86_FEATURE_XSAVE, how can it tell that the feature is actually usable?
>>
>> Thanks,
>>    J
>>
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com 
> http://lists.xensource.com/xen-devel 

Re: [Patch 2/4] Refining Xsave/Xrestore support

[Haitao Shan]

Then I would prefer to write XCR0 unconditionally. Otherwise, I can
only refer to the approach for handling CR4 switches: reading CR4
first and checking whether there is a need to write actually.
But I don't think <a read to XCR0 plus a data comparison> can save any
compared with one unconditional write to XCR0.
Are you OK with this?

Thanks for pointing out the memory leak when hvm_vcpu_initialize
fails. I will update accordingly.

Shan Haitao

2010/10/28 Jan Beulich <JBeulich@novell.com>:
>>>> On 28.10.10 at 06:58, Haitao Shan <maillists.shan@gmail.com> wrote:
>> This is the updated patch#2. Thanks.
>
> Sorry, but this is worse than not checking at all: You didn't consider
> the idle vCPU case here, and hence you may end up having more
> features enabled in xcr0 for a guest than it should have.
>
> Also I only now noticed that you're leaking the xsave_area allocation
> in vcpu_initialize() if hvm_vcpu_initialise() fails.
>
> Jan
>
>

Re: [Patch 2/4] Refining Xsave/Xrestore support

[Haitao Shan]

In my patch, if processor supports CR4.OSXSAVE, Xen will enable it and
won't clear it as long as we are in ROOT mode.

Shan Haitao

2010/10/28 Jan Beulich <JBeulich@novell.com>:
>>>> On 28.10.10 at 04:57, Haitao Shan <maillists.shan@gmail.com> wrote:
>> My approach is based an old PV-OPS kernel source. Kernel tries to set
>> CR4.OSXSAVE and read it back to determine whether Xsave is actually
>> available.
>
> Hmm, in our kernels we check the OSXSAVE feature bit instead,
> expecting (just like for FXSAVE) the hypervisor to enable the bit
> in CR4 when it supports the feature.
>
> I think a mechanism needs to be found in the hypervisor that
> accommodates both current pv-ops and current forward port
> kernels' behaviors.
>
> Jan
>
>> 2010/10/28 Jeremy Fitzhardinge <jeremy@goop.org>:
>>>  On 10/27/2010 12:04 AM, Haitao Shan wrote:
>>>> Hi, Keir,
>>>>
>>>> This is patch #2, which adds PV guest Xsave support.
>>>
>>> How does a PV guest know whether Xsave support is available?  Previous
>>> versions of Xen left the xsave cpu feature flag set even though xsave
>>> wasn't usable by the domain, so I had to forceably mask it from the
>>> cpuid features within the domain.  Given that a PV domain can't rely on
>>> X86_FEATURE_XSAVE, how can it tell that the feature is actually usable?
>>>
>>> Thanks,
>>>    J
>>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xensource.com
>> http://lists.xensource.com/xen-devel
>
>
>
>

Re: [Patch 2/4] Refining Xsave/Xrestore support

[Jan Beulich]

>>> On 28.10.10 at 09:55, Haitao Shan <maillists.shan@gmail.com> wrote:
> In my patch, if processor supports CR4.OSXSAVE, Xen will enable it and
> won't clear it as long as we are in ROOT mode.

Ah, okay. There's one problem with this, however - a pv domain the
kernel of which doesn't support xsave would still see CPUID report
the OSXSAVE bit set, and hence applications could be lead to
believe they can use the wider registers. I realize this also puts
under question our model of having the kernel look at CPUID's
OSXSAVE bit, but that could possibly be dealt with by having
pv_cpuid() (and perhaps xc_cpuid_pv_policy()) set the bit in case
Xen supports xsave.

Jan

Re: [Patch 2/4] Refining Xsave/Xrestore support

[Jan Beulich]

>>> On 28.10.10 at 09:52, Haitao Shan <maillists.shan@gmail.com> wrote:
> Then I would prefer to write XCR0 unconditionally. Otherwise, I can
> only refer to the approach for handling CR4 switches: reading CR4
> first and checking whether there is a need to write actually.
> But I don't think <a read to XCR0 plus a data comparison> can save any
> compared with one unconditional write to XCR0.
> Are you OK with this?

Depends on the performance expectations of xsetbv and xgetbv
(and its comparison to moves from/to control registers). At least
there's no word in the documentation that xsetbv would be
serializing. I would hope Intel could at least provide approximate
numbers...

Jan

Re: [Patch 2/4] Refining Xsave/Xrestore support

[Haitao Shan]

I once considered this problem, too. Originally, I suppose I can use
CR4 switching when entering/leaving PV guest, as what HW is doing for
us in VMX. But I suspect this will bring a lot of overhead.
Later I picked the current implementation, hoping guest applications
can have a check on XCR0 first before it uses extended states. And
according to spec, in order to use extend states, XCR0 must be set,
but this is a ring 0 instruction only.

Shan Haitao

2010/10/28 Jan Beulich <JBeulich@novell.com>:
>>>> On 28.10.10 at 09:55, Haitao Shan <maillists.shan@gmail.com> wrote:
>> In my patch, if processor supports CR4.OSXSAVE, Xen will enable it and
>> won't clear it as long as we are in ROOT mode.
>
> Ah, okay. There's one problem with this, however - a pv domain the
> kernel of which doesn't support xsave would still see CPUID report
> the OSXSAVE bit set, and hence applications could be lead to
> believe they can use the wider registers. I realize this also puts
> under question our model of having the kernel look at CPUID's
> OSXSAVE bit, but that could possibly be dealt with by having
> pv_cpuid() (and perhaps xc_cpuid_pv_policy()) set the bit in case
> Xen supports xsave.
>
> Jan
>
>

Re: [Patch 2/4] Refining Xsave/Xrestore support

[Jan Beulich]

>>> On 28.10.10 at 10:33, Haitao Shan <maillists.shan@gmail.com> wrote:
> I once considered this problem, too. Originally, I suppose I can use
> CR4 switching when entering/leaving PV guest, as what HW is doing for
> us in VMX. But I suspect this will bring a lot of overhead.
> Later I picked the current implementation, hoping guest applications
> can have a check on XCR0 first before it uses extended states. And
> according to spec, in order to use extend states, XCR0 must be set,
> but this is a ring 0 instruction only.

Ah, yes, that's correct of course. So I guess pv-ops could use the
CPUID.OSXSAVE approach then, too.

Jan

Re: [Patch 2/4] Refining Xsave/Xrestore support

[Haitao Shan]

I have done a quick test on my box, setting XCR0 is around 130 tsc
cycles while reading XCR0 is around 80 tsc cycles.

Shan Haitao

2010/10/28 Jan Beulich <JBeulich@novell.com>:
>>>> On 28.10.10 at 09:52, Haitao Shan <maillists.shan@gmail.com> wrote:
>> Then I would prefer to write XCR0 unconditionally. Otherwise, I can
>> only refer to the approach for handling CR4 switches: reading CR4
>> first and checking whether there is a need to write actually.
>> But I don't think <a read to XCR0 plus a data comparison> can save any
>> compared with one unconditional write to XCR0.
>> Are you OK with this?
>
> Depends on the performance expectations of xsetbv and xgetbv
> (and its comparison to moves from/to control registers). At least
> there's no word in the documentation that xsetbv would be
> serializing. I would hope Intel could at least provide approximate
> numbers...
>
> Jan
>
>

Re: [Patch 2/4] Refining Xsave/Xrestore support

[Keir Fraser]

On 28/10/2010 08:52, "Haitao Shan" <maillists.shan@gmail.com> wrote:

> Then I would prefer to write XCR0 unconditionally. Otherwise, I can
> only refer to the approach for handling CR4 switches: reading CR4
> first and checking whether there is a need to write actually.
> But I don't think <a read to XCR0 plus a data comparison> can save any
> compared with one unconditional write to XCR0.
> Are you OK with this?

Note that read_cr4() actually returns a cached copy of cr4, as stashed by
write_cr4(). You should use the same trick for XCR0, and then do the
cached-read-and-compare on context switch, again just as we do for cr4.

 -- Keir

> Thanks for pointing out the memory leak when hvm_vcpu_initialize
> fails. I will update accordingly.
> 
> Shan Haitao
> 
> 2010/10/28 Jan Beulich <JBeulich@novell.com>:
>>>>> On 28.10.10 at 06:58, Haitao Shan <maillists.shan@gmail.com> wrote:
>>> This is the updated patch#2. Thanks.
>> 
>> Sorry, but this is worse than not checking at all: You didn't consider
>> the idle vCPU case here, and hence you may end up having more
>> features enabled in xcr0 for a guest than it should have.
>> 
>> Also I only now noticed that you're leaking the xsave_area allocation
>> in vcpu_initialize() if hvm_vcpu_initialise() fails.
>> 
>> Jan
>> 
>> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [Patch 2/4] Refining Xsave/Xrestore support

[Haitao Shan]

Thanks! I will update and send the patch out later.

Shan Haitao

2010/10/28 Keir Fraser <keir@xen.org>:
> On 28/10/2010 08:52, "Haitao Shan" <maillists.shan@gmail.com> wrote:
>
>> Then I would prefer to write XCR0 unconditionally. Otherwise, I can
>> only refer to the approach for handling CR4 switches: reading CR4
>> first and checking whether there is a need to write actually.
>> But I don't think <a read to XCR0 plus a data comparison> can save any
>> compared with one unconditional write to XCR0.
>> Are you OK with this?
>
> Note that read_cr4() actually returns a cached copy of cr4, as stashed by
> write_cr4(). You should use the same trick for XCR0, and then do the
> cached-read-and-compare on context switch, again just as we do for cr4.
>
>  -- Keir
>
>> Thanks for pointing out the memory leak when hvm_vcpu_initialize
>> fails. I will update accordingly.
>>
>> Shan Haitao
>>
>> 2010/10/28 Jan Beulich <JBeulich@novell.com>:
>>>>>> On 28.10.10 at 06:58, Haitao Shan <maillists.shan@gmail.com> wrote:
>>>> This is the updated patch#2. Thanks.
>>>
>>> Sorry, but this is worse than not checking at all: You didn't consider
>>> the idle vCPU case here, and hence you may end up having more
>>> features enabled in xcr0 for a guest than it should have.
>>>
>>> Also I only now noticed that you're leaking the xsave_area allocation
>>> in vcpu_initialize() if hvm_vcpu_initialise() fails.
>>>
>>> Jan
>>>
>>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xensource.com
>> http://lists.xensource.com/xen-devel
>
>
>

Re: [Patch 2/4] Refining Xsave/Xrestore support

[Jeremy Fitzhardinge]

 On 10/27/2010 07:57 PM, Haitao Shan wrote:
> Hi, Jeremy,
>
> My approach is based an old PV-OPS kernel source. Kernel tries to set
> CR4.OSXSAVE and read it back to determine whether Xsave is actually
> available.
> Could you still use that?

No, because some older versions of Xen kill the domain when they try to
set unknown bits in CR4.

    J

> Shan Haitao
>
> 2010/10/28 Jeremy Fitzhardinge <jeremy@goop.org>:
>>  On 10/27/2010 12:04 AM, Haitao Shan wrote:
>>> Hi, Keir,
>>>
>>> This is patch #2, which adds PV guest Xsave support.
>> How does a PV guest know whether Xsave support is available?  Previous
>> versions of Xen left the xsave cpu feature flag set even though xsave
>> wasn't usable by the domain, so I had to forceably mask it from the
>> cpuid features within the domain.  Given that a PV domain can't rely on
>> X86_FEATURE_XSAVE, how can it tell that the feature is actually usable?
>>
>> Thanks,
>>    J
>>

Re: [Patch 2/4] Refining Xsave/Xrestore support

[Haitao Shan]

Then what about the approach that Jan uses in 2.6.27 tree, which is
reading CPUID-OSXSAVE? If supported, Xen will always set CR4.OSXSAVE.

Shan Haitao

2010/10/29 Jeremy Fitzhardinge <jeremy@goop.org>:
>  On 10/27/2010 07:57 PM, Haitao Shan wrote:
>> Hi, Jeremy,
>>
>> My approach is based an old PV-OPS kernel source. Kernel tries to set
>> CR4.OSXSAVE and read it back to determine whether Xsave is actually
>> available.
>> Could you still use that?
>
> No, because some older versions of Xen kill the domain when they try to
> set unknown bits in CR4.
>
>    J
>
>> Shan Haitao
>>
>> 2010/10/28 Jeremy Fitzhardinge <jeremy@goop.org>:
>>>  On 10/27/2010 12:04 AM, Haitao Shan wrote:
>>>> Hi, Keir,
>>>>
>>>> This is patch #2, which adds PV guest Xsave support.
>>> How does a PV guest know whether Xsave support is available?  Previous
>>> versions of Xen left the xsave cpu feature flag set even though xsave
>>> wasn't usable by the domain, so I had to forceably mask it from the
>>> cpuid features within the domain.  Given that a PV domain can't rely on
>>> X86_FEATURE_XSAVE, how can it tell that the feature is actually usable?
>>>
>>> Thanks,
>>>    J
>>>
>
>

Re: [Patch 2/4] Refining Xsave/Xrestore support

[Jeremy Fitzhardinge]

 On 10/28/2010 05:57 PM, Haitao Shan wrote:
> Then what about the approach that Jan uses in 2.6.27 tree, which is
> reading CPUID-OSXSAVE? If supported, Xen will always set CR4.OSXSAVE.

Sure, if CR4.OSXSAVE is set iff xsave is available, then it should work
fine if we test that.

    J

> Shan Haitao
>
> 2010/10/29 Jeremy Fitzhardinge <jeremy@goop.org>:
>>  On 10/27/2010 07:57 PM, Haitao Shan wrote:
>>> Hi, Jeremy,
>>>
>>> My approach is based an old PV-OPS kernel source. Kernel tries to set
>>> CR4.OSXSAVE and read it back to determine whether Xsave is actually
>>> available.
>>> Could you still use that?
>> No, because some older versions of Xen kill the domain when they try to
>> set unknown bits in CR4.
>>
>>    J
>>
>>> Shan Haitao
>>>
>>> 2010/10/28 Jeremy Fitzhardinge <jeremy@goop.org>:
>>>>  On 10/27/2010 12:04 AM, Haitao Shan wrote:
>>>>> Hi, Keir,
>>>>>
>>>>> This is patch #2, which adds PV guest Xsave support.
>>>> How does a PV guest know whether Xsave support is available?  Previous
>>>> versions of Xen left the xsave cpu feature flag set even though xsave
>>>> wasn't usable by the domain, so I had to forceably mask it from the
>>>> cpuid features within the domain.  Given that a PV domain can't rely on
>>>> X86_FEATURE_XSAVE, how can it tell that the feature is actually usable?
>>>>
>>>> Thanks,
>>>>    J
>>>>
>>