| Message ID | 20241021055156.2342564-10-nikunj@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add Secure TSC support for SNP guests | expand |
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. > > #endif /* CONFIG_AMD_MEM_ENCRYPT */ > > diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c > index 4e9b1cc1f26b..154d568c59cf 100644 > --- a/arch/x86/coco/sev/core.c > +++ b/arch/x86/coco/sev/core.c > @@ -3065,3 +3065,19 @@ void __init snp_secure_tsc_prepare(void) > > pr_debug("SecureTSC enabled"); > } > + > +static unsigned long securetsc_get_tsc_khz(void) > +{ > + unsigned long long tsc_freq_mhz; > + > + setup_force_cpu_cap(X86_FEATURE_TSC_KNOWN_FREQ); > + rdmsrl(MSR_AMD64_GUEST_TSC_FREQ, tsc_freq_mhz); > + > + return (unsigned long)(tsc_freq_mhz * 1000); > +} > + > +void __init securetsc_init(void) > +{ > + x86_platform.calibrate_cpu = securetsc_get_tsc_khz; > + x86_platform.calibrate_tsc = securetsc_get_tsc_khz; > +} > 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? Thanks, Tom > + > if (!determine_cpu_tsc_frequencies(true)) > return; > tsc_enable_sched_clock();
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
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) { } #endif /* CONFIG_AMD_MEM_ENCRYPT */ diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c index 4e9b1cc1f26b..154d568c59cf 100644 --- a/arch/x86/coco/sev/core.c +++ b/arch/x86/coco/sev/core.c @@ -3065,3 +3065,19 @@ void __init snp_secure_tsc_prepare(void) pr_debug("SecureTSC enabled"); } + +static unsigned long securetsc_get_tsc_khz(void) +{ + unsigned long long tsc_freq_mhz; + + setup_force_cpu_cap(X86_FEATURE_TSC_KNOWN_FREQ); + rdmsrl(MSR_AMD64_GUEST_TSC_FREQ, tsc_freq_mhz); + + return (unsigned long)(tsc_freq_mhz * 1000); +} + +void __init securetsc_init(void) +{ + x86_platform.calibrate_cpu = securetsc_get_tsc_khz; + x86_platform.calibrate_tsc = securetsc_get_tsc_khz; +} 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(); + if (!determine_cpu_tsc_frequencies(true)) return; tsc_enable_sched_clock();
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(+)