From mboxrd@z Thu Jan 1 00:00:00 1970 From: Radu Nicolau Subject: Re: [RFC PATCH 3/5] rte_security: updates and enabled security operations for ethdev Date: Tue, 29 Aug 2017 14:13:25 +0100 Message-ID: <5b7452db-e06b-bfa2-fdd6-5a0aaf1e21f7@intel.com> References: <1503673046-30651-1-git-send-email-radu.nicolau@intel.com> <1503673046-30651-4-git-send-email-radu.nicolau@intel.com> <30b3fe2f-3668-ffa7-e3a4-a19cb28c5fdf@nxp.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Cc: "hemant.agrawal@nxp.com" , Declan Doherty , Boris Pismenny To: Akhil Goyal , dev@dpdk.org Return-path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id 9A3C32BAC for ; Tue, 29 Aug 2017 15:13:43 +0200 (CEST) In-Reply-To: <30b3fe2f-3668-ffa7-e3a4-a19cb28c5fdf@nxp.com> Content-Language: en-US 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, On 8/29/2017 1:14 PM, Akhil Goyal wrote: > Hi Radu, > On 8/25/2017 8:27 PM, Radu Nicolau wrote: >> Signed-off-by: Radu Nicolau >> --- >> lib/Makefile | 1 + >> lib/librte_cryptodev/rte_cryptodev_pmd.h | 4 +-- >> lib/librte_cryptodev/rte_cryptodev_version.map | 10 ++++++++ >> lib/librte_cryptodev/rte_security.c | 34 >> +++++++++++++++++--------- >> lib/librte_cryptodev/rte_security.h | 12 ++++++--- >> 5 files changed, 44 insertions(+), 17 deletions(-) >> >> diff --git a/lib/Makefile b/lib/Makefile >> index 86caba1..08a1767 100644 >> --- a/lib/Makefile >> +++ b/lib/Makefile >> @@ -51,6 +51,7 @@ DEPDIRS-librte_ether += librte_mbuf >> DIRS-$(CONFIG_RTE_LIBRTE_CRYPTODEV) += librte_cryptodev >> DEPDIRS-librte_cryptodev := librte_eal librte_mempool librte_ring >> librte_mbuf >> DEPDIRS-librte_cryptodev += librte_kvargs >> +DEPDIRS-librte_cryptodev += librte_ether > Is the shared build working now? It does only for core dpdk (but not for the sample app), but after updating the exported symbols list it works for both. >> DIRS-$(CONFIG_RTE_LIBRTE_EVENTDEV) += librte_eventdev >> DEPDIRS-librte_eventdev := librte_eal librte_ring >> DIRS-$(CONFIG_RTE_LIBRTE_VHOST) += librte_vhost >> diff --git a/lib/librte_cryptodev/rte_cryptodev_pmd.h >> b/lib/librte_cryptodev/rte_cryptodev_pmd.h >> index 219fba6..ab3ecf7 100644 >> --- a/lib/librte_cryptodev/rte_cryptodev_pmd.h >> +++ b/lib/librte_cryptodev/rte_cryptodev_pmd.h >> @@ -371,7 +371,7 @@ struct rte_cryptodev_ops { >> * - Returns -ENOTSUP if crypto device does not support the crypto >> transform. >> * - Returns -ENOMEM if the private session could not be allocated. >> */ >> -typedef int (*security_configure_session_t)(struct rte_cryptodev *dev, >> +typedef int (*security_configure_session_t)(void *dev, >> struct rte_security_sess_conf *conf, >> struct rte_security_session *sess, >> struct rte_mempool *mp); >> @@ -382,7 +382,7 @@ typedef int >> (*security_configure_session_t)(struct rte_cryptodev *dev, >> * @param dev Crypto device pointer >> * @param sess Security session structure >> */ >> -typedef void (*security_free_session_t)(struct rte_cryptodev *dev, >> +typedef void (*security_free_session_t)(void *dev, >> struct rte_security_session *sess); >> /** Security operations function pointer table */ >> diff --git a/lib/librte_cryptodev/rte_cryptodev_version.map >> b/lib/librte_cryptodev/rte_cryptodev_version.map >> index e9ba88a..20b553e 100644 >> --- a/lib/librte_cryptodev/rte_cryptodev_version.map >> +++ b/lib/librte_cryptodev/rte_cryptodev_version.map >> @@ -79,3 +79,13 @@ DPDK_17.08 { >> rte_crypto_aead_operation_strings; >> } DPDK_17.05; >> + >> +DPDK_17.11 { >> + global: >> + >> + rte_security_session_create; >> + rte_security_session_init; >> + rte_security_attach_session; >> + rte_security_session_free; >> + >> +} DPDK_17.08; >> diff --git a/lib/librte_cryptodev/rte_security.c >> b/lib/librte_cryptodev/rte_security.c >> index 7c73c93..5f35355 100644 >> --- a/lib/librte_cryptodev/rte_security.c >> +++ b/lib/librte_cryptodev/rte_security.c >> @@ -86,8 +86,12 @@ rte_security_session_init(uint16_t dev_id, >> return -EINVAL; >> cdev = rte_cryptodev_pmd_get_dev(dev_id); >> index = cdev->driver_id; >> + if (cdev == NULL || sess == NULL || cdev->sec_ops == NULL >> + || cdev->sec_ops->session_configure == NULL) >> + return -EINVAL; >> if (sess->sess_private_data[index] == NULL) { >> - ret = cdev->sec_ops->session_configure(cdev, conf, sess, >> mp); >> + ret = cdev->sec_ops->session_configure((void *)cdev, >> + conf, sess, mp); >> if (ret < 0) { >> CDEV_LOG_ERR( >> "cdev_id %d failed to configure session details", >> @@ -100,14 +104,18 @@ rte_security_session_init(uint16_t dev_id, >> case RTE_SECURITY_SESS_ETH_PROTO_OFFLOAD: >> dev = &rte_eth_devices[dev_id]; >> index = dev->data->port_id; >> + if (dev == NULL || sess == NULL || dev->sec_ops == NULL >> + || dev->sec_ops->session_configure == NULL) >> + return -EINVAL; >> if (sess->sess_private_data[index] == NULL) { >> -// ret = dev->sec_ops->session_configure(dev, conf, sess, >> mp); >> -// if (ret < 0) { >> -// CDEV_LOG_ERR( >> -// "dev_id %d failed to configure session details", >> -// dev_id); >> -// return ret; >> -// } >> + ret = dev->sec_ops->session_configure((void *)dev, >> + conf, sess, mp); >> + if (ret < 0) { >> + CDEV_LOG_ERR( >> + "dev_id %d failed to configure session details", >> + dev_id); >> + return ret; >> + } >> } >> break; >> default: >> @@ -152,16 +160,18 @@ rte_security_session_clear(uint8_t dev_id, >> switch (action_type) { >> case RTE_SECURITY_SESS_CRYPTO_PROTO_OFFLOAD: >> cdev = rte_cryptodev_pmd_get_dev(dev_id); >> - if (cdev == NULL || sess == NULL) >> + if (cdev == NULL || sess == NULL || cdev->sec_ops == NULL >> + || cdev->sec_ops->session_clear == NULL) >> return -EINVAL; >> - cdev->sec_ops->session_clear(cdev, sess); >> + cdev->sec_ops->session_clear((void *)cdev, sess); >> break; >> case RTE_SECURITY_SESS_ETH_INLINE_CRYPTO: >> case RTE_SECURITY_SESS_ETH_PROTO_OFFLOAD: >> dev = &rte_eth_devices[dev_id]; >> - if (dev == NULL || sess == NULL) >> + if (dev == NULL || sess == NULL || dev->sec_ops == NULL >> + || dev->sec_ops->session_clear == NULL) >> return -EINVAL; >> -// dev->dev_ops->session_clear(dev, sess); >> + dev->sec_ops->session_clear((void *)dev, sess); >> break; >> default: >> return -EINVAL; >> diff --git a/lib/librte_cryptodev/rte_security.h >> b/lib/librte_cryptodev/rte_security.h >> index 9747d5e..0c8b358 100644 >> --- a/lib/librte_cryptodev/rte_security.h >> +++ b/lib/librte_cryptodev/rte_security.h >> @@ -20,7 +20,7 @@ extern "C" { >> #include >> #include >> #include >> -#include >> +#include "rte_crypto.h" >> /** IPSec protocol mode */ >> enum rte_security_conf_ipsec_sa_mode { >> @@ -70,9 +70,9 @@ struct rte_security_ipsec_tunnel_param { >> } ipv4; /**< IPv4 header parameters */ >> struct { >> - struct in6_addr *src_addr; >> + struct in6_addr src_addr; >> /**< IPv6 source address */ >> - struct in6_addr *dst_addr; >> + struct in6_addr dst_addr; >> /**< IPv6 destination address */ >> uint8_t dscp; >> /**< IPv6 Differentiated Services Code Point */ >> @@ -171,6 +171,12 @@ struct rte_security_ipsec_xform { >> uint8_t *data; /**< pointer to key data */ >> size_t length; /**< key length in bytes */ >> } auth_key; >> + enum rte_crypto_aead_algorithm aead_alg; >> + /**< AEAD Algorithm */ >> + struct { >> + uint8_t *data; /**< pointer to key data */ >> + size_t length; /**< key length in bytes */ >> + } aead_key; > I believe it would be better to use a union here. > union { > struct { > enum rte_crypto_cipher_algorithm cipher_alg; > /**< Cipher Algorithm */ > struct { > uint8_t *data; /**< pointer to key data */ > size_t length; /**< key length in bytes */ > } cipher_key; > enum rte_crypto_auth_algorithm auth_alg; > /**< Authentication Algorithm */ > struct { > uint8_t *data; /**< pointer to key data */ > size_t length; /**< key length in bytes */ > } auth_key; > }; > struct { > enum rte_crypto_aead_algorithm aead_alg; > /**< AEAD Algorithm */ > struct { > uint8_t *data; /**< pointer to key data */ > size_t length; /**< key length in bytes */ > } aead_key; > }; > }; Probably the best way will be to have a chain of transforms, I will follow up in the next patchset. > >> uint32_t salt; /**< salt for this SA */ >> enum rte_security_conf_ipsec_sa_mode mode; >> /**< IPsec SA Mode - transport/tunnel */ >> > > Thanks for the updates. I missed some of the checks. > > -Akhil