Aaron Durbin 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/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.
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?
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. The code should do the initialization once, the first time vboot is invoked from vboot_logic. That's what vboot_init_work_context was already doing. You could change the API to return the ctx pointer and then rely on the integrity of the object afterwards in the new vboot_get_context().
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. Where is the storage for ctx coming from? Allocated out of the work buf? Which vboot CL is providing the vb2api_reinit() ?
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/common.c... PS1, Line 131: /* TODO: We don't know the workbuf size with the current API... */ It is the size provided by the macro below.
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); I think we could use some comments here to explain the differences between:
working data context workbuf
Should we rename working data to selection result for clarity?