diff mbox series

[v2,06/18] KVM: nVMX: try to set EFER bits correctly when init'ing entry controls

Message ID 20180828160459.14093-7-sean.j.christopherson@intel.com (mailing list archive)
State New, archived
Headers show
Series KVM: nVMX: add option to perform early consistency checks via H/W | expand

Commit Message

Sean Christopherson Aug. 28, 2018, 4:04 p.m. UTC 
VM_ENTRY_IA32E_MODE and VM_{ENTRY,EXIT}_LOAD_IA32_EFER will be
explicitly set/cleared as needed by vmx_set_efer(), but attempt
to get the bits set correctly when intializing the control fields.
Setting the value correctly can avoid multiple VMWrites.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/vmx.c | 44 ++++++++++++++++++++++++++++++--------------
 1 file changed, 30 insertions(+), 14 deletions(-)

Comments

Jim Mattson Sept. 19, 2018, 9:57 p.m. UTC | #1 
On Tue, Aug 28, 2018 at 9:04 AM, Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
> VM_ENTRY_IA32E_MODE and VM_{ENTRY,EXIT}_LOAD_IA32_EFER will be
> explicitly set/cleared as needed by vmx_set_efer(), but attempt
> to get the bits set correctly when intializing the control fields.
> Setting the value correctly can avoid multiple VMWrites.
>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/x86/kvm/vmx.c | 44 ++++++++++++++++++++++++++++++--------------
>  1 file changed, 30 insertions(+), 14 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 1fcf374a1475..e58dd3a66abf 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -11896,6 +11896,17 @@ static int nested_vmx_load_cr3(struct kvm_vcpu *vcpu, unsigned long cr3, bool ne
>         return 0;
>  }
>
> +static u64 nested_vmx_calc_efer(struct vcpu_vmx *vmx, struct vmcs12 *vmcs12)
> +{
> +       if (vmx->nested.nested_run_pending &&
> +           (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_EFER))
> +               return vmcs12->guest_ia32_efer;
> +       else if (vmcs12->vm_entry_controls & VM_ENTRY_IA32E_MODE)
> +               return vmx->vcpu.arch.efer | (EFER_LMA | EFER_LME);
> +       else
> +               return vmx->vcpu.arch.efer & ~(EFER_LMA | EFER_LME);
> +}

This makes me a little uncomfortable from the save/restore standpoint,
though it does work given kvm's current behavior. VM-entry controls
should really only be applied when vmx->nested.nested_run_pending is
true. Can this be changed to:

if (!vmx->nested.nested_run_pending) {
        return vmx->vcpu.arch.efer;
} else {
        if (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_EFER)
                return vmcs12->guest_ia32_efer;
        else if (vmcs12->vm_entry_controls & VM_ENTRY_IA32E_MODE)
                return vmx->vcpu.arch.efer | (EFER_LMA | EFER_LME);
        else
                return vmx->vcpu.arch.efer & ~(EFER_LMA | EFER_LME);
}

Reviewed-by: Jim Mattson <jmattson@google.com>
Sean Christopherson Sept. 19, 2018, 10:19 p.m. UTC | #2 
On Wed, Sep 19, 2018 at 02:57:25PM -0700, Jim Mattson wrote:
> On Tue, Aug 28, 2018 at 9:04 AM, Sean Christopherson
> <sean.j.christopherson@intel.com> wrote:
> > VM_ENTRY_IA32E_MODE and VM_{ENTRY,EXIT}_LOAD_IA32_EFER will be
> > explicitly set/cleared as needed by vmx_set_efer(), but attempt
> > to get the bits set correctly when intializing the control fields.
> > Setting the value correctly can avoid multiple VMWrites.
> >
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > ---
> >  arch/x86/kvm/vmx.c | 44 ++++++++++++++++++++++++++++++--------------
> >  1 file changed, 30 insertions(+), 14 deletions(-)
> >
> > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> > index 1fcf374a1475..e58dd3a66abf 100644
> > --- a/arch/x86/kvm/vmx.c
> > +++ b/arch/x86/kvm/vmx.c
> > @@ -11896,6 +11896,17 @@ static int nested_vmx_load_cr3(struct kvm_vcpu *vcpu, unsigned long cr3, bool ne
> >         return 0;
> >  }
> >
> > +static u64 nested_vmx_calc_efer(struct vcpu_vmx *vmx, struct vmcs12 *vmcs12)
> > +{
> > +       if (vmx->nested.nested_run_pending &&
> > +           (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_EFER))
> > +               return vmcs12->guest_ia32_efer;
> > +       else if (vmcs12->vm_entry_controls & VM_ENTRY_IA32E_MODE)
> > +               return vmx->vcpu.arch.efer | (EFER_LMA | EFER_LME);
> > +       else
> > +               return vmx->vcpu.arch.efer & ~(EFER_LMA | EFER_LME);
> > +}
> 
> This makes me a little uncomfortable from the save/restore standpoint,
> though it does work given kvm's current behavior. VM-entry controls
> should really only be applied when vmx->nested.nested_run_pending is
> true. Can this be changed to:

nested_vmx_calc_efer() was copied verbatim from the existing code that
calculates vcpu->arch.efer.  I don't have any objections the proposed
change, but it would need to be done in a separate patch.

> 
> if (!vmx->nested.nested_run_pending) {
>         return vmx->vcpu.arch.efer;
> } else {
>         if (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_EFER)
>                 return vmcs12->guest_ia32_efer;
>         else if (vmcs12->vm_entry_controls & VM_ENTRY_IA32E_MODE)
>                 return vmx->vcpu.arch.efer | (EFER_LMA | EFER_LME);
>         else
>                 return vmx->vcpu.arch.efer & ~(EFER_LMA | EFER_LME);
> }
> 
> Reviewed-by: Jim Mattson <jmattson@google.com>
diff mbox series

Patch

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 1fcf374a1475..e58dd3a66abf 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -11896,6 +11896,17 @@  static int nested_vmx_load_cr3(struct kvm_vcpu *vcpu, unsigned long cr3, bool ne
 	return 0;
 }
 
+static u64 nested_vmx_calc_efer(struct vcpu_vmx *vmx, struct vmcs12 *vmcs12)
+{
+	if (vmx->nested.nested_run_pending &&
+	    (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_EFER))
+		return vmcs12->guest_ia32_efer;
+	else if (vmcs12->vm_entry_controls & VM_ENTRY_IA32E_MODE)
+		return vmx->vcpu.arch.efer | (EFER_LMA | EFER_LME);
+	else
+		return vmx->vcpu.arch.efer & ~(EFER_LMA | EFER_LME);
+}
+
 static void prepare_vmcs02_full(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
@@ -12035,6 +12046,7 @@  static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
 	u32 exec_control, vmcs12_exec_ctrl;
+	u64 guest_efer;
 
 	if (vmx->nested.dirty_vmcs12) {
 		prepare_vmcs02_full(vcpu, vmcs12);
@@ -12190,13 +12202,23 @@  static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
 	 */
 	vm_exit_controls_init(vmx, vmcs_config.vmexit_ctrl);
 
-	/* vmcs12's VM_ENTRY_LOAD_IA32_EFER and VM_ENTRY_IA32E_MODE are
-	 * emulated by vmx_set_efer(), below.
+	/*
+	 * vmcs12's VM_{ENTRY,EXIT}_LOAD_IA32_EFER and VM_ENTRY_IA32E_MODE
+	 * are emulated by vmx_set_efer(), below, but speculate on the
+	 * related bits (if supported by the CPU) in the hope that we can
+	 * avoid VMWrites during vmx_set_efer().
 	 */
-	vm_entry_controls_init(vmx, 
-		(vmcs12->vm_entry_controls & ~VM_ENTRY_LOAD_IA32_EFER &
-			~VM_ENTRY_IA32E_MODE) |
-		(vmcs_config.vmentry_ctrl & ~VM_ENTRY_IA32E_MODE));
+	exec_control = (vmcs12->vm_entry_controls | vmcs_config.vmentry_ctrl) &
+			~VM_ENTRY_IA32E_MODE & ~VM_ENTRY_LOAD_IA32_EFER;
+
+	guest_efer = nested_vmx_calc_efer(vmx, vmcs12);
+	if (cpu_has_load_ia32_efer) {
+		if (guest_efer & EFER_LMA)
+			exec_control |= VM_ENTRY_IA32E_MODE;
+		if (guest_efer != host_efer)
+			exec_control |= VM_ENTRY_LOAD_IA32_EFER;
+	}
+	vm_entry_controls_init(vmx, exec_control);
 
 	if (vmx->nested.nested_run_pending &&
 	    (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_PAT)) {
@@ -12262,14 +12284,8 @@  static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
 	vmx_set_cr4(vcpu, vmcs12->guest_cr4);
 	vmcs_writel(CR4_READ_SHADOW, nested_read_cr4(vmcs12));
 
-	if (vmx->nested.nested_run_pending &&
-	    (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_EFER))
-		vcpu->arch.efer = vmcs12->guest_ia32_efer;
-	else if (vmcs12->vm_entry_controls & VM_ENTRY_IA32E_MODE)
-		vcpu->arch.efer |= (EFER_LMA | EFER_LME);
-	else
-		vcpu->arch.efer &= ~(EFER_LMA | EFER_LME);
-	/* Note: modifies VM_ENTRY/EXIT_CONTROLS and GUEST/HOST_IA32_EFER */
+	vcpu->arch.efer = guest_efer;
+	/* Note: may modify VM_ENTRY/EXIT_CONTROLS and GUEST/HOST_IA32_EFER */
 	vmx_set_efer(vcpu, vcpu->arch.efer);
 
 	/*