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 8: Code-Review+1
(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?
Done
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;
No, there are different special cases. […]
Just to confirm, we are using the word "accessible" in two different senses: (1) symbols accessible: symbols are known to the linker (2) memory accessible: a read32() to the address does not hang the CPU
This function is concerned with (1), thus we don't need to worry about the specifics of (2) and can leave that to the user to select MIGRATE_WORKBUF if necessary.
Your example about the second half of SRAM on Mediatek SoCs helps clarify. Would that be useful to put in the HELP text for MIGRATE_WORKBUF?
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());
Oh, well... […]
Done
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
Done
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?
Done
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. […]
Yeah, fair enough. We end up with a duplicate cbmem_add call this way, but I think you're right that vb2_migrate_working_data not calling vb2_get_working_data at all is the best route.
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.
Done