Re: [PATCH v5] u-boot: kernel-fitimage: Fix dependency loop if UBOOT_SIGN_ENABLE and UBOOT_ENV enabled

[Richard Purdie <richard.purdie@linuxfoundation.org>]

On Sun, 2025-01-19 at 19:08 +0100, Marek Vasut wrote:
> In case both UBOOT_SIGN_ENABLE and UBOOT_ENV are enabled and
> kernel-fitimage.bbclass is in use to generate signed kernel
> fitImage, there is a circular dependency between uboot-sign
> and kernel-fitimage bbclasses . The loop looks like this:
> 
> kernel-fitimage.bbclass:
> - do_populate_sysroot depends on do_assemble_fitimage
>   - do_assemble_fitimage depends on virtual/bootloader:do_populate_sysroot
>     - virtual/bootloader:do_populate_sysroot depends on virtual/bootloader:do_install
>       => The virtual/bootloader:do_install installs and the
>          virtual/bootloader:do_populate_sysroot places into
>          sysroot an U-Boot environment script embedded into
>          kernel fitImage during do_assemble_fitimage run .
> 
> uboot-sign.bbclass:
> - DEPENDS on KERNEL_PN, which is really virtual/kernel. More accurately
>   - do_deploy depends on do_uboot_assemble_fitimage
>   - do_install depends on do_uboot_assemble_fitimage
>   - do_uboot_assemble_fitimage depends on virtual/kernel:do_populate_sysroot
>     => do_install depends on virtual/kernel:do_populate_sysroot
> 
> => virtual/bootloader:do_install depends on virtual/kernel:do_populate_sysroot
>    virtual/kernel:do_populate_sysroot depends on virtual/bootloader:do_install
> 
> Attempt to resolve the loop. Pull fitimage configuration options into separate
> new bbclass kernel-fitimage-config.bbclass so these configuration options can
> be shared by both uboot-sign.bbclass and kernel-fitimage.bbclass, and make use
> of mkimage -f auto-conf / mkimage -f auto option to insert /signature node key-*
> subnode into U-Boot control DT without depending on the layout of kernel fitImage
> itself. This is perfectly valid to do, because the U-Boot /signature node key-*
> subnodes 'required' property can contain either of two values, 'conf' or 'image'
> to authenticate either selected configuration or all of images when booting the
> fitImage.
> 
> For details of the U-Boot fitImage signing process, see:
> https://docs.u-boot.org/en/latest/usage/fit/signature.html
> For details of mkimage -f auto-conf and -f auto, see:
> https://manpages.debian.org/experimental/u-boot-tools/mkimage.1.en.html#EXAMPLES
> 
> Fixes: 5e12dc911d0c ("u-boot: Rework signing to remove interdependencies")
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Adrian Freihofer <adrian.freihofer@siemens.com>
> Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Cc: Richard Purdie <richard.purdie@linuxfoundation.org>
> Cc: Sean Anderson <sean.anderson@seco.com>
> ---
> V2: Take a different approach, split the kernel-fitimage.bbclass and
>     use it to generate dummy fitImage on demand
> V3: Use mkimage -f auto-conf and mkimage -f auto to break the loop,
>     the fitImage .its source is not even needed because the 'required'
>     property can only have two values, 'conf' or 'image' .
> V4: Restore CC list
> V5: - Add missing trailing backslash after unused.itb
>     - Replace ${UBOOT_MKIMAGE_MODE} with $UBOOT_MKIMAGE_MODE
>     - Move -d /dev/null to the end of mkimage invocation
> ---
>  .../kernel-fitimage-config.bbclass            | 50 +++++++++++++++++
>  meta/classes-recipe/kernel-fitimage.bbclass   | 54 +------------------
>  meta/classes-recipe/uboot-sign.bbclass        | 27 +++++-----
>  3 files changed, 65 insertions(+), 66 deletions(-)
>  create mode 100644 meta/classes-recipe/kernel-fitimage-config.bbclass

This looks great and I'm nearly ready to merge it but I do have one
further request. Instead of creating kernel-fitimage-config.bbclass,
could you create something like meta/conf/image-fitimage.conf?

There is already precedent for this with image-uefi.conf and you can
include with require ../conf/image-fitimage.conf. I'd like to see
config variables being in conf files rather than class files.

Adding some comments at the top of the new file explaining which config
settings are there and what it is for would also be good.

Thanks,

Richard