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

[Sughosh Ganu]

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

>
> > 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. For e.g. a platform which implements
rollback protection would have this scenario in the case where, after
updating a bank with images, the platform's rollback counter is
incremented. This would mean that the previous_active_index(all other
banks) no longer contains valid/bootable images. In such a scenario,
the platform owner would have to ensure that all banks are updated
with valid images. It is not the role to be played by the update code
to identify this situation. The FWU code is simply for updating the
images which are part of capsules to the update bank.

-sughosh

>
> > > .....
> > > > +/**
> > > > + * fwu_accept_image() - Set the Acceptance bit for the image
> > > > + * @img_type_id: GUID of the image type for which the accepted bit is to be
> > > > + *               cleared
> > > > + * @bank: Bank of which the image's Accept bit is to be set
> > > > + *
> > > > + * Set the accepted bit for the image specified by the img_guid parameter. This
> > > > + * indicates acceptance of image for subsequent boots by some governing component
> > > > + * like OS(or firmware).
> > > > + *
> > > > + * Return: 0 if OK, -ve on error
> > > > + *
> > > > + */
> > > > +int fwu_accept_image(efi_guid_t *img_type_id, u32 bank)
> > > > +{
> > > > +       return fwu_clrset_image_accept(img_type_id, bank,
> > > > +                                      IMAGE_ACCEPT_SET);
> > > > +}
> > > > +
> > > > +/**
> > > > + * fwu_clear_accept_image() - Clear the Acceptance bit for the image
> > > >
> > > Something more consistent like fwu_image_accepted_clear()  and
> > > fwu_image_accepted_set() ?
> >
> > Umm, the other related API is fwu_accept_image, and this is clearing
> > the accept bit, hence the name. If you don't feel strongly about this,
> > I would prefer the current name.
> >
> fwu_accept_image() and fwu_clear_accept_image()  don't seem like a
> pair.... is all I say.
>
> cheers.