Re: [PATCH v2 01/12] lib/rte_security: add security library

[Akhil Goyal <akhil.goyal@nxp.com>]

Hi Konstantin,

On 10/9/2017 7:12 PM, Ananyev, Konstantin wrote:
> Hi Akhil,
> 
>> -----Original Message-----
>> From: Akhil Goyal [mailto:akhil.goyal@nxp.com]
>> Sent: Friday, October 6, 2017 7:11 PM
>> To: Ananyev, Konstantin <konstantin.ananyev@intel.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>; olivier.matz@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@nxp.com>
>>>> Signed-off-by: Boris Pismenny <borisp@mellanox.com>
>>>> Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
>>>> Signed-off-by: Declan Doherty <declan.doherty@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.

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


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