Re: [PATCH v2 2/3] x86: Remove vendor checks from prefer_mwait_c1_over_halt

[Wyes Karny]

Hello Dave,

On 5/5/2022 10:34 PM, Dave Hansen wrote:
> On 5/5/22 04:01, Wyes Karny wrote:
>> -	if (c->x86_vendor != X86_VENDOR_INTEL)
>> +	/* MWAIT is not supported on this platform. Fallback to HALT */
>> +	if (!cpu_has(c, X86_FEATURE_MWAIT))
>> +		return 0;
>> +
>> +	/* Monitor has a bug. Fallback to HALT */
>> +	if (boot_cpu_has_bug(X86_BUG_MONITOR))
>>  		return 0;
>>  
>> -	if (!cpu_has(c, X86_FEATURE_MWAIT) || boot_cpu_has_bug(X86_BUG_MONITOR))
>> +	if (c->cpuid_level < CPUID_MWAIT_LEAF)
>>  		return 0;
> 
> First of all, thanks for all the detail in this new version of the patches!
> 
> In arch/x86/kernel/cpu/common.c, we have:
> 
> cpuid_dependent_features[] = {
>         { X86_FEATURE_MWAIT,            0x00000005 },
> 	...
> 
> Shouldn't that clear X86_FEATURE_MWAIT on all systems without
> CPUID_MWAIT_LEAF?  That would make the c->cpuid_level check above
> unnecessary.

Agreed. I will update it in the next version.

> 
>> +	/*
>> +	 * If ECX doesn't have extended info about MWAIT,
>> +	 * don't need to check substates.
>> +	 */
>> +	if (!(ecx & CPUID5_ECX_EXTENSIONS_SUPPORTED))
>> +		return 1;
> 
> Could you explain a bit more about this?  I don't know this CPUID leaf
> off the top of my head and the line after this is checking EDX.  It's
> not apparent from this comment why "!ECX_EXTENSIONS_SUPPORTED" means
> that MWAIT should be preferred.

MWAIT can be used for two cases - address range monitoring and advanced power management.

According to Intel Architectures Software Developer’s Manual, Volume2B:

"MWAIT accepts a hint and optional extension to the processor that it 
can enter a specified target C state while waiting for an event or a store 
operation to the address range armed by MONITOR. Support for MWAIT extensions
for power management is indicated by CPUID.05H:ECX[bit 0] reporting 1.

EAX and ECX are used to communicate the additional information to the MWAIT instruction,
such as the kind of optimized state the processor should enter.

ECX specifies optional extensions for the MWAIT instruction.
EAX may contain hints such as the preferred optimized state the processor should enter."

So, if CPUID.05H:ECX[0] = 0, MWAIT extensions are not available (not allowed) and hence 
it is safe to use MWAIT with EAX=0,ECX=0. Otherwise, we have to check CPUID.05H:EDX[bit 7:4]
to get the number of C1 substates and this should be greater than equal to 1.

> 
>> +	/* Check, whether at least 1 MWAIT C1 substate is present */
>> +	return (edx & MWAIT_C1_SUBSTATE_MASK);
> 

Thanks,
Wyes