[PATCH 00/18] image: Reduce #ifdef abuse in image code

[PATCH 00/18] image: Reduce #ifdef abuse in image code

[Alexandru Gagniuc]

This is a combination of select patches from Simon's series:
    "image: Reduce #ifdefs and ad-hoc defines in image code"
and alternative solutions I proposed in:
    "image: Reduce the abuse of #ifdefs in image-sig.c"

After syncing with Simon, we agree that this is a reasonable base for
further work in #ifdef reduction. Rather than describing changes from
Simon's series or mine, we present this series de novo.
Most patches are already peer-reviewed.

The latest patch in this series is optional:
    'image: Add support for relocating crypto_algos in linker lists"
I don't have a way to test it, and I don't know if it's still needed.
I have included it for completeness.

Alexandru Gagniuc (10):
  common: Move host-only logic in image-sig.c to separate file
  common: image-sig.c: Remove host-specific logic and #ifdefs
  image: Add support for placing crypto_algo in linker lists
  image: rsa: Move verification algorithm to a linker list
  image: image-sig.c: Remove crypto_algos array
  lib: ecdsa: Remove #ifdefs from ecdsa.h
  lib: rsa: Remove #ifdefs from rsa.h
  image: Eliminate IMAGE_ENABLE_VERIFY macro
  image: Eliminate IMAGE_ENABLE_VERIFY_ECDSA macro
  [UNTESTED] image: Add support for relocating crypto_algos in linker
    lists

Simon Glass (8):
  image: Shorten FIT_ENABLE_SHAxxx_SUPPORT
  image: Rename SPL_SHAxxx_SUPPORT to SPL_FIT_SHAxxx
  image: Rename CONFIG_FIT_ENABLE_RSASSA_PSS_SUPPORT
  Kconfig: Rename SPL_CRC32_SUPPORT to SPL_CRC32
  Kconfig: Rename SPL_MD5_SUPPORT to SPL_MD5
  image: Drop IMAGE_ENABLE_SHA1
  image: Drop IMAGE_ENABLE_SHAxxx
  image: Drop IMAGE_ENABLE_BEST_MATCH

 common/Kconfig.boot                       |   8 +-
 common/image-fit.c                        |  10 +-
 common/image-sig.c                        |  75 +++---------
 common/spl/Kconfig                        |  14 +--
 configs/axm_defconfig                     |   2 +-
 configs/bcm963158_ram_defconfig           |   2 +-
 configs/chromebit_mickey_defconfig        |   2 +-
 configs/chromebook_jerry_defconfig        |   2 +-
 configs/chromebook_minnie_defconfig       |   2 +-
 configs/chromebook_speedy_defconfig       |   2 +-
 configs/evb-px30_defconfig                |   2 +-
 configs/firefly-px30_defconfig            |   2 +-
 configs/imxrt1020-evk_defconfig           |   2 +-
 configs/imxrt1050-evk_defconfig           |   2 +-
 configs/mt8516_pumpkin_defconfig          |   2 +-
 configs/odroid-go2_defconfig              |   2 +-
 configs/px30-core-ctouch2-px30_defconfig  |   2 +-
 configs/px30-core-edimm2.2-px30_defconfig |   2 +-
 configs/sandbox_defconfig                 |   2 +-
 configs/socfpga_agilex_atf_defconfig      |   2 +-
 configs/socfpga_agilex_vab_defconfig      |   2 +-
 configs/socfpga_stratix10_atf_defconfig   |   2 +-
 configs/taurus_defconfig                  |   2 +-
 include/image.h                           |  59 ++--------
 include/u-boot/ecdsa.h                    |  25 ----
 include/u-boot/rsa.h                      |  51 +--------
 lib/rsa/rsa-sign.c                        |   4 +-
 lib/rsa/rsa-verify.c                      |  18 ++-
 tools/Makefile                            |   5 +-
 tools/image-sig-host.c                    | 133 ++++++++++++++++++++++
 30 files changed, 220 insertions(+), 220 deletions(-)
 create mode 100644 tools/image-sig-host.c

-- 
2.31.1

[PATCH 01/18] image: Shorten FIT_ENABLE_SHAxxx_SUPPORT

[Alexandru Gagniuc]

From: Simon Glass <sjg@chromium.org>

The ENABLE part of this name is redundant, since all boolean Kconfig
options serve to enable something. The SUPPORT part is also redundant
since Kconfigs can be assumed to enable support for something. Together
they just serve to make these options overly long and inconsistent
with other options.

Rename FIT_ENABLE_SHAxxx_SUPPORT to FIT_SHAxxx

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/Kconfig.boot              |  6 +++---
 configs/mt8516_pumpkin_defconfig |  2 +-
 include/image.h                  | 12 ++++++------
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/common/Kconfig.boot b/common/Kconfig.boot
index 5a18d62d78..af3325a7ce 100644
--- a/common/Kconfig.boot
+++ b/common/Kconfig.boot
@@ -35,7 +35,7 @@ config FIT_EXTERNAL_OFFSET
 	  could be put in the hole between data payload and fit image
 	  header, such as CSF data on i.MX platform.
 
-config FIT_ENABLE_SHA256_SUPPORT
+config FIT_SHA256
 	bool "Support SHA256 checksum of FIT image contents"
 	default y
 	select SHA256
@@ -44,7 +44,7 @@ config FIT_ENABLE_SHA256_SUPPORT
 	  SHA256 checksum is a 256-bit (32-byte) hash value used to check that
 	  the image contents have not been corrupted.
 
-config FIT_ENABLE_SHA384_SUPPORT
+config FIT_SHA384
 	bool "Support SHA384 checksum of FIT image contents"
 	default n
 	select SHA384
@@ -54,7 +54,7 @@ config FIT_ENABLE_SHA384_SUPPORT
 	  the image contents have not been corrupted. Use this for the highest
 	  security.
 
-config FIT_ENABLE_SHA512_SUPPORT
+config FIT_SHA512
 	bool "Support SHA512 checksum of FIT image contents"
 	default n
 	select SHA512
diff --git a/configs/mt8516_pumpkin_defconfig b/configs/mt8516_pumpkin_defconfig
index 780660058d..356f18acd8 100644
--- a/configs/mt8516_pumpkin_defconfig
+++ b/configs/mt8516_pumpkin_defconfig
@@ -13,7 +13,7 @@ CONFIG_DEBUG_UART_CLOCK=26000000
 CONFIG_DEFAULT_DEVICE_TREE="mt8516-pumpkin"
 CONFIG_DEBUG_UART=y
 CONFIG_FIT=y
-# CONFIG_FIT_ENABLE_SHA256_SUPPORT is not set
+# CONFIG_FIT_SHA256 is not set
 # CONFIG_ARCH_FIXUP_FDT_MEMORY is not set
 CONFIG_DEFAULT_FDT_FILE="mt8516-pumpkin"
 # CONFIG_DISPLAY_BOARDINFO is not set
diff --git a/include/image.h b/include/image.h
index 459685d4d4..9319a779b9 100644
--- a/include/image.h
+++ b/include/image.h
@@ -31,9 +31,9 @@ struct fdt_region;
 #define IMAGE_ENABLE_OF_LIBFDT	1
 #define CONFIG_FIT_VERBOSE	1 /* enable fit_format_{error,warning}() */
 #define CONFIG_FIT_ENABLE_RSASSA_PSS_SUPPORT 1
-#define CONFIG_FIT_ENABLE_SHA256_SUPPORT
-#define CONFIG_FIT_ENABLE_SHA384_SUPPORT
-#define CONFIG_FIT_ENABLE_SHA512_SUPPORT
+#define CONFIG_FIT_SHA256
+#define CONFIG_FIT_SHA384
+#define CONFIG_FIT_SHA512
 #define CONFIG_SHA1
 #define CONFIG_SHA256
 #define CONFIG_SHA384
@@ -89,21 +89,21 @@ struct fdt_region;
 #define IMAGE_ENABLE_SHA1	0
 #endif
 
-#if defined(CONFIG_FIT_ENABLE_SHA256_SUPPORT) || \
+#if defined(CONFIG_FIT_SHA256) || \
 	defined(CONFIG_SPL_SHA256_SUPPORT)
 #define IMAGE_ENABLE_SHA256	1
 #else
 #define IMAGE_ENABLE_SHA256	0
 #endif
 
-#if defined(CONFIG_FIT_ENABLE_SHA384_SUPPORT) || \
+#if defined(CONFIG_FIT_SHA384) || \
 	defined(CONFIG_SPL_SHA384_SUPPORT)
 #define IMAGE_ENABLE_SHA384	1
 #else
 #define IMAGE_ENABLE_SHA384	0
 #endif
 
-#if defined(CONFIG_FIT_ENABLE_SHA512_SUPPORT) || \
+#if defined(CONFIG_FIT_SHA512) || \
 	defined(CONFIG_SPL_SHA512_SUPPORT)
 #define IMAGE_ENABLE_SHA512	1
 #else
-- 
2.31.1

[PATCH 02/18] image: Rename SPL_SHAxxx_SUPPORT to SPL_FIT_SHAxxx

[Alexandru Gagniuc]

From: Simon Glass <sjg@chromium.org>

These option are named inconsistently with other SPL options, thus making
them incompatible with the CONFIG_IS_ENABLED() macro. Rename them.

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/spl/Kconfig | 8 ++++----
 include/image.h    | 8 ++++----
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/common/spl/Kconfig b/common/spl/Kconfig
index df5468f1ac..d94b989217 100644
--- a/common/spl/Kconfig
+++ b/common/spl/Kconfig
@@ -429,7 +429,7 @@ config SPL_MD5_SUPPORT
 	  applications where images may be changed maliciously, you should
 	  consider SHA256 or SHA384.
 
-config SPL_SHA1_SUPPORT
+config SPL_FIT_SHA1
 	bool "Support SHA1"
 	depends on SPL_FIT
 	select SHA1
@@ -441,7 +441,7 @@ config SPL_SHA1_SUPPORT
 	  due to the expanding computing power available to brute-force
 	  attacks. For more security, consider SHA256 or SHA384.
 
-config SPL_SHA256_SUPPORT
+config SPL_FIT_SHA256
 	bool "Support SHA256"
 	depends on SPL_FIT
 	select SHA256
@@ -450,7 +450,7 @@ config SPL_SHA256_SUPPORT
 	  checksum is a 256-bit (32-byte) hash value used to check that the
 	  image contents have not been corrupted.
 
-config SPL_SHA384_SUPPORT
+config SPL_FIT_SHA384
 	bool "Support SHA384"
 	depends on SPL_FIT
 	select SHA384
@@ -461,7 +461,7 @@ config SPL_SHA384_SUPPORT
 	  image contents have not been corrupted. Use this for the highest
 	  security.
 
-config SPL_SHA512_SUPPORT
+config SPL_FIT_SHA512
 	bool "Support SHA512"
 	depends on SPL_FIT
 	select SHA512
diff --git a/include/image.h b/include/image.h
index 9319a779b9..3284f36c97 100644
--- a/include/image.h
+++ b/include/image.h
@@ -68,7 +68,7 @@ struct fdt_region;
 #  ifdef CONFIG_SPL_MD5_SUPPORT
 #   define IMAGE_ENABLE_MD5	1
 #  endif
-#  ifdef CONFIG_SPL_SHA1_SUPPORT
+#  ifdef CONFIG_SPL_FIT_SHA1
 #   define IMAGE_ENABLE_SHA1	1
 #  endif
 # else
@@ -90,21 +90,21 @@ struct fdt_region;
 #endif
 
 #if defined(CONFIG_FIT_SHA256) || \
-	defined(CONFIG_SPL_SHA256_SUPPORT)
+	defined(CONFIG_SPL_FIT_SHA256)
 #define IMAGE_ENABLE_SHA256	1
 #else
 #define IMAGE_ENABLE_SHA256	0
 #endif
 
 #if defined(CONFIG_FIT_SHA384) || \
-	defined(CONFIG_SPL_SHA384_SUPPORT)
+	defined(CONFIG_SPL_FIT_SHA384)
 #define IMAGE_ENABLE_SHA384	1
 #else
 #define IMAGE_ENABLE_SHA384	0
 #endif
 
 #if defined(CONFIG_FIT_SHA512) || \
-	defined(CONFIG_SPL_SHA512_SUPPORT)
+	defined(CONFIG_SPL_FIT_SHA512)
 #define IMAGE_ENABLE_SHA512	1
 #else
 #define IMAGE_ENABLE_SHA512	0
-- 
2.31.1

[PATCH 03/18] image: Rename CONFIG_FIT_ENABLE_RSASSA_PSS_SUPPORT

[Alexandru Gagniuc]

From: Simon Glass <sjg@chromium.org>

Drop the ENABLE and SUPPORT parts of this, which are redundant.

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/Kconfig.boot             | 2 +-
 common/image-sig.c              | 4 ++--
 configs/bcm963158_ram_defconfig | 2 +-
 configs/sandbox_defconfig       | 2 +-
 include/image.h                 | 2 +-
 include/u-boot/rsa.h            | 8 ++++----
 lib/rsa/rsa-sign.c              | 4 ++--
 lib/rsa/rsa-verify.c            | 2 +-
 8 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/common/Kconfig.boot b/common/Kconfig.boot
index af3325a7ce..03a6e6f214 100644
--- a/common/Kconfig.boot
+++ b/common/Kconfig.boot
@@ -103,7 +103,7 @@ config FIT_SIGNATURE_MAX_SIZE
 	  device memory. Assure this size does not extend past expected storage
 	  space.
 
-config FIT_ENABLE_RSASSA_PSS_SUPPORT
+config FIT_RSASSA_PSS
 	bool "Support rsassa-pss signature scheme of FIT image contents"
 	depends on FIT_SIGNATURE
 	default n
diff --git a/common/image-sig.c b/common/image-sig.c
index 0f8e592aba..8b5cecbfa4 100644
--- a/common/image-sig.c
+++ b/common/image-sig.c
@@ -99,12 +99,12 @@ struct padding_algo padding_algos[] = {
 		.name = "pkcs-1.5",
 		.verify = padding_pkcs_15_verify,
 	},
-#ifdef CONFIG_FIT_ENABLE_RSASSA_PSS_SUPPORT
+#ifdef CONFIG_FIT_RSASSA_PSS
 	{
 		.name = "pss",
 		.verify = padding_pss_verify,
 	}
-#endif /* CONFIG_FIT_ENABLE_RSASSA_PSS_SUPPORT */
+#endif /* CONFIG_FIT_RSASSA_PSS */
 };
 
 struct checksum_algo *image_get_checksum_algo(const char *full_name)
diff --git a/configs/bcm963158_ram_defconfig b/configs/bcm963158_ram_defconfig
index 0be1e0981a..ec006514d1 100644
--- a/configs/bcm963158_ram_defconfig
+++ b/configs/bcm963158_ram_defconfig
@@ -11,7 +11,7 @@ CONFIG_DEFAULT_DEVICE_TREE="bcm963158"
 CONFIG_ENV_VARS_UBOOT_CONFIG=y
 CONFIG_FIT=y
 CONFIG_FIT_SIGNATURE=y
-CONFIG_FIT_ENABLE_RSASSA_PSS_SUPPORT=y
+CONFIG_FIT_RSASSA_PSS=y
 CONFIG_FIT_VERBOSE=y
 CONFIG_LEGACY_IMAGE_FORMAT=y
 CONFIG_SUPPORT_RAW_INITRD=y
diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
index bdbf714e2b..7b8603d1ef 100644
--- a/configs/sandbox_defconfig
+++ b/configs/sandbox_defconfig
@@ -10,7 +10,7 @@ CONFIG_DEBUG_UART=y
 CONFIG_DISTRO_DEFAULTS=y
 CONFIG_FIT=y
 CONFIG_FIT_SIGNATURE=y
-CONFIG_FIT_ENABLE_RSASSA_PSS_SUPPORT=y
+CONFIG_FIT_RSASSA_PSS=y
 CONFIG_FIT_CIPHER=y
 CONFIG_FIT_VERBOSE=y
 CONFIG_BOOTSTAGE=y
diff --git a/include/image.h b/include/image.h
index 3284f36c97..9eb45ffd40 100644
--- a/include/image.h
+++ b/include/image.h
@@ -30,7 +30,7 @@ struct fdt_region;
 #define IMAGE_ENABLE_FIT	1
 #define IMAGE_ENABLE_OF_LIBFDT	1
 #define CONFIG_FIT_VERBOSE	1 /* enable fit_format_{error,warning}() */
-#define CONFIG_FIT_ENABLE_RSASSA_PSS_SUPPORT 1
+#define CONFIG_FIT_RSASSA_PSS 1
 #define CONFIG_FIT_SHA256
 #define CONFIG_FIT_SHA384
 #define CONFIG_FIT_SHA512
diff --git a/include/u-boot/rsa.h b/include/u-boot/rsa.h
index bed1c097c2..bc564d56fa 100644
--- a/include/u-boot/rsa.h
+++ b/include/u-boot/rsa.h
@@ -119,11 +119,11 @@ int padding_pkcs_15_verify(struct image_sign_info *info,
 			   uint8_t *msg, int msg_len,
 			   const uint8_t *hash, int hash_len);
 
-#ifdef CONFIG_FIT_ENABLE_RSASSA_PSS_SUPPORT
+#ifdef CONFIG_FIT_RSASSA_PSS
 int padding_pss_verify(struct image_sign_info *info,
 		       uint8_t *msg, int msg_len,
 		       const uint8_t *hash, int hash_len);
-#endif /* CONFIG_FIT_ENABLE_RSASSA_PSS_SUPPORT */
+#endif /* CONFIG_FIT_RSASSA_PSS */
 #else
 static inline int rsa_verify_hash(struct image_sign_info *info,
 				  const uint8_t *hash,
@@ -146,14 +146,14 @@ static inline int padding_pkcs_15_verify(struct image_sign_info *info,
 	return -ENXIO;
 }
 
-#ifdef CONFIG_FIT_ENABLE_RSASSA_PSS_SUPPORT
+#ifdef CONFIG_FIT_RSASSA_PSS
 static inline int padding_pss_verify(struct image_sign_info *info,
 				     uint8_t *msg, int msg_len,
 				     const uint8_t *hash, int hash_len)
 {
 	return -ENXIO;
 }
-#endif /* CONFIG_FIT_ENABLE_RSASSA_PSS_SUPPORT */
+#endif /* CONFIG_FIT_RSASSA_PSS */
 #endif
 
 #define RSA_DEFAULT_PADDING_NAME		"pkcs-1.5"
diff --git a/lib/rsa/rsa-sign.c b/lib/rsa/rsa-sign.c
index 5a1583b8f7..f4ed11e74a 100644
--- a/lib/rsa/rsa-sign.c
+++ b/lib/rsa/rsa-sign.c
@@ -442,7 +442,7 @@ static int rsa_sign_with_key(EVP_PKEY *pkey, struct padding_algo *padding_algo,
 		goto err_sign;
 	}
 
-#ifdef CONFIG_FIT_ENABLE_RSASSA_PSS_SUPPORT
+#ifdef CONFIG_FIT_RSASSA_PSS
 	if (padding_algo && !strcmp(padding_algo->name, "pss")) {
 		if (EVP_PKEY_CTX_set_rsa_padding(ckey,
 						 RSA_PKCS1_PSS_PADDING) <= 0) {
@@ -450,7 +450,7 @@ static int rsa_sign_with_key(EVP_PKEY *pkey, struct padding_algo *padding_algo,
 			goto err_sign;
 		}
 	}
-#endif /* CONFIG_FIT_ENABLE_RSASSA_PSS_SUPPORT */
+#endif /* CONFIG_FIT_RSASSA_PSS */
 
 	for (i = 0; i < region_count; i++) {
 		if (!EVP_DigestSignUpdate(context, region[i].data,
diff --git a/lib/rsa/rsa-verify.c b/lib/rsa/rsa-verify.c
index aee76f42d5..1998c773fc 100644
--- a/lib/rsa/rsa-verify.c
+++ b/lib/rsa/rsa-verify.c
@@ -95,7 +95,7 @@ int padding_pkcs_15_verify(struct image_sign_info *info,
 	return 0;
 }
 
-#ifdef CONFIG_FIT_ENABLE_RSASSA_PSS_SUPPORT
+#ifdef CONFIG_FIT_RSASSA_PSS
 static void u32_i2osp(uint32_t val, uint8_t *buf)
 {
 	buf[0] = (uint8_t)((val >> 24) & 0xff);
-- 
2.31.1

[PATCH 04/18] Kconfig: Rename SPL_CRC32_SUPPORT to SPL_CRC32

[Alexandru Gagniuc]

From: Simon Glass <sjg@chromium.org>

Drop the _SUPPORT suffix so we can use CONFIG_IS_ENABLED() with this
option.

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/spl/Kconfig                        | 4 ++--
 configs/axm_defconfig                     | 2 +-
 configs/chromebit_mickey_defconfig        | 2 +-
 configs/chromebook_jerry_defconfig        | 2 +-
 configs/chromebook_minnie_defconfig       | 2 +-
 configs/chromebook_speedy_defconfig       | 2 +-
 configs/evb-px30_defconfig                | 2 +-
 configs/firefly-px30_defconfig            | 2 +-
 configs/imxrt1020-evk_defconfig           | 2 +-
 configs/imxrt1050-evk_defconfig           | 2 +-
 configs/odroid-go2_defconfig              | 2 +-
 configs/px30-core-ctouch2-px30_defconfig  | 2 +-
 configs/px30-core-edimm2.2-px30_defconfig | 2 +-
 configs/socfpga_agilex_atf_defconfig      | 2 +-
 configs/socfpga_agilex_vab_defconfig      | 2 +-
 configs/socfpga_stratix10_atf_defconfig   | 2 +-
 configs/taurus_defconfig                  | 2 +-
 include/image.h                           | 2 +-
 18 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/common/spl/Kconfig b/common/spl/Kconfig
index d94b989217..0329d93b29 100644
--- a/common/spl/Kconfig
+++ b/common/spl/Kconfig
@@ -204,7 +204,7 @@ config SPL_LEGACY_IMAGE_SUPPORT
 config SPL_LEGACY_IMAGE_CRC_CHECK
 	bool "Check CRC of Legacy images"
 	depends on SPL_LEGACY_IMAGE_SUPPORT
-	select SPL_CRC32_SUPPORT
+	select SPL_CRC32
 	help
 	  Enable this to check the CRC of Legacy images. While this increases
 	  reliability, it affects both code size and boot duration.
@@ -407,7 +407,7 @@ config SYS_MMCSD_RAW_MODE_EMMC_BOOT_PARTITION
 	  the eMMC EXT_CSC_PART_CONFIG selection should be overridden in SPL
 	  by user defined partition number.
 
-config SPL_CRC32_SUPPORT
+config SPL_CRC32
 	bool "Support CRC32"
 	default y if SPL_LEGACY_IMAGE_SUPPORT
 	help
diff --git a/configs/axm_defconfig b/configs/axm_defconfig
index 0bfd7548b0..4e776fd695 100644
--- a/configs/axm_defconfig
+++ b/configs/axm_defconfig
@@ -32,7 +32,7 @@ CONFIG_BOOTCOMMAND="run flash_self"
 CONFIG_BOARD_EARLY_INIT_F=y
 # CONFIG_SPL_LEGACY_IMAGE_SUPPORT is not set
 CONFIG_SPL_SYS_MALLOC_SIMPLE=y
-CONFIG_SPL_CRC32_SUPPORT=y
+CONFIG_SPL_CRC32=y
 CONFIG_SPL_NAND_SUPPORT=y
 CONFIG_SPL_NAND_DRIVERS=y
 CONFIG_SPL_NAND_ECC=y
diff --git a/configs/chromebit_mickey_defconfig b/configs/chromebit_mickey_defconfig
index c09b63b946..2b664e118c 100644
--- a/configs/chromebit_mickey_defconfig
+++ b/configs/chromebit_mickey_defconfig
@@ -25,7 +25,7 @@ CONFIG_BOARD_EARLY_INIT_R=y
 CONFIG_SPL_STACK_R=y
 CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN=0x2000
 # CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_SECTOR is not set
-# CONFIG_SPL_CRC32_SUPPORT is not set
+# CONFIG_SPL_CRC32 is not set
 CONFIG_SPL_SPI_LOAD=y
 CONFIG_CMD_GPIO=y
 CONFIG_CMD_GPT=y
diff --git a/configs/chromebook_jerry_defconfig b/configs/chromebook_jerry_defconfig
index 692b630174..a757d259f5 100644
--- a/configs/chromebook_jerry_defconfig
+++ b/configs/chromebook_jerry_defconfig
@@ -26,7 +26,7 @@ CONFIG_BOARD_EARLY_INIT_R=y
 CONFIG_SPL_STACK_R=y
 CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN=0x2000
 # CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_SECTOR is not set
-# CONFIG_SPL_CRC32_SUPPORT is not set
+# CONFIG_SPL_CRC32 is not set
 CONFIG_SPL_SPI_LOAD=y
 CONFIG_CMD_GPIO=y
 CONFIG_CMD_GPT=y
diff --git a/configs/chromebook_minnie_defconfig b/configs/chromebook_minnie_defconfig
index ae55842e3b..353aa01ea9 100644
--- a/configs/chromebook_minnie_defconfig
+++ b/configs/chromebook_minnie_defconfig
@@ -26,7 +26,7 @@ CONFIG_BOARD_EARLY_INIT_R=y
 CONFIG_SPL_STACK_R=y
 CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN=0x2000
 # CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_SECTOR is not set
-# CONFIG_SPL_CRC32_SUPPORT is not set
+# CONFIG_SPL_CRC32 is not set
 CONFIG_SPL_SPI_LOAD=y
 CONFIG_CMD_GPIO=y
 CONFIG_CMD_GPT=y
diff --git a/configs/chromebook_speedy_defconfig b/configs/chromebook_speedy_defconfig
index 4b460ee6a9..c5be5597b1 100644
--- a/configs/chromebook_speedy_defconfig
+++ b/configs/chromebook_speedy_defconfig
@@ -26,7 +26,7 @@ CONFIG_BOARD_EARLY_INIT_R=y
 CONFIG_SPL_STACK_R=y
 CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN=0x2000
 # CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_SECTOR is not set
-# CONFIG_SPL_CRC32_SUPPORT is not set
+# CONFIG_SPL_CRC32 is not set
 CONFIG_SPL_SPI_LOAD=y
 CONFIG_CMD_GPIO=y
 CONFIG_CMD_GPT=y
diff --git a/configs/evb-px30_defconfig b/configs/evb-px30_defconfig
index d2fdfef293..55e2702a17 100644
--- a/configs/evb-px30_defconfig
+++ b/configs/evb-px30_defconfig
@@ -29,7 +29,7 @@ CONFIG_SPL_BOOTROM_SUPPORT=y
 # CONFIG_SPL_RAW_IMAGE_SUPPORT is not set
 CONFIG_SPL_STACK_R=y
 # CONFIG_TPL_BANNER_PRINT is not set
-CONFIG_SPL_CRC32_SUPPORT=y
+CONFIG_SPL_CRC32=y
 CONFIG_SPL_ATF=y
 # CONFIG_TPL_FRAMEWORK is not set
 # CONFIG_CMD_BOOTD is not set
diff --git a/configs/firefly-px30_defconfig b/configs/firefly-px30_defconfig
index 6487615fe0..978a360405 100644
--- a/configs/firefly-px30_defconfig
+++ b/configs/firefly-px30_defconfig
@@ -30,7 +30,7 @@ CONFIG_SPL_BOOTROM_SUPPORT=y
 # CONFIG_SPL_RAW_IMAGE_SUPPORT is not set
 CONFIG_SPL_STACK_R=y
 # CONFIG_TPL_BANNER_PRINT is not set
-CONFIG_SPL_CRC32_SUPPORT=y
+CONFIG_SPL_CRC32=y
 CONFIG_SPL_ATF=y
 # CONFIG_TPL_FRAMEWORK is not set
 # CONFIG_CMD_BOOTD is not set
diff --git a/configs/imxrt1020-evk_defconfig b/configs/imxrt1020-evk_defconfig
index ad408ebef8..dec55de695 100644
--- a/configs/imxrt1020-evk_defconfig
+++ b/configs/imxrt1020-evk_defconfig
@@ -24,7 +24,7 @@ CONFIG_SPL_BOARD_INIT=y
 CONFIG_SPL_SYS_MALLOC_SIMPLE=y
 CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_SECTOR=y
 CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR=0x100
-# CONFIG_SPL_CRC32_SUPPORT is not set
+# CONFIG_SPL_CRC32 is not set
 # CONFIG_BOOTM_NETBSD is not set
 # CONFIG_BOOTM_PLAN9 is not set
 # CONFIG_BOOTM_RTEMS is not set
diff --git a/configs/imxrt1050-evk_defconfig b/configs/imxrt1050-evk_defconfig
index d03572e7db..08379ae022 100644
--- a/configs/imxrt1050-evk_defconfig
+++ b/configs/imxrt1050-evk_defconfig
@@ -27,7 +27,7 @@ CONFIG_SPL_BOARD_INIT=y
 CONFIG_SPL_SYS_MALLOC_SIMPLE=y
 CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_SECTOR=y
 CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR=0x100
-# CONFIG_SPL_CRC32_SUPPORT is not set
+# CONFIG_SPL_CRC32 is not set
 # CONFIG_BOOTM_NETBSD is not set
 # CONFIG_BOOTM_PLAN9 is not set
 # CONFIG_BOOTM_RTEMS is not set
diff --git a/configs/odroid-go2_defconfig b/configs/odroid-go2_defconfig
index 6aa41e3755..82e340a16e 100644
--- a/configs/odroid-go2_defconfig
+++ b/configs/odroid-go2_defconfig
@@ -33,7 +33,7 @@ CONFIG_SPL_BOOTROM_SUPPORT=y
 # CONFIG_SPL_RAW_IMAGE_SUPPORT is not set
 CONFIG_SPL_STACK_R=y
 # CONFIG_TPL_BANNER_PRINT is not set
-CONFIG_SPL_CRC32_SUPPORT=y
+CONFIG_SPL_CRC32=y
 CONFIG_SPL_I2C_SUPPORT=y
 CONFIG_SPL_POWER_SUPPORT=y
 CONFIG_SPL_ATF=y
diff --git a/configs/px30-core-ctouch2-px30_defconfig b/configs/px30-core-ctouch2-px30_defconfig
index 1afc146bbf..3ac0ea4ccc 100644
--- a/configs/px30-core-ctouch2-px30_defconfig
+++ b/configs/px30-core-ctouch2-px30_defconfig
@@ -30,7 +30,7 @@ CONFIG_SPL_BOOTROM_SUPPORT=y
 # CONFIG_SPL_RAW_IMAGE_SUPPORT is not set
 CONFIG_SPL_STACK_R=y
 # CONFIG_TPL_BANNER_PRINT is not set
-CONFIG_SPL_CRC32_SUPPORT=y
+CONFIG_SPL_CRC32=y
 CONFIG_SPL_ATF=y
 # CONFIG_TPL_FRAMEWORK is not set
 # CONFIG_CMD_BOOTD is not set
diff --git a/configs/px30-core-edimm2.2-px30_defconfig b/configs/px30-core-edimm2.2-px30_defconfig
index 9d78eee84d..f208297de5 100644
--- a/configs/px30-core-edimm2.2-px30_defconfig
+++ b/configs/px30-core-edimm2.2-px30_defconfig
@@ -30,7 +30,7 @@ CONFIG_SPL_BOOTROM_SUPPORT=y
 # CONFIG_SPL_RAW_IMAGE_SUPPORT is not set
 CONFIG_SPL_STACK_R=y
 # CONFIG_TPL_BANNER_PRINT is not set
-CONFIG_SPL_CRC32_SUPPORT=y
+CONFIG_SPL_CRC32=y
 CONFIG_SPL_ATF=y
 # CONFIG_TPL_FRAMEWORK is not set
 # CONFIG_CMD_BOOTD is not set
diff --git a/configs/socfpga_agilex_atf_defconfig b/configs/socfpga_agilex_atf_defconfig
index 29e3fb865e..8142cbdd4e 100644
--- a/configs/socfpga_agilex_atf_defconfig
+++ b/configs/socfpga_agilex_atf_defconfig
@@ -23,7 +23,7 @@ CONFIG_USE_BOOTARGS=y
 CONFIG_BOOTARGS="earlycon"
 CONFIG_USE_BOOTCOMMAND=y
 CONFIG_BOOTCOMMAND="run fatscript; run mmcfitload; run linux_qspi_enable; run mmcfitboot"
-CONFIG_SPL_CRC32_SUPPORT=y
+CONFIG_SPL_CRC32=y
 CONFIG_SPL_CACHE=y
 CONFIG_SPL_SPI_LOAD=y
 CONFIG_SPL_ATF=y
diff --git a/configs/socfpga_agilex_vab_defconfig b/configs/socfpga_agilex_vab_defconfig
index af726bab69..78b2b7558f 100644
--- a/configs/socfpga_agilex_vab_defconfig
+++ b/configs/socfpga_agilex_vab_defconfig
@@ -24,7 +24,7 @@ CONFIG_USE_BOOTARGS=y
 CONFIG_BOOTARGS="earlycon"
 CONFIG_USE_BOOTCOMMAND=y
 CONFIG_BOOTCOMMAND="run fatscript; run mmcfitload; run mmcfitboot"
-CONFIG_SPL_CRC32_SUPPORT=y
+CONFIG_SPL_CRC32=y
 CONFIG_SPL_CACHE=y
 CONFIG_SPL_SPI_LOAD=y
 CONFIG_SPL_ATF=y
diff --git a/configs/socfpga_stratix10_atf_defconfig b/configs/socfpga_stratix10_atf_defconfig
index 9f2f220c3a..bf89fe5a66 100644
--- a/configs/socfpga_stratix10_atf_defconfig
+++ b/configs/socfpga_stratix10_atf_defconfig
@@ -23,7 +23,7 @@ CONFIG_USE_BOOTARGS=y
 CONFIG_BOOTARGS="earlycon"
 CONFIG_USE_BOOTCOMMAND=y
 CONFIG_BOOTCOMMAND="run fatscript; run mmcfitload; run linux_qspi_enable; run mmcfitboot"
-CONFIG_SPL_CRC32_SUPPORT=y
+CONFIG_SPL_CRC32=y
 CONFIG_SPL_SPI_LOAD=y
 CONFIG_SPL_ATF=y
 CONFIG_SPL_ATF_NO_PLATFORM_PARAM=y
diff --git a/configs/taurus_defconfig b/configs/taurus_defconfig
index a79cdf3fa7..5caf9ab30d 100644
--- a/configs/taurus_defconfig
+++ b/configs/taurus_defconfig
@@ -36,7 +36,7 @@ CONFIG_BOOTCOMMAND="nand read 0x22000000 0x200000 0x300000; bootm"
 CONFIG_BOARD_EARLY_INIT_F=y
 # CONFIG_SPL_LEGACY_IMAGE_SUPPORT is not set
 CONFIG_SPL_SYS_MALLOC_SIMPLE=y
-CONFIG_SPL_CRC32_SUPPORT=y
+CONFIG_SPL_CRC32=y
 CONFIG_SPL_NAND_SUPPORT=y
 CONFIG_SPL_NAND_DRIVERS=y
 CONFIG_SPL_NAND_ECC=y
diff --git a/include/image.h b/include/image.h
index 9eb45ffd40..2c812d22a9 100644
--- a/include/image.h
+++ b/include/image.h
@@ -62,7 +62,7 @@ struct fdt_region;
 #include <linux/libfdt.h>
 #include <fdt_support.h>
 # ifdef CONFIG_SPL_BUILD
-#  ifdef CONFIG_SPL_CRC32_SUPPORT
+#  ifdef CONFIG_SPL_CRC32
 #   define IMAGE_ENABLE_CRC32	1
 #  endif
 #  ifdef CONFIG_SPL_MD5_SUPPORT
-- 
2.31.1

[PATCH 05/18] Kconfig: Rename SPL_MD5_SUPPORT to SPL_MD5

[Alexandru Gagniuc]

From: Simon Glass <sjg@chromium.org>

Drop the _SUPPORT suffix so we can use CONFIG_IS_ENABLED() with this
option.

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/spl/Kconfig | 2 +-
 include/image.h    | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/common/spl/Kconfig b/common/spl/Kconfig
index 0329d93b29..c83ce4d367 100644
--- a/common/spl/Kconfig
+++ b/common/spl/Kconfig
@@ -417,7 +417,7 @@ config SPL_CRC32
 	  for detected accidental image corruption. For secure applications you
 	  should consider SHA1 or SHA256.
 
-config SPL_MD5_SUPPORT
+config SPL_MD5
 	bool "Support MD5"
 	depends on SPL_FIT
 	help
diff --git a/include/image.h b/include/image.h
index 2c812d22a9..887a3270bd 100644
--- a/include/image.h
+++ b/include/image.h
@@ -65,7 +65,7 @@ struct fdt_region;
 #  ifdef CONFIG_SPL_CRC32
 #   define IMAGE_ENABLE_CRC32	1
 #  endif
-#  ifdef CONFIG_SPL_MD5_SUPPORT
+#  ifdef CONFIG_SPL_MD5
 #   define IMAGE_ENABLE_MD5	1
 #  endif
 #  ifdef CONFIG_SPL_FIT_SHA1
-- 
2.31.1

[PATCH 06/18] image: Drop IMAGE_ENABLE_SHA1

[Alexandru Gagniuc]

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) {
 		sha1_csum_wd((unsigned char *)data, data_len,
 			     (unsigned char *)value, CHUNKSZ_SHA1);
 		*value_len = 20;
diff --git a/include/image.h b/include/image.h
index 887a3270bd..8c718adba0 100644
--- a/include/image.h
+++ b/include/image.h
@@ -68,13 +68,9 @@ struct fdt_region;
 #  ifdef CONFIG_SPL_MD5
 #   define IMAGE_ENABLE_MD5	1
 #  endif
-#  ifdef CONFIG_SPL_FIT_SHA1
-#   define IMAGE_ENABLE_SHA1	1
-#  endif
 # else
 #  define IMAGE_ENABLE_CRC32	1
 #  define IMAGE_ENABLE_MD5	1
-#  define IMAGE_ENABLE_SHA1	1
 # endif
 
 #ifndef IMAGE_ENABLE_CRC32
@@ -85,10 +81,6 @@ struct fdt_region;
 #define IMAGE_ENABLE_MD5	0
 #endif
 
-#ifndef IMAGE_ENABLE_SHA1
-#define IMAGE_ENABLE_SHA1	0
-#endif
-
 #if defined(CONFIG_FIT_SHA256) || \
 	defined(CONFIG_SPL_FIT_SHA256)
 #define IMAGE_ENABLE_SHA256	1
-- 
2.31.1

[PATCH 07/18] image: Drop IMAGE_ENABLE_SHAxxx

[Alexandru Gagniuc]

From: Simon Glass <sjg@chromium.org>

We already have a host Kconfig for these SHA options. Use
CONFIG_IS_ENABLED(SHAxxx) directly in the code shared with the host build,
so we can drop the unnecessary indirections.

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 |  6 +++---
 include/image.h    | 21 ---------------------
 2 files changed, 3 insertions(+), 24 deletions(-)

diff --git a/common/image-fit.c b/common/image-fit.c
index 24e92a8e92..c0c75e92ba 100644
--- a/common/image-fit.c
+++ b/common/image-fit.c
@@ -1222,15 +1222,15 @@ int calculate_hash(const void *data, int data_len, const char *algo,
 		sha1_csum_wd((unsigned char *)data, data_len,
 			     (unsigned char *)value, CHUNKSZ_SHA1);
 		*value_len = 20;
-	} else if (IMAGE_ENABLE_SHA256 && strcmp(algo, "sha256") == 0) {
+	} else if (CONFIG_IS_ENABLED(SHA256) && strcmp(algo, "sha256") == 0) {
 		sha256_csum_wd((unsigned char *)data, data_len,
 			       (unsigned char *)value, CHUNKSZ_SHA256);
 		*value_len = SHA256_SUM_LEN;
-	} else if (IMAGE_ENABLE_SHA384 && strcmp(algo, "sha384") == 0) {
+	} else if (CONFIG_IS_ENABLED(SHA384) && strcmp(algo, "sha384") == 0) {
 		sha384_csum_wd((unsigned char *)data, data_len,
 			       (unsigned char *)value, CHUNKSZ_SHA384);
 		*value_len = SHA384_SUM_LEN;
-	} else if (IMAGE_ENABLE_SHA512 && strcmp(algo, "sha512") == 0) {
+	} else if (CONFIG_IS_ENABLED(SHA512) && strcmp(algo, "sha512") == 0) {
 		sha512_csum_wd((unsigned char *)data, data_len,
 			       (unsigned char *)value, CHUNKSZ_SHA512);
 		*value_len = SHA512_SUM_LEN;
diff --git a/include/image.h b/include/image.h
index 8c718adba0..aa03f0a722 100644
--- a/include/image.h
+++ b/include/image.h
@@ -81,27 +81,6 @@ struct fdt_region;
 #define IMAGE_ENABLE_MD5	0
 #endif
 
-#if defined(CONFIG_FIT_SHA256) || \
-	defined(CONFIG_SPL_FIT_SHA256)
-#define IMAGE_ENABLE_SHA256	1
-#else
-#define IMAGE_ENABLE_SHA256	0
-#endif
-
-#if defined(CONFIG_FIT_SHA384) || \
-	defined(CONFIG_SPL_FIT_SHA384)
-#define IMAGE_ENABLE_SHA384	1
-#else
-#define IMAGE_ENABLE_SHA384	0
-#endif
-
-#if defined(CONFIG_FIT_SHA512) || \
-	defined(CONFIG_SPL_FIT_SHA512)
-#define IMAGE_ENABLE_SHA512	1
-#else
-#define IMAGE_ENABLE_SHA512	0
-#endif
-
 #endif /* IMAGE_ENABLE_FIT */
 
 #ifdef CONFIG_SYS_BOOT_GET_CMDLINE
-- 
2.31.1

[PATCH 08/18] image: Drop IMAGE_ENABLE_BEST_MATCH

[Alexandru Gagniuc]

From: Simon Glass <sjg@chromium.org>

This is not needed with Kconfig, since we can use IS_ENABLED() easily
enough. Drop it.

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    | 5 -----
 2 files changed, 1 insertion(+), 6 deletions(-)

diff --git a/common/image-fit.c b/common/image-fit.c
index c0c75e92ba..5df2cf8571 100644
--- a/common/image-fit.c
+++ b/common/image-fit.c
@@ -2026,7 +2026,7 @@ int fit_image_load(bootm_headers_t *images, ulong addr,
 		 * fit_conf_get_node() will try to find default config node
 		 */
 		bootstage_mark(bootstage_id + BOOTSTAGE_SUB_NO_UNIT_NAME);
-		if (IMAGE_ENABLE_BEST_MATCH && !fit_uname_config) {
+		if (IS_ENABLED(CONFIG_FIT_BEST_MATCH) && !fit_uname_config) {
 			cfg_noffset = fit_conf_find_compat(fit, gd_fdt_blob());
 		} else {
 			cfg_noffset = fit_conf_get_node(fit,
diff --git a/include/image.h b/include/image.h
index aa03f0a722..dbb24993f1 100644
--- a/include/image.h
+++ b/include/image.h
@@ -1221,11 +1221,6 @@ void image_set_host_blob(void *host_blob);
 # define gd_fdt_blob()		(gd->fdt_blob)
 #endif
 
-#ifdef CONFIG_FIT_BEST_MATCH
-#define IMAGE_ENABLE_BEST_MATCH	1
-#else
-#define IMAGE_ENABLE_BEST_MATCH	0
-#endif
 #endif /* IMAGE_ENABLE_FIT */
 
 /*
-- 
2.31.1

[PATCH 09/18] common: Move host-only logic in image-sig.c to separate file

[Alexandru Gagniuc]

image-sig.c is used to map a hash or crypto algorithm name to a
handler of that algorithm. There is some similarity between the host
and target variants, with the differences worked out by #ifdefs. The
purpose of this change is to remove those ifdefs.

First, copy the file to a host-only version, and remove target
specific code. Although it looks like we are duplicating code,
subsequent patches will change the way target algorithms are searched.
Besides we are only duplicating three string to struct mapping
functions. This isn't something to fuss about.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 tools/Makefile         |   5 +-
 tools/image-sig-host.c | 133 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 136 insertions(+), 2 deletions(-)
 create mode 100644 tools/image-sig-host.c

diff --git a/tools/Makefile b/tools/Makefile
index d020c55d66..e39006b6f6 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -57,8 +57,9 @@ hostprogs-$(CONFIG_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_FIT) := image-sig-host.o fit_common.o fit_image.o \
+			image-host.o common/image-fit.o
+FIT_SIG_OBJS-$(CONFIG_FIT_SIGNATURE) := common/image-fit-sig.o
 FIT_CIPHER_OBJS-$(CONFIG_FIT_CIPHER) := common/image-cipher.o
 
 # The following files are synced with upstream DTC.
diff --git a/tools/image-sig-host.c b/tools/image-sig-host.c
new file mode 100644
index 0000000000..8ed6998dab
--- /dev/null
+++ b/tools/image-sig-host.c
@@ -0,0 +1,133 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2013, Google Inc.
+ */
+
+#include "mkimage.h"
+#include <fdt_support.h>
+#include <time.h>
+#include <linux/libfdt.h>
+#include <image.h>
+#include <u-boot/ecdsa.h>
+#include <u-boot/rsa.h>
+#include <u-boot/hash-checksum.h>
+
+struct checksum_algo checksum_algos[] = {
+	{
+		.name = "sha1",
+		.checksum_len = SHA1_SUM_LEN,
+		.der_len = SHA1_DER_LEN,
+		.der_prefix = sha1_der_prefix,
+		.calculate_sign = EVP_sha1,
+		.calculate = hash_calculate,
+	},
+	{
+		.name = "sha256",
+		.checksum_len = SHA256_SUM_LEN,
+		.der_len = SHA256_DER_LEN,
+		.der_prefix = sha256_der_prefix,
+		.calculate_sign = EVP_sha256,
+		.calculate = hash_calculate,
+	},
+	{
+		.name = "sha384",
+		.checksum_len = SHA384_SUM_LEN,
+		.der_len = SHA384_DER_LEN,
+		.der_prefix = sha384_der_prefix,
+		.calculate_sign = EVP_sha384,
+		.calculate = hash_calculate,
+	},
+	{
+		.name = "sha512",
+		.checksum_len = SHA512_SUM_LEN,
+		.der_len = SHA512_DER_LEN,
+		.der_prefix = sha512_der_prefix,
+		.calculate_sign = EVP_sha512,
+		.calculate = hash_calculate,
+	},
+};
+
+struct crypto_algo crypto_algos[] = {
+	{
+		.name = "rsa2048",
+		.key_len = RSA2048_BYTES,
+		.sign = rsa_sign,
+		.add_verify_data = rsa_add_verify_data,
+		.verify = rsa_verify,
+	},
+	{
+		.name = "rsa4096",
+		.key_len = RSA4096_BYTES,
+		.sign = rsa_sign,
+		.add_verify_data = rsa_add_verify_data,
+		.verify = rsa_verify,
+	},
+	{
+		.name = "ecdsa256",
+		.key_len = ECDSA256_BYTES,
+		.sign = ecdsa_sign,
+		.add_verify_data = ecdsa_add_verify_data,
+		.verify = ecdsa_verify,
+	},
+};
+
+struct padding_algo padding_algos[] = {
+	{
+		.name = "pkcs-1.5",
+		.verify = padding_pkcs_15_verify,
+	},
+	{
+		.name = "pss",
+		.verify = padding_pss_verify,
+	}
+};
+
+struct checksum_algo *image_get_checksum_algo(const char *full_name)
+{
+	int i;
+	const char *name;
+
+	for (i = 0; i < ARRAY_SIZE(checksum_algos); i++) {
+		name = checksum_algos[i].name;
+		/* Make sure names match and next char is a comma */
+		if (!strncmp(name, full_name, strlen(name)) &&
+		    full_name[strlen(name)] == ',')
+			return &checksum_algos[i];
+	}
+
+	return NULL;
+}
+
+struct crypto_algo *image_get_crypto_algo(const char *full_name)
+{
+	int i;
+	const char *name;
+
+	/* Move name to after the comma */
+	name = strchr(full_name, ',');
+	if (!name)
+		return NULL;
+	name += 1;
+
+	for (i = 0; i < ARRAY_SIZE(crypto_algos); i++) {
+		if (!strcmp(crypto_algos[i].name, name))
+			return &crypto_algos[i];
+	}
+
+	return NULL;
+}
+
+struct padding_algo *image_get_padding_algo(const char *name)
+{
+	int i;
+
+	if (!name)
+		return NULL;
+
+	for (i = 0; i < ARRAY_SIZE(padding_algos); i++) {
+		if (!strcmp(padding_algos[i].name, name))
+			return &padding_algos[i];
+	}
+
+	return NULL;
+}
-- 
2.31.1

[PATCH 10/18] common: image-sig.c: Remove host-specific logic and #ifdefs

[Alexandru Gagniuc]

Remove any ifdefs in image-sig.c that were previously used to
differentiate from the host code. Note that all code dedicated to
relocating ->sign() and ->add_verify_data)_ can be safely removed,
as signing is not supported target-side.

NOTE that although it appears we are removing ecdsa256 support, this
is intentional. ecdsa_verify() is a no-op on the target, and is
currently only used by host code.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 common/image-sig.c | 39 ++-------------------------------------
 1 file changed, 2 insertions(+), 37 deletions(-)

diff --git a/common/image-sig.c b/common/image-sig.c
index 8b5cecbfa4..5e2d171975 100644
--- a/common/image-sig.c
+++ b/common/image-sig.c
@@ -3,18 +3,11 @@
  * Copyright (c) 2013, Google Inc.
  */
 
-#ifdef USE_HOSTCC
-#include "mkimage.h"
-#include <fdt_support.h>
-#include <time.h>
-#include <linux/libfdt.h>
-#else
 #include <common.h>
 #include <log.h>
 #include <malloc.h>
 #include <asm/global_data.h>
 DECLARE_GLOBAL_DATA_PTR;
-#endif /* !USE_HOSTCC*/
 #include <image.h>
 #include <u-boot/ecdsa.h>
 #include <u-boot/rsa.h>
@@ -28,9 +21,6 @@ struct checksum_algo checksum_algos[] = {
 		.checksum_len = SHA1_SUM_LEN,
 		.der_len = SHA1_DER_LEN,
 		.der_prefix = sha1_der_prefix,
-#if IMAGE_ENABLE_SIGN
-		.calculate_sign = EVP_sha1,
-#endif
 		.calculate = hash_calculate,
 	},
 	{
@@ -38,9 +28,6 @@ struct checksum_algo checksum_algos[] = {
 		.checksum_len = SHA256_SUM_LEN,
 		.der_len = SHA256_DER_LEN,
 		.der_prefix = sha256_der_prefix,
-#if IMAGE_ENABLE_SIGN
-		.calculate_sign = EVP_sha256,
-#endif
 		.calculate = hash_calculate,
 	},
 #ifdef CONFIG_SHA384
@@ -49,9 +36,6 @@ struct checksum_algo checksum_algos[] = {
 		.checksum_len = SHA384_SUM_LEN,
 		.der_len = SHA384_DER_LEN,
 		.der_prefix = sha384_der_prefix,
-#if IMAGE_ENABLE_SIGN
-		.calculate_sign = EVP_sha384,
-#endif
 		.calculate = hash_calculate,
 	},
 #endif
@@ -61,9 +45,6 @@ struct checksum_algo checksum_algos[] = {
 		.checksum_len = SHA512_SUM_LEN,
 		.der_len = SHA512_DER_LEN,
 		.der_prefix = sha512_der_prefix,
-#if IMAGE_ENABLE_SIGN
-		.calculate_sign = EVP_sha512,
-#endif
 		.calculate = hash_calculate,
 	},
 #endif
@@ -74,24 +55,13 @@ struct crypto_algo crypto_algos[] = {
 	{
 		.name = "rsa2048",
 		.key_len = RSA2048_BYTES,
-		.sign = rsa_sign,
-		.add_verify_data = rsa_add_verify_data,
 		.verify = rsa_verify,
 	},
 	{
 		.name = "rsa4096",
 		.key_len = RSA4096_BYTES,
-		.sign = rsa_sign,
-		.add_verify_data = rsa_add_verify_data,
 		.verify = rsa_verify,
 	},
-	{
-		.name = "ecdsa256",
-		.key_len = ECDSA256_BYTES,
-		.sign = ecdsa_sign,
-		.add_verify_data = ecdsa_add_verify_data,
-		.verify = ecdsa_verify,
-	},
 };
 
 struct padding_algo padding_algos[] = {
@@ -112,16 +82,13 @@ struct checksum_algo *image_get_checksum_algo(const char *full_name)
 	int i;
 	const char *name;
 
-#if !defined(USE_HOSTCC) && defined(CONFIG_NEEDS_MANUAL_RELOC)
+#if defined(CONFIG_NEEDS_MANUAL_RELOC)
 	static bool done;
 
 	if (!done) {
 		done = true;
 		for (i = 0; i < ARRAY_SIZE(checksum_algos); i++) {
 			checksum_algos[i].name += gd->reloc_off;
-#if IMAGE_ENABLE_SIGN
-			checksum_algos[i].calculate_sign += gd->reloc_off;
-#endif
 			checksum_algos[i].calculate += gd->reloc_off;
 		}
 	}
@@ -143,15 +110,13 @@ struct crypto_algo *image_get_crypto_algo(const char *full_name)
 	int i;
 	const char *name;
 
-#if !defined(USE_HOSTCC) && defined(CONFIG_NEEDS_MANUAL_RELOC)
+#if defined(CONFIG_NEEDS_MANUAL_RELOC)
 	static bool done;
 
 	if (!done) {
 		done = true;
 		for (i = 0; i < ARRAY_SIZE(crypto_algos); i++) {
 			crypto_algos[i].name += gd->reloc_off;
-			crypto_algos[i].sign += gd->reloc_off;
-			crypto_algos[i].add_verify_data += gd->reloc_off;
 			crypto_algos[i].verify += gd->reloc_off;
 		}
 	}
-- 
2.31.1

[PATCH 11/18] image: Add support for placing crypto_algo in linker lists

[Alexandru Gagniuc]

The purpose of this change is to enable crypto algorithms to be placed
in linker lists, rather than be declared as a static array. The goal
is to remove the crypto_algos array in a subsequent patch.

Create a new linker list named "cryptos", and search it when
image_get_crypto_algo() is invoked.

NOTE that adding support for manual relocation of crypto_algos within
linker lists is beyond the scope of this patch.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 common/image-sig.c | 9 +++++++++
 include/image.h    | 5 +++++
 2 files changed, 14 insertions(+)

diff --git a/common/image-sig.c b/common/image-sig.c
index 5e2d171975..81a3b739fe 100644
--- a/common/image-sig.c
+++ b/common/image-sig.c
@@ -107,6 +107,7 @@ struct checksum_algo *image_get_checksum_algo(const char *full_name)
 
 struct crypto_algo *image_get_crypto_algo(const char *full_name)
 {
+	struct crypto_algo *crypto, *end;
 	int i;
 	const char *name;
 
@@ -133,6 +134,14 @@ struct crypto_algo *image_get_crypto_algo(const char *full_name)
 			return &crypto_algos[i];
 	}
 
+	crypto = ll_entry_start(struct crypto_algo, cryptos);
+	end = ll_entry_end(struct crypto_algo, cryptos);
+	for (; crypto < end; crypto++) {
+		if (!strcmp(crypto->name, name))
+			return crypto;
+	}
+
+	/* Not found */
 	return NULL;
 }
 
diff --git a/include/image.h b/include/image.h
index dbb24993f1..f7f8f8a029 100644
--- a/include/image.h
+++ b/include/image.h
@@ -47,6 +47,7 @@ struct fdt_region;
 #include <lmb.h>
 #include <asm/u-boot.h>
 #include <command.h>
+#include <linker_lists.h>
 
 /* Take notice of the 'ignore' property for hashes */
 #define IMAGE_ENABLE_IGNORE	1
@@ -1328,6 +1329,10 @@ struct crypto_algo {
 		      uint8_t *sig, uint sig_len);
 };
 
+/* Declare a new U-Boot crypto algorithm handler */
+#define U_BOOT_CRYPTO_ALGO(__name)						\
+ll_entry_declare(struct crypto_algo, __name, cryptos)
+
 struct padding_algo {
 	const char *name;
 	int (*verify)(struct image_sign_info *info,
-- 
2.31.1

[PATCH 12/18] image: rsa: Move verification algorithm to a linker list

[Alexandru Gagniuc]

Move the RSA verification crytpo_algo structure out of the
crypto_algos array, and into a linker list.

Although it appears we are adding an #ifdef to rsa-verify.c, the gains
outweigh this small inconvenience. This is because rsa_verify() is
defined differently based on #ifdefs. This change allows us to have
a single definition of rsa_verify().

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 common/image-sig.c   |  9 ---------
 lib/rsa/rsa-verify.c | 16 ++++++++++++++++
 2 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/common/image-sig.c b/common/image-sig.c
index 81a3b739fe..d996b7ba50 100644
--- a/common/image-sig.c
+++ b/common/image-sig.c
@@ -53,15 +53,6 @@ struct checksum_algo checksum_algos[] = {
 
 struct crypto_algo crypto_algos[] = {
 	{
-		.name = "rsa2048",
-		.key_len = RSA2048_BYTES,
-		.verify = rsa_verify,
-	},
-	{
-		.name = "rsa4096",
-		.key_len = RSA4096_BYTES,
-		.verify = rsa_verify,
-	},
 };
 
 struct padding_algo padding_algos[] = {
diff --git a/lib/rsa/rsa-verify.c b/lib/rsa/rsa-verify.c
index 1998c773fc..bb8cc61d94 100644
--- a/lib/rsa/rsa-verify.c
+++ b/lib/rsa/rsa-verify.c
@@ -571,3 +571,19 @@ int rsa_verify(struct image_sign_info *info,
 
 	return rsa_verify_hash(info, hash, sig, sig_len);
 }
+
+#ifndef USE_HOSTCC
+
+U_BOOT_CRYPTO_ALGO(rsa2048) = {
+	.name = "rsa2048",
+	.key_len = RSA2048_BYTES,
+	.verify = rsa_verify,
+};
+
+U_BOOT_CRYPTO_ALGO(rsa4096) = {
+	.name = "rsa4096",
+	.key_len = RSA4096_BYTES,
+	.verify = rsa_verify,
+};
+
+#endif
-- 
2.31.1

[PATCH 13/18] image: image-sig.c: Remove crypto_algos array

[Alexandru Gagniuc]

Crytographic algorithms (currently RSA), are stored in linker lists.
The crypto_algos array is unused, so remove it, and any logic
associated with it.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 common/image-sig.c | 21 ---------------------
 1 file changed, 21 deletions(-)

diff --git a/common/image-sig.c b/common/image-sig.c
index d996b7ba50..498cd78af4 100644
--- a/common/image-sig.c
+++ b/common/image-sig.c
@@ -51,10 +51,6 @@ struct checksum_algo checksum_algos[] = {
 
 };
 
-struct crypto_algo crypto_algos[] = {
-	{
-};
-
 struct padding_algo padding_algos[] = {
 	{
 		.name = "pkcs-1.5",
@@ -102,29 +98,12 @@ struct crypto_algo *image_get_crypto_algo(const char *full_name)
 	int i;
 	const char *name;
 
-#if defined(CONFIG_NEEDS_MANUAL_RELOC)
-	static bool done;
-
-	if (!done) {
-		done = true;
-		for (i = 0; i < ARRAY_SIZE(crypto_algos); i++) {
-			crypto_algos[i].name += gd->reloc_off;
-			crypto_algos[i].verify += gd->reloc_off;
-		}
-	}
-#endif
-
 	/* Move name to after the comma */
 	name = strchr(full_name, ',');
 	if (!name)
 		return NULL;
 	name += 1;
 
-	for (i = 0; i < ARRAY_SIZE(crypto_algos); i++) {
-		if (!strcmp(crypto_algos[i].name, name))
-			return &crypto_algos[i];
-	}
-
 	crypto = ll_entry_start(struct crypto_algo, cryptos);
 	end = ll_entry_end(struct crypto_algo, cryptos);
 	for (; crypto < end; crypto++) {
-- 
2.31.1

[PATCH 14/18] lib: ecdsa: Remove #ifdefs from ecdsa.h

[Alexandru Gagniuc]

It is no longer necessary to implement ecdsa_() functions as no-ops
depending on config options. It is merely sufficient to provide the
prototypes, as the ecdsa code is no longer linked when unused.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 include/u-boot/ecdsa.h | 25 -------------------------
 1 file changed, 25 deletions(-)

diff --git a/include/u-boot/ecdsa.h b/include/u-boot/ecdsa.h
index 979690d966..f6951c7346 100644
--- a/include/u-boot/ecdsa.h
+++ b/include/u-boot/ecdsa.h
@@ -15,7 +15,6 @@
  * @see "struct crypto_algo"
  * @{
  */
-#if IMAGE_ENABLE_SIGN
 /**
  * sign() - calculate and return signature for given input data
  *
@@ -49,22 +48,7 @@ int ecdsa_sign(struct image_sign_info *info, const struct image_region region[],
  * other -ve value on error
  */
 int ecdsa_add_verify_data(struct image_sign_info *info, void *keydest);
-#else
-static inline
-int ecdsa_sign(struct image_sign_info *info, const struct image_region region[],
-	       int region_count, uint8_t **sigp, uint *sig_len)
-{
-	return -ENXIO;
-}
-
-static inline
-int ecdsa_add_verify_data(struct image_sign_info *info, void *keydest)
-{
-	return -ENXIO;
-}
-#endif
 
-#if IMAGE_ENABLE_VERIFY_ECDSA
 /**
  * verify() - Verify a signature against some data
  *
@@ -78,15 +62,6 @@ int ecdsa_add_verify_data(struct image_sign_info *info, void *keydest)
 int ecdsa_verify(struct image_sign_info *info,
 		 const struct image_region region[], int region_count,
 		 uint8_t *sig, uint sig_len);
-#else
-static inline
-int ecdsa_verify(struct image_sign_info *info,
-		 const struct image_region region[], int region_count,
-		 uint8_t *sig, uint sig_len)
-{
-	return -ENXIO;
-}
-#endif
 /** @} */
 
 #define ECDSA256_BYTES	(256 / 8)
-- 
2.31.1

[PATCH 15/18] lib: rsa: Remove #ifdefs from rsa.h

[Alexandru Gagniuc]

It is no longer necessary to implement rsa_() functions as no-ops
depending on config options. It is merely sufficient to provide the
prototypes, as the rsa code is no longer linked when unused.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 include/u-boot/rsa.h | 47 --------------------------------------------
 1 file changed, 47 deletions(-)

diff --git a/include/u-boot/rsa.h b/include/u-boot/rsa.h
index bc564d56fa..89a9c4caa0 100644
--- a/include/u-boot/rsa.h
+++ b/include/u-boot/rsa.h
@@ -31,7 +31,6 @@ struct rsa_public_key {
 
 struct image_sign_info;
 
-#if IMAGE_ENABLE_SIGN
 /**
  * sign() - calculate and return signature for given input data
  *
@@ -66,22 +65,7 @@ int rsa_sign(struct image_sign_info *info,
 		other -ve value on error
 */
 int rsa_add_verify_data(struct image_sign_info *info, void *keydest);
-#else
-static inline int rsa_sign(struct image_sign_info *info,
-		const struct image_region region[], int region_count,
-		uint8_t **sigp, uint *sig_len)
-{
-	return -ENXIO;
-}
-
-static inline int rsa_add_verify_data(struct image_sign_info *info,
-				      void *keydest)
-{
-	return -ENXIO;
-}
-#endif
 
-#if IMAGE_ENABLE_VERIFY
 /**
  * rsa_verify_hash() - Verify a signature against a hash
  *
@@ -124,37 +108,6 @@ int padding_pss_verify(struct image_sign_info *info,
 		       uint8_t *msg, int msg_len,
 		       const uint8_t *hash, int hash_len);
 #endif /* CONFIG_FIT_RSASSA_PSS */
-#else
-static inline int rsa_verify_hash(struct image_sign_info *info,
-				  const uint8_t *hash,
-				  uint8_t *sig, uint sig_len)
-{
-	return -ENXIO;
-}
-
-static inline int rsa_verify(struct image_sign_info *info,
-		const struct image_region region[], int region_count,
-		uint8_t *sig, uint sig_len)
-{
-	return -ENXIO;
-}
-
-static inline int padding_pkcs_15_verify(struct image_sign_info *info,
-					 uint8_t *msg, int msg_len,
-					 const uint8_t *hash, int hash_len)
-{
-	return -ENXIO;
-}
-
-#ifdef CONFIG_FIT_RSASSA_PSS
-static inline int padding_pss_verify(struct image_sign_info *info,
-				     uint8_t *msg, int msg_len,
-				     const uint8_t *hash, int hash_len)
-{
-	return -ENXIO;
-}
-#endif /* CONFIG_FIT_RSASSA_PSS */
-#endif
 
 #define RSA_DEFAULT_PADDING_NAME		"pkcs-1.5"
 
-- 
2.31.1

[PATCH 16/18] image: Eliminate IMAGE_ENABLE_VERIFY macro

[Alexandru Gagniuc]

This macro is no longer needed for code flow or #ifdefs. Remove it.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 include/image.h | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/include/image.h b/include/image.h
index f7f8f8a029..ee930b0265 100644
--- a/include/image.h
+++ b/include/image.h
@@ -1196,19 +1196,16 @@ int calculate_hash(const void *data, int data_len, const char *algo,
 #if defined(USE_HOSTCC)
 # if defined(CONFIG_FIT_SIGNATURE)
 #  define IMAGE_ENABLE_SIGN	1
-#  define IMAGE_ENABLE_VERIFY	1
 #  define IMAGE_ENABLE_VERIFY_ECDSA	1
 #  define FIT_IMAGE_ENABLE_VERIFY	1
 #  include <openssl/evp.h>
 # else
 #  define IMAGE_ENABLE_SIGN	0
-#  define IMAGE_ENABLE_VERIFY	0
 # define IMAGE_ENABLE_VERIFY_ECDSA	0
 #  define FIT_IMAGE_ENABLE_VERIFY	0
 # endif
 #else
 # define IMAGE_ENABLE_SIGN	0
-# define IMAGE_ENABLE_VERIFY		CONFIG_IS_ENABLED(RSA_VERIFY)
 # define IMAGE_ENABLE_VERIFY_ECDSA	0
 # define FIT_IMAGE_ENABLE_VERIFY	CONFIG_IS_ENABLED(FIT_SIGNATURE)
 #endif
@@ -1260,7 +1257,7 @@ struct image_region {
 	int size;
 };
 
-#if IMAGE_ENABLE_VERIFY
+#if FIT_IMAGE_ENABLE_VERIFY
 # include <u-boot/hash-checksum.h>
 #endif
 struct checksum_algo {
-- 
2.31.1

[PATCH 17/18] image: Eliminate IMAGE_ENABLE_VERIFY_ECDSA macro

[Alexandru Gagniuc]

This macro is no longer needed for code flow or #ifdefs. Remove it.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 include/image.h | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/include/image.h b/include/image.h
index ee930b0265..2f48a6eecf 100644
--- a/include/image.h
+++ b/include/image.h
@@ -1196,17 +1196,14 @@ int calculate_hash(const void *data, int data_len, const char *algo,
 #if defined(USE_HOSTCC)
 # if defined(CONFIG_FIT_SIGNATURE)
 #  define IMAGE_ENABLE_SIGN	1
-#  define IMAGE_ENABLE_VERIFY_ECDSA	1
 #  define FIT_IMAGE_ENABLE_VERIFY	1
 #  include <openssl/evp.h>
 # else
 #  define IMAGE_ENABLE_SIGN	0
-# define IMAGE_ENABLE_VERIFY_ECDSA	0
 #  define FIT_IMAGE_ENABLE_VERIFY	0
 # endif
 #else
 # define IMAGE_ENABLE_SIGN	0
-# define IMAGE_ENABLE_VERIFY_ECDSA	0
 # define FIT_IMAGE_ENABLE_VERIFY	CONFIG_IS_ENABLED(FIT_SIGNATURE)
 #endif
 
-- 
2.31.1

[PATCH 18/18] [UNTESTED] image: Add support for relocating crypto_algos in linker lists

[Alexandru Gagniuc]

Function pointers from crypto_algos array are relocated, when
NEEDS_MANUAL_RELOC is set. This relocation doesn't happen if the algo
is placed in a linker list. Implement this relocation.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 common/image-sig.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/common/image-sig.c b/common/image-sig.c
index 498cd78af4..5c7ddd984d 100644
--- a/common/image-sig.c
+++ b/common/image-sig.c
@@ -98,6 +98,19 @@ struct crypto_algo *image_get_crypto_algo(const char *full_name)
 	int i;
 	const char *name;
 
+#if defined(CONFIG_NEEDS_MANUAL_RELOC)
+	static bool done;
+
+	if (!done) {
+		crypto = ll_entry_start(struct crypto_algo, cryptos);
+		end = ll_entry_end(struct crypto_algo, cryptos);
+		for (; crypto < end; crypto++) {
+			crypto->name += gd->reloc_off;
+			crypto->verify += gd->reloc_off;
+		}
+	}
+#endif
+
 	/* Move name to after the comma */
 	name = strchr(full_name, ',');
 	if (!name)
-- 
2.31.1

[PATCH 09/18] common: Move host-only logic in image-sig.c to separate file

[Alex G.]

On 5/17/21 11:38 AM, Alexandru Gagniuc wrote:
> image-sig.c is used to map a hash or crypto algorithm name to a
> handler of that algorithm. There is some similarity between the host
> and target variants, with the differences worked out by #ifdefs. The
> purpose of this change is to remove those ifdefs.
> 
> First, copy the file to a host-only version, and remove target
> specific code. Although it looks like we are duplicating code,
> subsequent patches will change the way target algorithms are searched.
> Besides we are only duplicating three string to struct mapping
> functions. This isn't something to fuss about.
> 
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> ---
>   tools/Makefile         |   5 +-
>   tools/image-sig-host.c | 133 +++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 136 insertions(+), 2 deletions(-)
>   create mode 100644 tools/image-sig-host.c
> 
> diff --git a/tools/Makefile b/tools/Makefile
> index d020c55d66..e39006b6f6 100644
> --- a/tools/Makefile
> +++ b/tools/Makefile
> @@ -57,8 +57,9 @@ hostprogs-$(CONFIG_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_FIT) := image-sig-host.o fit_common.o fit_image.o \
> +			image-host.o common/image-fit.o
> +FIT_SIG_OBJS-$(CONFIG_FIT_SIGNATURE) := common/image-fit-sig.o
>   FIT_CIPHER_OBJS-$(CONFIG_FIT_CIPHER) := common/image-cipher.o

This may cause a build failure with FIT_SIGNATURE disabled. I will have 
this fixed in v2. The correction is trivial.

Correct diff below for reference:

  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_SIG_OBJS-$(CONFIG_FIT_SIGNATURE) := image-sig-host.o 
common/image-fit-sig.o

[PATCH 06/18] image: Drop IMAGE_ENABLE_SHA1

[Simon Glass]

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.

>                 sha1_csum_wd((unsigned char *)data, data_len,
>                              (unsigned char *)value, CHUNKSZ_SHA1);
>                 *value_len = 20;
> diff --git a/include/image.h b/include/image.h
> index 887a3270bd..8c718adba0 100644
> --- a/include/image.h
> +++ b/include/image.h
> @@ -68,13 +68,9 @@ struct fdt_region;
>  #  ifdef CONFIG_SPL_MD5
>  #   define IMAGE_ENABLE_MD5    1
>  #  endif
> -#  ifdef CONFIG_SPL_FIT_SHA1
> -#   define IMAGE_ENABLE_SHA1   1
> -#  endif
>  # else
>  #  define IMAGE_ENABLE_CRC32   1
>  #  define IMAGE_ENABLE_MD5     1
> -#  define IMAGE_ENABLE_SHA1    1
>  # endif
>
>  #ifndef IMAGE_ENABLE_CRC32
> @@ -85,10 +81,6 @@ struct fdt_region;
>  #define IMAGE_ENABLE_MD5       0
>  #endif
>
> -#ifndef IMAGE_ENABLE_SHA1
> -#define IMAGE_ENABLE_SHA1      0
> -#endif
> -
>  #if defined(CONFIG_FIT_SHA256) || \
>         defined(CONFIG_SPL_FIT_SHA256)
>  #define IMAGE_ENABLE_SHA256    1
> --
> 2.31.1
>

Regards,
Simon

[PATCH 09/18] common: Move host-only logic in image-sig.c to separate file

[Simon Glass]

On Mon, 17 May 2021 at 13:47, Alex G. <mr.nuke.me@gmail.com> wrote:
>
>
>
> On 5/17/21 11:38 AM, Alexandru Gagniuc wrote:
> > image-sig.c is used to map a hash or crypto algorithm name to a
> > handler of that algorithm. There is some similarity between the host
> > and target variants, with the differences worked out by #ifdefs. The
> > purpose of this change is to remove those ifdefs.
> >
> > First, copy the file to a host-only version, and remove target
> > specific code. Although it looks like we are duplicating code,
> > subsequent patches will change the way target algorithms are searched.
> > Besides we are only duplicating three string to struct mapping
> > functions. This isn't something to fuss about.
> >
> > Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> > ---
> >   tools/Makefile         |   5 +-
> >   tools/image-sig-host.c | 133 +++++++++++++++++++++++++++++++++++++++++
> >   2 files changed, 136 insertions(+), 2 deletions(-)
> >   create mode 100644 tools/image-sig-host.c
> >
> > diff --git a/tools/Makefile b/tools/Makefile
> > index d020c55d66..e39006b6f6 100644
> > --- a/tools/Makefile
> > +++ b/tools/Makefile
> > @@ -57,8 +57,9 @@ hostprogs-$(CONFIG_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_FIT) := image-sig-host.o fit_common.o fit_image.o \
> > +                     image-host.o common/image-fit.o
> > +FIT_SIG_OBJS-$(CONFIG_FIT_SIGNATURE) := common/image-fit-sig.o
> >   FIT_CIPHER_OBJS-$(CONFIG_FIT_CIPHER) := common/image-cipher.o
>
> This may cause a build failure with FIT_SIGNATURE disabled. I will have
> this fixed in v2. The correction is trivial.

I see a build warning for an unused variable 'i', if that is what you mean.

>
> Correct diff below for reference:
>
>   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_SIG_OBJS-$(CONFIG_FIT_SIGNATURE) := image-sig-host.o
> common/image-fit-sig.o
>

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH 06/18] image: Drop IMAGE_ENABLE_SHA1

[Alex G]

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.

Alex

>>                  sha1_csum_wd((unsigned char *)data, data_len,
>>                               (unsigned char *)value, CHUNKSZ_SHA1);
>>                  *value_len = 20;
>> diff --git a/include/image.h b/include/image.h
>> index 887a3270bd..8c718adba0 100644
>> --- a/include/image.h
>> +++ b/include/image.h
>> @@ -68,13 +68,9 @@ struct fdt_region;
>>   #  ifdef CONFIG_SPL_MD5
>>   #   define IMAGE_ENABLE_MD5    1
>>   #  endif
>> -#  ifdef CONFIG_SPL_FIT_SHA1
>> -#   define IMAGE_ENABLE_SHA1   1
>> -#  endif
>>   # else
>>   #  define IMAGE_ENABLE_CRC32   1
>>   #  define IMAGE_ENABLE_MD5     1
>> -#  define IMAGE_ENABLE_SHA1    1
>>   # endif
>>
>>   #ifndef IMAGE_ENABLE_CRC32
>> @@ -85,10 +81,6 @@ struct fdt_region;
>>   #define IMAGE_ENABLE_MD5       0
>>   #endif
>>
>> -#ifndef IMAGE_ENABLE_SHA1
>> -#define IMAGE_ENABLE_SHA1      0
>> -#endif
>> -
>>   #if defined(CONFIG_FIT_SHA256) || \
>>          defined(CONFIG_SPL_FIT_SHA256)
>>   #define IMAGE_ENABLE_SHA256    1
>> --
>> 2.31.1
>>
> 
> Regards,
> Simon
> 

[PATCH 06/18] image: Drop IMAGE_ENABLE_SHA1

[Simon Glass]

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.

[..]

Regards,
Simon

[PATCH 06/18] image: Drop IMAGE_ENABLE_SHA1

[Alex G.]

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.

Alex

[PATCH 06/18] image: Drop IMAGE_ENABLE_SHA1

[Simon Glass]

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.

Regards,
Simon

[PATCH 06/18] image: Drop IMAGE_ENABLE_SHA1

[Alex G.]

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()

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.

Alex

[PATCH 06/18] image: Drop IMAGE_ENABLE_SHA1

[Simon Glass]

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.

>
> 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?

Regards,
Simon

[PATCH 06/18] image: Drop IMAGE_ENABLE_SHA1

[Alex G.]

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?

>>
>> 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.

Alex

Re: [PATCH 06/18] image: Drop IMAGE_ENABLE_SHA1

[Simon Glass]

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).

Tom, do you have any thoughts?

Regards,
SImon

Re: [PATCH 06/18] image: Drop IMAGE_ENABLE_SHA1

[Alex G.]

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?

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.

Re: [PATCH 06/18] image: Drop IMAGE_ENABLE_SHA1

[Simon Glass]

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