Re: [RESEND RFC PATCH 00/10] FWU: Add support for FWU Multi Bank Update feature

From: Heinrich Schuchardt <xypron.glpk@gmx.de>
To: Sughosh Ganu <sughosh.ganu@linaro.org>
Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>,
	Patrice Chotard <patrice.chotard@foss.st.com>,
	Alexander Graf <agraf@csgraf.de>, Simon Glass <sjg@chromium.org>,
	Bin Meng <bmeng.cn@gmail.com>, Peng Fan <peng.fan@nxp.com>,
	AKASHI Takahiro <takahiro.akashi@linaro.org>,
	Ilias Apalodimas <ilias.apalodimas@linaro.org>,
	Jose Marinho <jose.marinho@arm.com>,
	Grant Likely <grant.likely@arm.com>,
	Jason Liu <jason.hui.liu@nxp.com>,
	u-boot@lists.denx.de
Subject: Re: [RESEND RFC PATCH 00/10] FWU: Add support for FWU Multi Bank Update feature
Date: Fri, 26 Nov 2021 13:29:02 +0100	[thread overview]
Message-ID: <9b27ac54-6ccd-1306-f774-84eef2a57923@gmx.de> (raw)
In-Reply-To: <20211125071302.3644-1-sughosh.ganu@linaro.org>

On 11/25/21 08:12, Sughosh Ganu wrote:
> (resending to including the first paragraph which got deleted for some
> reason).
>
> The patchset adds support for the FWU Multi Bank Update[1]

The patch set is lacking a documentation update
The patch set is lacking an integration test.

Please, add both to enable reviewing.

Best regards

Heinrich

> feature. Certain aspects of the Dependable Boot[2] specification have
> also been implemented.
>
> The FWU multi bank update feature is used for supporting multiple
> sets(also called banks) of firmware image(s), allowing the platform to
> boot from a different bank, in case it fails to boot from the active
> bank. This functionality is supported by keeping the relevant
> information in a structure called metadata, which provides information
> on the images. Among other parameters, the metadata structure contains
> information on the currect active bank that is being used to boot
> image(s).
>
> Functionality is being added to work with the UEFI capsule driver in
> u-boot. The metadata is read to gather information on the update bank,
> which is the bank to which the firmware images would be flashed to. On
> a successful completion of the update of all components, the active
> bank field in the metadata is updated, to reflect the bank from which
> the platform will boot on the subsequent boots.
>
> Currently, the feature is being enabled on the STM32MP157C-DK2
> board which boots a FIP image from a uSD card partitioned with the GPT
> partioning scheme. This also requires changes in the previous stage of
> bootloader, which parses the metadata and selects the bank to boot the
> image(s) from. Support is being added in tf-a(BL2 stage) for the
> STM32MP157C-DK2 board to boot the active bank images. These changes
> are under review currently[3].
>
> Todo's
> ------
> 1) Add a test(selftest) for the metadata access.
> 2) Add a tool for generation of the metadata. Not sure if this needs to
>     be part of the u-boot repository though.
> 3) Add a tool for generation of the firmware accept/reject dummy
>     capsule. Need to check if this can be added to the mkeficapsule
>     tool in u-boot.
>
> [1] - https://developer.arm.com/documentation/den0118/a
> [2] - https://staging-git.codelinaro.org/linaro/firmware-dual-banked-updates/test
> [3] - https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/12566
>
> Sughosh Ganu (10):
>    GPT: Add function to get gpt header and partition entries
>    stm32mp: dfu: Move the ram partitions to the end of the dfu_alt_info
>      variable
>    FWU: Add metadata structure and functions for accessing metadata
>    FWU: Add metadata access functions for GPT partitioned block devices
>    FWU: stm32mp1: Add helper functions for accessing metadata
>    FWU: STM32MP1: Add support to read boot index from backup register
>    EFI: FMP: Add provision to update image's ImageTypeId in image
>      descriptor
>    FWU: Add boot time checks as highlighted by the FWU specification
>    FWU: Add support for FWU Multi Bank Update feature
>    FWU: cmd: Add a command to read metadata
>
>   arch/arm/mach-stm32mp/include/mach/stm32.h |   1 +
>   board/st/common/stm32mp_dfu.c              |  11 +-
>   board/st/stm32mp1/stm32mp1.c               |  70 ++
>   cmd/Kconfig                                |   6 +
>   cmd/Makefile                               |   1 +
>   cmd/fwu_metadata.c                         |  65 ++
>   common/board_r.c                           |   6 +
>   disk/part_efi.c                            |  10 +
>   include/fwu_metadata.h                     | 140 ++++
>   include/part.h                             |  14 +
>   lib/Kconfig                                |  32 +
>   lib/Makefile                               |   1 +
>   lib/efi_loader/efi_capsule.c               | 190 +++++-
>   lib/efi_loader/efi_firmware.c              |  76 ++-
>   lib/fwu_updates/Makefile                   |  11 +
>   lib/fwu_updates/fwu.c                      | 170 +++++
>   lib/fwu_updates/fwu_metadata.c             | 275 ++++++++
>   lib/fwu_updates/fwu_metadata_gpt_blk.c     | 716 +++++++++++++++++++++
>   18 files changed, 1784 insertions(+), 11 deletions(-)
>   create mode 100644 cmd/fwu_metadata.c
>   create mode 100644 include/fwu_metadata.h
>   create mode 100644 lib/fwu_updates/Makefile
>   create mode 100644 lib/fwu_updates/fwu.c
>   create mode 100644 lib/fwu_updates/fwu_metadata.c
>   create mode 100644 lib/fwu_updates/fwu_metadata_gpt_blk.c
>