From mboxrd@z Thu Jan 1 00:00:00 1970 From: Radu Nicolau Subject: Re: [PATCH v2 06/12] ethdev: extend ethdev to support security APIs Date: Fri, 6 Oct 2017 17:31:47 +0100 Message-ID: <52ecca28-3ffc-56fd-3803-bec0263817ca@intel.com> References: <20170914082651.26232-1-akhil.goyal@nxp.com> <20171003131413.23846-1-akhil.goyal@nxp.com> <20171003131413.23846-7-akhil.goyal@nxp.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Cc: "declan.doherty@intel.com" , "pablo.de.lara.guarch@intel.com" , "hemant.agrawal@nxp.com" , Boris Pismenny , Aviad Yehezkel , Thomas Monjalon , "sandeep.malik@nxp.com" , "jerin.jacob@caviumnetworks.com" , "john.mcnamara@intel.com" , "olivier.matz@6wind.com" To: Shahaf Shuler , Akhil Goyal , "dev@dpdk.org" Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id A63451B1FB for ; Fri, 6 Oct 2017 18:31:56 +0200 (CEST) In-Reply-To: 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, 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 >> >> rte_flow_action type and ethdev updated to support rte_security sessions >> for crypto offload to ethernet device. >> >> Signed-off-by: Boris Pismenny >> Signed-off-by: Aviad Yehezkel >> Signed-off-by: Radu Nicolau >> Signed-off-by: Declan Doherty >> --- >> 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 >> #include >> #include >> +#include >> + >> #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 >