Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31474 )
Change subject: vboot: standardize on working data size ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/#/c/31474/2/src/include/memlayout.h File src/include/memlayout.h:
https://review.coreboot.org/#/c/31474/2/src/include/memlayout.h@164 PS2, Line 164: REGION(vboot2_work, addr, CONFIG_VBOOT_WORKING_DATA_SIZE, 16)
Yeah, I was on the fence about this. I have switched it back to including a size argument. […]
I mean, you can put a quick /* 4K gap */ comment in there to make it easier to notice if you want, but in general there's nothing wrong with having gaps.
https://review.coreboot.org/#/c/31474/2/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/#/c/31474/2/src/security/vboot/Kconfig@95 PS2, Line 95: VBOOT_WORKING_DATA_SIZE
I came up with a slightly different strategy to avoid having to directly include a "vboot constants" […]
Yeah... like I mentioned on the vboot CL I really don't think that's very clean.
Where does the "everything goes through vb2_api.h" directive come from? I mean, I understand in general that we want to have all normal APIs grouped under a single file (and we can still preserve the "vb2_api.h is all you need to include" aspect by just making it chain-include the constants file), but if there's a good reason to separate something out I don't see why we should forbid it. (The same goes for that NEED_SHA2_WHATEVER_IT_WAS that seems to have been added there recently... why do we need to make that so weird? What would be so wrong about just having vb2_api.h for the general high-level verification APIs and a vb2_cryto.h or something like that for the low-level crypto primitives in case callers want to use those directly?)
https://review.coreboot.org/#/c/31474/3/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/#/c/31474/3/src/security/vboot/common.c@43 PS3, Line 43: static const size_t vb2_wd_size = VB2_WORKBUF_RECOMMENDED_SIZE; This constant doesn't seem useful now, why not just use VB2_WORKBUF_RECOMMENDED_SIZE directly in the code?