Sughosh, On Fri, Mar 25, 2022 at 03:14:20PM +0530, Sughosh Ganu wrote: > hi Takahiro, > > On Fri, 25 Mar 2022 at 10:23, AKASHI Takahiro > <takahiro.aka...@linaro.org> wrote: > > > > Sughosh, > > > > On Thu, Mar 24, 2022 at 06:08:55PM +0530, Sughosh Ganu wrote: > > > > > > This series is cleaning up the usage of the image GUIDs that are used > > > in capsule update and the EFI System Resource Table(ESRT). > > > > > > Currently, there are two instances of the Firmware Management > > > Protocol(FMP), one defined for updating the FIT images, and the other > > > for updating raw images. The FMP code defines two GUID values, one for > > > all FIT images, and one for raw images. Depending on the FMP instance > > > used on a platform, the platform needs to use the corresponding image > > > GUID value for all images on the platform, and also across platforms. > > > > > > A few issues are being fixed through the patch series. One, that an > > > image for a different platform can be flashed on another platform if > > > both the platforms are using the same FMP instance. So, for e.g. a > > > capsule generated for the Socionext DeveloperBox platform can be > > > flashed on the ZynqMP platform, since both the platforms use the > > > CONFIG_EFI_CAPSULE_FIRMWARE_RAW instance of the FMP. This can be > > > corrected if each firmware image that can be updated through the > > > capsule update mechanism has it's own unique image GUID. > > > > Ok although the specification doesn't explicitly require the uniqueness > > "across platforms". > > Yes, but unless we have a single image running on multiple platforms, > we do need different GUID values across platforms. > > > > > > The second issue that this patch series fixes is the value of FwClass > > > in the ESRT. With the current logic, all firmware image entries in the > > > ESRT display the same GUID value -- either the FIT GUID or the raw > > > GUID. This is not in compliance with the UEFI specification, as the > > > specification requires all entries to have unique GUID values. > > > > Well, this is *not* a problem of fit FMP driver, but a matter of how > > ESRT is populated. Table 23-16 "ESRT and FMP Fields" says, > > The ImageTypeId GUID from the Firmware > > Management Protocol instance for a device is > > used as the Firmware Class GUID in the ESRT. > > Where there are multiple identical devices in > > the system, system firmware must create a > > mapping to ensure that the ESRT FwClass > > GUIDs are unique and consistent. > > The second sentence suggests that UEFI implementation, i.e. > > efi_esrt_populate(), may and should allow some *mapping*. > > > > That said, I basically accept the requirement that you mention above. > > > > > The third issue being fixed is the population of the > > > EFI_FIRMWARE_IMAGE_DESCRIPTOR array. The current code uses the dfu > > > framework for populating the image descriptor array. However, there > > > might be other images that are not to be updated through the capsule > > > update mechanism also registered with the dfu framework. As a result > > > of this, the ESRT will show up entries of images that are not to be > > > targeted by the capsule update mechanism. > > > > > > These issues are being fixed by defining a structure, efi_fw_images. A > > > platform can then define image related information like the image GUID > > > and image name. Every platform that uses capsule update mechanism > > > needs to define fw_images array. This array will then be used to > > > populate the image descriptor array, and also in determining if a > > > particular capsule's payload can be used for updating an image on the > > > platform. > > > > When ESRT support patch was posted, I proposed that we should have > > a kind of configuration table that defines all the firmware on the system > > for ESRT, but this proposal was rejected. > > Your efi_fw_images[] looks quite similar as what I had in mind. > > Why not define efi_fw_images[] as ESRT itself. > > (Of course, some fields in an entry can still be populated through > > GetImageInfo API.) > > Currently, with this patch series, that is what is happening. The > efi_fw_images array gets read by the GetImageInfo function, and that > information gets used for populating the ESRT. Btw, I also want to > extend this structure as part of a separate task, where we have > information related to the dfu framework as part of the structure. > Then the capsule related dfu information can be populated from this > structure, instead of the dfu_alt_info env variable. This is what > Ilias has suggested. But I will take this up as a separate exercise. > > > > > > The first patch of this series adds the fw_images array in all > > > platforms which are using UEFI capsule updates > > > > > > The second patch of the series changes the logic for populating the > > > image descriptor array, using the information from the fw_images array > > > defined by the platform. > > > > So a FIT image can still work as a single binary of firmware > > under FIT FMP driver. Correct? > > Yes, this will still work. Only change is that every platform will > have a separate image GUID for the FIT image.
Ok. > > > > > The third patch of the series removes the test cases using the --raw > > > and --fit parameters, removes test case for FIT images, and adds a > > > test case for checking that the update happens only with the correct > > > image GUID value in the capsule. > > > > Your change in test_capsule_firmware.py tries to remove FIT FMP > > driver test and it eventually hides the fact that FIT driver may get broken. > > Yes, I am aware of this. I was required to do this since we cannot > have both instances of the FMP. I will move the FIT test into a > separate script. Instead, please add a *new* test script, or separate test "class", for your changes. I think that you should mention remaining issues or TODO items in your cover letter. > > > > I expect that you should maintain FIT FMP driver properly and it be > > tested as before. > > Okay. Although, I wanted to check if you think we need to have support > for updating the FIT image. Yes, I think so. FIT format has been long used, recognized as a standard on U-Boot and well integrated with DFU framework. That is why we can simply call fit_update() in SetImage API. So having FIT FMP driver is a good migration path. Moreover, treating binaries as a single capsule ensures that all the firmware can and should be updated atomically and simultaneously. -Takahiro Akashi > The individual images can very much be > updated through the raw FMP instance. And the raw images also map to > the ESRT, which is not the case with the FIT image. > > > # Moreover, you have not yet added capsule authentication support > > to FIT FMP driver. > > Yes, I have not used an FIT image in my testing up until now, although > it should not be very difficult. I will keep this on my Todo list and > will add support once this gets upstreamed. > > -sughosh > > > > > -Takahiro Akashi > > > > > > > > The fourth patch of the series makes corresponding changes in the > > > capsule update related documentation. > > > > > > The fifth patch of the series removes the now unused FIT and raw image > > > GUID values from the FMP module. > > > > > > The sixth patch of the series removes the --raw and --fit command line > > > parameters in the mkeficapsule utility. > > > > > > > > > Sughosh Ganu (6): > > > capsule: Add Image GUIDs for platforms using capsule updates > > > capsule: FMP: Populate the image descriptor array from platform data > > > test: capsule: Modify the capsule tests to use GUID values for sandbox > > > doc: uefi: Update the capsule update related documentation > > > FMP: Remove GUIDs for FIT and raw images > > > mkeficapsule: Remove raw and FIT GUID types > > > > > > .../imx8mp_rsb3720a1/imx8mp_rsb3720a1.c | 19 ++ > > > .../imx8mm-cl-iot-gate/imx8mm-cl-iot-gate.c | 18 ++ > > > board/emulation/qemu-arm/qemu-arm.c | 20 +++ > > > board/kontron/pitx_imx8m/pitx_imx8m.c | 15 +- > > > board/kontron/sl-mx8mm/sl-mx8mm.c | 14 ++ > > > board/kontron/sl28/sl28.c | 14 ++ > > > board/sandbox/sandbox.c | 17 ++ > > > board/socionext/developerbox/developerbox.c | 23 +++ > > > board/xilinx/common/board.h | 18 ++ > > > board/xilinx/zynq/board.c | 18 ++ > > > board/xilinx/zynqmp/zynqmp.c | 18 ++ > > > configs/sandbox64_defconfig | 1 - > > > configs/sandbox_defconfig | 1 - > > > doc/develop/uefi/uefi.rst | 10 +- > > > include/configs/imx8mm-cl-iot-gate.h | 10 ++ > > > include/configs/imx8mp_rsb3720.h | 10 ++ > > > include/configs/kontron-sl-mx8mm.h | 6 + > > > include/configs/kontron_pitx_imx8m.h | 6 + > > > include/configs/kontron_sl28.h | 6 + > > > include/configs/qemu-arm.h | 10 ++ > > > include/configs/sandbox.h | 10 ++ > > > include/configs/synquacer.h | 14 ++ > > > include/efi_api.h | 8 - > > > include/efi_loader.h | 18 ++ > > > lib/efi_loader/efi_firmware.c | 95 +++------- > > > test/py/tests/test_efi_capsule/conftest.py | 20 +-- > > > .../test_efi_capsule/test_capsule_firmware.py | 167 ++++++------------ > > > tools/eficapsule.h | 8 - > > > tools/mkeficapsule.c | 26 +-- > > > 29 files changed, 384 insertions(+), 236 deletions(-) > > > > > > -- > > > 2.25.1 > > > > > >
