Re: [PATCH v4 10/47] x86/tsc: Consolidate forcing of X86_FEATURE_TSC_KNOWN_FREQ for PV code

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Providing the `tsc_early_khz` command-line parameter on CoCo guests (TDX or SEV-SNP) completely bypasses critical guest-specific TSC initialization, leading to uninitialized LAPIC timer periods and missing `X86_FEATURE_TSC_RELIABLE` flags.
--

commit 5593e79bcd8390a1e744d159a1d9710aff43225a
Author: Sean Christopherson <seanjc@google.com>

x86/tsc: Consolidate forcing of X86_FEATURE_TSC_KNOWN_FREQ for PV code

This patch consolidates the setting of X86_FEATURE_TSC_KNOWN_FREQ for
paravirt code. It sets the flag if the TSC frequency is known, but
avoids setting it if the known frequency was provided by the user via
the tsc_early_khz command line parameter.

> diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> index 3625966124423..8cef918486db7 100644
> --- a/arch/x86/kernel/tsc.c
> +++ b/arch/x86/kernel/tsc.c
> @@ -1569,6 +1569,15 @@ void __init tsc_early_init(void)
>  	if (!known_tsc_khz && x86_init.hyper.get_tsc_khz)
>  		known_tsc_khz = x86_init.hyper.get_tsc_khz();
>  
> +	/*
> +	 * Mark the TSC frequency as known if it was obtained from a hypervisor
> +	 * or trusted firmware.  Don't mark the frequency as known if the user
> +	 * specified the frequency, as the user-provided frequency is intended
> +	 * as a "starting point", not a known, guaranteed frequency.
> +	 */
> +	if (known_tsc_khz && !tsc_early_khz)
> +		setup_force_cpu_cap(X86_FEATURE_TSC_KNOWN_FREQ);
> +

[Severity: High]
This isn't a bug introduced by this patch, but looking at tsc_early_init(),
if a user specifies tsc_early_khz on the command line, it completely bypasses
critical guest-specific TSC initialization for CoCo guests:

arch/x86/kernel/tsc.c:tsc_early_init() {
    ...
    if (tsc_early_khz)
        known_tsc_khz = tsc_early_khz;
    else if (cc_platform_has(CC_ATTR_GUEST_SNP_SECURE_TSC))
        known_tsc_khz = snp_secure_tsc_init();
    else if (boot_cpu_has(X86_FEATURE_TDX_GUEST))
        known_tsc_khz = tdx_tsc_init();
    ...
}

Does this bypass cause tdx_tsc_init() and snp_secure_tsc_init() to be skipped
entirely, missing their important side effects?

For example, tdx_tsc_init() computes lapic_timer_period and forces
X86_FEATURE_TSC_RELIABLE:

arch/x86/coco/tdx/tdx.c:tdx_tsc_init() {
    ...
    lapic_timer_period = info.crystal_khz * 1000 / HZ;

    /* TSC is the only reliable clock in TDX guest */
    setup_force_cpu_cap(X86_FEATURE_TSC_RELIABLE);
    ...
}

Could an uninitialized lapic_timer_period cause the local APIC timer to fail
on TDX guests, potentially leading to a boot hang?

Additionally, could the missing X86_FEATURE_TSC_RELIABLE flag cause the
clocksource watchdog to spuriously disqualify the TSC on these guests?

>  	if (!determine_cpu_tsc_frequencies(true, known_cpu_khz, known_tsc_khz))
>  		return;
>  	tsc_enable_sched_clock();

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260529144435.704127-1-seanjc@google.com?part=10