Re: [PATCH v7 08/13] FWU: Add boot time checks as highlighted by the FWU specification

From: Sughosh Ganu <sughosh.ganu@linaro.org>
To: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Cc: u-boot@lists.denx.de, Heinrich Schuchardt <xypron.glpk@gmx.de>,
	 Takahiro Akashi <takahiro.akashi@linaro.org>,
	 Patrick Delaunay <patrick.delaunay@foss.st.com>,
	 Patrice Chotard <patrice.chotard@foss.st.com>,
	Simon Glass <sjg@chromium.org>,  Bin Meng <bmeng.cn@gmail.com>,
	Tom Rini <trini@konsulko.com>,
	 Etienne Carriere <etienne.carriere@linaro.org>,
	Michal Simek <monstr@monstr.eu>,
	Jassi Brar <jaswinder.singh@linaro.org>
Subject: Re: [PATCH v7 08/13] FWU: Add boot time checks as highlighted by the FWU specification
Date: Wed, 27 Jul 2022 16:34:05 +0530	[thread overview]
Message-ID: <CADg8p95gTmX-dur_4HsvgMSNU9PH8sU35QcLtFJbbVLuojBPtA@mail.gmail.com> (raw)
In-Reply-To: <CAC_iWjKd54cNfefDHUhRv6Bi-Osi48bUEPhBLRYK9fG0N8N2jQ@mail.gmail.com>

hi Ilias,

On Wed, 20 Jul 2022 at 13:06, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Sughosh,
>
> >
> > > +       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;
> > > +               }
> > > +       }
> >
> > Is this used elsewhere in the patchset?  The function is starting to
> > be big, so perhaps moving this in a static bool "in_trial_state()" or
> > similar would make it more readable.
> >
>
> There was a discussion about this on the synquacer thread for A/B
> updates.  Once you split those in a function, it's better to extend
> the bootcount API with an EFI backed storage.  The reasoning that a
> user might disable editing env variables for security reasons and that
> device might not be able to preserve RAM or store the counter in CPU
> registers across reboots.
>
> If we extend the bootcount API with this code we can plug in the
> functionality seamlessly based on the hardware capabilities.

I have incorporated the rest of your comments on this patch. I moved
the access to the TrialStateCtr EFI variable to the bootcount API like
you suggested. However, I face an issue, in that on every boot, the
bootdelay_process function gets called which increments the bootcount
variable. This does not map with the requirement of the FWU feature
since the TrialStateCtr variable needs to be updated only when the
platform transitions to the Trial State, after a successful update.
Once the platform transitions from the Trial State to the Regular
State, the variable gets deleted. Hence I think it would be better to
use the current logic of the handling of the TrialStateCtr variable.
Thanks.

-sughosh

>
> [...]
>
> Regards
>
> /Ilias