RE: [PATCH v7 1/5] crypto: aspeed: Add HACE hash driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> -----Original Message-----
> From: Dhananjay Phadke <dphadke@xxxxxxxxxxxxxxxxxxx>
> Sent: Wednesday, July 13, 2022 1:32 PM
> To: Neal Liu <neal_liu@xxxxxxxxxxxxxx>; Corentin Labbe
> <clabbe.montjoie@xxxxxxxxx>; Christophe JAILLET
> <christophe.jaillet@xxxxxxxxxx>; Randy Dunlap <rdunlap@xxxxxxxxxxxxx>;
> Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>; David S . Miller
> <davem@xxxxxxxxxxxxx>; Rob Herring <robh+dt@xxxxxxxxxx>; Krzysztof
> Kozlowski <krzysztof.kozlowski+dt@xxxxxxxxxx>; Joel Stanley <joel@xxxxxxxxx>;
> Andrew Jeffery <andrew@xxxxxxxx>; Dhananjay Phadke
> <dhphadke@xxxxxxxxxxxxx>; Johnny Huang
> <johnny_huang@xxxxxxxxxxxxxx>
> Cc: linux-aspeed@xxxxxxxxxxxxxxxx; linux-crypto@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; BMC-SW <BMC-SW@xxxxxxxxxxxxxx>
> Subject: Re: [PATCH v7 1/5] crypto: aspeed: Add HACE hash driver
> 
> Minor comments inline.
> 
> On 7/4/2022 7:09 PM, Neal Liu wrote:
> > Hash and Crypto Engine (HACE) is designed to accelerate the
> > throughput of hash data digest, encryption, and decryption.
> >
> > Basically, HACE can be divided into two independently engines
> > - Hash Engine and Crypto Engine. This patch aims to add HACE
> > hash engine driver for hash accelerator.
> >
> > Signed-off-by: Neal Liu <neal_liu@xxxxxxxxxxxxxx>
> > Signed-off-by: Johnny Huang <johnny_huang@xxxxxxxxxxxxxx>
> > ---
> >   MAINTAINERS                              |    7 +
> >   drivers/crypto/Kconfig                   |    1 +
> >   drivers/crypto/Makefile                  |    1 +
> >   drivers/crypto/aspeed/Kconfig            |   23 +
> >   drivers/crypto/aspeed/Makefile           |    6 +
> >   drivers/crypto/aspeed/aspeed-hace-hash.c | 1428
> ++++++++++++++++++++++
> >   drivers/crypto/aspeed/aspeed-hace.c      |  213 ++++
> >   drivers/crypto/aspeed/aspeed-hace.h      |  181 +++
> >   8 files changed, 1860 insertions(+)
> >   create mode 100644 drivers/crypto/aspeed/Kconfig
> >   create mode 100644 drivers/crypto/aspeed/Makefile
> >   create mode 100644 drivers/crypto/aspeed/aspeed-hace-hash.c
> >   create mode 100644 drivers/crypto/aspeed/aspeed-hace.c
> >   create mode 100644 drivers/crypto/aspeed/aspeed-hace.h
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 3cf9842d9233..63276700ee26 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -3136,6 +3136,13 @@ S:	Maintained
> >   F:	Documentation/devicetree/bindings/media/aspeed-video.txt
> >   F:	drivers/media/platform/aspeed/
> >
> > +ASPEED CRYPTO DRIVER
> > +M:	Neal Liu <neal_liu@xxxxxxxxxxxxxx>
> > +L:	linux-aspeed@xxxxxxxxxxxxxxxx (moderated for non-subscribers)
> > +S:	Maintained
> > +F:
> 	Documentation/devicetree/bindings/crypto/aspeed,ast2500-hace.yaml
> > +F:	drivers/crypto/aspeed/
> > +
> >   ASUS NOTEBOOKS AND EEEPC ACPI/WMI EXTRAS DRIVERS
> >   M:	Corentin Chary <corentin.chary@xxxxxxxxx>
> >   L:	acpi4asus-user@xxxxxxxxxxxxxxxxxxxxx
> > diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
> > index ee99c02c84e8..b9f5ee126881 100644
> > --- a/drivers/crypto/Kconfig
> > +++ b/drivers/crypto/Kconfig
> > @@ -933,5 +933,6 @@ config CRYPTO_DEV_SA2UL
> >   	  acceleration for cryptographic algorithms on these devices.
> >
> >   source "drivers/crypto/keembay/Kconfig"
> > +source "drivers/crypto/aspeed/Kconfig"
> >
> >   endif # CRYPTO_HW
> > diff --git a/drivers/crypto/Makefile b/drivers/crypto/Makefile
> > index f81703a86b98..116de173a66c 100644
> > --- a/drivers/crypto/Makefile
> > +++ b/drivers/crypto/Makefile
> > @@ -1,5 +1,6 @@
> >   # SPDX-License-Identifier: GPL-2.0
> >   obj-$(CONFIG_CRYPTO_DEV_ALLWINNER) += allwinner/
> > +obj-$(CONFIG_CRYPTO_DEV_ASPEED) += aspeed/
> >   obj-$(CONFIG_CRYPTO_DEV_ATMEL_AES) += atmel-aes.o
> >   obj-$(CONFIG_CRYPTO_DEV_ATMEL_SHA) += atmel-sha.o
> >   obj-$(CONFIG_CRYPTO_DEV_ATMEL_TDES) += atmel-tdes.o
> > diff --git a/drivers/crypto/aspeed/Kconfig b/drivers/crypto/aspeed/Kconfig
> > new file mode 100644
> > index 000000000000..7c741695e9b1
> > --- /dev/null
> > +++ b/drivers/crypto/aspeed/Kconfig
> > @@ -0,0 +1,23 @@
> > +config CRYPTO_DEV_ASPEED
> > +	tristate "Support for Aspeed cryptographic engine driver"
> > +	depends on ARCH_ASPEED
> > +	help
> > +	  Hash and Crypto Engine (HACE) is designed to accelerate the
> > +	  throughput of hash data digest, encryption and decryption.
> > +
> > +	  Select y here to have support for the cryptographic driver
> > +	  available on Aspeed SoC.
> > +
> > +config CRYPTO_DEV_ASPEED_HACE_HASH
> > +	bool "Enable Aspeed Hash & Crypto Engine (HACE) hash"
> > +	depends on CRYPTO_DEV_ASPEED
> > +	select CRYPTO_ENGINE
> > +	select CRYPTO_SHA1
> > +	select CRYPTO_SHA256
> > +	select CRYPTO_SHA512
> > +	select CRYPTO_HMAC
> > +	help
> > +	  Select here to enable Aspeed Hash & Crypto Engine (HACE)
> > +	  hash driver.
> > +	  Supports multiple message digest standards, including
> > +	  SHA-1, SHA-224, SHA-256, SHA-384, SHA-512, and so on.
> > diff --git a/drivers/crypto/aspeed/Makefile
> b/drivers/crypto/aspeed/Makefile
> > new file mode 100644
> > index 000000000000..8bc8d4fed5a9
> > --- /dev/null
> > +++ b/drivers/crypto/aspeed/Makefile
> > @@ -0,0 +1,6 @@
> > +obj-$(CONFIG_CRYPTO_DEV_ASPEED) += aspeed_crypto.o
> > +aspeed_crypto-objs := aspeed-hace.o \
> > +		      $(hace-hash-y)
> > +
> > +obj-$(CONFIG_CRYPTO_DEV_ASPEED_HACE_HASH) += aspeed-hace-hash.o
> > +hace-hash-$(CONFIG_CRYPTO_DEV_ASPEED_HACE_HASH) :=
> aspeed-hace-hash.o
> > diff --git a/drivers/crypto/aspeed/aspeed-hace-hash.c
> b/drivers/crypto/aspeed/aspeed-hace-hash.c
> > new file mode 100644
> > index 000000000000..32631cf03244
> > --- /dev/null
> > +++ b/drivers/crypto/aspeed/aspeed-hace-hash.c
> > @@ -0,0 +1,1428 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (c) 2021 Aspeed Technology Inc.
> > + */
> > +
> > +#include "aspeed-hace.h"
> > +
> > +#ifdef ASPEED_AHASH_DEBUG
> > +#define AHASH_DBG(h, fmt, ...)	\
> > +	dev_info((h)->dev, "%s() " fmt, __func__, ##__VA_ARGS__)
> > +#else
> > +#define AHASH_DBG(h, fmt, ...)	\
> > +	dev_dbg((h)->dev, "%s() " fmt, __func__, ##__VA_ARGS__)
> > +#endif
> 
> Can this be defined in Kconfig (CONFIG_ASPEED_AHASH_DEBUG) like some
> other drivers?

Sure, I'll revise it in next patch.

> > +
> > +/* Initialization Vectors for SHA-family */
> > +static const u32 sha1_iv[8] = {
> > +	cpu_to_be32(SHA1_H0), cpu_to_be32(SHA1_H1),
> > +	cpu_to_be32(SHA1_H2), cpu_to_be32(SHA1_H3),
> > +	cpu_to_be32(SHA1_H4), 0, 0, 0
> > +};
> > +
> > +static const u32 sha224_iv[8] = {
> > +	cpu_to_be32(SHA224_H0), cpu_to_be32(SHA224_H1),
> > +	cpu_to_be32(SHA224_H2), cpu_to_be32(SHA224_H3),
> > +	cpu_to_be32(SHA224_H4), cpu_to_be32(SHA224_H5),
> > +	cpu_to_be32(SHA224_H6), cpu_to_be32(SHA224_H7),
> > +};
> > +
> > +static const u32 sha256_iv[8] = {
> > +	cpu_to_be32(SHA256_H0), cpu_to_be32(SHA256_H1),
> > +	cpu_to_be32(SHA256_H2), cpu_to_be32(SHA256_H3),
> > +	cpu_to_be32(SHA256_H4), cpu_to_be32(SHA256_H5),
> > +	cpu_to_be32(SHA256_H6), cpu_to_be32(SHA256_H7),
> > +};
> 
> static const __be32 since using cpu_to_be32()?

Okay, I'll revise it in next patch.

> > +
> > +static const u64 sha384_iv[8] = {
> > +	cpu_to_be64(SHA384_H0), cpu_to_be64(SHA384_H1),
> > +	cpu_to_be64(SHA384_H2), cpu_to_be64(SHA384_H3),
> > +	cpu_to_be64(SHA384_H4), cpu_to_be64(SHA384_H5),
> > +	cpu_to_be64(SHA384_H6), cpu_to_be64(SHA384_H7)
> > +};
> > +
> > +static const u64 sha512_iv[8] = {
> > +	cpu_to_be64(SHA512_H0), cpu_to_be64(SHA512_H1),
> > +	cpu_to_be64(SHA512_H2), cpu_to_be64(SHA512_H3),
> > +	cpu_to_be64(SHA512_H4), cpu_to_be64(SHA512_H5),
> > +	cpu_to_be64(SHA512_H6), cpu_to_be64(SHA512_H7)
> > +};
> 
> same as above, static const __be64 since using cpu_to_be64()?

Same.

> > +
> > +static const u32 sha512_224_iv[16] = {
> > +	cpu_to_be32(0xC8373D8CUL), cpu_to_be32(0xA24D5419UL),
> > +	cpu_to_be32(0x6699E173UL), cpu_to_be32(0xD6D4DC89UL),
> > +	cpu_to_be32(0xAEB7FA1DUL), cpu_to_be32(0x829CFF32UL),
> > +	cpu_to_be32(0x14D59D67UL), cpu_to_be32(0xCF9F2F58UL),
> > +	cpu_to_be32(0x692B6D0FUL), cpu_to_be32(0xA84DD47BUL),
> > +	cpu_to_be32(0x736FE377UL), cpu_to_be32(0x4289C404UL),
> > +	cpu_to_be32(0xA8859D3FUL), cpu_to_be32(0xC8361D6AUL),
> > +	cpu_to_be32(0xADE61211UL), cpu_to_be32(0xA192D691UL)
> > +};
> > +
> > +static const u32 sha512_256_iv[16] = {
> > +	cpu_to_be32(0x94213122UL), cpu_to_be32(0x2CF72BFCUL),
> > +	cpu_to_be32(0xA35F559FUL), cpu_to_be32(0xC2644CC8UL),
> > +	cpu_to_be32(0x6BB89323UL), cpu_to_be32(0x51B1536FUL),
> > +	cpu_to_be32(0x19773896UL), cpu_to_be32(0xBDEA4059UL),
> > +	cpu_to_be32(0xE23E2896UL), cpu_to_be32(0xE3FF8EA8UL),
> > +	cpu_to_be32(0x251E5EBEUL), cpu_to_be32(0x92398653UL),
> > +	cpu_to_be32(0xFC99012BUL), cpu_to_be32(0xAAB8852CUL),
> > +	cpu_to_be32(0xDC2DB70EUL), cpu_to_be32(0xA22CC581UL)
> > +};
> > +
> > +static void aspeed_ahash_iV(struct aspeed_sham_reqctx *rctx)
> > +{
> > +	if (rctx->flags & SHA_FLAGS_SHA1)
> > +		memcpy(rctx->digest, sha1_iv, 32);
> > +	else if (rctx->flags & SHA_FLAGS_SHA224)
> > +		memcpy(rctx->digest, sha224_iv, 32);
> > +	else if (rctx->flags & SHA_FLAGS_SHA256)
> > +		memcpy(rctx->digest, sha256_iv, 32);
> > +	else if (rctx->flags & SHA_FLAGS_SHA384)
> > +		memcpy(rctx->digest, sha384_iv, 64);
> > +	else if (rctx->flags & SHA_FLAGS_SHA512)
> > +		memcpy(rctx->digest, sha512_iv, 64);
> > +	else if (rctx->flags & SHA_FLAGS_SHA512_224)
> > +		memcpy(rctx->digest, sha512_224_iv, 64);
> > +	else if (rctx->flags & SHA_FLAGS_SHA512_256)
> > +		memcpy(rctx->digest, sha512_256_iv, 64);
> > +}
> 
> Can use the "digsize" from reqctx to memcpy() instead lots of if..else
> conditionals for every request?

> (Copy from another mail thread)
> Sorry, meant pre-initialized ivsize not digsize, which could be in alg wrapper structure (aspeed_hace_alg).

Are you suggesting adding a member named "ivsize" in structure aspeed_hace_alg, and pre-initialized?
Example:
struct aspeed_hace_alg aspeed_ahash_algs[] = {
	{
		.ivsize = 32;
		.alg.ahash = {...};	// aspeed-sha1
	};
	{
		.ivsize = 32;
		.alg.ahash = {...};	// aspeed-sha224
	};
	...
};

[...]

> > +static int aspeed_sham_cra_init_alg(struct crypto_tfm *tfm,
> > +				    const char *alg_base)
> > +{
> > +	struct ahash_alg *alg = __crypto_ahash_alg(tfm->__crt_alg);
> > +	struct aspeed_sham_ctx *tctx = crypto_tfm_ctx(tfm);
> > +	struct aspeed_hace_dev *hace_dev = tctx->hace_dev;
> > +	struct aspeed_hace_alg *ast_alg;
> > +
> > +	ast_alg = container_of(alg, struct aspeed_hace_alg, alg.ahash);
> > +	tctx->hace_dev = ast_alg->hace_dev;
> > +	tctx->flags = 0;
> > +
> > +	crypto_ahash_set_reqsize(__crypto_ahash_cast(tfm),
> > +				 sizeof(struct aspeed_sham_reqctx));
> > +
> > +	if (alg_base) {
> > +		struct aspeed_sha_hmac_ctx *bctx = tctx->base;
> > +
> > +		tctx->flags |= SHA_FLAGS_HMAC;
> > +		bctx->shash = crypto_alloc_shash(alg_base, 0,
> > +						 CRYPTO_ALG_NEED_FALLBACK);
> > +		if (IS_ERR(bctx->shash)) {
> > +			dev_warn(hace_dev->dev,
> > +				 "base driver '%s' could not be loaded.\n",
> > +				 alg_base);
> > +			return PTR_ERR(bctx->shash);
> > +		}
> > +	}
> > +
> > +	tctx->enginectx.op.do_one_request = aspeed_ahash_do_request;
> > +	tctx->enginectx.op.prepare_request = aspeed_ahash_prepare_request;
> > +	tctx->enginectx.op.unprepare_request = NULL;
> > +
> > +	return 0;
> > +}
> > +
> > +static int aspeed_sham_cra_init(struct crypto_tfm *tfm)
> > +{
> > +	return aspeed_sham_cra_init_alg(tfm, NULL);
> > +}
> > +
> > +static int aspeed_sham_cra_sha1_init(struct crypto_tfm *tfm)
> > +{
> > +	return aspeed_sham_cra_init_alg(tfm, "sha1");
> > +}
> > +
> > +static int aspeed_sham_cra_sha224_init(struct crypto_tfm *tfm)
> > +{
> > +	return aspeed_sham_cra_init_alg(tfm, "sha224");
> > +}
> > +
> > +static int aspeed_sham_cra_sha256_init(struct crypto_tfm *tfm)
> > +{
> > +	return aspeed_sham_cra_init_alg(tfm, "sha256");
> > +}
> > +
> > +static int aspeed_sham_cra_sha384_init(struct crypto_tfm *tfm)
> > +{
> > +	return aspeed_sham_cra_init_alg(tfm, "sha384");
> > +}
> > +
> > +static int aspeed_sham_cra_sha512_init(struct crypto_tfm *tfm)
> > +{
> > +	return aspeed_sham_cra_init_alg(tfm, "sha512");
> > +}
> > +
> > +static int aspeed_sham_cra_sha512_224_init(struct crypto_tfm *tfm)
> > +{
> > +	return aspeed_sham_cra_init_alg(tfm, "sha512_224");
> > +}
> > +
> > +static int aspeed_sham_cra_sha512_256_init(struct crypto_tfm *tfm)
> > +{
> > +	return aspeed_sham_cra_init_alg(tfm, "sha512_256");
> > +}
> 
> Looks like most of these cra_init callback functions are trying to
> distinguish hmac from sha algs by passing "alg_base" arg. Can collapse
> the logic in aspeed_sham_cra_init_alg() itself and storing alg_base in
> instances of aspeed_hace_alg.
> 
> 	if (ast_alg->ahash.setkey()) {
> 		/* hmac related logic */
> 		bctx->shash = crypto_alloc_shash(ast_alg->alg_base, 0,
> 			CRYPTO_ALG_NEED_FALLBACK);
> 		...

Thanks for your suggestion! It will simplify these cra_init() functions.
I'll revise it for next patch.

[...]




[Index of Archives]     [Kernel]     [Gnu Classpath]     [Gnu Crypto]     [DM Crypt]     [Netfilter]     [Bugtraq]
  Powered by Linux