linux-kernel - Re: [PATCH v2 15/17] x86/cpu/intel: Bound the non-architectural constant

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <49278389-9340-406f-ac82-204538b047f1@intel.com>
Date: Tue, 11 Feb 2025 16:45:46 -0800
From: Sohil Mehta <sohil.mehta@...el.com>
To: Dave Hansen <dave.hansen@...el.com>, <x86@...nel.org>, Dave Hansen
	<dave.hansen@...ux.intel.com>, Tony Luck <tony.luck@...el.com>
CC: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>,
	Arnaldo Carvalho de Melo <acme@...nel.org>, Namhyung Kim
	<namhyung@...nel.org>, Mark Rutland <mark.rutland@....com>, "Alexander
 Shishkin" <alexander.shishkin@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>,
	Ian Rogers <irogers@...gle.com>, Adrian Hunter <adrian.hunter@...el.com>,
	"Kan Liang" <kan.liang@...ux.intel.com>, Thomas Gleixner
	<tglx@...utronix.de>, Borislav Petkov <bp@...en8.de>, "H . Peter Anvin"
	<hpa@...or.com>, "Rafael J . Wysocki" <rafael@...nel.org>, Len Brown
	<lenb@...nel.org>, Andy Lutomirski <luto@...nel.org>, Viresh Kumar
	<viresh.kumar@...aro.org>, Fenghua Yu <fenghua.yu@...el.com>, Jean Delvare
	<jdelvare@...e.com>, Guenter Roeck <linux@...ck-us.net>, Zhang Rui
	<rui.zhang@...el.com>, Andrew Cooper <andrew.cooper3@...rix.com>, David
 Laight <david.laight.linux@...il.com>, <linux-perf-users@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <linux-acpi@...r.kernel.org>,
	<linux-pm@...r.kernel.org>, <linux-hwmon@...r.kernel.org>
Subject: Re: [PATCH v2 15/17] x86/cpu/intel: Bound the non-architectural
 constant_tsc model checks

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.

>