[PATCH v4 05/11] EFI: FMP: Add provision to update image's ImageTypeId in image descriptor
Sughosh Ganu
sughosh.ganu at linaro.org
Thu Feb 10 11:10:00 CET 2022
On Thu, 10 Feb 2022 at 13:28, AKASHI Takahiro
<takahiro.akashi at linaro.org> wrote:
>
> On Thu, Feb 10, 2022 at 12:48:13PM +0530, Sughosh Ganu wrote:
> > hi Takahiro,
> >
> > On Thu, 10 Feb 2022 at 08:18, AKASHI Takahiro
> > <takahiro.akashi at linaro.org> wrote:
> > >
> > > Hi Sughosh,
> > >
> > > On Mon, Feb 07, 2022 at 11:49:55PM +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.
> > >
> > > After re-thinking of your approach here, I would have to say NAK.
> > >
> > > You use ImageTypeId to identify a particular firmware object.
> > > (By "object," I mean one of firmware instances represented by "dfu_alto_info".
> > > Please don't confuse it with the binary blob embedded in a capsule file.)
> > > But ImageTypeId is not for that purpose, at least, as my intention
> > > in initially implementing capsule framework and FMP drivers.
> > >
> > > 1) ImageTypeId is used to uniquely identify a corresponding FMP driver,
> > > either FIT FMP driver or Raw FMP driver.
> >
> > I believe the identification of an FMP protocol should be done by the
> > FMP GUID,
>
> What does FMP GUID stand for?
EFI_FIRMWARE_MANAGEMENT_PROTOCOL_GUID, defined in include/efi_api.h.
What I mean is that even when installing the FMP protocol, the call to
efi_install_multiple_protocol_interfaces takes the above FMP GUID as
an argument -- nowhere is the ImageTypeId considered when installing
the protocol.
>
> > which is what is done in efi_fmp_find. The ImageTypeId is
> > nowhere involved in this identification.
>
> Please take a look at efi_capsule_update_firmware() carefully.
> efi_find_fmp() is called with the image's update_image_type_id
> which is to be set to EFI_FIRMWARE_IMAGE_TYPE_UBOOT_FIT_GUID or
> EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID by mkeficapsule
> (see create_fwbin()).
I think you are thinking from the point of view of the '--guid' value
that is being passed to the capsule generation tool. But the thing is
that it is the current design(or limitation) of the tool that it takes
only a single guid parameter. So the mkeficapsule tool currently can
generate only a single payload capsule. Please check the
GenerateCapsule script in EDK2. In case of a multi payload based
capsule, individual parameters like the UpdateImageTypeId are passed
through the json file, where each of the UpdateImageTypeId has a
different value per payload.
>
> > > 2) Each firmware object handled by a given FMP driver can further be
> > > identified by ImageIndex.
> > >
> > > My implementation of efi_fmp_find() does (1) and Raw FMP driver does
> > > (2) in efi_firmware_raw_set_image() which takes "image_index" as
> > > a parameter.
> > >
> > > Using ImageTypeId as an identifier is simply wrong in my opinion and
> > > doesn't meet the UEFI specification.
> >
> > So, as per what you are stating, all payloads under a given
> > EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER should have the same
> > ImageTypeId, either EFI_FIRMWARE_IMAGE_TYPE_UBOOT_FIT_GUID or
> > EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID. Same applies for all images in
> > the EFI_FIRMWARE_IMAGE_DESCRIPTOR. Because, without one of the two
> > values, > the check in efi_fmp_find to compare the UpdateImageTypeId
> > with the ImageTypeId retrieved from the image descriptor would simply
> > fail.
>
> I don't follow your point.
> Please elaborate a bit more.
The current implementation of GetImageInfo, passes either of
EFI_FIRMWARE_IMAGE_TYPE_UBOOT_FIT_GUID or
EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID for all the images of the image
descriptor array. So, in case the capsule is generated with a '--guid'
value which is different from these two values, the check in
efi_fmp_find on line 204 will fail. This means that unless the --guid
value passed to the capsule generation is either of u-boot FIT or
u-boot raw, the current FMP protocol for raw devices cannot be used.
Why do we need that restriction. It should be possible to use the raw
FMP protocol for any other type of image types as well.
>
> > I think this interpretation of the UEFI spec is incorrect, since the
> > spec states that the ImageTypeId and the UpdateImageTypeId are fields
> > used to identify the firmware component targeted for the update. If
> > all values in the image descriptor array and the UpdateImageTypeId are
> > the same, why have this field in the first place for individual
> > images.
>
> As I said, ImageIndex is for that purpose.
Yes, that is one possible way in the scenario where the ImageIndex is
determined at the capsule generation time. But, for the A/B update
scenario, we do not know the ImageIndex at build time -- this is
determined only at runtime, and is based on the bank to which the
image is to be updated. Which is why I am finding out the alt_num at
runtime in case the FWU Multi Bank feature is enabled. Like I said
above, I do not see a reason why the current FMP protocols should be
restricted to only the u-boot FIT and u-boot raw image types. It is
being extended, without affecting the default non FWU behaviour.
-sughosh
>
> -Takahiro Akashi
>
> >
> > -sughosh
> >
> > >
> > > -Takahiro Akashi
> > >
> > >
> > > > Signed-off-by: Sughosh Ganu <sughosh.ganu at linaro.org>
> > > > ---
> > > >
> > > > Changes since V3:
> > > > * Define a weak function fill_image_type_guid_array for populating the
> > > > image descriptor array with u-boot's raw and fit image GUIDs
> > > >
> > > > include/efi_loader.h | 2 +
> > > > lib/efi_loader/efi_firmware.c | 71 +++++++++++++++++++++++++++++++----
> > > > 2 files changed, 66 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > > > index f4860e87fc..ae60de0be5 100644
> > > > --- a/include/efi_loader.h
> > > > +++ b/include/efi_loader.h
> > > > @@ -992,4 +992,6 @@ efi_status_t efi_esrt_populate(void);
> > > > efi_status_t efi_load_capsule_drivers(void);
> > > >
> > > > efi_status_t platform_get_eventlog(struct udevice *dev, u64 *addr, u32 *sz);
> > > > +efi_status_t fill_image_type_guid_array(const efi_guid_t *default_guid,
> > > > + efi_guid_t **part_guid_arr);
> > > > #endif /* _EFI_LOADER_H */
> > > > diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c
> > > > index a1b88dbfc2..5642be9f9a 100644
> > > > --- a/lib/efi_loader/efi_firmware.c
> > > > +++ b/lib/efi_loader/efi_firmware.c
> > > > @@ -96,6 +96,46 @@ efi_status_t EFIAPI efi_firmware_set_package_info_unsupported(
> > > > return EFI_EXIT(EFI_UNSUPPORTED);
> > > > }
> > > >
> > > > +efi_status_t __weak fill_image_type_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 +144,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 +162,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 +212,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 +290,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 +305,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_image_type_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);
> > > > }
> > > >
> > > > @@ -359,6 +407,7 @@ efi_status_t EFIAPI efi_firmware_raw_get_image_info(
> > > > u16 **package_version_name)
> > > > {
> > > > efi_status_t ret = EFI_SUCCESS;
> > > > + 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 +422,20 @@ efi_status_t EFIAPI efi_firmware_raw_get_image_info(
> > > > !descriptor_size || !package_version || !package_version_name))
> > > > return EFI_EXIT(EFI_INVALID_PARAMETER);
> > > >
> > > > + ret = fill_image_type_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
> > > >
More information about the U-Boot
mailing list