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 2:
(9 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@a68 PS1, Line 68:
Seems logical to just remove it.
Actually, it may be worth keeping (as deprecated) so the cbmem userspace utility can still identify these regions on systems with older coreboot. Sorry for being ambiguous earlier.
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@43 PS1, Line 43: static struct vb2_working_data *vb2_wd = NULL;
Well, it's set down in vb2_store_working_data so that it points to the CBMEM copy for any other code […]
Okay, yeah, I just missed that, so using a global is fine then.
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@48 PS1, Line 48: #if ENV_POSTCAR || ENV_RAMSTAGE
The region where _vboot2_work resides is torn down on certain platforms. […]
Okay, I forgot about the symbol. I don't see a really easy way to write this just with cbmem_possibly_online() (because romstage may still need to access _vboot2_work but ramstage must not reference the symbol anymore).
I think the best solution may just be what I wrote above, but wrap the _vboot2_work assignment in a #ifdef __PRE_RAM__.
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@50 PS1, Line 50: vb2_wd = cbmem_find(CBMEM_ID_VBOOT_WORKBUF);
(1) How long can we expect the data to stay in SRAM? Will it be cleared after firmware finishes run […]
(1) It depends on the system, but for most we never touch SRAM again after coreboot. Those that are exceptions would have to manually select VBOOT_MIGRATE_WORKING_DATA. (Note that this only needs to be accessible until the end of depthcharge, anyway... when crossystem accesses VBSD, it works on a copy that depthcharge put into the FDT.)
(2) You're talking about passing this to the payload, right? Because within coreboot, it could always rely on that Kconfig to decide. For passing to the payload, we already have a separate pointer to the memory in the coreboot table anyway (see LB_TAB_VBOOT_HANDOFF). That doesn't need to point into CBMEM... just point it straight into SRAM on the platforms that don't need to migrate.
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@66 PS1, Line 66: static struct selected_region *vb2_selected_region(void)
Perhaps we can leave it for now? IMHO it's arguable whether or not it should be stored as part of t […]
I mean... where else would you put it? I think this is way easier than making a separate CBMEM region just for this or something like that.
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@135 PS1, Line 135: vb2_store_working_data
IMHO migrate implies some kind of transformation of the data. What about copy? Move? […]
I think we use "migrate" a lot when talking about moving things from pre-RAM to post-RAM (e.g. "CAR migration"). But you can call it copy if you prefer.
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@146 PS1, Line 146: vb2_working_data_size
Re. […]
Hmmm... yeah, I'm not sure why we're zeroing the whole thing in vb2_init_working_context() either. It's a waste of time. vboot already needs to (and does) pay attention to not rely on workbuffer initialization anyway.
I've also been wondering whether it's that great to copy the whole workbuffer around as is... but that seems to be the model that vboot is designed for. I think one of the points of this whole exercise is to make vb2_shared_data opaque to coreboot and depthcharge again, which means that you can't manually rejuggle the workbuffer afterwards like you said. This also gives vboot freedom to add or change things it wants to preserve without changing the calling code, which is somewhat nice. The only real alternatives I can see would be to either make vb2_sd *way* bigger (so that all data that needs preserving is directly part of it), or to move all the things that need to be preserved outside the workbuffer into separate buffers provided by the callers and pointed to in the vb2_context (which wouild be somewhat hard since the sizes of preserved objects aren't always known in advance, like with vb21 preambles).
So I think despite some inefficiency, preserving the whole workbuffer may be the cleanest option, and the closest to the way vboot was designed to work. After all, vboot also already relies upon the fact that the workbuffer stays the same between the different phases of firmware verification... and really, from vboot's point there's no big difference between the gap between vb2api_fw_phase1 <-> vb2api_fw_phase2 and between vb2api_fw_phase3 <-> vb2api_kernel_phase1. Keeping the same workbuffer throughout the whole boot means it can treat all of these gaps the same, which is nice (and may become more important as we pull vboot apart into more smaller-scoped invocations).
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@155 PS1, Line 155: SRAM
Thanks for the tip. […]
No. CAR is CAR and SRAM is SRAM. x86 has CAR and everything else usually has SRAM. I'm not a silicon technology guy, so I don't know how caches are actually physically implemented... it may be the same technology as SRAM. But when we say "SRAM" in coreboot, we always refer to memory that's separate from the cache and generally stays available for the lifetime of the system.
(The Mediatek MT8173 is an exception where part of the "SRAM" is actually cache, and at some point in romstage we flip a switch that makes that part disappear and increases the cache size respectively. So I guess that would technically be CAR, but we still talk about it as "SRAM" in the code. In coreboot, for now, when we say "CAR" we always mean the x86-specific implementation.)
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@157 PS1, Line 157: static void vb2_store_cbmem(int unused)
Previously -- yes I'm not entirely sure either. […]
No, I can't really guess its purpose. ;) Wasn't the whole point of this that coreboot doesn't need to access vb2_sd directly anymore?