[U-Boot] [PATCH 4/4] Make FIT support really optional
Due to some mistakes in the source code, it was not possible to really turn FIT support off. This commit fixes the problem by means of the following changes:
- Enclose "bootm_host_load_image" and "bootm_host_load_images" between checks for CONFIG_FIT_SIGNATURE, in common/bootm.c.
- Enclose the declaration of "bootm_host_load_images" between checks for CONFIG_FIT_SIGNATURE, in common/bootm.h.
- Condition the compilation and linking of fit_common.o fit_image.o image-host.o common/image-fit.o to CONFIG_FIT=y, in tools/Makefile.
Signed-off-by: Carlos Santos casantos@datacom.ind.br --- common/bootm.c | 2 ++ include/bootm.h | 2 ++ tools/Makefile | 6 ++---- 3 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/common/bootm.c b/common/bootm.c index c965326..ab477ba 100644 --- a/common/bootm.c +++ b/common/bootm.c @@ -891,6 +891,7 @@ void memmove_wd(void *to, void *from, size_t len, ulong chunksz) memmove(to, from, len); }
+#if defined(CONFIG_FIT_SIGNATURE) static int bootm_host_load_image(const void *fit, int req_image_type) { const char *fit_uname_config = NULL; @@ -955,5 +956,6 @@ int bootm_host_load_images(const void *fit, int cfg_noffset) /* Return the first error we found */ return err; } +#endif
#endif /* ndef USE_HOSTCC */ diff --git a/include/bootm.h b/include/bootm.h index 4981377..94d62a1 100644 --- a/include/bootm.h +++ b/include/bootm.h @@ -41,7 +41,9 @@ void lynxkdi_boot(image_header_t *hdr);
boot_os_fn *bootm_os_get_boot_func(int os);
+#if defined(CONFIG_FIT_SIGNATURE) int bootm_host_load_images(const void *fit, int cfg_noffset); +#endif
int boot_selected_os(int argc, char * const argv[], int state, bootm_headers_t *images, boot_os_fn *boot_fn); diff --git a/tools/Makefile b/tools/Makefile index da50e1b..0a3d279 100644 --- a/tools/Makefile +++ b/tools/Makefile @@ -54,6 +54,7 @@ mkenvimage-objs := mkenvimage.o os_support.o lib/crc32.o hostprogs-y += dumpimage mkimage hostprogs-$(CONFIG_FIT_SIGNATURE) += fit_info fit_check_sign
+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 # Flattened device tree objects LIBFDT_OBJS := $(addprefix lib/libfdt/, \ @@ -68,18 +69,15 @@ ROCKCHIP_OBS = lib/rc4.o rkcommon.o rkimage.o rksd.o rkspi.o # common objs for dumpimage and mkimage dumpimage-mkimage-objs := aisimage.o \ atmelimage.o \ + $(FIT_OBJS-y) \ $(FIT_SIG_OBJS-y) \ common/bootm.o \ lib/crc32.o \ default_image.o \ lib/fdtdec_common.o \ lib/fdtdec.o \ - fit_common.o \ - fit_image.o \ gpimage.o \ gpimage-common.o \ - common/image-fit.o \ - image-host.o \ common/image.o \ imagetool.o \ imximage.o \
Due to some mistakes in the source code, it was not possible to really turn FIT support off. This commit fixes the problem by means of the following changes:
- Enclose "bootm_host_load_image" and "bootm_host_load_images" between checks for CONFIG_FIT_SIGNATURE, in common/bootm.c.
- Enclose the declaration of "bootm_host_load_images" between checks for CONFIG_FIT_SIGNATURE, in common/bootm.h.
- Condition the compilation and linking of fit_common.o fit_image.o image-host.o common/image-fit.o to CONFIG_FIT=y, in tools/Makefile.
Signed-off-by: Carlos Santos casantos@datacom.ind.br --- Changes v1 -> v2 Rebased to the top of master branch.
common/bootm.c | 2 ++ include/bootm.h | 2 ++ tools/Makefile | 6 ++---- 3 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/common/bootm.c b/common/bootm.c index 4941414..bbf3f97 100644 --- a/common/bootm.c +++ b/common/bootm.c @@ -901,6 +901,7 @@ void memmove_wd(void *to, void *from, size_t len, ulong chunksz) memmove(to, from, len); }
+#if defined(CONFIG_FIT_SIGNATURE) static int bootm_host_load_image(const void *fit, int req_image_type) { const char *fit_uname_config = NULL; @@ -965,5 +966,6 @@ int bootm_host_load_images(const void *fit, int cfg_noffset) /* Return the first error we found */ return err; } +#endif
#endif /* ndef USE_HOSTCC */ diff --git a/include/bootm.h b/include/bootm.h index 4981377..94d62a1 100644 --- a/include/bootm.h +++ b/include/bootm.h @@ -41,7 +41,9 @@ void lynxkdi_boot(image_header_t *hdr);
boot_os_fn *bootm_os_get_boot_func(int os);
+#if defined(CONFIG_FIT_SIGNATURE) int bootm_host_load_images(const void *fit, int cfg_noffset); +#endif
int boot_selected_os(int argc, char * const argv[], int state, bootm_headers_t *images, boot_os_fn *boot_fn); diff --git a/tools/Makefile b/tools/Makefile index 63355aa..05122d5 100644 --- a/tools/Makefile +++ b/tools/Makefile @@ -54,6 +54,7 @@ mkenvimage-objs := mkenvimage.o os_support.o lib/crc32.o hostprogs-y += dumpimage mkimage hostprogs-$(CONFIG_FIT_SIGNATURE) += fit_info fit_check_sign
+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 # Flattened device tree objects LIBFDT_OBJS := $(addprefix lib/libfdt/, \ @@ -68,18 +69,15 @@ ROCKCHIP_OBS = lib/rc4.o rkcommon.o rkimage.o rksd.o rkspi.o # common objs for dumpimage and mkimage dumpimage-mkimage-objs := aisimage.o \ atmelimage.o \ + $(FIT_OBJS-y) \ $(FIT_SIG_OBJS-y) \ common/bootm.o \ lib/crc32.o \ default_image.o \ lib/fdtdec_common.o \ lib/fdtdec.o \ - fit_common.o \ - fit_image.o \ gpimage.o \ gpimage-common.o \ - common/image-fit.o \ - image-host.o \ common/image.o \ imagetool.o \ imximage.o \
Hello Carlos,
On Fri, Jun 3, 2016 at 4:16 PM, Carlos Santos casantos@datacom.ind.br wrote:
Due to some mistakes in the source code, it was not possible to really turn FIT support off. This commit fixes the problem by means of the following changes:
Enclose "bootm_host_load_image" and "bootm_host_load_images" between checks for CONFIG_FIT_SIGNATURE, in common/bootm.c.
Enclose the declaration of "bootm_host_load_images" between checks for CONFIG_FIT_SIGNATURE, in common/bootm.h.
Condition the compilation and linking of fit_common.o fit_image.o image-host.o common/image-fit.o to CONFIG_FIT=y, in tools/Makefile.
Signed-off-by: Carlos Santos casantos@datacom.ind.br
Reviewed-by: Otavio Salvador otavio@ossystems.com.br
Thanks for resending it; I am adding Tom on Cc as this is likely going straight for his tree.
On Fri, Jun 03, 2016 at 04:16:26PM -0300, Carlos Santos wrote:
Due to some mistakes in the source code, it was not possible to really turn FIT support off. This commit fixes the problem by means of the following changes:
Enclose "bootm_host_load_image" and "bootm_host_load_images" between checks for CONFIG_FIT_SIGNATURE, in common/bootm.c.
Enclose the declaration of "bootm_host_load_images" between checks for CONFIG_FIT_SIGNATURE, in common/bootm.h.
Condition the compilation and linking of fit_common.o fit_image.o image-host.o common/image-fit.o to CONFIG_FIT=y, in tools/Makefile.
Signed-off-by: Carlos Santos casantos@datacom.ind.br
Changes v1 -> v2 Rebased to the top of master branch.
common/bootm.c | 2 ++ include/bootm.h | 2 ++ tools/Makefile | 6 ++---- 3 files changed, 6 insertions(+), 4 deletions(-)
So, why? I don't like the idea of making FIT support in mkimage conditional. This makes the life of distribution people harder, not easier. The functions in common/bootm.c should be being discarded in U-Boot itself when we don't have CONFIG_FIT_SIGNATURE. Thanks!
From: "Tom Rini" trini@konsulko.com To: "Carlos Santos" casantos@datacom.ind.br Cc: u-boot@lists.denx.de Sent: Saturday, June 4, 2016 10:06:58 AM Subject: Re: [U-Boot] [PATCH v2] Make FIT support really optional
On Fri, Jun 03, 2016 at 04:16:26PM -0300, Carlos Santos wrote:
Due to some mistakes in the source code, it was not possible to really turn FIT support off. This commit fixes the problem by means of the following changes:
Enclose "bootm_host_load_image" and "bootm_host_load_images" between checks for CONFIG_FIT_SIGNATURE, in common/bootm.c.
Enclose the declaration of "bootm_host_load_images" between checks for CONFIG_FIT_SIGNATURE, in common/bootm.h.
Condition the compilation and linking of fit_common.o fit_image.o image-host.o common/image-fit.o to CONFIG_FIT=y, in tools/Makefile.
Signed-off-by: Carlos Santos casantos@datacom.ind.br
Changes v1 -> v2 Rebased to the top of master branch.
common/bootm.c | 2 ++ include/bootm.h | 2 ++ tools/Makefile | 6 ++---- 3 files changed, 6 insertions(+), 4 deletions(-)
So, why? I don't like the idea of making FIT support in mkimage conditional.
If FIT is not to be conditional then what's the purpose of the CONFIG_FIT_SIGNATURE configuration option? Looks like it exists exactly to make FIT support conditional, which seems to be a reasonable approach, since it helps to reduce the size of the boot loader.
This makes the life of distribution people harder, not easier. The functions in common/bootm.c should be being discarded in U-Boot itself when we don't have CONFIG_FIT_SIGNATURE. Thanks!
The patch exists because of "distribution people". I sent a patch to Buildroot[1] which was refused because it added dependencies on DTC and evolved to several follow-ups [2,3,4].
1. http://patchwork.ozlabs.org/patch/618486/ 2. http://patchwork.ozlabs.org/patch/619278/ 3. http://patchwork.ozlabs.org/patch/619696/ 4. http://patchwork.ozlabs.org/patch/629988/
Carlos Santos (Casantos) DATACOM, P&D
From: "Carlos Santos" casantos@datacom.ind.br To: "Tom Rini" trini@konsulko.com Cc: u-boot@lists.denx.de Sent: Saturday, June 4, 2016 2:39:22 PM Subject: Re: [U-Boot] [PATCH v2] Make FIT support really optional
From: "Tom Rini" trini@konsulko.com To: "Carlos Santos" casantos@datacom.ind.br Cc: u-boot@lists.denx.de Sent: Saturday, June 4, 2016 10:06:58 AM Subject: Re: [U-Boot] [PATCH v2] Make FIT support really optional
On Fri, Jun 03, 2016 at 04:16:26PM -0300, Carlos Santos wrote:
Due to some mistakes in the source code, it was not possible to really turn FIT support off. This commit fixes the problem by means of the following changes:
Enclose "bootm_host_load_image" and "bootm_host_load_images" between checks for CONFIG_FIT_SIGNATURE, in common/bootm.c.
Enclose the declaration of "bootm_host_load_images" between checks for CONFIG_FIT_SIGNATURE, in common/bootm.h.
Condition the compilation and linking of fit_common.o fit_image.o image-host.o common/image-fit.o to CONFIG_FIT=y, in tools/Makefile.
Signed-off-by: Carlos Santos casantos@datacom.ind.br
Changes v1 -> v2 Rebased to the top of master branch.
common/bootm.c | 2 ++ include/bootm.h | 2 ++ tools/Makefile | 6 ++---- 3 files changed, 6 insertions(+), 4 deletions(-)
So, why? I don't like the idea of making FIT support in mkimage conditional.
If FIT is not to be conditional then what's the purpose of the CONFIG_FIT_SIGNATURE configuration option? Looks like it exists exactly to make FIT support conditional, which seems to be a reasonable approach, since it helps to reduce the size of the boot loader.
Sorry, I meant "what is the purpose of the CONFIG_FIT option".
This makes the life of distribution people harder, not easier. The functions in common/bootm.c should be being discarded in U-Boot itself when we don't have CONFIG_FIT_SIGNATURE. Thanks!
The patch exists because of "distribution people". I sent a patch to Buildroot[1] which was refused because it added dependencies on DTC and evolved to several follow-ups [2,3,4].
- http://patchwork.ozlabs.org/patch/618486/
- http://patchwork.ozlabs.org/patch/619278/
- http://patchwork.ozlabs.org/patch/619696/
- http://patchwork.ozlabs.org/patch/629988/
Carlos Santos (Casantos) DATACOM, P&D
Carlos, Tom,
On Sat, 4 Jun 2016 14:39:22 -0300 (BRT), Carlos Santos wrote:
So, why? I don't like the idea of making FIT support in mkimage conditional.
If FIT is not to be conditional then what's the purpose of the CONFIG_FIT_SIGNATURE configuration option? Looks like it exists exactly to make FIT support conditional, which seems to be a reasonable approach, since it helps to reduce the size of the boot loader.
CONFIG_FIT_SIGNATURE is I guess optional because it requires OpenSSL at *build* time and the U-Boot developers don't want to force everyone to have OpenSSL available to build U-Boot.
However, FIT support does not require any special build dependency, so probably there's little interest from the U-Boot folks to make it optional.
This makes the life of distribution people harder, not easier. The functions in common/bootm.c should be being discarded in U-Boot itself when we don't have CONFIG_FIT_SIGNATURE. Thanks!
The patch exists because of "distribution people". I sent a patch to Buildroot[1] which was refused because it added dependencies on DTC and evolved to several follow-ups [2,3,4].
Right, *but* it is not because we make FIT support optional in Buildroot that we have to make it optional in U-Boot.
We can perfectly have an option in Buildroot to enable/disable FIT support which does *not* enable/disable FIT support in the U-Boot, but only ensures that the relevant runtime dependencies (i.e DTC) are enabled.
Of course, if the U-Boot developers want to make FIT support in the bootloader itself an optional feature, why not, but it's clearly not a requirement from our side.
Best regards,
Thomas
From: "Thomas Petazzoni" thomas.petazzoni@free-electrons.com To: "Carlos Santos" casantos@datacom.ind.br Cc: "Tom Rini" trini@konsulko.com, u-boot@lists.denx.de Sent: Tuesday, June 7, 2016 5:37:46 PM Subject: Re: [U-Boot] [PATCH v2] Make FIT support really optional
Carlos, Tom,
On Sat, 4 Jun 2016 14:39:22 -0300 (BRT), Carlos Santos wrote:
So, why? I don't like the idea of making FIT support in mkimage conditional.
If FIT is not to be conditional then what's the purpose of the CONFIG_FIT_SIGNATURE configuration option? Looks like it exists exactly to make FIT support conditional, which seems to be a reasonable approach, since it helps to reduce the size of the boot loader.
CONFIG_FIT_SIGNATURE is I guess optional because it requires OpenSSL at *build* time and the U-Boot developers don't want to force everyone to have OpenSSL available to build U-Boot.
However, FIT support does not require any special build dependency, so probably there's little interest from the U-Boot folks to make it optional.
There is already a configuration that makes FIT optional (CONFIG_FIT) but it is partially broken because it does not really remove FIT-related functionality from mkimage. That's the reason why it was not possible to disable FIT in the Buildroot package. My patch aims to fix that defect.
Carlos Santos (Casantos) DATACOM, P&D
Hello,
On Tue, 7 Jun 2016 21:18:17 -0300 (BRT), Carlos Santos wrote:
There is already a configuration that makes FIT optional (CONFIG_FIT) but it is partially broken because it does not really remove FIT-related functionality from mkimage. That's the reason why it was not possible to disable FIT in the Buildroot package. My patch aims to fix that defect.
Ah, indeed, I forgot that the CONFIG_FIT option already existed. In this case, I agree that it should either exist and work, or not exist.
Thomas
On Wed, Jun 08, 2016 at 07:47:12AM +0200, Thomas Petazzoni wrote:
Hello,
On Tue, 7 Jun 2016 21:18:17 -0300 (BRT), Carlos Santos wrote:
There is already a configuration that makes FIT optional (CONFIG_FIT) but it is partially broken because it does not really remove FIT-related functionality from mkimage. That's the reason why it was not possible to disable FIT in the Buildroot package. My patch aims to fix that defect.
Ah, indeed, I forgot that the CONFIG_FIT option already existed. In this case, I agree that it should either exist and work, or not exist.
Here is the problem with host tools living along side configurable binaries and sharing code. With respect to mkimage, FIT support is non-optional. The only reason that FIT signature stuff is optional is that it introduces too high of a set of dependencies for everyone else to have installed to even compile mkimage which is in turn required by the vast majority of targets. If the failing message that we couldn't run dtc is not clear enoguh, I am happy to take a patch to make the problem and solution clearer.
I would go so far as to say that requiring bison and m4 (along with dtc) to already be compiled in buildroot is not a step too far, but I'm an OpenEmbedded guy so it's possible I'm just dependency-happy :) Thanks!
Hi,
On 8 June 2016 at 05:17, Tom Rini trini@konsulko.com wrote:
On Wed, Jun 08, 2016 at 07:47:12AM +0200, Thomas Petazzoni wrote:
Hello,
On Tue, 7 Jun 2016 21:18:17 -0300 (BRT), Carlos Santos wrote:
There is already a configuration that makes FIT optional (CONFIG_FIT) but it is partially broken because it does not really remove FIT-related functionality from mkimage. That's the reason why it was not possible to disable FIT in the Buildroot package. My patch aims to fix that defect.
Ah, indeed, I forgot that the CONFIG_FIT option already existed. In this case, I agree that it should either exist and work, or not exist.
Here is the problem with host tools living along side configurable binaries and sharing code. With respect to mkimage, FIT support is non-optional. The only reason that FIT signature stuff is optional is that it introduces too high of a set of dependencies for everyone else to have installed to even compile mkimage which is in turn required by the vast majority of targets. If the failing message that we couldn't run dtc is not clear enoguh, I am happy to take a patch to make the problem and solution clearer.
I agree. I would prefer to always support signatures in mkimage, if we can.
The only purpose of turning off FIT in U-Boot is to reduce code size. But IMO it should be enabled by default in U-Boot. It is suppose to be the official format...
Regards, Simon
participants (5)
-
Carlos Santos -
Otavio Salvador -
Simon Glass -
Thomas Petazzoni -
Tom Rini