Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Wed, 16 Aug 2023 10:26:03 +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, Zeng Guang <guang.zeng@intel.com>,
        Yuan Yao <yuan.yao@intel.com>
Subject: Re: [PATCH v3 03/15] KVM: VMX: Recompute "XSAVES enabled" only after
 CPUID update
Message-ID: <20230816022603.lxbpf22auv7wc3f6@yy-desk-7060>
References: <20230815203653.519297-1-seanjc@google.com>
 <20230815203653.519297-4-seanjc@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230815203653.519297-4-seanjc@google.com>
User-Agent: NeoMutt/20171215
Precedence: bulk

On Tue, Aug 15, 2023 at 01:36:41PM -0700, Sean Christopherson wrote:
> Recompute whether or not XSAVES is enabled for the guest only if the
> guest's CPUID model changes instead of redoing the computation every time
> KVM generates vmcs01's secondary execution controls.  The boot_cpu_has()
> and cpu_has_vmx_xsaves() checks should never change after KVM is loaded,
> and if they do the kernel/KVM is hosed.
>
> Opportunistically add a comment explaining _why_ XSAVES is effectively
> exposed to the guest if and only if XSAVE is also exposed to the guest.
>
> Practically speaking, no functional change intended (KVM will do fewer
> computations, but should still see the same xsaves_enabled value whenever
> KVM looks at it).
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/vmx/vmx.c | 24 +++++++++++-------------
>  1 file changed, 11 insertions(+), 13 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 434bf524e712..1bf85bd53416 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -4612,19 +4612,10 @@ static u32 vmx_secondary_exec_control(struct vcpu_vmx *vmx)
>  	if (!enable_pml || !atomic_read(&vcpu->kvm->nr_memslots_dirty_logging))
>  		exec_control &= ~SECONDARY_EXEC_ENABLE_PML;
>
> -	if (cpu_has_vmx_xsaves()) {
> -		/* Exposing XSAVES only when XSAVE is exposed */
> -		bool xsaves_enabled =
> -			boot_cpu_has(X86_FEATURE_XSAVE) &&
> -			guest_cpuid_has(vcpu, X86_FEATURE_XSAVE) &&
> -			guest_cpuid_has(vcpu, X86_FEATURE_XSAVES);
> -
> -		vcpu->arch.xsaves_enabled = xsaves_enabled;
> -
> +	if (cpu_has_vmx_xsaves())
>  		vmx_adjust_secondary_exec_control(vmx, &exec_control,
>  						  SECONDARY_EXEC_XSAVES,
> -						  xsaves_enabled, false);
> -	}
> +						  vcpu->arch.xsaves_enabled, false);

xsaves_enabled is same as before in case of init_vmcs().

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

>
>  	/*
>  	 * RDPID is also gated by ENABLE_RDTSCP, turn on the control if either
> @@ -7749,8 +7740,15 @@ static void vmx_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
>  {
>  	struct vcpu_vmx *vmx = to_vmx(vcpu);
>
> -	/* xsaves_enabled is recomputed in vmx_compute_secondary_exec_control(). */
> -	vcpu->arch.xsaves_enabled = false;
> +	/*
> +	 * XSAVES is effectively enabled if and only if XSAVE is also exposed
> +	 * to the guest.  XSAVES depends on CR4.OSXSAVE, and CR4.OSXSAVE can be
> +	 * set if and only if XSAVE is supported.
> +	 */
> +	vcpu->arch.xsaves_enabled = cpu_has_vmx_xsaves() &&
> +				    boot_cpu_has(X86_FEATURE_XSAVE) &&
> +				    guest_cpuid_has(vcpu, X86_FEATURE_XSAVE) &&
> +				    guest_cpuid_has(vcpu, X86_FEATURE_XSAVES);
>
>  	vmx_setup_uret_msrs(vmx);
>
> --
> 2.41.0.694.ge786442a9b-goog
>