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

[Akhil Goyal <akhil.goyal@nxp.com>]

Hi Konstantin,
> 
> Hi Akhil,
> 
> > > > > > > > > > > > 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.
> >
> > No strong opinions here. OK with this one.
> >
> > > +        * Comments/suggestions are welcome.
> > > +         */
> > > +       uint16_t offset;
> 
> After another thought - it is probably a bit better to have offset as a separate
> field.
> In that case we can use the same xforms to create both type of sessions.
ok
> 
> > > +
> > > +       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.
> >
> >
> > What all things do we need to squeeze?
> > In this proposal I do not see the new struct cpu_sym_session  defined here.
> 
> The plan is to have it totally opaque to the user, i.e. just:
> struct rte_crypto_cpu_sym_session;
> in public header files.
> 
> > I believe you will have same lib API/struct for cpu_sym_session  and
> sym_session.
> 
> I thought about such way, but there are few things that looks clumsy to me:
> 1. Right now there is no 'type' (or so) field inside rte_cryptodev_sym_session,
> so it is not possible to easy distinguish what session do you have: lksd_sym or
> cpu_sym.
> In theory, there is a hole of 4B inside rte_cryptodev_sym_session, so we can add
> some extra field
> here, but in that case  we wouldn't be able to use the same xform for both
> lksd_sym or cpu_sym
> (which seems really plausible thing for me).
> 2.  Majority of rte_cryptodev_sym_session fields I think are unnecessary for
> rte_crypto_cpu_sym_session:
> sess_data[], opaque_data, user_data, nb_drivers.
> All that consumes space, that could be used somewhere else instead.
> 3. I am a bit reluctant to touch existing rte_cryptodev API - to avoid any
> breakages I can't foresee right now.
> From other side - if we'll add new functions/structs for cpu_sym_session we can
> mark it
> and keep it for some time as experimental, so further changes (if needed) would
> still be possible.
> 

OK let us assume that you have a separate structure. But I have a few queries:
1. how can multiple drivers use a same session
2. Can somebody use the scheduler pmd for scheduling the different type of payloads for the same session?

With your proposal the APIs would be very specific to your use case only.
When you would add more functionality to this sync API/struct, it will end up being the same API/struct.

Let us  see how close/ far we are from the existing APIs when the actual implementation is done.

> > I am not sure if that would be needed.
> > It would be internal to the driver that if synchronous processing is
> supported(from feature flag) and
> > Have relevant fields in xform(the newly added ones which are packed as per
> your suggestions) set,
> > It will create that type of session.
> >
> >
> > > + * 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.
> >
> > Which process function should be chosen is internal to PMD, how would that
> info
> > be visible to the application or the library. These will get stored in the session
> private
> > data. It would be upto the PMD writer, to store the per session process
> function in
> > the session private data.
> >
> > Process function would be a dev ops just like enc/deq operations and it should
> call
> > The respective process API stored in the session private data.
> 
> That model (via devops) is possible, but has several drawbacks from my
> perspective:
> 
> 1. It means we'll need to pass dev_id as a parameter to process() function.
> Though in fact dev_id is not a relevant information for us here
> (all we need is pointer to the session and pointer to the fuction to call)
> and I tried to avoid using it in data-path functions for that API.

You have a single vdev, but someone may have multiple vdevs for each thread, or may
Have same dev with multiple queues for each core.

> 2. As you pointed in that case it will be just one process() function per device.
> So if PMD would like to have several process() functions for different type of
> sessions
> (let say one per alg) first thing it has to do inside it's process() - read session data
> and
> based on that, do a jump/call to particular internal sub-routine.
> Something like:
> driver_id = get_pmd_driver_id();
> priv_ses = ses->sess_data[driver_id];
> Then either:
> switch(priv_sess->alg) {case XXX: process_XXX(priv_sess, ...);break;...}
> OR
> priv_ses->process(priv_sess, ...);
> 
> to select and call the proper function.
> Looks like totally unnecessary overhead to me.
> Though if we'll have ability to query/extract some sort session_ops based on the
> xform -
> we can avoid  this extra de-refererence+jump/call thing.

What is the issue in the priv_ses->process(); approach?
I don't understand what are you saving by not doing this.
In any case you would need to identify which session correspond to which process().
For that you would be doing it somewhere in your data path.

> 
> >
> > I am not sure if you would need a new session init API for this as nothing would
> be visible to
> > the app or lib.
> >
> > > + * - 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.
> >
> > If multiple sessions need to be processed via the same process function,
> > PMD would save the same process in all the sessions, I don't think there would
> > be any perf overhead with that.
> 
> I think it would, 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;
> > >  };
> > >
> > >
> > >