Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37028 )
Change subject: security/vboot: Drop CAR_GLOBAL_MIGRATION support ......................................................................
security/vboot: Drop CAR_GLOBAL_MIGRATION support
Change-Id: I9dee03da028b9111b685e325368815a86e444a47 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/security/vboot/common.c M src/security/vboot/misc.h M src/security/vboot/vbnv.c M src/security/vboot/vbnv_flash.c M src/security/vboot/vboot_loader.c 5 files changed, 37 insertions(+), 65 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/28/37028/1
diff --git a/src/security/vboot/common.c b/src/security/vboot/common.c index 043748c..b0b841e 100644 --- a/src/security/vboot/common.c +++ b/src/security/vboot/common.c @@ -24,7 +24,7 @@ #include <security/vboot/symbols.h> #include <security/vboot/vboot_common.h>
-static struct vb2_context *vboot_ctx CAR_GLOBAL; +static struct vb2_context *vboot_ctx;
struct vboot_working_data *vboot_get_working_data(void) { @@ -49,20 +49,19 @@
struct vb2_context *vboot_get_context(void) { - struct vb2_context **vboot_ctx_ptr = car_get_var_ptr(&vboot_ctx); struct vboot_working_data *wd;
/* Return if context has already been initialized/restored. */ - if (*vboot_ctx_ptr) - return *vboot_ctx_ptr; + if (vboot_ctx) + return vboot_ctx;
wd = vboot_get_working_data();
/* Restore context from a previous stage. */ if (vboot_logic_executed()) { assert(vb2api_reinit(vboot_get_workbuf(wd), - vboot_ctx_ptr) == VB2_SUCCESS); - return *vboot_ctx_ptr; + &vboot_ctx) == VB2_SUCCESS); + return vboot_ctx; }
assert(verification_should_run()); @@ -78,9 +77,9 @@
/* Initialize vb2_shared_data and friends. */ assert(vb2api_init(vboot_get_workbuf(wd), wd->buffer_size, - vboot_ctx_ptr) == VB2_SUCCESS); + &vboot_ctx) == VB2_SUCCESS);
- return *vboot_ctx_ptr; + return vboot_ctx; }
int vboot_get_selected_region(struct region *region) @@ -148,7 +147,7 @@ vb2api_relocate(vboot_get_workbuf(wd_cbmem), vboot_get_workbuf(wd_preram), cbmem_size - wd_cbmem->buffer_offset, - car_get_var_ptr(&vboot_ctx)); + &vboot_ctx); } ROMSTAGE_CBMEM_INIT_HOOK(vboot_migrate_cbmem) #else diff --git a/src/security/vboot/misc.h b/src/security/vboot/misc.h index 812bbe7..8496aa4 100644 --- a/src/security/vboot/misc.h +++ b/src/security/vboot/misc.h @@ -17,7 +17,6 @@ #define __VBOOT_MISC_H__
#include <assert.h> -#include <arch/early_variables.h> #include <security/vboot/vboot_common.h>
struct vb2_context; @@ -100,7 +99,7 @@ need to check a global to see if verfication has run. */ if (verification_should_run() || (verstage_should_load() && CONFIG(VBOOT_RETURN_FROM_VERSTAGE))) - return car_get_var(vboot_executed); + return vboot_executed;
if (CONFIG(VBOOT_STARTS_IN_BOOTBLOCK)) { /* All other stages are "after the bootblock" */ diff --git a/src/security/vboot/vbnv.c b/src/security/vboot/vbnv.c index eccd743..b455ee2 100644 --- a/src/security/vboot/vbnv.c +++ b/src/security/vboot/vbnv.c @@ -13,32 +13,13 @@ * GNU General Public License for more details. */
-#include <arch/early_variables.h> #include <string.h> #include <types.h> #include <security/vboot/vbnv.h> #include <security/vboot/vbnv_layout.h>
-static int vbnv_initialized CAR_GLOBAL; -static uint8_t vbnv[VBOOT_VBNV_BLOCK_SIZE] CAR_GLOBAL; - -/* Wrappers for accessing the variables marked as CAR_GLOBAL. */ -static inline int is_vbnv_initialized(void) -{ - return car_get_var(vbnv_initialized); -} - -static inline uint8_t *vbnv_data_addr(int index) -{ - uint8_t *vbnv_arr = car_get_var_ptr(vbnv); - - return &vbnv_arr[index]; -} - -static inline uint8_t vbnv_data(int index) -{ - return *vbnv_data_addr(index); -} +static int vbnv_initialized; +static uint8_t vbnv[VBOOT_VBNV_BLOCK_SIZE];
/* Return CRC-8 of the data, using x^8 + x^2 + x + 1 polynomial. */ static uint8_t crc8_vbnv(const uint8_t *data, int len) @@ -66,9 +47,9 @@ /* Read VBNV data into cache. */ static void vbnv_setup(void) { - if (!is_vbnv_initialized()) { - read_vbnv(vbnv_data_addr(0)); - car_set_var(vbnv_initialized, 1); + if (!vbnv_initialized) { + read_vbnv(&vbnv[0])); + vbnv_initialized = 1; } }
@@ -117,7 +98,7 @@ save_vbnv_flash(vbnv_copy);
/* Clear initialized flag to force cached data to be updated */ - car_set_var(vbnv_initialized, 0); + vbnv_initialized = 0; }
/* Save a recovery reason into VBNV. */ @@ -137,14 +118,14 @@ int get_recovery_mode_from_vbnv(void) { vbnv_setup(); - return vbnv_data(RECOVERY_OFFSET); + return vbnv[RECOVERY_OFFSET]; }
/* Read the USB Device Controller(UDC) enable flag from VBNV. */ int vbnv_udc_enable_flag(void) { vbnv_setup(); - return (vbnv_data(DEV_FLAGS_OFFSET) & DEV_ENABLE_UDC) ? 1 : 0; + return (vbnv[DEV_FLAGS_OFFSET] & DEV_ENABLE_UDC) ? 1 : 0; }
void vbnv_init(uint8_t *vbnv_copy) diff --git a/src/security/vboot/vbnv_flash.c b/src/security/vboot/vbnv_flash.c index 908846f..5600ef6 100644 --- a/src/security/vboot/vbnv_flash.c +++ b/src/security/vboot/vbnv_flash.c @@ -13,7 +13,6 @@ * GNU General Public License for more details. */
-#include <arch/early_variables.h> #include <commonlib/region.h> #include <console/console.h> #include <string.h> @@ -40,7 +39,7 @@ /* Cache of the current nvdata */ uint8_t cache[BLOB_SIZE]; }; -static struct vbnv_flash_ctx vbnv_flash CAR_GLOBAL; +static struct vbnv_flash_ctx vbnv_flash;
/* * This code assumes that flash is erased to 1-bits, and write operations can @@ -59,8 +58,7 @@
static int init_vbnv(void) { - struct vbnv_flash_ctx *ctx = car_get_var_ptr(&vbnv_flash); - struct region_device *rdev = &ctx->vbnv_dev; + struct region_device *rdev = &vbnv_flash.vbnv_dev; uint8_t buf[BLOB_SIZE]; uint8_t empty_blob[BLOB_SIZE]; int used_below, empty_above; @@ -77,7 +75,7 @@ for (i = 0; i < BLOB_SIZE; i++) empty_blob[i] = erase_value();
- ctx->top_offset = region_device_sz(rdev) - BLOB_SIZE; + vbnv_flash.top_offset = region_device_sz(rdev) - BLOB_SIZE;
/* Binary search for the border between used and empty */ used_below = 0; @@ -102,21 +100,20 @@ offset = used_below * BLOB_SIZE;
/* reread the nvdata and write it to the cache */ - if (rdev_readat(rdev, ctx->cache, offset, BLOB_SIZE) < 0) { + if (rdev_readat(rdev, vbnv_flash.cache, offset, BLOB_SIZE) < 0) { printk(BIOS_ERR, "failed to read nvdata\n"); return 1; }
- ctx->blob_offset = offset; - ctx->initialized = 1; + vbnv_flash.blob_offset = offset; + vbnv_flash.initialized = 1;
return 0; }
static int erase_nvram(void) { - struct vbnv_flash_ctx *ctx = car_get_var_ptr(&vbnv_flash); - const struct region_device *rdev = &ctx->vbnv_dev; + const struct region_device *rdev = &vbnv_flash.vbnv_dev;
if (rdev_eraseat(rdev, 0, region_device_sz(rdev)) < 0) { printk(BIOS_ERR, "failed to erase nvram\n"); @@ -129,38 +126,35 @@
void read_vbnv_flash(uint8_t *vbnv_copy) { - struct vbnv_flash_ctx *ctx = car_get_var_ptr(&vbnv_flash); - - if (!ctx->initialized) + if (!vbnv_flash.initialized) if (init_vbnv()) return; /* error */
- memcpy(vbnv_copy, ctx->cache, BLOB_SIZE); + memcpy(vbnv_copy, vbnv_flash.cache, BLOB_SIZE); }
void save_vbnv_flash(const uint8_t *vbnv_copy) { - struct vbnv_flash_ctx *ctx = car_get_var_ptr(&vbnv_flash); int new_offset; int i; - const struct region_device *rdev = &ctx->vbnv_dev; + const struct region_device *rdev = &vbnv_flash.vbnv_dev;
- if (!ctx->initialized) + if (!vbnv_flash.initialized) if (init_vbnv()) return; /* error */
/* Bail out if there have been no changes. */ - if (!memcmp(vbnv_copy, ctx->cache, BLOB_SIZE)) + if (!memcmp(vbnv_copy, vbnv_flash.cache, BLOB_SIZE)) return;
- new_offset = ctx->blob_offset; + new_offset = vbnv_flash.blob_offset;
/* See if we can overwrite the current blob with the new one */ for (i = 0; i < BLOB_SIZE; i++) { - if (!can_overwrite(ctx->cache[i], vbnv_copy[i])) { + if (!can_overwrite(vbnv_flash.cache[i], vbnv_copy[i])) { /* unable to overwrite. need to use the next blob */ new_offset += BLOB_SIZE; - if (new_offset > ctx->top_offset) { + if (new_offset > vbnv_flash.top_offset) { if (erase_nvram()) return; /* error */ new_offset = 0; @@ -171,8 +165,8 @@
if (rdev_writeat(rdev, vbnv_copy, new_offset, BLOB_SIZE) == BLOB_SIZE) { /* write was successful. safely move pointer forward */ - ctx->blob_offset = new_offset; - memcpy(ctx->cache, vbnv_copy, BLOB_SIZE); + vbnv_flash.blob_offset = new_offset; + memcpy(vbnv_flash.cache, vbnv_copy, BLOB_SIZE); } else { printk(BIOS_ERR, "failed to save nvdata\n"); } diff --git a/src/security/vboot/vboot_loader.c b/src/security/vboot/vboot_loader.c index 2b7ba83..23b4d7a 100644 --- a/src/security/vboot/vboot_loader.c +++ b/src/security/vboot/vboot_loader.c @@ -13,7 +13,6 @@ * GNU General Public License for more details. */
-#include <arch/early_variables.h> #include <cbfs.h> #include <console/console.h> #include <ec/google/chromeec/ec.h> @@ -33,14 +32,14 @@ CONFIG(VBOOT_SEPARATE_VERSTAGE), "return from verstage only makes sense for separate verstages");
-int vboot_executed CAR_GLOBAL; +int vboot_executed;
void vboot_run_logic(void) { if (verification_should_run()) { /* Note: this path is not used for VBOOT_RETURN_FROM_VERSTAGE */ verstage_main(); - car_set_var(vboot_executed, 1); + vboot_executed = 1; } else if (verstage_should_load()) { struct cbfsf file; struct prog verstage = @@ -67,7 +66,7 @@ if (!CONFIG(VBOOT_RETURN_FROM_VERSTAGE)) return;
- car_set_var(vboot_executed, 1); + vboot_executed = 1; } }
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37028 )
Change subject: security/vboot: Drop CAR_GLOBAL_MIGRATION support ......................................................................
Patch Set 1: Code-Review+2
Hello Aaron Durbin, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37028
to look at the new patch set (#2).
Change subject: security/vboot: Drop CAR_GLOBAL_MIGRATION support ......................................................................
security/vboot: Drop CAR_GLOBAL_MIGRATION support
Change-Id: I9dee03da028b9111b685e325368815a86e444a47 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/security/vboot/common.c M src/security/vboot/misc.h M src/security/vboot/vbnv.c M src/security/vboot/vbnv_flash.c M src/security/vboot/vboot_loader.c 5 files changed, 38 insertions(+), 67 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/28/37028/2
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37028 )
Change subject: security/vboot: Drop CAR_GLOBAL_MIGRATION support ......................................................................
Patch Set 4: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/37028/4/src/security/vboot/vbnv_fla... File src/security/vboot/vbnv_flash.c:
https://review.coreboot.org/c/coreboot/+/37028/4/src/security/vboot/vbnv_fla... PS4, Line 62: struct region_device *rdev = &vbnv_flash.vbnv_dev; fwiw, if you would have left ctx then only one line would have needed to change. Same is true in every function.
Hello Aaron Durbin, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37028
to look at the new patch set (#5).
Change subject: security/vboot: Drop CAR_GLOBAL_MIGRATION support ......................................................................
security/vboot: Drop CAR_GLOBAL_MIGRATION support
Change-Id: I9dee03da028b9111b685e325368815a86e444a47 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/security/vboot/common.c M src/security/vboot/misc.h M src/security/vboot/vbnv.c M src/security/vboot/vbnv_flash.c M src/security/vboot/vboot_loader.c 5 files changed, 26 insertions(+), 50 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/28/37028/5
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37028 )
Change subject: security/vboot: Drop CAR_GLOBAL_MIGRATION support ......................................................................
Patch Set 6: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/37028 )
Change subject: security/vboot: Drop CAR_GLOBAL_MIGRATION support ......................................................................
security/vboot: Drop CAR_GLOBAL_MIGRATION support
Change-Id: I9dee03da028b9111b685e325368815a86e444a47 Signed-off-by: Arthur Heymans arthur@aheymans.xyz Reviewed-on: https://review.coreboot.org/c/coreboot/+/37028 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Aaron Durbin adurbin@chromium.org --- M src/security/vboot/common.c M src/security/vboot/misc.h M src/security/vboot/vbnv.c M src/security/vboot/vbnv_flash.c M src/security/vboot/vboot_loader.c 5 files changed, 26 insertions(+), 50 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved
diff --git a/src/security/vboot/common.c b/src/security/vboot/common.c index bad01ff..290fa5e 100644 --- a/src/security/vboot/common.c +++ b/src/security/vboot/common.c @@ -25,7 +25,7 @@ #include <security/vboot/symbols.h> #include <security/vboot/vboot_common.h>
-static struct vb2_context *vboot_ctx CAR_GLOBAL; +static struct vb2_context *vboot_ctx;
struct vboot_working_data *vboot_get_working_data(void) { @@ -50,20 +50,19 @@
struct vb2_context *vboot_get_context(void) { - struct vb2_context **vboot_ctx_ptr = car_get_var_ptr(&vboot_ctx); struct vboot_working_data *wd;
/* Return if context has already been initialized/restored. */ - if (*vboot_ctx_ptr) - return *vboot_ctx_ptr; + if (vboot_ctx) + return vboot_ctx;
wd = vboot_get_working_data();
/* Restore context from a previous stage. */ if (vboot_logic_executed()) { assert(vb2api_reinit(vboot_get_workbuf(wd), - vboot_ctx_ptr) == VB2_SUCCESS); - return *vboot_ctx_ptr; + &vboot_ctx) == VB2_SUCCESS); + return vboot_ctx; }
assert(verification_should_run()); @@ -78,10 +77,10 @@ /* Initialize vb2_shared_data and friends. */ assert(vb2api_init(vboot_get_workbuf(wd), VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE - - wd->buffer_offset, - vboot_ctx_ptr) == VB2_SUCCESS); + wd->buffer_offset, + &vboot_ctx) == VB2_SUCCESS);
- return *vboot_ctx_ptr; + return vboot_ctx; }
int vboot_locate_firmware(const struct vb2_context *ctx, @@ -116,7 +115,7 @@ vb2api_relocate(vboot_get_workbuf(wd_cbmem), vboot_get_workbuf(wd_preram), cbmem_size - wd_cbmem->buffer_offset, - car_get_var_ptr(&vboot_ctx)); + &vboot_ctx); } ROMSTAGE_CBMEM_INIT_HOOK(vboot_migrate_cbmem) #else diff --git a/src/security/vboot/misc.h b/src/security/vboot/misc.h index 471f838..9f681f6 100644 --- a/src/security/vboot/misc.h +++ b/src/security/vboot/misc.h @@ -17,7 +17,6 @@ #define __VBOOT_MISC_H__
#include <assert.h> -#include <arch/early_variables.h> #include <security/vboot/vboot_common.h>
struct vb2_context; @@ -112,7 +111,7 @@ need to check a global to see if verfication has run. */ if (verification_should_run() || (verstage_should_load() && CONFIG(VBOOT_RETURN_FROM_VERSTAGE))) - return car_get_var(vboot_executed); + return vboot_executed;
if (CONFIG(VBOOT_STARTS_IN_BOOTBLOCK)) { /* All other stages are "after the bootblock" */ diff --git a/src/security/vboot/vbnv.c b/src/security/vboot/vbnv.c index eccd743..be598ac 100644 --- a/src/security/vboot/vbnv.c +++ b/src/security/vboot/vbnv.c @@ -13,32 +13,13 @@ * GNU General Public License for more details. */
-#include <arch/early_variables.h> #include <string.h> #include <types.h> #include <security/vboot/vbnv.h> #include <security/vboot/vbnv_layout.h>
-static int vbnv_initialized CAR_GLOBAL; -static uint8_t vbnv[VBOOT_VBNV_BLOCK_SIZE] CAR_GLOBAL; - -/* Wrappers for accessing the variables marked as CAR_GLOBAL. */ -static inline int is_vbnv_initialized(void) -{ - return car_get_var(vbnv_initialized); -} - -static inline uint8_t *vbnv_data_addr(int index) -{ - uint8_t *vbnv_arr = car_get_var_ptr(vbnv); - - return &vbnv_arr[index]; -} - -static inline uint8_t vbnv_data(int index) -{ - return *vbnv_data_addr(index); -} +static int vbnv_initialized; +static uint8_t vbnv[VBOOT_VBNV_BLOCK_SIZE];
/* Return CRC-8 of the data, using x^8 + x^2 + x + 1 polynomial. */ static uint8_t crc8_vbnv(const uint8_t *data, int len) @@ -66,9 +47,9 @@ /* Read VBNV data into cache. */ static void vbnv_setup(void) { - if (!is_vbnv_initialized()) { - read_vbnv(vbnv_data_addr(0)); - car_set_var(vbnv_initialized, 1); + if (!vbnv_initialized) { + read_vbnv(vbnv); + vbnv_initialized = 1; } }
@@ -117,7 +98,7 @@ save_vbnv_flash(vbnv_copy);
/* Clear initialized flag to force cached data to be updated */ - car_set_var(vbnv_initialized, 0); + vbnv_initialized = 0; }
/* Save a recovery reason into VBNV. */ @@ -137,14 +118,14 @@ int get_recovery_mode_from_vbnv(void) { vbnv_setup(); - return vbnv_data(RECOVERY_OFFSET); + return vbnv[RECOVERY_OFFSET]; }
/* Read the USB Device Controller(UDC) enable flag from VBNV. */ int vbnv_udc_enable_flag(void) { vbnv_setup(); - return (vbnv_data(DEV_FLAGS_OFFSET) & DEV_ENABLE_UDC) ? 1 : 0; + return (vbnv[DEV_FLAGS_OFFSET] & DEV_ENABLE_UDC) ? 1 : 0; }
void vbnv_init(uint8_t *vbnv_copy) diff --git a/src/security/vboot/vbnv_flash.c b/src/security/vboot/vbnv_flash.c index 86c43cd..58d3aba 100644 --- a/src/security/vboot/vbnv_flash.c +++ b/src/security/vboot/vbnv_flash.c @@ -13,7 +13,6 @@ * GNU General Public License for more details. */
-#include <arch/early_variables.h> #include <commonlib/region.h> #include <console/console.h> #include <fmap.h> @@ -41,7 +40,7 @@ /* Cache of the current nvdata */ uint8_t cache[BLOB_SIZE]; }; -static struct vbnv_flash_ctx vbnv_flash CAR_GLOBAL; +static struct vbnv_flash_ctx vbnv_flash;
/* * This code assumes that flash is erased to 1-bits, and write operations can @@ -60,7 +59,7 @@
static int init_vbnv(void) { - struct vbnv_flash_ctx *ctx = car_get_var_ptr(&vbnv_flash); + struct vbnv_flash_ctx *ctx = &vbnv_flash; struct region_device *rdev = &ctx->vbnv_dev; uint8_t buf[BLOB_SIZE]; uint8_t empty_blob[BLOB_SIZE]; @@ -116,7 +115,7 @@
static int erase_nvram(void) { - struct vbnv_flash_ctx *ctx = car_get_var_ptr(&vbnv_flash); + struct vbnv_flash_ctx *ctx = &vbnv_flash; const struct region_device *rdev = &ctx->vbnv_dev;
if (rdev_eraseat(rdev, 0, region_device_sz(rdev)) < 0) { @@ -130,7 +129,7 @@
void read_vbnv_flash(uint8_t *vbnv_copy) { - struct vbnv_flash_ctx *ctx = car_get_var_ptr(&vbnv_flash); + struct vbnv_flash_ctx *ctx = &vbnv_flash;
if (!ctx->initialized) if (init_vbnv()) @@ -141,7 +140,7 @@
void save_vbnv_flash(const uint8_t *vbnv_copy) { - struct vbnv_flash_ctx *ctx = car_get_var_ptr(&vbnv_flash); + struct vbnv_flash_ctx *ctx = &vbnv_flash; int new_offset; int i; const struct region_device *rdev = &ctx->vbnv_dev; diff --git a/src/security/vboot/vboot_loader.c b/src/security/vboot/vboot_loader.c index 3e491a7..9aaaff2 100644 --- a/src/security/vboot/vboot_loader.c +++ b/src/security/vboot/vboot_loader.c @@ -13,8 +13,6 @@ * GNU General Public License for more details. */
-#include <arch/early_variables.h> -#include <boot_device.h> #include <cbfs.h> #include <console/console.h> #include <ec/google/chromeec/ec.h> @@ -34,14 +32,14 @@ CONFIG(VBOOT_SEPARATE_VERSTAGE), "return from verstage only makes sense for separate verstages");
-int vboot_executed CAR_GLOBAL; +int vboot_executed;
void vboot_run_logic(void) { if (verification_should_run()) { /* Note: this path is not used for VBOOT_RETURN_FROM_VERSTAGE */ verstage_main(); - car_set_var(vboot_executed, 1); + vboot_executed = 1; } else if (verstage_should_load()) { struct cbfsf file; struct prog verstage = @@ -68,7 +66,7 @@ if (!CONFIG(VBOOT_RETURN_FROM_VERSTAGE)) return;
- car_set_var(vboot_executed, 1); + vboot_executed = 1; } }
-
Aaron Durbin (Code Review) -
Arthur Heymans (Code Review)
-
Patrick Georgi (Code Review)