[dpdk-dev] [PATCH 0/3] ZUC PMD fixes

[dpdk-dev] [PATCH 0/3] ZUC PMD fixes

[Pablo de Lara]

ZUC library provides an API to encrypt buffers
in parallel with different keys.
However, the PMD was developed assuming
that all needed to share the same session (therefore, the same key).

This patchset fixes this behaviour by allowing
multiple buffers with different keys to be processed in parallel,
plus it removes some unnecessary code.

Pablo de Lara (3):
  crypto/zuc: do not set default op status
  crypto/zuc: remove unnecessary check
  crypto/zuc: batch ops with same transform

 drivers/crypto/zuc/rte_zuc_pmd.c | 103 +++++++++++++++++++++++----------------
 1 file changed, 61 insertions(+), 42 deletions(-)

-- 
2.14.3

[dpdk-dev] [PATCH 1/3] crypto/zuc: do not set default op status

[Pablo de Lara]

When crypto operations are allocated from the operation
pool, their status get reset to NOT_PROCESSED.
Therefore, there is no need to set this status again.

Fixes: cf7685d68f00 ("crypto/zuc: add driver for ZUC library")
Cc: stable@dpdk.org

Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
---
 drivers/crypto/zuc/rte_zuc_pmd.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/crypto/zuc/rte_zuc_pmd.c b/drivers/crypto/zuc/rte_zuc_pmd.c
index 167c3c6fc..6d3834bcc 100644
--- a/drivers/crypto/zuc/rte_zuc_pmd.c
+++ b/drivers/crypto/zuc/rte_zuc_pmd.c
@@ -352,9 +352,6 @@ zuc_pmd_enqueue_burst(void *queue_pair, struct rte_crypto_op **ops,
 	for (i = 0; i < nb_ops; i++) {
 		curr_c_op = ops[i];
 
-		/* Set status as enqueued (not processed yet) by default. */
-		curr_c_op->status = RTE_CRYPTO_OP_STATUS_NOT_PROCESSED;
-
 		curr_sess = zuc_get_session(qp, curr_c_op);
 		if (unlikely(curr_sess == NULL ||
 				curr_sess->op == ZUC_OP_NOT_SUPPORTED)) {
-- 
2.14.3

Re: [dpdk-dev] [PATCH 1/3] crypto/zuc: do not set default op status

[Trahe, Fiona]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Pablo de Lara
> Sent: Thursday, March 29, 2018 4:56 PM
> To: dev@dpdk.org
> Cc: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; stable@dpdk.org
> Subject: [dpdk-dev] [PATCH 1/3] crypto/zuc: do not set default op status
> 
> When crypto operations are allocated from the operation
> pool, their status get reset to NOT_PROCESSED.
> Therefore, there is no need to set this status again.
> 
> Fixes: cf7685d68f00 ("crypto/zuc: add driver for ZUC library")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
Acked-by: Fiona Trahe <fiona.trahe@intel.com>

[dpdk-dev] [PATCH 2/3] crypto/zuc: remove unnecessary check

[Pablo de Lara]

When processing operations, the operation type was being
checked to avoid if it was set to NOT SUPPORTED.
In data path, doing so is not required since that is already
checked when creating the crypto session,
so that case will not ever happen.

Fixes: cf7685d68f00 ("crypto/zuc: add driver for ZUC library")
Cc: stable@dpdk.org

Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
---
 drivers/crypto/zuc/rte_zuc_pmd.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/crypto/zuc/rte_zuc_pmd.c b/drivers/crypto/zuc/rte_zuc_pmd.c
index 6d3834bcc..568c593ae 100644
--- a/drivers/crypto/zuc/rte_zuc_pmd.c
+++ b/drivers/crypto/zuc/rte_zuc_pmd.c
@@ -353,8 +353,7 @@ zuc_pmd_enqueue_burst(void *queue_pair, struct rte_crypto_op **ops,
 		curr_c_op = ops[i];
 
 		curr_sess = zuc_get_session(qp, curr_c_op);
-		if (unlikely(curr_sess == NULL ||
-				curr_sess->op == ZUC_OP_NOT_SUPPORTED)) {
+		if (unlikely(curr_sess == NULL)) {
 			curr_c_op->status =
 					RTE_CRYPTO_OP_STATUS_INVALID_SESSION;
 			break;
-- 
2.14.3

Re: [dpdk-dev] [PATCH 2/3] crypto/zuc: remove unnecessary check

[Trahe, Fiona]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Pablo de Lara
> Sent: Thursday, March 29, 2018 4:56 PM
> To: dev@dpdk.org
> Cc: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; stable@dpdk.org
> Subject: [dpdk-dev] [PATCH 2/3] crypto/zuc: remove unnecessary check
> 
> When processing operations, the operation type was being
> checked to avoid if it was set to NOT SUPPORTED.
> In data path, doing so is not required since that is already
> checked when creating the crypto session,
> so that case will not ever happen.
> 
> Fixes: cf7685d68f00 ("crypto/zuc: add driver for ZUC library")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
Acked-by: Fiona Trahe <fiona.trahe@intel.com>

[dpdk-dev] [PATCH 3/3] crypto/zuc: batch ops with same transform

[Pablo de Lara]

The ZUC API to encrypt packets does not require the operations
to share the same key. Currently, the operations were being
batched only when they shared the same key, but this is not needed.

Instead, now operations will be batched based on the transform
(cipher only, auth only...).

Fixes: cf7685d68f00 ("crypto/zuc: add driver for ZUC library")
Cc: stable@dpdk.org

Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
---
 drivers/crypto/zuc/rte_zuc_pmd.c | 97 +++++++++++++++++++++++++---------------
 1 file changed, 60 insertions(+), 37 deletions(-)

diff --git a/drivers/crypto/zuc/rte_zuc_pmd.c b/drivers/crypto/zuc/rte_zuc_pmd.c
index 568c593ae..ea0efcf6b 100644
--- a/drivers/crypto/zuc/rte_zuc_pmd.c
+++ b/drivers/crypto/zuc/rte_zuc_pmd.c
@@ -12,7 +12,7 @@
 
 #include "rte_zuc_pmd_private.h"
 
-#define ZUC_MAX_BURST 8
+#define ZUC_MAX_BURST 4
 #define BYTE_LEN 8
 
 static uint8_t cryptodev_driver_id;
@@ -171,7 +171,7 @@ zuc_get_session(struct zuc_qp *qp, struct rte_crypto_op *op)
 /** Encrypt/decrypt mbufs with same cipher key. */
 static uint8_t
 process_zuc_cipher_op(struct rte_crypto_op **ops,
-		struct zuc_session *session,
+		struct zuc_session **sessions,
 		uint8_t num_ops)
 {
 	unsigned i;
@@ -180,6 +180,7 @@ process_zuc_cipher_op(struct rte_crypto_op **ops,
 	uint8_t *iv[ZUC_MAX_BURST];
 	uint32_t num_bytes[ZUC_MAX_BURST];
 	uint8_t *cipher_keys[ZUC_MAX_BURST];
+	struct zuc_session *sess;
 
 	for (i = 0; i < num_ops; i++) {
 		if (((ops[i]->sym->cipher.data.length % BYTE_LEN) != 0)
@@ -190,6 +191,8 @@ process_zuc_cipher_op(struct rte_crypto_op **ops,
 			break;
 		}
 
+		sess = sessions[i];
+
 #ifdef RTE_LIBRTE_PMD_ZUC_DEBUG
 		if (!rte_pktmbuf_is_contiguous(ops[i]->sym->m_src) ||
 				(ops[i]->sym->m_dst != NULL &&
@@ -211,10 +214,10 @@ process_zuc_cipher_op(struct rte_crypto_op **ops,
 			rte_pktmbuf_mtod(ops[i]->sym->m_src, uint8_t *) +
 				(ops[i]->sym->cipher.data.offset >> 3);
 		iv[i] = rte_crypto_op_ctod_offset(ops[i], uint8_t *,
-				session->cipher_iv_offset);
+				sess->cipher_iv_offset);
 		num_bytes[i] = ops[i]->sym->cipher.data.length >> 3;
 
-		cipher_keys[i] = session->pKey_cipher;
+		cipher_keys[i] = sess->pKey_cipher;
 
 		processed_ops++;
 	}
@@ -228,7 +231,7 @@ process_zuc_cipher_op(struct rte_crypto_op **ops,
 /** Generate/verify hash from mbufs with same hash key. */
 static int
 process_zuc_hash_op(struct zuc_qp *qp, struct rte_crypto_op **ops,
-		struct zuc_session *session,
+		struct zuc_session **sessions,
 		uint8_t num_ops)
 {
 	unsigned i;
@@ -237,6 +240,7 @@ process_zuc_hash_op(struct zuc_qp *qp, struct rte_crypto_op **ops,
 	uint32_t *dst;
 	uint32_t length_in_bits;
 	uint8_t *iv;
+	struct zuc_session *sess;
 
 	for (i = 0; i < num_ops; i++) {
 		/* Data must be byte aligned */
@@ -246,17 +250,19 @@ process_zuc_hash_op(struct zuc_qp *qp, struct rte_crypto_op **ops,
 			break;
 		}
 
+		sess = sessions[i];
+
 		length_in_bits = ops[i]->sym->auth.data.length;
 
 		src = rte_pktmbuf_mtod(ops[i]->sym->m_src, uint8_t *) +
 				(ops[i]->sym->auth.data.offset >> 3);
 		iv = rte_crypto_op_ctod_offset(ops[i], uint8_t *,
-				session->auth_iv_offset);
+				sess->auth_iv_offset);
 
-		if (session->auth_op == RTE_CRYPTO_AUTH_OP_VERIFY) {
+		if (sess->auth_op == RTE_CRYPTO_AUTH_OP_VERIFY) {
 			dst = (uint32_t *)qp->temp_digest;
 
-			sso_zuc_eia3_1_buffer(session->pKey_hash,
+			sso_zuc_eia3_1_buffer(sess->pKey_hash,
 					iv, src,
 					length_in_bits,	dst);
 			/* Verify digest. */
@@ -266,7 +272,7 @@ process_zuc_hash_op(struct zuc_qp *qp, struct rte_crypto_op **ops,
 		} else  {
 			dst = (uint32_t *)ops[i]->sym->auth.digest.data;
 
-			sso_zuc_eia3_1_buffer(session->pKey_hash,
+			sso_zuc_eia3_1_buffer(sess->pKey_hash,
 					iv, src,
 					length_in_bits, dst);
 		}
@@ -278,31 +284,32 @@ process_zuc_hash_op(struct zuc_qp *qp, struct rte_crypto_op **ops,
 
 /** Process a batch of crypto ops which shares the same session. */
 static int
-process_ops(struct rte_crypto_op **ops, struct zuc_session *session,
+process_ops(struct rte_crypto_op **ops, enum zuc_operation op_type,
+		struct zuc_session **sessions,
 		struct zuc_qp *qp, uint8_t num_ops,
 		uint16_t *accumulated_enqueued_ops)
 {
 	unsigned i;
 	unsigned enqueued_ops, processed_ops;
 
-	switch (session->op) {
+	switch (op_type) {
 	case ZUC_OP_ONLY_CIPHER:
 		processed_ops = process_zuc_cipher_op(ops,
-				session, num_ops);
+				sessions, num_ops);
 		break;
 	case ZUC_OP_ONLY_AUTH:
-		processed_ops = process_zuc_hash_op(qp, ops, session,
+		processed_ops = process_zuc_hash_op(qp, ops, sessions,
 				num_ops);
 		break;
 	case ZUC_OP_CIPHER_AUTH:
-		processed_ops = process_zuc_cipher_op(ops, session,
+		processed_ops = process_zuc_cipher_op(ops, sessions,
 				num_ops);
-		process_zuc_hash_op(qp, ops, session, processed_ops);
+		process_zuc_hash_op(qp, ops, sessions, processed_ops);
 		break;
 	case ZUC_OP_AUTH_CIPHER:
-		processed_ops = process_zuc_hash_op(qp, ops, session,
+		processed_ops = process_zuc_hash_op(qp, ops, sessions,
 				num_ops);
-		process_zuc_cipher_op(ops, session, processed_ops);
+		process_zuc_cipher_op(ops, sessions, processed_ops);
 		break;
 	default:
 		/* Operation not supported. */
@@ -318,10 +325,10 @@ process_ops(struct rte_crypto_op **ops, struct zuc_session *session,
 			ops[i]->status = RTE_CRYPTO_OP_STATUS_SUCCESS;
 		/* Free session if a session-less crypto op. */
 		if (ops[i]->sess_type == RTE_CRYPTO_OP_SESSIONLESS) {
-			memset(session, 0, sizeof(struct zuc_session));
+			memset(sessions[i], 0, sizeof(struct zuc_session));
 			memset(ops[i]->sym->session, 0,
 					rte_cryptodev_get_header_session_size());
-			rte_mempool_put(qp->sess_mp, session);
+			rte_mempool_put(qp->sess_mp, sessions[i]);
 			rte_mempool_put(qp->sess_mp, ops[i]->sym->session);
 			ops[i]->sym->session = NULL;
 		}
@@ -342,7 +349,10 @@ zuc_pmd_enqueue_burst(void *queue_pair, struct rte_crypto_op **ops,
 	struct rte_crypto_op *c_ops[ZUC_MAX_BURST];
 	struct rte_crypto_op *curr_c_op;
 
-	struct zuc_session *prev_sess = NULL, *curr_sess = NULL;
+	struct zuc_session *curr_sess;
+	struct zuc_session *sessions[ZUC_MAX_BURST];
+	enum zuc_operation prev_zuc_op = ZUC_OP_NOT_SUPPORTED;
+	enum zuc_operation curr_zuc_op;
 	struct zuc_qp *qp = queue_pair;
 	unsigned i;
 	uint8_t burst_size = 0;
@@ -359,50 +369,63 @@ zuc_pmd_enqueue_burst(void *queue_pair, struct rte_crypto_op **ops,
 			break;
 		}
 
-		/* Batch ops that share the same session. */
-		if (prev_sess == NULL) {
-			prev_sess = curr_sess;
-			c_ops[burst_size++] = curr_c_op;
-		} else if (curr_sess == prev_sess) {
-			c_ops[burst_size++] = curr_c_op;
+		curr_zuc_op = curr_sess->op;
+
+		/*
+		 * Batch ops that share the same operation type
+		 * (cipher only, auth only...).
+		 */
+		if (burst_size == 0) {
+			prev_zuc_op = curr_zuc_op;
+			c_ops[0] = curr_c_op;
+			sessions[0] = curr_sess;
+			burst_size++;
+		} else if (curr_zuc_op == prev_zuc_op) {
+			c_ops[burst_size] = curr_c_op;
+			sessions[burst_size] = curr_sess;
+			burst_size++;
 			/*
 			 * When there are enough ops to process in a batch,
 			 * process them, and start a new batch.
 			 */
 			if (burst_size == ZUC_MAX_BURST) {
-				processed_ops = process_ops(c_ops, prev_sess,
-						qp, burst_size, &enqueued_ops);
+				processed_ops = process_ops(c_ops, curr_zuc_op,
+						sessions, qp, burst_size,
+						&enqueued_ops);
 				if (processed_ops < burst_size) {
 					burst_size = 0;
 					break;
 				}
 
 				burst_size = 0;
-				prev_sess = NULL;
 			}
 		} else {
 			/*
-			 * Different session, process the ops
-			 * of the previous session.
+			 * Different operation type, process the ops
+			 * of the previous type.
 			 */
-			processed_ops = process_ops(c_ops, prev_sess,
-					qp, burst_size, &enqueued_ops);
+			processed_ops = process_ops(c_ops, prev_zuc_op,
+					sessions, qp, burst_size,
+					&enqueued_ops);
 			if (processed_ops < burst_size) {
 				burst_size = 0;
 				break;
 			}
 
 			burst_size = 0;
-			prev_sess = curr_sess;
+			prev_zuc_op = curr_zuc_op;
 
-			c_ops[burst_size++] = curr_c_op;
+			c_ops[0] = curr_c_op;
+			sessions[0] = curr_sess;
+			burst_size++;
 		}
 	}
 
 	if (burst_size != 0) {
 		/* Process the crypto ops of the last session. */
-		processed_ops = process_ops(c_ops, prev_sess,
-				qp, burst_size, &enqueued_ops);
+		processed_ops = process_ops(c_ops, prev_zuc_op,
+				sessions, qp, burst_size,
+				&enqueued_ops);
 	}
 
 	qp->qp_stats.enqueue_err_count += nb_ops - enqueued_ops;
-- 
2.14.3

Re: [dpdk-dev] [PATCH 3/3] crypto/zuc: batch ops with same transform

[Trahe, Fiona]

Hi Pablo,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Pablo de Lara
> Sent: Thursday, March 29, 2018 4:56 PM
> To: dev@dpdk.org
> Cc: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; stable@dpdk.org
> Subject: [dpdk-dev] [PATCH 3/3] crypto/zuc: batch ops with same transform
> 
> The ZUC API to encrypt packets does not require the operations
> to share the same key. Currently, the operations were being
> batched only when they shared the same key, but this is not needed.
> 
> Instead, now operations will be batched based on the transform
> (cipher only, auth only...).
> 
> Fixes: cf7685d68f00 ("crypto/zuc: add driver for ZUC library")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>

[Fiona] A couple of comments need updating - see below
Apart from that 
Acked-by: Fiona Trahe <fiona.trahe@intel.com>

> ---
>  drivers/crypto/zuc/rte_zuc_pmd.c | 97 +++++++++++++++++++++++++---------------
>  1 file changed, 60 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/crypto/zuc/rte_zuc_pmd.c b/drivers/crypto/zuc/rte_zuc_pmd.c
> index 568c593ae..ea0efcf6b 100644
> --- a/drivers/crypto/zuc/rte_zuc_pmd.c
> +++ b/drivers/crypto/zuc/rte_zuc_pmd.c
> @@ -12,7 +12,7 @@
> 
>  #include "rte_zuc_pmd_private.h"
> 
> -#define ZUC_MAX_BURST 8
> +#define ZUC_MAX_BURST 4
>  #define BYTE_LEN 8
> 
>  static uint8_t cryptodev_driver_id;
> @@ -171,7 +171,7 @@ zuc_get_session(struct zuc_qp *qp, struct rte_crypto_op *op)
>  /** Encrypt/decrypt mbufs with same cipher key. */
[Fiona] Not necessarily same key

>  static uint8_t
>  process_zuc_cipher_op(struct rte_crypto_op **ops,
> -		struct zuc_session *session,
> +		struct zuc_session **sessions,
>  		uint8_t num_ops)
>  {
>  	unsigned i;
> @@ -180,6 +180,7 @@ process_zuc_cipher_op(struct rte_crypto_op **ops,
>  	uint8_t *iv[ZUC_MAX_BURST];
>  	uint32_t num_bytes[ZUC_MAX_BURST];
>  	uint8_t *cipher_keys[ZUC_MAX_BURST];
> +	struct zuc_session *sess;
> 
>  	for (i = 0; i < num_ops; i++) {
>  		if (((ops[i]->sym->cipher.data.length % BYTE_LEN) != 0)
> @@ -190,6 +191,8 @@ process_zuc_cipher_op(struct rte_crypto_op **ops,
>  			break;
>  		}
> 
> +		sess = sessions[i];
> +
>  #ifdef RTE_LIBRTE_PMD_ZUC_DEBUG
>  		if (!rte_pktmbuf_is_contiguous(ops[i]->sym->m_src) ||
>  				(ops[i]->sym->m_dst != NULL &&
> @@ -211,10 +214,10 @@ process_zuc_cipher_op(struct rte_crypto_op **ops,
>  			rte_pktmbuf_mtod(ops[i]->sym->m_src, uint8_t *) +
>  				(ops[i]->sym->cipher.data.offset >> 3);
>  		iv[i] = rte_crypto_op_ctod_offset(ops[i], uint8_t *,
> -				session->cipher_iv_offset);
> +				sess->cipher_iv_offset);
>  		num_bytes[i] = ops[i]->sym->cipher.data.length >> 3;
> 
> -		cipher_keys[i] = session->pKey_cipher;
> +		cipher_keys[i] = sess->pKey_cipher;
> 
>  		processed_ops++;
>  	}
> @@ -228,7 +231,7 @@ process_zuc_cipher_op(struct rte_crypto_op **ops,
>  /** Generate/verify hash from mbufs with same hash key. */
[Fiona] Not necessarily same key

>  static int
>  process_zuc_hash_op(struct zuc_qp *qp, struct rte_crypto_op **ops,
> -		struct zuc_session *session,
> +		struct zuc_session **sessions,
>  		uint8_t num_ops)
>  {
>  	unsigned i;
> @@ -237,6 +240,7 @@ process_zuc_hash_op(struct zuc_qp *qp, struct rte_crypto_op **ops,
>  	uint32_t *dst;
>  	uint32_t length_in_bits;
>  	uint8_t *iv;
> +	struct zuc_session *sess;
> 
>  	for (i = 0; i < num_ops; i++) {
>  		/* Data must be byte aligned */
> @@ -246,17 +250,19 @@ process_zuc_hash_op(struct zuc_qp *qp, struct rte_crypto_op **ops,
>  			break;
>  		}
> 
> +		sess = sessions[i];
> +
>  		length_in_bits = ops[i]->sym->auth.data.length;
> 
>  		src = rte_pktmbuf_mtod(ops[i]->sym->m_src, uint8_t *) +
>  				(ops[i]->sym->auth.data.offset >> 3);
>  		iv = rte_crypto_op_ctod_offset(ops[i], uint8_t *,
> -				session->auth_iv_offset);
> +				sess->auth_iv_offset);
> 
> -		if (session->auth_op == RTE_CRYPTO_AUTH_OP_VERIFY) {
> +		if (sess->auth_op == RTE_CRYPTO_AUTH_OP_VERIFY) {
>  			dst = (uint32_t *)qp->temp_digest;
> 
> -			sso_zuc_eia3_1_buffer(session->pKey_hash,
> +			sso_zuc_eia3_1_buffer(sess->pKey_hash,
>  					iv, src,
>  					length_in_bits,	dst);
>  			/* Verify digest. */
> @@ -266,7 +272,7 @@ process_zuc_hash_op(struct zuc_qp *qp, struct rte_crypto_op **ops,
>  		} else  {
>  			dst = (uint32_t *)ops[i]->sym->auth.digest.data;
> 
> -			sso_zuc_eia3_1_buffer(session->pKey_hash,
> +			sso_zuc_eia3_1_buffer(sess->pKey_hash,
>  					iv, src,
>  					length_in_bits, dst);
>  		}
> @@ -278,31 +284,32 @@ process_zuc_hash_op(struct zuc_qp *qp, struct rte_crypto_op **ops,
> 
>  /** Process a batch of crypto ops which shares the same session. */
[Fiona] same type, not same session

>  static int
> -process_ops(struct rte_crypto_op **ops, struct zuc_session *session,
> +process_ops(struct rte_crypto_op **ops, enum zuc_operation op_type,
> +		struct zuc_session **sessions,
>  		struct zuc_qp *qp, uint8_t num_ops,
>  		uint16_t *accumulated_enqueued_ops)
>  {
>  	unsigned i;
>  	unsigned enqueued_ops, processed_ops;
> 
> -	switch (session->op) {
> +	switch (op_type) {
>  	case ZUC_OP_ONLY_CIPHER:
>  		processed_ops = process_zuc_cipher_op(ops,
> -				session, num_ops);
> +				sessions, num_ops);
>  		break;
>  	case ZUC_OP_ONLY_AUTH:
> -		processed_ops = process_zuc_hash_op(qp, ops, session,
> +		processed_ops = process_zuc_hash_op(qp, ops, sessions,
>  				num_ops);
>  		break;
>  	case ZUC_OP_CIPHER_AUTH:
> -		processed_ops = process_zuc_cipher_op(ops, session,
> +		processed_ops = process_zuc_cipher_op(ops, sessions,
>  				num_ops);
> -		process_zuc_hash_op(qp, ops, session, processed_ops);
> +		process_zuc_hash_op(qp, ops, sessions, processed_ops);
>  		break;
>  	case ZUC_OP_AUTH_CIPHER:
> -		processed_ops = process_zuc_hash_op(qp, ops, session,
> +		processed_ops = process_zuc_hash_op(qp, ops, sessions,
>  				num_ops);
> -		process_zuc_cipher_op(ops, session, processed_ops);
> +		process_zuc_cipher_op(ops, sessions, processed_ops);
>  		break;
>  	default:
>  		/* Operation not supported. */
> @@ -318,10 +325,10 @@ process_ops(struct rte_crypto_op **ops, struct zuc_session *session,
>  			ops[i]->status = RTE_CRYPTO_OP_STATUS_SUCCESS;
>  		/* Free session if a session-less crypto op. */
>  		if (ops[i]->sess_type == RTE_CRYPTO_OP_SESSIONLESS) {
> -			memset(session, 0, sizeof(struct zuc_session));
> +			memset(sessions[i], 0, sizeof(struct zuc_session));
>  			memset(ops[i]->sym->session, 0,
>  					rte_cryptodev_get_header_session_size());
> -			rte_mempool_put(qp->sess_mp, session);
> +			rte_mempool_put(qp->sess_mp, sessions[i]);
>  			rte_mempool_put(qp->sess_mp, ops[i]->sym->session);
>  			ops[i]->sym->session = NULL;
>  		}
> @@ -342,7 +349,10 @@ zuc_pmd_enqueue_burst(void *queue_pair, struct rte_crypto_op **ops,
>  	struct rte_crypto_op *c_ops[ZUC_MAX_BURST];
>  	struct rte_crypto_op *curr_c_op;
> 
> -	struct zuc_session *prev_sess = NULL, *curr_sess = NULL;
> +	struct zuc_session *curr_sess;
> +	struct zuc_session *sessions[ZUC_MAX_BURST];
> +	enum zuc_operation prev_zuc_op = ZUC_OP_NOT_SUPPORTED;
> +	enum zuc_operation curr_zuc_op;
>  	struct zuc_qp *qp = queue_pair;
>  	unsigned i;
>  	uint8_t burst_size = 0;
> @@ -359,50 +369,63 @@ zuc_pmd_enqueue_burst(void *queue_pair, struct rte_crypto_op **ops,
>  			break;
>  		}
> 
> -		/* Batch ops that share the same session. */
> -		if (prev_sess == NULL) {
> -			prev_sess = curr_sess;
> -			c_ops[burst_size++] = curr_c_op;
> -		} else if (curr_sess == prev_sess) {
> -			c_ops[burst_size++] = curr_c_op;
> +		curr_zuc_op = curr_sess->op;
> +
> +		/*
> +		 * Batch ops that share the same operation type
> +		 * (cipher only, auth only...).
> +		 */
> +		if (burst_size == 0) {
> +			prev_zuc_op = curr_zuc_op;
> +			c_ops[0] = curr_c_op;
> +			sessions[0] = curr_sess;
> +			burst_size++;
> +		} else if (curr_zuc_op == prev_zuc_op) {
> +			c_ops[burst_size] = curr_c_op;
> +			sessions[burst_size] = curr_sess;
> +			burst_size++;
>  			/*
>  			 * When there are enough ops to process in a batch,
>  			 * process them, and start a new batch.
>  			 */
>  			if (burst_size == ZUC_MAX_BURST) {
> -				processed_ops = process_ops(c_ops, prev_sess,
> -						qp, burst_size, &enqueued_ops);
> +				processed_ops = process_ops(c_ops, curr_zuc_op,
> +						sessions, qp, burst_size,
> +						&enqueued_ops);
>  				if (processed_ops < burst_size) {
>  					burst_size = 0;
>  					break;
>  				}
> 
>  				burst_size = 0;
> -				prev_sess = NULL;
>  			}
>  		} else {
>  			/*
> -			 * Different session, process the ops
> -			 * of the previous session.
> +			 * Different operation type, process the ops
> +			 * of the previous type.
>  			 */
> -			processed_ops = process_ops(c_ops, prev_sess,
> -					qp, burst_size, &enqueued_ops);
> +			processed_ops = process_ops(c_ops, prev_zuc_op,
> +					sessions, qp, burst_size,
> +					&enqueued_ops);
>  			if (processed_ops < burst_size) {
>  				burst_size = 0;
>  				break;
>  			}
> 
>  			burst_size = 0;
> -			prev_sess = curr_sess;
> +			prev_zuc_op = curr_zuc_op;
> 
> -			c_ops[burst_size++] = curr_c_op;
> +			c_ops[0] = curr_c_op;
> +			sessions[0] = curr_sess;
> +			burst_size++;
>  		}
>  	}
> 
>  	if (burst_size != 0) {
>  		/* Process the crypto ops of the last session. */
[Fiona] ...of the last op_type

> -		processed_ops = process_ops(c_ops, prev_sess,
> -				qp, burst_size, &enqueued_ops);
> +		processed_ops = process_ops(c_ops, prev_zuc_op,
> +				sessions, qp, burst_size,
> +				&enqueued_ops);
>  	}
> 
>  	qp->qp_stats.enqueue_err_count += nb_ops - enqueued_ops;
> --
> 2.14.3

Re: [dpdk-dev] [PATCH 3/3] crypto/zuc: batch ops with same transform

[De Lara Guarch, Pablo]

Hi Fiona,

> -----Original Message-----
> From: Trahe, Fiona
> Sent: Thursday, April 19, 2018 3:23 PM
> To: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; dev@dpdk.org
> Cc: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; stable@dpdk.org;
> Trahe, Fiona <fiona.trahe@intel.com>
> Subject: RE: [dpdk-dev] [PATCH 3/3] crypto/zuc: batch ops with same transform
> 
> Hi Pablo,
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Pablo de Lara
> > Sent: Thursday, March 29, 2018 4:56 PM
> > To: dev@dpdk.org
> > Cc: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>;
> > stable@dpdk.org
> > Subject: [dpdk-dev] [PATCH 3/3] crypto/zuc: batch ops with same
> > transform
> >
> > The ZUC API to encrypt packets does not require the operations to
> > share the same key. Currently, the operations were being batched only
> > when they shared the same key, but this is not needed.
> >
> > Instead, now operations will be batched based on the transform (cipher
> > only, auth only...).
> >
> > Fixes: cf7685d68f00 ("crypto/zuc: add driver for ZUC library")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
> 
> [Fiona] A couple of comments need updating - see below Apart from that
> Acked-by: Fiona Trahe <fiona.trahe@intel.com>

Thanks for the comments. Agreed on them, will send a v2.

Pablo

[dpdk-dev] [PATCH v2 0/3] ZUC PMD fixes

[Pablo de Lara]

ZUC library provides an API to encrypt buffers in parallel with different keys.
However, the PMD was developed assuming
that all needed to share the same session (therefore, the same key).

This patchset fixes this behaviour by allowing multiple buffers with
different keys to be processed in parallel, plus it removes some unnecessary code.

Changes in v2:
- Fixed incorrect comments [Fiona]

Pablo de Lara (3):
  crypto/zuc: do not set default op status
  crypto/zuc: remove unnecessary check
  crypto/zuc: batch ops with same transform

 drivers/crypto/zuc/rte_zuc_pmd.c | 111 +++++++++++++++++++++++----------------
 1 file changed, 65 insertions(+), 46 deletions(-)

-- 
2.14.3

[dpdk-dev] [PATCH v2 1/3] crypto/zuc: do not set default op status

[Pablo de Lara]

When crypto operations are allocated from the operation
pool, their status get reset to NOT_PROCESSED.
Therefore, there is no need to set this status again.

Fixes: cf7685d68f00 ("crypto/zuc: add driver for ZUC library")
Cc: stable@dpdk.org

Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
Acked-by: Fiona Trahe <fiona.trahe@intel.com>
---
 drivers/crypto/zuc/rte_zuc_pmd.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/crypto/zuc/rte_zuc_pmd.c b/drivers/crypto/zuc/rte_zuc_pmd.c
index 167c3c6fc..6d3834bcc 100644
--- a/drivers/crypto/zuc/rte_zuc_pmd.c
+++ b/drivers/crypto/zuc/rte_zuc_pmd.c
@@ -352,9 +352,6 @@ zuc_pmd_enqueue_burst(void *queue_pair, struct rte_crypto_op **ops,
 	for (i = 0; i < nb_ops; i++) {
 		curr_c_op = ops[i];
 
-		/* Set status as enqueued (not processed yet) by default. */
-		curr_c_op->status = RTE_CRYPTO_OP_STATUS_NOT_PROCESSED;
-
 		curr_sess = zuc_get_session(qp, curr_c_op);
 		if (unlikely(curr_sess == NULL ||
 				curr_sess->op == ZUC_OP_NOT_SUPPORTED)) {
-- 
2.14.3

[dpdk-dev] [PATCH v2 2/3] crypto/zuc: remove unnecessary check

[Pablo de Lara]

When processing operations, the operation type was being
checked to avoid if it was set to NOT SUPPORTED.
In data path, doing so is not required since that is already
checked when creating the crypto session,
so that case will not ever happen.

Fixes: cf7685d68f00 ("crypto/zuc: add driver for ZUC library")
Cc: stable@dpdk.org

Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
Acked-by: Fiona Trahe <fiona.trahe@intel.com>
---
 drivers/crypto/zuc/rte_zuc_pmd.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/crypto/zuc/rte_zuc_pmd.c b/drivers/crypto/zuc/rte_zuc_pmd.c
index 6d3834bcc..568c593ae 100644
--- a/drivers/crypto/zuc/rte_zuc_pmd.c
+++ b/drivers/crypto/zuc/rte_zuc_pmd.c
@@ -353,8 +353,7 @@ zuc_pmd_enqueue_burst(void *queue_pair, struct rte_crypto_op **ops,
 		curr_c_op = ops[i];
 
 		curr_sess = zuc_get_session(qp, curr_c_op);
-		if (unlikely(curr_sess == NULL ||
-				curr_sess->op == ZUC_OP_NOT_SUPPORTED)) {
+		if (unlikely(curr_sess == NULL)) {
 			curr_c_op->status =
 					RTE_CRYPTO_OP_STATUS_INVALID_SESSION;
 			break;
-- 
2.14.3

[dpdk-dev] [PATCH v2 3/3] crypto/zuc: batch ops with same transform

[Pablo de Lara]

The ZUC API to encrypt packets does not require the operations
to share the same key. Currently, the operations were being
batched only when they shared the same key, but this is not needed.

Instead, now operations will be batched based on the transform
(cipher only, auth only...).

Fixes: cf7685d68f00 ("crypto/zuc: add driver for ZUC library")
Cc: stable@dpdk.org

Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
Acked-by: Fiona Trahe <fiona.trahe@intel.com>
---
 drivers/crypto/zuc/rte_zuc_pmd.c | 105 ++++++++++++++++++++++++---------------
 1 file changed, 64 insertions(+), 41 deletions(-)

diff --git a/drivers/crypto/zuc/rte_zuc_pmd.c b/drivers/crypto/zuc/rte_zuc_pmd.c
index 568c593ae..cd38f3030 100644
--- a/drivers/crypto/zuc/rte_zuc_pmd.c
+++ b/drivers/crypto/zuc/rte_zuc_pmd.c
@@ -12,7 +12,7 @@
 
 #include "rte_zuc_pmd_private.h"
 
-#define ZUC_MAX_BURST 8
+#define ZUC_MAX_BURST 4
 #define BYTE_LEN 8
 
 static uint8_t cryptodev_driver_id;
@@ -168,10 +168,10 @@ zuc_get_session(struct zuc_qp *qp, struct rte_crypto_op *op)
 	return sess;
 }
 
-/** Encrypt/decrypt mbufs with same cipher key. */
+/** Encrypt/decrypt mbufs. */
 static uint8_t
 process_zuc_cipher_op(struct rte_crypto_op **ops,
-		struct zuc_session *session,
+		struct zuc_session **sessions,
 		uint8_t num_ops)
 {
 	unsigned i;
@@ -180,6 +180,7 @@ process_zuc_cipher_op(struct rte_crypto_op **ops,
 	uint8_t *iv[ZUC_MAX_BURST];
 	uint32_t num_bytes[ZUC_MAX_BURST];
 	uint8_t *cipher_keys[ZUC_MAX_BURST];
+	struct zuc_session *sess;
 
 	for (i = 0; i < num_ops; i++) {
 		if (((ops[i]->sym->cipher.data.length % BYTE_LEN) != 0)
@@ -190,6 +191,8 @@ process_zuc_cipher_op(struct rte_crypto_op **ops,
 			break;
 		}
 
+		sess = sessions[i];
+
 #ifdef RTE_LIBRTE_PMD_ZUC_DEBUG
 		if (!rte_pktmbuf_is_contiguous(ops[i]->sym->m_src) ||
 				(ops[i]->sym->m_dst != NULL &&
@@ -211,10 +214,10 @@ process_zuc_cipher_op(struct rte_crypto_op **ops,
 			rte_pktmbuf_mtod(ops[i]->sym->m_src, uint8_t *) +
 				(ops[i]->sym->cipher.data.offset >> 3);
 		iv[i] = rte_crypto_op_ctod_offset(ops[i], uint8_t *,
-				session->cipher_iv_offset);
+				sess->cipher_iv_offset);
 		num_bytes[i] = ops[i]->sym->cipher.data.length >> 3;
 
-		cipher_keys[i] = session->pKey_cipher;
+		cipher_keys[i] = sess->pKey_cipher;
 
 		processed_ops++;
 	}
@@ -225,10 +228,10 @@ process_zuc_cipher_op(struct rte_crypto_op **ops,
 	return processed_ops;
 }
 
-/** Generate/verify hash from mbufs with same hash key. */
+/** Generate/verify hash from mbufs. */
 static int
 process_zuc_hash_op(struct zuc_qp *qp, struct rte_crypto_op **ops,
-		struct zuc_session *session,
+		struct zuc_session **sessions,
 		uint8_t num_ops)
 {
 	unsigned i;
@@ -237,6 +240,7 @@ process_zuc_hash_op(struct zuc_qp *qp, struct rte_crypto_op **ops,
 	uint32_t *dst;
 	uint32_t length_in_bits;
 	uint8_t *iv;
+	struct zuc_session *sess;
 
 	for (i = 0; i < num_ops; i++) {
 		/* Data must be byte aligned */
@@ -246,17 +250,19 @@ process_zuc_hash_op(struct zuc_qp *qp, struct rte_crypto_op **ops,
 			break;
 		}
 
+		sess = sessions[i];
+
 		length_in_bits = ops[i]->sym->auth.data.length;
 
 		src = rte_pktmbuf_mtod(ops[i]->sym->m_src, uint8_t *) +
 				(ops[i]->sym->auth.data.offset >> 3);
 		iv = rte_crypto_op_ctod_offset(ops[i], uint8_t *,
-				session->auth_iv_offset);
+				sess->auth_iv_offset);
 
-		if (session->auth_op == RTE_CRYPTO_AUTH_OP_VERIFY) {
+		if (sess->auth_op == RTE_CRYPTO_AUTH_OP_VERIFY) {
 			dst = (uint32_t *)qp->temp_digest;
 
-			sso_zuc_eia3_1_buffer(session->pKey_hash,
+			sso_zuc_eia3_1_buffer(sess->pKey_hash,
 					iv, src,
 					length_in_bits,	dst);
 			/* Verify digest. */
@@ -266,7 +272,7 @@ process_zuc_hash_op(struct zuc_qp *qp, struct rte_crypto_op **ops,
 		} else  {
 			dst = (uint32_t *)ops[i]->sym->auth.digest.data;
 
-			sso_zuc_eia3_1_buffer(session->pKey_hash,
+			sso_zuc_eia3_1_buffer(sess->pKey_hash,
 					iv, src,
 					length_in_bits, dst);
 		}
@@ -276,33 +282,34 @@ process_zuc_hash_op(struct zuc_qp *qp, struct rte_crypto_op **ops,
 	return processed_ops;
 }
 
-/** Process a batch of crypto ops which shares the same session. */
+/** Process a batch of crypto ops which shares the same operation type. */
 static int
-process_ops(struct rte_crypto_op **ops, struct zuc_session *session,
+process_ops(struct rte_crypto_op **ops, enum zuc_operation op_type,
+		struct zuc_session **sessions,
 		struct zuc_qp *qp, uint8_t num_ops,
 		uint16_t *accumulated_enqueued_ops)
 {
 	unsigned i;
 	unsigned enqueued_ops, processed_ops;
 
-	switch (session->op) {
+	switch (op_type) {
 	case ZUC_OP_ONLY_CIPHER:
 		processed_ops = process_zuc_cipher_op(ops,
-				session, num_ops);
+				sessions, num_ops);
 		break;
 	case ZUC_OP_ONLY_AUTH:
-		processed_ops = process_zuc_hash_op(qp, ops, session,
+		processed_ops = process_zuc_hash_op(qp, ops, sessions,
 				num_ops);
 		break;
 	case ZUC_OP_CIPHER_AUTH:
-		processed_ops = process_zuc_cipher_op(ops, session,
+		processed_ops = process_zuc_cipher_op(ops, sessions,
 				num_ops);
-		process_zuc_hash_op(qp, ops, session, processed_ops);
+		process_zuc_hash_op(qp, ops, sessions, processed_ops);
 		break;
 	case ZUC_OP_AUTH_CIPHER:
-		processed_ops = process_zuc_hash_op(qp, ops, session,
+		processed_ops = process_zuc_hash_op(qp, ops, sessions,
 				num_ops);
-		process_zuc_cipher_op(ops, session, processed_ops);
+		process_zuc_cipher_op(ops, sessions, processed_ops);
 		break;
 	default:
 		/* Operation not supported. */
@@ -318,10 +325,10 @@ process_ops(struct rte_crypto_op **ops, struct zuc_session *session,
 			ops[i]->status = RTE_CRYPTO_OP_STATUS_SUCCESS;
 		/* Free session if a session-less crypto op. */
 		if (ops[i]->sess_type == RTE_CRYPTO_OP_SESSIONLESS) {
-			memset(session, 0, sizeof(struct zuc_session));
+			memset(sessions[i], 0, sizeof(struct zuc_session));
 			memset(ops[i]->sym->session, 0,
 					rte_cryptodev_get_header_session_size());
-			rte_mempool_put(qp->sess_mp, session);
+			rte_mempool_put(qp->sess_mp, sessions[i]);
 			rte_mempool_put(qp->sess_mp, ops[i]->sym->session);
 			ops[i]->sym->session = NULL;
 		}
@@ -342,7 +349,10 @@ zuc_pmd_enqueue_burst(void *queue_pair, struct rte_crypto_op **ops,
 	struct rte_crypto_op *c_ops[ZUC_MAX_BURST];
 	struct rte_crypto_op *curr_c_op;
 
-	struct zuc_session *prev_sess = NULL, *curr_sess = NULL;
+	struct zuc_session *curr_sess;
+	struct zuc_session *sessions[ZUC_MAX_BURST];
+	enum zuc_operation prev_zuc_op = ZUC_OP_NOT_SUPPORTED;
+	enum zuc_operation curr_zuc_op;
 	struct zuc_qp *qp = queue_pair;
 	unsigned i;
 	uint8_t burst_size = 0;
@@ -359,50 +369,63 @@ zuc_pmd_enqueue_burst(void *queue_pair, struct rte_crypto_op **ops,
 			break;
 		}
 
-		/* Batch ops that share the same session. */
-		if (prev_sess == NULL) {
-			prev_sess = curr_sess;
-			c_ops[burst_size++] = curr_c_op;
-		} else if (curr_sess == prev_sess) {
-			c_ops[burst_size++] = curr_c_op;
+		curr_zuc_op = curr_sess->op;
+
+		/*
+		 * Batch ops that share the same operation type
+		 * (cipher only, auth only...).
+		 */
+		if (burst_size == 0) {
+			prev_zuc_op = curr_zuc_op;
+			c_ops[0] = curr_c_op;
+			sessions[0] = curr_sess;
+			burst_size++;
+		} else if (curr_zuc_op == prev_zuc_op) {
+			c_ops[burst_size] = curr_c_op;
+			sessions[burst_size] = curr_sess;
+			burst_size++;
 			/*
 			 * When there are enough ops to process in a batch,
 			 * process them, and start a new batch.
 			 */
 			if (burst_size == ZUC_MAX_BURST) {
-				processed_ops = process_ops(c_ops, prev_sess,
-						qp, burst_size, &enqueued_ops);
+				processed_ops = process_ops(c_ops, curr_zuc_op,
+						sessions, qp, burst_size,
+						&enqueued_ops);
 				if (processed_ops < burst_size) {
 					burst_size = 0;
 					break;
 				}
 
 				burst_size = 0;
-				prev_sess = NULL;
 			}
 		} else {
 			/*
-			 * Different session, process the ops
-			 * of the previous session.
+			 * Different operation type, process the ops
+			 * of the previous type.
 			 */
-			processed_ops = process_ops(c_ops, prev_sess,
-					qp, burst_size, &enqueued_ops);
+			processed_ops = process_ops(c_ops, prev_zuc_op,
+					sessions, qp, burst_size,
+					&enqueued_ops);
 			if (processed_ops < burst_size) {
 				burst_size = 0;
 				break;
 			}
 
 			burst_size = 0;
-			prev_sess = curr_sess;
+			prev_zuc_op = curr_zuc_op;
 
-			c_ops[burst_size++] = curr_c_op;
+			c_ops[0] = curr_c_op;
+			sessions[0] = curr_sess;
+			burst_size++;
 		}
 	}
 
 	if (burst_size != 0) {
-		/* Process the crypto ops of the last session. */
-		processed_ops = process_ops(c_ops, prev_sess,
-				qp, burst_size, &enqueued_ops);
+		/* Process the crypto ops of the last operation type. */
+		processed_ops = process_ops(c_ops, prev_zuc_op,
+				sessions, qp, burst_size,
+				&enqueued_ops);
 	}
 
 	qp->qp_stats.enqueue_err_count += nb_ops - enqueued_ops;
-- 
2.14.3

Re: [dpdk-dev] [PATCH v2 0/3] ZUC PMD fixes

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: De Lara Guarch, Pablo
> Sent: Thursday, April 19, 2018 3:56 PM
> To: dev@dpdk.org
> Cc: Trahe, Fiona <fiona.trahe@intel.com>; De Lara Guarch, Pablo
> <pablo.de.lara.guarch@intel.com>
> Subject: [PATCH v2 0/3] ZUC PMD fixes
> 
> ZUC library provides an API to encrypt buffers in parallel with different keys.
> However, the PMD was developed assuming
> that all needed to share the same session (therefore, the same key).
> 
> This patchset fixes this behaviour by allowing multiple buffers with different keys
> to be processed in parallel, plus it removes some unnecessary code.
> 
> Changes in v2:
> - Fixed incorrect comments [Fiona]
> 
> Pablo de Lara (3):
>   crypto/zuc: do not set default op status
>   crypto/zuc: remove unnecessary check
>   crypto/zuc: batch ops with same transform
> 
>  drivers/crypto/zuc/rte_zuc_pmd.c | 111 +++++++++++++++++++++++-------------
> ---
>  1 file changed, 65 insertions(+), 46 deletions(-)
> 
> --
> 2.14.3

Series applied to dpdk-next-crypto.

Pablo