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 7:
(7 comments)
https://review.coreboot.org/#/c/31329/7/src/include/symbols.h File src/include/symbols.h:
https://review.coreboot.org/#/c/31329/7/src/include/symbols.h@127 PS7, Line 127: nit: maybe a short comment explaining what this does?
https://review.coreboot.org/#/c/31329/5/src/include/symbols.h File src/include/symbols.h:
https://review.coreboot.org/#/c/31329/5/src/include/symbols.h@130 PS5, Line 130: return ENV_CACHE_AS_RAM || !ENV_X86;
So just to confirm, in SRAM architectures where we do eventually lose access to SRAM, we would never […]
No, there are different special cases. For example, Mediatek SoCs have a second half of SRAM that gets torn down during romstage (although we don't currently put the work buffer there, but if we did, that SoC would just have to select MIGRATE_WORKBUF directly).
Note that this is about whether the memory is actually accessible (i.e. whether a read32() to that address hangs your CPU). That's different from the question of whether the respective symbols are known to the linker. With the current memlayout design, that's always true on non-x86 systems unless someone puts an explicit #if ENV_xxx into memlayout.ld. For this helper function we only care about the second part.
https://review.coreboot.org/#/c/31329/5/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/#/c/31329/5/src/security/vboot/common.c@145 PS5, Line 145: wd_cbmem = cbmem_add(CBMEM_ID_VBOOT_WORKBUF, vb2_working_data_size());
The trouble here is that we want to get access to the non-CBMEM version so that we can migrate the d […]
Oh, well... why don't you just access _vboot2_work directly? At the moment we're migrating we know that we want to migrate from the pre-RAM area to the CBMEM area... there's no need to call the complicated helper function to get the "from" part and create a chicken-and-egg problem.
https://review.coreboot.org/#/c/31329/7/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/#/c/31329/7/src/security/vboot/common.c@140 PS7, Line 140: * vb2_working_data from SRAM/CAR into CBMEM. nit: maybe mention the part where it's not necessary on all platforms
https://review.coreboot.org/#/c/31329/7/src/security/vboot/common.c@145 PS7, Line 145: const struct vb2_working_data *wd = vb2_get_working_data(1); nit: "wd_preram" for clarity?
https://review.coreboot.org/#/c/31329/7/src/security/vboot/common.c@146 PS7, Line 146: struct vb2_working_data *wd_cbmem = vb2_get_working_data(0); I... guess... this works? But it still seems somewhat weird to me, honestly. I think it would be easier to just use cbmem_add() and _vboot_work directly in this function to make it absolutely clear what you're dealing with. The way you wrote it now relies on special knowledge about how your helper is implemented... but the whole point of the helper is to hide those details, and the whole point of this function here is that we know exactly which two instances of the working data we're dealing with, so we shouldn't need to use the helper to find them.
https://review.coreboot.org/#/c/31329/7/src/security/vboot/common.c@147 PS7, Line 147: printk(BIOS_INFO, "VBOOT: copying vb2_working_data to CBMEM...\n"); I'm not sure if this is really interesting enough to print. At least make it DEBUG, not INFO.