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