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 CC481A31F3 for ; Fri, 18 Oct 2019 15:17:17 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id B434D1D15D; Fri, 18 Oct 2019 15:17:12 +0200 (CEST) Received: from EUR02-HE1-obe.outbound.protection.outlook.com (mail-eopbgr10060.outbound.protection.outlook.com [40.107.1.60]) by dpdk.org (Postfix) with ESMTP id BD1AD1C231 for ; Fri, 18 Oct 2019 15:17:08 +0200 (CEST) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=FRnGpYLAUBUxn2jFVN7OPg7OA6vjI9fxUgAFuAQWlrUcBUfzDjrr0f5s9428oO0o6nCDokEYBvrG2chN1QkzBkhyCH0LQ1GvLR32yKySV8Iv/mqFEQoGnOa++9Tdn+vOlBe9T3+D1DJVtHyD2HhuJI+vZlJEDoYPO34NImB23M3AaTUmMocOZnbPN61kXjh4+P2uCzkicA9ZcXg8z7TdbjqjSDKONqsjYhK0icRItkFzokXBOOm8fSHUx6jB0PadX4vk2KtcGGu8rIYGFjr6Gb3DgJEgqgmIVKCzAkoK217pl/qCK4yWr9QlRtggM44tVeXyV2SWtoBRT/UrxdA5pQ== 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=Niu7Gc9/Dh+cCxVonmI0QpRYtjtL1uaWctT9c8VLA4s=; b=ihEBVKYuc63WqssY1n2UrnGNg67ffHLRr8UszMQOfwdGjGZMY9/UAfKfQinOYgCiRD0Zq3GpKbY5EgPEp12rJSbkguZriL9mhrMFEl23UJ+mzSTxdeH2wJl8Y8NTU7vBYCmyY5snzyo6nCn/JzjHFid2akw7wdDxwqGD9CeGzPazmtEgS6UvDO6bWeBBDH+YcIlRUbiuQZ0YY6397A8fxkIsyPZbBFGw+rXVVJc1IK3H2bB25CfS8UITzt5XHa7RLcgxOW+ElswYSd9jserc4pKF0XTDgNMZNKZEW7NHbptpIztTZ3s3q65POMY9mDNAr1KUPJGSw5heoiV2ONGWWQ== 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=Niu7Gc9/Dh+cCxVonmI0QpRYtjtL1uaWctT9c8VLA4s=; b=e/cWDLJkYaObcH9ldbiFpP9PJ0avT0Pj5z5P5/KWZR7BEKa4xLjoYyJD4AuJNG5N3FHaDYlpw8MYtYJ1kpNj4c7o5VTBMa0V7RyzFdXWnqkigu4SvwjFaivRwDerF6vbUUu6tr5B6WlRXMmeDQ5yOcdSd6wm8/QUpJSrBMr1E60= Received: from VE1PR04MB6639.eurprd04.prod.outlook.com (10.255.118.11) by VE1PR04MB6656.eurprd04.prod.outlook.com (20.179.235.95) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2347.19; Fri, 18 Oct 2019 13:17:07 +0000 Received: from VE1PR04MB6639.eurprd04.prod.outlook.com ([fe80::c045:5df2:ba1f:c3ee]) by VE1PR04MB6639.eurprd04.prod.outlook.com ([fe80::c045:5df2:ba1f:c3ee%5]) with mapi id 15.20.2347.024; Fri, 18 Oct 2019 13:17:07 +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+oMIAAcucAgAMT0cCACHlagIACi3ig Date: Fri, 18 Oct 2019 13:17:06 +0000 Message-ID: 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> <2601191342CEEE43887BDE71AB97725801A8C69C5E@IRSMSX104.ger.corp.intel.com> In-Reply-To: <2601191342CEEE43887BDE71AB97725801A8C69C5E@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.65] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: 57af6200-f2ef-40dd-90e4-08d753cd79cc x-ms-office365-filtering-ht: Tenant x-ms-traffictypediagnostic: VE1PR04MB6656:|VE1PR04MB6656: x-ms-exchange-transport-forked: True x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:8882; x-forefront-prvs: 01949FE337 x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(4636009)(346002)(39860400002)(376002)(136003)(366004)(396003)(189003)(199004)(33656002)(110136005)(9686003)(54906003)(81156014)(81166006)(316002)(256004)(15650500001)(14444005)(52536014)(8936002)(25786009)(229853002)(6436002)(5660300002)(66066001)(55016002)(44832011)(76116006)(66556008)(4326008)(3846002)(6116002)(476003)(66476007)(446003)(11346002)(26005)(186003)(64756008)(66446008)(2906002)(6246003)(486006)(86362001)(66946007)(478600001)(14454004)(7736002)(99286004)(305945005)(8676002)(74316002)(6506007)(76176011)(102836004)(7696005)(71190400001)(71200400001)(156664002)(921003)(1121003)(491001); DIR:OUT; SFP:1101; SCL:1; SRVR:VE1PR04MB6656; 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: /YtAqOZW+7iPGQZWmsb1H5++apuwzWX85AlDAW37NlD0iKV8PSUkKWULonmYVbHPC4JFHpYIvIF74pJ0lBc3wJzP+WvgXu3fO1tofj4iB/m0Q/2EoCYkwo5ezNMLzQUQM6eqHpEqcDve+MB38NyNs/JOwwi4w/YT+Pl8IMyxdTMdqLCLX1xGmjQheCCGCBr1q8bi5Y+ok2p34UyvGoAHWanvP/5k4UJeA2XQo0hOPLI52UCmxjXFYBLeqLOaIAW4qGUARjkBRJVWE+ovsaS12KKTwJlLscktc7oX+RgoMZUaZLxWURDNba9yLVM6MqObverVr1sqs1QZP2TBzSuiYk7RM1Qovi3y0lXtrk6K4EsokU+Nt9Hb8G5H8jCsfJQxzmOrNc0F5ELilUpbi3h9+j3XETH1Mbaw6/DO8vqjSOc= 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: 57af6200-f2ef-40dd-90e4-08d753cd79cc X-MS-Exchange-CrossTenant-originalarrivaltime: 18 Oct 2019 13:17:06.9297 (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: 6Gdmmn7ZjlBPM816IvSdVl9v1zFH0EOc3EcZaL86bQ9pgXVXPXaUkXmCe/9+mLAgJequRkWMBsDsg/p44MTf0g== X-MS-Exchange-Transport-CrossTenantHeadersStamped: VE1PR04MB6656 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, Added my comments inline with your draft. >=20 >=20 > Hi Akhil, >=20 > > > 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 i= ntroduced. > > Please go ahead and remove this. I have no issues with that. >=20 > If you are not happy with that structure, and admit there are issues with= it, > why do you push for reusing it for cpu-crypto API? > 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. [Akhil] This structure is serving some use case which is agreed upon in the Community, we cannot just remove a feature altogether. Rather it is Intel's Use case only. >=20 > About removing data[] from existing rte_cryptodev_sym_session - > Personally would like to do that, but the change seems to be too massive. > Definitely not ready for such effort right now. >=20 [snip].. >=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) > will have bring that subject to TB meeting to decide. > Sounds fair to you? Agreed >=20 > List is below. > Please add/correct me, if I missed something. >=20 > Konstantin Before going into comparison, we should define the requirement as well. 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) 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 s= ession is requested. For lksd-crypto session PMD is free to ignore these fields. =20 No ABI breakage is required.=20 [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_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 [Akhil] It will have breakage at some point till we don't hit the union siz= e. Rather I don't suspect there will be more parameters added. Or do we really care about the ABI breakage when the argument is about=20 the correct place to add a piece of code or do we really agree to add code anywhere just to avoid that 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. What you ar= e proposing is a new concept which doesn't have any extra benefit, rather it 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 per session b= asis, or on a per group of same sessions, or... [Akhil] Will the user really care which process API should be called from t= he PMD. Rather it should be driver's responsibility to store that in the session pr= ivate data which would be opaque to the user. As per my suggestion same process functi= on can be added to multiple sessions or a single session can be managed inside the= PMD. 4) No mandatory mempools for private sessions. User can allocate memory fo= r cpu-crypto session whenever he likes. [Akhil] you mean session private data? You would need that memory anyways, = user will be allocating that already. You do not need to manage that. 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 session for the same crypto processing. Suppose a fix or a new feature(or algo) is added, P= MD owner will need to add code in both the session create APIs. Hence more maintenan= ce and error prone. - Stacks which will be using these new APIs also need to maintain two code path for the same processing while doing session initialization for sync and async 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 the only option we have - make PMD devops->sym_sessio= n_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. [Akhil] It will not contradict with #1, you will only have few checks in th= e session init PMD Which support this mode, find appropriate values and set the appropriate pr= ocess() in it. User should be able to call, legacy enq-deq as well as the new process() wi= thout any issue. User would be at runtime will be able to change the datapath. So this is not a disadvantage, it would be additional flexibility for the u= ser. 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. [Akhil] So lets omit this for current discussion. And I hope we can find so= me way to deal with it. 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 [Akhil] I don't see any overhead of carrying dev id, at least it would be i= nline with the current DPDK methodology. What you are suggesting is a new way to get the things done without much be= nefit. Also I don't see any performance difference as crypto workload is heavier t= han Code cycles, so that wont matter. 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_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 Having said all this, if the disagreements cannot be resolved, you can go f= or a pmd API specific to your PMDs, because as per my understanding the solution doesn't look sca= lable to other PMDs. Your approach is aligned only to Intel, will not benefit others like openss= l which is used by all vendors. Regards, Akhil