Hello Sughosh,

On Thu, 9 Jun 2022 at 14: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

drivers/Kconfig | 2 + drivers/Makefile | 1 + drivers/fwu-mdata/Kconfig | 7 + drivers/fwu-mdata/Makefile | 6 + drivers/fwu-mdata/fwu-mdata-uclass.c | 459 +++++++++++++++++++++++++++ include/dm/uclass-id.h | 1 + include/fwu.h | 49 +++ include/fwu_mdata.h | 67 ++++ 8 files changed, 592 insertions(+) create mode 100644 drivers/fwu-mdata/Kconfig create mode 100644 drivers/fwu-mdata/Makefile create mode 100644 drivers/fwu-mdata/fwu-mdata-uclass.c create mode 100644 include/fwu.h create mode 100644 include/fwu_mdata.h

diff --git a/drivers/Kconfig b/drivers/Kconfig index b26ca8cf70..adc6079ecf 100644 --- a/drivers/Kconfig +++ b/drivers/Kconfig @@ -42,6 +42,8 @@ source "drivers/firmware/Kconfig"

source "drivers/fpga/Kconfig"

+source "drivers/fwu-mdata/Kconfig"

source "drivers/gpio/Kconfig"

source "drivers/hwspinlock/Kconfig" diff --git a/drivers/Makefile b/drivers/Makefile index 67c8af7442..901150bb35 100644 --- a/drivers/Makefile +++ b/drivers/Makefile @@ -83,6 +83,7 @@ obj-y += cache/ obj-$(CONFIG_CPU) += cpu/ obj-y += crypto/ obj-$(CONFIG_FASTBOOT) += fastboot/ +obj-$(CONFIG_DM_FWU_MDATA) += fwu-mdata/ obj-y += misc/ obj-$(CONFIG_MMC) += mmc/ obj-$(CONFIG_NVME) += nvme/ diff --git a/drivers/fwu-mdata/Kconfig b/drivers/fwu-mdata/Kconfig new file mode 100644 index 0000000000..d6a21c8e19 --- /dev/null +++ b/drivers/fwu-mdata/Kconfig @@ -0,0 +1,7 @@ +config DM_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.
  

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

+obj-$(CONFIG_DM_FWU_MDATA) += fwu-mdata-uclass.o diff --git a/drivers/fwu-mdata/fwu-mdata-uclass.c b/drivers/fwu-mdata/fwu-mdata-uclass.c new file mode 100644 index 0000000000..1530ceb01d --- /dev/null +++ b/drivers/fwu-mdata/fwu-mdata-uclass.c @@ -0,0 +1,459 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

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

+#include <common.h> +#include <dm.h> +#include <efi_loader.h> +#include <fwu.h> +#include <fwu_mdata.h> +#include <log.h> +#include <malloc.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_ops(struct udevice **dev,

•                      const struct fwu_mdata_ops **ops)
  

+{

•   int ret;
  

•   ret = uclass_get_device(UCLASS_FWU_MDATA, 0, dev);
  

•   if (ret) {
  

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

•           return ret;
  

•   }
  

•   if ((*ops = device_get_ops(*dev)) == NULL) {
  

•           log_debug("Cannot get fwu device ops\n");
  

•           return -ENOSYS;
  

•   }
  

•   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_err("crc32 check failed for %s FWU metadata partition\n",
  

•                   pri_part ? "primary" : "secondary");
  

•           return -1;
  

•   }
  

•   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 fwu_mdata *mdata = NULL;
  

•   ret = fwu_get_mdata(&mdata);
  

•   if (ret < 0) {
  

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

•           goto out;
  

•   }
  

•   /*
  

•    * Found the FWU metadata partition, now read the active_index
  

•    * value
  

•    */
  

•   *active_idx = mdata->active_index;
  

•   if (*active_idx > CONFIG_FWU_NUM_BANKS - 1) {
  

•           log_err("Active index value read is incorrect\n");
  

•           ret = -EINVAL;
  

•   }
  

+out:

•   free(mdata);
  

•   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(u32 active_idx) +{

•   int ret;
  

•   struct fwu_mdata *mdata = NULL;
  

•   if (active_idx > CONFIG_FWU_NUM_BANKS - 1) {
  

•           log_err("Active index value to be updated is incorrect\n");
  

•           return -1;
  

•   }
  

•   ret = fwu_get_mdata(&mdata);
  

•   if (ret < 0) {
  

•           log_err("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(mdata);
  

•   if (ret < 0) {> +               log_err("Failed to update FWU metadata partitions\n");
  

•           ret = -EIO;
  

•   }
  

+out:

•   free(mdata);
  

•   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 = NULL;
  

•   struct fwu_mdata *mdata = NULL;
  

•   struct fwu_image_entry *img_entry;
  

•   const struct fwu_mdata_ops *ops = NULL;
  

•   struct fwu_image_bank_info *img_bank_info;
  

•   ret = fwu_get_dev_ops(&dev, &ops);
  

•   if (ret)
  

•           return ret;
  

•   ret = fwu_get_mdata(&mdata);
  

•   if (ret) {
  

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

•           goto out;
  

•   }
  

•   /*
  

•    * 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_get_priv(dev),
  

•                                              image_guid, alt_num);
  

•                   break;
  

•           }
  

•   }
  

•   if (i == CONFIG_FWU_NUM_IMAGES_PER_BANK) {
  

•           log_err("Partition with the image type %pUs not found\n",
  

•                   image_type_id);
  

•           ret = -EINVAL;
  

•           goto out;
  

•   }
  

•   if (!ret) {
  

•           log_debug("alt_num %d for partition %pUs\n",
  

•                     *alt_num, &image_guid);
  

s/&image_guid/image_guid/ Ditto in trace below.

•   } else {
  

•           log_err("alt_num not found for partition with GUID %pUs\n",
  

•                   &image_guid);
  

•           ret = -EINVAL;
  

•   }
  

+out:

•   free(mdata);
  

•   return ret;
  

+}

+/**

• • fwu_mdata_check() - Check if the FWU metadata is valid
• • Validate both copies of the FWU metadata. If one of the copies
• • has gone bad, restore it from the other bad copy.
• • Return: 0 if OK, -ve on error
• */

+int fwu_mdata_check(void) +{

•   int ret;
  

•   struct udevice *dev = NULL;
  

•   const struct fwu_mdata_ops *ops = NULL;
  

•   ret = fwu_get_dev_ops(&dev, &ops);
  

•   if (ret)
  

•           return ret;
  

•   if (!ops->mdata_check) {
  

•           log_err("mdata_check() method not defined\n");
  

•           return -ENOSYS;
  

•   }
  

•   return ops->mdata_check(dev);
  

+}

+/**

• • fwu_revert_boot_index() - Revert the active index in the FWU metadata
• • Revert the active_index value in the FWU metadata, by swapping the values
• • of active_index and previous_active_index in both copies of the
• • FWU metadata.
• • Return: 0 if OK, -ve on error
• */

+int fwu_revert_boot_index(void) +{

•   int ret;
  

•   u32 cur_active_index;
  

•   struct fwu_mdata *mdata = NULL;
  

•   ret = fwu_get_mdata(&mdata);
  

•   if (ret < 0) {
  

•           log_err("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(mdata);
  

•   if (ret < 0) {
  

•           log_err("Failed to update FWU metadata partitions\n");
  

•           ret = -EIO;
  

•   }
  

+out:

•   free(mdata);
  

•   return ret;
  

+}

+/**

• • fwu_set_clear_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_set_clear_image_accept(efi_guid_t *img_type_id,

•                                 u32 bank, u8 action)
  

+{

•   int ret, i;
  

•   u32 nimages;
  

•   struct fwu_mdata *mdata = NULL;
  

•   struct fwu_image_entry *img_entry;
  

•   struct fwu_image_bank_info *img_bank_info;
  

•   ret = fwu_get_mdata(&mdata);
  

•   if (ret < 0) {
  

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

•           goto out;
  

•   }
  

•   nimages = CONFIG_FWU_NUM_IMAGES_PER_BANK;
  

•   img_entry = &mdata->img_entry[0];
  

•   for (i = 0; i < nimages; 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(mdata);
  

•                   goto out;
  

•           }
  

•   }
  

•   /* Image not found */
  

•   ret = -EINVAL;
  

+out:

•   free(mdata);
  

•   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_set_clear_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_set_clear_image_accept(img_type_id, bank,
  

•                                     IMAGE_ACCEPT_CLEAR);
  

+}

+/**

• • fwu_get_mdata() - Get a FWU metadata copy
• • @mdata: Copy of the FWU metadata
• • Get a valid copy of the FWU metadata.
• • Return: 0 if OK, -ve on error
• */

+int fwu_get_mdata(struct fwu_mdata **mdata)

Is there a real need for this function to allocate an instance of struct mdata. I think it would be clearer if it was the caller's responsibility to allocate/free the structure.

Or maybe rename this function fwu_alloc_and_copy_mdata() to highlight that the function gives an allocated copy of the data. One should be careful when calling these API functions as some act on a local copy (retrieved from fw_get_mdata()) while other functions modify straight fwu-mdata in the storage media.

Br, etienne

+{

•   int ret;
  

•   struct udevice *dev = NULL;
  

•   const struct fwu_mdata_ops *ops = NULL;
  

•   ret = fwu_get_dev_ops(&dev, &ops);
  

•   if (ret)
  

•           return ret;
  

•   if (!ops->get_mdata) {
  

•           log_err("get_mdata() method not defined\n");
  

•           return -ENOSYS;
  

•   }
  

•   return ops->get_mdata(dev, mdata);
  

+}

+/**

• • fwu_update_mdata() - Update the FWU metadata
• • @mdata: Copy of the FWU metadata
• • Update the FWU metadata structure by writing to the
• • FWU metadata partitions.
• • Return: 0 if OK, -ve on error
• */

+int fwu_update_mdata(struct fwu_mdata *mdata) +{

•   int ret;
  

•   void *buf;
  

•   struct udevice *dev = NULL;
  

•   const struct fwu_mdata_ops *ops = NULL;
  

•   ret = fwu_get_dev_ops(&dev, &ops);
  

•   if (ret)
  

•           return ret;
  

•   if (!ops->update_mdata) {
  

•           log_err("get_mdata() method not defined\n");
  

•           return -ENOSYS;
  

•   }
  

•   /*
  

•    * Calculate the crc32 for the updated FWU metadata
  

•    * and put the updated value in the FWU metadata crc32
  

•    * field
  

•    */
  

•   buf = &mdata->version;
  

•   mdata->crc32 = crc32(0, buf, sizeof(*mdata) - sizeof(u32));
  

•   return ops->update_mdata(dev, mdata);
  

+}

+UCLASS_DRIVER(fwu_mdata) = {

•   .id             = UCLASS_FWU_MDATA,
  

•   .name           = "fwu-mdata",
  

+}; diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h index 3ba69ad9a0..7da719c048 100644 --- a/include/dm/uclass-id.h +++ b/include/dm/uclass-id.h @@ -57,6 +57,7 @@ enum uclass_id { UCLASS_ETH_PHY, /* Ethernet PHY device */ UCLASS_FIRMWARE, /* Firmware */ UCLASS_FS_FIRMWARE_LOADER, /* Generic loader */

•   UCLASS_FWU_MDATA,       /* FWU Metadata Access */
    UCLASS_GPIO,            /* Bank of general-purpose I/O pins */
    UCLASS_HASH,            /* Hash device */
    UCLASS_HWSPINLOCK,      /* Hardware semaphores */
  

diff --git a/include/fwu.h b/include/fwu.h new file mode 100644 index 0000000000..f9e44e7b39 --- /dev/null +++ b/include/fwu.h @@ -0,0 +1,49 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/*

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

+#if !defined _FWU_H_ +#define _FWU_H_

+#include <blk.h> +#include <efi.h>

+#include <linux/types.h>

+struct fwu_mdata; +struct udevice;

+/**

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

+struct fwu_mdata_ops {

•   int (*mdata_check)(struct udevice *dev);
  

•   int (*get_mdata)(struct udevice *dev, struct fwu_mdata **mdata);
  

•   int (*update_mdata)(struct udevice *dev, struct fwu_mdata *mdata);
  

+};

+#define FWU_MDATA_VERSION 0x1

+#define FWU_MDATA_GUID \

•   EFI_GUID(0x8a7a84a0, 0x8387, 0x40f6, 0xab, 0x41, \
  

•            0xa8, 0xb9, 0xa5, 0xa6, 0x0d, 0x23)
  

+int fwu_get_mdata(struct fwu_mdata **mdata); +int fwu_update_mdata(struct fwu_mdata *mdata); +int fwu_get_active_index(u32 *active_idx); +int fwu_update_active_index(u32 active_idx); +int fwu_get_image_alt_num(efi_guid_t *image_type_id, u32 update_bank,

•                     int *alt_num);
  

+int fwu_mdata_check(void); +int fwu_revert_boot_index(void); +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);

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

•                    int *alt_num);
  

+#endif /* _FWU_H_ */ diff --git a/include/fwu_mdata.h b/include/fwu_mdata.h new file mode 100644 index 0000000000..701efbba03 --- /dev/null +++ b/include/fwu_mdata.h @@ -0,0 +1,67 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/*

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

+#if !defined _FWU_MDATA_H_ +#define _FWU_MDATA_H_

+#include <efi.h>

+/**

• • struct fwu_image_bank_info - firmware image information
• • @image_uuid: Guid value of the image in this bank
• • @accepted: Acceptance status of the image
• • @reserved: Reserved
• • The structure contains image specific fields which are
• • used to identify the image and to specify the image's
• • acceptance status
• */

+struct fwu_image_bank_info {

•   efi_guid_t  image_uuid;
  

•   uint32_t accepted;
  

•   uint32_t reserved;
  

+} __attribute__((__packed__));

+/**

• • struct fwu_image_entry - information for a particular type of image
• • @image_type_uuid: Guid value for identifying the image type
• • @location_uuid: Guid of the storage volume where the image is located
• • @img_bank_info: Array containing properties of images
• • This structure contains information on various types of updatable
• • firmware images. Each image type then contains an array of image
• • information per bank.
• */

+struct fwu_image_entry {

•   efi_guid_t image_type_uuid;
  

•   efi_guid_t location_uuid;
  

•   struct fwu_image_bank_info img_bank_info[CONFIG_FWU_NUM_BANKS];
  

+} __attribute__((__packed__));

+/**

• • struct fwu_mdata - FWU metadata structure for multi-bank updates
• • @crc32: crc32 value for the FWU metadata
• • @version: FWU metadata version
• • @active_index: Index of the bank currently used for booting images
• • @previous_active_inde: Index of the bank used before the current bank

• •                    being used for booting
    

• • @img_entry: Array of information on various firmware images that can

• •         be updated
    

• • This structure is used to store all the needed information for performing
• • multi bank updates on the platform. This contains info on the bank being
• • used to boot along with the information needed for identification of
• • individual images
• */

+struct fwu_mdata {

•   uint32_t crc32;
  

•   uint32_t version;
  

•   uint32_t active_index;
  

•   uint32_t previous_active_index;
  

•   struct fwu_image_entry img_entry[CONFIG_FWU_NUM_IMAGES_PER_BANK];
  

+} __attribute__((__packed__));

+#endif /* _FWU_MDATA_H_ */

2.25.1

Hi,

On 6/9/22 14:29, Sughosh Ganu 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 formated with GPT based partition scheme.

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

drivers/fwu-mdata/Kconfig | 9 + drivers/fwu-mdata/Makefile | 1 + drivers/fwu-mdata/fwu_mdata_gpt_blk.c | 404 ++++++++++++++++++++++++++ include/fwu.h | 2 + 4 files changed, 416 insertions(+) create mode 100644 drivers/fwu-mdata/fwu_mdata_gpt_blk.c

diff --git a/drivers/fwu-mdata/Kconfig b/drivers/fwu-mdata/Kconfig index d6a21c8e19..d5edef19d6 100644 --- a/drivers/fwu-mdata/Kconfig +++ b/drivers/fwu-mdata/Kconfig @@ -5,3 +5,12 @@ config DM_FWU_MDATA 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 DM && HAVE_BLOCK_DEVICE && 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 index 7fec7171f4..12a5b4fe04 100644 --- a/drivers/fwu-mdata/Makefile +++ b/drivers/fwu-mdata/Makefile @@ -4,3 +4,4 @@ #

obj-$(CONFIG_DM_FWU_MDATA) += fwu-mdata-uclass.o +obj-$(CONFIG_FWU_MDATA_GPT_BLK) += fwu_mdata_gpt_blk.o

It is strange to have '_' and '-' in file name for the same directory

=> to be coherent = fwu-mdata-gpt-blk.c

diff --git a/drivers/fwu-mdata/fwu_mdata_gpt_blk.c b/drivers/fwu-mdata/fwu_mdata_gpt_blk.c new file mode 100644 index 0000000000..329bd3779b --- /dev/null +++ b/drivers/fwu-mdata/fwu_mdata_gpt_blk.c @@ -0,0 +1,404 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

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

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

+#include <dm/device-internal.h> +#include <linux/errno.h> +#include <linux/types.h> +#include <u-boot/crc.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)

[...]

+int fwu_gpt_mdata_check(struct udevice *dev) +{

• /*

• * Check if both the copies of the FWU metadata are
  

• * valid. If one has gone bad, restore it from the
  

• * other good copy.
  

• */
  

• return gpt_check_mdata_validity(dev);

+}

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

• struct blk_desc *desc;
• desc = dev_get_uclass_plat(dev_get_priv(dev));

as dev = fwu_mdata_gpt_blk(UCLASS_FWU_MDATA)

dev_get_priv(dev) => get value saved in dev_set_priv(dev, mdata_dev);

even if it is OK, it not clear here.

can you add a struct to prepare addition of other elements in privdata:

struct fwu_mdata_gpt_blk_priv { struct udevice *blk_dev; }

+ struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);

+ struct blk_desc *desc;

+ desc = dev_get_uclass_plat(priv->blk_dev);

• if (!desc) {

• log_err("Block device not found\n");
  

• return -ENODEV;
  

• }
• return gpt_get_mdata(desc, mdata);

+}

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

• u32 phandle;
• int ret, size;
• struct udevice *parent, *child;
• const fdt32_t *phandle_p = NULL;
• phandle_p = ofnode_get_property(dev_ofnode(dev), "fwu-mdata-store",

• 			&size);
  

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

it is more simple

• if (!phandle_p) {

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

• return -ENOENT;
  

• }
• phandle = fdt32_to_cpu(*phandle_p);

or phandle can be read directly by

+ ret =dev_read_phandle_with_args(dev, "fwu-mdata-store", NULL, 0, 0, phandle_p)

+ if (ret) { + log_err("fwu-mdata-store property not found\n"); + return ret; + }

• ret = device_get_global_by_ofnode(ofnode_get_by_phandle(phandle),

• 			  &parent);
  

• if (ret)

• return ret;
  

• ret = -ENODEV;
• for (device_find_first_child(parent, &child); child;

•     device_find_next_child(&child)) {
  

• if (device_get_uclass_id(child) == UCLASS_BLK) {
  

• 	*mdata_dev = child;
  

• 	ret = 0;
  

• }
  

• }
• return ret;

+}

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

• int ret;
• struct udevice *mdata_dev = NULL;
• ret = fwu_get_mdata_device(dev, &mdata_dev);
• if (ret)

• return ret;
  

• dev_set_priv(dev, mdata_dev);

Avoid to use dev_set_priv() in driver :

/**  * dev_set_priv() - Set the private data for a device  *  * This is normally handled by driver model, which automatically allocates  * private data when an 'auto' size if provided by the driver.  *  * Use this function to override normal operation for special situations, such  * as needing to allocate a variable amount of data.  *

...

+ struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);

+ priv->blk_dev = mdata_dev;

• return 0;

+}

+static const struct fwu_mdata_ops fwu_gpt_blk_ops = {

• .mdata_check = 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 f9e44e7b39..3b1ee4e83e 100644 --- a/include/fwu.h +++ b/include/fwu.h @@ -39,6 +39,8 @@ int fwu_get_active_index(u32 *active_idx); int fwu_update_active_index(u32 active_idx); int fwu_get_image_alt_num(efi_guid_t *image_type_id, u32 update_bank, int *alt_num); +int fwu_get_mdata_device(struct udevice *dev, struct udevice **mdata_dev); +int fwu_verify_mdata(struct fwu_mdata *mdata, bool pri_part); int fwu_mdata_check(void); int fwu_revert_boot_index(void); int fwu_accept_image(efi_guid_t *img_type_id, u32 bank);

Regards

Patrick

hi Patrick, Apologies for the late reply. I had missed out replying to the review comments on this patch. There are some review comments on the Synquacer patches which need to be taken care of by another engineer. Once those review comments are taken care of, I will be sending the next version.

On Tue, 21 Jun 2022 at 15:04, Patrick DELAUNAY patrick.delaunay@foss.st.com wrote:

Hi,

On 6/9/22 14:29, Sughosh Ganu 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 formated with GPT based partition scheme.

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

drivers/fwu-mdata/Kconfig | 9 + drivers/fwu-mdata/Makefile | 1 + drivers/fwu-mdata/fwu_mdata_gpt_blk.c | 404 ++++++++++++++++++++++++++ include/fwu.h | 2 + 4 files changed, 416 insertions(+) create mode 100644 drivers/fwu-mdata/fwu_mdata_gpt_blk.c

<snip>

...

diff --git a/drivers/fwu-mdata/Makefile b/drivers/fwu-mdata/Makefile index 7fec7171f4..12a5b4fe04 100644 --- a/drivers/fwu-mdata/Makefile +++ b/drivers/fwu-mdata/Makefile @@ -4,3 +4,4 @@ #

obj-$(CONFIG_DM_FWU_MDATA) += fwu-mdata-uclass.o +obj-$(CONFIG_FWU_MDATA_GPT_BLK) += fwu_mdata_gpt_blk.o

It is strange to have '_' and '-' in file name for the same directory

=> to be coherent = fwu-mdata-gpt-blk.c

I see this kind of naming style in many other directories under drivers/. The uclass file is named using the foo-uclass.c, while the other driver files are named bar_driver.c

...

diff --git a/drivers/fwu-mdata/fwu_mdata_gpt_blk.c b/drivers/fwu-mdata/fwu_mdata_gpt_blk.c new file mode 100644 index 0000000000..329bd3779b --- /dev/null +++ b/drivers/fwu-mdata/fwu_mdata_gpt_blk.c @@ -0,0 +1,404 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

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

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

+#include <dm/device-internal.h> +#include <linux/errno.h> +#include <linux/types.h> +#include <u-boot/crc.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)

[...]

...

+int fwu_gpt_mdata_check(struct udevice *dev) +{

• /*
  

•  * Check if both the copies of the FWU metadata are
  

•  * valid. If one has gone bad, restore it from the
  

•  * other good copy.
  

•  */
  

• return gpt_check_mdata_validity(dev);
  

+}

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

• struct blk_desc *desc;
  

• desc = dev_get_uclass_plat(dev_get_priv(dev));
  

as dev = fwu_mdata_gpt_blk(UCLASS_FWU_MDATA)

dev_get_priv(dev) => get value saved in dev_set_priv(dev, mdata_dev);

even if it is OK, it not clear here.

can you add a struct to prepare addition of other elements in privdata:

struct fwu_mdata_gpt_blk_priv { struct udevice *blk_dev; }

• struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);

• struct blk_desc *desc;

• desc = dev_get_uclass_plat(priv->blk_dev);

Okay. Will add a priv structure as you suggest.

...

• if (!desc) {
  

•         log_err("Block device not found\n");
  

•         return -ENODEV;
  

• }
  

• return gpt_get_mdata(desc, mdata);
  

+}

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

• u32 phandle;
  

• int ret, size;
  

• struct udevice *parent, *child;
  

• const fdt32_t *phandle_p = NULL;
  

• phandle_p = ofnode_get_property(dev_ofnode(dev), "fwu-mdata-store",
  

•                                 &size);
  

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

it is more simple

Okay

...

• if (!phandle_p) {
  

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

•         return -ENOENT;
  

• }
  

• phandle = fdt32_to_cpu(*phandle_p);
  

or phandle can be read directly by

• ret =dev_read_phandle_with_args(dev, "fwu-mdata-store", NULL, 0, 0,

phandle_p)

I did not understand this review comment properly. In any case, I am using the API, dev_read_prop that you suggested above to read the phandle pointer.

•   if (ret) {
  

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

•           return ret;
  

•   }
  
  

...

• ret = device_get_global_by_ofnode(ofnode_get_by_phandle(phandle),
  

•                                   &parent);
  

• if (ret)
  

•         return ret;
  

• ret = -ENODEV;
  

• for (device_find_first_child(parent, &child); child;
  

•      device_find_next_child(&child)) {
  

•         if (device_get_uclass_id(child) == UCLASS_BLK) {
  

•                 *mdata_dev = child;
  

•                 ret = 0;
  

•         }
  

• }
  

• return ret;
  

+}

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

• int ret;
  

• struct udevice *mdata_dev = NULL;
  

• ret = fwu_get_mdata_device(dev, &mdata_dev);
  

• if (ret)
  

•         return ret;
  

• dev_set_priv(dev, mdata_dev);
  

Avoid to use dev_set_priv() in driver :

/**

• dev_set_priv() - Set the private data for a device
• This is normally handled by driver model, which automatically allocates
• private data when an 'auto' size if provided by the driver.
• Use this function to override normal operation for special

situations, such

• as needing to allocate a variable amount of data.

...

• struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);

• priv->blk_dev = mdata_dev;

Okay

...

• return 0;
  

+}

+static const struct fwu_mdata_ops fwu_gpt_blk_ops = {

• .mdata_check = 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),

Okay. I have incorporated these review comments. Thanks.

-sughosh

...

+}; diff --git a/include/fwu.h b/include/fwu.h index f9e44e7b39..3b1ee4e83e 100644 --- a/include/fwu.h +++ b/include/fwu.h @@ -39,6 +39,8 @@ int fwu_get_active_index(u32 *active_idx); int fwu_update_active_index(u32 active_idx); int fwu_get_image_alt_num(efi_guid_t *image_type_id, u32 update_bank, int *alt_num); +int fwu_get_mdata_device(struct udevice *dev, struct udevice **mdata_dev); +int fwu_verify_mdata(struct fwu_mdata *mdata, bool pri_part); int fwu_mdata_check(void); int fwu_revert_boot_index(void); int fwu_accept_image(efi_guid_t *img_type_id, u32 bank);

Regards

Patrick

hi Etienne,

On Tue, 21 Jun 2022 at 16:26, Etienne Carriere etienne.carriere@linaro.org wrote:

Hello Sughosh,

On Thu, 9 Jun 2022 at 14: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 formated with GPT based partition scheme.

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

drivers/fwu-mdata/Kconfig | 9 + drivers/fwu-mdata/Makefile | 1 + drivers/fwu-mdata/fwu_mdata_gpt_blk.c | 404 ++++++++++++++++++++++++++ include/fwu.h | 2 + 4 files changed, 416 insertions(+) create mode 100644 drivers/fwu-mdata/fwu_mdata_gpt_blk.c

diff --git a/drivers/fwu-mdata/Kconfig b/drivers/fwu-mdata/Kconfig index d6a21c8e19..d5edef19d6 100644 --- a/drivers/fwu-mdata/Kconfig +++ b/drivers/fwu-mdata/Kconfig @@ -5,3 +5,12 @@ config DM_FWU_MDATA 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 DM && HAVE_BLOCK_DEVICE && 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 index 7fec7171f4..12a5b4fe04 100644 --- a/drivers/fwu-mdata/Makefile +++ b/drivers/fwu-mdata/Makefile @@ -4,3 +4,4 @@ #

obj-$(CONFIG_DM_FWU_MDATA) += fwu-mdata-uclass.o +obj-$(CONFIG_FWU_MDATA_GPT_BLK) += fwu_mdata_gpt_blk.o diff --git a/drivers/fwu-mdata/fwu_mdata_gpt_blk.c b/drivers/fwu-mdata/fwu_mdata_gpt_blk.c new file mode 100644 index 0000000000..329bd3779b --- /dev/null +++ b/drivers/fwu-mdata/fwu_mdata_gpt_blk.c @@ -0,0 +1,404 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

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

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

+#include <dm/device-internal.h> +#include <linux/errno.h> +#include <linux/types.h> +#include <u-boot/crc.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)

<snip>

...

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

•   int ret;
  

•   u16 primary_mpart = 0, secondary_mpart = 0;
  

•   ret = gpt_get_mdata_partitions(desc, &primary_mpart,
  

•                                  &secondary_mpart);
  

•   if (ret < 0) {
  

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

•           return -ENODEV;
  

•   }
  

•   *mdata = malloc(sizeof(struct fwu_mdata));
  

•   if (!*mdata) {
  

•           log_err("Unable to allocate memory for reading FWU metadata\n");
  

•           return -ENOMEM;
  

•   }
  

•   ret = gpt_read_mdata(desc, *mdata, primary_mpart);
  

•   if (ret < 0) {
  

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

•           return -EIO;
  

•   }
  

•   ret = fwu_verify_mdata(*mdata, 1);
  

•   if (!ret)
  

Upon success, I think this function should also either ensure secondary_part contains a valid copy of primary part, Maybe this function should call gpt_check_mdata_validity() and then read mdata content.

Okay

...

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

•   if (ret < 0) {
  

•           log_err("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 -1;
  

+}

+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;
  

•   u16 primary_mpart = 0, secondary_mpart = 0;
  

•   u16 valid_partitions, invalid_partitions;
  

•   desc = dev_get_uclass_plat(dev_get_priv(dev));
  

•   if (!desc) {
  

•           log_err("Block device not found\n");
  

•           return -ENODEV;
  

•   }
  

•   /*
  

•    * 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, &primary_mpart,
  

•                                  &secondary_mpart);
  

•   if (ret < 0) {
  

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

•           return -ENODEV;
  

•   }
  

•   ret = gpt_read_mdata(desc, &pri_mdata, primary_mpart);
  

•   if (ret < 0) {
  

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

•           goto secondary_read;
  

•   }
  

•   ret = fwu_verify_mdata(&pri_mdata, 1);
  

•   if (!ret)
  

•           valid_partitions |= PRIMARY_PART;
  

+secondary_read:

•   /* Now check the secondary partition */
  

•   ret = gpt_read_mdata(desc, &secondary_mdata, secondary_mpart);
  

•   if (ret < 0) {
  

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

•           goto mdata_restore;
  

•   }
  

•   ret = fwu_verify_mdata(&secondary_mdata, 0);
  

•   if (!ret)
  

•           valid_partitions |= SECONDARY_PART;
  

+mdata_restore:

•   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_err("Both FWU metadata copies are valid but do not match. Please check!\n");
  

I think this function should select one of the part and copies to the other, e.g. assume primary is fine, copy primary to secondary and return upon success.

In this case, I think there is some kind of an unexpected scenario which has resulted in both the metadata partitions being valid but not being the same. Should this not be flagged as an error for the user to handle? Moreover, which metadata partition do we assume to be the correct one . We are just calling one partition as primary, and another one secondary. But the spec does not give any more importance to one over the other -- the spec just says that both the copies should be the same.

...

•           }
  

•           goto out;
  

•   }
  

•   ret = -1;
  

•   if (!(valid_partitions & BOTH_PARTS))
  

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

•                                   primary_mpart : secondary_mpart);
  

•   if (ret < 0)
  

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

•                   (invalid_partitions == PRIMARY_PART) ?
  

•                   "primary" : "secondary");
  

+out:

•   return ret;
  

+}

+int fwu_gpt_mdata_check(struct udevice *dev)

Function should be static.

Will change all the relevant functions as per your comment.

...

+{

•   /*
  

•    * Check if both the copies of the FWU metadata are
  

•    * valid. If one has gone bad, restore it from the
  

•    * other good copy.
  

•    */
  

•   return gpt_check_mdata_validity(dev);
  

+}

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

Function should be static.

...

+{

•   struct blk_desc *desc;
  

•   desc = dev_get_uclass_plat(dev_get_priv(dev));
  

•   if (!desc) {
  

•           log_err("Block device not found\n");
  

•           return -ENODEV;
  

•   }
  

•   return gpt_get_mdata(desc, mdata);
  

+}

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

Function could be static (and declaration removed from fwu.h).

Okay

...

+{

•   u32 phandle;
  

•   int ret, size;
  

•   struct udevice *parent, *child;
  

•   const fdt32_t *phandle_p = NULL;
  

•   phandle_p = ofnode_get_property(dev_ofnode(dev), "fwu-mdata-store",
  

•                                   &size);
  

Should this be retrieved from driver's ::of_to_plat method?

Patrick has suggested another API for getting the phandle. Thanks.

-sughosh

Br, etienne

...

•   if (!phandle_p) {
  

•           log_err("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;
  

•   ret = -ENODEV;
  

•   for (device_find_first_child(parent, &child); child;
  

•        device_find_next_child(&child)) {
  

•           if (device_get_uclass_id(child) == UCLASS_BLK) {
  

•                   *mdata_dev = child;
  

•                   ret = 0;
  

•           }
  

•   }
  

•   return ret;
  

+}

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

•   int ret;
  

•   struct udevice *mdata_dev = NULL;
  

•   ret = fwu_get_mdata_device(dev, &mdata_dev);
  

•   if (ret)
  

•           return ret;
  

•   dev_set_priv(dev, mdata_dev);
  

•   return 0;
  

+}

+static const struct fwu_mdata_ops fwu_gpt_blk_ops = {

•   .mdata_check = 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,
  

+}; diff --git a/include/fwu.h b/include/fwu.h index f9e44e7b39..3b1ee4e83e 100644 --- a/include/fwu.h +++ b/include/fwu.h @@ -39,6 +39,8 @@ int fwu_get_active_index(u32 *active_idx); int fwu_update_active_index(u32 active_idx); int fwu_get_image_alt_num(efi_guid_t *image_type_id, u32 update_bank, int *alt_num); +int fwu_get_mdata_device(struct udevice *dev, struct udevice **mdata_dev); +int fwu_verify_mdata(struct fwu_mdata *mdata, bool pri_part); int fwu_mdata_check(void); int fwu_revert_boot_index(void); int fwu_accept_image(efi_guid_t *img_type_id, u32 bank); -- 2.25.1

Hi,

On 6/9/22 14:29, Sughosh Ganu wrote:

The FWU metadata structure is accessed through the driver model interface. On the stm32mp157c-dk2 board, the FWU metadata is stored on the uSD card. Add the fwu-mdata node on the u-boot specifc dtsi file for accessing the metadata structure.

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

arch/arm/dts/stm32mp157c-dk2-u-boot.dtsi | 7 +++++++ 1 file changed, 7 insertions(+)

diff --git a/arch/arm/dts/stm32mp157c-dk2-u-boot.dtsi b/arch/arm/dts/stm32mp157c-dk2-u-boot.dtsi index 06ef3a4095..24f86209db 100644 --- a/arch/arm/dts/stm32mp157c-dk2-u-boot.dtsi +++ b/arch/arm/dts/stm32mp157c-dk2-u-boot.dtsi @@ -4,3 +4,10 @@ */

#include "stm32mp157a-dk1-u-boot.dtsi"

+/ {

• fwu-mdata {

• compatible = "u-boot,fwu-mdata-gpt";
  

• fwu-mdata-store = <&sdmmc1>;
  

• };

+};

Reviewed-by: Patrick Delaunay patrick.delaunay@foss.st.com

Thanks Patrick

hi Ilias,

On Fri, 10 Jun 2022 at 17:23, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:

Hi Sughosh,

On Thu, Jun 09, 2022 at 05:59:53PM +0530, Sughosh Ganu wrote:

...

Add helper functions needed for accessing the FWU metadata which contains information on the updatable images. These functions have been added for the STM32MP157C-DK2 board which has the updatable images on the uSD card, formatted as GPT partitions.

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

board/st/stm32mp1/stm32mp1.c | 115 +++++++++++++++++++++++++++++++++++ include/fwu.h | 2 + 2 files changed, 117 insertions(+)

diff --git a/board/st/stm32mp1/stm32mp1.c b/board/st/stm32mp1/stm32mp1.c index 62d98ad776..e68bf09955 100644 --- a/board/st/stm32mp1/stm32mp1.c +++ b/board/st/stm32mp1/stm32mp1.c @@ -7,9 +7,11 @@

#include <common.h> #include <adc.h> +#include <blk.h> #include <bootm.h> #include <clk.h> #include <config.h> +#include <dfu.h> #include <dm.h> #include <efi_loader.h> #include <env.h> @@ -25,9 +27,11 @@ #include <log.h> #include <malloc.h> #include <misc.h> +#include <mmc.h> #include <mtd_node.h> #include <net.h> #include <netdev.h> +#include <part.h> #include <phy.h> #include <remoteproc.h> #include <reset.h> @@ -967,3 +971,114 @@ static void board_copro_image_process(ulong fw_image, size_t fw_size) }

U_BOOT_FIT_LOADABLE_HANDLER(IH_TYPE_COPRO, board_copro_image_process);

+#if defined(CONFIG_FWU_MULTI_BANK_UPDATE) +#include <fwu.h> +#include <fwu_mdata.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 gpt_plat_get_alt_num(struct blk_desc *desc, efi_guid_t *image_guid,

•                         int *alt_num)
  

Does this really need to be defined per platform?

Most of the stuff in here are generic apart from the info of were the metadata is stored. So wouldn't it better to move this in the generic API and add an argument for the dfu device type?

The platform portion would then just call this function with an extra arg e.g DFU_DEV_MMC

Yes, it makes sense. I will make the change that you suggest. Thanks.

-sughosh

...

+{

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

[...]

Regards /Ilias

On Tue, 21 Jun 2022 at 15:19, Patrick DELAUNAY patrick.delaunay@foss.st.com wrote:

Hi,

On 6/9/22 14:29, Sughosh Ganu wrote:

...

Add helper functions needed for accessing the FWU metadata which contains information on the updatable images. These functions have been added for the STM32MP157C-DK2 board which has the updatable images on the uSD card, formatted as GPT partitions.

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

board/st/stm32mp1/stm32mp1.c | 115 +++++++++++++++++++++++++++++++++++ include/fwu.h | 2 + 2 files changed, 117 insertions(+)

diff --git a/board/st/stm32mp1/stm32mp1.c b/board/st/stm32mp1/stm32mp1.c index 62d98ad776..e68bf09955 100644 --- a/board/st/stm32mp1/stm32mp1.c +++ b/board/st/stm32mp1/stm32mp1.c @@ -7,9 +7,11 @@

#include <common.h> #include <adc.h> +#include <blk.h> #include <bootm.h> #include <clk.h> #include <config.h> +#include <dfu.h> #include <dm.h> #include <efi_loader.h> #include <env.h> @@ -25,9 +27,11 @@ #include <log.h> #include <malloc.h> #include <misc.h> +#include <mmc.h> #include <mtd_node.h> #include <net.h> #include <netdev.h> +#include <part.h> #include <phy.h> #include <remoteproc.h> #include <reset.h> @@ -967,3 +971,114 @@ static void board_copro_image_process(ulong fw_image, size_t fw_size) }

U_BOOT_FIT_LOADABLE_HANDLER(IH_TYPE_COPRO, board_copro_image_process);

+#if defined(CONFIG_FWU_MULTI_BANK_UPDATE) +#include <fwu.h> +#include <fwu_mdata.h>

[...]

...

+#endif /* CONFIG_FWU_MULTI_BANK_UPDATE */ diff --git a/include/fwu.h b/include/fwu.h index 3b1ee4e83e..36e58afa29 100644 --- a/include/fwu.h +++ b/include/fwu.h @@ -46,6 +46,8 @@ int fwu_revert_boot_index(void); 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);

Added empty line

Will remove

...

int fwu_plat_get_alt_num(struct udevice *dev, efi_guid_t *image_guid, int *alt_num); +int fwu_plat_get_update_index(u32 *update_idx); #endif /* _FWU_H_ */

And I am agree with Ilias remark, should be generic

=> search on the current UCLASS_FWU_MDATA

    perhaps need a new ops in u-class ? as implementation can be

different for GPT and MTD.

My understanding of Ilias's comments was that the function can be generic for all GPT based platforms. But I will check if this can be reused for both GPT and MTD devices, on the lines that you mention above. Thanks.

-sughosh

That's looks ok to me, but I'd rather ST people to have a look

On Thu, 9 Jun 2022 at 15:31, Sughosh Ganu sughosh.ganu@linaro.org wrote:

The FWU Multi Bank Update feature allows the platform to boot the firmware images from one of the partitions(banks). The first stage bootloader(fsbl) passes the value of the boot index, i.e. the bank from which the firmware images were booted from to U-Boot. On the STM32MP157C-DK2 board, this value is passed through one of the SoC's backup register. Add a function to read the boot index value from the backup register.

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

arch/arm/mach-stm32mp/include/mach/stm32.h | 4 ++++ board/st/stm32mp1/stm32mp1.c | 7 +++++++ include/fwu.h | 2 +- 3 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-stm32mp/include/mach/stm32.h b/arch/arm/mach-stm32mp/include/mach/stm32.h index 47e88fc3dc..40995ee142 100644 --- a/arch/arm/mach-stm32mp/include/mach/stm32.h +++ b/arch/arm/mach-stm32mp/include/mach/stm32.h @@ -100,6 +100,7 @@ enum boot_device { #define TAMP_BACKUP_REGISTER(x) (STM32_TAMP_BASE + 0x100 + 4 * x) #define TAMP_BACKUP_MAGIC_NUMBER TAMP_BACKUP_REGISTER(4) #define TAMP_BACKUP_BRANCH_ADDRESS TAMP_BACKUP_REGISTER(5) +#define TAMP_FWU_BOOT_INFO_REG TAMP_BACKUP_REGISTER(10) #define TAMP_COPRO_RSC_TBL_ADDRESS TAMP_BACKUP_REGISTER(17) #define TAMP_COPRO_STATE TAMP_BACKUP_REGISTER(18) #define TAMP_BOOT_CONTEXT TAMP_BACKUP_REGISTER(20) @@ -118,6 +119,9 @@ enum boot_device { #define TAMP_BOOT_INSTANCE_MASK GENMASK(3, 0) #define TAMP_BOOT_FORCED_MASK GENMASK(7, 0) #define TAMP_BOOT_DEBUG_ON BIT(16) +#define TAMP_FWU_BOOT_IDX_MASK GENMASK(3, 0)

+#define TAMP_FWU_BOOT_IDX_OFFSET 0

enum forced_boot_mode { BOOT_NORMAL = 0x00, diff --git a/board/st/stm32mp1/stm32mp1.c b/board/st/stm32mp1/stm32mp1.c index e68bf09955..dff41ed6f6 100644 --- a/board/st/stm32mp1/stm32mp1.c +++ b/board/st/stm32mp1/stm32mp1.c @@ -1081,4 +1081,11 @@ int fwu_plat_get_update_index(u32 *update_idx) return ret; }

+void fwu_plat_get_bootidx(void *boot_idx) +{

•   u32 *bootidx = boot_idx;
  

•   *bootidx = (readl(TAMP_FWU_BOOT_INFO_REG) >>
  

•               TAMP_FWU_BOOT_IDX_OFFSET) & TAMP_FWU_BOOT_IDX_MASK;
  

+} #endif /* CONFIG_FWU_MULTI_BANK_UPDATE */ diff --git a/include/fwu.h b/include/fwu.h index 36e58afa29..41774ff9e2 100644 --- a/include/fwu.h +++ b/include/fwu.h @@ -46,7 +46,7 @@ int fwu_revert_boot_index(void); 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);

+void fwu_plat_get_bootidx(void *boot_idx); int fwu_plat_get_alt_num(struct udevice *dev, efi_guid_t *image_guid, int *alt_num); int fwu_plat_get_update_index(u32 *update_idx); -- 2.25.1

Acked-by: Ilias Apalodimas ilias.apalodimas@linaro.org

On Tue, 21 Jun 2022 at 16:57, Patrick DELAUNAY patrick.delaunay@foss.st.com wrote:

Hi,

On 6/9/22 14:29, Sughosh Ganu wrote:

...

The FWU Multi Bank Update feature allows the platform to boot the firmware images from one of the partitions(banks). The first stage bootloader(fsbl) passes the value of the boot index, i.e. the bank from which the firmware images were booted from to U-Boot. On the STM32MP157C-DK2 board, this value is passed through one of the SoC's backup register. Add a function to read the boot index value from the backup register.

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

arch/arm/mach-stm32mp/include/mach/stm32.h | 4 ++++ board/st/stm32mp1/stm32mp1.c | 7 +++++++ include/fwu.h | 2 +- 3 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-stm32mp/include/mach/stm32.h b/arch/arm/mach-stm32mp/include/mach/stm32.h index 47e88fc3dc..40995ee142 100644 --- a/arch/arm/mach-stm32mp/include/mach/stm32.h +++ b/arch/arm/mach-stm32mp/include/mach/stm32.h @@ -100,6 +100,7 @@ enum boot_device { #define TAMP_BACKUP_REGISTER(x) (STM32_TAMP_BASE + 0x100 + 4 * x) #define TAMP_BACKUP_MAGIC_NUMBER TAMP_BACKUP_REGISTER(4) #define TAMP_BACKUP_BRANCH_ADDRESS TAMP_BACKUP_REGISTER(5) +#define TAMP_FWU_BOOT_INFO_REG TAMP_BACKUP_REGISTER(10) #define TAMP_COPRO_RSC_TBL_ADDRESS TAMP_BACKUP_REGISTER(17) #define TAMP_COPRO_STATE TAMP_BACKUP_REGISTER(18) #define TAMP_BOOT_CONTEXT TAMP_BACKUP_REGISTER(20) @@ -118,6 +119,9 @@ enum boot_device { #define TAMP_BOOT_INSTANCE_MASK GENMASK(3, 0) #define TAMP_BOOT_FORCED_MASK GENMASK(7, 0) #define TAMP_BOOT_DEBUG_ON BIT(16) +#define TAMP_FWU_BOOT_IDX_MASK GENMASK(3, 0)

+#define TAMP_FWU_BOOT_IDX_OFFSET 0

please don't mix the 2 TAMP_FWU defines with define and enum for TAMP_BOOT

=> move the 2 defines before TAMP_COPRO defines.

#define TAMP_BACKUP_MAGIC_NUMBER TAMP_BACKUP_REGISTER(4) #define TAMP_BACKUP_BRANCH_ADDRESS TAMP_BACKUP_REGISTER(5)

• #define TAMP_FWU_BOOT_INFO_REG TAMP_BACKUP_REGISTER(10)

#define TAMP_COPRO_RSC_TBL_ADDRESS TAMP_BACKUP_REGISTER(17) #define TAMP_COPRO_STATE TAMP_BACKUP_REGISTER(18) #define TAMP_BOOT_CONTEXT TAMP_BACKUP_REGISTER(20) #define TAMP_BOOTCOUNT TAMP_BACKUP_REGISTER(21)

+#define TAMP_FWU_BOOT_IDX_MASK GENMASK(3, 0)

• #define TAMP_FWU_BOOT_IDX_OFFSET 0

Will change as per your suggestion. Thanks.

-sughosh

#define TAMP_COPRO_STATE_OFF 0 #define TAMP_COPRO_STATE_INIT 1

...

enum forced_boot_mode { BOOT_NORMAL = 0x00, diff --git a/board/st/stm32mp1/stm32mp1.c b/board/st/stm32mp1/stm32mp1.c index e68bf09955..dff41ed6f6 100644 --- a/board/st/stm32mp1/stm32mp1.c +++ b/board/st/stm32mp1/stm32mp1.c @@ -1081,4 +1081,11 @@ int fwu_plat_get_update_index(u32 *update_idx) return ret; }

+void fwu_plat_get_bootidx(void *boot_idx) +{

• u32 *bootidx = boot_idx;
  

• *bootidx = (readl(TAMP_FWU_BOOT_INFO_REG) >>
  

•             TAMP_FWU_BOOT_IDX_OFFSET) & TAMP_FWU_BOOT_IDX_MASK;
  

+} #endif /* CONFIG_FWU_MULTI_BANK_UPDATE */ diff --git a/include/fwu.h b/include/fwu.h index 36e58afa29..41774ff9e2 100644 --- a/include/fwu.h +++ b/include/fwu.h @@ -46,7 +46,7 @@ int fwu_revert_boot_index(void); 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);

+void fwu_plat_get_bootidx(void *boot_idx); int fwu_plat_get_alt_num(struct udevice *dev, efi_guid_t *image_guid, int *alt_num); int fwu_plat_get_update_index(u32 *update_idx);

With the modifications:

Reviewed-by: Patrick Delaunay patrick.delaunay@foss.st.com

Thanks Patrick

On 6/9/22 14:29, 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.

Add these checks in the board initialisation sequence, invoked after relocation.

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

common/board_r.c | 5 ++ include/fwu.h | 3 + lib/fwu_updates/fwu.c | 170 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 178 insertions(+) create mode 100644 lib/fwu_updates/fwu.c

diff --git a/common/board_r.c b/common/board_r.c index 6f4aca2077..33a600715d 100644 --- a/common/board_r.c +++ b/common/board_r.c @@ -15,6 +15,7 @@ #include <cpu_func.h> #include <exports.h> #include <flash.h> +#include <fwu.h> #include <hang.h> #include <image.h> #include <irq_func.h> @@ -797,6 +798,10 @@ static init_fnc_t init_sequence_r[] = { #if defined(CONFIG_PRAM) initr_mem, #endif

+#ifdef CONFIG_FWU_MULTI_BANK_UPDATE

• fwu_boottime_checks,

+#endif run_main_loop, };

diff --git a/include/fwu.h b/include/fwu.h index 41774ff9e2..8fbd91b463 100644 --- a/include/fwu.h +++ b/include/fwu.h @@ -33,6 +33,9 @@ struct fwu_mdata_ops { EFI_GUID(0x8a7a84a0, 0x8387, 0x40f6, 0xab, 0x41, \ 0xa8, 0xb9, 0xa5, 0xa6, 0x0d, 0x23)

+int fwu_boottime_checks(void); +u8 fwu_update_checks_pass(void);

• int fwu_get_mdata(struct fwu_mdata **mdata); int fwu_update_mdata(struct fwu_mdata *mdata); int fwu_get_active_index(u32 *active_idx);

diff --git a/lib/fwu_updates/fwu.c b/lib/fwu_updates/fwu.c new file mode 100644 index 0000000000..af884439fb --- /dev/null +++ b/lib/fwu_updates/fwu.c @@ -0,0 +1,170 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

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

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

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

+static u8 trial_state; +static u8 boottime_check;

+static int fwu_trial_state_check(void) +{

• int ret, i;
• efi_status_t status;
• efi_uintn_t var_size;
• u16 trial_state_ctr;
• u32 nimages, active_bank, var_attributes, active_idx;
• struct fwu_mdata *mdata = NULL;
• struct fwu_image_entry *img_entry;
• struct fwu_image_bank_info *img_bank_info;
• ret = fwu_get_mdata(&mdata);
• if (ret)

• return ret;
  

• ret = 0;
• nimages = CONFIG_FWU_NUM_IMAGES_PER_BANK;
• active_bank = mdata->active_index;
• img_entry = &mdata->img_entry[0];
• for (i = 0; i < nimages; i++) {

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

• if (!img_bank_info->accepted) {
  

• 	trial_state = 1;
  

• 	break;
  

• }
  

• }
• 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(L"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");
  

• 	active_idx = mdata->active_index;
  

• 	ret = fwu_revert_boot_index();
  

• 	if (ret) {
  

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

• 		goto out;
  

• 	}
  

• 	trial_state_ctr = 0;
  

• 	status = efi_set_variable_int(L"TrialStateCtr",
  

• 				      &efi_global_variable_guid,
  

• 				      var_attributes,
  

• 				      0,
  

• 				      &trial_state_ctr, false);
  

• 	if (status != EFI_SUCCESS) {
  

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

• 		ret = -1;
  

• 		goto out;
  

• 	}
  

• } else {
  

• 	status = efi_set_variable_int(L"TrialStateCtr",
  

• 				      &efi_global_variable_guid,
  

• 				      var_attributes,
  

• 				      var_size,
  

• 				      &trial_state_ctr, false);
  

• 	if (status != EFI_SUCCESS) {
  

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

• 		ret = -1;
  

• 		goto out;
  

• 	}
  

• }
  

• } else {

• trial_state_ctr = 0;
  

• status = efi_set_variable_int(L"TrialStateCtr",
  

• 			      &efi_global_variable_guid,
  

• 			      0,
  

• 			      0, &trial_state_ctr,
  

• 			      NULL);
  

• }

+out:

• free(mdata);
• return ret;

+}

+u8 fwu_update_checks_pass(void) +{

• return !trial_state && boottime_check;

+}

+int fwu_boottime_checks(void) +{

• int ret;
• struct udevice *dev;
• u32 boot_idx, active_idx;
• if (uclass_get_device(UCLASS_FWU_MDATA, 0, &dev) || !dev) {

• log_err("FWU Metadata device not found\n");
  

• return 0;
  

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

• return 0;
  

• }
• /*

• * 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_update_active_index(boot_idx);
  

• if (!ret)
  

• 	boottime_check = 1;
  

• return 0;
  

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

efi_init_obj_list() slows down the boot process. Why do want to invoke it if no EFI binary is launched?

See also Takahiro's hint in https://lore.kernel.org/all/20220615061616.GD58082@laputa/

Best regards

Heinrich

• return 0;
  

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

• boottime_check = 1;
  

• return 0;

+}

Hi Sughosh,

On Thu, 9 Jun 2022 at 14:31, Sughosh Ganu sughosh.ganu@linaro.org 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.

Add these checks in the board initialisation sequence, invoked after relocation.

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

common/board_r.c | 5 ++ include/fwu.h | 3 + lib/fwu_updates/fwu.c | 170 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 178 insertions(+) create mode 100644 lib/fwu_updates/fwu.c

diff --git a/common/board_r.c b/common/board_r.c index 6f4aca2077..33a600715d 100644 --- a/common/board_r.c +++ b/common/board_r.c @@ -15,6 +15,7 @@ #include <cpu_func.h> #include <exports.h> #include <flash.h> +#include <fwu.h> #include <hang.h> #include <image.h> #include <irq_func.h> @@ -797,6 +798,10 @@ static init_fnc_t init_sequence_r[] = { #if defined(CONFIG_PRAM) initr_mem, #endif

+#ifdef CONFIG_FWU_MULTI_BANK_UPDATE

•   fwu_boottime_checks,
  

+#endif run_main_loop, };

diff --git a/include/fwu.h b/include/fwu.h index 41774ff9e2..8fbd91b463 100644 --- a/include/fwu.h +++ b/include/fwu.h @@ -33,6 +33,9 @@ struct fwu_mdata_ops { EFI_GUID(0x8a7a84a0, 0x8387, 0x40f6, 0xab, 0x41, \ 0xa8, 0xb9, 0xa5, 0xa6, 0x0d, 0x23)

+int fwu_boottime_checks(void); +u8 fwu_update_checks_pass(void);

int fwu_get_mdata(struct fwu_mdata **mdata); int fwu_update_mdata(struct fwu_mdata *mdata); int fwu_get_active_index(u32 *active_idx); diff --git a/lib/fwu_updates/fwu.c b/lib/fwu_updates/fwu.c new file mode 100644 index 0000000000..af884439fb --- /dev/null +++ b/lib/fwu_updates/fwu.c @@ -0,0 +1,170 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

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

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

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

+static u8 trial_state; +static u8 boottime_check;

+static int fwu_trial_state_check(void) +{

•   int ret, i;
  

•   efi_status_t status;
  

•   efi_uintn_t var_size;
  

•   u16 trial_state_ctr;
  

•   u32 nimages, active_bank, var_attributes, active_idx;
  

•   struct fwu_mdata *mdata = NULL;
  

•   struct fwu_image_entry *img_entry;
  

•   struct fwu_image_bank_info *img_bank_info;
  

•   ret = fwu_get_mdata(&mdata);
  

•   if (ret)
  

•           return ret;
  

•   ret = 0;
  

•   nimages = CONFIG_FWU_NUM_IMAGES_PER_BANK;
  

•   active_bank = mdata->active_index;
  

•   img_entry = &mdata->img_entry[0];
  

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

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

•           if (!img_bank_info->accepted) {
  

•                   trial_state = 1;
  

•                   break;
  

•           }
  

•   }
  

•   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(L"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");
  

•                   active_idx = mdata->active_index;
  

•                   ret = fwu_revert_boot_index();
  

•                   if (ret) {
  

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

•                           goto out;
  

•                   }
  

•                   trial_state_ctr = 0;
  

•                   status = efi_set_variable_int(L"TrialStateCtr",
  

•                                                 &efi_global_variable_guid,
  

•                                                 var_attributes,
  

•                                                 0,
  

s/0/var_size/ ? Ditto 24 lines below.

•                                                 &trial_state_ctr, false);
  

•                   if (status != EFI_SUCCESS) {
  

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

•                           ret = -1;
  

•                           goto out;
  

•                   }
  

•           } else {
  

•                   status = efi_set_variable_int(L"TrialStateCtr",
  

•                                                 &efi_global_variable_guid,
  

•                                                 var_attributes,
  

•                                                 var_size,
  

•                                                 &trial_state_ctr, false);
  

•                   if (status != EFI_SUCCESS) {
  

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

•                           ret = -1;
  

•                           goto out;
  

•                   }
  

•           }
  

•   } else {
  

•           trial_state_ctr = 0;
  

•           status = efi_set_variable_int(L"TrialStateCtr",
  

•                                         &efi_global_variable_guid,
  

•                                         0,
  

•                                         0, &trial_state_ctr,
  

•                                         NULL);
  

check status value.

•   }
  

+out:

•   free(mdata);
  

•   return ret;
  

+}

+u8 fwu_update_checks_pass(void) +{

•   return !trial_state && boottime_check;
  

+}

+int fwu_boottime_checks(void) +{

•   int ret;
  

•   struct udevice *dev;
  

•   u32 boot_idx, active_idx;
  

•   if (uclass_get_device(UCLASS_FWU_MDATA, 0, &dev) || !dev) {
  

•           log_err("FWU Metadata device not found\n");
  

•           return 0;
  

•   }
  

•   ret = fwu_mdata_check();
  

•   if (ret) {
  

•           return 0;
  

•   }
  

•   /*
  

•    * 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_update_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();
  

•   if (!ret)
  

•           boottime_check = 1;
  

•   return 0;
  

+}

2.25.1