Hi Akhil, > -----Original Message----- > From: Akhil Goyal [mailto:akhil.go...@nxp.com] > Sent: Tuesday, October 10, 2017 1:17 PM > To: Ananyev, Konstantin <konstantin.anan...@intel.com>; dev@dpdk.org > Cc: Doherty, Declan <declan.dohe...@intel.com>; De Lara Guarch, Pablo > <pablo.de.lara.gua...@intel.com>; hemant.agra...@nxp.com; > Nicolau, Radu <radu.nico...@intel.com>; bor...@mellanox.com; > avia...@mellanox.com; tho...@monjalon.net; > sandeep.ma...@nxp.com; jerin.ja...@caviumnetworks.com; Mcnamara, John > <john.mcnam...@intel.com>; olivier.m...@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.go...@nxp.com] > >> Sent: Friday, October 6, 2017 7:11 PM > >> To: Ananyev, Konstantin <konstantin.anan...@intel.com>; dev@dpdk.org > >> Cc: Doherty, Declan <declan.dohe...@intel.com>; De Lara Guarch, Pablo > >> <pablo.de.lara.gua...@intel.com>; > hemant.agra...@nxp.com; > >> Nicolau, Radu <radu.nico...@intel.com>; bor...@mellanox.com; > >> avia...@mellanox.com; tho...@monjalon.net; > >> sandeep.ma...@nxp.com; jerin.ja...@caviumnetworks.com; Mcnamara, John > >> <john.mcnam...@intel.com>; olivier.m...@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.go...@nxp.com> > >>>> Signed-off-by: Boris Pismenny <bor...@mellanox.com> > >>>> Signed-off-by: Radu Nicolau <radu.nico...@intel.com> > >>>> Signed-off-by: Declan Doherty <declan.dohe...@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(ð_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
