Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 14 Aug 2023 16:11:05 +0800
From:   Yuan Yao <yuan.yao@linux.intel.com>
To:     Sean Christopherson <seanjc@google.com>
Cc:     Paolo Bonzini <pbonzini@redhat.com>,
        Vitaly Kuznetsov <vkuznets@redhat.com>, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org, Maxim Levitsky <mlevitsk@redhat.com>
Subject: Re: [PATCH v2 13/21] KVM: nVMX: Use KVM-governed feature framework
 to track "nested VMX enabled"
Message-ID: <20230814081105.yr6bamyoutijc36i@yy-desk-7060>
References: <20230729011608.1065019-1-seanjc@google.com>
 <20230729011608.1065019-14-seanjc@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230729011608.1065019-14-seanjc@google.com>
User-Agent: NeoMutt/20171215
Precedence: bulk

On Fri, Jul 28, 2023 at 06:16:00PM -0700, Sean Christopherson wrote:
> Track "VMX exposed to L1" via a governed feature flag instead of using a
> dedicated helper to provide the same functionality.  The main goal is to
> drive convergence between VMX and SVM with respect to querying features
> that are controllable via module param (SVM likes to cache nested
> features), avoiding the guest CPUID lookups at runtime is just a bonus
> and unlikely to provide any meaningful performance benefits.
>
> No functional change intended.
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/governed_features.h |  1 +
>  arch/x86/kvm/vmx/nested.c        |  7 ++++---
>  arch/x86/kvm/vmx/vmx.c           | 21 ++++++---------------
>  arch/x86/kvm/vmx/vmx.h           |  1 -
>  4 files changed, 11 insertions(+), 19 deletions(-)
>
> diff --git a/arch/x86/kvm/governed_features.h b/arch/x86/kvm/governed_features.h
> index b896a64e4ac3..22446614bf49 100644
> --- a/arch/x86/kvm/governed_features.h
> +++ b/arch/x86/kvm/governed_features.h
> @@ -7,6 +7,7 @@ BUILD_BUG()
>
>  KVM_GOVERNED_X86_FEATURE(GBPAGES)
>  KVM_GOVERNED_X86_FEATURE(XSAVES)
> +KVM_GOVERNED_X86_FEATURE(VMX)
>
>  #undef KVM_GOVERNED_X86_FEATURE
>  #undef KVM_GOVERNED_FEATURE
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 22e08d30baef..c5ec0ef51ff7 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -6426,7 +6426,7 @@ static int vmx_get_nested_state(struct kvm_vcpu *vcpu,
>  	vmx = to_vmx(vcpu);
>  	vmcs12 = get_vmcs12(vcpu);
>
> -	if (nested_vmx_allowed(vcpu) &&
> +	if (guest_can_use(vcpu, X86_FEATURE_VMX) &&
>  	    (vmx->nested.vmxon || vmx->nested.smm.vmxon)) {
>  		kvm_state.hdr.vmx.vmxon_pa = vmx->nested.vmxon_ptr;
>  		kvm_state.hdr.vmx.vmcs12_pa = vmx->nested.current_vmptr;
> @@ -6567,7 +6567,7 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu,
>  		if (kvm_state->flags & ~KVM_STATE_NESTED_EVMCS)
>  			return -EINVAL;
>  	} else {
> -		if (!nested_vmx_allowed(vcpu))
> +		if (!guest_can_use(vcpu, X86_FEATURE_VMX))
>  			return -EINVAL;
>
>  		if (!page_address_valid(vcpu, kvm_state->hdr.vmx.vmxon_pa))
> @@ -6601,7 +6601,8 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu,
>  		return -EINVAL;
>
>  	if ((kvm_state->flags & KVM_STATE_NESTED_EVMCS) &&
> -		(!nested_vmx_allowed(vcpu) || !vmx->nested.enlightened_vmcs_enabled))
> +	    (!guest_can_use(vcpu, X86_FEATURE_VMX) ||
> +	     !vmx->nested.enlightened_vmcs_enabled))
>  			return -EINVAL;
>
>  	vmx_leave_nested(vcpu);
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 3100ed62615c..fdf932cfc64d 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -1894,17 +1894,6 @@ static void vmx_write_tsc_multiplier(struct kvm_vcpu *vcpu)
>  	vmcs_write64(TSC_MULTIPLIER, vcpu->arch.tsc_scaling_ratio);
>  }
>
> -/*
> - * nested_vmx_allowed() checks whether a guest should be allowed to use VMX
> - * instructions and MSRs (i.e., nested VMX). Nested VMX is disabled for
> - * all guests if the "nested" module option is off, and can also be disabled
> - * for a single guest by disabling its VMX cpuid bit.
> - */
> -bool nested_vmx_allowed(struct kvm_vcpu *vcpu)
> -{
> -	return nested && guest_cpuid_has(vcpu, X86_FEATURE_VMX);

the removed nested here already covered by
kvm_cpu_cap_set(X86_FEATURE_VMX) from vmx_set_cpu_caps().

Reviewed-by: Yuan Yao <yuan.yao@intel.com>

> -}
> -
>  /*
>   * Userspace is allowed to set any supported IA32_FEATURE_CONTROL regardless of
>   * guest CPUID.  Note, KVM allows userspace to set "VMX in SMX" to maintain
> @@ -2032,7 +2021,7 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  			[msr_info->index - MSR_IA32_SGXLEPUBKEYHASH0];
>  		break;
>  	case KVM_FIRST_EMULATED_VMX_MSR ... KVM_LAST_EMULATED_VMX_MSR:
> -		if (!nested_vmx_allowed(vcpu))
> +		if (!guest_can_use(vcpu, X86_FEATURE_VMX))
>  			return 1;
>  		if (vmx_get_vmx_msr(&vmx->nested.msrs, msr_info->index,
>  				    &msr_info->data))
> @@ -2340,7 +2329,7 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  	case KVM_FIRST_EMULATED_VMX_MSR ... KVM_LAST_EMULATED_VMX_MSR:
>  		if (!msr_info->host_initiated)
>  			return 1; /* they are read-only */
> -		if (!nested_vmx_allowed(vcpu))
> +		if (!guest_can_use(vcpu, X86_FEATURE_VMX))
>  			return 1;
>  		return vmx_set_vmx_msr(vcpu, msr_index, data);
>  	case MSR_IA32_RTIT_CTL:
> @@ -7727,13 +7716,15 @@ static void vmx_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
>  	    guest_cpuid_has(vcpu, X86_FEATURE_XSAVE))
>  		kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_XSAVES);
>
> +	kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_VMX);
> +
>  	vmx_setup_uret_msrs(vmx);
>
>  	if (cpu_has_secondary_exec_ctrls())
>  		vmcs_set_secondary_exec_control(vmx,
>  						vmx_secondary_exec_control(vmx));
>
> -	if (nested_vmx_allowed(vcpu))
> +	if (guest_can_use(vcpu, X86_FEATURE_VMX))
>  		vmx->msr_ia32_feature_control_valid_bits |=
>  			FEAT_CTL_VMX_ENABLED_INSIDE_SMX |
>  			FEAT_CTL_VMX_ENABLED_OUTSIDE_SMX;
> @@ -7742,7 +7733,7 @@ static void vmx_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
>  			~(FEAT_CTL_VMX_ENABLED_INSIDE_SMX |
>  			  FEAT_CTL_VMX_ENABLED_OUTSIDE_SMX);
>
> -	if (nested_vmx_allowed(vcpu))
> +	if (guest_can_use(vcpu, X86_FEATURE_VMX))
>  		nested_vmx_cr_fixed1_bits_update(vcpu);
>
>  	if (boot_cpu_has(X86_FEATURE_INTEL_PT) &&
> diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> index cde902b44d97..c2130d2c8e24 100644
> --- a/arch/x86/kvm/vmx/vmx.h
> +++ b/arch/x86/kvm/vmx/vmx.h
> @@ -374,7 +374,6 @@ struct kvm_vmx {
>  	u64 *pid_table;
>  };
>
> -bool nested_vmx_allowed(struct kvm_vcpu *vcpu);
>  void vmx_vcpu_load_vmcs(struct kvm_vcpu *vcpu, int cpu,
>  			struct loaded_vmcs *buddy);
>  int allocate_vpid(void);
> --
> 2.41.0.487.g6d72f3e995-goog
>