[PATCH] [RFC] cryptodev: replace LIST_END enumerators with APIs

[PATCH] [RFC] cryptodev: replace LIST_END enumerators with APIs

[Akhil Goyal]

Replace *_LIST_END enumerators from asymmetric crypto
lib to avoid ABI breakage for every new addition in
enums with inline APIs.

Signed-off-by: Akhil Goyal <gakhil@marvell.com>
---
This patch was discussed in ML long time back.
https://patches.dpdk.org/project/dpdk/patch/20211008204516.3497060-1-gakhil@marvell.com/
Now added inline APIs for getting the list end which need to be updated
for each new entry to the enum. This shall help in avoiding ABI break
for adding new algo.

 app/test/test_cryptodev_asym.c  |  2 +-
 lib/cryptodev/rte_crypto_asym.h | 18 ++++++++++++++----
 2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/app/test/test_cryptodev_asym.c b/app/test/test_cryptodev_asym.c
index f0b5d38543..f1ece475b8 100644
--- a/app/test/test_cryptodev_asym.c
+++ b/app/test/test_cryptodev_asym.c
@@ -581,7 +581,7 @@ static inline void print_asym_capa(
 			rte_cryptodev_asym_get_xform_string(capa->xform_type));
 	printf("operation supported -");
 
-	for (i = 0; i < RTE_CRYPTO_ASYM_OP_LIST_END; i++) {
+	for (i = 0; i < rte_crypto_asym_op_list_end(); i++) {
 		/* check supported operations */
 		if (rte_cryptodev_asym_xform_capability_check_optype(capa, i)) {
 			if (capa->xform_type == RTE_CRYPTO_ASYM_XFORM_DH)
diff --git a/lib/cryptodev/rte_crypto_asym.h b/lib/cryptodev/rte_crypto_asym.h
index 39d3da3952..290b300f84 100644
--- a/lib/cryptodev/rte_crypto_asym.h
+++ b/lib/cryptodev/rte_crypto_asym.h
@@ -72,6 +72,7 @@ enum rte_crypto_curve_id {
  * Asymmetric crypto transformation types.
  * Each xform type maps to one asymmetric algorithm
  * performing specific operation
+ * Note: Update rte_crypto_asym_xform_type_list_end() for every new type added.
  */
 enum rte_crypto_asym_xform_type {
 	RTE_CRYPTO_ASYM_XFORM_UNSPECIFIED = 0,
@@ -119,12 +120,17 @@ enum rte_crypto_asym_xform_type {
 	 * Performs Encrypt, Decrypt, Sign and Verify.
 	 * Refer to rte_crypto_asym_op_type.
 	 */
-	RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END
-	/**< End of list */
 };
 
+static inline int
+rte_crypto_asym_xform_type_list_end(void)
+{
+	return RTE_CRYPTO_ASYM_XFORM_SM2 + 1;
+}
+
 /**
  * Asymmetric crypto operation type variants
+ * Note: Update rte_crypto_asym_op_list_end for every new type added.
  */
 enum rte_crypto_asym_op_type {
 	RTE_CRYPTO_ASYM_OP_ENCRYPT,
@@ -135,9 +141,14 @@ enum rte_crypto_asym_op_type {
 	/**< Signature Generation operation */
 	RTE_CRYPTO_ASYM_OP_VERIFY,
 	/**< Signature Verification operation */
-	RTE_CRYPTO_ASYM_OP_LIST_END
 };
 
+static inline int
+rte_crypto_asym_op_list_end(void)
+{
+	return RTE_CRYPTO_ASYM_OP_VERIFY + 1;
+}
+
 /**
  * Asymmetric crypto key exchange operation type
  */
@@ -168,7 +179,6 @@ enum rte_crypto_rsa_padding_type {
 	/**< RSA PKCS#1 OAEP padding scheme */
 	RTE_CRYPTO_RSA_PADDING_PSS,
 	/**< RSA PKCS#1 PSS padding scheme */
-	RTE_CRYPTO_RSA_PADDING_TYPE_LIST_END
 };
 
 /**
-- 
2.25.1

RE: [PATCH] [RFC] cryptodev: replace LIST_END enumerators with APIs

[Morten Brørup]

> +++ b/app/test/test_cryptodev_asym.c
> @@ -581,7 +581,7 @@ static inline void print_asym_capa(
>  			rte_cryptodev_asym_get_xform_string(capa->xform_type));
>  	printf("operation supported -");
> 
> -	for (i = 0; i < RTE_CRYPTO_ASYM_OP_LIST_END; i++) {
> +	for (i = 0; i < rte_crypto_asym_op_list_end(); i++) {

> +++ b/lib/cryptodev/rte_crypto_asym.h
> +static inline int
> +rte_crypto_asym_xform_type_list_end(void)
> +{
> +	return RTE_CRYPTO_ASYM_XFORM_SM2 + 1;
> +}
> +
>  /**
>   * Asymmetric crypto operation type variants
> + * Note: Update rte_crypto_asym_op_list_end for every new type added.
>   */
>  enum rte_crypto_asym_op_type {
>  	RTE_CRYPTO_ASYM_OP_ENCRYPT,
> @@ -135,9 +141,14 @@ enum rte_crypto_asym_op_type {
>  	/**< Signature Generation operation */
>  	RTE_CRYPTO_ASYM_OP_VERIFY,
>  	/**< Signature Verification operation */
> -	RTE_CRYPTO_ASYM_OP_LIST_END
>  };
>
> +static inline int
> +rte_crypto_asym_op_list_end(void)
> +{
> +	return RTE_CRYPTO_ASYM_OP_VERIFY + 1;
> +}

I like the concept of replacing an "last enum value" with a "last enum function" for API/ABI compatibility purposes.

Here's an idea...

We can introduce a generic design pattern where we keep the _LIST_END enum value at the end, somehow marking it private (and not part of the API/ABI), and move the _list_end() function inside the C file, so it uses the _LIST_END enum value that the library was built with. E.g. like this:


In the header file:

enum rte_crypto_asym_op_type {
	RTE_CRYPTO_ASYM_OP_VERIFY,
	/**< Signature Verification operation */
#if RTE_BUILDING_INTERNAL
	__RTE_CRYPTO_ASYM_OP_LIST_END /* internal */
#endif
}

int rte_crypto_asym_op_list_end(void);


And in the associated library code file, when including rte_crypto_asym.h:

#define RTE_BUILDING_INTERNAL
#include <cryptodev/rte_crypto_asym.h>

int
rte_crypto_asym_op_list_end(void)
{
	return __RTE_CRYPTO_ASYM_OP_LIST_END;
}

RE: [PATCH] [RFC] cryptodev: replace LIST_END enumerators with APIs

[Kusztal, ArkadiuszX]

> -----Original Message-----
> From: Morten Brørup <mb@smartsharesystems.com>
> Sent: Thursday, September 5, 2024 5:09 PM
> To: Akhil Goyal <gakhil@marvell.com>; dev@dpdk.org
> Cc: thomas@monjalon.net; Marchand, David <david.marchand@redhat.com>;
> hemant.agrawal@nxp.com; anoobj@marvell.com; De Lara Guarch, Pablo
> <pablo.de.lara.guarch@intel.com>; Trahe, Fiona <fiona.trahe@intel.com>;
> Doherty, Declan <declan.doherty@intel.com>; matan@nvidia.com;
> g.singh@nxp.com; fanzhang.oss@gmail.com; jianjay.zhou@huawei.com;
> asomalap@amd.com; ruifeng.wang@arm.com;
> konstantin.v.ananyev@yandex.ru; Nicolau, Radu <radu.nicolau@intel.com>;
> ajit.khaparde@broadcom.com; rnagadheeraj@marvell.com; mdr@ashroe.eu
> Subject: RE: [PATCH] [RFC] cryptodev: replace LIST_END enumerators with APIs
> 
> > +++ b/app/test/test_cryptodev_asym.c
> > @@ -581,7 +581,7 @@ static inline void print_asym_capa(
> >  			rte_cryptodev_asym_get_xform_string(capa-
> >xform_type));
> >  	printf("operation supported -");
> >
> > -	for (i = 0; i < RTE_CRYPTO_ASYM_OP_LIST_END; i++) {
> > +	for (i = 0; i < rte_crypto_asym_op_list_end(); i++) {
> 
> > +++ b/lib/cryptodev/rte_crypto_asym.h
> > +static inline int
> > +rte_crypto_asym_xform_type_list_end(void)
> > +{
> > +	return RTE_CRYPTO_ASYM_XFORM_SM2 + 1; }
> > +
> >  /**
> >   * Asymmetric crypto operation type variants
> > + * Note: Update rte_crypto_asym_op_list_end for every new type added.
> >   */
> >  enum rte_crypto_asym_op_type {
> >  	RTE_CRYPTO_ASYM_OP_ENCRYPT,
> > @@ -135,9 +141,14 @@ enum rte_crypto_asym_op_type {
> >  	/**< Signature Generation operation */
> >  	RTE_CRYPTO_ASYM_OP_VERIFY,
> >  	/**< Signature Verification operation */
> > -	RTE_CRYPTO_ASYM_OP_LIST_END
> >  };
> >
> > +static inline int
> > +rte_crypto_asym_op_list_end(void)
> > +{
> > +	return RTE_CRYPTO_ASYM_OP_VERIFY + 1; }
> 
> I like the concept of replacing an "last enum value" with a "last enum function"
> for API/ABI compatibility purposes.
> 
> Here's an idea...
> 
> We can introduce a generic design pattern where we keep the _LIST_END enum
> value at the end, somehow marking it private (and not part of the API/ABI), and
> move the _list_end() function inside the C file, so it uses the _LIST_END enum
> value that the library was built with. E.g. like this:
> 

Why asym crypto API does need these ENDs at all? Will any PMD or the user ever or use it? Sym crypto does not have this at all, as well as rte_crypto_asym_ke_type in asym crypto.


> 
> In the header file:
> 
> enum rte_crypto_asym_op_type {
> 	RTE_CRYPTO_ASYM_OP_VERIFY,
> 	/**< Signature Verification operation */ #if RTE_BUILDING_INTERNAL
> 	__RTE_CRYPTO_ASYM_OP_LIST_END /* internal */ #endif }
> 
> int rte_crypto_asym_op_list_end(void);
> 
> 
> And in the associated library code file, when including rte_crypto_asym.h:
> 
> #define RTE_BUILDING_INTERNAL
> #include <cryptodev/rte_crypto_asym.h>
> 
> int
> rte_crypto_asym_op_list_end(void)
> {
> 	return __RTE_CRYPTO_ASYM_OP_LIST_END;
> }

Re: [PATCH] [RFC] cryptodev: replace LIST_END enumerators with APIs

[fengchengwen]

On 2024/9/5 23:09, Morten Brørup wrote:
>> +++ b/app/test/test_cryptodev_asym.c
>> @@ -581,7 +581,7 @@ static inline void print_asym_capa(
>>  			rte_cryptodev_asym_get_xform_string(capa->xform_type));
>>  	printf("operation supported -");
>>
>> -	for (i = 0; i < RTE_CRYPTO_ASYM_OP_LIST_END; i++) {
>> +	for (i = 0; i < rte_crypto_asym_op_list_end(); i++) {
> 
>> +++ b/lib/cryptodev/rte_crypto_asym.h
>> +static inline int
>> +rte_crypto_asym_xform_type_list_end(void)
>> +{
>> +	return RTE_CRYPTO_ASYM_XFORM_SM2 + 1;
>> +}
>> +
>>  /**
>>   * Asymmetric crypto operation type variants
>> + * Note: Update rte_crypto_asym_op_list_end for every new type added.
>>   */
>>  enum rte_crypto_asym_op_type {
>>  	RTE_CRYPTO_ASYM_OP_ENCRYPT,
>> @@ -135,9 +141,14 @@ enum rte_crypto_asym_op_type {
>>  	/**< Signature Generation operation */
>>  	RTE_CRYPTO_ASYM_OP_VERIFY,
>>  	/**< Signature Verification operation */
>> -	RTE_CRYPTO_ASYM_OP_LIST_END
>>  };
>>
>> +static inline int
>> +rte_crypto_asym_op_list_end(void)
>> +{
>> +	return RTE_CRYPTO_ASYM_OP_VERIFY + 1;
>> +}
> 
> I like the concept of replacing an "last enum value" with a "last enum function" for API/ABI compatibility purposes.

+1
There are many such define in DPDK, e.g. RTE_ETH_EVENT_MAX

> 
> Here's an idea...
> 
> We can introduce a generic design pattern where we keep the _LIST_END enum value at the end, somehow marking it private (and not part of the API/ABI), and move the _list_end() function inside the C file, so it uses the _LIST_END enum value that the library was built with. E.g. like this:
> 
> 
> In the header file:
> 
> enum rte_crypto_asym_op_type {
> 	RTE_CRYPTO_ASYM_OP_VERIFY,
> 	/**< Signature Verification operation */
> #if RTE_BUILDING_INTERNAL
> 	__RTE_CRYPTO_ASYM_OP_LIST_END /* internal */
> #endif
> }
> 
> int rte_crypto_asym_op_list_end(void);
> 
> 
> And in the associated library code file, when including rte_crypto_asym.h:
> 
> #define RTE_BUILDING_INTERNAL
> #include <cryptodev/rte_crypto_asym.h>
> 
> int
> rte_crypto_asym_op_list_end(void)
> {
> 	return __RTE_CRYPTO_ASYM_OP_LIST_END;
> }

It's more generic, and also keep LIST_END in the define, we just add new enum before it.
But based on my understanding of ABI compatibility, from the point view of application,
this API should return old-value even with the new library, but it will return new-value
with new library. It could also break ABI.

So this API should force inline, just as this patch did. But it seem can't work if move
this API to header file and add static inline.

> 
> .
> 

RE: [EXTERNAL] Re: [PATCH] [RFC] cryptodev: replace LIST_END enumerators with APIs

[Akhil Goyal]

> >
> > Here's an idea...
> >
> > We can introduce a generic design pattern where we keep the _LIST_END enum
> value at the end, somehow marking it private (and not part of the API/ABI), and
> move the _list_end() function inside the C file, so it uses the _LIST_END enum
> value that the library was built with. E.g. like this:
> >
> >
> > In the header file:
> >
> > enum rte_crypto_asym_op_type {
> > 	RTE_CRYPTO_ASYM_OP_VERIFY,
> > 	/**< Signature Verification operation */
> > #if RTE_BUILDING_INTERNAL
> > 	__RTE_CRYPTO_ASYM_OP_LIST_END /* internal */
> > #endif
> > }
> >
> > int rte_crypto_asym_op_list_end(void);
> >
> >
> > And in the associated library code file, when including rte_crypto_asym.h:
> >
> > #define RTE_BUILDING_INTERNAL
> > #include <cryptodev/rte_crypto_asym.h>
> >
> > int
> > rte_crypto_asym_op_list_end(void)
> > {
> > 	return __RTE_CRYPTO_ASYM_OP_LIST_END;
> > }
> 
> It's more generic, and also keep LIST_END in the define, we just add new enum
> before it.
> But based on my understanding of ABI compatibility, from the point view of
> application,
> this API should return old-value even with the new library, but it will return new-
> value
> with new library. It could also break ABI.
> 
> So this API should force inline, just as this patch did. But it seem can't work if
> move
> this API to header file and add static inline.
> 
Yes, moving to c file does not seem to solve the purpose.
So should we move with the way the patch is submitted or we have some other suggestion?

Regards,
Akhil

RE: [PATCH] [RFC] cryptodev: replace LIST_END enumerators with APIs

[Morten Brørup]

> From: fengchengwen [mailto:fengchengwen@huawei.com]
> Sent: Friday, 6 September 2024 08.33
> 
> On 2024/9/5 23:09, Morten Brørup wrote:
> >> +++ b/app/test/test_cryptodev_asym.c
> >> @@ -581,7 +581,7 @@ static inline void print_asym_capa(
> >>  			rte_cryptodev_asym_get_xform_string(capa->xform_type));
> >>  	printf("operation supported -");
> >>
> >> -	for (i = 0; i < RTE_CRYPTO_ASYM_OP_LIST_END; i++) {
> >> +	for (i = 0; i < rte_crypto_asym_op_list_end(); i++) {
> >
> >> +++ b/lib/cryptodev/rte_crypto_asym.h
> >> +static inline int
> >> +rte_crypto_asym_xform_type_list_end(void)
> >> +{
> >> +	return RTE_CRYPTO_ASYM_XFORM_SM2 + 1;
> >> +}
> >> +
> >>  /**
> >>   * Asymmetric crypto operation type variants
> >> + * Note: Update rte_crypto_asym_op_list_end for every new type added.
> >>   */
> >>  enum rte_crypto_asym_op_type {
> >>  	RTE_CRYPTO_ASYM_OP_ENCRYPT,
> >> @@ -135,9 +141,14 @@ enum rte_crypto_asym_op_type {
> >>  	/**< Signature Generation operation */
> >>  	RTE_CRYPTO_ASYM_OP_VERIFY,
> >>  	/**< Signature Verification operation */
> >> -	RTE_CRYPTO_ASYM_OP_LIST_END
> >>  };
> >>
> >> +static inline int
> >> +rte_crypto_asym_op_list_end(void)
> >> +{
> >> +	return RTE_CRYPTO_ASYM_OP_VERIFY + 1;
> >> +}
> >
> > I like the concept of replacing an "last enum value" with a "last enum
> function" for API/ABI compatibility purposes.
> 
> +1
> There are many such define in DPDK, e.g. RTE_ETH_EVENT_MAX
> 
> >
> > Here's an idea...
> >
> > We can introduce a generic design pattern where we keep the _LIST_END enum
> value at the end, somehow marking it private (and not part of the API/ABI),
> and move the _list_end() function inside the C file, so it uses the _LIST_END
> enum value that the library was built with. E.g. like this:
> >
> >
> > In the header file:
> >
> > enum rte_crypto_asym_op_type {
> > 	RTE_CRYPTO_ASYM_OP_VERIFY,
> > 	/**< Signature Verification operation */
> > #if RTE_BUILDING_INTERNAL
> > 	__RTE_CRYPTO_ASYM_OP_LIST_END /* internal */
> > #endif
> > }
> >
> > int rte_crypto_asym_op_list_end(void);
> >
> >
> > And in the associated library code file, when including rte_crypto_asym.h:
> >
> > #define RTE_BUILDING_INTERNAL
> > #include <cryptodev/rte_crypto_asym.h>
> >
> > int
> > rte_crypto_asym_op_list_end(void)
> > {
> > 	return __RTE_CRYPTO_ASYM_OP_LIST_END;
> > }
> 
> It's more generic, and also keep LIST_END in the define, we just add new enum
> before it.
> But based on my understanding of ABI compatibility, from the point view of
> application,
> this API should return old-value even with the new library, but it will return
> new-value
> with new library. It could also break ABI.
> 
> So this API should force inline, just as this patch did. But it seem can't
> work if move
> this API to header file and add static inline.

Maybe a combination, returning the lowest end of the two versions of the list, would work...

----------------------------------
Common header file (rte_common.h):
----------------------------------

/* Add at end of enum list in the header file. */
#define RTE_ENUM_LIST_END(name) \
_ # name # _ENUM_LIST_END /**< @internal */

/* Add somewhere in header file, preferably after the enum list. */
#define rte_declare_enum_list_end(name) \
/** @internal */ \
int _# name # _enum_list_end(void); \
\
static int name # _enum_list_end(void) \
{ \
	static int cached = 0; \
\
	if (likely(cached != 0)) \
		return cached; \
\
	return cached = RTE_MIN( \
			RTE_ENUM_LIST_END(name), \
			_ # name # _enum_list_end()); \
} \
\
int _# name # _enum_list_end(void)

/* Add in the library/driver implementation. */
#define rte_define_enum_list_end(name) \
int _# name # _enum_list_end(void) \
{ \
	return RTE_ENUM_LIST_END(name); \
} \
\
int _# name # _enum_list_end(void)

--------------------
Library header file:
--------------------

enum rte_crypto_asym_op_type {
	RTE_CRYPTO_ASYM_OP_VERIFY,
	/**< Signature Verification operation */
	RTE_ENUM_LIST_END(rte_crypto_asym_op)
}

rte_declare_enum_list_end(rte_crypto_asym_op);

---------------
Library C file:
---------------

rte_define_enum_list_end(rte_crypto_asym_op);