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 3:
(9 comments)
https://review.coreboot.org/#/c/31329/3/src/include/rules.h File src/include/rules.h:
https://review.coreboot.org/#/c/31329/3/src/include/rules.h@285 PS3, Line 285: /* Indicates that the current stage begins as pre-DRAM. */ Please do this in a separate CL, and maybe merge it with the defined(__PRE_RAM__) block above. (Although I think we won't actually need this -- see other comment -- but it would be nice to have anyway. Although if we're introducing it, it would be nice to just find&replace all the existing examples of __PRE_RAM__ with it while we're at it.)
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@48 PS1, Line 48: #if ENV_POSTCAR || ENV_RAMSTAGE I think ENV_CACHE_AS_RAM should work, actually, and is the best option (so that you can still access the SRAM area in the !VBOOT_MIGRATE_WORKING_DATA case).
I have another related question -- why does cbmem_possibly_online logic include a check of CONFIG_VBOOT_STARTS_IN_BOOTBLOCK?
Because that effects when the verstage runs. If it is true, the verstage runs before CBMEM comes online... if not, after.
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@50 PS1, Line 50: vb2_wd = cbmem_find(CBMEM_ID_VBOOT_WORKBUF);
So we will add a function to coreboot_table.c called lb_vboot_shared_data which will point to vb2_shared_data, regardless its location (SRAM or CBMEM, but probably not CAR).
Yes... I assume this is gonna replace lb_vboot_handoff which is also already a custom function anyway.
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@66 PS1, Line 66: static struct selected_region *vb2_selected_region(void)
Well... […]
Hmmm... well, I'm not convinced about the context thing. I was under the impression that the context was specifically not designed to survive, instead it defines how each individual caller interacts with vboot. Things like the workbuffer pointer can't just be copied around from one environment to the other.
But even if you did this, I don't see how it would affect whether we want to store the selected region in the same area as well? You'd still need that too, it doesn't change.
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@157 PS1, Line 157: static void vb2_store_cbmem(int unused)
Yes, part of the motivation is that coreboot no longer needs to access vb2_shared_data directly. […]
Isn't this something you can also just solve by adjusting the pointer in the coreboot table?
https://review.coreboot.org/#/c/31329/3/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/#/c/31329/3/src/security/vboot/common.c@44 PS3, Line 44: static uint8_t use_cbmem CAR_GLOBAL; Not sure how this is better than caching the pointer directly like you did before?
https://review.coreboot.org/#/c/31329/3/src/security/vboot/common.c@51 PS3, Line 51: if (ENV_PRE_RAM) So, actually, this should be ENV_CACHE_AS_RAM after all, so that you'll still return the SRAM buffer on boards that don't have VBOOT_MIGRATE_WORKING_DATA enabled.
https://review.coreboot.org/#/c/31329/3/src/security/vboot/common.c@152 PS3, Line 152: #if IS_ENABLED(CONFIG_VBOOT_MIGRATE_WORKING_DATA) I think this needs to include the function itself as well, or you'll get unused function errors in the other case.
https://review.coreboot.org/#/c/31329/3/src/security/vboot/vboot_loader.c File src/security/vboot/vboot_loader.c:
https://review.coreboot.org/#/c/31329/3/src/security/vboot/vboot_loader.c@31 PS3, Line 31: IS_ENABLED(CONFIG_VBOOT_MIGRATE_WORKING_DATA) == 1, This is not the right check -- this is essentially an XOR (either one or the other MUST be set), but what you want is an implication (if starts in romstage, then don't migrate). You can still not start in romstage but also not migrate. So you want to assert (!STARTS_IN_ROMSTAGE || !MIGRATE) (or (!(STARTS_IN_ROMSTAGE && MIGRATE)), depending on which side of De Morgan you prefer).