Attention is currently required from: Arthur Heymans, Subrata Banik, Kane Chen, Tim Wawrzynczak. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63566 )
Change subject: cpu/x86/mp_init.c: Add wait_finished_mp_run_on_all_cpus ......................................................................
Patch Set 5:
(3 comments)
File src/cpu/x86/mp_init.c:
https://review.coreboot.org/c/coreboot/+/63566/comment/d2ecce6b_c465b4cb PS5, Line 932: if (atomic_read(&ap_busy[i]) == -1) : cpus_finish++;
Instead of using 0/1 for busy/not busy, this patchset use 1 and -1 to indicate busy/not busy because of the below reason.
size of ap_busy array is CONFIG_MAX_CPUS however the actual running cpu could be less than CONFIG_MAX_CPUS That means when all AP takes the jobs, some elements of ap_busy are always 0 since actual running cpu could be less than CONFIG_MAX_CPUS ex: Assume we have 4 cpus, and CONFIG_MAX_CPUS = 8 when APs take jobs, ap_busy is like [1,1,1,1,0,0,0,0] when APs finishes all the task, ap_busy is like [0,0,0,0,0,0,0,0] the last four elements is always 0 since the dut doesn't have that many CPUs.
in this case cpus_finish will be 8 and never equal to global_num_aps (4 in this case) This is why i can't simply use 0/1 to judge it's busy/not busy.
hope this explains.
This has the problem that you needs to initialize all the ap_busy variables before you can successfully use the ap_busy variables in wait_ap_finish. Can you just initialize them to not busy after the MP init. If a CPU does not exist the busy bit will never be set on those. Also best to use an enum over raw numbers.
https://review.coreboot.org/c/coreboot/+/63566/comment/56ab2890_d55c5863 PS5, Line 977: atomic_set(&ap_busy[cur_cpu], 1); Probably best to add a comment that this needs to happen before storing NULL to the callback as the busy bit is checked later by the BSP (to avoid race conditions).
https://review.coreboot.org/c/coreboot/+/63566/comment/cf6e5515_cf15197c PS5, Line 985: goto finish; Reverse the if condition and avoid goto.
-
Arthur Heymans (Code Review)