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

["Verma, Shally" <Shally.Verma@cavium.com>]

HI Fiona

>-----Original Message-----
>From: Trahe, Fiona [mailto:fiona.trahe@intel.com]
>Sent: 15 February 2018 17:16
>To: Verma, Shally <Shally.Verma@cavium.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>; Trahe, Fiona <fiona.trahe@intel.com>
>Subject: RE: [dpdk-dev] [RFC v1 1/1] lib/cryptodev: add support of asymmetric crypto
>
>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.
>

[Shally] Clear.

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

[Shally] That's the intention but until then I assume we agree to current proposal?!

If yes, then just to align and close on our discussion on this here’s the summary :
- There's no differentiation to device capability and qp capability.  If PMD shows in its feature flag that it supports both sym and asym then it must support those on all its qps.
- if PMD support both but internally has hw with dedicated qp for each service then it *can* split itself into two: symmetric only and asymmetric only PMD instances.
- Currently we don’t see a requirement to dedicate qp to a service, thus no need for max_nb_sym/asym_qp or service_type info during qp_setup. However this is open for review and discussion and can be added later, if any requirement is identified for same.

Is that correct?

Thanks
Shally
>> 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
>>