[PATCH][HVM] fix migration from NX-capable machine to non-NX-capable machine

[PATCH][HVM] fix migration from NX-capable machine to non-NX-capable machine

[David Lively]

[-- Attachment #1: Type: text/plain, Size: 875 bytes --]

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 <dlively@virtualiron.com>

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




[-- Attachment #2: xen-ctxt-switch-nx-fix.patch --]
[-- Type: text/x-patch, Size: 2752 bytes --]

diff -r a1db76ddfa68 xen/arch/x86/hvm/vmx/vmx.c
--- a/xen/arch/x86/hvm/vmx/vmx.c	Thu Sep 27 13:21:28 2007 -0400
+++ b/xen/arch/x86/hvm/vmx/vmx.c	Thu Sep 27 16:13:06 2007 -0400
@@ -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;

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

Re: [PATCH][HVM] fix migration from NX-capable machine to non-NX-capable machine

[Keir Fraser]

On 27/9/07 22:12, "David Lively" <dlively@virtualiron.com> wrote:

> 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 <dlively@virtualiron.com>
> diff -r a1db76ddfa68 xen/arch/x86/hvm/vmx/vmx.c
> --- a/xen/arch/x86/hvm/vmx/vmx.c Thu Sep 27 13:21:28 2007 -0400
> +++ b/xen/arch/x86/hvm/vmx/vmx.c Thu Sep 27 16:13:06 2007 -0400
> @@ -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

Re: [PATCH][HVM] fix migration from NX-capable machine to non-NX-capable machine

[Dave Lively]

On 9/28/07, Keir Fraser <Keir.Fraser@cl.cam.ac.uk> wrote:
> On 27/9/07 22:12, "David Lively" <dlively@virtualiron.com> wrote:
> > +    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.

> 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

Re: [PATCH][HVM] fix migration from NX-capable machine to non-NX-capable machine

[Christoph Egger]

>
> > 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


-- 
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

Re: [PATCH][HVM] fix migration from NX-capable machine to non-NX-capable machine

[Keir Fraser]

On 27/9/07 22:12, "David Lively" <dlively@virtualiron.com> wrote:

> 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

Re: [PATCH][HVM] fix migration from NX-capable machine to non-NX-capable machine

[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


On 9/28/07, Keir Fraser <Keir.Fraser@cl.cam.ac.uk> wrote:
> On 27/9/07 22:12, "David Lively" <dlively@virtualiron.com> wrote:
>
> > 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
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
>

Re: [PATCH][HVM] fix migration from NX-capable machine to non-NX-capable machine

[Keir Fraser]

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

On 28/9/07 14:23, "Dave Lively" <dlively@virtualiron.com> wrote:

> 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
> 
> 
> On 9/28/07, Keir Fraser <Keir.Fraser@cl.cam.ac.uk> wrote:
>> On 27/9/07 22:12, "David Lively" <dlively@virtualiron.com> wrote:
>> 
>>> 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
>> Xen-devel@lists.xensource.com
>> http://lists.xensource.com/xen-devel
>>