[dpdk-dev] [PATCH v2 06/12] ethdev: extend ethdev to support security APIs

[Radu Nicolau]

Hi,

Comments inline, thanks for reviewing!


On 10/4/2017 11:52 AM, Shahaf Shuler wrote:
> Tuesday, October 3, 2017 4:14 PM, Akhil Goyal:
>> From: Declan Doherty <declan.doherty at 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 at mellanox.com>
>> Signed-off-by: Aviad Yehezkel <aviadye at mellanox.com>
>> Signed-off-by: Radu Nicolau <radu.nicolau at intel.com>
>> Signed-off-by: Declan Doherty <declan.doherty at 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.
I agree with you, that is if the new offload API will be merged we will 
also change this one. But until then it makes testing and developing 
more difficult.
>
>>   };
>>
>>   /**
>> @@ -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.
The DEV_TX_OFFLOAD_SEC_NEED_MDATA  will be moved to the capabilities in 
the v3.
>
>>   /**< 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.
I think the reason to have this is to allow to advertise that a device 
has security features without the need to check exactly which ones are 
they...
>
>>   /**
>>    * @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
>