Re: [dpdk-dev] [PATCH v3 6/9] ipsec: implement SA data-path API

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

Hi Declan,

> 
> Only some minor some cosmetic questions
> 
> On 06/12/2018 3:38 PM, Konstantin Ananyev wrote:
> > Provide implementation for rte_ipsec_pkt_crypto_prepare() and
> ...
> 
> > +/*
> > + * Move preceding (L3) headers up to free space for ESP header and IV.
> > + */
> > +static inline void
> > +insert_esph(char *np, char *op, uint32_t hlen)
> > +{
> > +	uint32_t i;
> > +
> > +	for (i = 0; i != hlen; i++)
> > +		np[i] = op[i];
> > +}
> > +
> > +/* update original ip header fields for trasnport case */
> 
>                                             ^^typo
> > +static inline int
> > +update_trs_l3hdr(const struct rte_ipsec_sa *sa, void *p, uint32_t plen,
> > +		uint32_t l2len, uint32_t l3len, uint8_t proto)
> > +{
> > +	struct ipv4_hdr *v4h;
> > +	struct ipv6_hdr *v6h;
> > +	int32_t rc;
> > +
> > +	if ((sa->type & RTE_IPSEC_SATP_IPV_MASK) == RTE_IPSEC_SATP_IPV4) {
> > +		v4h = p;
> > +		rc = v4h->next_proto_id;
> > +		v4h->next_proto_id = proto;
> > +		v4h->total_length = rte_cpu_to_be_16(plen - l2len);
> > +	} else if (l3len == sizeof(*v6h)) {
> 
> why are you using a different method of identifying ipv6 vs ipv4, would
> checking (sa->type & RTE_IPSEC_SATP_IPV_MASK)== RTE_IPSEC_SATP_IPV6 be
> not be valid here also?

Because right now we don't have a proper support for ipv6 extended headers here.
So we accept ipv4 packets and ipv6 packets without external headers.
For ipv6 wih xhdr it will retun an error.
xhdr support is planned in 19.05

> 
> >
> ...
> > diff --git a/lib/librte_ipsec/ipsec_sqn.h b/lib/librte_ipsec/ipsec_sqn.h
> > index 4471814f9..a33ff9cca 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)
> 
> Would it make more sense to have ESN as RTE_IPSEC_SATP_ property so it
> can be retrieve through the sa type API?

We need sqn_mask in our sqn calculations anyway, so internally it is more
convenient to use it.
Though it probably is a good idea to add ESN into SATP too, so external
users can acess this info.
Will try to add it to v4.   
Konstantin