Re: [PATCH v13 09/13] tsc: Use the GUEST_TSC_FREQ MSR for discovering TSC frequency

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

On 10/21/2024 8:03 PM, Tom Lendacky wrote:
> On 10/21/24 00:51, Nikunj A Dadhania wrote:
>> Calibrating the TSC frequency using the kvmclock is not correct for
>> SecureTSC enabled guests. Use the platform provided TSC frequency via the
>> GUEST_TSC_FREQ MSR (C001_0134h).
>>
>> Signed-off-by: Nikunj A Dadhania <nikunj@amd.com>
>> ---
>>  arch/x86/include/asm/sev.h |  2 ++
>>  arch/x86/coco/sev/core.c   | 16 ++++++++++++++++
>>  arch/x86/kernel/tsc.c      |  5 +++++
>>  3 files changed, 23 insertions(+)
>>
>> diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
>> index 9169b18eeb78..34f7b9fc363b 100644
>> --- a/arch/x86/include/asm/sev.h
>> +++ b/arch/x86/include/asm/sev.h
>> @@ -536,6 +536,7 @@ static inline int handle_guest_request(struct snp_msg_desc *mdesc, u64 exit_code
>>  }
>>  
>>  void __init snp_secure_tsc_prepare(void);
>> +void __init securetsc_init(void);
>>  
>>  #else	/* !CONFIG_AMD_MEM_ENCRYPT */
>>  
>> @@ -584,6 +585,7 @@ static inline int handle_guest_request(struct snp_msg_desc *mdesc, u64 exit_code
>>  				       u32 resp_sz) { return -ENODEV; }
>>  
>>  static inline void __init snp_secure_tsc_prepare(void) { }
>> +static inline void __init securetsc_init(void) { }
> 
> This should probably be named snp_securetsc_init() or
> snp_secure_tsc_init() (to be consistent with the function above it) so
> that it is in the snp namespace.

Ack.

>> diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
>> index dfe6847fd99e..c83f1091bb4f 100644
>> --- a/arch/x86/kernel/tsc.c
>> +++ b/arch/x86/kernel/tsc.c
>> @@ -30,6 +30,7 @@
>>  #include <asm/i8259.h>
>>  #include <asm/topology.h>
>>  #include <asm/uv/uv.h>
>> +#include <asm/sev.h>
>>  
>>  unsigned int __read_mostly cpu_khz;	/* TSC clocks / usec, not used here */
>>  EXPORT_SYMBOL(cpu_khz);
>> @@ -1514,6 +1515,10 @@ void __init tsc_early_init(void)
>>  	/* Don't change UV TSC multi-chassis synchronization */
>>  	if (is_early_uv_system())
>>  		return;
>> +
>> +	if (cc_platform_has(CC_ATTR_GUEST_SNP_SECURE_TSC))
>> +		securetsc_init();
> 
> Would this call be better in kvm_init_platform() or kvmclock_init()? Any
> reason it has to be here?

Added here to make sure that initialization happens on all the hypervisor,
not just for KVM. Moreover, kvmclock can be disabled from the command line.

Regards,
Nikunj