| Message ID | 20220324123901.429472-1-sughosh.ganu@linaro.org |
|---|---|
| Headers | show |
| Series | efi: capsule: Image GUID usage cleanup | expand |
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". > 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.) > 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? > 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. I expect that you should maintain FIT FMP driver properly and it be tested as before. # Moreover, you have not yet added capsule authentication support to FIT FMP driver. -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 > >
hi Takahiro, On Fri, 25 Mar 2022 at 10:23, AKASHI Takahiro <takahiro.akashi@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. > > > 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. > > 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. 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 > > > >
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.akashi@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 > > > > > >