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

[Akhil Goyal <akhil.goyal@nxp.com>]

On 12/20/2018 6:26 PM, Ananyev, Konstantin wrote:
>
>>> 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.
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.
>
>>> +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)
>
>>> +	} 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.