* [PATCH] ipsec: use sym_session_opaque_data for RTE_SECURITY_TYPE_CPU_CRYPTO @ 2023-09-25 20:11 Garry Marshall 2023-10-30 7:22 ` [EXT] " Akhil Goyal 2023-10-31 1:08 ` Konstantin Ananyev 0 siblings, 2 replies; 6+ messages in thread From: Garry Marshall @ 2023-09-25 20:11 UTC (permalink / raw) To: dev; +Cc: Garry Marshall, Konstantin Ananyev, Vladimir Medvedkin ipsec related processing in dpdk makes use of the crypto.ses opaque data pointer. This patch updates rte_ipsec_session_prepare to set ss->crypto.ses in the RTE_SECURITY_TYPE_CPU_CRYPTO case. Signed-off-by: Garry Marshall <gazmarsh@meaningfulname.net> --- lib/ipsec/ses.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/ipsec/ses.c b/lib/ipsec/ses.c index d9ab1e6d2b..29eb5ff6ca 100644 --- a/lib/ipsec/ses.c +++ b/lib/ipsec/ses.c @@ -44,7 +44,8 @@ rte_ipsec_session_prepare(struct rte_ipsec_session *ss) ss->pkt_func = fp; - if (ss->type == RTE_SECURITY_ACTION_TYPE_NONE) + if (ss->type == RTE_SECURITY_ACTION_TYPE_NONE || + ss->type == RTE_SECURITY_ACTION_TYPE_CPU_CRYPTO) rte_cryptodev_sym_session_opaque_data_set(ss->crypto.ses, (uintptr_t)ss); else -- 2.39.2 ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [EXT] [PATCH] ipsec: use sym_session_opaque_data for RTE_SECURITY_TYPE_CPU_CRYPTO 2023-09-25 20:11 [PATCH] ipsec: use sym_session_opaque_data for RTE_SECURITY_TYPE_CPU_CRYPTO Garry Marshall @ 2023-10-30 7:22 ` Akhil Goyal 2023-10-31 1:08 ` Konstantin Ananyev 1 sibling, 0 replies; 6+ messages in thread From: Akhil Goyal @ 2023-10-30 7:22 UTC (permalink / raw) To: dev, Brian Dooley, ciara.power, Kai Ji, Konstantin Ananyev, Vladimir Medvedkin Cc: Garry Marshall > ipsec related processing in dpdk makes use of the crypto.ses opaque > data pointer. This patch updates rte_ipsec_session_prepare to set > ss->crypto.ses in the RTE_SECURITY_TYPE_CPU_CRYPTO case. > > Signed-off-by: Garry Marshall <gazmarsh@meaningfulname.net> > --- Konstantin/ Kai, Is the below change ok for CPU crypto usecase? Please review and give ack. Regards, Akhil > lib/ipsec/ses.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/lib/ipsec/ses.c b/lib/ipsec/ses.c > index d9ab1e6d2b..29eb5ff6ca 100644 > --- a/lib/ipsec/ses.c > +++ b/lib/ipsec/ses.c > @@ -44,7 +44,8 @@ rte_ipsec_session_prepare(struct rte_ipsec_session *ss) > > ss->pkt_func = fp; > > - if (ss->type == RTE_SECURITY_ACTION_TYPE_NONE) > + if (ss->type == RTE_SECURITY_ACTION_TYPE_NONE || > + ss->type == RTE_SECURITY_ACTION_TYPE_CPU_CRYPTO) > rte_cryptodev_sym_session_opaque_data_set(ss->crypto.ses, > (uintptr_t)ss); > else > -- > 2.39.2 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ipsec: use sym_session_opaque_data for RTE_SECURITY_TYPE_CPU_CRYPTO 2023-09-25 20:11 [PATCH] ipsec: use sym_session_opaque_data for RTE_SECURITY_TYPE_CPU_CRYPTO Garry Marshall 2023-10-30 7:22 ` [EXT] " Akhil Goyal @ 2023-10-31 1:08 ` Konstantin Ananyev 2023-10-31 9:36 ` Garry Marshall 1 sibling, 1 reply; 6+ messages in thread From: Konstantin Ananyev @ 2023-10-31 1:08 UTC (permalink / raw) To: gazmarsh; +Cc: dev, konstantin.v.ananyev, vladimir.medvedkin > > > ipsec related processing in dpdk makes use of the crypto.ses opaque > data pointer. This patch updates rte_ipsec_session_prepare to set > ss->crypto.ses in the RTE_SECURITY_TYPE_CPU_CRYPTO case. Hmm.. not sure why we need to do that for CPU_CRYPTO? As I remember CPU_CRYPTO is synchronous operation and before calling rte_ipsec_pkt_cpu_prepare() should already know ipsec session these packets belong to. Can you probably explain the logic behind this patch a bit more? Konstantin > > Signed-off-by: Garry Marshall <gazmarsh@meaningfulname.net> > --- > lib/ipsec/ses.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/lib/ipsec/ses.c b/lib/ipsec/ses.c > index d9ab1e6d2b..29eb5ff6ca 100644 > --- a/lib/ipsec/ses.c > +++ b/lib/ipsec/ses.c > @@ -44,7 +44,8 @@ rte_ipsec_session_prepare(struct rte_ipsec_session *ss) > > ss->pkt_func = fp; > > - if (ss->type == RTE_SECURITY_ACTION_TYPE_NONE) > + if (ss->type == RTE_SECURITY_ACTION_TYPE_NONE || > + ss->type == RTE_SECURITY_ACTION_TYPE_CPU_CRYPTO) > rte_cryptodev_sym_session_opaque_data_set(ss->crypto.ses, > (uintptr_t)ss); > else > -- > 2.39.2 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ipsec: use sym_session_opaque_data for RTE_SECURITY_TYPE_CPU_CRYPTO 2023-10-31 1:08 ` Konstantin Ananyev @ 2023-10-31 9:36 ` Garry Marshall 2023-10-31 17:53 ` Konstantin Ananyev 0 siblings, 1 reply; 6+ messages in thread From: Garry Marshall @ 2023-10-31 9:36 UTC (permalink / raw) To: Konstantin Ananyev; +Cc: dev, vladimir.medvedkin Hi Konstantin, Akhil, The patch is based on an issue I encountered when using the CPU_CRYPTO support - I was having problems where the ipsec session lookup was failing / was inconsistent. Examining the code in DPDK and looking for the use of RTE_SECURITY_ACTION_TYPE_CPU_CRYPTO I could see a reasonably consistent pattern where if TYPE_NONE or TYPE_CPU_CRYPTO was set - then the code was making use of ss->crypto.ses instead of ss->security.ses. For example - see examples/ipsec-secgw.c where the one_session_free function has the following code: if (ips->type == RTE_SECURITY_ACTION_TYPE_NONE || ips->type == RTE_SECURITY_ACTION_TYPE_CPU_CRYPTO) { /* Session has not been created */ if (ips->crypto.ses == NULL) return 0; ret = rte_cryptodev_sym_session_free(ips->crypto.dev_id, ips->crypto.ses); } else { /* Session has not been created */ if (ips->security.ctx == NULL || ips->security.ses == NULL) return 0; ret = rte_security_session_destroy(ips->security.ctx, ips->security.ses); } And similarly - if we look at the session_check function in lib/ipsec/ses.c: if (ss->type == RTE_SECURITY_ACTION_TYPE_NONE || ss->type == RTE_SECURITY_ACTION_TYPE_CPU_CRYPTO) { if (ss->crypto.ses == NULL) return -EINVAL; } else { if (ss->security.ses == NULL) return -EINVAL; if ((ss->type == RTE_SECURITY_ACTION_TYPE_INLINE_CRYPTO || ss->type == RTE_SECURITY_ACTION_TYPE_INLINE_PROTOCOL) && ss->security.ctx == NULL) return -EINVAL; } Without the patch in rte_ipsec_session_prepare - for the RTE_SECURITY_ACTION_TYPE_CPU_CRYPTO type, then ss->crypto.ses will not be set. Regards, Garry. On Tue, Oct 31, 2023 at 1:09 AM Konstantin Ananyev <konstantin.v.ananyev@yandex.ru> wrote: > > > > > > > ipsec related processing in dpdk makes use of the crypto.ses opaque > > data pointer. This patch updates rte_ipsec_session_prepare to set > > ss->crypto.ses in the RTE_SECURITY_TYPE_CPU_CRYPTO case. > > > Hmm.. not sure why we need to do that for CPU_CRYPTO? > As I remember CPU_CRYPTO is synchronous operation and before calling > rte_ipsec_pkt_cpu_prepare() should already know ipsec session these > packets belong to. > Can you probably explain the logic behind this patch a bit more? > Konstantin > > > > > Signed-off-by: Garry Marshall <gazmarsh@meaningfulname.net> > > --- > > lib/ipsec/ses.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/lib/ipsec/ses.c b/lib/ipsec/ses.c > > index d9ab1e6d2b..29eb5ff6ca 100644 > > --- a/lib/ipsec/ses.c > > +++ b/lib/ipsec/ses.c > > @@ -44,7 +44,8 @@ rte_ipsec_session_prepare(struct rte_ipsec_session *ss) > > > > ss->pkt_func = fp; > > > > - if (ss->type == RTE_SECURITY_ACTION_TYPE_NONE) > > + if (ss->type == RTE_SECURITY_ACTION_TYPE_NONE || > > + ss->type == RTE_SECURITY_ACTION_TYPE_CPU_CRYPTO) > > rte_cryptodev_sym_session_opaque_data_set(ss->crypto.ses, > > (uintptr_t)ss); > > else > > -- > > 2.39.2 ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] ipsec: use sym_session_opaque_data for RTE_SECURITY_TYPE_CPU_CRYPTO 2023-10-31 9:36 ` Garry Marshall @ 2023-10-31 17:53 ` Konstantin Ananyev 2023-11-02 7:20 ` Garry Marshall 0 siblings, 1 reply; 6+ messages in thread From: Konstantin Ananyev @ 2023-10-31 17:53 UTC (permalink / raw) To: Garry Marshall, Konstantin Ananyev; +Cc: dev, vladimir.medvedkin Hi Garry, > Hi Konstantin, Akhil, > > The patch is based on an issue I encountered when using the CPU_CRYPTO > support - I was having problems where the ipsec session lookup was > failing / was inconsistent. > > Examining the code in DPDK and looking for the use of > RTE_SECURITY_ACTION_TYPE_CPU_CRYPTO I could see a reasonably > consistent pattern where if TYPE_NONE or TYPE_CPU_CRYPTO was set - > then the code was making use of ss->crypto.ses instead of > ss->security.ses. > > For example - see examples/ipsec-secgw.c where the one_session_free > function has the following code: > > if (ips->type == RTE_SECURITY_ACTION_TYPE_NONE || > ips->type == RTE_SECURITY_ACTION_TYPE_CPU_CRYPTO) { > /* Session has not been created */ > if (ips->crypto.ses == NULL) > return 0; > > ret = rte_cryptodev_sym_session_free(ips->crypto.dev_id, > ips->crypto.ses); > } else { > /* Session has not been created */ > if (ips->security.ctx == NULL || ips->security.ses == NULL) > return 0; > > ret = rte_security_session_destroy(ips->security.ctx, > ips->security.ses); > } > > And similarly - if we look at the session_check function in lib/ipsec/ses.c: > > if (ss->type == RTE_SECURITY_ACTION_TYPE_NONE || > ss->type == RTE_SECURITY_ACTION_TYPE_CPU_CRYPTO) { > if (ss->crypto.ses == NULL) > return -EINVAL; > } else { > if (ss->security.ses == NULL) > return -EINVAL; > if ((ss->type == RTE_SECURITY_ACTION_TYPE_INLINE_CRYPTO || > ss->type == > RTE_SECURITY_ACTION_TYPE_INLINE_PROTOCOL) && > ss->security.ctx == NULL) > return -EINVAL; > } Thanks for explanation. Yes, I agree that TYPE_NONE and TYPE_CPU_CRYPTO both use crypto session to keep/propagate crypto related pamaters. What is not clear to me why for and TYPE_CPU_CRYPTO we need to store pointer to rte_ipsec_session as opaque user data for crypto session. As I remember, for lookaside crypto we need to do that to extract related rte_ipsec_session pointer from crypto_op, after lookaside crypto device finished the processing and sending sym-ops back to user. But for CPU_CRYPTO it is not necessary, as all processing is synchronous and user already has a pointer for related rte_ipsec_session. We probably still can, but what is the benefit, who will use it? Actually looking at the rte_ipsec_session_prepare() once again, you probably right - it is a bug here, as we shouldn’t call rte_security_session_opaque_data_set() for TYPE_CPU_CRYPTO. So shouldn't it be like that: ss->pkt_func = fp; if (ss->type == RTE_SECURITY_ACTION_TYPE_NONE) rte_cryptodev_sym_session_opaque_data_set(ss->crypto.ses, (uintptr_t)ss); - else + else if (ss->type != RTE_SECURITY_ACTION_TYPE_CPU_CRYPTO) rte_security_session_opaque_data_set(ss->security.ses, (uintptr_t)ss); > Without the patch in rte_ipsec_session_prepare - for the > RTE_SECURITY_ACTION_TYPE_CPU_CRYPTO type, then ss->crypto.ses will not > be set. Hmm... not clear why? AFAIK, ss->crypto.ses supposed to be set by user *before* calling rte_ipsec_session_prepare(). From lib/ipsec/rte_ipsec.h: /** * Checks that inside given rte_ipsec_session crypto/security fields * are filled correctly and setups function pointers based on these values. * Expects that all fields except IPsec processing function pointers * (*pkt_func*) will be filled correctly by caller. * @param ss * Pointer to the *rte_ipsec_session* object * @return * - Zero if operation completed successfully. * - -EINVAL if the parameters are invalid. */ int rte_ipsec_session_prepare(struct rte_ipsec_session *ss); > > Regards, > > Garry. > > > On Tue, Oct 31, 2023 at 1:09 AM Konstantin Ananyev > <konstantin.v.ananyev@yandex.ru> wrote: > > > > > > > > > > > ipsec related processing in dpdk makes use of the crypto.ses opaque > > > data pointer. This patch updates rte_ipsec_session_prepare to set > > > ss->crypto.ses in the RTE_SECURITY_TYPE_CPU_CRYPTO case. > > > > Hmm.. not sure why we need to do that for CPU_CRYPTO? > > As I remember CPU_CRYPTO is synchronous operation and before calling > > rte_ipsec_pkt_cpu_prepare() should already know ipsec session these > > packets belong to. > > Can you probably explain the logic behind this patch a bit more? > > Konstantin > > > > > > > > Signed-off-by: Garry Marshall <gazmarsh@meaningfulname.net> > > > --- > > > lib/ipsec/ses.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/lib/ipsec/ses.c b/lib/ipsec/ses.c > > > index d9ab1e6d2b..29eb5ff6ca 100644 > > > --- a/lib/ipsec/ses.c > > > +++ b/lib/ipsec/ses.c > > > @@ -44,7 +44,8 @@ rte_ipsec_session_prepare(struct rte_ipsec_session *ss) > > > > > > ss->pkt_func = fp; > > > > > > - if (ss->type == RTE_SECURITY_ACTION_TYPE_NONE) > > > + if (ss->type == RTE_SECURITY_ACTION_TYPE_NONE || > > > + ss->type == RTE_SECURITY_ACTION_TYPE_CPU_CRYPTO) > > > rte_cryptodev_sym_session_opaque_data_set(ss->crypto.ses, > > > (uintptr_t)ss); > > > else > > > -- > > > 2.39.2 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ipsec: use sym_session_opaque_data for RTE_SECURITY_TYPE_CPU_CRYPTO 2023-10-31 17:53 ` Konstantin Ananyev @ 2023-11-02 7:20 ` Garry Marshall 0 siblings, 0 replies; 6+ messages in thread From: Garry Marshall @ 2023-11-02 7:20 UTC (permalink / raw) To: Konstantin Ananyev; +Cc: Konstantin Ananyev, dev, vladimir.medvedkin Ah - thanks Konstantin - I will go back and review. Regards, Garry. On Tue, Oct 31, 2023 at 5:53 PM Konstantin Ananyev <konstantin.ananyev@huawei.com> wrote: > > > Hi Garry, > > > Hi Konstantin, Akhil, > > > > The patch is based on an issue I encountered when using the CPU_CRYPTO > > support - I was having problems where the ipsec session lookup was > > failing / was inconsistent. > > > > Examining the code in DPDK and looking for the use of > > RTE_SECURITY_ACTION_TYPE_CPU_CRYPTO I could see a reasonably > > consistent pattern where if TYPE_NONE or TYPE_CPU_CRYPTO was set - > > then the code was making use of ss->crypto.ses instead of > > ss->security.ses. > > > > For example - see examples/ipsec-secgw.c where the one_session_free > > function has the following code: > > > > if (ips->type == RTE_SECURITY_ACTION_TYPE_NONE || > > ips->type == RTE_SECURITY_ACTION_TYPE_CPU_CRYPTO) { > > /* Session has not been created */ > > if (ips->crypto.ses == NULL) > > return 0; > > > > ret = rte_cryptodev_sym_session_free(ips->crypto.dev_id, > > ips->crypto.ses); > > } else { > > /* Session has not been created */ > > if (ips->security.ctx == NULL || ips->security.ses == NULL) > > return 0; > > > > ret = rte_security_session_destroy(ips->security.ctx, > > ips->security.ses); > > } > > > > And similarly - if we look at the session_check function in lib/ipsec/ses.c: > > > > if (ss->type == RTE_SECURITY_ACTION_TYPE_NONE || > > ss->type == RTE_SECURITY_ACTION_TYPE_CPU_CRYPTO) { > > if (ss->crypto.ses == NULL) > > return -EINVAL; > > } else { > > if (ss->security.ses == NULL) > > return -EINVAL; > > if ((ss->type == RTE_SECURITY_ACTION_TYPE_INLINE_CRYPTO || > > ss->type == > > RTE_SECURITY_ACTION_TYPE_INLINE_PROTOCOL) && > > ss->security.ctx == NULL) > > return -EINVAL; > > } > > Thanks for explanation. > Yes, I agree that TYPE_NONE and TYPE_CPU_CRYPTO both use crypto session > to keep/propagate crypto related pamaters. > What is not clear to me why for and TYPE_CPU_CRYPTO we need to store > pointer to rte_ipsec_session as opaque user data for crypto session. > As I remember, for lookaside crypto we need to do that to extract > related rte_ipsec_session pointer from crypto_op, after lookaside crypto device > finished the processing and sending sym-ops back to user. > But for CPU_CRYPTO it is not necessary, as all processing is synchronous and > user already has a pointer for related rte_ipsec_session. > We probably still can, but what is the benefit, who will use it? > > Actually looking at the rte_ipsec_session_prepare() once again, > you probably right - it is a bug here, as we shouldn’t call rte_security_session_opaque_data_set() > for TYPE_CPU_CRYPTO. > So shouldn't it be like that: > > ss->pkt_func = fp; > > if (ss->type == RTE_SECURITY_ACTION_TYPE_NONE) > rte_cryptodev_sym_session_opaque_data_set(ss->crypto.ses, > (uintptr_t)ss); > - else > + else if (ss->type != RTE_SECURITY_ACTION_TYPE_CPU_CRYPTO) > rte_security_session_opaque_data_set(ss->security.ses, (uintptr_t)ss); > > > Without the patch in rte_ipsec_session_prepare - for the > > RTE_SECURITY_ACTION_TYPE_CPU_CRYPTO type, then ss->crypto.ses will not > > be set. > > Hmm... not clear why? > AFAIK, ss->crypto.ses supposed to be set by user *before* calling rte_ipsec_session_prepare(). > From lib/ipsec/rte_ipsec.h: > /** > * Checks that inside given rte_ipsec_session crypto/security fields > * are filled correctly and setups function pointers based on these values. > * Expects that all fields except IPsec processing function pointers > * (*pkt_func*) will be filled correctly by caller. > * @param ss > * Pointer to the *rte_ipsec_session* object > * @return > * - Zero if operation completed successfully. > * - -EINVAL if the parameters are invalid. > */ > int > rte_ipsec_session_prepare(struct rte_ipsec_session *ss); > > > > > Regards, > > > > Garry. > > > > > > On Tue, Oct 31, 2023 at 1:09 AM Konstantin Ananyev > > <konstantin.v.ananyev@yandex.ru> wrote: > > > > > > > > > > > > > > > ipsec related processing in dpdk makes use of the crypto.ses opaque > > > > data pointer. This patch updates rte_ipsec_session_prepare to set > > > > ss->crypto.ses in the RTE_SECURITY_TYPE_CPU_CRYPTO case. > > > > > > Hmm.. not sure why we need to do that for CPU_CRYPTO? > > > As I remember CPU_CRYPTO is synchronous operation and before calling > > > rte_ipsec_pkt_cpu_prepare() should already know ipsec session these > > > packets belong to. > > > Can you probably explain the logic behind this patch a bit more? > > > Konstantin > > > > > > > > > > > Signed-off-by: Garry Marshall <gazmarsh@meaningfulname.net> > > > > --- > > > > lib/ipsec/ses.c | 3 ++- > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/lib/ipsec/ses.c b/lib/ipsec/ses.c > > > > index d9ab1e6d2b..29eb5ff6ca 100644 > > > > --- a/lib/ipsec/ses.c > > > > +++ b/lib/ipsec/ses.c > > > > @@ -44,7 +44,8 @@ rte_ipsec_session_prepare(struct rte_ipsec_session *ss) > > > > > > > > ss->pkt_func = fp; > > > > > > > > - if (ss->type == RTE_SECURITY_ACTION_TYPE_NONE) > > > > + if (ss->type == RTE_SECURITY_ACTION_TYPE_NONE || > > > > + ss->type == RTE_SECURITY_ACTION_TYPE_CPU_CRYPTO) > > > > rte_cryptodev_sym_session_opaque_data_set(ss->crypto.ses, > > > > (uintptr_t)ss); > > > > else > > > > -- > > > > 2.39.2 > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-11-02 7:21 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-09-25 20:11 [PATCH] ipsec: use sym_session_opaque_data for RTE_SECURITY_TYPE_CPU_CRYPTO Garry Marshall 2023-10-30 7:22 ` [EXT] " Akhil Goyal 2023-10-31 1:08 ` Konstantin Ananyev 2023-10-31 9:36 ` Garry Marshall 2023-10-31 17:53 ` Konstantin Ananyev 2023-11-02 7:20 ` Garry Marshall
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).