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

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

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

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.

Thanks,
Tom

>  
>  	x86_platform.calibrate_cpu = securetsc_get_tsc_khz;
>  	x86_platform.calibrate_tsc = securetsc_get_tsc_khz;
> 
> Regards,
> Nikunj