* [PATCH 01/18] image: Shorten FIT_ENABLE_SHAxxx_SUPPORT
2021-05-17 16:38 [PATCH 00/18] image: Reduce #ifdef abuse in image code Alexandru Gagniuc
@ 2021-05-17 16:38 ` Alexandru Gagniuc
2021-05-17 16:38 ` [PATCH 02/18] image: Rename SPL_SHAxxx_SUPPORT to SPL_FIT_SHAxxx Alexandru Gagniuc
` (16 subsequent siblings)
17 siblings, 0 replies; 32+ messages in thread
From: Alexandru Gagniuc @ 2021-05-17 16:38 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 02/18] image: Rename SPL_SHAxxx_SUPPORT to SPL_FIT_SHAxxx
2021-05-17 16:38 [PATCH 00/18] image: Reduce #ifdef abuse in image code Alexandru Gagniuc
2021-05-17 16:38 ` [PATCH 01/18] image: Shorten FIT_ENABLE_SHAxxx_SUPPORT Alexandru Gagniuc
@ 2021-05-17 16:38 ` Alexandru Gagniuc
2021-05-17 16:38 ` [PATCH 03/18] image: Rename CONFIG_FIT_ENABLE_RSASSA_PSS_SUPPORT Alexandru Gagniuc
` (15 subsequent siblings)
17 siblings, 0 replies; 32+ messages in thread
From: Alexandru Gagniuc @ 2021-05-17 16:38 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 03/18] image: Rename CONFIG_FIT_ENABLE_RSASSA_PSS_SUPPORT
2021-05-17 16:38 [PATCH 00/18] image: Reduce #ifdef abuse in image code Alexandru Gagniuc
2021-05-17 16:38 ` [PATCH 01/18] image: Shorten FIT_ENABLE_SHAxxx_SUPPORT Alexandru Gagniuc
2021-05-17 16:38 ` [PATCH 02/18] image: Rename SPL_SHAxxx_SUPPORT to SPL_FIT_SHAxxx Alexandru Gagniuc
@ 2021-05-17 16:38 ` Alexandru Gagniuc
2021-05-17 16:38 ` [PATCH 04/18] Kconfig: Rename SPL_CRC32_SUPPORT to SPL_CRC32 Alexandru Gagniuc
` (14 subsequent siblings)
17 siblings, 0 replies; 32+ messages in thread
From: Alexandru Gagniuc @ 2021-05-17 16:38 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 04/18] Kconfig: Rename SPL_CRC32_SUPPORT to SPL_CRC32
2021-05-17 16:38 [PATCH 00/18] image: Reduce #ifdef abuse in image code Alexandru Gagniuc
` (2 preceding siblings ...)
2021-05-17 16:38 ` [PATCH 03/18] image: Rename CONFIG_FIT_ENABLE_RSASSA_PSS_SUPPORT Alexandru Gagniuc
@ 2021-05-17 16:38 ` Alexandru Gagniuc
2021-05-17 16:38 ` [PATCH 05/18] Kconfig: Rename SPL_MD5_SUPPORT to SPL_MD5 Alexandru Gagniuc
` (13 subsequent siblings)
17 siblings, 0 replies; 32+ messages in thread
From: Alexandru Gagniuc @ 2021-05-17 16:38 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 05/18] Kconfig: Rename SPL_MD5_SUPPORT to SPL_MD5
2021-05-17 16:38 [PATCH 00/18] image: Reduce #ifdef abuse in image code Alexandru Gagniuc
` (3 preceding siblings ...)
2021-05-17 16:38 ` [PATCH 04/18] Kconfig: Rename SPL_CRC32_SUPPORT to SPL_CRC32 Alexandru Gagniuc
@ 2021-05-17 16:38 ` Alexandru Gagniuc
2021-05-17 16:38 ` [PATCH 06/18] image: Drop IMAGE_ENABLE_SHA1 Alexandru Gagniuc
` (12 subsequent siblings)
17 siblings, 0 replies; 32+ messages in thread
From: Alexandru Gagniuc @ 2021-05-17 16:38 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 06/18] image: Drop IMAGE_ENABLE_SHA1
2021-05-17 16:38 [PATCH 00/18] image: Reduce #ifdef abuse in image code Alexandru Gagniuc
` (4 preceding siblings ...)
2021-05-17 16:38 ` [PATCH 05/18] Kconfig: Rename SPL_MD5_SUPPORT to SPL_MD5 Alexandru Gagniuc
@ 2021-05-17 16:38 ` Alexandru Gagniuc
2021-05-19 16:36 ` Simon Glass
2021-05-17 16:38 ` [PATCH 07/18] image: Drop IMAGE_ENABLE_SHAxxx Alexandru Gagniuc
` (11 subsequent siblings)
17 siblings, 1 reply; 32+ messages in thread
From: Alexandru Gagniuc @ 2021-05-17 16:38 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 06/18] image: Drop IMAGE_ENABLE_SHA1
2021-05-17 16:38 ` [PATCH 06/18] image: Drop IMAGE_ENABLE_SHA1 Alexandru Gagniuc
@ 2021-05-19 16:36 ` Simon Glass
2021-05-19 17:44 ` Alex G
0 siblings, 1 reply; 32+ messages in thread
From: Simon Glass @ 2021-05-19 16:36 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 32+ messages in thread* [PATCH 06/18] image: Drop IMAGE_ENABLE_SHA1
2021-05-19 16:36 ` Simon Glass
@ 2021-05-19 17:44 ` Alex G
2021-05-19 21:55 ` Simon Glass
0 siblings, 1 reply; 32+ messages in thread
From: Alex G @ 2021-05-19 17:44 UTC (permalink / raw)
To: u-boot
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
>
^ permalink raw reply [flat|nested] 32+ messages in thread* [PATCH 06/18] image: Drop IMAGE_ENABLE_SHA1
2021-05-19 17:44 ` Alex G
@ 2021-05-19 21:55 ` Simon Glass
2021-05-20 2:41 ` Alex G.
0 siblings, 1 reply; 32+ messages in thread
From: Simon Glass @ 2021-05-19 21:55 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 32+ messages in thread* [PATCH 06/18] image: Drop IMAGE_ENABLE_SHA1
2021-05-19 21:55 ` Simon Glass
@ 2021-05-20 2:41 ` Alex G.
2021-05-20 17:52 ` Simon Glass
0 siblings, 1 reply; 32+ messages in thread
From: Alex G. @ 2021-05-20 2:41 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 32+ messages in thread* [PATCH 06/18] image: Drop IMAGE_ENABLE_SHA1
2021-05-20 2:41 ` Alex G.
@ 2021-05-20 17:52 ` Simon Glass
2021-05-20 23:13 ` Alex G.
0 siblings, 1 reply; 32+ messages in thread
From: Simon Glass @ 2021-05-20 17:52 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 32+ messages in thread* [PATCH 06/18] image: Drop IMAGE_ENABLE_SHA1
2021-05-20 17:52 ` Simon Glass
@ 2021-05-20 23:13 ` Alex G.
2021-05-20 23:17 ` Simon Glass
0 siblings, 1 reply; 32+ messages in thread
From: Alex G. @ 2021-05-20 23:13 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 32+ messages in thread* [PATCH 06/18] image: Drop IMAGE_ENABLE_SHA1
2021-05-20 23:13 ` Alex G.
@ 2021-05-20 23:17 ` Simon Glass
2021-05-21 0:07 ` Alex G.
0 siblings, 1 reply; 32+ messages in thread
From: Simon Glass @ 2021-05-20 23:17 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 32+ messages in thread* [PATCH 06/18] image: Drop IMAGE_ENABLE_SHA1
2021-05-20 23:17 ` Simon Glass
@ 2021-05-21 0:07 ` Alex G.
2021-05-21 19:39 ` Simon Glass
0 siblings, 1 reply; 32+ messages in thread
From: Alex G. @ 2021-05-21 0:07 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 06/18] image: Drop IMAGE_ENABLE_SHA1
2021-05-21 0:07 ` Alex G.
@ 2021-05-21 19:39 ` Simon Glass
2021-05-24 19:59 ` Alex G.
0 siblings, 1 reply; 32+ messages in thread
From: Simon Glass @ 2021-05-21 19:39 UTC (permalink / raw)
To: Alex G.; +Cc: U-Boot Mailing List, Tom Rini, Rasmus Villemoes
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
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 06/18] image: Drop IMAGE_ENABLE_SHA1
2021-05-21 19:39 ` Simon Glass
@ 2021-05-24 19:59 ` Alex G.
2021-06-22 13:31 ` Simon Glass
0 siblings, 1 reply; 32+ messages in thread
From: Alex G. @ 2021-05-24 19:59 UTC (permalink / raw)
To: Simon Glass; +Cc: U-Boot Mailing List, Tom Rini, Rasmus Villemoes
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.
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 06/18] image: Drop IMAGE_ENABLE_SHA1
2021-05-24 19:59 ` Alex G.
@ 2021-06-22 13:31 ` Simon Glass
0 siblings, 0 replies; 32+ messages in thread
From: Simon Glass @ 2021-06-22 13:31 UTC (permalink / raw)
To: Alex G.; +Cc: U-Boot Mailing List, Tom Rini, Rasmus Villemoes
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 07/18] image: Drop IMAGE_ENABLE_SHAxxx
2021-05-17 16:38 [PATCH 00/18] image: Reduce #ifdef abuse in image code Alexandru Gagniuc
` (5 preceding siblings ...)
2021-05-17 16:38 ` [PATCH 06/18] image: Drop IMAGE_ENABLE_SHA1 Alexandru Gagniuc
@ 2021-05-17 16:38 ` Alexandru Gagniuc
2021-05-17 16:38 ` [PATCH 08/18] image: Drop IMAGE_ENABLE_BEST_MATCH Alexandru Gagniuc
` (10 subsequent siblings)
17 siblings, 0 replies; 32+ messages in thread
From: Alexandru Gagniuc @ 2021-05-17 16:38 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 08/18] image: Drop IMAGE_ENABLE_BEST_MATCH
2021-05-17 16:38 [PATCH 00/18] image: Reduce #ifdef abuse in image code Alexandru Gagniuc
` (6 preceding siblings ...)
2021-05-17 16:38 ` [PATCH 07/18] image: Drop IMAGE_ENABLE_SHAxxx Alexandru Gagniuc
@ 2021-05-17 16:38 ` Alexandru Gagniuc
2021-05-17 16:38 ` [PATCH 09/18] common: Move host-only logic in image-sig.c to separate file Alexandru Gagniuc
` (9 subsequent siblings)
17 siblings, 0 replies; 32+ messages in thread
From: Alexandru Gagniuc @ 2021-05-17 16:38 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 09/18] common: Move host-only logic in image-sig.c to separate file
2021-05-17 16:38 [PATCH 00/18] image: Reduce #ifdef abuse in image code Alexandru Gagniuc
` (7 preceding siblings ...)
2021-05-17 16:38 ` [PATCH 08/18] image: Drop IMAGE_ENABLE_BEST_MATCH Alexandru Gagniuc
@ 2021-05-17 16:38 ` Alexandru Gagniuc
2021-05-17 19:47 ` Alex G.
2021-05-17 16:38 ` [PATCH 10/18] common: image-sig.c: Remove host-specific logic and #ifdefs Alexandru Gagniuc
` (8 subsequent siblings)
17 siblings, 1 reply; 32+ messages in thread
From: Alexandru Gagniuc @ 2021-05-17 16:38 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 09/18] common: Move host-only logic in image-sig.c to separate file
2021-05-17 16:38 ` [PATCH 09/18] common: Move host-only logic in image-sig.c to separate file Alexandru Gagniuc
@ 2021-05-17 19:47 ` Alex G.
2021-05-19 16:36 ` Simon Glass
0 siblings, 1 reply; 32+ messages in thread
From: Alex G. @ 2021-05-17 19:47 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 09/18] common: Move host-only logic in image-sig.c to separate file
2021-05-17 19:47 ` Alex G.
@ 2021-05-19 16:36 ` Simon Glass
0 siblings, 0 replies; 32+ messages in thread
From: Simon Glass @ 2021-05-19 16:36 UTC (permalink / raw)
To: u-boot
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>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 10/18] common: image-sig.c: Remove host-specific logic and #ifdefs
2021-05-17 16:38 [PATCH 00/18] image: Reduce #ifdef abuse in image code Alexandru Gagniuc
` (8 preceding siblings ...)
2021-05-17 16:38 ` [PATCH 09/18] common: Move host-only logic in image-sig.c to separate file Alexandru Gagniuc
@ 2021-05-17 16:38 ` Alexandru Gagniuc
2021-05-17 16:38 ` [PATCH 11/18] image: Add support for placing crypto_algo in linker lists Alexandru Gagniuc
` (7 subsequent siblings)
17 siblings, 0 replies; 32+ messages in thread
From: Alexandru Gagniuc @ 2021-05-17 16:38 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 11/18] image: Add support for placing crypto_algo in linker lists
2021-05-17 16:38 [PATCH 00/18] image: Reduce #ifdef abuse in image code Alexandru Gagniuc
` (9 preceding siblings ...)
2021-05-17 16:38 ` [PATCH 10/18] common: image-sig.c: Remove host-specific logic and #ifdefs Alexandru Gagniuc
@ 2021-05-17 16:38 ` Alexandru Gagniuc
2021-05-17 16:38 ` [PATCH 12/18] image: rsa: Move verification algorithm to a linker list Alexandru Gagniuc
` (6 subsequent siblings)
17 siblings, 0 replies; 32+ messages in thread
From: Alexandru Gagniuc @ 2021-05-17 16:38 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 12/18] image: rsa: Move verification algorithm to a linker list
2021-05-17 16:38 [PATCH 00/18] image: Reduce #ifdef abuse in image code Alexandru Gagniuc
` (10 preceding siblings ...)
2021-05-17 16:38 ` [PATCH 11/18] image: Add support for placing crypto_algo in linker lists Alexandru Gagniuc
@ 2021-05-17 16:38 ` Alexandru Gagniuc
2021-05-17 16:38 ` [PATCH 13/18] image: image-sig.c: Remove crypto_algos array Alexandru Gagniuc
` (5 subsequent siblings)
17 siblings, 0 replies; 32+ messages in thread
From: Alexandru Gagniuc @ 2021-05-17 16:38 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 13/18] image: image-sig.c: Remove crypto_algos array
2021-05-17 16:38 [PATCH 00/18] image: Reduce #ifdef abuse in image code Alexandru Gagniuc
` (11 preceding siblings ...)
2021-05-17 16:38 ` [PATCH 12/18] image: rsa: Move verification algorithm to a linker list Alexandru Gagniuc
@ 2021-05-17 16:38 ` Alexandru Gagniuc
2021-05-17 16:38 ` [PATCH 14/18] lib: ecdsa: Remove #ifdefs from ecdsa.h Alexandru Gagniuc
` (4 subsequent siblings)
17 siblings, 0 replies; 32+ messages in thread
From: Alexandru Gagniuc @ 2021-05-17 16:38 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 14/18] lib: ecdsa: Remove #ifdefs from ecdsa.h
2021-05-17 16:38 [PATCH 00/18] image: Reduce #ifdef abuse in image code Alexandru Gagniuc
` (12 preceding siblings ...)
2021-05-17 16:38 ` [PATCH 13/18] image: image-sig.c: Remove crypto_algos array Alexandru Gagniuc
@ 2021-05-17 16:38 ` Alexandru Gagniuc
2021-05-17 16:38 ` [PATCH 15/18] lib: rsa: Remove #ifdefs from rsa.h Alexandru Gagniuc
` (3 subsequent siblings)
17 siblings, 0 replies; 32+ messages in thread
From: Alexandru Gagniuc @ 2021-05-17 16:38 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 15/18] lib: rsa: Remove #ifdefs from rsa.h
2021-05-17 16:38 [PATCH 00/18] image: Reduce #ifdef abuse in image code Alexandru Gagniuc
` (13 preceding siblings ...)
2021-05-17 16:38 ` [PATCH 14/18] lib: ecdsa: Remove #ifdefs from ecdsa.h Alexandru Gagniuc
@ 2021-05-17 16:38 ` Alexandru Gagniuc
2021-05-17 16:38 ` [PATCH 16/18] image: Eliminate IMAGE_ENABLE_VERIFY macro Alexandru Gagniuc
` (2 subsequent siblings)
17 siblings, 0 replies; 32+ messages in thread
From: Alexandru Gagniuc @ 2021-05-17 16:38 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 16/18] image: Eliminate IMAGE_ENABLE_VERIFY macro
2021-05-17 16:38 [PATCH 00/18] image: Reduce #ifdef abuse in image code Alexandru Gagniuc
` (14 preceding siblings ...)
2021-05-17 16:38 ` [PATCH 15/18] lib: rsa: Remove #ifdefs from rsa.h Alexandru Gagniuc
@ 2021-05-17 16:38 ` Alexandru Gagniuc
2021-05-17 16:38 ` [PATCH 17/18] image: Eliminate IMAGE_ENABLE_VERIFY_ECDSA macro Alexandru Gagniuc
2021-05-17 16:38 ` [PATCH 18/18] [UNTESTED] image: Add support for relocating crypto_algos in linker lists Alexandru Gagniuc
17 siblings, 0 replies; 32+ messages in thread
From: Alexandru Gagniuc @ 2021-05-17 16:38 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 17/18] image: Eliminate IMAGE_ENABLE_VERIFY_ECDSA macro
2021-05-17 16:38 [PATCH 00/18] image: Reduce #ifdef abuse in image code Alexandru Gagniuc
` (15 preceding siblings ...)
2021-05-17 16:38 ` [PATCH 16/18] image: Eliminate IMAGE_ENABLE_VERIFY macro Alexandru Gagniuc
@ 2021-05-17 16:38 ` Alexandru Gagniuc
2021-05-17 16:38 ` [PATCH 18/18] [UNTESTED] image: Add support for relocating crypto_algos in linker lists Alexandru Gagniuc
17 siblings, 0 replies; 32+ messages in thread
From: Alexandru Gagniuc @ 2021-05-17 16:38 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 18/18] [UNTESTED] image: Add support for relocating crypto_algos in linker lists
2021-05-17 16:38 [PATCH 00/18] image: Reduce #ifdef abuse in image code Alexandru Gagniuc
` (16 preceding siblings ...)
2021-05-17 16:38 ` [PATCH 17/18] image: Eliminate IMAGE_ENABLE_VERIFY_ECDSA macro Alexandru Gagniuc
@ 2021-05-17 16:38 ` Alexandru Gagniuc
17 siblings, 0 replies; 32+ messages in thread
From: Alexandru Gagniuc @ 2021-05-17 16:38 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 32+ messages in thread