Patrick Rudolph 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 3:
(5 comments)
https://review.coreboot.org/c/coreboot/+/46821/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46821/1//COMMIT_MSG@8 PS1, Line 8:
What is the problem or motivation for the change?
Done
https://review.coreboot.org/c/coreboot/+/46821/1/src/cpu/x86/sipi_vector_c_h... File src/cpu/x86/sipi_vector_c_handler.c:
https://review.coreboot.org/c/coreboot/+/46821/1/src/cpu/x86/sipi_vector_c_h... PS1, Line 87: /* Enable sse instructions. */
Remove the (unneeded) dot/period at the end – also for consistency with the other comments.
Done
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 […]
yes, the plan is to clean the code. I just reused the existing comments from assebmly when converting it to C.
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. […]
params->microcode_lock is correct. it generates the following assembly code: 2e0: f0 0f ba 68 14 00 lock btsl $0x0,0x14(%eax) 2e6: 72 f8 jb 0x2e0
where eax is params
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. […]
Done