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

["Nikunj A. Dadhania" <nikunj@amd.com>]

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