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

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