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

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

Hi Sughosh, 

> > > +     }
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +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.
> 
> -sughosh

My previous reply wasn't that clear let me try again. 
Indeed the goto's aren't needed and that's the first confusing thing. On
top of that the fwu_trial_state_check() is a bit misleading as well, since
it does a lot more than checking. So that functions does
- check the trial state counter
- remove it if not on trial state
- bump the counter
- if the counter exceeds a threshold try to delete it
Also due to the fact that this runs from the event loop, the return codes
are a bit confusing

However this is only called from a fwu_boottime_checks() so we can break it
up in smaller pieces that would be easier to read. 
In fwu_trial_state_check() you only need the metadata to check whether you
are in trial state or not. 

So I would suggest 
1. Create a trial_counter_read() which only reads the EFI variable
2. move the metadata code in fwu_boottime_checks()a instead of
   fwu_trial_state_check()
3. rename fwu_trial_state_check() -> fwu_try_update_cnt()


static u8 trial_state; -> s/trial_state/in_trial/

static int fwu_boottime_checks(void *ctx, struct event *event)
{
	.....

	ret = fwu_get_dev_mdata(&dev, &mdata);
	if (ret)
		return ret;(NULL);
	
	in_trial = in_trial_state(&mdata) + 1;
	cnt = trial_counter_read();

	if (in_trial && cnt < CONFIG_FWU_TRIAL_STATE_CNT)
		ret = fwu_try_update_cnt()
		if (fail)
			trial_counter_update
	else
		trial_counter_update(NULL);
		
There will be an extra GetVariable call since we unconditionally read the
counter now, but we can add if (in_trial), although it doesn't matter that
much.
		
		
Can you give it a shot and see if that works for you?

Thanks
/Ilias

> 
> >
> > > +             } 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