[PATCH v4 07/11] FWU: Add boot time checks as highlighted by the FWU specification
Sughosh Ganu
sughosh.ganu at linaro.org
Fri Feb 11 11:46:48 CET 2022
On Tue, 8 Feb 2022 at 16:23, Michal Simek <monstr at monstr.eu> wrote:
>
> po 7. 2. 2022 v 19:22 odesílatel Sughosh Ganu <sughosh.ganu at linaro.org> napsal:
> >
> > 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 at linaro.org>
> > ---
> >
> > Changes since V3:
> > * Change the TrialStateCtr efi variable attribute to remove the
> > runtime attribute
> >
> > common/board_r.c | 6 ++
> > include/fwu.h | 3 +
> > lib/fwu_updates/fwu.c | 178 ++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 187 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
> > run_main_loop,
> > };
> > diff --git a/include/fwu.h b/include/fwu.h
> > index 90b8cd41e5..5a329d9ef7 100644
> > --- a/include/fwu.h
> > +++ b/include/fwu.h
> > @@ -37,6 +37,9 @@ struct fwu_mdata_ops {
> > 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_mdata(struct fwu_mdata **mdata);
> > int fwu_update_mdata(struct fwu_mdata *mdata);
> > int fwu_get_active_index(u32 *active_idx);
> > diff --git a/lib/fwu_updates/fwu.c b/lib/fwu_updates/fwu.c
> > new file mode 100644
> > index 0000000000..86933123e7
> > --- /dev/null
> > +++ b/lib/fwu_updates/fwu.c
> > @@ -0,0 +1,178 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (c) 2021, Linaro Limited
> > + */
> > +
> > +#include <dm.h>
> > +#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;
>
> you don't need to initialize them to 0. They are in bss which is setup
> to 0 already.
>
> > +
> > +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)
>
> Isn't it easier to have if (ret) here?
>
> > + return ret;
> > +
> > + ret = 0;
>
> Then you can remove this and the whole ret handling below is weird.
>
>
> > + 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;
> > + 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;
>
> Any reason not to use standard return macros? The same I see below too.
This is because the ret is just used to indicate whether the function
has returned successfully, or not. The value is not getting passed
anywhere from the caller function -- it is only used to either return
back or continue with further processing. And the error reason is also
being printed in all cases.
I have incorporated all your other comments. Thanks.
-sughosh
>
> > + 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) {
>
> I would use if (ret) here.
>
> > + 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;
>
> Why do you need this? ret should be 0 when you get here.
>
> > + }
> > + }
> > + } else {
> > + trial_state_ctr = 0;
> > + ret = 0;
>
> the same here. You have ret=0 above that's why I think this is just an
> additional line here.
>
> > + status = efi_set_variable_int(L"TrialStateCtr",
> > + &efi_global_variable_guid,
> > + 0,
> > + 0, &trial_state_ctr,
> > + NULL);
> > + }
> > +
> > +out:
> > + free(mdata);
> > + return ret;
> > +}
> > +
> > +u8 fwu_update_checks_pass(void)
> > +{
> > + return !trial_state && boottime_check;
> > +}
> > +
> > +int fwu_boottime_checks(void)
> > +{
> > + int ret;
> > + struct udevice *dev;
> > + u32 boot_idx, active_idx;
> > +
> > + if (uclass_get_device(UCLASS_FWU_MDATA, 0, &dev) || !dev) {
> > + log_err("FWU Metadata device not found\n");
> > + boottime_check = 0;
>
> All these boottime_check initialization to 0 seems to me useless.
>
> > + return 0;
> > + }
> > +
> > + ret = fwu_mdata_check();
> > + if (ret < 0) {
>
> up to you but I would also use if (ret) here.
>
> > + boottime_check = 0;
>
> It is zero already when you get here that's why all these = 0 lines
> should be IMHO removed.
>
> > + 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;
>
> ditto.
>
> > + return 0;
> > + }
> > +
> > + ret = fwu_get_active_index(&active_idx);
> > + if (ret < 0) {
>
> ditto.
>
> > + log_err("Unable to read active_index\n");
> > + boottime_check = 0;
>
> ditto.
>
> > + 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)
>
> ditto.
>
> > + boottime_check = 0;
>
> ditto
>
> > + else
> > + boottime_check = 1;
> > +
> > + return 0;
> > + }
> > +
> > + ret = fwu_trial_state_check();
> > + if (ret < 0)
>
> ditto
>
> > + boottime_check = 0;
>
> ditto.
>
> > + else
> > + boottime_check = 1;
> > +
> > + return 0;
> > +}
> > --
> > 2.17.1
> >
>
> Thanks,
> Michal
>
> --
> Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
> w: www.monstr.eu p: +42-0-721842854
> Maintainer of Linux kernel - Xilinx Microblaze
> Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
> U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP/Versal SoCs
More information about the U-Boot
mailing list