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

RE: [Xen-devel] Re: [PATCH] x86: add strictly sanity check for XSAVE/XRS

To: Keir Fraser <keir@xxxxxxx>, "Kay, Allen M" <allen.m.kay@xxxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>
Subject: RE: [Xen-devel] Re: [PATCH] x86: add strictly sanity check for XSAVE/XRSTOR
From: "Wei, Gang" <gang.wei@xxxxxxxxx>
Date: Mon, 21 Feb 2011 15:06:35 +0800
Accept-language: zh-CN, en-US
Acceptlanguage: zh-CN, en-US
Cc: "wei.huang2@xxxxxxx" <wei.huang2@xxxxxxx>, "Wei, Gang" <gang.wei@xxxxxxxxx>
Delivery-date: Sun, 20 Feb 2011 23:08:06 -0800
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <C985314B.139D3%keir@xxxxxxx>
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: <987664A83D2D224EAE907B061CE93D53019D58FB7D@xxxxxxxxxxxxxxxxxxxxxxxxxxxx> <C985314B.139D3%keir@xxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
Thread-index: AcvOSPtNY1iWQhS8TKOXCx9nhUW9CAAy6UcgAAmsdQUAJbhjMAAPb83YAF6LNkA=
Thread-topic: [Xen-devel] Re: [PATCH] x86: add strictly sanity check for XSAVE/XRSTOR
Keir Fraser wrote on 2011-02-19:
> On 19/02/2011 01:21, "Kay, Allen M" <allen.m.kay@xxxxxxxxx> wrote:
>> Maybe the proper thing to do is to have a new function call
>> xsave_enabled(), this function then checks for whether memory has
>> allocated properly in addition to checking cpu_has_xsave.
>> 
>> What do you think or do you have a better suggestion?
> 
> Yeah, a new function xsave_enabled() which encapsulates the check of
> cpu_has_xsave, plus your new assertions, would be acceptable I think.

Good suggestion. Attached is the reworked patch.

Jimmy

x86: add strictly sanity check for XSAVE/XRSTOR

Replace most checks on cpu_has_xsave with checks on new fn xsave_enabled(), do 
additional sanity checks in the new fn.

Signed-off-by: Wei Gang <gang.wei@xxxxxxxxx>

diff -r 137ad3347504 xen/arch/x86/domain.c
--- a/xen/arch/x86/domain.c     Mon Feb 14 17:02:55 2011 +0000
+++ b/xen/arch/x86/domain.c     Tue Feb 22 19:57:01 2011 +0800
@@ -628,7 +628,7 @@ unsigned long pv_guest_cr4_fixup(const s
         hv_cr4_mask &= ~X86_CR4_DE;
     if ( cpu_has_fsgsbase && !is_pv_32bit_domain(v->domain) )
         hv_cr4_mask &= ~X86_CR4_FSGSBASE;
-    if ( cpu_has_xsave )
+    if ( xsave_enabled(v) )
         hv_cr4_mask &= ~X86_CR4_OSXSAVE;
 
     if ( (guest_cr4 & hv_cr4_mask) != (hv_cr4 & hv_cr4_mask) )
@@ -1402,7 +1402,7 @@ static void __context_switch(void)
         memcpy(stack_regs,
                &n->arch.guest_context.user_regs,
                CTXT_SWITCH_STACK_BYTES);
-        if ( cpu_has_xsave && n->arch.xcr0 != get_xcr0() )
+        if ( xsave_enabled(n) && n->arch.xcr0 != get_xcr0() )
             set_xcr0(n->arch.xcr0);
         n->arch.ctxt_switch_to(n);
     }
diff -r 137ad3347504 xen/arch/x86/domctl.c
--- a/xen/arch/x86/domctl.c     Mon Feb 14 17:02:55 2011 +0000
+++ b/xen/arch/x86/domctl.c     Tue Feb 22 19:30:01 2011 +0800
@@ -1603,7 +1603,7 @@ void arch_get_info_guest(struct vcpu *v,
 #endif
 
     /* Fill legacy context from xsave area first */
-    if ( cpu_has_xsave )
+    if ( xsave_enabled(v) )
         memcpy(v->arch.xsave_area, &v->arch.guest_context.fpu_ctxt,
                sizeof(v->arch.guest_context.fpu_ctxt));
 
diff -r 137ad3347504 xen/arch/x86/hvm/hvm.c
--- a/xen/arch/x86/hvm/hvm.c    Mon Feb 14 17:02:55 2011 +0000
+++ b/xen/arch/x86/hvm/hvm.c    Tue Feb 22 19:58:20 2011 +0800
@@ -773,7 +773,7 @@ static int hvm_load_cpu_ctxt(struct doma
     memcpy(&vc->fpu_ctxt, ctxt.fpu_regs, sizeof(ctxt.fpu_regs));
 
     /* In case xsave-absent save file is restored on a xsave-capable host */
-    if ( cpu_has_xsave )
+    if ( xsave_enabled(v) )
     {
         struct xsave_struct *xsave_area = v->arch.xsave_area;
 
@@ -831,7 +831,7 @@ static int hvm_save_cpu_xsave_states(str
     struct vcpu *v;
     struct hvm_hw_cpu_xsave *ctxt;
 
-    if ( !cpu_has_xsave )
+    if ( !xsave_enabled(NULL) )
         return 0;   /* do nothing */
 
     for_each_vcpu ( d, v )
@@ -846,8 +846,12 @@ static int hvm_save_cpu_xsave_states(str
         ctxt->xcr0 = v->arch.xcr0;
         ctxt->xcr0_accum = v->arch.xcr0_accum;
         if ( v->fpu_initialised )
+        {
+            ASSERT(v->arch.xsave_area);
+
             memcpy(&ctxt->save_area,
                 v->arch.xsave_area, xsave_cntxt_size);
+        }
     }
 
     return 0;
@@ -861,11 +865,6 @@ static int hvm_load_cpu_xsave_states(str
     struct hvm_save_descriptor *desc;
     uint64_t _xfeature_mask;
 
-    /* fails since we can't restore an img saved on xsave-capable host */
-//XXX: 
-    if ( !cpu_has_xsave )
-        return -EINVAL;
-
     /* Which vcpu is this? */
     vcpuid = hvm_load_instance(h);
     if ( vcpuid >= d->max_vcpus || (v = d->vcpu[vcpuid]) == NULL )
@@ -873,6 +872,11 @@ static int hvm_load_cpu_xsave_states(str
         gdprintk(XENLOG_ERR, "HVM restore: domain has no vcpu %u\n", vcpuid);
         return -EINVAL;
     }
+
+    /* fails since we can't restore an img saved on xsave-capable host */
+//XXX: 
+    if ( !xsave_enabled(v) )
+        return -EINVAL;
 
     /* Customized checking for entry since our entry is of variable length */
     desc = (struct hvm_save_descriptor *)&h->data[h->cur];
@@ -2208,7 +2212,7 @@ void hvm_cpuid(unsigned int input, unsig
             __clear_bit(X86_FEATURE_APIC & 31, edx);
 
         /* Fix up OSXSAVE. */
-        if ( cpu_has_xsave )
+        if ( xsave_enabled(v) )
             *ecx |= (v->arch.hvm_vcpu.guest_cr[4] & X86_CR4_OSXSAVE) ?
                      bitmaskof(X86_FEATURE_OSXSAVE) : 0;
         break;
diff -r 137ad3347504 xen/arch/x86/hvm/vmx/vmcs.c
--- a/xen/arch/x86/hvm/vmx/vmcs.c       Mon Feb 14 17:02:55 2011 +0000
+++ b/xen/arch/x86/hvm/vmx/vmcs.c       Tue Feb 22 20:55:28 2011 +0800
@@ -760,7 +760,8 @@ static int construct_vmcs(struct vcpu *v
     /* Host control registers. */
     v->arch.hvm_vmx.host_cr0 = read_cr0() | X86_CR0_TS;
     __vmwrite(HOST_CR0, v->arch.hvm_vmx.host_cr0);
-    __vmwrite(HOST_CR4, mmu_cr4_features | (cpu_has_xsave ? X86_CR4_OSXSAVE : 
0));
+    __vmwrite(HOST_CR4,
+              mmu_cr4_features | (xsave_enabled(v) ? X86_CR4_OSXSAVE : 0));
 
     /* Host CS:RIP. */
     __vmwrite(HOST_CS_SELECTOR, __HYPERVISOR_CS);
diff -r 137ad3347504 xen/arch/x86/i387.c
--- a/xen/arch/x86/i387.c       Mon Feb 14 17:02:55 2011 +0000
+++ b/xen/arch/x86/i387.c       Tue Feb 22 19:29:14 2011 +0800
@@ -69,7 +69,7 @@ void setup_fpu(struct vcpu *v)
     if ( v->fpu_dirtied )
         return;
 
-    if ( cpu_has_xsave )
+    if ( xsave_enabled(v) )
     {
         /*
          * XCR0 normally represents what guest OS set. In case of Xen itself, 
@@ -116,7 +116,7 @@ void save_init_fpu(struct vcpu *v)
     if ( cr0 & X86_CR0_TS )
         clts();
 
-    if ( cpu_has_xsave )
+    if ( xsave_enabled(v) )
     {
         /* XCR0 normally represents what guest OS set. In case of Xen itself,
          * we set all accumulated feature mask before doing save/restore.
diff -r 137ad3347504 xen/arch/x86/traps.c
--- a/xen/arch/x86/traps.c      Mon Feb 14 17:02:55 2011 +0000
+++ b/xen/arch/x86/traps.c      Tue Feb 22 20:09:16 2011 +0800
@@ -771,7 +771,7 @@ static void pv_cpuid(struct cpu_user_reg
         __clear_bit(X86_FEATURE_XTPR % 32, &c);
         __clear_bit(X86_FEATURE_PDCM % 32, &c);
         __clear_bit(X86_FEATURE_DCA % 32, &c);
-        if ( !cpu_has_xsave )
+        if ( !xsave_enabled(current) )
         {
             __clear_bit(X86_FEATURE_XSAVE % 32, &c);
             __clear_bit(X86_FEATURE_AVX % 32, &c);
diff -r 137ad3347504 xen/include/asm-x86/domain.h
--- a/xen/include/asm-x86/domain.h      Mon Feb 14 17:02:55 2011 +0000
+++ b/xen/include/asm-x86/domain.h      Tue Feb 22 20:10:06 2011 +0800
@@ -464,7 +464,7 @@ unsigned long pv_guest_cr4_fixup(const s
     (((v)->arch.guest_context.ctrlreg[4]                    \
       | (mmu_cr4_features & (X86_CR4_PGE | X86_CR4_PSE))    \
       | ((v)->domain->arch.vtsc ? X86_CR4_TSD : 0)         \
-      | ((cpu_has_xsave)? X86_CR4_OSXSAVE : 0))              \
+      | ((xsave_enabled(v))? X86_CR4_OSXSAVE : 0))              \
       & ~X86_CR4_DE)
 #define real_cr4_to_pv_guest_cr4(c) \
     ((c) & ~(X86_CR4_PGE | X86_CR4_PSE | X86_CR4_TSD | X86_CR4_OSXSAVE))
diff -r 137ad3347504 xen/include/asm-x86/hvm/hvm.h
--- a/xen/include/asm-x86/hvm/hvm.h     Mon Feb 14 17:02:55 2011 +0000
+++ b/xen/include/asm-x86/hvm/hvm.h     Tue Feb 22 20:13:23 2011 +0800
@@ -291,7 +291,7 @@ static inline int hvm_do_pmu_interrupt(s
         X86_CR4_DE  | X86_CR4_PSE | X86_CR4_PAE |       \
         X86_CR4_MCE | X86_CR4_PGE | X86_CR4_PCE |       \
         X86_CR4_OSFXSR | X86_CR4_OSXMMEXCPT |           \
-        (cpu_has_xsave ? X86_CR4_OSXSAVE : 0))))
+        (xsave_enabled(NULL) ? X86_CR4_OSXSAVE : 0))))
 
 /* These exceptions must always be intercepted. */
 #define HVM_TRAP_MASK ((1U << TRAP_machine_check) | (1U << TRAP_invalid_op))
diff -r 137ad3347504 xen/include/asm-x86/i387.h
--- a/xen/include/asm-x86/i387.h        Mon Feb 14 17:02:55 2011 +0000
+++ b/xen/include/asm-x86/i387.h        Tue Feb 22 20:22:07 2011 +0800
@@ -31,6 +31,19 @@ void xsave_free_save_area(struct vcpu *v
 #define XSTATE_YMM_OFFSET  XSAVE_AREA_MIN_SIZE
 #define XSTATE_YMM_SIZE    256
 #define XSAVEOPT        (1 << 0)
+
+static inline int xsave_enabled(const struct vcpu *v)
+{
+    if ( cpu_has_xsave )
+    {
+        ASSERT(xsave_cntxt_size >= XSAVE_AREA_MIN_SIZE);
+
+        if ( v )
+            ASSERT(v->arch.xsave_area);
+    }
+
+    return cpu_has_xsave;      
+}
 
 struct xsave_struct
 {

Attachment: xsave_sanity_check_2.patch
Description: xsave_sanity_check_2.patch

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