[v2,1/6] eal: eal stub to add windows support
diff mbox series

Message ID 20190306041634.12976-2-anand.rawat@intel.com
State Superseded, archived
Delegated to: Thomas Monjalon
Headers show
Series
  • HelloWorld example for windows
Related show

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/intel-Performance-Testing success Performance Testing PASS
ci/mellanox-Performance-Testing success Performance Testing PASS
ci/Intel-compilation success Compilation OK

Commit Message

Anand Rawat March 6, 2019, 4:16 a.m. UTC
Added initial stub source files for windows support and meson
changes to build them.

Signed-off-by: Anand Rawat <anand.rawat@intel.com>
Signed-off-by: Pallavi Kadam <pallavi.kadam@intel.com>
Reviewed-by: Jeff Shaw <jeffrey.b.shaw@intel.com>
Reviewed-by: Ranjit Menon <ranjit.menon@intel.com>
---
 config/meson.build                      | 22 ++++---
 config/x86/meson.build                  | 14 +++--
 lib/librte_eal/common/meson.build       | 84 ++++++++++++++-----------
 lib/librte_eal/meson.build              | 10 ++-
 lib/librte_eal/windows/eal/eal.c        | 11 ++++
 lib/librte_eal/windows/eal/eal_debug.c  | 12 ++++
 lib/librte_eal/windows/eal/eal_lcore.c  | 26 ++++++++
 lib/librte_eal/windows/eal/eal_thread.c | 15 +++++
 lib/librte_eal/windows/eal/meson.build  | 10 +++
 lib/meson.build                         |  6 +-
 meson.build                             | 34 +++++-----
 11 files changed, 176 insertions(+), 68 deletions(-)
 create mode 100644 lib/librte_eal/windows/eal/eal.c
 create mode 100644 lib/librte_eal/windows/eal/eal_debug.c
 create mode 100644 lib/librte_eal/windows/eal/eal_lcore.c
 create mode 100644 lib/librte_eal/windows/eal/eal_thread.c
 create mode 100644 lib/librte_eal/windows/eal/meson.build

Comments

Thomas Monjalon March 6, 2019, 10:03 a.m. UTC | #1
Hi,

06/03/2019 05:16, Anand Rawat:
> -# some libs depend on maths lib
> -add_project_link_arguments('-lm', language: 'c')
> -dpdk_extra_ldflags += '-lm'
> +if cc.find_library('lm', required : false).found()
> +	# some libs depend on maths lib
> +	add_project_link_arguments('-lm', language: 'c')
> +	dpdk_extra_ldflags += '-lm'
> +endif

Either libmath is required or not.
I don't think it can be optional.
Why is it changed?

>  # get binutils version for the workaround of Bug 97
> -ldver = run_command('ld', '-v').stdout().strip()
> -if ldver.contains('2.30')
> -	if cc.has_argument('-mno-avx512f')
> -		march_opt += '-mno-avx512f'
> -		message('Binutils 2.30 detected, disabling AVX512 support as workaround for bug #97')
> +if host_machine.system() != 'windows'
> +	ldver = run_command('ld', '-v').stdout().strip()
> +	if ldver.contains('2.30')
> +		if cc.has_argument('-mno-avx512f')
> +			march_opt += '-mno-avx512f'
> +			message('Binutils 2.30 detected, disabling AVX512 support as workaround for bug #97')
> +		endif
>  	endif
>  endif

Same comment as in v1, you should check which linker is used.
This check is only for GNU ld 2.30.

> +if host_machine.system() != 'windows'
> +	common_sources = files(

The definitive solution should be to compile all common EAL files.
Please explain what are the issues in the common files.
I think we should not remove them and fix them one by one.
You could provide a separate patch to skip some files for
making helloworld working.

> -subdir(join_paths('arch', arch_subdir))
> +
> +common_headers += files('include/rte_common.h')
> +if host_machine.system() != 'windows'
> +	subdir(join_paths('arch', arch_subdir))
> +endif

Same comment as above, it should not be changed.

> -common_headers = files(
> +common_headers += files(
>  	'include/rte_alarm.h',
>  	'include/rte_branch_prediction.h',
>  	'include/rte_bus.h',
>  	'include/rte_bitmap.h',
>  	'include/rte_class.h',
> -	'include/rte_common.h',

Why rte_common.h is moved first in the list?

> -deps += 'kvargs'
> +if host_machine.system() != 'windows'
> +	deps += 'kvargs'
> +endif

Why kvargs is removed?

> --- /dev/null
> +++ b/lib/librte_eal/windows/eal/eal_lcore.c
> + /* Get the cpu core id value */
> +unsigned int
> +eal_cpu_core_id(unsigned int lcore_id)
> +{
> +	return lcore_id;
> +}

For this function and the others in this file,
please add a comment explaining this is a stub, not the expected result.
You can add a TODO or FIXME tag.

> --- a/lib/meson.build
> +++ b/lib/meson.build
> +if host_machine.system() == 'windows'
> +	libraries = ['eal'] # override libraries for windows
> +endif

Instead of simply "override", you could explain that the other libraries
are not yet supported on Windows.

> --- a/meson.build
> +++ b/meson.build
> -# build libs and drivers
> +# build libs
>  subdir('lib')
> -subdir('buildtools')
> -subdir('drivers')
>  
> -# build binaries and installable tools
> -subdir('usertools')
> -subdir('app')
> +if host_machine.system() != 'windows'
> +	# build buildtools and drivers
> +	subdir('buildtools')
> +	subdir('drivers')
>  
> -# build docs
> -subdir('doc')
> +	# build binaries and installable tools
> +	subdir('usertools')
> +	subdir('app')
> +	subdir('test')
> +
> +	# build kernel modules if enabled
> +	if get_option('enable_kmods')
> +		subdir('kernel')
> +	endif
> +
> +	# build docs
> +	subdir('doc')
> +endif

I don't like modifying this file.
Can we skip not supported directories inside the sub meson files?
Bruce Richardson March 6, 2019, 11:20 a.m. UTC | #2
On Wed, Mar 06, 2019 at 11:03:24AM +0100, Thomas Monjalon wrote:
> Hi,
> 
> 06/03/2019 05:16, Anand Rawat:
> > -# some libs depend on maths lib
> > -add_project_link_arguments('-lm', language: 'c')
> > -dpdk_extra_ldflags += '-lm'
> > +if cc.find_library('lm', required : false).found()
> > +	# some libs depend on maths lib
> > +	add_project_link_arguments('-lm', language: 'c')
> > +	dpdk_extra_ldflags += '-lm'
> > +endif
> 
> Either libmath is required or not.
> I don't think it can be optional.
> Why is it changed?
> 

I think these come as part of libc, it's just on Linux they are not in the
main libc library but need to be linked in separately.

https://en.wikipedia.org/wiki/C_mathematical_functions#libm

Therefore, this looks the best way of dealing with this.

> >  # get binutils version for the workaround of Bug 97
> > -ldver = run_command('ld', '-v').stdout().strip()
> > -if ldver.contains('2.30')
> > -	if cc.has_argument('-mno-avx512f')
> > -		march_opt += '-mno-avx512f'
> > -		message('Binutils 2.30 detected, disabling AVX512 support as workaround for bug #97')
> > +if host_machine.system() != 'windows'
> > +	ldver = run_command('ld', '-v').stdout().strip()
> > +	if ldver.contains('2.30')
> > +		if cc.has_argument('-mno-avx512f')
> > +			march_opt += '-mno-avx512f'
> > +			message('Binutils 2.30 detected, disabling AVX512 support as workaround for bug #97')
> > +		endif
> >  	endif
> >  endif
> 
> Same comment as in v1, you should check which linker is used.
> This check is only for GNU ld 2.30.
> 

I'm not aware of any better way to implement this in meson right now,
sadly. Through the compiler object we can get information about linker
flags, but not about the specific linker itself, since it is only called
through the compiler.

> > +if host_machine.system() != 'windows'
> > +	common_sources = files(
> 
> The definitive solution should be to compile all common EAL files.
> Please explain what are the issues in the common files.
> I think we should not remove them and fix them one by one.
> You could provide a separate patch to skip some files for
> making helloworld working.
> 

I believe that is exactly what this patch is trying to do - it's skipping
the files unneeded to get helloworld working, and the intention is to fix
them one by one and add them back in later. Perhaps this sort of change
should be a separate (precursor) patch where the cover letter can call this
out explicitly?

> > -subdir(join_paths('arch', arch_subdir))
> > +
> > +common_headers += files('include/rte_common.h')
> > +if host_machine.system() != 'windows'
> > +	subdir(join_paths('arch', arch_subdir))
> > +endif
> 
> Same comment as above, it should not be changed.
> 
> > -common_headers = files(
> > +common_headers += files(
> >  	'include/rte_alarm.h',
> >  	'include/rte_branch_prediction.h',
> >  	'include/rte_bus.h',
> >  	'include/rte_bitmap.h',
> >  	'include/rte_class.h',
> > -	'include/rte_common.h',
> 
> Why rte_common.h is moved first in the list?
> 
> > -deps += 'kvargs'
> > +if host_machine.system() != 'windows'
> > +	deps += 'kvargs'
> > +endif
> 
> Why kvargs is removed?
> 

Again, I believe these actions are to disable the parts of DPDK that are
not needed to enable helloworld, allowing later patches to come in and fix
them.

> > --- /dev/null
> > +++ b/lib/librte_eal/windows/eal/eal_lcore.c
> > + /* Get the cpu core id value */
> > +unsigned int
> > +eal_cpu_core_id(unsigned int lcore_id)
> > +{
> > +	return lcore_id;
> > +}
> 
> For this function and the others in this file,
> please add a comment explaining this is a stub, not the expected result.
> You can add a TODO or FIXME tag.
> 
> > --- a/lib/meson.build
> > +++ b/lib/meson.build
> > +if host_machine.system() == 'windows'
> > +	libraries = ['eal'] # override libraries for windows
> > +endif
> 
> Instead of simply "override", you could explain that the other libraries
> are not yet supported on Windows.
> 
> > --- a/meson.build
> > +++ b/meson.build
> > -# build libs and drivers
> > +# build libs
> >  subdir('lib')
> > -subdir('buildtools')
> > -subdir('drivers')
> >  
> > -# build binaries and installable tools
> > -subdir('usertools')
> > -subdir('app')
> > +if host_machine.system() != 'windows'
> > +	# build buildtools and drivers
> > +	subdir('buildtools')
> > +	subdir('drivers')
> >  
> > -# build docs
> > -subdir('doc')
> > +	# build binaries and installable tools
> > +	subdir('usertools')
> > +	subdir('app')
> > +	subdir('test')
> > +
> > +	# build kernel modules if enabled
> > +	if get_option('enable_kmods')
> > +		subdir('kernel')
> > +	endif
> > +
> > +	# build docs
> > +	subdir('doc')
> > +endif
> 
> I don't like modifying this file.
> Can we skip not supported directories inside the sub meson files?
> 
Since we now mandate meson 0.47 we can use the "subdir_done()" function to
this.

/Bruce
Thomas Monjalon March 6, 2019, 11:36 a.m. UTC | #3
06/03/2019 12:20, Bruce Richardson:
> On Wed, Mar 06, 2019 at 11:03:24AM +0100, Thomas Monjalon wrote:
> > Hi,
> > 
> > 06/03/2019 05:16, Anand Rawat:
> > > -# some libs depend on maths lib
> > > -add_project_link_arguments('-lm', language: 'c')
> > > -dpdk_extra_ldflags += '-lm'
> > > +if cc.find_library('lm', required : false).found()
> > > +	# some libs depend on maths lib
> > > +	add_project_link_arguments('-lm', language: 'c')
> > > +	dpdk_extra_ldflags += '-lm'
> > > +endif
> > 
> > Either libmath is required or not.
> > I don't think it can be optional.
> > Why is it changed?
> > 
> 
> I think these come as part of libc, it's just on Linux they are not in the
> main libc library but need to be linked in separately.
> 
> https://en.wikipedia.org/wiki/C_mathematical_functions#libm
> 
> Therefore, this looks the best way of dealing with this.

If it's the only solution, at least it deserves a comment.

> > > +if host_machine.system() != 'windows'
> > > +	common_sources = files(
> > 
> > The definitive solution should be to compile all common EAL files.
> > Please explain what are the issues in the common files.
> > I think we should not remove them and fix them one by one.
> > You could provide a separate patch to skip some files for
> > making helloworld working.
> > 
> 
> I believe that is exactly what this patch is trying to do - it's skipping
> the files unneeded to get helloworld working, and the intention is to fix
> them one by one and add them back in later. Perhaps this sort of change
> should be a separate (precursor) patch where the cover letter can call this
> out explicitly?
> 
> > > -deps += 'kvargs'
> > > +if host_machine.system() != 'windows'
> > > +	deps += 'kvargs'
> > > +endif
> > 
> > Why kvargs is removed?
> 
> Again, I believe these actions are to disable the parts of DPDK that are
> not needed to enable helloworld, allowing later patches to come in and fix
> them.

They are workarounds to build helloworld.
It is good to have progress in the draft tree,
but I see no point in merging this in master.
I think we should separate patches which are doing definitive changes
from temporary workaround patches disabling some files.
It is not an issue to merge some patches for Windows which are not compiling.
Bruce Richardson March 6, 2019, 11:52 a.m. UTC | #4
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Wednesday, March 6, 2019 11:36 AM
> To: Richardson, Bruce <bruce.richardson@intel.com>
> Cc: dev@dpdk.org; Rawat, Anand <anand.rawat@intel.com>; Kadam, Pallavi
> <pallavi.kadam@intel.com>; Menon, Ranjit <ranjit.menon@intel.com>; Shaw,
> Jeffrey B <jeffrey.b.shaw@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v2 1/6] eal: eal stub to add windows
> support
> 
> 06/03/2019 12:20, Bruce Richardson:
> > On Wed, Mar 06, 2019 at 11:03:24AM +0100, Thomas Monjalon wrote:
> > > Hi,
> > >
> > > 06/03/2019 05:16, Anand Rawat:
> > > > -# some libs depend on maths lib
> > > > -add_project_link_arguments('-lm', language: 'c')
> > > > -dpdk_extra_ldflags += '-lm'
> > > > +if cc.find_library('lm', required : false).found()
> > > > +	# some libs depend on maths lib
> > > > +	add_project_link_arguments('-lm', language: 'c')
> > > > +	dpdk_extra_ldflags += '-lm'
> > > > +endif
> > >
> > > Either libmath is required or not.
> > > I don't think it can be optional.
> > > Why is it changed?
> > >
> >
> > I think these come as part of libc, it's just on Linux they are not in
> > the main libc library but need to be linked in separately.
> >
> > https://en.wikipedia.org/wiki/C_mathematical_functions#libm
> >
> > Therefore, this looks the best way of dealing with this.
> 
> If it's the only solution, at least it deserves a comment.

Yes. I'd suggest changing the existing comment rather than adding a new one. Update it to point out that on some OS's the maths functions are in a separate library.

> 
> > > > +if host_machine.system() != 'windows'
> > > > +	common_sources = files(
> > >
> > > The definitive solution should be to compile all common EAL files.
> > > Please explain what are the issues in the common files.
> > > I think we should not remove them and fix them one by one.
> > > You could provide a separate patch to skip some files for making
> > > helloworld working.
> > >
> >
> > I believe that is exactly what this patch is trying to do - it's
> > skipping the files unneeded to get helloworld working, and the
> > intention is to fix them one by one and add them back in later.
> > Perhaps this sort of change should be a separate (precursor) patch
> > where the cover letter can call this out explicitly?
> >
> > > > -deps += 'kvargs'
> > > > +if host_machine.system() != 'windows'
> > > > +	deps += 'kvargs'
> > > > +endif
> > >
> > > Why kvargs is removed?
> >
> > Again, I believe these actions are to disable the parts of DPDK that
> > are not needed to enable helloworld, allowing later patches to come in
> > and fix them.
> 
> They are workarounds to build helloworld.
> It is good to have progress in the draft tree, but I see no point in
> merging this in master.
> I think we should separate patches which are doing definitive changes from
> temporary workaround patches disabling some files.
> It is not an issue to merge some patches for Windows which are not
> compiling.
>
Anand Rawat March 7, 2019, 1:04 a.m. UTC | #5
On 3/6/2019 3:52 AM, Richardson, Bruce wrote:
> 
> 
>> -----Original Message-----
>> From: Thomas Monjalon [mailto:thomas@monjalon.net]
>> Sent: Wednesday, March 6, 2019 11:36 AM
>> To: Richardson, Bruce <bruce.richardson@intel.com>
>> Cc: dev@dpdk.org; Rawat, Anand <anand.rawat@intel.com>; Kadam, Pallavi
>> <pallavi.kadam@intel.com>; Menon, Ranjit <ranjit.menon@intel.com>; Shaw,
>> Jeffrey B <jeffrey.b.shaw@intel.com>
>> Subject: Re: [dpdk-dev] [PATCH v2 1/6] eal: eal stub to add windows
>> support
>>
>> 06/03/2019 12:20, Bruce Richardson:
>>> On Wed, Mar 06, 2019 at 11:03:24AM +0100, Thomas Monjalon wrote:
>>>> Hi,
>>>>
>>>> 06/03/2019 05:16, Anand Rawat:
>>>>> -# some libs depend on maths lib
>>>>> -add_project_link_arguments('-lm', language: 'c')
>>>>> -dpdk_extra_ldflags += '-lm'
>>>>> +if cc.find_library('lm', required : false).found()
>>>>> +       # some libs depend on maths lib
>>>>> +       add_project_link_arguments('-lm', language: 'c')
>>>>> +       dpdk_extra_ldflags += '-lm'
>>>>> +endif
>>>>
>>>> Either libmath is required or not.
>>>> I don't think it can be optional.
>>>> Why is it changed?
>>>>
>>>
>>> I think these come as part of libc, it's just on Linux they are not in
>>> the main libc library but need to be linked in separately.
>>>
>>> https://en.wikipedia.org/wiki/C_mathematical_functions#libm
>>>
>>> Therefore, this looks the best way of dealing with this.
>>
>> If it's the only solution, at least it deserves a comment.
> 
> Yes. I'd suggest changing the existing comment rather than adding a new one. Update it to point out that on some OS's the maths functions are in a separate library.

Bruce is right, both pthreads and math lib are not required for windows 
as the functionalities associated with them are a part of the standard 
library. Not having the check for pthread cause a warning at link time. 
I will update the comment in v3 as suggested.

> 
>>
>>>>> +if host_machine.system() != 'windows'
>>>>> +       common_sources = files(
>>>>
>>>> The definitive solution should be to compile all common EAL files.
>>>> Please explain what are the issues in the common files.
>>>> I think we should not remove them and fix them one by one.
>>>> You could provide a separate patch to skip some files for making
>>>> helloworld working.
>>>>
>>>
>>> I believe that is exactly what this patch is trying to do - it's
>>> skipping the files unneeded to get helloworld working, and the
>>> intention is to fix them one by one and add them back in later.
>>> Perhaps this sort of change should be a separate (precursor) patch
>>> where the cover letter can call this out explicitly?
>>>
>>>>> -deps += 'kvargs'
>>>>> +if host_machine.system() != 'windows'
>>>>> +       deps += 'kvargs'
>>>>> +endif
>>>>
>>>> Why kvargs is removed?
>>>
>>> Again, I believe these actions are to disable the parts of DPDK that
>>> are not needed to enable helloworld, allowing later patches to come in
>>> and fix them.
>>
>> They are workarounds to build helloworld.
>> It is good to have progress in the draft tree, but I see no point in
>> merging this in master.
>> I think we should separate patches which are doing definitive changes from
>> temporary workaround patches disabling some files.
>> It is not an issue to merge some patches for Windows which are not
>> compiling.
>>
> 

Bruce is right, we only compile required header and source files in 
order to avoid compatibility errors on windows. Without these
change helloworld on windows would fail to compile. Adding windows 
specific implementations of the common headers and sources would
bloat up individual patches as well the number of patches. kvargs is 
removed as a dependency to have minimum viable product for helloworld.
If required for lcore mask, it'll added back in v3.
Anand Rawat March 7, 2019, 1:19 a.m. UTC | #6
On 3/6/2019 3:20 AM, Richardson, Bruce wrote:
> On Wed, Mar 06, 2019 at 11:03:24AM +0100, Thomas Monjalon wrote:
> 
>>>   # get binutils version for the workaround of Bug 97
>>> -ldver = run_command('ld', '-v').stdout().strip()
>>> -if ldver.contains('2.30')
>>> -	if cc.has_argument('-mno-avx512f')
>>> -		march_opt += '-mno-avx512f'
>>> -		message('Binutils 2.30 detected, disabling AVX512 support as workaround for bug #97')
>>> +if host_machine.system() != 'windows'
>>> +	ldver = run_command('ld', '-v').stdout().strip()
>>> +	if ldver.contains('2.30')
>>> +		if cc.has_argument('-mno-avx512f')
>>> +			march_opt += '-mno-avx512f'
>>> +			message('Binutils 2.30 detected, disabling AVX512 support as workaround for bug #97')
>>> +		endif
>>>   	endif
>>>   endif
>>
>> Same comment as in v1, you should check which linker is used.
>> This check is only for GNU ld 2.30.
>>
> 
> I'm not aware of any better way to implement this in meson right now,
> sadly. Through the compiler object we can get information about linker
> flags, but not about the specific linker itself, since it is only called
> through the compiler.

Since on windows we only support clang, this check cause an error.
'ERROR:  Program or command 'ld' not found or not executable'
It is for this reason we excluded this check from the build flow.
Clang uses lld as linker from the LLVM project.

> 
>>> --- /dev/null
>>> +++ b/lib/librte_eal/windows/eal/eal_lcore.c
>>> + /* Get the cpu core id value */
>>> +unsigned int
>>> +eal_cpu_core_id(unsigned int lcore_id)
>>> +{
>>> +	return lcore_id;
>>> +}
>>
>> For this function and the others in this file,
>> please add a comment explaining this is a stub, not the expected result.
>> You can add a TODO or FIXME tag.
>>
>>> --- a/lib/meson.build
>>> +++ b/lib/meson.build
>>> +if host_machine.system() == 'windows'
>>> +	libraries = ['eal'] # override libraries for windows
>>> +endif
>>
>> Instead of simply "override", you could explain that the other libraries
>> are not yet supported on Windows.
>>
>>> --- a/meson.build
>>> +++ b/meson.build
>>> -# build libs and drivers
>>> +# build libs
>>>   subdir('lib')
>>> -subdir('buildtools')
>>> -subdir('drivers')
>>>   
>>> -# build binaries and installable tools
>>> -subdir('usertools')
>>> -subdir('app')
>>> +if host_machine.system() != 'windows'
>>> +	# build buildtools and drivers
>>> +	subdir('buildtools')
>>> +	subdir('drivers')
>>>   
>>> -# build docs
>>> -subdir('doc')
>>> +	# build binaries and installable tools
>>> +	subdir('usertools')
>>> +	subdir('app')
>>> +	subdir('test')
>>> +
>>> +	# build kernel modules if enabled
>>> +	if get_option('enable_kmods')
>>> +		subdir('kernel')
>>> +	endif
>>> +
>>> +	# build docs
>>> +	subdir('doc')
>>> +endif
>>
>> I don't like modifying this file.
>> Can we skip not supported directories inside the sub meson files?
>>
> Since we now mandate meson 0.47 we can use the "subdir_done()" function to
> this.
> 
> /Bruce
> 

Will be done as a part of v3
Thomas Monjalon March 7, 2019, 8:59 a.m. UTC | #7
07/03/2019 02:04, Anand Rawat:
> On 3/6/2019 3:52 AM, Richardson, Bruce wrote:
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> >> 06/03/2019 12:20, Bruce Richardson:
> >>> On Wed, Mar 06, 2019 at 11:03:24AM +0100, Thomas Monjalon wrote:
> >>>> 06/03/2019 05:16, Anand Rawat:
> >>>>> +if host_machine.system() != 'windows'
> >>>>> +       common_sources = files(
> >>>>
> >>>> The definitive solution should be to compile all common EAL files.
> >>>> Please explain what are the issues in the common files.
> >>>> I think we should not remove them and fix them one by one.
> >>>> You could provide a separate patch to skip some files for making
> >>>> helloworld working.
> >>>>
> >>>
> >>> I believe that is exactly what this patch is trying to do - it's
> >>> skipping the files unneeded to get helloworld working, and the
> >>> intention is to fix them one by one and add them back in later.
> >>> Perhaps this sort of change should be a separate (precursor) patch
> >>> where the cover letter can call this out explicitly?
> >>>
> >>>>> -deps += 'kvargs'
> >>>>> +if host_machine.system() != 'windows'
> >>>>> +       deps += 'kvargs'
> >>>>> +endif
> >>>>
> >>>> Why kvargs is removed?
> >>>
> >>> Again, I believe these actions are to disable the parts of DPDK that
> >>> are not needed to enable helloworld, allowing later patches to come in
> >>> and fix them.
> >>
> >> They are workarounds to build helloworld.
> >> It is good to have progress in the draft tree, but I see no point in
> >> merging this in master.
> >> I think we should separate patches which are doing definitive changes from
> >> temporary workaround patches disabling some files.
> >> It is not an issue to merge some patches for Windows which are not
> >> compiling.
> 
> Bruce is right, we only compile required header and source files in 
> order to avoid compatibility errors on windows. Without these
> change helloworld on windows would fail to compile. Adding windows 
> specific implementations of the common headers and sources would
> bloat up individual patches as well the number of patches. kvargs is 
> removed as a dependency to have minimum viable product for helloworld.
> If required for lcore mask, it'll added back in v3.

Please make separate patches for workarounds.
I am interested to push some patches which are really required in master,
but the workarounds should stay in the Windows draft repository.

Patch
diff mbox series

diff --git a/config/meson.build b/config/meson.build
index 0419607d3..37adf5b03 100644
--- a/config/meson.build
+++ b/config/meson.build
@@ -1,5 +1,5 @@ 
 # SPDX-License-Identifier: BSD-3-Clause
-# Copyright(c) 2017 Intel Corporation
+# Copyright(c) 2017-2019 Intel Corporation
 
 # set the machine type and cflags for it
 if meson.is_cross_build()
@@ -52,18 +52,26 @@  dpdk_extra_ldflags += '-Wl,--no-as-needed'
 add_project_link_arguments('-pthread', language: 'c')
 dpdk_extra_ldflags += '-pthread'
 
-# some libs depend on maths lib
-add_project_link_arguments('-lm', language: 'c')
-dpdk_extra_ldflags += '-lm'
+if cc.find_library('lm', required : false).found()
+	# some libs depend on maths lib
+	add_project_link_arguments('-lm', language: 'c')
+	dpdk_extra_ldflags += '-lm'
+endif
 
 # for linux link against dl, for bsd execinfo
 if host_machine.system() == 'linux'
 	link_lib = 'dl'
-else
+elif host_machine.system() == 'freebsd'
 	link_lib = 'execinfo'
+else
+	link_lib = ''
+endif
+
+# if link_lib is empty, do not add it to project properties
+if link_lib != ''
+	add_project_link_arguments('-l' + link_lib, language: 'c')
+	dpdk_extra_ldflags += '-l' + link_lib
 endif
-add_project_link_arguments('-l' + link_lib, language: 'c')
-dpdk_extra_ldflags += '-l' + link_lib
 
 # check for libraries used in multiple places in DPDK
 has_libnuma = 0
diff --git a/config/x86/meson.build b/config/x86/meson.build
index 7504cb9e5..558edfda9 100644
--- a/config/x86/meson.build
+++ b/config/x86/meson.build
@@ -1,15 +1,17 @@ 
 # SPDX-License-Identifier: BSD-3-Clause
-# Copyright(c) 2017 Intel Corporation
+# Copyright(c) 2017-2019 Intel Corporation
 
 # for checking defines we need to use the correct compiler flags
 march_opt = ['-march=@0@'.format(machine)]
 
 # get binutils version for the workaround of Bug 97
-ldver = run_command('ld', '-v').stdout().strip()
-if ldver.contains('2.30')
-	if cc.has_argument('-mno-avx512f')
-		march_opt += '-mno-avx512f'
-		message('Binutils 2.30 detected, disabling AVX512 support as workaround for bug #97')
+if host_machine.system() != 'windows'
+	ldver = run_command('ld', '-v').stdout().strip()
+	if ldver.contains('2.30')
+		if cc.has_argument('-mno-avx512f')
+			march_opt += '-mno-avx512f'
+			message('Binutils 2.30 detected, disabling AVX512 support as workaround for bug #97')
+		endif
 	endif
 endif
 
diff --git a/lib/librte_eal/common/meson.build b/lib/librte_eal/common/meson.build
index 5ecae0b1f..c010e8737 100644
--- a/lib/librte_eal/common/meson.build
+++ b/lib/librte_eal/common/meson.build
@@ -1,58 +1,65 @@ 
 # SPDX-License-Identifier: BSD-3-Clause
-# Copyright(c) 2017 Intel Corporation
+# Copyright(c) 2017-2019 Intel Corporation
 
 eal_inc += include_directories('.', 'include',
 		join_paths('include/arch', arch_subdir))
 
 common_objs = []
-common_sources = files(
-	'eal_common_bus.c',
-	'eal_common_cpuflags.c',
-	'eal_common_class.c',
-	'eal_common_devargs.c',
-	'eal_common_dev.c',
-	'eal_common_errno.c',
-	'eal_common_fbarray.c',
-	'eal_common_hexdump.c',
-	'eal_common_hypervisor.c',
-	'eal_common_launch.c',
-	'eal_common_lcore.c',
-	'eal_common_log.c',
-	'eal_common_memalloc.c',
-	'eal_common_memory.c',
-	'eal_common_memzone.c',
-	'eal_common_options.c',
-	'eal_common_proc.c',
-	'eal_common_string_fns.c',
-	'eal_common_tailqs.c',
-	'eal_common_thread.c',
-	'eal_common_timer.c',
-	'eal_common_uuid.c',
-	'hotplug_mp.c',
-	'malloc_elem.c',
-	'malloc_heap.c',
-	'malloc_mp.c',
-	'rte_keepalive.c',
-	'rte_malloc.c',
-	'rte_option.c',
-	'rte_reciprocal.c',
-	'rte_service.c'
-)
+common_sources = []
+common_headers = []
+if host_machine.system() != 'windows'
+	common_sources = files(
+		'eal_common_bus.c',
+		'eal_common_cpuflags.c',
+		'eal_common_class.c',
+		'eal_common_devargs.c',
+		'eal_common_dev.c',
+		'eal_common_errno.c',
+		'eal_common_fbarray.c',
+		'eal_common_hexdump.c',
+		'eal_common_hypervisor.c',
+		'eal_common_launch.c',
+		'eal_common_lcore.c',
+		'eal_common_log.c',
+		'eal_common_memalloc.c',
+		'eal_common_memory.c',
+		'eal_common_memzone.c',
+		'eal_common_options.c',
+		'eal_common_proc.c',
+		'eal_common_string_fns.c',
+		'eal_common_tailqs.c',
+		'eal_common_thread.c',
+		'eal_common_timer.c',
+		'eal_common_uuid.c',
+		'hotplug_mp.c',
+		'malloc_elem.c',
+		'malloc_heap.c',
+		'malloc_mp.c',
+		'rte_keepalive.c',
+		'rte_malloc.c',
+		'rte_option.c',
+		'rte_reciprocal.c',
+		'rte_service.c'
+	)
+endif
 
 # get architecture specific sources and objs
 eal_common_arch_sources = []
 eal_common_arch_objs = []
-subdir(join_paths('arch', arch_subdir))
+
+common_headers += files('include/rte_common.h')
+if host_machine.system() != 'windows'
+	subdir(join_paths('arch', arch_subdir))
+endif
 common_sources += eal_common_arch_sources
 common_objs += eal_common_arch_objs
 
-common_headers = files(
+common_headers += files(
 	'include/rte_alarm.h',
 	'include/rte_branch_prediction.h',
 	'include/rte_bus.h',
 	'include/rte_bitmap.h',
 	'include/rte_class.h',
-	'include/rte_common.h',
 	'include/rte_compat.h',
 	'include/rte_debug.h',
 	'include/rte_devargs.h',
@@ -85,7 +92,8 @@  common_headers = files(
 	'include/rte_tailq.h',
 	'include/rte_time.h',
 	'include/rte_uuid.h',
-	'include/rte_version.h')
+	'include/rte_version.h'
+)
 
 # special case install the generic headers, since they go in a subdir
 generic_headers = files(
diff --git a/lib/librte_eal/meson.build b/lib/librte_eal/meson.build
index 98c1d1f31..61b654557 100644
--- a/lib/librte_eal/meson.build
+++ b/lib/librte_eal/meson.build
@@ -1,5 +1,5 @@ 
 # SPDX-License-Identifier: BSD-3-Clause
-# Copyright(c) 2017 Intel Corporation
+# Copyright(c) 2017-2019 Intel Corporation
 
 # Custom EAL processing. EAL is complicated enough that it can't just
 # have a straight list of headers and source files.
@@ -17,13 +17,19 @@  elif host_machine.system() == 'freebsd'
 	dpdk_conf.set('RTE_EXEC_ENV_BSDAPP', 1)
 	subdir('bsdapp/eal')
 
+elif host_machine.system() == 'windows'
+	dpdk_conf.set('RTE_EXEC_ENV_WINDOWS', 1)
+	subdir('windows/eal')
+
 else
 	error('unsupported system type "@0@"'.format(host_machine.system()))
 endif
 
 version = 9  # the version of the EAL API
 allow_experimental_apis = true
-deps += 'kvargs'
+if host_machine.system() != 'windows'
+	deps += 'kvargs'
+endif
 if dpdk_conf.has('RTE_USE_LIBBSD')
 	ext_deps += libbsd
 endif
diff --git a/lib/librte_eal/windows/eal/eal.c b/lib/librte_eal/windows/eal/eal.c
new file mode 100644
index 000000000..134452a77
--- /dev/null
+++ b/lib/librte_eal/windows/eal/eal.c
@@ -0,0 +1,11 @@ 
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2019 Intel Corporation
+ */
+
+#include "rte_common.h"
+
+int
+rte_eal_init(int argc __rte_unused, char **argv __rte_unused)
+{
+	return 0;
+}
diff --git a/lib/librte_eal/windows/eal/eal_debug.c b/lib/librte_eal/windows/eal/eal_debug.c
new file mode 100644
index 000000000..012eeccfa
--- /dev/null
+++ b/lib/librte_eal/windows/eal/eal_debug.c
@@ -0,0 +1,12 @@ 
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2019 Intel Corporation
+ */
+
+#include "rte_common.h"
+
+void
+__rte_panic(const char *funcname __rte_unused,
+		const char *format __rte_unused, ...)
+{
+	abort();
+}
diff --git a/lib/librte_eal/windows/eal/eal_lcore.c b/lib/librte_eal/windows/eal/eal_lcore.c
new file mode 100644
index 000000000..be7adeb18
--- /dev/null
+++ b/lib/librte_eal/windows/eal/eal_lcore.c
@@ -0,0 +1,26 @@ 
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2019 Intel Corporation
+ */
+
+#include "rte_common.h"
+
+ /* Get the cpu core id value */
+unsigned int
+eal_cpu_core_id(unsigned int lcore_id)
+{
+	return lcore_id;
+}
+
+/* Check if a cpu is present by the presence of the cpu information for it */
+int
+eal_cpu_detected(unsigned int lcore_id __rte_unused)
+{
+	return 1;
+}
+
+/* Get CPU socket id (NUMA node) for a logical core */
+unsigned int
+eal_cpu_socket_id(unsigned int cpu_id __rte_unused)
+{
+	return 0;
+}
diff --git a/lib/librte_eal/windows/eal/eal_thread.c b/lib/librte_eal/windows/eal/eal_thread.c
new file mode 100644
index 000000000..222bd8f4d
--- /dev/null
+++ b/lib/librte_eal/windows/eal/eal_thread.c
@@ -0,0 +1,15 @@ 
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2018 Intel Corporation
+ */
+
+#include <windows.h>
+
+#include "rte_common.h"
+
+typedef uintptr_t eal_thread_t;
+
+int
+eal_thread_create(eal_thread_t *thread __rte_unused)
+{
+	return 0;
+}
diff --git a/lib/librte_eal/windows/eal/meson.build b/lib/librte_eal/windows/eal/meson.build
new file mode 100644
index 000000000..8b1735623
--- /dev/null
+++ b/lib/librte_eal/windows/eal/meson.build
@@ -0,0 +1,10 @@ 
+# SPDX-License-Identifier: BSD-3-Clause
+# Copyright(c) 2019 Intel Corporation
+
+env_objs = []
+env_headers = []
+env_sources = files('eal.c',
+	'eal_debug.c',
+	'eal_lcore.c',
+	'eal_thread.c',
+)
diff --git a/lib/meson.build b/lib/meson.build
index 99957ba7d..995c0e1ac 100644
--- a/lib/meson.build
+++ b/lib/meson.build
@@ -1,5 +1,5 @@ 
 # SPDX-License-Identifier: BSD-3-Clause
-# Copyright(c) 2017 Intel Corporation
+# Copyright(c) 2017-2019 Intel Corporation
 
 
 # process all libraries equally, as far as possible
@@ -30,6 +30,10 @@  libraries = [
 	# flow_classify lib depends on pkt framework table lib
 	'flow_classify', 'bpf', 'telemetry']
 
+if host_machine.system() == 'windows'
+	libraries = ['eal'] # override libraries for windows
+endif
+
 default_cflags = machine_args
 if cc.has_argument('-Wno-format-truncation')
 	default_cflags += '-Wno-format-truncation'
diff --git a/meson.build b/meson.build
index 69833de82..856e94c37 100644
--- a/meson.build
+++ b/meson.build
@@ -1,5 +1,5 @@ 
 # SPDX-License-Identifier: BSD-3-Clause
-# Copyright(c) 2017 Intel Corporation
+# Copyright(c) 2017-2019 Intel Corporation
 
 project('DPDK', 'C',
 	version: '19.05.0-rc0',
@@ -13,6 +13,7 @@  cc = meson.get_compiler('c')
 dpdk_conf = configuration_data()
 dpdk_libraries = []
 dpdk_static_libraries = []
+driver_classes = []
 dpdk_drivers = []
 dpdk_extra_ldflags = []
 dpdk_app_link_libraries = []
@@ -35,28 +36,33 @@  eal_pmd_path = join_paths(get_option('prefix'), driver_install_path)
 global_inc = include_directories('.', 'config', 'lib/librte_eal/common/include')
 subdir('config')
 
-# build libs and drivers
+# build libs
 subdir('lib')
-subdir('buildtools')
-subdir('drivers')
 
-# build binaries and installable tools
-subdir('usertools')
-subdir('app')
+if host_machine.system() != 'windows'
+	# build buildtools and drivers
+	subdir('buildtools')
+	subdir('drivers')
 
-# build docs
-subdir('doc')
+	# build binaries and installable tools
+	subdir('usertools')
+	subdir('app')
+	subdir('test')
+
+	# build kernel modules if enabled
+	if get_option('enable_kmods')
+		subdir('kernel')
+	endif
+
+	# build docs
+	subdir('doc')
+endif
 
 # build any examples explicitly requested - useful for developers
 if get_option('examples') != ''
 	subdir('examples')
 endif
 
-# build kernel modules if enabled
-if get_option('enable_kmods')
-	subdir('kernel')
-endif
-
 # write the build config
 build_cfg = 'rte_build_config.h'
 configure_file(output: build_cfg,