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 D483EA317C for ; Thu, 17 Oct 2019 14:49:26 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id A9B411E954; Thu, 17 Oct 2019 14:49:25 +0200 (CEST) Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id C46E61E94A for ; Thu, 17 Oct 2019 14:49:23 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga006.jf.intel.com ([10.7.209.51]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 17 Oct 2019 05:49:22 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.67,307,1566889200"; d="scan'208";a="200377376" Received: from irsmsx151.ger.corp.intel.com ([163.33.192.59]) by orsmga006.jf.intel.com with ESMTP; 17 Oct 2019 05:49:20 -0700 Received: from irsmsx104.ger.corp.intel.com ([169.254.5.252]) by IRSMSX151.ger.corp.intel.com ([169.254.4.59]) with mapi id 14.03.0439.000; Thu, 17 Oct 2019 13:49:20 +0100 From: "Ananyev, Konstantin" To: "Ananyev, Konstantin" , Akhil Goyal , "'dev@dpdk.org'" , "De Lara Guarch, Pablo" , 'Thomas Monjalon' , "Zhang, Roy Fan" , "Doherty, Declan" CC: 'Anoob Joseph' Thread-Topic: [RFC PATCH 1/9] security: introduce CPU Crypto action type and API Thread-Index: AQHVYm4Y+AedzaNgY0qWMAmu5GwNVqcbQpEAgAArCICAAuADAIAARA4AgAYiKoCAAbQS0IABqquAgAZiqNCAAPA4gIABtviQgAu3KZCAAoImAIAExwxQgAA3zQCAAZ1E0IADFFEAgAZGHRCAAsIfgIAAUayAgAM4hICAB/TasIABfOUw Date: Thu, 17 Oct 2019 12:49:20 +0000 Message-ID: <2601191342CEEE43887BDE71AB97725801A8C6A3D0@IRSMSX104.ger.corp.intel.com> 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> <2601191342CEEE43887BDE71AB977258019197206C@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB977258019197446B@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB97725801A8C69C5E@IRSMSX104.ger.corp.intel.com> In-Reply-To: <2601191342CEEE43887BDE71AB97725801A8C69C5E@IRSMSX104.ger.corp.intel.com> Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiYTVjZjFhMDktNjJkYy00NWZhLTk1YmYtN2U3MGI3Y2Y1MTQyIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiY2VFa1NmQzFMVGl4RlJCaFg1aGNpNXVvcTVEUzBQRlloTnVJbnFKVlNiSUNhZGJYOWZGU2RBYmEwZ2RiY3RkTCJ9 x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.2.0.6 dlp-reaction: no-action x-originating-ip: [163.33.239.180] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 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" >=20 > > > > User can use the same session, that is what I am also insisting, bu= t it may have > > > separate > > > > Session private data. Cryptodev session create API provide that fun= ctionality > > > and we can > > > > Leverage that. > > > > > > rte_cryptodev_sym_session. sess_data[] is indexed by driver_id, which= means > > > we can't use > > > the same rte_cryptodev_sym_session to hold sessions for both sync and= async > > > mode > > > for the same device. Off course we can add a hard requirement that an= y driver > > > that wants to > > > support process() has to create sessions that can handle both proces= s and > > > enqueue/dequeue, > > > but then again what for to create such overhead? > > > > > > BTW, to be honest, I don't consider current rte_cryptodev_sym_session > > > construct for multiple device_ids: > > > __extension__ struct { > > > void *data; > > > uint16_t refcnt; > > > } sess_data[0]; > > > /**< Driver specific session material, variable size */ > > > > > Yes I also feel the same. I was also not in favor of this when it was i= ntroduced. > > Please go ahead and remove this. I have no issues with that. >=20 > If you are not happy with that structure, and admit there are issues with= it, > why do you push for reusing it for cpu-crypto API? > Why not to take step back, take into account current drawbacks > and define something that (hopefully) would suite us better? > Again new API will be experimental for some time, so we'll > have some opportunity to see does it works and if not fix it. >=20 > About removing data[] from existing rte_cryptodev_sym_session - > Personally would like to do that, but the change seems to be too massive. > Definitely not ready for such effort right now. >=20 > > > > > as an advantage. > > > It looks too error prone for me: > > > 1. Simultaneous session initialization/de-initialization for devices = with the same > > > driver_id is not possible. > > > 2. It assumes that all device driver will be loaded before we start t= o create > > > session pools. > > > > > > Right now it seems ok, as no-one requires such functionality, but I d= on't know > > > how it will be in future. > > > For me rte_security session model, where for each security context us= er have to > > > create new session > > > looks much more robust. > > Agreed > > > > > > > > > > > > > BTW, I can see a v2 to this RFC which is still based on security li= brary. > > > > > > Yes, v2 was concentrated on fixing found issues, some code restructur= ing, > > > i.e. - changes that would be needed anyway whatever API aproach we'll= choose. > > > > > > > When do you plan > > > > To submit the patches for crypto based APIs. We have RC1 merge dead= line for > > > this > > > > patchset on 21st Oct. > > > > > > We'd like to start working on it ASAP, but it seems we still have a m= ajor > > > disagreement > > > about how this crypto-dev API should look like. > > > Which makes me think - should we return to our original proposal via > > > rte_security? > > > It still looks to me like clean and straightforward way to enable thi= s new API, > > > and probably wouldn't cause that much controversy. > > > What do you think? > > > > I cannot spend more time discussing on this until RC1 date. I have some= other stuff pending. > > You can send the patches early next week with the approach that I menti= oned or else we > > can discuss this post RC1(which would mean deferring to 20.02). > > > > But moving back to security is not acceptable to me. The code should be= put where it is > > intended and not where it is easy to put. You are not doing any rte_sec= urity stuff. > > >=20 > Ok, then my suggestion: > Let's at least write down all points about crypto-dev approach where we > disagree and then probably try to resolve them one by one.... > If we fail to make an agreement/progress in next week or so, > (and no more reviews from the community) > will have bring that subject to TB meeting to decide. > Sounds fair to you? >=20 > List is below. > Please add/correct me, if I missed something. >=20 > Konstantin >=20 > 1. extra input parameters to create/init rte_(cpu)_sym_session. >=20 > Will leverage existing 6B gap inside rte_crypto_*_xform between 'algo' an= d 'key' fields. > New fields will be optional and would be used by PMD only when cpu-crypto= session is requested. > For lksd-crypto session PMD is free to ignore these fields. > No ABI breakage is required. >=20 > Hopefully no controversy here with #1. >=20 > 2. cpu-crypto create/init. > a) Our suggestion - introduce new API for that: > - rte_crypto_cpu_sym_init() that would init completely opaque rt= e_crypto_cpu_sym_session. > - struct rte_crypto_cpu_sym_session_ops {(*process)(...); (*clear= ); /*whatever else we'll need *'}; > - rte_crypto_cpu_sym_get_ops(const struct rte_crypto_sym_xform *x= forms) > that would return const struct rte_crypto_cpu_sym_session_ops *= based on input xforms. > Advantages: > 1) totally opaque data structure (no ABI breakages in future), PMD writ= er is totally free > with it format and contents. > 2) each session entity is self-contained, user doesn't need to bring alo= ng dev_id etc. > dev_id is needed only at init stage, after that user will use sessi= on ops to perform > all operations on that session (process(), clear(), etc.). > 3) User can decide does he wants to store ops[] pointer on a per session= basis, > or on a per group of same sessions, or... > 4) No mandatory mempools for private sessions. User can allocate memory = for cpu-crypto > session whenever he likes. > Disadvantages: > 5) Extra changes in control path > 6) User has to store session_ops pointer explicitly. After another thought if 2.a.6 is really that big deal we can have small sh= im layer on top: rte_crypto_cpu_sym_session { void *ses; struct rte_crypto_cpu_sym_session_o= ps * const ops; } OR even rte_crypto_cpu_sym_session { void *ses; struct rte_crypto_cpu_sym_session_o= ps ops; } And merge rte_crypto_cpu_sym_init() and rte_crypto_cpu_sym_get_ops() into o= ne (init). Then process() can become a wrapper: rte_crypto_cpu_sym_process(ses, ...) {return ses->ops->process(ses->ses, ..= .);} OR rte_crypto_cpu_sym_process(ses, ...) {return ses->ops.process(ses->ses, ...= );} if that would help to reach consensus - works for me.=20 > b) Your suggestion - reuse existing rte_cryptodev_sym_session_init()= and existing rte_cryptodev_sym_session > structure. > Advantages: > 1) allows to reuse same struct and init/create/clear() functions. > Probably less changes in control path. > Disadvantages: > 2) rte_cryptodev_sym_session. sess_data[] is indexed by driver_id, which= means that > we can't use the same rte_cryptodev_sym_session to hold private sess= ions pointers > for both sync and async mode for the same device. > So wthe only option we have - make PMD devops->sym_ses= sion_configure() > always create a session that can work in both cpu and lksd modes. > For some implementations that would probably mean that under the hoo= d PMD would create > 2 different session structs (sync/async) and then use one or another= depending on from what API been called. > Seems doable, but ...: > - will contradict with statement from 1: > " New fields will be optional and would be used by PMD only when c= pu-crypto session is requested." > Now it becomes mandatory for all apps to specify cp= u-crypto related parameters too, > even if they don't plan to use that mode - i.e. behavior change, = existing app change. > - might cause extra space overhead. > 3) not possible to store device (not driver) specific data within the se= ssion, but I think it is not really needed right now. > So probably minor compared to 2.b.2. >=20 > Actually #3 follows from #2, but decided to have them separated. >=20 > 3. process() parameters/behavior > a) Our suggestion: user stores ptr to session ops (or to (*process) i= tself) and just does: > session_ops->process(sess, ...); > Advantages: > 1) fastest possible execution path > 2) no need to carry on dev_id for data-path > Disadvantages: > 3) user has to carry on session_ops pointer explicitly > b) Your suggestion: add (*cpu_process) inside rte_cryptodev_ops and = then: > rte_crypto_cpu_sym_process(uint8_t dev_id, rte_cryptodev_sym_sess= ion *sess, /*data parameters*/) {... > rte_cryptodevs[dev_id].dev_ops->cpu_process(ses, ...= ); > /*and then inside PMD specifc process: */ > pmd_private_session =3D sess->sess_data[this_pmd_dri= ver_id].data; > /* and then most likely either */ > pmd_private_session->process(pmd_private_session, ..= .); > /* or jump based on session/input data */ > Advantages: > 1) don't see any... > Disadvantages: > 2) User has to carry on dev_id inside data-path > 3) Extra level of indirection (plus data dependency) - both for data and= instructions. > Possible slowdown compared to a) (not measured). >=20