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

[Sughosh Ganu <sughosh.ganu@linaro.org>]

hi Ilias,

On Wed, 7 Sept 2022 at 12:15, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Sughosh,
>
> On Fri, Aug 26, 2022 at 03:27:03PM +0530, Sughosh Ganu wrote:
> > 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>
> > ---
> > Changes since V8:
> > * Declare the metadata struct on the stack rather than heap
> > * Move the API's to fwu.c and only keep the DM ops in the uclass
> >   driver
> > * Fix return codes as suggested by Simon
> > * Change log_err to log_debug as suggested by Simon
> > * Remove the __packed attribute on the metadata structures as
> >   suggested by Simon
> > * Add comments to the API functions
> >
> >  drivers/fwu-mdata/fwu-mdata-uclass.c | 107 ++++++++
> >  include/dm/uclass-id.h               |   1 +
> >  include/fwu.h                        | 210 ++++++++++++++++
> >  include/fwu_mdata.h                  |  67 +++++
> >  lib/fwu_updates/fwu.c                | 357 +++++++++++++++++++++++++++
> >  5 files changed, 742 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

<snip>

> > diff --git a/lib/fwu_updates/fwu.c b/lib/fwu_updates/fwu.c
> > new file mode 100644
> > index 0000000000..a871d77b4c
> > --- /dev/null
> > +++ b/lib/fwu_updates/fwu.c
> > @@ -0,0 +1,357 @@
> > +// 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(struct udevice **dev)
> > +
> > +{
> > +     int ret;
> > +
> > +     ret = uclass_first_device(UCLASS_FWU_MDATA, dev);
> > +     if (ret) {
> > +             log_debug("Cannot find fwu device\n");
> > +             return ret;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +/**
> > + * 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(&dev);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = fwu_get_mdata(dev, &mdata);
> > +     if (ret < 0) {
> > +             log_debug("Unable to get valid FWU metadata\n");
> > +             goto out;
> > +     }
>
>
> This pattern is repeated on the entire patch.  Moreover the struct udevice
> *dev is seems only needed for calling fwu_get_mdata().  So is there any
> reason why can we can't change fwu_get_mdata() and fold in the fwu_get_dev()?

Sure, I will move the fetching of the device to fwu_get_mdata(). Will
also remove the braces as per your other review comment. Thanks.

-sughosh

>
> > +
> > +     /*
> > +      * 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;
> > +     }
> > +
> > +out:
> > +     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(&dev);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = fwu_get_mdata(dev, &mdata);
> > +     if (ret < 0) {
> > +             log_debug("Unable to get valid FWU metadata\n");
> > +             goto out;
> > +     }
> > +
> > +     /*
> > +      * 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;
> > +     }
> > +
> > +out:
> > +     return ret;
> > +}
> > +
> > +/**
> > + * fwu_get_image_alt_num() - Get the dfu alt number 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
> > + * @alt_num: The alt_num 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 dfu alt number which will be used for the
> > + * capsule update
> > + *
> > + * Return: 0 if OK, -ve on error
> > + *
> > + */
> > +int fwu_get_image_alt_num(efi_guid_t *image_type_id, u32 update_bank,
> > +                       int *alt_num)
> > +{
> > +     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(&dev);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = fwu_get_mdata(dev, &mdata);
> > +     if (ret) {
> > +             log_debug("Unable to get valid FWU metadata\n");
> > +             goto out;
> > +     }
> > +
> > +     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, alt_num);
> > +                     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",
> > +                                       *alt_num, image_guid);
> > +                     }
>
> Nit you don't need {}
>
> > +
> > +                     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(&dev);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = fwu_get_mdata(dev, &mdata);
> > +     if (ret < 0) {
> > +             log_debug("Unable to get valid FWU metadata\n");
> > +             goto out;
> > +     }
> > +
> > +     /*
> > +      * 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;
> > +     }
> > +
> > +out:
> > +     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(&dev);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = fwu_get_mdata(dev, &mdata);
> > +     if (ret < 0) {
> > +             log_debug("Unable to get valid FWU metadata\n");
> > +             goto out;
> > +     }
> > +
> > +     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);
> > +}
> > --
> > 2.34.1
> >
>
>
> Thanks
> /Ilias