On 06/07/2018 3:28 PM, Verma, Shally wrote:
Hi Declan
-----Original Message-----
From: Doherty, Declan [mailto:declan.dohe...@intel.com]
Sent: 05 July 2018 20:24
To: Verma, Shally <shally.ve...@cavium.com>; pablo.de.lara.gua...@intel.com
Cc: 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>; Kartha,
Umesh <umesh.kar...@cavium.com>
Subject: Re: [dpdk-dev] [PATCH v4 1/4] lib/cryptodev: add asymmetric algos in
cryptodev
External Email
Hey Shally,
just a few things inline below mainly concerned with the need to be able
to support session-less operations in future PMDs. I think with a few
minor changes to the API now it should allow session-less to be
supported later without the need for a major rework of the APIs, I don't
think this should cause any major rework to your PMD just the adoption
of some new more explicit op types.
Thanks
Declan
On 03/07/2018 4:24 PM, Shally Verma wrote:
Add rte_crypto_asym.h with supported xfrms
and associated op structures and APIs
API currently supports:
- RSA Encrypt, Decrypt, Sign and Verify
- Modular Exponentiation and Inversion
- DSA Sign and Verify
- Deffie-hellman private key exchange
- Deffie-hellman public key exchange
- Deffie-hellman shared secret compute
- Deffie-hellman public/private key pair generation
using xform chain
Signed-off-by: Shally Verma <shally.ve...@caviumnetworks.com>
Signed-off-by: Sunila Sahu <sunila.s...@caviumnetworks.com>
Signed-off-by: Ashish Gupta <ashish.gu...@caviumnetworks.com>
Signed-off-by: Umesh Kartha <umesh.kar...@caviumnetworks.com>
---
lib/librte_cryptodev/Makefile | 1 +
lib/librte_cryptodev/meson.build | 3 +-
lib/librte_cryptodev/rte_crypto_asym.h | 496
+++++++++++++++++++++++++++++++++
3 files changed, 499 insertions(+), 1 deletion(-)
diff --git a/lib/librte_cryptodev/Makefile b/lib/librte_cr
...
+typedef struct rte_crypto_param_t {
+ uint8_t *data;
+ /**< pointer to buffer holding data */
+ rte_iova_t iova;
+ /**< IO address of data buffer */
+ size_t length;
+ /**< length of data in bytes */
+} rte_crypto_param;
What is the intended way for this memory to be allocated,
[Shally] It should be pointer to flat buffers and added only to input/output
data to/from
asymmetric crypto engine.
it seems like
there might be a more general requirement in DPDK for IO addressable
memory (compression? other hardware acceleators implemented on FPGAs)
than just asymmetric crypto, will we end up needing to support features
like scatter gather lists in this structure?
[Shally] I don’t anticipate that we would need to support scatter-gather data
buffers as far as it is used for asymmetric.
And I'm not aware if we have requirement to support it for asymmetric
processing since data size is usually small for
such operations. Thus, app is expected to send linear buffers for input/output.
Does that answer your question? Or did I miss anything?
Sure I understand the rationale.
btw I think this is
probably fine for the moment as it will be expermential but I think it
will need to be addressed before the removal of the expermential tag.
...
+ RTE_CRYPTO_ASYM_XFORM_MODINV,
Would prefer if this was _MOD_INV :)
+ /**< Modular Inverse
+ * Perform Modulus inverse b^(-1) mod n
+ */
+ RTE_CRYPTO_ASYM_XFORM_MODEX,
any this was _MOD_EX :)
[Shally] fine will do name change.
+ /**< Modular Exponentiation
+ * Perform Modular Exponentiation b^e mod n
+ */
+ RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END
+ /**< End of list */
+};
+
+/**
+ * Asymmetric crypto operation type variants
+ */
+enum rte_crypto_asym_op_type {
+ RTE_CRYPTO_ASYM_OP_ENCRYPT,
+ /**< Asymmetric Encrypt operation */
+ RTE_CRYPTO_ASYM_OP_DECRYPT,
+ /**< Asymmetric Decrypt operation */
+ RTE_CRYPTO_ASYM_OP_SIGN,
+ /**< Signature Generation operation */
+ RTE_CRYPTO_ASYM_OP_VERIFY,
+ /**< Signature Verification operation */
+ RTE_CRYPTO_ASYM_OP_PRIVATE_KEY_GENERATE,
+ /**< DH Private Key generation operation */
+ RTE_CRYPTO_ASYM_OP_PUBLIC_KEY_GENERATE,
+ /**< DH Public Key generation operation */
+ RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE,
+ /**< DH Shared Secret compute operation */
+ RTE_CRYPTO_ASYM_OP_LIST_END
+};
+
I think that having generic operation types which may or may not apply
to all of the defined xforms is confusing from a user perspective and in
the longer term will make it impossible to support session-less
asymmetric operations. If we instead do something like
RTE_CRYPTO_ASYM_OP_RSA_ENCRYPT,
RTE_CRYPTO_ASYM_OP_RSA_DECRYPT,
RTE_CRYPTO_ASYM_OP_RSA_SIGN,
RTE_CRYPTO_ASYM_OP_RSA_VERIFY,
RTE_CRYPTO_ASYM_OP_DH_KEY_GENERATE,
RTE_CRYPTO_ASYM_OP_DH_SHARED_SECRET_COMPUTE,
etc...
Then the op type becomes very explicit and will allow session-less
operations to be supported by PMDs. This shouldn't have any impact on
your current implementation other than updating the op type.
[Shally] Ok, so you suggest to merge xform and op_type (including keeping
op_type in one place) for simplicity sake.
If we take this change, then I will prefer to keep it as xforms rather than
op_types, such as,
enum rte_crypto_asym_xform_types {
RTE_CRYPTO_ASYM_XFORM_RSA_ENCRYPT,
RTE_CRYPTO_ASYM_XFORM_RSA_SIGN,
RTE_CRYPTO_ASYM_XFORM_DSA_SIGN,
RTE_CRYPTO_ASYM_XFORM_DSA_VERIFY,
RTE_CRYPTO_ASYM_XFORM_DH_PRIV_KEY_GENERATE,
RTE_CRYPTO_ASYM_XFORM_DH_PUB_KEY_GENERATE,
RTE_CRYPTO_ASYM_XFORM_DH_SHARED_SECRET_COMPUTE, .... and so on..
}
These, when set on a rte_crypto_asym_xform , will work seamlessly for both
session and session-less.
I also see advantage here to support xform chaining. In this case, op_type in
rte_crypto_asym_xx_op_params isn't needed and will be removed.
It will add bit of repetition though, like RSA_SIGN / DSA_SIGN, but that would
occur in any case, if we are merging two types.
Does that sound okay?
I would be a bit concerned with dropping the
rte_crypto_asym_xx_op_params as if we are following the symmetric model,
the xform should only specify the immutable data in relation to a
transform and the operation should provide the mutable input data. I'm
not sure how this would look like for the different asymmetric
operations but if there are transforms that don't have immutable data
then it would make more sense not to have a specific xform data
structure for that and pass the data through that transformations
operation structure.
We were about to submit Openssl PMD with asym support today. But I would hold
back till we align on this.
...
Ok, I think I had missed part of the complexity of the chained used
case. I think if we are to follow the symmetric crypto model we end up
with something along the lines of a fundamental op types - dh, dsa, mod,
rsa etc.
enum rte_crypto_asym_op_types {
RTE_CRYPTO_ASYM_OP_DH,
RTE_CRYPTO_ASYM_OP_DSA,
RTE_CRYPTO_ASYM_OP_MOD,
RTE_CRYPTO_ASYM_OP_RSA,
};
with a corresponding asym_op as follows.
struct rte_crypto_asym_op {
union {
struct rte_cryptodev_asym_session *session;
/**< Handle for the initialized session context */
struct rte_crypto_asym_xform *xform;
};
union {
struct rte_crypto_asym_dh_op_data dh;
struct rte_crypto_asym_dsa_op_data dsa;
struct rte_crypto_asyn_mod_op_data mod;
struct rte_crypto_asym_rsa_op_data rsa;
} op_data;
};
In terms of the xform definitions I'm not sure that we should have all
the different transforms defined in the same enum. If we take the below
approach, all the specifics of each transform could be split out into
separate headers, giving a cleaner API. This would see a xform types
enum which would mirror the operation types:
enum rte_crypto_asym_xform_types {
RTE_CRYPTO_ASYM_XFORM_DH,
RTE_CRYPTO_ASYM_XFORM_DSA,
RTE_CRYPTO_ASYM_XFORM_MOD,
RTE_CRYPTO_ASYM_XFORM_RSA,
}
with corresponding xforms structures
struct rte_crypto_asym_xform dh;
struct rte_crypto_asym_xform dsa;
struct rte_crypto_asym_xform modlus;
struct rte_crypto_asym_xform rsa;
then within the xforms would define the sub-type of that xform and have
definitions of the relevant session data which is required, with the
rsa it might look like something like this:
enum rte_crypto_asym_xform_rsa_types {
RTE_CRYPTO_ASYM_XFORM_RSA_ENCRYPT,
RTE_CRYPTO_ASYM_XFORM_RSA_DECRYPT,
RTE_CRYPTO_ASYM_XFORM_RSA_SIGN,
RTE_CRYPTO_ASYM_XFORM_RSA_VERIFY,
}
struct rte_crypto_asym_xform rsa {
enum rte_crypto_asym_op_rsa_types type;
union {
struct rte_crypto_asym_xform_rsa_encrypt_data encrypt;
struct rte_crypto_asym_xform_rsa_decrypt_data decrypt;
etc...
} data;
};
The need for xform sub-type data structure would be dependent on whether
there was immutable data associated with a particular transform. Also if
we take this approach it would allow each operation type to be separated
out into there own headers.
....
+ */
+struct rte_crypto_dh_xform {
+ enum rte_crypto_asym_op_type type;
+ /**< Setup xform for key generate or shared secret compute */
+
there is an inconsistency here in terms of were the op_type is defined,
in this case it is in the xform but it other cases RSA, DSA it is
defined in the operation information itself. I don't know of any reason
why it is needed in the xform but I think it must be consistent across
all operations/xforms. Ideally from my perspective it would be in the
rte_crypto_asym_op structure, see below, as this would allow
session/session-less operations to be supported seamlessly.
[Shally] Reason was xform chaining. As per Fiona feedback, we identified
requirement to break DH Key pair
generation in to two : PRIV_KEY_GENERATION and PUB_KEY_GENERATION, so I had to
move this param around.
But this will be addressed once we change xform_types as per suggestion above.
...
...
+/**
+ * Asymmetric Cryptographic Operation.
+ *
+ * Structure describing asymmetric crypto operation params.
+ *
+ */
+struct rte_crypto_asym_op {
+ struct rte_cryptodev_asym_session *session;
+ /**< Handle for the initialised session context */
+
+ __extension__
+ union {
+ struct rte_crypto_rsa_op_param rsa;
+ struct rte_crypto_mod_op_param modex;
+ struct rte_crypto_mod_op_param modinv;
+ struct rte_crypto_dh_op_param dh;
+ struct rte_crypto_dsa_op_param dsa;
+ };
+} __rte_cache_aligned;
+
Relating to my comment on position of the op_type and the minor change
of having an union of session/xform in the rte_crypto_asym_op structure
would then enable sessionless support to be added seamless in the future
with minimal effect to the current proposal.
[Shally] Again, this will also be resolved with change to xform_types
struct rte_crypto_asym_op {
- struct rte_cryptodev_asym_session *session;
- /**< Handle for the initialised session context */
+ enum rte_crypto_asym_op_type op_type;
+
+ union {
+ struct rte_cryptodev_asym_session *session;
+ /**< Handle for the initialised session context */
+ struct rte_crypto_asym_xform *xform;
+ };
[Shally] Ok. Will add this change. But then I'll have to check if crypto PMD
has support to reflect which type of mode it supports?
The sess_type=RTE_CRYPTO_OP_WITH_SESSION/RTE_CRYPTO_OP_SESSIONLESS
should be set in the outer crypto_op struct, so if the PMD doesn't
support the selected sess_type, it should just fail the enqueue and
possibly log an error.
Thanks for review.
Shally
__extension__
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* _RTE_CRYPTO_ASYM_H_ */
