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

[Nikunj A. Dadhania]

On 6/26/2025 1:56 PM, Ingo Molnar wrote:
> 
> * Nikunj A Dadhania <nikunj@xxxxxxx> wrote:
> 
>> diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
>> index fbb616fcbfb8..869355367210 100644
>> --- a/arch/x86/include/asm/sev.h
>> +++ b/arch/x86/include/asm/sev.h
>> @@ -223,6 +223,19 @@ 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)
> 
> Nit: there's really no need to use parentheses in this expression,
> 'x * y / z' is equivalent and fine.

It will give wrong scale if I call with freq as "tsc + 1000000" 
without the parentheses?

SNP_SCALE_TSC_FREQ(tsc + 1000000, factor)

>> diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
>> index 8375ca7fbd8a..36f419ff25d4 100644
>> --- a/arch/x86/coco/sev/core.c
>> +++ b/arch/x86/coco/sev/core.c
>> @@ -2156,20 +2156,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;
> 
> This forced type cast is a signature of poor type choices. Please 
> harmonize the types of snp_tsc_freq_khz and securetsc_get_tsc_khz() to 
> avoid the type cast altogether. 

Sure, I can attempt that and send an updated patch.

> Does this code even get built and run on 32-bit kernels?

This code should not build for 32-bit kernels.

Thanks
Nikunj