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(). -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette