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

[Sean Christopherson <seanjc@google.com>]

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.

Even better would be for GCE to just enumerate support for TSC deadline already,
because KVM already does the right thing to convert guest TSC frequency to host
TSC frequency.  KVM TDX would still need to add full support for CPUID 0x15, but
at least any problems with using one-shot mode will unlikely to impact real guests.