Julius Werner 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 5:
(5 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@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 right now... but conceptually, if we want to allow possible future non-x86 architectures using CAR, I think it would make more sense to write this as:
return !CONFIG_CACHE_AS_RAM || ENV_CACHE_AS_RAM;
(I.e. it's always true on a non-CAR system, but on a CAR system it's only true if we're actually in a CAR stage.)
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
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.
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 cbmem_add() in get_working_data() so it will create the space the first time it looks for it? cbmem_add() always does a cbmem_find() first, so that shouldn't infringe on the other cases (both add() and find() should return NULL before CBMEM is online).
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. We only use helper macros and functions to wrap more complicated combinations of config options and whatever in a single conceptually tangible statement.