* [PATCH 1/3] hw_sha: Fix coding style errors
2021-02-17 3:20 [PATCH 0/3] Improvements to FIT hashing Joel Stanley
@ 2021-02-17 3:20 ` Joel Stanley
2021-04-13 14:27 ` Tom Rini
2021-02-17 3:20 ` [PATCH 2/3] fit: Use hash.c to call SHA code Joel Stanley
` (2 subsequent siblings)
3 siblings, 1 reply; 19+ messages in thread
From: Joel Stanley @ 2021-02-17 3:20 UTC (permalink / raw)
To: u-boot
Checkpatch complains about:
ERROR: "foo * bar" should be "foo *bar"
and
CHECK: Alignment should match open parenthesis
Signed-off-by: Joel Stanley <joel@jms.id.au>
---
include/hw_sha.h | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/include/hw_sha.h b/include/hw_sha.h
index 991e496a3cb2..15b1a1c79836 100644
--- a/include/hw_sha.h
+++ b/include/hw_sha.h
@@ -18,8 +18,8 @@
* should allocate at least 32 bytes at pOut in advance.
* @param chunk_size chunk size for sha256
*/
-void hw_sha256(const uchar * in_addr, uint buflen,
- uchar * out_addr, uint chunk_size);
+void hw_sha256(const uchar *in_addr, uint buflen, uchar *out_addr,
+ uint chunk_size);
/**
* Computes hash value of input pbuf using h/w acceleration
@@ -31,8 +31,8 @@ void hw_sha256(const uchar * in_addr, uint buflen,
* should allocate at least 32 bytes at pOut in advance.
* @param chunk_size chunk_size for sha1
*/
-void hw_sha1(const uchar * in_addr, uint buflen,
- uchar * out_addr, uint chunk_size);
+void hw_sha1(const uchar *in_addr, uint buflen, uchar *out_addr,
+ uint chunk_size);
/*
* Create the context for sha progressive hashing using h/w acceleration
@@ -56,7 +56,7 @@ int hw_sha_init(struct hash_algo *algo, void **ctxp);
* @return 0 if ok, -ve on error
*/
int hw_sha_update(struct hash_algo *algo, void *ctx, const void *buf,
- unsigned int size, int is_last);
+ unsigned int size, int is_last);
/*
* Copy sha hash result at destination location
@@ -70,6 +70,6 @@ int hw_sha_update(struct hash_algo *algo, void *ctx, const void *buf,
* @return 0 if ok, -ve on error
*/
int hw_sha_finish(struct hash_algo *algo, void *ctx, void *dest_buf,
- int size);
+ int size);
#endif
--
2.30.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 2/3] fit: Use hash.c to call SHA code
2021-02-17 3:20 [PATCH 0/3] Improvements to FIT hashing Joel Stanley
2021-02-17 3:20 ` [PATCH 1/3] hw_sha: Fix coding style errors Joel Stanley
@ 2021-02-17 3:20 ` Joel Stanley
2021-02-17 5:03 ` AKASHI Takahiro
2021-04-13 2:30 ` Tom Rini
2021-02-17 3:20 ` [PATCH 3/3] hash: Allow for SHA512 hardware implementations Joel Stanley
2021-04-12 23:50 ` [PATCH 0/3] Improvements to FIT hashing Joel Stanley
3 siblings, 2 replies; 19+ messages in thread
From: Joel Stanley @ 2021-02-17 3:20 UTC (permalink / raw)
To: u-boot
Currently the FIT hashing will call directly into the SHA algorithms to
get a hash.
This moves the fit code to use hash_lookup_algo, giving a common
entrypoint into the hashing code and removing the duplicated algorithm
look up. It also allows the use of hardware acceleration if configured.
Signed-off-by: Joel Stanley <joel@jms.id.au>
---
common/image-fit.c | 34 ++++++++--------------------------
1 file changed, 8 insertions(+), 26 deletions(-)
diff --git a/common/image-fit.c b/common/image-fit.c
index 28b3d2b19111..3451cdecc95b 100644
--- a/common/image-fit.c
+++ b/common/image-fit.c
@@ -1210,37 +1210,19 @@ int fit_set_timestamp(void *fit, int noffset, time_t timestamp)
* 0, on success
* -1, when algo is unsupported
*/
-int calculate_hash(const void *data, int data_len, const char *algo,
+int calculate_hash(const void *data, int data_len, const char *algo_name,
uint8_t *value, int *value_len)
{
- if (IMAGE_ENABLE_CRC32 && strcmp(algo, "crc32") == 0) {
- *((uint32_t *)value) = crc32_wd(0, data, data_len,
- CHUNKSZ_CRC32);
- *((uint32_t *)value) = cpu_to_uimage(*((uint32_t *)value));
- *value_len = 4;
- } else if (IMAGE_ENABLE_SHA1 && strcmp(algo, "sha1") == 0) {
- 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) {
- 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) {
- 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) {
- sha512_csum_wd((unsigned char *)data, data_len,
- (unsigned char *)value, CHUNKSZ_SHA512);
- *value_len = SHA512_SUM_LEN;
- } else if (IMAGE_ENABLE_MD5 && strcmp(algo, "md5") == 0) {
- md5_wd((unsigned char *)data, data_len, value, CHUNKSZ_MD5);
- *value_len = 16;
- } else {
+ struct hash_algo *algo;
+
+ if (hash_lookup_algo(algo_name, &algo)) {
debug("Unsupported hash alogrithm\n");
return -1;
}
+
+ algo->hash_func_ws(data, data_len, value, algo->chunk_size);
+ *value_len = algo->digest_size;
+
return 0;
}
--
2.30.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 2/3] fit: Use hash.c to call SHA code
2021-02-17 3:20 ` [PATCH 2/3] fit: Use hash.c to call SHA code Joel Stanley
@ 2021-02-17 5:03 ` AKASHI Takahiro
2021-04-12 23:52 ` Joel Stanley
2021-04-13 2:30 ` Tom Rini
1 sibling, 1 reply; 19+ messages in thread
From: AKASHI Takahiro @ 2021-02-17 5:03 UTC (permalink / raw)
To: u-boot
Simon,
# This is not a direct comment on this patch.
On Wed, Feb 17, 2021 at 01:50:41PM +1030, Joel Stanley wrote:
> Currently the FIT hashing will call directly into the SHA algorithms to
> get a hash.
>
> This moves the fit code to use hash_lookup_algo, giving a common
> entrypoint into the hashing code and removing the duplicated algorithm
> look up. It also allows the use of hardware acceleration if configured.
>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
> common/image-fit.c | 34 ++++++++--------------------------
> 1 file changed, 8 insertions(+), 26 deletions(-)
>
> diff --git a/common/image-fit.c b/common/image-fit.c
> index 28b3d2b19111..3451cdecc95b 100644
> --- a/common/image-fit.c
> +++ b/common/image-fit.c
> @@ -1210,37 +1210,19 @@ int fit_set_timestamp(void *fit, int noffset, time_t timestamp)
> * 0, on success
> * -1, when algo is unsupported
> */
> -int calculate_hash(const void *data, int data_len, const char *algo,
> +int calculate_hash(const void *data, int data_len, const char *algo_name,
> uint8_t *value, int *value_len)
> {
> - if (IMAGE_ENABLE_CRC32 && strcmp(algo, "crc32") == 0) {
> - *((uint32_t *)value) = crc32_wd(0, data, data_len,
> - CHUNKSZ_CRC32);
> - *((uint32_t *)value) = cpu_to_uimage(*((uint32_t *)value));
> - *value_len = 4;
> - } else if (IMAGE_ENABLE_SHA1 && strcmp(algo, "sha1") == 0) {
> - 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) {
> - 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) {
> - 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) {
> - sha512_csum_wd((unsigned char *)data, data_len,
> - (unsigned char *)value, CHUNKSZ_SHA512);
> - *value_len = SHA512_SUM_LEN;
> - } else if (IMAGE_ENABLE_MD5 && strcmp(algo, "md5") == 0) {
> - md5_wd((unsigned char *)data, data_len, value, CHUNKSZ_MD5);
> - *value_len = 16;
> - } else {
> + struct hash_algo *algo;
> +
> + if (hash_lookup_algo(algo_name, &algo)) {
> debug("Unsupported hash alogrithm\n");
> return -1;
> }
> +
> + algo->hash_func_ws(data, data_len, value, algo->chunk_size);
> + *value_len = algo->digest_size;
With this patch applied, there co-exists a very similar, hence
confusing function, hash_calculate(), in rsa-checksum.c (now checksum.c?).
If there is no particular reason for those two functions,
we'd better unify them?
-Takahiro Akashi
> return 0;
> }
>
> --
> 2.30.0
>
^ permalink raw reply [flat|nested] 19+ messages in thread* [PATCH 2/3] fit: Use hash.c to call SHA code
2021-02-17 5:03 ` AKASHI Takahiro
@ 2021-04-12 23:52 ` Joel Stanley
0 siblings, 0 replies; 19+ messages in thread
From: Joel Stanley @ 2021-04-12 23:52 UTC (permalink / raw)
To: u-boot
On Wed, 17 Feb 2021 at 05:04, AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:
>
> Simon,
>
> # This is not a direct comment on this patch.
>
> On Wed, Feb 17, 2021 at 01:50:41PM +1030, Joel Stanley wrote:
> > Currently the FIT hashing will call directly into the SHA algorithms to
> > get a hash.
> >
> > This moves the fit code to use hash_lookup_algo, giving a common
> > entrypoint into the hashing code and removing the duplicated algorithm
> > look up. It also allows the use of hardware acceleration if configured.
> >
> > Signed-off-by: Joel Stanley <joel@jms.id.au>
> > ---
> > common/image-fit.c | 34 ++++++++--------------------------
> > 1 file changed, 8 insertions(+), 26 deletions(-)
> >
> > diff --git a/common/image-fit.c b/common/image-fit.c
> > index 28b3d2b19111..3451cdecc95b 100644
> > --- a/common/image-fit.c
> > +++ b/common/image-fit.c
> > @@ -1210,37 +1210,19 @@ int fit_set_timestamp(void *fit, int noffset, time_t timestamp)
> > * 0, on success
> > * -1, when algo is unsupported
> > */
> > -int calculate_hash(const void *data, int data_len, const char *algo,
> > +int calculate_hash(const void *data, int data_len, const char *algo_name,
> > uint8_t *value, int *value_len)
> > {
> > - if (IMAGE_ENABLE_CRC32 && strcmp(algo, "crc32") == 0) {
> > - *((uint32_t *)value) = crc32_wd(0, data, data_len,
> > - CHUNKSZ_CRC32);
> > - *((uint32_t *)value) = cpu_to_uimage(*((uint32_t *)value));
> > - *value_len = 4;
> > - } else if (IMAGE_ENABLE_SHA1 && strcmp(algo, "sha1") == 0) {
> > - 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) {
> > - 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) {
> > - 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) {
> > - sha512_csum_wd((unsigned char *)data, data_len,
> > - (unsigned char *)value, CHUNKSZ_SHA512);
> > - *value_len = SHA512_SUM_LEN;
> > - } else if (IMAGE_ENABLE_MD5 && strcmp(algo, "md5") == 0) {
> > - md5_wd((unsigned char *)data, data_len, value, CHUNKSZ_MD5);
> > - *value_len = 16;
> > - } else {
> > + struct hash_algo *algo;
> > +
> > + if (hash_lookup_algo(algo_name, &algo)) {
> > debug("Unsupported hash alogrithm\n");
> > return -1;
> > }
> > +
> > + algo->hash_func_ws(data, data_len, value, algo->chunk_size);
> > + *value_len = algo->digest_size;
>
> With this patch applied, there co-exists a very similar, hence
> confusing function, hash_calculate(), in rsa-checksum.c (now checksum.c?).
> If there is no particular reason for those two functions,
> we'd better unify them?
hash_calculate is doing a progressive hash over a count of regions.
This code is hashing a single chunk of data.
I agree the naming could be improved to make this clearer.
Cheers,
Joel
>
> -Takahiro Akashi
>
>
> > return 0;
> > }
> >
> > --
> > 2.30.0
> >
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 2/3] fit: Use hash.c to call SHA code
2021-02-17 3:20 ` [PATCH 2/3] fit: Use hash.c to call SHA code Joel Stanley
2021-02-17 5:03 ` AKASHI Takahiro
@ 2021-04-13 2:30 ` Tom Rini
1 sibling, 0 replies; 19+ messages in thread
From: Tom Rini @ 2021-04-13 2:30 UTC (permalink / raw)
To: u-boot
On Wed, Feb 17, 2021 at 01:50:41PM +1030, Joel Stanley wrote:
> Currently the FIT hashing will call directly into the SHA algorithms to
> get a hash.
>
> This moves the fit code to use hash_lookup_algo, giving a common
> entrypoint into the hashing code and removing the duplicated algorithm
> look up. It also allows the use of hardware acceleration if configured.
>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
This breaks a few boards:
ls1046ardb_qspi_spl imx8mm_beacon imx8mn_beacon imx8mn_beacon_2g
imx8mm_evk imx8mn_ddr4_evk imx8mn_evk imx8mp_evk imx8mq_evk
imx8mm_venice imx8mq_phanbell phycore-imx8mm phycore-imx8mp pico-imx8mq
verdin-imx8mm mt8183_pumpkin mt8516_pumpkin
that use FIT images, in SPL, but don't actually check hashes apparently.
Just including hash.o for the hash-lookup function fails because we
don't have crc16, etc, enabled and also I think need:
https://patchwork.ozlabs.org/project/uboot/patch/20210322133331.1646575-1-mr.nuke.me at gmail.com/
for consistent symbol naming.
So, I like this patch in concept, but I think it'll need to be reworked
a bit, after I pull in some other changes soon. Thanks!
--
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210412/8e4c512e/attachment.sig>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 3/3] hash: Allow for SHA512 hardware implementations
2021-02-17 3:20 [PATCH 0/3] Improvements to FIT hashing Joel Stanley
2021-02-17 3:20 ` [PATCH 1/3] hw_sha: Fix coding style errors Joel Stanley
2021-02-17 3:20 ` [PATCH 2/3] fit: Use hash.c to call SHA code Joel Stanley
@ 2021-02-17 3:20 ` Joel Stanley
2021-04-13 14:27 ` Tom Rini
2021-05-12 16:01 ` Heinrich Schuchardt
2021-04-12 23:50 ` [PATCH 0/3] Improvements to FIT hashing Joel Stanley
3 siblings, 2 replies; 19+ messages in thread
From: Joel Stanley @ 2021-02-17 3:20 UTC (permalink / raw)
To: u-boot
Similar to support for SHA1 and SHA256, allow the use of hardware hashing
engine by enabling the algorithm and setting CONFIG_SHA_HW_ACCEL /
CONFIG_SHA_PROG_HW_ACCEL.
Signed-off-by: Joel Stanley <joel@jms.id.au>
---
common/hash.c | 24 ++++++++++++++++++++++--
include/hw_sha.h | 26 ++++++++++++++++++++++++++
lib/Kconfig | 15 +++++++--------
3 files changed, 55 insertions(+), 10 deletions(-)
diff --git a/common/hash.c b/common/hash.c
index fc64002f736a..10dff7ddb0e7 100644
--- a/common/hash.c
+++ b/common/hash.c
@@ -97,7 +97,7 @@ static int hash_finish_sha256(struct hash_algo *algo, void *ctx, void
}
#endif
-#if defined(CONFIG_SHA384)
+#if defined(CONFIG_SHA384) && !defined(CONFIG_SHA_PROG_HW_ACCEL)
static int hash_init_sha384(struct hash_algo *algo, void **ctxp)
{
sha512_context *ctx = malloc(sizeof(sha512_context));
@@ -125,7 +125,7 @@ static int hash_finish_sha384(struct hash_algo *algo, void *ctx, void
}
#endif
-#if defined(CONFIG_SHA512)
+#if defined(CONFIG_SHA512) && !defined(CONFIG_SHA_PROG_HW_ACCEL)
static int hash_init_sha512(struct hash_algo *algo, void **ctxp)
{
sha512_context *ctx = malloc(sizeof(sha512_context));
@@ -260,10 +260,20 @@ static struct hash_algo hash_algo[] = {
.name = "sha384",
.digest_size = SHA384_SUM_LEN,
.chunk_size = CHUNKSZ_SHA384,
+#ifdef CONFIG_SHA_HW_ACCEL
+ .hash_func_ws = hw_sha384,
+#else
.hash_func_ws = sha384_csum_wd,
+#endif
+#ifdef CONFIG_SHA_PROG_HW_ACCEL
+ .hash_init = hw_sha_init,
+ .hash_update = hw_sha_update,
+ .hash_finish = hw_sha_finish,
+#else
.hash_init = hash_init_sha384,
.hash_update = hash_update_sha384,
.hash_finish = hash_finish_sha384,
+#endif
},
#endif
#ifdef CONFIG_SHA512
@@ -271,10 +281,20 @@ static struct hash_algo hash_algo[] = {
.name = "sha512",
.digest_size = SHA512_SUM_LEN,
.chunk_size = CHUNKSZ_SHA512,
+#ifdef CONFIG_SHA_HW_ACCEL
+ .hash_func_ws = hw_sha512,
+#else
.hash_func_ws = sha512_csum_wd,
+#endif
+#ifdef CONFIG_SHA_PROG_HW_ACCEL
+ .hash_init = hw_sha_init,
+ .hash_update = hw_sha_update,
+ .hash_finish = hw_sha_finish,
+#else
.hash_init = hash_init_sha512,
.hash_update = hash_update_sha512,
.hash_finish = hash_finish_sha512,
+#endif
},
#endif
{
diff --git a/include/hw_sha.h b/include/hw_sha.h
index 15b1a1c79836..d4f3471c4308 100644
--- a/include/hw_sha.h
+++ b/include/hw_sha.h
@@ -8,6 +8,32 @@
#define __HW_SHA_H
#include <hash.h>
+/**
+ * Computes hash value of input pbuf using h/w acceleration
+ *
+ * @param in_addr A pointer to the input buffer
+ * @param bufleni Byte length of input buffer
+ * @param out_addr A pointer to the output buffer. When complete
+ * 64 bytes are copied to pout[0]...pout[63]. Thus, a user
+ * should allocate at least 64 bytes at pOut in advance.
+ * @param chunk_size chunk size for sha512
+ */
+void hw_sha512(const uchar *in_addr, uint buflen, uchar *out_addr,
+ uint chunk_size);
+
+/**
+ * Computes hash value of input pbuf using h/w acceleration
+ *
+ * @param in_addr A pointer to the input buffer
+ * @param bufleni Byte length of input buffer
+ * @param out_addr A pointer to the output buffer. When complete
+ * 48 bytes are copied to pout[0]...pout[47]. Thus, a user
+ * should allocate at least 48 bytes at pOut in advance.
+ * @param chunk_size chunk size for sha384
+ */
+void hw_sha384(const uchar *in_addr, uint buflen, uchar *out_addr,
+ uint chunk_size);
+
/**
* Computes hash value of input pbuf using h/w acceleration
*
diff --git a/lib/Kconfig b/lib/Kconfig
index b35a71ac368b..0d753eedeced 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -389,19 +389,18 @@ config SHA384
config SHA_HW_ACCEL
bool "Enable hashing using hardware"
help
- This option enables hardware acceleration
- for SHA1/SHA256 hashing.
- This affects the 'hash' command and also the
- hash_lookup_algo() function.
+ This option enables hardware acceleration for SHA hashing.
+ This affects the 'hash' command and also the hash_lookup_algo()
+ function.
config SHA_PROG_HW_ACCEL
bool "Enable Progressive hashing support using hardware"
depends on SHA_HW_ACCEL
help
- This option enables hardware-acceleration for
- SHA1/SHA256 progressive hashing.
- Data can be streamed in a block at a time and the hashing
- is performed in hardware.
+ This option enables hardware-acceleration for SHA progressive
+ hashing.
+ Data can be streamed in a block at a time and the hashing is
+ performed in hardware.
config MD5
bool "Support MD5 algorithm"
--
2.30.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 3/3] hash: Allow for SHA512 hardware implementations
2021-02-17 3:20 ` [PATCH 3/3] hash: Allow for SHA512 hardware implementations Joel Stanley
@ 2021-04-13 14:27 ` Tom Rini
2021-05-12 16:01 ` Heinrich Schuchardt
1 sibling, 0 replies; 19+ messages in thread
From: Tom Rini @ 2021-04-13 14:27 UTC (permalink / raw)
To: u-boot
On Wed, Feb 17, 2021 at 01:50:42PM +1030, Joel Stanley wrote:
> Similar to support for SHA1 and SHA256, allow the use of hardware hashing
> engine by enabling the algorithm and setting CONFIG_SHA_HW_ACCEL /
> CONFIG_SHA_PROG_HW_ACCEL.
>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
Applied to u-boot/master, thanks!
--
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210413/48c8dade/attachment.sig>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 3/3] hash: Allow for SHA512 hardware implementations
2021-02-17 3:20 ` [PATCH 3/3] hash: Allow for SHA512 hardware implementations Joel Stanley
2021-04-13 14:27 ` Tom Rini
@ 2021-05-12 16:01 ` Heinrich Schuchardt
2021-05-12 16:05 ` Simon Glass
1 sibling, 1 reply; 19+ messages in thread
From: Heinrich Schuchardt @ 2021-05-12 16:01 UTC (permalink / raw)
To: u-boot
On 17.02.21 04:20, Joel Stanley wrote:
> Similar to support for SHA1 and SHA256, allow the use of hardware hashing
> engine by enabling the algorithm and setting CONFIG_SHA_HW_ACCEL /
> CONFIG_SHA_PROG_HW_ACCEL.
>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
This merged patch leads to errors compiling the EFI TCG2 protocol on
boards with CONFIG_SHA_HW_ACCEL.
There is not as single implementation of hw_sha384 and hw_sha512. You
could only use CONFIG_SHA_HW_ACCEL for selecting these functions if
these were implemented for *all* boards with CONFIG_SHA_HW_ACCEL=y. But
this will never happen.
*This patch needs to be reverted.*
Why do we have CONFIG_SHA_HW_ACCEL at all and don't use weak functions
instead?
Best regards
Heinrich
> ---
> common/hash.c | 24 ++++++++++++++++++++++--
> include/hw_sha.h | 26 ++++++++++++++++++++++++++
> lib/Kconfig | 15 +++++++--------
> 3 files changed, 55 insertions(+), 10 deletions(-)
>
> diff --git a/common/hash.c b/common/hash.c
> index fc64002f736a..10dff7ddb0e7 100644
> --- a/common/hash.c
> +++ b/common/hash.c
> @@ -97,7 +97,7 @@ static int hash_finish_sha256(struct hash_algo *algo, void *ctx, void
> }
> #endif
>
> -#if defined(CONFIG_SHA384)
> +#if defined(CONFIG_SHA384) && !defined(CONFIG_SHA_PROG_HW_ACCEL)
> static int hash_init_sha384(struct hash_algo *algo, void **ctxp)
> {
> sha512_context *ctx = malloc(sizeof(sha512_context));
> @@ -125,7 +125,7 @@ static int hash_finish_sha384(struct hash_algo *algo, void *ctx, void
> }
> #endif
>
> -#if defined(CONFIG_SHA512)
> +#if defined(CONFIG_SHA512) && !defined(CONFIG_SHA_PROG_HW_ACCEL)
> static int hash_init_sha512(struct hash_algo *algo, void **ctxp)
> {
> sha512_context *ctx = malloc(sizeof(sha512_context));
> @@ -260,10 +260,20 @@ static struct hash_algo hash_algo[] = {
> .name = "sha384",
> .digest_size = SHA384_SUM_LEN,
> .chunk_size = CHUNKSZ_SHA384,
> +#ifdef CONFIG_SHA_HW_ACCEL
> + .hash_func_ws = hw_sha384,
> +#else
> .hash_func_ws = sha384_csum_wd,
> +#endif
> +#ifdef CONFIG_SHA_PROG_HW_ACCEL
> + .hash_init = hw_sha_init,
> + .hash_update = hw_sha_update,
> + .hash_finish = hw_sha_finish,
> +#else
> .hash_init = hash_init_sha384,
> .hash_update = hash_update_sha384,
> .hash_finish = hash_finish_sha384,
> +#endif
> },
> #endif
> #ifdef CONFIG_SHA512
> @@ -271,10 +281,20 @@ static struct hash_algo hash_algo[] = {
> .name = "sha512",
> .digest_size = SHA512_SUM_LEN,
> .chunk_size = CHUNKSZ_SHA512,
> +#ifdef CONFIG_SHA_HW_ACCEL
> + .hash_func_ws = hw_sha512,
> +#else
> .hash_func_ws = sha512_csum_wd,
> +#endif
> +#ifdef CONFIG_SHA_PROG_HW_ACCEL
> + .hash_init = hw_sha_init,
> + .hash_update = hw_sha_update,
> + .hash_finish = hw_sha_finish,
> +#else
> .hash_init = hash_init_sha512,
> .hash_update = hash_update_sha512,
> .hash_finish = hash_finish_sha512,
> +#endif
> },
> #endif
> {
> diff --git a/include/hw_sha.h b/include/hw_sha.h
> index 15b1a1c79836..d4f3471c4308 100644
> --- a/include/hw_sha.h
> +++ b/include/hw_sha.h
> @@ -8,6 +8,32 @@
> #define __HW_SHA_H
> #include <hash.h>
>
> +/**
> + * Computes hash value of input pbuf using h/w acceleration
> + *
> + * @param in_addr A pointer to the input buffer
> + * @param bufleni Byte length of input buffer
> + * @param out_addr A pointer to the output buffer. When complete
> + * 64 bytes are copied to pout[0]...pout[63]. Thus, a user
> + * should allocate at least 64 bytes at pOut in advance.
> + * @param chunk_size chunk size for sha512
> + */
> +void hw_sha512(const uchar *in_addr, uint buflen, uchar *out_addr,
> + uint chunk_size);
> +
> +/**
> + * Computes hash value of input pbuf using h/w acceleration
> + *
> + * @param in_addr A pointer to the input buffer
> + * @param bufleni Byte length of input buffer
> + * @param out_addr A pointer to the output buffer. When complete
> + * 48 bytes are copied to pout[0]...pout[47]. Thus, a user
> + * should allocate at least 48 bytes at pOut in advance.
> + * @param chunk_size chunk size for sha384
> + */
> +void hw_sha384(const uchar *in_addr, uint buflen, uchar *out_addr,
> + uint chunk_size);
> +
> /**
> * Computes hash value of input pbuf using h/w acceleration
> *
> diff --git a/lib/Kconfig b/lib/Kconfig
> index b35a71ac368b..0d753eedeced 100644
> --- a/lib/Kconfig
> +++ b/lib/Kconfig
> @@ -389,19 +389,18 @@ config SHA384
> config SHA_HW_ACCEL
> bool "Enable hashing using hardware"
> help
> - This option enables hardware acceleration
> - for SHA1/SHA256 hashing.
> - This affects the 'hash' command and also the
> - hash_lookup_algo() function.
> + This option enables hardware acceleration for SHA hashing.
> + This affects the 'hash' command and also the hash_lookup_algo()
> + function.
>
> config SHA_PROG_HW_ACCEL
> bool "Enable Progressive hashing support using hardware"
> depends on SHA_HW_ACCEL
> help
> - This option enables hardware-acceleration for
> - SHA1/SHA256 progressive hashing.
> - Data can be streamed in a block at a time and the hashing
> - is performed in hardware.
> + This option enables hardware-acceleration for SHA progressive
> + hashing.
> + Data can be streamed in a block at a time and the hashing is
> + performed in hardware.
>
> config MD5
> bool "Support MD5 algorithm"
>
^ permalink raw reply [flat|nested] 19+ messages in thread* [PATCH 3/3] hash: Allow for SHA512 hardware implementations
2021-05-12 16:01 ` Heinrich Schuchardt
@ 2021-05-12 16:05 ` Simon Glass
2021-05-12 16:19 ` Heinrich Schuchardt
0 siblings, 1 reply; 19+ messages in thread
From: Simon Glass @ 2021-05-12 16:05 UTC (permalink / raw)
To: u-boot
Hi Heinrich,
On Wed, 12 May 2021 at 10:01, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 17.02.21 04:20, Joel Stanley wrote:
> > Similar to support for SHA1 and SHA256, allow the use of hardware hashing
> > engine by enabling the algorithm and setting CONFIG_SHA_HW_ACCEL /
> > CONFIG_SHA_PROG_HW_ACCEL.
> >
> > Signed-off-by: Joel Stanley <joel@jms.id.au>
>
> This merged patch leads to errors compiling the EFI TCG2 protocol on
> boards with CONFIG_SHA_HW_ACCEL.
>
> There is not as single implementation of hw_sha384 and hw_sha512. You
> could only use CONFIG_SHA_HW_ACCEL for selecting these functions if
> these were implemented for *all* boards with CONFIG_SHA_HW_ACCEL=y. But
> this will never happen.
>
> *This patch needs to be reverted.*
>
> Why do we have CONFIG_SHA_HW_ACCEL at all and don't use weak functions
> instead?
This is all a mess. We should not use weak functions IMO, but instead
have a driver interface, like we do with filesystems.
Part of the challenge is the need to keep code size small for
platforms that only need one algorithm.
Regards,
Simon
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 3/3] hash: Allow for SHA512 hardware implementations
2021-05-12 16:05 ` Simon Glass
@ 2021-05-12 16:19 ` Heinrich Schuchardt
2021-05-12 17:29 ` Ilias Apalodimas
2021-05-12 17:32 ` Simon Glass
0 siblings, 2 replies; 19+ messages in thread
From: Heinrich Schuchardt @ 2021-05-12 16:19 UTC (permalink / raw)
To: u-boot
On 12.05.21 18:05, Simon Glass wrote:
> Hi Heinrich,
>
> On Wed, 12 May 2021 at 10:01, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 17.02.21 04:20, Joel Stanley wrote:
>>> Similar to support for SHA1 and SHA256, allow the use of hardware hashing
>>> engine by enabling the algorithm and setting CONFIG_SHA_HW_ACCEL /
>>> CONFIG_SHA_PROG_HW_ACCEL.
>>>
>>> Signed-off-by: Joel Stanley <joel@jms.id.au>
>>
>> This merged patch leads to errors compiling the EFI TCG2 protocol on
>> boards with CONFIG_SHA_HW_ACCEL.
>>
>> There is not as single implementation of hw_sha384 and hw_sha512. You
>> could only use CONFIG_SHA_HW_ACCEL for selecting these functions if
>> these were implemented for *all* boards with CONFIG_SHA_HW_ACCEL=y. But
>> this will never happen.
>>
>> *This patch needs to be reverted.*
>>
>> Why do we have CONFIG_SHA_HW_ACCEL at all and don't use weak functions
>> instead?
>
> This is all a mess. We should not use weak functions IMO, but instead
> have a driver interface, like we do with filesystems.
>
> Part of the challenge is the need to keep code size small for
> platforms that only need one algorithm.
If a function is not referenced, the linker will eliminate it. But with
a driver interface every function in the interface is referenced.
Best regards
Heinrich
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 3/3] hash: Allow for SHA512 hardware implementations
2021-05-12 16:19 ` Heinrich Schuchardt
@ 2021-05-12 17:29 ` Ilias Apalodimas
2021-05-12 17:32 ` Simon Glass
1 sibling, 0 replies; 19+ messages in thread
From: Ilias Apalodimas @ 2021-05-12 17:29 UTC (permalink / raw)
To: u-boot
Hi,
On Wed, May 12, 2021 at 06:19:58PM +0200, Heinrich Schuchardt wrote:
> On 12.05.21 18:05, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Wed, 12 May 2021 at 10:01, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> On 17.02.21 04:20, Joel Stanley wrote:
> >>> Similar to support for SHA1 and SHA256, allow the use of hardware hashing
> >>> engine by enabling the algorithm and setting CONFIG_SHA_HW_ACCEL /
> >>> CONFIG_SHA_PROG_HW_ACCEL.
> >>>
> >>> Signed-off-by: Joel Stanley <joel@jms.id.au>
> >>
> >> This merged patch leads to errors compiling the EFI TCG2 protocol on
> >> boards with CONFIG_SHA_HW_ACCEL.
> >>
> >> There is not as single implementation of hw_sha384 and hw_sha512. You
> >> could only use CONFIG_SHA_HW_ACCEL for selecting these functions if
> >> these were implemented for *all* boards with CONFIG_SHA_HW_ACCEL=y. But
> >> this will never happen.
> >>
> >> *This patch needs to be reverted.*
> >>
> >> Why do we have CONFIG_SHA_HW_ACCEL at all and don't use weak functions
> >> instead?
> >
> > This is all a mess. We should not use weak functions IMO, but instead
> > have a driver interface, like we do with filesystems.
> >
> > Part of the challenge is the need to keep code size small for
> > platforms that only need one algorithm.
>
> If a function is not referenced, the linker will eliminate it. But with
> a driver interface every function in the interface is referenced.
>
> Best regards
>
> Heinrich
There's a fundamental problem with TCG2. The algorithms you need to support
and log are dictated by the TPM2 hardware and it's configuration. That's a
thing we can't detect at build time.
So since we don't know that, we are requiring support for all the known
algorithms U-Boot supports and are mandated from the spec (SHA1/256/384/512).
One way to solve this is move the 'select' to 'depends', and only allow for
the protocol, if all the algorithms we need are present in the .config.
But this is just a way to sidestep the problem for now. I agree with Heinrich
that using a single Kconfig isn't realistic.
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 3/3] hash: Allow for SHA512 hardware implementations
2021-05-12 16:19 ` Heinrich Schuchardt
2021-05-12 17:29 ` Ilias Apalodimas
@ 2021-05-12 17:32 ` Simon Glass
2021-05-12 21:22 ` Heinrich Schuchardt
1 sibling, 1 reply; 19+ messages in thread
From: Simon Glass @ 2021-05-12 17:32 UTC (permalink / raw)
To: u-boot
Hi Heinrich,
On Wed, 12 May 2021 at 10:25, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 12.05.21 18:05, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Wed, 12 May 2021 at 10:01, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> On 17.02.21 04:20, Joel Stanley wrote:
> >>> Similar to support for SHA1 and SHA256, allow the use of hardware hashing
> >>> engine by enabling the algorithm and setting CONFIG_SHA_HW_ACCEL /
> >>> CONFIG_SHA_PROG_HW_ACCEL.
> >>>
> >>> Signed-off-by: Joel Stanley <joel@jms.id.au>
> >>
> >> This merged patch leads to errors compiling the EFI TCG2 protocol on
> >> boards with CONFIG_SHA_HW_ACCEL.
> >>
> >> There is not as single implementation of hw_sha384 and hw_sha512. You
> >> could only use CONFIG_SHA_HW_ACCEL for selecting these functions if
> >> these were implemented for *all* boards with CONFIG_SHA_HW_ACCEL=y. But
> >> this will never happen.
> >>
> >> *This patch needs to be reverted.*
> >>
> >> Why do we have CONFIG_SHA_HW_ACCEL at all and don't use weak functions
> >> instead?
> >
> > This is all a mess. We should not use weak functions IMO, but instead
> > have a driver interface, like we do with filesystems.
> >
> > Part of the challenge is the need to keep code size small for
> > platforms that only need one algorithm.
>
> If a function is not referenced, the linker will eliminate it. But with
> a driver interface every function in the interface is referenced.
Indeed, but we can still have an option for enabling the progressive
interface. My point is that the implementation (software or hardware)
should use a driver.
Regards,
Simon
> Heinrich
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 3/3] hash: Allow for SHA512 hardware implementations
2021-05-12 17:32 ` Simon Glass
@ 2021-05-12 21:22 ` Heinrich Schuchardt
2021-05-13 14:36 ` Simon Glass
0 siblings, 1 reply; 19+ messages in thread
From: Heinrich Schuchardt @ 2021-05-12 21:22 UTC (permalink / raw)
To: u-boot
Am 12. Mai 2021 19:32:42 MESZ schrieb Simon Glass <sjg@chromium.org>:
>Hi Heinrich,
>
>On Wed, 12 May 2021 at 10:25, Heinrich Schuchardt <xypron.glpk@gmx.de>
>wrote:
>>
>> On 12.05.21 18:05, Simon Glass wrote:
>> > Hi Heinrich,
>> >
>> > On Wed, 12 May 2021 at 10:01, Heinrich Schuchardt
><xypron.glpk@gmx.de> wrote:
>> >>
>> >> On 17.02.21 04:20, Joel Stanley wrote:
>> >>> Similar to support for SHA1 and SHA256, allow the use of hardware
>hashing
>> >>> engine by enabling the algorithm and setting CONFIG_SHA_HW_ACCEL
>/
>> >>> CONFIG_SHA_PROG_HW_ACCEL.
>> >>>
>> >>> Signed-off-by: Joel Stanley <joel@jms.id.au>
>> >>
>> >> This merged patch leads to errors compiling the EFI TCG2 protocol
>on
>> >> boards with CONFIG_SHA_HW_ACCEL.
>> >>
>> >> There is not as single implementation of hw_sha384 and hw_sha512.
>You
>> >> could only use CONFIG_SHA_HW_ACCEL for selecting these functions
>if
>> >> these were implemented for *all* boards with
>CONFIG_SHA_HW_ACCEL=y. But
>> >> this will never happen.
>> >>
>> >> *This patch needs to be reverted.*
>> >>
>> >> Why do we have CONFIG_SHA_HW_ACCEL at all and don't use weak
>functions
>> >> instead?
>> >
>> > This is all a mess. We should not use weak functions IMO, but
>instead
>> > have a driver interface, like we do with filesystems.
>> >
>> > Part of the challenge is the need to keep code size small for
>> > platforms that only need one algorithm.
>>
>> If a function is not referenced, the linker will eliminate it. But
>with
>> a driver interface every function in the interface is referenced.
>
>Indeed, but we can still have an option for enabling the progressive
>interface. My point is that the implementation (software or hardware)
>should use a driver.
>
>Regards,
>Simon
Anyway that should not stop us from reverting a problematic patch. We can work on refactoring afterwards.
Best regards
Heinrich
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 3/3] hash: Allow for SHA512 hardware implementations
2021-05-12 21:22 ` Heinrich Schuchardt
@ 2021-05-13 14:36 ` Simon Glass
2021-05-13 15:33 ` Heinrich Schuchardt
0 siblings, 1 reply; 19+ messages in thread
From: Simon Glass @ 2021-05-13 14:36 UTC (permalink / raw)
To: u-boot
Hi Heinrich,
On Wed, 12 May 2021 at 15:27, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> Am 12. Mai 2021 19:32:42 MESZ schrieb Simon Glass <sjg@chromium.org>:
> >Hi Heinrich,
> >
> >On Wed, 12 May 2021 at 10:25, Heinrich Schuchardt <xypron.glpk@gmx.de>
> >wrote:
> >>
> >> On 12.05.21 18:05, Simon Glass wrote:
> >> > Hi Heinrich,
> >> >
> >> > On Wed, 12 May 2021 at 10:01, Heinrich Schuchardt
> ><xypron.glpk@gmx.de> wrote:
> >> >>
> >> >> On 17.02.21 04:20, Joel Stanley wrote:
> >> >>> Similar to support for SHA1 and SHA256, allow the use of hardware
> >hashing
> >> >>> engine by enabling the algorithm and setting CONFIG_SHA_HW_ACCEL
> >/
> >> >>> CONFIG_SHA_PROG_HW_ACCEL.
> >> >>>
> >> >>> Signed-off-by: Joel Stanley <joel@jms.id.au>
> >> >>
> >> >> This merged patch leads to errors compiling the EFI TCG2 protocol
> >on
> >> >> boards with CONFIG_SHA_HW_ACCEL.
> >> >>
> >> >> There is not as single implementation of hw_sha384 and hw_sha512.
> >You
> >> >> could only use CONFIG_SHA_HW_ACCEL for selecting these functions
> >if
> >> >> these were implemented for *all* boards with
> >CONFIG_SHA_HW_ACCEL=y. But
> >> >> this will never happen.
> >> >>
> >> >> *This patch needs to be reverted.*
> >> >>
> >> >> Why do we have CONFIG_SHA_HW_ACCEL at all and don't use weak
> >functions
> >> >> instead?
> >> >
> >> > This is all a mess. We should not use weak functions IMO, but
> >instead
> >> > have a driver interface, like we do with filesystems.
> >> >
> >> > Part of the challenge is the need to keep code size small for
> >> > platforms that only need one algorithm.
> >>
> >> If a function is not referenced, the linker will eliminate it. But
> >with
> >> a driver interface every function in the interface is referenced.
> >
> >Indeed, but we can still have an option for enabling the progressive
> >interface. My point is that the implementation (software or hardware)
> >should use a driver.
> >
> >Regards,
> >Simon
>
> Anyway that should not stop us from reverting a problematic patch. We can work on refactoring afterwards.
Well the patch was applied because tests passed. We should be careful
about reverting a patch due to problems it causes in the future.
Regards,
Simon
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 3/3] hash: Allow for SHA512 hardware implementations
2021-05-13 14:36 ` Simon Glass
@ 2021-05-13 15:33 ` Heinrich Schuchardt
2021-05-13 23:56 ` Simon Glass
0 siblings, 1 reply; 19+ messages in thread
From: Heinrich Schuchardt @ 2021-05-13 15:33 UTC (permalink / raw)
To: u-boot
On 5/13/21 4:36 PM, Simon Glass wrote:
> Hi Heinrich,
>
> On Wed, 12 May 2021 at 15:27, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> Am 12. Mai 2021 19:32:42 MESZ schrieb Simon Glass <sjg@chromium.org>:
>>> Hi Heinrich,
>>>
>>> On Wed, 12 May 2021 at 10:25, Heinrich Schuchardt <xypron.glpk@gmx.de>
>>> wrote:
>>>>
>>>> On 12.05.21 18:05, Simon Glass wrote:
>>>>> Hi Heinrich,
>>>>>
>>>>> On Wed, 12 May 2021 at 10:01, Heinrich Schuchardt
>>> <xypron.glpk@gmx.de> wrote:
>>>>>>
>>>>>> On 17.02.21 04:20, Joel Stanley wrote:
>>>>>>> Similar to support for SHA1 and SHA256, allow the use of hardware
>>> hashing
>>>>>>> engine by enabling the algorithm and setting CONFIG_SHA_HW_ACCEL
>>> /
>>>>>>> CONFIG_SHA_PROG_HW_ACCEL.
>>>>>>>
>>>>>>> Signed-off-by: Joel Stanley <joel@jms.id.au>
>>>>>>
>>>>>> This merged patch leads to errors compiling the EFI TCG2 protocol
>>> on
>>>>>> boards with CONFIG_SHA_HW_ACCEL.
>>>>>>
>>>>>> There is not as single implementation of hw_sha384 and hw_sha512.
>>> You
>>>>>> could only use CONFIG_SHA_HW_ACCEL for selecting these functions
>>> if
>>>>>> these were implemented for *all* boards with
>>> CONFIG_SHA_HW_ACCEL=y. But
>>>>>> this will never happen.
>>>>>>
>>>>>> *This patch needs to be reverted.*
>>>>>>
>>>>>> Why do we have CONFIG_SHA_HW_ACCEL at all and don't use weak
>>> functions
>>>>>> instead?
>>>>>
>>>>> This is all a mess. We should not use weak functions IMO, but
>>> instead
>>>>> have a driver interface, like we do with filesystems.
>>>>>
>>>>> Part of the challenge is the need to keep code size small for
>>>>> platforms that only need one algorithm.
>>>>
>>>> If a function is not referenced, the linker will eliminate it. But
>>> with
>>>> a driver interface every function in the interface is referenced.
>>>
>>> Indeed, but we can still have an option for enabling the progressive
>>> interface. My point is that the implementation (software or hardware)
>>> should use a driver.
>>>
>>> Regards,
>>> Simon
>>
>> Anyway that should not stop us from reverting a problematic patch. We can work on refactoring afterwards.
>
> Well the patch was applied because tests passed. We should be careful
> about reverting a patch due to problems it causes in the future.
The code compiled because SHA384 and SHA512 were not used together with
CONFIG_SHA_HW_ACCEL=y. But we need these functions to implement the TCG2
protocol on boards with TPMv2.
Gitlab CI had no issues with reverting the patch.
Best regards
Heinrich
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 3/3] hash: Allow for SHA512 hardware implementations
2021-05-13 15:33 ` Heinrich Schuchardt
@ 2021-05-13 23:56 ` Simon Glass
0 siblings, 0 replies; 19+ messages in thread
From: Simon Glass @ 2021-05-13 23:56 UTC (permalink / raw)
To: u-boot
Hi Heinrich,
On Thu, 13 May 2021 at 09:38, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 5/13/21 4:36 PM, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Wed, 12 May 2021 at 15:27, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> Am 12. Mai 2021 19:32:42 MESZ schrieb Simon Glass <sjg@chromium.org>:
> >>> Hi Heinrich,
> >>>
> >>> On Wed, 12 May 2021 at 10:25, Heinrich Schuchardt <xypron.glpk@gmx.de>
> >>> wrote:
> >>>>
> >>>> On 12.05.21 18:05, Simon Glass wrote:
> >>>>> Hi Heinrich,
> >>>>>
> >>>>> On Wed, 12 May 2021 at 10:01, Heinrich Schuchardt
> >>> <xypron.glpk@gmx.de> wrote:
> >>>>>>
> >>>>>> On 17.02.21 04:20, Joel Stanley wrote:
> >>>>>>> Similar to support for SHA1 and SHA256, allow the use of hardware
> >>> hashing
> >>>>>>> engine by enabling the algorithm and setting CONFIG_SHA_HW_ACCEL
> >>> /
> >>>>>>> CONFIG_SHA_PROG_HW_ACCEL.
> >>>>>>>
> >>>>>>> Signed-off-by: Joel Stanley <joel@jms.id.au>
> >>>>>>
> >>>>>> This merged patch leads to errors compiling the EFI TCG2 protocol
> >>> on
> >>>>>> boards with CONFIG_SHA_HW_ACCEL.
> >>>>>>
> >>>>>> There is not as single implementation of hw_sha384 and hw_sha512.
> >>> You
> >>>>>> could only use CONFIG_SHA_HW_ACCEL for selecting these functions
> >>> if
> >>>>>> these were implemented for *all* boards with
> >>> CONFIG_SHA_HW_ACCEL=y. But
> >>>>>> this will never happen.
> >>>>>>
> >>>>>> *This patch needs to be reverted.*
> >>>>>>
> >>>>>> Why do we have CONFIG_SHA_HW_ACCEL at all and don't use weak
> >>> functions
> >>>>>> instead?
> >>>>>
> >>>>> This is all a mess. We should not use weak functions IMO, but
> >>> instead
> >>>>> have a driver interface, like we do with filesystems.
> >>>>>
> >>>>> Part of the challenge is the need to keep code size small for
> >>>>> platforms that only need one algorithm.
> >>>>
> >>>> If a function is not referenced, the linker will eliminate it. But
> >>> with
> >>>> a driver interface every function in the interface is referenced.
> >>>
> >>> Indeed, but we can still have an option for enabling the progressive
> >>> interface. My point is that the implementation (software or hardware)
> >>> should use a driver.
> >>>
> >>> Regards,
> >>> Simon
> >>
> >> Anyway that should not stop us from reverting a problematic patch. We can work on refactoring afterwards.
> >
> > Well the patch was applied because tests passed. We should be careful
> > about reverting a patch due to problems it causes in the future.
>
> The code compiled because SHA384 and SHA512 were not used together with
> CONFIG_SHA_HW_ACCEL=y. But we need these functions to implement the TCG2
> protocol on boards with TPMv2.
Let's fix it then. The patch makes all the hashing algorithms work the
same, which seems right to me. If we come to converting this to a
linker list, which we should, then it will be easier with this patch
applied than not.
>
> Gitlab CI had no issues with reverting the patch.
OK. It's up to Tom what he wants to do. But how about sending a patch
to do what you want, instead?
Regards,
SImon
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 0/3] Improvements to FIT hashing
2021-02-17 3:20 [PATCH 0/3] Improvements to FIT hashing Joel Stanley
` (2 preceding siblings ...)
2021-02-17 3:20 ` [PATCH 3/3] hash: Allow for SHA512 hardware implementations Joel Stanley
@ 2021-04-12 23:50 ` Joel Stanley
3 siblings, 0 replies; 19+ messages in thread
From: Joel Stanley @ 2021-04-12 23:50 UTC (permalink / raw)
To: u-boot
Helli Simon,
On Wed, 17 Feb 2021 at 03:20, Joel Stanley <joel@jms.id.au> wrote:
>
> Here are some small changes to the FIT hashing code in order to use
> more code from common/, which in turns allows hw implementations of SHA.
>
> This was motivated by a need to reduce the SPL size for the Aspeed
> platforms by using the hardware engine. I have a driver for the Aspeed
> SoC that I will submit.
Do you have any thoughts on this series?
Cheers,
Joel
>
> Joel Stanley (3):
> hw_sha: Fix coding style errors
> fit: Use hash.c to call SHA code
> hash: Allow for SHA512 hardware implementations
>
> common/hash.c | 24 ++++++++++++++++++++++--
> common/image-fit.c | 34 ++++++++--------------------------
> include/hw_sha.h | 38 ++++++++++++++++++++++++++++++++------
> lib/Kconfig | 15 +++++++--------
> 4 files changed, 69 insertions(+), 42 deletions(-)
>
> --
> 2.30.0
>
^ permalink raw reply [flat|nested] 19+ messages in thread