Hi Corentin,
Thanks for the review comments.
Please find my response/queries inline.
> -----Original Message-----
> From: Corentin Labbe <clabbe.montj...@gmail.com>
> Sent: Monday, September 2, 2019 12:29 PM
> To: Kalyani Akula <kalya...@xilinx.com>
> Cc: herb...@gondor.apana.org.au; kstew...@linuxfoundation.org;
> gre...@linuxfoundation.org; t...@linutronix.de; pombreda...@nexb.com;
> linux-crypto@vger.kernel.org; linux-ker...@vger.kernel.org;
> net...@vger.kernel.org; Kalyani Akula <kalya...@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.ak...@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
