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 0FFDFA0C51; Tue, 13 Jul 2021 14:33:21 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id EC4C541251; Tue, 13 Jul 2021 14:33:20 +0200 (CEST) Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by mails.dpdk.org (Postfix) with ESMTP id ACBE5410E8 for ; Tue, 13 Jul 2021 14:33:19 +0200 (CEST) X-IronPort-AV: E=McAfee;i="6200,9189,10043"; a="208334115" X-IronPort-AV: E=Sophos;i="5.84,236,1620716400"; d="scan'208";a="208334115" Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga104.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 13 Jul 2021 05:33:15 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.84,236,1620716400"; d="scan'208";a="649447672" Received: from orsmsx605.amr.corp.intel.com ([10.22.229.18]) by fmsmga006.fm.intel.com with ESMTP; 13 Jul 2021 05:33:14 -0700 Received: from orsmsx611.amr.corp.intel.com (10.22.229.24) by ORSMSX605.amr.corp.intel.com (10.22.229.18) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2242.4; Tue, 13 Jul 2021 05:33:13 -0700 Received: from ORSEDG601.ED.cps.intel.com (10.7.248.6) by orsmsx611.amr.corp.intel.com (10.22.229.24) 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 05:33:13 -0700 Received: from NAM10-DM6-obe.outbound.protection.outlook.com (104.47.58.108) by edgegateway.intel.com (134.134.137.102) 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 05:33:13 -0700 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=B7YFf+nRM5UKU/7fMGAYNmSlvJVdaYqNUjXwTtmhJQGQoVBEX/g6lN7HiZ52bXLwhyUeUlP2hXRfKWPASPcNA9XqNjAULnZUhTWCNiO9Ej9oq9PNtU5ln6LQ4IvQYErY5qstPEate1zY8SN8gTE0mlNGheoOrGfK+npqwUN35vKsVYYSSWPN6rPWZe9OdTSxjOeU2B6dmVoUe8/UkNo7QLHrsar6YUek2o1FgHzKj8/BXeW+Mzrzpprgvl6i+SYwKEuMHpQPM04Be03opt+YHr+s1rSKt5mJO5vEjnU+IvfFoTQKKGu1NaN/kwSbK0MdXlrPSk5jfGsKRFfsrUusDg== 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=wHmaJflvlw95i5h2/lZ/O8v9y0qHwtPIdCGYJAl7Ero=; b=e4DKEj+ORW3ilKZ3qoGjEJ3ljFGHrf2gO7kFoORFQ1drZi4Ui/YZdkkmu9jYpYwacKWLObaHtzgI3R3YATselqvICYYgvfTU4BJ1lpScYw4YOUNQylzYGVK5AfegWcq+noZM3r3DVHVZqhVnGE2lavJDdmNxgT18nnnz7FaREMkf8+maPN4yIpPP399HKlr48gzITiHHvgTayIDkI6lmzlTK0M39oXcW7QXe3knGxTl1h7zD7/CZf/50fJPTPdydDC4v3jlqVhTv1aolGDfIRduWnrtowy/6owrrdlNLQMEkdA37T5i+a8ppTGk0MNTpZXPb7vev/8NcTihlBujRAA== 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=wHmaJflvlw95i5h2/lZ/O8v9y0qHwtPIdCGYJAl7Ero=; b=Kb57onTtsh+NM/m7f5XQVHARbyGQDwYlGELqTTgl23Q1B/K3Bmvd1cYcF+YY/PX9kJgRvh9TzfWVpDVWIESFmsAPmORKUyFK+fH5o5YpvM8zWHwXjmy3w+8qJwbCelMgIO35S0QUtcy+v2MjJH7PsmpdXRqTxKV+2cSnXzIvCc4= Received: from DM6PR11MB4491.namprd11.prod.outlook.com (2603:10b6:5:204::19) by DM6PR11MB4740.namprd11.prod.outlook.com (2603:10b6:5:2ad::15) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4308.23; Tue, 13 Jul 2021 12:33:11 +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 12:33:11 +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/e16s12L8QgAAaAACAAAK4QIAABe0AgAAQV7CAAUGeAIAACA8AgAAdpACABMZzgIADc9iAgAEZGjA= Date: Tue, 13 Jul 2021 12:33:10 +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: 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: 1605704a-74b1-4d44-97e8-08d945fa6077 x-ms-traffictypediagnostic: DM6PR11MB4740: 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: PjjYnv1hAMZUpYioeKlVLk2UDqLeUpce02hS5wy1G0DSKAHgrgKWmSSZjk3JGxIAXUY7d+8xhj1gN75aQxfYqNMvnhTA1uLgHZRWo1HJmLOCNgRwNau8pXYWSuhEl2/B/J8JG2PWA2PYZT3VU6ByI7kJI3znhPquNxtH9wPo8MMkGdLqhmqnI+HkP/5WD4ee/mONpPLSIxYJe9GPuxSVdsP0xiAum9qjiUUNcr/HGWBHI3V33hVtrKM/evWpZy3i71Lq/DO/CvDiNu+Erc6fHr3D2u9QhXtwuaMQqOUAltj3lr1GXYYy3mxp81gZ6nnr2uABGS0Efh9y9ZHiyYuyFiQ/P9Gq5cOijo8buXXMSy+blG5ebTnSwoxKzJmgcLPm9MdWZox/IZeqnS3cIDUJqh2xZjXQ+62cFpwvMeucb9gJ1dRfqxx+9aIiqvcYndKVHxh7m84HvjAG3PomDFzauKoB2TWlYMTXu2ZNrc494kFCrKjVt4BDHwIBfoqWKtpJGMj5L2z8VWcn+kswe+BxIeTxfoLqfka5zkCvrO5jca5fwrGNUiNk5DXJf947eChOlS0qCluYrQKaprfUy8GnghrZ3mTaRh8TSjqUp+G6EhuMxh328W4yBfIOdT7ZWpKuAzV6U2pzxI6HbHcr08y6GQ== 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)(376002)(39860400002)(366004)(396003)(346002)(136003)(8936002)(33656002)(38100700002)(66476007)(6916009)(30864003)(316002)(4326008)(26005)(186003)(86362001)(6506007)(8676002)(71200400001)(55236004)(55016002)(66446008)(478600001)(9686003)(66946007)(54906003)(64756008)(2906002)(76116006)(122000001)(7696005)(83380400001)(5660300002)(15650500001)(52536014)(66556008)(7416002); DIR:OUT; SFP:1102; x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?Ku92n43A1LvDAvYJ1DkPAklOWNIUFlhNeJAFtYb1x6FanVb/DR3/rnHGaZg4?= =?us-ascii?Q?KcXVcdKFPKN1+gwsCbJJnuoic/lSJUZy2449rGQ8o2tXVwpiLmbM37iWBMZW?= =?us-ascii?Q?Jw/tSLPnMvqcbETZLIv8gf4ur0/vTZjlet0pJKRcjNjc5b4fmq+SZxjwmOaO?= =?us-ascii?Q?a084cyYMsW65yYcddqYxJvafXMmKJ27bwtk9qsV9nIMhjPg7Phu3DZ3XPOvf?= =?us-ascii?Q?mjwQgd9SJc+I1dkBcKx6BOThOJC/RShoJBf+1paEp41HTe/6tRj2E+T7byfu?= =?us-ascii?Q?A03da3ec8s7Xqr/LCPvF82MhhbM53DDgkpPJtyuR6f5o5M4NgYolXfINCKAZ?= =?us-ascii?Q?YiMDxhLj6mbv6AD4M6GTuxA9s36KX/MIVabhStKxQ0tyH9yZcpEgnowiQteo?= =?us-ascii?Q?ww39NjwN1nO6eYCuRA4fjDBxkoYlC1qN6nSBCx7bObA919ZNa39kzw5U4hjK?= =?us-ascii?Q?ZH4dXz8vCQ0wGtCf+J8nOR9bljrLdq6WxdO54p2dNR8ZwWEY0Jd62k8wi5dj?= =?us-ascii?Q?yMV5hGc8qn5DluNU6zCfbSkpjBimljXVw9W+YfhzU3HXDr8f8GPvoZryH5GI?= =?us-ascii?Q?Vscd0s/b6HzkkB3SKltdnW43a2mxuYMGpFSgZVAkVLS3C1iCDx50yR81cGoK?= =?us-ascii?Q?+QpMvdyqR3+6wI0GztVLSGCz1cURrHKs0XeqWG7LmkjfCMUHN4sf3TslriYD?= =?us-ascii?Q?T2LQ1EndP66Jgz04tm502fRnhBYDvyG2dn252wMWcvBkLmYn2uhOW9pulByA?= =?us-ascii?Q?XvQLCHFdwPYFkjEX3S73vOVjKPjPQLcBSu7oATEq+wr5L72EAeRFBUe60DDy?= =?us-ascii?Q?A73y+gEOj0fEs+6Fr8FOevuLVEmd0+O3djb7pXVNMeg1Gg08AW+HtbLhgH9y?= =?us-ascii?Q?oNtz7m5JHfQ+iYC5GEWKP4uSL4OnTgrK0yNZmlcqL8B8Tg5dR7CBwwzuRdyn?= =?us-ascii?Q?svblCWXFg1nqAqtutrUUVbX0X0dNbt2c1RHwu4RC+YBJu++iJiFJrQ21rpR0?= =?us-ascii?Q?SwePdRhxyg1wO68mLsx4Qrn5odYnWg14aOR8mcd2fHt8Uxjfi1JTdlnYDjeY?= =?us-ascii?Q?ylwjFGYjn4YEeEnt+8Qz/zWRGN+8Wi2AhdhrYdZDRSQ2yEJRlLonpZo6SbeJ?= =?us-ascii?Q?PVjpKniSUZOncfvDjLTciai4jQIhwntknhMboW3gH1SPfaoN9JSBI3vU5+Y+?= =?us-ascii?Q?YqcNUbSvbNZPKl2706UB9Grf+/qEV5P/aRHk9xdQ/P97lqR8d5qrkrk0P7Ts?= =?us-ascii?Q?Ij+KAusBsvppwe0dK172WGPmWMkD7HdreZOBlo9/GquIYWbVhQvf2X/ZGRZQ?= =?us-ascii?Q?AjFAfayZxniUkJWLGjSx5vOb?= 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: 1605704a-74b1-4d44-97e8-08d945fa6077 X-MS-Exchange-CrossTenant-originalarrivaltime: 13 Jul 2021 12:33:10.9818 (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: kjnBiqeInU+X58C9baGRQ4+oTiUVsLiUxEeXKR+pf9cWwURr11yX4NPhWG7TzYyhfpO1yswIAoFv75v7HWgpKyhYCS5uOAujAaKoRQllwTQ= X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR11MB4740 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" 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 call= ed for pkts > > > > > > > > > > > to associate a Security session with a mbuf before su= bmitting > > > > > > > > > > > to Ethdev Tx. This is apart from setting PKT_TX_SEC_O= FFLOAD in > > > > > > > > > > > mbuf.ol_flags. rte_security_set_pkt_metadata() is als= o used to > > > > > > > > > > > set some opaque metadata in mbuf for PMD's use. > > > > > > > > > > > This patch updates documentation that rte_security_se= t_pkt_metadata() > > > > > > > > > > > should be called only with mbuf containing Layer 3 an= d above data. > > > > > > > > > > > This behaviour is consistent with existing PMD's such= as ixgbe. > > > > > > > > > > > > > > > > > > > > > > On Tx, not all net PMD's/HW can parse packet and iden= tify > > > > > > > > > > > L2 header and L3 header locations on Tx. This is inli= ne with other > > > > > > > > > > > Tx offloads requirements such as L3 checksum, L4 chec= ksum offload, > > > > > > > > > > > etc, where mbuf.l2_len, mbuf.l3_len etc, needs to be = set for > > > > > > > > > > > HW to be able to generate checksum. Since Inline IPSe= c is also > > > > > > > > > > > such a Tx offload, some PMD's at least need mbuf.l2_l= en to be > > > > > > > > > > > valid to find L3 header and perform Outbound IPSec pr= ocessing. > > > > > > > > > > > Hence, this patch updates documentation to enforce se= tting > > > > > > > > > > > mbuf.l2_len while setting PKT_TX_SEC_OFFLOAD 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 deletion(-) > > > > > > > > > > > > > > > > > > > > > > diff --git a/doc/guides/nics/features.rst b/doc/guide= s/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 Secur= ity 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_creat= e``, ``session_update``, > > > > > > > > > > > ``session_stats_get``, ``session_destroy``, ``set_= pkt_metadata``, ``capabilities_get``. > > > > > > > > > > > * **[provides] rte_eth_dev_info**: ``rx_offload_capa= ,rx_queue_offload_capa:DEV_RX_OFFLOAD_SECURITY``, > > > > > > > > > > > @@ -451,6 +452,7 @@ protocol operations. See security= 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_creat= e``, ``session_update``, > > > > > > > > > > > ``session_stats_get``, ``session_destroy``, ``set_= 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 at= tached to the security session by the API > > > > > > > > > > > > > > > > > > > > > > For Inline Crypto and Inline protocol offload, devic= e specific defined metadata is > > > > > > > > > > > updated in the mbuf using ``rte_security_set_pkt_met= adata()`` if > > > > > > > > > > > -``DEV_TX_OFFLOAD_SEC_NEED_MDATA`` is set. > > > > > > > > > > > +``RTE_SECURITY_TX_OLOAD_NEED_MDATA`` is set. ``rte_s= ecurity_set_pkt_metadata()`` > > > > > > > > > > > +should be called on mbuf only with Layer 3 and above= data present and > > > > > > > > > > > +``mbuf.data_off`` should be pointing to Layer 3 Head= er. > > > > > > > > > > > > > > > > > > > > Hmm... not sure why mbuf.data_off should point to L3 hd= r. > > > > > > > > > > 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 bel= ow 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 b= e 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 l2h= dr. > > > > > > > > > 7. Send packet out. > > > > > > > > > > > > > > > > > > The above sequence is what I believe the current poll mod= e 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 missing. > > > > > > > > > > > > > > > > I think this L2 hdr manipulation is totally optional. > > > > > > > > If your rte_security_set_pkt_metadata() implementation real= ly needs to know L3 hdr offset (not sure why?), > > > > > > > Since rte_security_set_pkt_metadata() is PMD specific functio= n ptr call, we are currently doing some pre-processing > > > > > > > here before submitting packet to inline IPSec via rte_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 rt= e_security_set_pkt_metadata() call, then I think > > > > > > > having a PMD specific callback is not much of use except for = saving SA priv data to rte_mbuf. > > > > > > > > > > > > > > > then I suppose we can add a requirement that l2_len 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 propose: > > > > > > 1. before calling rte_security_set_pkt_metadata() mbuf.l2_len s= hould be properly set. > > > > > > 2. after rte_security_set_pkt_metadata() and before rte_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_tx_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/Pr= otocol via > > > > > a rte_eth_dev, and both rte_security_set_pkt_metadata() and rte_e= th_tx_prepare() > > > > > are callbacks from same PMD, do you see any issue ? > > > > > > > > > > The restriction is from user side, data is not supposed to be mod= ified 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 allowed = any more. > > > You can still modify L2 header and IPSEC is only concerned about L3 a= nd above. > > > > > > I think insisting that rte_security_set_pkt_metadata() be called afte= r all L3 > > > and above header modifications is no a problem. I guess existing ixgb= e/txgbe > > > PMD which are the ones only implementing the call back are already ex= pecting 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 IC= V): >=20 > This implies rte_security_set_pkt_metadata() cannot be called when mbuf d= oes't > have ESP trailer updated or when mbuf->pkt_len =3D 0 >=20 > > > > 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_b= urst() > will be using incorrect pad len ? 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 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 probably should= n'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 functio= n. >=20 > > > > > > > > > > > > > > > > > > > If your question is can't we do the preprocessing in rte_eth_tx_p= repare() 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 presu= me > > > > 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 fie= ld 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 though = ol_flags has security flag. > > > In our case, we need to know the security session details to do th= ings. > > > > I suppose you can store pointer to session (or so) inside mbuf in rte_s= ecurity_dynfield, no? >=20 > We can do. But having to call PMD specific function call via rte_security= _set_pkt_metadata() > just for storing session pointer in rte_security_dynfield consumes unnece= ssary > cycles per pkt. In fact there are two function calls: one for rte_security_set_pkt_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 be as fast = 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 defaul= t disabled under compile time > > > macro RTE_ETHDEV_TX_PREPARE_NOOP. > > > 3. Even if we do tx_prepare(), rte_security_set_pkt_mdata() is mandat= ory 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_dynfield inside = 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() for= inline security > > > processing, it can be done via security specific set_pkt_metadata(). > > > > But what you proposing introduces new limitations and might existing fu= nctionality. > > BTW, if you don't like to use tx_prepare() - why doing these calculatio= ns inside tx_burst() > > itself is not an option? >=20 > We can do things in tx_burst() but if we are doing it there, then we want= to avoid having callback for > rte_security_set_pkt_metadata(). >=20 > Are you fine if we can update the spec that "When DEV_TX_OFFLOAD_SEC_NEED= _MDATA is not > set, then, user needs to update struct rte_security_session's sess_privat= e_data in a in > rte_security_dynfield like below ? >=20 > >=20 > 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; >=20 > ol_flags =3D ss->security.ol_flags & RTE_SECURITY_TX_OLOAD_NEED_M= DATA; > for (i =3D 0; i !=3D num; i++) { >=20 > mb[i]->ol_flags |=3D PKT_TX_SEC_OFFLOAD; >=20 > 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_private_= data; >=20 >=20 > If the above can be done, then in our PMD, we will not have a callback fo= r > 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 r= te_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 behaviour) otherwise just: rte_security_dynfield(m) =3D sess->session_private_data; (fast-path) 2) consider to make rte_security_set_pkt_metadata() inline function.=20 We probably can have some special flag inside struct rte_security_ctx, or even store inside ctx a pointer to set_pkt_metadata() itself. 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 rt= e_mbuf *, void *); =20 };=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)(sessio= n->sess_priv_data); return 0;=20 /* slow path */=20 } else return instance->set_pkt_mdata(instance->device, sess, m, params= ); } That probably would be an ABI breakage (new fileld in rte_security_ctx) and= would require=20 some trivial changes for all existing PMDs that use RTE_SECURITY_TX_OFLOAD_= 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 f= uture) to > > > compensate for the overhead. > > > > > > If rte_security_set_pkt_metadata() was not a PMD specific function pt= r 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 whatever is need= ed. > > > > > then then I guess it would have been better to do the way you propose= d. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > This patch is trying to enforce semantics as above so tha= t > > > > > > > > > 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 a= round unless > > > > > > > > > > > +``rte_security_set_pkt_metadata()`` is called again. > > > > > > > > > > > > > > > > > > > > > > For inline protocol offloaded ingress traffic, the a= pplication can register a > > > > > > > > > > > pointer, ``userdata`` , in the security session. Whe= n 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 pac= ket. > > > > > > > > > > > + * To use Tx security offload, the user needs to fil= l l2_len in mbuf > > > > > > > > > > > + * indicating L2 header size and where L3 header sta= rts. > > > > > > > > > > > */ > > > > > > > > > > > #define PKT_TX_SEC_OFFLOAD (1ULL << 43) > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > > > > 2.25.1 > > > > > > > > > >