[PATCH 0/5] KVM: nVMX: Skip vmentry checks that are necessary only if VMCS12 is dirty

[PATCH 0/5] KVM: nVMX: Skip vmentry checks that are necessary only if VMCS12 is dirty

[Krish Sadhukhan]

The following functions,

	nested_vmx_check_controls
	nested_vmx_check_host_state
	nested_vmx_check_guest_state

do a number of vmentry checks for VMCS12. However, not all of these checks need
to be executed on every vmentry. This patchset makes some of these vmentry
checks optional based on the state of VMCS12 in that if VMCS12 is dirty, only
then the checks will be executed. This will reduce performance impact on
vmentry of nested guests.


[PATCH 1/5] KVM: nVMX: Skip VM-Execution Control vmentry checks that are
[PATCH 2/5] KVM: nVMX: Skip VM-Exit Control vmentry checks that are
[PATCH 3/5] KVM: nVMX: Skip VM-Entry Control checks that are necessary
[PATCH 4/5] KVM: nVMX: Skip Host State Area vmentry checks that are
[PATCH 5/5] KVM: nVMX: Skip Guest State Area vmentry checks that are

 arch/x86/kvm/vmx/nested.c | 149 ++++++++++++++++++++++++++++++++++------------
 1 file changed, 111 insertions(+), 38 deletions(-)

Krish Sadhukhan (5):
      nVMX: Skip VM-Execution Control vmentry checks that are necessary only if VMCS12 is dirty
      nVMX: Skip VM-Exit Control vmentry checks that are necessary only if VMCS12 is dirty
      nVMX: Skip VM-Entry Control checks that are necessary only if VMCS12 is dirty
      nVMX: Skip Host State Area vmentry checks that are necessary only if VMCS12 is dirty
      nVMX: Skip Guest State Area vmentry checks that are necessary only if VMCS12 is dirty

[PATCH 1/5] KVM: nVMX: Skip VM-Execution Control vmentry checks that are necessary only if VMCS12 is dirty

[Krish Sadhukhan]

 ..so that every nested vmentry is not slowed down by those checks.

Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
---
 arch/x86/kvm/vmx/nested.c | 57 +++++++++++++++++++++++++--------------
 1 file changed, 37 insertions(+), 20 deletions(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 856a83aa42f5..b0b59c78b3e8 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -2429,6 +2429,38 @@ static bool valid_ept_address(struct kvm_vcpu *vcpu, u64 address)
 	return true;
 }
 
+static int nested_check_vm_execution_controls_full(struct kvm_vcpu *vcpu,
+						   struct vmcs12 *vmcs12)
+{
+	struct vcpu_vmx *vmx = to_vmx(vcpu);
+
+	if (nested_vmx_check_msr_bitmap_controls(vcpu, vmcs12) ||
+	    nested_vmx_check_pml_controls(vcpu, vmcs12))
+		return -EINVAL;
+
+	if (nested_cpu_has_ept(vmcs12) &&
+	    !valid_ept_address(vcpu, vmcs12->ept_pointer))
+		return -EINVAL;
+
+	if (nested_cpu_has_vmfunc(vmcs12)) {
+		if (vmcs12->vm_function_control &
+		    ~vmx->nested.msrs.vmfunc_controls)
+			return -EINVAL;
+
+		if (nested_cpu_has_eptp_switching(vmcs12)) {
+			if (!nested_cpu_has_ept(vmcs12) ||
+			    !page_address_valid(vcpu,
+			     vmcs12->eptp_list_address))
+				return -EINVAL;
+		}
+	}
+
+	if (nested_cpu_has_vpid(vmcs12) && !vmcs12->virtual_processor_id)
+		return -EINVAL;
+
+	return 0;
+}
+
 /*
  * Checks related to VM-Execution Control Fields
  */
@@ -2437,6 +2469,10 @@ static int nested_check_vm_execution_controls(struct kvm_vcpu *vcpu,
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
 
+	if ((vmx->nested.dirty_vmcs12) &&
+	    nested_check_vm_execution_controls_full(vcpu, vmcs12))
+		return -EINVAL;
+
 	if (!vmx_control_verify(vmcs12->pin_based_vm_exec_control,
 				vmx->nested.msrs.pinbased_ctls_low,
 				vmx->nested.msrs.pinbased_ctls_high) ||
@@ -2453,38 +2489,19 @@ static int nested_check_vm_execution_controls(struct kvm_vcpu *vcpu,
 
 	if (vmcs12->cr3_target_count > nested_cpu_vmx_misc_cr3_count(vcpu) ||
 	    nested_vmx_check_io_bitmap_controls(vcpu, vmcs12) ||
-	    nested_vmx_check_msr_bitmap_controls(vcpu, vmcs12) ||
 	    nested_vmx_check_tpr_shadow_controls(vcpu, vmcs12) ||
 	    nested_vmx_check_apic_access_controls(vcpu, vmcs12) ||
 	    nested_vmx_check_apicv_controls(vcpu, vmcs12) ||
 	    nested_vmx_check_nmi_controls(vmcs12) ||
-	    nested_vmx_check_pml_controls(vcpu, vmcs12) ||
 	    nested_vmx_check_unrestricted_guest_controls(vcpu, vmcs12) ||
 	    nested_vmx_check_mode_based_ept_exec_controls(vcpu, vmcs12) ||
-	    nested_vmx_check_shadow_vmcs_controls(vcpu, vmcs12) ||
-	    (nested_cpu_has_vpid(vmcs12) && !vmcs12->virtual_processor_id))
+	    nested_vmx_check_shadow_vmcs_controls(vcpu, vmcs12))
 		return -EINVAL;
 
 	if (!nested_cpu_has_preemption_timer(vmcs12) &&
 	    nested_cpu_has_save_preemption_timer(vmcs12))
 		return -EINVAL;
 
-	if (nested_cpu_has_ept(vmcs12) &&
-	    !valid_ept_address(vcpu, vmcs12->ept_pointer))
-		return -EINVAL;
-
-	if (nested_cpu_has_vmfunc(vmcs12)) {
-		if (vmcs12->vm_function_control &
-		    ~vmx->nested.msrs.vmfunc_controls)
-			return -EINVAL;
-
-		if (nested_cpu_has_eptp_switching(vmcs12)) {
-			if (!nested_cpu_has_ept(vmcs12) ||
-			    !page_address_valid(vcpu, vmcs12->eptp_list_address))
-				return -EINVAL;
-		}
-	}
-
 	return 0;
 }
 
-- 
2.20.1

[PATCH 2/5] KVM: nVMX: Skip VM-Exit Control vmentry checks that are necessary only if VMCS12 is dirty

[Krish Sadhukhan]

 ..so that every nested vmentry is not slowed down by those checks.

Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
---
 arch/x86/kvm/vmx/nested.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index b0b59c78b3e8..056eba497730 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -2505,6 +2505,15 @@ static int nested_check_vm_execution_controls(struct kvm_vcpu *vcpu,
 	return 0;
 }
 
+static int nested_check_vm_exit_controls_full(struct kvm_vcpu *vcpu,
+					      struct vmcs12 *vmcs12)
+{
+	if (nested_vmx_check_exit_msr_switch_controls(vcpu, vmcs12))
+		return -EINVAL;
+
+	return 0;
+}
+
 /*
  * Checks related to VM-Exit Control Fields
  */
@@ -2515,8 +2524,11 @@ static int nested_check_vm_exit_controls(struct kvm_vcpu *vcpu,
 
 	if (!vmx_control_verify(vmcs12->vm_exit_controls,
 				vmx->nested.msrs.exit_ctls_low,
-				vmx->nested.msrs.exit_ctls_high) ||
-	    nested_vmx_check_exit_msr_switch_controls(vcpu, vmcs12))
+				vmx->nested.msrs.exit_ctls_high))
+		return -EINVAL;
+
+	if ((vmx->nested.dirty_vmcs12) &&
+	    nested_check_vm_exit_controls_full(vcpu, vmcs12))
 		return -EINVAL;
 
 	return 0;
-- 
2.20.1

[PATCH 3/5] KVM: nVMX: Skip VM-Entry Control checks that are necessary only if VMCS12 is dirty

[Krish Sadhukhan]

 ..so that every nested vmentry is not slowed down by those checks.

Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
---
 arch/x86/kvm/vmx/nested.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 056eba497730..ffeeeb5ff520 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -2534,6 +2534,15 @@ static int nested_check_vm_exit_controls(struct kvm_vcpu *vcpu,
 	return 0;
 }
 
+static int nested_check_vm_entry_controls_full(struct kvm_vcpu *vcpu,
+					       struct vmcs12 *vmcs12)
+{
+	if (nested_vmx_check_entry_msr_switch_controls(vcpu, vmcs12))
+		return -EINVAL;
+
+	return 0;
+}
+
 /*
  * Checks related to VM-Entry Control Fields
  */
@@ -2603,7 +2612,8 @@ static int nested_check_vm_entry_controls(struct kvm_vcpu *vcpu,
 		}
 	}
 
-	if (nested_vmx_check_entry_msr_switch_controls(vcpu, vmcs12))
+	if ((vmx->nested.dirty_vmcs12) &&
+	    nested_check_vm_entry_controls_full(vcpu, vmcs12))
 		return -EINVAL;
 
 	return 0;
-- 
2.20.1

[PATCH 4/5] KVM: nVMX: Skip Host State Area vmentry checks that are necessary only if VMCS12 is dirty

[Krish Sadhukhan]

  ..so that every nested vmentry is not slowed down by those checks.

Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
---
 arch/x86/kvm/vmx/nested.c | 48 ++++++++++++++++++++++++++++-----------
 1 file changed, 35 insertions(+), 13 deletions(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index ffeeeb5ff520..b610f389a01b 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -2630,18 +2630,16 @@ static int nested_vmx_check_controls(struct kvm_vcpu *vcpu,
 	return 0;
 }
 
-static int nested_vmx_check_host_state(struct kvm_vcpu *vcpu,
-				       struct vmcs12 *vmcs12)
+static int nested_vmx_check_host_state_full(struct kvm_vcpu *vcpu,
+					    struct vmcs12 *vmcs12)
 {
 	bool ia32e;
 
-	if (!nested_host_cr0_valid(vcpu, vmcs12->host_cr0) ||
-	    !nested_host_cr4_valid(vcpu, vmcs12->host_cr4) ||
+	if (!nested_host_cr4_valid(vcpu, vmcs12->host_cr4) ||
 	    !nested_cr3_valid(vcpu, vmcs12->host_cr3))
 		return -EINVAL;
 
-	if (is_noncanonical_address(vmcs12->host_ia32_sysenter_esp, vcpu) ||
-	    is_noncanonical_address(vmcs12->host_ia32_sysenter_eip, vcpu))
+	if (is_noncanonical_address(vmcs12->host_ia32_sysenter_eip, vcpu))
 		return -EINVAL;
 
 	if ((vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_PAT) &&
@@ -2655,8 +2653,6 @@ static int nested_vmx_check_host_state(struct kvm_vcpu *vcpu,
 	    vmcs12->host_ss_selector & (SEGMENT_RPL_MASK | SEGMENT_TI_MASK) ||
 	    vmcs12->host_ds_selector & (SEGMENT_RPL_MASK | SEGMENT_TI_MASK) ||
 	    vmcs12->host_es_selector & (SEGMENT_RPL_MASK | SEGMENT_TI_MASK) ||
-	    vmcs12->host_fs_selector & (SEGMENT_RPL_MASK | SEGMENT_TI_MASK) ||
-	    vmcs12->host_gs_selector & (SEGMENT_RPL_MASK | SEGMENT_TI_MASK) ||
 	    vmcs12->host_tr_selector & (SEGMENT_RPL_MASK | SEGMENT_TI_MASK) ||
 	    vmcs12->host_cs_selector == 0 ||
 	    vmcs12->host_tr_selector == 0 ||
@@ -2664,11 +2660,7 @@ static int nested_vmx_check_host_state(struct kvm_vcpu *vcpu,
 		return -EINVAL;
 
 #ifdef CONFIG_X86_64
-	if (is_noncanonical_address(vmcs12->host_fs_base, vcpu) ||
-	    is_noncanonical_address(vmcs12->host_gs_base, vcpu) ||
-	    is_noncanonical_address(vmcs12->host_gdtr_base, vcpu) ||
-	    is_noncanonical_address(vmcs12->host_idtr_base, vcpu) ||
-	    is_noncanonical_address(vmcs12->host_tr_base, vcpu))
+	if (is_noncanonical_address(vmcs12->host_idtr_base, vcpu))
 		return -EINVAL;
 #endif
 
@@ -2688,6 +2680,36 @@ static int nested_vmx_check_host_state(struct kvm_vcpu *vcpu,
 	return 0;
 }
 
+static int nested_vmx_check_host_state(struct kvm_vcpu *vcpu,
+				       struct vmcs12 *vmcs12)
+{
+	struct vcpu_vmx *vmx = to_vmx(vcpu);
+
+	if ((vmx->nested.dirty_vmcs12) &&
+	    nested_vmx_check_host_state_full(vcpu, vmcs12))
+		return -EINVAL;
+
+	if (!nested_host_cr0_valid(vcpu, vmcs12->host_cr0))
+		return -EINVAL;
+
+	if (is_noncanonical_address(vmcs12->host_ia32_sysenter_esp, vcpu))
+		return -EINVAL;
+
+	if (vmcs12->host_fs_selector & (SEGMENT_RPL_MASK | SEGMENT_TI_MASK) ||
+	    vmcs12->host_gs_selector & (SEGMENT_RPL_MASK | SEGMENT_TI_MASK))
+		return -EINVAL;
+
+#ifdef CONFIG_X86_64
+	if (is_noncanonical_address(vmcs12->host_fs_base, vcpu) ||
+	    is_noncanonical_address(vmcs12->host_gs_base, vcpu) ||
+	    is_noncanonical_address(vmcs12->host_gdtr_base, vcpu) ||
+	    is_noncanonical_address(vmcs12->host_tr_base, vcpu))
+		return -EINVAL;
+#endif
+
+	return 0;
+}
+
 static int nested_vmx_check_vmcs_link_ptr(struct kvm_vcpu *vcpu,
 					  struct vmcs12 *vmcs12)
 {
-- 
2.20.1

[PATCH 5/5] KVM: nVMX: Skip Guest State Area vmentry checks that are necessary only if VMCS12 is dirty

[Krish Sadhukhan]

  ..so that every nested vmentry is not slowed down by those checks.

Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
---
 arch/x86/kvm/vmx/nested.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index b610f389a01b..095923b1d765 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -2748,10 +2748,23 @@ static int nested_check_guest_non_reg_state(struct vmcs12 *vmcs12)
 	return 0;
 }
 
+static int nested_vmx_check_guest_state_full(struct kvm_vcpu *vcpu,
+					     struct vmcs12 *vmcs12,
+					     u32 *exit_qual)
+{
+	if ((vmcs12->vm_entry_controls & VM_ENTRY_LOAD_BNDCFGS) &&
+	    (is_noncanonical_address(vmcs12->guest_bndcfgs & PAGE_MASK, vcpu) ||
+	     (vmcs12->guest_bndcfgs & MSR_IA32_BNDCFGS_RSVD)))
+		return -EINVAL;
+
+	return 0;
+}
+
 static int nested_vmx_check_guest_state(struct kvm_vcpu *vcpu,
 					struct vmcs12 *vmcs12,
 					u32 *exit_qual)
 {
+	struct vcpu_vmx *vmx = to_vmx(vcpu);
 	bool ia32e;
 
 	*exit_qual = ENTRY_FAIL_DEFAULT;
@@ -2788,10 +2801,9 @@ static int nested_vmx_check_guest_state(struct kvm_vcpu *vcpu,
 			return -EINVAL;
 	}
 
-	if ((vmcs12->vm_entry_controls & VM_ENTRY_LOAD_BNDCFGS) &&
-	    (is_noncanonical_address(vmcs12->guest_bndcfgs & PAGE_MASK, vcpu) ||
-	     (vmcs12->guest_bndcfgs & MSR_IA32_BNDCFGS_RSVD)))
-		return -EINVAL;
+	if (vmx->nested.dirty_vmcs12 &&
+	    nested_vmx_check_guest_state_full(vcpu, vmcs12, exit_qual))
+			return -EINVAL;
 
 	if (nested_check_guest_non_reg_state(vmcs12))
 		return -EINVAL;
-- 
2.20.1

Re: [PATCH 0/5] KVM: nVMX: Skip vmentry checks that are necessary only if VMCS12 is dirty

[Sean Christopherson]

On Sun, Jul 07, 2019 at 03:11:42AM -0400, Krish Sadhukhan wrote:
> The following functions,
> 
> 	nested_vmx_check_controls
> 	nested_vmx_check_host_state
> 	nested_vmx_check_guest_state
> 
> do a number of vmentry checks for VMCS12. However, not all of these checks need
> to be executed on every vmentry. This patchset makes some of these vmentry
> checks optional based on the state of VMCS12 in that if VMCS12 is dirty, only
> then the checks will be executed. This will reduce performance impact on
> vmentry of nested guests.

All of these patches break vmx_set_nested_state(), which sets dirty_vmcs12
only after the aforementioned consistency checks pass.

The new nomenclature for the dirty paths is "rare", not "full".

In general, I dislike directly associating the consistency checks with
dirty_vmcs12.

  - It's difficult to assess the correctness of the resulting code, e.g.
    changing CPU_BASED_VM_EXEC_CONTROL doesn't set dirty_vmcs12, which
    calls into question any and all SECONDARY_VM_EXEC_CONTROL checks since
    an L1 could toggle CPU_BASED_ACTIVATE_SECONDARY_CONTROLS.

  - We lose the existing organization of the consistency checks, e.g.
    similar checks get arbitrarily split into separate flows based on
    the rarity of the field changing.

  - The performance gains are likely minimal since the majority of checks
    can't be skipped due to the coarseness of dirty_vmcs12.

Rather than a quick and dirty (pun intended) change to use dirty_vmcs12,
I think we should have some amount of dedicated infrastructure for
optimizing consistency checks from the get go, e.g. perhaps something
similar to how eVMCS categorizes fields.  The initial usage could be very
coarse grained, e.g. based purely on dirty_vmcs12, but having the
infrastructure would make it easier to reason about the correctness of
the code.  Future patches could then refine the triggerring of checks to
achieve better optimization, e.g. skipping the vast majority of checks
when L1 is simply toggling CPU_BASED_VIRTUAL_INTR_PENDING.

Re: [PATCH 0/5] KVM: nVMX: Skip vmentry checks that are necessary only if VMCS12 is dirty

[Krish Sadhukhan]

On 07/08/2019 11:17 AM, Sean Christopherson wrote:
> On Sun, Jul 07, 2019 at 03:11:42AM -0400, Krish Sadhukhan wrote:
>> The following functions,
>>
>> 	nested_vmx_check_controls
>> 	nested_vmx_check_host_state
>> 	nested_vmx_check_guest_state
>>
>> do a number of vmentry checks for VMCS12. However, not all of these checks need
>> to be executed on every vmentry. This patchset makes some of these vmentry
>> checks optional based on the state of VMCS12 in that if VMCS12 is dirty, only
>> then the checks will be executed. This will reduce performance impact on
>> vmentry of nested guests.
> All of these patches break vmx_set_nested_state(), which sets dirty_vmcs12
> only after the aforementioned consistency checks pass.

Perhaps vmx_set_nested_state() can set dirty_vmcs12 right before the 
consistency checks are done ? I see no difference in correctness. Also, 
it calls set_current_vmptr() which anyway sets dirty_vmcs12 for valid VMCSs.

>
> The new nomenclature for the dirty paths is "rare", not "full".
OK.
>
> In general, I dislike directly associating the consistency checks with
> dirty_vmcs12.
>
>    - It's difficult to assess the correctness of the resulting code, e.g.
>      changing CPU_BASED_VM_EXEC_CONTROL doesn't set dirty_vmcs12, which
>      calls into question any and all SECONDARY_VM_EXEC_CONTROL checks since
>      an L1 could toggle CPU_BASED_ACTIVATE_SECONDARY_CONTROLS.
>
>    - We lose the existing organization of the consistency checks, e.g.
>      similar checks get arbitrarily split into separate flows based on
>      the rarity of the field changing.

Initially, I was thinking of inserting the check for dirty_vmcs12 right 
in place of each of the checks without having to move them to separate 
functions. That approach saves the separation of the checks but results 
in poor readability. Hence I adopted the current approach.

>
>    - The performance gains are likely minimal since the majority of checks
>      can't be skipped due to the coarseness of dirty_vmcs12.
>
> Rather than a quick and dirty (pun intended) change to use dirty_vmcs12,
> I think we should have some amount of dedicated infrastructure for
> optimizing consistency checks from the get go, e.g. perhaps something
> similar to how eVMCS categorizes fields.
Are you referring to the categorization done in 
copy_vmcs12_to_enlightened() ? If so, what is the basis for 
categorization in there ?
We can re-order the checks in 
nested_vmx_check_{controls,host_state,guest_state} based on dirty_vmcs  
to create an initial framework for controlling the consistency checks. 
The only disadvantage will be that such an ordering will be completely 
off from how the SDM describes the checks.

>   The initial usage could be very
> coarse grained, e.g. based purely on dirty_vmcs12, but having the
> infrastructure would make it easier to reason about the correctness of
> the code.  Future patches could then refine the triggerring of checks to
> achieve better optimization, e.g. skipping the vast majority of checks
> when L1 is simply toggling CPU_BASED_VIRTUAL_INTR_PENDING.
It seems you are suggesting a finer granularity up to each VMCS field 
instead of groups of VMCS fields ? Then we need a per-field flag to 
track its modification and that seems an overkill.

Re: [PATCH 2/5] KVM: nVMX: Skip VM-Exit Control vmentry checks that are necessary only if VMCS12 is dirty

[Paolo Bonzini]

On 07/07/19 09:11, Krish Sadhukhan wrote:
>  
>  	if (!vmx_control_verify(vmcs12->vm_exit_controls,
>  				vmx->nested.msrs.exit_ctls_low,
> -				vmx->nested.msrs.exit_ctls_high) ||
> -	    nested_vmx_check_exit_msr_switch_controls(vcpu, vmcs12))
> +				vmx->nested.msrs.exit_ctls_high))
> +		return -EINVAL;
> +

Exit controls are not shadowed, are they?

Paolo

Re: [PATCH 0/5] KVM: nVMX: Skip vmentry checks that are necessary only if VMCS12 is dirty

[Paolo Bonzini]

On 08/07/19 20:17, Sean Christopherson wrote:
> On Sun, Jul 07, 2019 at 03:11:42AM -0400, Krish Sadhukhan wrote:
>> The following functions,
>>
>> 	nested_vmx_check_controls
>> 	nested_vmx_check_host_state
>> 	nested_vmx_check_guest_state
>>
>> do a number of vmentry checks for VMCS12. However, not all of these checks need
>> to be executed on every vmentry. This patchset makes some of these vmentry
>> checks optional based on the state of VMCS12 in that if VMCS12 is dirty, only
>> then the checks will be executed. This will reduce performance impact on
>> vmentry of nested guests.
> 
> All of these patches break vmx_set_nested_state(), which sets dirty_vmcs12
> only after the aforementioned consistency checks pass.
> 
> The new nomenclature for the dirty paths is "rare", not "full".
> 
> In general, I dislike directly associating the consistency checks with
> dirty_vmcs12.
> 
>   - It's difficult to assess the correctness of the resulting code, e.g.
>     changing CPU_BASED_VM_EXEC_CONTROL doesn't set dirty_vmcs12, which
>     calls into question any and all SECONDARY_VM_EXEC_CONTROL checks since
>     an L1 could toggle CPU_BASED_ACTIVATE_SECONDARY_CONTROLS.

Yes, CPU-based controls are tricky and should not be changed.  But I
don't see a big issue apart from the CPU-based controls, and the other
checks can also be quite expensive---and the point of dirty_vmcs12 and
shadow VMCS is that we _can_ exclude them most of the time.

This is all 5.4 material anyway, I'll do some testing of Krish's patches
2-5.

Thanks,

Paolo

>   - We lose the existing organization of the consistency checks, e.g.
>     similar checks get arbitrarily split into separate flows based on
>     the rarity of the field changing.
> 
>   - The performance gains are likely minimal since the majority of checks
>     can't be skipped due to the coarseness of dirty_vmcs12.
>
> Rather than a quick and dirty (pun intended) change to use dirty_vmcs12,
> I think we should have some amount of dedicated infrastructure for
> optimizing consistency checks from the get go, e.g. perhaps something
> similar to how eVMCS categorizes fields.  The initial usage could be very
> coarse grained, e.g. based purely on dirty_vmcs12, but having the
> infrastructure would make it easier to reason about the correctness of
> the code.  Future patches could then refine the triggerring of checks to
> achieve better optimization, e.g. skipping the vast majority of checks
> when L1 is simply toggling CPU_BASED_VIRTUAL_INTR_PENDING.

Re: [PATCH 0/5] KVM: nVMX: Skip vmentry checks that are necessary only if VMCS12 is dirty

[Sean Christopherson]

On Wed, Jul 10, 2019 at 04:35:46PM +0200, Paolo Bonzini wrote:
> On 08/07/19 20:17, Sean Christopherson wrote:
> > On Sun, Jul 07, 2019 at 03:11:42AM -0400, Krish Sadhukhan wrote:
> >> The following functions,
> >>
> >> 	nested_vmx_check_controls
> >> 	nested_vmx_check_host_state
> >> 	nested_vmx_check_guest_state
> >>
> >> do a number of vmentry checks for VMCS12. However, not all of these checks need
> >> to be executed on every vmentry. This patchset makes some of these vmentry
> >> checks optional based on the state of VMCS12 in that if VMCS12 is dirty, only
> >> then the checks will be executed. This will reduce performance impact on
> >> vmentry of nested guests.
> > 
> > All of these patches break vmx_set_nested_state(), which sets dirty_vmcs12
> > only after the aforementioned consistency checks pass.
> > 
> > The new nomenclature for the dirty paths is "rare", not "full".
> > 
> > In general, I dislike directly associating the consistency checks with
> > dirty_vmcs12.
> > 
> >   - It's difficult to assess the correctness of the resulting code, e.g.
> >     changing CPU_BASED_VM_EXEC_CONTROL doesn't set dirty_vmcs12, which
> >     calls into question any and all SECONDARY_VM_EXEC_CONTROL checks since
> >     an L1 could toggle CPU_BASED_ACTIVATE_SECONDARY_CONTROLS.
> 
> Yes, CPU-based controls are tricky and should not be changed.  But I
> don't see a big issue apart from the CPU-based controls, and the other
> checks can also be quite expensive---and the point of dirty_vmcs12 and
> shadow VMCS is that we _can_ exclude them most of the time.

No argument there.  My thought was do something like the following so that
all of the "which checks should we perform" logic is consolidated in a
single location and not spread piecemeal throughout the checks themselves.

static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch)
{
	unsigned long dirty_checks;

	...

	if (vmx->nested.dirty_vmcs12)
		dirty_checks = ENTRY_CONTROLS | EXIT_CONTROLS | HOST_STATE |
			       GUEST_STATE;
	else
		dirty_checks = 0;
}

Re: [PATCH 4/5] KVM: nVMX: Skip Host State Area vmentry checks that are necessary only if VMCS12 is dirty

[Paolo Bonzini]

On 07/07/19 09:11, Krish Sadhukhan wrote:
> +				       struct vmcs12 *vmcs12)
> +{
> +	struct vcpu_vmx *vmx = to_vmx(vcpu);
> +
> +	if ((vmx->nested.dirty_vmcs12) &&
> +	    nested_vmx_check_host_state_full(vcpu, vmcs12))
> +		return -EINVAL;
> +
> +	if (!nested_host_cr0_valid(vcpu, vmcs12->host_cr0))
> +		return -EINVAL;
> +
> +	if (is_noncanonical_address(vmcs12->host_ia32_sysenter_esp, vcpu))
> +		return -EINVAL;

These two are not part of the shadowed state, so they can be done only
in the "rare" case.

> +	if (vmcs12->host_fs_selector & (SEGMENT_RPL_MASK | SEGMENT_TI_MASK) ||
> +	    vmcs12->host_gs_selector & (SEGMENT_RPL_MASK | SEGMENT_TI_MASK))
> +		return -EINVAL;
> +
> +#ifdef CONFIG_X86_64
> +	if (is_noncanonical_address(vmcs12->host_fs_base, vcpu) ||
> +	    is_noncanonical_address(vmcs12->host_gs_base, vcpu) ||
> +	    is_noncanonical_address(vmcs12->host_gdtr_base, vcpu) ||
> +	    is_noncanonical_address(vmcs12->host_tr_base, vcpu))
> +		return -EINVAL;
> +#endif

Same for host GDTR and TR base.

Paolo

Re: [PATCH 3/5] KVM: nVMX: Skip VM-Entry Control checks that are necessary only if VMCS12 is dirty

[Paolo Bonzini]

On 07/07/19 09:11, Krish Sadhukhan wrote:
> @@ -2603,7 +2612,8 @@ static int nested_check_vm_entry_controls(struct kvm_vcpu *vcpu,
>  		}
>  	}
>  
> -	if (nested_vmx_check_entry_msr_switch_controls(vcpu, vmcs12))
> +	if ((vmx->nested.dirty_vmcs12) &&
> +	    nested_check_vm_entry_controls_full(vcpu, vmcs12))
>  		return -EINVAL;
>  
>  	return 0;
> -- 

The vmx_control_verify above can be moved to the "rare" case.

Paolo

Re: [PATCH 5/5] KVM: nVMX: Skip Guest State Area vmentry checks that are necessary only if VMCS12 is dirty

[Paolo Bonzini]

On 07/07/19 09:11, Krish Sadhukhan wrote:
>   ..so that every nested vmentry is not slowed down by those checks.
> 
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>

Here I think only the EFER check needs to be done always (before it
refers GUEST_CR0 which is shadowed).

Paolo

> ---
>  arch/x86/kvm/vmx/nested.c | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index b610f389a01b..095923b1d765 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -2748,10 +2748,23 @@ static int nested_check_guest_non_reg_state(struct vmcs12 *vmcs12)
>  	return 0;
>  }
>  
> +static int nested_vmx_check_guest_state_full(struct kvm_vcpu *vcpu,
> +					     struct vmcs12 *vmcs12,
> +					     u32 *exit_qual)
> +{
> +	if ((vmcs12->vm_entry_controls & VM_ENTRY_LOAD_BNDCFGS) &&
> +	    (is_noncanonical_address(vmcs12->guest_bndcfgs & PAGE_MASK, vcpu) ||
> +	     (vmcs12->guest_bndcfgs & MSR_IA32_BNDCFGS_RSVD)))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
>  static int nested_vmx_check_guest_state(struct kvm_vcpu *vcpu,
>  					struct vmcs12 *vmcs12,
>  					u32 *exit_qual)
>  {
> +	struct vcpu_vmx *vmx = to_vmx(vcpu);
>  	bool ia32e;
>  
>  	*exit_qual = ENTRY_FAIL_DEFAULT;
> @@ -2788,10 +2801,9 @@ static int nested_vmx_check_guest_state(struct kvm_vcpu *vcpu,
>  			return -EINVAL;
>  	}
>  
> -	if ((vmcs12->vm_entry_controls & VM_ENTRY_LOAD_BNDCFGS) &&
> -	    (is_noncanonical_address(vmcs12->guest_bndcfgs & PAGE_MASK, vcpu) ||
> -	     (vmcs12->guest_bndcfgs & MSR_IA32_BNDCFGS_RSVD)))
> -		return -EINVAL;
> +	if (vmx->nested.dirty_vmcs12 &&
> +	    nested_vmx_check_guest_state_full(vcpu, vmcs12, exit_qual))
> +			return -EINVAL;
>  
>  	if (nested_check_guest_non_reg_state(vmcs12))
>  		return -EINVAL;
> 

Re: [PATCH 0/5] KVM: nVMX: Skip vmentry checks that are necessary only if VMCS12 is dirty

[Paolo Bonzini]

On 10/07/19 18:15, Sean Christopherson wrote:
> On Wed, Jul 10, 2019 at 04:35:46PM +0200, Paolo Bonzini wrote:
>> On 08/07/19 20:17, Sean Christopherson wrote:
>>> On Sun, Jul 07, 2019 at 03:11:42AM -0400, Krish Sadhukhan wrote:
>>>> The following functions,
>>>>
>>>> 	nested_vmx_check_controls
>>>> 	nested_vmx_check_host_state
>>>> 	nested_vmx_check_guest_state
>>>>
>>>> do a number of vmentry checks for VMCS12. However, not all of these checks need
>>>> to be executed on every vmentry. This patchset makes some of these vmentry
>>>> checks optional based on the state of VMCS12 in that if VMCS12 is dirty, only
>>>> then the checks will be executed. This will reduce performance impact on
>>>> vmentry of nested guests.
>>>
>>> All of these patches break vmx_set_nested_state(), which sets dirty_vmcs12
>>> only after the aforementioned consistency checks pass.
>>>
>>> The new nomenclature for the dirty paths is "rare", not "full".
>>>
>>> In general, I dislike directly associating the consistency checks with
>>> dirty_vmcs12.
>>>
>>>   - It's difficult to assess the correctness of the resulting code, e.g.
>>>     changing CPU_BASED_VM_EXEC_CONTROL doesn't set dirty_vmcs12, which
>>>     calls into question any and all SECONDARY_VM_EXEC_CONTROL checks since
>>>     an L1 could toggle CPU_BASED_ACTIVATE_SECONDARY_CONTROLS.
>>
>> Yes, CPU-based controls are tricky and should not be changed.  But I
>> don't see a big issue apart from the CPU-based controls, and the other
>> checks can also be quite expensive---and the point of dirty_vmcs12 and
>> shadow VMCS is that we _can_ exclude them most of the time.
> 
> No argument there.  My thought was do something like the following so that
> all of the "which checks should we perform" logic is consolidated in a
> single location and not spread piecemeal throughout the checks themselves.
> 
> static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch)
> {
> 	unsigned long dirty_checks;
> 
> 	...
> 
> 	if (vmx->nested.dirty_vmcs12)
> 		dirty_checks = ENTRY_CONTROLS | EXIT_CONTROLS | HOST_STATE |
> 			       GUEST_STATE;
> 	else
> 		dirty_checks = 0;
> }

That makes sense, though it would be somewhat awkward:

	dirty_checks = EXEC_CONTROLS | HOST_STATE_FSGS |
		ENTRY_CONTROLS_INTRINFO | GUEST_STATE_EFER;
	if (vmx->nested.dirty_vmcs12)
		dirty_checks |= ENTRY_CONTROLS_FULL | EXIT_CONTROLS |
			HOST_STATE_FULL | GUEST_STATE_FULL;

Paolo

Re: [PATCH 2/5] KVM: nVMX: Skip VM-Exit Control vmentry checks that are necessary only if VMCS12 is dirty

[Krish Sadhukhan]

On 7/10/19 7:28 AM, Paolo Bonzini wrote:
> On 07/07/19 09:11, Krish Sadhukhan wrote:
>>   
>>   	if (!vmx_control_verify(vmcs12->vm_exit_controls,
>>   				vmx->nested.msrs.exit_ctls_low,
>> -				vmx->nested.msrs.exit_ctls_high) ||
>> -	    nested_vmx_check_exit_msr_switch_controls(vcpu, vmcs12))
>> +				vmx->nested.msrs.exit_ctls_high))
>> +		return -EINVAL;
>> +
> Exit controls are not shadowed, are they?

No, they aren't.   However, I see that prepare_vmcs02_constant_state() 
which is called in the path of prepare_vmcs02_early_full() writes those 
Exit Control fields:

        vmcs_write32(VM_EXIT_MSR_STORE_COUNT, 0);
         vmcs_write64(VM_EXIT_MSR_LOAD_ADDR, 
__pa(vmx->msr_autoload.host.val));
         vmcs_write64(VM_ENTRY_MSR_LOAD_ADDR, 
__pa(vmx->msr_autoload.guest.val));


Should we add these fields to the shadow list then ?

>
> Paolo

Re: [PATCH 5/5] KVM: nVMX: Skip Guest State Area vmentry checks that are necessary only if VMCS12 is dirty

[Krish Sadhukhan]

On 7/10/19 9:33 AM, Paolo Bonzini wrote:
> On 07/07/19 09:11, Krish Sadhukhan wrote:
>>    ..so that every nested vmentry is not slowed down by those checks.
>>
>> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> Here I think only the EFER check needs to be done always (before it
> refers GUEST_CR0 which is shadowed).


Should I send v2 ?

Also, I forgot to add  the following to the patchset,

            Suggested-by: Paolo Bonzini <pbonzini@redhat.com>

>
> Paolo
>
>> ---
>>   arch/x86/kvm/vmx/nested.c | 20 ++++++++++++++++----
>>   1 file changed, 16 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
>> index b610f389a01b..095923b1d765 100644
>> --- a/arch/x86/kvm/vmx/nested.c
>> +++ b/arch/x86/kvm/vmx/nested.c
>> @@ -2748,10 +2748,23 @@ static int nested_check_guest_non_reg_state(struct vmcs12 *vmcs12)
>>   	return 0;
>>   }
>>   
>> +static int nested_vmx_check_guest_state_full(struct kvm_vcpu *vcpu,
>> +					     struct vmcs12 *vmcs12,
>> +					     u32 *exit_qual)
>> +{
>> +	if ((vmcs12->vm_entry_controls & VM_ENTRY_LOAD_BNDCFGS) &&
>> +	    (is_noncanonical_address(vmcs12->guest_bndcfgs & PAGE_MASK, vcpu) ||
>> +	     (vmcs12->guest_bndcfgs & MSR_IA32_BNDCFGS_RSVD)))
>> +		return -EINVAL;
>> +
>> +	return 0;
>> +}
>> +
>>   static int nested_vmx_check_guest_state(struct kvm_vcpu *vcpu,
>>   					struct vmcs12 *vmcs12,
>>   					u32 *exit_qual)
>>   {
>> +	struct vcpu_vmx *vmx = to_vmx(vcpu);
>>   	bool ia32e;
>>   
>>   	*exit_qual = ENTRY_FAIL_DEFAULT;
>> @@ -2788,10 +2801,9 @@ static int nested_vmx_check_guest_state(struct kvm_vcpu *vcpu,
>>   			return -EINVAL;
>>   	}
>>   
>> -	if ((vmcs12->vm_entry_controls & VM_ENTRY_LOAD_BNDCFGS) &&
>> -	    (is_noncanonical_address(vmcs12->guest_bndcfgs & PAGE_MASK, vcpu) ||
>> -	     (vmcs12->guest_bndcfgs & MSR_IA32_BNDCFGS_RSVD)))
>> -		return -EINVAL;
>> +	if (vmx->nested.dirty_vmcs12 &&
>> +	    nested_vmx_check_guest_state_full(vcpu, vmcs12, exit_qual))
>> +			return -EINVAL;
>>   
>>   	if (nested_check_guest_non_reg_state(vmcs12))
>>   		return -EINVAL;
>>

Re: [PATCH 2/5] KVM: nVMX: Skip VM-Exit Control vmentry checks that are necessary only if VMCS12 is dirty

[Sean Christopherson]

On Wed, Jul 10, 2019 at 12:18:55PM -0700, Krish Sadhukhan wrote:
> 
> On 7/10/19 7:28 AM, Paolo Bonzini wrote:
> >On 07/07/19 09:11, Krish Sadhukhan wrote:
> >>  	if (!vmx_control_verify(vmcs12->vm_exit_controls,
> >>  				vmx->nested.msrs.exit_ctls_low,
> >>-				vmx->nested.msrs.exit_ctls_high) ||
> >>-	    nested_vmx_check_exit_msr_switch_controls(vcpu, vmcs12))
> >>+				vmx->nested.msrs.exit_ctls_high))
> >>+		return -EINVAL;
> >>+
> >Exit controls are not shadowed, are they?
> 
> No, they aren't.   However, I see that prepare_vmcs02_constant_state() which
> is called in the path of prepare_vmcs02_early_full() writes those Exit
> Control fields:
> 
>        vmcs_write32(VM_EXIT_MSR_STORE_COUNT, 0);
>         vmcs_write64(VM_EXIT_MSR_LOAD_ADDR,
> __pa(vmx->msr_autoload.host.val));
>         vmcs_write64(VM_ENTRY_MSR_LOAD_ADDR,
> __pa(vmx->msr_autoload.guest.val));

That's writing L0's values into vmcs02 when vmcs02 is first used.  L1's
MSR load lists are processed purely in software, e.g. nested_vmx_load_msr().
The vmcs12 entries are consumed by KVM on every nested transition to
emulate the load/store functionality, but the validity of the *controls*
only needs to be checked when vmcs12 is dirty.

> 
> 
> Should we add these fields to the shadow list then ?
> 
> >
> >Paolo