[PATCH v2] x86/sev: Use TSC_FACTOR for Secure TSC frequency calculation

[PATCH v2] x86/sev: Use TSC_FACTOR for Secure TSC frequency calculation

[Nikunj A Dadhania]

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@vger.kernel.org
Signed-off-by: Nikunj A Dadhania <nikunj@amd.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)
+
 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;
 }
 
 void __init snp_secure_tsc_init(void)
 {
+	struct snp_secrets_page *secrets;
 	unsigned long long tsc_freq_mhz;
+	void *mem;
 
 	if (!cc_platform_has(CC_ATTR_GUEST_SNP_SECURE_TSC))
 		return;
 
+	mem = early_memremap_encrypted(sev_secrets_pa, PAGE_SIZE);
+	if (!mem) {
+		pr_err("Unable to get TSC_FACTOR: failed to map the SNP secrets page.\n");
+		sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_SECURE_TSC);
+	}
+
+	secrets = (__force struct snp_secrets_page *)mem;
+
 	setup_force_cpu_cap(X86_FEATURE_TSC_KNOWN_FREQ);
 	rdmsrq(MSR_AMD64_GUEST_TSC_FREQ, tsc_freq_mhz);
-	snp_tsc_freq_khz = (unsigned long)(tsc_freq_mhz * 1000);
+	snp_tsc_freq_khz = SNP_SCALE_TSC_FREQ(tsc_freq_mhz * 1000, secrets->tsc_factor);
 
 	x86_platform.calibrate_cpu = securetsc_get_tsc_khz;
 	x86_platform.calibrate_tsc = securetsc_get_tsc_khz;
+
+	early_memunmap(mem, PAGE_SIZE);
 }

base-commit: 49151ac6671fe0372261054daf5e4da3567b8271
-- 
2.43.0

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

[Ingo Molnar]

* Nikunj A Dadhania <nikunj@amd.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@vger.kernel.org
> Signed-off-by: Nikunj A Dadhania <nikunj@amd.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

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

[Nikunj A. Dadhania]

On 6/26/2025 1:56 PM, Ingo Molnar wrote:
> 
> * Nikunj A Dadhania <nikunj@amd.com> wrote:
> 
>> 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.

It will give wrong scale if I call with freq as "tsc + 1000000" 
without the parentheses?

SNP_SCALE_TSC_FREQ(tsc + 1000000, factor)

>> 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. 

Sure, I can attempt that and send an updated patch.

> Does this code even get built and run on 32-bit kernels?

This code should not build for 32-bit kernels.

Thanks
Nikunj

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

[Tom Lendacky]

On 6/26/25 05:01, Nikunj A. Dadhania wrote:
> 
> 
> On 6/26/2025 1:56 PM, Ingo Molnar wrote:
>>
>> * Nikunj A Dadhania <nikunj@amd.com> wrote:
>>
>>> 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.
> 
> It will give wrong scale if I call with freq as "tsc + 1000000" 
> without the parentheses?

I think Ingo is saying this can be ((freq) - (freq) * (factor) / 100000)

in other words, getting rid of the parentheses around the multiplication.

Thanks,
Tom

> 
> SNP_SCALE_TSC_FREQ(tsc + 1000000, factor)
> 
>>> 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. 
> 
> Sure, I can attempt that and send an updated patch.
> 
>> Does this code even get built and run on 32-bit kernels?
> 
> This code should not build for 32-bit kernels.
> 
> Thanks
> Nikunj

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

[Nikunj A. Dadhania]

On 6/26/2025 7:11 PM, Tom Lendacky wrote:
> On 6/26/25 05:01, Nikunj A. Dadhania wrote:
>> On 6/26/2025 1:56 PM, Ingo Molnar wrote:
>>> * Nikunj A Dadhania <nikunj@amd.com> wrote:

>>>> +#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.
>>
>> It will give wrong scale if I call with freq as "tsc + 1000000" 
>> without the parentheses?
> 
> I think Ingo is saying this can be ((freq) - (freq) * (factor) / 100000)
> 
> in other words, getting rid of the parentheses around the multiplication.

Ak ok, that should be fine.

Regards
Nikunj