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

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

Jan Beulich wrote:
>>>> On 18.11.13 at 11:35, "Liu, Jinsong" <jinsong.liu@xxxxxxxxx> wrote:
>> Jan Beulich wrote:
>>>>>> On 15.11.13 at 17:55, "Liu, Jinsong" <jinsong.liu@xxxxxxxxx>
>>>>>> wrote: 
>>>> @@ -257,22 +257,29 @@ void vcpu_restore_fpu_lazy(struct vcpu *v)  
>>>>  */ void vcpu_save_fpu(struct vcpu *v)
>>>>  {
>>>> -    if ( !v->fpu_dirtied )
>>>> -        return;
>>>> -
>>> 
>>> And the - afaict - the only changed needed to this function is the
>>> deletion above. 
>>> 
>> 
>> If I didn't misunderstand your meaning, it can not only delete these
>> 2 lines, say, when (!v->fpu_dirtied) and in old platform that do
>> fpu_fxsave/fpu_fsave?
> 
> Sorry, I don't understand what you're asking.
> 

The problem is I don't understand your last comments:
'And the - afaict - the only changed needed to this function is the deletion 
above.'

Seems some misunderstanding here :)
So would you please give me the code of your thought based on the patch below?

Thanks,
Jinsong

=======================

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 */
     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();
 }
 
-- 
1.7.1

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