[Xen-devel] RE: Avoid alloc for xsave before xsave_init

["Wei, Gang" <gang.wei@xxxxxxxxx>]

>>>> While debugging some weird booting failure bugs, just found
>>>> currently, xsave_alloc_save_area will be called in
>>>> init_idle_domain->scheduler_init->alloc_vcpu->vcpu_initialise
>>>> calls, it is earlier than xsave_init called in identity_cpu(). This
>>>> may causing buffer overflow on xmem_pool. I am thinking about how to fix 
>>>> it.
>>>
>>> Was the issue caused by the uninitialized variable
>>> xsave_cntxt_size, triggering problem for _xmalloc()? If so, one
>>> solution is to set
>>> xsave_cntxt_size=576 (the default value after reset) as a default
>>> value. When
>>> xsave_alloc_save_area() is called for idel VCPU, _xmalloc() will
>>> initialize
>>> 576 bytes. Idle domain doesn't change xcr0 from my understanding.
>>> So its xcr0 is XSTATE_FP_SSE all the time.
>> 
>> Idle domain isn't using FPU,SSE,AVX or any such extended state and
>> doesn't need it saved. Xsave_{alloc,free}_save_area() should
>> test-and-exit on is_idle_vcpu(), and our context switch code should
>> not be doing XSAVE when switching out an idle vcpu (I hope this is
>> the case already, as it would be a pointless waste of time).
> 
> I agree that do test-and-exit on is_idle_vcpu() in 
> Xsave_{alloc,free}_save_area.
> Further, We'd better add assert(xsave_cntxt_size>=576) after the
> test-and-exit clause to ensure no buffer overflow will happen in the future.
> 
> I reviewed the context switch code and assure context switch code not
> be doing XSAVE when switching out an idle vcpu.

Here is the patch.

diff -r d839631b6048 xen/arch/x86/i387.c
--- a/xen/arch/x86/i387.c       Thu Jan 13 00:18:35 2011 +0000
+++ b/xen/arch/x86/i387.c       Sat Jan 15 20:15:24 2011 +0800
@@ -35,6 +35,9 @@ void save_init_fpu(struct vcpu *v)
 
     if ( cpu_has_xsave )
     {
+        if ( is_idle_vcpu(v) )
+            goto out;
+
         /* XCR0 normally represents what guest OS set. In case of Xen itself,
          * we set all accumulated feature mask before doing save/restore.
          */
@@ -87,6 +90,7 @@ void save_init_fpu(struct vcpu *v)
         asm volatile ( "fnsave %0 ; fwait" : "=m" (*fpu_ctxt) );
     }
 
+out:
     v->fpu_dirtied = 0;
     write_cr0(cr0|X86_CR0_TS);
 }
@@ -138,6 +142,7 @@ void restore_fpu(struct vcpu *v)
 }
 
 #define XSTATE_CPUID 0xd
+#define XSAVE_AREA_MIN_SIZE (512 + 64)
 
 /*
  * Maximum size (in byte) of the XSAVE/XRSTOR save area required by all
@@ -177,7 +182,7 @@ void xsave_init(void)
     }
 
     /* FP/SSE, XSAVE.HEADER, YMM */
-    min_size =  512 + 64 + ((eax & XSTATE_YMM) ? XSTATE_YMM_SIZE : 0);
+    min_size =  XSAVE_AREA_MIN_SIZE + ((eax & XSTATE_YMM) ? XSTATE_YMM_SIZE : 
0);
     BUG_ON(ecx < min_size);
 
     /*
@@ -214,8 +219,10 @@ int xsave_alloc_save_area(struct vcpu *v
 {
     void *save_area;
 
-    if ( !cpu_has_xsave )
+    if ( !cpu_has_xsave || is_idle_vcpu(v) )
         return 0;
+
+    BUG_ON(xsave_cntxt_size < XSAVE_AREA_MIN_SIZE);
 
     /* XSAVE/XRSTOR requires the save area be 64-byte-boundary aligned. */
     save_area = _xmalloc(xsave_cntxt_size, 64);
@@ -235,6 +242,9 @@ int xsave_alloc_save_area(struct vcpu *v
 
 void xsave_free_save_area(struct vcpu *v)
 {
+    if ( is_idle_vcpu(v) )
+        return;
+
     xfree(v->arch.xsave_area);
     v->arch.xsave_area = NULL;
 }
diff -r d839631b6048 xen/include/asm-x86/i387.h
--- a/xen/include/asm-x86/i387.h        Thu Jan 13 00:18:35 2011 +0000
+++ b/xen/include/asm-x86/i387.h        Sat Jan 15 20:08:14 2011 +0800
@@ -134,6 +134,9 @@ static inline void setup_fpu(struct vcpu
         v->fpu_dirtied = 1;
         if ( cpu_has_xsave )
         {
+            if ( is_idle_vcpu(v) )
+                return;
+
             if ( !v->fpu_initialised )
                 v->fpu_initialised = 1;

Jimmy

Attachment: xsave_init_fix.patch
Description: xsave_init_fix.patch

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