Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31887 )
Change subject: vboot: make vboot workbuf available to payload ......................................................................
Patch Set 7: Code-Review+1
(6 comments)
https://review.coreboot.org/#/c/31887/5/payloads/libpayload/include/sysinfo.... File payloads/libpayload/include/sysinfo.h:
https://review.coreboot.org/#/c/31887/5/payloads/libpayload/include/sysinfo.... PS5, Line 100: void *vb2_workbuf;
Don't you need to store the size somewhere as well?
Done
https://review.coreboot.org/#/c/31887/1/payloads/libpayload/libc/coreboot.c File payloads/libpayload/libc/coreboot.c:
https://review.coreboot.org/#/c/31887/1/payloads/libpayload/libc/coreboot.c@... PS1, Line 93: info->vboot_wd = (void *)(uintptr_t)vbwd->range_start;
Yeah, actually, I think that sounds like a good solution. Let's try that.
Done
https://review.coreboot.org/#/c/31887/1/src/lib/coreboot_table.c File src/lib/coreboot_table.c:
https://review.coreboot.org/#/c/31887/1/src/lib/coreboot_table.c@46 PS1, Line 46: #if CONFIG(VBOOT)
don't conditionally include this bit. It should be able to be unconditionally included.
Seems like there's a whole bunch of conditional includes above this one... has the policy changed? Or am I misunderstanding something?
https://review.coreboot.org/#/c/31887/1/src/lib/coreboot_table.c@562 PS1, Line 562: lb_vboot_wd(head);
This shouldn't be within #if CHROMEOS. It should be based on if (CONFIG(VBOOT)) -- c runtime.
Done -- but this means also changing lb_vboot_handoff.
https://review.coreboot.org/#/c/31887/5/src/lib/coreboot_table.c File src/lib/coreboot_table.c:
https://review.coreboot.org/#/c/31887/5/src/lib/coreboot_table.c@212 PS5, Line 212: #if CONFIG(VBOOT)
Do we still need this guard? Can we maybe get rid of it if we define some of the vboot_xxx() functio […]
I don't think that strategy quite works here. Say we put the following in src/include/boot/coreboot_tables.h:
#if CONFIG(VBOOT) #define lb_vboot_handoff() 0 #define lb_vboot_workbuf() 0 #endif /* CONFIG_VBOOT */
If we remove the guards around the function definitions in this file, the macro will attempt to expand the lb_vboot_handoff and lb_vboot_workbuff symbols in the function definitions, leading to compilation errors.
That said, since the actual calls to these two functions themselves are in a CONFIG(VBOOT) conditional, if we leave out the guard, and these functions are not called, they will be removed somewhere in the build process, correct?
https://review.coreboot.org/#/c/31887/5/src/lib/coreboot_table.c@560 PS5, Line 560: lb_vboot_handoff(head);
Maybe keep this where it is because we want to get rid of it very soon anyway?
Sure.