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: <457b00a1-ee0e-4435-9066-8ba587484e0f@amd.com>
Date: Wed, 25 Jun 2025 19:33:19 +0530
From: "Nikunj A. Dadhania" <nikunj@....com>
To: Tom Lendacky <thomas.lendacky@....com>,
 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,
 aik@....com, stable@...r.kernel.org
Subject: Re: [PATCH] x86/sev: Use TSC_FACTOR for Secure TSC frequency
 calculation



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