Re: [dpdk-dev] [PATCH 2/7] security: modify PDCP xform to support SDAP

["Coyle, David" <david.coyle@intel.com>]

Hi Akhil

> -----Original Message-----
> From: akhil.goyal@nxp.com <akhil.goyal@nxp.com>

<snip>

> diff --git a/doc/guides/prog_guide/rte_security.rst
> b/doc/guides/prog_guide/rte_security.rst
> index 127da2e4f..ab535d1cd 100644
> --- a/doc/guides/prog_guide/rte_security.rst
> +++ b/doc/guides/prog_guide/rte_security.rst
> @@ -1,5 +1,5 @@

<snip>

> @@ -693,6 +693,23 @@ PDCP related configuration parameters are defined
> in ``rte_security_pdcp_xform``
>          uint32_t hfn;
>          /** HFN Threshold for key renegotiation */
>          uint32_t hfn_threshold;
> +        /** 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 is set, user is expected to set the
> +         * per packet HFN in place of IV. PMDs will extract the HFN
> +         * and perform operations accordingly.
> +         */
> +         uint8_t hfn_ovrd;
> +         /** In case of 5G NR, a new protocol(SDAP) header may be set
> +          * inside PDCP payload which should be authenticated but not
> +          * encrypted. Hence, driver should be notified if SDAP is
> +          * enabled or not, so that SDAP header is not encrypted.
> +          */
> +         uint8_t sdap_enabled;
> +         /** Reserved for future */
> +         uint16_t reserved;
>      };

[DC] Should we consider removing the API code out of the security documentation?
It's a direct copy of the API code itself, and just means 2 files need to be updated for every API change.
And as with 'hfn_ovrd', sometimes it's forgotten.
From maintainability point of view, it might be better just remove it.

> 
>  DOCSIS related configuration parameters are defined in
> ``rte_security_docsis_xform`` diff --git a/lib/librte_security/rte_security.h
> b/lib/librte_security/rte_security.h
> index 16839e539..48b377b20 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,2019 NXP
> + * Copyright 2017,2019-2020 NXP
>   * Copyright(c) 2017-2020 Intel Corporation.
>   */
> 
> @@ -290,7 +290,15 @@ struct rte_security_pdcp_xform {
>  	 * per packet HFN in place of IV. PMDs will extract the HFN
>  	 * and perform operations accordingly.
>  	 */
> -	uint32_t hfn_ovrd;
> +	uint8_t hfn_ovrd;
> +	/** In case of 5G NR, a new protocol(SDAP) header may be set

[DC] Very minor thing... add space between 'protocol' and '(SDAP)' in the comment block.

And same comment for the documentation if you choose to keep the API code blocks there too.

> +	 * inside PDCP payload which should be authenticated but not
> +	 * encrypted. Hence, driver should be notified if SDAP is
> +	 * enabled or not, so that SDAP header is not encrypted.
> +	 */
> +	uint8_t sdap_enabled;
> +	/** Reserved for future */
> +	uint16_t reserved;
>  };
> 
>  /** DOCSIS direction */
> --
> 2.17.1