Re: [PATCH] x86/tdx: Override the tsc calibration for TDX VMs

[Sean Christopherson]

On Fri, Oct 13, 2023, Sean Christopherson wrote:
> On Fri, Oct 06, 2023, Vishal Annapurve wrote:
> > TSC calibration for native execution gets the TSC frequency from CPUID,
> > but also ends up setting lapic_timer_period. When using oneshot mode
> > with lapic timer, predefined value of lapic_timer_period causes lapic
> > timer calibration to be skipped with wrong multipliers set for lapic
> > timer.
> > 
> > To avoid this issue, override the TSC calibration step for TDX VMs to
> > just calculate the TSC frequency using cpuid values.
> 
> This is a hack to workaround a KVM TDX bug.  Per Intel's SDM:
> 
>   The APIC timer frequency will be the processor’s bus clock or core crystal
>   clock frequency (when TSC/core crystal clock ratio is enumerated in CPUID
>   leaf 0x15) divided by the value specified in the divide configuration register.
> 
> TDX hardcodes the core crystal frequency to 25Mhz, whereas KVM hardcodes the APIC
> bus frequency to 1Ghz.  Upstream KVM's *current* behavior is fine, because KVM
> doesn't advertise support for CPUID 0x15, i.e. doesn't announce to host userspace
> that it's safe to expose CPUID 0x15 to the guest.  Because TDX makes exposing
> CPUID 0x15 mandatory, KVM needs to be taught to correctly emulate the guest's
> APIC bus frequency, a.k.a. the TDX guest core crystal frequency of 25Mhz.
> 
> I.e. tmict_to_ns() needs to replace APIC_BUS_CYCLE_NS with some math that makes
> the guest's APIC timer actually run at 25Mhz given whatever the host APIC bus
> runs at.
> 
>   static inline u64 tmict_to_ns(struct kvm_lapic *apic, u32 tmict)
>   {
> 	return (u64)tmict * APIC_BUS_CYCLE_NS * (u64)apic->divide_count;
>   }
> 
> The existing guest code "works" because the calibration code effectively discovers
> the host APIC bus frequency.  If we really want to force calibration, then the
> best way to do that would be to add a command line option to do exactly that, not
> hack around a KVM TDX bug.
> 
> diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> index 15f97c0abc9d..ce1cec6b3c18 100644
> --- a/arch/x86/kernel/tsc.c
> +++ b/arch/x86/kernel/tsc.c
> @@ -723,7 +723,8 @@ unsigned long native_calibrate_tsc(void)
>          * lapic_timer_period here to avoid having to calibrate the APIC
>          * timer later.
>          */
> -       lapic_timer_period = crystal_khz * 1000 / HZ;
> +       if (!force_lapic_timer_calibration)
> +               lapic_timer_period = crystal_khz * 1000 / HZ;
>  #endif
>  
>         return crystal_khz * ebx_numerator / eax_denominator;
> 
> But I would be very leery of forcing calibration, as effectively calibrating to
> the *host* core crystal frequency will cause the guest APIC timer to be wrong if
> the VM is migrated to a host with a different core crystal frequency.  Relying
> on CPUID 0x15, if it's available, avoids that problem because it puts the onus on
> the hypervisor to account for the new host's frequency when emulating the APIC
> timer.  That mess exists today, but deliberate ignoring the mechanism that allows
> the host to fix the mess would be asinine IMO.

Gah, I had a brainfart.  tmict_to_ns() obviously converts TMICT to nanoseconds,
and then converts nanoseconds into whatever frequency the underlying host timer
runs at.

So it's really only TDX that is problematic, as TDX demands the APIC bus be
emulated at a frequency other than 1Ghz.