<p><a href="https://review.coreboot.org/24903">View Change</a></p><p>11 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/24903/33/src/cpu/intel/haswell/romstage.c">File src/cpu/intel/haswell/romstage.c:</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/24903/33/src/cpu/intel/haswell/romstage.c@248">Patch Set #33, Line 248:</a> <code style="font-family:monospace,monospace">   if (IS_ENABLED(CONFIG_TPM1) || IS_ENABLED(CONFIG_TPM2))</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Follow up: add helper tpm_enabled_in_build() which encapsulates this?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I wrote a generic driver which runs in ramstage. So it's already done in a follow up. :)</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/24903/33/src/mainboard/google/auron/Kconfig">File src/mainboard/google/auron/Kconfig:</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/24903/33/src/mainboard/google/auron/Kconfig@13">Patch Set #33, Line 13:</a> <code style="font-family:monospace,monospace"> select MAINBOARD_HAS_TPM1</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I'm a little confused by this. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Yeah that's normal. If you don't select it you will get a menu in kconfig for selecting the tpm version. This is needed for desktop and server boards having a TPM header</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/24903/33/src/mainboard/google/cheza/Kconfig">File src/mainboard/google/cheza/Kconfig:</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/24903/33/src/mainboard/google/cheza/Kconfig@15">Patch Set #33, Line 15:</a> <code style="font-family:monospace,monospace">       select MAINBOARD_HAS_TPM1</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Where'd you come up with this one? It's actually using cr50 over spi. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Okay will fix it.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/24903/33/src/mainboard/google/eve/Kconfig">File src/mainboard/google/eve/Kconfig:</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/24903/33/src/mainboard/google/eve/Kconfig@19">Patch Set #33, Line 19:</a> <code style="font-family:monospace,monospace">  select MAINBOARD_HAS_I2C_TPM_CR50</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I think TPM_CR50 should auto select HAS_TPM2... […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">CR50 is just an addition and get in through MAINBOARD_HAS_I2C_TPM_CR50. We could also a MAINBOARD_HAS_CR50</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/24903/33/src/security/tpm/tspi.h">File src/security/tpm/tspi.h:</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/24903/33/src/security/tpm/tspi.h@20">Patch Set #33, Line 20:</a> <code style="font-family:monospace,monospace">#include <security/tpm/tss.h></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">We should document the return values here so it's understood what the meaning of the values are.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Okay got it. Will also add the function parms as well</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/24903/33/src/security/tpm/tss.h">File src/security/tpm/tss.h:</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/24903/33/src/security/tpm/tss.h@57">Patch Set #33, Line 57:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Not necessarily true any longer as one can specify the policy? Or is the nv_policy different than th […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Yep good catch will fix it</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/24903/33/src/security/tpm/tss.h@172">Patch Set #33, Line 172:</a> <code style="font-family:monospace,monospace"> */</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Still need to get through the rest of the patch, but why does this declaration need to be here if it […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">It's used if you have a tpm variant based on TPM2 by googles CR50 So let's just rename the comment</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/24903/33/src/security/tpm/tss/tcg-2.0/tss_marshaling.h">File src/security/tpm/tss/tcg-2.0/tss_marshaling.h:</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/24903/33/src/security/tpm/tss/tcg-2.0/tss_marshaling.h@11">Patch Set #33, Line 11:</a> <code style="font-family:monospace,monospace">#include <security/tpm/tss/vendor/cr50/cr50.h></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">why is this header needed in here?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Okay I will fix it</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/24903/33/src/security/vboot/Kconfig">File src/security/vboot/Kconfig:</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/24903/33/src/security/vboot/Kconfig@20">Patch Set #33, Line 20:</a> <code style="font-family:monospace,monospace">  select VBOOT_MOCK_SECDATA if !TPM1 && !TPM2</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I was hoping we could get away with having this override everything.  But it is what it is. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I did that in order to support verified boot without TPM support even if it is more insecure. There is no other solution</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/24903/33/src/security/vboot/secdata_tpm.c">File src/security/vboot/secdata_tpm.c:</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/24903/33/src/security/vboot/secdata_tpm.c@166">Patch Set #33, Line 166:</a> <code style="font-family:monospace,monospace">const TPMA_NV ro_space_attributes = {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Were these extern'd in a header somewhere? If no one is using these outside of this compilation unit […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">gotcha</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/24903/33/src/soc/intel/apollolake/Kconfig">File src/soc/intel/apollolake/Kconfig:</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/24903/33/src/soc/intel/apollolake/Kconfig@126">Patch Set #33, Line 126:</a> <code style="font-family:monospace,monospace">     default n</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">why get rid of the auto-selection here?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">maybe depends on MAINBOARD_HAS_LPC_TPM makes more sense. Will fix it</p></li></ul></li></ul><p>To view, visit <a href="https://review.coreboot.org/24903">change 24903</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/24903"/><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: I7ec277e82a3c20c62a0548a1a2b013e6ce8f5b3f </div>
<div style="display:none"> Gerrit-Change-Number: 24903 </div>
<div style="display:none"> Gerrit-PatchSet: 36 </div>
<div style="display:none"> Gerrit-Owner: Philipp Deppenwiese <zaolin.daisuki@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> </div>
<div style="display:none"> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> </div>
<div style="display:none"> Gerrit-Reviewer: Furquan Shaikh <furquan@google.com> </div>
<div style="display:none"> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: Martin Roth <martinroth@google.com> </div>
<div style="display:none"> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> </div>
<div style="display:none"> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph@9elements.com> </div>
<div style="display:none"> Gerrit-Reviewer: Paul Menzel <paulepanter@users.sourceforge.net> </div>
<div style="display:none"> Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: Ronald G. Minnich <rminnich@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer@coreboot.org> </div>
<div style="display:none"> Gerrit-Reviewer: Trammell Hudson <hudson@trmm.net> </div>
<div style="display:none"> Gerrit-Reviewer: Vadim Bendebury <vbendeb@chromium.org> </div>
<div style="display:none"> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> </div>
<div style="display:none"> Gerrit-CC: Julius Werner <jwerner@chromium.org> </div>
<div style="display:none"> Gerrit-CC: Patrick Rudolph <siro@das-labor.org> </div>
<div style="display:none"> Gerrit-CC: Werner Zeh <werner.zeh@siemens.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Fri, 01 Jun 2018 14:56:41 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: No </div>