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 D2A1D41CE5; Mon, 20 Feb 2023 12:12:53 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id B088C43024; Mon, 20 Feb 2023 12:12:53 +0100 (CET) Received: from smartserver.smartsharesystems.com (smartserver.smartsharesystems.com [77.243.40.215]) by mails.dpdk.org (Postfix) with ESMTP id 940B240395 for ; Mon, 20 Feb 2023 12:12:52 +0100 (CET) X-MimeOLE: Produced By Microsoft Exchange V6.5 Content-class: urn:content-classes:message MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Subject: RE: [PATCH] net/hns3: support disable IOVA as PA mode Date: Mon, 20 Feb 2023 12:12:50 +0100 Message-ID: <98CBD80474FA8B44BF855DF32C47DC35D87757@smartserver.smartshare.dk> In-Reply-To: <1759668.5KxKD5qtyk@thomas> X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: [PATCH] net/hns3: support disable IOVA as PA mode Thread-Index: AdlFFHOX8RY/twm/RDaWahA2iB2DBgAAgz3g References: <20230214071141.50155-1-fengchengwen@huawei.com> <7487991.nlapOpYt14@thomas> <98CBD80474FA8B44BF855DF32C47DC35D87754@smartserver.smartshare.dk> <1759668.5KxKD5qtyk@thomas> From: =?iso-8859-1?Q?Morten_Br=F8rup?= To: "Thomas Monjalon" , , "Chengwen Feng" , "Ruifeng Wang" Cc: , "nd" , , "Dongdong Liu" , "Yisen Zhuang" 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 > From: Thomas Monjalon [mailto:thomas@monjalon.net] > Sent: Monday, 20 February 2023 11.17 >=20 > 20/02/2023 10:43, Morten Br=F8rup: > > > From: Thomas Monjalon [mailto:thomas@monjalon.net] > > > Sent: Monday, 20 February 2023 08.45 > > > > > > 16/02/2023 09:36, Ruifeng Wang: > > > > From: Chengwen Feng > > > > > Subject: [PATCH] net/hns3: support disable IOVA as PA mode > > > > > > Could we change the title to "support IOVA as VA" ? > > > > The underlying problem is the meson configuration option name for > this feature [1]: > > > > option('enable_iova_as_pa', type: 'boolean', value: true, > description: > > 'Support for IOVA as physical address. Disabling removes the > buf_iova field of mbuf.') > > > > [1]: > https://elixir.bootlin.com/dpdk/v22.11.1/source/meson_options.txt#L43 > > > > Formally, the patch provides the ability to set a boolean > configuration value ("enable_iova_as_pa") to false, and thus the patch > title is correct. > > > > Nonetheless, I agree that the title suggested by Thomas is an > improvement. > > > > > > Going back to the root cause, I think the configuration option = should > be an enum instead of a boolean, e.g. "iova_mode" with values = "iova_pa" > and "iova_va". >=20 > We can enable both and have it decided at runtime. So I think the > boolean is OK. I forgot that it could be changed at runtime. I'll share a few thoughts for consideration, but expect no further = replies. Sorry about the noise. ;-) The documentation [2] says that IOVA as PA is always supported, and is = the default mode. Support for IOVA as VA is optional. [2]: = https://www.intel.com/content/www/us/en/developer/articles/technical/memo= ry-in-dpdk-part-2-deep-dive-into-iova.html IOVA as VA can be selected at runtime, as you mention, or at build time. = But selecting IOVA as VA (at runtime or build time) requires support by = the underlying environment/hardware. If IOVA as PA is always supported (and is the default), the name of this = meson configuration option could be improved. Its current name says = "enable feature X", but if feature X is already supported by default, = the name seems meaningless. If we want to keep it boolean, it could be = inverted, e.g.: "iova_as_va_only" with default value "false". However, if modifying the meson configuration option (name and/or type) = doesn't reduce the risk of confusion with the various IOVA modes, it's = not worth the effort. >=20 > > It's somewhat similar to CPU endian macros. We have macros defining > both Big Endian and Little Endian, not just one macro defining Big > Endian or not. > > > > @Bruce, would it be hard for you to change the IOVA configuration > option from a boolean to a two-value enum? > > > > Or - also considering the resulting #define's - would it be too > difficult to keep a sufficient level of backwards/API compatibility? >=20 >=20 >=20