From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 2BC607CF5 for ; Thu, 4 May 2017 18:21:29 +0200 (CEST) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 04 May 2017 09:21:13 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.38,287,1491289200"; d="scan'208";a="964433336" Received: from irsmsx154.ger.corp.intel.com ([163.33.192.96]) by orsmga003.jf.intel.com with ESMTP; 04 May 2017 09:20:47 -0700 Received: from irsmsx108.ger.corp.intel.com ([169.254.11.239]) by IRSMSX154.ger.corp.intel.com ([169.254.12.233]) with mapi id 14.03.0319.002; Thu, 4 May 2017 17:20:46 +0100 From: "De Lara Guarch, Pablo" To: Akhil Goyal , "Gonzalez Monroy, Sergio" , "dev@dpdk.org" CC: "Doherty, Declan" , "hemant.agrawal@nxp.com" , "zbigniew.bodek@caviumnetworks.com" , "jerin.jacob@caviumnetworks.com" Thread-Topic: [PATCH] [RFC] cryptodev: crypto operation restructuring Thread-Index: AQHSxJ0xL6Pe/EJJJ0KpkAj5L6uyjKHjtuWAgAACAYCAAAtTgIAAM2wAgABdX5A= Date: Thu, 4 May 2017 16:20:46 +0000 Message-ID: References: <1493402588-163123-1-git-send-email-pablo.de.lara.guarch@intel.com> <46d732a5-3ab0-a63b-bd12-acc9372a3c57@nxp.com> <058e6ed7-9404-1333-31be-c389fe08d246@intel.com> <152e5bc5-26b3-2505-f5b7-842652110641@nxp.com> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiYTMwOGM1YzQtYzlkOS00ZjY4LWJmYWMtNThiZjRkZTZjZWM2IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6Ikc4ejByWGFCeXVOMVRDYzJBRTBYMGJ1c1dkblhEN3NYc29XblY2ellLeTQ9In0= x-ctpclassification: CTP_IC x-originating-ip: [163.33.239.181] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH] [RFC] cryptodev: crypto operation restructuring 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: , X-List-Received-Date: Thu, 04 May 2017 16:21:31 -0000 > -----Original Message----- > From: Akhil Goyal [mailto:akhil.goyal@nxp.com] > Sent: Thursday, May 04, 2017 12:23 PM > To: Gonzalez Monroy, Sergio; De Lara Guarch, Pablo; dev@dpdk.org > Cc: Doherty, Declan; hemant.agrawal@nxp.com; > zbigniew.bodek@caviumnetworks.com; jerin.jacob@caviumnetworks.com > Subject: Re: [PATCH] [RFC] cryptodev: crypto operation restructuring >=20 > On 5/4/2017 1:49 PM, Sergio Gonzalez Monroy wrote: > > On 04/05/2017 08:38, Akhil Goyal wrote: > >> On 5/4/2017 1:01 PM, Sergio Gonzalez Monroy wrote: > >>> On 04/05/2017 07:09, Akhil Goyal wrote: > >>>> Hi Sergio, > >>>> > >>>> On 5/3/2017 7:48 PM, Sergio Gonzalez Monroy wrote: > >>>>> On 03/05/2017 12:01, Akhil Goyal wrote: > >>>>>> Hi Pablo, > >>>>>> > >>>>>> On 4/28/2017 11:33 PM, Pablo de Lara wrote: > >>>>>>> This is a proposal to correct and improve the current crypto > >>>>>>> operation (rte_crypto_op) > >>>>>>> and symmetric crypto operation (rte_crypto_sym_op) structures, > >>>>>>> shrinking > >>>>>>> their sizes to fit both structures into two 64-byte cache lines a= s > >>>>>>> one of the goals. > >>>>>>> > >>>>>>> The following changes are proposed: > >>>>>>> > >>>>>>> In rte_crypto_op: > >>>>>>> > >>>>>>> - Move session type (with session/sessionless) from symmetric op > to > >>>>>>> crypto op, > >>>>>>> as this could be used for other types > >>>>>>> > >>>>>>> - Combine operation type, operation status and session type into > a > >>>>>>> 64-bit flag (each one taking 1 byte), > >>>>>>> instead of having enums taking 4 bytes each > >>>>>> [Akhil] wouldn't this be a problem? Bit fields create endianness > >>>>>> issues. Can we have uint8_t for each of the field. > >>>>> > >>>>> Sure, as it is proposed it would be the same as having 3 uint8_t > >>>>> fields. > >>>>> The idea was to possibly compact those fields (ie. we do not need 8 > >>>>> bits > >>>>> for sess_type) to make better use of the bits and add asym fields > >>>>> there > >>>>> if needed. > >>>>> > >>>>> I don't think bitfields would be a problem in this case. Agree, we > >>>>> should not use both bitmask and bitfields, but we would use just > >>>>> bitfields. > >>>>> Can you elaborate on the issue you see? > >>>>> > >>>>> Regards, > >>>>> Sergio > >>>>> > >>>> > >>>> The problem will come when we run on systems with different > >>>> endianness. The bit field positioning will be different for LE and B= E. > >>>> It would be like in LE > >>>> uint64_t type:8; > >>>> uint64_t status:8; > >>>> uint64_t sess_type:8; > >>>> uint64_t reserved:40; > >>>> > >>>> and on BE it would be > >>>> uint64_t reserved:40; > >>>> uint64_t sess_type:8; > >>>> uint64_t status:8; > >>>> uint64_t type:8; > >>>> > >>>> So it would be better to use uint8_t for each of the field. > >>> > >>> Understood, but why is that an issue? Those fields are used by > >>> application code and PMD, same system. > >>> Do you have a use case where you are offloading crypto ops to a > >>> different arch/system? > >>> > >>> Sergio > >> same application may run on LE or BE machines. So if we use masks for > >> accessing these fields and take the complete field as uint64_t, then > >> LE and BE machine would interpret it differently as the code is same. > >> > > > > Right, either you use bitfields or you define macros and work on the > > complete uint64_t. The proposal here was to just use bitfields and for > > the given use case it would not be an issue while IMHO allowing better > > packing and readability than defining each field as a Byte. > > > > Anyway, if you feel strongly against bitfields, we can just define it a= s > > uint8_t as you suggest or single uint64_t with macros. > > > > Sergio > > >=20 > I am not against bitfields. But we should take care of the endianness > using the compile time flags for byte ordering or we can use the uint8_t. > I am ok with both. >=20 > Thanks, > Akhil >=20 > >> Akhil > >>> > >>>> > >>>>>>> > >>>>>>> - Remove opaque data from crypto operation, as private data can > be > >>>>>>> allocated > >>>>>>> just after the symmetric (or other type) crypto operation > >>>>>>> > >>>>>>> - Modify symmetric operation pointer to zero-array, as the > symmetric > >>>>>>> op should be always after the crypto operation > >>>>>>> > >>>>>>> In rte_crypto_sym_xform: > >>>>>>> > >>>>>>> - Remove AAD length from sym_xform (will be taken from > operation > >>>>>>> only) > >>>>>>> > >>>>>>> - Add IV length in sym_xform, so this length will be fixed for al= l > >>>>>>> the operations in a session > >>>>>> A much needed change. This would remove hard codings for iv > length > >>>>>> while configuring sessions. > >>>>>>> > >>>>>>> In rte_crypto_sym_op: > >>>>>>> > >>>>>>> - Separate IV from cipher structure in symmetric crypto > >>>>>>> operation, as > >>>>>>> it is also used in authentication, for some algorithms > >>>>>>> > >>>>>>> - Remove IV pointer and length from sym crypto op, and leave just > >>>>>>> the > >>>>>>> offset (from the beginning of the crypto operation), > >>>>>>> as the IV can reside after the crypto operation > >>>>>>> > >>>>>>> - Create union with authentication data and AAD, as these two > values > >>>>>>> cannot be used at the same time > >>>>>> [Akhil] Does this mean, in case of AEAD, additional authentication > >>>>>> data and auth data are contiguous as we do not have explicit auth > >>>>>> data > >>>>>> offset here. >=20 > Pablo/Sergio, >=20 > Is my understanding correct here? Hi Akhil, Thanks for your comments! No, AAD can be anywhere, as we are passing a pointer to the data: union { struct { uint32_t offset; uint32_t length; } data; struct { uint32_t length; uint8_t *data; phys_addr_t phys_addr; } aad; }; We don't need auth data offset/length, because data to authenticate and cip= her is passed on cipher structure, plus the aad structure for additional data t= o authenticate. Another proposal would be creating an structure in that crypto operation fo= r AEAD algorithms, and have the union with that structure and the other two structures (auth a= nd cipher). In this case, it is more explicit that AAD cannot be used in authentication= algorithms such as SHA1. struct rte_crypto_sym_op { struct rte_mbuf *m_src;=20 struct rte_mbuf *m_dst;=20 RTE_STD_C11 union { struct rte_cryptodev_sym_session *session; struct rte_crypto_sym_xform *xform; }; struct { uint32_t offset; } iv; union { struct { struct { uint32_t offset; uint32_t length; } data; /**< Data offset and length for encryption/authentication */ struct { uint32_t length; =20 uint8_t *data; phys_addr_t phys_addr;=20 } aad; /**< Additional authentication parameters */ struct { uint8_t *data; phys_addr_t phys_addr; } digest; /**< Digest parameters (length is fixed in session) */ } aead; struct { struct { struct { uint32_t offset; uint32_t length; } data; /**< Data offset and length for authentication */ struct { uint8_t *data; phys_addr_t phys_addr; } digest; /**< Digest parameters (length is fixed in session) */ } auth; struct { struct { uint32_t offset; uint32_t length;=20 } data; /**< Data offset and length for ciphering */ } cipher; }; }; __extension__ char _private[0]; /**< Private crypto operation material */ }; Thanks, Pablo