Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46821 )
Change subject: cpu/x86/mpinit/sipi_vector: Move code to C ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/c/coreboot/+/46821/2/src/cpu/x86/sipi_vector.S File src/cpu/x86/sipi_vector.S:
https://review.coreboot.org/c/coreboot/+/46821/2/src/cpu/x86/sipi_vector.S@8... PS2, Line 86: 1: FWIW, we should move '1:' label above 'movl ap_count, %eax' so it reloads the real value in memory. For another patch, though. We could probably reduce the number of instructions to just use eax and stop using ecx.
https://review.coreboot.org/c/coreboot/+/46821/2/src/cpu/x86/sipi_vector_c_h... File src/cpu/x86/sipi_vector_c_handler.c:
https://review.coreboot.org/c/coreboot/+/46821/2/src/cpu/x86/sipi_vector_c_h... PS2, Line 27: /* Determine if parallel microcode loading is allowed. */ We should have a lot more commentary on this code as to why we're not using spinlocks and how we're inherently relying on sipi_params object being in uncacheable memory. However, spinlocks could be used if microcode_lock was transformed into a spinlock type.
https://review.coreboot.org/c/coreboot/+/46821/2/src/cpu/x86/sipi_vector_c_h... PS2, Line 35: : "=m"(params->microcode_lock) This should be ¶ms->microcode_lock, I think. What's the disassembly look like?
https://review.coreboot.org/c/coreboot/+/46821/2/src/cpu/x86/sipi_vector_c_h... PS2, Line 104: struct sipi_params * const... but it appears we're wanting to manipulate microcode_lock object itself. Everything but sipi_update_microcode() should be using const params.