On 25.06.2025 15:01, Andrew Cooper wrote:
> 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...
For me, the answer here is sufficient, I guess. Hence I won't insist on you
amending the description. It may help others and/or some time into the
future, though.
Jan
