[dpdk-dev] [PATCH v2 10/12] net/ixgbe: enable inline ipsec

[Radu Nicolau]

Thanks for reviewing!

Some comments inline.


On 10/5/2017 6:55 PM, Ananyev, Konstantin wrote:
>
>> -----Original Message-----
>> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Akhil Goyal
>> Sent: Tuesday, October 3, 2017 2:14 PM
>> To: dev at dpdk.org
>> Cc: Doherty, Declan <declan.doherty at intel.com>; De Lara Guarch, Pablo <pablo.de.lara.guarch at intel.com>; hemant.agrawal at nxp.com;
>> Nicolau, Radu <radu.nicolau at intel.com>; borisp at mellanox.com; aviadye at mellanox.com; thomas at monjalon.net;
>> sandeep.malik at nxp.com; jerin.jacob at caviumnetworks.com; Mcnamara, John <john.mcnamara at intel.com>; olivier.matz at 6wind.com
>> Subject: [dpdk-dev] [PATCH v2 10/12] net/ixgbe: enable inline ipsec
>>
>> From: Radu Nicolau <radu.nicolau at intel.com>
>>
>> Signed-off-by: Radu Nicolau <radu.nicolau at intel.com>
>> Signed-off-by: Declan Doherty <declan.doherty at intel.com>
>> ---
>> <snip>
>>
>>   	eth_dev->dev_ops = &ixgbe_eth_dev_ops;
>> +#ifdef RTE_LIBRTE_IXGBE_IPSEC
>> +	rte_security_register(&eth_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.