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 9:
(4 comments)
I ended up taking the cbmem_add call out of vb2_get_working_data -- making it lazily create the CBMEM entry caused issues when calling the vb2_working_data_size function after preram symbols already disappeared. I think it's best (safest and clearest) to proactively create the CBMEM entry once CBMEM comes up.
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;
Yes. […]
Switching to "available".
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 was planning on doing this in a subsequent CL -- saving a pointer to either vb2_shared_data or vb2_working_data in CBTABLE for libpayload to use. Perhaps I'll just take this text out for now.
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. […]
Right, thanks for catching this.
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?
Sure, but this function doesn't have access to vb2_context (that's stored in verstage_main). Another reason why it might be elegant to stack vb2_context and vb2_shared_data together in memory...