[PATCH 0/4] crypto: Add new UCLASS_HASH

[PATCH 0/4] crypto: Add new UCLASS_HASH

[Chia-Wei Wang]

This patch series proposes new UCLASS_HASH for hash devices.
Thus the hash drivers (SW or HW-accelerated) can be developed
in the DM-based fashion.

A purely software implemented hash driver is also added under
the newly added UCLASS_HASH uclass. In addition, the FIT image
hash verification is also updated to leverage the UCLASS_HASH
driver if configured.

As there is widly spread use of non-DM hash functions (common/hash.c),
this patch does not remove them. More patches are needed if UCLASS_HASH
is established.

Chia-Wei Wang (4):
  lib/md5: Export progressive APIs
  dm: hash: Add new UCLASS_HASH support
  crypto: hash: Add software hash DM driver
  fit: Use DM hash driver if supported

 common/image-fit.c                |  30 +++
 drivers/crypto/Kconfig            |   2 +
 drivers/crypto/Makefile           |   1 +
 drivers/crypto/hash/Kconfig       |  16 ++
 drivers/crypto/hash/Makefile      |   6 +
 drivers/crypto/hash/hash-uclass.c | 121 ++++++++++++
 drivers/crypto/hash/hash_sw.c     | 301 ++++++++++++++++++++++++++++++
 include/dm/uclass-id.h            |   1 +
 include/u-boot/hash.h             |  61 ++++++
 include/u-boot/md5.h              |   4 +
 lib/md5.c                         |   6 +-
 11 files changed, 546 insertions(+), 3 deletions(-)
 create mode 100644 drivers/crypto/hash/Kconfig
 create mode 100644 drivers/crypto/hash/Makefile
 create mode 100644 drivers/crypto/hash/hash-uclass.c
 create mode 100644 drivers/crypto/hash/hash_sw.c
 create mode 100644 include/u-boot/hash.h

-- 
2.17.1

[PATCH 1/4] lib/md5: Export progressive APIs

[Chia-Wei Wang]

Export the MD5 hash init/update/finish progressive APIs
for better flexibility.

Signed-off-by: Chia-Wei Wang <chiawei_wang@aspeedtech.com>
---
 include/u-boot/md5.h | 4 ++++
 lib/md5.c            | 6 +++---
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/include/u-boot/md5.h b/include/u-boot/md5.h
index e09c16a6e3..e5cb923d77 100644
--- a/include/u-boot/md5.h
+++ b/include/u-boot/md5.h
@@ -17,6 +17,10 @@ struct MD5Context {
 	};
 };
 
+void MD5Init(struct MD5Context *ctx);
+void MD5Update(struct MD5Context *ctx, unsigned char const *buf, unsigned len);
+void MD5Final(unsigned char digest[16], struct MD5Context *ctx);
+
 /*
  * Calculate and store in 'output' the MD5 digest of 'len' bytes at
  * 'input'. 'output' must have enough space to hold 16 bytes.
diff --git a/lib/md5.c b/lib/md5.c
index 2ae4a06319..688b7254c6 100644
--- a/lib/md5.c
+++ b/lib/md5.c
@@ -55,7 +55,7 @@ byteReverse(unsigned char *buf, unsigned longs)
  * Start MD5 accumulation.  Set bit count to 0 and buffer to mysterious
  * initialization constants.
  */
-static void
+void
 MD5Init(struct MD5Context *ctx)
 {
 	ctx->buf[0] = 0x67452301;
@@ -71,7 +71,7 @@ MD5Init(struct MD5Context *ctx)
  * Update context to reflect the concatenation of another buffer full
  * of bytes.
  */
-static void
+void
 MD5Update(struct MD5Context *ctx, unsigned char const *buf, unsigned len)
 {
 	register __u32 t;
@@ -120,7 +120,7 @@ MD5Update(struct MD5Context *ctx, unsigned char const *buf, unsigned len)
  * Final wrapup - pad to 64-byte boundary with the bit pattern
  * 1 0* (64-bit count of bits processed, MSB-first)
  */
-static void
+void
 MD5Final(unsigned char digest[16], struct MD5Context *ctx)
 {
 	unsigned int count;
-- 
2.17.1

[PATCH 2/4] dm: hash: Add new UCLASS_HASH support

[Chia-Wei Wang]

Add UCLASS_HASH for hash driver development. Thus the
hash drivers (SW or HW-accelerated) can be developed
in the DM-based fashion.

Signed-off-by: Chia-Wei Wang <chiawei_wang@aspeedtech.com>
---
 drivers/crypto/Kconfig            |   2 +
 drivers/crypto/Makefile           |   1 +
 drivers/crypto/hash/Kconfig       |   5 ++
 drivers/crypto/hash/Makefile      |   5 ++
 drivers/crypto/hash/hash-uclass.c | 121 ++++++++++++++++++++++++++++++
 include/dm/uclass-id.h            |   1 +
 include/u-boot/hash.h             |  61 +++++++++++++++
 7 files changed, 196 insertions(+)
 create mode 100644 drivers/crypto/hash/Kconfig
 create mode 100644 drivers/crypto/hash/Makefile
 create mode 100644 drivers/crypto/hash/hash-uclass.c
 create mode 100644 include/u-boot/hash.h

diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
index 1ea116be75..0082177c21 100644
--- a/drivers/crypto/Kconfig
+++ b/drivers/crypto/Kconfig
@@ -1,5 +1,7 @@
 menu "Hardware crypto devices"
 
+source drivers/crypto/hash/Kconfig
+
 source drivers/crypto/fsl/Kconfig
 
 endmenu
diff --git a/drivers/crypto/Makefile b/drivers/crypto/Makefile
index efbd1d3fca..4a12b56be6 100644
--- a/drivers/crypto/Makefile
+++ b/drivers/crypto/Makefile
@@ -6,3 +6,4 @@
 obj-$(CONFIG_EXYNOS_ACE_SHA)	+= ace_sha.o
 obj-y += rsa_mod_exp/
 obj-y += fsl/
+obj-y += hash/
diff --git a/drivers/crypto/hash/Kconfig b/drivers/crypto/hash/Kconfig
new file mode 100644
index 0000000000..e226144b9b
--- /dev/null
+++ b/drivers/crypto/hash/Kconfig
@@ -0,0 +1,5 @@
+config DM_HASH
+	bool "Enable Driver Model for Hash"
+	depends on DM
+	help
+	  If you want to use driver model for Hash, say Y.
diff --git a/drivers/crypto/hash/Makefile b/drivers/crypto/hash/Makefile
new file mode 100644
index 0000000000..83acf3d47b
--- /dev/null
+++ b/drivers/crypto/hash/Makefile
@@ -0,0 +1,5 @@
+# SPDX-License-Identifier: GPL-2.0+
+#
+# Copyright (c) 2021 ASPEED Technology Inc.
+
+obj-$(CONFIG_DM_HASH) += hash-uclass.o
diff --git a/drivers/crypto/hash/hash-uclass.c b/drivers/crypto/hash/hash-uclass.c
new file mode 100644
index 0000000000..446eb9e56a
--- /dev/null
+++ b/drivers/crypto/hash/hash-uclass.c
@@ -0,0 +1,121 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2021 ASPEED Technology Inc.
+ * Author: ChiaWei Wang <chiawei_wang@aspeedtech.com>
+ */
+
+#define LOG_CATEGORY UCLASS_HASH
+
+#include <common.h>
+#include <dm.h>
+#include <asm/global_data.h>
+#include <u-boot/hash.h>
+#include <errno.h>
+#include <fdtdec.h>
+#include <malloc.h>
+#include <asm/io.h>
+#include <linux/list.h>
+
+struct hash_info {
+	char *name;
+	uint32_t digest_size;
+};
+
+static const struct hash_info hash_info[HASH_ALGO_NUM] = {
+	[HASH_ALGO_CRC16_CCITT] = { "crc16-ccitt", 2 },
+	[HASH_ALGO_CRC32] = { "crc32", 4 },
+	[HASH_ALGO_MD5] = { "md5", 16 },
+	[HASH_ALGO_SHA1] = { "sha1", 20 },
+	[HASH_ALGO_SHA256] = { "sha256", 32 },
+	[HASH_ALGO_SHA384] = { "sha384", 48 },
+	[HASH_ALGO_SHA512] = { "sha512", 64},
+};
+
+enum HASH_ALGO hash_algo_lookup_by_name(const char *name)
+{
+	int i;
+
+	if (!name)
+		return HASH_ALGO_INVALID;
+
+	for (i = 0; i < HASH_ALGO_NUM; ++i)
+		if (!strcmp(name, hash_info[i].name))
+			return i;
+
+	return HASH_ALGO_INVALID;
+}
+
+ssize_t hash_algo_digest_size(enum HASH_ALGO algo)
+{
+	if (algo >= HASH_ALGO_NUM)
+		return -EINVAL;
+
+	return hash_info[algo].digest_size;
+}
+
+const char *hash_algo_name(enum HASH_ALGO algo)
+{
+	if (algo >= HASH_ALGO_NUM)
+		return NULL;
+
+	return hash_info[algo].name;
+}
+
+int hash_digest(struct udevice *dev, enum HASH_ALGO algo,
+		const void *ibuf, const uint32_t ilen,
+		void *obuf)
+{
+	struct hash_ops *ops = (struct hash_ops *)device_get_ops(dev);
+
+	if (!ops->hash_digest)
+		return -ENOSYS;
+
+	return ops->hash_digest(dev, algo, ibuf, ilen, obuf);
+}
+
+int hash_digest_wd(struct udevice *dev, enum HASH_ALGO algo,
+		   const void *ibuf, const uint32_t ilen,
+		   void *obuf, uint32_t chunk_sz)
+{
+	struct hash_ops *ops = (struct hash_ops *)device_get_ops(dev);
+
+	if (!ops->hash_digest_wd)
+		return -ENOSYS;
+
+	return ops->hash_digest_wd(dev, algo, ibuf, ilen, obuf, chunk_sz);
+}
+
+int hash_init(struct udevice *dev, enum HASH_ALGO algo, void **ctxp)
+{
+	struct hash_ops *ops = (struct hash_ops *)device_get_ops(dev);
+
+	if (!ops->hash_init)
+		return -ENOSYS;
+
+	return ops->hash_init(dev, algo, ctxp);
+}
+
+int hash_update(struct udevice *dev, void *ctx, const void *ibuf, const uint32_t ilen)
+{
+	struct hash_ops *ops = (struct hash_ops *)device_get_ops(dev);
+
+	if (!ops->hash_update)
+		return -ENOSYS;
+
+	return ops->hash_update(dev, ctx, ibuf, ilen);
+}
+
+int hash_finish(struct udevice *dev, void *ctx, void *obuf)
+{
+	struct hash_ops *ops = (struct hash_ops *)device_get_ops(dev);
+
+	if (!ops->hash_finish)
+		return -ENOSYS;
+
+	return ops->hash_finish(dev, ctx, obuf);
+}
+
+UCLASS_DRIVER(hash) = {
+	.id	= UCLASS_HASH,
+	.name	= "hash",
+};
diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
index 9d474533ba..a2c05ae99e 100644
--- a/include/dm/uclass-id.h
+++ b/include/dm/uclass-id.h
@@ -53,6 +53,7 @@ enum uclass_id {
 	UCLASS_FIRMWARE,	/* Firmware */
 	UCLASS_FS_FIRMWARE_LOADER,		/* Generic loader */
 	UCLASS_GPIO,		/* Bank of general-purpose I/O pins */
+	UCLASS_HASH,		/* Hash device */
 	UCLASS_HWSPINLOCK,	/* Hardware semaphores */
 	UCLASS_I2C,		/* I2C bus */
 	UCLASS_I2C_EEPROM,	/* I2C EEPROM device */
diff --git a/include/u-boot/hash.h b/include/u-boot/hash.h
new file mode 100644
index 0000000000..f9d47a99a7
--- /dev/null
+++ b/include/u-boot/hash.h
@@ -0,0 +1,61 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright (c) 2021 ASPEED Technology Inc.
+ */
+#ifndef _UBOOT_HASH_H
+#define _UBOOT_HASH_H
+
+enum HASH_ALGO {
+	HASH_ALGO_CRC16_CCITT,
+	HASH_ALGO_CRC32,
+	HASH_ALGO_MD5,
+	HASH_ALGO_SHA1,
+	HASH_ALGO_SHA256,
+	HASH_ALGO_SHA384,
+	HASH_ALGO_SHA512,
+
+	HASH_ALGO_NUM,
+
+	HASH_ALGO_INVALID = 0xffffffff,
+};
+
+/* general APIs for hash algo information */
+enum HASH_ALGO hash_algo_lookup_by_name(const char *name);
+ssize_t hash_algo_digest_size(enum HASH_ALGO algo);
+const char *hash_algo_name(enum HASH_ALGO algo);
+
+/* device-dependent APIs */
+int hash_digest(struct udevice *dev, enum HASH_ALGO algo,
+		const void *ibuf, const uint32_t ilen,
+		void *obuf);
+int hash_digest_wd(struct udevice *dev, enum HASH_ALGO algo,
+		   const void *ibuf, const uint32_t ilen,
+		   void *obuf, uint32_t chunk_sz);
+int hash_init(struct udevice *dev, enum HASH_ALGO algo, void **ctxp);
+int hash_update(struct udevice *dev, void *ctx, const void *ibuf, const uint32_t ilen);
+int hash_finish(struct udevice *dev, void *ctx, void *obuf);
+
+/*
+ * struct hash_ops - Driver model for Hash operations
+ *
+ * The uclass interface is implemented by all hash devices
+ * which use driver model.
+ */
+struct hash_ops {
+	/* progressive operations */
+	int (*hash_init)(struct udevice *dev, enum HASH_ALGO algo, void **ctxp);
+	int (*hash_update)(struct udevice *dev, void *ctx, const void *ibuf, const uint32_t ilen);
+	int (*hash_finish)(struct udevice *dev, void *ctx, void *obuf);
+
+	/* all-in-one operation */
+	int (*hash_digest)(struct udevice *dev, enum HASH_ALGO algo,
+			   const void *ibuf, const uint32_t ilen,
+			   void *obuf);
+
+	/* all-in-one operation with watchdog triggering every chunk_sz */
+	int (*hash_digest_wd)(struct udevice *dev, enum HASH_ALGO algo,
+			      const void *ibuf, const uint32_t ilen,
+			      void *obuf, uint32_t chunk_sz);
+};
+
+#endif
-- 
2.17.1

[PATCH 3/4] crypto: hash: Add software hash DM driver

[Chia-Wei Wang]

Add purely software-implmented drivers to support multiple
hash operations including CRC, MD5, and SHA family.

This driver is based on the new hash uclass.

Signed-off-by: Chia-Wei Wang <chiawei_wang@aspeedtech.com>
---
 drivers/crypto/hash/Kconfig   |  11 ++
 drivers/crypto/hash/Makefile  |   1 +
 drivers/crypto/hash/hash_sw.c | 301 ++++++++++++++++++++++++++++++++++
 3 files changed, 313 insertions(+)
 create mode 100644 drivers/crypto/hash/hash_sw.c

diff --git a/drivers/crypto/hash/Kconfig b/drivers/crypto/hash/Kconfig
index e226144b9b..cd29a5c6a4 100644
--- a/drivers/crypto/hash/Kconfig
+++ b/drivers/crypto/hash/Kconfig
@@ -3,3 +3,14 @@ config DM_HASH
 	depends on DM
 	help
 	  If you want to use driver model for Hash, say Y.
+
+config HASH_SOFTWARE
+	bool "Enable driver for Hash in software"
+	depends on DM_HASH
+	depends on MD5
+	depends on SHA1
+	depends on SHA256
+	depends on SHA512_ALGO
+	help
+	  Enable driver for hashing operations in software. Currently
+	  it support multiple hash algorithm including CRC/MD5/SHA.
diff --git a/drivers/crypto/hash/Makefile b/drivers/crypto/hash/Makefile
index 83acf3d47b..33d88161ed 100644
--- a/drivers/crypto/hash/Makefile
+++ b/drivers/crypto/hash/Makefile
@@ -3,3 +3,4 @@
 # Copyright (c) 2021 ASPEED Technology Inc.
 
 obj-$(CONFIG_DM_HASH) += hash-uclass.o
+obj-$(CONFIG_HASH_SOFTWARE) += hash_sw.o
diff --git a/drivers/crypto/hash/hash_sw.c b/drivers/crypto/hash/hash_sw.c
new file mode 100644
index 0000000000..fea9d12609
--- /dev/null
+++ b/drivers/crypto/hash/hash_sw.c
@@ -0,0 +1,301 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2021 ASPEED Technology Inc.
+ * Author: ChiaWei Wang <chiawei_wang@aspeedtech.com>
+ */
+#include <config.h>
+#include <common.h>
+#include <dm.h>
+#include <log.h>
+#include <malloc.h>
+#include <watchdog.h>
+#include <u-boot/hash.h>
+#include <u-boot/crc.h>
+#include <u-boot/md5.h>
+#include <u-boot/sha1.h>
+#include <u-boot/sha256.h>
+#include <u-boot/sha512.h>
+
+/* CRC16-CCITT */
+static void hash_init_crc16_ccitt(void *ctx)
+{
+	*((uint16_t *)ctx) = 0;
+}
+
+static void hash_update_crc16_ccitt(void *ctx, const void *ibuf, uint32_t ilen)
+{
+	*((uint16_t *)ctx) = crc16_ccitt(*((uint16_t *)ctx), ibuf, ilen);
+}
+
+static void hash_finish_crc16_ccitt(void *ctx, void *obuf)
+{
+	*((uint16_t *)obuf) = *((uint16_t *)ctx);
+}
+
+/* CRC32 */
+static void hash_init_crc32(void *ctx)
+{
+	*((uint32_t *)ctx) = 0;
+}
+
+static void hash_update_crc32(void *ctx, const void *ibuf, uint32_t ilen)
+{
+	*((uint32_t *)ctx) = crc32(*((uint32_t *)ctx), ibuf, ilen);
+}
+
+static void hash_finish_crc32(void *ctx, void *obuf)
+{
+	*((uint32_t *)obuf) = *((uint32_t *)ctx);
+}
+
+/* MD5 */
+static void hash_init_md5(void *ctx)
+{
+	MD5Init((struct MD5Context *)ctx);
+}
+
+static void hash_update_md5(void *ctx, const void *ibuf, uint32_t ilen)
+{
+	MD5Update((struct MD5Context *)ctx, ibuf, ilen);
+}
+
+static void hash_finish_md5(void *ctx, void *obuf)
+{
+	MD5Final(obuf, (struct MD5Context *)ctx);
+}
+
+/* SHA1 */
+static void hash_init_sha1(void *ctx)
+{
+	sha1_starts((sha1_context *)ctx);
+}
+
+static void hash_update_sha1(void *ctx, const void *ibuf, uint32_t ilen)
+{
+	sha1_update((sha1_context *)ctx, ibuf, ilen);
+}
+
+static void hash_finish_sha1(void *ctx, void *obuf)
+{
+	sha1_finish((sha1_context *)ctx, obuf);
+}
+
+/* SHA256 */
+static void hash_init_sha256(void *ctx)
+{
+	sha256_starts((sha256_context *)ctx);
+}
+
+static void hash_update_sha256(void *ctx, const void *ibuf, uint32_t ilen)
+{
+	sha256_update((sha256_context *)ctx, ibuf, ilen);
+}
+
+static void hash_finish_sha256(void *ctx, void *obuf)
+{
+	sha256_finish((sha256_context *)ctx, obuf);
+}
+
+/* SHA384 */
+static void hash_init_sha384(void *ctx)
+{
+	sha384_starts((sha512_context *)ctx);
+}
+
+static void hash_update_sha384(void *ctx, const void *ibuf, uint32_t ilen)
+{
+	sha384_update((sha512_context *)ctx, ibuf, ilen);
+}
+
+static void hash_finish_sha384(void *ctx, void *obuf)
+{
+	sha384_finish((sha512_context *)ctx, obuf);
+}
+
+/* SHA512 */
+static void hash_init_sha512(void *ctx)
+{
+	sha512_starts((sha512_context *)ctx);
+}
+
+static void hash_update_sha512(void *ctx, const void *ibuf, uint32_t ilen)
+{
+	sha512_update((sha512_context *)ctx, ibuf, ilen);
+}
+
+static void hash_finish_sha512(void *ctx, void *obuf)
+{
+	sha512_finish((sha512_context *)ctx, obuf);
+}
+
+struct sw_hash_ctx {
+	enum HASH_ALGO algo;
+	uint8_t algo_ctx[];
+};
+
+struct sw_hash_impl {
+	void (*init)(void *ctx);
+	void (*update)(void *ctx, const void *ibuf, uint32_t ilen);
+	void (*finish)(void *ctx, void *obuf);
+	uint32_t ctx_alloc_sz;
+};
+
+static struct sw_hash_impl sw_hash_impl[HASH_ALGO_NUM] = {
+	[HASH_ALGO_CRC16_CCITT] = {
+		.init = hash_init_crc16_ccitt,
+		.update = hash_update_crc16_ccitt,
+		.finish = hash_finish_crc16_ccitt,
+		.ctx_alloc_sz = sizeof(uint16_t),
+	},
+
+	[HASH_ALGO_CRC32] = {
+		.init = hash_init_crc32,
+		.update = hash_update_crc32,
+		.finish = hash_finish_crc32,
+		.ctx_alloc_sz = sizeof(uint32_t),
+	},
+
+	[HASH_ALGO_MD5] = {
+		.init = hash_init_md5,
+		.update = hash_update_md5,
+		.finish = hash_finish_md5,
+		.ctx_alloc_sz = sizeof(struct MD5Context),
+	},
+
+	[HASH_ALGO_SHA1] = {
+		.init = hash_init_sha1,
+		.update = hash_update_sha1,
+		.finish = hash_finish_sha1,
+		.ctx_alloc_sz = sizeof(sha1_context),
+	},
+
+	[HASH_ALGO_SHA256] = {
+		.init = hash_init_sha256,
+		.update = hash_update_sha256,
+		.finish = hash_finish_sha256,
+		.ctx_alloc_sz = sizeof(sha256_context),
+	},
+
+	[HASH_ALGO_SHA384] = {
+		.init = hash_init_sha384,
+		.update = hash_update_sha384,
+		.finish = hash_finish_sha384,
+		.ctx_alloc_sz = sizeof(sha512_context),
+	},
+
+	[HASH_ALGO_SHA512] = {
+		.init = hash_init_sha512,
+		.update = hash_update_sha512,
+		.finish = hash_finish_sha512,
+		.ctx_alloc_sz = sizeof(sha512_context),
+	},
+};
+
+static int sw_hash_init(struct udevice *dev, enum HASH_ALGO algo, void **ctxp)
+{
+	struct sw_hash_ctx *hash_ctx;
+	struct sw_hash_impl *hash_impl = &sw_hash_impl[algo];
+
+	hash_ctx = malloc(sizeof(hash_ctx->algo) + hash_impl->ctx_alloc_sz);
+	if (!hash_ctx)
+		return -ENOMEM;
+
+	hash_ctx->algo = algo;
+
+	hash_impl->init(hash_ctx->algo_ctx);
+
+	*ctxp = hash_ctx;
+
+	return 0;
+}
+
+static int sw_hash_update(struct udevice *dev, void *ctx, const void *ibuf, uint32_t ilen)
+{
+	struct sw_hash_ctx *hash_ctx = ctx;
+	struct sw_hash_impl *hash_impl = &sw_hash_impl[hash_ctx->algo];
+
+	hash_impl->update(hash_ctx->algo_ctx, ibuf, ilen);
+
+	return 0;
+}
+
+static int sw_hash_finish(struct udevice *dev, void *ctx, void *obuf)
+{
+	struct sw_hash_ctx *hash_ctx = ctx;
+	struct sw_hash_impl *hash_impl = &sw_hash_impl[hash_ctx->algo];
+
+	hash_impl->finish(hash_ctx->algo_ctx, obuf);
+
+	free(ctx);
+
+	return 0;
+}
+
+static int sw_hash_digest_wd(struct udevice *dev, enum HASH_ALGO algo,
+			     const void *ibuf, const uint32_t ilen,
+			     void *obuf, uint32_t chunk_sz)
+{
+	int rc;
+	void *ctx;
+	const void *cur, *end;
+	uint32_t chunk;
+
+	rc = sw_hash_init(dev, algo, &ctx);
+	if (rc)
+		return rc;
+
+	if (CONFIG_IS_ENABLED(HW_WATCHDOG) || CONFIG_IS_ENABLED(WATCHDOG)) {
+		cur = ibuf;
+		end = ibuf + ilen;
+
+		while (cur < end) {
+			chunk = end - cur;
+			if (chunk > chunk_sz)
+				chunk = chunk_sz;
+
+			rc = sw_hash_update(dev, ctx, cur, chunk);
+			if (rc)
+				return rc;
+
+			cur += chunk;
+			WATCHDOG_RESET();
+		}
+	} else {
+		rc = sw_hash_update(dev, ctx, ibuf, ilen);
+		if (rc)
+			return rc;
+	}
+
+	rc = sw_hash_finish(dev, ctx, obuf);
+	if (rc)
+		return rc;
+
+	return 0;
+}
+
+static int sw_hash_digest(struct udevice *dev, enum HASH_ALGO algo,
+			  const void *ibuf, const uint32_t ilen,
+			  void *obuf)
+{
+	/* re-use the watchdog version with input length as the chunk_sz */
+	return sw_hash_digest_wd(dev, algo, ibuf, ilen, obuf, ilen);
+}
+
+static const struct hash_ops hash_ops_sw = {
+	.hash_init = sw_hash_init,
+	.hash_update = sw_hash_update,
+	.hash_finish = sw_hash_finish,
+	.hash_digest_wd = sw_hash_digest_wd,
+	.hash_digest = sw_hash_digest,
+};
+
+U_BOOT_DRIVER(hash_sw) = {
+	.name = "hash_sw",
+	.id = UCLASS_HASH,
+	.ops = &hash_ops_sw,
+	.flags = DM_FLAG_PRE_RELOC,
+};
+
+U_BOOT_DRVINFO(hash_sw) = {
+	.name = "hash_sw",
+};
-- 
2.17.1

[PATCH 4/4] fit: Use DM hash driver if supported

[Chia-Wei Wang]

Calculate hash using DM driver if supported.
For backward compatibility, the call to legacy
hash functions is reserved.

Signed-off-by: Chia-Wei Wang <chiawei_wang@aspeedtech.com>
---
 common/image-fit.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/common/image-fit.c b/common/image-fit.c
index d6b2c3c7ec..ec2e526356 100644
--- a/common/image-fit.c
+++ b/common/image-fit.c
@@ -25,6 +25,10 @@
 #include <asm/io.h>
 #include <malloc.h>
 #include <asm/global_data.h>
+#ifdef CONFIG_DM_HASH
+#include <dm.h>
+#include <u-boot/hash.h>
+#endif
 DECLARE_GLOBAL_DATA_PTR;
 #endif /* !USE_HOSTCC*/
 
@@ -1214,6 +1218,31 @@ int fit_set_timestamp(void *fit, int noffset, time_t timestamp)
 int calculate_hash(const void *data, int data_len, const char *algo,
 			uint8_t *value, int *value_len)
 {
+#if !defined(USE_HOSTCC) && defined(CONFIG_DM_HASH)
+	int rc;
+	enum HASH_ALGO hash_algo;
+	struct udevice *dev;
+
+	rc = uclass_get_device(UCLASS_HASH, 0, &dev);
+	if (rc) {
+		debug("failed to get hash device, rc=%d\n", rc);
+		return -1;
+	}
+
+	hash_algo = hash_algo_lookup_by_name(algo);
+	if (hash_algo == HASH_ALGO_INVALID) {
+		debug("Unsupported hash algorithm\n");
+		return -1;
+	};
+
+	rc = hash_digest_wd(dev, hash_algo, data, data_len, value, CHUNKSZ);
+	if (rc) {
+		debug("failed to get hash value, rc=%d\n", rc);
+		return -1;
+	}
+
+	*value_len = hash_algo_digest_size(hash_algo);
+#else
 	if (IMAGE_ENABLE_CRC32 && strcmp(algo, "crc32") == 0) {
 		*((uint32_t *)value) = crc32_wd(0, data, data_len,
 							CHUNKSZ_CRC32);
@@ -1242,6 +1271,7 @@ int calculate_hash(const void *data, int data_len, const char *algo,
 		debug("Unsupported hash alogrithm\n");
 		return -1;
 	}
+#endif
 	return 0;
 }
 
-- 
2.17.1

RE: [PATCH 0/4] crypto: Add new UCLASS_HASH

[ChiaWei Wang]

Hi All,

Do you have update on this patch series?
We look forward to continuing the SPL FIT booting patch for Aspeed SoCs based on this one.
Any advice and suggestions are appreciated.

Chiawei

> From: U-Boot <u-boot-bounces@lists.denx.de> On Behalf Of Chia-Wei Wang
> Sent: Friday, July 30, 2021 9:08 AM
> 
> This patch series proposes new UCLASS_HASH for hash devices.
> Thus the hash drivers (SW or HW-accelerated) can be developed in the
> DM-based fashion.
> 
> A purely software implemented hash driver is also added under the newly
> added UCLASS_HASH uclass. In addition, the FIT image hash verification is also
> updated to leverage the UCLASS_HASH driver if configured.
> 
> As there is widly spread use of non-DM hash functions (common/hash.c), this
> patch does not remove them. More patches are needed if UCLASS_HASH is
> established.
> 
> Chia-Wei Wang (4):
>   lib/md5: Export progressive APIs
>   dm: hash: Add new UCLASS_HASH support
>   crypto: hash: Add software hash DM driver
>   fit: Use DM hash driver if supported
> 
>  common/image-fit.c                |  30 +++
>  drivers/crypto/Kconfig            |   2 +
>  drivers/crypto/Makefile           |   1 +
>  drivers/crypto/hash/Kconfig       |  16 ++
>  drivers/crypto/hash/Makefile      |   6 +
>  drivers/crypto/hash/hash-uclass.c | 121 ++++++++++++
>  drivers/crypto/hash/hash_sw.c     | 301
> ++++++++++++++++++++++++++++++
>  include/dm/uclass-id.h            |   1 +
>  include/u-boot/hash.h             |  61 ++++++
>  include/u-boot/md5.h              |   4 +
>  lib/md5.c                         |   6 +-
>  11 files changed, 546 insertions(+), 3 deletions(-)  create mode 100644
> drivers/crypto/hash/Kconfig  create mode 100644
> drivers/crypto/hash/Makefile  create mode 100644
> drivers/crypto/hash/hash-uclass.c  create mode 100644
> drivers/crypto/hash/hash_sw.c  create mode 100644 include/u-boot/hash.h
> 
> --
> 2.17.1

Re: [PATCH 1/4] lib/md5: Export progressive APIs

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 265 bytes --]

On Fri, Jul 30, 2021 at 09:08:02AM +0800, Chia-Wei Wang wrote:

> Export the MD5 hash init/update/finish progressive APIs
> for better flexibility.
> 
> Signed-off-by: Chia-Wei Wang <chiawei_wang@aspeedtech.com>

Applied to u-boot/next, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 2/4] dm: hash: Add new UCLASS_HASH support

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 320 bytes --]

On Fri, Jul 30, 2021 at 09:08:03AM +0800, Chia-Wei Wang wrote:

> Add UCLASS_HASH for hash driver development. Thus the
> hash drivers (SW or HW-accelerated) can be developed
> in the DM-based fashion.
> 
> Signed-off-by: Chia-Wei Wang <chiawei_wang@aspeedtech.com>

Applied to u-boot/next, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 3/4] crypto: hash: Add software hash DM driver

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 348 bytes --]

On Fri, Jul 30, 2021 at 09:08:04AM +0800, Chia-Wei Wang wrote:

> Add purely software-implmented drivers to support multiple
> hash operations including CRC, MD5, and SHA family.
> 
> This driver is based on the new hash uclass.
> 
> Signed-off-by: Chia-Wei Wang <chiawei_wang@aspeedtech.com>

Applied to u-boot/next, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 4/4] fit: Use DM hash driver if supported

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 308 bytes --]

On Fri, Jul 30, 2021 at 09:08:05AM +0800, Chia-Wei Wang wrote:

> Calculate hash using DM driver if supported.
> For backward compatibility, the call to legacy
> hash functions is reserved.
> 
> Signed-off-by: Chia-Wei Wang <chiawei_wang@aspeedtech.com>

Applied to u-boot/next, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 2/4] dm: hash: Add new UCLASS_HASH support

[Alex G.]

Hi,

On 7/29/21 8:08 PM, Chia-Wei Wang wrote:
> Add UCLASS_HASH for hash driver development. Thus the
> hash drivers (SW or HW-accelerated) can be developed
> in the DM-based fashion.

Software hashing implementations are shared tightly with host tools. 
With DM, there's no opportunity for code sharing with host tools. The 
design question that I have is "do we want to DM hashes, or do we want 
to DM hardware accelerators for hashes?"

I did some parallel work expose remaining hash algos via 
hash_lookup_algo() and hash_progressive_lookup_algo().

> Signed-off-by: Chia-Wei Wang <chiawei_wang@aspeedtech.com>
> ---
>   drivers/crypto/Kconfig            |   2 +
>   drivers/crypto/Makefile           |   1 +
>   drivers/crypto/hash/Kconfig       |   5 ++
>   drivers/crypto/hash/Makefile      |   5 ++
>   drivers/crypto/hash/hash-uclass.c | 121 ++++++++++++++++++++++++++++++
>   include/dm/uclass-id.h            |   1 +
>   include/u-boot/hash.h             |  61 +++++++++++++++
>   7 files changed, 196 insertions(+)
>   create mode 100644 drivers/crypto/hash/Kconfig
>   create mode 100644 drivers/crypto/hash/Makefile
>   create mode 100644 drivers/crypto/hash/hash-uclass.c
>   create mode 100644 include/u-boot/hash.h
> 
> diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
> index 1ea116be75..0082177c21 100644
> --- a/drivers/crypto/Kconfig
> +++ b/drivers/crypto/Kconfig
> @@ -1,5 +1,7 @@
>   menu "Hardware crypto devices"
>   
> +source drivers/crypto/hash/Kconfig
> +
Hashes are useful outside of cryptographic functions, so it seems odd to 
merge them in crypto. For example, CRC32 is not a hash useful in crypto, 
but otherwise widely used in u-boot.

[snip]
> diff --git a/drivers/crypto/hash/hash-uclass.c b/drivers/crypto/hash/hash-uclass.c
> new file mode 100644
> index 0000000000..446eb9e56a
> --- /dev/null
> +++ b/drivers/crypto/hash/hash-uclass.c
> @@ -0,0 +1,121 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (c) 2021 ASPEED Technology Inc.
> + * Author: ChiaWei Wang <chiawei_wang@aspeedtech.com>
> + */
> +
> +#define LOG_CATEGORY UCLASS_HASH
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <asm/global_data.h>
> +#include <u-boot/hash.h>
> +#include <errno.h>
> +#include <fdtdec.h>
> +#include <malloc.h>
> +#include <asm/io.h>
> +#include <linux/list.h>
> +
> +struct hash_info {
> +	char *name;
> +	uint32_t digest_size;
> +};
> +
> +static const struct hash_info hash_info[HASH_ALGO_NUM] = {
> +	[HASH_ALGO_CRC16_CCITT] = { "crc16-ccitt", 2 },
> +	[HASH_ALGO_CRC32] = { "crc32", 4 },
> +	[HASH_ALGO_MD5] = { "md5", 16 },
> +	[HASH_ALGO_SHA1] = { "sha1", 20 },
> +	[HASH_ALGO_SHA256] = { "sha256", 32 },
> +	[HASH_ALGO_SHA384] = { "sha384", 48 },
> +	[HASH_ALGO_SHA512] = { "sha512", 64},
> +};

It seems a step backwards to have to enum {} our hash algos, since we 
already identify them by their strings (e.g. "sha256"). and then 
associated ops structure. The

> +
> +enum HASH_ALGO hash_algo_lookup_by_name(const char *name)

     string -> hash_lookup_algo() -> ops struct

Is the current way to do things. hash_algo_lookup_by_name() does the 
roundabout through an enum. That doesn't make sense to me.


Alex

Re: [PATCH 3/4] crypto: hash: Add software hash DM driver

[Alex G.]

On 7/29/21 8:08 PM, Chia-Wei Wang wrote:
> Add purely software-implmented drivers to support multiple
> hash operations including CRC, MD5, and SHA family.
> 
> This driver is based on the new hash uclass.
> 
> Signed-off-by: Chia-Wei Wang <chiawei_wang@aspeedtech.com>
> ---
>   drivers/crypto/hash/Kconfig   |  11 ++
>   drivers/crypto/hash/Makefile  |   1 +
>   drivers/crypto/hash/hash_sw.c | 301 ++++++++++++++++++++++++++++++++++
>   3 files changed, 313 insertions(+)
>   create mode 100644 drivers/crypto/hash/hash_sw.c
> 
> diff --git a/drivers/crypto/hash/Kconfig b/drivers/crypto/hash/Kconfig
> index e226144b9b..cd29a5c6a4 100644
> --- a/drivers/crypto/hash/Kconfig
> +++ b/drivers/crypto/hash/Kconfig
> @@ -3,3 +3,14 @@ config DM_HASH
>   	depends on DM
>   	help
>   	  If you want to use driver model for Hash, say Y.
> +
> +config HASH_SOFTWARE
> +	bool "Enable driver for Hash in software"
> +	depends on DM_HASH
> +	depends on MD5
> +	depends on SHA1
> +	depends on SHA256
> +	depends on SHA512_ALGO

I would have expected a U_BOOT_DRIVER() for each hash algo, rather than 
a U_BOOT_DRIVER() wich encompassess all possible algos. If I'm trying to 
use SHA256 in SPL, I might not have the room too add SHA1 and MD5, so 
I'd have issues using HASH_SOFTWARE, as designed.

> diff --git a/drivers/crypto/hash/hash_sw.c b/drivers/crypto/hash/hash_sw.c
> new file mode 100644
> index 0000000000..fea9d12609
> --- /dev/null
> +++ b/drivers/crypto/hash/hash_sw.c
> @@ -0,0 +1,301 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (c) 2021 ASPEED Technology Inc.
> + * Author: ChiaWei Wang <chiawei_wang@aspeedtech.com>
> + */
> +#include <config.h>
> +#include <common.h>
> +#include <dm.h>
> +#include <log.h>
> +#include <malloc.h>
> +#include <watchdog.h>
> +#include <u-boot/hash.h>
> +#include <u-boot/crc.h>
> +#include <u-boot/md5.h>
> +#include <u-boot/sha1.h>
> +#include <u-boot/sha256.h>
> +#include <u-boot/sha512.h>
> +
> +/* CRC16-CCITT */
> +static void hash_init_crc16_ccitt(void *ctx)
> +{
> +	*((uint16_t *)ctx) = 0;

Undefined behavior: Pointer aliased type-punning. I would suggest using 
memset(). Might not be necessarrym as expleined in the next comment.

> +static void hash_update_crc16_ccitt(void *ctx, const void *ibuf, uint32_t ilen)
> +static void hash_finish_crc16_ccitt(void *ctx, void *obuf)
> +/* CRC32 */
> +static void hash_init_crc32(void *ctx)
> +static void hash_update_crc32(void *ctx, const void *ibuf, uint32_t ilen)
> +static void hash_finish_crc32(void *ctx, void *obuf)
> +/* SHA1 */
> +static void hash_init_sha1(void *ctx)
> +/* SHA256 */
> +/* SHA384 */
> +/* SHA512 */

This logic already exists in common/hash.c for hash_Lookup_algo() and 
hash_progressive_algo().

Alex

Re: [PATCH 4/4] fit: Use DM hash driver if supported

[Alex G.]

On 7/29/21 8:08 PM, Chia-Wei Wang wrote:
> Calculate hash using DM driver if supported.
> For backward compatibility, the call to legacy
> hash functions is reserved.
> 
> Signed-off-by: Chia-Wei Wang <chiawei_wang@aspeedtech.com>
> ---
>   common/image-fit.c | 30 ++++++++++++++++++++++++++++++
>   1 file changed, 30 insertions(+)
> 
> diff --git a/common/image-fit.c b/common/image-fit.c
> index d6b2c3c7ec..ec2e526356 100644
> --- a/common/image-fit.c
> +++ b/common/image-fit.c
> @@ -25,6 +25,10 @@
>   #include <asm/io.h>
>   #include <malloc.h>
>   #include <asm/global_data.h>
> +#ifdef CONFIG_DM_HASH
> +#include <dm.h>
> +#include <u-boot/hash.h>
> +#endif
>   DECLARE_GLOBAL_DATA_PTR;
>   #endif /* !USE_HOSTCC*/
>   
> @@ -1214,6 +1218,31 @@ int fit_set_timestamp(void *fit, int noffset, time_t timestamp)
>   int calculate_hash(const void *data, int data_len, const char *algo,
>   			uint8_t *value, int *value_len)
>   {
> +#if !defined(USE_HOSTCC) && defined(CONFIG_DM_HASH)

This file is shared in its entirety with host tools. There isn't a huge 
opportunity for using a DM-type approach here without #ifndef USE_HOSTCC.

Alex



> +	int rc;
> +	enum HASH_ALGO hash_algo;
> +	struct udevice *dev;
> +
> +	rc = uclass_get_device(UCLASS_HASH, 0, &dev);
> +	if (rc) {
> +		debug("failed to get hash device, rc=%d\n", rc);
> +		return -1;
> +	}
> +
> +	hash_algo = hash_algo_lookup_by_name(algo);
> +	if (hash_algo == HASH_ALGO_INVALID) {
> +		debug("Unsupported hash algorithm\n");
> +		return -1;
> +	};
> +
> +	rc = hash_digest_wd(dev, hash_algo, data, data_len, value, CHUNKSZ);
> +	if (rc) {
> +		debug("failed to get hash value, rc=%d\n", rc);
> +		return -1;
> +	}
> +
> +	*value_len = hash_algo_digest_size(hash_algo);
> +#else
>   	if (IMAGE_ENABLE_CRC32 && strcmp(algo, "crc32") == 0) {
>   		*((uint32_t *)value) = crc32_wd(0, data, data_len,
>   							CHUNKSZ_CRC32);
> @@ -1242,6 +1271,7 @@ int calculate_hash(const void *data, int data_len, const char *algo,
>   		debug("Unsupported hash alogrithm\n");
>   		return -1;
>   	}
> +#endif
>   	return 0;
>   }
>   
> 

RE: [PATCH 2/4] dm: hash: Add new UCLASS_HASH support

[ChiaWei Wang]

Hi Alex,

> From: Alex G. <mr.nuke.me@gmail.com>
> Sent: Thursday, September 16, 2021 11:43 PM
> 
> Hi,
> 
> On 7/29/21 8:08 PM, Chia-Wei Wang wrote:
> > Add UCLASS_HASH for hash driver development. Thus the hash drivers (SW
> > or HW-accelerated) can be developed in the DM-based fashion.
> 
> Software hashing implementations are shared tightly with host tools.
> With DM, there's no opportunity for code sharing with host tools. The design
> question that I have is "do we want to DM hashes, or do we want to DM
> hardware accelerators for hashes?"
> 
> I did some parallel work expose remaining hash algos via
> hash_lookup_algo() and hash_progressive_lookup_algo().

DM-based approach is mainly for the U-Boot bootloader part.
A consistent, abstract interface is therefore available for vendor drivers regardless of the hashing are conducted in the SW or HW-assisted fashion.
And the CONFIG_SHAxxx_HW_ACCEL/CONFIG_SHA_PROG_HW_ACCEL options can be dropped.

Most of the current DM-based, SW hash driver implementation reuses the code of hash lib.
The code sharing benefit is still greatly leveraged.

> 
> > Signed-off-by: Chia-Wei Wang <chiawei_wang@aspeedtech.com>
> > ---
> >   drivers/crypto/Kconfig            |   2 +
> >   drivers/crypto/Makefile           |   1 +
> >   drivers/crypto/hash/Kconfig       |   5 ++
> >   drivers/crypto/hash/Makefile      |   5 ++
> >   drivers/crypto/hash/hash-uclass.c | 121
> ++++++++++++++++++++++++++++++
> >   include/dm/uclass-id.h            |   1 +
> >   include/u-boot/hash.h             |  61 +++++++++++++++
> >   7 files changed, 196 insertions(+)
> >   create mode 100644 drivers/crypto/hash/Kconfig
> >   create mode 100644 drivers/crypto/hash/Makefile
> >   create mode 100644 drivers/crypto/hash/hash-uclass.c
> >   create mode 100644 include/u-boot/hash.h
> >
> > diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig index
> > 1ea116be75..0082177c21 100644
> > --- a/drivers/crypto/Kconfig
> > +++ b/drivers/crypto/Kconfig
> > @@ -1,5 +1,7 @@
> >   menu "Hardware crypto devices"
> >
> > +source drivers/crypto/hash/Kconfig
> > +
> Hashes are useful outside of cryptographic functions, so it seems odd to merge
> them in crypto. For example, CRC32 is not a hash useful in crypto, but
> otherwise widely used in u-boot.

Certain systems have the hash functionality included in their crypto engine. (e.g. ARM, FSL, ASPEED, etc.)
Based on this observation, the DM hash driver is placed under crypto/.
However, it is OK for me to move the hash/ out of crypto/ if a more specific place is created and preferred.

> 
> [snip]
> > diff --git a/drivers/crypto/hash/hash-uclass.c
> > b/drivers/crypto/hash/hash-uclass.c
> > new file mode 100644
> > index 0000000000..446eb9e56a
> > --- /dev/null
> > +++ b/drivers/crypto/hash/hash-uclass.c
> > @@ -0,0 +1,121 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (c) 2021 ASPEED Technology Inc.
> > + * Author: ChiaWei Wang <chiawei_wang@aspeedtech.com>  */
> > +
> > +#define LOG_CATEGORY UCLASS_HASH
> > +
> > +#include <common.h>
> > +#include <dm.h>
> > +#include <asm/global_data.h>
> > +#include <u-boot/hash.h>
> > +#include <errno.h>
> > +#include <fdtdec.h>
> > +#include <malloc.h>
> > +#include <asm/io.h>
> > +#include <linux/list.h>
> > +
> > +struct hash_info {
> > +	char *name;
> > +	uint32_t digest_size;
> > +};
> > +
> > +static const struct hash_info hash_info[HASH_ALGO_NUM] = {
> > +	[HASH_ALGO_CRC16_CCITT] = { "crc16-ccitt", 2 },
> > +	[HASH_ALGO_CRC32] = { "crc32", 4 },
> > +	[HASH_ALGO_MD5] = { "md5", 16 },
> > +	[HASH_ALGO_SHA1] = { "sha1", 20 },
> > +	[HASH_ALGO_SHA256] = { "sha256", 32 },
> > +	[HASH_ALGO_SHA384] = { "sha384", 48 },
> > +	[HASH_ALGO_SHA512] = { "sha512", 64}, };
> 
> It seems a step backwards to have to enum {} our hash algos, since we already
> identify them by their strings (e.g. "sha256"). and then associated ops structure.
> The
> 
> > +
> > +enum HASH_ALGO hash_algo_lookup_by_name(const char *name)
> 
>      string -> hash_lookup_algo() -> ops struct
> 
> Is the current way to do things. hash_algo_lookup_by_name() does the
> roundabout through an enum. That doesn't make sense to me.
> 

The common hash-uclass.c also provides the string_to_enum conversion.
Both the DM-based hash driver works on both the string-based and enum-based scenario.

Chiawei

RE: [PATCH 3/4] crypto: hash: Add software hash DM driver

[ChiaWei Wang]

Hi Alex,

> From: Alex G. <mr.nuke.me@gmail.com>
> Sent: Thursday, September 16, 2021 11:49 PM
> 
> On 7/29/21 8:08 PM, Chia-Wei Wang wrote:
> > Add purely software-implmented drivers to support multiple hash
> > operations including CRC, MD5, and SHA family.
> >
> > This driver is based on the new hash uclass.
> >
> > Signed-off-by: Chia-Wei Wang <chiawei_wang@aspeedtech.com>
> > ---
> >   drivers/crypto/hash/Kconfig   |  11 ++
> >   drivers/crypto/hash/Makefile  |   1 +
> >   drivers/crypto/hash/hash_sw.c | 301
> ++++++++++++++++++++++++++++++++++
> >   3 files changed, 313 insertions(+)
> >   create mode 100644 drivers/crypto/hash/hash_sw.c
> >
> > diff --git a/drivers/crypto/hash/Kconfig b/drivers/crypto/hash/Kconfig
> > index e226144b9b..cd29a5c6a4 100644
> > --- a/drivers/crypto/hash/Kconfig
> > +++ b/drivers/crypto/hash/Kconfig
> > @@ -3,3 +3,14 @@ config DM_HASH
> >   	depends on DM
> >   	help
> >   	  If you want to use driver model for Hash, say Y.
> > +
> > +config HASH_SOFTWARE
> > +	bool "Enable driver for Hash in software"
> > +	depends on DM_HASH
> > +	depends on MD5
> > +	depends on SHA1
> > +	depends on SHA256
> > +	depends on SHA512_ALGO
> 
> I would have expected a U_BOOT_DRIVER() for each hash algo, rather than a
> U_BOOT_DRIVER() wich encompassess all possible algos. If I'm trying to use
> SHA256 in SPL, I might not have the room too add SHA1 and MD5, so I'd have
> issues using HASH_SOFTWARE, as designed.

I agree with the SPL size issue.
A future patches to move those CONFIG_SHAxxx/CONFIG_MD5 options into the DM-based hash_sw.c could be an option.
Thus the balance between the hash algos support and the code size can be managed.

> 
> > diff --git a/drivers/crypto/hash/hash_sw.c
> > b/drivers/crypto/hash/hash_sw.c new file mode 100644 index
> > 0000000000..fea9d12609
> > --- /dev/null
> > +++ b/drivers/crypto/hash/hash_sw.c
> > @@ -0,0 +1,301 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (c) 2021 ASPEED Technology Inc.
> > + * Author: ChiaWei Wang <chiawei_wang@aspeedtech.com>  */ #include
> > +<config.h> #include <common.h> #include <dm.h> #include <log.h>
> > +#include <malloc.h> #include <watchdog.h> #include <u-boot/hash.h>
> > +#include <u-boot/crc.h> #include <u-boot/md5.h> #include
> > +<u-boot/sha1.h> #include <u-boot/sha256.h> #include <u-boot/sha512.h>
> > +
> > +/* CRC16-CCITT */
> > +static void hash_init_crc16_ccitt(void *ctx) {
> > +	*((uint16_t *)ctx) = 0;
> 
> Undefined behavior: Pointer aliased type-punning. I would suggest using
> memset(). Might not be necessarrym as expleined in the next comment.
> 
> > +static void hash_update_crc16_ccitt(void *ctx, const void *ibuf,
> > +uint32_t ilen) static void hash_finish_crc16_ccitt(void *ctx, void
> > +*obuf)
> > +/* CRC32 */
> > +static void hash_init_crc32(void *ctx) static void
> > +hash_update_crc32(void *ctx, const void *ibuf, uint32_t ilen) static
> > +void hash_finish_crc32(void *ctx, void *obuf)
> > +/* SHA1 */
> > +static void hash_init_sha1(void *ctx)
> > +/* SHA256 */
> > +/* SHA384 */
> > +/* SHA512 */
> 
> This logic already exists in common/hash.c for hash_Lookup_algo() and
> hash_progressive_algo().

Yes.
To prevent modifying the 'static' keyword in common/hash.c while reusing the hash lib, the same logic appears in the DM-based hash_sw.c.

Chiawei

RE: [PATCH 4/4] fit: Use DM hash driver if supported

[ChiaWei Wang]

Hi Alex,

> From: Alex G. <mr.nuke.me@gmail.com>
> Sent: Friday, September 17, 2021 12:00 AM
> 
> On 7/29/21 8:08 PM, Chia-Wei Wang wrote:
> > Calculate hash using DM driver if supported.
> > For backward compatibility, the call to legacy hash functions is
> > reserved.
> >
> > Signed-off-by: Chia-Wei Wang <chiawei_wang@aspeedtech.com>
> > ---
> >   common/image-fit.c | 30 ++++++++++++++++++++++++++++++
> >   1 file changed, 30 insertions(+)
> >
> > diff --git a/common/image-fit.c b/common/image-fit.c index
> > d6b2c3c7ec..ec2e526356 100644
> > --- a/common/image-fit.c
> > +++ b/common/image-fit.c
> > @@ -25,6 +25,10 @@
> >   #include <asm/io.h>
> >   #include <malloc.h>
> >   #include <asm/global_data.h>
> > +#ifdef CONFIG_DM_HASH
> > +#include <dm.h>
> > +#include <u-boot/hash.h>
> > +#endif
> >   DECLARE_GLOBAL_DATA_PTR;
> >   #endif /* !USE_HOSTCC*/
> >
> > @@ -1214,6 +1218,31 @@ int fit_set_timestamp(void *fit, int noffset,
> time_t timestamp)
> >   int calculate_hash(const void *data, int data_len, const char *algo,
> >   			uint8_t *value, int *value_len)
> >   {
> > +#if !defined(USE_HOSTCC) && defined(CONFIG_DM_HASH)
> 
> This file is shared in its entirety with host tools. There isn't a huge opportunity
> for using a DM-type approach here without #ifndef USE_HOSTCC.
> 

There are still clean up missions to make U-boot bootloader to move on to the DM-based approach.

For instance, when a FIT image is verified by a hash digest, the hash is calculated by calculate_hash() in image-fit.c.
However, when a FIT image is verified by a signature, the hash comes from hash_calculate() in hash-checksum.c.
In my personal opinion, a consistent way to calculate hash for U-Boot bootloader would be better for maintenance.

To make this transition more smoothly, currently the USE_HOSTCC and CONFIG_DM_HASH are added in an ad-hoc way.

Chiawei

> > +	int rc;
> > +	enum HASH_ALGO hash_algo;
> > +	struct udevice *dev;
> > +
> > +	rc = uclass_get_device(UCLASS_HASH, 0, &dev);
> > +	if (rc) {
> > +		debug("failed to get hash device, rc=%d\n", rc);
> > +		return -1;
> > +	}
> > +
> > +	hash_algo = hash_algo_lookup_by_name(algo);
> > +	if (hash_algo == HASH_ALGO_INVALID) {
> > +		debug("Unsupported hash algorithm\n");
> > +		return -1;
> > +	};
> > +
> > +	rc = hash_digest_wd(dev, hash_algo, data, data_len, value, CHUNKSZ);
> > +	if (rc) {
> > +		debug("failed to get hash value, rc=%d\n", rc);
> > +		return -1;
> > +	}
> > +
> > +	*value_len = hash_algo_digest_size(hash_algo); #else
> >   	if (IMAGE_ENABLE_CRC32 && strcmp(algo, "crc32") == 0) {
> >   		*((uint32_t *)value) = crc32_wd(0, data, data_len,
> >   							CHUNKSZ_CRC32);
> > @@ -1242,6 +1271,7 @@ int calculate_hash(const void *data, int data_len,
> const char *algo,
> >   		debug("Unsupported hash alogrithm\n");
> >   		return -1;
> >   	}
> > +#endif
> >   	return 0;
> >   }
> >
> >

Re: [PATCH 2/4] dm: hash: Add new UCLASS_HASH support

[Simon Glass]

Hi,

On Thu, 2 Sept 2021 at 07:28, Tom Rini <trini@konsulko.com> wrote:
>
> On Fri, Jul 30, 2021 at 09:08:03AM +0800, Chia-Wei Wang wrote:
>
> > Add UCLASS_HASH for hash driver development. Thus the
> > hash drivers (SW or HW-accelerated) can be developed
> > in the DM-based fashion.
> >
> > Signed-off-by: Chia-Wei Wang <chiawei_wang@aspeedtech.com>
>
> Applied to u-boot/next, thanks!

Oddly enough I didn't see this patch but did see Tom's reply.

Regards,
SImon

RE: [PATCH 2/4] dm: hash: Add new UCLASS_HASH support

[ChiaWei Wang]

Hi Simon,

> From: Simon Glass <sjg@chromium.org>
> Sent: Thursday, September 23, 2021 12:19 AM
> 
> Hi,
> 
> On Thu, 2 Sept 2021 at 07:28, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Fri, Jul 30, 2021 at 09:08:03AM +0800, Chia-Wei Wang wrote:
> >
> > > Add UCLASS_HASH for hash driver development. Thus the hash drivers
> > > (SW or HW-accelerated) can be developed in the DM-based fashion.
> > >
> > > Signed-off-by: Chia-Wei Wang <chiawei_wang@aspeedtech.com>
> >
> > Applied to u-boot/next, thanks!
> 
> Oddly enough I didn't see this patch but did see Tom's reply.

Truly odd. You and Tom are on the '--to' list. 
I also checked the content sent on U-Boot Patchwork as shown below.

---
To: <sjg@chromium.org>, <trini@konsulko.com>, <u-boot@lists.denx.de>
Subject: [PATCH 2/4] dm: hash: Add new UCLASS_HASH support
---

Regards,
Chiawei

Re: [PATCH 2/4] dm: hash: Add new UCLASS_HASH support

[Simon Glass]

Hi,

On Wed, 22 Sept 2021 at 17:56, ChiaWei Wang <chiawei_wang@aspeedtech.com> wrote:
>
> Hi Simon,
>
> > From: Simon Glass <sjg@chromium.org>
> > Sent: Thursday, September 23, 2021 12:19 AM
> >
> > Hi,
> >
> > On Thu, 2 Sept 2021 at 07:28, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Fri, Jul 30, 2021 at 09:08:03AM +0800, Chia-Wei Wang wrote:
> > >
> > > > Add UCLASS_HASH for hash driver development. Thus the hash drivers
> > > > (SW or HW-accelerated) can be developed in the DM-based fashion.
> > > >
> > > > Signed-off-by: Chia-Wei Wang <chiawei_wang@aspeedtech.com>
> > >
> > > Applied to u-boot/next, thanks!
> >
> > Oddly enough I didn't see this patch but did see Tom's reply.
>
> Truly odd. You and Tom are on the '--to' list.
> I also checked the content sent on U-Boot Patchwork as shown below.
>
> ---
> To: <sjg@chromium.org>, <trini@konsulko.com>, <u-boot@lists.denx.de>
> Subject: [PATCH 2/4] dm: hash: Add new UCLASS_HASH support

Well it doesn't matter. It actually happened about 6 months ago too, I
think. I don't know what causes it but I suspect a spam filter as I
found a few patches in my spam folder. I'll try to train it.

Regards,
Simon

Re: [PATCH 2/4] dm: hash: Add new UCLASS_HASH support

[Simon Glass]

Hi,

On Thu, 16 Sept 2021 at 09:43, Alex G. <mr.nuke.me@gmail.com> wrote:
>
> Hi,
>
> On 7/29/21 8:08 PM, Chia-Wei Wang wrote:
> > Add UCLASS_HASH for hash driver development. Thus the
> > hash drivers (SW or HW-accelerated) can be developed
> > in the DM-based fashion.
>
> Software hashing implementations are shared tightly with host tools.
> With DM, there's no opportunity for code sharing with host tools. The
> design question that I have is "do we want to DM hashes, or do we want
> to DM hardware accelerators for hashes?"
>
> I did some parallel work expose remaining hash algos via
> hash_lookup_algo() and hash_progressive_lookup_algo().
>
> > Signed-off-by: Chia-Wei Wang <chiawei_wang@aspeedtech.com>
> > ---
> >   drivers/crypto/Kconfig            |   2 +
> >   drivers/crypto/Makefile           |   1 +
> >   drivers/crypto/hash/Kconfig       |   5 ++
> >   drivers/crypto/hash/Makefile      |   5 ++
> >   drivers/crypto/hash/hash-uclass.c | 121 ++++++++++++++++++++++++++++++
> >   include/dm/uclass-id.h            |   1 +
> >   include/u-boot/hash.h             |  61 +++++++++++++++
> >   7 files changed, 196 insertions(+)
> >   create mode 100644 drivers/crypto/hash/Kconfig
> >   create mode 100644 drivers/crypto/hash/Makefile
> >   create mode 100644 drivers/crypto/hash/hash-uclass.c
> >   create mode 100644 include/u-boot/hash.h
> >
> > diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
> > index 1ea116be75..0082177c21 100644
> > --- a/drivers/crypto/Kconfig
> > +++ b/drivers/crypto/Kconfig
> > @@ -1,5 +1,7 @@
> >   menu "Hardware crypto devices"
> >
> > +source drivers/crypto/hash/Kconfig
> > +
> Hashes are useful outside of cryptographic functions, so it seems odd to
> merge them in crypto. For example, CRC32 is not a hash useful in crypto,
> but otherwise widely used in u-boot.

Are you syching to move this to drivers/hash ? I feel that hashing is
close enough to crypto that it might not be worth having a new
'top-level' driver directory.

Some might say that md5 is in the same board (not useful for crypto)
but it used to be. Similarly, sha1 is fading away.

>
> [snip]
> > diff --git a/drivers/crypto/hash/hash-uclass.c b/drivers/crypto/hash/hash-uclass.c
> > new file mode 100644
> > index 0000000000..446eb9e56a
> > --- /dev/null
> > +++ b/drivers/crypto/hash/hash-uclass.c
> > @@ -0,0 +1,121 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (c) 2021 ASPEED Technology Inc.
> > + * Author: ChiaWei Wang <chiawei_wang@aspeedtech.com>
> > + */
> > +
> > +#define LOG_CATEGORY UCLASS_HASH
> > +
> > +#include <common.h>
> > +#include <dm.h>
> > +#include <asm/global_data.h>
> > +#include <u-boot/hash.h>
> > +#include <errno.h>
> > +#include <fdtdec.h>
> > +#include <malloc.h>
> > +#include <asm/io.h>
> > +#include <linux/list.h>
> > +
> > +struct hash_info {
> > +     char *name;
> > +     uint32_t digest_size;
> > +};
> > +
> > +static const struct hash_info hash_info[HASH_ALGO_NUM] = {
> > +     [HASH_ALGO_CRC16_CCITT] = { "crc16-ccitt", 2 },
> > +     [HASH_ALGO_CRC32] = { "crc32", 4 },
> > +     [HASH_ALGO_MD5] = { "md5", 16 },
> > +     [HASH_ALGO_SHA1] = { "sha1", 20 },
> > +     [HASH_ALGO_SHA256] = { "sha256", 32 },
> > +     [HASH_ALGO_SHA384] = { "sha384", 48 },
> > +     [HASH_ALGO_SHA512] = { "sha512", 64},
> > +};
>
> It seems a step backwards to have to enum {} our hash algos, since we
> already identify them by their strings (e.g. "sha256"). and then
> associated ops structure. The
>
> > +
> > +enum HASH_ALGO hash_algo_lookup_by_name(const char *name)
>
>      string -> hash_lookup_algo() -> ops struct
>
> Is the current way to do things. hash_algo_lookup_by_name() does the
> roundabout through an enum. That doesn't make sense to me.

Actually I'd like to see an enum to describe these, since looking up a
string is less efficient in a bootloader. So some way of doing that
seems good to me.

Of course it means we need a global list of this algos in a header
file and each driver needs to indicate which one(s) it supports.

Regards,
Simon

Re: [PATCH 2/4] dm: hash: Add new UCLASS_HASH support

[Alex G.]

On 9/23/21 9:49 PM, Simon Glass wrote:> On Thu, 16 Sept 2021 at 09:43, 
Alex G. <mr.nuke.me@gmail.com> wrote:
>> On 7/29/21 8:08 PM, Chia-Wei Wang wrote:

>>> +
>>> +enum HASH_ALGO hash_algo_lookup_by_name(const char *name)
>>
>>       string -> hash_lookup_algo() -> ops struct
>>
>> Is the current way to do things. hash_algo_lookup_by_name() does the
>> roundabout through an enum. That doesn't make sense to me.
> 
> Actually I'd like to see an enum to describe these, since looking up a
> string is less efficient in a bootloader. So some way of doing that
> seems good to me.
> 
> Of course it means we need a global list of this algos in a header
> file and each driver needs to indicate which one(s) it supports.

Let's assume strcmp() takes a lot of efforts. For a lot of cases, the 
algo comes in as a string. Think about FIT and image signature 
verification. So we have to do the strcmp() anyway, making the enum 
roundabout.

Re: [PATCH 2/4] dm: hash: Add new UCLASS_HASH support

[Simon Glass]

Hi Alex,

On Mon, 27 Sept 2021 at 09:37, Alex G. <mr.nuke.me@gmail.com> wrote:
>
> On 9/23/21 9:49 PM, Simon Glass wrote:> On Thu, 16 Sept 2021 at 09:43,
> Alex G. <mr.nuke.me@gmail.com> wrote:
> >> On 7/29/21 8:08 PM, Chia-Wei Wang wrote:
>
> >>> +
> >>> +enum HASH_ALGO hash_algo_lookup_by_name(const char *name)
> >>
> >>       string -> hash_lookup_algo() -> ops struct
> >>
> >> Is the current way to do things. hash_algo_lookup_by_name() does the
> >> roundabout through an enum. That doesn't make sense to me.
> >
> > Actually I'd like to see an enum to describe these, since looking up a
> > string is less efficient in a bootloader. So some way of doing that
> > seems good to me.
> >
> > Of course it means we need a global list of this algos in a header
> > file and each driver needs to indicate which one(s) it supports.
>
> Let's assume strcmp() takes a lot of efforts. For a lot of cases, the
> algo comes in as a string. Think about FIT and image signature
> verification. So we have to do the strcmp() anyway, making the enum
> roundabout.

The point is that in most code we can use the enum, thus saving time
and space. FIT is an external interface, so using a string makes more
sense there. Having said that, we could invent a new version of FIT
that has a binding file and uses ints.

Regards,
SImon