Re: [dpdk-dev] [PATCH v2 00/15] add OCTEONTX2 inline IPsec support

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

> 
> On Wed, Jan 22, 2020 at 6:26 PM Akhil Goyal <akhil.goyal@nxp.com> wrote:
> >
> > > > >
> > > > > Hi Jerin,
> > > > >
> > > > > Will  do the suggested change (RX/rx-> Rx & TX/tx->Tx). Do you want me
> > > > > to trim the headline as well?
> > > > >
> > > >
> > > > Hi Anoob,
> > > >
> > > > > @Akhil, did you get a chance to review the series? Do you have any
> > > > > comments on the patches?
> > > > >
> > > >
> > > > You are adding inline ipsec support to ethernet device and not a crypto
> > > > device.
> > > > These patches should not be part of crypto PMD. There will be cyclic
> > > > dependency Between ethernet device and crypto device which can be
> easily
> > > > avoided.
> > >
> > > [Anoob] We have plans to use lookaside protocol to handle the "fallback"
> > > session. And that involves session sharing between inline and lookaside
> protocol
> > > offloads. Also, though the feature is exposed as a feature of ethdev, on our
> > > platform, it's the crypto block which primarily implements the feature. And
> so, if
> > > the code is moved to ethdev dir, there would be lot of code duplication. The
> > > idea is to have all security related code in one place.
> > >
> > > Also, the PMDs don't have any calls to each other. The communication
> between
> > > the two happens via common. The crypto dev PMD will register the required
> > > security ops to a common structure and ethdev would get it from there. So
> there
> > > won't be an issue of build dependency.
> > >
> >
> > - The code that need to be duplicated can be moved to drivers/common/
> 
> I would like to keep the common code that is common to all the coprocessors.
> 
> Moreover, there are logistic issues in that case where
> a) drivers/common/octeontx2/ going through master repo. So we will be
> creating unnecessary dependency with that 'master' tree.
> b) crypto and ethdev work is done by different teams so we would like
> to make responsibly clear wrt the review and ownership.

I can agree upon that the code shall not be placed in the common/octeontx2, 
But you can have a header file in crypto/octeontx2/ which has some inline functions
Which can be called from the ethernet as well as crypto device for inline proto
And lookaside cases respectively. I think with that the maintainability would not
be an issue.

IMO, following approach can be looked upon and may be cleaner
- define security ctx and its ops in net/octeontx2.c or net/octeontx2_security.c 
These are all control path and should not be any issue.

- define crypto specific code (inline functions) in crypto/octeontx2_security.h
Which can be called for both inline protocol as well as lookaside proto case.

- for data path you can have the processing as is. I hope all dependencies can
Be dealt with as the code will be there in crypto driver for all the mapping of 
Event and crypto queues.

- all APIs which are common for ethernet device and crypto device in
octeontx2_security.h should not prefix eth as is the case in current APIs.

I hope this will clear the ugliness of the code.
> 
> I would like to keep the security/crypto-related code to driver/crypto and
> hook to driver/net/octeontx2 with required functionalities over the
> driver/common
> using a few function pointers to remove the cyclic build dependency.
> 
> Considering there is no cyclic build and shared library dependency
> now, Can we make forward progress
> with the existing scheme?
> 
> > - You may not need to include rte_ethdev.h inside drivers/crypto
> > - otx2_sec_eth_ctx_create should be part of ethdev and you would need
> similar API
> > for crypto device as well when you would support the fallback session support.
> > So that would go in crypto. Session creation code may be common and can go
> > in drivers/common.
> > - You would have separate security_ctx for both eth device and crypto device
> and that should
> > In net device and crypto device separately. Similarly security->ops should be
> different in both of them.
> > However if they may have same session creation code and that can go in
> common.
> >