| Message ID | 20211219070605.14894-4-sughosh.ganu@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | FWU: Add support for FWU Multi Bank Update feature | expand |
() should Hi Sughosh and Ilias, I would like to confirm that what the plat_fill_gpt_partition_guids() should return. 2021年12月19日(日) 16:07 Sughosh Ganu <sughosh.ganu@linaro.org>: > +static int plat_fill_gpt_partition_guids(struct blk_desc *desc, > + efi_guid_t **part_guid_arr) > +{ > + int i, ret = 0; > + u32 part; > + struct dfu_entity *dfu; > + struct disk_partition info; > + efi_guid_t part_type_guid; > + int alt_num = dfu_get_alt_number(); > + > + dfu_init_env_entities(NULL, NULL); > + > + for (i = 0, part = 1; i < alt_num; i++) { > + dfu = dfu_get_entity(i); > + > + if (!dfu) > + continue; > + > + /* > + * Currently, Multi Bank update > + * feature is being supported > + * only on GPT partitioned > + * MMC/SD devices. > + */ > + if (dfu->dev_type != DFU_DEV_MMC) > + continue; > + > + if (part_get_info(desc, part, &info)) { > + part++; > + continue; > + } > + > + uuid_str_to_bin(info.type_guid, part_type_guid.b, > + UUID_STR_FORMAT_GUID); > + guidcpy((*part_guid_arr + i), &part_type_guid); > + part++; > + } > + > + dfu_free_entities(); > + > + return ret; > +} So on this platform which uses GPT partition for each bank, it stores the GUID for each partition. I think this information will be shown in the ESRT, but not used for specifying capsule image_type_uuid. This is strange. Without this FWU multi-bank update, the array will only have the image_type UUID. Thus, user can use ESRT to confirm what UUID should be specified for the capsule file. (it has image-type UUID) (of course that is fixed value on one platform anyway) And when FWU multi-bank update is enabled, that changes to the partition GUIDs. But those GUIDs are NOT used when making the capsule file nor doing the update process, because we should not specify the bank index directly, right? We will set a unique image-type UUID on each image entry, and use it for making a capsule file, but the capsule file itself doesn't specify the banks. And acceptance/revert capsule file also doesn't specify it. Acceptance file will also uses image_type UUID, but not the partition UUID for the bank. Maybe I'm wrong, but I'm confusing. And this is important for the platform which doesn't use GPT. Thank you,
hi Masami, On Fri, 24 Dec 2021 at 11:19, Masami Hiramatsu <masami.hiramatsu@linaro.org> wrote: > > () should Hi Sughosh and Ilias, > > I would like to confirm that what the plat_fill_gpt_partition_guids() > should return. > > 2021年12月19日(日) 16:07 Sughosh Ganu <sughosh.ganu@linaro.org>: > > > +static int plat_fill_gpt_partition_guids(struct blk_desc *desc, > > + efi_guid_t **part_guid_arr) > > +{ > > + int i, ret = 0; > > + u32 part; > > + struct dfu_entity *dfu; > > + struct disk_partition info; > > + efi_guid_t part_type_guid; > > + int alt_num = dfu_get_alt_number(); > > + > > + dfu_init_env_entities(NULL, NULL); > > + > > + for (i = 0, part = 1; i < alt_num; i++) { > > + dfu = dfu_get_entity(i); > > + > > + if (!dfu) > > + continue; > > + > > + /* > > + * Currently, Multi Bank update > > + * feature is being supported > > + * only on GPT partitioned > > + * MMC/SD devices. > > + */ > > + if (dfu->dev_type != DFU_DEV_MMC) > > + continue; > > + > > + if (part_get_info(desc, part, &info)) { > > + part++; > > + continue; > > + } > > + > > + uuid_str_to_bin(info.type_guid, part_type_guid.b, > > + UUID_STR_FORMAT_GUID); > > + guidcpy((*part_guid_arr + i), &part_type_guid); > > + part++; > > + } > > + > > + dfu_free_entities(); > > + > > + return ret; > > +} > > So on this platform which uses GPT partition for each bank, it stores > the GUID for each partition. > I think this information will be shown in the ESRT, but not used for > specifying capsule image_type_uuid. > > This is strange. > Without this FWU multi-bank update, the array will only have the > image_type UUID. > Thus, user can use ESRT to confirm what UUID should be specified for > the capsule file. (it has image-type UUID) > (of course that is fixed value on one platform anyway) > > And when FWU multi-bank update is enabled, that changes to the > partition GUIDs. But those GUIDs are NOT used when making the capsule > file nor doing the update process, because we should not specify the > bank index directly, right? The partition GUIDs are the same as ImageTypeId field in the EFI_FIRMWARE_IMAGE_DESCRIPTOR and the UpdateImageTypeId field in the EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER. This value is the same as the PartitionTypeGUID field in the GPT partition entry. ImageTypeId = UpdateImageTypeId = PartitionTypeGUID This is the GUID value that is used when generating the capsule. > > We will set a unique image-type UUID on each image entry, and use it > for making a capsule file, Even with the FWU feature enabled, the PartitionTypeGUID/ImageTypeId is used for making the capsule. What changes for the FWU feature is that the value of UpdateImageIndex/ImageIndex that is passed to the capsule generation tool becomes irrelevant. When the FWU feature is not enabled, the ImageIndex value that is passed to the capsule generation tool is important, since this is supposed to correspond to the DFU alt number -- this ImageIndex value is then used to identify the partition/address to which the capsule payload is to be written. With the FWU feature enabled, this value is not obtained from the capsule, since the partition/address to write the image to is identified at runtime based on the value of the update bank. > but the capsule file itself doesn't specify > the banks. And acceptance/revert capsule file also doesn't specify it. > Acceptance file will also uses image_type UUID, but not the partition > UUID for the bank. The image acceptance capsule will also contain the ImageTypeId. The Accept bit will be set for the active bank image of the image type specified in the acceptance capsule. -sughosh > > Maybe I'm wrong, but I'm confusing. And this is important for the > platform which doesn't use GPT. > > Thank you, > > > -- > Masami Hiramatsu
Hi Sughosh, 2021年12月24日(金) 19:08 Sughosh Ganu <sughosh.ganu@linaro.org>: > > hi Masami, > > On Fri, 24 Dec 2021 at 11:19, Masami Hiramatsu > <masami.hiramatsu@linaro.org> wrote: > > > > () should Hi Sughosh and Ilias, > > > > I would like to confirm that what the plat_fill_gpt_partition_guids() > > should return. > > > > 2021年12月19日(日) 16:07 Sughosh Ganu <sughosh.ganu@linaro.org>: > > > > > +static int plat_fill_gpt_partition_guids(struct blk_desc *desc, > > > + efi_guid_t **part_guid_arr) > > > +{ > > > + int i, ret = 0; > > > + u32 part; > > > + struct dfu_entity *dfu; > > > + struct disk_partition info; > > > + efi_guid_t part_type_guid; > > > + int alt_num = dfu_get_alt_number(); > > > + > > > + dfu_init_env_entities(NULL, NULL); > > > + > > > + for (i = 0, part = 1; i < alt_num; i++) { > > > + dfu = dfu_get_entity(i); > > > + > > > + if (!dfu) > > > + continue; > > > + > > > + /* > > > + * Currently, Multi Bank update > > > + * feature is being supported > > > + * only on GPT partitioned > > > + * MMC/SD devices. > > > + */ > > > + if (dfu->dev_type != DFU_DEV_MMC) > > > + continue; > > > + > > > + if (part_get_info(desc, part, &info)) { > > > + part++; > > > + continue; > > > + } > > > + > > > + uuid_str_to_bin(info.type_guid, part_type_guid.b, > > > + UUID_STR_FORMAT_GUID); > > > + guidcpy((*part_guid_arr + i), &part_type_guid); > > > + part++; > > > + } > > > + > > > + dfu_free_entities(); > > > + > > > + return ret; > > > +} > > > > So on this platform which uses GPT partition for each bank, it stores > > the GUID for each partition. > > I think this information will be shown in the ESRT, but not used for > > specifying capsule image_type_uuid. > > > > This is strange. > > Without this FWU multi-bank update, the array will only have the > > image_type UUID. > > Thus, user can use ESRT to confirm what UUID should be specified for > > the capsule file. (it has image-type UUID) > > (of course that is fixed value on one platform anyway) > > > > And when FWU multi-bank update is enabled, that changes to the > > partition GUIDs. But those GUIDs are NOT used when making the capsule > > file nor doing the update process, because we should not specify the > > bank index directly, right? > > The partition GUIDs are the same as ImageTypeId field in the > EFI_FIRMWARE_IMAGE_DESCRIPTOR and the UpdateImageTypeId field in the > EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER. This value is the same > as the PartitionTypeGUID field in the GPT partition entry. > > ImageTypeId = UpdateImageTypeId = PartitionTypeGUID > > This is the GUID value that is used when generating the capsule. So you meant the above "plat_fill_gpt_partition_guids()" fills the array with ImageTypeId? Thus, if we have a single image type, the array will be filled with that single GUID? > > We will set a unique image-type UUID on each image entry, and use it > > for making a capsule file, > > Even with the FWU feature enabled, the PartitionTypeGUID/ImageTypeId > is used for making the capsule. What changes for the FWU feature is > that the value of UpdateImageIndex/ImageIndex that is passed to the > capsule generation tool becomes irrelevant. When the FWU feature is > not enabled, the ImageIndex value that is passed to the capsule > generation tool is important, since this is supposed to correspond to > the DFU alt number -- this ImageIndex value is then used to identify > the partition/address to which the capsule payload is to be written. > With the FWU feature enabled, this value is not obtained from the > capsule, since the partition/address to write the image to is > identified at runtime based on the value of the update bank. OK. > > > but the capsule file itself doesn't specify > > the banks. And acceptance/revert capsule file also doesn't specify it. > > Acceptance file will also uses image_type UUID, but not the partition > > UUID for the bank. > > The image acceptance capsule will also contain the ImageTypeId. The > Accept bit will be set for the active bank image of the image type > specified in the acceptance capsule. OK, so the Image UUID(image slot for the banks) will not be used in the process, is that correct? I would like to confirm this, because this is important for the system which doesn't use GPT. Thank you, > > -sughosh > > > > > Maybe I'm wrong, but I'm confusing. And this is important for the > > platform which doesn't use GPT. > > > > Thank you, > > > > > > -- > > Masami Hiramatsu
hi Masami, On Fri, 24 Dec 2021 at 15:49, Masami Hiramatsu <masami.hiramatsu@linaro.org> wrote: > > Hi Sughosh, > > 2021年12月24日(金) 19:08 Sughosh Ganu <sughosh.ganu@linaro.org>: > > > > hi Masami, > > > > On Fri, 24 Dec 2021 at 11:19, Masami Hiramatsu > > <masami.hiramatsu@linaro.org> wrote: > > > > > > () should Hi Sughosh and Ilias, > > > > > > I would like to confirm that what the plat_fill_gpt_partition_guids() > > > should return. > > > > > > 2021年12月19日(日) 16:07 Sughosh Ganu <sughosh.ganu@linaro.org>: > > > > > > > +static int plat_fill_gpt_partition_guids(struct blk_desc *desc, > > > > + efi_guid_t **part_guid_arr) > > > > +{ > > > > + int i, ret = 0; > > > > + u32 part; > > > > + struct dfu_entity *dfu; > > > > + struct disk_partition info; > > > > + efi_guid_t part_type_guid; > > > > + int alt_num = dfu_get_alt_number(); > > > > + > > > > + dfu_init_env_entities(NULL, NULL); > > > > + > > > > + for (i = 0, part = 1; i < alt_num; i++) { > > > > + dfu = dfu_get_entity(i); > > > > + > > > > + if (!dfu) > > > > + continue; > > > > + > > > > + /* > > > > + * Currently, Multi Bank update > > > > + * feature is being supported > > > > + * only on GPT partitioned > > > > + * MMC/SD devices. > > > > + */ > > > > + if (dfu->dev_type != DFU_DEV_MMC) > > > > + continue; > > > > + > > > > + if (part_get_info(desc, part, &info)) { > > > > + part++; > > > > + continue; > > > > + } > > > > + > > > > + uuid_str_to_bin(info.type_guid, part_type_guid.b, > > > > + UUID_STR_FORMAT_GUID); > > > > + guidcpy((*part_guid_arr + i), &part_type_guid); > > > > + part++; > > > > + } > > > > + > > > > + dfu_free_entities(); > > > > + > > > > + return ret; > > > > +} > > > > > > So on this platform which uses GPT partition for each bank, it stores > > > the GUID for each partition. > > > I think this information will be shown in the ESRT, but not used for > > > specifying capsule image_type_uuid. > > > > > > This is strange. > > > Without this FWU multi-bank update, the array will only have the > > > image_type UUID. > > > Thus, user can use ESRT to confirm what UUID should be specified for > > > the capsule file. (it has image-type UUID) > > > (of course that is fixed value on one platform anyway) > > > > > > And when FWU multi-bank update is enabled, that changes to the > > > partition GUIDs. But those GUIDs are NOT used when making the capsule > > > file nor doing the update process, because we should not specify the > > > bank index directly, right? > > > > The partition GUIDs are the same as ImageTypeId field in the > > EFI_FIRMWARE_IMAGE_DESCRIPTOR and the UpdateImageTypeId field in the > > EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER. This value is the same > > as the PartitionTypeGUID field in the GPT partition entry. > > > > ImageTypeId = UpdateImageTypeId = PartitionTypeGUID > > > > This is the GUID value that is used when generating the capsule. > > So you meant the above "plat_fill_gpt_partition_guids()" fills the > array with ImageTypeId? Yes. Please note that the actual api is fwu_plat_fill_partition_guids. For the ST board, fwu_plat_fill_partition_guids calls plat_fill_gpt_partition_guids. > Thus, if we have a single image type, the array will be filled with > that single GUID? That will depend on the dfu_alt_info setting and the platform function fwu_plat_fill_partition_guids defined for the corresponding platform. > > > > > We will set a unique image-type UUID on each image entry, and use it > > > for making a capsule file, > > > > Even with the FWU feature enabled, the PartitionTypeGUID/ImageTypeId > > is used for making the capsule. What changes for the FWU feature is > > that the value of UpdateImageIndex/ImageIndex that is passed to the > > capsule generation tool becomes irrelevant. When the FWU feature is > > not enabled, the ImageIndex value that is passed to the capsule > > generation tool is important, since this is supposed to correspond to > > the DFU alt number -- this ImageIndex value is then used to identify > > the partition/address to which the capsule payload is to be written. > > With the FWU feature enabled, this value is not obtained from the > > capsule, since the partition/address to write the image to is > > identified at runtime based on the value of the update bank. > > OK. > > > > > > but the capsule file itself doesn't specify > > > the banks. And acceptance/revert capsule file also doesn't specify it. > > > Acceptance file will also uses image_type UUID, but not the partition > > > UUID for the bank. > > > > The image acceptance capsule will also contain the ImageTypeId. The > > Accept bit will be set for the active bank image of the image type > > specified in the acceptance capsule. > > OK, so the Image UUID(image slot for the banks) will not be used in > the process, is that correct? > I would like to confirm this, because this is important for the system > which doesn't use GPT. Yes, only the ImageTypeId will be used. You can check the implementation for the GPT partitioned block devices in fwu_gpt_set_clear_image_accept. -sughosh > > Thank you, > > > > > -sughosh > > > > > > > > Maybe I'm wrong, but I'm confusing. And this is important for the > > > platform which doesn't use GPT. > > > > > > Thank you, > > > > > > > > > -- > > > Masami Hiramatsu > > > > -- > Masami Hiramatsu
diff --git a/board/st/stm32mp1/stm32mp1.c b/board/st/stm32mp1/stm32mp1.c index 84592677e4..28402fd127 100644 --- a/board/st/stm32mp1/stm32mp1.c +++ b/board/st/stm32mp1/stm32mp1.c @@ -7,10 +7,13 @@ #include <common.h> #include <adc.h> +#include <blk.h> #include <bootm.h> #include <clk.h> #include <config.h> +#include <dfu.h> #include <dm.h> +#include <efi_loader.h> #include <env.h> #include <env_internal.h> #include <fdt_support.h> @@ -23,9 +26,11 @@ #include <log.h> #include <malloc.h> #include <misc.h> +#include <mmc.h> #include <mtd_node.h> #include <net.h> #include <netdev.h> +#include <part.h> #include <phy.h> #include <remoteproc.h> #include <reset.h> @@ -938,3 +943,160 @@ static void board_copro_image_process(ulong fw_image, size_t fw_size) } U_BOOT_FIT_LOADABLE_HANDLER(IH_TYPE_COPRO, board_copro_image_process); + +#if defined(CONFIG_FWU_MULTI_BANK_UPDATE) +#include <fwu.h> +#include <fwu_mdata.h> + +int fwu_plat_get_alt_num(int dev_num, void *identifier) +{ + int i; + int ret = -1; + u32 part; + int alt_num = dfu_get_alt_number(); + struct dfu_entity *dfu; + + part = *(u32 *)identifier; + dfu_init_env_entities(NULL, NULL); + + for (i = 0; i < alt_num; i++) { + dfu = dfu_get_entity(i); + + if (!dfu) + continue; + + /* + * Currently, Multi Bank update + * feature is being supported + * only on GPT partitioned + * MMC/SD devices. + */ + if (dfu->dev_type != DFU_DEV_MMC) + continue; + + if (dfu->layout == DFU_RAW_ADDR && + dfu->data.mmc.dev_num == dev_num && + dfu->data.mmc.part == part) { + ret = dfu->alt; + break; + } + } + + dfu_free_entities(); + + return ret; +} + +static int plat_fill_gpt_partition_guids(struct blk_desc *desc, + efi_guid_t **part_guid_arr) +{ + int i, ret = 0; + u32 part; + struct dfu_entity *dfu; + struct disk_partition info; + efi_guid_t part_type_guid; + int alt_num = dfu_get_alt_number(); + + dfu_init_env_entities(NULL, NULL); + + for (i = 0, part = 1; i < alt_num; i++) { + dfu = dfu_get_entity(i); + + if (!dfu) + continue; + + /* + * Currently, Multi Bank update + * feature is being supported + * only on GPT partitioned + * MMC/SD devices. + */ + if (dfu->dev_type != DFU_DEV_MMC) + continue; + + if (part_get_info(desc, part, &info)) { + part++; + continue; + } + + uuid_str_to_bin(info.type_guid, part_type_guid.b, + UUID_STR_FORMAT_GUID); + guidcpy((*part_guid_arr + i), &part_type_guid); + part++; + } + + dfu_free_entities(); + + return ret; +} + +int fwu_plat_fill_partition_guids(efi_guid_t **part_guid_arr) +{ + int ret; + struct blk_desc *desc; + + ret = fwu_plat_get_blk_desc(&desc); + if (ret < 0) { + log_err("Block device not found\n"); + return -ENODEV; + } + + return plat_fill_gpt_partition_guids(desc, part_guid_arr); +} + +int fwu_plat_get_update_index(u32 *update_idx) +{ + int ret; + u32 active_idx; + + ret = fwu_get_active_index(&active_idx); + + if (ret < 0) + return -1; + + *update_idx = active_idx ^= 0x1; + + return ret; +} + +int fwu_plat_get_blk_desc(struct blk_desc **desc) +{ + int ret; + struct mmc *mmc; + struct udevice *dev; + + /* + * Initial support is being added for the DK2 + * platform + */ + if (CONFIG_IS_ENABLED(TARGET_ST_STM32MP15x) && + (of_machine_is_compatible("st,stm32mp157c-dk2"))) { + ret = uclass_get_device(UCLASS_MMC, 0, &dev); + if (ret) + return -1; + + mmc = mmc_get_mmc_dev(dev); + if (!mmc) + return -1; + + if (mmc_init(mmc)) + return -1; + + *desc = mmc_get_blk_desc(mmc); + if (!*desc) + return -1; + } + + return 0; +} + +struct fwu_mdata_ops *get_plat_fwu_mdata_ops(void) +{ + if (CONFIG_IS_ENABLED(TARGET_ST_STM32MP15x) && + (of_machine_is_compatible("st,stm32mp157c-dk2"))) { + return &fwu_gpt_blk_ops; + } + + return NULL; +} +#endif /* CONFIG_FWU_MULTI_BANK_UPDATE */ diff --git a/include/fwu.h b/include/fwu.h index e6bc3e6b73..1e7a1eabff 100644 --- a/include/fwu.h +++ b/include/fwu.h @@ -25,4 +25,9 @@ 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); +int fwu_plat_get_update_index(u32 *update_idx); +int fwu_plat_get_blk_desc(struct blk_desc **desc); +int fwu_plat_get_alt_num(int dev_num, void *identifier); +int fwu_plat_fill_partition_guids(efi_guid_t **part_guid_arr); + #endif /* _FWU_H_ */ diff --git a/lib/fwu_updates/fwu_mdata_gpt_blk.c b/lib/fwu_updates/fwu_mdata_gpt_blk.c index 2dcac0c3d4..6a2fa176f9 100644 --- a/lib/fwu_updates/fwu_mdata_gpt_blk.c +++ b/lib/fwu_updates/fwu_mdata_gpt_blk.c @@ -53,6 +53,7 @@ static int gpt_get_mdata_partitions(struct blk_desc *desc, struct disk_partition info; const efi_guid_t fwu_mdata_guid = FWU_MDATA_GUID; + mdata_parts = 0; for (i = 1; i < MAX_SEARCH_PARTITIONS; i++) { if (part_get_info(desc, i, &info)) continue; @@ -340,7 +341,7 @@ out: int fwu_gpt_get_active_index(u32 *active_idx) { int ret; - struct fwu_mdata *mdata; + struct fwu_mdata *mdata = NULL; ret = gpt_get_mdata(&mdata); if (ret < 0) { @@ -371,7 +372,7 @@ static int gpt_get_image_alt_num(struct blk_desc *desc, { int ret, i; u32 part; - struct fwu_mdata *mdata; + struct fwu_mdata *mdata = NULL; struct fwu_image_entry *img_entry; struct fwu_image_bank_info *img_bank_info; struct disk_partition info; @@ -443,7 +444,7 @@ int fwu_gpt_update_active_index(u32 active_idx) { int ret; void *buf; - struct fwu_mdata *mdata; + struct fwu_mdata *mdata = NULL; if (active_idx > CONFIG_FWU_NUM_BANKS) { printf("Active index value to be updated is incorrect\n"); @@ -523,7 +524,7 @@ int fwu_gpt_revert_boot_index(void) int ret; void *buf; u32 cur_active_index; - struct fwu_mdata *mdata; + struct fwu_mdata *mdata = NULL; ret = gpt_get_mdata(&mdata); if (ret < 0) { @@ -569,7 +570,7 @@ static int fwu_gpt_set_clear_image_accept(efi_guid_t *img_type_id, void *buf; int ret, i; u32 nimages; - struct fwu_mdata *mdata; + struct fwu_mdata *mdata = NULL; struct fwu_image_entry *img_entry; struct fwu_image_bank_info *img_bank_info;
Add helper functions needed for accessing the FWU metadata which contains information on the updatable images. These functions have been added for the STM32MP157C-DK2 board which has the updatable images on the uSD card, formatted as GPT partitions. Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> --- Changes since V1: * Define a new function fwu_plat_get_alt_num using logic suggested by Patrick for returning the alt_num for the partition * Define a new function plat_fill_gpt_partition_guids to fill the guid array with Partition Type guids board/st/stm32mp1/stm32mp1.c | 162 ++++++++++++++++++++++++++++ include/fwu.h | 5 + lib/fwu_updates/fwu_mdata_gpt_blk.c | 11 +- 3 files changed, 173 insertions(+), 5 deletions(-)