[dpdk-dev,01/11] lib/rte_security: add security library

  rte_security library provides APIs for security session
create/free for protocol offload or offloaded crypto
operation to ethernet device.

Signed-off-by: Akhil Goyal <akhil.goyal@nxp.com>
Signed-off-by: Boris Pismenny <borisp@mellanox.com>
Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
Signed-off-by: Declan Doherty <declan.doherty@intel.com>
---
 lib/librte_security/Makefile                 |  53 +++
 lib/librte_security/rte_security.c           | 252 ++++++++++++++
 lib/librte_security/rte_security.h           | 494 +++++++++++++++++++++++++++
 lib/librte_security/rte_security_driver.h    | 181 ++++++++++
 lib/librte_security/rte_security_version.map |  13 +
 5 files changed, 993 insertions(+)
 create mode 100644 lib/librte_security/Makefile
 create mode 100644 lib/librte_security/rte_security.c
 create mode 100644 lib/librte_security/rte_security.h
 create mode 100644 lib/librte_security/rte_security_driver.h
 create mode 100644 lib/librte_security/rte_security_version.map
  

Hi,

On 9/14/2017 1:56 PM, Akhil Goyal wrote:
<snip>..

> diff --git a/lib/librte_security/rte_security.c b/lib/librte_security/rte_security.c
> new file mode 100644
> index 0000000..5776246
> --- /dev/null
> +++ b/lib/librte_security/rte_security.c
> @@ -0,0 +1,252 @@
> +/*-
> + *   BSD LICENSE
> + *
> + *   Copyright 2017 NXP.
> + *   Copyright(c) 2017 Intel Corporation. All rights reserved.
> + *
> + *   Redistribution and use in source and binary forms, with or without
> + *   modification, are permitted provided that the following conditions
> + *   are met:
> + *
> + *     * Redistributions of source code must retain the above copyright
> + *       notice, this list of conditions and the following disclaimer.
> + *     * Redistributions in binary form must reproduce the above copyright
> + *       notice, this list of conditions and the following disclaimer in
> + *       the documentation and/or other materials provided with the
> + *       distribution.
> + *     * Neither the name of NXP nor the names of its
> + *       contributors may be used to endorse or promote products derived
> + *       from this software without specific prior written permission.
> + *
> + *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#include <rte_malloc.h>
> +#include <rte_dev.h>
> +
> +#include "rte_security.h"
> +#include "rte_security_driver.h"
> +
> +#define RTE_SECURITY_INSTANCES_BLOCK_ALLOC_SZ	(8)
> +
> +struct rte_security_ctx {
> +	uint16_t id;
> +	enum {
> +		RTE_SECURITY_INSTANCE_INVALID = 0,
> +		RTE_SECURITY_INSTANCE_VALID
> +	} state;
> +	void *device;
> +	struct rte_security_ops *ops;
> +};
> +
> +static struct rte_security_ctx *security_instances;
> +static uint16_t max_nb_security_instances;
> +static uint16_t nb_security_instances;
> +
> +static int
> +rte_security_is_valid_id(uint16_t id)
> +{
> +	if (id >= nb_security_instances ||
> +	    (security_instances[id].state != RTE_SECURITY_INSTANCE_VALID))
> +		return 0;
> +	else
> +		return 1;
> +}
> +
> +/* Macros to check for valid id */
> +#define RTE_SEC_VALID_ID_OR_ERR_RET(id, retval) do { \
> +	if (!rte_security_is_valid_id(id)) { \
> +		RTE_PMD_DEBUG_TRACE("Invalid sec_id=%d\n", id); \
> +		return retval; \
> +	} \
> +} while (0)
> +
> +#define RTE_SEC_VALID_ID_OR_RET(id) do { \
> +	if (!rte_security_is_valid_id(id)) { \
> +		RTE_PMD_DEBUG_TRACE("Invalid sec_id=%d\n", id); \
> +		return; \
> +	} \
> +} while (0)
> +
> +int
> +rte_security_register(uint16_t *id, void *device,
> +		      struct rte_security_ops *ops)
> +{
> +	if (max_nb_security_instances == 0) {
> +		security_instances = rte_malloc(
> +				"rte_security_instances_ops",
> +				sizeof(*security_instances) *
> +				RTE_SECURITY_INSTANCES_BLOCK_ALLOC_SZ, 0);
> +
> +		if (security_instances == NULL)
> +			return -ENOMEM;
> +		max_nb_security_instances =
> +				RTE_SECURITY_INSTANCES_BLOCK_ALLOC_SZ;
> +	} else if (nb_security_instances >= max_nb_security_instances) {
> +		uint16_t *instances = rte_realloc(security_instances,
> +				sizeof(struct rte_security_ops *) *
> +				(max_nb_security_instances +
> +				RTE_SECURITY_INSTANCES_BLOCK_ALLOC_SZ), 0);

I think "RTE_SECURITY_INSTANCES_BLOCK_ALLOC_SZ" value as 8 is relatively 
small. you may want to keep it "64" or more.

you may change it into two parts
- Initial block size and incremental block size for realloc.

Also, do you want to make it a configurable variable. as some 
implementation may need really large number of SAs.

> +
> +		if (instances == NULL)
> +			return -ENOMEM;
> +
> +		max_nb_security_instances +=
> +				RTE_SECURITY_INSTANCES_BLOCK_ALLOC_SZ;
> +	}
> +
> +	*id = nb_security_instances++;
> +
> +	security_instances[*id].id = *id;
> +	security_instances[*id].state = RTE_SECURITY_INSTANCE_VALID;
> +	security_instances[*id].device = device;
> +	security_instances[*id].ops = ops;
> +
> +	return 0;
> +}
> +
> +int
> +rte_security_unregister(__rte_unused uint16_t *id)
> +{
> +	/* To be implemented */
> +	return 0;
> +}
> +
> +struct rte_security_session *
> +rte_security_session_create(uint16_t id,
> +			    struct rte_security_session_conf *conf,
> +			    struct rte_mempool *mp)
> +{
> +	struct rte_security_ctx *instance;
> +	struct rte_security_session *sess = NULL;
> +
> +	RTE_SEC_VALID_ID_OR_ERR_RET(id, NULL);
> +	instance = &security_instances[id];
> +
> +	if (conf == NULL)
> +		return NULL;
> +
> +	if (rte_mempool_get(mp, (void *)&sess))
> +		return NULL;
> +
> +	RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->session_create, NULL);

it will leak the sess memory, if returned on error.

> +	if (instance->ops->session_create(instance->device, conf, sess, mp)) {
> +		rte_mempool_put(mp, (void *)sess);
> +		return NULL;
> +	}

can the mempool operations be part of session_create api?

it will be similar to destroy, which is expected to free the 'sess' 
object to mempool?

> +	return sess;
> +}
> +

<snip>..

> diff --git a/lib/librte_security/rte_security.h b/lib/librte_security/rte_security.h
> new file mode 100644
> index 0000000..2faac96
> --- /dev/null
> +++ b/lib/librte_security/rte_security.h
> @@ -0,0 +1,494 @@
> +/*-
> + *   BSD LICENSE
> + *
> + *   Copyright 2017 NXP.
> + *   Copyright(c) 2017 Intel Corporation. All rights reserved.
> + *
> + *   Redistribution and use in source and binary forms, with or without
> + *   modification, are permitted provided that the following conditions
> + *   are met:
> + *
> + *     * Redistributions of source code must retain the above copyright
> + *       notice, this list of conditions and the following disclaimer.
> + *     * Redistributions in binary form must reproduce the above copyright
> + *       notice, this list of conditions and the following disclaimer in
> + *       the documentation and/or other materials provided with the
> + *       distribution.
> + *     * Neither the name of NXP nor the names of its
> + *       contributors may be used to endorse or promote products derived
> + *       from this software without specific prior written permission.
> + *
> + *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#ifndef _RTE_SECURITY_H_
> +#define _RTE_SECURITY_H_
> +
> +/**
> + * @file rte_security.h
> + *
> + * RTE Security Common Definitions
> + *
> + */
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +#include <netinet/in.h>
> +#include <netinet/ip.h>
> +#include <netinet/ip6.h>
> +
> +#include <rte_mbuf.h>
> +#include <rte_memory.h>
> +#include <rte_mempool.h>
> +#include <rte_common.h>
> +#include <rte_crypto.h>
> +
> +/** IPSec protocol mode */
> +enum rte_security_ipsec_sa_mode {
> +	RTE_SECURITY_IPSEC_SA_MODE_TRANSPORT,
> +	/**< IPSec Transport mode */
> +	RTE_SECURITY_IPSEC_SA_MODE_TUNNEL,
> +	/**< IPSec Tunnel mode */
> +};
> +
> +/** IPSec Protocol */
> +enum rte_security_ipsec_sa_protocol {
> +	RTE_SECURITY_IPSEC_SA_PROTO_AH,
> +	/**< AH protocol */
> +	RTE_SECURITY_IPSEC_SA_PROTO_ESP,
> +	/**< ESP protocol */
> +};
> +
> +/** IPSEC tunnel type */
> +enum rte_security_ipsec_tunnel_type {
> +	RTE_SECURITY_IPSEC_TUNNEL_IPV4 = 0,
> +	/**< Outer header is IPv4 */
> +	RTE_SECURITY_IPSEC_TUNNEL_IPV6,
> +	/**< Outer header is IPv6 */
> +};
> +
> +/**
> + * IPSEC tunnel parameters
> + *
> + * These parameters are used to build outbound tunnel headers.
> + */
> +struct rte_security_ipsec_tunnel_param {
> +	enum rte_security_ipsec_tunnel_type type;
> +	/**< Tunnel type: IPv4 or IPv6 */
> +	union {
> +		struct {
> +			struct in_addr src_ip;
> +			/**< IPv4 source address */
> +			struct in_addr dst_ip;
> +			/**< IPv4 destination address */
> +			uint8_t dscp;
> +			/**< IPv4 Differentiated Services Code Point */
> +			uint8_t df;
> +			/**< IPv4 Don't Fragment bit */
> +			uint8_t ttl;
> +			/**< IPv4 Time To Live */
> +		} ipv4;
> +		/**< IPv4 header parameters */
> +		struct {
> +			struct in6_addr src_addr;
> +			/**< IPv6 source address */
> +			struct in6_addr dst_addr;
> +			/**< IPv6 destination address */
> +			uint8_t dscp;
> +			/**< IPv6 Differentiated Services Code Point */
> +			uint32_t flabel;
> +			/**< IPv6 flow label */
> +			uint8_t hlimit;
> +			/**< IPv6 hop limit */
> +		} ipv6;
> +		/**< IPv6 header parameters */
> +	};
> +};
> +
> +/**
> + * IPsec Security Association option flags
> + */
> +struct rte_security_ipsec_sa_options {
> +	/** Extended Sequence Numbers (ESN)
> +	  *
> +	  * * 1: Use extended (64 bit) sequence numbers
> +	  * * 0: Use normal sequence numbers
> +	  */
> +	uint32_t esn : 1;
> +
> +	/** UDP encapsulation
> +	  *
> +	  * * 1: Do UDP encapsulation/decapsulation so that IPSEC packets can
> +	  *      traverse through NAT boxes.
> +	  * * 0: No UDP encapsulation
> +	  */
> +	uint32_t udp_encap : 1;
> +
> +	/** Copy DSCP bits
> +	  *
> +	  * * 1: Copy IPv4 or IPv6 DSCP bits from inner IP header to
> +	  *      the outer IP header in encapsulation, and vice versa in
> +	  *      decapsulation.
> +	  * * 0: Use values from odp_ipsec_tunnel_param_t in encapsulation and
> +	  *      do not change DSCP field in decapsulation.
> +	  */
> +	uint32_t copy_dscp : 1;
> +
> +	/** Copy IPv6 Flow Label
> +	  *
> +	  * * 1: Copy IPv6 flow label from inner IPv6 header to the
> +	  *      outer IPv6 header.
> +	  * * 0: Use value from odp_ipsec_tunnel_param_t
> +	  */
> +	uint32_t copy_flabel : 1;
> +
> +	/** Copy IPv4 Don't Fragment bit
> +	  *
> +	  * * 1: Copy the DF bit from the inner IPv4 header to the outer
> +	  *      IPv4 header.
> +	  * * 0: Use value from odp_ipsec_tunnel_param_t
> +	  */
> +	uint32_t copy_df : 1;
> +
> +	/** Decrement inner packet Time To Live (TTL) field
> +	  *
> +	  * * 1: In tunnel mode, decrement inner packet IPv4 TTL or
> +	  *      IPv6 Hop Limit after tunnel decapsulation, or before tunnel
> +	  *      encapsulation.
> +	  * * 0: Inner packet is not modified.
> +	  */
> +	uint32_t dec_ttl : 1;
> +
> +	/** HW constructs/removes trailer of packets
> +	  *
> +	  * * 1: Transmitted packets will have the trailer added to them by
> +	  *      hardawre. The next protocol field will be based on the
> +	  *      mbuf->inner_esp_next_proto field.
> +	  *      Received packets have no trailer, the next protocol field is
> +	  *      supplied in the mbuf->inner_esp_next_proto field.
> +	  * * 0: Inner packet is not modified.
> +	  */
> +	uint32_t no_trailer : 1;
> +};
> +
> +/** IPSec security association direction */
> +enum rte_security_ipsec_sa_direction {
> +	RTE_SECURITY_IPSEC_SA_DIR_EGRESS,
> +	/**< Encrypt and generate digest */
> +	RTE_SECURITY_IPSEC_SA_DIR_INGRESS,
> +	/**< Verify digest and decrypt */
> +};
> +
> +/**
> + * IPsec security association configuration data.
> + *
> + * This structure contains data required to create an IPsec SA security session.
> + */
> +struct rte_security_ipsec_xform {
> +	uint32_t spi;
> +	/**< SA security parameter index */
> +	uint32_t salt;
> +	/**< SA salt */
> +	struct rte_security_ipsec_sa_options options;
> +	/**< various SA options */
> +	enum rte_security_ipsec_sa_direction direction;
> +	/**< IPSec SA Direction - Egress/Ingress */
> +	enum rte_security_ipsec_sa_protocol proto;
> +	/**< IPsec SA Protocol - AH/ESP */
> +	enum rte_security_ipsec_sa_mode mode;
> +	/**< IPsec SA Mode - transport/tunnel */
> +	struct rte_security_ipsec_tunnel_param tunnel;
> +	/**< Tunnel parameters, NULL for transport mode */
> +};
> +
> +/**
> + * MACsec security session configuration
> + */
> +struct rte_security_macsec_xform {
> +	/** To be Filled */
> +};
> +
> +/**
> + * Security session action type.
> + */
> +enum rte_security_session_action_type {
> +	RTE_SECURITY_ACTION_TYPE_NONE,
> +	/**< No security actions */

This is not being used, it seems that you are only using it as marker to 
indicate end of capability set?

> +	RTE_SECURITY_ACTION_TYPE_INLINE_CRYPTO,
> +	/**< Crypto processing for security protocol is processed inline
> +	 * during transmission */
> +	RTE_SECURITY_ACTION_TYPE_INLINE_PROTOCOL,
> +	/**< All security protocol processing is performed inline during
> +	 * transmission */
> +	RTE_SECURITY_ACTION_TYPE_LOOKASIDE_PROTOCOL
> +	/**< All security protocol processing including crypto is performed
> +	 * on a lookaside accelerator */
> +};
> +
> +/** Security session protocol definition */
> +enum rte_security_session_protocol {
> +	RTE_SECURITY_PROTOCOL_IPSEC,
> +	/**< IPsec Protocol */
> +	RTE_SECURITY_PROTOCOL_MACSEC,
> +	/**< MACSec Protocol */
> +};
> +
> +/**
> + * Security session configuration
> + */
> +struct rte_security_session_conf {
> +	enum rte_security_session_action_type action_type;
> +	/**< Type of action to be performed on the session */
> +	enum rte_security_session_protocol protocol;
> +	/**< Security protocol to be configured */
> +	union {
> +		struct rte_security_ipsec_xform ipsec;
> +		struct rte_security_macsec_xform macsec;
> +	};
> +	/**< Configuration parameters for security session */
> +	struct rte_crypto_sym_xform *crypto_xform;
> +	/**< Security Session Crypto Transformations */
> +};
> +
> +struct rte_security_session {
> +	__extension__ void *sess_private_data;
> +	/**< Private session material */
> +};
> +


Do you need specific error handling for security sessions as well?
In case of full protocol offloads, you will need indications for
1. SEQ number overflow (egress side, if the SA is not refreshed on time)
2. Anti replay window config and err handlings?


> +/**
> + * Create security session as specified by the session configuration
> + *
> + * @param   id		security instance identifier id
> + * @param   conf	session configuration parameters

fix the indentation here and other places in this file.

> + * @param   mp		mempool to allocate session objects from
> + * @return
> + *  - On success, pointer to session
> + *  - On failure, NULL
> + */
> +struct rte_security_session *
> +rte_security_session_create(uint16_t id,
> +			    struct rte_security_session_conf *conf,
> +			    struct rte_mempool *mp);
> +
> +/**
  

Hi Hemant,

On 9/15/2017 8:33 AM, Hemant Agrawal wrote:
> 
> Hi,
> 
> On 9/14/2017 1:56 PM, Akhil Goyal wrote:
> <snip>..
> 
> > diff --git a/lib/librte_security/rte_security.c
> > b/lib/librte_security/rte_security.c
> > new file mode 100644
> > index 0000000..5776246
> > --- /dev/null
> > +++ b/lib/librte_security/rte_security.c
> > @@ -0,0 +1,252 @@
> > +/*-
> > + *   BSD LICENSE
> > + *
> > + *   Copyright 2017 NXP.
> > + *   Copyright(c) 2017 Intel Corporation. All rights reserved.
> > + *
> > + *   Redistribution and use in source and binary forms, with or without
> > + *   modification, are permitted provided that the following conditions
> > + *   are met:
> > + *
> > + *     * Redistributions of source code must retain the above copyright
> > + *       notice, this list of conditions and the following disclaimer.
> > + *     * Redistributions in binary form must reproduce the above copyright
> > + *       notice, this list of conditions and the following disclaimer in
> > + *       the documentation and/or other materials provided with the
> > + *       distribution.
> > + *     * Neither the name of NXP nor the names of its
> > + *       contributors may be used to endorse or promote products derived
> > + *       from this software without specific prior written permission.
> > + *
> > + *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
> CONTRIBUTORS
> > + *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT
> NOT
> > + *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND
> FITNESS FOR
> > + *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE
> COPYRIGHT
> > + *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT,
> INCIDENTAL,
> > + *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING,
> BUT NOT
> > + *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
> LOSS OF USE,
> > + *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED
> AND ON ANY
> > + *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR
> TORT
> > + *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
> OF THE USE
> > + *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH
> DAMAGE.
> > + */
> > +
> > +#include <rte_malloc.h>
> > +#include <rte_dev.h>
> > +
> > +#include "rte_security.h"
> > +#include "rte_security_driver.h"
> > +
> > +#define RTE_SECURITY_INSTANCES_BLOCK_ALLOC_SZ	(8)
> > +
> > +struct rte_security_ctx {
> > +	uint16_t id;
> > +	enum {
> > +		RTE_SECURITY_INSTANCE_INVALID = 0,
> > +		RTE_SECURITY_INSTANCE_VALID
> > +	} state;
> > +	void *device;
> > +	struct rte_security_ops *ops;
> > +};
> > +
> > +static struct rte_security_ctx *security_instances; static uint16_t
> > +max_nb_security_instances; static uint16_t nb_security_instances;
> > +
> > +static int
> > +rte_security_is_valid_id(uint16_t id) {
> > +	if (id >= nb_security_instances ||
> > +	    (security_instances[id].state != RTE_SECURITY_INSTANCE_VALID))
> > +		return 0;
> > +	else
> > +		return 1;
> > +}
> > +
> > +/* Macros to check for valid id */
> > +#define RTE_SEC_VALID_ID_OR_ERR_RET(id, retval) do { \
> > +	if (!rte_security_is_valid_id(id)) { \
> > +		RTE_PMD_DEBUG_TRACE("Invalid sec_id=%d\n", id); \
> > +		return retval; \
> > +	} \
> > +} while (0)
> > +
> > +#define RTE_SEC_VALID_ID_OR_RET(id) do { \
> > +	if (!rte_security_is_valid_id(id)) { \
> > +		RTE_PMD_DEBUG_TRACE("Invalid sec_id=%d\n", id); \
> > +		return; \
> > +	} \
> > +} while (0)
> > +
> > +int
> > +rte_security_register(uint16_t *id, void *device,
> > +		      struct rte_security_ops *ops) {
> > +	if (max_nb_security_instances == 0) {
> > +		security_instances = rte_malloc(
> > +				"rte_security_instances_ops",
> > +				sizeof(*security_instances) *
> > +
> 	RTE_SECURITY_INSTANCES_BLOCK_ALLOC_SZ, 0);
> > +
> > +		if (security_instances == NULL)
> > +			return -ENOMEM;
> > +		max_nb_security_instances =
> > +
> 	RTE_SECURITY_INSTANCES_BLOCK_ALLOC_SZ;
> > +	} else if (nb_security_instances >= max_nb_security_instances) {
> > +		uint16_t *instances = rte_realloc(security_instances,
> > +				sizeof(struct rte_security_ops *) *
> > +				(max_nb_security_instances +
> > +
> 	RTE_SECURITY_INSTANCES_BLOCK_ALLOC_SZ), 0);
> 
> I think "RTE_SECURITY_INSTANCES_BLOCK_ALLOC_SZ" value as 8 is relatively
> small. you may want to keep it "64" or more.
> 
> you may change it into two parts
> - Initial block size and incremental block size for realloc.
> 

Shouldn't the resize be double the original size to get the amortized O(1)?

> Also, do you want to make it a configurable variable. as some
> implementation may need really large number of SAs.
> 
> > +
> > +		if (instances == NULL)
> > +			return -ENOMEM;
> > +
> > +		max_nb_security_instances +=
> > +
> 	RTE_SECURITY_INSTANCES_BLOCK_ALLOC_SZ;
> > +	}
> > +
> > +	*id = nb_security_instances++;
> > +
> > +	security_instances[*id].id = *id;
> > +	security_instances[*id].state = RTE_SECURITY_INSTANCE_VALID;
> > +	security_instances[*id].device = device;
> > +	security_instances[*id].ops = ops;
> > +
> > +	return 0;
> > +}
> > +
> > +int
> > +rte_security_unregister(__rte_unused uint16_t *id) {
> > +	/* To be implemented */
> > +	return 0;
> > +}
> > +
> > +struct rte_security_session *
> > +rte_security_session_create(uint16_t id,
> > +			    struct rte_security_session_conf *conf,
> > +			    struct rte_mempool *mp)
> > +{
> > +	struct rte_security_ctx *instance;
> > +	struct rte_security_session *sess = NULL;
> > +
> > +	RTE_SEC_VALID_ID_OR_ERR_RET(id, NULL);
> > +	instance = &security_instances[id];
> > +
> > +	if (conf == NULL)
> > +		return NULL;
> > +
> > +	if (rte_mempool_get(mp, (void *)&sess))
> > +		return NULL;
> > +
> > +	RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->session_create,
> NULL);
> 
> it will leak the sess memory, if returned on error.
> 
> > +	if (instance->ops->session_create(instance->device, conf, sess, mp))
> {
> > +		rte_mempool_put(mp, (void *)sess);
> > +		return NULL;
> > +	}
> 
> can the mempool operations be part of session_create api?
> 
> it will be similar to destroy, which is expected to free the 'sess'
> object to mempool?
> 
> > +	return sess;
> > +}
> > +
> 
> <snip>..
> 
> > diff --git a/lib/librte_security/rte_security.h
> > b/lib/librte_security/rte_security.h
> > new file mode 100644
> > index 0000000..2faac96
> > --- /dev/null
> > +++ b/lib/librte_security/rte_security.h
> > @@ -0,0 +1,494 @@
> > +/*-
> > + *   BSD LICENSE
> > + *
> > + *   Copyright 2017 NXP.
> > + *   Copyright(c) 2017 Intel Corporation. All rights reserved.
> > + *
> > + *   Redistribution and use in source and binary forms, with or without
> > + *   modification, are permitted provided that the following conditions
> > + *   are met:
> > + *
> > + *     * Redistributions of source code must retain the above copyright
> > + *       notice, this list of conditions and the following disclaimer.
> > + *     * Redistributions in binary form must reproduce the above copyright
> > + *       notice, this list of conditions and the following disclaimer in
> > + *       the documentation and/or other materials provided with the
> > + *       distribution.
> > + *     * Neither the name of NXP nor the names of its
> > + *       contributors may be used to endorse or promote products derived
> > + *       from this software without specific prior written permission.
> > + *
> > + *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
> CONTRIBUTORS
> > + *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT
> NOT
> > + *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND
> FITNESS FOR
> > + *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE
> COPYRIGHT
> > + *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT,
> INCIDENTAL,
> > + *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING,
> BUT NOT
> > + *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
> LOSS OF USE,
> > + *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED
> AND ON ANY
> > + *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR
> TORT
> > + *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
> OF THE USE
> > + *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH
> DAMAGE.
> > + */
> > +
> > +#ifndef _RTE_SECURITY_H_
> > +#define _RTE_SECURITY_H_
> > +
> > +/**
> > + * @file rte_security.h
> > + *
> > + * RTE Security Common Definitions
> > + *
> > + */
> > +
> > +#ifdef __cplusplus
> > +extern "C" {
> > +#endif
> > +
> > +#include <netinet/in.h>
> > +#include <netinet/ip.h>
> > +#include <netinet/ip6.h>
> > +
> > +#include <rte_mbuf.h>
> > +#include <rte_memory.h>
> > +#include <rte_mempool.h>
> > +#include <rte_common.h>
> > +#include <rte_crypto.h>
> > +
> > +/** IPSec protocol mode */
> > +enum rte_security_ipsec_sa_mode {
> > +	RTE_SECURITY_IPSEC_SA_MODE_TRANSPORT,
> > +	/**< IPSec Transport mode */
> > +	RTE_SECURITY_IPSEC_SA_MODE_TUNNEL,
> > +	/**< IPSec Tunnel mode */
> > +};
> > +
> > +/** IPSec Protocol */
> > +enum rte_security_ipsec_sa_protocol {
> > +	RTE_SECURITY_IPSEC_SA_PROTO_AH,
> > +	/**< AH protocol */
> > +	RTE_SECURITY_IPSEC_SA_PROTO_ESP,
> > +	/**< ESP protocol */
> > +};
> > +
> > +/** IPSEC tunnel type */
> > +enum rte_security_ipsec_tunnel_type {
> > +	RTE_SECURITY_IPSEC_TUNNEL_IPV4 = 0,
> > +	/**< Outer header is IPv4 */
> > +	RTE_SECURITY_IPSEC_TUNNEL_IPV6,
> > +	/**< Outer header is IPv6 */
> > +};
> > +
> > +/**
> > + * IPSEC tunnel parameters
> > + *
> > + * These parameters are used to build outbound tunnel headers.
> > + */
> > +struct rte_security_ipsec_tunnel_param {
> > +	enum rte_security_ipsec_tunnel_type type;
> > +	/**< Tunnel type: IPv4 or IPv6 */
> > +	union {
> > +		struct {
> > +			struct in_addr src_ip;
> > +			/**< IPv4 source address */
> > +			struct in_addr dst_ip;
> > +			/**< IPv4 destination address */
> > +			uint8_t dscp;
> > +			/**< IPv4 Differentiated Services Code Point */
> > +			uint8_t df;
> > +			/**< IPv4 Don't Fragment bit */
> > +			uint8_t ttl;
> > +			/**< IPv4 Time To Live */
> > +		} ipv4;
> > +		/**< IPv4 header parameters */
> > +		struct {
> > +			struct in6_addr src_addr;
> > +			/**< IPv6 source address */
> > +			struct in6_addr dst_addr;
> > +			/**< IPv6 destination address */
> > +			uint8_t dscp;
> > +			/**< IPv6 Differentiated Services Code Point */
> > +			uint32_t flabel;
> > +			/**< IPv6 flow label */
> > +			uint8_t hlimit;
> > +			/**< IPv6 hop limit */
> > +		} ipv6;
> > +		/**< IPv6 header parameters */
> > +	};
> > +};
> > +
> > +/**
> > + * IPsec Security Association option flags  */ struct
> > +rte_security_ipsec_sa_options {
> > +	/** Extended Sequence Numbers (ESN)
> > +	  *
> > +	  * * 1: Use extended (64 bit) sequence numbers
> > +	  * * 0: Use normal sequence numbers
> > +	  */
> > +	uint32_t esn : 1;
> > +
> > +	/** UDP encapsulation
> > +	  *
> > +	  * * 1: Do UDP encapsulation/decapsulation so that IPSEC packets
> can
> > +	  *      traverse through NAT boxes.
> > +	  * * 0: No UDP encapsulation
> > +	  */
> > +	uint32_t udp_encap : 1;
> > +
> > +	/** Copy DSCP bits
> > +	  *
> > +	  * * 1: Copy IPv4 or IPv6 DSCP bits from inner IP header to
> > +	  *      the outer IP header in encapsulation, and vice versa in
> > +	  *      decapsulation.
> > +	  * * 0: Use values from odp_ipsec_tunnel_param_t in encapsulation
> and
> > +	  *      do not change DSCP field in decapsulation.
> > +	  */
> > +	uint32_t copy_dscp : 1;
> > +
> > +	/** Copy IPv6 Flow Label
> > +	  *
> > +	  * * 1: Copy IPv6 flow label from inner IPv6 header to the
> > +	  *      outer IPv6 header.
> > +	  * * 0: Use value from odp_ipsec_tunnel_param_t
> > +	  */
> > +	uint32_t copy_flabel : 1;
> > +
> > +	/** Copy IPv4 Don't Fragment bit
> > +	  *
> > +	  * * 1: Copy the DF bit from the inner IPv4 header to the outer
> > +	  *      IPv4 header.
> > +	  * * 0: Use value from odp_ipsec_tunnel_param_t
> > +	  */
> > +	uint32_t copy_df : 1;
> > +
> > +	/** Decrement inner packet Time To Live (TTL) field
> > +	  *
> > +	  * * 1: In tunnel mode, decrement inner packet IPv4 TTL or
> > +	  *      IPv6 Hop Limit after tunnel decapsulation, or before tunnel
> > +	  *      encapsulation.
> > +	  * * 0: Inner packet is not modified.
> > +	  */
> > +	uint32_t dec_ttl : 1;
> > +
> > +	/** HW constructs/removes trailer of packets
> > +	  *
> > +	  * * 1: Transmitted packets will have the trailer added to them by
> > +	  *      hardawre. The next protocol field will be based on the
> > +	  *      mbuf->inner_esp_next_proto field.
> > +	  *      Received packets have no trailer, the next protocol field is
> > +	  *      supplied in the mbuf->inner_esp_next_proto field.
> > +	  * * 0: Inner packet is not modified.
> > +	  */
> > +	uint32_t no_trailer : 1;
> > +};
> > +
> > +/** IPSec security association direction */ enum
> > +rte_security_ipsec_sa_direction {
> > +	RTE_SECURITY_IPSEC_SA_DIR_EGRESS,
> > +	/**< Encrypt and generate digest */
> > +	RTE_SECURITY_IPSEC_SA_DIR_INGRESS,
> > +	/**< Verify digest and decrypt */
> > +};
> > +
> > +/**
> > + * IPsec security association configuration data.
> > + *
> > + * This structure contains data required to create an IPsec SA security
> session.
> > + */
> > +struct rte_security_ipsec_xform {
> > +	uint32_t spi;
> > +	/**< SA security parameter index */
> > +	uint32_t salt;
> > +	/**< SA salt */
> > +	struct rte_security_ipsec_sa_options options;
> > +	/**< various SA options */
> > +	enum rte_security_ipsec_sa_direction direction;
> > +	/**< IPSec SA Direction - Egress/Ingress */
> > +	enum rte_security_ipsec_sa_protocol proto;
> > +	/**< IPsec SA Protocol - AH/ESP */
> > +	enum rte_security_ipsec_sa_mode mode;
> > +	/**< IPsec SA Mode - transport/tunnel */
> > +	struct rte_security_ipsec_tunnel_param tunnel;
> > +	/**< Tunnel parameters, NULL for transport mode */ };
> > +
> > +/**
> > + * MACsec security session configuration  */ struct
> > +rte_security_macsec_xform {
> > +	/** To be Filled */
> > +};
> > +
> > +/**
> > + * Security session action type.
> > + */
> > +enum rte_security_session_action_type {
> > +	RTE_SECURITY_ACTION_TYPE_NONE,
> > +	/**< No security actions */
> 
> This is not being used, it seems that you are only using it as marker to indicate
> end of capability set?
> 
> > +	RTE_SECURITY_ACTION_TYPE_INLINE_CRYPTO,
> > +	/**< Crypto processing for security protocol is processed inline
> > +	 * during transmission */
> > +	RTE_SECURITY_ACTION_TYPE_INLINE_PROTOCOL,
> > +	/**< All security protocol processing is performed inline during
> > +	 * transmission */
> > +	RTE_SECURITY_ACTION_TYPE_LOOKASIDE_PROTOCOL
> > +	/**< All security protocol processing including crypto is performed
> > +	 * on a lookaside accelerator */
> > +};
> > +
> > +/** Security session protocol definition */ enum
> > +rte_security_session_protocol {
> > +	RTE_SECURITY_PROTOCOL_IPSEC,
> > +	/**< IPsec Protocol */
> > +	RTE_SECURITY_PROTOCOL_MACSEC,
> > +	/**< MACSec Protocol */
> > +};
> > +
> > +/**
> > + * Security session configuration
> > + */
> > +struct rte_security_session_conf {
> > +	enum rte_security_session_action_type action_type;
> > +	/**< Type of action to be performed on the session */
> > +	enum rte_security_session_protocol protocol;
> > +	/**< Security protocol to be configured */
> > +	union {
> > +		struct rte_security_ipsec_xform ipsec;
> > +		struct rte_security_macsec_xform macsec;
> > +	};
> > +	/**< Configuration parameters for security session */
> > +	struct rte_crypto_sym_xform *crypto_xform;
> > +	/**< Security Session Crypto Transformations */ };
> > +
> > +struct rte_security_session {
> > +	__extension__ void *sess_private_data;
> > +	/**< Private session material */
> > +};
> > +
> 
> 
> Do you need specific error handling for security sessions as well?
> In case of full protocol offloads, you will need indications for 1. SEQ number
> overflow (egress side, if the SA is not refreshed on time) 2. Anti replay
> window config and err handlings?
> 
That's a good point. 

I've been think about it for some time. For inline we don't need any notifications,
but as we approach full offload it might be unavoidable.

I hope that we could cover some cases using the existing rte_flow facilities like
the MARK action which could indicate when the anti-replay window has reached a
critical point for both cases you've mentioned above.

> 
> > +/**
> > + * Create security session as specified by the session configuration
> > + *
> > + * @param   id		security instance identifier id
> > + * @param   conf	session configuration parameters
> 
> fix the indentation here and other places in this file.
> 
> > + * @param   mp		mempool to allocate session objects from
> > + * @return
> > + *  - On success, pointer to session
> > + *  - On failure, NULL
> > + */
> > +struct rte_security_session *
> > +rte_security_session_create(uint16_t id,
> > +			    struct rte_security_session_conf *conf,
> > +			    struct rte_mempool *mp);
> > +
> > +/**
> 
>
  

-----Original Message-----
> Date: Thu, 14 Sep 2017 13:56:41 +0530
> From: Akhil Goyal <akhil.goyal@nxp.com>
> To: dev@dpdk.org
> CC: declan.doherty@intel.com, pablo.de.lara.guarch@intel.com,
>  hemant.agrawal@nxp.com, radu.nicolau@intel.com, borisp@mellanox.com,
>  aviadye@mellanox.com, thomas@monjalon.net, sandeep.malik@nxp.com,
>  jerin.jacob@caviumnetworks.com
> Subject: [PATCH 01/11] lib/rte_security: add security library
> X-Mailer: git-send-email 2.9.3
> 
> rte_security library provides APIs for security session
> create/free for protocol offload or offloaded crypto
> operation to ethernet device.

Overall the API semantic looks good. A few comments inlined.
I think, This patch should split as minimum two. One just the
specification header file and other one implementation.


> 
> Signed-off-by: Akhil Goyal <akhil.goyal@nxp.com>
> Signed-off-by: Boris Pismenny <borisp@mellanox.com>
> Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
> Signed-off-by: Declan Doherty <declan.doherty@intel.com>
> ---
> +
> +#include <rte_malloc.h>
> +#include <rte_dev.h>
> +
> +#include "rte_security.h"
> +#include "rte_security_driver.h"
> +
> +#define RTE_SECURITY_INSTANCES_BLOCK_ALLOC_SZ	(8)
> +
> +struct rte_security_ctx {
> +	uint16_t id;
> +	enum {
> +		RTE_SECURITY_INSTANCE_INVALID = 0,

explicit zero is not required.

> +		RTE_SECURITY_INSTANCE_VALID
> +	} state;
> +	void *device;
> +	struct rte_security_ops *ops;
> +};
> +
> +
> +int
> +rte_security_register(uint16_t *id, void *device,
> +		      struct rte_security_ops *ops)
> +{
> +	if (max_nb_security_instances == 0) {
> +		security_instances = rte_malloc(
> +				"rte_security_instances_ops",
> +				sizeof(*security_instances) *
> +				RTE_SECURITY_INSTANCES_BLOCK_ALLOC_SZ, 0);
> +
> +		if (security_instances == NULL)
> +			return -ENOMEM;
> +		max_nb_security_instances =
> +				RTE_SECURITY_INSTANCES_BLOCK_ALLOC_SZ;
> +	} else if (nb_security_instances >= max_nb_security_instances) {
> +		uint16_t *instances = rte_realloc(security_instances,
> +				sizeof(struct rte_security_ops *) *
> +				(max_nb_security_instances +
> +				RTE_SECURITY_INSTANCES_BLOCK_ALLOC_SZ), 0);
> +
> +		if (instances == NULL)
> +			return -ENOMEM;
> +
> +		max_nb_security_instances +=
> +				RTE_SECURITY_INSTANCES_BLOCK_ALLOC_SZ;
> +	}
> +
> +	*id = nb_security_instances++;
> +
> +	security_instances[*id].id = *id;
> +	security_instances[*id].state = RTE_SECURITY_INSTANCE_VALID;
> +	security_instances[*id].device = device;
> +	security_instances[*id].ops = ops;

This whole thing will break in multi process case where ops needs to
cloned for each process. Check the mempool library as reference.


> +
> +	return 0;
> +}
> +
> +int
> +rte_security_unregister(__rte_unused uint16_t *id)
> +{
> +	/* To be implemented */

This should implemented before it reaches to master.

> +	return 0;
> +}
> +
> +struct rte_security_session *
> +int
> +rte_security_set_pkt_metadata(uint16_t id,
> +			      struct rte_security_session *sess,
> +			      struct rte_mbuf *m, void *params)
> +{
> +	struct rte_security_ctx *instance;
> +
> +	RTE_SEC_VALID_ID_OR_ERR_RET(id, -ENODEV);
> +	instance = &security_instances[id];
> +
> +	RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->set_pkt_metadata, -ENOTSUP);

Do you need all this checking for a fastpath function?

> +	return instance->ops->set_pkt_metadata(instance->device,
> +					       sess, m, params);
> +}
> +
> +
> +/**
> + * @file rte_security.h
> + *
> + * RTE Security Common Definitions
> + *
> + */
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +#include <netinet/in.h>
> +#include <netinet/ip.h>
> +#include <netinet/ip6.h>
> +
> +#include <rte_mbuf.h>
> +#include <rte_memory.h>
> +#include <rte_mempool.h>
> +#include <rte_common.h>
> +#include <rte_crypto.h>

Nice to have it in alphabetical order.

> +
> +/** IPSec protocol mode */
> +enum rte_security_ipsec_sa_mode {
> +	RTE_SECURITY_IPSEC_SA_MODE_TRANSPORT,
> +	/**< IPSec Transport mode */
> +	RTE_SECURITY_IPSEC_SA_MODE_TUNNEL,
> +	/**< IPSec Tunnel mode */
> +};
> +
> +/** IPSec Protocol */
> +enum rte_security_ipsec_sa_protocol {
> +	RTE_SECURITY_IPSEC_SA_PROTO_AH,
> +	/**< AH protocol */
> +	RTE_SECURITY_IPSEC_SA_PROTO_ESP,
> +	/**< ESP protocol */
> +};
> +
> +/** IPSEC tunnel type */
> +enum rte_security_ipsec_tunnel_type {
> +	RTE_SECURITY_IPSEC_TUNNEL_IPV4 = 0,

Explicit zero may not be required.

> +	/**< Outer header is IPv4 */
> +	RTE_SECURITY_IPSEC_TUNNEL_IPV6,
> +	/**< Outer header is IPv6 */
> +};


> +struct rte_security_ipsec_tunnel_param {
> +	enum rte_security_ipsec_tunnel_type type;
> +	/**< Tunnel type: IPv4 or IPv6 */
> +

Anonymous union, You need RTE_STD_C11 here.
> +
> +	union {

> +
> +
> +/**
> + * IPsec Security Association option flags
> + */
> +struct rte_security_ipsec_sa_options {
> +	/** Extended Sequence Numbers (ESN)

All the elements in this structure is missing the doxygen commenting scheme.
i.e starting with /**<

> +	  *
> +	  * * 1: Use extended (64 bit) sequence numbers
> +	  * * 0: Use normal sequence numbers
> +	  */
> +	uint32_t esn : 1;
> +
> +	/** UDP encapsulation
> +	  *
> +	  * * 1: Do UDP encapsulation/decapsulation so that IPSEC packets can
> +	  *      traverse through NAT boxes.
> +	  * * 0: No UDP encapsulation
> +	  */
> +	uint32_t udp_encap : 1;
> +
> +
> +struct rte_security_session {
> +	__extension__ void *sess_private_data;

Do we need an __extension__ here?

> +	/**< Private session material */
> +};
> +
> +/**
> + * Create security session as specified by the session configuration
> + *
> + * @param   id		security instance identifier id

Bad alignment. Check the doxygen alignment everywhere.

> + * @param   conf	session configuration parameters
> + * @param   mp		mempool to allocate session objects from
> + * @return
> + *  - On success, pointer to session
> + *  - On failure, NULL
> + */
> +struct rte_security_session *
> +rte_security_session_create(uint16_t id,
> +			    struct rte_security_session_conf *conf,

const struct rte_security_session_conf *conf ?

> +			    struct rte_mempool *mp);

const struct rte_mempool *mp?

> +
> +/**
> + * Update security session as specified by the session configuration
> + *
> + * @param   id		security instance identifier id
> + * @param   sess	session to update parameters
> + * @param   conf	update configuration parameters
> + * @return
> + *  - On success returns 0
> + *  - On failure return errno
> + */
> +int
> +rte_security_session_update(uint16_t id,
> +			    struct rte_security_session *sess,
> +			    struct rte_security_session_conf *conf);

const ?

> +
> +/**
> + * Free security session header and the session private data and
> + * return it to its original mempool.
> + *
> + * @param   id		security instance identifier id
> + * @param   sess	security session to freed
> + *
> + * @return
> + *  - 0 if successful.
> + *  - -EINVAL if session is NULL.
> + *  - -EBUSY if not all device private data has been freed.
> + */
> +int
> +rte_security_session_destroy(uint16_t id, struct rte_security_session *sess);
> +
> +/**
> + *  Updates the buffer with device-specific defined metadata
> + *

Mention that it needs to be called when DEV_TX_OFFLOAD_SEC_NEED_MDATA is set or
whatever name we are coming up for DEV_TX_OFFLOAD_SEC_NEED_MDATA.

> + * @param	id	security instance identifier id
> + * @param	sess	security session
> + * @param	m	packet mbuf to set metadata on.
> + * @param	params	device-specific defined parameters required for metadata
> + *
> + * @return
> + *  - On success, zero.
> + *  - On failure, a negative value.
> + */
> +int
> +rte_security_set_pkt_metadata(uint16_t id,
> +			      struct rte_security_session *sess,
> +			      struct rte_mbuf *mb, void *params);
> +
> +/**
> + * Attach a session to a crypto operation.
> + * This API is needed only in case of RTE_SECURITY_SESS_CRYPTO_PROTO_OFFLOAD
> + * For other rte_security_session_action_type, ol_flags in rte_mbuf may be
> + * defined to perform security operations.
> + *
> + * @param	op	crypto operation
> + * @param	sess	security session
> + */
> +static inline int
> +rte_security_attach_session(struct rte_crypto_op *op,
> +			    struct rte_security_session *sess)
> +{
> +	if (unlikely(op->type != RTE_CRYPTO_OP_TYPE_SYMMETRIC))
> +		return -1;

-EINVAL?

> +
> +	op->sess_type =  RTE_CRYPTO_OP_SECURITY_SESSION;
> +
> +	return __rte_security_attach_session(op->sym, sess);
> +}
> +
> +struct rte_security_macsec_stats {
> +	uint64_t reserved;
> +};
> +
> +struct rte_security_ipsec_stats {
> +	uint64_t reserved;
> +
> +};
> +
> +struct rte_security_stats {
> +	enum rte_security_session_protocol protocol;
> +	/**< Security protocol to be configured */
> +
> +	union {
> +		struct rte_security_macsec_stats macsec;
> +		struct rte_security_ipsec_stats ipsec;
> +	};
> +};
> +
> +/**
> + * Query security session statistics
> + *
> + * @param	id	security instance identifier id
> + * @param	sess	security session
> + * @param	stats	statistics
> + * @return
> + *  - On success return 0
> + *  - On failure errno
> + */
> +int
> +rte_security_session_query(uint16_t id,
> +			   struct rte_security_session *sess,
> +			   struct rte_security_stats *stats);

IMO, Changing to something with "stats" makes more sense and it will be
inline with another subsystems as well.

> +
> +/**
> + * Security capability definition
> + */
> +struct rte_security_capability {
> +	enum rte_security_session_action_type action;
> +	/**< Security action type*/
> +	enum rte_security_session_protocol protocol;
> +	/**< Security protocol */
> +	RTE_STD_C11
> +	union {
> +		struct {
> +			enum rte_security_ipsec_sa_protocol proto;
> +			/**< IPsec SA protocol */
> +			enum rte_security_ipsec_sa_mode mode;
> +			/**< IPsec SA mode */
> +			enum rte_security_ipsec_sa_direction direction;
> +			/**< IPsec SA direction */
> +			struct rte_security_ipsec_sa_options options;
> +			/**< IPsec SA supported options */
> +		} ipsec;
> +		/**< IPsec capability */
> +		struct {
> +			/* To be Filled */
> +		} macsec;
> +		/**< MACsec capability */
> +	};
> +
> +	const struct rte_cryptodev_capabilities *crypto_capabilities;
> +	/**< Corresponding crypto capabilities for security capability  */
> +};
> +
> +/**
> + * Security capability index used to query a security instance for a specific
> + * security capability
> + */
> +struct rte_security_capability_idx {
> +	enum rte_security_session_action_type action;
> +	enum rte_security_session_protocol protocol;
> +
> +	union {
> +		struct {
> +			enum rte_security_ipsec_sa_protocol proto;
> +			enum rte_security_ipsec_sa_mode mode;
> +			enum rte_security_ipsec_sa_direction direction;
> +		} ipsec;

Why to duplicate elements in this structure. Can we have common structure
which can be used for rte_security_capability and
rte_security_capability_idx


> +	};
> +};
> +
> +/**
> + *  Returns array of security instance capabilities
> + *
> + * @param	id	Security instance identifier.
> + *
> + * @return
> + *   - Returns array of security capabilities.
> + *   - Return NULL if no capabilities available.
> + */
> +const struct rte_security_capability *
> +rte_security_capabilities_get(uint16_t id);
> +
> +/**
> + * Query if a specific capability is available on security instance
> + *
> + * @param	id	security instance identifier.
> + * @param	idx	security capability index to match against
> + *
> + * @return
> + *   - Returns pointer to security capability on match of capability
> + *     index criteria.
> + *   - Return NULL if the capability not matched on security instance.
> + */
> +const struct rte_security_capability *
> +rte_security_capability_get(uint16_t id,
> +			    struct rte_security_capability_idx *idx);

const struct rte_security_capability_idx *idx ?

> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif /* _RTE_SECURITY_H_ */
> +/**
> + * Query stats from the PMD.
> + *
> + * @param	device		Crypto/eth device pointer
> + * @param	sess		Pointer to Security private session structure
> + * @param	stats		Security stats of the driver
> + *
> + * @return
> + *  - Returns 0 if private session structure have been updated successfully.
> + *  - Returns -EINVAL if session parameters are invalid.
> + */
> +typedef int (*security_session_query_t)(void *device,
> +		struct rte_security_session *sess,
> +		struct rte_security_stats *stats);
> +
> +/**
> + * Update buffer with provided metadata.

Update the mbuf ?

> + *
> + * @param	sess		Security session structure
> + * @param	mb		Packet buffer
> + * @param	mt		Metadata
> + *
> + * @return
> + *  - Returns 0 if metadata updated successfully.
> + *  - Returns -ve value for errors.
> + */
> +typedef int (*security_set_pkt_metadata_t)(void *device,
> +		struct rte_security_session *sess, struct rte_mbuf *m,
> +		void *params);
> +
  

Hi Hemant,
On 9/15/2017 11:02 AM, Hemant Agrawal wrote:
> Hi,
> 
> On 9/14/2017 1:56 PM, Akhil Goyal wrote:
> <snip>..
> 
>> diff --git a/lib/librte_security/rte_security.c 
>> b/lib/librte_security/rte_security.c
>> new file mode 100644
>> index 0000000..5776246
>> --- /dev/null
>> +++ b/lib/librte_security/rte_security.c
>> @@ -0,0 +1,252 @@
>> +/*-
>> + *   BSD LICENSE
>> + *
>> + *   Copyright 2017 NXP.
>> + *   Copyright(c) 2017 Intel Corporation. All rights reserved.
>> + *
>> + *   Redistribution and use in source and binary forms, with or without
>> + *   modification, are permitted provided that the following conditions
>> + *   are met:
>> + *
>> + *     * Redistributions of source code must retain the above copyright
>> + *       notice, this list of conditions and the following disclaimer.
>> + *     * Redistributions in binary form must reproduce the above 
>> copyright
>> + *       notice, this list of conditions and the following disclaimer in
>> + *       the documentation and/or other materials provided with the
>> + *       distribution.
>> + *     * Neither the name of NXP nor the names of its
>> + *       contributors may be used to endorse or promote products derived
>> + *       from this software without specific prior written permission.
>> + *
>> + *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
>> + *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
>> + *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND 
>> FITNESS FOR
>> + *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE 
>> COPYRIGHT
>> + *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, 
>> INCIDENTAL,
>> + *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
>> + *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF 
>> USE,
>> + *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND 
>> ON ANY
>> + *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
>> + *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF 
>> THE USE
>> + *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH 
>> DAMAGE.
>> + */
>> +
>> +#include <rte_malloc.h>
>> +#include <rte_dev.h>
>> +
>> +#include "rte_security.h"
>> +#include "rte_security_driver.h"
>> +
>> +#define RTE_SECURITY_INSTANCES_BLOCK_ALLOC_SZ    (8)
>> +
>> +struct rte_security_ctx {
>> +    uint16_t id;
>> +    enum {
>> +        RTE_SECURITY_INSTANCE_INVALID = 0,
>> +        RTE_SECURITY_INSTANCE_VALID
>> +    } state;
>> +    void *device;
>> +    struct rte_security_ops *ops;
>> +};
>> +
>> +static struct rte_security_ctx *security_instances;
>> +static uint16_t max_nb_security_instances;
>> +static uint16_t nb_security_instances;
>> +
>> +static int
>> +rte_security_is_valid_id(uint16_t id)
>> +{
>> +    if (id >= nb_security_instances ||
>> +        (security_instances[id].state != RTE_SECURITY_INSTANCE_VALID))
>> +        return 0;
>> +    else
>> +        return 1;
>> +}
>> +
>> +/* Macros to check for valid id */
>> +#define RTE_SEC_VALID_ID_OR_ERR_RET(id, retval) do { \
>> +    if (!rte_security_is_valid_id(id)) { \
>> +        RTE_PMD_DEBUG_TRACE("Invalid sec_id=%d\n", id); \
>> +        return retval; \
>> +    } \
>> +} while (0)
>> +
>> +#define RTE_SEC_VALID_ID_OR_RET(id) do { \
>> +    if (!rte_security_is_valid_id(id)) { \
>> +        RTE_PMD_DEBUG_TRACE("Invalid sec_id=%d\n", id); \
>> +        return; \
>> +    } \
>> +} while (0)
>> +
>> +int
>> +rte_security_register(uint16_t *id, void *device,
>> +              struct rte_security_ops *ops)
>> +{
>> +    if (max_nb_security_instances == 0) {
>> +        security_instances = rte_malloc(
>> +                "rte_security_instances_ops",
>> +                sizeof(*security_instances) *
>> +                RTE_SECURITY_INSTANCES_BLOCK_ALLOC_SZ, 0);
>> +
>> +        if (security_instances == NULL)
>> +            return -ENOMEM;
>> +        max_nb_security_instances =
>> +                RTE_SECURITY_INSTANCES_BLOCK_ALLOC_SZ;
>> +    } else if (nb_security_instances >= max_nb_security_instances) {
>> +        uint16_t *instances = rte_realloc(security_instances,
>> +                sizeof(struct rte_security_ops *) *
>> +                (max_nb_security_instances +
>> +                RTE_SECURITY_INSTANCES_BLOCK_ALLOC_SZ), 0);
> 
> I think "RTE_SECURITY_INSTANCES_BLOCK_ALLOC_SZ" value as 8 is relatively 
> small. you may want to keep it "64" or more.
> 
> you may change it into two parts
> - Initial block size and incremental block size for realloc.
> 
> Also, do you want to make it a configurable variable. as some 
> implementation may need really large number of SAs.
Security Instances are not per SA, these are per eth/crypto device which 
support security offload.
> 
>> +
>> +        if (instances == NULL)
>> +            return -ENOMEM;
>> +
>> +        max_nb_security_instances +=
>> +                RTE_SECURITY_INSTANCES_BLOCK_ALLOC_SZ;
>> +    }
>> +
>> +    *id = nb_security_instances++;
>> +
>> +    security_instances[*id].id = *id;
>> +    security_instances[*id].state = RTE_SECURITY_INSTANCE_VALID;
>> +    security_instances[*id].device = device;
>> +    security_instances[*id].ops = ops;
>> +
>> +    return 0;
>> +}
>> +
>> +int
>> +rte_security_unregister(__rte_unused uint16_t *id)
>> +{
>> +    /* To be implemented */
>> +    return 0;
>> +}
>> +
>> +struct rte_security_session *
>> +rte_security_session_create(uint16_t id,
>> +                struct rte_security_session_conf *conf,
>> +                struct rte_mempool *mp)
>> +{
>> +    struct rte_security_ctx *instance;
>> +    struct rte_security_session *sess = NULL;
>> +
>> +    RTE_SEC_VALID_ID_OR_ERR_RET(id, NULL);
>> +    instance = &security_instances[id];
>> +
>> +    if (conf == NULL)
>> +        return NULL;
>> +
>> +    if (rte_mempool_get(mp, (void *)&sess))
>> +        return NULL;
>> +
>> +    RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->session_create, NULL);
> 
> it will leak the sess memory, if returned on error.
ok I will fix this.

> 
>> +    if (instance->ops->session_create(instance->device, conf, sess, 
>> mp)) {
>> +        rte_mempool_put(mp, (void *)sess);
>> +        return NULL;
>> +    }
> 
> can the mempool operations be part of session_create api?
No, this is used for struct rte_security_session. session_create() would 
take another object for its private data which it would free in the 
session_destroy() in the driver.
> 
> it will be similar to destroy, which is expected to free the 'sess' 
> object to mempool?
rte_security_session_destroy should free the mempool object used for 
struct rte_security_session in the rte_security_session_create

I will fix this in the next version.
> 
>> +    return sess;
>> +}
>> +
> 
> <snip>..
> 
>> +struct rte_security_ipsec_xform {
>> +    uint32_t spi;
>> +    /**< SA security parameter index */
>> +    uint32_t salt;
>> +    /**< SA salt */
>> +    struct rte_security_ipsec_sa_options options;
>> +    /**< various SA options */
>> +    enum rte_security_ipsec_sa_direction direction;
>> +    /**< IPSec SA Direction - Egress/Ingress */
>> +    enum rte_security_ipsec_sa_protocol proto;
>> +    /**< IPsec SA Protocol - AH/ESP */
>> +    enum rte_security_ipsec_sa_mode mode;
>> +    /**< IPsec SA Mode - transport/tunnel */
>> +    struct rte_security_ipsec_tunnel_param tunnel;
>> +    /**< Tunnel parameters, NULL for transport mode */
>> +};
>> +
>> +/**
>> + * MACsec security session configuration
>> + */
>> +struct rte_security_macsec_xform {
>> +    /** To be Filled */
>> +};
>> +
>> +/**
>> + * Security session action type.
>> + */
>> +enum rte_security_session_action_type {
>> +    RTE_SECURITY_ACTION_TYPE_NONE,
>> +    /**< No security actions */
> 
> This is not being used, it seems that you are only using it as marker to 
> indicate end of capability set?
Yes.
> 
>> +    RTE_SECURITY_ACTION_TYPE_INLINE_CRYPTO,
>> +    /**< Crypto processing for security protocol is processed inline
>> +     * during transmission */
>> +    RTE_SECURITY_ACTION_TYPE_INLINE_PROTOCOL,
>> +    /**< All security protocol processing is performed inline during
>> +     * transmission */
>> +    RTE_SECURITY_ACTION_TYPE_LOOKASIDE_PROTOCOL
>> +    /**< All security protocol processing including crypto is performed
>> +     * on a lookaside accelerator */
>> +};
>> +
>> +/** Security session protocol definition */
>> +enum rte_security_session_protocol {
>> +    RTE_SECURITY_PROTOCOL_IPSEC,
>> +    /**< IPsec Protocol */
>> +    RTE_SECURITY_PROTOCOL_MACSEC,
>> +    /**< MACSec Protocol */
>> +};
>> +
>> +/**
>> + * Security session configuration
>> + */
>> +struct rte_security_session_conf {
>> +    enum rte_security_session_action_type action_type;
>> +    /**< Type of action to be performed on the session */
>> +    enum rte_security_session_protocol protocol;
>> +    /**< Security protocol to be configured */
>> +    union {
>> +        struct rte_security_ipsec_xform ipsec;
>> +        struct rte_security_macsec_xform macsec;
>> +    };
>> +    /**< Configuration parameters for security session */
>> +    struct rte_crypto_sym_xform *crypto_xform;
>> +    /**< Security Session Crypto Transformations */
>> +};
>> +
>> +struct rte_security_session {
>> +    __extension__ void *sess_private_data;
>> +    /**< Private session material */
>> +};
>> +
> 
> 
> Do you need specific error handling for security sessions as well?
> In case of full protocol offloads, you will need indications for
> 1. SEQ number overflow (egress side, if the SA is not refreshed on time)
> 2. Anti replay window config and err handlings?
> 
This is in our TODO list for future.
> 
>> +/**
>> + * Create security session as specified by the session configuration
>> + *
>> + * @param   id        security instance identifier id
>> + * @param   conf    session configuration parameters
> 
> fix the indentation here and other places in this file.
ok.
> 
>> + * @param   mp        mempool to allocate session objects from
>> + * @return
>> + *  - On success, pointer to session
>> + *  - On failure, NULL
>> + */
>> +struct rte_security_session *
>> +rte_security_session_create(uint16_t id,
>> +                struct rte_security_session_conf *conf,
>> +                struct rte_mempool *mp);
>> +
>> +/**
> 


Regards,
Akhil
  

Hi,

I will address most of the issues in the v2, except the one related to 
the multiprocess issue - we may need more discussions on that.

Thanks for reviewing,

Radu


On 9/18/2017 2:13 PM, Jerin Jacob wrote:
> -----Original Message-----
>> Date: Thu, 14 Sep 2017 13:56:41 +0530
>> From: Akhil Goyal <akhil.goyal@nxp.com>
>> To: dev@dpdk.org
>> CC: declan.doherty@intel.com, pablo.de.lara.guarch@intel.com,
>>   hemant.agrawal@nxp.com, radu.nicolau@intel.com, borisp@mellanox.com,
>>   aviadye@mellanox.com, thomas@monjalon.net, sandeep.malik@nxp.com,
>>   jerin.jacob@caviumnetworks.com
>> Subject: [PATCH 01/11] lib/rte_security: add security library
>> X-Mailer: git-send-email 2.9.3
>>
>> rte_security library provides APIs for security session
>> create/free for protocol offload or offloaded crypto
>> operation to ethernet device.
> Overall the API semantic looks good. A few comments inlined.
> I think, This patch should split as minimum two. One just the
> specification header file and other one implementation.
>
>
>> Signed-off-by: Akhil Goyal <akhil.goyal@nxp.com>
>> Signed-off-by: Boris Pismenny <borisp@mellanox.com>
>> Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
>> Signed-off-by: Declan Doherty <declan.doherty@intel.com>
>> ---
>> +
>> +#include <rte_malloc.h>
>> +#include <rte_dev.h>
>> +
>> +#include "rte_security.h"
>> +#include "rte_security_driver.h"
>> +
>> +#define RTE_SECURITY_INSTANCES_BLOCK_ALLOC_SZ	(8)
>> +
>> +struct rte_security_ctx {
>> +	uint16_t id;
>> +	enum {
>> +		RTE_SECURITY_INSTANCE_INVALID = 0,
> explicit zero is not required.
>
>> +		RTE_SECURITY_INSTANCE_VALID
>> +	} state;
>> +	void *device;
>> +	struct rte_security_ops *ops;
>> +};
>> +
>> +
>> +int
>> +rte_security_register(uint16_t *id, void *device,
>> +		      struct rte_security_ops *ops)
>> +{
>> +	if (max_nb_security_instances == 0) {
>> +		security_instances = rte_malloc(
>> +				"rte_security_instances_ops",
>> +				sizeof(*security_instances) *
>> +				RTE_SECURITY_INSTANCES_BLOCK_ALLOC_SZ, 0);
>> +
>> +		if (security_instances == NULL)
>> +			return -ENOMEM;
>> +		max_nb_security_instances =
>> +				RTE_SECURITY_INSTANCES_BLOCK_ALLOC_SZ;
>> +	} else if (nb_security_instances >= max_nb_security_instances) {
>> +		uint16_t *instances = rte_realloc(security_instances,
>> +				sizeof(struct rte_security_ops *) *
>> +				(max_nb_security_instances +
>> +				RTE_SECURITY_INSTANCES_BLOCK_ALLOC_SZ), 0);
>> +
>> +		if (instances == NULL)
>> +			return -ENOMEM;
>> +
>> +		max_nb_security_instances +=
>> +				RTE_SECURITY_INSTANCES_BLOCK_ALLOC_SZ;
>> +	}
>> +
>> +	*id = nb_security_instances++;
>> +
>> +	security_instances[*id].id = *id;
>> +	security_instances[*id].state = RTE_SECURITY_INSTANCE_VALID;
>> +	security_instances[*id].device = device;
>> +	security_instances[*id].ops = ops;
> This whole thing will break in multi process case where ops needs to
> cloned for each process. Check the mempool library as reference.
>
>
>> +
>> +	return 0;
>> +}
>> +
>> +int
>> +rte_security_unregister(__rte_unused uint16_t *id)
>> +{
>> +	/* To be implemented */
> This should implemented before it reaches to master.
>
>> +	return 0;
>> +}
>> +
>> +struct rte_security_session *
>> +int
>> +rte_security_set_pkt_metadata(uint16_t id,
>> +			      struct rte_security_session *sess,
>> +			      struct rte_mbuf *m, void *params)
>> +{
>> +	struct rte_security_ctx *instance;
>> +
>> +	RTE_SEC_VALID_ID_OR_ERR_RET(id, -ENODEV);
>> +	instance = &security_instances[id];
>> +
>> +	RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->set_pkt_metadata, -ENOTSUP);
> Do you need all this checking for a fastpath function?
>
>> +	return instance->ops->set_pkt_metadata(instance->device,
>> +					       sess, m, params);
>> +}
>> +
>> +
>> +/**
>> + * @file rte_security.h
>> + *
>> + * RTE Security Common Definitions
>> + *
>> + */
>> +
>> +#ifdef __cplusplus
>> +extern "C" {
>> +#endif
>> +
>> +#include <netinet/in.h>
>> +#include <netinet/ip.h>
>> +#include <netinet/ip6.h>
>> +
>> +#include <rte_mbuf.h>
>> +#include <rte_memory.h>
>> +#include <rte_mempool.h>
>> +#include <rte_common.h>
>> +#include <rte_crypto.h>
> Nice to have it in alphabetical order.
>
>> +
>> +/** IPSec protocol mode */
>> +enum rte_security_ipsec_sa_mode {
>> +	RTE_SECURITY_IPSEC_SA_MODE_TRANSPORT,
>> +	/**< IPSec Transport mode */
>> +	RTE_SECURITY_IPSEC_SA_MODE_TUNNEL,
>> +	/**< IPSec Tunnel mode */
>> +};
>> +
>> +/** IPSec Protocol */
>> +enum rte_security_ipsec_sa_protocol {
>> +	RTE_SECURITY_IPSEC_SA_PROTO_AH,
>> +	/**< AH protocol */
>> +	RTE_SECURITY_IPSEC_SA_PROTO_ESP,
>> +	/**< ESP protocol */
>> +};
>> +
>> +/** IPSEC tunnel type */
>> +enum rte_security_ipsec_tunnel_type {
>> +	RTE_SECURITY_IPSEC_TUNNEL_IPV4 = 0,
> Explicit zero may not be required.
>
>> +	/**< Outer header is IPv4 */
>> +	RTE_SECURITY_IPSEC_TUNNEL_IPV6,
>> +	/**< Outer header is IPv6 */
>> +};
>
>> +struct rte_security_ipsec_tunnel_param {
>> +	enum rte_security_ipsec_tunnel_type type;
>> +	/**< Tunnel type: IPv4 or IPv6 */
>> +
> Anonymous union, You need RTE_STD_C11 here.
>> +
>> +	union {
>> +
>> +
>> +/**
>> + * IPsec Security Association option flags
>> + */
>> +struct rte_security_ipsec_sa_options {
>> +	/** Extended Sequence Numbers (ESN)
> All the elements in this structure is missing the doxygen commenting scheme.
> i.e starting with /**<
>
>> +	  *
>> +	  * * 1: Use extended (64 bit) sequence numbers
>> +	  * * 0: Use normal sequence numbers
>> +	  */
>> +	uint32_t esn : 1;
>> +
>> +	/** UDP encapsulation
>> +	  *
>> +	  * * 1: Do UDP encapsulation/decapsulation so that IPSEC packets can
>> +	  *      traverse through NAT boxes.
>> +	  * * 0: No UDP encapsulation
>> +	  */
>> +	uint32_t udp_encap : 1;
>> +
>> +
>> +struct rte_security_session {
>> +	__extension__ void *sess_private_data;
> Do we need an __extension__ here?
>
>> +	/**< Private session material */
>> +};
>> +
>> +/**
>> + * Create security session as specified by the session configuration
>> + *
>> + * @param   id		security instance identifier id
> Bad alignment. Check the doxygen alignment everywhere.
>
>> + * @param   conf	session configuration parameters
>> + * @param   mp		mempool to allocate session objects from
>> + * @return
>> + *  - On success, pointer to session
>> + *  - On failure, NULL
>> + */
>> +struct rte_security_session *
>> +rte_security_session_create(uint16_t id,
>> +			    struct rte_security_session_conf *conf,
> const struct rte_security_session_conf *conf ?
>
>> +			    struct rte_mempool *mp);
> const struct rte_mempool *mp?
>
>> +
>> +/**
>> + * Update security session as specified by the session configuration
>> + *
>> + * @param   id		security instance identifier id
>> + * @param   sess	session to update parameters
>> + * @param   conf	update configuration parameters
>> + * @return
>> + *  - On success returns 0
>> + *  - On failure return errno
>> + */
>> +int
>> +rte_security_session_update(uint16_t id,
>> +			    struct rte_security_session *sess,
>> +			    struct rte_security_session_conf *conf);
> const ?
>
>> +
>> +/**
>> + * Free security session header and the session private data and
>> + * return it to its original mempool.
>> + *
>> + * @param   id		security instance identifier id
>> + * @param   sess	security session to freed
>> + *
>> + * @return
>> + *  - 0 if successful.
>> + *  - -EINVAL if session is NULL.
>> + *  - -EBUSY if not all device private data has been freed.
>> + */
>> +int
>> +rte_security_session_destroy(uint16_t id, struct rte_security_session *sess);
>> +
>> +/**
>> + *  Updates the buffer with device-specific defined metadata
>> + *
> Mention that it needs to be called when DEV_TX_OFFLOAD_SEC_NEED_MDATA is set or
> whatever name we are coming up for DEV_TX_OFFLOAD_SEC_NEED_MDATA.
>
>> + * @param	id	security instance identifier id
>> + * @param	sess	security session
>> + * @param	m	packet mbuf to set metadata on.
>> + * @param	params	device-specific defined parameters required for metadata
>> + *
>> + * @return
>> + *  - On success, zero.
>> + *  - On failure, a negative value.
>> + */
>> +int
>> +rte_security_set_pkt_metadata(uint16_t id,
>> +			      struct rte_security_session *sess,
>> +			      struct rte_mbuf *mb, void *params);
>> +
>> +/**
>> + * Attach a session to a crypto operation.
>> + * This API is needed only in case of RTE_SECURITY_SESS_CRYPTO_PROTO_OFFLOAD
>> + * For other rte_security_session_action_type, ol_flags in rte_mbuf may be
>> + * defined to perform security operations.
>> + *
>> + * @param	op	crypto operation
>> + * @param	sess	security session
>> + */
>> +static inline int
>> +rte_security_attach_session(struct rte_crypto_op *op,
>> +			    struct rte_security_session *sess)
>> +{
>> +	if (unlikely(op->type != RTE_CRYPTO_OP_TYPE_SYMMETRIC))
>> +		return -1;
> -EINVAL?
>
>> +
>> +	op->sess_type =  RTE_CRYPTO_OP_SECURITY_SESSION;
>> +
>> +	return __rte_security_attach_session(op->sym, sess);
>> +}
>> +
>> +struct rte_security_macsec_stats {
>> +	uint64_t reserved;
>> +};
>> +
>> +struct rte_security_ipsec_stats {
>> +	uint64_t reserved;
>> +
>> +};
>> +
>> +struct rte_security_stats {
>> +	enum rte_security_session_protocol protocol;
>> +	/**< Security protocol to be configured */
>> +
>> +	union {
>> +		struct rte_security_macsec_stats macsec;
>> +		struct rte_security_ipsec_stats ipsec;
>> +	};
>> +};
>> +
>> +/**
>> + * Query security session statistics
>> + *
>> + * @param	id	security instance identifier id
>> + * @param	sess	security session
>> + * @param	stats	statistics
>> + * @return
>> + *  - On success return 0
>> + *  - On failure errno
>> + */
>> +int
>> +rte_security_session_query(uint16_t id,
>> +			   struct rte_security_session *sess,
>> +			   struct rte_security_stats *stats);
> IMO, Changing to something with "stats" makes more sense and it will be
> inline with another subsystems as well.
>
>> +
>> +/**
>> + * Security capability definition
>> + */
>> +struct rte_security_capability {
>> +	enum rte_security_session_action_type action;
>> +	/**< Security action type*/
>> +	enum rte_security_session_protocol protocol;
>> +	/**< Security protocol */
>> +	RTE_STD_C11
>> +	union {
>> +		struct {
>> +			enum rte_security_ipsec_sa_protocol proto;
>> +			/**< IPsec SA protocol */
>> +			enum rte_security_ipsec_sa_mode mode;
>> +			/**< IPsec SA mode */
>> +			enum rte_security_ipsec_sa_direction direction;
>> +			/**< IPsec SA direction */
>> +			struct rte_security_ipsec_sa_options options;
>> +			/**< IPsec SA supported options */
>> +		} ipsec;
>> +		/**< IPsec capability */
>> +		struct {
>> +			/* To be Filled */
>> +		} macsec;
>> +		/**< MACsec capability */
>> +	};
>> +
>> +	const struct rte_cryptodev_capabilities *crypto_capabilities;
>> +	/**< Corresponding crypto capabilities for security capability  */
>> +};
>> +
>> +/**
>> + * Security capability index used to query a security instance for a specific
>> + * security capability
>> + */
>> +struct rte_security_capability_idx {
>> +	enum rte_security_session_action_type action;
>> +	enum rte_security_session_protocol protocol;
>> +
>> +	union {
>> +		struct {
>> +			enum rte_security_ipsec_sa_protocol proto;
>> +			enum rte_security_ipsec_sa_mode mode;
>> +			enum rte_security_ipsec_sa_direction direction;
>> +		} ipsec;
> Why to duplicate elements in this structure. Can we have common structure
> which can be used for rte_security_capability and
> rte_security_capability_idx
>
>
>> +	};
>> +};
>> +
>> +/**
>> + *  Returns array of security instance capabilities
>> + *
>> + * @param	id	Security instance identifier.
>> + *
>> + * @return
>> + *   - Returns array of security capabilities.
>> + *   - Return NULL if no capabilities available.
>> + */
>> +const struct rte_security_capability *
>> +rte_security_capabilities_get(uint16_t id);
>> +
>> +/**
>> + * Query if a specific capability is available on security instance
>> + *
>> + * @param	id	security instance identifier.
>> + * @param	idx	security capability index to match against
>> + *
>> + * @return
>> + *   - Returns pointer to security capability on match of capability
>> + *     index criteria.
>> + *   - Return NULL if the capability not matched on security instance.
>> + */
>> +const struct rte_security_capability *
>> +rte_security_capability_get(uint16_t id,
>> +			    struct rte_security_capability_idx *idx);
> const struct rte_security_capability_idx *idx ?
>
>> +
>> +#ifdef __cplusplus
>> +}
>> +#endif
>> +
>> +#endif /* _RTE_SECURITY_H_ */
>> +/**
>> + * Query stats from the PMD.
>> + *
>> + * @param	device		Crypto/eth device pointer
>> + * @param	sess		Pointer to Security private session structure
>> + * @param	stats		Security stats of the driver
>> + *
>> + * @return
>> + *  - Returns 0 if private session structure have been updated successfully.
>> + *  - Returns -EINVAL if session parameters are invalid.
>> + */
>> +typedef int (*security_session_query_t)(void *device,
>> +		struct rte_security_session *sess,
>> +		struct rte_security_stats *stats);
>> +
>> +/**
>> + * Update buffer with provided metadata.
> Update the mbuf ?
>
>> + *
>> + * @param	sess		Security session structure
>> + * @param	mb		Packet buffer
>> + * @param	mt		Metadata
>> + *
>> + * @return
>> + *  - Returns 0 if metadata updated successfully.
>> + *  - Returns -ve value for errors.
>> + */
>> +typedef int (*security_set_pkt_metadata_t)(void *device,
>> +		struct rte_security_session *sess, struct rte_mbuf *m,
>> +		void *params);
>> +