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] [thread-next>] [day] [month] [year] [list]

Message-ID: <8e0807b2-eef5-4172-8c9f-3e374a818344@amd.com>
Date: Wed, 25 Jun 2025 10:25:13 +0530
From: "Nikunj A. Dadhania" <nikunj@....com>
To: Dionna Amalie Glaze <dionnaglaze@...gle.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


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