[dpdk-dev] [RFC PATCH 1/9] security: introduce CPU Crypto action type and API

Ananyev, Konstantin konstantin.ananyev at intel.com
Wed Sep 25 20:24:17 CEST 2019


> > > > > > > > > This action type allows the burst of symmetric crypto workload using
> > > the
> > > > > > > same
> > > > > > > > > algorithm, key, and direction being processed by CPU cycles
> > > > > synchronously.
> > > > > > > > > This flexible action type does not require external hardware
> > > involvement,
> > > > > > > > > having the crypto workload processed synchronously, and is more
> > > > > > > performant
> > > > > > > > > than Cryptodev SW PMD due to the saved cycles on removed "async
> > > > > mode
> > > > > > > > > simulation" as well as 3 cacheline access of the crypto ops.
> > > > > > > >
> > > > > > > > Does that mean application will not call the cryptodev_enqueue_burst
> > > and
> > > > > > > corresponding dequeue burst.
> > > > > > >
> > > > > > > Yes, instead it just call rte_security_process_cpu_crypto_bulk(...)
> > > > > > >
> > > > > > > > It would be a new API something like process_packets and it will have
> > > the
> > > > > > > crypto processed packets while returning from the API?
> > > > > > >
> > > > > > > Yes, though the plan is that API will operate on raw data buffers, not
> > > mbufs.
> > > > > > >
> > > > > > > >
> > > > > > > > I still do not understand why we cannot do with the conventional
> > > crypto lib
> > > > > > > only.
> > > > > > > > As far as I can understand, you are not doing any protocol processing
> > > or
> > > > > any
> > > > > > > value add
> > > > > > > > To the crypto processing. IMO, you just need a synchronous crypto
> > > > > processing
> > > > > > > API which
> > > > > > > > Can be defined in cryptodev, you don't need to re-create a crypto
> > > session
> > > > > in
> > > > > > > the name of
> > > > > > > > Security session in the driver just to do a synchronous processing.
> > > > > > >
> > > > > > > I suppose your question is why not to have
> > > > > > > rte_crypot_process_cpu_crypto_bulk(...) instead?
> > > > > > > The main reason is that would require disruptive changes in existing
> > > > > cryptodev
> > > > > > > API
> > > > > > > (would cause ABI/API breakage).
> > > > > > > Session for  RTE_SECURITY_ACTION_TYPE_CPU_CRYPTO need some
> > > extra
> > > > > > > information
> > > > > > > that normal crypto_sym_xform doesn't contain
> > > > > > > (cipher offset from the start of the buffer, might be something extra in
> > > > > future).
> > > > > >
> > > > > > Cipher offset will be part of rte_crypto_op.
> > > > >
> > > > > fill/read (+ alloc/free) is one of the main things that slowdown current
> > > crypto-op
> > > > > approach.
> > > > > That's why the general idea - have all data that wouldn't change from packet
> > > to
> > > > > packet
> > > > > included into the session and setup it once at session_init().
> > > >
> > > > I agree that you cannot use crypto-op.
> > > > You can have the new API in crypto.
> > > > As per the current patch, you only need cipher_offset which you can have it as
> > > a parameter until
> > > > You get it approved in the crypto xform. I believe it will be beneficial in case of
> > > other crypto cases as well.
> > > > We can have cipher offset at both places(crypto-op and cipher_xform). It will
> > > give flexibility to the user to
> > > > override it.
> > >
> > > After having another thought on your proposal:
> > > Probably we can introduce new rte_crypto_sym_xform_types for CPU related
> > > stuff here?
> >
> > I also thought of adding new xforms, but that wont serve the purpose for may be all the cases.
> > You would be needing all information currently available in the current xforms.
> > So if you are adding new fields in the new xform, the size will be more than that of the union of xforms.
> > ABI breakage would still be there.
> >
> > If you think a valid compression of the AEAD xform can be done, then that can be done for each of the
> > Xforms and we can have a solution to this issue.
> 
> I think that we can re-use iv.offset for our purposes (for crypto offset).
> So for now we can make that path work without any ABI breakage.
> Fan, please feel free to correct me here, if I missed something.
> If in future we would need to add some extra information it might
> require ABI breakage, though by now I don't envision anything particular to add.
> Anyway, if there is no objection to go that way, we can try to make
> these changes for v2.
> 

Actually, after looking at it more deeply it appears not that easy as I thought it would be :)
Below is a very draft version of proposed API additions.
I think it avoids ABI breakages right now and provides enough flexibility for future extensions (if any). 
For now, it doesn't address your comments about naming conventions (_CPU_ vs _SYNC_) , etc.
but I suppose is comprehensive enough to provide a main idea beyond it.
Akhil and other interested parties, please try to review and provide feedback ASAP,
as related changes would take some time and we still like to hit 19.11 deadline.
Konstantin

 diff --git a/lib/librte_cryptodev/rte_crypto_sym.h b/lib/librte_cryptodev/rte_crypto_sym.h
index bc8da2466..c03069e23 100644
--- a/lib/librte_cryptodev/rte_crypto_sym.h
+++ b/lib/librte_cryptodev/rte_crypto_sym.h
@@ -103,6 +103,9 @@ rte_crypto_cipher_operation_strings[];
  *
  * This structure contains data relating to Cipher (Encryption and Decryption)
  *  use to create a session.
+ * Actually I was wrong saying that we don't have free space inside xforms.
+ * Making key struct packed (see below) allow us to regain 6B that could be
+ * used for future extensions.
  */
 struct rte_crypto_cipher_xform {
        enum rte_crypto_cipher_operation op;
@@ -116,7 +119,25 @@ struct rte_crypto_cipher_xform {
        struct {
                const uint8_t *data;    /**< pointer to key data */
                uint16_t length;        /**< key length in bytes */
-       } key;
+       } __attribute__((__packed__)) key;
+
+       /**
+         * offset for cipher to start within user provided data buffer.
+        * Fan suggested another (and less space consuming way) -
+         * reuse iv.offset space below, by changing:
+        * struct {uint16_t offset, length;} iv;
+        * to uunamed union:
+        * union {
+        *      struct {uint16_t offset, length;} iv;
+        *      struct {uint16_t iv_len, crypto_offset} cpu_crypto_param;
+        * };
+        * Both approaches seems ok to me in general.
+        * Comments/suggestions are welcome.
+         */
+       uint16_t offset;
+
+       uint8_t reserved1[4];
+
        /**< Cipher key
         *
         * For the RTE_CRYPTO_CIPHER_AES_F8 mode of operation, key.data will
@@ -284,7 +305,7 @@ struct rte_crypto_auth_xform {
        struct {
                const uint8_t *data;    /**< pointer to key data */
                uint16_t length;        /**< key length in bytes */
-       } key;
+       } __attribute__((__packed__)) key;
        /**< Authentication key data.
         * The authentication key length MUST be less than or equal to the
         * block size of the algorithm. It is the callers responsibility to
@@ -292,6 +313,8 @@ struct rte_crypto_auth_xform {
         * (for example RFC 2104, FIPS 198a).
         */

+       uint8_t reserved1[6];
+
        struct {
                uint16_t offset;
                /**< Starting point for Initialisation Vector or Counter,
@@ -376,7 +399,12 @@ struct rte_crypto_aead_xform {
        struct {
                const uint8_t *data;    /**< pointer to key data */
                uint16_t length;        /**< key length in bytes */
-       } key;
+       } __attribute__((__packed__)) key;
+
+       /** offset for cipher to start within data buffer */
+       uint16_t cipher_offset;
+
+       uint8_t reserved1[4];

        struct {
                uint16_t offset;
diff --git a/lib/librte_cryptodev/rte_cryptodev.h b/lib/librte_cryptodev/rte_cryptodev.h
index e175b838c..c0c7bfed7 100644
--- a/lib/librte_cryptodev/rte_cryptodev.h
+++ b/lib/librte_cryptodev/rte_cryptodev.h
@@ -1272,6 +1272,101 @@ void *
 rte_cryptodev_sym_session_get_user_data(
                                        struct rte_cryptodev_sym_session *sess);

+/*
+ * After several thoughts decided not to try to squeeze CPU_CRYPTO
+ * into existing rte_crypto_sym_session structure/API, but instead
+ * introduce an extentsion to it via new fully opaque
+ * struct rte_crypto_cpu_sym_session and additional related API.
+ * Main points:
+ * - Current crypto-dev API is reasonably mature and it is desirable
+ *   to keep it unchanged (API/ABI stability). From other side, this
+ *   new sync API is new one and probably would require extra changes.
+ *   Having it as a new one allows to mark it as experimental, without
+ *   affecting existing one.
+ * - Fully opaque cpu_sym_session structure gives more flexibility
+ *   to the PMD writers and again allows to avoid ABI breakages in future.
+ * - process() function per set of xforms
+ *   allows to expose different process() functions for different
+ *   xform combinations. PMD writer can decide, does he wants to
+ *   push all supported algorithms into one process() function,
+ *   or spread it across several ones.
+ *   I.E. More flexibility for PMD writer.
+ * - Not storing process() pointer inside the session -
+ *   Allows user to choose does he want to store a process() pointer
+ *   per session, or per group of sessions for that device that share
+ *   the same input xforms. I.E. extra flexibility for the user,
+ *   plus allows us to keep cpu_sym_session totally opaque, see above.
+ * Sketched usage model:
+ * ....
+ * /* control path, alloc/init session */
+ * int32_t sz = rte_crypto_cpu_sym_session_size(dev_id, &xform);
+ * struct rte_crypto_cpu_sym_session *ses = user_alloc(..., sz);
+ * rte_crypto_cpu_sym_process_t process =
+ *     rte_crypto_cpu_sym_session_func(dev_id, &xform);
+ * rte_crypto_cpu_sym_session_init(dev_id, ses, &xform);
+ * ...
+ * /* data-path*/
+ * process(ses, ....);
+ * ....
+ * /* control path, termiante/free session */
+ * rte_crypto_cpu_sym_session_fini(dev_id, ses);
+ */
+
+/**
+ * vector structure, contains pointer to vector array and the length
+ * of the array
+ */
+struct rte_crypto_vec {
+       struct iovec *vec;
+       uint32_t num;
+};
+
+/*
+ * Data-path bulk process crypto function.
+ */
+typedef void (*rte_crypto_cpu_sym_process_t)(
+               struct rte_crypto_cpu_sym_session *sess,
+               struct rte_crypto_vec buf[], void *iv[], void *aad[],
+               void *digest[], int status[], uint32_t num);
+/*
+ * for given device return process function specific to input xforms
+ * on error - return NULL and set rte_errno value.
+ * Note that for same input xfroms for the same device should return
+ * the same process function.
+ */
+__rte_experimental
+rte_crypto_cpu_sym_process_t
+rte_crypto_cpu_sym_session_func(uint8_t dev_id,
+                       const struct rte_crypto_sym_xform *xforms);
+
+/*
+ * Return required session size in bytes for given set of xforms.
+ * if xforms == NULL, then return the max possible session size,
+ * that would fit session for any supported by the device algorithm.
+ * if CPU mode is not supported at all, or requeted in xform
+ * algorithm is not supported, then return -ENOTSUP.
+ */
+__rte_experimental
+int
+rte_crypto_cpu_sym_session_size(uint8_t dev_id,
+                       const struct rte_crypto_sym_xform *xforms);
+
+/*
+ * Initialize session.
+ * It is caller responsibility to allocate enough space for it.
+ * See rte_crypto_cpu_sym_session_size above.
+ */
+__rte_experimental
+int rte_crypto_cpu_sym_session_init(uint8_t dev_id,
+                       struct rte_crypto_cpu_sym_session *sess,
+                       const struct rte_crypto_sym_xform *xforms);
+
+__rte_experimental
+void
+rte_crypto_cpu_sym_session_fini(uint8_t dev_id,
+                       struct rte_crypto_cpu_sym_session *sess);
+
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/lib/librte_cryptodev/rte_cryptodev_pmd.h b/lib/librte_cryptodev/rte_cryptodev_pmd.h
index defe05ea0..ed7e63fab 100644
--- a/lib/librte_cryptodev/rte_cryptodev_pmd.h
+++ b/lib/librte_cryptodev/rte_cryptodev_pmd.h
@@ -310,6 +310,20 @@ typedef void (*cryptodev_sym_free_session_t)(struct rte_cryptodev *dev,
 typedef void (*cryptodev_asym_free_session_t)(struct rte_cryptodev *dev,
                struct rte_cryptodev_asym_session *sess);

+typedef int (*cryptodev_cpu_sym_session_size_t) (struct rte_cryptodev *dev,
+                       const struct rte_crypto_sym_xform *xforms);
+
+typedef int (*cryptodev_cpu_sym_session_init_t) (struct rte_cryptodev *dev,
+                       struct rte_crypto_cpu_sym_session *sess,
+                       const struct rte_crypto_sym_xform *xforms);
+
+typedef void (*cryptodev_cpu_sym_session_fini_t) (struct rte_cryptodev *dev,
+                       struct rte_crypto_cpu_sym_session *sess);
+
+typedef rte_crypto_cpu_sym_process_t (*cryptodev_cpu_sym_session_func_t) (
+                       struct rte_cryptodev *dev,
+                       const struct rte_crypto_sym_xform *xforms);
+
 /** Crypto device operations function pointer table */
 struct rte_cryptodev_ops {
        cryptodev_configure_t dev_configure;    /**< Configure device. */
@@ -343,6 +357,11 @@ struct rte_cryptodev_ops {
        /**< Clear a Crypto sessions private data. */
        cryptodev_asym_free_session_t asym_session_clear;
        /**< Clear a Crypto sessions private data. */
+
+       cryptodev_cpu_sym_session_size_t sym_cpu_session_get_size;
+       cryptodev_cpu_sym_session_func_t sym_cpu_session_get_func;
+       cryptodev_cpu_sym_session_init_t sym_cpu_session_init;
+       cryptodev_cpu_sym_session_fini_t sym_cpu_session_fini;
 };






More information about the dev mailing list