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 D7815A0C47; Wed, 14 Jul 2021 19:29:04 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 972294068A; Wed, 14 Jul 2021 19:29:04 +0200 (CEST) Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by mails.dpdk.org (Postfix) with ESMTP id 785904014D for ; Wed, 14 Jul 2021 19:29:03 +0200 (CEST) X-IronPort-AV: E=McAfee;i="6200,9189,10045"; a="197580439" X-IronPort-AV: E=Sophos;i="5.84,239,1620716400"; d="scan'208";a="197580439" Received: from orsmga004.jf.intel.com ([10.7.209.38]) by orsmga101.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 14 Jul 2021 10:29:01 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.84,239,1620716400"; d="scan'208";a="562509325" Received: from fmsmsx601.amr.corp.intel.com ([10.18.126.81]) by orsmga004.jf.intel.com with ESMTP; 14 Jul 2021 10:29:00 -0700 Received: from fmsmsx608.amr.corp.intel.com (10.18.126.88) by fmsmsx601.amr.corp.intel.com (10.18.126.81) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2242.10; Wed, 14 Jul 2021 10:29:00 -0700 Received: from fmsmsx612.amr.corp.intel.com (10.18.126.92) by fmsmsx608.amr.corp.intel.com (10.18.126.88) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2242.10; Wed, 14 Jul 2021 10:29:00 -0700 Received: from fmsedg602.ED.cps.intel.com (10.1.192.136) by fmsmsx612.amr.corp.intel.com (10.18.126.92) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2242.10 via Frontend Transport; Wed, 14 Jul 2021 10:29:00 -0700 Received: from NAM12-DM6-obe.outbound.protection.outlook.com (104.47.59.177) by edgegateway.intel.com (192.55.55.71) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2242.10; Wed, 14 Jul 2021 10:28:59 -0700 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=FAotyx5JeBkmyQpNcF5BB6R/rVnJbB3h0eKysMquNeSQ5fGQUYyZu/Bu8XBCxFLmSzE0hsS536kkyrKMMdM4/yt0h00LZbYRBvecfv4AkpCM+iYa9J1r/b9C3PApGX8Cc5juQAXJi+nRE6t5LiVerPGPqNZja698YCLBOk43KN4jv3O9E//PmD8VIXkAHhQ0J4La7kweAQ86ufiG35rFOe2CpxNSDf6vPRmamD4j8Iz6/+bXB7meGPTb83DCotX4TtehHdlfH+q+BNmhVFXlBgC2NCdB+Iffsz/TMedJRahzSx2y02YUDMl6ZEH5Jnz0yOZxz2Xxurdza3bLCGH/Aw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=IkgTLTAYQD4PjL0A/DYBB7veHiCNXZ21D8Ewf4ue0Sc=; b=oOKX8HqRPkJnjM6TWvlIWSaegRRBxnzZwsluTw2yHN8ajgyw/R456fZtNB0NxG+bLQsIhoDZ090WaHjJmB2GUn31yHEJd136g09dOzMBGEid1xZU/cSVCvaPDxs/xtqsg65runEPVEWeh9FBpBhz42gIDn6k3dF2JtLIdRwePQfiMyVtfHf/dlD2jlW9aEtbBvEgGIwyozOr3WqpHZgYTNSvKR6Q/XRjgwsOkrHPpWNH6UX/T5XaMvmJ0A4TA2+qZnd54v91ggOzDoe7GYwysnGComcL6T4joK3zplZwMriNVubNR8xuOR9ZQkrWiWRT4sna+aMwK086V0TZrRz04Q== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=intel.com; dmarc=pass action=none header.from=intel.com; dkim=pass header.d=intel.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=intel.onmicrosoft.com; s=selector2-intel-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=IkgTLTAYQD4PjL0A/DYBB7veHiCNXZ21D8Ewf4ue0Sc=; b=UzGux5B+hiHOlL92kn1opmS+ZJ764LIfsDxFwyY7zCVYw8TKqGXvAUsErgSNQ35d1xryAyboDuymPyZmtgZnrvDfNqg+xGHxRvJZzIk+YmIL7TpWiD0/fut22xDphkTYXyag2gYXWSTYg/4uwctB9kkYjqDgR8Y4GfWGYr1ijso= Received: from DM6PR11MB4491.namprd11.prod.outlook.com (2603:10b6:5:204::19) by DM6PR11MB4531.namprd11.prod.outlook.com (2603:10b6:5:2a5::19) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4331.22; Wed, 14 Jul 2021 17:28:58 +0000 Received: from DM6PR11MB4491.namprd11.prod.outlook.com ([fe80::7dc4:66b0:f76b:6d48]) by DM6PR11MB4491.namprd11.prod.outlook.com ([fe80::7dc4:66b0:f76b:6d48%7]) with mapi id 15.20.4308.027; Wed, 14 Jul 2021 17:28:58 +0000 From: "Ananyev, Konstantin" To: Nithin Dabilpuram CC: Akhil Goyal , "dev@dpdk.org" , "hemant.agrawal@nxp.com" , "thomas@monjalon.net" , "g.singh@nxp.com" , "Yigit, Ferruh" , "Zhang, Roy Fan" , "olivier.matz@6wind.com" , "jerinj@marvell.com" , "Doherty, Declan" , "Nicolau, Radu" , "jiawenwu@trustnetic.com" , "jianwang@trustnetic.com" Thread-Topic: [dpdk-dev] [PATCH 1/2] security: enforce semantics for Tx inline processing Thread-Index: AQHXaOPPEvFIqFhEn0C8QRoI7a/e16s12L8QgAAaAACAAAK4QIAABe0AgAAQV7CAAUGeAIAACA8AgAAdpACABMZzgIADc9iAgAEZGjCAAEfdUIAAH6CAgAE9COCAACvmAIAAQlJA Date: Wed, 14 Jul 2021 17:28:58 +0000 Message-ID: References: In-Reply-To: Accept-Language: en-GB, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-reaction: no-action dlp-version: 11.5.1.3 authentication-results: gmail.com; dkim=none (message not signed) header.d=none;gmail.com; dmarc=none action=none header.from=intel.com; x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: f1c67544-cdfe-42a4-c42b-08d946ecdd11 x-ms-traffictypediagnostic: DM6PR11MB4531: x-ms-exchange-transport-forked: True x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:9508; x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: jHUduHTzijAyNmMhTJTbBySr4icEkNa4ogesQBR+ZGCsm5X9a8OnX7NVj0+aUuiAxqVsPpb+7ghP8rXRyHk/TJbnjecCqLBoaiHtXHn9AAMDGEs3DSk+ANmMMEJDliXk1ciaWepfV6oPqhOVSvod8V6+tgaR+2qWnS661eREpCvOU0B7bGnamX/bHe94U4nOkjgiG0DTPL6l72+Xl6EkuDlGRzBQAV47jGkusxWh6DEogoAjHr1eJiE17LUsRDbAU58eAf5lpBfTuRReGFoE3TGabdTlSAB8byVFaX5UXbxXDqdQZjXHLnTNwC6tLoXTNjTGip4uoAVKJmAhdhEs6MYkoTq5LDYTvJjwK4BtW00mtTjwJbT6y0KnV5o8wK2OuhrArDVkoWKhM2GyjgVb84Qnk2BqE9G/oN0Veg0aWg/fLe8jme/H4nn6mUbbuBcJWUJsrHldHsh9ORnwABmBueBTpxcA3ZMeRtUQn7687vjHrAfyE1sqj1tidlYpvkxVu+kmvc1hp6RAXvLbP+eUh0AMMuS56TL68WsmQWDLfEwR7wZkTyHRCkrNAtHosanHz8ike2/xaor5F1HwLwOqGWqYF3/sj/mOUUwy4wKIg5HnXo6oKXYbIaNfLrSIaa8qbUI7OX3ZLy2a+A/6jsFxeULpblUufauKB+SGsLRMZqjpPtwMZfo1KQt8bYfwsmO6XonhAbJc9IxvtLA5++SH5Q== x-forefront-antispam-report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:DM6PR11MB4491.namprd11.prod.outlook.com; PTR:; CAT:NONE; SFS:(4636009)(136003)(39860400002)(346002)(396003)(376002)(366004)(7416002)(8676002)(55236004)(53546011)(6506007)(52536014)(76116006)(186003)(7696005)(66446008)(66946007)(66476007)(64756008)(26005)(9686003)(86362001)(54906003)(478600001)(55016002)(66556008)(83380400001)(316002)(5660300002)(71200400001)(6916009)(2906002)(4326008)(33656002)(30864003)(15650500001)(8936002)(38100700002)(122000001)(38070700004)(579004)(559001); DIR:OUT; SFP:1102; x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?AvZhm2EIS/g/GuiDk0dEw/LntQEX0IkqmzY2dkRxX28szGoyiFB4gu8oC1RA?= =?us-ascii?Q?VxXKlA5ovNz54D1wJqqAFCkWfVvJGlu4pWoXUuhAt7jlnipGBbfxkKw8H+Ti?= =?us-ascii?Q?7R4nmGfe5j8O7aBoEz8w058X2HIeL3AOP6UJSvlGG85AgA1ZG+lMQz9sNRQK?= =?us-ascii?Q?WCz0kTDOtSzGqO0RTLZMFrMlpkbzNGOwBCeCxY9KqazInS2DKsEORFLFH5FT?= =?us-ascii?Q?efoDsM57UcEppScTJ3koWww3a3dm1qC4Mo2me1UpSQTYSyBpQr8F+j5HIf3G?= =?us-ascii?Q?32/RDoUpUtcScBbmKEfmUgyf9JeR/+FnieqQ7C7+74ON8uU2wkP6yzaWNp3y?= =?us-ascii?Q?VnWltaNt5Yl0/1pkiqO5Gx9jfMm8f9buaYcQ/2sJi8AgzwU8zzGh2lF0qMda?= =?us-ascii?Q?8BWabH5WwnGAKAacndKgf2Bboek9BPy0ihhx+yoot5TjLgb7mOjqZ8becLYY?= =?us-ascii?Q?Ffd9CbHVFnEu7isx2N1T/9NcNsTrHnqJcfekfRqOqX1jp86YfcJ9YiVLvHG+?= =?us-ascii?Q?V1NiZhBYNBGBioqe6c08F14EMW4jLzRl93vFGSmmT0i2NsDD5HOuBuoRkUM3?= =?us-ascii?Q?jihy8ibSHglX4Te08JB5sc539szKpNNJsk4XTnvHWgq3PY7ANkpUHuCLeLpb?= =?us-ascii?Q?DB9IXGE4H/3DzBaDwgQKTcWl9E4XIJobDUshs77kA+B8znP9Ea517Nb/zw4S?= =?us-ascii?Q?ur+vWqd4hotFPcAUG9o7SEfj1fS/+zItOUnWOakNNh3eoaUOwybCWZQxb8r9?= =?us-ascii?Q?RSFGgPEvtKivVE6jAcvvV15vg9y+elxYjIjzNlMUJYW7dhE0Onglup4yNE+B?= =?us-ascii?Q?NnXVXoMyPWiPE2m3WXSVUc88vr9Zokq28qaMuVFgssBybvXTlIIejPkwVQH+?= =?us-ascii?Q?tyOpUc5S2HuoM7vgB6Jl+5MfGwOem3zcyxy1s0kBBUpTHh5Vsenfzg+7i09l?= =?us-ascii?Q?7FvCqfyngV+xF7TBV5zNVLE2bvdNmRcLBF823SU+TWhCaeLaqXHfp9lhNLA7?= =?us-ascii?Q?8svhVLjACMVnoSuPKUg7URB6VWBLXFrzlhiGMjcBcCUsp3vz8RSnmvCQpRS6?= =?us-ascii?Q?gb2sofTNIVUL839X8dX02W/fRqWBuEs7/3NshVySOFWr9w6ftE3Vwkdgu+JY?= =?us-ascii?Q?AIYfNVcYQx2AEDCTnUxDHKAo23fSLdYlOz8qAPwWLUZ3q5GjvLjO6pt1EI9n?= =?us-ascii?Q?v+44ehGbhaiKYG/+4S4BrF+lpjoIqrBFzu7/FRoBmU8SZkADXAvZ1qQ5YR2t?= =?us-ascii?Q?If4Vh5Ql79hchXLZv7jnaewNIpGUirPA/8rxeWnK6OOmKF3yNMbnqYwFFniU?= =?us-ascii?Q?sz3mXZzdgYytRGNQuJiXOe0C?= Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB4491.namprd11.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: f1c67544-cdfe-42a4-c42b-08d946ecdd11 X-MS-Exchange-CrossTenant-originalarrivaltime: 14 Jul 2021 17:28:58.4231 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: ynXzHkO09TCcttxG9H4dW/FzflinnZDU6LG/brloPpktjp0hWpCQYa29q69/JKYpAKw9fLnhOENz30gfo7IGSWdEo7lZ5eLOPzii1ISIIk0= X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR11MB4531 X-OriginatorOrg: intel.com Subject: Re: [dpdk-dev] [PATCH 1/2] security: enforce semantics for Tx inline processing 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 Sender: "dev" > -----Original Message----- > From: Nithin Dabilpuram > Sent: Wednesday, July 14, 2021 2:30 PM > To: Ananyev, Konstantin > Cc: Akhil Goyal ; dev@dpdk.org; hemant.agrawal@nxp.co= m; thomas@monjalon.net; g.singh@nxp.com; Yigit, Ferruh > ; Zhang, Roy Fan ; olivi= er.matz@6wind.com; jerinj@marvell.com; Doherty, Declan > ; Nicolau, Radu ; jiawe= nwu@trustnetic.com; jianwang@trustnetic.com > Subject: Re: [dpdk-dev] [PATCH 1/2] security: enforce semantics for Tx in= line processing >=20 > On Wed, Jul 14, 2021 at 11:09:08AM +0000, Ananyev, Konstantin wrote: > > > > > > > > > > Adding more rte_security and PMD maintainers into the loop. > > > > > > > > > > > > > > > > > > > > > For Tx inline processing, when RTE_SECURITY= _TX_OLOAD_NEED_MDATA is > > > > > > > > > > > > > > > > set, rte_security_set_pkt_metadata() needs = to be called for pkts > > > > > > > > > > > > > > > > to associate a Security session with a mbuf= before submitting > > > > > > > > > > > > > > > > to Ethdev Tx. This is apart from setting PK= T_TX_SEC_OFFLOAD in > > > > > > > > > > > > > > > > mbuf.ol_flags. rte_security_set_pkt_metadat= a() is also used to > > > > > > > > > > > > > > > > set some opaque metadata in mbuf for PMD's = use. > > > > > > > > > > > > > > > > This patch updates documentation that rte_s= ecurity_set_pkt_metadata() > > > > > > > > > > > > > > > > should be called only with mbuf containing = Layer 3 and above data. > > > > > > > > > > > > > > > > This behaviour is consistent with existing = PMD's such as ixgbe. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Tx, not all net PMD's/HW can parse packe= t and identify > > > > > > > > > > > > > > > > L2 header and L3 header locations on Tx. Th= is is inline with other > > > > > > > > > > > > > > > > Tx offloads requirements such as L3 checksu= m, L4 checksum offload, > > > > > > > > > > > > > > > > etc, where mbuf.l2_len, mbuf.l3_len etc, ne= eds to be set for > > > > > > > > > > > > > > > > HW to be able to generate checksum. Since I= nline IPSec is also > > > > > > > > > > > > > > > > such a Tx offload, some PMD's at least need= mbuf.l2_len to be > > > > > > > > > > > > > > > > valid to find L3 header and perform Outboun= d IPSec processing. > > > > > > > > > > > > > > > > Hence, this patch updates documentation to = enforce setting > > > > > > > > > > > > > > > > mbuf.l2_len while setting PKT_TX_SEC_OFFLOA= D in mbuf.ol_flags > > > > > > > > > > > > > > > > for Inline IPSec Crypto / Protocol offload = processing to > > > > > > > > > > > > > > > > work on Tx. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Nithin Dabilpuram > > > > > > > > > > > > > > > > Reviewed-by: Akhil Goyal > > > > > > > > > > > > > > > > --- > > > > > > > > > > > > > > > > doc/guides/nics/features.rst | 2= ++ > > > > > > > > > > > > > > > > doc/guides/prog_guide/rte_security.rst | 6= +++++- > > > > > > > > > > > > > > > > lib/mbuf/rte_mbuf_core.h | 2= ++ > > > > > > > > > > > > > > > > 3 files changed, 9 insertions(+), 1 deleti= on(-) > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > diff --git a/doc/guides/nics/features.rst b= /doc/guides/nics/features.rst > > > > > > > > > > > > > > > > index 403c2b03a..414baf14f 100644 > > > > > > > > > > > > > > > > --- a/doc/guides/nics/features.rst > > > > > > > > > > > > > > > > +++ b/doc/guides/nics/features.rst > > > > > > > > > > > > > > > > @@ -430,6 +430,7 @@ of protocol operations.= See Security library and PMD documentation for more deta > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > * **[uses] rte_eth_rxconf,rte_eth_rx= mode**: ``offloads:DEV_RX_OFFLOAD_SECURITY``, > > > > > > > > > > > > > > > > * **[uses] rte_eth_txconf,rte_eth_tx= mode**: ``offloads:DEV_TX_OFFLOAD_SECURITY``. > > > > > > > > > > > > > > > > +* **[uses] mbuf**: ``mbuf.l2_len``. > > > > > > > > > > > > > > > > * **[implements] rte_security_ops**: ``ses= sion_create``, ``session_update``, > > > > > > > > > > > > > > > > ``session_stats_get``, ``session_destroy= ``, ``set_pkt_metadata``, ``capabilities_get``. > > > > > > > > > > > > > > > > * **[provides] rte_eth_dev_info**: ``rx_of= fload_capa,rx_queue_offload_capa:DEV_RX_OFFLOAD_SECURITY``, > > > > > > > > > > > > > > > > @@ -451,6 +452,7 @@ protocol operations. Se= e security library and PMD documentation for more details > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > * **[uses] rte_eth_rxconf,rte_eth_rx= mode**: ``offloads:DEV_RX_OFFLOAD_SECURITY``, > > > > > > > > > > > > > > > > * **[uses] rte_eth_txconf,rte_eth_tx= mode**: ``offloads:DEV_TX_OFFLOAD_SECURITY``. > > > > > > > > > > > > > > > > +* **[uses] mbuf**: ``mbuf.l2_len``. > > > > > > > > > > > > > > > > * **[implements] rte_security_ops**: ``ses= sion_create``, ``session_update``, > > > > > > > > > > > > > > > > ``session_stats_get``, ``session_destroy= ``, ``set_pkt_metadata``, ``get_userdata``, > > > > > > > > > > > > > > > > ``capabilities_get``. > > > > > > > > > > > > > > > > diff --git a/doc/guides/prog_guide/rte_secu= rity.rst b/doc/guides/prog_guide/rte_security.rst > > > > > > > > > > > > > > > > index f72bc8a78..7b68c698d 100644 > > > > > > > > > > > > > > > > --- a/doc/guides/prog_guide/rte_security.rs= t > > > > > > > > > > > > > > > > +++ b/doc/guides/prog_guide/rte_security.rs= t > > > > > > > > > > > > > > > > @@ -560,7 +560,11 @@ created by the applica= tion is attached to the security session by the API > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > For Inline Crypto and Inline protocol offl= oad, device specific defined metadata is > > > > > > > > > > > > > > > > updated in the mbuf using ``rte_security_s= et_pkt_metadata()`` if > > > > > > > > > > > > > > > > -``DEV_TX_OFFLOAD_SEC_NEED_MDATA`` is set. > > > > > > > > > > > > > > > > +``RTE_SECURITY_TX_OLOAD_NEED_MDATA`` is se= t. ``rte_security_set_pkt_metadata()`` > > > > > > > > > > > > > > > > +should be called on mbuf only with Layer 3= and above data present and > > > > > > > > > > > > > > > > +``mbuf.data_off`` should be pointing to La= yer 3 Header. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hmm... not sure why mbuf.data_off should poin= t to L3 hdr. > > > > > > > > > > > > > > > Who will add L2 hdr to the packet in that cas= e? > > > > > > > > > > > > > > > Or did you mean ``mbuf.data_off + mbuf.l2_len= `` here? > > > > > > > > > > > > > > > > > > > > > > > > > > > > That is the semantics I was trying to define. I= think below are the sequence of > > > > > > > > > > > > > > operations to be done for ipsec processing, > > > > > > > > > > > > > > > > > > > > > > > > > > > > 1. receive_pkt() > > > > > > > > > > > > > > 2. strip_l2_hdr() > > > > > > > > > > > > > > 3. Do policy lookup () > > > > > > > > > > > > > > 4. Call rte_security_set_pkt_metadata() if pkt = needs to be encrypted with a > > > > > > > > > > > > > > particular SA. Now pkt only has L3 and above da= ta. > > > > > > > > > > > > > > 5. Do route_lookup() > > > > > > > > > > > > > > 6. add_l2hdr() which might be different from st= ripped l2hdr. > > > > > > > > > > > > > > 7. Send packet out. > > > > > > > > > > > > > > > > > > > > > > > > > > > > The above sequence is what I believe the curren= t poll mode worker thread in > > > > > > > > > > > > > > ipsec-secgw is following. > > > > > > > > > > > > > > > > > > > > > > > > > > That's just a sample app, it doesn't mean it has = to be the only possible way. > > > > > > > > > > > > > > > > > > > > > > > > > > > While in event mode, step 2 and step 6 are miss= ing. > > > > > > > > > > > > > > > > > > > > > > > > > > I think this L2 hdr manipulation is totally optio= nal. > > > > > > > > > > > > > If your rte_security_set_pkt_metadata() implement= ation really needs to know L3 hdr offset (not sure why?), > > > > > > > > > > > > Since rte_security_set_pkt_metadata() is PMD specif= ic function ptr call, we are currently doing some pre-processing > > > > > > > > > > > > here before submitting packet to inline IPSec via r= te_eth_tx_burst(). This saves us cycles later in rte_eth_tx_burst(). > > > > > > > > > > > > If we cannot know for sure, the pkt content at the = time of rte_security_set_pkt_metadata() call, then I think > > > > > > > > > > > > having a PMD specific callback is not much of use e= xcept for saving SA priv data to rte_mbuf. > > > > > > > > > > > > > > > > > > > > > > > > > then I suppose we can add a requirement that l2_l= en has to be set properly before calling > > > rte_security_set_pkt_metadata(). > > > > > > > > > > > > > > > > > > > > > > > > This is also fine with us. > > > > > > > > > > > > > > > > > > > > > > Ok, so to make sure we are on the same page, you prop= ose: > > > > > > > > > > > 1. before calling rte_security_set_pkt_metadata() mbu= f.l2_len should be properly set. > > > > > > > > > > > 2. after rte_security_set_pkt_metadata() and before r= te_eth_tx_burst() packet contents > > > > > > > > > > > at [mbuf.l2_len, mbuf.pkt_len) can't be modified? > > > > > > > > > > Yes. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Is that correct understanding? > > > > > > > > > > > If yes, I wonder how 2) will correlate with rte_eth_t= x_prepare() concept? > > > > > > > > > > > > > > > > > > > > Since our PMD doesn't have a prepare function, I missed= that but, since > > > > > > > > > > rte_security_set_pkt_metadata() is only used for Inline= Crypto/Protocol via > > > > > > > > > > a rte_eth_dev, and both rte_security_set_pkt_metadata()= and rte_eth_tx_prepare() > > > > > > > > > > are callbacks from same PMD, do you see any issue ? > > > > > > > > > > > > > > > > > > > > The restriction is from user side, data is not supposed= to be modified unless > > > > > > > > > > rte_security_set_pkt_metadata() is called again. > > > > > > > > > > > > > > > > > > Yep, I do have a concern here. > > > > > > > > > Right now it is perfectly valid to do something like that= : > > > > > > > > > rte_security_set_pkt_metadata(..., mb, ...); > > > > > > > > > /* can modify contents of the packet */ > > > > > > > > > rte_eth_tx_prepare(..., &mb, 1); > > > > > > > > > rte_eth_tx_burst(..., &mb, 1); > > > > > > > > > > > > > > > > > > With the new restrictions you are proposing it wouldn't b= e allowed any more. > > > > > > > > You can still modify L2 header and IPSEC is only concerned = about L3 and above. > > > > > > > > > > > > > > > > I think insisting that rte_security_set_pkt_metadata() be c= alled after all L3 > > > > > > > > and above header modifications is no a problem. I guess exi= sting ixgbe/txgbe > > > > > > > > PMD which are the ones only implementing the call back are = already expecting the > > > > > > > > same ? > > > > > > > > > > > > > > AFAIK, no there are no such requirements for ixgbe or txgbe. > > > > > > > All that ixgbe callback does - store session related data ins= ide mbuf. > > > > > > > It's only expectation to have ESP trailer at the proper place= (after ICV): > > > > > > > > > > > > This implies rte_security_set_pkt_metadata() cannot be called w= hen mbuf does't > > > > > > have ESP trailer updated or when mbuf->pkt_len =3D 0 > > > > > > > > > > > > > > > > > > > > union ixgbe_crypto_tx_desc_md *mdata =3D (union ixgbe_crypto_= tx_desc_md *) > > > > > > > rte_security_dynfield(m); > > > > > > > mdata->enc =3D 1; > > > > > > > mdata->sa_idx =3D ic_session->sa_index; > > > > > > > mdata->pad_len =3D ixgbe_crypto_compute_pad_len(m); > > > > > > > > > > > > > > Then this data will be used by tx_burst() function. > > > > > > So it implies that after above rte_security_set_pkt_metadata() = call, and before tx_burst(), > > > > > > mbuf data / packet len cannot be modified right as if modified,= then tx_burst() > > > > > > will be using incorrect pad len ? > > > > > > > > > > No, pkt_len can be modified. > > > > > Though ESP trailer pad_len can't. > > > > > > > > > > > > > > > > > This patch is also trying to add similar restriction on when > > > > > > rte_security_set_pkt_metadata() should be called and what canno= t be done after > > > > > > calling rte_security_set_pkt_metadata(). > > > > > > > > > > No, I don't think it is really the same. > > > > > Also, IMO, inside ixgbe set_pkt_metadata() implementaion we proba= bly shouldn't silently imply > > > > > that ESP packet is already formed and trailer contains valid data= . > > > > > In fact, I think this pad_len calculation can be moved to actual = TX function. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > If your question is can't we do the preprocessing in rt= e_eth_tx_prepare() for > > > > > > > > > > security, > > > > > > > > > > > > > > > > > > Yes, that was my thought. > > > > > > > > > > > > > > > > > > > my only argument was that since there is already a hit = in > > > > > > > > > > rte_security_set_pkt_metadata() to PMD specific callbac= k and > > > > > > > > > > struct rte_security_session is passed as an argument to= it, it is more benefitial to > > > > > > > > > > do security related pre-processing there. > > > > > > > > > > > > > > > > > > Yes, it would be extra callback call that way. > > > > > > > > > Though tx_prepare() accepts burst of packets, so the over= head > > > > > > > > > of function call will be spread around the whole burst, a= nd I presume > > > > > > > > > shouldn't be too high. > > > > > > > > > > > > > > > > > > > Also rte_eth_tx_prepare() if implemented will be called= for both security and > > > > > > > > > > non-security pkts. > > > > > > > > > > > > > > > > > > Yes, but tx_prepare() can distinguish (by ol_flags and/or= other field contents) which > > > > > > > > > modifications are required for the packet. > > > > > > > > > > > > > > > > But the major issues I see are > > > > > > > > > > > > > > > > 1. tx_prepare() doesn't take rte_security_session as argume= nt though ol_flags has security flag. > > > > > > > > In our case, we need to know the security session detail= s to do things. > > > > > > > > > > > > > > I suppose you can store pointer to session (or so) inside mbu= f in rte_security_dynfield, no? > > > > > > > > > > > > We can do. But having to call PMD specific function call via rt= e_security_set_pkt_metadata() > > > > > > just for storing session pointer in rte_security_dynfield consu= mes unnecessary > > > > > > cycles per pkt. > > > > > > > > > > In fact there are two function calls: one for rte_security_set_pk= t_metadata(), > > > > > second for instance->ops->set_pkt_metadata() callback. > > > > > Which off-course way too expensive for such simple operation. > > > > > Actually same thought for rte_security_get_userdata(). > > > > > Both of these functions belong to data-path and ideally have to b= e as fast as possible. > > > > > Probably 21.11 is a right timeframe for that. > > > > > > > > > > > > > > > > > > > > 2. AFAIU tx_prepare() is not mandatory as per spec and even= by default disabled under compile time > > > > > > > > macro RTE_ETHDEV_TX_PREPARE_NOOP. > > > > > > > > 3. Even if we do tx_prepare(), rte_security_set_pkt_mdata()= is mandatory to associate > > > > > > > > struct rte_security_session to a pkt as unlike ol_flags,= there is no direct space to do the same. > > > > > > > > > > > > > > Didn't get you here, obviously we do have rte_security_dynfie= ld inside mbuf, > > > > > > > specially for that - to store secuiryt related data inside th= e mbuf. > > > > > > > Yes your PMD has to request it at initialization time, but I = suppose it is not a big deal. > > > > > > > > > > > > > > > So I think instead of enforcing yet another callback tx_pre= pare() for inline security > > > > > > > > processing, it can be done via security specific set_pkt_me= tadata(). > > > > > > > > > > > > > > But what you proposing introduces new limitations and might e= xisting functionality. > > > > > > > BTW, if you don't like to use tx_prepare() - why doing these = calculations inside tx_burst() > > > > > > > itself is not an option? > > > > > > > > > > > > We can do things in tx_burst() but if we are doing it there, th= en we want to avoid having callback for > > > > > > rte_security_set_pkt_metadata(). > > > > > > > > > > > > Are you fine if we can update the spec that "When DEV_TX_OFFLOA= D_SEC_NEED_MDATA is not > > > > > > set, then, user needs to update struct rte_security_session's s= ess_private_data in a in > > > > > > rte_security_dynfield like below ? > > > > > > > > > > > > > > > > > > > > > > > > static inline void > > > > > > inline_outb_mbuf_prepare(const struct rte_ipsec_session *ss, > > > > > > struct rte_mbuf *mb[], uint16_t num) > > > > > > { > > > > > > uint32_t i, ol_flags; > > > > > > > > > > > > ol_flags =3D ss->security.ol_flags & RTE_SECURITY_TX_OL= OAD_NEED_MDATA; > > > > > > for (i =3D 0; i !=3D num; i++) { > > > > > > > > > > > > mb[i]->ol_flags |=3D PKT_TX_SEC_OFFLOAD; > > > > > > > > > > > > if (ol_flags !=3D 0) > > > > > > rte_security_set_pkt_metadata(ss->secur= ity.ctx, > > > > > > ss->security.ses, mb[i], NULL); > > > > > > else > > > > > > *rte_security_dynfield(mb[i]) =3D > > > > > > (uint64_t)ss->security.ses->ses= s_private_data; > > > > > > > > > > > > > > > > > > If the above can be done, then in our PMD, we will not have a c= allback for > > > > > > set_pkt_metadata() and DEV_TX_OFFLOAD_SEC_NEED_MDATA will also = be not set > > > > > > in capabilities. > > > > > > > > > > That's an interesting idea, but what you propose is the change in= current rte_security API behaviour. > > > > > So all existing apps that use this API will have to be changed. > > > > > We'd better avoid such changes unless there is really good reason= for that. > > > > > So, I'd suggest to tweak your idea a bit: > > > > > > > > > > 1) change rte_security_set_pkt_metadata(): > > > > > if ops->set_pkt_metadata !=3D NULL, then call it (existing behavi= our) > > > > > otherwise just: rte_security_dynfield(m) =3D sess->session_privat= e_data; > > > > > (fast-path) > > > > > > > > > > 2) consider to make rte_security_set_pkt_metadata() inline functi= on. > > > > > We probably can have some special flag inside struct rte_security= _ctx, > > > > > or even store inside ctx a pointer to set_pkt_metadata() itself. > > > > > > > > After another thoughts some new flags might be better. > > > > Then later, if we'll realize that set_pkt_metadata() and get_userad= ata() > > > > are not really used by PMDs, it might be easier to deprecate these = callbacks. > > > > > > Thanks, I agree with your thoughts. I'll submit a V2 with above chang= e, new flags and > > > set_pkt_metadata() and get_userdata() function pointers moved to rte_= security_ctx for > > > review so that it can be targeted for 21.11. > > > > > > Even with flags moving set_pkt_metadata() and get_userdata() function= pointers is still needed > > > as we need to make rte_security_set_pkt_metadata() API inline while s= truct rte_security_ops is not > > > exposed to user. I think this is fine as it is inline with how fast p= ath function pointers > > > of rte_ethdev and rte_cryptodev are currently placed. > > > > My thought was we can get away with just flags only. > > Something like that: > > rte_security.h: > > > > ... > > > > enum { > > RTE_SEC_CTX_F_FAST_SET_MDATA =3D 0x1, > > RTE_SEC_CTX_F_FAST_GET_UDATA =3D 0x2, > > }; > > > > struct rte_security_ctx { > > void *device; > > /**< Crypto/ethernet device attached */ > > const struct rte_security_ops *ops; > > /**< Pointer to security ops for the device */ > > uint16_t sess_cnt; > > /**< Number of sessions attached to this context */ > > uint32_t flags; > > }; > > > > extern int > > __rte_security_set_pkt_metadata(struct rte_security_ctx *instance, > > struct rte_security_session *sess, > > struct rte_mbuf *m, void *params); > > > > static inline int > > rte_security_set_pkt_metadata(struct rte_security_ctx *instance, > > struct rte_security_session *sess, > > struct rte_mbuf *m, void *params) > > { > > /* fast-path */ > > if (instance->flags & RTE_SEC_CTX_F_FAST_SET_MDATA) { > > *rte_security_dynfield(m) =3D (rte_security_dynfield_t)(s= ession->sess_priv_data); > > return 0; > > /* slow path */ > > } else > > return __rte_security_set_pkt_metadata (instance->device, s= ess, m, params); > > } > > > > rte_security.c: > > > > ... > > /* existing one, just renamed */ > > int > > __rte_security_set_pkt_metadata(struct rte_security_ctx *instance, > > struct rte_security_session *sess, > > struct rte_mbuf *m, void *params) > > { > > #ifdef RTE_DEBUG > > RTE_PTR_OR_ERR_RET(sess, -EINVAL); > > RTE_PTR_OR_ERR_RET(instance, -EINVAL); > > RTE_PTR_OR_ERR_RET(instance->ops, -EINVAL); > > #endif > > RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->set_pkt_metadata, -ENOT= SUP); > > return instance->ops->set_pkt_metadata(instance->device, > > sess, m, params); > > } > > > > > > I think both ways are possible (flags vs actual func pointers) and both= have > > some pluses and minuses. > > I suppose the main choice here what do we think should be the future of > > set_pkt_metadata() and rte_security_get_userdata(). > > If we think that they will be useful for some future PMDs and we want t= o keep them, > > then probably storing actual func pointers inside ctx is a better appro= ach. > > If not, then flags seems like a better one, as in that case we can even= tually > > deprecate and remove these callbacks. > > From what I see right now, custom callbacks seems excessive, > > and rte_security_dynfield is enough. > > But might be there are some future plans that would require them? >=20 > Above method is also fine. Moving fn pointers to rte_security_ctx can be > done later if other PMD's need it. Yes, agree. >=20 > Atleast our HW PMD's doesn't plan to use set_pkt_metada()/get_user_data() > fn pointers in future if above is implemented. >=20 > > > > > > > > > > > > > > > > > > > As a brief code snippet: > > > > > > > > > > struct rte_security_ctx { > > > > > void *device; > > > > > /**< Crypto/ethernet device attached */ > > > > > const struct rte_security_ops *ops; > > > > > /**< Pointer to security ops for the device */ > > > > > uint16_t sess_cnt; > > > > > /**< Number of sessions attached to this context */ > > > > > + int (*set_pkt_mdata)(void *, struct rte_security_session *,= struct rte_mbuf *, void *); > > > > > }; > > > > > > > > > > static inline int > > > > > rte_security_set_pkt_metadata(struct rte_security_ctx *instance, > > > > > struct rte_security_session *sess, > > > > > struct rte_mbuf *m, void *params) > > > > > { > > > > > /* fast-path */ > > > > > if (instance->set_pkt_mdata =3D=3D NULL) { > > > > > *rte_security_dynfield(m) =3D (rte_security_dynfield= _t)(session->sess_priv_data); > > > > > return 0; > > > > > /* slow path */ > > > > > } else > > > > > return instance->set_pkt_mdata(instance->device, sess,= m, params); > > > > > } > > > > > > > > > > That probably would be an ABI breakage (new fileld in rte_securit= y_ctx) and would require > > > > > some trivial changes for all existing PMDs that use RTE_SECURITY_= TX_OFLOAD_NEED_MDATA > > > > > (ctx_create()), but hopefully will benefit everyone. > > > > > > > > > > > > > > > > > > > > > > > > > > I'm fine to > > > > > > > > introduce a burst call for the same(I was thinking to propo= se it in future) to > > > > > > > > compensate for the overhead. > > > > > > > > > > > > > > > > If rte_security_set_pkt_metadata() was not a PMD specific f= unction ptr call and > > > > > > > > rte_mbuf had space for struct rte_security_session pointer, > > > > > > > > > > > > > > But it does, see above. > > > > > > > In fact it even more flexible - because it is driver specific= , you are not limited to one 64-bit field. > > > > > > > If your PMD requires more data to be associated with mbuf > > > > > > > - you can request it via mbuf_dynfield and store there whatev= er is needed. > > > > > > > > > > > > > > > then then I guess it would have been better to do the way y= ou proposed. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > This patch is trying to enforce semantics as ab= ove so that > > > > > > > > > > > > > > rte_security_set_pkt_metadata() can predict wha= t comes in the pkt when he is > > > > > > > > > > > > > > called. > > > > > > > > > > > > > > > > > > > > > > > > > > > > I also think above sequence is what Linux kerne= l stack or other stacks follow. > > > > > > > > > > > > > > Does it makes sense ? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Once called, > > > > > > > > > > > > > > > > +Layer 3 and above data cannot be modified = or moved around unless > > > > > > > > > > > > > > > > +``rte_security_set_pkt_metadata()`` is cal= led again. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > For inline protocol offloaded ingress traf= fic, the application can register a > > > > > > > > > > > > > > > > pointer, ``userdata`` , in the security se= ssion. When the packet is received, > > > > > > > > > > > > > > > > diff --git a/lib/mbuf/rte_mbuf_core.h b/lib= /mbuf/rte_mbuf_core.h > > > > > > > > > > > > > > > > index bb38d7f58..9d8e3ddc8 100644 > > > > > > > > > > > > > > > > --- a/lib/mbuf/rte_mbuf_core.h > > > > > > > > > > > > > > > > +++ b/lib/mbuf/rte_mbuf_core.h > > > > > > > > > > > > > > > > @@ -228,6 +228,8 @@ extern "C" { > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > /** > > > > > > > > > > > > > > > > * Request security offload processing on = the TX packet. > > > > > > > > > > > > > > > > + * To use Tx security offload, the user ne= eds to fill l2_len in mbuf > > > > > > > > > > > > > > > > + * indicating L2 header size and where L3 = header starts. > > > > > > > > > > > > > > > > */ > > > > > > > > > > > > > > > > #define PKT_TX_SEC_OFFLOAD (1ULL << 43) > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > > > > > > > > > 2.25.1 > > > > > > > > > > > > > > >