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=-14.3 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 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 DB245C04FF3 for ; Mon, 24 May 2021 20:00:12 +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 2BD3C613E1 for ; Mon, 24 May 2021 20:00:12 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 2BD3C613E1 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com 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 F236582EA7; Mon, 24 May 2021 22:00:07 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=gmail.com 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=gmail.com header.i=@gmail.com header.b="XYHlsa9A"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id D84CE80F0A; Mon, 24 May 2021 22:00:00 +0200 (CEST) Received: from mail-oi1-x233.google.com (mail-oi1-x233.google.com [IPv6:2607:f8b0:4864:20::233]) (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 267B180F0A for ; Mon, 24 May 2021 21:59:57 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=mr.nuke.me@gmail.com Received: by mail-oi1-x233.google.com with SMTP id x15so28087218oic.13 for ; Mon, 24 May 2021 12:59:57 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=mHB3bCwanhI21qiWPphD7Xon/tofWg064ZpVxZ7L+c8=; b=XYHlsa9AsbdbHj+6CzjMq4HV69WcR5eIgTmMRgW6Pnw2ZqtwaG9TRH2VX8iAcgQe1g XIEugJJtGh0uNmeYjJhwxtAWOiRrY9oizgEG0sR88BLag5ecPPJRW3w6BdoAiLO9J76v 8FwrWIDL7o2ZGVkt9HETQ+vwsjINwQc6qx5X6E4laqjImkcnJgy0p0E3NIA2GMMVVJb4 4oeoUkSEhKpNmEV0A5WoF3VsnYmpBtE2S9xMpaCdBRTnd5uzkZyYXsf0GUINAN4KzKuQ l5y2Y3ISWEildz8QbVPg8c/EPB1M48h2lYvuUgb9hRq3bmeb+8LHxz6zaRmMFIwbLKCA cOkA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=mHB3bCwanhI21qiWPphD7Xon/tofWg064ZpVxZ7L+c8=; b=kRcyIiwB2qNEgy9wKVy2/W7CrC66uW1a8OmrMcJ5Tjme9lPnzfiS4zuDze+GYRQMWf RleKarPZOtG2AI3rsbC0IfrxAYu8cHap1MZVIP6ue3ly12Hfq8hl3h7CYgJhtzokrrxE YA6npzTXW4mOMyfdIeIrtBn34WJE9K1EtAXH+v3YNCIVCw+VnJTniOYv7CWieD/Azk3v COjVcNNtqvdevEKoa+iO45cHc4Lw9jmA2pO+wPKD/1RSL+TOw0qdHCfUTV+/feFT2DWT Ti8L/Gy3HDYSPncX57A1Dk+IKfbEQ2B+iX/aJXHRJAHf2ORTotaetTakWtbufiztDg1I C82g== X-Gm-Message-State: AOAM533sUF2rYy0J51k+QtLC97du+FsiFiSXPMDdnCjhj2nWor2jN7ny MbTAyDw8zlDeR/l6Rybx5HB9R5dG94Suxg== X-Google-Smtp-Source: ABdhPJxi54uoMBpWy/yrtOHkiQSSYEUJWZ29MoIDEpUo7VK79qJEATYNmr8hTBYHyC9EfyW3pesFVQ== X-Received: by 2002:a05:6808:1401:: with SMTP id w1mr11265098oiv.52.1621886395483; Mon, 24 May 2021 12:59:55 -0700 (PDT) Received: from nuclearis3.gtech (c-98-195-139-126.hsd1.tx.comcast.net. [98.195.139.126]) by smtp.gmail.com with ESMTPSA id q22sm3203140otl.11.2021.05.24.12.59.54 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 24 May 2021 12:59:55 -0700 (PDT) Subject: Re: [PATCH 06/18] image: Drop IMAGE_ENABLE_SHA1 To: Simon Glass Cc: U-Boot Mailing List , Tom Rini , Rasmus Villemoes 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> From: "Alex G." Message-ID: <5f2121e9-1f32-943f-5e88-f3a98b2d0a0d@gmail.com> Date: Mon, 24 May 2021 14:59:54 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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.102.4 at phobos.denx.de X-Virus-Status: Clean 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? 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.