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