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

[Wyes Karny <wyes.karny@amd.com>]

Hello Rui,

On 5/20/2022 7:13 PM, Zhang Rui wrote:
> On Thu, 2022-05-19 at 09:00 -0700, Dave Hansen wrote:
>> On 5/10/22 03:18, Wyes Karny wrote:
>>>  static int prefer_mwait_c1_over_halt(const struct cpuinfo_x86 *c)
>>>  {
>>> +	u32 eax, ebx, ecx, edx;
>>> +
>>>  	/* User has disallowed the use of MWAIT. Fallback to HALT */
>>>  	if (boot_option_idle_override == IDLE_NOMWAIT)
>>>  		return 0;
>>>  
>>> -	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;
> 
> I'm new to x86 code, a dumb question, what about the other vendors?
> with this patch, prefer_mwait_c1_over_halt() can return 1 for other
> vendors as well?

This decision tree is based on cpuid features, so if the processor
advertises MWAIT C1 support we would be choosing MWAIT.
If any vendor wants to avoid choosing MWAIT (even cpuid shows MWAIT C1
support), they can explicitly mention. Will add exceptions form them.

> 
>>>  
>>> -	if (!cpu_has(c, X86_FEATURE_MWAIT) ||
>>> boot_cpu_has_bug(X86_BUG_MONITOR))
>>> +	/* Monitor has a bug. Fallback to HALT */
>>> +	if (boot_cpu_has_bug(X86_BUG_MONITOR))
>>>  		return 0;
>>
>> So, before, we pretty much just assume that all Intel CPUs with MWAIT
>> should use MWAIT C1.
>>
>>> -	return 1;
>>> +	cpuid(CPUID_MWAIT_LEAF, &eax, &ebx, &ecx, &edx);
>>> +
>>> +	/*
>>> +	 * If MWAIT extensions are not available, it is safe to use
>>> MWAIT
>>> +	 * with EAX=0, ECX=0.
>>> +	 */
>>> +	if (!(ecx & CPUID5_ECX_EXTENSIONS_SUPPORTED))
>>> +		return 1;
>>> +
>>> +	/*
>>> +	 * If MWAIT extensions are available, there should be least one
>>> +	 * MWAIT C1 substate present.
>>> +	 */
>>> +	return (edx & MWAIT_C1_SUBSTATE_MASK);
>>>  }
>>
>> So, I guess the "If MWAIT extensions are not available" check is
>> consistent with the "always use it on Intel" behavior.
>>
>> But, this would change the behavior on Intel systems that both have
>> CPUID5_ECX_EXTENSIONS_SUPPORTED and do not set bits in
>> MWAIT_C1_SUBSTATE_MASK.
>>
>> Is that a problem or an improvement?
> 
> At least Intel processors since Nehalem have MWAIT C1 support.
> For elder ones, need to confirm with Len.
> 
> When no bits set in MWAIT_C1_SUBSTATE_MASK, it means MWAIT C1 is not
> available for some reason, let me check if I can make this happen or
> not in real life.
> 
> thanks,
> rui
>