Attention is currently required from: Hung-Te Lin, Jason Glenesk, Raul Rangel, Jakub Czapiga, Matt DeVillier, Fred Reitberger, Felix Held.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/66909 )
Change subject: vboot: Add VBOOT_CBFS_INTEGRATION support ......................................................................
Patch Set 15:
(2 comments)
File src/mainboard/google/asurada/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/66909/comment/6a5bbf1d_ff279249 PS14, Line 19: postcar-y += reset.c
I did not work with ARM chips, so I didn't know about that. […]
FWIW we should try migrating these kinds of things to `all-y +=` on future boards so we don't have to worry about such details.
File src/security/vboot/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/66909/comment/91957836_df96a787 PS15, Line 104: bootblock-y += secdata_mock.c So... this is actually a good indication for a problem. You should not need to be adding this here.
This is pulled in because the compiler thinks that vboot_fail_and_reboot() may need to be called on a bootblock CBFS load, and may need to save state back to the TPM. But we know that the bootblock is always in RO, so there's no point compiling that code into that stage, and we shouldn't waste space like that. Well... okay, that's not quite true -- in the case of RETURN_FROM_VERSTAGE, the bootblock may load the RW romstage. But only then. So I think we should add a `&& vboot_logic_executed()` to the check in cbfs.c to ensure we're not pulling in the whole vboot_fail_and_reboot() rat tail in stages that don't need it.
The other issue is that dropping into recovery mode because of a failed CBFS read actually never needs to write back to the TPM, but it's all bundled together in vboot_save_data() so the compiler can't tell. This is one of the unfortunate side-effects of making vboot so ubiquitous throughout coreboot rather than confining it to one stage. I think we should just add manual hints in vboot_save_data() to tell the compiler which stages may need TPM support -- i.e. just put a ``` if (!ENV_VERSTAGE && !(ENV_ROMSTAGE && CONFIG(VBOOT_EARLY_EC_SYNC) && ctx->flags & (VB2_CONTEXT_SECDATA_FIRMWARE_CHANGED | VB2_CONTEXT_SECDATA_KERNEL_CHANGED)) die("TPM writeback in " ENV_STRING "?"); ``` at the top of that function, and then those references to TPM functions should be garbage collected from all those other stages, and you shouldn't need most of the extra Makefile dependencies you're adding here.