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

Peter Zijlstra
2022-05-25T16:56:59Z

On Mon, May 23, 2022 at 10:25:50PM +0530, Wyes Karny wrote:

> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
> index 4e0178b066c5..7bf4d73c9522 100644
> --- a/arch/x86/kernel/process.c
> +++ b/arch/x86/kernel/process.c
> @@ -813,28 +813,43 @@ static void amd_e400_idle(void)
> }
>
> /*
> - * Intel Core2 and older machines prefer MWAIT over HALT for C1.
> - * We can't rely on cpuidle installing MWAIT, because it will not load
> - * on systems that support only C1 -- so the boot default must be MWAIT.
> + * Prefer MWAIT over HALT if MWAIT is supported, MWAIT_CPUID leaf
> + * exists and whenever MONITOR/MWAIT extensions are present there is at
> + * least one C1 substate.
> *
> - * Some AMD machines are the opposite, they depend on using HALT.
> - *
> - * So for default C1, which is used during boot until cpuidle loads,
> - * use MWAIT-C1 on Intel HW that has it, else use HALT.
> + * Do not prefer MWAIT if MONITOR instruction has a bug or idle=nomwait
> + * is passed to kernel commandline parameter.
> */
> 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;
>
> - 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;
>
> - 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 at least one
> + * MWAIT C1 substate present.
> + */
> + return (edx & MWAIT_C1_SUBSTATE_MASK);
> }

Seems reasonable enough to me,

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>