Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
Date:   Thu, 14 Sep 2023 13:28:30 -0700
In-Reply-To: <8a5c1d2637475c7fb9657cdd6cb0e86f2bb3bab6.1694721045.git.thomas.lendacky@amd.com>
Mime-Version: 1.0
References: <cover.1694721045.git.thomas.lendacky@amd.com> <8a5c1d2637475c7fb9657cdd6cb0e86f2bb3bab6.1694721045.git.thomas.lendacky@amd.com>
Message-ID: <ZQNs7uo8F62XQawJ@google.com>
Subject: Re: [PATCH 1/2] KVM: SVM: Fix TSC_AUX virtualization setup
From:   Sean Christopherson <seanjc@google.com>
To:     Tom Lendacky <thomas.lendacky@amd.com>
Cc:     kvm@vger.kernel.org, linux-kernel@vger.kernel.org, x86@kernel.org,
        Paolo Bonzini <pbonzini@redhat.com>,
        Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        Ingo Molnar <mingo@redhat.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Babu Moger <babu.moger@amd.com>
Content-Type: text/plain; charset="us-ascii"
Precedence: bulk

On Thu, Sep 14, 2023, Tom Lendacky wrote:
> The checks for virtualizing TSC_AUX occur during the vCPU reset processing
> path. However, at the time of initial vCPU reset processing, when the vCPU
> is first created, not all of the guest CPUID information has been set. In
> this case the RDTSCP and RDPID feature support for the guest is not in
> place and so TSC_AUX virtualization is not established.
> 
> This continues for each vCPU created for the guest. On the first boot of
> an AP, vCPU reset processing is executed as a result of an APIC INIT
> event, this time with all of the guest CPUID information set, resulting
> in TSC_AUX virtualization being enabled, but only for the APs. The BSP
> always sees a TSC_AUX value of 0 which probably went unnoticed because,
> at least for Linux, the BSP TSC_AUX value is 0.
> 
> Move the TSC_AUX virtualization enablement into the vcpu_after_set_cpuid()
> path to allow for proper initialization of the support after the guest
> CPUID information has been set.
> 
> Fixes: 296d5a17e793 ("KVM: SEV-ES: Use V_TSC_AUX if available instead of RDTSC/MSR_TSC_AUX intercepts")
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
>  arch/x86/kvm/svm/sev.c | 27 +++++++++++++++++++--------
>  arch/x86/kvm/svm/svm.c |  3 +++
>  arch/x86/kvm/svm/svm.h |  1 +
>  3 files changed, 23 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index b9a0a939d59f..565c9de87c6d 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -2962,6 +2962,25 @@ int sev_es_string_io(struct vcpu_svm *svm, int size, unsigned int port, int in)
>  				    count, in);
>  }
>  
> +static void sev_es_init_vmcb_after_set_cpuid(struct vcpu_svm *svm)

I would rather name this sev_es_after_set_cpuid() and call it directly from
svm_vcpu_after_set_cpuid().  Or I suppose bounce through sev_after_set_cpuid(),
but that seems gratuitous.

AFAICT, there's no point in calling this from init_vmcb(); guest_cpuid_has() is
guaranteed to be false when called during vCPU creation and so the intercept
behavior will be correct, and even if SEV-ES called init_vmcb() from
shutdown_interception(), which it doesn't, guest_cpuid_has() wouldn't change,
i.e. the intercepts wouldn't need to be changed.

init_vmcb_after_set_cpuid() is a special snowflake because it handles both SVM's
true defaults *and* guest CPUID updates.

> +{
> +	struct kvm_vcpu *vcpu = &svm->vcpu;
> +
> +	if (boot_cpu_has(X86_FEATURE_V_TSC_AUX) &&
> +	    (guest_cpuid_has(vcpu, X86_FEATURE_RDTSCP) ||
> +	     guest_cpuid_has(vcpu, X86_FEATURE_RDPID))) {
> +		set_msr_interception(vcpu, svm->msrpm, MSR_TSC_AUX, 1, 1);

This needs to toggled interception back on if RDTSCP and RDPID are hidden from
the guest.  KVM's wonderful ABI doesn't disallow multiple calls to KVM_SET_CPUID2
before KVM_RUN.

> +		if (guest_cpuid_has(vcpu, X86_FEATURE_RDTSCP))
> +			svm_clr_intercept(svm, INTERCEPT_RDTSCP);

Same thing here.

> +	}
> +}