Re: [PATCH v3 13/15] x86/cpu/intel: Bound the non-architectural constant_tsc model checks

[David Woodhouse <dwmw2@infradead.org>]

[-- Attachment #1: Type: text/plain, Size: 3876 bytes --]

On Wed, 2025-02-19 at 18:41 +0000, Sohil Mehta wrote:
> X86_FEATURE_CONSTANT_TSC is a Linux-defined, synthesized feature flag.
> It is used across several vendors. Intel CPUs will set the feature when
> the architectural CPUID.80000007.EDX[1] bit is set. There are also some
> Intel CPUs that have the X86_FEATURE_CONSTANT_TSC behavior but don't
> enumerate it with the architectural bit.  Those currently have a model
> range check.
> 
> Today, virtually all of the CPUs that have the CPUID bit *also* match
> the "model >= 0x0e" check. This is confusing. Instead of an open-ended
> check, pick some models (INTEL_IVYBRIDGE and P4_WILLAMETTE) as the end
> of goofy CPUs that should enumerate the bit but don't.  These models are
> relatively arbitrary but conservative pick for this.
> 
> This makes it obvious that later CPUs (like Family 18+) no longer need
> to synthesize X86_FEATURE_CONSTANT_TSC.
> 
> Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
> ---
> v3: Make the non-architectural model checks more explicit.
>     Improve commit message.
> 
> v2: No change.
> ---
>  arch/x86/kernel/cpu/intel.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> index fc68561d9f92..4fbc5465ca67 100644
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -210,10 +210,6 @@ static void early_init_intel(struct cpuinfo_x86 *c)
>  {
>  	u64 misc_enable;
>  
> -	if ((c->x86 == 0xf && c->x86_model >= 0x03) ||
> -		(c->x86 == 0x6 && c->x86_model >= 0x0e))
> -		set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
> -
>  	if (c->x86 >= 6 && !cpu_has(c, X86_FEATURE_IA64))
>  		c->microcode = intel_get_microcode_revision();
>  
> @@ -266,10 +262,16 @@ static void early_init_intel(struct cpuinfo_x86 *c)
>  	 *
>  	 * It is also reliable across cores and sockets. (but not across
>  	 * cabinets - we turn it off in that case explicitly.)
> +	 *
> +	 * Use a model-specific check for some older CPUs that have invariant
> +	 * TSC but may not report it architecturally via 8000_0007.
>  	 */
>  	if (c->x86_power & (1 << 8)) {
>  		set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
>  		set_cpu_cap(c, X86_FEATURE_NONSTOP_TSC);
> +	} else if ((c->x86_vfm >= INTEL_P4_PRESCOTT && c->x86_vfm <= INTEL_P4_WILLAMETTE) ||
> +		   (c->x86_vfm >= INTEL_CORE_YONAH  && c->x86_vfm <= INTEL_IVYBRIDGE)) {
> +		set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
>  	}
>  
>  	/* Penwell and Cloverview have the TSC which doesn't sleep on S3 */

Hm. My test host is INTEL_HASWELL_X (0x63f). For reasons which are
unclear to me, QEMU doesn't set bit 8 of 0x80000007 EDX unless I
explicitly append ',+invtsc' to the existing '-cpu host' on its command
line. So now my guest doesn't think it has X86_FEATURE_CONSTANT_TSC.

For reasons I also don't understand, for a Xen (in qemu/kvm) guest this
results in about a four-second delay when bringing up each vCPU.
Timestamps added to QEMU's stdout because the kernel's own timestamps
are lying...

1755781767: [    0.489434] smp: Bringing up secondary CPUs ...
1755781767: [    0.489434] installing Xen timer for CPU 1
1755781767: [    0.489434] smpboot: x86: Booting SMP configuration:
1755781767: [    0.489434] .... node  #0, CPUs:          #1
1755781767: [    0.489434] installing Xen timer for CPU 2
1755781767: [    0.489434]    #2
1755781767: [    0.489434] installing Xen timer for CPU 3
1755781767: [    0.489434]    #3
1755781771: [    0.489434] cpu 1 spinlock event irq 45
1755781775: [    0.544262] cpu 2 spinlock event irq 46
1755781779: [    0.604306] cpu 3 spinlock event irq 47
1755781779: [    0.604792] smp: Brought up 1 node, 4 CPUs
1755781779: [    0.604792] smpboot: Total of 4 processors activated (1034118.38 BogoMIPS)




[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5069 bytes --]