linux-kernel - Re: [PATCH v2] x86/sev: Use TSC_FACTOR for Secure TSC frequency calculation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aF0ESlmxi1uOHkrc@gmail.com>
Date: Thu, 26 Jun 2025 10:26:50 +0200
From: Ingo Molnar <mingo@...nel.org>
To: Nikunj A Dadhania <nikunj@....com>
Cc: linux-kernel@...r.kernel.org, bp@...en8.de, x86@...nel.org,
	tglx@...utronix.de, mingo@...hat.com, dave.hansen@...ux.intel.com,
	thomas.lendacky@....com, aik@....com, dionnaglaze@...gle.com,
	stable@...r.kernel.org
Subject: Re: [PATCH v2] x86/sev: Use TSC_FACTOR for Secure TSC frequency
 calculation


* Nikunj A Dadhania <nikunj@....com> wrote:

> When using Secure TSC, the GUEST_TSC_FREQ MSR reports a frequency based on
> the nominal P0 frequency, which deviates slightly (typically ~0.2%) from
> the actual mean TSC frequency due to clocking parameters. Over extended VM
> uptime, this discrepancy accumulates, causing clock skew between the
> hypervisor and SEV-SNP VM, leading to early timer interrupts as perceived
> by the guest.
> 
> The guest kernel relies on the reported nominal frequency for TSC-based
> timekeeping, while the actual frequency set during SNP_LAUNCH_START may
> differ. This mismatch results in inaccurate time calculations, causing the
> guest to perceive hrtimers as firing earlier than expected.
> 
> Utilize the TSC_FACTOR from the SEV firmware's secrets page (see "Secrets
> Page Format" in the SNP Firmware ABI Specification) to calculate the mean
> TSC frequency, ensuring accurate timekeeping and mitigating clock skew in
> SEV-SNP VMs.
> 
> Use early_ioremap_encrypted() to map the secrets page as
> ioremap_encrypted() uses kmalloc() which is not available during early TSC
> initialization and causes a panic.
> 
> Fixes: 73bbf3b0fbba ("x86/tsc: Init the TSC for Secure TSC guests")
> Cc: stable@...r.kernel.org
> Signed-off-by: Nikunj A Dadhania <nikunj@....com>
> 
> ---
> v2:
> * Move the SNP TSC scaling constant to the header (Dionna)
> * Drop the unsigned long cast and add in securetsc_get_tsc_khz (Tom)
> * Drop the RB from Tom as the code has changed
> ---
>  arch/x86/include/asm/sev.h | 18 +++++++++++++++++-
>  arch/x86/coco/sev/core.c   | 16 ++++++++++++++--
>  2 files changed, 31 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
> index fbb616fcbfb8..869355367210 100644
> --- a/arch/x86/include/asm/sev.h
> +++ b/arch/x86/include/asm/sev.h
> @@ -223,6 +223,19 @@ struct snp_tsc_info_resp {
>  	u8 rsvd2[100];
>  } __packed;
>  
> +
> +/*
> + * Obtain the mean TSC frequency by decreasing the nominal TSC frequency with
> + * TSC_FACTOR as documented in the SNP Firmware ABI specification:
> + *
> + * GUEST_TSC_FREQ * (1 - (TSC_FACTOR * 0.00001))
> + *
> + * which is equivalent to:
> + *
> + * GUEST_TSC_FREQ -= (GUEST_TSC_FREQ * TSC_FACTOR) / 100000;
> + */
> +#define SNP_SCALE_TSC_FREQ(freq, factor) ((freq) - ((freq) * (factor)) / 100000)

Nit: there's really no need to use parentheses in this expression,
'x * y / z' is equivalent and fine.

> +
>  struct snp_guest_req {
>  	void *req_buf;
>  	size_t req_sz;
> @@ -283,8 +296,11 @@ struct snp_secrets_page {
>  	u8 svsm_guest_vmpl;
>  	u8 rsvd3[3];
>  
> +	/* The percentage decrease from nominal to mean TSC frequency. */
> +	u32 tsc_factor;
> +
>  	/* Remainder of page */
> -	u8 rsvd4[3744];
> +	u8 rsvd4[3740];
>  } __packed;
>  
>  struct snp_msg_desc {
> diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
> index 8375ca7fbd8a..36f419ff25d4 100644
> --- a/arch/x86/coco/sev/core.c
> +++ b/arch/x86/coco/sev/core.c
> @@ -2156,20 +2156,32 @@ void __init snp_secure_tsc_prepare(void)
>  
>  static unsigned long securetsc_get_tsc_khz(void)
>  {
> -	return snp_tsc_freq_khz;
> +	return (unsigned long)snp_tsc_freq_khz;

This forced type cast is a signature of poor type choices. Please 
harmonize the types of snp_tsc_freq_khz and securetsc_get_tsc_khz() to 
avoid the type cast altogether. Does this code even get built and run 
on 32-bit kernels?

Thanks,

	Ingo