DPDK patches and discussions
 help / color / mirror / Atom feed
From: Akhil Goyal <akhil.goyal@nxp.com>
To: "Zhang, Roy Fan" <roy.fan.zhang@intel.com>,
	"dev@dpdk.org" <dev@dpdk.org>,
	 "Ananyev, Konstantin" <konstantin.ananyev@intel.com>,
	Thomas Monjalon <thomas@monjalon.net>
Cc: "Trahe, Fiona" <fiona.trahe@intel.com>,
	"Kusztal, ArkadiuszX" <arkadiuszx.kusztal@intel.com>,
	"Dybkowski, AdamX" <adamx.dybkowski@intel.com>,
	"Bronowski, PiotrX" <piotrx.bronowski@intel.com>,
	Anoob Joseph <anoobj@marvell.com>
Subject: Re: [dpdk-dev] [dpdk-dev v9 1/4] cryptodev: add crypto data-path service APIs
Date: Mon, 21 Sep 2020 11:59:42 +0000
Message-ID: <VI1PR04MB316866800565F8788413B7F7E63A0@VI1PR04MB3168.eurprd04.prod.outlook.com> (raw)
In-Reply-To: <BL0PR11MB30437EB24D98BEF232144AB8B83A0@BL0PR11MB3043.namprd11.prod.outlook.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


  reply	other threads:[~2020-09-21 11:59 UTC|newest]

Thread overview: 84+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-18 16:28 [dpdk-dev] [dpdk-dev v6 0/4] cryptodev: add " Fan Zhang
2020-08-18 16:28 ` [dpdk-dev] [dpdk-dev v6 1/4] cryptodev: add crypto " Fan Zhang
2020-08-18 16:28 ` [dpdk-dev] [dpdk-dev v6 2/4] crypto/qat: add crypto data-path service API support Fan Zhang
2020-08-18 16:28 ` [dpdk-dev] [dpdk-dev v6 3/4] test/crypto: add unit-test for cryptodev direct APIs Fan Zhang
2020-08-18 16:28 ` [dpdk-dev] [dpdk-dev v6 4/4] doc: add cryptodev service APIs guide Fan Zhang
2020-08-28 12:58 ` [dpdk-dev] [dpdk-dev v7 0/4] cryptodev: add data-path service APIs Fan Zhang
2020-08-28 12:58   ` [dpdk-dev] [dpdk-dev v7 1/4] cryptodev: add crypto " Fan Zhang
2020-08-31  6:23     ` Kusztal, ArkadiuszX
2020-08-31 12:21       ` Zhang, Roy Fan
2020-08-31 15:15       ` Zhang, Roy Fan
2020-08-28 12:58   ` [dpdk-dev] [dpdk-dev v7 2/4] crypto/qat: add crypto data-path service API support Fan Zhang
2020-08-28 12:58   ` [dpdk-dev] [dpdk-dev v7 3/4] test/crypto: add unit-test for cryptodev direct APIs Fan Zhang
2020-08-28 12:58   ` [dpdk-dev] [dpdk-dev v7 4/4] doc: add cryptodev service APIs guide Fan Zhang
2020-09-04 15:25   ` [dpdk-dev] [dpdk-dev v8 0/4] cryptodev: add data-path service APIs Fan Zhang
2020-09-04 15:25     ` [dpdk-dev] [dpdk-dev v8 1/4] cryptodev: add crypto " Fan Zhang
2020-09-07 12:36       ` Dybkowski, AdamX
2020-09-04 15:25     ` [dpdk-dev] [dpdk-dev v8 2/4] crypto/qat: add crypto data-path service API support Fan Zhang
2020-09-04 15:25     ` [dpdk-dev] [dpdk-dev v8 3/4] test/crypto: add unit-test for cryptodev direct APIs Fan Zhang
2020-09-04 15:25     ` [dpdk-dev] [dpdk-dev v8 4/4] doc: add cryptodev service APIs guide Fan Zhang
2020-09-08  8:42     ` [dpdk-dev] [dpdk-dev v9 0/4] cryptodev: add data-path service APIs Fan Zhang
2020-09-08  8:42       ` [dpdk-dev] [dpdk-dev v9 1/4] cryptodev: add crypto " Fan Zhang
2020-09-18 21:50         ` Akhil Goyal
2020-09-21 10:40           ` Zhang, Roy Fan
2020-09-21 11:59             ` Akhil Goyal [this message]
2020-09-21 15:26               ` Zhang, Roy Fan
2020-09-21 15:41               ` Zhang, Roy Fan
2020-09-21 15:49                 ` Akhil Goyal
2020-09-22  8:08                   ` Zhang, Roy Fan
2020-09-22  8:21                   ` Zhang, Roy Fan
2020-09-22  8:48                     ` Ananyev, Konstantin
2020-09-22  9:05                       ` Akhil Goyal
2020-09-22  9:28                         ` Zhang, Roy Fan
2020-09-22 10:18                           ` Ananyev, Konstantin
2020-09-22 12:15                             ` Zhang, Roy Fan
2020-09-22 12:50                             ` Zhang, Roy Fan
2020-09-22 12:52                               ` Akhil Goyal
2020-09-08  8:42       ` [dpdk-dev] [dpdk-dev v9 2/4] crypto/qat: add crypto data-path service API support Fan Zhang
2020-09-08  8:42       ` [dpdk-dev] [dpdk-dev v9 3/4] test/crypto: add unit-test for cryptodev direct APIs Fan Zhang
2020-09-18 20:03         ` Akhil Goyal
2020-09-21 12:41           ` Zhang, Roy Fan
2020-09-08  8:42       ` [dpdk-dev] [dpdk-dev v9 4/4] doc: add cryptodev service APIs guide Fan Zhang
2020-09-18 20:39         ` Akhil Goyal
2020-09-21 12:28           ` Zhang, Roy Fan
2020-09-23 13:37           ` Zhang, Roy Fan
2020-09-24 16:34       ` [dpdk-dev] [dpdk-dev v10 0/4] cryptodev: add raw data-path APIs Fan Zhang
2020-09-24 16:34         ` [dpdk-dev] [dpdk-dev v10 1/4] cryptodev: change crypto symmetric vector structure Fan Zhang
2020-09-25  8:03           ` Dybkowski, AdamX
2020-09-28 17:01           ` Ananyev, Konstantin
2020-09-24 16:34         ` [dpdk-dev] [dpdk-dev v10 2/4] cryptodev: add raw crypto data-path APIs Fan Zhang
2020-09-25  8:04           ` Dybkowski, AdamX
2020-10-08 14:26           ` Akhil Goyal
2020-10-08 15:29             ` Zhang, Roy Fan
2020-10-08 16:07               ` Akhil Goyal
2020-10-08 16:24                 ` Zhang, Roy Fan
2020-10-09  8:32                 ` Zhang, Roy Fan
2020-10-08 14:37           ` Akhil Goyal
2020-09-24 16:34         ` [dpdk-dev] [dpdk-dev v10 3/4] crypto/qat: add raw crypto data-path API support Fan Zhang
2020-09-25  8:04           ` Dybkowski, AdamX
2020-09-24 16:34         ` [dpdk-dev] [dpdk-dev v10 4/4] test/crypto: add unit-test for cryptodev raw API test Fan Zhang
2020-09-25  8:05           ` Dybkowski, AdamX
2020-10-08 15:01           ` Akhil Goyal
2020-10-08 15:04         ` [dpdk-dev] [dpdk-dev v10 0/4] cryptodev: add raw data-path APIs Akhil Goyal
2020-10-08 15:30           ` Zhang, Roy Fan
2020-10-09 21:11         ` [dpdk-dev] [dpdk-dev v11 " Fan Zhang
2020-10-09 21:11           ` [dpdk-dev] [dpdk-dev v11 1/4] cryptodev: change crypto symmetric vector structure Fan Zhang
2020-10-09 21:11           ` [dpdk-dev] [dpdk-dev v11 2/4] cryptodev: add raw crypto data-path APIs Fan Zhang
2020-10-10 19:38             ` Akhil Goyal
2020-10-10 20:40               ` Zhang, Roy Fan
2020-10-09 21:11           ` [dpdk-dev] [dpdk-dev v11 3/4] crypto/qat: add raw crypto data-path API support Fan Zhang
2020-10-09 21:11           ` [dpdk-dev] [dpdk-dev v11 4/4] test/crypto: add unit-test for cryptodev raw API test Fan Zhang
2020-10-10 19:55             ` Akhil Goyal
2020-10-10 20:50               ` Zhang, Roy Fan
2020-10-10 21:03                 ` Akhil Goyal
2020-10-11  0:32           ` [dpdk-dev] [dpdk-dev v12 0/4] cryptodev: add raw data-path APIs Fan Zhang
2020-10-11  0:32             ` [dpdk-dev] [dpdk-dev v12 1/4] cryptodev: change crypto symmetric vector structure Fan Zhang
2020-10-11  0:32             ` [dpdk-dev] [dpdk-dev v12 2/4] cryptodev: add raw crypto data-path APIs Fan Zhang
2020-10-11  0:32             ` [dpdk-dev] [dpdk-dev v12 3/4] crypto/qat: add raw crypto data-path API support Fan Zhang
2020-10-11  0:32             ` [dpdk-dev] [dpdk-dev v12 4/4] test/crypto: add unit-test for cryptodev raw API test Fan Zhang
2020-10-11  0:38             ` [dpdk-dev] [dpdk-dev v13 0/4] cryptodev: add raw data-path APIs Fan Zhang
2020-10-11  0:38               ` [dpdk-dev] [dpdk-dev v13 1/4] cryptodev: change crypto symmetric vector structure Fan Zhang
2020-10-11  0:38               ` [dpdk-dev] [dpdk-dev v13 2/4] cryptodev: add raw crypto data-path APIs Fan Zhang
2020-10-11  0:38               ` [dpdk-dev] [dpdk-dev v13 3/4] crypto/qat: add raw crypto data-path API support Fan Zhang
2020-10-11  0:38               ` [dpdk-dev] [dpdk-dev v13 4/4] test/crypto: add unit-test for cryptodev raw API test Fan Zhang
2020-10-12 16:15               ` [dpdk-dev] [dpdk-dev v13 0/4] cryptodev: add raw data-path APIs Akhil Goyal

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=VI1PR04MB316866800565F8788413B7F7E63A0@VI1PR04MB3168.eurprd04.prod.outlook.com \
    --to=akhil.goyal@nxp.com \
    --cc=adamx.dybkowski@intel.com \
    --cc=anoobj@marvell.com \
    --cc=arkadiuszx.kusztal@intel.com \
    --cc=dev@dpdk.org \
    --cc=fiona.trahe@intel.com \
    --cc=konstantin.ananyev@intel.com \
    --cc=piotrx.bronowski@intel.com \
    --cc=roy.fan.zhang@intel.com \
    --cc=thomas@monjalon.net \
    /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

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git