Re: [PATCH v2 1/2] build: clarify configuration without IOVA field in mbuf

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

On Mon, Mar 06, 2023 at 05:13:27PM +0100, Thomas Monjalon wrote:
> The impact of the option "enable_iova_as_pa" is explained for users.
> 
> Also the code flag "RTE_IOVA_AS_PA" is renamed as "RTE_IOVA_IN_MBUF"
> in order to be more accurate (IOVA mode is decided at runtime),
> and more readable in the code.
> 
> Similarly the drivers are using the variable "require_iova_in_mbuf"
> instead of "pmd_supports_disable_iova_as_pa" with an opposite meaning.
> By default, it is assumed that drivers require the IOVA field in mbuf.
> The drivers which support removing this field have to declare themselves.
> 
> If the option "enable_iova_as_pa" is disabled, the unsupported drivers
> will be listed with the new reason text "requires IOVA in mbuf".
> 
> Suggested-by: Bruce Richardson <bruce.richardson@intel.com>
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---

Reviewed-by: Bruce Richardson <bruce.richardson@intel.com>

Couple of comments inline below.

>  app/test/test_mbuf.c                   |  2 +-
<snip>

> @@ -127,9 +127,9 @@ foreach subpath:subdirs
>              # pull in driver directory which should update all the local variables
>              subdir(drv_path)
>  
> -            if dpdk_conf.get('RTE_IOVA_AS_PA') == 0 and not pmd_supports_disable_iova_as_pa and not always_enable.contains(drv_path)
> +            if not get_option('enable_iova_as_pa') and require_iova_in_mbuf and not always_enable.contains(drv_path)

I don't particularly like the always_enable check at the end. If a driver
is set to always-enable and it doesn't support the configured mode, we
should throw an error immediately IMHO, rather than silently continuing to
build the driver.

>                  build = false
> -                reason = 'driver does not support disabling IOVA as PA mode'
> +                reason = 'requires IOVA in mbuf'
>              endif

I think the reason given is more accurate, but for those users not familiar
with the internals of DPDK builds it could be confusing. I think the
message needs to reference the 'enable_iova_as_pa' option. How about?

	reason = 'requires IOVA in mbuf (set enable_iova_as_pa option)'

/Bruce