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 664D6A0C4F; Tue, 13 Jul 2021 16:08:30 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 4EE8941277; Tue, 13 Jul 2021 16:08:30 +0200 (CEST) Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by mails.dpdk.org (Postfix) with ESMTP id C85F44126E for ; Tue, 13 Jul 2021 16:08:28 +0200 (CEST) X-IronPort-AV: E=McAfee;i="6200,9189,10043"; a="207149359" X-IronPort-AV: E=Sophos;i="5.84,236,1620716400"; d="scan'208";a="207149359" Received: from orsmga007.jf.intel.com ([10.7.209.58]) by fmsmga102.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 13 Jul 2021 07:08:25 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.84,236,1620716400"; d="scan'208";a="451767514" Received: from fmsmsx602.amr.corp.intel.com ([10.18.126.82]) by orsmga007.jf.intel.com with ESMTP; 13 Jul 2021 07:08:24 -0700 Received: from fmsmsx611.amr.corp.intel.com (10.18.126.91) by fmsmsx602.amr.corp.intel.com (10.18.126.82) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2242.10; Tue, 13 Jul 2021 07:08:24 -0700 Received: from fmsmsx603.amr.corp.intel.com (10.18.126.83) by fmsmsx611.amr.corp.intel.com (10.18.126.91) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2242.10; Tue, 13 Jul 2021 07:08:24 -0700 Received: from FMSEDG603.ED.cps.intel.com (10.1.192.133) by fmsmsx603.amr.corp.intel.com (10.18.126.83) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2242.10 via Frontend Transport; Tue, 13 Jul 2021 07:08:24 -0700 Received: from NAM12-DM6-obe.outbound.protection.outlook.com (104.47.59.170) by edgegateway.intel.com (192.55.55.68) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2242.10; Tue, 13 Jul 2021 07:08:22 -0700 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=dX2MvWuyaj8KS2M8LvEusqbBUYK5peNtW/mYg61ChDBpFWvTIerzTzhLP4aKyNhZDaFHAGyPGp+VEvkDZ44PmK7kod6/alLXF8sfOLhMwjpyuLtqLa9Xt57g8T2Mga1/Ho3jLNouP8Lu8npGLyPLxnIK9TUiG92Tmem8ni1pkYSrxUbAY/AmrtfD0DFc60/ln3aWEQnmzkDTGfPgCdcTleCYNrwKZtlz14G95Rz3vKVy0OudUyZ0ZudOw//B7qs5DrNK3AjpQ3JugJtf2XPx7K9/eGQYZFnOfif98T5sUwW7yMZs+/03N+3bJL7LaURcg1ETwHHCIakNddVTJy3XtQ== 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=H7WJxNGtoDwFWlNqC+Z96x6JsvDK4Yo596tpNzyKdTg=; b=DOCNeafqPaImQmiqoZRhIGo3ScmQe5OOe2VTL5sfnk6l569tAoz7YEk2hoQQE9x8eowBz7pDWOdUunK6EYGTrS6BrETWFYSCQ8r8iSeS6CGgdjGXMmekhZqnqYNzV0jrAaqm/tJJK4o0/n72zGpLAFQQpuP2DsNu63gW5IUxQJzQH/xi1+LNG9O5dawmvEzxEBsnO8hYeffaGn8Tn6GTMM6rHUXK7FQZSgtvMoJGHt7TCu8kTrtBH8e7iohQNE268qD+i6ZUDs/JfqVWsN5sAQt/9PVBoWILckcbBudCnoDMLOZJu5VKe1XB2cnoAZgA1rJFqIXmWafd6lufODNiZw== 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=H7WJxNGtoDwFWlNqC+Z96x6JsvDK4Yo596tpNzyKdTg=; b=d4s02mbaM/dkHZyX1J+HvdgZrwcFIt/UdfsERuavrfRx45JZTfrwE8OENRIzizgWIXd9MXtA5EVdDlMqBsgPErU2vaXrVjrOCaObYeVFxypz9aKMnhpPOR1+YawF8SM6URwamqkoBIzjSZMjEI6tQPFYlTZJfIc2DqBKeCtiXgA= Received: from DM6PR11MB4491.namprd11.prod.outlook.com (2603:10b6:5:204::19) by DM5PR11MB1292.namprd11.prod.outlook.com (2603:10b6:3:7::15) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4308.20; Tue, 13 Jul 2021 14:08:19 +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; Tue, 13 Jul 2021 14:08:19 +0000 From: "Ananyev, Konstantin" To: "Ananyev, Konstantin" , 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/e16s12L8QgAAaAACAAAK4QIAABe0AgAAQV7CAAUGeAIAACA8AgAAdpACABMZzgIADc9iAgAEZGjCAAEfdUA== Date: Tue, 13 Jul 2021 14:08:18 +0000 Message-ID: References: <20210624102848.3878788-1-gakhil@marvell.com> 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: intel.com; dkim=none (message not signed) header.d=none;intel.com; dmarc=none action=none header.from=intel.com; x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: f3ee50b1-1634-4274-8d26-08d94607aa96 x-ms-traffictypediagnostic: DM5PR11MB1292: x-ld-processed: 46c98d88-e344-4ed4-8496-4ed7712e255d,ExtAddr x-ms-exchange-transport-forked: True x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:10000; x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: LY6ssIUqf0VnZghfAE4jokJqFIZD9BoVaE7Shv3tIQ50ANmC+STefMvq4Um1xv2Nmlmrhu0LsFSE0pACBZwa8uHbB+d7TstMzrggn38hLSM0yYtblE4hzUy1iGS2LyCO64b0mX23rRg37LsIHDAqYUKbU5ZCkAGr9bv/YTOvgsdo1EnsodqqZIN88h816QLFLbxAiOkc8CpEdDVmksnNPzR+UPEaEzakczNubR3gBmVYTLKrXqZrTVBpQsyfA67WQLzMQMeCMOeOH6TV52oUuRcam2wMZkn8F2r8IrRR4vZbALXwPPOcQUAsRMGzLwPAVJfxk95k0ZUAwZQthPRSWeU5hgLlx4DzNMiEtm+8+9ILIGhxZo2FlMy7x+XfOdz13gFvcb2qa1q7cBWN1jrFT9vF8IlVHHRz9g2GE0fZLc8vvTje9ysbGP9Zoqmz787Rjdz5DBYd8i7qb77TNo8mdbNG3WjL003KvEWc6s1BSmRPwP6bQ8hu+jESBy70t2SHuAadCeoRYZ20bZejMTzsAIoa+znTjKNBM3upquI+80HWqJwQwEBLwheD0GV6r3Gk7TeNCaQnXsai1kCwsE/EC7qrnsizK7tTr128HqmK0q86YgT9GA81ke188POj+kpb9I+LsGiYZ56I+tQmmToNiA== 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)(396003)(376002)(39860400002)(366004)(346002)(136003)(55236004)(2940100002)(55016002)(316002)(38100700002)(66556008)(122000001)(15650500001)(478600001)(7696005)(110136005)(9686003)(6506007)(54906003)(33656002)(30864003)(8936002)(64756008)(8676002)(66476007)(26005)(66446008)(86362001)(4326008)(66946007)(7416002)(83380400001)(186003)(52536014)(5660300002)(71200400001)(2906002)(76116006); DIR:OUT; SFP:1102; x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?0wKF37B0TDpaE4rUXHYPHba4d5ycry52pdKqgD+NBfRuts8a/TH9WZj4SM3b?= =?us-ascii?Q?HJglCLO4XfsEenRKEbouGu2GwZ2ZIayWISoD1+7MK8KvHrHm6zr5LwputkF/?= =?us-ascii?Q?X78yOqimarx5zKNdrbokVduCjhXQGczLqJ8QOXW9GoFl3OkXyHRSStp7CSt1?= =?us-ascii?Q?p/umtAUt9H7qfExabDu601xF5BXa29Jlpx600xX5ypPrfeNH28EkAyCHOTI9?= =?us-ascii?Q?Mvm6xKWetZJQCQX3RzMCmhP/JxWYXTgLz4UuB/M38ZEIF0Y02rGMdDnjITqw?= =?us-ascii?Q?kkMP/45X4KlxJgv7UNUAwEm0i7X73yCFJ6go1NeXiszd1g6Nwg238B2Kcu04?= =?us-ascii?Q?306KZ7/x1SDrgytAiUhuzp932mGvuArgUlmL03pb1m5o95yWvS13Eb8fuaYg?= =?us-ascii?Q?IOce0dlA1nU0S3kF23HOURhr8zQyKOgBTQ1KNuGveqxQjXNcBo9MZ3tOYjr9?= =?us-ascii?Q?+/XscAHBkIzQeTZ6n9qFF10gUioxmgxxteqYJffrzGFU4uilOL08B6VtYdX1?= =?us-ascii?Q?XsK5ltBwbDSx4AdA7GEK0xUZDK53AcJAbiczrgiJMLx+ng2PoxXv+cHO7vzu?= =?us-ascii?Q?pz/4XEpwGygbXODkD8RCzuEJWzTBDKsdtvz1KmodGBd7OkGXcL03h5fjBjfi?= =?us-ascii?Q?bcNPCysll+P6r95RrC41WEqK7nfKCm1HnMxHYnqHhXLhCMxnboOhnA9Yptmd?= =?us-ascii?Q?wN/8wFW7jFvEmm4JkAl6crg/9d1T3UP6Esth+JZtCTHmy8dsB0heN/wK+15F?= =?us-ascii?Q?CvJZRa3BY+nVKW0sSIolAOZUb4umKMZ81M7AwGDYp2cpsHdfORS0ACTbNhbj?= =?us-ascii?Q?Hw2dDNTGOX6dZe5Z4XVCBpZb7foG4WcF8uGR3hhnk6Y5KD/Q0pEmJ5JzXG6I?= =?us-ascii?Q?lfocaAixEj732jEJp1todCFN+oNFO7cDWFWxSWzvkln79A/KnkyyZ4KOfDnG?= =?us-ascii?Q?gE/bipY62uHlITLA+M+ZPHMjJPnCSFnr0HX3Nb/o4dyR2jrC+Jl8znM/kyaa?= =?us-ascii?Q?pZPmvV5mgQp/eP0TRiyCKz+1zlo5Lvx48jgjMerxQQYZ+xX573RDUz4x2IGf?= =?us-ascii?Q?O5/FFvV0hYXx/2EKI4TKDz6gtEQbGxNTUmdGr4ds34GUJ9sJyHP9bqoupaZL?= =?us-ascii?Q?EJi/WKQcrdXLbBc9CwzhiaHfzGCg2LHJwjGj7WmVvKv0+Nq0bMIgnbQxZ0bI?= =?us-ascii?Q?pv07BX+qNd0oWr+V2dRGxDpOKzMsTAlHxTRc98CT2T7YMZukIfkbQi1wHmam?= =?us-ascii?Q?2NSkPq+9LpIAaUhebzKBVL0J3O92fcBLpKydpeQjHN/0jpQn3O5gkUjjFQKv?= =?us-ascii?Q?/o0LAfO4VMPv122d5OmsNTwd?= 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: f3ee50b1-1634-4274-8d26-08d94607aa96 X-MS-Exchange-CrossTenant-originalarrivaltime: 13 Jul 2021 14:08:18.8325 (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: rXX0a15MqIiigmghOiqKa4EizJH8YIpEH6geGm/lDhkbXmJGkHSwNLomWdL4txC6KuRWkjKqtVFrsc5qDjlBcf4mFo361dNugwd7bkEikhY= X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM5PR11MB1292 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" >=20 > Adding more rte_security and PMD maintainers into the loop. >=20 > > > > > > > > > > > > For Tx inline processing, when RTE_SECURITY_TX_OLOA= D_NEED_MDATA is > > > > > > > > > > > > set, rte_security_set_pkt_metadata() needs to be ca= lled for pkts > > > > > > > > > > > > to associate a Security session with a mbuf before = submitting > > > > > > > > > > > > to Ethdev Tx. This is apart from setting PKT_TX_SEC= _OFFLOAD in > > > > > > > > > > > > mbuf.ol_flags. rte_security_set_pkt_metadata() is a= lso used to > > > > > > > > > > > > set some opaque metadata in mbuf for PMD's use. > > > > > > > > > > > > This patch updates documentation that rte_security_= set_pkt_metadata() > > > > > > > > > > > > should be called only with mbuf containing Layer 3 = and above data. > > > > > > > > > > > > This behaviour is consistent with existing PMD's su= ch as ixgbe. > > > > > > > > > > > > > > > > > > > > > > > > On Tx, not all net PMD's/HW can parse packet and id= entify > > > > > > > > > > > > L2 header and L3 header locations on Tx. This is in= line with other > > > > > > > > > > > > Tx offloads requirements such as L3 checksum, L4 ch= ecksum offload, > > > > > > > > > > > > etc, where mbuf.l2_len, mbuf.l3_len etc, needs to b= e set for > > > > > > > > > > > > HW to be able to generate checksum. Since Inline IP= Sec is also > > > > > > > > > > > > such a Tx offload, some PMD's at least need mbuf.l2= _len to be > > > > > > > > > > > > valid to find L3 header and perform Outbound IPSec = processing. > > > > > > > > > > > > Hence, this patch updates documentation to enforce = setting > > > > > > > > > > > > mbuf.l2_len while setting PKT_TX_SEC_OFFLOAD in mbu= f.ol_flags > > > > > > > > > > > > for Inline IPSec Crypto / Protocol offload processi= ng 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 deletion(-) > > > > > > > > > > > > > > > > > > > > > > > > diff --git a/doc/guides/nics/features.rst b/doc/gui= des/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 Sec= urity library and PMD documentation for more deta > > > > > > > > > > > > > > > > > > > > > > > > * **[uses] rte_eth_rxconf,rte_eth_rxmode**: = ``offloads:DEV_RX_OFFLOAD_SECURITY``, > > > > > > > > > > > > * **[uses] rte_eth_txconf,rte_eth_txmode**: = ``offloads:DEV_TX_OFFLOAD_SECURITY``. > > > > > > > > > > > > +* **[uses] mbuf**: ``mbuf.l2_len``. > > > > > > > > > > > > * **[implements] rte_security_ops**: ``session_cre= ate``, ``session_update``, > > > > > > > > > > > > ``session_stats_get``, ``session_destroy``, ``se= t_pkt_metadata``, ``capabilities_get``. > > > > > > > > > > > > * **[provides] rte_eth_dev_info**: ``rx_offload_ca= pa,rx_queue_offload_capa:DEV_RX_OFFLOAD_SECURITY``, > > > > > > > > > > > > @@ -451,6 +452,7 @@ protocol operations. See securi= ty library and PMD documentation for more details > > > > > > > > > > > > > > > > > > > > > > > > * **[uses] rte_eth_rxconf,rte_eth_rxmode**: = ``offloads:DEV_RX_OFFLOAD_SECURITY``, > > > > > > > > > > > > * **[uses] rte_eth_txconf,rte_eth_txmode**: = ``offloads:DEV_TX_OFFLOAD_SECURITY``. > > > > > > > > > > > > +* **[uses] mbuf**: ``mbuf.l2_len``. > > > > > > > > > > > > * **[implements] rte_security_ops**: ``session_cre= ate``, ``session_update``, > > > > > > > > > > > > ``session_stats_get``, ``session_destroy``, ``se= t_pkt_metadata``, ``get_userdata``, > > > > > > > > > > > > ``capabilities_get``. > > > > > > > > > > > > diff --git a/doc/guides/prog_guide/rte_security.rst= b/doc/guides/prog_guide/rte_security.rst > > > > > > > > > > > > index f72bc8a78..7b68c698d 100644 > > > > > > > > > > > > --- a/doc/guides/prog_guide/rte_security.rst > > > > > > > > > > > > +++ b/doc/guides/prog_guide/rte_security.rst > > > > > > > > > > > > @@ -560,7 +560,11 @@ created by the application is = attached to the security session by the API > > > > > > > > > > > > > > > > > > > > > > > > For Inline Crypto and Inline protocol offload, dev= ice specific defined metadata is > > > > > > > > > > > > updated in the mbuf using ``rte_security_set_pkt_m= etadata()`` if > > > > > > > > > > > > -``DEV_TX_OFFLOAD_SEC_NEED_MDATA`` is set. > > > > > > > > > > > > +``RTE_SECURITY_TX_OLOAD_NEED_MDATA`` is set. ``rte= _security_set_pkt_metadata()`` > > > > > > > > > > > > +should be called on mbuf only with Layer 3 and abo= ve data present and > > > > > > > > > > > > +``mbuf.data_off`` should be pointing to Layer 3 He= ader. > > > > > > > > > > > > > > > > > > > > > > Hmm... not sure why mbuf.data_off should point to L3 = hdr. > > > > > > > > > > > Who will add L2 hdr to the packet in that case? > > > > > > > > > > > Or did you mean ``mbuf.data_off + mbuf.l2_len`` here? > > > > > > > > > > > > > > > > > > > > That is the semantics I was trying to define. I think b= elow 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 data. > > > > > > > > > > 5. Do route_lookup() > > > > > > > > > > 6. add_l2hdr() which might be different from stripped l= 2hdr. > > > > > > > > > > 7. Send packet out. > > > > > > > > > > > > > > > > > > > > The above sequence is what I believe the current poll m= ode worker thread in > > > > > > > > > > ipsec-secgw is following. > > > > > > > > > > > > > > > > > > That's just a sample app, it doesn't mean it has to be th= e only possible way. > > > > > > > > > > > > > > > > > > > While in event mode, step 2 and step 6 are missing. > > > > > > > > > > > > > > > > > > I think this L2 hdr manipulation is totally optional. > > > > > > > > > If your rte_security_set_pkt_metadata() implementation re= ally needs to know L3 hdr offset (not sure why?), > > > > > > > > Since rte_security_set_pkt_metadata() is PMD specific funct= ion ptr call, we are currently doing some pre-processing > > > > > > > > here before submitting packet to inline IPSec via rte_eth_t= x_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 except fo= r saving SA priv data to rte_mbuf. > > > > > > > > > > > > > > > > > then I suppose we can add a requirement that l2_len has t= o 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 propose: > > > > > > > 1. before calling rte_security_set_pkt_metadata() mbuf.l2_len= should be properly set. > > > > > > > 2. after rte_security_set_pkt_metadata() and before rte_eth_t= x_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_tx_prepar= e() concept? > > > > > > > > > > > > Since our PMD doesn't have a prepare function, I missed that bu= t, 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 m= odified 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 be allowe= d 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 called af= ter all L3 > > > > and above header modifications is no a problem. I guess existing ix= gbe/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 inside 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 when 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, an= d before tx_burst(), > > mbuf data / packet len cannot be modified right as if modified, then tx= _burst() > > will be using incorrect pad len ? >=20 > No, pkt_len can be modified. > Though ESP trailer pad_len can't. >=20 > > > > This patch is also trying to add similar restriction on when > > rte_security_set_pkt_metadata() should be called and what cannot be don= e after > > calling rte_security_set_pkt_metadata(). >=20 > No, I don't think it is really the same. > Also, IMO, inside ixgbe set_pkt_metadata() implementaion we probably shou= ldn'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 funct= ion. >=20 > > > > > > > > > > > > > > > > > > > > > > > > > > If your question is can't we do the preprocessing in rte_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 callback 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 overhead > > > > > of function call will be spread around the whole burst, and I pre= sume > > > > > shouldn't be too high. > > > > > > > > > > > Also rte_eth_tx_prepare() if implemented will be called for bot= h security and > > > > > > non-security pkts. > > > > > > > > > > Yes, but tx_prepare() can distinguish (by ol_flags and/or other f= ield 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 argument thoug= h ol_flags has security flag. > > > > In our case, we need to know the security session details to do = things. > > > > > > I suppose you can store pointer to session (or so) inside mbuf in rte= _security_dynfield, no? > > > > We can do. But having to call PMD specific function call via rte_securi= ty_set_pkt_metadata() > > just for storing session pointer in rte_security_dynfield consumes unne= cessary > > cycles per pkt. >=20 > In fact there are two function calls: one for rte_security_set_pkt_metada= ta(), > 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 be as fas= t as possible. > Probably 21.11 is a right timeframe for that. >=20 > > > > > > > 2. AFAIU tx_prepare() is not mandatory as per spec and even by defa= ult disabled under compile time > > > > macro RTE_ETHDEV_TX_PREPARE_NOOP. > > > > 3. Even if we do tx_prepare(), rte_security_set_pkt_mdata() is mand= atory to associate > > > > struct rte_security_session to a pkt as unlike ol_flags, there i= s no direct space to do the same. > > > > > > Didn't get you here, obviously we do have rte_security_dynfield insid= e mbuf, > > > specially for that - to store secuiryt related data inside the 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_prepare() f= or inline security > > > > processing, it can be done via security specific set_pkt_metadata()= . > > > > > > But what you proposing introduces new limitations and might existing = functionality. > > > BTW, if you don't like to use tx_prepare() - why doing these calculat= ions inside tx_burst() > > > itself is not an option? > > > > We can do things in tx_burst() but if we are doing it there, then we wa= nt to avoid having callback for > > rte_security_set_pkt_metadata(). > > > > Are you fine if we can update the spec that "When DEV_TX_OFFLOAD_SEC_NE= ED_MDATA is not > > set, then, user needs to update struct rte_security_session's sess_priv= ate_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_OLOAD_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->security.ctx, > > ss->security.ses, mb[i], NULL); > > else > > *rte_security_dynfield(mb[i]) =3D > > (uint64_t)ss->security.ses->sess_privat= e_data; > > > > > > If the above can be done, then in our PMD, we will not have a callback = for > > set_pkt_metadata() and DEV_TX_OFFLOAD_SEC_NEED_MDATA will also be not s= et > > in capabilities. >=20 > 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 tha= t. > So, I'd suggest to tweak your idea a bit: >=20 > 1) change rte_security_set_pkt_metadata(): > if ops->set_pkt_metadata !=3D NULL, then call it (existing behaviour) > otherwise just: rte_security_dynfield(m) =3D sess->session_private_data; > (fast-path) >=20 > 2) consider to make rte_security_set_pkt_metadata() inline function. > 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_useradata() are not really used by PMDs, it might be easier to deprecate these callback= s. >=20 > As a brief code snippet: >=20 > 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 *); > }; >=20 > 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)(sess= ion->sess_priv_data); > return 0; > /* slow path */ > } else > return instance->set_pkt_mdata(instance->device, sess, m, para= ms); > } >=20 > That probably would be an ABI breakage (new fileld in rte_security_ctx) a= nd would require > some trivial changes for all existing PMDs that use RTE_SECURITY_TX_OFLOA= D_NEED_MDATA > (ctx_create()), but hopefully will benefit everyone. >=20 > > > > > > > > > I'm fine to > > > > introduce a burst call for the same(I was thinking to propose it in= future) to > > > > compensate for the overhead. > > > > > > > > If rte_security_set_pkt_metadata() was not a PMD specific function = 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 ar= e 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 whatever is ne= eded. > > > > > > > then then I guess it would have been better to do the way you propo= sed. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > This patch is trying to enforce semantics as above so t= hat > > > > > > > > > > rte_security_set_pkt_metadata() can predict what comes = in the pkt when he is > > > > > > > > > > called. > > > > > > > > > > > > > > > > > > > > I also think above sequence is what Linux kernel 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 called agai= n. > > > > > > > > > > > > > > > > > > > > > > > > For inline protocol offloaded ingress traffic, the= application can register a > > > > > > > > > > > > pointer, ``userdata`` , in the security session. W= hen the packet is received, > > > > > > > > > > > > diff --git a/lib/mbuf/rte_mbuf_core.h b/lib/mbuf/rt= e_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 p= acket. > > > > > > > > > > > > + * To use Tx security offload, the user needs to f= ill l2_len in mbuf > > > > > > > > > > > > + * indicating L2 header size and where L3 header s= tarts. > > > > > > > > > > > > */ > > > > > > > > > > > > #define PKT_TX_SEC_OFFLOAD (1ULL << 43) > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > > > > > 2.25.1 > > > > > > > > > > >