From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH] X86: Fix vcpu xsave bug Date: Fri, 15 Nov 2013 17:51:56 +0000 Message-ID: <52865F3C.6030003@citrix.com> References: Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============2501541286095861542==" Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: "Liu, Jinsong" Cc: Jan Beulich , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org --===============2501541286095861542== Content-Type: multipart/alternative; boundary="------------040805010801020501010805" --------------040805010801020501010805 Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit On 15/11/13 16:55, Liu, Jinsong wrote: > commit 420bacd209e31917fd732ef3c1aeae03d6d14d18 > Author: Liu Jinsong > 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 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@lists.xen.org > http://lists.xen.org/xen-devel --------------040805010801020501010805 Content-Type: text/html; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit
On 15/11/13 16:55, Liu, Jinsong wrote:
commit 420bacd209e31917fd732ef3c1aeae03d6d14d18
Author: Liu Jinsong <jinsong.liu@intel.com>
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@intel.com>

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@lists.xen.org
http://lists.xen.org/xen-devel

--------------040805010801020501010805-- --===============2501541286095861542== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============2501541286095861542==--