Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31329 )
Change subject: vboot: copy data structures to CBMEM for downstream use ......................................................................
Patch Set 6:
(7 comments)
https://review.coreboot.org/#/c/31329/5/src/include/symbols.h File src/include/symbols.h:
https://review.coreboot.org/#/c/31329/5/src/include/symbols.h@128 PS5, Line 128: pre_ram_
nit: I think "preram" without underscore has a little more precedence, e.g. […]
Done
https://review.coreboot.org/#/c/31329/5/src/include/symbols.h@130 PS5, Line 130: return ENV_CACHE_AS_RAM || !ENV_X86;
So I think this would work because CONFIG_CACHE_AS_RAM is essentially equivalent to CONFIG_ARCH_X86 […]
So just to confirm, in SRAM architectures where we do eventually lose access to SRAM, we would never lose access while coreboot is still running?
https://review.coreboot.org/#/c/31329/5/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/#/c/31329/5/src/security/vboot/Kconfig@97 PS5, Line 97: ARCH_X86
nit: maybe conceptually cleaner to make this CACHE_AS_RAM
Done
https://review.coreboot.org/#/c/31329/5/src/security/vboot/Kconfig@104 PS5, Line 104: should almost always be disabled in SRAM architectures
Maybe mention explicitly that SoCs which lose access to SRAM later should select it.
Done
https://review.coreboot.org/#/c/31329/5/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/#/c/31329/5/src/security/vboot/common.c@145 PS5, Line 145: wd_cbmem = cbmem_add(CBMEM_ID_VBOOT_WORKBUF, vb2_working_data_size());
Rather then letting the STARTS_IN_ROMSTAGE also run through here, maybe it's easier to just use cbme […]
The trouble here is that we want to get access to the non-CBMEM version so that we can migrate the data over to CBMEM. We have access over that functionality here, since we can grab a handle to vb2_working_data before resetting vb2_wb in the last line of this function.
But you're right, it is a little convoluted right now. I've uploaded yet another iteration which exposes a new argument on vb2_get_working_data called "ignore_cbmem". This makes vb2_migrate_working_data dead-simple -- it's probably worth the trade-off of getting rid of the static vb2_wd cache.
https://review.coreboot.org/#/c/31329/6/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/#/c/31329/6/src/security/vboot/common.c@46 PS6, Line 46: /* TODO(kitching): Use VB2_WORKBUF_RECOMMENDED_SIZE instead. */ This can be removed with: https://review.coreboot.org/c/coreboot/+/31474
https://review.coreboot.org/#/c/31329/5/src/security/vboot/symbols.h File src/security/vboot/symbols.h:
https://review.coreboot.org/#/c/31329/5/src/security/vboot/symbols.h@23 PS5, Line 23: #if IS_ENABLED(CONFIG_VBOOT_STARTS_IN_BOOTBLOCK)
We don't need things like this, you can check the Kconfig directly from code. […]
OK, makes sense. I had originally used this because it appeared that IS_ENABLED was causing some trouble optimizing out the branches of code which accessed the non-existent linker script regions. But it appears I was mistaken.