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 BE9F9A328D for ; Tue, 22 Oct 2019 15:31:58 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 901DB1BEF0; Tue, 22 Oct 2019 15:31:58 +0200 (CEST) Received: from EUR04-DB3-obe.outbound.protection.outlook.com (mail-eopbgr60080.outbound.protection.outlook.com [40.107.6.80]) by dpdk.org (Postfix) with ESMTP id 906D81BEEF for ; Tue, 22 Oct 2019 15:31:56 +0200 (CEST) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=XlNmmi35J73zRgRGwNx8dp2Uq2GZ2BZDeWhAMW7bUFdWytK5te5I2hWtIgxWo8AZr32qj7tMGmQX6b5f1N9de1S0OKtdYDmykpFRtYigrLMX4l0J9SqXfwOexOo1qRKVDTH7n9kGrkHW/e+FHUwEos6tT2ivfRdWcW/0D2TD34S2Qh4e19QHvxkh0m+wwFroxojEMWrNNVJEvpDO7ilWDXktVULnuGTMofsHIk7V5uwlDsu0Ma5tkLWeuMehOedLhgn+VuSjhIwZzbi9vGxhI9DUi/BzUGJboTvXHs8pS9ZE0pzfmetj/l1+DJDhiKwKRdmNOKU0Owndp36wVq7EkQ== 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=ocCAAPGpW+YTSK40Q65Xzh28cYsPMOQc1TRuTREXpus=; b=axklsWdJbFDSuC5U7dYuQt8X/sUIkZ+fxZNZXCvwhuMojvWKa5qYIxdKtDaleZzjnT/cifyOalPo4tzIZ/t/yWPJJlvxZvng2AKP4+aOpjsxwVK0t4U9iM3Wp2ijacC0ZhznHR0l/qTJDxwykO9ekmp0g/NqQ98NjaJQYdQ8XYFU4ztjoROdfnNuFx0pgL4DXuPJu2FvkaaJkxQf72YAX4lQ/0xRJdrs56DPce+4PYeP1vdOqEhyyhYMf1Ni96jUg5zeD/bkMtUh3aP1PvWPBJoqDpbDqUkkYaPAaQ5LSD8LqyzqSVVSYsSGQsGgrM3Rjhd2WOP8xcQSUI/Cd2lceQ== 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=ocCAAPGpW+YTSK40Q65Xzh28cYsPMOQc1TRuTREXpus=; b=cQUlLpDGlRyQITaFt7gpIIRoLwnf+dEzOTPlTDMmUYmTZrN+fTQ6Qh6Mc9vmFSOf5uFKwcXJu+2ip71RTv+Elh1FifJv8Sr8fmiwADwO3Bhsk0il0o7XP7pdcuhjYgccrY/U/1iHL6kWt1GUNODoHdjMXiD7+7Qj3qTECCInED8= Received: from VE1PR04MB6639.eurprd04.prod.outlook.com (10.255.118.11) by VE1PR04MB6590.eurprd04.prod.outlook.com (20.179.234.29) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2367.24; Tue, 22 Oct 2019 13:31:55 +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.2367.022; Tue, 22 Oct 2019 13:31:55 +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+oMIAAcucAgAMT0cCACHlagIACi3iggATErYCAAV8CAA== Date: Tue, 22 Oct 2019 13:31:55 +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> <2601191342CEEE43887BDE71AB97725801A8C6D6AC@IRSMSX104.ger.corp.intel.com> In-Reply-To: <2601191342CEEE43887BDE71AB97725801A8C6D6AC@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-ht: Tenant x-ms-office365-filtering-correlation-id: 921abb74-10da-415f-554d-08d756f434d8 x-ms-traffictypediagnostic: VE1PR04MB6590:|VE1PR04MB6590: x-ms-exchange-purlcount: 1 x-ms-exchange-transport-forked: True x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:8882; x-forefront-prvs: 01986AE76B x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(4636009)(366004)(396003)(39860400002)(346002)(376002)(136003)(199004)(189003)(305945005)(66556008)(86362001)(74316002)(66946007)(66446008)(66476007)(64756008)(76116006)(33656002)(7110500001)(316002)(7736002)(110136005)(66066001)(54906003)(102836004)(6506007)(6246003)(55016002)(966005)(99286004)(478600001)(76176011)(9686003)(6306002)(25786009)(446003)(11346002)(6436002)(52536014)(14444005)(256004)(8936002)(476003)(14454004)(44832011)(486006)(71200400001)(71190400001)(81156014)(81166006)(4326008)(186003)(30864003)(15650500001)(3846002)(5660300002)(26005)(6116002)(2420400007)(7696005)(2906002)(229853002)(8676002)(156664002)(921003)(491001)(1121003); DIR:OUT; SFP:1101; SCL:1; SRVR:VE1PR04MB6590; H:VE1PR04MB6639.eurprd04.prod.outlook.com; FPR:; SPF:None; LANG:en; PTR:InfoNoRecords; A:1; MX: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: ukcamky3HwttB7jTz+A1Y5F6sqJwBZdqMI2+9+ILiSpgrv/gZjdE3CAzenmdY18rSGneWLZx63f0UIp8Vp8RMM3LV2/ErBeZGMSrDkpKEJjWwHlsNG+19VAVmdm2IaFGexybDZxejMp2zskor/nFthgaiv38LwcCYAmdY/ZpJFJH4hUwW3B0u84GJRyLZSsiA3YYg9xjBJIOIoOxGc/yHSVc5RQ0GpeZiw5yj5n2K2XmGjMe6ieqoZ0paejDuXFfXEkqNR0ubj7U3kisBgjpPyaBqdAQoO8i3eaFPY/8O6lqwcSOThM4dTDrmwdn3jAj0U9ArR/AgWKURyXPhIawEpTaEU3SuDA7974byPa+e/MrApUU5Dzt03lPWRMc3oYD/Sl6kWurrsCs04b5gUzIsBST0hCxeQkj+ZPiqUTYcgGNGjkXWkcXRSNd4jLZYyrXdM9/SxWKXed+ukH2L1xdQvdQo8TJ90wTse/NnV3FKSs= 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: 921abb74-10da-415f-554d-08d756f434d8 X-MS-Exchange-CrossTenant-originalarrivaltime: 22 Oct 2019 13:31:55.3017 (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: jaJvH8NVcoCm8wudUg72v0hSQukhIy8TWlYGGX6akucxn5ZHJ05vtOyimEJHsVwrVTWw+UtpQfk1acL9pyzNvg== X-MS-Exchange-Transport-CrossTenantHeadersStamped: VE1PR04MB6590 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 >=20 > > Added my comments inline with your draft. > > [snip].. > > > > > > > > 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 > > > > > > List is below. > > > Please add/correct me, if I missed something. > > > > > > Konstantin > > > > Before going into comparison, we should define the requirement as well. >=20 > Good point. >=20 > > What I understood from the patchset, > > "You need a synchronous API to perform crypto operations on raw data us= ing > 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) >=20 > Yes, this is correct, I think. >=20 > > > > 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-cryp= to > 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)(...); (*cle= ar); > /*whatever else we'll need *'}; > > - rte_crypto_cpu_sym_get_ops(const struct rte_crypto_sym_xform > *xforms) > > that would return const struct rte_crypto_cpu_sym_session_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 the union= size. >=20 > Not sure, what union you are talking about? Union of xforms in rte_security_session_conf >=20 > > 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 > > the correct place to add a piece of code or do we really agree to add c= ode > > anywhere just to avoid that breakage. >=20 > I am talking about maintaining it in future. > if your struct is not seen externally, no chances to introduce ABI breaka= ge. >=20 > > > > 2) each session entity is self-contained, user doesn't need to bring a= long > dev_id etc. > > dev_id is needed only at init stage, after that user will use ses= sion ops > to perform > > all operations on that session (process(), clear(), etc.). > > > > [Akhil] There is nothing called as session ops in current DPDK. >=20 > 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. Creating 2 diff= erent code Paths for user is not desirable for the stack developers. >=20 > > What you are proposing > > is a new concept which doesn't have any extra benefit, rather it is add= ing > complexity > > to have two different code paths for session create. > > > > > > 3) User can decide does he wants to store ops[] pointer on a per sessi= on > basis, > > or on a per group of same sessions, or... > > > > [Akhil] Will the user really care which process API should be called fr= om the > PMD. > > Rather it should be driver's responsibility to store that in the sessio= n private > data > > which would be opaque to the user. As per my suggestion same process > function can > > be added to multiple sessions or a single session can be managed inside= the > PMD. >=20 > In that case we either need to have a function per session (stored intern= ally), > 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_ops() into > one (init). Again that will be a separate API call from the user perspective which is n= ot good. >=20 > > > > > > 4) No mandatory mempools for private sessions. User can allocate > memory for cpu-crypto > > session whenever he likes. > > > > [Akhil] you mean session private data? >=20 > Yes. >=20 > > You would need that memory anyways, user will be > > allocating that already. You do not need to manage that. >=20 > What I am saying - right now user has no choice but to allocate it via me= mpool. > Which is probably not the best options for all cases. >=20 > > > > 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 th= e > > same crypto processing. Suppose a fix or a new feature(or algo) is adde= d, PMD > owner > > will need to add code in both the session create APIs. Hence more > maintenance and > > error prone. >=20 > I think majority of code for both paths will be common, plus even we'll r= euse > 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 support. > Ok to add it as 7) to the list. >=20 > > - 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 >=20 > That's the same as #5 above, I think. >=20 > > > > > > b) Your suggestion - reuse existing rte_cryptodev_sym_session_init= () and > 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 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 anoth= er > 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 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= , > existing app change. > > - might cause extra space overhead. > > > > [Akhil] It will not contradict with #1, you will only have few checks i= n the > session init PMD > > Which support this mode, find appropriate values and set the appropriat= e > 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 flexibility for t= he user. >=20 > Ok, but that's what I am saying - if PMD would *always* have to create a > session that can handle > both modes (sync/async), then user would *always* have to provide paramet= ers > for both modes too. > Otherwise if let say user didn't setup sync specific parameters at all, w= hat 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 fo= r 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=20 session is not configured properly for sync mode. Internally the PMD will not store the process() API in the session priv dat= a And while calling the first packet, devops->process will give an assert tha= t session Is not configured for sync mode. The session validation would be done in an= y case your suggestion or mine. So no extra overhead at runtime. >=20 >=20 > > > > > > 3) not possible to store device (not driver) specific data within 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 fin= d some > way to deal with it. >=20 > I don't think there is an easy way to fix that with existing API. >=20 > > > > > > 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 it would = be inline > with the > > current DPDK methodology. >=20 > 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: > http://mails.dpdk.org/archives/dev/2019-September/144674.html > 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 whichever is good for you. Whether it is ABI breakage or not, as per your requirements, this is the co= rrect approach. Do you agree with this or not? Now handling the API/ABI breakage is a separate story. In 19.11 release we= =20 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 some other solution for next release. May be having a pmd API in 20.02 and=20 converting it into formal one in 20.11 >=20 > > What you are suggesting is a new way to get the things done without muc= h > benefit. >=20 > Would help with ABI stability plus better performance, isn't it enough? >=20 > > Also I don't see any performance difference as crypto workload is heavi= er than > > Code cycles, so that wont matter. >=20 > It depends. > Suppose function call costs you ~30 cycles. > If you have burst of big packets (let say crypto for each will take ~2K c= ycles) that > belong > to the same session, then yes you wouldn't notice these extra 30 cycles a= t all. > If you have burst of small packets (let say crypto for each will take ~30= 0 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 find out t= he difference. >=20 > > 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 an= d then: > > rte_crypto_cpu_sym_process(uint8_t dev_id, rte_cryptodev_sym_se= ssion > *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_d= river_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 a= nd > 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, >=20 > I don't think it is good idea. > PMD specific API is sort of deprecated path, also there is no clean way t= o use it > within the libraries. I know that this is a deprecated path, we can use it until we are not allow= ed to break ABI/API >=20 > > because as per my understanding the solution doesn't look scalable to o= ther > PMDs. > > Your approach is aligned only to Intel , will not benefit others like o= penssl > which is used by all > > vendors. >=20 > I feel quite opposite, from my perspective majority of SW backed PMDs wil= l > 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 code, I th= ink 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 underneath, and t= hey are > session based > (AFAIK you don't need any device/queue data, everything that needed for > crypto/auth is stored inside session). >=20 By vendor specific, I mean,=20 - no PMD would like to have 2 different variants of session Init APIs for d= oing 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 one for S= W PMDs. -Akhil