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

On Mon, 24 Jan 2022 at 08:03, AKASHI Takahiro takahiro.akashi@linaro.org wrote:

On Fri, Jan 21, 2022 at 12:32:04PM +0530, Sughosh Ganu wrote:

...

hi Takahiro,

On Thu, 20 Jan 2022 at 10:54, AKASHI Takahiro takahiro.akashi@linaro.org wrote:

...

On Thu, Jan 20, 2022 at 12:25:44AM +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 V2: None

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

Why do you not call fwu_plat_fill_partition_guids() for FIT FMP driver? If you have a specific reason, please describe it.

The idea here was to retain the current behaviour for the non FWU use case, where for both raw and FIT images, the same ImageTypeId is being used.

What I meant to say was fwu_plat_fill_partition_guids() is not called even in case of CONFIG_FWU_MULTI_BANK_UPDATE.

Ah, that is because I am testing the FWU code with the raw capsules and not FIT. I will add a weak function. If there is FWU support added for FIT based capsules in the future, the relevant plat function will have to be added.

-sughosh

...

Do you want to have a weak function which fills the array with a specific ImageTypeId value. Then, if any platform wants to have a different implementation, they can define a function which fills the array with relevant GUID values.

...

...

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

• }
  

The code: ret = fill_part_guid_array(&efi_firmware_image_type_uboot_raw, &part_guid_arr); if (IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE)) status = fwu_plat_fill_partition_guids(&part_guid_arr);

would be much simpler here.

Yep, will change.

...

But I don't know why you want to call fwu_plat_fill_partition_guids() only in case of CONFIG_FWU_MULTI_BANK_UPDATE. The functionality should be the same whether A/B update or not.

Let me know if you are okay with what I have proposed above.

Yes. The weak "plat" function will be expected to be optimized away.

-Takahiro Akashi

...

-sughosh

...

-Takahiro Akashi

...

• 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