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 02503A2EFC for ; Mon, 16 Sep 2019 17:08:45 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 9BFDF1BF75; Mon, 16 Sep 2019 17:08:44 +0200 (CEST) Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by dpdk.org (Postfix) with ESMTP id C3C631BF2D for ; Mon, 16 Sep 2019 17:08:41 +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 orsmga106.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 16 Sep 2019 08:08:39 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,513,1559545200"; d="scan'208";a="361551322" Received: from irsmsx109.ger.corp.intel.com ([163.33.3.23]) by orsmga005.jf.intel.com with ESMTP; 16 Sep 2019 08:08:37 -0700 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.164]) by IRSMSX109.ger.corp.intel.com ([169.254.13.29]) with mapi id 14.03.0439.000; Mon, 16 Sep 2019 16:08:36 +0100 From: "Ananyev, Konstantin" To: "Ananyev, Konstantin" , 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+AedzaNgY0qWMAmu5GwNVqcbQpEAgAArCICAAuADAIAARA4AgAYiKoCAAbQS0IABqquAgAZiqNCAAAa3AA== Date: Mon, 16 Sep 2019 15:08:35 +0000 Message-ID: <2601191342CEEE43887BDE71AB9772580191966138@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> <2601191342CEEE43887BDE71AB9772580191966116@irsmsx105.ger.corp.intel.com> In-Reply-To: <2601191342CEEE43887BDE71AB9772580191966116@irsmsx105.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: 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, >=20 > > > > > > > This action type allows the burst of symmetric crypto workloa= d using the > > > > > same > > > > > > > algorithm, key, and direction being processed by CPU cycles > > > synchronously. > > > > > > > This flexible action type does not require external hardware = involvement, > > > > > > > having the crypto workload processed synchronously, and is mo= re > > > > > performant > > > > > > > than Cryptodev SW PMD due to the saved cycles on removed "asy= nc > > > mode > > > > > > > simulation" as well as 3 cacheline access of the 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_packets and it wil= l 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 conventiona= l crypto lib > > > > > only. > > > > > > As far as I can understand, you are not doing any protocol proc= essing or > > > any > > > > > value add > > > > > > To the crypto processing. IMO, you just need a synchronous cryp= to > > > processing > > > > > API which > > > > > > Can be defined in cryptodev, you don't need to re-create a cryp= to session > > > in > > > > > the name of > > > > > > Security session in the driver just to do a synchronous process= ing. > > > > > > > > > > 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 exist= ing > > > 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 e= xtra in > > > future). > > > > > > > > Cipher offset will be part of rte_crypto_op. > > > > > > fill/read (+ alloc/free) is one of the main things that slowdown curr= ent 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 which you can hav= e it as a parameter until > > You get it approved in the crypto xform. I believe it will be beneficia= l in case of other crypto cases as well. > > We can have cipher offset at both places(crypto-op and cipher_xform). I= t will give flexibility to the user to > > override it. >=20 > After having another thought on your proposal: > Probably we can introduce new rte_crypto_sym_xform_types for CPU related = stuff 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_CR= YPTO_SYM_XFORM_CPU), Meant RTE_CRYPTO_SYM_XFORM_CPU_AEAD =3D (RTE_CRYPTO_SYM_XFORM_CPU | RTE_CRYPTO_SY= M_XFORM_AEAD), of course. > /* same for auth and crypto xforms */ > }; >=20 > Then we either can re-define some values in struct rte_crypto_aead_xform = (via unions), > or even have new struct rte_crypto_cpu_aead_xform (same for crypto and a= uth xforms). > Then if PMD wants to support new sync API it would need to recognize new = xform types > and internally it might end up with different session structure (one for= sync, another for async mode). > That I think should allow us to introduce cpu_crypto as part of crypto-de= v API without ABI breakage. > What do you think? > Konstantin >=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 sessi= on? > > > 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= current size. > > > > Saving an ABI/API breakage is more important or placing the code at the= correct place. > > We need to find a tradeoff. Others can comment on this. > > @Thomas Monjalon, @De Lara Guarch, Pablo Any comments? > > > > > > > > > > > > > > > > > Also right now there is no way to add new type of crypto_sym_sess= ion > > > without > > > > > either breaking existing crypto-dev ABI/API or introducing new st= ructure > > > > > (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 synchronousl= y 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= inside. > > > > 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= same > > > amount of changes. > > > > It will be required only in the PMDs which support it and would be mini= mal. > > You would need a feature flag, support for that synchronous API. Sessi= on 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. > > > > > > > > > > > > > > > > > > While rte_security is designed in a way that we can add new sessi= on types > > > and > > > > > related parameters without causing API/ABI breakage. > > > > > > > > Yes the intent is to add new sessions based on various protocols th= at can be > > > supported by the driver. > > > > > > Various protocols and different types of sessions (and devices they b= elong to). > > > Let say right now we have INLINE_CRYPTO, INLINE_PROTO, LOOKASIDE_PROT= O, > > > etc. > > > Here we introduce new type of session. > > > > What is the new value add to the existing sessions. The changes that we= are doing > > here is just to avoid an API/ABI breakage. The synchronous processing c= an happen on both > > crypto and security session. This would mean, only the processing API s= hould be defined, > > rest all should be already there in the sessions. > > In All other cases, INLINE - eth device was not having any format to pe= rform crypto op > > LOOKASIDE - PROTO - add protocol specific sessions which is not availab= le in crypto. > > > > > > > > > It is not that we should find it as an alternative to cryptodev and= using it just > > > because it will not cause > > > > ABI/API breakage. > > > > > > I am considering this new API as an alternative to existing ones, but= as 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 th= e purpose: > > > allows easily (and yes without ABI breakage) introduce new API for sp= ecial type > > > of crypto-dev (SW based). > > > > > > > > > > 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 require= d. > > Now whether to support that, we have ABI/API breakage, that is a differ= ent issue. > > And we may have to deal with it if no other option is there. > > > > > > > > > > > > > > > IMO the code should be placed where its intent is. > > > > > > > > > > > > > > BTW, what is your concern with proposed approach (via rte_securit= y)? > > > > > From my perspective it is a lightweight change and it is totally = optional > > > > > for the crypto PMDs to support it or not. > > > > > Konstantin > > > > > > > > > > > > > > > > > > > AESNI-GCM and AESNI-MB PMDs are updated with this support. Th= ere is > > > a > > > > > small > > > > > > > performance test app under app/test/security_aesni_gcm(mb)_pe= rftest > > > 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 bas= ed on the > > > > > session > > > > > > > data specified and preprocessed in the security session. Diff= erent > > > > > > > than the inline or lookaside modes, when the function exits, = the user will > > > > > > > expect the buffers are either processed successfully, or havi= ng 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 ; Doh= erty, > > > > > Declan > > > > > > > > ; De Lara Guarch, Pablo > > > > > > > > > > > > > > > > Subject: RE: [RFC PATCH 1/9] security: introduce CPU Crypto= action > > > type > > > > > and > > > > > > > > API > > > > > > > > > > > > > > > > Hi Fan, > > > > > > > > > > > > > > > > > > > > > > > > > > This patch introduce new > > > RTE_SECURITY_ACTION_TYPE_CPU_CRYPTO > > > > > > > > action > > > > > > > > > type to security library. The type represents performing = crypto > > > > > > > > > operation with CPU cycles. The patch also includes a new = API to > > > > > > > > > process crypto operations in bulk and the function pointe= rs for PMDs. > > > > > > > > > > > > > > > > > I am not able to get the flow of execution for this action = type. 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 op= erations in > > > bulk. > > > > > > > > What does that mean. How are they different from the existi= ng APIs > > > which > > > > > > > > are also handling bulk crypto ops depending on the budget. > > > > > > > > > > > > > > > > > > > > > > > > -Akhil