| Message ID | 20210506082420.v2.16.I64826ed33219988294468df7b95dfa3fffd7a0a1@changeid |
|---|---|
| State | Deferred |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | image: Reduce #ifdefs and ad-hoc defines in image code | expand |
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 <sjg@chromium.org> Reviewed-by: Alexandru Gagniuc <mr.nuke.me@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. 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. 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? > + > +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. > + > +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. > + > +config HOST_FIT_SIGNATURE > + def_bool y > + help > + Enable signature verification of FIT uImages in the host build s/verification/signing and verification/ > + > +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. > endmenu > diff --git a/tools/Makefile b/tools/Makefile > index 2b4bc547abd..d143198f7c9 100644 > --- a/tools/Makefile > +++ b/tools/Makefile > @@ -53,12 +53,12 @@ hostprogs-y += mkenvimage > mkenvimage-objs := mkenvimage.o os_support.o lib/crc32.o > > hostprogs-y += dumpimage mkimage > -hostprogs-$(CONFIG_FIT_SIGNATURE) += fit_info fit_check_sign > +hostprogs-$(CONFIG_HOST_FIT_SIGNATURE) += fit_info fit_check_sign > > hostprogs-$(CONFIG_CMD_BOOTEFI_SELFTEST) += file2include > > -FIT_OBJS-$(CONFIG_FIT) := fit_common.o fit_image.o image-host.o common/image-fit.o > -FIT_SIG_OBJS-$(CONFIG_FIT_SIGNATURE) := common/image-sig.o common/image-fit-sig.o > +FIT_OBJS-$(CONFIG_HOST_FIT) := fit_common.o fit_image.o image-host.o common/image-fit.o > +FIT_SIG_OBJS-$(CONFIG_HOST_FIT_SIGNATURE) := common/image-sig.o common/image-fit-sig.o > FIT_CIPHER_OBJS-$(CONFIG_FIT_CIPHER) := common/image-cipher.o > > # The following files are synced with upstream DTC. > @@ -66,17 +66,17 @@ FIT_CIPHER_OBJS-$(CONFIG_FIT_CIPHER) := common/image-cipher.o > LIBFDT_OBJS := $(addprefix libfdt/, fdt.o fdt_ro.o fdt_wip.o fdt_sw.o fdt_rw.o \ > fdt_strerror.o fdt_empty_tree.o fdt_addresses.o fdt_overlay.o) > > -RSA_OBJS-$(CONFIG_FIT_SIGNATURE) := $(addprefix lib/rsa/, \ > +RSA_OBJS-$(CONFIG_HOST_FIT_SIGNATURE) := $(addprefix lib/rsa/, \ > rsa-sign.o rsa-verify.o \ > rsa-mod-exp.o) > > -ECDSA_OBJS-$(CONFIG_FIT_SIGNATURE) := $(addprefix lib/ecdsa/, ecdsa-libcrypto.o) > +ECDSA_OBJS-$(CONFIG_HOST_FIT_SIGNATURE) := $(addprefix lib/ecdsa/, ecdsa-libcrypto.o) > > AES_OBJS-$(CONFIG_FIT_CIPHER) := $(addprefix lib/aes/, \ > aes-encrypt.o aes-decrypt.o) > > # Cryptographic helpers that depend on openssl/libcrypto > -LIBCRYPTO_OBJS-$(CONFIG_FIT_SIGNATURE) := $(addprefix lib/, \ > +LIBCRYPTO_OBJS-$(CONFIG_HOST_FIT_SIGNATURE) := $(addprefix lib/, \ > fdt-libcrypto.o) > > ROCKCHIP_OBS = lib/rc4.o rkcommon.o rkimage.o rksd.o rkspi.o > @@ -137,13 +137,13 @@ fit_info-objs := $(dumpimage-mkimage-objs) fit_info.o > fit_check_sign-objs := $(dumpimage-mkimage-objs) fit_check_sign.o > file2include-objs := file2include.o > > -ifneq ($(CONFIG_MX23)$(CONFIG_MX28)$(CONFIG_FIT_SIGNATURE),) > +ifneq ($(CONFIG_MX23)$(CONFIG_MX28)$(CONFIG_HOST_FIT_SIGNATURE),) > # Add CONFIG_MXS into host CFLAGS, so we can check whether or not register > # the mxsimage support within tools/mxsimage.c . > HOSTCFLAGS_mxsimage.o += -DCONFIG_MXS > endif > > -ifdef CONFIG_FIT_SIGNATURE > +ifdef CONFIG_HOST_FIT_SIGNATURE > # This affects include/image.h, but including the board config file > # is tricky, so manually define this options here. > HOST_EXTRACFLAGS += -DCONFIG_FIT_SIGNATURE > @@ -165,7 +165,7 @@ HOSTCFLAGS_kwbimage.o += -DCONFIG_KWB_SECURE > endif > > # MXSImage needs LibSSL > -ifneq ($(CONFIG_MX23)$(CONFIG_MX28)$(CONFIG_ARMADA_38X)$(CONFIG_FIT_SIGNATURE)$(CONFIG_FIT_CIPHER),) > +ifneq ($(CONFIG_MX23)$(CONFIG_MX28)$(CONFIG_ARMADA_38X)$(CONFIG_HOST_FIT_SIGNATURE)$(CONFIG_FIT_CIPHER),) > HOSTCFLAGS_kwbimage.o += \ > $(shell pkg-config --cflags libssl libcrypto 2> /dev/null || echo "") > HOSTLDLIBS_mkimage += \ >
On Tue, May 11, 2021 at 02:57:03PM -0500, Alex G. 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 <sjg@chromium.org> > > Reviewed-by: Alexandru Gagniuc <mr.nuke.me@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. > > 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. It would probably be best to have a single mkimage for everyone, with everything on. But before then we really need to move from openssl to gnutls or some other library that's compatible as it's been raised before that linking with openssl like we do is a license violation I believe.
On 5/11/21 5:34 PM, Tom Rini wrote: > On Tue, May 11, 2021 at 02:57:03PM -0500, Alex G. 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 <sjg@chromium.org> >> >> Reviewed-by: Alexandru Gagniuc <mr.nuke.me@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. >> >> 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. > > It would probably be best to have a single mkimage for everyone, with > everything on. But before then we really need to move from openssl to > gnutls or some other library that's compatible as it's been raised > before that linking with openssl like we do is a license violation I > believe. How about the former alternative for now? i.e. compile mkimage with or without openssl, and have that be the only host side switch. Alex
On Tue, May 11, 2021 at 07:50:38PM -0500, Alex G. wrote: > On 5/11/21 5:34 PM, Tom Rini wrote: > > On Tue, May 11, 2021 at 02:57:03PM -0500, Alex G. 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 <sjg@chromium.org> > > > > > > Reviewed-by: Alexandru Gagniuc <mr.nuke.me@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. > > > > > > 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. > > > > It would probably be best to have a single mkimage for everyone, with > > everything on. But before then we really need to move from openssl to > > gnutls or some other library that's compatible as it's been raised > > before that linking with openssl like we do is a license violation I > > believe. > > How about the former alternative for now? i.e. compile mkimage with or > without openssl, and have that be the only host side switch. That would be a step in the right direction, yeah.
Hi Alex, On Tue, 11 May 2021 at 13:57, Alex G. <mr.nuke.me@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 <sjg@chromium.org> > > Reviewed-by: Alexandru Gagniuc <mr.nuke.me@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
On 5/12/21 9:51 AM, Simon Glass wrote: > Hi Alex, > > On Tue, 11 May 2021 at 13:57, Alex G. <mr.nuke.me@gmail.com> wrote: >> >> On 5/6/21 9:24 AM, Simon Glass wrote: [snip] >> >>> + >>> +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. On the one hand, we have the cosmetic inconvenience caused by #ifdefs. On the other hand we have the config system. To most users, the config system is likely more visible, while it's mostly developers who will ever see the ifdefs. Therefore, in order to get the developer convenience of less ifdefs, we have to sacrifice user convenience by cloberring the Kconfig options. I think this is back-to-front. Can we reduce the host config count to just SLL/NOSSL? Alex
Hi, On Tue, 11 May 2021 at 19:10, Tom Rini <trini@konsulko.com> wrote: > > On Tue, May 11, 2021 at 07:50:38PM -0500, Alex G. wrote: > > On 5/11/21 5:34 PM, Tom Rini wrote: > > > On Tue, May 11, 2021 at 02:57:03PM -0500, Alex G. 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 <sjg@chromium.org> > > > > > > > > Reviewed-by: Alexandru Gagniuc <mr.nuke.me@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. > > > > > > > > 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. > > > > > > It would probably be best to have a single mkimage for everyone, with > > > everything on. But before then we really need to move from openssl to > > > gnutls or some other library that's compatible as it's been raised > > > before that linking with openssl like we do is a license violation I > > > believe. > > > > How about the former alternative for now? i.e. compile mkimage with or > > without openssl, and have that be the only host side switch. > > That would be a step in the right direction, yeah. We have a NO_SDL build-time control. Perhaps have a NO_SSL one as well? Regards, Simon
Hi Alex, On Wed, 12 May 2021 at 09:48, Alex G. <mr.nuke.me@gmail.com> wrote: > > > > On 5/12/21 9:51 AM, Simon Glass wrote: > > Hi Alex, > > > > On Tue, 11 May 2021 at 13:57, Alex G. <mr.nuke.me@gmail.com> wrote: > >> > >> On 5/6/21 9:24 AM, Simon Glass wrote: > > [snip] > > >> > >>> + > >>> +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. > > On the one hand, we have the cosmetic inconvenience caused by #ifdefs. > On the other hand we have the config system. To most users, the config > system is likely more visible, while it's mostly developers who will > ever see the ifdefs. > > Therefore, in order to get the developer convenience of less ifdefs, we > have to sacrifice user convenience by cloberring the Kconfig options. I > think this is back-to-front. These Kconfig options are not visible to users. They cannot be updated in defconfig, nor in 'make menuconfig', etc. They are purely there for the build system. > > Can we reduce the host config count to just SLL/NOSSL? The point here is that the code has a special case for host builds, and this is a means to remove that special case and make the code easier to maintain and follow. Regards, Simon
On 5/12/21 10:54 AM, Simon Glass wrote: > Hi Alex, > > On Wed, 12 May 2021 at 09:48, Alex G. <mr.nuke.me@gmail.com> wrote: >> >> >> >> On 5/12/21 9:51 AM, Simon Glass wrote: >>> Hi Alex, >>> >>> On Tue, 11 May 2021 at 13:57, Alex G. <mr.nuke.me@gmail.com> wrote: >>>> >>>> On 5/6/21 9:24 AM, Simon Glass wrote: >> >> [snip] >> >>>> >>>>> + >>>>> +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. >> >> On the one hand, we have the cosmetic inconvenience caused by #ifdefs. >> On the other hand we have the config system. To most users, the config >> system is likely more visible, while it's mostly developers who will >> ever see the ifdefs. >> >> Therefore, in order to get the developer convenience of less ifdefs, we >> have to sacrifice user convenience by cloberring the Kconfig options. I >> think this is back-to-front. > > These Kconfig options are not visible to users. They cannot be updated > in defconfig, nor in 'make menuconfig', etc. They are purely there for > the build system. > >> >> Can we reduce the host config count to just SLL/NOSSL? > > The point here is that the code has a special case for host builds, > and this is a means to remove that special case and make the code > easier to maintain and follow. I understand where you're coming from. Without these changes, the code knows what it should and should not do, correct? My argument is that if the code has the logic to do the correct thing, that logic should not be split with the config system. I agree with the goal of reducing clutter in the source code. I disagree with this specific course of fixing it. Instead, I propose a single kconfig for host tools for SSL on/off. The disadvantage of my proposal is that we have to refactor the common code in a way consistent with the goals, instead of just changing some #ifdefs to if(CONFIG_IS_ENABLED()). I admit that, off the top of my head, I don't have a detailed plan on how to achieve this. Alex
On 5/12/21 10:52 AM, Simon Glass wrote: > Hi, > > On Tue, 11 May 2021 at 19:10, Tom Rini <trini@konsulko.com> wrote: >> >> On Tue, May 11, 2021 at 07:50:38PM -0500, Alex G. wrote: >>> On 5/11/21 5:34 PM, Tom Rini wrote: >>>> On Tue, May 11, 2021 at 02:57:03PM -0500, Alex G. 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 <sjg@chromium.org> >>>>> >>>>> Reviewed-by: Alexandru Gagniuc <mr.nuke.me@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. >>>>> >>>>> 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. >>>> >>>> It would probably be best to have a single mkimage for everyone, with >>>> everything on. But before then we really need to move from openssl to >>>> gnutls or some other library that's compatible as it's been raised >>>> before that linking with openssl like we do is a license violation I >>>> believe. >>> >>> How about the former alternative for now? i.e. compile mkimage with or >>> without openssl, and have that be the only host side switch. >> >> That would be a step in the right direction, yeah. > > We have a NO_SDL build-time control. Perhaps have a NO_SSL one as well? It could be a config option instead of an environment variable. I think it can be independent of target options, since we don't sign images in the buildsystem anyway -- we can enable FIT verification, but mkimage without openssl. Alex
On Wed, May 12, 2021 at 11:19:52AM -0500, Alex G. wrote: > > > On 5/12/21 10:52 AM, Simon Glass wrote: > > Hi, > > > > On Tue, 11 May 2021 at 19:10, Tom Rini <trini@konsulko.com> wrote: > > > > > > On Tue, May 11, 2021 at 07:50:38PM -0500, Alex G. wrote: > > > > On 5/11/21 5:34 PM, Tom Rini wrote: > > > > > On Tue, May 11, 2021 at 02:57:03PM -0500, Alex G. 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 <sjg@chromium.org> > > > > > > > > > > > > Reviewed-by: Alexandru Gagniuc <mr.nuke.me@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. > > > > > > > > > > > > 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. > > > > > > > > > > It would probably be best to have a single mkimage for everyone, with > > > > > everything on. But before then we really need to move from openssl to > > > > > gnutls or some other library that's compatible as it's been raised > > > > > before that linking with openssl like we do is a license violation I > > > > > believe. > > > > > > > > How about the former alternative for now? i.e. compile mkimage with or > > > > without openssl, and have that be the only host side switch. > > > > > > That would be a step in the right direction, yeah. > > > > We have a NO_SDL build-time control. Perhaps have a NO_SSL one as well? > > It could be a config option instead of an environment variable. I think it > can be independent of target options, since we don't sign images in the > buildsystem anyway -- we can enable FIT verification, but mkimage without > openssl. As people point out from time to time, "NO_SDL" is very non-obvious and doesn't fit with how the rest of U-Boot is configured. So I would rather not see NO_SSL added. Frankly, given everything else that's needed to build today, I don't think just enabling the support for verified boot in mkimage by default and making it a bit odd to turn off is a problem. But given: https://lists.denx.de/pipermail/u-boot/2017-December/313742.html I would really like to see the switch to gnutls or some other clearly compatibly licensed library first.
Hi Alex, On Wed, 12 May 2021 at 10:18, Alex G. <mr.nuke.me@gmail.com> wrote: > > > > On 5/12/21 10:54 AM, Simon Glass wrote: > > Hi Alex, > > > > On Wed, 12 May 2021 at 09:48, Alex G. <mr.nuke.me@gmail.com> wrote: > >> > >> > >> > >> On 5/12/21 9:51 AM, Simon Glass wrote: > >>> Hi Alex, > >>> > >>> On Tue, 11 May 2021 at 13:57, Alex G. <mr.nuke.me@gmail.com> wrote: > >>>> > >>>> On 5/6/21 9:24 AM, Simon Glass wrote: > >> > >> [snip] > >> > >>>> > >>>>> + > >>>>> +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. > >> > >> On the one hand, we have the cosmetic inconvenience caused by #ifdefs. > >> On the other hand we have the config system. To most users, the config > >> system is likely more visible, while it's mostly developers who will > >> ever see the ifdefs. > >> > >> Therefore, in order to get the developer convenience of less ifdefs, we > >> have to sacrifice user convenience by cloberring the Kconfig options. I > >> think this is back-to-front. > > > > These Kconfig options are not visible to users. They cannot be updated > > in defconfig, nor in 'make menuconfig', etc. They are purely there for > > the build system. > > > >> > >> Can we reduce the host config count to just SLL/NOSSL? > > > > The point here is that the code has a special case for host builds, > > and this is a means to remove that special case and make the code > > easier to maintain and follow. > > I understand where you're coming from. Without these changes, the code > knows what it should and should not do, correct? My argument is that if > the code has the logic to do the correct thing, that logic should not be > split with the config system. > > I agree with the goal of reducing clutter in the source code. I disagree > with this specific course of fixing it. Instead, I propose a single > kconfig for host tools for SSL on/off. > > The disadvantage of my proposal is that we have to refactor the common > code in a way consistent with the goals, instead of just changing some > #ifdefs to if(CONFIG_IS_ENABLED()). I admit that, off the top of my > head, I don't have a detailed plan on how to achieve this. You are mostly describing the status quo, so far as I understand it. The problem is with the code that is built for both boards and tools. For boards, we want this code to be compiled conditionally, depending on what options are enabled. For tools, we want the code to be compiled unconditionally. I can think of only three ways to do this: - status quo (add #ifdefs USE_HOSTCC wherever we need to) - my series (make use of hidden Kconfig options to avoid that) - put every single feature and associated lines of code in separate files and compile them conditionally for boards, but always for tools I believe the last option is actually impossible, or at least impractical. It would cause an explosion of source files to deal with all the various combinations, and would be quite painful to maintain also. Regards, SImon
On 5/12/21 12:30 PM, Simon Glass wrote: > Hi Alex, > > On Wed, 12 May 2021 at 10:18, Alex G. <mr.nuke.me@gmail.com> wrote: >> >> >> >> On 5/12/21 10:54 AM, Simon Glass wrote: >>> Hi Alex, >>> >>> On Wed, 12 May 2021 at 09:48, Alex G. <mr.nuke.me@gmail.com> wrote: >>>> >>>> >>>> >>>> On 5/12/21 9:51 AM, Simon Glass wrote: >>>>> Hi Alex, >>>>> >>>>> On Tue, 11 May 2021 at 13:57, Alex G. <mr.nuke.me@gmail.com> wrote: >>>>>> >>>>>> On 5/6/21 9:24 AM, Simon Glass wrote: >>>> >>>> [snip] >>>> >>>>>> >>>>>>> + >>>>>>> +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. >>>> >>>> On the one hand, we have the cosmetic inconvenience caused by #ifdefs. >>>> On the other hand we have the config system. To most users, the config >>>> system is likely more visible, while it's mostly developers who will >>>> ever see the ifdefs. >>>> >>>> Therefore, in order to get the developer convenience of less ifdefs, we >>>> have to sacrifice user convenience by cloberring the Kconfig options. I >>>> think this is back-to-front. >>> >>> These Kconfig options are not visible to users. They cannot be updated >>> in defconfig, nor in 'make menuconfig', etc. They are purely there for >>> the build system. >>> >>>> >>>> Can we reduce the host config count to just SLL/NOSSL? >>> >>> The point here is that the code has a special case for host builds, >>> and this is a means to remove that special case and make the code >>> easier to maintain and follow. >> >> I understand where you're coming from. Without these changes, the code >> knows what it should and should not do, correct? My argument is that if >> the code has the logic to do the correct thing, that logic should not be >> split with the config system. >> >> I agree with the goal of reducing clutter in the source code. I disagree >> with this specific course of fixing it. Instead, I propose a single >> kconfig for host tools for SSL on/off. >> >> The disadvantage of my proposal is that we have to refactor the common >> code in a way consistent with the goals, instead of just changing some >> #ifdefs to if(CONFIG_IS_ENABLED()). I admit that, off the top of my >> head, I don't have a detailed plan on how to achieve this. > > You are mostly describing the status quo, so far as I understand it. > The problem is with the code that is built for both boards and tools. > For boards, we want this code to be compiled conditionally, depending > on what options are enabled. For tools, we want the code to be > compiled unconditionally. > > I can think of only three ways to do this: > > - status quo (add #ifdefs USE_HOSTCC wherever we need to) > - my series (make use of hidden Kconfig options to avoid that) > - put every single feature and associated lines of code in separate > files and compile them conditionally for boards, but always for tools > > I believe the last option is actually impossible, or at least > impractical. It would cause an explosion of source files to deal with > all the various combinations, and would be quite painful to maintain > also. I don't think the status quo is such a terrible solution, so I am looking at the aspects that can benefit from improvement. Hence why it may appear I am talking about the status quo. Let's assume CONFIG_IS_ENABLED() returns true on host builds, and for those cases where you need to know if IS_HOST_BUILD(), there's a macro for that. You have the oddball case that uses a CONFIG_ value that's only defined on the target, and those you would have to split according to your option #3. I don't think the above is as dire an explosion in source files as it seems. Another point of contention is checksum_algos and crypto_algos arrays image-sig.c. On the target side, these should be in .u-boot-list. Status quo is the definition of rsa_verify is hidden behind #if macros, which just pushes the complexity out into the rsa.h headers. The two ideas here are CONFIG_IS_ENABLED() returns true for host code, and image-sig.c is split bwtween host and target versions, the target version making use of .uboot-list. Using these as the starting points, I think we can get to a much better solution Alex
Hi Alex, On Thu, 13 May 2021 at 10:21, Alex G. <mr.nuke.me@gmail.com> wrote: > > > > On 5/12/21 12:30 PM, Simon Glass wrote: > > Hi Alex, > > > > On Wed, 12 May 2021 at 10:18, Alex G. <mr.nuke.me@gmail.com> wrote: > >> > >> > >> > >> On 5/12/21 10:54 AM, Simon Glass wrote: > >>> Hi Alex, > >>> > >>> On Wed, 12 May 2021 at 09:48, Alex G. <mr.nuke.me@gmail.com> wrote: > >>>> > >>>> > >>>> > >>>> On 5/12/21 9:51 AM, Simon Glass wrote: > >>>>> Hi Alex, > >>>>> > >>>>> On Tue, 11 May 2021 at 13:57, Alex G. <mr.nuke.me@gmail.com> wrote: > >>>>>> > >>>>>> On 5/6/21 9:24 AM, Simon Glass wrote: > >>>> > >>>> [snip] > >>>> > >>>>>> > >>>>>>> + > >>>>>>> +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. > >>>> > >>>> On the one hand, we have the cosmetic inconvenience caused by #ifdefs. > >>>> On the other hand we have the config system. To most users, the config > >>>> system is likely more visible, while it's mostly developers who will > >>>> ever see the ifdefs. > >>>> > >>>> Therefore, in order to get the developer convenience of less ifdefs, we > >>>> have to sacrifice user convenience by cloberring the Kconfig options. I > >>>> think this is back-to-front. > >>> > >>> These Kconfig options are not visible to users. They cannot be updated > >>> in defconfig, nor in 'make menuconfig', etc. They are purely there for > >>> the build system. > >>> > >>>> > >>>> Can we reduce the host config count to just SLL/NOSSL? > >>> > >>> The point here is that the code has a special case for host builds, > >>> and this is a means to remove that special case and make the code > >>> easier to maintain and follow. > >> > >> I understand where you're coming from. Without these changes, the code > >> knows what it should and should not do, correct? My argument is that if > >> the code has the logic to do the correct thing, that logic should not be > >> split with the config system. > >> > >> I agree with the goal of reducing clutter in the source code. I disagree > >> with this specific course of fixing it. Instead, I propose a single > >> kconfig for host tools for SSL on/off. > >> > >> The disadvantage of my proposal is that we have to refactor the common > >> code in a way consistent with the goals, instead of just changing some > >> #ifdefs to if(CONFIG_IS_ENABLED()). I admit that, off the top of my > >> head, I don't have a detailed plan on how to achieve this. > > > > You are mostly describing the status quo, so far as I understand it. > > The problem is with the code that is built for both boards and tools. > > For boards, we want this code to be compiled conditionally, depending > > on what options are enabled. For tools, we want the code to be > > compiled unconditionally. > > > > I can think of only three ways to do this: > > > > - status quo (add #ifdefs USE_HOSTCC wherever we need to) > > - my series (make use of hidden Kconfig options to avoid that) > > - put every single feature and associated lines of code in separate > > files and compile them conditionally for boards, but always for tools > > > > I believe the last option is actually impossible, or at least > > impractical. It would cause an explosion of source files to deal with > > all the various combinations, and would be quite painful to maintain > > also. > > I don't think the status quo is such a terrible solution, so I am > looking at the aspects that can benefit from improvement. Hence why it > may appear I am talking about the status quo. > > Let's assume CONFIG_IS_ENABLED() returns true on host builds, and for > those cases where you need to know if IS_HOST_BUILD(), there's a macro > for that. You have the oddball case that uses a CONFIG_ value that's > only defined on the target, and those you would have to split according > to your option #3. > > I don't think the above is as dire an explosion in source files as it seems. > > Another point of contention is checksum_algos and crypto_algos arrays > image-sig.c. On the target side, these should be in .u-boot-list. Status > quo is the definition of rsa_verify is hidden behind #if macros, which > just pushes the complexity out into the rsa.h headers. > > The two ideas here are CONFIG_IS_ENABLED() returns true for host code, > and image-sig.c is split bwtween host and target versions, the target > version making use of .uboot-list. Using these as the starting points, I > think we can get to a much better solution I did consider simply defining CONFIG_IS_ENABLED() to true for the host, but rejected that because I worried it would break down at some point. Examples I can think of at the moment: - conflicting or alternative options (OF_LIVE, OF_HOST_FILE, OF_SEPARATE) - code that is not actually wanted on the host (WATCHDOG, NEEDS_MANUAL_RELOC, FPGA) - code that we want on the host but not the board (so we end up with a dummy CONFIG for the boards?) - all the SPL / TPL / VPL options which would always end up being true, when in fact we probably want none of them I think you should more clearly explain your objection to the hidden Kconfig options, since your original reason ("clobbering the Kconfig space") doesn't seem to have survived further analysis. Having said that, I can imagine with a lot of refactoring it might be possible to address some of those. I just don't see it as a feasible option from here. The effort would be better spent improving testing, I think. But if you'd like to code something up for it, I'd be interested to see it. Re the linker-list stuff, yes we should push more things to those instead of #ifdefs and weak functions. Hashing and crypto are prime examples. In fact host tools can use linker lists too if we need that. Regards, Simon
On 5/13/21 6:56 PM, Simon Glass wrote: > Hi Alex, > > On Thu, 13 May 2021 at 10:21, Alex G. <mr.nuke.me@gmail.com> wrote: >> >> >> >> On 5/12/21 12:30 PM, Simon Glass wrote: >>> Hi Alex, >>> >>> On Wed, 12 May 2021 at 10:18, Alex G. <mr.nuke.me@gmail.com> wrote: >>>> >>>> >>>> >>>> On 5/12/21 10:54 AM, Simon Glass wrote: >>>>> Hi Alex, >>>>> >>>>> On Wed, 12 May 2021 at 09:48, Alex G. <mr.nuke.me@gmail.com> wrote: >>>>>> >>>>>> >>>>>> >>>>>> On 5/12/21 9:51 AM, Simon Glass wrote: >>>>>>> Hi Alex, >>>>>>> >>>>>>> On Tue, 11 May 2021 at 13:57, Alex G. <mr.nuke.me@gmail.com> wrote: >>>>>>>> >>>>>>>> On 5/6/21 9:24 AM, Simon Glass wrote: >>>>>> >>>>>> [snip] >>>>>> >>>>>>>> >>>>>>>>> + >>>>>>>>> +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. >>>>>> >>>>>> On the one hand, we have the cosmetic inconvenience caused by #ifdefs. >>>>>> On the other hand we have the config system. To most users, the config >>>>>> system is likely more visible, while it's mostly developers who will >>>>>> ever see the ifdefs. >>>>>> >>>>>> Therefore, in order to get the developer convenience of less ifdefs, we >>>>>> have to sacrifice user convenience by cloberring the Kconfig options. I >>>>>> think this is back-to-front. >>>>> >>>>> These Kconfig options are not visible to users. They cannot be updated >>>>> in defconfig, nor in 'make menuconfig', etc. They are purely there for >>>>> the build system. >>>>> >>>>>> >>>>>> Can we reduce the host config count to just SLL/NOSSL? >>>>> >>>>> The point here is that the code has a special case for host builds, >>>>> and this is a means to remove that special case and make the code >>>>> easier to maintain and follow. >>>> >>>> I understand where you're coming from. Without these changes, the code >>>> knows what it should and should not do, correct? My argument is that if >>>> the code has the logic to do the correct thing, that logic should not be >>>> split with the config system. >>>> >>>> I agree with the goal of reducing clutter in the source code. I disagree >>>> with this specific course of fixing it. Instead, I propose a single >>>> kconfig for host tools for SSL on/off. >>>> >>>> The disadvantage of my proposal is that we have to refactor the common >>>> code in a way consistent with the goals, instead of just changing some >>>> #ifdefs to if(CONFIG_IS_ENABLED()). I admit that, off the top of my >>>> head, I don't have a detailed plan on how to achieve this. >>> >>> You are mostly describing the status quo, so far as I understand it. >>> The problem is with the code that is built for both boards and tools. >>> For boards, we want this code to be compiled conditionally, depending >>> on what options are enabled. For tools, we want the code to be >>> compiled unconditionally. >>> >>> I can think of only three ways to do this: >>> >>> - status quo (add #ifdefs USE_HOSTCC wherever we need to) >>> - my series (make use of hidden Kconfig options to avoid that) >>> - put every single feature and associated lines of code in separate >>> files and compile them conditionally for boards, but always for tools >>> >>> I believe the last option is actually impossible, or at least >>> impractical. It would cause an explosion of source files to deal with >>> all the various combinations, and would be quite painful to maintain >>> also. >> >> I don't think the status quo is such a terrible solution, so I am >> looking at the aspects that can benefit from improvement. Hence why it >> may appear I am talking about the status quo. >> >> Let's assume CONFIG_IS_ENABLED() returns true on host builds, and for >> those cases where you need to know if IS_HOST_BUILD(), there's a macro >> for that. You have the oddball case that uses a CONFIG_ value that's >> only defined on the target, and those you would have to split according >> to your option #3. >> >> I don't think the above is as dire an explosion in source files as it seems. >> >> Another point of contention is checksum_algos and crypto_algos arrays >> image-sig.c. On the target side, these should be in .u-boot-list. Status >> quo is the definition of rsa_verify is hidden behind #if macros, which >> just pushes the complexity out into the rsa.h headers. >> >> The two ideas here are CONFIG_IS_ENABLED() returns true for host code, >> and image-sig.c is split bwtween host and target versions, the target >> version making use of .uboot-list. Using these as the starting points, I >> think we can get to a much better solution > > I did consider simply defining CONFIG_IS_ENABLED() to true for the > host, but rejected that because I worried it would break down at some > point. Examples I can think of at the moment: > > - conflicting or alternative options (OF_LIVE, OF_HOST_FILE, OF_SEPARATE) > - code that is not actually wanted on the host (WATCHDOG, > NEEDS_MANUAL_RELOC, FPGA) > - code that we want on the host but not the board (so we end up with a > dummy CONFIG for the boards?) > - all the SPL / TPL / VPL options which would always end up being > true, when in fact we probably want none of them > > I think you should more clearly explain your objection to the hidden > Kconfig options, since your original reason ("clobbering the Kconfig > space") doesn't seem to have survived further analysis. I thought it to have been already explained and settled. It objectively increase the complexity of the logic. Instead of the logic being defined in one place (code), it is now defined in two places (code and Kconfig). And subjectively, when adding ECDSA support, I found the ifs/ifdefs that were derived from kconfig code to be extremely confusing. It would have helped trememdously if the host code would flow with less 'kconfig' decision points. I don't find this series to improve on that. > Having said that, I can imagine with a lot of refactoring it might be > possible to address some of those. I just don't see it as a feasible > option from here. The effort would be better spent improving testing, > I think. But if you'd like to code something up for it, I'd be > interested to see it. We are in agreement that refactoring will improve the situation. I don't think it's so dire that it's unfeasible. However, if you'd like, we can start in smaller chunks. > Re the linker-list stuff, yes we should push more things to those > instead of #ifdefs and weak functions. Hashing and crypto are prime > examples. In fact host tools can use linker lists too if we need that. Pretty low hanging fruit here. Let me try to code something up. > Regards, > Simon >
Hi Alex, On Fri, 14 May 2021 at 09:12, Alex G. <mr.nuke.me@gmail.com> wrote: > > > > On 5/13/21 6:56 PM, Simon Glass wrote: > > Hi Alex, > > > > On Thu, 13 May 2021 at 10:21, Alex G. <mr.nuke.me@gmail.com> wrote: > >> > >> > >> > >> On 5/12/21 12:30 PM, Simon Glass wrote: > >>> Hi Alex, > >>> > >>> On Wed, 12 May 2021 at 10:18, Alex G. <mr.nuke.me@gmail.com> wrote: > >>>> > >>>> > >>>> > >>>> On 5/12/21 10:54 AM, Simon Glass wrote: > >>>>> Hi Alex, > >>>>> > >>>>> On Wed, 12 May 2021 at 09:48, Alex G. <mr.nuke.me@gmail.com> wrote: > >>>>>> > >>>>>> > >>>>>> > >>>>>> On 5/12/21 9:51 AM, Simon Glass wrote: > >>>>>>> Hi Alex, > >>>>>>> > >>>>>>> On Tue, 11 May 2021 at 13:57, Alex G. <mr.nuke.me@gmail.com> wrote: > >>>>>>>> > >>>>>>>> On 5/6/21 9:24 AM, Simon Glass wrote: > >>>>>> > >>>>>> [snip] > >>>>>> > >>>>>>>> > >>>>>>>>> + > >>>>>>>>> +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. > >>>>>> > >>>>>> On the one hand, we have the cosmetic inconvenience caused by #ifdefs. > >>>>>> On the other hand we have the config system. To most users, the config > >>>>>> system is likely more visible, while it's mostly developers who will > >>>>>> ever see the ifdefs. > >>>>>> > >>>>>> Therefore, in order to get the developer convenience of less ifdefs, we > >>>>>> have to sacrifice user convenience by cloberring the Kconfig options. I > >>>>>> think this is back-to-front. > >>>>> > >>>>> These Kconfig options are not visible to users. They cannot be updated > >>>>> in defconfig, nor in 'make menuconfig', etc. They are purely there for > >>>>> the build system. > >>>>> > >>>>>> > >>>>>> Can we reduce the host config count to just SLL/NOSSL? > >>>>> > >>>>> The point here is that the code has a special case for host builds, > >>>>> and this is a means to remove that special case and make the code > >>>>> easier to maintain and follow. > >>>> > >>>> I understand where you're coming from. Without these changes, the code > >>>> knows what it should and should not do, correct? My argument is that if > >>>> the code has the logic to do the correct thing, that logic should not be > >>>> split with the config system. > >>>> > >>>> I agree with the goal of reducing clutter in the source code. I disagree > >>>> with this specific course of fixing it. Instead, I propose a single > >>>> kconfig for host tools for SSL on/off. > >>>> > >>>> The disadvantage of my proposal is that we have to refactor the common > >>>> code in a way consistent with the goals, instead of just changing some > >>>> #ifdefs to if(CONFIG_IS_ENABLED()). I admit that, off the top of my > >>>> head, I don't have a detailed plan on how to achieve this. > >>> > >>> You are mostly describing the status quo, so far as I understand it. > >>> The problem is with the code that is built for both boards and tools. > >>> For boards, we want this code to be compiled conditionally, depending > >>> on what options are enabled. For tools, we want the code to be > >>> compiled unconditionally. > >>> > >>> I can think of only three ways to do this: > >>> > >>> - status quo (add #ifdefs USE_HOSTCC wherever we need to) > >>> - my series (make use of hidden Kconfig options to avoid that) > >>> - put every single feature and associated lines of code in separate > >>> files and compile them conditionally for boards, but always for tools > >>> > >>> I believe the last option is actually impossible, or at least > >>> impractical. It would cause an explosion of source files to deal with > >>> all the various combinations, and would be quite painful to maintain > >>> also. > >> > >> I don't think the status quo is such a terrible solution, so I am > >> looking at the aspects that can benefit from improvement. Hence why it > >> may appear I am talking about the status quo. > >> > >> Let's assume CONFIG_IS_ENABLED() returns true on host builds, and for > >> those cases where you need to know if IS_HOST_BUILD(), there's a macro > >> for that. You have the oddball case that uses a CONFIG_ value that's > >> only defined on the target, and those you would have to split according > >> to your option #3. > >> > >> I don't think the above is as dire an explosion in source files as it seems. > >> > >> Another point of contention is checksum_algos and crypto_algos arrays > >> image-sig.c. On the target side, these should be in .u-boot-list. Status > >> quo is the definition of rsa_verify is hidden behind #if macros, which > >> just pushes the complexity out into the rsa.h headers. > >> > >> The two ideas here are CONFIG_IS_ENABLED() returns true for host code, > >> and image-sig.c is split bwtween host and target versions, the target > >> version making use of .uboot-list. Using these as the starting points, I > >> think we can get to a much better solution > > > > I did consider simply defining CONFIG_IS_ENABLED() to true for the > > host, but rejected that because I worried it would break down at some > > point. Examples I can think of at the moment: > > > > - conflicting or alternative options (OF_LIVE, OF_HOST_FILE, OF_SEPARATE) > > - code that is not actually wanted on the host (WATCHDOG, > > NEEDS_MANUAL_RELOC, FPGA) > > - code that we want on the host but not the board (so we end up with a > > dummy CONFIG for the boards?) > > - all the SPL / TPL / VPL options which would always end up being > > true, when in fact we probably want none of them > > > > I think you should more clearly explain your objection to the hidden > > Kconfig options, since your original reason ("clobbering the Kconfig > > space") doesn't seem to have survived further analysis. > > I thought it to have been already explained and settled. It objectively > increase the complexity of the logic. Instead of the logic being defined > in one place (code), it is now defined in two places (code and Kconfig). > > And subjectively, when adding ECDSA support, I found the ifs/ifdefs that > were derived from kconfig code to be extremely confusing. It would have > helped trememdously if the host code would flow with less 'kconfig' > decision points. I don't find this series to improve on that. I think separating out host code is fine. But we do need to be careful. A few years back someone added fit_check_sign and something else that runs the board code. If we had had separate code at that point, the additional of that tool would have been a huge lift. So I think we should: - separate out cost that is clearly host-only (perhaps into the tools/ dir) - don't separate out board code, since we will likely want to run it with a tool one day > > > Having said that, I can imagine with a lot of refactoring it might be > > possible to address some of those. I just don't see it as a feasible > > option from here. The effort would be better spent improving testing, > > I think. But if you'd like to code something up for it, I'd be > > interested to see it. > > We are in agreement that refactoring will improve the situation. I don't > think it's so dire that it's unfeasible. However, if you'd like, we can > start in smaller chunks. OK. > > > Re the linker-list stuff, yes we should push more things to those > > instead of #ifdefs and weak functions. Hashing and crypto are prime > > examples. In fact host tools can use linker lists too if we need that. > > Pretty low hanging fruit here. Let me try to code something up. OK. Perhaps with some separation, the Kconfig stuff will become easier. Regards, Simon
On 5/12/21 12:14 PM, Tom Rini wrote: > On Wed, May 12, 2021 at 11:19:52AM -0500, Alex G. wrote: >> >> >> On 5/12/21 10:52 AM, Simon Glass wrote: [snip] >>> We have a NO_SDL build-time control. Perhaps have a NO_SSL one as well? >> >> It could be a config option instead of an environment variable. I think it >> can be independent of target options, since we don't sign images in the >> buildsystem anyway -- we can enable FIT verification, but mkimage without >> openssl. > > As people point out from time to time, "NO_SDL" is very non-obvious and > doesn't fit with how the rest of U-Boot is configured. So I would > rather not see NO_SSL added. FYI, I have a proof-of-concept for the NO_SSL idea using Kconfig [1] instead of environment variahles. It's not yet ready for publication. [1] https://github.com/mrnuke/u-boot/commit/c054c546a8de54e41d3802fe60ad9389095e673b > Frankly, given everything else that's > needed to build today, I don't think just enabling the support for > verified boot in mkimage by default and making it a bit odd to turn off > is a problem. But given: > https://lists.denx.de/pipermail/u-boot/2017-December/313742.html > I would really like to see the switch to gnutls or some other clearly > compatibly licensed library first. Might be interesting to switch to gnutls, even if only because it doesn't burn your eyes looking at function names and variable types. I wouldn't mind looking into this, but I just don't have the bandwidth nowadays. Alex
On Mon, May 17, 2021 at 05:29:44PM -0500, Alex G. wrote: > On 5/12/21 12:14 PM, Tom Rini wrote: > > On Wed, May 12, 2021 at 11:19:52AM -0500, Alex G. wrote: > > > > > > > > > On 5/12/21 10:52 AM, Simon Glass wrote: > > [snip] > > > > > We have a NO_SDL build-time control. Perhaps have a NO_SSL one as well? > > > > > > It could be a config option instead of an environment variable. I think it > > > can be independent of target options, since we don't sign images in the > > > buildsystem anyway -- we can enable FIT verification, but mkimage without > > > openssl. > > > > As people point out from time to time, "NO_SDL" is very non-obvious and > > doesn't fit with how the rest of U-Boot is configured. So I would > > rather not see NO_SSL added. > > FYI, I have a proof-of-concept for the NO_SSL idea using Kconfig [1] instead > of environment variahles. It's not yet ready for publication. > > [1] https://github.com/mrnuke/u-boot/commit/c054c546a8de54e41d3802fe60ad9389095e673b FYI, I have posted a patch[1] for a similar *signing* tool using OpenSSL. Basically, I'd like to follow the way agreed here about how OpenSSL be handled in host tools. So please keep in mind that there can be another use case of this kind of host Kconfig option. [1] https://lists.denx.de/pipermail/u-boot/2021-May/449572.html -Takahiro Akashi > > > Frankly, given everything else that's > > needed to build today, I don't think just enabling the support for > > verified boot in mkimage by default and making it a bit odd to turn off > > is a problem. But given: > > https://lists.denx.de/pipermail/u-boot/2017-December/313742.html > > I would really like to see the switch to gnutls or some other clearly > > compatibly licensed library first. > > Might be interesting to switch to gnutls, even if only because it doesn't > burn your eyes looking at function names and variable types. I wouldn't mind > looking into this, but I just don't have the bandwidth nowadays. > > Alex
On 5/17/21 8:23 PM, AKASHI Takahiro wrote: > On Mon, May 17, 2021 at 05:29:44PM -0500, Alex G. wrote: >> On 5/12/21 12:14 PM, Tom Rini wrote: >>> On Wed, May 12, 2021 at 11:19:52AM -0500, Alex G. wrote: >>>> >>>> >>>> On 5/12/21 10:52 AM, Simon Glass wrote: >> >> [snip] >> >>>>> We have a NO_SDL build-time control. Perhaps have a NO_SSL one as well? >>>> >>>> It could be a config option instead of an environment variable. I think it >>>> can be independent of target options, since we don't sign images in the >>>> buildsystem anyway -- we can enable FIT verification, but mkimage without >>>> openssl. >>> >>> As people point out from time to time, "NO_SDL" is very non-obvious and >>> doesn't fit with how the rest of U-Boot is configured. So I would >>> rather not see NO_SSL added. >> >> FYI, I have a proof-of-concept for the NO_SSL idea using Kconfig [1] instead >> of environment variahles. It's not yet ready for publication. >> >> [1] https://github.com/mrnuke/u-boot/commit/c054c546a8de54e41d3802fe60ad9389095e673b > > > FYI, > I have posted a patch[1] for a similar *signing* tool using OpenSSL. > Basically, I'd like to follow the way agreed here about how OpenSSL > be handled in host tools. > So please keep in mind that there can be another use case of this kind > of host Kconfig option. > > [1] https://lists.denx.de/pipermail/u-boot/2021-May/449572.html I can't ask you to change your patch based on my ideas, as I my changes have not yet been submitted for review. However, should you want to anticipate, make sure that there's one and only one variable that determines if OpenSSL is linked. I also suspect Tom would be quite thrilled if your patch started using gnutls instead of openssl. I'm not sure how sane things would look having both gnutls and openssl dependencies; however, I suspect it might be acceptable as long as it's temporary. These decisions haven't been made yet. I don't want to send you on a wild goose refactoring chase, only to have the rug pulled from under you later. I think it's okay to continue with your patch as submitted. I'll update my patch accordingly when yours gets merged first -- looks easy enough. Alex
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. + +config HOST_FIT_FULL_CHECK + def_bool y + help + Do a full check of the FIT before using it in the host build + +config HOST_FIT_PRINT + def_bool y + help + Print the content of the FIT verbosely in the host build + +config HOST_FIT_SIGNATURE + def_bool y + help + Enable signature verification of FIT uImages in the host build + +config HOST_FIT_SIGNATURE_MAX_SIZE + hex + depends on HOST_FIT_SIGNATURE + default 0x10000000 + endmenu diff --git a/tools/Makefile b/tools/Makefile index 2b4bc547abd..d143198f7c9 100644 --- a/tools/Makefile +++ b/tools/Makefile @@ -53,12 +53,12 @@ hostprogs-y += mkenvimage mkenvimage-objs := mkenvimage.o os_support.o lib/crc32.o hostprogs-y += dumpimage mkimage -hostprogs-$(CONFIG_FIT_SIGNATURE) += fit_info fit_check_sign +hostprogs-$(CONFIG_HOST_FIT_SIGNATURE) += fit_info fit_check_sign hostprogs-$(CONFIG_CMD_BOOTEFI_SELFTEST) += file2include -FIT_OBJS-$(CONFIG_FIT) := fit_common.o fit_image.o image-host.o common/image-fit.o -FIT_SIG_OBJS-$(CONFIG_FIT_SIGNATURE) := common/image-sig.o common/image-fit-sig.o +FIT_OBJS-$(CONFIG_HOST_FIT) := fit_common.o fit_image.o image-host.o common/image-fit.o +FIT_SIG_OBJS-$(CONFIG_HOST_FIT_SIGNATURE) := common/image-sig.o common/image-fit-sig.o FIT_CIPHER_OBJS-$(CONFIG_FIT_CIPHER) := common/image-cipher.o # The following files are synced with upstream DTC. @@ -66,17 +66,17 @@ FIT_CIPHER_OBJS-$(CONFIG_FIT_CIPHER) := common/image-cipher.o LIBFDT_OBJS := $(addprefix libfdt/, fdt.o fdt_ro.o fdt_wip.o fdt_sw.o fdt_rw.o \ fdt_strerror.o fdt_empty_tree.o fdt_addresses.o fdt_overlay.o) -RSA_OBJS-$(CONFIG_FIT_SIGNATURE) := $(addprefix lib/rsa/, \ +RSA_OBJS-$(CONFIG_HOST_FIT_SIGNATURE) := $(addprefix lib/rsa/, \ rsa-sign.o rsa-verify.o \ rsa-mod-exp.o) -ECDSA_OBJS-$(CONFIG_FIT_SIGNATURE) := $(addprefix lib/ecdsa/, ecdsa-libcrypto.o) +ECDSA_OBJS-$(CONFIG_HOST_FIT_SIGNATURE) := $(addprefix lib/ecdsa/, ecdsa-libcrypto.o) AES_OBJS-$(CONFIG_FIT_CIPHER) := $(addprefix lib/aes/, \ aes-encrypt.o aes-decrypt.o) # Cryptographic helpers that depend on openssl/libcrypto -LIBCRYPTO_OBJS-$(CONFIG_FIT_SIGNATURE) := $(addprefix lib/, \ +LIBCRYPTO_OBJS-$(CONFIG_HOST_FIT_SIGNATURE) := $(addprefix lib/, \ fdt-libcrypto.o) ROCKCHIP_OBS = lib/rc4.o rkcommon.o rkimage.o rksd.o rkspi.o @@ -137,13 +137,13 @@ fit_info-objs := $(dumpimage-mkimage-objs) fit_info.o fit_check_sign-objs := $(dumpimage-mkimage-objs) fit_check_sign.o file2include-objs := file2include.o -ifneq ($(CONFIG_MX23)$(CONFIG_MX28)$(CONFIG_FIT_SIGNATURE),) +ifneq ($(CONFIG_MX23)$(CONFIG_MX28)$(CONFIG_HOST_FIT_SIGNATURE),) # Add CONFIG_MXS into host CFLAGS, so we can check whether or not register # the mxsimage support within tools/mxsimage.c . HOSTCFLAGS_mxsimage.o += -DCONFIG_MXS endif -ifdef CONFIG_FIT_SIGNATURE +ifdef CONFIG_HOST_FIT_SIGNATURE # This affects include/image.h, but including the board config file # is tricky, so manually define this options here. HOST_EXTRACFLAGS += -DCONFIG_FIT_SIGNATURE @@ -165,7 +165,7 @@ HOSTCFLAGS_kwbimage.o += -DCONFIG_KWB_SECURE endif # MXSImage needs LibSSL -ifneq ($(CONFIG_MX23)$(CONFIG_MX28)$(CONFIG_ARMADA_38X)$(CONFIG_FIT_SIGNATURE)$(CONFIG_FIT_CIPHER),) +ifneq ($(CONFIG_MX23)$(CONFIG_MX28)$(CONFIG_ARMADA_38X)$(CONFIG_HOST_FIT_SIGNATURE)$(CONFIG_FIT_CIPHER),) HOSTCFLAGS_kwbimage.o += \ $(shell pkg-config --cflags libssl libcrypto 2> /dev/null || echo "") HOSTLDLIBS_mkimage += \
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 <sjg@chromium.org> --- (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(-)