| Message ID | 20211125071302.3644-5-sughosh.ganu@linaro.org |
|---|---|
| State | RFC |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | FWU: Add support for FWU Multi Bank Update feature | expand |
Hi Sughosh, On Thu, Nov 25, 2021 at 12:42:56PM +0530, Sughosh Ganu wrote: > In the FWU Multi Bank Update feature, the information about the > updatable images is stored as part of the metadata, on a separate > partition. Add functions for reading from and writing to the metadata > when the updatable images and the metadata are stored on a block > device which is formated with GPT based partition scheme. > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > --- > lib/fwu_updates/fwu_metadata_gpt_blk.c | 716 +++++++++++++++++++++++++ > 1 file changed, 716 insertions(+) > create mode 100644 lib/fwu_updates/fwu_metadata_gpt_blk.c > > +#define SECONDARY_VALID 0x2 Change those to BIT(0), BIT(1) etc please > + > +#define MDATA_READ (u8)0x1 > +#define MDATA_WRITE (u8)0x2 > + > +#define IMAGE_ACCEPT_SET (u8)0x1 > +#define IMAGE_ACCEPT_CLEAR (u8)0x2 > + > +static int gpt_verify_metadata(struct fwu_metadata *metadata, bool pri_part) > +{ > + u32 calc_crc32; > + void *buf; > + > + buf = &metadata->version; > + calc_crc32 = crc32(0, buf, sizeof(*metadata) - sizeof(u32)); > + > + if (calc_crc32 != metadata->crc32) { > + log_err("crc32 check failed for %s metadata partition\n", > + pri_part ? "primary" : "secondary"); I think we can rid of the 'bool pri_part'. It's only used for printing and you can have more that 2 partitions anyway right? > + return -1; > + } > + > + return 0; > +} > + > +static int gpt_get_metadata_partitions(struct blk_desc *desc, Please add a function descriptions (on following functions as well) > + u32 *primary_mpart, > + u32 *secondary_mpart) u16 maybe? This is the number of gpt partitions with metadata right? > +{ > + int i, ret; > + u32 nparts, mparts; > + gpt_entry *gpt_pte = NULL; > + const efi_guid_t fwu_metadata_guid = FWU_METADATA_GUID; > + > + ALLOC_CACHE_ALIGN_BUFFER_PAD(gpt_header, gpt_head, 1, > + desc->blksz); > + > + ret = get_gpt_hdr_parts(desc, gpt_head, &gpt_pte); > + if (ret < 0) { > + log_err("Error getting GPT header and partitions\n"); > + ret = -EIO; > + goto out; > + } > + > + nparts = gpt_head->num_partition_entries; > + for (i = 0, mparts = 0; i < nparts; i++) { > + if (!guidcmp(&fwu_metadata_guid, > + &gpt_pte[i].partition_type_guid)) { > + ++mparts; > + if (!*primary_mpart) > + *primary_mpart = i + 1; > + else > + *secondary_mpart = i + 1; > + } > + } > + > + if (mparts != 2) { > + log_err("Expect two copies of the metadata instead of %d\n", > + mparts); > + ret = -EINVAL; > + } else { > + ret = 0; > + } > +out: > + free(gpt_pte); > + > + return ret; > +} > + > +static int gpt_get_metadata_disk_part(struct blk_desc *desc, > + struct disk_partition *info, > + u32 part_num) > +{ > + int ret; > + char *metadata_guid_str = "8a7a84a0-8387-40f6-ab41-a8b9a5a60d23"; > + > + ret = part_get_info(desc, part_num, info); > + if (ret < 0) { > + log_err("Unable to get the partition info for the metadata part %d", > + part_num); > + return -1; > + } > + > + /* Check that it is indeed the metadata partition */ > + if (!strncmp(info->type_guid, metadata_guid_str, UUID_STR_LEN)) { > + /* Found the metadata partition */ > + return 0; > + } > + > + return -1; > +} > + > +static int gpt_read_write_metadata(struct blk_desc *desc, > + struct fwu_metadata *metadata, > + u8 access, u32 part_num) > +{ > + int ret; > + u32 len, blk_start, blkcnt; > + struct disk_partition info; > + > + ALLOC_CACHE_ALIGN_BUFFER_PAD(struct fwu_metadata, mdata, 1, desc->blksz); > + > + ret = gpt_get_metadata_disk_part(desc, &info, part_num); > + if (ret < 0) { > + printf("Unable to get the metadata partition\n"); > + return -ENODEV; > + } > + > + len = sizeof(*metadata); > + blkcnt = BLOCK_CNT(len, desc); > + if (blkcnt > info.size) { > + log_err("Block count exceeds metadata partition size\n"); > + return -ERANGE; > + } > + > + blk_start = info.start; > + if (access == MDATA_READ) { > + if (blk_dread(desc, blk_start, blkcnt, mdata) != blkcnt) { > + log_err("Error reading metadata from the device\n"); > + return -EIO; > + } > + memcpy(metadata, mdata, sizeof(struct fwu_metadata)); > + } else { > + if (blk_dwrite(desc, blk_start, blkcnt, metadata) != blkcnt) { > + log_err("Error writing metadata to the device\n"); > + return -EIO; > + } > + } > + > + return 0; > +} > + > +static int gpt_read_metadata(struct blk_desc *desc, > + struct fwu_metadata *metadata, u32 part_num) > +{ > + return gpt_read_write_metadata(desc, metadata, MDATA_READ, part_num); > +} > + > +static int gpt_write_metadata_partition(struct blk_desc *desc, > + struct fwu_metadata *metadata, > + u32 part_num) > +{ > + return gpt_read_write_metadata(desc, metadata, MDATA_WRITE, part_num); > +} > + > +static int gpt_update_metadata(struct fwu_metadata *metadata) > +{ > + int ret; > + struct blk_desc *desc; > + u32 primary_mpart, secondary_mpart; > + > + ret = fwu_plat_get_blk_desc(&desc); > + if (ret < 0) { > + log_err("Block device not found\n"); > + return -ENODEV; > + } > + > + primary_mpart = secondary_mpart = 0; > + ret = gpt_get_metadata_partitions(desc, &primary_mpart, > + &secondary_mpart); > + > + if (ret < 0) { > + log_err("Error getting the metadata partitions\n"); > + return -ENODEV; > + } > + > + /* First write the primary partition*/ > + ret = gpt_write_metadata_partition(desc, metadata, primary_mpart); > + if (ret < 0) { > + log_err("Updating primary metadata partition failed\n"); > + return ret; > + } > + > + /* And now the replica */ > + ret = gpt_write_metadata_partition(desc, metadata, secondary_mpart); > + if (ret < 0) { > + log_err("Updating secondary metadata partition failed\n"); > + return ret; > + } So shouldn't we do something about this case? The first partition was correctly written and the second failed. Now if the primary GPT somehow gets corrupted the device is now unusable. I assume that depending on what happened to the firmware rollback counter, we could either try to rewrite this, or revert the first one as well (assuming rollback counters allow that). > + > + return 0; > +} > + > +static int gpt_get_valid_metadata(struct fwu_metadata **metadata) > +{ > + int ret; > + struct blk_desc *desc; > + u32 primary_mpart, secondary_mpart; > + > + ret = fwu_plat_get_blk_desc(&desc); > + if (ret < 0) { > + log_err("Block device not found\n"); > + return -ENODEV; > + } > + > + primary_mpart = secondary_mpart = 0; > + ret = gpt_get_metadata_partitions(desc, &primary_mpart, > + &secondary_mpart); > + > + if (ret < 0) { > + log_err("Error getting the metadata partitions\n"); > + return -ENODEV; > + } > + > + *metadata = malloc(sizeof(struct fwu_metadata)); > + if (!*metadata) { > + log_err("Unable to allocate memory for reading metadata\n"); > + return -ENOMEM; > + } > + > + ret = gpt_read_metadata(desc, *metadata, primary_mpart); > + if (ret < 0) { > + log_err("Failed to read the metadata from the device\n"); > + return -EIO; > + } > + > + ret = gpt_verify_metadata(*metadata, 1); > + if (!ret) > + return 0; > + > + /* > + * Verification of the primary metadata copy failed. > + * Try to read the replica. > + */ > + memset(*metadata, 0, sizeof(struct fwu_metadata)); > + ret = gpt_read_metadata(desc, *metadata, secondary_mpart); > + if (ret < 0) { > + log_err("Failed to read the metadata from the device\n"); > + return -EIO; > + } > + > + ret = gpt_verify_metadata(*metadata, 0); > + if (!ret) > + return 0; > + > + /* Both the metadata copies are corrupted. */ > + return -1; > +} > + > +static int gpt_check_metadata_validity(void) > +{ > + int ret; > + struct blk_desc *desc; > + struct fwu_metadata *pri_metadata; > + struct fwu_metadata *secondary_metadata; init those to NULL so you can goto out and free pri_metadata/secondary_metadata unconditionally But do you really need a pointer here? Can't this just be struct fwu_metadata pri_metadata, secondary_metadata;? > + u32 primary_mpart, secondary_mpart; > + u32 valid_partitions; u16 for both I guess? > + > + ret = fwu_plat_get_blk_desc(&desc); > + if (ret < 0) { > + log_err("Block device not found\n"); > + return -ENODEV; > + } > + > + /* > + * Two metadata partitions are expected. > + * If we don't have two, user needs to create > + * them first > + */ > + primary_mpart = secondary_mpart = 0; > + valid_partitions = 0; > + ret = gpt_get_metadata_partitions(desc, &primary_mpart, > + &secondary_mpart); > + > + if (ret < 0) { > + log_err("Error getting the metadata partitions\n"); > + return -ENODEV; > + } > + > + pri_metadata = malloc(sizeof(*pri_metadata)); > + if (!pri_metadata) { > + log_err("Unable to allocate memory for reading metadata\n"); > + return -ENOMEM; > + } > + > + secondary_metadata = malloc(sizeof(*secondary_metadata)); > + if (!secondary_metadata) { > + log_err("Unable to allocate memory for reading metadata\n"); > + return -ENOMEM; > + } > + > + ret = gpt_read_metadata(desc, pri_metadata, primary_mpart); > + if (ret < 0) { > + log_err("Failed to read the metadata from the device\n"); > + ret = -EIO; > + goto out; It doesn't make sense to exit here without checking the secondary partition. > + } > + > + ret = gpt_verify_metadata(pri_metadata, 1); > + if (!ret) > + valid_partitions |= PRIMARY_VALID; > + > + /* Now check the secondary partition */ > + ret = gpt_read_metadata(desc, secondary_metadata, secondary_mpart); > + if (ret < 0) { > + log_err("Failed to read the metadata from the device\n"); > + ret = -EIO; Ditto, if the first is valid we can still rescue that. > + goto out; > + } > + > + ret = gpt_verify_metadata(secondary_metadata, 0); > + if (!ret) > + valid_partitions |= SECONDARY_VALID; > + > + if (valid_partitions == (PRIMARY_VALID | SECONDARY_VALID)) { > + ret = -1; > + /* > + * Before returning, check that both the > + * metadata copies are the same. If not, > + * the metadata copies need to be > + * re-populated. > + */ > + if (!memcmp(pri_metadata, secondary_metadata, > + sizeof(*pri_metadata))) > + ret = 0; Is anyone else copying the metadata if this fails? In that case would it make sense to just copy pri_metadata-> secondary_metadata here and sync them up? > + goto out; > + } else if (valid_partitions == SECONDARY_VALID) { > + ret = gpt_write_metadata_partition(desc, secondary_metadata, > + primary_mpart); > + if (ret < 0) { > + log_err("Restoring primary metadata partition failed\n"); > + goto out; > + } > + } else if (valid_partitions == PRIMARY_VALID) { > + ret = gpt_write_metadata_partition(desc, pri_metadata, > + secondary_mpart); > + if (ret < 0) { > + log_err("Restoring secondary metadata partition failed\n"); > + goto out; > + } > + } else { > + ret = -1; > + } I would write this whole if a bit differently. Since you have the valid partitions in a bitmap. redefine your original definitions like #define PRIMARY_VALID BIT(0) #define SECONDARY_VALID BIT(1) #define BOTH_VALID (PRIMARY_VALID | SECONDARY_VALID) if (!(valid_partitions & BOTH_VALID)) goto out; wrong = valid_partitions ^ BOTH_VALID; if (!out) <both valid code> else <'wrong' is the number of invalid partition now> gpt_write_metadata_partition(desc, (wrong == PRIMARY_VALID) ? secondary_metadata : pri_metadata), (wrong == PRIMARY_VALID) ? primary_mpart : secondary_mpart) > + > +out: > + free(pri_metadata); secondary_metadata needs freeing as well if you keep the ptrs > + > + return ret; > +} > + > +static int gpt_fill_partition_guid_array(struct blk_desc *desc, > + efi_guid_t **part_guid_arr, > + u32 *nparts) > +{ > + int ret, i; > + u32 parts; > + gpt_entry *gpt_pte = NULL; > + const efi_guid_t null_guid = NULL_GUID; > + > + ALLOC_CACHE_ALIGN_BUFFER_PAD(gpt_header, gpt_head, 1, > + desc->blksz); > + > + ret = get_gpt_hdr_parts(desc, gpt_head, &gpt_pte); > + if (ret < 0) { > + log_err("Error getting GPT header and partitions\n"); > + ret = -EIO; > + goto out; > + } > + > + *nparts = gpt_head->num_partition_entries; > + > + /* > + * There could be a scenario where the number of partition entries > + * configured in the GPT header is the default value of 128. Find > + * the actual number of populated partitioned entries > + */ > + for (i = 0, parts = 0; i < *nparts; i++) { Just init 'parts' on the declaration > + if (!guidcmp(&gpt_pte[i].partition_type_guid, &null_guid)) > + continue; > + ++parts; > + } > + > + *nparts = parts; > + *part_guid_arr = malloc(sizeof(efi_guid_t) * *nparts); > + if (!part_guid_arr) { > + log_err("Unable to allocate memory for guid array\n"); > + ret = -ENOMEM; > + goto out; > + } > + > + for (i = 0; i < *nparts; i++) { > + guidcpy((*part_guid_arr + i), > + &gpt_pte[i].partition_type_guid); > + } > + > +out: > + free(gpt_pte); > + return ret; > +} > + > +int fwu_gpt_get_active_index(u32 *active_idx) > +{ > + int ret; > + struct fwu_metadata *metadata; > + > + ret = gpt_get_valid_metadata(&metadata); > + if (ret < 0) { > + log_err("Unable to get valid metadata\n"); > + goto out; > + } > + > + /* > + * Found the metadata partition, now read the active_index > + * value > + */ > + *active_idx = metadata->active_index; > + if (*active_idx > CONFIG_FWU_NUM_BANKS - 1) { > + printf("Active index value read is incorrect\n"); > + ret = -EINVAL; > + goto out; > + } > + > +out: > + free(metadata); > + > + return ret; > +} > + > +static int gpt_get_image_alt_num(struct blk_desc *desc, > + efi_guid_t image_type_id, > + u32 update_bank, int *alt_no) > +{ > + int ret, i; > + u32 nparts; > + gpt_entry *gpt_pte = NULL; > + struct fwu_metadata *metadata; > + struct fwu_image_entry *img_entry; > + struct fwu_image_bank_info *img_bank_info; > + efi_guid_t unique_part_guid; > + efi_guid_t image_guid = NULL_GUID; > + > + ALLOC_CACHE_ALIGN_BUFFER_PAD(gpt_header, gpt_head, 1, > + desc->blksz); > + > + ret = gpt_get_valid_metadata(&metadata); > + if (ret < 0) { > + log_err("Unable to read valid metadata\n"); > + goto out; > + } > + > + /* > + * The metadata has been read. Now get the image_uuid for the > + * image with the update_bank. > + */ > + for (i = 0; i < CONFIG_FWU_NUM_IMAGES_PER_BANK; i++) { > + if (!guidcmp(&image_type_id, > + &metadata->img_entry[i].image_type_uuid)) { > + img_entry = &metadata->img_entry[i]; > + img_bank_info = &img_entry->img_bank_info[update_bank]; > + guidcpy(&image_guid, &img_bank_info->image_uuid); break;? > + } > + } > + > + /* > + * Now read the GPT Partition Table Entries to find a matching > + * partition with UniquePartitionGuid value. We need to iterate > + * through all the GPT partitions since they might be in any > + * order > + */ > + ret = get_gpt_hdr_parts(desc, gpt_head, &gpt_pte); > + if (ret < 0) { > + log_err("Error getting GPT header and partitions\n"); > + ret = -EIO; > + goto out; > + } > + > + nparts = gpt_head->num_partition_entries; > + > + for (i = 0; i < nparts; i++) { > + unique_part_guid = gpt_pte[i].unique_partition_guid; > + if (!guidcmp(&unique_part_guid, &image_guid)) { > + /* Found the partition */ > + *alt_no = i + 1; > + break; > + } > + } > + > + if (i == nparts) { > + log_err("Partition with the image guid not found\n"); > + ret = -EINVAL; > + } > + > +out: > + free(metadata); > + free(gpt_pte); > + return ret; > +} > + > +int fwu_gpt_update_active_index(u32 active_idx) > +{ > + int ret; > + void *buf; > + u32 cur_active_index; > + struct fwu_metadata *metadata; > + > + if (active_idx > CONFIG_FWU_NUM_BANKS) { > + printf("Active index value to be updated is incorrect\n"); > + return -1; > + } > + > + ret = gpt_get_valid_metadata(&metadata); > + if (ret < 0) { > + log_err("Unable to get valid metadata\n"); > + goto out; > + } > + > + /* > + * Update the active index and previous_active_index fields > + * in the metadata > + */ > + cur_active_index = metadata->active_index; > + metadata->active_index = active_idx; > + metadata->previous_active_index = cur_active_index; You don't need the cur_active_index, just swap the 2 lines above. metadata->previous_active_index = metadata->active_index; metadata->active_index = active_idx; > + > + /* > + * Calculate the crc32 for the updated metadata > + * and put the updated value in the metadata crc32 > + * field > + */ > + buf = &metadata->version; > + metadata->crc32 = crc32(0, buf, sizeof(*metadata) - sizeof(u32)); > + > + /* > + * Now write this updated metadata to both the > + * metadata partitions > + */ > + ret = gpt_update_metadata(metadata); > + if (ret < 0) { > + log_err("Failed to update metadata partitions\n"); > + ret = -EIO; > + } > + > +out: > + free(metadata); > + > + return ret; > +} > + > +int fwu_gpt_fill_partition_guid_array(efi_guid_t **part_guid_arr, u32 *nparts) > +{ > + 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 gpt_fill_partition_guid_array(desc, part_guid_arr, nparts); > +} > + > +int fwu_gpt_get_image_alt_num(efi_guid_t image_type_id, u32 update_bank, > + int *alt_no) > +{ > + 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 gpt_get_image_alt_num(desc, image_type_id, update_bank, alt_no); > +} > + > +int fwu_gpt_metadata_check(void) > +{ > + /* > + * Check if both the copies of the metadata are valid. > + * If one has gone bad, restore it from the other good > + * copy. > + */ > + return gpt_check_metadata_validity(); > +} > + > +int fwu_gpt_get_metadata(struct fwu_metadata **metadata) > +{ > + return gpt_get_valid_metadata(metadata); > +} > + > +int fwu_gpt_revert_boot_index(u32 *active_idx) > +{ > + int ret; > + void *buf; > + u32 cur_active_index; > + struct fwu_metadata *metadata; > + > + ret = gpt_get_valid_metadata(&metadata); > + if (ret < 0) { > + log_err("Unable to get valid metadata\n"); > + goto out; > + } > + > + /* > + * Swap the active index and previous_active_index fields > + * in the metadata > + */ > + cur_active_index = metadata->active_index; > + metadata->active_index = metadata->previous_active_index; > + metadata->previous_active_index = cur_active_index; Ditto, you don't need cur_active_index; > + *active_idx = metadata->active_index; > + > + /* > + * Calculate the crc32 for the updated metadata > + * and put the updated value in the metadata crc32 > + * field > + */ > + buf = &metadata->version; > + metadata->crc32 = crc32(0, buf, sizeof(*metadata) - sizeof(u32)); > + > + /* > + * Now write this updated metadata to both the > + * metadata partitions > + */ > + ret = gpt_update_metadata(metadata); > + if (ret < 0) { > + log_err("Failed to update metadata partitions\n"); > + ret = -EIO; > + } > + > +out: > + free(metadata); > + > + return ret; > +} > + > +static int fwu_gpt_set_clear_image_accept(efi_guid_t *img_type_id, > + u32 bank, u8 action) > +{ > + void *buf; > + int ret, i; > + u32 nimages; > + struct fwu_metadata *metadata; > + struct fwu_image_entry *img_entry; > + struct fwu_image_bank_info *img_bank_info; > + > + ret = gpt_get_valid_metadata(&metadata); > + if (ret < 0) { > + log_err("Unable to get valid metadata\n"); > + goto out; > + } > + > + if (action == IMAGE_ACCEPT_SET) > + bank = metadata->active_index; I think it's clearer if fwu_gpt_accept_image() / fwu_gpt_clear_accept_image() read the metadata themselves and pass them as a ptr. That would mean you also have the right bank number and you wont be needing this anymore. > + > + nimages = CONFIG_FWU_NUM_IMAGES_PER_BANK; > + img_entry = &metadata->img_entry[0]; > + for (i = 0; i < nimages; i++) { > + if (!guidcmp(&img_entry[i].image_type_uuid, img_type_id)) { > + img_bank_info = &img_entry[i].img_bank_info[bank]; > + if (action == IMAGE_ACCEPT_SET) > + img_bank_info->accepted |= FWU_IMAGE_ACCEPTED; Do you need to preserve existing bits on 'accepted' here? > + else > + img_bank_info->accepted = 0; > + > + buf = &metadata->version; > + metadata->crc32 = crc32(0, buf, sizeof(*metadata) - > + sizeof(u32)); > + > + ret = gpt_update_metadata(metadata); > + goto out; > + } > + } > + > + /* Image not found */ > + ret = -EINVAL; > + > +out: > + free(metadata); > + > + return ret; > +} > + > +int fwu_gpt_accept_image(efi_guid_t *img_type_id) > +{ > + return fwu_gpt_set_clear_image_accept(img_type_id, 0, > + IMAGE_ACCEPT_SET); > +} > + > +int fwu_gpt_clear_accept_image(efi_guid_t *img_type_id, u32 bank) > +{ > + return fwu_gpt_set_clear_image_accept(img_type_id, bank, > + IMAGE_ACCEPT_CLEAR); > +} > + > +struct fwu_metadata_ops fwu_gpt_blk_ops = { > + .get_active_index = fwu_gpt_get_active_index, > + .update_active_index = fwu_gpt_update_active_index, > + .fill_partition_guid_array = fwu_gpt_fill_partition_guid_array, > + .get_image_alt_num = fwu_gpt_get_image_alt_num, > + .metadata_check = fwu_gpt_metadata_check, > + .revert_boot_index = fwu_gpt_revert_boot_index, > + .set_accept_image = fwu_gpt_accept_image, > + .clear_accept_image = fwu_gpt_clear_accept_image, > + .get_metadata = fwu_gpt_get_metadata, > +}; > -- > 2.17.1 > Cheers /Ilias
Hi Sughosh, On Thu, 25 Nov 2021 at 00:13, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > In the FWU Multi Bank Update feature, the information about the > updatable images is stored as part of the metadata, on a separate > partition. Add functions for reading from and writing to the metadata > when the updatable images and the metadata are stored on a block > device which is formated with GPT based partition scheme. > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > --- > lib/fwu_updates/fwu_metadata_gpt_blk.c | 716 +++++++++++++++++++++++++ > 1 file changed, 716 insertions(+) > create mode 100644 lib/fwu_updates/fwu_metadata_gpt_blk.c Can we pick a better name than metadata? It is just so vague...everything is metadata. Regards, Simon
hi Ilias, On Wed, 1 Dec 2021 at 17:56, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > Hi Sughosh, > > On Thu, Nov 25, 2021 at 12:42:56PM +0530, Sughosh Ganu wrote: > > In the FWU Multi Bank Update feature, the information about the > > updatable images is stored as part of the metadata, on a separate > > partition. Add functions for reading from and writing to the metadata > > when the updatable images and the metadata are stored on a block > > device which is formated with GPT based partition scheme. > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > --- > > lib/fwu_updates/fwu_metadata_gpt_blk.c | 716 +++++++++++++++++++++++++ > > 1 file changed, 716 insertions(+) > > create mode 100644 lib/fwu_updates/fwu_metadata_gpt_blk.c > > > > +#define SECONDARY_VALID 0x2 > > > Change those to BIT(0), BIT(1) etc please > Will change. > > > + > > +#define MDATA_READ (u8)0x1 > > +#define MDATA_WRITE (u8)0x2 > > + > > +#define IMAGE_ACCEPT_SET (u8)0x1 > > +#define IMAGE_ACCEPT_CLEAR (u8)0x2 > > + > > +static int gpt_verify_metadata(struct fwu_metadata *metadata, bool > pri_part) > > +{ > > + u32 calc_crc32; > > + void *buf; > > + > > + buf = &metadata->version; > > + calc_crc32 = crc32(0, buf, sizeof(*metadata) - sizeof(u32)); > > + > > + if (calc_crc32 != metadata->crc32) { > > + log_err("crc32 check failed for %s metadata partition\n", > > + pri_part ? "primary" : "secondary"); > > I think we can rid of the 'bool pri_part'. It's only used for printing and > you can have more that 2 partitions anyway right? > We will have only two partitions for the metadata. But i think looking at it now, it is a bit fuzzy on which is the primary metadata partition and which is the secondary one. Can we instead print the UniquePartitionGUID of the metadata partition instead. That will help in identifying for which metadata copy the verification failed. Let me know your thoughts on this. > > + return -1; > > + } > > + > > + return 0; > > +} > > + > > +static int gpt_get_metadata_partitions(struct blk_desc *desc, > > > Please add a function descriptions (on following functions as well) > I have added the function descriptions in the fwu_metadata.c, where the api's are being defined. Do we need to add the descriptions for the functions in this file as well? > > > + u32 *primary_mpart, > > + u32 *secondary_mpart) > > u16 maybe? This is the number of gpt partitions with metadata right? Okay. > > > > +{ > > + int i, ret; > > + u32 nparts, mparts; > > + gpt_entry *gpt_pte = NULL; > > + const efi_guid_t fwu_metadata_guid = FWU_METADATA_GUID; > > + > > + ALLOC_CACHE_ALIGN_BUFFER_PAD(gpt_header, gpt_head, 1, > > + desc->blksz); > > + > > + ret = get_gpt_hdr_parts(desc, gpt_head, &gpt_pte); > > + if (ret < 0) { > > + log_err("Error getting GPT header and partitions\n"); > > + ret = -EIO; > > + goto out; > > + } > > + > > + nparts = gpt_head->num_partition_entries; > > + for (i = 0, mparts = 0; i < nparts; i++) { > > + if (!guidcmp(&fwu_metadata_guid, > > + &gpt_pte[i].partition_type_guid)) { > > + ++mparts; > > + if (!*primary_mpart) > > + *primary_mpart = i + 1; > > + else > > + *secondary_mpart = i + 1; > > + } > > + } > > + > > + if (mparts != 2) { > > + log_err("Expect two copies of the metadata instead of > %d\n", > > + mparts); > > + ret = -EINVAL; > > + } else { > > + ret = 0; > > + } > > +out: > > + free(gpt_pte); > > + > > + return ret; > > +} > > + > > +static int gpt_get_metadata_disk_part(struct blk_desc *desc, > > + struct disk_partition *info, > > + u32 part_num) > > +{ > > + int ret; > > + char *metadata_guid_str = "8a7a84a0-8387-40f6-ab41-a8b9a5a60d23"; > > + > > + ret = part_get_info(desc, part_num, info); > > + if (ret < 0) { > > + log_err("Unable to get the partition info for the metadata > part %d", > > + part_num); > > + return -1; > > + } > > + > > + /* Check that it is indeed the metadata partition */ > > + if (!strncmp(info->type_guid, metadata_guid_str, UUID_STR_LEN)) { > > + /* Found the metadata partition */ > > + return 0; > > + } > > + > > + return -1; > > +} > > + > > +static int gpt_read_write_metadata(struct blk_desc *desc, > > + struct fwu_metadata *metadata, > > + u8 access, u32 part_num) > > +{ > > + int ret; > > + u32 len, blk_start, blkcnt; > > + struct disk_partition info; > > + > > + ALLOC_CACHE_ALIGN_BUFFER_PAD(struct fwu_metadata, mdata, 1, > desc->blksz); > > + > > + ret = gpt_get_metadata_disk_part(desc, &info, part_num); > > + if (ret < 0) { > > + printf("Unable to get the metadata partition\n"); > > + return -ENODEV; > > + } > > + > > + len = sizeof(*metadata); > > + blkcnt = BLOCK_CNT(len, desc); > > + if (blkcnt > info.size) { > > + log_err("Block count exceeds metadata partition size\n"); > > + return -ERANGE; > > + } > > + > > + blk_start = info.start; > > + if (access == MDATA_READ) { > > + if (blk_dread(desc, blk_start, blkcnt, mdata) != blkcnt) { > > + log_err("Error reading metadata from the > device\n"); > > + return -EIO; > > + } > > + memcpy(metadata, mdata, sizeof(struct fwu_metadata)); > > + } else { > > + if (blk_dwrite(desc, blk_start, blkcnt, metadata) != > blkcnt) { > > + log_err("Error writing metadata to the device\n"); > > + return -EIO; > > + } > > + } > > + > > + return 0; > > +} > > + > > +static int gpt_read_metadata(struct blk_desc *desc, > > + struct fwu_metadata *metadata, u32 part_num) > > +{ > > + return gpt_read_write_metadata(desc, metadata, MDATA_READ, > part_num); > > +} > > + > > +static int gpt_write_metadata_partition(struct blk_desc *desc, > > + struct fwu_metadata *metadata, > > + u32 part_num) > > +{ > > + return gpt_read_write_metadata(desc, metadata, MDATA_WRITE, > part_num); > > +} > > + > > +static int gpt_update_metadata(struct fwu_metadata *metadata) > > +{ > > + int ret; > > + struct blk_desc *desc; > > + u32 primary_mpart, secondary_mpart; > > + > > + ret = fwu_plat_get_blk_desc(&desc); > > + if (ret < 0) { > > + log_err("Block device not found\n"); > > + return -ENODEV; > > + } > > + > > + primary_mpart = secondary_mpart = 0; > > + ret = gpt_get_metadata_partitions(desc, &primary_mpart, > > + &secondary_mpart); > > + > > + if (ret < 0) { > > + log_err("Error getting the metadata partitions\n"); > > + return -ENODEV; > > + } > > + > > + /* First write the primary partition*/ > > + ret = gpt_write_metadata_partition(desc, metadata, primary_mpart); > > + if (ret < 0) { > > + log_err("Updating primary metadata partition failed\n"); > > + return ret; > > + } > > + > > + /* And now the replica */ > > + ret = gpt_write_metadata_partition(desc, metadata, > secondary_mpart); > > + if (ret < 0) { > > + log_err("Updating secondary metadata partition failed\n"); > > + return ret; > > + } > > So shouldn't we do something about this case? The first partition was > correctly written and the second failed. Now if the primary GPT somehow > gets corrupted the device is now unusable. > I assume that depending on what happened to the firmware rollback counter, > we could either try to rewrite this, or revert the first one as well > (assuming rollback counters allow that). > Okay, although this might be indicative of some underlying hardware issue with the device, else this scenario should not play out. > > + > > + return 0; > > +} > > + > > +static int gpt_get_valid_metadata(struct fwu_metadata **metadata) > > +{ > > + int ret; > > + struct blk_desc *desc; > > + u32 primary_mpart, secondary_mpart; > > + > > + ret = fwu_plat_get_blk_desc(&desc); > > + if (ret < 0) { > > + log_err("Block device not found\n"); > > + return -ENODEV; > > + } > > + > > + primary_mpart = secondary_mpart = 0; > > + ret = gpt_get_metadata_partitions(desc, &primary_mpart, > > + &secondary_mpart); > > + > > + if (ret < 0) { > > + log_err("Error getting the metadata partitions\n"); > > + return -ENODEV; > > + } > > + > > + *metadata = malloc(sizeof(struct fwu_metadata)); > > + if (!*metadata) { > > + log_err("Unable to allocate memory for reading > metadata\n"); > > + return -ENOMEM; > > + } > > + > > + ret = gpt_read_metadata(desc, *metadata, primary_mpart); > > + if (ret < 0) { > > + log_err("Failed to read the metadata from the device\n"); > > + return -EIO; > > + } > > + > > + ret = gpt_verify_metadata(*metadata, 1); > > + if (!ret) > > + return 0; > > + > > + /* > > + * Verification of the primary metadata copy failed. > > + * Try to read the replica. > > + */ > > + memset(*metadata, 0, sizeof(struct fwu_metadata)); > > + ret = gpt_read_metadata(desc, *metadata, secondary_mpart); > > + if (ret < 0) { > > + log_err("Failed to read the metadata from the device\n"); > > + return -EIO; > > + } > > + > > + ret = gpt_verify_metadata(*metadata, 0); > > + if (!ret) > > + return 0; > > + > > + /* Both the metadata copies are corrupted. */ > > + return -1; > > +} > > + > > +static int gpt_check_metadata_validity(void) > > +{ > > + int ret; > > + struct blk_desc *desc; > > + struct fwu_metadata *pri_metadata; > > + struct fwu_metadata *secondary_metadata; > > init those to NULL so you can goto out and free > pri_metadata/secondary_metadata unconditionally > But do you really need a pointer here? Can't this just be > struct fwu_metadata pri_metadata, secondary_metadata;? > Yes, these can be declared as local variables, and that will get rid of the malloc. Will change. > > + u32 primary_mpart, secondary_mpart; > > + u32 valid_partitions; > > u16 for both I guess? > Okay. > > > + > > + ret = fwu_plat_get_blk_desc(&desc); > > + if (ret < 0) { > > + log_err("Block device not found\n"); > > + return -ENODEV; > > + } > > + > > + /* > > + * Two metadata partitions are expected. > > + * If we don't have two, user needs to create > > + * them first > > + */ > > + primary_mpart = secondary_mpart = 0; > > + valid_partitions = 0; > > + ret = gpt_get_metadata_partitions(desc, &primary_mpart, > > + &secondary_mpart); > > + > > + if (ret < 0) { > > + log_err("Error getting the metadata partitions\n"); > > + return -ENODEV; > > + } > > + > > + pri_metadata = malloc(sizeof(*pri_metadata)); > > + if (!pri_metadata) { > > + log_err("Unable to allocate memory for reading > metadata\n"); > > + return -ENOMEM; > > + } > > + > > + secondary_metadata = malloc(sizeof(*secondary_metadata)); > > + if (!secondary_metadata) { > > + log_err("Unable to allocate memory for reading > metadata\n"); > > + return -ENOMEM; > > + } > > + > > + ret = gpt_read_metadata(desc, pri_metadata, primary_mpart); > > + if (ret < 0) { > > + log_err("Failed to read the metadata from the device\n"); > > + ret = -EIO; > > + goto out; > > It doesn't make sense to exit here without checking the secondary > partition. > Okay. Will revisit this logic. Although, this scenario should not play out unless there is some underlying issue with the device. But you are correct that if one of the metadata copies is valid, we can try restoring the other one. > > > + } > > + > > + ret = gpt_verify_metadata(pri_metadata, 1); > > + if (!ret) > > + valid_partitions |= PRIMARY_VALID; > > + > > + /* Now check the secondary partition */ > > + ret = gpt_read_metadata(desc, secondary_metadata, secondary_mpart); > > + if (ret < 0) { > > + log_err("Failed to read the metadata from the device\n"); > > + ret = -EIO; > > Ditto, if the first is valid we can still rescue that. > Okay. > > > + goto out; > > + } > > + > > + ret = gpt_verify_metadata(secondary_metadata, 0); > > + if (!ret) > > + valid_partitions |= SECONDARY_VALID; > > + > > + if (valid_partitions == (PRIMARY_VALID | SECONDARY_VALID)) { > > + ret = -1; > > + /* > > + * Before returning, check that both the > > + * metadata copies are the same. If not, > > + * the metadata copies need to be > > + * re-populated. > > + */ > > + if (!memcmp(pri_metadata, secondary_metadata, > > + sizeof(*pri_metadata))) > > + ret = 0; > > Is anyone else copying the metadata if this fails? In that case would it > make sense to just copy pri_metadata-> secondary_metadata here and sync > them up? > So this is a pretty fundamental error scenario where both metadata copies are valid, but are out of sync -- this should never happen. Will it be better instead to return an error and let the user check why this happened. > > + goto out; > > + } else if (valid_partitions == SECONDARY_VALID) { > > + ret = gpt_write_metadata_partition(desc, > secondary_metadata, > > + primary_mpart); > > + if (ret < 0) { > > + log_err("Restoring primary metadata partition > failed\n"); > > + goto out; > > + } > > + } else if (valid_partitions == PRIMARY_VALID) { > > + ret = gpt_write_metadata_partition(desc, pri_metadata, > > + secondary_mpart); > > + if (ret < 0) { > > + log_err("Restoring secondary metadata partition > failed\n"); > > + goto out; > > + } > > + } else { > > + ret = -1; > > + } > > I would write this whole if a bit differently. Since you have the valid > partitions in a bitmap. > redefine your original definitions like > > #define PRIMARY_VALID BIT(0) > #define SECONDARY_VALID BIT(1) > #define BOTH_VALID (PRIMARY_VALID | SECONDARY_VALID) > > if (!(valid_partitions & BOTH_VALID)) > goto out; > > wrong = valid_partitions ^ BOTH_VALID; > if (!out) > <both valid code> > else > <'wrong' is the number of invalid partition now> > gpt_write_metadata_partition(desc, > (wrong == > PRIMARY_VALID) ? secondary_metadata : pri_metadata), > (wrong == > PRIMARY_VALID) ? primary_mpart : secondary_mpart) > I will check with you on this offline. Am a little confused here :) > > > + > > +out: > > + free(pri_metadata); > > secondary_metadata needs freeing as well if you keep the ptrs > Yes, this is a remnant from my earlier implementation where i was allocating memory for both copies of metadata through a single call to malloc. But this will go away with declaration of local variables instead of malloc. > > > + > > + return ret; > > +} > > + > > +static int gpt_fill_partition_guid_array(struct blk_desc *desc, > > + efi_guid_t **part_guid_arr, > > + u32 *nparts) > > +{ > > + int ret, i; > > + u32 parts; > > + gpt_entry *gpt_pte = NULL; > > + const efi_guid_t null_guid = NULL_GUID; > > + > > + ALLOC_CACHE_ALIGN_BUFFER_PAD(gpt_header, gpt_head, 1, > > + desc->blksz); > > + > > + ret = get_gpt_hdr_parts(desc, gpt_head, &gpt_pte); > > + if (ret < 0) { > > + log_err("Error getting GPT header and partitions\n"); > > + ret = -EIO; > > + goto out; > > + } > > + > > + *nparts = gpt_head->num_partition_entries; > > + > > + /* > > + * There could be a scenario where the number of partition entries > > + * configured in the GPT header is the default value of 128. Find > > + * the actual number of populated partitioned entries > > + */ > > + for (i = 0, parts = 0; i < *nparts; i++) { > > Just init 'parts' on the declaration > Okay. > > > + if (!guidcmp(&gpt_pte[i].partition_type_guid, &null_guid)) > > + continue; > > + ++parts; > > + } > > + > > + *nparts = parts; > > + *part_guid_arr = malloc(sizeof(efi_guid_t) * *nparts); > > + if (!part_guid_arr) { > > + log_err("Unable to allocate memory for guid array\n"); > > + ret = -ENOMEM; > > + goto out; > > + } > > + > > + for (i = 0; i < *nparts; i++) { > > + guidcpy((*part_guid_arr + i), > > + &gpt_pte[i].partition_type_guid); > > + } > > + > > +out: > > + free(gpt_pte); > > + return ret; > > +} > > + > > +int fwu_gpt_get_active_index(u32 *active_idx) > > +{ > > + int ret; > > + struct fwu_metadata *metadata; > > + > > + ret = gpt_get_valid_metadata(&metadata); > > + if (ret < 0) { > > + log_err("Unable to get valid metadata\n"); > > + goto out; > > + } > > + > > + /* > > + * Found the metadata partition, now read the active_index > > + * value > > + */ > > + *active_idx = metadata->active_index; > > + if (*active_idx > CONFIG_FWU_NUM_BANKS - 1) { > > + printf("Active index value read is incorrect\n"); > > + ret = -EINVAL; > > + goto out; > > + } > > + > > +out: > > + free(metadata); > > + > > + return ret; > > +} > > + > > +static int gpt_get_image_alt_num(struct blk_desc *desc, > > + efi_guid_t image_type_id, > > + u32 update_bank, int *alt_no) > > +{ > > + int ret, i; > > + u32 nparts; > > + gpt_entry *gpt_pte = NULL; > > + struct fwu_metadata *metadata; > > + struct fwu_image_entry *img_entry; > > + struct fwu_image_bank_info *img_bank_info; > > + efi_guid_t unique_part_guid; > > + efi_guid_t image_guid = NULL_GUID; > > + > > + ALLOC_CACHE_ALIGN_BUFFER_PAD(gpt_header, gpt_head, 1, > > + desc->blksz); > > + > > + ret = gpt_get_valid_metadata(&metadata); > > + if (ret < 0) { > > + log_err("Unable to read valid metadata\n"); > > + goto out; > > + } > > + > > + /* > > + * The metadata has been read. Now get the image_uuid for the > > + * image with the update_bank. > > + */ > > + for (i = 0; i < CONFIG_FWU_NUM_IMAGES_PER_BANK; i++) { > > + if (!guidcmp(&image_type_id, > > + &metadata->img_entry[i].image_type_uuid)) { > > + img_entry = &metadata->img_entry[i]; > > + img_bank_info = > &img_entry->img_bank_info[update_bank]; > > + guidcpy(&image_guid, &img_bank_info->image_uuid); > > break;? > Okay. > > > + } > > + } > > + > > + /* > > + * Now read the GPT Partition Table Entries to find a matching > > + * partition with UniquePartitionGuid value. We need to iterate > > + * through all the GPT partitions since they might be in any > > + * order > > + */ > > + ret = get_gpt_hdr_parts(desc, gpt_head, &gpt_pte); > > + if (ret < 0) { > > + log_err("Error getting GPT header and partitions\n"); > > + ret = -EIO; > > + goto out; > > + } > > + > > + nparts = gpt_head->num_partition_entries; > > + > > + for (i = 0; i < nparts; i++) { > > + unique_part_guid = gpt_pte[i].unique_partition_guid; > > + if (!guidcmp(&unique_part_guid, &image_guid)) { > > + /* Found the partition */ > > + *alt_no = i + 1; > > + break; > > + } > > + } > > + > > + if (i == nparts) { > > + log_err("Partition with the image guid not found\n"); > > + ret = -EINVAL; > > + } > > + > > +out: > > + free(metadata); > > + free(gpt_pte); > > + return ret; > > +} > > + > > +int fwu_gpt_update_active_index(u32 active_idx) > > +{ > > + int ret; > > + void *buf; > > + u32 cur_active_index; > > + struct fwu_metadata *metadata; > > + > > + if (active_idx > CONFIG_FWU_NUM_BANKS) { > > + printf("Active index value to be updated is incorrect\n"); > > + return -1; > > + } > > + > > + ret = gpt_get_valid_metadata(&metadata); > > + if (ret < 0) { > > + log_err("Unable to get valid metadata\n"); > > + goto out; > > + } > > + > > + /* > > + * Update the active index and previous_active_index fields > > + * in the metadata > > + */ > > + cur_active_index = metadata->active_index; > > + metadata->active_index = active_idx; > > + metadata->previous_active_index = cur_active_index; > > You don't need the cur_active_index, just swap the 2 lines above. > metadata->previous_active_index = metadata->active_index; > metadata->active_index = active_idx; > Will change. > > > + > > + /* > > + * Calculate the crc32 for the updated metadata > > + * and put the updated value in the metadata crc32 > > + * field > > + */ > > + buf = &metadata->version; > > + metadata->crc32 = crc32(0, buf, sizeof(*metadata) - sizeof(u32)); > > + > > + /* > > + * Now write this updated metadata to both the > > + * metadata partitions > > + */ > > + ret = gpt_update_metadata(metadata); > > + if (ret < 0) { > > + log_err("Failed to update metadata partitions\n"); > > + ret = -EIO; > > + } > > + > > +out: > > + free(metadata); > > + > > + return ret; > > +} > > + > > +int fwu_gpt_fill_partition_guid_array(efi_guid_t **part_guid_arr, u32 > *nparts) > > +{ > > + 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 gpt_fill_partition_guid_array(desc, part_guid_arr, nparts); > > +} > > + > > +int fwu_gpt_get_image_alt_num(efi_guid_t image_type_id, u32 update_bank, > > + int *alt_no) > > +{ > > + 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 gpt_get_image_alt_num(desc, image_type_id, update_bank, > alt_no); > > +} > > + > > +int fwu_gpt_metadata_check(void) > > +{ > > + /* > > + * Check if both the copies of the metadata are valid. > > + * If one has gone bad, restore it from the other good > > + * copy. > > + */ > > + return gpt_check_metadata_validity(); > > +} > > + > > +int fwu_gpt_get_metadata(struct fwu_metadata **metadata) > > +{ > > + return gpt_get_valid_metadata(metadata); > > +} > > + > > +int fwu_gpt_revert_boot_index(u32 *active_idx) > > +{ > > + int ret; > > + void *buf; > > + u32 cur_active_index; > > + struct fwu_metadata *metadata; > > + > > + ret = gpt_get_valid_metadata(&metadata); > > + if (ret < 0) { > > + log_err("Unable to get valid metadata\n"); > > + goto out; > > + } > > + > > + /* > > + * Swap the active index and previous_active_index fields > > + * in the metadata > > + */ > > + cur_active_index = metadata->active_index; > > + metadata->active_index = metadata->previous_active_index; > > + metadata->previous_active_index = cur_active_index; > > Ditto, you don't need cur_active_index; > Will change. > > > + *active_idx = metadata->active_index; > > + > > + /* > > + * Calculate the crc32 for the updated metadata > > + * and put the updated value in the metadata crc32 > > + * field > > + */ > > + buf = &metadata->version; > > + metadata->crc32 = crc32(0, buf, sizeof(*metadata) - sizeof(u32)); > > + > > + /* > > + * Now write this updated metadata to both the > > + * metadata partitions > > + */ > > + ret = gpt_update_metadata(metadata); > > + if (ret < 0) { > > + log_err("Failed to update metadata partitions\n"); > > + ret = -EIO; > > + } > > + > > +out: > > + free(metadata); > > + > > + return ret; > > +} > > + > > +static int fwu_gpt_set_clear_image_accept(efi_guid_t *img_type_id, > > + u32 bank, u8 action) > > +{ > > + void *buf; > > + int ret, i; > > + u32 nimages; > > + struct fwu_metadata *metadata; > > + struct fwu_image_entry *img_entry; > > + struct fwu_image_bank_info *img_bank_info; > > + > > + ret = gpt_get_valid_metadata(&metadata); > > + if (ret < 0) { > > + log_err("Unable to get valid metadata\n"); > > + goto out; > > + } > > + > > + if (action == IMAGE_ACCEPT_SET) > > + bank = metadata->active_index; > > I think it's clearer if fwu_gpt_accept_image() / > fwu_gpt_clear_accept_image() read the metadata themselves and pass them as > a ptr. That would mean you also have the right bank number and you wont be > needing this anymore. > For clearing the accepted bit, the fwu_clear_accept_image function passes the updated bank as a parameter. We thus need to pass the bank as a parameter in any case. This would not be needed if the platform only has two banks, but would be needed if the number of banks is more than two. > > > + > > + nimages = CONFIG_FWU_NUM_IMAGES_PER_BANK; > > + img_entry = &metadata->img_entry[0]; > > + for (i = 0; i < nimages; i++) { > > + if (!guidcmp(&img_entry[i].image_type_uuid, img_type_id)) { > > + img_bank_info = &img_entry[i].img_bank_info[bank]; > > + if (action == IMAGE_ACCEPT_SET) > > + img_bank_info->accepted |= > FWU_IMAGE_ACCEPTED; > > Do you need to preserve existing bits on 'accepted' here? > The spec says that the Accepted field either should be 0 or 1. So this should be fine. -sughosh > > + else > > + img_bank_info->accepted = 0; > > + > > + buf = &metadata->version; > > + metadata->crc32 = crc32(0, buf, sizeof(*metadata) - > > + sizeof(u32)); > > + > > + ret = gpt_update_metadata(metadata); > > + goto out; > > + } > > + } > > + > > + /* Image not found */ > > + ret = -EINVAL; > > + > > +out: > > + free(metadata); > > + > > + return ret; > > +} > > + > > +int fwu_gpt_accept_image(efi_guid_t *img_type_id) > > +{ > > + return fwu_gpt_set_clear_image_accept(img_type_id, 0, > > + IMAGE_ACCEPT_SET); > > +} > > + > > +int fwu_gpt_clear_accept_image(efi_guid_t *img_type_id, u32 bank) > > +{ > > + return fwu_gpt_set_clear_image_accept(img_type_id, bank, > > + IMAGE_ACCEPT_CLEAR); > > +} > > + > > +struct fwu_metadata_ops fwu_gpt_blk_ops = { > > + .get_active_index = fwu_gpt_get_active_index, > > + .update_active_index = fwu_gpt_update_active_index, > > + .fill_partition_guid_array = fwu_gpt_fill_partition_guid_array, > > + .get_image_alt_num = fwu_gpt_get_image_alt_num, > > + .metadata_check = fwu_gpt_metadata_check, > > + .revert_boot_index = fwu_gpt_revert_boot_index, > > + .set_accept_image = fwu_gpt_accept_image, > > + .clear_accept_image = fwu_gpt_clear_accept_image, > > + .get_metadata = fwu_gpt_get_metadata, > > +}; > > -- > > 2.17.1 > > > > Cheers > /Ilias >
hi Simon, On Wed, 1 Dec 2021 at 23:32, Simon Glass <sjg@chromium.org> wrote: > Hi Sughosh, > > On Thu, 25 Nov 2021 at 00:13, Sughosh Ganu <sughosh.ganu@linaro.org> > wrote: > > > > In the FWU Multi Bank Update feature, the information about the > > updatable images is stored as part of the metadata, on a separate > > partition. Add functions for reading from and writing to the metadata > > when the updatable images and the metadata are stored on a block > > device which is formated with GPT based partition scheme. > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > --- > > lib/fwu_updates/fwu_metadata_gpt_blk.c | 716 +++++++++++++++++++++++++ > > 1 file changed, 716 insertions(+) > > create mode 100644 lib/fwu_updates/fwu_metadata_gpt_blk.c > > Can we pick a better name than metadata? It is just so > vague...everything is metadata. > The specification calls it metadata. And the functions in this file are for accessing the metadata on a GPT partitioned block device. Do you have any alternate name for this? -sughosh
Hi Sughosh, On Thu, 2 Dec 2021 at 01:05, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > hi Simon, > > On Wed, 1 Dec 2021 at 23:32, Simon Glass <sjg@chromium.org> wrote: >> >> Hi Sughosh, >> >> On Thu, 25 Nov 2021 at 00:13, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: >> > >> > In the FWU Multi Bank Update feature, the information about the >> > updatable images is stored as part of the metadata, on a separate >> > partition. Add functions for reading from and writing to the metadata >> > when the updatable images and the metadata are stored on a block >> > device which is formated with GPT based partition scheme. >> > >> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> >> > --- >> > lib/fwu_updates/fwu_metadata_gpt_blk.c | 716 +++++++++++++++++++++++++ >> > 1 file changed, 716 insertions(+) >> > create mode 100644 lib/fwu_updates/fwu_metadata_gpt_blk.c >> >> Can we pick a better name than metadata? It is just so >> vague...everything is metadata. > > > The specification calls it metadata. And the functions in this file are for accessing the metadata on a GPT partitioned block device. Do you have any alternate name for this? Well lots of things call their metadata "metadata". That doesn't mean it is a good name for the code base. Which specification? Do you have a link? Regards, Simon
hi Simon, On Thu, 2 Dec 2021 at 19:04, Simon Glass <sjg@chromium.org> wrote: > Hi Sughosh, > > On Thu, 2 Dec 2021 at 01:05, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > hi Simon, > > > > On Wed, 1 Dec 2021 at 23:32, Simon Glass <sjg@chromium.org> wrote: > >> > >> Hi Sughosh, > >> > >> On Thu, 25 Nov 2021 at 00:13, Sughosh Ganu <sughosh.ganu@linaro.org> > wrote: > >> > > >> > In the FWU Multi Bank Update feature, the information about the > >> > updatable images is stored as part of the metadata, on a separate > >> > partition. Add functions for reading from and writing to the metadata > >> > when the updatable images and the metadata are stored on a block > >> > device which is formated with GPT based partition scheme. > >> > > >> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > >> > --- > >> > lib/fwu_updates/fwu_metadata_gpt_blk.c | 716 > +++++++++++++++++++++++++ > >> > 1 file changed, 716 insertions(+) > >> > create mode 100644 lib/fwu_updates/fwu_metadata_gpt_blk.c > >> > >> Can we pick a better name than metadata? It is just so > >> vague...everything is metadata. > > > > > > The specification calls it metadata. And the functions in this file are > for accessing the metadata on a GPT partitioned block device. Do you have > any alternate name for this? > > Well lots of things call their metadata "metadata". That doesn't mean > it is a good name for the code base. Which specification? Do you have > a link? > I am referring to the FWU specification[1]. Please refer to section 4 in the document. -sughosh [1] - https://developer.arm.com/documentation/den0118/a
Hi Sughosh, Ilias (and all), On Thu, 2 Dec 2021 at 08:43, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > hi Ilias, > > On Wed, 1 Dec 2021 at 17:56, Ilias Apalodimas <ilias.apalodimas@linaro.org> > wrote: > > > Hi Sughosh, > > > > On Thu, Nov 25, 2021 at 12:42:56PM +0530, Sughosh Ganu wrote: > > > In the FWU Multi Bank Update feature, the information about the > > > updatable images is stored as part of the metadata, on a separate > > > partition. Add functions for reading from and writing to the metadata > > > when the updatable images and the metadata are stored on a block > > > device which is formated with GPT based partition scheme. > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > --- > > > lib/fwu_updates/fwu_metadata_gpt_blk.c | 716 +++++++++++++++++++++++++ > > > 1 file changed, 716 insertions(+) > > > create mode 100644 lib/fwu_updates/fwu_metadata_gpt_blk.c > > > > > > +#define SECONDARY_VALID 0x2 > > > > > > Change those to BIT(0), BIT(1) etc please > > > > Will change. > > > > > > > + > > > +#define MDATA_READ (u8)0x1 > > > +#define MDATA_WRITE (u8)0x2 > > > + > > > +#define IMAGE_ACCEPT_SET (u8)0x1 > > > +#define IMAGE_ACCEPT_CLEAR (u8)0x2 > > > + > > > +static int gpt_verify_metadata(struct fwu_metadata *metadata, bool > > pri_part) > > > +{ > > > + u32 calc_crc32; > > > + void *buf; > > > + > > > + buf = &metadata->version; > > > + calc_crc32 = crc32(0, buf, sizeof(*metadata) - sizeof(u32)); > > > + > > > + if (calc_crc32 != metadata->crc32) { > > > + log_err("crc32 check failed for %s metadata partition\n", > > > + pri_part ? "primary" : "secondary"); > > > > I think we can rid of the 'bool pri_part'. It's only used for printing and > > you can have more that 2 partitions anyway right? > > > > We will have only two partitions for the metadata. But i think looking at > it now, it is a bit fuzzy on which is the primary metadata partition and > which is the secondary one. Can we instead print the UniquePartitionGUID of > the metadata partition instead. That will help in identifying for which > metadata copy the verification failed. Let me know your thoughts on this. > > > > > + return -1; > > > + } > > > + > > > + return 0; > > > +} > > > + > > > +static int gpt_get_metadata_partitions(struct blk_desc *desc, > > > > > > Please add a function descriptions (on following functions as well) > > > > I have added the function descriptions in the fwu_metadata.c, where the > api's are being defined. Do we need to add the descriptions for the > functions in this file as well? > > > > > > > + u32 *primary_mpart, > > > + u32 *secondary_mpart) > > > > u16 maybe? This is the number of gpt partitions with metadata right? > > > Okay. > > > > > > > > > +{ > > > + int i, ret; > > > + u32 nparts, mparts; I think the 2 variables labels are too similar, it's a source of confusion. One is a number of entries, the second is a counter. It would be nice it's a bit more explicit. > > > + gpt_entry *gpt_pte = NULL; > > > + const efi_guid_t fwu_metadata_guid = FWU_METADATA_GUID; > > > + > > > + ALLOC_CACHE_ALIGN_BUFFER_PAD(gpt_header, gpt_head, 1, > > > + desc->blksz); > > > + > > > + ret = get_gpt_hdr_parts(desc, gpt_head, &gpt_pte); > > > + if (ret < 0) { > > > + log_err("Error getting GPT header and partitions\n"); > > > + ret = -EIO; > > > + goto out; > > > + } > > > + > > > + nparts = gpt_head->num_partition_entries; > > > + for (i = 0, mparts = 0; i < nparts; i++) { > > > + if (!guidcmp(&fwu_metadata_guid, > > > + &gpt_pte[i].partition_type_guid)) { > > > + ++mparts; > > > + if (!*primary_mpart) > > > + *primary_mpart = i + 1; > > > + else > > > + *secondary_mpart = i + 1; > > > + } > > > + } > > > + > > > + if (mparts != 2) { > > > + log_err("Expect two copies of the metadata instead of > > %d\n", > > > + mparts); > > > + ret = -EINVAL; > > > + } else { > > > + ret = 0; > > > + } > > > +out: > > > + free(gpt_pte); > > > + > > > + return ret; > > > +} > > > + > > > +static int gpt_get_metadata_disk_part(struct blk_desc *desc, > > > + struct disk_partition *info, > > > + u32 part_num) > > > +{ > > > + int ret; > > > + char *metadata_guid_str = "8a7a84a0-8387-40f6-ab41-a8b9a5a60d23"; > > > + > > > + ret = part_get_info(desc, part_num, info); > > > + if (ret < 0) { > > > + log_err("Unable to get the partition info for the metadata > > part %d", > > > + part_num); > > > + return -1; > > > + } > > > + > > > + /* Check that it is indeed the metadata partition */ > > > + if (!strncmp(info->type_guid, metadata_guid_str, UUID_STR_LEN)) { > > > + /* Found the metadata partition */ > > > + return 0; > > > + } > > > + > > > + return -1; > > > +} > > > + > > > +static int gpt_read_write_metadata(struct blk_desc *desc, > > > + struct fwu_metadata *metadata, > > > + u8 access, u32 part_num) > > > +{ > > > + int ret; > > > + u32 len, blk_start, blkcnt; > > > + struct disk_partition info; > > > + > > > + ALLOC_CACHE_ALIGN_BUFFER_PAD(struct fwu_metadata, mdata, 1, > > desc->blksz); > > > + > > > + ret = gpt_get_metadata_disk_part(desc, &info, part_num); > > > + if (ret < 0) { > > > + printf("Unable to get the metadata partition\n"); > > > + return -ENODEV; > > > + } > > > + > > > + len = sizeof(*metadata); > > > + blkcnt = BLOCK_CNT(len, desc); > > > + if (blkcnt > info.size) { > > > + log_err("Block count exceeds metadata partition size\n"); > > > + return -ERANGE; > > > + } > > > + > > > + blk_start = info.start; > > > + if (access == MDATA_READ) { > > > + if (blk_dread(desc, blk_start, blkcnt, mdata) != blkcnt) { > > > + log_err("Error reading metadata from the > > device\n"); > > > + return -EIO; > > > + } > > > + memcpy(metadata, mdata, sizeof(struct fwu_metadata)); > > > + } else { > > > + if (blk_dwrite(desc, blk_start, blkcnt, metadata) != > > blkcnt) { > > > + log_err("Error writing metadata to the device\n"); > > > + return -EIO; > > > + } > > > + } > > > + > > > + return 0; > > > +} > > > + > > > +static int gpt_read_metadata(struct blk_desc *desc, > > > + struct fwu_metadata *metadata, u32 part_num) > > > +{ > > > + return gpt_read_write_metadata(desc, metadata, MDATA_READ, > > part_num); > > > +} > > > + > > > +static int gpt_write_metadata_partition(struct blk_desc *desc, > > > + struct fwu_metadata *metadata, > > > + u32 part_num) > > > +{ > > > + return gpt_read_write_metadata(desc, metadata, MDATA_WRITE, > > part_num); > > > +} > > > + > > > +static int gpt_update_metadata(struct fwu_metadata *metadata) > > > +{ > > > + int ret; > > > + struct blk_desc *desc; > > > + u32 primary_mpart, secondary_mpart; > > > + > > > + ret = fwu_plat_get_blk_desc(&desc); > > > + if (ret < 0) { > > > + log_err("Block device not found\n"); > > > + return -ENODEV; > > > + } > > > + > > > + primary_mpart = secondary_mpart = 0; > > > + ret = gpt_get_metadata_partitions(desc, &primary_mpart, > > > + &secondary_mpart); > > > + > > > + if (ret < 0) { > > > + log_err("Error getting the metadata partitions\n"); > > > + return -ENODEV; > > > + } > > > + > > > + /* First write the primary partition*/ > > > + ret = gpt_write_metadata_partition(desc, metadata, primary_mpart); > > > + if (ret < 0) { > > > + log_err("Updating primary metadata partition failed\n"); > > > + return ret; > > > + } > > > + > > > + /* And now the replica */ > > > + ret = gpt_write_metadata_partition(desc, metadata, > > secondary_mpart); > > > + if (ret < 0) { > > > + log_err("Updating secondary metadata partition failed\n"); > > > + return ret; > > > + } > > > > So shouldn't we do something about this case? The first partition was > > correctly written and the second failed. Now if the primary GPT somehow > > gets corrupted the device is now unusable. The replica is not there to overcome bitflips of the storage media. It's here to allow updates while reliable info a still available in the counter part. The scheme could be to rely on only 1 instance of the fwu-metadata (sorry Simon) image is valid. A first load: load 1st instance, crap the second. At update: find the crapped one: write it with new data. Upon success crapped the alternate one. This is a suggestion. There are many ways to handle that. For sure, the scheme should be well defined so that the boot stage that read fwu-data complies with the scheme used to write them. > > I assume that depending on what happened to the firmware rollback counter, > > we could either try to rewrite this, or revert the first one as well > > (assuming rollback counters allow that). Rollback counters should protect image version management, not metadata updates (imho). > > > > Okay, although this might be indicative of some underlying hardware issue > with the device, else this scenario should not play out. > > > > > + > > > + return 0; > > > +} > > > + > > > +static int gpt_get_valid_metadata(struct fwu_metadata **metadata) Could be renamed gpt_get_metadata(), we don't expect to get invalid data :) > > > +{ > > > + int ret; > > > + struct blk_desc *desc; > > > + u32 primary_mpart, secondary_mpart; > > > + > > > + ret = fwu_plat_get_blk_desc(&desc); > > > + if (ret < 0) { > > > + log_err("Block device not found\n"); > > > + return -ENODEV; > > > + } > > > + > > > + primary_mpart = secondary_mpart = 0; Wouldn't it be better to have such default initialization values where those variables are defined? > > > + ret = gpt_get_metadata_partitions(desc, &primary_mpart, > > > + &secondary_mpart); > > > + > > > + if (ret < 0) { > > > + log_err("Error getting the metadata partitions\n"); > > > + return -ENODEV; > > > + } > > > + > > > + *metadata = malloc(sizeof(struct fwu_metadata)); > > > + if (!*metadata) { > > > + log_err("Unable to allocate memory for reading > > metadata\n"); > > > + return -ENOMEM; > > > + } > > > + > > > + ret = gpt_read_metadata(desc, *metadata, primary_mpart); > > > + if (ret < 0) { > > > + log_err("Failed to read the metadata from the device\n"); > > > + return -EIO; > > > + } > > > + > > > + ret = gpt_verify_metadata(*metadata, 1); > > > + if (!ret) > > > + return 0; > > > + > > > + /* > > > + * Verification of the primary metadata copy failed. > > > + * Try to read the replica. > > > + */ > > > + memset(*metadata, 0, sizeof(struct fwu_metadata)); > > > + ret = gpt_read_metadata(desc, *metadata, secondary_mpart); > > > + if (ret < 0) { > > > + log_err("Failed to read the metadata from the device\n"); > > > + return -EIO; > > > + } > > > + > > > + ret = gpt_verify_metadata(*metadata, 0); > > > + if (!ret) > > > + return 0; > > > + > > > + /* Both the metadata copies are corrupted. */ > > > + return -1; > > > +} > > > + > > > +static int gpt_check_metadata_validity(void) > > > +{ > > > + int ret; > > > + struct blk_desc *desc; > > > + struct fwu_metadata *pri_metadata; > > > + struct fwu_metadata *secondary_metadata; > > > > init those to NULL so you can goto out and free > > pri_metadata/secondary_metadata unconditionally > > But do you really need a pointer here? Can't this just be > > struct fwu_metadata pri_metadata, secondary_metadata;? > > > > Yes, these can be declared as local variables, and that will get rid of the > malloc. Will change. > > > > > + u32 primary_mpart, secondary_mpart; > > > + u32 valid_partitions; > > > > u16 for both I guess? > > > > Okay. unsigned int would do the work. No need for explicit byte-size type here, it doesn't add value. > > > > > > > + > > > + ret = fwu_plat_get_blk_desc(&desc); > > > + if (ret < 0) { > > > + log_err("Block device not found\n"); > > > + return -ENODEV; > > > + } > > > + > > > + /* > > > + * Two metadata partitions are expected. > > > + * If we don't have two, user needs to create > > > + * them first > > > + */ > > > + primary_mpart = secondary_mpart = 0; > > > + valid_partitions = 0; > > > + ret = gpt_get_metadata_partitions(desc, &primary_mpart, > > > + &secondary_mpart); > > > + > > > + if (ret < 0) { > > > + log_err("Error getting the metadata partitions\n"); > > > + return -ENODEV; > > > + } > > > + > > > + pri_metadata = malloc(sizeof(*pri_metadata)); > > > + if (!pri_metadata) { > > > + log_err("Unable to allocate memory for reading > > metadata\n"); > > > + return -ENOMEM; > > > + } > > > + > > > + secondary_metadata = malloc(sizeof(*secondary_metadata)); > > > + if (!secondary_metadata) { > > > + log_err("Unable to allocate memory for reading > > metadata\n"); > > > + return -ENOMEM; > > > + } > > > + > > > + ret = gpt_read_metadata(desc, pri_metadata, primary_mpart); > > > + if (ret < 0) { > > > + log_err("Failed to read the metadata from the device\n"); > > > + ret = -EIO; > > > + goto out; > > > > It doesn't make sense to exit here without checking the secondary > > partition. > > > > Okay. Will revisit this logic. Although, this scenario should not play out > unless there is some underlying issue with the device. But you are correct > that if one of the metadata copies is valid, we can try restoring the other > one. > > > > > > > + } > > > + > > > + ret = gpt_verify_metadata(pri_metadata, 1); > > > + if (!ret) > > > + valid_partitions |= PRIMARY_VALID; > > > + > > > + /* Now check the secondary partition */ > > > + ret = gpt_read_metadata(desc, secondary_metadata, secondary_mpart); > > > + if (ret < 0) { > > > + log_err("Failed to read the metadata from the device\n"); > > > + ret = -EIO; > > > > Ditto, if the first is valid we can still rescue that. > > > > Okay. > > > > > > > + goto out; > > > + } > > > + > > > + ret = gpt_verify_metadata(secondary_metadata, 0); > > > + if (!ret) > > > + valid_partitions |= SECONDARY_VALID; > > > + > > > + if (valid_partitions == (PRIMARY_VALID | SECONDARY_VALID)) { > > > + ret = -1; > > > + /* > > > + * Before returning, check that both the > > > + * metadata copies are the same. If not, > > > + * the metadata copies need to be > > > + * re-populated. > > > + */ > > > + if (!memcmp(pri_metadata, secondary_metadata, > > > + sizeof(*pri_metadata))) > > > + ret = 0; > > > > Is anyone else copying the metadata if this fails? In that case would it > > make sense to just copy pri_metadata-> secondary_metadata here and sync > > them up? > > > > So this is a pretty fundamental error scenario where both metadata copies > are valid, but are out of sync -- this should never happen. Will it be > better instead to return an error and let the user check why this happened. > > > > > + goto out; > > > + } else if (valid_partitions == SECONDARY_VALID) { > > > + ret = gpt_write_metadata_partition(desc, > > secondary_metadata, > > > + primary_mpart); > > > + if (ret < 0) { > > > + log_err("Restoring primary metadata partition > > failed\n"); > > > + goto out; > > > + } > > > + } else if (valid_partitions == PRIMARY_VALID) { > > > + ret = gpt_write_metadata_partition(desc, pri_metadata, > > > + secondary_mpart); > > > + if (ret < 0) { > > > + log_err("Restoring secondary metadata partition > > failed\n"); > > > + goto out; > > > + } > > > + } else { > > > + ret = -1; > > > + } > > > > I would write this whole if a bit differently. Since you have the valid > > partitions in a bitmap. > > redefine your original definitions like > > > > #define PRIMARY_VALID BIT(0) > > #define SECONDARY_VALID BIT(1) > > #define BOTH_VALID (PRIMARY_VALID | SECONDARY_VALID) > > > > if (!(valid_partitions & BOTH_VALID)) > > goto out; > > > > wrong = valid_partitions ^ BOTH_VALID; > > if (!out) > > <both valid code> > > else > > <'wrong' is the number of invalid partition now> > > gpt_write_metadata_partition(desc, > > (wrong == > > PRIMARY_VALID) ? secondary_metadata : pri_metadata), > > (wrong == > > PRIMARY_VALID) ? primary_mpart : secondary_mpart) > > > > I will check with you on this offline. Am a little confused here :) > > > > > > > + > > > +out: > > > + free(pri_metadata); > > > > secondary_metadata needs freeing as well if you keep the ptrs > > > > Yes, this is a remnant from my earlier implementation where i was > allocating memory for both copies of metadata through a single call to > malloc. But this will go away with declaration of local variables instead > of malloc. > > > > > > > + > > > + return ret; > > > +} > > > + > > > +static int gpt_fill_partition_guid_array(struct blk_desc *desc, > > > + efi_guid_t **part_guid_arr, > > > + u32 *nparts) > > > +{ > > > + int ret, i; > > > + u32 parts; > > > + gpt_entry *gpt_pte = NULL; > > > + const efi_guid_t null_guid = NULL_GUID; > > > + > > > + ALLOC_CACHE_ALIGN_BUFFER_PAD(gpt_header, gpt_head, 1, > > > + desc->blksz); > > > + > > > + ret = get_gpt_hdr_parts(desc, gpt_head, &gpt_pte); > > > + if (ret < 0) { > > > + log_err("Error getting GPT header and partitions\n"); > > > + ret = -EIO; > > > + goto out; > > > + } > > > + > > > + *nparts = gpt_head->num_partition_entries; > > > + > > > + /* > > > + * There could be a scenario where the number of partition entries > > > + * configured in the GPT header is the default value of 128. Find > > > + * the actual number of populated partitioned entries > > > + */ > > > + for (i = 0, parts = 0; i < *nparts; i++) { > > > > Just init 'parts' on the declaration > > > > Okay. > > > > > > > + if (!guidcmp(&gpt_pte[i].partition_type_guid, &null_guid)) > > > + continue; > > > + ++parts; > > > + } > > > + > > > + *nparts = parts; > > > + *part_guid_arr = malloc(sizeof(efi_guid_t) * *nparts); > > > + if (!part_guid_arr) { > > > + log_err("Unable to allocate memory for guid array\n"); > > > + ret = -ENOMEM; > > > + goto out; > > > + } > > > + > > > + for (i = 0; i < *nparts; i++) { > > > + guidcpy((*part_guid_arr + i), > > > + &gpt_pte[i].partition_type_guid); > > > + } > > > + > > > +out: > > > + free(gpt_pte); > > > + return ret; > > > +} > > > + > > > +int fwu_gpt_get_active_index(u32 *active_idx) > > > +{ > > > + int ret; > > > + struct fwu_metadata *metadata; > > > + > > > + ret = gpt_get_valid_metadata(&metadata); > > > + if (ret < 0) { > > > + log_err("Unable to get valid metadata\n"); > > > + goto out; > > > + } > > > + > > > + /* > > > + * Found the metadata partition, now read the active_index > > > + * value > > > + */ > > > + *active_idx = metadata->active_index; > > > + if (*active_idx > CONFIG_FWU_NUM_BANKS - 1) { > > > + printf("Active index value read is incorrect\n"); > > > + ret = -EINVAL; > > > + goto out; > > > + } > > > + > > > +out: > > > + free(metadata); > > > + > > > + return ret; > > > +} > > > + > > > +static int gpt_get_image_alt_num(struct blk_desc *desc, > > > + efi_guid_t image_type_id, > > > + u32 update_bank, int *alt_no) > > > +{ > > > + int ret, i; > > > + u32 nparts; > > > + gpt_entry *gpt_pte = NULL; > > > + struct fwu_metadata *metadata; > > > + struct fwu_image_entry *img_entry; > > > + struct fwu_image_bank_info *img_bank_info; > > > + efi_guid_t unique_part_guid; > > > + efi_guid_t image_guid = NULL_GUID; > > > + > > > + ALLOC_CACHE_ALIGN_BUFFER_PAD(gpt_header, gpt_head, 1, > > > + desc->blksz); > > > + > > > + ret = gpt_get_valid_metadata(&metadata); > > > + if (ret < 0) { > > > + log_err("Unable to read valid metadata\n"); > > > + goto out; > > > + } > > > + > > > + /* > > > + * The metadata has been read. Now get the image_uuid for the > > > + * image with the update_bank. > > > + */ > > > + for (i = 0; i < CONFIG_FWU_NUM_IMAGES_PER_BANK; i++) { > > > + if (!guidcmp(&image_type_id, > > > + &metadata->img_entry[i].image_type_uuid)) { > > > + img_entry = &metadata->img_entry[i]; > > > + img_bank_info = > > &img_entry->img_bank_info[update_bank]; > > > + guidcpy(&image_guid, &img_bank_info->image_uuid); > > > > break;? > > > > Okay. > > > > > > > + } > > > + } > > > + > > > + /* > > > + * Now read the GPT Partition Table Entries to find a matching > > > + * partition with UniquePartitionGuid value. We need to iterate > > > + * through all the GPT partitions since they might be in any > > > + * order > > > + */ > > > + ret = get_gpt_hdr_parts(desc, gpt_head, &gpt_pte); > > > + if (ret < 0) { > > > + log_err("Error getting GPT header and partitions\n"); > > > + ret = -EIO; > > > + goto out; > > > + } > > > + > > > + nparts = gpt_head->num_partition_entries; > > > + > > > + for (i = 0; i < nparts; i++) { > > > + unique_part_guid = gpt_pte[i].unique_partition_guid; > > > + if (!guidcmp(&unique_part_guid, &image_guid)) { > > > + /* Found the partition */ > > > + *alt_no = i + 1; > > > + break; > > > + } > > > + } > > > + > > > + if (i == nparts) { > > > + log_err("Partition with the image guid not found\n"); > > > + ret = -EINVAL; > > > + } > > > + > > > +out: > > > + free(metadata); > > > + free(gpt_pte); > > > + return ret; > > > +} > > > + > > > +int fwu_gpt_update_active_index(u32 active_idx) > > > +{ > > > + int ret; > > > + void *buf; > > > + u32 cur_active_index; > > > + struct fwu_metadata *metadata; > > > + > > > + if (active_idx > CONFIG_FWU_NUM_BANKS) { > > > + printf("Active index value to be updated is incorrect\n"); > > > + return -1; > > > + } > > > + > > > + ret = gpt_get_valid_metadata(&metadata); > > > + if (ret < 0) { > > > + log_err("Unable to get valid metadata\n"); > > > + goto out; > > > + } > > > + > > > + /* > > > + * Update the active index and previous_active_index fields > > > + * in the metadata > > > + */ > > > + cur_active_index = metadata->active_index; > > > + metadata->active_index = active_idx; > > > + metadata->previous_active_index = cur_active_index; > > > > You don't need the cur_active_index, just swap the 2 lines above. > > metadata->previous_active_index = metadata->active_index; > > metadata->active_index = active_idx; > > > > Will change. > > > > > > > + > > > + /* > > > + * Calculate the crc32 for the updated metadata > > > + * and put the updated value in the metadata crc32 > > > + * field > > > + */ > > > + buf = &metadata->version; > > > + metadata->crc32 = crc32(0, buf, sizeof(*metadata) - sizeof(u32)); > > > + > > > + /* > > > + * Now write this updated metadata to both the > > > + * metadata partitions > > > + */ > > > + ret = gpt_update_metadata(metadata); > > > + if (ret < 0) { > > > + log_err("Failed to update metadata partitions\n"); > > > + ret = -EIO; > > > + } > > > + > > > +out: > > > + free(metadata); > > > + > > > + return ret; > > > +} > > > + > > > +int fwu_gpt_fill_partition_guid_array(efi_guid_t **part_guid_arr, u32 > > *nparts) > > > +{ > > > + 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 gpt_fill_partition_guid_array(desc, part_guid_arr, nparts); > > > +} > > > + > > > +int fwu_gpt_get_image_alt_num(efi_guid_t image_type_id, u32 update_bank, > > > + int *alt_no) > > > +{ > > > + 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 gpt_get_image_alt_num(desc, image_type_id, update_bank, > > alt_no); > > > +} > > > + > > > +int fwu_gpt_metadata_check(void) > > > +{ > > > + /* > > > + * Check if both the copies of the metadata are valid. > > > + * If one has gone bad, restore it from the other good > > > + * copy. > > > + */ > > > + return gpt_check_metadata_validity(); > > > +} > > > + > > > +int fwu_gpt_get_metadata(struct fwu_metadata **metadata) > > > +{ > > > + return gpt_get_valid_metadata(metadata); > > > +} > > > + > > > +int fwu_gpt_revert_boot_index(u32 *active_idx) > > > +{ > > > + int ret; > > > + void *buf; > > > + u32 cur_active_index; > > > + struct fwu_metadata *metadata; > > > + > > > + ret = gpt_get_valid_metadata(&metadata); > > > + if (ret < 0) { > > > + log_err("Unable to get valid metadata\n"); > > > + goto out; > > > + } > > > + > > > + /* > > > + * Swap the active index and previous_active_index fields > > > + * in the metadata > > > + */ > > > + cur_active_index = metadata->active_index; > > > + metadata->active_index = metadata->previous_active_index; > > > + metadata->previous_active_index = cur_active_index; > > > > Ditto, you don't need cur_active_index; > > > > Will change. > > > > > > > + *active_idx = metadata->active_index; > > > + > > > + /* > > > + * Calculate the crc32 for the updated metadata > > > + * and put the updated value in the metadata crc32 > > > + * field > > > + */ > > > + buf = &metadata->version; > > > + metadata->crc32 = crc32(0, buf, sizeof(*metadata) - sizeof(u32)); > > > + > > > + /* > > > + * Now write this updated metadata to both the > > > + * metadata partitions > > > + */ > > > + ret = gpt_update_metadata(metadata); > > > + if (ret < 0) { > > > + log_err("Failed to update metadata partitions\n"); > > > + ret = -EIO; > > > + } > > > + > > > +out: > > > + free(metadata); > > > + > > > + return ret; > > > +} > > > + > > > +static int fwu_gpt_set_clear_image_accept(efi_guid_t *img_type_id, > > > + u32 bank, u8 action) > > > +{ > > > + void *buf; > > > + int ret, i; > > > + u32 nimages; > > > + struct fwu_metadata *metadata; > > > + struct fwu_image_entry *img_entry; > > > + struct fwu_image_bank_info *img_bank_info; > > > + > > > + ret = gpt_get_valid_metadata(&metadata); > > > + if (ret < 0) { > > > + log_err("Unable to get valid metadata\n"); > > > + goto out; > > > + } > > > + > > > + if (action == IMAGE_ACCEPT_SET) > > > + bank = metadata->active_index; > > > > I think it's clearer if fwu_gpt_accept_image() / > > fwu_gpt_clear_accept_image() read the metadata themselves and pass them as > > a ptr. That would mean you also have the right bank number and you wont be > > needing this anymore. > > > > For clearing the accepted bit, the fwu_clear_accept_image function passes > the updated bank as a parameter. We thus need to pass the bank as a > parameter in any case. This would not be needed if the platform only has > two banks, but would be needed if the number of banks is more than two. > > > > > > > + > > > + nimages = CONFIG_FWU_NUM_IMAGES_PER_BANK; > > > + img_entry = &metadata->img_entry[0]; > > > + for (i = 0; i < nimages; i++) { > > > + if (!guidcmp(&img_entry[i].image_type_uuid, img_type_id)) { > > > + img_bank_info = &img_entry[i].img_bank_info[bank]; > > > + if (action == IMAGE_ACCEPT_SET) > > > + img_bank_info->accepted |= > > FWU_IMAGE_ACCEPTED; > > > > Do you need to preserve existing bits on 'accepted' here? > > > > The spec says that the Accepted field either should be 0 or 1. So this > should be fine. > > -sughosh > > > > > + else > > > + img_bank_info->accepted = 0; > > > + > > > + buf = &metadata->version; > > > + metadata->crc32 = crc32(0, buf, sizeof(*metadata) - > > > + sizeof(u32)); > > > + > > > + ret = gpt_update_metadata(metadata); > > > + goto out; > > > + } > > > + } > > > + > > > + /* Image not found */ > > > + ret = -EINVAL; > > > + > > > +out: > > > + free(metadata); > > > + > > > + return ret; > > > +} > > > + > > > +int fwu_gpt_accept_image(efi_guid_t *img_type_id) > > > +{ > > > + return fwu_gpt_set_clear_image_accept(img_type_id, 0, > > > + IMAGE_ACCEPT_SET); > > > +} > > > + > > > +int fwu_gpt_clear_accept_image(efi_guid_t *img_type_id, u32 bank) > > > +{ > > > + return fwu_gpt_set_clear_image_accept(img_type_id, bank, > > > + IMAGE_ACCEPT_CLEAR); > > > +} > > > + > > > +struct fwu_metadata_ops fwu_gpt_blk_ops = { > > > + .get_active_index = fwu_gpt_get_active_index, > > > + .update_active_index = fwu_gpt_update_active_index, > > > + .fill_partition_guid_array = fwu_gpt_fill_partition_guid_array, > > > + .get_image_alt_num = fwu_gpt_get_image_alt_num, > > > + .metadata_check = fwu_gpt_metadata_check, > > > + .revert_boot_index = fwu_gpt_revert_boot_index, > > > + .set_accept_image = fwu_gpt_accept_image, > > > + .clear_accept_image = fwu_gpt_clear_accept_image, > > > + .get_metadata = fwu_gpt_get_metadata, > > > +}; > > > -- > > > 2.17.1 > > > > > > > Cheers > > /Ilias > >
Hi Etienne, On Wed, 8 Dec 2021 at 07:18, Etienne Carriere <etienne.carriere@linaro.org> wrote: > > Hi Sughosh, Ilias (and all), > > On Thu, 2 Dec 2021 at 08:43, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > hi Ilias, > > > > On Wed, 1 Dec 2021 at 17:56, Ilias Apalodimas <ilias.apalodimas@linaro.org> > > wrote: > > > > > Hi Sughosh, > > > > > > On Thu, Nov 25, 2021 at 12:42:56PM +0530, Sughosh Ganu wrote: > > > > In the FWU Multi Bank Update feature, the information about the > > > > updatable images is stored as part of the metadata, on a separate > > > > partition. Add functions for reading from and writing to the metadata > > > > when the updatable images and the metadata are stored on a block > > > > device which is formated with GPT based partition scheme. > > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > > --- > > > > lib/fwu_updates/fwu_metadata_gpt_blk.c | 716 +++++++++++++++++++++++++ > > > > 1 file changed, 716 insertions(+) > > > > create mode 100644 lib/fwu_updates/fwu_metadata_gpt_blk.c > > > > > > > > +#define SECONDARY_VALID 0x2 > > > > > > > > > Change those to BIT(0), BIT(1) etc please > > > > > > > Will change. > > > > > > > > > > > + > > > > +#define MDATA_READ (u8)0x1 > > > > +#define MDATA_WRITE (u8)0x2 > > > > + > > > > +#define IMAGE_ACCEPT_SET (u8)0x1 > > > > +#define IMAGE_ACCEPT_CLEAR (u8)0x2 > > > > + > > > > +static int gpt_verify_metadata(struct fwu_metadata *metadata, bool > > > pri_part) > > > > +{ > > > > + u32 calc_crc32; > > > > + void *buf; > > > > + > > > > + buf = &metadata->version; > > > > + calc_crc32 = crc32(0, buf, sizeof(*metadata) - sizeof(u32)); > > > > + > > > > + if (calc_crc32 != metadata->crc32) { > > > > + log_err("crc32 check failed for %s metadata partition\n", > > > > + pri_part ? "primary" : "secondary"); > > > > > > I think we can rid of the 'bool pri_part'. It's only used for printing and > > > you can have more that 2 partitions anyway right? > > > > > > > We will have only two partitions for the metadata. But i think looking at > > it now, it is a bit fuzzy on which is the primary metadata partition and > > which is the secondary one. Can we instead print the UniquePartitionGUID of > > the metadata partition instead. That will help in identifying for which > > metadata copy the verification failed. Let me know your thoughts on this. > > > > > > > > + return -1; > > > > + } > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int gpt_get_metadata_partitions(struct blk_desc *desc, > > > > > > > > > Please add a function descriptions (on following functions as well) > > > > > > > I have added the function descriptions in the fwu_metadata.c, where the > > api's are being defined. Do we need to add the descriptions for the > > functions in this file as well? > > > > > > > > > > > + u32 *primary_mpart, > > > > + u32 *secondary_mpart) > > > > > > u16 maybe? This is the number of gpt partitions with metadata right? > > > > > > Okay. > > > > > > > > > > > > > > +{ > > > > + int i, ret; > > > > + u32 nparts, mparts; > > I think the 2 variables labels are too similar, it's a source of confusion. > One is a number of entries, the second is a counter. It would be nice > it's a bit more explicit. > > > > > + gpt_entry *gpt_pte = NULL; > > > > + const efi_guid_t fwu_metadata_guid = FWU_METADATA_GUID; > > > > + > > > > + ALLOC_CACHE_ALIGN_BUFFER_PAD(gpt_header, gpt_head, 1, > > > > + desc->blksz); > > > > + > > > > + ret = get_gpt_hdr_parts(desc, gpt_head, &gpt_pte); > > > > + if (ret < 0) { > > > > + log_err("Error getting GPT header and partitions\n"); > > > > + ret = -EIO; > > > > + goto out; > > > > + } > > > > + > > > > + nparts = gpt_head->num_partition_entries; > > > > + for (i = 0, mparts = 0; i < nparts; i++) { > > > > + if (!guidcmp(&fwu_metadata_guid, > > > > + &gpt_pte[i].partition_type_guid)) { > > > > + ++mparts; > > > > + if (!*primary_mpart) > > > > + *primary_mpart = i + 1; > > > > + else > > > > + *secondary_mpart = i + 1; > > > > + } > > > > + } > > > > + > > > > + if (mparts != 2) { > > > > + log_err("Expect two copies of the metadata instead of > > > %d\n", > > > > + mparts); > > > > + ret = -EINVAL; > > > > + } else { > > > > + ret = 0; > > > > + } > > > > +out: > > > > + free(gpt_pte); > > > > + > > > > + return ret; > > > > +} > > > > + > > > > +static int gpt_get_metadata_disk_part(struct blk_desc *desc, > > > > + struct disk_partition *info, > > > > + u32 part_num) > > > > +{ > > > > + int ret; > > > > + char *metadata_guid_str = "8a7a84a0-8387-40f6-ab41-a8b9a5a60d23"; > > > > + > > > > + ret = part_get_info(desc, part_num, info); > > > > + if (ret < 0) { > > > > + log_err("Unable to get the partition info for the metadata > > > part %d", > > > > + part_num); > > > > + return -1; > > > > + } > > > > + > > > > + /* Check that it is indeed the metadata partition */ > > > > + if (!strncmp(info->type_guid, metadata_guid_str, UUID_STR_LEN)) { > > > > + /* Found the metadata partition */ > > > > + return 0; > > > > + } > > > > + > > > > + return -1; > > > > +} > > > > + > > > > +static int gpt_read_write_metadata(struct blk_desc *desc, > > > > + struct fwu_metadata *metadata, > > > > + u8 access, u32 part_num) > > > > +{ > > > > + int ret; > > > > + u32 len, blk_start, blkcnt; > > > > + struct disk_partition info; > > > > + > > > > + ALLOC_CACHE_ALIGN_BUFFER_PAD(struct fwu_metadata, mdata, 1, > > > desc->blksz); > > > > + > > > > + ret = gpt_get_metadata_disk_part(desc, &info, part_num); > > > > + if (ret < 0) { > > > > + printf("Unable to get the metadata partition\n"); > > > > + return -ENODEV; > > > > + } > > > > + > > > > + len = sizeof(*metadata); > > > > + blkcnt = BLOCK_CNT(len, desc); > > > > + if (blkcnt > info.size) { > > > > + log_err("Block count exceeds metadata partition size\n"); > > > > + return -ERANGE; > > > > + } > > > > + > > > > + blk_start = info.start; > > > > + if (access == MDATA_READ) { > > > > + if (blk_dread(desc, blk_start, blkcnt, mdata) != blkcnt) { > > > > + log_err("Error reading metadata from the > > > device\n"); > > > > + return -EIO; > > > > + } > > > > + memcpy(metadata, mdata, sizeof(struct fwu_metadata)); > > > > + } else { > > > > + if (blk_dwrite(desc, blk_start, blkcnt, metadata) != > > > blkcnt) { > > > > + log_err("Error writing metadata to the device\n"); > > > > + return -EIO; > > > > + } > > > > + } > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int gpt_read_metadata(struct blk_desc *desc, > > > > + struct fwu_metadata *metadata, u32 part_num) > > > > +{ > > > > + return gpt_read_write_metadata(desc, metadata, MDATA_READ, > > > part_num); > > > > +} > > > > + > > > > +static int gpt_write_metadata_partition(struct blk_desc *desc, > > > > + struct fwu_metadata *metadata, > > > > + u32 part_num) > > > > +{ > > > > + return gpt_read_write_metadata(desc, metadata, MDATA_WRITE, > > > part_num); > > > > +} > > > > + > > > > +static int gpt_update_metadata(struct fwu_metadata *metadata) > > > > +{ > > > > + int ret; > > > > + struct blk_desc *desc; > > > > + u32 primary_mpart, secondary_mpart; > > > > + > > > > + ret = fwu_plat_get_blk_desc(&desc); > > > > + if (ret < 0) { > > > > + log_err("Block device not found\n"); > > > > + return -ENODEV; > > > > + } > > > > + > > > > + primary_mpart = secondary_mpart = 0; > > > > + ret = gpt_get_metadata_partitions(desc, &primary_mpart, > > > > + &secondary_mpart); > > > > + > > > > + if (ret < 0) { > > > > + log_err("Error getting the metadata partitions\n"); > > > > + return -ENODEV; > > > > + } > > > > + > > > > + /* First write the primary partition*/ > > > > + ret = gpt_write_metadata_partition(desc, metadata, primary_mpart); > > > > + if (ret < 0) { > > > > + log_err("Updating primary metadata partition failed\n"); > > > > + return ret; > > > > + } > > > > + > > > > + /* And now the replica */ > > > > + ret = gpt_write_metadata_partition(desc, metadata, > > > secondary_mpart); > > > > + if (ret < 0) { > > > > + log_err("Updating secondary metadata partition failed\n"); > > > > + return ret; > > > > + } > > > > > > So shouldn't we do something about this case? The first partition was > > > correctly written and the second failed. Now if the primary GPT somehow > > > gets corrupted the device is now unusable. > > The replica is not there to overcome bitflips of the storage media. > It's here to allow updates while reliable info a still available in > the counter part. > The scheme could be to rely on only 1 instance of the fwu-metadata > (sorry Simon) image is valid. > A first load: load 1st instance, crap the second. > At update: find the crapped one: write it with new data. Upon success > crapped the alternate one. > This is a suggestion. There are many ways to handle that. > > For sure, the scheme should be well defined so that the boot stage > that read fwu-data complies with the scheme used to write them. > > > > > > I assume that depending on what happened to the firmware rollback counter, > > > we could either try to rewrite this, or revert the first one as well > > > (assuming rollback counters allow that). > > Rollback counters should protect image version management, not > metadata updates (imho). > > > > > > > > Okay, although this might be indicative of some underlying hardware issue > > with the device, else this scenario should not play out. > > > > > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int gpt_get_valid_metadata(struct fwu_metadata **metadata) > > Could be renamed gpt_get_metadata(), we don't expect to get invalid data :) How about gpt_get_mdata() ? When I read this I thought it was getting the GPT table...'metadata' is just too generic. Regards, Simon
Hi Etienne, [...] > > > > + > > > > + /* And now the replica */ > > > > + ret = gpt_write_metadata_partition(desc, metadata, > > > secondary_mpart); > > > > + if (ret < 0) { > > > > + log_err("Updating secondary metadata partition failed\n"); > > > > + return ret; > > > > + } > > > > > > So shouldn't we do something about this case? The first partition was > > > correctly written and the second failed. Now if the primary GPT somehow > > > gets corrupted the device is now unusable. > > The replica is not there to overcome bitflips of the storage media. > It's here to allow updates while reliable info a still available in > the counter part. Sure but with this piece of code this assumption is broken. At the point the secondary partition fails to write, you loose that reliability. When the next update happens you are left with one valid and one invalid partition of metadata. > The scheme could be to rely on only 1 instance of the fwu-metadata > (sorry Simon) image is valid. > A first load: load 1st instance, crap the second. > At update: find the crapped one: write it with new data. Upon success > crapped the alternate one. > This is a suggestion. There are many ways to handle that. We could change to something like that, however this is not what's currently happening. gpt_check_metadata_validity() is trying to check and make sure both of the partitions are sane. If they aren't it tries to recover those looking at a sane partition. So the question for really is, should we do something *here* or rely on the fact that the next update will try to fix the broken metadata. Cheers /Ilias
Hi Ilias, On Thu, 9 Dec 2021 at 08:37, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Etienne, > > > [...] > > > > > + > > > > > + /* And now the replica */ > > > > > + ret = gpt_write_metadata_partition(desc, metadata, > > > > secondary_mpart); > > > > > + if (ret < 0) { > > > > > + log_err("Updating secondary metadata partition failed\n"); > > > > > + return ret; > > > > > + } > > > > > > > > So shouldn't we do something about this case? The first partition was > > > > correctly written and the second failed. Now if the primary GPT somehow > > > > gets corrupted the device is now unusable. > > > > The replica is not there to overcome bitflips of the storage media. > > It's here to allow updates while reliable info a still available in > > the counter part. > > Sure but with this piece of code this assumption is broken. At the > point the secondary partition fails to write, you loose that > reliability. When the next update happens you are left with one valid > and one invalid partition of metadata. > > > The scheme could be to rely on only 1 instance of the fwu-metadata > > (sorry Simon) image is valid. > > A first load: load 1st instance, crap the second. > > At update: find the crapped one: write it with new data. Upon success > > crapped the alternate one. > > This is a suggestion. There are many ways to handle that. > > We could change to something like that, however this is not what's > currently happening. gpt_check_metadata_validity() is trying to check > and make sure both of the partitions are sane. If they aren't it > tries to recover those looking at a sane partition. So the question > for really is, should we do something *here* or rely on the fact that > the next update will try to fix the broken metadata. > > Cheers > /Ilias I think the right sequence would be to check if 1 of the 2 mdat partitions is broken, update that first and return an error on failure, then update the one sane and emit a warning on failure. Cheers, etienne
diff --git a/lib/fwu_updates/fwu_metadata_gpt_blk.c b/lib/fwu_updates/fwu_metadata_gpt_blk.c new file mode 100644 index 0000000000..98cc53f706 --- /dev/null +++ b/lib/fwu_updates/fwu_metadata_gpt_blk.c @@ -0,0 +1,716 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Copyright (c) 2021, Linaro Limited + */ + +#include <blk.h> +#include <efi_loader.h> +#include <fwu_metadata.h> +#include <malloc.h> +#include <memalign.h> +#include <part.h> +#include <part_efi.h> + +#include <linux/errno.h> +#include <linux/types.h> +#include <u-boot/crc.h> + +#define PRIMARY_VALID 0x1 +#define SECONDARY_VALID 0x2 + +#define MDATA_READ (u8)0x1 +#define MDATA_WRITE (u8)0x2 + +#define IMAGE_ACCEPT_SET (u8)0x1 +#define IMAGE_ACCEPT_CLEAR (u8)0x2 + +static int gpt_verify_metadata(struct fwu_metadata *metadata, bool pri_part) +{ + u32 calc_crc32; + void *buf; + + buf = &metadata->version; + calc_crc32 = crc32(0, buf, sizeof(*metadata) - sizeof(u32)); + + if (calc_crc32 != metadata->crc32) { + log_err("crc32 check failed for %s metadata partition\n", + pri_part ? "primary" : "secondary"); + return -1; + } + + return 0; +} + +static int gpt_get_metadata_partitions(struct blk_desc *desc, + u32 *primary_mpart, + u32 *secondary_mpart) +{ + int i, ret; + u32 nparts, mparts; + gpt_entry *gpt_pte = NULL; + const efi_guid_t fwu_metadata_guid = FWU_METADATA_GUID; + + ALLOC_CACHE_ALIGN_BUFFER_PAD(gpt_header, gpt_head, 1, + desc->blksz); + + ret = get_gpt_hdr_parts(desc, gpt_head, &gpt_pte); + if (ret < 0) { + log_err("Error getting GPT header and partitions\n"); + ret = -EIO; + goto out; + } + + nparts = gpt_head->num_partition_entries; + for (i = 0, mparts = 0; i < nparts; i++) { + if (!guidcmp(&fwu_metadata_guid, + &gpt_pte[i].partition_type_guid)) { + ++mparts; + if (!*primary_mpart) + *primary_mpart = i + 1; + else + *secondary_mpart = i + 1; + } + } + + if (mparts != 2) { + log_err("Expect two copies of the metadata instead of %d\n", + mparts); + ret = -EINVAL; + } else { + ret = 0; + } +out: + free(gpt_pte); + + return ret; +} + +static int gpt_get_metadata_disk_part(struct blk_desc *desc, + struct disk_partition *info, + u32 part_num) +{ + int ret; + char *metadata_guid_str = "8a7a84a0-8387-40f6-ab41-a8b9a5a60d23"; + + ret = part_get_info(desc, part_num, info); + if (ret < 0) { + log_err("Unable to get the partition info for the metadata part %d", + part_num); + return -1; + } + + /* Check that it is indeed the metadata partition */ + if (!strncmp(info->type_guid, metadata_guid_str, UUID_STR_LEN)) { + /* Found the metadata partition */ + return 0; + } + + return -1; +} + +static int gpt_read_write_metadata(struct blk_desc *desc, + struct fwu_metadata *metadata, + u8 access, u32 part_num) +{ + int ret; + u32 len, blk_start, blkcnt; + struct disk_partition info; + + ALLOC_CACHE_ALIGN_BUFFER_PAD(struct fwu_metadata, mdata, 1, desc->blksz); + + ret = gpt_get_metadata_disk_part(desc, &info, part_num); + if (ret < 0) { + printf("Unable to get the metadata partition\n"); + return -ENODEV; + } + + len = sizeof(*metadata); + blkcnt = BLOCK_CNT(len, desc); + if (blkcnt > info.size) { + log_err("Block count exceeds metadata partition size\n"); + return -ERANGE; + } + + blk_start = info.start; + if (access == MDATA_READ) { + if (blk_dread(desc, blk_start, blkcnt, mdata) != blkcnt) { + log_err("Error reading metadata from the device\n"); + return -EIO; + } + memcpy(metadata, mdata, sizeof(struct fwu_metadata)); + } else { + if (blk_dwrite(desc, blk_start, blkcnt, metadata) != blkcnt) { + log_err("Error writing metadata to the device\n"); + return -EIO; + } + } + + return 0; +} + +static int gpt_read_metadata(struct blk_desc *desc, + struct fwu_metadata *metadata, u32 part_num) +{ + return gpt_read_write_metadata(desc, metadata, MDATA_READ, part_num); +} + +static int gpt_write_metadata_partition(struct blk_desc *desc, + struct fwu_metadata *metadata, + u32 part_num) +{ + return gpt_read_write_metadata(desc, metadata, MDATA_WRITE, part_num); +} + +static int gpt_update_metadata(struct fwu_metadata *metadata) +{ + int ret; + struct blk_desc *desc; + u32 primary_mpart, secondary_mpart; + + ret = fwu_plat_get_blk_desc(&desc); + if (ret < 0) { + log_err("Block device not found\n"); + return -ENODEV; + } + + primary_mpart = secondary_mpart = 0; + ret = gpt_get_metadata_partitions(desc, &primary_mpart, + &secondary_mpart); + + if (ret < 0) { + log_err("Error getting the metadata partitions\n"); + return -ENODEV; + } + + /* First write the primary partition*/ + ret = gpt_write_metadata_partition(desc, metadata, primary_mpart); + if (ret < 0) { + log_err("Updating primary metadata partition failed\n"); + return ret; + } + + /* And now the replica */ + ret = gpt_write_metadata_partition(desc, metadata, secondary_mpart); + if (ret < 0) { + log_err("Updating secondary metadata partition failed\n"); + return ret; + } + + return 0; +} + +static int gpt_get_valid_metadata(struct fwu_metadata **metadata) +{ + int ret; + struct blk_desc *desc; + u32 primary_mpart, secondary_mpart; + + ret = fwu_plat_get_blk_desc(&desc); + if (ret < 0) { + log_err("Block device not found\n"); + return -ENODEV; + } + + primary_mpart = secondary_mpart = 0; + ret = gpt_get_metadata_partitions(desc, &primary_mpart, + &secondary_mpart); + + if (ret < 0) { + log_err("Error getting the metadata partitions\n"); + return -ENODEV; + } + + *metadata = malloc(sizeof(struct fwu_metadata)); + if (!*metadata) { + log_err("Unable to allocate memory for reading metadata\n"); + return -ENOMEM; + } + + ret = gpt_read_metadata(desc, *metadata, primary_mpart); + if (ret < 0) { + log_err("Failed to read the metadata from the device\n"); + return -EIO; + } + + ret = gpt_verify_metadata(*metadata, 1); + if (!ret) + return 0; + + /* + * Verification of the primary metadata copy failed. + * Try to read the replica. + */ + memset(*metadata, 0, sizeof(struct fwu_metadata)); + ret = gpt_read_metadata(desc, *metadata, secondary_mpart); + if (ret < 0) { + log_err("Failed to read the metadata from the device\n"); + return -EIO; + } + + ret = gpt_verify_metadata(*metadata, 0); + if (!ret) + return 0; + + /* Both the metadata copies are corrupted. */ + return -1; +} + +static int gpt_check_metadata_validity(void) +{ + int ret; + struct blk_desc *desc; + struct fwu_metadata *pri_metadata; + struct fwu_metadata *secondary_metadata; + u32 primary_mpart, secondary_mpart; + u32 valid_partitions; + + ret = fwu_plat_get_blk_desc(&desc); + if (ret < 0) { + log_err("Block device not found\n"); + return -ENODEV; + } + + /* + * Two metadata partitions are expected. + * If we don't have two, user needs to create + * them first + */ + primary_mpart = secondary_mpart = 0; + valid_partitions = 0; + ret = gpt_get_metadata_partitions(desc, &primary_mpart, + &secondary_mpart); + + if (ret < 0) { + log_err("Error getting the metadata partitions\n"); + return -ENODEV; + } + + pri_metadata = malloc(sizeof(*pri_metadata)); + if (!pri_metadata) { + log_err("Unable to allocate memory for reading metadata\n"); + return -ENOMEM; + } + + secondary_metadata = malloc(sizeof(*secondary_metadata)); + if (!secondary_metadata) { + log_err("Unable to allocate memory for reading metadata\n"); + return -ENOMEM; + } + + ret = gpt_read_metadata(desc, pri_metadata, primary_mpart); + if (ret < 0) { + log_err("Failed to read the metadata from the device\n"); + ret = -EIO; + goto out; + } + + ret = gpt_verify_metadata(pri_metadata, 1); + if (!ret) + valid_partitions |= PRIMARY_VALID; + + /* Now check the secondary partition */ + ret = gpt_read_metadata(desc, secondary_metadata, secondary_mpart); + if (ret < 0) { + log_err("Failed to read the metadata from the device\n"); + ret = -EIO; + goto out; + } + + ret = gpt_verify_metadata(secondary_metadata, 0); + if (!ret) + valid_partitions |= SECONDARY_VALID; + + if (valid_partitions == (PRIMARY_VALID | SECONDARY_VALID)) { + ret = -1; + /* + * Before returning, check that both the + * metadata copies are the same. If not, + * the metadata copies need to be + * re-populated. + */ + if (!memcmp(pri_metadata, secondary_metadata, + sizeof(*pri_metadata))) + ret = 0; + goto out; + } else if (valid_partitions == SECONDARY_VALID) { + ret = gpt_write_metadata_partition(desc, secondary_metadata, + primary_mpart); + if (ret < 0) { + log_err("Restoring primary metadata partition failed\n"); + goto out; + } + } else if (valid_partitions == PRIMARY_VALID) { + ret = gpt_write_metadata_partition(desc, pri_metadata, + secondary_mpart); + if (ret < 0) { + log_err("Restoring secondary metadata partition failed\n"); + goto out; + } + } else { + ret = -1; + } + +out: + free(pri_metadata); + + return ret; +} + +static int gpt_fill_partition_guid_array(struct blk_desc *desc, + efi_guid_t **part_guid_arr, + u32 *nparts) +{ + int ret, i; + u32 parts; + gpt_entry *gpt_pte = NULL; + const efi_guid_t null_guid = NULL_GUID; + + ALLOC_CACHE_ALIGN_BUFFER_PAD(gpt_header, gpt_head, 1, + desc->blksz); + + ret = get_gpt_hdr_parts(desc, gpt_head, &gpt_pte); + if (ret < 0) { + log_err("Error getting GPT header and partitions\n"); + ret = -EIO; + goto out; + } + + *nparts = gpt_head->num_partition_entries; + + /* + * There could be a scenario where the number of partition entries + * configured in the GPT header is the default value of 128. Find + * the actual number of populated partitioned entries + */ + for (i = 0, parts = 0; i < *nparts; i++) { + if (!guidcmp(&gpt_pte[i].partition_type_guid, &null_guid)) + continue; + ++parts; + } + + *nparts = parts; + *part_guid_arr = malloc(sizeof(efi_guid_t) * *nparts); + if (!part_guid_arr) { + log_err("Unable to allocate memory for guid array\n"); + ret = -ENOMEM; + goto out; + } + + for (i = 0; i < *nparts; i++) { + guidcpy((*part_guid_arr + i), + &gpt_pte[i].partition_type_guid); + } + +out: + free(gpt_pte); + return ret; +} + +int fwu_gpt_get_active_index(u32 *active_idx) +{ + int ret; + struct fwu_metadata *metadata; + + ret = gpt_get_valid_metadata(&metadata); + if (ret < 0) { + log_err("Unable to get valid metadata\n"); + goto out; + } + + /* + * Found the metadata partition, now read the active_index + * value + */ + *active_idx = metadata->active_index; + if (*active_idx > CONFIG_FWU_NUM_BANKS - 1) { + printf("Active index value read is incorrect\n"); + ret = -EINVAL; + goto out; + } + +out: + free(metadata); + + return ret; +} + +static int gpt_get_image_alt_num(struct blk_desc *desc, + efi_guid_t image_type_id, + u32 update_bank, int *alt_no) +{ + int ret, i; + u32 nparts; + gpt_entry *gpt_pte = NULL; + struct fwu_metadata *metadata; + struct fwu_image_entry *img_entry; + struct fwu_image_bank_info *img_bank_info; + efi_guid_t unique_part_guid; + efi_guid_t image_guid = NULL_GUID; + + ALLOC_CACHE_ALIGN_BUFFER_PAD(gpt_header, gpt_head, 1, + desc->blksz); + + ret = gpt_get_valid_metadata(&metadata); + if (ret < 0) { + log_err("Unable to read valid metadata\n"); + goto out; + } + + /* + * The metadata has been read. Now get the image_uuid for the + * image with the update_bank. + */ + for (i = 0; i < CONFIG_FWU_NUM_IMAGES_PER_BANK; i++) { + if (!guidcmp(&image_type_id, + &metadata->img_entry[i].image_type_uuid)) { + img_entry = &metadata->img_entry[i]; + img_bank_info = &img_entry->img_bank_info[update_bank]; + guidcpy(&image_guid, &img_bank_info->image_uuid); + } + } + + /* + * Now read the GPT Partition Table Entries to find a matching + * partition with UniquePartitionGuid value. We need to iterate + * through all the GPT partitions since they might be in any + * order + */ + ret = get_gpt_hdr_parts(desc, gpt_head, &gpt_pte); + if (ret < 0) { + log_err("Error getting GPT header and partitions\n"); + ret = -EIO; + goto out; + } + + nparts = gpt_head->num_partition_entries; + + for (i = 0; i < nparts; i++) { + unique_part_guid = gpt_pte[i].unique_partition_guid; + if (!guidcmp(&unique_part_guid, &image_guid)) { + /* Found the partition */ + *alt_no = i + 1; + break; + } + } + + if (i == nparts) { + log_err("Partition with the image guid not found\n"); + ret = -EINVAL; + } + +out: + free(metadata); + free(gpt_pte); + return ret; +} + +int fwu_gpt_update_active_index(u32 active_idx) +{ + int ret; + void *buf; + u32 cur_active_index; + struct fwu_metadata *metadata; + + if (active_idx > CONFIG_FWU_NUM_BANKS) { + printf("Active index value to be updated is incorrect\n"); + return -1; + } + + ret = gpt_get_valid_metadata(&metadata); + if (ret < 0) { + log_err("Unable to get valid metadata\n"); + goto out; + } + + /* + * Update the active index and previous_active_index fields + * in the metadata + */ + cur_active_index = metadata->active_index; + metadata->active_index = active_idx; + metadata->previous_active_index = cur_active_index; + + /* + * Calculate the crc32 for the updated metadata + * and put the updated value in the metadata crc32 + * field + */ + buf = &metadata->version; + metadata->crc32 = crc32(0, buf, sizeof(*metadata) - sizeof(u32)); + + /* + * Now write this updated metadata to both the + * metadata partitions + */ + ret = gpt_update_metadata(metadata); + if (ret < 0) { + log_err("Failed to update metadata partitions\n"); + ret = -EIO; + } + +out: + free(metadata); + + return ret; +} + +int fwu_gpt_fill_partition_guid_array(efi_guid_t **part_guid_arr, u32 *nparts) +{ + 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 gpt_fill_partition_guid_array(desc, part_guid_arr, nparts); +} + +int fwu_gpt_get_image_alt_num(efi_guid_t image_type_id, u32 update_bank, + int *alt_no) +{ + 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 gpt_get_image_alt_num(desc, image_type_id, update_bank, alt_no); +} + +int fwu_gpt_metadata_check(void) +{ + /* + * Check if both the copies of the metadata are valid. + * If one has gone bad, restore it from the other good + * copy. + */ + return gpt_check_metadata_validity(); +} + +int fwu_gpt_get_metadata(struct fwu_metadata **metadata) +{ + return gpt_get_valid_metadata(metadata); +} + +int fwu_gpt_revert_boot_index(u32 *active_idx) +{ + int ret; + void *buf; + u32 cur_active_index; + struct fwu_metadata *metadata; + + ret = gpt_get_valid_metadata(&metadata); + if (ret < 0) { + log_err("Unable to get valid metadata\n"); + goto out; + } + + /* + * Swap the active index and previous_active_index fields + * in the metadata + */ + cur_active_index = metadata->active_index; + metadata->active_index = metadata->previous_active_index; + metadata->previous_active_index = cur_active_index; + *active_idx = metadata->active_index; + + /* + * Calculate the crc32 for the updated metadata + * and put the updated value in the metadata crc32 + * field + */ + buf = &metadata->version; + metadata->crc32 = crc32(0, buf, sizeof(*metadata) - sizeof(u32)); + + /* + * Now write this updated metadata to both the + * metadata partitions + */ + ret = gpt_update_metadata(metadata); + if (ret < 0) { + log_err("Failed to update metadata partitions\n"); + ret = -EIO; + } + +out: + free(metadata); + + return ret; +} + +static int fwu_gpt_set_clear_image_accept(efi_guid_t *img_type_id, + u32 bank, u8 action) +{ + void *buf; + int ret, i; + u32 nimages; + struct fwu_metadata *metadata; + struct fwu_image_entry *img_entry; + struct fwu_image_bank_info *img_bank_info; + + ret = gpt_get_valid_metadata(&metadata); + if (ret < 0) { + log_err("Unable to get valid metadata\n"); + goto out; + } + + if (action == IMAGE_ACCEPT_SET) + bank = metadata->active_index; + + nimages = CONFIG_FWU_NUM_IMAGES_PER_BANK; + img_entry = &metadata->img_entry[0]; + for (i = 0; i < nimages; i++) { + if (!guidcmp(&img_entry[i].image_type_uuid, img_type_id)) { + img_bank_info = &img_entry[i].img_bank_info[bank]; + if (action == IMAGE_ACCEPT_SET) + img_bank_info->accepted |= FWU_IMAGE_ACCEPTED; + else + img_bank_info->accepted = 0; + + buf = &metadata->version; + metadata->crc32 = crc32(0, buf, sizeof(*metadata) - + sizeof(u32)); + + ret = gpt_update_metadata(metadata); + goto out; + } + } + + /* Image not found */ + ret = -EINVAL; + +out: + free(metadata); + + return ret; +} + +int fwu_gpt_accept_image(efi_guid_t *img_type_id) +{ + return fwu_gpt_set_clear_image_accept(img_type_id, 0, + IMAGE_ACCEPT_SET); +} + +int fwu_gpt_clear_accept_image(efi_guid_t *img_type_id, u32 bank) +{ + return fwu_gpt_set_clear_image_accept(img_type_id, bank, + IMAGE_ACCEPT_CLEAR); +} + +struct fwu_metadata_ops fwu_gpt_blk_ops = { + .get_active_index = fwu_gpt_get_active_index, + .update_active_index = fwu_gpt_update_active_index, + .fill_partition_guid_array = fwu_gpt_fill_partition_guid_array, + .get_image_alt_num = fwu_gpt_get_image_alt_num, + .metadata_check = fwu_gpt_metadata_check, + .revert_boot_index = fwu_gpt_revert_boot_index, + .set_accept_image = fwu_gpt_accept_image, + .clear_accept_image = fwu_gpt_clear_accept_image, + .get_metadata = fwu_gpt_get_metadata, +};
In the FWU Multi Bank Update feature, the information about the updatable images is stored as part of the metadata, on a separate partition. Add functions for reading from and writing to the metadata when the updatable images and the metadata are stored on a block device which is formated with GPT based partition scheme. Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> --- lib/fwu_updates/fwu_metadata_gpt_blk.c | 716 +++++++++++++++++++++++++ 1 file changed, 716 insertions(+) create mode 100644 lib/fwu_updates/fwu_metadata_gpt_blk.c