Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31474 )
Change subject: vboot: standardize on working data size ......................................................................
Patch Set 2:
(6 comments)
https://review.coreboot.org/#/c/31474/2/src/arch/x86/car.ld File src/arch/x86/car.ld:
https://review.coreboot.org/#/c/31474/2/src/arch/x86/car.ld@31 PS2, Line 31: #if IS_ENABLED(CONFIG_VBOOT_STARTS_IN_BOOTBLOCK) Can we push this as a separate change to keep things clear?
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@162 PS2, Line 162: asserted minimum This is probably not relevant any more.
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 Can we avoid having this Kconfig and simply use VB2_WORKBUF_RECOMMENDED_SIZE directly since it is exported in 2api.h via vb2_api.h?
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@25 PS2, Line 25: # When VBOOT_STARTS_IN_ROMSTAGE is selected, DRAM is already up by the time Can we push this change as a separate CL just to keep things clearer?
https://review.coreboot.org/#/c/31474/1/src/security/vboot/vboot_loader.c File src/security/vboot/vboot_loader.c:
https://review.coreboot.org/#/c/31474/1/src/security/vboot/vboot_loader.c@37 PS1, Line 37: ==
Also considering making this >=, but I'm not sure if there ever would be an actual case that only a […]
If we expect this CONFIG_VBOOT_WORKING_DATA_SIZE to always be same as VB2_WORKBUF_RECOMMENDED_SIZE, is there any advantage of defining that config? Can we just re-use the VB2_WORKBUF_RECOMMENDED_SIZE from 2api.h?
https://review.coreboot.org/#/c/31474/1/src/soc/cavium/cn81xx/include/soc/me... File src/soc/cavium/cn81xx/include/soc/memlayout.ld:
https://review.coreboot.org/#/c/31474/1/src/soc/cavium/cn81xx/include/soc/me... PS1, Line 37: )
Actually, I'm still on the fence of whether hard-coding a number here is still a better option. […]
If we end up increasing the global config value, then the macros for REGION already take care of complaining about a region overlapping the previous one.
Reason I like having the size in here is just to make it easier when doing the math for different regions. But it is not very often that you would do that. So, I don't have a very strong opinion for/against this.