Hi,
On Fri, Dec 13, 2024 at 6:25 PM Julius Werner jwerner@chromium.org wrote:
I feel like this patch is maybe taking a bit of a wrong angle at achieving what you're trying to do. The code seems to be structured in a way that it has separate functions to test for each of the possible mitigation options one at a time, and pushing the default case into spectre_bhb_loop_affected() feels like a bit of a layering violation. I think it would work the way you wrote it, but it depends heavily on the order functions are called in is_spectre_bhb_affected(), which seems counter-intuitive with the way the functions seem to be designed as independent checks.
What do you think about an approach like this instead:
- Refactor max_bhb_k in spectre_bhb_loop_affected() to be a global
instead, which starts out as zero, is updated by spectre_bhb_loop_affected(), and is directly read by spectre_bhb_patch_loop_iter() (could probably remove the `scope` argument from spectre_bhb_loop_affected() then).
Refactoring "max_bhb_k" would be a general cleanup and not related to anything else here, right?
...but the function is_spectre_bhb_affected() is called from "cpu_errata.c" and has a scope. Can we guarantee that it's always "SCOPE_LOCAL_CPU"? I tried reading through the code and it's _probably_ SCOPE_LOCAL_CPU most of the time, but it doesn't seem worth it to add an assumption here for a small cleanup.
I'm not going to do this, though I will move "max_bhb_k" to be a global for the suggestion below.
- Add a function is_spectre_bhb_safe() that just checks if the MIDR is
in the new list you're adding
- Add an `if (is_spectre_bhb_safe()) return false;` to the top of
is_spectre_bhb_affected
That seems reasonable to do and lets me get rid of the "safe" checks from is_spectre_bhb_fw_affected() and spectre_bhb_loop_affected(), so it sounds good.
- Change the `return false` into `return true` at the end of
is_spectre_bhb_affected (in fact, you can probably take out some of the other calls that result in returning true as well then)
I'm not sure you can take out the other calls. Specifically, both spectre_bhb_loop_affected() and is_spectre_bhb_fw_affected() _need_ to be called for each CPU so that they update static globals, right? Maybe we could get rid of the call to supports_clearbhb(), but that _would_ change things in ways that are not obvious. Specifically I could believe that someone could have backported "clear BHB" to their core but their core is otherwise listed as "loop affected". That would affect "max_bhb_k". Maybe (?) it doesn't matter in this case, but I'd rather not mess with it unless someone really wants me to and is sure it's safe.
- In spectre_bhb_enable_mitigations(), at the end of the long if-else
chain, put a last else block that prints your WARN_ONCE(), sets the max_bhb_k global to 32, and then does the same stuff that the `if (spectre_bhb_loop_affected())` block would have installed (maybe factoring that out into a helper function called from both cases).
...or just reorder it so that the spectre_bhb_loop_affected() code is last and it can be the "else". Then I can WARN_ONCE() if spectre_bhb_loop_affected(). ...or I could just do the WARN_ONCE() when I get to the end of is_spectre_bhb_affected() and set "max_bhb_k" to 32 there. I'd actually rather do that so that "max_bhb_k" is consistently set after is_spectre_bhb_affected() is called on all cores regardless of whether or not some cores are unknown.