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 3:
(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;
This may be a silly question, but -- if coreboot already has functionality to migrate data from CAR to CBMEM, why don't we just rely on it for the entire vb2_working_data struct, rather than migrating it ourselves?
It's not tagged correctly. The CAR_GLOBAL migration, which we are actively trying to kill, only moves the CAR_GLOBAL section of memory into cbmem as a single chunk. The layout and ordering of that region is dictated how romstage is linked. No other program knows that information. In other words, that entry is short lived w.r.t. its usage: only while still in romstage after CAR is torn down.
Is it because the "lose access to SRAM on some SRAM architectures" case is not covered?
Assuming that this is a bad idea, and we still want to store a global pointer here -- what is the benefit of storing it as a CAR_GLOBAL?
The only point would be to support platforms that are still inherently reliant on CAR_GLOBAL which would be platforms that don't employ postcar stage, tear down CAR, and return back into C code for running next part of bootflow (linked in vboot logic in romstage while enabling STARTS_IN_ROMSTAGE).
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@48 PS1, Line 48: #if ENV_POSTCAR || ENV_RAMSTAGE
So far, Julius's original solution + #ifdef __PRE_RAM__ seems to make the most sense. […]
There is a ENV_CACHE_AS_RAM from rules.h that could be used in conjunction with ENV_ROMSTAGE. But my guess is that won't handle the more persistent SRAM platforms. I'd like to get rid of the direct __PRE_RAM__ usage instead of adding more.
for the logic:
cbmem should only be queried in ENV_POSTCAR, ENV_RAMSTAGE, and ENV_ROMSTAGE which is effectively cbmem_possibly_online() returning true.
else use the _vboot2_work symbol.
This is pretty much the same logic that already existed in the code.