Attention is currently required from: Nico Huber, Jonathan Zhang, David Hendricks, Rocky Phagura, Arthur Heymans. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/52518 )
Change subject: src/cpu/x86/smm: move console init to thread safe section ......................................................................
Patch Set 1: -Code-Review
(3 comments)
File src/cpu/x86/smm/smm_module_handler.c:
https://review.coreboot.org/c/coreboot/+/52518/comment/d738c44a_009ef57f PS1, Line 149: if (cpu > CONFIG_MAX_CPUS) {
What's the most useful thing to do in this situation? Remove the check entirely since it should be d […]
So, Rocky said that the current code causes lock-ups and there are no console messages, and I agree it is not a good thing.
I'd say we can omit printing the CPU number (or numbers) in here. Instead, have CPUs with an invalid CPU number set (e.g. write 1 to) a shared static variable. The SMM monarch (the CPU that acquired the SMI lock) would check this variable after running the chipset SMI handlers, and log an error if it has been set.
This mechanism is meant to be simple, and would report errors on a best-effort basis. Because of this, I would not use any synchronization for the shared variable. Concurrent writes can race because the written value is the same on all threads.
However, if the SMM monarch reads the shared variable before another thread has written to it, the mechanism would not report any errors, *on the current SMI handler invocation*. The shared variable would never be cleared, so subsequent SMI handler invocations would then always report an error.
https://review.coreboot.org/c/coreboot/+/52518/comment/b3e9a8e6_9d42f30b PS1, Line 149: cpu > CONFIG_MAX_CPUS The `=` in the check was lost. This should be:
if (cpu >= CONFIG_MAX_CPUS) {
https://review.coreboot.org/c/coreboot/+/52518/comment/0528f627_849e2775 PS1, Line 157: console_init();
Exactly. […]
Do you mean placing the `if (cpu >= CONFIG_MAX_CPUS)` check here? Note that only one thread executes this code.
Aside, looks like the `=` was lost in the condition.
-
Angel Pons (Code Review)