Re: [PATCH 1/2] KVM: SVM: Fix TSC_AUX virtualization setup

[Sean Christopherson <seanjc@google.com>]

On Thu, Sep 14, 2023, Tom Lendacky wrote:
> On 9/14/23 15:28, Sean Christopherson wrote:
> > 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.
> 
> There is a sev_guest() check in svm_vcpu_after_set_cpuid(), so I can move
> that into sev_vcpu_after_set_cpuid() and keep the separate
> sev_es_vcpu_after_set_cpuid().

Works for me.

> And it looks like you would prefer to not have "vcpu" in the function name?
> Might be better search-wise if vcpu remains part of the name?

Oh, that was just a typo/oversight, not intentional.

> > 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.
> 
> Ok, I thought that's how it worked, but wasn't 100% sure. I'll move it out
> of the init_vmcb() path.
> 
> > 
> > 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.
> 
> Do you want that as a separate patch with the first patch purely addressing
> the current issue? Or combine them?

Hmm, now that you mention it, probably a seperate patch on top.