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

[Maxim Levitsky <mlevitsk@redhat.com>]

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