From a6c5ddd595e145ffd091a9fcadb5e0ffbf0fa8d1 Mon Sep 17 00:00:00 2001 From: Furquan Shaikh Date: Fri, 22 Jul 2016 06:59:40 -0700 Subject: [PATCH] vboot: Clean up vboot code 1. Remove unused functions/structures. 2. Add checks for NULL return values. 3. Change prefixes to vb2 instead of vboot for functions used internally within vboot2/ 4. Get rid of vboot_handoff.h file and move the structure definition to vboot_common.h 5. Rename all functions using handoff structure to have prefix vboot_handoff_*. All the handoff functions can be run _only_ after cbmem is online. 6. Organize vboot_common.h content according to different functionalities. BUG=chrome-os-partner:55431 Change-Id: I4c07d50327d88cddbdfbb0b6f82c264e2b8620eb Signed-off-by: Furquan Shaikh Reviewed-on: https://review.coreboot.org/15799 Reviewed-by: Aaron Durbin Tested-by: build bot (Jenkins) --- src/lib/bootmode.c | 10 +-- src/mainboard/google/rush_ryu/mainboard.c | 2 +- src/soc/intel/skylake/fsp_reset.c | 8 +- src/vendorcode/google/chromeos/chromeos.c | 2 - src/vendorcode/google/chromeos/chromeos.h | 2 - src/vendorcode/google/chromeos/elog.c | 2 +- src/vendorcode/google/chromeos/gnvs.c | 2 +- .../google/chromeos/vboot2/common.c | 18 +++-- src/vendorcode/google/chromeos/vboot2/misc.h | 4 +- .../google/chromeos/vboot2/vboot_handoff.c | 1 - .../google/chromeos/vboot2/vboot_loader.c | 6 +- src/vendorcode/google/chromeos/vboot_common.c | 51 ++++++------ src/vendorcode/google/chromeos/vboot_common.h | 80 ++++++++++++------- .../google/chromeos/vboot_handoff.h | 36 --------- 14 files changed, 106 insertions(+), 118 deletions(-) delete mode 100644 src/vendorcode/google/chromeos/vboot_handoff.h diff --git a/src/lib/bootmode.c b/src/lib/bootmode.c index 15f7a5a5e5..c69502691a 100644 --- a/src/lib/bootmode.c +++ b/src/lib/bootmode.c @@ -23,7 +23,7 @@ int developer_mode_enabled(void) if (get_developer_mode_switch()) return 1; #if CONFIG_VBOOT_VERIFY_FIRMWARE - if (vboot_enable_developer()) + if (vboot_handoff_check_developer_flag()) return 1; #endif return 0; @@ -36,7 +36,7 @@ int developer_mode_enabled(void) * * If shared recovery reason is set: * - before VbInit then get_recovery_mode_from_vbnv() is true - * - after VbInit then vboot_enable_recovery() is true + * - after VbInit then vboot_handoff_check_recovery_flag() is true * * Otherwise the mainboard handler for get_recovery_mode_switch() * will detect recovery mode initiated by the EC. @@ -50,7 +50,7 @@ int recovery_mode_enabled(void) return 1; #endif #if CONFIG_VBOOT_VERIFY_FIRMWARE - if (vboot_enable_recovery()) + if (vboot_handoff_check_recovery_flag()) return 1; #endif return 0; @@ -75,9 +75,9 @@ void gfx_set_init_done(int done) int display_init_required(void) { - /* For Chrome OS always honor vboot_skip_display_init(). */ + /* For Chrome OS always honor vboot_handoff_skip_display_init(). */ if (IS_ENABLED(CONFIG_CHROMEOS)) - return !vboot_skip_display_init(); + return !vboot_handoff_skip_display_init(); /* By default always initialize display. */ return 1; diff --git a/src/mainboard/google/rush_ryu/mainboard.c b/src/mainboard/google/rush_ryu/mainboard.c index e0479f321f..85034b3e47 100644 --- a/src/mainboard/google/rush_ryu/mainboard.c +++ b/src/mainboard/google/rush_ryu/mainboard.c @@ -35,8 +35,8 @@ #include #if IS_ENABLED(CONFIG_CHROMEOS) #include -#include #include +#include #endif #include "gpio.h" diff --git a/src/soc/intel/skylake/fsp_reset.c b/src/soc/intel/skylake/fsp_reset.c index 4751872a47..f25f1572f5 100644 --- a/src/soc/intel/skylake/fsp_reset.c +++ b/src/soc/intel/skylake/fsp_reset.c @@ -13,7 +13,7 @@ * GNU General Public License for more details. */ #include -#include +#include static int is_recovery; /* flag to identify recovery mode */ @@ -35,8 +35,10 @@ static void set_recovery_request(void *unused) * Set recovery flag during Recovery Mode Silicon Init * & store recovery request into VBNV */ - if (is_recovery) - set_recovery_mode_into_vbnv(vboot_recovery_reason()); + if (is_recovery) { + int reason = vboot_handoff_get_recovery_reason(); + set_recovery_mode_into_vbnv(reason); + } } diff --git a/src/vendorcode/google/chromeos/chromeos.c b/src/vendorcode/google/chromeos/chromeos.c index 926ed39414..563f6fd99b 100644 --- a/src/vendorcode/google/chromeos/chromeos.c +++ b/src/vendorcode/google/chromeos/chromeos.c @@ -23,7 +23,6 @@ int __attribute__((weak)) clear_recovery_mode_switch(void) return 0; } -#ifdef __ROMSTAGE__ void __attribute__((weak)) save_chromeos_gpios(void) { // Can be implemented by a mainboard @@ -34,4 +33,3 @@ int __attribute__((weak)) get_sw_write_protect_state(void) // Can be implemented by a platform / mainboard return 0; } -#endif diff --git a/src/vendorcode/google/chromeos/chromeos.h b/src/vendorcode/google/chromeos/chromeos.h index 57a2f71b29..ad9ef2f88f 100644 --- a/src/vendorcode/google/chromeos/chromeos.h +++ b/src/vendorcode/google/chromeos/chromeos.h @@ -24,9 +24,7 @@ #include "vboot_common.h" #include "vboot2/misc.h" -#if ENV_ROMSTAGE void save_chromeos_gpios(void); -#endif #if CONFIG_CHROMEOS /* functions implemented in elog.c */ diff --git a/src/vendorcode/google/chromeos/elog.c b/src/vendorcode/google/chromeos/elog.c index bb71da34da..710d6f693b 100644 --- a/src/vendorcode/google/chromeos/elog.c +++ b/src/vendorcode/google/chromeos/elog.c @@ -18,7 +18,7 @@ #include #include #if CONFIG_VBOOT_VERIFY_FIRMWARE -#include "vboot_handoff.h" +#include "vboot_common.h" #include #endif diff --git a/src/vendorcode/google/chromeos/gnvs.c b/src/vendorcode/google/chromeos/gnvs.c index 3443188e11..48668e8361 100644 --- a/src/vendorcode/google/chromeos/gnvs.c +++ b/src/vendorcode/google/chromeos/gnvs.c @@ -24,7 +24,7 @@ #include "chromeos.h" #include "gnvs.h" #if CONFIG_VBOOT_VERIFY_FIRMWARE -#include "vboot_handoff.h" +#include "vboot_common.h" #include #endif diff --git a/src/vendorcode/google/chromeos/vboot2/common.c b/src/vendorcode/google/chromeos/vboot2/common.c index 749f328a01..58ea95d219 100644 --- a/src/vendorcode/google/chromeos/vboot2/common.c +++ b/src/vendorcode/google/chromeos/vboot2/common.c @@ -13,6 +13,7 @@ * GNU General Public License for more details. */ +#include #include #include #include @@ -21,7 +22,7 @@ #include #include "../chromeos.h" #include "../symbols.h" -#include "../vboot_handoff.h" +#include "../vboot_common.h" #include "misc.h" struct selected_region { @@ -134,19 +135,20 @@ int vb2_get_selected_region(struct region *region) void vb2_set_selected_region(const struct region *region) { struct selected_region *reg = vb2_selected_region(); + + assert(reg != NULL); + reg->offset = region_offset(region); reg->size = region_sz(region); } -int vboot_is_slot_selected(void) +int vb2_is_slot_selected(void) { const struct selected_region *reg = vb2_selected_region(); - return reg->size > 0; -} -int vboot_is_readonly_path(void) -{ - return !vboot_is_slot_selected(); + assert(reg != NULL); + + return reg->size > 0; } void vb2_store_selected_region(void) @@ -160,6 +162,8 @@ void vb2_store_selected_region(void) sel_reg = cbmem_add(CBMEM_ID_VBOOT_SEL_REG, sizeof(*sel_reg)); + assert(sel_reg != NULL); + sel_reg->offset = wd->selected_region.offset; sel_reg->size = wd->selected_region.size; } diff --git a/src/vendorcode/google/chromeos/vboot2/misc.h b/src/vendorcode/google/chromeos/vboot2/misc.h index fc9d2547cb..ca64b378df 100644 --- a/src/vendorcode/google/chromeos/vboot2/misc.h +++ b/src/vendorcode/google/chromeos/vboot2/misc.h @@ -29,8 +29,8 @@ struct vb2_shared_data *vb2_get_shared_data(void); /* Returns 0 on success. < 0 on failure. */ int vb2_get_selected_region(struct region *region); void vb2_set_selected_region(const struct region *region); -int vboot_is_slot_selected(void); -int vboot_is_readonly_path(void); +int vb2_is_slot_selected(void); +int vb2_logic_executed(void); /* Store the selected region in cbmem for later use. */ void vb2_store_selected_region(void); diff --git a/src/vendorcode/google/chromeos/vboot2/vboot_handoff.c b/src/vendorcode/google/chromeos/vboot2/vboot_handoff.c index c89e9a0373..0e2cb84ccf 100644 --- a/src/vendorcode/google/chromeos/vboot2/vboot_handoff.c +++ b/src/vendorcode/google/chromeos/vboot2/vboot_handoff.c @@ -29,7 +29,6 @@ #include #include #include "../chromeos.h" -#include "../vboot_handoff.h" #include "misc.h" /** diff --git a/src/vendorcode/google/chromeos/vboot2/vboot_loader.c b/src/vendorcode/google/chromeos/vboot2/vboot_loader.c index f6efe0f2c7..7541518f13 100644 --- a/src/vendorcode/google/chromeos/vboot2/vboot_loader.c +++ b/src/vendorcode/google/chromeos/vboot2/vboot_loader.c @@ -21,7 +21,7 @@ #include #include #include "misc.h" -#include "../vboot_handoff.h" +#include "../vboot_common.h" #include "../symbols.h" /* The stage loading code is compiled and entered from multiple stages. The @@ -61,7 +61,7 @@ static int verstage_should_load(void) static int vboot_executed CAR_GLOBAL; -static int vboot_logic_executed(void) +int vb2_logic_executed(void) { /* If this stage is supposed to run the vboot logic ensure it has been * executed. */ @@ -139,7 +139,7 @@ static int vboot_locate(struct cbfs_props *props) struct region selected_region; /* Don't honor vboot results until the vboot logic has run. */ - if (!vboot_logic_executed()) + if (!vb2_logic_executed()) return -1; if (vb2_get_selected_region(&selected_region)) diff --git a/src/vendorcode/google/chromeos/vboot_common.c b/src/vendorcode/google/chromeos/vboot_common.c index 28135a0d65..66800ed635 100644 --- a/src/vendorcode/google/chromeos/vboot_common.c +++ b/src/vendorcode/google/chromeos/vboot_common.c @@ -26,72 +26,70 @@ #include "chromeos.h" #include "vboot_common.h" -#include "vboot_handoff.h" int vboot_named_region_device(const char *name, struct region_device *rdev) { return fmap_locate_area_as_rdev(name, rdev); } -int vboot_region_device(const struct region *reg, struct region_device *rdev) -{ - return boot_device_ro_subregion(reg, rdev); -} - +/* ========================== VBOOT HANDOFF APIs =========================== */ int vboot_get_handoff_info(void **addr, uint32_t *size) { - struct vboot_handoff *vboot_handoff; - - /* No flags are available in a separate verstage or bootblock because - * cbmem only comes online when dram does. */ - if ((ENV_VERSTAGE && IS_ENABLED(CONFIG_VBOOT_STARTS_IN_BOOTBLOCK)) || - ENV_BOOTBLOCK) + /* + * vboot_handoff is present only after cbmem comes online. If we are in + * pre-ram stage, then bail out early. + */ + if (ENV_BOOTBLOCK || + (ENV_VERSTAGE && IS_ENABLED(CONFIG_VBOOT_STARTS_IN_BOOTBLOCK))) return -1; + struct vboot_handoff *vboot_handoff; vboot_handoff = cbmem_find(CBMEM_ID_VBOOT_HANDOFF); if (vboot_handoff == NULL) return -1; *addr = vboot_handoff; - *size = sizeof(*vboot_handoff); + + if (size) + *size = sizeof(*vboot_handoff); return 0; } -static int vboot_handoff_flag(uint32_t flag) +static int vboot_get_handoff_flag(uint32_t flag) { struct vboot_handoff *vbho; - uint32_t size; - if (vboot_get_handoff_info((void **)&vbho, &size)) + /* + * If vboot_handoff cannot be found, return default value of flag as 0. + */ + if (vboot_get_handoff_info((void **)&vbho, NULL)) return 0; return !!(vbho->init_params.out_flags & flag); } -int vboot_skip_display_init(void) +int vboot_handoff_skip_display_init(void) { - return !vboot_handoff_flag(VB_INIT_OUT_ENABLE_DISPLAY); + return !vboot_get_handoff_flag(VB_INIT_OUT_ENABLE_DISPLAY); } -int vboot_enable_developer(void) +int vboot_handoff_check_developer_flag(void) { - return vboot_handoff_flag(VB_INIT_OUT_ENABLE_DEVELOPER); + return vboot_get_handoff_flag(VB_INIT_OUT_ENABLE_DEVELOPER); } -int vboot_enable_recovery(void) +int vboot_handoff_check_recovery_flag(void) { - return vboot_handoff_flag(VB_INIT_OUT_ENABLE_RECOVERY); + return vboot_get_handoff_flag(VB_INIT_OUT_ENABLE_RECOVERY); } -int vboot_recovery_reason(void) +int vboot_handoff_get_recovery_reason(void) { struct vboot_handoff *vbho; VbSharedDataHeader *sd; - vbho = cbmem_find(CBMEM_ID_VBOOT_HANDOFF); - - if (vbho == NULL) + if (vboot_get_handoff_info((void **)&vbho, NULL)) return 0; sd = (VbSharedDataHeader *)vbho->shared_data; @@ -99,6 +97,7 @@ int vboot_recovery_reason(void) return sd->recovery_reason; } +/* ============================ VBOOT REBOOT ============================== */ void __attribute__((weak)) vboot_platform_prepare_reboot(void) { } diff --git a/src/vendorcode/google/chromeos/vboot_common.h b/src/vendorcode/google/chromeos/vboot_common.h index 250b0e50ac..ae135fe66f 100644 --- a/src/vendorcode/google/chromeos/vboot_common.h +++ b/src/vendorcode/google/chromeos/vboot_common.h @@ -15,37 +15,61 @@ #ifndef VBOOT_COMMON_H #define VBOOT_COMMON_H -#include #include +#include +#include +#include -/* The FW areas consist of multiple components. At the beginning of - * each area is the number of total compoments as well as the size and - * offset for each component. One needs to caculate the total size of the - * signed firmware region based off of the embedded metadata. */ -struct vboot_component_entry { - uint32_t offset; - uint32_t size; -} __attribute__((packed)); +#include "chromeos.h" -struct vboot_components { - uint32_t num_components; - struct vboot_component_entry entries[0]; -} __attribute__((packed)); - -/* The following functions return 0 on success, < 0 on error. */ +/* Locate vboot area by name. Returns 0 on success and -1 on error. */ int vboot_named_region_device(const char *name, struct region_device *rdev); -int vboot_region_device(const struct region *reg, struct region_device *rdev); + +/* ========================== VBOOT HANDOFF APIs =========================== */ +/* + * The vboot_handoff structure contains the data to be consumed by downstream + * firmware after firmware selection has been completed. Namely it provides + * vboot shared data as well as the flags from VbInit. + */ +struct vboot_handoff { + VbInitParams init_params; + uint32_t selected_firmware; + char shared_data[VB_SHARED_DATA_MIN_SIZE]; +} __attribute__((packed)); + +/* + * vboot_get_handoff_info returns pointer to the vboot_handoff structure if + * available. vboot_handoff is available only after CBMEM comes online. If size + * is not NULL, size of the vboot_handoff structure is returned in it. + * Returns 0 on success and -1 on error. + */ int vboot_get_handoff_info(void **addr, uint32_t *size); -/* The following functions return 1 for true and 0 for false. */ -int vboot_skip_display_init(void); -int vboot_enable_recovery(void); -int vboot_enable_developer(void); - -int vboot_recovery_reason(void); +/* + * The following functions read vboot_handoff structure to obtain requested + * information. If vboot handoff is not available, 0 is returned by default. + * If vboot handoff is available: + * Returns 1 for flag if true + * Returns 0 for flag if false + * Returns value read for other fields + */ +int vboot_handoff_skip_display_init(void); +int vboot_handoff_check_recovery_flag(void); +int vboot_handoff_check_developer_flag(void); +int vboot_handoff_get_recovery_reason(void); +/* ============================ VBOOT REBOOT ============================== */ +/* + * vboot_reboot handles the reboot requests made by vboot_reference library. It + * allows the platform to run any preparation steps before the reboot and then + * does a hard reset. + */ void vboot_reboot(void); +/* Allow the platform to do any clean up work when vboot requests a reboot. */ +void vboot_platform_prepare_reboot(void); + +/* ============================ VBOOT RESUME ============================== */ /* * Save the provided hash digest to a secure location to check against in * the resume path. Returns 0 on success, < 0 on error. @@ -64,13 +88,13 @@ int vboot_retrieve_hash(void *digest, size_t digest_size); */ int vboot_platform_is_resuming(void); -/* Allow the platform to do any clean up work when vboot requests a reboot. */ -void vboot_platform_prepare_reboot(void); - -/* Main logic for verified boot. verstage() is the stage entry point - * while the verstage_main() is just the core logic. */ +/* ============================= VERSTAGE ================================== */ +/* + * Main logic for verified boot. verstage() is the stage entry point + * while the verstage_main() is just the core logic. + */ void verstage_main(void); -void verstage_mainboard_init(void); void verstage(void); +void verstage_mainboard_init(void); #endif /* VBOOT_COMMON_H */ diff --git a/src/vendorcode/google/chromeos/vboot_handoff.h b/src/vendorcode/google/chromeos/vboot_handoff.h deleted file mode 100644 index f06044403d..0000000000 --- a/src/vendorcode/google/chromeos/vboot_handoff.h +++ /dev/null @@ -1,36 +0,0 @@ -/* - * This file is part of the coreboot project. - * - * Copyright (C) 2013 Google, Inc. - * - * This program is free software; you can redistribute it and/or modify - * it under the terms of the GNU General Public License as published by - * the Free Software Foundation; version 2 of the License. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU General Public License for more details. - */ -#ifndef VBOOT_HANDOFF_H -#define VBOOT_HANDOFF_H - - -#include -#include -#include "chromeos.h" -#include "vboot_common.h" - -/* - * The vboot_handoff structure contains the data to be consumed by downstream - * firmware after firmware selection has been completed. Namely it provides - * vboot shared data as well as the flags from VbInit. - */ -struct vboot_handoff { - VbInitParams init_params; - uint32_t selected_firmware; - char shared_data[VB_SHARED_DATA_MIN_SIZE]; -} __attribute__((packed)); - - -#endif /* VBOOT_HANDOFF_H */