Re: [PATCH] common/cnxk: allow enabling IOVA field in mbuf

[Shijith Thotton <sthotton@marvell.com>]

>> >> >
>> >> > Value of RTE_IOVA_IN_MBUF was always disabled on cnxk platforms, as
>> >IOVA
>> >> > in the mbuf is not required. This change modifies that behavior,
>> >> > allowing RTE_IOVA_IN_MBUF to be enabled if the build option
>> >> > -Denable_iova_as_pa=true is explicitly specified.
>> >> >
>> >> > Signed-off-by: Shijith Thotton <sthotton@marvell.com>
>> >> > ---
>> >> >
>> >> > diff --git a/config/arm/meson.build b/config/arm/meson.build
>> >> > index 012935d5d7..ca54524376 100644
>> >> > --- a/config/arm/meson.build
>> >> > +++ b/config/arm/meson.build
>> >> > @@ -439,10 +439,7 @@ soc_cn9k = {
>> >> >      'description': 'Marvell OCTEON 9',
>> >> >      'implementer': '0x43',
>> >> >      'part_number': '0xb2',
>> >> > -    'numa': false,
>> >> > -    'flags': [
>> >> > -        ['RTE_IOVA_IN_MBUF', 0]
>> >> > -    ]
>> >> > +    'numa': false
>> >> >  }
>> >> >
>> >> >  soc_cn10k = {
>> >> > @@ -451,8 +448,7 @@ soc_cn10k = {
>> >> >      'flags': [
>> >> >          ['RTE_MAX_LCORE', 24],
>> >> >          ['RTE_MAX_NUMA_NODES', 1],
>> >> > -        ['RTE_MEMPOOL_ALIGN', 128],
>> >> > -        ['RTE_IOVA_IN_MBUF', 0]
>> >> > +        ['RTE_MEMPOOL_ALIGN', 128]
>> >> >      ],
>> >> >      'part_number': '0xd49',
>> >> >      'extra_march_features': ['crypto'],
>> >> > diff --git a/drivers/common/cnxk/meson.build
>> >b/drivers/common/cnxk/meson.build
>> >> > index dc2ddf1f20..bba780e750 100644
>> >> > --- a/drivers/common/cnxk/meson.build
>> >> > +++ b/drivers/common/cnxk/meson.build
>> >> > @@ -108,4 +108,13 @@ deps += ['bus_pci', 'net', 'telemetry']
>> >> >
>> >> >  require_iova_in_mbuf = false
>> >> >
>> >> > +cnxk_socs = ['cn9k', 'cn10k', 'cn20k']
>> >> > +
>> >> > +# Enable RTE_IOVA_IN_MBUF only if enable_iova_as_pa is set explicitly,
>> >else disable it
>> >> > +if meson.version().version_compare('>=1.1.0')
>> >> > +    if '-Denable_iova_as_pa' not in meson.build_options() and soc_type
>in
>> >cnxk_socs
>> >> > +        dpdk_conf.set10('RTE_IOVA_IN_MBUF', false)
>> >> > +    endif
>> >> > +endif
>> >>
>> >> Since this is added in driver/common/cnxk, it will be late to decide.
>> >> For example,
>> >>
>> >> Following PMDs will have mis match:
>> >> common - cpt, dpaax, idpf, ionic
>> >> bus - cdx, dpaa, fslmc, ifpga, uacce
>> >>
>> >> I think, this check needs to move up in the chain. @Richardson, Bruce
>> >> Any thoughts on cleanly adding this kind of check in top-level meson
>> >> objects?
>> >>
>> >Can you explain what you mean by a mismatch?
>> >
>> >Can I assume that the common/cnxk is processed before any of these other
>> >drivers? If so, then whatever values or variables set by that meson.build
>> >file can be queried by all the others.
>> >
>>
>> The goal is to change the default value of enable_iova_as_pa to false on
>> CNXK platforms.
>>
>> Jerin pointed out the following issue with the current patch, which causes a
>> mismatch during the default build (enable_iova_as_pa not specified):
>>
>> 1. The configuration initially sets RTE_IOVA_IN_MBUF to 1.
>> 2. Meson proceeds through the lib and driver directories, enabling the
>>     build of drivers that require IOVA.
>> 3. When Meson reaches the common/cnxk folder, it sets RTE_IOVA_IN_MBUF
>>      to 0.
>> 4. The remaining configuration then disables the build of drivers that depend
>>      on IOVA.
>>
>> The problem arises due to the extra PMDs being enabled during step 2.
>>
>> We could avoid this issue by moving the check added in this patch to a
>> top-level Meson file, such as config/arm/meson.build.
>>
>> We would like your feedback on this approach or suggestions for a better
>> alternative.
>>
>Two alternatives I see:
>
>1. One, you document that for CNXK platforms that the value must be set to
>false in the build configuration. Then when processing common/cnxk folder,
>you error out the configuration if the value is set to true, giving the
>user a proper error message.
>2. You see about changing the value as you suggest in a config meson.build
>file.
>
>Of the 2, option 1 would be by far the most preferred option. If an option
>is set by the user to X, the build system should not override it to Y. This
>is why meson does not provide a set_option() function, only a get_option()
>one.
>

Understood. We’ll proceed with option 1. Instead of throwing an error, we’ll
display a warning when processing the common/cnxk folder if IOVA is enabled.
This way, the user can choose whether to disable IOVA in the mbuf.

Thanks,
Shijith