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

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

> >
> > diff --git a/lib/librte_ipsec/crypto.h b/lib/librte_ipsec/crypto.h
> > new file mode 100644
> > index 000000000..61f5c1433
> > --- /dev/null
> > +++ b/lib/librte_ipsec/crypto.h
> > @@ -0,0 +1,123 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(c) 2018 Intel Corporation
> > + */
> > +
> > +#ifndef _CRYPTO_H_
> > +#define _CRYPTO_H_
> > +
> > +/**
> > + * @file crypto.h
> > + * Contains crypto specific functions/structures/macros used internally
> > + * by ipsec library.
> > + */
> > +
> > + /*
> > +  * AES-GCM devices have some specific requirements for IV and AAD formats.
> > +  * Ideally that to be done by the driver itself.
> > +  */
> I believe these can be moved to rte_crypto_sym.h. All crypto related
> stuff should be at same place.

Not sure what exactly you suggest to put into rte_crypto_sym.h?
struct aead_gcm_iv? Something else?
From my perspective it would be good if user in ctypto_sym_op
just fill salt and IV fields, and then PMD setup things in needed 
format internally.
Again it would be really good if crypto_sym_op has reserved space
for AAD...
But  all that implies quite a big change in cryptodev and PMDs,
so I think should be subject of a separate patch.

> > +
> > +struct aead_gcm_iv {
> > +	uint32_t salt;
> > +	uint64_t iv;
> > +	uint32_t cnt;
> > +} __attribute__((packed));
> > +
> > +struct aead_gcm_aad {
> > +	uint32_t spi;
> > +	/*
> > +	 * RFC 4106, section 5:
> > +	 * Two formats of the AAD are defined:
> > +	 * one for 32-bit sequence numbers, and one for 64-bit ESN.
> > +	 */
> > +	union {
> > +		uint32_t u32[2];
> > +		uint64_t u64;
> > +	} sqn;
> > +	uint32_t align0; /* align to 16B boundary */
> > +} __attribute__((packed));
> > +
> > +struct gcm_esph_iv {
> > +	struct esp_hdr esph;
> > +	uint64_t iv;
> > +} __attribute__((packed));
> > +
> > +
> > +static inline void
> > +aead_gcm_iv_fill(struct aead_gcm_iv *gcm, uint64_t iv, uint32_t salt)
> > +{
> > +	gcm->salt = salt;
> > +	gcm->iv = iv;
> > +	gcm->cnt = rte_cpu_to_be_32(1);
> > +}
> > +
> > +/*


> > diff --git a/lib/librte_ipsec/iph.h b/lib/librte_ipsec/iph.h
> > new file mode 100644
> > index 000000000..3fd93016d
> > --- /dev/null
> > +++ b/lib/librte_ipsec/iph.h
> > @@ -0,0 +1,84 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(c) 2018 Intel Corporation
> > + */
> > +
> > +#ifndef _IPH_H_
> > +#define _IPH_H_
> > +
> > +/**
> > + * @file iph.h
> > + * Contains functions/structures/macros to manipulate IPv/IPv6 headers
> IPv4
> > + * used internally by ipsec library.
> > + */
> > +
> > +/*
> > + * 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.

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

> > +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.

> > +	} 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?