From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tiwei Bie Subject: Re: [PATCH v3 0/6] Add MACsec offload support for ixgbe Date: Tue, 27 Dec 2016 09:33:31 +0800 Message-ID: <20161227013330.GA145018@dpdk19> References: <1481852611-103254-1-git-send-email-tiwei.bie@intel.com> <1482677880-117158-1-git-send-email-tiwei.bie@intel.com> <20161226151537.GD22106@6wind.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Cc: dev@dpdk.org, wenzhuo.lu@intel.com, wei.dai@intel.com, xiao.w.wang@intel.com, olivier.matz@6wind.com, thomas.monjalon@6wind.com, konstantin.ananyev@intel.com, helin.zhang@intel.com To: Adrien Mazarguil Return-path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 4FD4A282 for ; Tue, 27 Dec 2016 02:38:56 +0100 (CET) Content-Disposition: inline In-Reply-To: <20161226151537.GD22106@6wind.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi Adrien, On Mon, Dec 26, 2016 at 04:15:37PM +0100, Adrien Mazarguil wrote: > Hi Tiwei, > > On Sun, Dec 25, 2016 at 10:57:54PM +0800, Tiwei Bie wrote: > > This patch set adds the MACsec offload support for ixgbe. > > The testpmd is also updated to support MACsec cmds. > > I'm not commenting on any specific patch from this series, however I'm > noticing this new trend of working around ethdev to add PMD-specific APIs. > I would like to make sure it's not getting out of hand. > > To use the "rte_pmd_ixgbe_macsec_*()" API, applications must be linked with > librte_pmd_ixgbe directly and have the related code under #ifdef > RTE_LIBRTE_IXGBE_PMD like testpmd. > > Here we can see this ixgbe-specific API affects rte_mbuf.h and rte_ethdev.h > (new PKT_TX_MACSEC, RTE_ETH_EVENT_MACSEC, DEV_RX_OFFLOAD_MACSEC_STRIP and > DEV_TX_OFFLOAD_MACSEC_INSERT flags). > > - Shouldn't these flags have "IXGBE" somewhere in their name and/or be > defined under #ifdef RTE_LIBRTE_IXGBE_PMD? > I think those flags are very generic, simple and innocent, so we could just define them in the normal way. And currently it seems that we lack a way to define the PMD-specific flags for mbuf, ethdev, etc. > - Why can't the MACsec API be defined globally, for instance won't i40e > implement it as well someday? > I think currently we prefer to implement some PMD features based on the PMD-specific APIs at first to avoid the bloating of the ethdev APIs. And when it proves to be generic (which means many people really need it and care about it, and more drivers are really going to implement it), then we make it as the global ethdev API. > - Why bothering with TX/RX offload capabilities if applications know the > underlying PMD anyway? > Not every NIC supported by IXGBE supports the MACsec offload. So even if the application knows it's using IXGBE PMD, it still needs to check whether the MACsec offload capabilities are supported. > Assuming these patches are kept as-is, I suggest we define a reserved space > documented as such for PMD-specific flags wherever they are used. > It sounds good to me. But it may involve many pieces of the lib, such as the mbuf ol_flags, ethdev event type, ethdev offload capabilities, and so on. So maybe it can be done as another work. Thank you very much for your comments! :-) Best regards, Tiwei Bie > > v2: > > - Update the documents for testpmd; > > - Update the release notes; > > - Reuse the functions provided by base code; > > > > v3: > > - Add the missing parts of MACsec mbuf flag and reorganize the patch set; > > - Add an ethdev event type for MACsec; > > - Advertise the MACsec offload capabilities based on the mac type; > > - Minor fixes and improvements; > > > > Tiwei Bie (6): > > mbuf: add flag for MACsec > > ethdev: add event type for MACsec > > ethdev: add MACsec offload capability flags > > net/ixgbe: add MACsec offload support > > app/testpmd: add MACsec offload commands > > doc: add ixgbe specific APIs > > > > app/test-pmd/cmdline.c | 389 ++++++++++++++++++++++ > > app/test-pmd/macfwd.c | 2 + > > app/test-pmd/macswap.c | 2 + > > app/test-pmd/testpmd.h | 2 + > > app/test-pmd/txonly.c | 2 + > > doc/guides/rel_notes/release_17_02.rst | 10 + > > doc/guides/testpmd_app_ug/testpmd_funcs.rst | 32 ++ > > drivers/net/ixgbe/ixgbe_ethdev.c | 481 +++++++++++++++++++++++++++- > > drivers/net/ixgbe/ixgbe_ethdev.h | 45 +++ > > drivers/net/ixgbe/ixgbe_rxtx.c | 3 + > > drivers/net/ixgbe/rte_pmd_ixgbe.h | 100 ++++++ > > drivers/net/ixgbe/rte_pmd_ixgbe_version.map | 11 + > > lib/librte_ether/rte_ethdev.h | 3 + > > lib/librte_mbuf/rte_mbuf.c | 2 + > > lib/librte_mbuf/rte_mbuf.h | 6 + > > 15 files changed, 1085 insertions(+), 5 deletions(-) > > > > -- > > 2.7.4 > > > > -- > Adrien Mazarguil > 6WIND