Re: [PATCH 06/18] image: Drop IMAGE_ENABLE_SHA1

["Alex G." <mr.nuke.me@gmail.com>]

On 5/21/21 2:39 PM, Simon Glass wrote:
> Hi Alex,
> 
> On Thu, 20 May 2021 at 18:07, Alex G. <mr.nuke.me@gmail.com> wrote:
>>
>>
>>
>> On 5/20/21 6:17 PM, Simon Glass wrote:
>>> Hi Alex,
>>>
>>> On Thu, 20 May 2021 at 17:13, Alex G. <mr.nuke.me@gmail.com> wrote:
>>>>
>>>>
>>>>
>>>> On 5/20/21 12:52 PM, Simon Glass wrote:
>>>>> Hi Alex,
>>>>>
>>>>> On Wed, 19 May 2021 at 20:41, Alex G. <mr.nuke.me@gmail.com> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 5/19/21 4:55 PM, Simon Glass wrote:
>>>>>>> Hi Alex,
>>>>>>>
>>>>>>> On Wed, 19 May 2021 at 11:44, Alex G <mr.nuke.me@gmail.com> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 5/19/21 11:36 AM, Simon Glass wrote:
>>>>>>>>> Hi Alexandru,
>>>>>>>>>
>>>>>>>>> On Mon, 17 May 2021 at 10:38, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote:
>>>>>>>>>>
>>>>>>>>>> From: Simon Glass <sjg@chromium.org>
>>>>>>>>>>
>>>>>>>>>> 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 <sjg@chromium.org>
>>>>>>>>>> Reviewed-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
>>>>>>>>>> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
>>>>>>>>>> ---
>>>>>>>>>>       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.