diff mbox series

cryptodev: add missing algorithm strings

Message ID 20220915082635.418873-1-vfialko@marvell.com (mailing list archive)
State Accepted, archived
Delegated to: akhil goyal
Headers
Series cryptodev: add missing algorithm strings |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-aarch64-unit-testing success Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/Intel-compilation fail Compilation issues
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/github-robot: build success github build: passed
ci/intel-Testing success Testing PASS

Commit Message

Volodymyr Fialko Sept. 15, 2022, 8:26 a.m. UTC 
  SHA3 family algorithms were missing in the array of algorithm strings.

Fixes: 1df800f89518 ("crypto/ccp: support SHA3 family")

Signed-off-by: Volodymyr Fialko <vfialko@marvell.com>
---
 lib/cryptodev/rte_cryptodev.c | 9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Akhil Goyal Sept. 22, 2022, 4:01 p.m. UTC | #1 
> Subject: [PATCH] cryptodev: add missing algorithm strings
> 
> SHA3 family algorithms were missing in the array of algorithm strings.
> 
> Fixes: 1df800f89518 ("crypto/ccp: support SHA3 family")
> 
> Signed-off-by: Volodymyr Fialko <vfialko@marvell.com>
> ---
Acked-by: Akhil Goyal <gakhil@marvell.com>

Applied to dpdk-next-crypto
Thanks.
Kevin Traynor Nov. 2, 2022, 10:50 a.m. UTC | #2 
On 15/09/2022 09:26, Volodymyr Fialko wrote:
> SHA3 family algorithms were missing in the array of algorithm strings.
> 
> Fixes: 1df800f89518 ("crypto/ccp: support SHA3 family")
> 
> Signed-off-by: Volodymyr Fialko <vfialko@marvell.com>
> ---
>   lib/cryptodev/rte_cryptodev.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/lib/cryptodev/rte_cryptodev.c b/lib/cryptodev/rte_cryptodev.c
> index 42f3221052..35661f5347 100644
> --- a/lib/cryptodev/rte_cryptodev.c
> +++ b/lib/cryptodev/rte_cryptodev.c
> @@ -130,6 +130,15 @@ rte_crypto_auth_algorithm_strings[] = {
>   	[RTE_CRYPTO_AUTH_SHA512]	= "sha2-512",
>   	[RTE_CRYPTO_AUTH_SHA512_HMAC]	= "sha2-512-hmac",
>   
> +	[RTE_CRYPTO_AUTH_SHA3_224]	= "sha3-224",
> +	[RTE_CRYPTO_AUTH_SHA3_224_HMAC] = "sha3-224-hmac",
> +	[RTE_CRYPTO_AUTH_SHA3_256]	= "sha3-256",
> +	[RTE_CRYPTO_AUTH_SHA3_256_HMAC] = "sha3-256-hmac",
> +	[RTE_CRYPTO_AUTH_SHA3_384]	= "sha3-384",
> +	[RTE_CRYPTO_AUTH_SHA3_384_HMAC] = "sha3-384-hmac",
> +	[RTE_CRYPTO_AUTH_SHA3_512]	= "sha3-512",
> +	[RTE_CRYPTO_AUTH_SHA3_512_HMAC]	= "sha3-512-hmac",
> +
>   	[RTE_CRYPTO_AUTH_KASUMI_F9]	= "kasumi-f9",
>   	[RTE_CRYPTO_AUTH_SNOW3G_UIA2]	= "snow3g-uia2",
>   	[RTE_CRYPTO_AUTH_ZUC_EIA3]	= "zuc-eia3"

This is being flagged as an ABI break for 21.11.3 [1]. I don't see it 
mentioned in the commit message or discussed, is it ok for main branch?

Thanks to Ali for reporting. I will revert on 21.11 branch.

[1]
1 Changed variable:

   [C] 'const char* rte_crypto_auth_algorithm_strings[]' was changed at 
rte_crypto_sym.h:372:1:
     size of symbol changed from 168 to 232

Error: ABI issue reported for 'abidiff --suppr 
dpdk/devtools/libabigail.abignore --no-added-syms --headers-dir1 
/opt/dpdklab/abi_references/v21.11/armgigabyteref/include --headers-dir2 
build_install/include 
/opt/dpdklab/abi_references/v21.11/armgigabyteref/dump/librte_cryptodev.dump 
build_install/dump/librte_cryptodev.dump'
Akhil Goyal Nov. 2, 2022, 10:58 a.m. UTC | #3 
> On 15/09/2022 09:26, Volodymyr Fialko wrote:
> > SHA3 family algorithms were missing in the array of algorithm strings.
> >
> > Fixes: 1df800f89518 ("crypto/ccp: support SHA3 family")
> >
> > Signed-off-by: Volodymyr Fialko <vfialko@marvell.com>
> > ---
> >   lib/cryptodev/rte_cryptodev.c | 9 +++++++++
> >   1 file changed, 9 insertions(+)
> >
> > diff --git a/lib/cryptodev/rte_cryptodev.c b/lib/cryptodev/rte_cryptodev.c
> > index 42f3221052..35661f5347 100644
> > --- a/lib/cryptodev/rte_cryptodev.c
> > +++ b/lib/cryptodev/rte_cryptodev.c
> > @@ -130,6 +130,15 @@ rte_crypto_auth_algorithm_strings[] = {
> >   	[RTE_CRYPTO_AUTH_SHA512]	= "sha2-512",
> >   	[RTE_CRYPTO_AUTH_SHA512_HMAC]	= "sha2-512-hmac",
> >
> > +	[RTE_CRYPTO_AUTH_SHA3_224]	= "sha3-224",
> > +	[RTE_CRYPTO_AUTH_SHA3_224_HMAC] = "sha3-224-hmac",
> > +	[RTE_CRYPTO_AUTH_SHA3_256]	= "sha3-256",
> > +	[RTE_CRYPTO_AUTH_SHA3_256_HMAC] = "sha3-256-hmac",
> > +	[RTE_CRYPTO_AUTH_SHA3_384]	= "sha3-384",
> > +	[RTE_CRYPTO_AUTH_SHA3_384_HMAC] = "sha3-384-hmac",
> > +	[RTE_CRYPTO_AUTH_SHA3_512]	= "sha3-512",
> > +	[RTE_CRYPTO_AUTH_SHA3_512_HMAC]	= "sha3-512-hmac",
> > +
> >   	[RTE_CRYPTO_AUTH_KASUMI_F9]	= "kasumi-f9",
> >   	[RTE_CRYPTO_AUTH_SNOW3G_UIA2]	= "snow3g-uia2",
> >   	[RTE_CRYPTO_AUTH_ZUC_EIA3]	= "zuc-eia3"
> 
> This is being flagged as an ABI break for 21.11.3 [1]. I don't see it
> mentioned in the commit message or discussed, is it ok for main branch?

Ok, we can keep it to main only.
But it will be an issue on 21.11. 

> 
> Thanks to Ali for reporting. I will revert on 21.11 branch.
> 
> [1]
> 1 Changed variable:
> 
>    [C] 'const char* rte_crypto_auth_algorithm_strings[]' was changed at
> rte_crypto_sym.h:372:1:
>      size of symbol changed from 168 to 232
> 
> Error: ABI issue reported for 'abidiff --suppr
> dpdk/devtools/libabigail.abignore --no-added-syms --headers-dir1
> /opt/dpdklab/abi_references/v21.11/armgigabyteref/include --headers-dir2
> build_install/include
> /opt/dpdklab/abi_references/v21.11/armgigabyteref/dump/librte_cryptodev.du
> mp
> build_install/dump/librte_cryptodev.dump'
David Marchand Nov. 2, 2022, 12:13 p.m. UTC | #4 
On Wed, Nov 2, 2022 at 11:58 AM Akhil Goyal <gakhil@marvell.com> wrote:
> > This is being flagged as an ABI break for 21.11.3 [1]. I don't see it
> > mentioned in the commit message or discussed, is it ok for main branch?
>
> Ok, we can keep it to main only.
> But it will be an issue on 21.11.
>
> >
> > Thanks to Ali for reporting. I will revert on 21.11 branch.
> >
> > [1]
> > 1 Changed variable:
> >
> >    [C] 'const char* rte_crypto_auth_algorithm_strings[]' was changed at
> > rte_crypto_sym.h:372:1:
> >      size of symbol changed from 168 to 232

My two cents.

We have a algo "string to num" helper (rte_cryptodev_get_auth_algo_enum).

This code is not performance sensitive, is it?
If we add the, opposite, "num to string" helper, we can hide the
rte_crypto_auth_algorithm_strings symbol from the public ABI and avoid
this kind of issues in the future.

And looking at lib/crypto map, there are other arrays (*_strings
symbols) that are subject to similar "extending" issues.

We are late in the release for adding new API though such helpers
would be really simple.
Hiding such symbols is something to consider before entering ABI freeze.
Akhil Goyal Nov. 2, 2022, 12:31 p.m. UTC | #5 
> Subject: Re: [EXT] Re: [PATCH] cryptodev: add missing algorithm strings
> 
> On Wed, Nov 2, 2022 at 11:58 AM Akhil Goyal <gakhil@marvell.com> wrote:
> > > This is being flagged as an ABI break for 21.11.3 [1]. I don't see it
> > > mentioned in the commit message or discussed, is it ok for main branch?
> >
> > Ok, we can keep it to main only.
> > But it will be an issue on 21.11.
> >
> > >
> > > Thanks to Ali for reporting. I will revert on 21.11 branch.
> > >
> > > [1]
> > > 1 Changed variable:
> > >
> > >    [C] 'const char* rte_crypto_auth_algorithm_strings[]' was changed at
> > > rte_crypto_sym.h:372:1:
> > >      size of symbol changed from 168 to 232
> 
> My two cents.
> 
> We have a algo "string to num" helper (rte_cryptodev_get_auth_algo_enum).
> 
> This code is not performance sensitive, is it?
> If we add the, opposite, "num to string" helper, we can hide the
> rte_crypto_auth_algorithm_strings symbol from the public ABI and avoid
> this kind of issues in the future.
> 
> And looking at lib/crypto map, there are other arrays (*_strings
> symbols) that are subject to similar "extending" issues.
> 
> We are late in the release for adding new API though such helpers
> would be really simple.
> Hiding such symbols is something to consider before entering ABI freeze.
> 
Agreed, but it is quite late for this cycle.
We can plan these helper APIs for next release cycle and remove these symbols in 23.11.
diff mbox series

Patch

diff --git a/lib/cryptodev/rte_cryptodev.c b/lib/cryptodev/rte_cryptodev.c
index 42f3221052..35661f5347 100644
--- a/lib/cryptodev/rte_cryptodev.c
+++ b/lib/cryptodev/rte_cryptodev.c
@@ -130,6 +130,15 @@  rte_crypto_auth_algorithm_strings[] = {
 	[RTE_CRYPTO_AUTH_SHA512]	= "sha2-512",
 	[RTE_CRYPTO_AUTH_SHA512_HMAC]	= "sha2-512-hmac",
 
+	[RTE_CRYPTO_AUTH_SHA3_224]	= "sha3-224",
+	[RTE_CRYPTO_AUTH_SHA3_224_HMAC] = "sha3-224-hmac",
+	[RTE_CRYPTO_AUTH_SHA3_256]	= "sha3-256",
+	[RTE_CRYPTO_AUTH_SHA3_256_HMAC] = "sha3-256-hmac",
+	[RTE_CRYPTO_AUTH_SHA3_384]	= "sha3-384",
+	[RTE_CRYPTO_AUTH_SHA3_384_HMAC] = "sha3-384-hmac",
+	[RTE_CRYPTO_AUTH_SHA3_512]	= "sha3-512",
+	[RTE_CRYPTO_AUTH_SHA3_512_HMAC]	= "sha3-512-hmac",
+
 	[RTE_CRYPTO_AUTH_KASUMI_F9]	= "kasumi-f9",
 	[RTE_CRYPTO_AUTH_SNOW3G_UIA2]	= "snow3g-uia2",
 	[RTE_CRYPTO_AUTH_ZUC_EIA3]	= "zuc-eia3"