diff mbox series

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

Message ID 20220915081451.633983-3-sughosh.ganu@linaro.org
State Superseded
Headers show
Series FWU: Add FWU Multi Bank Update feature support | expand

Commit Message

Sughosh Ganu Sept. 15, 2022, 8:14 a.m. UTC
In the FWU Multi Bank Update feature, the information about the
updatable images is stored as part of the metadata, which is stored on
a dedicated partition. Add the metadata structure, and a driver model
uclass which provides functions to access the metadata. These are
generic API's, and implementations can be added based on parameters
like how the metadata partition is accessed and what type of storage
device houses the metadata.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
Reviewed-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
---
Changes since V9:
* Add a helper function fwu_get_dev_mdata() to get the FWU metadata
  device and read the metadata.
* Rename fwu_get_image_alt_num() as fwu_get_image_image_index() based
  on suggestion from Takahiro.

 drivers/fwu-mdata/fwu-mdata-uclass.c | 107 +++++++++
 include/dm/uclass-id.h               |   1 +
 include/fwu.h                        | 214 +++++++++++++++++
 include/fwu_mdata.h                  |  67 ++++++
 lib/fwu_updates/fwu.c                | 333 +++++++++++++++++++++++++++
 5 files changed, 722 insertions(+)
 create mode 100644 drivers/fwu-mdata/fwu-mdata-uclass.c
 create mode 100644 include/fwu.h
 create mode 100644 include/fwu_mdata.h
 create mode 100644 lib/fwu_updates/fwu.c

Comments

Jassi Brar Sept. 19, 2022, 12:33 a.m. UTC | #1
On Thu, 15 Sept 2022 at 03:15, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
....
> +/**
> + * fwu_get_active_index() - Get active_index from the FWU metadata
> + * @active_idxp: active_index value to be read
> + *
> + * Read the active_index field from the FWU metadata and place it in
> + * the variable pointed to be the function argument.
> + *
> + * Return: 0 if OK, -ve on error
> + *
> + */
> +int fwu_get_active_index(u32 *active_idxp);
> +
Bank index is u32 here and uint in fwu_update_active_index(), so  s/u32/uint ?

cheers.
Sughosh Ganu Sept. 19, 2022, 12:39 p.m. UTC | #2
On Mon, 19 Sept 2022 at 06:03, Jassi Brar <jaswinder.singh@linaro.org> wrote:
>
> On Thu, 15 Sept 2022 at 03:15, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> ....
> > +/**
> > + * fwu_get_active_index() - Get active_index from the FWU metadata
> > + * @active_idxp: active_index value to be read
> > + *
> > + * Read the active_index field from the FWU metadata and place it in
> > + * the variable pointed to be the function argument.
> > + *
> > + * Return: 0 if OK, -ve on error
> > + *
> > + */
> > +int fwu_get_active_index(u32 *active_idxp);
> > +
> Bank index is u32 here and uint in fwu_update_active_index(), so  s/u32/uint ?

Will change. Thanks.

-sughosh
Jassi Brar Sept. 26, 2022, 2:57 a.m. UTC | #3
On Thu, Sep 15, 2022 at 3:15 AM Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
....
> +/**
> + * @mdata_check: check the validity of the FWU metadata partitions
> + * @get_mdata() - Get a FWU metadata copy
> + * @update_mdata() - Update the FWU metadata copy
> + */
> +struct fwu_mdata_ops {
> +       /**
> +        * mdata_check() - Check if the FWU metadata is valid
> +        * @dev:        FWU device
> +        *
> +        * Validate both copies of the FWU metadata. If one of the copies
> +        * has gone bad, restore it from the other bad copy.
> +        *
> +        * Return: 0 if OK, -ve on error
> +        */
> +       int (*mdata_check)(struct udevice *dev);
>
Like get_mdata and update_mdata, maybe  check_mdata too ?

.....
> +/**
> + * fwu_get_active_index() - Get active_index from the FWU metadata
> + * @active_idxp: active_index value to be read
> + *
> + * Read the active_index field from the FWU metadata and place it in
> + * the variable pointed to be the function argument.
> + *
> + * Return: 0 if OK, -ve on error
> + *
> + */
> +int fwu_get_active_index(u32 *active_idxp);
> +
> +/**
> + * fwu_update_active_index() - Update active_index from the FWU metadata
> + * @active_idx: active_index value to be updated
> + *
> + * Update the active_index field in the FWU metadata
> + *
> + * Return: 0 if OK, -ve on error
> + *
> + */
> +int fwu_update_active_index(uint active_idx);
>
maybe  fwu_set_active_index  ? just like fwu_get_active_index

.....
> +/**
> + * 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.

.....
> +/**
> + * 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() ?

cheers.
Sughosh Ganu Sept. 26, 2022, 10 a.m. UTC | #4
On Mon, 26 Sept 2022 at 08:28, Jassi Brar <jassisinghbrar@gmail.com> wrote:
>
> On Thu, Sep 15, 2022 at 3:15 AM Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> ....
> > +/**
> > + * @mdata_check: check the validity of the FWU metadata partitions
> > + * @get_mdata() - Get a FWU metadata copy
> > + * @update_mdata() - Update the FWU metadata copy
> > + */
> > +struct fwu_mdata_ops {
> > +       /**
> > +        * mdata_check() - Check if the FWU metadata is valid
> > +        * @dev:        FWU device
> > +        *
> > +        * Validate both copies of the FWU metadata. If one of the copies
> > +        * has gone bad, restore it from the other bad copy.
> > +        *
> > +        * Return: 0 if OK, -ve on error
> > +        */
> > +       int (*mdata_check)(struct udevice *dev);
> >
> Like get_mdata and update_mdata, maybe  check_mdata too ?

Okay

>
> .....
> > +/**
> > + * fwu_get_active_index() - Get active_index from the FWU metadata
> > + * @active_idxp: active_index value to be read
> > + *
> > + * Read the active_index field from the FWU metadata and place it in
> > + * the variable pointed to be the function argument.
> > + *
> > + * Return: 0 if OK, -ve on error
> > + *
> > + */
> > +int fwu_get_active_index(u32 *active_idxp);
> > +
> > +/**
> > + * fwu_update_active_index() - Update active_index from the FWU metadata
> > + * @active_idx: active_index value to be updated
> > + *
> > + * Update the active_index field in the FWU metadata
> > + *
> > + * Return: 0 if OK, -ve on error
> > + *
> > + */
> > +int fwu_update_active_index(uint active_idx);
> >
> maybe  fwu_set_active_index  ? just like fwu_get_active_index

Okay

>
> .....
> > +/**
> > + * 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. All that this function does is use the
previous_active_index as the partition/bank to boot from in the
subsequent boot cycle.

>
> .....
> > +/**
> > + * 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.

-sughosh
Jassi Brar Sept. 26, 2022, 2:42 p.m. UTC | #5
On Mon, Sep 26, 2022 at 5:00 AM Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> On Mon, 26 Sept 2022 at 08:28, Jassi Brar <jassisinghbrar@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).

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

> > .....
> > > +/**
> > > + * 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.
Sughosh Ganu Sept. 27, 2022, 7:14 a.m. UTC | #6
On Mon, 26 Sept 2022 at 20:12, Jassi Brar <jassisinghbrar@gmail.com> wrote:
>
> On Mon, Sep 26, 2022 at 5:00 AM Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > On Mon, 26 Sept 2022 at 08:28, Jassi Brar <jassisinghbrar@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.
Jassi Brar Sept. 27, 2022, 4:25 p.m. UTC | #7
On Tue, Sep 27, 2022 at 2:14 AM Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> On Mon, 26 Sept 2022 at 20:12, Jassi Brar <jassisinghbrar@gmail.com> wrote:
> >
> > On Mon, Sep 26, 2022 at 5:00 AM Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > >
> > > On Mon, 26 Sept 2022 at 08:28, Jassi Brar <jassisinghbrar@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.


> 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)?
Doesn't simply copying over the bank make more sense?

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

-j
Sughosh Ganu Sept. 28, 2022, 6 a.m. UTC | #8
On Tue, 27 Sept 2022 at 21:55, Jassi Brar <jassisinghbrar@gmail.com> wrote:
>
> On Tue, Sep 27, 2022 at 2:14 AM Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > On Mon, 26 Sept 2022 at 20:12, Jassi Brar <jassisinghbrar@gmail.com> wrote:
> > >
> > > On Mon, Sep 26, 2022 at 5:00 AM Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > >
> > > > On Mon, 26 Sept 2022 at 08:28, Jassi Brar <jassisinghbrar@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
Jassi Brar Sept. 28, 2022, 7:29 p.m. UTC | #9
On Wed, Sep 28, 2022 at 1:00 AM Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> On Tue, 27 Sept 2022 at 21:55, Jassi Brar <jassisinghbrar@gmail.com> wrote:
> >
> > On Tue, Sep 27, 2022 at 2:14 AM Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > >
> > > On Mon, 26 Sept 2022 at 20:12, Jassi Brar <jassisinghbrar@gmail.com> wrote:
> > > >
> > > > On Mon, Sep 26, 2022 at 5:00 AM Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > >
> > > > > On Mon, 26 Sept 2022 at 08:28, Jassi Brar <jassisinghbrar@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
Sughosh Ganu Sept. 29, 2022, 6:01 a.m. UTC | #10
On Thu, 29 Sept 2022 at 00:59, Jassi Brar <jassisinghbrar@gmail.com> wrote:
>
> On Wed, Sep 28, 2022 at 1:00 AM Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > On Tue, 27 Sept 2022 at 21:55, Jassi Brar <jassisinghbrar@gmail.com> wrote:
> > >
> > > On Tue, Sep 27, 2022 at 2:14 AM Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > >
> > > > On Mon, 26 Sept 2022 at 20:12, Jassi Brar <jassisinghbrar@gmail.com> wrote:
> > > > >
> > > > > On Mon, Sep 26, 2022 at 5:00 AM Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > > >
> > > > > > On Mon, 26 Sept 2022 at 08:28, Jassi Brar <jassisinghbrar@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
diff mbox series

Patch

diff --git a/drivers/fwu-mdata/fwu-mdata-uclass.c b/drivers/fwu-mdata/fwu-mdata-uclass.c
new file mode 100644
index 0000000000..65ae93c21f
--- /dev/null
+++ b/drivers/fwu-mdata/fwu-mdata-uclass.c
@@ -0,0 +1,107 @@ 
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2022, Linaro Limited
+ */
+
+#define LOG_CATEGORY UCLASS_FWU_MDATA
+
+#include <common.h>
+#include <dm.h>
+#include <efi_loader.h>
+#include <fwu.h>
+#include <fwu_mdata.h>
+#include <log.h>
+
+#include <linux/errno.h>
+#include <linux/types.h>
+#include <u-boot/crc.h>
+
+/**
+ * fwu_mdata_check() - Check if the FWU metadata is valid
+ * @dev: FWU metadata device
+ *
+ * Validate both copies of the FWU metadata. If one of the copies
+ * has gone bad, restore it from the other bad copy.
+ *
+ * Return: 0 if OK, -ve on error
+ *
+ */
+int fwu_mdata_check(struct udevice *dev)
+{
+	const struct fwu_mdata_ops *ops = device_get_ops(dev);
+
+	if (!ops->mdata_check) {
+		log_debug("mdata_check() method not defined\n");
+		return -ENOSYS;
+	}
+
+	return ops->mdata_check(dev);
+}
+
+/**
+ * fwu_get_mdata() - Get a FWU metadata copy
+ * @dev: FWU metadata device
+ * @mdata: Copy of the FWU metadata
+ *
+ * Get a valid copy of the FWU metadata.
+ *
+ * Note: This function is to be called first when modifying any fields
+ * in the metadata. The sequence of calls to modify any field in the
+ * metadata would  be 1) fwu_get_mdata 2) Modify metadata, followed by
+ * 3) fwu_update_mdata
+ *
+ * Return: 0 if OK, -ve on error
+ *
+ */
+int fwu_get_mdata(struct udevice *dev, struct fwu_mdata *mdata)
+{
+	const struct fwu_mdata_ops *ops = device_get_ops(dev);
+
+	if (!ops->get_mdata) {
+		log_debug("get_mdata() method not defined\n");
+		return -ENOSYS;
+	}
+
+	return ops->get_mdata(dev, mdata);
+}
+
+/**
+ * fwu_update_mdata() - Update the FWU metadata
+ * @dev: FWU metadata device
+ * @mdata: Copy of the FWU metadata
+ *
+ * Update the FWU metadata structure by writing to the
+ * FWU metadata partitions.
+ *
+ * Note: This function is not to be called directly to update the
+ * metadata fields. The sequence of function calls should be
+ * 1) fwu_get_mdata() 2) Modify the medata fields 3) fwu_update_mdata()
+ *
+ * Return: 0 if OK, -ve on error
+ *
+ */
+int fwu_update_mdata(struct udevice *dev, struct fwu_mdata *mdata)
+{
+	void *buf;
+	const struct fwu_mdata_ops *ops = device_get_ops(dev);
+
+	if (!ops->update_mdata) {
+		log_debug("get_mdata() method not defined\n");
+		return -ENOSYS;
+	}
+
+	/*
+	 * Calculate the crc32 for the updated FWU metadata
+	 * and put the updated value in the FWU metadata crc32
+	 * field
+	 */
+	buf = &mdata->version;
+	mdata->crc32 = crc32(0, buf, sizeof(*mdata) - sizeof(u32));
+
+	return ops->update_mdata(dev, mdata);
+}
+
+UCLASS_DRIVER(fwu_mdata) = {
+	.id		= UCLASS_FWU_MDATA,
+	.name		= "fwu-mdata",
+};
diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
index a432e43871..598a8c10a0 100644
--- a/include/dm/uclass-id.h
+++ b/include/dm/uclass-id.h
@@ -58,6 +58,7 @@  enum uclass_id {
 	UCLASS_FIRMWARE,	/* Firmware */
 	UCLASS_FUZZING_ENGINE,	/* Fuzzing engine */
 	UCLASS_FS_FIRMWARE_LOADER,		/* Generic loader */
+	UCLASS_FWU_MDATA,	/* FWU Metadata Access */
 	UCLASS_GPIO,		/* Bank of general-purpose I/O pins */
 	UCLASS_HASH,		/* Hash device */
 	UCLASS_HWSPINLOCK,	/* Hardware semaphores */
diff --git a/include/fwu.h b/include/fwu.h
new file mode 100644
index 0000000000..745f6225d0
--- /dev/null
+++ b/include/fwu.h
@@ -0,0 +1,214 @@ 
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright (c) 2022, Linaro Limited
+ */
+
+#if !defined _FWU_H_
+#define _FWU_H_
+
+#include <blk.h>
+#include <efi.h>
+
+#include <linux/types.h>
+
+struct fwu_mdata;
+struct udevice;
+
+/**
+ * @mdata_check: check the validity of the FWU metadata partitions
+ * @get_mdata() - Get a FWU metadata copy
+ * @update_mdata() - Update the FWU metadata copy
+ */
+struct fwu_mdata_ops {
+	/**
+	 * mdata_check() - Check if the FWU metadata is valid
+	 * @dev:	FWU device
+	 *
+	 * Validate both copies of the FWU metadata. If one of the copies
+	 * has gone bad, restore it from the other bad copy.
+	 *
+	 * Return: 0 if OK, -ve on error
+	 */
+	int (*mdata_check)(struct udevice *dev);
+
+	/**
+	 * get_mdata() - Get a FWU metadata copy
+	 * @dev:	FWU device
+	 * @mdata:	Pointer to FWU metadata
+	 *
+	 * Get a valid copy of the FWU metadata.
+	 *
+	 * Return: 0 if OK, -ve on error
+	 */
+	int (*get_mdata)(struct udevice *dev, struct fwu_mdata *mdata);
+
+	/**
+	 * fwu_update_mdata() - Update the FWU metadata
+	 * @dev:	FWU device
+	 * @mdata:	Copy of the FWU metadata
+	 *
+	 * Update the FWU metadata structure by writing to the
+	 * FWU metadata partitions.
+	 *
+	 * Return: 0 if OK, -ve on error
+	 */
+	int (*update_mdata)(struct udevice *dev, struct fwu_mdata *mdata);
+};
+
+#define FWU_MDATA_VERSION	0x1
+
+/*
+* GUID value defined in the FWU specification for identification
+* of the FWU metadata partition.
+*/
+#define FWU_MDATA_GUID \
+	EFI_GUID(0x8a7a84a0, 0x8387, 0x40f6, 0xab, 0x41, \
+		 0xa8, 0xb9, 0xa5, 0xa6, 0x0d, 0x23)
+
+/**
+ * fwu_get_mdata() - Get a FWU metadata copy
+ * @dev: FWU metadata device
+ * @mdata: Copy of the FWU metadata
+ *
+ * Get a valid copy of the FWU metadata.
+ *
+ * Note: This function is to be called first when modifying any fields
+ * in the metadata. The sequence of calls to modify any field in the
+ * metadata would  be 1) fwu_get_mdata 2) Modify metadata, followed by
+ * 3) fwu_update_mdata
+ *
+ * Return: 0 if OK, -ve on error
+ *
+ */
+int fwu_get_mdata(struct udevice *dev, struct fwu_mdata *mdata);
+
+/**
+ * fwu_update_mdata() - Update the FWU metadata
+ * @dev: FWU metadata device
+ * @mdata: Copy of the FWU metadata
+ *
+ * Update the FWU metadata structure by writing to the
+ * FWU metadata partitions.
+ *
+ * Note: This function is not to be called directly to update the
+ * metadata fields. The sequence of function calls should be
+ * 1) fwu_get_mdata() 2) Modify the medata fields 3) fwu_update_mdata()
+ *
+ * Return: 0 if OK, -ve on error
+ *
+ */
+int fwu_update_mdata(struct udevice *dev, struct fwu_mdata *mdata);
+
+/**
+ * fwu_get_active_index() - Get active_index from the FWU metadata
+ * @active_idxp: active_index value to be read
+ *
+ * Read the active_index field from the FWU metadata and place it in
+ * the variable pointed to be the function argument.
+ *
+ * Return: 0 if OK, -ve on error
+ *
+ */
+int fwu_get_active_index(u32 *active_idxp);
+
+/**
+ * fwu_update_active_index() - Update active_index from the FWU metadata
+ * @active_idx: active_index value to be updated
+ *
+ * Update the active_index field in the FWU metadata
+ *
+ * Return: 0 if OK, -ve on error
+ *
+ */
+int fwu_update_active_index(uint active_idx);
+
+/**
+ * fwu_get_image_index() - Get the Image Index to be used for capsule update
+ * @image_type_id: pointer to the image GUID as passed in the capsule
+ * @update_bank: Bank to which the update is to be made
+ * @image_index: The Image Index for the image
+ *
+ * Based on the GUID value passed in the capsule, along with the bank to which the
+ * image needs to be updated, get the Image Index value which will be used for the
+ * capsule update.
+ *
+ * Currently, the capsule update driver uses the DFU framework for
+ * the updates. This function gets the DFU alt number which is to
+ * be used as the Image Index
+ *
+ * Return: 0 if OK, -ve on error
+ *
+ */
+int fwu_get_image_index(efi_guid_t *image_type_id, u32 update_bank,
+			u8 *image_index);
+
+/**
+ * fwu_mdata_check() - Check if the FWU metadata is valid
+ * @dev: FWU metadata device
+ *
+ * Validate both copies of the FWU metadata. If one of the copies
+ * has gone bad, restore it from the other bad copy.
+ *
+ * Return: 0 if OK, -ve on error
+ *
+ */
+int fwu_mdata_check(struct udevice *dev);
+
+/**
+ * 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);
+
+/**
+ * fwu_verify_mdata() - Verify the FWU metadata
+ * @mdata: FWU metadata structure
+ * @pri_part: FWU metadata partition is primary or secondary
+ *
+ * Verify the FWU metadata by computing the CRC32 for the metadata
+ * structure and comparing it against the CRC32 value stored as part
+ * of the structure.
+ *
+ * Return: 0 if OK, -ve on error
+ *
+ */
+int fwu_verify_mdata(struct fwu_mdata *mdata, bool pri_part);
+
+/**
+ * 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);
+
+/**
+ * fwu_clear_accept_image() - Clear 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 cleared
+ *
+ * Clear the accepted bit for the image type specified by the img_type_id parameter.
+ * This function is called after the image has been updated. The accepted bit is
+ * cleared to be set subsequently after passing the image acceptance criteria, by
+ * either the OS(or firmware)
+ *
+ * Return: 0 if OK, -ve on error
+ *
+ */
+int fwu_clear_accept_image(efi_guid_t *img_type_id, u32 bank);
+
+#endif /* _FWU_H_ */
diff --git a/include/fwu_mdata.h b/include/fwu_mdata.h
new file mode 100644
index 0000000000..8fda4f4ac2
--- /dev/null
+++ b/include/fwu_mdata.h
@@ -0,0 +1,67 @@ 
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright (c) 2022, Linaro Limited
+ */
+
+#if !defined _FWU_MDATA_H_
+#define _FWU_MDATA_H_
+
+#include <efi.h>
+
+/**
+ * struct fwu_image_bank_info - firmware image information
+ * @image_uuid: Guid value of the image in this bank
+ * @accepted: Acceptance status of the image
+ * @reserved: Reserved
+ *
+ * The structure contains image specific fields which are
+ * used to identify the image and to specify the image's
+ * acceptance status
+ */
+struct fwu_image_bank_info {
+	efi_guid_t  image_uuid;
+	uint32_t accepted;
+	uint32_t reserved;
+};
+
+/**
+ * struct fwu_image_entry - information for a particular type of image
+ * @image_type_uuid: Guid value for identifying the image type
+ * @location_uuid: Guid of the storage volume where the image is located
+ * @img_bank_info: Array containing properties of images
+ *
+ * This structure contains information on various types of updatable
+ * firmware images. Each image type then contains an array of image
+ * information per bank.
+ */
+struct fwu_image_entry {
+	efi_guid_t image_type_uuid;
+	efi_guid_t location_uuid;
+	struct fwu_image_bank_info img_bank_info[CONFIG_FWU_NUM_BANKS];
+};
+
+/**
+ * struct fwu_mdata - FWU metadata structure for multi-bank updates
+ * @crc32: crc32 value for the FWU metadata
+ * @version: FWU metadata version
+ * @active_index: Index of the bank currently used for booting images
+ * @previous_active_inde: Index of the bank used before the current bank
+ *                        being used for booting
+ * @img_entry: Array of information on various firmware images that can
+ *             be updated
+ *
+ * This structure is used to store all the needed information for performing
+ * multi bank updates on the platform. This contains info on the bank being
+ * used to boot along with the information needed for identification of
+ * individual images
+ */
+struct fwu_mdata {
+	uint32_t crc32;
+	uint32_t version;
+	uint32_t active_index;
+	uint32_t previous_active_index;
+
+	struct fwu_image_entry img_entry[CONFIG_FWU_NUM_IMAGES_PER_BANK];
+};
+
+#endif /* _FWU_MDATA_H_ */
diff --git a/lib/fwu_updates/fwu.c b/lib/fwu_updates/fwu.c
new file mode 100644
index 0000000000..fb9dbca307
--- /dev/null
+++ b/lib/fwu_updates/fwu.c
@@ -0,0 +1,333 @@ 
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2022, Linaro Limited
+ */
+
+#include <dm.h>
+#include <efi_loader.h>
+#include <fwu.h>
+#include <fwu_mdata.h>
+#include <log.h>
+
+#include <linux/errno.h>
+#include <linux/types.h>
+#include <u-boot/crc.h>
+
+#define IMAGE_ACCEPT_SET	BIT(0)
+#define IMAGE_ACCEPT_CLEAR	BIT(1)
+
+static int fwu_get_dev_mdata(struct udevice **dev, struct fwu_mdata *mdata)
+
+{
+	int ret;
+
+	ret = uclass_first_device(UCLASS_FWU_MDATA, dev);
+	if (ret) {
+		log_debug("Cannot find fwu device\n");
+		return ret;
+	}
+
+	ret = fwu_get_mdata(*dev, mdata);
+	if (ret < 0)
+		log_debug("Unable to get valid FWU metadata\n");
+
+	return ret;
+}
+
+/**
+ * fwu_verify_mdata() - Verify the FWU metadata
+ * @mdata: FWU metadata structure
+ * @pri_part: FWU metadata partition is primary or secondary
+ *
+ * Verify the FWU metadata by computing the CRC32 for the metadata
+ * structure and comparing it against the CRC32 value stored as part
+ * of the structure.
+ *
+ * Return: 0 if OK, -ve on error
+ *
+ */
+int fwu_verify_mdata(struct fwu_mdata *mdata, bool pri_part)
+{
+	u32 calc_crc32;
+	void *buf;
+
+	buf = &mdata->version;
+	calc_crc32 = crc32(0, buf, sizeof(*mdata) - sizeof(u32));
+
+	if (calc_crc32 != mdata->crc32) {
+		log_debug("crc32 check failed for %s FWU metadata partition\n",
+			  pri_part ? "primary" : "secondary");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+/**
+ * fwu_get_active_index() - Get active_index from the FWU metadata
+ * @active_idx: active_index value to be read
+ *
+ * Read the active_index field from the FWU metadata and place it in
+ * the variable pointed to be the function argument.
+ *
+ * Return: 0 if OK, -ve on error
+ *
+ */
+int fwu_get_active_index(u32 *active_idx)
+{
+	int ret;
+	struct udevice *dev;
+	struct fwu_mdata mdata = { 0 };
+
+	ret = fwu_get_dev_mdata(&dev, &mdata);
+	if (ret)
+		return ret;
+
+	/*
+	 * Found the FWU metadata partition, now read the active_index
+	 * value
+	 */
+	*active_idx = mdata.active_index;
+	if (*active_idx >= CONFIG_FWU_NUM_BANKS) {
+		log_debug("Active index value read is incorrect\n");
+		ret = -EINVAL;
+	}
+
+	return ret;
+}
+
+/**
+ * fwu_update_active_index() - Update active_index from the FWU metadata
+ * @active_idx: active_index value to be updated
+ *
+ * Update the active_index field in the FWU metadata
+ *
+ * Return: 0 if OK, -ve on error
+ *
+ */
+int fwu_update_active_index(uint active_idx)
+{
+	int ret;
+	struct udevice *dev;
+	struct fwu_mdata mdata = { 0 };
+
+	if (active_idx >= CONFIG_FWU_NUM_BANKS) {
+		log_debug("Invalid active index value\n");
+		return -EINVAL;
+	}
+
+	ret = fwu_get_dev_mdata(&dev, &mdata);
+	if (ret)
+		return ret;
+
+	/*
+	 * Update the active index and previous_active_index fields
+	 * in the FWU metadata
+	 */
+	mdata.previous_active_index = mdata.active_index;
+	mdata.active_index = active_idx;
+
+	/*
+	 * Now write this updated FWU metadata to both the
+	 * FWU metadata partitions
+	 */
+	ret = fwu_update_mdata(dev, &mdata);
+	if (ret < 0) {
+		log_debug("Failed to update FWU metadata partitions\n");
+		ret = -EIO;
+	}
+
+	return ret;
+}
+
+/**
+ * fwu_get_image_index() - Get the Image Index to be used for capsule update
+ * @image_type_id: pointer to the image GUID as passed in the capsule
+ * @update_bank: Bank to which the update is to be made
+ * @image_index: The Image Index for the image
+ *
+ * Based on the GUID value passed in the capsule, along with the bank to which the
+ * image needs to be updated, get the Image Index value which will be used for the
+ * capsule update.
+ *
+ * Currently, the capsule update driver uses the DFU framework for
+ * the updates. This function gets the DFU alt number which is to
+ * be used as the Image Index
+ *
+ * Return: 0 if OK, -ve on error
+ *
+ */
+int fwu_get_image_index(efi_guid_t *image_type_id, u32 update_bank,
+			u8 *image_index)
+{
+	int ret, i;
+	efi_guid_t *image_guid;
+	struct udevice *dev;
+	struct fwu_mdata mdata = { 0 };
+	struct fwu_image_entry *img_entry;
+	struct fwu_image_bank_info *img_bank_info;
+
+	ret = fwu_get_dev_mdata(&dev, &mdata);
+	if (ret)
+		return ret;
+
+	ret = -EINVAL;
+	/*
+	 * The FWU metadata has been read. Now get the image_uuid for the
+	 * image with the update_bank.
+	 */
+	for (i = 0; i < CONFIG_FWU_NUM_IMAGES_PER_BANK; i++) {
+		if (!guidcmp(image_type_id,
+			     &mdata.img_entry[i].image_type_uuid)) {
+			img_entry = &mdata.img_entry[i];
+			img_bank_info = &img_entry->img_bank_info[update_bank];
+			image_guid = &img_bank_info->image_uuid;
+			ret = fwu_plat_get_alt_num(dev, image_guid, image_index);
+			if (ret) {
+				log_debug("alt_num not found for partition with GUID %pUs\n",
+					  image_guid);
+			} else {
+				log_debug("alt_num %d for partition %pUs\n",
+					  *image_index, image_guid);
+				++*image_index;
+			}
+
+			goto out;
+		}
+	}
+
+	log_debug("Partition with the image type %pUs not found\n",
+		  image_type_id);
+
+out:
+	return ret;
+}
+
+/**
+ * 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;
+
+	/*
+	 * Now write this updated FWU metadata to both the
+	 * FWU metadata partitions
+	 */
+	ret = fwu_update_mdata(dev, &mdata);
+	if (ret < 0) {
+		log_debug("Failed to update FWU metadata partitions\n");
+		ret = -EIO;
+	}
+
+	return ret;
+}
+
+/**
+ * fwu_clrset_image_accept() - Set or Clear the Acceptance bit for the image
+ * @img_type_id: GUID of the image type for which the accepted bit is to be
+ *               set or cleared
+ * @bank: Bank of which the image's Accept bit is to be set or cleared
+ * @action: Action which specifies whether image's Accept bit is to be set or
+ *          cleared
+ *
+ * Set/Clear the accepted bit for the image specified by the img_guid parameter.
+ * This indicates acceptance or rejection of image for subsequent boots by some
+ * governing component like OS(or firmware).
+ *
+ * Return: 0 if OK, -ve on error
+ *
+ */
+static int fwu_clrset_image_accept(efi_guid_t *img_type_id, u32 bank, u8 action)
+{
+	int ret, i;
+	struct udevice *dev;
+	struct fwu_mdata mdata = { 0 };
+	struct fwu_image_entry *img_entry;
+	struct fwu_image_bank_info *img_bank_info;
+
+	ret = fwu_get_dev_mdata(&dev, &mdata);
+	if (ret)
+		return ret;
+
+	img_entry = &mdata.img_entry[0];
+	for (i = 0; i < CONFIG_FWU_NUM_IMAGES_PER_BANK; i++) {
+		if (!guidcmp(&img_entry[i].image_type_uuid, img_type_id)) {
+			img_bank_info = &img_entry[i].img_bank_info[bank];
+			if (action == IMAGE_ACCEPT_SET)
+				img_bank_info->accepted |= FWU_IMAGE_ACCEPTED;
+			else
+				img_bank_info->accepted = 0;
+
+			ret = fwu_update_mdata(dev, &mdata);
+			goto out;
+		}
+	}
+
+	/* Image not found */
+	ret = -ENOENT;
+
+out:
+	return ret;
+}
+
+/**
+ * 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
+ * @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 cleared
+ *
+ * Clear the accepted bit for the image type specified by the img_type_id parameter.
+ * This function is called after the image has been updated. The accepted bit is
+ * cleared to be set subsequently after passing the image acceptance criteria, by
+ * either the OS(or firmware)
+ *
+ * Return: 0 if OK, -ve on error
+ *
+ */
+int fwu_clear_accept_image(efi_guid_t *img_type_id, u32 bank)
+{
+	return fwu_clrset_image_accept(img_type_id, bank,
+				       IMAGE_ACCEPT_CLEAR);
+}