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 2:
(3 comments)
+1 to Furquan's CL splitting suggestions.
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) I'd prefer if we stick to the model that memlayout sections always contain address and size, even if the size is fixed. The purpose of those files is both to define the memory and to have an easy reference for it later, and for the second purpose being able to see the size right there is very helpful. (We already have other areas like arm32 TTB that are essentially fixed but still take a size argument for this reason.)
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 believe Julius suggested that one option would be putting the constant we wish to use (VB2_WORKB […]
This shouldn't be a Kconfig anyway, because it's not intended to change. If you don't want to change vboot, just hardcode it in some coreboot header. But making a separate file in vboot (e.g. <vboot_constants.h> or something) is probably better anyway.
https://review.coreboot.org/#/c/31474/2/src/security/vboot/Makefile.inc File src/security/vboot/Makefile.inc:
https://review.coreboot.org/#/c/31474/2/src/security/vboot/Makefile.inc@28 PS2, Line 28: verstage-generic-ccopts += -D__PRE_RAM__ Huh, funny, I thought we had this already... guess we really haven't tested the STARTS_IN_ROMSTAGE much.