Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36300 )
Change subject: vboot: use vboot persistent context ......................................................................
Patch Set 1:
(6 comments)
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(); I'd prefer to do the same thing vboot does here (container_of(vboot_get_context(), vb2_shared_data, ctx)), rather than magically relying on shared data being the first thing in the workbuf.
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
Are all the platforms using vboot able to use static variables in their earlier stages?
Should be no harm in marking this CAR_GLOBAL, just in case.
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)) {
We shouldn't be dereferencing fields that may be uninitialized. […]
I think this should be something like
if (!verification_should_run()) { assert(vboot_logic_executed()); assert(vb2api_reinit(vboot_get_workbuf(), car_get_var_ptr(&ctx)) == VB2_SUCCESS); return ctx; }
...the stuff that was formerly vboot_init_work_context()...
That way it should always get set up the first time it's used during verstage code, without having to worry about where to set it up explicitly.
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/common.c... PS1, Line 56: assert(vb2api_reinit(vboot_get_workbuf(), &ctx));
struct vboot_working_data isn't currently including the size of the vb2_context object. […]
Yes, we're moving the context inside the workbuf (so it can persist across stages). See CL:1716351 and go/vboot2-persistent-context.
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 global, not static local, so that it can be updated here.)
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?
Well, working data includes the workbuf as well. It's really more of a workbuf wrapper. Not sure if vboot_get_workbuf_wrapper() would be the best way to differentiate these? (We also have a few VBOOT_WD flags in there right now, which can be eliminated once this CL is in, but that should probably be left for a follow-up patch.)
Why is vboot_get_workbuf() even exported? Can't we keep it local to common.c?