<p><a href="https://review.coreboot.org/23756">View Change</a></p><p>9 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/23756/14/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/23756/14/src/drivers/i2c/tpm/Makefile.inc@5">Patch Set #14, Line 5:</a> <code style="font-family:monospace,monospace">postcar-$(CONFIG_DRIVER_TIS_DEFAULT) += tis.c</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">you shouldn't replace bootblock with postcar. you should add postcar.</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/23756/14/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/23756/14/src/drivers/spi/tpm/Makefile.inc@4">Patch Set #14, Line 4:</a> <code style="font-family:monospace,monospace">postcar-$(CONFIG_SPI_TPM) += tis.c tpm.c</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">need bootblock here too</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/23756/14/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/23756/14/src/mainboard/google/poppy/Kconfig@123">Patch Set #14, Line 123:</a> <code style="font-family:monospace,monospace">   select VARIANT_HAS_I2C_TPM</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">What's this one for? What's the interaction when VBOOT_MOCK_SECDATA is enabled?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Only enable the tpm if antirollback protection isn't disabled. Does not make sense because can be set through defconfig</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/23756/14/src/security/vboot/Makefile.inc">File src/security/vboot/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/23756/14/src/security/vboot/Makefile.inc@75">Patch Set #14, Line 75:</a> <code style="font-family:monospace,monospace">verstage-y += secdata_measurements.c</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This should be based on a Kconfig.</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/23756/14/src/security/vboot/secdata_measurements.c">File src/security/vboot/secdata_measurements.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/23756/14/src/security/vboot/secdata_measurements.c@21">Patch Set #14, Line 21:</a> <code style="font-family:monospace,monospace">uint32_t vboot_measure_self(void)</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">'self' is not really self. It's only bootblock. Sounds like you want vboot_measure_bootblock().</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">vboot_measure_crtm() -> core root of trust for measurement</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://review.coreboot.org/#/c/23756/14/src/security/vboot/secdata_measurements.c@24">Patch Set #14, Line 24:</a> <code style="font-family:monospace,monospace">PROG_UNKNOWN</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Why not have PROG_BOOTBLOCK? You could just call measured_prog_run() in that case instead of hardcod […]</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/23756/14/src/security/vboot/secdata_measurements.c@30">Patch Set #14, Line 30:</a> <code style="font-family:monospace,monospace">     if (cbfs_boot_locate(&file1, prog_name(&bootblock), NULL))</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">So this is assuming SEPARATE_VERSTAGE it seems?</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/23756/14/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/23756/14/src/security/vboot/secdata_tpm.c@a448">Patch Set #14, Line 448:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Where is this being done now?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">vboot_logic at the beginning of verstage_main()</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://review.coreboot.org/#/c/23756/14/src/security/vboot/vboot_logic.c">File src/security/vboot/vboot_logic.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/23756/14/src/security/vboot/vboot_logic.c@315">Patch Set #14, Line 315:</a> <code style="font-family:monospace,monospace">#if IS_ENABLED(CONFIG_VBOOT_MODE_VERIFIED_AND_MEASURED_BOOT)</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">no #ifs</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li></ul><p>To view, visit <a href="https://review.coreboot.org/23756">change 23756</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/23756"/><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: I43d233d5a8766af2dd7f07cc0b64293a80d5d7d2 </div>
<div style="display:none"> Gerrit-Change-Number: 23756 </div>
<div style="display:none"> Gerrit-PatchSet: 14 </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: 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-Comment-Date: Thu, 15 Feb 2018 01:15:57 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: No </div>