Message ID | 20201205011020.6276-3-pallavi.kadam@intel.com (mailing list archive) |
---|---|
State | Superseded, archived |
Delegated to: | Thomas Monjalon |
Headers | show |
Series | Support i40e PMD on Windows | expand |
Context | Check | Description |
---|---|---|
ci/checkpatch | warning | coding style issues |
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! > >
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 #define FIELD_SIZEOF(t, f) (sizeof(((t*)0)->f)) diff --git a/drivers/net/i40e/i40e_ethdev_vf.c b/drivers/net/i40e/i40e_ethdev_vf.c index c26b036b8..daf4a3f99 100644 --- a/drivers/net/i40e/i40e_ethdev_vf.c +++ b/drivers/net/i40e/i40e_ethdev_vf.c @@ -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(); 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 #ifndef __INTEL_COMPILER #pragma GCC diagnostic ignored "-Wcast-qual" diff --git a/drivers/net/i40e/i40e_tm.c b/drivers/net/i40e/i40e_tm.c index 5d722f92c..cab296e1a 100644 --- a/drivers/net/i40e/i40e_tm.c +++ b/drivers/net/i40e/i40e_tm.c @@ -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; diff --git a/drivers/net/meson.build b/drivers/net/meson.build index 29f477750..b3bc0277e 100644 --- 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 + 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 diff --git a/lib/librte_eal/common/meson.build b/lib/librte_eal/common/meson.build index 39abf7a0a..98e8fffd4 100644 --- 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 + /* Must come first. */ #include <windows.h>