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

Jassi Brar jassisinghbrar at gmail.com
Wed Sep 28 21:29:35 CEST 2022


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?


> >
> > > 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.

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) ?


> 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).
IMO, always keeping a good previous_bank is not even a violation of
the spec, it is internal to the implementation.

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


More information about the U-Boot mailing list