Re: [PATCH v2 06/12] ethdev: extend ethdev to support security APIs

[Shahaf Shuler <shahafs@mellanox.com>]

Tuesday, October 3, 2017 4:14 PM, Akhil Goyal:
> From: Declan Doherty <declan.doherty@intel.com>
> 
> rte_flow_action type and ethdev updated to support rte_security sessions
> for crypto offload to ethernet device.
> 
> Signed-off-by: Boris Pismenny <borisp@mellanox.com>
> Signed-off-by: Aviad Yehezkel <aviadye@mellanox.com>
> Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
> Signed-off-by: Declan Doherty <declan.doherty@intel.com>
> ---
>  lib/librte_ether/rte_ethdev.c           | 11 +++++++++++
>  lib/librte_ether/rte_ethdev.h           | 19 +++++++++++++++++--
>  lib/librte_ether/rte_ethdev_version.map |  7 +++++++
>  3 files changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index 0597641..f51c5a5 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -302,6 +302,17 @@ rte_eth_dev_socket_id(uint8_t port_id)
>  	return rte_eth_devices[port_id].data->numa_node;
>  }
> 
> +uint16_t
> +rte_eth_dev_get_sec_id(uint8_t port_id) {
> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -1);
> +
> +	if (rte_eth_devices[port_id].data->dev_flags &
> RTE_ETH_DEV_SECURITY)
> +		return rte_eth_devices[port_id].data->sec_id;
> +
> +	return -1;
> +}
> +
>  uint8_t
>  rte_eth_dev_count(void)
>  {
> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
> index 0adf327..193ad62 100644
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -180,6 +180,8 @@ extern "C" {
>  #include <rte_dev.h>
>  #include <rte_devargs.h>
>  #include <rte_errno.h>
> +#include <rte_common.h>
> +
>  #include "rte_ether.h"
>  #include "rte_eth_ctrl.h"
>  #include "rte_dev_info.h"
> @@ -357,7 +359,8 @@ struct rte_eth_rxmode {
>  		jumbo_frame      : 1, /**< Jumbo Frame Receipt enable. */
>  		hw_strip_crc     : 1, /**< Enable CRC stripping by hardware. */
>  		enable_scatter   : 1, /**< Enable scatter packets rx handler */
> -		enable_lro       : 1; /**< Enable LRO */
> +		enable_lro       : 1, /**< Enable LRO */
> +		enable_sec       : 1; /**< Enable security offload */
>  };
> 
>  /**
> @@ -679,8 +682,10 @@ struct rte_eth_txmode {
>  		/**< If set, reject sending out tagged pkts */
>  		hw_vlan_reject_untagged : 1,
>  		/**< If set, reject sending out untagged pkts */
> -		hw_vlan_insert_pvid : 1;
> +		hw_vlan_insert_pvid : 1,
>  		/**< If set, enable port based VLAN insertion */
> +		enable_sec       : 1;
> +		/**< Enable security offload */

Already comment on it in the previous version [1]. 
I don't think there is a justification to introduce new approach to set Tx offloads given there is already patch set which provides such new API [2].
I think this patch should be on top of it.

>  };
> 
>  /**
> @@ -907,6 +912,7 @@ struct rte_eth_conf {  #define
> DEV_RX_OFFLOAD_QINQ_STRIP  0x00000020  #define
> DEV_RX_OFFLOAD_OUTER_IPV4_CKSUM 0x00000040
>  #define DEV_RX_OFFLOAD_MACSEC_STRIP     0x00000080
> +#define DEV_RX_OFFLOAD_SECURITY         0x00000100
> 
>  /**
>   * TX offload capabilities of a device.
> @@ -926,6 +932,8 @@ struct rte_eth_conf {
>  #define DEV_TX_OFFLOAD_GENEVE_TNL_TSO   0x00001000    /**< Used for
> tunneling packet. */
>  #define DEV_TX_OFFLOAD_MACSEC_INSERT    0x00002000
>  #define DEV_TX_OFFLOAD_MT_LOCKFREE      0x00004000
> +#define DEV_TX_OFFLOAD_SECURITY         0x00008000
> +#define DEV_TX_OFFLOAD_SEC_NEED_MDATA   0x00010000

Isn't it better to have the DEV_TX_OFFLOAD_SEC_NEED_MDATA  as part of rte_security_capability struct?
The "need metadata" should be per security operation indication.
For example It is possible that PMD will be able to do IPSEC without the need in metadata and PDCP with the need in metadata. 

IMO the better model is for the PMD to advertise it support all kind of security offloads by setting the DEV_TX_OFFLOAD_SECURITY flag. Application will be able to query more fine-grained capabilities per security operation using rte_security APIs.

>  /**< Multiple threads can invoke rte_eth_tx_burst() concurrently on the
> same
>   * tx queue without SW lock.
>   */
> @@ -1651,6 +1659,9 @@ struct rte_eth_dev {
>  	enum rte_eth_dev_state state; /**< Flag indicating the port state */
> } __rte_cache_aligned;
> 
> +uint16_t
> +rte_eth_dev_get_sec_id(uint8_t port_id);
> +
>  struct rte_eth_dev_sriov {
>  	uint8_t active;               /**< SRIOV is active with 16, 32 or 64 pools */
>  	uint8_t nb_q_per_pool;        /**< rx queue number per pool */
> @@ -1711,6 +1722,8 @@ struct rte_eth_dev_data {
>  	int numa_node;  /**< NUMA node connection */
>  	struct rte_vlan_filter_conf vlan_filter_conf;
>  	/**< VLAN filter configuration. */
> +	uint16_t sec_id;
> +	/**< security instance identifier */
>  };
> 
>  /** Device supports hotplug detach */
> @@ -1721,6 +1734,8 @@ struct rte_eth_dev_data {  #define
> RTE_ETH_DEV_BONDED_SLAVE 0x0004
>  /** Device supports device removal interrupt */
>  #define RTE_ETH_DEV_INTR_RMV     0x0008
> +/** Device supports inline security processing */
> +#define RTE_ETH_DEV_SECURITY    0x0010

I still not understand why this flag is needed. 
The PMD can advertise its supports rte_security by setting the corresponding DEV_TX_OFFLOAD_SECURITY and DEV_RX_OFFLOAD_SECURITY flags.
Etherdev layer can validate the sec_id using those flags. 
The various security offloads, as I mentioned above, should be part of rte_security_capability query. 

> 
>  /**
>   * @internal
> diff --git a/lib/librte_ether/rte_ethdev_version.map
> b/lib/librte_ether/rte_ethdev_version.map
> index 4283728..24cbd7d 100644
> --- a/lib/librte_ether/rte_ethdev_version.map
> +++ b/lib/librte_ether/rte_ethdev_version.map
> @@ -187,3 +187,10 @@ DPDK_17.08 {
>  	rte_tm_wred_profile_delete;
> 
>  } DPDK_17.05;
> +
> +DPDK_17.11 {
> +	global:
> +
> +	rte_eth_dev_get_sec_id;
> +
> +} DPDK_17.08;
> --
> 2.9.3


[1] http://dpdk.org/ml/archives/dev/2017-September/076382.html
[2] http://dpdk.org/ml/archives/dev/2017-October/077329.html