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

[Tom Lendacky <thomas.lendacky@amd.com>]

On 6/26/25 05:01, Nikunj A. Dadhania wrote:
> 
> 
> On 6/26/2025 1:56 PM, Ingo Molnar wrote:
>>
>> * Nikunj A Dadhania <nikunj@amd.com> 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?

I think Ingo is saying this can be ((freq) - (freq) * (factor) / 100000)

in other words, getting rid of the parentheses around the multiplication.

Thanks,
Tom

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