From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 61BFCA2F18 for ; Thu, 3 Oct 2019 15:24:33 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 227931C0CD; Thu, 3 Oct 2019 15:24:33 +0200 (CEST) Received: from EUR02-VE1-obe.outbound.protection.outlook.com (mail-eopbgr20061.outbound.protection.outlook.com [40.107.2.61]) by dpdk.org (Postfix) with ESMTP id 044361C0CC for ; Thu, 3 Oct 2019 15:24:32 +0200 (CEST) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=nboFZL/LBtdhM9fniYUZPYo1rIltg3XekrAanH6R6hkENfA3Ku6BLGkY7jnNu2eTVp92eaFiyEmtcXmj8Aowrih7ov3pgbd2WbboaMxRFL8xs6UYsym8kiVgpqg1RrFcuD+lRzMAXNFTMVMuSBmcaNSMxMNlrpLJSzr0453ypPBJmO1nMFwqC9uh6dBxdLNz31MCq+w0ZGhO4rdI9U17QrRk3cD//7oVWrTrvQj/SR0ajAZlFe8lT6OMRp8RjM35wgYd4ddKmFa9Zy0rVmx5uAD6JVNHDBHJc87vazVDHgLZ2FeoHe6ww0miNhbYh4DDUI5lZzMp4bOEsNSKbgA07Q== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=AEl5MEJQgVs3T3sJUEfNrmlcjBVV6HAjN1sxvhnoNvA=; b=RJI2AxagWm7BDy5D7994cUNYAo/TvbnxvVtVnurMTovixceicqmi7xLLq8aziHQZWG4iNDIL1dmMYJcOCSi/+Wc9OroP7b6MigrgYQY5GaM+1ELZGH5mGHPRWzTNpS+JjzJpKYXVUK98fj65BH0KmIYX8BSe9MD3DGPsRjpQH68bjx7Ix1yagk+J5Pzwo4klmntHUF+t3jWO+tVTOsVrpjXX4YAoiszsC53JrUZbA+0C5MwZ+aB/oAMYTaO6UBZu1D+k75aaO5QyiDhgS1vtQepGU/X54d0/9+mL5Ponnot47OLsiIVgJPHeAdTtvUqIfd+1LlQaJ+5wIv/kqEHm0Q== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=nxp.com; dmarc=pass action=none header.from=nxp.com; dkim=pass header.d=nxp.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nxp.com; s=selector2; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=AEl5MEJQgVs3T3sJUEfNrmlcjBVV6HAjN1sxvhnoNvA=; b=TyPHd0qGQU107xaC09w9D09yYyIgGMcNR/E/UFVco2UFfAz/9aYs+zA0/toS5JEgIoJkOjxv76+1yoiXkdeUfxea+DCw2zVwQ9ywuNZwk+9rm7QGswdlLzD2sEPzv1uODYUeAPBjXG0goFl5EAjGSP9hKe09Qg2KwOwdJu0lSo4= Received: from VE1PR04MB6639.eurprd04.prod.outlook.com (10.255.118.11) by VE1PR04MB6479.eurprd04.prod.outlook.com (20.179.232.225) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2305.15; Thu, 3 Oct 2019 13:24:30 +0000 Received: from VE1PR04MB6639.eurprd04.prod.outlook.com ([fe80::c045:5df2:ba1f:c3ee]) by VE1PR04MB6639.eurprd04.prod.outlook.com ([fe80::c045:5df2:ba1f:c3ee%5]) with mapi id 15.20.2305.023; Thu, 3 Oct 2019 13:24:30 +0000 From: Akhil Goyal To: "Ananyev, Konstantin" , "'dev@dpdk.org'" , "De Lara Guarch, Pablo" , 'Thomas Monjalon' CC: "Zhang, Roy Fan" , "Doherty, Declan" , 'Anoob Joseph' Thread-Topic: [RFC PATCH 1/9] security: introduce CPU Crypto action type and API Thread-Index: AQHVYm4LqyJkewM9NkuUWAfAmrqx1acbUiZggAAsN4CAAtsIgIAAT02AgAYXC5CAAbSDgIABbRGggAaWxgCAAPjG4IABs/OAgAuzNYCAAoY34IAE8G8AgAAH4mCAAbN9gIADA/pw Date: Thu, 3 Oct 2019 13:24:30 +0000 Message-ID: References: <20190903154046.55992-1-roy.fan.zhang@intel.com> <20190903154046.55992-2-roy.fan.zhang@intel.com> <9F7182E3F746AB4EA17801C148F3C6043369D686@IRSMSX101.ger.corp.intel.com> <2601191342CEEE43887BDE71AB9772580191926A17@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB9772580191962CD5@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB9772580191966116@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB9772580191966C23@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB977258019196A767@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB977258019196D53D@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB977258019196F386@irsmsx105.ger.corp.intel.com> In-Reply-To: <2601191342CEEE43887BDE71AB977258019196F386@irsmsx105.ger.corp.intel.com> Accept-Language: en-IN, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: spf=none (sender IP is ) smtp.mailfrom=akhil.goyal@nxp.com; x-originating-ip: [92.120.1.65] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: e275fe2a-f4e3-4e11-ccc8-08d7480505e1 x-ms-office365-filtering-ht: Tenant x-ms-traffictypediagnostic: VE1PR04MB6479: x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:5797; x-forefront-prvs: 01792087B6 x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(4636009)(136003)(39860400002)(376002)(346002)(396003)(366004)(52314003)(51444003)(189003)(199004)(14454004)(256004)(33656002)(7736002)(86362001)(4326008)(8936002)(81156014)(76176011)(81166006)(5660300002)(8676002)(14444005)(2906002)(6436002)(11346002)(6246003)(478600001)(446003)(6116002)(186003)(3846002)(54906003)(110136005)(229853002)(561944003)(99286004)(30864003)(7696005)(15650500001)(52536014)(74316002)(486006)(66446008)(64756008)(66556008)(71200400001)(66476007)(55016002)(66946007)(71190400001)(44832011)(25786009)(102836004)(76116006)(316002)(476003)(305945005)(9686003)(6506007)(26005)(66066001)(21314003)(491001)(559001)(579004); DIR:OUT; SFP:1101; SCL:1; SRVR:VE1PR04MB6479; H:VE1PR04MB6639.eurprd04.prod.outlook.com; FPR:; SPF:None; LANG:en; PTR:InfoNoRecords; MX:1; A:1; received-spf: None (protection.outlook.com: nxp.com does not designate permitted sender hosts) x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: Mgp7s+aAiMruJz3/r5eH4xC7y/Ch9yowsck97lyN+EvS393NwgDp6raH96RAtG4u0UVKSvythnnadHkbnrcXyR1ywYDE4/iHKvJIA9AmUzI/+juA8pNSWVbgwRpZbk+U/DTFx6WhkmcIhDsndSFgFUaBGcTgt5PxT8+3ltpOs500TSl+u8wf15lank7xhX5a0xDmpCjkbhze2m0+OcbQ8nlVgZ/R++1HKoSc9eFzcHasCfDw2WokdgzT7J9B9CSgUKhkSsHuEmfpmtgALxpzoevpMaUxq0YtR40ylGRUdA/85+c/lsksE6SDjaXP/RKKt8J+vksdlrA5+uPITPmlqU4jaLr1o+xnmT9uzqxl68Nw2LTWvt9rjNg4z+09RVWNOu75Q7GFcS3XIICpsj7pbkwiHAoNxaKUY7iKrF8o90A= x-ms-exchange-transport-forked: True Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-OriginatorOrg: nxp.com X-MS-Exchange-CrossTenant-Network-Message-Id: e275fe2a-f4e3-4e11-ccc8-08d7480505e1 X-MS-Exchange-CrossTenant-originalarrivaltime: 03 Oct 2019 13:24:30.5116 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635 X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: rHzSKVL8Wy3WygIuU48C2osz8/5yABcuqI8PQaPXHpiWcxZtEuH6bDzrnvzVx6VO2UdEqRhthFefMnq3uGDd/g== X-MS-Exchange-Transport-CrossTenantHeadersStamped: VE1PR04MB6479 Subject: Re: [dpdk-dev] [RFC PATCH 1/9] security: introduce CPU Crypto action type and API 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: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi Konstantin, >=20 > Hi Akhil, >=20 > > > > > > > > > > > > > > This action type allows the burst of symmetric = crypto > > > workload > > > > > using > > > > > > > > the > > > > > > > > > > > > same > > > > > > > > > > > > > > algorithm, key, and direction being processed b= y CPU > cycles > > > > > > > > > > synchronously. > > > > > > > > > > > > > > This flexible action type does not require exte= rnal > hardware > > > > > > > > involvement, > > > > > > > > > > > > > > having the crypto workload processed synchronou= sly, > and is > > > > > more > > > > > > > > > > > > performant > > > > > > > > > > > > > > than Cryptodev SW PMD due to the saved cycles o= n > removed > > > > > "async > > > > > > > > > > mode > > > > > > > > > > > > > > simulation" as well as 3 cacheline access of th= e crypto > ops. > > > > > > > > > > > > > > > > > > > > > > > > > > Does that mean application will not call the > > > > > cryptodev_enqueue_burst > > > > > > > > and > > > > > > > > > > > > corresponding dequeue burst. > > > > > > > > > > > > > > > > > > > > > > > > Yes, instead it just call > rte_security_process_cpu_crypto_bulk(...) > > > > > > > > > > > > > > > > > > > > > > > > > It would be a new API something like process_pack= ets and > it > > > will > > > > > have > > > > > > > > the > > > > > > > > > > > > crypto processed packets while returning from the A= PI? > > > > > > > > > > > > > > > > > > > > > > > > Yes, though the plan is that API will operate on ra= w data > buffers, > > > > > not > > > > > > > > mbufs. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I still do not understand why we cannot do with t= he > > > conventional > > > > > > > > crypto lib > > > > > > > > > > > > only. > > > > > > > > > > > > > As far as I can understand, you are not doing any= protocol > > > > > processing > > > > > > > > or > > > > > > > > > > any > > > > > > > > > > > > value add > > > > > > > > > > > > > To the crypto processing. IMO, you just need a > synchronous > > > > > crypto > > > > > > > > > > processing > > > > > > > > > > > > API which > > > > > > > > > > > > > Can be defined in cryptodev, you don't need to re= -create a > > > crypto > > > > > > > > session > > > > > > > > > > in > > > > > > > > > > > > the name of > > > > > > > > > > > > > Security session in the driver just to do a synch= ronous > > > processing. > > > > > > > > > > > > > > > > > > > > > > > > I suppose your question is why not to have > > > > > > > > > > > > rte_crypot_process_cpu_crypto_bulk(...) instead? > > > > > > > > > > > > The main reason is that would require disruptive ch= anges in > > > existing > > > > > > > > > > cryptodev > > > > > > > > > > > > API > > > > > > > > > > > > (would cause ABI/API breakage). > > > > > > > > > > > > Session for RTE_SECURITY_ACTION_TYPE_CPU_CRYPTO > need > > > > > some > > > > > > > > extra > > > > > > > > > > > > information > > > > > > > > > > > > that normal crypto_sym_xform doesn't contain > > > > > > > > > > > > (cipher offset from the start of the buffer, might = be > something > > > extra > > > > > in > > > > > > > > > > future). > > > > > > > > > > > > > > > > > > > > > > Cipher offset will be part of rte_crypto_op. > > > > > > > > > > > > > > > > > > > > fill/read (+ alloc/free) is one of the main things that= slowdown > > > current > > > > > > > > crypto-op > > > > > > > > > > approach. > > > > > > > > > > That's why the general idea - have all data that wouldn= 't change > > > from > > > > > packet > > > > > > > > to > > > > > > > > > > packet > > > > > > > > > > included into the session and setup it once at session_= init(). > > > > > > > > > > > > > > > > > > I agree that you cannot use crypto-op. > > > > > > > > > You can have the new API in crypto. > > > > > > > > > As per the current patch, you only need cipher_offset whi= ch you > can > > > have > > > > > it as > > > > > > > > a parameter until > > > > > > > > > You get it approved in the crypto xform. I believe it wil= l be > beneficial > > > in > > > > > case of > > > > > > > > other crypto cases as well. > > > > > > > > > We can have cipher offset at both places(crypto-op and > > > cipher_xform). It > > > > > will > > > > > > > > give flexibility to the user to > > > > > > > > > override it. > > > > > > > > > > > > > > > > After having another thought on your proposal: > > > > > > > > Probably we can introduce new rte_crypto_sym_xform_types fo= r > CPU > > > > > related > > > > > > > > stuff here? > > > > > > > > > > > > > > I also thought of adding new xforms, but that wont serve the = purpose > for > > > > > may be all the cases. > > > > > > > You would be needing all information currently available in t= he > current > > > > > xforms. > > > > > > > So if you are adding new fields in the new xform, the size wi= ll be more > > > than > > > > > that of the union of xforms. > > > > > > > ABI breakage would still be there. > > > > > > > > > > > > > > If you think a valid compression of the AEAD xform can be don= e, then > > > that > > > > > can be done for each of the > > > > > > > Xforms and we can have a solution to this issue. > > > > > > > > > > > > I think that we can re-use iv.offset for our purposes (for cryp= to offset). > > > > > > So for now we can make that path work without any ABI breakage. > > > > > > Fan, please feel free to correct me here, if I missed something= . > > > > > > If in future we would need to add some extra information it mig= ht > > > > > > require ABI breakage, though by now I don't envision anything > particular to > > > > > add. > > > > > > Anyway, if there is no objection to go that way, we can try to = make > > > > > > these changes for v2. > > > > > > > > > > > > > > > > Actually, after looking at it more deeply it appears not that eas= y as I > thought > > > it > > > > > would be :) > > > > > Below is a very draft version of proposed API additions. > > > > > I think it avoids ABI breakages right now and provides enough fle= xibility > for > > > > > future extensions (if any). > > > > > For now, it doesn't address your comments about naming convention= s > > > (_CPU_ > > > > > vs _SYNC_) , etc. > > > > > but I suppose is comprehensive enough to provide a main idea beyo= nd it. > > > > > Akhil and other interested parties, please try to review and prov= ide > feedback > > > > > ASAP, > > > > > as related changes would take some time and we still like to hit = 19.11 > > > deadline. > > > > > Konstantin > > > > > > > > > > diff --git a/lib/librte_cryptodev/rte_crypto_sym.h > > > > > b/lib/librte_cryptodev/rte_crypto_sym.h > > > > > index bc8da2466..c03069e23 100644 > > > > > --- a/lib/librte_cryptodev/rte_crypto_sym.h > > > > > +++ b/lib/librte_cryptodev/rte_crypto_sym.h > > > > > @@ -103,6 +103,9 @@ rte_crypto_cipher_operation_strings[]; > > > > > * > > > > > * This structure contains data relating to Cipher (Encryption a= nd > Decryption) > > > > > * use to create a session. > > > > > + * Actually I was wrong saying that we don't have free space ins= ide > xforms. > > > > > + * Making key struct packed (see below) allow us to regain 6B th= at could > be > > > > > + * used for future extensions. > > > > > */ > > > > > struct rte_crypto_cipher_xform { > > > > > enum rte_crypto_cipher_operation op; > > > > > @@ -116,7 +119,25 @@ struct rte_crypto_cipher_xform { > > > > > struct { > > > > > const uint8_t *data; /**< pointer to key data = */ > > > > > uint16_t length; /**< key length in bytes = */ > > > > > - } key; > > > > > + } __attribute__((__packed__)) key; > > > > > + > > > > > + /** > > > > > + * offset for cipher to start within user provided data = buffer. > > > > > + * Fan suggested another (and less space consuming way) - > > > > > + * reuse iv.offset space below, by changing: > > > > > + * struct {uint16_t offset, length;} iv; > > > > > + * to uunamed union: > > > > > + * union { > > > > > + * struct {uint16_t offset, length;} iv; > > > > > + * struct {uint16_t iv_len, crypto_offset} cpu_crypt= o_param; > > > > > + * }; > > > > > + * Both approaches seems ok to me in general. > > > > > > > > No strong opinions here. OK with this one. > > > > > > > > > + * Comments/suggestions are welcome. > > > > > + */ > > > > > + uint16_t offset; > > > > > > After another thought - it is probably a bit better to have offset as= a separate > > > field. > > > In that case we can use the same xforms to create both type of sessio= ns. > > ok > > > > > > > > + > > > > > + uint8_t reserved1[4]; > > > > > + > > > > > /**< Cipher key > > > > > * > > > > > * For the RTE_CRYPTO_CIPHER_AES_F8 mode of operation, > key.data > > > will > > > > > @@ -284,7 +305,7 @@ struct rte_crypto_auth_xform { > > > > > struct { > > > > > const uint8_t *data; /**< pointer to key data = */ > > > > > uint16_t length; /**< key length in bytes = */ > > > > > - } key; > > > > > + } __attribute__((__packed__)) key; > > > > > /**< Authentication key data. > > > > > * The authentication key length MUST be less than or equ= al to the > > > > > * block size of the algorithm. It is the callers respons= ibility to > > > > > @@ -292,6 +313,8 @@ struct rte_crypto_auth_xform { > > > > > * (for example RFC 2104, FIPS 198a). > > > > > */ > > > > > > > > > > + uint8_t reserved1[6]; > > > > > + > > > > > struct { > > > > > uint16_t offset; > > > > > /**< Starting point for Initialisation Vector or = Counter, > > > > > @@ -376,7 +399,12 @@ struct rte_crypto_aead_xform { > > > > > struct { > > > > > const uint8_t *data; /**< pointer to key data = */ > > > > > uint16_t length; /**< key length in bytes = */ > > > > > - } key; > > > > > + } __attribute__((__packed__)) key; > > > > > + > > > > > + /** offset for cipher to start within data buffer */ > > > > > + uint16_t cipher_offset; > > > > > + > > > > > + uint8_t reserved1[4]; > > > > > > > > > > struct { > > > > > uint16_t offset; > > > > > diff --git a/lib/librte_cryptodev/rte_cryptodev.h > > > > > b/lib/librte_cryptodev/rte_cryptodev.h > > > > > index e175b838c..c0c7bfed7 100644 > > > > > --- a/lib/librte_cryptodev/rte_cryptodev.h > > > > > +++ b/lib/librte_cryptodev/rte_cryptodev.h > > > > > @@ -1272,6 +1272,101 @@ void * > > > > > rte_cryptodev_sym_session_get_user_data( > > > > > struct rte_cryptodev_sym_= session *sess); > > > > > > > > > > +/* > > > > > + * After several thoughts decided not to try to squeeze CPU_CRYP= TO > > > > > + * into existing rte_crypto_sym_session structure/API, but inste= ad > > > > > + * introduce an extentsion to it via new fully opaque > > > > > + * struct rte_crypto_cpu_sym_session and additional related API. > > > > > > > > > > > > What all things do we need to squeeze? > > > > In this proposal I do not see the new struct cpu_sym_session defin= ed here. > > > > > > The plan is to have it totally opaque to the user, i.e. just: > > > struct rte_crypto_cpu_sym_session; > > > in public header files. > > > > > > > I believe you will have same lib API/struct for cpu_sym_session an= d > > > sym_session. > > > > > > I thought about such way, but there are few things that looks clumsy = to me: > > > 1. Right now there is no 'type' (or so) field inside rte_cryptodev_sy= m_session, > > > so it is not possible to easy distinguish what session do you have: l= ksd_sym or > > > cpu_sym. > > > In theory, there is a hole of 4B inside rte_cryptodev_sym_session, so= we can > add > > > some extra field > > > here, but in that case we wouldn't be able to use the same xform for= both > > > lksd_sym or cpu_sym > > > (which seems really plausible thing for me). > > > 2. Majority of rte_cryptodev_sym_session fields I think are unnecess= ary for > > > rte_crypto_cpu_sym_session: > > > sess_data[], opaque_data, user_data, nb_drivers. > > > All that consumes space, that could be used somewhere else instead. > > > 3. I am a bit reluctant to touch existing rte_cryptodev API - to avoi= d any > > > breakages I can't foresee right now. > > > From other side - if we'll add new functions/structs for cpu_sym_sess= ion we > can > > > mark it > > > and keep it for some time as experimental, so further changes (if nee= ded) > would > > > still be possible. > > > > > > > OK let us assume that you have a separate structure. But I have a few q= ueries: > > 1. how can multiple drivers use a same session >=20 > As a short answer: they can't. > It is pretty much the same approach as with rte_security - each device ne= eds to > create/init its own session. > So upper layer would need to maintain its own array (or so) for such case= . > Though the question is why would you like to have same session over multi= ple > SW backed devices? > As it would be anyway just a synchronous function call that will be execu= ted on > the same cpu. I may have single FAT tunnel which may be distributed over multiple Cores, and each core is affined to a different SW device. So a single session may be accessed by multiple devices. One more example would be depending on packet sizes, I may switch between HW/SW PMDs with the same session. >=20 > > 2. Can somebody use the scheduler pmd for scheduling the different type= of > payloads for the same session? >=20 > In theory yes. > Though for that scheduler pmd should have inside it's > rte_crypto_cpu_sym_session an array of pointers to > the underlying devices sessions. >=20 > > > > With your proposal the APIs would be very specific to your use case onl= y. >=20 > Yes in some way. > I consider that API specific for SW backed crypto PMDs. > I can hardly see how any 'real HW' PMDs (lksd-none, lksd-proto) will bene= fit > from it. > Current crypto-op API is very much HW oriented. > Which is ok, that's for it was intended for, but I think we also need one= that > would be designed > for SW backed implementation in mind. We may re-use your API for HW PMDs as well which do not have requirement of Crypto-op/mbuf etc. The return type of your new process API may have a status which say 'proces= sed' Or can be say 'enqueued'. So if it is 'enqueued', we may have a new API fo= r raw Bufs dequeue as well. This requirement can be for any hardware PMDs like QAT as well. That is why a dev-ops would be a better option. >=20 > > When you would add more functionality to this sync API/struct, it will = end up > being the same API/struct. > > > > Let us see how close/ far we are from the existing APIs when the actua= l > implementation is done. > > > > > > I am not sure if that would be needed. > > > > It would be internal to the driver that if synchronous processing i= s > > > supported(from feature flag) and > > > > Have relevant fields in xform(the newly added ones which are packed= as > per > > > your suggestions) set, > > > > It will create that type of session. > > > > > > > > > > > > > + * Main points: > > > > > + * - Current crypto-dev API is reasonably mature and it is desir= able > > > > > + * to keep it unchanged (API/ABI stability). From other side, = this > > > > > + * new sync API is new one and probably would require extra ch= anges. > > > > > + * Having it as a new one allows to mark it as experimental, w= ithout > > > > > + * affecting existing one. > > > > > + * - Fully opaque cpu_sym_session structure gives more flexibili= ty > > > > > + * to the PMD writers and again allows to avoid ABI breakages = in future. > > > > > + * - process() function per set of xforms > > > > > + * allows to expose different process() functions for differen= t > > > > > + * xform combinations. PMD writer can decide, does he wants to > > > > > + * push all supported algorithms into one process() function, > > > > > + * or spread it across several ones. > > > > > + * I.E. More flexibility for PMD writer. > > > > > > > > Which process function should be chosen is internal to PMD, how wou= ld > that > > > info > > > > be visible to the application or the library. These will get stored= in the > session > > > private > > > > data. It would be upto the PMD writer, to store the per session pro= cess > > > function in > > > > the session private data. > > > > > > > > Process function would be a dev ops just like enc/deq operations an= d it > should > > > call > > > > The respective process API stored in the session private data. > > > > > > That model (via devops) is possible, but has several drawbacks from m= y > > > perspective: > > > > > > 1. It means we'll need to pass dev_id as a parameter to process() fun= ction. > > > Though in fact dev_id is not a relevant information for us here > > > (all we need is pointer to the session and pointer to the fuction to = call) > > > and I tried to avoid using it in data-path functions for that API. > > > > You have a single vdev, but someone may have multiple vdevs for each th= read, > or may > > Have same dev with multiple queues for each core. >=20 > That's fine. As I said above it is a SW backed implementation. > Each session has to be a separate entity that contains all necessary info= rmation > (keys, alg/mode info, etc.) to process input buffers. > Plus we need the actual function pointer to call. > I just don't see what for we need a dev_id in that situation. To iterate the session private data in the session. > Again, here we don't need care about queues and their pinning to cores. > If let say someone would like to process buffers from the same IPsec SA o= n 2 > different cores in parallel, he can just create 2 sessions for the same x= form, > give one to thread #1 and second to thread #2. > After that both threads are free to call process(this_thread_ses, ...) at= will. Say you have a 16core device to handle 100G of traffic on a single tunnel. Will we make 16 sessions with same parameters? >=20 > > > > > 2. As you pointed in that case it will be just one process() function= per device. > > > So if PMD would like to have several process() functions for differen= t type of > > > sessions > > > (let say one per alg) first thing it has to do inside it's process() = - read session > data > > > and > > > based on that, do a jump/call to particular internal sub-routine. > > > Something like: > > > driver_id =3D get_pmd_driver_id(); > > > priv_ses =3D ses->sess_data[driver_id]; > > > Then either: > > > switch(priv_sess->alg) {case XXX: process_XXX(priv_sess, ...);break;.= ..} > > > OR > > > priv_ses->process(priv_sess, ...); > > > > > > to select and call the proper function. > > > Looks like totally unnecessary overhead to me. > > > Though if we'll have ability to query/extract some sort session_ops b= ased on > the > > > xform - > > > we can avoid this extra de-refererence+jump/call thing. > > > > What is the issue in the priv_ses->process(); approach? >=20 > Nothing at all. > What I am saying that schema with dev_ops > dev[dev_id]->dev_ops.process(ses->priv_ses[driver_id], ...) > | > |-> priv_ses->process(...) >=20 > Has bigger overhead then just: > process(ses,...); >=20 > So what for to introduce extra-level of indirection here? Explained above. >=20 > > I don't understand what are you saving by not doing this. > > In any case you would need to identify which session correspond to whic= h > process(). >=20 > Yes, sure, but I think we can make user to store information that relatio= nship, > in a way he likes: store process() pointer for each session, or group ses= sions > that share the same process() somehow, or... So whatever relationship that user will make and store will make its life c= omplicated. If we can hide that information in the driver, then what is the issue in th= at and user Will not need to worry. He would just call the process() and driver will ch= oose which Process need to be called. I think we should have a POC around this and see the difference in the cycl= e count. IMO it would be negligible and we would end up making a generic API set whi= ch can be used by others as well. >=20 > > For that you would be doing it somewhere in your data path. >=20 > Why at data-path? > Only once at session creation/initialization time. > Or might be even once per group of sessions. >=20 > > > > > > > > > > > > > I am not sure if you would need a new session init API for this as = nothing > would > > > be visible to > > > > the app or lib. > > > > > > > > > + * - Not storing process() pointer inside the session - > > > > > + * Allows user to choose does he want to store a process() poi= nter > > > > > + * per session, or per group of sessions for that device that = share > > > > > + * the same input xforms. I.E. extra flexibility for the user, > > > > > + * plus allows us to keep cpu_sym_session totally opaque, see = above. > > > > > > > > If multiple sessions need to be processed via the same process func= tion, > > > > PMD would save the same process in all the sessions, I don't think = there > would > > > > be any perf overhead with that. > > > > > > I think it would, see above. > > > > > > > > > > > > + * Sketched usage model: > > > > > + * .... > > > > > + * /* control path, alloc/init session */ > > > > > + * int32_t sz =3D rte_crypto_cpu_sym_session_size(dev_id, &xform= ); > > > > > + * struct rte_crypto_cpu_sym_session *ses =3D user_alloc(..., sz= ); > > > > > + * rte_crypto_cpu_sym_process_t process =3D > > > > > + * rte_crypto_cpu_sym_session_func(dev_id, &xform); > > > > > + * rte_crypto_cpu_sym_session_init(dev_id, ses, &xform); > > > > > + * ... > > > > > + * /* data-path*/ > > > > > + * process(ses, ....); > > > > > + * .... > > > > > + * /* control path, termiante/free session */ > > > > > + * rte_crypto_cpu_sym_session_fini(dev_id, ses); > > > > > + */ > > > > > + > > > > > +/** > > > > > + * vector structure, contains pointer to vector array and the le= ngth > > > > > + * of the array > > > > > + */ > > > > > +struct rte_crypto_vec { > > > > > + struct iovec *vec; > > > > > + uint32_t num; > > > > > +}; > > > > > + > > > > > +/* > > > > > + * Data-path bulk process crypto function. > > > > > + */ > > > > > +typedef void (*rte_crypto_cpu_sym_process_t)( > > > > > + struct rte_crypto_cpu_sym_session *sess, > > > > > + struct rte_crypto_vec buf[], void *iv[], void *aa= d[], > > > > > + void *digest[], int status[], uint32_t num); > > > > > +/* > > > > > + * for given device return process function specific to input xf= orms > > > > > + * on error - return NULL and set rte_errno value. > > > > > + * Note that for same input xfroms for the same device should re= turn > > > > > + * the same process function. > > > > > + */ > > > > > +__rte_experimental > > > > > +rte_crypto_cpu_sym_process_t > > > > > +rte_crypto_cpu_sym_session_func(uint8_t dev_id, > > > > > + const struct rte_crypto_sym_xform *xforms= ); > > > > > + > > > > > +/* > > > > > + * Return required session size in bytes for given set of xforms= . > > > > > + * if xforms =3D=3D NULL, then return the max possible session s= ize, > > > > > + * that would fit session for any supported by the device algori= thm. > > > > > + * if CPU mode is not supported at all, or requeted in xform > > > > > + * algorithm is not supported, then return -ENOTSUP. > > > > > + */ > > > > > +__rte_experimental > > > > > +int > > > > > +rte_crypto_cpu_sym_session_size(uint8_t dev_id, > > > > > + const struct rte_crypto_sym_xform *xforms= ); > > > > > + > > > > > +/* > > > > > + * Initialize session. > > > > > + * It is caller responsibility to allocate enough space for it. > > > > > + * See rte_crypto_cpu_sym_session_size above. > > > > > + */ > > > > > +__rte_experimental > > > > > +int rte_crypto_cpu_sym_session_init(uint8_t dev_id, > > > > > + struct rte_crypto_cpu_sym_session *sess, > > > > > + const struct rte_crypto_sym_xform *xforms= ); > > > > > + > > > > > +__rte_experimental > > > > > +void > > > > > +rte_crypto_cpu_sym_session_fini(uint8_t dev_id, > > > > > + struct rte_crypto_cpu_sym_session *sess); > > > > > + > > > > > + > > > > > #ifdef __cplusplus > > > > > } > > > > > #endif > > > > > diff --git a/lib/librte_cryptodev/rte_cryptodev_pmd.h > > > > > b/lib/librte_cryptodev/rte_cryptodev_pmd.h > > > > > index defe05ea0..ed7e63fab 100644 > > > > > --- a/lib/librte_cryptodev/rte_cryptodev_pmd.h > > > > > +++ b/lib/librte_cryptodev/rte_cryptodev_pmd.h > > > > > @@ -310,6 +310,20 @@ typedef void > > > (*cryptodev_sym_free_session_t)(struct > > > > > rte_cryptodev *dev, > > > > > typedef void (*cryptodev_asym_free_session_t)(struct rte_cryptod= ev > *dev, > > > > > struct rte_cryptodev_asym_session *sess); > > > > > > > > > > +typedef int (*cryptodev_cpu_sym_session_size_t) (struct rte_cryp= todev > > > *dev, > > > > > + const struct rte_crypto_sym_xform *xforms= ); > > > > > + > > > > > +typedef int (*cryptodev_cpu_sym_session_init_t) (struct rte_cryp= todev > > > *dev, > > > > > + struct rte_crypto_cpu_sym_session *sess, > > > > > + const struct rte_crypto_sym_xform *xforms= ); > > > > > + > > > > > +typedef void (*cryptodev_cpu_sym_session_fini_t) (struct rte_cry= ptodev > > > *dev, > > > > > + struct rte_crypto_cpu_sym_session *sess); > > > > > + > > > > > +typedef rte_crypto_cpu_sym_process_t > > > (*cryptodev_cpu_sym_session_func_t) > > > > > ( > > > > > + struct rte_cryptodev *dev, > > > > > + const struct rte_crypto_sym_xform *xforms= ); > > > > > + > > > > > /** Crypto device operations function pointer table */ > > > > > struct rte_cryptodev_ops { > > > > > cryptodev_configure_t dev_configure; /**< Configure de= vice. */ > > > > > @@ -343,6 +357,11 @@ struct rte_cryptodev_ops { > > > > > /**< Clear a Crypto sessions private data. */ > > > > > cryptodev_asym_free_session_t asym_session_clear; > > > > > /**< Clear a Crypto sessions private data. */ > > > > > + > > > > > + cryptodev_cpu_sym_session_size_t sym_cpu_session_get_size= ; > > > > > + cryptodev_cpu_sym_session_func_t sym_cpu_session_get_func= ; > > > > > + cryptodev_cpu_sym_session_init_t sym_cpu_session_init; > > > > > + cryptodev_cpu_sym_session_fini_t sym_cpu_session_fini; > > > > > }; > > > > > > > > > > > > > > >