[dpdk-dev] [PATCH v2 01/12] lib/rte_security: add security library

Ananyev, Konstantin konstantin.ananyev at intel.com
Wed Oct 11 11:02:02 CEST 2017


Hi Akhil,

> -----Original Message-----
> From: Akhil Goyal [mailto:akhil.goyal at nxp.com]
> Sent: Tuesday, October 10, 2017 1:17 PM
> To: Ananyev, Konstantin <konstantin.ananyev at intel.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>; borisp at mellanox.com; aviadye at mellanox.com; thomas at monjalon.net;
> sandeep.malik at nxp.com; jerin.jacob at caviumnetworks.com; Mcnamara, John <john.mcnamara at intel.com>; olivier.matz at 6wind.com
> Subject: Re: [PATCH v2 01/12] lib/rte_security: add security library
> 
> Hi Konstantin,
> 
> On 10/9/2017 7:12 PM, Ananyev, Konstantin wrote:
> > Hi Akhil,
> >
> >> -----Original Message-----
> >> From: Akhil Goyal [mailto:akhil.goyal at nxp.com]
> >> Sent: Friday, October 6, 2017 7:11 PM
> >> To: Ananyev, Konstantin <konstantin.ananyev at intel.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>; borisp at mellanox.com; aviadye at mellanox.com; thomas at monjalon.net;
> >> sandeep.malik at nxp.com; jerin.jacob at caviumnetworks.com; Mcnamara, John <john.mcnamara at intel.com>; olivier.matz at 6wind.com
> >> Subject: Re: [PATCH v2 01/12] lib/rte_security: add security library
> >>
> >> Hi Konstantin,
> >>
> >> Thanks for your comments.
> >> On 10/5/2017 10:00 PM, Ananyev, Konstantin wrote:
> >>> Hi lads,
> >>>
> >>>>
> >>>> rte_security library provides APIs for security session
> >>>> create/free for protocol offload or offloaded crypto
> >>>> operation to ethernet device.
> >>>>
> >>>> Signed-off-by: Akhil Goyal <akhil.goyal at nxp.com>
> >>>> Signed-off-by: Boris Pismenny <borisp 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_security/Makefile                 |  53 +++
> >>>>    lib/librte_security/rte_security.c           | 275 +++++++++++++++
> >>>>    lib/librte_security/rte_security.h           | 495 +++++++++++++++++++++++++++
> >>>>    lib/librte_security/rte_security_driver.h    | 182 ++++++++++
> >>>>    lib/librte_security/rte_security_version.map |  13 +
> >>>>    5 files changed, 1018 insertions(+)
> >>>>    create mode 100644 lib/librte_security/Makefile
> >>>>    create mode 100644 lib/librte_security/rte_security.c
> >>>>    create mode 100644 lib/librte_security/rte_security.h
> >>>>    create mode 100644 lib/librte_security/rte_security_driver.h
> >>>>    create mode 100644 lib/librte_security/rte_security_version.map
> >>>>
> 
> [...]
> 
> >>>> +
> >>>> +struct rte_security_ctx {
> >>>> +	uint16_t id;
> >>>> +	enum {
> >>>> +		RTE_SECURITY_INSTANCE_INVALID,
> >>>> +		RTE_SECURITY_INSTANCE_VALID
> >>>> +	} state;
> >>>> +	void *device;
> >>>> +	struct rte_security_ops *ops;
> >>>> +	uint16_t sess_cnt;
> >>>> +};
> >>>> +
> >>>> +static struct rte_security_ctx *security_instances;
> >>>> +static uint16_t max_nb_security_instances;
> >>>> +static uint16_t nb_security_instances;
> >>>
> >>> Probably a dumb question - but why do you need a global security_instances []
> >>> and why security_instance has to be refrenced by index?
> >>> As I understand, with proposed model all drivers have to do something like:
> >>> rte_security_register(&eth_dev->data->sec_id,  (void *)eth_dev, &ixgbe_security_ops);
> >>> and then all apps would have to:
> >>> rte_eth_dev_get_sec_id(portid)
> >>> to retrieve that security_instance index.
> >>> Why not just treat struct rte_security_ctx* as opaque pointer and make
> >>> all related API get/accept it as a paratemer.
> >>> To retrieve sec_ctx from device just:
> >>> struct rte_security_ctx* rte_ethdev_get_sec_ctx(portid);
> >>> struct rte_security_ctx* rte_cryptodev_get_sec_ctx(portid);
> >>> ?
> >> We would look into this separately.
> >
> > Could you clarify what does it mean?
> > It will be addressed in v4 or don't plan to address it at all or something else?
> We were thinking of improving this beyond the scope of this patchset as
> an incremental patch.
> 
> 
> >
> >
> >>>
> >>> Another question how currently proposed model with global static array and friends,
> >>> supposed to work for DPDK MP model?
> >>> Or MP support is not planned?
> >> multi process case is planned for future enhancement. This is mentioned
> >> in the cover note.
> >
> > Great, then I suppose you should have a generic idea how that model will work
> > for DPDK multi-process model?
> > If so, can you probably share your thoughts, because it is not clear to me.
> > Let say user has an ethdev device with ipsec capability and  wants to use it
> > from both primary and secondary process.
> > What would be a procedure?
> > Can user use the same security instance from both processes?
> > If yes, then
> > - how secondary process will get security_instance_id for that device
> > from primary process?
> >   By calling rte_eth_dev_get_sec_id(), or something else?
> > - how guarantee that all these func pointers inside ops will be mapped to the
> > same address inside  processes?
> > If not, then does it mean each process has to call rte_security_register()
> > for each device?
> > But right now you have only one sec_id  inside rte_eth_dev_data.
> >
> > Would secondary process be allowed to register/unregister/update security instances?
> >   if yes, how the will synchronize?
> > Same question for session ops.
> >
> 
> Currently multi process case is not handled and we have not put our
> thoughts on resolving this as of now. We were planning to improve this
> beyond the scope of this patchset as an incremental patch.
> 
> >>>
> >>>> +
> >>>> +static int
> >>>> +rte_security_is_valid_id(uint16_t id)
> >>>> +{
> >>>> +	if (id >= nb_security_instances ||
> >>>> +	    (security_instances[id].state != RTE_SECURITY_INSTANCE_VALID))
> >>>> +		return 0;
> >>>> +	else
> >>>> +		return 1;
> >>>> +}
> >>>> +
> >>>> +/* Macros to check for valid id */
> >>>> +#define RTE_SEC_VALID_ID_OR_ERR_RET(id, retval) do { \
> >>>> +	if (!rte_security_is_valid_id(id)) { \
> >>>> +		RTE_PMD_DEBUG_TRACE("Invalid sec_id=%d\n", id); \
> >>>> +		return retval; \
> >>>> +	} \
> >>>> +} while (0)
> >>>> +
> >>>> +#define RTE_SEC_VALID_ID_OR_RET(id) do { \
> >>>> +	if (!rte_security_is_valid_id(id)) { \
> >>>> +		RTE_PMD_DEBUG_TRACE("Invalid sec_id=%d\n", id); \
> >>>> +		return; \
> >>>> +	} \
> >>>> +} while (0)
> >>>> +
> >>>> +int
> >>>> +rte_security_register(uint16_t *id, void *device,
> >>>> +		      struct rte_security_ops *ops)
> >>>> +{
> >
> > Probably const  struct rte_security_ops *ops
> >
> >>>> +	if (max_nb_security_instances == 0) {
> >>>> +		security_instances = rte_malloc(
> >>>> +				"rte_security_instances_ops",
> >>>> +				sizeof(*security_instances) *
> >>>> +				RTE_SECURITY_INSTANCES_BLOCK_ALLOC_SZ, 0);
> >>>> +
> >>>> +		if (security_instances == NULL)
> >>>> +			return -ENOMEM;
> >>>> +		max_nb_security_instances =
> >>>> +				RTE_SECURITY_INSTANCES_BLOCK_ALLOC_SZ;
> >>>> +	} else if (nb_security_instances >= max_nb_security_instances) {
> >>>
> >>> You probably need try to reuse unregistered entries first?
> >>> Konstantin
> >>>
> >> These APIs are experimental at this moment as mentioned in the patchset.
> >> We will try accommodate your comments in future.
> >
> > Again could you clarify what do you mean by 'future' here?
> >
> As I said before, these APIs need to be re-looked to incorporate the
> multi process cases and better memory utilization.
> We intend to include most of your comments separately beyond the scope
> of this patchset.
> I believe the complete code should not be put on hold due to these
> issues (multi process and optimizations in register/deregister APIs).
> As per my understanding the code is not impacting any of the existing
> functionalities.

I don't have a problem if rte_security code by itself will be reworked in future releases.
But rte_security is not standalone - changes in public API and major design principles
for rte_security means code changes for rte_ethdev, ixgbe, etc.
Again not the end of the world, but would be better to minimize such code churn if possible.
So if you plan to make changes in public API I talked before:
rework/remove rte_security_register/unregister(),
replace rte_eth_dev_get_sec_id(portid) with struct rte_security_ctx* rte_ethdev_get_sec_ctx(portid) or so.
etc. 
anyway - I think it would be really good to have them in 17.11 timeframe.

> 
> However,we will discuss internally if these can be resolved in v4 or not.
> 

Will wait for v4 then.
Konstantin

> 
> >>>
> >>>> +		uint16_t *instances = rte_realloc(security_instances,
> >>>> +				sizeof(struct rte_security_ops *) *
> >>>> +				(max_nb_security_instances +
> >>>> +				RTE_SECURITY_INSTANCES_BLOCK_ALLOC_SZ), 0);
> >>>> +
> >>>> +		if (instances == NULL)
> >>>> +			return -ENOMEM;
> >>>> +
> >>>> +		max_nb_security_instances +=
> >>>> +				RTE_SECURITY_INSTANCES_BLOCK_ALLOC_SZ;
> >>>> +	}
> >>>> +
> >>>> +	*id = nb_security_instances++;
> >>>> +
> >>>> +	security_instances[*id].id = *id;
> >>>> +	security_instances[*id].state = RTE_SECURITY_INSTANCE_VALID;
> >>>> +	security_instances[*id].device = device;
> >>>> +	security_instances[*id].ops = ops;
> >
> > BTW, I think you need to copy actual contents of ops.
> > Otherwise you assuming that ops are static
> > (would be valid though all processes lifetimes).
> >
> > Konstantin
> >
> 
> 
> Regards,
> Akhil


More information about the dev mailing list