Attention is currently required from: Raul Rangel, Julius Werner, Rob Barnes, Karthik Ramasubramanian. Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59320 )
Change subject: lib: Add a mutex ......................................................................
Patch Set 8:
(3 comments)
File src/include/mutex.h:
https://review.coreboot.org/c/coreboot/+/59320/comment/36123955_1fd8f3c8 PS6, Line 32: }
Are you saying we should make ENV_STATE_SUPPORTS_SMP evaluate to 1 in x86 romstage, Kyösti? But why? […]
AMD x86 romstages run APs. There used to be poor spinlock-like support for amdfam10 as it really needed it. And there is paid efforts to bring amdfam (kgpe-d16) back upstrem. With AGESA romstage SMP is serialized by other means, BSP waits for APs to execute one by one (and possibly implement exclusion within AGESA).
Say you do some PCI configuration register read-modify-write ops. Would you put a spinlock or mutex around it? I would use a spinlock, Thus I think it makes sense to start by getting rid of the .data requirement of existing <smp/spinlock.h>, use .bss and compiller builtin __atomic_load() etc.
You could start the entire patchtrain with that, and maybe the change from spinlocks to mutexes becomes easier to grasp.
If you have SMP=y is it really worth the trouble to drop the atomicity for stages where you don't really need it? I am asking if it is really worth having _mutex_{lock/unlock]_coop() implemented separately. A better optimisation might be to have spinlock implememntations, here _mutex_(un)lock_{coop,smp} always inlined.
File src/lib/mutex.c:
https://review.coreboot.org/c/coreboot/+/59320/comment/f43cc8f3_363b0c7f PS8, Line 12: return; This could go into <smp/spinlock.h> ?
static __always_inline int try_get_spin_lock(x) { return __atomic_test_and_set(xxxx) == 0; }
https://review.coreboot.org/c/coreboot/+/59320/comment/2e744d93_37c00e62 PS8, Line 27: } while (__atomic_load_1(&mutex->locked, __ATOMIC_RELAXED)); static __always_inline int spin_is_locked(x) { return __atomic_load_1(xxxx); }