[2/3] net/i40e: add changes to support i40e PMD on windows
Checks
Commit Message
Adding build changes to compile i40e PMD on windows.
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(-)
Comments
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.
05/12/2020 02:10, Pallavi Kadam:
> --- a/drivers/net/meson.build
> +++ b/drivers/net/meson.build
> @@ -1,9 +1,6 @@
> # SPDX-License-Identifier: BSD-3-Clause
> # Copyright(c) 2017 Intel Corporation
>
> -if is_windows
> - subdir_done()
> -endif
>
> drivers = ['af_packet',
> 'af_xdp',
> @@ -56,6 +53,12 @@ drivers = ['af_packet',
> 'virtio',
> 'vmxnet3',
> ]
> +
> +if is_windows
> + drivers = ['i40e',
> + ]
> +endif
Let's not add an alternative list please.
I prefer disabling compilation in other drivers.
> --- a/lib/librte_eal/common/meson.build
> +++ b/lib/librte_eal/common/meson.build
> @@ -33,6 +33,7 @@ if is_windows
> 'malloc_heap.c',
> 'rte_malloc.c',
> 'eal_common_timer.c',
> + 'rte_random.c',
> 'rte_service.c',
> )
> subdir_done()
> diff --git a/lib/librte_eal/rte_eal_exports.def b/lib/librte_eal/rte_eal_exports.def
> index 89166acd7..428201872 100644
> --- a/lib/librte_eal/rte_eal_exports.def
> +++ b/lib/librte_eal/rte_eal_exports.def
> @@ -124,6 +124,7 @@ EXPORTS
> rte_memzone_reserve_bounded
> rte_memzone_walk
> rte_openlog_stream
> + rte_rand
> rte_realloc
> rte_rtm_supported
> rte_service_attr_get
> 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
These changes are not specific to i40e, please separate.
On Sun, Dec 06, 2020 at 04:49:40PM +0100, Thomas Monjalon wrote:
> 05/12/2020 02:10, Pallavi Kadam:
> > --- a/drivers/net/meson.build
> > +++ b/drivers/net/meson.build
> > @@ -1,9 +1,6 @@
> > # SPDX-License-Identifier: BSD-3-Clause
> > # Copyright(c) 2017 Intel Corporation
> >
> > -if is_windows
> > - subdir_done()
> > -endif
> >
> > drivers = ['af_packet',
> > 'af_xdp',
> > @@ -56,6 +53,12 @@ drivers = ['af_packet',
> > 'virtio',
> > 'vmxnet3',
> > ]
> > +
> > +if is_windows
> > + drivers = ['i40e',
> > + ]
> > +endif
>
> Let's not add an alternative list please.
> I prefer disabling compilation in other drivers.
>
+1 for this. It's more work, but it keeps the existing style where all info
about whether a driver should be built, and how it's built in a single file
for each driver.
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.
On 12/7/2020 1:14 AM, Bruce Richardson wrote:
> On Sun, Dec 06, 2020 at 04:49:40PM +0100, Thomas Monjalon wrote:
>> 05/12/2020 02:10, Pallavi Kadam:
>>> --- a/drivers/net/meson.build
>>> +++ b/drivers/net/meson.build
>>> @@ -1,9 +1,6 @@
>>> # SPDX-License-Identifier: BSD-3-Clause
>>> # Copyright(c) 2017 Intel Corporation
>>>
>>> -if is_windows
>>> - subdir_done()
>>> -endif
>>>
>>> drivers = ['af_packet',
>>> 'af_xdp',
>>> @@ -56,6 +53,12 @@ drivers = ['af_packet',
>>> 'virtio',
>>> 'vmxnet3',
>>> ]
>>> +
>>> +if is_windows
>>> + drivers = ['i40e',
>>> + ]
>>> +endif
>> Let's not add an alternative list please.
>> I prefer disabling compilation in other drivers.
>>
> +1 for this. It's more work, but it keeps the existing style where all info
> about whether a driver should be built, and how it's built in a single file
> for each driver.
Thanks Thomas and Bruce.
Will incorporate this change in v2.
On 12/6/2020 7:49 AM, Thomas Monjalon wrote:
> 05/12/2020 02:10, Pallavi Kadam:
>> --- a/drivers/net/meson.build
>> +++ b/drivers/net/meson.build
>> @@ -1,9 +1,6 @@
>> # SPDX-License-Identifier: BSD-3-Clause
>> # Copyright(c) 2017 Intel Corporation
>>
>> -if is_windows
>> - subdir_done()
>> -endif
>>
>> drivers = ['af_packet',
>> 'af_xdp',
>> @@ -56,6 +53,12 @@ drivers = ['af_packet',
>> 'virtio',
>> 'vmxnet3',
>> ]
>> +
>> +if is_windows
>> + drivers = ['i40e',
>> + ]
>> +endif
> Let's not add an alternative list please.
> I prefer disabling compilation in other drivers.
>
>
>> --- a/lib/librte_eal/common/meson.build
>> +++ b/lib/librte_eal/common/meson.build
>> @@ -33,6 +33,7 @@ if is_windows
>> 'malloc_heap.c',
>> 'rte_malloc.c',
>> 'eal_common_timer.c',
>> + 'rte_random.c',
>> 'rte_service.c',
>> )
>> subdir_done()
>> diff --git a/lib/librte_eal/rte_eal_exports.def b/lib/librte_eal/rte_eal_exports.def
>> index 89166acd7..428201872 100644
>> --- a/lib/librte_eal/rte_eal_exports.def
>> +++ b/lib/librte_eal/rte_eal_exports.def
>> @@ -124,6 +124,7 @@ EXPORTS
>> rte_memzone_reserve_bounded
>> rte_memzone_walk
>> rte_openlog_stream
>> + rte_rand
>> rte_realloc
>> rte_rtm_supported
>> rte_service_attr_get
>> 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
>
> These changes are not specific to i40e, please separate.
Ok, will create a separate patch in v2.
This change is required once we add rte_random.c file on windows.
So, may be addition of rte_random.c file and this change should go together?
Please suggest. Thanks,
>
>
09/12/2020 01:21, Kadam, Pallavi:
> On 12/6/2020 7:49 AM, Thomas Monjalon wrote:
> > 05/12/2020 02:10, Pallavi Kadam:
> >> --- 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
> >
> > These changes are not specific to i40e, please separate.
> Ok, will create a separate patch in v2.
> This change is required once we add rte_random.c file on windows.
> So, may be addition of rte_random.c file and this change should go together?
> Please suggest. Thanks,
Yes absolutely, addition of random feature should be atomic in a patch.
On 12/9/2020 12:59 AM, Thomas Monjalon wrote:
> 09/12/2020 01:21, Kadam, Pallavi:
>> On 12/6/2020 7:49 AM, Thomas Monjalon wrote:
>>> 05/12/2020 02:10, Pallavi Kadam:
>>>> --- 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
>>> These changes are not specific to i40e, please separate.
>> Ok, will create a separate patch in v2.
>> This change is required once we add rte_random.c file on windows.
>> So, may be addition of rte_random.c file and this change should go together?
>> Please suggest. Thanks,
> Yes absolutely, addition of random feature should be atomic in a patch.
Ok. Created a separate patch in v2. Thanks!
>
>
@@ -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
#define FIELD_SIZEOF(t, f) (sizeof(((t*)0)->f))
@@ -1495,7 +1495,8 @@ i40evf_handle_aq_msg(struct rte_eth_dev *dev)
info.msg_len);
else {
/* read message and it's expected one */
- if (msg_opc == vf->pend_cmd) {
+ if ((volatile uint32_t)msg_opc ==
+ vf->pend_cmd) {
vf->cmd_retval = msg_ret;
/* prevent compiler reordering */
rte_compiler_barrier();
@@ -12,7 +12,9 @@
#include "i40e_rxtx.h"
#include "i40e_rxtx_vec_common.h"
+#ifndef RTE_EXEC_ENV_WINDOWS
#include <x86intrin.h>
+#endif
#ifndef __INTEL_COMPILER
#pragma GCC diagnostic ignored "-Wcast-qual"
@@ -554,7 +554,7 @@ i40e_node_add(struct rte_eth_dev *dev, uint32_t node_id,
}
/* check level */
if (level_id != RTE_TM_NODE_LEVEL_ID_ANY &&
- level_id != parent_node_type + 1) {
+ level_id != (uint32_t)parent_node_type + 1) {
error->type = RTE_TM_ERROR_TYPE_NODE_PARAMS;
error->message = "Wrong level";
return -EINVAL;
@@ -1,9 +1,6 @@
# SPDX-License-Identifier: BSD-3-Clause
# Copyright(c) 2017 Intel Corporation
-if is_windows
- subdir_done()
-endif
drivers = ['af_packet',
'af_xdp',
@@ -56,6 +53,12 @@ drivers = ['af_packet',
'virtio',
'vmxnet3',
]
+
+if is_windows
+ drivers = ['i40e',
+ ]
+endif
+
std_deps = ['ethdev', 'kvargs'] # 'ethdev' also pulls in mbuf, net, eal etc
std_deps += ['bus_pci'] # very many PMDs depend on PCI, so make std
std_deps += ['bus_vdev'] # same with vdev bus
@@ -33,6 +33,7 @@ if is_windows
'malloc_heap.c',
'rte_malloc.c',
'eal_common_timer.c',
+ 'rte_random.c',
'rte_service.c',
)
subdir_done()
@@ -124,6 +124,7 @@ EXPORTS
rte_memzone_reserve_bounded
rte_memzone_walk
rte_openlog_stream
+ rte_rand
rte_realloc
rte_rtm_supported
rte_service_attr_get
@@ -18,6 +18,11 @@
#define WIN32_LEAN_AND_MEAN
#endif
+#ifdef __clang__
+#undef _m_prefetchw
+#define _m_prefetchw __m_prefetchw
+#endif
+
/* Must come first. */
#include <windows.h>