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

[Marek Vasut <marex@denx.de>]

On 1/21/25 1:34 PM, Richard Purdie wrote:
> 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.
I hope they are all addressed in V6, thanks.