<p style="white-space: pre-wrap; word-wrap: break-word;">Thanks for writing this up. The relation to the `SkipMpInit` wasn't<br>clear to me before (I'm not 100% sure yet, but much closer).</p><p style="white-space: pre-wrap; word-wrap: break-word;">Please make it clear if you intend to force usage of this interface<br>or if it is optional (for `SkipMpInit=1`).</p><p><a href="https://review.coreboot.org/25921">View Change</a></p><p>5 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/25921/3/Documentation/Intel/MultiProcessorInit/MultiProcessorInit.md">File Documentation/Intel/MultiProcessorInit/MultiProcessorInit.md:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/25921/3/Documentation/Intel/MultiProcessorInit/MultiProcessorInit.md@7">Patch Set #3, Line 7:</a> <code style="font-family:monospace,monospace">for Intel 9th Gen (Cannon Lake) and beyond CPUs</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This is why you are doing it, but it seems to me that the result<br>is 100% generic. Both, coreboot's MP API and EFI_MP_SERVICES_PPI,<br>are SoC and vendor independent. Hence I wouldn't expect this in<br>Documentation/Intel/ and the implementation in cpu/x86/ along-<br>side `mp_init.c`.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Maybe split this in two documents, one that states that coreboot<br>implements EFI_MP_SERVICES_PPI APIs and why (foreign code linked<br>with coreboot may rely on it) and one that explains how it's<br>working together with FSP?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/25921/3/Documentation/Intel/MultiProcessorInit/MultiProcessorInit.md@25">Patch Set #3, Line 25:</a> <code style="font-family:monospace,monospace">closed source in nature</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">It might be just me, but to me "closed source in nature" sounds like<br>"could not even be trusted if published".</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/25921/3/Documentation/Intel/MultiProcessorInit/MultiProcessorInit.md@29">Patch Set #3, Line 29:</a> <code style="font-family:monospace,monospace">get-rid of such close source</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">If I understand it correctly (please correct me if not), it only<br>reduces closed-source overhead in case of `SkipMpInit=0`, right?<br>`SkipMpInit=1` will still work and run the least closed-source code?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/25921/3/Documentation/Intel/MultiProcessorInit/MultiProcessorInit.md@35">Patch Set #3, Line 35:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">It’s also possible for coreboot to extend its own<br>support model and create a sets of APIs which later can be used by FSP to run CPU feature<br>programming using coreboot published APIs<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I don't understand. coreboot already has such an API<br>(include/cpu/x86/mp.h). If it could be used by FSP later,<br>why can't it be used now?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/25921/3/Documentation/Intel/MultiProcessorInit/MultiProcessorInit.md@68">Patch Set #3, Line 68:</a> <code style="font-family:monospace,monospace">1. coreboot was using SkipMpInit=1 which will skip entire FSP CPU feature programming.</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This is decided per board and not for coreboot in general, AFAICS.</p></li></ul></li></ul><p>To view, visit <a href="https://review.coreboot.org/25921">change 25921</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://review.coreboot.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://review.coreboot.org/25921"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: coreboot </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I5b6096ef31d8a523c00cbad39ab9d4884e735fde </div>
<div style="display:none"> Gerrit-Change-Number: 25921 </div>
<div style="display:none"> Gerrit-PatchSet: 3 </div>
<div style="display:none"> Gerrit-Owner: Subrata Banik <subrata.banik@intel.com> </div>
<div style="display:none"> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> </div>
<div style="display:none"> Gerrit-CC: Nico Huber <nico.h@gmx.de> </div>
<div style="display:none"> Gerrit-CC: Patrick Georgi <pgeorgi@google.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 01 May 2018 11:26:51 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: No </div>