From mboxrd@z Thu Jan 1 00:00:00 1970 From: Akhil Goyal Subject: Re: [PATCH v2 01/12] lib/rte_security: add security library Date: Fri, 6 Oct 2017 23:41:00 +0530 Message-ID: <15007bf0-d263-05a0-2009-31210f686085@nxp.com> References: <20170914082651.26232-1-akhil.goyal@nxp.com> <20171003131413.23846-1-akhil.goyal@nxp.com> <20171003131413.23846-2-akhil.goyal@nxp.com> <2601191342CEEE43887BDE71AB9772585FAA4E58@IRSMSX103.ger.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Cc: "Doherty, Declan" , "De Lara Guarch, Pablo" , "hemant.agrawal@nxp.com" , "Nicolau, Radu" , "borisp@mellanox.com" , "aviadye@mellanox.com" , "thomas@monjalon.net" , "sandeep.malik@nxp.com" , "jerin.jacob@caviumnetworks.com" , "Mcnamara, John" , "olivier.matz@6wind.com" To: "Ananyev, Konstantin" , "dev@dpdk.org" Return-path: Received: from NAM01-BN3-obe.outbound.protection.outlook.com (mail-bn3nam01on0049.outbound.protection.outlook.com [104.47.33.49]) by dpdk.org (Postfix) with ESMTP id AE0861B206 for ; Fri, 6 Oct 2017 20:11:14 +0200 (CEST) In-Reply-To: <2601191342CEEE43887BDE71AB9772585FAA4E58@IRSMSX103.ger.corp.intel.com> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi 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 >> Signed-off-by: Boris Pismenny >> Signed-off-by: Radu Nicolau >> Signed-off-by: Declan Doherty >> --- >> 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 >> +#include >> + >> +#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(ð_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