[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(ð_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