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

Message ID 20171003131413.23846-2-akhil.goyal@nxp.com (mailing list archive)
State Superseded, archived
Headers

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/Intel-compilation success Compilation OK

Commit Message

Akhil Goyal Oct. 3, 2017, 1:14 p.m. UTC
  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           | 275 +++++++++++++++
 lib/librte_security/rte_security.h           | 495 +++++++++++++++++++++++++++
 lib/librte_security/rte_security_driver.h    | 182 ++++++++++
 lib/librte_security/rte_security_version.map |  13 +
 5 files changed, 1018 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
  

Comments

De Lara Guarch, Pablo Oct. 5, 2017, 3:32 p.m. UTC | #1
> -----Original Message-----
> From: Akhil Goyal [mailto:akhil.goyal@nxp.com]
> Sent: Tuesday, October 3, 2017 2:14 PM
> To: dev@dpdk.org
> Cc: Doherty, Declan <declan.doherty@intel.com>; De Lara Guarch, Pablo
> <pablo.de.lara.guarch@intel.com>; hemant.agrawal@nxp.com; Nicolau,
> Radu <radu.nicolau@intel.com>; borisp@mellanox.com;
> aviadye@mellanox.com; thomas@monjalon.net; sandeep.malik@nxp.com;
> jerin.jacob@caviumnetworks.com; Mcnamara, John
> <john.mcnamara@intel.com>; olivier.matz@6wind.com
> Subject: [PATCH v2 01/12] 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>

...

> +}
> diff --git a/lib/librte_security/rte_security.h
> b/lib/librte_security/rte_security.h

...

When building the docs with "make-guides-html",
It is complaining about an item in this list:

lib/librte_security/rte_security.h:139: warning: Invalid list item found


> +/**
> + * 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;

...

> +/**
> + *  Updates the buffer with device-specific defined metadata
> + *
> + * @param	id	security instance identifier id
> + * @param	sess	security session
> + * @param	m	packet mbuf to set metadata on.

Parameter is called "mb".

> + * @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);
  
Ananyev, Konstantin Oct. 5, 2017, 4:30 p.m. UTC | #2
Hi lads,

> 
> 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           | 275 +++++++++++++++
>  lib/librte_security/rte_security.h           | 495 +++++++++++++++++++++++++++
>  lib/librte_security/rte_security_driver.h    | 182 ++++++++++
>  lib/librte_security/rte_security_version.map |  13 +
>  5 files changed, 1018 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
> 
> diff --git a/lib/librte_security/Makefile b/lib/librte_security/Makefile
> new file mode 100644
> index 0000000..af87bb2
> --- /dev/null
> +++ b/lib/librte_security/Makefile
> @@ -0,0 +1,53 @@
> +#   BSD LICENSE
> +#
> +#   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 Intel Corporation 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_SDK)/mk/rte.vars.mk
> +
> +# library name
> +LIB = librte_security.a
> +
> +# library version
> +LIBABIVER := 1
> +
> +# build flags
> +CFLAGS += -O3
> +CFLAGS += $(WERROR_FLAGS)
> +
> +# library source files
> +SRCS-y += rte_security.c
> +
> +# export include files
> +SYMLINK-y-include += rte_security.h
> +SYMLINK-y-include += rte_security_driver.h
> +
> +# versioning export map
> +EXPORT_MAP := rte_security_version.map
> +
> +include $(RTE_SDK)/mk/rte.lib.mk
> diff --git a/lib/librte_security/rte_security.c b/lib/librte_security/rte_security.c
> new file mode 100644
> index 0000000..97d3857
> --- /dev/null
> +++ b/lib/librte_security/rte_security.c
> @@ -0,0 +1,275 @@
> +/*-
> + *   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,
> +		RTE_SECURITY_INSTANCE_VALID
> +	} state;
> +	void *device;
> +	struct rte_security_ops *ops;
> +	uint16_t sess_cnt;
> +};
> +
> +static struct rte_security_ctx *security_instances;
> +static uint16_t max_nb_security_instances;
> +static uint16_t nb_security_instances;

Probably a dumb question - but why do you need a global security_instances []
and why security_instance has to be refrenced by index?
As I understand, with proposed model all drivers have to do something like:
rte_security_register(&eth_dev->data->sec_id,  (void *)eth_dev, &ixgbe_security_ops);  
and then all apps would have to:
rte_eth_dev_get_sec_id(portid)
to retrieve that security_instance index.
Why not just treat struct rte_security_ctx* as opaque pointer and make
all related API get/accept it as a paratemer. 
To retrieve sec_ctx from device just:
struct rte_security_ctx* rte_ethdev_get_sec_ctx(portid);
struct rte_security_ctx* rte_cryptodev_get_sec_ctx(portid);
?

Another question how currently proposed model with global static array and friends,
supposed to work for DPDK MP model? 
Or MP support is not planned?

> +
> +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) {

You probably need try to reuse unregistered entries first? 
Konstantin


> +		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;
> +	security_instances[*id].sess_cnt = 0;
> +
> +	return 0;
> +}
> +
> +int
> +rte_security_unregister(uint16_t id)
> +{
> +	struct rte_security_ctx *instance;
> +
> +	RTE_SEC_VALID_ID_OR_ERR_RET(id, -ENODEV);
> +	instance = &security_instances[id];
> +
> +	if (instance->sess_cnt)
> +		return -EBUSY;
> +
> +	memset(instance, 0, sizeof(*instance));
> +	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;
> +
> +	RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->session_create, NULL);
> +
> +	if (rte_mempool_get(mp, (void *)&sess))
> +		return NULL;
> +
> +	if (instance->ops->session_create(instance->device, conf, sess, mp)) {
> +		rte_mempool_put(mp, (void *)sess);
> +		return NULL;
> +	}
> +	instance->sess_cnt++;
> +
> +	return sess;
> +}
> +
> +int
> +rte_security_session_update(uint16_t id,
> +			    struct rte_security_session *sess,
> +			    struct rte_security_session_conf *conf)
> +{
> +	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->session_update, -ENOTSUP);
> +	return instance->ops->session_update(instance->device, sess, conf);
> +}
> +
> +int
> +rte_security_session_stats_get(uint16_t id,
> +			       struct rte_security_session *sess,
> +			       struct rte_security_stats *stats)
> +{
> +	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->session_stats_get, -ENOTSUP);
> +	return instance->ops->session_stats_get(instance->device, sess, stats);
> +}
> +
> +int
> +rte_security_session_destroy(uint16_t id, struct rte_security_session *sess)
> +{
> +	int ret;
> +	struct rte_security_ctx *instance;
> +	struct rte_mempool *mp = rte_mempool_from_obj(sess);
> +
> +	RTE_SEC_VALID_ID_OR_ERR_RET(id, -ENODEV);
> +	instance = &security_instances[id];
> +
> +	RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->session_destroy, -ENOTSUP);
> +
> +	if (instance->sess_cnt)
> +		instance->sess_cnt--;
> +
> +	ret = instance->ops->session_destroy(instance->device, sess);
> +	if (!ret)
> +		rte_mempool_put(mp, (void *)sess);
> +
> +	return ret;
> +}
> +
> +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);
> +	return instance->ops->set_pkt_metadata(instance->device,
> +					       sess, m, params);
> +}
> +
> +const struct rte_security_capability *
> +rte_security_capabilities_get(uint16_t id)
> +{
> +	struct rte_security_ctx *instance;
> +
> +	RTE_SEC_VALID_ID_OR_ERR_RET(id, NULL);
> +	instance = &security_instances[id];
> +
> +	RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->capabilities_get, NULL);
> +	return instance->ops->capabilities_get(instance->device);
> +}
> +
> +const struct rte_security_capability *
> +rte_security_capability_get(uint16_t id,
> +			    struct rte_security_capability_idx *idx)
> +{
> +	struct rte_security_ctx *instance;
> +	const struct rte_security_capability *capabilities;
> +	const struct rte_security_capability *capability;
> +	uint16_t i = 0;
> +
> +	RTE_SEC_VALID_ID_OR_ERR_RET(id, NULL);
> +	instance = &security_instances[id];
> +
> +	RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->capabilities_get, NULL);
> +	capabilities = instance->ops->capabilities_get(instance->device);
> +
> +	if (capabilities == NULL)
> +		return NULL;
> +
> +	while ((capability = &capabilities[i++])->action
> +			!= RTE_SECURITY_ACTION_TYPE_NONE) {
> +		if (capability->action  == idx->action &&
> +				capability->protocol == idx->protocol) {
> +			if (idx->protocol == RTE_SECURITY_PROTOCOL_IPSEC) {
> +				if (capability->ipsec.proto ==
> +						idx->ipsec.proto &&
> +					capability->ipsec.mode ==
> +							idx->ipsec.mode &&
> +					capability->ipsec.direction ==
> +							idx->ipsec.direction)
> +					return capability;
> +			}
> +		}
> +	}
> +
> +	return NULL;
> +}
  
Akhil Goyal Oct. 6, 2017, 6:11 p.m. UTC | #3
Hi Konstantin,

Thanks for your comments.
On 10/5/2017 10:00 PM, Ananyev, Konstantin wrote:
> Hi lads,
> 
>>
>> 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           | 275 +++++++++++++++
>>   lib/librte_security/rte_security.h           | 495 +++++++++++++++++++++++++++
>>   lib/librte_security/rte_security_driver.h    | 182 ++++++++++
>>   lib/librte_security/rte_security_version.map |  13 +
>>   5 files changed, 1018 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
>>
>> diff --git a/lib/librte_security/Makefile b/lib/librte_security/Makefile
>> new file mode 100644
>> index 0000000..af87bb2
>> --- /dev/null
>> +++ b/lib/librte_security/Makefile
>> @@ -0,0 +1,53 @@
>> +#   BSD LICENSE
>> +#
>> +#   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 Intel Corporation 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_SDK)/mk/rte.vars.mk
>> +
>> +# library name
>> +LIB = librte_security.a
>> +
>> +# library version
>> +LIBABIVER := 1
>> +
>> +# build flags
>> +CFLAGS += -O3
>> +CFLAGS += $(WERROR_FLAGS)
>> +
>> +# library source files
>> +SRCS-y += rte_security.c
>> +
>> +# export include files
>> +SYMLINK-y-include += rte_security.h
>> +SYMLINK-y-include += rte_security_driver.h
>> +
>> +# versioning export map
>> +EXPORT_MAP := rte_security_version.map
>> +
>> +include $(RTE_SDK)/mk/rte.lib.mk
>> diff --git a/lib/librte_security/rte_security.c b/lib/librte_security/rte_security.c
>> new file mode 100644
>> index 0000000..97d3857
>> --- /dev/null
>> +++ b/lib/librte_security/rte_security.c
>> @@ -0,0 +1,275 @@
>> +/*-
>> + *   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,
>> +		RTE_SECURITY_INSTANCE_VALID
>> +	} state;
>> +	void *device;
>> +	struct rte_security_ops *ops;
>> +	uint16_t sess_cnt;
>> +};
>> +
>> +static struct rte_security_ctx *security_instances;
>> +static uint16_t max_nb_security_instances;
>> +static uint16_t nb_security_instances;
> 
> Probably a dumb question - but why do you need a global security_instances []
> and why security_instance has to be refrenced by index?
> As I understand, with proposed model all drivers have to do something like:
> rte_security_register(&eth_dev->data->sec_id,  (void *)eth_dev, &ixgbe_security_ops);
> and then all apps would have to:
> rte_eth_dev_get_sec_id(portid)
> to retrieve that security_instance index.
> Why not just treat struct rte_security_ctx* as opaque pointer and make
> all related API get/accept it as a paratemer.
> To retrieve sec_ctx from device just:
> struct rte_security_ctx* rte_ethdev_get_sec_ctx(portid);
> struct rte_security_ctx* rte_cryptodev_get_sec_ctx(portid);
> ?
We would look into this separately.
> 
> Another question how currently proposed model with global static array and friends,
> supposed to work for DPDK MP model?
> Or MP support is not planned?
multi process case is planned for future enhancement. This is mentioned 
in the cover note.
> 
>> +
>> +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) {
> 
> You probably need try to reuse unregistered entries first?
> Konstantin
> 
These APIs are experimental at this moment as mentioned in the patchset. 
We will try accommodate your comments in future.
> 
>> +		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;
>> +	security_instances[*id].sess_cnt = 0;
>> +
>> +	return 0;
>> +}
>> +
>> +int
>> +rte_security_unregister(uint16_t id)
>> +{
>> +	struct rte_security_ctx *instance;
>> +
>> +	RTE_SEC_VALID_ID_OR_ERR_RET(id, -ENODEV);
>> +	instance = &security_instances[id];
>> +
>> +	if (instance->sess_cnt)
>> +		return -EBUSY;
>> +
>> +	memset(instance, 0, sizeof(*instance));
>> +	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;
>> +
>> +	RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->session_create, NULL);
>> +
>> +	if (rte_mempool_get(mp, (void *)&sess))
>> +		return NULL;
>> +
>> +	if (instance->ops->session_create(instance->device, conf, sess, mp)) {
>> +		rte_mempool_put(mp, (void *)sess);
>> +		return NULL;
>> +	}
>> +	instance->sess_cnt++;
>> +
>> +	return sess;
>> +}
>> +
>> +int
>> +rte_security_session_update(uint16_t id,
>> +			    struct rte_security_session *sess,
>> +			    struct rte_security_session_conf *conf)
>> +{
>> +	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->session_update, -ENOTSUP);
>> +	return instance->ops->session_update(instance->device, sess, conf);
>> +}
>> +
>> +int
>> +rte_security_session_stats_get(uint16_t id,
>> +			       struct rte_security_session *sess,
>> +			       struct rte_security_stats *stats)
>> +{
>> +	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->session_stats_get, -ENOTSUP);
>> +	return instance->ops->session_stats_get(instance->device, sess, stats);
>> +}
>> +
>> +int
>> +rte_security_session_destroy(uint16_t id, struct rte_security_session *sess)
>> +{
>> +	int ret;
>> +	struct rte_security_ctx *instance;
>> +	struct rte_mempool *mp = rte_mempool_from_obj(sess);
>> +
>> +	RTE_SEC_VALID_ID_OR_ERR_RET(id, -ENODEV);
>> +	instance = &security_instances[id];
>> +
>> +	RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->session_destroy, -ENOTSUP);
>> +
>> +	if (instance->sess_cnt)
>> +		instance->sess_cnt--;
>> +
>> +	ret = instance->ops->session_destroy(instance->device, sess);
>> +	if (!ret)
>> +		rte_mempool_put(mp, (void *)sess);
>> +
>> +	return ret;
>> +}
>> +
>> +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);
>> +	return instance->ops->set_pkt_metadata(instance->device,
>> +					       sess, m, params);
>> +}
>> +
>> +const struct rte_security_capability *
>> +rte_security_capabilities_get(uint16_t id)
>> +{
>> +	struct rte_security_ctx *instance;
>> +
>> +	RTE_SEC_VALID_ID_OR_ERR_RET(id, NULL);
>> +	instance = &security_instances[id];
>> +
>> +	RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->capabilities_get, NULL);
>> +	return instance->ops->capabilities_get(instance->device);
>> +}
>> +
>> +const struct rte_security_capability *
>> +rte_security_capability_get(uint16_t id,
>> +			    struct rte_security_capability_idx *idx)
>> +{
>> +	struct rte_security_ctx *instance;
>> +	const struct rte_security_capability *capabilities;
>> +	const struct rte_security_capability *capability;
>> +	uint16_t i = 0;
>> +
>> +	RTE_SEC_VALID_ID_OR_ERR_RET(id, NULL);
>> +	instance = &security_instances[id];
>> +
>> +	RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->capabilities_get, NULL);
>> +	capabilities = instance->ops->capabilities_get(instance->device);
>> +
>> +	if (capabilities == NULL)
>> +		return NULL;
>> +
>> +	while ((capability = &capabilities[i++])->action
>> +			!= RTE_SECURITY_ACTION_TYPE_NONE) {
>> +		if (capability->action  == idx->action &&
>> +				capability->protocol == idx->protocol) {
>> +			if (idx->protocol == RTE_SECURITY_PROTOCOL_IPSEC) {
>> +				if (capability->ipsec.proto ==
>> +						idx->ipsec.proto &&
>> +					capability->ipsec.mode ==
>> +							idx->ipsec.mode &&
>> +					capability->ipsec.direction ==
>> +							idx->ipsec.direction)
>> +					return capability;
>> +			}
>> +		}
>> +	}
>> +
>> +	return NULL;
>> +}
> 

Regards,
Akhil
  
Ananyev, Konstantin Oct. 9, 2017, 1:42 p.m. UTC | #4
Hi Akhil,

> -----Original Message-----

> From: Akhil Goyal [mailto:akhil.goyal@nxp.com]

> Sent: Friday, October 6, 2017 7:11 PM

> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org

> Cc: Doherty, Declan <declan.doherty@intel.com>; De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; hemant.agrawal@nxp.com;

> Nicolau, Radu <radu.nicolau@intel.com>; borisp@mellanox.com; aviadye@mellanox.com; thomas@monjalon.net;

> sandeep.malik@nxp.com; jerin.jacob@caviumnetworks.com; Mcnamara, John <john.mcnamara@intel.com>; olivier.matz@6wind.com

> Subject: Re: [PATCH v2 01/12] lib/rte_security: add security library

> 

> Hi Konstantin,

> 

> Thanks for your comments.

> On 10/5/2017 10:00 PM, Ananyev, Konstantin wrote:

> > Hi lads,

> >

> >>

> >> 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           | 275 +++++++++++++++

> >>   lib/librte_security/rte_security.h           | 495 +++++++++++++++++++++++++++

> >>   lib/librte_security/rte_security_driver.h    | 182 ++++++++++

> >>   lib/librte_security/rte_security_version.map |  13 +

> >>   5 files changed, 1018 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

> >>

> >> diff --git a/lib/librte_security/Makefile b/lib/librte_security/Makefile

> >> new file mode 100644

> >> index 0000000..af87bb2

> >> --- /dev/null

> >> +++ b/lib/librte_security/Makefile

> >> @@ -0,0 +1,53 @@

> >> +#   BSD LICENSE

> >> +#

> >> +#   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 Intel Corporation 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_SDK)/mk/rte.vars.mk

> >> +

> >> +# library name

> >> +LIB = librte_security.a

> >> +

> >> +# library version

> >> +LIBABIVER := 1

> >> +

> >> +# build flags

> >> +CFLAGS += -O3

> >> +CFLAGS += $(WERROR_FLAGS)

> >> +

> >> +# library source files

> >> +SRCS-y += rte_security.c

> >> +

> >> +# export include files

> >> +SYMLINK-y-include += rte_security.h

> >> +SYMLINK-y-include += rte_security_driver.h

> >> +

> >> +# versioning export map

> >> +EXPORT_MAP := rte_security_version.map

> >> +

> >> +include $(RTE_SDK)/mk/rte.lib.mk

> >> diff --git a/lib/librte_security/rte_security.c b/lib/librte_security/rte_security.c

> >> new file mode 100644

> >> index 0000000..97d3857

> >> --- /dev/null

> >> +++ b/lib/librte_security/rte_security.c

> >> @@ -0,0 +1,275 @@

> >> +/*-

> >> + *   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,

> >> +		RTE_SECURITY_INSTANCE_VALID

> >> +	} state;

> >> +	void *device;

> >> +	struct rte_security_ops *ops;

> >> +	uint16_t sess_cnt;

> >> +};

> >> +

> >> +static struct rte_security_ctx *security_instances;

> >> +static uint16_t max_nb_security_instances;

> >> +static uint16_t nb_security_instances;

> >

> > Probably a dumb question - but why do you need a global security_instances []

> > and why security_instance has to be refrenced by index?

> > As I understand, with proposed model all drivers have to do something like:

> > rte_security_register(&eth_dev->data->sec_id,  (void *)eth_dev, &ixgbe_security_ops);

> > and then all apps would have to:

> > rte_eth_dev_get_sec_id(portid)

> > to retrieve that security_instance index.

> > Why not just treat struct rte_security_ctx* as opaque pointer and make

> > all related API get/accept it as a paratemer.

> > To retrieve sec_ctx from device just:

> > struct rte_security_ctx* rte_ethdev_get_sec_ctx(portid);

> > struct rte_security_ctx* rte_cryptodev_get_sec_ctx(portid);

> > ?

> We would look into this separately.


Could you clarify what does it mean?
It will be addressed in v4 or don't plan to address it at all or something else?
  

> >

> > Another question how currently proposed model with global static array and friends,

> > supposed to work for DPDK MP model?

> > Or MP support is not planned?

> multi process case is planned for future enhancement. This is mentioned

> in the cover note.


Great, then I suppose you should have a generic idea how that model will work
for DPDK multi-process model?
If so, can you probably share your thoughts, because it is not clear to me.
Let say user has an ethdev device with ipsec capability and  wants to use it
from both primary and secondary process.
What would be a procedure?
Can user use the same security instance from both processes?
If yes, then  
- how secondary process will get security_instance_id for that device
from primary process?
 By calling rte_eth_dev_get_sec_id(), or something else? 
- how guarantee that all these func pointers inside ops will be mapped to the
same address inside  processes?  
If not, then does it mean each process has to call rte_security_register()
for each device?
But right now you have only one sec_id  inside rte_eth_dev_data.

Would secondary process be allowed to register/unregister/update security instances?
 if yes, how the will synchronize?
Same question for session ops.

> >

> >> +

> >> +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)

> >> +{


Probably const  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) {

> >

> > You probably need try to reuse unregistered entries first?

> > Konstantin

> >

> These APIs are experimental at this moment as mentioned in the patchset.

> We will try accommodate your comments in future.


Again could you clarify what do you mean by 'future' here?

> >

> >> +		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;


BTW, I think you need to copy actual contents of ops.
Otherwise you assuming that ops are static
(would be valid though all processes lifetimes). 

Konstantin

> >> +	security_instances[*id].sess_cnt = 0;

> >> +

> >> +	return 0;

> >> +}

> >> +

> >> +int

> >> +rte_security_unregister(uint16_t id)

> >> +{

> >> +	struct rte_security_ctx *instance;

> >> +

> >> +	RTE_SEC_VALID_ID_OR_ERR_RET(id, -ENODEV);

> >> +	instance = &security_instances[id];

> >> +

> >> +	if (instance->sess_cnt)

> >> +		return -EBUSY;

> >> +

> >> +	memset(instance, 0, sizeof(*instance));

> >> +	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;

> >> +

> >> +	RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->session_create, NULL);

> >> +

> >> +	if (rte_mempool_get(mp, (void *)&sess))

> >> +		return NULL;

> >> +

> >> +	if (instance->ops->session_create(instance->device, conf, sess, mp)) {

> >> +		rte_mempool_put(mp, (void *)sess);

> >> +		return NULL;

> >> +	}

> >> +	instance->sess_cnt++;

> >> +

> >> +	return sess;

> >> +}

> >> +

> >> +int

> >> +rte_security_session_update(uint16_t id,

> >> +			    struct rte_security_session *sess,

> >> +			    struct rte_security_session_conf *conf)

> >> +{

> >> +	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->session_update, -ENOTSUP);

> >> +	return instance->ops->session_update(instance->device, sess, conf);

> >> +}

> >> +

> >> +int

> >> +rte_security_session_stats_get(uint16_t id,

> >> +			       struct rte_security_session *sess,

> >> +			       struct rte_security_stats *stats)

> >> +{

> >> +	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->session_stats_get, -ENOTSUP);

> >> +	return instance->ops->session_stats_get(instance->device, sess, stats);

> >> +}

> >> +

> >> +int

> >> +rte_security_session_destroy(uint16_t id, struct rte_security_session *sess)

> >> +{

> >> +	int ret;

> >> +	struct rte_security_ctx *instance;

> >> +	struct rte_mempool *mp = rte_mempool_from_obj(sess);

> >> +

> >> +	RTE_SEC_VALID_ID_OR_ERR_RET(id, -ENODEV);

> >> +	instance = &security_instances[id];

> >> +

> >> +	RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->session_destroy, -ENOTSUP);

> >> +

> >> +	if (instance->sess_cnt)

> >> +		instance->sess_cnt--;

> >> +

> >> +	ret = instance->ops->session_destroy(instance->device, sess);

> >> +	if (!ret)

> >> +		rte_mempool_put(mp, (void *)sess);

> >> +

> >> +	return ret;

> >> +}

> >> +

> >> +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);

> >> +	return instance->ops->set_pkt_metadata(instance->device,

> >> +					       sess, m, params);

> >> +}

> >> +

> >> +const struct rte_security_capability *

> >> +rte_security_capabilities_get(uint16_t id)

> >> +{

> >> +	struct rte_security_ctx *instance;

> >> +

> >> +	RTE_SEC_VALID_ID_OR_ERR_RET(id, NULL);

> >> +	instance = &security_instances[id];

> >> +

> >> +	RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->capabilities_get, NULL);

> >> +	return instance->ops->capabilities_get(instance->device);

> >> +}

> >> +

> >> +const struct rte_security_capability *

> >> +rte_security_capability_get(uint16_t id,

> >> +			    struct rte_security_capability_idx *idx)

> >> +{

> >> +	struct rte_security_ctx *instance;

> >> +	const struct rte_security_capability *capabilities;

> >> +	const struct rte_security_capability *capability;

> >> +	uint16_t i = 0;

> >> +

> >> +	RTE_SEC_VALID_ID_OR_ERR_RET(id, NULL);

> >> +	instance = &security_instances[id];

> >> +

> >> +	RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->capabilities_get, NULL);

> >> +	capabilities = instance->ops->capabilities_get(instance->device);

> >> +

> >> +	if (capabilities == NULL)

> >> +		return NULL;

> >> +

> >> +	while ((capability = &capabilities[i++])->action

> >> +			!= RTE_SECURITY_ACTION_TYPE_NONE) {

> >> +		if (capability->action  == idx->action &&

> >> +				capability->protocol == idx->protocol) {

> >> +			if (idx->protocol == RTE_SECURITY_PROTOCOL_IPSEC) {

> >> +				if (capability->ipsec.proto ==

> >> +						idx->ipsec.proto &&

> >> +					capability->ipsec.mode ==

> >> +							idx->ipsec.mode &&

> >> +					capability->ipsec.direction ==

> >> +							idx->ipsec.direction)

> >> +					return capability;

> >> +			}

> >> +		}

> >> +	}

> >> +

> >> +	return NULL;

> >> +}

> >

> 

> Regards,

> Akhil
  
Akhil Goyal Oct. 10, 2017, 12:17 p.m. UTC | #5
Hi Konstantin,

On 10/9/2017 7:12 PM, Ananyev, Konstantin wrote:
> Hi Akhil,
> 
>> -----Original Message-----
>> From: Akhil Goyal [mailto:akhil.goyal@nxp.com]
>> Sent: Friday, October 6, 2017 7:11 PM
>> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org
>> Cc: Doherty, Declan <declan.doherty@intel.com>; De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; hemant.agrawal@nxp.com;
>> Nicolau, Radu <radu.nicolau@intel.com>; borisp@mellanox.com; aviadye@mellanox.com; thomas@monjalon.net;
>> sandeep.malik@nxp.com; jerin.jacob@caviumnetworks.com; Mcnamara, John <john.mcnamara@intel.com>; olivier.matz@6wind.com
>> Subject: Re: [PATCH v2 01/12] lib/rte_security: add security library
>>
>> Hi Konstantin,
>>
>> Thanks for your comments.
>> On 10/5/2017 10:00 PM, Ananyev, Konstantin wrote:
>>> Hi lads,
>>>
>>>>
>>>> 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           | 275 +++++++++++++++
>>>>    lib/librte_security/rte_security.h           | 495 +++++++++++++++++++++++++++
>>>>    lib/librte_security/rte_security_driver.h    | 182 ++++++++++
>>>>    lib/librte_security/rte_security_version.map |  13 +
>>>>    5 files changed, 1018 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
>>>>

[...]

>>>> +
>>>> +struct rte_security_ctx {
>>>> +	uint16_t id;
>>>> +	enum {
>>>> +		RTE_SECURITY_INSTANCE_INVALID,
>>>> +		RTE_SECURITY_INSTANCE_VALID
>>>> +	} state;
>>>> +	void *device;
>>>> +	struct rte_security_ops *ops;
>>>> +	uint16_t sess_cnt;
>>>> +};
>>>> +
>>>> +static struct rte_security_ctx *security_instances;
>>>> +static uint16_t max_nb_security_instances;
>>>> +static uint16_t nb_security_instances;
>>>
>>> Probably a dumb question - but why do you need a global security_instances []
>>> and why security_instance has to be refrenced by index?
>>> As I understand, with proposed model all drivers have to do something like:
>>> rte_security_register(&eth_dev->data->sec_id,  (void *)eth_dev, &ixgbe_security_ops);
>>> and then all apps would have to:
>>> rte_eth_dev_get_sec_id(portid)
>>> to retrieve that security_instance index.
>>> Why not just treat struct rte_security_ctx* as opaque pointer and make
>>> all related API get/accept it as a paratemer.
>>> To retrieve sec_ctx from device just:
>>> struct rte_security_ctx* rte_ethdev_get_sec_ctx(portid);
>>> struct rte_security_ctx* rte_cryptodev_get_sec_ctx(portid);
>>> ?
>> We would look into this separately.
> 
> Could you clarify what does it mean?
> It will be addressed in v4 or don't plan to address it at all or something else?
We were thinking of improving this beyond the scope of this patchset as 
an incremental patch.


>    
> 
>>>
>>> Another question how currently proposed model with global static array and friends,
>>> supposed to work for DPDK MP model?
>>> Or MP support is not planned?
>> multi process case is planned for future enhancement. This is mentioned
>> in the cover note.
> 
> Great, then I suppose you should have a generic idea how that model will work
> for DPDK multi-process model?
> If so, can you probably share your thoughts, because it is not clear to me.
> Let say user has an ethdev device with ipsec capability and  wants to use it
> from both primary and secondary process.
> What would be a procedure?
> Can user use the same security instance from both processes?
> If yes, then
> - how secondary process will get security_instance_id for that device
> from primary process?
>   By calling rte_eth_dev_get_sec_id(), or something else?
> - how guarantee that all these func pointers inside ops will be mapped to the
> same address inside  processes?
> If not, then does it mean each process has to call rte_security_register()
> for each device?
> But right now you have only one sec_id  inside rte_eth_dev_data.
> 
> Would secondary process be allowed to register/unregister/update security instances?
>   if yes, how the will synchronize?
> Same question for session ops.
> 

Currently multi process case is not handled and we have not put our 
thoughts on resolving this as of now. We were planning to improve this 
beyond the scope of this patchset as an incremental patch.

>>>
>>>> +
>>>> +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)
>>>> +{
> 
> Probably const  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) {
>>>
>>> You probably need try to reuse unregistered entries first?
>>> Konstantin
>>>
>> These APIs are experimental at this moment as mentioned in the patchset.
>> We will try accommodate your comments in future.
> 
> Again could you clarify what do you mean by 'future' here?
> 
As I said before, these APIs need to be re-looked to incorporate the 
multi process cases and better memory utilization.
We intend to include most of your comments separately beyond the scope 
of this patchset.
I believe the complete code should not be put on hold due to these 
issues (multi process and optimizations in register/deregister APIs).
As per my understanding the code is not impacting any of the existing 
functionalities.

However,we will discuss internally if these can be resolved in v4 or not.


>>>
>>>> +		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;
> 
> BTW, I think you need to copy actual contents of ops.
> Otherwise you assuming that ops are static
> (would be valid though all processes lifetimes).
> 
> Konstantin
> 


Regards,
Akhil
  
Ananyev, Konstantin Oct. 11, 2017, 9:02 a.m. UTC | #6
Hi Akhil,

> -----Original Message-----

> From: Akhil Goyal [mailto:akhil.goyal@nxp.com]

> Sent: Tuesday, October 10, 2017 1:17 PM

> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org

> Cc: Doherty, Declan <declan.doherty@intel.com>; De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; hemant.agrawal@nxp.com;

> Nicolau, Radu <radu.nicolau@intel.com>; borisp@mellanox.com; aviadye@mellanox.com; thomas@monjalon.net;

> sandeep.malik@nxp.com; jerin.jacob@caviumnetworks.com; Mcnamara, John <john.mcnamara@intel.com>; olivier.matz@6wind.com

> Subject: Re: [PATCH v2 01/12] lib/rte_security: add security library

> 

> Hi Konstantin,

> 

> On 10/9/2017 7:12 PM, Ananyev, Konstantin wrote:

> > Hi Akhil,

> >

> >> -----Original Message-----

> >> From: Akhil Goyal [mailto:akhil.goyal@nxp.com]

> >> Sent: Friday, October 6, 2017 7:11 PM

> >> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org

> >> Cc: Doherty, Declan <declan.doherty@intel.com>; De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>;

> hemant.agrawal@nxp.com;

> >> Nicolau, Radu <radu.nicolau@intel.com>; borisp@mellanox.com; aviadye@mellanox.com; thomas@monjalon.net;

> >> sandeep.malik@nxp.com; jerin.jacob@caviumnetworks.com; Mcnamara, John <john.mcnamara@intel.com>; olivier.matz@6wind.com

> >> Subject: Re: [PATCH v2 01/12] lib/rte_security: add security library

> >>

> >> Hi Konstantin,

> >>

> >> Thanks for your comments.

> >> On 10/5/2017 10:00 PM, Ananyev, Konstantin wrote:

> >>> Hi lads,

> >>>

> >>>>

> >>>> 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           | 275 +++++++++++++++

> >>>>    lib/librte_security/rte_security.h           | 495 +++++++++++++++++++++++++++

> >>>>    lib/librte_security/rte_security_driver.h    | 182 ++++++++++

> >>>>    lib/librte_security/rte_security_version.map |  13 +

> >>>>    5 files changed, 1018 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

> >>>>

> 

> [...]

> 

> >>>> +

> >>>> +struct rte_security_ctx {

> >>>> +	uint16_t id;

> >>>> +	enum {

> >>>> +		RTE_SECURITY_INSTANCE_INVALID,

> >>>> +		RTE_SECURITY_INSTANCE_VALID

> >>>> +	} state;

> >>>> +	void *device;

> >>>> +	struct rte_security_ops *ops;

> >>>> +	uint16_t sess_cnt;

> >>>> +};

> >>>> +

> >>>> +static struct rte_security_ctx *security_instances;

> >>>> +static uint16_t max_nb_security_instances;

> >>>> +static uint16_t nb_security_instances;

> >>>

> >>> Probably a dumb question - but why do you need a global security_instances []

> >>> and why security_instance has to be refrenced by index?

> >>> As I understand, with proposed model all drivers have to do something like:

> >>> rte_security_register(&eth_dev->data->sec_id,  (void *)eth_dev, &ixgbe_security_ops);

> >>> and then all apps would have to:

> >>> rte_eth_dev_get_sec_id(portid)

> >>> to retrieve that security_instance index.

> >>> Why not just treat struct rte_security_ctx* as opaque pointer and make

> >>> all related API get/accept it as a paratemer.

> >>> To retrieve sec_ctx from device just:

> >>> struct rte_security_ctx* rte_ethdev_get_sec_ctx(portid);

> >>> struct rte_security_ctx* rte_cryptodev_get_sec_ctx(portid);

> >>> ?

> >> We would look into this separately.

> >

> > Could you clarify what does it mean?

> > It will be addressed in v4 or don't plan to address it at all or something else?

> We were thinking of improving this beyond the scope of this patchset as

> an incremental patch.

> 

> 

> >

> >

> >>>

> >>> Another question how currently proposed model with global static array and friends,

> >>> supposed to work for DPDK MP model?

> >>> Or MP support is not planned?

> >> multi process case is planned for future enhancement. This is mentioned

> >> in the cover note.

> >

> > Great, then I suppose you should have a generic idea how that model will work

> > for DPDK multi-process model?

> > If so, can you probably share your thoughts, because it is not clear to me.

> > Let say user has an ethdev device with ipsec capability and  wants to use it

> > from both primary and secondary process.

> > What would be a procedure?

> > Can user use the same security instance from both processes?

> > If yes, then

> > - how secondary process will get security_instance_id for that device

> > from primary process?

> >   By calling rte_eth_dev_get_sec_id(), or something else?

> > - how guarantee that all these func pointers inside ops will be mapped to the

> > same address inside  processes?

> > If not, then does it mean each process has to call rte_security_register()

> > for each device?

> > But right now you have only one sec_id  inside rte_eth_dev_data.

> >

> > Would secondary process be allowed to register/unregister/update security instances?

> >   if yes, how the will synchronize?

> > Same question for session ops.

> >

> 

> Currently multi process case is not handled and we have not put our

> thoughts on resolving this as of now. We were planning to improve this

> beyond the scope of this patchset as an incremental patch.

> 

> >>>

> >>>> +

> >>>> +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)

> >>>> +{

> >

> > Probably const  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) {

> >>>

> >>> You probably need try to reuse unregistered entries first?

> >>> Konstantin

> >>>

> >> These APIs are experimental at this moment as mentioned in the patchset.

> >> We will try accommodate your comments in future.

> >

> > Again could you clarify what do you mean by 'future' here?

> >

> As I said before, these APIs need to be re-looked to incorporate the

> multi process cases and better memory utilization.

> We intend to include most of your comments separately beyond the scope

> of this patchset.

> I believe the complete code should not be put on hold due to these

> issues (multi process and optimizations in register/deregister APIs).

> As per my understanding the code is not impacting any of the existing

> functionalities.


I don't have a problem if rte_security code by itself will be reworked in future releases.
But rte_security is not standalone - changes in public API and major design principles
for rte_security means code changes for rte_ethdev, ixgbe, etc.
Again not the end of the world, but would be better to minimize such code churn if possible.
So if you plan to make changes in public API I talked before:
rework/remove rte_security_register/unregister(),
replace rte_eth_dev_get_sec_id(portid) with struct rte_security_ctx* rte_ethdev_get_sec_ctx(portid) or so.
etc. 
anyway - I think it would be really good to have them in 17.11 timeframe.

> 

> However,we will discuss internally if these can be resolved in v4 or not.

> 


Will wait for v4 then.
Konstantin

> 

> >>>

> >>>> +		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;

> >

> > BTW, I think you need to copy actual contents of ops.

> > Otherwise you assuming that ops are static

> > (would be valid though all processes lifetimes).

> >

> > Konstantin

> >

> 

> 

> Regards,

> Akhil
  

Patch

diff --git a/lib/librte_security/Makefile b/lib/librte_security/Makefile
new file mode 100644
index 0000000..af87bb2
--- /dev/null
+++ b/lib/librte_security/Makefile
@@ -0,0 +1,53 @@ 
+#   BSD LICENSE
+#
+#   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 Intel Corporation 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_SDK)/mk/rte.vars.mk
+
+# library name
+LIB = librte_security.a
+
+# library version
+LIBABIVER := 1
+
+# build flags
+CFLAGS += -O3
+CFLAGS += $(WERROR_FLAGS)
+
+# library source files
+SRCS-y += rte_security.c
+
+# export include files
+SYMLINK-y-include += rte_security.h
+SYMLINK-y-include += rte_security_driver.h
+
+# versioning export map
+EXPORT_MAP := rte_security_version.map
+
+include $(RTE_SDK)/mk/rte.lib.mk
diff --git a/lib/librte_security/rte_security.c b/lib/librte_security/rte_security.c
new file mode 100644
index 0000000..97d3857
--- /dev/null
+++ b/lib/librte_security/rte_security.c
@@ -0,0 +1,275 @@ 
+/*-
+ *   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,
+		RTE_SECURITY_INSTANCE_VALID
+	} state;
+	void *device;
+	struct rte_security_ops *ops;
+	uint16_t sess_cnt;
+};
+
+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);
+
+		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;
+	security_instances[*id].sess_cnt = 0;
+
+	return 0;
+}
+
+int
+rte_security_unregister(uint16_t id)
+{
+	struct rte_security_ctx *instance;
+
+	RTE_SEC_VALID_ID_OR_ERR_RET(id, -ENODEV);
+	instance = &security_instances[id];
+
+	if (instance->sess_cnt)
+		return -EBUSY;
+
+	memset(instance, 0, sizeof(*instance));
+	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;
+
+	RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->session_create, NULL);
+
+	if (rte_mempool_get(mp, (void *)&sess))
+		return NULL;
+
+	if (instance->ops->session_create(instance->device, conf, sess, mp)) {
+		rte_mempool_put(mp, (void *)sess);
+		return NULL;
+	}
+	instance->sess_cnt++;
+
+	return sess;
+}
+
+int
+rte_security_session_update(uint16_t id,
+			    struct rte_security_session *sess,
+			    struct rte_security_session_conf *conf)
+{
+	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->session_update, -ENOTSUP);
+	return instance->ops->session_update(instance->device, sess, conf);
+}
+
+int
+rte_security_session_stats_get(uint16_t id,
+			       struct rte_security_session *sess,
+			       struct rte_security_stats *stats)
+{
+	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->session_stats_get, -ENOTSUP);
+	return instance->ops->session_stats_get(instance->device, sess, stats);
+}
+
+int
+rte_security_session_destroy(uint16_t id, struct rte_security_session *sess)
+{
+	int ret;
+	struct rte_security_ctx *instance;
+	struct rte_mempool *mp = rte_mempool_from_obj(sess);
+
+	RTE_SEC_VALID_ID_OR_ERR_RET(id, -ENODEV);
+	instance = &security_instances[id];
+
+	RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->session_destroy, -ENOTSUP);
+
+	if (instance->sess_cnt)
+		instance->sess_cnt--;
+
+	ret = instance->ops->session_destroy(instance->device, sess);
+	if (!ret)
+		rte_mempool_put(mp, (void *)sess);
+
+	return ret;
+}
+
+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);
+	return instance->ops->set_pkt_metadata(instance->device,
+					       sess, m, params);
+}
+
+const struct rte_security_capability *
+rte_security_capabilities_get(uint16_t id)
+{
+	struct rte_security_ctx *instance;
+
+	RTE_SEC_VALID_ID_OR_ERR_RET(id, NULL);
+	instance = &security_instances[id];
+
+	RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->capabilities_get, NULL);
+	return instance->ops->capabilities_get(instance->device);
+}
+
+const struct rte_security_capability *
+rte_security_capability_get(uint16_t id,
+			    struct rte_security_capability_idx *idx)
+{
+	struct rte_security_ctx *instance;
+	const struct rte_security_capability *capabilities;
+	const struct rte_security_capability *capability;
+	uint16_t i = 0;
+
+	RTE_SEC_VALID_ID_OR_ERR_RET(id, NULL);
+	instance = &security_instances[id];
+
+	RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->capabilities_get, NULL);
+	capabilities = instance->ops->capabilities_get(instance->device);
+
+	if (capabilities == NULL)
+		return NULL;
+
+	while ((capability = &capabilities[i++])->action
+			!= RTE_SECURITY_ACTION_TYPE_NONE) {
+		if (capability->action  == idx->action &&
+				capability->protocol == idx->protocol) {
+			if (idx->protocol == RTE_SECURITY_PROTOCOL_IPSEC) {
+				if (capability->ipsec.proto ==
+						idx->ipsec.proto &&
+					capability->ipsec.mode ==
+							idx->ipsec.mode &&
+					capability->ipsec.direction ==
+							idx->ipsec.direction)
+					return capability;
+			}
+		}
+	}
+
+	return NULL;
+}
diff --git a/lib/librte_security/rte_security.h b/lib/librte_security/rte_security.h
new file mode 100644
index 0000000..9e639fd
--- /dev/null
+++ b/lib/librte_security/rte_security.h
@@ -0,0 +1,495 @@ 
+/*-
+ *   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_common.h>
+#include <rte_crypto.h>
+#include <rte_mbuf.h>
+#include <rte_memory.h>
+#include <rte_mempool.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,
+	/**< 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 */
+	RTE_STD_C11
+	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 */
+	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 {
+	void *sess_private_data;
+	/**< Private session material */
+};
+
+/**
+ * Create security session as specified by the session configuration
+ *
+ * @param   id		security instance identifier id
+ * @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,
+			    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);
+
+/**
+ * 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
+ *
+ * @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 symmetric crypto operation
+ *
+ * @param	sym_op	crypto operation
+ * @param	sess	security session
+ */
+static inline int
+__rte_security_attach_session(struct rte_crypto_sym_op *sym_op,
+			      struct rte_security_session *sess)
+{
+	sym_op->sec_session = sess;
+
+	return 0;
+}
+
+static inline void *
+get_sec_session_private_data(const struct rte_security_session *sess)
+{
+	return sess->sess_private_data;
+}
+
+static inline void
+set_sec_session_private_data(struct rte_security_session *sess,
+			     void *private_data)
+{
+	sess->sess_private_data = private_data;
+}
+
+/**
+ * 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 -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;
+	};
+};
+
+/**
+ * Get 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_stats_get(uint16_t id,
+			       struct rte_security_session *sess,
+			       struct rte_security_stats *stats);
+
+/**
+ * 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;
+	};
+};
+
+/**
+ *  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);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* _RTE_SECURITY_H_ */
diff --git a/lib/librte_security/rte_security_driver.h b/lib/librte_security/rte_security_driver.h
new file mode 100644
index 0000000..5134128
--- /dev/null
+++ b/lib/librte_security/rte_security_driver.h
@@ -0,0 +1,182 @@ 
+/*-
+ *   BSD LICENSE
+ *
+ *   Copyright(c) 2017 Intel Corporation. All rights reserved.
+ *   Copyright 2017 NXP.
+ *
+ *   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 Intel Corporation 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_DRIVER_H_
+#define _RTE_SECURITY_DRIVER_H_
+
+/**
+ * @file rte_security_driver.h
+ *
+ * RTE Security Common Definitions
+ *
+ */
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#include "rte_security.h"
+
+/**
+ * Configure a security session on a device.
+ *
+ * @param	device		Crypto/eth device pointer
+ * @param	conf		Security session configuration
+ * @param	sess		Pointer to Security private session structure
+ * @param	mp		Mempool where the private session is allocated
+ *
+ * @return
+ *  - Returns 0 if private session structure have been created successfully.
+ *  - Returns -EINVAL if input parameters are invalid.
+ *  - Returns -ENOTSUP if crypto device does not support the crypto transform.
+ *  - Returns -ENOMEM if the private session could not be allocated.
+ */
+typedef int (*security_session_create_t)(void *device,
+		struct rte_security_session_conf *conf,
+		struct rte_security_session *sess,
+		struct rte_mempool *mp);
+
+/**
+ * Free driver private session data.
+ *
+ * @param	dev		Crypto/eth device pointer
+ * @param	sess		Security session structure
+ */
+typedef int (*security_session_destroy_t)(void *device,
+		struct rte_security_session *sess);
+
+/**
+ * Update driver private session data.
+ *
+ * @param	device		Crypto/eth device pointer
+ * @param	sess		Pointer to Security private session structure
+ * @param	conf		Security session configuration
+ *
+ * @return
+ *  - Returns 0 if private session structure have been updated successfully.
+ *  - Returns -EINVAL if input parameters are invalid.
+ *  - Returns -ENOTSUP if crypto device does not support the crypto transform.
+ */
+typedef int (*security_session_update_t)(void *device,
+		struct rte_security_session *sess,
+		struct rte_security_session_conf *conf);
+/**
+ * Get 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_stats_get_t)(void *device,
+		struct rte_security_session *sess,
+		struct rte_security_stats *stats);
+
+/**
+ * Update the mbuf with provided metadata.
+ *
+ * @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);
+
+/**
+ * Get security capabilities of the device.
+ *
+ * @param	device		crypto/eth device pointer
+ *
+ * @return
+ *  - Returns rte_security_capability pointer on success.
+ *  - Returns NULL on error.
+ */
+typedef const struct rte_security_capability *(*security_capabilities_get_t)(
+		void *device);
+
+/** Security operations function pointer table */
+struct rte_security_ops {
+	security_session_create_t session_create;
+	/**< Configure a security session. */
+	security_session_update_t session_update;
+	/**< Update a security session. */
+	security_session_stats_get_t session_stats_get;
+	/**< Get security session statistics. */
+	security_session_destroy_t session_destroy;
+	/**< Clear a security sessions private data. */
+	security_set_pkt_metadata_t set_pkt_metadata;
+	/**< Update mbuf metadata. */
+	security_capabilities_get_t capabilities_get;
+	/**< Get security capabilities. */
+};
+
+/**
+ * Register a Crypto/eth device for security operations.
+ *
+ * @param	id		id of the crypto/eth device
+ * @param	device		crypto/eth device pointer
+ * @param	ops		Security ops to be supported by device
+ *
+ * @return
+ *  - Returns 0 if metadata updated successfully.
+ *  - Returns -ve value for errors.
+ */
+int
+rte_security_register(uint16_t *id, void *device,
+		      struct rte_security_ops *ops);
+
+/**
+ * Unregister a Crypto/eth device for security operations.
+ *
+ * @param	id		id of the crypto/eth device
+ *
+ * @return
+ *  - Returns 0 if device is successfully unregistered.
+ *  - Returns -ve value for errors.
+ */
+int
+rte_security_unregister(uint16_t id);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* _RTE_SECURITY_DRIVER_H_ */
diff --git a/lib/librte_security/rte_security_version.map b/lib/librte_security/rte_security_version.map
new file mode 100644
index 0000000..82b7921
--- /dev/null
+++ b/lib/librte_security/rte_security_version.map
@@ -0,0 +1,13 @@ 
+DPDK_17.11 {
+	global:
+
+	rte_security_attach_session;
+	rte_security_capabilities_get;
+	rte_security_capability_get;
+	rte_security_session_create;
+	rte_security_session_free;
+	rte_security_session_query;
+	rte_security_session_update;
+	rte_security_set_pkt_metadata;
+
+};