linux-kernel - Re: [RFC PATCH 03/15] x86/cpu/intel: Fix init_intel() checks for extended family numbers

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] [day] [month] [year] [list]

Message-ID: <369605f0-9d6c-4a5c-bfc0-f480b537f34d@intel.com>
Date: Mon, 23 Dec 2024 15:41:54 -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>, <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: [RFC PATCH 03/15] x86/cpu/intel: Fix init_intel() checks for
 extended family numbers

On 12/20/2024 3:27 PM, Dave Hansen wrote:
> On 12/20/24 13:36, Sohil Mehta wrote:
>> X86_FEATURE_REP_GOOD is only set for family 6 processors.  Extend the
>> check to family numbers beyond 15.
> 
> Could you explain why, please?
> 

To answer this I was trying to understand where Fast string
(X86_FEATURE_REP_GOOD) is used. It looks like copy_page() in
lib/copy_page_64.S is the only place it really matters. clear_page() in
include/asm/page_64.h would likely use Enhanced fast strings (ERMS) if
available.

Would it be correct to say that copy_page() and potentially clear_page()
would be slower on Family 18/19 CPUs without the fix?

>> It is uncertain whether the Pentium 4s (family 15) should set the
>> feature flag as well. Commit 185f3b9da24c ("x86: make intel.c have
>> 64-bit support code") that originally set X86_FEATURE_REP_GOOD also set
>> the x86_cache_alignment preference for family 15 processors. The
>> omission of the family 15 seems intentional.
>>

Analyzing more, it seems the below check in early_init_intel() is not
really effective.

/*
 * If fast string is not enabled in IA32_MISC_ENABLE for any reason,
 * clear the fast string and enhanced fast string CPU capabilities.
 */
if (c->x86_vfm >= INTEL_PENTIUM_M_DOTHAN) {
	rdmsrl(MSR_IA32_MISC_ENABLE, misc_enable);
	if (!(misc_enable & MSR_IA32_MISC_ENABLE_FAST_STRING)) {
		pr_info("Disabled fast string operations\n");
		setup_clear_cpu_cap(X86_FEATURE_REP_GOOD);
		setup_clear_cpu_cap(X86_FEATURE_ERMS);
	}
}


It gets overridden later in intel_init() with the below code:

if (c->x86 == 6)
	set_cpu_cap(c, X86_FEATURE_REP_GOOD);

Shouldn't the order of this be the other way around?

>> Also, the 32-bit user copy alignment preference is only set for family 6
>> and 15 processors. Extend the preference to family numbers beyond 15.
> 
> Can you please provide some more context so it's clear which hunk this
> refers to?  Alternatively, can you break this out into a separate patch?
> 

This is referring to the below chunk. Separating it seems like a better
idea to avoid ambiguity.

> -
>  #ifdef CONFIG_X86_INTEL_USERCOPY
>  	/*
>  	 * Set up the preferred alignment for movsl bulk memory moves
> +	 * Family 4 - 486: untested
> +	 * Family 5 - Pentium: untested
> +	 * Family 6 - PII/PIII only like movsl with 8-byte alignment
> +	 * Family 15 - P4 is OK down to 8-byte alignment
>  	 */
> -	switch (c->x86) {
> -	case 4:		/* 486: untested */
> -		break;
> -	case 5:		/* Old Pentia: untested */
> -		break;
> -	case 6:		/* PII/PIII only like movsl with 8-byte alignment */
> -		movsl_mask.mask = 7;
> -		break;
> -	case 15:	/* P4 is OK down to 8-byte alignment */
> +	if (c->x86_vfm >= INTEL_PENTIUM_PRO)
>  		movsl_mask.mask = 7;
> -		break;
> -	}
>  #endif