Attention is currently required from: Raul Rangel, Furquan Shaikh, Tim Wawrzynczak, Kyösti Mälkki, Rob Barnes, Karthik Ramasubramanian. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59021 )
Change subject: arch/x86/smp/spinlock: Fix threading when !STAGE_HAS_SPINLOCKS ......................................................................
Patch Set 2:
(1 comment)
File src/arch/x86/include/arch/smp/spinlock.h:
https://review.coreboot.org/c/coreboot/+/59021/comment/7c0900ac_6e8a7be5 PS2, Line 50: thread_coop_disable();
I can tell you with certainty that AMD codebase has had places where __always_inline was mandatory. […]
Can you explain why this __always_inline is needed? (Git says it has been here since 2005 and was added as part of a giant patch doing loads of other things where the commit message just says "1201_ht_bus0_dev0_fidvid_core.diff https://openbios.org/roundup/linuxbios/issue41 Lord have mercy upon us." ...so that is helpful. :/ ) I would have naively seconded Raul here that it probably doesn't need to be there and can just be replaced with a normal `inline` or something. If it actually is load-bearing then there needs to be a prominent comment explaining the issue next to it.
FWIW, thread_coop_disable() is an inline no-op for !CONFIG(COOP_MULTITASKING), so this can still be fully inlined for all those boards not using that. For those that do, I think combining the two concepts makes a lot more sense than having to maintain two separate kinds of concurrency protection across coreboot. For 90% of the cases you'll always want to have both anyway, and making special cases to save one rdmsr() in the remainder doesn't seem worth it (unless you have a concrete example where this would really cause a notable issue).