[PATCH v4 2/8] capsule: FMP: Populate the image descriptor array from platform data
Sughosh Ganu
sughosh.ganu at linaro.org
Thu Mar 31 18:29:41 CEST 2022
hi Ilias,
On Thu, 31 Mar 2022 at 20:38, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> Hi Sughosh,
>
> On Thu, Mar 31, 2022 at 06:57:44PM +0530, Sughosh Ganu wrote:
> > Currently, the image descriptor array that has been passed to the
> > GetImageInfo function of the Firmware Management Protocol(FMP) gets
> > populated through the data stored with the dfu framework. The
> > dfu data is not restricted to contain information only of the images
> > updatable through the capsule update mechanism, but it also contains
> > information on other images.
> >
> > The image descriptor array is also parsed by the ESRT generation code,
> > and thus the ESRT table contains entries for other images that are not
> > being handled by the FMP for the capsule updates.
> >
> > The other issue fixed is assignment of a separate GUID for all images
> > in the image descriptor array. The UEFI specification mandates that
> > all entries in the ESRT table should have a unique GUID value as part
> > of the FwClass member of the EFI_SYSTEM_RESOURCE_ENTRY. Currently, all
> > images are assigned a single GUID value, either an FIT GUID or a raw
> > image GUID. This is fixed by obtaining the GUID values from the
> > efi_fw_images array defined per platform.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu at linaro.org>
> > Reviewed-by: Masami Hiramatsu <masami.hiramatsu at linaro.org>
> > ---
> >
> > Changes since V3: None
> >
> > include/efi_loader.h | 3 ++
> > lib/efi_loader/efi_firmware.c | 91 +++++++++++------------------------
> > 2 files changed, 30 insertions(+), 64 deletions(-)
> >
> > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > index 284d64547b..9704397bd7 100644
> > --- a/include/efi_loader.h
> > +++ b/include/efi_loader.h
> > @@ -997,6 +997,9 @@ struct efi_fw_images {
> > u8 image_index;
> > };
> >
> > +extern struct efi_fw_images fw_images[];
> > +extern u8 num_image_type_guids;
> > +
> > /**
> > * Install the ESRT system table.
> > *
> > diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c
> > index a5ff32f121..169f3a29bb 100644
> > --- a/lib/efi_loader/efi_firmware.c
> > +++ b/lib/efi_loader/efi_firmware.c
> > @@ -97,91 +97,60 @@ efi_status_t EFIAPI efi_firmware_set_package_info_unsupported(
> > }
> >
> > /**
> > - * efi_get_dfu_info - return information about the current firmware image
> > + * efi_fill_image_desc_array - populate image descriptor array
> > * @this: Protocol instance
> > * @image_info_size: Size of @image_info
> > * @image_info: Image information
> > * @descriptor_version: Pointer to version number
> > - * @descriptor_count: Pointer to number of descriptors
> > + * @descriptor_count: Image count
> > * @descriptor_size: Pointer to descriptor size
> > - * package_version: Package version
> > - * package_version_name: Package version's name
> > - * image_type: Image type GUID
> > + * @package_version: Package version
> > + * @package_version_name: Package version's name
> > *
> > * Return information bout the current firmware image in @image_info.
> > * @image_info will consist of a number of descriptors.
> > - * Each descriptor will be created based on "dfu_alt_info" variable.
> > + * Each descriptor will be created based on "efi_fw_images" variable.
> > *
> > * Return status code
> > */
> > -static efi_status_t efi_get_dfu_info(
> > +static efi_status_t efi_fill_image_desc_array(
> > efi_uintn_t *image_info_size,
> > struct efi_firmware_image_descriptor *image_info,
> > u32 *descriptor_version,
> > u8 *descriptor_count,
> > efi_uintn_t *descriptor_size,
> > u32 *package_version,
> > - u16 **package_version_name,
> > - const efi_guid_t *image_type)
> > + u16 **package_version_name)
> > {
> > - struct dfu_entity *dfu;
> > size_t names_len, total_size;
> > - int dfu_num, i;
> > - u16 *name, *next;
> > - int ret;
> > -
> > - ret = dfu_init_env_entities(NULL, NULL);
> > - if (ret)
> > - return EFI_SUCCESS;
> > + struct efi_fw_images *fw_array;
> > + u8 image_count;
> > + int i;
> >
> > + fw_array = &fw_images[0];
>
> nit, but fw_array = fw_images looks better
Okay. Will change.
>
> > + *descriptor_count = image_count = num_image_type_guids;
>
> We could get rid of image_count here
Will remove.
>
> > names_len = 0;
> > - dfu_num = 0;
> > - list_for_each_entry(dfu, &dfu_list, list) {
> > - names_len += (utf8_utf16_strlen(dfu->name) + 1) * 2;
> > - dfu_num++;
> > - }
> > - if (!dfu_num) {
> > - log_warning("No entities in dfu_alt_info\n");
> > - *image_info_size = 0;
> > - dfu_free_entities();
> >
> > - return EFI_SUCCESS;
> > - }
> > + total_size = sizeof(*image_info) * image_count;
> >
> > - total_size = sizeof(*image_info) * dfu_num + names_len;
> > - /*
> > - * we will assume that sizeof(*image_info) * dfu_name
> > - * is, at least, a multiple of 2. So the start address for
> > - * image_id_name would be aligned with 2 bytes.
> > - */
> > if (*image_info_size < total_size) {
> > *image_info_size = total_size;
> > - dfu_free_entities();
> >
> > return EFI_BUFFER_TOO_SMALL;
> > }
> > *image_info_size = total_size;
> >
> > *descriptor_version = EFI_FIRMWARE_IMAGE_DESCRIPTOR_VERSION;
> > - *descriptor_count = dfu_num;
> > *descriptor_size = sizeof(*image_info);
> > *package_version = 0xffffffff; /* not supported */
> > *package_version_name = NULL; /* not supported */
> >
> > - /* DFU alt number should correspond to image_index */
> > - i = 0;
> > - /* Name area starts just after descriptors */
> > - name = (u16 *)((u8 *)image_info + sizeof(*image_info) * dfu_num);
> > - next = name;
> > - list_for_each_entry(dfu, &dfu_list, list) {
> > - image_info[i].image_index = dfu->alt + 1;
> > - image_info[i].image_type_id = *image_type;
> > - image_info[i].image_id = dfu->alt;
> > -
> > - /* copy the DFU entity name */
> > - utf8_utf16_strcpy(&next, dfu->name);
> > - image_info[i].image_id_name = name;
> > - name = ++next;
> > + for (i = 0; i < image_count; i++) {
> > + image_info[i].image_index = fw_array[i].image_index;
> > + image_info[i].image_type_id = fw_array[i].image_type_id;
> > + image_info[i].image_id = fw_array[i].image_index;
> > +
> > + image_info[i].image_id_name = (u16 *)fw_array[i].fw_name;
>
> We shouldn't be casting const qualifications away. In theory no one will
> even modify that in this case and nothing bad will happen. Since the efi
> spec defines the image_id_name as u16 * are we better of removing the const
> from efi_fw_images?
Sure. I will change the structure member to a u16 *. Thanks.
-sughosh
>
> Thanks
> /Ilias
> >
> > image_info[i].version = 0; /* not supported */
> > image_info[i].version_name = NULL; /* not supported */
> > @@ -202,12 +171,8 @@ static efi_status_t efi_get_dfu_info(
> > image_info[i].last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS;
> > image_info[i].hardware_instance = 1;
> > image_info[i].dependencies = NULL;
> > -
> > - i++;
> > }
> >
> > - dfu_free_entities();
> > -
> > return EFI_SUCCESS;
> > }
> >
> > @@ -267,11 +232,10 @@ efi_status_t EFIAPI efi_firmware_fit_get_image_info(
> > !descriptor_size || !package_version || !package_version_name))
> > return EFI_EXIT(EFI_INVALID_PARAMETER);
> >
> > - ret = efi_get_dfu_info(image_info_size, image_info,
> > - descriptor_version, descriptor_count,
> > - descriptor_size,
> > - package_version, package_version_name,
> > - &efi_firmware_image_type_uboot_fit);
> > + ret = efi_fill_image_desc_array(image_info_size, image_info,
> > + descriptor_version, descriptor_count,
> > + descriptor_size, package_version,
> > + package_version_name);
> >
> > return EFI_EXIT(ret);
> > }
> > @@ -376,11 +340,10 @@ efi_status_t EFIAPI efi_firmware_raw_get_image_info(
> > !descriptor_size || !package_version || !package_version_name))
> > return EFI_EXIT(EFI_INVALID_PARAMETER);
> >
> > - ret = efi_get_dfu_info(image_info_size, image_info,
> > - descriptor_version, descriptor_count,
> > - descriptor_size,
> > - package_version, package_version_name,
> > - &efi_firmware_image_type_uboot_raw);
> > + ret = efi_fill_image_desc_array(image_info_size, image_info,
> > + descriptor_version, descriptor_count,
> > + descriptor_size, package_version,
> > + package_version_name);
> >
> > return EFI_EXIT(ret);
> > }
> > --
> > 2.25.1
> >
More information about the U-Boot
mailing list