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

["Nicolau, Radu" <radu.nicolau@intel.com>]

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