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 56BA941CEC; Mon, 20 Feb 2023 16:07:15 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id E235943069; Mon, 20 Feb 2023 16:07:14 +0100 (CET) Received: from smartserver.smartsharesystems.com (smartserver.smartsharesystems.com [77.243.40.215]) by mails.dpdk.org (Postfix) with ESMTP id F014C40395 for ; Mon, 20 Feb 2023 16:07:13 +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 16:07:09 +0100 Message-ID: <98CBD80474FA8B44BF855DF32C47DC35D8775F@smartserver.smartshare.dk> In-Reply-To: X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: [PATCH] net/hns3: support disable IOVA as PA mode Thread-Index: AdlFNXFlIYJk3WJITne4IOU/sxXf4AAAF85g References: <20230214071141.50155-1-fengchengwen@huawei.com> <7487991.nlapOpYt14@thomas> <98CBD80474FA8B44BF855DF32C47DC35D87754@smartserver.smartshare.dk> <1759668.5KxKD5qtyk@thomas> <98CBD80474FA8B44BF855DF32C47DC35D87757@smartserver.smartshare.dk> <98CBD80474FA8B44BF855DF32C47DC35D8775B@smartserver.smartshare.dk> <98CBD80474FA8B44BF855DF32C47DC35D8775C@smartserver.smartshare.dk> From: =?iso-8859-1?Q?Morten_Br=F8rup?= To: "Bruce Richardson" Cc: "Thomas Monjalon" , "Chengwen Feng" , "Ruifeng Wang" , , "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: Bruce Richardson [mailto:bruce.richardson@intel.com] > Sent: Monday, 20 February 2023 15.13 >=20 > On Mon, Feb 20, 2023 at 01:47:13PM +0100, Morten Br=F8rup wrote: > > > From: Bruce Richardson [mailto:bruce.richardson@intel.com] > > > > > > On Mon, Feb 20, 2023 at 01:04:02PM +0100, Morten Br=F8rup wrote: > > > > > From: Bruce Richardson [mailto:bruce.richardson@intel.com] > > > > > Sent: Monday, 20 February 2023 12.53 > > > > > > > > > > On Mon, Feb 20, 2023 at 12:12:50PM +0100, Morten Br=F8rup = wrote: > > > > > > > From: Thomas Monjalon [mailto:thomas@monjalon.net] > > > > > > > Sent: Monday, 20 February 2023 11.17 > > > > > > > > > > > > > > 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". > > > > > > > > > > > > > > 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/me > > > > > mory-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. > > > > > > > > > > > I agree that this option is confusing, and thinking about it, = I > > > agree > > > > > that > > > > > a pair of named option is probably better than just a > true/false > > > > > booleans. > > > > > My current thinking is that a combo option is best - maybe > named: > > > > > "supported_iova_modes", with possible values ["va_and_pa", > > > "va_only"] > > > > > may > > > > > be clearest. However, that would be a change in how things are > > > > > currently > > > > > configured. > > > > > > > > > > A alternative if we want to keep compatibility, is to expand = or > > > clarify > > > > > the > > > > > help text for the existing "enable_iova_as_pa" option. The > current > > > help > > > > > text reads: > > > > > > > > > > "Support for IOVA as physical address. Disabling removes the > > > buf_iova > > > > > field > > > > > of mbuf." > > > > > > > > > > We could expand that to e.g.: > > > > > > > > > > "Support the use of physical addresses for IO addresses, such > as > > > used > > > > > by > > > > > VFIO in no-iommu mode, or UIO-based drivers. When disabled, > DPDK > > > can > > > > > only > > > > > run with IOMMU support for address mappings, but will have = more > > > space > > > > > available in the mbuf structure". > > > > > > > > > > Such an explanation is quite a bit longer, but I see meson = does > a > > > > > decent > > > > > job of wrapping the output of "meson configure" in latest > versions. > > > > > > > > > > /Bruce > > > > > > > > Updating the description of the meson configuration option would > be > > > an improvement. > > > > > > > > But I'm thinking more about the ripple effect into the resulting > > > #define's, and the code using those. It would be nice getting this > > > cleaned up. Which is why I compare the IOVA mode to CPU = Endianness, > as > > > an example of a Boolean value represented by multiple #define's = for > > > code readability purposes. > > > > > > > > But I suppose such a change has too widely reaching side = effects, > > > regarding backwards compatibility. > > > > > > Actually, I think the current internal define is pretty ok right > now. > > > RTE_IOVA_AS_PA would probably be better as > "RTE_SUPPORT_IOVA_AS_PA", > > > but I > > > don't think the lack of the alternative value > "RTE_SUPPORT_IOVA_AS_VA" > > > is > > > an issue since a DPDK build always supports that - it's only at > runtime > > > it > > > may not be supported e.g. if no IOMMU is present. > > > > It's not that simple. There's a difference between runtime IOVA as = VA > mode and build time IOVA as VA mode. > > > > E.g. this hns3 patch adds support for build time IOVA_AS_VA mode. > > > Ok, that is another way of looking at it, though to me I still view > that > mode as dropping support for PA rather than doing anything with VA > mode. It seems I have just been looking at it the wrong way, and needed to = shake my head. > However, no problem putting in an extra build-time define for > RTE_IOVA_AS_VA_ONLY or similar. With the new viewing angle, the current define RTE_IOVA_AS_PA makes more = sense to me now than before. So we should probably stick with it, rather = than introduce something that might confuse developers who already have = the same viewing angle. But it still seems counterintuitive to me that disabling some feature = ("enable_iova_as_pa") is not supported throughout DPDK; the logic seems = inverted. Apparently, it also makes it difficult to assign good titles = to patches that support disabling such a feature. :-) On the positive side, since everything supports this "enable_iova_as_pa" = feature, we don't need to add it to the PMD feature list. If the logic = wasn't inverted like this, the PMD feature list should probably reflect = which PMDs supported the "iova_as_va_only" compile time option. ;-)