9 comments:
Patch Set #3, 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.)
File src/security/vboot/common.c:
Patch Set #1, 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.
Patch Set #1, 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.
Patch Set #1, 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.
Patch Set #1, 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?
File src/security/vboot/common.c:
Patch Set #3, Line 44: static uint8_t use_cbmem CAR_GLOBAL;
Not sure how this is better than caching the pointer directly like you did before?
Patch Set #3, 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.
Patch Set #3, 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.
File src/security/vboot/vboot_loader.c:
Patch Set #3, 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).
To view, visit change 31329. To unsubscribe, or for help writing mail filters, visit settings.