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 ECE73A2EFC for ; Mon, 16 Sep 2019 16:53:27 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 5659B1BFE3; Mon, 16 Sep 2019 16:53:26 +0200 (CEST) Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id 770961BF75 for ; Mon, 16 Sep 2019 16:53:24 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 16 Sep 2019 07:53:23 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,513,1559545200"; d="scan'208";a="188631189" Received: from irsmsx106.ger.corp.intel.com ([163.33.3.31]) by orsmga003.jf.intel.com with ESMTP; 16 Sep 2019 07:53:20 -0700 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.164]) by IRSMSX106.ger.corp.intel.com ([169.254.8.184]) with mapi id 14.03.0439.000; Mon, 16 Sep 2019 15:53:19 +0100 From: "Ananyev, Konstantin" To: Akhil Goyal , "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: AQHVYm4Y+AedzaNgY0qWMAmu5GwNVqcbQpEAgAArCICAAuADAIAARA4AgAYiKoCAAbQS0IABqquAgAZiqNA= Date: Mon, 16 Sep 2019 14:53:18 +0000 Message-ID: <2601191342CEEE43887BDE71AB9772580191966116@irsmsx105.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> 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: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMmQ4Y2Y4NmUtOTg1Ny00MGNmLThiNzUtNjMwYjIyMDlhNzQ3IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiYmdTK05KQ2FySUxFZWdBcDZCQkN6eXcwa3RiTHZEMjhRZ0RxcHBpZVNrdW80MVwvQXdmMEtqb3BrUE1hV09nTTQifQ== x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.2.0.6 dlp-reaction: no-action x-originating-ip: [163.33.239.182] 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, > > > > > > This action type allows the burst of symmetric crypto workload = using the > > > > same > > > > > > algorithm, key, and direction being processed by CPU cycles > > synchronously. > > > > > > This flexible action type does not require external hardware in= volvement, > > > > > > having the crypto workload processed synchronously, and is more > > > > performant > > > > > > than Cryptodev SW PMD due to the saved cycles on removed "async > > mode > > > > > > simulation" as well as 3 cacheline access of the crypto ops. > > > > > > > > > > Does that mean application will not call the cryptodev_enqueue_bu= rst and > > > > corresponding dequeue burst. > > > > > > > > Yes, instead it just call rte_security_process_cpu_crypto_bulk(...) > > > > > > > > > It would be a new API something like process_packets and it will = have the > > > > crypto processed packets while returning from the API? > > > > > > > > Yes, though the plan is that API will operate on raw data buffers, = not mbufs. > > > > > > > > > > > > > > I still do not understand why we cannot do with the conventional = crypto lib > > > > only. > > > > > As far as I can understand, you are not doing any protocol proces= sing 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 synchronous processin= g. > > > > > > > > I suppose your question is why not to have > > > > rte_crypot_process_cpu_crypto_bulk(...) instead? > > > > The main reason is that would require disruptive changes in existin= g > > 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 ext= ra in > > future). > > > > > > Cipher offset will be part of rte_crypto_op. > > > > fill/read (+ alloc/free) is one of the main things that slowdown curren= t crypto-op > > approach. > > That's why the general idea - have all data that wouldn't change from p= acket to > > packet > > included into the session and setup it once at session_init(). >=20 > 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 which you can have = it as a parameter until > You get it approved in the crypto xform. I believe it will 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:=20 Probably we can introduce new rte_crypto_sym_xform_types for CPU related st= uff here? Let say we can have : num rte_crypto_sym_xform_type { RTE_CRYPTO_SYM_XFORM_NOT_SPECIFIED =3D 0, /**< No xform specified *= / RTE_CRYPTO_SYM_XFORM_AUTH, /**< Authentication xform *= / RTE_CRYPTO_SYM_XFORM_CIPHER, /**< Cipher xform */ RTE_CRYPTO_SYM_XFORM_AEAD /**< AEAD xform */ + RTE_CRYPTO_SYM_XFORM_CPU =3D INT32_MIN, + RTE_CRYPTO_SYM_XFORM_CPU_AEAD =3D (RTE_CRYPTO_SYM_XFORM_CPU | RTE_CRYP= TO_SYM_XFORM_CPU), /* same for auth and crypto xforms */ }; Then we either can re-define some values in struct rte_crypto_aead_xform (v= ia unions), or even have new struct rte_crypto_cpu_aead_xform (same for crypto and aut= h xforms). Then if PMD wants to support new sync API it would need to recognize new xf= orm types and internally it might end up with different session structure (one for s= ync, another for async mode). That I think should allow us to introduce cpu_crypto as part of crypto-dev = API without ABI breakage. What do you think? Konstantin=20 =20 >=20 > > > > > If you intend not to use rte_crypto_op > > > You can pass this as an argument in the new cryptodev API. > > > > You mean extra parameter in rte_security_process_cpu_crypto_bulk()? > > It can be in theory, but that solution looks a bit ugly: > > why to pass for each call something that would be constant per session= ? > > Again having that value constant per session might allow some extra > > optimisations > > That would be hard to achieve for dynamic case. > > and not extendable: > > Suppose tomorrow will need to add something extra (some new algorithm > > support or so). > > With what you proposing will need to new parameter to the function, > > which means API breakage. > > > > > Something extra will also cause ABI breakage in security as well. > > > So it will be same. > > > > I don't think it would. > > AFAIK, right now this patch doesn't introduce any API/ABI breakage. > > Iinside struct rte_security_session_conf we have a union of xforms > > depending on session type. > > So as long as cpu_crypto_xform wouldn't exceed sizes of other xform - > > I believe no ABI breakage will appear. > Agreed, it will not break ABI in case of security till we do not exceed c= urrent size. >=20 > Saving an ABI/API breakage is more important or placing the code at the c= orrect place. > We need to find a tradeoff. Others can comment on this. > @Thomas Monjalon, @De Lara Guarch, Pablo Any comments? >=20 > > > > > > > > > > > Also right now there is no way to add new type of crypto_sym_sessio= n > > without > > > > either breaking existing crypto-dev ABI/API or introducing new stru= cture > > > > (rte_crypto_sym_cpu_session or so) for that. > > > > > > What extra info is required in rte_cryptodev_sym_session to get the > > rte_crypto_sym_cpu_session. > > > > Right now - just cipher_offset (see above). > > What else in future (if any) - don't know. > > > > > I don't think there is any. > > > I believe the same crypto session will be able to work synchronously = as well. > > > > Exactly the same - problematically, see above. > > > > > We would only need a new API to perform synchronous actions. > > > That will reduce the duplication code significantly > > > in the driver to support 2 different kind of APIs with similar code i= nside. > > > Please correct me in case I am missing something. > > > > To add new API into crypto-dev would also require changes in the PMD, > > it wouldn't come totally free and I believe would require roughly the s= ame > > amount of changes. >=20 > It will be required only in the PMDs which support it and would be minima= l. > You would need a feature flag, support for that synchronous API. Session= information will > already be there in the session. The changes wrt cipher_offset need to be= added > but with some default value to identify override will be done or not. >=20 > > > > > > > > > > > > While rte_security is designed in a way that we can add new session= types > > and > > > > related parameters without causing API/ABI breakage. > > > > > > Yes the intent is to add new sessions based on various protocols that= can be > > supported by the driver. > > > > Various protocols and different types of sessions (and devices they bel= ong to). > > Let say right now we have INLINE_CRYPTO, INLINE_PROTO, LOOKASIDE_PROTO, > > etc. > > Here we introduce new type of session. >=20 > What is the new value add to the existing sessions. The changes that we a= re doing > here is just to avoid an API/ABI breakage. The synchronous processing can= happen on both > crypto and security session. This would mean, only the processing API sho= uld be defined, > rest all should be already there in the sessions. > In All other cases, INLINE - eth device was not having any format to perf= orm crypto op > LOOKASIDE - PROTO - add protocol specific sessions which is not available= in crypto. >=20 > > > > > It is not that we should find it as an alternative to cryptodev and u= sing it just > > because it will not cause > > > ABI/API breakage. > > > > I am considering this new API as an alternative to existing ones, but a= s an > > extension. > > Existing crypto-op API has its own advantages (generic), and I think we= should > > keep it supported by all crypto-devs. > > From other side rte_security is an extendable framework that suits the = purpose: > > allows easily (and yes without ABI breakage) introduce new API for spec= ial type > > of crypto-dev (SW based). > > > > >=20 > Adding a synchronous processing API is understandable and can be added in= both > Crypto as well as Security, but a new action type for it is not required. > Now whether to support that, we have ABI/API breakage, that is a differen= t issue. > And we may have to deal with it if no other option is there. >=20 > > > > > > > > > IMO the code should be placed where its intent is. > > > > > > > > > > > BTW, what is your concern with proposed approach (via rte_security)= ? > > > > From my perspective it is a lightweight change and it is totally op= tional > > > > for the crypto PMDs to support it or not. > > > > Konstantin > > > > > > > > > > > > > > > > AESNI-GCM and AESNI-MB PMDs are updated with this support. Ther= e is > > a > > > > small > > > > > > performance test app under app/test/security_aesni_gcm(mb)_perf= test > > to > > > > > > prove. > > > > > > > > > > > > For the new API > > > > > > The packet is sent to the crypto device for symmetric crypto > > > > > > processing. The device will encrypt or decrypt the buffer based= on the > > > > session > > > > > > data specified and preprocessed in the security session. Differ= ent > > > > > > than the inline or lookaside modes, when the function exits, th= e user will > > > > > > expect the buffers are either processed successfully, or having= the error > > > > number > > > > > > assigned to the appropriate index of the status array. > > > > > > > > > > > > Will update the program's guide in the v1 patch. > > > > > > > > > > > > Regards, > > > > > > Fan > > > > > > > > > > > > > -----Original Message----- > > > > > > > From: Akhil Goyal [mailto:akhil.goyal@nxp.com] > > > > > > > Sent: Wednesday, September 4, 2019 11:33 AM > > > > > > > To: Zhang, Roy Fan ; dev@dpdk.org > > > > > > > Cc: Ananyev, Konstantin ; Doher= ty, > > > > Declan > > > > > > > ; De Lara Guarch, Pablo > > > > > > > > > > > > > > Subject: RE: [RFC PATCH 1/9] security: introduce CPU Crypto a= ction > > type > > > > and > > > > > > > API > > > > > > > > > > > > > > Hi Fan, > > > > > > > > > > > > > > > > > > > > > > > This patch introduce new > > RTE_SECURITY_ACTION_TYPE_CPU_CRYPTO > > > > > > > action > > > > > > > > type to security library. The type represents performing cr= ypto > > > > > > > > operation with CPU cycles. The patch also includes a new AP= I to > > > > > > > > process crypto operations in bulk and the function pointers= for PMDs. > > > > > > > > > > > > > > > I am not able to get the flow of execution for this action ty= pe. Could > > you > > > > > > > please elaborate the flow in the documentation. If not in > > documentation > > > > > > > right now, then please elaborate the flow in cover letter. > > > > > > > Also I see that there are new APIs for processing crypto oper= ations in > > bulk. > > > > > > > What does that mean. How are they different from the existing= APIs > > which > > > > > > > are also handling bulk crypto ops depending on the budget. > > > > > > > > > > > > > > > > > > > > > -Akhil