Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38546 )
Change subject: cpu/x86: Make MP init timeout configurable ......................................................................
Patch Set 9:
(4 comments)
https://review.coreboot.org/c/coreboot/+/38546/9//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38546/9//COMMIT_MSG@15 PS9, Line 15: needs to be heuristic
I must miss something here. On a system with only few threads, […]
During the MP init (flight plan) process, the MSRs of each cpu (thread) need to be set up. When the core design gets more complex and when new features are added, it takes longer time. Moreover, the number of cpus increase very fast in recent years. On 72 cpus Tiogapass I am working on, most of the time 1s works well, but occasionally it is not enough (I do not have statistics as this is not current focus). If you add a single line of debug print in this process, almost every reboot times out. To guarantee reboot stability, a larger timeout is needed. Good question that why not always use a big timeout? For smaller system like notebook, 1 second is already a big number. I work on server instead of smaller system, so I cannot speak for them. I believe their automation system runs thousands of reboot tests in one hour, hence the timeout value is not desired to be too high.
https://review.coreboot.org/c/coreboot/+/38546/9/src/cpu/x86/mp_init.c File src/cpu/x86/mp_init.c:
https://review.coreboot.org/c/coreboot/+/38546/9/src/cpu/x86/mp_init.c@152 PS9, Line 152: static int max(int x, int y)
Can you #include <stdlib. […]
stdlib.h does not have support for max. I also tried.
https://review.coreboot.org/c/coreboot/+/38546/9/src/cpu/x86/mp_init.c@528 PS9, Line 528: configurable value
It's no longer configurable
Done
https://review.coreboot.org/c/coreboot/+/38546/9/src/cpu/x86/mp_init.c@531 PS9, Line 531: It is set to at least 1 second, while each AP adds 0.1 second.
The mp_params->num_cpus includes the NBSP. […]
Done