Re: [PATCH v5 08/51] x86/sev: Shove SNP's secure/trusted TSC frequency directly into "calibration"

["Nikunj A. Dadhania" <nikunj@amd.com>]

On 7/2/2026 1:01 AM, Sean Christopherson wrote:
> As a first step towards dropping .calibrate_{cpu,tsc}() and explicitly
> defining precedence/priority for "calibration" routines, pass the secure
> TSC frequency obtained from SNP firmware directly to
> determine_cpu_tsc_frequencies() instead of overriding the .calibrate_tsc()
> hook.
> 
> Unlike the native calibration routines, all of the paravirtual overrides,
> including SNP and TDX, are constant in the sense that the frequency
> provided by the hypervisor or trusted firmware is fixed, known, and always
> available during early boot.  More importantly, for CoCo (SNP and TDX) VMs,
> it's imperative that the kernel uses the frequency provided by the trusted
> firmware, not by the untrusted hypervisor.  Enforcing the priority between
> sources by carefully ordering seemingly unrelated init calls, so that the
> trusted override "wins", is brittle and all but impossible to follow.
> 
> Explicitly ignore tsc_early_khz if the exact TSC frequency was obtained
> from trusted firmware, as per commit bd35c77e32e4 ("x86/tsc: Add
> tsc_early_khz command line parameter"), the goal of the param is to play
> nice with setups that provide partial frequency information in CPUID, i.e.
> is NOT intended to be a hard override.  Neither SNP's secure TSC nor TDX
> was supported when commit bd35c77e32e4 landed back in 2020, i.e. lack of
> consideration for the interaction was purely due to oversight when SNP and
> TDX support came along.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  .../admin-guide/kernel-parameters.txt         |  4 +++
>  arch/x86/coco/sev/core.c                      | 14 +++--------
>  arch/x86/include/asm/sev.h                    |  4 +--
>  arch/x86/kernel/tsc.c                         | 25 ++++++++++++++-----
>  4 files changed, 29 insertions(+), 18 deletions(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index b5493a7f8f22..181149f633c3 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -7946,6 +7946,10 @@ Kernel parameters
>  			with CPUID.16h support and partial CPUID.15h support.
>  			Format: <unsigned int>
>  
> +			Note, tsc_early_khz is ignored if the TSC frequency is
> +			provided by trusted firmware when running as an SNP
> +			guest.
> +
>  	tsx=		[X86] Control Transactional Synchronization
>  			Extensions (TSX) feature in Intel processors that
>  			support TSX control.
> diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
> index 403dcea86452..bc5ae9ef74da 100644
> --- a/arch/x86/coco/sev/core.c
> +++ b/arch/x86/coco/sev/core.c
> @@ -99,7 +99,6 @@ static const char * const sev_status_feat_names[] = {
>   */
>  static u64 snp_tsc_scale __ro_after_init;
>  static u64 snp_tsc_offset __ro_after_init;
> -static unsigned long snp_tsc_freq_khz __ro_after_init;
>  
>  DEFINE_PER_CPU(struct sev_es_runtime_data*, runtime_data);
>  DEFINE_PER_CPU(struct sev_es_save_area *, sev_vmsa);
> @@ -2014,15 +2013,10 @@ void __init snp_secure_tsc_prepare(void)
>  	pr_debug("SecureTSC enabled");
>  }
>  
> -static unsigned long securetsc_get_tsc_khz(void)
> -{
> -	return snp_tsc_freq_khz;
> -}
> -
> -void __init snp_secure_tsc_init(void)
> +unsigned int __init snp_secure_tsc_init(void)
>  {
> +	unsigned long snp_tsc_freq_khz, tsc_freq_mhz;
>  	struct snp_secrets_page *secrets;
> -	unsigned long tsc_freq_mhz;
>  	void *mem;
>  
>  	mem = early_memremap_encrypted(sev_secrets_pa, PAGE_SIZE);
> @@ -2043,7 +2037,7 @@ void __init snp_secure_tsc_init(void)
>  
>  	snp_tsc_freq_khz = SNP_SCALE_TSC_FREQ(tsc_freq_mhz * 1000, secrets->tsc_factor);
>  
> -	x86_platform.calibrate_tsc = securetsc_get_tsc_khz;
> -
>  	early_memunmap(mem, PAGE_SIZE);
> +
> +	return snp_tsc_freq_khz;
>  }
> diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
> index 594cfa19cbd4..05ebf0b73ef4 100644
> --- a/arch/x86/include/asm/sev.h
> +++ b/arch/x86/include/asm/sev.h
> @@ -530,7 +530,7 @@ int snp_send_guest_request(struct snp_msg_desc *mdesc, struct snp_guest_req *req
>  int snp_svsm_vtpm_send_command(u8 *buffer);
>  
>  void __init snp_secure_tsc_prepare(void);
> -void __init snp_secure_tsc_init(void);
> +unsigned int snp_secure_tsc_init(void);

It seems __init got dropped here accidentally?

Apart from this:

Reviewed-by: Nikunj A Dadhania <nikunj@amd.com>
Tested-by: Nikunj A Dadhania <nikunj@amd.com>

>  enum es_result savic_register_gpa(u64 gpa);
>  enum es_result savic_unregister_gpa(u64 *gpa);
>  u64 savic_ghcb_msr_read(u32 reg);
> @@ -637,7 +637,7 @@ static inline int snp_send_guest_request(struct snp_msg_desc *mdesc,
>  					 struct snp_guest_req *req) { return -ENODEV; }
>  static inline int snp_svsm_vtpm_send_command(u8 *buffer) { return -ENODEV; }
>  static inline void __init snp_secure_tsc_prepare(void) { }
> -static inline void __init snp_secure_tsc_init(void) { }
> +static inline unsigned int __init snp_secure_tsc_init(void) { return 0; }
>  static inline void sev_evict_cache(void *va, int npages) {}
>  static inline enum es_result savic_register_gpa(u64 gpa) { return ES_UNSUPPORTED; }
>  static inline enum es_result savic_unregister_gpa(u64 *gpa) { return ES_UNSUPPORTED; }
> diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> index 8f1604ffe986..f049c126e47c 100644
> --- a/arch/x86/kernel/tsc.c
> +++ b/arch/x86/kernel/tsc.c
> @@ -1440,15 +1440,16 @@ static int __init init_tsc_clocksource(void)
>   */
>  device_initcall(init_tsc_clocksource);
>  
> -static bool __init determine_cpu_tsc_frequencies(bool early)
> +static bool __init determine_cpu_tsc_frequencies(bool early,
> +						 unsigned int known_tsc_khz)
>  {
>  	/* Make sure that cpu and tsc are not already calibrated */
>  	WARN_ON(cpu_khz || tsc_khz);
>  
>  	if (early) {
>  		cpu_khz = x86_platform.calibrate_cpu();
> -		if (tsc_early_khz)
> -			tsc_khz = tsc_early_khz;
> +		if (known_tsc_khz)
> +			tsc_khz = known_tsc_khz;
>  		else
>  			tsc_khz = x86_platform.calibrate_tsc();
>  	} else {
> @@ -1503,6 +1504,8 @@ static void __init tsc_enable_sched_clock(void)
>  
>  void __init tsc_early_init(void)
>  {
> +	unsigned int known_tsc_khz = 0;
> +
>  	if (!boot_cpu_has(X86_FEATURE_TSC))
>  		return;
>  	/* Don't change UV TSC multi-chassis synchronization */
> @@ -1510,9 +1513,19 @@ void __init tsc_early_init(void)
>  		return;
>  
>  	if (cc_platform_has(CC_ATTR_GUEST_SNP_SECURE_TSC))
> -		snp_secure_tsc_init();
> +		known_tsc_khz = snp_secure_tsc_init();
>  
> -	if (!determine_cpu_tsc_frequencies(true))
> +	/*
> +	 * Ignore the user-provided TSC frequency if the exact frequency was
> +	 * obtained from trusted firmware, as the user-provided frequency is
> +	 * intended as a "starting point", not a known, guaranteed frequency.
> +	 */
> +	if (!known_tsc_khz)
> +		known_tsc_khz = tsc_early_khz;
> +	else if (tsc_early_khz)
> +		pr_err("Ignoring 'tsc_early_khz' in favor of trusted firmware.\n");
> +
> +	if (!determine_cpu_tsc_frequencies(true, known_tsc_khz))
>  		return;
>  	tsc_enable_sched_clock();
>  }
> @@ -1533,7 +1546,7 @@ void __init tsc_init(void)
>  
>  	if (!tsc_khz) {
>  		/* We failed to determine frequencies earlier, try again */
> -		if (!determine_cpu_tsc_frequencies(false)) {
> +		if (!determine_cpu_tsc_frequencies(false, 0)) {
>  			mark_tsc_unstable("could not calculate TSC khz");
>  			setup_clear_cpu_cap(X86_FEATURE_TSC_DEADLINE_TIMER);
>  			return;