On Thu, 17 Feb 2022 at 13:52, Ilias Apalodimas <ilias.apalodi...@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. > > 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