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

[Sean Christopherson <seanjc@google.com>]

On Wed, Nov 20, 2024, Dave Hansen wrote:
> 
> From: Dave Hansen <dave.hansen@linux.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.

> It is much more clear to
> just check for MWAIT support directly.  Also, because of the CPU level
> dependency code, any CPU that has X86_FEATURE_MWAIT also has a
> high-enough CPUID level.

No?  The MWAIT feature flag is in leaf 1.  1 < 5, and I don't see any cpuid_level
magic that would force it beyond '5'.

Why not check both?

> Check X86_FEATURE_MWAIT instead of the CPUID level.
> 
> Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
> ---
> 
>  b/arch/x86/kernel/acpi/cstate.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 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 :-)

>  		return -1;
>  
>  	if (reg->bit_offset != NATIVE_CSTATE_BEYOND_HALT)
> _