| Message ID | 20211219070605.14894-2-sughosh.ganu@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | FWU: Add support for FWU Multi Bank Update feature | expand |
Hi Sughosh, 2021年12月19日(日) 16:06 Sughosh Ganu <sughosh.ganu@linaro.org>: > > 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 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> > --- > Changes since V1: > * Move all function declarations to a separate header fwu.h > * Rename metadata with mdata for all symbols > * Drop the parameter in the function fwu_revert_boot_index > as suggested by Etienne > > include/fwu.h | 28 +++++ > include/fwu_mdata.h | 102 ++++++++++++++++ > lib/fwu_updates/fwu_mdata.c | 236 ++++++++++++++++++++++++++++++++++++ > 3 files changed, 366 insertions(+) > create mode 100644 include/fwu.h > create mode 100644 include/fwu_mdata.h > create mode 100644 lib/fwu_updates/fwu_mdata.c > > diff --git a/include/fwu.h b/include/fwu.h > new file mode 100644 > index 0000000000..e6bc3e6b73 > --- /dev/null > +++ b/include/fwu.h > @@ -0,0 +1,28 @@ > +/* SPDX-License-Identifier: GPL-2.0+ */ > +/* > + * Copyright (c) 2021, Linaro Limited > + */ > + > +#if !defined _FWU_H_ > +#define _FWU_H_ > + > +#include <efi.h> > + > +#include <linux/types.h> > + > +#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_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); > +int fwu_clear_accept_image(efi_guid_t *img_type_id, u32 bank); > + > +#endif /* _FWU_H_ */ > diff --git a/include/fwu_mdata.h b/include/fwu_mdata.h > new file mode 100644 > index 0000000000..11eb570012 > --- /dev/null > +++ b/include/fwu_mdata.h > @@ -0,0 +1,102 @@ > +/* SPDX-License-Identifier: GPL-2.0+ */ > +/* > + * Copyright (c) 2021, Linaro Limited > + */ > + > +#if !defined _FWU_MDATA_H_ > +#define _FWU_MDATA_H_ > + > +#include <efi.h> > +#include <uuid.h> > + > +#include <linux/types.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; > + u32 accepted; > + u32 reserved; > +}; Could you explicitly pack these metadata data structures? Since these metadata will be shared among bootloaders (TF-A, SCP-firmware), it is better to ensure that those are packed and fixed offset for each field. > + > +/** > + * 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]; > +}; > + > +/** > + * 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 { > + u32 crc32; > + u32 version; > + u32 active_index; > + u32 previous_active_index; > + > + struct fwu_image_entry img_entry[CONFIG_FWU_NUM_IMAGES_PER_BANK]; > +}; > + > +/** > + * @get_active_index: get the current active_index value > + * @update_active_index: update the active_index value > + * @get_image_alt_num: get the alt number to be used for the image > + * @mdata_check: check the validity of the FWU metadata partitions > + * @revert_boot_index: set the active_index to previous_active_index > + * @set_accept_image: set the accepted bit for the image > + * @clear_accept_image: clear the accepted bit for the image > + * @get_mdata() - Get a FWU metadata copy > + */ > +struct fwu_mdata_ops { > + int (*get_active_index)(u32 *active_idx); > + > + int (*update_active_index)(u32 active_idx); > + > + int (*get_image_alt_num)(efi_guid_t image_type_id, u32 update_bank, > + int *alt_num); > + > + int (*mdata_check)(void); > + > + int (*revert_boot_index)(void); > + > + int (*set_accept_image)(efi_guid_t *img_type_id); > + > + int (*clear_accept_image)(efi_guid_t *img_type_id, u32 bank); > + > + int (*get_mdata)(struct fwu_mdata **mdata); > +}; > + > +int fwu_get_mdata(struct fwu_mdata **mdata); > +struct fwu_mdata_ops *get_plat_fwu_mdata_ops(void); Also, could you also move these function prototypes and operators to fwu? I would like to copy this header file to the boot loaders as is. In that case, I just need the definition of the metadata data structure, but no function prototypes. Thank you, > + > +#endif /* _FWU_MDATA_H_ */ > diff --git a/lib/fwu_updates/fwu_mdata.c b/lib/fwu_updates/fwu_mdata.c > new file mode 100644 > index 0000000000..348e9c85f7 > --- /dev/null > +++ b/lib/fwu_updates/fwu_mdata.c > @@ -0,0 +1,236 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Copyright (c) 2021, Linaro Limited > + */ > + > +#include <fwu_mdata.h> > + > +#include <linux/errno.h> > +#include <linux/types.h> > + > +static struct fwu_mdata_ops *get_fwu_mdata_ops(void) > +{ > + struct fwu_mdata_ops *ops; > + > + ops = get_plat_fwu_mdata_ops(); > + if (!ops) { > + log_err("Unable to get fwu ops\n"); > + return NULL; > + } > + > + return ops; > +} > + > +/** > + * 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) > +{ > + struct fwu_mdata_ops *ops; > + > + ops = get_fwu_mdata_ops(); > + if (!ops) > + return -EPROTONOSUPPORT; > + > + if (!ops->get_active_index) { > + log_err("get_active_index() method not defined for the platform\n"); > + return -ENOSYS; > + } > + > + return ops->get_active_index(active_idx); > +} > + > +/** > + * 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) > +{ > + struct fwu_mdata_ops *ops; > + > + ops = get_fwu_mdata_ops(); > + if (!ops) > + return -EPROTONOSUPPORT; > + > + if (!ops->update_active_index) { > + log_err("update_active_index() method not defined for the platform\n"); > + return -ENOSYS; > + } > + > + return ops->update_active_index(active_idx); > +} > + > +/** > + * fwu_get_image_alt_num() - Get the dfu alt number to be used for capsule update > + * @image_type_id: 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) > +{ > + struct fwu_mdata_ops *ops; > + > + ops = get_fwu_mdata_ops(); > + if (!ops) > + return -EPROTONOSUPPORT; > + > + if (!ops->get_image_alt_num) { > + log_err("get_image_alt_num() method not defined for the platform\n"); > + return -ENOSYS; > + } > + > + return ops->get_image_alt_num(image_type_id, update_bank, alt_num); > +} > + > +/** > + * 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) > +{ > + struct fwu_mdata_ops *ops; > + > + ops = get_fwu_mdata_ops(); > + if (!ops) > + return -EPROTONOSUPPORT; > + > + if (!ops->mdata_check) { > + log_err("mdata_check() method not defined for the platform\n"); > + return -ENOSYS; > + } > + > + return ops->mdata_check(); > +} > + > +/** > + * 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) > +{ > + struct fwu_mdata_ops *ops; > + > + ops = get_fwu_mdata_ops(); > + if (!ops) > + return -EPROTONOSUPPORT; > + > + if (!ops->revert_boot_index) { > + log_err("revert_boot_index() method not defined for the platform\n"); > + return -ENOSYS; > + } > + > + return ops->revert_boot_index(); > +} > + > +/** > + * 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 > + * > + * 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) > +{ > + struct fwu_mdata_ops *ops; > + > + ops = get_fwu_mdata_ops(); > + if (!ops) > + return -EPROTONOSUPPORT; > + > + if (!ops->set_accept_image) { > + log_err("set_accept_image() method not defined for the platform\n"); > + return -ENOSYS; > + } > + > + return ops->set_accept_image(img_type_id); > +} > + > +/** > + * 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 > + * > + * 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) > +{ > + struct fwu_mdata_ops *ops; > + > + ops = get_fwu_mdata_ops(); > + if (!ops) > + return -EPROTONOSUPPORT; > + > + if (!ops->clear_accept_image) { > + log_err("clear_accept_image() method not defined for the platform\n"); > + return -ENOSYS; > + } > + > + return ops->clear_accept_image(img_type_id, bank); > +} > + > +/** > + * fwu_get_mdata() - Get a FWU metadata copy > + * @mdata: Copy of the FWU metadata > + * > + * Get a valid copy of the FWU metadata. > + * > + * Return: 0 if OK, -ve on error > + * > + */ > +int fwu_get_mdata(struct fwu_mdata **mdata) > +{ > + struct fwu_mdata_ops *ops; > + > + ops = get_fwu_mdata_ops(); > + if (!ops) > + return -EPROTONOSUPPORT; > + > + if (!ops->get_mdata) { > + log_err("get_mdata() method not defined for the platform\n"); > + return -ENOSYS; > + } > + > + return ops->get_mdata(mdata); > +} > -- > 2.17.1 >
hi Masami, On Tue, 21 Dec 2021 at 05:40, Masami Hiramatsu <masami.hiramatsu@linaro.org> wrote: > Hi Sughosh, > > 2021年12月19日(日) 16:06 Sughosh Ganu <sughosh.ganu@linaro.org>: > > > > 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 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> > > --- > > Changes since V1: > > * Move all function declarations to a separate header fwu.h > > * Rename metadata with mdata for all symbols > > * Drop the parameter in the function fwu_revert_boot_index > > as suggested by Etienne > > > > include/fwu.h | 28 +++++ > > include/fwu_mdata.h | 102 ++++++++++++++++ > > lib/fwu_updates/fwu_mdata.c | 236 ++++++++++++++++++++++++++++++++++++ > > 3 files changed, 366 insertions(+) > > create mode 100644 include/fwu.h > > create mode 100644 include/fwu_mdata.h > > create mode 100644 lib/fwu_updates/fwu_mdata.c > > > > diff --git a/include/fwu.h b/include/fwu.h > > new file mode 100644 > > index 0000000000..e6bc3e6b73 > > --- /dev/null > > +++ b/include/fwu.h > > @@ -0,0 +1,28 @@ > > +/* SPDX-License-Identifier: GPL-2.0+ */ > > +/* > > + * Copyright (c) 2021, Linaro Limited > > + */ > > + > > +#if !defined _FWU_H_ > > +#define _FWU_H_ > > + > > +#include <efi.h> > > + > > +#include <linux/types.h> > > + > > +#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_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); > > +int fwu_clear_accept_image(efi_guid_t *img_type_id, u32 bank); > > + > > +#endif /* _FWU_H_ */ > > diff --git a/include/fwu_mdata.h b/include/fwu_mdata.h > > new file mode 100644 > > index 0000000000..11eb570012 > > --- /dev/null > > +++ b/include/fwu_mdata.h > > @@ -0,0 +1,102 @@ > > +/* SPDX-License-Identifier: GPL-2.0+ */ > > +/* > > + * Copyright (c) 2021, Linaro Limited > > + */ > > + > > +#if !defined _FWU_MDATA_H_ > > +#define _FWU_MDATA_H_ > > + > > +#include <efi.h> > > +#include <uuid.h> > > + > > +#include <linux/types.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; > > + u32 accepted; > > + u32 reserved; > > +}; > > Could you explicitly pack these metadata data structures? > Since these metadata will be shared among bootloaders (TF-A, SCP-firmware), > it is better to ensure that those are packed and fixed offset for each > field. > Okay. WIll do it. > > > + > > +/** > > + * 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]; > > +}; > > + > > +/** > > + * 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 { > > + u32 crc32; > > + u32 version; > > + u32 active_index; > > + u32 previous_active_index; > > + > > + struct fwu_image_entry img_entry[CONFIG_FWU_NUM_IMAGES_PER_BANK]; > > +}; > > + > > +/** > > + * @get_active_index: get the current active_index value > > + * @update_active_index: update the active_index value > > + * @get_image_alt_num: get the alt number to be used for the image > > + * @mdata_check: check the validity of the FWU metadata partitions > > + * @revert_boot_index: set the active_index to previous_active_index > > + * @set_accept_image: set the accepted bit for the image > > + * @clear_accept_image: clear the accepted bit for the image > > + * @get_mdata() - Get a FWU metadata copy > > + */ > > +struct fwu_mdata_ops { > > + int (*get_active_index)(u32 *active_idx); > > + > > + int (*update_active_index)(u32 active_idx); > > + > > + int (*get_image_alt_num)(efi_guid_t image_type_id, u32 > update_bank, > > + int *alt_num); > > + > > + int (*mdata_check)(void); > > + > > + int (*revert_boot_index)(void); > > + > > + int (*set_accept_image)(efi_guid_t *img_type_id); > > + > > + int (*clear_accept_image)(efi_guid_t *img_type_id, u32 bank); > > + > > + int (*get_mdata)(struct fwu_mdata **mdata); > > +}; > > + > > +int fwu_get_mdata(struct fwu_mdata **mdata); > > +struct fwu_mdata_ops *get_plat_fwu_mdata_ops(void); > > Also, could you also move these function prototypes and operators to > fwu? I would like to copy this header file to the boot loaders as is. > In that case, I just need the definition of the metadata data structure, > but no function prototypes. > I have moved the function declarations to fwu.h. The one function prototype that I have in fwu_mdata.h is the one which has the metadata structure as one of it's parameters. Same for the ops as well. The get_mdata function has the metadata structure as its parameter. Are you facing any issues with the current structure of the files. -sughosh > Thank you, > > > + > > +#endif /* _FWU_MDATA_H_ */ > > diff --git a/lib/fwu_updates/fwu_mdata.c b/lib/fwu_updates/fwu_mdata.c > > new file mode 100644 > > index 0000000000..348e9c85f7 > > --- /dev/null > > +++ b/lib/fwu_updates/fwu_mdata.c > > @@ -0,0 +1,236 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * Copyright (c) 2021, Linaro Limited > > + */ > > + > > +#include <fwu_mdata.h> > > + > > +#include <linux/errno.h> > > +#include <linux/types.h> > > + > > +static struct fwu_mdata_ops *get_fwu_mdata_ops(void) > > +{ > > + struct fwu_mdata_ops *ops; > > + > > + ops = get_plat_fwu_mdata_ops(); > > + if (!ops) { > > + log_err("Unable to get fwu ops\n"); > > + return NULL; > > + } > > + > > + return ops; > > +} > > + > > +/** > > + * 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) > > +{ > > + struct fwu_mdata_ops *ops; > > + > > + ops = get_fwu_mdata_ops(); > > + if (!ops) > > + return -EPROTONOSUPPORT; > > + > > + if (!ops->get_active_index) { > > + log_err("get_active_index() method not defined for the > platform\n"); > > + return -ENOSYS; > > + } > > + > > + return ops->get_active_index(active_idx); > > +} > > + > > +/** > > + * 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) > > +{ > > + struct fwu_mdata_ops *ops; > > + > > + ops = get_fwu_mdata_ops(); > > + if (!ops) > > + return -EPROTONOSUPPORT; > > + > > + if (!ops->update_active_index) { > > + log_err("update_active_index() method not defined for > the platform\n"); > > + return -ENOSYS; > > + } > > + > > + return ops->update_active_index(active_idx); > > +} > > + > > +/** > > + * fwu_get_image_alt_num() - Get the dfu alt number to be used for > capsule update > > + * @image_type_id: 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) > > +{ > > + struct fwu_mdata_ops *ops; > > + > > + ops = get_fwu_mdata_ops(); > > + if (!ops) > > + return -EPROTONOSUPPORT; > > + > > + if (!ops->get_image_alt_num) { > > + log_err("get_image_alt_num() method not defined for the > platform\n"); > > + return -ENOSYS; > > + } > > + > > + return ops->get_image_alt_num(image_type_id, update_bank, > alt_num); > > +} > > + > > +/** > > + * 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) > > +{ > > + struct fwu_mdata_ops *ops; > > + > > + ops = get_fwu_mdata_ops(); > > + if (!ops) > > + return -EPROTONOSUPPORT; > > + > > + if (!ops->mdata_check) { > > + log_err("mdata_check() method not defined for the > platform\n"); > > + return -ENOSYS; > > + } > > + > > + return ops->mdata_check(); > > +} > > + > > +/** > > + * 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) > > +{ > > + struct fwu_mdata_ops *ops; > > + > > + ops = get_fwu_mdata_ops(); > > + if (!ops) > > + return -EPROTONOSUPPORT; > > + > > + if (!ops->revert_boot_index) { > > + log_err("revert_boot_index() method not defined for the > platform\n"); > > + return -ENOSYS; > > + } > > + > > + return ops->revert_boot_index(); > > +} > > + > > +/** > > + * 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 > > + * > > + * 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) > > +{ > > + struct fwu_mdata_ops *ops; > > + > > + ops = get_fwu_mdata_ops(); > > + if (!ops) > > + return -EPROTONOSUPPORT; > > + > > + if (!ops->set_accept_image) { > > + log_err("set_accept_image() method not defined for the > platform\n"); > > + return -ENOSYS; > > + } > > + > > + return ops->set_accept_image(img_type_id); > > +} > > + > > +/** > > + * 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 > > + * > > + * 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) > > +{ > > + struct fwu_mdata_ops *ops; > > + > > + ops = get_fwu_mdata_ops(); > > + if (!ops) > > + return -EPROTONOSUPPORT; > > + > > + if (!ops->clear_accept_image) { > > + log_err("clear_accept_image() method not defined for the > platform\n"); > > + return -ENOSYS; > > + } > > + > > + return ops->clear_accept_image(img_type_id, bank); > > +} > > + > > +/** > > + * fwu_get_mdata() - Get a FWU metadata copy > > + * @mdata: Copy of the FWU metadata > > + * > > + * Get a valid copy of the FWU metadata. > > + * > > + * Return: 0 if OK, -ve on error > > + * > > + */ > > +int fwu_get_mdata(struct fwu_mdata **mdata) > > +{ > > + struct fwu_mdata_ops *ops; > > + > > + ops = get_fwu_mdata_ops(); > > + if (!ops) > > + return -EPROTONOSUPPORT; > > + > > + if (!ops->get_mdata) { > > + log_err("get_mdata() method not defined for the > platform\n"); > > + return -ENOSYS; > > + } > > + > > + return ops->get_mdata(mdata); > > +} > > -- > > 2.17.1 > > > > > -- > Masami Hiramatsu >
On Tue, Dec 21, 2021 at 10:05:57AM +0530, Sughosh Ganu wrote: > hi Masami, > > On Tue, 21 Dec 2021 at 05:40, Masami Hiramatsu <masami.hiramatsu@linaro.org> > wrote: > > > Hi Sughosh, > > > > 2021年12月19日(日) 16:06 Sughosh Ganu <sughosh.ganu@linaro.org>: > > > > > > 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 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> > > > --- > > > Changes since V1: > > > * Move all function declarations to a separate header fwu.h > > > * Rename metadata with mdata for all symbols > > > * Drop the parameter in the function fwu_revert_boot_index > > > as suggested by Etienne > > > > > > include/fwu.h | 28 +++++ > > > include/fwu_mdata.h | 102 ++++++++++++++++ > > > lib/fwu_updates/fwu_mdata.c | 236 ++++++++++++++++++++++++++++++++++++ > > > 3 files changed, 366 insertions(+) > > > create mode 100644 include/fwu.h > > > create mode 100644 include/fwu_mdata.h > > > create mode 100644 lib/fwu_updates/fwu_mdata.c > > > > > > diff --git a/include/fwu.h b/include/fwu.h > > > new file mode 100644 > > > index 0000000000..e6bc3e6b73 > > > --- /dev/null > > > +++ b/include/fwu.h > > > @@ -0,0 +1,28 @@ > > > +/* SPDX-License-Identifier: GPL-2.0+ */ > > > +/* > > > + * Copyright (c) 2021, Linaro Limited > > > + */ > > > + > > > +#if !defined _FWU_H_ > > > +#define _FWU_H_ > > > + > > > +#include <efi.h> > > > + > > > +#include <linux/types.h> > > > + > > > +#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_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); > > > +int fwu_clear_accept_image(efi_guid_t *img_type_id, u32 bank); > > > + > > > +#endif /* _FWU_H_ */ > > > diff --git a/include/fwu_mdata.h b/include/fwu_mdata.h > > > new file mode 100644 > > > index 0000000000..11eb570012 > > > --- /dev/null > > > +++ b/include/fwu_mdata.h > > > @@ -0,0 +1,102 @@ > > > +/* SPDX-License-Identifier: GPL-2.0+ */ > > > +/* > > > + * Copyright (c) 2021, Linaro Limited > > > + */ > > > + > > > +#if !defined _FWU_MDATA_H_ > > > +#define _FWU_MDATA_H_ > > > + > > > +#include <efi.h> > > > +#include <uuid.h> > > > + > > > +#include <linux/types.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; > > > + u32 accepted; > > > + u32 reserved; > > > +}; > > > > Could you explicitly pack these metadata data structures? > > Since these metadata will be shared among bootloaders (TF-A, SCP-firmware), > > it is better to ensure that those are packed and fixed offset for each > > field. > > > > Okay. WIll do it. > > > > > > > + > > > +/** > > > + * 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]; > > > +}; > > > + > > > +/** > > > + * 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 { > > > + u32 crc32; > > > + u32 version; > > > + u32 active_index; > > > + u32 previous_active_index; > > > + > > > + struct fwu_image_entry img_entry[CONFIG_FWU_NUM_IMAGES_PER_BANK]; > > > +}; > > > + > > > +/** > > > + * @get_active_index: get the current active_index value > > > + * @update_active_index: update the active_index value > > > + * @get_image_alt_num: get the alt number to be used for the image > > > + * @mdata_check: check the validity of the FWU metadata partitions > > > + * @revert_boot_index: set the active_index to previous_active_index > > > + * @set_accept_image: set the accepted bit for the image > > > + * @clear_accept_image: clear the accepted bit for the image > > > + * @get_mdata() - Get a FWU metadata copy > > > + */ > > > +struct fwu_mdata_ops { > > > + int (*get_active_index)(u32 *active_idx); > > > + > > > + int (*update_active_index)(u32 active_idx); > > > + > > > + int (*get_image_alt_num)(efi_guid_t image_type_id, u32 > > update_bank, > > > + int *alt_num); > > > + > > > + int (*mdata_check)(void); > > > + > > > + int (*revert_boot_index)(void); > > > + > > > + int (*set_accept_image)(efi_guid_t *img_type_id); > > > + > > > + int (*clear_accept_image)(efi_guid_t *img_type_id, u32 bank); > > > + > > > + int (*get_mdata)(struct fwu_mdata **mdata); > > > +}; > > > + > > > +int fwu_get_mdata(struct fwu_mdata **mdata); > > > +struct fwu_mdata_ops *get_plat_fwu_mdata_ops(void); > > > > Also, could you also move these function prototypes and operators to > > fwu? I would like to copy this header file to the boot loaders as is. > > In that case, I just need the definition of the metadata data structure, > > but no function prototypes. > > > > I have moved the function declarations to fwu.h. The one function prototype > that I have in fwu_mdata.h is the one which has the metadata structure as > one of it's parameters. Same for the ops as well. The get_mdata function > has the metadata structure as its parameter. Are you facing any issues with > the current structure of the files. struct fwu_mdata; int fwu_get_mdata(struct fwu_**mdata); would be good enough? I'm not sure whether it is a good idea that we re-use the *same* file in other software, but I would suggest you to use standard POSIX types like 'uint32_t' instead of 'u32' anyway. -Takahiro Akashi > -sughosh > > > > Thank you, > > > > > + > > > +#endif /* _FWU_MDATA_H_ */ > > > diff --git a/lib/fwu_updates/fwu_mdata.c b/lib/fwu_updates/fwu_mdata.c > > > new file mode 100644 > > > index 0000000000..348e9c85f7 > > > --- /dev/null > > > +++ b/lib/fwu_updates/fwu_mdata.c > > > @@ -0,0 +1,236 @@ > > > +// SPDX-License-Identifier: GPL-2.0+ > > > +/* > > > + * Copyright (c) 2021, Linaro Limited > > > + */ > > > + > > > +#include <fwu_mdata.h> > > > + > > > +#include <linux/errno.h> > > > +#include <linux/types.h> > > > + > > > +static struct fwu_mdata_ops *get_fwu_mdata_ops(void) > > > +{ > > > + struct fwu_mdata_ops *ops; > > > + > > > + ops = get_plat_fwu_mdata_ops(); > > > + if (!ops) { > > > + log_err("Unable to get fwu ops\n"); > > > + return NULL; > > > + } > > > + > > > + return ops; > > > +} > > > + > > > +/** > > > + * 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) > > > +{ > > > + struct fwu_mdata_ops *ops; > > > + > > > + ops = get_fwu_mdata_ops(); > > > + if (!ops) > > > + return -EPROTONOSUPPORT; > > > + > > > + if (!ops->get_active_index) { > > > + log_err("get_active_index() method not defined for the > > platform\n"); > > > + return -ENOSYS; > > > + } > > > + > > > + return ops->get_active_index(active_idx); > > > +} > > > + > > > +/** > > > + * 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) > > > +{ > > > + struct fwu_mdata_ops *ops; > > > + > > > + ops = get_fwu_mdata_ops(); > > > + if (!ops) > > > + return -EPROTONOSUPPORT; > > > + > > > + if (!ops->update_active_index) { > > > + log_err("update_active_index() method not defined for > > the platform\n"); > > > + return -ENOSYS; > > > + } > > > + > > > + return ops->update_active_index(active_idx); > > > +} > > > + > > > +/** > > > + * fwu_get_image_alt_num() - Get the dfu alt number to be used for > > capsule update > > > + * @image_type_id: 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) > > > +{ > > > + struct fwu_mdata_ops *ops; > > > + > > > + ops = get_fwu_mdata_ops(); > > > + if (!ops) > > > + return -EPROTONOSUPPORT; > > > + > > > + if (!ops->get_image_alt_num) { > > > + log_err("get_image_alt_num() method not defined for the > > platform\n"); > > > + return -ENOSYS; > > > + } > > > + > > > + return ops->get_image_alt_num(image_type_id, update_bank, > > alt_num); > > > +} > > > + > > > +/** > > > + * 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) > > > +{ > > > + struct fwu_mdata_ops *ops; > > > + > > > + ops = get_fwu_mdata_ops(); > > > + if (!ops) > > > + return -EPROTONOSUPPORT; > > > + > > > + if (!ops->mdata_check) { > > > + log_err("mdata_check() method not defined for the > > platform\n"); > > > + return -ENOSYS; > > > + } > > > + > > > + return ops->mdata_check(); > > > +} > > > + > > > +/** > > > + * 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) > > > +{ > > > + struct fwu_mdata_ops *ops; > > > + > > > + ops = get_fwu_mdata_ops(); > > > + if (!ops) > > > + return -EPROTONOSUPPORT; > > > + > > > + if (!ops->revert_boot_index) { > > > + log_err("revert_boot_index() method not defined for the > > platform\n"); > > > + return -ENOSYS; > > > + } > > > + > > > + return ops->revert_boot_index(); > > > +} > > > + > > > +/** > > > + * 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 > > > + * > > > + * 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) > > > +{ > > > + struct fwu_mdata_ops *ops; > > > + > > > + ops = get_fwu_mdata_ops(); > > > + if (!ops) > > > + return -EPROTONOSUPPORT; > > > + > > > + if (!ops->set_accept_image) { > > > + log_err("set_accept_image() method not defined for the > > platform\n"); > > > + return -ENOSYS; > > > + } > > > + > > > + return ops->set_accept_image(img_type_id); > > > +} > > > + > > > +/** > > > + * 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 > > > + * > > > + * 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) > > > +{ > > > + struct fwu_mdata_ops *ops; > > > + > > > + ops = get_fwu_mdata_ops(); > > > + if (!ops) > > > + return -EPROTONOSUPPORT; > > > + > > > + if (!ops->clear_accept_image) { > > > + log_err("clear_accept_image() method not defined for the > > platform\n"); > > > + return -ENOSYS; > > > + } > > > + > > > + return ops->clear_accept_image(img_type_id, bank); > > > +} > > > + > > > +/** > > > + * fwu_get_mdata() - Get a FWU metadata copy > > > + * @mdata: Copy of the FWU metadata > > > + * > > > + * Get a valid copy of the FWU metadata. > > > + * > > > + * Return: 0 if OK, -ve on error > > > + * > > > + */ > > > +int fwu_get_mdata(struct fwu_mdata **mdata) > > > +{ > > > + struct fwu_mdata_ops *ops; > > > + > > > + ops = get_fwu_mdata_ops(); > > > + if (!ops) > > > + return -EPROTONOSUPPORT; > > > + > > > + if (!ops->get_mdata) { > > > + log_err("get_mdata() method not defined for the > > platform\n"); > > > + return -ENOSYS; > > > + } > > > + > > > + return ops->get_mdata(mdata); > > > +} > > > -- > > > 2.17.1 > > > > > > > > > -- > > Masami Hiramatsu > >
On Tue, 21 Dec 2021 at 10:33, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > On Tue, Dec 21, 2021 at 10:05:57AM +0530, Sughosh Ganu wrote: > > hi Masami, > > > > On Tue, 21 Dec 2021 at 05:40, Masami Hiramatsu < > masami.hiramatsu@linaro.org> > > wrote: > > > > > Hi Sughosh, > > > > > > 2021年12月19日(日) 16:06 Sughosh Ganu <sughosh.ganu@linaro.org>: > > > > > > > > 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 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> > > > > --- > > > > Changes since V1: > > > > * Move all function declarations to a separate header fwu.h > > > > * Rename metadata with mdata for all symbols > > > > * Drop the parameter in the function fwu_revert_boot_index > > > > as suggested by Etienne > > > > > > > > include/fwu.h | 28 +++++ > > > > include/fwu_mdata.h | 102 ++++++++++++++++ > > > > lib/fwu_updates/fwu_mdata.c | 236 > ++++++++++++++++++++++++++++++++++++ > > > > 3 files changed, 366 insertions(+) > > > > create mode 100644 include/fwu.h > > > > create mode 100644 include/fwu_mdata.h > > > > create mode 100644 lib/fwu_updates/fwu_mdata.c > > > > > > > > diff --git a/include/fwu.h b/include/fwu.h > > > > new file mode 100644 > > > > index 0000000000..e6bc3e6b73 > > > > --- /dev/null > > > > +++ b/include/fwu.h > > > > @@ -0,0 +1,28 @@ > > > > +/* SPDX-License-Identifier: GPL-2.0+ */ > > > > +/* > > > > + * Copyright (c) 2021, Linaro Limited > > > > + */ > > > > + > > > > +#if !defined _FWU_H_ > > > > +#define _FWU_H_ > > > > + > > > > +#include <efi.h> > > > > + > > > > +#include <linux/types.h> > > > > + > > > > +#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_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); > > > > +int fwu_clear_accept_image(efi_guid_t *img_type_id, u32 bank); > > > > + > > > > +#endif /* _FWU_H_ */ > > > > diff --git a/include/fwu_mdata.h b/include/fwu_mdata.h > > > > new file mode 100644 > > > > index 0000000000..11eb570012 > > > > --- /dev/null > > > > +++ b/include/fwu_mdata.h > > > > @@ -0,0 +1,102 @@ > > > > +/* SPDX-License-Identifier: GPL-2.0+ */ > > > > +/* > > > > + * Copyright (c) 2021, Linaro Limited > > > > + */ > > > > + > > > > +#if !defined _FWU_MDATA_H_ > > > > +#define _FWU_MDATA_H_ > > > > + > > > > +#include <efi.h> > > > > +#include <uuid.h> > > > > + > > > > +#include <linux/types.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; > > > > + u32 accepted; > > > > + u32 reserved; > > > > +}; > > > > > > Could you explicitly pack these metadata data structures? > > > Since these metadata will be shared among bootloaders (TF-A, > SCP-firmware), > > > it is better to ensure that those are packed and fixed offset for each > > > field. > > > > > > > Okay. WIll do it. > > > > > > > > > > > + > > > > +/** > > > > + * 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]; > > > > +}; > > > > + > > > > +/** > > > > + * 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 { > > > > + u32 crc32; > > > > + u32 version; > > > > + u32 active_index; > > > > + u32 previous_active_index; > > > > + > > > > + struct fwu_image_entry > img_entry[CONFIG_FWU_NUM_IMAGES_PER_BANK]; > > > > +}; > > > > + > > > > +/** > > > > + * @get_active_index: get the current active_index value > > > > + * @update_active_index: update the active_index value > > > > + * @get_image_alt_num: get the alt number to be used for the image > > > > + * @mdata_check: check the validity of the FWU metadata partitions > > > > + * @revert_boot_index: set the active_index to previous_active_index > > > > + * @set_accept_image: set the accepted bit for the image > > > > + * @clear_accept_image: clear the accepted bit for the image > > > > + * @get_mdata() - Get a FWU metadata copy > > > > + */ > > > > +struct fwu_mdata_ops { > > > > + int (*get_active_index)(u32 *active_idx); > > > > + > > > > + int (*update_active_index)(u32 active_idx); > > > > + > > > > + int (*get_image_alt_num)(efi_guid_t image_type_id, u32 > > > update_bank, > > > > + int *alt_num); > > > > + > > > > + int (*mdata_check)(void); > > > > + > > > > + int (*revert_boot_index)(void); > > > > + > > > > + int (*set_accept_image)(efi_guid_t *img_type_id); > > > > + > > > > + int (*clear_accept_image)(efi_guid_t *img_type_id, u32 bank); > > > > + > > > > + int (*get_mdata)(struct fwu_mdata **mdata); > > > > +}; > > > > + > > > > +int fwu_get_mdata(struct fwu_mdata **mdata); > > > > +struct fwu_mdata_ops *get_plat_fwu_mdata_ops(void); > > > > > > Also, could you also move these function prototypes and operators to > > > fwu? I would like to copy this header file to the boot loaders as is. > > > In that case, I just need the definition of the metadata data > structure, > > > but no function prototypes. > > > > > > > I have moved the function declarations to fwu.h. The one function > prototype > > that I have in fwu_mdata.h is the one which has the metadata structure as > > one of it's parameters. Same for the ops as well. The get_mdata function > > has the metadata structure as its parameter. Are you facing any issues > with > > the current structure of the files. > > struct fwu_mdata; > int fwu_get_mdata(struct fwu_**mdata); > > would be good enough? > Yes, I had considered putting this kind of declaration in fwu.h. But I decided against it thinking that any function declaration that involved the fwu metadata structure can be put in one file. But I just want to understand if there is some issue in putting these declarations in the fwu_mdata.h. The rest of the function declarations and symbols are put in the fwu.h header. If you see, I only include the fwu.h in the efi_firmware.c and efi_capsule.c. > > I'm not sure whether it is a good idea that we re-use the *same* file > in other software, but I would suggest you to use standard POSIX types > like 'uint32_t' instead of 'u32' anyway. > Hmm, this is on the same lines to the types used in the capsule code. That was the primary reason that I stuck with u<size> types. Any issue with using the short type? The Linux kernel seems to be using it. -sughosh > > -Takahiro Akashi > > > > -sughosh > > > > > > > Thank you, > > > > > > > + > > > > +#endif /* _FWU_MDATA_H_ */ > > > > diff --git a/lib/fwu_updates/fwu_mdata.c > b/lib/fwu_updates/fwu_mdata.c > > > > new file mode 100644 > > > > index 0000000000..348e9c85f7 > > > > --- /dev/null > > > > +++ b/lib/fwu_updates/fwu_mdata.c > > > > @@ -0,0 +1,236 @@ > > > > +// SPDX-License-Identifier: GPL-2.0+ > > > > +/* > > > > + * Copyright (c) 2021, Linaro Limited > > > > + */ > > > > + > > > > +#include <fwu_mdata.h> > > > > + > > > > +#include <linux/errno.h> > > > > +#include <linux/types.h> > > > > + > > > > +static struct fwu_mdata_ops *get_fwu_mdata_ops(void) > > > > +{ > > > > + struct fwu_mdata_ops *ops; > > > > + > > > > + ops = get_plat_fwu_mdata_ops(); > > > > + if (!ops) { > > > > + log_err("Unable to get fwu ops\n"); > > > > + return NULL; > > > > + } > > > > + > > > > + return ops; > > > > +} > > > > + > > > > +/** > > > > + * 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) > > > > +{ > > > > + struct fwu_mdata_ops *ops; > > > > + > > > > + ops = get_fwu_mdata_ops(); > > > > + if (!ops) > > > > + return -EPROTONOSUPPORT; > > > > + > > > > + if (!ops->get_active_index) { > > > > + log_err("get_active_index() method not defined for > the > > > platform\n"); > > > > + return -ENOSYS; > > > > + } > > > > + > > > > + return ops->get_active_index(active_idx); > > > > +} > > > > + > > > > +/** > > > > + * 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) > > > > +{ > > > > + struct fwu_mdata_ops *ops; > > > > + > > > > + ops = get_fwu_mdata_ops(); > > > > + if (!ops) > > > > + return -EPROTONOSUPPORT; > > > > + > > > > + if (!ops->update_active_index) { > > > > + log_err("update_active_index() method not defined for > > > the platform\n"); > > > > + return -ENOSYS; > > > > + } > > > > + > > > > + return ops->update_active_index(active_idx); > > > > +} > > > > + > > > > +/** > > > > + * fwu_get_image_alt_num() - Get the dfu alt number to be used for > > > capsule update > > > > + * @image_type_id: 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) > > > > +{ > > > > + struct fwu_mdata_ops *ops; > > > > + > > > > + ops = get_fwu_mdata_ops(); > > > > + if (!ops) > > > > + return -EPROTONOSUPPORT; > > > > + > > > > + if (!ops->get_image_alt_num) { > > > > + log_err("get_image_alt_num() method not defined for > the > > > platform\n"); > > > > + return -ENOSYS; > > > > + } > > > > + > > > > + return ops->get_image_alt_num(image_type_id, update_bank, > > > alt_num); > > > > +} > > > > + > > > > +/** > > > > + * 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) > > > > +{ > > > > + struct fwu_mdata_ops *ops; > > > > + > > > > + ops = get_fwu_mdata_ops(); > > > > + if (!ops) > > > > + return -EPROTONOSUPPORT; > > > > + > > > > + if (!ops->mdata_check) { > > > > + log_err("mdata_check() method not defined for the > > > platform\n"); > > > > + return -ENOSYS; > > > > + } > > > > + > > > > + return ops->mdata_check(); > > > > +} > > > > + > > > > +/** > > > > + * 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) > > > > +{ > > > > + struct fwu_mdata_ops *ops; > > > > + > > > > + ops = get_fwu_mdata_ops(); > > > > + if (!ops) > > > > + return -EPROTONOSUPPORT; > > > > + > > > > + if (!ops->revert_boot_index) { > > > > + log_err("revert_boot_index() method not defined for > the > > > platform\n"); > > > > + return -ENOSYS; > > > > + } > > > > + > > > > + return ops->revert_boot_index(); > > > > +} > > > > + > > > > +/** > > > > + * 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 > > > > + * > > > > + * 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) > > > > +{ > > > > + struct fwu_mdata_ops *ops; > > > > + > > > > + ops = get_fwu_mdata_ops(); > > > > + if (!ops) > > > > + return -EPROTONOSUPPORT; > > > > + > > > > + if (!ops->set_accept_image) { > > > > + log_err("set_accept_image() method not defined for > the > > > platform\n"); > > > > + return -ENOSYS; > > > > + } > > > > + > > > > + return ops->set_accept_image(img_type_id); > > > > +} > > > > + > > > > +/** > > > > + * 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 > > > > + * > > > > + * 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) > > > > +{ > > > > + struct fwu_mdata_ops *ops; > > > > + > > > > + ops = get_fwu_mdata_ops(); > > > > + if (!ops) > > > > + return -EPROTONOSUPPORT; > > > > + > > > > + if (!ops->clear_accept_image) { > > > > + log_err("clear_accept_image() method not defined for > the > > > platform\n"); > > > > + return -ENOSYS; > > > > + } > > > > + > > > > + return ops->clear_accept_image(img_type_id, bank); > > > > +} > > > > + > > > > +/** > > > > + * fwu_get_mdata() - Get a FWU metadata copy > > > > + * @mdata: Copy of the FWU metadata > > > > + * > > > > + * Get a valid copy of the FWU metadata. > > > > + * > > > > + * Return: 0 if OK, -ve on error > > > > + * > > > > + */ > > > > +int fwu_get_mdata(struct fwu_mdata **mdata) > > > > +{ > > > > + struct fwu_mdata_ops *ops; > > > > + > > > > + ops = get_fwu_mdata_ops(); > > > > + if (!ops) > > > > + return -EPROTONOSUPPORT; > > > > + > > > > + if (!ops->get_mdata) { > > > > + log_err("get_mdata() method not defined for the > > > platform\n"); > > > > + return -ENOSYS; > > > > + } > > > > + > > > > + return ops->get_mdata(mdata); > > > > +} > > > > -- > > > > 2.17.1 > > > > > > > > > > > > > -- > > > Masami Hiramatsu > > > >
Hi Sughosh, 2021年12月21日(火) 14:47 Sughosh Ganu <sughosh.ganu@linaro.org>: > > > On Tue, 21 Dec 2021 at 10:33, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: >> >> On Tue, Dec 21, 2021 at 10:05:57AM +0530, Sughosh Ganu wrote: >> > hi Masami, >> > >> > On Tue, 21 Dec 2021 at 05:40, Masami Hiramatsu <masami.hiramatsu@linaro.org> >> > wrote: >> > >> > > Hi Sughosh, >> > > >> > > 2021年12月19日(日) 16:06 Sughosh Ganu <sughosh.ganu@linaro.org>: >> > > > >> > > > 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 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> >> > > > --- >> > > > Changes since V1: >> > > > * Move all function declarations to a separate header fwu.h >> > > > * Rename metadata with mdata for all symbols >> > > > * Drop the parameter in the function fwu_revert_boot_index >> > > > as suggested by Etienne >> > > > >> > > > include/fwu.h | 28 +++++ >> > > > include/fwu_mdata.h | 102 ++++++++++++++++ >> > > > lib/fwu_updates/fwu_mdata.c | 236 ++++++++++++++++++++++++++++++++++++ >> > > > 3 files changed, 366 insertions(+) >> > > > create mode 100644 include/fwu.h >> > > > create mode 100644 include/fwu_mdata.h >> > > > create mode 100644 lib/fwu_updates/fwu_mdata.c >> > > > >> > > > diff --git a/include/fwu.h b/include/fwu.h >> > > > new file mode 100644 >> > > > index 0000000000..e6bc3e6b73 >> > > > --- /dev/null >> > > > +++ b/include/fwu.h >> > > > @@ -0,0 +1,28 @@ >> > > > +/* SPDX-License-Identifier: GPL-2.0+ */ >> > > > +/* >> > > > + * Copyright (c) 2021, Linaro Limited >> > > > + */ >> > > > + >> > > > +#if !defined _FWU_H_ >> > > > +#define _FWU_H_ >> > > > + >> > > > +#include <efi.h> >> > > > + >> > > > +#include <linux/types.h> >> > > > + >> > > > +#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_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); >> > > > +int fwu_clear_accept_image(efi_guid_t *img_type_id, u32 bank); >> > > > + >> > > > +#endif /* _FWU_H_ */ >> > > > diff --git a/include/fwu_mdata.h b/include/fwu_mdata.h >> > > > new file mode 100644 >> > > > index 0000000000..11eb570012 >> > > > --- /dev/null >> > > > +++ b/include/fwu_mdata.h >> > > > @@ -0,0 +1,102 @@ >> > > > +/* SPDX-License-Identifier: GPL-2.0+ */ >> > > > +/* >> > > > + * Copyright (c) 2021, Linaro Limited >> > > > + */ >> > > > + >> > > > +#if !defined _FWU_MDATA_H_ >> > > > +#define _FWU_MDATA_H_ >> > > > + >> > > > +#include <efi.h> >> > > > +#include <uuid.h> >> > > > + >> > > > +#include <linux/types.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; >> > > > + u32 accepted; >> > > > + u32 reserved; >> > > > +}; >> > > >> > > Could you explicitly pack these metadata data structures? >> > > Since these metadata will be shared among bootloaders (TF-A, SCP-firmware), >> > > it is better to ensure that those are packed and fixed offset for each >> > > field. >> > > >> > >> > Okay. WIll do it. >> > >> > >> > > >> > > > + >> > > > +/** >> > > > + * 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]; >> > > > +}; >> > > > + >> > > > +/** >> > > > + * 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 { >> > > > + u32 crc32; >> > > > + u32 version; >> > > > + u32 active_index; >> > > > + u32 previous_active_index; >> > > > + >> > > > + struct fwu_image_entry img_entry[CONFIG_FWU_NUM_IMAGES_PER_BANK]; >> > > > +}; >> > > > + >> > > > +/** >> > > > + * @get_active_index: get the current active_index value >> > > > + * @update_active_index: update the active_index value >> > > > + * @get_image_alt_num: get the alt number to be used for the image >> > > > + * @mdata_check: check the validity of the FWU metadata partitions >> > > > + * @revert_boot_index: set the active_index to previous_active_index >> > > > + * @set_accept_image: set the accepted bit for the image >> > > > + * @clear_accept_image: clear the accepted bit for the image >> > > > + * @get_mdata() - Get a FWU metadata copy >> > > > + */ >> > > > +struct fwu_mdata_ops { >> > > > + int (*get_active_index)(u32 *active_idx); >> > > > + >> > > > + int (*update_active_index)(u32 active_idx); >> > > > + >> > > > + int (*get_image_alt_num)(efi_guid_t image_type_id, u32 >> > > update_bank, >> > > > + int *alt_num); >> > > > + >> > > > + int (*mdata_check)(void); >> > > > + >> > > > + int (*revert_boot_index)(void); >> > > > + >> > > > + int (*set_accept_image)(efi_guid_t *img_type_id); >> > > > + >> > > > + int (*clear_accept_image)(efi_guid_t *img_type_id, u32 bank); >> > > > + >> > > > + int (*get_mdata)(struct fwu_mdata **mdata); >> > > > +}; >> > > > + >> > > > +int fwu_get_mdata(struct fwu_mdata **mdata); >> > > > +struct fwu_mdata_ops *get_plat_fwu_mdata_ops(void); >> > > >> > > Also, could you also move these function prototypes and operators to >> > > fwu? I would like to copy this header file to the boot loaders as is. >> > > In that case, I just need the definition of the metadata data structure, >> > > but no function prototypes. >> > > >> > >> > I have moved the function declarations to fwu.h. The one function prototype >> > that I have in fwu_mdata.h is the one which has the metadata structure as >> > one of it's parameters. Same for the ops as well. The get_mdata function >> > has the metadata structure as its parameter. Are you facing any issues with >> > the current structure of the files. The structure is OK (if it is packed). But the prototypes, interfaces are no needed because the metadata access method is not needed for the bootloader, like TF-A and SCP-firmware. Of course I can write it again for those firmwares, as far as it's coding style suits to the project, I would like to reuse it. If any update applied, I can apply the change to the other firmware too. >> >> struct fwu_mdata; >> int fwu_get_mdata(struct fwu_**mdata); >> >> would be good enough? > > > Yes, I had considered putting this kind of declaration in fwu.h. But I decided against it thinking that any function declaration that involved the fwu metadata structure can be put in one file. But I just want to understand if there is some issue in putting these declarations in the fwu_mdata.h. The rest of the function declarations and symbols are put in the fwu.h header. If you see, I only include the fwu.h in the efi_firmware.c and efi_capsule.c. I just want to share the metadata definition among firmware projects. In that case, the metadata is on the storage media, but the operation will be re-implemented for each firmware. I think it is better to split it. :) Thank you, > >> >> >> I'm not sure whether it is a good idea that we re-use the *same* file >> in other software, but I would suggest you to use standard POSIX types >> like 'uint32_t' instead of 'u32' anyway. > > > Hmm, this is on the same lines to the types used in the capsule code. That was the primary reason that I stuck with u<size> types. Any issue with using the short type? The Linux kernel seems to be using it. > > -sughosh > >> >> >> -Takahiro Akashi >> >> >> > -sughosh >> > >> > >> > > Thank you, >> > > >> > > > + >> > > > +#endif /* _FWU_MDATA_H_ */ >> > > > diff --git a/lib/fwu_updates/fwu_mdata.c b/lib/fwu_updates/fwu_mdata.c >> > > > new file mode 100644 >> > > > index 0000000000..348e9c85f7 >> > > > --- /dev/null >> > > > +++ b/lib/fwu_updates/fwu_mdata.c >> > > > @@ -0,0 +1,236 @@ >> > > > +// SPDX-License-Identifier: GPL-2.0+ >> > > > +/* >> > > > + * Copyright (c) 2021, Linaro Limited >> > > > + */ >> > > > + >> > > > +#include <fwu_mdata.h> >> > > > + >> > > > +#include <linux/errno.h> >> > > > +#include <linux/types.h> >> > > > + >> > > > +static struct fwu_mdata_ops *get_fwu_mdata_ops(void) >> > > > +{ >> > > > + struct fwu_mdata_ops *ops; >> > > > + >> > > > + ops = get_plat_fwu_mdata_ops(); >> > > > + if (!ops) { >> > > > + log_err("Unable to get fwu ops\n"); >> > > > + return NULL; >> > > > + } >> > > > + >> > > > + return ops; >> > > > +} >> > > > + >> > > > +/** >> > > > + * 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) >> > > > +{ >> > > > + struct fwu_mdata_ops *ops; >> > > > + >> > > > + ops = get_fwu_mdata_ops(); >> > > > + if (!ops) >> > > > + return -EPROTONOSUPPORT; >> > > > + >> > > > + if (!ops->get_active_index) { >> > > > + log_err("get_active_index() method not defined for the >> > > platform\n"); >> > > > + return -ENOSYS; >> > > > + } >> > > > + >> > > > + return ops->get_active_index(active_idx); >> > > > +} >> > > > + >> > > > +/** >> > > > + * 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) >> > > > +{ >> > > > + struct fwu_mdata_ops *ops; >> > > > + >> > > > + ops = get_fwu_mdata_ops(); >> > > > + if (!ops) >> > > > + return -EPROTONOSUPPORT; >> > > > + >> > > > + if (!ops->update_active_index) { >> > > > + log_err("update_active_index() method not defined for >> > > the platform\n"); >> > > > + return -ENOSYS; >> > > > + } >> > > > + >> > > > + return ops->update_active_index(active_idx); >> > > > +} >> > > > + >> > > > +/** >> > > > + * fwu_get_image_alt_num() - Get the dfu alt number to be used for >> > > capsule update >> > > > + * @image_type_id: 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) >> > > > +{ >> > > > + struct fwu_mdata_ops *ops; >> > > > + >> > > > + ops = get_fwu_mdata_ops(); >> > > > + if (!ops) >> > > > + return -EPROTONOSUPPORT; >> > > > + >> > > > + if (!ops->get_image_alt_num) { >> > > > + log_err("get_image_alt_num() method not defined for the >> > > platform\n"); >> > > > + return -ENOSYS; >> > > > + } >> > > > + >> > > > + return ops->get_image_alt_num(image_type_id, update_bank, >> > > alt_num); >> > > > +} >> > > > + >> > > > +/** >> > > > + * 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) >> > > > +{ >> > > > + struct fwu_mdata_ops *ops; >> > > > + >> > > > + ops = get_fwu_mdata_ops(); >> > > > + if (!ops) >> > > > + return -EPROTONOSUPPORT; >> > > > + >> > > > + if (!ops->mdata_check) { >> > > > + log_err("mdata_check() method not defined for the >> > > platform\n"); >> > > > + return -ENOSYS; >> > > > + } >> > > > + >> > > > + return ops->mdata_check(); >> > > > +} >> > > > + >> > > > +/** >> > > > + * 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) >> > > > +{ >> > > > + struct fwu_mdata_ops *ops; >> > > > + >> > > > + ops = get_fwu_mdata_ops(); >> > > > + if (!ops) >> > > > + return -EPROTONOSUPPORT; >> > > > + >> > > > + if (!ops->revert_boot_index) { >> > > > + log_err("revert_boot_index() method not defined for the >> > > platform\n"); >> > > > + return -ENOSYS; >> > > > + } >> > > > + >> > > > + return ops->revert_boot_index(); >> > > > +} >> > > > + >> > > > +/** >> > > > + * 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 >> > > > + * >> > > > + * 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) >> > > > +{ >> > > > + struct fwu_mdata_ops *ops; >> > > > + >> > > > + ops = get_fwu_mdata_ops(); >> > > > + if (!ops) >> > > > + return -EPROTONOSUPPORT; >> > > > + >> > > > + if (!ops->set_accept_image) { >> > > > + log_err("set_accept_image() method not defined for the >> > > platform\n"); >> > > > + return -ENOSYS; >> > > > + } >> > > > + >> > > > + return ops->set_accept_image(img_type_id); >> > > > +} >> > > > + >> > > > +/** >> > > > + * 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 >> > > > + * >> > > > + * 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) >> > > > +{ >> > > > + struct fwu_mdata_ops *ops; >> > > > + >> > > > + ops = get_fwu_mdata_ops(); >> > > > + if (!ops) >> > > > + return -EPROTONOSUPPORT; >> > > > + >> > > > + if (!ops->clear_accept_image) { >> > > > + log_err("clear_accept_image() method not defined for the >> > > platform\n"); >> > > > + return -ENOSYS; >> > > > + } >> > > > + >> > > > + return ops->clear_accept_image(img_type_id, bank); >> > > > +} >> > > > + >> > > > +/** >> > > > + * fwu_get_mdata() - Get a FWU metadata copy >> > > > + * @mdata: Copy of the FWU metadata >> > > > + * >> > > > + * Get a valid copy of the FWU metadata. >> > > > + * >> > > > + * Return: 0 if OK, -ve on error >> > > > + * >> > > > + */ >> > > > +int fwu_get_mdata(struct fwu_mdata **mdata) >> > > > +{ >> > > > + struct fwu_mdata_ops *ops; >> > > > + >> > > > + ops = get_fwu_mdata_ops(); >> > > > + if (!ops) >> > > > + return -EPROTONOSUPPORT; >> > > > + >> > > > + if (!ops->get_mdata) { >> > > > + log_err("get_mdata() method not defined for the >> > > platform\n"); >> > > > + return -ENOSYS; >> > > > + } >> > > > + >> > > > + return ops->get_mdata(mdata); >> > > > +} >> > > > -- >> > > > 2.17.1 >> > > > >> > > >> > > >> > > -- >> > > Masami Hiramatsu >> > >
hi Masami, On Tue, 21 Dec 2021 at 11:31, Masami Hiramatsu <masami.hiramatsu@linaro.org> wrote: > Hi Sughosh, > > > 2021年12月21日(火) 14:47 Sughosh Ganu <sughosh.ganu@linaro.org>: > > > > > > On Tue, 21 Dec 2021 at 10:33, AKASHI Takahiro < > takahiro.akashi@linaro.org> wrote: > >> > >> On Tue, Dec 21, 2021 at 10:05:57AM +0530, Sughosh Ganu wrote: > >> > hi Masami, > >> > > >> > On Tue, 21 Dec 2021 at 05:40, Masami Hiramatsu < > masami.hiramatsu@linaro.org> > >> > wrote: > >> > > >> > > Hi Sughosh, > >> > > > >> > > 2021年12月19日(日) 16:06 Sughosh Ganu <sughosh.ganu@linaro.org>: > >> > > > > >> > > > 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 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> > >> > > > --- > >> > > > Changes since V1: > >> > > > * Move all function declarations to a separate header fwu.h > >> > > > * Rename metadata with mdata for all symbols > >> > > > * Drop the parameter in the function fwu_revert_boot_index > >> > > > as suggested by Etienne > >> > > > > >> > > > include/fwu.h | 28 +++++ > >> > > > include/fwu_mdata.h | 102 ++++++++++++++++ > >> > > > lib/fwu_updates/fwu_mdata.c | 236 > ++++++++++++++++++++++++++++++++++++ > >> > > > 3 files changed, 366 insertions(+) > >> > > > create mode 100644 include/fwu.h > >> > > > create mode 100644 include/fwu_mdata.h > >> > > > create mode 100644 lib/fwu_updates/fwu_mdata.c > >> > > > > >> > > > diff --git a/include/fwu.h b/include/fwu.h > >> > > > new file mode 100644 > >> > > > index 0000000000..e6bc3e6b73 > >> > > > --- /dev/null > >> > > > +++ b/include/fwu.h > >> > > > @@ -0,0 +1,28 @@ > >> > > > +/* SPDX-License-Identifier: GPL-2.0+ */ > >> > > > +/* > >> > > > + * Copyright (c) 2021, Linaro Limited > >> > > > + */ > >> > > > + > >> > > > +#if !defined _FWU_H_ > >> > > > +#define _FWU_H_ > >> > > > + > >> > > > +#include <efi.h> > >> > > > + > >> > > > +#include <linux/types.h> > >> > > > + > >> > > > +#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_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); > >> > > > +int fwu_clear_accept_image(efi_guid_t *img_type_id, u32 bank); > >> > > > + > >> > > > +#endif /* _FWU_H_ */ > >> > > > diff --git a/include/fwu_mdata.h b/include/fwu_mdata.h > >> > > > new file mode 100644 > >> > > > index 0000000000..11eb570012 > >> > > > --- /dev/null > >> > > > +++ b/include/fwu_mdata.h > >> > > > @@ -0,0 +1,102 @@ > >> > > > +/* SPDX-License-Identifier: GPL-2.0+ */ > >> > > > +/* > >> > > > + * Copyright (c) 2021, Linaro Limited > >> > > > + */ > >> > > > + > >> > > > +#if !defined _FWU_MDATA_H_ > >> > > > +#define _FWU_MDATA_H_ > >> > > > + > >> > > > +#include <efi.h> > >> > > > +#include <uuid.h> > >> > > > + > >> > > > +#include <linux/types.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; > >> > > > + u32 accepted; > >> > > > + u32 reserved; > >> > > > +}; > >> > > > >> > > Could you explicitly pack these metadata data structures? > >> > > Since these metadata will be shared among bootloaders (TF-A, > SCP-firmware), > >> > > it is better to ensure that those are packed and fixed offset for > each > >> > > field. > >> > > > >> > > >> > Okay. WIll do it. > >> > > >> > > >> > > > >> > > > + > >> > > > +/** > >> > > > + * 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]; > >> > > > +}; > >> > > > + > >> > > > +/** > >> > > > + * 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 { > >> > > > + u32 crc32; > >> > > > + u32 version; > >> > > > + u32 active_index; > >> > > > + u32 previous_active_index; > >> > > > + > >> > > > + struct fwu_image_entry > img_entry[CONFIG_FWU_NUM_IMAGES_PER_BANK]; > >> > > > +}; > >> > > > + > >> > > > +/** > >> > > > + * @get_active_index: get the current active_index value > >> > > > + * @update_active_index: update the active_index value > >> > > > + * @get_image_alt_num: get the alt number to be used for the > image > >> > > > + * @mdata_check: check the validity of the FWU metadata > partitions > >> > > > + * @revert_boot_index: set the active_index to > previous_active_index > >> > > > + * @set_accept_image: set the accepted bit for the image > >> > > > + * @clear_accept_image: clear the accepted bit for the image > >> > > > + * @get_mdata() - Get a FWU metadata copy > >> > > > + */ > >> > > > +struct fwu_mdata_ops { > >> > > > + int (*get_active_index)(u32 *active_idx); > >> > > > + > >> > > > + int (*update_active_index)(u32 active_idx); > >> > > > + > >> > > > + int (*get_image_alt_num)(efi_guid_t image_type_id, u32 > >> > > update_bank, > >> > > > + int *alt_num); > >> > > > + > >> > > > + int (*mdata_check)(void); > >> > > > + > >> > > > + int (*revert_boot_index)(void); > >> > > > + > >> > > > + int (*set_accept_image)(efi_guid_t *img_type_id); > >> > > > + > >> > > > + int (*clear_accept_image)(efi_guid_t *img_type_id, u32 > bank); > >> > > > + > >> > > > + int (*get_mdata)(struct fwu_mdata **mdata); > >> > > > +}; > >> > > > + > >> > > > +int fwu_get_mdata(struct fwu_mdata **mdata); > >> > > > +struct fwu_mdata_ops *get_plat_fwu_mdata_ops(void); > >> > > > >> > > Also, could you also move these function prototypes and operators to > >> > > fwu? I would like to copy this header file to the boot loaders as > is. > >> > > In that case, I just need the definition of the metadata data > structure, > >> > > but no function prototypes. > >> > > > >> > > >> > I have moved the function declarations to fwu.h. The one function > prototype > >> > that I have in fwu_mdata.h is the one which has the metadata > structure as > >> > one of it's parameters. Same for the ops as well. The get_mdata > function > >> > has the metadata structure as its parameter. Are you facing any > issues with > >> > the current structure of the files. > > The structure is OK (if it is packed). But the prototypes, interfaces > are no needed > because the metadata access method is not needed for the bootloader, like > TF-A and SCP-firmware. > Of course I can write it again for those firmwares, as far as it's > coding style suits > to the project, I would like to reuse it. If any update applied, I can > apply the > change to the other firmware too. > Okay, I now get why you want the metadata structure in a separate file :). That makes sense. I will remove all other stuff apart from the structure definitions in my next version. Thanks for your explanation. -sughosh > > >> > >> struct fwu_mdata; > >> int fwu_get_mdata(struct fwu_**mdata); > >> > >> would be good enough? > > > > > > Yes, I had considered putting this kind of declaration in fwu.h. But I > decided against it thinking that any function declaration that involved the > fwu metadata structure can be put in one file. But I just want to > understand if there is some issue in putting these declarations in the > fwu_mdata.h. The rest of the function declarations and symbols are put in > the fwu.h header. If you see, I only include the fwu.h in the > efi_firmware.c and efi_capsule.c. > > I just want to share the metadata definition among firmware projects. > In that case, the metadata is on the storage media, but the operation > will be re-implemented for each firmware. I think it is better to > split it. :) > > Thank you, > > > > >> > >> > >> I'm not sure whether it is a good idea that we re-use the *same* file > >> in other software, but I would suggest you to use standard POSIX types > >> like 'uint32_t' instead of 'u32' anyway. > > > > > > Hmm, this is on the same lines to the types used in the capsule code. > That was the primary reason that I stuck with u<size> types. Any issue with > using the short type? The Linux kernel seems to be using it. > > > > -sughosh > > > >> > >> > >> -Takahiro Akashi > >> > >> > >> > -sughosh > >> > > >> > > >> > > Thank you, > >> > > > >> > > > + > >> > > > +#endif /* _FWU_MDATA_H_ */ > >> > > > diff --git a/lib/fwu_updates/fwu_mdata.c > b/lib/fwu_updates/fwu_mdata.c > >> > > > new file mode 100644 > >> > > > index 0000000000..348e9c85f7 > >> > > > --- /dev/null > >> > > > +++ b/lib/fwu_updates/fwu_mdata.c > >> > > > @@ -0,0 +1,236 @@ > >> > > > +// SPDX-License-Identifier: GPL-2.0+ > >> > > > +/* > >> > > > + * Copyright (c) 2021, Linaro Limited > >> > > > + */ > >> > > > + > >> > > > +#include <fwu_mdata.h> > >> > > > + > >> > > > +#include <linux/errno.h> > >> > > > +#include <linux/types.h> > >> > > > + > >> > > > +static struct fwu_mdata_ops *get_fwu_mdata_ops(void) > >> > > > +{ > >> > > > + struct fwu_mdata_ops *ops; > >> > > > + > >> > > > + ops = get_plat_fwu_mdata_ops(); > >> > > > + if (!ops) { > >> > > > + log_err("Unable to get fwu ops\n"); > >> > > > + return NULL; > >> > > > + } > >> > > > + > >> > > > + return ops; > >> > > > +} > >> > > > + > >> > > > +/** > >> > > > + * 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) > >> > > > +{ > >> > > > + struct fwu_mdata_ops *ops; > >> > > > + > >> > > > + ops = get_fwu_mdata_ops(); > >> > > > + if (!ops) > >> > > > + return -EPROTONOSUPPORT; > >> > > > + > >> > > > + if (!ops->get_active_index) { > >> > > > + log_err("get_active_index() method not defined > for the > >> > > platform\n"); > >> > > > + return -ENOSYS; > >> > > > + } > >> > > > + > >> > > > + return ops->get_active_index(active_idx); > >> > > > +} > >> > > > + > >> > > > +/** > >> > > > + * 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) > >> > > > +{ > >> > > > + struct fwu_mdata_ops *ops; > >> > > > + > >> > > > + ops = get_fwu_mdata_ops(); > >> > > > + if (!ops) > >> > > > + return -EPROTONOSUPPORT; > >> > > > + > >> > > > + if (!ops->update_active_index) { > >> > > > + log_err("update_active_index() method not defined > for > >> > > the platform\n"); > >> > > > + return -ENOSYS; > >> > > > + } > >> > > > + > >> > > > + return ops->update_active_index(active_idx); > >> > > > +} > >> > > > + > >> > > > +/** > >> > > > + * fwu_get_image_alt_num() - Get the dfu alt number to be used > for > >> > > capsule update > >> > > > + * @image_type_id: 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) > >> > > > +{ > >> > > > + struct fwu_mdata_ops *ops; > >> > > > + > >> > > > + ops = get_fwu_mdata_ops(); > >> > > > + if (!ops) > >> > > > + return -EPROTONOSUPPORT; > >> > > > + > >> > > > + if (!ops->get_image_alt_num) { > >> > > > + log_err("get_image_alt_num() method not defined > for the > >> > > platform\n"); > >> > > > + return -ENOSYS; > >> > > > + } > >> > > > + > >> > > > + return ops->get_image_alt_num(image_type_id, update_bank, > >> > > alt_num); > >> > > > +} > >> > > > + > >> > > > +/** > >> > > > + * 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) > >> > > > +{ > >> > > > + struct fwu_mdata_ops *ops; > >> > > > + > >> > > > + ops = get_fwu_mdata_ops(); > >> > > > + if (!ops) > >> > > > + return -EPROTONOSUPPORT; > >> > > > + > >> > > > + if (!ops->mdata_check) { > >> > > > + log_err("mdata_check() method not defined for the > >> > > platform\n"); > >> > > > + return -ENOSYS; > >> > > > + } > >> > > > + > >> > > > + return ops->mdata_check(); > >> > > > +} > >> > > > + > >> > > > +/** > >> > > > + * 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) > >> > > > +{ > >> > > > + struct fwu_mdata_ops *ops; > >> > > > + > >> > > > + ops = get_fwu_mdata_ops(); > >> > > > + if (!ops) > >> > > > + return -EPROTONOSUPPORT; > >> > > > + > >> > > > + if (!ops->revert_boot_index) { > >> > > > + log_err("revert_boot_index() method not defined > for the > >> > > platform\n"); > >> > > > + return -ENOSYS; > >> > > > + } > >> > > > + > >> > > > + return ops->revert_boot_index(); > >> > > > +} > >> > > > + > >> > > > +/** > >> > > > + * 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 > >> > > > + * > >> > > > + * 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) > >> > > > +{ > >> > > > + struct fwu_mdata_ops *ops; > >> > > > + > >> > > > + ops = get_fwu_mdata_ops(); > >> > > > + if (!ops) > >> > > > + return -EPROTONOSUPPORT; > >> > > > + > >> > > > + if (!ops->set_accept_image) { > >> > > > + log_err("set_accept_image() method not defined > for the > >> > > platform\n"); > >> > > > + return -ENOSYS; > >> > > > + } > >> > > > + > >> > > > + return ops->set_accept_image(img_type_id); > >> > > > +} > >> > > > + > >> > > > +/** > >> > > > + * 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 > >> > > > + * > >> > > > + * 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) > >> > > > +{ > >> > > > + struct fwu_mdata_ops *ops; > >> > > > + > >> > > > + ops = get_fwu_mdata_ops(); > >> > > > + if (!ops) > >> > > > + return -EPROTONOSUPPORT; > >> > > > + > >> > > > + if (!ops->clear_accept_image) { > >> > > > + log_err("clear_accept_image() method not defined > for the > >> > > platform\n"); > >> > > > + return -ENOSYS; > >> > > > + } > >> > > > + > >> > > > + return ops->clear_accept_image(img_type_id, bank); > >> > > > +} > >> > > > + > >> > > > +/** > >> > > > + * fwu_get_mdata() - Get a FWU metadata copy > >> > > > + * @mdata: Copy of the FWU metadata > >> > > > + * > >> > > > + * Get a valid copy of the FWU metadata. > >> > > > + * > >> > > > + * Return: 0 if OK, -ve on error > >> > > > + * > >> > > > + */ > >> > > > +int fwu_get_mdata(struct fwu_mdata **mdata) > >> > > > +{ > >> > > > + struct fwu_mdata_ops *ops; > >> > > > + > >> > > > + ops = get_fwu_mdata_ops(); > >> > > > + if (!ops) > >> > > > + return -EPROTONOSUPPORT; > >> > > > + > >> > > > + if (!ops->get_mdata) { > >> > > > + log_err("get_mdata() method not defined for the > >> > > platform\n"); > >> > > > + return -ENOSYS; > >> > > > + } > >> > > > + > >> > > > + return ops->get_mdata(mdata); > >> > > > +} > >> > > > -- > >> > > > 2.17.1 > >> > > > > >> > > > >> > > > >> > > -- > >> > > Masami Hiramatsu > >> > > > > > > -- > Masami Hiramatsu >
On Tue, Dec 21, 2021 at 11:17:47AM +0530, Sughosh Ganu wrote: > On Tue, 21 Dec 2021 at 10:33, AKASHI Takahiro <takahiro.akashi@linaro.org> > wrote: > > > On Tue, Dec 21, 2021 at 10:05:57AM +0530, Sughosh Ganu wrote: > > > hi Masami, > > > > > > On Tue, 21 Dec 2021 at 05:40, Masami Hiramatsu < > > masami.hiramatsu@linaro.org> > > > wrote: > > > > > > > Hi Sughosh, > > > > > > > > 2021年12月19日(日) 16:06 Sughosh Ganu <sughosh.ganu@linaro.org>: > > > > > > > > > > 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 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> > > > > > --- > > > > > Changes since V1: > > > > > * Move all function declarations to a separate header fwu.h > > > > > * Rename metadata with mdata for all symbols > > > > > * Drop the parameter in the function fwu_revert_boot_index > > > > > as suggested by Etienne > > > > > > > > > > include/fwu.h | 28 +++++ > > > > > include/fwu_mdata.h | 102 ++++++++++++++++ > > > > > lib/fwu_updates/fwu_mdata.c | 236 > > ++++++++++++++++++++++++++++++++++++ > > > > > 3 files changed, 366 insertions(+) > > > > > create mode 100644 include/fwu.h > > > > > create mode 100644 include/fwu_mdata.h > > > > > create mode 100644 lib/fwu_updates/fwu_mdata.c > > > > > > > > > > diff --git a/include/fwu.h b/include/fwu.h > > > > > new file mode 100644 > > > > > index 0000000000..e6bc3e6b73 > > > > > --- /dev/null > > > > > +++ b/include/fwu.h > > > > > @@ -0,0 +1,28 @@ > > > > > +/* SPDX-License-Identifier: GPL-2.0+ */ > > > > > +/* > > > > > + * Copyright (c) 2021, Linaro Limited > > > > > + */ > > > > > + > > > > > +#if !defined _FWU_H_ > > > > > +#define _FWU_H_ > > > > > + > > > > > +#include <efi.h> > > > > > + > > > > > +#include <linux/types.h> > > > > > + > > > > > +#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_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); > > > > > +int fwu_clear_accept_image(efi_guid_t *img_type_id, u32 bank); > > > > > + > > > > > +#endif /* _FWU_H_ */ > > > > > diff --git a/include/fwu_mdata.h b/include/fwu_mdata.h > > > > > new file mode 100644 > > > > > index 0000000000..11eb570012 > > > > > --- /dev/null > > > > > +++ b/include/fwu_mdata.h > > > > > @@ -0,0 +1,102 @@ > > > > > +/* SPDX-License-Identifier: GPL-2.0+ */ > > > > > +/* > > > > > + * Copyright (c) 2021, Linaro Limited > > > > > + */ > > > > > + > > > > > +#if !defined _FWU_MDATA_H_ > > > > > +#define _FWU_MDATA_H_ > > > > > + > > > > > +#include <efi.h> > > > > > +#include <uuid.h> > > > > > + > > > > > +#include <linux/types.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; > > > > > + u32 accepted; > > > > > + u32 reserved; > > > > > +}; > > > > > > > > Could you explicitly pack these metadata data structures? > > > > Since these metadata will be shared among bootloaders (TF-A, > > SCP-firmware), > > > > it is better to ensure that those are packed and fixed offset for each > > > > field. > > > > > > > > > > Okay. WIll do it. > > > > > > > > > > > > > > > + > > > > > +/** > > > > > + * 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]; > > > > > +}; > > > > > + > > > > > +/** > > > > > + * 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 { > > > > > + u32 crc32; > > > > > + u32 version; > > > > > + u32 active_index; > > > > > + u32 previous_active_index; > > > > > + > > > > > + struct fwu_image_entry > > img_entry[CONFIG_FWU_NUM_IMAGES_PER_BANK]; > > > > > +}; > > > > > + > > > > > +/** > > > > > + * @get_active_index: get the current active_index value > > > > > + * @update_active_index: update the active_index value > > > > > + * @get_image_alt_num: get the alt number to be used for the image > > > > > + * @mdata_check: check the validity of the FWU metadata partitions > > > > > + * @revert_boot_index: set the active_index to previous_active_index > > > > > + * @set_accept_image: set the accepted bit for the image > > > > > + * @clear_accept_image: clear the accepted bit for the image > > > > > + * @get_mdata() - Get a FWU metadata copy > > > > > + */ > > > > > +struct fwu_mdata_ops { > > > > > + int (*get_active_index)(u32 *active_idx); > > > > > + > > > > > + int (*update_active_index)(u32 active_idx); > > > > > + > > > > > + int (*get_image_alt_num)(efi_guid_t image_type_id, u32 > > > > update_bank, > > > > > + int *alt_num); > > > > > + > > > > > + int (*mdata_check)(void); > > > > > + > > > > > + int (*revert_boot_index)(void); > > > > > + > > > > > + int (*set_accept_image)(efi_guid_t *img_type_id); > > > > > + > > > > > + int (*clear_accept_image)(efi_guid_t *img_type_id, u32 bank); > > > > > + > > > > > + int (*get_mdata)(struct fwu_mdata **mdata); > > > > > +}; > > > > > + > > > > > +int fwu_get_mdata(struct fwu_mdata **mdata); > > > > > +struct fwu_mdata_ops *get_plat_fwu_mdata_ops(void); > > > > > > > > Also, could you also move these function prototypes and operators to > > > > fwu? I would like to copy this header file to the boot loaders as is. > > > > In that case, I just need the definition of the metadata data > > structure, > > > > but no function prototypes. > > > > > > > > > > I have moved the function declarations to fwu.h. The one function > > prototype > > > that I have in fwu_mdata.h is the one which has the metadata structure as > > > one of it's parameters. Same for the ops as well. The get_mdata function > > > has the metadata structure as its parameter. Are you facing any issues > > with > > > the current structure of the files. > > > > struct fwu_mdata; > > int fwu_get_mdata(struct fwu_**mdata); > > > > would be good enough? > > > > Yes, I had considered putting this kind of declaration in fwu.h. But I > decided against it thinking that any function declaration that involved the > fwu metadata structure can be put in one file. But I just want to > understand if there is some issue in putting these declarations in the > fwu_mdata.h. The rest of the function declarations and symbols are put in > the fwu.h header. If you see, I only include the fwu.h in the > efi_firmware.c and efi_capsule.c. > > > > > > I'm not sure whether it is a good idea that we re-use the *same* file > > in other software, but I would suggest you to use standard POSIX types > > like 'uint32_t' instead of 'u32' anyway. > > > > Hmm, this is on the same lines to the types used in the capsule code. That > was the primary reason that I stuck with u<size> types. Any issue with > using the short type? The Linux kernel seems to be using it. That is my point. Why should we use linux-specific types in other software, especially when we want to import a generic header, which is essentially platform agnostic? -Takahiro Akashi > -sughosh > > > > > > -Takahiro Akashi > > > > > > > -sughosh > > > > > > > > > > Thank you, > > > > > > > > > + > > > > > +#endif /* _FWU_MDATA_H_ */ > > > > > diff --git a/lib/fwu_updates/fwu_mdata.c > > b/lib/fwu_updates/fwu_mdata.c > > > > > new file mode 100644 > > > > > index 0000000000..348e9c85f7 > > > > > --- /dev/null > > > > > +++ b/lib/fwu_updates/fwu_mdata.c > > > > > @@ -0,0 +1,236 @@ > > > > > +// SPDX-License-Identifier: GPL-2.0+ > > > > > +/* > > > > > + * Copyright (c) 2021, Linaro Limited > > > > > + */ > > > > > + > > > > > +#include <fwu_mdata.h> > > > > > + > > > > > +#include <linux/errno.h> > > > > > +#include <linux/types.h> > > > > > + > > > > > +static struct fwu_mdata_ops *get_fwu_mdata_ops(void) > > > > > +{ > > > > > + struct fwu_mdata_ops *ops; > > > > > + > > > > > + ops = get_plat_fwu_mdata_ops(); > > > > > + if (!ops) { > > > > > + log_err("Unable to get fwu ops\n"); > > > > > + return NULL; > > > > > + } > > > > > + > > > > > + return ops; > > > > > +} > > > > > + > > > > > +/** > > > > > + * 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) > > > > > +{ > > > > > + struct fwu_mdata_ops *ops; > > > > > + > > > > > + ops = get_fwu_mdata_ops(); > > > > > + if (!ops) > > > > > + return -EPROTONOSUPPORT; > > > > > + > > > > > + if (!ops->get_active_index) { > > > > > + log_err("get_active_index() method not defined for > > the > > > > platform\n"); > > > > > + return -ENOSYS; > > > > > + } > > > > > + > > > > > + return ops->get_active_index(active_idx); > > > > > +} > > > > > + > > > > > +/** > > > > > + * 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) > > > > > +{ > > > > > + struct fwu_mdata_ops *ops; > > > > > + > > > > > + ops = get_fwu_mdata_ops(); > > > > > + if (!ops) > > > > > + return -EPROTONOSUPPORT; > > > > > + > > > > > + if (!ops->update_active_index) { > > > > > + log_err("update_active_index() method not defined for > > > > the platform\n"); > > > > > + return -ENOSYS; > > > > > + } > > > > > + > > > > > + return ops->update_active_index(active_idx); > > > > > +} > > > > > + > > > > > +/** > > > > > + * fwu_get_image_alt_num() - Get the dfu alt number to be used for > > > > capsule update > > > > > + * @image_type_id: 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) > > > > > +{ > > > > > + struct fwu_mdata_ops *ops; > > > > > + > > > > > + ops = get_fwu_mdata_ops(); > > > > > + if (!ops) > > > > > + return -EPROTONOSUPPORT; > > > > > + > > > > > + if (!ops->get_image_alt_num) { > > > > > + log_err("get_image_alt_num() method not defined for > > the > > > > platform\n"); > > > > > + return -ENOSYS; > > > > > + } > > > > > + > > > > > + return ops->get_image_alt_num(image_type_id, update_bank, > > > > alt_num); > > > > > +} > > > > > + > > > > > +/** > > > > > + * 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) > > > > > +{ > > > > > + struct fwu_mdata_ops *ops; > > > > > + > > > > > + ops = get_fwu_mdata_ops(); > > > > > + if (!ops) > > > > > + return -EPROTONOSUPPORT; > > > > > + > > > > > + if (!ops->mdata_check) { > > > > > + log_err("mdata_check() method not defined for the > > > > platform\n"); > > > > > + return -ENOSYS; > > > > > + } > > > > > + > > > > > + return ops->mdata_check(); > > > > > +} > > > > > + > > > > > +/** > > > > > + * 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) > > > > > +{ > > > > > + struct fwu_mdata_ops *ops; > > > > > + > > > > > + ops = get_fwu_mdata_ops(); > > > > > + if (!ops) > > > > > + return -EPROTONOSUPPORT; > > > > > + > > > > > + if (!ops->revert_boot_index) { > > > > > + log_err("revert_boot_index() method not defined for > > the > > > > platform\n"); > > > > > + return -ENOSYS; > > > > > + } > > > > > + > > > > > + return ops->revert_boot_index(); > > > > > +} > > > > > + > > > > > +/** > > > > > + * 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 > > > > > + * > > > > > + * 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) > > > > > +{ > > > > > + struct fwu_mdata_ops *ops; > > > > > + > > > > > + ops = get_fwu_mdata_ops(); > > > > > + if (!ops) > > > > > + return -EPROTONOSUPPORT; > > > > > + > > > > > + if (!ops->set_accept_image) { > > > > > + log_err("set_accept_image() method not defined for > > the > > > > platform\n"); > > > > > + return -ENOSYS; > > > > > + } > > > > > + > > > > > + return ops->set_accept_image(img_type_id); > > > > > +} > > > > > + > > > > > +/** > > > > > + * 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 > > > > > + * > > > > > + * 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) > > > > > +{ > > > > > + struct fwu_mdata_ops *ops; > > > > > + > > > > > + ops = get_fwu_mdata_ops(); > > > > > + if (!ops) > > > > > + return -EPROTONOSUPPORT; > > > > > + > > > > > + if (!ops->clear_accept_image) { > > > > > + log_err("clear_accept_image() method not defined for > > the > > > > platform\n"); > > > > > + return -ENOSYS; > > > > > + } > > > > > + > > > > > + return ops->clear_accept_image(img_type_id, bank); > > > > > +} > > > > > + > > > > > +/** > > > > > + * fwu_get_mdata() - Get a FWU metadata copy > > > > > + * @mdata: Copy of the FWU metadata > > > > > + * > > > > > + * Get a valid copy of the FWU metadata. > > > > > + * > > > > > + * Return: 0 if OK, -ve on error > > > > > + * > > > > > + */ > > > > > +int fwu_get_mdata(struct fwu_mdata **mdata) > > > > > +{ > > > > > + struct fwu_mdata_ops *ops; > > > > > + > > > > > + ops = get_fwu_mdata_ops(); > > > > > + if (!ops) > > > > > + return -EPROTONOSUPPORT; > > > > > + > > > > > + if (!ops->get_mdata) { > > > > > + log_err("get_mdata() method not defined for the > > > > platform\n"); > > > > > + return -ENOSYS; > > > > > + } > > > > > + > > > > > + return ops->get_mdata(mdata); > > > > > +} > > > > > -- > > > > > 2.17.1 > > > > > > > > > > > > > > > > > -- > > > > Masami Hiramatsu > > > > > >
On Tue, 21 Dec 2021 at 11:46, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > On Tue, Dec 21, 2021 at 11:17:47AM +0530, Sughosh Ganu wrote: > > On Tue, 21 Dec 2021 at 10:33, AKASHI Takahiro < > takahiro.akashi@linaro.org> > > wrote: > > > > > On Tue, Dec 21, 2021 at 10:05:57AM +0530, Sughosh Ganu wrote: > > > > hi Masami, > > > > > > > > On Tue, 21 Dec 2021 at 05:40, Masami Hiramatsu < > > > masami.hiramatsu@linaro.org> > > > > wrote: > > > > > > > > > Hi Sughosh, > > > > > > > > > > 2021年12月19日(日) 16:06 Sughosh Ganu <sughosh.ganu@linaro.org>: > > > > > > > > > > > > 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 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> > > > > > > --- > > > > > > Changes since V1: > > > > > > * Move all function declarations to a separate header fwu.h > > > > > > * Rename metadata with mdata for all symbols > > > > > > * Drop the parameter in the function fwu_revert_boot_index > > > > > > as suggested by Etienne > > > > > > > > > > > > include/fwu.h | 28 +++++ > > > > > > include/fwu_mdata.h | 102 ++++++++++++++++ > > > > > > lib/fwu_updates/fwu_mdata.c | 236 > > > ++++++++++++++++++++++++++++++++++++ > > > > > > 3 files changed, 366 insertions(+) > > > > > > create mode 100644 include/fwu.h > > > > > > create mode 100644 include/fwu_mdata.h > > > > > > create mode 100644 lib/fwu_updates/fwu_mdata.c > > > > > > > > > > > > diff --git a/include/fwu.h b/include/fwu.h > > > > > > new file mode 100644 > > > > > > index 0000000000..e6bc3e6b73 > > > > > > --- /dev/null > > > > > > +++ b/include/fwu.h > > > > > > @@ -0,0 +1,28 @@ > > > > > > +/* SPDX-License-Identifier: GPL-2.0+ */ > > > > > > +/* > > > > > > + * Copyright (c) 2021, Linaro Limited > > > > > > + */ > > > > > > + > > > > > > +#if !defined _FWU_H_ > > > > > > +#define _FWU_H_ > > > > > > + > > > > > > +#include <efi.h> > > > > > > + > > > > > > +#include <linux/types.h> > > > > > > + > > > > > > +#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_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); > > > > > > +int fwu_clear_accept_image(efi_guid_t *img_type_id, u32 bank); > > > > > > + > > > > > > +#endif /* _FWU_H_ */ > > > > > > diff --git a/include/fwu_mdata.h b/include/fwu_mdata.h > > > > > > new file mode 100644 > > > > > > index 0000000000..11eb570012 > > > > > > --- /dev/null > > > > > > +++ b/include/fwu_mdata.h > > > > > > @@ -0,0 +1,102 @@ > > > > > > +/* SPDX-License-Identifier: GPL-2.0+ */ > > > > > > +/* > > > > > > + * Copyright (c) 2021, Linaro Limited > > > > > > + */ > > > > > > + > > > > > > +#if !defined _FWU_MDATA_H_ > > > > > > +#define _FWU_MDATA_H_ > > > > > > + > > > > > > +#include <efi.h> > > > > > > +#include <uuid.h> > > > > > > + > > > > > > +#include <linux/types.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; > > > > > > + u32 accepted; > > > > > > + u32 reserved; > > > > > > +}; > > > > > > > > > > Could you explicitly pack these metadata data structures? > > > > > Since these metadata will be shared among bootloaders (TF-A, > > > SCP-firmware), > > > > > it is better to ensure that those are packed and fixed offset for > each > > > > > field. > > > > > > > > > > > > > Okay. WIll do it. > > > > > > > > > > > > > > > > > > > + > > > > > > +/** > > > > > > + * 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]; > > > > > > +}; > > > > > > + > > > > > > +/** > > > > > > + * 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 { > > > > > > + u32 crc32; > > > > > > + u32 version; > > > > > > + u32 active_index; > > > > > > + u32 previous_active_index; > > > > > > + > > > > > > + struct fwu_image_entry > > > img_entry[CONFIG_FWU_NUM_IMAGES_PER_BANK]; > > > > > > +}; > > > > > > + > > > > > > +/** > > > > > > + * @get_active_index: get the current active_index value > > > > > > + * @update_active_index: update the active_index value > > > > > > + * @get_image_alt_num: get the alt number to be used for the > image > > > > > > + * @mdata_check: check the validity of the FWU metadata > partitions > > > > > > + * @revert_boot_index: set the active_index to > previous_active_index > > > > > > + * @set_accept_image: set the accepted bit for the image > > > > > > + * @clear_accept_image: clear the accepted bit for the image > > > > > > + * @get_mdata() - Get a FWU metadata copy > > > > > > + */ > > > > > > +struct fwu_mdata_ops { > > > > > > + int (*get_active_index)(u32 *active_idx); > > > > > > + > > > > > > + int (*update_active_index)(u32 active_idx); > > > > > > + > > > > > > + int (*get_image_alt_num)(efi_guid_t image_type_id, u32 > > > > > update_bank, > > > > > > + int *alt_num); > > > > > > + > > > > > > + int (*mdata_check)(void); > > > > > > + > > > > > > + int (*revert_boot_index)(void); > > > > > > + > > > > > > + int (*set_accept_image)(efi_guid_t *img_type_id); > > > > > > + > > > > > > + int (*clear_accept_image)(efi_guid_t *img_type_id, u32 > bank); > > > > > > + > > > > > > + int (*get_mdata)(struct fwu_mdata **mdata); > > > > > > +}; > > > > > > + > > > > > > +int fwu_get_mdata(struct fwu_mdata **mdata); > > > > > > +struct fwu_mdata_ops *get_plat_fwu_mdata_ops(void); > > > > > > > > > > Also, could you also move these function prototypes and operators > to > > > > > fwu? I would like to copy this header file to the boot loaders as > is. > > > > > In that case, I just need the definition of the metadata data > > > structure, > > > > > but no function prototypes. > > > > > > > > > > > > > I have moved the function declarations to fwu.h. The one function > > > prototype > > > > that I have in fwu_mdata.h is the one which has the metadata > structure as > > > > one of it's parameters. Same for the ops as well. The get_mdata > function > > > > has the metadata structure as its parameter. Are you facing any > issues > > > with > > > > the current structure of the files. > > > > > > struct fwu_mdata; > > > int fwu_get_mdata(struct fwu_**mdata); > > > > > > would be good enough? > > > > > > > Yes, I had considered putting this kind of declaration in fwu.h. But I > > decided against it thinking that any function declaration that involved > the > > fwu metadata structure can be put in one file. But I just want to > > understand if there is some issue in putting these declarations in the > > fwu_mdata.h. The rest of the function declarations and symbols are put in > > the fwu.h header. If you see, I only include the fwu.h in the > > efi_firmware.c and efi_capsule.c. > > > > > > > > > > I'm not sure whether it is a good idea that we re-use the *same* file > > > in other software, but I would suggest you to use standard POSIX types > > > like 'uint32_t' instead of 'u32' anyway. > > > > > > > Hmm, this is on the same lines to the types used in the capsule code. > That > > was the primary reason that I stuck with u<size> types. Any issue with > > using the short type? The Linux kernel seems to be using it. > > That is my point. Why should we use linux-specific types in > other software, especially when we want to import a generic > header, which is essentially platform agnostic? > Well, I was not aware that the header was to be imported in other software, till Masami explained. I will change the types in the fwu_mdata.h header. -sughosh > > -Takahiro Akashi > > > -sughosh > > > > > > > > > > -Takahiro Akashi > > > > > > > > > > -sughosh > > > > > > > > > > > > > Thank you, > > > > > > > > > > > + > > > > > > +#endif /* _FWU_MDATA_H_ */ > > > > > > diff --git a/lib/fwu_updates/fwu_mdata.c > > > b/lib/fwu_updates/fwu_mdata.c > > > > > > new file mode 100644 > > > > > > index 0000000000..348e9c85f7 > > > > > > --- /dev/null > > > > > > +++ b/lib/fwu_updates/fwu_mdata.c > > > > > > @@ -0,0 +1,236 @@ > > > > > > +// SPDX-License-Identifier: GPL-2.0+ > > > > > > +/* > > > > > > + * Copyright (c) 2021, Linaro Limited > > > > > > + */ > > > > > > + > > > > > > +#include <fwu_mdata.h> > > > > > > + > > > > > > +#include <linux/errno.h> > > > > > > +#include <linux/types.h> > > > > > > + > > > > > > +static struct fwu_mdata_ops *get_fwu_mdata_ops(void) > > > > > > +{ > > > > > > + struct fwu_mdata_ops *ops; > > > > > > + > > > > > > + ops = get_plat_fwu_mdata_ops(); > > > > > > + if (!ops) { > > > > > > + log_err("Unable to get fwu ops\n"); > > > > > > + return NULL; > > > > > > + } > > > > > > + > > > > > > + return ops; > > > > > > +} > > > > > > + > > > > > > +/** > > > > > > + * 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) > > > > > > +{ > > > > > > + struct fwu_mdata_ops *ops; > > > > > > + > > > > > > + ops = get_fwu_mdata_ops(); > > > > > > + if (!ops) > > > > > > + return -EPROTONOSUPPORT; > > > > > > + > > > > > > + if (!ops->get_active_index) { > > > > > > + log_err("get_active_index() method not defined > for > > > the > > > > > platform\n"); > > > > > > + return -ENOSYS; > > > > > > + } > > > > > > + > > > > > > + return ops->get_active_index(active_idx); > > > > > > +} > > > > > > + > > > > > > +/** > > > > > > + * 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) > > > > > > +{ > > > > > > + struct fwu_mdata_ops *ops; > > > > > > + > > > > > > + ops = get_fwu_mdata_ops(); > > > > > > + if (!ops) > > > > > > + return -EPROTONOSUPPORT; > > > > > > + > > > > > > + if (!ops->update_active_index) { > > > > > > + log_err("update_active_index() method not > defined for > > > > > the platform\n"); > > > > > > + return -ENOSYS; > > > > > > + } > > > > > > + > > > > > > + return ops->update_active_index(active_idx); > > > > > > +} > > > > > > + > > > > > > +/** > > > > > > + * fwu_get_image_alt_num() - Get the dfu alt number to be used > for > > > > > capsule update > > > > > > + * @image_type_id: 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) > > > > > > +{ > > > > > > + struct fwu_mdata_ops *ops; > > > > > > + > > > > > > + ops = get_fwu_mdata_ops(); > > > > > > + if (!ops) > > > > > > + return -EPROTONOSUPPORT; > > > > > > + > > > > > > + if (!ops->get_image_alt_num) { > > > > > > + log_err("get_image_alt_num() method not defined > for > > > the > > > > > platform\n"); > > > > > > + return -ENOSYS; > > > > > > + } > > > > > > + > > > > > > + return ops->get_image_alt_num(image_type_id, update_bank, > > > > > alt_num); > > > > > > +} > > > > > > + > > > > > > +/** > > > > > > + * 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) > > > > > > +{ > > > > > > + struct fwu_mdata_ops *ops; > > > > > > + > > > > > > + ops = get_fwu_mdata_ops(); > > > > > > + if (!ops) > > > > > > + return -EPROTONOSUPPORT; > > > > > > + > > > > > > + if (!ops->mdata_check) { > > > > > > + log_err("mdata_check() method not defined for the > > > > > platform\n"); > > > > > > + return -ENOSYS; > > > > > > + } > > > > > > + > > > > > > + return ops->mdata_check(); > > > > > > +} > > > > > > + > > > > > > +/** > > > > > > + * 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) > > > > > > +{ > > > > > > + struct fwu_mdata_ops *ops; > > > > > > + > > > > > > + ops = get_fwu_mdata_ops(); > > > > > > + if (!ops) > > > > > > + return -EPROTONOSUPPORT; > > > > > > + > > > > > > + if (!ops->revert_boot_index) { > > > > > > + log_err("revert_boot_index() method not defined > for > > > the > > > > > platform\n"); > > > > > > + return -ENOSYS; > > > > > > + } > > > > > > + > > > > > > + return ops->revert_boot_index(); > > > > > > +} > > > > > > + > > > > > > +/** > > > > > > + * 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 > > > > > > + * > > > > > > + * 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) > > > > > > +{ > > > > > > + struct fwu_mdata_ops *ops; > > > > > > + > > > > > > + ops = get_fwu_mdata_ops(); > > > > > > + if (!ops) > > > > > > + return -EPROTONOSUPPORT; > > > > > > + > > > > > > + if (!ops->set_accept_image) { > > > > > > + log_err("set_accept_image() method not defined > for > > > the > > > > > platform\n"); > > > > > > + return -ENOSYS; > > > > > > + } > > > > > > + > > > > > > + return ops->set_accept_image(img_type_id); > > > > > > +} > > > > > > + > > > > > > +/** > > > > > > + * 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 > > > > > > + * > > > > > > + * 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) > > > > > > +{ > > > > > > + struct fwu_mdata_ops *ops; > > > > > > + > > > > > > + ops = get_fwu_mdata_ops(); > > > > > > + if (!ops) > > > > > > + return -EPROTONOSUPPORT; > > > > > > + > > > > > > + if (!ops->clear_accept_image) { > > > > > > + log_err("clear_accept_image() method not defined > for > > > the > > > > > platform\n"); > > > > > > + return -ENOSYS; > > > > > > + } > > > > > > + > > > > > > + return ops->clear_accept_image(img_type_id, bank); > > > > > > +} > > > > > > + > > > > > > +/** > > > > > > + * fwu_get_mdata() - Get a FWU metadata copy > > > > > > + * @mdata: Copy of the FWU metadata > > > > > > + * > > > > > > + * Get a valid copy of the FWU metadata. > > > > > > + * > > > > > > + * Return: 0 if OK, -ve on error > > > > > > + * > > > > > > + */ > > > > > > +int fwu_get_mdata(struct fwu_mdata **mdata) > > > > > > +{ > > > > > > + struct fwu_mdata_ops *ops; > > > > > > + > > > > > > + ops = get_fwu_mdata_ops(); > > > > > > + if (!ops) > > > > > > + return -EPROTONOSUPPORT; > > > > > > + > > > > > > + if (!ops->get_mdata) { > > > > > > + log_err("get_mdata() method not defined for the > > > > > platform\n"); > > > > > > + return -ENOSYS; > > > > > > + } > > > > > > + > > > > > > + return ops->get_mdata(mdata); > > > > > > +} > > > > > > -- > > > > > > 2.17.1 > > > > > > > > > > > > > > > > > > > > > -- > > > > > Masami Hiramatsu > > > > > > > > >
diff --git a/include/fwu.h b/include/fwu.h new file mode 100644 index 0000000000..e6bc3e6b73 --- /dev/null +++ b/include/fwu.h @@ -0,0 +1,28 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/* + * Copyright (c) 2021, Linaro Limited + */ + +#if !defined _FWU_H_ +#define _FWU_H_ + +#include <efi.h> + +#include <linux/types.h> + +#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_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); +int fwu_clear_accept_image(efi_guid_t *img_type_id, u32 bank); + +#endif /* _FWU_H_ */ diff --git a/include/fwu_mdata.h b/include/fwu_mdata.h new file mode 100644 index 0000000000..11eb570012 --- /dev/null +++ b/include/fwu_mdata.h @@ -0,0 +1,102 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/* + * Copyright (c) 2021, Linaro Limited + */ + +#if !defined _FWU_MDATA_H_ +#define _FWU_MDATA_H_ + +#include <efi.h> +#include <uuid.h> + +#include <linux/types.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; + u32 accepted; + u32 reserved; +}; + +/** + * 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]; +}; + +/** + * 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 { + u32 crc32; + u32 version; + u32 active_index; + u32 previous_active_index; + + struct fwu_image_entry img_entry[CONFIG_FWU_NUM_IMAGES_PER_BANK]; +}; + +/** + * @get_active_index: get the current active_index value + * @update_active_index: update the active_index value + * @get_image_alt_num: get the alt number to be used for the image + * @mdata_check: check the validity of the FWU metadata partitions + * @revert_boot_index: set the active_index to previous_active_index + * @set_accept_image: set the accepted bit for the image + * @clear_accept_image: clear the accepted bit for the image + * @get_mdata() - Get a FWU metadata copy + */ +struct fwu_mdata_ops { + int (*get_active_index)(u32 *active_idx); + + int (*update_active_index)(u32 active_idx); + + int (*get_image_alt_num)(efi_guid_t image_type_id, u32 update_bank, + int *alt_num); + + int (*mdata_check)(void); + + int (*revert_boot_index)(void); + + int (*set_accept_image)(efi_guid_t *img_type_id); + + int (*clear_accept_image)(efi_guid_t *img_type_id, u32 bank); + + int (*get_mdata)(struct fwu_mdata **mdata); +}; + +int fwu_get_mdata(struct fwu_mdata **mdata); +struct fwu_mdata_ops *get_plat_fwu_mdata_ops(void); + +#endif /* _FWU_MDATA_H_ */ diff --git a/lib/fwu_updates/fwu_mdata.c b/lib/fwu_updates/fwu_mdata.c new file mode 100644 index 0000000000..348e9c85f7 --- /dev/null +++ b/lib/fwu_updates/fwu_mdata.c @@ -0,0 +1,236 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Copyright (c) 2021, Linaro Limited + */ + +#include <fwu_mdata.h> + +#include <linux/errno.h> +#include <linux/types.h> + +static struct fwu_mdata_ops *get_fwu_mdata_ops(void) +{ + struct fwu_mdata_ops *ops; + + ops = get_plat_fwu_mdata_ops(); + if (!ops) { + log_err("Unable to get fwu ops\n"); + return NULL; + } + + return ops; +} + +/** + * 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) +{ + struct fwu_mdata_ops *ops; + + ops = get_fwu_mdata_ops(); + if (!ops) + return -EPROTONOSUPPORT; + + if (!ops->get_active_index) { + log_err("get_active_index() method not defined for the platform\n"); + return -ENOSYS; + } + + return ops->get_active_index(active_idx); +} + +/** + * 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) +{ + struct fwu_mdata_ops *ops; + + ops = get_fwu_mdata_ops(); + if (!ops) + return -EPROTONOSUPPORT; + + if (!ops->update_active_index) { + log_err("update_active_index() method not defined for the platform\n"); + return -ENOSYS; + } + + return ops->update_active_index(active_idx); +} + +/** + * fwu_get_image_alt_num() - Get the dfu alt number to be used for capsule update + * @image_type_id: 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) +{ + struct fwu_mdata_ops *ops; + + ops = get_fwu_mdata_ops(); + if (!ops) + return -EPROTONOSUPPORT; + + if (!ops->get_image_alt_num) { + log_err("get_image_alt_num() method not defined for the platform\n"); + return -ENOSYS; + } + + return ops->get_image_alt_num(image_type_id, update_bank, alt_num); +} + +/** + * 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) +{ + struct fwu_mdata_ops *ops; + + ops = get_fwu_mdata_ops(); + if (!ops) + return -EPROTONOSUPPORT; + + if (!ops->mdata_check) { + log_err("mdata_check() method not defined for the platform\n"); + return -ENOSYS; + } + + return ops->mdata_check(); +} + +/** + * 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) +{ + struct fwu_mdata_ops *ops; + + ops = get_fwu_mdata_ops(); + if (!ops) + return -EPROTONOSUPPORT; + + if (!ops->revert_boot_index) { + log_err("revert_boot_index() method not defined for the platform\n"); + return -ENOSYS; + } + + return ops->revert_boot_index(); +} + +/** + * 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 + * + * 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) +{ + struct fwu_mdata_ops *ops; + + ops = get_fwu_mdata_ops(); + if (!ops) + return -EPROTONOSUPPORT; + + if (!ops->set_accept_image) { + log_err("set_accept_image() method not defined for the platform\n"); + return -ENOSYS; + } + + return ops->set_accept_image(img_type_id); +} + +/** + * 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 + * + * 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) +{ + struct fwu_mdata_ops *ops; + + ops = get_fwu_mdata_ops(); + if (!ops) + return -EPROTONOSUPPORT; + + if (!ops->clear_accept_image) { + log_err("clear_accept_image() method not defined for the platform\n"); + return -ENOSYS; + } + + return ops->clear_accept_image(img_type_id, bank); +} + +/** + * fwu_get_mdata() - Get a FWU metadata copy + * @mdata: Copy of the FWU metadata + * + * Get a valid copy of the FWU metadata. + * + * Return: 0 if OK, -ve on error + * + */ +int fwu_get_mdata(struct fwu_mdata **mdata) +{ + struct fwu_mdata_ops *ops; + + ops = get_fwu_mdata_ops(); + if (!ops) + return -EPROTONOSUPPORT; + + if (!ops->get_mdata) { + log_err("get_mdata() method not defined for the platform\n"); + return -ENOSYS; + } + + return ops->get_mdata(mdata); +}
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 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> --- Changes since V1: * Move all function declarations to a separate header fwu.h * Rename metadata with mdata for all symbols * Drop the parameter in the function fwu_revert_boot_index as suggested by Etienne include/fwu.h | 28 +++++ include/fwu_mdata.h | 102 ++++++++++++++++ lib/fwu_updates/fwu_mdata.c | 236 ++++++++++++++++++++++++++++++++++++ 3 files changed, 366 insertions(+) create mode 100644 include/fwu.h create mode 100644 include/fwu_mdata.h create mode 100644 lib/fwu_updates/fwu_mdata.c