[PATCH v4 05/11] EFI: FMP: Add provision to update image's ImageTypeId in image descriptor

Tue Mar 8 14:13:47 CET 2022

On Sat, Feb 26, 2022 at 03:24:10PM +0530, Sughosh Ganu wrote:
> hello Heinrich,
> 
> On Sat, 26 Feb 2022 at 12:24, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> >
> > On 2/17/22 11:10, Sughosh Ganu wrote:
> > > On Thu, 17 Feb 2022 at 13:52, Ilias Apalodimas
> > > <ilias.apalodimas at linaro.org> wrote:
> > >>
> > >>>>>>>
> > >>
> > >> [...]
> > >>
> > >>>>>>> Yes, we can use --index when we know the index value corresponding to
> > >>>>>>> the firmware image that we need to update. But like I mentioned in my
> > >>>>>>> earlier reply, for A/B updates, we do not know what the index value is
> > >>>>>>> going to be. That is going to be determined at runtime.
> > >>>>>>
> > >>>>>> I don't think so. See below for alternative approach.
> > >>>>>>
> > >>>>>>> Also, the point I was making is that we can have a capsule which is
> > >>>>>>> consumed by an FMP protocol which has more than one image, and those
> > >>>>>>> images have different ImageTypeId/UpdateImageTypeId.
> > >>>>>>
> > >>>>>> Yes, but it is a design choice in my first implementation.
> > >>>>>> I didn't think that we need to "have a capsule which is consumed
> > >>>>>> by an FMP protocol which has more than one image" as long as we
> > >>>>>> use DFU framework (and FIT as standard format of aggregation on U-Boot).
> > >>>>>
> > >>>>> But this design can be extended without any hassle, and more
> > >>>>> importantly without any regression, no? What kind of a problem does it
> > >>>>> create if the FMP can handle more than one image type.
> > >>>>>
> > >>>>> Even as per the UEFI spec, we have the EFI_FIRMWARE_MANAGEMENT_CAPSULE
> > >>>>> header for all images to be managed by the FMP protocol which has
> > >>>>> multiple images with different UpdateImageTypeId.
> > >>>>>
> > >>>>>>
> > >>>>>>>>
> > >>>>>>>>> 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.
> > >>>>>>>>
> > >>>>>>>> That is an expected behavior, isn't it?
> > >>>>>>>
> > >>>>>>> Yes it is. Do not contest that.
> > >>>>>>>
> > >>>>>>>> If you want to use a different FMP driver (with another GUID),
> > >>>>>>>> you naturally need to add your own FMP driver.
> > >>>>>>>
> > >>>>>>> This is where I differ. We can use the same FMP protocol instance for
> > >>>>>>> any type of ImageTypeId. I do not see why we need to define a
> > >>>>>>> different FMP protocol instance for a GUID value other than what has
> > >>>>>>> been defined for u-boot raw and u-boot FIT GUIDs.
> > >>>>>>
> > >>>>>> I do understand part of your concern a bit.
> > >>>>>> I thought of using the same ImageType GUID, say IMAGE_TYPE_DFU_GUID, at first
> > >>>>>> but we needed different GUIDs here simply because we need to determine
> > >>>>>> the format of payload, FIT format or raw binary.
> > >>>>>>
> > >>>>>>> The platform can give us the image descriptor array, and with that,
> > >>>>>>> the same FMP instance can be used for any type of image(ImageTypeId).
> > >>>>>>
> > >>>>>> "any type of image"? Really?
> > >>>>>
> > >>>>> The raw FMP instance can certainly handle any type of binary payloads
> > >>>>> right. There is no restriction on what type of payload it is as long
> > >>>>> as it is all going as a single entity to a given dfu partition.
> > >>>>>
> > >>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>> 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
> > >>>>>>>>
> > >>>>>>>> "Build time" of what?
> > >>>>>>>
> > >>>>>>> Of the capsule.
> > >>>>>>>
> > >>>>>>>> I think that users should know how "dfu_alt_info" is defined
> > >>>>>>>> (in other words, where the firmware be located on the target system)
> > >>>>>>>> when capsule files are created.
> > >>>>>>>
> > >>>>>>> That is true for a non A/B scenario. And that is how it works in the
> > >>>>>>> non A/B updates case. But for A/B updates, since the determination of
> > >>>>>>> the "location" where the firmware image has to be written will be done
> > >>>>>>> only at runtime, we cannot use the --index to differentiate.
> > >>>>>>
> > >>>>>> Yes, we can :)
> > >>>>>
> > >>>>> You know what I mean -- if we could use the same logic, I would not
> > >>>>> have added all that code :)
> > >>>>>
> > >>>>>>
> > >>>>>> First of all, my essential assumption in either FIT or RAW FMP driver
> > >>>>>> is that U-Boot has (somehow conceptually) single firmware blob represented
> > >>>>>> by DFU or dfu_alt_info. As I said, each object or location in
> > >>>>>> dfu_alt_info can be further identified by index or "UpdateImageIndex".
> > >>>>>>
> > >>>>>> Let's assume that we have two locations of firmware, fw1 and fw2, and
> > >>>>>> that we have two bank A and B.
> > >>>>>> Then we will define dfu_alt_info as follows:
> > >>>>>>    <loc of fw1 for A>;<loc of fw2 for A>;<loc of fw1 for B>;<loc of fw2 for B>;
> > >>>>>>    |<---      1st set               --->|<---       2nd set               --->|
> > >>>>>>
> > >>>>>> When you want to update bank A, we can use the first set of dfu_alt_info,
> > >>>>>> and use the second set of dfu_alt_info for bank B.
> > >>>>>> At runtime, you should know which bank you're working on, and therefore
> > >>>>>> you should know the exact physical location from dfu_alt_info.
> > >>>>>>
> > >>>>>> Please note that you don't have to change the syntax of dfu_alt_info
> > >>>>>> at all. Simply offset the location with 0 for bank A and with 2 for bank B.
> > >>>>
> > >>>> I'll try digging a bit more, but I think the current approach is not
> > >>>> working as it was intended wrt to the EFI spec.  My reading of the spec
> > >>>> and specifically section 23.3.2 is that a Capsule consists of an
> > >>>> EFI capsule header and a payload.  The payload now has an
> > >>>> EFI_FIRMWARE_MANAGEMENT_CAPSULE_HEADER which in it's turn can host multiple
> > >>>> firmware images of different type described in EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER.
> > >>>>
> > >>>> An FMP implementation should read the  UpdateImageTypeId's used to identify
> > >>>> the image you are updating and from that derive the UpdateImageIndex
> > >>>> which SetImage will use. That would give you the ability to update the
> > >>>> all the firmware components with a single capsule.
> > >>>>
> > >>>> Sughosh what about the ESRT table generation?  If you use different UpdateImageTypeId
> > >>>> those should be reflected on the ESRT tables from the OS
> > >>>
> > >>> That would depend on the values populated in the
> > >>> EFI_FIRMWARE_IMAGE_DESCRIPTOR array by the GetImageInfo function. The
> > >>> image descriptor structure has an ImageTypeId field. The value of
> > >>> ImageTypeId is what will be reflected in the ESRT table.
> > >>>
> > >>> In the current implementation, all the images in the ESRT table will
> > >>> show the same ImageTypeId value, either
> > >>> EFI_FIRMWARE_IMAGE_TYPE_UBOOT_FIT_GUID or
> > >>> EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID.
> >
> > This is wrong. According to the the UEFI 2.9 specification the
> > UpdateImageTypeId is used to "identify (the) device firmware targeted by
> > this update". It does not identify the format in which the firmware is
> > delivered.
> >
> > So this needs to be fixed in the next revision of this patch series.
> 
> This patch series is actually adding that platform function which
> populates the image descriptor array with the image GUID's -- patch 6
> of this series[1] actually does that for the ST DK2 platform. This
> discussion was because Takahiro wanted to use the same image
> GUID(u-boot raw/FIT) for all the images, and use the image index for
> identifying where the image is to be written.

The discussion depends on what the *firmware* means.
With my FMP drivers (either FIT or raw), I intended a *set of firmware*
managed with a *single* "dfu_alt_info", which may include various *images*
for different target *components* as the original DFU framework does.

-Takahiro Akashi

> I guess with what you are stating, along with Ilias's opinion on this,
> I will send the next version with the same approach, i.e. using a
> platform function to populate the image GUIDs in the firmware image
> descriptor array. With this, each firmware image will have a different
> GUID which can be used to identify the image.
> 
> -sughosh
> 
> [1] - https://lists.denx.de/pipermail/u-boot/2022-February/474434.html
> 
> >
> > For each firmware part that can be updated provide a unique GUID.
> >
> > Best regards
> >
> > Heinrich
> >
> > >>
> > >> Yea I was mostly asking wrt to A/B updates.  Would the correct UUID be
> > >> shown in the ESRT instead of EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID?
> > >
> > > Yes, the platform would have to define the fill_image_type_guid_array
> > > function which would populate the ImageTypeId values in the
> > > EFI_FIRMWARE_IMAGE_DESCRIPTOR array, and the image GUID values would
> > > then show up as part of the ESRT table.
> > >
> > > As part of this patchset, I have added this function for the STM32MP1 DK2 board.
> > >
> > > -sughosh
> > >
> > >>
> > >>>
> > >>> The UpdateImageTypeId value from the capsule is used to compare with
> > >>> the ImageTypeId values returned by the GetImageInfo function to check
> > >>> if the given FMP protocol can be used for the update.
> > >>>
> > >>> -sughosh
> > >>>
> > >> [...]
> > >>
> > >>
> > >> Regards
> > >> /Ilias
> >