Re: [Xen-devel] [PATCH] X86: Fix vcpu xsave bug

["Liu, Jinsong" <jinsong.liu@xxxxxxxxx>]

No, what I meant is, nonlazy xstates should be xsaved each time vcpu_save_fpu.

Operation to nonlazy xstates will not trigger #NM exception, so whenever vcpu scheduled in it should get restored and whenever vcpu scheduled out it should get saved.

 

As for the comments I just want to unify it with other 'clts' comments.

 

Thanks,

Jinsong

---

From: Andrew Cooper [mailto:andrew.cooper3@xxxxxxxxxx]
Sent: Saturday, November 16, 2013 1:52 AM
To: Liu, Jinsong
Cc: Jan Beulich; xen-devel@xxxxxxxxxxxxx
Subject: Re: [Xen-devel] [PATCH] X86: Fix vcpu xsave bug

On 15/11/13 16:55, Liu, Jinsong wrote:

commit 420bacd209e31917fd732ef3c1aeae03d6d14d18
Author: Liu Jinsong <jinsong.liu@xxxxxxxxx>
Date:   Sat Nov 16 06:15:11 2013 +0800

    X86: Fix vcpu xsave bug
    
    When nonlazy xstates used, it should be xsaved though lazy xstates are not dirty.
    
    Signed-off-by: Liu Jinsong <jinsong.liu@xxxxxxxxx>

Do you mean "...xsaved as though lazy...", are you stating that currently, lazy states are not actually dirty?

Furthermore, can you describe why, and what goes wrong if you dont?  It is not obvious why this change is needed.

diff --git a/xen/arch/x86/i387.c b/xen/arch/x86/i387.c
index 7649274..f1d2ccc 100644
--- a/xen/arch/x86/i387.c
+++ b/xen/arch/x86/i387.c
@@ -134,7 +134,7 @@ static inline void fpu_frstor(struct vcpu *v)
 /*      FPU Save Functions     */
 /*******************************/
 /* Save x87 extended state */
-static inline void fpu_xsave(struct vcpu *v)
+static inline void fpu_xsave(struct vcpu *v, uint64_t mask)
 {
     bool_t ok;
 
@@ -145,7 +145,7 @@ static inline void fpu_xsave(struct vcpu *v)
      */
     ok = set_xcr0(v->arch.xcr0_accum | XSTATE_FP_SSE);
     ASSERT(ok);
-    xsave(v, v->arch.nonlazy_xstate_used ? XSTATE_ALL : XSTATE_LAZY);
+    xsave(v, mask);
     ok = set_xcr0(v->arch.xcr0 ?: XSTATE_FP_SSE);
     ASSERT(ok);
 }
@@ -257,22 +257,29 @@ void vcpu_restore_fpu_lazy(struct vcpu *v)
  */
 void vcpu_save_fpu(struct vcpu *v)
 {
-    if ( !v->fpu_dirtied )
-        return;
-
     ASSERT(!is_idle_vcpu(v));
 
-    /* This can happen, if a paravirtualised guest OS has set its CR0.TS. */
+    /* Avoid recursion */

Why are you changing this comment?

~Andrew

     clts();
-
-    if ( cpu_has_xsave )
-        fpu_xsave(v);
-    else if ( cpu_has_fxsr )
-        fpu_fxsave(v);
+    if ( !v->fpu_dirtied )
+    {
+        if ( v->arch.nonlazy_xstate_used )
+        {
+            ASSERT(cpu_has_xsave);
+            fpu_xsave(v, XSTATE_NONLAZY);
+        }
+    }
     else
-        fpu_fsave(v);
+    {
+        if ( cpu_has_xsave )
+            fpu_xsave(v, XSTATE_ALL);
+        else if ( cpu_has_fxsr )
+            fpu_fxsave(v);
+        else
+            fpu_fsave(v);
 
-    v->fpu_dirtied = 0;
+        v->fpu_dirtied = 0;
+    }
     stts();
 }
 

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

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