[PATCH V2 0/4] Add Xilinx's ZynqMP AES driver support

[PATCH V2 0/4] Add Xilinx's ZynqMP AES driver support

[Kalyani Akula]

This patch set adds support for
- dt-binding docs for Xilinx ZynqMP AES driver
- Adds device tree node for ZynqMP AES driver
- Adds communication layer support for aes in zynqmp.c
- Adds Xilinx ZynqMP driver for AES Algorithm

V2 Changes :
- Converted RFC PATCH to PATCH
- Removed ALG_SET_KEY_TYPE that was added to support keytype
  attribute. Taken using setkey interface.
- Removed deprecated BLKCIPHER in Kconfig
- Erased Key/IV from the buffer.
- Renamed zynqmp-aes driver to zynqmp-aes-gcm.
- Addressed few other review comments

Kalyani Akula (4):
  dt-bindings: crypto: Add bindings for ZynqMP AES driver
  ARM64: zynqmp: Add Xilinix AES node.
  firmware: xilinx: Add ZynqMP aes API for AES functionality
  crypto: Add Xilinx AES driver

 .../devicetree/bindings/crypto/xlnx,zynqmp-aes.txt |  12 +
 arch/arm64/boot/dts/xilinx/zynqmp.dtsi             |   4 +
 drivers/crypto/Kconfig                             |  11 +
 drivers/crypto/Makefile                            |   1 +
 drivers/crypto/zynqmp-aes-gcm.c                    | 297 +++++++++++++++++++++
 drivers/firmware/xilinx/zynqmp.c                   |  23 ++
 include/linux/firmware/xlnx-zynqmp.h               |   2 +
 7 files changed, 350 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/crypto/xlnx,zynqmp-aes.txt
 create mode 100644 drivers/crypto/zynqmp-aes-gcm.c

-- 
1.8.3.1

[PATCH V2 1/4] dt-bindings: crypto: Add bindings for ZynqMP AES driver

[Kalyani Akula]

Add documentation to describe Xilinx ZynqMP AES driver
bindings.

Signed-off-by: Kalyani Akula <kalyani.akula@xilinx.com>
---
 Documentation/devicetree/bindings/crypto/xlnx,zynqmp-aes.txt | 12 ++++++++++++
 1 file changed, 12 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/crypto/xlnx,zynqmp-aes.txt

diff --git a/Documentation/devicetree/bindings/crypto/xlnx,zynqmp-aes.txt b/Documentation/devicetree/bindings/crypto/xlnx,zynqmp-aes.txt
new file mode 100644
index 0000000..226bfb9
--- /dev/null
+++ b/Documentation/devicetree/bindings/crypto/xlnx,zynqmp-aes.txt
@@ -0,0 +1,12 @@
+Xilinx ZynqMP AES hw acceleration support
+
+The ZynqMP PS-AES hw accelerator is used to encrypt/decrypt
+the given user data.
+
+Required properties:
+- compatible: should contain "xlnx,zynqmp-aes"
+
+Example:
+	zynqmp_aes {
+		compatible = "xlnx,zynqmp-aes";
+	};
-- 
1.8.3.1

[PATCH V2 2/4] ARM64: zynqmp: Add Xilinix AES node.

[Kalyani Akula]

This patch adds a AES DT node for Xilinx ZynqMP SoC.

Signed-off-by: Kalyani Akula <kalyani.akula@xilinx.com>
---
 arch/arm64/boot/dts/xilinx/zynqmp.dtsi | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
index 9aa6734..b41febc 100644
--- a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
+++ b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
@@ -124,6 +124,10 @@
 			     <1 10 0xf08>;
 	};
 
+	xlnx_aes: zynqmp_aes {
+		compatible = "xlnx,zynqmp-aes";
+	};
+
 	amba_apu: amba-apu@0 {
 		compatible = "simple-bus";
 		#address-cells = <2>;
-- 
1.8.3.1

[PATCH V2 3/4] firmware: xilinx: Add ZynqMP aes API for AES functionality

[Kalyani Akula]

Add ZynqMP firmware AES API to perform encryption/decryption
of given data.

Signed-off-by: Kalyani Akula <kalyani.akula@xilinx.com>
---
 drivers/firmware/xilinx/zynqmp.c     | 23 +++++++++++++++++++++++
 include/linux/firmware/xlnx-zynqmp.h |  2 ++
 2 files changed, 25 insertions(+)

diff --git a/drivers/firmware/xilinx/zynqmp.c b/drivers/firmware/xilinx/zynqmp.c
index fd3d837..74f3354 100644
--- a/drivers/firmware/xilinx/zynqmp.c
+++ b/drivers/firmware/xilinx/zynqmp.c
@@ -663,6 +663,28 @@ static int zynqmp_pm_set_requirement(const u32 node, const u32 capabilities,
 	return zynqmp_pm_invoke_fn(PM_SET_REQUIREMENT, node, capabilities,
 				   qos, ack, NULL);
 }
+/**
+ * zynqmp_pm_aes - Access AES hardware to encrypt/decrypt the data using
+ * AES-GCM core.
+ * @address:	Address of the AesParams structure.
+ * @out:	Returned output value
+ *
+ * Return:	Returns status, either success or error code.
+ */
+static int zynqmp_pm_aes_engine(const u64 address, u32 *out)
+{
+	u32 ret_payload[PAYLOAD_ARG_CNT];
+	int ret;
+
+	if (!out)
+		return -EINVAL;
+
+	ret = zynqmp_pm_invoke_fn(PM_SECURE_AES, upper_32_bits(address),
+				  lower_32_bits(address),
+				  0, 0, ret_payload);
+	*out = ret_payload[1];
+	return ret;
+}
 
 static const struct zynqmp_eemi_ops eemi_ops = {
 	.get_api_version = zynqmp_pm_get_api_version,
@@ -687,6 +709,7 @@ static int zynqmp_pm_set_requirement(const u32 node, const u32 capabilities,
 	.set_requirement = zynqmp_pm_set_requirement,
 	.fpga_load = zynqmp_pm_fpga_load,
 	.fpga_get_status = zynqmp_pm_fpga_get_status,
+	.aes = zynqmp_pm_aes_engine,
 };
 
 /**
diff --git a/include/linux/firmware/xlnx-zynqmp.h b/include/linux/firmware/xlnx-zynqmp.h
index 778abbb..508edd7 100644
--- a/include/linux/firmware/xlnx-zynqmp.h
+++ b/include/linux/firmware/xlnx-zynqmp.h
@@ -77,6 +77,7 @@ enum pm_api_id {
 	PM_CLOCK_GETRATE,
 	PM_CLOCK_SETPARENT,
 	PM_CLOCK_GETPARENT,
+	PM_SECURE_AES = 47,
 };
 
 /* PMU-FW return status codes */
@@ -294,6 +295,7 @@ struct zynqmp_eemi_ops {
 			       const u32 capabilities,
 			       const u32 qos,
 			       const enum zynqmp_pm_request_ack ack);
+	int (*aes)(const u64 address, u32 *out);
 };
 
 int zynqmp_pm_invoke_fn(u32 pm_api_id, u32 arg0, u32 arg1,
-- 
1.8.3.1

[PATCH V2 4/4] crypto: Add Xilinx AES driver

[Kalyani Akula]

This patch adds AES driver support for the Xilinx
ZynqMP SoC.

Signed-off-by: Kalyani Akula <kalyani.akula@xilinx.com>
---
 drivers/crypto/Kconfig          |  11 ++
 drivers/crypto/Makefile         |   1 +
 drivers/crypto/zynqmp-aes-gcm.c | 297 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 309 insertions(+)
 create mode 100644 drivers/crypto/zynqmp-aes-gcm.c

diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
index 603413f..a0d058a 100644
--- a/drivers/crypto/Kconfig
+++ b/drivers/crypto/Kconfig
@@ -677,6 +677,17 @@ config CRYPTO_DEV_ROCKCHIP
 	  This driver interfaces with the hardware crypto accelerator.
 	  Supporting cbc/ecb chainmode, and aes/des/des3_ede cipher mode.
 
+config CRYPTO_DEV_ZYNQMP_AES
+	tristate "Support for Xilinx ZynqMP AES hw accelerator"
+	depends on ARCH_ZYNQMP || COMPILE_TEST
+	select CRYPTO_AES
+	select CRYPTO_SKCIPHER
+	help
+	  Xilinx ZynqMP has AES-GCM engine used for symmetric key
+	  encryption and decryption. This driver interfaces with AES hw
+	  accelerator. Select this if you want to use the ZynqMP module
+	  for AES algorithms.
+
 config CRYPTO_DEV_MEDIATEK
 	tristate "MediaTek's EIP97 Cryptographic Engine driver"
 	depends on (ARM && ARCH_MEDIATEK) || COMPILE_TEST
diff --git a/drivers/crypto/Makefile b/drivers/crypto/Makefile
index afc4753..c99663a 100644
--- a/drivers/crypto/Makefile
+++ b/drivers/crypto/Makefile
@@ -48,3 +48,4 @@ obj-$(CONFIG_CRYPTO_DEV_BCM_SPU) += bcm/
 obj-$(CONFIG_CRYPTO_DEV_SAFEXCEL) += inside-secure/
 obj-$(CONFIG_CRYPTO_DEV_ARTPEC6) += axis/
 obj-y += hisilicon/
+obj-$(CONFIG_CRYPTO_DEV_ZYNQMP_AES) += zynqmp-aes-gcm.o
diff --git a/drivers/crypto/zynqmp-aes-gcm.c b/drivers/crypto/zynqmp-aes-gcm.c
new file mode 100644
index 0000000..d65f038
--- /dev/null
+++ b/drivers/crypto/zynqmp-aes-gcm.c
@@ -0,0 +1,297 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Xilinx ZynqMP AES Driver.
+ * Copyright (c) 2019 Xilinx Inc.
+ */
+
+#include <crypto/aes.h>
+#include <crypto/scatterwalk.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/scatterlist.h>
+#include <linux/firmware/xlnx-zynqmp.h>
+
+#define ZYNQMP_AES_IV_SIZE			12
+#define ZYNQMP_AES_GCM_SIZE			16
+#define ZYNQMP_AES_KEY_SIZE			32
+
+#define ZYNQMP_AES_DECRYPT			0
+#define ZYNQMP_AES_ENCRYPT			1
+
+#define ZYNQMP_AES_KUP_KEY			0
+#define ZYNQMP_AES_DEVICE_KEY			1
+#define ZYNQMP_AES_PUF_KEY			2
+
+#define ZYNQMP_AES_GCM_TAG_MISMATCH_ERR		0x01
+#define ZYNQMP_AES_SIZE_ERR			0x06
+#define ZYNQMP_AES_WRONG_KEY_SRC_ERR		0x13
+#define ZYNQMP_AES_PUF_NOT_PROGRAMMED		0xE300
+
+#define ZYNQMP_AES_BLOCKSIZE			0x04
+
+static const struct zynqmp_eemi_ops *eemi_ops;
+struct zynqmp_aes_dev *aes_dd;
+
+struct zynqmp_aes_dev {
+	struct device *dev;
+};
+
+struct zynqmp_aes_op {
+	struct zynqmp_aes_dev *dd;
+	void *src;
+	void *dst;
+	int len;
+	u8 key[ZYNQMP_AES_KEY_SIZE];
+	u8 *iv;
+	u32 keylen;
+	u32 keytype;
+};
+
+struct zynqmp_aes_data {
+	u64 src;
+	u64 iv;
+	u64 key;
+	u64 dst;
+	u64 size;
+	u64 optype;
+	u64 keysrc;
+};
+
+static int zynqmp_setkey_blk(struct crypto_tfm *tfm, const u8 *key,
+			     unsigned int len)
+{
+	struct zynqmp_aes_op *op = crypto_tfm_ctx(tfm);
+
+	if (((len != 1) && (len !=  ZYNQMP_AES_KEY_SIZE)) || (!key))
+		return -EINVAL;
+
+	if (len == 1) {
+		op->keytype = *key;
+
+		if ((op->keytype < ZYNQMP_AES_KUP_KEY) ||
+			(op->keytype > ZYNQMP_AES_PUF_KEY))
+			return -EINVAL;
+
+	} else if (len == ZYNQMP_AES_KEY_SIZE) {
+		op->keytype = ZYNQMP_AES_KUP_KEY;
+		op->keylen = len;
+		memcpy(op->key, key, len);
+	}
+
+	return 0;
+}
+
+static int zynqmp_aes_xcrypt(struct blkcipher_desc *desc,
+			     struct scatterlist *dst,
+			     struct scatterlist *src,
+			     unsigned int nbytes,
+			     unsigned int flags)
+{
+	struct zynqmp_aes_op *op = crypto_blkcipher_ctx(desc->tfm);
+	struct zynqmp_aes_dev *dd = aes_dd;
+	int err, ret, copy_bytes, src_data = 0, dst_data = 0;
+	dma_addr_t dma_addr, dma_addr_buf;
+	struct zynqmp_aes_data *abuf;
+	struct blkcipher_walk walk;
+	unsigned int data_size;
+	size_t dma_size;
+	char *kbuf;
+
+	if (!eemi_ops->aes)
+		return -ENOTSUPP;
+
+	if (op->keytype == ZYNQMP_AES_KUP_KEY)
+		dma_size = nbytes + ZYNQMP_AES_KEY_SIZE
+			+ ZYNQMP_AES_IV_SIZE;
+	else
+		dma_size = nbytes + ZYNQMP_AES_IV_SIZE;
+
+	kbuf = dma_alloc_coherent(dd->dev, dma_size, &dma_addr, GFP_KERNEL);
+	if (!kbuf)
+		return -ENOMEM;
+
+	abuf = dma_alloc_coherent(dd->dev, sizeof(struct zynqmp_aes_data),
+				  &dma_addr_buf, GFP_KERNEL);
+	if (!abuf) {
+		dma_free_coherent(dd->dev, dma_size, kbuf, dma_addr);
+		return -ENOMEM;
+	}
+
+	data_size = nbytes;
+	blkcipher_walk_init(&walk, dst, src, data_size);
+	err = blkcipher_walk_virt(desc, &walk);
+	op->iv = walk.iv;
+
+	while ((nbytes = walk.nbytes)) {
+		op->src = walk.src.virt.addr;
+		memcpy(kbuf + src_data, op->src, nbytes);
+		src_data = src_data + nbytes;
+		nbytes &= (ZYNQMP_AES_BLOCKSIZE - 1);
+		err = blkcipher_walk_done(desc, &walk, nbytes);
+	}
+	memcpy(kbuf + data_size, op->iv, ZYNQMP_AES_IV_SIZE);
+	abuf->src = dma_addr;
+	abuf->dst = dma_addr;
+	abuf->iv = abuf->src + data_size;
+	abuf->size = data_size - ZYNQMP_AES_GCM_SIZE;
+	abuf->optype = flags;
+	abuf->keysrc = op->keytype;
+
+	if (op->keytype == ZYNQMP_AES_KUP_KEY) {
+		memcpy(kbuf + data_size + ZYNQMP_AES_IV_SIZE,
+		       op->key, ZYNQMP_AES_KEY_SIZE);
+
+		abuf->key = abuf->src + data_size + ZYNQMP_AES_IV_SIZE;
+	} else {
+		abuf->key = 0;
+	}
+	eemi_ops->aes(dma_addr_buf, &ret);
+
+	if (ret != 0) {
+		switch (ret) {
+		case ZYNQMP_AES_GCM_TAG_MISMATCH_ERR:
+			dev_err(dd->dev, "ERROR: Gcm Tag mismatch\n\r");
+			break;
+		case ZYNQMP_AES_SIZE_ERR:
+			dev_err(dd->dev, "ERROR : Non word aligned data\n\r");
+			break;
+		case ZYNQMP_AES_WRONG_KEY_SRC_ERR:
+			dev_err(dd->dev, "ERROR: Wrong KeySrc, enable secure mode\n\r");
+			break;
+		case ZYNQMP_AES_PUF_NOT_PROGRAMMED:
+			dev_err(dd->dev, "ERROR: PUF is not registered\r\n");
+			break;
+		default:
+			dev_err(dd->dev, "ERROR: Invalid");
+			break;
+		}
+		goto END;
+	}
+	if (flags)
+		copy_bytes = data_size;
+	else
+		copy_bytes = data_size - ZYNQMP_AES_GCM_SIZE;
+
+	blkcipher_walk_init(&walk, dst, src, copy_bytes);
+	err = blkcipher_walk_virt(desc, &walk);
+
+	while ((nbytes = walk.nbytes)) {
+		memcpy(walk.dst.virt.addr, kbuf + dst_data, nbytes);
+		dst_data = dst_data + nbytes;
+		nbytes &= (ZYNQMP_AES_BLOCKSIZE - 1);
+		err = blkcipher_walk_done(desc, &walk, nbytes);
+	}
+END:
+	memset(kbuf, 0, dma_size);
+	memset(abuf, 0, sizeof(struct zynqmp_aes_data));
+	dma_free_coherent(dd->dev, dma_size, kbuf, dma_addr);
+	dma_free_coherent(dd->dev, sizeof(struct zynqmp_aes_data),
+			  abuf, dma_addr_buf);
+	return err;
+}
+
+static int zynqmp_aes_decrypt(struct blkcipher_desc *desc,
+			      struct scatterlist *dst,
+			      struct scatterlist *src,
+			      unsigned int nbytes)
+{
+	return zynqmp_aes_xcrypt(desc, dst, src, nbytes, ZYNQMP_AES_DECRYPT);
+}
+
+static int zynqmp_aes_encrypt(struct blkcipher_desc *desc,
+			      struct scatterlist *dst,
+			      struct scatterlist *src,
+			      unsigned int nbytes)
+{
+	return zynqmp_aes_xcrypt(desc, dst, src, nbytes, ZYNQMP_AES_ENCRYPT);
+}
+
+static struct crypto_alg zynqmp_alg = {
+	.cra_name		=	"xilinx-zynqmp-aes",
+	.cra_driver_name	=	"zynqmp-aes-gcm",
+	.cra_priority		=	400,
+	.cra_flags		=	CRYPTO_ALG_TYPE_BLKCIPHER |
+					CRYPTO_ALG_KERN_DRIVER_ONLY,
+	.cra_blocksize		=	ZYNQMP_AES_BLOCKSIZE,
+	.cra_ctxsize		=	sizeof(struct zynqmp_aes_op),
+	.cra_alignmask		=	15,
+	.cra_type		=	&crypto_blkcipher_type,
+	.cra_module		=	THIS_MODULE,
+	.cra_u			=	{
+	.blkcipher	=	{
+			.min_keysize	=	0,
+			.max_keysize	=	ZYNQMP_AES_KEY_SIZE,
+			.setkey		=	zynqmp_setkey_blk,
+			.encrypt	=	zynqmp_aes_encrypt,
+			.decrypt	=	zynqmp_aes_decrypt,
+			.ivsize		=	ZYNQMP_AES_IV_SIZE,
+		}
+	}
+};
+
+static const struct of_device_id zynqmp_aes_dt_ids[] = {
+	{ .compatible = "xlnx,zynqmp-aes" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, zynqmp_aes_dt_ids);
+
+static int zynqmp_aes_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	int ret;
+
+	eemi_ops = zynqmp_pm_get_eemi_ops();
+
+	if (IS_ERR(eemi_ops))
+		return PTR_ERR(eemi_ops);
+
+	aes_dd = devm_kzalloc(dev, sizeof(*aes_dd), GFP_KERNEL);
+	if (!aes_dd)
+		return -ENOMEM;
+
+	aes_dd->dev = dev;
+	platform_set_drvdata(pdev, aes_dd);
+
+	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(44));
+	if (ret < 0) {
+		dev_err(dev, "no usable DMA configuration");
+		return ret;
+	}
+
+	ret = crypto_register_alg(&zynqmp_alg);
+	if (ret)
+		goto err_algs;
+
+	dev_info(dev, "AES Successfully Registered\n\r");
+	return 0;
+
+err_algs:
+	return ret;
+}
+
+static int zynqmp_aes_remove(struct platform_device *pdev)
+{
+	aes_dd = platform_get_drvdata(pdev);
+	if (!aes_dd)
+		return -ENODEV;
+	crypto_unregister_alg(&zynqmp_alg);
+
+	return 0;
+}
+
+static struct platform_driver xilinx_aes_driver = {
+	.probe = zynqmp_aes_probe,
+	.remove = zynqmp_aes_remove,
+	.driver = {
+		.name = "zynqmp_aes",
+		.of_match_table = of_match_ptr(zynqmp_aes_dt_ids),
+	},
+};
+
+module_platform_driver(xilinx_aes_driver);
+
+MODULE_DESCRIPTION("Xilinx ZynqMP AES hw acceleration support.");
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Nava kishore Manne <nava.manne@xilinx.com>");
+MODULE_AUTHOR("Kalyani Akula <kalyani.akula@xilinx.com>");
-- 
1.8.3.1

Re: [PATCH V2 4/4] crypto: Add Xilinx AES driver

[Corentin Labbe]

On Sun, Sep 01, 2019 at 07:24:58PM +0530, Kalyani Akula wrote:
> This patch adds AES driver support for the Xilinx
> ZynqMP SoC.
> 
> Signed-off-by: Kalyani Akula <kalyani.akula@xilinx.com>
> ---

Hello

I have some comment below

>  drivers/crypto/Kconfig          |  11 ++
>  drivers/crypto/Makefile         |   1 +
>  drivers/crypto/zynqmp-aes-gcm.c | 297 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 309 insertions(+)
>  create mode 100644 drivers/crypto/zynqmp-aes-gcm.c
> 
> diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
> index 603413f..a0d058a 100644
> --- a/drivers/crypto/Kconfig
> +++ b/drivers/crypto/Kconfig
> @@ -677,6 +677,17 @@ config CRYPTO_DEV_ROCKCHIP
>  	  This driver interfaces with the hardware crypto accelerator.
>  	  Supporting cbc/ecb chainmode, and aes/des/des3_ede cipher mode.
>  
> +config CRYPTO_DEV_ZYNQMP_AES
> +	tristate "Support for Xilinx ZynqMP AES hw accelerator"
> +	depends on ARCH_ZYNQMP || COMPILE_TEST
> +	select CRYPTO_AES
> +	select CRYPTO_SKCIPHER
> +	help
> +	  Xilinx ZynqMP has AES-GCM engine used for symmetric key
> +	  encryption and decryption. This driver interfaces with AES hw
> +	  accelerator. Select this if you want to use the ZynqMP module
> +	  for AES algorithms.
> +
>  config CRYPTO_DEV_MEDIATEK
>  	tristate "MediaTek's EIP97 Cryptographic Engine driver"
>  	depends on (ARM && ARCH_MEDIATEK) || COMPILE_TEST
> diff --git a/drivers/crypto/Makefile b/drivers/crypto/Makefile
> index afc4753..c99663a 100644
> --- a/drivers/crypto/Makefile
> +++ b/drivers/crypto/Makefile
> @@ -48,3 +48,4 @@ obj-$(CONFIG_CRYPTO_DEV_BCM_SPU) += bcm/
>  obj-$(CONFIG_CRYPTO_DEV_SAFEXCEL) += inside-secure/
>  obj-$(CONFIG_CRYPTO_DEV_ARTPEC6) += axis/
>  obj-y += hisilicon/
> +obj-$(CONFIG_CRYPTO_DEV_ZYNQMP_AES) += zynqmp-aes-gcm.o
> diff --git a/drivers/crypto/zynqmp-aes-gcm.c b/drivers/crypto/zynqmp-aes-gcm.c
> new file mode 100644
> index 0000000..d65f038
> --- /dev/null
> +++ b/drivers/crypto/zynqmp-aes-gcm.c
> @@ -0,0 +1,297 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Xilinx ZynqMP AES Driver.
> + * Copyright (c) 2019 Xilinx Inc.
> + */
> +
> +#include <crypto/aes.h>
> +#include <crypto/scatterwalk.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/scatterlist.h>
> +#include <linux/firmware/xlnx-zynqmp.h>
> +
> +#define ZYNQMP_AES_IV_SIZE			12
> +#define ZYNQMP_AES_GCM_SIZE			16
> +#define ZYNQMP_AES_KEY_SIZE			32
> +
> +#define ZYNQMP_AES_DECRYPT			0
> +#define ZYNQMP_AES_ENCRYPT			1
> +
> +#define ZYNQMP_AES_KUP_KEY			0
> +#define ZYNQMP_AES_DEVICE_KEY			1
> +#define ZYNQMP_AES_PUF_KEY			2
> +
> +#define ZYNQMP_AES_GCM_TAG_MISMATCH_ERR		0x01
> +#define ZYNQMP_AES_SIZE_ERR			0x06
> +#define ZYNQMP_AES_WRONG_KEY_SRC_ERR		0x13
> +#define ZYNQMP_AES_PUF_NOT_PROGRAMMED		0xE300
> +
> +#define ZYNQMP_AES_BLOCKSIZE			0x04
> +
> +static const struct zynqmp_eemi_ops *eemi_ops;
> +struct zynqmp_aes_dev *aes_dd;

I still think that using a global variable for storing device driver data is bad.

> +
> +struct zynqmp_aes_dev {
> +	struct device *dev;
> +};
> +
> +struct zynqmp_aes_op {
> +	struct zynqmp_aes_dev *dd;
> +	void *src;
> +	void *dst;
> +	int len;
> +	u8 key[ZYNQMP_AES_KEY_SIZE];
> +	u8 *iv;
> +	u32 keylen;
> +	u32 keytype;
> +};
> +
> +struct zynqmp_aes_data {
> +	u64 src;
> +	u64 iv;
> +	u64 key;
> +	u64 dst;
> +	u64 size;
> +	u64 optype;
> +	u64 keysrc;
> +};
> +
> +static int zynqmp_setkey_blk(struct crypto_tfm *tfm, const u8 *key,
> +			     unsigned int len)
> +{
> +	struct zynqmp_aes_op *op = crypto_tfm_ctx(tfm);
> +
> +	if (((len != 1) && (len !=  ZYNQMP_AES_KEY_SIZE)) || (!key))

typo, two space

> +		return -EINVAL;
> +
> +	if (len == 1) {
> +		op->keytype = *key;
> +
> +		if ((op->keytype < ZYNQMP_AES_KUP_KEY) ||
> +			(op->keytype > ZYNQMP_AES_PUF_KEY))
> +			return -EINVAL;
> +
> +	} else if (len == ZYNQMP_AES_KEY_SIZE) {
> +		op->keytype = ZYNQMP_AES_KUP_KEY;
> +		op->keylen = len;
> +		memcpy(op->key, key, len);
> +	}
> +
> +	return 0;
> +}

It seems your driver does not support AES keysize of 128/196, you need to fallback in that case.
You need to comment the keylen=1 usecase and use a define for this value.

> +
> +static int zynqmp_aes_xcrypt(struct blkcipher_desc *desc,
> +			     struct scatterlist *dst,
> +			     struct scatterlist *src,
> +			     unsigned int nbytes,
> +			     unsigned int flags)
> +{
> +	struct zynqmp_aes_op *op = crypto_blkcipher_ctx(desc->tfm);
> +	struct zynqmp_aes_dev *dd = aes_dd;
> +	int err, ret, copy_bytes, src_data = 0, dst_data = 0;
> +	dma_addr_t dma_addr, dma_addr_buf;
> +	struct zynqmp_aes_data *abuf;
> +	struct blkcipher_walk walk;
> +	unsigned int data_size;
> +	size_t dma_size;
> +	char *kbuf;
> +
> +	if (!eemi_ops->aes)
> +		return -ENOTSUPP;
> +
> +	if (op->keytype == ZYNQMP_AES_KUP_KEY)
> +		dma_size = nbytes + ZYNQMP_AES_KEY_SIZE
> +			+ ZYNQMP_AES_IV_SIZE;
> +	else
> +		dma_size = nbytes + ZYNQMP_AES_IV_SIZE;
> +
> +	kbuf = dma_alloc_coherent(dd->dev, dma_size, &dma_addr, GFP_KERNEL);
> +	if (!kbuf)
> +		return -ENOMEM;
> +
> +	abuf = dma_alloc_coherent(dd->dev, sizeof(struct zynqmp_aes_data),
> +				  &dma_addr_buf, GFP_KERNEL);
> +	if (!abuf) {
> +		dma_free_coherent(dd->dev, dma_size, kbuf, dma_addr);
> +		return -ENOMEM;
> +	}
> +
> +	data_size = nbytes;
> +	blkcipher_walk_init(&walk, dst, src, data_size);
> +	err = blkcipher_walk_virt(desc, &walk);
> +	op->iv = walk.iv;
> +
> +	while ((nbytes = walk.nbytes)) {
> +		op->src = walk.src.virt.addr;
> +		memcpy(kbuf + src_data, op->src, nbytes);
> +		src_data = src_data + nbytes;
> +		nbytes &= (ZYNQMP_AES_BLOCKSIZE - 1);
> +		err = blkcipher_walk_done(desc, &walk, nbytes);
> +	}
> +	memcpy(kbuf + data_size, op->iv, ZYNQMP_AES_IV_SIZE);
> +	abuf->src = dma_addr;
> +	abuf->dst = dma_addr;
> +	abuf->iv = abuf->src + data_size;
> +	abuf->size = data_size - ZYNQMP_AES_GCM_SIZE;
> +	abuf->optype = flags;
> +	abuf->keysrc = op->keytype;
> +
> +	if (op->keytype == ZYNQMP_AES_KUP_KEY) {
> +		memcpy(kbuf + data_size + ZYNQMP_AES_IV_SIZE,
> +		       op->key, ZYNQMP_AES_KEY_SIZE);
> +
> +		abuf->key = abuf->src + data_size + ZYNQMP_AES_IV_SIZE;
> +	} else {
> +		abuf->key = 0;
> +	}
> +	eemi_ops->aes(dma_addr_buf, &ret);
> +
> +	if (ret != 0) {
> +		switch (ret) {
> +		case ZYNQMP_AES_GCM_TAG_MISMATCH_ERR:
> +			dev_err(dd->dev, "ERROR: Gcm Tag mismatch\n\r");
> +			break;
> +		case ZYNQMP_AES_SIZE_ERR:
> +			dev_err(dd->dev, "ERROR : Non word aligned data\n\r");
> +			break;
> +		case ZYNQMP_AES_WRONG_KEY_SRC_ERR:
> +			dev_err(dd->dev, "ERROR: Wrong KeySrc, enable secure mode\n\r");
> +			break;
> +		case ZYNQMP_AES_PUF_NOT_PROGRAMMED:
> +			dev_err(dd->dev, "ERROR: PUF is not registered\r\n");
> +			break;
> +		default:
> +			dev_err(dd->dev, "ERROR: Invalid");
> +			break;
> +		}
> +		goto END;
> +	}
> +	if (flags)
> +		copy_bytes = data_size;
> +	else
> +		copy_bytes = data_size - ZYNQMP_AES_GCM_SIZE;
> +
> +	blkcipher_walk_init(&walk, dst, src, copy_bytes);
> +	err = blkcipher_walk_virt(desc, &walk);
> +
> +	while ((nbytes = walk.nbytes)) {
> +		memcpy(walk.dst.virt.addr, kbuf + dst_data, nbytes);
> +		dst_data = dst_data + nbytes;
> +		nbytes &= (ZYNQMP_AES_BLOCKSIZE - 1);
> +		err = blkcipher_walk_done(desc, &walk, nbytes);
> +	}
> +END:
> +	memset(kbuf, 0, dma_size);
> +	memset(abuf, 0, sizeof(struct zynqmp_aes_data));
> +	dma_free_coherent(dd->dev, dma_size, kbuf, dma_addr);
> +	dma_free_coherent(dd->dev, sizeof(struct zynqmp_aes_data),
> +			  abuf, dma_addr_buf);
> +	return err;
> +}
> +
> +static int zynqmp_aes_decrypt(struct blkcipher_desc *desc,
> +			      struct scatterlist *dst,
> +			      struct scatterlist *src,
> +			      unsigned int nbytes)
> +{
> +	return zynqmp_aes_xcrypt(desc, dst, src, nbytes, ZYNQMP_AES_DECRYPT);
> +}
> +
> +static int zynqmp_aes_encrypt(struct blkcipher_desc *desc,
> +			      struct scatterlist *dst,
> +			      struct scatterlist *src,
> +			      unsigned int nbytes)
> +{
> +	return zynqmp_aes_xcrypt(desc, dst, src, nbytes, ZYNQMP_AES_ENCRYPT);
> +}
> +
> +static struct crypto_alg zynqmp_alg = {
> +	.cra_name		=	"xilinx-zynqmp-aes",
> +	.cra_driver_name	=	"zynqmp-aes-gcm",
> +	.cra_priority		=	400,
> +	.cra_flags		=	CRYPTO_ALG_TYPE_BLKCIPHER |
> +					CRYPTO_ALG_KERN_DRIVER_ONLY,
> +	.cra_blocksize		=	ZYNQMP_AES_BLOCKSIZE,
> +	.cra_ctxsize		=	sizeof(struct zynqmp_aes_op),
> +	.cra_alignmask		=	15,
> +	.cra_type		=	&crypto_blkcipher_type,
> +	.cra_module		=	THIS_MODULE,
> +	.cra_u			=	{
> +	.blkcipher	=	{
> +			.min_keysize	=	0,

Are you sure to accept this a keysize of 0 ?

Regards

RE: [PATCH V2 4/4] crypto: Add Xilinx AES driver

[Kalyani Akula]

Hi Corentin,

Thanks for the review comments.
Please find my response/queries inline.

> -----Original Message-----
> From: Corentin Labbe <clabbe.montjoie@gmail.com>
> Sent: Monday, September 2, 2019 12:29 PM
> To: Kalyani Akula <kalyania@xilinx.com>
> Cc: herbert@gondor.apana.org.au; kstewart@linuxfoundation.org;
> gregkh@linuxfoundation.org; tglx@linutronix.de; pombredanne@nexb.com;
> linux-crypto@vger.kernel.org; linux-kernel@vger.kernel.org;
> netdev@vger.kernel.org; Kalyani Akula <kalyania@xilinx.com>
> Subject: Re: [PATCH V2 4/4] crypto: Add Xilinx AES driver
> 
> On Sun, Sep 01, 2019 at 07:24:58PM +0530, Kalyani Akula wrote:
> > This patch adds AES driver support for the Xilinx ZynqMP SoC.
> >
> > Signed-off-by: Kalyani Akula <kalyani.akula@xilinx.com>
> > ---
> 
> Hello
> 
> I have some comment below
> 
> >  drivers/crypto/Kconfig          |  11 ++
> >  drivers/crypto/Makefile         |   1 +
> >  drivers/crypto/zynqmp-aes-gcm.c | 297
> > ++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 309 insertions(+)
> >  create mode 100644 drivers/crypto/zynqmp-aes-gcm.c
> >
> > diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig index
> > 603413f..a0d058a 100644
> > --- a/drivers/crypto/Kconfig
> > +++ b/drivers/crypto/Kconfig
> > @@ -677,6 +677,17 @@ config CRYPTO_DEV_ROCKCHIP
> >  	  This driver interfaces with the hardware crypto accelerator.
> >  	  Supporting cbc/ecb chainmode, and aes/des/des3_ede cipher mode.
> >
> > +config CRYPTO_DEV_ZYNQMP_AES
> > +	tristate "Support for Xilinx ZynqMP AES hw accelerator"
> > +	depends on ARCH_ZYNQMP || COMPILE_TEST
> > +	select CRYPTO_AES
> > +	select CRYPTO_SKCIPHER
> > +	help
> > +	  Xilinx ZynqMP has AES-GCM engine used for symmetric key
> > +	  encryption and decryption. This driver interfaces with AES hw
> > +	  accelerator. Select this if you want to use the ZynqMP module
> > +	  for AES algorithms.
> > +
> >  config CRYPTO_DEV_MEDIATEK
> >  	tristate "MediaTek's EIP97 Cryptographic Engine driver"
> >  	depends on (ARM && ARCH_MEDIATEK) || COMPILE_TEST diff --git
> > a/drivers/crypto/Makefile b/drivers/crypto/Makefile index
> > afc4753..c99663a 100644
> > --- a/drivers/crypto/Makefile
> > +++ b/drivers/crypto/Makefile
> > @@ -48,3 +48,4 @@ obj-$(CONFIG_CRYPTO_DEV_BCM_SPU) += bcm/
> >  obj-$(CONFIG_CRYPTO_DEV_SAFEXCEL) += inside-secure/
> >  obj-$(CONFIG_CRYPTO_DEV_ARTPEC6) += axis/  obj-y += hisilicon/
> > +obj-$(CONFIG_CRYPTO_DEV_ZYNQMP_AES) += zynqmp-aes-gcm.o
> > diff --git a/drivers/crypto/zynqmp-aes-gcm.c
> > b/drivers/crypto/zynqmp-aes-gcm.c new file mode 100644 index
> > 0000000..d65f038
> > --- /dev/null
> > +++ b/drivers/crypto/zynqmp-aes-gcm.c
> > @@ -0,0 +1,297 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Xilinx ZynqMP AES Driver.
> > + * Copyright (c) 2019 Xilinx Inc.
> > + */
> > +
> > +#include <crypto/aes.h>
> > +#include <crypto/scatterwalk.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of_device.h>
> > +#include <linux/scatterlist.h>
> > +#include <linux/firmware/xlnx-zynqmp.h>
> > +
> > +#define ZYNQMP_AES_IV_SIZE			12
> > +#define ZYNQMP_AES_GCM_SIZE			16
> > +#define ZYNQMP_AES_KEY_SIZE			32
> > +
> > +#define ZYNQMP_AES_DECRYPT			0
> > +#define ZYNQMP_AES_ENCRYPT			1
> > +
> > +#define ZYNQMP_AES_KUP_KEY			0
> > +#define ZYNQMP_AES_DEVICE_KEY			1
> > +#define ZYNQMP_AES_PUF_KEY			2
> > +
> > +#define ZYNQMP_AES_GCM_TAG_MISMATCH_ERR		0x01
> > +#define ZYNQMP_AES_SIZE_ERR			0x06
> > +#define ZYNQMP_AES_WRONG_KEY_SRC_ERR		0x13
> > +#define ZYNQMP_AES_PUF_NOT_PROGRAMMED		0xE300
> > +
> > +#define ZYNQMP_AES_BLOCKSIZE			0x04
> > +
> > +static const struct zynqmp_eemi_ops *eemi_ops; struct zynqmp_aes_dev
> > +*aes_dd;
> 
> I still think that using a global variable for storing device driver data is bad.

I think storing the list of dd's would solve up the issue with global variable, but there is only one AES instance here.
Please suggest

> 
> > +
> > +struct zynqmp_aes_dev {
> > +	struct device *dev;
> > +};
> > +
> > +struct zynqmp_aes_op {
> > +	struct zynqmp_aes_dev *dd;
> > +	void *src;
> > +	void *dst;
> > +	int len;
> > +	u8 key[ZYNQMP_AES_KEY_SIZE];
> > +	u8 *iv;
> > +	u32 keylen;
> > +	u32 keytype;
> > +};
> > +
> > +struct zynqmp_aes_data {
> > +	u64 src;
> > +	u64 iv;
> > +	u64 key;
> > +	u64 dst;
> > +	u64 size;
> > +	u64 optype;
> > +	u64 keysrc;
> > +};
> > +
> > +static int zynqmp_setkey_blk(struct crypto_tfm *tfm, const u8 *key,
> > +			     unsigned int len)
> > +{
> > +	struct zynqmp_aes_op *op = crypto_tfm_ctx(tfm);
> > +
> > +	if (((len != 1) && (len !=  ZYNQMP_AES_KEY_SIZE)) || (!key))
> 
> typo, two space

Will fix in the next version

> 
> > +		return -EINVAL;
> > +
> > +	if (len == 1) {
> > +		op->keytype = *key;
> > +
> > +		if ((op->keytype < ZYNQMP_AES_KUP_KEY) ||
> > +			(op->keytype > ZYNQMP_AES_PUF_KEY))
> > +			return -EINVAL;
> > +
> > +	} else if (len == ZYNQMP_AES_KEY_SIZE) {
> > +		op->keytype = ZYNQMP_AES_KUP_KEY;
> > +		op->keylen = len;
> > +		memcpy(op->key, key, len);
> > +	}
> > +
> > +	return 0;
> > +}
> 
> It seems your driver does not support AES keysize of 128/196, you need to
> fallback in that case.

[Kalyani] In case of 128/196 keysize, returning the error would suffice ?
Or still algorithm need to work ?
If error is enough, it is taken care by this condition 
if (((len != 1) && (len !=  ZYNQMP_AES_KEY_SIZE)) || (!key))


> You need to comment the keylen=1 usecase and use a define for this value.
> 

Will fix in next version

> > +
> > +static int zynqmp_aes_xcrypt(struct blkcipher_desc *desc,
> > +			     struct scatterlist *dst,
> > +			     struct scatterlist *src,
> > +			     unsigned int nbytes,
> > +			     unsigned int flags)
> > +{
> > +	struct zynqmp_aes_op *op = crypto_blkcipher_ctx(desc->tfm);
> > +	struct zynqmp_aes_dev *dd = aes_dd;
> > +	int err, ret, copy_bytes, src_data = 0, dst_data = 0;
> > +	dma_addr_t dma_addr, dma_addr_buf;
> > +	struct zynqmp_aes_data *abuf;
> > +	struct blkcipher_walk walk;
> > +	unsigned int data_size;
> > +	size_t dma_size;
> > +	char *kbuf;
> > +
> > +	if (!eemi_ops->aes)
> > +		return -ENOTSUPP;
> > +
> > +	if (op->keytype == ZYNQMP_AES_KUP_KEY)
> > +		dma_size = nbytes + ZYNQMP_AES_KEY_SIZE
> > +			+ ZYNQMP_AES_IV_SIZE;
> > +	else
> > +		dma_size = nbytes + ZYNQMP_AES_IV_SIZE;
> > +
> > +	kbuf = dma_alloc_coherent(dd->dev, dma_size, &dma_addr,
> GFP_KERNEL);
> > +	if (!kbuf)
> > +		return -ENOMEM;
> > +
> > +	abuf = dma_alloc_coherent(dd->dev, sizeof(struct zynqmp_aes_data),
> > +				  &dma_addr_buf, GFP_KERNEL);
> > +	if (!abuf) {
> > +		dma_free_coherent(dd->dev, dma_size, kbuf, dma_addr);
> > +		return -ENOMEM;
> > +	}
> > +
> > +	data_size = nbytes;
> > +	blkcipher_walk_init(&walk, dst, src, data_size);
> > +	err = blkcipher_walk_virt(desc, &walk);
> > +	op->iv = walk.iv;
> > +
> > +	while ((nbytes = walk.nbytes)) {
> > +		op->src = walk.src.virt.addr;
> > +		memcpy(kbuf + src_data, op->src, nbytes);
> > +		src_data = src_data + nbytes;
> > +		nbytes &= (ZYNQMP_AES_BLOCKSIZE - 1);
> > +		err = blkcipher_walk_done(desc, &walk, nbytes);
> > +	}
> > +	memcpy(kbuf + data_size, op->iv, ZYNQMP_AES_IV_SIZE);
> > +	abuf->src = dma_addr;
> > +	abuf->dst = dma_addr;
> > +	abuf->iv = abuf->src + data_size;
> > +	abuf->size = data_size - ZYNQMP_AES_GCM_SIZE;
> > +	abuf->optype = flags;
> > +	abuf->keysrc = op->keytype;
> > +
> > +	if (op->keytype == ZYNQMP_AES_KUP_KEY) {
> > +		memcpy(kbuf + data_size + ZYNQMP_AES_IV_SIZE,
> > +		       op->key, ZYNQMP_AES_KEY_SIZE);
> > +
> > +		abuf->key = abuf->src + data_size + ZYNQMP_AES_IV_SIZE;
> > +	} else {
> > +		abuf->key = 0;
> > +	}
> > +	eemi_ops->aes(dma_addr_buf, &ret);
> > +
> > +	if (ret != 0) {
> > +		switch (ret) {
> > +		case ZYNQMP_AES_GCM_TAG_MISMATCH_ERR:
> > +			dev_err(dd->dev, "ERROR: Gcm Tag mismatch\n\r");
> > +			break;
> > +		case ZYNQMP_AES_SIZE_ERR:
> > +			dev_err(dd->dev, "ERROR : Non word aligned
> data\n\r");
> > +			break;
> > +		case ZYNQMP_AES_WRONG_KEY_SRC_ERR:
> > +			dev_err(dd->dev, "ERROR: Wrong KeySrc, enable secure
> mode\n\r");
> > +			break;
> > +		case ZYNQMP_AES_PUF_NOT_PROGRAMMED:
> > +			dev_err(dd->dev, "ERROR: PUF is not registered\r\n");
> > +			break;
> > +		default:
> > +			dev_err(dd->dev, "ERROR: Invalid");
> > +			break;
> > +		}
> > +		goto END;
> > +	}
> > +	if (flags)
> > +		copy_bytes = data_size;
> > +	else
> > +		copy_bytes = data_size - ZYNQMP_AES_GCM_SIZE;
> > +
> > +	blkcipher_walk_init(&walk, dst, src, copy_bytes);
> > +	err = blkcipher_walk_virt(desc, &walk);
> > +
> > +	while ((nbytes = walk.nbytes)) {
> > +		memcpy(walk.dst.virt.addr, kbuf + dst_data, nbytes);
> > +		dst_data = dst_data + nbytes;
> > +		nbytes &= (ZYNQMP_AES_BLOCKSIZE - 1);
> > +		err = blkcipher_walk_done(desc, &walk, nbytes);
> > +	}
> > +END:
> > +	memset(kbuf, 0, dma_size);
> > +	memset(abuf, 0, sizeof(struct zynqmp_aes_data));
> > +	dma_free_coherent(dd->dev, dma_size, kbuf, dma_addr);
> > +	dma_free_coherent(dd->dev, sizeof(struct zynqmp_aes_data),
> > +			  abuf, dma_addr_buf);
> > +	return err;
> > +}
> > +
> > +static int zynqmp_aes_decrypt(struct blkcipher_desc *desc,
> > +			      struct scatterlist *dst,
> > +			      struct scatterlist *src,
> > +			      unsigned int nbytes)
> > +{
> > +	return zynqmp_aes_xcrypt(desc, dst, src, nbytes,
> > +ZYNQMP_AES_DECRYPT); }
> > +
> > +static int zynqmp_aes_encrypt(struct blkcipher_desc *desc,
> > +			      struct scatterlist *dst,
> > +			      struct scatterlist *src,
> > +			      unsigned int nbytes)
> > +{
> > +	return zynqmp_aes_xcrypt(desc, dst, src, nbytes,
> > +ZYNQMP_AES_ENCRYPT); }
> > +
> > +static struct crypto_alg zynqmp_alg = {
> > +	.cra_name		=	"xilinx-zynqmp-aes",
> > +	.cra_driver_name	=	"zynqmp-aes-gcm",
> > +	.cra_priority		=	400,
> > +	.cra_flags		=	CRYPTO_ALG_TYPE_BLKCIPHER |
> > +					CRYPTO_ALG_KERN_DRIVER_ONLY,
> > +	.cra_blocksize		=	ZYNQMP_AES_BLOCKSIZE,
> > +	.cra_ctxsize		=	sizeof(struct zynqmp_aes_op),
> > +	.cra_alignmask		=	15,
> > +	.cra_type		=	&crypto_blkcipher_type,
> > +	.cra_module		=	THIS_MODULE,
> > +	.cra_u			=	{
> > +	.blkcipher	=	{
> > +			.min_keysize	=	0,
> 
> Are you sure to accept this a keysize of 0 ?
> 

Will correct in next version 

Regards
kalyani
> Regards

Re: [PATCH V2 4/4] crypto: Add Xilinx AES driver

[Corentin Labbe]

On Wed, Sep 04, 2019 at 05:40:22PM +0000, Kalyani Akula wrote:
> Hi Corentin,
> 
> Thanks for the review comments.
> Please find my response/queries inline.
> 
> > -----Original Message-----
> > From: Corentin Labbe <clabbe.montjoie@gmail.com>
> > Sent: Monday, September 2, 2019 12:29 PM
> > To: Kalyani Akula <kalyania@xilinx.com>
> > Cc: herbert@gondor.apana.org.au; kstewart@linuxfoundation.org;
> > gregkh@linuxfoundation.org; tglx@linutronix.de; pombredanne@nexb.com;
> > linux-crypto@vger.kernel.org; linux-kernel@vger.kernel.org;
> > netdev@vger.kernel.org; Kalyani Akula <kalyania@xilinx.com>
> > Subject: Re: [PATCH V2 4/4] crypto: Add Xilinx AES driver
> > 
> > On Sun, Sep 01, 2019 at 07:24:58PM +0530, Kalyani Akula wrote:
> > > This patch adds AES driver support for the Xilinx ZynqMP SoC.
> > >
> > > Signed-off-by: Kalyani Akula <kalyani.akula@xilinx.com>
> > > ---
> > 
> > Hello
> > 
> > I have some comment below
> > 
> > >  drivers/crypto/Kconfig          |  11 ++
> > >  drivers/crypto/Makefile         |   1 +
> > >  drivers/crypto/zynqmp-aes-gcm.c | 297
> > > ++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 309 insertions(+)
> > >  create mode 100644 drivers/crypto/zynqmp-aes-gcm.c
> > >
> > > diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig index
> > > 603413f..a0d058a 100644
> > > --- a/drivers/crypto/Kconfig
> > > +++ b/drivers/crypto/Kconfig
> > > @@ -677,6 +677,17 @@ config CRYPTO_DEV_ROCKCHIP
> > >  	  This driver interfaces with the hardware crypto accelerator.
> > >  	  Supporting cbc/ecb chainmode, and aes/des/des3_ede cipher mode.
> > >
> > > +config CRYPTO_DEV_ZYNQMP_AES
> > > +	tristate "Support for Xilinx ZynqMP AES hw accelerator"
> > > +	depends on ARCH_ZYNQMP || COMPILE_TEST
> > > +	select CRYPTO_AES
> > > +	select CRYPTO_SKCIPHER
> > > +	help
> > > +	  Xilinx ZynqMP has AES-GCM engine used for symmetric key
> > > +	  encryption and decryption. This driver interfaces with AES hw
> > > +	  accelerator. Select this if you want to use the ZynqMP module
> > > +	  for AES algorithms.
> > > +
> > >  config CRYPTO_DEV_MEDIATEK
> > >  	tristate "MediaTek's EIP97 Cryptographic Engine driver"
> > >  	depends on (ARM && ARCH_MEDIATEK) || COMPILE_TEST diff --git
> > > a/drivers/crypto/Makefile b/drivers/crypto/Makefile index
> > > afc4753..c99663a 100644
> > > --- a/drivers/crypto/Makefile
> > > +++ b/drivers/crypto/Makefile
> > > @@ -48,3 +48,4 @@ obj-$(CONFIG_CRYPTO_DEV_BCM_SPU) += bcm/
> > >  obj-$(CONFIG_CRYPTO_DEV_SAFEXCEL) += inside-secure/
> > >  obj-$(CONFIG_CRYPTO_DEV_ARTPEC6) += axis/  obj-y += hisilicon/
> > > +obj-$(CONFIG_CRYPTO_DEV_ZYNQMP_AES) += zynqmp-aes-gcm.o
> > > diff --git a/drivers/crypto/zynqmp-aes-gcm.c
> > > b/drivers/crypto/zynqmp-aes-gcm.c new file mode 100644 index
> > > 0000000..d65f038
> > > --- /dev/null
> > > +++ b/drivers/crypto/zynqmp-aes-gcm.c
> > > @@ -0,0 +1,297 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Xilinx ZynqMP AES Driver.
> > > + * Copyright (c) 2019 Xilinx Inc.
> > > + */
> > > +
> > > +#include <crypto/aes.h>
> > > +#include <crypto/scatterwalk.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of_device.h>
> > > +#include <linux/scatterlist.h>
> > > +#include <linux/firmware/xlnx-zynqmp.h>
> > > +
> > > +#define ZYNQMP_AES_IV_SIZE			12
> > > +#define ZYNQMP_AES_GCM_SIZE			16
> > > +#define ZYNQMP_AES_KEY_SIZE			32
> > > +
> > > +#define ZYNQMP_AES_DECRYPT			0
> > > +#define ZYNQMP_AES_ENCRYPT			1
> > > +
> > > +#define ZYNQMP_AES_KUP_KEY			0
> > > +#define ZYNQMP_AES_DEVICE_KEY			1
> > > +#define ZYNQMP_AES_PUF_KEY			2
> > > +
> > > +#define ZYNQMP_AES_GCM_TAG_MISMATCH_ERR		0x01
> > > +#define ZYNQMP_AES_SIZE_ERR			0x06
> > > +#define ZYNQMP_AES_WRONG_KEY_SRC_ERR		0x13
> > > +#define ZYNQMP_AES_PUF_NOT_PROGRAMMED		0xE300
> > > +
> > > +#define ZYNQMP_AES_BLOCKSIZE			0x04
> > > +
> > > +static const struct zynqmp_eemi_ops *eemi_ops; struct zynqmp_aes_dev
> > > +*aes_dd;
> > 
> > I still think that using a global variable for storing device driver data is bad.
> 
> I think storing the list of dd's would solve up the issue with global variable, but there is only one AES instance here.
> Please suggest
> 

Look what I do for amlogic driver https://patchwork.kernel.org/patch/11059633/.
I store the device driver in the instatiation of a crypto template.

[...]
> > > +static int zynqmp_setkey_blk(struct crypto_tfm *tfm, const u8 *key,
> > > +			     unsigned int len)
> > > +{
> > > +	struct zynqmp_aes_op *op = crypto_tfm_ctx(tfm);
> > > +
> > > +	if (((len != 1) && (len !=  ZYNQMP_AES_KEY_SIZE)) || (!key))
> > 
> > typo, two space
> 
> Will fix in the next version
> 
> > 
> > > +		return -EINVAL;
> > > +
> > > +	if (len == 1) {
> > > +		op->keytype = *key;
> > > +
> > > +		if ((op->keytype < ZYNQMP_AES_KUP_KEY) ||
> > > +			(op->keytype > ZYNQMP_AES_PUF_KEY))
> > > +			return -EINVAL;
> > > +
> > > +	} else if (len == ZYNQMP_AES_KEY_SIZE) {
> > > +		op->keytype = ZYNQMP_AES_KUP_KEY;
> > > +		op->keylen = len;
> > > +		memcpy(op->key, key, len);
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > 
> > It seems your driver does not support AES keysize of 128/196, you need to
> > fallback in that case.
> 
> [Kalyani] In case of 128/196 keysize, returning the error would suffice ?
> Or still algorithm need to work ?
> If error is enough, it is taken care by this condition 
> if (((len != 1) && (len !=  ZYNQMP_AES_KEY_SIZE)) || (!key))

I think this problem just simply show us that your driver is not tested against crypto selftest.
I have tried to refuse 128/196 in my driver and I get:
alg: skcipher: cbc-aes-sun8i-ce setkey failed on test vector 0; expected_error=0, actual_error=-22, flags=0x1

So if your hardware lack 128/196 keys support, you must fallback to a software version.

Anyway please test your driver with crypto selftest enabled (and also CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y)

Regards