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