From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-0.8 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id B3202C4CEC4 for ; Wed, 18 Sep 2019 07:44:11 +0000 (UTC) Received: from dpdk.org (dpdk.org [92.243.14.124]) by mail.kernel.org (Postfix) with ESMTP id 1C2FA20640 for ; Wed, 18 Sep 2019 07:44:10 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 1C2FA20640 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=intel.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=dev-bounces@dpdk.org Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 76DD31C119; Wed, 18 Sep 2019 09:44:09 +0200 (CEST) Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by dpdk.org (Postfix) with ESMTP id 992C01C10E for ; Wed, 18 Sep 2019 09:44:07 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga107.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 18 Sep 2019 00:44:06 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,520,1559545200"; d="scan'208";a="387834080" Received: from irsmsx106.ger.corp.intel.com ([163.33.3.31]) by fmsmga006.fm.intel.com with ESMTP; 18 Sep 2019 00:44:03 -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; Wed, 18 Sep 2019 08:44:02 +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+AedzaNgY0qWMAmu5GwNVqcbQpEAgAArCICAAuADAIAARA4AgAYiKoCAAbQS0IABqquAgAZiqNCAAPA4gIABtviQ Date: Wed, 18 Sep 2019 07:44:01 +0000 Message-ID: <2601191342CEEE43887BDE71AB9772580191966C23@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: Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiZGU3NTQ1YTEtODQwYi00Y2ViLTg5NzEtMDVhOWQxYTUxZjFkIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiazJKRFRzVGRaemgyZStqTTRHbEZMcFNPdzFVN3IwRWJ6N0JhaUdyYkh6SFdxakJcL1dVN2J4aVNJa2NPZENQd28ifQ== 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, > > > > > > > > This action type allows the burst of symmetric crypto workl= oad using > > the > > > > > > same > > > > > > > > algorithm, key, and direction being processed by CPU cycles > > > > synchronously. > > > > > > > > This flexible action type does not require external hardwar= e > > involvement, > > > > > > > > having the crypto workload processed synchronously, and is = more > > > > > > performant > > > > > > > > than Cryptodev SW PMD due to the saved cycles on removed "a= sync > > > > mode > > > > > > > > simulation" as well as 3 cacheline access of the crypto ops= . > > > > > > > > > > > > > > Does that mean application will not call the cryptodev_enqueu= e_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 w= ill have > > the > > > > > > crypto processed packets while returning from the API? > > > > > > > > > > > > Yes, though the plan is that API will operate on raw data buffe= rs, not > > mbufs. > > > > > > > > > > > > > > > > > > > > I still do not understand why we cannot do with the conventio= nal > > crypto lib > > > > > > only. > > > > > > > As far as I can understand, you are not doing any protocol pr= ocessing > > or > > > > any > > > > > > value add > > > > > > > To the crypto processing. IMO, you just need a synchronous cr= ypto > > > > processing > > > > > > API which > > > > > > > Can be defined in cryptodev, you don't need to re-create a cr= ypto > > session > > > > in > > > > > > the name of > > > > > > > Security session in the driver just to do a synchronous proce= ssing. > > > > > > > > > > > > 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 exi= sting > > > > 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 cu= rrent > > crypto-op > > > > approach. > > > > That's why the general idea - have all data that wouldn't change fr= om 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 h= ave it as > > a parameter until > > > You get it approved in the crypto xform. I believe it will be benefic= ial 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 for CPU relate= d > > stuff here? >=20 > 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 the current x= forms. > So if you are adding new fields in the new xform, the size will be more t= han that of the union of xforms. > ABI breakage would still be there. >=20 > If you think a valid compression of the AEAD xform can be done, 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 crypto offset). So for now we can make that path work without any ABI breakage.=20 Fan, please feel free to correct me here, if I missed something. If in future we would need to add some extra information it might 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.=20 >=20 > > Let say we can have : > > num rte_crypto_sym_xform_type { > > RTE_CRYPTO_SYM_XFORM_NOT_SPECIFIED =3D 0, /**< No xform specifi= ed > > */ > > RTE_CRYPTO_SYM_XFORM_AUTH, /**< Authentication xfo= rm */ > > 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_CRYPTO_SYM_XFORM_CPU), >=20 > Instead of CPU I believe SYNC would be better. I don't mind to name it to SYNC, but I'd like to outline, that it's not really more CPU then generic SYNC API (it doesn't pass IOVA for data buffers, etc., only VA).=20 >=20 > > /* same for auth and crypto xforms */ > > }; > > > > Then we either can re-define some values in struct rte_crypto_aead_xfor= m (via > > unions), > > or even have new struct rte_crypto_cpu_aead_xform (same for crypto and= auth > > xforms). > > Then if PMD wants to support new sync API it would need to recognize ne= w > > xform types > > and internally it might end up with different session structure (one f= or sync, > > 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 > > > > > > > > > > > > > > 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 ses= sion? > > > > Again having that value constant per session might allow some extr= a > > > > optimisations > > > > That would be hard to achieve for dynamic case. > > > > and not extendable: > > > > Suppose tomorrow will need to add something extra (some new algorit= hm > > > > 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 exce= ed current > > size. > > > > > > Saving an ABI/API breakage is more important or placing the code at t= he > > 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_se= ssion > > > > without > > > > > > either breaking existing crypto-dev ABI/API or introducing new = structure > > > > > > (rte_crypto_sym_cpu_session or so) for that. > > > > > > > > > > What extra info is required in rte_cryptodev_sym_session to get t= he > > > > 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 synchronou= sly 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 co= de inside. > > > > > Please correct me in case I am missing something. > > > > > > > > To add new API into crypto-dev would also require changes in the PM= D, > > > > it wouldn't come totally free and I believe would require roughly t= he same > > > > amount of changes. > > > > > > It will be required only in the PMDs which support it and would be mi= nimal. > > > You would need a feature flag, support for that synchronous API. Ses= sion > > information will > > > already be there in the session. The changes wrt cipher_offset need t= o 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 ses= sion > > 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= belong > > to). > > > > Let say right now we have INLINE_CRYPTO, INLINE_PROTO, > > LOOKASIDE_PROTO, > > > > 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= can > > happen on both > > > crypto and security session. This would mean, only the processing API= should > > be defined, > > > rest all should be already there in the sessions. > > > In All other cases, INLINE - eth device was not having any format to = perform > > crypto op > > > LOOKASIDE - PROTO - add protocol specific sessions which is not avail= able in > > crypto. > > > > > > > > > > > > It is not that we should find it as an alternative to cryptodev a= nd using it > > just > > > > because it will not cause > > > > > ABI/API breakage. > > > > > > > > I am considering this new API as an alternative to existing ones, b= ut as an > > > > extension. > > > > Existing crypto-op API has its own advantages (generic), and I thin= k 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 = special > > type > > > > of crypto-dev (SW based). > > > > > > > > > > > > > > Adding a synchronous processing API is understandable and can be adde= d in > > both > > > Crypto as well as Security, but a new action type for it is not requi= red. > > > Now whether to support that, we have ABI/API breakage, that is a diff= erent > > 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_secur= ity)? > > > > > > From my perspective it is a lightweight change and it is totall= y optional > > > > > > for the crypto PMDs to support it or not. > > > > > > Konstantin > > > > > > > > > > > > > > > > > > > > > > AESNI-GCM and AESNI-MB PMDs are updated with this support. > > There is > > > > a > > > > > > small > > > > > > > > performance test app under > > app/test/security_aesni_gcm(mb)_perftest > > > > to > > > > > > > > prove. > > > > > > > > > > > > > > > > For the new API > > > > > > > > The packet is sent to the crypto device for symmetric crypt= o > > > > > > > > processing. The device will encrypt or decrypt the buffer b= ased on the > > > > > > session > > > > > > > > data specified and preprocessed in the security session. Di= fferent > > > > > > > > than the inline or lookaside modes, when the function exits= , the user > > will > > > > > > > > expect the buffers are either processed successfully, or ha= ving 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.or= g > > > > > > > > > Cc: Ananyev, Konstantin ; D= oherty, > > > > > > Declan > > > > > > > > > ; De Lara Guarch, Pablo > > > > > > > > > > > > > > > > > > Subject: RE: [RFC PATCH 1/9] security: introduce CPU Cryp= to action > > > > type > > > > > > and > > > > > > > > > API > > > > > > > > > > > > > > > > > > Hi Fan, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > This patch introduce new > > > > RTE_SECURITY_ACTION_TYPE_CPU_CRYPTO > > > > > > > > > action > > > > > > > > > > type to security library. The type represents performin= g crypto > > > > > > > > > > operation with CPU cycles. The patch also includes a ne= w API to > > > > > > > > > > process crypto operations in bulk and the function poin= ters for > > PMDs. > > > > > > > > > > > > > > > > > > > I am not able to get the flow of execution for this actio= n 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 = operations > > in > > > > bulk. > > > > > > > > > What does that mean. How are they different from the exis= ting APIs > > > > which > > > > > > > > > are also handling bulk crypto ops depending on the budget= . > > > > > > > > > > > > > > > > > > > > > > > > > > > -Akhil