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

[Sohil Mehta <sohil.mehta@intel.com>]

On 2/11/2025 1:41 PM, Dave Hansen wrote:
> On 2/11/25 11:44, Sohil Mehta wrote:
>> Constant TSC has been architectural on Intel CPUs for a while. Supported
>> CPUs use the architectural Invariant TSC bit in CPUID.80000007. A
>> Family-model check is not required for these CPUs.
>>
>> Prevent unnecessary confusion but restricting the model specific checks
>> to CPUs that need it and moving it closer to the architectural check.
>>
>> Invariant TSC was likely introduced around the Nehalam timeframe on the
>> Xeon side and Saltwell timeframe on the Atom side.  Due to interspersed
>> model numbers extend the non-architectural capability setting until
>> Ivybridge to be safe.
> 
> How about:
> 
> 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.
> 

Looks much better.

>> +	/* Some older CPUs have invariant TSC but may not report it architecturally via 8000_0007 */
>> +	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);
> 
> Please do vertically align this too.
> 
> Would it make logical sense to do:
> 
>         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 ...
> 
> ?
> 
> That would make it *totally* clear that it's an either/or situation.  Right?
> 

Yup, will change it.

>