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 <[email protected]>
> > > ---
> > > 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.
On 9/14/23 16:13, Sean Christopherson wrote:
> 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 <[email protected]>
>>>> ---
>>>> 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.
This toggling possibility raises a question related to the second patch in
this series that eliminates the use of the user return MSR for TSC_AUX.
Depending on when the interfaces are called (set CPUID, host-initiated
WRMSR of TSC_AUX, set CPUID again), I think we could end up in a state
where the host TSC_AUX may not get restored properly, not 100% sure at the
moment, though.
Let me drop that patch from the series for now and just send the fix(es).
I'll work through the other scenarios and code paths and send the user
return MSR optimization as a separate series later.
Thanks,
Tom