<p><a href="https://review.coreboot.org/24903">View Change</a></p><p>16 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/24903/18/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/18/src/cpu/intel/haswell/romstage.c@248">Patch Set #18, 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;">Actually, how are all these tpm_setup()s in chipset code going to interact with vboot? CONFIG_VBOOT  […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I fixed this issue in a later commit by introducing a tpm ramstage driver. People told me to split it up</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/24903/18/src/drivers/i2c/tpm/Makefile.inc">File src/drivers/i2c/tpm/Makefile.inc:</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/18/src/drivers/i2c/tpm/Makefile.inc@1">Patch Set #18, Line 1:</a> <code style="font-family:monospace,monospace">ifneq ($(I2C_TPM),)</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Shouldn't this just check for I2C_TPM? (Not sure why I didn't say that before... […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/24903/18/src/drivers/pc80/tpm/Makefile.inc">File src/drivers/pc80/tpm/Makefile.inc:</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/18/src/drivers/pc80/tpm/Makefile.inc@1">Patch Set #18, Line 1:</a> <code style="font-family:monospace,monospace">ifneq ($(LPC_TPM),)</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Same here. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/24903/18/src/drivers/spi/tpm/Makefile.inc">File src/drivers/spi/tpm/Makefile.inc:</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/18/src/drivers/spi/tpm/Makefile.inc@1">Patch Set #18, Line 1:</a> <code style="font-family:monospace,monospace">ifneq ($(SPI_TPM),)</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Same here</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/24903/18/src/drivers/spi/tpm/Makefile.inc@7">Patch Set #18, Line 7:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">...also, I'm not sure where this came from, but this could be cleaned up while you're here.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/24903/18/src/mainboard/gigabyte/ga-b75m-d3h/Kconfig">File src/mainboard/gigabyte/ga-b75m-d3h/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/18/src/mainboard/gigabyte/ga-b75m-d3h/Kconfig@20">Patch Set #18, Line 20:</a> <code style="font-family:monospace,monospace">        select MAINBOARD_HAS_LPC_TPM</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Don't you need MAINBOARD_HAS_TPM1 if this board has TPM1. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">No it's like before. You can select that via kconfig</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/24903/18/src/mainboard/google/kahlee/Kconfig">File src/mainboard/google/kahlee/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/18/src/mainboard/google/kahlee/Kconfig@128">Patch Set #18, Line 128:</a> <code style="font-family:monospace,monospace">config KAHLEE_TPM</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I don't think this is the best way to deal with this, with two separate options. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/24903/18/src/mainboard/google/oak/Kconfig">File src/mainboard/google/oak/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/18/src/mainboard/google/oak/Kconfig@26">Patch Set #18, Line 26:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This is not enough. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/24903/18/src/mainboard/google/octopus/Kconfig">File src/mainboard/google/octopus/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/18/src/mainboard/google/octopus/Kconfig@19">Patch Set #18, Line 19:</a> <code style="font-family:monospace,monospace">   select MAINBOARD_HAS_SPI_TPM_CR50</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Also needs MAINBOARD_HAS_TPM2, right?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/24903/18/src/mainboard/google/poppy/Kconfig">File src/mainboard/google/poppy/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/18/src/mainboard/google/poppy/Kconfig@141">Patch Set #18, Line 141:</a> <code style="font-family:monospace,monospace">     select EXCLUDE_NATIVE_SD_INTERFACE</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This could just go to the BASEBOARD option, I think.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/24903/18/src/mainboard/google/stout/Kconfig">File src/mainboard/google/stout/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/18/src/mainboard/google/stout/Kconfig@17">Patch Set #18, Line 17:</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;">You're still missing a lot of Google boards in this patch. Daisy, Storm, Veyron, etc... […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/24903/18/src/mainboard/intel/glkrvp/Kconfig">File src/mainboard/intel/glkrvp/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/18/src/mainboard/intel/glkrvp/Kconfig@12">Patch Set #18, Line 12:</a> <code style="font-family:monospace,monospace">   select MAINBOARD_HAS_LPC_TPM</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Wouldn't you need MAINBOARD_HAS_TPM2 here though? You still need to select the used version of the T […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">see comment before</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/24903/18/src/mainboard/intel/kblrvp/Kconfig">File src/mainboard/intel/kblrvp/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/18/src/mainboard/intel/kblrvp/Kconfig@17">Patch Set #18, Line 17:</a> <code style="font-family:monospace,monospace">     select MAINBOARD_HAS_LPC_TPM</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I think this should select both MAINBOARD_HAS_TPM1 and MAINBOARD_HAS_TPM2, so that the user can sele […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">already done by not setting it.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/24903/18/src/security/tpm/tss/vendor/cr50/tss_structures.h">File src/security/tpm/tss/vendor/cr50/tss_structures.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/18/src/security/tpm/tss/vendor/cr50/tss_structures.h@23">Patch Set #18, Line 23:</a> <code style="font-family:monospace,monospace">    knowledge of all commands. */</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Is this a comment you added or did you copy it from somewhere? Because I don't really understand wha […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">It's google stuff. Ask Vadim..</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/24903/18/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/18/src/security/vboot/Kconfig@20">Patch Set #18, 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;">This is still not right...</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">To be honest if you don't have a tpm you can use a verified boot. Maybe it's not secure against replay attacks but that's how it is.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/24903/18/src/security/vboot/secdata_mock.c">File src/security/vboot/secdata_mock.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/18/src/security/vboot/secdata_mock.c@43">Patch Set #18, Line 43:</a> <code style="font-family:monospace,monospace">       return VB2_SUCCESS;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Still disagreeing with the return code change.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Let's ask someone else</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: 19 </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: 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: 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: Sat, 19 May 2018 15:07:33 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: No </div>