Re: [dpdk-dev] [dpdk-dev v9 1/4] cryptodev: add crypto data-path service APIs

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

Hi Fan,

> > >
> > > +/** Crypto data-path service types */
> > > +enum rte_crypto_dp_service {
> > > +	RTE_CRYPTO_DP_SYM_CIPHER_ONLY = 0,
> > > +	RTE_CRYPTO_DP_SYM_AUTH_ONLY,
> > > +	RTE_CRYPTO_DP_SYM_CHAIN,
> > > +	RTE_CRYPTO_DP_SYM_AEAD,
> > > +	RTE_CRYPTO_DP_N_SERVICE
> > > +};
> >
> > Comments missing for this enum.
> > Do we really need this enum?
> > Can we not have this info in the driver from the xform list?
> > And if we really want to add this, why to have it specific to raw data path APIs?
> >
> Will add comments to this enum.
> Unless the driver will store xform data in certain way (in fact QAT has it) the
> driver may not know which data-path to choose from.
> The purpose of having this enum is that the driver knows to attach the correct
> handler into the service data structure fast.
> 
I believe all drivers are storing that information already in some way in the session private data.
This enum is maintained inside driver as of current implementation. This is not specific to raw
Data path APIs. If you are introducing this enum in library, then it should be generic for the legacy
Case as well.


> >
> > > +union rte_crypto_sym_additional_data {
> > > +	struct {
> > > +		void *cipher_iv_ptr;
> > > +		rte_iova_t cipher_iv_iova;
> > > +		void *auth_iv_ptr;
> > > +		rte_iova_t auth_iv_iova;
> > > +		void *digest_ptr;
> > > +		rte_iova_t digest_iova;
> > > +	} cipher_auth;
> >
> > Should be chain instead of cipher_auth
> This field is used for cipher only, auth only, or chain use-cases so I believe this is
> a better name for it.

Agreed that this struct will be used for all 3 cases, that is what is happening in
Other crypto cases. We use chain for all these three cases in legacy codepath.
Chain can be of one or two xforms and ordering can be anything -
Cipher only, auth only, cipher auth and auth cipher.


> >
> > > +	struct {
> > > +		void *iv_ptr;
> > > +		rte_iova_t iv_iova;
> > > +		void *digest_ptr;
> > > +		rte_iova_t digest_iova;
> > > +		void *aad_ptr;
> > > +		rte_iova_t aad_iova;
> > > +	} aead;
> > > +};
> > > +
> > >  /**
> > >   * Synchronous operation descriptor.
> > >   * Supposed to be used with CPU crypto API call.
> > > @@ -57,12 +81,25 @@ struct rte_crypto_sgl {
> > >  struct rte_crypto_sym_vec {
> > >  	/** array of SGL vectors */
> > >  	struct rte_crypto_sgl *sgl;
> > > -	/** array of pointers to IV */
> > > -	void **iv;
> > > -	/** array of pointers to AAD */
> > > -	void **aad;
> > > -	/** array of pointers to digest */
> > > -	void **digest;
> > > +
> > > +	union {
> > > +
> > > +		/* Supposed to be used with CPU crypto API call. */
> > > +		struct {
> > > +			/** array of pointers to IV */
> > > +			void **iv;
> > > +			/** array of pointers to AAD */
> > > +			void **aad;
> > > +			/** array of pointers to digest */
> > > +			void **digest;
> > > +		};
> >
> > Can we also name this struct?
> > Probably we should split this as a separate patch.
> [Then this is an API break right?]

Since this an LTS release, I am ok to take this change.
But others can comment on this.
@Ananyev, Konstantin, @Thomas Monjalon
Can you comment on this?

> >
> > > +
> > > +		/* Supposed to be used with
> > > rte_cryptodev_dp_sym_submit_vec()
> > > +		 * call.
> > > +		 */
> > > +		union rte_crypto_sym_additional_data *additional_data;
> > > +	};
> > > +
> >
> > Can we get rid of this unnecessary union rte_crypto_sym_additional_data
> > And place chain and aead directly in the union? At any point, only one of the
> > three
> > would be used.
> We have 2 main different uses cases, 1 for cpu crypto and 1 for data path APIs.
> Within each main uses case there are 4 types of algo (cipher only/auth
> only/aead/chain), one requiring HW address and virtual address, the other
> doesn't.
> It seems to causing too much confusion to include these many union into the
> structure that initially was designed for cpu crypto only.
> I suggest better to use different structure than squeeze all into a big union.
> 

IMO, the following union can clarify all doubts.
@Ananyev, Konstantin: Any suggestions from your side?

/** IV and aad information for various use cases. */
union {
        /** Supposed to be used with CPU crypto API call. */
        struct {
                /** array of pointers to IV */
                void **iv;
                /** array of pointers to AAD */
                void **aad;
                /** array of pointers to digest */
                void **digest;
        } cpu_crypto;  < or any other useful name> 
        /* Supposed to be used with HW raw crypto API call. */
        struct {
                void *cipher_iv_ptr;
                rte_iova_t cipher_iv_iova;
                void *auth_iv_ptr;
                rte_iova_t auth_iv_iova;
                void *digest_ptr;
                rte_iova_t digest_iova;
        } hw_chain;
        /* Supposed to be used with HW raw crypto API call. */
        struct {
                void *iv_ptr;
                rte_iova_t iv_iova;
                void *digest_ptr;
                rte_iova_t digest_iova;
                void *aad_ptr;
                rte_iova_t aad_iova;
        } hw_aead;
};



> > > +/**
> > > + * Context data for asynchronous crypto process.
> > > + */
> > > +struct rte_crypto_dp_service_ctx {
> > > +	void *qp_data;
> > > +
> > > +	struct {
> > > +		cryptodev_dp_submit_single_job_t submit_single_job;
> > > +		cryptodev_dp_sym_submit_vec_t submit_vec;
> > > +		cryptodev_dp_sym_operation_done_t submit_done;
> > > +		cryptodev_dp_sym_dequeue_t dequeue_opaque;
> > > +		cryptodev_dp_sym_dequeue_single_job_t dequeue_single;
> > > +		cryptodev_dp_sym_operation_done_t dequeue_done;
> > > +	};
> > > +
> > > +	/* Driver specific service data */
> > > +	__extension__ uint8_t drv_service_data[];
> > > +};
> >
> > Comments missing for structure params.
> > Struct name can be rte_crypto_raw_dp_ctx.
> >
> > Who allocate and free this structure?
> Same as crypto session, the user need to query the driver specific service data
> Size and allocate the buffer accordingly. The difference is it does not have to
> Be from mempool as it can be reused.

So this structure is saved and filled by the lib/driver and not the application. Right?
This struct is opaque to application and will be part of session private data. Right?
Assignment and calling appropriate driver's call backs will be hidden inside library
and will be opaque to the application. In other words, the structure is not exposed
to the application.
Please add relevant comments on top of this structure.


> > > +static __rte_always_inline int
> > > +_cryptodev_dp_sym_dequeue_single_job(struct
> > rte_crypto_dp_service_ctx
> > > *ctx,
> > > +		void **out_opaque)
> > > +{
> > > +	return (*ctx->dequeue_single)(ctx->qp_data, ctx->drv_service_data,
> > > +		out_opaque);
> > > +}
> > > +
> > > +/**
> > > + * Submit single job into device queue but the driver will not start
> > > + * processing until rte_cryptodev_dp_submit_done() is called. This is a
> > > + * simplified

Comment not complete.

Regards,
Akhil