From mboxrd@z Thu Jan 1 00:00:00 1970 From: Radu Nicolau Subject: Re: [PATCH 01/11] lib/rte_security: add security library Date: Fri, 22 Sep 2017 12:55:52 +0100 Message-ID: References: <20170914082651.26232-1-akhil.goyal@nxp.com> <20170914082651.26232-2-akhil.goyal@nxp.com> <20170918131333.GA23830@jerin> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Cc: dev@dpdk.org, declan.doherty@intel.com, pablo.de.lara.guarch@intel.com, hemant.agrawal@nxp.com, borisp@mellanox.com, aviadye@mellanox.com, thomas@monjalon.net, sandeep.malik@nxp.com To: Jerin Jacob , Akhil Goyal Return-path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id 88344DE3 for ; Fri, 22 Sep 2017 13:55:56 +0200 (CEST) In-Reply-To: <20170918131333.GA23830@jerin> 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, I will address most of the issues in the v2, except the one related to the multiprocess issue - we may need more discussions on that. Thanks for reviewing, Radu On 9/18/2017 2:13 PM, Jerin Jacob wrote: > -----Original Message----- >> Date: Thu, 14 Sep 2017 13:56:41 +0530 >> From: Akhil Goyal >> 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); >> +