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.g...@gmx.de> wrote: > > > > On 2/17/22 11:10, Sughosh Ganu wrote: > > > 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. > > > > 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 > >