[dpdk-dev] [PATCH 2/4] crypto/dpaa_sec: add crypto driver for NXP DPAA platform

[Akhil Goyal]

Hi Pablo,

On 9/18/2017 11:41 PM, De Lara Guarch, Pablo wrote:
> 
> 
>> -----Original Message-----
>> From: Akhil Goyal [mailto:akhil.goyal at nxp.com]
>> Sent: Thursday, August 24, 2017 1:01 AM
>> To: dev at dpdk.org; De Lara Guarch, Pablo
>> <pablo.de.lara.guarch at intel.com>
>> Cc: Doherty, Declan <declan.doherty at intel.com>; Mcnamara, John
>> <john.mcnamara at intel.com>; hemant.agrawal at nxp.com; Akhil Goyal
>> <akhil.goyal at nxp.com>
>> Subject: [PATCH 2/4] crypto/dpaa_sec: add crypto driver for NXP DPAA
>> platform
>>
>> Signed-off-by: Forrest Shi <xuelin.shi at nxp.com>
>> Signed-off-by: Akhil Goyal <akhil.goyal at nxp.com>
>> Signed-off-by: Hemant Agrawal <hemant.agrawal at nxp.com>
>> ---
>>   MAINTAINERS                                        |    5 +
>>   config/common_base                                 |    8 +
>>   config/defconfig_arm64-dpaa-linuxapp-gcc           |   14 +
>>   drivers/Makefile                                   |    2 +-
>>   drivers/crypto/Makefile                            |    2 +
>>   drivers/crypto/dpaa_sec/Makefile                   |   71 +
>>   drivers/crypto/dpaa_sec/dpaa_sec.c                 | 1552
>> ++++++++++++++++++++
>>   drivers/crypto/dpaa_sec/dpaa_sec.h                 |  403 +++++
>>   drivers/crypto/dpaa_sec/dpaa_sec_log.h             |   70 +
>>   .../crypto/dpaa_sec/rte_pmd_dpaa_sec_version.map   |    4 +
>>   mk/rte.app.mk                                      |    6 +
>>   11 files changed, 2136 insertions(+), 1 deletion(-)
>>   create mode 100644 drivers/crypto/dpaa_sec/Makefile
>>   create mode 100644 drivers/crypto/dpaa_sec/dpaa_sec.c
>>   create mode 100644 drivers/crypto/dpaa_sec/dpaa_sec.h
>>   create mode 100644 drivers/crypto/dpaa_sec/dpaa_sec_log.h
>>   create mode 100644
>> drivers/crypto/dpaa_sec/rte_pmd_dpaa_sec_version.map
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 48afbfc..24b3b41 100644
> 
> ...
> 
>> +
>> +include $(RTE_SDK)/mk/rte.lib.mk
>> diff --git a/drivers/crypto/dpaa_sec/dpaa_sec.c
>> b/drivers/crypto/dpaa_sec/dpaa_sec.c
>> new file mode 100644
>> index 0000000..c8f8be9
>> --- /dev/null
>> +++ b/drivers/crypto/dpaa_sec/dpaa_sec.c
>> @@ -0,0 +1,1552 @@
>> +/*-
>> + *   BSD LICENSE
>> + *
> 
> ...
> 
>> +
>> +static inline struct dpaa_sec_op_ctx *
>> +dpaa_sec_alloc_ctx(dpaa_sec_session *ses)
>> +{
>> +	struct dpaa_sec_op_ctx *ctx;
>> +	int retval;
>> +
>> +	retval = rte_mempool_get(ses->ctx_pool, (void **)(&ctx));
>> +	if (!ctx || retval) {
>> +		PMD_TX_LOG(ERR, "Alloc sec descriptor failed!");
>> +		return NULL;
>> +	}
>> +	dcbz_64(&ctx->job.sg[0]);
>> +	dcbz_64(&ctx->job.sg[5]);
>> +	dcbz_64(&ctx->job.sg[9]);
>> +	dcbz_64(&ctx->job.sg[13]);
> 
> Are these numbers ok? First, you should define macros for them, but it looks strange
> that there is a gap of 5 between the first and the second, and the rest has a gap of 4.
> 
>> +
>> +	ctx->ctx_pool = ses->ctx_pool;
>> +
>> +	return ctx;
>> +}
>> +
> 
> ...
> 
>> +/* prepare command block of the session */
>> +static int
>> +dpaa_sec_prep_cdb(dpaa_sec_session *ses)
>> +{
>> +	struct alginfo alginfo_c = {0}, alginfo_a = {0}, alginfo = {0};
>> +	uint32_t shared_desc_len = 0;
>> +	struct sec_cdb *cdb = &ses->qp->cdb;
>> +	int err;
>> +#if RTE_BYTE_ORDER == RTE_BIG_ENDIAN
>> +	int swap = false;
>> +#else
>> +	int swap = true;
>> +#endif
>> +
>> +	memset(cdb, 0, sizeof(struct sec_cdb));
>> +
>> +	if (is_cipher_only(ses)) {
>> +		caam_cipher_alg(ses, &alginfo_c);
>> +		if (alginfo_c.algtype == (unsigned
>> int)DPAA_SEC_ALG_UNSUPPORT) {
>> +			PMD_TX_LOG(ERR, "not supported cipher alg\n");
>> +			return -1;
> 
> You could use -ENOTSUP, instead of -1.
> I also checked that this function is called, but the return value is not verified,
> so either check it when calling it, or change the return type to "void".
> 
>> +		}
>> +
>> +		alginfo_c.key = (uint64_t)ses->cipher_key.data;
>> +		alginfo_c.keylen = ses->cipher_key.length;
>> +		alginfo_c.key_enc_flags = 0;
>> +		alginfo_c.key_type = RTA_DATA_IMM;
>> +
>> +		shared_desc_len = cnstr_shdsc_blkcipher(
>> +						cdb->sh_desc, true,
>> +						swap, &alginfo_c,
>> +						NULL,
>> +						ses->iv.length,
>> +						ses->dir);
> 
> ...
> 
>> +static uint16_t
>> +dpaa_sec_enqueue_burst(void *qp, struct rte_crypto_op **ops,
>> +		       uint16_t nb_ops)
>> +{
>> +	/* Function to transmit the frames to given device and queuepair */
>> +	uint32_t loop;
>> +	int32_t ret;
>> +	struct dpaa_sec_qp *dpaa_qp = (struct dpaa_sec_qp *)qp;
>> +	uint16_t num_tx = 0;
>> +
>> +	if (unlikely(nb_ops == 0))
>> +		return 0;
>> +
>> +	if (ops[0]->sess_type != RTE_CRYPTO_OP_WITH_SESSION) {
>> +		PMD_TX_LOG(ERR, "sessionless crypto op not supported");
>> +		return 0;
>> +	}
> 
> Each operation is independent from the other ones, so that means that some operations
> could have a session, while others not. Shouldn't you check each operation?
>> +
>> +	/*Prepare each packet which is to be sent*/
>> +	for (loop = 0; loop < nb_ops; loop++) {
>> +		ret = dpaa_sec_enqueue_op(ops[loop], dpaa_qp);
>> +		if (!ret)
>> +			num_tx++;
>> +	}
>> +	dpaa_qp->tx_pkts += num_tx;
>> +	dpaa_qp->tx_errs += nb_ops - num_tx;
>> +
>> +	return num_tx;
>> +}
>> +
> 
> ...
> 
>> +/** Release queue pair */
>> +static int
>> +dpaa_sec_queue_pair_release(struct rte_cryptodev *dev,
>> +			    uint16_t qp_id)
>> +{
>> +	struct dpaa_sec_dev_private *internals;
>> +	struct dpaa_sec_qp *qp = NULL;
>> +
>> +	PMD_INIT_FUNC_TRACE();
>> +
>> +	PMD_INIT_LOG(DEBUG, "dev =%p, queue =%d", dev, qp_id);
>> +
>> +	internals = dev->data->dev_private;
>> +	if (qp_id >= internals->max_nb_queue_pairs) {
>> +		PMD_INIT_LOG(ERR, "Max supported qpid %d",
>> +			     internals->max_nb_queue_pairs);
>> +		return -1;
>> +	}
> 
> Better to return -EINVAL.
> 
>> +
>> +	qp = &internals->qps[qp_id];
>> +	qp->internals = NULL;
>> +	dev->data->queue_pairs[qp_id] = NULL;
>> +
>> +	return 0;
>> +}
>> +
>> +/** Setup a queue pair */
>> +static int
>> +dpaa_sec_queue_pair_setup(struct rte_cryptodev *dev, uint16_t qp_id,
>> +		__rte_unused const struct rte_cryptodev_qp_conf
>> *qp_conf,
>> +		__rte_unused int socket_id,
>> +		__rte_unused struct rte_mempool *session_pool)
>> +{
>> +	struct dpaa_sec_dev_private *internals;
>> +	struct dpaa_sec_qp *qp = NULL;
>> +
>> +	PMD_INIT_LOG(DEBUG, "dev =%p, queue =%d, conf =%p",
>> +		     dev, qp_id, qp_conf);
>> +
>> +	internals = dev->data->dev_private;
>> +	if (qp_id >= internals->max_nb_queue_pairs) {
>> +		PMD_INIT_LOG(ERR, "Max supported qpid %d",
>> +			     internals->max_nb_queue_pairs);
>> +		return -1;
>> +	}
> 
> Better to return -EINVAL.
> 
>> +
>> +	qp = &internals->qps[qp_id];
>> +	qp->internals = internals;
>> +	dev->data->queue_pairs[qp_id] = qp;
>> +
>> +	return 0;
>> +}
>> +
> 
> ...
> 
> 
>> +static
>> +void dpaa_sec_stats_get(struct rte_cryptodev *dev __rte_unused,
>> +			struct rte_cryptodev_stats *stats __rte_unused)
> 
> "static void" should be in the same line. Anyway, if this function is not going to be implemented,
> then it is probably better to just remove it, so when rte_cryptodev_stats_get() gets called,
> it will return -ENOTSUP, as the PMD does not actually support this. Same for the next function.
> 
>> +{
>> +	PMD_INIT_FUNC_TRACE();
>> +	/* -ENOTSUP; */
>> +}
>> +
>> +static
>> +void dpaa_sec_stats_reset(struct rte_cryptodev *dev __rte_unused)
>> +{
>> +	PMD_INIT_FUNC_TRACE();
>> +	/* -ENOTSUP; */
>> +}
>> +
> 
> ...
> 
>> diff --git a/drivers/crypto/dpaa_sec/dpaa_sec.h
>> b/drivers/crypto/dpaa_sec/dpaa_sec.h
>> new file mode 100644
>> index 0000000..2677f8b
>> --- /dev/null
>> +++ b/drivers/crypto/dpaa_sec/dpaa_sec.h
>> @@ -0,0 +1,403 @@
> 
> ...
> 
>> +static const struct rte_cryptodev_capabilities dpaa_sec_capabilities[] = {
>> +	{	/* MD5 HMAC */
>> +		.op = RTE_CRYPTO_OP_TYPE_SYMMETRIC,
>> +		{.sym = {
>> +			.xform_type = RTE_CRYPTO_SYM_XFORM_AUTH,
>> +			{.auth = {
>> +				.algo = RTE_CRYPTO_AUTH_MD5_HMAC,
>> +				.block_size = 64,
>> +				.key_size = {
>> +					.min = 1,
>> +					.max = 64,
>> +					.increment = 1
>> +				},
>> +				.digest_size = {
>> +					.min = 16,
>> +					.max = 16,
>> +					.increment = 0
>> +				},
>> +				.aad_size = { 0 }
> 
> No need to include aad_size here, as it is only applicable to AEAD algorithms.
> I just realized that this was left after the rework done in 17.08.
> Unfortunately, removing it will be an API change, so it will not be removed at least until 18.02.
> As it is not used, we should remove it from here, to avoid confusion.
> 
>> +			}, }
>> +		}, }
>> +	},
> 
> ...
> 
>> +	{	/* SHA256 HMAC */
>> +		.op = RTE_CRYPTO_OP_TYPE_SYMMETRIC,
>> +		{.sym = {
>> +			.xform_type = RTE_CRYPTO_SYM_XFORM_AUTH,
>> +			{.auth = {
>> +				.algo = RTE_CRYPTO_AUTH_SHA256_HMAC,
>> +				.block_size = 64,
>> +				.key_size = {
>> +					.min = 1,
>> +					.max = 64,
>> +					.increment = 1
>> +				},
>> +				.digest_size = {
>> +						.min = 32,
>> +						.max = 32,
>> +						.increment = 0
>> +					},
> 
> Unnecessary extra tab.
> 
>> +					.aad_size = { 0 }
>> +				}, }
>> +			}, }
> 
> 
Thanks for your comments. I would incorporate all the comments in my v2.

-Akhil