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

[Ilias Apalodimas]

> > > > >

[...]

> > > > > 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.

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? 

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