On Wed, Feb 22, 2023 at 09:16:21AM -0800, KP Singh wrote:
Thanks for iterating. I think your commit description and rewrite omits a few key subtleties which I have tried to reinforce in both the commit log and the comments.
Q: What does STIBP have to do with IBRS? A: Setting the IBRS bit implicitly enables STIBP / some form of cross thread protection.
That belongs in the docs, if you want to explain this properly.
Q: Why does it work with eIBRS? A: Because we set the IBRS bit once and leave it set when using eIBRS
Also docs.
I think this subtlety should be reinforced in the commit description and code comments so that we don't get it wrong again. Your commit does answer this one (thanks!)
Commit messages are fine when explaining *why* a change is being done. What is even finer is when you put a lenghtier explanation in our documentation so that people can actually find it. Finding text in commit messages is harder...
Q: Why does it not work with the way the kernel currently implements legacy IBRS? A: Because the kernel clears the bit on returning to user space.
From the commit message:
However, on return to userspace, the IBRS bit is cleared for performance reasons. That leaves userspace threads vulnerable to cross-thread predictions influence against which STIBP protects.
The reason why I refactored this into a separate helper was to document the subtleties I mentioned above and anchor them to one place as the function is used in 2 places. But this is a maintainer's choice, so it's your call :)
The less code gets added in that thing, the better. Not yet another helper pls.
I do agree with Pawan that it's worth adding a pr_info about what the kernel is doing about STIBP.
STIBP status gets dumped through stibp_state().