Hi Akhil,
Please see inline.
Thanks,
Anoob
> -----Original Message-----
> From: Akhil Goyal <gak...@marvell.com>
> Sent: Thursday, May 18, 2023 5:36 PM
> To: Anoob Joseph <ano...@marvell.com>; Thomas Monjalon
> <tho...@monjalon.net>; Jerin Jacob Kollanukkaran <jer...@marvell.com>;
> Konstantin Ananyev <konstantin.v.anan...@yandex.ru>; Bernard
> Iremonger <bernard.iremon...@intel.com>
> Cc: Hemant Agrawal <hemant.agra...@nxp.com>; Mattias Rönnblom
> <mattias.ronnb...@ericsson.com>; Kiran Kumar Kokkilagadda
> <kirankum...@marvell.com>; Volodymyr Fialko <vfia...@marvell.com>;
> dev@dpdk.org; Olivier Matz <olivier.m...@6wind.com>
> Subject: RE: [PATCH v2 09/22] app/test: add lib pdcp tests
>
> > > > diff --git a/app/test/meson.build b/app/test/meson.build index
> > > > 52d9088578..0f658aa2ab 100644
> > > > --- a/app/test/meson.build
> > > > +++ b/app/test/meson.build
> > > > @@ -96,6 +96,7 @@ test_sources = files(
> > > > 'test_meter.c',
> > > > 'test_mcslock.c',
> > > > 'test_mp_secondary.c',
> > > > + 'test_pdcp.c',
> > > > 'test_per_lcore.c',
> > > > 'test_pflock.c',
> > > > 'test_pmd_perf.c',
> > > > diff --git a/app/test/test_cryptodev.h b/app/test/test_cryptodev.h
> > > > index abd795f54a..89057dba22 100644
> > > > --- a/app/test/test_cryptodev.h
> > > > +++ b/app/test/test_cryptodev.h
> > > > @@ -4,6 +4,9 @@
> > > > #ifndef TEST_CRYPTODEV_H_
> > > > #define TEST_CRYPTODEV_H_
> > > >
> > > > +#include <rte_crypto.h>
> > > > +#include <rte_cryptodev.h>
> > > > +
> > >
> > > Can we remove these includes from here and add in test_pdcp.c directly?
> >
> > [Anoob] Why? 'test_cryptodev.h' already has many references to
> > rte_cryptodev symbols. Not including the dependencies is not correct.
> >
> In that case, it can be a separate patch. But not in this patch.
[Anoob] Is your suggestion to push this specific change as a separate patch? I
can do that if you suggest so.
>
> > >
> > >
> > > > + if (conf->is_integrity_protected) {
> > > > + if (conf->entity.pdcp_xfrm.pkt_dir ==
> > > > RTE_SECURITY_PDCP_UPLINK) {
> > > > + conf->entity.crypto_xfrm = &conf->a_xfrm;
> > > > +
> > > > + a_xfrm.auth.op =
> > > RTE_CRYPTO_AUTH_OP_GENERATE;
> > > > + a_xfrm.next = &conf->c_xfrm;
> > > > +
> > > > + c_xfrm.cipher.op =
> > > > RTE_CRYPTO_CIPHER_OP_ENCRYPT;
> > > > + c_xfrm.next = NULL;
> > > > + } else {
> > > > + conf->entity.crypto_xfrm = &conf->c_xfrm;
> > > > +
> > > > + c_xfrm.cipher.op =
> > > > RTE_CRYPTO_CIPHER_OP_DECRYPT;
> > > > + c_xfrm.next = &conf->a_xfrm;
> > > > +
> > > > + a_xfrm.auth.op = RTE_CRYPTO_AUTH_OP_VERIFY;
> > > > + a_xfrm.next = NULL;
> > > > + }
> > > > + } else {
> > > > + conf->entity.crypto_xfrm = &conf->c_xfrm;
> > > > + c_xfrm.next = NULL;
> > > > +
> > > > + if (conf->entity.pdcp_xfrm.pkt_dir ==
> > > > RTE_SECURITY_PDCP_UPLINK)
> > > > + c_xfrm.cipher.op =
> > > > RTE_CRYPTO_CIPHER_OP_ENCRYPT;
> > > > + else
> > > > + c_xfrm.cipher.op =
> > > > RTE_CRYPTO_CIPHER_OP_DECRYPT;
> > > > + }
> > > > + /* Update xforms to match PDCP requirements */
> > > > +
> > > > + if ((c_xfrm.cipher.algo == RTE_CRYPTO_CIPHER_AES_CTR) ||
> > > > + (c_xfrm.cipher.algo == RTE_CRYPTO_CIPHER_ZUC_EEA3 ||
> > > > + (c_xfrm.cipher.algo == RTE_CRYPTO_CIPHER_SNOW3G_UEA2)))
> > > > + c_xfrm.cipher.iv.length = 16;
> > > > + else
> > > > + c_xfrm.cipher.iv.length = 0;
> > > > +
> > > > + if (conf->is_integrity_protected) {
> > > > + if (a_xfrm.auth.algo == RTE_CRYPTO_AUTH_NULL)
> > > > + a_xfrm.auth.digest_length = 0;
> > > > + else
> > > > + a_xfrm.auth.digest_length = 4;
> > >
> > > This if-else is not needed. If is_integrity_protected, digest_length
> > > should always be 4.
> >
> > [Anoob] I had explained this in v1 patch set. Will try again.
> >
> > In PDCP, with AUTH_NULL it is expected to have 4 bytes of zeroized digest.
> >
> > With AUTH_NULL, it is lib PDCP which would add zeroized digest. No PMD
> > currently supported in DPDK supports non-zero digest with AUTH-NULL.
> > Also, it is not specified what is the digest added in case of AUTH-NULL.
>
> In auth_xform, digest_length is the expected length of digest coming out of
> crypto device.
[Anoob] Yes. But the packet append is the duty lib PDCP. Crypto PMD is expected
to just write the digest at a specific location.
> Now if the device is expected to support PDCP, and we are reusing the
> crypto APIs, We can specify the digest length required for NULL auth.
> The PMDs capability can be updated accordingly.
[Anoob] Again, none of the current PMDs do a zeroized digest with NULL auth.
And it is not a requirement as well. Here, we are doing PDCP offload and the
crypto_xforms provided here are for specific crypto transformations.
> You can add a comment in the rte_crypto_auth_xform specifically for NULL
> auth for PDCP case.
>
> The reason, I am insisting on this is, for the user, while configuring
> auth_xform, it is setting digest length as 0 and when the packet is received
> the packet length is increased by digest. This will create confusion.
> And it will also help in identifying the case of no integrity and null
> integrity.
[Anoob] When working with protocol, the packet would change both at header and
trailer. It is just that for PDCP, the trailer is only digest. For IPsec, the
change is more than digest.
>
> >
> > > Also define a macro for MAC-I len. It is being used at multiple places.
> > > Similarly for IV length macro can be defined.
> > >
> >
> > [Anoob] Agreed. You want me to introduce RTE_PDCP_MAC_I_LEN or
> > something local would do?
> I am ok either way. Having defined in library would be better, to be used in
> lib and PMD as well.
>
> >
> > > > +
> > > > + if ((a_xfrm.auth.algo == RTE_CRYPTO_AUTH_ZUC_EIA3) ||
> > > > + (a_xfrm.auth.algo ==
> > > RTE_CRYPTO_AUTH_SNOW3G_UIA2))
> > > > + a_xfrm.auth.iv.length = 16;
> > > > + else
> > > > + a_xfrm.auth.iv.length = 0;
> > > > + }
> > > > +
> > > > + conf->c_xfrm = c_xfrm;
> > > > + conf->a_xfrm = a_xfrm;
> > > > +
> > > > + conf->entity.dev_id = (uint8_t)cryptodev_id_get(conf-
> > > > >is_integrity_protected,
> > > > + &conf->c_xfrm, &conf->a_xfrm);
> > > > +
> > > > + if (pdcp_test_params[index].domain ==
> > > > RTE_SECURITY_PDCP_MODE_CONTROL ||
> > > > + pdcp_test_params[index].domain ==
> > > > RTE_SECURITY_PDCP_MODE_DATA) {
> > > > + data = pdcp_test_data_in[index];
> > > > + hfn = pdcp_test_hfn[index] <<
> > > pdcp_test_data_sn_size[index];
> > > > + sn = pdcp_sn_from_raw_get(data,
> > > > pdcp_test_data_sn_size[index]);
> > > > + count = hfn | sn;
> > > > + }
> > >
> > > The above logic can go inside lib PDCP as well.
> > > This is specific to PDCP and not user dependent.
> > > You can reuse the pdcp_xfrm.hfn as well.
> > >
> >
> > [Anoob] Sorry, what exactly can go into lib PDCP? This snippet is
> > reading SN used in a test vector and constructs the count based on SN &
> HFN value from vector.
>
> This count value is being used to establish entity. I am saying, instead of
> taking Count, take sn as input and in the libpdcp combine pdcp_xfrm.hfn and
> sn as needed to create count and store in entity_priv.
> Just wanted to reduce the application headache to make bitshifting and
> ORing to SN.
[Anoob] I was not able to identify a use case where having two 32 bit values
for HFN & SN would make sense. But then, it would align better with current
rte_security specification. Will make the change in next version.
