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

Sean Christopherson
2024-11-21T15:33:53Z

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)
> _