Message ID | 20220331132750.1532722-3-sughosh.ganu@linaro.org |
---|---|
State | Superseded |
Headers | show |
Series | efi: capsule: Capsule Update fixes and enhancements | expand |
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@linaro.org> > Reviewed-by: Masami Hiramatsu <masami.hiramatsu@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 > + *descriptor_count = image_count = num_image_type_guids; We could get rid of image_count here > 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? 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 >
hi Ilias, On Thu, 31 Mar 2022 at 20:38, Ilias Apalodimas <ilias.apalodimas@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@linaro.org> > > Reviewed-by: Masami Hiramatsu <masami.hiramatsu@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 > >
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]; + *descriptor_count = image_count = num_image_type_guids; 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; 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); }