linux-kernel - Re: [PATCH v2 2/3] x86/smp: Allow forcing the mwait hint for play dead loop

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: <63f7dbbc-831f-4c16-9f11-5cf2c36996d0@intel.com>
Date: Wed, 30 Oct 2024 15:55:12 -0700
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 v2 2/3] x86/smp: Allow forcing the mwait hint for play
 dead loop

On 10/30/24 06:33, Patryk Wlazlyn wrote:
>>> +void smp_set_mwait_play_dead_hint(unsigned int hint)
>>> +{
>>> +    WRITE_ONCE(play_dead_mwait_hint, hint);
>>> +}
>>
>> This all feels a bit hacky and unstructured to me.
>>
>> Could we at least set up a few rules here?  Like, say what the hints
>> are, what values can they have?  Where do they come from?  Can this get
>> called more than once?  Does it _need_ to be set?  What's the behavior
>> when it is not set?  Who is responsible for calling this?
> 
> The other idea is to first check if currently loaded idle driver provides
> enter_dead() callback first and leave the current, deepest mwait hint
> computation code as a fallback.
> 
> Does that sound less hacky?

Yes.

> Unfortunately, it comes with a little problem. In case of kexec, we need to
> have a way to exit from the mwait loop and enter hlt to prevent offlined CPU
> from crashing when the old memory is being overwritten.

Why is this a problem?  Like I mentioning to Rafael, just *call*
mwait_play_dead() from the idle driver.

mwait_play_dead() could probably also use some refactoring because it
has 3 pieces:

	1. Should the code run at all or defer to another play dead
	   implementation?
	2. Calculating the hint (obviously not needed in your new case)
	3. Actually running mwait (including the kexec hack)

The "should the code run?" bit is superfluous but harmless if called
from the idle driver.  Ditto on the hint calculation, but you already
factored it out.  That leaves the "actually run mwait" bit which you
100% need.

> I think, we can solve it by bringing the CPU back online before we proceed
> with kexec, but I would appreciate some feedback from someone who is more
> familiar with kexec, before merging that.
> 
> We may also signal that by touching the resched flag on which enter_dead()
> code will monitor in case of mwait and enter hlt right after, but that's a
> bit hackier IMO.

That route is also fine with me, but I'm not sure it's necessary.

>> What good does the smp_ prefix do?  I don't think _callers_ care whether
>> this is getting optimized out or not.
> 
> The prefix makes it a little bit cleaner by not exporting new global symbol
> with "set_mwait_play_dead_hint" name.

I'm not following.