> -----Original Message----- > From: Akhil Goyal [mailto:akhil.go...@nxp.com] > Sent: Friday, December 21, 2018 2:51 PM > To: Ananyev, Konstantin <konstantin.anan...@intel.com>; dev@dpdk.org > Cc: Thomas Monjalon <tho...@monjalon.net>; Awal, Mohammad Abdul > <mohammad.abdul.a...@intel.com>; olivier.m...@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