[RFC PATCH 1/6] capsule: Add Image GUIDs for platforms using capsule updates

[Sughosh Ganu]

On Thu, 24 Mar 2022 at 20:21, Michal Simek <michal.simek at xilinx.com> wrote:
>
>
>
> On 3/24/22 15:44, Sughosh Ganu wrote:
> > On Thu, 24 Mar 2022 at 19:55, Michal Simek <michal.simek at xilinx.com> wrote:
> >>
> >>
> >>
> >> On 3/24/22 13:38, Sughosh Ganu wrote:
> >>> Currently, all platforms that enable capsule updates do so using
> >>> either EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID or
> >>> EFI_FIRMWARE_IMAGE_TYPE_UBOOT_FIT_GUID. This is based on the Firmware
> >>> Management Protocol(FMP) instance used on the platform. However, this
> >>> means that all platforms that enable a particular FMP instance have
> >>> the same GUID value for all the updatable images, either the FIT image
> >>> GUID or the raw image GUID, and that an image for some platform can be
> >>> updated on any other platform which uses the same FMP instance. Another
> >>> issue with this implementation is that the ESRT table shows the same
> >>> GUID value for all images on the platform and also across platforms,
> >>> which is not in compliance with the UEFI specification.
> >>>
> >>> Fix this by defining image GUID values and firmware names for
> >>> individual images per platform. The GetImageInfo FMP hook would then
> >>> populate these values in the image descriptor array.
> >>>
> >>> Signed-off-by: Sughosh Ganu <sughosh.ganu at linaro.org>
> >>> ---
> >>>    .../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 +++++++++++++++
> >>>    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_loader.h                          | 15 ++++++++++++
> >>>    20 files changed, 280 insertions(+), 1 deletion(-)
> >
> > <snip>
> >
> >>> diff --git a/board/xilinx/common/board.h b/board/xilinx/common/board.h
> >>> index 69e642429b..9bcac14946 100644
> >>> --- a/board/xilinx/common/board.h
> >>> +++ b/board/xilinx/common/board.h
> >>> @@ -7,6 +7,24 @@
> >>>    #ifndef _BOARD_XILINX_COMMON_BOARD_H
> >>>    #define _BOARD_XILINX_COMMON_BOARD_H
> >>>
> >>> +#if CONFIG_IS_ENABLED(EFI_HAVE_CAPSULE_SUPPORT)
> >>> +#define ZYNQ_BOOT_IMAGE_GUID \
> >>> +     EFI_GUID(0x1ba29a15, 0x9969, 0x40aa, 0xb4, 0x24, \
> >>> +              0xe8, 0x61, 0x21, 0x61, 0x86, 0x64)
> >>> +
> >>> +#define ZYNQ_UBOOT_IMAGE_GUID \
> >>> +     EFI_GUID(0x1a5178f0, 0x87d3, 0x4f36, 0xac, 0x63, \
> >>> +              0x3b, 0x31, 0xa2, 0x3b, 0xe3, 0x05)
> >>> +
> >>> +#define ZYNQMP_BOOT_IMAGE_GUID \
> >>> +     EFI_GUID(0xde6066e8, 0x0256, 0x4fad, 0x82, 0x38, \
> >>> +              0xe4, 0x06, 0xe2, 0x74, 0xc4, 0xcf)
> >>> +
> >>> +#define ZYNQMP_UBOOT_IMAGE_GUID \
> >>> +     EFI_GUID(0xcf9ecfd4, 0x938b, 0x41c5, 0x85, 0x51, \
> >>> +              0x1f, 0x88, 0x3a, 0xb7, 0xdc, 0x18)
> >>> +#endif /* EFI_HAVE_CAPSULE_SUPPORT */
> >>
> >> I can't see any benefit to have it defined here for all.
> >> Directly in board or in include/configs/* seems to be better option.
> >
> > I had initially put these definitions in
> > include/configs/zynq-common.h, but that breaks the build for both
> > zynq_virt and zynqmp_virt, since this file does not get included in
> > the corresponding board file. Which is why I put these under
> > board/xilinx/common/board.h, as this gets included in both the board
> > files. I don't see any other zynq* file under include/configs/. Can
> > you suggest one for both the platforms.
>
> This is for zynq
> include/configs/zynq-common.h
>
> This is for zynqmp
> include/configs/xilinx_zynqmp.h
>
> there is also one for versal which doesn't have capsule enabled yet
> include/configs/xilinx_versal.h
>
> but maybe make sense to add fw_images structure to
> board/xilinx/common/board.c

Okay, I will move the array definition to the suggested file in the
next version.

>
> and define that macros in headers above with generic macro name
> XILINX_BOOT_IMAGE_GUID and XILINX_UBOOT_IMAGE_GUID
> and fw_name and XILINX-BOOT and XILINX-UBOOT to have it ready for versal.

Okay, will do as you suggest.

-sughosh