[RFC PATCH v3 5/9] EFI: FMP: Add provision to update image's ImageTypeId in image descriptor
Sughosh Ganu
sughosh.ganu at linaro.org
Fri Jan 21 08:02:04 CET 2022
hi Takahiro,
On Thu, 20 Jan 2022 at 10:54, AKASHI Takahiro
<takahiro.akashi at 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 at 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. 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.
-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
> >
More information about the U-Boot
mailing list