Hi Alex,
On Mon, 24 May 2021 at 13:59, Alex G. mr.nuke.me@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.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?
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