Hi Alex,
On Tue, 11 May 2021 at 13:57, Alex G. <mr.nuke...@gmail.com> wrote:
>
> On 5/6/21 9:24 AM, Simon Glass wrote:
> > In preparation for enabling CONFIG_IS_ENABLED() on the host build, add
> > some options to enable the various FIT options expected in these tools.
> > This will ensure that the code builds correctly when CONFIG_HOST_xxx
> > is distinct from CONFIG_xxx.
> >
> > Signed-off-by: Simon Glass <s...@chromium.org>
>
> Reviewed-by: Alexandru Gagniuc <mr.nuke...@gmail.com>
>
> This makes me wonder whether we should just always enable host features.
> Right now, each defconfig can have a different mkimage config. So we
> should really have mkimage-imx8, mkimage-stm32mp, etc, which support
> different feature sets. This doesn't make much sense.
My intent is that we enable all features in host tools. For
distributions they build the tools-only config and make things work
that way. But perhaps we could avoid building the tools, or do it
separately, if there were no different between boards.
>
> The alternative is to get rid of all these configs and always enable
> mkimage features. The disadvantage is that we'd require openssl for
> building target code.
>
> A second alternative is to have a mkimage-nossl that gets built and used
> when openssl isn't available. It's really just openssl that causes such
> a schism.
Why is openssl such a problem?
>
> Alex
>
> > ---
> >
> > (no changes since v1)
> >
> > common/image-fit-sig.c | 3 ++-
> > common/image-fit.c | 4 ++--
> > tools/Kconfig | 25 +++++++++++++++++++++++++
> > tools/Makefile | 18 +++++++++---------
> > 4 files changed, 38 insertions(+), 12 deletions(-)
> >
> > diff --git a/common/image-fit-sig.c b/common/image-fit-sig.c
> > index 55ddf1879ed..12a6745c642 100644
> > --- a/common/image-fit-sig.c
> > +++ b/common/image-fit-sig.c
> > @@ -72,11 +72,12 @@ static int fit_image_setup_verify(struct
> > image_sign_info *info,
> > char *algo_name;
> > const char *padding_name;
> >
> > +#ifndef USE_HOSTCC
> > if (fdt_totalsize(fit) > CONFIG_FIT_SIGNATURE_MAX_SIZE) {
> > *err_msgp = "Total size too large";
> > return 1;
> > }
> > -
> > +#endif
> > if (fit_image_hash_get_algo(fit, noffset, &algo_name)) {
> > *err_msgp = "Can't get hash algo property";
> > return -1;
> > diff --git a/common/image-fit.c b/common/image-fit.c
> > index e614643fe39..a16e2dd54a5 100644
> > --- a/common/image-fit.c
> > +++ b/common/image-fit.c
> > @@ -165,7 +165,7 @@ int fit_get_subimage_count(const void *fit, int
> > images_noffset)
> > return count;
> > }
> >
> > -#if CONFIG_IS_ENABLED(FIT_PRINT) || CONFIG_IS_ENABLED(SPL_FIT_PRINT)
> > +#if CONFIG_IS_ENABLED(FIT_PRINT)
> > /**
> > * fit_image_print_data() - prints out the hash node details
> > * @fit: pointer to the FIT format image header
> > @@ -573,7 +573,7 @@ void fit_image_print(const void *fit, int
> > image_noffset, const char *p)
> > #else
> > void fit_print_contents(const void *fit) { }
> > void fit_image_print(const void *fit, int image_noffset, const char *p) {
> > }
> > -#endif /* CONFIG_IS_ENABLED(FIR_PRINT) || CONFIG_IS_ENABLED(SPL_FIT_PRINT)
> > */
> > +#endif /* CONFIG_IS_ENABLED(FIT_PRINT) */
> >
> > /**
> > * fit_get_desc - get node description property
> > diff --git a/tools/Kconfig b/tools/Kconfig
> > index b2f5012240c..f00ab661135 100644
> > --- a/tools/Kconfig
> > +++ b/tools/Kconfig
> > @@ -9,4 +9,29 @@ config MKIMAGE_DTC_PATH
> > some cases the system dtc may not support all required features
> > and the path to a different version should be given here.
> >
> > +config HOST_FIT
> > + def_bool y
> > + help
> > + Enable FIT support in the host build.
>
> Don't we always want to enable this for mkimage?
Yes, that's right.
>
> > +
> > +config HOST_FIT_FULL_CHECK
> > + def_bool y
> > + help
> > + Do a full check of the FIT before using it in the host build
>
> How would this be used? FIT vs FIT_FULL is mostly an SPL distinction. I
> don't think we should have it in host tools.
It allows us to use CONFIG_IS_ENABLED() everywhere, including in code
built as part of host tools. In fact that is the main purpose of this
series.
>
> > +
> > +config HOST_FIT_PRINT
> > + def_bool y
> > + help
> > + Print the content of the FIT verbosely in the host build
>
> This option also doesn't make sense.This seems to do what 'mkimage -l'
> already supports.
Are you sure you have looked at the goal of the CONFIG_IS_ENABLED()
changes? This is here purely to avoid #ifdefs in the share code.
>
> > +
> > +config HOST_FIT_SIGNATURE
> > + def_bool y
> > + help
> > + Enable signature verification of FIT uImages in the host build
>
> s/verification/signing and verification/
OK, yes it does control that in the tools, by virtue of tools/Makefile
>
> > +
> > +config HOST_FIT_SIGNATURE_MAX_SIZE
> > + hex
> > + depends on HOST_FIT_SIGNATURE
> > + default 0x10000000
> > +
>
> The only use of FIT_SIGNATURE_MAX_SIZE is under "#ifndef USE_HOSTCC". So
> this wouldn't make any sense for the host.
This is used in fit_image_setup_verify() which is build by tools.
[..]
Regards,
Simon
