Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36300 )
Change subject: vboot: use vboot persistent context ......................................................................
Patch Set 2:
(7 comments)
https://review.coreboot.org/c/coreboot/+/36300/1/3rdparty/blobs File 3rdparty/blobs:
https://review.coreboot.org/c/coreboot/+/36300/1/3rdparty/blobs@1 PS1, Line 1: Subproject commit 678b4c4a81069bb6e10e2e59f5374b83d727cd2b
Please update your submodules and re-push.
Done
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/bootmode... File src/security/vboot/bootmode.c:
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/bootmode... PS1, Line 30: (struct vb2_shared_data *)vboot_get_workbuf();
edit: We could also make vb2_get_sd() available as one of the NEED_VB2_INTERNALS APIs. […]
Actually, I already did for the depthcharge netboot case >< Let's use it here too.
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/common.c... PS1, Line 49: ctx
Should be no harm in marking this CAR_GLOBAL, just in case.
Sure, let's do that.
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/common.c... PS1, Line 55: if (wd->buffer_offset == ALIGN_UP(sizeof(*wd), 16)) {
I think this should be something like […]
I agree with Julius -- I'd prefer to encapsulate this in one function without having to set it up explicitly. On the other hand, this comes with a pretty big assumption, that we are only ever calling vboot_get_context() once in verstage. And we've already added a call to vboot_get_recovery_reason_shared_data() [in the latest PS], which gets called via vboot_save_recovery_reason_vbnv(), breaking this assumption.
The other option would be to add a field (flag? or we might even be able to get rid of flags with persistent context...) to vboot_working_data which is set after initialization is complete. Although this isn't much different than the current (less than ideal) solution.
How do other parts of coreboot typically deal with this situation?
For now, I think the safest thing is to explicitly initialize like Aaron suggested.
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/common.c... PS1, Line 56: assert(vb2api_reinit(vboot_get_workbuf(), &ctx));
Yes, we're moving the context inside the workbuf (so it can persist across stages). […]
Ack
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/common.c... PS1, Line 140: memcpy(wd_cbmem, wd_preram, cbmem_size);
Didn't we agree on adding a vb2api_relocate() for this, Joel? (Then the 'ctx' pointer should be glob […]
Right -- I was just unsure of what to do because of the whole inability to get workbuf_used issue above. I'll change to VB2_KERNEL_WORKBUF_RECOMMENDED_SIZE as discussed above, and use vb2api_relocate() here.
And I suppose you're right -- the ctx pointer MUST be a global.
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/misc.h File src/security/vboot/misc.h:
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/misc.h@5... PS1, Line 58: void *vboot_get_workbuf(void);
Should we rename working data to selection result for clarity? […]
Aaron, you're right that these can be pretty confusing. We do have a bit of explanation right above struct vboot_working_data. I'll re-work that comment.
On the other hand, as I mentioned in another comment, I wonder if vboot_working_data is even necessary at this point, now that vb2_context is always accessible? Is it possible to get the selected region from from vb2_context / vb2_shared_data?
Re. vboot_get_workbuf() -- changing to a local function.