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 X-Spam-Level: X-Spam-Status: No, score=-16.0 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3182AC2B9F4 for ; Tue, 22 Jun 2021 13:31:46 +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 3C285613AD for ; Tue, 22 Jun 2021 13:31:45 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 3C285613AD Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=chromium.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 75194829B8; Tue, 22 Jun 2021 15:31:42 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=chromium.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (1024-bit key; unprotected) header.d=chromium.org header.i=@chromium.org header.b="XPTWAACQ"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id D2FDE829B2; Tue, 22 Jun 2021 15:31:40 +0200 (CEST) Received: from mail-wr1-x430.google.com (mail-wr1-x430.google.com [IPv6:2a00:1450:4864:20::430]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 7ECE7828EB for ; Tue, 22 Jun 2021 15:31:36 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=chromium.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=sjg@google.com Received: by mail-wr1-x430.google.com with SMTP id b3so13424560wrm.6 for ; Tue, 22 Jun 2021 06:31:36 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=BT9IjFd7xeb8gxRNrrARJLNbLhL6I6k0hGrqKJLr4fU=; b=XPTWAACQ1rrGMhidxnq9gxPjTnA/gxey5h95RUhzsMV+Awv7yveFc3U/MkT+oegpD/ Ty3oBe1llYZuF6LGyydYTMYBf8Pb9/xdV8Sj24FmnNXcLl2LPAMpfws/CYtHK4FG+GhO 5rLp4fRzv7ppcmLXVHiPzFzHOgV+5RS7F05pQ= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=BT9IjFd7xeb8gxRNrrARJLNbLhL6I6k0hGrqKJLr4fU=; b=EYcHvBzGcN3xAdkj2aAnR/Au1pZdI+qiOJ/z/Pzm4EYN2no/coMvARpEaQx4fJthNA kXprZs6xglQUi7wB8yfUJemMUhx7tcbuqKmbNDD6/Zls8nPgntH4ZlTqnkBNzcmjsEWx USckuA3dZJbF73JHI2/XHV9MU+3iJJiE5pntuK/JIFzLl5Dxi3ms25x+UMCFy0k5r5sl ezZTqyE7HaPAqvDwUj1hgrJIPCepPYU8Ciu4bAlBao7QF521STROaKKCmJOOhgXJ2UmE 6W3edybU4w+WI9ynlwWA5Qi5mcNcK4l8OqgQAcmFuBxZ5ttmuARPsSVpqiHhYxPAmykG hYGQ== X-Gm-Message-State: AOAM533Fqmi7+M8NwTCTivGBdVzSQWSqpvIsi6s78FChCX0QBT7cBaMQ KHCm3o9WYHkSc8kF6ca4rydYtSGkTS90gW98r0OzXNgi/m4= X-Google-Smtp-Source: ABdhPJzj30kyny6MGhf0GsnB7MnNJMuPZLfaflqRrebVKccH5rYQBqgxF11GKIFhwFsCcX4kVeI871kgsFR3vA4gsWI= X-Received: by 2002:a5d:4f0b:: with SMTP id c11mr4891114wru.56.1624368695734; Tue, 22 Jun 2021 06:31:35 -0700 (PDT) MIME-Version: 1.0 References: <20210517163840.839097-1-mr.nuke.me@gmail.com> <20210517163840.839097-7-mr.nuke.me@gmail.com> <9b8e856a-d5e7-0565-1365-f600df61be4d@gmail.com> <4810e21e-004f-ef3d-b833-467a68afa5bf@gmail.com> <9b8e8725-9538-56c9-ee9e-8c4aea34b273@gmail.com> <5f2121e9-1f32-943f-5e88-f3a98b2d0a0d@gmail.com> In-Reply-To: <5f2121e9-1f32-943f-5e88-f3a98b2d0a0d@gmail.com> From: Simon Glass Date: Tue, 22 Jun 2021 07:31:22 -0600 Message-ID: Subject: Re: [PATCH 06/18] image: Drop IMAGE_ENABLE_SHA1 To: "Alex G." Cc: U-Boot Mailing List , Tom Rini , Rasmus Villemoes Content-Type: text/plain; charset="UTF-8" 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 Hi Alex, On Mon, 24 May 2021 at 13:59, Alex G. wrote: > > > > On 5/21/21 2:39 PM, Simon Glass wrote: > > Hi Alex, > > > > On Thu, 20 May 2021 at 18:07, Alex G. wrote: > >> > >> > >> > >> On 5/20/21 6:17 PM, Simon Glass wrote: > >>> Hi Alex, > >>> > >>> On Thu, 20 May 2021 at 17:13, Alex G. wrote: > >>>> > >>>> > >>>> > >>>> On 5/20/21 12:52 PM, Simon Glass wrote: > >>>>> Hi Alex, > >>>>> > >>>>> On Wed, 19 May 2021 at 20:41, Alex G. wrote: > >>>>>> > >>>>>> > >>>>>> > >>>>>> On 5/19/21 4:55 PM, Simon Glass wrote: > >>>>>>> Hi Alex, > >>>>>>> > >>>>>>> On Wed, 19 May 2021 at 11:44, Alex G wrote: > >>>>>>>> > >>>>>>>> > >>>>>>>> > >>>>>>>> On 5/19/21 11:36 AM, Simon Glass wrote: > >>>>>>>>> Hi Alexandru, > >>>>>>>>> > >>>>>>>>> On Mon, 17 May 2021 at 10:38, Alexandru Gagniuc wrote: > >>>>>>>>>> > >>>>>>>>>> From: Simon Glass > >>>>>>>>>> > >>>>>>>>>> We already have a host Kconfig for SHA1. Use CONFIG_IS_ENABLED(SHA1) > >>>>>>>>>> directly in the code shared with the host build, so we can drop the > >>>>>>>>>> unnecessary indirection. > >>>>>>>>>> > >>>>>>>>>> Signed-off-by: Simon Glass > >>>>>>>>>> Reviewed-by: Alexandru Gagniuc > >>>>>>>>>> Signed-off-by: Alexandru Gagniuc > >>>>>>>>>> --- > >>>>>>>>>> common/image-fit.c | 2 +- > >>>>>>>>>> include/image.h | 8 -------- > >>>>>>>>>> 2 files changed, 1 insertion(+), 9 deletions(-) > >>>>>>>>>> > >>>>>>>>>> diff --git a/common/image-fit.c b/common/image-fit.c > >>>>>>>>>> index e614643fe3..24e92a8e92 100644 > >>>>>>>>>> --- a/common/image-fit.c > >>>>>>>>>> +++ b/common/image-fit.c > >>>>>>>>>> @@ -1218,7 +1218,7 @@ int calculate_hash(const void *data, int data_len, const char *algo, > >>>>>>>>>> CHUNKSZ_CRC32); > >>>>>>>>>> *((uint32_t *)value) = cpu_to_uimage(*((uint32_t *)value)); > >>>>>>>>>> *value_len = 4; > >>>>>>>>>> - } else if (IMAGE_ENABLE_SHA1 && strcmp(algo, "sha1") == 0) { > >>>>>>>>>> + } else if (CONFIG_IS_ENABLED(SHA1) && strcmp(algo, "sha1") == 0) { > >>>>>>>>> > >>>>>>>>> This can only work if the my host Kconfig patch comes first. Otherwise > >>>>>>>>> this code will just be skipped on the host. > >>>>>>>> > >>>>>>>> I was scratching my head too as to why this works in practice, but not > >>>>>>>> in theory. There is a #define CONFIG_SHA1 in image.h. > >>>>>>>> > >>>>>>>> Although not a perfect fix, we go from two ways to enable SHA1 ("#define > >>>>>>>> IMAGE_ENABLE_SHA1", and "#define CONFIG_SHA1"), to just one. That's why > >>>>>>>> I think this change is an improvement, and part of this series. > >>>>>>> > >>>>>>> No, we really should not do that...everything needs to be in Kconfig. > >>>>>> > >>>>>> I agree for target code. But, as a long term solution, let's look at how > >>>>>> we can get hash algos in linker lists, like we're proposing to do for > >>>>>> crytpo algos. Or I could just drop this change in v2. > >>>>> > >>>>> Would it not be easier to have a host Kconfig for these? You seem to > >>>>> be going to extreme lengths to avoid it, but it seems like the > >>>>> simplest solution, easy to understand, no effect on code size and > >>>>> scalable to the future. > >>>> > >>>> It's easy for the short term in terms if the goal is to get something > >>>> merged. It just hides more fundamental issues with the code. For > >>>> ecample, why is there hash_calculate() and clacultae_hash() > >>> > >>> It is just a naming issue, isn't it? They are quite different functions. > >> > >> Because one resets the watchdog after every CHUNK bytes and the other > >> doesn't? > > > > Well hash_calculate() is used for hashing parts of a devicetree, so is > > quite a different function. > > > >> > >>>> > >>>> I was under the impression that we were agreed on the combination of > >>>> patches. I won't try to defend your patch from yourself. I'll drop the > >>>> hash changes from v2 if it helps get things moving along. > >>> > >>> I'm OK with this as a short-term fix to get this series through. But I > >>> think we are going to end up with a Kconfig solution at some point. > >>> What do you think? > >> > >> I think it's possible and reasonable to have common code without host > >> Kconfig. coreboot did it. > > > > There is very little code shared between target and tools there. I am > > sure there is some, but can't think of anything except some library > > functions. There is also no equivalent of CONFIG_IS_ENABLED(), but > > instead a log of ENV_... macros and entirely separate rules in the > > Makefile. > > > > Can you point to another way to do this? I think your approach is > > valuable in untangling code that does not need to be shared, but I > > still think that the host Kconfig thing is a great idea for everything > > else. It isn't my idea, but Rasmus' (now on cc). > > I have a couple of ideas to start, but nothing submittable. > > Let's start with hash_calculate(). It's just a big switch() with string > processing. But we've already done part of the work in > fit_image_check_hash(). So instead of stopping at a "char *algo", why > not get an actual "struct hash_algo *" with the correct ops to begin > with, and not need a huge switch() function() ? > > We have "struct hash_algo" and "struct checksum_algo" that seem to serve > very similar purposes. Would it actually make sense to merge them? I'm not convinced it is. The checksum_algo thing allows checksumming lots of regions. We could perhaps have a helper function that reads each region and runs it through the hash_algo API. Then we could drop checksum_algo perhaps? Of course we still have the calculate_sign() call but I presume you could make that host-only. > > And if that is done, you open the gates to significantly reducing the > CONFIG_IS_ENABLED() use for hashes, as well as really simplify the hash > selection in Kconfig. > > In order to do this, we are reducing the occurrence of IS_ENABLED(), > which is just an eye-candy version of #ifdef. This leads to the natural > conclusion of eliminating IS_ENABLED() from common code. Well if you are going to do this, let's see the patches. What's the plan for getting rid of the '#define CONFIG_xxxx' on the host side? Regards, Simon