[PATCH v10 02/15] FWU: Add FWU metadata structure and driver for accessing metadata

[Sughosh Ganu]

On Thu, 29 Sept 2022 at 00:59, Jassi Brar <jassisinghbrar at gmail.com> wrote:
>
> On Wed, Sep 28, 2022 at 1:00 AM Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> >
> > On Tue, 27 Sept 2022 at 21:55, Jassi Brar <jassisinghbrar at gmail.com> wrote:
> > >
> > > On Tue, Sep 27, 2022 at 2:14 AM Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > > >
> > > > On Mon, 26 Sept 2022 at 20:12, Jassi Brar <jassisinghbrar at gmail.com> wrote:
> > > > >
> > > > > On Mon, Sep 26, 2022 at 5:00 AM Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > > > > >
> > > > > > On Mon, 26 Sept 2022 at 08:28, Jassi Brar <jassisinghbrar at gmail.com> wrote:
> > > > > > >
> > > > >
> > > > > > >
> > > > > > > .....
> > > > > > > > +/**
> > > > > > > > + * fwu_revert_boot_index() - Revert the active index in the FWU metadata
> > > > > > > > + *
> > > > > > > > + * Revert the active_index value in the FWU metadata, by swapping the values
> > > > > > > > + * of active_index and previous_active_index in both copies of the
> > > > > > > > + * FWU metadata.
> > > > > > > > + *
> > > > > > > > + * Return: 0 if OK, -ve on error
> > > > > > > > + *
> > > > > > > > + */
> > > > > > > > +int fwu_revert_boot_index(void)
> > > > > > > > +{
> > > > > > > > +       int ret;
> > > > > > > > +       u32 cur_active_index;
> > > > > > > > +       struct udevice *dev;
> > > > > > > > +       struct fwu_mdata mdata = { 0 };
> > > > > > > > +
> > > > > > > > +       ret = fwu_get_dev_mdata(&dev, &mdata);
> > > > > > > > +       if (ret)
> > > > > > > > +               return ret;
> > > > > > > > +
> > > > > > > > +       /*
> > > > > > > > +        * Swap the active index and previous_active_index fields
> > > > > > > > +        * in the FWU metadata
> > > > > > > > +        */
> > > > > > > > +       cur_active_index = mdata.active_index;
> > > > > > > > +       mdata.active_index = mdata.previous_active_index;
> > > > > > > > +       mdata.previous_active_index = cur_active_index;
> > > > > > > >
> > > > > > > This may cause problems.
> > > > > > > We are reverting because active_index does not work, and here we set
> > > > > > > it to previous_active_index which is supposed to mean "last good
> > > > > > > index".
> > > > > > >  Also this logic assumes a 2-banks setup, and is obviously incorrect
> > > > > > > for >2 banks where the previous_active_index should point to
> > > > > > > "boot_index minus 2" bank (but of course there is no guarantee that
> > > > > > > that bank is preserved still).
> > > > > > >  So either previous_active_index be left changed OR we also copy the
> > > > > > > previous bank to active bank before the swap.
> > > > > >
> > > > > > Sorry, but I don't understand the review comment here. Even in the
> > > > > > case of num_banks > 2, this function is simply using the
> > > > > > previous_active_index value. It does not care what the
> > > > > > previous_active_index value is. If you remember, the setting of the
> > > > > > update bank is really a platform
> > > > > > function(fwu_plat_get_update_index()). A platform can set any bank
> > > > > > number as the update bank. So we cannot tell what the value of the
> > > > > > previous_active_index will be.
> > > > > >
> > > > > Do you remember you pick update_bank in a circular-buffer manner in
> > > > > fwu_plat_get_update_index() ? But don't even bother the >2 banks.
> > > > >
> > > > > Consider the simple 2-banks platform....
> > > > > Initially:
> > > > >        active_index = 1
> > > > >        previous_active_index = 0
> > > > >
> > > > > After update and before reboot
> > > > >        active_index = 0                  <<<< updated bank 0
> > > > >        previous_active_index = 1
> > > > >
> > > > > After reboot, for some reason update fails (reject bank0) and we call
> > > > > fwu_revert_boot_index()
> > > > >        active_index = 1                    <<< good
> > > > >        previous_active_index = 0    <<<< points to unbootable bank
> > > > >
> > > > > Which may be seen as inconsistency if we assume previous_bank to
> > > > > always contain a bootable set of images.
> > > > > So we also need to copy bank1 into bank0 as part of the revert (at
> > > > > least as a backup for reasons other than a/b update failure).
> > > >
> > > > If the platform owner wants to restore a particular bank with good
> > > > images, the procedure to update that bank needs to be followed just
> > > > like it was any other update.
> > > >
> > > The banks are under FWU and the platform has (should have) no control
> > > over which bank the image goes in.
> >
> > It is the platform code that can decide on how the update banks are
> > selected. Even in the case of num_banks > 2, the platform can have a
> > round robin selection of the update banks through which all the other
> > banks can be restored/updated. And currently, the default function for
> > selection of the update bank does use round robin selection of the
> > banks.
> >
> Platform only reads from and writes to a bank number given by the fwu.
> Platform has no means to sense if a bank is accepted or rejected. Or
> is there?

What I meant is that the bank to which the update will be written to
is governed by the function fwu_plat_get_update_index(), which can be
provided by every platform. And the platform can compute the update
bank in a round robin fashion. This is another way to restore a bank
from which the images are not booting for any reason(like rollback
protection). Unless there is a hardware issue in the storage device,
which will require manual intervention. So what I am trying to say
here is that there is a way the rest of the banks can be restored
without having to copy the banks.

>
>
> > >
> > > > If an updated bank fails the image
> > > > acceptance test, the following boot would be from the
> > > > previous_active_bank. In that case, the other bank needs to be updated
> > > > by explicitly putting capsules in the ESP and initiating the update.
> > > >
> > > Which capsule - the one that just failed or the previous one (which
> > > may not be available/provided)?
> >
> > One with working images. Please note that in the normal flow, a user
> > would have tested the images locally before getting them deployed.
> >
> In the "normal flow" of the world many industries shouldn't exist -
> security, anti-virus, customer-support, repair etc :)
>
> > > Doesn't simply copying over the bank make more sense?
> >
> > When we are talking about "simply copying", I hope you appreciate that
> > 1) this(restoration of the banks) is not under the purview of the FWU
> > spec
> >
> I think it is. The restoration is part of the management of the
> banks...  just like FWU makes sure metadata replicas are always in
> sync.

Okay. I did not find mention of this in the FWU specification. Can you
point me to this.

>
> 2) it will need some thought on how to have a generic
> > implementation which fetches images from the good bank and then writes
> > them over to the other banks.
> >
> Not from "good bank" to "other banks".
> FWU only needs to copy from previous_bank to active_bank before
> swapping their pointers in fwu_revert_boot_index() maybe by a platform
> specific callback copy_bank(int from, int to) ?

Yes, but you might have a situation where all the other banks need to
be updated, say due to rollback protection kicking in. I was talking
from that point of view.

>
>
> > The implementation of the FWU spec
> > requires using the UEFI capsule update interface for the updates.
> >
> The spec actually expects FWU to reside in BL32, but here we are
> implementing it in BL33 (I know we had to).

Not really. The spec does actually mention this scenario of the Update
Agent and Client both residing in the normal world. This is a valid
scenario that is described by the FWU spec, pg 39, "Normal World
controlled FW store".

> IMO, always keeping a good previous_bank is not even a violation of
> the spec, it is internal to the implementation.

Yes, I take your point. Only thing I was trying to highlight is that
it can be achieved even without the copy.

-sughosh

>
> But ok. I don't want to be the one to slow down the uboot's fwu
> progress. We can always implement it later when more people see the
> need.
>
> -jassi