DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Zhang, Roy Fan" <roy.fan.zhang@intel.com>
To: Akhil Goyal <akhil.goyal@nxp.com>, "dev@dpdk.org" <dev@dpdk.org>,
	"anoobj@marvell.com" <anoobj@marvell.com>,
	"asomalap@amd.com" <asomalap@amd.com>,
	"ruifeng.wang@arm.com" <ruifeng.wang@arm.com>,
	Nagadheeraj Rottela <rnagadheeraj@marvell.com>,
	Michael Shamis <michaelsh@marvell.com>,
	Ankur Dwivedi <adwivedi@marvell.com>,
	Jay Zhou <jianjay.zhou@huawei.com>,
	"De Lara Guarch, Pablo" <pablo.de.lara.guarch@intel.com>
Cc: "Trahe, Fiona" <fiona.trahe@intel.com>,
	"Bronowski, PiotrX" <piotrx.bronowski@intel.com>,
	"Ananyev, Konstantin" <konstantin.ananyev@intel.com>,
	Thomas Monjalon <thomas@monjalon.net>
Subject: Re: [dpdk-dev] [dpdk-dev v4 1/4] cryptodev: add symmetric crypto data-path APIs
Date: Mon, 6 Jul 2020 10:02:27 +0000	[thread overview]
Message-ID: <BL0PR11MB30438061A6C5275B004B4369B8690@BL0PR11MB3043.namprd11.prod.outlook.com> (raw)
In-Reply-To: <VI1PR04MB316870AC08947B06B5F8C870E66B0@VI1PR04MB3168.eurprd04.prod.outlook.com>

Hi Akhil,

> -----Original Message-----
> From: Akhil Goyal <akhil.goyal@nxp.com>
> Sent: Saturday, July 4, 2020 7:16 PM
> To: Zhang, Roy Fan <roy.fan.zhang@intel.com>; dev@dpdk.org;
> anoobj@marvell.com; asomalap@amd.com; ruifeng.wang@arm.com;
> Nagadheeraj Rottela <rnagadheeraj@marvell.com>; Michael Shamis
> <michaelsh@marvell.com>; Ankur Dwivedi <adwivedi@marvell.com>; Jay
> Zhou <jianjay.zhou@huawei.com>; De Lara Guarch, Pablo
> <pablo.de.lara.guarch@intel.com>
> Cc: Trahe, Fiona <fiona.trahe@intel.com>; Bronowski, PiotrX
> <piotrx.bronowski@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Thomas Monjalon
> <thomas@monjalon.net>
> Subject: RE: [dpdk-dev v4 1/4] cryptodev: add symmetric crypto data-path
> APIs
> 
> Hi Fan,
> 
> > +
> > +/**
> > + * Asynchronous operation job descriptor.
> > + * Used by HW crypto devices direct API call that supports such activity
> > + **/
> > +struct rte_crypto_sym_job {
> > +	union {
> > +		/**
> > +		 * When RTE_CRYPTO_HW_ENQ_FLAG_IS_SGL bit is set in
> flags,
> > sgl
> > +		 * field is used as input data. Otherwise data_iova is
> > +		 * used.
> > +		 **/
> > +		rte_iova_t data_iova;
> > +		struct rte_crypto_sgl *sgl;
> > +	};
> > +	union {
> > +		/**
> > +		 * Different than cryptodev ops, all ofs and len fields have
> > +		 * the unit of bytes (including Snow3G/Kasumi/Zuc.
> > +		 **/
> > +		struct {
> > +			uint32_t cipher_ofs;
> > +			uint32_t cipher_len;
> > +		} cipher_only;
> > +		struct {
> > +			uint32_t auth_ofs;
> > +			uint32_t auth_len;
> > +			rte_iova_t digest_iova;
> > +		} auth_only;
> > +		struct {
> > +			uint32_t aead_ofs;
> > +			uint32_t aead_len;
> > +			rte_iova_t tag_iova;
> > +			uint8_t *aad;
> > +			rte_iova_t aad_iova;
> > +		} aead;
> > +		struct {
> > +			uint32_t cipher_ofs;
> > +			uint32_t cipher_len;
> > +			uint32_t auth_ofs;
> > +			uint32_t auth_len;
> > +			rte_iova_t digest_iova;
> > +		} chain;
> > +	};
> > +	uint8_t *iv;
> > +	rte_iova_t iv_iova;
> > +};
> 
> NACK,
> Why do you need this structure definitions again when you have similar ones
> (the ones used in CPU crypto) available for the same purpose.
> In case of CPU crypto, there were 2 main requirements
> - synchronous API instead of enq +deq
> - raw buffers.
> 

As you may have seen the structure definition is hW centric with the
IOVA addresses all over. Also as you will from the patch series the operation is 
Per operation basis instead of operating in a burst. The external application
may sooner know when a specific enqueue is failed. 

> Now for this patchset, the requirement is
> - raw buffers
> - asynchronous APIs
> 
> The data structure for raw buffers and crypto related offsets are already
> defined
> So they should be reused.
> And I believe with some changes in rte_crypto_op  and rte_crypto_sym_op,
> We can support raw buffers with the same APIs.
> Instead of m_src and m_dst, raw buffer data structures can be combined in a
> Union and some of the fields in the rte_crypto_op can be left NULL in case of
> raw buffers.
> 

This is a good point but we still face too many unnecessary fields to be NULL, such as
digest pointers, I have given a lot thought to this structure. Hopefully it covers
all vendor's HW symmetric crypto needs and in the same time it well squeeze 
the required HW addresses into 1 cacheline, instead of rte_crypto_op + 
rte_crypto_sym_op 3 cacheline footprint. Another purpose of the structure design
is the structure buffer can be taken from stack and can be used to fill all
jobs to the PMD HW.

> 
> > +/* Struct for user to perform HW specific enqueue/dequeue function calls
> */
> > +struct rte_crypto_hw_ops {
> > +	/* Driver written queue pair data pointer, should NOT be alterred by
> > +	 * the user.
> > +	 */
> > +	void *qp;
> > +	/* Function handler to enqueue AEAD job */
> > +	rte_crypto_hw_enq_cb_fn enqueue_aead;
> > +	/* Function handler to enqueue cipher only job */
> > +	rte_crypto_hw_enq_cb_fn enqueue_cipher;
> > +	/* Function handler to enqueue auth only job */
> > +	rte_crypto_hw_enq_cb_fn enqueue_auth;
> > +	/* Function handler to enqueue cipher + hash chaining job */
> > +	rte_crypto_hw_enq_cb_fn enqueue_chain;
> > +	/* Function handler to query processed jobs */
> > +	rte_crypto_hw_query_processed query_processed;
> > +	/* Function handler to dequeue one job and return opaque data
> stored
> > */
> > +	rte_crypto_hw_deq_one_cb_fn dequeue_one;
> > +	/* Function handler to dequeue many jobs */
> > +	rte_crypto_hw_deq_many_cb_fn dequeue_many;
> > +	/* Reserved */
> > +	void *reserved[8];
> > +};
> 
> Why do we need such callbacks in the library?
> These should be inside the drivers, or else we do the same for
> Legacy case as well. The pain of finding the correct enq function for
> Appropriate crypto operation is already handled by all the drivers
> And we can reuse that or else we modify it there as well.
> 

Providing different types of enqueue functions for specific operation type
could save a lot of branches for the driver to handle. As mentioned this
data-path API is intended to be used as an advanced feature to provide
close-to-native perf to external library/applications that are not mbuf
centric. And I don't agree classifying choosing 1 enqueue function from
4 candidates as "pain".

> We should not add a lot of data paths for the user, otherwise the
> APIs will become centric to a particular vendor and it will be very difficult
> For the user to migrate from one vendor to another and would defeat the
> Purpose of DPDK which provide uniform abstraction layer for all the
> hardware
> Vendors.
> 

The purpose of adding data-path for the user is performance for non-mbuf
data-path centric applications/libraries, in the meantime not creating
confusion. In this version we aim to provide a more friendly data-path for
them, and aims to be useful to all vendor's PMDs. If there is any place in
the API that blocks a PMD please let me know.

> Adding other vendors to comment.
> 
> Regards,
> Akhil

  reply	other threads:[~2020-07-06 10:02 UTC|newest]

Thread overview: 39+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-12 14:39 [dpdk-dev] [PATCH] crypto/qat: add " Fan Zhang
2020-06-18 17:50 ` Trahe, Fiona
2020-06-25 13:31 ` [dpdk-dev] [dpdk-dev v2 0/3] crypto/qat: add symmetric crypto " Fan Zhang
2020-06-25 13:31   ` [dpdk-dev] [dpdk-dev v2 1/3] crypto/qat: add " Fan Zhang
2020-06-25 13:31   ` [dpdk-dev] [dpdk-dev v2 2/3] test/crypto: add unit-test for QAT direct APIs Fan Zhang
2020-06-30 17:47     ` Trahe, Fiona
2020-06-25 13:31   ` [dpdk-dev] [dpdk-dev v2 3/3] doc: add QAT direct APIs guide Fan Zhang
2020-07-03 10:14   ` [dpdk-dev] [dpdk-dev v3 0/3] cryptodev: add symmetric crypto data-path APIs Fan Zhang
2020-07-03 10:14     ` [dpdk-dev] [dpdk-dev v3 1/3] crypto/qat: add support to direct " Fan Zhang
2020-07-03 10:14     ` [dpdk-dev] [dpdk-dev v3 2/3] test/crypto: add unit-test for cryptodev direct APIs Fan Zhang
2020-07-03 10:14     ` [dpdk-dev] [dpdk-dev v3 3/3] doc: add cryptodev direct APIs guide Fan Zhang
2020-07-03 11:09   ` [dpdk-dev] [dpdk-dev v3 0/4] cryptodev: add symmetric crypto data-path APIs Fan Zhang
2020-07-03 11:09     ` [dpdk-dev] [dpdk-dev v3 1/4] " Fan Zhang
2020-07-03 11:09     ` [dpdk-dev] [dpdk-dev v3 1/3] crypto/qat: add support to direct " Fan Zhang
2020-07-03 11:09     ` [dpdk-dev] [dpdk-dev v3 2/4] " Fan Zhang
2020-07-03 11:09     ` [dpdk-dev] [dpdk-dev v3 2/3] test/crypto: add unit-test for cryptodev direct APIs Fan Zhang
2020-07-03 11:09     ` [dpdk-dev] [dpdk-dev v3 3/3] doc: add cryptodev direct APIs guide Fan Zhang
2020-07-03 11:09     ` [dpdk-dev] [dpdk-dev v3 3/4] test/crypto: add unit-test for cryptodev direct APIs Fan Zhang
2020-07-03 11:09     ` [dpdk-dev] [dpdk-dev v3 4/4] doc: add cryptodev direct APIs guide Fan Zhang
2020-07-03 12:49     ` [dpdk-dev] [dpdk-dev v4 0/4] cryptodev: add symmetric crypto data-path APIs Fan Zhang
2020-07-03 12:49       ` [dpdk-dev] [dpdk-dev v4 1/4] " Fan Zhang
2020-07-04 18:16         ` Akhil Goyal
2020-07-06 10:02           ` Zhang, Roy Fan [this message]
2020-07-06 12:13             ` Akhil Goyal
2020-07-07 12:37               ` Zhang, Roy Fan
2020-07-07 20:37                 ` Akhil Goyal
2020-07-08 15:09                   ` Zhang, Roy Fan
2020-07-03 12:49       ` [dpdk-dev] [dpdk-dev v4 2/4] crypto/qat: add support to direct " Fan Zhang
2020-07-03 12:49       ` [dpdk-dev] [dpdk-dev v4 3/4] test/crypto: add unit-test for cryptodev direct APIs Fan Zhang
2020-07-03 12:49       ` [dpdk-dev] [dpdk-dev v4 4/4] doc: add cryptodev direct APIs guide Fan Zhang
2020-07-13 16:57       ` [dpdk-dev] [dpdk-dev v5 0/4] cryptodev: add symmetric crypto data-path APIs Fan Zhang
2020-07-13 16:57         ` [dpdk-dev] [dpdk-dev v5 1/4] cryptodev: add " Fan Zhang
2020-07-13 16:57         ` [dpdk-dev] [dpdk-dev v5 2/4] crypto/qat: add support to direct " Fan Zhang
2020-07-13 16:57         ` [dpdk-dev] [dpdk-dev v5 3/4] test/crypto: add unit-test for cryptodev direct APIs Fan Zhang
2020-07-13 16:57         ` [dpdk-dev] [dpdk-dev v5 4/4] doc: add cryptodev direct APIs guide Fan Zhang
2020-06-26  6:55 ` [dpdk-dev] [PATCH] crypto/qat: add data-path APIs Jerin Jacob
2020-06-26 10:38   ` [dpdk-dev] [dpdk-techboard] " Thomas Monjalon
2020-06-30 20:33     ` Honnappa Nagarahalli
2020-06-30 21:00       ` Thomas Monjalon

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=BL0PR11MB30438061A6C5275B004B4369B8690@BL0PR11MB3043.namprd11.prod.outlook.com \
    --to=roy.fan.zhang@intel.com \
    --cc=adwivedi@marvell.com \
    --cc=akhil.goyal@nxp.com \
    --cc=anoobj@marvell.com \
    --cc=asomalap@amd.com \
    --cc=dev@dpdk.org \
    --cc=fiona.trahe@intel.com \
    --cc=jianjay.zhou@huawei.com \
    --cc=konstantin.ananyev@intel.com \
    --cc=michaelsh@marvell.com \
    --cc=pablo.de.lara.guarch@intel.com \
    --cc=piotrx.bronowski@intel.com \
    --cc=rnagadheeraj@marvell.com \
    --cc=ruifeng.wang@arm.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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).