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 AFDC4A00BE; Fri, 1 Nov 2019 14:53:20 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 262051DFF2; Fri, 1 Nov 2019 14:53:20 +0100 (CET) Received: from EUR01-VE1-obe.outbound.protection.outlook.com (mail-eopbgr140054.outbound.protection.outlook.com [40.107.14.54]) by dpdk.org (Postfix) with ESMTP id B839F1DFEE for ; Fri, 1 Nov 2019 14:53:18 +0100 (CET) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=MJoJAYJQu1AhM6eFdIBvhCgxI0JaM81J1Vk3UchAYqcOuL3Br9lA+SVCUzGxSTP9GlF0eyj/irsMY/4KLXT+ctPNOhzbwpd7w3rEaOQZPruVa+q72IduiDteinwdUidaYFTb++BU7WhdzOBHy/uXQFTOF1QyQ9E8WgIoxV/9lcG507KOoZXDS2HlE0Z84St3IGQXeb2pSBPZ/Lpazi5DRkHVhMEMWUd1Wg5KnBgmvHXOZsQp2xU9e7yQ8wm3QSjaeAnEuNu3xANJHvpIrKcEnLkkPVlXgFV/rKsEHFq1hPImaHdAWdJqtVbTJzx5omlaWM8Ig/iBz7OyBJWO+X5vPQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=q2UBqHJ0fi5+aFsMVDrN7yQYvlFH+UEH4GrQye5DadA=; b=OVnvl+m23Kb2LcUzyCWGVyaR6lWzmiyFYXl5+iST8ynvQqKVmpjb6qZ6AUqVD56s5GhK9S6LtAjHY60tn32LfgyUxZYL3OtZuZwOTSd/XwMYeQvKgmg/m0L1h3NkiIfw+DNH2hBLbWFCX50L9O/3AsvpFcNEvqJA+Yr9HRmCLgw3A78MSPmqtm5OoJcS3HHwefv/3b7J0FWylwYu9UBln24Ds9Xl2mZ897zVKImzyMHMgjTnVpvIJ90X/Ok3b/QL4hRsWiXRjrxGNTMEnfRQpwPYObnKrnxfVrDLo1bnO9LXDPG4Fpx+9RtpzwELlQwggx6lBdMRsaaKsAPD0s5xXQ== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=nxp.com; dmarc=pass action=none header.from=nxp.com; dkim=pass header.d=nxp.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nxp.com; s=selector2; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=q2UBqHJ0fi5+aFsMVDrN7yQYvlFH+UEH4GrQye5DadA=; b=Ju2uU4hyMVMAjYkkb2tZ1Q2THXQkg7Gvn8+6byeDeOhivMk47Lv0kZTHpJnRNdsABKbaLCw+T/xJiCPlFK/fj6X+zcqjLOgYrx5P9/hCAIDojEFrin0AKJvOoozNS49V25Bfu4xIgpVFpqOwFxMJXkwMc8TCP5zavoZTwSXkFgg= Received: from VE1PR04MB6639.eurprd04.prod.outlook.com (10.255.118.11) by VE1PR04MB6413.eurprd04.prod.outlook.com (20.179.232.94) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2408.24; Fri, 1 Nov 2019 13:53:17 +0000 Received: from VE1PR04MB6639.eurprd04.prod.outlook.com ([fe80::9dc:aa5c:2bb8:b561]) by VE1PR04MB6639.eurprd04.prod.outlook.com ([fe80::9dc:aa5c:2bb8:b561%6]) with mapi id 15.20.2387.028; Fri, 1 Nov 2019 13:53:17 +0000 From: Akhil Goyal To: "Ananyev, Konstantin" , "'dev@dpdk.org'" , "De Lara Guarch, Pablo" , 'Thomas Monjalon' , "Zhang, Roy Fan" , "Doherty, Declan" CC: 'Anoob Joseph' , Hemant Agrawal Thread-Topic: [RFC PATCH 1/9] security: introduce CPU Crypto action type and API Thread-Index: AQHVYm4LqyJkewM9NkuUWAfAmrqx1acbUiZggAAsN4CAAtsIgIAAT02AgAYXC5CAAbSDgIABbRGggAaWxgCAAPjG4IABs/OAgAuzNYCAAoY34IAE8G8AgAAH4mCAAbN9gIADA/pwgAZJhgCAAr+oMIAAcucAgAMT0cCACHlagIACi3iggATErYCAAV8CAIAAdXYAgAD56qCAC2B8AIADE8cg Date: Fri, 1 Nov 2019 13:53:17 +0000 Message-ID: References: <2601191342CEEE43887BDE71AB97725801A8C6E152@IRSMSX104.ger.corp.intel.com> <2601191342CEEE43887BDE71AB97725801A8C7292F@IRSMSX104.ger.corp.intel.com> In-Reply-To: <2601191342CEEE43887BDE71AB97725801A8C7292F@IRSMSX104.ger.corp.intel.com> Accept-Language: en-IN, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: spf=none (sender IP is ) smtp.mailfrom=akhil.goyal@nxp.com; x-originating-ip: [92.120.1.72] x-ms-publictraffictype: Email x-ms-office365-filtering-ht: Tenant x-ms-office365-filtering-correlation-id: dfb271ff-2126-4c4f-5bdd-08d75ed2d90c x-ms-traffictypediagnostic: VE1PR04MB6413:|VE1PR04MB6413: x-ms-exchange-purlcount: 1 x-ms-exchange-transport-forked: True x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:9508; x-forefront-prvs: 020877E0CB x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(4636009)(396003)(346002)(366004)(136003)(39860400002)(376002)(189003)(199004)(316002)(30864003)(25786009)(99286004)(76116006)(66476007)(66556008)(64756008)(66446008)(5660300002)(66946007)(71200400001)(71190400001)(110136005)(54906003)(9686003)(76176011)(7696005)(3846002)(6116002)(6506007)(6306002)(52536014)(102836004)(55016002)(478600001)(74316002)(7736002)(33656002)(305945005)(486006)(6246003)(186003)(14454004)(256004)(26005)(86362001)(2906002)(229853002)(8936002)(14444005)(4326008)(966005)(8676002)(66066001)(81156014)(81166006)(446003)(6436002)(15650500001)(11346002)(44832011)(45080400002)(476003)(156664002)(921003)(1121003)(491001)(579004)(559001); DIR:OUT; SFP:1101; SCL:1; SRVR:VE1PR04MB6413; H:VE1PR04MB6639.eurprd04.prod.outlook.com; FPR:; SPF:None; LANG:en; PTR:InfoNoRecords; MX:1; A:1; received-spf: None (protection.outlook.com: nxp.com does not designate permitted sender hosts) x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: 8IHe3TgPlTgb16D7tEJq6T+mluHRh6BE0TlyESzxHgbMuVQuS0mWS/KXhrJfhGnoTtmnPIkH7JYgiRG13zUKx8rAQ825mD+8p/8FduNrRfeYUmHDXuOddpMUFNZ2eOleJ6fOFfR9yKN0pGe4m77QXzZ+Dke4ajE1Yuh3vYcEkBnv6WZe2njwqEi6ZXtnSpUCaZ6n1eNDpAOPelRLeoMahdftCNV3QCczqnrE/tLUdPCALBvHRFSbVLUVQ8NkrLi4Z7jAFwzRXmfSTq6bfxGZdxwEv/yN1swYdPmNVBhCltv4znlgj3eVZYBomIH5vVTqKj4c165Qgsnpxgp84CZJeEfItuucFuCZK8fIlVsrwBzkVrM6fzd+FxzMLwfQbO1OCxAabm+3h0v17+jbxP7Zu+ZnZFSr5mrphLNL1PE4dV9GwoLceCmdJOOik4W8S5zJTMmix4K84+P/z386xmtzzD94K2e+jpbDQAjjaAxNH+0= Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-OriginatorOrg: nxp.com X-MS-Exchange-CrossTenant-Network-Message-Id: dfb271ff-2126-4c4f-5bdd-08d75ed2d90c X-MS-Exchange-CrossTenant-originalarrivaltime: 01 Nov 2019 13:53:17.2074 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635 X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: 3T6A1k7qEDke5XDOl+HnGsi5z/8e5wEg1CX3TQszgJGF1dcka5LAR5mzSlG2q620xPvcEWKyHkX3vrhtC+ruVQ== X-MS-Exchange-Transport-CrossTenantHeadersStamped: VE1PR04MB6413 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 Konstantin, >=20 >=20 > Hi Akhil, >=20 > > > > > > Added my comments inline with your draft. > > > > > > [snip].. > > > > > > > > > > > > > > > > > > > > Ok, then my suggestion: > > > > > > > Let's at least write down all points about crypto-dev approac= h 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? > > > > > > Agreed > > > > > > > > > > > > > > List is below. > > > > > > > Please add/correct me, if I missed something. > > > > > > > > > > > > > > Konstantin > > > > > > > > > > > > Before going into comparison, we should define the requirement = as > well. > > > > > > > > > > Good point. > > > > > > > > > > > What I understood from the patchset, > > > > > > "You need a synchronous API to perform crypto operations on raw= data > > > using > > > > > SW PMDs" > > > > > > So, > > > > > > - no crypto-ops, > > > > > > - no separate enq-deq, only single process API for data path > > > > > > - Do not need any value addition to the session parameters. > > > > > > (You would need some parameters from the crypto-op which > > > > > > Are constant per session and since you wont use crypto-op, > > > > > > You need some place to store that) > > > > > > > > > > Yes, this is correct, I think. > > > > > > > > > > > > > > > > > Now as per your mail, the comparison > > > > > > 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 > > > > > session is requested. > > > > > > For lksd-crypto session PMD is free to ignore these fields. > > > > > > No ABI breakage is required. > > > > > > > > > > > > [Akhil] Agreed, no issues. > > > > > > > > > > > > 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_sy= m_xform > > > > > *xforms) > > > > > > that would return const struct rte_crypto_cpu_sym_ses= sion_ops > > > *based > > > > > on input xforms. > > > > > > Advantages: > > > > > > 1) totally opaque data structure (no ABI breakages in future)= , > PMD > > > > > writer is totally free > > > > > > with it format and contents. > > > > > > > > > > > > [Akhil] It will have breakage at some point till we don't hit t= he union > size. > > > > > > > > > > Not sure, what union you are talking about? > > > > > > > > Union of xforms in rte_security_session_conf > > > > > > Hmm, how does it relates here? > > > I thought we discussing pure rte_cryptodev_sym_session, no? > > > > > > > > > > > > > > > > > > Rather I don't suspect there will be more parameters added. > > > > > > Or do we really care about the ABI breakage when the argument i= s > about > > > > > > the correct place to add a piece of code or do we really agree = to add > code > > > > > > anywhere just to avoid that breakage. > > > > > > > > > > I am talking about maintaining it in future. > > > > > if your struct is not seen externally, no chances to introduce AB= I > breakage. > > > > > > > > > > > > > > > > > 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.). > > > > > > > > > > > > [Akhil] There is nothing called as session ops in current DPDK. > > > > > > > > > > True, but it doesn't mean we can't/shouldn't have it. > > > > > > > > We can have it if it is not adding complexity for the user. Creatin= g 2 > different > > > code > > > > Paths for user is not desirable for the stack developers. > > > > > > > > > > > > > > > What you are proposing > > > > > > is a new concept which doesn't have any extra benefit, rather i= t is > adding > > > > > complexity > > > > > > to have two different code paths for session create. > > > > > > > > > > > > > > > > > > 3) User can decide does he wants to store ops[] pointer on a p= er > session > > > > > basis, > > > > > > or on a per group of same sessions, or... > > > > > > > > > > > > [Akhil] Will the user really care which process API should be c= alled from > the > > > > > PMD. > > > > > > Rather it should be driver's responsibility to store that in th= e session > private > > > > > data > > > > > > which would be opaque to the user. As per my suggestion same pr= ocess > > > > > function can > > > > > > be added to multiple sessions or a single session can be manage= d inside > the > > > > > PMD. > > > > > > > > > > In that case we either need to have a function per session (store= d > internally), > > > > > or make decision (branches) at run-time. > > > > > But as I said in other mail - I am ok to add small shim structure= here: > > > > > either rte_crypto_cpu_sym_session { void *ses; struct > > > > > rte_crypto_cpu_sym_session_ops ops; } > > > > > or rte_crypto_cpu_sym_session { void *ses; struct > > > > > rte_crypto_cpu_sym_session_ops *ops; } > > > > > And merge rte_crypto_cpu_sym_init() and rte_crypto_cpu_sym_get_op= s() > > > into > > > > > one (init). > > > > > > > > Again that will be a separate API call from the user perspective wh= ich is not > > > good. > > > > > > > > > > > > > > > > > > > > > > > > > > > 4) No mandatory mempools for private sessions. User can > allocate > > > > > memory for cpu-crypto > > > > > > session whenever he likes. > > > > > > > > > > > > [Akhil] you mean session private data? > > > > > > > > > > Yes. > > > > > > > > > > > You would need that memory anyways, user will be > > > > > > allocating that already. You do not need to manage that. > > > > > > > > > > What I am saying - right now user has no choice but to allocate i= t via > > > mempool. > > > > > Which is probably not the best options for all cases. > > > > > > > > > > > > > > > > > Disadvantages: > > > > > > 5) Extra changes in control path > > > > > > 6) User has to store session_ops pointer explicitly. > > > > > > > > > > > > [Akhil] More disadvantages: > > > > > > - All supporting PMDs will need to maintain TWO types of sessio= n for > the > > > > > > same crypto processing. Suppose a fix or a new feature(or algo)= is > added, > > > PMD > > > > > owner > > > > > > will need to add code in both the session create APIs. Hence mo= re > > > > > maintenance and > > > > > > error prone. > > > > > > > > > > I think majority of code for both paths will be common, plus even= we'll > reuse > > > > > current sym_session_init() - > > > > > changes in PMD session_init() code will be unavoidable. > > > > > But yes, it will be new entry in devops, that PMD will have to su= pport. > > > > > Ok to add it as 7) to the list. > > > > > > > > > > > - Stacks which will be using these new APIs also need to mainta= in two > > > > > > code path for the same processing while doing session initializ= ation > > > > > > for sync and async > > > > > > > > > > That's the same as #5 above, I think. > > > > > > > > > > > > > > > > > > > > > > > b) Your suggestion - reuse existing rte_cryptodev_sym_sess= ion_init() > and > > > > > existing rte_cryptodev_sym_session > > > > > > structure. > > > > > > Advantages: > > > > > > 1) allows to reuse same struct and init/create/clear() functio= ns. > > > > > > 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 > > > > > sessions pointers > > > > > > for both sync and async mode for the same device. > > > > > > So the only option we have - make PMD devops= - > > > > > >sym_session_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 > > > > > 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 o= nly > 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. behavio= r > change, > > > > > existing app change. > > > > > > - might cause extra space overhead. > > > > > > > > > > > > [Akhil] It will not contradict with #1, you will only have few = checks in > the > > > > > session init PMD > > > > > > Which support this mode, find appropriate values and set the > appropriate > > > > > process() in it. > > > > > > User should be able to call, legacy enq-deq as well as the new = process() > > > > > without any issue. > > > > > > User would be at runtime will be able to change the datapath. > > > > > > So this is not a disadvantage, it would be additional flexibili= ty for the > user. > > > > > > > > > > Ok, but that's what I am saying - if PMD would *always* have to c= reate a > > > > > session that can handle > > > > > both modes (sync/async), then user would *always* have to provide > > > parameters > > > > > for both modes too. > > > > > Otherwise if let say user didn't setup sync specific parameters a= t all, what > > > PMD > > > > > should do? > > > > > - return with error? > > > > > - init session that can be used with async path only? > > > > > My current assumption is #1. > > > > > If #2, then how user will be able to distinguish is that session = valid for > both > > > > > modes, or only for one? > > > > > > > > I would say a 3rd option, do nothing if sync params are not set. > > > > Probably have a debug print in the PMD(which support sync mode) to > specify > > > that > > > > session is not configured properly for sync mode. > > > > > > So, just print warning and proceed with init session that can be used= with > async > > > path only? > > > Then it sounds the same as #2 above. > > > Which actually means that sync mode parameters for sym_session_init() > > > becomes optional. > > > Then we need an API to provide to the user information what modes > > > (sync+async/async only) is supported by that session for given dev_id= . > > > And user would have to query/retain this information at control-path, > > > and store it somewhere in user-space together with session pointer an= d > dev_ids > > > to use later at data-path (same as we do now for session type). > > > That definitely requires changes in control-path to start using it. > > > Plus the fact that this value can differ for different dev_ids for th= e same > session - > > > doesn't make things easier here. > > > > API wont be required to specify that. Feature flag will be sufficient, = not a big > change > > From the application perspective. > > > > Here is some pseudo code just to elaborate my understanding. This will = need > some > > > > From application, > > If(dev_info->feature_flags & RTE_CRYPTODEV_FF_SYNC) { > > /* set additional params in crypto xform */ > > } > > > > Now in the driver, > > pmd_sym_session_configure(dev,xform,sess,mempool) { > > ... > > If(dev_info->feature_flags & RTE_CRYPTODEV_FF_SYNC > > && xform->/*sync params are set*/) { > > /*Assign process function pointer in sess->priv_data*/ > > } /* It may return error if FF_SYNC is set and params are not correct. >=20 > Then all apps will always *have to* setup sync parameters in xform. > What you suggest is *mandatory* sync mode: user always has to setup sync > mode params if PMD does support it (no matter does he plan to use sync mo= de > or not). > Which means behavior change in existing apps. We are adding new params in xform, and user may not fill those params and d= efaults To 0 for all the params. Or we can pack a flag in xform when all sync param= s are set. It can be dealt with when we do the code. I don't say, user will always have to set the params when sync mode is supp= orted. It will be a warning from the PMD and user may ignore it if he doesn't want= to use sync mode.=20 >=20 > > It would be upto the driver whether it support both SYNC and > ASYNC.*/ > > } > > > > Now the new sync API > > > > pmd_process(...) { > > If(dev_info->feature_flags & RTE_CRYPTODEV_FF_SYNC > > && sess_priv->process !=3D NULL) > > sess_priv->process(...); > > else > > ASSERT("sync mode not configured properly or not supported"); > > } > > > > In the data path, there is no extra processing happening. > > Even in case of your suggestion, you should have these type of error ch= ecks, > > You cannot blindly trust on the application that the pointers are corre= ct. > > > > > > > > > Internally the PMD will not store the process() API in the session = priv data > > > > And while calling the first packet, devops->process will give an as= sert that > > > session > > > > Is not configured for sync mode. The session validation would be do= ne in > any > > > case > > > > your suggestion or mine. So no extra overhead at runtime. > > > > > > I believe that after session_init() user should get either an error o= r > > > valid session handler that he can use at runtime. > > > Pushing session validation to runtime doesn't seem like a good idea. > > > > > It may get a warning from the PMD, that FF_SYNC is set but params are n= ot > > Correct/available. See above. >=20 > I think warning is not enough. > There should be a clear way (API) for developer to realize is the created= session > can be used by sync API data-path or not. Warning is a clear notification to the user, that SYNC mode can be supporte= d by the device But user does not want to use that. Moreover, when first packet is sent, sync API will throw error. So what is = the issue. >=20 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 3) not possible to store device (not driver) specific data wit= hin > the > > > > > session, but I think it is not really needed right now. > > > > > > So probably minor compared to 2.b.2. > > > > > > > > > > > > [Akhil] So lets omit this for current discussion. And I hope we= can find > some > > > > > way to deal with it. > > > > > > > > > > I don't think there is an easy way to fix that with existing API. > > > > > > > > > > > > > > > > > > > > > > > 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) > itself) > > > and > > > > > just does: > > > > > > session_ops->process(sess, ...); > > > > > > Advantages: > > > > > > 1) fastest possible execution path > > > > > > 2) no need to carry on dev_id for data-path > > > > > > > > > > > > [Akhil] I don't see any overhead of carrying dev id, at least i= t would be > > > inline > > > > > with the > > > > > > current DPDK methodology. > > > > > > > > > > If we'll add process() into rte_cryptodev itself (same as we have > > > > > enqueue_burst/dequeue_burst), > > > > > then it will be an ABI breakage. > > > > > Also there are discussions to get rid of that approach completely= : > > > > > > > > > https://eur01.safelinks.protection.outlook.com/?url=3Dhttp%3A%2F%2Fmails.= dpd > > > k.org%2Farchives%2Fdev%2F2019- > > > > September%2F144674.html&data=3D02%7C01%7Cakhil.goyal%40nxp.com%7 > > > > C1859dc1d29cd45a51e9908d7571784bb%7C686ea1d3bc2b4c6fa92cd99c5c301 > > > > 635%7C0%7C0%7C637073630835415165&sdata=3DBz9jgisyVzRJNt1BijtvSlurh > > > JU1vXBbynNwlMDjaco%3D&reserved=3D0 > > > > > So I am not sure this is a recommended way these days. > > > > > > > > We can either have it in rte_cryptodev or in rte_cryptodev_ops whic= hever > > > > is good for you. > > > > > > > > Whether it is ABI breakage or not, as per your requirements, this i= s the > correct > > > > approach. Do you agree with this or not? > > > > > > I think it is possible approach, but not the best one: > > > it looks quite flakey to me (see all these uncertainty with sym_sessi= on_init > > > above), > > > plus introduces extra overhead at data-path. > > > > Uncertainties can be handled appropriately using a feature flag > > > > > > > > > > > > > Now handling the API/ABI breakage is a separate story. In 19.11 rel= ease we > > > > Are not much concerned about the ABI breakages, this was discussed = in > > > > community. So adding a new dev_ops wouldn't have been an issue. > > > > Now since we are so close to RC1 deadline, we should come up with s= ome > > > > other solution for next release. May be having a pmd API in 20.02 a= nd > > > > converting it into formal one in 20.11 > > > > > > > > > > > > > > > > > > > What you are suggesting is a new way to get the things done wit= hout > much > > > > > benefit. > > > > > > > > > > Would help with ABI stability plus better performance, isn't it e= nough? > > > > > > > > > > > Also I don't see any performance difference as crypto workload = is > heavier > > > than > > > > > > Code cycles, so that wont matter. > > > > > > > > > > It depends. > > > > > Suppose function call costs you ~30 cycles. > > > > > If you have burst of big packets (let say crypto for each will ta= ke ~2K > cycles) > > > that > > > > > belong > > > > > to the same session, then yes you wouldn't notice these extra 30 = cycles > at all. > > > > > If you have burst of small packets (let say crypto for each will = take ~300 > > > cycles) > > > > > each > > > > > belongs to different session, then it will cost you ~10% extra. > > > > > > > > Let us do some profiling on openssl with both the approaches and fi= nd out > the > > > > difference. > > > > > > > > > > > > > > > So IMO, there is no advantage in your suggestion as well. > > > > > > > > > > > > > > > > > > Disadvantages: > > > > > > 3) user has to carry on session_ops pointer explicitly > > > > > > b) Your suggestion: add (*cpu_process) inside rte_cryptode= v_ops > and > > > then: > > > > > > rte_crypto_cpu_sym_process(uint8_t dev_id, > > > rte_cryptodev_sym_session > > > > > *sess, /*data parameters*/) {... > > > > > > rte_cryptodevs[dev_id].dev_ops->cpu_proces= s(ses, ...); > > > > > > /*and then inside PMD specifc process: */ > > > > > > pmd_private_session =3D sess- > > > >sess_data[this_pmd_driver_id].data; > > > > > > /* and then most likely either */ > > > > > > pmd_private_session->process(pmd_private_s= ession, ...); > > > > > > /* 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 fo= r > data and > > > > > instructions. > > > > > > Possible slowdown compared to a) (not measured). > > > > > > > > > > > > Having said all this, if the disagreements cannot be resolved, = you can > go > > > for a > > > > > pmd API specific > > > > > > to your PMDs, > > > > > > > > > > I don't think it is good idea. > > > > > PMD specific API is sort of deprecated path, also there is no cle= an way to > use > > > it > > > > > within the libraries. > > > > > > > > I know that this is a deprecated path, we can use it until we are n= ot > allowed > > > > to break ABI/API > > > > > > > > > > > > > > > because as per my understanding the solution doesn't look scala= ble to > > > other > > > > > PMDs. > > > > > > Your approach is aligned only to Intel , will not benefit other= s like > openssl > > > > > which is used by all > > > > > > vendors. > > > > > > > > > > I feel quite opposite, from my perspective majority of SW backed = PMDs > will > > > > > benefit from it. > > > > > And I don't see anything Intel specific in my proposals above. > > > > > About openssl PMD: I am not an expert here, but looking at the co= de, I > think > > > it > > > > > will fit really well. > > > > > Look yourself at its internal functions: > > > > > process_openssl_auth_op/process_openssl_crypto_op, > > > > > I think they doing exactly the same - they use sync API underneat= h, and > they > > > are > > > > > session based > > > > > (AFAIK you don't need any device/queue data, everything that need= ed for > > > > > crypto/auth is stored inside session). > > > > > > > > > By vendor specific, I mean, > > > > - no PMD would like to have 2 different variants of session Init AP= Is for > doing > > > the same stuff. > > > > - stacks will become vendor specific while using 2 separate session= create > APIs. > > > No stack would > > > > Like to support 2 variants of session create- one for HW PMDs and o= ne for > SW > > > PMDs. > > > > > > I think what you refer on has nothing to do with 'vendor specific'. > > > I would name it 'extra overhead for PMD and stack writers'. > > > Yes, for sure there is extra overhead (as always with new API) - > > > for both producer (PMD writer) and consumer (stack writer): > > > New function(s) to support, probably more tests to create/run, etc. > > > Though this API is optional - if PMD/stack maintainer doesn't see > > > value in it, they are free not to support it. > > > From other side, re-using rte_cryptodev_sym_session_init() > > > wouldn't help anyway - both data-path and control-path would differ > > > from async mode anyway. > > > BTW, right now to support different HW flavors > > > we do have 4 different control and data-paths for both > > > ipsec-secgw and librte_ipsec: > > > lkds-none/lksd-proto/inline-crypto/inline-proto. > > > And that is considered to be ok. > > > > No that is not ok. We cannot add new paths for every other case. >=20 > What I am saying: if let-say lookaside-proto/inline-crypto/inline-proto > deserves its own case in rte_security/rte_crypto API, > I don't understand why cpu-crypto doesn't. Because cpu-crypto is done by a crypto device and for that we have lookasid= e none. SW PMDs are registered as crypto device and we should leverage crypto frame= work. I would suggest in future may be 20.11, we can have a security wrapper over= cryptodev API For lookaside none use case. So that we will have a single API set for all = cases. >=20 > > Those 4 are controlled using 2 set of APIs. >=20 > Yes there are 2 API sets (rte_cryptodev/rte_security), > but in fact if you look at ipsec-secgw and librte_ipsec we have 4 differe= nt code > paths. > For both create_session() and ipsec_enqueue() we have a big switch() with= 4 > different cases. > Nearly the same for librte_ipsec - we have different prepare/process > function pointers for each security type. >=20 > > We should try our best to > > Have minimum overhead to the application writer. This pain was also > discussed > > In the one of DPDK conference as well. > > DPDK is not a standalone entity, there are stacks running over it alway= s. > > We should not add API for every other use case when we have an alternat= ive > > Approach with the existing API set. > > > > Now introducing another one would add to that pain and a lot of work fo= r > > Both producer and consumer. >=20 > If I would see a clean approach to implement desired functionality > without introducing new API - I would definitely support it. > The problem is that from my perspective, > what you suggesting with existing API will bring more drawbacks then posi= tives. >From my perspective I see more benefits than the negatives. - less changes in driver/app - no major performance gap - easier migration for the stack from one SOC to other. The main argument from my side is that: You need synchronous processing for SW PMDs which is data path. Why do you need a special session control path to do that. You should have = some extra Params packed in the same control API. > BTW, our first approach (via rte_security) does reuse existing API, > so if adding new API is the main concern - let's reconsider that path. >=20 That will be there only if we have security wrapper on cryptodev session cr= eate For lookaside none use case. But the issue would still remain the same. No special session create for supporting sync mode. > > It would be interesting to see how much performance difference will be = there > in the > > Two approaches. As per my understanding it wont be much as compared to > the > > Extra work that you will be inducing. > > > > -Akhil > > > > > Honestly, I don't understand why SW backed implementations > > > can't have their own path that would suite them most. > > > Konstantin > > > > > > > > > > > > > > >