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 2A03AA3168 for ; Thu, 17 Oct 2019 00:07:20 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id B66EC1C027; Thu, 17 Oct 2019 00:07:19 +0200 (CEST) Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by dpdk.org (Postfix) with ESMTP id 6B9F81BFF7 for ; Thu, 17 Oct 2019 00:07:17 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga107.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 16 Oct 2019 15:07:16 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.67,305,1566889200"; d="scan'208";a="370940584" Received: from irsmsx107.ger.corp.intel.com ([163.33.3.99]) by orsmga005.jf.intel.com with ESMTP; 16 Oct 2019 15:07:14 -0700 Received: from irsmsx104.ger.corp.intel.com ([169.254.5.252]) by IRSMSX107.ger.corp.intel.com ([169.254.10.18]) with mapi id 14.03.0439.000; Wed, 16 Oct 2019 23:07:14 +0100 From: "Ananyev, Konstantin" To: 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/TasA== Date: Wed, 16 Oct 2019 22:07:13 +0000 Message-ID: <2601191342CEEE43887BDE71AB97725801A8C69C5E@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> In-Reply-To: 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" Hi Akhil, > > > User can use the same session, that is what I am also insisting, but = it may have > > separate > > > Session private data. Cryptodev session create API provide that funct= ionality > > and we can > > > Leverage that. > > > > rte_cryptodev_sym_session. sess_data[] is indexed by driver_id, which m= eans > > we can't use > > the same rte_cryptodev_sym_session to hold sessions for both sync and a= sync > > mode > > for the same device. Off course we can add a hard requirement that any = driver > > that wants to > > support process() has to create sessions that can handle both process = 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 int= roduced. > Please go ahead and remove this. I have no issues with that. If you are not happy with that structure, and admit there are issues with i= t, why do you push for reusing it for cpu-crypto API? =20 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 -=20 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 wi= th the same > > driver_id is not possible. > > 2. It assumes that all device driver will be loaded before we start to = create > > session pools. > > > > Right now it seems ok, as no-one requires such functionality, but I don= 't know > > how it will be in future. > > For me rte_security session model, where for each security context user= have to > > create new session > > looks much more robust. > Agreed >=20 > > > > > > > > BTW, I can see a v2 to this RFC which is still based on security libr= ary. > > > > Yes, v2 was concentrated on fixing found issues, some code restructurin= g, > > i.e. - changes that would be needed anyway whatever API aproach we'll c= hoose. > > > > > When do you plan > > > To submit the patches for crypto based APIs. We have RC1 merge deadli= ne for > > this > > > patchset on 21st Oct. > > > > We'd like to start working on it ASAP, but it seems we still have a maj= or > > 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 this = new API, > > and probably wouldn't cause that much controversy. > > What do you think? >=20 > I cannot spend more time discussing on this until RC1 date. I have some o= ther stuff pending. > You can send the patches early next week with the approach that I mention= ed or else we > can discuss this post RC1(which would mean deferring to 20.02). >=20 > But moving back to security is not acceptable to me. The code should be p= ut where it is > intended and not where it is easy to put. You are not doing any rte_secur= ity 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)=20 will have bring that subject to TB meeting to decide. Sounds fair to you? List is below. Please add/correct me, if I missed something. Konstantin 1. extra input parameters to create/init rte_(cpu)_sym_session. Will leverage existing 6B gap inside rte_crypto_*_xform between 'algo' and = 'key' fields. New fields will be optional and would be used by PMD only when cpu-crypto s= ession is requested. For lksd-crypto session PMD is free to ignore these fields. =20 No ABI breakage is required.=20 Hopefully no controversy here with #1. 2. cpu-crypto create/init. a) Our suggestion - introduce new API for that: - rte_crypto_cpu_sym_init() that would init completely opaque rte_= 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 *xfo= rms) that would return const struct rte_crypto_cpu_sym_session_ops *ba= sed on input xforms. Advantages: 1) totally opaque data structure (no ABI breakages in future), PMD writer= is totally free with it format and contents.=20 2) each session entity is self-contained, user doesn't need to bring along= dev_id etc. dev_id is needed only at init stage, after that user will use session= 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 b= asis, or on a per group of same sessions, or... 4) No mandatory mempools for private sessions. User can allocate memory fo= r cpu-crypto session whenever he likes. Disadvantages: 5) Extra changes in control path 6) User has to store session_ops pointer explicitly. b) Your suggestion - reuse existing rte_cryptodev_sym_session_init() a= nd 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 m= eans that=20 we can't use the same rte_cryptodev_sym_session to hold private sessio= ns pointers for both sync and async mode for the same device. So wthe only option we have - make PMD devops->sym_sessi= on_configure() always create a session that can work in both cpu and lksd modes. For some implementations that would probably mean that under the hood = PMD would create 2 different session structs (sync/async) and then use one or another d= epending on from what API been called. Seems doable, but ...: - will contradict with statement from 1:=20 " New fields will be optional and would be used by PMD only when cpu= -crypto session is requested." Now it becomes mandatory for all apps to specify cpu-= crypto related parameters too, even if they don't plan to use that mode - i.e. behavior change, ex= isting app change. - might cause extra space overhead. 3) not possible to store device (not driver) specific data within the sess= ion, but I think it is not really needed right now. =20 So probably minor compared to 2.b.2. Actually #3 follows from #2, but decided to have them separated. 3. process() parameters/behavior a) Our suggestion: user stores ptr to session ops (or to (*process) its= elf) 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 th= en: rte_crypto_cpu_sym_process(uint8_t dev_id, rte_cryptodev_sym_sessio= n *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_drive= r_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 i= nstructions. Possible slowdown compared to a) (not measured).=20 =20