| Message ID | 20220817124323.375968-3-sughosh.ganu@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | FWU: Add FWU Multi Bank Update feature support | expand |
Hi Sughosh, On Wed, 17 Aug 2022 at 06:44, 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> > --- > Changes since V7: > * Rephrased the error message in fwu_update_active_index as per > suggestion from Ilias. > * Reworked the logic in fwu_get_image_alt_num() as per the suggestion > from Ilias. > > drivers/Kconfig | 2 + > drivers/Makefile | 1 + > drivers/fwu-mdata/Kconfig | 7 + > drivers/fwu-mdata/Makefile | 6 + > drivers/fwu-mdata/fwu-mdata-uclass.c | 463 +++++++++++++++++++++++++++ > include/dm/uclass-id.h | 1 + > include/fwu.h | 49 +++ > include/fwu_mdata.h | 67 ++++ > 8 files changed, 596 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 8b6fead351..75ac149d31 100644 > --- a/drivers/Kconfig > +++ b/drivers/Kconfig > @@ -44,6 +44,8 @@ source "drivers/fuzz/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 eba9940231..70bbc2f5e0 100644 > --- a/drivers/Makefile > +++ b/drivers/Makefile > @@ -84,6 +84,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 Please drop DM_ as there is no non-DM version > + 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..e53a8c9983 > --- /dev/null > +++ b/drivers/fwu-mdata/Makefile > @@ -0,0 +1,6 @@ > +# SPDX-License-Identifier: GPL-2.0-or-later > +# > +# 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..6bf7fa1b03 > --- /dev/null > +++ b/drivers/fwu-mdata/fwu-mdata-uclass.c > @@ -0,0 +1,463 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Copyright (c) 2022, Linaro Limited > + */ > + > +#define LOG_CATEGORY UCLASS_FWU_MDATA > + > +#include <common.h> > +#include <dm.h> > +#include <efi_loader.h> > +#include <fwu.h> > +#include <fwu_mdata.h> > +#include <log.h> > +#include <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); uclass_first_device() if there is only one > + 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; > + } This is actually a bug, so drop this. It is OK to have an assert() if you want one. > + > + 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; Please use an -Exxx value like -EPERM > + } > + > + 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) Return the active index rather than having an arg? > +{ > + int ret; > + struct fwu_mdata *mdata = NULL; > + > + ret = fwu_get_mdata(&mdata); Can you use a local var and avoid the malloc() / free() ? > + 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) uint > +{ > + int ret; > + struct fwu_mdata *mdata = NULL; > + > + if (active_idx > CONFIG_FWU_NUM_BANKS - 1) { >= CONFIG_FWU_NUM_BANKS > + log_err("Invalid active index value\n"); > + return -1; error code, pleae fix globally > + } > + > + 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) alt_nump is often used with driver model to indicate a return value.Again I wonder if the return value could be used? > +{ > + int ret, i; > + efi_guid_t *image_guid; > + struct udevice *dev = NULL; > + struct fwu_mdata *mdata = NULL; Please don't set things to NULL for no reason, it cluttlers the code. > + 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); Rather than this, see the pattern used by other uclasses > + if (ret) > + return ret; > + > + ret = fwu_get_mdata(&mdata); > + if (ret) { > + log_err("Unable to get valid FWU metadata\n"); > + goto out; > + } > + > + ret = -EINVAL; > + /* > + * The FWU metadata has been read. Now get the image_uuid for the > + * image with the update_bank. > + */ > + for (i = 0; i < CONFIG_FWU_NUM_IMAGES_PER_BANK; i++) { > + if (!guidcmp(image_type_id, > + &mdata->img_entry[i].image_type_uuid)) { > + img_entry = &mdata->img_entry[i]; > + img_bank_info = &img_entry->img_bank_info[update_bank]; > + image_guid = &img_bank_info->image_uuid; > + ret = fwu_plat_get_alt_num(dev, image_guid, alt_num); > + if (ret) { > + log_err("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); > + } > + > + goto out; > + } > + } > + > + log_err("Partition with the image type %pUs not found\n", > + image_type_id); > + > +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"); log_debug() to avoid code-size issue. This is a s/w bug. Please fix globally and see other uclasses for how this should be done. > + 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"); This is supposed to be a uclass so it should not be outputting this sort of thing except in debug mode. > + 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; > + 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; > + } > + > + 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(mdata); > + goto out; > + } > + } > + > + /* Image not found */ > + ret = -EINVAL; -ENOENT then? > + > +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); Use fwu_clrset_... since we have something similar in U-Boot > +} > + > +/** > + * 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. > + * > + * Note: This function is to be called first when modifying any fields > + * in the metadata. The sequence of calls to modify any field in the > + * metadata would be 1) fwu_get_mdata 2) Modify metadata, followed by > + * 3) fwu_update_mdata > + * > + * Return: 0 if OK, -ve on error > + * > + */ > +int fwu_get_mdata(struct fwu_mdata **mdata) > +{ > + 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. > + * > + * Note: This function is not to be called directly to update the > + * metadata fields. The sequence of function calls should be > + * 1) fwu_get_mdata() 2) Modify the medata fields 3) fwu_update_mdata() > + * > + * Return: 0 if OK, -ve on error > + * > + */ > +int fwu_update_mdata(struct 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 a432e43871..598a8c10a0 100644 > --- a/include/dm/uclass-id.h > +++ b/include/dm/uclass-id.h > @@ -58,6 +58,7 @@ enum uclass_id { > UCLASS_FIRMWARE, /* Firmware */ > UCLASS_FUZZING_ENGINE, /* Fuzzing engine */ > UCLASS_FS_FIRMWARE_LOADER, /* Generic loader */ > + UCLASS_FWU_MDATA, /* FWU Metadata Access */ > UCLASS_GPIO, /* Bank of general-purpose I/O pins */ > UCLASS_HASH, /* Hash device */ > UCLASS_HWSPINLOCK, /* Hardware semaphores */ > diff --git a/include/fwu.h b/include/fwu.h > new file mode 100644 > index 0000000000..e03cfff800 > --- /dev/null > +++ b/include/fwu.h > @@ -0,0 +1,49 @@ > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > +/* > + * Copyright (c) 2022, Linaro Limited > + */ > + > +#if !defined _FWU_H_ > +#define _FWU_H_ > + > +#include <blk.h> > +#include <efi.h> > + > +#include <linux/types.h> > + > +struct fwu_mdata; > +struct udevice; > + > +/** > + * @mdata_check: check the validity of the FWU metadata partitions > + * @get_mdata() - Get a FWU metadata copy > + * @update_mdata() - Update the FWU metadata copy > + */ > +struct fwu_mdata_ops { > + 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); Needs full comments - see other uclasses > +}; > + > +#define FWU_MDATA_VERSION 0x1 > + > +#define FWU_MDATA_GUID \ > + EFI_GUID(0x8a7a84a0, 0x8387, 0x40f6, 0xab, 0x41, \ > + 0xa8, 0xb9, 0xa5, 0xa6, 0x0d, 0x23) What is this? Comment? > + > +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); These all need full comments > +#endif /* _FWU_H_ */ > diff --git a/include/fwu_mdata.h b/include/fwu_mdata.h > new file mode 100644 > index 0000000000..72e3edab43 > --- /dev/null > +++ b/include/fwu_mdata.h > @@ -0,0 +1,67 @@ > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > +/* > + * Copyright (c) 2022, Linaro Limited > + */ > + > +#if !defined _FWU_MDATA_H_ > +#define _FWU_MDATA_H_ > + > +#include <efi.h> > + > +/** > + * struct fwu_image_bank_info - firmware image information > + * @image_uuid: Guid value of the image in this bank > + * @accepted: Acceptance status of the image > + * @reserved: Reserved > + * > + * The structure contains image specific fields which are > + * used to identify the image and to specify the image's > + * acceptance status > + */ > +struct fwu_image_bank_info { > + efi_guid_t image_uuid; > + uint32_t accepted; > + uint32_t reserved; > +} __attribute__((__packed__)); Why is this 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__)); why 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__)); same question > + > +#endif /* _FWU_MDATA_H_ */ > -- > 2.34.1 > Regards, Simon
hi Simon, On Thu, 18 Aug 2022 at 06:43, Simon Glass <sjg@chromium.org> wrote: > > Hi Sughosh, > > On Wed, 17 Aug 2022 at 06:44, 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> > > --- > > Changes since V7: > > * Rephrased the error message in fwu_update_active_index as per > > suggestion from Ilias. > > * Reworked the logic in fwu_get_image_alt_num() as per the suggestion > > from Ilias. > > > > drivers/Kconfig | 2 + > > drivers/Makefile | 1 + > > drivers/fwu-mdata/Kconfig | 7 + > > drivers/fwu-mdata/Makefile | 6 + > > drivers/fwu-mdata/fwu-mdata-uclass.c | 463 +++++++++++++++++++++++++++ > > include/dm/uclass-id.h | 1 + > > include/fwu.h | 49 +++ > > include/fwu_mdata.h | 67 ++++ > > 8 files changed, 596 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 8b6fead351..75ac149d31 100644 > > --- a/drivers/Kconfig > > +++ b/drivers/Kconfig > > @@ -44,6 +44,8 @@ source "drivers/fuzz/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 eba9940231..70bbc2f5e0 100644 > > --- a/drivers/Makefile > > +++ b/drivers/Makefile > > @@ -84,6 +84,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 > > Please drop DM_ as there is no non-DM version Okay > > > > + 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..e53a8c9983 > > --- /dev/null > > +++ b/drivers/fwu-mdata/Makefile > > @@ -0,0 +1,6 @@ > > +# SPDX-License-Identifier: GPL-2.0-or-later > > +# > > +# 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..6bf7fa1b03 > > --- /dev/null > > +++ b/drivers/fwu-mdata/fwu-mdata-uclass.c > > @@ -0,0 +1,463 @@ > > +// SPDX-License-Identifier: GPL-2.0-or-later > > +/* > > + * Copyright (c) 2022, Linaro Limited > > + */ > > + > > +#define LOG_CATEGORY UCLASS_FWU_MDATA > > + > > +#include <common.h> > > +#include <dm.h> > > +#include <efi_loader.h> > > +#include <fwu.h> > > +#include <fwu_mdata.h> > > +#include <log.h> > > +#include <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); > > uclass_first_device() if there is only one Okay > > > + 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; > > + } > > This is actually a bug, so drop this. It is OK to have an assert() if > you want one. Okay > > > + > > + 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; > > Please use an -Exxx value like -EPERM I don't think there is any value which relates to a crc mismatch. If you insist, I can think of -EIO, but I don't think -EPERM is a good match in this case. > > > + } > > + > > + 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) > > Return the active index rather than having an arg? I would prefer to keep it the way it is. That allows returning an error value and keep the active_index separate. Moreover it is just a single parameter. > > > +{ > > + int ret; > > + struct fwu_mdata *mdata = NULL; > > + > > + ret = fwu_get_mdata(&mdata); > > Can you use a local var and avoid the malloc() / free() ? Do you see any disadvantages of using space on the heap? If you don't have a strong opinion on this, I would prefer to keep it as is. > > > + 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) > > uint Are you referring to the function parameter? If so, it is a u32 since the active_index field in the metadata structure defined in the spec is a 32 bit value. > > > +{ > > + int ret; > > + struct fwu_mdata *mdata = NULL; > > + > > + if (active_idx > CONFIG_FWU_NUM_BANKS - 1) { > > >= CONFIG_FWU_NUM_BANKS > > > + log_err("Invalid active index value\n"); > > + return -1; > > error code, pleae fix globally Okay > > > + } > > + > > + 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) > > alt_nump is often used with driver model to indicate a return > value.Again I wonder if the return value could be used? Can you point me to an example? I could not find any instance of this. Again, I believe this is a matter of taste, so I would prefer the current method. > > > +{ > > + int ret, i; > > + efi_guid_t *image_guid; > > + struct udevice *dev = NULL; > > + struct fwu_mdata *mdata = NULL; > > Please don't set things to NULL for no reason, it cluttlers the code. This is being set to NULL in case the called function that is supposed to populate the metadata structure fails to do so. We can pass NULL to free, else it could result in freeing up of unrelated memory. > > > + 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); > > Rather than this, see the pattern used by other uclasses Can you point me to some example code. Also, not sure what the issue is with using this function. > > > + if (ret) > > + return ret; > > + > > + ret = fwu_get_mdata(&mdata); > > + if (ret) { > > + log_err("Unable to get valid FWU metadata\n"); > > + goto out; > > + } > > + > > + ret = -EINVAL; > > + /* > > + * The FWU metadata has been read. Now get the image_uuid for the > > + * image with the update_bank. > > + */ > > + for (i = 0; i < CONFIG_FWU_NUM_IMAGES_PER_BANK; i++) { > > + if (!guidcmp(image_type_id, > > + &mdata->img_entry[i].image_type_uuid)) { > > + img_entry = &mdata->img_entry[i]; > > + img_bank_info = &img_entry->img_bank_info[update_bank]; > > + image_guid = &img_bank_info->image_uuid; > > + ret = fwu_plat_get_alt_num(dev, image_guid, alt_num); > > + if (ret) { > > + log_err("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); > > + } > > + > > + goto out; > > + } > > + } > > + > > + log_err("Partition with the image type %pUs not found\n", > > + image_type_id); > > + > > +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"); > > log_debug() to avoid code-size issue. This is a s/w bug. Please fix > globally and see other uclasses for how this should be done. Okay > > > + 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"); > > This is supposed to be a uclass so it should not be outputting this > sort of thing except in debug mode. Will fix > > > + 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; > > + 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; > > + } > > + > > + 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(mdata); > > + goto out; > > + } > > + } > > + > > + /* Image not found */ > > + ret = -EINVAL; > > -ENOENT then? Okay > > > + > > +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); > > Use fwu_clrset_... since we have something similar in U-Boot Will change > > > +} > > + > > +/** > > + * 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. > > + * > > + * Note: This function is to be called first when modifying any fields > > + * in the metadata. The sequence of calls to modify any field in the > > + * metadata would be 1) fwu_get_mdata 2) Modify metadata, followed by > > + * 3) fwu_update_mdata > > + * > > + * Return: 0 if OK, -ve on error > > + * > > + */ > > +int fwu_get_mdata(struct fwu_mdata **mdata) > > +{ > > + 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. > > + * > > + * Note: This function is not to be called directly to update the > > + * metadata fields. The sequence of function calls should be > > + * 1) fwu_get_mdata() 2) Modify the medata fields 3) fwu_update_mdata() > > + * > > + * Return: 0 if OK, -ve on error > > + * > > + */ > > +int fwu_update_mdata(struct 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 a432e43871..598a8c10a0 100644 > > --- a/include/dm/uclass-id.h > > +++ b/include/dm/uclass-id.h > > @@ -58,6 +58,7 @@ enum uclass_id { > > UCLASS_FIRMWARE, /* Firmware */ > > UCLASS_FUZZING_ENGINE, /* Fuzzing engine */ > > UCLASS_FS_FIRMWARE_LOADER, /* Generic loader */ > > + UCLASS_FWU_MDATA, /* FWU Metadata Access */ > > UCLASS_GPIO, /* Bank of general-purpose I/O pins */ > > UCLASS_HASH, /* Hash device */ > > UCLASS_HWSPINLOCK, /* Hardware semaphores */ > > diff --git a/include/fwu.h b/include/fwu.h > > new file mode 100644 > > index 0000000000..e03cfff800 > > --- /dev/null > > +++ b/include/fwu.h > > @@ -0,0 +1,49 @@ > > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > > +/* > > + * Copyright (c) 2022, Linaro Limited > > + */ > > + > > +#if !defined _FWU_H_ > > +#define _FWU_H_ > > + > > +#include <blk.h> > > +#include <efi.h> > > + > > +#include <linux/types.h> > > + > > +struct fwu_mdata; > > +struct udevice; > > + > > +/** > > + * @mdata_check: check the validity of the FWU metadata partitions > > + * @get_mdata() - Get a FWU metadata copy > > + * @update_mdata() - Update the FWU metadata copy > > + */ > > +struct fwu_mdata_ops { > > + 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); > > Needs full comments - see other uclasses Okay > > > +}; > > + > > +#define FWU_MDATA_VERSION 0x1 > > + > > +#define FWU_MDATA_GUID \ > > + EFI_GUID(0x8a7a84a0, 0x8387, 0x40f6, 0xab, 0x41, \ > > + 0xa8, 0xb9, 0xa5, 0xa6, 0x0d, 0x23) > > What is this? Comment? Will add > > > + > > +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); > > These all need full comments Will add > > > +#endif /* _FWU_H_ */ > > diff --git a/include/fwu_mdata.h b/include/fwu_mdata.h > > new file mode 100644 > > index 0000000000..72e3edab43 > > --- /dev/null > > +++ b/include/fwu_mdata.h > > @@ -0,0 +1,67 @@ > > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > > +/* > > + * Copyright (c) 2022, Linaro Limited > > + */ > > + > > +#if !defined _FWU_MDATA_H_ > > +#define _FWU_MDATA_H_ > > + > > +#include <efi.h> > > + > > +/** > > + * struct fwu_image_bank_info - firmware image information > > + * @image_uuid: Guid value of the image in this bank > > + * @accepted: Acceptance status of the image > > + * @reserved: Reserved > > + * > > + * The structure contains image specific fields which are > > + * used to identify the image and to specify the image's > > + * acceptance status > > + */ > > +struct fwu_image_bank_info { > > + efi_guid_t image_uuid; > > + uint32_t accepted; > > + uint32_t reserved; > > +} __attribute__((__packed__)); > > Why is this packed? This was based on a review comment from Masami [1], as he wanted to use the same structure in the low level bootloader as well. > > > + > > +/** > > + * 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__)); > > why packed? Same as above > > > + > > +/** > > + * 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__)); > > same question Same as above -sughosh [1] - https://lists.denx.de/pipermail/u-boot/2021-December/470118.html
pHi Sughosh, On Thu, 18 Aug 2022 at 05:03, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > hi Simon, > > On Thu, 18 Aug 2022 at 06:43, Simon Glass <sjg@chromium.org> wrote: > > > > Hi Sughosh, > > > > On Wed, 17 Aug 2022 at 06:44, 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> > > > --- > > > Changes since V7: > > > * Rephrased the error message in fwu_update_active_index as per > > > suggestion from Ilias. > > > * Reworked the logic in fwu_get_image_alt_num() as per the suggestion > > > from Ilias. > > > > > > drivers/Kconfig | 2 + > > > drivers/Makefile | 1 + > > > drivers/fwu-mdata/Kconfig | 7 + > > > drivers/fwu-mdata/Makefile | 6 + > > > drivers/fwu-mdata/fwu-mdata-uclass.c | 463 +++++++++++++++++++++++++++ > > > include/dm/uclass-id.h | 1 + > > > include/fwu.h | 49 +++ > > > include/fwu_mdata.h | 67 ++++ > > > 8 files changed, 596 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 8b6fead351..75ac149d31 100644 > > > --- a/drivers/Kconfig > > > +++ b/drivers/Kconfig > > > @@ -44,6 +44,8 @@ source "drivers/fuzz/Kconfig" > > > > > > source "drivers/fpga/Kconfig" > > > > > > +source "drivers/fwu-mdata/Kconfig" > > > + > > > source "drivers/gpio/Kconfig" > > > > > > source "drivers/hwspinlock/Kconfig" [..] > > > +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; > > > > Please use an -Exxx value like -EPERM > > I don't think there is any value which relates to a crc mismatch. If > you insist, I can think of -EIO, but I don't think -EPERM is a good > match in this case. See my comment in the other patch. You should not be printing messages in a uclass or driver, except in extreme situations. Choose a value that is somewhat meaningful so you can report the error sensibly in top-level command code. > > > > > > + } > > > + > > > + 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) > > > > Return the active index rather than having an arg? > > I would prefer to keep it the way it is. That allows returning an > error value and keep the active_index separate. Moreover it is just a > single parameter. OK, then can you use active_idxp for the name, as is generally done in driver model? > > > > > > +{ > > > + int ret; > > > + struct fwu_mdata *mdata = NULL; > > > + > > > + ret = fwu_get_mdata(&mdata); > > > > Can you use a local var and avoid the malloc() / free() ? > > Do you see any disadvantages of using space on the heap? If you don't > have a strong opinion on this, I would prefer to keep it as is. Yes, this is not UEFI and we try to avoid allocating memory to no purpose. It takes time and fragments the heap. > > > > > > + 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) > > > > uint > > Are you referring to the function parameter? If so, it is a u32 since > the active_index field in the metadata structure defined in the spec > is a 32 bit value. Sure, but how does that affect the function parameter? Generally we should use natural types in function parameters so we don't do silly things like force a 32-bit arg into a 64-bit register, causing the compiler to have to mask the value, etc. [..] > > > +/** > > > + * 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) > > > > alt_nump is often used with driver model to indicate a return > > value.Again I wonder if the return value could be used? > > Can you point me to an example? I could not find any instance of this. Where did you look? Just to take one example, returning a device: git grep -w devp drivers/ |wc -l 316 > Again, I believe this is a matter of taste, so I would prefer the > current method. Please fit into the uclass / driver model code style. > > > > > > +{ > > > + int ret, i; > > > + efi_guid_t *image_guid; > > > + struct udevice *dev = NULL; > > > + struct fwu_mdata *mdata = NULL; > > > > Please don't set things to NULL for no reason, it cluttlers the code. > > This is being set to NULL in case the called function that is supposed > to populate the metadata structure fails to do so. We can pass NULL to > free, else it could result in freeing up of unrelated memory. If the function fails to set it, then the compiler will complain. I don't understand your comment about NULL. free(NULL) is valid in U-Boot, if that is what you are wondering? > > > > > > + 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); > > > > Rather than this, see the pattern used by other uclasses > > Can you point me to some example code. Also, not sure what the issue > is with using this function. It is a pointless obfuscation, mainly. See for example clk_request(), but most uclasses use this pattern. I really encourage you to read more of the code before submitting a patch. int clk_request(struct udevice *dev, struct clk *clk) { const struct clk_ops *ops; ... ops = clk_dev_ops(dev); (where that is defined in the header file) [..] > > > +/** > > > + * 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__)); > > > > Why is this packed? > > This was based on a review comment from Masami [1], as he wanted to > use the same structure in the low level bootloader as well. It doesn't actually make any sense though. The packed struct is the same as the normal struct, so far as I can tell. What am I missing? It does have a cost for the compiler. For example I tried adding __packed to struct image_header (which has the same issue mentioned by Masami in that it is a file header) and it added 400 bytes to snow. [..] > > -sughosh > > [1] - https://lists.denx.de/pipermail/u-boot/2021-December/470118.html
hi Simon, On Thu, 18 Aug 2022 at 23:20, Simon Glass <sjg@chromium.org> wrote: > > pHi Sughosh, > > On Thu, 18 Aug 2022 at 05:03, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > hi Simon, > > > > On Thu, 18 Aug 2022 at 06:43, Simon Glass <sjg@chromium.org> wrote: > > > > > > Hi Sughosh, > > > > > > On Wed, 17 Aug 2022 at 06:44, 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> > > > > --- > > > > Changes since V7: > > > > * Rephrased the error message in fwu_update_active_index as per > > > > suggestion from Ilias. > > > > * Reworked the logic in fwu_get_image_alt_num() as per the suggestion > > > > from Ilias. > > > > > > > > drivers/Kconfig | 2 + > > > > drivers/Makefile | 1 + > > > > drivers/fwu-mdata/Kconfig | 7 + > > > > drivers/fwu-mdata/Makefile | 6 + > > > > drivers/fwu-mdata/fwu-mdata-uclass.c | 463 +++++++++++++++++++++++++++ > > > > include/dm/uclass-id.h | 1 + > > > > include/fwu.h | 49 +++ > > > > include/fwu_mdata.h | 67 ++++ > > > > 8 files changed, 596 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 8b6fead351..75ac149d31 100644 > > > > --- a/drivers/Kconfig > > > > +++ b/drivers/Kconfig > > > > @@ -44,6 +44,8 @@ source "drivers/fuzz/Kconfig" > > > > > > > > source "drivers/fpga/Kconfig" > > > > > > > > +source "drivers/fwu-mdata/Kconfig" > > > > + > > > > source "drivers/gpio/Kconfig" > > > > > > > > source "drivers/hwspinlock/Kconfig" > > [..] > > > > > +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; > > > > > > Please use an -Exxx value like -EPERM > > > > I don't think there is any value which relates to a crc mismatch. If > > you insist, I can think of -EIO, but I don't think -EPERM is a good > > match in this case. > > See my comment in the other patch. You should not be printing messages in a uclass or driver, except in extreme situations. > > Choose a value that is somewhat meaningful so you can report the error sensibly in top-level command code. > > > > > > > > > > + } > > > > + > > > > + 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) > > > > > > Return the active index rather than having an arg? > > > > I would prefer to keep it the way it is. That allows returning an > > error value and keep the active_index separate. Moreover it is just a > > single parameter. > > OK, then can you use active_idxp for the name, as is generally done in driver model? > > > > > > > > > > +{ > > > > + int ret; > > > > + struct fwu_mdata *mdata = NULL; > > > > + > > > > + ret = fwu_get_mdata(&mdata); > > > > > > Can you use a local var and avoid the malloc() / free() ? > > > > Do you see any disadvantages of using space on the heap? If you don't > > have a strong opinion on this, I would prefer to keep it as is. > > Yes, this is not UEFI and we try to avoid allocating memory to no purpose. It takes time and fragments the heap. > > > > > > > > > > + 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) > > > > > > uint > > > > Are you referring to the function parameter? If so, it is a u32 since > > the active_index field in the metadata structure defined in the spec > > is a 32 bit value. > > Sure, but how does that affect the function parameter? Generally we should use natural types in function parameters so we don't do silly things like force a 32-bit arg into a 64-bit register, causing the compiler to have to mask the value, etc. > > [..] > > > > > +/** > > > > + * 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) > > > > > > alt_nump is often used with driver model to indicate a return > > > value.Again I wonder if the return value could be used? > > > > Can you point me to an example? I could not find any instance of this. > > Where did you look? Just to take one example, returning a device: > > git grep -w devp drivers/ |wc -l > 316 > > > Again, I believe this is a matter of taste, so I would prefer the > > current method. > > Please fit into the uclass / driver model code style. > > > > > > > > > > +{ > > > > + int ret, i; > > > > + efi_guid_t *image_guid; > > > > + struct udevice *dev = NULL; > > > > + struct fwu_mdata *mdata = NULL; > > > > > > Please don't set things to NULL for no reason, it cluttlers the code. > > > > This is being set to NULL in case the called function that is supposed > > to populate the metadata structure fails to do so. We can pass NULL to > > free, else it could result in freeing up of unrelated memory. > > If the function fails to set it, then the compiler will complain. > > I don't understand your comment about NULL. free(NULL) is valid in U-Boot, if that is what you are wondering? > > > > > > > > > > + 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); > > > > > > Rather than this, see the pattern used by other uclasses > > > > Can you point me to some example code. Also, not sure what the issue > > is with using this function. > > It is a pointless obfuscation, mainly. See for example clk_request(), but most uclasses use this pattern. I really encourage you to read more of the code before submitting a patch. > > int clk_request(struct udevice *dev, struct clk *clk) > { > const struct clk_ops *ops; > > ... > ops = clk_dev_ops(dev); > > (where that is defined in the header file) If you mean clk_dev_ops, it is defined in clk-uclass.c, and IMO that makes sense, since it is being called multiple times in the driver. Similarly, the fwu_get_dev_ops is being called multiple times in the uclass driver, so I think it makes sense to have it as a function. If you want me to move it to a header file or rename it, I can do that. -sughosh > > [..] > > > > > +/** > > > > + * 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__)); > > > > > > Why is this packed? > > > > This was based on a review comment from Masami [1], as he wanted to > > use the same structure in the low level bootloader as well. > > It doesn't actually make any sense though. The packed struct is the same as the normal struct, so far as I can tell. What am I missing? > > It does have a cost for the compiler. For example I tried adding __packed to struct image_header (which has the same issue mentioned by Masami in that it is a file header) and it added 400 bytes to snow. > > [..] > > > > > -sughosh > > > > [1] - https://lists.denx.de/pipermail/u-boot/2021-December/470118.html
Hi Simon, On Thu, Aug 18, 2022 at 12:50 PM Simon Glass <sjg@chromium.org> wrote: > > > > +/** > > > > + * 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__)); > > > > > > Why is this packed? > > > > This was based on a review comment from Masami [1], as he wanted to > > use the same structure in the low level bootloader as well. > > It doesn't actually make any sense though. The packed struct is the same as > the normal struct, so far as I can tell. What am I missing? > I think because we want the structure to be read/written onto persistent storage, and possibly manipulated by entities other than uboot. I totally agree with all the rest of your comments on the patchset. cheers!
Hi Sughosh, On Fri, 19 Aug 2022 at 07:36, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > hi Simon, > > On Thu, 18 Aug 2022 at 23:20, Simon Glass <sjg@chromium.org> wrote: > > > > pHi Sughosh, > > > > On Thu, 18 Aug 2022 at 05:03, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > hi Simon, > > > > > > On Thu, 18 Aug 2022 at 06:43, Simon Glass <sjg@chromium.org> wrote: > > > > > > > > Hi Sughosh, > > > > > > > > On Wed, 17 Aug 2022 at 06:44, 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> > > > > > --- > > > > > Changes since V7: > > > > > * Rephrased the error message in fwu_update_active_index as per > > > > > suggestion from Ilias. > > > > > * Reworked the logic in fwu_get_image_alt_num() as per the suggestion > > > > > from Ilias. > > > > > > > > > > drivers/Kconfig | 2 + > > > > > drivers/Makefile | 1 + > > > > > drivers/fwu-mdata/Kconfig | 7 + > > > > > drivers/fwu-mdata/Makefile | 6 + > > > > > drivers/fwu-mdata/fwu-mdata-uclass.c | 463 +++++++++++++++++++++++++++ > > > > > include/dm/uclass-id.h | 1 + > > > > > include/fwu.h | 49 +++ > > > > > include/fwu_mdata.h | 67 ++++ > > > > > 8 files changed, 596 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 > > > > > [..] > > > > I don't understand your comment about NULL. free(NULL) is valid in U-Boot, if that is what you are wondering? > > > > > > > > > > > > > > + 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); > > > > > > > > Rather than this, see the pattern used by other uclasses > > > > > > Can you point me to some example code. Also, not sure what the issue > > > is with using this function. > > > > It is a pointless obfuscation, mainly. See for example clk_request(), but most uclasses use this pattern. I really encourage you to read more of the code before submitting a patch. > > > > int clk_request(struct udevice *dev, struct clk *clk) > > { > > const struct clk_ops *ops; > > > > ... > > ops = clk_dev_ops(dev); > > > > (where that is defined in the header file) > > If you mean clk_dev_ops, it is defined in clk-uclass.c, and IMO that > makes sense, since it is being called multiple times in the driver. > Similarly, the fwu_get_dev_ops is being called multiple times in the > uclass driver, so I think it makes sense to have it as a function. If > you want me to move it to a header file or rename it, I can do that. Yes, rename and move to header, and use the same function signature as clk and the other uclasses (i.e. no dev parameter). Your uclass functions need to be defined with a device parameter as the first arg. They cannot go and hunt down a uclass themselves. See other uclasses for how this works. If you want a common function to find the first device, use uclass_first_device() in the caller, not internal to the uclass! Regards, Simon
Hi Jassi, On Fri, 19 Aug 2022 at 08:59, Jassi Brar <jassisinghbrar@gmail.com> wrote: > > Hi Simon, > > On Thu, Aug 18, 2022 at 12:50 PM Simon Glass <sjg@chromium.org> wrote: > > > > > > +/** > > > > > + * 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__)); > > > > > > > > Why is this packed? > > > > > > This was based on a review comment from Masami [1], as he wanted to > > > use the same structure in the low level bootloader as well. > > > > It doesn't actually make any sense though. The packed struct is the same as > > the normal struct, so far as I can tell. What am I missing? > > > I think because we want the structure to be read/written onto > persistent storage, and possibly manipulated by entities other than > uboot. But specifically, how does __packed help here? What are the other entities doing that changes the format and what is __packed doing to update that? This is actually standard C code. > > I totally agree with all the rest of your comments on the patchset. Regards, Simon
On Fri, Aug 19, 2022 at 10:25 AM Simon Glass <sjg@chromium.org> wrote: > > Hi Jassi, > > On Fri, 19 Aug 2022 at 08:59, Jassi Brar <jassisinghbrar@gmail.com> wrote: > > > > Hi Simon, > > > > On Thu, Aug 18, 2022 at 12:50 PM Simon Glass <sjg@chromium.org> wrote: > > > > > > > > +/** > > > > > > + * 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__)); > > > > > > > > > > Why is this packed? > > > > > > > > This was based on a review comment from Masami [1], as he wanted to > > > > use the same structure in the low level bootloader as well. > > > > > > It doesn't actually make any sense though. The packed struct is the same as > > > the normal struct, so far as I can tell. What am I missing? > > > > > I think because we want the structure to be read/written onto > > persistent storage, and possibly manipulated by entities other than > > uboot. > > But specifically, how does __packed help here? What are the other > entities doing that changes the format and what is __packed doing to > update that? This is actually standard C code. > It is just to make sure there is no padding between structure members, and the size of structure is exactly as in the specification. Though in this case it likely won't make any difference because the members are already 16, 4 and 4 bytes. Cheers.
hi Simon, On Fri, 19 Aug 2022 at 20:53, Simon Glass <sjg@chromium.org> wrote: > > Hi Sughosh, > > On Fri, 19 Aug 2022 at 07:36, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > hi Simon, > > > > On Thu, 18 Aug 2022 at 23:20, Simon Glass <sjg@chromium.org> wrote: > > > > > > pHi Sughosh, > > > > > > On Thu, 18 Aug 2022 at 05:03, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > hi Simon, > > > > > > > > On Thu, 18 Aug 2022 at 06:43, Simon Glass <sjg@chromium.org> wrote: > > > > > > > > > > Hi Sughosh, > > > > > > > > > > On Wed, 17 Aug 2022 at 06:44, 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> > > > > > > --- > > > > > > Changes since V7: > > > > > > * Rephrased the error message in fwu_update_active_index as per > > > > > > suggestion from Ilias. > > > > > > * Reworked the logic in fwu_get_image_alt_num() as per the suggestion > > > > > > from Ilias. > > > > > > > > > > > > drivers/Kconfig | 2 + > > > > > > drivers/Makefile | 1 + > > > > > > drivers/fwu-mdata/Kconfig | 7 + > > > > > > drivers/fwu-mdata/Makefile | 6 + > > > > > > drivers/fwu-mdata/fwu-mdata-uclass.c | 463 +++++++++++++++++++++++++++ > > > > > > include/dm/uclass-id.h | 1 + > > > > > > include/fwu.h | 49 +++ > > > > > > include/fwu_mdata.h | 67 ++++ > > > > > > 8 files changed, 596 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 > > > > > > > > [..] > > > > > > > I don't understand your comment about NULL. free(NULL) is valid in U-Boot, if that is what you are wondering? > > > > > > > > > > > > > > > > > > + 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); > > > > > > > > > > Rather than this, see the pattern used by other uclasses > > > > > > > > Can you point me to some example code. Also, not sure what the issue > > > > is with using this function. > > > > > > It is a pointless obfuscation, mainly. See for example clk_request(), but most uclasses use this pattern. I really encourage you to read more of the code before submitting a patch. > > > > > > int clk_request(struct udevice *dev, struct clk *clk) > > > { > > > const struct clk_ops *ops; > > > > > > ... > > > ops = clk_dev_ops(dev); > > > > > > (where that is defined in the header file) > > > > If you mean clk_dev_ops, it is defined in clk-uclass.c, and IMO that > > makes sense, since it is being called multiple times in the driver. > > Similarly, the fwu_get_dev_ops is being called multiple times in the > > uclass driver, so I think it makes sense to have it as a function. If > > you want me to move it to a header file or rename it, I can do that. > > Yes, rename and move to header, and use the same function signature as > clk and the other uclasses (i.e. no dev parameter). > > Your uclass functions need to be defined with a device parameter as > the first arg. They cannot go and hunt down a uclass themselves. See > other uclasses for how this works. > > If you want a common function to find the first device, use > uclass_first_device() in the caller, not internal to the uclass! I can do that, sure. But there doesn't seem to be uniformity in these rules. There are a bunch of uclass drivers which seem to be doing exactly the same thing, including, but not limited to bootcount, pinctrl, regulators. These uclass drivers all have instances of common API being called and the uclass driver finding the device. Please note that the FWU uclass is not representing a real device -- it is pretty similar to bootcount in that regard. Which was the primary reason I kept the FWU API's device agnostic. -sughosh > > Regards, > Simon
Hi Sughosh, On Fri, 19 Aug 2022 at 10:18, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > hi Simon, > > On Fri, 19 Aug 2022 at 20:53, Simon Glass <sjg@chromium.org> wrote: > > > > Hi Sughosh, > > > > On Fri, 19 Aug 2022 at 07:36, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > hi Simon, > > > > > > On Thu, 18 Aug 2022 at 23:20, Simon Glass <sjg@chromium.org> wrote: > > > > > > > > pHi Sughosh, > > > > > > > > On Thu, 18 Aug 2022 at 05:03, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > > > hi Simon, > > > > > > > > > > On Thu, 18 Aug 2022 at 06:43, Simon Glass <sjg@chromium.org> wrote: > > > > > > > > > > > > Hi Sughosh, > > > > > > > > > > > > On Wed, 17 Aug 2022 at 06:44, 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> > > > > > > > --- > > > > > > > Changes since V7: > > > > > > > * Rephrased the error message in fwu_update_active_index as per > > > > > > > suggestion from Ilias. > > > > > > > * Reworked the logic in fwu_get_image_alt_num() as per the suggestion > > > > > > > from Ilias. > > > > > > > > > > > > > > drivers/Kconfig | 2 + > > > > > > > drivers/Makefile | 1 + > > > > > > > drivers/fwu-mdata/Kconfig | 7 + > > > > > > > drivers/fwu-mdata/Makefile | 6 + > > > > > > > drivers/fwu-mdata/fwu-mdata-uclass.c | 463 +++++++++++++++++++++++++++ > > > > > > > include/dm/uclass-id.h | 1 + > > > > > > > include/fwu.h | 49 +++ > > > > > > > include/fwu_mdata.h | 67 ++++ > > > > > > > 8 files changed, 596 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 > > > > > > > > > > > [..] > > > > > > > > > > I don't understand your comment about NULL. free(NULL) is valid in U-Boot, if that is what you are wondering? > > > > > > > > > > > > > > > > > > > > > > + 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); > > > > > > > > > > > > Rather than this, see the pattern used by other uclasses > > > > > > > > > > Can you point me to some example code. Also, not sure what the issue > > > > > is with using this function. > > > > > > > > It is a pointless obfuscation, mainly. See for example clk_request(), but most uclasses use this pattern. I really encourage you to read more of the code before submitting a patch. > > > > > > > > int clk_request(struct udevice *dev, struct clk *clk) > > > > { > > > > const struct clk_ops *ops; > > > > > > > > ... > > > > ops = clk_dev_ops(dev); > > > > > > > > (where that is defined in the header file) > > > > > > If you mean clk_dev_ops, it is defined in clk-uclass.c, and IMO that > > > makes sense, since it is being called multiple times in the driver. > > > Similarly, the fwu_get_dev_ops is being called multiple times in the > > > uclass driver, so I think it makes sense to have it as a function. If > > > you want me to move it to a header file or rename it, I can do that. > > > > Yes, rename and move to header, and use the same function signature as > > clk and the other uclasses (i.e. no dev parameter). > > > > Your uclass functions need to be defined with a device parameter as > > the first arg. They cannot go and hunt down a uclass themselves. See > > other uclasses for how this works. > > > > If you want a common function to find the first device, use > > uclass_first_device() in the caller, not internal to the uclass! > > I can do that, sure. But there doesn't seem to be uniformity in these > rules. There are a bunch of uclass drivers which seem to be doing > exactly the same thing, including, but not limited to bootcount, > pinctrl, regulators. These uclass drivers all have instances of common pinctrl has pinctrl_select_state() and two GPIO functions that violate this. It should be fixed to store the default pinctrl driver in gd, like we do with serial_dev. Please do send a patch if you can. regulators have some functions which act on all regulators, or at least a selection. That is fine, I think, but let me know if you see somethign wrong. bootcount is not fully converted to driver model and assumes a single bootcount. That should be fixed. in that it should work like sysreset, loading / storing from anything that exists (e.g. the 'walk' functions). > API being called and the uclass driver finding the device. Please note > that the FWU uclass is not representing a real device -- it is pretty > similar to bootcount in that regard. Which was the primary reason I > kept the FWU API's device agnostic. The 'real device' thing doesn't really matter. What is a real device, anyway? Anything we want to model with drivers, a uclass and methods is a device. It is also the primary way we call code from the top-level part of U-Boot. Regards, Simon
diff --git a/drivers/Kconfig b/drivers/Kconfig index 8b6fead351..75ac149d31 100644 --- a/drivers/Kconfig +++ b/drivers/Kconfig @@ -44,6 +44,8 @@ source "drivers/fuzz/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 eba9940231..70bbc2f5e0 100644 --- a/drivers/Makefile +++ b/drivers/Makefile @@ -84,6 +84,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..e53a8c9983 --- /dev/null +++ b/drivers/fwu-mdata/Makefile @@ -0,0 +1,6 @@ +# SPDX-License-Identifier: GPL-2.0-or-later +# +# 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..6bf7fa1b03 --- /dev/null +++ b/drivers/fwu-mdata/fwu-mdata-uclass.c @@ -0,0 +1,463 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (c) 2022, Linaro Limited + */ + +#define LOG_CATEGORY UCLASS_FWU_MDATA + +#include <common.h> +#include <dm.h> +#include <efi_loader.h> +#include <fwu.h> +#include <fwu_mdata.h> +#include <log.h> +#include <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("Invalid active index value\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; + } + + 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_err("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); + } + + goto out; + } + } + + log_err("Partition with the image type %pUs not found\n", + image_type_id); + +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; + 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; + } + + 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(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. + * + * Note: This function is to be called first when modifying any fields + * in the metadata. The sequence of calls to modify any field in the + * metadata would be 1) fwu_get_mdata 2) Modify metadata, followed by + * 3) fwu_update_mdata + * + * Return: 0 if OK, -ve on error + * + */ +int fwu_get_mdata(struct fwu_mdata **mdata) +{ + 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. + * + * Note: This function is not to be called directly to update the + * metadata fields. The sequence of function calls should be + * 1) fwu_get_mdata() 2) Modify the medata fields 3) fwu_update_mdata() + * + * Return: 0 if OK, -ve on error + * + */ +int fwu_update_mdata(struct 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 a432e43871..598a8c10a0 100644 --- a/include/dm/uclass-id.h +++ b/include/dm/uclass-id.h @@ -58,6 +58,7 @@ enum uclass_id { UCLASS_FIRMWARE, /* Firmware */ UCLASS_FUZZING_ENGINE, /* Fuzzing engine */ UCLASS_FS_FIRMWARE_LOADER, /* Generic loader */ + UCLASS_FWU_MDATA, /* FWU Metadata Access */ UCLASS_GPIO, /* Bank of general-purpose I/O pins */ UCLASS_HASH, /* Hash device */ UCLASS_HWSPINLOCK, /* Hardware semaphores */ diff --git a/include/fwu.h b/include/fwu.h new file mode 100644 index 0000000000..e03cfff800 --- /dev/null +++ b/include/fwu.h @@ -0,0 +1,49 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* + * Copyright (c) 2022, Linaro Limited + */ + +#if !defined _FWU_H_ +#define _FWU_H_ + +#include <blk.h> +#include <efi.h> + +#include <linux/types.h> + +struct fwu_mdata; +struct udevice; + +/** + * @mdata_check: check the validity of the FWU metadata partitions + * @get_mdata() - Get a FWU metadata copy + * @update_mdata() - Update the FWU metadata copy + */ +struct fwu_mdata_ops { + 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..72e3edab43 --- /dev/null +++ b/include/fwu_mdata.h @@ -0,0 +1,67 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* + * Copyright (c) 2022, Linaro Limited + */ + +#if !defined _FWU_MDATA_H_ +#define _FWU_MDATA_H_ + +#include <efi.h> + +/** + * struct fwu_image_bank_info - firmware image information + * @image_uuid: Guid value of the image in this bank + * @accepted: Acceptance status of the image + * @reserved: Reserved + * + * The structure contains image specific fields which are + * used to identify the image and to specify the image's + * acceptance status + */ +struct fwu_image_bank_info { + efi_guid_t image_uuid; + uint32_t accepted; + uint32_t reserved; +} __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_ */