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 8:
(4 comments)
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;
Just to confirm, we are using the word "accessible" in two different senses: […]
Yes. Not sure if accessible was the best word for this, feel free to pick something else if you want (preram_symbols_linked? preram_symbols_available?).
I think the help text should just say that any platform where the the original location of the VBOOT_WORKBUF region becomes inaccessible (here's it's in the sense of (2)) in later stages should manually select MIGRATE_WORKBUF.
https://review.coreboot.org/#/c/31329/8/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/#/c/31329/8/src/security/vboot/Kconfig@107 PS8, Line 107: a pointer will be stored to the original data Note you're not actually doing that yet. I think that needs to be part of this patch or you'll break the SRAM boards.
https://review.coreboot.org/#/c/31329/8/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/#/c/31329/8/src/security/vboot/common.c@61 PS8, Line 61: return cbmem_add(CBMEM_ID_VBOOT_WORKBUF, No, we took a wrong turn somewhere now. This still needs to check whether that cbmem_add() returns NULL and then fall back to the below in case that condition is true (for early romstage).
https://review.coreboot.org/#/c/31329/8/src/security/vboot/common.c@152 PS8, Line 152: vb2_working_data_size Can't we restrict this to be based on workbuf_used?