Extract retrieval of TSC frequency information from CPUID into standalone
helpers so that TDX guest support and kvmlock can reuse the logic. Provide
a version that includes the multiplier math as TDX in particular does NOT
want to use native_calibrate_tsc()'s fallback logic that derives the TSC
frequency based on CPUID.0x16 when the core crystal frequency isn't known.
No functional change intended.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
arch/x86/include/asm/tsc.h | 41 ++++++++++++++++++++++++++++++++++++++
arch/x86/kernel/tsc.c | 14 ++-----------
2 files changed, 43 insertions(+), 12 deletions(-)
diff --git a/arch/x86/include/asm/tsc.h b/arch/x86/include/asm/tsc.h
index 94408a784c8e..14a81a66b37c 100644
--- a/arch/x86/include/asm/tsc.h
+++ b/arch/x86/include/asm/tsc.h
@@ -28,6 +28,47 @@ static inline cycles_t get_cycles(void)
}
#define get_cycles get_cycles
+static inline int cpuid_get_tsc_info(unsigned int *crystal_khz,
+ unsigned int *denominator,
+ unsigned int *numerator)
+{
+ unsigned int ecx_hz, edx;
+
+ if (boot_cpu_data.cpuid_level < CPUID_LEAF_TSC)
+ return -ENOENT;
+
+ *crystal_khz = *denominator = *numerator = ecx_hz = edx = 0;
+
+ /* CPUID 15H TSC/Crystal ratio, plus optionally Crystal Hz */
+ cpuid(CPUID_LEAF_TSC, denominator, numerator, &ecx_hz, &edx);
+
+ if (!*denominator || !*numerator)
+ return -ENOENT;
+
+ /*
+ * Note, some CPUs provide the multiplier information, but not the core
+ * crystal frequency. The multiplier information is still useful for
+ * such CPUs, as the crystal frequency can be gleaned from CPUID.0x16.
+ */
+ *crystal_khz = ecx_hz / 1000;
+ return 0;
+}
+
+static inline int cpuid_get_tsc_freq(unsigned int *tsc_khz,
+ unsigned int *crystal_khz)
+{
+ unsigned int denominator, numerator;
+
+ if (cpuid_get_tsc_info(tsc_khz, &denominator, &numerator))
+ return -ENOENT;
+
+ if (!*crystal_khz)
+ return -ENOENT;
+
+ *tsc_khz = *crystal_khz * numerator / denominator;
+ return 0;
+}
+
extern void tsc_early_init(void);
extern void tsc_init(void);
extern void mark_tsc_unstable(char *reason);
diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 34dec0b72ea8..e3faa2b36910 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -661,25 +661,15 @@ static unsigned long quick_pit_calibrate(void)
*/
unsigned long native_calibrate_tsc(void)
{
- unsigned int eax_denominator, ebx_numerator, ecx_hz, edx;
+ unsigned int eax_denominator, ebx_numerator;
unsigned int crystal_khz;
if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL)
return 0;
- if (boot_cpu_data.cpuid_level < CPUID_LEAF_TSC)
+ if (cpuid_get_tsc_info(&crystal_khz, &eax_denominator, &ebx_numerator))
return 0;
- eax_denominator = ebx_numerator = ecx_hz = edx = 0;
-
- /* CPUID 15H TSC/Crystal ratio, plus optionally Crystal Hz */
- cpuid(CPUID_LEAF_TSC, &eax_denominator, &ebx_numerator, &ecx_hz, &edx);
-
- if (ebx_numerator == 0 || eax_denominator == 0)
- return 0;
-
- crystal_khz = ecx_hz / 1000;
-
/*
* Denverton SoCs don't report crystal clock, and also don't support
* CPUID_LEAF_FREQ for the calculation below, so hardcode the 25MHz
--
2.48.1.362.g079036d154-googOn Fri, Jan 31, 2025 at 06:17:03PM -0800, Sean Christopherson wrote:
> Extract retrieval of TSC frequency information from CPUID into standalone
> helpers so that TDX guest support and kvmlock can reuse the logic. Provide
> a version that includes the multiplier math as TDX in particular does NOT
> want to use native_calibrate_tsc()'s fallback logic that derives the TSC
> frequency based on CPUID.0x16 when the core crystal frequency isn't known.
>
> No functional change intended.
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
> arch/x86/include/asm/tsc.h | 41 ++++++++++++++++++++++++++++++++++++++
> arch/x86/kernel/tsc.c | 14 ++-----------
> 2 files changed, 43 insertions(+), 12 deletions(-)
>
> diff --git a/arch/x86/include/asm/tsc.h b/arch/x86/include/asm/tsc.h
> index 94408a784c8e..14a81a66b37c 100644
> --- a/arch/x86/include/asm/tsc.h
> +++ b/arch/x86/include/asm/tsc.h
Bah, why in the header as inlines? Just leave them in tsc.c and call them...
> @@ -28,6 +28,47 @@ static inline cycles_t get_cycles(void)
> }
> #define get_cycles get_cycles
>
> +static inline int cpuid_get_tsc_info(unsigned int *crystal_khz,
> + unsigned int *denominator,
> + unsigned int *numerator)
Can we pls do a
struct cpuid_tsc_info {
unsigned int denominator;
unsigned int numerator;
unsigned int crystal_khz;
unsigned int tsc_khz;
}
and hand that around instead of those I/O pointers?
It would make the code a bit saner to stare at and follow.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Tue, Feb 11, 2025, Borislav Petkov wrote:
> On Fri, Jan 31, 2025 at 06:17:03PM -0800, Sean Christopherson wrote:
> > Extract retrieval of TSC frequency information from CPUID into standalone
> > helpers so that TDX guest support and kvmlock can reuse the logic. Provide
> > a version that includes the multiplier math as TDX in particular does NOT
> > want to use native_calibrate_tsc()'s fallback logic that derives the TSC
> > frequency based on CPUID.0x16 when the core crystal frequency isn't known.
> >
> > No functional change intended.
> >
> > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > ---
> > arch/x86/include/asm/tsc.h | 41 ++++++++++++++++++++++++++++++++++++++
> > arch/x86/kernel/tsc.c | 14 ++-----------
> > 2 files changed, 43 insertions(+), 12 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/tsc.h b/arch/x86/include/asm/tsc.h
> > index 94408a784c8e..14a81a66b37c 100644
> > --- a/arch/x86/include/asm/tsc.h
> > +++ b/arch/x86/include/asm/tsc.h
>
> Bah, why in the header as inlines?
Because obviously optimizing code that's called once during boot is super
critical?
> Just leave them in tsc.c and call them...
>
> > @@ -28,6 +28,47 @@ static inline cycles_t get_cycles(void)
> > }
> > #define get_cycles get_cycles
> >
> > +static inline int cpuid_get_tsc_info(unsigned int *crystal_khz,
> > + unsigned int *denominator,
> > + unsigned int *numerator)
>
> Can we pls do a
>
> struct cpuid_tsc_info {
> unsigned int denominator;
> unsigned int numerator;
> unsigned int crystal_khz;
> unsigned int tsc_khz;
> }
>
> and hand that around instead of those I/O pointers?
Ah, yeah, that's way better.
On Tue, Feb 11, 2025 at 09:25:47AM -0800, Sean Christopherson wrote:
> Because obviously optimizing code that's called once during boot is super
> critical?
Because let's stick 'em where they belong and keep headers containing only
small, trivial and inlineable functions. Having unusually big functions in
a header triggers my weird code patterns detector. :)
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Tue, Feb 11, 2025, Borislav Petkov wrote: > On Tue, Feb 11, 2025 at 09:25:47AM -0800, Sean Christopherson wrote: > > Because obviously optimizing code that's called once during boot is super > > critical? > > Because let's stick 'em where they belong and keep headers containing only > small, trivial and inlineable functions. LOL, sorry, I was being sarcastic and poking fun at myself. I completely agree there's no reason to make them inline.
On Fri, Jan 31, 2025, Sean Christopherson wrote:
> +static inline int cpuid_get_tsc_freq(unsigned int *tsc_khz,
> + unsigned int *crystal_khz)
> +{
> + unsigned int denominator, numerator;
> +
> + if (cpuid_get_tsc_info(tsc_khz, &denominator, &numerator))
As pointed out by Dan, this is broken. It should be crystal_khz, not tsc_khz.
I fixed the bug in my test build but clobbered it before posting.
> + return -ENOENT;
> +
> + if (!*crystal_khz)
> + return -ENOENT;
> +
> + *tsc_khz = *crystal_khz * numerator / denominator;
> + return 0;
> +}
Sean Christopherson <seanjc@google.com> writes: > Extract retrieval of TSC frequency information from CPUID into standalone > helpers so that TDX guest support and kvmlock can reuse the logic. Provide s/kvmlock/kvmclock > a version that includes the multiplier math as TDX in particular does NOT > want to use native_calibrate_tsc()'s fallback logic that derives the TSC > frequency based on CPUID.0x16 when the core crystal frequency isn't known. > > No functional change intended. > > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- ... > + > +static inline int cpuid_get_tsc_freq(unsigned int *tsc_khz, > + unsigned int *crystal_khz) Should we add this in patch 6/16 where it is being used for the first time ? Regards Nikunj
On Mon, Feb 03, 2025, Nikunj A Dadhania wrote: > Sean Christopherson <seanjc@google.com> writes: > > Extract retrieval of TSC frequency information from CPUID into standalone > > helpers so that TDX guest support and kvmlock can reuse the logic. Provide > > s/kvmlock/kvmclock > > > a version that includes the multiplier math as TDX in particular does NOT > > want to use native_calibrate_tsc()'s fallback logic that derives the TSC > > frequency based on CPUID.0x16 when the core crystal frequency isn't known. > > > > No functional change intended. > > > > Signed-off-by: Sean Christopherson <seanjc@google.com> > > --- > > ... > > > + > > +static inline int cpuid_get_tsc_freq(unsigned int *tsc_khz, > > + unsigned int *crystal_khz) > > Should we add this in patch 6/16 where it is being used for the first time ? No strong preference on my end. I put it here mostly to keep each patch focused on a single subsystem where possible, since the series touches so many areas. I also wanted to show the "full" API in a single patch, but I agree that adding a helper without a user is generally undesirable.
© 2016 - 2025 Red Hat, Inc.