On 1/27/2020 2:54 PM, Anoob Joseph wrote: > Hi Jerin, Akhil, > > Let me summarize the design changes from the discussions below. > > Currently, drivers/crypto/octeontx2/otx2_security.c defines all security ctx > ops for the ethdev (idea was to add all crypto security ctx for lookaside > also there). That will be moved to drivers/net/octeontx2 as is. The routines > which are doing qp_add & qp_remove would be moved to common (discussed > below). Otherwise, the rest should remain as is. If Jerin/Akhil wants further > isolation, please do share specifics. Almost all functions in otx2_security.c > is dereferencing 'rte_eth_dev'. So having (void *) will not help. > > The functions in otx2_security.c is calling inline functions in > otx2_ipsec_fp.h (which has lower level implementations of session create > etc). This will remain as is in drivers/crypto/octeontx2 but would be called > from drivers/net/octeontx2/otx2_security.c. > > We will need to include otx2_cryptodev_qp.h (internal header in > drivers/crypto/octeontx2) since the crypto queue pair is required for > outbound processing. Since otx2_cryptodev_qp.h has dependency on > rte_cryptodev.h, the ethdev file will have dependency on rte_cryptodev.h. > > I want all the maintainers (Akhil, Jerin & Ferruh) to ack the above behavior > so that I can proceed with the restructuring. (Currently issue is > rte_ethdev.h getting included in a cryptodev PMD file. The case we are > proposing is the exact mirror of that) > > Currently, the cryptodev has to do qp-eth port mapping and save it somewhere > for eth dev to use during security session create. This will have to be saved > in drivers/common/octeontx2. > > @Ferruh, do you agree with the suggestions here? With the proposed changes, > parts of the patches would go into ethdev space (for reviews and merge), and > the rest would be in crypto space.
Hi Anoob, It is OK to move the 'rte_security_ops' to net driver, as far as I can see there is no way to isolate ethdev and cryptodev completely, at least having one way ethdev->crypto dependency is better. Not sure about using inline functions though, existing solution to use common/x looks OK to me. > > Thanks, > Anoob > >> -----Original Message----- >> From: Akhil Goyal <akhil.go...@nxp.com> >> Sent: Monday, January 27, 2020 5:18 PM >> To: Jerin Jacob <jerinjac...@gmail.com> >> Cc: Anoob Joseph <ano...@marvell.com>; Declan Doherty >> <declan.dohe...@intel.com>; Thomas Monjalon <tho...@monjalon.net>; Jerin >> Jacob Kollanukkaran <jer...@marvell.com>; Narayana Prasad Raju Athreya >> <pathr...@marvell.com>; Kiran Kumar Kokkilagadda >> <kirankum...@marvell.com>; Nithin Kumar Dabilpuram >> <ndabilpu...@marvell.com>; Pavan Nikhilesh Bhagavatula >> <pbhagavat...@marvell.com>; Ankur Dwivedi <adwiv...@marvell.com>; >> Archana Muniganti <march...@marvell.com>; Tejasree Kondoj >> <ktejas...@marvell.com>; Vamsi Krishna Attunuru <vattun...@marvell.com>; >> Lukas Bartosik <lbarto...@marvell.com>; dpdk-dev <dev@dpdk.org> >> Subject: [EXT] RE: [dpdk-dev] [PATCH v2 00/15] add OCTEONTX2 inline IPsec >> support >> >> External Email >> >> ---------------------------------------------------------------------- >> Hi Jerin, >> >>> >>> On Mon, Jan 27, 2020 at 4:10 PM Akhil Goyal <akhil.go...@nxp.com> wrote: >>>> >>>> >>>>> >>>>> On Wed, Jan 22, 2020 at 6:26 PM Akhil Goyal <akhil.go...@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. >>> >>> One problem with such an approach is we need to have fat inline functions. >>> In some case, those inline functions to needs accessing the >>> array/driver specific symbols in another driver namespace then those >>> array needs to be exported in map file and hence the build dependency >>> comes. >> >> How many such symbols are there. I don’t they will be many. Can they be >> Passed >> as argument in the APIs to avoid build dependencies. >> >>> >>> >>>> >>>> - 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. >>> >>> If I understand it correctly, You have a concern in having the >>> *rte_eth* symbols in driver/crypto/octeontx2? >>> If so, we can check what can be done. Let us know the exact your >>> concern in managing the code in this model? >> >> Yes, rte_eth* symbols should not be there in crypto driver. >> Because crypto driver is not leveraging any ethernet functionality, It is >> the other >> way, ethernet device is using the crypto functionality/ Structs etc for >> supporting >> inline IPSEC. >> >> Also, the security ctx should be part of ethdev and its ops should be >> Defined in >> ethernet device which may call some inline APIs placed in >> Drivers/crypto/octeontx2/ >> >>> >>>> >>>> I hope this will clear the ugliness of the code. >>> >>> It is relative, I think, having fat inline functions and accessing >>> both drivers is ugly. >> >> Breaking the way an API need to be defined and used is even more uglier. >> IMO, having fat inline functions will act as external library functions >> which Are >> independent of the device which is calling it. >> Something similar to drivers/common/dpaax/caamflib/. >> >> My original suggestion was to put it in common, but I am ok, if you want >> that In >> the crypto driver. I agree with the decision that all crypto/ipsec related >> stuff >> Should be there under drivers/crypto if it is getting used from both the net >> and >> Crypto driver. But atleast the API definitions should be there where it >> should be. >> >> >>> >>>>> >>>>> 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. >>>>>> >>>>
