[PATCH] tools: Do not build kwbimage if CONFIG_TOOLS_LIBCRYPTO=n

[PATCH] tools: Do not build kwbimage if CONFIG_TOOLS_LIBCRYPTO=n

[Marek Vasut]

The kwbimage has hard dependency on OpenSSL, do not build it
in case TOOLS_LIBCRYPTO is disabled.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
Cc: Marek Behún <marek.behun@nic.cz>
Cc: Pali Rohár <pali@kernel.org>
Cc: Stefan Roese <sr@denx.de>
Cc: Tom Rini <trini@konsulko.com>
---
 tools/Makefile | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/Makefile b/tools/Makefile
index 1763f44cac4..72488315d9d 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -101,6 +101,9 @@ LIBCRYPTO_OBJS-$(CONFIG_TOOLS_LIBCRYPTO) := $(addprefix lib/, \
 ROCKCHIP_OBS = lib/rc4.o rkcommon.o rkimage.o rksd.o rkspi.o
 
 # common objs for dumpimage and mkimage
+ifdef CONFIG_TOOLS_LIBCRYPTO
+KWB_IMAGE_OBJS-y := kwbimage.o
+endif
 dumpimage-mkimage-objs := aisimage.o \
 			atmelimage.o \
 			$(FIT_OBJS-y) \
@@ -118,7 +121,7 @@ dumpimage-mkimage-objs := aisimage.o \
 			imximage.o \
 			imx8image.o \
 			imx8mimage.o \
-			kwbimage.o \
+			$(KWB_IMAGE_OBJS-y) \
 			lib/md5.o \
 			lpc32xximage.o \
 			mxsimage.o \
-- 
2.34.1

Re: [PATCH] tools: Do not build kwbimage if CONFIG_TOOLS_LIBCRYPTO=n

[Pali Rohár]

On Tuesday 11 January 2022 16:31:20 Marek Vasut wrote:
> The kwbimage has hard dependency on OpenSSL, do not build it
> in case TOOLS_LIBCRYPTO is disabled.

This patch does not work as kwbimage is required for 32-bit Armada
platforms. So kwbimage.o cannot be disabled on these platforms.

There is already proposal for fixing this issue:
https://patchwork.ozlabs.org/project/uboot/patch/20211021093304.25399-1-pali@kernel.org/

> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> Cc: Marek Behún <marek.behun@nic.cz>
> Cc: Pali Rohár <pali@kernel.org>
> Cc: Stefan Roese <sr@denx.de>
> Cc: Tom Rini <trini@konsulko.com>
> ---
>  tools/Makefile | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/Makefile b/tools/Makefile
> index 1763f44cac4..72488315d9d 100644
> --- a/tools/Makefile
> +++ b/tools/Makefile
> @@ -101,6 +101,9 @@ LIBCRYPTO_OBJS-$(CONFIG_TOOLS_LIBCRYPTO) := $(addprefix lib/, \
>  ROCKCHIP_OBS = lib/rc4.o rkcommon.o rkimage.o rksd.o rkspi.o
>  
>  # common objs for dumpimage and mkimage
> +ifdef CONFIG_TOOLS_LIBCRYPTO
> +KWB_IMAGE_OBJS-y := kwbimage.o
> +endif
>  dumpimage-mkimage-objs := aisimage.o \
>  			atmelimage.o \
>  			$(FIT_OBJS-y) \
> @@ -118,7 +121,7 @@ dumpimage-mkimage-objs := aisimage.o \
>  			imximage.o \
>  			imx8image.o \
>  			imx8mimage.o \
> -			kwbimage.o \
> +			$(KWB_IMAGE_OBJS-y) \
>  			lib/md5.o \
>  			lpc32xximage.o \
>  			mxsimage.o \
> -- 
> 2.34.1
> 

Re: [PATCH] tools: Do not build kwbimage if CONFIG_TOOLS_LIBCRYPTO=n

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 805 bytes --]

On Tue, Jan 11, 2022 at 04:36:34PM +0100, Pali Rohár wrote:
> On Tuesday 11 January 2022 16:31:20 Marek Vasut wrote:
> > The kwbimage has hard dependency on OpenSSL, do not build it
> > in case TOOLS_LIBCRYPTO is disabled.
> 
> This patch does not work as kwbimage is required for 32-bit Armada
> platforms. So kwbimage.o cannot be disabled on these platforms.
> 
> There is already proposal for fixing this issue:
> https://patchwork.ozlabs.org/project/uboot/patch/20211021093304.25399-1-pali@kernel.org/

And needs to be respun to not have Kconfig issues.  To answer something
noted in that thread, yes, it would be good if Kconfig did, or had an
option to make WARNING like that fatal.  Or is the problem really that
no, it's non-optional, really, to have OpenSSL installed?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH] tools: Do not build kwbimage if CONFIG_TOOLS_LIBCRYPTO=n

[Marek Vasut]

On 1/11/22 17:16, Tom Rini wrote:
> On Tue, Jan 11, 2022 at 04:36:34PM +0100, Pali Rohár wrote:
>> On Tuesday 11 January 2022 16:31:20 Marek Vasut wrote:
>>> The kwbimage has hard dependency on OpenSSL, do not build it
>>> in case TOOLS_LIBCRYPTO is disabled.
>>
>> This patch does not work as kwbimage is required for 32-bit Armada
>> platforms. So kwbimage.o cannot be disabled on these platforms.
>>
>> There is already proposal for fixing this issue:
>> https://patchwork.ozlabs.org/project/uboot/patch/20211021093304.25399-1-pali@kernel.org/
> 
> And needs to be respun to not have Kconfig issues.  To answer something
> noted in that thread, yes, it would be good if Kconfig did, or had an
> option to make WARNING like that fatal.  Or is the problem really that
> no, it's non-optional, really, to have OpenSSL installed?

OpenSSL should be optional, I got this bug report where someone tried to 
build u-boot on ancient debian oldoldstable with openssl 1.1.0 (without 
ecdsa support), for a platform which doesn't need any of this crypto 
stuff. So, disabling TOOLS_LIBCRYPTO was a sufficient for that platform 
to build in that setup (with this patch). And there are plenty of such 
platforms in the U-Boot tree (all that are not marvell I think).

Re: [PATCH] tools: Do not build kwbimage if CONFIG_TOOLS_LIBCRYPTO=n

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1644 bytes --]

On Tue, Jan 11, 2022 at 07:58:05PM +0100, Marek Vasut wrote:
> On 1/11/22 17:16, Tom Rini wrote:
> > On Tue, Jan 11, 2022 at 04:36:34PM +0100, Pali Rohár wrote:
> > > On Tuesday 11 January 2022 16:31:20 Marek Vasut wrote:
> > > > The kwbimage has hard dependency on OpenSSL, do not build it
> > > > in case TOOLS_LIBCRYPTO is disabled.
> > > 
> > > This patch does not work as kwbimage is required for 32-bit Armada
> > > platforms. So kwbimage.o cannot be disabled on these platforms.
> > > 
> > > There is already proposal for fixing this issue:
> > > https://patchwork.ozlabs.org/project/uboot/patch/20211021093304.25399-1-pali@kernel.org/
> > 
> > And needs to be respun to not have Kconfig issues.  To answer something
> > noted in that thread, yes, it would be good if Kconfig did, or had an
> > option to make WARNING like that fatal.  Or is the problem really that
> > no, it's non-optional, really, to have OpenSSL installed?
> 
> OpenSSL should be optional, I got this bug report where someone tried to
> build u-boot on ancient debian oldoldstable with openssl 1.1.0 (without
> ecdsa support), for a platform which doesn't need any of this crypto stuff.
> So, disabling TOOLS_LIBCRYPTO was a sufficient for that platform to build in
> that setup (with this patch). And there are plenty of such platforms in the
> U-Boot tree (all that are not marvell I think).

Right.  So your patch is a step in the right direction, but we also need
to have the appropriate platforms depend'ing on TOOLS_LIBCRYPTO and
updating the help to note that some platforms require it as well, to
build their images.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH] tools: Do not build kwbimage if CONFIG_TOOLS_LIBCRYPTO=n

[Alex G.]

On 1/11/22 13:09, Tom Rini wrote:
> On Tue, Jan 11, 2022 at 07:58:05PM +0100, Marek Vasut wrote:
>> On 1/11/22 17:16, Tom Rini wrote:
>>> On Tue, Jan 11, 2022 at 04:36:34PM +0100, Pali Rohár wrote:
>>>> On Tuesday 11 January 2022 16:31:20 Marek Vasut wrote:
>>>>> The kwbimage has hard dependency on OpenSSL, do not build it
>>>>> in case TOOLS_LIBCRYPTO is disabled.
>>>>
>>>> This patch does not work as kwbimage is required for 32-bit Armada
>>>> platforms. So kwbimage.o cannot be disabled on these platforms.
>>>>
>>>> There is already proposal for fixing this issue:
>>>> https://patchwork.ozlabs.org/project/uboot/patch/20211021093304.25399-1-pali@kernel.org/
>>>
>>> And needs to be respun to not have Kconfig issues.  To answer something
>>> noted in that thread, yes, it would be good if Kconfig did, or had an
>>> option to make WARNING like that fatal.  Or is the problem really that
>>> no, it's non-optional, really, to have OpenSSL installed?
>>
>> OpenSSL should be optional, I got this bug report where someone tried to
>> build u-boot on ancient debian oldoldstable with openssl 1.1.0 (without
>> ecdsa support), for a platform which doesn't need any of this crypto stuff.
>> So, disabling TOOLS_LIBCRYPTO was a sufficient for that platform to build in
>> that setup (with this patch). And there are plenty of such platforms in the
>> U-Boot tree (all that are not marvell I think).
> 
> Right.  So your patch is a step in the right direction, but we also need
> to have the appropriate platforms depend'ing on TOOLS_LIBCRYPTO and
> updating the help to note that some platforms require it as well, to
> build their images.

I did not intend for TOOLS_LIBCRYPTO to be used beyond an on/off toggle 
for the user.

My suggestion is to allow platforms to build irrespective of 
TOOLS_LIBCRYPTO, and print a warning if for some reason we can't 
generate a bootable image. I see quite a few ARMv8 platforms throw such 
warnings on gitlab-ci. We can compile an elf, right? Any good reason why 
kwbimage should be different?

Alex

Re: [PATCH] tools: Do not build kwbimage if CONFIG_TOOLS_LIBCRYPTO=n

[Pali Rohár]

On Tuesday 11 January 2022 14:22:43 Alex G. wrote:
> On 1/11/22 13:09, Tom Rini wrote:
> > On Tue, Jan 11, 2022 at 07:58:05PM +0100, Marek Vasut wrote:
> > > On 1/11/22 17:16, Tom Rini wrote:
> > > > On Tue, Jan 11, 2022 at 04:36:34PM +0100, Pali Rohár wrote:
> > > > > On Tuesday 11 January 2022 16:31:20 Marek Vasut wrote:
> > > > > > The kwbimage has hard dependency on OpenSSL, do not build it
> > > > > > in case TOOLS_LIBCRYPTO is disabled.
> > > > > 
> > > > > This patch does not work as kwbimage is required for 32-bit Armada
> > > > > platforms. So kwbimage.o cannot be disabled on these platforms.
> > > > > 
> > > > > There is already proposal for fixing this issue:
> > > > > https://patchwork.ozlabs.org/project/uboot/patch/20211021093304.25399-1-pali@kernel.org/
> > > > 
> > > > And needs to be respun to not have Kconfig issues.  To answer something
> > > > noted in that thread, yes, it would be good if Kconfig did, or had an
> > > > option to make WARNING like that fatal.  Or is the problem really that
> > > > no, it's non-optional, really, to have OpenSSL installed?
> > > 
> > > OpenSSL should be optional, I got this bug report where someone tried to
> > > build u-boot on ancient debian oldoldstable with openssl 1.1.0 (without
> > > ecdsa support), for a platform which doesn't need any of this crypto stuff.
> > > So, disabling TOOLS_LIBCRYPTO was a sufficient for that platform to build in
> > > that setup (with this patch). And there are plenty of such platforms in the
> > > U-Boot tree (all that are not marvell I think).
> > 
> > Right.  So your patch is a step in the right direction, but we also need
> > to have the appropriate platforms depend'ing on TOOLS_LIBCRYPTO and
> > updating the help to note that some platforms require it as well, to
> > build their images.
> 
> I did not intend for TOOLS_LIBCRYPTO to be used beyond an on/off toggle for
> the user.
> 
> My suggestion is to allow platforms to build irrespective of
> TOOLS_LIBCRYPTO, and print a warning if for some reason we can't generate a
> bootable image. I see quite a few ARMv8 platforms throw such warnings on
> gitlab-ci. We can compile an elf, right? Any good reason why kwbimage should
> be different?
> 
> Alex

For me:

build = generate a bootable image

or at least I do not see a reason why an end user should be able to
generate e.g. cmd/common.o object file without having working full
toolchain for successful generation of final executable.

Some platforms (e.g. that Marvell) are not ELF-based, so compiling ELF
means nothing.

kwbimage for these Marvell platforms is what generates final
"executable" image and it does also other checks and tests. It is
possible that build process generates SPL ELF binary but kwbimage refuse
it. In most cases it means that user chose .config options unusable for
booting, but it could mean also other issues. And this is something
which CI tests can detect... if some patch is doing to break U-Boot on
Marvell platforms. On mailing list are pending other patches which
extends those kwbimage checks to prevent generating broken images
(e.g. chosen SPL load address cannot be set into kwbimage).

So printing a warning does not solve anything because warnings are
ignored. Lot of packaging tools and also CI tests pass successfully if
build process throw a warning.

And if some platforms are throwing warnings on CI, is not this an issue
which somebody should care about?

Re: [PATCH] tools: Do not build kwbimage if CONFIG_TOOLS_LIBCRYPTO=n

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 5091 bytes --]

On Tue, Jan 11, 2022 at 09:44:10PM +0100, Pali Rohár wrote:
> On Tuesday 11 January 2022 14:22:43 Alex G. wrote:
> > On 1/11/22 13:09, Tom Rini wrote:
> > > On Tue, Jan 11, 2022 at 07:58:05PM +0100, Marek Vasut wrote:
> > > > On 1/11/22 17:16, Tom Rini wrote:
> > > > > On Tue, Jan 11, 2022 at 04:36:34PM +0100, Pali Rohár wrote:
> > > > > > On Tuesday 11 January 2022 16:31:20 Marek Vasut wrote:
> > > > > > > The kwbimage has hard dependency on OpenSSL, do not build it
> > > > > > > in case TOOLS_LIBCRYPTO is disabled.
> > > > > > 
> > > > > > This patch does not work as kwbimage is required for 32-bit Armada
> > > > > > platforms. So kwbimage.o cannot be disabled on these platforms.
> > > > > > 
> > > > > > There is already proposal for fixing this issue:
> > > > > > https://patchwork.ozlabs.org/project/uboot/patch/20211021093304.25399-1-pali@kernel.org/
> > > > > 
> > > > > And needs to be respun to not have Kconfig issues.  To answer something
> > > > > noted in that thread, yes, it would be good if Kconfig did, or had an
> > > > > option to make WARNING like that fatal.  Or is the problem really that
> > > > > no, it's non-optional, really, to have OpenSSL installed?
> > > > 
> > > > OpenSSL should be optional, I got this bug report where someone tried to
> > > > build u-boot on ancient debian oldoldstable with openssl 1.1.0 (without
> > > > ecdsa support), for a platform which doesn't need any of this crypto stuff.
> > > > So, disabling TOOLS_LIBCRYPTO was a sufficient for that platform to build in
> > > > that setup (with this patch). And there are plenty of such platforms in the
> > > > U-Boot tree (all that are not marvell I think).
> > > 
> > > Right.  So your patch is a step in the right direction, but we also need
> > > to have the appropriate platforms depend'ing on TOOLS_LIBCRYPTO and
> > > updating the help to note that some platforms require it as well, to
> > > build their images.
> > 
> > I did not intend for TOOLS_LIBCRYPTO to be used beyond an on/off toggle for
> > the user.
> > 
> > My suggestion is to allow platforms to build irrespective of
> > TOOLS_LIBCRYPTO, and print a warning if for some reason we can't generate a
> > bootable image. I see quite a few ARMv8 platforms throw such warnings on
> > gitlab-ci. We can compile an elf, right? Any good reason why kwbimage should
> > be different?
> > 
> > Alex
> 
> For me:
> 
> build = generate a bootable image
> 
> or at least I do not see a reason why an end user should be able to
> generate e.g. cmd/common.o object file without having working full
> toolchain for successful generation of final executable.
> 
> Some platforms (e.g. that Marvell) are not ELF-based, so compiling ELF
> means nothing.

Almost nothing is ELF based.  So yes, Alex's point is right, in many
respects this is no different than the common ARMv8 case of needing
something we can't / don't provide to make a functional image.  Just
building u-boot (and spl/u-boot and/or tpl/u-boot) isn't enough and
isn't what's booted on the platform as-is.

> kwbimage for these Marvell platforms is what generates final
> "executable" image and it does also other checks and tests. It is
> possible that build process generates SPL ELF binary but kwbimage refuse
> it. In most cases it means that user chose .config options unusable for
> booting, but it could mean also other issues. And this is something
> which CI tests can detect... if some patch is doing to break U-Boot on
> Marvell platforms. On mailing list are pending other patches which
> extends those kwbimage checks to prevent generating broken images
> (e.g. chosen SPL load address cannot be set into kwbimage).
> 
> So printing a warning does not solve anything because warnings are
> ignored. Lot of packaging tools and also CI tests pass successfully if
> build process throw a warning.
> 
> And if some platforms are throwing warnings on CI, is not this an issue
> which somebody should care about?

What makes kwbimage different from the ARMv8 case is that in this case
we're talking about building the tool to make the image and in the other
case we're talking about using our tooling to generate (or be unable to
generate) a functional image.  We cannot ship TF-A or other assorted
external projects and their build outputs, so we throw a non-fatal but
obvious to anyone reading warning message out.  CI isn't even the issue
for kwbimage as we just include openssl-dev in the base.  The issues
are:
- How do we handle non-kwbimage platforms?  Especially since the general
  expectation is that mkimage will work for pretty much anything.  And
  not all distros / distro builders (OE) use tools-only_defconfig to
  configure their build of mkimage.
- How do we handle the case of kwbimage platforms on hosts that don't
  have openssl-dev?  This I think should be a build time error.  Failing
  to build over missing ssl headers should be a fairly easy to diagnose
  problem, and also should be documented as appropriate under doc/board/

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH] tools: Do not build kwbimage if CONFIG_TOOLS_LIBCRYPTO=n

[Marek Vasut]

On 1/11/22 22:14, Tom Rini wrote:
> The issues
> are:
> - How do we handle non-kwbimage platforms?  Especially since the general
>    expectation is that mkimage will work for pretty much anything.  And
>    not all distros / distro builders (OE) use tools-only_defconfig to
>    configure their build of mkimage.

The TOOLS_LIBCRYPTO is default y, I would keep it that way.

If someone really needs to build without openssl, they can turn this to 
=n if they understand the implications.

> - How do we handle the case of kwbimage platforms on hosts that don't
>    have openssl-dev?  This I think should be a build time error.  Failing
>    to build over missing ssl headers should be a fairly easy to diagnose
>    problem, and also should be documented as appropriate under doc/board/

mkimage -T kwbimage would fail if you don't have kwbimage built in (i.e. 
with TOOLS_LIBCRYPTO=n), so that's already handled.

[...]