[PATCH v3 0/4] KVM: nVMX: Enlightened MSR Bitmap feature for Hyper-V on KVM

[PATCH v3 0/4] KVM: nVMX: Enlightened MSR Bitmap feature for Hyper-V on KVM

[Vitaly Kuznetsov]

Changes since v2:
- Renamed 'msr_bitmap_changed' to 'msr_bitmap_force_recalc' [Paolo] and
  expanded the comment near its definition explaining its limited 
  usefulness [Sean].

Original description:

Updating MSR bitmap for L2 is not cheap and rearly needed. TLFS for Hyper-V
offers 'Enlightened MSR Bitmap' feature which allows L1 hypervisor to
inform L0 when it changes MSR bitmap, this eliminates the need to examine
L1's MSR bitmap for L2 every time when 'real' MSR bitmap for L2 gets
constructed.

When the feature is enabled for Win10+WSL2, it shaves off around 700 CPU
cycles from a nested vmexit cost (tight cpuid loop test).

First patch of the series is unrelated to the newly implemented feature,
it fixes a bug in Enlightened MSR Bitmap usage when KVM runs as a nested
hypervisor on top of Hyper-V.

Vitaly Kuznetsov (4):
  KVM: nVMX: Don't use Enlightened MSR Bitmap for L3
  KVM: VMX: Introduce vmx_msr_bitmap_l01_changed() helper
  KVM: nVMX: Track whether changes in L0 require MSR bitmap for L2 to be
    rebuilt
  KVM: nVMX: Implement Enlightened MSR Bitmap feature

 arch/x86/kvm/hyperv.c     |  2 ++
 arch/x86/kvm/vmx/nested.c | 29 +++++++++++++++++++++++++---
 arch/x86/kvm/vmx/vmx.c    | 40 ++++++++++++++++++++++++++-------------
 arch/x86/kvm/vmx/vmx.h    |  9 +++++++++
 4 files changed, 64 insertions(+), 16 deletions(-)

-- 
2.31.1

[PATCH v3 1/4] KVM: nVMX: Don't use Enlightened MSR Bitmap for L3

[Vitaly Kuznetsov]

When KVM runs as a nested hypervisor on top of Hyper-V it uses Enlightened
VMCS and enables Enlightened MSR Bitmap feature for its L1s and L2s (which
are actually L2s and L3s from Hyper-V's perspective). When MSR bitmap is
updated, KVM has to reset HV_VMX_ENLIGHTENED_CLEAN_FIELD_MSR_BITMAP from
clean fields to make Hyper-V aware of the change. For KVM's L1s, this is
done in vmx_disable_intercept_for_msr()/vmx_enable_intercept_for_msr().
MSR bitmap for L2 is build in nested_vmx_prepare_msr_bitmap() by blending
MSR bitmap for L1 and L1's idea of MSR bitmap for L2. KVM, however, doesn't
check if the resulting bitmap is different and never cleans
HV_VMX_ENLIGHTENED_CLEAN_FIELD_MSR_BITMAP in eVMCS02. This is incorrect and
may result in Hyper-V missing the update.

The issue could've been solved by calling evmcs_touch_msr_bitmap() for
eVMCS02 from nested_vmx_prepare_msr_bitmap() unconditionally but doing so
would not give any performance benefits (compared to not using Enlightened
MSR Bitmap at all). 3-level nesting is also not a very common setup
nowadays.

Don't enable 'Enlightened MSR Bitmap' feature for KVM's L2s (real L3s) for
now.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 arch/x86/kvm/vmx/vmx.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 1c8b2b6e7ed9..e82cdde58119 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -2655,15 +2655,6 @@ int alloc_loaded_vmcs(struct loaded_vmcs *loaded_vmcs)
 		if (!loaded_vmcs->msr_bitmap)
 			goto out_vmcs;
 		memset(loaded_vmcs->msr_bitmap, 0xff, PAGE_SIZE);
-
-		if (IS_ENABLED(CONFIG_HYPERV) &&
-		    static_branch_unlikely(&enable_evmcs) &&
-		    (ms_hyperv.nested_features & HV_X64_NESTED_MSR_BITMAP)) {
-			struct hv_enlightened_vmcs *evmcs =
-				(struct hv_enlightened_vmcs *)loaded_vmcs->vmcs;
-
-			evmcs->hv_enlightenments_control.msr_bitmap = 1;
-		}
 	}
 
 	memset(&loaded_vmcs->host_state, 0, sizeof(struct vmcs_host_state));
@@ -6903,6 +6894,18 @@ static int vmx_create_vcpu(struct kvm_vcpu *vcpu)
 
 	vmx->loaded_vmcs = &vmx->vmcs01;
 
+	/*
+	 * Use Hyper-V 'Enlightened MSR Bitmap' feature when KVM runs as a
+	 * nested (L1) hypervisor and Hyper-V in L0 supports it.
+	 */
+	if (IS_ENABLED(CONFIG_HYPERV) && static_branch_unlikely(&enable_evmcs)
+	    && (ms_hyperv.nested_features & HV_X64_NESTED_MSR_BITMAP)) {
+		struct hv_enlightened_vmcs *evmcs =
+			(struct hv_enlightened_vmcs *)vmx->loaded_vmcs->vmcs;
+
+		evmcs->hv_enlightenments_control.msr_bitmap = 1;
+	}
+
 	if (cpu_need_virtualize_apic_accesses(vcpu)) {
 		err = alloc_apic_access_page(vcpu->kvm);
 		if (err)
-- 
2.31.1

[PATCH v3 2/4] KVM: VMX: Introduce vmx_msr_bitmap_l01_changed() helper

[Vitaly Kuznetsov]

In preparation to enabling 'Enlightened MSR Bitmap' feature for Hyper-V
guests move MSR bitmap update tracking to a dedicated helper.

Note: vmx_msr_bitmap_l01_changed() is called when MSR bitmap might be
updated. KVM doesn't check if the bit we're trying to set is already set
(or the bit it's trying to clear is already cleared). Such situations
should not be common and a few false positives should not be a problem.

No functional change intended.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 arch/x86/kvm/vmx/vmx.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index e82cdde58119..3fdaaef291e8 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -3726,6 +3726,17 @@ static void vmx_set_msr_bitmap_write(ulong *msr_bitmap, u32 msr)
 		__set_bit(msr & 0x1fff, msr_bitmap + 0xc00 / f);
 }
 
+static void vmx_msr_bitmap_l01_changed(struct vcpu_vmx *vmx)
+{
+	/*
+	 * When KVM is a nested hypervisor on top of Hyper-V and uses
+	 * 'Enlightened MSR Bitmap' feature L0 needs to know that MSR
+	 * bitmap has changed.
+	 */
+	if (static_branch_unlikely(&enable_evmcs))
+		evmcs_touch_msr_bitmap();
+}
+
 void vmx_disable_intercept_for_msr(struct kvm_vcpu *vcpu, u32 msr, int type)
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
@@ -3734,8 +3745,7 @@ void vmx_disable_intercept_for_msr(struct kvm_vcpu *vcpu, u32 msr, int type)
 	if (!cpu_has_vmx_msr_bitmap())
 		return;
 
-	if (static_branch_unlikely(&enable_evmcs))
-		evmcs_touch_msr_bitmap();
+	vmx_msr_bitmap_l01_changed(vmx);
 
 	/*
 	 * Mark the desired intercept state in shadow bitmap, this is needed
@@ -3779,8 +3789,7 @@ void vmx_enable_intercept_for_msr(struct kvm_vcpu *vcpu, u32 msr, int type)
 	if (!cpu_has_vmx_msr_bitmap())
 		return;
 
-	if (static_branch_unlikely(&enable_evmcs))
-		evmcs_touch_msr_bitmap();
+	vmx_msr_bitmap_l01_changed(vmx);
 
 	/*
 	 * Mark the desired intercept state in shadow bitmap, this is needed
-- 
2.31.1

[PATCH v3 3/4] KVM: nVMX: Track whether changes in L0 require MSR bitmap for L2 to be rebuilt

[Vitaly Kuznetsov]

Introduce a flag to keep track of whether MSR bitmap for L2 needs to be
rebuilt due to changes in MSR bitmap for L1 or switching to a different
L2. This information will be used for Enlightened MSR Bitmap feature for
Hyper-V guests.

Note, setting msr_bitmap_changed to 'true' from set_current_vmptr() is
not really needed for Enlightened MSR Bitmap as the feature can only
be used in conjunction with Enlightened VMCS but let's keep tracking
information complete, it's cheap and in the future similar PV feature can
easily be implemented for KVM on KVM too.

No functional change intended.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 arch/x86/kvm/vmx/nested.c | 9 ++++++++-
 arch/x86/kvm/vmx/vmx.c    | 2 ++
 arch/x86/kvm/vmx/vmx.h    | 9 +++++++++
 3 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index af1bbb73430a..bf4fa63ed371 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -700,6 +700,8 @@ static inline bool nested_vmx_prepare_msr_bitmap(struct kvm_vcpu *vcpu,
 
 	kvm_vcpu_unmap(vcpu, &to_vmx(vcpu)->nested.msr_bitmap_map, false);
 
+	vmx->nested.msr_bitmap_force_recalc = false;
+
 	return true;
 }
 
@@ -2054,10 +2056,13 @@ static enum nested_evmptrld_status nested_vmx_handle_enlightened_vmptrld(
 	 * Clean fields data can't be used on VMLAUNCH and when we switch
 	 * between different L2 guests as KVM keeps a single VMCS12 per L1.
 	 */
-	if (from_launch || evmcs_gpa_changed)
+	if (from_launch || evmcs_gpa_changed) {
 		vmx->nested.hv_evmcs->hv_clean_fields &=
 			~HV_VMX_ENLIGHTENED_CLEAN_FIELD_ALL;
 
+		vmx->nested.msr_bitmap_force_recalc = true;
+	}
+
 	return EVMPTRLD_SUCCEEDED;
 }
 
@@ -5274,6 +5279,7 @@ static void set_current_vmptr(struct vcpu_vmx *vmx, gpa_t vmptr)
 		vmx->nested.need_vmcs12_to_shadow_sync = true;
 	}
 	vmx->nested.dirty_vmcs12 = true;
+	vmx->nested.msr_bitmap_force_recalc = true;
 }
 
 /* Emulate the VMPTRLD instruction */
@@ -6400,6 +6406,7 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu,
 		goto error_guest_mode;
 
 	vmx->nested.dirty_vmcs12 = true;
+	vmx->nested.msr_bitmap_force_recalc = true;
 	ret = nested_vmx_enter_non_root_mode(vcpu, false);
 	if (ret)
 		goto error_guest_mode;
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 3fdaaef291e8..c78c315def59 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -3735,6 +3735,8 @@ static void vmx_msr_bitmap_l01_changed(struct vcpu_vmx *vmx)
 	 */
 	if (static_branch_unlikely(&enable_evmcs))
 		evmcs_touch_msr_bitmap();
+
+	vmx->nested.msr_bitmap_force_recalc = true;
 }
 
 void vmx_disable_intercept_for_msr(struct kvm_vcpu *vcpu, u32 msr, int type)
diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
index 592217fd7d92..2cdf66e6d1b0 100644
--- a/arch/x86/kvm/vmx/vmx.h
+++ b/arch/x86/kvm/vmx/vmx.h
@@ -148,6 +148,15 @@ struct nested_vmx {
 	bool need_vmcs12_to_shadow_sync;
 	bool dirty_vmcs12;
 
+	/*
+	 * Indicates whether MSR bitmap for L2 needs to be rebuilt due to
+	 * changes in MSR bitmap for L1 or switching to a different L2. Note,
+	 * this flag can only be used reliably in conjunction with a paravirt L1
+	 * which informs L0 whether any changes to MSR bitmap for L2 were done
+	 * on its side.
+	 */
+	bool msr_bitmap_force_recalc;
+
 	/*
 	 * Indicates lazily loaded guest state has not yet been decached from
 	 * vmcs02.
-- 
2.31.1

[PATCH v3 4/4] KVM: nVMX: Implement Enlightened MSR Bitmap feature

[Vitaly Kuznetsov]

Updating MSR bitmap for L2 is not cheap and rearly needed. TLFS for Hyper-V
offers 'Enlightened MSR Bitmap' feature which allows L1 hypervisor to
inform L0 when it changes MSR bitmap, this eliminates the need to examine
L1's MSR bitmap for L2 every time when 'real' MSR bitmap for L2 gets
constructed.

Use 'vmx->nested.msr_bitmap_changed' flag to implement the feature.

Note, KVM already uses 'Enlightened MSR bitmap' feature when it runs as a
nested hypervisor on top of Hyper-V. The newly introduced feature is going
to be used by Hyper-V guests on KVM.

When the feature is enabled for Win10+WSL2, it shaves off around 700 CPU
cycles from a nested vmexit cost (tight cpuid loop test).

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 arch/x86/kvm/hyperv.c     |  2 ++
 arch/x86/kvm/vmx/nested.c | 20 ++++++++++++++++++--
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index 6f11cda2bfa4..a00de1dbec57 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -2516,6 +2516,8 @@ int kvm_get_hv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid2 *cpuid,
 
 		case HYPERV_CPUID_NESTED_FEATURES:
 			ent->eax = evmcs_ver;
+			if (evmcs_ver)
+				ent->eax |= HV_X64_NESTED_MSR_BITMAP;
 
 			break;
 
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index bf4fa63ed371..7cd0c20d4557 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -608,15 +608,30 @@ static inline bool nested_vmx_prepare_msr_bitmap(struct kvm_vcpu *vcpu,
 						 struct vmcs12 *vmcs12)
 {
 	int msr;
+	struct vcpu_vmx *vmx = to_vmx(vcpu);
 	unsigned long *msr_bitmap_l1;
-	unsigned long *msr_bitmap_l0 = to_vmx(vcpu)->nested.vmcs02.msr_bitmap;
-	struct kvm_host_map *map = &to_vmx(vcpu)->nested.msr_bitmap_map;
+	unsigned long *msr_bitmap_l0 = vmx->nested.vmcs02.msr_bitmap;
+	struct hv_enlightened_vmcs *evmcs = vmx->nested.hv_evmcs;
+	struct kvm_host_map *map = &vmx->nested.msr_bitmap_map;
 
 	/* Nothing to do if the MSR bitmap is not in use.  */
 	if (!cpu_has_vmx_msr_bitmap() ||
 	    !nested_cpu_has(vmcs12, CPU_BASED_USE_MSR_BITMAPS))
 		return false;
 
+	/*
+	 * MSR bitmap update can be skipped when:
+	 * - MSR bitmap for L1 hasn't changed.
+	 * - Nested hypervisor (L1) is attempting to launch the same L2 as
+	 *   before.
+	 * - Nested hypervisor (L1) has enabled 'Enlightened MSR Bitmap' feature
+	 *   and tells KVM (L0) there were no changes in MSR bitmap for L2.
+	 */
+	if (!vmx->nested.msr_bitmap_force_recalc && evmcs &&
+	    evmcs->hv_enlightenments_control.msr_bitmap &&
+	    evmcs->hv_clean_fields & HV_VMX_ENLIGHTENED_CLEAN_FIELD_MSR_BITMAP)
+		goto out_clear_msr_bitmap_force_recalc;
+
 	if (kvm_vcpu_map(vcpu, gpa_to_gfn(vmcs12->msr_bitmap), map))
 		return false;
 
@@ -700,6 +715,7 @@ static inline bool nested_vmx_prepare_msr_bitmap(struct kvm_vcpu *vcpu,
 
 	kvm_vcpu_unmap(vcpu, &to_vmx(vcpu)->nested.msr_bitmap_map, false);
 
+out_clear_msr_bitmap_force_recalc:
 	vmx->nested.msr_bitmap_force_recalc = false;
 
 	return true;
-- 
2.31.1

Re: [PATCH v3 0/4] KVM: nVMX: Enlightened MSR Bitmap feature for Hyper-V on KVM

[Vitaly Kuznetsov]

Vitaly Kuznetsov <vkuznets@redhat.com> writes:

> Changes since v2:
> - Renamed 'msr_bitmap_changed' to 'msr_bitmap_force_recalc' [Paolo] and
>   expanded the comment near its definition explaining its limited 
>   usefulness [Sean].
>
> Original description:
>
> Updating MSR bitmap for L2 is not cheap and rearly needed. TLFS for Hyper-V
> offers 'Enlightened MSR Bitmap' feature which allows L1 hypervisor to
> inform L0 when it changes MSR bitmap, this eliminates the need to examine
> L1's MSR bitmap for L2 every time when 'real' MSR bitmap for L2 gets
> constructed.
>
> When the feature is enabled for Win10+WSL2, it shaves off around 700 CPU
> cycles from a nested vmexit cost (tight cpuid loop test).
>
> First patch of the series is unrelated to the newly implemented feature,
> it fixes a bug in Enlightened MSR Bitmap usage when KVM runs as a nested
> hypervisor on top of Hyper-V.
>

Ping?

-- 
Vitaly

Re: [PATCH v3 1/4] KVM: nVMX: Don't use Enlightened MSR Bitmap for L3

[Sean Christopherson]

On Wed, Oct 13, 2021, Vitaly Kuznetsov wrote:
> 3-level nesting is also not a very common setup nowadays.

Says who? :-D

> Don't enable 'Enlightened MSR Bitmap' feature for KVM's L2s (real L3s) for
> now.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  arch/x86/kvm/vmx/vmx.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 1c8b2b6e7ed9..e82cdde58119 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -2655,15 +2655,6 @@ int alloc_loaded_vmcs(struct loaded_vmcs *loaded_vmcs)
>  		if (!loaded_vmcs->msr_bitmap)
>  			goto out_vmcs;
>  		memset(loaded_vmcs->msr_bitmap, 0xff, PAGE_SIZE);
> -
> -		if (IS_ENABLED(CONFIG_HYPERV) &&
> -		    static_branch_unlikely(&enable_evmcs) &&
> -		    (ms_hyperv.nested_features & HV_X64_NESTED_MSR_BITMAP)) {
> -			struct hv_enlightened_vmcs *evmcs =
> -				(struct hv_enlightened_vmcs *)loaded_vmcs->vmcs;
> -
> -			evmcs->hv_enlightenments_control.msr_bitmap = 1;
> -		}
>  	}
>  
>  	memset(&loaded_vmcs->host_state, 0, sizeof(struct vmcs_host_state));
> @@ -6903,6 +6894,18 @@ static int vmx_create_vcpu(struct kvm_vcpu *vcpu)
>  
>  	vmx->loaded_vmcs = &vmx->vmcs01;
>  
> +	/*
> +	 * Use Hyper-V 'Enlightened MSR Bitmap' feature when KVM runs as a
> +	 * nested (L1) hypervisor and Hyper-V in L0 supports it.

And maybe call out specifically that KVM intentionally uses this only for vmcs02?

> +	 */
> +	if (IS_ENABLED(CONFIG_HYPERV) && static_branch_unlikely(&enable_evmcs)
> +	    && (ms_hyperv.nested_features & HV_X64_NESTED_MSR_BITMAP)) {

&& on the previous line, I think we'll survive the 82 char line :-)

> +		struct hv_enlightened_vmcs *evmcs =
> +			(struct hv_enlightened_vmcs *)vmx->loaded_vmcs->vmcs;

Hmm, what about landing this right after vmcs01's VMCS is allocated?  It's kinda
weird, but it makes it more obvious that ->vmcs is not NULL.  And if the cast is
simply via a "void *" it all fits on one line.

	err = alloc_loaded_vmcs(&vmx->vmcs01);
	if (err < 0)
		goto free_pml;

	/*
	 * Use Hyper-V 'Enlightened MSR Bitmap' feature when KVM runs as a
	 * nested (L1) hypervisor and Hyper-V in L0 supports it.  Enable an
	 * enlightened bitmap only for vmcs01, KVM currently isn't equipped to
	 * realize any performance benefits from enabling it for vmcs02.
	 */ 
	if (IS_ENABLED(CONFIG_HYPERV) && static_branch_unlikely(&enable_evmcs) &&
	    (ms_hyperv.nested_features & HV_X64_NESTED_MSR_BITMAP)) {
		struct hv_enlightened_vmcs *evmcs = (void *)vmx->vmcs01.vmcs;

		evmcs->hv_enlightenments_control.msr_bitmap = 1;
	}

> +
> +		evmcs->hv_enlightenments_control.msr_bitmap = 1;
> +	}
> +
>  	if (cpu_need_virtualize_apic_accesses(vcpu)) {
>  		err = alloc_apic_access_page(vcpu->kvm);
>  		if (err)
> -- 
> 2.31.1
> 

Re: [PATCH v3 2/4] KVM: VMX: Introduce vmx_msr_bitmap_l01_changed() helper

[Sean Christopherson]

On Wed, Oct 13, 2021, Vitaly Kuznetsov wrote:
> In preparation to enabling 'Enlightened MSR Bitmap' feature for Hyper-V
> guests move MSR bitmap update tracking to a dedicated helper.
> 
> Note: vmx_msr_bitmap_l01_changed() is called when MSR bitmap might be
> updated. KVM doesn't check if the bit we're trying to set is already set
> (or the bit it's trying to clear is already cleared). Such situations
> should not be common and a few false positives should not be a problem.
> 
> No functional change intended.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---

Reviewed-by: Sean Christopherson <seanjc@google.com>

Re: [PATCH v3 3/4] KVM: nVMX: Track whether changes in L0 require MSR bitmap for L2 to be rebuilt

[Sean Christopherson]

On Wed, Oct 13, 2021, Vitaly Kuznetsov wrote:
> diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> index 592217fd7d92..2cdf66e6d1b0 100644
> --- a/arch/x86/kvm/vmx/vmx.h
> +++ b/arch/x86/kvm/vmx/vmx.h
> @@ -148,6 +148,15 @@ struct nested_vmx {
>  	bool need_vmcs12_to_shadow_sync;
>  	bool dirty_vmcs12;
>  
> +	/*
> +	 * Indicates whether MSR bitmap for L2 needs to be rebuilt due to
> +	 * changes in MSR bitmap for L1 or switching to a different L2. Note,
> +	 * this flag can only be used reliably in conjunction with a paravirt L1
> +	 * which informs L0 whether any changes to MSR bitmap for L2 were done
> +	 * on its side.
> +	 */
> +	bool msr_bitmap_force_recalc;

Belated bikeshedding...  What about need_msr_bitmap_recalc to follow the above
need_vmcs12_to_shadow_sync?

> +
>  	/*
>  	 * Indicates lazily loaded guest state has not yet been decached from
>  	 * vmcs02.
> -- 
> 2.31.1
> 

Re: [PATCH v3 4/4] KVM: nVMX: Implement Enlightened MSR Bitmap feature

[Sean Christopherson]

On Wed, Oct 13, 2021, Vitaly Kuznetsov wrote:
> Updating MSR bitmap for L2 is not cheap and rearly needed. TLFS for Hyper-V
> offers 'Enlightened MSR Bitmap' feature which allows L1 hypervisor to
> inform L0 when it changes MSR bitmap, this eliminates the need to examine
> L1's MSR bitmap for L2 every time when 'real' MSR bitmap for L2 gets
> constructed.
> 
> Use 'vmx->nested.msr_bitmap_changed' flag to implement the feature.
> 
> Note, KVM already uses 'Enlightened MSR bitmap' feature when it runs as a
> nested hypervisor on top of Hyper-V. The newly introduced feature is going
> to be used by Hyper-V guests on KVM.
> 
> When the feature is enabled for Win10+WSL2, it shaves off around 700 CPU
> cycles from a nested vmexit cost (tight cpuid loop test).
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  arch/x86/kvm/hyperv.c     |  2 ++
>  arch/x86/kvm/vmx/nested.c | 20 ++++++++++++++++++--
>  2 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> index 6f11cda2bfa4..a00de1dbec57 100644
> --- a/arch/x86/kvm/hyperv.c
> +++ b/arch/x86/kvm/hyperv.c
> @@ -2516,6 +2516,8 @@ int kvm_get_hv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid2 *cpuid,
>  
>  		case HYPERV_CPUID_NESTED_FEATURES:
>  			ent->eax = evmcs_ver;
> +			if (evmcs_ver)
> +				ent->eax |= HV_X64_NESTED_MSR_BITMAP;
>  
>  			break;
>  
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index bf4fa63ed371..7cd0c20d4557 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -608,15 +608,30 @@ static inline bool nested_vmx_prepare_msr_bitmap(struct kvm_vcpu *vcpu,
>  						 struct vmcs12 *vmcs12)
>  {
>  	int msr;
> +	struct vcpu_vmx *vmx = to_vmx(vcpu);
>  	unsigned long *msr_bitmap_l1;
> -	unsigned long *msr_bitmap_l0 = to_vmx(vcpu)->nested.vmcs02.msr_bitmap;
> -	struct kvm_host_map *map = &to_vmx(vcpu)->nested.msr_bitmap_map;
> +	unsigned long *msr_bitmap_l0 = vmx->nested.vmcs02.msr_bitmap;
> +	struct hv_enlightened_vmcs *evmcs = vmx->nested.hv_evmcs;
> +	struct kvm_host_map *map = &vmx->nested.msr_bitmap_map;

That reminds me, can my nested bitmap fixes get merged?  Superficial conflicts,
but still conflicts that I'd rather not have to resolve :-)

https://lkml.kernel.org/r/20210924204907.1111817-1-seanjc@google.com

>  
>  	/* Nothing to do if the MSR bitmap is not in use.  */
>  	if (!cpu_has_vmx_msr_bitmap() ||
>  	    !nested_cpu_has(vmcs12, CPU_BASED_USE_MSR_BITMAPS))
>  		return false;
>  
> +	/*
> +	 * MSR bitmap update can be skipped when:
> +	 * - MSR bitmap for L1 hasn't changed.
> +	 * - Nested hypervisor (L1) is attempting to launch the same L2 as
> +	 *   before.
> +	 * - Nested hypervisor (L1) has enabled 'Enlightened MSR Bitmap' feature
> +	 *   and tells KVM (L0) there were no changes in MSR bitmap for L2.
> +	 */
> +	if (!vmx->nested.msr_bitmap_force_recalc && evmcs &&
> +	    evmcs->hv_enlightenments_control.msr_bitmap &&
> +	    evmcs->hv_clean_fields & HV_VMX_ENLIGHTENED_CLEAN_FIELD_MSR_BITMAP)
> +		goto out_clear_msr_bitmap_force_recalc;

Huh?  Why clear it, it's already clear.  Any reason not to simply return true?

> +
>  	if (kvm_vcpu_map(vcpu, gpa_to_gfn(vmcs12->msr_bitmap), map))
>  		return false;
>  
> @@ -700,6 +715,7 @@ static inline bool nested_vmx_prepare_msr_bitmap(struct kvm_vcpu *vcpu,
>  
>  	kvm_vcpu_unmap(vcpu, &to_vmx(vcpu)->nested.msr_bitmap_map, false);
>  
> +out_clear_msr_bitmap_force_recalc:
>  	vmx->nested.msr_bitmap_force_recalc = false;
>  
>  	return true;
> -- 
> 2.31.1
> 

Re: [PATCH v3 1/4] KVM: nVMX: Don't use Enlightened MSR Bitmap for L3

[Vitaly Kuznetsov]

Sean Christopherson <seanjc@google.com> writes:

> On Wed, Oct 13, 2021, Vitaly Kuznetsov wrote:
>> 3-level nesting is also not a very common setup nowadays.
>
> Says who? :-D
>

The one who wants to sleep well at night ;-)

>> Don't enable 'Enlightened MSR Bitmap' feature for KVM's L2s (real L3s) for
>> now.
>> 
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
>> ---
>>  arch/x86/kvm/vmx/vmx.c | 21 ++++++++++++---------
>>  1 file changed, 12 insertions(+), 9 deletions(-)
>> 
>> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
>> index 1c8b2b6e7ed9..e82cdde58119 100644
>> --- a/arch/x86/kvm/vmx/vmx.c
>> +++ b/arch/x86/kvm/vmx/vmx.c
>> @@ -2655,15 +2655,6 @@ int alloc_loaded_vmcs(struct loaded_vmcs *loaded_vmcs)
>>  		if (!loaded_vmcs->msr_bitmap)
>>  			goto out_vmcs;
>>  		memset(loaded_vmcs->msr_bitmap, 0xff, PAGE_SIZE);
>> -
>> -		if (IS_ENABLED(CONFIG_HYPERV) &&
>> -		    static_branch_unlikely(&enable_evmcs) &&
>> -		    (ms_hyperv.nested_features & HV_X64_NESTED_MSR_BITMAP)) {
>> -			struct hv_enlightened_vmcs *evmcs =
>> -				(struct hv_enlightened_vmcs *)loaded_vmcs->vmcs;
>> -
>> -			evmcs->hv_enlightenments_control.msr_bitmap = 1;
>> -		}
>>  	}
>>  
>>  	memset(&loaded_vmcs->host_state, 0, sizeof(struct vmcs_host_state));
>> @@ -6903,6 +6894,18 @@ static int vmx_create_vcpu(struct kvm_vcpu *vcpu)
>>  
>>  	vmx->loaded_vmcs = &vmx->vmcs01;
>>  
>> +	/*
>> +	 * Use Hyper-V 'Enlightened MSR Bitmap' feature when KVM runs as a
>> +	 * nested (L1) hypervisor and Hyper-V in L0 supports it.
>
> And maybe call out specifically that KVM intentionally uses this only for vmcs02?
>
>> +	 */
>> +	if (IS_ENABLED(CONFIG_HYPERV) && static_branch_unlikely(&enable_evmcs)
>> +	    && (ms_hyperv.nested_features & HV_X64_NESTED_MSR_BITMAP)) {
>
> && on the previous line, I think we'll survive the 82 char line :-)
>
>> +		struct hv_enlightened_vmcs *evmcs =
>> +			(struct hv_enlightened_vmcs *)vmx->loaded_vmcs->vmcs;
>
> Hmm, what about landing this right after vmcs01's VMCS is allocated?  It's kinda
> weird, but it makes it more obvious that ->vmcs is not NULL.  And if the cast is
> simply via a "void *" it all fits on one line.
>
> 	err = alloc_loaded_vmcs(&vmx->vmcs01);
> 	if (err < 0)
> 		goto free_pml;
>
> 	/*
> 	 * Use Hyper-V 'Enlightened MSR Bitmap' feature when KVM runs as a
> 	 * nested (L1) hypervisor and Hyper-V in L0 supports it.  Enable an
> 	 * enlightened bitmap only for vmcs01, KVM currently isn't equipped to
> 	 * realize any performance benefits from enabling it for vmcs02.
> 	 */ 
> 	if (IS_ENABLED(CONFIG_HYPERV) && static_branch_unlikely(&enable_evmcs) &&
> 	    (ms_hyperv.nested_features & HV_X64_NESTED_MSR_BITMAP)) {
> 		struct hv_enlightened_vmcs *evmcs = (void *)vmx->vmcs01.vmcs;

(void *) usually smells a bit fishy to me but it seems to fit here.

>
> 		evmcs->hv_enlightenments_control.msr_bitmap = 1;
> 	}
>

>> +
>> +		evmcs->hv_enlightenments_control.msr_bitmap = 1;
>> +	}
>> +
>>  	if (cpu_need_virtualize_apic_accesses(vcpu)) {
>>  		err = alloc_apic_access_page(vcpu->kvm);
>>  		if (err)
>> -- 
>> 2.31.1
>> 
>

-- 
Vitaly

Re: [PATCH v3 3/4] KVM: nVMX: Track whether changes in L0 require MSR bitmap for L2 to be rebuilt

[Vitaly Kuznetsov]

Sean Christopherson <seanjc@google.com> writes:

> On Wed, Oct 13, 2021, Vitaly Kuznetsov wrote:
>> diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
>> index 592217fd7d92..2cdf66e6d1b0 100644
>> --- a/arch/x86/kvm/vmx/vmx.h
>> +++ b/arch/x86/kvm/vmx/vmx.h
>> @@ -148,6 +148,15 @@ struct nested_vmx {
>>  	bool need_vmcs12_to_shadow_sync;
>>  	bool dirty_vmcs12;
>>  
>> +	/*
>> +	 * Indicates whether MSR bitmap for L2 needs to be rebuilt due to
>> +	 * changes in MSR bitmap for L1 or switching to a different L2. Note,
>> +	 * this flag can only be used reliably in conjunction with a paravirt L1
>> +	 * which informs L0 whether any changes to MSR bitmap for L2 were done
>> +	 * on its side.
>> +	 */
>> +	bool msr_bitmap_force_recalc;
>
> Belated bikeshedding...  What about need_msr_bitmap_recalc to follow the above
> need_vmcs12_to_shadow_sync?
>

'msr_bitmap_force_recalc' was suggested by Paolo but
'need_msr_bitmap_recalc' sounds equally good to me.

>> +
>>  	/*
>>  	 * Indicates lazily loaded guest state has not yet been decached from
>>  	 * vmcs02.
>> -- 
>> 2.31.1
>> 
>

-- 
Vitaly

Re: [PATCH v3 4/4] KVM: nVMX: Implement Enlightened MSR Bitmap feature

[Vitaly Kuznetsov]

Sean Christopherson <seanjc@google.com> writes:

> On Wed, Oct 13, 2021, Vitaly Kuznetsov wrote:
>> Updating MSR bitmap for L2 is not cheap and rearly needed. TLFS for Hyper-V
>> offers 'Enlightened MSR Bitmap' feature which allows L1 hypervisor to
>> inform L0 when it changes MSR bitmap, this eliminates the need to examine
>> L1's MSR bitmap for L2 every time when 'real' MSR bitmap for L2 gets
>> constructed.
>> 
>> Use 'vmx->nested.msr_bitmap_changed' flag to implement the feature.
>> 
>> Note, KVM already uses 'Enlightened MSR bitmap' feature when it runs as a
>> nested hypervisor on top of Hyper-V. The newly introduced feature is going
>> to be used by Hyper-V guests on KVM.
>> 
>> When the feature is enabled for Win10+WSL2, it shaves off around 700 CPU
>> cycles from a nested vmexit cost (tight cpuid loop test).
>> 
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> ---
>>  arch/x86/kvm/hyperv.c     |  2 ++
>>  arch/x86/kvm/vmx/nested.c | 20 ++++++++++++++++++--
>>  2 files changed, 20 insertions(+), 2 deletions(-)
>> 
>> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
>> index 6f11cda2bfa4..a00de1dbec57 100644
>> --- a/arch/x86/kvm/hyperv.c
>> +++ b/arch/x86/kvm/hyperv.c
>> @@ -2516,6 +2516,8 @@ int kvm_get_hv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid2 *cpuid,
>>  
>>  		case HYPERV_CPUID_NESTED_FEATURES:
>>  			ent->eax = evmcs_ver;
>> +			if (evmcs_ver)
>> +				ent->eax |= HV_X64_NESTED_MSR_BITMAP;
>>  
>>  			break;
>>  
>> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
>> index bf4fa63ed371..7cd0c20d4557 100644
>> --- a/arch/x86/kvm/vmx/nested.c
>> +++ b/arch/x86/kvm/vmx/nested.c
>> @@ -608,15 +608,30 @@ static inline bool nested_vmx_prepare_msr_bitmap(struct kvm_vcpu *vcpu,
>>  						 struct vmcs12 *vmcs12)
>>  {
>>  	int msr;
>> +	struct vcpu_vmx *vmx = to_vmx(vcpu);
>>  	unsigned long *msr_bitmap_l1;
>> -	unsigned long *msr_bitmap_l0 = to_vmx(vcpu)->nested.vmcs02.msr_bitmap;
>> -	struct kvm_host_map *map = &to_vmx(vcpu)->nested.msr_bitmap_map;
>> +	unsigned long *msr_bitmap_l0 = vmx->nested.vmcs02.msr_bitmap;
>> +	struct hv_enlightened_vmcs *evmcs = vmx->nested.hv_evmcs;
>> +	struct kvm_host_map *map = &vmx->nested.msr_bitmap_map;
>
> That reminds me, can my nested bitmap fixes get merged?  Superficial conflicts,
> but still conflicts that I'd rather not have to resolve :-)
>
> https://lkml.kernel.org/r/20210924204907.1111817-1-seanjc@google.com
>

From my side I can suggest to combine these two series in v4)

>>  
>>  	/* Nothing to do if the MSR bitmap is not in use.  */
>>  	if (!cpu_has_vmx_msr_bitmap() ||
>>  	    !nested_cpu_has(vmcs12, CPU_BASED_USE_MSR_BITMAPS))
>>  		return false;
>>  
>> +	/*
>> +	 * MSR bitmap update can be skipped when:
>> +	 * - MSR bitmap for L1 hasn't changed.
>> +	 * - Nested hypervisor (L1) is attempting to launch the same L2 as
>> +	 *   before.
>> +	 * - Nested hypervisor (L1) has enabled 'Enlightened MSR Bitmap' feature
>> +	 *   and tells KVM (L0) there were no changes in MSR bitmap for L2.
>> +	 */
>> +	if (!vmx->nested.msr_bitmap_force_recalc && evmcs &&
>> +	    evmcs->hv_enlightenments_control.msr_bitmap &&
>> +	    evmcs->hv_clean_fields & HV_VMX_ENLIGHTENED_CLEAN_FIELD_MSR_BITMAP)
>> +		goto out_clear_msr_bitmap_force_recalc;
>
> Huh?  Why clear it, it's already clear.  Any reason not to simply return true?
>

No need indeed, will drop in v4.

>> +
>>  	if (kvm_vcpu_map(vcpu, gpa_to_gfn(vmcs12->msr_bitmap), map))
>>  		return false;
>>  
>> @@ -700,6 +715,7 @@ static inline bool nested_vmx_prepare_msr_bitmap(struct kvm_vcpu *vcpu,
>>  
>>  	kvm_vcpu_unmap(vcpu, &to_vmx(vcpu)->nested.msr_bitmap_map, false);
>>  
>> +out_clear_msr_bitmap_force_recalc:
>>  	vmx->nested.msr_bitmap_force_recalc = false;
>>  
>>  	return true;
>> -- 
>> 2.31.1
>> 
>

-- 
Vitaly

Re: [PATCH v3 3/4] KVM: nVMX: Track whether changes in L0 require MSR bitmap for L2 to be rebuilt

[Sean Christopherson]

On Fri, Nov 05, 2021, Vitaly Kuznetsov wrote:
> Sean Christopherson <seanjc@google.com> writes:
> 
> > On Wed, Oct 13, 2021, Vitaly Kuznetsov wrote:
> >> diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> >> index 592217fd7d92..2cdf66e6d1b0 100644
> >> --- a/arch/x86/kvm/vmx/vmx.h
> >> +++ b/arch/x86/kvm/vmx/vmx.h
> >> @@ -148,6 +148,15 @@ struct nested_vmx {
> >>  	bool need_vmcs12_to_shadow_sync;
> >>  	bool dirty_vmcs12;
> >>  
> >> +	/*
> >> +	 * Indicates whether MSR bitmap for L2 needs to be rebuilt due to
> >> +	 * changes in MSR bitmap for L1 or switching to a different L2. Note,
> >> +	 * this flag can only be used reliably in conjunction with a paravirt L1
> >> +	 * which informs L0 whether any changes to MSR bitmap for L2 were done
> >> +	 * on its side.
> >> +	 */
> >> +	bool msr_bitmap_force_recalc;
> >
> > Belated bikeshedding...  What about need_msr_bitmap_recalc to follow the above
> > need_vmcs12_to_shadow_sync?
> >
> 
> 'msr_bitmap_force_recalc' was suggested by Paolo but
> 'need_msr_bitmap_recalc' sounds equally good to me.

Ah, actually, Paolo's is better.  "!need" implies that the recalc can be skipped
regardless of any other behavior, whereas "!force" provides the hint that a recalc
may still be needed for other reasons.

Can we move the "force" to the front though, i.e. force_msr_bitmap_recalc?  The
other fields in nested_vmx all have the verb at the front.

	bool need_vmcs12_to_shadow_sync;
	bool need_sync_vmcs02_to_vmcs12_rare;
	bool change_vmcs01_virtual_apic_mode;
	bool reload_vmcs01_apic_access_page;
	bool update_vmcs01_cpu_dirty_logging;

Re: [PATCH v3 1/4] KVM: nVMX: Don't use Enlightened MSR Bitmap for L3

[Maxim Levitsky]

On Fri, 2021-11-05 at 00:52 +0000, Sean Christopherson wrote:
> On Wed, Oct 13, 2021, Vitaly Kuznetsov wrote:
> > 3-level nesting is also not a very common setup nowadays.
> 
> Says who? :-D

I regularly test 4 level nesting :P
It's KVM all the way down.... 
 
But jokes aside 3 level nesting will start to happen occasionally more and more often,
IMHO with windows guests which have accidently/or on purpose enabled HypoerV/Core isolation/WSL3 inside,
and that are run nested on KVM.
 
Just FYI. I have a patch series pending (reviews are welcome!) which implement nested vVMLOAD/vVMSAVE and
vGIF which allows L1 to use these optional SVM features to run its nested guests (that is L3s) faster.
(This series is the reason I was recently stress testing 3/4 level nesting. 
 
4 levels usually work so slow that VM doesn't boot and timeouts in various systemd settings).
3rd level works not that bad IMHO.

All that said I don't have any objections to the patch itself.


Best regards,
	Maxim Levitsky
> 
> > Don't enable 'Enlightened MSR Bitmap' feature for KVM's L2s (real L3s) for
> > now.
> > 
> > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> > Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> >  arch/x86/kvm/vmx/vmx.c | 21 ++++++++++++---------
> >  1 file changed, 12 insertions(+), 9 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > index 1c8b2b6e7ed9..e82cdde58119 100644
> > --- a/arch/x86/kvm/vmx/vmx.c
> > +++ b/arch/x86/kvm/vmx/vmx.c
> > @@ -2655,15 +2655,6 @@ int alloc_loaded_vmcs(struct loaded_vmcs *loaded_vmcs)
> >  		if (!loaded_vmcs->msr_bitmap)
> >  			goto out_vmcs;
> >  		memset(loaded_vmcs->msr_bitmap, 0xff, PAGE_SIZE);
> > -
> > -		if (IS_ENABLED(CONFIG_HYPERV) &&
> > -		    static_branch_unlikely(&enable_evmcs) &&
> > -		    (ms_hyperv.nested_features & HV_X64_NESTED_MSR_BITMAP)) {
> > -			struct hv_enlightened_vmcs *evmcs =
> > -				(struct hv_enlightened_vmcs *)loaded_vmcs->vmcs;
> > -
> > -			evmcs->hv_enlightenments_control.msr_bitmap = 1;
> > -		}
> >  	}
> >  
> >  	memset(&loaded_vmcs->host_state, 0, sizeof(struct vmcs_host_state));
> > @@ -6903,6 +6894,18 @@ static int vmx_create_vcpu(struct kvm_vcpu *vcpu)
> >  
> >  	vmx->loaded_vmcs = &vmx->vmcs01;
> >  
> > +	/*
> > +	 * Use Hyper-V 'Enlightened MSR Bitmap' feature when KVM runs as a
> > +	 * nested (L1) hypervisor and Hyper-V in L0 supports it.
> 
> And maybe call out specifically that KVM intentionally uses this only for vmcs02?
> 
> > +	 */
> > +	if (IS_ENABLED(CONFIG_HYPERV) && static_branch_unlikely(&enable_evmcs)
> > +	    && (ms_hyperv.nested_features & HV_X64_NESTED_MSR_BITMAP)) {
> 
> && on the previous line, I think we'll survive the 82 char line :-)
> 
> > +		struct hv_enlightened_vmcs *evmcs =
> > +			(struct hv_enlightened_vmcs *)vmx->loaded_vmcs->vmcs;
> 
> Hmm, what about landing this right after vmcs01's VMCS is allocated?  It's kinda
> weird, but it makes it more obvious that ->vmcs is not NULL.  And if the cast is
> simply via a "void *" it all fits on one line.
> 
> 	err = alloc_loaded_vmcs(&vmx->vmcs01);
> 	if (err < 0)
> 		goto free_pml;
> 
> 	/*
> 	 * Use Hyper-V 'Enlightened MSR Bitmap' feature when KVM runs as a
> 	 * nested (L1) hypervisor and Hyper-V in L0 supports it.  Enable an
> 	 * enlightened bitmap only for vmcs01, KVM currently isn't equipped to
> 	 * realize any performance benefits from enabling it for vmcs02.
> 	 */ 
> 	if (IS_ENABLED(CONFIG_HYPERV) && static_branch_unlikely(&enable_evmcs) &&
> 	    (ms_hyperv.nested_features & HV_X64_NESTED_MSR_BITMAP)) {
> 		struct hv_enlightened_vmcs *evmcs = (void *)vmx->vmcs01.vmcs;
> 
> 		evmcs->hv_enlightenments_control.msr_bitmap = 1;
> 	}
> 
> > +
> > +		evmcs->hv_enlightenments_control.msr_bitmap = 1;
> > +	}
> > +
> >  	if (cpu_need_virtualize_apic_accesses(vcpu)) {
> >  		err = alloc_apic_access_page(vcpu->kvm);
> >  		if (err)
> > -- 
> > 2.31.1
> > 

Re: [PATCH v3 1/4] KVM: nVMX: Don't use Enlightened MSR Bitmap for L3

[Maxim Levitsky]

On Fri, 2021-11-05 at 17:38 +0200, Maxim Levitsky wrote:
> On Fri, 2021-11-05 at 00:52 +0000, Sean Christopherson wrote:
> > On Wed, Oct 13, 2021, Vitaly Kuznetsov wrote:
> > > 3-level nesting is also not a very common setup nowadays.
> > 
> > Says who? :-D
> 
> I regularly test 4 level nesting :P
> It's KVM all the way down.... 
>  
> But jokes aside 3 level nesting will start to happen occasionally more and more often,
> IMHO with windows guests which have accidently/or on purpose enabled HypoerV/Core isolation/WSL3 inside,

*insert some joke about coffee here* 

I mean HyperV/Core Isolation/WSL2. There is no WSL3 yet :)

Best regards,
	Maxim Levitsky
> and that are run nested on KVM.
>  
> Just FYI. I have a patch series pending (reviews are welcome!) which implement nested vVMLOAD/vVMSAVE and
> vGIF which allows L1 to use these optional SVM features to run its nested guests (that is L3s) faster.
> (This series is the reason I was recently stress testing 3/4 level nesting. 
>  
> 4 levels usually work so slow that VM doesn't boot and timeouts in various systemd settings).
> 3rd level works not that bad IMHO.
> 
> All that said I don't have any objections to the patch itself.
> 
> 
> Best regards,
> 	Maxim Levitsky
> > > Don't enable 'Enlightened MSR Bitmap' feature for KVM's L2s (real L3s) for
> > > now.
> > > 
> > > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> > > Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
> > > ---
> > >  arch/x86/kvm/vmx/vmx.c | 21 ++++++++++++---------
> > >  1 file changed, 12 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > > index 1c8b2b6e7ed9..e82cdde58119 100644
> > > --- a/arch/x86/kvm/vmx/vmx.c
> > > +++ b/arch/x86/kvm/vmx/vmx.c
> > > @@ -2655,15 +2655,6 @@ int alloc_loaded_vmcs(struct loaded_vmcs *loaded_vmcs)
> > >  		if (!loaded_vmcs->msr_bitmap)
> > >  			goto out_vmcs;
> > >  		memset(loaded_vmcs->msr_bitmap, 0xff, PAGE_SIZE);
> > > -
> > > -		if (IS_ENABLED(CONFIG_HYPERV) &&
> > > -		    static_branch_unlikely(&enable_evmcs) &&
> > > -		    (ms_hyperv.nested_features & HV_X64_NESTED_MSR_BITMAP)) {
> > > -			struct hv_enlightened_vmcs *evmcs =
> > > -				(struct hv_enlightened_vmcs *)loaded_vmcs->vmcs;
> > > -
> > > -			evmcs->hv_enlightenments_control.msr_bitmap = 1;
> > > -		}
> > >  	}
> > >  
> > >  	memset(&loaded_vmcs->host_state, 0, sizeof(struct vmcs_host_state));
> > > @@ -6903,6 +6894,18 @@ static int vmx_create_vcpu(struct kvm_vcpu *vcpu)
> > >  
> > >  	vmx->loaded_vmcs = &vmx->vmcs01;
> > >  
> > > +	/*
> > > +	 * Use Hyper-V 'Enlightened MSR Bitmap' feature when KVM runs as a
> > > +	 * nested (L1) hypervisor and Hyper-V in L0 supports it.
> > 
> > And maybe call out specifically that KVM intentionally uses this only for vmcs02?
> > 
> > > +	 */
> > > +	if (IS_ENABLED(CONFIG_HYPERV) && static_branch_unlikely(&enable_evmcs)
> > > +	    && (ms_hyperv.nested_features & HV_X64_NESTED_MSR_BITMAP)) {
> > 
> > && on the previous line, I think we'll survive the 82 char line :-)
> > 
> > > +		struct hv_enlightened_vmcs *evmcs =
> > > +			(struct hv_enlightened_vmcs *)vmx->loaded_vmcs->vmcs;
> > 
> > Hmm, what about landing this right after vmcs01's VMCS is allocated?  It's kinda
> > weird, but it makes it more obvious that ->vmcs is not NULL.  And if the cast is
> > simply via a "void *" it all fits on one line.
> > 
> > 	err = alloc_loaded_vmcs(&vmx->vmcs01);
> > 	if (err < 0)
> > 		goto free_pml;
> > 
> > 	/*
> > 	 * Use Hyper-V 'Enlightened MSR Bitmap' feature when KVM runs as a
> > 	 * nested (L1) hypervisor and Hyper-V in L0 supports it.  Enable an
> > 	 * enlightened bitmap only for vmcs01, KVM currently isn't equipped to
> > 	 * realize any performance benefits from enabling it for vmcs02.
> > 	 */ 
> > 	if (IS_ENABLED(CONFIG_HYPERV) && static_branch_unlikely(&enable_evmcs) &&
> > 	    (ms_hyperv.nested_features & HV_X64_NESTED_MSR_BITMAP)) {
> > 		struct hv_enlightened_vmcs *evmcs = (void *)vmx->vmcs01.vmcs;
> > 
> > 		evmcs->hv_enlightenments_control.msr_bitmap = 1;
> > 	}
> > 
> > > +
> > > +		evmcs->hv_enlightenments_control.msr_bitmap = 1;
> > > +	}
> > > +
> > >  	if (cpu_need_virtualize_apic_accesses(vcpu)) {
> > >  		err = alloc_apic_access_page(vcpu->kvm);
> > >  		if (err)
> > > -- 
> > > 2.31.1
> > >