Hi Alex,
On Mon, 24 May 2021 at 13:59, Alex G. <mr.nuke...@gmail.com> wrote:
>
>
>
> On 5/21/21 2:39 PM, Simon Glass wrote:
> > Hi Alex,
> >
> > On Thu, 20 May 2021 at 18:07, Alex G. <mr.nuke...@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...@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...@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...@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...@gmail.com> wrote:
> >>>>>>>>>>
> >>>>>>>>>> From: Simon Glass <s...@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 <s...@chromium.org>
> >>>>>>>>>> Reviewed-by: Alexandru Gagniuc <mr.nuke...@gmail.com>
> >>>>>>>>>> Signed-off-by: Alexandru Gagniuc <mr.nuke...@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?
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
