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

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

Hi Akhil

> -----Original Message-----
> From: Akhil Goyal <akhil.goyal@nxp.com>
> Sent: Wednesday, April 22, 2020 2:44 PM
> 
> Hi David,
> > Hi Akhil,
> >
> > > > >
> > > I did not look at your patches completely, but looking at the ops
> > > that you have added For rawdev are pretty much same as that of a crypto
> device.
> > >
> > > I see that there are 2 types of ops that you need
> > > - session create/destroy
> > > - enq/deq
> > >
> > > On the first impression of your patchset, I see that you want to enq
> > > to driver only once for both The operations - CRC and crypto.
> > >
> > > So what is the issue in using the cryptodev_enqueue for processing
> > > in the existing AESNI-MB driver.
> > > For session creation, the cryptodev layer will not give flexibility
> > > to add
> > > CRC+crypto kind of sessions.
> > > But in case of rte_security, you can define your new session xform
> > > based on your requirement.
> > >
> > > And while doing the cryptodev enq/deq, based on the session type,
> > > you can process the packet Specific to your usecase in your aesni-mb
> > > PMD
> > >
> > > Now if you want to add compression also along with crypto, then you
> > > can define another xform which Will be combination of
> > > crypto+compression and the aesni-mb PMD can have another mode
> which
> > > Can make sessions based on the new xform and the enq and deq can be
> > > done using the cryptodev enq/deq.
> > > For all your cases you will be having only one action type -
> > > lookaside protocol and can define different Protocols (that may not be
> standard).
> > >
> > > So to conclude, your AESNI-MB will have 3 types of operations
> > > - plain crypto
> > > - crc+crypto
> > > - compression+crypto
> > >
> > > I believe this is doable or did I miss something very obvious?
> >
> > [DC] Thank you for this feedback
> >
> > I have done this exact same analysis on rte_security and how we could use
> it.
> >
> > The main issue of this approach (and it may be possible to easily
> > overcome) is that ultimately crypto_op's need to be enqueued into
> > cryptodev. This means we can't easily control the CRC (or compression
> > in the future) at the operation level - application developers using
> > this API would create a
> > Crypto+CRC security xform session  for a
> > particular flow but may want to turn off the CRC part for some packets
> > in that flow.
> >
> > There are a number of ways this issue could possibly be overcome:
> > 1) the auth offset/length fields in a rte_crypto_op could be
> > overloaded to control the CRC part of the combined operation
> >     - this is not the cleanest approach
> > 2) we add a "security" op struct of some type to the union at end of
> > the rte_crypto_op
> >     - to avoid any circular dependencies, this would need to be opaque
> > to rte_cryptodev
> >     - rte_cryptodev should not be aware of rte_security
> >
> > Number 2 above is probably the cleaner and more preferable approach.
> 
> 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.

[DC]  Yes we would add to this union at the end of rte_crypto_op

        __extension__
        union {
                struct rte_crypto_sym_op sym[0];
                /**< Symmetric operation parameters */

                struct rte_crypto_asym_op asym[0];
                /**< Asymmetric operation parameters */

        }; /**< operation specific parameters */

I haven't figured out the finer details yet, but it should be straightforward to add some security element here.
As these are zero length arrays, we won't be affecting the size of rte_crypto_op if we add another zero length array.

We should not include rte_security.h and add something like struct rte_security_op sec[0] here though, as that would
cause a circular dependency between rte_cryptodev and rte_security.
This should be resolvable though

> 
> One more thing that can be looked into is the recently added CPU crypto
> process API If that could of any use, we may extend that if need be.

[DC] This is also being targeted at QAT and we would like to maintain the same
Interface for these use-cases for both AESNI-MB and QAT.

So I think the traditional enqueue/dequeue API is what we would initially use as it
means users of this API can easily switch between AESNI-MB and QAT. However, we
may look at the CPU crypto API for AESNI-MB in the future.

> 
> >
> > The other approach is that CRC is either on/off at the session level.
> > That limitation would then need to be adhered by application
> > developers, which is something we would ideally like to avoid.
> 
> You mean that CRC can be on/off per session as well as per packet?
> I think that can also be handled when you are defining your own security_op
> for per packet.

[DC] I meant that if we didn't take the approach defining a security_op, then
we would have turn on/off CRC at the session level and impose that limit on
the app developers. But yes, by defining a security_op, we can probably turn
it on/off at both session and op level.

> 
> >
> > The rawdev multi-function approach did not have these issues which is
> > one of the reasons we have pursued this approach to date.
> >
> > However, we think the rte_security approach is workable.
> > It still requires some deeper analysis but with your support, we think
> > we can overcome the challenges.
> >
> Yes, please let me know where ever my help is required.
[DC] Thank you, appreciate that