Re: [PATCH v2 05/17] x86/cpu/intel: Fix page copy performance for extended Families

[Sohil Mehta]

On 2/11/2025 4:54 PM, Andrew Cooper wrote:

> If you're going to override the BIOS setting, then you need to
> explicitly set MSR_MISC_ENABLE.FAST_STRINGS.
> 
> Otherwise you're claiming to Linux that REP is good even when hardware
> is prohibited from using optimisations.
> 

I think the current checks have unnecessary overlap which makes them
confusing. We should be fine if we only rely on the architectural
MSR_MISC_ENABLE.FAST_STRINGS bit and rely just on the BIOS setting. My
justification is below.

The simplified version of the current checks is as follows:

Check 1 (Based on Family Model numbers):
> /*
>  * Unconditionally set REP_GOOD on early Family 6 processors
>  */
> if (IS_ENABLED(CONFIG_X86_64) &&
>     (c->x86_vfm >= INTEL_PENTIUM_PRO && c->x86_vfm < INTEL_PENTIUM_M_DOTHAN))
> 	set_cpu_cap(c, X86_FEATURE_REP_GOOD);

This check is mostly redundant since it is targeted for 64 bit and very
few if any of those CPUs support 64 bit processing. I suggest that we
get rid of this check completely. The risk here is fairly limited as well.

Check 2 (Based on MISC_ENABLE.FAST_STRING):
> /*
>  * 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) {
> 		/* X86_FEATURE_ERMS will be automatically set based on CPUID */
> 		set_cpu_cap(c, X86_FEATURE_REP_GOOD);
> 	} else {
> 		pr_info("Disabled fast string operations\n");
> 		setup_clear_cpu_cap(X86_FEATURE_REP_GOOD);
> 		setup_clear_cpu_cap(X86_FEATURE_ERMS);
> 	}
> }

This is the only real check that is needed and should likely suffice in
all meaningful scenarios.

Comments?