Re: [RFC PATCH v2 5/8] EFI: FMP: Add provision to update image's ImageTypeId in image descriptor

hi Takahiro,

On Tue, 21 Dec 2021 at 10:23, AKASHI Takahiro takahiro.akashi@linaro.org wrote:

Sughosh,

On Sun, Dec 19, 2021 at 12:36:02PM +0530, Sughosh Ganu wrote:

...

The FWU Multi Banks Update feature allows updating different types of updatable firmware images on the platform. These image types are identified using the ImageTypeId GUID value. Add support in the GetImageInfo function of the FMP protocol to get the GUID values for the individual images and populate these in the image descriptor for the corresponding images.

Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org

Changes since V1:

• Define a new function fwu_plat_get_alt_num for filling up all the dfu partitions with a preset ImageTypeId guid
• Remove the distinction made in the earlier version for setting image_type_id as suggested by Heinrich

lib/efi_loader/efi_firmware.c | 90 ++++++++++++++++++++++++++++++++--- 1 file changed, 83 insertions(+), 7 deletions(-)

diff --git a/lib/efi_loader/efi_firmware.c

b/lib/efi_loader/efi_firmware.c

...

index a1b88dbfc2..648342ae72 100644 --- a/lib/efi_loader/efi_firmware.c +++ b/lib/efi_loader/efi_firmware.c @@ -10,6 +10,7 @@ #include <charset.h> #include <dfu.h> #include <efi_loader.h> +#include <fwu.h> #include <image.h> #include <signatures.h>

@@ -96,6 +97,46 @@ efi_status_t EFIAPI

efi_firmware_set_package_info_unsupported(

...

  return EFI_EXIT(EFI_UNSUPPORTED);

}

+static efi_status_t fill_part_guid_array(const efi_guid_t *guid,

•                                  efi_guid_t **part_guid_arr)
  

+{

• int i;
  

• int dfu_num = 0;
  

• efi_guid_t *guid_arr;
  

• struct dfu_entity *dfu;
  

• efi_status_t ret = EFI_SUCCESS;
  

• dfu_init_env_entities(NULL, NULL);
  

• dfu_num = 0;
  

• list_for_each_entry(dfu, &dfu_list, list) {
  

•         dfu_num++;
  

• }
  

• if (!dfu_num) {
  

•         log_warning("Probably dfu_alt_info not defined\n");
  

•         ret = EFI_NOT_READY;
  

•         goto out;
  

• }
  

• *part_guid_arr = malloc(sizeof(efi_guid_t) * dfu_num);
  

• if (!*part_guid_arr) {
  

•         ret = EFI_OUT_OF_RESOURCES;
  

•         goto out;
  

• }
  

• guid_arr = *part_guid_arr;
  

• for (i = 0; i < dfu_num; i++) {
  

•         guidcpy(guid_arr, guid);
  

•         ++guid_arr;
  

• }
  

+out:

• dfu_free_entities();
  

• return ret;
  

+}

/**

• efi_get_dfu_info - return information about the current firmware

image

...

• @this: Protocol instance

@@ -104,9 +145,9 @@ efi_status_t EFIAPI

efi_firmware_set_package_info_unsupported(

...

• @descriptor_version: Pointer to version number
• @descriptor_count: Pointer to number of descriptors
• @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
• • @guid_array: Image type GUID array

If I understand your code correctly, a guid in descriptor for any image (I mean any part defined by dfu_alto_info) will have the same value even with this change. If so, why do we have to take an array of GUIDs as a parameter?

One of Heinrich's comments in my previous version was not to distinguish between the FWU case and the normal case when setting the image_type_id. The existing code sets the same GUID value(image_type) for all images. I have kept this consistent. With the FWU feature enabled, fwu_plat_fill_partition_guids is called and this function fills in the array with the relevant values of partition GUIDs. fwu_plat_fill_partition_guids has been defined for the ST board in patch 3/8.

On your previous version, Heinrich made a comment like:

! The sequence of GUIDs in a capsule should not influence the update. The ! design lacks a configuration defining which GUID maps to which DFU part. ! ! Please, get rid of all DFU environment variables and describe the update ! in a configuration file that you add to the capsule.

Have you addressed this issue in this series? (To do that, we may have to introduce a new format of capsule file.)

No, Ilias and I had replied back that detaching capsule update from DFU is a separate effort, not related to the addition of the FWU functionality and should be taken up as a separate activity.

-sughosh

-Takahiro Akashi

...

• Return information bout the current firmware image in @image_info.
• @image_info will consist of a number of descriptors.

@@ -122,7 +163,7 @@ static efi_status_t efi_get_dfu_info( efi_uintn_t *descriptor_size, u32 *package_version, u16 **package_version_name,

• const efi_guid_t *image_type)
  

• const efi_guid_t *guid_array)
  

{ struct dfu_entity *dfu; size_t names_len, total_size; @@ -172,7 +213,7 @@ static efi_status_t efi_get_dfu_info( 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_type_id = guid_array[i];
          image_info[i].image_id = dfu->alt;
  
          /* copy the DFU entity name */
  

@@ -250,6 +291,7 @@ efi_status_t EFIAPI efi_firmware_fit_get_image_info( u16 **package_version_name) { efi_status_t ret;

• efi_guid_t *part_guid_arr = NULL;
  
  EFI_ENTRY("%p %p %p %p %p %p %p %p\n", this,
            image_info_size, image_info,
  

@@ -264,12 +306,19 @@ efi_status_t EFIAPI

efi_firmware_fit_get_image_info(

...

       !descriptor_size || !package_version ||

!package_version_name))

...

          return EFI_EXIT(EFI_INVALID_PARAMETER);

• ret = fill_part_guid_array(&efi_firmware_image_type_uboot_fit,
  

•                            &part_guid_arr);
  

• if (ret != EFI_SUCCESS)
  

•         goto out;
  

• 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);
  

•                        part_guid_arr);
  
  

+out:

• free(part_guid_arr);
  return EFI_EXIT(ret);
  

}

@@ -358,7 +407,10 @@ efi_status_t EFIAPI efi_firmware_raw_get_image_info( u32 *package_version, u16 **package_version_name) {

• int status;
  efi_status_t ret = EFI_SUCCESS;
  

• const efi_guid_t null_guid = NULL_GUID;
  

• efi_guid_t *part_guid_arr = NULL;
  
  EFI_ENTRY("%p %p %p %p %p %p %p %p\n", this,
            image_info_size, image_info,
  

@@ -373,12 +425,36 @@ efi_status_t EFIAPI

efi_firmware_raw_get_image_info(

...

       !descriptor_size || !package_version ||

!package_version_name))

...

          return EFI_EXIT(EFI_INVALID_PARAMETER);

• if (IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE)) {
  

•         ret = fill_part_guid_array(&null_guid, &part_guid_arr);
  

•         if (ret != EFI_SUCCESS)
  

•                 goto out;
  

•         /*
  

•          * Call the platform function to fill the GUID array
  

•          * with the corresponding partition GUID values
  

•          */
  

•         status = fwu_plat_fill_partition_guids(&part_guid_arr);
  

•         if (status < 0) {
  

•                 log_err("Unable to get partiion guid's\n");
  

•                 ret = EFI_DEVICE_ERROR;
  

•                 goto out;
  

•         }
  

• } else {
  

•         ret =
  

fill_part_guid_array(&efi_firmware_image_type_uboot_raw,

...

•                                    &part_guid_arr);
  

•         if (ret != EFI_SUCCESS)
  

•                 goto out;
  

• }
  

• 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);
  

•                        part_guid_arr);
  
  

+out:

• free(part_guid_arr);
  return EFI_EXIT(ret);
  

}

-- 2.17.1