Re: [dpdk-dev] [PATCH v3 0/4] add AESNI-MB rawdev for multi-function processing

["Coyle, David" <david.coyle@intel.com>]

Hi Fan & Akhil,

> -----Original Message-----
> From: Zhang, Roy Fan <roy.fan.zhang@intel.com>
> Sent: Friday, May 1, 2020 2:18 PM
> 
> Hi Akhil,
> 
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Akhil Goyal
> > Sent: Wednesday, April 22, 2020 2:44 PM
> > To: Coyle, David <david.coyle@intel.com>; Doherty, Declan
> > <declan.doherty@intel.com>; Thomas Monjalon
> <thomas@monjalon.net>;
> > Yigit, Ferruh <ferruh.yigit@intel.com>; Trahe, Fiona
> > <fiona.trahe@intel.com>
> > Cc: techboard@dpdk.org; dev@dpdk.org; De Lara Guarch, Pablo
> > <pablo.de.lara.guarch@intel.com>; Ryan, Brendan
> > <brendan.ryan@intel.com>; Hemant Agrawal
> <hemant.agrawal@nxp.com>;
> > Anoob Joseph <anoobj@marvell.com>; Ruifeng Wang
> > <ruifeng.wang@arm.com>; Liron Himi <lironh@marvell.com>; Nagadheeraj
> > Rottela <rnagadheeraj@marvell.com>; Srikanth Jampala
> > <jsrikanth@marvell.com>; Gagandeep Singh <G.Singh@nxp.com>; Jay
> Zhou
> > <jianjay.zhou@huawei.com>; Ravi Kumar <ravi1.kumar@amd.com>;
> > Richardson, Bruce <bruce.richardson@intel.com>;
> > olivier.matz@6wind.com; honnappa.nagarahalli@arm.com; Stephen
> > Hemminger <stephen@networkplumber.org>; alexr@mellanox.com
> > Subject: Re: [dpdk-dev] [PATCH v3 0/4] add AESNI-MB rawdev for multi-
> > function processing
> ...
> > Yes, it is preferred, but it should be a union to
> > rte_crypto_sym_op/rte_crypto_asym_op.
> > Crypto_op->type as RTE_CRYPTO_OP_TYPE_SECURITY and sess_type as
> > RTE_CRYPTO_OP_SECURITY_SESSION The size of rte_crypto_op will remain
> > as is and there will be no ABI breakage I guess.
> >
> [Fan: with this way the PMD will have to do rte_crypto_op.type check, and
> then look into rte_security_op field, only when it find the security_op type is
> crypto_crc, it will process the security_op data. Would that being too many
> reads and checking for a single op? Can we create a new API for rte_security
> to process rte_security_ops for Crypto_CRC or future needs?] ...

[DC] If we were to add new enqueue/dequeue APIs to rte_security, then this may
cause extra churn and extra paths of code in a customer's application. For the
DOCSIS Crypto-CRC use-case which is currently supported by IPSecMB, only the
AES-DOCSISBPI cipher algorithm is supported. For these Crypto-CRC ops, they would
create rte_security sessions, attach these to rte_security_ops and enqueue/dequeue
using the new APIs in rte_security.

However, the customer may also be using the legacy DES-DOCSISBPI cipher algorithm
for some subscribers, and this algorithm is not supported in the chained Crypto-CRC
functionality in IPSecMB (and most likely never will be). So for these the customer
would need to create cryptodev sessions, attach these to rte_crypto_ops and enqueue/
dequeue with the cryptodev enq/deq APIs. That is 2 different paths of code now in
the application datapath, where some packets in a batch need to be enqueued through
rte_security and some need to be enqueued through cryptodev.

If rte_crypto_ops are always used and enqueued/dequeued through cryptodev, then the
only thing that changes is the type of session that is created and either the security session
or the cryptodev session gets attached to the crypto_op.

Now, we could add support to rte_security for DES-DOCSISBPI too, but it would not be a
combined operation with CRC - it would be a simple cipher operation going through
rte_security. But that, to me, does not seem like a good use of rte_security.

For DOCSIS Crypto-CRC, we may also want to take advantage of the
rte_cryptodev_sym_cpu_crypto_process() API which was added to cryptodev recently to
avoid the enqueue/dequeue overhead. A similar API would also then need to be added
to rte_security.

Taking all of the above into account, I feel keeping the normal cryptodev enqueue/dequeue
would be best. Having said all that, we do need to consider performance in the PMD of the
extra op type checks. Take aesni_mb PMD as an example. It would need to check
rte_crypto_op->type and if it's not RTE_CRYPTO_OP_TYPE_SECURITY, then it can assume
it's an RTE_CRYPTO_OP_TYPE_SYMMETRIC op and carry on as normal for existing symmetric
operations. Security ops will need some extra parsing but this is new functionality. The impact
on existing functionality of the extra checks would certainly need to be tested though, but as
all the op data will be in the same cache line, I don't see any major impact.

Akhil & Fan (& others), I would be interested to hear your feedback on this.

Regards,
David

> 
> Regards,
> Fan