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 <[email protected]> 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/[email protected]?part=10
