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

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

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.

Sounds reasonable.

> 
> >
> >>
> >> 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().

The problem is that we first invoke sa_check_offloads() which sets required rx/tx offloads.
If in that function we just blindly set |= RTE_ETH_TX_OFFLOAD_IPV4_CKSUM to req_tx_offloads,
while actual device doesn't support it, then later port_init() for that device will fail:

port_init(...)
{
	....
	local_port_conf.txmode.offloads |= req_tx_offloads;
	....
	if ((local_port_conf.txmode.offloads & dev_info.tx_offload_capa) !=
                        local_port_conf.txmode.offloads)
                rte_exit(EXIT_FAILURE,
                        "Error: port %u required TX offloads: 0x%" PRIx64
                        ", available TX offloads: 0x%" PRIx64 "\n",
                        portid, local_port_conf.txmode.offloads,
                        dev_info.tx_offload_capa);  

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