linux-kernel - Re: [PATCH v3 2/3] x86/smp native_play_dead: Prefer cpuidle_play_dead() over mwait_play

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: <394fa854-41a9-4ad1-880b-629108d52b41@intel.com>
Date: Fri, 8 Nov 2024 08:14:21 -0800
From: Dave Hansen <dave.hansen@...el.com>
To: Patryk Wlazlyn <patryk.wlazlyn@...ux.intel.com>, x86@...nel.org
Cc: linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
 rafael.j.wysocki@...el.com, len.brown@...el.com,
 artem.bityutskiy@...ux.intel.com, dave.hansen@...ux.intel.com
Subject: Re: [PATCH v3 2/3] x86/smp native_play_dead: Prefer
 cpuidle_play_dead() over mwait_play_dead()

On 11/8/24 04:29, Patryk Wlazlyn wrote:
> The generic implementation, based on cpuid leaf 0x5, for looking up the
> mwait hint for the deepest cstate, depends on them to be continuous in
> range [0, NUM_SUBSTATES-1]. While that is correct on most Intel x86
> platforms, it is not architectural and may not result in reaching the
> most optimized idle state on some of them.
> 
> Prefer cpuidle_play_dead() over the generic mwait_play_dead() loop and
> fallback to the later in case of missing enter_dead() handler.

I don't think the bug has anything to do with this patch, really.
There's no need to rehash it here.

The issue here is that the only way to call mwait today is via
mwait_play_dead() directly, using its internally-calculated hint.

What you want is for a cpuidle-driver-calculated hint to be used.  So,
you're using the new hint function via the cpuidle driver.  But you just
need the cpuidle driver to be called first, not the old
mwait_play_dead()-calculated hint.  The new code will still do mwait,
just via a different path and with a different hint.

The thing this doesn't mention is what the impact on everyone else is.
I _think_ the ACPI cpuidle driver is the only worry.  Today, if there's
a system that supports mwait and ACPI cpuidle, it'll use mwait.  After
this patch, it'll use ACPI cpuidle.

The changelog doesn't mention that behavior change or why that's OK.
Also, looking at this:

> -	mwait_play_dead();
>  	if (cpuidle_play_dead())
> -		hlt_play_dead();
> +		mwait_play_dead();
> +	hlt_play_dead();

None of those return on success, right?

Is there any reason this couldn't just be:

	/* The first successful play_dead() will not return: */
	cpuidle_play_dead();
	mwait_play_dead();
	hlt_play_dead();

That has the added bonus of not needing to return anything from
mwait_play_dead() and the resulting churn from the last patch.