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

[Sughosh Ganu]

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.

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

> 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 and 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. The implementation of the FWU spec
requires using the UEFI capsule update interface for the updates. This
would require some thought on how to use the FMP functions to copy
over the images to other banks in a generic way. But please note that
even in case of num_banks > 2, this can be done through the normal
capsule update flow if the platform implements a round robin update
bank selection.

>
>  >
> > > > All that this function does is use the
> > > > previous_active_index as the partition/bank to boot from in the
> > > > subsequent boot cycle.
> > > >
> > > That is, you assume the previous_active_index bank contains working images.
> >
> > It is the responsibility of the platform owner to ensure that all
> > partitions have valid images.
> >
> I differ. The platform should not be modifying banks and meta-data
> beneath the fwu framework.
> The specification says "A Client can only read from or write to images
> in the update bank"

I did not say modifying the banks through a parallel mechanism. Like I
mentioned above, this can be done through the normal update flow.

-sughosh