[coreboot-gerrit] Change in coreboot[master]: Documentation/Intel: Add MultiProcessorInit documentation

[Nico Huber (Code Review)]

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/MultiProcessorInit/MultiProcessorInit.md
File Documentation/Intel/MultiProcessorInit/MultiProcessorInit.md:

https://review.coreboot.org/#/c/25921/3/Documentation/Intel/MultiProcessorInit/MultiProcessorInit.md@7
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/MultiProcessorInit/MultiProcessorInit.md@25
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/MultiProcessorInit/MultiProcessorInit.md@29
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/MultiProcessorInit/MultiProcessorInit.md@35
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/MultiProcessorInit/MultiProcessorInit.md@68
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.



-- 
To view, visit https://review.coreboot.org/25921
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5b6096ef31d8a523c00cbad39ab9d4884e735fde
Gerrit-Change-Number: 25921
Gerrit-PatchSet: 3
Gerrit-Owner: Subrata Banik <subrata.banik at intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply at coreboot.org>
Gerrit-CC: Nico Huber <nico.h at gmx.de>
Gerrit-CC: Patrick Georgi <pgeorgi at google.com>
Gerrit-Comment-Date: Tue, 01 May 2018 11:26:51 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.coreboot.org/pipermail/coreboot-gerrit/attachments/20180501/58cad8dc/attachment.html>