Thanks for reviewing!
Some comments inline.
On 10/5/2017 6:55 PM, Ananyev, Konstantin wrote:
-----Original Message-----
From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Akhil Goyal
Sent: Tuesday, October 3, 2017 2:14 PM
To: dev@dpdk.org
Cc: Doherty, Declan <declan.dohe...@intel.com>; De Lara Guarch, Pablo
<pablo.de.lara.gua...@intel.com>; hemant.agra...@nxp.com;
Nicolau, Radu <radu.nico...@intel.com>; bor...@mellanox.com;
avia...@mellanox.com; tho...@monjalon.net;
sandeep.ma...@nxp.com; jerin.ja...@caviumnetworks.com; Mcnamara, John
<john.mcnam...@intel.com>; olivier.m...@6wind.com
Subject: [dpdk-dev] [PATCH v2 10/12] net/ixgbe: enable inline ipsec
From: Radu Nicolau <radu.nico...@intel.com>
Signed-off-by: Radu Nicolau <radu.nico...@intel.com>
Signed-off-by: Declan Doherty <declan.dohe...@intel.com>
---
<snip>
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.
<snip>
+#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.
<snip>
}
@@ -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.
