[v2,09/18] KVM: nVMX: assimilate nested_vmx_entry_failure() into nested_vmx_enter_non_root_mode()
diff mbox series

Message ID 20180828160459.14093-10-sean.j.christopherson@intel.com
State New
Headers show
Series
  • KVM: nVMX: add option to perform early consistency checks via H/W
Related show

Commit Message

Sean Christopherson Aug. 28, 2018, 4:04 p.m. UTC
Handling consistency check VMExits in nested_vmx_enter_non_root_mode()
consolidates all relevant code into a single location, and removing
nested_vmx_entry_failure() eliminates a confusing function name and
label.  For a VMEntry, "fail" and its derivatives has a very specific
meaning due to the different behavior of a VMEnter VMFail versus
VMExit, i.e. a more appropriate name for nested_vmx_entry_failure()
would have been nested_vmx_entry_consistency_check_vmexit().

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

Comments

Jim Mattson Sept. 20, 2018, 5:45 p.m. UTC | #1
On Tue, Aug 28, 2018 at 9:04 AM, Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
> Handling consistency check VMExits in nested_vmx_enter_non_root_mode()
> consolidates all relevant code into a single location, and removing
> nested_vmx_entry_failure() eliminates a confusing function name and
> label.  For a VMEntry, "fail" and its derivatives has a very specific
> meaning due to the different behavior of a VMEnter VMFail versus
> VMExit, i.e. a more appropriate name for nested_vmx_entry_failure()
> would have been nested_vmx_entry_consistency_check_vmexit().

Though there is definitely some confusion regarding VMFail scenarios
versus "VM-entry failure" scenarios, "VM-entry failure" is a real
thing, and this is the terminology used in the SDM. Specifically,
Table 24-14 Format of Exit Reason describes bit 31 as "VM-entry
failure (0 = true VM exit; 1 = VM-entry failure)." I think it's better
to use the vendor's terminology than to make up terminology of our
own. These aren't "consistency check VM-exits;" they are "VM-entry
failure VM-exits."

> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/x86/kvm/vmx.c | 90 ++++++++++++++++++++--------------------------
>  1 file changed, 39 insertions(+), 51 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 43e87a2e172e..cb8df73e9b49 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -2056,9 +2056,6 @@ static inline bool is_nmi(u32 intr_info)
>  static void nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 exit_reason,
>                               u32 exit_intr_info,
>                               unsigned long exit_qualification);
> -static void nested_vmx_entry_failure(struct kvm_vcpu *vcpu,
> -                       struct vmcs12 *vmcs12,
> -                       u32 reason, unsigned long qualification);
>
>  static int __find_msr_index(struct vcpu_vmx *vmx, u32 msr)
>  {
> @@ -12546,25 +12543,23 @@ static int check_vmentry_postreqs(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
>         return 0;
>  }
>
> +static void load_vmcs12_host_state(struct kvm_vcpu *vcpu,
> +                                  struct vmcs12 *vmcs12);
>  /*
>   * If exit_qual is NULL, this is being called from state restore (either RSM
>   * or KVM_SET_NESTED_STATE).  Otherwise it's called from vmlaunch/vmresume.
>   */
> -static int nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu, u32 *exit_qual)
> +static int nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu,
> +                                         bool from_vmentry)
>  {
>         struct vcpu_vmx *vmx = to_vmx(vcpu);
>         struct vmcs12 *vmcs12 = get_vmcs12(vcpu);
> -       bool from_vmentry = !!exit_qual;
> -       u32 dummy_exit_qual;
> -       int r;
> +       u32 exit_reason = EXIT_REASON_INVALID_STATE;
> +       u32 exit_qual;
>
>         if (from_vmentry) {
> -               r = check_vmentry_postreqs(vcpu, vmcs12, exit_qual);
> -               if (r) {
> -                       nested_vmx_entry_failure(vcpu, vmcs12,
> -                               EXIT_REASON_INVALID_STATE, *exit_qual);
> -                       return 1;
> -               }
> +               if (check_vmentry_postreqs(vcpu, vmcs12, &exit_qual))
> +                       goto consistency_check_vmexit;
>         }
>
>         enter_guest_mode(vcpu);
> @@ -12578,18 +12573,17 @@ static int nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu, u32 *exit_qual)
>         if (vmcs12->cpu_based_vm_exec_control & CPU_BASED_USE_TSC_OFFSETING)
>                 vcpu->arch.tsc_offset += vmcs12->tsc_offset;
>
> -       r = EXIT_REASON_INVALID_STATE;
> -       if (prepare_vmcs02(vcpu, vmcs12, from_vmentry ? exit_qual : &dummy_exit_qual))
> +       if (prepare_vmcs02(vcpu, vmcs12, &exit_qual))
>                 goto fail;
>
>         if (from_vmentry) {
>                 nested_get_vmcs12_pages(vcpu);
>
> -               r = EXIT_REASON_MSR_LOAD_FAIL;
> -               *exit_qual = nested_vmx_load_msr(vcpu,
> -                                                vmcs12->vm_entry_msr_load_addr,
> -                                                vmcs12->vm_entry_msr_load_count);
> -               if (*exit_qual)
> +               exit_reason = EXIT_REASON_MSR_LOAD_FAIL;
> +               exit_qual = nested_vmx_load_msr(vcpu,
> +                                               vmcs12->vm_entry_msr_load_addr,
> +                                               vmcs12->vm_entry_msr_load_count);
> +               if (exit_qual)
>                         goto fail;
>         } else {
>                 /*
> @@ -12615,7 +12609,28 @@ static int nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu, u32 *exit_qual)
>                 vcpu->arch.tsc_offset -= vmcs12->tsc_offset;
>         leave_guest_mode(vcpu);
>         vmx_switch_vmcs(vcpu, &vmx->vmcs01);
> -       return r;
> +
> +       /*
> +        * A consistency check VMExit during L1's VMEnter to L2 is a subset
> +        * of a normal VMexit, as explained in 23.7 "VM-entry failures during
> +        * or after loading guest state" (this also lists the acceptable exit-
> +        * reason and exit-qualification parameters).
> +        */
> +consistency_check_vmexit:

Obviously, I prefer vmentry_failure_vmexit, or some other label based
on the terminology used in the SDM.

> +       vm_entry_controls_reset_shadow(vmx);
> +       vm_exit_controls_reset_shadow(vmx);
> +       vmx_segment_cache_clear(vmx);

Move to vmx_switch_vmcs.

> +       if (!from_vmentry)
> +               return 1;
> +
> +       load_vmcs12_host_state(vcpu, vmcs12);
> +       vmcs12->vm_exit_reason = exit_reason | VMX_EXIT_REASONS_FAILED_VMENTRY;
> +       vmcs12->exit_qualification = exit_qual;
> +       nested_vmx_succeed(vcpu);

> +       if (enable_shadow_vmcs)
> +               vmx->nested.sync_shadow_vmcs = true;

And does this belong in vmx_switch_vmcs as well?
Sean Christopherson Sept. 20, 2018, 6:08 p.m. UTC | #2
On Thu, Sep 20, 2018 at 10:45:58AM -0700, Jim Mattson wrote:
> On Tue, Aug 28, 2018 at 9:04 AM, Sean Christopherson
> <sean.j.christopherson@intel.com> wrote:
> > Handling consistency check VMExits in nested_vmx_enter_non_root_mode()
> > consolidates all relevant code into a single location, and removing
> > nested_vmx_entry_failure() eliminates a confusing function name and
> > label.  For a VMEntry, "fail" and its derivatives has a very specific
> > meaning due to the different behavior of a VMEnter VMFail versus
> > VMExit, i.e. a more appropriate name for nested_vmx_entry_failure()
> > would have been nested_vmx_entry_consistency_check_vmexit().
> 
> Though there is definitely some confusion regarding VMFail scenarios
> versus "VM-entry failure" scenarios, "VM-entry failure" is a real
> thing, and this is the terminology used in the SDM. Specifically,
> Table 24-14 Format of Exit Reason describes bit 31 as "VM-entry
> failure (0 = true VM exit; 1 = VM-entry failure)." I think it's better
> to use the vendor's terminology than to make up terminology of our
> own. These aren't "consistency check VM-exits;" they are "VM-entry
> failure VM-exits."

Darn external documentation :)  I'll rewrite the label, comment and
changelog to match the SDM.

> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > ---
> >  arch/x86/kvm/vmx.c | 90 ++++++++++++++++++++--------------------------
> >  1 file changed, 39 insertions(+), 51 deletions(-)
> >
> > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> > index 43e87a2e172e..cb8df73e9b49 100644
> > --- a/arch/x86/kvm/vmx.c
> > +++ b/arch/x86/kvm/vmx.c
> > @@ -2056,9 +2056,6 @@ static inline bool is_nmi(u32 intr_info)
> >  static void nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 exit_reason,
> >                               u32 exit_intr_info,
> >                               unsigned long exit_qualification);
> > -static void nested_vmx_entry_failure(struct kvm_vcpu *vcpu,
> > -                       struct vmcs12 *vmcs12,
> > -                       u32 reason, unsigned long qualification);
> >
> >  static int __find_msr_index(struct vcpu_vmx *vmx, u32 msr)
> >  {
> > @@ -12546,25 +12543,23 @@ static int check_vmentry_postreqs(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
> >         return 0;
> >  }
> >
> > +static void load_vmcs12_host_state(struct kvm_vcpu *vcpu,
> > +                                  struct vmcs12 *vmcs12);
> >  /*
> >   * If exit_qual is NULL, this is being called from state restore (either RSM
> >   * or KVM_SET_NESTED_STATE).  Otherwise it's called from vmlaunch/vmresume.
> >   */
> > -static int nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu, u32 *exit_qual)
> > +static int nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu,
> > +                                         bool from_vmentry)
> >  {
> >         struct vcpu_vmx *vmx = to_vmx(vcpu);
> >         struct vmcs12 *vmcs12 = get_vmcs12(vcpu);
> > -       bool from_vmentry = !!exit_qual;
> > -       u32 dummy_exit_qual;
> > -       int r;
> > +       u32 exit_reason = EXIT_REASON_INVALID_STATE;
> > +       u32 exit_qual;
> >
> >         if (from_vmentry) {
> > -               r = check_vmentry_postreqs(vcpu, vmcs12, exit_qual);
> > -               if (r) {
> > -                       nested_vmx_entry_failure(vcpu, vmcs12,
> > -                               EXIT_REASON_INVALID_STATE, *exit_qual);
> > -                       return 1;
> > -               }
> > +               if (check_vmentry_postreqs(vcpu, vmcs12, &exit_qual))
> > +                       goto consistency_check_vmexit;
> >         }
> >
> >         enter_guest_mode(vcpu);
> > @@ -12578,18 +12573,17 @@ static int nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu, u32 *exit_qual)
> >         if (vmcs12->cpu_based_vm_exec_control & CPU_BASED_USE_TSC_OFFSETING)
> >                 vcpu->arch.tsc_offset += vmcs12->tsc_offset;
> >
> > -       r = EXIT_REASON_INVALID_STATE;
> > -       if (prepare_vmcs02(vcpu, vmcs12, from_vmentry ? exit_qual : &dummy_exit_qual))
> > +       if (prepare_vmcs02(vcpu, vmcs12, &exit_qual))
> >                 goto fail;
> >
> >         if (from_vmentry) {
> >                 nested_get_vmcs12_pages(vcpu);
> >
> > -               r = EXIT_REASON_MSR_LOAD_FAIL;
> > -               *exit_qual = nested_vmx_load_msr(vcpu,
> > -                                                vmcs12->vm_entry_msr_load_addr,
> > -                                                vmcs12->vm_entry_msr_load_count);
> > -               if (*exit_qual)
> > +               exit_reason = EXIT_REASON_MSR_LOAD_FAIL;
> > +               exit_qual = nested_vmx_load_msr(vcpu,
> > +                                               vmcs12->vm_entry_msr_load_addr,
> > +                                               vmcs12->vm_entry_msr_load_count);
> > +               if (exit_qual)
> >                         goto fail;
> >         } else {
> >                 /*
> > @@ -12615,7 +12609,28 @@ static int nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu, u32 *exit_qual)
> >                 vcpu->arch.tsc_offset -= vmcs12->tsc_offset;
> >         leave_guest_mode(vcpu);
> >         vmx_switch_vmcs(vcpu, &vmx->vmcs01);
> > -       return r;
> > +
> > +       /*
> > +        * A consistency check VMExit during L1's VMEnter to L2 is a subset
> > +        * of a normal VMexit, as explained in 23.7 "VM-entry failures during
> > +        * or after loading guest state" (this also lists the acceptable exit-
> > +        * reason and exit-qualification parameters).
> > +        */
> > +consistency_check_vmexit:
> 
> Obviously, I prefer vmentry_failure_vmexit, or some other label based
> on the terminology used in the SDM.

Works for me.

> > +       vm_entry_controls_reset_shadow(vmx);
> > +       vm_exit_controls_reset_shadow(vmx);
> > +       vmx_segment_cache_clear(vmx);
> 
> Move to vmx_switch_vmcs.
> 
> > +       if (!from_vmentry)
> > +               return 1;
> > +
> > +       load_vmcs12_host_state(vcpu, vmcs12);
> > +       vmcs12->vm_exit_reason = exit_reason | VMX_EXIT_REASONS_FAILED_VMENTRY;
> > +       vmcs12->exit_qualification = exit_qual;
> > +       nested_vmx_succeed(vcpu);
> 
> > +       if (enable_shadow_vmcs)
> > +               vmx->nested.sync_shadow_vmcs = true;
> 
> And does this belong in vmx_switch_vmcs as well?

I don't think so, that would cause sync_shadow_vmcs to be incorrectly
set in the nested_vmx_vmexit() flow for a late VMFail.

Patch
diff mbox series

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 43e87a2e172e..cb8df73e9b49 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -2056,9 +2056,6 @@  static inline bool is_nmi(u32 intr_info)
 static void nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 exit_reason,
 			      u32 exit_intr_info,
 			      unsigned long exit_qualification);
-static void nested_vmx_entry_failure(struct kvm_vcpu *vcpu,
-			struct vmcs12 *vmcs12,
-			u32 reason, unsigned long qualification);
 
 static int __find_msr_index(struct vcpu_vmx *vmx, u32 msr)
 {
@@ -12546,25 +12543,23 @@  static int check_vmentry_postreqs(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
 	return 0;
 }
 
+static void load_vmcs12_host_state(struct kvm_vcpu *vcpu,
+				   struct vmcs12 *vmcs12);
 /*
  * If exit_qual is NULL, this is being called from state restore (either RSM
  * or KVM_SET_NESTED_STATE).  Otherwise it's called from vmlaunch/vmresume.
  */
-static int nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu, u32 *exit_qual)
+static int nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu,
+					  bool from_vmentry)
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
 	struct vmcs12 *vmcs12 = get_vmcs12(vcpu);
-	bool from_vmentry = !!exit_qual;
-	u32 dummy_exit_qual;
-	int r;
+	u32 exit_reason = EXIT_REASON_INVALID_STATE;
+	u32 exit_qual;
 
 	if (from_vmentry) {
-		r = check_vmentry_postreqs(vcpu, vmcs12, exit_qual);
-		if (r) {
-			nested_vmx_entry_failure(vcpu, vmcs12,
-				EXIT_REASON_INVALID_STATE, *exit_qual);
-			return 1;
-		}
+		if (check_vmentry_postreqs(vcpu, vmcs12, &exit_qual))
+			goto consistency_check_vmexit;
 	}
 
 	enter_guest_mode(vcpu);
@@ -12578,18 +12573,17 @@  static int nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu, u32 *exit_qual)
 	if (vmcs12->cpu_based_vm_exec_control & CPU_BASED_USE_TSC_OFFSETING)
 		vcpu->arch.tsc_offset += vmcs12->tsc_offset;
 
-	r = EXIT_REASON_INVALID_STATE;
-	if (prepare_vmcs02(vcpu, vmcs12, from_vmentry ? exit_qual : &dummy_exit_qual))
+	if (prepare_vmcs02(vcpu, vmcs12, &exit_qual))
 		goto fail;
 
 	if (from_vmentry) {
 		nested_get_vmcs12_pages(vcpu);
 
-		r = EXIT_REASON_MSR_LOAD_FAIL;
-		*exit_qual = nested_vmx_load_msr(vcpu,
-	     					 vmcs12->vm_entry_msr_load_addr,
-					      	 vmcs12->vm_entry_msr_load_count);
-		if (*exit_qual)
+		exit_reason = EXIT_REASON_MSR_LOAD_FAIL;
+		exit_qual = nested_vmx_load_msr(vcpu,
+						vmcs12->vm_entry_msr_load_addr,
+						vmcs12->vm_entry_msr_load_count);
+		if (exit_qual)
 			goto fail;
 	} else {
 		/*
@@ -12615,7 +12609,28 @@  static int nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu, u32 *exit_qual)
 		vcpu->arch.tsc_offset -= vmcs12->tsc_offset;
 	leave_guest_mode(vcpu);
 	vmx_switch_vmcs(vcpu, &vmx->vmcs01);
-	return r;
+
+	/*
+	 * A consistency check VMExit during L1's VMEnter to L2 is a subset
+	 * of a normal VMexit, as explained in 23.7 "VM-entry failures during
+	 * or after loading guest state" (this also lists the acceptable exit-
+	 * reason and exit-qualification parameters).
+	 */
+consistency_check_vmexit:
+	vm_entry_controls_reset_shadow(vmx);
+	vm_exit_controls_reset_shadow(vmx);
+	vmx_segment_cache_clear(vmx);
+
+	if (!from_vmentry)
+		return 1;
+
+	load_vmcs12_host_state(vcpu, vmcs12);
+	vmcs12->vm_exit_reason = exit_reason | VMX_EXIT_REASONS_FAILED_VMENTRY;
+	vmcs12->exit_qualification = exit_qual;
+	nested_vmx_succeed(vcpu);
+	if (enable_shadow_vmcs)
+		vmx->nested.sync_shadow_vmcs = true;
+	return 1;
 }
 
 /*
@@ -12627,7 +12642,6 @@  static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch)
 	struct vmcs12 *vmcs12;
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
 	u32 interrupt_shadow = vmx_get_interrupt_shadow(vcpu);
-	u32 exit_qual;
 	int ret;
 
 	if (!nested_vmx_check_permission(vcpu))
@@ -12696,9 +12710,8 @@  static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch)
 	 */
 
 	vmx->nested.nested_run_pending = 1;
-	ret = nested_vmx_enter_non_root_mode(vcpu, &exit_qual);
+	ret = nested_vmx_enter_non_root_mode(vcpu, true);
 	if (ret) {
-		nested_vmx_entry_failure(vcpu, vmcs12, ret, exit_qual);
 		vmx->nested.nested_run_pending = 0;
 		return 1;
 	}
@@ -13364,31 +13377,6 @@  static void vmx_leave_nested(struct kvm_vcpu *vcpu)
 	free_nested(to_vmx(vcpu));
 }
 
-/*
- * L1's failure to enter L2 is a subset of a normal exit, as explained in
- * 23.7 "VM-entry failures during or after loading guest state" (this also
- * lists the acceptable exit-reason and exit-qualification parameters).
- * It should only be called before L2 actually succeeded to run, and when
- * vmcs01 is current (it doesn't leave_guest_mode() or switch vmcss).
- */
-static void nested_vmx_entry_failure(struct kvm_vcpu *vcpu,
-			struct vmcs12 *vmcs12,
-			u32 reason, unsigned long qualification)
-{
-	struct vcpu_vmx *vmx = to_vmx(vcpu);
-
-	vm_entry_controls_reset_shadow(vmx);
-	vm_exit_controls_reset_shadow(vmx);
-	vmx_segment_cache_clear(vmx);
-
-	load_vmcs12_host_state(vcpu, vmcs12);
-	vmcs12->vm_exit_reason = reason | VMX_EXIT_REASONS_FAILED_VMENTRY;
-	vmcs12->exit_qualification = qualification;
-	nested_vmx_succeed(vcpu);
-	if (enable_shadow_vmcs)
-		vmx->nested.sync_shadow_vmcs = true;
-}
-
 static int vmx_check_intercept(struct kvm_vcpu *vcpu,
 			       struct x86_instruction_info *info,
 			       enum x86_intercept_stage stage)
@@ -13818,7 +13806,7 @@  static int vmx_pre_leave_smm(struct kvm_vcpu *vcpu, u64 smbase)
 
 	if (vmx->nested.smm.guest_mode) {
 		vcpu->arch.hflags &= ~HF_SMM_MASK;
-		ret = nested_vmx_enter_non_root_mode(vcpu, NULL);
+		ret = nested_vmx_enter_non_root_mode(vcpu, false);
 		vcpu->arch.hflags |= HF_SMM_MASK;
 		if (ret)
 			return ret;
@@ -14019,7 +14007,7 @@  static int vmx_set_nested_state(struct kvm_vcpu *vcpu,
 		vmx->nested.nested_run_pending = 1;
 
 	vmx->nested.dirty_vmcs12 = true;
-	ret = nested_vmx_enter_non_root_mode(vcpu, NULL);
+	ret = nested_vmx_enter_non_root_mode(vcpu, false);
 	if (ret)
 		return -EINVAL;