From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id BA2D8C433F5 for ; Wed, 20 Oct 2021 13:48:05 +0000 (UTC) Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id E603961260 for ; Wed, 20 Oct 2021 13:48:04 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org E603961260 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=lists.denx.de Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 190168048A; Wed, 20 Oct 2021 15:48:03 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=kernel.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=kernel.org header.i=@kernel.org header.b="d5kw09kX"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 30BB58314B; Wed, 20 Oct 2021 15:48:01 +0200 (CEST) Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id EB99A80215 for ; Wed, 20 Oct 2021 15:47:56 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=kernel.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=pali@kernel.org Received: by mail.kernel.org (Postfix) with ESMTPSA id 1CDC560F24; Wed, 20 Oct 2021 13:47:55 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1634737675; bh=ptU5uLOI3NU2yovHcsyXS7U8NWZd3Jc5XOPKGmdv9W0=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=d5kw09kX9E/sJpOn+Xo5sOuI76PAoz3aq1N+5/iR27WwC0oiF1CiN1s7PNw3WZUqC 5n5FnnUdQsY0IZu31erWqz59qCsKDdI9aQWRcfEmzOIhazjM7Asv1UJOmkSoO+2+9V SiCwScZ2B/CX7XvtljsvtiAHTEKGhxsFD6NZZypVleaFrshsaQIfX6zzlLRDwN+9cI iyM6Knt7DbYgtueGLmWto515YaXVoZ6bUdi/tkmI3qRFdEJP2vurXH+1BaI6ospWX+ bhU+iTOP6He3ewGTUd1JBb+hrmCAec0NCu+jEy7Uk5na7WwkZn3JCvNpkxEPHjTIwD uBuKj5qcSy4xw== Received: by pali.im (Postfix) id CCE96883; Wed, 20 Oct 2021 15:47:52 +0200 (CEST) Date: Wed, 20 Oct 2021 15:47:52 +0200 From: Pali =?utf-8?B?Um9ow6Fy?= To: Andre Przywara Cc: Samuel Holland , u-boot@lists.denx.de, Jagan Teki , "Alex G ." , Artem Lapkin , Priyanka Jain , Sughosh Ganu , Marek =?utf-8?B?QmVow7pu?= Subject: Re: [PATCH v4 1/4] tools: Separate image types which depend on OpenSSL Message-ID: <20211020134752.62k4fxukucj5rodh@pali> References: <20211020024455.48136-1-samuel@sholland.org> <20211020024455.48136-2-samuel@sholland.org> <20211020072925.drf6622qhq4yykg6@pali> <20211020142902.12219c45@donnerap.cambridge.arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20211020142902.12219c45@donnerap.cambridge.arm.com> User-Agent: NeoMutt/20180716 X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.34 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.2 at phobos.denx.de X-Virus-Status: Clean On Wednesday 20 October 2021 14:29:02 Andre Przywara wrote: > On Wed, 20 Oct 2021 09:29:25 +0200 > Pali Rohár wrote: > > Hi, > > > On Tuesday 19 October 2021 21:44:51 Samuel Holland wrote: > > > Some image types (kwbimage and mxsimage) always depend on OpenSSL, so > > > they can only be included in mkimage when TOOLS_LIBCRYPTO is selected. > > > Use Makefile logic to conditionally link the files. > > > > > > Signed-off-by: Samuel Holland > > > > NAK. > > > > As explained in previous email [1], kwbimage is required for building > > Kirkwood, Dove, A370, AXP, A375, A38x, A39x and MSYS platforms. > > Therefore it cannot be disabled or hidden behind some user config > > options for these platforms (and it does not matter if it is crypto > > option or any other option). > > So somehow we need to find a solution between your view and Alex' view of > things. > First: Pali, do you see any actual problem at the moment? TOOLS_LIBCRYPTO > defaults to y, so if I am not mistaken that means that a user would need > to deliberately turn that off to trigger build errors? > And also: the situation with this patch is not worse as before, isn't it? I do not think so. Without this patch, kwbimage is always going compiled, which means it is compiled also for mvebu platforms which needs it. With this patch it is possible via _valid_ config options to disable compilation of kwbimage which would lead to totally bogus and unintuitive behavior and errors. Without this patch compilation fails on exact and clear error = cryto header files (or libs) were not found and user / developer / package maintainer would know what is needed (= mean to install missing dependency). > So if it's just the missing improvement that you are concerned about, I am > not sure that should block this patch? V3 patch was improvement - it enforced required dependencies and I guess it showed some build system error message when dependences were not met. This is improvement from situation without patch when it failed on compiling kwbimage which probably showed error message about missing header files (or libs, depends on which stage it failed). It was not the best choice as in some cases it enforced crypto dependencies also in cases when they were not really required -- but it should have solve the problem that dependences are there for platforms which require them. v4 patch is not improvement, it is step backward - it started allowing to compile mkimage without required functionality, even on platforms which needs them, and effectively hides issue which is there. > I mean you could always propose > your own version of that missing piece, to improve the situation for the > boards you care about? And we should have this discussion there? I think I have written details and some proposed solution in that email. What is needed is to ensure that kwbimage is always compiled for platforms which require it (list of platform are in email 1 in previous email). This can be done as hard dependency like it is currently without this patch (ugly, does not show nice error message and enforce everybody to have crypto dependency, even platforms which do not need it). Other solution could be to define some select symbol which says that kwbimage is required and then kwbimage would be unconditionally compiled when this symbol is selected. And to make error message nice in build system, this symbol could depends on crypto symbol to ensure that all dependencies are met when trying to compile something which needs it. Another option could be to implement required crypto functions directly in u-boot source tree to remove external dependency. I do not know which solution is the best or how hard they are to implement, and neither what can be accepted by other u-boot developers / maintainers. I see an issue here that fixing this problem need to touch more parts of u-boot source code and build system which, which means that u-boot maintainers need to say what they are willing to accept and what not. > As it stands right now, this patch just improves things (just not > *everything*), and it is a prerequisite for the rest of the series > (unrelated to your problems), so I would like to go ahead on this one. Well, maybe this patch improves some things about non-mvebu platforms, but makes mvebu platform code / build system worse. And due to this fact I cannot say that I want this change... > Cheers, > Andre. > > > kwbimage must be unconditionally enabled on > > these platforms like it was before this change, as it is crucial part of > > build. > > > > [1] - https://lore.kernel.org/u-boot/20211015114735.rig3e4cuc7mn6a7e@pali/ > > > > > --- > > > > > > Changes in v4: > > > - Do not select TOOLS_LIBCRYPTO anywhere > > > > > > Changes in v3: > > > - Selected TOOLS_LIBCRYPTO on all platforms that use kwbimage (as best > > > as I can tell, using the suggestions from Pali Rohár) > > > > > > Changes in v2: > > > - Refactored the first patch on top of TOOLS_LIBCRYPTO > > > > > > scripts/config_whitelist.txt | 1 - > > > tools/Makefile | 19 +++++-------------- > > > tools/mxsimage.c | 3 --- > > > 3 files changed, 5 insertions(+), 18 deletions(-) > > > > > > diff --git a/scripts/config_whitelist.txt b/scripts/config_whitelist.txt > > > index cd94b5777a..affae6875d 100644 > > > --- a/scripts/config_whitelist.txt > > > +++ b/scripts/config_whitelist.txt > > > @@ -828,7 +828,6 @@ CONFIG_MXC_UART_BASE > > > CONFIG_MXC_USB_FLAGS > > > CONFIG_MXC_USB_PORT > > > CONFIG_MXC_USB_PORTSC > > > -CONFIG_MXS > > > CONFIG_MXS_AUART > > > CONFIG_MXS_AUART_BASE > > > CONFIG_MXS_OCOTP > > > diff --git a/tools/Makefile b/tools/Makefile > > > index 999fd46531..a9b3d982d8 100644 > > > --- a/tools/Makefile > > > +++ b/tools/Makefile > > > @@ -94,9 +94,11 @@ ECDSA_OBJS-$(CONFIG_TOOLS_LIBCRYPTO) := $(addprefix lib/ecdsa/, ecdsa-libcrypto. > > > AES_OBJS-$(CONFIG_TOOLS_LIBCRYPTO) := $(addprefix lib/aes/, \ > > > aes-encrypt.o aes-decrypt.o) > > > > > > -# Cryptographic helpers that depend on openssl/libcrypto > > > -LIBCRYPTO_OBJS-$(CONFIG_TOOLS_LIBCRYPTO) := $(addprefix lib/, \ > > > - fdt-libcrypto.o) > > > +# Cryptographic helpers and image types that depend on openssl/libcrypto > > > +LIBCRYPTO_OBJS-$(CONFIG_TOOLS_LIBCRYPTO) := \ > > > + lib/fdt-libcrypto.o \ > > > + kwbimage.o \ > > > + mxsimage.o > > > > > > ROCKCHIP_OBS = lib/rc4.o rkcommon.o rkimage.o rksd.o rkspi.o > > > > > > @@ -118,10 +120,8 @@ dumpimage-mkimage-objs := aisimage.o \ > > > imximage.o \ > > > imx8image.o \ > > > imx8mimage.o \ > > > - kwbimage.o \ > > > lib/md5.o \ > > > lpc32xximage.o \ > > > - mxsimage.o \ > > > omapimage.o \ > > > os_support.o \ > > > pblimage.o \ > > > @@ -156,22 +156,13 @@ fit_info-objs := $(dumpimage-mkimage-objs) fit_info.o > > > fit_check_sign-objs := $(dumpimage-mkimage-objs) fit_check_sign.o > > > file2include-objs := file2include.o > > > > > > -ifneq ($(CONFIG_MX23)$(CONFIG_MX28)$(CONFIG_TOOLS_LIBCRYPTO),) > > > -# Add CONFIG_MXS into host CFLAGS, so we can check whether or not register > > > -# the mxsimage support within tools/mxsimage.c . > > > -HOSTCFLAGS_mxsimage.o += -DCONFIG_MXS > > > -endif > > > - > > > ifdef CONFIG_TOOLS_LIBCRYPTO > > > # This affects include/image.h, but including the board config file > > > # is tricky, so manually define this options here. > > > HOST_EXTRACFLAGS += -DCONFIG_FIT_SIGNATURE > > > HOST_EXTRACFLAGS += -DCONFIG_FIT_SIGNATURE_MAX_SIZE=0xffffffff > > > HOST_EXTRACFLAGS += -DCONFIG_FIT_CIPHER > > > -endif > > > > > > -# MXSImage needs LibSSL > > > -ifneq ($(CONFIG_MX23)$(CONFIG_MX28)$(CONFIG_ARMADA_38X)$(CONFIG_TOOLS_LIBCRYPTO),) > > > HOSTCFLAGS_kwbimage.o += \ > > > $(shell pkg-config --cflags libssl libcrypto 2> /dev/null || echo "") > > > HOSTLDLIBS_mkimage += \ > > > diff --git a/tools/mxsimage.c b/tools/mxsimage.c > > > index 002f4b525a..2bfbb421eb 100644 > > > --- a/tools/mxsimage.c > > > +++ b/tools/mxsimage.c > > > @@ -5,8 +5,6 @@ > > > * Copyright (C) 2012-2013 Marek Vasut > > > */ > > > > > > -#ifdef CONFIG_MXS > > > - > > > #include > > > #include > > > #include > > > @@ -2363,4 +2361,3 @@ U_BOOT_IMAGE_TYPE( > > > NULL, > > > mxsimage_generate > > > ); > > > -#endif > > > -- > > > 2.32.0 > > > >