Joel Kitching has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31474
Change subject: vboot: standardize on working data size ......................................................................
vboot: standardize on working data size
Previously, the size of memory made for vb2_working_data through the macro VBOOT2_WORK was always specified in each individual memlayout file. However, there is effectively no reason to provide this customizability -- the workbuf size required for verifying firmware has never been more than 12K. (This could potentially increase in the future if key sizes or algorithms are changed, but this could be applied globally rather than for each individual platform.)
This CL creates a new Kconfig option called VBOOT_WORKING_DATA_SIZE, which is tied to the value provided by vboot2 as VB2_WORKBUF_RECOMMENDED_DATA_SIZE via a _Static_assert in vboot_loader.c
BUG=b:124141368, b:124192753 TEST=Build locally for eve TEST=util/lint/checkpatch.pl -g origin/master..HEAD TEST=util/abuild/abuild -B -e -y -c 50 -p none -x TEST=make clean && make test-abuild BRANCH=none
Change-Id: Id71a8ab2401efcc0194d48c8af9017fc90513cb8 Signed-off-by: Joel Kitching kitching@google.com --- M src/arch/x86/car.ld M src/include/memlayout.h M src/security/vboot/Kconfig M src/security/vboot/Makefile.inc M src/security/vboot/common.c M src/security/vboot/vboot_loader.c M src/soc/cavium/cn81xx/include/soc/memlayout.ld M src/soc/imgtec/pistachio/include/soc/memlayout.ld M src/soc/mediatek/mt8173/include/soc/memlayout.ld M src/soc/mediatek/mt8183/include/soc/memlayout.ld M src/soc/nvidia/tegra124/include/soc/memlayout.ld M src/soc/nvidia/tegra210/include/soc/memlayout.ld M src/soc/qualcomm/ipq40xx/include/soc/memlayout.ld M src/soc/qualcomm/ipq806x/include/soc/memlayout.ld M src/soc/qualcomm/sdm845/include/soc/memlayout.ld M src/soc/rockchip/rk3288/include/soc/memlayout.ld M src/soc/rockchip/rk3399/include/soc/memlayout.ld M src/soc/samsung/exynos5250/include/soc/memlayout.ld 18 files changed, 40 insertions(+), 36 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/31474/1
diff --git a/src/arch/x86/car.ld b/src/arch/x86/car.ld index 6fe8f14..b8d8c03 100644 --- a/src/arch/x86/car.ld +++ b/src/arch/x86/car.ld @@ -26,11 +26,11 @@ . += 4096 * CONFIG_NUM_CAR_PAGE_TABLE_PAGES; _epagetables = . ; #endif - /* Vboot work buffer is completely volatile outside of verstage and - * romstage. Appropriate code needs to handle the transition. */ -#if IS_ENABLED(CONFIG_VBOOT_SEPARATE_VERSTAGE) - VBOOT2_WORK(., 16K) -#endif + /* Vboot work buffer only needs to be available when verified boot + * starts in bootblock. */ +#if IS_ENABLED(CONFIG_VBOOT_STARTS_IN_BOOTBLOCK) + VBOOT2_WORK(.) + #endif /* Stack for CAR stages. Since it persists across all stages that * use CAR it can be reused. The chipset/SoC is expected to provide * the stack size. */ diff --git a/src/include/memlayout.h b/src/include/memlayout.h index a8b3a60..4fa1802 100644 --- a/src/include/memlayout.h +++ b/src/include/memlayout.h @@ -160,9 +160,8 @@ /* Careful: required work buffer size depends on RW properties such as key size * and algorithm -- what works for you might stop working after an update. Do * NOT lower the asserted minimum without consulting vboot devs (rspangler)! */ -#define VBOOT2_WORK(addr, size) \ - REGION(vboot2_work, addr, size, 16) \ - _ = ASSERT(size >= 12K, "vboot2 work buffer must be at least 12K!"); +#define VBOOT2_WORK(addr) \ + REGION(vboot2_work, addr, CONFIG_VBOOT_WORKING_DATA_SIZE, 16)
#if ENV_VERSTAGE #define VERSTAGE(addr, sz) \ diff --git a/src/security/vboot/Kconfig b/src/security/vboot/Kconfig index a3e9b86..1b34c91 100644 --- a/src/security/vboot/Kconfig +++ b/src/security/vboot/Kconfig @@ -92,6 +92,13 @@ memory initialization). This implies that vboot working data is allocated in CBMEM.
+config VBOOT_WORKING_DATA_SIZE + int + default 12288 # 12K, must match VB2_WORKBUF_RECOMMENDED_SIZE + help + Size in bytes that should be allocated for verified boot's working + data structure. + config VBOOT_MOCK_SECDATA bool "Mock secdata for firmware verification" default n diff --git a/src/security/vboot/Makefile.inc b/src/security/vboot/Makefile.inc index 6c63f7b..d1e2fe4 100644 --- a/src/security/vboot/Makefile.inc +++ b/src/security/vboot/Makefile.inc @@ -22,7 +22,13 @@ verstage-y += bootmode.c postcar-y += bootmode.c
-verstage-generic-ccopts += -D__PRE_RAM__ -D__VERSTAGE__ +# When VBOOT_STARTS_IN_ROMSTAGE is selected, DRAM is already up by the time +# verstage runs. +ifneq ($(CONFIG_VBOOT_STARTS_IN_ROMSTAGE),y) +verstage-generic-ccopts += -D__PRE_RAM__ +endif + +verstage-generic-ccopts += -D__VERSTAGE__
ramstage-y += gbb.c
diff --git a/src/security/vboot/common.c b/src/security/vboot/common.c index 747644a..c3d065e 100644 --- a/src/security/vboot/common.c +++ b/src/security/vboot/common.c @@ -40,25 +40,16 @@ uint32_t buffer_size; };
-static const size_t vb_work_buf_size = 16 * KiB; - static struct vb2_working_data * const vboot_get_working_data(void) { if (IS_ENABLED(CONFIG_VBOOT_STARTS_IN_ROMSTAGE)) /* cbmem_add() does a cbmem_find() first. */ - return cbmem_add(CBMEM_ID_VBOOT_WORKBUF, vb_work_buf_size); + return cbmem_add(CBMEM_ID_VBOOT_WORKBUF, + CONFIG_VBOOT_WORKING_DATA_SIZE); else return (struct vb2_working_data *)_vboot2_work; }
-static size_t vb2_working_data_size(void) -{ - if (IS_ENABLED(CONFIG_VBOOT_STARTS_IN_ROMSTAGE)) - return vb_work_buf_size; - else - return _vboot2_work_size; -} - static struct selected_region *vb2_selected_region(void) { struct selected_region *sel_reg = NULL; @@ -86,19 +77,17 @@ void vb2_init_work_context(struct vb2_context *ctx) { struct vb2_working_data *wd; - size_t work_size;
/* First initialize the working data region. */ - work_size = vb2_working_data_size(); wd = vboot_get_working_data(); - memset(wd, 0, work_size); + memset(wd, 0, CONFIG_VBOOT_WORKING_DATA_SIZE);
/* * vboot prefers 16-byte alignment. This takes away 16 bytes * from the VBOOT2_WORK region, but the vboot devs said that's okay. */ wd->buffer_offset = ALIGN_UP(sizeof(*wd), 16); - wd->buffer_size = work_size - wd->buffer_offset; + wd->buffer_size = CONFIG_VBOOT_WORKING_DATA_SIZE - wd->buffer_offset;
/* Initialize the vb2_context. */ memset(ctx, 0, sizeof(*ctx)); diff --git a/src/security/vboot/vboot_loader.c b/src/security/vboot/vboot_loader.c index 75f75b5..7b0b8d8 100644 --- a/src/security/vboot/vboot_loader.c +++ b/src/security/vboot/vboot_loader.c @@ -22,6 +22,7 @@ #include <security/vboot/misc.h> #include <security/vboot/symbols.h> #include <security/vboot/vboot_common.h> +#include <vb2_api.h>
/* Ensure vboot configuration is valid: */ _Static_assert(IS_ENABLED(CONFIG_VBOOT_STARTS_IN_BOOTBLOCK) + @@ -33,6 +34,8 @@ _Static_assert(!IS_ENABLED(CONFIG_VBOOT_RETURN_FROM_VERSTAGE) || IS_ENABLED(CONFIG_VBOOT_SEPARATE_VERSTAGE), "return from verstage only makes sense for separate verstages"); +_Static_assert(CONFIG_VBOOT_WORKING_DATA_SIZE == VB2_WORKBUF_RECOMMENDED_SIZE, + "working data size must be set to VB2_WORKBUF_RECOMMENDED_SIZE");
/* The stage loading code is compiled and entered from multiple stages. The * helper functions below attempt to provide more clarity on when certain diff --git a/src/soc/cavium/cn81xx/include/soc/memlayout.ld b/src/soc/cavium/cn81xx/include/soc/memlayout.ld index 2222617..ab47f1f 100644 --- a/src/soc/cavium/cn81xx/include/soc/memlayout.ld +++ b/src/soc/cavium/cn81xx/include/soc/memlayout.ld @@ -34,7 +34,7 @@ PRERAM_CBFS_CACHE(BOOTROM_OFFSET + 0x6000, 8K) PRERAM_CBMEM_CONSOLE(BOOTROM_OFFSET + 0x8000, 8K) BOOTBLOCK(BOOTROM_OFFSET + 0x20000, 64K) - VBOOT2_WORK(BOOTROM_OFFSET + 0x30000, 12K) + VBOOT2_WORK(BOOTROM_OFFSET + 0x30000) VERSTAGE(BOOTROM_OFFSET + 0x33000, 52K) ROMSTAGE(BOOTROM_OFFSET + 0x40000, 256K)
diff --git a/src/soc/imgtec/pistachio/include/soc/memlayout.ld b/src/soc/imgtec/pistachio/include/soc/memlayout.ld index 05042ef..e0a92cf8 100644 --- a/src/soc/imgtec/pistachio/include/soc/memlayout.ld +++ b/src/soc/imgtec/pistachio/include/soc/memlayout.ld @@ -45,7 +45,7 @@ SRAM_START(0x1a000000) REGION(gram_bootblock, 0x1a000000, 28K, 1) ROMSTAGE(0x1a007000, 60K) - VBOOT2_WORK(0x1a016000, 12K) + VBOOT2_WORK(0x1a016000) PRERAM_CBFS_CACHE(0x1a019000, 48K) SRAM_END(0x1a066000)
diff --git a/src/soc/mediatek/mt8173/include/soc/memlayout.ld b/src/soc/mediatek/mt8173/include/soc/memlayout.ld index 5b92153..3705ca1 100644 --- a/src/soc/mediatek/mt8173/include/soc/memlayout.ld +++ b/src/soc/mediatek/mt8173/include/soc/memlayout.ld @@ -38,7 +38,7 @@ SRAM_L2C_END(0x00100000)
SRAM_START(0x00100000) - VBOOT2_WORK(0x00100000, 12K) + VBOOT2_WORK(0x00100000) PRERAM_CBMEM_CONSOLE(0x00103000, 16K) WATCHDOG_TOMBSTONE(0x00107000, 4) PRERAM_CBFS_CACHE(0x00107004, 16K - 4) diff --git a/src/soc/mediatek/mt8183/include/soc/memlayout.ld b/src/soc/mediatek/mt8183/include/soc/memlayout.ld index a547083..4d065c7 100644 --- a/src/soc/mediatek/mt8183/include/soc/memlayout.ld +++ b/src/soc/mediatek/mt8183/include/soc/memlayout.ld @@ -28,7 +28,7 @@ SECTIONS { SRAM_START(0x00100000) - VBOOT2_WORK(0x00100000, 12K) + VBOOT2_WORK(0x00100000) PRERAM_CBMEM_CONSOLE(0x00103000, 16K) WATCHDOG_TOMBSTONE(0x00107000, 4) PRERAM_CBFS_CACHE(0x00107004, 16K - 4) diff --git a/src/soc/nvidia/tegra124/include/soc/memlayout.ld b/src/soc/nvidia/tegra124/include/soc/memlayout.ld index 615f0563..9907afe 100644 --- a/src/soc/nvidia/tegra124/include/soc/memlayout.ld +++ b/src/soc/nvidia/tegra124/include/soc/memlayout.ld @@ -29,7 +29,7 @@ TTB(0x40000000, 16K + 32) PRERAM_CBMEM_CONSOLE(0x40004020, 8K - 32) PRERAM_CBFS_CACHE(0x40006000, 16K) - VBOOT2_WORK(0x4000A000, 16K) + VBOOT2_WORK(0x4000A000) STACK(0x4000E000, 8K) BOOTBLOCK(0x40010000, 30K) VERSTAGE(0x40017800, 72K) diff --git a/src/soc/nvidia/tegra210/include/soc/memlayout.ld b/src/soc/nvidia/tegra210/include/soc/memlayout.ld index d807c06..1521431 100644 --- a/src/soc/nvidia/tegra210/include/soc/memlayout.ld +++ b/src/soc/nvidia/tegra210/include/soc/memlayout.ld @@ -30,7 +30,7 @@ SRAM_START(0x40000000) PRERAM_CBMEM_CONSOLE(0x40000000, 4K) PRERAM_CBFS_CACHE(0x40001000, 36K) - VBOOT2_WORK(0x4000A000, 12K) + VBOOT2_WORK(0x4000A000) #if ENV_ARM64 STACK(0x4000D000, 3K) #else /* AVP gets a separate stack to avoid any chance of handoff races. */ diff --git a/src/soc/qualcomm/ipq40xx/include/soc/memlayout.ld b/src/soc/qualcomm/ipq40xx/include/soc/memlayout.ld index 5e97077..59281cc 100644 --- a/src/soc/qualcomm/ipq40xx/include/soc/memlayout.ld +++ b/src/soc/qualcomm/ipq40xx/include/soc/memlayout.ld @@ -46,7 +46,7 @@ REGION_START(wifi_imem_1, 0x0A8C0000) BOOTBLOCK(0x0A8C0000, 24K) OVERLAP_VERSTAGE_ROMSTAGE(0x0A8C6000, 64K) - VBOOT2_WORK(0x0A8D6000, 16K) + VBOOT2_WORK(0x0A8D6000) PRERAM_CBMEM_CONSOLE(0x0A8DA000, 32K) TIMESTAMP(0x0A8E2000, 1K)
diff --git a/src/soc/qualcomm/ipq806x/include/soc/memlayout.ld b/src/soc/qualcomm/ipq806x/include/soc/memlayout.ld index 6ff2b77..8ecd924 100644 --- a/src/soc/qualcomm/ipq806x/include/soc/memlayout.ld +++ b/src/soc/qualcomm/ipq806x/include/soc/memlayout.ld @@ -27,7 +27,7 @@ /* DDR(0x2A000000, 48K) */ BOOTBLOCK(0x2A00C000, 24K) OVERLAP_VERSTAGE_ROMSTAGE(0x2A012000, 64K) - VBOOT2_WORK(0x2A022000, 16K) + VBOOT2_WORK(0x2A022000) PRERAM_CBMEM_CONSOLE(0x2A026000, 32K) TIMESTAMP(0x2A02E000, 1K)
diff --git a/src/soc/qualcomm/sdm845/include/soc/memlayout.ld b/src/soc/qualcomm/sdm845/include/soc/memlayout.ld index 5d99a8e..7a6394e 100644 --- a/src/soc/qualcomm/sdm845/include/soc/memlayout.ld +++ b/src/soc/qualcomm/sdm845/include/soc/memlayout.ld @@ -35,7 +35,7 @@ REGION(fw_reserved2, 0x14800000, 0x16000, 4096) BOOTBLOCK(0x14816000, 40K) TTB(0x14820000, 56K) - VBOOT2_WORK(0x1482E000, 16K) + VBOOT2_WORK(0x1482E000) STACK(0x14832000, 16K) TIMESTAMP(0x14836000, 1K) PRERAM_CBMEM_CONSOLE(0x14836400, 32K) diff --git a/src/soc/rockchip/rk3288/include/soc/memlayout.ld b/src/soc/rockchip/rk3288/include/soc/memlayout.ld index 6320fad..de957d5 100644 --- a/src/soc/rockchip/rk3288/include/soc/memlayout.ld +++ b/src/soc/rockchip/rk3288/include/soc/memlayout.ld @@ -32,7 +32,7 @@ TTB(0xFF700000, 16K) BOOTBLOCK(0xFF704004, 20K - 4) PRERAM_CBMEM_CONSOLE(0xFF709000, 2K) - VBOOT2_WORK(0xFF709800, 12K) + VBOOT2_WORK(0xFF709800) OVERLAP_VERSTAGE_ROMSTAGE(0xFF70C800, 42K) PRERAM_CBFS_CACHE(0xFF717000, 1K) TIMESTAMP(0xFF717400, 0x180) diff --git a/src/soc/rockchip/rk3399/include/soc/memlayout.ld b/src/soc/rockchip/rk3399/include/soc/memlayout.ld index 01e352f..a3581a8 100644 --- a/src/soc/rockchip/rk3399/include/soc/memlayout.ld +++ b/src/soc/rockchip/rk3399/include/soc/memlayout.ld @@ -35,7 +35,7 @@ /* 0xFF8C2004 is the entry point address the masked ROM will jump to. */ OVERLAP_DECOMPRESSOR_VERSTAGE_ROMSTAGE(0xFF8C2004, 88K - 4) BOOTBLOCK(0xFF8D8000, 40K) - VBOOT2_WORK(0XFF8E2000, 12K) + VBOOT2_WORK(0XFF8E2000) TTB(0xFF8E5000, 24K) PRERAM_CBMEM_CONSOLE(0xFF8EB000, 8K) STACK(0xFF8ED000, 12K) diff --git a/src/soc/samsung/exynos5250/include/soc/memlayout.ld b/src/soc/samsung/exynos5250/include/soc/memlayout.ld index 4daf2b9..9bdf105 100644 --- a/src/soc/samsung/exynos5250/include/soc/memlayout.ld +++ b/src/soc/samsung/exynos5250/include/soc/memlayout.ld @@ -32,7 +32,7 @@ /* 32K hole */ TTB(0x2058000, 16K) PRERAM_CBFS_CACHE(0x205C000, 80K) - VBOOT2_WORK(0x2070000, 16K) + VBOOT2_WORK(0x2070000) STACK(0x2074000, 16K) SRAM_END(0x2078000)
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 1:
(2 comments)
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 specific platform needs more space for firmware verification?
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 ever increased the global config value, it could mess up the regions following this one if they overlap.
Any recommendations?
Hello Randall Spangler, Aaron Durbin, Julius Werner, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31474
to look at the new patch set (#2).
Change subject: vboot: standardize on working data size ......................................................................
vboot: standardize on working data size
Previously, the size of memory made for vb2_working_data through the macro VBOOT2_WORK was always specified in each individual memlayout file. However, there is effectively no reason to provide this customizability -- the workbuf size required for verifying firmware has never been more than 12K. (This could potentially increase in the future if key sizes or algorithms are changed, but this could be applied globally rather than for each individual platform.)
This CL creates a new Kconfig option called VBOOT_WORKING_DATA_SIZE, which is tied to the value provided by vboot2 as VB2_WORKBUF_RECOMMENDED_DATA_SIZE via a _Static_assert in vboot_loader.c
Additionally, this CL fixes up the logic of when to include VBOOT2_WORK symbols on x86, which are only needed when VBOOT_STARTS_IN_BOOTBLOCK is enabled.
Finally, this CL corrects the value of the __PRE_RAM__ macro in the case that VBOOT_STARTS_IN_ROMSTAGE is selected. In this case, DRAM is already up and verstage should not be considered pre-ram.
BUG=b:124141368, b:124192753 TEST=Build locally for eve TEST=util/lint/checkpatch.pl -g origin/master..HEAD TEST=util/abuild/abuild -B -e -y -c 50 -p none -x TEST=make clean && make test-abuild BRANCH=none
Change-Id: Id71a8ab2401efcc0194d48c8af9017fc90513cb8 Signed-off-by: Joel Kitching kitching@google.com --- M src/arch/x86/car.ld M src/include/memlayout.h M src/security/vboot/Kconfig M src/security/vboot/Makefile.inc M src/security/vboot/common.c M src/security/vboot/vboot_loader.c M src/soc/cavium/cn81xx/include/soc/memlayout.ld M src/soc/imgtec/pistachio/include/soc/memlayout.ld M src/soc/mediatek/mt8173/include/soc/memlayout.ld M src/soc/mediatek/mt8183/include/soc/memlayout.ld M src/soc/nvidia/tegra124/include/soc/memlayout.ld M src/soc/nvidia/tegra210/include/soc/memlayout.ld M src/soc/qualcomm/ipq40xx/include/soc/memlayout.ld M src/soc/qualcomm/ipq806x/include/soc/memlayout.ld M src/soc/qualcomm/sdm845/include/soc/memlayout.ld M src/soc/rockchip/rk3288/include/soc/memlayout.ld M src/soc/rockchip/rk3399/include/soc/memlayout.ld M src/soc/samsung/exynos5250/include/soc/memlayout.ld 18 files changed, 40 insertions(+), 36 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/31474/2
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.
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 2:
(1 comment)
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 ex […]
Hi Furquan, This was my original thought, however I realized it's quite difficult since including vb2_api.h also seems to include stuff that messes up the linker. The first problem I encountered was the definition of ALIGN (which needs to be left alone), which I solved by #undef right after the #include. The second one simply caused a syntax error in a .ld file. I believe Julius suggested that one option would be putting the constant we wish to use (VB2_WORKBUF_RECCOMEDED_SIZE) in a separate header file and including that header directly. Would that be a preferable option?
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:
(1 comment)
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_WORKBUF_RECCOMEDED_SIZE) in a separate header file and including that header directly. Would that be a preferable option?
Yeah that sounds like a good option -- especially because it ensures that we don't get out of sync with vboot. If it is too much work, we can go ahead with this config, but it would be good to at least add a comment in vboot indicating any firmware component relying on the value of VB2_WORKBUF_RECCOMEDED_SIZE should be updated any time vboot macro is updated.
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.
Hello Randall Spangler, Aaron Durbin, Julius Werner, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31474
to look at the new patch set (#3).
Change subject: vboot: standardize on working data size ......................................................................
vboot: standardize on working data size
Previously, the size of memory made for vb2_working_data through the macro VBOOT2_WORK was always specified in each individual memlayout file. However, there is effectively no reason to provide this customizability -- the workbuf size required for verifying firmware has never been more than 12K. (This could potentially increase in the future if key sizes or algorithms are changed, but this could be applied globally rather than for each individual platform.)
This CL binds the VBOOT2_WORK macro to directly use the VB2_WORKBUF_RECOMMENDED_DATA_SIZE constant as defined by vboot API. Note that we must employ the MINIMAL_VB2_CONSTANTS switch to ensure that the vboot API does not interfere with the memlayout namespace.
BUG=b:124141368, b:124192753 TEST=Build locally for eve TEST=util/lint/checkpatch.pl -g origin/master..HEAD TEST=util/abuild/abuild -B -e -y -c 50 -p none -x TEST=make clean && make test-abuild BRANCH=none
Change-Id: Id71a8ab2401efcc0194d48c8af9017fc90513cb8 Signed-off-by: Joel Kitching kitching@google.com --- M src/arch/x86/car.ld M src/include/memlayout.h M src/security/vboot/common.c M src/soc/nvidia/tegra124/include/soc/memlayout.ld M src/soc/qualcomm/ipq40xx/include/soc/memlayout.ld M src/soc/qualcomm/ipq806x/include/soc/memlayout.ld M src/soc/qualcomm/sdm845/include/soc/memlayout.ld M src/soc/samsung/exynos5250/include/soc/memlayout.ld 8 files changed, 19 insertions(+), 26 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/31474/3
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31474 )
Change subject: vboot: standardize on working data size ......................................................................
Patch Set 3:
(4 comments)
https://review.coreboot.org/#/c/31474/3/src/include/memlayout.h File src/include/memlayout.h:
https://review.coreboot.org/#/c/31474/3/src/include/memlayout.h@164 PS3, Line 164: #define VBOOT2_WORK(addr, sz) \ Macros with multiple statements should be enclosed in a do - while loop
https://review.coreboot.org/#/c/31474/3/src/include/memlayout.h@164 PS3, Line 164: #define VBOOT2_WORK(addr, sz) \ macros should not use a trailing semicolon
https://review.coreboot.org/#/c/31474/3/src/include/memlayout.h@168 PS3, Line 168: VB2_WORKBUF_RECOMMENDED_SIZE! (sz))); space required before that '!' (ctx:VxW)
https://review.coreboot.org/#/c/31474/3/src/include/memlayout.h@168 PS3, Line 168: VB2_WORKBUF_RECOMMENDED_SIZE! (sz))); space prohibited after that '!' (ctx:VxW)
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.
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 3:
(3 comments)
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)
Yeah, I was on the fence about this. I have switched it back to including a size argument. […]
I mean, you can put a quick /* 4K gap */ comment in there to make it easier to notice if you want, but in general there's nothing wrong with having gaps.
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 came up with a slightly different strategy to avoid having to directly include a "vboot constants" […]
Yeah... like I mentioned on the vboot CL I really don't think that's very clean.
Where does the "everything goes through vb2_api.h" directive come from? I mean, I understand in general that we want to have all normal APIs grouped under a single file (and we can still preserve the "vb2_api.h is all you need to include" aspect by just making it chain-include the constants file), but if there's a good reason to separate something out I don't see why we should forbid it. (The same goes for that NEED_SHA2_WHATEVER_IT_WAS that seems to have been added there recently... why do we need to make that so weird? What would be so wrong about just having vb2_api.h for the general high-level verification APIs and a vb2_cryto.h or something like that for the low-level crypto primitives in case callers want to use those directly?)
https://review.coreboot.org/#/c/31474/3/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/#/c/31474/3/src/security/vboot/common.c@43 PS3, Line 43: static const size_t vb2_wd_size = VB2_WORKBUF_RECOMMENDED_SIZE; This constant doesn't seem useful now, why not just use VB2_WORKBUF_RECOMMENDED_SIZE directly in the code?
Hello Randall Spangler, Aaron Durbin, Simon Glass, Julius Werner, Philipp Deppenwiese, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31474
to look at the new patch set (#4).
Change subject: vboot: standardize on working data size ......................................................................
vboot: standardize on working data size
Previously, the size of memory made for vb2_working_data through the macro VBOOT2_WORK was always specified in each individual memlayout file. However, there is effectively no reason to provide this customizability -- the workbuf size required for verifying firmware has never been more than 12K. (This could potentially increase in the future if key sizes or algorithms are changed, but this could be applied globally rather than for each individual platform.)
This CL binds the VBOOT2_WORK macro to directly use the VB2_WORKBUF_RECOMMENDED_DATA_SIZE constant as defined by vboot API. Directly include the vb2_workbuf_size.h file to ensure that the vboot API does not interfere with the memlayout namespace.
BUG=b:124141368, b:124192753 TEST=Build locally for eve TEST=util/lint/checkpatch.pl -g origin/master..HEAD TEST=util/abuild/abuild -B -e -y -c 50 -p none -x TEST=make clean && make test-abuild BRANCH=none
Change-Id: Id71a8ab2401efcc0194d48c8af9017fc90513cb8 Signed-off-by: Joel Kitching kitching@google.com --- M src/arch/x86/car.ld M src/include/memlayout.h M src/security/vboot/common.c M src/soc/nvidia/tegra124/include/soc/memlayout.ld M src/soc/qualcomm/ipq40xx/include/soc/memlayout.ld M src/soc/qualcomm/ipq806x/include/soc/memlayout.ld M src/soc/qualcomm/sdm845/include/soc/memlayout.ld M src/soc/samsung/exynos5250/include/soc/memlayout.ld 8 files changed, 18 insertions(+), 26 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/31474/4
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31474 )
Change subject: vboot: standardize on working data size ......................................................................
Patch Set 4:
(4 comments)
https://review.coreboot.org/#/c/31474/4/src/include/memlayout.h File src/include/memlayout.h:
https://review.coreboot.org/#/c/31474/4/src/include/memlayout.h@163 PS4, Line 163: #define VBOOT2_WORK(addr, sz) \ Macros with multiple statements should be enclosed in a do - while loop
https://review.coreboot.org/#/c/31474/4/src/include/memlayout.h@163 PS4, Line 163: #define VBOOT2_WORK(addr, sz) \ macros should not use a trailing semicolon
https://review.coreboot.org/#/c/31474/4/src/include/memlayout.h@167 PS4, Line 167: VB2_WORKBUF_RECOMMENDED_SIZE! (sz))); space required before that '!' (ctx:VxW)
https://review.coreboot.org/#/c/31474/4/src/include/memlayout.h@167 PS4, Line 167: VB2_WORKBUF_RECOMMENDED_SIZE! (sz))); space prohibited after that '!' (ctx:VxW)
Hello Randall Spangler, Aaron Durbin, Simon Glass, Julius Werner, Philipp Deppenwiese, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31474
to look at the new patch set (#5).
Change subject: vboot: standardize on working data size ......................................................................
vboot: standardize on working data size
Previously, the size of memory made for vb2_working_data through the macro VBOOT2_WORK was always specified in each individual memlayout file. However, there is effectively no reason to provide this customizability -- the workbuf size required for verifying firmware has never been more than 12K. (This could potentially increase in the future if key sizes or algorithms are changed, but this could be applied globally rather than for each individual platform.)
This CL binds the VBOOT2_WORK macro to directly use the VB2_WORKBUF_RECOMMENDED_DATA_SIZE constant as defined by vboot API. Directly include the vb2_workbuf_size.h file to ensure that the vboot API does not interfere with the memlayout namespace.
BUG=b:124141368, b:124192753 TEST=Build locally for eve TEST=util/lint/checkpatch.pl -g origin/master..HEAD TEST=util/abuild/abuild -B -e -y -c 50 -p none -x TEST=make clean && make test-abuild BRANCH=none CQ-DEPEND=CL:1504490
Change-Id: Id71a8ab2401efcc0194d48c8af9017fc90513cb8 Signed-off-by: Joel Kitching kitching@google.com --- M src/arch/x86/car.ld M src/include/memlayout.h M src/security/vboot/common.c M src/soc/nvidia/tegra124/include/soc/memlayout.ld M src/soc/qualcomm/ipq40xx/include/soc/memlayout.ld M src/soc/qualcomm/ipq806x/include/soc/memlayout.ld M src/soc/qualcomm/sdm845/include/soc/memlayout.ld M src/soc/samsung/exynos5250/include/soc/memlayout.ld 8 files changed, 18 insertions(+), 26 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/31474/5
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31474 )
Change subject: vboot: standardize on working data size ......................................................................
Patch Set 5:
(4 comments)
https://review.coreboot.org/#/c/31474/5/src/include/memlayout.h File src/include/memlayout.h:
https://review.coreboot.org/#/c/31474/5/src/include/memlayout.h@163 PS5, Line 163: #define VBOOT2_WORK(addr, sz) \ Macros with multiple statements should be enclosed in a do - while loop
https://review.coreboot.org/#/c/31474/5/src/include/memlayout.h@163 PS5, Line 163: #define VBOOT2_WORK(addr, sz) \ macros should not use a trailing semicolon
https://review.coreboot.org/#/c/31474/5/src/include/memlayout.h@167 PS5, Line 167: VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE! (sz))); space required before that '!' (ctx:VxW)
https://review.coreboot.org/#/c/31474/5/src/include/memlayout.h@167 PS5, Line 167: VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE! (sz))); space prohibited after that '!' (ctx:VxW)
Hello Randall Spangler, Aaron Durbin, Simon Glass, Julius Werner, Philipp Deppenwiese, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31474
to look at the new patch set (#6).
Change subject: vboot: standardize on working data size ......................................................................
vboot: standardize on working data size
Previously, the size of memory made for vb2_working_data through the macro VBOOT2_WORK was always specified in each individual memlayout file. However, there is effectively no reason to provide this customizability -- the workbuf size required for verifying firmware has never been more than 12K. (This could potentially increase in the future if key sizes or algorithms are changed, but this could be applied globally rather than for each individual platform.)
This CL binds the VBOOT2_WORK macro to directly use the VB2_WORKBUF_RECOMMENDED_DATA_SIZE constant as defined by vboot API. Directly include the vb2_workbuf_size.h file to ensure that the vboot API does not interfere with the memlayout namespace.
BUG=b:124141368, b:124192753 TEST=Build locally for eve TEST=util/lint/checkpatch.pl -g origin/master..HEAD TEST=util/abuild/abuild -B -e -y -c 50 -p none -x TEST=make clean && make test-abuild BRANCH=none CQ-DEPEND=CL:1504490
Change-Id: Id71a8ab2401efcc0194d48c8af9017fc90513cb8 Signed-off-by: Joel Kitching kitching@google.com --- M src/arch/x86/car.ld M src/include/memlayout.h M src/security/vboot/common.c M src/soc/nvidia/tegra124/include/soc/memlayout.ld M src/soc/qualcomm/ipq40xx/include/soc/memlayout.ld M src/soc/qualcomm/ipq806x/include/soc/memlayout.ld M src/soc/qualcomm/sdm845/include/soc/memlayout.ld M src/soc/samsung/exynos5250/include/soc/memlayout.ld 8 files changed, 19 insertions(+), 26 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/31474/6
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31474 )
Change subject: vboot: standardize on working data size ......................................................................
Patch Set 6:
(4 comments)
https://review.coreboot.org/#/c/31474/6/src/include/memlayout.h File src/include/memlayout.h:
https://review.coreboot.org/#/c/31474/6/src/include/memlayout.h@163 PS6, Line 163: #define VBOOT2_WORK(addr, sz) \ Macros with multiple statements should be enclosed in a do - while loop
https://review.coreboot.org/#/c/31474/6/src/include/memlayout.h@163 PS6, Line 163: #define VBOOT2_WORK(addr, sz) \ macros should not use a trailing semicolon
https://review.coreboot.org/#/c/31474/6/src/include/memlayout.h@167 PS6, Line 167: VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE! (sz))); space required before that '!' (ctx:VxW)
https://review.coreboot.org/#/c/31474/6/src/include/memlayout.h@167 PS6, Line 167: VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE! (sz))); space prohibited after that '!' (ctx:VxW)
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 6:
(3 comments)
Provided everyone is okay with the current method -- then we're stuck waiting for the CQ-DEPEND CL to be downstreamed through vboot git submodule.
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 mean, you can put a quick /* 4K gap */ comment in there to make it easier to notice if you want, b […]
I'll just leave it without a note.
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
Yeah... like I mentioned on the vboot CL I really don't think that's very clean. […]
Yeah, I guess as long as *which files may be included* is well-defined, we can allow more flexibility than just vb2_api.h. Which is probably necessary if we want to turn vboot_reference into the "toolkit" concept. I also asked a question on the other CL about the other NEED_SHA2_WHATEVER_IT_WAS -- I suppose you've answered it here.
https://review.coreboot.org/#/c/31474/3/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/#/c/31474/3/src/security/vboot/common.c@43 PS3, Line 43: static const size_t vb2_wd_size = VB2_WORKBUF_RECOMMENDED_SIZE;
This constant doesn't seem useful now, why not just use VB2_WORKBUF_RECOMMENDED_SIZE directly in the […]
Right, let's do that instead.
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31474 )
Change subject: vboot: standardize on working data size ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/#/c/31474/6/src/include/memlayout.h File src/include/memlayout.h:
https://review.coreboot.org/#/c/31474/6/src/include/memlayout.h@161 PS6, Line 161: should "should because if you don't, an assert will break the build" is also called "must" :-)
Hello Randall Spangler, Aaron Durbin, Simon Glass, Julius Werner, Philipp Deppenwiese, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31474
to look at the new patch set (#7).
Change subject: vboot: standardize on working data size ......................................................................
vboot: standardize on working data size
Previously, the size of memory made for vb2_working_data through the macro VBOOT2_WORK was always specified in each individual memlayout file. However, there is effectively no reason to provide this customizability -- the workbuf size required for verifying firmware has never been more than 12K. (This could potentially increase in the future if key sizes or algorithms are changed, but this could be applied globally rather than for each individual platform.)
This CL binds the VBOOT2_WORK macro to directly use the VB2_WORKBUF_RECOMMENDED_DATA_SIZE constant as defined by vboot API. Since the constant needs to be used in a linker script, we may not include the full vboot API, and must instead directly include the vb2_constants.h header.
BUG=b:124141368, b:124192753 TEST=Build locally for eve TEST=util/lint/checkpatch.pl -g origin/master..HEAD TEST=util/abuild/abuild -B -e -y -c 50 -p none -x TEST=make clean && make test-abuild BRANCH=none CQ-DEPEND=CL:1504490
Change-Id: Id71a8ab2401efcc0194d48c8af9017fc90513cb8 Signed-off-by: Joel Kitching kitching@google.com --- M src/arch/x86/car.ld M src/include/memlayout.h M src/security/vboot/common.c M src/soc/nvidia/tegra124/include/soc/memlayout.ld M src/soc/qualcomm/ipq40xx/include/soc/memlayout.ld M src/soc/qualcomm/ipq806x/include/soc/memlayout.ld M src/soc/qualcomm/sdm845/include/soc/memlayout.ld M src/soc/samsung/exynos5250/include/soc/memlayout.ld 8 files changed, 19 insertions(+), 26 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/31474/7
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31474 )
Change subject: vboot: standardize on working data size ......................................................................
Patch Set 7:
(4 comments)
https://review.coreboot.org/#/c/31474/7/src/include/memlayout.h File src/include/memlayout.h:
https://review.coreboot.org/#/c/31474/7/src/include/memlayout.h@163 PS7, Line 163: #define VBOOT2_WORK(addr, sz) \ Macros with multiple statements should be enclosed in a do - while loop
https://review.coreboot.org/#/c/31474/7/src/include/memlayout.h@163 PS7, Line 163: #define VBOOT2_WORK(addr, sz) \ macros should not use a trailing semicolon
https://review.coreboot.org/#/c/31474/7/src/include/memlayout.h@167 PS7, Line 167: VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE! (sz))); space required before that '!' (ctx:VxW)
https://review.coreboot.org/#/c/31474/7/src/include/memlayout.h@167 PS7, Line 167: VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE! (sz))); space prohibited after that '!' (ctx:VxW)
Hello Randall Spangler, Aaron Durbin, Simon Glass, Julius Werner, Philipp Deppenwiese, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31474
to look at the new patch set (#8).
Change subject: vboot: standardize on working data size ......................................................................
vboot: standardize on working data size
Previously, the size of memory made for vb2_working_data through the macro VBOOT2_WORK was always specified in each individual memlayout file. However, there is effectively no reason to provide this customizability -- the workbuf size required for verifying firmware has never been more than 12K. (This could potentially increase in the future if key sizes or algorithms are changed, but this could be applied globally rather than for each individual platform.)
This CL binds the VBOOT2_WORK macro to directly use the VB2_WORKBUF_RECOMMENDED_DATA_SIZE constant as defined by vboot API. Since the constant needs to be used in a linker script, we may not include the full vboot API, and must instead directly include the vb2_constants.h header.
BUG=b:124141368, b:124192753 TEST=Build locally for eve TEST=util/lint/checkpatch.pl -g origin/master..HEAD TEST=util/abuild/abuild -B -e -y -c 50 -p none -x TEST=make clean && make test-abuild BRANCH=none CQ-DEPEND=CL:1504490
Change-Id: Id71a8ab2401efcc0194d48c8af9017fc90513cb8 Signed-off-by: Joel Kitching kitching@google.com --- M src/arch/x86/car.ld M src/include/memlayout.h M src/security/vboot/common.c M src/soc/nvidia/tegra124/include/soc/memlayout.ld M src/soc/qualcomm/ipq40xx/include/soc/memlayout.ld M src/soc/qualcomm/ipq806x/include/soc/memlayout.ld M src/soc/qualcomm/sdm845/include/soc/memlayout.ld M src/soc/samsung/exynos5250/include/soc/memlayout.ld 8 files changed, 19 insertions(+), 26 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/31474/8
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 8: Code-Review+1
(1 comment)
https://review.coreboot.org/#/c/31474/6/src/include/memlayout.h File src/include/memlayout.h:
https://review.coreboot.org/#/c/31474/6/src/include/memlayout.h@161 PS6, Line 161: should
"should because if you don't, an assert will break the build" is also called "must" :-)
Done
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31474 )
Change subject: vboot: standardize on working data size ......................................................................
Patch Set 8:
(4 comments)
https://review.coreboot.org/#/c/31474/8/src/include/memlayout.h File src/include/memlayout.h:
https://review.coreboot.org/#/c/31474/8/src/include/memlayout.h@163 PS8, Line 163: #define VBOOT2_WORK(addr, sz) \ Macros with multiple statements should be enclosed in a do - while loop
https://review.coreboot.org/#/c/31474/8/src/include/memlayout.h@163 PS8, Line 163: #define VBOOT2_WORK(addr, sz) \ macros should not use a trailing semicolon
https://review.coreboot.org/#/c/31474/8/src/include/memlayout.h@167 PS8, Line 167: VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE! (sz))); space required before that '!' (ctx:VxW)
https://review.coreboot.org/#/c/31474/8/src/include/memlayout.h@167 PS8, Line 167: VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE! (sz))); space prohibited after that '!' (ctx:VxW)
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 8: Code-Review+2
LGTM but needs the vboot patch first, of course.
Patrick Georgi has uploaded a new patch set (#9) to the change originally created by Joel Kitching. ( https://review.coreboot.org/c/coreboot/+/31474 )
Change subject: vboot: standardize on working data size ......................................................................
vboot: standardize on working data size
Previously, the size of memory made for vb2_working_data through the macro VBOOT2_WORK was always specified in each individual memlayout file. However, there is effectively no reason to provide this customizability -- the workbuf size required for verifying firmware has never been more than 12K. (This could potentially increase in the future if key sizes or algorithms are changed, but this could be applied globally rather than for each individual platform.)
This CL binds the VBOOT2_WORK macro to directly use the VB2_WORKBUF_RECOMMENDED_DATA_SIZE constant as defined by vboot API. Since the constant needs to be used in a linker script, we may not include the full vboot API, and must instead directly include the vb2_constants.h header.
BUG=b:124141368, b:124192753 TEST=Build locally for eve TEST=util/lint/checkpatch.pl -g origin/master..HEAD TEST=util/abuild/abuild -B -e -y -c 50 -p none -x TEST=make clean && make test-abuild BRANCH=none CQ-DEPEND=CL:1504490
Change-Id: Id71a8ab2401efcc0194d48c8af9017fc90513cb8 Signed-off-by: Joel Kitching kitching@google.com --- M src/arch/x86/car.ld M src/include/memlayout.h M src/security/vboot/common.c M src/soc/nvidia/tegra124/include/soc/memlayout.ld M src/soc/qualcomm/ipq40xx/include/soc/memlayout.ld M src/soc/qualcomm/ipq806x/include/soc/memlayout.ld M src/soc/qualcomm/sdm845/include/soc/memlayout.ld M src/soc/samsung/exynos5250/include/soc/memlayout.ld 8 files changed, 19 insertions(+), 26 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/31474/9
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31474 )
Change subject: vboot: standardize on working data size ......................................................................
Patch Set 9:
(4 comments)
https://review.coreboot.org/#/c/31474/9/src/include/memlayout.h File src/include/memlayout.h:
https://review.coreboot.org/#/c/31474/9/src/include/memlayout.h@163 PS9, Line 163: #define VBOOT2_WORK(addr, sz) \ Macros with multiple statements should be enclosed in a do - while loop
https://review.coreboot.org/#/c/31474/9/src/include/memlayout.h@163 PS9, Line 163: #define VBOOT2_WORK(addr, sz) \ macros should not use a trailing semicolon
https://review.coreboot.org/#/c/31474/9/src/include/memlayout.h@167 PS9, Line 167: VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE! (sz))); space required before that '!' (ctx:VxW)
https://review.coreboot.org/#/c/31474/9/src/include/memlayout.h@167 PS9, Line 167: VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE! (sz))); space prohibited after that '!' (ctx:VxW)
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31474 )
Change subject: vboot: standardize on working data size ......................................................................
Patch Set 8:
rebased the CL, which was mostly trivial but PTAL
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31474 )
Change subject: vboot: standardize on working data size ......................................................................
Patch Set 10:
(4 comments)
https://review.coreboot.org/#/c/31474/10/src/include/memlayout.h File src/include/memlayout.h:
https://review.coreboot.org/#/c/31474/10/src/include/memlayout.h@163 PS10, Line 163: #define VBOOT2_WORK(addr, sz) \ Macros with multiple statements should be enclosed in a do - while loop
https://review.coreboot.org/#/c/31474/10/src/include/memlayout.h@163 PS10, Line 163: #define VBOOT2_WORK(addr, sz) \ macros should not use a trailing semicolon
https://review.coreboot.org/#/c/31474/10/src/include/memlayout.h@167 PS10, Line 167: VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE! (sz))); space required before that '!' (ctx:VxW)
https://review.coreboot.org/#/c/31474/10/src/include/memlayout.h@167 PS10, Line 167: VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE! (sz))); space prohibited after that '!' (ctx:VxW)
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 10: Code-Review+2
Simon Glass has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31474 )
Change subject: vboot: standardize on working data size ......................................................................
Patch Set 10: Code-Review+2
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 10: Code-Review+2
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 10: Code-Review+1
Hello Randall Spangler, Aaron Durbin, Simon Glass, Julius Werner, Philipp Deppenwiese, build bot (Jenkins), Furquan Shaikh, Simon Glass, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31474
to look at the new patch set (#11).
Change subject: vboot: standardize on working data size ......................................................................
vboot: standardize on working data size
Previously, the size of memory made for vboot_working_data through the macro VBOOT2_WORK was always specified in each individual memlayout file. However, there is effectively no reason to provide this customizability -- the workbuf size required for verifying firmware has never been more than 12K. (This could potentially increase in the future if key sizes or algorithms are changed, but this could be applied globally rather than for each individual platform.)
This CL binds the VBOOT2_WORK macro to directly use the VB2_WORKBUF_RECOMMENDED_DATA_SIZE constant as defined by vboot API. Since the constant needs to be used in a linker script, we may not include the full vboot API, and must instead directly include the vb2_constants.h header.
BUG=b:124141368, b:124192753 TEST=Build locally for eve TEST=util/lint/checkpatch.pl -g origin/master..HEAD TEST=util/abuild/abuild -B -e -y -c 50 -p none -x TEST=make clean && make test-abuild BRANCH=none CQ-DEPEND=CL:1504490
Change-Id: Id71a8ab2401efcc0194d48c8af9017fc90513cb8 Signed-off-by: Joel Kitching kitching@google.com --- M src/arch/x86/car.ld M src/include/memlayout.h M src/security/vboot/common.c M src/soc/nvidia/tegra124/include/soc/memlayout.ld M src/soc/qualcomm/ipq40xx/include/soc/memlayout.ld M src/soc/qualcomm/ipq806x/include/soc/memlayout.ld M src/soc/qualcomm/sdm845/include/soc/memlayout.ld M src/soc/samsung/exynos5250/include/soc/memlayout.ld 8 files changed, 18 insertions(+), 28 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/31474/11
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31474 )
Change subject: vboot: standardize on working data size ......................................................................
Patch Set 11:
(4 comments)
https://review.coreboot.org/#/c/31474/11/src/include/memlayout.h File src/include/memlayout.h:
https://review.coreboot.org/#/c/31474/11/src/include/memlayout.h@163 PS11, Line 163: #define VBOOT2_WORK(addr, sz) \ Macros with multiple statements should be enclosed in a do - while loop
https://review.coreboot.org/#/c/31474/11/src/include/memlayout.h@163 PS11, Line 163: #define VBOOT2_WORK(addr, sz) \ macros should not use a trailing semicolon
https://review.coreboot.org/#/c/31474/11/src/include/memlayout.h@167 PS11, Line 167: VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE! (sz))); space required before that '!' (ctx:VxW)
https://review.coreboot.org/#/c/31474/11/src/include/memlayout.h@167 PS11, Line 167: VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE! (sz))); space prohibited after that '!' (ctx:VxW)
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 11: Code-Review+1
Since the other src/security/vboot/common.c changes were submitted, this one needed a rebase. Ready for +2 and submit, please!
Simon Glass has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31474 )
Change subject: vboot: standardize on working data size ......................................................................
Patch Set 11: Code-Review+2
Looks right to me
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 11: Code-Review+2
(1 comment)
LGTM after fix
https://review.coreboot.org/#/c/31474/11/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/#/c/31474/11/src/security/vboot/common.c@166 PS11, Line 166: vboot_working_data_size Here too
Hello Randall Spangler, Aaron Durbin, Simon Glass, Julius Werner, Philipp Deppenwiese, build bot (Jenkins), Simon Glass, Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31474
to look at the new patch set (#12).
Change subject: vboot: standardize on working data size ......................................................................
vboot: standardize on working data size
Previously, the size of memory made for vboot_working_data through the macro VBOOT2_WORK was always specified in each individual memlayout file. However, there is effectively no reason to provide this customizability -- the workbuf size required for verifying firmware has never been more than 12K. (This could potentially increase in the future if key sizes or algorithms are changed, but this could be applied globally rather than for each individual platform.)
This CL binds the VBOOT2_WORK macro to directly use the VB2_WORKBUF_RECOMMENDED_DATA_SIZE constant as defined by vboot API. Since the constant needs to be used in a linker script, we may not include the full vboot API, and must instead directly include the vb2_constants.h header.
BUG=b:124141368, b:124192753 TEST=Build locally for eve TEST=util/lint/checkpatch.pl -g origin/master..HEAD TEST=util/abuild/abuild -B -e -y -c 50 -p none -x TEST=make clean && make test-abuild BRANCH=none CQ-DEPEND=CL:1504490
Change-Id: Id71a8ab2401efcc0194d48c8af9017fc90513cb8 Signed-off-by: Joel Kitching kitching@google.com --- M src/arch/x86/car.ld M src/include/memlayout.h M src/security/vboot/common.c M src/soc/nvidia/tegra124/include/soc/memlayout.ld M src/soc/qualcomm/ipq40xx/include/soc/memlayout.ld M src/soc/qualcomm/ipq806x/include/soc/memlayout.ld M src/soc/qualcomm/sdm845/include/soc/memlayout.ld M src/soc/samsung/exynos5250/include/soc/memlayout.ld 8 files changed, 20 insertions(+), 29 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/31474/12
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31474 )
Change subject: vboot: standardize on working data size ......................................................................
Patch Set 12:
(4 comments)
https://review.coreboot.org/#/c/31474/12/src/include/memlayout.h File src/include/memlayout.h:
https://review.coreboot.org/#/c/31474/12/src/include/memlayout.h@163 PS12, Line 163: #define VBOOT2_WORK(addr, sz) \ Macros with multiple statements should be enclosed in a do - while loop
https://review.coreboot.org/#/c/31474/12/src/include/memlayout.h@163 PS12, Line 163: #define VBOOT2_WORK(addr, sz) \ macros should not use a trailing semicolon
https://review.coreboot.org/#/c/31474/12/src/include/memlayout.h@167 PS12, Line 167: VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE! (sz))); space required before that '!' (ctx:VxW)
https://review.coreboot.org/#/c/31474/12/src/include/memlayout.h@167 PS12, Line 167: VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE! (sz))); space prohibited after that '!' (ctx:VxW)
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 12: Code-Review+1
(1 comment)
https://review.coreboot.org/#/c/31474/11/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/#/c/31474/11/src/security/vboot/common.c@166 PS11, Line 166: vboot_working_data_size
Here too
Done
Hello Randall Spangler, Aaron Durbin, Simon Glass, Julius Werner, Philipp Deppenwiese, build bot (Jenkins), Simon Glass, Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31474
to look at the new patch set (#13).
Change subject: vboot: standardize on working data size ......................................................................
vboot: standardize on working data size
Previously, the size of memory made for vboot_working_data through the macro VBOOT2_WORK was always specified in each individual memlayout file. However, there is effectively no reason to provide this customizability -- the workbuf size required for verifying firmware has never been more than 12K. (This could potentially increase in the future if key sizes or algorithms are changed, but this could be applied globally rather than for each individual platform.)
This CL binds the VBOOT2_WORK macro to directly use the VB2_WORKBUF_RECOMMENDED_DATA_SIZE constant as defined by vboot API. Since the constant needs to be used in a linker script, we may not include the full vboot API, and must instead directly include the vb2_constants.h header.
BUG=b:124141368, b:124192753 TEST=Build locally for eve TEST=util/lint/checkpatch.pl -g origin/master..HEAD TEST=util/abuild/abuild -B -e -y -c 50 -p none -x TEST=make clean && make test-abuild BRANCH=none CQ-DEPEND=CL:1504490
Change-Id: Id71a8ab2401efcc0194d48c8af9017fc90513cb8 Signed-off-by: Joel Kitching kitching@google.com --- M src/arch/x86/car.ld M src/include/memlayout.h M src/security/vboot/common.c M src/soc/nvidia/tegra124/include/soc/memlayout.ld M src/soc/qualcomm/ipq40xx/include/soc/memlayout.ld M src/soc/qualcomm/ipq806x/include/soc/memlayout.ld M src/soc/qualcomm/qcs405/include/soc/memlayout.ld M src/soc/qualcomm/sdm845/include/soc/memlayout.ld M src/soc/samsung/exynos5250/include/soc/memlayout.ld 9 files changed, 21 insertions(+), 30 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/31474/13
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31474 )
Change subject: vboot: standardize on working data size ......................................................................
Patch Set 13:
(4 comments)
https://review.coreboot.org/#/c/31474/13/src/include/memlayout.h File src/include/memlayout.h:
https://review.coreboot.org/#/c/31474/13/src/include/memlayout.h@163 PS13, Line 163: #define VBOOT2_WORK(addr, sz) \ Macros with multiple statements should be enclosed in a do - while loop
https://review.coreboot.org/#/c/31474/13/src/include/memlayout.h@163 PS13, Line 163: #define VBOOT2_WORK(addr, sz) \ macros should not use a trailing semicolon
https://review.coreboot.org/#/c/31474/13/src/include/memlayout.h@167 PS13, Line 167: VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE! (sz))); space required before that '!' (ctx:VxW)
https://review.coreboot.org/#/c/31474/13/src/include/memlayout.h@167 PS13, Line 167: VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE! (sz))); space prohibited after that '!' (ctx:VxW)
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 13:
aarch64-elf-ld.bfd: vboot2 work buffer size must be equivalent to VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE! (16K) make[1]: *** [src/arch/arm64/Makefile.inc:67: /cb-build/coreboot-gerrit.0/GOOGLE_MISTRAL/cbfs/fallback/bootblock.debug] Error 1 make[1]: Leaving directory '/home/coreboot/slave-root/workspace/coreboot-gerrit'
Well, looks like it's working properly =)
But I'm not so sure about these other errors:
make[2]: Entering directory '/home/coreboot/slave-root/workspace/coreboot-gerrit/src/soc/nvidia/tegra124/lp0' arm-eabi-gcc -marm -march=armv4t -mno-unaligned-access -nostdlib -static \ -Os -fpie -Wl,--build-id=none -ggdb3 -T tegra_lp0_resume.ld \ -I ../../../../include -I ../../../../arch/arm/include \ -include ../../../../commonlib/include/commonlib/compiler.h \ -o tegra_lp0_resume.elf tegra_lp0_resume.c tegra_lp0_resume.c:267:40: error: static declaration of 'halt' follows non-static declaration static __always_inline void __noreturn halt(void) ^~~~ In file included from tegra_lp0_resume.c:17: ../../../../include/halt.h:26:17: note: previous declaration of 'halt' was here void __noreturn halt(void); ^~~~ make[2]: *** [Makefile:35: tegra_lp0_resume.elf] Error 1 make[2]: Leaving directory '/home/coreboot/slave-root/workspace/coreboot-gerrit/src/soc/nvidia/tegra124/lp0'
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31474 )
Change subject: vboot: standardize on working data size ......................................................................
Patch Set 13:
Patch Set 13: But I'm not so sure about these other errors:
make[2]: Entering directory '/home/coreboot/slave-root/workspace/coreboot-gerrit/src/soc/nvidia/tegra124/lp0' arm-eabi-gcc -marm -march=armv4t -mno-unaligned-access -nostdlib -static \ -Os -fpie -Wl,--build-id=none -ggdb3 -T tegra_lp0_resume.ld \ -I ../../../../include -I ../../../../arch/arm/include \ -include ../../../../commonlib/include/commonlib/compiler.h \ -o tegra_lp0_resume.elf tegra_lp0_resume.c tegra_lp0_resume.c:267:40: error: static declaration of 'halt' follows non-static declaration static __always_inline void __noreturn halt(void) ^~~~ In file included from tegra_lp0_resume.c:17: ../../../../include/halt.h:26:17: note: previous declaration of 'halt' was here void __noreturn halt(void); ^~~~ make[2]: *** [Makefile:35: tegra_lp0_resume.elf] Error 1 make[2]: Leaving directory '/home/coreboot/slave-root/workspace/coreboot-gerrit/src/soc/nvidia/tegra124/lp0'
https://review.coreboot.org/c/coreboot/+/31979 slipped through
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31474 )
Change subject: vboot: standardize on working data size ......................................................................
Patch Set 13:
Patch Set 13:
Patch Set 13: But I'm not so sure about these other errors:
make[2]: Entering directory '/home/coreboot/slave-root/workspace/coreboot-gerrit/src/soc/nvidia/tegra124/lp0' arm-eabi-gcc -marm -march=armv4t -mno-unaligned-access -nostdlib -static \ -Os -fpie -Wl,--build-id=none -ggdb3 -T tegra_lp0_resume.ld \ -I ../../../../include -I ../../../../arch/arm/include \ -include ../../../../commonlib/include/commonlib/compiler.h \ -o tegra_lp0_resume.elf tegra_lp0_resume.c tegra_lp0_resume.c:267:40: error: static declaration of 'halt' follows non-static declaration static __always_inline void __noreturn halt(void) ^~~~ In file included from tegra_lp0_resume.c:17: ../../../../include/halt.h:26:17: note: previous declaration of 'halt' was here void __noreturn halt(void); ^~~~ make[2]: *** [Makefile:35: tegra_lp0_resume.elf] Error 1 make[2]: Leaving directory '/home/coreboot/slave-root/workspace/coreboot-gerrit/src/soc/nvidia/tegra124/lp0'
https://review.coreboot.org/c/coreboot/+/31979 slipped through
I just merged https://review.coreboot.org/c/coreboot/+/32005 which should fix it.
As for the mistral side issue, I can take care of that.
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 13:
As for the mistral side issue, I can take care of that.
Thanks, Patrick! I actually already added src/soc/qualcomm/qcs405/include/soc/memlayout.ld into the patch. Is this ready for commit now?
Hello Randall Spangler, Aaron Durbin, Simon Glass, Julius Werner, Philipp Deppenwiese, build bot (Jenkins), Patrick Georgi, Simon Glass, Furquan Shaikh, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31474
to look at the new patch set (#14).
Change subject: vboot: standardize on working data size ......................................................................
vboot: standardize on working data size
Previously, the size of memory made for vboot_working_data through the macro VBOOT2_WORK was always specified in each individual memlayout file. However, there is effectively no reason to provide this customizability -- the workbuf size required for verifying firmware has never been more than 12K. (This could potentially increase in the future if key sizes or algorithms are changed, but this could be applied globally rather than for each individual platform.)
This CL binds the VBOOT2_WORK macro to directly use the VB2_WORKBUF_RECOMMENDED_DATA_SIZE constant as defined by vboot API. Since the constant needs to be used in a linker script, we may not include the full vboot API, and must instead directly include the vb2_constants.h header.
BUG=b:124141368, b:124192753 TEST=Build locally for eve TEST=util/lint/checkpatch.pl -g origin/master..HEAD TEST=util/abuild/abuild -B -e -y -c 50 -p none -x TEST=make clean && make test-abuild BRANCH=none CQ-DEPEND=CL:1504490
Change-Id: Id71a8ab2401efcc0194d48c8af9017fc90513cb8 Signed-off-by: Joel Kitching kitching@google.com --- M src/arch/x86/car.ld M src/include/memlayout.h M src/security/vboot/common.c M src/soc/nvidia/tegra124/include/soc/memlayout.ld M src/soc/qualcomm/ipq40xx/include/soc/memlayout.ld M src/soc/qualcomm/ipq806x/include/soc/memlayout.ld M src/soc/qualcomm/qcs405/include/soc/memlayout.ld M src/soc/qualcomm/sdm845/include/soc/memlayout.ld M src/soc/samsung/exynos5250/include/soc/memlayout.ld 9 files changed, 21 insertions(+), 30 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/31474/14
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31474 )
Change subject: vboot: standardize on working data size ......................................................................
Patch Set 14:
(4 comments)
https://review.coreboot.org/#/c/31474/14/src/include/memlayout.h File src/include/memlayout.h:
https://review.coreboot.org/#/c/31474/14/src/include/memlayout.h@163 PS14, Line 163: #define VBOOT2_WORK(addr, sz) \ Macros with multiple statements should be enclosed in a do - while loop
https://review.coreboot.org/#/c/31474/14/src/include/memlayout.h@163 PS14, Line 163: #define VBOOT2_WORK(addr, sz) \ macros should not use a trailing semicolon
https://review.coreboot.org/#/c/31474/14/src/include/memlayout.h@167 PS14, Line 167: VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE! (sz))); space required before that '!' (ctx:VxW)
https://review.coreboot.org/#/c/31474/14/src/include/memlayout.h@167 PS14, Line 167: VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE! (sz))); space prohibited after that '!' (ctx:VxW)
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31474 )
Change subject: vboot: standardize on working data size ......................................................................
Patch Set 14: Code-Review+2
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31474 )
Change subject: vboot: standardize on working data size ......................................................................
vboot: standardize on working data size
Previously, the size of memory made for vboot_working_data through the macro VBOOT2_WORK was always specified in each individual memlayout file. However, there is effectively no reason to provide this customizability -- the workbuf size required for verifying firmware has never been more than 12K. (This could potentially increase in the future if key sizes or algorithms are changed, but this could be applied globally rather than for each individual platform.)
This CL binds the VBOOT2_WORK macro to directly use the VB2_WORKBUF_RECOMMENDED_DATA_SIZE constant as defined by vboot API. Since the constant needs to be used in a linker script, we may not include the full vboot API, and must instead directly include the vb2_constants.h header.
BUG=b:124141368, b:124192753 TEST=Build locally for eve TEST=util/lint/checkpatch.pl -g origin/master..HEAD TEST=util/abuild/abuild -B -e -y -c 50 -p none -x TEST=make clean && make test-abuild BRANCH=none CQ-DEPEND=CL:1504490
Change-Id: Id71a8ab2401efcc0194d48c8af9017fc90513cb8 Signed-off-by: Joel Kitching kitching@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/31474 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Patrick Georgi pgeorgi@google.com --- M src/arch/x86/car.ld M src/include/memlayout.h M src/security/vboot/common.c M src/soc/nvidia/tegra124/include/soc/memlayout.ld M src/soc/qualcomm/ipq40xx/include/soc/memlayout.ld M src/soc/qualcomm/ipq806x/include/soc/memlayout.ld M src/soc/qualcomm/qcs405/include/soc/memlayout.ld M src/soc/qualcomm/sdm845/include/soc/memlayout.ld M src/soc/samsung/exynos5250/include/soc/memlayout.ld 9 files changed, 21 insertions(+), 30 deletions(-)
Approvals: build bot (Jenkins): Verified Patrick Georgi: Looks good to me, approved
diff --git a/src/arch/x86/car.ld b/src/arch/x86/car.ld index 4637362..37fb087 100644 --- a/src/arch/x86/car.ld +++ b/src/arch/x86/car.ld @@ -29,7 +29,7 @@ /* Vboot work buffer only needs to be available when verified boot * starts in bootblock. */ #if CONFIG(VBOOT_STARTS_IN_BOOTBLOCK) - VBOOT2_WORK(., 16K) + VBOOT2_WORK(., 12K) #endif /* Vboot measured boot TCPA log measurements. * Needs to be transferred until CBMEM is available diff --git a/src/include/memlayout.h b/src/include/memlayout.h index 1ccb9f7..273a80d 100644 --- a/src/include/memlayout.h +++ b/src/include/memlayout.h @@ -19,6 +19,7 @@ #define __MEMLAYOUT_H
#include <arch/memlayout.h> +#include <vb2_constants.h>
/* Macros that the architecture can override. */ #ifndef ARCH_POINTER_ALIGN_SIZE @@ -157,12 +158,13 @@ REGION(ramstage, addr, sz, 1) #endif
-/* Careful: required work buffer size depends on RW properties such as key size - * and algorithm -- what works for you might stop working after an update. Do - * NOT lower the asserted minimum without consulting vboot devs (rspangler)! */ -#define VBOOT2_WORK(addr, size) \ - REGION(vboot2_work, addr, size, 16) \ - _ = ASSERT(size >= 12K, "vboot2 work buffer must be at least 12K!"); +/* VBOOT2_WORK must always use VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE for its + * size argument. The constant is imported via vb2_workbuf_size.h. */ +#define VBOOT2_WORK(addr, sz) \ + REGION(vboot2_work, addr, sz, 16) \ + _ = ASSERT(sz == VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE, \ + STR(vboot2 work buffer size must be equivalent to \ + VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE! (sz)));
#define VBOOT2_TPM_LOG(addr, size) \ REGION(vboot2_tpm_log, addr, size, 16) \ diff --git a/src/security/vboot/common.c b/src/security/vboot/common.c index 2348d31..5b49ebf 100644 --- a/src/security/vboot/common.c +++ b/src/security/vboot/common.c @@ -25,19 +25,6 @@ #include <security/vboot/symbols.h> #include <security/vboot/vboot_common.h>
-/* TODO(kitching): Use VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE instead. */ -static size_t vboot_working_data_size(void) -{ - if (CONFIG(VBOOT_STARTS_IN_ROMSTAGE)) - return 12 * KiB; - - else if (CONFIG(VBOOT_STARTS_IN_BOOTBLOCK) && - preram_symbols_available()) - return REGION_SIZE(vboot2_work); - - die("impossible!"); -} - struct vboot_working_data * const vboot_get_working_data(void) { struct vboot_working_data *wd = NULL; @@ -58,16 +45,17 @@ { struct vboot_working_data *wd;
- /* First initialize the working data struct. */ + /* First initialize the working data region. */ wd = vboot_get_working_data(); - memset(wd, 0, sizeof(struct vboot_working_data)); + memset(wd, 0, VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE);
/* * vboot prefers 16-byte alignment. This takes away 16 bytes * from the VBOOT2_WORK region, but the vboot devs said that's okay. */ wd->buffer_offset = ALIGN_UP(sizeof(*wd), 16); - wd->buffer_size = vboot_working_data_size() - wd->buffer_offset; + wd->buffer_size = VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE + - wd->buffer_offset;
/* Initialize the vb2_context. */ memset(ctx, 0, sizeof(*ctx)); @@ -157,7 +145,8 @@ static void vboot_setup_cbmem(int unused) { struct vboot_working_data *wd_cbmem = - cbmem_add(CBMEM_ID_VBOOT_WORKBUF, vboot_working_data_size()); + cbmem_add(CBMEM_ID_VBOOT_WORKBUF, + VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE); assert(wd_cbmem != NULL); } ROMSTAGE_CBMEM_INIT_HOOK(vboot_setup_cbmem) diff --git a/src/soc/nvidia/tegra124/include/soc/memlayout.ld b/src/soc/nvidia/tegra124/include/soc/memlayout.ld index 40af0d5..7e2f9ec 100644 --- a/src/soc/nvidia/tegra124/include/soc/memlayout.ld +++ b/src/soc/nvidia/tegra124/include/soc/memlayout.ld @@ -29,7 +29,7 @@ TTB(0x40000000, 16K + 32) PRERAM_CBMEM_CONSOLE(0x40004020, 6K - 32) PRERAM_CBFS_CACHE(0x40005800, 16K) - VBOOT2_WORK(0x40009800, 16K) + VBOOT2_WORK(0x40009800, 12K) VBOOT2_TPM_LOG(0x4000D800, 2K) STACK(0x4000E000, 8K) BOOTBLOCK(0x40010000, 30K) diff --git a/src/soc/qualcomm/ipq40xx/include/soc/memlayout.ld b/src/soc/qualcomm/ipq40xx/include/soc/memlayout.ld index 5e97077..a69b60b 100644 --- a/src/soc/qualcomm/ipq40xx/include/soc/memlayout.ld +++ b/src/soc/qualcomm/ipq40xx/include/soc/memlayout.ld @@ -46,7 +46,7 @@ REGION_START(wifi_imem_1, 0x0A8C0000) BOOTBLOCK(0x0A8C0000, 24K) OVERLAP_VERSTAGE_ROMSTAGE(0x0A8C6000, 64K) - VBOOT2_WORK(0x0A8D6000, 16K) + VBOOT2_WORK(0x0A8D6000, 12K) PRERAM_CBMEM_CONSOLE(0x0A8DA000, 32K) TIMESTAMP(0x0A8E2000, 1K)
diff --git a/src/soc/qualcomm/ipq806x/include/soc/memlayout.ld b/src/soc/qualcomm/ipq806x/include/soc/memlayout.ld index 6ff2b77..25db175 100644 --- a/src/soc/qualcomm/ipq806x/include/soc/memlayout.ld +++ b/src/soc/qualcomm/ipq806x/include/soc/memlayout.ld @@ -27,7 +27,7 @@ /* DDR(0x2A000000, 48K) */ BOOTBLOCK(0x2A00C000, 24K) OVERLAP_VERSTAGE_ROMSTAGE(0x2A012000, 64K) - VBOOT2_WORK(0x2A022000, 16K) + VBOOT2_WORK(0x2A022000, 12K) PRERAM_CBMEM_CONSOLE(0x2A026000, 32K) TIMESTAMP(0x2A02E000, 1K)
diff --git a/src/soc/qualcomm/qcs405/include/soc/memlayout.ld b/src/soc/qualcomm/qcs405/include/soc/memlayout.ld index 8ed258b..68642d6 100644 --- a/src/soc/qualcomm/qcs405/include/soc/memlayout.ld +++ b/src/soc/qualcomm/qcs405/include/soc/memlayout.ld @@ -34,7 +34,7 @@ REGION(fw_reserved2, 0x8C19000, 0x16000, 4096) BOOTBLOCK(0x8C2F000, 40K) TTB(0x8C39000, 56K) - VBOOT2_WORK(0x8C47000, 16K) + VBOOT2_WORK(0x8C47000, 12K) STACK(0x8C4B000, 16K) TIMESTAMP(0x8C4F000, 1K) PRERAM_CBMEM_CONSOLE(0x8C4F400, 32K) diff --git a/src/soc/qualcomm/sdm845/include/soc/memlayout.ld b/src/soc/qualcomm/sdm845/include/soc/memlayout.ld index 5d99a8e..7063c69 100644 --- a/src/soc/qualcomm/sdm845/include/soc/memlayout.ld +++ b/src/soc/qualcomm/sdm845/include/soc/memlayout.ld @@ -35,7 +35,7 @@ REGION(fw_reserved2, 0x14800000, 0x16000, 4096) BOOTBLOCK(0x14816000, 40K) TTB(0x14820000, 56K) - VBOOT2_WORK(0x1482E000, 16K) + VBOOT2_WORK(0x1482E000, 12K) STACK(0x14832000, 16K) TIMESTAMP(0x14836000, 1K) PRERAM_CBMEM_CONSOLE(0x14836400, 32K) diff --git a/src/soc/samsung/exynos5250/include/soc/memlayout.ld b/src/soc/samsung/exynos5250/include/soc/memlayout.ld index ab79594..0bd319e 100644 --- a/src/soc/samsung/exynos5250/include/soc/memlayout.ld +++ b/src/soc/samsung/exynos5250/include/soc/memlayout.ld @@ -33,7 +33,7 @@ TTB(0x2058000, 16K) PRERAM_CBFS_CACHE(0x205C000, 78K) VBOOT2_TPM_LOG(0x206F800, 2K) - VBOOT2_WORK(0x2070000, 16K) + VBOOT2_WORK(0x2070000, 12K) STACK(0x2074000, 16K) SRAM_END(0x2078000)
-
build bot (Jenkins) (Code Review) -
Furquan Shaikh (Code Review)
-
Joel Kitching (Code Review)
-
Julius Werner (Code Review)
-
Patrick Georgi (Code Review)
-
Simon Glass (Code Review)