From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by dpdk.org (Postfix) with ESMTP id 7E787A48C for ; Mon, 19 Feb 2018 11:56:03 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga107.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 19 Feb 2018 02:56:02 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.46,534,1511856000"; d="scan'208";a="205284776" Received: from pgsmsx101.gar.corp.intel.com ([10.221.44.78]) by fmsmga005.fm.intel.com with ESMTP; 19 Feb 2018 02:56:00 -0800 Received: from pgsmsx110.gar.corp.intel.com (10.221.44.111) by PGSMSX101.gar.corp.intel.com (10.221.44.78) with Microsoft SMTP Server (TLS) id 14.3.319.2; Mon, 19 Feb 2018 18:55:59 +0800 Received: from pgsmsx102.gar.corp.intel.com ([169.254.6.72]) by PGSMSX110.gar.corp.intel.com ([169.254.13.90]) with mapi id 14.03.0319.002; Mon, 19 Feb 2018 18:55:58 +0800 From: "Gujjar, Abhinandan S" To: Jerin Jacob CC: "dev@dpdk.org" , "Vangati, Narender" , "Rao, Nikhil" , "Eads, Gage" , "hemant.agrawal@nxp.com" , "akhil.goyal@nxp.com" , "narayanaprasad.athreya@cavium.com" , "nidadavolu.murthy@cavium.com" , "nithin.dabilpuram@cavium.com" Thread-Topic: [RFC v2, 2/2] eventdev: add crypto adapter API header Thread-Index: AQHTje820XjHLBSSDk6omwDVW3/+OqOnGCiAgASaP7A= Date: Mon, 19 Feb 2018 10:55:58 +0000 Message-ID: <5612CB344B05EE4F95FC5B729939F7807069B737@PGSMSX102.gar.corp.intel.com> References: <1516013630-146114-1-git-send-email-abhinandan.gujjar@intel.com> <20180216193348.GA8882@jerin> In-Reply-To: <20180216193348.GA8882@jerin> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ctpclassification: CTP_NT x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMTE1NzFjYmYtZjljMi00Y2Y4LWJkNzUtMGZjMzc3NmZmMWEwIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE2LjUuOS4zIiwiVHJ1c3RlZExhYmVsSGFzaCI6Ikp2SXZUZWlXTWVwTDJoa0N3SVFuZExnazg5VmdHNWtIU1FcLzhGUE4zcTVVPSJ9 dlp-product: dlpe-windows dlp-version: 11.0.0.116 dlp-reaction: no-action x-originating-ip: [172.30.20.206] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [RFC v2, 2/2] eventdev: add crypto adapter API header X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 19 Feb 2018 10:56:04 -0000 Hi Jerin, Thanks for the review. Please find few comments inline. > -----Original Message----- > From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com] > Sent: Saturday, February 17, 2018 1:04 AM > To: Gujjar, Abhinandan S > Cc: dev@dpdk.org; Vangati, Narender ; Rao, > Nikhil ; Eads, Gage ; > hemant.agrawal@nxp.com; akhil.goyal@nxp.com; > narayanaprasad.athreya@cavium.com; nidadavolu.murthy@cavium.com; > nithin.dabilpuram@cavium.com > Subject: Re: [RFC v2, 2/2] eventdev: add crypto adapter API header >=20 > -----Original Message----- > > Date: Mon, 15 Jan 2018 16:23:50 +0530 > > From: Abhinandan Gujjar > > To: jerin.jacob@caviumnetworks.com > > CC: dev@dpdk.org, narender.vangati@intel.com, Abhinandan Gujjar > > , Nikhil Rao , Gage > > Eads > > Subject: [RFC v2, 2/2] eventdev: add crypto adapter API header > > X-Mailer: git-send-email 1.9.1 > > > > Add crypto event adapter APIs to support packet transfer mechanism > > between cryptodev and event device. > > > > Signed-off-by: Abhinandan Gujjar > > Signed-off-by: Nikhil Rao > > Signed-off-by: Gage Eads > > --- >=20 > Overall it looks good to me. Though I have some confusion over ENQ-DEQ > scheme. May be it will be get cleared along with implementation. >=20 > > Notes: > > V2: > > 1. Updated type as ENQ-DEQ in rte_event_crypto_adapter_type > > 2. Removed enum rte_event_crypto_conf_type > > 3. Updated struct rte_event_crypto_metadata > > 4. Removed struct rte_event_crypto_queue_pair_conf > > 5. Updated rte_event_crypto_adapter_queue_pair_add() API > > > > lib/librte_eventdev/Makefile | 1 + > > lib/librte_eventdev/rte_event_crypto_adapter.h | 452 > > +++++++++++++++++++++++++ >=20 > 1) Please update MAINTAINERS file > 2) Add meson build support > 3) Update doc/api/doxy-api-index.md file and check the doxygen output usi= ng > "make doc-api-html" > 4) Please add the programmers guide in doc/guides/prog_guide/ Sure >=20 > > > > +++ b/lib/librte_eventdev/rte_event_crypto_adapter.h > > @@ -0,0 +1,452 @@ > > +/* > > + * Copyright(c) 2018 Intel Corporation. All rights reserved. > > + * All rights reserved. > > + * > > + * Redistribution and use in source and binary forms, with or withou= t >=20 > Use SPDX tag scheme Ok >=20 > > +#ifndef _RTE_EVENT_CRYPTO_ADAPTER_ > > +#define _RTE_EVENT_CRYPTO_ADAPTER_ > > + > > +/** > > + * This adapter adds support to enqueue crypto completions to event de= vice. > > + * The packet flow from cryptodev to the event device can be > > +accomplished > > + * using both SW and HW based transfer mechanisms. > > + * The adapter uses a EAL service core function for SW based packet > > +transfer > > + * and uses the eventdev PMD functions to configure HW based packet > > +transfer > > + * between the cryptodev and the event device. > > + * > > + * In the case of SW based transfers, application can choose to > > +submit a >=20 > I think, we can remove "In the case of SW based transfers" as it should b= e > applicable for HW case too Ok. In that case, adapter will detect the presence of HW connection between cryptodev & eventdev and will not dequeue crypto completions. >=20 > > + * crypto operation directly to cryptodev or send it to the > > + cryptodev > > + * adapter via eventdev, the cryptodev adapter then submits the > > + crypto > > + * operation to the crypto device. The first mode is known as the >=20 > The first mode (DEQ) is very clear. In the second mode(ENQ_DEQ), > - How does "worker" submits the crypto work through crypto-adapter? > If I understand it correctly, "workers" always deals with only cryptodev'= s > rte_cryptodev_enqueue_burst() API and "service" function in crypto adapte= r > would be responsible for dequeue() from cryptodev and enqueue to eventdev= ? >=20 > I understand the need for OP_NEW vs OP_FWD mode difference in both modes. > Other than that, What makes ENQ_DEQ different? Could you share the flow f= or > ENQ_DEQ mode with APIs. /* Application changes for ENQ_DEQ mode: ------------------------------------------------- /* In ENQ_DEQ mode, to enqueue to adapter app * has to fill out following details. */ struct rte_event_crypto_request *req; struct rte_crypto_op *op =3D rte_crypto_op_alloc(); =09 /* fill request info */ req =3D (void *)((char *)op + op.private_data_offset); req->cdev_id =3D 1; req->queue_pair_id =3D 1; /* fill response info */ ... /* send event to crypto adapter */ ev->event_ptr =3D op; ev->queue_id =3D dst_event_qid; ev->priority =3D dst_priority; ev->sched_type =3D dst_sched_type; ev->event_type =3D RTE_EVENT_TYPE_CRYPTODEV; ev->sub_event_type =3D sub_event_type; ev->flow_id =3D dst_flow_id; ret =3D rte_event_enqueue_burst(event_dev_id, event_port_id, ev, 1); Adapter in ENQ_DEQ mode, submitting crypto ops to cryptodev: ---------------------------------------------------------------------------= -- n =3D rte_event_dequeue_burst(event_dev_id, event_port_id, ev, BATCH_SIZE,= time_out); struct rte_crypto_op *op =3D ev->event_ptr; struct rte_event_crypto_request *req =3D (void *)op + op.private_data_offs= et; cdev_id =3D req->cdev_id; qp_id =3D req->queue_pair_id ret =3D rte_cryptodev_enqueue_burst(cdev_id, qp_id, op, 1); */ >=20 > > + * dequeue only (DEQ) mode and the second as the enqueue - dequeue >=20 > extra space between "mode" and "and" Ok >=20 > > + * (ENQ_DEQ) mode. The choice of mode can be specified when creating > > + * the adapter. > > + * In the latter choice, the cryptodev adapter is able to use > > + * RTE_OP_FORWARD as the event dev enqueue type, this has a > > + performance > > + * advantage in "closed system" eventdevs like the eventdev SW PMD > > + and >=20 > s/eventdevs/eventdev ?? Ok >=20 > > + * also, the event cannot be dropped. > > + * > > + * In the ENQ_DEQ mode the application needs to specify the cryptodev > > + ID > > + * and queue pair ID (request information) in addition to the event > > + * information (response information) needed to enqueue an event > > + after > > + * the crypto operation has completed. The request and response > > + information > > + * are specified in the rte_crypto_op private_data. In the DEQ mode > > + the > > + * the application is required to provide only the response informatio= n. >=20 > Why ENQ_DEQ modes needs cryptodev ID and queue pair ID? Which is part of > rte_cryptodev_enqueue_burst(). May be I am missing something. >=20 > If ENQ_DEQ modes help in SW case then we can add it as capability. > I am just trying to see any scope of convergence. In ENQ_DEQ mode, adapter dequeues eventdev and dereferences cryptodev ID an= d queue pair ID to enqueue the crypto operations to cryptodev using rte_cryptodev_enqueue_b= urst() API. I hope, above code snippet would have made things more clear. >=20 > > + * > > + * In the ENQ_DEQ mode, application sends crypto operations as events > > + to > > + * the adapter which dequeues events and programs cryptodev operations= . > > + * The adapter then dequeues crypto completions from cryptodev and > > + * enqueue events to the event device. > > + * > > + * In the case of HW based transfers, the cryptodev PMD callback > > + invoked > > + * from rte_cryptodev_enqueue_burst() uses the response information > > + to > > + * setup the event for the cryptodev completion. > > + * > > + * The event crypto adapter provides common APIs to configure the > > + packet flow > > + * from the cryptodev to event devices across both SW and HW based > transfers. > > + * The crypto event adapter's functions are: > > + * - rte_event_crypto_adapter_create_ext() > > + * - rte_event_crypto_adapter_create() > > + * - rte_event_crypto_adapter_free() > > + * - rte_event_crypto_adapter_queue_pair_add() > > + * - rte_event_crypto_adapter_queue_pair_del() > > + * - rte_event_crypto_adapter_start() > > + * - rte_event_crypto_adapter_stop() > > + * - rte_event_crypto_adapter_stats_get() > > + * - rte_event_crypto_adapter_stats_reset() > > + > > + * The applicaton creates an instance using > > + rte_event_crypto_adapter_create() >=20 > s/applicaton/application Ok >=20 > > + * or rte_event_crypto_adapter_create_ext(). > > + * > > + * Cryptodev queue pair addition/deletion is done > > + * using the rte_event_crypto_adapter_queue_pair_xxx() APIs. > > + * > > + * The SW adapter or HW PMD uses rte_crypto_op::private_data_type to > > + decide > > + * whether request/response data is located in the crypto > > + session/crypto > > + * security session or at an offset in the rte_crypto_op. > > + * rte_crypto_op::private_data_offset is used to locate the > > + request/response > > + * in the rte_crypto_op. If the rte_crypto_op::private_data_type > > + * indicates that the data is in the crypto session/crypto security > > + session > > + * then the rte_crypto_op::sess_type is used to decide whether the > > + private > > + * data is in the session or security session. > > + * > > + * For session-less it is mandatory to place the request/response > > + data with > > + * the rte_crypto_op where as with crypto session/security session it > > + can be > > + * placed with the rte_crypto_op or in the session/security session. >=20 > Please update(if needed) above two paragraphs based on the conclusion fro= m > cryptodev change Sure >=20 > > + */ > > + > > +#ifdef __cplusplus > > +extern "C" { > > +#endif > > + > > +#include > > +#include > > + > > +#include "rte_eventdev.h" > > + > > +#define RTE_EVENT_CRYPTO_ADAPTER_MAX_INSTANCE 32 > > + > > +/** > > + * @warning > > + * @b EXPERIMENTAL: this enum may change without prior notice >=20 > Change to new EXPERIMENTAL tag scheme. Ok >=20 > > + * > > + * Crypto event adapter type > > + */ > > +enum rte_event_crypto_adapter_type { > > + RTE_EVENT_CRYPTO_ADAPTER_DEQ_ONLY =3D 1, > > + /**< Start only dequeue part of crypto adapter. > > + * Packets dequeued from cryptodev are enqueued to eventdev > > + * as new events and events will be treated as RTE_EVENT_OP_NEW. */ > > + RTE_EVENT_CRYPTO_ADAPTER_ENQ_DEQ, > > + /**< Start both enqueue & dequeue part of crypto adapter. > > + * Packet's event context will be retained and > > + * event will be treated as RTE_EVENT_OP_FORWARD. */ >=20 > This description makes sense. >=20 > > +}; > > + > > +/** > > + * @warning > > + * @b EXPERIMENTAL: this structure may change without prior notice > > + * > > + * Crypto event request structure will be fill by application to > > + * 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 > > + */ > > + uint16_t cdev_id; > > + /**< cryptodev ID to be used */ > > + uint16_t queue_pair_id; > > + /**< cryptodev queue pair ID to be used */ >=20 > Shouldn't we say it is need only with ENQ_DEQ mode as described earlier. Yes. I will update it. > adding "@see" section would help Ok >=20 > > + uint32_t resv1; > > + /**< Reserved bits */ > > +}; > > + > > +/** > > + * @warning > > + * @b EXPERIMENTAL: this structure may change without prior notice > > + * > > + * Crypto event metadata structure will be filled by application > > + * to provide crypto request and event response information. > > + * > > + * If crypto events are enqueued using a HW mechanism, the cryptodev > > + * PMD will use the event response information to set up the event > > + * that is enqueued back to eventdev after completion of the crypto > > + * operation. If the transfer is done by SW, it will be used by the > > + * adapter. > > + */ > > +union rte_event_crypto_metadata { > > + struct rte_event_crypto_request request_info; > > + struct rte_event response_info; >=20 > Perfect. >=20 > Other than above comments, everything else looks OK to me. -Abhinandan