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

["Kadam, Pallavi" <pallavi.kadam@intel.com>]

On 12/5/2020 5:52 AM, Dmitry Kozlyuk wrote:
> 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).

Ok, I'll update the title and description in v2.

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

Thanks for the suggestion. Will first #undef min and max in the same file in v2.

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

Ok, will  #include <rte_vect.h> instead.

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

Sure, will explain it in v2.

> Can't x86intrin.h be replaced by rte_vect.h in rte_random.c?
Please correct if I am wrong. Here, cause of an error is indirectly due 
to rte_windows.h
and not because of #include <x86intrin.h>

Please see below error:

FAILED: lib/76b5a35@@rte_eal@sta/librte_eal_common_rte_random.c.obj
clang @lib/76b5a35@@rte_eal@sta/librte_eal_common_rte_random.c.obj.rsp
In file included from ../lib/librte_eal/common/rte_random.c:13:
In file included from ..\lib/librte_eal/include\rte_eal.h:20:
In file included from ..\lib/librte_eal/include\rte_per_lcore.h:25:
In file included from ..\lib/librte_eal/windows/include\pthread.h:21:
In file included from ..\lib/librte_eal/windows/include\rte_windows.h:27:
In file included from C:\Program Files (x86)\Windows 
Kits\10\include\10.0.18362.0\um\windows.h:171:
In file included from C:\Program Files (x86)\Windows 
Kits\10\include\10.0.18362.0\shared\windef.h:24:
In file included from C:\Program Files (x86)\Windows 
Kits\10\include\10.0.18362.0\shared\minwindef.h:182:
C:\Program Files (x86)\Windows 
Kits\10\include\10.0.18362.0\um\winnt.h:3324:1: error: conflicting types 
for '_m_prefetchw'
_m_prefetchw (
^
C:\Program Files\LLVM\lib\clang\10.0.0\include\prfchwintrin.h:50:1: 
note: previous definition is here
_m_prefetchw(void *__P)
^
1 error generated.


> If it's still necessary, __clang__ should be RTE_TOOLCHAIN_CLANG.

will update this in v2. Thank you.