Joel Kitching 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 4:
(10 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. […]
Right -- perhaps I'll deal with this in a separate CL, especially if we don't end up needing it for this one.
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@43 PS1, Line 43: static struct vb2_working_data *vb2_wd = NULL;
This may be a silly question, but -- if coreboot already has functionality to migrate data from CA […]
Okay, so unless I'm mistaken, we should mark the vb2_wb cache pointer as CAR_GLOBAL, even though we are trying to kill CAR_GLOBAL in the (near) future.
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 […]
If we don't end up using ENV_PRE_RAM, perhaps we should add it in a separate CL.
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. […]
Right... Anyways, it will either point to vb2_working_data or vb2_shared_data, and lb_vboot_handoff will be removed.
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@66 PS1, Line 66: static struct selected_region *vb2_selected_region(void)
Hmmm... well, I'm not convinced about the context thing. […]
Let's also continue this discussion over in the design doc.
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@157 PS1, Line 157: static void vb2_store_cbmem(int unused)
Isn't this something you can also just solve by adjusting the pointer in the coreboot table?
So now we're discussing in the design doc whether or not we should expose the vb2_working_data struct directly to the payload. Let's just continue the discussion there.
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?
I thought it was a slightly simpler solution. But you're right, caching the pointer results in less cbmem lookups. I'll switch it back.
https://review.coreboot.org/#/c/31329/3/src/security/vboot/common.c@51 PS3, Line 51: if (ENV_PRE_RAM)
Sorry, I confused myself. […]
Julius, I don't think this is quite right.
It's impossible to distinguish x86 vs. SRAM architectures using these two macro definitions. When ramstage is reached, both are set to 0.
[This is because the definition of ENV_CACHE_AS_RAM is only set to 1 in the case of __PRE_RAM__ being enabled.]
We could redefine ENV_CACHE_AS_RAM as being equivalent to CONFIG_CACHE_AS_RAM instead. But then we'd have to make sure the places that use this macro (early_variables.h, pgtbl.c, printk.c, rules.h) won't be affected.
Or we could make a new macro definition called ENV_CONFIG_CACHE_AS_RAM or the like, but that seems a bit excessive.
Actually, I'm still wondering about at what stage it could be potentially possible to lose access to SRAM. Perhaps there should be a new config that defines this instead? Or perhaps we can just use VBOOT_MIGRATE_WORKING_DATA directly here? (If VBOOT_MIGRATE_WORKING_DATA == 0, assume we can access preram symbols in ramstage.) Or redefine VBOOT_MIGRATE_WORKING_DATA to something like PRE_RAM_ALWAYS_ACCESSIBLE.
[I've temporarily created the helper function you suggested with the definition of ENV_CACHE_AS_RAM || !ENV_X86, but that's creating some pretty heavy assumptions about non-x86 architectures.]
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 t […]
Done
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 […]
Right, good point. I think I'll use STARTS_IN_BOOTBLOCK || !MIGRATE.