linux-kernel - Re: [PATCH 04/11] x86/acpi: Check MWAIT feature instead of CPUID level

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]

Message-ID: <49690bcc-e2ac-43f6-95da-3711801e195a@intel.com>
Date: Thu, 21 Nov 2024 13:46:17 -0800
From: Dave Hansen <dave.hansen@...el.com>
To: Sean Christopherson <seanjc@...gle.com>,
 Dave Hansen <dave.hansen@...ux.intel.com>
Cc: linux-kernel@...r.kernel.org, x86@...nel.org, tglx@...utronix.de,
 bp@...en8.de, rafael@...nel.org, lenb@...nel.org, dave.jiang@...el.com,
 irenic.rajneesh@...il.com, david.e.box@...el.com
Subject: Re: [PATCH 04/11] x86/acpi: Check MWAIT feature instead of CPUID
 level

On 11/21/24 07:33, Sean Christopherson wrote:
> On Wed, Nov 20, 2024, Dave Hansen wrote:
>> From: Dave Hansen <dave.hansen@...ux.intel.com>
>>
>> I think this code is possibly buggy.  The CPU could have a high
>> c->cpuid_level and not support MWAIT at all.
> 
> Reading CPUID.0x5 is totally fine in that case though.  Wasteful and pointless,
> but functionally ok.  If the CPU provides non-zero values when MWAIT is unsupported,
> then that's a broken CPU.

That's a good point.

I was mostly worried about consuming *bad* data from CPUID.5, but the
leaf check takes care of that now.

>> diff -puN arch/x86/kernel/acpi/cstate.c~mwait-leaf-checks-3 arch/x86/kernel/acpi/cstate.c
>> --- a/arch/x86/kernel/acpi/cstate.c~mwait-leaf-checks-3	2024-11-20 11:44:17.225650902 -0800
>> +++ b/arch/x86/kernel/acpi/cstate.c	2024-11-20 11:44:17.225650902 -0800
>> @@ -173,7 +173,7 @@ int acpi_processor_ffh_cstate_probe(unsi
>>  	struct cpuinfo_x86 *c = &cpu_data(cpu);
>>  	long retval;
>>  
>> -	if (!cpu_cstate_entry || c->cpuid_level < CPUID_MWAIT_LEAF)
>> +	if (!cpu_cstate_entry || cpu_has(c, X86_FEATURE_MWAIT))
> 
> Someone didn't test this :-)

Guilty.

I'll just drop this.