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

[Ilias Apalodimas]

Akashi-san

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

I still think we should fix that.  The current code is working, but
it's not what the EFI spec describes.  I think we should do it the
other way around.

IOW the board defines the UUID's for the firmware partitions it needs
to update.  The FMP driver should then populate those properly in an
ESRT table and also generate an appropriate dfu_alt_info (or similar)
on the fly.

Regards
/Ilias
>
> -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
> > >