[PATCH v10 09/15] FWU: Add boot time checks as highlighted by the FWU specification

[Sughosh Ganu]

On Mon, 26 Sept 2022 at 08:29, Jassi Brar <jassisinghbrar at gmail.com> wrote:
>
> On Thu, Sep 15, 2022 at 3:17 AM Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
>
> ....
> > diff --git a/include/fwu.h b/include/fwu.h
> > index 484289ed4f..d5f77ce83c 100644
> > --- a/include/fwu.h
> > +++ b/include/fwu.h
> > @@ -256,4 +256,17 @@ int fwu_plat_get_update_index(uint *update_idx);
> >   *
> >   */
> >  void fwu_plat_get_bootidx(uint *boot_idx);
> >
> Or simply return the boot_idx instead of modifying the pointed variable

This is following the prototype that is being used for all the
functions. Would prefer to have that consistency.

>
> .....
> > diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
> > index c633fcd91e..557e97de4a 100644
> > --- a/lib/efi_loader/efi_setup.c
> > +++ b/lib/efi_loader/efi_setup.c
> > @@ -199,6 +199,7 @@ static efi_status_t __efi_init_early(void)
> >                 goto out;
> >
> >         ret = efi_disk_init();
> > +
> >  out:
> >         return ret;
> >  }
> We can do without this change in this patchset :)

Will remove.

>
>
> .....
> > +static int fwu_trial_state_check(struct udevice *dev)
> > +{
> > +       int ret;
> > +       efi_status_t status;
> > +       efi_uintn_t var_size;
> > +       u16 trial_state_ctr;
> > +       u32 var_attributes;
> > +       struct fwu_mdata mdata = { 0 };
> > +
> > +       ret = fwu_get_mdata(dev, &mdata);
> > +       if (ret)
> > +               return ret;
> > +
> > +       if ((trial_state = in_trial_state(&mdata))) {
> >
> This may raise warnings on code checkers. So maybe move the assignment
> out of the check.

I did get a compiler warning asking me to use the parentheses around
the assignment. But I can move the assignment out.

>
>
> .....
> > +static int fwu_boottime_checks(void *ctx, struct event *event)
> > +{
> > +       int ret;
> > +       struct udevice *dev;
> > +       u32 boot_idx, active_idx;
> > +
> > +       ret = fwu_get_dev_mdata(&dev, NULL);
> > +       if (ret)
> > +               return ret;
> > +
> > +       ret = fwu_mdata_check(dev);
> > +       if (ret) {
> > +               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");
> > +               return 0;
> > +       }
> > +
> > +       ret = fwu_get_active_index(&active_idx);
> > +       if (ret) {
> > +               log_err("Unable to read active_index\n");
> > +               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)
> > +                       boottime_check = 1;
> >
> We may not want to do anything FWU (accept, reject, modify mdata)
> until we reboot, if we are recovering from last bad upgrade. So maybe
> not set boottime_check

Actually, the difference between the boot bank and active bank will
happen when there is some kind of corruption on the media due to which
the platform could not boot from the active bank(could also be due to
repeated wd timeouts). What issue do you see in attempting to update
the other bank in case of a mismatch.

-sughosh