[dpdk-dev] [PATCH] doc: announce change in crypto adapter metadata

[dpdk-dev] [PATCH] doc: announce change in crypto adapter metadata

[Shijith Thotton]

In crypto adapter metadata, first 8 bytes of request info is a space
holder for response info. For better clarity, reserved field should be
removed from request info. New space for response info can be made by
changing type of event crypto metadata to structure from union.

Signed-off-by: Shijith Thotton <sthotton@marvell.com>
---
 doc/guides/rel_notes/deprecation.rst | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index 9584d6bfd7..c131f1a947 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -147,3 +147,9 @@ Deprecation Notices
 * cmdline: ``cmdline`` structure will be made opaque to hide platform-specific
   content. On Linux and FreeBSD, supported prior to DPDK 20.11,
   original structure will be kept until DPDK 21.11.
+
+* eventdev: Reserved bytes of ``rte_event_crypto_request`` is a space holder
+  for ``response_info``. Both should be decoupled for better clarity. New
+  space for ``response_info`` can be made by changing
+  ``rte_event_crypto_metadata`` type to structure from union. This change is
+  targeted for DPDK 21.11.
-- 
2.25.1

Re: [dpdk-dev] [PATCH] doc: announce change in crypto adapter metadata

[Akhil Goyal]

> Subject: [PATCH] doc: announce change in crypto adapter metadata
> 
> In crypto adapter metadata, first 8 bytes of request info is a space
> holder for response info. For better clarity, reserved field should be
> removed from request info. New space for response info can be made by
> changing type of event crypto metadata to structure from union.
> 
> Signed-off-by: Shijith Thotton <sthotton@marvell.com>
Acked-by: Akhil Goyal <gakhil@marvell.com>

[dpdk-dev] [RFC] eventdev: update crypto adapter metadata structures

[Shijith Thotton]

In crypto adapter metadata, reserved bytes in request info structure is
a space holder for response info. It enforces an order of operation if
the structures are updated using memcpy to avoid overwriting response
info. It is logical to move the reserved space out of request info. It
also solves the ordering issue mentioned above.

This patch removes the reserve field from request info and makes event
crypto metadata type to structure from union to make space for response
info.

App and drivers are updated as per metadata change.

Signed-off-by: Shijith Thotton <sthotton@marvell.com>
---
 app/test/test_event_crypto_adapter.c              | 10 +++++-----
 drivers/crypto/octeontx/otx_cryptodev_ops.c       |  8 ++++----
 drivers/crypto/octeontx2/otx2_cryptodev_ops.c     |  4 ++--
 .../event/octeontx2/otx2_evdev_crypto_adptr_tx.h  |  4 ++--
 lib/eventdev/rte_event_crypto_adapter.c           |  8 ++++----
 lib/eventdev/rte_event_crypto_adapter.h           | 15 +++++----------
 6 files changed, 22 insertions(+), 27 deletions(-)

diff --git a/app/test/test_event_crypto_adapter.c b/app/test/test_event_crypto_adapter.c
index 3ad20921e2..8344719186 100644
--- a/app/test/test_event_crypto_adapter.c
+++ b/app/test/test_event_crypto_adapter.c
@@ -168,7 +168,7 @@ test_op_forward_mode(uint8_t session_less)
 {
 	struct rte_crypto_sym_xform cipher_xform;
 	struct rte_cryptodev_sym_session *sess;
-	union rte_event_crypto_metadata m_data;
+	struct rte_event_crypto_metadata m_data;
 	struct rte_crypto_sym_op *sym_op;
 	struct rte_crypto_op *op;
 	struct rte_mbuf *m;
@@ -368,7 +368,7 @@ test_op_new_mode(uint8_t session_less)
 {
 	struct rte_crypto_sym_xform cipher_xform;
 	struct rte_cryptodev_sym_session *sess;
-	union rte_event_crypto_metadata m_data;
+	struct rte_event_crypto_metadata m_data;
 	struct rte_crypto_sym_op *sym_op;
 	struct rte_crypto_op *op;
 	struct rte_mbuf *m;
@@ -519,7 +519,7 @@ configure_cryptodev(void)
 			DEFAULT_NUM_XFORMS *
 			sizeof(struct rte_crypto_sym_xform) +
 			MAXIMUM_IV_LENGTH +
-			sizeof(union rte_event_crypto_metadata),
+			sizeof(struct rte_event_crypto_metadata),
 			rte_socket_id());
 	if (params.op_mpool == NULL) {
 		RTE_LOG(ERR, USER1, "Can't create CRYPTO_OP_POOL\n");
@@ -549,12 +549,12 @@ configure_cryptodev(void)
 	 * to include the session headers & private data
 	 */
 	session_size = rte_cryptodev_sym_get_private_session_size(TEST_CDEV_ID);
-	session_size += sizeof(union rte_event_crypto_metadata);
+	session_size += sizeof(struct rte_event_crypto_metadata);
 
 	params.session_mpool = rte_cryptodev_sym_session_pool_create(
 			"CRYPTO_ADAPTER_SESSION_MP",
 			MAX_NB_SESSIONS, 0, 0,
-			sizeof(union rte_event_crypto_metadata),
+			sizeof(struct rte_event_crypto_metadata),
 			SOCKET_ID_ANY);
 	TEST_ASSERT_NOT_NULL(params.session_mpool,
 			"session mempool allocation failed\n");
diff --git a/drivers/crypto/octeontx/otx_cryptodev_ops.c b/drivers/crypto/octeontx/otx_cryptodev_ops.c
index eac6796cfb..c51be63146 100644
--- a/drivers/crypto/octeontx/otx_cryptodev_ops.c
+++ b/drivers/crypto/octeontx/otx_cryptodev_ops.c
@@ -710,17 +710,17 @@ submit_request_to_sso(struct ssows *ws, uintptr_t req,
 	ssovf_store_pair(add_work, req, ws->grps[rsp_info->queue_id]);
 }
 
-static inline union rte_event_crypto_metadata *
+static inline struct rte_event_crypto_metadata *
 get_event_crypto_mdata(struct rte_crypto_op *op)
 {
-	union rte_event_crypto_metadata *ec_mdata;
+	struct rte_event_crypto_metadata *ec_mdata;
 
 	if (op->sess_type == RTE_CRYPTO_OP_WITH_SESSION)
 		ec_mdata = rte_cryptodev_sym_session_get_user_data(
 							   op->sym->session);
 	else if (op->sess_type == RTE_CRYPTO_OP_SESSIONLESS &&
 		 op->private_data_offset)
-		ec_mdata = (union rte_event_crypto_metadata *)
+		ec_mdata = (struct rte_event_crypto_metadata *)
 			((uint8_t *)op + op->private_data_offset);
 	else
 		return NULL;
@@ -731,7 +731,7 @@ get_event_crypto_mdata(struct rte_crypto_op *op)
 uint16_t __rte_hot
 otx_crypto_adapter_enqueue(void *port, struct rte_crypto_op *op)
 {
-	union rte_event_crypto_metadata *ec_mdata;
+	struct rte_event_crypto_metadata *ec_mdata;
 	struct cpt_instance *instance;
 	struct cpt_request_info *req;
 	struct rte_event *rsp_info;
diff --git a/drivers/crypto/octeontx2/otx2_cryptodev_ops.c b/drivers/crypto/octeontx2/otx2_cryptodev_ops.c
index 42100154cd..952d1352f4 100644
--- a/drivers/crypto/octeontx2/otx2_cryptodev_ops.c
+++ b/drivers/crypto/octeontx2/otx2_cryptodev_ops.c
@@ -453,7 +453,7 @@ otx2_ca_enqueue_req(const struct otx2_cpt_qp *qp,
 		    struct rte_crypto_op *op,
 		    uint64_t cpt_inst_w7)
 {
-	union rte_event_crypto_metadata *m_data;
+	struct rte_event_crypto_metadata *m_data;
 	union cpt_inst_s inst;
 	uint64_t lmt_status;
 
@@ -468,7 +468,7 @@ otx2_ca_enqueue_req(const struct otx2_cpt_qp *qp,
 		}
 	} else if (op->sess_type == RTE_CRYPTO_OP_SESSIONLESS &&
 		   op->private_data_offset) {
-		m_data = (union rte_event_crypto_metadata *)
+		m_data = (struct rte_event_crypto_metadata *)
 			 ((uint8_t *)op +
 			  op->private_data_offset);
 	} else {
diff --git a/drivers/event/octeontx2/otx2_evdev_crypto_adptr_tx.h b/drivers/event/octeontx2/otx2_evdev_crypto_adptr_tx.h
index ecf7eb9f56..458e8306d7 100644
--- a/drivers/event/octeontx2/otx2_evdev_crypto_adptr_tx.h
+++ b/drivers/event/octeontx2/otx2_evdev_crypto_adptr_tx.h
@@ -16,7 +16,7 @@
 static inline uint16_t
 otx2_ca_enq(uintptr_t tag_op, const struct rte_event *ev)
 {
-	union rte_event_crypto_metadata *m_data;
+	struct rte_event_crypto_metadata *m_data;
 	struct rte_crypto_op *crypto_op;
 	struct rte_cryptodev *cdev;
 	struct otx2_cpt_qp *qp;
@@ -37,7 +37,7 @@ otx2_ca_enq(uintptr_t tag_op, const struct rte_event *ev)
 		qp_id = m_data->request_info.queue_pair_id;
 	} else if (crypto_op->sess_type == RTE_CRYPTO_OP_SESSIONLESS &&
 		   crypto_op->private_data_offset) {
-		m_data = (union rte_event_crypto_metadata *)
+		m_data = (struct rte_event_crypto_metadata *)
 			 ((uint8_t *)crypto_op +
 			  crypto_op->private_data_offset);
 		cdev_id = m_data->request_info.cdev_id;
diff --git a/lib/eventdev/rte_event_crypto_adapter.c b/lib/eventdev/rte_event_crypto_adapter.c
index e1d38d383d..6977391ae9 100644
--- a/lib/eventdev/rte_event_crypto_adapter.c
+++ b/lib/eventdev/rte_event_crypto_adapter.c
@@ -333,7 +333,7 @@ eca_enq_to_cryptodev(struct rte_event_crypto_adapter *adapter,
 		 struct rte_event *ev, unsigned int cnt)
 {
 	struct rte_event_crypto_adapter_stats *stats = &adapter->crypto_stats;
-	union rte_event_crypto_metadata *m_data = NULL;
+	struct rte_event_crypto_metadata *m_data = NULL;
 	struct crypto_queue_pair_info *qp_info = NULL;
 	struct rte_crypto_op *crypto_op;
 	unsigned int i, n;
@@ -371,7 +371,7 @@ eca_enq_to_cryptodev(struct rte_event_crypto_adapter *adapter,
 			len++;
 		} else if (crypto_op->sess_type == RTE_CRYPTO_OP_SESSIONLESS &&
 				crypto_op->private_data_offset) {
-			m_data = (union rte_event_crypto_metadata *)
+			m_data = (struct rte_event_crypto_metadata *)
 				 ((uint8_t *)crypto_op +
 					crypto_op->private_data_offset);
 			cdev_id = m_data->request_info.cdev_id;
@@ -504,7 +504,7 @@ eca_ops_enqueue_burst(struct rte_event_crypto_adapter *adapter,
 		  struct rte_crypto_op **ops, uint16_t num)
 {
 	struct rte_event_crypto_adapter_stats *stats = &adapter->crypto_stats;
-	union rte_event_crypto_metadata *m_data = NULL;
+	struct rte_event_crypto_metadata *m_data = NULL;
 	uint8_t event_dev_id = adapter->eventdev_id;
 	uint8_t event_port_id = adapter->event_port_id;
 	struct rte_event events[BATCH_SIZE];
@@ -523,7 +523,7 @@ eca_ops_enqueue_burst(struct rte_event_crypto_adapter *adapter,
 					ops[i]->sym->session);
 		} else if (ops[i]->sess_type == RTE_CRYPTO_OP_SESSIONLESS &&
 				ops[i]->private_data_offset) {
-			m_data = (union rte_event_crypto_metadata *)
+			m_data = (struct rte_event_crypto_metadata *)
 				 ((uint8_t *)ops[i] +
 				  ops[i]->private_data_offset);
 		}
diff --git a/lib/eventdev/rte_event_crypto_adapter.h b/lib/eventdev/rte_event_crypto_adapter.h
index f8c6cca87c..3c24d9d9df 100644
--- a/lib/eventdev/rte_event_crypto_adapter.h
+++ b/lib/eventdev/rte_event_crypto_adapter.h
@@ -200,11 +200,6 @@ enum rte_event_crypto_adapter_mode {
  * provide event request information to the adapter.
  */
 struct rte_event_crypto_request {
-	uint8_t resv[8];
-	/**< Overlaps with first 8 bytes of struct rte_event
-	 * that encode the response event information. Application
-	 * is expected to fill in struct rte_event response_info.
-	 */
 	uint16_t cdev_id;
 	/**< cryptodev ID to be used */
 	uint16_t queue_pair_id;
@@ -223,16 +218,16 @@ struct rte_event_crypto_request {
  * operation. If the transfer is done by SW, event response information
  * will be used by the adapter.
  */
-union rte_event_crypto_metadata {
-	struct rte_event_crypto_request request_info;
-	/**< Request information to be filled in by application
-	 * for RTE_EVENT_CRYPTO_ADAPTER_OP_FORWARD mode.
-	 */
+struct rte_event_crypto_metadata {
 	struct rte_event response_info;
 	/**< Response information to be filled in by application
 	 * for RTE_EVENT_CRYPTO_ADAPTER_OP_NEW and
 	 * RTE_EVENT_CRYPTO_ADAPTER_OP_FORWARD mode.
 	 */
+	struct rte_event_crypto_request request_info;
+	/**< Request information to be filled in by application
+	 * for RTE_EVENT_CRYPTO_ADAPTER_OP_FORWARD mode.
+	 */
 };
 
 /**
-- 
2.25.1

Re: [dpdk-dev] [PATCH] doc: announce change in crypto adapter metadata

[Kinsella, Ray]

On 04/08/2021 19:17, Akhil Goyal wrote:
>> Subject: [PATCH] doc: announce change in crypto adapter metadata
>>
>> In crypto adapter metadata, first 8 bytes of request info is a space
>> holder for response info. For better clarity, reserved field should be
>> removed from request info. New space for response info can be made by
>> changing type of event crypto metadata to structure from union.
>>
>> Signed-off-by: Shijith Thotton <sthotton@marvell.com>
> Acked-by: Akhil Goyal <gakhil@marvell.com>
> 
Acked-by: Ray Kinsella <mdr@ashroe.eu>

Re: [dpdk-dev] [PATCH] doc: announce change in crypto adapter metadata

[Jerin Jacob]

On Thu, Aug 5, 2021 at 3:42 PM Kinsella, Ray <mdr@ashroe.eu> wrote:
>
>
>
> On 04/08/2021 19:17, Akhil Goyal wrote:
> >> Subject: [PATCH] doc: announce change in crypto adapter metadata
> >>
> >> In crypto adapter metadata, first 8 bytes of request info is a space
> >> holder for response info. For better clarity, reserved field should be
> >> removed from request info. New space for response info can be made by
> >> changing type of event crypto metadata to structure from union.
> >>
> >> Signed-off-by: Shijith Thotton <sthotton@marvell.com>
> > Acked-by: Akhil Goyal <gakhil@marvell.com>
> >
> Acked-by: Ray Kinsella <mdr@ashroe.eu>

+ @Gujjar, Abhinandan S

Acked-by: Jerin Jacob <jerinj@marvell.com>

Re: [dpdk-dev] [PATCH] doc: announce change in crypto adapter metadata

[Thomas Monjalon]

> > >> In crypto adapter metadata, first 8 bytes of request info is a space
> > >> holder for response info. For better clarity, reserved field should be
> > >> removed from request info. New space for response info can be made by
> > >> changing type of event crypto metadata to structure from union.
> > >>
> > >> Signed-off-by: Shijith Thotton <sthotton@marvell.com>
> > > Acked-by: Akhil Goyal <gakhil@marvell.com>
> > >
> > Acked-by: Ray Kinsella <mdr@ashroe.eu>
> 
> + @Gujjar, Abhinandan S
> 
> Acked-by: Jerin Jacob <jerinj@marvell.com>

Applied, thanks.

[dpdk-dev] [PATCH] eventdev: update crypto adapter metadata structures

[Shijith Thotton]

In crypto adapter metadata, reserved bytes in request info structure is
a space holder for response info. It enforces an order of operation if
the structures are updated using memcpy to avoid overwriting response
info. It is logical to move the reserved space out of request info. It
also solves the ordering issue mentioned before.

This patch removes the reserve field from request info and makes event
crypto metadata type to structure from union to make space for response
info.

App and drivers are updated as per metadata change.

Signed-off-by: Shijith Thotton <sthotton@marvell.com>
---
v1:
* Rebased.

 app/test/test_event_crypto_adapter.c              | 14 +++++++-------
 drivers/crypto/octeontx/otx_cryptodev_ops.c       |  8 ++++----
 drivers/crypto/octeontx2/otx2_cryptodev_ops.c     |  4 ++--
 .../event/octeontx2/otx2_evdev_crypto_adptr_tx.h  |  4 ++--
 lib/eventdev/rte_event_crypto_adapter.c           |  8 ++++----
 lib/eventdev/rte_event_crypto_adapter.h           | 15 +++++----------
 6 files changed, 24 insertions(+), 29 deletions(-)

diff --git a/app/test/test_event_crypto_adapter.c b/app/test/test_event_crypto_adapter.c
index 3ad20921e2..0d73694d3a 100644
--- a/app/test/test_event_crypto_adapter.c
+++ b/app/test/test_event_crypto_adapter.c
@@ -168,7 +168,7 @@ test_op_forward_mode(uint8_t session_less)
 {
 	struct rte_crypto_sym_xform cipher_xform;
 	struct rte_cryptodev_sym_session *sess;
-	union rte_event_crypto_metadata m_data;
+	struct rte_event_crypto_metadata m_data;
 	struct rte_crypto_sym_op *sym_op;
 	struct rte_crypto_op *op;
 	struct rte_mbuf *m;
@@ -368,7 +368,7 @@ test_op_new_mode(uint8_t session_less)
 {
 	struct rte_crypto_sym_xform cipher_xform;
 	struct rte_cryptodev_sym_session *sess;
-	union rte_event_crypto_metadata m_data;
+	struct rte_event_crypto_metadata m_data;
 	struct rte_crypto_sym_op *sym_op;
 	struct rte_crypto_op *op;
 	struct rte_mbuf *m;
@@ -406,7 +406,7 @@ test_op_new_mode(uint8_t session_less)
 		if (cap & RTE_EVENT_CRYPTO_ADAPTER_CAP_SESSION_PRIVATE_DATA) {
 			/* Fill in private user data information */
 			rte_memcpy(&m_data.response_info, &response_info,
-				   sizeof(m_data));
+				   sizeof(response_info));
 			rte_cryptodev_sym_session_set_user_data(sess,
 						&m_data, sizeof(m_data));
 		}
@@ -426,7 +426,7 @@ test_op_new_mode(uint8_t session_less)
 		op->private_data_offset = len;
 		/* Fill in private data information */
 		rte_memcpy(&m_data.response_info, &response_info,
-			   sizeof(m_data));
+			   sizeof(response_info));
 		rte_memcpy((uint8_t *)op + len, &m_data, sizeof(m_data));
 	}
 
@@ -519,7 +519,7 @@ configure_cryptodev(void)
 			DEFAULT_NUM_XFORMS *
 			sizeof(struct rte_crypto_sym_xform) +
 			MAXIMUM_IV_LENGTH +
-			sizeof(union rte_event_crypto_metadata),
+			sizeof(struct rte_event_crypto_metadata),
 			rte_socket_id());
 	if (params.op_mpool == NULL) {
 		RTE_LOG(ERR, USER1, "Can't create CRYPTO_OP_POOL\n");
@@ -549,12 +549,12 @@ configure_cryptodev(void)
 	 * to include the session headers & private data
 	 */
 	session_size = rte_cryptodev_sym_get_private_session_size(TEST_CDEV_ID);
-	session_size += sizeof(union rte_event_crypto_metadata);
+	session_size += sizeof(struct rte_event_crypto_metadata);
 
 	params.session_mpool = rte_cryptodev_sym_session_pool_create(
 			"CRYPTO_ADAPTER_SESSION_MP",
 			MAX_NB_SESSIONS, 0, 0,
-			sizeof(union rte_event_crypto_metadata),
+			sizeof(struct rte_event_crypto_metadata),
 			SOCKET_ID_ANY);
 	TEST_ASSERT_NOT_NULL(params.session_mpool,
 			"session mempool allocation failed\n");
diff --git a/drivers/crypto/octeontx/otx_cryptodev_ops.c b/drivers/crypto/octeontx/otx_cryptodev_ops.c
index eac6796cfb..c51be63146 100644
--- a/drivers/crypto/octeontx/otx_cryptodev_ops.c
+++ b/drivers/crypto/octeontx/otx_cryptodev_ops.c
@@ -710,17 +710,17 @@ submit_request_to_sso(struct ssows *ws, uintptr_t req,
 	ssovf_store_pair(add_work, req, ws->grps[rsp_info->queue_id]);
 }
 
-static inline union rte_event_crypto_metadata *
+static inline struct rte_event_crypto_metadata *
 get_event_crypto_mdata(struct rte_crypto_op *op)
 {
-	union rte_event_crypto_metadata *ec_mdata;
+	struct rte_event_crypto_metadata *ec_mdata;
 
 	if (op->sess_type == RTE_CRYPTO_OP_WITH_SESSION)
 		ec_mdata = rte_cryptodev_sym_session_get_user_data(
 							   op->sym->session);
 	else if (op->sess_type == RTE_CRYPTO_OP_SESSIONLESS &&
 		 op->private_data_offset)
-		ec_mdata = (union rte_event_crypto_metadata *)
+		ec_mdata = (struct rte_event_crypto_metadata *)
 			((uint8_t *)op + op->private_data_offset);
 	else
 		return NULL;
@@ -731,7 +731,7 @@ get_event_crypto_mdata(struct rte_crypto_op *op)
 uint16_t __rte_hot
 otx_crypto_adapter_enqueue(void *port, struct rte_crypto_op *op)
 {
-	union rte_event_crypto_metadata *ec_mdata;
+	struct rte_event_crypto_metadata *ec_mdata;
 	struct cpt_instance *instance;
 	struct cpt_request_info *req;
 	struct rte_event *rsp_info;
diff --git a/drivers/crypto/octeontx2/otx2_cryptodev_ops.c b/drivers/crypto/octeontx2/otx2_cryptodev_ops.c
index 42100154cd..952d1352f4 100644
--- a/drivers/crypto/octeontx2/otx2_cryptodev_ops.c
+++ b/drivers/crypto/octeontx2/otx2_cryptodev_ops.c
@@ -453,7 +453,7 @@ otx2_ca_enqueue_req(const struct otx2_cpt_qp *qp,
 		    struct rte_crypto_op *op,
 		    uint64_t cpt_inst_w7)
 {
-	union rte_event_crypto_metadata *m_data;
+	struct rte_event_crypto_metadata *m_data;
 	union cpt_inst_s inst;
 	uint64_t lmt_status;
 
@@ -468,7 +468,7 @@ otx2_ca_enqueue_req(const struct otx2_cpt_qp *qp,
 		}
 	} else if (op->sess_type == RTE_CRYPTO_OP_SESSIONLESS &&
 		   op->private_data_offset) {
-		m_data = (union rte_event_crypto_metadata *)
+		m_data = (struct rte_event_crypto_metadata *)
 			 ((uint8_t *)op +
 			  op->private_data_offset);
 	} else {
diff --git a/drivers/event/octeontx2/otx2_evdev_crypto_adptr_tx.h b/drivers/event/octeontx2/otx2_evdev_crypto_adptr_tx.h
index ecf7eb9f56..458e8306d7 100644
--- a/drivers/event/octeontx2/otx2_evdev_crypto_adptr_tx.h
+++ b/drivers/event/octeontx2/otx2_evdev_crypto_adptr_tx.h
@@ -16,7 +16,7 @@
 static inline uint16_t
 otx2_ca_enq(uintptr_t tag_op, const struct rte_event *ev)
 {
-	union rte_event_crypto_metadata *m_data;
+	struct rte_event_crypto_metadata *m_data;
 	struct rte_crypto_op *crypto_op;
 	struct rte_cryptodev *cdev;
 	struct otx2_cpt_qp *qp;
@@ -37,7 +37,7 @@ otx2_ca_enq(uintptr_t tag_op, const struct rte_event *ev)
 		qp_id = m_data->request_info.queue_pair_id;
 	} else if (crypto_op->sess_type == RTE_CRYPTO_OP_SESSIONLESS &&
 		   crypto_op->private_data_offset) {
-		m_data = (union rte_event_crypto_metadata *)
+		m_data = (struct rte_event_crypto_metadata *)
 			 ((uint8_t *)crypto_op +
 			  crypto_op->private_data_offset);
 		cdev_id = m_data->request_info.cdev_id;
diff --git a/lib/eventdev/rte_event_crypto_adapter.c b/lib/eventdev/rte_event_crypto_adapter.c
index e1d38d383d..6977391ae9 100644
--- a/lib/eventdev/rte_event_crypto_adapter.c
+++ b/lib/eventdev/rte_event_crypto_adapter.c
@@ -333,7 +333,7 @@ eca_enq_to_cryptodev(struct rte_event_crypto_adapter *adapter,
 		 struct rte_event *ev, unsigned int cnt)
 {
 	struct rte_event_crypto_adapter_stats *stats = &adapter->crypto_stats;
-	union rte_event_crypto_metadata *m_data = NULL;
+	struct rte_event_crypto_metadata *m_data = NULL;
 	struct crypto_queue_pair_info *qp_info = NULL;
 	struct rte_crypto_op *crypto_op;
 	unsigned int i, n;
@@ -371,7 +371,7 @@ eca_enq_to_cryptodev(struct rte_event_crypto_adapter *adapter,
 			len++;
 		} else if (crypto_op->sess_type == RTE_CRYPTO_OP_SESSIONLESS &&
 				crypto_op->private_data_offset) {
-			m_data = (union rte_event_crypto_metadata *)
+			m_data = (struct rte_event_crypto_metadata *)
 				 ((uint8_t *)crypto_op +
 					crypto_op->private_data_offset);
 			cdev_id = m_data->request_info.cdev_id;
@@ -504,7 +504,7 @@ eca_ops_enqueue_burst(struct rte_event_crypto_adapter *adapter,
 		  struct rte_crypto_op **ops, uint16_t num)
 {
 	struct rte_event_crypto_adapter_stats *stats = &adapter->crypto_stats;
-	union rte_event_crypto_metadata *m_data = NULL;
+	struct rte_event_crypto_metadata *m_data = NULL;
 	uint8_t event_dev_id = adapter->eventdev_id;
 	uint8_t event_port_id = adapter->event_port_id;
 	struct rte_event events[BATCH_SIZE];
@@ -523,7 +523,7 @@ eca_ops_enqueue_burst(struct rte_event_crypto_adapter *adapter,
 					ops[i]->sym->session);
 		} else if (ops[i]->sess_type == RTE_CRYPTO_OP_SESSIONLESS &&
 				ops[i]->private_data_offset) {
-			m_data = (union rte_event_crypto_metadata *)
+			m_data = (struct rte_event_crypto_metadata *)
 				 ((uint8_t *)ops[i] +
 				  ops[i]->private_data_offset);
 		}
diff --git a/lib/eventdev/rte_event_crypto_adapter.h b/lib/eventdev/rte_event_crypto_adapter.h
index f8c6cca87c..3c24d9d9df 100644
--- a/lib/eventdev/rte_event_crypto_adapter.h
+++ b/lib/eventdev/rte_event_crypto_adapter.h
@@ -200,11 +200,6 @@ enum rte_event_crypto_adapter_mode {
  * provide event request information to the adapter.
  */
 struct rte_event_crypto_request {
-	uint8_t resv[8];
-	/**< Overlaps with first 8 bytes of struct rte_event
-	 * that encode the response event information. Application
-	 * is expected to fill in struct rte_event response_info.
-	 */
 	uint16_t cdev_id;
 	/**< cryptodev ID to be used */
 	uint16_t queue_pair_id;
@@ -223,16 +218,16 @@ struct rte_event_crypto_request {
  * operation. If the transfer is done by SW, event response information
  * will be used by the adapter.
  */
-union rte_event_crypto_metadata {
-	struct rte_event_crypto_request request_info;
-	/**< Request information to be filled in by application
-	 * for RTE_EVENT_CRYPTO_ADAPTER_OP_FORWARD mode.
-	 */
+struct rte_event_crypto_metadata {
 	struct rte_event response_info;
 	/**< Response information to be filled in by application
 	 * for RTE_EVENT_CRYPTO_ADAPTER_OP_NEW and
 	 * RTE_EVENT_CRYPTO_ADAPTER_OP_FORWARD mode.
 	 */
+	struct rte_event_crypto_request request_info;
+	/**< Request information to be filled in by application
+	 * for RTE_EVENT_CRYPTO_ADAPTER_OP_FORWARD mode.
+	 */
 };
 
 /**
-- 
2.25.1

[dpdk-dev] [PATCH v2] eventdev: update crypto adapter metadata structures

[Shijith Thotton]

In crypto adapter metadata, reserved bytes in request info structure is
a space holder for response info. It enforces an order of operation if
the structures are updated using memcpy to avoid overwriting response
info. It is logical to move the reserved space out of request info. It
also solves the ordering issue mentioned before.

This patch removes the reserve field from request info and makes event
crypto metadata type to structure from union to make space for response
info.

App and drivers are updated as per metadata change.

Signed-off-by: Shijith Thotton <sthotton@marvell.com>
---
v2:
* Updated deprecation notice.

v1:
* Rebased.

 app/test/test_event_crypto_adapter.c              | 14 +++++++-------
 doc/guides/rel_notes/deprecation.rst              |  6 ------
 drivers/crypto/octeontx/otx_cryptodev_ops.c       |  8 ++++----
 drivers/crypto/octeontx2/otx2_cryptodev_ops.c     |  4 ++--
 .../event/octeontx2/otx2_evdev_crypto_adptr_tx.h  |  4 ++--
 lib/eventdev/rte_event_crypto_adapter.c           |  8 ++++----
 lib/eventdev/rte_event_crypto_adapter.h           | 15 +++++----------
 7 files changed, 24 insertions(+), 35 deletions(-)

diff --git a/app/test/test_event_crypto_adapter.c b/app/test/test_event_crypto_adapter.c
index 3ad20921e2..0d73694d3a 100644
--- a/app/test/test_event_crypto_adapter.c
+++ b/app/test/test_event_crypto_adapter.c
@@ -168,7 +168,7 @@ test_op_forward_mode(uint8_t session_less)
 {
 	struct rte_crypto_sym_xform cipher_xform;
 	struct rte_cryptodev_sym_session *sess;
-	union rte_event_crypto_metadata m_data;
+	struct rte_event_crypto_metadata m_data;
 	struct rte_crypto_sym_op *sym_op;
 	struct rte_crypto_op *op;
 	struct rte_mbuf *m;
@@ -368,7 +368,7 @@ test_op_new_mode(uint8_t session_less)
 {
 	struct rte_crypto_sym_xform cipher_xform;
 	struct rte_cryptodev_sym_session *sess;
-	union rte_event_crypto_metadata m_data;
+	struct rte_event_crypto_metadata m_data;
 	struct rte_crypto_sym_op *sym_op;
 	struct rte_crypto_op *op;
 	struct rte_mbuf *m;
@@ -406,7 +406,7 @@ test_op_new_mode(uint8_t session_less)
 		if (cap & RTE_EVENT_CRYPTO_ADAPTER_CAP_SESSION_PRIVATE_DATA) {
 			/* Fill in private user data information */
 			rte_memcpy(&m_data.response_info, &response_info,
-				   sizeof(m_data));
+				   sizeof(response_info));
 			rte_cryptodev_sym_session_set_user_data(sess,
 						&m_data, sizeof(m_data));
 		}
@@ -426,7 +426,7 @@ test_op_new_mode(uint8_t session_less)
 		op->private_data_offset = len;
 		/* Fill in private data information */
 		rte_memcpy(&m_data.response_info, &response_info,
-			   sizeof(m_data));
+			   sizeof(response_info));
 		rte_memcpy((uint8_t *)op + len, &m_data, sizeof(m_data));
 	}
 
@@ -519,7 +519,7 @@ configure_cryptodev(void)
 			DEFAULT_NUM_XFORMS *
 			sizeof(struct rte_crypto_sym_xform) +
 			MAXIMUM_IV_LENGTH +
-			sizeof(union rte_event_crypto_metadata),
+			sizeof(struct rte_event_crypto_metadata),
 			rte_socket_id());
 	if (params.op_mpool == NULL) {
 		RTE_LOG(ERR, USER1, "Can't create CRYPTO_OP_POOL\n");
@@ -549,12 +549,12 @@ configure_cryptodev(void)
 	 * to include the session headers & private data
 	 */
 	session_size = rte_cryptodev_sym_get_private_session_size(TEST_CDEV_ID);
-	session_size += sizeof(union rte_event_crypto_metadata);
+	session_size += sizeof(struct rte_event_crypto_metadata);
 
 	params.session_mpool = rte_cryptodev_sym_session_pool_create(
 			"CRYPTO_ADAPTER_SESSION_MP",
 			MAX_NB_SESSIONS, 0, 0,
-			sizeof(union rte_event_crypto_metadata),
+			sizeof(struct rte_event_crypto_metadata),
 			SOCKET_ID_ANY);
 	TEST_ASSERT_NOT_NULL(params.session_mpool,
 			"session mempool allocation failed\n");
diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index 76a4abfd6b..58ee95c020 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -266,12 +266,6 @@ Deprecation Notices
   values to the function ``rte_event_eth_rx_adapter_queue_add`` using
   the structure ``rte_event_eth_rx_adapter_queue_add``.
 
-* eventdev: Reserved bytes of ``rte_event_crypto_request`` is a space holder
-  for ``response_info``. Both should be decoupled for better clarity.
-  New space for ``response_info`` can be made by changing
-  ``rte_event_crypto_metadata`` type to structure from union.
-  This change is targeted for DPDK 21.11.
-
 * metrics: The function ``rte_metrics_init`` will have a non-void return
   in order to notify errors instead of calling ``rte_exit``.
 
diff --git a/drivers/crypto/octeontx/otx_cryptodev_ops.c b/drivers/crypto/octeontx/otx_cryptodev_ops.c
index eac6796cfb..c51be63146 100644
--- a/drivers/crypto/octeontx/otx_cryptodev_ops.c
+++ b/drivers/crypto/octeontx/otx_cryptodev_ops.c
@@ -710,17 +710,17 @@ submit_request_to_sso(struct ssows *ws, uintptr_t req,
 	ssovf_store_pair(add_work, req, ws->grps[rsp_info->queue_id]);
 }
 
-static inline union rte_event_crypto_metadata *
+static inline struct rte_event_crypto_metadata *
 get_event_crypto_mdata(struct rte_crypto_op *op)
 {
-	union rte_event_crypto_metadata *ec_mdata;
+	struct rte_event_crypto_metadata *ec_mdata;
 
 	if (op->sess_type == RTE_CRYPTO_OP_WITH_SESSION)
 		ec_mdata = rte_cryptodev_sym_session_get_user_data(
 							   op->sym->session);
 	else if (op->sess_type == RTE_CRYPTO_OP_SESSIONLESS &&
 		 op->private_data_offset)
-		ec_mdata = (union rte_event_crypto_metadata *)
+		ec_mdata = (struct rte_event_crypto_metadata *)
 			((uint8_t *)op + op->private_data_offset);
 	else
 		return NULL;
@@ -731,7 +731,7 @@ get_event_crypto_mdata(struct rte_crypto_op *op)
 uint16_t __rte_hot
 otx_crypto_adapter_enqueue(void *port, struct rte_crypto_op *op)
 {
-	union rte_event_crypto_metadata *ec_mdata;
+	struct rte_event_crypto_metadata *ec_mdata;
 	struct cpt_instance *instance;
 	struct cpt_request_info *req;
 	struct rte_event *rsp_info;
diff --git a/drivers/crypto/octeontx2/otx2_cryptodev_ops.c b/drivers/crypto/octeontx2/otx2_cryptodev_ops.c
index 42100154cd..952d1352f4 100644
--- a/drivers/crypto/octeontx2/otx2_cryptodev_ops.c
+++ b/drivers/crypto/octeontx2/otx2_cryptodev_ops.c
@@ -453,7 +453,7 @@ otx2_ca_enqueue_req(const struct otx2_cpt_qp *qp,
 		    struct rte_crypto_op *op,
 		    uint64_t cpt_inst_w7)
 {
-	union rte_event_crypto_metadata *m_data;
+	struct rte_event_crypto_metadata *m_data;
 	union cpt_inst_s inst;
 	uint64_t lmt_status;
 
@@ -468,7 +468,7 @@ otx2_ca_enqueue_req(const struct otx2_cpt_qp *qp,
 		}
 	} else if (op->sess_type == RTE_CRYPTO_OP_SESSIONLESS &&
 		   op->private_data_offset) {
-		m_data = (union rte_event_crypto_metadata *)
+		m_data = (struct rte_event_crypto_metadata *)
 			 ((uint8_t *)op +
 			  op->private_data_offset);
 	} else {
diff --git a/drivers/event/octeontx2/otx2_evdev_crypto_adptr_tx.h b/drivers/event/octeontx2/otx2_evdev_crypto_adptr_tx.h
index ecf7eb9f56..458e8306d7 100644
--- a/drivers/event/octeontx2/otx2_evdev_crypto_adptr_tx.h
+++ b/drivers/event/octeontx2/otx2_evdev_crypto_adptr_tx.h
@@ -16,7 +16,7 @@
 static inline uint16_t
 otx2_ca_enq(uintptr_t tag_op, const struct rte_event *ev)
 {
-	union rte_event_crypto_metadata *m_data;
+	struct rte_event_crypto_metadata *m_data;
 	struct rte_crypto_op *crypto_op;
 	struct rte_cryptodev *cdev;
 	struct otx2_cpt_qp *qp;
@@ -37,7 +37,7 @@ otx2_ca_enq(uintptr_t tag_op, const struct rte_event *ev)
 		qp_id = m_data->request_info.queue_pair_id;
 	} else if (crypto_op->sess_type == RTE_CRYPTO_OP_SESSIONLESS &&
 		   crypto_op->private_data_offset) {
-		m_data = (union rte_event_crypto_metadata *)
+		m_data = (struct rte_event_crypto_metadata *)
 			 ((uint8_t *)crypto_op +
 			  crypto_op->private_data_offset);
 		cdev_id = m_data->request_info.cdev_id;
diff --git a/lib/eventdev/rte_event_crypto_adapter.c b/lib/eventdev/rte_event_crypto_adapter.c
index e1d38d383d..6977391ae9 100644
--- a/lib/eventdev/rte_event_crypto_adapter.c
+++ b/lib/eventdev/rte_event_crypto_adapter.c
@@ -333,7 +333,7 @@ eca_enq_to_cryptodev(struct rte_event_crypto_adapter *adapter,
 		 struct rte_event *ev, unsigned int cnt)
 {
 	struct rte_event_crypto_adapter_stats *stats = &adapter->crypto_stats;
-	union rte_event_crypto_metadata *m_data = NULL;
+	struct rte_event_crypto_metadata *m_data = NULL;
 	struct crypto_queue_pair_info *qp_info = NULL;
 	struct rte_crypto_op *crypto_op;
 	unsigned int i, n;
@@ -371,7 +371,7 @@ eca_enq_to_cryptodev(struct rte_event_crypto_adapter *adapter,
 			len++;
 		} else if (crypto_op->sess_type == RTE_CRYPTO_OP_SESSIONLESS &&
 				crypto_op->private_data_offset) {
-			m_data = (union rte_event_crypto_metadata *)
+			m_data = (struct rte_event_crypto_metadata *)
 				 ((uint8_t *)crypto_op +
 					crypto_op->private_data_offset);
 			cdev_id = m_data->request_info.cdev_id;
@@ -504,7 +504,7 @@ eca_ops_enqueue_burst(struct rte_event_crypto_adapter *adapter,
 		  struct rte_crypto_op **ops, uint16_t num)
 {
 	struct rte_event_crypto_adapter_stats *stats = &adapter->crypto_stats;
-	union rte_event_crypto_metadata *m_data = NULL;
+	struct rte_event_crypto_metadata *m_data = NULL;
 	uint8_t event_dev_id = adapter->eventdev_id;
 	uint8_t event_port_id = adapter->event_port_id;
 	struct rte_event events[BATCH_SIZE];
@@ -523,7 +523,7 @@ eca_ops_enqueue_burst(struct rte_event_crypto_adapter *adapter,
 					ops[i]->sym->session);
 		} else if (ops[i]->sess_type == RTE_CRYPTO_OP_SESSIONLESS &&
 				ops[i]->private_data_offset) {
-			m_data = (union rte_event_crypto_metadata *)
+			m_data = (struct rte_event_crypto_metadata *)
 				 ((uint8_t *)ops[i] +
 				  ops[i]->private_data_offset);
 		}
diff --git a/lib/eventdev/rte_event_crypto_adapter.h b/lib/eventdev/rte_event_crypto_adapter.h
index f8c6cca87c..3c24d9d9df 100644
--- a/lib/eventdev/rte_event_crypto_adapter.h
+++ b/lib/eventdev/rte_event_crypto_adapter.h
@@ -200,11 +200,6 @@ enum rte_event_crypto_adapter_mode {
  * provide event request information to the adapter.
  */
 struct rte_event_crypto_request {
-	uint8_t resv[8];
-	/**< Overlaps with first 8 bytes of struct rte_event
-	 * that encode the response event information. Application
-	 * is expected to fill in struct rte_event response_info.
-	 */
 	uint16_t cdev_id;
 	/**< cryptodev ID to be used */
 	uint16_t queue_pair_id;
@@ -223,16 +218,16 @@ struct rte_event_crypto_request {
  * operation. If the transfer is done by SW, event response information
  * will be used by the adapter.
  */
-union rte_event_crypto_metadata {
-	struct rte_event_crypto_request request_info;
-	/**< Request information to be filled in by application
-	 * for RTE_EVENT_CRYPTO_ADAPTER_OP_FORWARD mode.
-	 */
+struct rte_event_crypto_metadata {
 	struct rte_event response_info;
 	/**< Response information to be filled in by application
 	 * for RTE_EVENT_CRYPTO_ADAPTER_OP_NEW and
 	 * RTE_EVENT_CRYPTO_ADAPTER_OP_FORWARD mode.
 	 */
+	struct rte_event_crypto_request request_info;
+	/**< Request information to be filled in by application
+	 * for RTE_EVENT_CRYPTO_ADAPTER_OP_FORWARD mode.
+	 */
 };
 
 /**
-- 
2.25.1

Re: [dpdk-dev] [PATCH v2] eventdev: update crypto adapter metadata structures

[Anoob Joseph]

> 
> In crypto adapter metadata, reserved bytes in request info structure is a space
> holder for response info. It enforces an order of operation if the structures are
> updated using memcpy to avoid overwriting response info. It is logical to move
> the reserved space out of request info. It also solves the ordering issue
> mentioned before.
> 
> This patch removes the reserve field from request info and makes event crypto
> metadata type to structure from union to make space for response info.
> 
> App and drivers are updated as per metadata change.
> 
> Signed-off-by: Shijith Thotton <sthotton@marvell.com>

Acked-by: Anoob Joseph <anoobj@marvell.com>

Re: [dpdk-dev] [PATCH v2] eventdev: update crypto adapter metadata structures

[Akhil Goyal]

> In crypto adapter metadata, reserved bytes in request info structure is
> a space holder for response info. It enforces an order of operation if
> the structures are updated using memcpy to avoid overwriting response
> info. It is logical to move the reserved space out of request info. It
> also solves the ordering issue mentioned before.
> 
> This patch removes the reserve field from request info and makes event
> crypto metadata type to structure from union to make space for response
> info.
> 
> App and drivers are updated as per metadata change.
> 
> Signed-off-by: Shijith Thotton <sthotton@marvell.com>
> ---
> v2:
> * Updated deprecation notice.
> 
Please also update release notes API/ABI section for the changes introduced in this patch.

Re: [dpdk-dev] [PATCH v2] eventdev: update crypto adapter metadata structures

[Shijith Thotton]

>
>> In crypto adapter metadata, reserved bytes in request info structure is
>> a space holder for response info. It enforces an order of operation if
>> the structures are updated using memcpy to avoid overwriting response
>> info. It is logical to move the reserved space out of request info. It
>> also solves the ordering issue mentioned before.
>>
>> This patch removes the reserve field from request info and makes event
>> crypto metadata type to structure from union to make space for response
>> info.
>>
>> App and drivers are updated as per metadata change.
>>
>> Signed-off-by: Shijith Thotton <sthotton@marvell.com>
>> ---
>> v2:
>> * Updated deprecation notice.
>>
>Please also update release notes API/ABI section for the changes introduced in
>this patch.
I will send v3 with the changes.

[dpdk-dev] [PATCH v3] eventdev: update crypto adapter metadata structures

[Shijith Thotton]

In crypto adapter metadata, reserved bytes in request info structure is
a space holder for response info. It enforces an order of operation if
the structures are updated using memcpy to avoid overwriting response
info. It is logical to move the reserved space out of request info. It
also solves the ordering issue mentioned before.

This patch removes the reserve field from request info and makes event
crypto metadata type to structure from union to make space for response
info.

App and drivers are updated as per metadata change.

Signed-off-by: Shijith Thotton <sthotton@marvell.com>
Acked-by: Anoob Joseph <anoobj@marvell.com>
---
v3:
* Updated ABI section of release notes.

v2:
* Updated deprecation notice.

v1:
* Rebased.

 app/test/test_event_crypto_adapter.c              | 14 +++++++-------
 doc/guides/rel_notes/deprecation.rst              |  6 ------
 doc/guides/rel_notes/release_21_11.rst            |  2 ++
 drivers/crypto/octeontx/otx_cryptodev_ops.c       |  8 ++++----
 drivers/crypto/octeontx2/otx2_cryptodev_ops.c     |  4 ++--
 .../event/octeontx2/otx2_evdev_crypto_adptr_tx.h  |  4 ++--
 lib/eventdev/rte_event_crypto_adapter.c           |  8 ++++----
 lib/eventdev/rte_event_crypto_adapter.h           | 15 +++++----------
 8 files changed, 26 insertions(+), 35 deletions(-)

diff --git a/app/test/test_event_crypto_adapter.c b/app/test/test_event_crypto_adapter.c
index 3ad20921e2..0d73694d3a 100644
--- a/app/test/test_event_crypto_adapter.c
+++ b/app/test/test_event_crypto_adapter.c
@@ -168,7 +168,7 @@ test_op_forward_mode(uint8_t session_less)
 {
 	struct rte_crypto_sym_xform cipher_xform;
 	struct rte_cryptodev_sym_session *sess;
-	union rte_event_crypto_metadata m_data;
+	struct rte_event_crypto_metadata m_data;
 	struct rte_crypto_sym_op *sym_op;
 	struct rte_crypto_op *op;
 	struct rte_mbuf *m;
@@ -368,7 +368,7 @@ test_op_new_mode(uint8_t session_less)
 {
 	struct rte_crypto_sym_xform cipher_xform;
 	struct rte_cryptodev_sym_session *sess;
-	union rte_event_crypto_metadata m_data;
+	struct rte_event_crypto_metadata m_data;
 	struct rte_crypto_sym_op *sym_op;
 	struct rte_crypto_op *op;
 	struct rte_mbuf *m;
@@ -406,7 +406,7 @@ test_op_new_mode(uint8_t session_less)
 		if (cap & RTE_EVENT_CRYPTO_ADAPTER_CAP_SESSION_PRIVATE_DATA) {
 			/* Fill in private user data information */
 			rte_memcpy(&m_data.response_info, &response_info,
-				   sizeof(m_data));
+				   sizeof(response_info));
 			rte_cryptodev_sym_session_set_user_data(sess,
 						&m_data, sizeof(m_data));
 		}
@@ -426,7 +426,7 @@ test_op_new_mode(uint8_t session_less)
 		op->private_data_offset = len;
 		/* Fill in private data information */
 		rte_memcpy(&m_data.response_info, &response_info,
-			   sizeof(m_data));
+			   sizeof(response_info));
 		rte_memcpy((uint8_t *)op + len, &m_data, sizeof(m_data));
 	}
 
@@ -519,7 +519,7 @@ configure_cryptodev(void)
 			DEFAULT_NUM_XFORMS *
 			sizeof(struct rte_crypto_sym_xform) +
 			MAXIMUM_IV_LENGTH +
-			sizeof(union rte_event_crypto_metadata),
+			sizeof(struct rte_event_crypto_metadata),
 			rte_socket_id());
 	if (params.op_mpool == NULL) {
 		RTE_LOG(ERR, USER1, "Can't create CRYPTO_OP_POOL\n");
@@ -549,12 +549,12 @@ configure_cryptodev(void)
 	 * to include the session headers & private data
 	 */
 	session_size = rte_cryptodev_sym_get_private_session_size(TEST_CDEV_ID);
-	session_size += sizeof(union rte_event_crypto_metadata);
+	session_size += sizeof(struct rte_event_crypto_metadata);
 
 	params.session_mpool = rte_cryptodev_sym_session_pool_create(
 			"CRYPTO_ADAPTER_SESSION_MP",
 			MAX_NB_SESSIONS, 0, 0,
-			sizeof(union rte_event_crypto_metadata),
+			sizeof(struct rte_event_crypto_metadata),
 			SOCKET_ID_ANY);
 	TEST_ASSERT_NOT_NULL(params.session_mpool,
 			"session mempool allocation failed\n");
diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index 76a4abfd6b..58ee95c020 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -266,12 +266,6 @@ Deprecation Notices
   values to the function ``rte_event_eth_rx_adapter_queue_add`` using
   the structure ``rte_event_eth_rx_adapter_queue_add``.
 
-* eventdev: Reserved bytes of ``rte_event_crypto_request`` is a space holder
-  for ``response_info``. Both should be decoupled for better clarity.
-  New space for ``response_info`` can be made by changing
-  ``rte_event_crypto_metadata`` type to structure from union.
-  This change is targeted for DPDK 21.11.
-
 * metrics: The function ``rte_metrics_init`` will have a non-void return
   in order to notify errors instead of calling ``rte_exit``.
 
diff --git a/doc/guides/rel_notes/release_21_11.rst b/doc/guides/rel_notes/release_21_11.rst
index d707a554ef..ab76d5dd55 100644
--- a/doc/guides/rel_notes/release_21_11.rst
+++ b/doc/guides/rel_notes/release_21_11.rst
@@ -100,6 +100,8 @@ ABI Changes
    Also, make sure to start the actual text at the margin.
    =======================================================
 
+* eventdev: Modified type of ``union rte_event_crypto_metadata`` to struct and
+  removed reserved bytes from ``struct rte_event_crypto_request``.
 
 Known Issues
 ------------
diff --git a/drivers/crypto/octeontx/otx_cryptodev_ops.c b/drivers/crypto/octeontx/otx_cryptodev_ops.c
index eac6796cfb..c51be63146 100644
--- a/drivers/crypto/octeontx/otx_cryptodev_ops.c
+++ b/drivers/crypto/octeontx/otx_cryptodev_ops.c
@@ -710,17 +710,17 @@ submit_request_to_sso(struct ssows *ws, uintptr_t req,
 	ssovf_store_pair(add_work, req, ws->grps[rsp_info->queue_id]);
 }
 
-static inline union rte_event_crypto_metadata *
+static inline struct rte_event_crypto_metadata *
 get_event_crypto_mdata(struct rte_crypto_op *op)
 {
-	union rte_event_crypto_metadata *ec_mdata;
+	struct rte_event_crypto_metadata *ec_mdata;
 
 	if (op->sess_type == RTE_CRYPTO_OP_WITH_SESSION)
 		ec_mdata = rte_cryptodev_sym_session_get_user_data(
 							   op->sym->session);
 	else if (op->sess_type == RTE_CRYPTO_OP_SESSIONLESS &&
 		 op->private_data_offset)
-		ec_mdata = (union rte_event_crypto_metadata *)
+		ec_mdata = (struct rte_event_crypto_metadata *)
 			((uint8_t *)op + op->private_data_offset);
 	else
 		return NULL;
@@ -731,7 +731,7 @@ get_event_crypto_mdata(struct rte_crypto_op *op)
 uint16_t __rte_hot
 otx_crypto_adapter_enqueue(void *port, struct rte_crypto_op *op)
 {
-	union rte_event_crypto_metadata *ec_mdata;
+	struct rte_event_crypto_metadata *ec_mdata;
 	struct cpt_instance *instance;
 	struct cpt_request_info *req;
 	struct rte_event *rsp_info;
diff --git a/drivers/crypto/octeontx2/otx2_cryptodev_ops.c b/drivers/crypto/octeontx2/otx2_cryptodev_ops.c
index 42100154cd..952d1352f4 100644
--- a/drivers/crypto/octeontx2/otx2_cryptodev_ops.c
+++ b/drivers/crypto/octeontx2/otx2_cryptodev_ops.c
@@ -453,7 +453,7 @@ otx2_ca_enqueue_req(const struct otx2_cpt_qp *qp,
 		    struct rte_crypto_op *op,
 		    uint64_t cpt_inst_w7)
 {
-	union rte_event_crypto_metadata *m_data;
+	struct rte_event_crypto_metadata *m_data;
 	union cpt_inst_s inst;
 	uint64_t lmt_status;
 
@@ -468,7 +468,7 @@ otx2_ca_enqueue_req(const struct otx2_cpt_qp *qp,
 		}
 	} else if (op->sess_type == RTE_CRYPTO_OP_SESSIONLESS &&
 		   op->private_data_offset) {
-		m_data = (union rte_event_crypto_metadata *)
+		m_data = (struct rte_event_crypto_metadata *)
 			 ((uint8_t *)op +
 			  op->private_data_offset);
 	} else {
diff --git a/drivers/event/octeontx2/otx2_evdev_crypto_adptr_tx.h b/drivers/event/octeontx2/otx2_evdev_crypto_adptr_tx.h
index ecf7eb9f56..458e8306d7 100644
--- a/drivers/event/octeontx2/otx2_evdev_crypto_adptr_tx.h
+++ b/drivers/event/octeontx2/otx2_evdev_crypto_adptr_tx.h
@@ -16,7 +16,7 @@
 static inline uint16_t
 otx2_ca_enq(uintptr_t tag_op, const struct rte_event *ev)
 {
-	union rte_event_crypto_metadata *m_data;
+	struct rte_event_crypto_metadata *m_data;
 	struct rte_crypto_op *crypto_op;
 	struct rte_cryptodev *cdev;
 	struct otx2_cpt_qp *qp;
@@ -37,7 +37,7 @@ otx2_ca_enq(uintptr_t tag_op, const struct rte_event *ev)
 		qp_id = m_data->request_info.queue_pair_id;
 	} else if (crypto_op->sess_type == RTE_CRYPTO_OP_SESSIONLESS &&
 		   crypto_op->private_data_offset) {
-		m_data = (union rte_event_crypto_metadata *)
+		m_data = (struct rte_event_crypto_metadata *)
 			 ((uint8_t *)crypto_op +
 			  crypto_op->private_data_offset);
 		cdev_id = m_data->request_info.cdev_id;
diff --git a/lib/eventdev/rte_event_crypto_adapter.c b/lib/eventdev/rte_event_crypto_adapter.c
index e1d38d383d..6977391ae9 100644
--- a/lib/eventdev/rte_event_crypto_adapter.c
+++ b/lib/eventdev/rte_event_crypto_adapter.c
@@ -333,7 +333,7 @@ eca_enq_to_cryptodev(struct rte_event_crypto_adapter *adapter,
 		 struct rte_event *ev, unsigned int cnt)
 {
 	struct rte_event_crypto_adapter_stats *stats = &adapter->crypto_stats;
-	union rte_event_crypto_metadata *m_data = NULL;
+	struct rte_event_crypto_metadata *m_data = NULL;
 	struct crypto_queue_pair_info *qp_info = NULL;
 	struct rte_crypto_op *crypto_op;
 	unsigned int i, n;
@@ -371,7 +371,7 @@ eca_enq_to_cryptodev(struct rte_event_crypto_adapter *adapter,
 			len++;
 		} else if (crypto_op->sess_type == RTE_CRYPTO_OP_SESSIONLESS &&
 				crypto_op->private_data_offset) {
-			m_data = (union rte_event_crypto_metadata *)
+			m_data = (struct rte_event_crypto_metadata *)
 				 ((uint8_t *)crypto_op +
 					crypto_op->private_data_offset);
 			cdev_id = m_data->request_info.cdev_id;
@@ -504,7 +504,7 @@ eca_ops_enqueue_burst(struct rte_event_crypto_adapter *adapter,
 		  struct rte_crypto_op **ops, uint16_t num)
 {
 	struct rte_event_crypto_adapter_stats *stats = &adapter->crypto_stats;
-	union rte_event_crypto_metadata *m_data = NULL;
+	struct rte_event_crypto_metadata *m_data = NULL;
 	uint8_t event_dev_id = adapter->eventdev_id;
 	uint8_t event_port_id = adapter->event_port_id;
 	struct rte_event events[BATCH_SIZE];
@@ -523,7 +523,7 @@ eca_ops_enqueue_burst(struct rte_event_crypto_adapter *adapter,
 					ops[i]->sym->session);
 		} else if (ops[i]->sess_type == RTE_CRYPTO_OP_SESSIONLESS &&
 				ops[i]->private_data_offset) {
-			m_data = (union rte_event_crypto_metadata *)
+			m_data = (struct rte_event_crypto_metadata *)
 				 ((uint8_t *)ops[i] +
 				  ops[i]->private_data_offset);
 		}
diff --git a/lib/eventdev/rte_event_crypto_adapter.h b/lib/eventdev/rte_event_crypto_adapter.h
index f8c6cca87c..3c24d9d9df 100644
--- a/lib/eventdev/rte_event_crypto_adapter.h
+++ b/lib/eventdev/rte_event_crypto_adapter.h
@@ -200,11 +200,6 @@ enum rte_event_crypto_adapter_mode {
  * provide event request information to the adapter.
  */
 struct rte_event_crypto_request {
-	uint8_t resv[8];
-	/**< Overlaps with first 8 bytes of struct rte_event
-	 * that encode the response event information. Application
-	 * is expected to fill in struct rte_event response_info.
-	 */
 	uint16_t cdev_id;
 	/**< cryptodev ID to be used */
 	uint16_t queue_pair_id;
@@ -223,16 +218,16 @@ struct rte_event_crypto_request {
  * operation. If the transfer is done by SW, event response information
  * will be used by the adapter.
  */
-union rte_event_crypto_metadata {
-	struct rte_event_crypto_request request_info;
-	/**< Request information to be filled in by application
-	 * for RTE_EVENT_CRYPTO_ADAPTER_OP_FORWARD mode.
-	 */
+struct rte_event_crypto_metadata {
 	struct rte_event response_info;
 	/**< Response information to be filled in by application
 	 * for RTE_EVENT_CRYPTO_ADAPTER_OP_NEW and
 	 * RTE_EVENT_CRYPTO_ADAPTER_OP_FORWARD mode.
 	 */
+	struct rte_event_crypto_request request_info;
+	/**< Request information to be filled in by application
+	 * for RTE_EVENT_CRYPTO_ADAPTER_OP_FORWARD mode.
+	 */
 };
 
 /**
-- 
2.25.1

Re: [dpdk-dev] [PATCH v3] eventdev: update crypto adapter metadata structures

[Jerin Jacob]

On Tue, Aug 31, 2021 at 1:27 PM Shijith Thotton <sthotton@marvell.com> wrote:
>
> In crypto adapter metadata, reserved bytes in request info structure is
> a space holder for response info. It enforces an order of operation if
> the structures are updated using memcpy to avoid overwriting response
> info. It is logical to move the reserved space out of request info. It
> also solves the ordering issue mentioned before.
>
> This patch removes the reserve field from request info and makes event
> crypto metadata type to structure from union to make space for response
> info.
>
> App and drivers are updated as per metadata change.
>
> Signed-off-by: Shijith Thotton <sthotton@marvell.com>
> Acked-by: Anoob Joseph <anoobj@marvell.com>


@Gujjar, Abhinandan S  @Akhil Goyal

Could you review the crypto adapter-related change?



> ---
> v3:
> * Updated ABI section of release notes.
>
> v2:
> * Updated deprecation notice.
>
> v1:
> * Rebased.
>
>  app/test/test_event_crypto_adapter.c              | 14 +++++++-------
>  doc/guides/rel_notes/deprecation.rst              |  6 ------
>  doc/guides/rel_notes/release_21_11.rst            |  2 ++
>  drivers/crypto/octeontx/otx_cryptodev_ops.c       |  8 ++++----
>  drivers/crypto/octeontx2/otx2_cryptodev_ops.c     |  4 ++--
>  .../event/octeontx2/otx2_evdev_crypto_adptr_tx.h  |  4 ++--
>  lib/eventdev/rte_event_crypto_adapter.c           |  8 ++++----
>  lib/eventdev/rte_event_crypto_adapter.h           | 15 +++++----------
>  8 files changed, 26 insertions(+), 35 deletions(-)
>
> diff --git a/app/test/test_event_crypto_adapter.c b/app/test/test_event_crypto_adapter.c
> index 3ad20921e2..0d73694d3a 100644
> --- a/app/test/test_event_crypto_adapter.c
> +++ b/app/test/test_event_crypto_adapter.c
> @@ -168,7 +168,7 @@ test_op_forward_mode(uint8_t session_less)
>  {
>         struct rte_crypto_sym_xform cipher_xform;
>         struct rte_cryptodev_sym_session *sess;
> -       union rte_event_crypto_metadata m_data;
> +       struct rte_event_crypto_metadata m_data;
>         struct rte_crypto_sym_op *sym_op;
>         struct rte_crypto_op *op;
>         struct rte_mbuf *m;
> @@ -368,7 +368,7 @@ test_op_new_mode(uint8_t session_less)
>  {
>         struct rte_crypto_sym_xform cipher_xform;
>         struct rte_cryptodev_sym_session *sess;
> -       union rte_event_crypto_metadata m_data;
> +       struct rte_event_crypto_metadata m_data;
>         struct rte_crypto_sym_op *sym_op;
>         struct rte_crypto_op *op;
>         struct rte_mbuf *m;
> @@ -406,7 +406,7 @@ test_op_new_mode(uint8_t session_less)
>                 if (cap & RTE_EVENT_CRYPTO_ADAPTER_CAP_SESSION_PRIVATE_DATA) {
>                         /* Fill in private user data information */
>                         rte_memcpy(&m_data.response_info, &response_info,
> -                                  sizeof(m_data));
> +                                  sizeof(response_info));
>                         rte_cryptodev_sym_session_set_user_data(sess,
>                                                 &m_data, sizeof(m_data));
>                 }
> @@ -426,7 +426,7 @@ test_op_new_mode(uint8_t session_less)
>                 op->private_data_offset = len;
>                 /* Fill in private data information */
>                 rte_memcpy(&m_data.response_info, &response_info,
> -                          sizeof(m_data));
> +                          sizeof(response_info));
>                 rte_memcpy((uint8_t *)op + len, &m_data, sizeof(m_data));
>         }
>
> @@ -519,7 +519,7 @@ configure_cryptodev(void)
>                         DEFAULT_NUM_XFORMS *
>                         sizeof(struct rte_crypto_sym_xform) +
>                         MAXIMUM_IV_LENGTH +
> -                       sizeof(union rte_event_crypto_metadata),
> +                       sizeof(struct rte_event_crypto_metadata),
>                         rte_socket_id());
>         if (params.op_mpool == NULL) {
>                 RTE_LOG(ERR, USER1, "Can't create CRYPTO_OP_POOL\n");
> @@ -549,12 +549,12 @@ configure_cryptodev(void)
>          * to include the session headers & private data
>          */
>         session_size = rte_cryptodev_sym_get_private_session_size(TEST_CDEV_ID);
> -       session_size += sizeof(union rte_event_crypto_metadata);
> +       session_size += sizeof(struct rte_event_crypto_metadata);
>
>         params.session_mpool = rte_cryptodev_sym_session_pool_create(
>                         "CRYPTO_ADAPTER_SESSION_MP",
>                         MAX_NB_SESSIONS, 0, 0,
> -                       sizeof(union rte_event_crypto_metadata),
> +                       sizeof(struct rte_event_crypto_metadata),
>                         SOCKET_ID_ANY);
>         TEST_ASSERT_NOT_NULL(params.session_mpool,
>                         "session mempool allocation failed\n");
> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> index 76a4abfd6b..58ee95c020 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -266,12 +266,6 @@ Deprecation Notices
>    values to the function ``rte_event_eth_rx_adapter_queue_add`` using
>    the structure ``rte_event_eth_rx_adapter_queue_add``.
>
> -* eventdev: Reserved bytes of ``rte_event_crypto_request`` is a space holder
> -  for ``response_info``. Both should be decoupled for better clarity.
> -  New space for ``response_info`` can be made by changing
> -  ``rte_event_crypto_metadata`` type to structure from union.
> -  This change is targeted for DPDK 21.11.
> -
>  * metrics: The function ``rte_metrics_init`` will have a non-void return
>    in order to notify errors instead of calling ``rte_exit``.
>
> diff --git a/doc/guides/rel_notes/release_21_11.rst b/doc/guides/rel_notes/release_21_11.rst
> index d707a554ef..ab76d5dd55 100644
> --- a/doc/guides/rel_notes/release_21_11.rst
> +++ b/doc/guides/rel_notes/release_21_11.rst
> @@ -100,6 +100,8 @@ ABI Changes
>     Also, make sure to start the actual text at the margin.
>     =======================================================
>
> +* eventdev: Modified type of ``union rte_event_crypto_metadata`` to struct and
> +  removed reserved bytes from ``struct rte_event_crypto_request``.
>
>  Known Issues
>  ------------
> diff --git a/drivers/crypto/octeontx/otx_cryptodev_ops.c b/drivers/crypto/octeontx/otx_cryptodev_ops.c
> index eac6796cfb..c51be63146 100644
> --- a/drivers/crypto/octeontx/otx_cryptodev_ops.c
> +++ b/drivers/crypto/octeontx/otx_cryptodev_ops.c
> @@ -710,17 +710,17 @@ submit_request_to_sso(struct ssows *ws, uintptr_t req,
>         ssovf_store_pair(add_work, req, ws->grps[rsp_info->queue_id]);
>  }
>
> -static inline union rte_event_crypto_metadata *
> +static inline struct rte_event_crypto_metadata *
>  get_event_crypto_mdata(struct rte_crypto_op *op)
>  {
> -       union rte_event_crypto_metadata *ec_mdata;
> +       struct rte_event_crypto_metadata *ec_mdata;
>
>         if (op->sess_type == RTE_CRYPTO_OP_WITH_SESSION)
>                 ec_mdata = rte_cryptodev_sym_session_get_user_data(
>                                                            op->sym->session);
>         else if (op->sess_type == RTE_CRYPTO_OP_SESSIONLESS &&
>                  op->private_data_offset)
> -               ec_mdata = (union rte_event_crypto_metadata *)
> +               ec_mdata = (struct rte_event_crypto_metadata *)
>                         ((uint8_t *)op + op->private_data_offset);
>         else
>                 return NULL;
> @@ -731,7 +731,7 @@ get_event_crypto_mdata(struct rte_crypto_op *op)
>  uint16_t __rte_hot
>  otx_crypto_adapter_enqueue(void *port, struct rte_crypto_op *op)
>  {
> -       union rte_event_crypto_metadata *ec_mdata;
> +       struct rte_event_crypto_metadata *ec_mdata;
>         struct cpt_instance *instance;
>         struct cpt_request_info *req;
>         struct rte_event *rsp_info;
> diff --git a/drivers/crypto/octeontx2/otx2_cryptodev_ops.c b/drivers/crypto/octeontx2/otx2_cryptodev_ops.c
> index 42100154cd..952d1352f4 100644
> --- a/drivers/crypto/octeontx2/otx2_cryptodev_ops.c
> +++ b/drivers/crypto/octeontx2/otx2_cryptodev_ops.c
> @@ -453,7 +453,7 @@ otx2_ca_enqueue_req(const struct otx2_cpt_qp *qp,
>                     struct rte_crypto_op *op,
>                     uint64_t cpt_inst_w7)
>  {
> -       union rte_event_crypto_metadata *m_data;
> +       struct rte_event_crypto_metadata *m_data;
>         union cpt_inst_s inst;
>         uint64_t lmt_status;
>
> @@ -468,7 +468,7 @@ otx2_ca_enqueue_req(const struct otx2_cpt_qp *qp,
>                 }
>         } else if (op->sess_type == RTE_CRYPTO_OP_SESSIONLESS &&
>                    op->private_data_offset) {
> -               m_data = (union rte_event_crypto_metadata *)
> +               m_data = (struct rte_event_crypto_metadata *)
>                          ((uint8_t *)op +
>                           op->private_data_offset);
>         } else {
> diff --git a/drivers/event/octeontx2/otx2_evdev_crypto_adptr_tx.h b/drivers/event/octeontx2/otx2_evdev_crypto_adptr_tx.h
> index ecf7eb9f56..458e8306d7 100644
> --- a/drivers/event/octeontx2/otx2_evdev_crypto_adptr_tx.h
> +++ b/drivers/event/octeontx2/otx2_evdev_crypto_adptr_tx.h
> @@ -16,7 +16,7 @@
>  static inline uint16_t
>  otx2_ca_enq(uintptr_t tag_op, const struct rte_event *ev)
>  {
> -       union rte_event_crypto_metadata *m_data;
> +       struct rte_event_crypto_metadata *m_data;
>         struct rte_crypto_op *crypto_op;
>         struct rte_cryptodev *cdev;
>         struct otx2_cpt_qp *qp;
> @@ -37,7 +37,7 @@ otx2_ca_enq(uintptr_t tag_op, const struct rte_event *ev)
>                 qp_id = m_data->request_info.queue_pair_id;
>         } else if (crypto_op->sess_type == RTE_CRYPTO_OP_SESSIONLESS &&
>                    crypto_op->private_data_offset) {
> -               m_data = (union rte_event_crypto_metadata *)
> +               m_data = (struct rte_event_crypto_metadata *)
>                          ((uint8_t *)crypto_op +
>                           crypto_op->private_data_offset);
>                 cdev_id = m_data->request_info.cdev_id;
> diff --git a/lib/eventdev/rte_event_crypto_adapter.c b/lib/eventdev/rte_event_crypto_adapter.c
> index e1d38d383d..6977391ae9 100644
> --- a/lib/eventdev/rte_event_crypto_adapter.c
> +++ b/lib/eventdev/rte_event_crypto_adapter.c
> @@ -333,7 +333,7 @@ eca_enq_to_cryptodev(struct rte_event_crypto_adapter *adapter,
>                  struct rte_event *ev, unsigned int cnt)
>  {
>         struct rte_event_crypto_adapter_stats *stats = &adapter->crypto_stats;
> -       union rte_event_crypto_metadata *m_data = NULL;
> +       struct rte_event_crypto_metadata *m_data = NULL;
>         struct crypto_queue_pair_info *qp_info = NULL;
>         struct rte_crypto_op *crypto_op;
>         unsigned int i, n;
> @@ -371,7 +371,7 @@ eca_enq_to_cryptodev(struct rte_event_crypto_adapter *adapter,
>                         len++;
>                 } else if (crypto_op->sess_type == RTE_CRYPTO_OP_SESSIONLESS &&
>                                 crypto_op->private_data_offset) {
> -                       m_data = (union rte_event_crypto_metadata *)
> +                       m_data = (struct rte_event_crypto_metadata *)
>                                  ((uint8_t *)crypto_op +
>                                         crypto_op->private_data_offset);
>                         cdev_id = m_data->request_info.cdev_id;
> @@ -504,7 +504,7 @@ eca_ops_enqueue_burst(struct rte_event_crypto_adapter *adapter,
>                   struct rte_crypto_op **ops, uint16_t num)
>  {
>         struct rte_event_crypto_adapter_stats *stats = &adapter->crypto_stats;
> -       union rte_event_crypto_metadata *m_data = NULL;
> +       struct rte_event_crypto_metadata *m_data = NULL;
>         uint8_t event_dev_id = adapter->eventdev_id;
>         uint8_t event_port_id = adapter->event_port_id;
>         struct rte_event events[BATCH_SIZE];
> @@ -523,7 +523,7 @@ eca_ops_enqueue_burst(struct rte_event_crypto_adapter *adapter,
>                                         ops[i]->sym->session);
>                 } else if (ops[i]->sess_type == RTE_CRYPTO_OP_SESSIONLESS &&
>                                 ops[i]->private_data_offset) {
> -                       m_data = (union rte_event_crypto_metadata *)
> +                       m_data = (struct rte_event_crypto_metadata *)
>                                  ((uint8_t *)ops[i] +
>                                   ops[i]->private_data_offset);
>                 }
> diff --git a/lib/eventdev/rte_event_crypto_adapter.h b/lib/eventdev/rte_event_crypto_adapter.h
> index f8c6cca87c..3c24d9d9df 100644
> --- a/lib/eventdev/rte_event_crypto_adapter.h
> +++ b/lib/eventdev/rte_event_crypto_adapter.h
> @@ -200,11 +200,6 @@ enum rte_event_crypto_adapter_mode {
>   * provide event request information to the adapter.
>   */
>  struct rte_event_crypto_request {
> -       uint8_t resv[8];
> -       /**< Overlaps with first 8 bytes of struct rte_event
> -        * that encode the response event information. Application
> -        * is expected to fill in struct rte_event response_info.
> -        */
>         uint16_t cdev_id;
>         /**< cryptodev ID to be used */
>         uint16_t queue_pair_id;
> @@ -223,16 +218,16 @@ struct rte_event_crypto_request {
>   * operation. If the transfer is done by SW, event response information
>   * will be used by the adapter.
>   */
> -union rte_event_crypto_metadata {
> -       struct rte_event_crypto_request request_info;
> -       /**< Request information to be filled in by application
> -        * for RTE_EVENT_CRYPTO_ADAPTER_OP_FORWARD mode.
> -        */
> +struct rte_event_crypto_metadata {
>         struct rte_event response_info;
>         /**< Response information to be filled in by application
>          * for RTE_EVENT_CRYPTO_ADAPTER_OP_NEW and
>          * RTE_EVENT_CRYPTO_ADAPTER_OP_FORWARD mode.
>          */
> +       struct rte_event_crypto_request request_info;
> +       /**< Request information to be filled in by application
> +        * for RTE_EVENT_CRYPTO_ADAPTER_OP_FORWARD mode.
> +        */
>  };
>
>  /**
> --
> 2.25.1
>

Re: [dpdk-dev] [EXT] Re: [PATCH v3] eventdev: update crypto adapter metadata structures

[Akhil Goyal]

> On Tue, Aug 31, 2021 at 1:27 PM Shijith Thotton <sthotton@marvell.com>
> wrote:
> >
> > In crypto adapter metadata, reserved bytes in request info structure is
> > a space holder for response info. It enforces an order of operation if
> > the structures are updated using memcpy to avoid overwriting response
> > info. It is logical to move the reserved space out of request info. It
> > also solves the ordering issue mentioned before.
> >
> > This patch removes the reserve field from request info and makes event
> > crypto metadata type to structure from union to make space for response
> > info.
> >
> > App and drivers are updated as per metadata change.
> >
> > Signed-off-by: Shijith Thotton <sthotton@marvell.com>
> > Acked-by: Anoob Joseph <anoobj@marvell.com>
> 
> 
> @Gujjar, Abhinandan S  @Akhil Goyal
> 
> Could you review the crypto adapter-related change?
> 
Acked-by: Akhil Goyal <gakhil@marvell.com>

Re: [dpdk-dev] [PATCH v3] eventdev: update crypto adapter metadata structures

[Gujjar, Abhinandan S]

Hi Shijith,

> -----Original Message-----
> From: Shijith Thotton <sthotton@marvell.com>
> Sent: Tuesday, August 31, 2021 1:27 PM
> To: dev@dpdk.org
> Cc: Shijith Thotton <sthotton@marvell.com>; jerinj@marvell.com;
> anoobj@marvell.com; pbhagavatula@marvell.com; gakhil@marvell.com;
> Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>; Ray Kinsella
> <mdr@ashroe.eu>; Ankur Dwivedi <adwivedi@marvell.com>
> Subject: [PATCH v3] eventdev: update crypto adapter metadata structures
> 
> In crypto adapter metadata, reserved bytes in request info structure is a
> space holder for response info. It enforces an order of operation if the
> structures are updated using memcpy to avoid overwriting response info. It
> is logical to move the reserved space out of request info. It also solves the
> ordering issue mentioned before.
I would like to understand what kind of ordering issue you have faced with
the current approach. Could you please give an example/sequence and explain?

> 
> This patch removes the reserve field from request info and makes event
> crypto metadata type to structure from union to make space for response
> info.
> 
> App and drivers are updated as per metadata change.
> 
> Signed-off-by: Shijith Thotton <sthotton@marvell.com>
> Acked-by: Anoob Joseph <anoobj@marvell.com>
> ---
> v3:
> * Updated ABI section of release notes.
> 
> v2:
> * Updated deprecation notice.
> 
> v1:
> * Rebased.
> 
>  app/test/test_event_crypto_adapter.c              | 14 +++++++-------
>  doc/guides/rel_notes/deprecation.rst              |  6 ------
>  doc/guides/rel_notes/release_21_11.rst            |  2 ++
>  drivers/crypto/octeontx/otx_cryptodev_ops.c       |  8 ++++----
>  drivers/crypto/octeontx2/otx2_cryptodev_ops.c     |  4 ++--
>  .../event/octeontx2/otx2_evdev_crypto_adptr_tx.h  |  4 ++--
>  lib/eventdev/rte_event_crypto_adapter.c           |  8 ++++----
>  lib/eventdev/rte_event_crypto_adapter.h           | 15 +++++----------
>  8 files changed, 26 insertions(+), 35 deletions(-)
> 
> diff --git a/app/test/test_event_crypto_adapter.c
> b/app/test/test_event_crypto_adapter.c
> index 3ad20921e2..0d73694d3a 100644
> --- a/app/test/test_event_crypto_adapter.c
> +++ b/app/test/test_event_crypto_adapter.c
> @@ -168,7 +168,7 @@ test_op_forward_mode(uint8_t session_less)  {
>  	struct rte_crypto_sym_xform cipher_xform;
>  	struct rte_cryptodev_sym_session *sess;
> -	union rte_event_crypto_metadata m_data;
> +	struct rte_event_crypto_metadata m_data;
>  	struct rte_crypto_sym_op *sym_op;
>  	struct rte_crypto_op *op;
>  	struct rte_mbuf *m;
> @@ -368,7 +368,7 @@ test_op_new_mode(uint8_t session_less)  {
>  	struct rte_crypto_sym_xform cipher_xform;
>  	struct rte_cryptodev_sym_session *sess;
> -	union rte_event_crypto_metadata m_data;
> +	struct rte_event_crypto_metadata m_data;
>  	struct rte_crypto_sym_op *sym_op;
>  	struct rte_crypto_op *op;
>  	struct rte_mbuf *m;
> @@ -406,7 +406,7 @@ test_op_new_mode(uint8_t session_less)
>  		if (cap &
> RTE_EVENT_CRYPTO_ADAPTER_CAP_SESSION_PRIVATE_DATA) {
>  			/* Fill in private user data information */
>  			rte_memcpy(&m_data.response_info,
> &response_info,
> -				   sizeof(m_data));
> +				   sizeof(response_info));
>  			rte_cryptodev_sym_session_set_user_data(sess,
>  						&m_data, sizeof(m_data));
>  		}
> @@ -426,7 +426,7 @@ test_op_new_mode(uint8_t session_less)
>  		op->private_data_offset = len;
>  		/* Fill in private data information */
>  		rte_memcpy(&m_data.response_info, &response_info,
> -			   sizeof(m_data));
> +			   sizeof(response_info));
>  		rte_memcpy((uint8_t *)op + len, &m_data, sizeof(m_data));
>  	}
> 
> @@ -519,7 +519,7 @@ configure_cryptodev(void)
>  			DEFAULT_NUM_XFORMS *
>  			sizeof(struct rte_crypto_sym_xform) +
>  			MAXIMUM_IV_LENGTH +
> -			sizeof(union rte_event_crypto_metadata),
> +			sizeof(struct rte_event_crypto_metadata),
>  			rte_socket_id());
>  	if (params.op_mpool == NULL) {
>  		RTE_LOG(ERR, USER1, "Can't create CRYPTO_OP_POOL\n");
> @@ -549,12 +549,12 @@ configure_cryptodev(void)
>  	 * to include the session headers & private data
>  	 */
>  	session_size =
> rte_cryptodev_sym_get_private_session_size(TEST_CDEV_ID);
> -	session_size += sizeof(union rte_event_crypto_metadata);
> +	session_size += sizeof(struct rte_event_crypto_metadata);
> 
>  	params.session_mpool = rte_cryptodev_sym_session_pool_create(
>  			"CRYPTO_ADAPTER_SESSION_MP",
>  			MAX_NB_SESSIONS, 0, 0,
> -			sizeof(union rte_event_crypto_metadata),
> +			sizeof(struct rte_event_crypto_metadata),
>  			SOCKET_ID_ANY);
>  	TEST_ASSERT_NOT_NULL(params.session_mpool,
>  			"session mempool allocation failed\n"); diff --git
> a/doc/guides/rel_notes/deprecation.rst
> b/doc/guides/rel_notes/deprecation.rst
> index 76a4abfd6b..58ee95c020 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -266,12 +266,6 @@ Deprecation Notices
>    values to the function ``rte_event_eth_rx_adapter_queue_add`` using
>    the structure ``rte_event_eth_rx_adapter_queue_add``.
> 
> -* eventdev: Reserved bytes of ``rte_event_crypto_request`` is a space
> holder
> -  for ``response_info``. Both should be decoupled for better clarity.
> -  New space for ``response_info`` can be made by changing
> -  ``rte_event_crypto_metadata`` type to structure from union.
> -  This change is targeted for DPDK 21.11.
> -
>  * metrics: The function ``rte_metrics_init`` will have a non-void return
>    in order to notify errors instead of calling ``rte_exit``.
> 
> diff --git a/doc/guides/rel_notes/release_21_11.rst
> b/doc/guides/rel_notes/release_21_11.rst
> index d707a554ef..ab76d5dd55 100644
> --- a/doc/guides/rel_notes/release_21_11.rst
> +++ b/doc/guides/rel_notes/release_21_11.rst
> @@ -100,6 +100,8 @@ ABI Changes
>     Also, make sure to start the actual text at the margin.
>     =======================================================
> 
> +* eventdev: Modified type of ``union rte_event_crypto_metadata`` to
> +struct and
> +  removed reserved bytes from ``struct rte_event_crypto_request``.
> 
>  Known Issues
>  ------------
> diff --git a/drivers/crypto/octeontx/otx_cryptodev_ops.c
> b/drivers/crypto/octeontx/otx_cryptodev_ops.c
> index eac6796cfb..c51be63146 100644
> --- a/drivers/crypto/octeontx/otx_cryptodev_ops.c
> +++ b/drivers/crypto/octeontx/otx_cryptodev_ops.c
> @@ -710,17 +710,17 @@ submit_request_to_sso(struct ssows *ws,
> uintptr_t req,
>  	ssovf_store_pair(add_work, req, ws->grps[rsp_info->queue_id]);  }
> 
> -static inline union rte_event_crypto_metadata *
> +static inline struct rte_event_crypto_metadata *
>  get_event_crypto_mdata(struct rte_crypto_op *op)  {
> -	union rte_event_crypto_metadata *ec_mdata;
> +	struct rte_event_crypto_metadata *ec_mdata;
> 
>  	if (op->sess_type == RTE_CRYPTO_OP_WITH_SESSION)
>  		ec_mdata = rte_cryptodev_sym_session_get_user_data(
>  							   op->sym->session);
>  	else if (op->sess_type == RTE_CRYPTO_OP_SESSIONLESS &&
>  		 op->private_data_offset)
> -		ec_mdata = (union rte_event_crypto_metadata *)
> +		ec_mdata = (struct rte_event_crypto_metadata *)
>  			((uint8_t *)op + op->private_data_offset);
>  	else
>  		return NULL;
> @@ -731,7 +731,7 @@ get_event_crypto_mdata(struct rte_crypto_op *op)
> uint16_t __rte_hot  otx_crypto_adapter_enqueue(void *port, struct
> rte_crypto_op *op)  {
> -	union rte_event_crypto_metadata *ec_mdata;
> +	struct rte_event_crypto_metadata *ec_mdata;
>  	struct cpt_instance *instance;
>  	struct cpt_request_info *req;
>  	struct rte_event *rsp_info;
> diff --git a/drivers/crypto/octeontx2/otx2_cryptodev_ops.c
> b/drivers/crypto/octeontx2/otx2_cryptodev_ops.c
> index 42100154cd..952d1352f4 100644
> --- a/drivers/crypto/octeontx2/otx2_cryptodev_ops.c
> +++ b/drivers/crypto/octeontx2/otx2_cryptodev_ops.c
> @@ -453,7 +453,7 @@ otx2_ca_enqueue_req(const struct otx2_cpt_qp *qp,
>  		    struct rte_crypto_op *op,
>  		    uint64_t cpt_inst_w7)
>  {
> -	union rte_event_crypto_metadata *m_data;
> +	struct rte_event_crypto_metadata *m_data;
>  	union cpt_inst_s inst;
>  	uint64_t lmt_status;
> 
> @@ -468,7 +468,7 @@ otx2_ca_enqueue_req(const struct otx2_cpt_qp *qp,
>  		}
>  	} else if (op->sess_type == RTE_CRYPTO_OP_SESSIONLESS &&
>  		   op->private_data_offset) {
> -		m_data = (union rte_event_crypto_metadata *)
> +		m_data = (struct rte_event_crypto_metadata *)
>  			 ((uint8_t *)op +
>  			  op->private_data_offset);
>  	} else {
> diff --git a/drivers/event/octeontx2/otx2_evdev_crypto_adptr_tx.h
> b/drivers/event/octeontx2/otx2_evdev_crypto_adptr_tx.h
> index ecf7eb9f56..458e8306d7 100644
> --- a/drivers/event/octeontx2/otx2_evdev_crypto_adptr_tx.h
> +++ b/drivers/event/octeontx2/otx2_evdev_crypto_adptr_tx.h
> @@ -16,7 +16,7 @@
>  static inline uint16_t
>  otx2_ca_enq(uintptr_t tag_op, const struct rte_event *ev)  {
> -	union rte_event_crypto_metadata *m_data;
> +	struct rte_event_crypto_metadata *m_data;
>  	struct rte_crypto_op *crypto_op;
>  	struct rte_cryptodev *cdev;
>  	struct otx2_cpt_qp *qp;
> @@ -37,7 +37,7 @@ otx2_ca_enq(uintptr_t tag_op, const struct rte_event
> *ev)
>  		qp_id = m_data->request_info.queue_pair_id;
>  	} else if (crypto_op->sess_type == RTE_CRYPTO_OP_SESSIONLESS
> &&
>  		   crypto_op->private_data_offset) {
> -		m_data = (union rte_event_crypto_metadata *)
> +		m_data = (struct rte_event_crypto_metadata *)
>  			 ((uint8_t *)crypto_op +
>  			  crypto_op->private_data_offset);
>  		cdev_id = m_data->request_info.cdev_id; diff --git
> a/lib/eventdev/rte_event_crypto_adapter.c
> b/lib/eventdev/rte_event_crypto_adapter.c
> index e1d38d383d..6977391ae9 100644
> --- a/lib/eventdev/rte_event_crypto_adapter.c
> +++ b/lib/eventdev/rte_event_crypto_adapter.c
> @@ -333,7 +333,7 @@ eca_enq_to_cryptodev(struct
> rte_event_crypto_adapter *adapter,
>  		 struct rte_event *ev, unsigned int cnt)  {
>  	struct rte_event_crypto_adapter_stats *stats = &adapter-
> >crypto_stats;
> -	union rte_event_crypto_metadata *m_data = NULL;
> +	struct rte_event_crypto_metadata *m_data = NULL;
>  	struct crypto_queue_pair_info *qp_info = NULL;
>  	struct rte_crypto_op *crypto_op;
>  	unsigned int i, n;
> @@ -371,7 +371,7 @@ eca_enq_to_cryptodev(struct
> rte_event_crypto_adapter *adapter,
>  			len++;
>  		} else if (crypto_op->sess_type ==
> RTE_CRYPTO_OP_SESSIONLESS &&
>  				crypto_op->private_data_offset) {
> -			m_data = (union rte_event_crypto_metadata *)
> +			m_data = (struct rte_event_crypto_metadata *)
>  				 ((uint8_t *)crypto_op +
>  					crypto_op->private_data_offset);
>  			cdev_id = m_data->request_info.cdev_id; @@ -
> 504,7 +504,7 @@ eca_ops_enqueue_burst(struct rte_event_crypto_adapter
> *adapter,
>  		  struct rte_crypto_op **ops, uint16_t num)  {
>  	struct rte_event_crypto_adapter_stats *stats = &adapter-
> >crypto_stats;
> -	union rte_event_crypto_metadata *m_data = NULL;
> +	struct rte_event_crypto_metadata *m_data = NULL;
>  	uint8_t event_dev_id = adapter->eventdev_id;
>  	uint8_t event_port_id = adapter->event_port_id;
>  	struct rte_event events[BATCH_SIZE];
> @@ -523,7 +523,7 @@ eca_ops_enqueue_burst(struct
> rte_event_crypto_adapter *adapter,
>  					ops[i]->sym->session);
>  		} else if (ops[i]->sess_type ==
> RTE_CRYPTO_OP_SESSIONLESS &&
>  				ops[i]->private_data_offset) {
> -			m_data = (union rte_event_crypto_metadata *)
> +			m_data = (struct rte_event_crypto_metadata *)
>  				 ((uint8_t *)ops[i] +
>  				  ops[i]->private_data_offset);
>  		}
> diff --git a/lib/eventdev/rte_event_crypto_adapter.h
> b/lib/eventdev/rte_event_crypto_adapter.h
> index f8c6cca87c..3c24d9d9df 100644
> --- a/lib/eventdev/rte_event_crypto_adapter.h
> +++ b/lib/eventdev/rte_event_crypto_adapter.h
> @@ -200,11 +200,6 @@ enum rte_event_crypto_adapter_mode {
>   * provide event request information to the adapter.
>   */
>  struct rte_event_crypto_request {
> -	uint8_t resv[8];
> -	/**< Overlaps with first 8 bytes of struct rte_event
> -	 * that encode the response event information. Application
> -	 * is expected to fill in struct rte_event response_info.
> -	 */
>  	uint16_t cdev_id;
>  	/**< cryptodev ID to be used */
>  	uint16_t queue_pair_id;
> @@ -223,16 +218,16 @@ struct rte_event_crypto_request {
>   * operation. If the transfer is done by SW, event response information
>   * will be used by the adapter.
>   */
> -union rte_event_crypto_metadata {
> -	struct rte_event_crypto_request request_info;
> -	/**< Request information to be filled in by application
> -	 * for RTE_EVENT_CRYPTO_ADAPTER_OP_FORWARD mode.
> -	 */
> +struct rte_event_crypto_metadata {
>  	struct rte_event response_info;
>  	/**< Response information to be filled in by application
>  	 * for RTE_EVENT_CRYPTO_ADAPTER_OP_NEW and
>  	 * RTE_EVENT_CRYPTO_ADAPTER_OP_FORWARD mode.
>  	 */
> +	struct rte_event_crypto_request request_info;
> +	/**< Request information to be filled in by application
> +	 * for RTE_EVENT_CRYPTO_ADAPTER_OP_FORWARD mode.
> +	 */
>  };
> 
>  /**
> --
> 2.25.1

Re: [dpdk-dev] [PATCH v3] eventdev: update crypto adapter metadata structures

[Shijith Thotton]

Hi Abhinandan,

>> In crypto adapter metadata, reserved bytes in request info structure is a
>> space holder for response info. It enforces an order of operation if the
>> structures are updated using memcpy to avoid overwriting response info. It
>> is logical to move the reserved space out of request info. It also solves the
>> ordering issue mentioned before.
>I would like to understand what kind of ordering issue you have faced with
>the current approach. Could you please give an example/sequence and explain?
>

I have seen this issue with crypto adapter autotest (#n215).

Example:
rte_memcpy(&m_data.response_info, &response_info, sizeof(response_info));
rte_memcpy(&m_data.request_info, &request_info, sizeof(request_info));

Here response info is getting overwritten by request info.
Above lines can reordered to fix the issue, but can be ignored with this patch.

>>
>> This patch removes the reserve field from request info and makes event
>> crypto metadata type to structure from union to make space for response
>> info.
>>
>> App and drivers are updated as per metadata change.
>>
>> Signed-off-by: Shijith Thotton <sthotton@marvell.com>
>> Acked-by: Anoob Joseph <anoobj@marvell.com>
>> ---
>> v3:
>> * Updated ABI section of release notes.
>>
>> v2:
>> * Updated deprecation notice.
>>
>> v1:
>> * Rebased.
>>
>>  app/test/test_event_crypto_adapter.c              | 14 +++++++-------
>>  doc/guides/rel_notes/deprecation.rst              |  6 ------
>>  doc/guides/rel_notes/release_21_11.rst            |  2 ++
>>  drivers/crypto/octeontx/otx_cryptodev_ops.c       |  8 ++++----
>>  drivers/crypto/octeontx2/otx2_cryptodev_ops.c     |  4 ++--
>>  .../event/octeontx2/otx2_evdev_crypto_adptr_tx.h  |  4 ++--
>>  lib/eventdev/rte_event_crypto_adapter.c           |  8 ++++----
>>  lib/eventdev/rte_event_crypto_adapter.h           | 15 +++++----------
>>  8 files changed, 26 insertions(+), 35 deletions(-)
>>
>> diff --git a/app/test/test_event_crypto_adapter.c
>> b/app/test/test_event_crypto_adapter.c
>> index 3ad20921e2..0d73694d3a 100644
>> --- a/app/test/test_event_crypto_adapter.c
>> +++ b/app/test/test_event_crypto_adapter.c
>> @@ -168,7 +168,7 @@ test_op_forward_mode(uint8_t session_less)  {
>>  	struct rte_crypto_sym_xform cipher_xform;
>>  	struct rte_cryptodev_sym_session *sess;
>> -	union rte_event_crypto_metadata m_data;
>> +	struct rte_event_crypto_metadata m_data;
>>  	struct rte_crypto_sym_op *sym_op;
>>  	struct rte_crypto_op *op;
>>  	struct rte_mbuf *m;
>> @@ -368,7 +368,7 @@ test_op_new_mode(uint8_t session_less)  {
>>  	struct rte_crypto_sym_xform cipher_xform;
>>  	struct rte_cryptodev_sym_session *sess;
>> -	union rte_event_crypto_metadata m_data;
>> +	struct rte_event_crypto_metadata m_data;
>>  	struct rte_crypto_sym_op *sym_op;
>>  	struct rte_crypto_op *op;
>>  	struct rte_mbuf *m;
>> @@ -406,7 +406,7 @@ test_op_new_mode(uint8_t session_less)
>>  		if (cap &
>> RTE_EVENT_CRYPTO_ADAPTER_CAP_SESSION_PRIVATE_DATA) {
>>  			/* Fill in private user data information */
>>  			rte_memcpy(&m_data.response_info,
>> &response_info,
>> -				   sizeof(m_data));
>> +				   sizeof(response_info));
>>  			rte_cryptodev_sym_session_set_user_data(sess,
>>  						&m_data, sizeof(m_data));
>>  		}
>> @@ -426,7 +426,7 @@ test_op_new_mode(uint8_t session_less)
>>  		op->private_data_offset = len;
>>  		/* Fill in private data information */
>>  		rte_memcpy(&m_data.response_info, &response_info,
>> -			   sizeof(m_data));
>> +			   sizeof(response_info));
>>  		rte_memcpy((uint8_t *)op + len, &m_data, sizeof(m_data));
>>  	}
>>
>> @@ -519,7 +519,7 @@ configure_cryptodev(void)
>>  			DEFAULT_NUM_XFORMS *
>>  			sizeof(struct rte_crypto_sym_xform) +
>>  			MAXIMUM_IV_LENGTH +
>> -			sizeof(union rte_event_crypto_metadata),
>> +			sizeof(struct rte_event_crypto_metadata),
>>  			rte_socket_id());
>>  	if (params.op_mpool == NULL) {
>>  		RTE_LOG(ERR, USER1, "Can't create CRYPTO_OP_POOL\n");
>> @@ -549,12 +549,12 @@ configure_cryptodev(void)
>>  	 * to include the session headers & private data
>>  	 */
>>  	session_size =
>> rte_cryptodev_sym_get_private_session_size(TEST_CDEV_ID);
>> -	session_size += sizeof(union rte_event_crypto_metadata);
>> +	session_size += sizeof(struct rte_event_crypto_metadata);
>>
>>  	params.session_mpool = rte_cryptodev_sym_session_pool_create(
>>  			"CRYPTO_ADAPTER_SESSION_MP",
>>  			MAX_NB_SESSIONS, 0, 0,
>> -			sizeof(union rte_event_crypto_metadata),
>> +			sizeof(struct rte_event_crypto_metadata),
>>  			SOCKET_ID_ANY);
>>  	TEST_ASSERT_NOT_NULL(params.session_mpool,
>>  			"session mempool allocation failed\n"); diff --git
>> a/doc/guides/rel_notes/deprecation.rst
>> b/doc/guides/rel_notes/deprecation.rst
>> index 76a4abfd6b..58ee95c020 100644
>> --- a/doc/guides/rel_notes/deprecation.rst
>> +++ b/doc/guides/rel_notes/deprecation.rst
>> @@ -266,12 +266,6 @@ Deprecation Notices
>>    values to the function ``rte_event_eth_rx_adapter_queue_add`` using
>>    the structure ``rte_event_eth_rx_adapter_queue_add``.
>>
>> -* eventdev: Reserved bytes of ``rte_event_crypto_request`` is a space
>> holder
>> -  for ``response_info``. Both should be decoupled for better clarity.
>> -  New space for ``response_info`` can be made by changing
>> -  ``rte_event_crypto_metadata`` type to structure from union.
>> -  This change is targeted for DPDK 21.11.
>> -
>>  * metrics: The function ``rte_metrics_init`` will have a non-void return
>>    in order to notify errors instead of calling ``rte_exit``.
>>
>> diff --git a/doc/guides/rel_notes/release_21_11.rst
>> b/doc/guides/rel_notes/release_21_11.rst
>> index d707a554ef..ab76d5dd55 100644
>> --- a/doc/guides/rel_notes/release_21_11.rst
>> +++ b/doc/guides/rel_notes/release_21_11.rst
>> @@ -100,6 +100,8 @@ ABI Changes
>>     Also, make sure to start the actual text at the margin.
>>     =======================================================
>>
>> +* eventdev: Modified type of ``union rte_event_crypto_metadata`` to
>> +struct and
>> +  removed reserved bytes from ``struct rte_event_crypto_request``.
>>
>>  Known Issues
>>  ------------
>> diff --git a/drivers/crypto/octeontx/otx_cryptodev_ops.c
>> b/drivers/crypto/octeontx/otx_cryptodev_ops.c
>> index eac6796cfb..c51be63146 100644
>> --- a/drivers/crypto/octeontx/otx_cryptodev_ops.c
>> +++ b/drivers/crypto/octeontx/otx_cryptodev_ops.c
>> @@ -710,17 +710,17 @@ submit_request_to_sso(struct ssows *ws,
>> uintptr_t req,
>>  	ssovf_store_pair(add_work, req, ws->grps[rsp_info->queue_id]);  }
>>
>> -static inline union rte_event_crypto_metadata *
>> +static inline struct rte_event_crypto_metadata *
>>  get_event_crypto_mdata(struct rte_crypto_op *op)  {
>> -	union rte_event_crypto_metadata *ec_mdata;
>> +	struct rte_event_crypto_metadata *ec_mdata;
>>
>>  	if (op->sess_type == RTE_CRYPTO_OP_WITH_SESSION)
>>  		ec_mdata = rte_cryptodev_sym_session_get_user_data(
>>  							   op->sym->session);
>>  	else if (op->sess_type == RTE_CRYPTO_OP_SESSIONLESS &&
>>  		 op->private_data_offset)
>> -		ec_mdata = (union rte_event_crypto_metadata *)
>> +		ec_mdata = (struct rte_event_crypto_metadata *)
>>  			((uint8_t *)op + op->private_data_offset);
>>  	else
>>  		return NULL;
>> @@ -731,7 +731,7 @@ get_event_crypto_mdata(struct rte_crypto_op *op)
>> uint16_t __rte_hot  otx_crypto_adapter_enqueue(void *port, struct
>> rte_crypto_op *op)  {
>> -	union rte_event_crypto_metadata *ec_mdata;
>> +	struct rte_event_crypto_metadata *ec_mdata;
>>  	struct cpt_instance *instance;
>>  	struct cpt_request_info *req;
>>  	struct rte_event *rsp_info;
>> diff --git a/drivers/crypto/octeontx2/otx2_cryptodev_ops.c
>> b/drivers/crypto/octeontx2/otx2_cryptodev_ops.c
>> index 42100154cd..952d1352f4 100644
>> --- a/drivers/crypto/octeontx2/otx2_cryptodev_ops.c
>> +++ b/drivers/crypto/octeontx2/otx2_cryptodev_ops.c
>> @@ -453,7 +453,7 @@ otx2_ca_enqueue_req(const struct otx2_cpt_qp *qp,
>>  		    struct rte_crypto_op *op,
>>  		    uint64_t cpt_inst_w7)
>>  {
>> -	union rte_event_crypto_metadata *m_data;
>> +	struct rte_event_crypto_metadata *m_data;
>>  	union cpt_inst_s inst;
>>  	uint64_t lmt_status;
>>
>> @@ -468,7 +468,7 @@ otx2_ca_enqueue_req(const struct otx2_cpt_qp *qp,
>>  		}
>>  	} else if (op->sess_type == RTE_CRYPTO_OP_SESSIONLESS &&
>>  		   op->private_data_offset) {
>> -		m_data = (union rte_event_crypto_metadata *)
>> +		m_data = (struct rte_event_crypto_metadata *)
>>  			 ((uint8_t *)op +
>>  			  op->private_data_offset);
>>  	} else {
>> diff --git a/drivers/event/octeontx2/otx2_evdev_crypto_adptr_tx.h
>> b/drivers/event/octeontx2/otx2_evdev_crypto_adptr_tx.h
>> index ecf7eb9f56..458e8306d7 100644
>> --- a/drivers/event/octeontx2/otx2_evdev_crypto_adptr_tx.h
>> +++ b/drivers/event/octeontx2/otx2_evdev_crypto_adptr_tx.h
>> @@ -16,7 +16,7 @@
>>  static inline uint16_t
>>  otx2_ca_enq(uintptr_t tag_op, const struct rte_event *ev)  {
>> -	union rte_event_crypto_metadata *m_data;
>> +	struct rte_event_crypto_metadata *m_data;
>>  	struct rte_crypto_op *crypto_op;
>>  	struct rte_cryptodev *cdev;
>>  	struct otx2_cpt_qp *qp;
>> @@ -37,7 +37,7 @@ otx2_ca_enq(uintptr_t tag_op, const struct rte_event
>> *ev)
>>  		qp_id = m_data->request_info.queue_pair_id;
>>  	} else if (crypto_op->sess_type == RTE_CRYPTO_OP_SESSIONLESS
>> &&
>>  		   crypto_op->private_data_offset) {
>> -		m_data = (union rte_event_crypto_metadata *)
>> +		m_data = (struct rte_event_crypto_metadata *)
>>  			 ((uint8_t *)crypto_op +
>>  			  crypto_op->private_data_offset);
>>  		cdev_id = m_data->request_info.cdev_id; diff --git
>> a/lib/eventdev/rte_event_crypto_adapter.c
>> b/lib/eventdev/rte_event_crypto_adapter.c
>> index e1d38d383d..6977391ae9 100644
>> --- a/lib/eventdev/rte_event_crypto_adapter.c
>> +++ b/lib/eventdev/rte_event_crypto_adapter.c
>> @@ -333,7 +333,7 @@ eca_enq_to_cryptodev(struct
>> rte_event_crypto_adapter *adapter,
>>  		 struct rte_event *ev, unsigned int cnt)  {
>>  	struct rte_event_crypto_adapter_stats *stats = &adapter-
>> >crypto_stats;
>> -	union rte_event_crypto_metadata *m_data = NULL;
>> +	struct rte_event_crypto_metadata *m_data = NULL;
>>  	struct crypto_queue_pair_info *qp_info = NULL;
>>  	struct rte_crypto_op *crypto_op;
>>  	unsigned int i, n;
>> @@ -371,7 +371,7 @@ eca_enq_to_cryptodev(struct
>> rte_event_crypto_adapter *adapter,
>>  			len++;
>>  		} else if (crypto_op->sess_type ==
>> RTE_CRYPTO_OP_SESSIONLESS &&
>>  				crypto_op->private_data_offset) {
>> -			m_data = (union rte_event_crypto_metadata *)
>> +			m_data = (struct rte_event_crypto_metadata *)
>>  				 ((uint8_t *)crypto_op +
>>  					crypto_op->private_data_offset);
>>  			cdev_id = m_data->request_info.cdev_id; @@ -
>> 504,7 +504,7 @@ eca_ops_enqueue_burst(struct rte_event_crypto_adapter
>> *adapter,
>>  		  struct rte_crypto_op **ops, uint16_t num)  {
>>  	struct rte_event_crypto_adapter_stats *stats = &adapter-
>> >crypto_stats;
>> -	union rte_event_crypto_metadata *m_data = NULL;
>> +	struct rte_event_crypto_metadata *m_data = NULL;
>>  	uint8_t event_dev_id = adapter->eventdev_id;
>>  	uint8_t event_port_id = adapter->event_port_id;
>>  	struct rte_event events[BATCH_SIZE];
>> @@ -523,7 +523,7 @@ eca_ops_enqueue_burst(struct
>> rte_event_crypto_adapter *adapter,
>>  					ops[i]->sym->session);
>>  		} else if (ops[i]->sess_type ==
>> RTE_CRYPTO_OP_SESSIONLESS &&
>>  				ops[i]->private_data_offset) {
>> -			m_data = (union rte_event_crypto_metadata *)
>> +			m_data = (struct rte_event_crypto_metadata *)
>>  				 ((uint8_t *)ops[i] +
>>  				  ops[i]->private_data_offset);
>>  		}
>> diff --git a/lib/eventdev/rte_event_crypto_adapter.h
>> b/lib/eventdev/rte_event_crypto_adapter.h
>> index f8c6cca87c..3c24d9d9df 100644
>> --- a/lib/eventdev/rte_event_crypto_adapter.h
>> +++ b/lib/eventdev/rte_event_crypto_adapter.h
>> @@ -200,11 +200,6 @@ enum rte_event_crypto_adapter_mode {
>>   * provide event request information to the adapter.
>>   */
>>  struct rte_event_crypto_request {
>> -	uint8_t resv[8];
>> -	/**< Overlaps with first 8 bytes of struct rte_event
>> -	 * that encode the response event information. Application
>> -	 * is expected to fill in struct rte_event response_info.
>> -	 */
>>  	uint16_t cdev_id;
>>  	/**< cryptodev ID to be used */
>>  	uint16_t queue_pair_id;
>> @@ -223,16 +218,16 @@ struct rte_event_crypto_request {
>>   * operation. If the transfer is done by SW, event response information
>>   * will be used by the adapter.
>>   */
>> -union rte_event_crypto_metadata {
>> -	struct rte_event_crypto_request request_info;
>> -	/**< Request information to be filled in by application
>> -	 * for RTE_EVENT_CRYPTO_ADAPTER_OP_FORWARD mode.
>> -	 */
>> +struct rte_event_crypto_metadata {
>>  	struct rte_event response_info;
>>  	/**< Response information to be filled in by application
>>  	 * for RTE_EVENT_CRYPTO_ADAPTER_OP_NEW and
>>  	 * RTE_EVENT_CRYPTO_ADAPTER_OP_FORWARD mode.
>>  	 */
>> +	struct rte_event_crypto_request request_info;
>> +	/**< Request information to be filled in by application
>> +	 * for RTE_EVENT_CRYPTO_ADAPTER_OP_FORWARD mode.
>> +	 */
>>  };
>>
>>  /**
>> --
>> 2.25.1

Re: [dpdk-dev] [PATCH v3] eventdev: update crypto adapter metadata structures

[Gujjar, Abhinandan S]

Hi Shijith,

> -----Original Message-----
> From: Shijith Thotton <sthotton@marvell.com>
> Sent: Tuesday, September 7, 2021 4:07 PM
> To: Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>; dev@dpdk.org
> Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Anoob Joseph
> <anoobj@marvell.com>; Pavan Nikhilesh Bhagavatula
> <pbhagavatula@marvell.com>; Akhil Goyal <gakhil@marvell.com>; Ray
> Kinsella <mdr@ashroe.eu>; Ankur Dwivedi <adwivedi@marvell.com>
> Subject: RE: [PATCH v3] eventdev: update crypto adapter metadata
> structures
> 
> Hi Abhinandan,
> 
> >> In crypto adapter metadata, reserved bytes in request info structure
> >> is a space holder for response info. It enforces an order of
> >> operation if the structures are updated using memcpy to avoid
> >> overwriting response info. It is logical to move the reserved space
> >> out of request info. It also solves the ordering issue mentioned before.
> >I would like to understand what kind of ordering issue you have faced
> >with the current approach. Could you please give an example/sequence
> and explain?
> >
> 
> I have seen this issue with crypto adapter autotest (#n215).
> 
> Example:
> rte_memcpy(&m_data.response_info, &response_info,
> sizeof(response_info)); rte_memcpy(&m_data.request_info,
> &request_info, sizeof(request_info));
> 
> Here response info is getting overwritten by request info.
> Above lines can reordered to fix the issue, but can be ignored with this patch.
There is a reason for designing the metadata in this way.
Right now, sizeof (union rte_event_crypto_metadata) is 16 bytes.
So, the session based case needs just 16 bytes to store the data.
Whereas, for sessionless case each crypto_ops requires another 16 bytes.

By changing the struct in the following way you are doubling the memory requirement.
With the changes, for sessionless case, each crypto op requires 32 bytes of space instead of 16 bytes and the mempool will be bigger.
This will have the perf impact too!

You can just copy the individual members(cdev_id & queue_pair_id) after the response_info.
OR You have a better way?

> 
> >>
> >> This patch removes the reserve field from request info and makes
> >> event crypto metadata type to structure from union to make space for
> >> response info.
> >>
> >> App and drivers are updated as per metadata change.
> >>
> >> Signed-off-by: Shijith Thotton <sthotton@marvell.com>
> >> Acked-by: Anoob Joseph <anoobj@marvell.com>
> >> ---
> >> v3:
> >> * Updated ABI section of release notes.
> >>
> >> v2:
> >> * Updated deprecation notice.
> >>
> >> v1:
> >> * Rebased.
> >>
> >>  app/test/test_event_crypto_adapter.c              | 14 +++++++-------
> >>  doc/guides/rel_notes/deprecation.rst              |  6 ------
> >>  doc/guides/rel_notes/release_21_11.rst            |  2 ++
> >>  drivers/crypto/octeontx/otx_cryptodev_ops.c       |  8 ++++----
> >>  drivers/crypto/octeontx2/otx2_cryptodev_ops.c     |  4 ++--
> >>  .../event/octeontx2/otx2_evdev_crypto_adptr_tx.h  |  4 ++--
> >>  lib/eventdev/rte_event_crypto_adapter.c           |  8 ++++----
> >>  lib/eventdev/rte_event_crypto_adapter.h           | 15 +++++----------
> >>  8 files changed, 26 insertions(+), 35 deletions(-)
> >>
> >> diff --git a/app/test/test_event_crypto_adapter.c
> >> b/app/test/test_event_crypto_adapter.c
> >> index 3ad20921e2..0d73694d3a 100644
> >> --- a/app/test/test_event_crypto_adapter.c
> >> +++ b/app/test/test_event_crypto_adapter.c
> >> @@ -168,7 +168,7 @@ test_op_forward_mode(uint8_t session_less)  {
> >>  	struct rte_crypto_sym_xform cipher_xform;
> >>  	struct rte_cryptodev_sym_session *sess;
> >> -	union rte_event_crypto_metadata m_data;
> >> +	struct rte_event_crypto_metadata m_data;
> >>  	struct rte_crypto_sym_op *sym_op;
> >>  	struct rte_crypto_op *op;
> >>  	struct rte_mbuf *m;
> >> @@ -368,7 +368,7 @@ test_op_new_mode(uint8_t session_less)  {
> >>  	struct rte_crypto_sym_xform cipher_xform;
> >>  	struct rte_cryptodev_sym_session *sess;
> >> -	union rte_event_crypto_metadata m_data;
> >> +	struct rte_event_crypto_metadata m_data;
> >>  	struct rte_crypto_sym_op *sym_op;
> >>  	struct rte_crypto_op *op;
> >>  	struct rte_mbuf *m;
> >> @@ -406,7 +406,7 @@ test_op_new_mode(uint8_t session_less)
> >>  		if (cap &
> >> RTE_EVENT_CRYPTO_ADAPTER_CAP_SESSION_PRIVATE_DATA) {
> >>  			/* Fill in private user data information */
> >>  			rte_memcpy(&m_data.response_info,
> &response_info,
> >> -				   sizeof(m_data));
> >> +				   sizeof(response_info));
> >>  			rte_cryptodev_sym_session_set_user_data(sess,
> >>  						&m_data, sizeof(m_data));
> >>  		}
> >> @@ -426,7 +426,7 @@ test_op_new_mode(uint8_t session_less)
> >>  		op->private_data_offset = len;
> >>  		/* Fill in private data information */
> >>  		rte_memcpy(&m_data.response_info, &response_info,
> >> -			   sizeof(m_data));
> >> +			   sizeof(response_info));
> >>  		rte_memcpy((uint8_t *)op + len, &m_data, sizeof(m_data));
> >>  	}
> >>
> >> @@ -519,7 +519,7 @@ configure_cryptodev(void)
> >>  			DEFAULT_NUM_XFORMS *
> >>  			sizeof(struct rte_crypto_sym_xform) +
> >>  			MAXIMUM_IV_LENGTH +
> >> -			sizeof(union rte_event_crypto_metadata),
> >> +			sizeof(struct rte_event_crypto_metadata),
> >>  			rte_socket_id());
> >>  	if (params.op_mpool == NULL) {
> >>  		RTE_LOG(ERR, USER1, "Can't create CRYPTO_OP_POOL\n");
> @@ -549,12
> >> +549,12 @@ configure_cryptodev(void)
> >>  	 * to include the session headers & private data
> >>  	 */
> >>  	session_size =
> >> rte_cryptodev_sym_get_private_session_size(TEST_CDEV_ID);
> >> -	session_size += sizeof(union rte_event_crypto_metadata);
> >> +	session_size += sizeof(struct rte_event_crypto_metadata);
> >>
> >>  	params.session_mpool = rte_cryptodev_sym_session_pool_create(
> >>  			"CRYPTO_ADAPTER_SESSION_MP",
> >>  			MAX_NB_SESSIONS, 0, 0,
> >> -			sizeof(union rte_event_crypto_metadata),
> >> +			sizeof(struct rte_event_crypto_metadata),
> >>  			SOCKET_ID_ANY);
> >>  	TEST_ASSERT_NOT_NULL(params.session_mpool,
> >>  			"session mempool allocation failed\n"); diff --git
> >> a/doc/guides/rel_notes/deprecation.rst
> >> b/doc/guides/rel_notes/deprecation.rst
> >> index 76a4abfd6b..58ee95c020 100644
> >> --- a/doc/guides/rel_notes/deprecation.rst
> >> +++ b/doc/guides/rel_notes/deprecation.rst
> >> @@ -266,12 +266,6 @@ Deprecation Notices
> >>    values to the function ``rte_event_eth_rx_adapter_queue_add`` using
> >>    the structure ``rte_event_eth_rx_adapter_queue_add``.
> >>
> >> -* eventdev: Reserved bytes of ``rte_event_crypto_request`` is a
> >> space holder
> >> -  for ``response_info``. Both should be decoupled for better clarity.
> >> -  New space for ``response_info`` can be made by changing
> >> -  ``rte_event_crypto_metadata`` type to structure from union.
> >> -  This change is targeted for DPDK 21.11.
> >> -
> >>  * metrics: The function ``rte_metrics_init`` will have a non-void return
> >>    in order to notify errors instead of calling ``rte_exit``.
> >>
> >> diff --git a/doc/guides/rel_notes/release_21_11.rst
> >> b/doc/guides/rel_notes/release_21_11.rst
> >> index d707a554ef..ab76d5dd55 100644
> >> --- a/doc/guides/rel_notes/release_21_11.rst
> >> +++ b/doc/guides/rel_notes/release_21_11.rst
> >> @@ -100,6 +100,8 @@ ABI Changes
> >>     Also, make sure to start the actual text at the margin.
> >>
> =======================================================
> >>
> >> +* eventdev: Modified type of ``union rte_event_crypto_metadata`` to
> >> +struct and
> >> +  removed reserved bytes from ``struct rte_event_crypto_request``.
> >>
> >>  Known Issues
> >>  ------------
> >> diff --git a/drivers/crypto/octeontx/otx_cryptodev_ops.c
> >> b/drivers/crypto/octeontx/otx_cryptodev_ops.c
> >> index eac6796cfb..c51be63146 100644
> >> --- a/drivers/crypto/octeontx/otx_cryptodev_ops.c
> >> +++ b/drivers/crypto/octeontx/otx_cryptodev_ops.c
> >> @@ -710,17 +710,17 @@ submit_request_to_sso(struct ssows *ws,
> >> uintptr_t req,
> >>  	ssovf_store_pair(add_work, req, ws->grps[rsp_info->queue_id]);  }
> >>
> >> -static inline union rte_event_crypto_metadata *
> >> +static inline struct rte_event_crypto_metadata *
> >>  get_event_crypto_mdata(struct rte_crypto_op *op)  {
> >> -	union rte_event_crypto_metadata *ec_mdata;
> >> +	struct rte_event_crypto_metadata *ec_mdata;
> >>
> >>  	if (op->sess_type == RTE_CRYPTO_OP_WITH_SESSION)
> >>  		ec_mdata = rte_cryptodev_sym_session_get_user_data(
> >>  							   op->sym->session);
> >>  	else if (op->sess_type == RTE_CRYPTO_OP_SESSIONLESS &&
> >>  		 op->private_data_offset)
> >> -		ec_mdata = (union rte_event_crypto_metadata *)
> >> +		ec_mdata = (struct rte_event_crypto_metadata *)
> >>  			((uint8_t *)op + op->private_data_offset);
> >>  	else
> >>  		return NULL;
> >> @@ -731,7 +731,7 @@ get_event_crypto_mdata(struct rte_crypto_op
> *op)
> >> uint16_t __rte_hot  otx_crypto_adapter_enqueue(void *port, struct
> >> rte_crypto_op *op)  {
> >> -	union rte_event_crypto_metadata *ec_mdata;
> >> +	struct rte_event_crypto_metadata *ec_mdata;
> >>  	struct cpt_instance *instance;
> >>  	struct cpt_request_info *req;
> >>  	struct rte_event *rsp_info;
> >> diff --git a/drivers/crypto/octeontx2/otx2_cryptodev_ops.c
> >> b/drivers/crypto/octeontx2/otx2_cryptodev_ops.c
> >> index 42100154cd..952d1352f4 100644
> >> --- a/drivers/crypto/octeontx2/otx2_cryptodev_ops.c
> >> +++ b/drivers/crypto/octeontx2/otx2_cryptodev_ops.c
> >> @@ -453,7 +453,7 @@ otx2_ca_enqueue_req(const struct otx2_cpt_qp
> *qp,
> >>  		    struct rte_crypto_op *op,
> >>  		    uint64_t cpt_inst_w7)
> >>  {
> >> -	union rte_event_crypto_metadata *m_data;
> >> +	struct rte_event_crypto_metadata *m_data;
> >>  	union cpt_inst_s inst;
> >>  	uint64_t lmt_status;
> >>
> >> @@ -468,7 +468,7 @@ otx2_ca_enqueue_req(const struct otx2_cpt_qp
> *qp,
> >>  		}
> >>  	} else if (op->sess_type == RTE_CRYPTO_OP_SESSIONLESS &&
> >>  		   op->private_data_offset) {
> >> -		m_data = (union rte_event_crypto_metadata *)
> >> +		m_data = (struct rte_event_crypto_metadata *)
> >>  			 ((uint8_t *)op +
> >>  			  op->private_data_offset);
> >>  	} else {
> >> diff --git a/drivers/event/octeontx2/otx2_evdev_crypto_adptr_tx.h
> >> b/drivers/event/octeontx2/otx2_evdev_crypto_adptr_tx.h
> >> index ecf7eb9f56..458e8306d7 100644
> >> --- a/drivers/event/octeontx2/otx2_evdev_crypto_adptr_tx.h
> >> +++ b/drivers/event/octeontx2/otx2_evdev_crypto_adptr_tx.h
> >> @@ -16,7 +16,7 @@
> >>  static inline uint16_t
> >>  otx2_ca_enq(uintptr_t tag_op, const struct rte_event *ev)  {
> >> -	union rte_event_crypto_metadata *m_data;
> >> +	struct rte_event_crypto_metadata *m_data;
> >>  	struct rte_crypto_op *crypto_op;
> >>  	struct rte_cryptodev *cdev;
> >>  	struct otx2_cpt_qp *qp;
> >> @@ -37,7 +37,7 @@ otx2_ca_enq(uintptr_t tag_op, const struct
> >> rte_event
> >> *ev)
> >>  		qp_id = m_data->request_info.queue_pair_id;
> >>  	} else if (crypto_op->sess_type == RTE_CRYPTO_OP_SESSIONLESS
> &&
> >>  		   crypto_op->private_data_offset) {
> >> -		m_data = (union rte_event_crypto_metadata *)
> >> +		m_data = (struct rte_event_crypto_metadata *)
> >>  			 ((uint8_t *)crypto_op +
> >>  			  crypto_op->private_data_offset);
> >>  		cdev_id = m_data->request_info.cdev_id; diff --git
> >> a/lib/eventdev/rte_event_crypto_adapter.c
> >> b/lib/eventdev/rte_event_crypto_adapter.c
> >> index e1d38d383d..6977391ae9 100644
> >> --- a/lib/eventdev/rte_event_crypto_adapter.c
> >> +++ b/lib/eventdev/rte_event_crypto_adapter.c
> >> @@ -333,7 +333,7 @@ eca_enq_to_cryptodev(struct
> >> rte_event_crypto_adapter *adapter,
> >>  		 struct rte_event *ev, unsigned int cnt)  {
> >>  	struct rte_event_crypto_adapter_stats *stats = &adapter-
> >> >crypto_stats;
> >> -	union rte_event_crypto_metadata *m_data = NULL;
> >> +	struct rte_event_crypto_metadata *m_data = NULL;
> >>  	struct crypto_queue_pair_info *qp_info = NULL;
> >>  	struct rte_crypto_op *crypto_op;
> >>  	unsigned int i, n;
> >> @@ -371,7 +371,7 @@ eca_enq_to_cryptodev(struct
> >> rte_event_crypto_adapter *adapter,
> >>  			len++;
> >>  		} else if (crypto_op->sess_type ==
> RTE_CRYPTO_OP_SESSIONLESS &&
> >>  				crypto_op->private_data_offset) {
> >> -			m_data = (union rte_event_crypto_metadata *)
> >> +			m_data = (struct rte_event_crypto_metadata *)
> >>  				 ((uint8_t *)crypto_op +
> >>  					crypto_op->private_data_offset);
> >>  			cdev_id = m_data->request_info.cdev_id; @@ -
> >> 504,7 +504,7 @@ eca_ops_enqueue_burst(struct
> rte_event_crypto_adapter
> >> *adapter,
> >>  		  struct rte_crypto_op **ops, uint16_t num)  {
> >>  	struct rte_event_crypto_adapter_stats *stats = &adapter-
> >> >crypto_stats;
> >> -	union rte_event_crypto_metadata *m_data = NULL;
> >> +	struct rte_event_crypto_metadata *m_data = NULL;
> >>  	uint8_t event_dev_id = adapter->eventdev_id;
> >>  	uint8_t event_port_id = adapter->event_port_id;
> >>  	struct rte_event events[BATCH_SIZE]; @@ -523,7 +523,7 @@
> >> eca_ops_enqueue_burst(struct rte_event_crypto_adapter *adapter,
> >>  					ops[i]->sym->session);
> >>  		} else if (ops[i]->sess_type ==
> >> RTE_CRYPTO_OP_SESSIONLESS &&
> >>  				ops[i]->private_data_offset) {
> >> -			m_data = (union rte_event_crypto_metadata *)
> >> +			m_data = (struct rte_event_crypto_metadata *)
> >>  				 ((uint8_t *)ops[i] +
> >>  				  ops[i]->private_data_offset);
> >>  		}
> >> diff --git a/lib/eventdev/rte_event_crypto_adapter.h
> >> b/lib/eventdev/rte_event_crypto_adapter.h
> >> index f8c6cca87c..3c24d9d9df 100644
> >> --- a/lib/eventdev/rte_event_crypto_adapter.h
> >> +++ b/lib/eventdev/rte_event_crypto_adapter.h
> >> @@ -200,11 +200,6 @@ enum rte_event_crypto_adapter_mode {
> >>   * provide event request information to the adapter.
> >>   */
> >>  struct rte_event_crypto_request {
> >> -	uint8_t resv[8];
> >> -	/**< Overlaps with first 8 bytes of struct rte_event
> >> -	 * that encode the response event information. Application
> >> -	 * is expected to fill in struct rte_event response_info.
> >> -	 */
> >>  	uint16_t cdev_id;
> >>  	/**< cryptodev ID to be used */
> >>  	uint16_t queue_pair_id;
> >> @@ -223,16 +218,16 @@ struct rte_event_crypto_request {
> >>   * operation. If the transfer is done by SW, event response information
> >>   * will be used by the adapter.
> >>   */
> >> -union rte_event_crypto_metadata {
> >> -	struct rte_event_crypto_request request_info;
> >> -	/**< Request information to be filled in by application
> >> -	 * for RTE_EVENT_CRYPTO_ADAPTER_OP_FORWARD mode.
> >> -	 */
> >> +struct rte_event_crypto_metadata {
> >>  	struct rte_event response_info;
> >>  	/**< Response information to be filled in by application
> >>  	 * for RTE_EVENT_CRYPTO_ADAPTER_OP_NEW and
> >>  	 * RTE_EVENT_CRYPTO_ADAPTER_OP_FORWARD mode.
> >>  	 */
> >> +	struct rte_event_crypto_request request_info;
> >> +	/**< Request information to be filled in by application
> >> +	 * for RTE_EVENT_CRYPTO_ADAPTER_OP_FORWARD mode.
> >> +	 */
> >>  };
> >>
> >>  /**
> >> --
> >> 2.25.1

Re: [dpdk-dev] [PATCH v3] eventdev: update crypto adapter metadata structures

[Shijith Thotton]

>>
>> >> In crypto adapter metadata, reserved bytes in request info structure
>> >> is a space holder for response info. It enforces an order of
>> >> operation if the structures are updated using memcpy to avoid
>> >> overwriting response info. It is logical to move the reserved space
>> >> out of request info. It also solves the ordering issue mentioned before.
>> >I would like to understand what kind of ordering issue you have faced
>> >with the current approach. Could you please give an example/sequence
>> and explain?
>> >
>>
>> I have seen this issue with crypto adapter autotest (#n215).
>>
>> Example:
>> rte_memcpy(&m_data.response_info, &response_info,
>> sizeof(response_info)); rte_memcpy(&m_data.request_info,
>> &request_info, sizeof(request_info));
>>
>> Here response info is getting overwritten by request info.
>> Above lines can reordered to fix the issue, but can be ignored with this patch.
>There is a reason for designing the metadata in this way.
>Right now, sizeof (union rte_event_crypto_metadata) is 16 bytes.
>So, the session based case needs just 16 bytes to store the data.
>Whereas, for sessionless case each crypto_ops requires another 16 bytes.
>
>By changing the struct in the following way you are doubling the memory
>requirement.
>With the changes, for sessionless case, each crypto op requires 32 bytes of space
>instead of 16 bytes and the mempool will be bigger.
>This will have the perf impact too!
>
>You can just copy the individual members(cdev_id & queue_pair_id) after the
>response_info.
>OR You have a better way?
>
 
I missed the second word of event structure. It adds an extra 8 bytes, which is not required.
Let me know, what you think of below change to metadata structure.

struct rte_event_crypto_metadata {
	uint64_t response_info; // 8 bytes
	struct rte_event_crypto_request request_info; // 8 bytes
};

Total structure size is 16 bytes.

Response info can be set like below in test app:
	m_data.response_info = response_info.event;

The main aim of this patch is to decouple response info from request info.

>>
>> >>
>> >> This patch removes the reserve field from request info and makes
>> >> event crypto metadata type to structure from union to make space for
>> >> response info.
>> >>
>> >> App and drivers are updated as per metadata change.
>> >>
>> >> Signed-off-by: Shijith Thotton <sthotton@marvell.com>
>> >> Acked-by: Anoob Joseph <anoobj@marvell.com>
>> >> ---
>> >> v3:
>> >> * Updated ABI section of release notes.
>> >>
>> >> v2:
>> >> * Updated deprecation notice.
>> >>
>> >> v1:
>> >> * Rebased.
>> >>
>> >>  app/test/test_event_crypto_adapter.c              | 14 +++++++-------
>> >>  doc/guides/rel_notes/deprecation.rst              |  6 ------
>> >>  doc/guides/rel_notes/release_21_11.rst            |  2 ++
>> >>  drivers/crypto/octeontx/otx_cryptodev_ops.c       |  8 ++++----
>> >>  drivers/crypto/octeontx2/otx2_cryptodev_ops.c     |  4 ++--
>> >>  .../event/octeontx2/otx2_evdev_crypto_adptr_tx.h  |  4 ++--
>> >>  lib/eventdev/rte_event_crypto_adapter.c           |  8 ++++----
>> >>  lib/eventdev/rte_event_crypto_adapter.h           | 15 +++++----------
>> >>  8 files changed, 26 insertions(+), 35 deletions(-)
>> >>
>> >> diff --git a/app/test/test_event_crypto_adapter.c
>> >> b/app/test/test_event_crypto_adapter.c
>> >> index 3ad20921e2..0d73694d3a 100644
>> >> --- a/app/test/test_event_crypto_adapter.c
>> >> +++ b/app/test/test_event_crypto_adapter.c
>> >> @@ -168,7 +168,7 @@ test_op_forward_mode(uint8_t session_less)  {
>> >>  	struct rte_crypto_sym_xform cipher_xform;
>> >>  	struct rte_cryptodev_sym_session *sess;
>> >> -	union rte_event_crypto_metadata m_data;
>> >> +	struct rte_event_crypto_metadata m_data;
>> >>  	struct rte_crypto_sym_op *sym_op;
>> >>  	struct rte_crypto_op *op;
>> >>  	struct rte_mbuf *m;
>> >> @@ -368,7 +368,7 @@ test_op_new_mode(uint8_t session_less)  {
>> >>  	struct rte_crypto_sym_xform cipher_xform;
>> >>  	struct rte_cryptodev_sym_session *sess;
>> >> -	union rte_event_crypto_metadata m_data;
>> >> +	struct rte_event_crypto_metadata m_data;
>> >>  	struct rte_crypto_sym_op *sym_op;
>> >>  	struct rte_crypto_op *op;
>> >>  	struct rte_mbuf *m;
>> >> @@ -406,7 +406,7 @@ test_op_new_mode(uint8_t session_less)
>> >>  		if (cap &
>> >> RTE_EVENT_CRYPTO_ADAPTER_CAP_SESSION_PRIVATE_DATA) {
>> >>  			/* Fill in private user data information */
>> >>  			rte_memcpy(&m_data.response_info,
>> &response_info,
>> >> -				   sizeof(m_data));
>> >> +				   sizeof(response_info));
>> >>  			rte_cryptodev_sym_session_set_user_data(sess,
>> >>  						&m_data, sizeof(m_data));
>> >>  		}
>> >> @@ -426,7 +426,7 @@ test_op_new_mode(uint8_t session_less)
>> >>  		op->private_data_offset = len;
>> >>  		/* Fill in private data information */
>> >>  		rte_memcpy(&m_data.response_info, &response_info,
>> >> -			   sizeof(m_data));
>> >> +			   sizeof(response_info));
>> >>  		rte_memcpy((uint8_t *)op + len, &m_data, sizeof(m_data));
>> >>  	}
>> >>
>> >> @@ -519,7 +519,7 @@ configure_cryptodev(void)
>> >>  			DEFAULT_NUM_XFORMS *
>> >>  			sizeof(struct rte_crypto_sym_xform) +
>> >>  			MAXIMUM_IV_LENGTH +
>> >> -			sizeof(union rte_event_crypto_metadata),
>> >> +			sizeof(struct rte_event_crypto_metadata),
>> >>  			rte_socket_id());
>> >>  	if (params.op_mpool == NULL) {
>> >>  		RTE_LOG(ERR, USER1, "Can't create CRYPTO_OP_POOL\n");
>> @@ -549,12
>> >> +549,12 @@ configure_cryptodev(void)
>> >>  	 * to include the session headers & private data
>> >>  	 */
>> >>  	session_size =
>> >> rte_cryptodev_sym_get_private_session_size(TEST_CDEV_ID);
>> >> -	session_size += sizeof(union rte_event_crypto_metadata);
>> >> +	session_size += sizeof(struct rte_event_crypto_metadata);
>> >>
>> >>  	params.session_mpool = rte_cryptodev_sym_session_pool_create(
>> >>  			"CRYPTO_ADAPTER_SESSION_MP",
>> >>  			MAX_NB_SESSIONS, 0, 0,
>> >> -			sizeof(union rte_event_crypto_metadata),
>> >> +			sizeof(struct rte_event_crypto_metadata),
>> >>  			SOCKET_ID_ANY);
>> >>  	TEST_ASSERT_NOT_NULL(params.session_mpool,
>> >>  			"session mempool allocation failed\n"); diff --git
>> >> a/doc/guides/rel_notes/deprecation.rst
>> >> b/doc/guides/rel_notes/deprecation.rst
>> >> index 76a4abfd6b..58ee95c020 100644
>> >> --- a/doc/guides/rel_notes/deprecation.rst
>> >> +++ b/doc/guides/rel_notes/deprecation.rst
>> >> @@ -266,12 +266,6 @@ Deprecation Notices
>> >>    values to the function ``rte_event_eth_rx_adapter_queue_add`` using
>> >>    the structure ``rte_event_eth_rx_adapter_queue_add``.
>> >>
>> >> -* eventdev: Reserved bytes of ``rte_event_crypto_request`` is a
>> >> space holder
>> >> -  for ``response_info``. Both should be decoupled for better clarity.
>> >> -  New space for ``response_info`` can be made by changing
>> >> -  ``rte_event_crypto_metadata`` type to structure from union.
>> >> -  This change is targeted for DPDK 21.11.
>> >> -
>> >>  * metrics: The function ``rte_metrics_init`` will have a non-void return
>> >>    in order to notify errors instead of calling ``rte_exit``.
>> >>
>> >> diff --git a/doc/guides/rel_notes/release_21_11.rst
>> >> b/doc/guides/rel_notes/release_21_11.rst
>> >> index d707a554ef..ab76d5dd55 100644
>> >> --- a/doc/guides/rel_notes/release_21_11.rst
>> >> +++ b/doc/guides/rel_notes/release_21_11.rst
>> >> @@ -100,6 +100,8 @@ ABI Changes
>> >>     Also, make sure to start the actual text at the margin.
>> >>
>> =======================================================
>> >>
>> >> +* eventdev: Modified type of ``union rte_event_crypto_metadata`` to
>> >> +struct and
>> >> +  removed reserved bytes from ``struct rte_event_crypto_request``.
>> >>
>> >>  Known Issues
>> >>  ------------
>> >> diff --git a/drivers/crypto/octeontx/otx_cryptodev_ops.c
>> >> b/drivers/crypto/octeontx/otx_cryptodev_ops.c
>> >> index eac6796cfb..c51be63146 100644
>> >> --- a/drivers/crypto/octeontx/otx_cryptodev_ops.c
>> >> +++ b/drivers/crypto/octeontx/otx_cryptodev_ops.c
>> >> @@ -710,17 +710,17 @@ submit_request_to_sso(struct ssows *ws,
>> >> uintptr_t req,
>> >>  	ssovf_store_pair(add_work, req, ws->grps[rsp_info->queue_id]);  }
>> >>
>> >> -static inline union rte_event_crypto_metadata *
>> >> +static inline struct rte_event_crypto_metadata *
>> >>  get_event_crypto_mdata(struct rte_crypto_op *op)  {
>> >> -	union rte_event_crypto_metadata *ec_mdata;
>> >> +	struct rte_event_crypto_metadata *ec_mdata;
>> >>
>> >>  	if (op->sess_type == RTE_CRYPTO_OP_WITH_SESSION)
>> >>  		ec_mdata = rte_cryptodev_sym_session_get_user_data(
>> >>  							   op->sym->session);
>> >>  	else if (op->sess_type == RTE_CRYPTO_OP_SESSIONLESS &&
>> >>  		 op->private_data_offset)
>> >> -		ec_mdata = (union rte_event_crypto_metadata *)
>> >> +		ec_mdata = (struct rte_event_crypto_metadata *)
>> >>  			((uint8_t *)op + op->private_data_offset);
>> >>  	else
>> >>  		return NULL;
>> >> @@ -731,7 +731,7 @@ get_event_crypto_mdata(struct rte_crypto_op
>> *op)
>> >> uint16_t __rte_hot  otx_crypto_adapter_enqueue(void *port, struct
>> >> rte_crypto_op *op)  {
>> >> -	union rte_event_crypto_metadata *ec_mdata;
>> >> +	struct rte_event_crypto_metadata *ec_mdata;
>> >>  	struct cpt_instance *instance;
>> >>  	struct cpt_request_info *req;
>> >>  	struct rte_event *rsp_info;
>> >> diff --git a/drivers/crypto/octeontx2/otx2_cryptodev_ops.c
>> >> b/drivers/crypto/octeontx2/otx2_cryptodev_ops.c
>> >> index 42100154cd..952d1352f4 100644
>> >> --- a/drivers/crypto/octeontx2/otx2_cryptodev_ops.c
>> >> +++ b/drivers/crypto/octeontx2/otx2_cryptodev_ops.c
>> >> @@ -453,7 +453,7 @@ otx2_ca_enqueue_req(const struct otx2_cpt_qp
>> *qp,
>> >>  		    struct rte_crypto_op *op,
>> >>  		    uint64_t cpt_inst_w7)
>> >>  {
>> >> -	union rte_event_crypto_metadata *m_data;
>> >> +	struct rte_event_crypto_metadata *m_data;
>> >>  	union cpt_inst_s inst;
>> >>  	uint64_t lmt_status;
>> >>
>> >> @@ -468,7 +468,7 @@ otx2_ca_enqueue_req(const struct otx2_cpt_qp
>> *qp,
>> >>  		}
>> >>  	} else if (op->sess_type == RTE_CRYPTO_OP_SESSIONLESS &&
>> >>  		   op->private_data_offset) {
>> >> -		m_data = (union rte_event_crypto_metadata *)
>> >> +		m_data = (struct rte_event_crypto_metadata *)
>> >>  			 ((uint8_t *)op +
>> >>  			  op->private_data_offset);
>> >>  	} else {
>> >> diff --git a/drivers/event/octeontx2/otx2_evdev_crypto_adptr_tx.h
>> >> b/drivers/event/octeontx2/otx2_evdev_crypto_adptr_tx.h
>> >> index ecf7eb9f56..458e8306d7 100644
>> >> --- a/drivers/event/octeontx2/otx2_evdev_crypto_adptr_tx.h
>> >> +++ b/drivers/event/octeontx2/otx2_evdev_crypto_adptr_tx.h
>> >> @@ -16,7 +16,7 @@
>> >>  static inline uint16_t
>> >>  otx2_ca_enq(uintptr_t tag_op, const struct rte_event *ev)  {
>> >> -	union rte_event_crypto_metadata *m_data;
>> >> +	struct rte_event_crypto_metadata *m_data;
>> >>  	struct rte_crypto_op *crypto_op;
>> >>  	struct rte_cryptodev *cdev;
>> >>  	struct otx2_cpt_qp *qp;
>> >> @@ -37,7 +37,7 @@ otx2_ca_enq(uintptr_t tag_op, const struct
>> >> rte_event
>> >> *ev)
>> >>  		qp_id = m_data->request_info.queue_pair_id;
>> >>  	} else if (crypto_op->sess_type == RTE_CRYPTO_OP_SESSIONLESS
>> &&
>> >>  		   crypto_op->private_data_offset) {
>> >> -		m_data = (union rte_event_crypto_metadata *)
>> >> +		m_data = (struct rte_event_crypto_metadata *)
>> >>  			 ((uint8_t *)crypto_op +
>> >>  			  crypto_op->private_data_offset);
>> >>  		cdev_id = m_data->request_info.cdev_id; diff --git
>> >> a/lib/eventdev/rte_event_crypto_adapter.c
>> >> b/lib/eventdev/rte_event_crypto_adapter.c
>> >> index e1d38d383d..6977391ae9 100644
>> >> --- a/lib/eventdev/rte_event_crypto_adapter.c
>> >> +++ b/lib/eventdev/rte_event_crypto_adapter.c
>> >> @@ -333,7 +333,7 @@ eca_enq_to_cryptodev(struct
>> >> rte_event_crypto_adapter *adapter,
>> >>  		 struct rte_event *ev, unsigned int cnt)  {
>> >>  	struct rte_event_crypto_adapter_stats *stats = &adapter-
>> >> >crypto_stats;
>> >> -	union rte_event_crypto_metadata *m_data = NULL;
>> >> +	struct rte_event_crypto_metadata *m_data = NULL;
>> >>  	struct crypto_queue_pair_info *qp_info = NULL;
>> >>  	struct rte_crypto_op *crypto_op;
>> >>  	unsigned int i, n;
>> >> @@ -371,7 +371,7 @@ eca_enq_to_cryptodev(struct
>> >> rte_event_crypto_adapter *adapter,
>> >>  			len++;
>> >>  		} else if (crypto_op->sess_type ==
>> RTE_CRYPTO_OP_SESSIONLESS &&
>> >>  				crypto_op->private_data_offset) {
>> >> -			m_data = (union rte_event_crypto_metadata *)
>> >> +			m_data = (struct rte_event_crypto_metadata *)
>> >>  				 ((uint8_t *)crypto_op +
>> >>  					crypto_op->private_data_offset);
>> >>  			cdev_id = m_data->request_info.cdev_id; @@ -
>> >> 504,7 +504,7 @@ eca_ops_enqueue_burst(struct
>> rte_event_crypto_adapter
>> >> *adapter,
>> >>  		  struct rte_crypto_op **ops, uint16_t num)  {
>> >>  	struct rte_event_crypto_adapter_stats *stats = &adapter-
>> >> >crypto_stats;
>> >> -	union rte_event_crypto_metadata *m_data = NULL;
>> >> +	struct rte_event_crypto_metadata *m_data = NULL;
>> >>  	uint8_t event_dev_id = adapter->eventdev_id;
>> >>  	uint8_t event_port_id = adapter->event_port_id;
>> >>  	struct rte_event events[BATCH_SIZE]; @@ -523,7 +523,7 @@
>> >> eca_ops_enqueue_burst(struct rte_event_crypto_adapter *adapter,
>> >>  					ops[i]->sym->session);
>> >>  		} else if (ops[i]->sess_type ==
>> >> RTE_CRYPTO_OP_SESSIONLESS &&
>> >>  				ops[i]->private_data_offset) {
>> >> -			m_data = (union rte_event_crypto_metadata *)
>> >> +			m_data = (struct rte_event_crypto_metadata *)
>> >>  				 ((uint8_t *)ops[i] +
>> >>  				  ops[i]->private_data_offset);
>> >>  		}
>> >> diff --git a/lib/eventdev/rte_event_crypto_adapter.h
>> >> b/lib/eventdev/rte_event_crypto_adapter.h
>> >> index f8c6cca87c..3c24d9d9df 100644
>> >> --- a/lib/eventdev/rte_event_crypto_adapter.h
>> >> +++ b/lib/eventdev/rte_event_crypto_adapter.h
>> >> @@ -200,11 +200,6 @@ enum rte_event_crypto_adapter_mode {
>> >>   * provide event request information to the adapter.
>> >>   */
>> >>  struct rte_event_crypto_request {
>> >> -	uint8_t resv[8];
>> >> -	/**< Overlaps with first 8 bytes of struct rte_event
>> >> -	 * that encode the response event information. Application
>> >> -	 * is expected to fill in struct rte_event response_info.
>> >> -	 */
>> >>  	uint16_t cdev_id;
>> >>  	/**< cryptodev ID to be used */
>> >>  	uint16_t queue_pair_id;
>> >> @@ -223,16 +218,16 @@ struct rte_event_crypto_request {
>> >>   * operation. If the transfer is done by SW, event response information
>> >>   * will be used by the adapter.
>> >>   */
>> >> -union rte_event_crypto_metadata {
>> >> -	struct rte_event_crypto_request request_info;
>> >> -	/**< Request information to be filled in by application
>> >> -	 * for RTE_EVENT_CRYPTO_ADAPTER_OP_FORWARD mode.
>> >> -	 */
>> >> +struct rte_event_crypto_metadata {
>> >>  	struct rte_event response_info;
>> >>  	/**< Response information to be filled in by application
>> >>  	 * for RTE_EVENT_CRYPTO_ADAPTER_OP_NEW and
>> >>  	 * RTE_EVENT_CRYPTO_ADAPTER_OP_FORWARD mode.
>> >>  	 */
>> >> +	struct rte_event_crypto_request request_info;
>> >> +	/**< Request information to be filled in by application
>> >> +	 * for RTE_EVENT_CRYPTO_ADAPTER_OP_FORWARD mode.
>> >> +	 */
>> >>  };
>> >>
>> >>  /**
>> >> --
>> >> 2.25.1

Re: [dpdk-dev] [PATCH v3] eventdev: update crypto adapter metadata structures

[Gujjar, Abhinandan S]

Hi Shijith,

> -----Original Message-----
> From: Shijith Thotton <sthotton@marvell.com>
> Sent: Wednesday, September 8, 2021 1:13 PM
> To: Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>; dev@dpdk.org
> Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Anoob Joseph
> <anoobj@marvell.com>; Pavan Nikhilesh Bhagavatula
> <pbhagavatula@marvell.com>; Akhil Goyal <gakhil@marvell.com>; Ray
> Kinsella <mdr@ashroe.eu>; Ankur Dwivedi <adwivedi@marvell.com>
> Subject: RE: [PATCH v3] eventdev: update crypto adapter metadata
> structures
> 
> >>
> >> >> In crypto adapter metadata, reserved bytes in request info
> >> >> structure is a space holder for response info. It enforces an
> >> >> order of operation if the structures are updated using memcpy to
> >> >> avoid overwriting response info. It is logical to move the
> >> >> reserved space out of request info. It also solves the ordering issue
> mentioned before.
> >> >I would like to understand what kind of ordering issue you have
> >> >faced with the current approach. Could you please give an
> >> >example/sequence
> >> and explain?
> >> >
> >>
> >> I have seen this issue with crypto adapter autotest (#n215).
> >>
> >> Example:
> >> rte_memcpy(&m_data.response_info, &response_info,
> >> sizeof(response_info)); rte_memcpy(&m_data.request_info,
> >> &request_info, sizeof(request_info));
> >>
> >> Here response info is getting overwritten by request info.
> >> Above lines can reordered to fix the issue, but can be ignored with this
> patch.
> >There is a reason for designing the metadata in this way.
> >Right now, sizeof (union rte_event_crypto_metadata) is 16 bytes.
> >So, the session based case needs just 16 bytes to store the data.
> >Whereas, for sessionless case each crypto_ops requires another 16 bytes.
> >
> >By changing the struct in the following way you are doubling the memory
> >requirement.
> >With the changes, for sessionless case, each crypto op requires 32
> >bytes of space instead of 16 bytes and the mempool will be bigger.
> >This will have the perf impact too!
> >
> >You can just copy the individual members(cdev_id & queue_pair_id) after
> >the response_info.
> >OR You have a better way?
> >
> 
> I missed the second word of event structure. It adds an extra 8 bytes, which
> is not required.
I guess you meant not adding, it is overlapping with request information.
> Let me know, what you think of below change to metadata structure.
> 
> struct rte_event_crypto_metadata {
> 	uint64_t response_info; // 8 bytes
With this, it lags clarity saying it is an event information.
> 	struct rte_event_crypto_request request_info; // 8 bytes };
> 
> Total structure size is 16 bytes.
> 
> Response info can be set like below in test app:
> 	m_data.response_info = response_info.event;
> 
> The main aim of this patch is to decouple response info from request info.
The decoupling was required as it was doing memcpy.
If you copy the individual members of request info(after response), you don't require it.
> 
> >>
> >> >>
> >> >> This patch removes the reserve field from request info and makes
> >> >> event crypto metadata type to structure from union to make space
> >> >> for response info.
> >> >>
> >> >> App and drivers are updated as per metadata change.
> >> >>
> >> >> Signed-off-by: Shijith Thotton <sthotton@marvell.com>
> >> >> Acked-by: Anoob Joseph <anoobj@marvell.com>
> >> >> ---
> >> >> v3:
> >> >> * Updated ABI section of release notes.
> >> >>
> >> >> v2:
> >> >> * Updated deprecation notice.
> >> >>
> >> >> v1:
> >> >> * Rebased.
> >> >>
> >> >>  app/test/test_event_crypto_adapter.c              | 14 +++++++-------
> >> >>  doc/guides/rel_notes/deprecation.rst              |  6 ------
> >> >>  doc/guides/rel_notes/release_21_11.rst            |  2 ++
> >> >>  drivers/crypto/octeontx/otx_cryptodev_ops.c       |  8 ++++----
> >> >>  drivers/crypto/octeontx2/otx2_cryptodev_ops.c     |  4 ++--
> >> >>  .../event/octeontx2/otx2_evdev_crypto_adptr_tx.h  |  4 ++--
> >> >>  lib/eventdev/rte_event_crypto_adapter.c           |  8 ++++----
> >> >>  lib/eventdev/rte_event_crypto_adapter.h           | 15 +++++----------
> >> >>  8 files changed, 26 insertions(+), 35 deletions(-)
> >> >>
> >> >> diff --git a/app/test/test_event_crypto_adapter.c
> >> >> b/app/test/test_event_crypto_adapter.c
> >> >> index 3ad20921e2..0d73694d3a 100644
> >> >> --- a/app/test/test_event_crypto_adapter.c
> >> >> +++ b/app/test/test_event_crypto_adapter.c
> >> >> @@ -168,7 +168,7 @@ test_op_forward_mode(uint8_t session_less)
> {
> >> >>  	struct rte_crypto_sym_xform cipher_xform;
> >> >>  	struct rte_cryptodev_sym_session *sess;
> >> >> -	union rte_event_crypto_metadata m_data;
> >> >> +	struct rte_event_crypto_metadata m_data;
> >> >>  	struct rte_crypto_sym_op *sym_op;
> >> >>  	struct rte_crypto_op *op;
> >> >>  	struct rte_mbuf *m;
> >> >> @@ -368,7 +368,7 @@ test_op_new_mode(uint8_t session_less)  {
> >> >>  	struct rte_crypto_sym_xform cipher_xform;
> >> >>  	struct rte_cryptodev_sym_session *sess;
> >> >> -	union rte_event_crypto_metadata m_data;
> >> >> +	struct rte_event_crypto_metadata m_data;
> >> >>  	struct rte_crypto_sym_op *sym_op;
> >> >>  	struct rte_crypto_op *op;
> >> >>  	struct rte_mbuf *m;
> >> >> @@ -406,7 +406,7 @@ test_op_new_mode(uint8_t session_less)
> >> >>  		if (cap &
> >> >> RTE_EVENT_CRYPTO_ADAPTER_CAP_SESSION_PRIVATE_DATA) {
> >> >>  			/* Fill in private user data information */
> >> >>  			rte_memcpy(&m_data.response_info,
> >> &response_info,
> >> >> -				   sizeof(m_data));
> >> >> +				   sizeof(response_info));
> >> >>  			rte_cryptodev_sym_session_set_user_data(sess,
> >> >>  						&m_data, sizeof(m_data));
> >> >>  		}
> >> >> @@ -426,7 +426,7 @@ test_op_new_mode(uint8_t session_less)
> >> >>  		op->private_data_offset = len;
> >> >>  		/* Fill in private data information */
> >> >>  		rte_memcpy(&m_data.response_info, &response_info,
> >> >> -			   sizeof(m_data));
> >> >> +			   sizeof(response_info));
> >> >>  		rte_memcpy((uint8_t *)op + len, &m_data, sizeof(m_data));
> >> >>  	}
> >> >>
> >> >> @@ -519,7 +519,7 @@ configure_cryptodev(void)
> >> >>  			DEFAULT_NUM_XFORMS *
> >> >>  			sizeof(struct rte_crypto_sym_xform) +
> >> >>  			MAXIMUM_IV_LENGTH +
> >> >> -			sizeof(union rte_event_crypto_metadata),
> >> >> +			sizeof(struct rte_event_crypto_metadata),
> >> >>  			rte_socket_id());
> >> >>  	if (params.op_mpool == NULL) {
> >> >>  		RTE_LOG(ERR, USER1, "Can't create CRYPTO_OP_POOL\n");
> >> @@ -549,12
> >> >> +549,12 @@ configure_cryptodev(void)
> >> >>  	 * to include the session headers & private data
> >> >>  	 */
> >> >>  	session_size =
> >> >> rte_cryptodev_sym_get_private_session_size(TEST_CDEV_ID);
> >> >> -	session_size += sizeof(union rte_event_crypto_metadata);
> >> >> +	session_size += sizeof(struct rte_event_crypto_metadata);
> >> >>
> >> >>  	params.session_mpool = rte_cryptodev_sym_session_pool_create(
> >> >>  			"CRYPTO_ADAPTER_SESSION_MP",
> >> >>  			MAX_NB_SESSIONS, 0, 0,
> >> >> -			sizeof(union rte_event_crypto_metadata),
> >> >> +			sizeof(struct rte_event_crypto_metadata),
> >> >>  			SOCKET_ID_ANY);
> >> >>  	TEST_ASSERT_NOT_NULL(params.session_mpool,
> >> >>  			"session mempool allocation failed\n"); diff --git
> >> >> a/doc/guides/rel_notes/deprecation.rst
> >> >> b/doc/guides/rel_notes/deprecation.rst
> >> >> index 76a4abfd6b..58ee95c020 100644
> >> >> --- a/doc/guides/rel_notes/deprecation.rst
> >> >> +++ b/doc/guides/rel_notes/deprecation.rst
> >> >> @@ -266,12 +266,6 @@ Deprecation Notices
> >> >>    values to the function ``rte_event_eth_rx_adapter_queue_add``
> using
> >> >>    the structure ``rte_event_eth_rx_adapter_queue_add``.
> >> >>
> >> >> -* eventdev: Reserved bytes of ``rte_event_crypto_request`` is a
> >> >> space holder
> >> >> -  for ``response_info``. Both should be decoupled for better clarity.
> >> >> -  New space for ``response_info`` can be made by changing
> >> >> -  ``rte_event_crypto_metadata`` type to structure from union.
> >> >> -  This change is targeted for DPDK 21.11.
> >> >> -
> >> >>  * metrics: The function ``rte_metrics_init`` will have a non-void return
> >> >>    in order to notify errors instead of calling ``rte_exit``.
> >> >>
> >> >> diff --git a/doc/guides/rel_notes/release_21_11.rst
> >> >> b/doc/guides/rel_notes/release_21_11.rst
> >> >> index d707a554ef..ab76d5dd55 100644
> >> >> --- a/doc/guides/rel_notes/release_21_11.rst
> >> >> +++ b/doc/guides/rel_notes/release_21_11.rst
> >> >> @@ -100,6 +100,8 @@ ABI Changes
> >> >>     Also, make sure to start the actual text at the margin.
> >> >>
> >> =======================================================
> >> >>
> >> >> +* eventdev: Modified type of ``union rte_event_crypto_metadata``
> >> >> +to struct and
> >> >> +  removed reserved bytes from ``struct rte_event_crypto_request``.
> >> >>
> >> >>  Known Issues
> >> >>  ------------
> >> >> diff --git a/drivers/crypto/octeontx/otx_cryptodev_ops.c
> >> >> b/drivers/crypto/octeontx/otx_cryptodev_ops.c
> >> >> index eac6796cfb..c51be63146 100644
> >> >> --- a/drivers/crypto/octeontx/otx_cryptodev_ops.c
> >> >> +++ b/drivers/crypto/octeontx/otx_cryptodev_ops.c
> >> >> @@ -710,17 +710,17 @@ submit_request_to_sso(struct ssows *ws,
> >> >> uintptr_t req,
> >> >>  	ssovf_store_pair(add_work, req, ws->grps[rsp_info->queue_id]);
> >> >> }
> >> >>
> >> >> -static inline union rte_event_crypto_metadata *
> >> >> +static inline struct rte_event_crypto_metadata *
> >> >>  get_event_crypto_mdata(struct rte_crypto_op *op)  {
> >> >> -	union rte_event_crypto_metadata *ec_mdata;
> >> >> +	struct rte_event_crypto_metadata *ec_mdata;
> >> >>
> >> >>  	if (op->sess_type == RTE_CRYPTO_OP_WITH_SESSION)
> >> >>  		ec_mdata = rte_cryptodev_sym_session_get_user_data(
> >> >>  							   op->sym->session);
> >> >>  	else if (op->sess_type == RTE_CRYPTO_OP_SESSIONLESS &&
> >> >>  		 op->private_data_offset)
> >> >> -		ec_mdata = (union rte_event_crypto_metadata *)
> >> >> +		ec_mdata = (struct rte_event_crypto_metadata *)
> >> >>  			((uint8_t *)op + op->private_data_offset);
> >> >>  	else
> >> >>  		return NULL;
> >> >> @@ -731,7 +731,7 @@ get_event_crypto_mdata(struct rte_crypto_op
> >> *op)
> >> >> uint16_t __rte_hot  otx_crypto_adapter_enqueue(void *port, struct
> >> >> rte_crypto_op *op)  {
> >> >> -	union rte_event_crypto_metadata *ec_mdata;
> >> >> +	struct rte_event_crypto_metadata *ec_mdata;
> >> >>  	struct cpt_instance *instance;
> >> >>  	struct cpt_request_info *req;
> >> >>  	struct rte_event *rsp_info;
> >> >> diff --git a/drivers/crypto/octeontx2/otx2_cryptodev_ops.c
> >> >> b/drivers/crypto/octeontx2/otx2_cryptodev_ops.c
> >> >> index 42100154cd..952d1352f4 100644
> >> >> --- a/drivers/crypto/octeontx2/otx2_cryptodev_ops.c
> >> >> +++ b/drivers/crypto/octeontx2/otx2_cryptodev_ops.c
> >> >> @@ -453,7 +453,7 @@ otx2_ca_enqueue_req(const struct
> otx2_cpt_qp
> >> *qp,
> >> >>  		    struct rte_crypto_op *op,
> >> >>  		    uint64_t cpt_inst_w7)
> >> >>  {
> >> >> -	union rte_event_crypto_metadata *m_data;
> >> >> +	struct rte_event_crypto_metadata *m_data;
> >> >>  	union cpt_inst_s inst;
> >> >>  	uint64_t lmt_status;
> >> >>
> >> >> @@ -468,7 +468,7 @@ otx2_ca_enqueue_req(const struct
> otx2_cpt_qp
> >> *qp,
> >> >>  		}
> >> >>  	} else if (op->sess_type == RTE_CRYPTO_OP_SESSIONLESS &&
> >> >>  		   op->private_data_offset) {
> >> >> -		m_data = (union rte_event_crypto_metadata *)
> >> >> +		m_data = (struct rte_event_crypto_metadata *)
> >> >>  			 ((uint8_t *)op +
> >> >>  			  op->private_data_offset);
> >> >>  	} else {
> >> >> diff --git a/drivers/event/octeontx2/otx2_evdev_crypto_adptr_tx.h
> >> >> b/drivers/event/octeontx2/otx2_evdev_crypto_adptr_tx.h
> >> >> index ecf7eb9f56..458e8306d7 100644
> >> >> --- a/drivers/event/octeontx2/otx2_evdev_crypto_adptr_tx.h
> >> >> +++ b/drivers/event/octeontx2/otx2_evdev_crypto_adptr_tx.h
> >> >> @@ -16,7 +16,7 @@
> >> >>  static inline uint16_t
> >> >>  otx2_ca_enq(uintptr_t tag_op, const struct rte_event *ev)  {
> >> >> -	union rte_event_crypto_metadata *m_data;
> >> >> +	struct rte_event_crypto_metadata *m_data;
> >> >>  	struct rte_crypto_op *crypto_op;
> >> >>  	struct rte_cryptodev *cdev;
> >> >>  	struct otx2_cpt_qp *qp;
> >> >> @@ -37,7 +37,7 @@ otx2_ca_enq(uintptr_t tag_op, const struct
> >> >> rte_event
> >> >> *ev)
> >> >>  		qp_id = m_data->request_info.queue_pair_id;
> >> >>  	} else if (crypto_op->sess_type == RTE_CRYPTO_OP_SESSIONLESS
> >> &&
> >> >>  		   crypto_op->private_data_offset) {
> >> >> -		m_data = (union rte_event_crypto_metadata *)
> >> >> +		m_data = (struct rte_event_crypto_metadata *)
> >> >>  			 ((uint8_t *)crypto_op +
> >> >>  			  crypto_op->private_data_offset);
> >> >>  		cdev_id = m_data->request_info.cdev_id; diff --git
> >> >> a/lib/eventdev/rte_event_crypto_adapter.c
> >> >> b/lib/eventdev/rte_event_crypto_adapter.c
> >> >> index e1d38d383d..6977391ae9 100644
> >> >> --- a/lib/eventdev/rte_event_crypto_adapter.c
> >> >> +++ b/lib/eventdev/rte_event_crypto_adapter.c
> >> >> @@ -333,7 +333,7 @@ eca_enq_to_cryptodev(struct
> >> >> rte_event_crypto_adapter *adapter,
> >> >>  		 struct rte_event *ev, unsigned int cnt)  {
> >> >>  	struct rte_event_crypto_adapter_stats *stats = &adapter-
> >> >> >crypto_stats;
> >> >> -	union rte_event_crypto_metadata *m_data = NULL;
> >> >> +	struct rte_event_crypto_metadata *m_data = NULL;
> >> >>  	struct crypto_queue_pair_info *qp_info = NULL;
> >> >>  	struct rte_crypto_op *crypto_op;
> >> >>  	unsigned int i, n;
> >> >> @@ -371,7 +371,7 @@ eca_enq_to_cryptodev(struct
> >> >> rte_event_crypto_adapter *adapter,
> >> >>  			len++;
> >> >>  		} else if (crypto_op->sess_type ==
> >> RTE_CRYPTO_OP_SESSIONLESS &&
> >> >>  				crypto_op->private_data_offset) {
> >> >> -			m_data = (union rte_event_crypto_metadata *)
> >> >> +			m_data = (struct rte_event_crypto_metadata *)
> >> >>  				 ((uint8_t *)crypto_op +
> >> >>  					crypto_op->private_data_offset);
> >> >>  			cdev_id = m_data->request_info.cdev_id; @@ -
> >> >> 504,7 +504,7 @@ eca_ops_enqueue_burst(struct
> >> rte_event_crypto_adapter
> >> >> *adapter,
> >> >>  		  struct rte_crypto_op **ops, uint16_t num)  {
> >> >>  	struct rte_event_crypto_adapter_stats *stats = &adapter-
> >> >> >crypto_stats;
> >> >> -	union rte_event_crypto_metadata *m_data = NULL;
> >> >> +	struct rte_event_crypto_metadata *m_data = NULL;
> >> >>  	uint8_t event_dev_id = adapter->eventdev_id;
> >> >>  	uint8_t event_port_id = adapter->event_port_id;
> >> >>  	struct rte_event events[BATCH_SIZE]; @@ -523,7 +523,7 @@
> >> >> eca_ops_enqueue_burst(struct rte_event_crypto_adapter *adapter,
> >> >>  					ops[i]->sym->session);
> >> >>  		} else if (ops[i]->sess_type ==
> RTE_CRYPTO_OP_SESSIONLESS &&
> >> >>  				ops[i]->private_data_offset) {
> >> >> -			m_data = (union rte_event_crypto_metadata *)
> >> >> +			m_data = (struct rte_event_crypto_metadata *)
> >> >>  				 ((uint8_t *)ops[i] +
> >> >>  				  ops[i]->private_data_offset);
> >> >>  		}
> >> >> diff --git a/lib/eventdev/rte_event_crypto_adapter.h
> >> >> b/lib/eventdev/rte_event_crypto_adapter.h
> >> >> index f8c6cca87c..3c24d9d9df 100644
> >> >> --- a/lib/eventdev/rte_event_crypto_adapter.h
> >> >> +++ b/lib/eventdev/rte_event_crypto_adapter.h
> >> >> @@ -200,11 +200,6 @@ enum rte_event_crypto_adapter_mode {
> >> >>   * provide event request information to the adapter.
> >> >>   */
> >> >>  struct rte_event_crypto_request {
> >> >> -	uint8_t resv[8];
> >> >> -	/**< Overlaps with first 8 bytes of struct rte_event
> >> >> -	 * that encode the response event information. Application
> >> >> -	 * is expected to fill in struct rte_event response_info.
> >> >> -	 */
> >> >>  	uint16_t cdev_id;
> >> >>  	/**< cryptodev ID to be used */
> >> >>  	uint16_t queue_pair_id;
> >> >> @@ -223,16 +218,16 @@ struct rte_event_crypto_request {
> >> >>   * operation. If the transfer is done by SW, event response
> information
> >> >>   * will be used by the adapter.
> >> >>   */
> >> >> -union rte_event_crypto_metadata {
> >> >> -	struct rte_event_crypto_request request_info;
> >> >> -	/**< Request information to be filled in by application
> >> >> -	 * for RTE_EVENT_CRYPTO_ADAPTER_OP_FORWARD mode.
> >> >> -	 */
> >> >> +struct rte_event_crypto_metadata {
> >> >>  	struct rte_event response_info;
> >> >>  	/**< Response information to be filled in by application
> >> >>  	 * for RTE_EVENT_CRYPTO_ADAPTER_OP_NEW and
> >> >>  	 * RTE_EVENT_CRYPTO_ADAPTER_OP_FORWARD mode.
> >> >>  	 */
> >> >> +	struct rte_event_crypto_request request_info;
> >> >> +	/**< Request information to be filled in by application
> >> >> +	 * for RTE_EVENT_CRYPTO_ADAPTER_OP_FORWARD mode.
> >> >> +	 */
> >> >>  };
> >> >>
> >> >>  /**
> >> >> --
> >> >> 2.25.1

Re: [dpdk-dev] [PATCH v3] eventdev: update crypto adapter metadata structures

[Shijith Thotton]

>> >>
>> >> >> In crypto adapter metadata, reserved bytes in request info
>> >> >> structure is a space holder for response info. It enforces an
>> >> >> order of operation if the structures are updated using memcpy to
>> >> >> avoid overwriting response info. It is logical to move the
>> >> >> reserved space out of request info. It also solves the ordering issue
>> mentioned before.
>> >> >I would like to understand what kind of ordering issue you have
>> >> >faced with the current approach. Could you please give an
>> >> >example/sequence
>> >> and explain?
>> >> >
>> >>
>> >> I have seen this issue with crypto adapter autotest (#n215).
>> >>
>> >> Example:
>> >> rte_memcpy(&m_data.response_info, &response_info,
>> >> sizeof(response_info)); rte_memcpy(&m_data.request_info,
>> >> &request_info, sizeof(request_info));
>> >>
>> >> Here response info is getting overwritten by request info.
>> >> Above lines can reordered to fix the issue, but can be ignored with this
>> patch.
>> >There is a reason for designing the metadata in this way.
>> >Right now, sizeof (union rte_event_crypto_metadata) is 16 bytes.
>> >So, the session based case needs just 16 bytes to store the data.
>> >Whereas, for sessionless case each crypto_ops requires another 16 bytes.
>> >
>> >By changing the struct in the following way you are doubling the memory
>> >requirement.
>> >With the changes, for sessionless case, each crypto op requires 32
>> >bytes of space instead of 16 bytes and the mempool will be bigger.
>> >This will have the perf impact too!
>> >
>> >You can just copy the individual members(cdev_id & queue_pair_id) after
>> >the response_info.
>> >OR You have a better way?
>> >
>>
>> I missed the second word of event structure. It adds an extra 8 bytes, which
>> is not required.
>I guess you meant not adding, it is overlapping with request information.
>> Let me know, what you think of below change to metadata structure.
>>
>> struct rte_event_crypto_metadata {
>> 	uint64_t response_info; // 8 bytes
>With this, it lags clarity saying it is an event information.
>> 	struct rte_event_crypto_request request_info; // 8 bytes };
>>
>> Total structure size is 16 bytes.
>>
>> Response info can be set like below in test app:
>> 	m_data.response_info = response_info.event;
>>
>> The main aim of this patch is to decouple response info from request info.
>The decoupling was required as it was doing memcpy.
>If you copy the individual members of request info(after response), you don't
>require it.
 
With this change, the application will be free of such constraints.

>>
>> >>
>> >> >>
>> >> >> This patch removes the reserve field from request info and makes
>> >> >> event crypto metadata type to structure from union to make space
>> >> >> for response info.
>> >> >>
>> >> >> App and drivers are updated as per metadata change.
>> >> >>
>> >> >> Signed-off-by: Shijith Thotton <sthotton@marvell.com>
>> >> >> Acked-by: Anoob Joseph <anoobj@marvell.com>
>> >> >> ---
>> >> >> v3:
>> >> >> * Updated ABI section of release notes.
>> >> >>
>> >> >> v2:
>> >> >> * Updated deprecation notice.
>> >> >>
>> >> >> v1:
>> >> >> * Rebased.
>> >> >>
>> >> >>  app/test/test_event_crypto_adapter.c              | 14 +++++++-------
>> >> >>  doc/guides/rel_notes/deprecation.rst              |  6 ------
>> >> >>  doc/guides/rel_notes/release_21_11.rst            |  2 ++
>> >> >>  drivers/crypto/octeontx/otx_cryptodev_ops.c       |  8 ++++----
>> >> >>  drivers/crypto/octeontx2/otx2_cryptodev_ops.c     |  4 ++--
>> >> >>  .../event/octeontx2/otx2_evdev_crypto_adptr_tx.h  |  4 ++--
>> >> >>  lib/eventdev/rte_event_crypto_adapter.c           |  8 ++++----
>> >> >>  lib/eventdev/rte_event_crypto_adapter.h           | 15 +++++----------
>> >> >>  8 files changed, 26 insertions(+), 35 deletions(-)
>> >> >>
>> >> >> diff --git a/app/test/test_event_crypto_adapter.c
>> >> >> b/app/test/test_event_crypto_adapter.c
>> >> >> index 3ad20921e2..0d73694d3a 100644
>> >> >> --- a/app/test/test_event_crypto_adapter.c
>> >> >> +++ b/app/test/test_event_crypto_adapter.c
>> >> >> @@ -168,7 +168,7 @@ test_op_forward_mode(uint8_t session_less)
>> {
>> >> >>  	struct rte_crypto_sym_xform cipher_xform;
>> >> >>  	struct rte_cryptodev_sym_session *sess;
>> >> >> -	union rte_event_crypto_metadata m_data;
>> >> >> +	struct rte_event_crypto_metadata m_data;
>> >> >>  	struct rte_crypto_sym_op *sym_op;
>> >> >>  	struct rte_crypto_op *op;
>> >> >>  	struct rte_mbuf *m;
>> >> >> @@ -368,7 +368,7 @@ test_op_new_mode(uint8_t session_less)  {
>> >> >>  	struct rte_crypto_sym_xform cipher_xform;
>> >> >>  	struct rte_cryptodev_sym_session *sess;
>> >> >> -	union rte_event_crypto_metadata m_data;
>> >> >> +	struct rte_event_crypto_metadata m_data;
>> >> >>  	struct rte_crypto_sym_op *sym_op;
>> >> >>  	struct rte_crypto_op *op;
>> >> >>  	struct rte_mbuf *m;
>> >> >> @@ -406,7 +406,7 @@ test_op_new_mode(uint8_t session_less)
>> >> >>  		if (cap &
>> >> >> RTE_EVENT_CRYPTO_ADAPTER_CAP_SESSION_PRIVATE_DATA) {
>> >> >>  			/* Fill in private user data information */
>> >> >>  			rte_memcpy(&m_data.response_info,
>> >> &response_info,
>> >> >> -				   sizeof(m_data));
>> >> >> +				   sizeof(response_info));
>> >> >>  			rte_cryptodev_sym_session_set_user_data(sess,
>> >> >>  						&m_data, sizeof(m_data));
>> >> >>  		}
>> >> >> @@ -426,7 +426,7 @@ test_op_new_mode(uint8_t session_less)
>> >> >>  		op->private_data_offset = len;
>> >> >>  		/* Fill in private data information */
>> >> >>  		rte_memcpy(&m_data.response_info, &response_info,
>> >> >> -			   sizeof(m_data));
>> >> >> +			   sizeof(response_info));
>> >> >>  		rte_memcpy((uint8_t *)op + len, &m_data,
>sizeof(m_data));
>> >> >>  	}
>> >> >>
>> >> >> @@ -519,7 +519,7 @@ configure_cryptodev(void)
>> >> >>  			DEFAULT_NUM_XFORMS *
>> >> >>  			sizeof(struct rte_crypto_sym_xform) +
>> >> >>  			MAXIMUM_IV_LENGTH +
>> >> >> -			sizeof(union rte_event_crypto_metadata),
>> >> >> +			sizeof(struct rte_event_crypto_metadata),
>> >> >>  			rte_socket_id());
>> >> >>  	if (params.op_mpool == NULL) {
>> >> >>  		RTE_LOG(ERR, USER1, "Can't create
>CRYPTO_OP_POOL\n");
>> >> @@ -549,12
>> >> >> +549,12 @@ configure_cryptodev(void)
>> >> >>  	 * to include the session headers & private data
>> >> >>  	 */
>> >> >>  	session_size =
>> >> >> rte_cryptodev_sym_get_private_session_size(TEST_CDEV_ID);
>> >> >> -	session_size += sizeof(union rte_event_crypto_metadata);
>> >> >> +	session_size += sizeof(struct rte_event_crypto_metadata);
>> >> >>
>> >> >>  	params.session_mpool =
>rte_cryptodev_sym_session_pool_create(
>> >> >>  			"CRYPTO_ADAPTER_SESSION_MP",
>> >> >>  			MAX_NB_SESSIONS, 0, 0,
>> >> >> -			sizeof(union rte_event_crypto_metadata),
>> >> >> +			sizeof(struct rte_event_crypto_metadata),
>> >> >>  			SOCKET_ID_ANY);
>> >> >>  	TEST_ASSERT_NOT_NULL(params.session_mpool,
>> >> >>  			"session mempool allocation failed\n"); diff --git
>> >> >> a/doc/guides/rel_notes/deprecation.rst
>> >> >> b/doc/guides/rel_notes/deprecation.rst
>> >> >> index 76a4abfd6b..58ee95c020 100644
>> >> >> --- a/doc/guides/rel_notes/deprecation.rst
>> >> >> +++ b/doc/guides/rel_notes/deprecation.rst
>> >> >> @@ -266,12 +266,6 @@ Deprecation Notices
>> >> >>    values to the function ``rte_event_eth_rx_adapter_queue_add``
>> using
>> >> >>    the structure ``rte_event_eth_rx_adapter_queue_add``.
>> >> >>
>> >> >> -* eventdev: Reserved bytes of ``rte_event_crypto_request`` is a
>> >> >> space holder
>> >> >> -  for ``response_info``. Both should be decoupled for better clarity.
>> >> >> -  New space for ``response_info`` can be made by changing
>> >> >> -  ``rte_event_crypto_metadata`` type to structure from union.
>> >> >> -  This change is targeted for DPDK 21.11.
>> >> >> -
>> >> >>  * metrics: The function ``rte_metrics_init`` will have a non-void return
>> >> >>    in order to notify errors instead of calling ``rte_exit``.
>> >> >>
>> >> >> diff --git a/doc/guides/rel_notes/release_21_11.rst
>> >> >> b/doc/guides/rel_notes/release_21_11.rst
>> >> >> index d707a554ef..ab76d5dd55 100644
>> >> >> --- a/doc/guides/rel_notes/release_21_11.rst
>> >> >> +++ b/doc/guides/rel_notes/release_21_11.rst
>> >> >> @@ -100,6 +100,8 @@ ABI Changes
>> >> >>     Also, make sure to start the actual text at the margin.
>> >> >>
>> >> =======================================================
>> >> >>
>> >> >> +* eventdev: Modified type of ``union rte_event_crypto_metadata``
>> >> >> +to struct and
>> >> >> +  removed reserved bytes from ``struct rte_event_crypto_request``.
>> >> >>
>> >> >>  Known Issues
>> >> >>  ------------
>> >> >> diff --git a/drivers/crypto/octeontx/otx_cryptodev_ops.c
>> >> >> b/drivers/crypto/octeontx/otx_cryptodev_ops.c
>> >> >> index eac6796cfb..c51be63146 100644
>> >> >> --- a/drivers/crypto/octeontx/otx_cryptodev_ops.c
>> >> >> +++ b/drivers/crypto/octeontx/otx_cryptodev_ops.c
>> >> >> @@ -710,17 +710,17 @@ submit_request_to_sso(struct ssows *ws,
>> >> >> uintptr_t req,
>> >> >>  	ssovf_store_pair(add_work, req, ws->grps[rsp_info->queue_id]);
>> >> >> }
>> >> >>
>> >> >> -static inline union rte_event_crypto_metadata *
>> >> >> +static inline struct rte_event_crypto_metadata *
>> >> >>  get_event_crypto_mdata(struct rte_crypto_op *op)  {
>> >> >> -	union rte_event_crypto_metadata *ec_mdata;
>> >> >> +	struct rte_event_crypto_metadata *ec_mdata;
>> >> >>
>> >> >>  	if (op->sess_type == RTE_CRYPTO_OP_WITH_SESSION)
>> >> >>  		ec_mdata = rte_cryptodev_sym_session_get_user_data(
>> >> >>  							   op->sym-
>>session);
>> >> >>  	else if (op->sess_type == RTE_CRYPTO_OP_SESSIONLESS &&
>> >> >>  		 op->private_data_offset)
>> >> >> -		ec_mdata = (union rte_event_crypto_metadata *)
>> >> >> +		ec_mdata = (struct rte_event_crypto_metadata *)
>> >> >>  			((uint8_t *)op + op->private_data_offset);
>> >> >>  	else
>> >> >>  		return NULL;
>> >> >> @@ -731,7 +731,7 @@ get_event_crypto_mdata(struct rte_crypto_op
>> >> *op)
>> >> >> uint16_t __rte_hot  otx_crypto_adapter_enqueue(void *port, struct
>> >> >> rte_crypto_op *op)  {
>> >> >> -	union rte_event_crypto_metadata *ec_mdata;
>> >> >> +	struct rte_event_crypto_metadata *ec_mdata;
>> >> >>  	struct cpt_instance *instance;
>> >> >>  	struct cpt_request_info *req;
>> >> >>  	struct rte_event *rsp_info;
>> >> >> diff --git a/drivers/crypto/octeontx2/otx2_cryptodev_ops.c
>> >> >> b/drivers/crypto/octeontx2/otx2_cryptodev_ops.c
>> >> >> index 42100154cd..952d1352f4 100644
>> >> >> --- a/drivers/crypto/octeontx2/otx2_cryptodev_ops.c
>> >> >> +++ b/drivers/crypto/octeontx2/otx2_cryptodev_ops.c
>> >> >> @@ -453,7 +453,7 @@ otx2_ca_enqueue_req(const struct
>> otx2_cpt_qp
>> >> *qp,
>> >> >>  		    struct rte_crypto_op *op,
>> >> >>  		    uint64_t cpt_inst_w7)
>> >> >>  {
>> >> >> -	union rte_event_crypto_metadata *m_data;
>> >> >> +	struct rte_event_crypto_metadata *m_data;
>> >> >>  	union cpt_inst_s inst;
>> >> >>  	uint64_t lmt_status;
>> >> >>
>> >> >> @@ -468,7 +468,7 @@ otx2_ca_enqueue_req(const struct
>> otx2_cpt_qp
>> >> *qp,
>> >> >>  		}
>> >> >>  	} else if (op->sess_type == RTE_CRYPTO_OP_SESSIONLESS &&
>> >> >>  		   op->private_data_offset) {
>> >> >> -		m_data = (union rte_event_crypto_metadata *)
>> >> >> +		m_data = (struct rte_event_crypto_metadata *)
>> >> >>  			 ((uint8_t *)op +
>> >> >>  			  op->private_data_offset);
>> >> >>  	} else {
>> >> >> diff --git a/drivers/event/octeontx2/otx2_evdev_crypto_adptr_tx.h
>> >> >> b/drivers/event/octeontx2/otx2_evdev_crypto_adptr_tx.h
>> >> >> index ecf7eb9f56..458e8306d7 100644
>> >> >> --- a/drivers/event/octeontx2/otx2_evdev_crypto_adptr_tx.h
>> >> >> +++ b/drivers/event/octeontx2/otx2_evdev_crypto_adptr_tx.h
>> >> >> @@ -16,7 +16,7 @@
>> >> >>  static inline uint16_t
>> >> >>  otx2_ca_enq(uintptr_t tag_op, const struct rte_event *ev)  {
>> >> >> -	union rte_event_crypto_metadata *m_data;
>> >> >> +	struct rte_event_crypto_metadata *m_data;
>> >> >>  	struct rte_crypto_op *crypto_op;
>> >> >>  	struct rte_cryptodev *cdev;
>> >> >>  	struct otx2_cpt_qp *qp;
>> >> >> @@ -37,7 +37,7 @@ otx2_ca_enq(uintptr_t tag_op, const struct
>> >> >> rte_event
>> >> >> *ev)
>> >> >>  		qp_id = m_data->request_info.queue_pair_id;
>> >> >>  	} else if (crypto_op->sess_type == RTE_CRYPTO_OP_SESSIONLESS
>> >> &&
>> >> >>  		   crypto_op->private_data_offset) {
>> >> >> -		m_data = (union rte_event_crypto_metadata *)
>> >> >> +		m_data = (struct rte_event_crypto_metadata *)
>> >> >>  			 ((uint8_t *)crypto_op +
>> >> >>  			  crypto_op->private_data_offset);
>> >> >>  		cdev_id = m_data->request_info.cdev_id; diff --git
>> >> >> a/lib/eventdev/rte_event_crypto_adapter.c
>> >> >> b/lib/eventdev/rte_event_crypto_adapter.c
>> >> >> index e1d38d383d..6977391ae9 100644
>> >> >> --- a/lib/eventdev/rte_event_crypto_adapter.c
>> >> >> +++ b/lib/eventdev/rte_event_crypto_adapter.c
>> >> >> @@ -333,7 +333,7 @@ eca_enq_to_cryptodev(struct
>> >> >> rte_event_crypto_adapter *adapter,
>> >> >>  		 struct rte_event *ev, unsigned int cnt)  {
>> >> >>  	struct rte_event_crypto_adapter_stats *stats = &adapter-
>> >> >> >crypto_stats;
>> >> >> -	union rte_event_crypto_metadata *m_data = NULL;
>> >> >> +	struct rte_event_crypto_metadata *m_data = NULL;
>> >> >>  	struct crypto_queue_pair_info *qp_info = NULL;
>> >> >>  	struct rte_crypto_op *crypto_op;
>> >> >>  	unsigned int i, n;
>> >> >> @@ -371,7 +371,7 @@ eca_enq_to_cryptodev(struct
>> >> >> rte_event_crypto_adapter *adapter,
>> >> >>  			len++;
>> >> >>  		} else if (crypto_op->sess_type ==
>> >> RTE_CRYPTO_OP_SESSIONLESS &&
>> >> >>  				crypto_op->private_data_offset) {
>> >> >> -			m_data = (union rte_event_crypto_metadata *)
>> >> >> +			m_data = (struct rte_event_crypto_metadata *)
>> >> >>  				 ((uint8_t *)crypto_op +
>> >> >>  					crypto_op->private_data_offset);
>> >> >>  			cdev_id = m_data->request_info.cdev_id; @@ -
>> >> >> 504,7 +504,7 @@ eca_ops_enqueue_burst(struct
>> >> rte_event_crypto_adapter
>> >> >> *adapter,
>> >> >>  		  struct rte_crypto_op **ops, uint16_t num)  {
>> >> >>  	struct rte_event_crypto_adapter_stats *stats = &adapter-
>> >> >> >crypto_stats;
>> >> >> -	union rte_event_crypto_metadata *m_data = NULL;
>> >> >> +	struct rte_event_crypto_metadata *m_data = NULL;
>> >> >>  	uint8_t event_dev_id = adapter->eventdev_id;
>> >> >>  	uint8_t event_port_id = adapter->event_port_id;
>> >> >>  	struct rte_event events[BATCH_SIZE]; @@ -523,7 +523,7 @@
>> >> >> eca_ops_enqueue_burst(struct rte_event_crypto_adapter *adapter,
>> >> >>  					ops[i]->sym->session);
>> >> >>  		} else if (ops[i]->sess_type ==
>> RTE_CRYPTO_OP_SESSIONLESS &&
>> >> >>  				ops[i]->private_data_offset) {
>> >> >> -			m_data = (union rte_event_crypto_metadata *)
>> >> >> +			m_data = (struct rte_event_crypto_metadata *)
>> >> >>  				 ((uint8_t *)ops[i] +
>> >> >>  				  ops[i]->private_data_offset);
>> >> >>  		}
>> >> >> diff --git a/lib/eventdev/rte_event_crypto_adapter.h
>> >> >> b/lib/eventdev/rte_event_crypto_adapter.h
>> >> >> index f8c6cca87c..3c24d9d9df 100644
>> >> >> --- a/lib/eventdev/rte_event_crypto_adapter.h
>> >> >> +++ b/lib/eventdev/rte_event_crypto_adapter.h
>> >> >> @@ -200,11 +200,6 @@ enum rte_event_crypto_adapter_mode {
>> >> >>   * provide event request information to the adapter.
>> >> >>   */
>> >> >>  struct rte_event_crypto_request {
>> >> >> -	uint8_t resv[8];
>> >> >> -	/**< Overlaps with first 8 bytes of struct rte_event
>> >> >> -	 * that encode the response event information. Application
>> >> >> -	 * is expected to fill in struct rte_event response_info.
>> >> >> -	 */
>> >> >>  	uint16_t cdev_id;
>> >> >>  	/**< cryptodev ID to be used */
>> >> >>  	uint16_t queue_pair_id;
>> >> >> @@ -223,16 +218,16 @@ struct rte_event_crypto_request {
>> >> >>   * operation. If the transfer is done by SW, event response
>> information
>> >> >>   * will be used by the adapter.
>> >> >>   */
>> >> >> -union rte_event_crypto_metadata {
>> >> >> -	struct rte_event_crypto_request request_info;
>> >> >> -	/**< Request information to be filled in by application
>> >> >> -	 * for RTE_EVENT_CRYPTO_ADAPTER_OP_FORWARD mode.
>> >> >> -	 */
>> >> >> +struct rte_event_crypto_metadata {
>> >> >>  	struct rte_event response_info;
>> >> >>  	/**< Response information to be filled in by application
>> >> >>  	 * for RTE_EVENT_CRYPTO_ADAPTER_OP_NEW and
>> >> >>  	 * RTE_EVENT_CRYPTO_ADAPTER_OP_FORWARD mode.
>> >> >>  	 */
>> >> >> +	struct rte_event_crypto_request request_info;
>> >> >> +	/**< Request information to be filled in by application
>> >> >> +	 * for RTE_EVENT_CRYPTO_ADAPTER_OP_FORWARD mode.
>> >> >> +	 */
>> >> >>  };
>> >> >>
>> >> >>  /**
>> >> >> --
>> >> >> 2.25.1

Re: [dpdk-dev] [PATCH v3] eventdev: update crypto adapter metadata structures

[Anoob Joseph]

Hi Abhinandan, Shijith,

Please see inline.

Thanks,
Anoob

> -----Original Message-----
> From: Shijith Thotton <sthotton@marvell.com>
> Sent: Tuesday, September 14, 2021 10:07 AM
> To: Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>; dev@dpdk.org
> Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Anoob Joseph
> <anoobj@marvell.com>; Pavan Nikhilesh Bhagavatula
> <pbhagavatula@marvell.com>; Akhil Goyal <gakhil@marvell.com>; Ray
> Kinsella <mdr@ashroe.eu>; Ankur Dwivedi <adwivedi@marvell.com>
> Subject: RE: [PATCH v3] eventdev: update crypto adapter metadata
> structures
> 
> >> >>
> >> >> >> In crypto adapter metadata, reserved bytes in request info
> >> >> >> structure is a space holder for response info. It enforces an
> >> >> >> order of operation if the structures are updated using memcpy
> >> >> >> to avoid overwriting response info. It is logical to move the
> >> >> >> reserved space out of request info. It also solves the ordering
> >> >> >> issue
> >> mentioned before.
> >> >> >I would like to understand what kind of ordering issue you have
> >> >> >faced with the current approach. Could you please give an
> >> >> >example/sequence
> >> >> and explain?
> >> >> >
> >> >>
> >> >> I have seen this issue with crypto adapter autotest (#n215).
> >> >>
> >> >> Example:
> >> >> rte_memcpy(&m_data.response_info, &response_info,
> >> >> sizeof(response_info)); rte_memcpy(&m_data.request_info,
> >> >> &request_info, sizeof(request_info));
> >> >>
> >> >> Here response info is getting overwritten by request info.
> >> >> Above lines can reordered to fix the issue, but can be ignored
> >> >> with this
> >> patch.
> >> >There is a reason for designing the metadata in this way.
> >> >Right now, sizeof (union rte_event_crypto_metadata) is 16 bytes.
> >> >So, the session based case needs just 16 bytes to store the data.
> >> >Whereas, for sessionless case each crypto_ops requires another 16
> bytes.
> >> >
> >> >By changing the struct in the following way you are doubling the
> >> >memory requirement.
> >> >With the changes, for sessionless case, each crypto op requires 32
> >> >bytes of space instead of 16 bytes and the mempool will be bigger.
> >> >This will have the perf impact too!
> >> >
> >> >You can just copy the individual members(cdev_id & queue_pair_id)
> >> >after the response_info.
> >> >OR You have a better way?
> >> >
> >>
> >> I missed the second word of event structure. It adds an extra 8
> >> bytes, which is not required.
> >I guess you meant not adding, it is overlapping with request information.
> >> Let me know, what you think of below change to metadata structure.
> >>
> >> struct rte_event_crypto_metadata {
> >> 	uint64_t response_info; // 8 bytes
> >With this, it lags clarity saying it is an event information.
> >> 	struct rte_event_crypto_request request_info; // 8 bytes };
> >>
> >> Total structure size is 16 bytes.
> >>
> >> Response info can be set like below in test app:
> >> 	m_data.response_info = response_info.event;
> >>
> >> The main aim of this patch is to decouple response info from request info.
> >The decoupling was required as it was doing memcpy.
> >If you copy the individual members of request info(after response), you
> >don't require it.
> 
> With this change, the application will be free of such constraints.

[Anoob] Why don't we make it like,

struct rte_event_crypto_metadata {
 	uint64_t ev_w0_template;
              /**< Template of the first word of ``struct rte_event`` (rte_event.event) to be set in the events generated by crypto adapter in both RTE_EVENT_CRYPTO_ADAPTER_OP_NEW and
	 * RTE_EVENT_CRYPTO_ADAPTER_OP_FORWARD modes.
	*/
 	struct rte_event_crypto_request request_info;
};
 
This way, the usage would become more obvious and we will not have additional space reserved as well.

> 
> >>
> >> >>
> >> >> >>
> >> >> >> This patch removes the reserve field from request info and
> >> >> >> makes event crypto metadata type to structure from union to
> >> >> >> make space for response info.
> >> >> >>
> >> >> >> App and drivers are updated as per metadata change.
> >> >> >>
> >> >> >> Signed-off-by: Shijith Thotton <sthotton@marvell.com>
> >> >> >> Acked-by: Anoob Joseph <anoobj@marvell.com>
> >> >> >> ---
> >> >> >> v3:
> >> >> >> * Updated ABI section of release notes.
> >> >> >>
> >> >> >> v2:
> >> >> >> * Updated deprecation notice.
> >> >> >>
> >> >> >> v1:
> >> >> >> * Rebased.
> >> >> >>
> >> >> >>  app/test/test_event_crypto_adapter.c              | 14 +++++++-------
> >> >> >>  doc/guides/rel_notes/deprecation.rst              |  6 ------
> >> >> >>  doc/guides/rel_notes/release_21_11.rst            |  2 ++
> >> >> >>  drivers/crypto/octeontx/otx_cryptodev_ops.c       |  8 ++++----
> >> >> >>  drivers/crypto/octeontx2/otx2_cryptodev_ops.c     |  4 ++--
> >> >> >>  .../event/octeontx2/otx2_evdev_crypto_adptr_tx.h  |  4 ++--
> >> >> >>  lib/eventdev/rte_event_crypto_adapter.c           |  8 ++++----
> >> >> >>  lib/eventdev/rte_event_crypto_adapter.h           | 15 +++++---------
> -
> >> >> >>  8 files changed, 26 insertions(+), 35 deletions(-)
> >> >> >>
> >> >> >> diff --git a/app/test/test_event_crypto_adapter.c
> >> >> >> b/app/test/test_event_crypto_adapter.c
> >> >> >> index 3ad20921e2..0d73694d3a 100644
> >> >> >> --- a/app/test/test_event_crypto_adapter.c
> >> >> >> +++ b/app/test/test_event_crypto_adapter.c
> >> >> >> @@ -168,7 +168,7 @@ test_op_forward_mode(uint8_t
> session_less)
> >> {
> >> >> >>  	struct rte_crypto_sym_xform cipher_xform;
> >> >> >>  	struct rte_cryptodev_sym_session *sess;
> >> >> >> -	union rte_event_crypto_metadata m_data;
> >> >> >> +	struct rte_event_crypto_metadata m_data;
> >> >> >>  	struct rte_crypto_sym_op *sym_op;
> >> >> >>  	struct rte_crypto_op *op;
> >> >> >>  	struct rte_mbuf *m;
> >> >> >> @@ -368,7 +368,7 @@ test_op_new_mode(uint8_t session_less)  {
> >> >> >>  	struct rte_crypto_sym_xform cipher_xform;
> >> >> >>  	struct rte_cryptodev_sym_session *sess;
> >> >> >> -	union rte_event_crypto_metadata m_data;
> >> >> >> +	struct rte_event_crypto_metadata m_data;
> >> >> >>  	struct rte_crypto_sym_op *sym_op;
> >> >> >>  	struct rte_crypto_op *op;
> >> >> >>  	struct rte_mbuf *m;
> >> >> >> @@ -406,7 +406,7 @@ test_op_new_mode(uint8_t session_less)
> >> >> >>  		if (cap &
> >> >> >> RTE_EVENT_CRYPTO_ADAPTER_CAP_SESSION_PRIVATE_DATA) {
> >> >> >>  			/* Fill in private user data information */
> >> >> >>  			rte_memcpy(&m_data.response_info,
> >> >> &response_info,
> >> >> >> -				   sizeof(m_data));
> >> >> >> +				   sizeof(response_info));
> >> >> >>
> 	rte_cryptodev_sym_session_set_user_data(sess,
> >> >> >>  						&m_data,
> sizeof(m_data));
> >> >> >>  		}
> >> >> >> @@ -426,7 +426,7 @@ test_op_new_mode(uint8_t session_less)
> >> >> >>  		op->private_data_offset = len;
> >> >> >>  		/* Fill in private data information */
> >> >> >>  		rte_memcpy(&m_data.response_info,
> &response_info,
> >> >> >> -			   sizeof(m_data));
> >> >> >> +			   sizeof(response_info));
> >> >> >>  		rte_memcpy((uint8_t *)op + len, &m_data,
> >sizeof(m_data));
> >> >> >>  	}
> >> >> >>
> >> >> >> @@ -519,7 +519,7 @@ configure_cryptodev(void)
> >> >> >>  			DEFAULT_NUM_XFORMS *
> >> >> >>  			sizeof(struct rte_crypto_sym_xform) +
> >> >> >>  			MAXIMUM_IV_LENGTH +
> >> >> >> -			sizeof(union rte_event_crypto_metadata),
> >> >> >> +			sizeof(struct rte_event_crypto_metadata),
> >> >> >>  			rte_socket_id());
> >> >> >>  	if (params.op_mpool == NULL) {
> >> >> >>  		RTE_LOG(ERR, USER1, "Can't create
> >CRYPTO_OP_POOL\n");
> >> >> @@ -549,12
> >> >> >> +549,12 @@ configure_cryptodev(void)
> >> >> >>  	 * to include the session headers & private data
> >> >> >>  	 */
> >> >> >>  	session_size =
> >> >> >> rte_cryptodev_sym_get_private_session_size(TEST_CDEV_ID);
> >> >> >> -	session_size += sizeof(union rte_event_crypto_metadata);
> >> >> >> +	session_size += sizeof(struct rte_event_crypto_metadata);
> >> >> >>
> >> >> >>  	params.session_mpool =
> >rte_cryptodev_sym_session_pool_create(
> >> >> >>  			"CRYPTO_ADAPTER_SESSION_MP",
> >> >> >>  			MAX_NB_SESSIONS, 0, 0,
> >> >> >> -			sizeof(union rte_event_crypto_metadata),
> >> >> >> +			sizeof(struct rte_event_crypto_metadata),
> >> >> >>  			SOCKET_ID_ANY);
> >> >> >>  	TEST_ASSERT_NOT_NULL(params.session_mpool,
> >> >> >>  			"session mempool allocation failed\n"); diff --
> git
> >> >> >> a/doc/guides/rel_notes/deprecation.rst
> >> >> >> b/doc/guides/rel_notes/deprecation.rst
> >> >> >> index 76a4abfd6b..58ee95c020 100644
> >> >> >> --- a/doc/guides/rel_notes/deprecation.rst
> >> >> >> +++ b/doc/guides/rel_notes/deprecation.rst
> >> >> >> @@ -266,12 +266,6 @@ Deprecation Notices
> >> >> >>    values to the function
> >> >> >> ``rte_event_eth_rx_adapter_queue_add``
> >> using
> >> >> >>    the structure ``rte_event_eth_rx_adapter_queue_add``.
> >> >> >>
> >> >> >> -* eventdev: Reserved bytes of ``rte_event_crypto_request`` is
> >> >> >> a space holder
> >> >> >> -  for ``response_info``. Both should be decoupled for better clarity.
> >> >> >> -  New space for ``response_info`` can be made by changing
> >> >> >> -  ``rte_event_crypto_metadata`` type to structure from union.
> >> >> >> -  This change is targeted for DPDK 21.11.
> >> >> >> -
> >> >> >>  * metrics: The function ``rte_metrics_init`` will have a non-void
> return
> >> >> >>    in order to notify errors instead of calling ``rte_exit``.
> >> >> >>
> >> >> >> diff --git a/doc/guides/rel_notes/release_21_11.rst
> >> >> >> b/doc/guides/rel_notes/release_21_11.rst
> >> >> >> index d707a554ef..ab76d5dd55 100644
> >> >> >> --- a/doc/guides/rel_notes/release_21_11.rst
> >> >> >> +++ b/doc/guides/rel_notes/release_21_11.rst
> >> >> >> @@ -100,6 +100,8 @@ ABI Changes
> >> >> >>     Also, make sure to start the actual text at the margin.
> >> >> >>
> >> >>
> =======================================================
> >> >> >>
> >> >> >> +* eventdev: Modified type of ``union
> >> >> >> +rte_event_crypto_metadata`` to struct and
> >> >> >> +  removed reserved bytes from ``struct
> rte_event_crypto_request``.
> >> >> >>
> >> >> >>  Known Issues
> >> >> >>  ------------
> >> >> >> diff --git a/drivers/crypto/octeontx/otx_cryptodev_ops.c
> >> >> >> b/drivers/crypto/octeontx/otx_cryptodev_ops.c
> >> >> >> index eac6796cfb..c51be63146 100644
> >> >> >> --- a/drivers/crypto/octeontx/otx_cryptodev_ops.c
> >> >> >> +++ b/drivers/crypto/octeontx/otx_cryptodev_ops.c
> >> >> >> @@ -710,17 +710,17 @@ submit_request_to_sso(struct ssows
> *ws,
> >> >> >> uintptr_t req,
> >> >> >>  	ssovf_store_pair(add_work, req,
> >> >> >> ws->grps[rsp_info->queue_id]); }
> >> >> >>
> >> >> >> -static inline union rte_event_crypto_metadata *
> >> >> >> +static inline struct rte_event_crypto_metadata *
> >> >> >>  get_event_crypto_mdata(struct rte_crypto_op *op)  {
> >> >> >> -	union rte_event_crypto_metadata *ec_mdata;
> >> >> >> +	struct rte_event_crypto_metadata *ec_mdata;
> >> >> >>
> >> >> >>  	if (op->sess_type == RTE_CRYPTO_OP_WITH_SESSION)
> >> >> >>  		ec_mdata =
> rte_cryptodev_sym_session_get_user_data(
> >> >> >>  							   op->sym-
> >>session);
> >> >> >>  	else if (op->sess_type == RTE_CRYPTO_OP_SESSIONLESS &&
> >> >> >>  		 op->private_data_offset)
> >> >> >> -		ec_mdata = (union rte_event_crypto_metadata *)
> >> >> >> +		ec_mdata = (struct rte_event_crypto_metadata *)
> >> >> >>  			((uint8_t *)op + op->private_data_offset);
> >> >> >>  	else
> >> >> >>  		return NULL;
> >> >> >> @@ -731,7 +731,7 @@ get_event_crypto_mdata(struct
> rte_crypto_op
> >> >> *op)
> >> >> >> uint16_t __rte_hot  otx_crypto_adapter_enqueue(void *port,
> >> >> >> struct rte_crypto_op *op)  {
> >> >> >> -	union rte_event_crypto_metadata *ec_mdata;
> >> >> >> +	struct rte_event_crypto_metadata *ec_mdata;
> >> >> >>  	struct cpt_instance *instance;
> >> >> >>  	struct cpt_request_info *req;
> >> >> >>  	struct rte_event *rsp_info;
> >> >> >> diff --git a/drivers/crypto/octeontx2/otx2_cryptodev_ops.c
> >> >> >> b/drivers/crypto/octeontx2/otx2_cryptodev_ops.c
> >> >> >> index 42100154cd..952d1352f4 100644
> >> >> >> --- a/drivers/crypto/octeontx2/otx2_cryptodev_ops.c
> >> >> >> +++ b/drivers/crypto/octeontx2/otx2_cryptodev_ops.c
> >> >> >> @@ -453,7 +453,7 @@ otx2_ca_enqueue_req(const struct
> >> otx2_cpt_qp
> >> >> *qp,
> >> >> >>  		    struct rte_crypto_op *op,
> >> >> >>  		    uint64_t cpt_inst_w7)
> >> >> >>  {
> >> >> >> -	union rte_event_crypto_metadata *m_data;
> >> >> >> +	struct rte_event_crypto_metadata *m_data;
> >> >> >>  	union cpt_inst_s inst;
> >> >> >>  	uint64_t lmt_status;
> >> >> >>
> >> >> >> @@ -468,7 +468,7 @@ otx2_ca_enqueue_req(const struct
> >> otx2_cpt_qp
> >> >> *qp,
> >> >> >>  		}
> >> >> >>  	} else if (op->sess_type == RTE_CRYPTO_OP_SESSIONLESS
> &&
> >> >> >>  		   op->private_data_offset) {
> >> >> >> -		m_data = (union rte_event_crypto_metadata *)
> >> >> >> +		m_data = (struct rte_event_crypto_metadata *)
> >> >> >>  			 ((uint8_t *)op +
> >> >> >>  			  op->private_data_offset);
> >> >> >>  	} else {
> >> >> >> diff --git
> >> >> >> a/drivers/event/octeontx2/otx2_evdev_crypto_adptr_tx.h
> >> >> >> b/drivers/event/octeontx2/otx2_evdev_crypto_adptr_tx.h
> >> >> >> index ecf7eb9f56..458e8306d7 100644
> >> >> >> --- a/drivers/event/octeontx2/otx2_evdev_crypto_adptr_tx.h
> >> >> >> +++ b/drivers/event/octeontx2/otx2_evdev_crypto_adptr_tx.h
> >> >> >> @@ -16,7 +16,7 @@
> >> >> >>  static inline uint16_t
> >> >> >>  otx2_ca_enq(uintptr_t tag_op, const struct rte_event *ev)  {
> >> >> >> -	union rte_event_crypto_metadata *m_data;
> >> >> >> +	struct rte_event_crypto_metadata *m_data;
> >> >> >>  	struct rte_crypto_op *crypto_op;
> >> >> >>  	struct rte_cryptodev *cdev;
> >> >> >>  	struct otx2_cpt_qp *qp;
> >> >> >> @@ -37,7 +37,7 @@ otx2_ca_enq(uintptr_t tag_op, const struct
> >> >> >> rte_event
> >> >> >> *ev)
> >> >> >>  		qp_id = m_data->request_info.queue_pair_id;
> >> >> >>  	} else if (crypto_op->sess_type ==
> RTE_CRYPTO_OP_SESSIONLESS
> >> >> &&
> >> >> >>  		   crypto_op->private_data_offset) {
> >> >> >> -		m_data = (union rte_event_crypto_metadata *)
> >> >> >> +		m_data = (struct rte_event_crypto_metadata *)
> >> >> >>  			 ((uint8_t *)crypto_op +
> >> >> >>  			  crypto_op->private_data_offset);
> >> >> >>  		cdev_id = m_data->request_info.cdev_id; diff --git
> >> >> >> a/lib/eventdev/rte_event_crypto_adapter.c
> >> >> >> b/lib/eventdev/rte_event_crypto_adapter.c
> >> >> >> index e1d38d383d..6977391ae9 100644
> >> >> >> --- a/lib/eventdev/rte_event_crypto_adapter.c
> >> >> >> +++ b/lib/eventdev/rte_event_crypto_adapter.c
> >> >> >> @@ -333,7 +333,7 @@ eca_enq_to_cryptodev(struct
> >> >> >> rte_event_crypto_adapter *adapter,
> >> >> >>  		 struct rte_event *ev, unsigned int cnt)  {
> >> >> >>  	struct rte_event_crypto_adapter_stats *stats = &adapter-
> >> >> >> >crypto_stats;
> >> >> >> -	union rte_event_crypto_metadata *m_data = NULL;
> >> >> >> +	struct rte_event_crypto_metadata *m_data = NULL;
> >> >> >>  	struct crypto_queue_pair_info *qp_info = NULL;
> >> >> >>  	struct rte_crypto_op *crypto_op;
> >> >> >>  	unsigned int i, n;
> >> >> >> @@ -371,7 +371,7 @@ eca_enq_to_cryptodev(struct
> >> >> >> rte_event_crypto_adapter *adapter,
> >> >> >>  			len++;
> >> >> >>  		} else if (crypto_op->sess_type ==
> >> >> RTE_CRYPTO_OP_SESSIONLESS &&
> >> >> >>  				crypto_op->private_data_offset) {
> >> >> >> -			m_data = (union
> rte_event_crypto_metadata *)
> >> >> >> +			m_data = (struct
> rte_event_crypto_metadata *)
> >> >> >>  				 ((uint8_t *)crypto_op +
> >> >> >>  					crypto_op-
> >private_data_offset);
> >> >> >>  			cdev_id = m_data->request_info.cdev_id;
> @@ -
> >> >> >> 504,7 +504,7 @@ eca_ops_enqueue_burst(struct
> >> >> rte_event_crypto_adapter
> >> >> >> *adapter,
> >> >> >>  		  struct rte_crypto_op **ops, uint16_t num)  {
> >> >> >>  	struct rte_event_crypto_adapter_stats *stats = &adapter-
> >> >> >> >crypto_stats;
> >> >> >> -	union rte_event_crypto_metadata *m_data = NULL;
> >> >> >> +	struct rte_event_crypto_metadata *m_data = NULL;
> >> >> >>  	uint8_t event_dev_id = adapter->eventdev_id;
> >> >> >>  	uint8_t event_port_id = adapter->event_port_id;
> >> >> >>  	struct rte_event events[BATCH_SIZE]; @@ -523,7 +523,7 @@
> >> >> >> eca_ops_enqueue_burst(struct rte_event_crypto_adapter
> *adapter,
> >> >> >>  					ops[i]->sym->session);
> >> >> >>  		} else if (ops[i]->sess_type ==
> >> RTE_CRYPTO_OP_SESSIONLESS &&
> >> >> >>  				ops[i]->private_data_offset) {
> >> >> >> -			m_data = (union
> rte_event_crypto_metadata *)
> >> >> >> +			m_data = (struct
> rte_event_crypto_metadata *)
> >> >> >>  				 ((uint8_t *)ops[i] +
> >> >> >>  				  ops[i]->private_data_offset);
> >> >> >>  		}
> >> >> >> diff --git a/lib/eventdev/rte_event_crypto_adapter.h
> >> >> >> b/lib/eventdev/rte_event_crypto_adapter.h
> >> >> >> index f8c6cca87c..3c24d9d9df 100644
> >> >> >> --- a/lib/eventdev/rte_event_crypto_adapter.h
> >> >> >> +++ b/lib/eventdev/rte_event_crypto_adapter.h
> >> >> >> @@ -200,11 +200,6 @@ enum rte_event_crypto_adapter_mode {
> >> >> >>   * provide event request information to the adapter.
> >> >> >>   */
> >> >> >>  struct rte_event_crypto_request {
> >> >> >> -	uint8_t resv[8];
> >> >> >> -	/**< Overlaps with first 8 bytes of struct rte_event
> >> >> >> -	 * that encode the response event information. Application
> >> >> >> -	 * is expected to fill in struct rte_event response_info.
> >> >> >> -	 */
> >> >> >>  	uint16_t cdev_id;
> >> >> >>  	/**< cryptodev ID to be used */
> >> >> >>  	uint16_t queue_pair_id;
> >> >> >> @@ -223,16 +218,16 @@ struct rte_event_crypto_request {
> >> >> >>   * operation. If the transfer is done by SW, event response
> >> information
> >> >> >>   * will be used by the adapter.
> >> >> >>   */
> >> >> >> -union rte_event_crypto_metadata {
> >> >> >> -	struct rte_event_crypto_request request_info;
> >> >> >> -	/**< Request information to be filled in by application
> >> >> >> -	 * for RTE_EVENT_CRYPTO_ADAPTER_OP_FORWARD mode.
> >> >> >> -	 */
> >> >> >> +struct rte_event_crypto_metadata {
> >> >> >>  	struct rte_event response_info;
> >> >> >>  	/**< Response information to be filled in by application
> >> >> >>  	 * for RTE_EVENT_CRYPTO_ADAPTER_OP_NEW and
> >> >> >>  	 * RTE_EVENT_CRYPTO_ADAPTER_OP_FORWARD mode.
> >> >> >>  	 */
> >> >> >> +	struct rte_event_crypto_request request_info;
> >> >> >> +	/**< Request information to be filled in by application
> >> >> >> +	 * for RTE_EVENT_CRYPTO_ADAPTER_OP_FORWARD mode.
> >> >> >> +	 */
> >> >> >>  };
> >> >> >>
> >> >> >>  /**
> >> >> >> --
> >> >> >> 2.25.1

Re: [dpdk-dev] [PATCH v3] eventdev: update crypto adapter metadata structures

[Gujjar, Abhinandan S]

Hi Shijith & Anoob,

> -----Original Message-----
> From: Anoob Joseph <anoobj@marvell.com>
> Sent: Tuesday, September 14, 2021 10:16 AM
> To: Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>
> Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Pavan Nikhilesh
> Bhagavatula <pbhagavatula@marvell.com>; Akhil Goyal
> <gakhil@marvell.com>; Ray Kinsella <mdr@ashroe.eu>; Ankur Dwivedi
> <adwivedi@marvell.com>; Shijith Thotton <sthotton@marvell.com>;
> dev@dpdk.org
> Subject: RE: [PATCH v3] eventdev: update crypto adapter metadata
> structures
> 
> Hi Abhinandan, Shijith,
> 
> Please see inline.
> 
> Thanks,
> Anoob
> 
> > -----Original Message-----
> > From: Shijith Thotton <sthotton@marvell.com>
> > Sent: Tuesday, September 14, 2021 10:07 AM
> > To: Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>; dev@dpdk.org
> > Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Anoob Joseph
> > <anoobj@marvell.com>; Pavan Nikhilesh Bhagavatula
> > <pbhagavatula@marvell.com>; Akhil Goyal <gakhil@marvell.com>; Ray
> > Kinsella <mdr@ashroe.eu>; Ankur Dwivedi <adwivedi@marvell.com>
> > Subject: RE: [PATCH v3] eventdev: update crypto adapter metadata
> > structures
> >
> > >> >>
> > >> >> >> In crypto adapter metadata, reserved bytes in request info
> > >> >> >> structure is a space holder for response info. It enforces an
> > >> >> >> order of operation if the structures are updated using memcpy
> > >> >> >> to avoid overwriting response info. It is logical to move the
> > >> >> >> reserved space out of request info. It also solves the
> > >> >> >> ordering issue
> > >> mentioned before.
> > >> >> >I would like to understand what kind of ordering issue you have
> > >> >> >faced with the current approach. Could you please give an
> > >> >> >example/sequence
> > >> >> and explain?
> > >> >> >
> > >> >>
> > >> >> I have seen this issue with crypto adapter autotest (#n215).
> > >> >>
> > >> >> Example:
> > >> >> rte_memcpy(&m_data.response_info, &response_info,
> > >> >> sizeof(response_info)); rte_memcpy(&m_data.request_info,
> > >> >> &request_info, sizeof(request_info));
> > >> >>
> > >> >> Here response info is getting overwritten by request info.
> > >> >> Above lines can reordered to fix the issue, but can be ignored
> > >> >> with this
> > >> patch.
> > >> >There is a reason for designing the metadata in this way.
> > >> >Right now, sizeof (union rte_event_crypto_metadata) is 16 bytes.
> > >> >So, the session based case needs just 16 bytes to store the data.
> > >> >Whereas, for sessionless case each crypto_ops requires another 16
> > bytes.
> > >> >
> > >> >By changing the struct in the following way you are doubling the
> > >> >memory requirement.
> > >> >With the changes, for sessionless case, each crypto op requires 32
> > >> >bytes of space instead of 16 bytes and the mempool will be bigger.
> > >> >This will have the perf impact too!
> > >> >
> > >> >You can just copy the individual members(cdev_id & queue_pair_id)
> > >> >after the response_info.
> > >> >OR You have a better way?
> > >> >
> > >>
> > >> I missed the second word of event structure. It adds an extra 8
> > >> bytes, which is not required.
> > >I guess you meant not adding, it is overlapping with request information.
> > >> Let me know, what you think of below change to metadata structure.
> > >>
> > >> struct rte_event_crypto_metadata {
> > >> 	uint64_t response_info; // 8 bytes
> > >With this, it lags clarity saying it is an event information.
> > >> 	struct rte_event_crypto_request request_info; // 8 bytes };
> > >>
> > >> Total structure size is 16 bytes.
> > >>
> > >> Response info can be set like below in test app:
> > >> 	m_data.response_info = response_info.event;
> > >>
> > >> The main aim of this patch is to decouple response info from request
> info.
> > >The decoupling was required as it was doing memcpy.
> > >If you copy the individual members of request info(after response),
> > >you don't require it.
> >
> > With this change, the application will be free of such constraints.
> 
> [Anoob] Why don't we make it like,
> 
> struct rte_event_crypto_metadata {
>  	uint64_t ev_w0_template;
>               /**< Template of the first word of ``struct rte_event``
> (rte_event.event) to be set in the events generated by crypto adapter in
> both RTE_EVENT_CRYPTO_ADAPTER_OP_NEW and
> 	 * RTE_EVENT_CRYPTO_ADAPTER_OP_FORWARD modes.
> 	*/
>  	struct rte_event_crypto_request request_info; };
> 
> This way, the usage would become more obvious and we will not have
> additional space reserved as well.

Thanks for the suggestion. At this point, it is like an application without understanding the data structure/ internals
has used memcpy and we are trying to fix it by changing the actual data structure instead of fixing the application!
With this patch, the other applications(outside of dpdk) which are using the data structures in a right are forced to change!
I don't think, this is the right way to handle this. I think, we should be updating the test case and documentation for this rather
than changing the data structure.  I expect the next version of this patch should have those changes. Thanks!

> 
> >
> > >>
> > >> >>
> > >> >> >>
> > >> >> >> This patch removes the reserve field from request info and
> > >> >> >> makes event crypto metadata type to structure from union to
> > >> >> >> make space for response info.
> > >> >> >>
> > >> >> >> App and drivers are updated as per metadata change.
> > >> >> >>
> > >> >> >> Signed-off-by: Shijith Thotton <sthotton@marvell.com>
> > >> >> >> Acked-by: Anoob Joseph <anoobj@marvell.com>
> > >> >> >> ---
> > >> >> >> v3:
> > >> >> >> * Updated ABI section of release notes.
> > >> >> >>
> > >> >> >> v2:
> > >> >> >> * Updated deprecation notice.
> > >> >> >>
> > >> >> >> v1:
> > >> >> >> * Rebased.
> > >> >> >>
> > >> >> >>  app/test/test_event_crypto_adapter.c              | 14 +++++++------
> -
> > >> >> >>  doc/guides/rel_notes/deprecation.rst              |  6 ------
> > >> >> >>  doc/guides/rel_notes/release_21_11.rst            |  2 ++
> > >> >> >>  drivers/crypto/octeontx/otx_cryptodev_ops.c       |  8 ++++----
> > >> >> >>  drivers/crypto/octeontx2/otx2_cryptodev_ops.c     |  4 ++--
> > >> >> >>  .../event/octeontx2/otx2_evdev_crypto_adptr_tx.h  |  4 ++--
> > >> >> >>  lib/eventdev/rte_event_crypto_adapter.c           |  8 ++++----
> > >> >> >>  lib/eventdev/rte_event_crypto_adapter.h           | 15 +++++-------
> --
> > -
> > >> >> >>  8 files changed, 26 insertions(+), 35 deletions(-)
> > >> >> >>
> > >> >> >> diff --git a/app/test/test_event_crypto_adapter.c
> > >> >> >> b/app/test/test_event_crypto_adapter.c
> > >> >> >> index 3ad20921e2..0d73694d3a 100644
> > >> >> >> --- a/app/test/test_event_crypto_adapter.c
> > >> >> >> +++ b/app/test/test_event_crypto_adapter.c
> > >> >> >> @@ -168,7 +168,7 @@ test_op_forward_mode(uint8_t
> > session_less)
> > >> {
> > >> >> >>  	struct rte_crypto_sym_xform cipher_xform;
> > >> >> >>  	struct rte_cryptodev_sym_session *sess;
> > >> >> >> -	union rte_event_crypto_metadata m_data;
> > >> >> >> +	struct rte_event_crypto_metadata m_data;
> > >> >> >>  	struct rte_crypto_sym_op *sym_op;
> > >> >> >>  	struct rte_crypto_op *op;
> > >> >> >>  	struct rte_mbuf *m;
> > >> >> >> @@ -368,7 +368,7 @@ test_op_new_mode(uint8_t session_less)
> {
> > >> >> >>  	struct rte_crypto_sym_xform cipher_xform;
> > >> >> >>  	struct rte_cryptodev_sym_session *sess;
> > >> >> >> -	union rte_event_crypto_metadata m_data;
> > >> >> >> +	struct rte_event_crypto_metadata m_data;
> > >> >> >>  	struct rte_crypto_sym_op *sym_op;
> > >> >> >>  	struct rte_crypto_op *op;
> > >> >> >>  	struct rte_mbuf *m;
> > >> >> >> @@ -406,7 +406,7 @@ test_op_new_mode(uint8_t session_less)
> > >> >> >>  		if (cap &
> > >> >> >> RTE_EVENT_CRYPTO_ADAPTER_CAP_SESSION_PRIVATE_DATA) {
> > >> >> >>  			/* Fill in private user data information */
> > >> >> >>  			rte_memcpy(&m_data.response_info,
> > >> >> &response_info,
> > >> >> >> -				   sizeof(m_data));
> > >> >> >> +				   sizeof(response_info));
> > >> >> >>
> > 	rte_cryptodev_sym_session_set_user_data(sess,
> > >> >> >>  						&m_data,
> > sizeof(m_data));
> > >> >> >>  		}
> > >> >> >> @@ -426,7 +426,7 @@ test_op_new_mode(uint8_t session_less)
> > >> >> >>  		op->private_data_offset = len;
> > >> >> >>  		/* Fill in private data information */
> > >> >> >>  		rte_memcpy(&m_data.response_info,
> > &response_info,
> > >> >> >> -			   sizeof(m_data));
> > >> >> >> +			   sizeof(response_info));
> > >> >> >>  		rte_memcpy((uint8_t *)op + len, &m_data,
> > >sizeof(m_data));
> > >> >> >>  	}
> > >> >> >>
> > >> >> >> @@ -519,7 +519,7 @@ configure_cryptodev(void)
> > >> >> >>  			DEFAULT_NUM_XFORMS *
> > >> >> >>  			sizeof(struct rte_crypto_sym_xform) +
> > >> >> >>  			MAXIMUM_IV_LENGTH +
> > >> >> >> -			sizeof(union rte_event_crypto_metadata),
> > >> >> >> +			sizeof(struct rte_event_crypto_metadata),
> > >> >> >>  			rte_socket_id());
> > >> >> >>  	if (params.op_mpool == NULL) {
> > >> >> >>  		RTE_LOG(ERR, USER1, "Can't create
> > >CRYPTO_OP_POOL\n");
> > >> >> @@ -549,12
> > >> >> >> +549,12 @@ configure_cryptodev(void)
> > >> >> >>  	 * to include the session headers & private data
> > >> >> >>  	 */
> > >> >> >>  	session_size =
> > >> >> >> rte_cryptodev_sym_get_private_session_size(TEST_CDEV_ID);
> > >> >> >> -	session_size += sizeof(union rte_event_crypto_metadata);
> > >> >> >> +	session_size += sizeof(struct rte_event_crypto_metadata);
> > >> >> >>
> > >> >> >>  	params.session_mpool =
> > >rte_cryptodev_sym_session_pool_create(
> > >> >> >>  			"CRYPTO_ADAPTER_SESSION_MP",
> > >> >> >>  			MAX_NB_SESSIONS, 0, 0,
> > >> >> >> -			sizeof(union rte_event_crypto_metadata),
> > >> >> >> +			sizeof(struct rte_event_crypto_metadata),
> > >> >> >>  			SOCKET_ID_ANY);
> > >> >> >>  	TEST_ASSERT_NOT_NULL(params.session_mpool,
> > >> >> >>  			"session mempool allocation failed\n"); diff --
> > git
> > >> >> >> a/doc/guides/rel_notes/deprecation.rst
> > >> >> >> b/doc/guides/rel_notes/deprecation.rst
> > >> >> >> index 76a4abfd6b..58ee95c020 100644
> > >> >> >> --- a/doc/guides/rel_notes/deprecation.rst
> > >> >> >> +++ b/doc/guides/rel_notes/deprecation.rst
> > >> >> >> @@ -266,12 +266,6 @@ Deprecation Notices
> > >> >> >>    values to the function
> > >> >> >> ``rte_event_eth_rx_adapter_queue_add``
> > >> using
> > >> >> >>    the structure ``rte_event_eth_rx_adapter_queue_add``.
> > >> >> >>
> > >> >> >> -* eventdev: Reserved bytes of ``rte_event_crypto_request``
> > >> >> >> is a space holder
> > >> >> >> -  for ``response_info``. Both should be decoupled for better
> clarity.
> > >> >> >> -  New space for ``response_info`` can be made by changing
> > >> >> >> -  ``rte_event_crypto_metadata`` type to structure from union.
> > >> >> >> -  This change is targeted for DPDK 21.11.
> > >> >> >> -
> > >> >> >>  * metrics: The function ``rte_metrics_init`` will have a
> > >> >> >> non-void
> > return
> > >> >> >>    in order to notify errors instead of calling ``rte_exit``.
> > >> >> >>
> > >> >> >> diff --git a/doc/guides/rel_notes/release_21_11.rst
> > >> >> >> b/doc/guides/rel_notes/release_21_11.rst
> > >> >> >> index d707a554ef..ab76d5dd55 100644
> > >> >> >> --- a/doc/guides/rel_notes/release_21_11.rst
> > >> >> >> +++ b/doc/guides/rel_notes/release_21_11.rst
> > >> >> >> @@ -100,6 +100,8 @@ ABI Changes
> > >> >> >>     Also, make sure to start the actual text at the margin.
> > >> >> >>
> > >> >>
> > =======================================================
> > >> >> >>
> > >> >> >> +* eventdev: Modified type of ``union
> > >> >> >> +rte_event_crypto_metadata`` to struct and
> > >> >> >> +  removed reserved bytes from ``struct
> > rte_event_crypto_request``.
> > >> >> >>
> > >> >> >>  Known Issues
> > >> >> >>  ------------
> > >> >> >> diff --git a/drivers/crypto/octeontx/otx_cryptodev_ops.c
> > >> >> >> b/drivers/crypto/octeontx/otx_cryptodev_ops.c
> > >> >> >> index eac6796cfb..c51be63146 100644
> > >> >> >> --- a/drivers/crypto/octeontx/otx_cryptodev_ops.c
> > >> >> >> +++ b/drivers/crypto/octeontx/otx_cryptodev_ops.c
> > >> >> >> @@ -710,17 +710,17 @@ submit_request_to_sso(struct ssows
> > *ws,
> > >> >> >> uintptr_t req,
> > >> >> >>  	ssovf_store_pair(add_work, req,
> > >> >> >> ws->grps[rsp_info->queue_id]); }
> > >> >> >>
> > >> >> >> -static inline union rte_event_crypto_metadata *
> > >> >> >> +static inline struct rte_event_crypto_metadata *
> > >> >> >>  get_event_crypto_mdata(struct rte_crypto_op *op)  {
> > >> >> >> -	union rte_event_crypto_metadata *ec_mdata;
> > >> >> >> +	struct rte_event_crypto_metadata *ec_mdata;
> > >> >> >>
> > >> >> >>  	if (op->sess_type == RTE_CRYPTO_OP_WITH_SESSION)
> > >> >> >>  		ec_mdata =
> > rte_cryptodev_sym_session_get_user_data(
> > >> >> >>  							   op->sym-
> > >>session);
> > >> >> >>  	else if (op->sess_type == RTE_CRYPTO_OP_SESSIONLESS &&
> > >> >> >>  		 op->private_data_offset)
> > >> >> >> -		ec_mdata = (union rte_event_crypto_metadata *)
> > >> >> >> +		ec_mdata = (struct rte_event_crypto_metadata *)
> > >> >> >>  			((uint8_t *)op + op->private_data_offset);
> > >> >> >>  	else
> > >> >> >>  		return NULL;
> > >> >> >> @@ -731,7 +731,7 @@ get_event_crypto_mdata(struct
> > rte_crypto_op
> > >> >> *op)
> > >> >> >> uint16_t __rte_hot  otx_crypto_adapter_enqueue(void *port,
> > >> >> >> struct rte_crypto_op *op)  {
> > >> >> >> -	union rte_event_crypto_metadata *ec_mdata;
> > >> >> >> +	struct rte_event_crypto_metadata *ec_mdata;
> > >> >> >>  	struct cpt_instance *instance;
> > >> >> >>  	struct cpt_request_info *req;
> > >> >> >>  	struct rte_event *rsp_info; diff --git
> > >> >> >> a/drivers/crypto/octeontx2/otx2_cryptodev_ops.c
> > >> >> >> b/drivers/crypto/octeontx2/otx2_cryptodev_ops.c
> > >> >> >> index 42100154cd..952d1352f4 100644
> > >> >> >> --- a/drivers/crypto/octeontx2/otx2_cryptodev_ops.c
> > >> >> >> +++ b/drivers/crypto/octeontx2/otx2_cryptodev_ops.c
> > >> >> >> @@ -453,7 +453,7 @@ otx2_ca_enqueue_req(const struct
> > >> otx2_cpt_qp
> > >> >> *qp,
> > >> >> >>  		    struct rte_crypto_op *op,
> > >> >> >>  		    uint64_t cpt_inst_w7)
> > >> >> >>  {
> > >> >> >> -	union rte_event_crypto_metadata *m_data;
> > >> >> >> +	struct rte_event_crypto_metadata *m_data;
> > >> >> >>  	union cpt_inst_s inst;
> > >> >> >>  	uint64_t lmt_status;
> > >> >> >>
> > >> >> >> @@ -468,7 +468,7 @@ otx2_ca_enqueue_req(const struct
> > >> otx2_cpt_qp
> > >> >> *qp,
> > >> >> >>  		}
> > >> >> >>  	} else if (op->sess_type == RTE_CRYPTO_OP_SESSIONLESS
> > &&
> > >> >> >>  		   op->private_data_offset) {
> > >> >> >> -		m_data = (union rte_event_crypto_metadata *)
> > >> >> >> +		m_data = (struct rte_event_crypto_metadata *)
> > >> >> >>  			 ((uint8_t *)op +
> > >> >> >>  			  op->private_data_offset);
> > >> >> >>  	} else {
> > >> >> >> diff --git
> > >> >> >> a/drivers/event/octeontx2/otx2_evdev_crypto_adptr_tx.h
> > >> >> >> b/drivers/event/octeontx2/otx2_evdev_crypto_adptr_tx.h
> > >> >> >> index ecf7eb9f56..458e8306d7 100644
> > >> >> >> --- a/drivers/event/octeontx2/otx2_evdev_crypto_adptr_tx.h
> > >> >> >> +++ b/drivers/event/octeontx2/otx2_evdev_crypto_adptr_tx.h
> > >> >> >> @@ -16,7 +16,7 @@
> > >> >> >>  static inline uint16_t
> > >> >> >>  otx2_ca_enq(uintptr_t tag_op, const struct rte_event *ev)  {
> > >> >> >> -	union rte_event_crypto_metadata *m_data;
> > >> >> >> +	struct rte_event_crypto_metadata *m_data;
> > >> >> >>  	struct rte_crypto_op *crypto_op;
> > >> >> >>  	struct rte_cryptodev *cdev;
> > >> >> >>  	struct otx2_cpt_qp *qp;
> > >> >> >> @@ -37,7 +37,7 @@ otx2_ca_enq(uintptr_t tag_op, const struct
> > >> >> >> rte_event
> > >> >> >> *ev)
> > >> >> >>  		qp_id = m_data->request_info.queue_pair_id;
> > >> >> >>  	} else if (crypto_op->sess_type ==
> > RTE_CRYPTO_OP_SESSIONLESS
> > >> >> &&
> > >> >> >>  		   crypto_op->private_data_offset) {
> > >> >> >> -		m_data = (union rte_event_crypto_metadata *)
> > >> >> >> +		m_data = (struct rte_event_crypto_metadata *)
> > >> >> >>  			 ((uint8_t *)crypto_op +
> > >> >> >>  			  crypto_op->private_data_offset);
> > >> >> >>  		cdev_id = m_data->request_info.cdev_id; diff --git
> > >> >> >> a/lib/eventdev/rte_event_crypto_adapter.c
> > >> >> >> b/lib/eventdev/rte_event_crypto_adapter.c
> > >> >> >> index e1d38d383d..6977391ae9 100644
> > >> >> >> --- a/lib/eventdev/rte_event_crypto_adapter.c
> > >> >> >> +++ b/lib/eventdev/rte_event_crypto_adapter.c
> > >> >> >> @@ -333,7 +333,7 @@ eca_enq_to_cryptodev(struct
> > >> >> >> rte_event_crypto_adapter *adapter,
> > >> >> >>  		 struct rte_event *ev, unsigned int cnt)  {
> > >> >> >>  	struct rte_event_crypto_adapter_stats *stats = &adapter-
> > >> >> >> >crypto_stats;
> > >> >> >> -	union rte_event_crypto_metadata *m_data = NULL;
> > >> >> >> +	struct rte_event_crypto_metadata *m_data = NULL;
> > >> >> >>  	struct crypto_queue_pair_info *qp_info = NULL;
> > >> >> >>  	struct rte_crypto_op *crypto_op;
> > >> >> >>  	unsigned int i, n;
> > >> >> >> @@ -371,7 +371,7 @@ eca_enq_to_cryptodev(struct
> > >> >> >> rte_event_crypto_adapter *adapter,
> > >> >> >>  			len++;
> > >> >> >>  		} else if (crypto_op->sess_type ==
> > >> >> RTE_CRYPTO_OP_SESSIONLESS &&
> > >> >> >>  				crypto_op->private_data_offset) {
> > >> >> >> -			m_data = (union
> > rte_event_crypto_metadata *)
> > >> >> >> +			m_data = (struct
> > rte_event_crypto_metadata *)
> > >> >> >>  				 ((uint8_t *)crypto_op +
> > >> >> >>  					crypto_op-
> > >private_data_offset);
> > >> >> >>  			cdev_id = m_data->request_info.cdev_id;
> > @@ -
> > >> >> >> 504,7 +504,7 @@ eca_ops_enqueue_burst(struct
> > >> >> rte_event_crypto_adapter
> > >> >> >> *adapter,
> > >> >> >>  		  struct rte_crypto_op **ops, uint16_t num)  {
> > >> >> >>  	struct rte_event_crypto_adapter_stats *stats = &adapter-
> > >> >> >> >crypto_stats;
> > >> >> >> -	union rte_event_crypto_metadata *m_data = NULL;
> > >> >> >> +	struct rte_event_crypto_metadata *m_data = NULL;
> > >> >> >>  	uint8_t event_dev_id = adapter->eventdev_id;
> > >> >> >>  	uint8_t event_port_id = adapter->event_port_id;
> > >> >> >>  	struct rte_event events[BATCH_SIZE]; @@ -523,7 +523,7 @@
> > >> >> >> eca_ops_enqueue_burst(struct rte_event_crypto_adapter
> > *adapter,
> > >> >> >>  					ops[i]->sym->session);
> > >> >> >>  		} else if (ops[i]->sess_type ==
> > >> RTE_CRYPTO_OP_SESSIONLESS &&
> > >> >> >>  				ops[i]->private_data_offset) {
> > >> >> >> -			m_data = (union
> > rte_event_crypto_metadata *)
> > >> >> >> +			m_data = (struct
> > rte_event_crypto_metadata *)
> > >> >> >>  				 ((uint8_t *)ops[i] +
> > >> >> >>  				  ops[i]->private_data_offset);
> > >> >> >>  		}
> > >> >> >> diff --git a/lib/eventdev/rte_event_crypto_adapter.h
> > >> >> >> b/lib/eventdev/rte_event_crypto_adapter.h
> > >> >> >> index f8c6cca87c..3c24d9d9df 100644
> > >> >> >> --- a/lib/eventdev/rte_event_crypto_adapter.h
> > >> >> >> +++ b/lib/eventdev/rte_event_crypto_adapter.h
> > >> >> >> @@ -200,11 +200,6 @@ enum rte_event_crypto_adapter_mode
> {
> > >> >> >>   * provide event request information to the adapter.
> > >> >> >>   */
> > >> >> >>  struct rte_event_crypto_request {
> > >> >> >> -	uint8_t resv[8];
> > >> >> >> -	/**< Overlaps with first 8 bytes of struct rte_event
> > >> >> >> -	 * that encode the response event information. Application
> > >> >> >> -	 * is expected to fill in struct rte_event response_info.
> > >> >> >> -	 */
> > >> >> >>  	uint16_t cdev_id;
> > >> >> >>  	/**< cryptodev ID to be used */
> > >> >> >>  	uint16_t queue_pair_id;
> > >> >> >> @@ -223,16 +218,16 @@ struct rte_event_crypto_request {
> > >> >> >>   * operation. If the transfer is done by SW, event response
> > >> information
> > >> >> >>   * will be used by the adapter.
> > >> >> >>   */
> > >> >> >> -union rte_event_crypto_metadata {
> > >> >> >> -	struct rte_event_crypto_request request_info;
> > >> >> >> -	/**< Request information to be filled in by application
> > >> >> >> -	 * for RTE_EVENT_CRYPTO_ADAPTER_OP_FORWARD mode.
> > >> >> >> -	 */
> > >> >> >> +struct rte_event_crypto_metadata {
> > >> >> >>  	struct rte_event response_info;
> > >> >> >>  	/**< Response information to be filled in by application
> > >> >> >>  	 * for RTE_EVENT_CRYPTO_ADAPTER_OP_NEW and
> > >> >> >>  	 * RTE_EVENT_CRYPTO_ADAPTER_OP_FORWARD mode.
> > >> >> >>  	 */
> > >> >> >> +	struct rte_event_crypto_request request_info;
> > >> >> >> +	/**< Request information to be filled in by application
> > >> >> >> +	 * for RTE_EVENT_CRYPTO_ADAPTER_OP_FORWARD mode.
> > >> >> >> +	 */
> > >> >> >>  };
> > >> >> >>
> > >> >> >>  /**
> > >> >> >> --
> > >> >> >> 2.25.1

Re: [dpdk-dev] [PATCH v3] eventdev: update crypto adapter metadata structures

[Akhil Goyal]

Hi Abhinandan,
> > > >> >>
> > > >> >> >> In crypto adapter metadata, reserved bytes in request info
> > > >> >> >> structure is a space holder for response info. It enforces an
> > > >> >> >> order of operation if the structures are updated using memcpy
> > > >> >> >> to avoid overwriting response info. It is logical to move the
> > > >> >> >> reserved space out of request info. It also solves the
> > > >> >> >> ordering issue
> > > >> mentioned before.
> > > >> >> >I would like to understand what kind of ordering issue you have
> > > >> >> >faced with the current approach. Could you please give an
> > > >> >> >example/sequence
> > > >> >> and explain?
> > > >> >> >
> > > >> >>
> > > >> >> I have seen this issue with crypto adapter autotest (#n215).
> > > >> >>
> > > >> >> Example:
> > > >> >> rte_memcpy(&m_data.response_info, &response_info,
> > > >> >> sizeof(response_info)); rte_memcpy(&m_data.request_info,
> > > >> >> &request_info, sizeof(request_info));
> > > >> >>
> > > >> >> Here response info is getting overwritten by request info.
> > > >> >> Above lines can reordered to fix the issue, but can be ignored
> > > >> >> with this
> > > >> patch.
> > > >> >There is a reason for designing the metadata in this way.
> > > >> >Right now, sizeof (union rte_event_crypto_metadata) is 16 bytes.
> > > >> >So, the session based case needs just 16 bytes to store the data.
> > > >> >Whereas, for sessionless case each crypto_ops requires another 16
> > > bytes.
> > > >> >
> > > >> >By changing the struct in the following way you are doubling the
> > > >> >memory requirement.
> > > >> >With the changes, for sessionless case, each crypto op requires 32
> > > >> >bytes of space instead of 16 bytes and the mempool will be bigger.
> > > >> >This will have the perf impact too!
> > > >> >
> > > >> >You can just copy the individual members(cdev_id & queue_pair_id)
> > > >> >after the response_info.
> > > >> >OR You have a better way?
> > > >> >
> > > >>
> > > >> I missed the second word of event structure. It adds an extra 8
> > > >> bytes, which is not required.
> > > >I guess you meant not adding, it is overlapping with request
> information.
> > > >> Let me know, what you think of below change to metadata structure.
> > > >>
> > > >> struct rte_event_crypto_metadata {
> > > >> 	uint64_t response_info; // 8 bytes
> > > >With this, it lags clarity saying it is an event information.
> > > >> 	struct rte_event_crypto_request request_info; // 8 bytes };
> > > >>
> > > >> Total structure size is 16 bytes.
> > > >>
> > > >> Response info can be set like below in test app:
> > > >> 	m_data.response_info = response_info.event;
> > > >>
> > > >> The main aim of this patch is to decouple response info from request
> > info.
> > > >The decoupling was required as it was doing memcpy.
> > > >If you copy the individual members of request info(after response),
> > > >you don't require it.
> > >
> > > With this change, the application will be free of such constraints.
> >
> > [Anoob] Why don't we make it like,
> >
> > struct rte_event_crypto_metadata {
> >  	uint64_t ev_w0_template;
> >               /**< Template of the first word of ``struct rte_event``
> > (rte_event.event) to be set in the events generated by crypto adapter in
> > both RTE_EVENT_CRYPTO_ADAPTER_OP_NEW and
> > 	 * RTE_EVENT_CRYPTO_ADAPTER_OP_FORWARD modes.
> > 	*/
> >  	struct rte_event_crypto_request request_info; };
> >
> > This way, the usage would become more obvious and we will not have
> > additional space reserved as well.
> 
> Thanks for the suggestion. At this point, it is like an application without
> understanding the data structure/ internals
> has used memcpy and we are trying to fix it by changing the actual data
> structure instead of fixing the application!
> With this patch, the other applications(outside of dpdk) which are using the
> data structures in a right are forced to change!
> I don't think, this is the right way to handle this. I think, we should be
> updating the test case and documentation for this rather
> than changing the data structure.  I expect the next version of this patch
> should have those changes. Thanks!

The point here is about making the specification better and clearer to the user.
If the structure is not clear and is error prone if somebody does not follow
Proper documentation, we can modify it to reduce possible issues.

This is a cleanup which was added in deprecation notice as well in the last release.
This is a ABI break release and we should do this cleanup if it is legit.
Applications outside DPDK are notified as per the deprecation notice in the last release.
We have followed standard procedure to modify this structure,
hence, no need to worry about that.
We have provided, 2-3 suggestions to clean this up. Please suggest which
one is best for Intel use case.

Having first element as reserved in the structure doesn't look quite obvious.
This was also highlighted in the original patch, but was not taken up seriously
as we were not supporting it at that point.
See this
http://patches.dpdk.org/project/dpdk/patch/1524573807-168522-2-git-send-email-abhinandan.gujjar@intel.com/

-Akhil

Please note:
Avoid top posting for comments and remove unnecessary lines while commenting back.

Re: [dpdk-dev] [PATCH v3] eventdev: update crypto adapter metadata structures

[Gujjar, Abhinandan S]

Hi Akhil,

> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Monday, September 20, 2021 12:19 AM
> To: Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>; Anoob Joseph
> <anoobj@marvell.com>; Shijith Thotton <sthotton@marvell.com>
> Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Pavan Nikhilesh
> Bhagavatula <pbhagavatula@marvell.com>; Ray Kinsella <mdr@ashroe.eu>;
> Ankur Dwivedi <adwivedi@marvell.com>; dev@dpdk.org;
> thomas@monjalon.net; nipun.gupta@nxp.com; Hemant Agrawal
> <hemant.agrawal@nxp.com>
> Subject: RE: [PATCH v3] eventdev: update crypto adapter metadata
> structures
> 
> Hi Abhinandan,
> > > > >> >>
> > > > >> >> >> In crypto adapter metadata, reserved bytes in request
> > > > >> >> >> info structure is a space holder for response info. It
> > > > >> >> >> enforces an order of operation if the structures are
> > > > >> >> >> updated using memcpy to avoid overwriting response info.
> > > > >> >> >> It is logical to move the reserved space out of request
> > > > >> >> >> info. It also solves the ordering issue
> > > > >> mentioned before.
> > > > >> >> >I would like to understand what kind of ordering issue you
> > > > >> >> >have faced with the current approach. Could you please give
> > > > >> >> >an example/sequence
> > > > >> >> and explain?
> > > > >> >> >
> > > > >> >>
> > > > >> >> I have seen this issue with crypto adapter autotest (#n215).
> > > > >> >>
> > > > >> >> Example:
> > > > >> >> rte_memcpy(&m_data.response_info, &response_info,
> > > > >> >> sizeof(response_info)); rte_memcpy(&m_data.request_info,
> > > > >> >> &request_info, sizeof(request_info));
> > > > >> >>
> > > > >> >> Here response info is getting overwritten by request info.
> > > > >> >> Above lines can reordered to fix the issue, but can be
> > > > >> >> ignored with this
> > > > >> patch.
> > > > >> >There is a reason for designing the metadata in this way.
> > > > >> >Right now, sizeof (union rte_event_crypto_metadata) is 16 bytes.
> > > > >> >So, the session based case needs just 16 bytes to store the data.
> > > > >> >Whereas, for sessionless case each crypto_ops requires another
> > > > >> >16
> > > > bytes.
> > > > >> >
> > > > >> >By changing the struct in the following way you are doubling
> > > > >> >the memory requirement.
> > > > >> >With the changes, for sessionless case, each crypto op
> > > > >> >requires 32 bytes of space instead of 16 bytes and the mempool
> will be bigger.
> > > > >> >This will have the perf impact too!
> > > > >> >
> > > > >> >You can just copy the individual members(cdev_id &
> > > > >> >queue_pair_id) after the response_info.
> > > > >> >OR You have a better way?
> > > > >> >
> > > > >>
> > > > >> I missed the second word of event structure. It adds an extra 8
> > > > >> bytes, which is not required.
> > > > >I guess you meant not adding, it is overlapping with request
> > information.
> > > > >> Let me know, what you think of below change to metadata
> structure.
> > > > >>
> > > > >> struct rte_event_crypto_metadata {
> > > > >> 	uint64_t response_info; // 8 bytes
> > > > >With this, it lags clarity saying it is an event information.
> > > > >> 	struct rte_event_crypto_request request_info; // 8 bytes };
> > > > >>
> > > > >> Total structure size is 16 bytes.
> > > > >>
> > > > >> Response info can be set like below in test app:
> > > > >> 	m_data.response_info = response_info.event;
> > > > >>
> > > > >> The main aim of this patch is to decouple response info from
> > > > >> request
> > > info.
> > > > >The decoupling was required as it was doing memcpy.
> > > > >If you copy the individual members of request info(after
> > > > >response), you don't require it.
> > > >
> > > > With this change, the application will be free of such constraints.
> > >
> > > [Anoob] Why don't we make it like,
> > >
> > > struct rte_event_crypto_metadata {
> > >  	uint64_t ev_w0_template;
> > >               /**< Template of the first word of ``struct
> > > rte_event``
> > > (rte_event.event) to be set in the events generated by crypto
> > > adapter in both RTE_EVENT_CRYPTO_ADAPTER_OP_NEW and
> > > 	 * RTE_EVENT_CRYPTO_ADAPTER_OP_FORWARD modes.
> > > 	*/
> > >  	struct rte_event_crypto_request request_info; };
> > >
> > > This way, the usage would become more obvious and we will not have
> > > additional space reserved as well.
> >
> > Thanks for the suggestion. At this point, it is like an application
> > without understanding the data structure/ internals has used memcpy
> > and we are trying to fix it by changing the actual data structure
> > instead of fixing the application!
> > With this patch, the other applications(outside of dpdk) which are
> > using the data structures in a right are forced to change!
> > I don't think, this is the right way to handle this. I think, we
> > should be updating the test case and documentation for this rather
> > than changing the data structure.  I expect the next version of this
> > patch should have those changes. Thanks!
> 
> The point here is about making the specification better and clearer to the
> user.
> If the structure is not clear and is error prone if somebody does not follow
> Proper documentation, we can modify it to reduce possible issues.
I think, the structure is clear. Apart from adding documentation,
I feel forming a structure with appropriate data type and naming it to make it self-explanatory is important.
That is what we have done, and this is already discussed in the original patch as well.
It is important for any application writer to go through the documentation and structure to understand before using it.
The current documentation clearing says it has overlap. If you still feel it lags clarity in some places please go ahead and add documentation.
It is not about the ABI breakage; it is about changing a proper structure for an ignorant application!
I am not convinced with the proposed changes. If you have a better one, please let me know.

> 
> This is a cleanup which was added in deprecation notice as well in the last
> release.
> This is a ABI break release and we should do this cleanup if it is legit.
> Applications outside DPDK are notified as per the deprecation notice in the
> last release.
> We have followed standard procedure to modify this structure, hence, no
> need to worry about that.
> We have provided, 2-3 suggestions to clean this up. Please suggest which
> one is best for Intel use case.
> 
> Having first element as reserved in the structure doesn't look quite obvious.
> This was also highlighted in the original patch, but was not taken up seriously
> as we were not supporting it at that point.
> See this
> http://patches.dpdk.org/project/dpdk/patch/1524573807-168522-2-git-
> send-email-abhinandan.gujjar@intel.com/

> 
> -Akhil
> 
> Please note:
> Avoid top posting for comments and remove unnecessary lines while
> commenting back.
> 
Please note that, I am using outlook with appropriate settings to respond back.
I am not sure, why this is coming as top posting.

[dpdk-dev] [PATCH v4] doc: remove event crypto metadata deprecation note

[Shijith Thotton]

Proposed change to event crypto metadata is not done as per deprecation
note. Instead, comments are updated in spec to improve readability.

Signed-off-by: Shijith Thotton <sthotton@marvell.com>
---
v4:
* Removed changes as per deprecation note.
* Updated spec comments.

v3:
* Updated ABI section of release notes.

v2:
* Updated deprecation notice.

v1:
* Rebased.

 doc/guides/rel_notes/deprecation.rst    | 6 ------
 lib/eventdev/rte_event_crypto_adapter.h | 1 +
 2 files changed, 1 insertion(+), 6 deletions(-)

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index bf1e07c0a8..fae3abd282 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -254,12 +254,6 @@ Deprecation Notices
   An 8-byte reserved field will be added to the structure ``rte_event_timer`` to
   support future extensions.
 
-* eventdev: Reserved bytes of ``rte_event_crypto_request`` is a space holder
-  for ``response_info``. Both should be decoupled for better clarity.
-  New space for ``response_info`` can be made by changing
-  ``rte_event_crypto_metadata`` type to structure from union.
-  This change is targeted for DPDK 21.11.
-
 * metrics: The function ``rte_metrics_init`` will have a non-void return
   in order to notify errors instead of calling ``rte_exit``.
 
diff --git a/lib/eventdev/rte_event_crypto_adapter.h b/lib/eventdev/rte_event_crypto_adapter.h
index 27fb628eef..edbd5c61a3 100644
--- a/lib/eventdev/rte_event_crypto_adapter.h
+++ b/lib/eventdev/rte_event_crypto_adapter.h
@@ -227,6 +227,7 @@ union rte_event_crypto_metadata {
 	struct rte_event_crypto_request request_info;
 	/**< Request information to be filled in by application
 	 * for RTE_EVENT_CRYPTO_ADAPTER_OP_FORWARD mode.
+	 * First 8 bytes of request_info is reserved for response_info.
 	 */
 	struct rte_event response_info;
 	/**< Response information to be filled in by application
-- 
2.25.1

[dpdk-dev] [PATCH] test/event_crypto: fix event crypto metadata write

[Shijith Thotton]

Using memcpy to update event crypto metadata fields (request/response)
will result in one overwriting the other. To avoid this, fields of each
structure should be updated one by one.

Fixes: 3c2c535ecfc0 ("test: add event crypto adapter auto-test")

Signed-off-by: Shijith Thotton <sthotton@marvell.com>
---
 app/test/test_event_crypto_adapter.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/app/test/test_event_crypto_adapter.c b/app/test/test_event_crypto_adapter.c
index 279aa3abf5..3d7e9fb93c 100644
--- a/app/test/test_event_crypto_adapter.c
+++ b/app/test/test_event_crypto_adapter.c
@@ -212,10 +212,10 @@ test_op_forward_mode(uint8_t session_less)
 
 		if (cap & RTE_EVENT_CRYPTO_ADAPTER_CAP_SESSION_PRIVATE_DATA) {
 			/* Fill in private user data information */
-			rte_memcpy(&m_data.response_info, &response_info,
-				sizeof(response_info));
-			rte_memcpy(&m_data.request_info, &request_info,
-				sizeof(request_info));
+			m_data.request_info.cdev_id = request_info.cdev_id;
+			m_data.request_info.queue_pair_id =
+				request_info.queue_pair_id;
+			m_data.response_info.event = response_info.event;
 			rte_cryptodev_sym_session_set_user_data(sess,
 						&m_data, sizeof(m_data));
 		}
@@ -231,10 +231,9 @@ test_op_forward_mode(uint8_t session_less)
 		uint32_t len = IV_OFFSET + MAXIMUM_IV_LENGTH;
 		op->private_data_offset = len;
 		/* Fill in private data information */
-		rte_memcpy(&m_data.response_info, &response_info,
-			   sizeof(response_info));
-		rte_memcpy(&m_data.request_info, &request_info,
-			   sizeof(request_info));
+		m_data.request_info.cdev_id = request_info.cdev_id;
+		m_data.request_info.queue_pair_id = request_info.queue_pair_id;
+		m_data.response_info.event = response_info.event;
 		rte_memcpy((uint8_t *)op + len, &m_data, sizeof(m_data));
 	}
 
@@ -405,8 +404,7 @@ test_op_new_mode(uint8_t session_less)
 
 		if (cap & RTE_EVENT_CRYPTO_ADAPTER_CAP_SESSION_PRIVATE_DATA) {
 			/* Fill in private user data information */
-			rte_memcpy(&m_data.response_info, &response_info,
-				   sizeof(m_data));
+			m_data.response_info.event = response_info.event;
 			rte_cryptodev_sym_session_set_user_data(sess,
 						&m_data, sizeof(m_data));
 		}
@@ -425,8 +423,7 @@ test_op_new_mode(uint8_t session_less)
 		uint32_t len = IV_OFFSET + MAXIMUM_IV_LENGTH;
 		op->private_data_offset = len;
 		/* Fill in private data information */
-		rte_memcpy(&m_data.response_info, &response_info,
-			   sizeof(m_data));
+		m_data.response_info.event = response_info.event;
 		rte_memcpy((uint8_t *)op + len, &m_data, sizeof(m_data));
 	}
 
-- 
2.25.1

Re: [dpdk-dev] [PATCH] test/event_crypto: fix event crypto metadata write

[Gujjar, Abhinandan S]

Acked-by: Abhinandan Gujjar <Abhinandan.gujjar@intel.com>

> -----Original Message-----
> From: Shijith Thotton <sthotton@marvell.com>
> Sent: Monday, September 27, 2021 8:59 PM
> To: dev@dpdk.org
> Cc: stable@dpdk.org; Shijith Thotton <sthotton@marvell.com>;
> jerinj@marvell.com; anoobj@marvell.com; Gujjar, Abhinandan S
> <abhinandan.gujjar@intel.com>
> Subject: [PATCH] test/event_crypto: fix event crypto metadata write
> 
> Using memcpy to update event crypto metadata fields (request/response)
> will result in one overwriting the other. To avoid this, fields of each structure
> should be updated one by one.
> 
> Fixes: 3c2c535ecfc0 ("test: add event crypto adapter auto-test")
> 
> Signed-off-by: Shijith Thotton <sthotton@marvell.com>
> ---
>  app/test/test_event_crypto_adapter.c | 21 +++++++++------------
>  1 file changed, 9 insertions(+), 12 deletions(-)
> 
> diff --git a/app/test/test_event_crypto_adapter.c
> b/app/test/test_event_crypto_adapter.c
> index 279aa3abf5..3d7e9fb93c 100644
> --- a/app/test/test_event_crypto_adapter.c
> +++ b/app/test/test_event_crypto_adapter.c
> @@ -212,10 +212,10 @@ test_op_forward_mode(uint8_t session_less)
> 
>  		if (cap &
> RTE_EVENT_CRYPTO_ADAPTER_CAP_SESSION_PRIVATE_DATA) {
>  			/* Fill in private user data information */
> -			rte_memcpy(&m_data.response_info,
> &response_info,
> -				sizeof(response_info));
> -			rte_memcpy(&m_data.request_info, &request_info,
> -				sizeof(request_info));
> +			m_data.request_info.cdev_id =
> request_info.cdev_id;
> +			m_data.request_info.queue_pair_id =
> +				request_info.queue_pair_id;
> +			m_data.response_info.event =
> response_info.event;
>  			rte_cryptodev_sym_session_set_user_data(sess,
>  						&m_data, sizeof(m_data));
>  		}
> @@ -231,10 +231,9 @@ test_op_forward_mode(uint8_t session_less)
>  		uint32_t len = IV_OFFSET + MAXIMUM_IV_LENGTH;
>  		op->private_data_offset = len;
>  		/* Fill in private data information */
> -		rte_memcpy(&m_data.response_info, &response_info,
> -			   sizeof(response_info));
> -		rte_memcpy(&m_data.request_info, &request_info,
> -			   sizeof(request_info));
> +		m_data.request_info.cdev_id = request_info.cdev_id;
> +		m_data.request_info.queue_pair_id =
> request_info.queue_pair_id;
> +		m_data.response_info.event = response_info.event;
>  		rte_memcpy((uint8_t *)op + len, &m_data, sizeof(m_data));
>  	}
> 
> @@ -405,8 +404,7 @@ test_op_new_mode(uint8_t session_less)
> 
>  		if (cap &
> RTE_EVENT_CRYPTO_ADAPTER_CAP_SESSION_PRIVATE_DATA) {
>  			/* Fill in private user data information */
> -			rte_memcpy(&m_data.response_info,
> &response_info,
> -				   sizeof(m_data));
> +			m_data.response_info.event =
> response_info.event;
>  			rte_cryptodev_sym_session_set_user_data(sess,
>  						&m_data, sizeof(m_data));
>  		}
> @@ -425,8 +423,7 @@ test_op_new_mode(uint8_t session_less)
>  		uint32_t len = IV_OFFSET + MAXIMUM_IV_LENGTH;
>  		op->private_data_offset = len;
>  		/* Fill in private data information */
> -		rte_memcpy(&m_data.response_info, &response_info,
> -			   sizeof(m_data));
> +		m_data.response_info.event = response_info.event;
>  		rte_memcpy((uint8_t *)op + len, &m_data, sizeof(m_data));
>  	}
> 
> --
> 2.25.1

Re: [dpdk-dev] [PATCH v4] doc: remove event crypto metadata deprecation note

[Gujjar, Abhinandan S]

Acked-by: Abhinandan Gujjar <Abhinandan.gujjar@intel.com>

> -----Original Message-----
> From: Shijith Thotton <sthotton@marvell.com>
> Sent: Monday, September 27, 2021 8:53 PM
> To: dev@dpdk.org
> Cc: Shijith Thotton <sthotton@marvell.com>; Gujjar, Abhinandan S
> <abhinandan.gujjar@intel.com>; adwivedi@marvell.com;
> anoobj@marvell.com; gakhil@marvell.com; jerinj@marvell.com;
> pbhagavatula@marvell.com
> Subject: [PATCH v4] doc: remove event crypto metadata deprecation note
> 
> Proposed change to event crypto metadata is not done as per deprecation
> note. Instead, comments are updated in spec to improve readability.
> 
> Signed-off-by: Shijith Thotton <sthotton@marvell.com>
> ---
> v4:
> * Removed changes as per deprecation note.
> * Updated spec comments.
> 
> v3:
> * Updated ABI section of release notes.
> 
> v2:
> * Updated deprecation notice.
> 
> v1:
> * Rebased.
> 
>  doc/guides/rel_notes/deprecation.rst    | 6 ------
>  lib/eventdev/rte_event_crypto_adapter.h | 1 +
>  2 files changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/doc/guides/rel_notes/deprecation.rst
> b/doc/guides/rel_notes/deprecation.rst
> index bf1e07c0a8..fae3abd282 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -254,12 +254,6 @@ Deprecation Notices
>    An 8-byte reserved field will be added to the structure ``rte_event_timer``
> to
>    support future extensions.
> 
> -* eventdev: Reserved bytes of ``rte_event_crypto_request`` is a space
> holder
> -  for ``response_info``. Both should be decoupled for better clarity.
> -  New space for ``response_info`` can be made by changing
> -  ``rte_event_crypto_metadata`` type to structure from union.
> -  This change is targeted for DPDK 21.11.
> -
>  * metrics: The function ``rte_metrics_init`` will have a non-void return
>    in order to notify errors instead of calling ``rte_exit``.
> 
> diff --git a/lib/eventdev/rte_event_crypto_adapter.h
> b/lib/eventdev/rte_event_crypto_adapter.h
> index 27fb628eef..edbd5c61a3 100644
> --- a/lib/eventdev/rte_event_crypto_adapter.h
> +++ b/lib/eventdev/rte_event_crypto_adapter.h
> @@ -227,6 +227,7 @@ union rte_event_crypto_metadata {
>  	struct rte_event_crypto_request request_info;
>  	/**< Request information to be filled in by application
>  	 * for RTE_EVENT_CRYPTO_ADAPTER_OP_FORWARD mode.
> +	 * First 8 bytes of request_info is reserved for response_info.
>  	 */
>  	struct rte_event response_info;
>  	/**< Response information to be filled in by application
> --
> 2.25.1

Re: [dpdk-dev] [PATCH v4] doc: remove event crypto metadata deprecation note

[Akhil Goyal]

> Acked-by: Abhinandan Gujjar <Abhinandan.gujjar@intel.com>
> 
Applied to dpdk-next-crypto

Thanks.

Re: [dpdk-dev] [PATCH] test/event_crypto: fix event crypto metadata write

[Akhil Goyal]

> Acked-by: Abhinandan Gujjar <Abhinandan.gujjar@intel.com>
> 
Applied to dpdk-next-crypto

Thanks.