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

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

> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Monday, 20 February 2023 15.13
> 
> On Mon, Feb 20, 2023 at 01:47:13PM +0100, Morten Brørup wrote:
> > > 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.
> >
> 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. :-)

<irony>
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. ;-)
</irony>