Re: [PATCH v4 2/8] capsule: FMP: Populate the image descriptor array from platform data

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

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
>