[RFC PATCH v2 2/8] FWU: Add FWU metadata access functions for GPT partitioned block devices

Masami Hiramatsu masami.hiramatsu at linaro.org
Sat Dec 25 07:57:46 CET 2021


Hi Sughosh,

2021年12月25日(土) 2:08 Sughosh Ganu <sughosh.ganu at linaro.org>:
>
> hi Masami,
>
> On Fri, 24 Dec 2021 at 15:29, Masami Hiramatsu
> <masami.hiramatsu at linaro.org> wrote:
> >
> > Hi Sughosh,
> >
> > 2021年12月19日(日) 16:06 Sughosh Ganu <sughosh.ganu at linaro.org>:
> >
> > > +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 part;
> > > +       struct fwu_mdata *mdata;
> > > +       struct fwu_image_entry *img_entry;
> > > +       struct fwu_image_bank_info *img_bank_info;
> > > +       struct disk_partition info;
> > > +       efi_guid_t unique_part_guid;
> > > +       efi_guid_t image_guid = NULL_GUID;
> > > +
> > > +       ret = gpt_get_mdata(&mdata);
> > > +       if (ret < 0) {
> > > +               log_err("Unable to read valid FWU metadata\n");
> > > +               goto out;
> > > +       }
> > > +
> > > +       /*
> > > +        * The FWU metadata has been read. Now get the image_uuid for the
> > > +        * image with the update_bank.
> > > +        */
> > > +       for (i = 0; i < CONFIG_FWU_NUM_IMAGES_PER_BANK; i++) {
> > > +               if (!guidcmp(&image_type_id,
> > > +                            &mdata->img_entry[i].image_type_uuid)) {
> > > +                       img_entry = &mdata->img_entry[i];
> > > +                       img_bank_info = &img_entry->img_bank_info[update_bank];
> > > +                       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
> > > +        */
> > > +       for (i = 1; i < MAX_SEARCH_PARTITIONS; i++) {
> > > +               if (part_get_info(desc, i, &info))
> > > +                       continue;
> > > +               uuid_str_to_bin(info.uuid, unique_part_guid.b,
> > > +                               UUID_STR_FORMAT_GUID);
> > > +
> > > +               if (!guidcmp(&unique_part_guid, &image_guid)) {
> > > +                       /* Found the partition */
> > > +                       part = i;
> > > +                       *alt_no = fwu_plat_get_alt_num(desc->devnum, &part);
> >
> > This is still GPT depending interface. In my use case (no GPT but raw SPI flash)
> > I don't have devnum. Moreover, this one is used only from the
> > fwu_mdata_gpt_blk.c.
>
> You are right. I will change this api to not have the first parameter.
> The function will then pass only the identifier which is a void *.

What about passing image index and bank index ? :-)

>
> > Please ensure (and comment) that this is only for GPT user.
> >
> > > +                       if (*alt_no != -1)
> > > +                               log_info("alt_num %d for partition %pUl\n",
> > > +                                         *alt_no, &image_guid);
> > > +                       ret = 0;
> > > +                       break;
> > > +               }
> > > +       }
> > > +
> > > +       if (*alt_no == -1) {
> > > +               log_err("alt_num not found for partition with GUID %pUl\n",
> > > +                       &image_guid);
> > > +               ret = -EINVAL;
> > > +       }
> > > +
> > > +       if (i == MAX_SEARCH_PARTITIONS) {
> > > +               log_err("Partition with the image guid not found\n");
> > > +               ret = -EINVAL;
> > > +       }
> > > +
> > > +out:
> > > +       free(mdata);
> > > +
> > > +       return ret;
> > > +}
> > > +
> > > +int fwu_gpt_update_active_index(u32 active_idx)
> > > +{
> > > +       int ret;
> > > +       void *buf;
> > > +       struct fwu_mdata *mdata;
> > > +
> > > +       if (active_idx > CONFIG_FWU_NUM_BANKS) {
> > > +               printf("Active index value to be updated is incorrect\n");
> > > +               return -1;
> > > +       }
> > > +
> > > +       ret = gpt_get_mdata(&mdata);
> > > +       if (ret < 0) {
> > > +               log_err("Unable to get valid FWU metadata\n");
> > > +               goto out;
> > > +       }
> > > +
> > > +       /*
> > > +        * Update the active index and previous_active_index fields
> > > +        * in the FWU metadata
> > > +        */
> > > +       mdata->previous_active_index = mdata->active_index;
> > > +       mdata->active_index = active_idx;
> > > +
> > > +
> > > +       /*
> > > +        * Calculate the crc32 for the updated FWU metadata
> > > +        * and put the updated value in the FWU metadata crc32
> > > +        * field
> > > +        */
> > > +       buf = &mdata->version;
> > > +       mdata->crc32 = crc32(0, buf, sizeof(*mdata) - sizeof(u32));
> > > +
> > > +       /*
> > > +        * Now write this updated FWU metadata to both the
> > > +        * FWU metadata partitions
> > > +        */
> > > +       ret = gpt_update_mdata(mdata);
> > > +       if (ret < 0) {
> > > +               log_err("Failed to update FWU metadata partitions\n");
> > > +               ret = -EIO;
> > > +       }
> > > +
> > > +out:
> > > +       free(mdata);
> > > +
> > > +       return ret;
> > > +}
> > > +
> > > +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_mdata_check(void)
> > > +{
> > > +       /*
> > > +        * Check if both the copies of the FWU metadata are
> > > +        * valid. If one has gone bad, restore it from the
> > > +        * other good copy.
> > > +        */
> > > +       return gpt_check_mdata_validity();
> > > +}
> > > +
> > > +int fwu_gpt_get_mdata(struct fwu_mdata **mdata)
> > > +{
> > > +       return gpt_get_mdata(mdata);
> > > +}
> > > +
> > > +int fwu_gpt_revert_boot_index(void)
> > > +{
> > > +       int ret;
> > > +       void *buf;
> > > +       u32 cur_active_index;
> > > +       struct fwu_mdata *mdata;
> > > +
> > > +       ret = gpt_get_mdata(&mdata);
> > > +       if (ret < 0) {
> > > +               log_err("Unable to get valid FWU metadata\n");
> > > +               goto out;
> > > +       }
> > > +
> > > +       /*
> > > +        * Swap the active index and previous_active_index fields
> > > +        * in the FWU metadata
> > > +        */
> > > +       cur_active_index = mdata->active_index;
> > > +       mdata->active_index = mdata->previous_active_index;
> > > +       mdata->previous_active_index = cur_active_index;
> > > +
> > > +       /*
> > > +        * Calculate the crc32 for the updated FWU metadata
> > > +        * and put the updated value in the FWU metadata crc32
> > > +        * field
> > > +        */
> > > +       buf = &mdata->version;
> > > +       mdata->crc32 = crc32(0, buf, sizeof(*mdata) - sizeof(u32));
> > > +
> > > +       /*
> > > +        * Now write this updated FWU metadata to both the
> > > +        * FWU metadata partitions
> > > +        */
> > > +       ret = gpt_update_mdata(mdata);
> >
> > If we have the fwu_update_mdata(), you can make this fwu_gpt_revert_boot_index()
> > and above fwu_gpt_update_boot_index() platform agnostic. (in that case
> > "gpt" must
> > be removed), because there is no dependency to GPT.
>
> Okay. I believe you are suggesting having a GPT specific
> implementation only for fwu_update_mdata, and move
> fwu_gpt_revert_boot_index and fwu_gpt_update_active_index as common
> functions with appropriate renaming. I can do that.

Yes, that's right. Then I don't need to repeat the same code :-)

Thank you,

>
> -sughosh
>
> >
> > > +       if (ret < 0) {
> > > +               log_err("Failed to update FWU metadata partitions\n");
> > > +               ret = -EIO;
> > > +       }
> > > +
> > > +out:
> > > +       free(mdata);
> > > +
> > > +       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_mdata *mdata;
> > > +       struct fwu_image_entry *img_entry;
> > > +       struct fwu_image_bank_info *img_bank_info;
> > > +
> > > +       ret = gpt_get_mdata(&mdata);
> > > +       if (ret < 0) {
> > > +               log_err("Unable to get valid FWU metadata\n");
> > > +               goto out;
> > > +       }
> > > +
> > > +       if (action == IMAGE_ACCEPT_SET)
> > > +               bank = mdata->active_index;
> > > +
> > > +       nimages = CONFIG_FWU_NUM_IMAGES_PER_BANK;
> > > +       img_entry = &mdata->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 = &mdata->version;
> > > +                       mdata->crc32 = crc32(0, buf, sizeof(*mdata) -
> > > +                                            sizeof(u32));
> > > +
> > > +                       ret = gpt_update_mdata(mdata);
> >
> > This function is also doing the generic things. Only gpt_update_mdata() is
> > the barrier. I would like to suggest you to add ".update_mdata" member
> > to the fwu_mdata_ops.
> >
> >
> > Thank you,
> >
> > > +                       goto out;
> > > +               }
> > > +       }
> > > +
> > > +       /* Image not found */
> > > +       ret = -EINVAL;
> > > +
> > > +out:
> > > +       free(mdata);
> > > +
> > > +       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_mdata_ops fwu_gpt_blk_ops = {
> > > +       .get_active_index = fwu_gpt_get_active_index,
> > > +       .update_active_index = fwu_gpt_update_active_index,
> > > +       .get_image_alt_num = fwu_gpt_get_image_alt_num,
> > > +       .mdata_check = fwu_gpt_mdata_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_mdata = fwu_gpt_get_mdata,
> > > +};
> > > --
> > > 2.17.1
> > >
> >
> >
> > --
> > Masami Hiramatsu



-- 
Masami Hiramatsu


More information about the U-Boot mailing list