Re: [dpdk-dev] [PATCH 2/3] net/i40e: add changes to support i40e PMD on windows

[Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>]

On Fri,  4 Dec 2020 17:10:19 -0800, Pallavi Kadam wrote:

You could drop "add changes" and "i40e PMD" from subject line, as any commit
changes something and topic is "net/i40e" already.

> Adding build changes to compile i40e PMD on windows.

This is redundant given the commit subject.
Please use present simple tense for changes description (this applies to
sibling patches).

> Disabling few warnings with Clang such as comparison of integers of
> different signs and macro redefinitions.
> Also, adding linking dependency source file rte_random.c file to
> Windows.
> 
> Signed-off-by: Pallavi Kadam <pallavi.kadam@intel.com>
> Reviewed-by: Ranjit Menon <ranjit.menon@intel.com>
> ---
>  drivers/net/i40e/base/i40e_osdep.h           | 3 +++
>  drivers/net/i40e/i40e_ethdev_vf.c            | 3 ++-
>  drivers/net/i40e/i40e_rxtx_vec_avx2.c        | 2 ++
>  drivers/net/i40e/i40e_tm.c                   | 2 +-
>  drivers/net/meson.build                      | 9 ++++++---
>  lib/librte_eal/common/meson.build            | 1 +
>  lib/librte_eal/rte_eal_exports.def           | 1 +
>  lib/librte_eal/windows/include/rte_windows.h | 5 +++++
>  8 files changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/i40e/base/i40e_osdep.h b/drivers/net/i40e/base/i40e_osdep.h
> index 9b5033024..fa22df122 100644
> --- a/drivers/net/i40e/base/i40e_osdep.h
> +++ b/drivers/net/i40e/base/i40e_osdep.h
> @@ -67,8 +67,11 @@ typedef enum i40e_status_code i40e_status;
>  #define false           0
>  #define true            1
>  
> +/* Avoid macro redifinition warning on Windows */
> +#ifndef RTE_EXEC_ENV_WINDOWS
>  #define min(a,b) RTE_MIN(a,b)
>  #define max(a,b) RTE_MAX(a,b)
> +#endif

Windows min() and max() macros evaluate arguments twice [1], which can be
unacceptable in driver code if used with MMIO. Better #undef min and max
first, then let PMD define them.

It seems we'll have to do that for many PMDs, because rte_os.h must not erase
platform-specific macros, and rte_windows.h is not for generic PMDs.

[1]: https://docs.microsoft.com/en-us/windows/win32/multimedia/max

> diff --git a/drivers/net/i40e/i40e_rxtx_vec_avx2.c b/drivers/net/i40e/i40e_rxtx_vec_avx2.c
> index 7a558fc73..cf2dc88c5 100644
> --- a/drivers/net/i40e/i40e_rxtx_vec_avx2.c
> +++ b/drivers/net/i40e/i40e_rxtx_vec_avx2.c
> @@ -12,7 +12,9 @@
>  #include "i40e_rxtx.h"
>  #include "i40e_rxtx_vec_common.h"
>  
> +#ifndef RTE_EXEC_ENV_WINDOWS
>  #include <x86intrin.h>
> +#endif

Just #include <rte_vect.h>, it takes care of x86intrin.h for Windows.

> diff --git a/lib/librte_eal/windows/include/rte_windows.h b/lib/librte_eal/windows/include/rte_windows.h
> index b82af34f6..822922c11 100644
> --- a/lib/librte_eal/windows/include/rte_windows.h
> +++ b/lib/librte_eal/windows/include/rte_windows.h
> @@ -18,6 +18,11 @@
>  #define WIN32_LEAN_AND_MEAN
>  #endif
>  
> +#ifdef __clang__
> +#undef _m_prefetchw
> +#define _m_prefetchw __m_prefetchw
> +#endif
> +

Please explain in the commit message which problem this solves.
Can't x86intrin.h be replaced by rte_vect.h in rte_random.c?
If it's still necessary, __clang__ should be RTE_TOOLCHAIN_CLANG.