[PATCH 2/4] XSAVE/XRSTOR: enable guest save/restore

[PATCH 2/4] XSAVE/XRSTOR: enable guest save/restore

[Han, Weidong]

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

This patch enables save/restore xsave states for guests. Because xsave area size is not fixed, this patch uses 4K Bytes for future extension.

Signed-off-by: Weidong Han <weidong.han@intel.com>

[-- Attachment #2: guest-sr.patch --]
[-- Type: application/octet-stream, Size: 5320 bytes --]

diff -r 38ca9e4e30bc tools/include/xen-foreign/reference.size
--- a/tools/include/xen-foreign/reference.size	Fri Aug 27 12:33:20 2010 -0400
+++ b/tools/include/xen-foreign/reference.size	Fri Aug 27 12:36:00 2010 -0400
@@ -9,7 +9,7 @@ ia64_tr_entry             |       -     
 ia64_tr_entry             |       -       -      32
 vcpu_tr_regs              |       -       -     768
 vcpu_guest_context_regs   |       -       -   22176
-vcpu_guest_context        |    2800    5168   22208
+vcpu_guest_context        |    6384    8752   22208
 arch_vcpu_info            |      24      16       0
 vcpu_time_info            |      32      32      32
 vcpu_info                 |      64      64      48
diff -r 38ca9e4e30bc xen/arch/x86/hvm/hvm.c
--- a/xen/arch/x86/hvm/hvm.c	Fri Aug 27 12:33:20 2010 -0400
+++ b/xen/arch/x86/hvm/hvm.c	Fri Aug 27 12:38:01 2010 -0400
@@ -575,8 +575,19 @@ static int hvm_save_cpu_ctxt(struct doma
         vc = &v->arch.guest_context;
 
         if ( v->fpu_initialised )
+        {
             memcpy(ctxt.fpu_regs, &vc->fpu_ctxt, sizeof(ctxt.fpu_regs));
-        else 
+
+            /*
+             * If save a guest on platforms which doesn't support xsave/xrstor,
+             * but restore it on platforms which support xsave/xrstor, it needs
+             * to set XSTATE_BV to XSTATE_FP_SSE to make it succeed
+             */
+            if ( !cpu_has_xsave )
+                ((struct xsave_struct *)ctxt.fpu_regs)->xsave_hdr.xstate_bv
+                    = XSTATE_FP_SSE;
+        }
+        else
             memset(ctxt.fpu_regs, 0, sizeof(ctxt.fpu_regs));
 
         ctxt.rax = vc->user_regs.eax;
@@ -807,13 +818,7 @@ int hvm_vcpu_initialise(struct 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;
+        xsave_init_save_area(&v->arch.guest_context.fpu_ctxt);
         v->arch.hvm_vcpu.xcr0 = XSTATE_FP_SSE;
     }
 
@@ -879,7 +884,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 38ca9e4e30bc xen/arch/x86/i387.c
--- a/xen/arch/x86/i387.c	Fri Aug 27 12:33:20 2010 -0400
+++ b/xen/arch/x86/i387.c	Fri Aug 27 12:36:00 2010 -0400
@@ -89,6 +89,7 @@ void restore_fpu(struct vcpu *v)
 void restore_fpu(struct vcpu *v)
 {
     char *fpu_ctxt = v->arch.guest_context.fpu_ctxt.x;
+    int fxsave_size = 512;
 
     /*
      * FXRSTOR can fault if passed a corrupted data block. We handle this
@@ -123,7 +124,7 @@ void restore_fpu(struct vcpu *v)
             ".previous                \n"
             : 
             : "m" (*fpu_ctxt),
-              "i" (sizeof(v->arch.guest_context.fpu_ctxt)/4)
+              "i" (fxsave_size/4)
 #ifdef __x86_64__
              ,"cdaSDb" (fpu_ctxt)
 #endif
diff -r 38ca9e4e30bc xen/include/asm-x86/hvm/vcpu.h
--- a/xen/include/asm-x86/hvm/vcpu.h	Fri Aug 27 12:33:20 2010 -0400
+++ b/xen/include/asm-x86/hvm/vcpu.h	Fri Aug 27 12:37:03 2010 -0400
@@ -48,13 +48,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;
diff -r 38ca9e4e30bc xen/include/public/arch-x86/hvm/save.h
--- a/xen/include/public/arch-x86/hvm/save.h	Fri Aug 27 12:33:20 2010 -0400
+++ b/xen/include/public/arch-x86/hvm/save.h	Fri Aug 27 12:40:03 2010 -0400
@@ -49,7 +49,7 @@ DECLARE_HVM_SAVE_TYPE(HEADER, 1, struct 
  */
 
 struct hvm_hw_cpu {
-    uint8_t  fpu_regs[512];
+    uint8_t  fpu_regs[4096];
 
     uint64_t rax;
     uint64_t rbx;
diff -r 38ca9e4e30bc xen/include/public/arch-x86/xen.h
--- a/xen/include/public/arch-x86/xen.h	Fri Aug 27 12:33:20 2010 -0400
+++ b/xen/include/public/arch-x86/xen.h	Fri Aug 27 12:36:00 2010 -0400
@@ -107,11 +107,17 @@ typedef uint64_t tsc_timestamp_t; /* RDT
 
 /*
  * The following is all CPU context. Note that the fpu_ctxt block is filled 
- * in by FXSAVE if the CPU has feature FXSR; otherwise FSAVE is used.
+ * in by XSAVE if the CPU has feature XSAVE, otherwise use FXSAVE if the CPU
+ * has feature FXSR; otherwise FSAVE is used.
  */
 struct vcpu_guest_context {
-    /* FPU registers come first so they can be aligned for FXSAVE/FXRSTOR. */
-    struct { char x[512]; } fpu_ctxt;       /* User-level FPU registers     */
+    /*
+     * FPU registers come first so they can be aligned for
+     * FXSAVE/FXRSTOR and XSAVE/XRSTOR. Using 4096 Bytes for
+     * future state extensions
+     */
+    struct { char x[4096]; } fpu_ctxt;
+
 #define VGCF_I387_VALID                (1<<0)
 #define VGCF_IN_KERNEL                 (1<<2)
 #define _VGCF_i387_valid               0

[-- 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] XSAVE/XRSTOR: enable guest save/restore

[Jan Beulich]

>>> On 31.08.10 at 04:59, "Han, Weidong" <weidong.han@intel.com> wrote:
>--- a/xen/include/public/arch-x86/xen.h	Fri Aug 27 12:33:20 2010 -0400
>+++ b/xen/include/public/arch-x86/xen.h	Fri Aug 27 12:36:00 2010 -0400
>@@ -107,11 +107,17 @@ typedef uint64_t tsc_timestamp_t; /* RDT
> 
> /*
>  * The following is all CPU context. Note that the fpu_ctxt block is filled 
>- * in by FXSAVE if the CPU has feature FXSR; otherwise FSAVE is used.
>+ * in by XSAVE if the CPU has feature XSAVE, otherwise use FXSAVE if the CPU
>+ * has feature FXSR; otherwise FSAVE is used.
>  */
> struct vcpu_guest_context {
>-    /* FPU registers come first so they can be aligned for FXSAVE/FXRSTOR. */
>-    struct { char x[512]; } fpu_ctxt;       /* User-level FPU registers     */
>+    /*
>+     * FPU registers come first so they can be aligned for
>+     * FXSAVE/FXRSTOR and XSAVE/XRSTOR. Using 4096 Bytes for
>+     * future state extensions
>+     */
>+    struct { char x[4096]; } fpu_ctxt;
>+
> #define VGCF_I387_VALID                (1<<0)
> #define VGCF_IN_KERNEL                 (1<<2)
> #define _VGCF_i387_valid               0

As Keir already indicated, you can't change the size of this structure.
I'd say that it was a mistake to include the FPU state directly here in
the first place - you'll have to invent a mechanism to (compatibly)
not make this a requirement anymore. E.g. use the reserved part of
fpu_ctxt to store a guest handle referring to the actual area: This
ought to work as (iirc) the structure is used as input only
(VCPUOP_initialize) outside of the tools, and the domctl interface is
allowed to change as long as you don't break compatibility with
stored data (saved guest images).

Jan

Re: [PATCH 2/4] XSAVE/XRSTOR: enable guest save/restore

[Keir Fraser]

On 31/08/2010 08:08, "Jan Beulich" <JBeulich@novell.com> wrote:

>> #define VGCF_I387_VALID                (1<<0)
>> #define VGCF_IN_KERNEL                 (1<<2)
>> #define _VGCF_i387_valid               0
> 
> As Keir already indicated, you can't change the size of this structure.
> I'd say that it was a mistake to include the FPU state directly here in
> the first place - you'll have to invent a mechanism to (compatibly)
> not make this a requirement anymore. E.g. use the reserved part of
> fpu_ctxt to store a guest handle referring to the actual area: This
> ought to work as (iirc) the structure is used as input only
> (VCPUOP_initialize) outside of the tools, and the domctl interface is
> allowed to change as long as you don't break compatibility with
> stored data (saved guest images).

I would agree but for the fact that it appears that XSAVE is only supported
for HVM guests right now. Hence this whole rigmarole is actually currently
pointless, since HVM guests do not initialise/save/restore the
vcpu_guest_context structure. I think probably this bit of the patch could
simply be dropped until PV support is added.

 -- Keir

Re: [PATCH 2/4] XSAVE/XRSTOR: enable guest save/restore

[Weidong Han]

Keir Fraser wrote:
> On 31/08/2010 08:08, "Jan Beulich" <JBeulich@novell.com> wrote:
>
>   
>>> #define VGCF_I387_VALID                (1<<0)
>>> #define VGCF_IN_KERNEL                 (1<<2)
>>> #define _VGCF_i387_valid               0
>>>       
>> As Keir already indicated, you can't change the size of this structure.
>> I'd say that it was a mistake to include the FPU state directly here in
>> the first place - you'll have to invent a mechanism to (compatibly)
>> not make this a requirement anymore. E.g. use the reserved part of
>> fpu_ctxt to store a guest handle referring to the actual area: This
>> ought to work as (iirc) the structure is used as input only
>> (VCPUOP_initialize) outside of the tools, and the domctl interface is
>> allowed to change as long as you don't break compatibility with
>> stored data (saved guest images).
>>     
>
> I would agree but for the fact that it appears that XSAVE is only supported
> for HVM guests right now. Hence this whole rigmarole is actually currently
> pointless, since HVM guests do not initialise/save/restore the
> vcpu_guest_context structure. I think probably this bit of the patch could
> simply be dropped until PV support is added.
>
>  -- Keir
>
> I will send out other patches soon, and revise this patch later.
Regards,
Weidong
>  
>