Re: [dpdk-dev] [RFC PATCH 3/5] rte_security: updates and enabled security operations for ethdev

[Akhil Goyal <akhil.goyal@nxp.com>]

Hi Radu,
On 8/25/2017 8:27 PM, Radu Nicolau wrote:
> Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
> ---
>   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?
>   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 <rte_memory.h>
>   #include <rte_mempool.h>
>   #include <rte_common.h>
> -#include <rte_crypto.h>
> +#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;
	};
};

>   	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