Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31329 )
Change subject: vboot: copy data structures to cbmem for downstream use ......................................................................
Patch Set 1:
(11 comments)
Can you try to find a STARTS_IN_ROMSTAGE board to test this on to make sure you're not breaking that path? I'm not sure if any of them still works... I used to keep testing stuff on Falco for a while, so if you can find one of those (or other Slippy variant like Peppy, Leon, Wolf) it might work. Otherwise, it might also work to just hack up the Kconfig of a newer board from STARTS_IN_BOOTBLOCK to STARTS_IN_ROMSTAGE.
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@a68 PS1, Line 68: If you eliminate this CBMEM ID please remove it from <commonlib/cbmem_id.h>, or at least mark it as deprecated.
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@a151 PS1, Line 151: There's another call to this function in vboot_loader.c that you also need to remove.
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@43 PS1, Line 43: static struct vb2_working_data *vb2_wd = NULL; If this is just supposed to be a cache for vboot_get_working_data() and not supposed to be accessed directly by anything else, better make it a static local.
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@48 PS1, Line 48: #if ENV_POSTCAR || ENV_RAMSTAGE Please never use preprocessor #if where a C if () would also work. Also, this whole thing should probably be written like this instead:
if (vb2_wd) return vb2_wd;
if (cbmem_possibly_online()) vb2_wd = cbmem_find(CBMEM_ID_VBOOT_WORKBUF);
if (!vb2_wd) vb2_wb = (...)_vboot2_work;
return vb2_wb;
(That way, it also does the right thing in early romstage before CBMEM has been initialized, which may be relevant if CBFS accesses happen at that time and run the vboot locator.)
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@50 PS1, Line 50: vb2_wd = cbmem_find(CBMEM_ID_VBOOT_WORKBUF); Can we add another option VBOOT_MIGRATE_WORKING_DATA (which would be default y if ARCH_X86, otherwise default n) to control the CBMEM copying? SRAM architectures *usually* never lose access to their SRAM, so for those it should be okay to just keep the working data where it is and pass a pointer to there to depthcharge.
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@62 PS1, Line 62: /* _vboot2_work_size is accessible until end of romstage */ How does this work for the STARTS_IN_ROMSTAGE case? I don't think the _vboot2_work_size label is available there.
Really, I think we should just make this a constant (to be used both here and in memlayout.h when checking the size). The recommended size is 12K right now which we declare "safe" (covered by vboot unit tests, I think/hope) and as long as that works there's never a reason to use a larger size. Maybe we should even directly use the VB2_WORKBUF_RECOMMENDED_SIZE constant from vboot's headers (although that's tricky because memlayout files aren't C files, so vboot would have to provide that constant in a separate header... another option would be to maintain our own constant but _Static_assert() that it is equal to VB2_WORKBUF_RECOMMENDED_SIZE somewhere).
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@66 PS1, Line 66: static struct selected_region *vb2_selected_region(void) nit: arguable if this still needs to be a separate function if the implementation is so trivial.
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@135 PS1, Line 135: vb2_store_working_data nit: maybe vb2_migrate_working_data() would be more accurate?
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@146 PS1, Line 146: vb2_working_data_size This should just be (buffer_offset + workbuf_used), no need to copy the leftover garbage near the end that vboot didn't ask us to preserve. In fact, you need to preserve workbuf_used anyway so that you can set it in the next vb2_context you initialize, otherwise vboot won't know that there is existing data left in the work buffer and just overwrite it on the first allocation. I suggest you add a new field for that to vb2_working_data.
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@155 PS1, Line 155: SRAM SRAM/CAR (also, CBMEM usually capitalized in prose)
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@157 PS1, Line 157: static void vb2_store_cbmem(int unused) nit: I'm not really sure why this ever needed to be a separate function... just make vb2_store_working_data the init hook directly.