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.
>
