From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id AE03E4326B; Thu, 2 Nov 2023 08:21:06 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 803474029E; Thu, 2 Nov 2023 08:21:06 +0100 (CET) Received: from mail-pj1-f51.google.com (mail-pj1-f51.google.com [209.85.216.51]) by mails.dpdk.org (Postfix) with ESMTP id A065940282 for ; Thu, 2 Nov 2023 08:21:05 +0100 (CET) Received: by mail-pj1-f51.google.com with SMTP id 98e67ed59e1d1-28028f92709so542767a91.0 for ; Thu, 02 Nov 2023 00:21:05 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=meaningfulname-net.20230601.gappssmtp.com; s=20230601; t=1698909665; x=1699514465; darn=dpdk.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=pQYymkQqI4zzh9Xe8QGwPJ9ikFkuHB3gtTj196Of9K4=; b=Ko1qQLPx8coAUgVsOGgaNcqxAKxt6+SsPG/F3jVIwT5tIF/ODFmwpbCF8HadBggnT9 7ly6F6XMNYmxvhWI8sC9d2WNfG8HXFrCXHebdqOWLjFwYGYC4wmeJkM8dSyWLSf/7gRq IdFONQeF6RI4gAJofcvwy8i3d8X60AVEcF6+rjSVkXMGmQaXYvvkW/WXwPLlqaq/plS7 nzeapt33npLmn/yw++laNtbzxmR2AzGe/RZs2nMapcH5Z1mNxUAROPKLht/jw69ROg+C bMO8ID5GdyORkGrZ1eIuru0iP0xBTFdXC7+aDWX8JPDW1g/ZkmK+AME72G7cRGQhjxDH RUrw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1698909665; x=1699514465; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=pQYymkQqI4zzh9Xe8QGwPJ9ikFkuHB3gtTj196Of9K4=; b=R2Kb6QoGo7f8KcN8UDobRWsFyCg2+FORXlz1nZUkxNPEOoQLdUrM79iFHWvPSj3kT2 rHkUS24Udf4hI6X2MU3zYTzYKqI6eIKvpyhz7V404h+0tM94wMzh80KGx617TqkwSzSm +PgcYE6o3mr5AFIX5Bgepy7CJfAz05MeVn9keI0Jq1W1zWPJ+HcjzgpdIBPd2QxZceMG DAGqmDYyAm3umyi2qcOOgT14LMPpV2TWmkw4G+1QFb8/+bZDiVRFIFw+y1OODC5f+lS+ bx9qk1aP7e7pqZ24TzVscAmSL0yM35ydZFui/uNiO+qCHSYUw8PBLyg2DYVci+AvzvIh rzXw== X-Gm-Message-State: AOJu0YxFoTP9R3wERdhfwMYWo7cxssv2ZZLIQICccMCnKs2ZGyjQBpdb 1ux2wt8yJev6YtCI2QuGU3GhI6pZ9NipCYPZcbdOJg== X-Google-Smtp-Source: AGHT+IEW9EZeHmtA16HzfdYXCEDjqGVI9JKuXXF5Pj1/0Z96QkgnQLyy7G6fnqKHXJULZSZlwYODJd0gBOR82Tnl0qM= X-Received: by 2002:a17:90b:1c0f:b0:280:7cd2:429 with SMTP id oc15-20020a17090b1c0f00b002807cd20429mr7126597pjb.18.1698909664742; Thu, 02 Nov 2023 00:21:04 -0700 (PDT) MIME-Version: 1.0 References: <20230925201128.861-1-gazmarsh@meaningfulname.net> <64c6794e-69ef-4469-a596-32cd9d70d0bd@yandex.ru> In-Reply-To: From: Garry Marshall Date: Thu, 2 Nov 2023 07:20:53 +0000 Message-ID: Subject: Re: [PATCH] ipsec: use sym_session_opaque_data for RTE_SECURITY_TYPE_CPU_CRYPTO To: Konstantin Ananyev Cc: Konstantin Ananyev , "dev@dpdk.org" , "vladimir.medvedkin@intel.com" Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Ah - thanks Konstantin - I will go back and review. Regards, Garry. On Tue, Oct 31, 2023 at 5:53=E2=80=AFPM Konstantin Ananyev 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 =3D=3D RTE_SECURITY_ACTION_TYPE_NONE || > > ips->type =3D=3D RTE_SECURITY_ACTION_TYPE_CPU_CRYPTO) { > > /* Session has not been created */ > > if (ips->crypto.ses =3D=3D NULL) > > return 0; > > > > ret =3D rte_cryptodev_sym_session_free(ips->crypto.dev_id, > > ips->crypto.ses); > > } else { > > /* Session has not been created */ > > if (ips->security.ctx =3D=3D NULL || ips->security.ses =3D=3D N= ULL) > > return 0; > > > > ret =3D rte_security_session_destroy(ips->security.ctx, > > ips->security.ses); > > } > > > > And similarly - if we look at the session_check function in lib/ipsec/s= es.c: > > > > if (ss->type =3D=3D RTE_SECURITY_ACTION_TYPE_NONE || > > ss->type =3D=3D RTE_SECURITY_ACTION_TYPE_CPU_CRYPTO) { > > if (ss->crypto.ses =3D=3D NULL) > > return -EINVAL; > > } else { > > if (ss->security.ses =3D=3D NULL) > > return -EINVAL; > > if ((ss->type =3D=3D RTE_SECURITY_ACTION_TYPE_INLINE_CRYPTO || > > ss->type =3D=3D > > RTE_SECURITY_ACTION_TYPE_INLINE_PROTOCOL) && > > ss->security.ctx =3D=3D 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=E2=80=99t call rte_= security_session_opaque_data_set() > for TYPE_CPU_CRYPTO. > So shouldn't it be like that: > > ss->pkt_func =3D fp; > > if (ss->type =3D=3D RTE_SECURITY_ACTION_TYPE_NONE) > rte_cryptodev_sym_session_opaque_data_set(ss->crypto.ses, > (uintptr_t)ss); > - else > + else if (ss->type !=3D RTE_SECURITY_ACTION_TYPE_CPU_CRYPTO) > rte_security_session_opaque_data_set(ss->security.ses, (u= intptr_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_ips= ec_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 value= s. > * 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=E2=80=AFAM Konstantin Ananyev > > 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 > > > > --- > > > > 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_sessio= n *ss) > > > > > > > > ss->pkt_func =3D fp; > > > > > > > > - if (ss->type =3D=3D RTE_SECURITY_ACTION_TYPE_NONE) > > > > + if (ss->type =3D=3D RTE_SECURITY_ACTION_TYPE_NONE || > > > > + ss->type =3D=3D RTE_SECURITY_ACTION_TYPE_CPU_CRYPTO) > > > > rte_cryptodev_sym_session_opaque_data_set(ss->crypto.= ses, > > > > (uintptr_t)ss); > > > > else > > > > -- > > > > 2.39.2 >