On 25/06/2025 10:58 am, Jan Beulich wrote:
> On 24.06.2025 18:39, Andrew Cooper wrote:
>> Most of the patch (handling of CPUIDLE_FLAG_IBRS) is fine, but the
>> adjustements to mwait_idle() are not.
>>
>> spec_ctrl_{enter,exit}_idle() do more than just alter MSR_SPEC_CTRL.IBRS.
>> The
>> VERW and RSB stuff are **unsafe** to omit.
>>
>> The only reason this doesn't need an XSA is because no changes were made to
>> the lower level mwait_idle_with_hints(), and thus it remained properly
>> protected.
>>
>> I.e. This change only served to double the expensive operations in the case
>> it
>> was trying to optimise.
>>
>> I have an idea of how to plumb this more nicely, but it requires larger
>> changes to legacy IBRS handling to not make spec_ctrl_enter_idle() vulnerable
>> in other ways.
> What are the concerns here? As it looks skipping the MSR write would look
> to require checking some (per-CPU) conditional. Conditional branches can't
> really be of concern, or the "if (cx->ibrs_disable)" that you're now
> removing again would have been of concern, too.
The conditional branches are what set off alarm bells in the first place.
A conditional branch in enter should be ok; HLT and MWAIT should be
serialising enough.
A conditional branch in exit is not ok without extra safety measures.
I can expand on this in the commit message if you'd like. I was trying
to not be overly critical...
> Hence simply a new SCF_
> flag would look to be sufficient, for mwait_idle() to convey the necessary
> info to spec_ctrl_enter_idle()?
I've got a local patch going that route, but it needs more than just an
SCF flag. This is the "requires larger changes".
>
>> In the short term, simply take out the perf hit.
>>
>> Fixes: 08acdf9a2615 ("x86/mwait-idle: disable IBRS during long idle")
>> Signed-off-by: Andrew Cooper <andrew.coop...@citrix.com>
> Reviewed-by: Jan Beulich <jbeul...@suse.com>
>
Thanks.
~Andrew
