From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id 5B0DB6DB2 for ; Thu, 4 May 2017 09:31:33 +0200 (CEST) Received: from orsmga004.jf.intel.com ([10.7.209.38]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 04 May 2017 00:31:32 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.38,287,1491289200"; d="scan'208";a="83514746" Received: from smonroyx-mobl.ger.corp.intel.com (HELO [10.237.221.113]) ([10.237.221.113]) by orsmga004.jf.intel.com with ESMTP; 04 May 2017 00:31:30 -0700 To: Akhil Goyal , Pablo de Lara , dev@dpdk.org 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> Cc: declan.doherty@intel.com, hemant.agrawal@nxp.com, zbigniew.bodek@caviumnetworks.com, jerin.jacob@caviumnetworks.com From: Sergio Gonzalez Monroy Message-ID: Date: Thu, 4 May 2017 08:31:29 +0100 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 MIME-Version: 1.0 In-Reply-To: <152e5bc5-26b3-2505-f5b7-842652110641@nxp.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit 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 07:31:34 -0000 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 as >>>> 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 BE. > 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 > >>>> >>>> - 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 all >>>> 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. >>>> >>>> - Remove digest length from sym crypto op, so this length will be >>>> fixed for all the operations in a session >>>> >>>> - Add zero-array at the end of sym crypto op to be used to get extra >>>> allocated memory (IV + other user data) >>>> >>>> Previous rte_crypto_op (40 bytes) and rte_crypto_sym_op (114 bytes) >>>> structures: >>>> >>>> struct rte_crypto_op { >>>> enum rte_crypto_op_type type; >>>> >>>> enum rte_crypto_op_status status; >>>> >>>> struct rte_mempool *mempool; >>>> >>>> phys_addr_t phys_addr; >>>> >>>> void *opaque_data; >>>> >>>> union { >>>> struct rte_crypto_sym_op *sym; >>>> }; >>>> } __rte_cache_aligned; >>>> >>>> struct rte_crypto_sym_op { >>>> struct rte_mbuf *m_src; >>>> struct rte_mbuf *m_dst; >>>> >>>> enum rte_crypto_sym_op_sess_type sess_type; >>>> >>>> RTE_STD_C11 >>>> union { >>>> struct rte_cryptodev_sym_session *session; >>>> struct rte_crypto_sym_xform *xform; >>>> }; >>>> >>>> struct { >>>> struct { >>>> uint32_t offset; >>>> uint32_t length; >>>> } data; >>>> >>>> struct { >>>> uint8_t *data; >>>> phys_addr_t phys_addr; >>>> uint16_t length; >>>> } iv; >>>> } cipher; >>>> >>>> struct { >>>> struct { >>>> uint32_t offset; >>>> uint32_t length; >>>> } data; >>>> struct { >>>> uint8_t *data; >>>> phys_addr_t phys_addr; >>>> uint16_t length; >>>> } digest; /**< Digest parameters */ >>>> >>>> struct { >>>> uint8_t *data; >>>> phys_addr_t phys_addr; >>>> uint16_t length; >>>> } aad; >>>> >>>> } auth; >>>> } __rte_cache_aligned; >>>> >>>> New rte_crypto_op (24 bytes) and rte_crypto_sym_op (72 bytes) >>>> structures: >>>> >>>> struct rte_crypto_op { >>>> uint64_t type: 8; >>>> uint64_t status: 8; >>>> uint64_t sess_type: 8; >>>> >>>> struct rte_mempool *mempool; >>>> >>>> phys_addr_t phys_addr; >>>> >>>> RTE_STD_C11 >>>> union { >>>> struct rte_crypto_sym_op sym[0]; >>>> }; >>>> } __rte_cache_aligned; >>>> >>>> struct rte_crypto_sym_op { >>>> struct rte_mbuf *m_src; >>>> struct rte_mbuf *m_dst; >>>> >>>> RTE_STD_C11 >>>> union { >>>> struct rte_cryptodev_sym_session *session; >>>> struct rte_crypto_sym_xform *xform; >>>> }; >>>> >>>> struct { >>>> uint8_t offset; >>>> } iv; >>>> >>>> struct { >>>> union { >>>> struct { >>>> uint32_t offset; >>>> uint32_t length; >>>> } data; >>>> struct { >>>> uint32_t length; >>>> uint8_t *data; >>>> phys_addr_t phys_addr; >>>> } aad; >>>> }; >>>> >>>> struct { >>>> uint8_t *data; >>>> phys_addr_t phys_addr; >>>> } digest; >>>> >>>> } auth; >>>> struct { >>>> struct { >>>> uint32_t offset; >>>> uint32_t length; >>>> } data; >>>> >>>> } cipher; >>>> >>>> __extension__ char _private[0]; >>>> }; >>>> >>>> Signed-off-by: Pablo de Lara >>>> --- >>> >>> Comments inline. >>> >>> Regards, >>> Akhil >>> >>> >> >> > >