Re: [EXT] [PATCH v5 06/12] cryptodev: add elliptic curve diffie hellman

[Ray Kinsella <mdr@ashroe.eu>]

"Kusztal, ArkadiuszX" <arkadiuszx.kusztal@intel.com> writes:

>> -----Original Message-----
>> From: Akhil Goyal <gakhil@marvell.com>
>> Sent: Thursday, June 2, 2022 4:25 PM
>> To: Ray Kinsella <mdr@ashroe.eu>
>> Cc: Kusztal, ArkadiuszX <arkadiuszx.kusztal@intel.com>; dev@dpdk.org; Zhang,
>> Roy Fan <roy.fan.zhang@intel.com>
>> Subject: RE: [EXT] [PATCH v5 06/12] cryptodev: add elliptic curve diffie hellman
>> 
>> >
>> > Akhil Goyal <gakhil@marvell.com> writes:
>> >
>> > >> - Added elliptic curve Diffie-Hellman parameters.
>> > >> Point multiplication allows the user to process every phase of
>> > >> ECDH, but for phase 1, user should not really care about the generator.
>> > >> The user does not even need to know what the generator looks like,
>> > >> therefore setting ec xform would make this work.
>> > >>
>> > >> Signed-off-by: Arek Kusztal <arkadiuszx.kusztal@intel.com>
>> > >> ---
>> > >>  devtools/libabigail.abignore               |  3 +++
>> > >>  doc/guides/cryptodevs/features/default.ini |  1 +
>> > >>  doc/guides/prog_guide/cryptodev_lib.rst    |  7 +++---
>> > >>  doc/guides/rel_notes/release_22_07.rst     |  2 ++
>> > >>  lib/cryptodev/rte_crypto_asym.h            | 38
>> > >> ++++++++++++++++++++++++++++++
>> > >>  5 files changed, 48 insertions(+), 3 deletions(-)
>> > >>
>> > >> diff --git a/devtools/libabigail.abignore
>> > >> b/devtools/libabigail.abignore index 79ff15dc4e..6d174b291f 100644
>> > >> --- a/devtools/libabigail.abignore
>> > >> +++ b/devtools/libabigail.abignore
>> > >> @@ -27,6 +27,9 @@
>> > >>  ; Ignore changes to rte_crypto_asym_op, asymmetric crypto API is
>> > >> experimental  [suppress_type]
>> > >>          name = rte_crypto_asym_op
>> > >> +; Ignore changes to rte_crypto_asym_xform_type, asymmetric crypto
>> > >> +API is
>> > >> experimental
>> > >> +[suppress_type]
>> > >> +        name = rte_crypto_asym_xform_type
>> > >>
>> > > This exception does not seem to work.
>> > > Thomas and I are getting ABI issues even with this suppress rule.
>> > >
>> > >   [C] 'function void rte_cryptodev_info_get(uint8_t,
>> > > rte_cryptodev_info*)' at
>> > rte_cryptodev.c:1582:1 has some indirect sub-type changes:
>> > >     parameter 2 of type 'rte_cryptodev_info*' has sub-type changes:
>> > >       in pointed to type 'struct rte_cryptodev_info' at rte_cryptodev.h:503:1:
>> > >         type size hasn't changed
>> > >         1 data member change:
>> > >           type of 'const rte_cryptodev_capabilities* capabilities' changed:
>> > >             in pointed to type 'const rte_cryptodev_capabilities':
>> > >               in unqualified underlying type 'struct
>> > > rte_cryptodev_capabilities' at
>> > rte_cryptodev.h:198:1:
>> > >                 type size hasn't changed
>> > >                 1 data member change:
>> > >                   type of 'anonymous data member union
>> > {rte_cryptodev_symmetric_capability sym;
>> > rte_cryptodev_asymmetric_capability asym;}' changed:
>> > >                     type size hasn't changed
>> > >                     1 data member change:
>> > >                       type of 'rte_cryptodev_asymmetric_capability asym' changed:
>> > >                         type size hasn't changed
>> > >                         1 data member change:
>> > >                           type of
>> > > 'rte_cryptodev_asymmetric_xform_capability
>> > xform_capa' changed:
>> > >                             type size hasn't changed
>> > >                             1 data member change:
>> > >                               type of 'rte_crypto_asym_xform_type xform_type' changed:
>> > >                                 type size hasn't changed
>> > >                                 1 enumerator insertion:
>> > >
>> > 'rte_crypto_asym_xform_type::RTE_CRYPTO_ASYM_XFORM_ECDH' value '8'
>> > >                                 2 enumerator changes:
>> > >
>> > 'rte_crypto_asym_xform_type::RTE_CRYPTO_ASYM_XFORM_ECPM' from
>> value
>> > '8' to '9' at rte_crypto_asym.h:80:1
>> > >
>> > 'rte_crypto_asym_xform_type::RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END'
>> > from value '9' to '10' at rte_crypto_asym.h:80:1
>> >
>> > Ok - will take a look see.
>> Thomas has fixed the suppress rule while pulling on main.
>
> For some unknown reason both:
> [suppress_type]
>         type_kind = enum
>         changed_enumerators = RTE_CRYPTO_ASYM_XFORM_ECPM, RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END
> and
> [suppress_type]
>        name = rte_crypto_asym_xform_type
>
> works fine for me with check-abi tool:
> {DPDK_NEW}/devtools/check-abi.sh ${DPDK_REF}/build/ref ${DPDK_NEW}/build/new
>
> Otherwise I get:
>
> 1 function with some indirect sub-type change:
>
>   [C]'function void rte_cryptodev_info_get(uint8_t, rte_cryptodev_info*)' at rte_cryptodev.c:1582:1 has some indirect sub-type changes:
>     parameter 2 of type 'rte_cryptodev_info*' has sub-type changes:	
>
> abidiff: 1.2.0 version

So like you with 1.2.0 I didn't see the error, with 2.0.0 I see the
following, so I recommend that you update to 2.0.0 or later. I had
inadvertently reverted versions after a system rebuild.

ninja: Entering directory `./build-gcc-static'
ninja: no work to do.
ninja: Entering directory `./build-gcc-shared'
ninja: no work to do.
Error: ABI issue reported for 'abidiff --suppr /root/src/dpdk/devtools/../devtools/libabigail.abignore --no-added-syms --headers-dir1 /root/src/dpdk/build_ref/v21.11/build-gcc-shared/usr/local/include --headers-dir2 /root/src/dpdk/build-gcc-shared/install/usr/local/include /root/src/dpdk/build_ref/v21.11/build-gcc-shared/dump/librte_cryptodev.dump /root/src/dpdk/build-gcc-shared/install/dump/librte_cryptodev.dump'
ABIDIFF_ABI_CHANGE, this change requires a review (abidiff flagged this as a potential issue).
Error: ABI issue reported for 'abidiff --suppr /root/src/dpdk/devtools/../devtools/libabigail.abignore --no-added-syms --headers-dir1 /root/src/dpdk/build_ref/v21.11/build-gcc-shared/usr/local/include --headers-dir2 /root/src/dpdk/build-gcc-shared/install/usr/local/include /root/src/dpdk/build_ref/v21.11/build-gcc-shared/dump/librte_security.dump /root/src/dpdk/build-gcc-shared/install/dump/librte_security.dump'
ABIDIFF_ABI_CHANGE, this change requires a review (abidiff flagged this as a potential issue).

Me thinks, we need to add a check for libabigail version to the build
scripts.

That all said, Thomas's supression rule has better percision than the
original proposed. 


> @Akhil - additionally RTE_CRYPTO_ASYM_XFORM_ECDH was supposed to be placed at the end of the enum. My mistake, I have placed it incorrectly.
> Would a fix that swaps these enums be accepted? This could potentially remove changed_enumerators = RTE_CRYPTO_ASYM_XFORM_ECPM.



-- 
Regards, Ray K