Re: [PATCH] net/hns3: support disable IOVA as PA mode

[Bruce Richardson <bruce.richardson@intel.com>]

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