linux-kernel - Re: [PATCH] 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] [day] [month] [year] [list]
Message-ID: <CAAH4kHZPrDRF3sZ2GxFxMeue3o9PsEL7p-j8bKL2mxgBjR0ATg@mail.gmail.com>
Date: Tue, 24 Jun 2025 12:04:45 -0700
From: Dionna Amalie Glaze <dionnaglaze@...gle.com>
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, stable@...r.kernel.org
Subject: Re: [PATCH] x86/sev: Use TSC_FACTOR for Secure TSC frequency calculation

On Mon, Jun 23, 2025 at 9:17 PM Nikunj A Dadhania <nikunj@....com> wrote:
>
> Nikunj A Dadhania <nikunj@....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@....com>
> > Cc: stable@...r.kernel.org
> > Signed-off-by: Nikunj A Dadhania <nikunj@....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)