Re: [PATCH 2/4] dm: hash: Add new UCLASS_HASH support
Hi Alex, On Mon, 27 Sept 2021 at 09:37, Alex G. wrote: > > On 9/23/21 9:49 PM, Simon Glass wrote:> On Thu, 16 Sept 2021 at 09:43, > Alex G. 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
Re: [PATCH 2/4] dm: hash: Add new UCLASS_HASH support
On 9/23/21 9:49 PM, Simon Glass wrote:> On Thu, 16 Sept 2021 at 09:43, Alex G. 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
Hi,
On Thu, 16 Sept 2021 at 09:43, Alex G. 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
> > ---
> > 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 00..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
> > + */
> > +
> > +#define LOG_CATEGORY UCLASS_HASH
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +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
Hi, On Wed, 22 Sept 2021 at 17:56, ChiaWei Wang wrote: > > Hi Simon, > > > From: Simon Glass > > Sent: Thursday, September 23, 2021 12:19 AM > > > > Hi, > > > > On Thu, 2 Sept 2021 at 07:28, Tom Rini 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 > > > > > > 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: , , > 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
Hi Simon, > From: Simon Glass > Sent: Thursday, September 23, 2021 12:19 AM > > Hi, > > On Thu, 2 Sept 2021 at 07:28, Tom Rini 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 > > > > 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: , , Subject: [PATCH 2/4] dm: hash: Add new UCLASS_HASH support --- Regards, Chiawei
Re: [PATCH 2/4] dm: hash: Add new UCLASS_HASH support
Hi, On Thu, 2 Sept 2021 at 07:28, Tom Rini 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 > > 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
Hi Alex,
> From: Alex G.
> 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
> > ---
> > 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 00..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 */
> > +
> > +#define LOG_CATEGORY UCLASS_HASH
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +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 2/4] dm: hash: Add new UCLASS_HASH support
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
---
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 00..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
+ */
+
+#define LOG_CATEGORY UCLASS_HASH
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+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 2/4] dm: hash: Add new UCLASS_HASH support
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 Applied to u-boot/next, thanks! -- Tom signature.asc Description: PGP signature
[PATCH 2/4] dm: hash: Add new UCLASS_HASH support
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
---
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 00..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 00..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 00..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
+ */
+
+#define LOG_CATEGORY UCLASS_HASH
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+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_finis
