From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id C27A11B299 for ; Fri, 6 Oct 2017 11:18:01 +0200 (CEST) Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 06 Oct 2017 02:18:00 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.42,482,1500966000"; d="scan'208";a="159559132" Received: from rnicolau-mobl.ger.corp.intel.com (HELO [10.237.221.79]) ([10.237.221.79]) by fmsmga005.fm.intel.com with ESMTP; 06 Oct 2017 02:17:58 -0700 To: "Ananyev, Konstantin" , Akhil Goyal , "dev@dpdk.org" Cc: "Doherty, Declan" , "De Lara Guarch, Pablo" , "hemant.agrawal@nxp.com" , "borisp@mellanox.com" , "aviadye@mellanox.com" , "thomas@monjalon.net" , "sandeep.malik@nxp.com" , "jerin.jacob@caviumnetworks.com" , "Mcnamara, John" , "olivier.matz@6wind.com" References: <20170914082651.26232-1-akhil.goyal@nxp.com> <20171003131413.23846-1-akhil.goyal@nxp.com> <20171003131413.23846-11-akhil.goyal@nxp.com> <2601191342CEEE43887BDE71AB9772585FAA4EE0@IRSMSX103.ger.corp.intel.com> From: Radu Nicolau Message-ID: <1377fcac-61f1-c545-b354-52343a903349@intel.com> Date: Fri, 6 Oct 2017 10:17:57 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.1.0 MIME-Version: 1.0 In-Reply-To: <2601191342CEEE43887BDE71AB9772585FAA4EE0@IRSMSX103.ger.corp.intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US Subject: Re: [dpdk-dev] [PATCH v2 10/12] net/ixgbe: enable inline ipsec X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 06 Oct 2017 09:18:02 -0000 Thanks for reviewing! Some comments inline. On 10/5/2017 6:55 PM, Ananyev, Konstantin wrote: > >> -----Original Message----- >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Akhil Goyal >> Sent: Tuesday, October 3, 2017 2:14 PM >> To: dev@dpdk.org >> Cc: Doherty, Declan ; De Lara Guarch, Pablo ; hemant.agrawal@nxp.com; >> Nicolau, Radu ; borisp@mellanox.com; aviadye@mellanox.com; thomas@monjalon.net; >> sandeep.malik@nxp.com; jerin.jacob@caviumnetworks.com; Mcnamara, John ; olivier.matz@6wind.com >> Subject: [dpdk-dev] [PATCH v2 10/12] net/ixgbe: enable inline ipsec >> >> From: Radu Nicolau >> >> Signed-off-by: Radu Nicolau >> Signed-off-by: Declan Doherty >> --- >> >> >> eth_dev->dev_ops = &ixgbe_eth_dev_ops; >> +#ifdef RTE_LIBRTE_IXGBE_IPSEC >> + rte_security_register(ð_dev->data->sec_id, >> + (void *)eth_dev, &ixgbe_security_ops); >> +#endif /* RTE_LIBRTE_IXGBE_IPSEC */ > I still wonder do we really need new config macro and > Ifdef it through all ixgbe code? > Can we have it just always on? > If the RX/TX performance suffers a lot we can have a special > RX/TX functions for ipsec enabled case. I only put it there in case there is a performance degradation, but I'm fairly certain that there is none. So if you think that's best I will remove it, but just in case that there is a performance degradation for non-ipsec traffic it will provide a quick way to turn the feature off. > >> >> +#include "base/ixgbe_type.h" >> +#include "base/ixgbe_api.h" >> +#include "ixgbe_ethdev.h" >> +#include "ixgbe_ipsec.h" >> + >> + >> +#define IXGBE_WAIT_RW(__reg, __rw) \ >> +{ \ >> + int cnt = 100; \ >> + IXGBE_WRITE_REG(hw, (__reg), reg); \ >> + while (((IXGBE_READ_REG(hw, (__reg))) & (__rw)) && (cnt--)) \ >> + rte_delay_us(1); \ >> +} > Looks usefull. > Probably worth to add cnt as a parameter and put the macro (or even better inline func) > Inside base/ixgbe_osdep.h. First let me explain why I've put it there: in the datasheet it is stated that after the software requests a write the hw will perform the write and afterwards clear the write bit (7.12.9.2.1). My understanding is that I need to wait for the write bit to clear until I request another subsequent write (and there are multiple writes into multiple tables in succession for setting up the Rx SA). I added the cnt because I wasn't comfortable with a potentially endless loop... So if you think that this will be useful in other places I will move it as you say. > >> >> } >> >> @@ -4981,6 +5024,22 @@ ixgbe_dev_rxtx_start(struct rte_eth_dev *dev) >> dev->data->dev_conf.lpbk_mode == IXGBE_LPBK_82599_TX_RX) >> ixgbe_setup_loopback_link_82599(hw); > As I can see from the datasheet LRO and IPsec are mutually exclusive, > plus IPsec requires hw crc strip enabled. > I think you need add extra checks regarding that in ixgbe_dev_rx_init() or so. > Another thing - probably need to update ixgbe_set_tx_function() to > select full-featured TX func when txmode.enable_sec is on. I will look into it.