[PATCH] cryptodev: add missing algorithm strings

[PATCH] cryptodev: add missing algorithm strings

[Volodymyr Fialko]

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"
-- 
2.25.1

RE: [PATCH] cryptodev: add missing algorithm strings

[Akhil Goyal]

> 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.

Re: [PATCH] cryptodev: add missing algorithm strings

[Kevin Traynor]

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'

RE: [EXT] Re: [PATCH] cryptodev: add missing algorithm strings

[Akhil Goyal]

> 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'

Re: [EXT] Re: [PATCH] cryptodev: add missing algorithm strings

[David Marchand]

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.


-- 
David Marchand

RE: [EXT] Re: [PATCH] cryptodev: add missing algorithm strings

[Akhil Goyal]

> 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.