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

["Morten Brørup" <mb@smartsharesystems.com>]

> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> 
> On Mon, Feb 20, 2023 at 01:04:02PM +0100, Morten Brørup 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ø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/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.