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@xxxxxxxxxx>
> ---
>  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@xxxxxxxxxx>


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