Aaron Durbin 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 2:
(2 comments)
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;
Okay, yeah, I just missed that, so using a global is fine then.
fwiw, static uninitialized variables are 0 by default as they land in the .bss. Any platform that relies on CAR_GLOBAL semantics would be broken with this global. Though, I'm hoping at this point it isn't any.
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@48 PS1, Line 48: #if ENV_POSTCAR || ENV_RAMSTAGE
Okay, I forgot about the symbol. […]
Yes, you have to handle the ENV_ cases as well when combined w/ VBOOT_STARTS_IN and SEPARATE_VERSTAGE. As I noted most of those cases exist in the same form elsewhere and could be lifted and/or reused. We removed some of those combinations so it's a little simpler, but that's what needs to be done to handle all the cases.
vboot_possibly_executed() also exists (but not exposed). I think we could put together some foundational helpers that deal with the currently supported combos. That would likely help for these cases.
The other logic lives in src/security/vboot/vboot_loader.c where it handles many of those cases as well. Pushing those helpers up would be beneficial, I think.