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

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

> 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.

> 
> > It's somewhat similar to CPU endian macros. We have macros defining
> both Big Endian and Little Endian, not just one macro defining Big
> Endian or not.
> >
> > @Bruce, would it be hard for you to change the IOVA configuration
> option from a boolean to a two-value enum?
> >
> > Or - also considering the resulting #define's - would it be too
> difficult to keep a sufficient level of backwards/API compatibility?
> 
> 
>