[dpdk-dev] [PATCH v1 0/2] declare crypto asym xform immutable

[dpdk-dev] [PATCH v1 0/2] declare crypto asym xform immutable

[Ayuj Verma]

Mark asym xform as immutable till lifetime
of session. It will save session setup time for
PMDs, which doesn't require any manipulation of
xform data, by directly using these buffers.

* Updated xform type in session init/configure
  API as constant.
* Updated doc with proper transform description.
* Updated openssl PMD with above changes.

Ayuj Verma (2):
  lib/crypto: declare crypto asym xform immutable
  crypto/openssl: mark asym xform constant

 doc/guides/prog_guide/cryptodev_lib.rst      | 10 ++++++++++
 drivers/crypto/openssl/rte_openssl_pmd_ops.c |  8 ++++----
 lib/librte_cryptodev/rte_cryptodev.c         |  2 +-
 lib/librte_cryptodev/rte_cryptodev.h         |  2 +-
 lib/librte_cryptodev/rte_cryptodev_pmd.h     |  2 +-
 5 files changed, 17 insertions(+), 7 deletions(-)

-- 
1.8.3.1

[dpdk-dev] [PATCH v1 1/2] lib/crypto: declare crypto asym xform immutable

[Ayuj Verma]

Update asym xform usage in cryptodev documentation.

Change lib spec to mark xform as const read only
pointer which cannot be manipulated once initiallized
on session.

Signed-off-by: Ayuj Verma <ayverma@marvell.com>
---
 doc/guides/prog_guide/cryptodev_lib.rst  | 10 ++++++++++
 lib/librte_cryptodev/rte_cryptodev.c     |  2 +-
 lib/librte_cryptodev/rte_cryptodev.h     |  2 +-
 lib/librte_cryptodev/rte_cryptodev_pmd.h |  2 +-
 4 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/doc/guides/prog_guide/cryptodev_lib.rst b/doc/guides/prog_guide/cryptodev_lib.rst
index 9719944..cf0930a 100644
--- a/doc/guides/prog_guide/cryptodev_lib.rst
+++ b/doc/guides/prog_guide/cryptodev_lib.rst
@@ -894,6 +894,16 @@ asymmetric crypto chaining is Diffie-Hellman private key generation followed by
 public generation. Also, currently API does not support chaining of symmetric and
 asymmetric crypto xforms.
 
+Transform is attached to session during asym session initialization should not be
+modified either by PMD or application during and after session configuration.
+
+It and all the data buffers it points to should
+remain read only till the end of life span of a session. It should be used as it
+is in PMD, PMDs which requires modification of these immutable data should internally
+do memcpy of data and perform required operations. In that case, it's the PMDs
+responsibility to ensure that any private data copied to local PMD storage
+during session configuration is not stored by the PMD for longer than the session lifetime
+
 Each xform defines specific asymmetric crypto algo. Currently supported are:
 * RSA
 * Modular operations (Exponentiation and Inverse)
diff --git a/lib/librte_cryptodev/rte_cryptodev.c b/lib/librte_cryptodev/rte_cryptodev.c
index 43bc335..1cee406 100644
--- a/lib/librte_cryptodev/rte_cryptodev.c
+++ b/lib/librte_cryptodev/rte_cryptodev.c
@@ -1272,7 +1272,7 @@ struct rte_cryptodev *
 int
 rte_cryptodev_asym_session_init(uint8_t dev_id,
 		struct rte_cryptodev_asym_session *sess,
-		struct rte_crypto_asym_xform *xforms,
+		const struct rte_crypto_asym_xform *xforms,
 		struct rte_mempool *mp)
 {
 	struct rte_cryptodev *dev;
diff --git a/lib/librte_cryptodev/rte_cryptodev.h b/lib/librte_cryptodev/rte_cryptodev.h
index e175b83..4de23bb 100644
--- a/lib/librte_cryptodev/rte_cryptodev.h
+++ b/lib/librte_cryptodev/rte_cryptodev.h
@@ -1118,7 +1118,7 @@ struct rte_cryptodev_asym_session *
 int
 rte_cryptodev_asym_session_init(uint8_t dev_id,
 			struct rte_cryptodev_asym_session *sess,
-			struct rte_crypto_asym_xform *xforms,
+			const struct rte_crypto_asym_xform *xforms,
 			struct rte_mempool *mempool);
 
 /**
diff --git a/lib/librte_cryptodev/rte_cryptodev_pmd.h b/lib/librte_cryptodev/rte_cryptodev_pmd.h
index defe05e..1f083ea 100644
--- a/lib/librte_cryptodev/rte_cryptodev_pmd.h
+++ b/lib/librte_cryptodev/rte_cryptodev_pmd.h
@@ -290,7 +290,7 @@ typedef int (*cryptodev_sym_configure_session_t)(struct rte_cryptodev *dev,
  *  - Returns -ENOMEM if the private session could not be allocated.
  */
 typedef int (*cryptodev_asym_configure_session_t)(struct rte_cryptodev *dev,
-		struct rte_crypto_asym_xform *xform,
+		const struct rte_crypto_asym_xform *xform,
 		struct rte_cryptodev_asym_session *session,
 		struct rte_mempool *mp);
 /**
-- 
1.8.3.1

[dpdk-dev] [PATCH v1 2/2] crypto/openssl: mark asym xform constant

[Ayuj Verma]

Marked asym xform as constant.

Signed-off-by: Ayuj Verma <ayverma@marvell.com>
---
 drivers/crypto/openssl/rte_openssl_pmd_ops.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/crypto/openssl/rte_openssl_pmd_ops.c b/drivers/crypto/openssl/rte_openssl_pmd_ops.c
index a307c91..022a096 100644
--- a/drivers/crypto/openssl/rte_openssl_pmd_ops.c
+++ b/drivers/crypto/openssl/rte_openssl_pmd_ops.c
@@ -813,7 +813,7 @@
 
 static int openssl_set_asym_session_parameters(
 		struct openssl_asym_session *asym_session,
-		struct rte_crypto_asym_xform *xform)
+		const struct rte_crypto_asym_xform *xform)
 {
 	int ret = 0;
 
@@ -925,7 +925,7 @@ static int openssl_set_asym_session_parameters(
 	}
 	case RTE_CRYPTO_ASYM_XFORM_MODEX:
 	{
-		struct rte_crypto_modex_xform *xfrm = &(xform->modex);
+		const struct rte_crypto_modex_xform *xfrm = &(xform->modex);
 
 		BN_CTX *ctx = BN_CTX_new();
 		if (ctx == NULL) {
@@ -956,7 +956,7 @@ static int openssl_set_asym_session_parameters(
 	}
 	case RTE_CRYPTO_ASYM_XFORM_MODINV:
 	{
-		struct rte_crypto_modinv_xform *xfrm = &(xform->modinv);
+		const struct rte_crypto_modinv_xform *xfrm = &(xform->modinv);
 
 		BN_CTX *ctx = BN_CTX_new();
 		if (ctx == NULL) {
@@ -1125,7 +1125,7 @@ static int openssl_set_asym_session_parameters(
 /** Configure the session from a crypto xform chain */
 static int
 openssl_pmd_asym_session_configure(struct rte_cryptodev *dev __rte_unused,
-		struct rte_crypto_asym_xform *xform,
+		const struct rte_crypto_asym_xform *xform,
 		struct rte_cryptodev_asym_session *sess,
 		struct rte_mempool *mempool)
 {
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH v1 0/2] declare crypto asym xform immutable

[Ayuj Verma]

+Fiona.

________________________________
From: Ayuj Verma <ayverma@marvell.com>
Sent: 24 July 2019 14:21:55
To: akhil.goyal@nxp.com <akhil.goyal@nxp.com>
Cc: arkadiuszx.kusztal@intel.com <arkadiuszx.kusztal@intel.com>; Shally Verma <shallyv@marvell.com>; Sunila Sahu <ssahu@marvell.com>; Kanaka Durga Kotamarthy <kkotamarthy@marvell.com>; Anoob Joseph <anoobj@marvell.com>; dev@dpdk.org <dev@dpdk.org>; Ayuj Verma <ayverma@marvell.com>
Subject: [PATCH v1 0/2] declare crypto asym xform immutable

Mark asym xform as immutable till lifetime
of session. It will save session setup time for
PMDs, which doesn't require any manipulation of
xform data, by directly using these buffers.

* Updated xform type in session init/configure
  API as constant.
* Updated doc with proper transform description.
* Updated openssl PMD with above changes.

Ayuj Verma (2):
  lib/crypto: declare crypto asym xform immutable
  crypto/openssl: mark asym xform constant

 doc/guides/prog_guide/cryptodev_lib.rst      | 10 ++++++++++
 drivers/crypto/openssl/rte_openssl_pmd_ops.c |  8 ++++----
 lib/librte_cryptodev/rte_cryptodev.c         |  2 +-
 lib/librte_cryptodev/rte_cryptodev.h         |  2 +-
 lib/librte_cryptodev/rte_cryptodev_pmd.h     |  2 +-
 5 files changed, 17 insertions(+), 7 deletions(-)

--
1.8.3.1

Re: [dpdk-dev] [PATCH v1 0/2] declare crypto asym xform immutable

[Anoob Joseph]

Hi Ayuj,

I believe there are couple of issues with this patch.

Are these experimental APIs? I believe they were made stable this release and I'm not sure if it is a right practice to edit an API without deprecation notice after it is made stable. Especially now that RC2 is done. @Akhil, what is your take on this?

I think, the approach here is wrong. If the lifetime of the session is expected to be only few packets, then session-less (which I believe is in the pipeline) would make more sense. If the lifetime of the session is expected to be more than that, then having this feature/limitation would make application more complicated. Also, since one asymmetric session can hold both public & private keys, the implicit assumption would be, the session can be used for multiple kinds of operations. This change is in contradiction with that.

But my major concern is how this can lead to accidental errors. Making the argument as const will mean the API won't edit its contents. But if there is a pointer in that (key happens to be a pointer inside the xform), having const for xform will not help. This is my understanding. Please correct me if I'm wrong. Also, I could have the xform allocated from stack (non const, regular local variable) and then call the session_init. Would compiler throw an issue in that case? I doubt so.

void abc(const int t)
{
        printf("%d\n", t);
}

void main()
{
        int t = 0;
        abc(t);
        t = 2;
        abc(t);
}

To summarize, if this assumption is accepted, then compiler will not be able to ensure it. And to properly use it, application will have to be drafted differently. And when similar effect can be achieved by having session-less, this seems redundant.

So this change is NACK from my side.

Thanks,
Anoob

From: Ayuj Verma
Sent: Wednesday, July 24, 2019 2:23 PM
To: akhil.goyal@nxp.com
Cc: arkadiuszx.kusztal@intel.com; Shally Verma <shallyv@marvell.com>; Sunila Sahu <ssahu@marvell.com>; Kanaka Durga Kotamarthy <kkotamarthy@marvell.com>; Anoob Joseph <anoobj@marvell.com>; dev@dpdk.org; Fiona Trahe <fiona.trahe@intel.com>
Subject: Re: [PATCH v1 0/2] declare crypto asym xform immutable


+Fiona.

________________________________
From: Ayuj Verma <ayverma@marvell.com<mailto:ayverma@marvell.com>>
Sent: 24 July 2019 14:21:55
To: akhil.goyal@nxp.com<mailto:akhil.goyal@nxp.com> <akhil.goyal@nxp.com<mailto:akhil.goyal@nxp.com>>
Cc: arkadiuszx.kusztal@intel.com<mailto:arkadiuszx.kusztal@intel.com> <arkadiuszx.kusztal@intel.com<mailto:arkadiuszx.kusztal@intel.com>>; Shally Verma <shallyv@marvell.com<mailto:shallyv@marvell.com>>; Sunila Sahu <ssahu@marvell.com<mailto:ssahu@marvell.com>>; Kanaka Durga Kotamarthy <kkotamarthy@marvell.com<mailto:kkotamarthy@marvell.com>>; Anoob Joseph <anoobj@marvell.com<mailto:anoobj@marvell.com>>; dev@dpdk.org<mailto:dev@dpdk.org> <dev@dpdk.org<mailto:dev@dpdk.org>>; Ayuj Verma <ayverma@marvell.com<mailto:ayverma@marvell.com>>
Subject: [PATCH v1 0/2] declare crypto asym xform immutable

Mark asym xform as immutable till lifetime
of session. It will save session setup time for
PMDs, which doesn't require any manipulation of
xform data, by directly using these buffers.

* Updated xform type in session init/configure
  API as constant.
* Updated doc with proper transform description.
* Updated openssl PMD with above changes.

Ayuj Verma (2):
  lib/crypto: declare crypto asym xform immutable
  crypto/openssl: mark asym xform constant

 doc/guides/prog_guide/cryptodev_lib.rst      | 10 ++++++++++
 drivers/crypto/openssl/rte_openssl_pmd_ops.c |  8 ++++----
 lib/librte_cryptodev/rte_cryptodev.c         |  2 +-
 lib/librte_cryptodev/rte_cryptodev.h         |  2 +-
 lib/librte_cryptodev/rte_cryptodev_pmd.h     |  2 +-
 5 files changed, 17 insertions(+), 7 deletions(-)

--
1.8.3.1

Re: [dpdk-dev] [PATCH v1 0/2] declare crypto asym xform immutable

[Shally Verma]

Hi Anoob

Seems your reply is not in plaintext which will make it difficult for inline response. So, please take care of that when you reply.
Rest, please see my response inline.

From: Anoob Joseph 
Sent: Thursday, July 25, 2019 9:46 AM
To: Ayuj Verma <ayverma@marvell.com>; akhil.goyal@nxp.com
Cc: arkadiuszx.kusztal@intel.com; Shally Verma <shallyv@marvell.com>; Sunila Sahu <ssahu@marvell.com>; Kanaka Durga Kotamarthy <kkotamarthy@marvell.com>; dev@dpdk.org; Fiona Trahe <fiona.trahe@intel.com>
Subject: RE: [PATCH v1 0/2] declare crypto asym xform immutable

Hi Ayuj,

I believe there are couple of issues with this patch.

Are these experimental APIs? I believe they were made stable this release and I'm not sure if it is a right practice to edit an API without deprecation notice after it is made stable. Especially now that RC2 is done. @Akhil, what is your take on this?
[Shally] These are experimental still, hence no deprecation notice. We checked about it with Fiona, Akhil before.

I think, the approach here is wrong. If the lifetime of the session is expected to be only few packets, then session-less (which I believe is in the pipeline) would make more sense. 
[Shally] See my response further below on this.

If the lifetime of the session is expected to be more than that, then having this feature/limitation would make application more complicated. Also, since one asymmetric session can hold both public & private keys, the implicit assumption would be, the session can be used for multiple kinds of operations. This change is in contradiction with that. 
[Shally] Why the contradiction here? There's no change in session usage from current version. Currently too, once keys are set on asymmetric session, they are used with multiple operations using that sessions, example - once RSA xform is set with keys, then one can perform sign/verify/enc/dec. So, I don't see any change in that notion with this proposal. All we are changing is, PMD which does not need to store keys in specific format (like openssl PMD), can just hold app buffer pointer till session-lifetime (eventually giving same effect as sessionless). It will help such PMDs to optimize their session setup time by avoiding unnecessary memcpy of keys buffers.

But my major concern is how this can lead to accidental errors. Making the argument as const will mean the API won't edit its contents. But if there is a pointer in that (key happens to be a pointer inside the xform), having const for xform will not help. This is my understanding. Please correct me if I'm wrong. 
[Shally] This spec says " xform and its buffers remain constant" . So, intention is to state to apps that buffer passed to xform should be const in nature and that they should not modify it.

Also, I could have the xform allocated from stack (non const, regular local variable) and then call the session_init. Would compiler throw an issue in that case? I doubt so.

void abc(const int t)
{
        printf("%d\n", t);
}

void main()
{
        int t = 0;
        abc(t);
        t = 2;
        abc(t);
}

To summarize, if this assumption is accepted, then compiler will not be able to ensure it. And to properly use it, application will have to be drafted differently. And when similar effect can be achieved by having session-less, this seems redundant.
[Shally]  Compiler may or may not warn on typecast error here. That's why spec and documentation are put in place to ensure that application don't reuse them or destroy them once "xform and its buffers" are set on session. And, same will need to be documented about xform for session-less usage as well.
Even there, we would ensure that application do not re-use or modify xform and its buffers until dequeue happen. So, practically I see, application would have to take of these cases in session-less as well.

Since in session-based case, xform are set on it than ops, so we're moving same definition on session. So for PMDs which support sessions-based implementations ( like ours) , believe it completely make sense to enable sessions to have sessionless effect.  If we don't change spec to enable optimization, then we're making 1-approach slower than other.  PMDs can adopt any approach more suitable to them. But spec could be made flexible to allow them to experiment with both approaches for performance. Else , PMDs will be forced to experiment around sessionless which may be eventually be an unnecessary overhead for them. 

Thanks
Shally

So this change is NACK from my side.

Thanks,
Anoob

From: Ayuj Verma 
Sent: Wednesday, July 24, 2019 2:23 PM
To: mailto:akhil.goyal@nxp.com
Cc: mailto:arkadiuszx.kusztal@intel.com; Shally Verma <mailto:shallyv@marvell.com>; Sunila Sahu <mailto:ssahu@marvell.com>; Kanaka Durga Kotamarthy <mailto:kkotamarthy@marvell.com>; Anoob Joseph <mailto:anoobj@marvell.com>; mailto:dev@dpdk.org; Fiona Trahe <mailto:fiona.trahe@intel.com>
Subject: Re: [PATCH v1 0/2] declare crypto asym xform immutable

+Fiona.
________________________________________
From: Ayuj Verma <mailto:ayverma@marvell.com>
Sent: 24 July 2019 14:21:55
To: mailto:akhil.goyal@nxp.com <mailto:akhil.goyal@nxp.com>
Cc: mailto:arkadiuszx.kusztal@intel.com <mailto:arkadiuszx.kusztal@intel.com>; Shally Verma <mailto:shallyv@marvell.com>; Sunila Sahu <mailto:ssahu@marvell.com>; Kanaka Durga Kotamarthy <mailto:kkotamarthy@marvell.com>; Anoob Joseph <mailto:anoobj@marvell.com>; mailto:dev@dpdk.org <mailto:dev@dpdk.org>; Ayuj Verma <mailto:ayverma@marvell.com>
Subject: [PATCH v1 0/2] declare crypto asym xform immutable 
 
Mark asym xform as immutable till lifetime
of session. It will save session setup time for
PMDs, which doesn't require any manipulation of
xform data, by directly using these buffers.

* Updated xform type in session init/configure
  API as constant.
* Updated doc with proper transform description.
* Updated openssl PMD with above changes.

Ayuj Verma (2):
  lib/crypto: declare crypto asym xform immutable
  crypto/openssl: mark asym xform constant

 doc/guides/prog_guide/cryptodev_lib.rst      | 10 ++++++++++
 drivers/crypto/openssl/rte_openssl_pmd_ops.c |  8 ++++----
 lib/librte_cryptodev/rte_cryptodev.c         |  2 +-
 lib/librte_cryptodev/rte_cryptodev.h         |  2 +-
 lib/librte_cryptodev/rte_cryptodev_pmd.h     |  2 +-
 5 files changed, 17 insertions(+), 7 deletions(-)

-- 
1.8.3.1

Re: [dpdk-dev] [PATCH v1 0/2] declare crypto asym xform immutable

[Anoob Joseph]

Hi Shally,

Oh yeah! Should've noticed. My settings is to use the incoming mail's formatting. Mail Ayuj created the issue.

Thanks,
Anoob

> -----Original Message-----
> From: Shally Verma
> Sent: Thursday, July 25, 2019 10:52 AM
> To: Anoob Joseph <anoobj@marvell.com>; Ayuj Verma
> <ayverma@marvell.com>; akhil.goyal@nxp.com
> Cc: arkadiuszx.kusztal@intel.com; Sunila Sahu <ssahu@marvell.com>; Kanaka
> Durga Kotamarthy <kkotamarthy@marvell.com>; dev@dpdk.org; Fiona
> Trahe <fiona.trahe@intel.com>
> Subject: RE: [PATCH v1 0/2] declare crypto asym xform immutable
> 
> Hi Anoob
> 
> Seems your reply is not in plaintext which will make it difficult for inline
> response. So, please take care of that when you reply.
> Rest, please see my response inline.
> 
> From: Anoob Joseph
> Sent: Thursday, July 25, 2019 9:46 AM
> To: Ayuj Verma <ayverma@marvell.com>; akhil.goyal@nxp.com
> Cc: arkadiuszx.kusztal@intel.com; Shally Verma <shallyv@marvell.com>;
> Sunila Sahu <ssahu@marvell.com>; Kanaka Durga Kotamarthy
> <kkotamarthy@marvell.com>; dev@dpdk.org; Fiona Trahe
> <fiona.trahe@intel.com>
> Subject: RE: [PATCH v1 0/2] declare crypto asym xform immutable
> 
> Hi Ayuj,
> 
> I believe there are couple of issues with this patch.
> 
> Are these experimental APIs? I believe they were made stable this release
> and I'm not sure if it is a right practice to edit an API without deprecation
> notice after it is made stable. Especially now that RC2 is done. @Akhil, what is
> your take on this?
> [Shally] These are experimental still, hence no deprecation notice. We
> checked about it with Fiona, Akhil before.
> 
> I think, the approach here is wrong. If the lifetime of the session is expected
> to be only few packets, then session-less (which I believe is in the pipeline)
> would make more sense.
> [Shally] See my response further below on this.
> 
> If the lifetime of the session is expected to be more than that, then having
> this feature/limitation would make application more complicated. Also, since
> one asymmetric session can hold both public & private keys, the implicit
> assumption would be, the session can be used for multiple kinds of
> operations. This change is in contradiction with that.
> [Shally] Why the contradiction here? There's no change in session usage from
> current version. Currently too, once keys are set on asymmetric session, they
> are used with multiple operations using that sessions, example - once RSA
> xform is set with keys, then one can perform sign/verify/enc/dec. So, I don't
> see any change in that notion with this proposal. All we are changing is, PMD
> which does not need to store keys in specific format (like openssl PMD), can
> just hold app buffer pointer till session-lifetime (eventually giving same
> effect as sessionless). It will help such PMDs to optimize their session setup
> time by avoiding unnecessary memcpy of keys buffers.
> 
> But my major concern is how this can lead to accidental errors. Making the
> argument as const will mean the API won't edit its contents. But if there is a
> pointer in that (key happens to be a pointer inside the xform), having const
> for xform will not help. This is my understanding. Please correct me if I'm
> wrong.
> [Shally] This spec says " xform and its buffers remain constant" . So, intention
> is to state to apps that buffer passed to xform should be const in nature and
> that they should not modify it.
> 
> Also, I could have the xform allocated from stack (non const, regular local
> variable) and then call the session_init. Would compiler throw an issue in that
> case? I doubt so.
> 
> void abc(const int t)
> {
>         printf("%d\n", t);
> }
> 
> void main()
> {
>         int t = 0;
>         abc(t);
>         t = 2;
>         abc(t);
> }
> 
> To summarize, if this assumption is accepted, then compiler will not be able
> to ensure it. And to properly use it, application will have to be drafted
> differently. And when similar effect can be achieved by having session-less,
> this seems redundant.
> [Shally]  Compiler may or may not warn on typecast error here. That's why
> spec and documentation are put in place to ensure that application don't
> reuse them or destroy them once "xform and its buffers" are set on session.
> And, same will need to be documented about xform for session-less usage
> as well.
> Even there, we would ensure that application do not re-use or modify xform
> and its buffers until dequeue happen. So, practically I see, application would
> have to take of these cases in session-less as well.
> 
> Since in session-based case, xform are set on it than ops, so we're moving
> same definition on session. So for PMDs which support sessions-based
> implementations ( like ours) , believe it completely make sense to enable
> sessions to have sessionless effect.  If we don't change spec to enable
> optimization, then we're making 1-approach slower than other.  PMDs can
> adopt any approach more suitable to them. But spec could be made flexible
> to allow them to experiment with both approaches for performance. Else ,
> PMDs will be forced to experiment around sessionless which may be
> eventually be an unnecessary overhead for them.
> 
> Thanks
> Shally
> 
> So this change is NACK from my side.
> 
> Thanks,
> Anoob
> 
> From: Ayuj Verma
> Sent: Wednesday, July 24, 2019 2:23 PM
> To: mailto:akhil.goyal@nxp.com
> Cc: mailto:arkadiuszx.kusztal@intel.com; Shally Verma
> <mailto:shallyv@marvell.com>; Sunila Sahu <mailto:ssahu@marvell.com>;
> Kanaka Durga Kotamarthy <mailto:kkotamarthy@marvell.com>; Anoob
> Joseph <mailto:anoobj@marvell.com>; mailto:dev@dpdk.org; Fiona Trahe
> <mailto:fiona.trahe@intel.com>
> Subject: Re: [PATCH v1 0/2] declare crypto asym xform immutable
> 
> +Fiona.
> ________________________________________
> From: Ayuj Verma <mailto:ayverma@marvell.com>
> Sent: 24 July 2019 14:21:55
> To: mailto:akhil.goyal@nxp.com <mailto:akhil.goyal@nxp.com>
> Cc: mailto:arkadiuszx.kusztal@intel.com
> <mailto:arkadiuszx.kusztal@intel.com>; Shally Verma
> <mailto:shallyv@marvell.com>; Sunila Sahu <mailto:ssahu@marvell.com>;
> Kanaka Durga Kotamarthy <mailto:kkotamarthy@marvell.com>; Anoob
> Joseph <mailto:anoobj@marvell.com>; mailto:dev@dpdk.org
> <mailto:dev@dpdk.org>; Ayuj Verma <mailto:ayverma@marvell.com>
> Subject: [PATCH v1 0/2] declare crypto asym xform immutable
> 
> Mark asym xform as immutable till lifetime of session. It will save session
> setup time for PMDs, which doesn't require any manipulation of xform data,
> by directly using these buffers.
> 
> * Updated xform type in session init/configure
>   API as constant.
> * Updated doc with proper transform description.
> * Updated openssl PMD with above changes.
> 
> Ayuj Verma (2):
>   lib/crypto: declare crypto asym xform immutable
>   crypto/openssl: mark asym xform constant
> 
>  doc/guides/prog_guide/cryptodev_lib.rst      | 10 ++++++++++
>  drivers/crypto/openssl/rte_openssl_pmd_ops.c |  8 ++++----
>  lib/librte_cryptodev/rte_cryptodev.c         |  2 +-
>  lib/librte_cryptodev/rte_cryptodev.h         |  2 +-
>  lib/librte_cryptodev/rte_cryptodev_pmd.h     |  2 +-
>  5 files changed, 17 insertions(+), 7 deletions(-)
> 
> --
> 1.8.3.1

Re: [dpdk-dev] [PATCH v1 0/2] declare crypto asym xform immutable

[Anoob Joseph]

Hi Shally,

Please see inline.

Thanks,
Anoob

> -----Original Message-----
> From: Shally Verma
> Sent: Thursday, July 25, 2019 10:52 AM
> To: Anoob Joseph <anoobj@marvell.com>; Ayuj Verma
> <ayverma@marvell.com>; akhil.goyal@nxp.com
> Cc: arkadiuszx.kusztal@intel.com; Sunila Sahu <ssahu@marvell.com>; Kanaka
> Durga Kotamarthy <kkotamarthy@marvell.com>; dev@dpdk.org; Fiona
> Trahe <fiona.trahe@intel.com>
> Subject: RE: [PATCH v1 0/2] declare crypto asym xform immutable
> 
> Hi Anoob
> 
> Seems your reply is not in plaintext which will make it difficult for inline
> response. So, please take care of that when you reply.
> Rest, please see my response inline.
> 
> From: Anoob Joseph
> Sent: Thursday, July 25, 2019 9:46 AM
> To: Ayuj Verma <ayverma@marvell.com>; akhil.goyal@nxp.com
> Cc: arkadiuszx.kusztal@intel.com; Shally Verma <shallyv@marvell.com>;
> Sunila Sahu <ssahu@marvell.com>; Kanaka Durga Kotamarthy
> <kkotamarthy@marvell.com>; dev@dpdk.org; Fiona Trahe
> <fiona.trahe@intel.com>
> Subject: RE: [PATCH v1 0/2] declare crypto asym xform immutable
> 
> Hi Ayuj,
> 
> I believe there are couple of issues with this patch.
> 
> Are these experimental APIs? I believe they were made stable this release
> and I'm not sure if it is a right practice to edit an API without deprecation
> notice after it is made stable. Especially now that RC2 is done. @Akhil, what is
> your take on this?
> [Shally] These are experimental still, hence no deprecation notice. We
> checked about it with Fiona, Akhil before.

[Anoob] In the patch, the edited APIs doesn't have experimental tag. I leave it to Akhil's judgement on this.
 
> 
> I think, the approach here is wrong. If the lifetime of the session is expected
> to be only few packets, then session-less (which I believe is in the pipeline)
> would make more sense.
> [Shally] See my response further below on this.
> 
> If the lifetime of the session is expected to be more than that, then having
> this feature/limitation would make application more complicated. Also, since
> one asymmetric session can hold both public & private keys, the implicit
> assumption would be, the session can be used for multiple kinds of
> operations. This change is in contradiction with that.
> [Shally] Why the contradiction here? There's no change in session usage from
> current version. Currently too, once keys are set on asymmetric session, they
> are used with multiple operations using that sessions, example - once RSA
> xform is set with keys, then one can perform sign/verify/enc/dec. So, I don't
> see any change in that notion with this proposal. All we are changing is, PMD
> which does not need to store keys in specific format (like openssl PMD), can
> just hold app buffer pointer till session-lifetime (eventually giving same
> effect as sessionless). It will help such PMDs to optimize their session setup
> time by avoiding unnecessary memcpy of keys buffers.

[Anoob] Contradiction is in the sense of what is a session. Here we are saying one session can have SIGN & VERIFY, but the lifetime  of the session is assumed to be "short". 

> 
> But my major concern is how this can lead to accidental errors. Making the
> argument as const will mean the API won't edit its contents. But if there is a
> pointer in that (key happens to be a pointer inside the xform), having const
> for xform will not help. This is my understanding. Please correct me if I'm
> wrong.
> [Shally] This spec says " xform and its buffers remain constant" . So, intention
> is to state to apps that buffer passed to xform should be const in nature and
> that they should not modify it.

[Anoob] The current change could break existing applications. And the compiler will not be able to detect it.
 
> 
> Also, I could have the xform allocated from stack (non const, regular local
> variable) and then call the session_init. Would compiler throw an issue in that
> case? I doubt so.
> 
> void abc(const int t)
> {
>         printf("%d\n", t);
> }
> 
> void main()
> {
>         int t = 0;
>         abc(t);
>         t = 2;
>         abc(t);
> }
> 
> To summarize, if this assumption is accepted, then compiler will not be able
> to ensure it. And to properly use it, application will have to be drafted
> differently. And when similar effect can be achieved by having session-less,
> this seems redundant.
> [Shally]  Compiler may or may not warn on typecast error here.

[Anoob] May or may not be?

> That's why
> spec and documentation are put in place to ensure that application don't
> reuse them or destroy them once "xform and its buffers" are set on session.
> And, same will need to be documented about xform for session-less usage
> as well.

[Anoob] Can you describe how this is required in session-less?
 
> Even there, we would ensure that application do not re-use or modify xform
> and its buffers until dequeue happen. So, practically I see, application would
> have to take of these cases in session-less as well.
> 
> Since in session-based case, xform are set on it than ops, so we're moving
> same definition on session. So for PMDs which support sessions-based
> implementations ( like ours) , believe it completely make sense to enable
> sessions to have sessionless effect.  If we don't change spec to enable
> optimization, then we're making 1-approach slower than other.  PMDs can
> adopt any approach more suitable to them. But spec could be made flexible
> to allow them to experiment with both approaches for performance. Else ,
> PMDs will be forced to experiment around sessionless which may be
> eventually be an unnecessary overhead for them.

[Anoob] I get your intention on avoiding the memcpys. But the current changes would make the spec more prone to errors. And if we think we should improve the key handling done in both session & session-less, I would suggest not to rush with this change. We can keep the API experimental and continue improving it to fix this issue for all PMDs more cleanly.

> 
> Thanks
> Shally
> 
> So this change is NACK from my side.
> 
> Thanks,
> Anoob
> 
> From: Ayuj Verma
> Sent: Wednesday, July 24, 2019 2:23 PM
> To: mailto:akhil.goyal@nxp.com
> Cc: mailto:arkadiuszx.kusztal@intel.com; Shally Verma
> <mailto:shallyv@marvell.com>; Sunila Sahu <mailto:ssahu@marvell.com>;
> Kanaka Durga Kotamarthy <mailto:kkotamarthy@marvell.com>; Anoob
> Joseph <mailto:anoobj@marvell.com>; mailto:dev@dpdk.org; Fiona Trahe
> <mailto:fiona.trahe@intel.com>
> Subject: Re: [PATCH v1 0/2] declare crypto asym xform immutable
> 
> +Fiona.
> ________________________________________
> From: Ayuj Verma <mailto:ayverma@marvell.com>
> Sent: 24 July 2019 14:21:55
> To: mailto:akhil.goyal@nxp.com <mailto:akhil.goyal@nxp.com>
> Cc: mailto:arkadiuszx.kusztal@intel.com
> <mailto:arkadiuszx.kusztal@intel.com>; Shally Verma
> <mailto:shallyv@marvell.com>; Sunila Sahu <mailto:ssahu@marvell.com>;
> Kanaka Durga Kotamarthy <mailto:kkotamarthy@marvell.com>; Anoob
> Joseph <mailto:anoobj@marvell.com>; mailto:dev@dpdk.org
> <mailto:dev@dpdk.org>; Ayuj Verma <mailto:ayverma@marvell.com>
> Subject: [PATCH v1 0/2] declare crypto asym xform immutable
> 
> Mark asym xform as immutable till lifetime of session. It will save session
> setup time for PMDs, which doesn't require any manipulation of xform data,
> by directly using these buffers.
> 
> * Updated xform type in session init/configure
>   API as constant.
> * Updated doc with proper transform description.
> * Updated openssl PMD with above changes.
> 
> Ayuj Verma (2):
>   lib/crypto: declare crypto asym xform immutable
>   crypto/openssl: mark asym xform constant
> 
>  doc/guides/prog_guide/cryptodev_lib.rst      | 10 ++++++++++
>  drivers/crypto/openssl/rte_openssl_pmd_ops.c |  8 ++++----
>  lib/librte_cryptodev/rte_cryptodev.c         |  2 +-
>  lib/librte_cryptodev/rte_cryptodev.h         |  2 +-
>  lib/librte_cryptodev/rte_cryptodev_pmd.h     |  2 +-
>  5 files changed, 17 insertions(+), 7 deletions(-)
> 
> --
> 1.8.3.1

Re: [dpdk-dev] [PATCH v1 0/2] declare crypto asym xform immutable

[Shally Verma]

> -----Original Message-----
> From: Anoob Joseph
> Sent: Thursday, July 25, 2019 12:07 PM
> To: Shally Verma <shallyv@marvell.com>; Ayuj Verma
> <ayverma@marvell.com>; akhil.goyal@nxp.com
> Cc: arkadiuszx.kusztal@intel.com; Sunila Sahu <ssahu@marvell.com>; Kanaka
> Durga Kotamarthy <kkotamarthy@marvell.com>; dev@dpdk.org; Fiona
> Trahe <fiona.trahe@intel.com>
> Subject: RE: [PATCH v1 0/2] declare crypto asym xform immutable
> 
> Hi Shally,
> 
> Please see inline.
> 
> Thanks,
> Anoob
> 
> > -----Original Message-----
> > From: Shally Verma
> > Sent: Thursday, July 25, 2019 10:52 AM
> > To: Anoob Joseph <anoobj@marvell.com>; Ayuj Verma
> > <ayverma@marvell.com>; akhil.goyal@nxp.com
> > Cc: arkadiuszx.kusztal@intel.com; Sunila Sahu <ssahu@marvell.com>;
> > Kanaka Durga Kotamarthy <kkotamarthy@marvell.com>; dev@dpdk.org;
> Fiona
> > Trahe <fiona.trahe@intel.com>
> > Subject: RE: [PATCH v1 0/2] declare crypto asym xform immutable
> >
> > Hi Anoob
> >
> > Seems your reply is not in plaintext which will make it difficult for
> > inline response. So, please take care of that when you reply.
> > Rest, please see my response inline.
> >
> > From: Anoob Joseph
> > Sent: Thursday, July 25, 2019 9:46 AM
> > To: Ayuj Verma <ayverma@marvell.com>; akhil.goyal@nxp.com
> > Cc: arkadiuszx.kusztal@intel.com; Shally Verma <shallyv@marvell.com>;
> > Sunila Sahu <ssahu@marvell.com>; Kanaka Durga Kotamarthy
> > <kkotamarthy@marvell.com>; dev@dpdk.org; Fiona Trahe
> > <fiona.trahe@intel.com>
> > Subject: RE: [PATCH v1 0/2] declare crypto asym xform immutable
> >
> > Hi Ayuj,
> >
> > I believe there are couple of issues with this patch.
> >
> > Are these experimental APIs? I believe they were made stable this
> > release and I'm not sure if it is a right practice to edit an API
> > without deprecation notice after it is made stable. Especially now
> > that RC2 is done. @Akhil, what is your take on this?
> > [Shally] These are experimental still, hence no deprecation notice. We
> > checked about it with Fiona, Akhil before.
> 
> [Anoob] In the patch, the edited APIs doesn't have experimental tag. I leave
> it to Akhil's judgement on this.
> 
> >
> > I think, the approach here is wrong. If the lifetime of the session is
> > expected to be only few packets, then session-less (which I believe is
> > in the pipeline) would make more sense.
> > [Shally] See my response further below on this.
> >
> > If the lifetime of the session is expected to be more than that, then
> > having this feature/limitation would make application more
> > complicated. Also, since one asymmetric session can hold both public &
> > private keys, the implicit assumption would be, the session can be
> > used for multiple kinds of operations. This change is in contradiction with
> that.
> > [Shally] Why the contradiction here? There's no change in session
> > usage from current version. Currently too, once keys are set on
> > asymmetric session, they are used with multiple operations using that
> > sessions, example - once RSA xform is set with keys, then one can
> > perform sign/verify/enc/dec. So, I don't see any change in that notion
> > with this proposal. All we are changing is, PMD which does not need to
> > store keys in specific format (like openssl PMD), can just hold app
> > buffer pointer till session-lifetime (eventually giving same effect as
> > sessionless). It will help such PMDs to optimize their session setup time by
> avoiding unnecessary memcpy of keys buffers.
> 
> [Anoob] Contradiction is in the sense of what is a session. Here we are saying
> one session can have SIGN & VERIFY, but the lifetime  of the session is
> assumed to be "short".
[Shally] No. we only said, Session will only have keys (which is in xform) with which SIGN & VERIFY will be performed during enqueue.
As long as there're operations to be performed using data set in session, session data should not be manipulated.

> 
> >
> > But my major concern is how this can lead to accidental errors. Making
> > the argument as const will mean the API won't edit its contents. But
> > if there is a pointer in that (key happens to be a pointer inside the
> > xform), having const for xform will not help. This is my
> > understanding. Please correct me if I'm wrong.
> > [Shally] This spec says " xform and its buffers remain constant" . So,
> > intention is to state to apps that buffer passed to xform should be
> > const in nature and that they should not modify it.
> 
> [Anoob] The current change could break existing applications. And the
> compiler will not be able to detect it.
> 
[Shally] Application would need to be changed to make sure this criteria is met. We took care to check same in asym unit test app while proposing change.

> >
> > Also, I could have the xform allocated from stack (non const, regular
> > local
> > variable) and then call the session_init. Would compiler throw an
> > issue in that case? I doubt so.
> >
> > void abc(const int t)
> > {
> >         printf("%d\n", t);
> > }
> >
> > void main()
> > {
> >         int t = 0;
> >         abc(t);
> >         t = 2;
> >         abc(t);
> > }
> >
> > To summarize, if this assumption is accepted, then compiler will not
> > be able to ensure it. And to properly use it, application will have to
> > be drafted differently. And when similar effect can be achieved by
> > having session-less, this seems redundant.
> > [Shally]  Compiler may or may not warn on typecast error here.
> 
> [Anoob] May or may not be?
> 
[Shally] Yes. Depending on compiler version type or optimization level. Havent given try to all.

> > That's why
> > spec and documentation are put in place to ensure that application
> > don't reuse them or destroy them once "xform and its buffers" are set on
> session.
> > And, same will need to be documented about xform for session-less
> > usage as well.
> 
> [Anoob] Can you describe how this is required in session-less?
> 
[Shally] We don't know how every PMD might use them. So, it is safe to mark xform and buffers immutable there too.
So this is my thought but I have asked from POC for sessionless, as it is proposal by Arek, am yet to get more feedback on that.

> > Even there, we would ensure that application do not re-use or modify
> > xform and its buffers until dequeue happen. So, practically I see,
> > application would have to take of these cases in session-less as well.
> >
> > Since in session-based case, xform are set on it than ops, so we're
> > moving same definition on session. So for PMDs which support
> > sessions-based implementations ( like ours) , believe it completely
> > make sense to enable sessions to have sessionless effect.  If we don't
> > change spec to enable optimization, then we're making 1-approach
> > slower than other.  PMDs can adopt any approach more suitable to them.
> > But spec could be made flexible to allow them to experiment with both
> > approaches for performance. Else , PMDs will be forced to experiment
> > around sessionless which may be eventually be an unnecessary overhead
> for them.
> 
> [Anoob] I get your intention on avoiding the memcpys. But the current
> changes would make the spec more prone to errors. And if we think we
> should improve the key handling done in both session & session-less, I would
> suggest not to rush with this change. We can keep the API experimental and
> continue improving it to fix this issue for all PMDs more cleanly.
> 
[Shally] There's no rush. It is up and open for feedback. 
This change has an intent to optimize session setup time and since xform data is not supposed to change
once it is set on session. Only change proposed is, why not just use app buffers instead of redundant copy of same into PMD buffer.
if you see better suggestions , please provide.

> >
> > Thanks
> > Shally
> >
> > So this change is NACK from my side.
> >
> > Thanks,
> > Anoob
> >
> > From: Ayuj Verma
> > Sent: Wednesday, July 24, 2019 2:23 PM
> > To: mailto:akhil.goyal@nxp.com
> > Cc: mailto:arkadiuszx.kusztal@intel.com; Shally Verma
> > <mailto:shallyv@marvell.com>; Sunila Sahu <mailto:ssahu@marvell.com>;
> > Kanaka Durga Kotamarthy <mailto:kkotamarthy@marvell.com>; Anoob
> Joseph
> > <mailto:anoobj@marvell.com>; mailto:dev@dpdk.org; Fiona Trahe
> > <mailto:fiona.trahe@intel.com>
> > Subject: Re: [PATCH v1 0/2] declare crypto asym xform immutable
> >
> > +Fiona.
> > ________________________________________
> > From: Ayuj Verma <mailto:ayverma@marvell.com>
> > Sent: 24 July 2019 14:21:55
> > To: mailto:akhil.goyal@nxp.com <mailto:akhil.goyal@nxp.com>
> > Cc: mailto:arkadiuszx.kusztal@intel.com
> > <mailto:arkadiuszx.kusztal@intel.com>; Shally Verma
> > <mailto:shallyv@marvell.com>; Sunila Sahu <mailto:ssahu@marvell.com>;
> > Kanaka Durga Kotamarthy <mailto:kkotamarthy@marvell.com>; Anoob
> Joseph
> > <mailto:anoobj@marvell.com>; mailto:dev@dpdk.org
> > <mailto:dev@dpdk.org>; Ayuj Verma <mailto:ayverma@marvell.com>
> > Subject: [PATCH v1 0/2] declare crypto asym xform immutable
> >
> > Mark asym xform as immutable till lifetime of session. It will save
> > session setup time for PMDs, which doesn't require any manipulation of
> > xform data, by directly using these buffers.
> >
> > * Updated xform type in session init/configure
> >   API as constant.
> > * Updated doc with proper transform description.
> > * Updated openssl PMD with above changes.
> >
> > Ayuj Verma (2):
> >   lib/crypto: declare crypto asym xform immutable
> >   crypto/openssl: mark asym xform constant
> >
> >  doc/guides/prog_guide/cryptodev_lib.rst      | 10 ++++++++++
> >  drivers/crypto/openssl/rte_openssl_pmd_ops.c |  8 ++++----
> >  lib/librte_cryptodev/rte_cryptodev.c         |  2 +-
> >  lib/librte_cryptodev/rte_cryptodev.h         |  2 +-
> >  lib/librte_cryptodev/rte_cryptodev_pmd.h     |  2 +-
> >  5 files changed, 17 insertions(+), 7 deletions(-)
> >
> > --
> > 1.8.3.1

Re: [dpdk-dev] [PATCH v1 0/2] declare crypto asym xform immutable

[Kusztal, ArkadiuszX]

Hi All,

> > > I believe there are couple of issues with this patch.
> > >
> > > Are these experimental APIs? I believe they were made stable this
> > > release and I'm not sure if it is a right practice to edit an API
> > > without deprecation notice after it is made stable. Especially now
> > > that RC2 is done. @Akhil, what is your take on this?
> > > [Shally] These are experimental still, hence no deprecation notice.
> > > We checked about it with Fiona, Akhil before.
> >
> > [Anoob] In the patch, the edited APIs doesn't have experimental tag. I
> > leave it to Akhil's judgement on this.
> >
> > >
> > > I think, the approach here is wrong. If the lifetime of the session
> > > is expected to be only few packets, then session-less (which I
> > > believe is in the pipeline) would make more sense.
> > > [Shally] See my response further below on this.
> > >
> > > If the lifetime of the session is expected to be more than that,
> > > then having this feature/limitation would make application more
> > > complicated. Also, since one asymmetric session can hold both public
> > > & private keys, the implicit assumption would be, the session can be
> > > used for multiple kinds of operations. This change is in
> > > contradiction with
> > that.
> > > [Shally] Why the contradiction here? There's no change in session
> > > usage from current version. Currently too, once keys are set on
> > > asymmetric session, they are used with multiple operations using
> > > that sessions, example - once RSA xform is set with keys, then one
> > > can perform sign/verify/enc/dec. So, I don't see any change in that
> > > notion with this proposal. All we are changing is, PMD which does
> > > not need to store keys in specific format (like openssl PMD), can
> > > just hold app buffer pointer till session-lifetime (eventually
> > > giving same effect as sessionless). It will help such PMDs to
> > > optimize their session setup time by
> > avoiding unnecessary memcpy of keys buffers.
> >
> > [Anoob] Contradiction is in the sense of what is a session. Here we
> > are saying one session can have SIGN & VERIFY, but the lifetime  of
> > the session is assumed to be "short".
> [Shally] No. we only said, Session will only have keys (which is in xform) with
> which SIGN & VERIFY will be performed during enqueue.
> As long as there're operations to be performed using data set in session,
> session data should not be manipulated.
[AK] - we need to keep in mind that if we will copy user private key inside our internal structs,
It will be stored in two places in the same time, making it more vulnerable, that's why I have asked about session lifetime.
At least we could inform user of this when PMD copy data internally.
Of course usually this data need to be copied anyway for op lifetime (because of some padding/alignement requirements hw may have).

> 
> >
> > >
> > > But my major concern is how this can lead to accidental errors.
> > > Making the argument as const will mean the API won't edit its
> > > contents. But if there is a pointer in that (key happens to be a
> > > pointer inside the xform), having const for xform will not help.
> > > This is my understanding. Please correct me if I'm wrong.
> > > [Shally] This spec says " xform and its buffers remain constant" .
> > > So, intention is to state to apps that buffer passed to xform should
> > > be const in nature and that they should not modify it.
> >
> > [Anoob] The current change could break existing applications. And the
> > compiler will not be able to detect it.
> >
> [Shally] Application would need to be changed to make sure this criteria is
> met. We took care to check same in asym unit test app while proposing
> change.
> 
> > >
> > > Also, I could have the xform allocated from stack (non const,
> > > regular local
> > > variable) and then call the session_init. Would compiler throw an
> > > issue in that case? I doubt so.
> > >
> > > void abc(const int t)
> > > {
> > >         printf("%d\n", t);
> > > }
> > >
> > > void main()
> > > {
> > >         int t = 0;
> > >         abc(t);
> > >         t = 2;
> > >         abc(t);
> > > }
> > >
> > > To summarize, if this assumption is accepted, then compiler will not
> > > be able to ensure it. And to properly use it, application will have
> > > to be drafted differently. And when similar effect can be achieved
> > > by having session-less, this seems redundant.
> > > [Shally]  Compiler may or may not warn on typecast error here.
> >
> > [Anoob] May or may not be?
> >
> [Shally] Yes. Depending on compiler version type or optimization level.
> Havent given try to all.

[AK] Integers conversions are not good example as integers in C are governed by different rules than pointers.
In terms of this
-			struct rte_crypto_asym_xform *xforms,
+			const struct rte_crypto_asym_xform *xforms,
No compiler should complain about that as per spec: for any qualifier q, a pointer to a non-q-qualified type may be converted to a pointer to
the q-qualified version of the type.
We of course expect user will not change this data when there are still ops to be enqueued with this xform.
> 
> > > That's why
> > > spec and documentation are put in place to ensure that application
> > > don't reuse them or destroy them once "xform and its buffers" are
> > > set on
> > session.
> > > And, same will need to be documented about xform for session-less
> > > usage as well.
> >
> > [Anoob] Can you describe how this is required in session-less?
> >
> [Shally] We don't know how every PMD might use them. So, it is safe to mark
> xform and buffers immutable there too.
> So this is my thought but I have asked from POC for sessionless, as it is
> proposal by Arek, am yet to get more feedback on that.
> 
> > > Even there, we would ensure that application do not re-use or modify
> > > xform and its buffers until dequeue happen. So, practically I see,
> > > application would have to take of these cases in session-less as well.
> > >
> > > Since in session-based case, xform are set on it than ops, so we're
> > > moving same definition on session. So for PMDs which support
> > > sessions-based implementations ( like ours) , believe it completely
> > > make sense to enable sessions to have sessionless effect.  If we
> > > don't change spec to enable optimization, then we're making
> > > 1-approach slower than other.  PMDs can adopt any approach more
> suitable to them.
> > > But spec could be made flexible to allow them to experiment with
> > > both approaches for performance. Else , PMDs will be forced to
> > > experiment around sessionless which may be eventually be an
> > > unnecessary overhead
> > for them.
> >
> > [Anoob] I get your intention on avoiding the memcpys. But the current
> > changes would make the spec more prone to errors. And if we think we
> > should improve the key handling done in both session & session-less, I
> > would suggest not to rush with this change. We can keep the API
> > experimental and continue improving it to fix this issue for all PMDs more
> cleanly.
> >
> [Shally] There's no rush. It is up and open for feedback.
> This change has an intent to optimize session setup time and since xform
> data is not supposed to change once it is set on session. Only change
> proposed is, why not just use app buffers instead of redundant copy of same
> into PMD buffer.
> if you see better suggestions , please provide.
> 
> > >
> > > Thanks
> > > Shally
> > >
> > > So this change is NACK from my side.
> > >
> > > Thanks,
> > > Anoob
> > >
> > > From: Ayuj Verma
> > > Sent: Wednesday, July 24, 2019 2:23 PM
> > > To: mailto:akhil.goyal@nxp.com
> > > Cc: mailto:arkadiuszx.kusztal@intel.com; Shally Verma
> > > <mailto:shallyv@marvell.com>; Sunila Sahu
> > > <mailto:ssahu@marvell.com>; Kanaka Durga Kotamarthy
> > > <mailto:kkotamarthy@marvell.com>; Anoob
> > Joseph
> > > <mailto:anoobj@marvell.com>; mailto:dev@dpdk.org; Fiona Trahe
> > > <mailto:fiona.trahe@intel.com>
> > > Subject: Re: [PATCH v1 0/2] declare crypto asym xform immutable
> > >
> > > +Fiona.
> > > ________________________________________
> > > From: Ayuj Verma <mailto:ayverma@marvell.com>
> > > Sent: 24 July 2019 14:21:55
> > > To: mailto:akhil.goyal@nxp.com <mailto:akhil.goyal@nxp.com>
> > > Cc: mailto:arkadiuszx.kusztal@intel.com
> > > <mailto:arkadiuszx.kusztal@intel.com>; Shally Verma
> > > <mailto:shallyv@marvell.com>; Sunila Sahu
> > > <mailto:ssahu@marvell.com>; Kanaka Durga Kotamarthy
> > > <mailto:kkotamarthy@marvell.com>; Anoob
> > Joseph
> > > <mailto:anoobj@marvell.com>; mailto:dev@dpdk.org
> > > <mailto:dev@dpdk.org>; Ayuj Verma <mailto:ayverma@marvell.com>
> > > Subject: [PATCH v1 0/2] declare crypto asym xform immutable
> > >
> > > Mark asym xform as immutable till lifetime of session. It will save
> > > session setup time for PMDs, which doesn't require any manipulation
> > > of xform data, by directly using these buffers.
> > >
> > > * Updated xform type in session init/configure
> > >   API as constant.
> > > * Updated doc with proper transform description.
> > > * Updated openssl PMD with above changes.
> > >
> > > Ayuj Verma (2):
> > >   lib/crypto: declare crypto asym xform immutable
> > >   crypto/openssl: mark asym xform constant
> > >
> > >  doc/guides/prog_guide/cryptodev_lib.rst      | 10 ++++++++++
> > >  drivers/crypto/openssl/rte_openssl_pmd_ops.c |  8 ++++----
> > >  lib/librte_cryptodev/rte_cryptodev.c         |  2 +-
> > >  lib/librte_cryptodev/rte_cryptodev.h         |  2 +-
> > >  lib/librte_cryptodev/rte_cryptodev_pmd.h     |  2 +-
> > >  5 files changed, 17 insertions(+), 7 deletions(-)
> > >
> > > --
> > > 1.8.3.1

Re: [dpdk-dev] [PATCH v1 0/2] declare crypto asym xform immutable

[Ayuj Verma]

________________________________
From: Kusztal, ArkadiuszX <arkadiuszx.kusztal@intel.com>
Sent: 25 July 2019 19:57
To: Shally Verma <shallyv@marvell.com>; Anoob Joseph <anoobj@marvell.com>; Ayuj Verma <ayverma@marvell.com>; akhil.goyal@nxp.com <akhil.goyal@nxp.com>
Cc: Sunila Sahu <ssahu@marvell.com>; Kanaka Durga Kotamarthy <kkotamarthy@marvell.com>; dev@dpdk.org <dev@dpdk.org>; Trahe, Fiona <fiona.trahe@intel.com>
Subject: RE: [PATCH v1 0/2] declare crypto asym xform immutable

Hi All,

> > > I believe there are couple of issues with this patch.
> > >
> > > Are these experimental APIs? I believe they were made stable this
> > > release and I'm not sure if it is a right practice to edit an API
> > > without deprecation notice after it is made stable. Especially now
> > > that RC2 is done. @Akhil, what is your take on this?
> > > [Shally] These are experimental still, hence no deprecation notice.
> > > We checked about it with Fiona, Akhil before.
> >
> > [Anoob] In the patch, the edited APIs doesn't have experimental tag. I
> > leave it to Akhil's judgement on this.
> >
> > >
> > > I think, the approach here is wrong. If the lifetime of the session
> > > is expected to be only few packets, then session-less (which I
> > > believe is in the pipeline) would make more sense.
> > > [Shally] See my response further below on this.
> > >
> > > If the lifetime of the session is expected to be more than that,
> > > then having this feature/limitation would make application more
> > > complicated. Also, since one asymmetric session can hold both public
> > > & private keys, the implicit assumption would be, the session can be
> > > used for multiple kinds of operations. This change is in
> > > contradiction with
> > that.
> > > [Shally] Why the contradiction here? There's no change in session
> > > usage from current version. Currently too, once keys are set on
> > > asymmetric session, they are used with multiple operations using
> > > that sessions, example - once RSA xform is set with keys, then one
> > > can perform sign/verify/enc/dec. So, I don't see any change in that
> > > notion with this proposal. All we are changing is, PMD which does
> > > not need to store keys in specific format (like openssl PMD), can
> > > just hold app buffer pointer till session-lifetime (eventually
> > > giving same effect as sessionless). It will help such PMDs to
> > > optimize their session setup time by
> > avoiding unnecessary memcpy of keys buffers.
> >
> > [Anoob] Contradiction is in the sense of what is a session. Here we
> > are saying one session can have SIGN & VERIFY, but the lifetime  of
> > the session is assumed to be "short".
> [Shally] No. we only said, Session will only have keys (which is in xform) with
> which SIGN & VERIFY will be performed during enqueue.
> As long as there're operations to be performed using data set in session,
> session data should not be manipulated.
[AK] - we need to keep in mind that if we will copy user private key inside our internal structs,
It will be stored in two places in the same time, making it more vulnerable, that's why I have asked about session lifetime.
At least we could inform user of this when PMD copy data internally.
Of course usually this data need to be copied anyway for op lifetime (because of some padding/alignement requirements hw may have).

>
> >
> > >
> > > But my major concern is how this can lead to accidental errors.
> > > Making the argument as const will mean the API won't edit its
> > > contents. But if there is a pointer in that (key happens to be a
> > > pointer inside the xform), having const for xform will not help.
> > > This is my understanding. Please correct me if I'm wrong.
> > > [Shally] This spec says " xform and its buffers remain constant" .
> > > So, intention is to state to apps that buffer passed to xform should
> > > be const in nature and that they should not modify it.
> >
> > [Anoob] The current change could break existing applications. And the
> > compiler will not be able to detect it.
> >
> [Shally] Application would need to be changed to make sure this criteria is
> met. We took care to check same in asym unit test app while proposing
> change.
>
> > >
> > > Also, I could have the xform allocated from stack (non const,
> > > regular local
> > > variable) and then call the session_init. Would compiler throw an
> > > issue in that case? I doubt so.
> > >
> > > void abc(const int t)
> > > {
> > >         printf("%d\n", t);
> > > }
> > >
> > > void main()
> > > {
> > >         int t = 0;
> > >         abc(t);
> > >         t = 2;
> > >         abc(t);
> > > }
> > >
> > > To summarize, if this assumption is accepted, then compiler will not
> > > be able to ensure it. And to properly use it, application will have
> > > to be drafted differently. And when similar effect can be achieved
> > > by having session-less, this seems redundant.
> > > [Shally]  Compiler may or may not warn on typecast error here.
> >
> > [Anoob] May or may not be?
> >
> [Shally] Yes. Depending on compiler version type or optimization level.
> Havent given try to all.

[AK] Integers conversions are not good example as integers in C are governed by different rules than pointers.
In terms of this
-                       struct rte_crypto_asym_xform *xforms,
+                       const struct rte_crypto_asym_xform *xforms,
No compiler should complain about that as per spec: for any qualifier q, a pointer to a non-q-qualified type may be converted to a pointer to
the q-qualified version of the type.
We of course expect user will not change this data when there are still ops to be enqueued with this xform.
[Ayuj] Yes, also can't do below :

                          const struct rte_crypto_asym_xform *xform;

                          struct rte_crypto_modex_xform *xfrm = &(xform->modex);

          it should needs to be const for both which will ensure its sanity :

                          const struct rte_crypto_asym_xform *xform;

                          const struct rte_crypto_modex_xform *xfrm = &(xform->modex);


>
> > > That's why
> > > spec and documentation are put in place to ensure that application
> > > don't reuse them or destroy them once "xform and its buffers" are
> > > set on
> > session.
> > > And, same will need to be documented about xform for session-less
> > > usage as well.
> >
> > [Anoob] Can you describe how this is required in session-less?
> >
> [Shally] We don't know how every PMD might use them. So, it is safe to mark
> xform and buffers immutable there too.
> So this is my thought but I have asked from POC for sessionless, as it is
> proposal by Arek, am yet to get more feedback on that.
>
> > > Even there, we would ensure that application do not re-use or modify
> > > xform and its buffers until dequeue happen. So, practically I see,
> > > application would have to take of these cases in session-less as well.
> > >
> > > Since in session-based case, xform are set on it than ops, so we're
> > > moving same definition on session. So for PMDs which support
> > > sessions-based implementations ( like ours) , believe it completely
> > > make sense to enable sessions to have sessionless effect.  If we
> > > don't change spec to enable optimization, then we're making
> > > 1-approach slower than other.  PMDs can adopt any approach more
> suitable to them.
> > > But spec could be made flexible to allow them to experiment with
> > > both approaches for performance. Else , PMDs will be forced to
> > > experiment around sessionless which may be eventually be an
> > > unnecessary overhead
> > for them.
> >
> > [Anoob] I get your intention on avoiding the memcpys. But the current
> > changes would make the spec more prone to errors. And if we think we
> > should improve the key handling done in both session & session-less, I
> > would suggest not to rush with this change. We can keep the API
> > experimental and continue improving it to fix this issue for all PMDs more
> cleanly.
> >
> [Shally] There's no rush. It is up and open for feedback.
> This change has an intent to optimize session setup time and since xform
> data is not supposed to change once it is set on session. Only change
> proposed is, why not just use app buffers instead of redundant copy of same
> into PMD buffer.
> if you see better suggestions , please provide.
>
> > >
> > > Thanks
> > > Shally
> > >
> > > So this change is NACK from my side.
> > >
> > > Thanks,
> > > Anoob
> > >
> > > From: Ayuj Verma
> > > Sent: Wednesday, July 24, 2019 2:23 PM
> > > To: mailto:akhil.goyal@nxp.com
> > > Cc: mailto:arkadiuszx.kusztal@intel.com; Shally Verma
> > > <mailto:shallyv@marvell.com>; Sunila Sahu
> > > <mailto:ssahu@marvell.com>; Kanaka Durga Kotamarthy
> > > <mailto:kkotamarthy@marvell.com>; Anoob
> > Joseph
> > > <mailto:anoobj@marvell.com>; mailto:dev@dpdk.org; Fiona Trahe
> > > <mailto:fiona.trahe@intel.com>
> > > Subject: Re: [PATCH v1 0/2] declare crypto asym xform immutable
> > >
> > > +Fiona.
> > > ________________________________________
> > > From: Ayuj Verma <mailto:ayverma@marvell.com>
> > > Sent: 24 July 2019 14:21:55
> > > To: mailto:akhil.goyal@nxp.com <mailto:akhil.goyal@nxp.com>
> > > Cc: mailto:arkadiuszx.kusztal@intel.com
> > > <mailto:arkadiuszx.kusztal@intel.com>; Shally Verma
> > > <mailto:shallyv@marvell.com>; Sunila Sahu
> > > <mailto:ssahu@marvell.com>; Kanaka Durga Kotamarthy
> > > <mailto:kkotamarthy@marvell.com>; Anoob
> > Joseph
> > > <mailto:anoobj@marvell.com>; mailto:dev@dpdk.org
> > > <mailto:dev@dpdk.org>; Ayuj Verma <mailto:ayverma@marvell.com>
> > > Subject: [PATCH v1 0/2] declare crypto asym xform immutable
> > >
> > > Mark asym xform as immutable till lifetime of session. It will save
> > > session setup time for PMDs, which doesn't require any manipulation
> > > of xform data, by directly using these buffers.
> > >
> > > * Updated xform type in session init/configure
> > >   API as constant.
> > > * Updated doc with proper transform description.
> > > * Updated openssl PMD with above changes.
> > >
> > > Ayuj Verma (2):
> > >   lib/crypto: declare crypto asym xform immutable
> > >   crypto/openssl: mark asym xform constant
> > >
> > >  doc/guides/prog_guide/cryptodev_lib.rst      | 10 ++++++++++
> > >  drivers/crypto/openssl/rte_openssl_pmd_ops.c |  8 ++++----
> > >  lib/librte_cryptodev/rte_cryptodev.c         |  2 +-
> > >  lib/librte_cryptodev/rte_cryptodev.h         |  2 +-
> > >  lib/librte_cryptodev/rte_cryptodev_pmd.h     |  2 +-
> > >  5 files changed, 17 insertions(+), 7 deletions(-)
> > >
> > > --
> > > 1.8.3.1

Re: [dpdk-dev] [PATCH v1 0/2] declare crypto asym xform immutable

[Kusztal, ArkadiuszX]

Hi all,
What is the state of this patch, is it intended for 19.11 release?
I would also propose to extend .rst comments to session-less case too like in this thread below (with [AK]).
Regards,
Arek
________________________________
From: Kusztal, ArkadiuszX <arkadiuszx.kusztal@intel.com<mailto:arkadiuszx.kusztal@intel.com>>
Sent: 25 July 2019 19:57
To: Shally Verma <shallyv@marvell.com<mailto:shallyv@marvell.com>>; Anoob Joseph <anoobj@marvell.com<mailto:anoobj@marvell.com>>; Ayuj Verma <ayverma@marvell.com<mailto:ayverma@marvell.com>>; akhil.goyal@nxp.com<mailto:akhil.goyal@nxp.com> <akhil.goyal@nxp.com<mailto:akhil.goyal@nxp.com>>
Cc: Sunila Sahu <ssahu@marvell.com<mailto:ssahu@marvell.com>>; Kanaka Durga Kotamarthy <kkotamarthy@marvell.com<mailto:kkotamarthy@marvell.com>>; dev@dpdk.org<mailto:dev@dpdk.org> <dev@dpdk.org<mailto:dev@dpdk.org>>; Trahe, Fiona <fiona.trahe@intel.com<mailto:fiona.trahe@intel.com>>
Subject: RE: [PATCH v1 0/2] declare crypto asym xform immutable

Hi All,

> > > I believe there are couple of issues with this patch.
> > >
> > > Are these experimental APIs? I believe they were made stable this
> > > release and I'm not sure if it is a right practice to edit an API
> > > without deprecation notice after it is made stable. Especially now
> > > that RC2 is done. @Akhil, what is your take on this?
> > > [Shally] These are experimental still, hence no deprecation notice.
> > > We checked about it with Fiona, Akhil before.
> >
> > [Anoob] In the patch, the edited APIs doesn't have experimental tag. I
> > leave it to Akhil's judgement on this.
> >
> > >
> > > I think, the approach here is wrong. If the lifetime of the session
> > > is expected to be only few packets, then session-less (which I
> > > believe is in the pipeline) would make more sense.
> > > [Shally] See my response further below on this.
> > >
> > > If the lifetime of the session is expected to be more than that,
> > > then having this feature/limitation would make application more
> > > complicated. Also, since one asymmetric session can hold both public
> > > & private keys, the implicit assumption would be, the session can be
> > > used for multiple kinds of operations. This change is in
> > > contradiction with
> > that.
> > > [Shally] Why the contradiction here? There's no change in session
> > > usage from current version. Currently too, once keys are set on
> > > asymmetric session, they are used with multiple operations using
> > > that sessions, example - once RSA xform is set with keys, then one
> > > can perform sign/verify/enc/dec. So, I don't see any change in that
> > > notion with this proposal. All we are changing is, PMD which does
> > > not need to store keys in specific format (like openssl PMD), can
> > > just hold app buffer pointer till session-lifetime (eventually
> > > giving same effect as sessionless). It will help such PMDs to
> > > optimize their session setup time by
> > avoiding unnecessary memcpy of keys buffers.
> >
> > [Anoob] Contradiction is in the sense of what is a session. Here we
> > are saying one session can have SIGN & VERIFY, but the lifetime  of
> > the session is assumed to be "short".
> [Shally] No. we only said, Session will only have keys (which is in xform) with
> which SIGN & VERIFY will be performed during enqueue.
> As long as there're operations to be performed using data set in session,
> session data should not be manipulated.
[AK] - we need to keep in mind that if we will copy user private key inside our internal structs,
It will be stored in two places in the same time, making it more vulnerable, that's why I have asked about session lifetime.
At least we could inform user of this when PMD copy data internally.
Of course usually this data need to be copied anyway for op lifetime (because of some padding/alignement requirements hw may have).

>
> >
> > >
> > > But my major concern is how this can lead to accidental errors.
> > > Making the argument as const will mean the API won't edit its
> > > contents. But if there is a pointer in that (key happens to be a
> > > pointer inside the xform), having const for xform will not help.
> > > This is my understanding. Please correct me if I'm wrong.
> > > [Shally] This spec says " xform and its buffers remain constant" .
> > > So, intention is to state to apps that buffer passed to xform should
> > > be const in nature and that they should not modify it.
> >
> > [Anoob] The current change could break existing applications. And the
> > compiler will not be able to detect it.
> >
> [Shally] Application would need to be changed to make sure this criteria is
> met. We took care to check same in asym unit test app while proposing
> change.
>
> > >
> > > Also, I could have the xform allocated from stack (non const,
> > > regular local
> > > variable) and then call the session_init. Would compiler throw an
> > > issue in that case? I doubt so.
> > >
> > > void abc(const int t)
> > > {
> > >         printf("%d\n", t);
> > > }
> > >
> > > void main()
> > > {
> > >         int t = 0;
> > >         abc(t);
> > >         t = 2;
> > >         abc(t);
> > > }
> > >
> > > To summarize, if this assumption is accepted, then compiler will not
> > > be able to ensure it. And to properly use it, application will have
> > > to be drafted differently. And when similar effect can be achieved
> > > by having session-less, this seems redundant.
> > > [Shally]  Compiler may or may not warn on typecast error here.
> >
> > [Anoob] May or may not be?
> >
> [Shally] Yes. Depending on compiler version type or optimization level.
> Havent given try to all.

[AK] Integers conversions are not good example as integers in C are governed by different rules than pointers.
In terms of this
-                       struct rte_crypto_asym_xform *xforms,
+                       const struct rte_crypto_asym_xform *xforms,
No compiler should complain about that as per spec: for any qualifier q, a pointer to a non-q-qualified type may be converted to a pointer to
the q-qualified version of the type.
We of course expect user will not change this data when there are still ops to be enqueued with this xform.
[AK] - This note could be added to the session-less case.
[Ayuj] Yes, also can't do below :

                          const struct rte_crypto_asym_xform *xform;

                          struct rte_crypto_modex_xform *xfrm = &(xform->modex);

          it should needs to be const for both which will ensure its sanity :

                          const struct rte_crypto_asym_xform *xform;

                          const struct rte_crypto_modex_xform *xfrm = &(xform->modex);

>
> > > That's why
> > > spec and documentation are put in place to ensure that application
> > > don't reuse them or destroy them once "xform and its buffers" are
> > > set on
> > session.
> > > And, same will need to be documented about xform for session-less
> > > usage as well.
> >
> > [Anoob] Can you describe how this is required in session-less?
> >
> [Shally] We don't know how every PMD might use them. So, it is safe to mark
> xform and buffers immutable there too.
> So this is my thought but I have asked from POC for sessionless, as it is
> proposal by Arek, am yet to get more feedback on that.
>
> > > Even there, we would ensure that application do not re-use or modify
> > > xform and its buffers until dequeue happen. So, practically I see,
> > > application would have to take of these cases in session-less as well.
> > >
> > > Since in session-based case, xform are set on it than ops, so we're
> > > moving same definition on session. So for PMDs which support
> > > sessions-based implementations ( like ours) , believe it completely
> > > make sense to enable sessions to have sessionless effect.  If we
> > > don't change spec to enable optimization, then we're making
> > > 1-approach slower than other.  PMDs can adopt any approach more
> suitable to them.
> > > But spec could be made flexible to allow them to experiment with
> > > both approaches for performance. Else , PMDs will be forced to
> > > experiment around sessionless which may be eventually be an
> > > unnecessary overhead
> > for them.
> >
> > [Anoob] I get your intention on avoiding the memcpys. But the current
> > changes would make the spec more prone to errors. And if we think we
> > should improve the key handling done in both session & session-less, I
> > would suggest not to rush with this change. We can keep the API
> > experimental and continue improving it to fix this issue for all PMDs more
> cleanly.
> >
> [Shally] There's no rush. It is up and open for feedback.
> This change has an intent to optimize session setup time and since xform
> data is not supposed to change once it is set on session. Only change
> proposed is, why not just use app buffers instead of redundant copy of same
> into PMD buffer.
> if you see better suggestions , please provide.
>
> > >
> > > Thanks
> > > Shally
> > >
> > > So this change is NACK from my side.
> > >
> > > Thanks,
> > > Anoob
> > >
> > > From: Ayuj Verma
> > > Sent: Wednesday, July 24, 2019 2:23 PM
> > > To: mailto:akhil.goyal@nxp.com
> > > Cc: mailto:arkadiuszx.kusztal@intel.com; Shally Verma
> > > <mailto:shallyv@marvell.com>; Sunila Sahu
> > > <mailto:ssahu@marvell.com>; Kanaka Durga Kotamarthy
> > > <mailto:kkotamarthy@marvell.com>; Anoob
> > Joseph
> > > <mailto:anoobj@marvell.com>; mailto:dev@dpdk.org; Fiona Trahe
> > > <mailto:fiona.trahe@intel.com>
> > > Subject: Re: [PATCH v1 0/2] declare crypto asym xform immutable
> > >
> > > +Fiona.
> > > ________________________________________
> > > From: Ayuj Verma <mailto:ayverma@marvell.com>
> > > Sent: 24 July 2019 14:21:55
> > > To: mailto:akhil.goyal@nxp.com <mailto:akhil.goyal@nxp.com>
> > > Cc: mailto:arkadiuszx.kusztal@intel.com
> > > <mailto:arkadiuszx.kusztal@intel.com>; Shally Verma
> > > <mailto:shallyv@marvell.com>; Sunila Sahu
> > > <mailto:ssahu@marvell.com>; Kanaka Durga Kotamarthy
> > > <mailto:kkotamarthy@marvell.com>; Anoob
> > Joseph
> > > <mailto:anoobj@marvell.com>; mailto:dev@dpdk.org
> > > <mailto:dev@dpdk.org>; Ayuj Verma <mailto:ayverma@marvell.com>
> > > Subject: [PATCH v1 0/2] declare crypto asym xform immutable
> > >
> > > Mark asym xform as immutable till lifetime of session. It will save
> > > session setup time for PMDs, which doesn't require any manipulation
> > > of xform data, by directly using these buffers.
> > >
> > > * Updated xform type in session init/configure
> > >   API as constant.
> > > * Updated doc with proper transform description.
> > > * Updated openssl PMD with above changes.
> > >
> > > Ayuj Verma (2):
> > >   lib/crypto: declare crypto asym xform immutable
> > >   crypto/openssl: mark asym xform constant
> > >
> > >  doc/guides/prog_guide/cryptodev_lib.rst      | 10 ++++++++++
> > >  drivers/crypto/openssl/rte_openssl_pmd_ops.c |  8 ++++----
> > >  lib/librte_cryptodev/rte_cryptodev.c         |  2 +-
> > >  lib/librte_cryptodev/rte_cryptodev.h         |  2 +-
> > >  lib/librte_cryptodev/rte_cryptodev_pmd.h     |  2 +-
> > >  5 files changed, 17 insertions(+), 7 deletions(-)
> > >
> > > --
> > > 1.8.3.1

Re: [dpdk-dev] [PATCH v1 0/2] declare crypto asym xform immutable

[Shally Verma]

Hi Arek

Anoob has to have go, no-go on this further. Regarding your question on sessionless, have you submitted POC for same?

Thanks
Shally

From: Kusztal, ArkadiuszX <arkadiuszx.kusztal@intel.com>
Sent: Thursday, September 5, 2019 2:52 PM
To: Ayuj Verma <ayverma@marvell.com>; Shally Verma <shallyv@marvell.com>; Anoob Joseph <anoobj@marvell.com>; akhil.goyal@nxp.com
Cc: Sunila Sahu <ssahu@marvell.com>; Kanaka Durga Kotamarthy <kkotamarthy@marvell.com>; dev@dpdk.org; Trahe, Fiona <fiona.trahe@intel.com>
Subject: RE: [PATCH v1 0/2] declare crypto asym xform immutable

Hi all,
What is the state of this patch, is it intended for 19.11 release?
I would also propose to extend .rst comments to session-less case too like in this thread below (with [AK]).
Regards,
Arek
________________________________
From: Kusztal, ArkadiuszX <arkadiuszx.kusztal@intel.com<mailto:arkadiuszx.kusztal@intel.com>>
Sent: 25 July 2019 19:57
To: Shally Verma <shallyv@marvell.com<mailto:shallyv@marvell.com>>; Anoob Joseph <anoobj@marvell.com<mailto:anoobj@marvell.com>>; Ayuj Verma <ayverma@marvell.com<mailto:ayverma@marvell.com>>; akhil.goyal@nxp.com<mailto:akhil.goyal@nxp.com> <akhil.goyal@nxp.com<mailto:akhil.goyal@nxp.com>>
Cc: Sunila Sahu <ssahu@marvell.com<mailto:ssahu@marvell.com>>; Kanaka Durga Kotamarthy <kkotamarthy@marvell.com<mailto:kkotamarthy@marvell.com>>; dev@dpdk.org<mailto:dev@dpdk.org> <dev@dpdk.org<mailto:dev@dpdk.org>>; Trahe, Fiona <fiona.trahe@intel.com<mailto:fiona.trahe@intel.com>>
Subject: RE: [PATCH v1 0/2] declare crypto asym xform immutable

Hi All,

> > > I believe there are couple of issues with this patch.
> > >
> > > Are these experimental APIs? I believe they were made stable this
> > > release and I'm not sure if it is a right practice to edit an API
> > > without deprecation notice after it is made stable. Especially now
> > > that RC2 is done. @Akhil, what is your take on this?
> > > [Shally] These are experimental still, hence no deprecation notice.
> > > We checked about it with Fiona, Akhil before.
> >
> > [Anoob] In the patch, the edited APIs doesn't have experimental tag. I
> > leave it to Akhil's judgement on this.
> >
> > >
> > > I think, the approach here is wrong. If the lifetime of the session
> > > is expected to be only few packets, then session-less (which I
> > > believe is in the pipeline) would make more sense.
> > > [Shally] See my response further below on this.
> > >
> > > If the lifetime of the session is expected to be more than that,
> > > then having this feature/limitation would make application more
> > > complicated. Also, since one asymmetric session can hold both public
> > > & private keys, the implicit assumption would be, the session can be
> > > used for multiple kinds of operations. This change is in
> > > contradiction with
> > that.
> > > [Shally] Why the contradiction here? There's no change in session
> > > usage from current version. Currently too, once keys are set on
> > > asymmetric session, they are used with multiple operations using
> > > that sessions, example - once RSA xform is set with keys, then one
> > > can perform sign/verify/enc/dec. So, I don't see any change in that
> > > notion with this proposal. All we are changing is, PMD which does
> > > not need to store keys in specific format (like openssl PMD), can
> > > just hold app buffer pointer till session-lifetime (eventually
> > > giving same effect as sessionless). It will help such PMDs to
> > > optimize their session setup time by
> > avoiding unnecessary memcpy of keys buffers.
> >
> > [Anoob] Contradiction is in the sense of what is a session. Here we
> > are saying one session can have SIGN & VERIFY, but the lifetime  of
> > the session is assumed to be "short".
> [Shally] No. we only said, Session will only have keys (which is in xform) with
> which SIGN & VERIFY will be performed during enqueue.
> As long as there're operations to be performed using data set in session,
> session data should not be manipulated.
[AK] - we need to keep in mind that if we will copy user private key inside our internal structs,
It will be stored in two places in the same time, making it more vulnerable, that's why I have asked about session lifetime.
At least we could inform user of this when PMD copy data internally.
Of course usually this data need to be copied anyway for op lifetime (because of some padding/alignement requirements hw may have).

>
> >
> > >
> > > But my major concern is how this can lead to accidental errors.
> > > Making the argument as const will mean the API won't edit its
> > > contents. But if there is a pointer in that (key happens to be a
> > > pointer inside the xform), having const for xform will not help.
> > > This is my understanding. Please correct me if I'm wrong.
> > > [Shally] This spec says " xform and its buffers remain constant" .
> > > So, intention is to state to apps that buffer passed to xform should
> > > be const in nature and that they should not modify it.
> >
> > [Anoob] The current change could break existing applications. And the
> > compiler will not be able to detect it.
> >
> [Shally] Application would need to be changed to make sure this criteria is
> met. We took care to check same in asym unit test app while proposing
> change.
>
> > >
> > > Also, I could have the xform allocated from stack (non const,
> > > regular local
> > > variable) and then call the session_init. Would compiler throw an
> > > issue in that case? I doubt so.
> > >
> > > void abc(const int t)
> > > {
> > >         printf("%d\n", t);
> > > }
> > >
> > > void main()
> > > {
> > >         int t = 0;
> > >         abc(t);
> > >         t = 2;
> > >         abc(t);
> > > }
> > >
> > > To summarize, if this assumption is accepted, then compiler will not
> > > be able to ensure it. And to properly use it, application will have
> > > to be drafted differently. And when similar effect can be achieved
> > > by having session-less, this seems redundant.
> > > [Shally]  Compiler may or may not warn on typecast error here.
> >
> > [Anoob] May or may not be?
> >
> [Shally] Yes. Depending on compiler version type or optimization level.
> Havent given try to all.

[AK] Integers conversions are not good example as integers in C are governed by different rules than pointers.
In terms of this
-                       struct rte_crypto_asym_xform *xforms,
+                       const struct rte_crypto_asym_xform *xforms,
No compiler should complain about that as per spec: for any qualifier q, a pointer to a non-q-qualified type may be converted to a pointer to
the q-qualified version of the type.
We of course expect user will not change this data when there are still ops to be enqueued with this xform.
[AK] - This note could be added to the session-less case.
[Ayuj] Yes, also can't do below :

                          const struct rte_crypto_asym_xform *xform;

                          struct rte_crypto_modex_xform *xfrm = &(xform->modex);

          it should needs to be const for both which will ensure its sanity :

                          const struct rte_crypto_asym_xform *xform;

                          const struct rte_crypto_modex_xform *xfrm = &(xform->modex);

>
> > > That's why
> > > spec and documentation are put in place to ensure that application
> > > don't reuse them or destroy them once "xform and its buffers" are
> > > set on
> > session.
> > > And, same will need to be documented about xform for session-less
> > > usage as well.
> >
> > [Anoob] Can you describe how this is required in session-less?
> >
> [Shally] We don't know how every PMD might use them. So, it is safe to mark
> xform and buffers immutable there too.
> So this is my thought but I have asked from POC for sessionless, as it is
> proposal by Arek, am yet to get more feedback on that.
>
> > > Even there, we would ensure that application do not re-use or modify
> > > xform and its buffers until dequeue happen. So, practically I see,
> > > application would have to take of these cases in session-less as well.
> > >
> > > Since in session-based case, xform are set on it than ops, so we're
> > > moving same definition on session. So for PMDs which support
> > > sessions-based implementations ( like ours) , believe it completely
> > > make sense to enable sessions to have sessionless effect.  If we
> > > don't change spec to enable optimization, then we're making
> > > 1-approach slower than other.  PMDs can adopt any approach more
> suitable to them.
> > > But spec could be made flexible to allow them to experiment with
> > > both approaches for performance. Else , PMDs will be forced to
> > > experiment around sessionless which may be eventually be an
> > > unnecessary overhead
> > for them.
> >
> > [Anoob] I get your intention on avoiding the memcpys. But the current
> > changes would make the spec more prone to errors. And if we think we
> > should improve the key handling done in both session & session-less, I
> > would suggest not to rush with this change. We can keep the API
> > experimental and continue improving it to fix this issue for all PMDs more
> cleanly.
> >
> [Shally] There's no rush. It is up and open for feedback.
> This change has an intent to optimize session setup time and since xform
> data is not supposed to change once it is set on session. Only change
> proposed is, why not just use app buffers instead of redundant copy of same
> into PMD buffer.
> if you see better suggestions , please provide.
>
> > >
> > > Thanks
> > > Shally
> > >
> > > So this change is NACK from my side.
> > >
> > > Thanks,
> > > Anoob
> > >
> > > From: Ayuj Verma
> > > Sent: Wednesday, July 24, 2019 2:23 PM
> > > To: mailto:akhil.goyal@nxp.com
> > > Cc: mailto:arkadiuszx.kusztal@intel.com; Shally Verma
> > > <mailto:shallyv@marvell.com>; Sunila Sahu
> > > <mailto:ssahu@marvell.com>; Kanaka Durga Kotamarthy
> > > <mailto:kkotamarthy@marvell.com>; Anoob
> > Joseph
> > > <mailto:anoobj@marvell.com>; mailto:dev@dpdk.org; Fiona Trahe
> > > <mailto:fiona.trahe@intel.com>
> > > Subject: Re: [PATCH v1 0/2] declare crypto asym xform immutable
> > >
> > > +Fiona.
> > > ________________________________________
> > > From: Ayuj Verma <mailto:ayverma@marvell.com>
> > > Sent: 24 July 2019 14:21:55
> > > To: mailto:akhil.goyal@nxp.com <mailto:akhil.goyal@nxp.com>
> > > Cc: mailto:arkadiuszx.kusztal@intel.com
> > > <mailto:arkadiuszx.kusztal@intel.com>; Shally Verma
> > > <mailto:shallyv@marvell.com>; Sunila Sahu
> > > <mailto:ssahu@marvell.com>; Kanaka Durga Kotamarthy
> > > <mailto:kkotamarthy@marvell.com>; Anoob
> > Joseph
> > > <mailto:anoobj@marvell.com>; mailto:dev@dpdk.org
> > > <mailto:dev@dpdk.org>; Ayuj Verma <mailto:ayverma@marvell.com>
> > > Subject: [PATCH v1 0/2] declare crypto asym xform immutable
> > >
> > > Mark asym xform as immutable till lifetime of session. It will save
> > > session setup time for PMDs, which doesn't require any manipulation
> > > of xform data, by directly using these buffers.
> > >
> > > * Updated xform type in session init/configure
> > >   API as constant.
> > > * Updated doc with proper transform description.
> > > * Updated openssl PMD with above changes.
> > >
> > > Ayuj Verma (2):
> > >   lib/crypto: declare crypto asym xform immutable
> > >   crypto/openssl: mark asym xform constant
> > >
> > >  doc/guides/prog_guide/cryptodev_lib.rst      | 10 ++++++++++
> > >  drivers/crypto/openssl/rte_openssl_pmd_ops.c |  8 ++++----
> > >  lib/librte_cryptodev/rte_cryptodev.c         |  2 +-
> > >  lib/librte_cryptodev/rte_cryptodev.h         |  2 +-
> > >  lib/librte_cryptodev/rte_cryptodev_pmd.h     |  2 +-
> > >  5 files changed, 17 insertions(+), 7 deletions(-)
> > >
> > > --
> > > 1.8.3.1

Re: [dpdk-dev] [PATCH v1 0/2] declare crypto asym xform immutable

[Anoob Joseph]

Hi Shally, Arek,

I still think it's a bit too early to take a decision on this. 'xform' is being used for symmetric operations as well. Everywhere, 'xform' is treated as an input argument to PMD ops. Making this change will define 'xform' in a different way, and that will be applicable only for asymmetric. I believe we can wait till we have a PMD which will be able to fully leverage this. Moreover, the same behavior could be achieved with session-less, if we declare that the number of operations done is "few". So my opinion is to push for session-less now and if it comes that the session based operation with immutable fields is superior to session-less model, we can purse this change then.

Also, the main concern here is that the library will become more prone to errors. Issues arising out of this would be hard to debug. So if we make this change, we should make sure there are no better alternatives.

Following is something, that we often do,

xform.abc = 123
sess1 = create_session(xform);

xform.abc = 456;
sess2 = create_session(xform);

do_operation(sess1, op1);
do_operation(sess2, op2);

With the said change, the above could give bogus results and the code wouldn't look buggy until you read the documentation (and understand the usage).

Thanks,
Anoob

From: Shally Verma <shallyv@marvell.com> 
Sent: Thursday, September 5, 2019 7:52 PM
To: Kusztal, ArkadiuszX <arkadiuszx.kusztal@intel.com>; Ayuj Verma <ayverma@marvell.com>; Anoob Joseph <anoobj@marvell.com>; akhil.goyal@nxp.com
Cc: Sunila Sahu <ssahu@marvell.com>; Kanaka Durga Kotamarthy <kkotamarthy@marvell.com>; dev@dpdk.org; Trahe, Fiona <fiona.trahe@intel.com>
Subject: RE: [PATCH v1 0/2] declare crypto asym xform immutable

Hi Arek

Anoob has to have go, no-go on this further. Regarding your question on sessionless, have you submitted POC for same?

Thanks
Shally

From: Kusztal, ArkadiuszX <mailto:arkadiuszx.kusztal@intel.com> 
Sent: Thursday, September 5, 2019 2:52 PM
To: Ayuj Verma <mailto:ayverma@marvell.com>; Shally Verma <mailto:shallyv@marvell.com>; Anoob Joseph <mailto:anoobj@marvell.com>; mailto:akhil.goyal@nxp.com
Cc: Sunila Sahu <mailto:ssahu@marvell.com>; Kanaka Durga Kotamarthy <mailto:kkotamarthy@marvell.com>; mailto:dev@dpdk.org; Trahe, Fiona <mailto:fiona.trahe@intel.com>
Subject: RE: [PATCH v1 0/2] declare crypto asym xform immutable

Hi all,
What is the state of this patch, is it intended for 19.11 release?
I would also propose to extend .rst comments to session-less case too like in this thread below (with [AK]).
Regards,
Arek
________________________________________
From: Kusztal, ArkadiuszX <mailto:arkadiuszx.kusztal@intel.com>
Sent: 25 July 2019 19:57
To: Shally Verma <mailto:shallyv@marvell.com>; Anoob Joseph <mailto:anoobj@marvell.com>; Ayuj Verma <mailto:ayverma@marvell.com>; mailto:akhil.goyal@nxp.com <mailto:akhil.goyal@nxp.com>
Cc: Sunila Sahu <mailto:ssahu@marvell.com>; Kanaka Durga Kotamarthy <mailto:kkotamarthy@marvell.com>; mailto:dev@dpdk.org <mailto:dev@dpdk.org>; Trahe, Fiona <mailto:fiona.trahe@intel.com>
Subject: RE: [PATCH v1 0/2] declare crypto asym xform immutable 
 
Hi All,

> > > I believe there are couple of issues with this patch.
> > >
> > > Are these experimental APIs? I believe they were made stable this
> > > release and I'm not sure if it is a right practice to edit an API
> > > without deprecation notice after it is made stable. Especially now
> > > that RC2 is done. @Akhil, what is your take on this?
> > > [Shally] These are experimental still, hence no deprecation notice.
> > > We checked about it with Fiona, Akhil before.
> >
> > [Anoob] In the patch, the edited APIs doesn't have experimental tag. I
> > leave it to Akhil's judgement on this.
> >
> > >
> > > I think, the approach here is wrong. If the lifetime of the session
> > > is expected to be only few packets, then session-less (which I
> > > believe is in the pipeline) would make more sense.
> > > [Shally] See my response further below on this.
> > >
> > > If the lifetime of the session is expected to be more than that,
> > > then having this feature/limitation would make application more
> > > complicated. Also, since one asymmetric session can hold both public
> > > & private keys, the implicit assumption would be, the session can be
> > > used for multiple kinds of operations. This change is in
> > > contradiction with
> > that.
> > > [Shally] Why the contradiction here? There's no change in session
> > > usage from current version. Currently too, once keys are set on
> > > asymmetric session, they are used with multiple operations using
> > > that sessions, example - once RSA xform is set with keys, then one
> > > can perform sign/verify/enc/dec. So, I don't see any change in that
> > > notion with this proposal. All we are changing is, PMD which does
> > > not need to store keys in specific format (like openssl PMD), can
> > > just hold app buffer pointer till session-lifetime (eventually
> > > giving same effect as sessionless). It will help such PMDs to
> > > optimize their session setup time by
> > avoiding unnecessary memcpy of keys buffers.
> >
> > [Anoob] Contradiction is in the sense of what is a session. Here we
> > are saying one session can have SIGN & VERIFY, but the lifetime  of
> > the session is assumed to be "short".
> [Shally] No. we only said, Session will only have keys (which is in xform) with
> which SIGN & VERIFY will be performed during enqueue.
> As long as there're operations to be performed using data set in session,
> session data should not be manipulated.
[AK] - we need to keep in mind that if we will copy user private key inside our internal structs,
It will be stored in two places in the same time, making it more vulnerable, that's why I have asked about session lifetime.
At least we could inform user of this when PMD copy data internally.
Of course usually this data need to be copied anyway for op lifetime (because of some padding/alignement requirements hw may have).

> 
> >
> > >
> > > But my major concern is how this can lead to accidental errors.
> > > Making the argument as const will mean the API won't edit its
> > > contents. But if there is a pointer in that (key happens to be a
> > > pointer inside the xform), having const for xform will not help.
> > > This is my understanding. Please correct me if I'm wrong.
> > > [Shally] This spec says " xform and its buffers remain constant" .
> > > So, intention is to state to apps that buffer passed to xform should
> > > be const in nature and that they should not modify it.
> >
> > [Anoob] The current change could break existing applications. And the
> > compiler will not be able to detect it.
> >
> [Shally] Application would need to be changed to make sure this criteria is
> met. We took care to check same in asym unit test app while proposing
> change.
> 
> > >
> > > Also, I could have the xform allocated from stack (non const,
> > > regular local
> > > variable) and then call the session_init. Would compiler throw an
> > > issue in that case? I doubt so.
> > >
> > > void abc(const int t)
> > > {
> > >         printf("%d\n", t);
> > > }
> > >
> > > void main()
> > > {
> > >         int t = 0;
> > >         abc(t);
> > >         t = 2;
> > >         abc(t);
> > > }
> > >
> > > To summarize, if this assumption is accepted, then compiler will not
> > > be able to ensure it. And to properly use it, application will have
> > > to be drafted differently. And when similar effect can be achieved
> > > by having session-less, this seems redundant.
> > > [Shally]  Compiler may or may not warn on typecast error here.
> >
> > [Anoob] May or may not be?
> >
> [Shally] Yes. Depending on compiler version type or optimization level.
> Havent given try to all.

[AK] Integers conversions are not good example as integers in C are governed by different rules than pointers.
In terms of this
-                       struct rte_crypto_asym_xform *xforms,
+                       const struct rte_crypto_asym_xform *xforms,
No compiler should complain about that as per spec: for any qualifier q, a pointer to a non-q-qualified type may be converted to a pointer to
the q-qualified version of the type.
We of course expect user will not change this data when there are still ops to be enqueued with this xform.
[AK] - This note could be added to the session-less case.
[Ayuj] Yes, also can't do below :
                          const struct rte_crypto_asym_xform *xform;
                          struct rte_crypto_modex_xform *xfrm = &(xform->modex);
          it should needs to be const for both which will ensure its sanity :                        
                          const struct rte_crypto_asym_xform *xform;
                          const struct rte_crypto_modex_xform *xfrm = &(xform->modex);

> 
> > > That's why
> > > spec and documentation are put in place to ensure that application
> > > don't reuse them or destroy them once "xform and its buffers" are
> > > set on
> > session.
> > > And, same will need to be documented about xform for session-less
> > > usage as well.
> >
> > [Anoob] Can you describe how this is required in session-less?
> >
> [Shally] We don't know how every PMD might use them. So, it is safe to mark
> xform and buffers immutable there too.
> So this is my thought but I have asked from POC for sessionless, as it is
> proposal by Arek, am yet to get more feedback on that.
> 
> > > Even there, we would ensure that application do not re-use or modify
> > > xform and its buffers until dequeue happen. So, practically I see,
> > > application would have to take of these cases in session-less as well.
> > >
> > > Since in session-based case, xform are set on it than ops, so we're
> > > moving same definition on session. So for PMDs which support
> > > sessions-based implementations ( like ours) , believe it completely
> > > make sense to enable sessions to have sessionless effect.  If we
> > > don't change spec to enable optimization, then we're making
> > > 1-approach slower than other.  PMDs can adopt any approach more
> suitable to them.
> > > But spec could be made flexible to allow them to experiment with
> > > both approaches for performance. Else , PMDs will be forced to
> > > experiment around sessionless which may be eventually be an
> > > unnecessary overhead
> > for them.
> >
> > [Anoob] I get your intention on avoiding the memcpys. But the current
> > changes would make the spec more prone to errors. And if we think we
> > should improve the key handling done in both session & session-less, I
> > would suggest not to rush with this change. We can keep the API
> > experimental and continue improving it to fix this issue for all PMDs more
> cleanly.
> >
> [Shally] There's no rush. It is up and open for feedback.
> This change has an intent to optimize session setup time and since xform
> data is not supposed to change once it is set on session. Only change
> proposed is, why not just use app buffers instead of redundant copy of same
> into PMD buffer.
> if you see better suggestions , please provide.
> 
> > >
> > > Thanks
> > > Shally
> > >
> > > So this change is NACK from my side.
> > >
> > > Thanks,
> > > Anoob
> > >
> > > From: Ayuj Verma
> > > Sent: Wednesday, July 24, 2019 2:23 PM
> > > To: mailto:akhil.goyal@nxp.com
> > > Cc: mailto:arkadiuszx.kusztal@intel.com; Shally Verma
> > > <mailto:shallyv@marvell.com>; Sunila Sahu
> > > <mailto:ssahu@marvell.com>; Kanaka Durga Kotamarthy
> > > <mailto:kkotamarthy@marvell.com>; Anoob
> > Joseph
> > > <mailto:anoobj@marvell.com>; mailto:dev@dpdk.org; Fiona Trahe
> > > <mailto:fiona.trahe@intel.com>
> > > Subject: Re: [PATCH v1 0/2] declare crypto asym xform immutable
> > >
> > > +Fiona.
> > > ________________________________________
> > > From: Ayuj Verma <mailto:ayverma@marvell.com>
> > > Sent: 24 July 2019 14:21:55
> > > To: mailto:akhil.goyal@nxp.com <mailto:akhil.goyal@nxp.com>
> > > Cc: mailto:arkadiuszx.kusztal@intel.com
> > > <mailto:arkadiuszx.kusztal@intel.com>; Shally Verma
> > > <mailto:shallyv@marvell.com>; Sunila Sahu
> > > <mailto:ssahu@marvell.com>; Kanaka Durga Kotamarthy
> > > <mailto:kkotamarthy@marvell.com>; Anoob
> > Joseph
> > > <mailto:anoobj@marvell.com>; mailto:dev@dpdk.org
> > > <mailto:dev@dpdk.org>; Ayuj Verma <mailto:ayverma@marvell.com>
> > > Subject: [PATCH v1 0/2] declare crypto asym xform immutable
> > >
> > > Mark asym xform as immutable till lifetime of session. It will save
> > > session setup time for PMDs, which doesn't require any manipulation
> > > of xform data, by directly using these buffers.
> > >
> > > * Updated xform type in session init/configure
> > >   API as constant.
> > > * Updated doc with proper transform description.
> > > * Updated openssl PMD with above changes.
> > >
> > > Ayuj Verma (2):
> > >   lib/crypto: declare crypto asym xform immutable
> > >   crypto/openssl: mark asym xform constant
> > >
> > >  doc/guides/prog_guide/cryptodev_lib.rst      | 10 ++++++++++
> > >  drivers/crypto/openssl/rte_openssl_pmd_ops.c |  8 ++++----
> > >  lib/librte_cryptodev/rte_cryptodev.c         |  2 +-
> > >  lib/librte_cryptodev/rte_cryptodev.h         |  2 +-
> > >  lib/librte_cryptodev/rte_cryptodev_pmd.h     |  2 +-
> > >  5 files changed, 17 insertions(+), 7 deletions(-)
> > >
> > > --
> > > 1.8.3.1

Re: [dpdk-dev] [PATCH v1 0/2] declare crypto asym xform immutable

[Trahe, Fiona]

Hi Anoob, Arek, Shally,

The fundamental question here seems to be whether it's ok to specify the asymmetric xform to
have different behaviour to the symmetric xform.
In my opinion it is.
At least in terms of our QuickAssist PMD and its use by symmetric and
asymmetric applications I don't see any conflict with the xforms having different characteristics.
They are two different structures.

For rte_crypto_sym_xform, there are 2, usually small, params - key and iv - that need to be copied into
the descriptor to be sent to the hardware, so a copy can't be avoided there and it helps to get this
 copy out of the way during the session. This prep helps to make with-session symmetric more
performant than session-less would be.
And so I don't see any problem in an application re-using the sym_xforms as you've described, and
the API not constraining them to be immutable for the lifetime of the session.
 
For asymmetric on the other hand the rte_crypto_asym_xform has from 2 to 7 larger buffers set up by the application
with input data to be used in the session. In QAT case, none of this data can be copied into the descriptor
and so the PMD would have to alloc duplicate structs and buffers for all this data and all the associated
meta-data (phys_addr ptrs) and copy it all, just to facilitate the application freeing the xform.
From a performance perspective, these copies seem wasteful, especially for an asymmetric session which is likely to have far fewer ops associate with it than a symmetric session would.
From a memory mgmt point of view it increases the memory resources needed.
From a security perspective, minimising the copies of key data in memory is preferable.
And it seems like a reasonable constraint on the Application to not free the xform - there are very few if any fields set up in it that it could re-use, so the only advantage I can see is that it would need a smaller pool of xforms if it could re-use them. 

Though above reasons are from a QAT perspective, they surely also apply to sw PMDs and I'd guess they also apply to other hw devices - though would like feedback if different.
So I'm in favour of the API constraining the asym_xform and all its fields to be immutable for the lifetime of the session.
I get that this doesn't provide programmatic protection if an application ignores the API directive. 
If you can suggest a way to protect this programmatically, that would be better, but even in the absence of this, I think it's worth 
doing and think it's reasonable for application developers to be expected to follow the API documentation.

Fiona

> -----Original Message-----
> From: Anoob Joseph [mailto:anoobj@marvell.com]
> Sent: Friday, September 6, 2019 7:08 AM
> To: Shally Verma <shallyv@marvell.com>; Kusztal, ArkadiuszX <arkadiuszx.kusztal@intel.com>; Ayuj
> Verma <ayverma@marvell.com>; akhil.goyal@nxp.com
> Cc: Sunila Sahu <ssahu@marvell.com>; Kanaka Durga Kotamarthy <kkotamarthy@marvell.com>;
> dev@dpdk.org; Trahe, Fiona <fiona.trahe@intel.com>
> Subject: RE: [PATCH v1 0/2] declare crypto asym xform immutable
> 
> Hi Shally, Arek,
> 
> I still think it's a bit too early to take a decision on this. 'xform' is being used for symmetric operations
> as well. Everywhere, 'xform' is treated as an input argument to PMD ops. Making this change will
> define 'xform' in a different way, and that will be applicable only for asymmetric. I believe we can wait
> till we have a PMD which will be able to fully leverage this. Moreover, the same behavior could be
> achieved with session-less, if we declare that the number of operations done is "few". So my opinion
> is to push for session-less now and if it comes that the session based operation with immutable fields
> is superior to session-less model, we can purse this change then.
> 
> Also, the main concern here is that the library will become more prone to errors. Issues arising out of
> this would be hard to debug. So if we make this change, we should make sure there are no better
> alternatives.
> 
> Following is something, that we often do,
> 
> xform.abc = 123
> sess1 = create_session(xform);
> 
> xform.abc = 456;
> sess2 = create_session(xform);
> 
> do_operation(sess1, op1);
> do_operation(sess2, op2);
> 
> With the said change, the above could give bogus results and the code wouldn't look buggy until you
> read the documentation (and understand the usage).
> 
> Thanks,
> Anoob
> 
> From: Shally Verma <shallyv@marvell.com>
> Sent: Thursday, September 5, 2019 7:52 PM
> To: Kusztal, ArkadiuszX <arkadiuszx.kusztal@intel.com>; Ayuj Verma <ayverma@marvell.com>;
> Anoob Joseph <anoobj@marvell.com>; akhil.goyal@nxp.com
> Cc: Sunila Sahu <ssahu@marvell.com>; Kanaka Durga Kotamarthy <kkotamarthy@marvell.com>;
> dev@dpdk.org; Trahe, Fiona <fiona.trahe@intel.com>
> Subject: RE: [PATCH v1 0/2] declare crypto asym xform immutable
> 
> Hi Arek
> 
> Anoob has to have go, no-go on this further. Regarding your question on sessionless, have you
> submitted POC for same?
> 
> Thanks
> Shally
> 
> From: Kusztal, ArkadiuszX <mailto:arkadiuszx.kusztal@intel.com>
> Sent: Thursday, September 5, 2019 2:52 PM
> To: Ayuj Verma <mailto:ayverma@marvell.com>; Shally Verma <mailto:shallyv@marvell.com>; Anoob
> Joseph <mailto:anoobj@marvell.com>; mailto:akhil.goyal@nxp.com
> Cc: Sunila Sahu <mailto:ssahu@marvell.com>; Kanaka Durga Kotamarthy
> <mailto:kkotamarthy@marvell.com>; mailto:dev@dpdk.org; Trahe, Fiona
> <mailto:fiona.trahe@intel.com>
> Subject: RE: [PATCH v1 0/2] declare crypto asym xform immutable
> 
> Hi all,
> What is the state of this patch, is it intended for 19.11 release?
> I would also propose to extend .rst comments to session-less case too like in this thread below (with
> [AK]).
> Regards,
> Arek
> ________________________________________
> From: Kusztal, ArkadiuszX <mailto:arkadiuszx.kusztal@intel.com>
> Sent: 25 July 2019 19:57
> To: Shally Verma <mailto:shallyv@marvell.com>; Anoob Joseph <mailto:anoobj@marvell.com>; Ayuj
> Verma <mailto:ayverma@marvell.com>; mailto:akhil.goyal@nxp.com <mailto:akhil.goyal@nxp.com>
> Cc: Sunila Sahu <mailto:ssahu@marvell.com>; Kanaka Durga Kotamarthy
> <mailto:kkotamarthy@marvell.com>; mailto:dev@dpdk.org <mailto:dev@dpdk.org>; Trahe, Fiona
> <mailto:fiona.trahe@intel.com>
> Subject: RE: [PATCH v1 0/2] declare crypto asym xform immutable
> 
> Hi All,
> 
> > > > I believe there are couple of issues with this patch.
> > > >
> > > > Are these experimental APIs? I believe they were made stable this
> > > > release and I'm not sure if it is a right practice to edit an API
> > > > without deprecation notice after it is made stable. Especially now
> > > > that RC2 is done. @Akhil, what is your take on this?
> > > > [Shally] These are experimental still, hence no deprecation notice.
> > > > We checked about it with Fiona, Akhil before.
> > >
> > > [Anoob] In the patch, the edited APIs doesn't have experimental tag. I
> > > leave it to Akhil's judgement on this.
> > >
> > > >
> > > > I think, the approach here is wrong. If the lifetime of the session
> > > > is expected to be only few packets, then session-less (which I
> > > > believe is in the pipeline) would make more sense.
> > > > [Shally] See my response further below on this.
> > > >
> > > > If the lifetime of the session is expected to be more than that,
> > > > then having this feature/limitation would make application more
> > > > complicated. Also, since one asymmetric session can hold both public
> > > > & private keys, the implicit assumption would be, the session can be
> > > > used for multiple kinds of operations. This change is in
> > > > contradiction with
> > > that.
> > > > [Shally] Why the contradiction here? There's no change in session
> > > > usage from current version. Currently too, once keys are set on
> > > > asymmetric session, they are used with multiple operations using
> > > > that sessions, example - once RSA xform is set with keys, then one
> > > > can perform sign/verify/enc/dec. So, I don't see any change in that
> > > > notion with this proposal. All we are changing is, PMD which does
> > > > not need to store keys in specific format (like openssl PMD), can
> > > > just hold app buffer pointer till session-lifetime (eventually
> > > > giving same effect as sessionless). It will help such PMDs to
> > > > optimize their session setup time by
> > > avoiding unnecessary memcpy of keys buffers.
> > >
> > > [Anoob] Contradiction is in the sense of what is a session. Here we
> > > are saying one session can have SIGN & VERIFY, but the lifetime  of
> > > the session is assumed to be "short".
> > [Shally] No. we only said, Session will only have keys (which is in xform) with
> > which SIGN & VERIFY will be performed during enqueue.
> > As long as there're operations to be performed using data set in session,
> > session data should not be manipulated.
> [AK] - we need to keep in mind that if we will copy user private key inside our internal structs,
> It will be stored in two places in the same time, making it more vulnerable, that's why I have asked
> about session lifetime.
> At least we could inform user of this when PMD copy data internally.
> Of course usually this data need to be copied anyway for op lifetime (because of some
> padding/alignement requirements hw may have).
> 
> >
> > >
> > > >
> > > > But my major concern is how this can lead to accidental errors.
> > > > Making the argument as const will mean the API won't edit its
> > > > contents. But if there is a pointer in that (key happens to be a
> > > > pointer inside the xform), having const for xform will not help.
> > > > This is my understanding. Please correct me if I'm wrong.
> > > > [Shally] This spec says " xform and its buffers remain constant" .
> > > > So, intention is to state to apps that buffer passed to xform should
> > > > be const in nature and that they should not modify it.
> > >
> > > [Anoob] The current change could break existing applications. And the
> > > compiler will not be able to detect it.
> > >
> > [Shally] Application would need to be changed to make sure this criteria is
> > met. We took care to check same in asym unit test app while proposing
> > change.
> >
> > > >
> > > > Also, I could have the xform allocated from stack (non const,
> > > > regular local
> > > > variable) and then call the session_init. Would compiler throw an
> > > > issue in that case? I doubt so.
> > > >
> > > > void abc(const int t)
> > > > {
> > > >         printf("%d\n", t);
> > > > }
> > > >
> > > > void main()
> > > > {
> > > >         int t = 0;
> > > >         abc(t);
> > > >         t = 2;
> > > >         abc(t);
> > > > }
> > > >
> > > > To summarize, if this assumption is accepted, then compiler will not
> > > > be able to ensure it. And to properly use it, application will have
> > > > to be drafted differently. And when similar effect can be achieved
> > > > by having session-less, this seems redundant.
> > > > [Shally]  Compiler may or may not warn on typecast error here.
> > >
> > > [Anoob] May or may not be?
> > >
> > [Shally] Yes. Depending on compiler version type or optimization level.
> > Havent given try to all.
> 
> [AK] Integers conversions are not good example as integers in C are governed by different rules than
> pointers.
> In terms of this
> -                       struct rte_crypto_asym_xform *xforms,
> +                       const struct rte_crypto_asym_xform *xforms,
> No compiler should complain about that as per spec: for any qualifier q, a pointer to a non-q-qualified
> type may be converted to a pointer to
> the q-qualified version of the type.
> We of course expect user will not change this data when there are still ops to be enqueued with this
> xform.
> [AK] - This note could be added to the session-less case.
> [Ayuj] Yes, also can't do below :
>                           const struct rte_crypto_asym_xform *xform;
>                           struct rte_crypto_modex_xform *xfrm = &(xform->modex);
>           it should needs to be const for both which will ensure its sanity :
>                           const struct rte_crypto_asym_xform *xform;
>                           const struct rte_crypto_modex_xform *xfrm = &(xform->modex);
> 
> >
> > > > That's why
> > > > spec and documentation are put in place to ensure that application
> > > > don't reuse them or destroy them once "xform and its buffers" are
> > > > set on
> > > session.
> > > > And, same will need to be documented about xform for session-less
> > > > usage as well.
> > >
> > > [Anoob] Can you describe how this is required in session-less?
> > >
> > [Shally] We don't know how every PMD might use them. So, it is safe to mark
> > xform and buffers immutable there too.
> > So this is my thought but I have asked from POC for sessionless, as it is
> > proposal by Arek, am yet to get more feedback on that.
> >
> > > > Even there, we would ensure that application do not re-use or modify
> > > > xform and its buffers until dequeue happen. So, practically I see,
> > > > application would have to take of these cases in session-less as well.
> > > >
> > > > Since in session-based case, xform are set on it than ops, so we're
> > > > moving same definition on session. So for PMDs which support
> > > > sessions-based implementations ( like ours) , believe it completely
> > > > make sense to enable sessions to have sessionless effect.  If we
> > > > don't change spec to enable optimization, then we're making
> > > > 1-approach slower than other.  PMDs can adopt any approach more
> > suitable to them.
> > > > But spec could be made flexible to allow them to experiment with
> > > > both approaches for performance. Else , PMDs will be forced to
> > > > experiment around sessionless which may be eventually be an
> > > > unnecessary overhead
> > > for them.
> > >
> > > [Anoob] I get your intention on avoiding the memcpys. But the current
> > > changes would make the spec more prone to errors. And if we think we
> > > should improve the key handling done in both session & session-less, I
> > > would suggest not to rush with this change. We can keep the API
> > > experimental and continue improving it to fix this issue for all PMDs more
> > cleanly.
> > >
> > [Shally] There's no rush. It is up and open for feedback.
> > This change has an intent to optimize session setup time and since xform
> > data is not supposed to change once it is set on session. Only change
> > proposed is, why not just use app buffers instead of redundant copy of same
> > into PMD buffer.
> > if you see better suggestions , please provide.
> >
> > > >
> > > > Thanks
> > > > Shally
> > > >
> > > > So this change is NACK from my side.
> > > >
> > > > Thanks,
> > > > Anoob
> > > >
> > > > From: Ayuj Verma
> > > > Sent: Wednesday, July 24, 2019 2:23 PM
> > > > To: mailto:akhil.goyal@nxp.com
> > > > Cc: mailto:arkadiuszx.kusztal@intel.com; Shally Verma
> > > > <mailto:shallyv@marvell.com>; Sunila Sahu
> > > > <mailto:ssahu@marvell.com>; Kanaka Durga Kotamarthy
> > > > <mailto:kkotamarthy@marvell.com>; Anoob
> > > Joseph
> > > > <mailto:anoobj@marvell.com>; mailto:dev@dpdk.org; Fiona Trahe
> > > > <mailto:fiona.trahe@intel.com>
> > > > Subject: Re: [PATCH v1 0/2] declare crypto asym xform immutable
> > > >
> > > > +Fiona.
> > > > ________________________________________
> > > > From: Ayuj Verma <mailto:ayverma@marvell.com>
> > > > Sent: 24 July 2019 14:21:55
> > > > To: mailto:akhil.goyal@nxp.com <mailto:akhil.goyal@nxp.com>
> > > > Cc: mailto:arkadiuszx.kusztal@intel.com
> > > > <mailto:arkadiuszx.kusztal@intel.com>; Shally Verma
> > > > <mailto:shallyv@marvell.com>; Sunila Sahu
> > > > <mailto:ssahu@marvell.com>; Kanaka Durga Kotamarthy
> > > > <mailto:kkotamarthy@marvell.com>; Anoob
> > > Joseph
> > > > <mailto:anoobj@marvell.com>; mailto:dev@dpdk.org
> > > > <mailto:dev@dpdk.org>; Ayuj Verma <mailto:ayverma@marvell.com>
> > > > Subject: [PATCH v1 0/2] declare crypto asym xform immutable
> > > >
> > > > Mark asym xform as immutable till lifetime of session. It will save
> > > > session setup time for PMDs, which doesn't require any manipulation
> > > > of xform data, by directly using these buffers.
> > > >
> > > > * Updated xform type in session init/configure
> > > >   API as constant.
> > > > * Updated doc with proper transform description.
> > > > * Updated openssl PMD with above changes.
> > > >
> > > > Ayuj Verma (2):
> > > >   lib/crypto: declare crypto asym xform immutable
> > > >   crypto/openssl: mark asym xform constant
> > > >
> > > >  doc/guides/prog_guide/cryptodev_lib.rst      | 10 ++++++++++
> > > >  drivers/crypto/openssl/rte_openssl_pmd_ops.c |  8 ++++----
> > > >  lib/librte_cryptodev/rte_cryptodev.c         |  2 +-
> > > >  lib/librte_cryptodev/rte_cryptodev.h         |  2 +-
> > > >  lib/librte_cryptodev/rte_cryptodev_pmd.h     |  2 +-
> > > >  5 files changed, 17 insertions(+), 7 deletions(-)
> > > >
> > > > --
> > > > 1.8.3.1

Re: [dpdk-dev] [PATCH v1 0/2] declare crypto asym xform immutable

[Sunila Sahu]

Hi Fiona, Anoob,

Fiona,

> Though above reasons are from a QAT perspective, they surely also apply to sw PMDs and
> I'd guess they also apply to other hw devices - though would like feedback if different.

Declaring crypto asym xform immutable does save allocation and copy of key data into OCTEON PMD
during session configuration.
But since hw requires a definitive alignment, xform data has to be copied for op enqueue. So in
this case session-less implementation is beneficial.
I think there is value in both the implementations depending on the PMD.

Anoob,

> Moreover, the same behavior could be
> achieved with session-less, if we declare that the number of operations done is "few".

Not necessarily the number of operations done here is "few". It entirely depends on key usage.
If the key is ephemeral then your operation will be limited to a single transaction as in case of a key
exchange mechanism.
But for other operations as digital signature, the life-time of the session is not limited to a single operation.
Also, there can be different ops (public key or private key authentication) with a single session.
In that case, session implementation will have an advantage over sessionless.

Thanks,
Sunila

________________________________
From: Trahe, Fiona <fiona.trahe@intel.com>
Sent: Friday, September 6, 2019 4:55 PM
To: Anoob Joseph <anoobj@marvell.com>; Shally Verma <shallyv@marvell.com>; Kusztal, ArkadiuszX <arkadiuszx.kusztal@intel.com>; Ayuj Verma <ayverma@marvell.com>; akhil.goyal@nxp.com <akhil.goyal@nxp.com>
Cc: Sunila Sahu <ssahu@marvell.com>; Kanaka Durga Kotamarthy <kkotamarthy@marvell.com>; dev@dpdk.org <dev@dpdk.org>; Trahe, Fiona <fiona.trahe@intel.com>
Subject: [EXT] RE: [PATCH v1 0/2] declare crypto asym xform immutable

Hi Anoob, Arek, Shally,

The fundamental question here seems to be whether it's ok to specify the asymmetric xform to
have different behaviour to the symmetric xform.
In my opinion it is.
At least in terms of our QuickAssist PMD and its use by symmetric and
asymmetric applications I don't see any conflict with the xforms having different characteristics.
They are two different structures.

For rte_crypto_sym_xform, there are 2, usually small, params - key and iv - that need to be copied into
the descriptor to be sent to the hardware, so a copy can't be avoided there and it helps to get this
 copy out of the way during the session. This prep helps to make with-session symmetric more
performant than session-less would be.
And so I don't see any problem in an application re-using the sym_xforms as you've described, and
the API not constraining them to be immutable for the lifetime of the session.

For asymmetric on the other hand the rte_crypto_asym_xform has from 2 to 7 larger buffers set up by the application
with input data to be used in the session. In QAT case, none of this data can be copied into the descriptor
and so the PMD would have to alloc duplicate structs and buffers for all this data and all the associated
meta-data (phys_addr ptrs) and copy it all, just to facilitate the application freeing the xform.
From a performance perspective, these copies seem wasteful, especially for an asymmetric session which is likely to have far fewer ops associate with it than a symmetric session would.
From a memory mgmt point of view it increases the memory resources needed.
From a security perspective, minimising the copies of key data in memory is preferable.
And it seems like a reasonable constraint on the Application to not free the xform - there are very few if any fields set up in it that it could re-use, so the only advantage I can see is that it would need a smaller pool of xforms if it could re-use them.

Though above reasons are from a QAT perspective, they surely also apply to sw PMDs and I'd guess they also apply to other hw devices - though would like feedback if different.
So I'm in favour of the API constraining the asym_xform and all its fields to be immutable for the lifetime of the session.
I get that this doesn't provide programmatic protection if an application ignores the API directive.
If you can suggest a way to protect this programmatically, that would be better, but even in the absence of this, I think it's worth
doing and think it's reasonable for application developers to be expected to follow the API documentation.

Fiona

> -----Original Message-----
> From: Anoob Joseph [mailto:anoobj@marvell.com]
> Sent: Friday, September 6, 2019 7:08 AM
> To: Shally Verma <shallyv@marvell.com>; Kusztal, ArkadiuszX <arkadiuszx.kusztal@intel.com>; Ayuj
> Verma <ayverma@marvell.com>; akhil.goyal@nxp.com
> Cc: Sunila Sahu <ssahu@marvell.com>; Kanaka Durga Kotamarthy <kkotamarthy@marvell.com>;
> dev@dpdk.org; Trahe, Fiona <fiona.trahe@intel.com>
> Subject: RE: [PATCH v1 0/2] declare crypto asym xform immutable
>
> Hi Shally, Arek,
>
> I still think it's a bit too early to take a decision on this. 'xform' is being used for symmetric operations
> as well. Everywhere, 'xform' is treated as an input argument to PMD ops. Making this change will
> define 'xform' in a different way, and that will be applicable only for asymmetric. I believe we can wait
> till we have a PMD which will be able to fully leverage this. Moreover, the same behavior could be
> achieved with session-less, if we declare that the number of operations done is "few". So my opinion
> is to push for session-less now and if it comes that the session based operation with immutable fields
> is superior to session-less model, we can purse this change then.
>
> Also, the main concern here is that the library will become more prone to errors. Issues arising out of
> this would be hard to debug. So if we make this change, we should make sure there are no better
> alternatives.
>
> Following is something, that we often do,
>
> xform.abc = 123
> sess1 = create_session(xform);
>
> xform.abc = 456;
> sess2 = create_session(xform);
>
> do_operation(sess1, op1);
> do_operation(sess2, op2);
>
> With the said change, the above could give bogus results and the code wouldn't look buggy until you
> read the documentation (and understand the usage).
>
> Thanks,
> Anoob
>
> From: Shally Verma <shallyv@marvell.com>
> Sent: Thursday, September 5, 2019 7:52 PM
> To: Kusztal, ArkadiuszX <arkadiuszx.kusztal@intel.com>; Ayuj Verma <ayverma@marvell.com>;
> Anoob Joseph <anoobj@marvell.com>; akhil.goyal@nxp.com
> Cc: Sunila Sahu <ssahu@marvell.com>; Kanaka Durga Kotamarthy <kkotamarthy@marvell.com>;
> dev@dpdk.org; Trahe, Fiona <fiona.trahe@intel.com>
> Subject: RE: [PATCH v1 0/2] declare crypto asym xform immutable
>
> Hi Arek
>
> Anoob has to have go, no-go on this further. Regarding your question on sessionless, have you
> submitted POC for same?
>
> Thanks
> Shally
>
> From: Kusztal, ArkadiuszX <mailto:arkadiuszx.kusztal@intel.com>
> Sent: Thursday, September 5, 2019 2:52 PM
> To: Ayuj Verma <mailto:ayverma@marvell.com>; Shally Verma <mailto:shallyv@marvell.com>; Anoob
> Joseph <mailto:anoobj@marvell.com>; mailto:akhil.goyal@nxp.com
> Cc: Sunila Sahu <mailto:ssahu@marvell.com>; Kanaka Durga Kotamarthy
> <mailto:kkotamarthy@marvell.com>; mailto:dev@dpdk.org; Trahe, Fiona
> <mailto:fiona.trahe@intel.com>
> Subject: RE: [PATCH v1 0/2] declare crypto asym xform immutable
>
> Hi all,
> What is the state of this patch, is it intended for 19.11 release?
> I would also propose to extend .rst comments to session-less case too like in this thread below (with
> [AK]).
> Regards,
> Arek
> ________________________________________
> From: Kusztal, ArkadiuszX <mailto:arkadiuszx.kusztal@intel.com>
> Sent: 25 July 2019 19:57
> To: Shally Verma <mailto:shallyv@marvell.com>; Anoob Joseph <mailto:anoobj@marvell.com>; Ayuj
> Verma <mailto:ayverma@marvell.com>; mailto:akhil.goyal@nxp.com <mailto:akhil.goyal@nxp.com>
> Cc: Sunila Sahu <mailto:ssahu@marvell.com>; Kanaka Durga Kotamarthy
> <mailto:kkotamarthy@marvell.com>; mailto:dev@dpdk.org <mailto:dev@dpdk.org>; Trahe, Fiona
> <mailto:fiona.trahe@intel.com>
> Subject: RE: [PATCH v1 0/2] declare crypto asym xform immutable
>
> Hi All,
>
> > > > I believe there are couple of issues with this patch.
> > > >
> > > > Are these experimental APIs? I believe they were made stable this
> > > > release and I'm not sure if it is a right practice to edit an API
> > > > without deprecation notice after it is made stable. Especially now
> > > > that RC2 is done. @Akhil, what is your take on this?
> > > > [Shally] These are experimental still, hence no deprecation notice.
> > > > We checked about it with Fiona, Akhil before.
> > >
> > > [Anoob] In the patch, the edited APIs doesn't have experimental tag. I
> > > leave it to Akhil's judgement on this.
> > >
> > > >
> > > > I think, the approach here is wrong. If the lifetime of the session
> > > > is expected to be only few packets, then session-less (which I
> > > > believe is in the pipeline) would make more sense.
> > > > [Shally] See my response further below on this.
> > > >
> > > > If the lifetime of the session is expected to be more than that,
> > > > then having this feature/limitation would make application more
> > > > complicated. Also, since one asymmetric session can hold both public
> > > > & private keys, the implicit assumption would be, the session can be
> > > > used for multiple kinds of operations. This change is in
> > > > contradiction with
> > > that.
> > > > [Shally] Why the contradiction here? There's no change in session
> > > > usage from current version. Currently too, once keys are set on
> > > > asymmetric session, they are used with multiple operations using
> > > > that sessions, example - once RSA xform is set with keys, then one
> > > > can perform sign/verify/enc/dec. So, I don't see any change in that
> > > > notion with this proposal. All we are changing is, PMD which does
> > > > not need to store keys in specific format (like openssl PMD), can
> > > > just hold app buffer pointer till session-lifetime (eventually
> > > > giving same effect as sessionless). It will help such PMDs to
> > > > optimize their session setup time by
> > > avoiding unnecessary memcpy of keys buffers.
> > >
> > > [Anoob] Contradiction is in the sense of what is a session. Here we
> > > are saying one session can have SIGN & VERIFY, but the lifetime  of
> > > the session is assumed to be "short".
> > [Shally] No. we only said, Session will only have keys (which is in xform) with
> > which SIGN & VERIFY will be performed during enqueue.
> > As long as there're operations to be performed using data set in session,
> > session data should not be manipulated.
> [AK] - we need to keep in mind that if we will copy user private key inside our internal structs,
> It will be stored in two places in the same time, making it more vulnerable, that's why I have asked
> about session lifetime.
> At least we could inform user of this when PMD copy data internally.
> Of course usually this data need to be copied anyway for op lifetime (because of some
> padding/alignement requirements hw may have).
>
> >
> > >
> > > >
> > > > But my major concern is how this can lead to accidental errors.
> > > > Making the argument as const will mean the API won't edit its
> > > > contents. But if there is a pointer in that (key happens to be a
> > > > pointer inside the xform), having const for xform will not help.
> > > > This is my understanding. Please correct me if I'm wrong.
> > > > [Shally] This spec says " xform and its buffers remain constant" .
> > > > So, intention is to state to apps that buffer passed to xform should
> > > > be const in nature and that they should not modify it.
> > >
> > > [Anoob] The current change could break existing applications. And the
> > > compiler will not be able to detect it.
> > >
> > [Shally] Application would need to be changed to make sure this criteria is
> > met. We took care to check same in asym unit test app while proposing
> > change.
> >
> > > >
> > > > Also, I could have the xform allocated from stack (non const,
> > > > regular local
> > > > variable) and then call the session_init. Would compiler throw an
> > > > issue in that case? I doubt so.
> > > >
> > > > void abc(const int t)
> > > > {
> > > >         printf("%d\n", t);
> > > > }
> > > >
> > > > void main()
> > > > {
> > > >         int t = 0;
> > > >         abc(t);
> > > >         t = 2;
> > > >         abc(t);
> > > > }
> > > >
> > > > To summarize, if this assumption is accepted, then compiler will not
> > > > be able to ensure it. And to properly use it, application will have
> > > > to be drafted differently. And when similar effect can be achieved
> > > > by having session-less, this seems redundant.
> > > > [Shally]  Compiler may or may not warn on typecast error here.
> > >
> > > [Anoob] May or may not be?
> > >
> > [Shally] Yes. Depending on compiler version type or optimization level.
> > Havent given try to all.
>
> [AK] Integers conversions are not good example as integers in C are governed by different rules than
> pointers.
> In terms of this
> -                       struct rte_crypto_asym_xform *xforms,
> +                       const struct rte_crypto_asym_xform *xforms,
> No compiler should complain about that as per spec: for any qualifier q, a pointer to a non-q-qualified
> type may be converted to a pointer to
> the q-qualified version of the type.
> We of course expect user will not change this data when there are still ops to be enqueued with this
> xform.
> [AK] - This note could be added to the session-less case.
> [Ayuj] Yes, also can't do below :
>                           const struct rte_crypto_asym_xform *xform;
>                           struct rte_crypto_modex_xform *xfrm = &(xform->modex);
>           it should needs to be const for both which will ensure its sanity :
>                           const struct rte_crypto_asym_xform *xform;
>                           const struct rte_crypto_modex_xform *xfrm = &(xform->modex);
>
> >
> > > > That's why
> > > > spec and documentation are put in place to ensure that application
> > > > don't reuse them or destroy them once "xform and its buffers" are
> > > > set on
> > > session.
> > > > And, same will need to be documented about xform for session-less
> > > > usage as well.
> > >
> > > [Anoob] Can you describe how this is required in session-less?
> > >
> > [Shally] We don't know how every PMD might use them. So, it is safe to mark
> > xform and buffers immutable there too.
> > So this is my thought but I have asked from POC for sessionless, as it is
> > proposal by Arek, am yet to get more feedback on that.
> >
> > > > Even there, we would ensure that application do not re-use or modify
> > > > xform and its buffers until dequeue happen. So, practically I see,
> > > > application would have to take of these cases in session-less as well.
> > > >
> > > > Since in session-based case, xform are set on it than ops, so we're
> > > > moving same definition on session. So for PMDs which support
> > > > sessions-based implementations ( like ours) , believe it completely
> > > > make sense to enable sessions to have sessionless effect.  If we
> > > > don't change spec to enable optimization, then we're making
> > > > 1-approach slower than other.  PMDs can adopt any approach more
> > suitable to them.
> > > > But spec could be made flexible to allow them to experiment with
> > > > both approaches for performance. Else , PMDs will be forced to
> > > > experiment around sessionless which may be eventually be an
> > > > unnecessary overhead
> > > for them.
> > >
> > > [Anoob] I get your intention on avoiding the memcpys. But the current
> > > changes would make the spec more prone to errors. And if we think we
> > > should improve the key handling done in both session & session-less, I
> > > would suggest not to rush with this change. We can keep the API
> > > experimental and continue improving it to fix this issue for all PMDs more
> > cleanly.
> > >
> > [Shally] There's no rush. It is up and open for feedback.
> > This change has an intent to optimize session setup time and since xform
> > data is not supposed to change once it is set on session. Only change
> > proposed is, why not just use app buffers instead of redundant copy of same
> > into PMD buffer.
> > if you see better suggestions , please provide.
> >
> > > >
> > > > Thanks
> > > > Shally
> > > >
> > > > So this change is NACK from my side.
> > > >
> > > > Thanks,
> > > > Anoob
> > > >
> > > > From: Ayuj Verma
> > > > Sent: Wednesday, July 24, 2019 2:23 PM
> > > > To: mailto:akhil.goyal@nxp.com
> > > > Cc: mailto:arkadiuszx.kusztal@intel.com; Shally Verma
> > > > <mailto:shallyv@marvell.com>; Sunila Sahu
> > > > <mailto:ssahu@marvell.com>; Kanaka Durga Kotamarthy
> > > > <mailto:kkotamarthy@marvell.com>; Anoob
> > > Joseph
> > > > <mailto:anoobj@marvell.com>; mailto:dev@dpdk.org; Fiona Trahe
> > > > <mailto:fiona.trahe@intel.com>
> > > > Subject: Re: [PATCH v1 0/2] declare crypto asym xform immutable
> > > >
> > > > +Fiona.
> > > > ________________________________________
> > > > From: Ayuj Verma <mailto:ayverma@marvell.com>
> > > > Sent: 24 July 2019 14:21:55
> > > > To: mailto:akhil.goyal@nxp.com <mailto:akhil.goyal@nxp.com>
> > > > Cc: mailto:arkadiuszx.kusztal@intel.com
> > > > <mailto:arkadiuszx.kusztal@intel.com>; Shally Verma
> > > > <mailto:shallyv@marvell.com>; Sunila Sahu
> > > > <mailto:ssahu@marvell.com>; Kanaka Durga Kotamarthy
> > > > <mailto:kkotamarthy@marvell.com>; Anoob
> > > Joseph
> > > > <mailto:anoobj@marvell.com>; mailto:dev@dpdk.org
> > > > <mailto:dev@dpdk.org>; Ayuj Verma <mailto:ayverma@marvell.com>
> > > > Subject: [PATCH v1 0/2] declare crypto asym xform immutable
> > > >
> > > > Mark asym xform as immutable till lifetime of session. It will save
> > > > session setup time for PMDs, which doesn't require any manipulation
> > > > of xform data, by directly using these buffers.
> > > >
> > > > * Updated xform type in session init/configure
> > > >   API as constant.
> > > > * Updated doc with proper transform description.
> > > > * Updated openssl PMD with above changes.
> > > >
> > > > Ayuj Verma (2):
> > > >   lib/crypto: declare crypto asym xform immutable
> > > >   crypto/openssl: mark asym xform constant
> > > >
> > > >  doc/guides/prog_guide/cryptodev_lib.rst      | 10 ++++++++++
> > > >  drivers/crypto/openssl/rte_openssl_pmd_ops.c |  8 ++++----
> > > >  lib/librte_cryptodev/rte_cryptodev.c         |  2 +-
> > > >  lib/librte_cryptodev/rte_cryptodev.h         |  2 +-
> > > >  lib/librte_cryptodev/rte_cryptodev_pmd.h     |  2 +-
> > > >  5 files changed, 17 insertions(+), 7 deletions(-)
> > > >
> > > > --
> > > > 1.8.3.1

Re: [dpdk-dev] [PATCH v1 0/2] declare crypto asym xform immutable

[Kusztal, ArkadiuszX]

Hi Sunila,

With [AK]
From: Sunila Sahu [mailto:ssahu@marvell.com]
Sent: Friday, September 13, 2019 4:02 PM
To: Trahe, Fiona <fiona.trahe@intel.com>; Anoob Joseph <anoobj@marvell.com>; Shally Verma <shallyv@marvell.com>; Kusztal, ArkadiuszX <arkadiuszx.kusztal@intel.com>; Ayuj Verma <ayverma@marvell.com>; akhil.goyal@nxp.com
Cc: Kanaka Durga Kotamarthy <kkotamarthy@marvell.com>; dev@dpdk.org
Subject: Re: [PATCH v1 0/2] declare crypto asym xform immutable

Hi Fiona, Anoob,

Fiona,

> Though above reasons are from a QAT perspective, they surely also apply to sw PMDs and
> I'd guess they also apply to other hw devices - though would like feedback if different.

Declaring crypto asym xform immutable does save allocation and copy of key data into OCTEON PMD
during session configuration.
But since hw requires a definitive alignment, xform data has to be copied for op enqueue. So in
this case session-less implementation is beneficial.
I think there is value in both the implementations depending on the PMD.

Anoob,

> Moreover, the same behavior could be
> achieved with session-less, if we declare that the number of operations done is "few".

Not necessarily the number of operations done here is "few". It entirely depends on key usage.
If the key is ephemeral then your operation will be limited to a single transaction as in case of a key
exchange mechanism.
But for other operations as digital signature, the life-time of the session is not limited to a single operation.
Also, there can be different ops (public key or private key authentication) with a single session.
In that case, session implementation will have an advantage over sessionless.
[AK] - not necessarily agree with ephemeral part, for DH generator and group order are in xform and may be reused for another ephemeral key exchange (if another user will pick the same group number, you don't have too many of them). Same would apply to domain parameters of elliptic curves. But the question remains "will any hw will make gain in copying this data into some internal structs during session initialization?"


Thanks,
Sunila


Regards,
Arek
________________________________
From: Trahe, Fiona <fiona.trahe@intel.com<mailto:fiona.trahe@intel.com>>
Sent: Friday, September 6, 2019 4:55 PM
To: Anoob Joseph <anoobj@marvell.com<mailto:anoobj@marvell.com>>; Shally Verma <shallyv@marvell.com<mailto:shallyv@marvell.com>>; Kusztal, ArkadiuszX <arkadiuszx.kusztal@intel.com<mailto:arkadiuszx.kusztal@intel.com>>; Ayuj Verma <ayverma@marvell.com<mailto:ayverma@marvell.com>>; akhil.goyal@nxp.com<mailto:akhil.goyal@nxp.com> <akhil.goyal@nxp.com<mailto:akhil.goyal@nxp.com>>
Cc: Sunila Sahu <ssahu@marvell.com<mailto:ssahu@marvell.com>>; Kanaka Durga Kotamarthy <kkotamarthy@marvell.com<mailto:kkotamarthy@marvell.com>>; dev@dpdk.org<mailto:dev@dpdk.org> <dev@dpdk.org<mailto:dev@dpdk.org>>; Trahe, Fiona <fiona.trahe@intel.com<mailto:fiona.trahe@intel.com>>
Subject: [EXT] RE: [PATCH v1 0/2] declare crypto asym xform immutable

Hi Anoob, Arek, Shally,

The fundamental question here seems to be whether it's ok to specify the asymmetric xform to
have different behaviour to the symmetric xform.
In my opinion it is.
At least in terms of our QuickAssist PMD and its use by symmetric and
asymmetric applications I don't see any conflict with the xforms having different characteristics.
They are two different structures.

For rte_crypto_sym_xform, there are 2, usually small, params - key and iv - that need to be copied into
the descriptor to be sent to the hardware, so a copy can't be avoided there and it helps to get this
 copy out of the way during the session. This prep helps to make with-session symmetric more
performant than session-less would be.
And so I don't see any problem in an application re-using the sym_xforms as you've described, and
the API not constraining them to be immutable for the lifetime of the session.

For asymmetric on the other hand the rte_crypto_asym_xform has from 2 to 7 larger buffers set up by the application
with input data to be used in the session. In QAT case, none of this data can be copied into the descriptor
and so the PMD would have to alloc duplicate structs and buffers for all this data and all the associated
meta-data (phys_addr ptrs) and copy it all, just to facilitate the application freeing the xform.
From a performance perspective, these copies seem wasteful, especially for an asymmetric session which is likely to have far fewer ops associate with it than a symmetric session would.
From a memory mgmt point of view it increases the memory resources needed.
From a security perspective, minimising the copies of key data in memory is preferable.
And it seems like a reasonable constraint on the Application to not free the xform - there are very few if any fields set up in it that it could re-use, so the only advantage I can see is that it would need a smaller pool of xforms if it could re-use them.

Though above reasons are from a QAT perspective, they surely also apply to sw PMDs and I'd guess they also apply to other hw devices - though would like feedback if different.
So I'm in favour of the API constraining the asym_xform and all its fields to be immutable for the lifetime of the session.
I get that this doesn't provide programmatic protection if an application ignores the API directive.
If you can suggest a way to protect this programmatically, that would be better, but even in the absence of this, I think it's worth
doing and think it's reasonable for application developers to be expected to follow the API documentation.

Fiona

> -----Original Message-----
> From: Anoob Joseph [mailto:anoobj@marvell.com]
> Sent: Friday, September 6, 2019 7:08 AM
> To: Shally Verma <shallyv@marvell.com<mailto:shallyv@marvell.com>>; Kusztal, ArkadiuszX <arkadiuszx.kusztal@intel.com<mailto:arkadiuszx.kusztal@intel.com>>; Ayuj
> Verma <ayverma@marvell.com<mailto:ayverma@marvell.com>>; akhil.goyal@nxp.com<mailto:akhil.goyal@nxp.com>
> Cc: Sunila Sahu <ssahu@marvell.com<mailto:ssahu@marvell.com>>; Kanaka Durga Kotamarthy <kkotamarthy@marvell.com<mailto:kkotamarthy@marvell.com>>;
> dev@dpdk.org<mailto:dev@dpdk.org>; Trahe, Fiona <fiona.trahe@intel.com<mailto:fiona.trahe@intel.com>>
> Subject: RE: [PATCH v1 0/2] declare crypto asym xform immutable
>
> Hi Shally, Arek,
>
> I still think it's a bit too early to take a decision on this. 'xform' is being used for symmetric operations
> as well. Everywhere, 'xform' is treated as an input argument to PMD ops. Making this change will
> define 'xform' in a different way, and that will be applicable only for asymmetric. I believe we can wait
> till we have a PMD which will be able to fully leverage this. Moreover, the same behavior could be
> achieved with session-less, if we declare that the number of operations done is "few". So my opinion
> is to push for session-less now and if it comes that the session based operation with immutable fields
> is superior to session-less model, we can purse this change then.
>
> Also, the main concern here is that the library will become more prone to errors. Issues arising out of
> this would be hard to debug. So if we make this change, we should make sure there are no better
> alternatives.
>
> Following is something, that we often do,
>
> xform.abc = 123
> sess1 = create_session(xform);
>
> xform.abc = 456;
> sess2 = create_session(xform);
>
> do_operation(sess1, op1);
> do_operation(sess2, op2);
>
> With the said change, the above could give bogus results and the code wouldn't look buggy until you
> read the documentation (and understand the usage).
>
> Thanks,
> Anoob
>
> From: Shally Verma <shallyv@marvell.com<mailto:shallyv@marvell.com>>
> Sent: Thursday, September 5, 2019 7:52 PM
> To: Kusztal, ArkadiuszX <arkadiuszx.kusztal@intel.com<mailto:arkadiuszx.kusztal@intel.com>>; Ayuj Verma <ayverma@marvell.com<mailto:ayverma@marvell.com>>;
> Anoob Joseph <anoobj@marvell.com<mailto:anoobj@marvell.com>>; akhil.goyal@nxp.com<mailto:akhil.goyal@nxp.com>
> Cc: Sunila Sahu <ssahu@marvell.com<mailto:ssahu@marvell.com>>; Kanaka Durga Kotamarthy <kkotamarthy@marvell.com<mailto:kkotamarthy@marvell.com>>;
> dev@dpdk.org<mailto:dev@dpdk.org>; Trahe, Fiona <fiona.trahe@intel.com<mailto:fiona.trahe@intel.com>>
> Subject: RE: [PATCH v1 0/2] declare crypto asym xform immutable
>
> Hi Arek
>
> Anoob has to have go, no-go on this further. Regarding your question on sessionless, have you
> submitted POC for same?
>
> Thanks
> Shally
>
> From: Kusztal, ArkadiuszX <mailto:arkadiuszx.kusztal@intel.com>
> Sent: Thursday, September 5, 2019 2:52 PM
> To: Ayuj Verma <mailto:ayverma@marvell.com>; Shally Verma <mailto:shallyv@marvell.com>; Anoob
> Joseph <mailto:anoobj@marvell.com>; mailto:akhil.goyal@nxp.com
> Cc: Sunila Sahu <mailto:ssahu@marvell.com>; Kanaka Durga Kotamarthy
> <mailto:kkotamarthy@marvell.com>; mailto:dev@dpdk.org; Trahe, Fiona
> <mailto:fiona.trahe@intel.com>
> Subject: RE: [PATCH v1 0/2] declare crypto asym xform immutable
>
> Hi all,
> What is the state of this patch, is it intended for 19.11 release?
> I would also propose to extend .rst comments to session-less case too like in this thread below (with
> [AK]).
> Regards,
> Arek
> ________________________________________
> From: Kusztal, ArkadiuszX <mailto:arkadiuszx.kusztal@intel.com>
> Sent: 25 July 2019 19:57
> To: Shally Verma <mailto:shallyv@marvell.com>; Anoob Joseph <mailto:anoobj@marvell.com>; Ayuj
> Verma <mailto:ayverma@marvell.com>; mailto:akhil.goyal@nxp.com <mailto:akhil.goyal@nxp.com>
> Cc: Sunila Sahu <mailto:ssahu@marvell.com>; Kanaka Durga Kotamarthy
> <mailto:kkotamarthy@marvell.com>; mailto:dev@dpdk.org <mailto:dev@dpdk.org>; Trahe, Fiona
> <mailto:fiona.trahe@intel.com>
> Subject: RE: [PATCH v1 0/2] declare crypto asym xform immutable
>
> Hi All,
>
> > > > I believe there are couple of issues with this patch.
> > > >
> > > > Are these experimental APIs? I believe they were made stable this
> > > > release and I'm not sure if it is a right practice to edit an API
> > > > without deprecation notice after it is made stable. Especially now
> > > > that RC2 is done. @Akhil, what is your take on this?
> > > > [Shally] These are experimental still, hence no deprecation notice.
> > > > We checked about it with Fiona, Akhil before.
> > >
> > > [Anoob] In the patch, the edited APIs doesn't have experimental tag. I
> > > leave it to Akhil's judgement on this.
> > >
> > > >
> > > > I think, the approach here is wrong. If the lifetime of the session
> > > > is expected to be only few packets, then session-less (which I
> > > > believe is in the pipeline) would make more sense.
> > > > [Shally] See my response further below on this.
> > > >
> > > > If the lifetime of the session is expected to be more than that,
> > > > then having this feature/limitation would make application more
> > > > complicated. Also, since one asymmetric session can hold both public
> > > > & private keys, the implicit assumption would be, the session can be
> > > > used for multiple kinds of operations. This change is in
> > > > contradiction with
> > > that.
> > > > [Shally] Why the contradiction here? There's no change in session
> > > > usage from current version. Currently too, once keys are set on
> > > > asymmetric session, they are used with multiple operations using
> > > > that sessions, example - once RSA xform is set with keys, then one
> > > > can perform sign/verify/enc/dec. So, I don't see any change in that
> > > > notion with this proposal. All we are changing is, PMD which does
> > > > not need to store keys in specific format (like openssl PMD), can
> > > > just hold app buffer pointer till session-lifetime (eventually
> > > > giving same effect as sessionless). It will help such PMDs to
> > > > optimize their session setup time by
> > > avoiding unnecessary memcpy of keys buffers.
> > >
> > > [Anoob] Contradiction is in the sense of what is a session. Here we
> > > are saying one session can have SIGN & VERIFY, but the lifetime  of
> > > the session is assumed to be "short".
> > [Shally] No. we only said, Session will only have keys (which is in xform) with
> > which SIGN & VERIFY will be performed during enqueue.
> > As long as there're operations to be performed using data set in session,
> > session data should not be manipulated.
> [AK] - we need to keep in mind that if we will copy user private key inside our internal structs,
> It will be stored in two places in the same time, making it more vulnerable, that's why I have asked
> about session lifetime.
> At least we could inform user of this when PMD copy data internally.
> Of course usually this data need to be copied anyway for op lifetime (because of some
> padding/alignement requirements hw may have).
>
> >
> > >
> > > >
> > > > But my major concern is how this can lead to accidental errors.
> > > > Making the argument as const will mean the API won't edit its
> > > > contents. But if there is a pointer in that (key happens to be a
> > > > pointer inside the xform), having const for xform will not help.
> > > > This is my understanding. Please correct me if I'm wrong.
> > > > [Shally] This spec says " xform and its buffers remain constant" .
> > > > So, intention is to state to apps that buffer passed to xform should
> > > > be const in nature and that they should not modify it.
> > >
> > > [Anoob] The current change could break existing applications. And the
> > > compiler will not be able to detect it.
> > >
> > [Shally] Application would need to be changed to make sure this criteria is
> > met. We took care to check same in asym unit test app while proposing
> > change.
> >
> > > >
> > > > Also, I could have the xform allocated from stack (non const,
> > > > regular local
> > > > variable) and then call the session_init. Would compiler throw an
> > > > issue in that case? I doubt so.
> > > >
> > > > void abc(const int t)
> > > > {
> > > >         printf("%d\n", t);
> > > > }
> > > >
> > > > void main()
> > > > {
> > > >         int t = 0;
> > > >         abc(t);
> > > >         t = 2;
> > > >         abc(t);
> > > > }
> > > >
> > > > To summarize, if this assumption is accepted, then compiler will not
> > > > be able to ensure it. And to properly use it, application will have
> > > > to be drafted differently. And when similar effect can be achieved
> > > > by having session-less, this seems redundant.
> > > > [Shally]  Compiler may or may not warn on typecast error here.
> > >
> > > [Anoob] May or may not be?
> > >
> > [Shally] Yes. Depending on compiler version type or optimization level.
> > Havent given try to all.
>
> [AK] Integers conversions are not good example as integers in C are governed by different rules than
> pointers.
> In terms of this
> -                       struct rte_crypto_asym_xform *xforms,
> +                       const struct rte_crypto_asym_xform *xforms,
> No compiler should complain about that as per spec: for any qualifier q, a pointer to a non-q-qualified
> type may be converted to a pointer to
> the q-qualified version of the type.
> We of course expect user will not change this data when there are still ops to be enqueued with this
> xform.
> [AK] - This note could be added to the session-less case.
> [Ayuj] Yes, also can't do below :
>                           const struct rte_crypto_asym_xform *xform;
>                           struct rte_crypto_modex_xform *xfrm = &(xform->modex);
>           it should needs to be const for both which will ensure its sanity :
>                           const struct rte_crypto_asym_xform *xform;
>                           const struct rte_crypto_modex_xform *xfrm = &(xform->modex);
>
> >
> > > > That's why
> > > > spec and documentation are put in place to ensure that application
> > > > don't reuse them or destroy them once "xform and its buffers" are
> > > > set on
> > > session.
> > > > And, same will need to be documented about xform for session-less
> > > > usage as well.
> > >
> > > [Anoob] Can you describe how this is required in session-less?
> > >
> > [Shally] We don't know how every PMD might use them. So, it is safe to mark
> > xform and buffers immutable there too.
> > So this is my thought but I have asked from POC for sessionless, as it is
> > proposal by Arek, am yet to get more feedback on that.
> >
> > > > Even there, we would ensure that application do not re-use or modify
> > > > xform and its buffers until dequeue happen. So, practically I see,
> > > > application would have to take of these cases in session-less as well.
> > > >
> > > > Since in session-based case, xform are set on it than ops, so we're
> > > > moving same definition on session. So for PMDs which support
> > > > sessions-based implementations ( like ours) , believe it completely
> > > > make sense to enable sessions to have sessionless effect.  If we
> > > > don't change spec to enable optimization, then we're making
> > > > 1-approach slower than other.  PMDs can adopt any approach more
> > suitable to them.
> > > > But spec could be made flexible to allow them to experiment with
> > > > both approaches for performance. Else , PMDs will be forced to
> > > > experiment around sessionless which may be eventually be an
> > > > unnecessary overhead
> > > for them.
> > >
> > > [Anoob] I get your intention on avoiding the memcpys. But the current
> > > changes would make the spec more prone to errors. And if we think we
> > > should improve the key handling done in both session & session-less, I
> > > would suggest not to rush with this change. We can keep the API
> > > experimental and continue improving it to fix this issue for all PMDs more
> > cleanly.
> > >
> > [Shally] There's no rush. It is up and open for feedback.
> > This change has an intent to optimize session setup time and since xform
> > data is not supposed to change once it is set on session. Only change
> > proposed is, why not just use app buffers instead of redundant copy of same
> > into PMD buffer.
> > if you see better suggestions , please provide.
> >
> > > >
> > > > Thanks
> > > > Shally
> > > >
> > > > So this change is NACK from my side.
> > > >
> > > > Thanks,
> > > > Anoob
> > > >
> > > > From: Ayuj Verma
> > > > Sent: Wednesday, July 24, 2019 2:23 PM
> > > > To: mailto:akhil.goyal@nxp.com
> > > > Cc: mailto:arkadiuszx.kusztal@intel.com; Shally Verma
> > > > <mailto:shallyv@marvell.com>; Sunila Sahu
> > > > <mailto:ssahu@marvell.com>; Kanaka Durga Kotamarthy
> > > > <mailto:kkotamarthy@marvell.com>; Anoob
> > > Joseph
> > > > <mailto:anoobj@marvell.com>; mailto:dev@dpdk.org; Fiona Trahe
> > > > <mailto:fiona.trahe@intel.com>
> > > > Subject: Re: [PATCH v1 0/2] declare crypto asym xform immutable
> > > >
> > > > +Fiona.
> > > > ________________________________________
> > > > From: Ayuj Verma <mailto:ayverma@marvell.com>
> > > > Sent: 24 July 2019 14:21:55
> > > > To: mailto:akhil.goyal@nxp.com <mailto:akhil.goyal@nxp.com>
> > > > Cc: mailto:arkadiuszx.kusztal@intel.com
> > > > <mailto:arkadiuszx.kusztal@intel.com>; Shally Verma
> > > > <mailto:shallyv@marvell.com>; Sunila Sahu
> > > > <mailto:ssahu@marvell.com>; Kanaka Durga Kotamarthy
> > > > <mailto:kkotamarthy@marvell.com>; Anoob
> > > Joseph
> > > > <mailto:anoobj@marvell.com>; mailto:dev@dpdk.org
> > > > <mailto:dev@dpdk.org>; Ayuj Verma <mailto:ayverma@marvell.com>
> > > > Subject: [PATCH v1 0/2] declare crypto asym xform immutable
> > > >
> > > > Mark asym xform as immutable till lifetime of session. It will save
> > > > session setup time for PMDs, which doesn't require any manipulation
> > > > of xform data, by directly using these buffers.
> > > >
> > > > * Updated xform type in session init/configure
> > > >   API as constant.
> > > > * Updated doc with proper transform description.
> > > > * Updated openssl PMD with above changes.
> > > >
> > > > Ayuj Verma (2):
> > > >   lib/crypto: declare crypto asym xform immutable
> > > >   crypto/openssl: mark asym xform constant
> > > >
> > > >  doc/guides/prog_guide/cryptodev_lib.rst      | 10 ++++++++++
> > > >  drivers/crypto/openssl/rte_openssl_pmd_ops.c |  8 ++++----
> > > >  lib/librte_cryptodev/rte_cryptodev.c         |  2 +-
> > > >  lib/librte_cryptodev/rte_cryptodev.h         |  2 +-
> > > >  lib/librte_cryptodev/rte_cryptodev_pmd.h     |  2 +-
> > > >  5 files changed, 17 insertions(+), 7 deletions(-)
> > > >
> > > > --
> > > > 1.8.3.1

Re: [dpdk-dev] [PATCH v1 0/2] declare crypto asym xform immutable

[Akhil Goyal]

Hi Anoob/Shally

> >
> > Hi Ayuj,
> >
> > I believe there are couple of issues with this patch.
> >
> > Are these experimental APIs? I believe they were made stable this release
> > and I'm not sure if it is a right practice to edit an API without deprecation
> > notice after it is made stable. Especially now that RC2 is done. @Akhil, what is
> > your take on this?
> > [Shally] These are experimental still, hence no deprecation notice. We
> > checked about it with Fiona, Akhil before.
> 
> [Anoob] In the patch, the edited APIs doesn't have experimental tag. I leave it to
> Akhil's judgement on this.
> 
The API is experimental. Check the .map file.

But these kind of patches cannot go in 1908 release, as we have already tagged it for RC2.
We need to defer this change for 19.11 release.

One more issue that I see in this patch. QAT also uses these APIs, so it shall also be modified along with openssl.	


> >
> > I think, the approach here is wrong. If the lifetime of the session is expected
> > to be only few packets, then session-less (which I believe is in the pipeline)
> > would make more sense.
> > [Shally] See my response further below on this.
> >
> > If the lifetime of the session is expected to be more than that, then having
> > this feature/limitation would make application more complicated. Also, since
> > one asymmetric session can hold both public & private keys, the implicit
> > assumption would be, the session can be used for multiple kinds of
> > operations. This change is in contradiction with that.
> > [Shally] Why the contradiction here? There's no change in session usage from
> > current version. Currently too, once keys are set on asymmetric session, they
> > are used with multiple operations using that sessions, example - once RSA
> > xform is set with keys, then one can perform sign/verify/enc/dec. So, I don't
> > see any change in that notion with this proposal. All we are changing is, PMD
> > which does not need to store keys in specific format (like openssl PMD), can
> > just hold app buffer pointer till session-lifetime (eventually giving same
> > effect as sessionless). It will help such PMDs to optimize their session setup
> > time by avoiding unnecessary memcpy of keys buffers.
> 
> [Anoob] Contradiction is in the sense of what is a session. Here we are saying
> one session can have SIGN & VERIFY, but the lifetime  of the session is assumed
> to be "short".
> 
> >
> > But my major concern is how this can lead to accidental errors. Making the
> > argument as const will mean the API won't edit its contents. But if there is a
> > pointer in that (key happens to be a pointer inside the xform), having const
> > for xform will not help. This is my understanding. Please correct me if I'm
> > wrong.
> > [Shally] This spec says " xform and its buffers remain constant" . So, intention
> > is to state to apps that buffer passed to xform should be const in nature and
> > that they should not modify it.
> 
> [Anoob] The current change could break existing applications. And the compiler
> will not be able to detect it.
> 
> >
> > Also, I could have the xform allocated from stack (non const, regular local
> > variable) and then call the session_init. Would compiler throw an issue in that
> > case? I doubt so.
> >
> > void abc(const int t)
> > {
> >         printf("%d\n", t);
> > }
> >
> > void main()
> > {
> >         int t = 0;
> >         abc(t);
> >         t = 2;
> >         abc(t);
> > }
> >
> > To summarize, if this assumption is accepted, then compiler will not be able
> > to ensure it. And to properly use it, application will have to be drafted
> > differently. And when similar effect can be achieved by having session-less,
> > this seems redundant.
> > [Shally]  Compiler may or may not warn on typecast error here.
> 
> [Anoob] May or may not be?
> 
> > That's why
> > spec and documentation are put in place to ensure that application don't
> > reuse them or destroy them once "xform and its buffers" are set on session.
> > And, same will need to be documented about xform for session-less usage
> > as well.
> 
> [Anoob] Can you describe how this is required in session-less?
> 
> > Even there, we would ensure that application do not re-use or modify xform
> > and its buffers until dequeue happen. So, practically I see, application would
> > have to take of these cases in session-less as well.
> >
> > Since in session-based case, xform are set on it than ops, so we're moving
> > same definition on session. So for PMDs which support sessions-based
> > implementations ( like ours) , believe it completely make sense to enable
> > sessions to have sessionless effect.  If we don't change spec to enable
> > optimization, then we're making 1-approach slower than other.  PMDs can
> > adopt any approach more suitable to them. But spec could be made flexible
> > to allow them to experiment with both approaches for performance. Else ,
> > PMDs will be forced to experiment around sessionless which may be
> > eventually be an unnecessary overhead for them.
> 
> [Anoob] I get your intention on avoiding the memcpys. But the current changes
> would make the spec more prone to errors. And if we think we should improve
> the key handling done in both session & session-less, I would suggest not to rush
> with this change. We can keep the API experimental and continue improving it to
> fix this issue for all PMDs more cleanly.
> 
> >
> > Thanks
> > Shally
> >
> > So this change is NACK from my side.
> >
> > Thanks,
> > Anoob
> >
> > From: Ayuj Verma
> > Sent: Wednesday, July 24, 2019 2:23 PM
> > To: mailto:akhil.goyal@nxp.com
> > Cc: mailto:arkadiuszx.kusztal@intel.com; Shally Verma
> > <mailto:shallyv@marvell.com>; Sunila Sahu <mailto:ssahu@marvell.com>;
> > Kanaka Durga Kotamarthy <mailto:kkotamarthy@marvell.com>; Anoob
> > Joseph <mailto:anoobj@marvell.com>; mailto:dev@dpdk.org; Fiona Trahe
> > <mailto:fiona.trahe@intel.com>
> > Subject: Re: [PATCH v1 0/2] declare crypto asym xform immutable
> >
> > +Fiona.
> > ________________________________________
> > From: Ayuj Verma <mailto:ayverma@marvell.com>
> > Sent: 24 July 2019 14:21:55
> > To: mailto:akhil.goyal@nxp.com <mailto:akhil.goyal@nxp.com>
> > Cc: mailto:arkadiuszx.kusztal@intel.com
> > <mailto:arkadiuszx.kusztal@intel.com>; Shally Verma
> > <mailto:shallyv@marvell.com>; Sunila Sahu <mailto:ssahu@marvell.com>;
> > Kanaka Durga Kotamarthy <mailto:kkotamarthy@marvell.com>; Anoob
> > Joseph <mailto:anoobj@marvell.com>; mailto:dev@dpdk.org
> > <mailto:dev@dpdk.org>; Ayuj Verma <mailto:ayverma@marvell.com>
> > Subject: [PATCH v1 0/2] declare crypto asym xform immutable
> >
> > Mark asym xform as immutable till lifetime of session. It will save session
> > setup time for PMDs, which doesn't require any manipulation of xform data,
> > by directly using these buffers.
> >
> > * Updated xform type in session init/configure
> >   API as constant.
> > * Updated doc with proper transform description.
> > * Updated openssl PMD with above changes.
> >
> > Ayuj Verma (2):
> >   lib/crypto: declare crypto asym xform immutable
> >   crypto/openssl: mark asym xform constant
> >
> >  doc/guides/prog_guide/cryptodev_lib.rst      | 10 ++++++++++
> >  drivers/crypto/openssl/rte_openssl_pmd_ops.c |  8 ++++----
> >  lib/librte_cryptodev/rte_cryptodev.c         |  2 +-
> >  lib/librte_cryptodev/rte_cryptodev.h         |  2 +-
> >  lib/librte_cryptodev/rte_cryptodev_pmd.h     |  2 +-
> >  5 files changed, 17 insertions(+), 7 deletions(-)
> >
> > --
> > 1.8.3.1

Re: [dpdk-dev] [PATCH v1 0/2] declare crypto asym xform immutable

[Shally Verma]

Hi Akhil

> -----Original Message-----
> From: Akhil Goyal <akhil.goyal@nxp.com>
> Sent: Thursday, July 25, 2019 4:58 PM
> To: Anoob Joseph <anoobj@marvell.com>; Shally Verma
> <shallyv@marvell.com>; Ayuj Verma <ayverma@marvell.com>
> Cc: arkadiuszx.kusztal@intel.com; Sunila Sahu <ssahu@marvell.com>; Kanaka
> Durga Kotamarthy <kkotamarthy@marvell.com>; dev@dpdk.org; Fiona
> Trahe <fiona.trahe@intel.com>
> Subject: RE: [PATCH v1 0/2] declare crypto asym xform immutable
> 
> Hi Anoob/Shally
> 
> > >
> > > Hi Ayuj,
> > >
> > > I believe there are couple of issues with this patch.
> > >
> > > Are these experimental APIs? I believe they were made stable this
> > > release and I'm not sure if it is a right practice to edit an API
> > > without deprecation notice after it is made stable. Especially now
> > > that RC2 is done. @Akhil, what is your take on this?
> > > [Shally] These are experimental still, hence no deprecation notice.
> > > We checked about it with Fiona, Akhil before.
> >
> > [Anoob] In the patch, the edited APIs doesn't have experimental tag. I
> > leave it to Akhil's judgement on this.
> >
> The API is experimental. Check the .map file.
> 
> But these kind of patches cannot go in 1908 release, as we have already
> tagged it for RC2.
> We need to defer this change for 19.11 release.
> 
[Shally] Well. Cant do anything if we have missed deadline here. Right now its up for feedback.
Meanwhile if we get any agreement and window gets open for nearest release, then please consider it.

> One more issue that I see in this patch. QAT also uses these APIs, so it shall
> also be modified along with openssl.
> 
[Shally] We assume QAT changes will be done by QAT maintainer, once they review and approve this change.

Thanks
Shally
> 
> > >
> > > I think, the approach here is wrong. If the lifetime of the session
> > > is expected to be only few packets, then session-less (which I
> > > believe is in the pipeline) would make more sense.
> > > [Shally] See my response further below on this.
> > >
> > > If the lifetime of the session is expected to be more than that,
> > > then having this feature/limitation would make application more
> > > complicated. Also, since one asymmetric session can hold both public
> > > & private keys, the implicit assumption would be, the session can be
> > > used for multiple kinds of operations. This change is in contradiction with
> that.
> > > [Shally] Why the contradiction here? There's no change in session
> > > usage from current version. Currently too, once keys are set on
> > > asymmetric session, they are used with multiple operations using
> > > that sessions, example - once RSA xform is set with keys, then one
> > > can perform sign/verify/enc/dec. So, I don't see any change in that
> > > notion with this proposal. All we are changing is, PMD which does
> > > not need to store keys in specific format (like openssl PMD), can
> > > just hold app buffer pointer till session-lifetime (eventually
> > > giving same effect as sessionless). It will help such PMDs to optimize their
> session setup time by avoiding unnecessary memcpy of keys buffers.
> >
> > [Anoob] Contradiction is in the sense of what is a session. Here we
> > are saying one session can have SIGN & VERIFY, but the lifetime  of
> > the session is assumed to be "short".
> >
> > >
> > > But my major concern is how this can lead to accidental errors.
> > > Making the argument as const will mean the API won't edit its
> > > contents. But if there is a pointer in that (key happens to be a
> > > pointer inside the xform), having const for xform will not help.
> > > This is my understanding. Please correct me if I'm wrong.
> > > [Shally] This spec says " xform and its buffers remain constant" .
> > > So, intention is to state to apps that buffer passed to xform should
> > > be const in nature and that they should not modify it.
> >
> > [Anoob] The current change could break existing applications. And the
> > compiler will not be able to detect it.
> >
> > >
> > > Also, I could have the xform allocated from stack (non const,
> > > regular local
> > > variable) and then call the session_init. Would compiler throw an
> > > issue in that case? I doubt so.
> > >
> > > void abc(const int t)
> > > {
> > >         printf("%d\n", t);
> > > }
> > >
> > > void main()
> > > {
> > >         int t = 0;
> > >         abc(t);
> > >         t = 2;
> > >         abc(t);
> > > }
> > >
> > > To summarize, if this assumption is accepted, then compiler will not
> > > be able to ensure it. And to properly use it, application will have
> > > to be drafted differently. And when similar effect can be achieved
> > > by having session-less, this seems redundant.
> > > [Shally]  Compiler may or may not warn on typecast error here.
> >
> > [Anoob] May or may not be?
> >
> > > That's why
> > > spec and documentation are put in place to ensure that application
> > > don't reuse them or destroy them once "xform and its buffers" are set on
> session.
> > > And, same will need to be documented about xform for session-less
> > > usage as well.
> >
> > [Anoob] Can you describe how this is required in session-less?
> >
> > > Even there, we would ensure that application do not re-use or modify
> > > xform and its buffers until dequeue happen. So, practically I see,
> > > application would have to take of these cases in session-less as well.
> > >
> > > Since in session-based case, xform are set on it than ops, so we're
> > > moving same definition on session. So for PMDs which support
> > > sessions-based implementations ( like ours) , believe it completely
> > > make sense to enable sessions to have sessionless effect.  If we
> > > don't change spec to enable optimization, then we're making
> > > 1-approach slower than other.  PMDs can adopt any approach more
> > > suitable to them. But spec could be made flexible to allow them to
> > > experiment with both approaches for performance. Else , PMDs will be
> > > forced to experiment around sessionless which may be eventually be an
> unnecessary overhead for them.
> >
> > [Anoob] I get your intention on avoiding the memcpys. But the current
> > changes would make the spec more prone to errors. And if we think we
> > should improve the key handling done in both session & session-less, I
> > would suggest not to rush with this change. We can keep the API
> > experimental and continue improving it to fix this issue for all PMDs more
> cleanly.
> >
> > >
> > > Thanks
> > > Shally
> > >
> > > So this change is NACK from my side.
> > >
> > > Thanks,
> > > Anoob
> > >
> > > From: Ayuj Verma
> > > Sent: Wednesday, July 24, 2019 2:23 PM
> > > To: mailto:akhil.goyal@nxp.com
> > > Cc: mailto:arkadiuszx.kusztal@intel.com; Shally Verma
> > > <mailto:shallyv@marvell.com>; Sunila Sahu
> > > <mailto:ssahu@marvell.com>; Kanaka Durga Kotamarthy
> > > <mailto:kkotamarthy@marvell.com>; Anoob Joseph
> > > <mailto:anoobj@marvell.com>; mailto:dev@dpdk.org; Fiona Trahe
> > > <mailto:fiona.trahe@intel.com>
> > > Subject: Re: [PATCH v1 0/2] declare crypto asym xform immutable
> > >
> > > +Fiona.
> > > ________________________________________
> > > From: Ayuj Verma <mailto:ayverma@marvell.com>
> > > Sent: 24 July 2019 14:21:55
> > > To: mailto:akhil.goyal@nxp.com <mailto:akhil.goyal@nxp.com>
> > > Cc: mailto:arkadiuszx.kusztal@intel.com
> > > <mailto:arkadiuszx.kusztal@intel.com>; Shally Verma
> > > <mailto:shallyv@marvell.com>; Sunila Sahu
> > > <mailto:ssahu@marvell.com>; Kanaka Durga Kotamarthy
> > > <mailto:kkotamarthy@marvell.com>; Anoob Joseph
> > > <mailto:anoobj@marvell.com>; mailto:dev@dpdk.org
> > > <mailto:dev@dpdk.org>; Ayuj Verma <mailto:ayverma@marvell.com>
> > > Subject: [PATCH v1 0/2] declare crypto asym xform immutable
> > >
> > > Mark asym xform as immutable till lifetime of session. It will save
> > > session setup time for PMDs, which doesn't require any manipulation
> > > of xform data, by directly using these buffers.
> > >
> > > * Updated xform type in session init/configure
> > >   API as constant.
> > > * Updated doc with proper transform description.
> > > * Updated openssl PMD with above changes.
> > >
> > > Ayuj Verma (2):
> > >   lib/crypto: declare crypto asym xform immutable
> > >   crypto/openssl: mark asym xform constant
> > >
> > >  doc/guides/prog_guide/cryptodev_lib.rst      | 10 ++++++++++
> > >  drivers/crypto/openssl/rte_openssl_pmd_ops.c |  8 ++++----
> > >  lib/librte_cryptodev/rte_cryptodev.c         |  2 +-
> > >  lib/librte_cryptodev/rte_cryptodev.h         |  2 +-
> > >  lib/librte_cryptodev/rte_cryptodev_pmd.h     |  2 +-
> > >  5 files changed, 17 insertions(+), 7 deletions(-)
> > >
> > > --
> > > 1.8.3.1

Re: [dpdk-dev] [PATCH v1 0/2] declare crypto asym xform immutable

[Akhil Goyal]

> 
> Hi Akhil
> 
> >
> > Hi Anoob/Shally
> >
> > > >
> > > > Hi Ayuj,
> > > >
> > > > I believe there are couple of issues with this patch.
> > > >
> > > > Are these experimental APIs? I believe they were made stable this
> > > > release and I'm not sure if it is a right practice to edit an API
> > > > without deprecation notice after it is made stable. Especially now
> > > > that RC2 is done. @Akhil, what is your take on this?
> > > > [Shally] These are experimental still, hence no deprecation notice.
> > > > We checked about it with Fiona, Akhil before.
> > >
> > > [Anoob] In the patch, the edited APIs doesn't have experimental tag. I
> > > leave it to Akhil's judgement on this.
> > >
> > The API is experimental. Check the .map file.
> >
> > But these kind of patches cannot go in 1908 release, as we have already
> > tagged it for RC2.
> > We need to defer this change for 19.11 release.
> >
> [Shally] Well. Cant do anything if we have missed deadline here. Right now its up
> for feedback.
> Meanwhile if we get any agreement and window gets open for nearest release,
> then please consider it.

library changes cannot go beyond RC2 unless it is a fix for some issue.

> 
> > One more issue that I see in this patch. QAT also uses these APIs, so it shall
> > also be modified along with openssl.
> >
> [Shally] We assume QAT changes will be done by QAT maintainer, once they
> review and approve this change.
> 

Patch 1/2 will break the compilation for QAT and openssl.
All changes should be part of a single patch (both for QAT and openssl).
If somebody is modifying the common code, then it should not atleast break the compilation.

Check the following commit.
http://git.dpdk.org/next/dpdk-next-crypto/commit/?id=186b14d6850654eb84a8ae9ea29b736f0ba5e093


-Akhil

Re: [dpdk-dev] [PATCH v1 0/2] declare crypto asym xform immutable

[Shally Verma]

> -----Original Message-----
> From: Akhil Goyal <akhil.goyal@nxp.com>
> Sent: Thursday, July 25, 2019 5:13 PM
> To: Shally Verma <shallyv@marvell.com>; Anoob Joseph
> <anoobj@marvell.com>; Ayuj Verma <ayverma@marvell.com>
> Cc: arkadiuszx.kusztal@intel.com; Sunila Sahu <ssahu@marvell.com>; Kanaka
> Durga Kotamarthy <kkotamarthy@marvell.com>; dev@dpdk.org; Fiona
> Trahe <fiona.trahe@intel.com>
> Subject: RE: [PATCH v1 0/2] declare crypto asym xform immutable
> 
> 
> 
> >
> > Hi Akhil
> >
> > >
> > > Hi Anoob/Shally
> > >
> > > > >
> > > > > Hi Ayuj,
> > > > >
> > > > > I believe there are couple of issues with this patch.
> > > > >
> > > > > Are these experimental APIs? I believe they were made stable
> > > > > this release and I'm not sure if it is a right practice to edit
> > > > > an API without deprecation notice after it is made stable.
> > > > > Especially now that RC2 is done. @Akhil, what is your take on this?
> > > > > [Shally] These are experimental still, hence no deprecation notice.
> > > > > We checked about it with Fiona, Akhil before.
> > > >
> > > > [Anoob] In the patch, the edited APIs doesn't have experimental
> > > > tag. I leave it to Akhil's judgement on this.
> > > >
> > > The API is experimental. Check the .map file.
> > >
> > > But these kind of patches cannot go in 1908 release, as we have
> > > already tagged it for RC2.
> > > We need to defer this change for 19.11 release.
> > >
> > [Shally] Well. Cant do anything if we have missed deadline here. Right
> > now its up for feedback.
> > Meanwhile if we get any agreement and window gets open for nearest
> > release, then please consider it.
> 
> library changes cannot go beyond RC2 unless it is a fix for some issue.
> 
> >
> > > One more issue that I see in this patch. QAT also uses these APIs,
> > > so it shall also be modified along with openssl.
> > >
> > [Shally] We assume QAT changes will be done by QAT maintainer, once
> > they review and approve this change.
> >
> 
> Patch 1/2 will break the compilation for QAT and openssl.
> All changes should be part of a single patch (both for QAT and openssl).
> If somebody is modifying the common code, then it should not atleast break
> the compilation.
> 
> Check the following commit.
> http://git.dpdk.org/next/dpdk-next-
> crypto/commit/?id=186b14d6850654eb84a8ae9ea29b736f0ba5e093
> 
[Shally] Okay. Agree. We will look into it.
> 
> -Akhil