DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Verma, Shally" <Shally.Verma@cavium.com>
To: "Trahe, Fiona" <fiona.trahe@intel.com>,
	"dev@dpdk.org" <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>,
	"Tadepalli, Hari K" <hari.k.tadepalli@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
Date: Fri, 16 Mar 2018 08:02:44 +0000	[thread overview]
Message-ID: <CY4PR0701MB3634DAAA8C5309ABB3FC67EFF0D70@CY4PR0701MB3634.namprd07.prod.outlook.com> (raw)
In-Reply-To: <348A99DA5F5B7549AA880327E580B4358934AC7A@IRSMSX101.ger.corp.intel.com>

Hi Fiona

Thanks for feedback. PSB.

>-----Original Message-----
>From: Trahe, Fiona [mailto:fiona.trahe@intel.com]
>Sent: 14 March 2018 17:42
>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>; Tadepalli, Hari K <hari.k.tadepalli@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
>

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


      reply	other threads:[~2018-03-16  8:02 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-01-23  9:54 [dpdk-dev] [RFC v1 0/1] " Shally Verma
2018-01-23  9:54 ` [dpdk-dev] [RFC v1 1/1] " Shally Verma
2018-02-01 11:36   ` [dpdk-dev] FW: " Verma, Shally
2018-02-02 15:12     ` Jain, Deepak K
2018-02-09 18:13   ` [dpdk-dev] " Trahe, Fiona
2018-02-15  5:32     ` Verma, Shally
2018-02-15 11:46       ` Trahe, Fiona
2018-02-22  5:50         ` Verma, Shally
2018-02-22 10:38           ` Trahe, Fiona
2018-03-07 12:16     ` Verma, Shally
2018-03-14 12:12       ` Trahe, Fiona
2018-03-16  8:02         ` Verma, Shally [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CY4PR0701MB3634DAAA8C5309ABB3FC67EFF0D70@CY4PR0701MB3634.namprd07.prod.outlook.com \
    --to=shally.verma@cavium.com \
    --cc=Ashish.Gupta@cavium.com \
    --cc=NarayanaPrasad.Athreya@cavium.com \
    --cc=Nidadavolu.Murthy@cavium.com \
    --cc=Sunila.Sahu@cavium.com \
    --cc=brian.a.keating@intel.com \
    --cc=declan.doherty@intel.com \
    --cc=dev@dpdk.org \
    --cc=fiona.trahe@intel.com \
    --cc=hari.k.tadepalli@intel.com \
    --cc=john.griffin@intel.com \
    --cc=pablo.de.lara.guarch@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).