[dpdk-dev] [PATCH 0/4] cryptodev and security ABI improvements

[dpdk-dev] [PATCH 0/4] cryptodev and security ABI improvements

[Akhil Goyal]

This is a first series planned for ABI improvements
in cryptodev and security library.

Other planned improvements under development.
- cryptodev: export driver interface as internal
- cryptodev: split and hide struct rte_cryptodev, struct
rte_cryptodev_data
- cryptodev: hide struct rte_cryptodev_sym_session,
rte_cryptodev_asym_session
- security: hide struct rte_security_session

Request everyone to review and contribute for the missing
pieces to improve ABI stability. 

Akhil Goyal (4):
  cryptodev: remove LIST_END enumerators
  cryptodev: promote asym APIs to stable
  security: hide internal API
  security: add reserved bitfields

 app/test/test_cryptodev_asym.c     |  4 ++--
 devtools/libabigail.abignore       |  4 ++++
 drivers/crypto/qat/qat_asym.c      |  2 +-
 lib/cryptodev/rte_crypto_asym.h    |  4 ----
 lib/cryptodev/rte_cryptodev.h      | 10 ----------
 lib/cryptodev/version.map          | 24 +++++++++++++-----------
 lib/security/rte_security.h        |  6 ++++++
 lib/security/rte_security_driver.h |  2 +-
 lib/security/version.map           |  7 ++++++-
 9 files changed, 33 insertions(+), 30 deletions(-)

-- 
2.25.1

[dpdk-dev] [PATCH 1/4] cryptodev: remove LIST_END enumerators

[Akhil Goyal]

Remove *_LIST_END enumerators from asymmetric crypto
lib to avoid ABI breakage for every new addition in
enums.

Signed-off-by: Akhil Goyal <gakhil@marvell.com>
---
 app/test/test_cryptodev_asym.c  | 4 ++--
 drivers/crypto/qat/qat_asym.c   | 2 +-
 lib/cryptodev/rte_crypto_asym.h | 4 ----
 3 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/app/test/test_cryptodev_asym.c b/app/test/test_cryptodev_asym.c
index 847b074a4f..afa0e91a45 100644
--- a/app/test/test_cryptodev_asym.c
+++ b/app/test/test_cryptodev_asym.c
@@ -542,7 +542,7 @@ test_one_case(const void *test_case, int sessionless)
 		printf("  %u) TestCase %s %s\n", test_index++,
 			tc.modex.description, test_msg);
 	} else {
-		for (i = 0; i < RTE_CRYPTO_ASYM_OP_LIST_END; i++) {
+		for (i = 0; i <= RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE; i++) {
 			if (tc.modex.xform_type == RTE_CRYPTO_ASYM_XFORM_RSA) {
 				if (tc.rsa_data.op_type_flags & (1 << i)) {
 					if (tc.rsa_data.key_exp) {
@@ -1028,7 +1028,7 @@ static inline void print_asym_capa(
 			rte_crypto_asym_xform_strings[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_SHARED_SECRET_COMPUTE; i++) {
 		/* check supported operations */
 		if (rte_cryptodev_asym_xform_capability_check_optype(capa, i))
 			printf(" %s",
diff --git a/drivers/crypto/qat/qat_asym.c b/drivers/crypto/qat/qat_asym.c
index 85973812a8..026625a4d2 100644
--- a/drivers/crypto/qat/qat_asym.c
+++ b/drivers/crypto/qat/qat_asym.c
@@ -742,7 +742,7 @@ qat_asym_session_configure(struct rte_cryptodev *dev,
 			err = -EINVAL;
 			goto error;
 		}
-	} else if (xform->xform_type >= RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END
+	} else if (xform->xform_type > RTE_CRYPTO_ASYM_XFORM_ECPM
 			|| xform->xform_type <= RTE_CRYPTO_ASYM_XFORM_NONE) {
 		QAT_LOG(ERR, "Invalid asymmetric crypto xform");
 		err = -EINVAL;
diff --git a/lib/cryptodev/rte_crypto_asym.h b/lib/cryptodev/rte_crypto_asym.h
index 9c866f553f..5edf658572 100644
--- a/lib/cryptodev/rte_crypto_asym.h
+++ b/lib/cryptodev/rte_crypto_asym.h
@@ -94,8 +94,6 @@ enum rte_crypto_asym_xform_type {
 	 */
 	RTE_CRYPTO_ASYM_XFORM_ECPM,
 	/**< Elliptic Curve Point Multiplication */
-	RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END
-	/**< End of list */
 };
 
 /**
@@ -116,7 +114,6 @@ enum rte_crypto_asym_op_type {
 	/**< DH Public Key generation operation */
 	RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE,
 	/**< DH Shared Secret compute operation */
-	RTE_CRYPTO_ASYM_OP_LIST_END
 };
 
 /**
@@ -133,7 +130,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

[dpdk-dev] [PATCH 2/4] cryptodev: promote asym APIs to stable

[Akhil Goyal]

Asymmetric crypto APIs have been stable from
quite some time, hence moving them from experimental
to stable in DPDK 21.11

Signed-off-by: Akhil Goyal <gakhil@marvell.com>
---
 lib/cryptodev/rte_cryptodev.h | 10 ----------
 lib/cryptodev/version.map     | 24 +++++++++++++-----------
 2 files changed, 13 insertions(+), 21 deletions(-)

diff --git a/lib/cryptodev/rte_cryptodev.h b/lib/cryptodev/rte_cryptodev.h
index 11f4e6fdbf..425f459143 100644
--- a/lib/cryptodev/rte_cryptodev.h
+++ b/lib/cryptodev/rte_cryptodev.h
@@ -247,7 +247,6 @@ rte_cryptodev_sym_capability_get(uint8_t dev_id,
  *   - Return description of the asymmetric crypto capability if exist.
  *   - Return NULL if the capability not exist.
  */
-__rte_experimental
 const struct rte_cryptodev_asymmetric_xform_capability *
 rte_cryptodev_asym_capability_get(uint8_t dev_id,
 		const struct rte_cryptodev_asym_capability_idx *idx);
@@ -317,7 +316,6 @@ rte_cryptodev_sym_capability_check_aead(
  *   - Return 1 if the op type is supported
  *   - Return 0 if unsupported
  */
-__rte_experimental
 int
 rte_cryptodev_asym_xform_capability_check_optype(
 	const struct rte_cryptodev_asymmetric_xform_capability *capability,
@@ -333,7 +331,6 @@ rte_cryptodev_asym_xform_capability_check_optype(
  *   - Return 0 if the parameters are in range of the capability.
  *   - Return -1 if the parameters are out of range of the capability.
  */
-__rte_experimental
 int
 rte_cryptodev_asym_xform_capability_check_modlen(
 	const struct rte_cryptodev_asymmetric_xform_capability *capability,
@@ -395,7 +392,6 @@ rte_cryptodev_get_aead_algo_enum(enum rte_crypto_aead_algorithm *algo_enum,
  * - Return -1 if string is not valid
  * - Return 0 if the string is valid
  */
-__rte_experimental
 int
 rte_cryptodev_asym_get_xform_enum(enum rte_crypto_asym_xform_type *xform_enum,
 		const char *xform_string);
@@ -1192,7 +1188,6 @@ rte_cryptodev_sym_session_create(struct rte_mempool *mempool);
  *  - On success return pointer to asym-session
  *  - On failure returns NULL
  */
-__rte_experimental
 struct rte_cryptodev_asym_session *
 rte_cryptodev_asym_session_create(struct rte_mempool *mempool);
 
@@ -1223,7 +1218,6 @@ rte_cryptodev_sym_session_free(struct rte_cryptodev_sym_session *sess);
  *  - -EINVAL if session is NULL.
  *  - -EBUSY if not all device private data has been freed.
  */
-__rte_experimental
 int
 rte_cryptodev_asym_session_free(struct rte_cryptodev_asym_session *sess);
 
@@ -1264,7 +1258,6 @@ rte_cryptodev_sym_session_init(uint8_t dev_id,
  *  - -ENOTSUP if crypto device does not support the crypto transform.
  *  - -ENOMEM if the private session could not be allocated.
  */
-__rte_experimental
 int
 rte_cryptodev_asym_session_init(uint8_t dev_id,
 			struct rte_cryptodev_asym_session *sess,
@@ -1299,7 +1292,6 @@ rte_cryptodev_sym_session_clear(uint8_t dev_id,
  *  - 0 if successful.
  *  - -EINVAL if device is invalid or session is NULL.
  */
-__rte_experimental
 int
 rte_cryptodev_asym_session_clear(uint8_t dev_id,
 			struct rte_cryptodev_asym_session *sess);
@@ -1336,7 +1328,6 @@ rte_cryptodev_sym_get_existing_header_session_size(
  * @return
  *   Size of the asymmetric header session.
  */
-__rte_experimental
 unsigned int
 rte_cryptodev_asym_get_header_session_size(void);
 
@@ -1364,7 +1355,6 @@ rte_cryptodev_sym_get_private_session_size(uint8_t dev_id);
  *   - Size of the asymmetric private data, if successful
  *   - 0 if device is invalid or does not have private session
  */
-__rte_experimental
 unsigned int
 rte_cryptodev_asym_get_private_session_size(uint8_t dev_id);
 
diff --git a/lib/cryptodev/version.map b/lib/cryptodev/version.map
index 9f04737aed..707a2e32d3 100644
--- a/lib/cryptodev/version.map
+++ b/lib/cryptodev/version.map
@@ -1,4 +1,4 @@
-DPDK_21 {
+DPDK_22 {
 	global:
 
 	rte_crypto_aead_algorithm_strings;
@@ -9,6 +9,18 @@ DPDK_21 {
 	rte_crypto_cipher_operation_strings;
 	rte_crypto_op_pool_create;
 	rte_cryptodev_allocate_driver;
+
+	rte_cryptodev_asym_capability_get;
+	rte_cryptodev_asym_get_header_session_size;
+	rte_cryptodev_asym_get_private_session_size;
+	rte_cryptodev_asym_get_xform_enum;
+	rte_cryptodev_asym_session_clear;
+	rte_cryptodev_asym_session_create;
+	rte_cryptodev_asym_session_free;
+	rte_cryptodev_asym_session_init;
+	rte_cryptodev_asym_xform_capability_check_modlen;
+	rte_cryptodev_asym_xform_capability_check_optype;
+
 	rte_cryptodev_callback_register;
 	rte_cryptodev_callback_unregister;
 	rte_cryptodev_close;
@@ -61,16 +73,6 @@ DPDK_21 {
 EXPERIMENTAL {
 	global:
 
-	rte_cryptodev_asym_capability_get;
-	rte_cryptodev_asym_get_header_session_size;
-	rte_cryptodev_asym_get_private_session_size;
-	rte_cryptodev_asym_get_xform_enum;
-	rte_cryptodev_asym_session_clear;
-	rte_cryptodev_asym_session_create;
-	rte_cryptodev_asym_session_free;
-	rte_cryptodev_asym_session_init;
-	rte_cryptodev_asym_xform_capability_check_modlen;
-	rte_cryptodev_asym_xform_capability_check_optype;
 	rte_cryptodev_sym_cpu_crypto_process;
 	rte_cryptodev_sym_get_existing_header_session_size;
 	rte_cryptodev_sym_session_get_user_data;
-- 
2.25.1

Re: [dpdk-dev] [PATCH 2/4] cryptodev: promote asym APIs to stable

[Kusztal, ArkadiuszX]

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Akhil Goyal
> Sent: Saturday, July 31, 2021 8:13 PM
> To: dev@dpdk.org
> Cc: thomas@monjalon.net; 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; Zhang, Roy Fan <roy.fan.zhang@intel.com>;
> jianjay.zhou@huawei.com; asomalap@amd.com; ruifeng.wang@arm.com;
> Akhil Goyal <gakhil@marvell.com>
> Subject: [dpdk-dev] [PATCH 2/4] cryptodev: promote asym APIs to stable
> 

Hi Akhil,

I am not sure if this API is ready to be stable so I will add few comments here:

RSA:
	rte_crypto_param message;
	...
	 * - to be signed for RSA sign generation.

If this message is plaintext, then in case of:
1) PKCS1_1.5 padding:
Standard defines data to be signed as DER encoded struct of digestAlgorithm + digest
(few exceptions I am aware of were TLS prior to 1.2 or IKE version 1)
- There is no field to specify that, even if PMD would be correctly implemented it still would lack information about hash aglorithm.
- Currently what openssl pmd for example is doing is RSA_private_encrypt which omits this step (https://www.openssl.org/docs/man1.1.1/man3/RSA_private_encrypt.html - mentions this).
2) PADDING_NONE:
I cannot find what user is supposed to do in this case, and I think it may be quite common option for hw due to reliance on strong CSPRNG for PSS or OAEP.

DSA:
	struct rte_crypto_dsa_op_param {
	...
There is no 'k' parameter? I though I have added it, how hw with no CSRNG should work with DSA?

For ECDSA private key is in Op, for DSA is in xform. Where this inconsistency comes from?

	/**< x: Private key of the signer in octet-string network
	 * byte order format.
	 * Used when app has pre-defined private key.
	 * Valid only when xform chain is DSA ONLY.
	 * if xform chain is DH private key generate + DSA, then DSA sign
	 * compute will use internally generated key.

And this one I cannot understand, there is DH and DSA in one line plus seems that private dsa key would be generated and used in the same operation.
We want to create self-signed certificate here on the fly or something?

	RTE_CRYPTO_ASYM_OP_PRIVATE_KEY_GENERATE,
	/**< DH Private Key generation operation */

This is another interesting part (similar to 'k' in (EC)DSA, PSS, QAEO in RSA), there was no any type of hw random number generation concept for symmetric crypto (i.e. salt, IV, nonce) and here we have
standalone Diffie Hellman private key generator.
And since it is no crypto computation but random number generation, maybe there should be another module to handle CSRNG or we could register randomness
source into cryptodev, like callback? Another option would be to predefine randomness source per device like (i.e. x86 RDRAND, /dev/random) for user to decide.

Additionally there is DH op but there is no ECDH (I know there is ECPM, but the same way there is MODEXP which creates another inconsistency). Optionally we can extend DH API to work with EC?
EDDSA, EDDH needs to be implemented soon too.

Regards,
Arek

Re: [dpdk-dev] [PATCH 2/4] cryptodev: promote asym APIs to stable

[Akhil Goyal]

Hi Arek,

Do you think all the asym APIs are not eligible for promoting it to stable APIs?
I haven't seen any changes for quite some time and we cannot have it experimental
Forever.
The APIs which you think are expected to change, we can leave them as experimental
And mark the others as stable.

Can you post a patch for it? I will drop it from my series.

Also, could you review the other patches in the series as well.

Regards,
Akhil

> Hi Akhil,
> 
> I am not sure if this API is ready to be stable so I will add few comments here:
> 
> RSA:
> 	rte_crypto_param message;
> 	...
> 	 * - to be signed for RSA sign generation.
> 
> If this message is plaintext, then in case of:
> 1) PKCS1_1.5 padding:
> Standard defines data to be signed as DER encoded struct of digestAlgorithm
> + digest
> (few exceptions I am aware of were TLS prior to 1.2 or IKE version 1)
> - There is no field to specify that, even if PMD would be correctly
> implemented it still would lack information about hash aglorithm.
> - Currently what openssl pmd for example is doing is RSA_private_encrypt
> which omits this step (https://www.openssl.org/docs/man1.1.1/man3/RSA_private_encrypt.html  - mentions this).
> 2) PADDING_NONE:
> I cannot find what user is supposed to do in this case, and I think it may be
> quite common option for hw due to reliance on strong CSPRNG for PSS or
> OAEP.
> 
> DSA:
> 	struct rte_crypto_dsa_op_param {
> 	...
> There is no 'k' parameter? I though I have added it, how hw with no CSRNG
> should work with DSA?
> 
> For ECDSA private key is in Op, for DSA is in xform. Where this inconsistency
> comes from?
> 
> 	/**< x: Private key of the signer in octet-string network
> 	 * byte order format.
> 	 * Used when app has pre-defined private key.
> 	 * Valid only when xform chain is DSA ONLY.
> 	 * if xform chain is DH private key generate + DSA, then DSA sign
> 	 * compute will use internally generated key.
> 
> And this one I cannot understand, there is DH and DSA in one line plus seems
> that private dsa key would be generated and used in the same operation.
> We want to create self-signed certificate here on the fly or something?
> 
> 	RTE_CRYPTO_ASYM_OP_PRIVATE_KEY_GENERATE,
> 	/**< DH Private Key generation operation */
> 
> This is another interesting part (similar to 'k' in (EC)DSA, PSS, QAEO in RSA),
> there was no any type of hw random number generation concept for
> symmetric crypto (i.e. salt, IV, nonce) and here we have
> standalone Diffie Hellman private key generator.
> And since it is no crypto computation but random number generation,
> maybe there should be another module to handle CSRNG or we could
> register randomness
> source into cryptodev, like callback? Another option would be to predefine
> randomness source per device like (i.e. x86 RDRAND, /dev/random) for user
> to decide.
> 
> Additionally there is DH op but there is no ECDH (I know there is ECPM, but
> the same way there is MODEXP which creates another inconsistency).
> Optionally we can extend DH API to work with EC?
> EDDSA, EDDH needs to be implemented soon too.
> 
> Regards,
> Arek

Re: [dpdk-dev] [PATCH 2/4] cryptodev: promote asym APIs to stable

[Kusztal, ArkadiuszX]

> Do you think all the asym APIs are not eligible for promoting it to stable APIs?
> I haven't seen any changes for quite some time and we cannot have it
> experimental Forever.
> The APIs which you think are expected to change, we can leave them as
> experimental And mark the others as stable.
We could potentially make capability related functions stable but for others there are still some many uncertainties, another example:
Ecdsa op expects 'k' in "in the interval (1, n-1)", openssl pmd will not even have function that accepts 'k' (although optionally inverse of k yes), what should user put then here?

This API needs more attention I believe, I can send patches for it after 21.11 release. 
My opinion is that we should push all this by another year.

> 
> Can you post a patch for it? I will drop it from my series.
> 
> Also, could you review the other patches in the series as well.
> 
> Regards,
> Akhil
> 
> > Hi Akhil,
> >
> > I am not sure if this API is ready to be stable so I will add few comments here:
> >
> > RSA:
> > 	rte_crypto_param message;
> > 	...
> > 	 * - to be signed for RSA sign generation.
> >
> > If this message is plaintext, then in case of:
> > 1) PKCS1_1.5 padding:
> > Standard defines data to be signed as DER encoded struct of
> > digestAlgorithm
> > + digest
> > (few exceptions I am aware of were TLS prior to 1.2 or IKE version 1)
> > - There is no field to specify that, even if PMD would be correctly
> > implemented it still would lack information about hash aglorithm.
> > - Currently what openssl pmd for example is doing is
> > RSA_private_encrypt which omits this step
> (https://www.openssl.org/docs/man1.1.1/man3/RSA_private_encrypt.html  -
> mentions this).
> > 2) PADDING_NONE:
> > I cannot find what user is supposed to do in this case, and I think it
> > may be quite common option for hw due to reliance on strong CSPRNG for
> > PSS or OAEP.
> >
> > DSA:
> > 	struct rte_crypto_dsa_op_param {
> > 	...
> > There is no 'k' parameter? I though I have added it, how hw with no
> > CSRNG should work with DSA?
> >
> > For ECDSA private key is in Op, for DSA is in xform. Where this
> > inconsistency comes from?
> >
> > 	/**< x: Private key of the signer in octet-string network
> > 	 * byte order format.
> > 	 * Used when app has pre-defined private key.
> > 	 * Valid only when xform chain is DSA ONLY.
> > 	 * if xform chain is DH private key generate + DSA, then DSA sign
> > 	 * compute will use internally generated key.
> >
> > And this one I cannot understand, there is DH and DSA in one line plus
> > seems that private dsa key would be generated and used in the same
> operation.
> > We want to create self-signed certificate here on the fly or something?
> >
> > 	RTE_CRYPTO_ASYM_OP_PRIVATE_KEY_GENERATE,
> > 	/**< DH Private Key generation operation */
> >
> > This is another interesting part (similar to 'k' in (EC)DSA, PSS, QAEO
> > in RSA), there was no any type of hw random number generation concept
> > for symmetric crypto (i.e. salt, IV, nonce) and here we have
> > standalone Diffie Hellman private key generator.
> > And since it is no crypto computation but random number generation,
> > maybe there should be another module to handle CSRNG or we could
> > register randomness source into cryptodev, like callback? Another
> > option would be to predefine randomness source per device like (i.e.
> > x86 RDRAND, /dev/random) for user to decide.
> >
> > Additionally there is DH op but there is no ECDH (I know there is
> > ECPM, but the same way there is MODEXP which creates another
> inconsistency).
> > Optionally we can extend DH API to work with EC?
> > EDDSA, EDDH needs to be implemented soon too.
> >
> > Regards,
> > Arek

Re: [dpdk-dev] [PATCH 2/4] cryptodev: promote asym APIs to stable

[Akhil Goyal]

> > Do you think all the asym APIs are not eligible for promoting it to stable
> APIs?
> > I haven't seen any changes for quite some time and we cannot have it
> > experimental Forever.
> > The APIs which you think are expected to change, we can leave them as
> > experimental And mark the others as stable.
> We could potentially make capability related functions stable but for others
> there are still some many uncertainties, another example:
> Ecdsa op expects 'k' in "in the interval (1, n-1)", openssl pmd will not even
> have function that accepts 'k' (although optionally inverse of k yes), what
> should user put then here?
> 
> This API needs more attention I believe, I can send patches for it after 21.11
> release.
> My opinion is that we should push all this by another year.
> 
Ok will drop this patch for now.

Re: [dpdk-dev] [PATCH 2/4] cryptodev: promote asym APIs to stable

[Kinsella, Ray]

Folks,

On 07/09/2021 12:45, Akhil Goyal wrote:
>>> Do you think all the asym APIs are not eligible for promoting it to stable
>> APIs?
>>> I haven't seen any changes for quite some time and we cannot have it
>>> experimental Forever.
>>> The APIs which you think are expected to change, we can leave them as
>>> experimental And mark the others as stable.
>> We could potentially make capability related functions stable but for others
>> there are still some many uncertainties, another example:
>> Ecdsa op expects 'k' in "in the interval (1, n-1)", openssl pmd will not even
>> have function that accepts 'k' (although optionally inverse of k yes), what
>> should user put then here?
>>
>> This API needs more attention I believe, I can send patches for it after 21.11
>> release.
>> My opinion is that we should push all this by another year.
>>
> Ok will drop this patch for now.
> 

Look since everyone is in alignment here, I am going to ask the symbol bot to ignore
the asym crypto APIs for the next year. Thanks for the diligence on this, and thanks to 
Fan for sending me an FYI.

Ray K

RE: [EXT] Re: [dpdk-dev] [PATCH 2/4] cryptodev: promote asym APIs to stable

[Akhil Goyal]

 Folks,

Can we promote the Asym APIs to stable now?

Regards,
Akhil

> 
> On 07/09/2021 12:45, Akhil Goyal wrote:
> >>> Do you think all the asym APIs are not eligible for promoting it to stable
> >> APIs?
> >>> I haven't seen any changes for quite some time and we cannot have it
> >>> experimental Forever.
> >>> The APIs which you think are expected to change, we can leave them as
> >>> experimental And mark the others as stable.
> >> We could potentially make capability related functions stable but for others
> >> there are still some many uncertainties, another example:
> >> Ecdsa op expects 'k' in "in the interval (1, n-1)", openssl pmd will not even
> >> have function that accepts 'k' (although optionally inverse of k yes), what
> >> should user put then here?
> >>
> >> This API needs more attention I believe, I can send patches for it after 21.11
> >> release.
> >> My opinion is that we should push all this by another year.
> >>
> > Ok will drop this patch for now.
> >
> 
> Look since everyone is in alignment here, I am going to ask the symbol bot to
> ignore
> the asym crypto APIs for the next year. Thanks for the diligence on this, and
> thanks to
> Fan for sending me an FYI.
> 
> Ray K

RE: [EXT] Re: [dpdk-dev] [PATCH 2/4] cryptodev: promote asym APIs to stable

[Hemant Agrawal]

Hi Akhil

> 
> Can we promote the Asym APIs to stable now?

+1 for it


> 
> Regards,
> Akhil
> 
> >
> > On 07/09/2021 12:45, Akhil Goyal wrote:
> > >>> Do you think all the asym APIs are not eligible for promoting it
> > >>> to stable
> > >> APIs?
> > >>> I haven't seen any changes for quite some time and we cannot have
> > >>> it experimental Forever.
> > >>> The APIs which you think are expected to change, we can leave them
> > >>> as experimental And mark the others as stable.
> > >> We could potentially make capability related functions stable but
> > >> for others there are still some many uncertainties, another example:
> > >> Ecdsa op expects 'k' in "in the interval (1, n-1)", openssl pmd
> > >> will not even have function that accepts 'k' (although optionally
> > >> inverse of k yes), what should user put then here?
> > >>
> > >> This API needs more attention I believe, I can send patches for it
> > >> after 21.11 release.
> > >> My opinion is that we should push all this by another year.
> > >>
> > > Ok will drop this patch for now.
> > >
> >
> > Look since everyone is in alignment here, I am going to ask the symbol
> > bot to ignore the asym crypto APIs for the next year. Thanks for the
> > diligence on this, and thanks to Fan for sending me an FYI.
> >
> > Ray K

RE: [EXT] Re: [dpdk-dev] [PATCH 2/4] cryptodev: promote asym APIs to stable

[Kusztal, ArkadiuszX]

> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Thursday, February 2, 2023 11:49 AM
> To: Kinsella, Ray <mdr@ashroe.eu>; Kusztal, ArkadiuszX
> <arkadiuszx.kusztal@intel.com>; dev@dpdk.org
> Cc: thomas@monjalon.net; david.marchand@redhat.com;
> hemant.agrawal@nxp.com; Anoob Joseph <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; Zhang, Roy Fan
> <roy.fan.zhang@intel.com>; jianjay.zhou@huawei.com; asomalap@amd.com;
> ruifeng.wang@arm.com; David Marchand <david.marchand@redhat.com>
> Subject: RE: [EXT] Re: [dpdk-dev] [PATCH 2/4] cryptodev: promote asym APIs to
> stable
> 
>  Folks,
> 
> Can we promote the Asym APIs to stable now?

Yes, I think we cannot keep it as experimental forever. Although there is still much work to be done starting with capabilities and tests, I will describe it in separate threads.

> 
> Regards,
> Akhil
> 
> >
> > On 07/09/2021 12:45, Akhil Goyal wrote:
> > >>> Do you think all the asym APIs are not eligible for promoting it
> > >>> to stable
> > >> APIs?
> > >>> I haven't seen any changes for quite some time and we cannot have
> > >>> it experimental Forever.
> > >>> The APIs which you think are expected to change, we can leave them
> > >>> as experimental And mark the others as stable.
> > >> We could potentially make capability related functions stable but
> > >> for others there are still some many uncertainties, another example:
> > >> Ecdsa op expects 'k' in "in the interval (1, n-1)", openssl pmd
> > >> will not even have function that accepts 'k' (although optionally
> > >> inverse of k yes), what should user put then here?
> > >>
> > >> This API needs more attention I believe, I can send patches for it
> > >> after 21.11 release.
> > >> My opinion is that we should push all this by another year.
> > >>
> > > Ok will drop this patch for now.
> > >
> >
> > Look since everyone is in alignment here, I am going to ask the symbol
> > bot to ignore the asym crypto APIs for the next year. Thanks for the
> > diligence on this, and thanks to Fan for sending me an FYI.
> >
> > Ray K

[dpdk-dev] [PATCH 3/4] security: hide internal API

[Akhil Goyal]

rte_security_dynfield_register() is an internal
API to be used by the driver, hence moving it to internal.

Signed-off-by: Akhil Goyal <gakhil@marvell.com>
---
 lib/security/rte_security_driver.h | 2 +-
 lib/security/version.map           | 7 ++++++-
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/lib/security/rte_security_driver.h b/lib/security/rte_security_driver.h
index 938373205c..b0253e962e 100644
--- a/lib/security/rte_security_driver.h
+++ b/lib/security/rte_security_driver.h
@@ -89,7 +89,7 @@ typedef int (*security_session_stats_get_t)(void *device,
 		struct rte_security_session *sess,
 		struct rte_security_stats *stats);
 
-__rte_experimental
+__rte_internal
 int rte_security_dynfield_register(void);
 
 /**
diff --git a/lib/security/version.map b/lib/security/version.map
index 22775558c8..bd91d9a16c 100644
--- a/lib/security/version.map
+++ b/lib/security/version.map
@@ -16,8 +16,13 @@ EXPERIMENTAL {
 	global:
 
 	rte_security_dynfield_offset;
-	rte_security_dynfield_register;
 	rte_security_get_userdata;
 	rte_security_session_stats_get;
 	rte_security_session_update;
 };
+
+INTERNAL {
+	global:
+
+	rte_security_dynfield_register;
+};
-- 
2.25.1

Re: [dpdk-dev] [PATCH 3/4] security: hide internal API

[Ananyev, Konstantin]

> rte_security_dynfield_register() is an internal
> API to be used by the driver, hence moving it to internal.
> 
> Signed-off-by: Akhil Goyal <gakhil@marvell.com>
> ---
>  lib/security/rte_security_driver.h | 2 +-
>  lib/security/version.map           | 7 ++++++-
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/security/rte_security_driver.h b/lib/security/rte_security_driver.h
> index 938373205c..b0253e962e 100644
> --- a/lib/security/rte_security_driver.h
> +++ b/lib/security/rte_security_driver.h
> @@ -89,7 +89,7 @@ typedef int (*security_session_stats_get_t)(void *device,
>  		struct rte_security_session *sess,
>  		struct rte_security_stats *stats);
> 
> -__rte_experimental
> +__rte_internal
>  int rte_security_dynfield_register(void);
> 
>  /**
> diff --git a/lib/security/version.map b/lib/security/version.map
> index 22775558c8..bd91d9a16c 100644
> --- a/lib/security/version.map
> +++ b/lib/security/version.map
> @@ -16,8 +16,13 @@ EXPERIMENTAL {
>  	global:
> 
>  	rte_security_dynfield_offset;
> -	rte_security_dynfield_register;
>  	rte_security_get_userdata;
>  	rte_security_session_stats_get;
>  	rte_security_session_update;
>  };
> +
> +INTERNAL {
> +	global:
> +
> +	rte_security_dynfield_register;
> +};
> --

Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

> 2.25.1

[dpdk-dev] [PATCH 4/4] security: add reserved bitfields

[Akhil Goyal]

In struct rte_security_ipsec_sa_options, for every new option
added, there is an ABI breakage, to avoid, a reserved_opts
bitfield is added to for the remaining bits available in the
structure.
Now for every new sa option, these reserved_opts can be reduced
and new option can be added. A corresponding exception is also
added in devtools/libabigail.abignore

Signed-off-by: Akhil Goyal <gakhil@marvell.com>
---
 devtools/libabigail.abignore | 4 ++++
 lib/security/rte_security.h  | 6 ++++++
 2 files changed, 10 insertions(+)

diff --git a/devtools/libabigail.abignore b/devtools/libabigail.abignore
index 93158405e0..5d8da28e55 100644
--- a/devtools/libabigail.abignore
+++ b/devtools/libabigail.abignore
@@ -52,3 +52,7 @@
 ; https://sourceware.org/bugzilla/show_bug.cgi?id=28060
 [suppress_type]
 	name = rte_eth_dev_data
+
+; Ignore changes in reserved_opts bitfield of rte_security_ipsec_sa_options
+[suppress_variable]
+	name = reserved_opts
diff --git a/lib/security/rte_security.h b/lib/security/rte_security.h
index 88d31de0a6..4606425e8d 100644
--- a/lib/security/rte_security.h
+++ b/lib/security/rte_security.h
@@ -181,6 +181,12 @@ struct rte_security_ipsec_sa_options {
 	 * * 0: Disable per session security statistics collection for this SA.
 	 */
 	uint32_t stats : 1;
+
+	/** Reserved bit fields for future extension
+	 *
+	 * Note: reduce number of bits in reserved_opts for every new option
+	 */
+	uint32_t reserved_opts : 24;
 };
 
 /** IPSec security association direction */
-- 
2.25.1

Re: [dpdk-dev] [PATCH 4/4] security: add reserved bitfields

[Ananyev, Konstantin]

> In struct rte_security_ipsec_sa_options, for every new option
> added, there is an ABI breakage, to avoid, a reserved_opts
> bitfield is added to for the remaining bits available in the
> structure.
> Now for every new sa option, these reserved_opts can be reduced
> and new option can be added. A corresponding exception is also
> added in devtools/libabigail.abignore
> 
> Signed-off-by: Akhil Goyal <gakhil@marvell.com>
> ---
>  devtools/libabigail.abignore | 4 ++++
>  lib/security/rte_security.h  | 6 ++++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/devtools/libabigail.abignore b/devtools/libabigail.abignore
> index 93158405e0..5d8da28e55 100644
> --- a/devtools/libabigail.abignore
> +++ b/devtools/libabigail.abignore
> @@ -52,3 +52,7 @@
>  ; https://sourceware.org/bugzilla/show_bug.cgi?id=28060
>  [suppress_type]
>  	name = rte_eth_dev_data
> +
> +; Ignore changes in reserved_opts bitfield of rte_security_ipsec_sa_options
> +[suppress_variable]
> +	name = reserved_opts
> diff --git a/lib/security/rte_security.h b/lib/security/rte_security.h
> index 88d31de0a6..4606425e8d 100644
> --- a/lib/security/rte_security.h
> +++ b/lib/security/rte_security.h
> @@ -181,6 +181,12 @@ struct rte_security_ipsec_sa_options {
>  	 * * 0: Disable per session security statistics collection for this SA.
>  	 */
>  	uint32_t stats : 1;
> +
> +	/** Reserved bit fields for future extension
> +	 *
> +	 * Note: reduce number of bits in reserved_opts for every new option
> +	 */
> +	uint32_t reserved_opts : 24;
>  };
> 
>  /** IPSec security association direction */
> --

Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

> 2.25.1

Re: [dpdk-dev] [PATCH 4/4] security: add reserved bitfields

[Stephen Hemminger]

On Sat, 31 Jul 2021 23:43:27 +0530
Akhil Goyal <gakhil@marvell.com> wrote:

> diff --git a/lib/security/rte_security.h b/lib/security/rte_security.h
> index 88d31de0a6..4606425e8d 100644
> --- a/lib/security/rte_security.h
> +++ b/lib/security/rte_security.h
> @@ -181,6 +181,12 @@ struct rte_security_ipsec_sa_options {
>  	 * * 0: Disable per session security statistics collection for this SA.
>  	 */
>  	uint32_t stats : 1;
> +
> +	/** Reserved bit fields for future extension
> +	 *
> +	 * Note: reduce number of bits in reserved_opts for every new option
> +	 */
> +	uint32_t reserved_opts : 24;
>  };
>  
>  /** IPSec security association direction */


You must add a check that these are 0 otherwise they are useless as being
reserved later. You can't assume the old application will leave these bits
as zero.

Re: [dpdk-dev] [PATCH 0/4] cryptodev and security ABI improvements

[Akhil Goyal]

> Subject: [PATCH 0/4] cryptodev and security ABI improvements
> 
> This is a first series planned for ABI improvements
> in cryptodev and security library.
> 
> Other planned improvements under development.
> - cryptodev: export driver interface as internal
> - cryptodev: split and hide struct rte_cryptodev, struct
> rte_cryptodev_data
> - cryptodev: hide struct rte_cryptodev_sym_session,
> rte_cryptodev_asym_session
> - security: hide struct rte_security_session
> 
> Request everyone to review and contribute for the missing
> pieces to improve ABI stability.
> 
Forgot to mention, this is an RFC series for DPDK 21.11

> Akhil Goyal (4):
>   cryptodev: remove LIST_END enumerators
>   cryptodev: promote asym APIs to stable
>   security: hide internal API
>   security: add reserved bitfields
> 
>  app/test/test_cryptodev_asym.c     |  4 ++--
>  devtools/libabigail.abignore       |  4 ++++
>  drivers/crypto/qat/qat_asym.c      |  2 +-
>  lib/cryptodev/rte_crypto_asym.h    |  4 ----
>  lib/cryptodev/rte_cryptodev.h      | 10 ----------
>  lib/cryptodev/version.map          | 24 +++++++++++++-----------
>  lib/security/rte_security.h        |  6 ++++++
>  lib/security/rte_security_driver.h |  2 +-
>  lib/security/version.map           |  7 ++++++-
>  9 files changed, 33 insertions(+), 30 deletions(-)
> 
> --
> 2.25.1

[dpdk-dev] [PATCH v2 1/3] cryptodev: remove LIST_END enumerators

[Akhil Goyal]

Remove *_LIST_END enumerators from asymmetric crypto
lib to avoid ABI breakage for every new addition in
enums.

Signed-off-by: Akhil Goyal <gakhil@marvell.com>
---
v2: no change

 app/test/test_cryptodev_asym.c  | 4 ++--
 drivers/crypto/qat/qat_asym.c   | 2 +-
 lib/cryptodev/rte_crypto_asym.h | 4 ----
 3 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/app/test/test_cryptodev_asym.c b/app/test/test_cryptodev_asym.c
index 9d19a6d6d9..603b2e4609 100644
--- a/app/test/test_cryptodev_asym.c
+++ b/app/test/test_cryptodev_asym.c
@@ -541,7 +541,7 @@ test_one_case(const void *test_case, int sessionless)
 		printf("  %u) TestCase %s %s\n", test_index++,
 			tc.modex.description, test_msg);
 	} else {
-		for (i = 0; i < RTE_CRYPTO_ASYM_OP_LIST_END; i++) {
+		for (i = 0; i <= RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE; i++) {
 			if (tc.modex.xform_type == RTE_CRYPTO_ASYM_XFORM_RSA) {
 				if (tc.rsa_data.op_type_flags & (1 << i)) {
 					if (tc.rsa_data.key_exp) {
@@ -1027,7 +1027,7 @@ static inline void print_asym_capa(
 			rte_crypto_asym_xform_strings[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_SHARED_SECRET_COMPUTE; i++) {
 		/* check supported operations */
 		if (rte_cryptodev_asym_xform_capability_check_optype(capa, i))
 			printf(" %s",
diff --git a/drivers/crypto/qat/qat_asym.c b/drivers/crypto/qat/qat_asym.c
index 85973812a8..026625a4d2 100644
--- a/drivers/crypto/qat/qat_asym.c
+++ b/drivers/crypto/qat/qat_asym.c
@@ -742,7 +742,7 @@ qat_asym_session_configure(struct rte_cryptodev *dev,
 			err = -EINVAL;
 			goto error;
 		}
-	} else if (xform->xform_type >= RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END
+	} else if (xform->xform_type > RTE_CRYPTO_ASYM_XFORM_ECPM
 			|| xform->xform_type <= RTE_CRYPTO_ASYM_XFORM_NONE) {
 		QAT_LOG(ERR, "Invalid asymmetric crypto xform");
 		err = -EINVAL;
diff --git a/lib/cryptodev/rte_crypto_asym.h b/lib/cryptodev/rte_crypto_asym.h
index 9c866f553f..5edf658572 100644
--- a/lib/cryptodev/rte_crypto_asym.h
+++ b/lib/cryptodev/rte_crypto_asym.h
@@ -94,8 +94,6 @@ enum rte_crypto_asym_xform_type {
 	 */
 	RTE_CRYPTO_ASYM_XFORM_ECPM,
 	/**< Elliptic Curve Point Multiplication */
-	RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END
-	/**< End of list */
 };
 
 /**
@@ -116,7 +114,6 @@ enum rte_crypto_asym_op_type {
 	/**< DH Public Key generation operation */
 	RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE,
 	/**< DH Shared Secret compute operation */
-	RTE_CRYPTO_ASYM_OP_LIST_END
 };
 
 /**
@@ -133,7 +130,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

[dpdk-dev] [PATCH v2 2/3] security: hide internal API

[Akhil Goyal]

rte_security_dynfield_register() is an internal
API to be used by the driver, hence moving it to internal.

Signed-off-by: Akhil Goyal <gakhil@marvell.com>
Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
---
v2: no change

 lib/security/rte_security_driver.h | 2 +-
 lib/security/version.map           | 7 ++++++-
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/lib/security/rte_security_driver.h b/lib/security/rte_security_driver.h
index 938373205c..b0253e962e 100644
--- a/lib/security/rte_security_driver.h
+++ b/lib/security/rte_security_driver.h
@@ -89,7 +89,7 @@ typedef int (*security_session_stats_get_t)(void *device,
 		struct rte_security_session *sess,
 		struct rte_security_stats *stats);
 
-__rte_experimental
+__rte_internal
 int rte_security_dynfield_register(void);
 
 /**
diff --git a/lib/security/version.map b/lib/security/version.map
index a1f46bfd27..edf4887e12 100644
--- a/lib/security/version.map
+++ b/lib/security/version.map
@@ -16,7 +16,12 @@ EXPERIMENTAL {
 	__rte_security_get_userdata;
 	__rte_security_set_pkt_metadata;
 	rte_security_dynfield_offset;
-	rte_security_dynfield_register;
 	rte_security_session_stats_get;
 	rte_security_session_update;
 };
+
+INTERNAL {
+	global:
+
+	rte_security_dynfield_register;
+};
-- 
2.25.1

Re: [dpdk-dev] [PATCH v2 2/3] security: hide internal API

[Kinsella, Ray]

On 08/10/2021 21:45, Akhil Goyal wrote:
> rte_security_dynfield_register() is an internal
> API to be used by the driver, hence moving it to internal.
> 
> Signed-off-by: Akhil Goyal <gakhil@marvell.com>
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> ---
> v2: no change
> 
>  lib/security/rte_security_driver.h | 2 +-
>  lib/security/version.map           | 7 ++++++-
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/security/rte_security_driver.h b/lib/security/rte_security_driver.h
> index 938373205c..b0253e962e 100644
> --- a/lib/security/rte_security_driver.h
> +++ b/lib/security/rte_security_driver.h
> @@ -89,7 +89,7 @@ typedef int (*security_session_stats_get_t)(void *device,
>  		struct rte_security_session *sess,
>  		struct rte_security_stats *stats);
>  
> -__rte_experimental
> +__rte_internal
>  int rte_security_dynfield_register(void);
>  
>  /**
> diff --git a/lib/security/version.map b/lib/security/version.map
> index a1f46bfd27..edf4887e12 100644
> --- a/lib/security/version.map
> +++ b/lib/security/version.map
> @@ -16,7 +16,12 @@ EXPERIMENTAL {
>  	__rte_security_get_userdata;
>  	__rte_security_set_pkt_metadata;
>  	rte_security_dynfield_offset;
> -	rte_security_dynfield_register;
>  	rte_security_session_stats_get;
>  	rte_security_session_update;
>  };
> +
> +INTERNAL {
> +	global:
> +
> +	rte_security_dynfield_register;
> +};
> 
Acked-by: Ray Kinsella <mdr@ashroe.eu>

[dpdk-dev] [PATCH v2 3/3] security: add reserved bitfields

[Akhil Goyal]

In struct rte_security_ipsec_sa_options, for every new option
added, there is an ABI breakage, to avoid, a reserved_opts
bitfield is added to for the remaining bits available in the
structure.
Now for every new sa option, these reserved_opts can be reduced
and new option can be added.

Signed-off-by: Akhil Goyal <gakhil@marvell.com>
Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
---
v2: rebase and removed libabigail.abignore change.
    Exception may be added when there is a need for change.

 lib/security/rte_security.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lib/security/rte_security.h b/lib/security/rte_security.h
index 7eb9f109ae..c0ea13892e 100644
--- a/lib/security/rte_security.h
+++ b/lib/security/rte_security.h
@@ -258,6 +258,12 @@ struct rte_security_ipsec_sa_options {
 	 * PKT_TX_UDP_CKSUM or PKT_TX_L4_MASK in mbuf.
 	 */
 	uint32_t l4_csum_enable : 1;
+
+	/** Reserved bit fields for future extension
+	 *
+	 * Note: reduce number of bits in reserved_opts for every new option
+	 */
+	uint32_t reserved_opts : 18;
 };
 
 /** IPSec security association direction */
-- 
2.25.1

Re: [dpdk-dev] [PATCH v2 3/3] security: add reserved bitfields

[Thomas Monjalon]

08/10/2021 22:45, Akhil Goyal:
> In struct rte_security_ipsec_sa_options, for every new option
> added, there is an ABI breakage, to avoid, a reserved_opts
> bitfield is added to for the remaining bits available in the
> structure.
> Now for every new sa option, these reserved_opts can be reduced
> and new option can be added.

How do you make sure this field is initialized to 0?

Re: [dpdk-dev] [EXT] Re: [PATCH v2 3/3] security: add reserved bitfields

[Akhil Goyal]

> 08/10/2021 22:45, Akhil Goyal:
> > In struct rte_security_ipsec_sa_options, for every new option
> > added, there is an ABI breakage, to avoid, a reserved_opts
> > bitfield is added to for the remaining bits available in the
> > structure.
> > Now for every new sa option, these reserved_opts can be reduced
> > and new option can be added.
> 
> How do you make sure this field is initialized to 0?
> 
Struct rte_security_ipsec_xform Is part of rte_security_capability as well
As a configuration structure in session create.
User, should ensure that if a device support that option(in capability), then
only these options will take into effect or else it will be don't care for the PMD.
The initial values of capabilities are set by PMD statically based on the features
that it support.
So if someone sets a bit in reserved_opts, it will work only if PMD support it
And sets the corresponding field in capabilities.
But yes, if a new field is added in future, and user sets the reserved_opts by mistake
And the PMD supports that feature as well, then that feature will be enabled.
This may or may not create issue depending on the feature which is enabled.

Should I add a note in the comments to clarify that reserved_opts should be set as 0
And future releases may change this without notice(But reserved in itself suggest that)?
Adding an explicit check in session_create does not make sense to me.
What do you suggest?

Regards,
Akhil

Re: [dpdk-dev] [EXT] Re: [PATCH v2 3/3] security: add reserved bitfields

[Stephen Hemminger]

On Mon, 11 Oct 2021 16:58:24 +0000
Akhil Goyal <gakhil@marvell.com> wrote:

> > 08/10/2021 22:45, Akhil Goyal:  
> > > In struct rte_security_ipsec_sa_options, for every new option
> > > added, there is an ABI breakage, to avoid, a reserved_opts
> > > bitfield is added to for the remaining bits available in the
> > > structure.
> > > Now for every new sa option, these reserved_opts can be reduced
> > > and new option can be added.  
> > 
> > How do you make sure this field is initialized to 0?
> >   
> Struct rte_security_ipsec_xform Is part of rte_security_capability as well
> As a configuration structure in session create.
> User, should ensure that if a device support that option(in capability), then
> only these options will take into effect or else it will be don't care for the PMD.
> The initial values of capabilities are set by PMD statically based on the features
> that it support.
> So if someone sets a bit in reserved_opts, it will work only if PMD support it
> And sets the corresponding field in capabilities.
> But yes, if a new field is added in future, and user sets the reserved_opts by mistake
> And the PMD supports that feature as well, then that feature will be enabled.
> This may or may not create issue depending on the feature which is enabled.
> 
> Should I add a note in the comments to clarify that reserved_opts should be set as 0
> And future releases may change this without notice(But reserved in itself suggest that)?
> Adding an explicit check in session_create does not make sense to me.
> What do you suggest?
> 
> Regards,
> Akhil
> 

The problem is if user creates an on stack variable and sets the unreserved
fields to good values but other parts are garbage.  This passes API/ABI unless
you strictly enforce that all reserved fields are zero.

Re: [dpdk-dev] [EXT] Re: [PATCH v2 3/3] security: add reserved bitfields

[Kinsella, Ray]

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Monday 11 October 2021 23:16
> To: Akhil Goyal <gakhil@marvell.com>
> Cc: Thomas Monjalon <thomas@monjalon.net>; dev@dpdk.org;
> david.marchand@redhat.com; hemant.agrawal@nxp.com; Anoob Joseph
> <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; Zhang, Roy Fan <roy.fan.zhang@intel.com>;
> jianjay.zhou@huawei.com; asomalap@amd.com; ruifeng.wang@arm.com;
> Ananyev, Konstantin <konstantin.ananyev@intel.com>; Nicolau, Radu
> <radu.nicolau@intel.com>; ajit.khaparde@broadcom.com; Nagadheeraj
> Rottela <rnagadheeraj@marvell.com>; Ankur Dwivedi
> <adwivedi@marvell.com>; Power, Ciara <ciara.power@intel.com>; Kinsella,
> Ray <ray.kinsella@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>
> Subject: Re: [EXT] Re: [PATCH v2 3/3] security: add reserved bitfields
> 
> On Mon, 11 Oct 2021 16:58:24 +0000
> Akhil Goyal <gakhil@marvell.com> wrote:
> 
> > > 08/10/2021 22:45, Akhil Goyal:
> > > > In struct rte_security_ipsec_sa_options, for every new option
> > > > added, there is an ABI breakage, to avoid, a reserved_opts
> > > > bitfield is added to for the remaining bits available in the
> > > > structure.
> > > > Now for every new sa option, these reserved_opts can be reduced
> > > > and new option can be added.
> > >
> > > How do you make sure this field is initialized to 0?
> > >
> > Struct rte_security_ipsec_xform Is part of rte_security_capability as
> > well As a configuration structure in session create.
> > User, should ensure that if a device support that option(in
> > capability), then only these options will take into effect or else it
> will be don't care for the PMD.
> > The initial values of capabilities are set by PMD statically based on
> > the features that it support.
> > So if someone sets a bit in reserved_opts, it will work only if PMD
> > support it And sets the corresponding field in capabilities.
> > But yes, if a new field is added in future, and user sets the
> > reserved_opts by mistake And the PMD supports that feature as well,
> then that feature will be enabled.
> > This may or may not create issue depending on the feature which is
> enabled.
> >
> > Should I add a note in the comments to clarify that reserved_opts
> > should be set as 0 And future releases may change this without
> notice(But reserved in itself suggest that)?
> > Adding an explicit check in session_create does not make sense to me.
> > What do you suggest?
> >
> > Regards,
> > Akhil
> >
> 
> The problem is if user creates an on stack variable and sets the
> unreserved fields to good values but other parts are garbage.  This
> passes API/ABI unless you strictly enforce that all reserved fields are
> zero.

Right, but that is no better or worse than the current struct, in that respect, right?
User case be careless there also - declare it on the stack and forget to memset.

struct rte_security_ipsec_sa_options {
     uint32_t esn : 1;
 
     uint32_t udp_encap : 1;
 
     uint32_t copy_dscp : 1;
 
     uint32_t copy_flabel : 1;
 
     uint32_t copy_df : 1;
 
     uint32_t dec_ttl : 1;
 
     uint32_t ecn : 1;
 
     uint32_t stats : 1;
 
     uint32_t iv_gen_disable : 1;
 
     uint32_t tunnel_hdr_verify : 2;
 };

Re: [dpdk-dev] [EXT] Re: [PATCH v2 3/3] security: add reserved bitfields

[Thomas Monjalon]

11/10/2021 18:58, Akhil Goyal:
> > 08/10/2021 22:45, Akhil Goyal:
> > > In struct rte_security_ipsec_sa_options, for every new option
> > > added, there is an ABI breakage, to avoid, a reserved_opts
> > > bitfield is added to for the remaining bits available in the
> > > structure.
> > > Now for every new sa option, these reserved_opts can be reduced
> > > and new option can be added.
> > 
> > How do you make sure this field is initialized to 0?
> > 
> Struct rte_security_ipsec_xform Is part of rte_security_capability as well
> As a configuration structure in session create.
> User, should ensure that if a device support that option(in capability), then
> only these options will take into effect or else it will be don't care for the PMD.
> The initial values of capabilities are set by PMD statically based on the features
> that it support.
> So if someone sets a bit in reserved_opts, it will work only if PMD support it
> And sets the corresponding field in capabilities.
> But yes, if a new field is added in future, and user sets the reserved_opts by mistake
> And the PMD supports that feature as well, then that feature will be enabled.
> This may or may not create issue depending on the feature which is enabled.
> 
> Should I add a note in the comments to clarify that reserved_opts should be set as 0
> And future releases may change this without notice(But reserved in itself suggest that)?
> Adding an explicit check in session_create does not make sense to me.
> What do you suggest?

Yes at the minimum you should add a comment.
You could also initialize it in the lib, but it is not always possible.

Re: [dpdk-dev] [EXT] Re: [PATCH v2 3/3] security: add reserved bitfields

[Kinsella, Ray]

On 12/10/2021 07:59, Thomas Monjalon wrote:
> 11/10/2021 18:58, Akhil Goyal:
>>> 08/10/2021 22:45, Akhil Goyal:
>>>> In struct rte_security_ipsec_sa_options, for every new option
>>>> added, there is an ABI breakage, to avoid, a reserved_opts
>>>> bitfield is added to for the remaining bits available in the
>>>> structure.
>>>> Now for every new sa option, these reserved_opts can be reduced
>>>> and new option can be added.
>>>
>>> How do you make sure this field is initialized to 0?
>>>
>> Struct rte_security_ipsec_xform Is part of rte_security_capability as well
>> As a configuration structure in session create.
>> User, should ensure that if a device support that option(in capability), then
>> only these options will take into effect or else it will be don't care for the PMD.
>> The initial values of capabilities are set by PMD statically based on the features
>> that it support.
>> So if someone sets a bit in reserved_opts, it will work only if PMD support it
>> And sets the corresponding field in capabilities.
>> But yes, if a new field is added in future, and user sets the reserved_opts by mistake
>> And the PMD supports that feature as well, then that feature will be enabled.
>> This may or may not create issue depending on the feature which is enabled.
>>
>> Should I add a note in the comments to clarify that reserved_opts should be set as 0
>> And future releases may change this without notice(But reserved in itself suggest that)?
>> Adding an explicit check in session_create does not make sense to me.
>> What do you suggest?
> 
> Yes at the minimum you should add a comment.
> You could also initialize it in the lib, but it is not always possible.
> 
Provide a macro  for initialization perhaps ... but there would be no way to enforce using it.

Ray K

Re: [dpdk-dev] [PATCH v2 3/3] security: add reserved bitfields

[Kinsella, Ray]

On 08/10/2021 21:45, Akhil Goyal wrote:
> In struct rte_security_ipsec_sa_options, for every new option
> added, there is an ABI breakage, to avoid, a reserved_opts
> bitfield is added to for the remaining bits available in the
> structure.
> Now for every new sa option, these reserved_opts can be reduced
> and new option can be added.
> 
> Signed-off-by: Akhil Goyal <gakhil@marvell.com>
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> ---
> v2: rebase and removed libabigail.abignore change.
>     Exception may be added when there is a need for change.
> 
>  lib/security/rte_security.h | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/lib/security/rte_security.h b/lib/security/rte_security.h
> index 7eb9f109ae..c0ea13892e 100644
> --- a/lib/security/rte_security.h
> +++ b/lib/security/rte_security.h
> @@ -258,6 +258,12 @@ struct rte_security_ipsec_sa_options {
>  	 * PKT_TX_UDP_CKSUM or PKT_TX_L4_MASK in mbuf.
>  	 */
>  	uint32_t l4_csum_enable : 1;
> +
> +	/** Reserved bit fields for future extension
> +	 *
> +	 * Note: reduce number of bits in reserved_opts for every new option
> +	 */
> +	uint32_t reserved_opts : 18;
>  };
>  
>  /** IPSec security association direction */
> 
Acked-by: Ray Kinsella <mdr@ashroe.eu>

Re: [dpdk-dev] [PATCH v2 1/3] cryptodev: remove LIST_END enumerators

[Zhang, Roy Fan]

> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Friday, October 8, 2021 9:45 PM
> To: dev@dpdk.org
> Cc: thomas@monjalon.net; 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; Zhang, Roy Fan <roy.fan.zhang@intel.com>;
> jianjay.zhou@huawei.com; asomalap@amd.com; ruifeng.wang@arm.com;
> Ananyev, Konstantin <konstantin.ananyev@intel.com>; Nicolau, Radu
> <radu.nicolau@intel.com>; ajit.khaparde@broadcom.com;
> rnagadheeraj@marvell.com; adwivedi@marvell.com; Power, Ciara
> <ciara.power@intel.com>; Akhil Goyal <gakhil@marvell.com>
> Subject: [PATCH v2 1/3] cryptodev: remove LIST_END enumerators
> 
> Remove *_LIST_END enumerators from asymmetric crypto
> lib to avoid ABI breakage for every new addition in
> enums.
> 
> Signed-off-by: Akhil Goyal <gakhil@marvell.com>
> ---
Acked-by: Fan Zhang <roy.fan.zhang@intel.com>

Re: [dpdk-dev] [PATCH v2 1/3] cryptodev: remove LIST_END enumerators

[Kinsella, Ray]

On 08/10/2021 21:45, Akhil Goyal wrote:
> Remove *_LIST_END enumerators from asymmetric crypto
> lib to avoid ABI breakage for every new addition in
> enums.
> 
> Signed-off-by: Akhil Goyal <gakhil@marvell.com>
> ---
> v2: no change
> 
>  app/test/test_cryptodev_asym.c  | 4 ++--
>  drivers/crypto/qat/qat_asym.c   | 2 +-
>  lib/cryptodev/rte_crypto_asym.h | 4 ----
>  3 files changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/app/test/test_cryptodev_asym.c b/app/test/test_cryptodev_asym.c
> index 9d19a6d6d9..603b2e4609 100644
> --- a/app/test/test_cryptodev_asym.c
> +++ b/app/test/test_cryptodev_asym.c
> @@ -541,7 +541,7 @@ test_one_case(const void *test_case, int sessionless)
>  		printf("  %u) TestCase %s %s\n", test_index++,
>  			tc.modex.description, test_msg);
>  	} else {
> -		for (i = 0; i < RTE_CRYPTO_ASYM_OP_LIST_END; i++) {
> +		for (i = 0; i <= RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE; i++) {
>  			if (tc.modex.xform_type == RTE_CRYPTO_ASYM_XFORM_RSA) {
>  				if (tc.rsa_data.op_type_flags & (1 << i)) {
>  					if (tc.rsa_data.key_exp) {
> @@ -1027,7 +1027,7 @@ static inline void print_asym_capa(
>  			rte_crypto_asym_xform_strings[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_SHARED_SECRET_COMPUTE; i++) {
>  		/* check supported operations */
>  		if (rte_cryptodev_asym_xform_capability_check_optype(capa, i))
>  			printf(" %s",
> diff --git a/drivers/crypto/qat/qat_asym.c b/drivers/crypto/qat/qat_asym.c
> index 85973812a8..026625a4d2 100644
> --- a/drivers/crypto/qat/qat_asym.c
> +++ b/drivers/crypto/qat/qat_asym.c
> @@ -742,7 +742,7 @@ qat_asym_session_configure(struct rte_cryptodev *dev,
>  			err = -EINVAL;
>  			goto error;
>  		}
> -	} else if (xform->xform_type >= RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END
> +	} else if (xform->xform_type > RTE_CRYPTO_ASYM_XFORM_ECPM
>  			|| xform->xform_type <= RTE_CRYPTO_ASYM_XFORM_NONE) {
>  		QAT_LOG(ERR, "Invalid asymmetric crypto xform");
>  		err = -EINVAL;
> diff --git a/lib/cryptodev/rte_crypto_asym.h b/lib/cryptodev/rte_crypto_asym.h
> index 9c866f553f..5edf658572 100644
> --- a/lib/cryptodev/rte_crypto_asym.h
> +++ b/lib/cryptodev/rte_crypto_asym.h
> @@ -94,8 +94,6 @@ enum rte_crypto_asym_xform_type {
>  	 */
>  	RTE_CRYPTO_ASYM_XFORM_ECPM,
>  	/**< Elliptic Curve Point Multiplication */
> -	RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END
> -	/**< End of list */
>  };
>  
>  /**
> @@ -116,7 +114,6 @@ enum rte_crypto_asym_op_type {
>  	/**< DH Public Key generation operation */
>  	RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE,
>  	/**< DH Shared Secret compute operation */
> -	RTE_CRYPTO_ASYM_OP_LIST_END
>  };
>  
>  /**
> @@ -133,7 +130,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
>  };
>  
>  /**

So I am not sure that this is an improvement.
The cryptodev issue we had, was that _LIST_END was being used to size arrays. 
And that broke when new algorithms got added. Is that an issue, in this case?

I am not sure that swapping out _LIST_END, and then littering the code with
RTE_CRYPTO_ASYM_XFORM_ECPM and RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE, is an improvement here.

My 2c is that from an ABI PoV RTE_CRYPTO_ASYM_OP_LIST_END is not better or worse,
than RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE?

Interested to hear other thoughts.

Re: [dpdk-dev] [EXT] Re: [PATCH v2 1/3] cryptodev: remove LIST_END enumerators

[Akhil Goyal]

> 
> On 08/10/2021 21:45, Akhil Goyal wrote:
> > Remove *_LIST_END enumerators from asymmetric crypto
> > lib to avoid ABI breakage for every new addition in
> > enums.
> >
> > Signed-off-by: Akhil Goyal <gakhil@marvell.com>
> > ---
> > v2: no change
> >
> >  app/test/test_cryptodev_asym.c  | 4 ++--
> >  drivers/crypto/qat/qat_asym.c   | 2 +-
> >  lib/cryptodev/rte_crypto_asym.h | 4 ----
> >  3 files changed, 3 insertions(+), 7 deletions(-)
> >
> > diff --git a/app/test/test_cryptodev_asym.c
> b/app/test/test_cryptodev_asym.c
> > index 9d19a6d6d9..603b2e4609 100644
> > --- a/app/test/test_cryptodev_asym.c
> > +++ b/app/test/test_cryptodev_asym.c
> > @@ -541,7 +541,7 @@ test_one_case(const void *test_case, int
> sessionless)
> >  		printf("  %u) TestCase %s %s\n", test_index++,
> >  			tc.modex.description, test_msg);
> >  	} else {
> > -		for (i = 0; i < RTE_CRYPTO_ASYM_OP_LIST_END; i++) {
> > +		for (i = 0; i <=
> RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE; i++) {
> >  			if (tc.modex.xform_type ==
> RTE_CRYPTO_ASYM_XFORM_RSA) {
> >  				if (tc.rsa_data.op_type_flags & (1 << i)) {
> >  					if (tc.rsa_data.key_exp) {
> > @@ -1027,7 +1027,7 @@ static inline void print_asym_capa(
> >  			rte_crypto_asym_xform_strings[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_SHARED_SECRET_COMPUTE;
> i++) {
> >  		/* check supported operations */
> >  		if
> (rte_cryptodev_asym_xform_capability_check_optype(capa, i))
> >  			printf(" %s",
> > diff --git a/drivers/crypto/qat/qat_asym.c b/drivers/crypto/qat/qat_asym.c
> > index 85973812a8..026625a4d2 100644
> > --- a/drivers/crypto/qat/qat_asym.c
> > +++ b/drivers/crypto/qat/qat_asym.c
> > @@ -742,7 +742,7 @@ qat_asym_session_configure(struct rte_cryptodev
> *dev,
> >  			err = -EINVAL;
> >  			goto error;
> >  		}
> > -	} else if (xform->xform_type >=
> RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END
> > +	} else if (xform->xform_type > RTE_CRYPTO_ASYM_XFORM_ECPM
> >  			|| xform->xform_type <=
> RTE_CRYPTO_ASYM_XFORM_NONE) {
> >  		QAT_LOG(ERR, "Invalid asymmetric crypto xform");
> >  		err = -EINVAL;
> > diff --git a/lib/cryptodev/rte_crypto_asym.h
> b/lib/cryptodev/rte_crypto_asym.h
> > index 9c866f553f..5edf658572 100644
> > --- a/lib/cryptodev/rte_crypto_asym.h
> > +++ b/lib/cryptodev/rte_crypto_asym.h
> > @@ -94,8 +94,6 @@ enum rte_crypto_asym_xform_type {
> >  	 */
> >  	RTE_CRYPTO_ASYM_XFORM_ECPM,
> >  	/**< Elliptic Curve Point Multiplication */
> > -	RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END
> > -	/**< End of list */
> >  };
> >
> >  /**
> > @@ -116,7 +114,6 @@ enum rte_crypto_asym_op_type {
> >  	/**< DH Public Key generation operation */
> >  	RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE,
> >  	/**< DH Shared Secret compute operation */
> > -	RTE_CRYPTO_ASYM_OP_LIST_END
> >  };
> >
> >  /**
> > @@ -133,7 +130,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
> >  };
> >
> >  /**
> 
> So I am not sure that this is an improvement.
> The cryptodev issue we had, was that _LIST_END was being used to size
> arrays.
> And that broke when new algorithms got added. Is that an issue, in this case?

Yes we did this same exercise for symmetric crypto enums earlier.
Asym enums were left as it was experimental at that point.
They are still experimental, but thought of making this uniform
throughout DPDK enums.

> 
> I am not sure that swapping out _LIST_END, and then littering the code with
> RTE_CRYPTO_ASYM_XFORM_ECPM and
> RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE, is an improvement
> here.
> 
> My 2c is that from an ABI PoV RTE_CRYPTO_ASYM_OP_LIST_END is not
> better or worse,
> than RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE?
> 
> Interested to hear other thoughts.

I don’t have any better solution for avoiding ABI issues for now.
The change is for avoiding ABI breakage. But we can drop this patch
For now as asym is still experimental.

Re: [dpdk-dev] [EXT] Re: [PATCH v2 1/3] cryptodev: remove LIST_END enumerators

[Anoob Joseph]

Hi Ray, Akhil,

Please see inline.

Thanks,
Anoob

> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Tuesday, October 12, 2021 3:49 PM
> To: Kinsella, Ray <mdr@ashroe.eu>; dev@dpdk.org
> Cc: thomas@monjalon.net; david.marchand@redhat.com;
> hemant.agrawal@nxp.com; Anoob Joseph <anoobj@marvell.com>;
> pablo.de.lara.guarch@intel.com; fiona.trahe@intel.com;
> declan.doherty@intel.com; matan@nvidia.com; g.singh@nxp.com;
> roy.fan.zhang@intel.com; jianjay.zhou@huawei.com; asomalap@amd.com;
> ruifeng.wang@arm.com; konstantin.ananyev@intel.com;
> radu.nicolau@intel.com; ajit.khaparde@broadcom.com; Nagadheeraj Rottela
> <rnagadheeraj@marvell.com>; Ankur Dwivedi <adwivedi@marvell.com>;
> ciara.power@intel.com; Stephen Hemminger <stephen@networkplumber.org>;
> Yigit, Ferruh <ferruh.yigit@intel.com>
> Subject: RE: [EXT] Re: [dpdk-dev] [PATCH v2 1/3] cryptodev: remove LIST_END
> enumerators
> 
> >
> > On 08/10/2021 21:45, Akhil Goyal wrote:
> > > Remove *_LIST_END enumerators from asymmetric crypto lib to avoid
> > > ABI breakage for every new addition in enums.
> > >
> > > Signed-off-by: Akhil Goyal <gakhil@marvell.com>
> > > ---
> > > v2: no change
> > >
> > >  app/test/test_cryptodev_asym.c  | 4 ++--
> > >  drivers/crypto/qat/qat_asym.c   | 2 +-
> > >  lib/cryptodev/rte_crypto_asym.h | 4 ----
> > >  3 files changed, 3 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/app/test/test_cryptodev_asym.c
> > b/app/test/test_cryptodev_asym.c
> > > index 9d19a6d6d9..603b2e4609 100644
> > > --- a/app/test/test_cryptodev_asym.c
> > > +++ b/app/test/test_cryptodev_asym.c
> > > @@ -541,7 +541,7 @@ test_one_case(const void *test_case, int
> > sessionless)
> > >  		printf("  %u) TestCase %s %s\n", test_index++,
> > >  			tc.modex.description, test_msg);
> > >  	} else {
> > > -		for (i = 0; i < RTE_CRYPTO_ASYM_OP_LIST_END; i++) {
> > > +		for (i = 0; i <=
> > RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE; i++) {
> > >  			if (tc.modex.xform_type ==
> > RTE_CRYPTO_ASYM_XFORM_RSA) {
> > >  				if (tc.rsa_data.op_type_flags & (1 << i)) {
> > >  					if (tc.rsa_data.key_exp) {
> > > @@ -1027,7 +1027,7 @@ static inline void print_asym_capa(
> > >  			rte_crypto_asym_xform_strings[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_SHARED_SECRET_COMPUTE;
> > i++) {
> > >  		/* check supported operations */
> > >  		if
> > (rte_cryptodev_asym_xform_capability_check_optype(capa, i))
> > >  			printf(" %s",
> > > diff --git a/drivers/crypto/qat/qat_asym.c
> > > b/drivers/crypto/qat/qat_asym.c index 85973812a8..026625a4d2 100644
> > > --- a/drivers/crypto/qat/qat_asym.c
> > > +++ b/drivers/crypto/qat/qat_asym.c
> > > @@ -742,7 +742,7 @@ qat_asym_session_configure(struct rte_cryptodev
> > *dev,
> > >  			err = -EINVAL;
> > >  			goto error;
> > >  		}
> > > -	} else if (xform->xform_type >=
> > RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END
> > > +	} else if (xform->xform_type > RTE_CRYPTO_ASYM_XFORM_ECPM
> > >  			|| xform->xform_type <=
> > RTE_CRYPTO_ASYM_XFORM_NONE) {
> > >  		QAT_LOG(ERR, "Invalid asymmetric crypto xform");
> > >  		err = -EINVAL;
> > > diff --git a/lib/cryptodev/rte_crypto_asym.h
> > b/lib/cryptodev/rte_crypto_asym.h
> > > index 9c866f553f..5edf658572 100644
> > > --- a/lib/cryptodev/rte_crypto_asym.h
> > > +++ b/lib/cryptodev/rte_crypto_asym.h
> > > @@ -94,8 +94,6 @@ enum rte_crypto_asym_xform_type {
> > >  	 */
> > >  	RTE_CRYPTO_ASYM_XFORM_ECPM,
> > >  	/**< Elliptic Curve Point Multiplication */
> > > -	RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END
> > > -	/**< End of list */
> > >  };
> > >
> > >  /**
> > > @@ -116,7 +114,6 @@ enum rte_crypto_asym_op_type {
> > >  	/**< DH Public Key generation operation */
> > >  	RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE,
> > >  	/**< DH Shared Secret compute operation */
> > > -	RTE_CRYPTO_ASYM_OP_LIST_END
> > >  };
> > >
> > >  /**
> > > @@ -133,7 +130,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
> > >  };
> > >
> > >  /**
> >
> > So I am not sure that this is an improvement.
> > The cryptodev issue we had, was that _LIST_END was being used to size
> > arrays.
> > And that broke when new algorithms got added. Is that an issue, in this case?
> 
> Yes we did this same exercise for symmetric crypto enums earlier.
> Asym enums were left as it was experimental at that point.
> They are still experimental, but thought of making this uniform throughout DPDK
> enums.
> 
> >
> > I am not sure that swapping out _LIST_END, and then littering the code
> > with RTE_CRYPTO_ASYM_XFORM_ECPM and
> > RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE, is an improvement
> here.
> >
> > My 2c is that from an ABI PoV RTE_CRYPTO_ASYM_OP_LIST_END is not
> > better or worse, than RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE?
> >
> > Interested to hear other thoughts.
> 
> I don’t have any better solution for avoiding ABI issues for now.
> The change is for avoiding ABI breakage. But we can drop this patch For now as
> asym is still experimental.

[Anoob] Having LIST_END would preclude new additions to asymmetric algos? If yes, then I would suggest we address it now.

Looking at the "problematic changes", we only have 2-3 application & PMD changes. For unit test application, we could may be do something like,

-               for (i = 0; i < RTE_CRYPTO_ASYM_OP_LIST_END; i++) {
+               enum rte_crypto_asym_op_type types[] = {
+                               RTE_CRYPTO_ASYM_OP_ENCRYPT,
+                               RTE_CRYPTO_ASYM_OP_DECRYPT,
+                               RTE_CRYPTO_ASYM_OP_SIGN,
+                               RTE_CRYPTO_ASYM_OP_VERIFY,
+                               RTE_CRYPTO_ASYM_OP_PRIVATE_KEY_GENERATE,
+                               RTE_CRYPTO_ASYM_OP_PUBLIC_KEY_GENERATE,
+                               RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE,
+               };
+               for (i = 0; i <= RTE_DIM(types); i++) {
                        if (tc.modex.xform_type == RTE_CRYPTO_ASYM_XFORM_RSA) {
-                               if (tc.rsa_data.op_type_flags & (1 << i)) {
+                               if (tc.rsa_data.op_type_flags & (1 << types[i])) {
                                        if (tc.rsa_data.key_exp) {
                                                status = test_cryptodev_asym_op(
                                                        &testsuite_params, &tc,
-                                                       test_msg, sessionless, i,
+                                                       test_msg, sessionless, types[i],
                                                        RTE_RSA_KEY_TYPE_EXP);
                                        }
                                        if (status)
                                                break;
-                                       if (tc.rsa_data.key_qt && (i ==
+                                       if (tc.rsa_data.key_qt && (types[i] ==
                                                        RTE_CRYPTO_ASYM_OP_DECRYPT ||
-                                                       i == RTE_CRYPTO_ASYM_OP_SIGN)) {
+                                                       types[i] == RTE_CRYPTO_ASYM_OP_SIGN)) {
                                                status = test_cryptodev_asym_op(
                                                        &testsuite_params,
-                                                       &tc, test_msg, sessionless, i,
+                                                       &tc, test_msg, sessionless, types[i],
                                                        RTE_RSA_KET_TYPE_QT);
                                        }
                                        if (status)

This way, application would only use the ones which it is designed to work with. For QAT driver changes, we could have an overload if condition (if alg == x || alg = y || ...) to get the same effect. 

Re: [dpdk-dev] [EXT] Re: [PATCH v2 1/3] cryptodev: remove LIST_END enumerators

[Kinsella, Ray]

On 12/10/2021 11:50, Anoob Joseph wrote:
> Hi Ray, Akhil,
> 
> Please see inline.
> 
> Thanks,
> Anoob
> 
>> -----Original Message-----
>> From: Akhil Goyal <gakhil@marvell.com>
>> Sent: Tuesday, October 12, 2021 3:49 PM
>> To: Kinsella, Ray <mdr@ashroe.eu>; dev@dpdk.org
>> Cc: thomas@monjalon.net; david.marchand@redhat.com;
>> hemant.agrawal@nxp.com; Anoob Joseph <anoobj@marvell.com>;
>> pablo.de.lara.guarch@intel.com; fiona.trahe@intel.com;
>> declan.doherty@intel.com; matan@nvidia.com; g.singh@nxp.com;
>> roy.fan.zhang@intel.com; jianjay.zhou@huawei.com; asomalap@amd.com;
>> ruifeng.wang@arm.com; konstantin.ananyev@intel.com;
>> radu.nicolau@intel.com; ajit.khaparde@broadcom.com; Nagadheeraj Rottela
>> <rnagadheeraj@marvell.com>; Ankur Dwivedi <adwivedi@marvell.com>;
>> ciara.power@intel.com; Stephen Hemminger <stephen@networkplumber.org>;
>> Yigit, Ferruh <ferruh.yigit@intel.com>
>> Subject: RE: [EXT] Re: [dpdk-dev] [PATCH v2 1/3] cryptodev: remove LIST_END
>> enumerators
>>
>>>
>>> On 08/10/2021 21:45, Akhil Goyal wrote:
>>>> Remove *_LIST_END enumerators from asymmetric crypto lib to avoid
>>>> ABI breakage for every new addition in enums.
>>>>
>>>> Signed-off-by: Akhil Goyal <gakhil@marvell.com>
>>>> ---
>>>> v2: no change
>>>>
>>>>  app/test/test_cryptodev_asym.c  | 4 ++--
>>>>  drivers/crypto/qat/qat_asym.c   | 2 +-
>>>>  lib/cryptodev/rte_crypto_asym.h | 4 ----
>>>>  3 files changed, 3 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/app/test/test_cryptodev_asym.c
>>> b/app/test/test_cryptodev_asym.c
>>>> index 9d19a6d6d9..603b2e4609 100644
>>>> --- a/app/test/test_cryptodev_asym.c
>>>> +++ b/app/test/test_cryptodev_asym.c
>>>> @@ -541,7 +541,7 @@ test_one_case(const void *test_case, int
>>> sessionless)
>>>>  		printf("  %u) TestCase %s %s\n", test_index++,
>>>>  			tc.modex.description, test_msg);
>>>>  	} else {
>>>> -		for (i = 0; i < RTE_CRYPTO_ASYM_OP_LIST_END; i++) {
>>>> +		for (i = 0; i <=
>>> RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE; i++) {
>>>>  			if (tc.modex.xform_type ==
>>> RTE_CRYPTO_ASYM_XFORM_RSA) {
>>>>  				if (tc.rsa_data.op_type_flags & (1 << i)) {
>>>>  					if (tc.rsa_data.key_exp) {
>>>> @@ -1027,7 +1027,7 @@ static inline void print_asym_capa(
>>>>  			rte_crypto_asym_xform_strings[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_SHARED_SECRET_COMPUTE;
>>> i++) {
>>>>  		/* check supported operations */
>>>>  		if
>>> (rte_cryptodev_asym_xform_capability_check_optype(capa, i))
>>>>  			printf(" %s",
>>>> diff --git a/drivers/crypto/qat/qat_asym.c
>>>> b/drivers/crypto/qat/qat_asym.c index 85973812a8..026625a4d2 100644
>>>> --- a/drivers/crypto/qat/qat_asym.c
>>>> +++ b/drivers/crypto/qat/qat_asym.c
>>>> @@ -742,7 +742,7 @@ qat_asym_session_configure(struct rte_cryptodev
>>> *dev,
>>>>  			err = -EINVAL;
>>>>  			goto error;
>>>>  		}
>>>> -	} else if (xform->xform_type >=
>>> RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END
>>>> +	} else if (xform->xform_type > RTE_CRYPTO_ASYM_XFORM_ECPM
>>>>  			|| xform->xform_type <=
>>> RTE_CRYPTO_ASYM_XFORM_NONE) {
>>>>  		QAT_LOG(ERR, "Invalid asymmetric crypto xform");
>>>>  		err = -EINVAL;
>>>> diff --git a/lib/cryptodev/rte_crypto_asym.h
>>> b/lib/cryptodev/rte_crypto_asym.h
>>>> index 9c866f553f..5edf658572 100644
>>>> --- a/lib/cryptodev/rte_crypto_asym.h
>>>> +++ b/lib/cryptodev/rte_crypto_asym.h
>>>> @@ -94,8 +94,6 @@ enum rte_crypto_asym_xform_type {
>>>>  	 */
>>>>  	RTE_CRYPTO_ASYM_XFORM_ECPM,
>>>>  	/**< Elliptic Curve Point Multiplication */
>>>> -	RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END
>>>> -	/**< End of list */
>>>>  };
>>>>
>>>>  /**
>>>> @@ -116,7 +114,6 @@ enum rte_crypto_asym_op_type {
>>>>  	/**< DH Public Key generation operation */
>>>>  	RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE,
>>>>  	/**< DH Shared Secret compute operation */
>>>> -	RTE_CRYPTO_ASYM_OP_LIST_END
>>>>  };
>>>>
>>>>  /**
>>>> @@ -133,7 +130,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
>>>>  };
>>>>
>>>>  /**
>>>
>>> So I am not sure that this is an improvement.
>>> The cryptodev issue we had, was that _LIST_END was being used to size
>>> arrays.
>>> And that broke when new algorithms got added. Is that an issue, in this case?
>>
>> Yes we did this same exercise for symmetric crypto enums earlier.
>> Asym enums were left as it was experimental at that point.
>> They are still experimental, but thought of making this uniform throughout DPDK
>> enums.
>>
>>>
>>> I am not sure that swapping out _LIST_END, and then littering the code
>>> with RTE_CRYPTO_ASYM_XFORM_ECPM and
>>> RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE, is an improvement
>> here.
>>>
>>> My 2c is that from an ABI PoV RTE_CRYPTO_ASYM_OP_LIST_END is not
>>> better or worse, than RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE?
>>>
>>> Interested to hear other thoughts.
>>
>> I don’t have any better solution for avoiding ABI issues for now.
>> The change is for avoiding ABI breakage. But we can drop this patch For now as
>> asym is still experimental.
> 
> [Anoob] Having LIST_END would preclude new additions to asymmetric algos? If yes, then I would suggest we address it now.

Not at all - but it can be problematic, if two versions of DPDK disagree with the value of LIST_END.

> Looking at the "problematic changes", we only have 2-3 application & PMD changes. For unit test application, we could may be do something like,

The essental functionality not that different, I am just not sure that the verbosity below is helping. 
What you are really trying to guard against is people using LIST_END to size arrays.

> 
> -               for (i = 0; i < RTE_CRYPTO_ASYM_OP_LIST_END; i++) {
> +               enum rte_crypto_asym_op_type types[] = {
> +                               RTE_CRYPTO_ASYM_OP_ENCRYPT,
> +                               RTE_CRYPTO_ASYM_OP_DECRYPT,
> +                               RTE_CRYPTO_ASYM_OP_SIGN,
> +                               RTE_CRYPTO_ASYM_OP_VERIFY,
> +                               RTE_CRYPTO_ASYM_OP_PRIVATE_KEY_GENERATE,
> +                               RTE_CRYPTO_ASYM_OP_PUBLIC_KEY_GENERATE,
> +                               RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE,
> +               };
> +               for (i = 0; i <= RTE_DIM(types); i++) {
>                         if (tc.modex.xform_type == RTE_CRYPTO_ASYM_XFORM_RSA) {
> -                               if (tc.rsa_data.op_type_flags & (1 << i)) {
> +                               if (tc.rsa_data.op_type_flags & (1 << types[i])) {
>                                         if (tc.rsa_data.key_exp) {
>                                                 status = test_cryptodev_asym_op(
>                                                         &testsuite_params, &tc,
> -                                                       test_msg, sessionless, i,
> +                                                       test_msg, sessionless, types[i],
>                                                         RTE_RSA_KEY_TYPE_EXP);
>                                         }
>                                         if (status)
>                                                 break;
> -                                       if (tc.rsa_data.key_qt && (i ==
> +                                       if (tc.rsa_data.key_qt && (types[i] ==
>                                                         RTE_CRYPTO_ASYM_OP_DECRYPT ||
> -                                                       i == RTE_CRYPTO_ASYM_OP_SIGN)) {
> +                                                       types[i] == RTE_CRYPTO_ASYM_OP_SIGN)) {
>                                                 status = test_cryptodev_asym_op(
>                                                         &testsuite_params,
> -                                                       &tc, test_msg, sessionless, i,
> +                                                       &tc, test_msg, sessionless, types[i],
>                                                         RTE_RSA_KET_TYPE_QT);
>                                         }
>                                         if (status)
> 
> This way, application would only use the ones which it is designed to work with. For QAT driver changes, we could have an overload if condition (if alg == x || alg = y || ...) to get the same effect. 
> 

Re: [dpdk-dev] [EXT] Re: [PATCH v2 1/3] cryptodev: remove LIST_END enumerators

[Anoob Joseph]

Hi Ray,

Please see inline.

Thanks,
Anoob

> -----Original Message-----
> From: Kinsella, Ray <mdr@ashroe.eu>
> Sent: Tuesday, October 12, 2021 4:58 PM
> To: Anoob Joseph <anoobj@marvell.com>; Akhil Goyal <gakhil@marvell.com>;
> dev@dpdk.org
> Cc: thomas@monjalon.net; david.marchand@redhat.com;
> hemant.agrawal@nxp.com; pablo.de.lara.guarch@intel.com;
> fiona.trahe@intel.com; declan.doherty@intel.com; matan@nvidia.com;
> g.singh@nxp.com; roy.fan.zhang@intel.com; jianjay.zhou@huawei.com;
> asomalap@amd.com; ruifeng.wang@arm.com; konstantin.ananyev@intel.com;
> radu.nicolau@intel.com; ajit.khaparde@broadcom.com; Nagadheeraj Rottela
> <rnagadheeraj@marvell.com>; Ankur Dwivedi <adwivedi@marvell.com>;
> ciara.power@intel.com; Stephen Hemminger <stephen@networkplumber.org>;
> Yigit, Ferruh <ferruh.yigit@intel.com>
> Subject: Re: [EXT] Re: [dpdk-dev] [PATCH v2 1/3] cryptodev: remove LIST_END
> enumerators
> 
> 
> 
> On 12/10/2021 11:50, Anoob Joseph wrote:
> > Hi Ray, Akhil,
> >
> > Please see inline.
> >
> > Thanks,
> > Anoob
> >
> >> -----Original Message-----
> >> From: Akhil Goyal <gakhil@marvell.com>
> >> Sent: Tuesday, October 12, 2021 3:49 PM
> >> To: Kinsella, Ray <mdr@ashroe.eu>; dev@dpdk.org
> >> Cc: thomas@monjalon.net; david.marchand@redhat.com;
> >> hemant.agrawal@nxp.com; Anoob Joseph <anoobj@marvell.com>;
> >> pablo.de.lara.guarch@intel.com; fiona.trahe@intel.com;
> >> declan.doherty@intel.com; matan@nvidia.com; g.singh@nxp.com;
> >> roy.fan.zhang@intel.com; jianjay.zhou@huawei.com; asomalap@amd.com;
> >> ruifeng.wang@arm.com; konstantin.ananyev@intel.com;
> >> radu.nicolau@intel.com; ajit.khaparde@broadcom.com; Nagadheeraj
> >> Rottela <rnagadheeraj@marvell.com>; Ankur Dwivedi
> >> <adwivedi@marvell.com>; ciara.power@intel.com; Stephen Hemminger
> >> <stephen@networkplumber.org>; Yigit, Ferruh <ferruh.yigit@intel.com>
> >> Subject: RE: [EXT] Re: [dpdk-dev] [PATCH v2 1/3] cryptodev: remove
> >> LIST_END enumerators
> >>
> >>>
> >>> On 08/10/2021 21:45, Akhil Goyal wrote:
> >>>> Remove *_LIST_END enumerators from asymmetric crypto lib to avoid
> >>>> ABI breakage for every new addition in enums.
> >>>>
> >>>> Signed-off-by: Akhil Goyal <gakhil@marvell.com>
> >>>> ---
> >>>> v2: no change
> >>>>
> >>>>  app/test/test_cryptodev_asym.c  | 4 ++--
> >>>>  drivers/crypto/qat/qat_asym.c   | 2 +-
> >>>>  lib/cryptodev/rte_crypto_asym.h | 4 ----
> >>>>  3 files changed, 3 insertions(+), 7 deletions(-)
> >>>>
> >>>> diff --git a/app/test/test_cryptodev_asym.c
> >>> b/app/test/test_cryptodev_asym.c
> >>>> index 9d19a6d6d9..603b2e4609 100644
> >>>> --- a/app/test/test_cryptodev_asym.c
> >>>> +++ b/app/test/test_cryptodev_asym.c
> >>>> @@ -541,7 +541,7 @@ test_one_case(const void *test_case, int
> >>> sessionless)
> >>>>  		printf("  %u) TestCase %s %s\n", test_index++,
> >>>>  			tc.modex.description, test_msg);
> >>>>  	} else {
> >>>> -		for (i = 0; i < RTE_CRYPTO_ASYM_OP_LIST_END; i++) {
> >>>> +		for (i = 0; i <=
> >>> RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE; i++) {
> >>>>  			if (tc.modex.xform_type ==
> >>> RTE_CRYPTO_ASYM_XFORM_RSA) {
> >>>>  				if (tc.rsa_data.op_type_flags & (1 << i)) {
> >>>>  					if (tc.rsa_data.key_exp) {
> >>>> @@ -1027,7 +1027,7 @@ static inline void print_asym_capa(
> >>>>  			rte_crypto_asym_xform_strings[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_SHARED_SECRET_COMPUTE;
> >>> i++) {
> >>>>  		/* check supported operations */
> >>>>  		if
> >>> (rte_cryptodev_asym_xform_capability_check_optype(capa, i))
> >>>>  			printf(" %s",
> >>>> diff --git a/drivers/crypto/qat/qat_asym.c
> >>>> b/drivers/crypto/qat/qat_asym.c index 85973812a8..026625a4d2 100644
> >>>> --- a/drivers/crypto/qat/qat_asym.c
> >>>> +++ b/drivers/crypto/qat/qat_asym.c
> >>>> @@ -742,7 +742,7 @@ qat_asym_session_configure(struct rte_cryptodev
> >>> *dev,
> >>>>  			err = -EINVAL;
> >>>>  			goto error;
> >>>>  		}
> >>>> -	} else if (xform->xform_type >=
> >>> RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END
> >>>> +	} else if (xform->xform_type > RTE_CRYPTO_ASYM_XFORM_ECPM
> >>>>  			|| xform->xform_type <=
> >>> RTE_CRYPTO_ASYM_XFORM_NONE) {
> >>>>  		QAT_LOG(ERR, "Invalid asymmetric crypto xform");
> >>>>  		err = -EINVAL;
> >>>> diff --git a/lib/cryptodev/rte_crypto_asym.h
> >>> b/lib/cryptodev/rte_crypto_asym.h
> >>>> index 9c866f553f..5edf658572 100644
> >>>> --- a/lib/cryptodev/rte_crypto_asym.h
> >>>> +++ b/lib/cryptodev/rte_crypto_asym.h
> >>>> @@ -94,8 +94,6 @@ enum rte_crypto_asym_xform_type {
> >>>>  	 */
> >>>>  	RTE_CRYPTO_ASYM_XFORM_ECPM,
> >>>>  	/**< Elliptic Curve Point Multiplication */
> >>>> -	RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END
> >>>> -	/**< End of list */
> >>>>  };
> >>>>
> >>>>  /**
> >>>> @@ -116,7 +114,6 @@ enum rte_crypto_asym_op_type {
> >>>>  	/**< DH Public Key generation operation */
> >>>>  	RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE,
> >>>>  	/**< DH Shared Secret compute operation */
> >>>> -	RTE_CRYPTO_ASYM_OP_LIST_END
> >>>>  };
> >>>>
> >>>>  /**
> >>>> @@ -133,7 +130,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
> >>>>  };
> >>>>
> >>>>  /**
> >>>
> >>> So I am not sure that this is an improvement.
> >>> The cryptodev issue we had, was that _LIST_END was being used to
> >>> size arrays.
> >>> And that broke when new algorithms got added. Is that an issue, in this
> case?
> >>
> >> Yes we did this same exercise for symmetric crypto enums earlier.
> >> Asym enums were left as it was experimental at that point.
> >> They are still experimental, but thought of making this uniform
> >> throughout DPDK enums.
> >>
> >>>
> >>> I am not sure that swapping out _LIST_END, and then littering the
> >>> code with RTE_CRYPTO_ASYM_XFORM_ECPM and
> >>> RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE, is an improvement
> >> here.
> >>>
> >>> My 2c is that from an ABI PoV RTE_CRYPTO_ASYM_OP_LIST_END is not
> >>> better or worse, than
> RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE?
> >>>
> >>> Interested to hear other thoughts.
> >>
> >> I don’t have any better solution for avoiding ABI issues for now.
> >> The change is for avoiding ABI breakage. But we can drop this patch
> >> For now as asym is still experimental.
> >
> > [Anoob] Having LIST_END would preclude new additions to asymmetric algos?
> If yes, then I would suggest we address it now.
> 
> Not at all - but it can be problematic, if two versions of DPDK disagree with the
> value of LIST_END.
> 
> > Looking at the "problematic changes", we only have 2-3 application &
> > PMD changes. For unit test application, we could may be do something
> > like,
> 
> The essental functionality not that different, I am just not sure that the verbosity
> below is helping.
> What you are really trying to guard against is people using LIST_END to size
> arrays.

[Anoob] Our problem is application using LIST_END (which comes from library) to determine the number of iterations for the loop. My suggestion is to modify the UT such that, we could use RTE_DIM(types) (which comes from application) to determine iterations of loop. This would solve the problem, right?
 
> 
> >
> > -               for (i = 0; i < RTE_CRYPTO_ASYM_OP_LIST_END; i++) {
> > +               enum rte_crypto_asym_op_type types[] = {
> > +                               RTE_CRYPTO_ASYM_OP_ENCRYPT,
> > +                               RTE_CRYPTO_ASYM_OP_DECRYPT,
> > +                               RTE_CRYPTO_ASYM_OP_SIGN,
> > +                               RTE_CRYPTO_ASYM_OP_VERIFY,
> > +                               RTE_CRYPTO_ASYM_OP_PRIVATE_KEY_GENERATE,
> > +                               RTE_CRYPTO_ASYM_OP_PUBLIC_KEY_GENERATE,
> > +                               RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE,
> > +               };
> > +               for (i = 0; i <= RTE_DIM(types); i++) {
> >                         if (tc.modex.xform_type == RTE_CRYPTO_ASYM_XFORM_RSA) {
> > -                               if (tc.rsa_data.op_type_flags & (1 << i)) {
> > +                               if (tc.rsa_data.op_type_flags & (1 <<
> > + types[i])) {
> >                                         if (tc.rsa_data.key_exp) {
> >                                                 status = test_cryptodev_asym_op(
> >                                                         &testsuite_params, &tc,
> > -                                                       test_msg, sessionless, i,
> > +                                                       test_msg,
> > + sessionless, types[i],
> >                                                         RTE_RSA_KEY_TYPE_EXP);
> >                                         }
> >                                         if (status)
> >                                                 break;
> > -                                       if (tc.rsa_data.key_qt && (i ==
> > +                                       if (tc.rsa_data.key_qt &&
> > + (types[i] ==
> >                                                         RTE_CRYPTO_ASYM_OP_DECRYPT ||
> > -                                                       i == RTE_CRYPTO_ASYM_OP_SIGN)) {
> > +                                                       types[i] ==
> > + RTE_CRYPTO_ASYM_OP_SIGN)) {
> >                                                 status = test_cryptodev_asym_op(
> >                                                         &testsuite_params,
> > -                                                       &tc, test_msg, sessionless, i,
> > +                                                       &tc, test_msg,
> > + sessionless, types[i],
> >                                                         RTE_RSA_KET_TYPE_QT);
> >                                         }
> >                                         if (status)
> >
> > This way, application would only use the ones which it is designed to work
> with. For QAT driver changes, we could have an overload if condition (if alg == x
> || alg = y || ...) to get the same effect.
> >

Re: [dpdk-dev] [EXT] Re: [PATCH v2 1/3] cryptodev: remove LIST_END enumerators

[Thomas Monjalon]

12/10/2021 13:34, Anoob Joseph:
> From: Kinsella, Ray <mdr@ashroe.eu>
> > On 12/10/2021 11:50, Anoob Joseph wrote:
> > > From: Akhil Goyal <gakhil@marvell.com>
> > >>> On 08/10/2021 21:45, Akhil Goyal wrote:
> > >>>> Remove *_LIST_END enumerators from asymmetric crypto lib to avoid
> > >>>> ABI breakage for every new addition in enums.
> > >>>>
> > >>>> Signed-off-by: Akhil Goyal <gakhil@marvell.com>
> > >>>> ---
> > >>>> -	} else if (xform->xform_type >=
> > >>> RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END
> > >>>> +	} else if (xform->xform_type > RTE_CRYPTO_ASYM_XFORM_ECPM
[...]
> > >>>
> > >>> So I am not sure that this is an improvement.

Indeed, it is not an improvement.

> > >>> The cryptodev issue we had, was that _LIST_END was being used to
> > >>> size arrays.
> > >>> And that broke when new algorithms got added. Is that an issue, in this
> > case?
> > >>
> > >> Yes we did this same exercise for symmetric crypto enums earlier.
> > >> Asym enums were left as it was experimental at that point.
> > >> They are still experimental, but thought of making this uniform
> > >> throughout DPDK enums.
> > >>
> > >>>
> > >>> I am not sure that swapping out _LIST_END, and then littering the
> > >>> code with RTE_CRYPTO_ASYM_XFORM_ECPM and
> > >>> RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE, is an improvement
> > >> here.
> > >>>
> > >>> My 2c is that from an ABI PoV RTE_CRYPTO_ASYM_OP_LIST_END is not
> > >>> better or worse, than
> > RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE?
> > >>>
> > >>> Interested to hear other thoughts.
> > >>
> > >> I don’t have any better solution for avoiding ABI issues for now.
> > >> The change is for avoiding ABI breakage. But we can drop this patch
> > >> For now as asym is still experimental.
> > >
> > > [Anoob] Having LIST_END would preclude new additions to asymmetric algos?
> > If yes, then I would suggest we address it now.
> > 
> > Not at all - but it can be problematic, if two versions of DPDK disagree with the
> > value of LIST_END.
> > 
> > > Looking at the "problematic changes", we only have 2-3 application &
> > > PMD changes. For unit test application, we could may be do something
> > > like,
> > 
> > The essental functionality not that different, I am just not sure that the verbosity
> > below is helping.
> > What you are really trying to guard against is people using LIST_END to size
> > arrays.
> 
> [Anoob] Our problem is application using LIST_END (which comes from library) to determine the number of iterations for the loop. My suggestion is to modify the UT such that, we could use RTE_DIM(types) (which comes from application) to determine iterations of loop. This would solve the problem, right?

The problem is not the application.
Are you asking the app to define DPDK types?

The problem is in DPDK API. We must not suggest a size for enums.
If we really need a size, then it must be explicit and updated in the lib binary
(through a function) when the size increases.



> > > -               for (i = 0; i < RTE_CRYPTO_ASYM_OP_LIST_END; i++) {
> > > +               enum rte_crypto_asym_op_type types[] = {
> > > +                               RTE_CRYPTO_ASYM_OP_ENCRYPT,
> > > +                               RTE_CRYPTO_ASYM_OP_DECRYPT,
> > > +                               RTE_CRYPTO_ASYM_OP_SIGN,
> > > +                               RTE_CRYPTO_ASYM_OP_VERIFY,
> > > +                               RTE_CRYPTO_ASYM_OP_PRIVATE_KEY_GENERATE,
> > > +                               RTE_CRYPTO_ASYM_OP_PUBLIC_KEY_GENERATE,
> > > +                               RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE,
> > > +               };
> > > +               for (i = 0; i <= RTE_DIM(types); i++) {
> > >                         if (tc.modex.xform_type == RTE_CRYPTO_ASYM_XFORM_RSA) {
> > > -                               if (tc.rsa_data.op_type_flags & (1 << i)) {
> > > +                               if (tc.rsa_data.op_type_flags & (1 <<
> > > + types[i])) {
> > >                                         if (tc.rsa_data.key_exp) {
> > >                                                 status = test_cryptodev_asym_op(
> > >                                                         &testsuite_params, &tc,
> > > -                                                       test_msg, sessionless, i,
> > > +                                                       test_msg,
> > > + sessionless, types[i],
> > >                                                         RTE_RSA_KEY_TYPE_EXP);
> > >                                         }
> > >                                         if (status)
> > >                                                 break;
> > > -                                       if (tc.rsa_data.key_qt && (i ==
> > > +                                       if (tc.rsa_data.key_qt &&
> > > + (types[i] ==
> > >                                                         RTE_CRYPTO_ASYM_OP_DECRYPT ||
> > > -                                                       i == RTE_CRYPTO_ASYM_OP_SIGN)) {
> > > +                                                       types[i] ==
> > > + RTE_CRYPTO_ASYM_OP_SIGN)) {
> > >                                                 status = test_cryptodev_asym_op(
> > >                                                         &testsuite_params,
> > > -                                                       &tc, test_msg, sessionless, i,
> > > +                                                       &tc, test_msg,
> > > + sessionless, types[i],
> > >                                                         RTE_RSA_KET_TYPE_QT);
> > >                                         }
> > >                                         if (status)
> > >
> > > This way, application would only use the ones which it is designed to work
> > with. For QAT driver changes, we could have an overload if condition (if alg == x
> > || alg = y || ...) to get the same effect.

Re: [dpdk-dev] [EXT] Re: [PATCH v2 1/3] cryptodev: remove LIST_END enumerators

[Anoob Joseph]

Hi Thomas,

Please see inline.

Thanks,
Anoob

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Tuesday, October 12, 2021 5:22 PM
> To: Kinsella, Ray <mdr@ashroe.eu>; Akhil Goyal <gakhil@marvell.com>;
> dev@dpdk.org; Anoob Joseph <anoobj@marvell.com>
> Cc: david.marchand@redhat.com; hemant.agrawal@nxp.com;
> pablo.de.lara.guarch@intel.com; fiona.trahe@intel.com;
> declan.doherty@intel.com; matan@nvidia.com; g.singh@nxp.com;
> roy.fan.zhang@intel.com; jianjay.zhou@huawei.com; asomalap@amd.com;
> ruifeng.wang@arm.com; konstantin.ananyev@intel.com;
> radu.nicolau@intel.com; ajit.khaparde@broadcom.com; Nagadheeraj Rottela
> <rnagadheeraj@marvell.com>; Ankur Dwivedi <adwivedi@marvell.com>;
> ciara.power@intel.com; Stephen Hemminger <stephen@networkplumber.org>;
> Yigit, Ferruh <ferruh.yigit@intel.com>
> Subject: Re: [EXT] Re: [dpdk-dev] [PATCH v2 1/3] cryptodev: remove LIST_END
> enumerators
> 
> 12/10/2021 13:34, Anoob Joseph:
> > From: Kinsella, Ray <mdr@ashroe.eu>
> > > On 12/10/2021 11:50, Anoob Joseph wrote:
> > > > From: Akhil Goyal <gakhil@marvell.com>
> > > >>> On 08/10/2021 21:45, Akhil Goyal wrote:
> > > >>>> Remove *_LIST_END enumerators from asymmetric crypto lib to
> > > >>>> avoid ABI breakage for every new addition in enums.
> > > >>>>
> > > >>>> Signed-off-by: Akhil Goyal <gakhil@marvell.com>
> > > >>>> ---
> > > >>>> -	} else if (xform->xform_type >=
> > > >>> RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END
> > > >>>> +	} else if (xform->xform_type >
> RTE_CRYPTO_ASYM_XFORM_ECPM
> [...]
> > > >>>
> > > >>> So I am not sure that this is an improvement.
> 
> Indeed, it is not an improvement.
> 
> > > >>> The cryptodev issue we had, was that _LIST_END was being used to
> > > >>> size arrays.
> > > >>> And that broke when new algorithms got added. Is that an issue,
> > > >>> in this
> > > case?
> > > >>
> > > >> Yes we did this same exercise for symmetric crypto enums earlier.
> > > >> Asym enums were left as it was experimental at that point.
> > > >> They are still experimental, but thought of making this uniform
> > > >> throughout DPDK enums.
> > > >>
> > > >>>
> > > >>> I am not sure that swapping out _LIST_END, and then littering
> > > >>> the code with RTE_CRYPTO_ASYM_XFORM_ECPM and
> > > >>> RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE, is an
> improvement
> > > >> here.
> > > >>>
> > > >>> My 2c is that from an ABI PoV RTE_CRYPTO_ASYM_OP_LIST_END is not
> > > >>> better or worse, than
> > > RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE?
> > > >>>
> > > >>> Interested to hear other thoughts.
> > > >>
> > > >> I don’t have any better solution for avoiding ABI issues for now.
> > > >> The change is for avoiding ABI breakage. But we can drop this
> > > >> patch For now as asym is still experimental.
> > > >
> > > > [Anoob] Having LIST_END would preclude new additions to asymmetric
> algos?
> > > If yes, then I would suggest we address it now.
> > >
> > > Not at all - but it can be problematic, if two versions of DPDK
> > > disagree with the value of LIST_END.
> > >
> > > > Looking at the "problematic changes", we only have 2-3 application
> > > > & PMD changes. For unit test application, we could may be do
> > > > something like,
> > >
> > > The essental functionality not that different, I am just not sure
> > > that the verbosity below is helping.
> > > What you are really trying to guard against is people using LIST_END
> > > to size arrays.
> >
> > [Anoob] Our problem is application using LIST_END (which comes from library)
> to determine the number of iterations for the loop. My suggestion is to modify
> the UT such that, we could use RTE_DIM(types) (which comes from application)
> to determine iterations of loop. This would solve the problem, right?
> 
> The problem is not the application.
> Are you asking the app to define DPDK types?

[Anoob] I didn't understand how you concluded that. The app is supposed to test "n" asymmetric features supported by DPDK. Currently, it does that by looping from 0 to LIST_END which happens to give you the first n features. Now, if we add any new asymmetric feature, LIST_END value would change. Isn't that the very reason why we removed LIST_END from symmetric library and applications?

Now coming to what I proposed, the app is supposed to test "n" asymmetric features. LIST_END helps in doing the loops. If we remove LIST_END, then application will not be in a position to do a loop. My suggestion is, we list the types that are supposed to be tested by the app, and let that array be used as feature list.

PS: Just to reiterate, my proposal is just a local array which would hold DPDK defined RTE enum values for the features that would be tested by this app/function. 
> > > > +               enum rte_crypto_asym_op_type types[] = { 

> 
> The problem is in DPDK API. We must not suggest a size for enums.

[Anoob] So agreed that LIST_END should be removed?
 
> If we really need a size, then it must be explicit and updated in the lib binary
> (through a function) when the size increases.

[Anoob] Precisely my thoughts. The loop with LIST_END done in application is not correct. 
> 
> 
> 
> > > > -               for (i = 0; i < RTE_CRYPTO_ASYM_OP_LIST_END; i++) {
> > > > +               enum rte_crypto_asym_op_type types[] = {
> > > > +                               RTE_CRYPTO_ASYM_OP_ENCRYPT,
> > > > +                               RTE_CRYPTO_ASYM_OP_DECRYPT,
> > > > +                               RTE_CRYPTO_ASYM_OP_SIGN,
> > > > +                               RTE_CRYPTO_ASYM_OP_VERIFY,
> > > > +                               RTE_CRYPTO_ASYM_OP_PRIVATE_KEY_GENERATE,
> > > > +                               RTE_CRYPTO_ASYM_OP_PUBLIC_KEY_GENERATE,
> > > > +                               RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE,
> > > > +               };
> > > > +               for (i = 0; i <= RTE_DIM(types); i++) {
> > > >                         if (tc.modex.xform_type ==
> RTE_CRYPTO_ASYM_XFORM_RSA) {
> > > > -                               if (tc.rsa_data.op_type_flags & (1 << i)) {
> > > > +                               if (tc.rsa_data.op_type_flags & (1
> > > > + <<
> > > > + types[i])) {
> > > >                                         if (tc.rsa_data.key_exp) {
> > > >                                                 status = test_cryptodev_asym_op(
> > > >                                                         &testsuite_params, &tc,
> > > > -                                                       test_msg, sessionless, i,
> > > > +                                                       test_msg,
> > > > + sessionless, types[i],
> > > >                                                         RTE_RSA_KEY_TYPE_EXP);
> > > >                                         }
> > > >                                         if (status)
> > > >                                                 break;
> > > > -                                       if (tc.rsa_data.key_qt && (i ==
> > > > +                                       if (tc.rsa_data.key_qt &&
> > > > + (types[i] ==
> > > >                                                         RTE_CRYPTO_ASYM_OP_DECRYPT ||
> > > > -                                                       i == RTE_CRYPTO_ASYM_OP_SIGN)) {
> > > > +                                                       types[i]
> > > > + ==
> > > > + RTE_CRYPTO_ASYM_OP_SIGN)) {
> > > >                                                 status = test_cryptodev_asym_op(
> > > >                                                         &testsuite_params,
> > > > -                                                       &tc, test_msg, sessionless, i,
> > > > +                                                       &tc,
> > > > + test_msg, sessionless, types[i],
> > > >                                                         RTE_RSA_KET_TYPE_QT);
> > > >                                         }
> > > >                                         if (status)
> > > >
> > > > This way, application would only use the ones which it is designed
> > > > to work
> > > with. For QAT driver changes, we could have an overload if condition
> > > (if alg == x
> > > || alg = y || ...) to get the same effect.
> 
> 
> 

Re: [dpdk-dev] [EXT] Re: [PATCH v2 1/3] cryptodev: remove LIST_END enumerators

[Thomas Monjalon]

12/10/2021 15:38, Anoob Joseph:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 12/10/2021 13:34, Anoob Joseph:
> > > From: Kinsella, Ray <mdr@ashroe.eu>
> > > > On 12/10/2021 11:50, Anoob Joseph wrote:
> > > > > From: Akhil Goyal <gakhil@marvell.com>
> > > > >>> On 08/10/2021 21:45, Akhil Goyal wrote:
> > > > >>>> Remove *_LIST_END enumerators from asymmetric crypto lib to
> > > > >>>> avoid ABI breakage for every new addition in enums.
> > > > >>>>
> > > > >>>> Signed-off-by: Akhil Goyal <gakhil@marvell.com>
> > > > >>>> ---
> > > > >>>> -	} else if (xform->xform_type >=
> > > > >>> RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END
> > > > >>>> +	} else if (xform->xform_type >
> > RTE_CRYPTO_ASYM_XFORM_ECPM
> > [...]
> > > > >>>
> > > > >>> So I am not sure that this is an improvement.
> > 
> > Indeed, it is not an improvement.
> > 
> > > > >>> The cryptodev issue we had, was that _LIST_END was being used to
> > > > >>> size arrays.
> > > > >>> And that broke when new algorithms got added. Is that an issue,
> > > > >>> in this
> > > > case?
> > > > >>
> > > > >> Yes we did this same exercise for symmetric crypto enums earlier.
> > > > >> Asym enums were left as it was experimental at that point.
> > > > >> They are still experimental, but thought of making this uniform
> > > > >> throughout DPDK enums.
> > > > >>
> > > > >>>
> > > > >>> I am not sure that swapping out _LIST_END, and then littering
> > > > >>> the code with RTE_CRYPTO_ASYM_XFORM_ECPM and
> > > > >>> RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE, is an
> > improvement
> > > > >> here.
> > > > >>>
> > > > >>> My 2c is that from an ABI PoV RTE_CRYPTO_ASYM_OP_LIST_END is not
> > > > >>> better or worse, than
> > > > RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE?
> > > > >>>
> > > > >>> Interested to hear other thoughts.
> > > > >>
> > > > >> I don’t have any better solution for avoiding ABI issues for now.
> > > > >> The change is for avoiding ABI breakage. But we can drop this
> > > > >> patch For now as asym is still experimental.
> > > > >
> > > > > [Anoob] Having LIST_END would preclude new additions to asymmetric
> > algos?
> > > > If yes, then I would suggest we address it now.
> > > >
> > > > Not at all - but it can be problematic, if two versions of DPDK
> > > > disagree with the value of LIST_END.
> > > >
> > > > > Looking at the "problematic changes", we only have 2-3 application
> > > > > & PMD changes. For unit test application, we could may be do
> > > > > something like,
> > > >
> > > > The essental functionality not that different, I am just not sure
> > > > that the verbosity below is helping.
> > > > What you are really trying to guard against is people using LIST_END
> > > > to size arrays.
> > >
> > > [Anoob] Our problem is application using LIST_END (which comes from library)
> > to determine the number of iterations for the loop. My suggestion is to modify
> > the UT such that, we could use RTE_DIM(types) (which comes from application)
> > to determine iterations of loop. This would solve the problem, right?
> > 
> > The problem is not the application.
> > Are you asking the app to define DPDK types?
> 
> [Anoob] I didn't understand how you concluded that.

Because you define a specific array in the test app.

> The app is supposed to test "n" asymmetric features supported by DPDK. Currently, it does that by looping from 0 to LIST_END which happens to give you the first n features. Now, if we add any new asymmetric feature, LIST_END value would change. Isn't that the very reason why we removed LIST_END from symmetric library and applications?

Yes

> Now coming to what I proposed, the app is supposed to test "n" asymmetric features. LIST_END helps in doing the loops. If we remove LIST_END, then application will not be in a position to do a loop. My suggestion is, we list the types that are supposed to be tested by the app, and let that array be used as feature list.
> 
> PS: Just to reiterate, my proposal is just a local array which would hold DPDK defined RTE enum values for the features that would be tested by this app/function.

I am more concerned by the general case than the test app.
I think a function returning a number is more app-friendly.

> > > > > +               enum rte_crypto_asym_op_type types[] = { 
> 
> > 
> > The problem is in DPDK API. We must not suggest a size for enums.
> 
> [Anoob] So agreed that LIST_END should be removed?

Yes

> > If we really need a size, then it must be explicit and updated in the lib binary
> > (through a function) when the size increases.
> 
> [Anoob] Precisely my thoughts. The loop with LIST_END done in application is not correct. 
> > 
> > 
> > 
> > > > > -               for (i = 0; i < RTE_CRYPTO_ASYM_OP_LIST_END; i++) {
> > > > > +               enum rte_crypto_asym_op_type types[] = {
> > > > > +                               RTE_CRYPTO_ASYM_OP_ENCRYPT,
> > > > > +                               RTE_CRYPTO_ASYM_OP_DECRYPT,
> > > > > +                               RTE_CRYPTO_ASYM_OP_SIGN,
> > > > > +                               RTE_CRYPTO_ASYM_OP_VERIFY,
> > > > > +                               RTE_CRYPTO_ASYM_OP_PRIVATE_KEY_GENERATE,
> > > > > +                               RTE_CRYPTO_ASYM_OP_PUBLIC_KEY_GENERATE,
> > > > > +                               RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE,
> > > > > +               };
> > > > > +               for (i = 0; i <= RTE_DIM(types); i++) {
> > > > >                         if (tc.modex.xform_type ==
> > RTE_CRYPTO_ASYM_XFORM_RSA) {
> > > > > -                               if (tc.rsa_data.op_type_flags & (1 << i)) {
> > > > > +                               if (tc.rsa_data.op_type_flags & (1
> > > > > + <<
> > > > > + types[i])) {
> > > > >                                         if (tc.rsa_data.key_exp) {
> > > > >                                                 status = test_cryptodev_asym_op(
> > > > >                                                         &testsuite_params, &tc,
> > > > > -                                                       test_msg, sessionless, i,
> > > > > +                                                       test_msg,
> > > > > + sessionless, types[i],
> > > > >                                                         RTE_RSA_KEY_TYPE_EXP);
> > > > >                                         }
> > > > >                                         if (status)
> > > > >                                                 break;
> > > > > -                                       if (tc.rsa_data.key_qt && (i ==
> > > > > +                                       if (tc.rsa_data.key_qt &&
> > > > > + (types[i] ==
> > > > >                                                         RTE_CRYPTO_ASYM_OP_DECRYPT ||
> > > > > -                                                       i == RTE_CRYPTO_ASYM_OP_SIGN)) {
> > > > > +                                                       types[i]
> > > > > + ==
> > > > > + RTE_CRYPTO_ASYM_OP_SIGN)) {
> > > > >                                                 status = test_cryptodev_asym_op(
> > > > >                                                         &testsuite_params,
> > > > > -                                                       &tc, test_msg, sessionless, i,
> > > > > +                                                       &tc,
> > > > > + test_msg, sessionless, types[i],
> > > > >                                                         RTE_RSA_KET_TYPE_QT);
> > > > >                                         }
> > > > >                                         if (status)
> > > > >
> > > > > This way, application would only use the ones which it is designed
> > > > > to work
> > > > with. For QAT driver changes, we could have an overload if condition
> > > > (if alg == x
> > > > || alg = y || ...) to get the same effect.

Re: [dpdk-dev] [EXT] Re: [PATCH v2 1/3] cryptodev: remove LIST_END enumerators

[Anoob Joseph]

Hi Thomas,

Please see inline.

Thanks,
Anoob

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Tuesday, October 12, 2021 7:25 PM
> To: Kinsella, Ray <mdr@ashroe.eu>; Akhil Goyal <gakhil@marvell.com>;
> dev@dpdk.org; Anoob Joseph <anoobj@marvell.com>
> Cc: david.marchand@redhat.com; hemant.agrawal@nxp.com;
> pablo.de.lara.guarch@intel.com; fiona.trahe@intel.com;
> declan.doherty@intel.com; matan@nvidia.com; g.singh@nxp.com;
> roy.fan.zhang@intel.com; jianjay.zhou@huawei.com; asomalap@amd.com;
> ruifeng.wang@arm.com; konstantin.ananyev@intel.com;
> radu.nicolau@intel.com; ajit.khaparde@broadcom.com; Nagadheeraj Rottela
> <rnagadheeraj@marvell.com>; Ankur Dwivedi <adwivedi@marvell.com>;
> ciara.power@intel.com; Stephen Hemminger <stephen@networkplumber.org>;
> Yigit, Ferruh <ferruh.yigit@intel.com>; bruce.richardson@intel.com
> Subject: Re: [EXT] Re: [dpdk-dev] [PATCH v2 1/3] cryptodev: remove LIST_END
> enumerators
> 
> 12/10/2021 15:38, Anoob Joseph:
> > From: Thomas Monjalon <thomas@monjalon.net>
> > > 12/10/2021 13:34, Anoob Joseph:
> > > > From: Kinsella, Ray <mdr@ashroe.eu>
> > > > > On 12/10/2021 11:50, Anoob Joseph wrote:
> > > > > > From: Akhil Goyal <gakhil@marvell.com>
> > > > > >>> On 08/10/2021 21:45, Akhil Goyal wrote:
> > > > > >>>> Remove *_LIST_END enumerators from asymmetric crypto lib to
> > > > > >>>> avoid ABI breakage for every new addition in enums.
> > > > > >>>>
> > > > > >>>> Signed-off-by: Akhil Goyal <gakhil@marvell.com>
> > > > > >>>> ---
> > > > > >>>> -	} else if (xform->xform_type >=
> > > > > >>> RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END
> > > > > >>>> +	} else if (xform->xform_type >
> > > RTE_CRYPTO_ASYM_XFORM_ECPM
> > > [...]
> > > > > >>>
> > > > > >>> So I am not sure that this is an improvement.
> > >
> > > Indeed, it is not an improvement.
> > >
> > > > > >>> The cryptodev issue we had, was that _LIST_END was being
> > > > > >>> used to size arrays.
> > > > > >>> And that broke when new algorithms got added. Is that an
> > > > > >>> issue, in this
> > > > > case?
> > > > > >>
> > > > > >> Yes we did this same exercise for symmetric crypto enums earlier.
> > > > > >> Asym enums were left as it was experimental at that point.
> > > > > >> They are still experimental, but thought of making this
> > > > > >> uniform throughout DPDK enums.
> > > > > >>
> > > > > >>>
> > > > > >>> I am not sure that swapping out _LIST_END, and then
> > > > > >>> littering the code with RTE_CRYPTO_ASYM_XFORM_ECPM and
> > > > > >>> RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE, is an
> > > improvement
> > > > > >> here.
> > > > > >>>
> > > > > >>> My 2c is that from an ABI PoV RTE_CRYPTO_ASYM_OP_LIST_END is
> > > > > >>> not better or worse, than
> > > > > RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE?
> > > > > >>>
> > > > > >>> Interested to hear other thoughts.
> > > > > >>
> > > > > >> I don’t have any better solution for avoiding ABI issues for now.
> > > > > >> The change is for avoiding ABI breakage. But we can drop this
> > > > > >> patch For now as asym is still experimental.
> > > > > >
> > > > > > [Anoob] Having LIST_END would preclude new additions to
> > > > > > asymmetric
> > > algos?
> > > > > If yes, then I would suggest we address it now.
> > > > >
> > > > > Not at all - but it can be problematic, if two versions of DPDK
> > > > > disagree with the value of LIST_END.
> > > > >
> > > > > > Looking at the "problematic changes", we only have 2-3
> > > > > > application & PMD changes. For unit test application, we could
> > > > > > may be do something like,
> > > > >
> > > > > The essental functionality not that different, I am just not
> > > > > sure that the verbosity below is helping.
> > > > > What you are really trying to guard against is people using
> > > > > LIST_END to size arrays.
> > > >
> > > > [Anoob] Our problem is application using LIST_END (which comes
> > > > from library)
> > > to determine the number of iterations for the loop. My suggestion is
> > > to modify the UT such that, we could use RTE_DIM(types) (which comes
> > > from application) to determine iterations of loop. This would solve the
> problem, right?
> > >
> > > The problem is not the application.
> > > Are you asking the app to define DPDK types?
> >
> > [Anoob] I didn't understand how you concluded that.
> 
> Because you define a specific array in the test app.
> 
> > The app is supposed to test "n" asymmetric features supported by DPDK.
> Currently, it does that by looping from 0 to LIST_END which happens to give you
> the first n features. Now, if we add any new asymmetric feature, LIST_END
> value would change. Isn't that the very reason why we removed LIST_END from
> symmetric library and applications?
> 
> Yes
> 
> > Now coming to what I proposed, the app is supposed to test "n" asymmetric
> features. LIST_END helps in doing the loops. If we remove LIST_END, then
> application will not be in a position to do a loop. My suggestion is, we list the
> types that are supposed to be tested by the app, and let that array be used as
> feature list.
> >
> > PS: Just to reiterate, my proposal is just a local array which would hold DPDK
> defined RTE enum values for the features that would be tested by this
> app/function.
> 
> I am more concerned by the general case than the test app.
> I think a function returning a number is more app-friendly.

[Anoob] Indeed. But there are 3 LIST_ENDs removed with this patch. Do you propose 3 new APIs to just get max number? 
 
> 
> > > > > > +               enum rte_crypto_asym_op_type types[] = {
> >
> > >
> > > The problem is in DPDK API. We must not suggest a size for enums.
> >
> > [Anoob] So agreed that LIST_END should be removed?
> 
> Yes
> 
> > > If we really need a size, then it must be explicit and updated in
> > > the lib binary (through a function) when the size increases.
> >
> > [Anoob] Precisely my thoughts. The loop with LIST_END done in application is
> not correct.
> > >
> > >
> > >
> > > > > > -               for (i = 0; i < RTE_CRYPTO_ASYM_OP_LIST_END; i++) {
> > > > > > +               enum rte_crypto_asym_op_type types[] = {
> > > > > > +                               RTE_CRYPTO_ASYM_OP_ENCRYPT,
> > > > > > +                               RTE_CRYPTO_ASYM_OP_DECRYPT,
> > > > > > +                               RTE_CRYPTO_ASYM_OP_SIGN,
> > > > > > +                               RTE_CRYPTO_ASYM_OP_VERIFY,
> > > > > > +                               RTE_CRYPTO_ASYM_OP_PRIVATE_KEY_GENERATE,
> > > > > > +                               RTE_CRYPTO_ASYM_OP_PUBLIC_KEY_GENERATE,
> > > > > > +
> RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE,
> > > > > > +               };
> > > > > > +               for (i = 0; i <= RTE_DIM(types); i++) {
> > > > > >                         if (tc.modex.xform_type ==
> > > RTE_CRYPTO_ASYM_XFORM_RSA) {
> > > > > > -                               if (tc.rsa_data.op_type_flags & (1 << i)) {
> > > > > > +                               if (tc.rsa_data.op_type_flags
> > > > > > + & (1 <<
> > > > > > + types[i])) {
> > > > > >                                         if (tc.rsa_data.key_exp) {
> > > > > >                                                 status = test_cryptodev_asym_op(
> > > > > >                                                         &testsuite_params, &tc,
> > > > > > -                                                       test_msg, sessionless, i,
> > > > > > +
> > > > > > + test_msg, sessionless, types[i],
> > > > > >                                                         RTE_RSA_KEY_TYPE_EXP);
> > > > > >                                         }
> > > > > >                                         if (status)
> > > > > >                                                 break;
> > > > > > -                                       if (tc.rsa_data.key_qt && (i ==
> > > > > > +                                       if (tc.rsa_data.key_qt
> > > > > > + && (types[i] ==
> > > > > >                                                         RTE_CRYPTO_ASYM_OP_DECRYPT ||
> > > > > > -                                                       i == RTE_CRYPTO_ASYM_OP_SIGN)) {
> > > > > > +
> > > > > > + types[i] ==
> > > > > > + RTE_CRYPTO_ASYM_OP_SIGN)) {
> > > > > >                                                 status = test_cryptodev_asym_op(
> > > > > >                                                         &testsuite_params,
> > > > > > -                                                       &tc, test_msg, sessionless, i,
> > > > > > +                                                       &tc,
> > > > > > + test_msg, sessionless, types[i],
> > > > > >                                                         RTE_RSA_KET_TYPE_QT);
> > > > > >                                         }
> > > > > >                                         if (status)
> > > > > >
> > > > > > This way, application would only use the ones which it is
> > > > > > designed to work
> > > > > with. For QAT driver changes, we could have an overload if
> > > > > condition (if alg == x
> > > > > || alg = y || ...) to get the same effect.
> 
> 

Re: [dpdk-dev] [EXT] Re: [PATCH v2 1/3] cryptodev: remove LIST_END enumerators

[Kinsella, Ray]

On 12/10/2021 15:18, Anoob Joseph wrote:
> Hi Thomas,
> 
> Please see inline.
> 
> Thanks,
> Anoob
> 
>> -----Original Message-----
>> From: Thomas Monjalon <thomas@monjalon.net>
>> Sent: Tuesday, October 12, 2021 7:25 PM
>> To: Kinsella, Ray <mdr@ashroe.eu>; Akhil Goyal <gakhil@marvell.com>;
>> dev@dpdk.org; Anoob Joseph <anoobj@marvell.com>
>> Cc: david.marchand@redhat.com; hemant.agrawal@nxp.com;
>> pablo.de.lara.guarch@intel.com; fiona.trahe@intel.com;
>> declan.doherty@intel.com; matan@nvidia.com; g.singh@nxp.com;
>> roy.fan.zhang@intel.com; jianjay.zhou@huawei.com; asomalap@amd.com;
>> ruifeng.wang@arm.com; konstantin.ananyev@intel.com;
>> radu.nicolau@intel.com; ajit.khaparde@broadcom.com; Nagadheeraj Rottela
>> <rnagadheeraj@marvell.com>; Ankur Dwivedi <adwivedi@marvell.com>;
>> ciara.power@intel.com; Stephen Hemminger <stephen@networkplumber.org>;
>> Yigit, Ferruh <ferruh.yigit@intel.com>; bruce.richardson@intel.com
>> Subject: Re: [EXT] Re: [dpdk-dev] [PATCH v2 1/3] cryptodev: remove LIST_END
>> enumerators
>>
>> 12/10/2021 15:38, Anoob Joseph:
>>> From: Thomas Monjalon <thomas@monjalon.net>
>>>> 12/10/2021 13:34, Anoob Joseph:
>>>>> From: Kinsella, Ray <mdr@ashroe.eu>
>>>>>> On 12/10/2021 11:50, Anoob Joseph wrote:
>>>>>>> From: Akhil Goyal <gakhil@marvell.com>
>>>>>>>>> On 08/10/2021 21:45, Akhil Goyal wrote:
>>>>>>>>>> Remove *_LIST_END enumerators from asymmetric crypto lib to
>>>>>>>>>> avoid ABI breakage for every new addition in enums.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Akhil Goyal <gakhil@marvell.com>
>>>>>>>>>> ---
>>>>>>>>>> -	} else if (xform->xform_type >=
>>>>>>>>> RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END
>>>>>>>>>> +	} else if (xform->xform_type >
>>>> RTE_CRYPTO_ASYM_XFORM_ECPM
>>>> [...]
>>>>>>>>>
>>>>>>>>> So I am not sure that this is an improvement.
>>>>
>>>> Indeed, it is not an improvement.
>>>>
>>>>>>>>> The cryptodev issue we had, was that _LIST_END was being
>>>>>>>>> used to size arrays.
>>>>>>>>> And that broke when new algorithms got added. Is that an
>>>>>>>>> issue, in this
>>>>>> case?
>>>>>>>>
>>>>>>>> Yes we did this same exercise for symmetric crypto enums earlier.
>>>>>>>> Asym enums were left as it was experimental at that point.
>>>>>>>> They are still experimental, but thought of making this
>>>>>>>> uniform throughout DPDK enums.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> I am not sure that swapping out _LIST_END, and then
>>>>>>>>> littering the code with RTE_CRYPTO_ASYM_XFORM_ECPM and
>>>>>>>>> RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE, is an
>>>> improvement
>>>>>>>> here.
>>>>>>>>>
>>>>>>>>> My 2c is that from an ABI PoV RTE_CRYPTO_ASYM_OP_LIST_END is
>>>>>>>>> not better or worse, than
>>>>>> RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE?
>>>>>>>>>
>>>>>>>>> Interested to hear other thoughts.
>>>>>>>>
>>>>>>>> I don’t have any better solution for avoiding ABI issues for now.
>>>>>>>> The change is for avoiding ABI breakage. But we can drop this
>>>>>>>> patch For now as asym is still experimental.
>>>>>>>
>>>>>>> [Anoob] Having LIST_END would preclude new additions to
>>>>>>> asymmetric
>>>> algos?
>>>>>> If yes, then I would suggest we address it now.
>>>>>>
>>>>>> Not at all - but it can be problematic, if two versions of DPDK
>>>>>> disagree with the value of LIST_END.
>>>>>>
>>>>>>> Looking at the "problematic changes", we only have 2-3
>>>>>>> application & PMD changes. For unit test application, we could
>>>>>>> may be do something like,
>>>>>>
>>>>>> The essental functionality not that different, I am just not
>>>>>> sure that the verbosity below is helping.
>>>>>> What you are really trying to guard against is people using
>>>>>> LIST_END to size arrays.
>>>>>
>>>>> [Anoob] Our problem is application using LIST_END (which comes
>>>>> from library)
>>>> to determine the number of iterations for the loop. My suggestion is
>>>> to modify the UT such that, we could use RTE_DIM(types) (which comes
>>>> from application) to determine iterations of loop. This would solve the
>> problem, right?
>>>>
>>>> The problem is not the application.
>>>> Are you asking the app to define DPDK types?
>>>
>>> [Anoob] I didn't understand how you concluded that.
>>
>> Because you define a specific array in the test app.
>>
>>> The app is supposed to test "n" asymmetric features supported by DPDK.
>> Currently, it does that by looping from 0 to LIST_END which happens to give you
>> the first n features. Now, if we add any new asymmetric feature, LIST_END
>> value would change. Isn't that the very reason why we removed LIST_END from
>> symmetric library and applications?
>>
>> Yes
>>
>>> Now coming to what I proposed, the app is supposed to test "n" asymmetric
>> features. LIST_END helps in doing the loops. If we remove LIST_END, then
>> application will not be in a position to do a loop. My suggestion is, we list the
>> types that are supposed to be tested by the app, and let that array be used as
>> feature list.
>>>
>>> PS: Just to reiterate, my proposal is just a local array which would hold DPDK
>> defined RTE enum values for the features that would be tested by this
>> app/function.
>>
>> I am more concerned by the general case than the test app.
>> I think a function returning a number is more app-friendly.
> 
> [Anoob] Indeed. But there are 3 LIST_ENDs removed with this patch. Do you propose 3 new APIs to just get max number? 

1 API returning a single "info" structure perhaps - as being the most extensible?

>  
>>
>>>>>>> +               enum rte_crypto_asym_op_type types[] = {
>>>
>>>>
>>>> The problem is in DPDK API. We must not suggest a size for enums.
>>>
>>> [Anoob] So agreed that LIST_END should be removed?
>>
>> Yes
>>
>>>> If we really need a size, then it must be explicit and updated in
>>>> the lib binary (through a function) when the size increases.
>>>
>>> [Anoob] Precisely my thoughts. The loop with LIST_END done in application is
>> not correct.
>>>>
>>>>
>>>>
>>>>>>> -               for (i = 0; i < RTE_CRYPTO_ASYM_OP_LIST_END; i++) {
>>>>>>> +               enum rte_crypto_asym_op_type types[] = {
>>>>>>> +                               RTE_CRYPTO_ASYM_OP_ENCRYPT,
>>>>>>> +                               RTE_CRYPTO_ASYM_OP_DECRYPT,
>>>>>>> +                               RTE_CRYPTO_ASYM_OP_SIGN,
>>>>>>> +                               RTE_CRYPTO_ASYM_OP_VERIFY,
>>>>>>> +                               RTE_CRYPTO_ASYM_OP_PRIVATE_KEY_GENERATE,
>>>>>>> +                               RTE_CRYPTO_ASYM_OP_PUBLIC_KEY_GENERATE,
>>>>>>> +
>> RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE,
>>>>>>> +               };
>>>>>>> +               for (i = 0; i <= RTE_DIM(types); i++) {
>>>>>>>                         if (tc.modex.xform_type ==
>>>> RTE_CRYPTO_ASYM_XFORM_RSA) {
>>>>>>> -                               if (tc.rsa_data.op_type_flags & (1 << i)) {
>>>>>>> +                               if (tc.rsa_data.op_type_flags
>>>>>>> + & (1 <<
>>>>>>> + types[i])) {
>>>>>>>                                         if (tc.rsa_data.key_exp) {
>>>>>>>                                                 status = test_cryptodev_asym_op(
>>>>>>>                                                         &testsuite_params, &tc,
>>>>>>> -                                                       test_msg, sessionless, i,
>>>>>>> +
>>>>>>> + test_msg, sessionless, types[i],
>>>>>>>                                                         RTE_RSA_KEY_TYPE_EXP);
>>>>>>>                                         }
>>>>>>>                                         if (status)
>>>>>>>                                                 break;
>>>>>>> -                                       if (tc.rsa_data.key_qt && (i ==
>>>>>>> +                                       if (tc.rsa_data.key_qt
>>>>>>> + && (types[i] ==
>>>>>>>                                                         RTE_CRYPTO_ASYM_OP_DECRYPT ||
>>>>>>> -                                                       i == RTE_CRYPTO_ASYM_OP_SIGN)) {
>>>>>>> +
>>>>>>> + types[i] ==
>>>>>>> + RTE_CRYPTO_ASYM_OP_SIGN)) {
>>>>>>>                                                 status = test_cryptodev_asym_op(
>>>>>>>                                                         &testsuite_params,
>>>>>>> -                                                       &tc, test_msg, sessionless, i,
>>>>>>> +                                                       &tc,
>>>>>>> + test_msg, sessionless, types[i],
>>>>>>>                                                         RTE_RSA_KET_TYPE_QT);
>>>>>>>                                         }
>>>>>>>                                         if (status)
>>>>>>>
>>>>>>> This way, application would only use the ones which it is
>>>>>>> designed to work
>>>>>> with. For QAT driver changes, we could have an overload if
>>>>>> condition (if alg == x
>>>>>> || alg = y || ...) to get the same effect.
>>
>>
> 

Re: [dpdk-dev] [EXT] Re: [PATCH v2 1/3] cryptodev: remove LIST_END enumerators

[Thomas Monjalon]

12/10/2021 16:47, Kinsella, Ray:
> On 12/10/2021 15:18, Anoob Joseph wrote:
> > From: Thomas Monjalon <thomas@monjalon.net>
> >> 12/10/2021 15:38, Anoob Joseph:
> >>> From: Thomas Monjalon <thomas@monjalon.net>
> >>>> 12/10/2021 13:34, Anoob Joseph:
> >>>>> From: Kinsella, Ray <mdr@ashroe.eu>
> >>>>>> On 12/10/2021 11:50, Anoob Joseph wrote:
> >>>>>>> From: Akhil Goyal <gakhil@marvell.com>
> >>>>>>>>> On 08/10/2021 21:45, Akhil Goyal wrote:
> >>>>>>>>>> Remove *_LIST_END enumerators from asymmetric crypto lib to
> >>>>>>>>>> avoid ABI breakage for every new addition in enums.
> >>>>>>>>>>
> >>>>>>>>>> Signed-off-by: Akhil Goyal <gakhil@marvell.com>
> >>>>>>>>>> ---
> >>>>>>>>>> -	} else if (xform->xform_type >=
> >>>>>>>>> RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END
> >>>>>>>>>> +	} else if (xform->xform_type >
> >>>> RTE_CRYPTO_ASYM_XFORM_ECPM
> >>>> [...]
> >>>>>>>>>
> >>>>>>>>> So I am not sure that this is an improvement.
> >>>>
> >>>> Indeed, it is not an improvement.
> >>>>
> >>>>>>>>> The cryptodev issue we had, was that _LIST_END was being
> >>>>>>>>> used to size arrays.
> >>>>>>>>> And that broke when new algorithms got added. Is that an
> >>>>>>>>> issue, in this
> >>>>>> case?
> >>>>>>>>
> >>>>>>>> Yes we did this same exercise for symmetric crypto enums earlier.
> >>>>>>>> Asym enums were left as it was experimental at that point.
> >>>>>>>> They are still experimental, but thought of making this
> >>>>>>>> uniform throughout DPDK enums.
> >>>>>>>>
> >>>>>>>>>
> >>>>>>>>> I am not sure that swapping out _LIST_END, and then
> >>>>>>>>> littering the code with RTE_CRYPTO_ASYM_XFORM_ECPM and
> >>>>>>>>> RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE, is an
> >>>> improvement
> >>>>>>>> here.
> >>>>>>>>>
> >>>>>>>>> My 2c is that from an ABI PoV RTE_CRYPTO_ASYM_OP_LIST_END is
> >>>>>>>>> not better or worse, than
> >>>>>> RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE?
> >>>>>>>>>
> >>>>>>>>> Interested to hear other thoughts.
> >>>>>>>>
> >>>>>>>> I don’t have any better solution for avoiding ABI issues for now.
> >>>>>>>> The change is for avoiding ABI breakage. But we can drop this
> >>>>>>>> patch For now as asym is still experimental.
> >>>>>>>
> >>>>>>> [Anoob] Having LIST_END would preclude new additions to
> >>>>>>> asymmetric
> >>>> algos?
> >>>>>> If yes, then I would suggest we address it now.
> >>>>>>
> >>>>>> Not at all - but it can be problematic, if two versions of DPDK
> >>>>>> disagree with the value of LIST_END.
> >>>>>>
> >>>>>>> Looking at the "problematic changes", we only have 2-3
> >>>>>>> application & PMD changes. For unit test application, we could
> >>>>>>> may be do something like,
> >>>>>>
> >>>>>> The essental functionality not that different, I am just not
> >>>>>> sure that the verbosity below is helping.
> >>>>>> What you are really trying to guard against is people using
> >>>>>> LIST_END to size arrays.
> >>>>>
> >>>>> [Anoob] Our problem is application using LIST_END (which comes
> >>>>> from library)
> >>>> to determine the number of iterations for the loop. My suggestion is
> >>>> to modify the UT such that, we could use RTE_DIM(types) (which comes
> >>>> from application) to determine iterations of loop. This would solve the
> >> problem, right?
> >>>>
> >>>> The problem is not the application.
> >>>> Are you asking the app to define DPDK types?
> >>>
> >>> [Anoob] I didn't understand how you concluded that.
> >>
> >> Because you define a specific array in the test app.
> >>
> >>> The app is supposed to test "n" asymmetric features supported by DPDK.
> >> Currently, it does that by looping from 0 to LIST_END which happens to give you
> >> the first n features. Now, if we add any new asymmetric feature, LIST_END
> >> value would change. Isn't that the very reason why we removed LIST_END from
> >> symmetric library and applications?
> >>
> >> Yes
> >>
> >>> Now coming to what I proposed, the app is supposed to test "n" asymmetric
> >> features. LIST_END helps in doing the loops. If we remove LIST_END, then
> >> application will not be in a position to do a loop. My suggestion is, we list the
> >> types that are supposed to be tested by the app, and let that array be used as
> >> feature list.
> >>>
> >>> PS: Just to reiterate, my proposal is just a local array which would hold DPDK
> >> defined RTE enum values for the features that would be tested by this
> >> app/function.
> >>
> >> I am more concerned by the general case than the test app.
> >> I think a function returning a number is more app-friendly.
> > 
> > [Anoob] Indeed. But there are 3 LIST_ENDs removed with this patch. Do you propose 3 new APIs to just get max number? 
> 
> 1 API returning a single "info" structure perhaps - as being the most extensible?

Or 3 iterators (foreach construct).
Instead of just returning a size, we can have an iterator for each enum
which needs to be iterated.

Feel free to consider the alternative which fits the best in cryptodev.

Re: [dpdk-dev] [EXT] Re: [PATCH v2 1/3] cryptodev: remove LIST_END enumerators

[Anoob Joseph]

Hi Thomas, Ray,

Please see inline.

Thanks,
Anoob

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Tuesday, October 12, 2021 8:37 PM
> To: Anoob Joseph <anoobj@marvell.com>; Akhil Goyal
> <gakhil@marvell.com>; dev@dpdk.org; Kinsella, Ray <mdr@ashroe.eu>
> Cc: david.marchand@redhat.com; hemant.agrawal@nxp.com;
> pablo.de.lara.guarch@intel.com; fiona.trahe@intel.com;
> declan.doherty@intel.com; matan@nvidia.com; g.singh@nxp.com;
> roy.fan.zhang@intel.com; jianjay.zhou@huawei.com; asomalap@amd.com;
> ruifeng.wang@arm.com; konstantin.ananyev@intel.com;
> radu.nicolau@intel.com; ajit.khaparde@broadcom.com; Nagadheeraj
> Rottela <rnagadheeraj@marvell.com>; Ankur Dwivedi
> <adwivedi@marvell.com>; ciara.power@intel.com; Stephen Hemminger
> <stephen@networkplumber.org>; Yigit, Ferruh <ferruh.yigit@intel.com>;
> bruce.richardson@intel.com
> Subject: Re: [EXT] Re: [dpdk-dev] [PATCH v2 1/3] cryptodev: remove
> LIST_END enumerators
> 
> 12/10/2021 16:47, Kinsella, Ray:
> > On 12/10/2021 15:18, Anoob Joseph wrote:
> > > From: Thomas Monjalon <thomas@monjalon.net>
> > >> 12/10/2021 15:38, Anoob Joseph:
> > >>> From: Thomas Monjalon <thomas@monjalon.net>
> > >>>> 12/10/2021 13:34, Anoob Joseph:
> > >>>>> From: Kinsella, Ray <mdr@ashroe.eu>
> > >>>>>> On 12/10/2021 11:50, Anoob Joseph wrote:
> > >>>>>>> From: Akhil Goyal <gakhil@marvell.com>
> > >>>>>>>>> On 08/10/2021 21:45, Akhil Goyal wrote:
> > >>>>>>>>>> Remove *_LIST_END enumerators from asymmetric crypto
> lib to
> > >>>>>>>>>> avoid ABI breakage for every new addition in enums.
> > >>>>>>>>>>
> > >>>>>>>>>> Signed-off-by: Akhil Goyal <gakhil@marvell.com>
> > >>>>>>>>>> ---
> > >>>>>>>>>> -	} else if (xform->xform_type >=
> > >>>>>>>>> RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END
> > >>>>>>>>>> +	} else if (xform->xform_type >
> > >>>> RTE_CRYPTO_ASYM_XFORM_ECPM
> > >>>> [...]
> > >>>>>>>>>
> > >>>>>>>>> So I am not sure that this is an improvement.
> > >>>>
> > >>>> Indeed, it is not an improvement.
> > >>>>
> > >>>>>>>>> The cryptodev issue we had, was that _LIST_END was being
> > >>>>>>>>> used to size arrays.
> > >>>>>>>>> And that broke when new algorithms got added. Is that an
> > >>>>>>>>> issue, in this
> > >>>>>> case?
> > >>>>>>>>
> > >>>>>>>> Yes we did this same exercise for symmetric crypto enums
> earlier.
> > >>>>>>>> Asym enums were left as it was experimental at that point.
> > >>>>>>>> They are still experimental, but thought of making this
> > >>>>>>>> uniform throughout DPDK enums.
> > >>>>>>>>
> > >>>>>>>>>
> > >>>>>>>>> I am not sure that swapping out _LIST_END, and then
> > >>>>>>>>> littering the code with RTE_CRYPTO_ASYM_XFORM_ECPM and
> > >>>>>>>>> RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE, is an
> > >>>> improvement
> > >>>>>>>> here.
> > >>>>>>>>>
> > >>>>>>>>> My 2c is that from an ABI PoV
> RTE_CRYPTO_ASYM_OP_LIST_END is
> > >>>>>>>>> not better or worse, than
> > >>>>>> RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE?
> > >>>>>>>>>
> > >>>>>>>>> Interested to hear other thoughts.
> > >>>>>>>>
> > >>>>>>>> I don’t have any better solution for avoiding ABI issues for now.
> > >>>>>>>> The change is for avoiding ABI breakage. But we can drop this
> > >>>>>>>> patch For now as asym is still experimental.
> > >>>>>>>
> > >>>>>>> [Anoob] Having LIST_END would preclude new additions to
> > >>>>>>> asymmetric
> > >>>> algos?
> > >>>>>> If yes, then I would suggest we address it now.
> > >>>>>>
> > >>>>>> Not at all - but it can be problematic, if two versions of DPDK
> > >>>>>> disagree with the value of LIST_END.
> > >>>>>>
> > >>>>>>> Looking at the "problematic changes", we only have 2-3
> > >>>>>>> application & PMD changes. For unit test application, we could
> > >>>>>>> may be do something like,
> > >>>>>>
> > >>>>>> The essental functionality not that different, I am just not
> > >>>>>> sure that the verbosity below is helping.
> > >>>>>> What you are really trying to guard against is people using
> > >>>>>> LIST_END to size arrays.
> > >>>>>
> > >>>>> [Anoob] Our problem is application using LIST_END (which comes
> > >>>>> from library)
> > >>>> to determine the number of iterations for the loop. My suggestion
> > >>>> is to modify the UT such that, we could use RTE_DIM(types) (which
> > >>>> comes from application) to determine iterations of loop. This
> > >>>> would solve the
> > >> problem, right?
> > >>>>
> > >>>> The problem is not the application.
> > >>>> Are you asking the app to define DPDK types?
> > >>>
> > >>> [Anoob] I didn't understand how you concluded that.
> > >>
> > >> Because you define a specific array in the test app.
> > >>
> > >>> The app is supposed to test "n" asymmetric features supported by
> DPDK.
> > >> Currently, it does that by looping from 0 to LIST_END which happens
> > >> to give you the first n features. Now, if we add any new asymmetric
> > >> feature, LIST_END value would change. Isn't that the very reason
> > >> why we removed LIST_END from symmetric library and applications?
> > >>
> > >> Yes
> > >>
> > >>> Now coming to what I proposed, the app is supposed to test "n"
> > >>> asymmetric
> > >> features. LIST_END helps in doing the loops. If we remove LIST_END,
> > >> then application will not be in a position to do a loop. My
> > >> suggestion is, we list the types that are supposed to be tested by
> > >> the app, and let that array be used as feature list.
> > >>>
> > >>> PS: Just to reiterate, my proposal is just a local array which
> > >>> would hold DPDK
> > >> defined RTE enum values for the features that would be tested by
> > >> this app/function.
> > >>
> > >> I am more concerned by the general case than the test app.
> > >> I think a function returning a number is more app-friendly.
> > >
> > > [Anoob] Indeed. But there are 3 LIST_ENDs removed with this patch. Do
> you propose 3 new APIs to just get max number?
> >
> > 1 API returning a single "info" structure perhaps - as being the most
> extensible?
> 
> Or 3 iterators (foreach construct).
> Instead of just returning a size, we can have an iterator for each enum which
> needs to be iterated.

[Anoob] Something like this?

diff --git a/app/test/test_cryptodev_asym.c b/app/test/test_cryptodev_asym.c
index 847b074a4f..68a6197851 100644
--- a/app/test/test_cryptodev_asym.c
+++ b/app/test/test_cryptodev_asym.c
@@ -542,7 +542,7 @@ test_one_case(const void *test_case, int sessionless)
                printf("  %u) TestCase %s %s\n", test_index++,
                        tc.modex.description, test_msg);
        } else {
-               for (i = 0; i < RTE_CRYPTO_ASYM_OP_LIST_END; i++) {
+               RTE_CRYPTO_ASYM_FOREACH_OP_TYPE(i) {
                        if (tc.modex.xform_type == RTE_CRYPTO_ASYM_XFORM_RSA) {
                                if (tc.rsa_data.op_type_flags & (1 << i)) {
                                        if (tc.rsa_data.key_exp) {
diff --git a/lib/cryptodev/rte_crypto_asym.h b/lib/cryptodev/rte_crypto_asym.h
index 9c866f553f..5627dcaff1 100644
--- a/lib/cryptodev/rte_crypto_asym.h
+++ b/lib/cryptodev/rte_crypto_asym.h
@@ -119,6 +119,11 @@ enum rte_crypto_asym_op_type {
        RTE_CRYPTO_ASYM_OP_LIST_END
 };

+#define RTE_CRYPTO_ASYM_FOREACH_OP_TYPE(i) \
+       for (i = RTE_CRYPTO_ASYM_OP_ENCRYPT; \
+            i <= RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE; \
+            i++)
+
 /**
  * Padding types for RSA signature.
  */ 

Re: [dpdk-dev] [EXT] Re: [PATCH v2 1/3] cryptodev: remove LIST_END enumerators

[Thomas Monjalon]

13/10/2021 07:36, Anoob Joseph:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 12/10/2021 16:47, Kinsella, Ray:
> > > On 12/10/2021 15:18, Anoob Joseph wrote:
> > > > From: Thomas Monjalon <thomas@monjalon.net>
> > > >> 12/10/2021 15:38, Anoob Joseph:
> > > >>> From: Thomas Monjalon <thomas@monjalon.net>
> > > >>>> 12/10/2021 13:34, Anoob Joseph:
> > > >>>>> From: Kinsella, Ray <mdr@ashroe.eu>
> > > >>>>>> On 12/10/2021 11:50, Anoob Joseph wrote:
> > > >>>>>>> From: Akhil Goyal <gakhil@marvell.com>
> > > >>>>>>>>> On 08/10/2021 21:45, Akhil Goyal wrote:
> > > >>>>>>>>>> Remove *_LIST_END enumerators from asymmetric crypto
> > lib to
> > > >>>>>>>>>> avoid ABI breakage for every new addition in enums.
> > > >>>>>>>>>>
> > > >>>>>>>>>> Signed-off-by: Akhil Goyal <gakhil@marvell.com>
> > > >>>>>>>>>> ---
> > > >>>>>>>>>> -	} else if (xform->xform_type >=
> > > >>>>>>>>> RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END
> > > >>>>>>>>>> +	} else if (xform->xform_type >
> > > >>>> RTE_CRYPTO_ASYM_XFORM_ECPM
> > > >>>> [...]
> > > >>>>>>>>>
> > > >>>>>>>>> So I am not sure that this is an improvement.
> > > >>>>
> > > >>>> Indeed, it is not an improvement.
> > > >>>>
> > > >>>>>>>>> The cryptodev issue we had, was that _LIST_END was being
> > > >>>>>>>>> used to size arrays.
> > > >>>>>>>>> And that broke when new algorithms got added. Is that an
> > > >>>>>>>>> issue, in this
> > > >>>>>> case?
> > > >>>>>>>>
> > > >>>>>>>> Yes we did this same exercise for symmetric crypto enums
> > earlier.
> > > >>>>>>>> Asym enums were left as it was experimental at that point.
> > > >>>>>>>> They are still experimental, but thought of making this
> > > >>>>>>>> uniform throughout DPDK enums.
> > > >>>>>>>>
> > > >>>>>>>>>
> > > >>>>>>>>> I am not sure that swapping out _LIST_END, and then
> > > >>>>>>>>> littering the code with RTE_CRYPTO_ASYM_XFORM_ECPM and
> > > >>>>>>>>> RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE, is an
> > > >>>> improvement
> > > >>>>>>>> here.
> > > >>>>>>>>>
> > > >>>>>>>>> My 2c is that from an ABI PoV
> > RTE_CRYPTO_ASYM_OP_LIST_END is
> > > >>>>>>>>> not better or worse, than
> > > >>>>>> RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE?
> > > >>>>>>>>>
> > > >>>>>>>>> Interested to hear other thoughts.
> > > >>>>>>>>
> > > >>>>>>>> I don’t have any better solution for avoiding ABI issues for now.
> > > >>>>>>>> The change is for avoiding ABI breakage. But we can drop this
> > > >>>>>>>> patch For now as asym is still experimental.
> > > >>>>>>>
> > > >>>>>>> [Anoob] Having LIST_END would preclude new additions to
> > > >>>>>>> asymmetric
> > > >>>> algos?
> > > >>>>>> If yes, then I would suggest we address it now.
> > > >>>>>>
> > > >>>>>> Not at all - but it can be problematic, if two versions of DPDK
> > > >>>>>> disagree with the value of LIST_END.
> > > >>>>>>
> > > >>>>>>> Looking at the "problematic changes", we only have 2-3
> > > >>>>>>> application & PMD changes. For unit test application, we could
> > > >>>>>>> may be do something like,
> > > >>>>>>
> > > >>>>>> The essental functionality not that different, I am just not
> > > >>>>>> sure that the verbosity below is helping.
> > > >>>>>> What you are really trying to guard against is people using
> > > >>>>>> LIST_END to size arrays.
> > > >>>>>
> > > >>>>> [Anoob] Our problem is application using LIST_END (which comes
> > > >>>>> from library)
> > > >>>> to determine the number of iterations for the loop. My suggestion
> > > >>>> is to modify the UT such that, we could use RTE_DIM(types) (which
> > > >>>> comes from application) to determine iterations of loop. This
> > > >>>> would solve the
> > > >> problem, right?
> > > >>>>
> > > >>>> The problem is not the application.
> > > >>>> Are you asking the app to define DPDK types?
> > > >>>
> > > >>> [Anoob] I didn't understand how you concluded that.
> > > >>
> > > >> Because you define a specific array in the test app.
> > > >>
> > > >>> The app is supposed to test "n" asymmetric features supported by
> > DPDK.
> > > >> Currently, it does that by looping from 0 to LIST_END which happens
> > > >> to give you the first n features. Now, if we add any new asymmetric
> > > >> feature, LIST_END value would change. Isn't that the very reason
> > > >> why we removed LIST_END from symmetric library and applications?
> > > >>
> > > >> Yes
> > > >>
> > > >>> Now coming to what I proposed, the app is supposed to test "n"
> > > >>> asymmetric
> > > >> features. LIST_END helps in doing the loops. If we remove LIST_END,
> > > >> then application will not be in a position to do a loop. My
> > > >> suggestion is, we list the types that are supposed to be tested by
> > > >> the app, and let that array be used as feature list.
> > > >>>
> > > >>> PS: Just to reiterate, my proposal is just a local array which
> > > >>> would hold DPDK
> > > >> defined RTE enum values for the features that would be tested by
> > > >> this app/function.
> > > >>
> > > >> I am more concerned by the general case than the test app.
> > > >> I think a function returning a number is more app-friendly.
> > > >
> > > > [Anoob] Indeed. But there are 3 LIST_ENDs removed with this patch. Do
> > you propose 3 new APIs to just get max number?
> > >
> > > 1 API returning a single "info" structure perhaps - as being the most
> > extensible?
> > 
> > Or 3 iterators (foreach construct).
> > Instead of just returning a size, we can have an iterator for each enum which
> > needs to be iterated.
> 
> [Anoob] Something like this?
> 
> diff --git a/app/test/test_cryptodev_asym.c b/app/test/test_cryptodev_asym.c
> index 847b074a4f..68a6197851 100644
> --- a/app/test/test_cryptodev_asym.c
> +++ b/app/test/test_cryptodev_asym.c
> @@ -542,7 +542,7 @@ test_one_case(const void *test_case, int sessionless)
>                 printf("  %u) TestCase %s %s\n", test_index++,
>                         tc.modex.description, test_msg);
>         } else {
> -               for (i = 0; i < RTE_CRYPTO_ASYM_OP_LIST_END; i++) {
> +               RTE_CRYPTO_ASYM_FOREACH_OP_TYPE(i) {
>                         if (tc.modex.xform_type == RTE_CRYPTO_ASYM_XFORM_RSA) {
>                                 if (tc.rsa_data.op_type_flags & (1 << i)) {
>                                         if (tc.rsa_data.key_exp) {
> diff --git a/lib/cryptodev/rte_crypto_asym.h b/lib/cryptodev/rte_crypto_asym.h
> index 9c866f553f..5627dcaff1 100644
> --- a/lib/cryptodev/rte_crypto_asym.h
> +++ b/lib/cryptodev/rte_crypto_asym.h
> @@ -119,6 +119,11 @@ enum rte_crypto_asym_op_type {
>         RTE_CRYPTO_ASYM_OP_LIST_END
>  };
> 
> +#define RTE_CRYPTO_ASYM_FOREACH_OP_TYPE(i) \
> +       for (i = RTE_CRYPTO_ASYM_OP_ENCRYPT; \
> +            i <= RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE; \
> +            i++)

You must not use enum values in the .h, otherwise ABI compatibility is not ensured.
Yes you can do a macro, but it must call functions, not using direct values.

Re: [dpdk-dev] [EXT] Re: [PATCH v2 1/3] cryptodev: remove LIST_END enumerators

[Anoob Joseph]

Hi Akhil, Ray, Thomas,

Please see inline.

Thanks,
Anoob

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Wednesday, October 13, 2021 12:32 PM
> To: Akhil Goyal <gakhil@marvell.com>; dev@dpdk.org; Kinsella, Ray
> <mdr@ashroe.eu>; Anoob Joseph <anoobj@marvell.com>
> Cc: david.marchand@redhat.com; hemant.agrawal@nxp.com;
> pablo.de.lara.guarch@intel.com; fiona.trahe@intel.com;
> declan.doherty@intel.com; matan@nvidia.com; g.singh@nxp.com;
> roy.fan.zhang@intel.com; jianjay.zhou@huawei.com; asomalap@amd.com;
> ruifeng.wang@arm.com; konstantin.ananyev@intel.com;
> radu.nicolau@intel.com; ajit.khaparde@broadcom.com; Nagadheeraj
> Rottela <rnagadheeraj@marvell.com>; Ankur Dwivedi
> <adwivedi@marvell.com>; ciara.power@intel.com; Stephen Hemminger
> <stephen@networkplumber.org>; Yigit, Ferruh <ferruh.yigit@intel.com>;
> bruce.richardson@intel.com
> Subject: Re: [EXT] Re: [dpdk-dev] [PATCH v2 1/3] cryptodev: remove
> LIST_END enumerators
> 
> 13/10/2021 07:36, Anoob Joseph:
> > From: Thomas Monjalon <thomas@monjalon.net>
> > > 12/10/2021 16:47, Kinsella, Ray:
> > > > On 12/10/2021 15:18, Anoob Joseph wrote:
> > > > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > >> 12/10/2021 15:38, Anoob Joseph:
> > > > >>> From: Thomas Monjalon <thomas@monjalon.net>
> > > > >>>> 12/10/2021 13:34, Anoob Joseph:
> > > > >>>>> From: Kinsella, Ray <mdr@ashroe.eu>
> > > > >>>>>> On 12/10/2021 11:50, Anoob Joseph wrote:
> > > > >>>>>>> From: Akhil Goyal <gakhil@marvell.com>
> > > > >>>>>>>>> On 08/10/2021 21:45, Akhil Goyal wrote:
> > > > >>>>>>>>>> Remove *_LIST_END enumerators from asymmetric
> crypto
> > > lib to
> > > > >>>>>>>>>> avoid ABI breakage for every new addition in enums.
> > > > >>>>>>>>>>
> > > > >>>>>>>>>> Signed-off-by: Akhil Goyal <gakhil@marvell.com>
> > > > >>>>>>>>>> ---
> > > > >>>>>>>>>> -	} else if (xform->xform_type >=
> > > > >>>>>>>>> RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END
> > > > >>>>>>>>>> +	} else if (xform->xform_type >
> > > > >>>> RTE_CRYPTO_ASYM_XFORM_ECPM
> > > > >>>> [...]
> > > > >>>>>>>>>
> > > > >>>>>>>>> So I am not sure that this is an improvement.
> > > > >>>>
> > > > >>>> Indeed, it is not an improvement.
> > > > >>>>
> > > > >>>>>>>>> The cryptodev issue we had, was that _LIST_END was being
> > > > >>>>>>>>> used to size arrays.
> > > > >>>>>>>>> And that broke when new algorithms got added. Is that an
> > > > >>>>>>>>> issue, in this
> > > > >>>>>> case?
> > > > >>>>>>>>
> > > > >>>>>>>> Yes we did this same exercise for symmetric crypto enums
> > > earlier.
> > > > >>>>>>>> Asym enums were left as it was experimental at that point.
> > > > >>>>>>>> They are still experimental, but thought of making this
> > > > >>>>>>>> uniform throughout DPDK enums.
> > > > >>>>>>>>
> > > > >>>>>>>>>
> > > > >>>>>>>>> I am not sure that swapping out _LIST_END, and then
> > > > >>>>>>>>> littering the code with RTE_CRYPTO_ASYM_XFORM_ECPM
> and
> > > > >>>>>>>>> RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE, is an
> > > > >>>> improvement
> > > > >>>>>>>> here.
> > > > >>>>>>>>>
> > > > >>>>>>>>> My 2c is that from an ABI PoV
> > > RTE_CRYPTO_ASYM_OP_LIST_END is
> > > > >>>>>>>>> not better or worse, than
> > > > >>>>>> RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE?
> > > > >>>>>>>>>
> > > > >>>>>>>>> Interested to hear other thoughts.
> > > > >>>>>>>>
> > > > >>>>>>>> I don’t have any better solution for avoiding ABI issues for
> now.
> > > > >>>>>>>> The change is for avoiding ABI breakage. But we can drop
> > > > >>>>>>>> this patch For now as asym is still experimental.
> > > > >>>>>>>
> > > > >>>>>>> [Anoob] Having LIST_END would preclude new additions to
> > > > >>>>>>> asymmetric
> > > > >>>> algos?
> > > > >>>>>> If yes, then I would suggest we address it now.
> > > > >>>>>>
> > > > >>>>>> Not at all - but it can be problematic, if two versions of
> > > > >>>>>> DPDK disagree with the value of LIST_END.
> > > > >>>>>>
> > > > >>>>>>> Looking at the "problematic changes", we only have 2-3
> > > > >>>>>>> application & PMD changes. For unit test application, we
> > > > >>>>>>> could may be do something like,
> > > > >>>>>>
> > > > >>>>>> The essental functionality not that different, I am just
> > > > >>>>>> not sure that the verbosity below is helping.
> > > > >>>>>> What you are really trying to guard against is people using
> > > > >>>>>> LIST_END to size arrays.
> > > > >>>>>
> > > > >>>>> [Anoob] Our problem is application using LIST_END (which
> > > > >>>>> comes from library)
> > > > >>>> to determine the number of iterations for the loop. My
> > > > >>>> suggestion is to modify the UT such that, we could use
> > > > >>>> RTE_DIM(types) (which comes from application) to determine
> > > > >>>> iterations of loop. This would solve the
> > > > >> problem, right?
> > > > >>>>
> > > > >>>> The problem is not the application.
> > > > >>>> Are you asking the app to define DPDK types?
> > > > >>>
> > > > >>> [Anoob] I didn't understand how you concluded that.
> > > > >>
> > > > >> Because you define a specific array in the test app.
> > > > >>
> > > > >>> The app is supposed to test "n" asymmetric features supported
> > > > >>> by
> > > DPDK.
> > > > >> Currently, it does that by looping from 0 to LIST_END which
> > > > >> happens to give you the first n features. Now, if we add any
> > > > >> new asymmetric feature, LIST_END value would change. Isn't that
> > > > >> the very reason why we removed LIST_END from symmetric library
> and applications?
> > > > >>
> > > > >> Yes
> > > > >>
> > > > >>> Now coming to what I proposed, the app is supposed to test "n"
> > > > >>> asymmetric
> > > > >> features. LIST_END helps in doing the loops. If we remove
> > > > >> LIST_END, then application will not be in a position to do a
> > > > >> loop. My suggestion is, we list the types that are supposed to
> > > > >> be tested by the app, and let that array be used as feature list.
> > > > >>>
> > > > >>> PS: Just to reiterate, my proposal is just a local array which
> > > > >>> would hold DPDK
> > > > >> defined RTE enum values for the features that would be tested
> > > > >> by this app/function.
> > > > >>
> > > > >> I am more concerned by the general case than the test app.
> > > > >> I think a function returning a number is more app-friendly.
> > > > >
> > > > > [Anoob] Indeed. But there are 3 LIST_ENDs removed with this
> > > > > patch. Do
> > > you propose 3 new APIs to just get max number?
> > > >
> > > > 1 API returning a single "info" structure perhaps - as being the
> > > > most
> > > extensible?
> > >
> > > Or 3 iterators (foreach construct).
> > > Instead of just returning a size, we can have an iterator for each
> > > enum which needs to be iterated.
> >
> > [Anoob] Something like this?
> >
> > diff --git a/app/test/test_cryptodev_asym.c
> > b/app/test/test_cryptodev_asym.c index 847b074a4f..68a6197851 100644
> > --- a/app/test/test_cryptodev_asym.c
> > +++ b/app/test/test_cryptodev_asym.c
> > @@ -542,7 +542,7 @@ test_one_case(const void *test_case, int
> sessionless)
> >                 printf("  %u) TestCase %s %s\n", test_index++,
> >                         tc.modex.description, test_msg);
> >         } else {
> > -               for (i = 0; i < RTE_CRYPTO_ASYM_OP_LIST_END; i++) {
> > +               RTE_CRYPTO_ASYM_FOREACH_OP_TYPE(i) {
> >                         if (tc.modex.xform_type == RTE_CRYPTO_ASYM_XFORM_RSA)
> {
> >                                 if (tc.rsa_data.op_type_flags & (1 << i)) {
> >                                         if (tc.rsa_data.key_exp) {
> > diff --git a/lib/cryptodev/rte_crypto_asym.h
> > b/lib/cryptodev/rte_crypto_asym.h index 9c866f553f..5627dcaff1 100644
> > --- a/lib/cryptodev/rte_crypto_asym.h
> > +++ b/lib/cryptodev/rte_crypto_asym.h
> > @@ -119,6 +119,11 @@ enum rte_crypto_asym_op_type {
> >         RTE_CRYPTO_ASYM_OP_LIST_END
> >  };
> >
> > +#define RTE_CRYPTO_ASYM_FOREACH_OP_TYPE(i) \
> > +       for (i = RTE_CRYPTO_ASYM_OP_ENCRYPT; \
> > +            i <= RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE; \
> > +            i++)
> 
> You must not use enum values in the .h, otherwise ABI compatibility is not
> ensured.
> Yes you can do a macro, but it must call functions, not using direct values.
> 

[Anoob] Understood. Will do that.

@Ray, @Akhil, you are also in agreement, right?

Re: [dpdk-dev] [EXT] Re: [PATCH v2 1/3] cryptodev: remove LIST_END enumerators

[Kinsella, Ray]

On 13/10/2021 08:04, Anoob Joseph wrote:
> Hi Akhil, Ray, Thomas,
> 
> Please see inline.
> 
> Thanks,
> Anoob
> 
>> -----Original Message-----
>> From: Thomas Monjalon <thomas@monjalon.net>
>> Sent: Wednesday, October 13, 2021 12:32 PM
>> To: Akhil Goyal <gakhil@marvell.com>; dev@dpdk.org; Kinsella, Ray
>> <mdr@ashroe.eu>; Anoob Joseph <anoobj@marvell.com>
>> Cc: david.marchand@redhat.com; hemant.agrawal@nxp.com;
>> pablo.de.lara.guarch@intel.com; fiona.trahe@intel.com;
>> declan.doherty@intel.com; matan@nvidia.com; g.singh@nxp.com;
>> roy.fan.zhang@intel.com; jianjay.zhou@huawei.com; asomalap@amd.com;
>> ruifeng.wang@arm.com; konstantin.ananyev@intel.com;
>> radu.nicolau@intel.com; ajit.khaparde@broadcom.com; Nagadheeraj
>> Rottela <rnagadheeraj@marvell.com>; Ankur Dwivedi
>> <adwivedi@marvell.com>; ciara.power@intel.com; Stephen Hemminger
>> <stephen@networkplumber.org>; Yigit, Ferruh <ferruh.yigit@intel.com>;
>> bruce.richardson@intel.com
>> Subject: Re: [EXT] Re: [dpdk-dev] [PATCH v2 1/3] cryptodev: remove
>> LIST_END enumerators
>>
>> 13/10/2021 07:36, Anoob Joseph:
>>> From: Thomas Monjalon <thomas@monjalon.net>
>>>> 12/10/2021 16:47, Kinsella, Ray:
>>>>> On 12/10/2021 15:18, Anoob Joseph wrote:
>>>>>> From: Thomas Monjalon <thomas@monjalon.net>
>>>>>>> 12/10/2021 15:38, Anoob Joseph:
>>>>>>>> From: Thomas Monjalon <thomas@monjalon.net>
>>>>>>>>> 12/10/2021 13:34, Anoob Joseph:
>>>>>>>>>> From: Kinsella, Ray <mdr@ashroe.eu>
>>>>>>>>>>> On 12/10/2021 11:50, Anoob Joseph wrote:
>>>>>>>>>>>> From: Akhil Goyal <gakhil@marvell.com>
>>>>>>>>>>>>>> On 08/10/2021 21:45, Akhil Goyal wrote:
>>>>>>>>>>>>>>> Remove *_LIST_END enumerators from asymmetric
>> crypto
>>>> lib to
>>>>>>>>>>>>>>> avoid ABI breakage for every new addition in enums.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Signed-off-by: Akhil Goyal <gakhil@marvell.com>
>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>> -	} else if (xform->xform_type >=
>>>>>>>>>>>>>> RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END
>>>>>>>>>>>>>>> +	} else if (xform->xform_type >
>>>>>>>>> RTE_CRYPTO_ASYM_XFORM_ECPM
>>>>>>>>> [...]
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> So I am not sure that this is an improvement.
>>>>>>>>>
>>>>>>>>> Indeed, it is not an improvement.
>>>>>>>>>
>>>>>>>>>>>>>> The cryptodev issue we had, was that _LIST_END was being
>>>>>>>>>>>>>> used to size arrays.
>>>>>>>>>>>>>> And that broke when new algorithms got added. Is that an
>>>>>>>>>>>>>> issue, in this
>>>>>>>>>>> case?
>>>>>>>>>>>>>
>>>>>>>>>>>>> Yes we did this same exercise for symmetric crypto enums
>>>> earlier.
>>>>>>>>>>>>> Asym enums were left as it was experimental at that point.
>>>>>>>>>>>>> They are still experimental, but thought of making this
>>>>>>>>>>>>> uniform throughout DPDK enums.
>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I am not sure that swapping out _LIST_END, and then
>>>>>>>>>>>>>> littering the code with RTE_CRYPTO_ASYM_XFORM_ECPM
>> and
>>>>>>>>>>>>>> RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE, is an
>>>>>>>>> improvement
>>>>>>>>>>>>> here.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> My 2c is that from an ABI PoV
>>>> RTE_CRYPTO_ASYM_OP_LIST_END is
>>>>>>>>>>>>>> not better or worse, than
>>>>>>>>>>> RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Interested to hear other thoughts.
>>>>>>>>>>>>>
>>>>>>>>>>>>> I don’t have any better solution for avoiding ABI issues for
>> now.
>>>>>>>>>>>>> The change is for avoiding ABI breakage. But we can drop
>>>>>>>>>>>>> this patch For now as asym is still experimental.
>>>>>>>>>>>>
>>>>>>>>>>>> [Anoob] Having LIST_END would preclude new additions to
>>>>>>>>>>>> asymmetric
>>>>>>>>> algos?
>>>>>>>>>>> If yes, then I would suggest we address it now.
>>>>>>>>>>>
>>>>>>>>>>> Not at all - but it can be problematic, if two versions of
>>>>>>>>>>> DPDK disagree with the value of LIST_END.
>>>>>>>>>>>
>>>>>>>>>>>> Looking at the "problematic changes", we only have 2-3
>>>>>>>>>>>> application & PMD changes. For unit test application, we
>>>>>>>>>>>> could may be do something like,
>>>>>>>>>>>
>>>>>>>>>>> The essental functionality not that different, I am just
>>>>>>>>>>> not sure that the verbosity below is helping.
>>>>>>>>>>> What you are really trying to guard against is people using
>>>>>>>>>>> LIST_END to size arrays.
>>>>>>>>>>
>>>>>>>>>> [Anoob] Our problem is application using LIST_END (which
>>>>>>>>>> comes from library)
>>>>>>>>> to determine the number of iterations for the loop. My
>>>>>>>>> suggestion is to modify the UT such that, we could use
>>>>>>>>> RTE_DIM(types) (which comes from application) to determine
>>>>>>>>> iterations of loop. This would solve the
>>>>>>> problem, right?
>>>>>>>>>
>>>>>>>>> The problem is not the application.
>>>>>>>>> Are you asking the app to define DPDK types?
>>>>>>>>
>>>>>>>> [Anoob] I didn't understand how you concluded that.
>>>>>>>
>>>>>>> Because you define a specific array in the test app.
>>>>>>>
>>>>>>>> The app is supposed to test "n" asymmetric features supported
>>>>>>>> by
>>>> DPDK.
>>>>>>> Currently, it does that by looping from 0 to LIST_END which
>>>>>>> happens to give you the first n features. Now, if we add any
>>>>>>> new asymmetric feature, LIST_END value would change. Isn't that
>>>>>>> the very reason why we removed LIST_END from symmetric library
>> and applications?
>>>>>>>
>>>>>>> Yes
>>>>>>>
>>>>>>>> Now coming to what I proposed, the app is supposed to test "n"
>>>>>>>> asymmetric
>>>>>>> features. LIST_END helps in doing the loops. If we remove
>>>>>>> LIST_END, then application will not be in a position to do a
>>>>>>> loop. My suggestion is, we list the types that are supposed to
>>>>>>> be tested by the app, and let that array be used as feature list.
>>>>>>>>
>>>>>>>> PS: Just to reiterate, my proposal is just a local array which
>>>>>>>> would hold DPDK
>>>>>>> defined RTE enum values for the features that would be tested
>>>>>>> by this app/function.
>>>>>>>
>>>>>>> I am more concerned by the general case than the test app.
>>>>>>> I think a function returning a number is more app-friendly.
>>>>>>
>>>>>> [Anoob] Indeed. But there are 3 LIST_ENDs removed with this
>>>>>> patch. Do
>>>> you propose 3 new APIs to just get max number?
>>>>>
>>>>> 1 API returning a single "info" structure perhaps - as being the
>>>>> most
>>>> extensible?
>>>>
>>>> Or 3 iterators (foreach construct).
>>>> Instead of just returning a size, we can have an iterator for each
>>>> enum which needs to be iterated.
>>>
>>> [Anoob] Something like this?
>>>
>>> diff --git a/app/test/test_cryptodev_asym.c
>>> b/app/test/test_cryptodev_asym.c index 847b074a4f..68a6197851 100644
>>> --- a/app/test/test_cryptodev_asym.c
>>> +++ b/app/test/test_cryptodev_asym.c
>>> @@ -542,7 +542,7 @@ test_one_case(const void *test_case, int
>> sessionless)
>>>                 printf("  %u) TestCase %s %s\n", test_index++,
>>>                         tc.modex.description, test_msg);
>>>         } else {
>>> -               for (i = 0; i < RTE_CRYPTO_ASYM_OP_LIST_END; i++) {
>>> +               RTE_CRYPTO_ASYM_FOREACH_OP_TYPE(i) {
>>>                         if (tc.modex.xform_type == RTE_CRYPTO_ASYM_XFORM_RSA)
>> {
>>>                                 if (tc.rsa_data.op_type_flags & (1 << i)) {
>>>                                         if (tc.rsa_data.key_exp) {
>>> diff --git a/lib/cryptodev/rte_crypto_asym.h
>>> b/lib/cryptodev/rte_crypto_asym.h index 9c866f553f..5627dcaff1 100644
>>> --- a/lib/cryptodev/rte_crypto_asym.h
>>> +++ b/lib/cryptodev/rte_crypto_asym.h
>>> @@ -119,6 +119,11 @@ enum rte_crypto_asym_op_type {
>>>         RTE_CRYPTO_ASYM_OP_LIST_END
>>>  };
>>>
>>> +#define RTE_CRYPTO_ASYM_FOREACH_OP_TYPE(i) \
>>> +       for (i = RTE_CRYPTO_ASYM_OP_ENCRYPT; \
>>> +            i <= RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE; \
>>> +            i++)
>>
>> You must not use enum values in the .h, otherwise ABI compatibility is not
>> ensured.
>> Yes you can do a macro, but it must call functions, not using direct values.
>>
> 
> [Anoob] Understood. Will do that.
> 
> @Ray, @Akhil, you are also in agreement, right?
> 
Yes - whether you use the MACRO or not less important.
In order to maintain the ABI ... you need to learn the array size through an API.

[dpdk-dev] [PATCH v3 1/2] security: hide internal API

[Akhil Goyal]

rte_security_dynfield_register() is an internal
API to be used by the driver, hence moving it to internal.

Signed-off-by: Akhil Goyal <gakhil@marvell.com>
Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
Acked-by: Ray Kinsella <mdr@ashroe.eu>
---
 lib/security/rte_security_driver.h | 2 +-
 lib/security/version.map           | 7 ++++++-
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/lib/security/rte_security_driver.h b/lib/security/rte_security_driver.h
index 938373205c..b0253e962e 100644
--- a/lib/security/rte_security_driver.h
+++ b/lib/security/rte_security_driver.h
@@ -89,7 +89,7 @@ typedef int (*security_session_stats_get_t)(void *device,
 		struct rte_security_session *sess,
 		struct rte_security_stats *stats);
 
-__rte_experimental
+__rte_internal
 int rte_security_dynfield_register(void);
 
 /**
diff --git a/lib/security/version.map b/lib/security/version.map
index a1f46bfd27..edf4887e12 100644
--- a/lib/security/version.map
+++ b/lib/security/version.map
@@ -16,7 +16,12 @@ EXPERIMENTAL {
 	__rte_security_get_userdata;
 	__rte_security_set_pkt_metadata;
 	rte_security_dynfield_offset;
-	rte_security_dynfield_register;
 	rte_security_session_stats_get;
 	rte_security_session_update;
 };
+
+INTERNAL {
+	global:
+
+	rte_security_dynfield_register;
+};
-- 
2.25.1

[dpdk-dev] [PATCH v3 2/2] security: add reserved bitfields

[Akhil Goyal]

In struct rte_security_ipsec_sa_options, for every new option
added, there is an ABI breakage, to avoid, a reserved_opts
bitfield is added to for the remaining bits available in the
structure.
Now for every new sa option, these reserved_opts can be reduced
and new option can be added.

Signed-off-by: Akhil Goyal <gakhil@marvell.com>
Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
Acked-by: Ray Kinsella <mdr@ashroe.eu>
---
v3:
- added a comment for requesting user to clear reserved_opts.
- removed LIST_END enumerators patch. It will be handled separately.


 lib/security/rte_security.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/lib/security/rte_security.h b/lib/security/rte_security.h
index 17d0e95412..4c55dcd744 100644
--- a/lib/security/rte_security.h
+++ b/lib/security/rte_security.h
@@ -263,6 +263,15 @@ struct rte_security_ipsec_sa_options {
 	 * PKT_TX_UDP_CKSUM or PKT_TX_L4_MASK in mbuf.
 	 */
 	uint32_t l4_csum_enable : 1;
+
+	/** Reserved bit fields for future extension
+	 *
+	 * User should ensure reserved_opts is cleared as it may change in
+	 * subsequent releases to support new options.
+	 *
+	 * Note: Reduce number of bits in reserved_opts for every new option.
+	 */
+	uint32_t reserved_opts : 18;
 };
 
 /** IPSec security association direction */
-- 
2.25.1

Re: [dpdk-dev] [PATCH v3 2/2] security: add reserved bitfields

[Akhil Goyal]

> In struct rte_security_ipsec_sa_options, for every new option
> added, there is an ABI breakage, to avoid, a reserved_opts
> bitfield is added to for the remaining bits available in the
> structure.
> Now for every new sa option, these reserved_opts can be reduced
> and new option can be added.
> 
> Signed-off-by: Akhil Goyal <gakhil@marvell.com>
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> Acked-by: Ray Kinsella <mdr@ashroe.eu>
> ---
> v3:
> - added a comment for requesting user to clear reserved_opts.
> - removed LIST_END enumerators patch. It will be handled separately.
> 
Series applied to dpdk-next-crypto