Re: [dpdk-dev] [PATCH 03/20] security: add hfn override option in PDCP

[Anoob Joseph <anoobj@marvell.com>]

Hi Akhil,

Please see inline.

Thanks,
Anoob

> -----Original Message-----
> From: Akhil Goyal <akhil.goyal@nxp.com>
> Sent: Thursday, September 19, 2019 9:01 PM
> To: konstantin.ananyev@intel.com; Anoob Joseph <anoobj@marvell.com>;
> Radu Nicolau <radu.nicolau@intel.com>
> Cc: Hemant Agrawal <hemant.agrawal@nxp.com>; Vakul Garg
> <vakul.garg@nxp.com>; dev@dpdk.org; Akhil Goyal <akhil.goyal@nxp.com>
> Subject: [EXT] RE: [PATCH 03/20] security: add hfn override option in PDCP
> 
> External Email
> 
> ----------------------------------------------------------------------
> Hi Konstantin/Anoob/Radu,
> 
> Any comments on this patch.
> 
> Regards,
> Akhil
> >
> > HFN can be given as a per packet value also.
> > As we do not have IV in case of PDCP, and HFN is used to generate IV.
> > IV field can be used to get the per packet HFN while enq/deq If
> > hfn_ovrd field in pdcp_xform is set, application is expected to set
> > the per packet HFN in place of IV. Driver will extract the HFN and
> > perform operations accordingly.
> >
> > Signed-off-by: Akhil Goyal <akhil.goyal@nxp.com>
> > ---
> >  lib/librte_security/rte_security.h | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/librte_security/rte_security.h
> > b/lib/librte_security/rte_security.h
> > index 96806e3a2..4452545fe 100644
> > --- a/lib/librte_security/rte_security.h
> > +++ b/lib/librte_security/rte_security.h
> > @@ -1,5 +1,5 @@
> >  /* SPDX-License-Identifier: BSD-3-Clause
> > - * Copyright 2017 NXP.
> > + * Copyright 2017,2019 NXP
> >   * Copyright(c) 2017 Intel Corporation.
> >   */
> >
> > @@ -270,6 +270,8 @@ struct rte_security_pdcp_xform {
> >  	uint32_t hfn;
> >  	/** HFN Threshold for key renegotiation */
> >  	uint32_t hfn_threshold;
> > +	/** Enable per packet HFN override */
> > +	uint32_t hfn_ovrd;

[Anoob] I think you should document the fact that IV field will be used for HFN. Your patch description accurately describes the procedure but the above comment fails to capture it. Also I would suggest renaming "hfn_ovrd" to something else to make it obvious that IV field is being used. Something like, use_iv_for_hfn or something. 

Otherwise, I don't see any issues with the approach.
 
> >  };
> >
> >  /**
> > --
> > 2.17.1