Re: [dpdk-dev] [RFC v1 1/1] lib/cryptodev: add support of asymmetric crypto

["Trahe, Fiona" <fiona.trahe@intel.com>]

Hi Shally,

> -----Original Message-----
> From: Verma, Shally [mailto:Shally.Verma@cavium.com]
> Sent: Thursday, February 15, 2018 5:33 AM
> To: Trahe, Fiona <fiona.trahe@intel.com>; dev@dpdk.org; Athreya, Narayana Prasad
> <NarayanaPrasad.Athreya@cavium.com>; Murthy, Nidadavolu <Nidadavolu.Murthy@cavium.com>;
> Sahu, Sunila <Sunila.Sahu@cavium.com>; Gupta, Ashish <Ashish.Gupta@cavium.com>; Doherty, Declan
> <declan.doherty@intel.com>; Keating, Brian A <brian.a.keating@intel.com>; Griffin, John
> <john.griffin@intel.com>
> Cc: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>
> Subject: RE: [dpdk-dev] [RFC v1 1/1] lib/cryptodev: add support of asymmetric crypto
> 
> HI Fiona
> 
> Thanks for your feedback. Response below.
> 
> >-----Original Message-----
> >From: Trahe, Fiona [mailto:fiona.trahe@intel.com]
> >Sent: 09 February 2018 23:43
> >To: dev@dpdk.org; Athreya, Narayana Prasad <NarayanaPrasad.Athreya@cavium.com>; Murthy,
> Nidadavolu
> ><Nidadavolu.Murthy@cavium.com>; Sahu, Sunila <Sunila.Sahu@cavium.com>; Gupta, Ashish
> <Ashish.Gupta@cavium.com>; Verma,
> >Shally <Shally.Verma@cavium.com>; Doherty, Declan <declan.doherty@intel.com>; Keating, Brian A
> <brian.a.keating@intel.com>;
> >Griffin, John <john.griffin@intel.com>
> >Cc: Trahe, Fiona <fiona.trahe@intel.com>; De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>
> >Subject: RE: [dpdk-dev] [RFC v1 1/1] lib/cryptodev: add support of asymmetric crypto
> >
> >Hi Shally,
> >Comments below.
> >
> >> -----Original Message-----
> >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Shally Verma
> >> Sent: Tuesday, January 23, 2018 9:54 AM
> >> To: Doherty, Declan <declan.doherty@intel.com>
> >> Cc: dev@dpdk.org; pathreya@caviumnetworks.com; nmurthy@caviumnetworks.com;
> >> ssahu@caviumnetworks.com; agupta@caviumnetworks.com; Shally Verma
> >> <sverma@caviumnetworks.com>
> >> Subject: [dpdk-dev] [RFC v1 1/1] lib/cryptodev: add support of asymmetric crypto
> >>
> >> From: Shally Verma <sverma@caviumnetworks.com>
> >>
> >> Add support for asymmetric crypto operations in DPDK lib cryptodev
> >>
> >> Key feature include:
> >> - Only session based asymmetric crypto operations
> >> - new get and set APIs for symmetric and asymmetric session private
> >>   data and other informations
> >> - APIs to create, configure and attch queue pair to asymmetric sessions
> >> - new capabilities in struct device_info to indicate
> >>   -- number of dedicated queue pairs available for symmetric and
> >>      asymmetric operations, if any
> >>   -- number of asymmetric sessions possible per qp
> >>
> >[Fiona] Though it's probably premature to include on the API have you considered
> >providing for pre-loaded keys in future, i.e. the ability to wrap keys or refer to keys
> >already stored securely on the device, as an alternative to passing the keys on the API?
> >
> [Shally] Current intention of DPDK asym spec is to expose HW capabilities to application to offload these
> compute intensive ops.
> Though your use-case is very much a practical requirement , but to achieve this I believe we need a layer
> above this spec and need different interfaces. Such as, some public/secure key library which provide
> interfaces to generate / store/ perform op using an opaque key handles which internally can use DPDK
> to do part of it. For such case, I envision dpdk (or library above) will likely run on some secure processor.
> Thus, currently I kept such use-case out of current spec scope.
[Fiona] ok

> >
> >> Proposed asymmetric cryptographic operations are:
> >> - rsa
> >> - dsa
> >> - deffie-hellman key pair generation and shared key computation
> >> - ecdeffie-hellman
> >> - fundamental elliptic curve operations
> >> - elliptic curve DSA
> >> - modular exponentiation and inversion
> >>
> >> This patch primarily defines PMD operations and device capabilities
> >> to perform asymmetric crypto ops on queue pairs and intend to
> >> invite feedbacks on current proposal so as to ensure it encompass
> >> all kind of crypto devices with different capabilities and queue
> >> pair management.
> >>
> >> List of TBDs:
> >> - Currently, patch only updated for RSA xform and associated params.
> >>   Other algoritms to be added in subsequent versions.
> >> - per-service stats update
> >>
> >> Signed-off-by: Shally Verma <sverma@caviumnetworks.com>
> >> ---
> >>
> >> It is derivative of RFC v2 asymmetric crypto patch series initiated by
> >> Umesh Kartha(mailto:umesh.kartha@caviumnetworks.com):
> >>
> >>  http://dpdk.org/dev/patchwork/patch/24245/
> >>  http://dpdk.org/dev/patchwork/patch/24246/
> >>  http://dpdk.org/dev/patchwork/patch/24247/
> >>
> >> And inclusive of all review comments given on RFC v2.
> >>  ( See complete discussion thread here:
> >> http://dev.dpdk.narkive.com/yqTFFLHw/dpdk-dev-rfc-specifications-for-asymmetric-crypto-
> >> algorithms#post12)
> >>
> >> Some of the RFCv2 Review comments pending for closure:
> >> > " [Fiona] The count fn isn't used at all for sym - probably no need to add for asym
> >>      better instead to remove the sym fn."
> >>
> >>      It is still present in dpdk-next-crypto for sym, so what has been decision
> >>      on it?
> >[Fiona] No change. The rte_cryptodev_ops fn is still not called so useless and should be removed.
> >rte_cryptodev_queue_pair_count() returns the num_qps configured in
> >rte_cryptodev_configure(), but never calls the PMD dev_ops.queue_pair_count().
> >So cryptodev_sym_queue_pair_count_t should be deprecated.
> >And no point in adding one for asym.
> >
> 
> [Shally] Ok
> 
> >>
> >> >"[Fiona] if each qp can handle only a specific service, i.e. a subset off the capabilities
> >>     Indicated by the device capability list, there's a need for a new API to query
> >>     the capability of a qp."
> >>
> >>     Current proposal doesn’t distinguish between device capability and qp capability.
> >>     It rather leave such differences handling internal to PMDs. Thus no capability
> >>     or API added for qp in current version. It is subject to revisit based on review
> >>     feedback on current proposal.
> >[Fiona] This would not work for some devices, comments below.
> >
> >>
> >> - Sessionless Support.
> >>     Current proposal only support Session-based because:
> >>      1. All one-time setup i.e.  algos and associated params, such as, public-private keys
> >>         or modulus length can be done in control path using session-init API
> >>      2. it’s an easier way to dedicate qp to do specific service (using queue_pair_attach())
> >>         which cannot be case in sessionless
> >>      3. Couldn’t find any significant advantage going sessionless way. Also existing most of PMDs are
> >> session-based.
> >>
> >>     It could be added in subsequent versions, if requirement is identified, based on review comment
> >>     on this RFC.
> >[Fiona] Our preference would be for sessionless, as it would need fewer API calls (no
> session_create/session_clear)
> >and e.g. DH and ECDH sessions are likely to be only used for a single op. However this is not a blocker
> for
> >this API, we can POC it later and propose an extension to the API if it gives a performance
> improvement.
> >
> [Shally] You mean you recommend to have session-less support along with session-based?
[Fiona] Yes. But as stated, it's ok to start without it and add later.


> >>
> >> Summary
> >> -------
> >>
> >> This section provides an overview of key feature enabled in current specification.
> >> It comprise of key design challenges as have been identified on RFCv2 and
> >> summary description of new interfaces and definitions added to handle same.
> >>
> >> Description
> >> -----------
> >>
> >> This API set assumes that the max_nb_queue_pairs on a
> >> device can be allocated to any mix of sym or asym. Some devices
> >> may have a fixed max per service. Thus, rte_cryptodev_info
> >> is updated with max_sym_nb_queues and max_asym_nb_queues with rule:
> >>
> >> max_nb_queue_pair = max_nb_sym_qp + max_nb_asym_qp.
> >>
> >> If device has no restrictions on qp to be used per service, such PMDs can leave
> >> max_nb_sym_qp = max_nb_asym_qp = 0. In such case, application can setup any of
> >> the service upto limit defined by max_nb_queue_pair.
> >[Fiona] This seems awkward if a genuine 0  e.g. An appl wants to set up a sym qp, and
> >sees that max_nb_queue_pair=4. Can't trust this - needs to look deeper. Sees
> >max_nb_sym_qp=0. Doesn't know if that means it can set up 4 sym qps or no qps
> >until it also checks max_nb_asym_qp. If=4 then it can't set up any sym qp, if=0 it can set up 4.
> >Would it be better if
> >	max_nb_queue_pair = max(max_nb_sym_qp, max_nb_asym_qp).
> >	max_nb_sym_qp = 0..max_nb_queue_pair
> >	max_nb_asym_qp = 0..max_nb_queue_pair
> >
> 
> [Shally] Ok. I can change definition to replace 0 by max num for no-limit case with slight modification  to
> the condition i.e.
> max_nb_queue_pair >=  max(max_nb_sym_qp, max_nb_asym_qp)
> 
> because device can have distribution of (max_nb_queue_pair, max_nb_sym_qp, max_nb_asym_qp) as
> (4,4,4) or (4,3,1)
> where total number of qp setup by application cannot exceed max_nb_queue_pairs.
> 
> >> Here, max_nb_sym_qp and max_nb_asym_qp, if non-zero, just define limit on qp which are
> >> available for each service and *are not* ids to be used during qp setup and enqueue i.e.
> >> if device supports both symmetric and asymmetric with n qp, then any of them
> >> can be configured for symmetric or asymmetric subject to limit defined by
> >> max_nb_sym_qp and max_nb_asym_qp. For example,
> >> if device has 6 queues and 5 for symmetric and 1 for asymmetric that imply
> >> application can setup only 1 out of any 6 qp for asymmetric and rest for
> >> symmetric.
> >>
> >> Additionally, application can dedicate qp to perform specific service via optional
> >> queue_pair_attach_sym/asym_session() API.
> >[Fiona] This is too late - some devices have qps which need to know at setup which
> >service they'll be used for, as they reserve resources based on the service.
> >e.g. a device reports in info that it can support qps(max 4, max_sym 2, max asym 2)
> >An application just wants to use 1 sym qp and 1 asym, so configures device with
> >qps(max 2, max_sym 1, max asym 1)
> >Now appl calls qp_setup for qp index 0. Wants to start with running asymmetric
> >service and will set up the symmetric later, maybe in a separate thread.
> >Our devices need to know at setup time which service will be used on the qp.
> >So haven't been given enough information to setup the qp.
> >
> >Even if the PMD could set up service-agnostic qps and just map later, you
> >suggest the qp_attach_sym/asym_session() would be optional. How could the appl
> >make the decision whether to call it or not in above case? It needs to know whether
> >the created qp is agnostic or not, so a capability must be reported by the PMD.
> >OR the API should not be optional, must be called for every session.
> >But that's perf impacting and not helpful if a device could support service-agnostic qps.
> >
> >I don't have a solution for service-agnostic qps. For our devices that's not necessary.
> >Is it likely that anyone wants to set up a qp to process both sym and asym ops?
> >Is it sufficient to accommodate qps which can support either
> >service, but one must be picked at setup after which the qp supports only that service?
> >How about the following - :
> > - device reports in info qps(max, max_sym, max asym) where
> >	max = max(max_sym, max_asym).
> >	max_sym = 0..max
> >	max_asym = 0..max
> >	So if a device reports 4,2,2, it has 4 total, 2 can be setup for sym, 2 for asym
> >	Or if it reports 4,4,4, all 4 of them can be setup as sym or all 4 as
> >	asym or a mix, but total setup can't be more than 4.
> > - appl configures qps(max, max_sym, max_asym)
> >	If it specifies 4,1,3 it wants to use 4 in total, 1 for sym, 3 for asym.
> >	If it specifies 1,1,0 it wants to use 1 in total, 1 for sym, 0 for asym.
> >	The numbers it configures must be <= those reported in info.
> > - appl calls qp_setup(dev, qp, service) where service = sym or asym
> >	qp index must be between 0..max-1
> >	The PMD maps that index to an internal qp which can support the requested service.
> >	If qp_setup is called on an index and the max configured qps have already
> >	been setup for that service then the setup fails.
> >In case the appl forgets which qp it set up for which service, I think it would be necessary to add a new
> API
> >	rte_cryptodev_qp_info() which would return what service is configured on the qp
> >
> >If it's necessary to support service-agnostic qps, then one way would be to specify a third service above
> as
> > - sym+asym.
> >And have devices report max_sym_asym_qps?
> >
> >It's difficult to find a solution for this without breaking the current API - see simpler
> >alternative below.
> >
> >> Except the ones mentioned above, specification doesn't define any restrictions on
> >> enqueue/dequeue API for different crypto services. Application can enqueue both symmetric
> >> and asymmetric operations to same device and qp if device info indicate support of both
> >> symmetric and asymmetric and qp is not dedicated to any. However in practice, it could
> >> result in head-of-line blocking due to the asym ops taking longer than the symmetric ops.
> >>
> >> Some HW accelerators avoid this issue by supporting sym and asym ops
> >> on different qps on the same device.  Such devices can set max_nb_sym_qp and
> >> max_nb_asym_qp to number of queues available for each respective service.
> >>
> >> This may give problem in indexing on enqueue/dequeue i.e.
> >> if 2 asym qps and 2 sym qps are created on the same device,
> >> the queue_pair_ids are duplicated. To handle such scenario,
> >> 2 alternatives are proposed for PMD design:
> >>
> >> 1. Allow all qp to input any operation type virtually. Say,
> >>
> >>     Consider device with symmetric and asymmetric capability having
> >>     max_nb_queue_pair as 6, then by design, all 6 are capable to input
> >>     both op types.
> >>
> >>     If PMD uses fixed set of qp, then internally, can map qp id to
> >>     actual physical qp id to be used according to session/op type.
> >>     If such mapping is done on data path, may impact performance of PMDs.
> >>     Thus, to handle such mapping out of data path, library provides an API
> >>     queue_pair_attach_sym/asym_session() which facilitate application to
> >>     dedicate qp id to perform specific (sym/asym) service in control path
> >>     and should be called before 1st call to enqueue_burst().
> >>     Here PMD can bind app provided logical qp id to actual physical qp id to use
> >>     depending on session type , OR alternatively
> >>
> >> 2. PMDs can split it into two PMDs, one with symmetric ONLY capability and other
> >>     with asymmetric ONLY capability (also proposed by Fiona in RFCv2).
> >>     This may require some  changes to EAL to facilitate pci device sharing between
> >>     PMDs, enabling separation of sym and  asym services onto different PMDs.
> >>     As per RFCv2 feedback, Intel  proposed to submit a patch to  enable this.
> >[Fiona] The eal changes were problematic, and feedback from the Dublin userspace
> >event last September was that several others have tried similar but reverted to resolving
> >the problem within the PMD. This is what we are also in the process of doing.
> >With this we intend to avoid all the above complications.
> >i.e. one pci device, in its probe fn, can create multiple API device instances.
> >One instance of cryptodev will provide a sym crypto service, one instance of
> >compressdev will provide a compression service and a second cryptodev instance will
> >provide the asymmetric service. All qps on each API device instance can then support all
> >of the capabilities reported by that device.
> >
> >A suggestion for keeping the simplicity on the API would be to follow the same pattern.
> >i.e. don't provide any support for specification of sym or asym qps. Expect all
> >qps on each device to support all capabilities reported.
> >
> >
> 
> [Shally] Initially I had thought of same proposal to add an input field in qp_conf so that PMD can know
> at qp_setup time which service it going to be used for.
> However looking that spec already had qp_attach_sym/asym APIs and also the possibility that some
> device can support service - agnostic qp, I initiated with current proposal.
> I marked them optional with a disclaimer that , if not attached, then PMD will then need to map logical
> qpid to physical during enqueue_burst() which might result in impacted performance.
> This is particularly applicable to devices which internally have fixed qps per each service and set
> max_nb_sym/asym_qp to indicate same.
> And thus for such devices, I primarily recommended that they should appear as two separate PMDs
> where one support sym and other asym.
> However, only point to is to see how well suited is this for each device driver design as it might rather
> turn to be simpler for some implementation to just have single way to dedicate a qp to a service via
> attach or qp_setup change than having two separate instances.
> 
> But, I concur from your thoughts. We can start with simple definition and remove concept of
> max_nb_sym_qp and max_nb_asym_qp in first version and say
> -if device support service agnostic qp, that means all of its qp can be set up for any service. PMD can
> show in its feature flag support for symmetric and asymmetric crypto. Application can set up qp the way
> it want, spec define no recommended way of usage here.
[Fiona] I would word this differently, as no mechanism is provided for an appl to setup a qp for a specific service.
If PMD shows in its feature flag that it supports both sym and asym then it must support those on all its qps.
Application may choose to direct all asym ops to one qp and all asym to a different qp to avoid
head-of-line blocking, but there is nothing in the qp setup or capabilities to prevent the application
from sending an enqueue_burst() with a mix of asym and sym ops all to the same qp and it should work.

> -if device has fixed set of qp per each service, then it should split itself into two : symmetric only and
> asymmetric only.
[Fiona] For us this is ok, but it would be good to get feedback from other providers of hw acceleration
as it is quite a constraint.

> This is well suited for session-less also.  IMO, If any issues reported with these rules, then we can add
> this support later based on the then identified requirement. However, if we end up adding the support
> back of max_nb_sym/asym_qp in spec later, then attach/detach_qp APIs will not be sufficient as they
> are session specific. Given that, having an input param during qp_setup() is more generic way that suit
> both session and sessionless way. Does this all sounds right?
[Fiona] agreed.

> 
> I will wait to close on this design discussion first before giving feedback to rest of the comment further
> below. But consider all feedback regarding typo and licensing as acknowledged.
> 
> Thanks
> Shally
>