hi Etienne,

On Fri, 30 Sept 2022 at 11:24, Etienne Carriere etienne.carriere@linaro.org wrote:

Hello Sughosh,

On Wed, 28 Sept 2022 at 11:30, Sughosh Ganu sughosh.ganu@linaro.org 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 Reviewed-by: Etienne Carriere etienne.carriere@linaro.org

---

Changes since V10:

• s/fwu_update_active_index/fwu_set_active_index as per comment from Jassi
• Change the argument type of fwu_set_active_index() to uint from u32 as per comment from Jassi
• s/mdata_check/check_mdata as per comment from Jassi
• Fix the typo in the function comment of fwu_mdata_check()

drivers/fwu-mdata/fwu-mdata-uclass.c | 107 ++++++++ include/dm/uclass-id.h | 1 + include/fwu.h | 211 ++++++++++++++++ include/fwu_mdata.h | 67 +++++ lib/fwu_updates/fwu.c | 363 +++++++++++++++++++++++++++ 5 files changed, 749 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..fe8058bc37 --- /dev/null +++ b/lib/fwu_updates/fwu.c @@ -0,0 +1,363 @@ +// 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)

Nitpicking: i think an enumerated type would be better here. (not a strong opinion)

Okay

...

+static int fwu_get_dev_mdata(struct udevice **dev, struct fwu_mdata *mdata)

Remove this empty line.

Will do

...

+{

•   int ret;
  

•   ret = uclass_first_device(UCLASS_FWU_MDATA, dev);
  

•   if (ret) {
  

•           log_debug("Cannot find fwu device\n");
  

•           return ret;
  

•   }
  

`uclass_first_device()` returns 0 when node is not found but no issue reported while parsing the FDT (learned by experience :(. In that case, *dev is set to NULL and next instruction segfaults. Proposal:

 ret = uclass_first_device(UCLASS_FWU_MDATA, dev);

• if (ret) {

• if (ret || !*dev) { log_debug("Cannot find fwu device\n");

•    if (!ret)
  

•        ret = -ENOENT;
     return ret;
  

  }

I think I can instead use uclass_first_device_err(). I think that is doing what you are proposing above.

...

•   ret = fwu_get_mdata(*dev, mdata);
  

•   if (ret < 0)
  

•           log_debug("Unable to get valid FWU metadata\n");
  

•   return ret;
  

+}

+static int fwu_get_image_type_id(u8 *image_index, efi_guid_t *image_type_id) +{

•   u8 index;
  

•   int i;
  

•   struct efi_fw_image *image;
  

•   index = *image_index;
  

•   image = update_info.images;
  

•   for (i = 0; i < num_image_type_guids; i++) {
  

•           if (index == image[i].image_index) {
  

•                   guidcpy(image_type_id, &image[i].image_type_id);
  

•                   return 0;
  

•           }
  

•   }
  

•   return -ENOENT;
  

+}

+/**

• • 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(uint *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_set_active_index() - Set active_index in the FWU metadata
• • @active_idx: active_index value to be set
• • Update the active_index field in the FWU metadata
• • Return: 0 if OK, -ve on error
• */

+int fwu_set_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) {
  

Function description says it returns 0 on success so prefer here 'if (ret)'.

Okay

...

•           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_index: The Image Index for the image
• • The FWU multi bank update feature computes the value of image_index at
• • runtime, based on the bank to which the image needs to be written to.
• • Derive the image_index value for the image.
• • 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(u8 *image_index) +{

•   int ret, i;
  

•   u8 alt_num;
  

•   u32 update_bank;
  

•   efi_guid_t *image_guid, image_type_id;
  

•   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 = fwu_plat_get_update_index(&update_bank);
  

•   if (ret) {
  

•           log_debug("Failed to get the FWU update bank\n");
  

•           goto out;
  

•   }
  

•   ret = fwu_get_image_type_id(image_index, &image_type_id);
  

•   if (ret) {
  

•           log_debug("Unable to get image_type_id for image_index %u\n",
  

•                     *image_index);
  

•           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);
  

•                           *image_index = alt_num + 1;
  

•                   }
  

•                   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) {
  

Same comment here: error case should be trapped with 'if (ret)'.

Okay

-sughosh

br, etienne

...

•           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);
  

+}

2.34.1

Hi Sughosh,

My R-b tag still applies, with the 2 below comments addressed or not.

Br, etienne

On Wed, 28 Sept 2022 at 11:30, Sughosh Ganu sughosh.ganu@linaro.org wrote:

In the FWU Multi Bank Update feature, the information about the updatable images is stored as part of the metadata, on a separate partition. Add a driver for reading from and writing to the metadata when the updatable images and the metadata are stored on a block device which is formatted with GPT based partition scheme.

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 V10:

• Use array for storing the metadata partition numbers as suggested by Ilias
• Skip a couple of goto's in gpt_check_mdata_validity() as suggested by Ilias
• Have the FWU_MDATA_GPT_BLK config symbol depend on FWU_MDATA and BLK
• Remove the comment in fwu_gpt_mdata_check() as it is already mentioned in the corresponding API comment

drivers/fwu-mdata/Kconfig | 16 ++ drivers/fwu-mdata/Makefile | 8 + drivers/fwu-mdata/gpt_blk.c | 356 ++++++++++++++++++++++++++++++++++++ include/fwu.h | 4 + 4 files changed, 384 insertions(+) create mode 100644 drivers/fwu-mdata/Kconfig create mode 100644 drivers/fwu-mdata/Makefile create mode 100644 drivers/fwu-mdata/gpt_blk.c

diff --git a/drivers/fwu-mdata/Kconfig b/drivers/fwu-mdata/Kconfig new file mode 100644 index 0000000000..36c4479a59 --- /dev/null +++ b/drivers/fwu-mdata/Kconfig @@ -0,0 +1,16 @@ +config FWU_MDATA

•   bool "Driver support for accessing FWU Metadata"
  

•   depends on DM
  

•   help
  

•     Enable support for accessing FWU Metadata partitions. The
  

•     FWU Metadata partitions reside on the same storage device
  

•     which contains the other FWU updatable firmware images.
  

+config FWU_MDATA_GPT_BLK

•   bool "FWU Metadata access for GPT partitioned Block devices"
  

•   select PARTITION_TYPE_GUID
  

•   select PARTITION_UUIDS
  

•   depends on FWU_MDATA && BLK && EFI_PARTITION
  

•   help
  

•     Enable support for accessing FWU Metadata on GPT partitioned
  

•     block devices.
  

diff --git a/drivers/fwu-mdata/Makefile b/drivers/fwu-mdata/Makefile new file mode 100644 index 0000000000..3fee64c10c --- /dev/null +++ b/drivers/fwu-mdata/Makefile @@ -0,0 +1,8 @@ +# SPDX-License-Identifier: GPL-2.0-or-later +# +# Copyright (c) 2022, Linaro Limited +#

+obj-$(CONFIG_FWU_MDATA) += fwu-mdata-uclass.o +obj-$(CONFIG_FWU_MDATA_GPT_BLK) += gpt_blk.o diff --git a/drivers/fwu-mdata/gpt_blk.c b/drivers/fwu-mdata/gpt_blk.c new file mode 100644 index 0000000000..d20bb19d52 --- /dev/null +++ b/drivers/fwu-mdata/gpt_blk.c @@ -0,0 +1,356 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/*

• • Copyright (c) 2022, Linaro Limited
• */

+#define LOG_CATEGORY UCLASS_FWU_MDATA

+#include <blk.h> +#include <dm.h> +#include <efi_loader.h> +#include <fwu.h> +#include <fwu_mdata.h> +#include <log.h> +#include <memalign.h> +#include <part.h> +#include <part_efi.h>

+#include <dm/device-internal.h> +#include <linux/errno.h> +#include <linux/types.h>

+#define PRIMARY_PART BIT(0) +#define SECONDARY_PART BIT(1) +#define BOTH_PARTS (PRIMARY_PART | SECONDARY_PART)

+#define MDATA_READ BIT(0) +#define MDATA_WRITE BIT(1)

+static int gpt_get_mdata_partitions(struct blk_desc *desc,

•                               uint *mdata_parts)
  

Maybe nitpicking but I think this should better explicit declare a 2 cell array argument: static int gpt_get_mdata_partitions(struct blk_desc *desc, uint mdata_parts[2])

+{

•   int i, ret;
  

•   u32 nparts;
  

•   efi_guid_t part_type_guid;
  

•   struct disk_partition info;
  

•   const efi_guid_t fwu_mdata_guid = FWU_MDATA_GUID;
  

•   nparts = 0;
  

•   for (i = 1; i < MAX_SEARCH_PARTITIONS; i++) {
  

•           if (part_get_info(desc, i, &info))
  

•                   continue;
  

•           uuid_str_to_bin(info.type_guid, part_type_guid.b,
  

•                           UUID_STR_FORMAT_GUID);
  

•           if (nparts < 2 && !guidcmp(&fwu_mdata_guid, &part_type_guid)) {
  

•                   mdata_parts[nparts] = i;
  

•                   ++nparts;
  

•           }
  

•   }
  

•   if (nparts != 2) {
  

•           log_debug("Expect two copies of the FWU metadata instead of %d\n",
  

•                     nparts);
  

•           ret = -EINVAL;
  

•   } else {
  

•           ret = 0;
  

•   }
  

•   return ret;
  

+}

+static int gpt_get_mdata_disk_part(struct blk_desc *desc,

•                              struct disk_partition *info,
  

•                              u32 part_num)
  

+{

•   int ret;
  

•   char *mdata_guid_str = "8a7a84a0-8387-40f6-ab41-a8b9a5a60d23";
  

•   ret = part_get_info(desc, part_num, info);
  

•   if (ret < 0) {
  

•           log_debug("Unable to get the partition info for the FWU metadata part %d\n",
  

•                     part_num);
  

•           return -ENOENT;
  

•   }
  

•   /* Check that it is indeed the FWU metadata partition */
  

•   if (!strncmp(info->type_guid, mdata_guid_str, UUID_STR_LEN)) {
  

•           /* Found the FWU metadata partition */
  

•           return 0;
  

•   }
  

•   return -ENOENT;
  

+}

+static int gpt_read_write_mdata(struct blk_desc *desc,

•                           struct fwu_mdata *mdata,
  

•                           u8 access, u32 part_num)
  

+{

•   int ret;
  

•   u32 len, blk_start, blkcnt;
  

•   struct disk_partition info;
  

•   ALLOC_CACHE_ALIGN_BUFFER_PAD(struct fwu_mdata, mdata_aligned, 1,
  

•                                desc->blksz);
  

•   ret = gpt_get_mdata_disk_part(desc, &info, part_num);
  

•   if (ret < 0) {
  

•           printf("Unable to get the FWU metadata partition\n");
  

•           return -ENOENT;
  

•   }
  

•   len = sizeof(*mdata);
  

•   blkcnt = BLOCK_CNT(len, desc);
  

•   if (blkcnt > info.size) {
  

•           log_debug("Block count exceeds FWU metadata partition size\n");
  

•           return -ERANGE;
  

•   }
  

•   blk_start = info.start;
  

•   if (access == MDATA_READ) {
  

•           if (blk_dread(desc, blk_start, blkcnt, mdata_aligned) != blkcnt) {
  

•                   log_debug("Error reading FWU metadata from the device\n");
  

•                   return -EIO;
  

•           }
  

•           memcpy(mdata, mdata_aligned, sizeof(struct fwu_mdata));
  

•   } else {
  

•           if (blk_dwrite(desc, blk_start, blkcnt, mdata) != blkcnt) {
  

•                   log_debug("Error writing FWU metadata to the device\n");
  

•                   return -EIO;
  

•           }
  

•   }
  

•   return 0;
  

+}

+static int gpt_read_mdata(struct blk_desc *desc,

•                     struct fwu_mdata *mdata, u32 part_num)
  

+{

•   return gpt_read_write_mdata(desc, mdata, MDATA_READ, part_num);
  

+}

+static int gpt_write_mdata_partition(struct blk_desc *desc,

•                                struct fwu_mdata *mdata,
  

•                                u32 part_num)
  

+{

•   return gpt_read_write_mdata(desc, mdata, MDATA_WRITE, part_num);
  

+}

+static int fwu_gpt_update_mdata(struct udevice *dev, struct fwu_mdata *mdata) +{

•   int ret;
  

•   struct blk_desc *desc;
  

•   uint mdata_parts[2];
  

•   struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
  

•   desc = dev_get_uclass_plat(priv->blk_dev);
  

•   ret = gpt_get_mdata_partitions(desc, mdata_parts);
  

•   if (ret < 0) {
  

•           log_debug("Error getting the FWU metadata partitions\n");
  

•           return -ENOENT;
  

•   }
  

•   /* First write the primary partition */
  

•   ret = gpt_write_mdata_partition(desc, mdata, mdata_parts[0]);
  

•   if (ret < 0) {
  

•           log_debug("Updating primary FWU metadata partition failed\n");
  

•           return ret;
  

•   }
  

•   /* And now the replica */
  

•   ret = gpt_write_mdata_partition(desc, mdata, mdata_parts[1]);
  

•   if (ret < 0) {
  

•           log_debug("Updating secondary FWU metadata partition failed\n");
  

•           return ret;
  

•   }
  

•   return 0;
  

+}

+static int gpt_get_mdata(struct blk_desc *desc, struct fwu_mdata *mdata) +{

•   int ret;
  

•   uint mdata_parts[2];
  

•   ret = gpt_get_mdata_partitions(desc, mdata_parts);
  

•   if (ret < 0) {
  

•           log_debug("Error getting the FWU metadata partitions\n");
  

•           return -ENOENT;
  

•   }
  

•   ret = gpt_read_mdata(desc, mdata, mdata_parts[0]);
  

•   if (ret < 0) {
  

•           log_debug("Failed to read the FWU metadata from the device\n");
  

•           return -EIO;
  

•   }
  

•   ret = fwu_verify_mdata(mdata, 1);
  

•   if (!ret)
  

•           return 0;
  

•   /*
  

•    * Verification of the primary FWU metadata copy failed.
  

•    * Try to read the replica.
  

•    */
  

•   memset(mdata, '\0', sizeof(struct fwu_mdata));
  

•   ret = gpt_read_mdata(desc, mdata, mdata_parts[1]);
  

•   if (ret < 0) {
  

•           log_debug("Failed to read the FWU metadata from the device\n");
  

•           return -EIO;
  

•   }
  

•   ret = fwu_verify_mdata(mdata, 0);
  

•   if (!ret)
  

•           return 0;
  

•   /* Both the FWU metadata copies are corrupted. */
  

•   return -EIO;
  

+}

+static int gpt_check_mdata_validity(struct udevice *dev) +{

•   int ret;
  

•   struct blk_desc *desc;
  

•   struct fwu_mdata pri_mdata;
  

•   struct fwu_mdata secondary_mdata;
  

•   uint mdata_parts[2];
  

•   uint valid_partitions, invalid_partitions;
  

•   struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
  

•   desc = dev_get_uclass_plat(priv->blk_dev);
  

•   /*
  

•    * Two FWU metadata partitions are expected.
  

•    * If we don't have two, user needs to create
  

•    * them first
  

•    */
  

•   valid_partitions = 0;
  

•   ret = gpt_get_mdata_partitions(desc, mdata_parts);
  

•   if (ret < 0) {
  

•           log_debug("Error getting the FWU metadata partitions\n");
  

•           return -ENOENT;
  

•   }
  

•   ret = gpt_read_mdata(desc, &pri_mdata, mdata_parts[0]);
  

•   if (!ret) {
  

•           ret = fwu_verify_mdata(&pri_mdata, 1);
  

•           if (!ret)
  

•                   valid_partitions |= PRIMARY_PART;
  

•   }
  

•   /* Now check the secondary partition */
  

•   ret = gpt_read_mdata(desc, &secondary_mdata, mdata_parts[1]);
  

•   if (!ret) {
  

•           ret = fwu_verify_mdata(&secondary_mdata, 0);
  

•           if (!ret)
  

•                   valid_partitions |= SECONDARY_PART;
  

•   }
  

•   if (valid_partitions == (PRIMARY_PART | SECONDARY_PART)) {
  

•           ret = -1;
  

•           /*
  

•            * Before returning, check that both the
  

•            * FWU metadata copies are the same. If not,
  

•            * the FWU metadata copies need to be
  

•            * re-populated.
  

•            */
  

•           if (!memcmp(&pri_mdata, &secondary_mdata,
  

•                       sizeof(struct fwu_mdata))) {
  

•                   ret = 0;
  

•           } else {
  

•                   log_debug("Both FWU metadata copies are valid but do not match. Please check!\n");
  

•           }
  

•           goto out;
  

•   }
  

•   ret = -1;
  

•   if (!(valid_partitions & BOTH_PARTS))
  

•           goto out;
  

The 3 above preload of @ret are a bit misplaced (yet no functional issue) i think. Better a classic way:

if (valid_partitions == (PRIMARY_PART | SECONDARY_PART)) { - ret = -1; (...) if (!memcmp(&pri_mdata, &secondary_mdata, sizeof(struct fwu_mdata))) { ret = 0; } else { log_debug("Both FWU metadata copies are valid but do not match. Please check!\n"); + ret = -1; } goto out; }

- ret = -1; if (!(valid_partitions & BOTH_PARTS)) { + ret = -1; goto out; }

•   invalid_partitions = valid_partitions ^ BOTH_PARTS;
  

•   ret = gpt_write_mdata_partition(desc,
  

•                                   (invalid_partitions == PRIMARY_PART) ?
  

•                                   &secondary_mdata : &pri_mdata,
  

•                                   (invalid_partitions == PRIMARY_PART) ?
  

•                                   mdata_parts[0] : mdata_parts[1]);
  

•   if (ret < 0)
  

•           log_debug("Restoring %s FWU metadata partition failed\n",
  

•                     (invalid_partitions == PRIMARY_PART) ?
  

•                     "primary" : "secondary");
  

+out:

•   return ret;
  

+}

+static int fwu_gpt_mdata_check(struct udevice *dev) +{

•   return gpt_check_mdata_validity(dev);
  

+}

+static int fwu_gpt_get_mdata(struct udevice *dev, struct fwu_mdata *mdata) +{

•   struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
  

•   return gpt_get_mdata(dev_get_uclass_plat(priv->blk_dev), mdata);
  

+}

+static int fwu_get_mdata_device(struct udevice *dev, struct udevice **mdata_dev) +{

•   u32 phandle;
  

•   int ret, size;
  

•   struct udevice *parent;
  

•   const fdt32_t *phandle_p = NULL;
  

•   phandle_p = dev_read_prop(dev, "fwu-mdata-store", &size);
  

•   if (!phandle_p) {
  

•           log_debug("fwu-mdata-store property not found\n");
  

•           return -ENOENT;
  

•   }
  

•   phandle = fdt32_to_cpu(*phandle_p);
  

•   ret = device_get_global_by_ofnode(ofnode_get_by_phandle(phandle),
  

•                                     &parent);
  

•   if (ret)
  

•           return ret;
  

•   return blk_get_from_parent(parent, mdata_dev);
  

+}

+static int fwu_mdata_gpt_blk_probe(struct udevice *dev) +{

•   int ret;
  

•   struct udevice *mdata_dev = NULL;
  

•   struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
  

•   ret = fwu_get_mdata_device(dev, &mdata_dev);
  

•   if (ret)
  

•           return ret;
  

•   priv->blk_dev = mdata_dev;
  

•   return 0;
  

+}

+static const struct fwu_mdata_ops fwu_gpt_blk_ops = {

•   .check_mdata = fwu_gpt_mdata_check,
  

•   .get_mdata = fwu_gpt_get_mdata,
  

•   .update_mdata = fwu_gpt_update_mdata,
  

+};

+static const struct udevice_id fwu_mdata_ids[] = {

•   { .compatible = "u-boot,fwu-mdata-gpt" },
  

•   { }
  

+};

+U_BOOT_DRIVER(fwu_mdata_gpt_blk) = {

•   .name           = "fwu-mdata-gpt-blk",
  

•   .id             = UCLASS_FWU_MDATA,
  

•   .of_match       = fwu_mdata_ids,
  

•   .ops            = &fwu_gpt_blk_ops,
  

•   .probe          = fwu_mdata_gpt_blk_probe,
  

•   .priv_auto      = sizeof(struct fwu_mdata_gpt_blk_priv),
  

+}; diff --git a/include/fwu.h b/include/fwu.h index a4d2d35d9a..167f91388e 100644 --- a/include/fwu.h +++ b/include/fwu.h @@ -14,6 +14,10 @@ struct fwu_mdata; struct udevice;

+struct fwu_mdata_gpt_blk_priv {

•   struct udevice *blk_dev;
  

+};

/**

• @mdata_check: check the validity of the FWU metadata partitions
• @get_mdata() - Get a FWU metadata copy

-- 2.34.1

Hi Sughosh,

On Wed, Sep 28, 2022 at 02:59:46PM +0530, Sughosh Ganu wrote:

Enabling capsule update functionality on the platform requires populating information on the images that are to be updated using the functionality. Do so for the DK2 board.

Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org Reviewed-by: Patrick Delaunay patrick.delaunay@foss.st.com Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org

---

Changes since V10:

• Use image_index value of 1 for the FIP image as it is now relevant

board/st/stm32mp1/stm32mp1.c | 18 ++++++++++++++++++ include/configs/stm32mp15_common.h | 4 ++++ 2 files changed, 22 insertions(+)

diff --git a/board/st/stm32mp1/stm32mp1.c b/board/st/stm32mp1/stm32mp1.c index 8c162b42a5..e43dab018f 100644 --- a/board/st/stm32mp1/stm32mp1.c +++ b/board/st/stm32mp1/stm32mp1.c @@ -11,6 +11,7 @@ #include <clk.h> #include <config.h> #include <dm.h> +#include <efi_loader.h> #include <env.h> #include <env_internal.h> #include <fdt_simplefb.h> @@ -87,6 +88,16 @@ #define USB_START_LOW_THRESHOLD_UV 1230000 #define USB_START_HIGH_THRESHOLD_UV 2150000

+#if CONFIG_IS_ENABLED(EFI_HAVE_CAPSULE_SUPPORT) +struct efi_fw_image fw_images[1];

+struct efi_capsule_update_info update_info = {

• .images = fw_images,

+};

+u8 num_image_type_guids = ARRAY_SIZE(fw_images);

The definition of num_image_type_guids is always the same across boards. Why do we need it for every platform? I believe that we can remove it. Anyhow it's not documented in doc/develop/uefi/uefi.rst.

+#endif /* EFI_HAVE_CAPSULE_SUPPORT */

int board_early_init_f(void) { /* nothing to do, only used in SPL */ @@ -666,6 +677,13 @@ int board_init(void)

setup_led(LEDST_ON);

+#if CONFIG_IS_ENABLED(EFI_HAVE_CAPSULE_SUPPORT)

• efi_guid_t image_type_guid = STM32MP_FIP_IMAGE_GUID;
• guidcpy(&fw_images[0].image_type_id, &image_type_guid);
• fw_images[0].fw_name = u"STM32MP-FIP";
• fw_images[0].image_index = 1;

Then, we should describe that image_index must be 1 to num_image_type_guids (or strictly, number of descriptors returned by GetImageInfo()) in the document above. (I hope that you add a sanity checker against it as well.)

-Takahiro Akashi

+#endif return 0; }

diff --git a/include/configs/stm32mp15_common.h b/include/configs/stm32mp15_common.h index c5412ffeb3..bb19dae945 100644 --- a/include/configs/stm32mp15_common.h +++ b/include/configs/stm32mp15_common.h @@ -34,6 +34,10 @@ #define CONFIG_SERVERIP 192.168.1.1 #endif

+#define STM32MP_FIP_IMAGE_GUID \

• EFI_GUID(0x19d5df83, 0x11b0, 0x457b, 0xbe, 0x2c, \

•  0x75, 0x59, 0xc1, 0x31, 0x42, 0xa5)
  

/*****************************************************************************/ #ifdef CONFIG_DISTRO_DEFAULTS /*****************************************************************************/ -- 2.34.1

On Mon, Oct 03, 2022 at 04:40:04PM +0530, Sughosh Ganu wrote:

hi Takahiro,

On Mon, 3 Oct 2022 at 16:27, Takahiro Akashi takahiro.akashi@linaro.org wrote:

...

Hi Sughosh,

On Wed, Sep 28, 2022 at 02:59:46PM +0530, Sughosh Ganu wrote:

...

Enabling capsule update functionality on the platform requires populating information on the images that are to be updated using the functionality. Do so for the DK2 board.

Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org Reviewed-by: Patrick Delaunay patrick.delaunay@foss.st.com Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org

---

Changes since V10:

• Use image_index value of 1 for the FIP image as it is now relevant

board/st/stm32mp1/stm32mp1.c | 18 ++++++++++++++++++ include/configs/stm32mp15_common.h | 4 ++++ 2 files changed, 22 insertions(+)

diff --git a/board/st/stm32mp1/stm32mp1.c b/board/st/stm32mp1/stm32mp1.c index 8c162b42a5..e43dab018f 100644 --- a/board/st/stm32mp1/stm32mp1.c +++ b/board/st/stm32mp1/stm32mp1.c @@ -11,6 +11,7 @@ #include <clk.h> #include <config.h> #include <dm.h> +#include <efi_loader.h> #include <env.h> #include <env_internal.h> #include <fdt_simplefb.h> @@ -87,6 +88,16 @@ #define USB_START_LOW_THRESHOLD_UV 1230000 #define USB_START_HIGH_THRESHOLD_UV 2150000

+#if CONFIG_IS_ENABLED(EFI_HAVE_CAPSULE_SUPPORT) +struct efi_fw_image fw_images[1];

+struct efi_capsule_update_info update_info = {

• .images = fw_images,
  

+};

+u8 num_image_type_guids = ARRAY_SIZE(fw_images);

The definition of num_image_type_guids is always the same across boards. Why do we need it for every platform?

Yes, but that is because at present every platform is declaring the same variable name(fw_images) for the struct efi_fw_image.

That's because fw_images is hard-coded in efi_firmware.c by your change.

That is not mandatory,

In this sense, it is mandatory unless you want to write a new FMP driver from the scratch.

and a subsequent addition can declare a variable with a different name. But in case there is consensus on using a fixed variable name for the structure, I can make the change that you are suggesting subsequently. I will work on it if needed after the FWU patches upstreaming work is done.

...

I believe that we can remove it. Anyhow it's not documented in doc/develop/uefi/uefi.rst.

...

+#endif /* EFI_HAVE_CAPSULE_SUPPORT */

int board_early_init_f(void) { /* nothing to do, only used in SPL */ @@ -666,6 +677,13 @@ int board_init(void)

  setup_led(LEDST_ON);

+#if CONFIG_IS_ENABLED(EFI_HAVE_CAPSULE_SUPPORT)

• efi_guid_t image_type_guid = STM32MP_FIP_IMAGE_GUID;
  

• guidcpy(&fw_images[0].image_type_id, &image_type_guid);
  

• fw_images[0].fw_name = u"STM32MP-FIP";
  

• fw_images[0].image_index = 1;
  

Then, we should describe that image_index must be 1 to num_image_type_guids (or strictly, number of descriptors returned by GetImageInfo()) in the document above.

Okay

...

(I hope that you add a sanity checker against it as well.)

Yes, in the efi_fmp_find(), there is a check for the index value passed through the capsule against the value returned by the GetImageInfo().

I meant that the check takes place at some time in initialization, but yes, efi_fmp_find() works as a safe guard.

-Takahiro Akashi

-sughosh

...

-Takahiro Akashi

...

+#endif return 0; }

diff --git a/include/configs/stm32mp15_common.h b/include/configs/stm32mp15_common.h index c5412ffeb3..bb19dae945 100644 --- a/include/configs/stm32mp15_common.h +++ b/include/configs/stm32mp15_common.h @@ -34,6 +34,10 @@ #define CONFIG_SERVERIP 192.168.1.1 #endif

+#define STM32MP_FIP_IMAGE_GUID \

• EFI_GUID(0x19d5df83, 0x11b0, 0x457b, 0xbe, 0x2c, \
  

•          0x75, 0x59, 0xc1, 0x31, 0x42, 0xa5)
  

/*****************************************************************************/ #ifdef CONFIG_DISTRO_DEFAULTS /*****************************************************************************/ -- 2.34.1

On Wed, Sep 28, 2022 at 02:59:47PM +0530, Sughosh Ganu wrote:

Add weak functions for getting the update index value and dfu alternate number needed for FWU Multi Bank update functionality.

The current implementation for getting the update index value is for platforms with 2 banks. If a platform supports more than 2 banks, it can implement it's own function. The function to get the dfu alternate number has been added for platforms with GPT partitioned storage devices. Platforms with other storage partition scheme need to implement their own function.

Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org Reviewed-by: Patrick Delaunay patrick.delaunay@foss.st.com Acked-by: Etienne Carriere etienne.carriere@linaro.org

---

Changes since V10: None

Note: I have not changed the logic of the check in fwu_gpt_get_alt_num(). This can be extended if and when another GPT based device gets added to the DFU framework.

include/fwu.h | 29 ++++++++++ lib/fwu_updates/fwu.c | 27 +++++++++ lib/fwu_updates/fwu_gpt.c | 114 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 170 insertions(+) create mode 100644 lib/fwu_updates/fwu_gpt.c

diff --git a/include/fwu.h b/include/fwu.h index 167f91388e..99f43f07b0 100644 --- a/include/fwu.h +++ b/include/fwu.h @@ -212,4 +212,33 @@ int fwu_accept_image(efi_guid_t *img_type_id, u32 bank); */ int fwu_clear_accept_image(efi_guid_t *img_type_id, u32 bank);

+/**

• • fwu_plat_get_alt_num() - Get the DFU Alt Num for the image from the platform
• • @dev: FWU device
• • @image_guid: Image GUID for which DFU alt number needs to be retrieved
• • @alt_num: Pointer to the alt_num
• • Get the DFU alt number from the platform for the image specified by the
• • image GUID.
• • Return: 0 if OK, -ve on error
• */

+int fwu_plat_get_alt_num(struct udevice *dev, efi_guid_t *image_guid,

• 	 u8 *alt_num);
  

+/**

• • fwu_plat_get_update_index() - Get the value of the update bank
• • @update_idx: Bank number to which images are to be updated
• • Get the value of the bank(partition) to which the update needs to be
• • made.
• • Note: This is a weak function and platforms can override this with
• • their own implementation for selection of the update bank.
• • Return: 0 if OK, -ve on error
• */

+int fwu_plat_get_update_index(uint *update_idx); #endif /* _FWU_H_ */ diff --git a/lib/fwu_updates/fwu.c b/lib/fwu_updates/fwu.c index fe8058bc37..5cf6bec6a0 100644 --- a/lib/fwu_updates/fwu.c +++ b/lib/fwu_updates/fwu.c @@ -361,3 +361,30 @@ 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); }

+/**

• • fwu_plat_get_update_index() - Get the value of the update bank
• • @update_idx: Bank number to which images are to be updated
• • Get the value of the bank(partition) to which the update needs to be
• • made.
• • Note: This is a weak function and platforms can override this with
• • their own implementation for selection of the update bank.
• • Return: 0 if OK, -ve on error
• */

+__weak int fwu_plat_get_update_index(uint *update_idx) +{

• int ret;
• u32 active_idx;
• ret = fwu_get_active_index(&active_idx);
• if (ret < 0)

• return -1;
  

• *update_idx = (active_idx + 1) % CONFIG_FWU_NUM_BANKS;
• return ret;

+} diff --git a/lib/fwu_updates/fwu_gpt.c b/lib/fwu_updates/fwu_gpt.c new file mode 100644 index 0000000000..a1b0f3bea7 --- /dev/null +++ b/lib/fwu_updates/fwu_gpt.c @@ -0,0 +1,114 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/*

• • Copyright (c) 2022, Linaro Limited
• */

+#include <blk.h> +#include <dfu.h> +#include <efi.h> +#include <efi_loader.h> +#include <fwu.h> +#include <log.h> +#include <part.h>

+#include <linux/errno.h>

+static int get_gpt_dfu_identifier(struct blk_desc *desc, efi_guid_t *image_guid) +{

• int i;
• struct disk_partition info;
• efi_guid_t unique_part_guid;
• for (i = 1; i < MAX_SEARCH_PARTITIONS; i++) {

• if (part_get_info(desc, i, &info))
  

• 	continue;
  

• uuid_str_to_bin(info.uuid, unique_part_guid.b,
  

• 		UUID_STR_FORMAT_GUID);
  

• if (!guidcmp(&unique_part_guid, image_guid))
  

• 	return i;
  

• }
• log_err("No partition found with image_guid %pUs\n", image_guid);
• return -ENOENT;

+}

+static int fwu_gpt_get_alt_num(struct blk_desc *desc, efi_guid_t *image_guid,

• 	       u8 *alt_num, unsigned char dfu_dev)
  

+{

• int ret = -1;
• int i, part, dev_num;
• int nalt;
• struct dfu_entity *dfu;
• dev_num = desc->devnum;
• part = get_gpt_dfu_identifier(desc, image_guid);
• if (part < 0)

• return -ENOENT;
  

• dfu_init_env_entities(NULL, NULL);
• nalt = 0;
• list_for_each_entry(dfu, &dfu_list, list)

• nalt++;
  

Why do we have to do this? Can't we just call dfu_get_entity() on a for(;;) loop and break out if the result is NULL?

• if (!nalt) {

• log_warning("No entities in dfu_alt_info\n");
  

• dfu_free_entities();
  

• return -ENOENT;
  

• }
• for (i = 0; i < nalt; i++) {

• dfu = dfu_get_entity(i);
  

• if (!dfu)
  

• 	continue;
  

• /*
  

•  * Currently, Multi Bank update
  

•  * feature is being supported
  

•  * only on GPT partitioned
  

•  * MMC/SD devices.
  

•  */
  

• if (dfu->dev_type != dfu_dev)
  

• 	continue;
  

• if (dfu->layout == DFU_RAW_ADDR &&
  

•     dfu->data.mmc.dev_num == dev_num &&
  

•     dfu->data.mmc.part == part) {
  

• 	*alt_num = dfu->alt;
  

• 	ret = 0;
  

• 	break;
  

• }
  

I still this needs to go into a different function. Maybe in one with switch/cases where you only check DFU_DEV_MMC for now and return -ENOTSUPP for other device types

• }
• dfu_free_entities();
• return ret;

+}

+/**

• • fwu_plat_get_alt_num() - Get the DFU alt number
• • @dev: FWU metadata device
• • @image_guid: GUID value of the image for which the alt num is to

• •          be obtained
    

• • @alt_num: The DFU alt number for the image that is to be updated
• • Get the DFU alt number for the image that is to be updated. The
• • image is identified with the image_guid parameter that is passed
• • to the function.
• • Note: This is a weak function and platforms can override this with
• • their own implementation for obtaining the alt number value.
• • Return: 0 if OK, -ve on error
• */

+__weak int fwu_plat_get_alt_num(struct udevice *dev, efi_guid_t *image_guid,

• 		u8 *alt_num)
  

+{

• struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
• return fwu_gpt_get_alt_num(dev_get_uclass_plat(priv->blk_dev),

• 		   image_guid, alt_num, DFU_DEV_MMC);
  

+}

2.34.1

Cheers /Ilias

On Wed, Sep 28, 2022 at 02:59:50PM +0530, Sughosh Ganu wrote:

The FWU Multi Bank Update specification requires the Update Agent to carry out certain checks at the time of platform boot. The Update Agent is the component which is responsible for updating the firmware components and maintaining and keeping the metadata in sync.

The spec requires that the Update Agent perform the following checks at the time of boot

• Sanity check of both the metadata copies maintained by the platform.
• Get the boot index passed to U-Boot by the prior stage bootloader and use this value for metadata bookkeeping.
• Check if the system is booting in Trial State. If the system boots in the Trial State for more than a specified number of boot counts, change the Active Bank to be booting the platform from.

Call these checks through the main loop event at the time of platform boot.

Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org Reviewed-by: Etienne Carriere etienne.carriere@linaro.org

---

Changes since V10:

• Remove the spurious newline addition in efi_setup.c
• Move the assignment of trial_state outside the if() as suggested by Jassi

include/fwu.h | 13 +++ lib/fwu_updates/fwu.c | 193 +++++++++++++++++++++++++++++++++++++++++- 2 files changed, 204 insertions(+), 2 deletions(-)

diff --git a/include/fwu.h b/include/fwu.h index 71a03cf70b..2effa7da38 100644 --- a/include/fwu.h +++ b/include/fwu.h @@ -253,4 +253,17 @@ int fwu_plat_get_update_index(uint *update_idx);

*/ void fwu_plat_get_bootidx(uint *boot_idx);

+/**

• • fwu_update_checks_pass() - Check if FWU update can be done
• • Check if the FWU update can be executed. The updates are
• • allowed only when the platform is not in Trial State and
• • the boot time checks have passed
• • Return: 1 if OK, 0 on error
• */

+u8 fwu_update_checks_pass(void);

#endif /* _FWU_H_ */ diff --git a/lib/fwu_updates/fwu.c b/lib/fwu_updates/fwu.c index 5cf6bec6a0..5cde5514d2 100644 --- a/lib/fwu_updates/fwu.c +++ b/lib/fwu_updates/fwu.c @@ -4,10 +4,19 @@ */

#include <dm.h> +#include <efi.h> #include <efi_loader.h> +#include <efi_variable.h> +#include <event.h> #include <fwu.h> #include <fwu_mdata.h> -#include <log.h> +#include <malloc.h>

+#include <linux/errno.h> +#include <linux/types.h>

+static u8 trial_state; +static u8 boottime_check;

#include <linux/errno.h> #include <linux/types.h> @@ -16,8 +25,113 @@ #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) +static int trial_counter_update(u16 *trial_state_ctr) +{

• bool delete;
• u32 var_attr;
• efi_status_t status;
• efi_uintn_t var_size;
• delete = !trial_state_ctr ? true : false;
• var_size = !trial_state_ctr ? 0 : (efi_uintn_t)sizeof(*trial_state_ctr);
• var_attr = !trial_state_ctr ? 0 : EFI_VARIABLE_NON_VOLATILE |

• EFI_VARIABLE_BOOTSERVICE_ACCESS;
  

• status = efi_set_variable_int(u"TrialStateCtr",

• 		      &efi_global_variable_guid,
  

• 		      var_attr,
  

• 		      var_size, trial_state_ctr, false);
  

• if ((delete && (status != EFI_NOT_FOUND &&

Is there a legitimate case where we need to delete the variable while it's not there? Wouldn't that be some kind of failure we shouldn't hide?

• 	status != EFI_SUCCESS)) ||
  

•    (!delete && status != EFI_SUCCESS))
  

• return -1;
  

• return 0;

+}

+static int in_trial_state(struct fwu_mdata *mdata) +{

• u32 i, active_bank;
• struct fwu_image_entry *img_entry;
• struct fwu_image_bank_info *img_bank_info;
• active_bank = mdata->active_index;
• img_entry = &mdata->img_entry[0];
• for (i = 0; i < CONFIG_FWU_NUM_IMAGES_PER_BANK; i++) {

• img_bank_info = &img_entry[i].img_bank_info[active_bank];
  

• if (!img_bank_info->accepted) {
  

• 	return 1;
  

• }
  

• }
• return 0;

+}

+static int fwu_trial_state_check(struct udevice *dev) +{

• int ret;
• efi_status_t status;
• efi_uintn_t var_size;
• u16 trial_state_ctr;
• u32 var_attributes;
• struct fwu_mdata mdata = { 0 };
• ret = fwu_get_mdata(dev, &mdata);
• if (ret)

• return ret;
  

• trial_state = in_trial_state(&mdata);
• if (trial_state) {

• var_size = (efi_uintn_t)sizeof(trial_state_ctr);
  

• log_info("System booting in Trial State\n");
  

• var_attributes = EFI_VARIABLE_NON_VOLATILE |
  

• 	EFI_VARIABLE_BOOTSERVICE_ACCESS;
  

• status = efi_get_variable_int(u"TrialStateCtr",
  

• 			      &efi_global_variable_guid,
  

• 			      &var_attributes,
  

• 			      &var_size, &trial_state_ctr,
  

• 			      NULL);
  

• if (status != EFI_SUCCESS) {
  

• 	log_err("Unable to read TrialStateCtr variable\n");
  

• 	ret = -1;
  

• 	goto out;
  

• }
  

• ++trial_state_ctr;
  

• if (trial_state_ctr > CONFIG_FWU_TRIAL_STATE_CNT) {
  

• 	log_info("Trial State count exceeded. Revert back to previous_active_index\n");
  

• 	ret = fwu_revert_boot_index();
  

• 	if (ret) {
  

• 		log_err("Unable to revert active_index\n");
  

• 		goto out;
  

• 	}
  

• 	/* Delete the TrialStateCtr variable */
  

• 	ret = trial_counter_update(NULL);
  

• 	if (ret) {
  

• 		log_err("Unable to delete TrialStateCtr variable\n");
  

• 		goto out;
  

• 	}
  

• } else {
  

• 	ret = trial_counter_update(&trial_state_ctr);
  

• 	if (ret) {
  

• 		log_err("Unable to increment TrialStateCtr variable\n");
  

• 		goto out;
  

• 	}
  

• }
  

• } else {

• /* Delete the variable */
  

• ret = trial_counter_update(NULL);
  

• if (ret) {
  

• 	log_err("Unable to delete TrialStateCtr variable\n");
  

• }
  

• }

+out:

• return ret;

+}

+static int fwu_get_dev_mdata(struct udevice **dev, struct fwu_mdata *mdata) { int ret;

@@ -27,6 +141,9 @@ static int fwu_get_dev_mdata(struct udevice **dev, struct fwu_mdata *mdata) return ret; }

• if (!mdata)

• return 0;
  

Doesn't this belong to the patch that introduced the function?

ret = fwu_get_mdata(*dev, mdata); if (ret < 0) log_debug("Unable to get valid FWU metadata\n"); @@ -388,3 +505,75 @@ __weak int fwu_plat_get_update_index(uint *update_idx)

return ret; }

+/**

• • fwu_update_checks_pass() - Check if FWU update can be done
• • Check if the FWU update can be executed. The updates are
• • allowed only when the platform is not in Trial State and
• • the boot time checks have passed
• • Return: 1 if OK, 0 on error
• */

+u8 fwu_update_checks_pass(void) +{

• return !trial_state && boottime_check;

+}

+static int fwu_boottime_checks(void *ctx, struct event *event)

I think this should be declared as bool

+{

• int ret;
• struct udevice *dev;
• u32 boot_idx, active_idx;
• ret = fwu_get_dev_mdata(&dev, NULL);
• if (ret)

• return ret;
  

• ret = fwu_mdata_check(dev);
• if (ret) {

• return 0;
  

• }

You don't need {}

• /*

• * Get the Boot Index, i.e. the bank from
  

• * which the platform has booted. This value
  

• * gets passed from the ealier stage bootloader
  

• * which booted u-boot, e.g. tf-a. If the
  

• * boot index is not the same as the
  

• * active_index read from the FWU metadata,
  

• * update the active_index.
  

• */
  

• fwu_plat_get_bootidx(&boot_idx);
• if (boot_idx >= CONFIG_FWU_NUM_BANKS) {

• log_err("Received incorrect value of boot_index\n");
  

• return 0;
  

• }
• ret = fwu_get_active_index(&active_idx);
• if (ret) {

• log_err("Unable to read active_index\n");
  

• return 0;
  

• }
• if (boot_idx != active_idx) {

• log_info("Boot idx %u is not matching active idx %u, changing active_idx\n",
  

• 	 boot_idx, active_idx);
  

• ret = fwu_set_active_index(boot_idx);
  

• if (!ret)
  

• 	boottime_check = 1;
  

• return 0;
  

• }
• if (efi_init_obj_list() != EFI_SUCCESS)

• return 0;
  

• ret = fwu_trial_state_check(dev);
• if (!ret)

• boottime_check = 1;
  

• return 0;

+}

+EVENT_SPY(EVT_MAIN_LOOP, fwu_boottime_checks);

2.34.1

Cheers /Ilias

Hello Sughosh,

2 comments on the documentation. Otherwise it looks all good to me.

Best regards, Etienne

On Wed, 28 Sept 2022 at 11:31, Sughosh Ganu sughosh.ganu@linaro.org wrote:

Add documentation for the FWU Multi Bank Update feature. The document describes the steps needed for setting up the platform for the feature, as well as steps for enabling the feature on the platform.

Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org

Changes since V10:

• Fix review comments suggested by Etienne
• Add a paragraph in the capsule update section to highlight the difference in ImageIndex correlation with DFU alt num with FWU feature enabled

doc/develop/uefi/fwu_updates.rst | 173 +++++++++++++++++++++++++++++++ doc/develop/uefi/index.rst | 1 + doc/develop/uefi/uefi.rst | 10 ++ 3 files changed, 184 insertions(+) create mode 100644 doc/develop/uefi/fwu_updates.rst

diff --git a/doc/develop/uefi/fwu_updates.rst b/doc/develop/uefi/fwu_updates.rst new file mode 100644 index 0000000000..292feceb9a --- /dev/null +++ b/doc/develop/uefi/fwu_updates.rst @@ -0,0 +1,173 @@ +.. SPDX-License-Identifier: GPL-2.0+ +.. Copyright (c) 2022 Linaro Limited

+FWU Multi Bank Updates in U-Boot +================================

+The FWU Multi Bank Update feature implements the firmware update +mechanism described in the PSA Firmware Update for A-profile Arm +Architecture specification [1]. Certain aspects of the Dependable +Boot specification [2] are also implemented. The feature provides a +mechanism to have multiple banks of updatable firmware images and for +updating the firmware images on the non-booted bank. On a successful +update, the platform boots from the updated bank on subsequent +boot. The UEFI capsule-on-disk update feature is used for performing +the actual updates of the updatable firmware images.

+The bookkeeping of the updatable images is done through a structure +called metadata. Currently, the FWU metadata supports identification +of images based on image GUIDs stored on a GPT partitioned storage +media. There are plans to extend the metadata structure for non GPT +partitioned devices as well.

+Accessing the FWU metadata is done through generic API's which are +defined in a driver which complies with the U-Boot's driver model. A +new uclass UCLASS_FWU_MDATA has been added for accessing the FWU +metadata. Individual drivers can be added based on the type of storage +media, and its partitioning method. Details of the storage device +containing the FWU metadata partitions are specified through a U-Boot +specific device tree property `fwu-mdata-store`. Please refer to +U-Boot `doc <doc/device-tree-bindings/firmware/fwu-mdata-gpt.yaml>`__ +for the device tree bindings.

+Enabling the FWU Multi Bank Update feature +------------------------------------------

+The feature can be enabled by specifying the following configs::

• CONFIG_EFI_CAPSULE_ON_DISK=y
• CONFIG_EFI_CAPSULE_FIRMWARE_MANAGEMENT=y
• CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
• CONFIG_FWU_MULTI_BANK_UPDATE=y
• CONFIG_CMD_FWU_METADATA=y

Actually the command is optional. Even without cmd support, capsules can be consumed from CAPSULE_ON_DISK_EARLY. Is it worth mentioning here?

• CONFIG_FWU_MDATA=y
• CONFIG_FWU_MDATA_GPT_BLK=y
• CONFIG_FWU_NUM_BANKS=<val>
• CONFIG_FWU_NUM_IMAGES_PER_BANK=<val>

+in the .config file

+The first group of configuration settings enable the UEFI +capsule-on-disk update functionality. The second group of configs +enable the FWU Multi Bank Update functionality. Please refer to the +section :ref:`uefi_capsule_update_ref` for more details on generation +of the UEFI capsule.

+Setting up the device for GPT partitioned storage +-------------------------------------------------

+Before enabling the functionality in U-Boot, a GPT partitioned storage +device is required. Assuming a GPT partitioned storage device, the +storage media needs to be partitioned with the correct number of +partitions, given the number of banks and number of images per bank +that the platform is going to support. Each updatable firmware image +will be stored on a separate partition. In addition, the two copies +of the FWU metadata will be stored on two separate partitions.

+As an example, a platform supporting two banks with each bank +containing three images would need to have 2 * 3 = 6 partitions plus +the two metadata partitions, or 8 partitions. In addition the storage +media can have additional partitions of non-updatable images, like the +EFI System Partition(ESP), a partition for the root file system +etc. An example list of images on the storage medium would be

+* FWU metadata 1 +* U-Boot 1 +* OP-TEE 1 +* FWU metadata 2 +* OP-TEE 2 +* U-Boot 2 +* ESP +* rootfs

Here or below, maybe mention FWU metadata1/2 and ESP (at least) expect dedicated partition type guid and reference the 2 U-Boot macros FWU_MDATA_GUID and PARTITION_SYSTEM_GUID.

+When generating the partitions, a few aspects need to be taken care +of. Each GPT partition entry in the GPT header has two GUIDs::

+* PartitionTypeGUID +* UniquePartitionGUID

+The PartitionTypeGUID value should correspond to the +``image_type_uuid`` field of the FWU metadata. This field is used to +identify a given type of updatable firmware image, e.g. U-Boot, +OP-TEE, FIP etc. This GUID should also be used for specifying the +`--guid` parameter when generating the capsule.

+The UniquePartitionGUID value should correspond to the ``image_uuid`` +field in the FWU metadata. This GUID is used to identify images of a +given image type in different banks.

+Similarly, the FWU specification defines the GUID value to be used +for the metadata partitions. This would be the PartitionTypeGUID for +the metadata partitions.

+When generating the metadata, the ``image_type_uuid`` and the +``image_uuid`` values should match the *PartitionTypeGUID* and the +*UniquePartitionGUID* values respectively.

+Performing the Update +---------------------

+Once the storage media has been partitioned and populated with the +metadata partitions, the UEFI capsule-on-disk update functionality can +be used for performing the update. Refer to the section +:ref:`uefi_capsule_update_ref` for details on how the update can be +invoked.

+On a successful update, the FWU metadata gets updated to reflect the +bank from which the platform would be booting on subsequent boot.

+Based on the value of bit15 of the Flags member of the capsule header, +the updated images would either be accepted by the U-Boot's UEFI +implementation, or by the Operating System. If the Operating System is +accepting the firmware images, it does so by generating an empty +*accept* capsule. The Operating System can also reject the updated +firmware by generating a *revert* capsule. The empty capsule can be +applied by using the exact same procedure used for performing the +capsule-on-disk update.

+The task of accepting the different firmware images, post an update +may be done by multiple, separate components in the Operating +System. To help identify the firmware image that is being accepted, +the accept capsule passes the image GUID of the firmware image being +accepted. The relevant code in U-Boot then sets the Accept bit of the +corresponding firmware image for which the accept capsule was +found. Only when all the firmware components in a bank have been +accepted does the platform transition from trial state to regular +state.

+The revert capsule on the other hand does not pass any image GUID, +since reverting any image of the bank has the same result of the +platform booting from the other bank on subsequent boot.

+Generating an empty capsule +---------------------------

+The empty capsule can be generated using the mkeficapsule utility. To +build the tool, enable::

• CONFIG_TOOLS_MKEFICAPSULE=y

+Run the following commands to generate the accept/revert capsules::

+.. code-block:: bash

• $ ./tools/mkeficapsule \

•  [--fw-accept --guid <image guid>] | \
  

•  [--fw-revert] \
  

•  <capsule_file_name>
  

+Some examples of using the mkeficapsule tool for generation of the +empty capsule would be::

+.. code-block:: bash

• $ ./tools/mkeficapsule --fw-accept --guid <image guid> \

• <accept_capsule_name>

• $ ./tools/mkeficapsule --fw-revert <revert_capsule_name>

+Links +-----

+* [1] https://developer.arm.com/documentation/den0118/a/ - FWU Specification +* [2] https://git.codelinaro.org/linaro/dependable-boot/mbfw/uploads/6f7ddfe3be24e... - Dependable Boot Specification diff --git a/doc/develop/uefi/index.rst b/doc/develop/uefi/index.rst index 7e65dbc5d5..e26b1fbe05 100644 --- a/doc/develop/uefi/index.rst +++ b/doc/develop/uefi/index.rst @@ -13,3 +13,4 @@ can be run an UEFI payload. uefi.rst u-boot_on_efi.rst iscsi.rst

• fwu_updates.rst

diff --git a/doc/develop/uefi/uefi.rst b/doc/develop/uefi/uefi.rst index 941e427093..b5c83db65a 100644 --- a/doc/develop/uefi/uefi.rst +++ b/doc/develop/uefi/uefi.rst @@ -277,6 +277,8 @@ Enable ``CONFIG_OPTEE``, ``CONFIG_CMD_OPTEE_RPMB`` and ``CONFIG_EFI_MM_COMM_TEE`

[1] https://optee.readthedocs.io/en/latest/building/efi_vars/stmm.html

+.. _uefi_capsule_update_ref:

Enabling UEFI Capsule Update feature

@@ -377,6 +379,14 @@ following command::

    dfu list

+When the FWU Multi Bank Update feature is enabled on the platform, the
+image index is used only to identify the image index with the image
+GUID. The image index would not correspond to the dfu alt number. This
+is because the FWU feature supports multiple partitions(banks) of
+updatable images, and the actual dfu alt number to which the image is
+to be written to is determined at runtime, based on the value of the
+update bank to which the image is to be written.
+
When using the FMP for FIT images, the image index value needs to be
set to 1.

--
2.34.1

Sughosh,

On Wed, Sep 28, 2022 at 02:59:56PM +0530, Sughosh Ganu wrote:

Add documentation for the FWU Multi Bank Update feature. The document describes the steps needed for setting up the platform for the feature, as well as steps for enabling the feature on the platform.

Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org

Changes since V10:

• Fix review comments suggested by Etienne
• Add a paragraph in the capsule update section to highlight the difference in ImageIndex correlation with DFU alt num with FWU feature enabled

doc/develop/uefi/fwu_updates.rst | 173 +++++++++++++++++++++++++++++++ doc/develop/uefi/index.rst | 1 + doc/develop/uefi/uefi.rst | 10 ++ 3 files changed, 184 insertions(+) create mode 100644 doc/develop/uefi/fwu_updates.rst

diff --git a/doc/develop/uefi/fwu_updates.rst b/doc/develop/uefi/fwu_updates.rst new file mode 100644 index 0000000000..292feceb9a --- /dev/null +++ b/doc/develop/uefi/fwu_updates.rst @@ -0,0 +1,173 @@ +.. SPDX-License-Identifier: GPL-2.0+ +.. Copyright (c) 2022 Linaro Limited

+FWU Multi Bank Updates in U-Boot +================================

+The FWU Multi Bank Update feature implements the firmware update +mechanism described in the PSA Firmware Update for A-profile Arm +Architecture specification [1]. Certain aspects of the Dependable +Boot specification [2] are also implemented. The feature provides a +mechanism to have multiple banks of updatable firmware images and for +updating the firmware images on the non-booted bank. On a successful +update, the platform boots from the updated bank on subsequent +boot. The UEFI capsule-on-disk update feature is used for performing +the actual updates of the updatable firmware images.

+The bookkeeping of the updatable images is done through a structure +called metadata. Currently, the FWU metadata supports identification +of images based on image GUIDs stored on a GPT partitioned storage +media. There are plans to extend the metadata structure for non GPT +partitioned devices as well.

+Accessing the FWU metadata is done through generic API's which are +defined in a driver which complies with the U-Boot's driver model. A +new uclass UCLASS_FWU_MDATA has been added for accessing the FWU +metadata. Individual drivers can be added based on the type of storage +media, and its partitioning method. Details of the storage device +containing the FWU metadata partitions are specified through a U-Boot +specific device tree property `fwu-mdata-store`. Please refer to +U-Boot `doc <doc/device-tree-bindings/firmware/fwu-mdata-gpt.yaml>`__ +for the device tree bindings.

+Enabling the FWU Multi Bank Update feature +------------------------------------------

+The feature can be enabled by specifying the following configs::

• CONFIG_EFI_CAPSULE_ON_DISK=y
• CONFIG_EFI_CAPSULE_FIRMWARE_MANAGEMENT=y
• CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
• CONFIG_FWU_MULTI_BANK_UPDATE=y
• CONFIG_CMD_FWU_METADATA=y
• CONFIG_FWU_MDATA=y
• CONFIG_FWU_MDATA_GPT_BLK=y
• CONFIG_FWU_NUM_BANKS=<val>
• CONFIG_FWU_NUM_IMAGES_PER_BANK=<val>

+in the .config file

+The first group of configuration settings enable the UEFI +capsule-on-disk update functionality. The second group of configs +enable the FWU Multi Bank Update functionality. Please refer to the +section :ref:`uefi_capsule_update_ref` for more details on generation +of the UEFI capsule.

+Setting up the device for GPT partitioned storage +-------------------------------------------------

+Before enabling the functionality in U-Boot, a GPT partitioned storage +device is required. Assuming a GPT partitioned storage device, the +storage media needs to be partitioned with the correct number of +partitions, given the number of banks and number of images per bank +that the platform is going to support. Each updatable firmware image +will be stored on a separate partition. In addition, the two copies +of the FWU metadata will be stored on two separate partitions.

+As an example, a platform supporting two banks with each bank +containing three images would need to have 2 * 3 = 6 partitions plus +the two metadata partitions, or 8 partitions. In addition the storage +media can have additional partitions of non-updatable images, like the +EFI System Partition(ESP), a partition for the root file system +etc. An example list of images on the storage medium would be

+* FWU metadata 1 +* U-Boot 1 +* OP-TEE 1 +* FWU metadata 2 +* OP-TEE 2 +* U-Boot 2 +* ESP +* rootfs

+When generating the partitions, a few aspects need to be taken care +of. Each GPT partition entry in the GPT header has two GUIDs::

+* PartitionTypeGUID +* UniquePartitionGUID

+The PartitionTypeGUID value should correspond to the +``image_type_uuid`` field of the FWU metadata. This field is used to +identify a given type of updatable firmware image, e.g. U-Boot, +OP-TEE, FIP etc. This GUID should also be used for specifying the +`--guid` parameter when generating the capsule.

+The UniquePartitionGUID value should correspond to the ``image_uuid`` +field in the FWU metadata. This GUID is used to identify images of a +given image type in different banks.

Well, what is not clear to me here is: - who is responsible to set up FWU metadata and when - how FWU metadata is related to fw_images and update_info which are used in normal case, which is mentioned in develop/uefi/uefi.rst

I know the whole text here is dedicated to A/B update, but I think it would also be helpful to have a single document which covers both cases for developers to understand how differently FWU is configured for those cases.

-Takahiro Akashi

+Similarly, the FWU specification defines the GUID value to be used +for the metadata partitions. This would be the PartitionTypeGUID for +the metadata partitions.

+When generating the metadata, the ``image_type_uuid`` and the +``image_uuid`` values should match the *PartitionTypeGUID* and the +*UniquePartitionGUID* values respectively.

+Performing the Update +---------------------

+Once the storage media has been partitioned and populated with the +metadata partitions, the UEFI capsule-on-disk update functionality can +be used for performing the update. Refer to the section +:ref:`uefi_capsule_update_ref` for details on how the update can be +invoked.

+On a successful update, the FWU metadata gets updated to reflect the +bank from which the platform would be booting on subsequent boot.

+Based on the value of bit15 of the Flags member of the capsule header, +the updated images would either be accepted by the U-Boot's UEFI +implementation, or by the Operating System. If the Operating System is +accepting the firmware images, it does so by generating an empty +*accept* capsule. The Operating System can also reject the updated +firmware by generating a *revert* capsule. The empty capsule can be +applied by using the exact same procedure used for performing the +capsule-on-disk update.

+The task of accepting the different firmware images, post an update +may be done by multiple, separate components in the Operating +System. To help identify the firmware image that is being accepted, +the accept capsule passes the image GUID of the firmware image being +accepted. The relevant code in U-Boot then sets the Accept bit of the +corresponding firmware image for which the accept capsule was +found. Only when all the firmware components in a bank have been +accepted does the platform transition from trial state to regular +state.

+The revert capsule on the other hand does not pass any image GUID, +since reverting any image of the bank has the same result of the +platform booting from the other bank on subsequent boot.

+Generating an empty capsule +---------------------------

+The empty capsule can be generated using the mkeficapsule utility. To +build the tool, enable::

• CONFIG_TOOLS_MKEFICAPSULE=y

+Run the following commands to generate the accept/revert capsules::

+.. code-block:: bash

• $ ./tools/mkeficapsule \

•  [--fw-accept --guid <image guid>] | \
  

•  [--fw-revert] \
  

•  <capsule_file_name>
  

+Some examples of using the mkeficapsule tool for generation of the +empty capsule would be::

+.. code-block:: bash

• $ ./tools/mkeficapsule --fw-accept --guid <image guid> \

• <accept_capsule_name>

• $ ./tools/mkeficapsule --fw-revert <revert_capsule_name>

+Links +-----

+* [1] https://developer.arm.com/documentation/den0118/a/ - FWU Specification +* [2] https://git.codelinaro.org/linaro/dependable-boot/mbfw/uploads/6f7ddfe3be24e... - Dependable Boot Specification diff --git a/doc/develop/uefi/index.rst b/doc/develop/uefi/index.rst index 7e65dbc5d5..e26b1fbe05 100644 --- a/doc/develop/uefi/index.rst +++ b/doc/develop/uefi/index.rst @@ -13,3 +13,4 @@ can be run an UEFI payload. uefi.rst u-boot_on_efi.rst iscsi.rst

• fwu_updates.rst

diff --git a/doc/develop/uefi/uefi.rst b/doc/develop/uefi/uefi.rst index 941e427093..b5c83db65a 100644 --- a/doc/develop/uefi/uefi.rst +++ b/doc/develop/uefi/uefi.rst @@ -277,6 +277,8 @@ Enable ``CONFIG_OPTEE``, ``CONFIG_CMD_OPTEE_RPMB`` and ``CONFIG_EFI_MM_COMM_TEE`

[1] https://optee.readthedocs.io/en/latest/building/efi_vars/stmm.html

+.. _uefi_capsule_update_ref:

Enabling UEFI Capsule Update feature

@@ -377,6 +379,14 @@ following command::

    dfu list

+When the FWU Multi Bank Update feature is enabled on the platform, the
+image index is used only to identify the image index with the image
+GUID. The image index would not correspond to the dfu alt number. This
+is because the FWU feature supports multiple partitions(banks) of
+updatable images, and the actual dfu alt number to which the image is
+to be written to is determined at runtime, based on the value of the
+update bank to which the image is to be written.
+
When using the FMP for FIT images, the image index value needs to be
set to 1.

-- 
2.34.1

On Tue, Oct 04, 2022 at 12:10:27PM +0530, Sughosh Ganu wrote:

hi Takahiro,

On Tue, 4 Oct 2022 at 08:24, Takahiro Akashi takahiro.akashi@linaro.org wrote:

...

Sughosh,

On Wed, Sep 28, 2022 at 02:59:56PM +0530, Sughosh Ganu wrote:

...

Add documentation for the FWU Multi Bank Update feature. The document describes the steps needed for setting up the platform for the feature, as well as steps for enabling the feature on the platform.

Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org

Changes since V10:

• Fix review comments suggested by Etienne
• Add a paragraph in the capsule update section to highlight the difference in ImageIndex correlation with DFU alt num with FWU feature enabled

doc/develop/uefi/fwu_updates.rst | 173 +++++++++++++++++++++++++++++++ doc/develop/uefi/index.rst | 1 + doc/develop/uefi/uefi.rst | 10 ++ 3 files changed, 184 insertions(+) create mode 100644 doc/develop/uefi/fwu_updates.rst

diff --git a/doc/develop/uefi/fwu_updates.rst b/doc/develop/uefi/fwu_updates.rst new file mode 100644 index 0000000000..292feceb9a --- /dev/null +++ b/doc/develop/uefi/fwu_updates.rst @@ -0,0 +1,173 @@ +.. SPDX-License-Identifier: GPL-2.0+ +.. Copyright (c) 2022 Linaro Limited

+FWU Multi Bank Updates in U-Boot +================================

+The FWU Multi Bank Update feature implements the firmware update +mechanism described in the PSA Firmware Update for A-profile Arm +Architecture specification [1]. Certain aspects of the Dependable +Boot specification [2] are also implemented. The feature provides a +mechanism to have multiple banks of updatable firmware images and for +updating the firmware images on the non-booted bank. On a successful +update, the platform boots from the updated bank on subsequent +boot. The UEFI capsule-on-disk update feature is used for performing +the actual updates of the updatable firmware images.

+The bookkeeping of the updatable images is done through a structure +called metadata. Currently, the FWU metadata supports identification +of images based on image GUIDs stored on a GPT partitioned storage +media. There are plans to extend the metadata structure for non GPT +partitioned devices as well.

+Accessing the FWU metadata is done through generic API's which are +defined in a driver which complies with the U-Boot's driver model. A +new uclass UCLASS_FWU_MDATA has been added for accessing the FWU +metadata. Individual drivers can be added based on the type of storage +media, and its partitioning method. Details of the storage device +containing the FWU metadata partitions are specified through a U-Boot +specific device tree property `fwu-mdata-store`. Please refer to +U-Boot `doc <doc/device-tree-bindings/firmware/fwu-mdata-gpt.yaml>`__ +for the device tree bindings.

+Enabling the FWU Multi Bank Update feature +------------------------------------------

+The feature can be enabled by specifying the following configs::

• CONFIG_EFI_CAPSULE_ON_DISK=y
• CONFIG_EFI_CAPSULE_FIRMWARE_MANAGEMENT=y
• CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
• CONFIG_FWU_MULTI_BANK_UPDATE=y
• CONFIG_CMD_FWU_METADATA=y
• CONFIG_FWU_MDATA=y
• CONFIG_FWU_MDATA_GPT_BLK=y
• CONFIG_FWU_NUM_BANKS=<val>
• CONFIG_FWU_NUM_IMAGES_PER_BANK=<val>

+in the .config file

+The first group of configuration settings enable the UEFI +capsule-on-disk update functionality. The second group of configs +enable the FWU Multi Bank Update functionality. Please refer to the +section :ref:`uefi_capsule_update_ref` for more details on generation +of the UEFI capsule.

+Setting up the device for GPT partitioned storage +-------------------------------------------------

+Before enabling the functionality in U-Boot, a GPT partitioned storage +device is required. Assuming a GPT partitioned storage device, the +storage media needs to be partitioned with the correct number of +partitions, given the number of banks and number of images per bank +that the platform is going to support. Each updatable firmware image +will be stored on a separate partition. In addition, the two copies +of the FWU metadata will be stored on two separate partitions.

+As an example, a platform supporting two banks with each bank +containing three images would need to have 2 * 3 = 6 partitions plus +the two metadata partitions, or 8 partitions. In addition the storage +media can have additional partitions of non-updatable images, like the +EFI System Partition(ESP), a partition for the root file system +etc. An example list of images on the storage medium would be

+* FWU metadata 1 +* U-Boot 1 +* OP-TEE 1 +* FWU metadata 2 +* OP-TEE 2 +* U-Boot 2 +* ESP +* rootfs

+When generating the partitions, a few aspects need to be taken care +of. Each GPT partition entry in the GPT header has two GUIDs::

+* PartitionTypeGUID +* UniquePartitionGUID

+The PartitionTypeGUID value should correspond to the +``image_type_uuid`` field of the FWU metadata. This field is used to +identify a given type of updatable firmware image, e.g. U-Boot, +OP-TEE, FIP etc. This GUID should also be used for specifying the +`--guid` parameter when generating the capsule.

+The UniquePartitionGUID value should correspond to the ``image_uuid`` +field in the FWU metadata. This GUID is used to identify images of a +given image type in different banks.

Well, what is not clear to me here is:

• who is responsible to set up FWU metadata and when

Initially, when the board is being provisioned, the metadata is supposed to be installed on two separate partitions. This is described under "Setting up the device for GPT partitioned storage".

...

• how FWU metadata is related to fw_images and update_info which are used in normal case, which is mentioned in develop/uefi/uefi.rst

The entire update process is the same as the UEFI capsule update. This is being highlighted in the first paragraph under "Performing the Update".

I think of developers.

For instance, as I mentioned above, image_type_uuid/image_index in FWU metadata must match with the ones in fw_images. And how about update_info, especially dfu_string? This field won't be used in A/B update IIUC. There are a couple of points that developers should be aware. That is why I suggested the whole explanation is in one place.

I have also added a paragraph under "Performing the update" which specifies how the image index value used with the FWU feature enabled is different from the normal capsule update.

...

I know the whole text here is dedicated to A/B update, but I think it would also be helpful to have a single document which covers both cases for developers to understand how differently FWU is configured for those cases.

I would think that it would be more clear to have a separate document for the FWU updates. In any case, the fwu_updates.rst is under the same directory as the uefi.rst which explains the normal capsule update functionality. There would also be addition of the section for the FWU updates on platforms using MTD based devices. So I think it would be better to keep the documents separate. Even if we move the content in fwu_updates.rst under uefi.rst, the actual update process would still be explained by the section under 'uefi_capsule_update_ref'. Do you see difficulty in understanding the FWU update feature being kept in a separate file?

Yes.

I don't care whether the text is contained in a physically-single document, but do expect that the normal case and A/B update are described (in different sections) under the logically-same chapter for better understanding of differences.

-Takahiro Akashi

-sughosh

...

-Takahiro Akashi

...

+Similarly, the FWU specification defines the GUID value to be used +for the metadata partitions. This would be the PartitionTypeGUID for +the metadata partitions.

+When generating the metadata, the ``image_type_uuid`` and the +``image_uuid`` values should match the *PartitionTypeGUID* and the +*UniquePartitionGUID* values respectively.

+Performing the Update +---------------------

+Once the storage media has been partitioned and populated with the +metadata partitions, the UEFI capsule-on-disk update functionality can +be used for performing the update. Refer to the section +:ref:`uefi_capsule_update_ref` for details on how the update can be +invoked.

+On a successful update, the FWU metadata gets updated to reflect the +bank from which the platform would be booting on subsequent boot.

+Based on the value of bit15 of the Flags member of the capsule header, +the updated images would either be accepted by the U-Boot's UEFI +implementation, or by the Operating System. If the Operating System is +accepting the firmware images, it does so by generating an empty +*accept* capsule. The Operating System can also reject the updated +firmware by generating a *revert* capsule. The empty capsule can be +applied by using the exact same procedure used for performing the +capsule-on-disk update.

+The task of accepting the different firmware images, post an update +may be done by multiple, separate components in the Operating +System. To help identify the firmware image that is being accepted, +the accept capsule passes the image GUID of the firmware image being +accepted. The relevant code in U-Boot then sets the Accept bit of the +corresponding firmware image for which the accept capsule was +found. Only when all the firmware components in a bank have been +accepted does the platform transition from trial state to regular +state.

+The revert capsule on the other hand does not pass any image GUID, +since reverting any image of the bank has the same result of the +platform booting from the other bank on subsequent boot.

+Generating an empty capsule +---------------------------

+The empty capsule can be generated using the mkeficapsule utility. To +build the tool, enable::

• CONFIG_TOOLS_MKEFICAPSULE=y

+Run the following commands to generate the accept/revert capsules::

+.. code-block:: bash

• $ ./tools/mkeficapsule \

•  [--fw-accept --guid <image guid>] | \
  

•  [--fw-revert] \
  

•  <capsule_file_name>
  

+Some examples of using the mkeficapsule tool for generation of the +empty capsule would be::

+.. code-block:: bash

• $ ./tools/mkeficapsule --fw-accept --guid <image guid> \

• <accept_capsule_name>

• $ ./tools/mkeficapsule --fw-revert <revert_capsule_name>

+Links +-----

+* [1] https://developer.arm.com/documentation/den0118/a/ - FWU Specification +* [2] https://git.codelinaro.org/linaro/dependable-boot/mbfw/uploads/6f7ddfe3be24e... - Dependable Boot Specification diff --git a/doc/develop/uefi/index.rst b/doc/develop/uefi/index.rst index 7e65dbc5d5..e26b1fbe05 100644 --- a/doc/develop/uefi/index.rst +++ b/doc/develop/uefi/index.rst @@ -13,3 +13,4 @@ can be run an UEFI payload. uefi.rst u-boot_on_efi.rst iscsi.rst

• fwu_updates.rst

diff --git a/doc/develop/uefi/uefi.rst b/doc/develop/uefi/uefi.rst index 941e427093..b5c83db65a 100644 --- a/doc/develop/uefi/uefi.rst +++ b/doc/develop/uefi/uefi.rst @@ -277,6 +277,8 @@ Enable ``CONFIG_OPTEE``, ``CONFIG_CMD_OPTEE_RPMB`` and ``CONFIG_EFI_MM_COMM_TEE`

[1] https://optee.readthedocs.io/en/latest/building/efi_vars/stmm.html

+.. _uefi_capsule_update_ref:

Enabling UEFI Capsule Update feature

@@ -377,6 +379,14 @@ following command::

    dfu list

+When the FWU Multi Bank Update feature is enabled on the platform, the
+image index is used only to identify the image index with the image
+GUID. The image index would not correspond to the dfu alt number. This
+is because the FWU feature supports multiple partitions(banks) of
+updatable images, and the actual dfu alt number to which the image is
+to be written to is determined at runtime, based on the value of the
+update bank to which the image is to be written.
+
When using the FMP for FIT images, the image index value needs to be
set to 1.

--
2.34.1