Joel Kitching has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31329
Change subject: vboot: copy data structures to cbmem for downstream use ......................................................................
vboot: copy data structures to cbmem for downstream use
For platforms that do not employ VBOOT_STARTS_IN_ROMSTAGE, vboot verification occurs before cbmem is brought online. In order to make vboot data structures available downstream, copy vb2_working_data from SRAM into cbmem when cbmem comes online.
BUG=b:124141368, b:124192753 TEST=Build locally TEST=util/lint/checkpatch.pl --ignore GERRIT_CHANGE_ID -g origin/master..HEAD TEST=util/abuild/abuild -B -e -y -c 50 -p none -x BRANCH=none
Change-Id: I62c11268a83927bc00ae9bd93b1b31363b38e8cf Signed-off-by: Joel Kitching kitching@google.com --- M src/security/vboot/common.c 1 file changed, 32 insertions(+), 51 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/31329/1
diff --git a/src/security/vboot/common.c b/src/security/vboot/common.c index 747644a..8b73697 100644 --- a/src/security/vboot/common.c +++ b/src/security/vboot/common.c @@ -40,47 +40,32 @@ uint32_t buffer_size; };
-static const size_t vb_work_buf_size = 16 * KiB; +static struct vb2_working_data *vb2_wd = NULL;
static struct vb2_working_data * const vboot_get_working_data(void) { - if (IS_ENABLED(CONFIG_VBOOT_STARTS_IN_ROMSTAGE)) - /* cbmem_add() does a cbmem_find() first. */ - return cbmem_add(CBMEM_ID_VBOOT_WORKBUF, vb_work_buf_size); - else - return (struct vb2_working_data *)_vboot2_work; + if (vb2_wd == NULL) { +#if ENV_POSTCAR || ENV_RAMSTAGE + /* cbmem_find is linked from romstage onward */ + vb2_wd = cbmem_find(CBMEM_ID_VBOOT_WORKBUF); +#else + /* _vboot2_work is accessible until end of romstage */ + vb2_wd = (struct vb2_working_data *)_vboot2_work; +#endif + } + assert(vb2_wd != NULL); + return vb2_wd; }
static size_t vb2_working_data_size(void) { - if (IS_ENABLED(CONFIG_VBOOT_STARTS_IN_ROMSTAGE)) - return vb_work_buf_size; - else - return _vboot2_work_size; + /* _vboot2_work_size is accessible until end of romstage */ + return _vboot2_work_size; }
static struct selected_region *vb2_selected_region(void) { - struct selected_region *sel_reg = NULL; - - /* Ramstage and postcar always uses cbmem as a source of truth. */ - if (ENV_RAMSTAGE || ENV_POSTCAR) - sel_reg = cbmem_find(CBMEM_ID_VBOOT_SEL_REG); - else if (ENV_ROMSTAGE) { - /* Try cbmem first. Fall back on working data if not found. */ - sel_reg = cbmem_find(CBMEM_ID_VBOOT_SEL_REG); - - if (sel_reg == NULL) { - struct vb2_working_data *wd = vboot_get_working_data(); - sel_reg = &wd->selected_region; - } - } else { - /* Stages such as bootblock and verstage use working data. */ - struct vb2_working_data *wd = vboot_get_working_data(); - sel_reg = &wd->selected_region; - } - - return sel_reg; + return &vboot_get_working_data()->selected_region; }
void vb2_init_work_context(struct vb2_context *ctx) @@ -104,7 +89,6 @@ memset(ctx, 0, sizeof(*ctx)); ctx->workbuf = (void *)vb2_get_shared_data(); ctx->workbuf_size = wd->buffer_size; - }
struct vb2_shared_data *vb2_get_shared_data(void) @@ -148,33 +132,30 @@ return reg->size > 0; }
-void vb2_store_selected_region(void) +void vb2_store_working_data(void) { - const struct vb2_working_data *wd; - struct selected_region *sel_reg; + struct vb2_working_data *wd; + struct vb2_working_data *wd_cbmem;
- /* Always use the working data in this path since it's the object - * which has the result.. */ + wd_cbmem = cbmem_add(CBMEM_ID_VBOOT_WORKBUF, vb2_working_data_size()); + assert(wd_cbmem != NULL); + +#if !IS_ENABLED(CONFIG_VBOOT_STARTS_IN_ROMSTAGE) wd = vboot_get_working_data(); + /* No data to copy yet on platforms with VBOOT_STARTS_IN_ROMSTAGE. */ + memcpy(wd_cbmem, wd, vb2_working_data_size()); +#endif
- sel_reg = cbmem_add(CBMEM_ID_VBOOT_SEL_REG, sizeof(*sel_reg)); - - assert(sel_reg != NULL); - - sel_reg->offset = wd->selected_region.offset; - sel_reg->size = wd->selected_region.size; + vb2_wd = wd_cbmem; }
/* - * For platforms that employ VBOOT_STARTS_IN_ROMSTAGE, the vboot - * verification doesn't happen until after cbmem is brought online. - * Therefore, the selected region contents would not be initialized - * so don't automatically add results when cbmem comes online. + * For platforms that do not employ VBOOT_STARTS_IN_ROMSTAGE, vboot verification + * occurs before cbmem is brought online. In order to make vboot data + * structures available downstream, copy vb2_working_data from SRAM into cbmem. */ -#if !IS_ENABLED(CONFIG_VBOOT_STARTS_IN_ROMSTAGE) -static void vb2_store_selected_region_cbmem(int unused) +static void vb2_store_cbmem(int unused) { - vb2_store_selected_region(); + vb2_store_working_data(); } -ROMSTAGE_CBMEM_INIT_HOOK(vb2_store_selected_region_cbmem) -#endif +ROMSTAGE_CBMEM_INIT_HOOK(vb2_store_cbmem)
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 1:
(11 comments)
Can you try to find a STARTS_IN_ROMSTAGE board to test this on to make sure you're not breaking that path? I'm not sure if any of them still works... I used to keep testing stuff on Falco for a while, so if you can find one of those (or other Slippy variant like Peppy, Leon, Wolf) it might work. Otherwise, it might also work to just hack up the Kconfig of a newer board from STARTS_IN_BOOTBLOCK to STARTS_IN_ROMSTAGE.
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: If you eliminate this CBMEM ID please remove it from <commonlib/cbmem_id.h>, or at least mark it as deprecated.
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@a151 PS1, Line 151: There's another call to this function in vboot_loader.c that you also need to remove.
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@43 PS1, Line 43: static struct vb2_working_data *vb2_wd = NULL; If this is just supposed to be a cache for vboot_get_working_data() and not supposed to be accessed directly by anything else, better make it a static local.
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@48 PS1, Line 48: #if ENV_POSTCAR || ENV_RAMSTAGE Please never use preprocessor #if where a C if () would also work. Also, this whole thing should probably be written like this instead:
if (vb2_wd) return vb2_wd;
if (cbmem_possibly_online()) vb2_wd = cbmem_find(CBMEM_ID_VBOOT_WORKBUF);
if (!vb2_wd) vb2_wb = (...)_vboot2_work;
return vb2_wb;
(That way, it also does the right thing in early romstage before CBMEM has been initialized, which may be relevant if CBFS accesses happen at that time and run the vboot locator.)
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@50 PS1, Line 50: vb2_wd = cbmem_find(CBMEM_ID_VBOOT_WORKBUF); Can we add another option VBOOT_MIGRATE_WORKING_DATA (which would be default y if ARCH_X86, otherwise default n) to control the CBMEM copying? SRAM architectures *usually* never lose access to their SRAM, so for those it should be okay to just keep the working data where it is and pass a pointer to there to depthcharge.
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@62 PS1, Line 62: /* _vboot2_work_size is accessible until end of romstage */ How does this work for the STARTS_IN_ROMSTAGE case? I don't think the _vboot2_work_size label is available there.
Really, I think we should just make this a constant (to be used both here and in memlayout.h when checking the size). The recommended size is 12K right now which we declare "safe" (covered by vboot unit tests, I think/hope) and as long as that works there's never a reason to use a larger size. Maybe we should even directly use the VB2_WORKBUF_RECOMMENDED_SIZE constant from vboot's headers (although that's tricky because memlayout files aren't C files, so vboot would have to provide that constant in a separate header... another option would be to maintain our own constant but _Static_assert() that it is equal to VB2_WORKBUF_RECOMMENDED_SIZE somewhere).
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@66 PS1, Line 66: static struct selected_region *vb2_selected_region(void) nit: arguable if this still needs to be a separate function if the implementation is so trivial.
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@135 PS1, Line 135: vb2_store_working_data nit: maybe vb2_migrate_working_data() would be more accurate?
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@146 PS1, Line 146: vb2_working_data_size This should just be (buffer_offset + workbuf_used), no need to copy the leftover garbage near the end that vboot didn't ask us to preserve. In fact, you need to preserve workbuf_used anyway so that you can set it in the next vb2_context you initialize, otherwise vboot won't know that there is existing data left in the work buffer and just overwrite it on the first allocation. I suggest you add a new field for that to vb2_working_data.
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@155 PS1, Line 155: SRAM SRAM/CAR (also, CBMEM usually capitalized in prose)
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@157 PS1, Line 157: static void vb2_store_cbmem(int unused) nit: I'm not really sure why this ever needed to be a separate function... just make vb2_store_working_data the init hook directly.
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 1:
(11 comments)
Patch Set 1:
(11 comments)
Can you try to find a STARTS_IN_ROMSTAGE board to test this on to make sure you're not breaking that path? I'm not sure if any of them still works... I used to keep testing stuff on Falco for a while, so if you can find one of those (or other Slippy variant like Peppy, Leon, Wolf) it might work. Otherwise, it might also work to just hack up the Kconfig of a newer board from STARTS_IN_BOOTBLOCK to STARTS_IN_ROMSTAGE.
Yes, I will find a STARTS_IN_ROMSTAGE board as you suggested.
However, since we don't necessarily care about old boards running on ToT, and since there would conceivably be no reason for new boards to start vboot in romstage, I wonder whether we can just deprecate STARTS_IN_ROMSTAGE and continue simplifying some code?
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:
If you eliminate this CBMEM ID please remove it from <commonlib/cbmem_id. […]
Seems logical to just remove it.
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@a151 PS1, Line 151:
There's another call to this function in vboot_loader.c that you also need to remove.
Right, thanks for the reminder!
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@43 PS1, Line 43: static struct vb2_working_data *vb2_wd = NULL;
If this is just supposed to be a cache for vboot_get_working_data() and not supposed to be accessed […]
Well, it's set down in vb2_store_working_data so that it points to the CBMEM copy for any other code that might run afterwards in romstage. If you have any other ideas of how to structure this I'm open to suggestions.
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@48 PS1, Line 48: #if ENV_POSTCAR || ENV_RAMSTAGE
Please never use preprocessor #if where a C if () would also work. […]
The reason I used preprocessor conditionals -- * ramstage complained of undefined reference to _vboot2_work * verstage complained of undefined reference to cbmem_find
For the version that you wrote, cbmem_find doesn't seem to be a problem (maybe it gets optimized out for verstage?), but I'm encountering the same problem with undefined _vboot2_work.
I wonder if we should just make _vboot2_work available to ramstage onward?
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@50 PS1, Line 50: vb2_wd = cbmem_find(CBMEM_ID_VBOOT_WORKBUF);
Can we add another option VBOOT_MIGRATE_WORKING_DATA (which would be default y if ARCH_X86, otherwis […]
(1) How long can we expect the data to stay in SRAM? Will it be cleared after firmware finishes running? (2) This would make code a little bit more fragmented. The cleanest way I can think of is to create two CBMEM entries -- one called CBMEM_ID_VBOOT_WORKBUF_PTR (always exists), and one called CBMEM_ID_VBOOT_WORKBUF (sometimes exists). PTR points to either CBMEM or SRAM. Is it worth it?
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@62 PS1, Line 62: /* _vboot2_work_size is accessible until end of romstage */
How does this work for the STARTS_IN_ROMSTAGE case? I don't think the _vboot2_work_size label is ava […]
Still need to think about this...
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@66 PS1, Line 66: static struct selected_region *vb2_selected_region(void)
nit: arguable if this still needs to be a separate function if the implementation is so trivial.
Perhaps we can leave it for now? IMHO it's arguable whether or not it should be stored as part of the vb2_working_data struct at all.
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@135 PS1, Line 135: vb2_store_working_data
nit: maybe vb2_migrate_working_data() would be more accurate?
IMHO migrate implies some kind of transformation of the data. What about copy? Move? Any thoughts?
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@146 PS1, Line 146: vb2_working_data_size
This should just be (buffer_offset + workbuf_used), no need to copy the leftover garbage near the en […]
Re. your first point -- Seems like on creating a vb2_context, the memory following it is always set to 0. If we only copy buffer_offset + workbuf_used, then we'd potentially have garbage instead of 0's afterwards (in CBMEM version). Now, this may not be an issue if: * Code that writes to the workbuf doesn't assume it was already 0'd * We don't need to write any more data to the workbuf after this point
But I'm not sure if these two statements are true.
Re. your second point -- Still trying to decide the best thing to do here. Seems rather awkward to have vb2_shared_data followed by the rest of the workbuf, of which nobody is quite sure of the contents, and whether or not all of it is needed for the vb2_shared_data to be restored (in the next binary). But maybe as part of the "import vb2_shared_data object from previous binary" function, we can just take the relevant parts (pointed to in vb2_shared_data with offset+size) and defragment them together.
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@155 PS1, Line 155: SRAM
SRAM/CAR (also, CBMEM usually capitalized in prose)
Thanks for the tip. Probably not the right place to ask, but is SRAM and CAR (presumably cache-as-RAM) one and the same thing? Or is CAR only *sometimes* SRAM?
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@157 PS1, Line 157: static void vb2_store_cbmem(int unused)
nit: I'm not really sure why this ever needed to be a separate function... […]
Previously -- yes I'm not entirely sure either. For the future though -- I was going to add something called vb2_store_shared_data_ptr() (you can probably guess its purpose) after the vb2_store_working_data call. Of course they could be merged into the same function, but it seems somewhat elegant to separate them.
Hello Randall Spangler, Aaron Durbin, Julius Werner, Duncan Laurie, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31329
to look at the new patch set (#2).
Change subject: vboot: copy data structures to CBMEM for downstream use ......................................................................
vboot: copy data structures to CBMEM for downstream use
For platforms that do not employ VBOOT_STARTS_IN_ROMSTAGE, vboot verification occurs before CBMEM is brought online. In order to make vboot data structures available downstream, copy vb2_working_data from SRAM into CBMEM when CBMEM comes online.
BUG=b:124141368, b:124192753 TEST=Build locally TEST=util/lint/checkpatch.pl -g origin/master..HEAD TEST=util/abuild/abuild -B -e -y -c 50 -p none -x BRANCH=none
Change-Id: I62c11268a83927bc00ae9bd93b1b31363b38e8cf Signed-off-by: Joel Kitching kitching@google.com --- M src/commonlib/include/commonlib/cbmem_id.h M src/security/vboot/common.c M src/security/vboot/misc.h M src/security/vboot/vboot_loader.c 4 files changed, 35 insertions(+), 56 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/31329/2
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:
(1 comment)
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@48 PS1, Line 48: #if ENV_POSTCAR || ENV_RAMSTAGE
The reason I used preprocessor conditionals -- […]
The region where _vboot2_work resides is torn down on certain platforms. It doesn't just continue to exist. You have to be more careful with the C conditionals.
cbmem_possibly_online() is your friend for postcar and ramstage as well as odd conditionals for verstage being ran after romstage (separate stage vs linked matters here as well).
I believe nearly all this similar logic was already handled in here or vboot_logic since it needs to deal with most of those cases.
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?
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.
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:
(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;
fwiw, static uninitialized variables are 0 by default as they land in the .bss. […]
We could just mark this CAR_GLOBAL, right? Shouldn't hurt?
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@48 PS1, Line 48: #if ENV_POSTCAR || ENV_RAMSTAGE
Yes, you have to handle the ENV_ cases as well when combined w/ VBOOT_STARTS_IN and SEPARATE_VERSTAG […]
Well... I don't see any existing helpers that would really help here (other than cbmem_possibly_online() in the way we're already using it). Whether vboot executed is mostly irrelevant here, the question is whether CBMEM is available. We could add another cbmem_definitely_online() for it, but in the end that's really just the same as __PRE_RAM__. (We could also add an ENV_PRE_RAM so we don't always need to use a raw #ifdef for that?)
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:
(1 comment)
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@48 PS1, Line 48: #if ENV_POSTCAR || ENV_RAMSTAGE
other than cbmem_possibly_online() in the way we're already using it
edit: I meant in the way I already suggested it in the first comment.
Hello Randall Spangler, Aaron Durbin, Julius Werner, Duncan Laurie, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31329
to look at the new patch set (#3).
Change subject: vboot: copy data structures to CBMEM for downstream use ......................................................................
vboot: copy data structures to CBMEM for downstream use
For platforms that do not employ VBOOT_STARTS_IN_ROMSTAGE, vboot verification occurs before CBMEM is brought online. In order to make vboot data structures available downstream, copy vb2_working_data from SRAM into CBMEM when CBMEM comes online.
BUG=b:124141368, b:124192753 TEST=Build locally TEST=util/lint/checkpatch.pl -g origin/master..HEAD TEST=util/abuild/abuild -B -e -y -c 50 -p none -x BRANCH=none
Change-Id: I62c11268a83927bc00ae9bd93b1b31363b38e8cf Signed-off-by: Joel Kitching kitching@google.com --- M src/commonlib/include/commonlib/cbmem_id.h M src/include/rules.h M src/security/vboot/Kconfig M src/security/vboot/common.c M src/security/vboot/misc.h M src/security/vboot/vboot_loader.c 6 files changed, 67 insertions(+), 73 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/31329/3
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 3:
(8 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:
Actually, it may be worth keeping (as deprecated) so the cbmem userspace utility can still identify […]
Good point. I'll add a comment to mark it as deprecated.
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@43 PS1, Line 43: static struct vb2_working_data *vb2_wd = NULL;
We could just mark this CAR_GLOBAL, right? Shouldn't hurt?
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?
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?
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@48 PS1, Line 48: #if ENV_POSTCAR || ENV_RAMSTAGE
other than cbmem_possibly_online() in the way we're already using it […]
So far, Julius's original solution + #ifdef __PRE_RAM__ seems to make the most sense. And yes, we still have the option of implementing a helper like cbmem_definitely_online() or creating ENV_PRE_RAM. * The logic between !cbmem_definitely_online implying that we still have access to _vboot2_work seems a bit far. * ENV_PRE_RAM seems like a nice way of getting rid of precompiler logic. There's already a ENV_CACHE_AS_RAM defined in src/include/rules.h, but it's not *quite* the same meaning that we are looking for, since we also want to allow for the SRAM case.
I'll add ENV_PRE_RAM -- does that work for everyone?
I'm also considering switching back to a boolean with CAR_GLOBAL instead of storing a cached pointer.
I have another related question -- why does cbmem_possibly_online logic include a check of CONFIG_VBOOT_STARTS_IN_BOOTBLOCK?
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) It depends on the system, but for most we never touch SRAM again after coreboot. […]
(1) Right, unless we want to access the full working_data buffer for some kind of debugging purposes, after booting into the OS. But I can't think of a specific use-case to keep a copy laying around somewhere on these SRAM architectures.
(2) Yes, I'm talking about passing to the payload. Actually it seems somewhat limiting that libpayload doesn't seem to have first-class support for CBMEM. Everything that libpayload needs to access seems to get shoved into the coreboot table. Yes, we could store a pointer directly to the SRAM, which requires writing custom code to write to coreboot table (unfortunately can't use add_cbmem_pointers), and custom code to read from it on the depthcharge side. I suppose that's the most straightforward way to do it at this point.
So we will add a function to coreboot_table.c called lb_vboot_shared_data which will point to vb2_shared_data, regardless its location (SRAM or CBMEM, but probably not CAR).
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@66 PS1, Line 66: static struct selected_region *vb2_selected_region(void)
I mean... […]
Well... Randall and I were also toying with the idea of giving vb2_context the ability to be cross-binary compatible. In this case, the whole chunk of vb2_context + vb2_shared_data + rest of workbuf would always travel as a contiguous chunk, and could be directly imported downstream by vboot. In this case, it would be convenient if we could directly point to a CBMEM object rather than point to the middle of one.
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@135 PS1, Line 135: vb2_store_working_data
I think we use "migrate" a lot when talking about moving things from pre-RAM to post-RAM (e.g. […]
OK, probably best to follow prior usage of the word then. I will switch to "migrate".
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@146 PS1, Line 146: vb2_working_data_size
Hmmm... yeah, I'm not sure why we're zeroing the whole thing in vb2_init_working_context() either. […]
Re. the zeroing in vb2_init_working_context: Let's get rid of that step. Zero-filling the vb2_working_data struct is sufficient.
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@157 PS1, Line 157: static void vb2_store_cbmem(int unused)
No, I can't really guess its purpose. […]
Yes, part of the motivation is that coreboot no longer needs to access vb2_shared_data directly. However, another point is to get rid of the whole vboot_handoff step. So we should find a way to send vb2_shared_data downstream directly, rather than transforming it to vboot1 equivalent and wrapping it in a vboot_handoff struct.
We can't just expect depthcharge to know where to find vb2_shared_data within the vb2_working_data structure, which is private to coreboot. I believe it would also be undesirable to teach it how to read this data structure. So we add another entry to CBMEM pointing to the data it needs -- vb2_shared_data. Thus the vb2_store_shared_data_ptr function.
I'll put all this in the design doc and send it out for review...
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.
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 3:
(9 comments)
https://review.coreboot.org/#/c/31329/3/src/include/rules.h File src/include/rules.h:
https://review.coreboot.org/#/c/31329/3/src/include/rules.h@285 PS3, Line 285: /* Indicates that the current stage begins as pre-DRAM. */ Please do this in a separate CL, and maybe merge it with the defined(__PRE_RAM__) block above. (Although I think we won't actually need this -- see other comment -- but it would be nice to have anyway. Although if we're introducing it, it would be nice to just find&replace all the existing examples of __PRE_RAM__ with it while we're at it.)
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@48 PS1, Line 48: #if ENV_POSTCAR || ENV_RAMSTAGE I think ENV_CACHE_AS_RAM should work, actually, and is the best option (so that you can still access the SRAM area in the !VBOOT_MIGRATE_WORKING_DATA case).
I have another related question -- why does cbmem_possibly_online logic include a check of CONFIG_VBOOT_STARTS_IN_BOOTBLOCK?
Because that effects when the verstage runs. If it is true, the verstage runs before CBMEM comes online... if not, after.
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@50 PS1, Line 50: vb2_wd = cbmem_find(CBMEM_ID_VBOOT_WORKBUF);
So we will add a function to coreboot_table.c called lb_vboot_shared_data which will point to vb2_shared_data, regardless its location (SRAM or CBMEM, but probably not CAR).
Yes... I assume this is gonna replace lb_vboot_handoff which is also already a custom function anyway.
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@66 PS1, Line 66: static struct selected_region *vb2_selected_region(void)
Well... […]
Hmmm... well, I'm not convinced about the context thing. I was under the impression that the context was specifically not designed to survive, instead it defines how each individual caller interacts with vboot. Things like the workbuffer pointer can't just be copied around from one environment to the other.
But even if you did this, I don't see how it would affect whether we want to store the selected region in the same area as well? You'd still need that too, it doesn't change.
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@157 PS1, Line 157: static void vb2_store_cbmem(int unused)
Yes, part of the motivation is that coreboot no longer needs to access vb2_shared_data directly. […]
Isn't this something you can also just solve by adjusting the pointer in the coreboot table?
https://review.coreboot.org/#/c/31329/3/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/#/c/31329/3/src/security/vboot/common.c@44 PS3, Line 44: static uint8_t use_cbmem CAR_GLOBAL; Not sure how this is better than caching the pointer directly like you did before?
https://review.coreboot.org/#/c/31329/3/src/security/vboot/common.c@51 PS3, Line 51: if (ENV_PRE_RAM) So, actually, this should be ENV_CACHE_AS_RAM after all, so that you'll still return the SRAM buffer on boards that don't have VBOOT_MIGRATE_WORKING_DATA enabled.
https://review.coreboot.org/#/c/31329/3/src/security/vboot/common.c@152 PS3, Line 152: #if IS_ENABLED(CONFIG_VBOOT_MIGRATE_WORKING_DATA) I think this needs to include the function itself as well, or you'll get unused function errors in the other case.
https://review.coreboot.org/#/c/31329/3/src/security/vboot/vboot_loader.c File src/security/vboot/vboot_loader.c:
https://review.coreboot.org/#/c/31329/3/src/security/vboot/vboot_loader.c@31 PS3, Line 31: IS_ENABLED(CONFIG_VBOOT_MIGRATE_WORKING_DATA) == 1, This is not the right check -- this is essentially an XOR (either one or the other MUST be set), but what you want is an implication (if starts in romstage, then don't migrate). You can still not start in romstage but also not migrate. So you want to assert (!STARTS_IN_ROMSTAGE || !MIGRATE) (or (!(STARTS_IN_ROMSTAGE && MIGRATE)), depending on which side of De Morgan you prefer).
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 3:
(1 comment)
https://review.coreboot.org/#/c/31329/3/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/#/c/31329/3/src/security/vboot/common.c@51 PS3, Line 51: if (ENV_PRE_RAM)
So, actually, this should be ENV_CACHE_AS_RAM after all, so that you'll still return the SRAM buffer […]
Sorry, I confused myself. ENV_CACHE_AS_RAM is always 0 for SRAM boards, which is of course not what I wanted. I wanted always 1.
Maybe we should add a new helper function somewhere to cover this case, something like preram_symbols_accessible() (true iff (!CACHE_AS_RAM || ENV_PRE_RAM)). Maybe just throw it in <symbols.h>.
Hello Randall Spangler, Aaron Durbin, Julius Werner, Duncan Laurie, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31329
to look at the new patch set (#4).
Change subject: vboot: copy data structures to CBMEM for downstream use ......................................................................
vboot: copy data structures to CBMEM for downstream use
For platforms that do not employ VBOOT_STARTS_IN_ROMSTAGE, vboot verification occurs before CBMEM is brought online. In order to make vboot data structures available downstream, copy vb2_working_data from SRAM into CBMEM when CBMEM comes online.
BUG=b:124141368, b:124192753 TEST=Build locally TEST=util/lint/checkpatch.pl -g origin/master..HEAD TEST=util/abuild/abuild -B -e -y -c 50 -p none -x BRANCH=none
Change-Id: I62c11268a83927bc00ae9bd93b1b31363b38e8cf Signed-off-by: Joel Kitching kitching@google.com --- M src/commonlib/include/commonlib/cbmem_id.h M src/include/symbols.h M src/security/vboot/Kconfig M src/security/vboot/common.c M src/security/vboot/misc.h M src/security/vboot/vboot_loader.c 6 files changed, 71 insertions(+), 74 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/31329/4
build bot (Jenkins) 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 4:
(1 comment)
https://review.coreboot.org/#/c/31329/4/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/#/c/31329/4/src/security/vboot/common.c@142 PS4, Line 142: #if IS_ENABLED(CONFIG_VBOOT_MIGRATE_WORKING_DATA) || IS_ENABLED(CONFIG_VBOOT_STARTS_IN_ROMSTAGE) line over 80 characters
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 4:
(10 comments)
https://review.coreboot.org/#/c/31329/3/src/include/rules.h File src/include/rules.h:
https://review.coreboot.org/#/c/31329/3/src/include/rules.h@285 PS3, Line 285: /* Indicates that the current stage begins as pre-DRAM. */
Please do this in a separate CL, and maybe merge it with the defined(__PRE_RAM__) block above. […]
Right -- perhaps I'll deal with this in a separate CL, especially if we don't end up needing it for this one.
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 CA […]
Okay, so unless I'm mistaken, we should mark the vb2_wb cache pointer as CAR_GLOBAL, even though we are trying to kill CAR_GLOBAL in the (near) future.
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@48 PS1, Line 48: #if ENV_POSTCAR || ENV_RAMSTAGE
I think ENV_CACHE_AS_RAM should work, actually, and is the best option (so that you can still access […]
If we don't end up using ENV_PRE_RAM, perhaps we should add it in a separate CL.
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@50 PS1, Line 50: vb2_wd = cbmem_find(CBMEM_ID_VBOOT_WORKBUF);
So we will add a function to coreboot_table. […]
Right... Anyways, it will either point to vb2_working_data or vb2_shared_data, and lb_vboot_handoff will be removed.
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@66 PS1, Line 66: static struct selected_region *vb2_selected_region(void)
Hmmm... well, I'm not convinced about the context thing. […]
Let's also continue this discussion over in the design doc.
https://review.coreboot.org/#/c/31329/1/src/security/vboot/common.c@157 PS1, Line 157: static void vb2_store_cbmem(int unused)
Isn't this something you can also just solve by adjusting the pointer in the coreboot table?
So now we're discussing in the design doc whether or not we should expose the vb2_working_data struct directly to the payload. Let's just continue the discussion there.
https://review.coreboot.org/#/c/31329/3/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/#/c/31329/3/src/security/vboot/common.c@44 PS3, Line 44: static uint8_t use_cbmem CAR_GLOBAL;
Not sure how this is better than caching the pointer directly like you did before?
I thought it was a slightly simpler solution. But you're right, caching the pointer results in less cbmem lookups. I'll switch it back.
https://review.coreboot.org/#/c/31329/3/src/security/vboot/common.c@51 PS3, Line 51: if (ENV_PRE_RAM)
Sorry, I confused myself. […]
Julius, I don't think this is quite right.
It's impossible to distinguish x86 vs. SRAM architectures using these two macro definitions. When ramstage is reached, both are set to 0.
[This is because the definition of ENV_CACHE_AS_RAM is only set to 1 in the case of __PRE_RAM__ being enabled.]
We could redefine ENV_CACHE_AS_RAM as being equivalent to CONFIG_CACHE_AS_RAM instead. But then we'd have to make sure the places that use this macro (early_variables.h, pgtbl.c, printk.c, rules.h) won't be affected.
Or we could make a new macro definition called ENV_CONFIG_CACHE_AS_RAM or the like, but that seems a bit excessive.
Actually, I'm still wondering about at what stage it could be potentially possible to lose access to SRAM. Perhaps there should be a new config that defines this instead? Or perhaps we can just use VBOOT_MIGRATE_WORKING_DATA directly here? (If VBOOT_MIGRATE_WORKING_DATA == 0, assume we can access preram symbols in ramstage.) Or redefine VBOOT_MIGRATE_WORKING_DATA to something like PRE_RAM_ALWAYS_ACCESSIBLE.
[I've temporarily created the helper function you suggested with the definition of ENV_CACHE_AS_RAM || !ENV_X86, but that's creating some pretty heavy assumptions about non-x86 architectures.]
https://review.coreboot.org/#/c/31329/3/src/security/vboot/common.c@152 PS3, Line 152: #if IS_ENABLED(CONFIG_VBOOT_MIGRATE_WORKING_DATA)
I think this needs to include the function itself as well, or you'll get unused function errors in t […]
Done
https://review.coreboot.org/#/c/31329/3/src/security/vboot/vboot_loader.c File src/security/vboot/vboot_loader.c:
https://review.coreboot.org/#/c/31329/3/src/security/vboot/vboot_loader.c@31 PS3, Line 31: IS_ENABLED(CONFIG_VBOOT_MIGRATE_WORKING_DATA) == 1,
This is not the right check -- this is essentially an XOR (either one or the other MUST be set), but […]
Right, good point. I think I'll use STARTS_IN_BOOTBLOCK || !MIGRATE.
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 4:
(1 comment)
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@62 PS1, Line 62: /* _vboot2_work_size is accessible until end of romstage */
Still need to think about this...
This should be fixed by: https://review.coreboot.org/c/coreboot/+/31474
Hello Randall Spangler, Aaron Durbin, Julius Werner, Duncan Laurie, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31329
to look at the new patch set (#5).
Change subject: vboot: copy data structures to CBMEM for downstream use ......................................................................
vboot: copy data structures to CBMEM for downstream use
For platforms that do not employ VBOOT_STARTS_IN_ROMSTAGE, vboot verification occurs before CBMEM is brought online. In order to make vboot data structures available downstream, copy vb2_working_data from SRAM into CBMEM when CBMEM comes online.
BUG=b:124141368, b:124192753 TEST=Build locally TEST=util/lint/checkpatch.pl -g origin/master..HEAD TEST=util/abuild/abuild -B -e -y -c 50 -p none -x BRANCH=none
Change-Id: I62c11268a83927bc00ae9bd93b1b31363b38e8cf Signed-off-by: Joel Kitching kitching@google.com --- M src/commonlib/include/commonlib/cbmem_id.h M src/include/symbols.h M src/security/vboot/Kconfig M src/security/vboot/common.c M src/security/vboot/misc.h M src/security/vboot/symbols.h M src/security/vboot/vboot_loader.c 7 files changed, 76 insertions(+), 74 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/31329/5
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 5:
(1 comment)
https://review.coreboot.org/#/c/31329/3/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/#/c/31329/3/src/security/vboot/common.c@44 PS3, Line 44: static uint8_t use_cbmem CAR_GLOBAL;
I thought it was a slightly simpler solution. […]
We could also just simplify and get rid of the global altogether:
struct vb2_working_data *vb2_wd;
if (vb2_wd == NULL && cbmem_possibly_online()) vb2_wd = cbmem_find(CBMEM_ID_VBOOT_WORKBUF);
if (vb2_wd == NULL && VBOOT_PRE_RAM_SYMBOLS && pre_ram_symbols_accessible()) vb2_wd = (struct vb2_working_data *)_vboot2_work;
assert(vb2_wd != NULL);
return vb2_wd;
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 5:
(1 comment)
https://review.coreboot.org/#/c/31329/3/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/#/c/31329/3/src/security/vboot/common.c@44 PS3, Line 44: static uint8_t use_cbmem CAR_GLOBAL;
We could also just simplify and get rid of the global altogether: […]
if vb2_wd is a local variable it's value is undefined. It'd need to be marked static. That's the equivalent of a compilation unit global -- just scoped to be accessed only within the function. In both cases the object would be placed in .bss w/o decoration or .car.global with CAR_GLOBAL decoration.
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 5:
(5 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; So I think this would work because CONFIG_CACHE_AS_RAM is essentially equivalent to CONFIG_ARCH_X86 right now... but conceptually, if we want to allow possible future non-x86 architectures using CAR, I think it would make more sense to write this as:
return !CONFIG_CACHE_AS_RAM || ENV_CACHE_AS_RAM;
(I.e. it's always true on a non-CAR system, but on a CAR system it's only true if we're actually in a CAR stage.)
https://review.coreboot.org/#/c/31329/5/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/#/c/31329/5/src/security/vboot/Kconfig@97 PS5, Line 97: ARCH_X86 nit: maybe conceptually cleaner to make this CACHE_AS_RAM
https://review.coreboot.org/#/c/31329/5/src/security/vboot/Kconfig@104 PS5, Line 104: should almost always be disabled in SRAM architectures Maybe mention explicitly that SoCs which lose access to SRAM later should select it.
https://review.coreboot.org/#/c/31329/5/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/#/c/31329/5/src/security/vboot/common.c@145 PS5, Line 145: wd_cbmem = cbmem_add(CBMEM_ID_VBOOT_WORKBUF, vb2_working_data_size()); Rather then letting the STARTS_IN_ROMSTAGE also run through here, maybe it's easier to just use cbmem_add() in get_working_data() so it will create the space the first time it looks for it? cbmem_add() always does a cbmem_find() first, so that shouldn't infringe on the other cases (both add() and find() should return NULL before CBMEM is online).
https://review.coreboot.org/#/c/31329/5/src/security/vboot/symbols.h File src/security/vboot/symbols.h:
https://review.coreboot.org/#/c/31329/5/src/security/vboot/symbols.h@23 PS5, Line 23: #if IS_ENABLED(CONFIG_VBOOT_STARTS_IN_BOOTBLOCK) We don't need things like this, you can check the Kconfig directly from code. We only use helper macros and functions to wrap more complicated combinations of config options and whatever in a single conceptually tangible statement.
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 5:
(1 comment)
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@128 PS5, Line 128: pre_ram_ nit: I think "preram" without underscore has a little more precedence, e.g. in things like _preram_cbmem_console.
Hello Randall Spangler, Aaron Durbin, Julius Werner, Duncan Laurie, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31329
to look at the new patch set (#6).
Change subject: vboot: copy data structures to CBMEM for downstream use ......................................................................
vboot: copy data structures to CBMEM for downstream use
For platforms that do not employ VBOOT_STARTS_IN_ROMSTAGE, vboot verification occurs before CBMEM is brought online. In order to make vboot data structures available downstream, copy vb2_working_data from SRAM into CBMEM when CBMEM comes online.
BUG=b:124141368, b:124192753 TEST=Build locally TEST=util/lint/checkpatch.pl -g origin/master..HEAD TEST=util/abuild/abuild -B -e -y -c 50 -p none -x BRANCH=none
Change-Id: I62c11268a83927bc00ae9bd93b1b31363b38e8cf Signed-off-by: Joel Kitching kitching@google.com --- M src/commonlib/include/commonlib/cbmem_id.h M src/include/symbols.h M src/security/vboot/Kconfig M src/security/vboot/common.c M src/security/vboot/misc.h M src/security/vboot/vboot_loader.c 6 files changed, 64 insertions(+), 71 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/31329/6
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 6:
(7 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@128 PS5, Line 128: pre_ram_
nit: I think "preram" without underscore has a little more precedence, e.g. […]
Done
https://review.coreboot.org/#/c/31329/5/src/include/symbols.h@130 PS5, Line 130: return ENV_CACHE_AS_RAM || !ENV_X86;
So I think this would work because CONFIG_CACHE_AS_RAM is essentially equivalent to CONFIG_ARCH_X86 […]
So just to confirm, in SRAM architectures where we do eventually lose access to SRAM, we would never lose access while coreboot is still running?
https://review.coreboot.org/#/c/31329/5/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/#/c/31329/5/src/security/vboot/Kconfig@97 PS5, Line 97: ARCH_X86
nit: maybe conceptually cleaner to make this CACHE_AS_RAM
Done
https://review.coreboot.org/#/c/31329/5/src/security/vboot/Kconfig@104 PS5, Line 104: should almost always be disabled in SRAM architectures
Maybe mention explicitly that SoCs which lose access to SRAM later should select it.
Done
https://review.coreboot.org/#/c/31329/5/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/#/c/31329/5/src/security/vboot/common.c@145 PS5, Line 145: wd_cbmem = cbmem_add(CBMEM_ID_VBOOT_WORKBUF, vb2_working_data_size());
Rather then letting the STARTS_IN_ROMSTAGE also run through here, maybe it's easier to just use cbme […]
The trouble here is that we want to get access to the non-CBMEM version so that we can migrate the data over to CBMEM. We have access over that functionality here, since we can grab a handle to vb2_working_data before resetting vb2_wb in the last line of this function.
But you're right, it is a little convoluted right now. I've uploaded yet another iteration which exposes a new argument on vb2_get_working_data called "ignore_cbmem". This makes vb2_migrate_working_data dead-simple -- it's probably worth the trade-off of getting rid of the static vb2_wd cache.
https://review.coreboot.org/#/c/31329/6/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/#/c/31329/6/src/security/vboot/common.c@46 PS6, Line 46: /* TODO(kitching): Use VB2_WORKBUF_RECOMMENDED_SIZE instead. */ This can be removed with: https://review.coreboot.org/c/coreboot/+/31474
https://review.coreboot.org/#/c/31329/5/src/security/vboot/symbols.h File src/security/vboot/symbols.h:
https://review.coreboot.org/#/c/31329/5/src/security/vboot/symbols.h@23 PS5, Line 23: #if IS_ENABLED(CONFIG_VBOOT_STARTS_IN_BOOTBLOCK)
We don't need things like this, you can check the Kconfig directly from code. […]
OK, makes sense. I had originally used this because it appeared that IS_ENABLED was causing some trouble optimizing out the branches of code which accessed the non-existent linker script regions. But it appears I was mistaken.
Hello Randall Spangler, Aaron Durbin, Julius Werner, Duncan Laurie, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31329
to look at the new patch set (#7).
Change subject: vboot: copy data structures to CBMEM for downstream use ......................................................................
vboot: copy data structures to CBMEM for downstream use
For platforms that do not employ VBOOT_STARTS_IN_ROMSTAGE, vboot verification occurs before CBMEM is brought online. In order to make vboot data structures available downstream, copy vb2_working_data from SRAM into CBMEM when CBMEM comes online.
BUG=b:124141368, b:124192753 TEST=Build locally TEST=util/lint/checkpatch.pl -g origin/master..HEAD TEST=util/abuild/abuild -B -e -y -c 50 -p none -x BRANCH=none
Change-Id: I62c11268a83927bc00ae9bd93b1b31363b38e8cf Signed-off-by: Joel Kitching kitching@google.com --- M src/commonlib/include/commonlib/cbmem_id.h M src/include/symbols.h M src/security/vboot/Kconfig M src/security/vboot/common.c M src/security/vboot/misc.h M src/security/vboot/vboot_loader.c 6 files changed, 61 insertions(+), 71 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/31329/7
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 7:
(7 comments)
https://review.coreboot.org/#/c/31329/7/src/include/symbols.h File src/include/symbols.h:
https://review.coreboot.org/#/c/31329/7/src/include/symbols.h@127 PS7, Line 127: nit: maybe a short comment explaining what this does?
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;
So just to confirm, in SRAM architectures where we do eventually lose access to SRAM, we would never […]
No, there are different special cases. For example, Mediatek SoCs have a second half of SRAM that gets torn down during romstage (although we don't currently put the work buffer there, but if we did, that SoC would just have to select MIGRATE_WORKBUF directly).
Note that this is about whether the memory is actually accessible (i.e. whether a read32() to that address hangs your CPU). That's different from the question of whether the respective symbols are known to the linker. With the current memlayout design, that's always true on non-x86 systems unless someone puts an explicit #if ENV_xxx into memlayout.ld. For this helper function we only care about the second part.
https://review.coreboot.org/#/c/31329/5/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/#/c/31329/5/src/security/vboot/common.c@145 PS5, Line 145: wd_cbmem = cbmem_add(CBMEM_ID_VBOOT_WORKBUF, vb2_working_data_size());
The trouble here is that we want to get access to the non-CBMEM version so that we can migrate the d […]
Oh, well... why don't you just access _vboot2_work directly? At the moment we're migrating we know that we want to migrate from the pre-RAM area to the CBMEM area... there's no need to call the complicated helper function to get the "from" part and create a chicken-and-egg problem.
https://review.coreboot.org/#/c/31329/7/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/#/c/31329/7/src/security/vboot/common.c@140 PS7, Line 140: * vb2_working_data from SRAM/CAR into CBMEM. nit: maybe mention the part where it's not necessary on all platforms
https://review.coreboot.org/#/c/31329/7/src/security/vboot/common.c@145 PS7, Line 145: const struct vb2_working_data *wd = vb2_get_working_data(1); nit: "wd_preram" for clarity?
https://review.coreboot.org/#/c/31329/7/src/security/vboot/common.c@146 PS7, Line 146: struct vb2_working_data *wd_cbmem = vb2_get_working_data(0); I... guess... this works? But it still seems somewhat weird to me, honestly. I think it would be easier to just use cbmem_add() and _vboot_work directly in this function to make it absolutely clear what you're dealing with. The way you wrote it now relies on special knowledge about how your helper is implemented... but the whole point of the helper is to hide those details, and the whole point of this function here is that we know exactly which two instances of the working data we're dealing with, so we shouldn't need to use the helper to find them.
https://review.coreboot.org/#/c/31329/7/src/security/vboot/common.c@147 PS7, Line 147: printk(BIOS_INFO, "VBOOT: copying vb2_working_data to CBMEM...\n"); I'm not sure if this is really interesting enough to print. At least make it DEBUG, not INFO.
Hello Randall Spangler, Aaron Durbin, Simon Glass, Julius Werner, Duncan Laurie, Philipp Deppenwiese, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31329
to look at the new patch set (#8).
Change subject: vboot: copy data structures to CBMEM for downstream use ......................................................................
vboot: copy data structures to CBMEM for downstream use
For platforms that do not employ VBOOT_STARTS_IN_ROMSTAGE, vboot verification occurs before CBMEM is brought online. In order to make vboot data structures available downstream, copy vb2_working_data from SRAM into CBMEM when CBMEM comes online.
BUG=b:124141368, b:124192753 TEST=Build locally TEST=util/lint/checkpatch.pl -g origin/master..HEAD TEST=util/abuild/abuild -B -e -y -c 50 -p none -x BRANCH=none
Change-Id: I62c11268a83927bc00ae9bd93b1b31363b38e8cf Signed-off-by: Joel Kitching kitching@google.com --- M src/commonlib/include/commonlib/cbmem_id.h M src/include/symbols.h M src/security/vboot/Kconfig M src/security/vboot/common.c M src/security/vboot/misc.h M src/security/vboot/vboot_loader.c 6 files changed, 67 insertions(+), 71 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/31329/8
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 8: Code-Review+1
(7 comments)
https://review.coreboot.org/#/c/31329/7/src/include/symbols.h File src/include/symbols.h:
https://review.coreboot.org/#/c/31329/7/src/include/symbols.h@127 PS7, Line 127:
nit: maybe a short comment explaining what this does?
Done
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;
No, there are different special cases. […]
Just to confirm, we are using the word "accessible" in two different senses: (1) symbols accessible: symbols are known to the linker (2) memory accessible: a read32() to the address does not hang the CPU
This function is concerned with (1), thus we don't need to worry about the specifics of (2) and can leave that to the user to select MIGRATE_WORKBUF if necessary.
Your example about the second half of SRAM on Mediatek SoCs helps clarify. Would that be useful to put in the HELP text for MIGRATE_WORKBUF?
https://review.coreboot.org/#/c/31329/5/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/#/c/31329/5/src/security/vboot/common.c@145 PS5, Line 145: wd_cbmem = cbmem_add(CBMEM_ID_VBOOT_WORKBUF, vb2_working_data_size());
Oh, well... […]
Done
https://review.coreboot.org/#/c/31329/7/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/#/c/31329/7/src/security/vboot/common.c@140 PS7, Line 140: * vb2_working_data from SRAM/CAR into CBMEM.
nit: maybe mention the part where it's not necessary on all platforms
Done
https://review.coreboot.org/#/c/31329/7/src/security/vboot/common.c@145 PS7, Line 145: const struct vb2_working_data *wd = vb2_get_working_data(1);
nit: "wd_preram" for clarity?
Done
https://review.coreboot.org/#/c/31329/7/src/security/vboot/common.c@146 PS7, Line 146: struct vb2_working_data *wd_cbmem = vb2_get_working_data(0);
I... guess... this works? But it still seems somewhat weird to me, honestly. […]
Yeah, fair enough. We end up with a duplicate cbmem_add call this way, but I think you're right that vb2_migrate_working_data not calling vb2_get_working_data at all is the best route.
https://review.coreboot.org/#/c/31329/7/src/security/vboot/common.c@147 PS7, Line 147: printk(BIOS_INFO, "VBOOT: copying vb2_working_data to CBMEM...\n");
I'm not sure if this is really interesting enough to print. At least make it DEBUG, not INFO.
Done
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 8: Code-Review-1
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?
Hello Randall Spangler, Aaron Durbin, Simon Glass, Julius Werner, Duncan Laurie, Philipp Deppenwiese, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31329
to look at the new patch set (#9).
Change subject: vboot: copy data structures to CBMEM for downstream use ......................................................................
vboot: copy data structures to CBMEM for downstream use
For platforms that do not employ VBOOT_STARTS_IN_ROMSTAGE, vboot verification occurs before CBMEM is brought online. In order to make vboot data structures available downstream, copy vb2_working_data from CAR/SRAM into CBMEM when CBMEM comes online.
BUG=b:124141368, b:124192753 TEST=Build locally TEST=util/lint/checkpatch.pl -g origin/master..HEAD TEST=util/abuild/abuild -B -e -y -c 50 -p none -x BRANCH=none
Change-Id: I62c11268a83927bc00ae9bd93b1b31363b38e8cf Signed-off-by: Joel Kitching kitching@google.com --- M src/commonlib/include/commonlib/cbmem_id.h M src/include/symbols.h M src/security/vboot/Kconfig M src/security/vboot/common.c M src/security/vboot/misc.h M src/security/vboot/vboot_loader.c M src/soc/intel/skylake/Kconfig 7 files changed, 77 insertions(+), 73 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/31329/9
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...
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 9:
(2 comments)
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@152 PS8, Line 152: vb2_working_data_size
Sure, but this function doesn't have access to vb2_context (that's stored in verstage_main). […]
I think the solution here was to store workbuf_used (just that, not the whole context) in struct vb2_working_data. You'll also need that later when you pass vb2_working_data to depthcharge and try to recreate the workbuffer there (unless you wanna copy around the whole 12KB there again, I guess...).
https://review.coreboot.org/#/c/31329/9/src/soc/intel/skylake/Kconfig File src/soc/intel/skylake/Kconfig:
https://review.coreboot.org/#/c/31329/9/src/soc/intel/skylake/Kconfig@110 PS9, Line 110: select VBOOT_STARTS_IN_ROMSTAGE I don't think you want to commit this?
Hello Randall Spangler, Patrick Rudolph, Aaron Durbin, Simon Glass, Julius Werner, Duncan Laurie, Philipp Deppenwiese, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31329
to look at the new patch set (#10).
Change subject: vboot: copy data structures to CBMEM for downstream use ......................................................................
vboot: copy data structures to CBMEM for downstream use
For platforms that do not employ VBOOT_STARTS_IN_ROMSTAGE, vboot verification occurs before CBMEM is brought online. In order to make vboot data structures available downstream, copy vb2_working_data from CAR/SRAM into CBMEM when CBMEM comes online. Create VBOOT_MIGRATE_WORKING_DATA config option to toggle this functionality.
BUG=b:124141368, b:124192753 TEST=Built and deployed on eve with STARTS_IN_BOOTBLOCK TEST=Built and deployed on eve with STARTS_IN_ROMSTAGE TEST=util/lint/checkpatch.pl -g origin/master..HEAD TEST=util/abuild/abuild -B -e -y -c 50 -p none -x BRANCH=none
Change-Id: I62c11268a83927bc00ae9bd93b1b31363b38e8cf Signed-off-by: Joel Kitching kitching@google.com --- M src/commonlib/include/commonlib/cbmem_id.h M src/include/symbols.h M src/security/vboot/Kconfig M src/security/vboot/common.c M src/security/vboot/misc.h M src/security/vboot/vboot_loader.c M src/security/vboot/vboot_logic.c 7 files changed, 87 insertions(+), 69 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/31329/10
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: Code-Review+1
(2 comments)
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@152 PS8, Line 152: vb2_working_data_size
I think the solution here was to store workbuf_used (just that, not the whole context) in struct vb2 […]
Done. But now we're basically under contract to never modify workbuf after verstage finishes executing.
https://review.coreboot.org/#/c/31329/9/src/soc/intel/skylake/Kconfig File src/soc/intel/skylake/Kconfig:
https://review.coreboot.org/#/c/31329/9/src/soc/intel/skylake/Kconfig@110 PS9, Line 110: select VBOOT_STARTS_IN_ROMSTAGE
I don't think you want to commit this?
Well, it would be an interesting experiment xD
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 10: Code-Review+1
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 10:
(4 comments)
https://review.coreboot.org/#/c/31329/10/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/#/c/31329/10/src/security/vboot/common.c@78 PS10, Line 78: size_t work_size; nit: this variable is a bit pointless now
https://review.coreboot.org/#/c/31329/10/src/security/vboot/common.c@98 PS10, Line 98: end nit: maybe it's just me but "end" sounds weird. How about "finalize"?
https://review.coreboot.org/#/c/31329/10/src/security/vboot/common.c@162 PS10, Line 162: cbmem_size); I think you need to adjust buffer_size as well? In fact, I'm not sure if having both a buffer_size and buffer_used is necessary... maybe it's good enough to just shrink buffer_size after we're done with the context?
https://review.coreboot.org/#/c/31329/10/src/security/vboot/vboot_loader.c File src/security/vboot/vboot_loader.c:
https://review.coreboot.org/#/c/31329/10/src/security/vboot/vboot_loader.c@1... PS10, Line 132: if (ENV_ROMSTAGE && CONFIG(VBOOT_STARTS_IN_ROMSTAGE)) { nit: could lose the braces now
Hello Randall Spangler, Patrick Rudolph, Aaron Durbin, Simon Glass, Julius Werner, Duncan Laurie, Philipp Deppenwiese, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31329
to look at the new patch set (#11).
Change subject: vboot: copy data structures to CBMEM for downstream use ......................................................................
vboot: copy data structures to CBMEM for downstream use
For platforms that do not employ VBOOT_STARTS_IN_ROMSTAGE, vboot verification occurs before CBMEM is brought online. In order to make vboot data structures available downstream, copy vb2_working_data from CAR/SRAM into CBMEM when CBMEM comes online. Create VBOOT_MIGRATE_WORKING_DATA config option to toggle this functionality.
BUG=b:124141368, b:124192753 TEST=Built and deployed on eve with STARTS_IN_BOOTBLOCK TEST=Built and deployed on eve with STARTS_IN_ROMSTAGE TEST=util/lint/checkpatch.pl -g origin/master..HEAD TEST=util/abuild/abuild -B -e -y -c 50 -p none -x BRANCH=none
Change-Id: I62c11268a83927bc00ae9bd93b1b31363b38e8cf Signed-off-by: Joel Kitching kitching@google.com --- M src/commonlib/include/commonlib/cbmem_id.h M src/include/symbols.h M src/security/vboot/Kconfig M src/security/vboot/common.c M src/security/vboot/misc.h M src/security/vboot/vboot_loader.c M src/security/vboot/vboot_logic.c 7 files changed, 93 insertions(+), 73 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/31329/11
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 11: Code-Review+1
(4 comments)
https://review.coreboot.org/#/c/31329/10/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/#/c/31329/10/src/security/vboot/common.c@78 PS10, Line 78: size_t work_size;
nit: this variable is a bit pointless now
Done
https://review.coreboot.org/#/c/31329/10/src/security/vboot/common.c@98 PS10, Line 98: end
nit: maybe it's just me but "end" sounds weird. […]
Done
https://review.coreboot.org/#/c/31329/10/src/security/vboot/common.c@162 PS10, Line 162: cbmem_size);
I think you need to adjust buffer_size as well? In fact, I'm not sure if having both a buffer_size a […]
Good point... Let's just do the "shrinking" strategy as you suggested. No need for buffer_used.
https://review.coreboot.org/#/c/31329/10/src/security/vboot/vboot_loader.c File src/security/vboot/vboot_loader.c:
https://review.coreboot.org/#/c/31329/10/src/security/vboot/vboot_loader.c@1... PS10, Line 132: if (ENV_ROMSTAGE && CONFIG(VBOOT_STARTS_IN_ROMSTAGE)) {
nit: could lose the braces now
Done
Hello Randall Spangler, Patrick Rudolph, Aaron Durbin, Simon Glass, Julius Werner, Duncan Laurie, Philipp Deppenwiese, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31329
to look at the new patch set (#12).
Change subject: vboot: copy data structures to CBMEM for downstream use ......................................................................
vboot: copy data structures to CBMEM for downstream use
For platforms that do not employ VBOOT_STARTS_IN_ROMSTAGE, vboot verification occurs before CBMEM is brought online. In order to make vboot data structures available downstream, copy vb2_working_data from CAR/SRAM into CBMEM when CBMEM comes online. Create VBOOT_MIGRATE_WORKING_DATA config option to toggle this functionality.
BUG=b:124141368, b:124192753 TEST=Built and deployed on eve with STARTS_IN_BOOTBLOCK TEST=Built and deployed on eve with STARTS_IN_ROMSTAGE TEST=util/lint/checkpatch.pl -g origin/master..HEAD TEST=util/abuild/abuild -B -e -y -c 50 -p none -x BRANCH=none
Change-Id: I62c11268a83927bc00ae9bd93b1b31363b38e8cf Signed-off-by: Joel Kitching kitching@google.com --- M src/commonlib/include/commonlib/cbmem_id.h M src/include/symbols.h M src/security/vboot/Kconfig M src/security/vboot/common.c M src/security/vboot/misc.h M src/security/vboot/vboot_loader.c M src/security/vboot/vboot_logic.c 7 files changed, 94 insertions(+), 73 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/31329/12
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 12: Code-Review+2
I think we're good here, thanks!
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31329 )
Change subject: vboot: copy data structures to CBMEM for downstream use ......................................................................
vboot: copy data structures to CBMEM for downstream use
For platforms that do not employ VBOOT_STARTS_IN_ROMSTAGE, vboot verification occurs before CBMEM is brought online. In order to make vboot data structures available downstream, copy vb2_working_data from CAR/SRAM into CBMEM when CBMEM comes online. Create VBOOT_MIGRATE_WORKING_DATA config option to toggle this functionality.
BUG=b:124141368, b:124192753 TEST=Built and deployed on eve with STARTS_IN_BOOTBLOCK TEST=Built and deployed on eve with STARTS_IN_ROMSTAGE TEST=util/lint/checkpatch.pl -g origin/master..HEAD TEST=util/abuild/abuild -B -e -y -c 50 -p none -x BRANCH=none
Change-Id: I62c11268a83927bc00ae9bd93b1b31363b38e8cf Signed-off-by: Joel Kitching kitching@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/31329 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Julius Werner jwerner@chromium.org --- M src/commonlib/include/commonlib/cbmem_id.h M src/include/symbols.h M src/security/vboot/Kconfig M src/security/vboot/common.c M src/security/vboot/misc.h M src/security/vboot/vboot_loader.c M src/security/vboot/vboot_logic.c 7 files changed, 94 insertions(+), 73 deletions(-)
Approvals: build bot (Jenkins): Verified Julius Werner: Looks good to me, approved
diff --git a/src/commonlib/include/commonlib/cbmem_id.h b/src/commonlib/include/commonlib/cbmem_id.h index 2e0eeb6..af79a59 100644 --- a/src/commonlib/include/commonlib/cbmem_id.h +++ b/src/commonlib/include/commonlib/cbmem_id.h @@ -67,7 +67,7 @@ #define CBMEM_ID_TIMESTAMP 0x54494d45 #define CBMEM_ID_TPM2_TCG_LOG 0x54504d32 #define CBMEM_ID_VBOOT_HANDOFF 0x780074f0 -#define CBMEM_ID_VBOOT_SEL_REG 0x780074f1 +#define CBMEM_ID_VBOOT_SEL_REG 0x780074f1 /* deprecated */ #define CBMEM_ID_VBOOT_WORKBUF 0x78007343 #define CBMEM_ID_VPD 0x56504420 #define CBMEM_ID_WIFI_CALIBRATION 0x57494649 diff --git a/src/include/symbols.h b/src/include/symbols.h index 8786db7..c35e191 100644 --- a/src/include/symbols.h +++ b/src/include/symbols.h @@ -67,4 +67,11 @@ */ #define DECLARE_OPTIONAL_REGION(name) asm (".weak _" #name ", _e" #name)
+/* Returns true when pre-RAM symbols are known to the linker. + * (Does not necessarily mean that the memory is accessible.) */ +static inline int preram_symbols_available(void) +{ + return !IS_ENABLED(CONFIG_CACHE_AS_RAM) || ENV_CACHE_AS_RAM; +} + #endif /* __SYMBOLS_H */ diff --git a/src/security/vboot/Kconfig b/src/security/vboot/Kconfig index a382e67..ea5f2a3 100644 --- a/src/security/vboot/Kconfig +++ b/src/security/vboot/Kconfig @@ -107,6 +107,21 @@ memory initialization). This implies that vboot working data is allocated in CBMEM.
+config VBOOT_MIGRATE_WORKING_DATA + bool + default y if CACHE_AS_RAM + depends on !VBOOT_STARTS_IN_ROMSTAGE + help + In order to make vboot data structures available downstream, + migrate verified boot working data to CBMEM after CBMEM comes + online, when VBOOT_STARTS_IN_BOOTBLOCK is employed. This should + always be enabled on x86 architectures to migrate data from CAR + before losing access in ramstage, and should almost always be + disabled in SRAM architectures, where access to SRAM is usually + retained. Any SRAM platform where the original location of the + VBOOT_WORKBUF region becomes inaccessible in later stages should + manually select this option. + config VBOOT_MOCK_SECDATA bool "Mock secdata for firmware verification" default n diff --git a/src/security/vboot/common.c b/src/security/vboot/common.c index 59c830f..ade1b2c 100644 --- a/src/security/vboot/common.c +++ b/src/security/vboot/common.c @@ -15,8 +15,11 @@
#include <assert.h> #include <cbmem.h> +#include <console/console.h> #include <reset.h> +#include <stdint.h> #include <string.h> +#include <symbols.h> #include <vb2_api.h> #include <security/vboot/misc.h> #include <security/vboot/symbols.h> @@ -40,82 +43,77 @@ uint32_t buffer_size; };
-static const size_t vb_work_buf_size = 16 * KiB; - -static struct vb2_working_data * const vboot_get_working_data(void) -{ - if (CONFIG(VBOOT_STARTS_IN_ROMSTAGE)) - /* cbmem_add() does a cbmem_find() first. */ - return cbmem_add(CBMEM_ID_VBOOT_WORKBUF, vb_work_buf_size); - else - return (struct vb2_working_data *)_vboot2_work; -} - +/* TODO(kitching): Use VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE instead. */ static size_t vb2_working_data_size(void) { if (CONFIG(VBOOT_STARTS_IN_ROMSTAGE)) - return vb_work_buf_size; - else + return 12 * KiB; + + else if (CONFIG(VBOOT_STARTS_IN_BOOTBLOCK) && + preram_symbols_available()) return REGION_SIZE(vboot2_work); + + die("impossible!"); }
-static struct selected_region *vb2_selected_region(void) +static struct vb2_working_data * const vb2_get_working_data(void) { - struct selected_region *sel_reg = NULL; + struct vb2_working_data *wd = NULL;
- /* Ramstage and postcar always uses cbmem as a source of truth. */ - if (ENV_RAMSTAGE || ENV_POSTCAR) - sel_reg = cbmem_find(CBMEM_ID_VBOOT_SEL_REG); - else if (ENV_ROMSTAGE) { - /* Try cbmem first. Fall back on working data if not found. */ - sel_reg = cbmem_find(CBMEM_ID_VBOOT_SEL_REG); + if (cbmem_possibly_online()) + wd = cbmem_find(CBMEM_ID_VBOOT_WORKBUF);
- if (sel_reg == NULL) { - struct vb2_working_data *wd = vboot_get_working_data(); - sel_reg = &wd->selected_region; - } - } else { - /* Stages such as bootblock and verstage use working data. */ - struct vb2_working_data *wd = vboot_get_working_data(); - sel_reg = &wd->selected_region; - } + if (wd == NULL && CONFIG(VBOOT_STARTS_IN_BOOTBLOCK) && + preram_symbols_available()) + wd = (struct vb2_working_data *)_vboot2_work;
- return sel_reg; + assert(wd != NULL); + + return wd; }
void vb2_init_work_context(struct vb2_context *ctx) { struct vb2_working_data *wd; - size_t work_size;
- /* First initialize the working data region. */ - work_size = vb2_working_data_size(); - wd = vboot_get_working_data(); - memset(wd, 0, work_size); + /* First initialize the working data struct. */ + wd = vb2_get_working_data(); + memset(wd, 0, sizeof(struct vb2_working_data));
/* * vboot prefers 16-byte alignment. This takes away 16 bytes * from the VBOOT2_WORK region, but the vboot devs said that's okay. */ wd->buffer_offset = ALIGN_UP(sizeof(*wd), 16); - wd->buffer_size = work_size - wd->buffer_offset; + wd->buffer_size = vb2_working_data_size() - wd->buffer_offset;
/* Initialize the vb2_context. */ memset(ctx, 0, sizeof(*ctx)); ctx->workbuf = (void *)vb2_get_shared_data(); ctx->workbuf_size = wd->buffer_size; +}
+void vb2_finalize_work_context(struct vb2_context *ctx) +{ + /* + * Shrink buffer_size so that vb2_migrate_cbmem knows how much + * of vb2_working_data needs to be copied into CBMEM (if applicable), + * and so that downstream users know how much of the workbuf is + * currently used. + */ + vb2_get_working_data()->buffer_size = ctx->workbuf_used; }
struct vb2_shared_data *vb2_get_shared_data(void) { - struct vb2_working_data *wd = vboot_get_working_data(); + struct vb2_working_data *wd = vb2_get_working_data(); return (void *)((uintptr_t)wd + wd->buffer_offset); }
int vb2_get_selected_region(struct region *region) { - const struct selected_region *reg = vb2_selected_region(); + const struct selected_region *reg = + &vb2_get_working_data()->selected_region;
if (reg == NULL) return -1; @@ -131,7 +129,7 @@
void vb2_set_selected_region(const struct region *region) { - struct selected_region *reg = vb2_selected_region(); + struct selected_region *reg = &vb2_get_working_data()->selected_region;
assert(reg != NULL);
@@ -141,40 +139,41 @@
int vb2_is_slot_selected(void) { - const struct selected_region *reg = vb2_selected_region(); + struct selected_region *reg = &vb2_get_working_data()->selected_region;
assert(reg != NULL);
return reg->size > 0; }
-void vb2_store_selected_region(void) -{ - const struct vb2_working_data *wd; - struct selected_region *sel_reg; - - /* Always use the working data in this path since it's the object - * which has the result.. */ - wd = vboot_get_working_data(); - - sel_reg = cbmem_add(CBMEM_ID_VBOOT_SEL_REG, sizeof(*sel_reg)); - - assert(sel_reg != NULL); - - sel_reg->offset = wd->selected_region.offset; - sel_reg->size = wd->selected_region.size; -} - +#if CONFIG(VBOOT_MIGRATE_WORKING_DATA) /* - * For platforms that employ VBOOT_STARTS_IN_ROMSTAGE, the vboot - * verification doesn't happen until after cbmem is brought online. - * Therefore, the selected region contents would not be initialized - * so don't automatically add results when cbmem comes online. + * For platforms that do not employ VBOOT_STARTS_IN_ROMSTAGE, vboot + * verification occurs before CBMEM is brought online, using pre-RAM. + * In order to make vboot data structures available downstream, copy + * vb2_working_data from SRAM/CAR into CBMEM on platforms where this + * memory later becomes unavailable. */ -#if !CONFIG(VBOOT_STARTS_IN_ROMSTAGE) -static void vb2_store_selected_region_cbmem(int unused) +static void vb2_migrate_cbmem(int unused) { - vb2_store_selected_region(); + const struct vb2_working_data *wd_preram = + (struct vb2_working_data *)_vboot2_work; + size_t cbmem_size = wd_preram->buffer_offset + wd_preram->buffer_size; + struct vb2_working_data *wd_cbmem = + cbmem_add(CBMEM_ID_VBOOT_WORKBUF, cbmem_size); + printk(BIOS_DEBUG, + "VBOOT: copying vb2_working_data (%zu bytes) to CBMEM...\n", + cbmem_size); + memcpy(wd_cbmem, wd_preram, cbmem_size); + assert(wd_cbmem != NULL); } -ROMSTAGE_CBMEM_INIT_HOOK(vb2_store_selected_region_cbmem) +ROMSTAGE_CBMEM_INIT_HOOK(vb2_migrate_cbmem) +#elif CONFIG(VBOOT_STARTS_IN_ROMSTAGE) +static void vb2_setup_cbmem(int unused) +{ + struct vb2_working_data *wd_cbmem = + cbmem_add(CBMEM_ID_VBOOT_WORKBUF, vb2_working_data_size()); + assert(wd_cbmem != NULL); +} +ROMSTAGE_CBMEM_INIT_HOOK(vb2_setup_cbmem) #endif diff --git a/src/security/vboot/misc.h b/src/security/vboot/misc.h index b5e3fcf..f1dff61 100644 --- a/src/security/vboot/misc.h +++ b/src/security/vboot/misc.h @@ -24,6 +24,7 @@ void vboot_fill_handoff(void);
void vb2_init_work_context(struct vb2_context *ctx); +void vb2_finalize_work_context(struct vb2_context *ctx); struct vb2_shared_data *vb2_get_shared_data(void);
/* Returns 0 on success. < 0 on failure. */ @@ -32,9 +33,6 @@ int vb2_is_slot_selected(void); int vb2_logic_executed(void);
-/* Store the selected region in cbmem for later use. */ -void vb2_store_selected_region(void); - void vb2_save_recovery_reason_vbnv(void);
#endif /* __VBOOT_MISC_H__ */ diff --git a/src/security/vboot/vboot_loader.c b/src/security/vboot/vboot_loader.c index b6c216d..dd8c15c 100644 --- a/src/security/vboot/vboot_loader.c +++ b/src/security/vboot/vboot_loader.c @@ -27,6 +27,9 @@ _Static_assert(CONFIG(VBOOT_STARTS_IN_BOOTBLOCK) + CONFIG(VBOOT_STARTS_IN_ROMSTAGE) == 1, "vboot must either start in bootblock or romstage (not both!)"); +_Static_assert(CONFIG(VBOOT_STARTS_IN_BOOTBLOCK) || + !CONFIG(VBOOT_MIGRATE_WORKING_DATA), + "no need to migrate working data after CBMEM is already up!"); _Static_assert(!CONFIG(VBOOT_SEPARATE_VERSTAGE) || CONFIG(VBOOT_STARTS_IN_BOOTBLOCK), "stand-alone verstage must start in (i.e. after) bootblock"); @@ -126,10 +129,8 @@ * other platforms the vboot cbmem objects are initialized when * cbmem comes online. */ - if (ENV_ROMSTAGE && CONFIG(VBOOT_STARTS_IN_ROMSTAGE)) { - vb2_store_selected_region(); + if (ENV_ROMSTAGE && CONFIG(VBOOT_STARTS_IN_ROMSTAGE)) vboot_fill_handoff(); - } }
static int vboot_locate(struct cbfs_props *props) diff --git a/src/security/vboot/vboot_logic.c b/src/security/vboot/vboot_logic.c index 4aab795..d5bfa89 100644 --- a/src/security/vboot/vboot_logic.c +++ b/src/security/vboot/vboot_logic.c @@ -438,5 +438,6 @@
printk(BIOS_INFO, "Slot %c is selected\n", is_slot_a(&ctx) ? 'A' : 'B'); vb2_set_selected_region(region_device_region(&fw_main)); + vb2_finalize_work_context(&ctx); timestamp_add_now(TS_END_VBOOT); }
-
Aaron Durbin (Code Review) -
build bot (Jenkins) (Code Review)
-
Joel Kitching (Code Review)
-
Julius Werner (Code Review)
-
Patrick Georgi (Code Review)