Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3;
Date:   Fri, 13 Oct 2023 15:43:53 -0700
In-Reply-To: <20231006011255.4163884-1-vannapurve@google.com>
Mime-Version: 1.0
References: <20231006011255.4163884-1-vannapurve@google.com>
Message-ID: <ZSnIKQ4bUavAtBz6@google.com>
Subject: Re: [PATCH] x86/tdx: Override the tsc calibration for TDX VMs
From:   Sean Christopherson <seanjc@google.com>
To:     Vishal Annapurve <vannapurve@google.com>
Cc:     "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Jun Nakajima <jun.nakajima@intel.com>,
        Isaku Yamahata <isaku.yamahata@intel.com>,
        Erdem Aktas <erdemaktas@google.com>,
        Sagi Shahar <sagis@google.com>,
        Nikolay Borisov <nik.borisov@suse.com>,
        "Jason A. Donenfeld" <Jason@zx2c4.com>,
        Kuppuswamy Sathyanarayanan 
        <sathyanarayanan.kuppuswamy@linux.intel.com>,
        "H. Peter Anvin" <hpa@zytor.com>, x86@kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

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.
>=20
> 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=E2=80=99s bus clock or cor=
e crystal
  clock frequency (when TSC/core crystal clock ratio is enumerated in CPUID
  leaf 0x15) divided by the value specified in the divide configuration reg=
ister.

TDX hardcodes the core crystal frequency to 25Mhz, whereas KVM hardcodes th=
e 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 use=
rspace
that it's safe to expose CPUID 0x15 to the guest.  Because TDX makes exposi=
ng
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 m=
akes
the guest's APIC timer actually run at 25Mhz given whatever the host APIC b=
us
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 di=
scovers
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 tha=
t, 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 =3D crystal_khz * 1000 / HZ;
+       if (!force_lapic_timer_calibration)
+               lapic_timer_period =3D crystal_khz * 1000 / HZ;
 #endif
=20
        return crystal_khz * ebx_numerator / eax_denominator;

But I would be very leery of forcing calibration, as effectively calibratin=
g to
the *host* core crystal frequency will cause the guest APIC timer to be wro=
ng if
the VM is migrated to a host with a different core crystal frequency.  Rely=
ing
on CPUID 0x15, if it's available, avoids that problem because it puts the o=
nus on
the hypervisor to account for the new host's frequency when emulating the A=
PIC
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 alr=
eady,
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.