Re: [PATCH 1/7] examples/ipsec-secgw: disable Tx chksum offload for inline

[Nithin Kumar Dabilpuram <ndabilpuram@marvell.com>]

Hi Konstantin,

Please see inline.

On 4/14/22 7:37 PM, Ananyev, Konstantin wrote:
> Hi Nithin,
> 
> 
>> Enable Tx IPv4 checksum offload only when Tx inline crypto, lookaside
>> crypto/protocol or cpu crypto is needed.
>> For Tx Inline protocol offload, checksum computation
>> is implicitly taken care by HW.
> 
> The thing is that right now it is not stated explicitly that
> RTE_SECURITY_ACTION_TYPE_INLINE_PROTOCOL implies TSO support. It says that it 'might', it is not guaranteed.
> At least in dpdk docs.
>  From https://doc.dpdk.org/guides/prog_guide/rte_security.html:
> "22.1.2. Inline protocol offload
> ...
> Egress Data path - The software will send the plain packet without any security protocol headers added to the packet. The driver will configure the security index and other requirement in tx descriptors. The hardware device will do security processing on the packet that includes adding the relevant protocol headers and encrypting the data before sending the packet out. The software should make sure that the buffer has required head room and tail room for any protocol header addition. The software may also do early fragmentation if the resultant packet is expected to cross the MTU size.
> Note
> The underlying device will manage state information required for egress processing. E.g. in case of IPsec, the seq number will be added to the packet, however the device shall provide indication when the sequence number is about to overflow. The underlying device may support post encryption TSO."
> 
> So, if I am not mistaken, what you suggest will change HW/PMD requirements.
> AFAIK, right now only Marvell supports RTE_SECURITY_ACTION_TYPE_INLINE_PROTOCOL,
> so in theory I don't mind if you'd like to harden the requirements here.
> Though such change probably needs to be properly documented and
> acked by other vendors.

Ok, I was only thinking of IPV4 CKSUM offload without TSO and thought 
that is not needed in case of INLINE PROTOCOL.

To maintain the behavior for TSO with INLINE_PROTO, I can set both 
IPV4_CKSUM offload and TCP_TSO if TSO is requested i.e rule->mss is set.
We can revist the spec for TSO+INLINE_PROTOCOL offload combination later 
as our HW doesn't support TSO before INLINE IPSEC Processing.


> 
>>
>> Signed-off-by: Nithin Dabilpuram <ndabilpuram@marvell.com>
>> ---
>>   examples/ipsec-secgw/ipsec-secgw.c |  3 ---
>>   examples/ipsec-secgw/sa.c          | 32 +++++++++++++++++++++++++-------
>>   2 files changed, 25 insertions(+), 10 deletions(-)
>>
>> diff --git a/examples/ipsec-secgw/ipsec-secgw.c b/examples/ipsec-secgw/ipsec-secgw.c
>> index 42b5081..76919e5 100644
>> --- a/examples/ipsec-secgw/ipsec-secgw.c
>> +++ b/examples/ipsec-secgw/ipsec-secgw.c
>> @@ -2330,9 +2330,6 @@ port_init(uint16_t portid, uint64_t req_rx_offloads, uint64_t req_tx_offloads)
>>   		local_port_conf.txmode.offloads |=
>>   			RTE_ETH_TX_OFFLOAD_MBUF_FAST_FREE;
>>
>> -	if (dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_IPV4_CKSUM)
>> -		local_port_conf.txmode.offloads |= RTE_ETH_TX_OFFLOAD_IPV4_CKSUM;
>> -
>>   	printf("port %u configuring rx_offloads=0x%" PRIx64
>>   		", tx_offloads=0x%" PRIx64 "\n",
>>   		portid, local_port_conf.rxmode.offloads,
>> diff --git a/examples/ipsec-secgw/sa.c b/examples/ipsec-secgw/sa.c
>> index 1839ac7..36d890f 100644
>> --- a/examples/ipsec-secgw/sa.c
>> +++ b/examples/ipsec-secgw/sa.c
>> @@ -1785,13 +1785,31 @@ sa_check_offloads(uint16_t port_id, uint64_t *rx_offloads,
>>   	for (idx_sa = 0; idx_sa < nb_sa_out; idx_sa++) {
>>   		rule = &sa_out[idx_sa];
>>   		rule_type = ipsec_get_action_type(rule);
>> -		if ((rule_type == RTE_SECURITY_ACTION_TYPE_INLINE_CRYPTO ||
>> -				rule_type ==
>> -				RTE_SECURITY_ACTION_TYPE_INLINE_PROTOCOL)
>> -				&& rule->portid == port_id) {
>> -			*tx_offloads |= RTE_ETH_TX_OFFLOAD_SECURITY;
>> -			if (rule->mss)
>> -				*tx_offloads |= RTE_ETH_TX_OFFLOAD_TCP_TSO;
>> +		switch (rule_type) {
>> +		case RTE_SECURITY_ACTION_TYPE_INLINE_PROTOCOL:
>> +			/* Checksum offload is not needed for inline protocol as
>> +			 * all processing for Outbound IPSec packets will be
>> +			 * implicitly taken care and for non-IPSec packets,
>> +			 * there is no need of IPv4 Checksum offload.
>> +			 */
>> +			if (rule->portid == port_id)
>> +				*tx_offloads |= RTE_ETH_TX_OFFLOAD_SECURITY;
>> +			break;
>> +		case RTE_SECURITY_ACTION_TYPE_INLINE_CRYPTO:
>> +			if (rule->portid == port_id) {
>> +				*tx_offloads |= RTE_ETH_TX_OFFLOAD_SECURITY;
>> +				if (rule->mss)
>> +					*tx_offloads |=
>> +						RTE_ETH_TX_OFFLOAD_TCP_TSO;
>> +				*tx_offloads |= RTE_ETH_TX_OFFLOAD_IPV4_CKSUM;
>> +			}
>> +			break;
>> +		default:
>> +			/* Enable IPv4 checksum offload even if one of lookaside
>> +			 * SA's are present.
>> +			 */
>> +			*tx_offloads |= RTE_ETH_TX_OFFLOAD_IPV4_CKSUM;
> 
> Shouldn't we check here that given port really supports IPV4_CKSUM offload?

It is already being checked at port_init().

> 
>> +			break;
>>   		}
>>   	}
>>   	return 0;
>> --
>> 2.8.4
>