[PATCH 06/18] image: Drop IMAGE_ENABLE_SHA1

[Simon Glass]

Hi Alex,

On Thu, 20 May 2021 at 18:07, Alex G. <mr.nuke.me at 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 at 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 at 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 at 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 at gmail.com> wrote:
> >>>>>>>>
> >>>>>>>> From: Simon Glass <sjg at 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 at chromium.org>
> >>>>>>>> Reviewed-by: Alexandru Gagniuc <mr.nuke.me at gmail.com>
> >>>>>>>> Signed-off-by: Alexandru Gagniuc <mr.nuke.me at 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).

Tom, do you have any thoughts?

Regards,
SImon