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

  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           | 18 ++++++++++++++++--
 lib/librte_ether/rte_ethdev_version.map |  1 +
 3 files changed, 28 insertions(+), 2 deletions(-)
  

On 10/15/2017 1:17 AM, Akhil Goyal wrote:
> 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           | 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 */
>   };
>   
>   /**
> @@ -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 */
>   };
>   
>   /**
> @@ -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
>   
>   /**
>    * @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;

Tested-by: Aviad Yehezkel <aviadye@mellanox.com>
  

Hi Akhil,

Sunday, October 15, 2017 1:17 AM, 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           | 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
  

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@mellanox.com]
> Sent: Sunday, October 15, 2017 2:13 PM
> To: Akhil Goyal <akhil.goyal@nxp.com>; dev@dpdk.org
> Cc: Doherty, Declan <declan.doherty@intel.com>; De Lara Guarch, Pablo
> <pablo.de.lara.guarch@intel.com>; hemant.agrawal@nxp.com; Nicolau,
> Radu <radu.nicolau@intel.com>; Boris Pismenny <borisp@mellanox.com>;
> Aviad Yehezkel <aviadye@mellanox.com>; Thomas Monjalon
> <thomas@monjalon.net>; sandeep.malik@nxp.com;
> jerin.jacob@caviumnetworks.com; Mcnamara, John
> <john.mcnamara@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; olivier.matz@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@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           | 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
  

Hi guys,

> -----Original Message-----
> From: Akhil Goyal [mailto:akhil.goyal@nxp.com]
> Sent: Saturday, October 14, 2017 11:17 PM
> To: dev@dpdk.org
> Cc: Doherty, Declan <declan.doherty@intel.com>; De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; hemant.agrawal@nxp.com;
> Nicolau, Radu <radu.nicolau@intel.com>; borisp@mellanox.com; aviadye@mellanox.com; thomas@monjalon.net;
> sandeep.malik@nxp.com; jerin.jacob@caviumnetworks.com; Mcnamara, John <john.mcnamara@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; shahafs@mellanox.com; olivier.matz@6wind.com
> Subject: [PATCH v4 06/12] ethdev: support security APIs
> 
> 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           | 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)


As you don't currently support MP, it is probably worth to add somewhere
(here or at PMD layer) check for process type.
Something like:
if (rte_eal_process_type() != RTE_PROC_PRIMARY)
       return NULL;
or so.
Konstantin
  

15/10/2017 00:17, Akhil Goyal:
> --- 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;

Please keep alphabetical order.
  

Hi Konstantin,

On 10/19/2017 2:53 PM, Ananyev, Konstantin wrote:
> Hi guys,
> 
>> -----Original Message-----
>> From: Akhil Goyal [mailto:akhil.goyal@nxp.com]
>> Sent: Saturday, October 14, 2017 11:17 PM
>> To: dev@dpdk.org
>> Cc: Doherty, Declan <declan.doherty@intel.com>; De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; hemant.agrawal@nxp.com;
>> Nicolau, Radu <radu.nicolau@intel.com>; borisp@mellanox.com; aviadye@mellanox.com; thomas@monjalon.net;
>> sandeep.malik@nxp.com; jerin.jacob@caviumnetworks.com; Mcnamara, John <john.mcnamara@intel.com>; Ananyev, Konstantin
>> <konstantin.ananyev@intel.com>; shahafs@mellanox.com; olivier.matz@6wind.com
>> Subject: [PATCH v4 06/12] ethdev: support security APIs
>>
>> 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           | 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)
> 
> 
> As you don't currently support MP, it is probably worth to add somewhere
> (here or at PMD layer) check for process type.
> Something like:
> if (rte_eal_process_type() != RTE_PROC_PRIMARY)
>         return NULL;
> or so.
> Konstantin
> 
> 
The MP issue is resolved as per my understanding in the v4.
SO I believe this check is not required anymore. Do you see any issue in MP.

-Akhil
  

On 10/20/2017 4:28 PM, Thomas Monjalon wrote:
> 15/10/2017 00:17, Akhil Goyal:
>> --- 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;
> 
> Please keep alphabetical order.
> 
> 
ok. Will fix the same for rte_cryptodev_version.map also.
  

Hi Akhil,

> 

> Hi Konstantin,

> 

> On 10/19/2017 2:53 PM, Ananyev, Konstantin wrote:

> > Hi guys,

> >

> >> -----Original Message-----

> >> From: Akhil Goyal [mailto:akhil.goyal@nxp.com]

> >> Sent: Saturday, October 14, 2017 11:17 PM

> >> To: dev@dpdk.org

> >> Cc: Doherty, Declan <declan.doherty@intel.com>; De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>;

> hemant.agrawal@nxp.com;

> >> Nicolau, Radu <radu.nicolau@intel.com>; borisp@mellanox.com; aviadye@mellanox.com; thomas@monjalon.net;

> >> sandeep.malik@nxp.com; jerin.jacob@caviumnetworks.com; Mcnamara, John <john.mcnamara@intel.com>; Ananyev, Konstantin

> >> <konstantin.ananyev@intel.com>; shahafs@mellanox.com; olivier.matz@6wind.com

> >> Subject: [PATCH v4 06/12] ethdev: support security APIs

> >>

> >> 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           | 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)

> >

> >

> > As you don't currently support MP, it is probably worth to add somewhere

> > (here or at PMD layer) check for process type.

> > Something like:

> > if (rte_eal_process_type() != RTE_PROC_PRIMARY)

> >         return NULL;

> > or so.

> > Konstantin

> >

> >

> The MP issue is resolved as per my understanding in the v4.


As I can see from v4 - MP is still not supported:

1. security_ctx is placed into rte_eth_dev_data (which is shared between multiple processes)
while it still contains a pointer to particular ops functions.
To support MP you'll probably need to split security_ctx into 2 parts: 
private to process (ops) and shared between processes (actual data),
or comeup with some other (smarter) way.  
2. At least ixgbe_dev_init() right now always blindly allocates new 
    security_ctx and overwrites  eth_dev->data->security_ctx with this new value.

I do remember that for you didn't plan to support MP for 17.11 anyway.
So I suggest for now just to make sure that secondary process wouldn't touch
that shared security_ctx in any way.
The easiest thing would probably be to move it from shared to private part of ethdev:
i.e. move void *security_ctx; from struct rte_eth_dev_data to struct rte_eth_dev
(you'll probably have to do it anyway later, because of #1)
and make sure it is initialized only for primary process.
Konstantin

> SO I believe this check is not required anymore. Do you see any issue in MP.

> 

> -Akhil
  

> -----Original Message-----

> From: Ananyev, Konstantin

> Sent: Monday, October 23, 2017 10:57 AM

> To: Akhil Goyal <akhil.goyal@nxp.com>; dev@dpdk.org

> Cc: Doherty, Declan <declan.doherty@intel.com>; De Lara Guarch, Pablo

> <pablo.de.lara.guarch@intel.com>; hemant.agrawal@nxp.com; Nicolau,

> Radu <radu.nicolau@intel.com>; borisp@mellanox.com;

> aviadye@mellanox.com; thomas@monjalon.net; sandeep.malik@nxp.com;

> jerin.jacob@caviumnetworks.com; Mcnamara, John

> <john.mcnamara@intel.com>; shahafs@mellanox.com;

> olivier.matz@6wind.com

> Subject: RE: [PATCH v4 06/12] ethdev: support security APIs

> 

> 

> Hi Akhil,

> 

> >

> > Hi Konstantin,

> >

> > On 10/19/2017 2:53 PM, Ananyev, Konstantin wrote:

> > > Hi guys,

> > >

> > >> -----Original Message-----

> > >> From: Akhil Goyal [mailto:akhil.goyal@nxp.com]

> > >> Sent: Saturday, October 14, 2017 11:17 PM

> > >> To: dev@dpdk.org

> > >> Cc: Doherty, Declan <declan.doherty@intel.com>; De Lara Guarch,

> > >> Pablo <pablo.de.lara.guarch@intel.com>;

> > hemant.agrawal@nxp.com;

> > >> Nicolau, Radu <radu.nicolau@intel.com>; borisp@mellanox.com;

> > >> aviadye@mellanox.com; thomas@monjalon.net;

> sandeep.malik@nxp.com;

> > >> jerin.jacob@caviumnetworks.com; Mcnamara, John

> > >> <john.mcnamara@intel.com>; Ananyev, Konstantin

> > >> <konstantin.ananyev@intel.com>; shahafs@mellanox.com;

> > >> olivier.matz@6wind.com

> > >> Subject: [PATCH v4 06/12] ethdev: support security APIs

> > >>

> > >> 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           | 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)

> > >

> > >

> > > As you don't currently support MP, it is probably worth to add

> > > somewhere (here or at PMD layer) check for process type.

> > > Something like:

> > > if (rte_eal_process_type() != RTE_PROC_PRIMARY)

> > >         return NULL;

> > > or so.

> > > Konstantin

> > >

> > >

> > The MP issue is resolved as per my understanding in the v4.

> 

> As I can see from v4 - MP is still not supported:

> 

> 1. security_ctx is placed into rte_eth_dev_data (which is shared between

> multiple processes) while it still contains a pointer to particular ops functions.

> To support MP you'll probably need to split security_ctx into 2 parts:

> private to process (ops) and shared between processes (actual data), or

> comeup with some other (smarter) way.

> 2. At least ixgbe_dev_init() right now always blindly allocates new

>     security_ctx and overwrites  eth_dev->data->security_ctx with this new

> value.

> 

> I do remember that for you didn't plan to support MP for 17.11 anyway.

> So I suggest for now just to make sure that secondary process wouldn't

> touch that shared security_ctx in any way.

> The easiest thing would probably be to move it from shared to private part of

> ethdev:

> i.e. move void *security_ctx; from struct rte_eth_dev_data to struct

> rte_eth_dev (you'll probably have to do it anyway later, because of #1) and

> make sure it is initialized only for primary process.

> Konstantin

> 

> > SO I believe this check is not required anymore. Do you see any issue in

> MP.

> >

> > -Akhil


I moved the security_ctx ftom dev->data to dev as Konstantin suggested, and only initialize it for the primary process. This means that the secondary process will not be supported at the moment, but we will follow up for the next release.

Regards,
Radu