Re: [RFC PATCH] tools: kwbimage: Allow to disable compilation of kwbimage on non-mvebu platforms

["Pali Rohár" <pali@kernel.org>]

Hello!

On Thursday 21 October 2021 20:48:22 Samuel Holland wrote:
> Hi,
> 
> Thanks for sending this patch!
> 
> On 10/21/21 4:33 AM, Pali Rohár wrote:
> > kwbimage depends on libcrypto. 32-bit mvebu platforms (except Orion and
> > Discovery, which are not in mach-mvebu) require kwimage for building SPL.
> > 
> > Some users want to compile u-boot tools without libcrypto.
> > 
> > Therefore add a new symbol CONFIG_TOOLS_KWBIMAGE which controls compilation
> > of kwbimage and define correct dependences between mvebu, kwbimage and
> > libcrypto targets.
> > 
> > This allows disabling of kwbimage compilation on non-mvebu platforms.
> > 
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > ---
> >  arch/arm/mach-mvebu/Kconfig | 1 +
> >  tools/Kconfig               | 5 +++++
> >  tools/Makefile              | 5 ++++-
> >  3 files changed, 10 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm/mach-mvebu/Kconfig b/arch/arm/mach-mvebu/Kconfig
> > index 54dff9986b41..1ccbccea1dda 100644
> > --- a/arch/arm/mach-mvebu/Kconfig
> > +++ b/arch/arm/mach-mvebu/Kconfig
> > @@ -15,6 +15,7 @@ config ARMADA_32BIT
> >  	select SPL_SIMPLE_BUS if SPL
> >  	select SUPPORT_SPL
> >  	select TRANSLATION_OFFSET
> > +	select TOOLS_KWBIMAGE if SPL
> >  
> >  config ARMADA_64BIT
> >  	bool
> > diff --git a/tools/Kconfig b/tools/Kconfig
> > index 91ce8ae3e516..40866c5713d9 100644
> > --- a/tools/Kconfig
> > +++ b/tools/Kconfig
> > @@ -25,6 +25,11 @@ config TOOLS_LIBCRYPTO
> >  	  This selection does not affect target features, such as runtime FIT
> >  	  signature verification.
> >  
> > +config TOOLS_KWBIMAGE
> > +	bool "Enable kwbimage support in host tools"
> > +	default y
> > +	depends on TOOLS_LIBCRYPTO
> 
> This symbol needs to select TOOLS_LIBCRYPTO. Otherwise that option can
> still be deselected by the user, and the build fails when doing so:

Yes, that it truth. But in case user does not have openssl installed
then it show another compile / link error.

And I do not know what is the better "user experience". If build system
should throw warnings (like below) about unmet dependences (user chose
something which cannot be satisfied by build system) with link errors.
Or if build system should silently enable openssl symbols, even when
they were explicitly disabled by user (via config file) and then throw
compile error about missing header files.

I do not have opinion which of these two option is better.

Of course changing "depends on" to "select" is not a problem.

> $ make CROSS_COMPILE=arm-linux-musleabi- db-mv784mp-gp_defconfig
> #
> # configuration written to .config
> #
> $ make CROSS_COMPILE=arm-linux-musleabi- nconfig
> scripts/kconfig/nconf  Kconfig
> $ make CROSS_COMPILE=arm-linux-musleabi-
> scripts/kconfig/conf  --syncconfig Kconfig
> 
> WARNING: unmet direct dependencies detected for TOOLS_KWBIMAGE
>   Depends on [n]: TOOLS_LIBCRYPTO [=n]
>   Selected by [y]:
>   - ARMADA_32BIT [=y] && ARM [=y] && ARCH_MVEBU [=y] && SPL [=y]
> 
> WARNING: unmet direct dependencies detected for TOOLS_KWBIMAGE
>   Depends on [n]: TOOLS_LIBCRYPTO [=n]
>   Selected by [y]:
>   - ARMADA_32BIT [=y] && ARM [=y] && ARCH_MVEBU [=y] && SPL [=y]
> 
> WARNING: unmet direct dependencies detected for TOOLS_KWBIMAGE
>   Depends on [n]: TOOLS_LIBCRYPTO [=n]
>   Selected by [y]:
>   - ARMADA_32BIT [=y] && ARM [=y] && ARCH_MVEBU [=y] && SPL [=y]

I known that these warning lines come from kconfig tools... But should
not they be rather fatal errors? As it indicates issues with
configuration.

>   CFG     u-boot.cfg
>   GEN     include/autoconf.mk.dep
>   CFG     spl/u-boot.cfg
>   GEN     include/autoconf.mk
>   GEN     spl/include/autoconf.mk
> ===================== WARNING ======================
> This board does not use CONFIG_DM_I2C (Driver Model
> for I2C drivers). Please update the board to use
> CONFIG_DM_I2C before the v2022.04 release. Failure to
> update by the deadline may result in board removal.
> See doc/driver-model/migration.rst for more info.
> ====================================================
>   CFGCHK  u-boot.cfg
>   HOSTLD  tools/dumpimage
>   HOSTLD  tools/mkimage
> ld: tools/kwbimage.o: in function `kwb_compute_pubkey_hash':
> kwbimage.c:(.text+0x1a00): undefined reference to `EVP_MD_CTX_new'
> ld: kwbimage.c:(.text+0x1a10): undefined reference to `EVP_MD_CTX_reset'
> ld: kwbimage.c:(.text+0x1a18): undefined reference to `EVP_sha256'
> [and more linker errors]
> 
> Regards,
> Samuel
> 
> > +
> >  config TOOLS_FIT
> >  	def_bool y
> >  	help
> > diff --git a/tools/Makefile b/tools/Makefile
> > index 75d8fe71d668..08f1f5a51fb3 100644
> > --- a/tools/Makefile
> > +++ b/tools/Makefile
> > @@ -118,7 +118,6 @@ dumpimage-mkimage-objs := aisimage.o \
> >  			imximage.o \
> >  			imx8image.o \
> >  			imx8mimage.o \
> > -			kwbimage.o \
> >  			lib/md5.o \
> >  			lpc32xximage.o \
> >  			mxsimage.o \
> > @@ -150,6 +149,10 @@ dumpimage-mkimage-objs := aisimage.o \
> >  			$(RSA_OBJS-y) \
> >  			$(AES_OBJS-y)
> >  
> > +ifdef CONFIG_TOOLS_KWBIMAGE
> > +dumpimage-mkimage-objs += kwbimage.o
> > +endif
> > +
> >  dumpimage-objs := $(dumpimage-mkimage-objs) dumpimage.o
> >  mkimage-objs   := $(dumpimage-mkimage-objs) mkimage.o
> >  fit_info-objs   := $(dumpimage-mkimage-objs) fit_info.o
> > 
>