Hi Fiona Thanks for feedback. PSB.
>-----Original Message----- >From: Trahe, Fiona [mailto:fiona.tr...@intel.com] >Sent: 14 March 2018 17:42 >To: Verma, Shally <shally.ve...@cavium.com>; dev@dpdk.org; Athreya, Narayana >Prasad <narayanaprasad.athr...@cavium.com>; >Murthy, Nidadavolu <nidadavolu.mur...@cavium.com>; Sahu, Sunila ><sunila.s...@cavium.com>; Gupta, Ashish ><ashish.gu...@cavium.com>; Doherty, Declan <declan.dohe...@intel.com>; >Keating, Brian A <brian.a.keat...@intel.com>; Griffin, >John <john.grif...@intel.com>; Tadepalli, Hari K <hari.k.tadepa...@intel.com> >Cc: De Lara Guarch, Pablo <pablo.de.lara.gua...@intel.com>; Trahe, Fiona ><fiona.tr...@intel.com> >Subject: RE: [dpdk-dev] [RFC v1 1/1] lib/cryptodev: add support of asymmetric >crypto > //snip >> >> +enum rte_crypto_asym_op_type { >> >> + RTE_CRYPTO_ASYM_OP_NOT_SPECIFIED = 1, >> >[Fiona] Why does this start at 1? >> >And is it necessary? >> > >> [Shally] We need to indicate list of supported op in xform capability >> structure. Because an implementation >> may support RSA encrypt and decrypt but not RSA Sign and verify. Or, Can >> support DSA Sign compute but >> not verify. >> So, it was added to indicate end-of-array marker (though doesn’t need to be >> 1 for that reason). but now >> when I think to re-design its support, then it won't be needed. >> So, I thought rather than carrying op_type array, I can add an op_type >> bitmask in xform capability to show >> supported ops. >[Fiona] I think a bitmask is ok. Would only need an array if there were other >params whose capabilities would > vary depending on the op_type. E.g. like range of digest_size in sym depends > on the algo. But does code below >not need a xform_type input? Or is capa the capability for one specific >xform_type? [Shally] Yes. capability here is of one xform. I will change variable name to rte_crypto_asym_xform_capabilities to be more clear here. > >> Example capability check code then would look like: >> int rte_crypto_asym_check_op_type ( const rte_crypto_asym_capabilties *capa, >> int op_type) { >> If(capa->op_types & (1 << op_type)) >> return 0; >> return -1; >> } //snip >> >> + /**< Signature verification operation */ >> >> + RTE_CRYPTO_ASYM_OP_KEY_PAIR_GENERATION, >> >> + /**< Public/Private key pair generation operation */ >> >[Fiona] In the comment, clarify that this is for DH and ECDH, and for the >> > generation of the public key (and optionally the private key?) >> > >> >> [Shally] so far, I was assuming it will generate both but when you say >> private key optional, where you >> expect it to be coming from? - from app or generated internally? Is their hw >> variant which may not >> generate private key? >> >[Fiona] In our native driver the private key, which is usually just a random >number conforming to >0 < private_key < (primeP - 1), is passed in by the application and only the >public key is generated. >Some hw accelerators may have RNG capabilities, others may not or the >application may prefer to generate >its own RN. > [Shally] Ok. I will work around to add this support. //snip >> >> [Shally] I would take this question in-parts: >> >> " Also do we want to list all "published" curves, or allow customers to >> specify their own curve parameters," >> - Currently specification allow app to do both i.e. it can either setup >> these published curve ids or specify its >> own domain params to all elliptic curve based xforms (ecdh, ecdsa, and fecc). >> If input curve has curve_type = UNSPECIFIED, PMD uses domain parameters >> else it uses curveid given by >> app from this published list. >> So, is this missing any requirement that need to be supported?! >[Fiona] So you mean the params in ECDH xform (a,b,G,n,h) are only specified if >curve_type = UNSPECIFIED, >else not needed? And in FECC the params (order,G,a,b,h) ? >Ecdsa xform not yet specified, but it will have a similar set? >Then I would suggest creating a struct to hold this param set and using same >struct in all 3 xforms. > [Shally] Ok. Sounds better. See new proposed struct below. //snip >> "do we have to publish curves < 224 bits" >> - We just listed all based on previous feedback to include non-nist curve >> id. But agree it can be narrowed >> down to few (may be to one used by ssl) >[Fiona] I'm looking for feedback internally on this - will get back to you >later. >But I think it could be trimmed to at least removing those curves < 224 bits. >The xform params can be used to cover unlikely curves trimmed from the list. > [Shally] I look forward to it. //snip >> >> + >> >> +/** >> >> + * Elliptic curve id >> >> + */ >> >> +struct rte_crypto_ec_curve_id { >> >> + RTE_STD_C11 >> >> + union { >> >> + enum rte_crypto_ec_prime_curve pcurve; >> >> + enum rte_crypto_ec_binary_curve bcurve; >> >> + }; >> >> +}; >> >[Fiona] Because the values of these two enums overlap, if you specify the >> >curve type incorrectly, you'll still >> >have a potentially valid curve enum specified. I suggest it's safer to >> >include the type here with the union, >> >rather than in the wrapper xform struct, i.e. >> >struct rte_crypto_ec_curve { >> > enum rte_crypto_ec_curve_type curve_type; >> > /**< curve type: Prime vs Binary */ >> > union { >> > enum rte_crypto_ec_prime_curve pcurve_id; >> > enum rte_crypto_ec_binary_curve bcurve_id; >> > }; >> >}; >> [Shally] We would need curve type if we need to define a new curve based on >> domain params. Let's cover >> it later once we clarify above feedback. >[Fiona] So how about adding the struct with curve params I mentioned above as >a 3rd element in the union? > [Shally] Yes agreed. So new structs looks like: struct rte_crypto_ec_domain_params { g, a , b, n ,h }; struct rte_crypto_ec_curve { enum rte_crypto_ec_curve_type curve_type; /**< curve type: Prime vs Binary */ union { enum rte_crypto_ec_prime_curve pcurve_id; enum rte_crypto_ec_binary_curve bcurve_id; struct rte_crypto_ec_domain_params dparams; }; }; //snip >> >> cryptodev_sym_configure_session_t session_configure; >> >> /**< Configure a Crypto session. */ >> >[Fiona] This should really be renamed sym_session_configure, same for >> >session_clear, >> >qp_attach_session and qp_detach_session >> > >> >> [Shally] I thought to keep it backward compatible for now. I think we can >> take this change later as it need >> announcement first >[Fiona] As these are on the internal API between PMDs and API layer, rather >than the application<->cryptodev >interface I think they don't need an announcement. >But would need to be done in a standalone patch, with all PMDs changed > [Shally] ok. So, I will cover elliptic curve and session_configure change in separate patches and send 1st patch with features covered and agreed. //snip Thanks Shally
