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

[Ananyev, Konstantin]

> -----Original Message-----
> From: Nicolau, Radu
> Sent: Thursday, October 19, 2017 2:14 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
> 
> 
> 
> 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;

Yes, as I said that's for future.

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

By metadata do you mean a session or ...?
Konstantin

> >
> >>>>>>> +	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 :)
> >>
> >>
> >>