[dpdk-dev] [PATCH v4 10/12] net/ixgbe: enable inline ipsec

[Radu Nicolau]

On 10/19/2017 1:29 PM, Ananyev, Konstantin wrote:
>
>> -----Original Message-----
>> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Ananyev, Konstantin
>> Sent: Thursday, October 19, 2017 1:17 PM
>> To: Nicolau, Radu <radu.nicolau at intel.com>; Akhil Goyal <akhil.goyal at nxp.com>; dev at dpdk.org
>> Cc: Doherty, Declan <declan.doherty at intel.com>; De Lara Guarch, Pablo <pablo.de.lara.guarch at intel.com>; hemant.agrawal at nxp.com;
>> borisp at mellanox.com; aviadye at mellanox.com; thomas at monjalon.net; sandeep.malik at nxp.com; jerin.jacob at caviumnetworks.com;
>> Mcnamara, John <john.mcnamara at intel.com>; shahafs at mellanox.com; olivier.matz at 6wind.com
>> Subject: Re: [dpdk-dev] [PATCH v4 10/12] net/ixgbe: enable inline ipsec
>>
>>
>>
>>> -----Original Message-----
>>> From: Nicolau, Radu
>>> Sent: Thursday, October 19, 2017 12:57 PM
>>> To: Ananyev, Konstantin <konstantin.ananyev at intel.com>; Akhil Goyal <akhil.goyal at nxp.com>; dev at dpdk.org
>>> Cc: Doherty, Declan <declan.doherty at intel.com>; De Lara Guarch, Pablo <pablo.de.lara.guarch at intel.com>; hemant.agrawal at nxp.com;
>>> borisp at mellanox.com; aviadye at mellanox.com; thomas at monjalon.net; sandeep.malik at nxp.com; jerin.jacob at caviumnetworks.com;
>>> Mcnamara, John <john.mcnamara at intel.com>; shahafs at mellanox.com; olivier.matz at 6wind.com
>>> Subject: RE: [PATCH v4 10/12] net/ixgbe: enable inline ipsec
>>>
>>>
>>>
>>>> -----Original Message-----
>>>> From: Ananyev, Konstantin
>>>> Sent: Thursday, October 19, 2017 12:04 PM
>>>> To: Nicolau, Radu <radu.nicolau at intel.com>; Akhil Goyal
>>>> <akhil.goyal at nxp.com>; dev at dpdk.org
>>>> Cc: Doherty, Declan <declan.doherty at intel.com>; De Lara Guarch, Pablo
>>>> <pablo.de.lara.guarch at intel.com>; hemant.agrawal at nxp.com;
>>>> borisp at mellanox.com; aviadye at mellanox.com; thomas at monjalon.net;
>>>> sandeep.malik at nxp.com; jerin.jacob at caviumnetworks.com; Mcnamara,
>>>> John <john.mcnamara at intel.com>; shahafs at mellanox.com;
>>>> olivier.matz at 6wind.com
>>>> Subject: RE: [PATCH v4 10/12] net/ixgbe: enable inline ipsec
>>>>
>>>>
>>>>
>>>>>>> <snip>
>>>>>>> +
>>>>>>> +static int
>>>>>>> +ixgbe_crypto_update_mb(void *device __rte_unused,
>>>>>>> +		struct rte_security_session *session,
>>>>>>> +		       struct rte_mbuf *m, void *params __rte_unused) {
>
>
> Another sort of generic question - why not make security_set_pkt_metadata function
> to accept  bulk of packets?
> In that case o can minimize the cost of function calls, accessing session data, etc.
> Though I suppose that could wait till next patch series.
> Konstantin
It is a good suggestion, but we need to discuss it further; for example 
if it can accept a bulk of packets, will it need also a bulk of metadata 
pointers, or just one for all the packets?
>
>>>>>>> +	struct ixgbe_crypto_session *ic_session =
>>>>>>> +			get_sec_session_private_data(session);
>>>>>>> +	if (ic_session->op == IXGBE_OP_AUTHENTICATED_ENCRYPTION) {
>>>>>>> +		struct ixgbe_crypto_tx_desc_md *mdata =
>>>>>>> +			(struct ixgbe_crypto_tx_desc_md *)&m->udata64;
>>>>>>> +		mdata->enc = 1;
>>>>>>> +		mdata->sa_idx = ic_session->sa_index;
>>>>>>> +		mdata->pad_len = *rte_pktmbuf_mtod_offset(m,
>>>>>>> +			uint8_t *, rte_pktmbuf_pkt_len(m) - 18) + 18;
>>>>>> Could you explain what pad_len supposed to contain?
>>>>>> Also what is a magical constant '18'?
>>>>>> Could you create some macro if needed?
>>>>> I added an explanation in the code, we read the payload padding size
>>>>> that is stored at the len-18 bytes and add 18 bytes, 2 for ESP trailer
>>>>> and 16 for ICV.
>>>> Ok, can we at least have a macros for all these constants?
>>>> Another question: you do use pkt_len() here - does it mean that multi-
>>>> segment packets are not supported by ixgbe-ipsec?
>>>> Konstantin
>>> It does support multisegment, but the pad_len has to be set only for single send, it will be ignored otherwise. I have updated the code to
>> set
>>> it for single segment packets only.
>> Sorry, I didn't understand that.
>> If that function does support multiseg packets, then it has to go to the last segment via m->next,
>> If it doesn't, then it should return an error I case of m->nb_seg != 1.
>> Right?
>>
>>> Also, our test app does not support multisegment packets.
>> Ok, I suppose that means, multi-seg case wasn't tested :)
>>
>>
>>