Attention is currently required from: Raul Rangel, Furquan Shaikh. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56320 )
Change subject: x86/smp/spinlock: Disable thread coop when taking spinlock ......................................................................
Patch Set 1:
(1 comment)
File src/arch/x86/include/arch/smp/spinlock.h:
https://review.coreboot.org/c/coreboot/+/56320/comment/a25c6b01_f68618a9 PS1, Line 61: lock->coop = thread_coop_enabled(); I don't think this works, although I'm not very familiar with the x86 SMP stuff. Hopefully +Furquan understands these things better.
These spinlocks are primarily for the actual SMP in ramstage, not for the thread mechanism. So you may have two actual physical CPUs spinning on this lock at the same time (while a third one is holding it), and then they'll keep overwriting each other here if you just store this value in the lock struct. If you want a per-CPU copy you'll actually have to put it in the cpu_info or thread structures.
However, I think(?) the non-boot CPUs actually can't participate in the thread mechanism anyway. I just see thread_init_non_bsp_cpu() set their cpu_info->thread pointer to NULL and I don't think it ever changes from that. So there's no real point in storing coop status for them.
Maybe the best option would be to just make thread_cooperate() and thread_prevent_coop() work like a semaphore instead of a boolean value? I could see that being useful in other cases as well. The fundamental problem here is just that you're calling code that wants to establish a critical section from code that may already be in a critical section -- that situation can happen in all sorts of places and would probably come up again if we start using this API more. If you just make them reference count how many critical sections deep they are, they can take care of this problem themselves (and the non-boot-CPUs would just ignore all those thread_prevent_coop() and thread_cooperate() calls, because their current_thread() is NULL).