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

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

[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.

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 | 28 +++++++++++++++++++++++---
 arch/x86/kvm/vmx/vmx.c    | 41 ++++++++++++++++++++++++++-------------
 arch/x86/kvm/vmx/vmx.h    |  6 ++++++
 4 files changed, 61 insertions(+), 16 deletions(-)

-- 
2.31.1

[PATCH 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>
---
 arch/x86/kvm/vmx/vmx.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 0c2c0d5ae873..ae470afcb699 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -2654,15 +2654,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));
@@ -6861,6 +6852,19 @@ 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;
+	}
+
 	cpu = get_cpu();
 	vmx_vcpu_load(vcpu, cpu);
 	vcpu->cpu = cpu;
-- 
2.31.1

[PATCH 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>
---
 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 ae470afcb699..ad33032e8588 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -3725,6 +3725,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);
@@ -3733,8 +3744,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
@@ -3778,8 +3788,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 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    | 6 ++++++
 3 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index ccb03d69546c..42cd95611892 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -2053,10 +2053,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_changed = true;
+	}
+
 	return EVMPTRLD_SUCCEEDED;
 }
 
@@ -3240,6 +3243,8 @@ static bool nested_get_vmcs12_pages(struct kvm_vcpu *vcpu)
 	else
 		exec_controls_clearbit(vmx, CPU_BASED_USE_MSR_BITMAPS);
 
+	vmx->nested.msr_bitmap_changed = false;
+
 	return true;
 }
 
@@ -5273,6 +5278,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_changed = true;
 }
 
 /* Emulate the VMPTRLD instruction */
@@ -6393,6 +6399,7 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu,
 		goto error_guest_mode;
 
 	vmx->nested.dirty_vmcs12 = true;
+	vmx->nested.msr_bitmap_changed = 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 ad33032e8588..2dbfb5d838db 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -3734,6 +3734,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_changed = 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 4858c5fd95f2..b6596fc2943a 100644
--- a/arch/x86/kvm/vmx/vmx.h
+++ b/arch/x86/kvm/vmx/vmx.h
@@ -148,6 +148,12 @@ 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.
+	 */
+	bool msr_bitmap_changed;
+
 	/*
 	 * Indicates lazily loaded guest state has not yet been decached from
 	 * vmcs02.
-- 
2.31.1

[PATCH 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 | 19 +++++++++++++++++--
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index 232a86a6faaf..7124dbe79ac2 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -2517,6 +2517,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 42cd95611892..5ac5ba2f6191 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -607,15 +607,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_changed && evmcs &&
+	    evmcs->hv_enlightenments_control.msr_bitmap &&
+	    evmcs->hv_clean_fields & HV_VMX_ENLIGHTENED_CLEAN_FIELD_MSR_BITMAP)
+		return true;
+
 	if (kvm_vcpu_map(vcpu, gpa_to_gfn(vmcs12->msr_bitmap), map))
 		return false;
 
-- 
2.31.1

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

[Maxim Levitsky]

On Fri, 2021-09-10 at 18:06 +0200, Vitaly Kuznetsov wrote:
> 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>
> ---
>  arch/x86/kvm/vmx/vmx.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 0c2c0d5ae873..ae470afcb699 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -2654,15 +2654,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));
> @@ -6861,6 +6852,19 @@ 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;
> +	}
> +
>  	cpu = get_cpu();
>  	vmx_vcpu_load(vcpu, cpu);
>  	vcpu->cpu = cpu;

Makes sense.

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>


However, just a note that it is very very confusing that KVM can use eVMCS in both ways.
 
 
'Client': It can both run under HyperV, and thus take advantage of eVMCS when it runs its guests (with
help of
HyperV)
 
'Server' KVM can emulate some HyperV features, and one of these is eVMCS, thus a windows guest running
under KVM, can use KVM's eVMCS implementation to run nested guests.
 
This patch fails under
'Client', while the other patches in the series fall under the 'Server' category,
and even more confusing, the patch 2 moves 'Client' code around, but it is intended for following patches
3,4 which are
for Server.
 

Thus this patch probably should be a separate patch, just to avoid confusion.

However, since this patch series is already posted, and I figured that out, and hopefully explained it here,
no need to do anything though!


Best regards,
	Maxim Levitsky

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

[Maxim Levitsky]

On Fri, 2021-09-10 at 18:06 +0200, 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>
> ---
>  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 ae470afcb699..ad33032e8588 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -3725,6 +3725,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);
> @@ -3733,8 +3744,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
> @@ -3778,8 +3788,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
Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Best regards,
	Maxim Levitsky

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

[Maxim Levitsky]

On Fri, 2021-09-10 at 18:06 +0200, Vitaly Kuznetsov wrote:
> 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    | 6 ++++++
>  3 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index ccb03d69546c..42cd95611892 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -2053,10 +2053,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_changed = true;
> +	}
> +
>  	return EVMPTRLD_SUCCEEDED;
>  }
>  
> @@ -3240,6 +3243,8 @@ static bool nested_get_vmcs12_pages(struct kvm_vcpu *vcpu)
>  	else
>  		exec_controls_clearbit(vmx, CPU_BASED_USE_MSR_BITMAPS);
>  
> +	vmx->nested.msr_bitmap_changed = false;

Very minor nitpick: Maybe I would put this into nested_vmx_prepare_msr_bitmap,
a bit closer to the action, but this is fine like this as well.

> +
>  	return true;
>  }
>  
> @@ -5273,6 +5278,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_changed = true;
>  }
>  
>  /* Emulate the VMPTRLD instruction */
> @@ -6393,6 +6399,7 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu,
>  		goto error_guest_mode;
>  
>  	vmx->nested.dirty_vmcs12 = true;
> +	vmx->nested.msr_bitmap_changed = true;

Is this needed? Setting the nested state should eventually trigger call to
nested_vmx_handle_enlightened_vmptrld.

 

>  	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 ad33032e8588..2dbfb5d838db 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -3734,6 +3734,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_changed = 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 4858c5fd95f2..b6596fc2943a 100644
> --- a/arch/x86/kvm/vmx/vmx.h
> +++ b/arch/x86/kvm/vmx/vmx.h
> @@ -148,6 +148,12 @@ 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.
> +	 */
> +	bool msr_bitmap_changed;
> +
>  	/*
>  	 * Indicates lazily loaded guest state has not yet been decached from
>  	 * vmcs02.


Best regards,
	Maxim Levitsky

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

[Maxim Levitsky]

On Fri, 2021-09-10 at 18:06 +0200, 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 | 19 +++++++++++++++++--
>  2 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> index 232a86a6faaf..7124dbe79ac2 100644
> --- a/arch/x86/kvm/hyperv.c
> +++ b/arch/x86/kvm/hyperv.c
> @@ -2517,6 +2517,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 42cd95611892..5ac5ba2f6191 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -607,15 +607,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_changed && evmcs &&
> +	    evmcs->hv_enlightenments_control.msr_bitmap &&
> +	    evmcs->hv_clean_fields & HV_VMX_ENLIGHTENED_CLEAN_FIELD_MSR_BITMAP)
> +		return true;
> +
>  	if (kvm_vcpu_map(vcpu, gpa_to_gfn(vmcs12->msr_bitmap), map))
>  		return false;
>  

I am not an expert on HyperV, but it looks OK to me.

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Best regards,
	Maxim Levitsky

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

[Vitaly Kuznetsov]

Maxim Levitsky <mlevitsk@redhat.com> writes:

> On Fri, 2021-09-10 at 18:06 +0200, Vitaly Kuznetsov wrote:
>> 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>
>> ---
>>  arch/x86/kvm/vmx/vmx.c | 22 +++++++++++++---------
>>  1 file changed, 13 insertions(+), 9 deletions(-)
>> 
>> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
>> index 0c2c0d5ae873..ae470afcb699 100644
>> --- a/arch/x86/kvm/vmx/vmx.c
>> +++ b/arch/x86/kvm/vmx/vmx.c
>> @@ -2654,15 +2654,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));
>> @@ -6861,6 +6852,19 @@ 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;
>> +	}
>> +
>>  	cpu = get_cpu();
>>  	vmx_vcpu_load(vcpu, cpu);
>>  	vcpu->cpu = cpu;
>
> Makes sense.
>
> Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
>
>
> However, just a note that it is very very confusing that KVM can use eVMCS in both ways.
>  
>  
> 'Client': It can both run under HyperV, and thus take advantage of eVMCS when it runs its guests (with
> help of
> HyperV)
>  
> 'Server' KVM can emulate some HyperV features, and one of these is eVMCS, thus a windows guest running
> under KVM, can use KVM's eVMCS implementation to run nested guests.
>  
> This patch fails under
> 'Client', while the other patches in the series fall under the 'Server' category,
> and even more confusing, the patch 2 moves 'Client' code around, but it is intended for following patches
> 3,4 which are
> for Server.
>  

All this is confusing indeed, KVM-on-Hyper-V and Hyper-V-on-KVM are two
different beasts but it's not always clear from patch subject. I was
thinking about adding this to patch prexes:

"KVM: VMX: KVM-on-Hyper-V: ... " 
"KVM: nVMX: Hyper-V-on-KVM ..."

or something similar.

>
> Thus this patch probably should be a separate patch, just to avoid confusion.
>

This patch is a weird one. We actually fix

Hyper-V-on-KVM-on-Hyper-V case.

Don't get confused! :-)


> However, since this patch series is already posted, and I figured that out, and hopefully explained it here,
> no need to do anything though!
>
>
> Best regards,
> 	Maxim Levitsky
>
>
>

-- 
Vitaly

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

[Vitaly Kuznetsov]

Maxim Levitsky <mlevitsk@redhat.com> writes:

> On Fri, 2021-09-10 at 18:06 +0200, Vitaly Kuznetsov wrote:
>> 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    | 6 ++++++
>>  3 files changed, 16 insertions(+), 1 deletion(-)
>> 
>> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
>> index ccb03d69546c..42cd95611892 100644
>> --- a/arch/x86/kvm/vmx/nested.c
>> +++ b/arch/x86/kvm/vmx/nested.c
>> @@ -2053,10 +2053,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_changed = true;
>> +	}
>> +
>>  	return EVMPTRLD_SUCCEEDED;
>>  }
>>  
>> @@ -3240,6 +3243,8 @@ static bool nested_get_vmcs12_pages(struct kvm_vcpu *vcpu)
>>  	else
>>  		exec_controls_clearbit(vmx, CPU_BASED_USE_MSR_BITMAPS);
>>  
>> +	vmx->nested.msr_bitmap_changed = false;
>
> Very minor nitpick: Maybe I would put this into nested_vmx_prepare_msr_bitmap,
> a bit closer to the action, but this is fine like this as well.
>

I don't have a strong preference here, can move it to nested_vmx_prepare_msr_bitmap().

>> +
>>  	return true;
>>  }
>>  
>> @@ -5273,6 +5278,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_changed = true;
>>  }
>>  
>>  /* Emulate the VMPTRLD instruction */
>> @@ -6393,6 +6399,7 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu,
>>  		goto error_guest_mode;
>>  
>>  	vmx->nested.dirty_vmcs12 = true;
>> +	vmx->nested.msr_bitmap_changed = true;
>
> Is this needed? Setting the nested state should eventually trigger call to
> nested_vmx_handle_enlightened_vmptrld.
>
>  

Strictly speaking - no (meaning that nothing is going to change if we
just drop this hunk). My intention was to keep tracking information
complete: after vmx_set_nested_state() we certainly need to re-build MSR
Bitmap 02 and that's what 'msr_bitmap_changed' tracks. We can replace
this with a comment if needed (but I'd slightly prefer to keep it -
unless there's a reason not to).

>
>>  	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 ad33032e8588..2dbfb5d838db 100644
>> --- a/arch/x86/kvm/vmx/vmx.c
>> +++ b/arch/x86/kvm/vmx/vmx.c
>> @@ -3734,6 +3734,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_changed = 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 4858c5fd95f2..b6596fc2943a 100644
>> --- a/arch/x86/kvm/vmx/vmx.h
>> +++ b/arch/x86/kvm/vmx/vmx.h
>> @@ -148,6 +148,12 @@ 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.
>> +	 */
>> +	bool msr_bitmap_changed;
>> +
>>  	/*
>>  	 * Indicates lazily loaded guest state has not yet been decached from
>>  	 * vmcs02.
>
>
> Best regards,
> 	Maxim Levitsky
>

-- 
Vitaly

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

[Maxim Levitsky]

On Mon, 2021-09-13 at 08:53 +0200, Vitaly Kuznetsov wrote:
> Maxim Levitsky <mlevitsk@redhat.com> writes:
> 
> > On Fri, 2021-09-10 at 18:06 +0200, Vitaly Kuznetsov wrote:
> > > 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>
> > > ---
> > >  arch/x86/kvm/vmx/vmx.c | 22 +++++++++++++---------
> > >  1 file changed, 13 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > > index 0c2c0d5ae873..ae470afcb699 100644
> > > --- a/arch/x86/kvm/vmx/vmx.c
> > > +++ b/arch/x86/kvm/vmx/vmx.c
> > > @@ -2654,15 +2654,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));
> > > @@ -6861,6 +6852,19 @@ 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;
> > > +	}
> > > +
> > >  	cpu = get_cpu();
> > >  	vmx_vcpu_load(vcpu, cpu);
> > >  	vcpu->cpu = cpu;
> > 
> > Makes sense.
> > 
> > Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
> > 
> > 
> > However, just a note that it is very very confusing that KVM can use eVMCS in both ways.
> >  
> >  
> > 'Client': It can both run under HyperV, and thus take advantage of eVMCS when it runs its guests (with
> > help of
> > HyperV)
> >  
> > 'Server' KVM can emulate some HyperV features, and one of these is eVMCS, thus a windows guest running
> > under KVM, can use KVM's eVMCS implementation to run nested guests.
> >  
> > This patch fails under
> > 'Client', while the other patches in the series fall under the 'Server' category,
> > and even more confusing, the patch 2 moves 'Client' code around, but it is intended for following patches
> > 3,4 which are
> > for Server.
> >  
> 
> All this is confusing indeed, KVM-on-Hyper-V and Hyper-V-on-KVM are two
> different beasts but it's not always clear from patch subject. I was
> thinking about adding this to patch prexes:
> 
> "KVM: VMX: KVM-on-Hyper-V: ... " 
> "KVM: nVMX: Hyper-V-on-KVM ..."

Makes sense!
> 
> or something similar.
> 
> > Thus this patch probably should be a separate patch, just to avoid confusion.
> > 
> 
> This patch is a weird one. We actually fix
> 
> Hyper-V-on-KVM-on-Hyper-V case.
> 
> Don't get confused! :-)

Ah right! 

This reminds me of these double windows with an air gap in between two glass layers :-)


Best regards,
	Maxim Levitsky

> 
> 
> > However, since this patch series is already posted, and I figured that out, and hopefully explained it here,
> > no need to do anything though!
> > 
> > 
> > Best regards,
> > 	Maxim Levitsky
> > 
> > 
> > 

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

[Maxim Levitsky]

On Mon, 2021-09-13 at 08:57 +0200, Vitaly Kuznetsov wrote:
> Maxim Levitsky <mlevitsk@redhat.com> writes:
> 
> > On Fri, 2021-09-10 at 18:06 +0200, Vitaly Kuznetsov wrote:
> > > 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    | 6 ++++++
> > >  3 files changed, 16 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> > > index ccb03d69546c..42cd95611892 100644
> > > --- a/arch/x86/kvm/vmx/nested.c
> > > +++ b/arch/x86/kvm/vmx/nested.c
> > > @@ -2053,10 +2053,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_changed = true;
> > > +	}
> > > +
> > >  	return EVMPTRLD_SUCCEEDED;
> > >  }
> > >  
> > > @@ -3240,6 +3243,8 @@ static bool nested_get_vmcs12_pages(struct kvm_vcpu *vcpu)
> > >  	else
> > >  		exec_controls_clearbit(vmx, CPU_BASED_USE_MSR_BITMAPS);
> > >  
> > > +	vmx->nested.msr_bitmap_changed = false;
> > 
> > Very minor nitpick: Maybe I would put this into nested_vmx_prepare_msr_bitmap,
> > a bit closer to the action, but this is fine like this as well.
> > 
> 
> I don't have a strong preference here, can move it to nested_vmx_prepare_msr_bitmap().
> 
> > > +
> > >  	return true;
> > >  }
> > >  
> > > @@ -5273,6 +5278,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_changed = true;
> > >  }
> > >  
> > >  /* Emulate the VMPTRLD instruction */
> > > @@ -6393,6 +6399,7 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu,
> > >  		goto error_guest_mode;
> > >  
> > >  	vmx->nested.dirty_vmcs12 = true;
> > > +	vmx->nested.msr_bitmap_changed = true;
> > 
> > Is this needed? Setting the nested state should eventually trigger call to
> > nested_vmx_handle_enlightened_vmptrld.
> > 
> >  
> 
> Strictly speaking - no (meaning that nothing is going to change if we
> just drop this hunk). My intention was to keep tracking information
> complete: after vmx_set_nested_state() we certainly need to re-build MSR
> Bitmap 02 and that's what 'msr_bitmap_changed' tracks. We can replace
> this with a comment if needed (but I'd slightly prefer to keep it -
> unless there's a reason not to).

Makes sense. let it be like that.

Best regards,
	Maxim Levitsky


> 
> > >  	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 ad33032e8588..2dbfb5d838db 100644
> > > --- a/arch/x86/kvm/vmx/vmx.c
> > > +++ b/arch/x86/kvm/vmx/vmx.c
> > > @@ -3734,6 +3734,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_changed = 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 4858c5fd95f2..b6596fc2943a 100644
> > > --- a/arch/x86/kvm/vmx/vmx.h
> > > +++ b/arch/x86/kvm/vmx/vmx.h
> > > @@ -148,6 +148,12 @@ 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.
> > > +	 */
> > > +	bool msr_bitmap_changed;
> > > +
> > >  	/*
> > >  	 * Indicates lazily loaded guest state has not yet been decached from
> > >  	 * vmcs02.
> > 
> > Best regards,
> > 	Maxim Levitsky
> >