* [PATCH] x86/sev: Use TSC_FACTOR for Secure TSC frequency calculation
@ 2025-06-09 8:08 Nikunj A Dadhania
2025-06-24 4:13 ` Nikunj A Dadhania
0 siblings, 1 reply; 6+ messages in thread
From: Nikunj A Dadhania @ 2025-06-09 8:08 UTC (permalink / raw)
To: linux-kernel, bp, x86
Cc: tglx, mingo, dave.hansen, thomas.lendacky, aik, Nikunj A Dadhania,
stable
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")
Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
Cc: stable@vger.kernel.org
Signed-off-by: Nikunj A Dadhania <nikunj@amd.com>
---
arch/x86/include/asm/sev.h | 5 ++++-
arch/x86/coco/sev/core.c | 24 ++++++++++++++++++++++++
2 files changed, 28 insertions(+), 1 deletion(-)
diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
index 58e028d42e41..655d7e37bbcc 100644
--- a/arch/x86/include/asm/sev.h
+++ b/arch/x86/include/asm/sev.h
@@ -282,8 +282,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 b6db4e0b936b..ffd44712cec0 100644
--- a/arch/x86/coco/sev/core.c
+++ b/arch/x86/coco/sev/core.c
@@ -2167,15 +2167,39 @@ static unsigned long securetsc_get_tsc_khz(void)
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);
+ /*
+ * 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;
+ */
+ snp_tsc_freq_khz -= (snp_tsc_freq_khz * secrets->tsc_factor) / 100000;
+
x86_platform.calibrate_cpu = securetsc_get_tsc_khz;
x86_platform.calibrate_tsc = securetsc_get_tsc_khz;
+
+ early_memunmap(mem, PAGE_SIZE);
}
base-commit: 337964c8abfbef645cbbe25245e25c11d9d1fc4c
--
2.43.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/sev: Use TSC_FACTOR for Secure TSC frequency calculation
2025-06-09 8:08 [PATCH] x86/sev: Use TSC_FACTOR for Secure TSC frequency calculation Nikunj A Dadhania
@ 2025-06-24 4:13 ` Nikunj A Dadhania
2025-06-24 19:04 ` Dionna Amalie Glaze
0 siblings, 1 reply; 6+ messages in thread
From: Nikunj A Dadhania @ 2025-06-24 4:13 UTC (permalink / raw)
To: linux-kernel, bp, x86
Cc: tglx, mingo, dave.hansen, thomas.lendacky, aik, stable
Nikunj A Dadhania <nikunj@amd.com> writes:
> 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")
> Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Nikunj A Dadhania <nikunj@amd.com>
A gentle reminder for review.
Thanks
Nikunj
> ---
> arch/x86/include/asm/sev.h | 5 ++++-
> arch/x86/coco/sev/core.c | 24 ++++++++++++++++++++++++
> 2 files changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
> index 58e028d42e41..655d7e37bbcc 100644
> --- a/arch/x86/include/asm/sev.h
> +++ b/arch/x86/include/asm/sev.h
> @@ -282,8 +282,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 b6db4e0b936b..ffd44712cec0 100644
> --- a/arch/x86/coco/sev/core.c
> +++ b/arch/x86/coco/sev/core.c
> @@ -2167,15 +2167,39 @@ static unsigned long securetsc_get_tsc_khz(void)
>
> 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);
>
> + /*
> + * 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;
> + */
> + snp_tsc_freq_khz -= (snp_tsc_freq_khz * secrets->tsc_factor) / 100000;
> +
> x86_platform.calibrate_cpu = securetsc_get_tsc_khz;
> x86_platform.calibrate_tsc = securetsc_get_tsc_khz;
> +
> + early_memunmap(mem, PAGE_SIZE);
> }
>
> base-commit: 337964c8abfbef645cbbe25245e25c11d9d1fc4c
> --
> 2.43.0
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/sev: Use TSC_FACTOR for Secure TSC frequency calculation
2025-06-24 4:13 ` Nikunj A Dadhania
@ 2025-06-24 19:04 ` Dionna Amalie Glaze
2025-06-25 4:55 ` Nikunj A. Dadhania
0 siblings, 1 reply; 6+ messages in thread
From: Dionna Amalie Glaze @ 2025-06-24 19:04 UTC (permalink / raw)
To: Nikunj A Dadhania
Cc: linux-kernel, bp, x86, tglx, mingo, dave.hansen, thomas.lendacky,
aik, stable
On Mon, Jun 23, 2025 at 9:17 PM Nikunj A Dadhania <nikunj@amd.com> wrote:
>
> Nikunj A Dadhania <nikunj@amd.com> writes:
>
> > 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")
> > Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Nikunj A Dadhania <nikunj@amd.com>
>
> A gentle reminder for review.
>
> Thanks
> Nikunj
>
> > ---
> > arch/x86/include/asm/sev.h | 5 ++++-
> > arch/x86/coco/sev/core.c | 24 ++++++++++++++++++++++++
> > 2 files changed, 28 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
> > index 58e028d42e41..655d7e37bbcc 100644
> > --- a/arch/x86/include/asm/sev.h
> > +++ b/arch/x86/include/asm/sev.h
> > @@ -282,8 +282,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 b6db4e0b936b..ffd44712cec0 100644
> > --- a/arch/x86/coco/sev/core.c
> > +++ b/arch/x86/coco/sev/core.c
> > @@ -2167,15 +2167,39 @@ static unsigned long securetsc_get_tsc_khz(void)
> >
> > 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);
> >
> > + /*
> > + * 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;
> > + */
> > + snp_tsc_freq_khz -= (snp_tsc_freq_khz * secrets->tsc_factor) / 100000;
> > +
To better match the documentation and to not store an intermediate
result in a global, it'd be
good to add local variables. I'm also not a big fan of ABI constants
in the implementation.
guest_tsc_freq_khz = (unsigned long)(tsc_freq_mhz * 1000);
snp_tsc_freq_khz = guest_tsc_freq_khz -
SNP_SCALE_TSC_FACTOR(guest_tsc_freq_khz * secrets->tsc_factor);
...in a header somewhere...
/**
* The SEV-SNP secrets page contains an encoding of the percentage decrease
* from nominal TSC frequency to mean TSC frequency due to clocking parameters.
* The encoding is in parts per 100,000, so the following is an
integer-based scaling macro.
*/
#define SNP_SCALE_TSC_FACTOR(x) ((x) / 100000)
> > x86_platform.calibrate_cpu = securetsc_get_tsc_khz;
> > x86_platform.calibrate_tsc = securetsc_get_tsc_khz;
> > +
> > + early_memunmap(mem, PAGE_SIZE);
> > }
> >
> > base-commit: 337964c8abfbef645cbbe25245e25c11d9d1fc4c
> > --
> > 2.43.0
>
--
-Dionna Glaze, PhD, CISSP, CCSP (she/her)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/sev: Use TSC_FACTOR for Secure TSC frequency calculation
2025-06-24 19:04 ` Dionna Amalie Glaze
@ 2025-06-25 4:55 ` Nikunj A. Dadhania
2025-06-25 13:31 ` Tom Lendacky
0 siblings, 1 reply; 6+ messages in thread
From: Nikunj A. Dadhania @ 2025-06-25 4:55 UTC (permalink / raw)
To: Dionna Amalie Glaze
Cc: linux-kernel, bp, x86, tglx, mingo, dave.hansen, thomas.lendacky,
aik, stable
Thanks for the review.
On 6/25/2025 12:34 AM, Dionna Amalie Glaze wrote:
> On Mon, Jun 23, 2025 at 9:17 PM Nikunj A Dadhania <nikunj@amd.com> wrote:
>>
>>> diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
>>> index b6db4e0b936b..ffd44712cec0 100644
>>> --- a/arch/x86/coco/sev/core.c
>>> +++ b/arch/x86/coco/sev/core.c
>>> @@ -2167,15 +2167,39 @@ static unsigned long securetsc_get_tsc_khz(void)
>>>
>>> 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);
>>>
>>> + /*
>>> + * 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;
>>> + */
>>> + snp_tsc_freq_khz -= (snp_tsc_freq_khz * secrets->tsc_factor) / 100000;
>>> +
>
> To better match the documentation and to not store an intermediate
> result in a global, it'd be
> good to add local variables.
As there is no branches, IMHO having intermediate result should be fine and not sure
if that will improve the readability as there will be three variables now in the function
(tsc_freq_mhz, guest_tsc_freq_khz and snp_tsc_freq_khz) adding to confusion.
> I'm also not a big fan of ABI constants
> in the implementation.
Sure, and we will need to move the comment above to the header as well.
>
> guest_tsc_freq_khz = (unsigned long)(tsc_freq_mhz * 1000);
> snp_tsc_freq_khz = guest_tsc_freq_khz -
> SNP_SCALE_TSC_FACTOR(guest_tsc_freq_khz * secrets->tsc_factor);
>
> ...in a header somewhere...
> /**
> * The SEV-SNP secrets page contains an encoding of the percentage decrease
> * from nominal TSC frequency to mean TSC frequency due to clocking parameters.
> * The encoding is in parts per 100,000, so the following is an
> integer-based scaling macro.
> */
> #define SNP_SCALE_TSC_FACTOR(x) ((x) / 100000)
How about something below:
diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
index 655d7e37bbcc..d4151f0aa03c 100644
--- a/arch/x86/include/asm/sev.h
+++ b/arch/x86/include/asm/sev.h
@@ -223,6 +223,18 @@ 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;
diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
index ffd44712cec0..9e1e8affb5a8 100644
--- a/arch/x86/coco/sev/core.c
+++ b/arch/x86/coco/sev/core.c
@@ -2184,19 +2184,8 @@ void __init snp_secure_tsc_init(void)
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);
-
- /*
- * 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;
- */
- snp_tsc_freq_khz -= (snp_tsc_freq_khz * secrets->tsc_factor) / 100000;
+ snp_tsc_freq_khz = (unsigned long) 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;
Regards,
Nikunj
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/sev: Use TSC_FACTOR for Secure TSC frequency calculation
2025-06-25 4:55 ` Nikunj A. Dadhania
@ 2025-06-25 13:31 ` Tom Lendacky
2025-06-25 14:03 ` Nikunj A. Dadhania
0 siblings, 1 reply; 6+ messages in thread
From: Tom Lendacky @ 2025-06-25 13:31 UTC (permalink / raw)
To: Nikunj A. Dadhania, Dionna Amalie Glaze
Cc: linux-kernel, bp, x86, tglx, mingo, dave.hansen, aik, stable
On 6/24/25 23:55, Nikunj A. Dadhania wrote:
>
> Thanks for the review.
>
> On 6/25/2025 12:34 AM, Dionna Amalie Glaze wrote:
>> On Mon, Jun 23, 2025 at 9:17 PM Nikunj A Dadhania <nikunj@amd.com> wrote:
>>>
>>>> diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
>>>> index b6db4e0b936b..ffd44712cec0 100644
>>>> --- a/arch/x86/coco/sev/core.c
>>>> +++ b/arch/x86/coco/sev/core.c
>>>> @@ -2167,15 +2167,39 @@ static unsigned long securetsc_get_tsc_khz(void)
>>>>
>>>> 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);
>>>>
>>>> + /*
>>>> + * 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;
>>>> + */
>>>> + snp_tsc_freq_khz -= (snp_tsc_freq_khz * secrets->tsc_factor) / 100000;
>>>> +
>>
>> To better match the documentation and to not store an intermediate
>> result in a global, it'd be
>> good to add local variables.
>
> As there is no branches, IMHO having intermediate result should be fine and not sure
> if that will improve the readability as there will be three variables now in the function
> (tsc_freq_mhz, guest_tsc_freq_khz and snp_tsc_freq_khz) adding to confusion.
>
>> I'm also not a big fan of ABI constants
>> in the implementation.
>
> Sure, and we will need to move the comment above to the header as well.
>
>>
>> guest_tsc_freq_khz = (unsigned long)(tsc_freq_mhz * 1000);
>> snp_tsc_freq_khz = guest_tsc_freq_khz -
>> SNP_SCALE_TSC_FACTOR(guest_tsc_freq_khz * secrets->tsc_factor);
>>
>> ...in a header somewhere...
>> /**
>> * The SEV-SNP secrets page contains an encoding of the percentage decrease
>> * from nominal TSC frequency to mean TSC frequency due to clocking parameters.
>> * The encoding is in parts per 100,000, so the following is an
>> integer-based scaling macro.
>> */
>> #define SNP_SCALE_TSC_FACTOR(x) ((x) / 100000)
>
> How about something below:
>
> diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
> index 655d7e37bbcc..d4151f0aa03c 100644
> --- a/arch/x86/include/asm/sev.h
> +++ b/arch/x86/include/asm/sev.h
> @@ -223,6 +223,18 @@ 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;
> diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
> index ffd44712cec0..9e1e8affb5a8 100644
> --- a/arch/x86/coco/sev/core.c
> +++ b/arch/x86/coco/sev/core.c
> @@ -2184,19 +2184,8 @@ void __init snp_secure_tsc_init(void)
>
> 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);
> -
> - /*
> - * 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;
> - */
> - snp_tsc_freq_khz -= (snp_tsc_freq_khz * secrets->tsc_factor) / 100000;
> + snp_tsc_freq_khz = (unsigned long) SNP_SCALE_TSC_FREQ(tsc_freq_mhz * 1000,
> + secrets->tsc_factor);
I would make any casts live in the macro. Although snp_tsc_freq_khz is a
u64, right, but is always returned/used as an unsigned long? I'm wondering
why it isn't defined as an unsigned long? Not sure how everything would look.
Thanks,
Tom
>
> x86_platform.calibrate_cpu = securetsc_get_tsc_khz;
> x86_platform.calibrate_tsc = securetsc_get_tsc_khz;
>
> Regards,
> Nikunj
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/sev: Use TSC_FACTOR for Secure TSC frequency calculation
2025-06-25 13:31 ` Tom Lendacky
@ 2025-06-25 14:03 ` Nikunj A. Dadhania
0 siblings, 0 replies; 6+ messages in thread
From: Nikunj A. Dadhania @ 2025-06-25 14:03 UTC (permalink / raw)
To: Tom Lendacky, Dionna Amalie Glaze
Cc: linux-kernel, bp, x86, tglx, mingo, dave.hansen, aik, stable
On 6/25/2025 7:01 PM, Tom Lendacky wrote:
> On 6/24/25 23:55, Nikunj A. Dadhania wrote:
>>
>> Thanks for the review.
>>
>> On 6/25/2025 12:34 AM, Dionna Amalie Glaze wrote:
>>> On Mon, Jun 23, 2025 at 9:17 PM Nikunj A Dadhania <nikunj@amd.com> wrote:
>> diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
>> index ffd44712cec0..9e1e8affb5a8 100644
>> --- a/arch/x86/coco/sev/core.c
>> +++ b/arch/x86/coco/sev/core.c
>> @@ -2184,19 +2184,8 @@ void __init snp_secure_tsc_init(void)
>>
>> 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);
>> -
>> - /*
>> - * 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;
>> - */
>> - snp_tsc_freq_khz -= (snp_tsc_freq_khz * secrets->tsc_factor) / 100000;
>> + snp_tsc_freq_khz = (unsigned long) SNP_SCALE_TSC_FREQ(tsc_freq_mhz * 1000,
>> + secrets->tsc_factor);
>
> I would make any casts live in the macro. Although snp_tsc_freq_khz is a
> u64, right, but is always returned/used as an unsigned long? I'm wondering
> why it isn't defined as an unsigned long? Not sure how everything would look.
The unsigned long requirement came from the calibrate callbacks:
arch/x86/include/asm/x86_init.h:312: unsigned long (*calibrate_cpu)(void);
arch/x86/include/asm/x86_init.h:313: unsigned long (*calibrate_tsc)(void);
But as you suggested we can drop the cast here and securetsc_get_tsc_khz() should
cast the return to unsigned long. I am trying to recall why didn't we do this in the
first place.
@@ -2162,20 +2162,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;
}
And:
- 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);
I will send an updated patch with the above changes.
Regards
Nikunj
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-06-25 14:03 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-09 8:08 [PATCH] x86/sev: Use TSC_FACTOR for Secure TSC frequency calculation Nikunj A Dadhania
2025-06-24 4:13 ` Nikunj A Dadhania
2025-06-24 19:04 ` Dionna Amalie Glaze
2025-06-25 4:55 ` Nikunj A. Dadhania
2025-06-25 13:31 ` Tom Lendacky
2025-06-25 14:03 ` Nikunj A. Dadhania
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).