fix migration from NX-capable machine to non-NX-capable machine

PATCH In review David Lively 3 participants 7 messages Sep 27, 2007 – Sep 28, 2007

lore

Share & tools

lore

David Lively

author

2007-09-27 21:12 UTC #0 lore raw

PATCH 1/1 fix migration from NX-capable machine to non-NX-capable machine

Hi Keir -

The attached patch (to 3.1.1-rc2) fixes a hypervisor crash that we're seeing when migrating a HVM guest from a machine that supports the NX bit to one that doesn't (e.g., because it's disabled in the BIOS). It keeps the guest copy of EFER "as is", so the guest will see EFER_NX if it previously set it -- we just won't propagate this EFER bit to a non-NX-capable host.

Signed-off-by David Lively <[email protected]>

I'm not sure whether this problem exists for AMDV, which has an EFER shadow in its vmcb. Does their vmentry mechanism take care of this masking, or will we GPF when trying to propagate the shadow EFER_NX bit to the real one (when NX is disabled)? I'll followup on this and submit a separate AMD patch if it's necessary.

Note this doesn't apply to unstable. I'll do an unstable version if/when this one settles out.

Dave

xen/arch/x86/hvm/vmx/vmx.c +10 -8

diff -r a1db76ddfa68 xen/arch/x86/hvm/vmx/vmx.c
@@ -369,6 +369,7 @@ static int long_mode_do_msr_write(struct
     struct vcpu *v = current;
     struct vmx_msr_state *guest_msr_state = &v->arch.hvm_vmx.msr_state;
     struct vmx_msr_state *host_msr_state = &this_cpu(host_msr_state);
+    u64 efer_mask = EFER_SCE || (cpu_has_nx ? EFER_NX : 0);
 
     HVM_DBG_LOG(DBG_LEVEL_0, "msr 0x%x content 0x%"PRIx64, ecx, msr_content);

@@ -406,9 +407,9 @@ static int long_mode_do_msr_write(struct
             }
         }
 
-        if ( (msr_content ^ v->arch.hvm_vmx.efer) & (EFER_NX|EFER_SCE) )
-            write_efer((read_efer() & ~(EFER_NX|EFER_SCE)) |
-                       (msr_content & (EFER_NX|EFER_SCE)));
+        if ( (msr_content ^ v->arch.hvm_vmx.efer) & efer_mask )
+            write_efer((read_efer() & ~efer_mask) |
+                       (msr_content & efer_mask));
 
         v->arch.hvm_vmx.efer = msr_content;
         break;

@@ -497,6 +498,7 @@ static void vmx_restore_guest_msrs(struc
     struct vmx_msr_state *guest_msr_state, *host_msr_state;
     unsigned long guest_flags;
     int i;
+    u64 efer_mask = EFER_SCE || (cpu_has_nx ? EFER_NX : 0);
 
     guest_msr_state = &v->arch.hvm_vmx.msr_state;
     host_msr_state = &this_cpu(host_msr_state);

@@ -517,13 +519,13 @@ static void vmx_restore_guest_msrs(struc
         clear_bit(i, &guest_flags);
     }
 
-    if ( (v->arch.hvm_vmx.efer ^ read_efer()) & (EFER_NX | EFER_SCE) )
+    if ( (v->arch.hvm_vmx.efer ^ read_efer()) & efer_mask )
     {
         HVM_DBG_LOG(DBG_LEVEL_2,
                     "restore guest's EFER with value %lx",
                     v->arch.hvm_vmx.efer);
-        write_efer((read_efer() & ~(EFER_NX | EFER_SCE)) |
-                   (v->arch.hvm_vmx.efer & (EFER_NX | EFER_SCE)));
+        write_efer((read_efer() & ~efer_mask) |
+                   (v->arch.hvm_vmx.efer & efer_mask));
     }
 }

@@ -550,7 +552,7 @@ static void vmx_restore_host_msrs(void)
 
 static void vmx_restore_guest_msrs(struct vcpu *v)
 {
-    if ( (v->arch.hvm_vmx.efer ^ read_efer()) & EFER_NX )
+    if ( cpu_has_nx && ((v->arch.hvm_vmx.efer ^ read_efer()) & EFER_NX) )
     {
         HVM_DBG_LOG(DBG_LEVEL_2,
                     "restore guest's EFER with value %lx",

@@ -598,7 +600,7 @@ static int long_mode_do_msr_write(struct
             return 0;
         }
 
-        if ( (msr_content ^ v->arch.hvm_vmx.efer) & EFER_NX )
+        if ( cpu_has_nx && ((msr_content ^ v->arch.hvm_vmx.efer) & EFER_NX) )
             write_efer((read_efer() & ~EFER_NX) | (msr_content & EFER_NX));
 
         v->arch.hvm_vmx.efer = msr_content;

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Keir Fraser

2007-09-28 06:58 UTC #1 lore raw

Quoted 12 lines · David Lively

The attached patch (to 3.1.1-rc2) fixes a hypervisor crash that we're seeing when migrating a HVM guest from a machine that supports the NX bit to one that doesn't (e.g., because it's disabled in the BIOS). It keeps the guest copy of EFER "as is", so the guest will see EFER_NX if it previously set it -- we just won't propagate this EFER bit to a non-NX-capable host.
Signed-off-by David Lively <[email protected]>
diff -r a1db76ddfa68 xen/arch/x86/hvm/vmx/vmx.c

xen/arch/x86/hvm/vmx/vmx.c Thu Sep 27 16:13:06 2007 -0400 static int long_mode_do_msr_write(struct · line 372

Quoted 4 lines

xen/arch/x86/hvm/vmx/vmx.c Thu Sep 27 16:13:06 2007 -0400 renamed from xen/arch/x86/hvm/vmx/vmx.c Thu Sep 27 13:21:28 2007 -0400 +1 -0

@@ -369,6 +369,7 @@ static int long_mode_do_msr_write(struct
     struct vcpu *v = current;
     struct vmx_msr_state *guest_msr_state = &v->arch.hvm_vmx.msr_state;
     struct vmx_msr_state *host_msr_state = &this_cpu(host_msr_state);
+    u64 efer_mask = EFER_SCE || (cpu_has_nx ? EFER_NX : 0);

Wrong kind of OR?

As for AMD, are there any SVM CPUs that do not also support NX?

-- Keir

Keir Fraser

2007-09-28 07:18 UTC #4 lore raw

In reply to David Lively

Quoted 6 lines · David Lively

The attached patch (to 3.1.1-rc2) fixes a hypervisor crash that we're seeing when migrating a HVM guest from a machine that supports the NX bit to one that doesn't (e.g., because it's disabled in the BIOS). It keeps the guest copy of EFER "as is", so the guest will see EFER_NX if it previously set it -- we just won't propagate this EFER bit to a non-NX-capable host.

Actually this issue is very nearly handled by xen-3.1-testing:15064 and xen-unstable:15066. The HVM state load code that sets guest efer should barf on !cpu_has_nx && EFER_NX, just as the wrmsr-handling code does.

-- Keir

Dave Lively

author

2007-09-28 13:22 UTC #2 lore raw

In reply to Keir Fraser

Quoted 4 lines · Keir Fraser

Quoted 1 line · David Lively
+ u64 efer_mask = EFER_SCE || (cpu_has_nx ? EFER_NX : 0);
Wrong kind of OR?

Ack. Yes, thank you. It was "working" because EFER_SCE == 1, so efer_mask was always 1, but this meant I was never propagating EFER_NX .... I'll retest with '|' and get back to you.

Quoted 1 line

As for AMD, are there any SVM CPUs that do not also support NX?

I saw this bug on an Intel CPU that supported NX -- it was just disabled in the BIOS. I'll be speaking with some AMD folks later this morning, so I'll ask whether the same thing is possible w/AMD SVM.

Dave

Dave Lively

author

2007-09-28 13:23 UTC #5 lore raw

In reply to Keir Fraser

But that means you'll crash a guest that migrates from a NX-capable machine to a on-NX-capable machine. (Though of course this is detectable ahead of time, so the migration control code should just refuse to migrate in this case.)

If we really believe that it's dangerous to let a guest see NX-capability that doesn't really exist, perhaps we're better off hiding NX altogether (perhaps optionally)? I thought over that beforehand, but it seemed kind of drastic to me, though I realize it's a much more "pure" solution in that the guest can't see inconsistencies due to virtualization.

Dave

Quoted 22 lines · Keir Fraser

Quoted 6 lines · David Lively
The attached patch (to 3.1.1-rc2) fixes a hypervisor crash that we're seeing when migrating a HVM guest from a machine that supports the NX bit to one that doesn't (e.g., because it's disabled in the BIOS). It keeps the guest copy of EFER "as is", so the guest will see EFER_NX if it previously set it -- we just won't propagate this EFER bit to a non-NX-capable host.
Actually this issue is very nearly handled by xen-3.1-testing:15064 and xen-unstable:15066. The HVM state load code that sets guest efer should barf on !cpu_has_nx && EFER_NX, just as the wrmsr-handling code does.
-- Keir
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Christoph Egger

2007-09-28 13:32 UTC #3 lore raw

In reply to Dave Lively

Quoted 6 lines

Quoted 2 lines
As for AMD, are there any SVM CPUs that do not also support NX?
I saw this bug on an Intel CPU that supported NX -- it was just disabled in the BIOS. I'll be speaking with some AMD folks later this morning, so I'll ask whether the same thing is possible w/AMD SVM.

You can figure out, if NX is supported by looking up the EFER. It is possible to enable/disable NX in the PAE-paging mode by setting/clearing the NXE bit in EFER.

Christoph

Signature · 12 lines

AMD Saxony, Dresden, Germany
Operating System Research Center

Legal Information:
AMD Saxony Limited Liability Company & Co. KG
Sitz (Geschäftsanschrift):
   Wilschdorfer Landstr. 101, 01109 Dresden, Deutschland
Registergericht Dresden: HRA 4896
vertretungsberechtigter Komplementär:
   AMD Saxony LLC (Sitz Wilmington, Delaware, USA)
Geschäftsführer der AMD Saxony LLC:
   Dr. Hans-R. Deppe, Thomas McCoy

Keir Fraser

2007-09-28 14:01 UTC #6 lore raw

In reply to Dave Lively

Well, either we should consistently silently mask NX, or we should consistently fail on it. Currently the code mostly does the latter. And I think that makes most sense, and what we're looking at here is a lack of CPUID configurability. I'd rather see patches to fix that, so that NX is consistently hidden, according to user preference.

-- Keir

Quoted 38 lines · Dave Lively

But that means you'll crash a guest that migrates from a NX-capable machine to a on-NX-capable machine. (Though of course this is detectable ahead of time, so the migration control code should just refuse to migrate in this case.)
If we really believe that it's dangerous to let a guest see NX-capability that doesn't really exist, perhaps we're better off hiding NX altogether (perhaps optionally)? I thought over that beforehand, but it seemed kind of drastic to me, though I realize it's a much more "pure" solution in that the guest can't see inconsistencies due to virtualization.
Dave
Quoted 22 lines · Keir Fraser
Quoted 6 lines · David Lively
The attached patch (to 3.1.1-rc2) fixes a hypervisor crash that we're seeing when migrating a HVM guest from a machine that supports the NX bit to one that doesn't (e.g., because it's disabled in the BIOS). It keeps the guest copy of EFER "as is", so the guest will see EFER_NX if it previously set it -- we just won't propagate this EFER bit to a non-NX-capable host.
Actually this issue is very nearly handled by xen-3.1-testing:15064 and xen-unstable:15066. The HVM state load code that sets guest efer should barf on !cpu_has_nx && EFER_NX, just as the wrmsr-handling code does.
-- Keir
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel