Re: [RFC PATCH v6 06/36] KVM: nVMX: Treat eVMCS as enabled for guest iff Hyper-V is also enabled

[Vitaly Kuznetsov <vkuznets@redhat.com>]

Sean Christopherson <seanjc@google.com> writes:

> When querying whether or not eVMCS is enabled on behalf of the guest,
> treat eVMCS as enable if and only if Hyper-V is enabled/exposed to the
> guest.
>
> Note, flows that come from the host, e.g. KVM_SET_NESTED_STATE, must NOT
> check for Hyper-V being enabled as KVM doesn't require guest CPUID to be
> set before most ioctls().
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/vmx/evmcs.c  |  3 +++
>  arch/x86/kvm/vmx/nested.c |  8 ++++----
>  arch/x86/kvm/vmx/vmx.c    |  3 +--
>  arch/x86/kvm/vmx/vmx.h    | 10 ++++++++++
>  4 files changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/evmcs.c b/arch/x86/kvm/vmx/evmcs.c
> index 6a61b1ae7942..9139c70b6008 100644
> --- a/arch/x86/kvm/vmx/evmcs.c
> +++ b/arch/x86/kvm/vmx/evmcs.c
> @@ -334,6 +334,9 @@ uint16_t nested_get_evmcs_version(struct kvm_vcpu *vcpu)
>  	 * versions: lower 8 bits is the minimal version, higher 8 bits is the
>  	 * maximum supported version. KVM supports versions from 1 to
>  	 * KVM_EVMCS_VERSION.
> +	 *
> +	 * Note, do not check the Hyper-V is fully enabled in guest CPUID, this
> +	 * helper is used to _get_ the vCPU's supported CPUID.
>  	 */
>  	if (kvm_cpu_cap_get(X86_FEATURE_VMX) &&
>  	    (!vcpu || to_vmx(vcpu)->nested.enlightened_vmcs_enabled))
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index ddd4367d4826..28f9d64851b3 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -1982,7 +1982,7 @@ static enum nested_evmptrld_status nested_vmx_handle_enlightened_vmptrld(
>  	bool evmcs_gpa_changed = false;
>  	u64 evmcs_gpa;
>  
> -	if (likely(!vmx->nested.enlightened_vmcs_enabled))
> +	if (likely(!guest_cpuid_has_evmcs(vcpu)))
>  		return EVMPTRLD_DISABLED;
>  
>  	if (!nested_enlightened_vmentry(vcpu, &evmcs_gpa)) {
> @@ -2863,7 +2863,7 @@ static int nested_vmx_check_controls(struct kvm_vcpu *vcpu,
>  	    nested_check_vm_entry_controls(vcpu, vmcs12))
>  		return -EINVAL;
>  
> -	if (to_vmx(vcpu)->nested.enlightened_vmcs_enabled)
> +	if (guest_cpuid_has_evmcs(vcpu))
>  		return nested_evmcs_check_controls(vmcs12);
>  
>  	return 0;
> @@ -3145,7 +3145,7 @@ static bool nested_get_evmcs_page(struct kvm_vcpu *vcpu)
>  	 * L2 was running), map it here to make sure vmcs12 changes are
>  	 * properly reflected.
>  	 */
> -	if (vmx->nested.enlightened_vmcs_enabled &&
> +	if (guest_cpuid_has_evmcs(vcpu) &&
>  	    vmx->nested.hv_evmcs_vmptr == EVMPTR_MAP_PENDING) {
>  		enum nested_evmptrld_status evmptrld_status =
>  			nested_vmx_handle_enlightened_vmptrld(vcpu, false);
> @@ -5067,7 +5067,7 @@ static int handle_vmclear(struct kvm_vcpu *vcpu)
>  	 * state. It is possible that the area will stay mapped as
>  	 * vmx->nested.hv_evmcs but this shouldn't be a problem.
>  	 */
> -	if (likely(!vmx->nested.enlightened_vmcs_enabled ||
> +	if (likely(!guest_cpuid_has_evmcs(vcpu) ||
>  		   !nested_enlightened_vmentry(vcpu, &evmcs_gpa))) {
>  		if (vmptr == vmx->nested.current_vmptr)
>  			nested_release_vmcs12(vcpu);
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index c9b49a09e6b5..d4ed802947d7 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -1930,8 +1930,7 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  		 * sanity checking and refuse to boot. Filter all unsupported
>  		 * features out.
>  		 */
> -		if (!msr_info->host_initiated &&
> -		    vmx->nested.enlightened_vmcs_enabled)
> +		if (!msr_info->host_initiated && guest_cpuid_has_evmcs(vcpu))
>  			nested_evmcs_filter_control_msr(msr_info->index,
>  							&msr_info->data);
>  		break;
> diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> index 24d58c2ffaa3..35c7e6aef301 100644
> --- a/arch/x86/kvm/vmx/vmx.h
> +++ b/arch/x86/kvm/vmx/vmx.h
> @@ -626,4 +626,14 @@ static inline bool vmx_can_use_ipiv(struct kvm_vcpu *vcpu)
>  	return  lapic_in_kernel(vcpu) && enable_ipiv;
>  }
>  
> +static inline bool guest_cpuid_has_evmcs(struct kvm_vcpu *vcpu)
> +{
> +	/*
> +	 * eVMCS is exposed to the guest if Hyper-V is enabled in CPUID and
> +	 * eVMCS has been explicitly enabled by userspace.
> +	 */
> +	return vcpu->arch.hyperv_enabled &&
> +	       to_vmx(vcpu)->nested.enlightened_vmcs_enabled;

I don't quite like 'guest_cpuid_has_evmcs' name as it makes me think
we're checking if eVMCS was exposed in guest CPUID but in fact we don't
do that. eVMCS can be enabled on a vCPU even if it is not exposed in
CPUID (and we should probably keep that to not mandate setting CPUID
before enabling eVMCS).

What about e.g. vcpu_has_evmcs_enabled() instead?

On a related not, any reason to put this to vmx/vmx.h and not
vmx/evmcs.h?


> +}
> +
>  #endif /* __KVM_X86_VMX_H */

-- 
Vitaly