[dpdk-dev] [PATCH v4 06/12] ethdev: support security APIs

[Nicolau, Radu]

Hi Shahaf,

I will address the issues asap, they didn't made it into v4 because of timing reasons.

Regards,
Radu

> -----Original Message-----
> From: Shahaf Shuler [mailto:shahafs at mellanox.com]
> Sent: Sunday, October 15, 2017 2:13 PM
> To: Akhil Goyal <akhil.goyal at nxp.com>; dev at dpdk.org
> Cc: Doherty, Declan <declan.doherty at intel.com>; De Lara Guarch, Pablo
> <pablo.de.lara.guarch at intel.com>; hemant.agrawal at nxp.com; Nicolau,
> Radu <radu.nicolau at intel.com>; Boris Pismenny <borisp at mellanox.com>;
> Aviad Yehezkel <aviadye at mellanox.com>; Thomas Monjalon
> <thomas at monjalon.net>; sandeep.malik at nxp.com;
> jerin.jacob at caviumnetworks.com; Mcnamara, John
> <john.mcnamara at intel.com>; Ananyev, Konstantin
> <konstantin.ananyev at intel.com>; olivier.matz at 6wind.com
> Subject: RE: [PATCH v4 06/12] ethdev: support security APIs
> 
> Hi Akhil,
> 
> Sunday, October 15, 2017 1:17 AM, 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           | 18 ++++++++++++++++--
> >  lib/librte_ether/rte_ethdev_version.map |  1 +
> >  3 files changed, 28 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/librte_ether/rte_ethdev.c
> > b/lib/librte_ether/rte_ethdev.c index 0b1e928..9520f1e 100644
> > --- a/lib/librte_ether/rte_ethdev.c
> > +++ b/lib/librte_ether/rte_ethdev.c
> > @@ -301,6 +301,17 @@ rte_eth_dev_socket_id(uint16_t port_id)
> >  	return rte_eth_devices[port_id].data->numa_node;
> >  }
> >
> > +void *
> > +rte_eth_dev_get_sec_ctx(uint8_t port_id) {
> > +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, NULL);
> > +
> > +	if (rte_eth_devices[port_id].data->dev_flags &
> > RTE_ETH_DEV_SECURITY)
> > +		return rte_eth_devices[port_id].data->security_ctx;
> > +
> > +	return NULL;
> > +}
> > +
> >  uint16_t
> >  rte_eth_dev_count(void)
> >  {
> > diff --git a/lib/librte_ether/rte_ethdev.h
> > b/lib/librte_ether/rte_ethdev.h index aaf02b3..159bb73 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"
> > @@ -379,7 +381,8 @@ struct rte_eth_rxmode {
> >  		 * This bit is temporary till rxmode bitfield offloads API will
> >  		 * be deprecated.
> >  		 */
> > -		ignore_offload_bitfield : 1;
> > +		ignore_offload_bitfield : 1,
> > +		enable_sec       : 1; /**< Enable security offload */
> 
> I suggest to keep the ignore_offload_bitfield last.
> 
> Also you should update the convert function. See:
> rte_eth_convert_rx_offload_bitfield
> rte_eth_convert_rx_offloads
> 
> >  };
> >
> >  /**
> > @@ -707,8 +710,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 */
> 
> Am copying the comment and answer from v2 on the Tx offload. Seems like
> we agreed, why it is not addressed?
> 
> From: Radu Nicolau radu.nicolau at intel.com
> > 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.
> 
> 
> >  };
> >
> >  /**
> > @@ -969,6 +974,7 @@ struct rte_eth_conf {  #define
> DEV_RX_OFFLOAD_VLAN
> > (DEV_RX_OFFLOAD_VLAN_STRIP | \
> >  			     DEV_RX_OFFLOAD_VLAN_FILTER | \
> >  			     DEV_RX_OFFLOAD_VLAN_EXTEND)
> > +#define DEV_RX_OFFLOAD_SECURITY         0x00000100
> >
> >  /**
> >   * TX offload capabilities of a device.
> > @@ -998,6 +1004,7 @@ struct rte_eth_conf {
> >   *   When set application must guarantee that per-queue all mbufs comes
> > from
> >   *   the same mempool and has refcnt = 1.
> >   */
> > +#define DEV_TX_OFFLOAD_SECURITY         0x00008000
> >
> >  struct rte_pci_device;
> >
> > @@ -1736,6 +1743,9 @@ struct rte_eth_dev {
> >  	enum rte_eth_dev_state state; /**< Flag indicating the port state */
> > } __rte_cache_aligned;
> >
> > +void *
> > +rte_eth_dev_get_sec_ctx(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 */
> > @@ -1796,6 +1806,8 @@ struct rte_eth_dev_data {
> >  	int numa_node;  /**< NUMA node connection */
> >  	struct rte_vlan_filter_conf vlan_filter_conf;
> >  	/**< VLAN filter configuration. */
> > +	void *security_ctx;
> > +	/**< Context for security ops  */
> >  };
> >
> >  /** Device supports hotplug detach */ @@ -1806,6 +1818,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 have to insist about this one. I don't understand which extra functionality it
> provides in compare to the DEV_RX_OFFLOAD_SECURITY or
> DEV_TX_OFFLOAD_SECURITY.
> Answer from previous version was to "allow to advertise that a device  has
> security features without the need to check exactly which ones are they".
> I think this is exactly what DEV_RX_OFFLOAD_SECURITY and
> DEV_TX_OFFLOAD_SECURITY means. Those flags does not provide the full
> capabilities of the different security offload supported by the device (those
> should be queried through rte_scurity APIs).
> 
> >
> >  /**
> >   * @internal
> > diff --git a/lib/librte_ether/rte_ethdev_version.map
> > b/lib/librte_ether/rte_ethdev_version.map
> > index e27f596..3cc6a64 100644
> > --- a/lib/librte_ether/rte_ethdev_version.map
> > +++ b/lib/librte_ether/rte_ethdev_version.map
> > @@ -194,5 +194,6 @@ DPDK_17.11 {
> >  	rte_eth_dev_pool_ops_supported;
> >  	rte_eth_dev_reset;
> >  	rte_flow_error_set;
> > +	rte_eth_dev_get_sec_ctx;
> >
> >  } DPDK_17.08;
> > --
> > 2.9.3