[RESEND RFC PATCH 04/10] FWU: Add metadata access functions for GPT partitioned block devices
Sughosh Ganu
sughosh.ganu at linaro.org
Thu Dec 2 08:43:03 CET 2021
hi Ilias,
On Wed, 1 Dec 2021 at 17:56, Ilias Apalodimas <ilias.apalodimas at 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 at 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
>
More information about the U-Boot
mailing list