Re: [dpdk-dev] [PATCH v4 06/10] ipsec: implement SA data-path API

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

> -----Original Message-----
> From: Akhil Goyal [mailto:akhil.goyal@nxp.com]
> Sent: Friday, December 21, 2018 2:51 PM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org
> Cc: Thomas Monjalon <thomas@monjalon.net>; Awal, Mohammad Abdul <mohammad.abdul.awal@intel.com>; olivier.matz@6wind.com
> Subject: Re: [dpdk-dev] [PATCH v4 06/10] ipsec: implement SA data-path API
> 
> 
> 
> On 12/21/2018 7:57 PM, Ananyev, Konstantin wrote:
> >>>>> + */
> >>>>> +
> >>>>> +/*
> >>>>> + * Move preceding (L3) headers down to remove ESP header and IV.
> >>>>> + */
> >>>> why cant we use rte_mbuf APIs to append/prepend/trim/adjust lengths.
> >>> We do use rte_mbuf append/trim, etc. adjust mbuf's data_ofs and data_len.
> >>> But apart from that for transport mode we have to move actual packet headers.
> >>> Let say for inbound we have to get rid of ESP header (which is after IP header),
> >>> but preserve IP header, so we moving L2/L3 headers down, overwriting ESP header.
> >> ok got your point
> >>>> I believe these adjustments are happening in the mbuf itself.
> >>>> Moreover these APIs are not specific to esp headers.
> >>> I didn't get your last sentence: that function is used to remove esp header
> >>> (see above) - that's why I named it that way.
> >> These can be used to remove any header and not specifically esp. So this
> >> API could be generic in rte_mbuf.
> > That function has nothing to do with mbuf in general.
> > It just copies bytes between overlapping in certain way buffers
> > (src.start < dst.start < src.end < dst.end).
> > Right now it is very primitive - copies on byte at a time in
> > descending order.
> > Wrote it just to avoid using memmove().
> > I don't think there is any point to have such dummy function in the lib/eal.
> If this is better than memmove, then probably it is a candidate to a
> function in lib.

If it would be something really smart - I would try to push it into the EAL myself.
But it is a dumb for() loop, nothing more.

> I think Thomas/ Olivier can better comment on this
> >
> >>>>> +static inline void
> >>>>> +remove_esph(char *np, char *op, uint32_t hlen)
> >>>>> +{
> >>>>> +	uint32_t i;
> >>>>> +
> >>>>> +	for (i = hlen; i-- != 0; np[i] = op[i])
> >>>>> +		;
> >>>>> +}
> >>>>> +
> >>>>> +/*
> >>>>> +
> >>>>> +/* update original and new ip header fields for tunnel case */
> >>>>> +static inline void
> >>>>> +update_tun_l3hdr(const struct rte_ipsec_sa *sa, void *p, uint32_t plen,
> >>>>> +		uint32_t l2len, rte_be16_t pid)
> >>>>> +{
> >>>>> +	struct ipv4_hdr *v4h;
> >>>>> +	struct ipv6_hdr *v6h;
> >>>>> +
> >>>>> +	if (sa->type & RTE_IPSEC_SATP_MODE_TUNLV4) {
> >>>>> +		v4h = p;
> >>>>> +		v4h->packet_id = pid;
> >>>>> +		v4h->total_length = rte_cpu_to_be_16(plen - l2len);
> >>>> where are we updating the rest of the fields, like ttl, checksum, ip
> >>>> addresses, etc
> >>> TTL, ip addresses and other fileds supposed to be setuped by user
> >>> and provided via rte_ipsec_sa_init():
> >>> struct rte_ipsec_sa_prm.tun.hdr  should contain prepared template
> >>> for L3(and L2 if user wants to) header.
> >>> Checksum calculation is not done inside the lib right now -
> >>> it is a user responsibility to caclucate/set it after librte_ipsec
> >>> finishes processing the packet.
> >> I believe static fields are updated during sa init but some fields like
> >> ttl and checksum,
> >> can be updated in the library itself which is updated for every packet.
> >> (https://tools.ietf.org/html/rfc1624)
> > About checksum - there is no point to calculate cksum it in the lib,
> > as user may choose to use HW chksum offload.
> > All other libraries ip_frag, GSO, etc. leave it to the user,
> > I don't see why ipsec should be different here.
> > About TTL and other fields - I suppose you refer to:
> > https://tools.ietf.org/html/rfc4301#section-5.1.2
> > Header Construction for Tunnel Mode
> > right?
> > Surely that has to be supported, one way or the other,
> > but we don't plan to implement it in 19.02.
> > Current plan to add it in 19.05, if time permits.
> I am not talking about the outer ip checksum.
> Sorry the placement of the
> comment was not quite right. But I do not see that happening.
> My question is will the function ipip_outbound in ipsec-secgw called
> from the application or will it be moved inside the library.
> I believe this should be inside the lib.

I think the same - we probably need to support all described in RFC
header updates inside the process/prepare lib functions,
or at least provide a separate function for the user to perform them.
Though as I said above it is definitely not in 19.02 scope.

> 
> 
> >>>>> +	} else {
> >>>>> +		v6h = p;
> >>>>> +		v6h->payload_len = rte_cpu_to_be_16(plen - l2len -
> >>>>> +				sizeof(*v6h));
> >>>>> +	}
> >>>>> +}
> >>>>> +
> >>>>> +#endif /* _IPH_H_ */
> >>>>> diff --git a/lib/librte_ipsec/ipsec_sqn.h b/lib/librte_ipsec/ipsec_sqn.h
> >>>>> index 1935f6e30..6e18c34eb 100644
> >>>>> --- a/lib/librte_ipsec/ipsec_sqn.h
> >>>>> +++ b/lib/librte_ipsec/ipsec_sqn.h
> >>>>> @@ -15,6 +15,45 @@
> >>>>>
> >>>>>     #define IS_ESN(sa)	((sa)->sqn_mask == UINT64_MAX)
> >>>>>
> >>>>> +/*
> >>>>> + * gets SQN.hi32 bits, SQN supposed to be in network byte order.
> >>>>> + */
> >>>>> +static inline rte_be32_t
> >>>>> +sqn_hi32(rte_be64_t sqn)
> >>>>> +{
> >>>>> +#if RTE_BYTE_ORDER == RTE_BIG_ENDIAN
> >>>>> +	return (sqn >> 32);
> >>>>> +#else
> >>>>> +	return sqn;
> >>>>> +#endif
> >>>>> +}
> >>>>> +
> >>>>> +/*
> >>>>> + * gets SQN.low32 bits, SQN supposed to be in network byte order.
> >>>>> + */
> >>>>> +static inline rte_be32_t
> >>>>> +sqn_low32(rte_be64_t sqn)
> >>>>> +{
> >>>>> +#if RTE_BYTE_ORDER == RTE_BIG_ENDIAN
> >>>>> +	return sqn;
> >>>>> +#else
> >>>>> +	return (sqn >> 32);
> >>>>> +#endif
> >>>>> +}
> >>>>> +
> >>>>> +/*
> >>>>> + * gets SQN.low16 bits, SQN supposed to be in network byte order.
> >>>>> + */
> >>>>> +static inline rte_be16_t
> >>>>> +sqn_low16(rte_be64_t sqn)
> >>>>> +{
> >>>>> +#if RTE_BYTE_ORDER == RTE_BIG_ENDIAN
> >>>>> +	return sqn;
> >>>>> +#else
> >>>>> +	return (sqn >> 48);
> >>>>> +#endif
> >>>>> +}
> >>>>> +
> >>>> shouldn't we move these seq number APIs in rte_esp.h and make them generic
> >>> It could be done, but who will use them except librte_ipsec?
> >> Whoever uses rte_esp.h and not use ipsec lib. The intent of rte_esp.h is
> >> just for that only, otherwise we don't need rte_esp.h, we can have the
> >> content of rte_esp.h in ipsec itself.
> > Again these functions are used just inside the lib to help avoid
> > extra byteswapping during crypto-data/packet header constructions.
> Agreed, my point is why adding a new file for managing seq numbering in
> esp headers, when this can be easily moved to rte_esp.h.
> 
> > I don't see how they will be useful in general.
> > Sure, if there will be demand from users in future - we can move them,
> > but right now I don't think that would happen.
> In that case we can get away with esp.h as well and move that in this
> new file and see if users need it separately, then we move it.

esp.h already exists and is used in several other places: 
find lib drivers -type f | xargs grep '<rte_esp.h>' | grep include
lib/librte_pipeline/rte_table_action.c:#include <rte_esp.h>
lib/librte_ethdev/rte_flow.h:#include <rte_esp.h>

Konstantin