Linux-HyperV List
 help / color / mirror / Atom feed
From: Sean Christopherson <seanjc@google.com>
To: sashiko-reviews@lists.linux.dev
Cc: linux-hyperv@vger.kernel.org
Subject: Re: [PATCH v3 32/41] x86/tsc: Rejects attempts to override TSC calibration with lesser routine
Date: Mon, 18 May 2026 12:17:44 -0700	[thread overview]
Message-ID: <agtl2L4Y_jE1MAai@google.com> (raw)
In-Reply-To: <20260515201628.9E3C5C2BCB3@smtp.kernel.org>

On Fri, May 15, 2026, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] Spurious WARN_ON stack trace on every boot for CoCo (SEV-SNP/TDX) guests under KVM.
> --
> 
> commit e57255e4d076143a67d0e6e44f914aec31aed8c7
> Author: Sean Christopherson <seanjc@google.com>
> 
> x86/tsc: Rejects attempts to override TSC calibration with lesser routine
> 
> This commit adds a sanity check when registering a TSC frequency calibration
> routine to ensure the incoming routine is as robust as the outgoing one,
> rejecting it with a warning if it is not. This prevents replacing a trusted
> calibration source with a less trusted one.
> 
> > diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> > index 98bef1d06fa9d..7a261214fa3ef 100644
> > --- a/arch/x86/kernel/tsc.c
> > +++ b/arch/x86/kernel/tsc.c
> > @@ -1319,8 +1319,13 @@ void tsc_register_calibration_routines(unsigned long (*calibrate_tsc)(void),
> >  
> >  	if (properties & TSC_FREQUENCY_KNOWN)
> >  		setup_force_cpu_cap(X86_FEATURE_TSC_KNOWN_FREQ);
> > +	else if (WARN_ON(boot_cpu_has(X86_FEATURE_TSC_KNOWN_FREQ)))
> > +		return;
> > +
> >  	if (properties & TSC_RELIABLE)
> >  		setup_force_cpu_cap(X86_FEATURE_TSC_RELIABLE);
> > +	else if (WARN_ON(boot_cpu_has(X86_FEATURE_TSC_RELIABLE)))
> > +		return;
> 
> Will this cause a spurious warning stack trace on every boot for CoCo
> (SEV-SNP/TDX) guests under KVM?
> 
> For KVM CoCo guests, CoCo registers first via tsc_early_init(), setting both
> TSC_FREQUENCY_KNOWN and X86_FEATURE_TSC_RELIABLE.
> 
> KVM then registers second via kvmclock_init(). At this point in the patch
> series, KVM only passes TSC_FREQUENCY_KNOWN, so the properties & TSC_RELIABLE
> check is false, and it falls through to this WARN_ON. Since CoCo already set
> the capability, the condition evaluates to true.
> 
> I see this is fixed later in the series by the commit "x86/kvmclock: Mark TSC
> as reliable when it's constant and nonstop", which modifies KVM to pass
> TSC_FREQ_KNOWN_AND_RELIABLE on modern hardware, preventing this warning.
> 
> Could the patch ordering be adjusted to prevent this intermediate regression?

Ah shoot, no.  Well, not entirely.  Because when running as a CoCo guest with a
trusted TSC, the kernel needs to prevent overwriting the TSC calibration, *period*.
I.e. changing the ordering will eliminate the unwanted WARN, but it won't fix the
underlying goof that the trusted calibration routines are still being overwritten
with untrusted routines.

Hrm, the SNP secure TSC code complicates things, but I suspect it's broken.  If
it's indeed broken, then I think we can add a TSC_TRUSTED flag and then do:

	if (WARN_ON_ONCE(!calibrate_tsc))
		return;

	if (properties & TSC_FREQUENCY_KNOWN)
		setup_force_cpu_cap(X86_FEATURE_TSC_KNOWN_FREQ);
	else if (WARN_ON(boot_cpu_has(X86_FEATURE_TSC_KNOWN_FREQ)))
		return;

	if (properties & TSC_RELIABLE)
		setup_force_cpu_cap(X86_FEATURE_TSC_RELIABLE);
	else if (WARN_ON(boot_cpu_has(X86_FEATURE_TSC_RELIABLE)))
		return;

	if (!cpu_has_trusted_tsc() || (properties & TSC_TRUSTED))
		x86_platform.calibrate_tsc = calibrate_tsc;

	if (calibrate_cpu)
		x86_platform.calibrate_cpu = calibrate_cpu;


Tom / Nikunj,

Isn't it completely wrong to assume the CPU frequency is the same as the TSC
frequency?  The changelog says the difference "does not apply", but that makes
no sense.

    Use the GUEST_TSC_FREQ MSR to discover the TSC frequency instead of
    relying on kvm-clock based frequency calibration.  Override both CPU and
    TSC frequency calibration callbacks with securetsc_get_tsc_khz(). Since
    the difference between CPU base and TSC frequency does not apply in this
    case, the same callback is being used.

E.g. if the host passed through APERF/MPERF, then the difference most definitely
applies.  If TSC != CPU frequency, then knowingly using bad data is even worse
(far, far worse) than hoping the untrusted host is playing nice.

If the TSC and CPU frequencies are somehow guaranteed to be the same (which I
can't possibly imagine is the case), then the above won't work because we also
want to prevent overriding calibrate_cpu().

  reply	other threads:[~2026-05-18 19:17 UTC|newest]

Thread overview: 70+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-05-15 19:19 [PATCH v3 00/41] x86: Try to wrangle PV clocks vs. TSC Sean Christopherson
2026-05-15 19:19 ` [PATCH v3 01/41] x86/tsc: Add a standalone helpers for getting TSC info from CPUID.0x15 Sean Christopherson
2026-05-15 19:19 ` [PATCH v3 02/41] x86/tsc: Add helper to register CPU and TSC freq calibration routines Sean Christopherson
2026-05-15 20:06   ` sashiko-bot
2026-05-18 21:59   ` Woodhouse, David
2026-05-15 19:19 ` [PATCH v3 03/41] x86/sev: Mark TSC as reliable when configuring Secure TSC Sean Christopherson
2026-05-15 19:19 ` [PATCH v3 04/41] x86/sev: Move check for SNP Secure TSC support to tsc_early_init() Sean Christopherson
2026-05-15 19:19 ` [PATCH v3 05/41] x86/tdx: Override PV calibration routines with CPUID-based calibration Sean Christopherson
2026-05-15 19:19 ` [PATCH v3 06/41] x86/acrn: Mark TSC frequency as known when using ACRN for calibration Sean Christopherson
2026-05-15 19:19 ` [PATCH v3 07/41] clocksource: hyper-v: Register sched_clock save/restore iff it's necessary Sean Christopherson
2026-05-15 19:19 ` [PATCH v3 08/41] clocksource: hyper-v: Drop wrappers to sched_clock save/restore helpers Sean Christopherson
2026-05-15 19:19 ` [PATCH v3 09/41] clocksource: hyper-v: Don't save/restore TSC offset when using HV sched_clock Sean Christopherson
2026-05-15 19:19 ` [PATCH v3 10/41] x86/kvmclock: Setup kvmclock for secondary CPUs iff CONFIG_SMP=y Sean Christopherson
2026-05-15 19:19 ` [PATCH v3 11/41] x86/kvm: Don't disable kvmclock on BSP in syscore_suspend() Sean Christopherson
2026-05-15 20:34   ` sashiko-bot
2026-05-15 22:29     ` Sean Christopherson
2026-05-15 19:19 ` [PATCH v3 12/41] x86/paravirt: Remove unnecessary PARAVIRT=n stub for paravirt_set_sched_clock() Sean Christopherson
2026-05-15 19:19 ` [PATCH v3 13/41] x86/paravirt: Move handling of unstable PV clocks into paravirt_set_sched_clock() Sean Christopherson
2026-05-15 19:19 ` [PATCH v3 14/41] x86/kvmclock: Move sched_clock save/restore helpers up in kvmclock.c Sean Christopherson
2026-05-15 19:19 ` [PATCH v3 15/41] x86/xen/time: Nullify x86_platform's sched_clock save/restore hooks Sean Christopherson
2026-05-15 19:48   ` sashiko-bot
2026-05-15 22:43     ` Sean Christopherson
2026-05-15 19:19 ` [PATCH v3 16/41] x86/vmware: Nullify save/restore hooks when using VMware's sched_clock Sean Christopherson
2026-05-15 19:42   ` sashiko-bot
2026-05-15 19:19 ` [PATCH v3 17/41] x86/tsc: WARN if TSC sched_clock save/restore used with PV sched_clock Sean Christopherson
2026-05-15 19:55   ` sashiko-bot
2026-05-15 19:19 ` [PATCH v3 18/41] x86/paravirt: Pass sched_clock save/restore helpers during registration Sean Christopherson
2026-05-15 19:56   ` sashiko-bot
2026-05-15 19:19 ` [PATCH v3 19/41] x86/kvmclock: Move kvm_sched_clock_init() down in kvmclock.c Sean Christopherson
2026-05-15 19:19 ` [PATCH v3 20/41] x86/xen/time: Mark xen_setup_vsyscall_time_info() as __init Sean Christopherson
2026-05-15 19:19 ` [PATCH v3 21/41] x86/pvclock: Mark setup helpers and related various as __init/__ro_after_init Sean Christopherson
2026-05-15 19:19 ` [PATCH v3 22/41] x86/pvclock: WARN if pvclock's valid_flags are overwritten Sean Christopherson
2026-05-15 19:19 ` [PATCH v3 23/41] x86/kvmclock: Refactor handling of PVCLOCK_TSC_STABLE_BIT during kvmclock_init() Sean Christopherson
2026-05-15 19:19 ` [PATCH v3 24/41] timekeeping: Resume clocksources before reading persistent clock Sean Christopherson
2026-05-15 19:19 ` [PATCH v3 25/41] x86/kvmclock: Hook clocksource.suspend/resume when kvmclock isn't sched_clock Sean Christopherson
2026-05-15 19:19 ` [PATCH v3 26/41] x86/kvmclock: WARN if wall clock is read while kvmclock is suspended Sean Christopherson
2026-05-15 19:19 ` [PATCH v3 27/41] x86/kvmclock: Enable kvmclock on APs during onlining if kvmclock isn't sched_clock Sean Christopherson
2026-05-15 19:47   ` sashiko-bot
2026-05-18 23:04     ` Sean Christopherson
2026-05-15 19:19 ` [PATCH v3 28/41] x86/paravirt: Mark __paravirt_set_sched_clock() as __init Sean Christopherson
2026-05-15 19:19 ` [PATCH v3 29/41] x86/paravirt: Plumb a return code into __paravirt_set_sched_clock() Sean Christopherson
2026-05-15 19:48   ` sashiko-bot
2026-05-18 21:14     ` Sean Christopherson
2026-05-15 19:19 ` [PATCH v3 30/41] x86/paravirt: Don't use a PV sched_clock in CoCo guests with trusted TSC Sean Christopherson
2026-05-15 19:19 ` [PATCH v3 31/41] x86/tsc: Pass KNOWN_FREQ and RELIABLE as params to registration Sean Christopherson
2026-05-15 19:45   ` sashiko-bot
2026-05-18 22:18     ` Sean Christopherson
2026-05-19  3:12       ` Michael Kelley
2026-05-15 19:19 ` [PATCH v3 32/41] x86/tsc: Rejects attempts to override TSC calibration with lesser routine Sean Christopherson
2026-05-15 20:16   ` sashiko-bot
2026-05-18 19:17     ` Sean Christopherson [this message]
2026-05-15 19:19 ` [PATCH v3 33/41] x86/kvmclock: Mark TSC as reliable when it's constant and nonstop Sean Christopherson
2026-05-15 19:19 ` [PATCH v3 34/41] KVM: x86: Officially define CPUID 0x40000010 as PV Timing Info (TSC and Bus) Sean Christopherson
2026-05-15 19:19 ` [PATCH v3 35/41] x86/kvmclock: Obtain TSC frequency from CPUID if present Sean Christopherson
2026-05-15 19:19 ` [PATCH v3 36/41] x86/kvmclock: Get local APIC bus frequency from PV CPUID Timing Info Sean Christopherson
2026-05-15 19:55   ` sashiko-bot
2026-05-18 20:57     ` Sean Christopherson
2026-05-15 19:19 ` [PATCH v3 37/41] x86/kvmclock: Use TSC for sched_clock if it's constant and non-stop Sean Christopherson
2026-05-15 20:09   ` sashiko-bot
2026-05-18 20:28     ` Sean Christopherson
2026-05-15 19:19 ` [PATCH v3 38/41] x86/paravirt: kvmclock: Setup kvmclock early iff it's sched_clock Sean Christopherson
2026-05-15 19:19 ` [PATCH v3 39/41] x86/paravirt: Move using_native_sched_clock() stub into timer.h Sean Christopherson
2026-05-15 19:19 ` [PATCH v3 40/41] x86/tsc: Add standalone helper for getting CPU frequency from CPUID Sean Christopherson
2026-05-15 19:51   ` sashiko-bot
2026-05-15 23:04     ` Sean Christopherson
2026-05-16  7:42   ` Paolo Bonzini
2026-05-15 19:19 ` [PATCH v3 41/41] x86/kvmclock: Get CPU base frequency from CPUID when it's available Sean Christopherson
2026-05-15 19:59   ` sashiko-bot
2026-05-18 21:11 ` [PATCH v3 00/41] x86: Try to wrangle PV clocks vs. TSC Sean Christopherson
2026-05-18 23:38 ` David Woodhouse

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=agtl2L4Y_jE1MAai@google.com \
    --to=seanjc@google.com \
    --cc=linux-hyperv@vger.kernel.org \
    --cc=sashiko-reviews@lists.linux.dev \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox