Hi, On Fri, Dec 13, 2024 at 6:25 PM Julius Werner <jwerner@xxxxxxxxxxxx> 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.
