| Message ID | 20211219070605.14894-7-sughosh.ganu@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | FWU: Add support for FWU Multi Bank Update feature | expand |
Sughosh, On Sun, Dec 19, 2021 at 12:36:03PM +0530, Sughosh Ganu wrote: > The FWU Multi Bank Update specification requires the Update Agent to > carry out certain checks at the time of platform boot. The Update > Agent is the component which is responsible for updating the firmware > components and maintaining and keeping the metadata in sync. > > The spec requires that the Update Agent perform the following checks > at the time of boot > * Sanity check of both the metadata copies maintained by the platform. > * Get the boot index passed to U-Boot by the prior stage bootloader > and use this value for metadata bookkeeping. > * Check if the system is booting in Trial State. If the system boots > in the Trial State for more than a specified number of boot counts, > change the Active Bank to be booting the platform from. > > Add these checks in the board initialisation sequence, invoked after > relocation. > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > --- > Changes since V1: > * Define a funtion fwu_update_checks_pass to do the checks > before initiating the update > * Log the status of the boottime checks using boottime_check > variable and allow system to boot instead of hanging the > platform(fwu_boottime_checks) > > common/board_r.c | 6 ++ > include/fwu.h | 3 + > lib/fwu_updates/fwu.c | 163 ++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 172 insertions(+) > create mode 100644 lib/fwu_updates/fwu.c > > diff --git a/common/board_r.c b/common/board_r.c > index 31a59c585a..81678870b9 100644 > --- a/common/board_r.c > +++ b/common/board_r.c > @@ -78,6 +78,9 @@ > #ifdef CONFIG_EFI_SETUP_EARLY > #include <efi_loader.h> > #endif > +#ifdef CONFIG_FWU_MULTI_BANK_UPDATE > +#include <fwu.h> > +#endif > > DECLARE_GLOBAL_DATA_PTR; > > @@ -805,6 +808,9 @@ static init_fnc_t init_sequence_r[] = { > #endif > #ifdef CONFIG_EFI_SETUP_EARLY > (init_fnc_t)efi_init_obj_list, > +#endif > +#ifdef CONFIG_FWU_MULTI_BANK_UPDATE > + fwu_boottime_checks, > #endif Maybe I don't understand your code well. Why do you want to call fwu_boottime_checks() here? Why not in CONFIG_EFI_CAPSULE_UPDATE_EARLY (i.e. efi_launch_capsules() in main_loop())? -Takahiro Akashi > run_main_loop, > }; > diff --git a/include/fwu.h b/include/fwu.h > index 5ba437798d..2d2e674d6a 100644 > --- a/include/fwu.h > +++ b/include/fwu.h > @@ -16,6 +16,9 @@ > EFI_GUID(0x8a7a84a0, 0x8387, 0x40f6, 0xab, 0x41, \ > 0xa8, 0xb9, 0xa5, 0xa6, 0x0d, 0x23) > > +int fwu_boottime_checks(void); > +u8 fwu_update_checks_pass(void); > + > int fwu_get_active_index(u32 *active_idx); > int fwu_update_active_index(u32 active_idx); > int fwu_get_image_alt_num(efi_guid_t image_type_id, u32 update_bank, > diff --git a/lib/fwu_updates/fwu.c b/lib/fwu_updates/fwu.c > new file mode 100644 > index 0000000000..e964f9b0b1 > --- /dev/null > +++ b/lib/fwu_updates/fwu.c > @@ -0,0 +1,163 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Copyright (c) 2021, Linaro Limited > + */ > + > +#include <efi.h> > +#include <efi_loader.h> > +#include <efi_variable.h> > +#include <fwu.h> > +#include <fwu_mdata.h> > +#include <malloc.h> > + > +#include <linux/errno.h> > +#include <linux/types.h> > + > +static u8 trial_state = 0; > +static u8 boottime_check = 0; > + > +static int fwu_trial_state_check(void) > +{ > + int ret, i; > + efi_status_t status; > + efi_uintn_t var_size; > + u16 trial_state_ctr; > + u32 nimages, active_bank, var_attributes, active_idx; > + struct fwu_mdata *mdata = NULL; > + struct fwu_image_entry *img_entry; > + struct fwu_image_bank_info *img_bank_info; > + > + ret = fwu_get_mdata(&mdata); > + if (ret < 0) > + return ret; > + > + ret = 0; > + nimages = CONFIG_FWU_NUM_IMAGES_PER_BANK; > + active_bank = mdata->active_index; > + img_entry = &mdata->img_entry[0]; > + for (i = 0; i < nimages; i++) { > + img_bank_info = &img_entry[i].img_bank_info[active_bank]; > + if (!img_bank_info->accepted) { > + trial_state = 1; > + break; > + } > + } > + > + if (trial_state) { > + var_size = (efi_uintn_t)sizeof(trial_state_ctr); > + log_info("System booting in Trial State\n"); > + var_attributes = EFI_VARIABLE_NON_VOLATILE | > + EFI_VARIABLE_BOOTSERVICE_ACCESS | > + EFI_VARIABLE_RUNTIME_ACCESS; > + status = efi_get_variable_int(L"TrialStateCtr", > + &efi_global_variable_guid, > + &var_attributes, > + &var_size, &trial_state_ctr, > + NULL); > + if (status != EFI_SUCCESS) { > + log_err("Unable to read TrialStateCtr variable\n"); > + ret = -1; > + goto out; > + } > + > + ++trial_state_ctr; > + if (trial_state_ctr > CONFIG_FWU_TRIAL_STATE_CNT) { > + log_info("Trial State count exceeded. Revert back to previous_active_index\n"); > + active_idx = mdata->active_index; > + ret = fwu_revert_boot_index(); > + if (ret < 0) { > + log_err("Unable to revert active_index\n"); > + goto out; > + } > + > + trial_state_ctr = 0; > + status = efi_set_variable_int(L"TrialStateCtr", > + &efi_global_variable_guid, > + var_attributes, > + 0, > + &trial_state_ctr, false); > + if (status != EFI_SUCCESS) { > + log_err("Unable to clear TrialStateCtr variable\n"); > + ret = -1; > + goto out; > + } > + } else { > + status = efi_set_variable_int(L"TrialStateCtr", > + &efi_global_variable_guid, > + var_attributes, > + var_size, > + &trial_state_ctr, false); > + if (status != EFI_SUCCESS) { > + log_err("Unable to increment TrialStateCtr variable\n"); > + ret = -1; > + goto out; > + } else { > + ret = 0; > + } > + } > + } > + > +out: > + free(mdata); > + return ret; > +} > + > +u8 fwu_update_checks_pass(void) > +{ > + return !trial_state && boottime_check; > +} > + > +int fwu_boottime_checks(void) > +{ > + int ret; > + u32 boot_idx, active_idx; > + > + ret = fwu_mdata_check(); > + if (ret < 0) { > + boottime_check = 0; > + return 0; > + } > + > + /* > + * Get the Boot Index, i.e. the bank from > + * which the platform has booted. This value > + * gets passed from the ealier stage bootloader > + * which booted u-boot, e.g. tf-a. If the > + * boot index is not the same as the > + * active_index read from the FWU metadata, > + * update the active_index. > + */ > + fwu_plat_get_bootidx(&boot_idx); > + if (boot_idx >= CONFIG_FWU_NUM_BANKS) { > + log_err("Received incorrect value of boot_index\n"); > + boottime_check = 0; > + return 0; > + } > + > + ret = fwu_get_active_index(&active_idx); > + if (ret < 0) { > + log_err("Unable to read active_index\n"); > + boottime_check = 0; > + return 0; > + } > + > + if (boot_idx != active_idx) { > + log_info("Boot idx %u is not matching active idx %u, changing active_idx\n", > + boot_idx, active_idx); > + ret = fwu_update_active_index(boot_idx); > + if (ret < 0) > + boottime_check = 0; > + else > + boottime_check = 1; > + > + return 0; > + } > + > + ret = fwu_trial_state_check(); > + if (ret < 0) > + boottime_check = 0; > + else > + boottime_check = 1; > + > + return 0; > +} > -- > 2.17.1 >
hi Takahiro, On Mon, 20 Dec 2021 at 11:39, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > Sughosh, > > On Sun, Dec 19, 2021 at 12:36:03PM +0530, Sughosh Ganu wrote: > > The FWU Multi Bank Update specification requires the Update Agent to > > carry out certain checks at the time of platform boot. The Update > > Agent is the component which is responsible for updating the firmware > > components and maintaining and keeping the metadata in sync. > > > > The spec requires that the Update Agent perform the following checks > > at the time of boot > > * Sanity check of both the metadata copies maintained by the platform. > > * Get the boot index passed to U-Boot by the prior stage bootloader > > and use this value for metadata bookkeeping. > > * Check if the system is booting in Trial State. If the system boots > > in the Trial State for more than a specified number of boot counts, > > change the Active Bank to be booting the platform from. > > > > Add these checks in the board initialisation sequence, invoked after > > relocation. > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > --- > > Changes since V1: > > * Define a funtion fwu_update_checks_pass to do the checks > > before initiating the update > > * Log the status of the boottime checks using boottime_check > > variable and allow system to boot instead of hanging the > > platform(fwu_boottime_checks) > > > > common/board_r.c | 6 ++ > > include/fwu.h | 3 + > > lib/fwu_updates/fwu.c | 163 ++++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 172 insertions(+) > > create mode 100644 lib/fwu_updates/fwu.c > > > > diff --git a/common/board_r.c b/common/board_r.c > > index 31a59c585a..81678870b9 100644 > > --- a/common/board_r.c > > +++ b/common/board_r.c > > @@ -78,6 +78,9 @@ > > #ifdef CONFIG_EFI_SETUP_EARLY > > #include <efi_loader.h> > > #endif > > +#ifdef CONFIG_FWU_MULTI_BANK_UPDATE > > +#include <fwu.h> > > +#endif > > > > DECLARE_GLOBAL_DATA_PTR; > > > > @@ -805,6 +808,9 @@ static init_fnc_t init_sequence_r[] = { > > #endif > > #ifdef CONFIG_EFI_SETUP_EARLY > > (init_fnc_t)efi_init_obj_list, > > +#endif > > +#ifdef CONFIG_FWU_MULTI_BANK_UPDATE > > + fwu_boottime_checks, > > #endif > > Maybe I don't understand your code well. > Why do you want to call fwu_boottime_checks() here? > Why not in CONFIG_EFI_CAPSULE_UPDATE_EARLY (i.e. efi_launch_capsules() > in main_loop())? > These are boot time checks which are to be carried out during platform boot. Do you see any issue if we place the call to fwu_boottime_checks in the init_sequence. I would like to avoid having the fwu boot time checks tied up with CONFIG_EFI_CAPSULE_ON_DISK_EARLY. -sughosh > > -Takahiro Akashi > > > run_main_loop, > > }; > > diff --git a/include/fwu.h b/include/fwu.h > > index 5ba437798d..2d2e674d6a 100644 > > --- a/include/fwu.h > > +++ b/include/fwu.h > > @@ -16,6 +16,9 @@ > > EFI_GUID(0x8a7a84a0, 0x8387, 0x40f6, 0xab, 0x41, \ > > 0xa8, 0xb9, 0xa5, 0xa6, 0x0d, 0x23) > > > > +int fwu_boottime_checks(void); > > +u8 fwu_update_checks_pass(void); > > + > > int fwu_get_active_index(u32 *active_idx); > > int fwu_update_active_index(u32 active_idx); > > int fwu_get_image_alt_num(efi_guid_t image_type_id, u32 update_bank, > > diff --git a/lib/fwu_updates/fwu.c b/lib/fwu_updates/fwu.c > > new file mode 100644 > > index 0000000000..e964f9b0b1 > > --- /dev/null > > +++ b/lib/fwu_updates/fwu.c > > @@ -0,0 +1,163 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * Copyright (c) 2021, Linaro Limited > > + */ > > + > > +#include <efi.h> > > +#include <efi_loader.h> > > +#include <efi_variable.h> > > +#include <fwu.h> > > +#include <fwu_mdata.h> > > +#include <malloc.h> > > + > > +#include <linux/errno.h> > > +#include <linux/types.h> > > + > > +static u8 trial_state = 0; > > +static u8 boottime_check = 0; > > + > > +static int fwu_trial_state_check(void) > > +{ > > + int ret, i; > > + efi_status_t status; > > + efi_uintn_t var_size; > > + u16 trial_state_ctr; > > + u32 nimages, active_bank, var_attributes, active_idx; > > + struct fwu_mdata *mdata = NULL; > > + struct fwu_image_entry *img_entry; > > + struct fwu_image_bank_info *img_bank_info; > > + > > + ret = fwu_get_mdata(&mdata); > > + if (ret < 0) > > + return ret; > > + > > + ret = 0; > > + nimages = CONFIG_FWU_NUM_IMAGES_PER_BANK; > > + active_bank = mdata->active_index; > > + img_entry = &mdata->img_entry[0]; > > + for (i = 0; i < nimages; i++) { > > + img_bank_info = &img_entry[i].img_bank_info[active_bank]; > > + if (!img_bank_info->accepted) { > > + trial_state = 1; > > + break; > > + } > > + } > > + > > + if (trial_state) { > > + var_size = (efi_uintn_t)sizeof(trial_state_ctr); > > + log_info("System booting in Trial State\n"); > > + var_attributes = EFI_VARIABLE_NON_VOLATILE | > > + EFI_VARIABLE_BOOTSERVICE_ACCESS | > > + EFI_VARIABLE_RUNTIME_ACCESS; > > + status = efi_get_variable_int(L"TrialStateCtr", > > + &efi_global_variable_guid, > > + &var_attributes, > > + &var_size, &trial_state_ctr, > > + NULL); > > + if (status != EFI_SUCCESS) { > > + log_err("Unable to read TrialStateCtr variable\n"); > > + ret = -1; > > + goto out; > > + } > > + > > + ++trial_state_ctr; > > + if (trial_state_ctr > CONFIG_FWU_TRIAL_STATE_CNT) { > > + log_info("Trial State count exceeded. Revert back > to previous_active_index\n"); > > + active_idx = mdata->active_index; > > + ret = fwu_revert_boot_index(); > > + if (ret < 0) { > > + log_err("Unable to revert active_index\n"); > > + goto out; > > + } > > + > > + trial_state_ctr = 0; > > + status = efi_set_variable_int(L"TrialStateCtr", > > + > &efi_global_variable_guid, > > + var_attributes, > > + 0, > > + &trial_state_ctr, > false); > > + if (status != EFI_SUCCESS) { > > + log_err("Unable to clear TrialStateCtr > variable\n"); > > + ret = -1; > > + goto out; > > + } > > + } else { > > + status = efi_set_variable_int(L"TrialStateCtr", > > + > &efi_global_variable_guid, > > + var_attributes, > > + var_size, > > + &trial_state_ctr, > false); > > + if (status != EFI_SUCCESS) { > > + log_err("Unable to increment TrialStateCtr > variable\n"); > > + ret = -1; > > + goto out; > > + } else { > > + ret = 0; > > + } > > + } > > + } > > + > > +out: > > + free(mdata); > > + return ret; > > +} > > + > > +u8 fwu_update_checks_pass(void) > > +{ > > + return !trial_state && boottime_check; > > +} > > + > > +int fwu_boottime_checks(void) > > +{ > > + int ret; > > + u32 boot_idx, active_idx; > > + > > + ret = fwu_mdata_check(); > > + if (ret < 0) { > > + boottime_check = 0; > > + return 0; > > + } > > + > > + /* > > + * Get the Boot Index, i.e. the bank from > > + * which the platform has booted. This value > > + * gets passed from the ealier stage bootloader > > + * which booted u-boot, e.g. tf-a. If the > > + * boot index is not the same as the > > + * active_index read from the FWU metadata, > > + * update the active_index. > > + */ > > + fwu_plat_get_bootidx(&boot_idx); > > + if (boot_idx >= CONFIG_FWU_NUM_BANKS) { > > + log_err("Received incorrect value of boot_index\n"); > > + boottime_check = 0; > > + return 0; > > + } > > + > > + ret = fwu_get_active_index(&active_idx); > > + if (ret < 0) { > > + log_err("Unable to read active_index\n"); > > + boottime_check = 0; > > + return 0; > > + } > > + > > + if (boot_idx != active_idx) { > > + log_info("Boot idx %u is not matching active idx %u, > changing active_idx\n", > > + boot_idx, active_idx); > > + ret = fwu_update_active_index(boot_idx); > > + if (ret < 0) > > + boottime_check = 0; > > + else > > + boottime_check = 1; > > + > > + return 0; > > + } > > + > > + ret = fwu_trial_state_check(); > > + if (ret < 0) > > + boottime_check = 0; > > + else > > + boottime_check = 1; > > + > > + return 0; > > +} > > -- > > 2.17.1 > > >
Hi Sughosh, On Mon, Dec 20, 2021 at 03:36:37PM +0530, Sughosh Ganu wrote: > hi Takahiro, > > On Mon, 20 Dec 2021 at 11:39, AKASHI Takahiro <takahiro.akashi@linaro.org> > wrote: > > > Sughosh, > > > > On Sun, Dec 19, 2021 at 12:36:03PM +0530, Sughosh Ganu wrote: > > > The FWU Multi Bank Update specification requires the Update Agent to > > > carry out certain checks at the time of platform boot. The Update > > > Agent is the component which is responsible for updating the firmware > > > components and maintaining and keeping the metadata in sync. > > > > > > The spec requires that the Update Agent perform the following checks > > > at the time of boot > > > * Sanity check of both the metadata copies maintained by the platform. > > > * Get the boot index passed to U-Boot by the prior stage bootloader > > > and use this value for metadata bookkeeping. > > > * Check if the system is booting in Trial State. If the system boots > > > in the Trial State for more than a specified number of boot counts, > > > change the Active Bank to be booting the platform from. > > > > > > Add these checks in the board initialisation sequence, invoked after > > > relocation. > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > --- > > > Changes since V1: > > > * Define a funtion fwu_update_checks_pass to do the checks > > > before initiating the update > > > * Log the status of the boottime checks using boottime_check > > > variable and allow system to boot instead of hanging the > > > platform(fwu_boottime_checks) > > > > > > common/board_r.c | 6 ++ > > > include/fwu.h | 3 + > > > lib/fwu_updates/fwu.c | 163 ++++++++++++++++++++++++++++++++++++++++++ > > > 3 files changed, 172 insertions(+) > > > create mode 100644 lib/fwu_updates/fwu.c > > > > > > diff --git a/common/board_r.c b/common/board_r.c > > > index 31a59c585a..81678870b9 100644 > > > --- a/common/board_r.c > > > +++ b/common/board_r.c > > > @@ -78,6 +78,9 @@ > > > #ifdef CONFIG_EFI_SETUP_EARLY > > > #include <efi_loader.h> > > > #endif > > > +#ifdef CONFIG_FWU_MULTI_BANK_UPDATE > > > +#include <fwu.h> > > > +#endif > > > > > > DECLARE_GLOBAL_DATA_PTR; > > > > > > @@ -805,6 +808,9 @@ static init_fnc_t init_sequence_r[] = { > > > #endif > > > #ifdef CONFIG_EFI_SETUP_EARLY > > > (init_fnc_t)efi_init_obj_list, > > > +#endif > > > +#ifdef CONFIG_FWU_MULTI_BANK_UPDATE > > > + fwu_boottime_checks, > > > #endif > > > > Maybe I don't understand your code well. > > Why do you want to call fwu_boottime_checks() here? > > Why not in CONFIG_EFI_CAPSULE_UPDATE_EARLY (i.e. efi_launch_capsules() > > in main_loop())? > > > > These are boot time checks which are to be carried out during platform > boot. Do you see any issue if we place the call to fwu_boottime_checks in > the init_sequence. No, I don't see any specific issue right now. > I would like to avoid having the fwu boot time checks > tied up with CONFIG_EFI_CAPSULE_ON_DISK_EARLY. My question was why you want to avoid that. In main_loop(), update_tftp(), as well as efi capsule code, is put there. It would be a good practice to have similar functionality called in one place. -Takahiro Akashi > -sughosh > > > > > > -Takahiro Akashi > > > > > run_main_loop, > > > }; > > > diff --git a/include/fwu.h b/include/fwu.h > > > index 5ba437798d..2d2e674d6a 100644 > > > --- a/include/fwu.h > > > +++ b/include/fwu.h > > > @@ -16,6 +16,9 @@ > > > EFI_GUID(0x8a7a84a0, 0x8387, 0x40f6, 0xab, 0x41, \ > > > 0xa8, 0xb9, 0xa5, 0xa6, 0x0d, 0x23) > > > > > > +int fwu_boottime_checks(void); > > > +u8 fwu_update_checks_pass(void); > > > + > > > int fwu_get_active_index(u32 *active_idx); > > > int fwu_update_active_index(u32 active_idx); > > > int fwu_get_image_alt_num(efi_guid_t image_type_id, u32 update_bank, > > > diff --git a/lib/fwu_updates/fwu.c b/lib/fwu_updates/fwu.c > > > new file mode 100644 > > > index 0000000000..e964f9b0b1 > > > --- /dev/null > > > +++ b/lib/fwu_updates/fwu.c > > > @@ -0,0 +1,163 @@ > > > +// SPDX-License-Identifier: GPL-2.0+ > > > +/* > > > + * Copyright (c) 2021, Linaro Limited > > > + */ > > > + > > > +#include <efi.h> > > > +#include <efi_loader.h> > > > +#include <efi_variable.h> > > > +#include <fwu.h> > > > +#include <fwu_mdata.h> > > > +#include <malloc.h> > > > + > > > +#include <linux/errno.h> > > > +#include <linux/types.h> > > > + > > > +static u8 trial_state = 0; > > > +static u8 boottime_check = 0; > > > + > > > +static int fwu_trial_state_check(void) > > > +{ > > > + int ret, i; > > > + efi_status_t status; > > > + efi_uintn_t var_size; > > > + u16 trial_state_ctr; > > > + u32 nimages, active_bank, var_attributes, active_idx; > > > + struct fwu_mdata *mdata = NULL; > > > + struct fwu_image_entry *img_entry; > > > + struct fwu_image_bank_info *img_bank_info; > > > + > > > + ret = fwu_get_mdata(&mdata); > > > + if (ret < 0) > > > + return ret; > > > + > > > + ret = 0; > > > + nimages = CONFIG_FWU_NUM_IMAGES_PER_BANK; > > > + active_bank = mdata->active_index; > > > + img_entry = &mdata->img_entry[0]; > > > + for (i = 0; i < nimages; i++) { > > > + img_bank_info = &img_entry[i].img_bank_info[active_bank]; > > > + if (!img_bank_info->accepted) { > > > + trial_state = 1; > > > + break; > > > + } > > > + } > > > + > > > + if (trial_state) { > > > + var_size = (efi_uintn_t)sizeof(trial_state_ctr); > > > + log_info("System booting in Trial State\n"); > > > + var_attributes = EFI_VARIABLE_NON_VOLATILE | > > > + EFI_VARIABLE_BOOTSERVICE_ACCESS | > > > + EFI_VARIABLE_RUNTIME_ACCESS; > > > + status = efi_get_variable_int(L"TrialStateCtr", > > > + &efi_global_variable_guid, > > > + &var_attributes, > > > + &var_size, &trial_state_ctr, > > > + NULL); > > > + if (status != EFI_SUCCESS) { > > > + log_err("Unable to read TrialStateCtr variable\n"); > > > + ret = -1; > > > + goto out; > > > + } > > > + > > > + ++trial_state_ctr; > > > + if (trial_state_ctr > CONFIG_FWU_TRIAL_STATE_CNT) { > > > + log_info("Trial State count exceeded. Revert back > > to previous_active_index\n"); > > > + active_idx = mdata->active_index; > > > + ret = fwu_revert_boot_index(); > > > + if (ret < 0) { > > > + log_err("Unable to revert active_index\n"); > > > + goto out; > > > + } > > > + > > > + trial_state_ctr = 0; > > > + status = efi_set_variable_int(L"TrialStateCtr", > > > + > > &efi_global_variable_guid, > > > + var_attributes, > > > + 0, > > > + &trial_state_ctr, > > false); > > > + if (status != EFI_SUCCESS) { > > > + log_err("Unable to clear TrialStateCtr > > variable\n"); > > > + ret = -1; > > > + goto out; > > > + } > > > + } else { > > > + status = efi_set_variable_int(L"TrialStateCtr", > > > + > > &efi_global_variable_guid, > > > + var_attributes, > > > + var_size, > > > + &trial_state_ctr, > > false); > > > + if (status != EFI_SUCCESS) { > > > + log_err("Unable to increment TrialStateCtr > > variable\n"); > > > + ret = -1; > > > + goto out; > > > + } else { > > > + ret = 0; > > > + } > > > + } > > > + } > > > + > > > +out: > > > + free(mdata); > > > + return ret; > > > +} > > > + > > > +u8 fwu_update_checks_pass(void) > > > +{ > > > + return !trial_state && boottime_check; > > > +} > > > + > > > +int fwu_boottime_checks(void) > > > +{ > > > + int ret; > > > + u32 boot_idx, active_idx; > > > + > > > + ret = fwu_mdata_check(); > > > + if (ret < 0) { > > > + boottime_check = 0; > > > + return 0; > > > + } > > > + > > > + /* > > > + * Get the Boot Index, i.e. the bank from > > > + * which the platform has booted. This value > > > + * gets passed from the ealier stage bootloader > > > + * which booted u-boot, e.g. tf-a. If the > > > + * boot index is not the same as the > > > + * active_index read from the FWU metadata, > > > + * update the active_index. > > > + */ > > > + fwu_plat_get_bootidx(&boot_idx); > > > + if (boot_idx >= CONFIG_FWU_NUM_BANKS) { > > > + log_err("Received incorrect value of boot_index\n"); > > > + boottime_check = 0; > > > + return 0; > > > + } > > > + > > > + ret = fwu_get_active_index(&active_idx); > > > + if (ret < 0) { > > > + log_err("Unable to read active_index\n"); > > > + boottime_check = 0; > > > + return 0; > > > + } > > > + > > > + if (boot_idx != active_idx) { > > > + log_info("Boot idx %u is not matching active idx %u, > > changing active_idx\n", > > > + boot_idx, active_idx); > > > + ret = fwu_update_active_index(boot_idx); > > > + if (ret < 0) > > > + boottime_check = 0; > > > + else > > > + boottime_check = 1; > > > + > > > + return 0; > > > + } > > > + > > > + ret = fwu_trial_state_check(); > > > + if (ret < 0) > > > + boottime_check = 0; > > > + else > > > + boottime_check = 1; > > > + > > > + return 0; > > > +} > > > -- > > > 2.17.1 > > > > >
hi Takahiro, On Mon, 20 Dec 2021 at 15:55, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > Hi Sughosh, > > On Mon, Dec 20, 2021 at 03:36:37PM +0530, Sughosh Ganu wrote: > > hi Takahiro, > > > > On Mon, 20 Dec 2021 at 11:39, AKASHI Takahiro < > takahiro.akashi@linaro.org> > > wrote: > > > > > Sughosh, > > > > > > On Sun, Dec 19, 2021 at 12:36:03PM +0530, Sughosh Ganu wrote: > > > > The FWU Multi Bank Update specification requires the Update Agent to > > > > carry out certain checks at the time of platform boot. The Update > > > > Agent is the component which is responsible for updating the firmware > > > > components and maintaining and keeping the metadata in sync. > > > > > > > > The spec requires that the Update Agent perform the following checks > > > > at the time of boot > > > > * Sanity check of both the metadata copies maintained by the > platform. > > > > * Get the boot index passed to U-Boot by the prior stage bootloader > > > > and use this value for metadata bookkeeping. > > > > * Check if the system is booting in Trial State. If the system boots > > > > in the Trial State for more than a specified number of boot counts, > > > > change the Active Bank to be booting the platform from. > > > > > > > > Add these checks in the board initialisation sequence, invoked after > > > > relocation. > > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > > --- > > > > Changes since V1: > > > > * Define a funtion fwu_update_checks_pass to do the checks > > > > before initiating the update > > > > * Log the status of the boottime checks using boottime_check > > > > variable and allow system to boot instead of hanging the > > > > platform(fwu_boottime_checks) > > > > > > > > common/board_r.c | 6 ++ > > > > include/fwu.h | 3 + > > > > lib/fwu_updates/fwu.c | 163 > ++++++++++++++++++++++++++++++++++++++++++ > > > > 3 files changed, 172 insertions(+) > > > > create mode 100644 lib/fwu_updates/fwu.c > > > > > > > > diff --git a/common/board_r.c b/common/board_r.c > > > > index 31a59c585a..81678870b9 100644 > > > > --- a/common/board_r.c > > > > +++ b/common/board_r.c > > > > @@ -78,6 +78,9 @@ > > > > #ifdef CONFIG_EFI_SETUP_EARLY > > > > #include <efi_loader.h> > > > > #endif > > > > +#ifdef CONFIG_FWU_MULTI_BANK_UPDATE > > > > +#include <fwu.h> > > > > +#endif > > > > > > > > DECLARE_GLOBAL_DATA_PTR; > > > > > > > > @@ -805,6 +808,9 @@ static init_fnc_t init_sequence_r[] = { > > > > #endif > > > > #ifdef CONFIG_EFI_SETUP_EARLY > > > > (init_fnc_t)efi_init_obj_list, > > > > +#endif > > > > +#ifdef CONFIG_FWU_MULTI_BANK_UPDATE > > > > + fwu_boottime_checks, > > > > #endif > > > > > > Maybe I don't understand your code well. > > > Why do you want to call fwu_boottime_checks() here? > > > Why not in CONFIG_EFI_CAPSULE_UPDATE_EARLY (i.e. efi_launch_capsules() > > > in main_loop())? > > > > > > > These are boot time checks which are to be carried out during platform > > boot. Do you see any issue if we place the call to fwu_boottime_checks in > > the init_sequence. > > No, I don't see any specific issue right now. > > > I would like to avoid having the fwu boot time checks > > tied up with CONFIG_EFI_CAPSULE_ON_DISK_EARLY. > > My question was why you want to avoid that. > In main_loop(), update_tftp(), as well as efi capsule code, is put there. > It would be a good practice to have similar functionality called in one > place. > Keeping the call to fwu_boottime_checks separate allows one to try it out separately from the u-boot shell, by not enabling CONFIG_EFI_CAPSULE_ON_DISK_EARLY. Also, currently, the FWU code is closely tied up with the capsule update code. But in the future, if someone wants to use the fwu metadata without using the capsule format for the update, that would be possible with the call being separate. But I agree that this is a hypothetical case which may or may not happen. If you don't have a strong opinion on this, I would prefer it be kept separate. Let me know. -sughosh > -Takahiro Akashi > > > > -sughosh > > > > > > > > > > -Takahiro Akashi > > > > > > > run_main_loop, > > > > }; > > > > diff --git a/include/fwu.h b/include/fwu.h > > > > index 5ba437798d..2d2e674d6a 100644 > > > > --- a/include/fwu.h > > > > +++ b/include/fwu.h > > > > @@ -16,6 +16,9 @@ > > > > EFI_GUID(0x8a7a84a0, 0x8387, 0x40f6, 0xab, 0x41, \ > > > > 0xa8, 0xb9, 0xa5, 0xa6, 0x0d, 0x23) > > > > > > > > +int fwu_boottime_checks(void); > > > > +u8 fwu_update_checks_pass(void); > > > > + > > > > int fwu_get_active_index(u32 *active_idx); > > > > int fwu_update_active_index(u32 active_idx); > > > > int fwu_get_image_alt_num(efi_guid_t image_type_id, u32 update_bank, > > > > diff --git a/lib/fwu_updates/fwu.c b/lib/fwu_updates/fwu.c > > > > new file mode 100644 > > > > index 0000000000..e964f9b0b1 > > > > --- /dev/null > > > > +++ b/lib/fwu_updates/fwu.c > > > > @@ -0,0 +1,163 @@ > > > > +// SPDX-License-Identifier: GPL-2.0+ > > > > +/* > > > > + * Copyright (c) 2021, Linaro Limited > > > > + */ > > > > + > > > > +#include <efi.h> > > > > +#include <efi_loader.h> > > > > +#include <efi_variable.h> > > > > +#include <fwu.h> > > > > +#include <fwu_mdata.h> > > > > +#include <malloc.h> > > > > + > > > > +#include <linux/errno.h> > > > > +#include <linux/types.h> > > > > + > > > > +static u8 trial_state = 0; > > > > +static u8 boottime_check = 0; > > > > + > > > > +static int fwu_trial_state_check(void) > > > > +{ > > > > + int ret, i; > > > > + efi_status_t status; > > > > + efi_uintn_t var_size; > > > > + u16 trial_state_ctr; > > > > + u32 nimages, active_bank, var_attributes, active_idx; > > > > + struct fwu_mdata *mdata = NULL; > > > > + struct fwu_image_entry *img_entry; > > > > + struct fwu_image_bank_info *img_bank_info; > > > > + > > > > + ret = fwu_get_mdata(&mdata); > > > > + if (ret < 0) > > > > + return ret; > > > > + > > > > + ret = 0; > > > > + nimages = CONFIG_FWU_NUM_IMAGES_PER_BANK; > > > > + active_bank = mdata->active_index; > > > > + img_entry = &mdata->img_entry[0]; > > > > + for (i = 0; i < nimages; i++) { > > > > + img_bank_info = > &img_entry[i].img_bank_info[active_bank]; > > > > + if (!img_bank_info->accepted) { > > > > + trial_state = 1; > > > > + break; > > > > + } > > > > + } > > > > + > > > > + if (trial_state) { > > > > + var_size = (efi_uintn_t)sizeof(trial_state_ctr); > > > > + log_info("System booting in Trial State\n"); > > > > + var_attributes = EFI_VARIABLE_NON_VOLATILE | > > > > + EFI_VARIABLE_BOOTSERVICE_ACCESS | > > > > + EFI_VARIABLE_RUNTIME_ACCESS; > > > > + status = efi_get_variable_int(L"TrialStateCtr", > > > > + > &efi_global_variable_guid, > > > > + &var_attributes, > > > > + &var_size, > &trial_state_ctr, > > > > + NULL); > > > > + if (status != EFI_SUCCESS) { > > > > + log_err("Unable to read TrialStateCtr > variable\n"); > > > > + ret = -1; > > > > + goto out; > > > > + } > > > > + > > > > + ++trial_state_ctr; > > > > + if (trial_state_ctr > CONFIG_FWU_TRIAL_STATE_CNT) { > > > > + log_info("Trial State count exceeded. Revert > back > > > to previous_active_index\n"); > > > > + active_idx = mdata->active_index; > > > > + ret = fwu_revert_boot_index(); > > > > + if (ret < 0) { > > > > + log_err("Unable to revert > active_index\n"); > > > > + goto out; > > > > + } > > > > + > > > > + trial_state_ctr = 0; > > > > + status = efi_set_variable_int(L"TrialStateCtr", > > > > + > > > &efi_global_variable_guid, > > > > + var_attributes, > > > > + 0, > > > > + &trial_state_ctr, > > > false); > > > > + if (status != EFI_SUCCESS) { > > > > + log_err("Unable to clear TrialStateCtr > > > variable\n"); > > > > + ret = -1; > > > > + goto out; > > > > + } > > > > + } else { > > > > + status = efi_set_variable_int(L"TrialStateCtr", > > > > + > > > &efi_global_variable_guid, > > > > + var_attributes, > > > > + var_size, > > > > + &trial_state_ctr, > > > false); > > > > + if (status != EFI_SUCCESS) { > > > > + log_err("Unable to increment > TrialStateCtr > > > variable\n"); > > > > + ret = -1; > > > > + goto out; > > > > + } else { > > > > + ret = 0; > > > > + } > > > > + } > > > > + } > > > > + > > > > +out: > > > > + free(mdata); > > > > + return ret; > > > > +} > > > > + > > > > +u8 fwu_update_checks_pass(void) > > > > +{ > > > > + return !trial_state && boottime_check; > > > > +} > > > > + > > > > +int fwu_boottime_checks(void) > > > > +{ > > > > + int ret; > > > > + u32 boot_idx, active_idx; > > > > + > > > > + ret = fwu_mdata_check(); > > > > + if (ret < 0) { > > > > + boottime_check = 0; > > > > + return 0; > > > > + } > > > > + > > > > + /* > > > > + * Get the Boot Index, i.e. the bank from > > > > + * which the platform has booted. This value > > > > + * gets passed from the ealier stage bootloader > > > > + * which booted u-boot, e.g. tf-a. If the > > > > + * boot index is not the same as the > > > > + * active_index read from the FWU metadata, > > > > + * update the active_index. > > > > + */ > > > > + fwu_plat_get_bootidx(&boot_idx); > > > > + if (boot_idx >= CONFIG_FWU_NUM_BANKS) { > > > > + log_err("Received incorrect value of boot_index\n"); > > > > + boottime_check = 0; > > > > + return 0; > > > > + } > > > > + > > > > + ret = fwu_get_active_index(&active_idx); > > > > + if (ret < 0) { > > > > + log_err("Unable to read active_index\n"); > > > > + boottime_check = 0; > > > > + return 0; > > > > + } > > > > + > > > > + if (boot_idx != active_idx) { > > > > + log_info("Boot idx %u is not matching active idx %u, > > > changing active_idx\n", > > > > + boot_idx, active_idx); > > > > + ret = fwu_update_active_index(boot_idx); > > > > + if (ret < 0) > > > > + boottime_check = 0; > > > > + else > > > > + boottime_check = 1; > > > > + > > > > + return 0; > > > > + } > > > > + > > > > + ret = fwu_trial_state_check(); > > > > + if (ret < 0) > > > > + boottime_check = 0; > > > > + else > > > > + boottime_check = 1; > > > > + > > > > + return 0; > > > > +} > > > > -- > > > > 2.17.1 > > > > > > > >
diff --git a/common/board_r.c b/common/board_r.c index 31a59c585a..81678870b9 100644 --- a/common/board_r.c +++ b/common/board_r.c @@ -78,6 +78,9 @@ #ifdef CONFIG_EFI_SETUP_EARLY #include <efi_loader.h> #endif +#ifdef CONFIG_FWU_MULTI_BANK_UPDATE +#include <fwu.h> +#endif DECLARE_GLOBAL_DATA_PTR; @@ -805,6 +808,9 @@ static init_fnc_t init_sequence_r[] = { #endif #ifdef CONFIG_EFI_SETUP_EARLY (init_fnc_t)efi_init_obj_list, +#endif +#ifdef CONFIG_FWU_MULTI_BANK_UPDATE + fwu_boottime_checks, #endif run_main_loop, }; diff --git a/include/fwu.h b/include/fwu.h index 5ba437798d..2d2e674d6a 100644 --- a/include/fwu.h +++ b/include/fwu.h @@ -16,6 +16,9 @@ EFI_GUID(0x8a7a84a0, 0x8387, 0x40f6, 0xab, 0x41, \ 0xa8, 0xb9, 0xa5, 0xa6, 0x0d, 0x23) +int fwu_boottime_checks(void); +u8 fwu_update_checks_pass(void); + int fwu_get_active_index(u32 *active_idx); int fwu_update_active_index(u32 active_idx); int fwu_get_image_alt_num(efi_guid_t image_type_id, u32 update_bank, diff --git a/lib/fwu_updates/fwu.c b/lib/fwu_updates/fwu.c new file mode 100644 index 0000000000..e964f9b0b1 --- /dev/null +++ b/lib/fwu_updates/fwu.c @@ -0,0 +1,163 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Copyright (c) 2021, Linaro Limited + */ + +#include <efi.h> +#include <efi_loader.h> +#include <efi_variable.h> +#include <fwu.h> +#include <fwu_mdata.h> +#include <malloc.h> + +#include <linux/errno.h> +#include <linux/types.h> + +static u8 trial_state = 0; +static u8 boottime_check = 0; + +static int fwu_trial_state_check(void) +{ + int ret, i; + efi_status_t status; + efi_uintn_t var_size; + u16 trial_state_ctr; + u32 nimages, active_bank, var_attributes, active_idx; + struct fwu_mdata *mdata = NULL; + struct fwu_image_entry *img_entry; + struct fwu_image_bank_info *img_bank_info; + + ret = fwu_get_mdata(&mdata); + if (ret < 0) + return ret; + + ret = 0; + nimages = CONFIG_FWU_NUM_IMAGES_PER_BANK; + active_bank = mdata->active_index; + img_entry = &mdata->img_entry[0]; + for (i = 0; i < nimages; i++) { + img_bank_info = &img_entry[i].img_bank_info[active_bank]; + if (!img_bank_info->accepted) { + trial_state = 1; + break; + } + } + + if (trial_state) { + var_size = (efi_uintn_t)sizeof(trial_state_ctr); + log_info("System booting in Trial State\n"); + var_attributes = EFI_VARIABLE_NON_VOLATILE | + EFI_VARIABLE_BOOTSERVICE_ACCESS | + EFI_VARIABLE_RUNTIME_ACCESS; + status = efi_get_variable_int(L"TrialStateCtr", + &efi_global_variable_guid, + &var_attributes, + &var_size, &trial_state_ctr, + NULL); + if (status != EFI_SUCCESS) { + log_err("Unable to read TrialStateCtr variable\n"); + ret = -1; + goto out; + } + + ++trial_state_ctr; + if (trial_state_ctr > CONFIG_FWU_TRIAL_STATE_CNT) { + log_info("Trial State count exceeded. Revert back to previous_active_index\n"); + active_idx = mdata->active_index; + ret = fwu_revert_boot_index(); + if (ret < 0) { + log_err("Unable to revert active_index\n"); + goto out; + } + + trial_state_ctr = 0; + status = efi_set_variable_int(L"TrialStateCtr", + &efi_global_variable_guid, + var_attributes, + 0, + &trial_state_ctr, false); + if (status != EFI_SUCCESS) { + log_err("Unable to clear TrialStateCtr variable\n"); + ret = -1; + goto out; + } + } else { + status = efi_set_variable_int(L"TrialStateCtr", + &efi_global_variable_guid, + var_attributes, + var_size, + &trial_state_ctr, false); + if (status != EFI_SUCCESS) { + log_err("Unable to increment TrialStateCtr variable\n"); + ret = -1; + goto out; + } else { + ret = 0; + } + } + } + +out: + free(mdata); + return ret; +} + +u8 fwu_update_checks_pass(void) +{ + return !trial_state && boottime_check; +} + +int fwu_boottime_checks(void) +{ + int ret; + u32 boot_idx, active_idx; + + ret = fwu_mdata_check(); + if (ret < 0) { + boottime_check = 0; + return 0; + } + + /* + * Get the Boot Index, i.e. the bank from + * which the platform has booted. This value + * gets passed from the ealier stage bootloader + * which booted u-boot, e.g. tf-a. If the + * boot index is not the same as the + * active_index read from the FWU metadata, + * update the active_index. + */ + fwu_plat_get_bootidx(&boot_idx); + if (boot_idx >= CONFIG_FWU_NUM_BANKS) { + log_err("Received incorrect value of boot_index\n"); + boottime_check = 0; + return 0; + } + + ret = fwu_get_active_index(&active_idx); + if (ret < 0) { + log_err("Unable to read active_index\n"); + boottime_check = 0; + return 0; + } + + if (boot_idx != active_idx) { + log_info("Boot idx %u is not matching active idx %u, changing active_idx\n", + boot_idx, active_idx); + ret = fwu_update_active_index(boot_idx); + if (ret < 0) + boottime_check = 0; + else + boottime_check = 1; + + return 0; + } + + ret = fwu_trial_state_check(); + if (ret < 0) + boottime_check = 0; + else + boottime_check = 1; + + return 0; +}
The FWU Multi Bank Update specification requires the Update Agent to carry out certain checks at the time of platform boot. The Update Agent is the component which is responsible for updating the firmware components and maintaining and keeping the metadata in sync. The spec requires that the Update Agent perform the following checks at the time of boot * Sanity check of both the metadata copies maintained by the platform. * Get the boot index passed to U-Boot by the prior stage bootloader and use this value for metadata bookkeeping. * Check if the system is booting in Trial State. If the system boots in the Trial State for more than a specified number of boot counts, change the Active Bank to be booting the platform from. Add these checks in the board initialisation sequence, invoked after relocation. Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> --- Changes since V1: * Define a funtion fwu_update_checks_pass to do the checks before initiating the update * Log the status of the boottime checks using boottime_check variable and allow system to boot instead of hanging the platform(fwu_boottime_checks) common/board_r.c | 6 ++ include/fwu.h | 3 + lib/fwu_updates/fwu.c | 163 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 172 insertions(+) create mode 100644 lib/fwu_updates/fwu.c