Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31474 )
Change subject: vboot: standardize on working data size ......................................................................
Patch Set 3:
(7 comments)
I believe we can ignore Jenkins' warnings on memlayout.h?
Currently we are getting some errors like this on compile: In file included from src/arch/x86/memlayout.ld:53: src/arch/x86/car.ld:32:1: error: macro "ALIGN" requires 2 arguments, but only 1 given VBOOT2_WORK(., 12K) ^~~~~~~~~~~~
These will be fixed once this CL is committed and upstreamed (downstreamed?): https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_referen...
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.
Done
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 […]
Yeah, I was on the fence about this. I have switched it back to including a size argument. Hopefully that should be more in line with the other TTB area you mentioned.
(But I've also changed all the 16K instances to 12K. Thus there are a few 4K gaps now. Is that important enough to mention in the ldscript?)
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
This shouldn't be a Kconfig anyway, because it's not intended to change. […]
I came up with a slightly different strategy to avoid having to directly include a "vboot constants" file: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_referen... Since we prefer that all vboot use goes through vb2_api.h, I think this is a better approach. (Of course now we have to wait for cros vboot commit, import upstream to coreboot, uprev vboot repo, yadda yadda.)
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?
Moved to https://review.coreboot.org/c/coreboot/+/31541
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... […]
Ack
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: ==
If we expect this CONFIG_VBOOT_WORKING_DATA_SIZE to always be same as VB2_WORKBUF_RECOMMENDED_SIZE, […]
Done
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: )
If we end up increasing the global config value, then the macros for REGION already take care of com […]
As per later comment by jwerner, we decide to include the number here after all.