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 <ndabilpu...@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
> >
