[PATCH] x86/VMX: don't risk corrupting host CR4

[PATCH] x86/VMX: don't risk corrupting host CR4

[Jan Beulich]

Instead of "syncing" the live value to what mmu_cr4_features has, make
sure vCPU-s run with the value most recently loaded into %cr4, such that
after the next VM exit we continue to run with the intended value rather
than a possibly stale one.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
TBD: Is the conditional really worthwhile, i.e. is the VMWRITE perhaps
     not meaningfully more expensive than the VMREAD?

--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -1676,6 +1676,7 @@ void vmx_vmentry_failure(void)
 void vmx_do_resume(struct vcpu *v)
 {
     bool_t debug_state;
+    unsigned long host_cr4;
 
     if ( v->arch.hvm_vmx.active_cpu == smp_processor_id() )
         vmx_vmcs_reload(v);
@@ -1725,6 +1726,12 @@ void vmx_do_resume(struct vcpu *v)
     }
 
     hvm_do_resume(v);
+
+    /* Sync host CR4 in case its value has changed. */
+    __vmread(HOST_CR4, &host_cr4);
+    if ( host_cr4 != read_cr4() )
+        __vmwrite(HOST_CR4, read_cr4());
+
     reset_stack_and_jump(vmx_asm_do_vmentry);
 }
 
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -947,12 +947,6 @@ static void vmx_ctxt_switch_from(struct
 
 static void vmx_ctxt_switch_to(struct vcpu *v)
 {
-    unsigned long old_cr4 = read_cr4(), new_cr4 = mmu_cr4_features;
-
-    /* HOST_CR4 in VMCS is always mmu_cr4_features. Sync CR4 now. */
-    if ( old_cr4 != new_cr4 )
-        write_cr4(new_cr4);
-
     vmx_restore_guest_msrs(v);
     vmx_restore_dr(v);
 




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] x86/VMX: don't risk corrupting host CR4

[Andrew Cooper]

On 09/03/18 14:18, Jan Beulich wrote:
> Instead of "syncing" the live value to what mmu_cr4_features has, make
> sure vCPU-s run with the value most recently loaded into %cr4, such that
> after the next VM exit we continue to run with the intended value rather
> than a possibly stale one.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> TBD: Is the conditional really worthwhile, i.e. is the VMWRITE perhaps
>      not meaningfully more expensive than the VMREAD?

Which bits are you worried about here?  We play with %cr4 quite a bit in
PV context, but we shouldn't be changing after boot in HVM context.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] x86/VMX: don't risk corrupting host CR4

[Jan Beulich]

>>> On 09.03.18 at 15:35, <andrew.cooper3@citrix.com> wrote:
> On 09/03/18 14:18, Jan Beulich wrote:
>> Instead of "syncing" the live value to what mmu_cr4_features has, make
>> sure vCPU-s run with the value most recently loaded into %cr4, such that
>> after the next VM exit we continue to run with the intended value rather
>> than a possibly stale one.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> TBD: Is the conditional really worthwhile, i.e. is the VMWRITE perhaps
>>      not meaningfully more expensive than the VMREAD?
> 
> Which bits are you worried about here?  We play with %cr4 quite a bit in
> PV context, but we shouldn't be changing after boot in HVM context.

See the discussion on Jürgen's series, playing with CR4.PGE. By
doing what I do here, VMX and SVM will no longer behave
differently in regard to CR4, which I think is quite desirable. But
even beyond that it is a recipe for problems to force a control
register to some fixed value.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] x86/VMX: don't risk corrupting host CR4

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Friday, March 9, 2018 10:19 PM
> 
> Instead of "syncing" the live value to what mmu_cr4_features has, make
> sure vCPU-s run with the value most recently loaded into %cr4, such that
> after the next VM exit we continue to run with the intended value rather
> than a possibly stale one.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Kevin Tian <kevin.tian@intel.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel