On Mon, Feb 20, 2023 at 12:12:50PM +0100, Morten Brørup wrote: > > From: Thomas Monjalon [mailto:tho...@monjalon.net] > > Sent: Monday, 20 February 2023 11.17 > > > > 20/02/2023 10:43, Morten Brørup: > > > > From: Thomas Monjalon [mailto:tho...@monjalon.net] > > > > Sent: Monday, 20 February 2023 08.45 > > > > > > > > 16/02/2023 09:36, Ruifeng Wang: > > > > > From: Chengwen Feng <fengcheng...@huawei.com> > > > > > > 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/memory-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
