Nico Huber has posted comments on this change. ( https://review.coreboot.org/25921 )
Change subject: Documentation/Intel: Add MultiProcessorInit documentation ......................................................................
Patch Set 3:
(5 comments)
Thanks for writing this up. The relation to the `SkipMpInit` wasn't clear to me before (I'm not 100% sure yet, but much closer).
Please make it clear if you intend to force usage of this interface or if it is optional (for `SkipMpInit=1`).
https://review.coreboot.org/#/c/25921/3/Documentation/Intel/MultiProcessorIn... File Documentation/Intel/MultiProcessorInit/MultiProcessorInit.md:
https://review.coreboot.org/#/c/25921/3/Documentation/Intel/MultiProcessorIn... PS3, Line 7: for Intel 9th Gen (Cannon Lake) and beyond CPUs This is why you are doing it, but it seems to me that the result is 100% generic. Both, coreboot's MP API and EFI_MP_SERVICES_PPI, are SoC and vendor independent. Hence I wouldn't expect this in Documentation/Intel/ and the implementation in cpu/x86/ along- side `mp_init.c`.
Maybe split this in two documents, one that states that coreboot implements EFI_MP_SERVICES_PPI APIs and why (foreign code linked with coreboot may rely on it) and one that explains how it's working together with FSP?
https://review.coreboot.org/#/c/25921/3/Documentation/Intel/MultiProcessorIn... PS3, Line 25: closed source in nature It might be just me, but to me "closed source in nature" sounds like "could not even be trusted if published".
https://review.coreboot.org/#/c/25921/3/Documentation/Intel/MultiProcessorIn... PS3, Line 29: get-rid of such close source If I understand it correctly (please correct me if not), it only reduces closed-source overhead in case of `SkipMpInit=0`, right? `SkipMpInit=1` will still work and run the least closed-source code?
https://review.coreboot.org/#/c/25921/3/Documentation/Intel/MultiProcessorIn... PS3, Line 35: It’s also possible for coreboot to extend its own : support model and create a sets of APIs which later can be used by FSP to run CPU feature : programming using coreboot published APIs I don't understand. coreboot already has such an API (include/cpu/x86/mp.h). If it could be used by FSP later, why can't it be used now?
https://review.coreboot.org/#/c/25921/3/Documentation/Intel/MultiProcessorIn... PS3, Line 68: 1. coreboot was using SkipMpInit=1 which will skip entire FSP CPU feature programming. This is decided per board and not for coreboot in general, AFAICS.
-
Nico Huber (Code Review)