linux-kernel - Re: [PATCH v2 2/3] x86: Remove vendor checks from prefer_mwait_c1_over

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]

Date:   Fri, 6 May 2022 15:12:33 +0530
From:   Wyes Karny <wyes.karny@....com>
To:     Dave Hansen <dave.hansen@...el.com>, linux-kernel@...r.kernel.org
Cc:     Lewis.Carroll@....com, Mario.Limonciello@....com,
        gautham.shenoy@....com, Ananth.Narayan@....com, bharata@....com,
        len.brown@...el.com, x86@...nel.org, tglx@...utronix.de,
        mingo@...hat.com, bp@...en8.de, dave.hansen@...ux.intel.com,
        hpa@...or.com, peterz@...radead.org, chang.seok.bae@...el.com,
        keescook@...omium.org, metze@...ba.org, zhengqi.arch@...edance.com,
        mark.rutland@....com, puwen@...on.cn, rafael.j.wysocki@...el.com,
        andrew.cooper3@...rix.com, jing2.liu@...el.com,
        jmattson@...gle.com, pawan.kumar.gupta@...ux.intel.com
Subject: Re: [PATCH v2 2/3] x86: Remove vendor checks from
 prefer_mwait_c1_over_halt

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