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

[Ilias Apalodimas]

Hi Sughosh,

[...]

> > > +
> > > +static int fwu_trial_state_check(void)
> > > +{
> > > +     int ret;
> > > +     struct udevice *dev;
> > > +     efi_status_t status;
> > > +     efi_uintn_t var_size;
> > > +     u16 trial_state_ctr;
> > > +     struct fwu_mdata mdata = { 0 };
> > > +
> > > +     ret = fwu_get_dev_mdata(&dev, &mdata);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     trial_state = in_trial_state(&mdata);
> > > +     if (trial_state) {
> > > +             var_size = (efi_uintn_t)sizeof(trial_state_ctr);
> > > +             log_info("System booting in Trial State\n");
> > > +             status = efi_get_variable_int(u"TrialStateCtr",
> > > +                                           &efi_global_variable_guid,
> > > +                                           NULL,
> > > +                                           &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");
> > > +                     ret = fwu_revert_boot_index();
> > > +                     if (ret) {
> > > +                             log_err("Unable to revert active_index\n");
> > > +                             goto out;
> > > +                     }
> > > +
> > > +                     /* Delete the TrialStateCtr variable */
> > > +                     ret = trial_counter_update(NULL);
> > > +                     if (ret) {
> > > +                             log_err("Unable to delete TrialStateCtr variable\n");
> > > +                             goto out;
> > > +                     }
> >
> > This is a bit confusing for me.  If the trial_state_ctr we need to goto out
> > anyway right?  So why don't we explicitly add a goto out at the end and get
> > rid of the else that's following ?
>
> Actually, we don't need the goto statement above, as well as the one
> used below, in the else part. I can get rid of it. Personally I feel
> that this provides more clarity as to how the code flow is, but I can
> get rid of it if you so prefer. Thanks.

I prefer keeping a single goto and getting rid of the else tbh.  It's
normal code flow you execute while having a single error check

Thanks
/Ilias
>
> -sughosh
>
> >
> > > +             } else {
> > > +                     ret = trial_counter_update(&trial_state_ctr);
> > > +                     if (ret) {
> > > +                             log_err("Unable to increment TrialStateCtr variable\n");
> > > +                             goto out;
> > > +                     }
> > > +             }
> > > +     } else {
> > > +             /* Delete the variable */
> > > +             ret = trial_counter_update(NULL);
> > > +             if (ret) {
> > > +                     log_err("Unable to delete TrialStateCtr variable\n");
> > > +             }
> > > +     }
> > > +
> > > +out:
> > > +     return ret;
> > > +}
> > > +
> > >  static int fwu_get_image_type_id(u8 *image_index, efi_guid_t *image_type_id)
> > >  {
> > >       u8 index;
> > > @@ -494,3 +607,69 @@ __weak int fwu_plat_get_update_index(uint *update_idx)
> > >
> > >       return ret;
> > >  }
> > > +
> > > +/**
> > > + * fwu_update_checks_pass() - Check if FWU update can be done
> > > + *
> > > + * Check if the FWU update can be executed. The updates are
> > > + * allowed only when the platform is not in Trial State and
> > > + * the boot time checks have passed
> > > + *
> > > + * Return: 1 if OK, 0 on error
> > > + *
> > > + */
> > > +u8 fwu_update_checks_pass(void)
> > > +{
> > > +     return !trial_state && boottime_check;
> > > +}
> > > +
> > > +static int fwu_boottime_checks(void *ctx, struct event *event)
> > > +{
> > > +     int ret;
> > > +     u32 boot_idx, active_idx;
> > > +
> > > +     ret = fwu_check_mdata_validity();
> > > +     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_set_active_index(boot_idx);
> > > +             if (!ret)
> > > +                     boottime_check = 1;
> > > +
> > > +             return 0;
> > > +     }
> > > +
> > > +     if (efi_init_obj_list() != EFI_SUCCESS)
> > > +             return 0;
> > > +
> > > +     ret = fwu_trial_state_check();
> > > +     if (!ret)
> > > +             boottime_check = 1;
> > > +
> > > +     return 0;
> > > +}
> > > +EVENT_SPY(EVT_MAIN_LOOP, fwu_boottime_checks);
> > > --
> > > 2.34.1
> > >
> >
> > Thanks
> > /Ilias