WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

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

To: "Wei, Gang" <gang.wei@xxxxxxxxx>, Keir Fraser <keir@xxxxxxx>, "Huang2, Wei" <Wei.Huang2@xxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>
Subject: [Xen-devel] RE: Avoid alloc for xsave before xsave_init
From: "Wei, Gang" <gang.wei@xxxxxxxxx>
Date: Fri, 14 Jan 2011 15:11:28 +0800
Accept-language: zh-CN, en-US
Acceptlanguage: zh-CN, en-US
Cc: "Wei, Gang" <gang.wei@xxxxxxxxx>
Delivery-date: Thu, 13 Jan 2011 23:13:28 -0800
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <F26D193E20BBDC42A43B611D1BDEDE7125198A85EE@xxxxxxxxxxxxxxxxxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <EE335F95F28A664DB4A21289D2AA053BB50CA381@xxxxxxxxxxxxxxxxxxx> <C9551BFA.11CEB%keir@xxxxxxx> <F26D193E20BBDC42A43B611D1BDEDE7125198A85EE@xxxxxxxxxxxxxxxxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
Thread-index: AcuzUojLeS2z6tQSQCO88b+hMX2tAQAC+pjAAAIkzIwAEIzTQAAD8xug
Thread-topic: Avoid alloc for xsave before xsave_init
>>>> 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