Change in ...coreboot[master]: vboot: Use CONFIG_VBOOT_MIGRATE_WORKING_DATA on all platforms
Hello Joel Kitching,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/33952
to review the following change.
Change subject: vboot: Use CONFIG_VBOOT_MIGRATE_WORKING_DATA on all platforms ......................................................................
vboot: Use CONFIG_VBOOT_MIGRATE_WORKING_DATA on all platforms
When we added CONFIG_VBOOT_MIGRATE_WORKING_DATA, the idea was that on some Arm platforms the original working data buffer was in SRAM, which stays accessbile for the whole runtime of the system. There is no reason to migrate it into CBMEM on those platforms because ramstage and the payload could continue to access it in SRAM.
Now that we've had a couple of months of experience with this option, we found that most of our Arm platforms have some issue that requires migrating anyway, because BL31 often claims SRAM for itself and makes it inaccessible to the payload. On the remaining platforms, accessing SRAM from the payload is possible but still an issue, because libpayload doesn't have enough memory layout information to set up proper page tables for it, so we're accessing it uncached and at risk of alignment errors.
Rather than having to figure out how to map the right SRAM range for every platform in the payload, let's just get rid of the option. memcpy()ing 12KB isn't worth this much hassle.
Change-Id: I1b94e01c998f723c8950be4d12cc8f02b363a1bf Signed-off-by: Julius Werner jwerner@chromium.org --- M src/security/vboot/Kconfig M src/security/vboot/common.c M src/security/vboot/vboot_loader.c M src/soc/qualcomm/qcs405/Kconfig M src/soc/qualcomm/sdm845/Kconfig M src/soc/rockchip/rk3399/Kconfig 6 files changed, 3 insertions(+), 25 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/33952/1
diff --git a/src/security/vboot/Kconfig b/src/security/vboot/Kconfig index 66bcc1e..ea1f738 100644 --- a/src/security/vboot/Kconfig +++ b/src/security/vboot/Kconfig @@ -107,21 +107,6 @@ memory initialization). This implies that vboot working data is allocated in CBMEM.
-config VBOOT_MIGRATE_WORKING_DATA - bool - default y if CACHE_AS_RAM - depends on !VBOOT_STARTS_IN_ROMSTAGE - help - In order to make vboot data structures available downstream, - migrate verified boot working data to CBMEM after CBMEM comes - online, when VBOOT_STARTS_IN_BOOTBLOCK is employed. This should - always be enabled on x86 architectures to migrate data from CAR - before losing access in ramstage, and should almost always be - disabled in SRAM architectures, where access to SRAM is usually - retained. Any SRAM platform where the original location of the - VBOOT_WORKBUF region becomes inaccessible in later stages should - manually select this option. - config VBOOT_MOCK_SECDATA bool "Mock secdata for firmware verification" default n diff --git a/src/security/vboot/common.c b/src/security/vboot/common.c index bd72683..626fbc5 100644 --- a/src/security/vboot/common.c +++ b/src/security/vboot/common.c @@ -117,13 +117,12 @@ return reg->size > 0; }
-#if CONFIG(VBOOT_MIGRATE_WORKING_DATA) +#if CONFIG(VBOOT_STARTS_IN_BOOTBLOCK) /* * For platforms that do not employ VBOOT_STARTS_IN_ROMSTAGE, vboot * verification occurs before CBMEM is brought online, using pre-RAM. * In order to make vboot data structures available downstream, copy - * vboot_working_data from SRAM/CAR into CBMEM on platforms where this - * memory later becomes unavailable. + * vboot_working_data from SRAM/CAR into CBMEM. */ static void vboot_migrate_cbmem(int unused) { @@ -140,7 +139,7 @@ memcpy(wd_cbmem, wd_preram, cbmem_size); } ROMSTAGE_CBMEM_INIT_HOOK(vboot_migrate_cbmem) -#elif CONFIG(VBOOT_STARTS_IN_ROMSTAGE) +#else static void vboot_setup_cbmem(int unused) { struct vboot_working_data *wd_cbmem = diff --git a/src/security/vboot/vboot_loader.c b/src/security/vboot/vboot_loader.c index 9e2cd00..af4a3fd 100644 --- a/src/security/vboot/vboot_loader.c +++ b/src/security/vboot/vboot_loader.c @@ -26,9 +26,6 @@ _Static_assert(CONFIG(VBOOT_STARTS_IN_BOOTBLOCK) + CONFIG(VBOOT_STARTS_IN_ROMSTAGE) == 1, "vboot must either start in bootblock or romstage (not both!)"); -_Static_assert(CONFIG(VBOOT_STARTS_IN_BOOTBLOCK) || - !CONFIG(VBOOT_MIGRATE_WORKING_DATA), - "no need to migrate working data after CBMEM is already up!"); _Static_assert(!CONFIG(VBOOT_SEPARATE_VERSTAGE) || CONFIG(VBOOT_STARTS_IN_BOOTBLOCK), "stand-alone verstage must start in (i.e. after) bootblock"); diff --git a/src/soc/qualcomm/qcs405/Kconfig b/src/soc/qualcomm/qcs405/Kconfig index e24993a..aa867c2 100644 --- a/src/soc/qualcomm/qcs405/Kconfig +++ b/src/soc/qualcomm/qcs405/Kconfig @@ -19,7 +19,6 @@ select VBOOT_SEPARATE_VERSTAGE select VBOOT_RETURN_FROM_VERSTAGE select VBOOT_STARTS_IN_BOOTBLOCK - select VBOOT_MIGRATE_WORKING_DATA
config QCS405_BLSP_SPI bool diff --git a/src/soc/qualcomm/sdm845/Kconfig b/src/soc/qualcomm/sdm845/Kconfig index f6268c9..459a441 100644 --- a/src/soc/qualcomm/sdm845/Kconfig +++ b/src/soc/qualcomm/sdm845/Kconfig @@ -19,7 +19,6 @@ select VBOOT_RETURN_FROM_VERSTAGE select VBOOT_MUST_REQUEST_DISPLAY select VBOOT_STARTS_IN_BOOTBLOCK - select VBOOT_MIGRATE_WORKING_DATA
config SDM845_QSPI bool diff --git a/src/soc/rockchip/rk3399/Kconfig b/src/soc/rockchip/rk3399/Kconfig index 897a597..83fc437 100644 --- a/src/soc/rockchip/rk3399/Kconfig +++ b/src/soc/rockchip/rk3399/Kconfig @@ -17,7 +17,6 @@ if SOC_ROCKCHIP_RK3399
config VBOOT - select VBOOT_MIGRATE_WORKING_DATA select VBOOT_SEPARATE_VERSTAGE select VBOOT_RETURN_FROM_VERSTAGE select VBOOT_MUST_REQUEST_DISPLAY
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33952 )
Change subject: vboot: Use CONFIG_VBOOT_MIGRATE_WORKING_DATA on all platforms ......................................................................
Patch Set 1: Code-Review+2
I shall weep for the extra bits being copied around. But I think this is simpler and more consistent in the long run.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33952 )
Change subject: vboot: Use CONFIG_VBOOT_MIGRATE_WORKING_DATA on all platforms ......................................................................
Patch Set 1: Code-Review+1
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33952 )
Change subject: vboot: Use CONFIG_VBOOT_MIGRATE_WORKING_DATA on all platforms ......................................................................
Patch Set 1: Code-Review+1
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33952 )
Change subject: vboot: Use CONFIG_VBOOT_MIGRATE_WORKING_DATA on all platforms ......................................................................
Patch Set 1: Code-Review-2
Talked to Joel today and we decided that we may want to solve this issue differently after all. Holding back on this CL until we've decided. (If you need immediate relief for MT8183, just select CONFIG_VBOOT_MIGRATE_WORKING_DATA from the SoC Kconfig for now.)
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33952 )
Change subject: vboot: Use CONFIG_VBOOT_MIGRATE_WORKING_DATA on all platforms ......................................................................
Patch Set 1: -Code-Review
Thought about this more and flip-flopped again, I think we should just stick with this patch. The other idea was modeling SRAM in the coreboot table so the payload could map it, but the problem remains that for the majority of Arm platforms we have, SRAM isn't accessible in non-secure EL2 after BL31 runs anyway. Mapping inaccessible memory as normal/write-back is dangerous even the program flow never accesses it, so adding that memory type would mean that coreboot has to know exactly what ranges BL31 does or does not protect. That is a hard mapping to maintain manually and not easy to notice if it breaks. When I came up with this I thought that it would also solve an issue of checking that BL31 targets usable memory during selfload(), but I forgot that we had already solved that issue with BM_MEM_BL31 (which is only used in coreboot and not passed on through the coreboot table), so it wouldn't really add anything there. It remains that doing the CBMEM migration here just doesn't cost enough to be worth creating new interfaces that bring their own problems instead.
Julius Werner has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33952 )
Change subject: vboot: Use CONFIG_VBOOT_MIGRATE_WORKING_DATA on all platforms ......................................................................
vboot: Use CONFIG_VBOOT_MIGRATE_WORKING_DATA on all platforms
When we added CONFIG_VBOOT_MIGRATE_WORKING_DATA, the idea was that on some Arm platforms the original working data buffer was in SRAM, which stays accessbile for the whole runtime of the system. There is no reason to migrate it into CBMEM on those platforms because ramstage and the payload could continue to access it in SRAM.
Now that we've had a couple of months of experience with this option, we found that most of our Arm platforms have some issue that requires migrating anyway, because BL31 often claims SRAM for itself and makes it inaccessible to the payload. On the remaining platforms, accessing SRAM from the payload is possible but still an issue, because libpayload doesn't have enough memory layout information to set up proper page tables for it, so we're accessing it uncached and at risk of alignment errors.
Rather than having to figure out how to map the right SRAM range for every platform in the payload, let's just get rid of the option. memcpy()ing 12KB isn't worth this much hassle.
Change-Id: I1b94e01c998f723c8950be4d12cc8f02b363a1bf Signed-off-by: Julius Werner jwerner@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/33952 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Joel Kitching kitching@google.com Reviewed-by: Paul Menzel paulepanter@users.sourceforge.net Reviewed-by: Hung-Te Lin hungte@chromium.org --- M src/security/vboot/Kconfig M src/security/vboot/common.c M src/security/vboot/vboot_loader.c M src/soc/qualcomm/qcs405/Kconfig M src/soc/qualcomm/sdm845/Kconfig M src/soc/rockchip/rk3399/Kconfig 6 files changed, 3 insertions(+), 25 deletions(-)
Approvals: build bot (Jenkins): Verified Paul Menzel: Looks good to me, but someone else must approve Joel Kitching: Looks good to me, approved Hung-Te Lin: Looks good to me, but someone else must approve
diff --git a/src/security/vboot/Kconfig b/src/security/vboot/Kconfig index 66bcc1e..ea1f738 100644 --- a/src/security/vboot/Kconfig +++ b/src/security/vboot/Kconfig @@ -107,21 +107,6 @@ memory initialization). This implies that vboot working data is allocated in CBMEM.
-config VBOOT_MIGRATE_WORKING_DATA - bool - default y if CACHE_AS_RAM - depends on !VBOOT_STARTS_IN_ROMSTAGE - help - In order to make vboot data structures available downstream, - migrate verified boot working data to CBMEM after CBMEM comes - online, when VBOOT_STARTS_IN_BOOTBLOCK is employed. This should - always be enabled on x86 architectures to migrate data from CAR - before losing access in ramstage, and should almost always be - disabled in SRAM architectures, where access to SRAM is usually - retained. Any SRAM platform where the original location of the - VBOOT_WORKBUF region becomes inaccessible in later stages should - manually select this option. - config VBOOT_MOCK_SECDATA bool "Mock secdata for firmware verification" default n diff --git a/src/security/vboot/common.c b/src/security/vboot/common.c index bd72683..626fbc5 100644 --- a/src/security/vboot/common.c +++ b/src/security/vboot/common.c @@ -117,13 +117,12 @@ return reg->size > 0; }
-#if CONFIG(VBOOT_MIGRATE_WORKING_DATA) +#if CONFIG(VBOOT_STARTS_IN_BOOTBLOCK) /* * For platforms that do not employ VBOOT_STARTS_IN_ROMSTAGE, vboot * verification occurs before CBMEM is brought online, using pre-RAM. * In order to make vboot data structures available downstream, copy - * vboot_working_data from SRAM/CAR into CBMEM on platforms where this - * memory later becomes unavailable. + * vboot_working_data from SRAM/CAR into CBMEM. */ static void vboot_migrate_cbmem(int unused) { @@ -140,7 +139,7 @@ memcpy(wd_cbmem, wd_preram, cbmem_size); } ROMSTAGE_CBMEM_INIT_HOOK(vboot_migrate_cbmem) -#elif CONFIG(VBOOT_STARTS_IN_ROMSTAGE) +#else static void vboot_setup_cbmem(int unused) { struct vboot_working_data *wd_cbmem = diff --git a/src/security/vboot/vboot_loader.c b/src/security/vboot/vboot_loader.c index 9e2cd00..af4a3fd 100644 --- a/src/security/vboot/vboot_loader.c +++ b/src/security/vboot/vboot_loader.c @@ -26,9 +26,6 @@ _Static_assert(CONFIG(VBOOT_STARTS_IN_BOOTBLOCK) + CONFIG(VBOOT_STARTS_IN_ROMSTAGE) == 1, "vboot must either start in bootblock or romstage (not both!)"); -_Static_assert(CONFIG(VBOOT_STARTS_IN_BOOTBLOCK) || - !CONFIG(VBOOT_MIGRATE_WORKING_DATA), - "no need to migrate working data after CBMEM is already up!"); _Static_assert(!CONFIG(VBOOT_SEPARATE_VERSTAGE) || CONFIG(VBOOT_STARTS_IN_BOOTBLOCK), "stand-alone verstage must start in (i.e. after) bootblock"); diff --git a/src/soc/qualcomm/qcs405/Kconfig b/src/soc/qualcomm/qcs405/Kconfig index e24993a..aa867c2 100644 --- a/src/soc/qualcomm/qcs405/Kconfig +++ b/src/soc/qualcomm/qcs405/Kconfig @@ -19,7 +19,6 @@ select VBOOT_SEPARATE_VERSTAGE select VBOOT_RETURN_FROM_VERSTAGE select VBOOT_STARTS_IN_BOOTBLOCK - select VBOOT_MIGRATE_WORKING_DATA
config QCS405_BLSP_SPI bool diff --git a/src/soc/qualcomm/sdm845/Kconfig b/src/soc/qualcomm/sdm845/Kconfig index f6268c9..459a441 100644 --- a/src/soc/qualcomm/sdm845/Kconfig +++ b/src/soc/qualcomm/sdm845/Kconfig @@ -19,7 +19,6 @@ select VBOOT_RETURN_FROM_VERSTAGE select VBOOT_MUST_REQUEST_DISPLAY select VBOOT_STARTS_IN_BOOTBLOCK - select VBOOT_MIGRATE_WORKING_DATA
config SDM845_QSPI bool diff --git a/src/soc/rockchip/rk3399/Kconfig b/src/soc/rockchip/rk3399/Kconfig index 897a597..83fc437 100644 --- a/src/soc/rockchip/rk3399/Kconfig +++ b/src/soc/rockchip/rk3399/Kconfig @@ -17,7 +17,6 @@ if SOC_ROCKCHIP_RK3399
config VBOOT - select VBOOT_MIGRATE_WORKING_DATA select VBOOT_SEPARATE_VERSTAGE select VBOOT_RETURN_FROM_VERSTAGE select VBOOT_MUST_REQUEST_DISPLAY
-
Hung-Te Lin (Code Review) -
Joel Kitching (Code Review)
-
Julius Werner (Code Review)
-
Paul Menzel (Code Review)