tools: hv: Fix cross-compilation

[PATCH] tools: hv: Fix cross-compilation

Posted by Saurabh Sengar 1 year ago

Use the native ARCH only incase it is not set, this will allow
the cross complilation where ARCH is explicitly set. Add few
info prints as well to know what arch and toolchain is getting
used to build it.

Additionally, simplify the check for ARCH so that fcopy daemon
is build only for x86_64.

Fixes: 82b0945ce2c2 ("tools: hv: Add new fcopy application based on uio driver")
Reported-by: Adrian Vladu <avladu@cloudbasesolutions.com>
Closes: https://lore.kernel.org/linux-hyperv/Z1Y9ZkAt9GPjQsGi@liuwe-devbox-debian-v2/
Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
---
 tools/hv/Makefile | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/tools/hv/Makefile b/tools/hv/Makefile
index 34ffcec264ab..d29e6be6309b 100644
--- a/tools/hv/Makefile
+++ b/tools/hv/Makefile
@@ -2,7 +2,7 @@
 # Makefile for Hyper-V tools
 include ../scripts/Makefile.include
 
-ARCH := $(shell uname -m 2>/dev/null)
+ARCH ?= $(shell uname -m 2>/dev/null)
 sbindir ?= /usr/sbin
 libexecdir ?= /usr/libexec
 sharedstatedir ?= /var/lib
@@ -20,18 +20,26 @@ override CFLAGS += -O2 -Wall -g -D_GNU_SOURCE -I$(OUTPUT)include
 override CFLAGS += -Wno-address-of-packed-member
 
 ALL_TARGETS := hv_kvp_daemon hv_vss_daemon
-ifneq ($(ARCH), aarch64)
+ifeq ($(ARCH), x86_64)
 ALL_TARGETS += hv_fcopy_uio_daemon
 endif
 ALL_PROGRAMS := $(patsubst %,$(OUTPUT)%,$(ALL_TARGETS))
 
 ALL_SCRIPTS := hv_get_dhcp_info.sh hv_get_dns_info.sh hv_set_ifconfig.sh
 
-all: $(ALL_PROGRAMS)
+all: info $(ALL_PROGRAMS)
 
 export srctree OUTPUT CC LD CFLAGS
 include $(srctree)/tools/build/Makefile.include
 
+info:
+	@echo "---------------------"
+	@echo "Building for:"
+	@echo "CC $(CC)"
+	@echo "LD $(LD)"
+	@echo "ARCH $(ARCH)"
+	@echo "---------------------"
+
 HV_KVP_DAEMON_IN := $(OUTPUT)hv_kvp_daemon-in.o
 $(HV_KVP_DAEMON_IN): FORCE
 	$(Q)$(MAKE) $(build)=hv_kvp_daemon
-- 
2.43.0

Re: [PATCH] tools: hv: Fix cross-compilation

Posted by Roman Kisel 1 year ago

Thanks, LGTM!

Reviewed-by: Roman Kisel <romank@linux.microsoft.com>

On 12/12/2024 12:28 AM, Saurabh Sengar wrote:
> Use the native ARCH only incase it is not set, this will allow
> the cross complilation where ARCH is explicitly set. Add few
> info prints as well to know what arch and toolchain is getting
> used to build it.
> 
> Additionally, simplify the check for ARCH so that fcopy daemon
> is build only for x86_64.
> 
> Fixes: 82b0945ce2c2 ("tools: hv: Add new fcopy application based on uio driver")
> Reported-by: Adrian Vladu <avladu@cloudbasesolutions.com>
> Closes: https://lore.kernel.org/linux-hyperv/Z1Y9ZkAt9GPjQsGi@liuwe-devbox-debian-v2/
> Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> ---
>   tools/hv/Makefile | 14 +++++++++++---
>   1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/hv/Makefile b/tools/hv/Makefile
> index 34ffcec264ab..d29e6be6309b 100644
> --- a/tools/hv/Makefile
> +++ b/tools/hv/Makefile
> @@ -2,7 +2,7 @@
>   # Makefile for Hyper-V tools
>   include ../scripts/Makefile.include
>   
> -ARCH := $(shell uname -m 2>/dev/null)
> +ARCH ?= $(shell uname -m 2>/dev/null)
>   sbindir ?= /usr/sbin
>   libexecdir ?= /usr/libexec
>   sharedstatedir ?= /var/lib
> @@ -20,18 +20,26 @@ override CFLAGS += -O2 -Wall -g -D_GNU_SOURCE -I$(OUTPUT)include
>   override CFLAGS += -Wno-address-of-packed-member
>   
>   ALL_TARGETS := hv_kvp_daemon hv_vss_daemon
> -ifneq ($(ARCH), aarch64)
> +ifeq ($(ARCH), x86_64)
>   ALL_TARGETS += hv_fcopy_uio_daemon
>   endif
>   ALL_PROGRAMS := $(patsubst %,$(OUTPUT)%,$(ALL_TARGETS))
>   
>   ALL_SCRIPTS := hv_get_dhcp_info.sh hv_get_dns_info.sh hv_set_ifconfig.sh
>   
> -all: $(ALL_PROGRAMS)
> +all: info $(ALL_PROGRAMS)
>   
>   export srctree OUTPUT CC LD CFLAGS
>   include $(srctree)/tools/build/Makefile.include
>   
> +info:
> +	@echo "---------------------"
> +	@echo "Building for:"
> +	@echo "CC $(CC)"
> +	@echo "LD $(LD)"
> +	@echo "ARCH $(ARCH)"
> +	@echo "---------------------"
> +
>   HV_KVP_DAEMON_IN := $(OUTPUT)hv_kvp_daemon-in.o
>   $(HV_KVP_DAEMON_IN): FORCE
>   	$(Q)$(MAKE) $(build)=hv_kvp_daemon

-- 
Thank you,
Roman

Re: [PATCH] tools: hv: Fix cross-compilation

Posted by Wei Liu 1 year ago

On Thu, Dec 12, 2024 at 12:28:34AM -0800, Saurabh Sengar wrote:
> Use the native ARCH only incase it is not set, this will allow
> the cross complilation where ARCH is explicitly set. Add few
> info prints as well to know what arch and toolchain is getting
> used to build it.
> 
> Additionally, simplify the check for ARCH so that fcopy daemon
> is build only for x86_64.
> 
> Fixes: 82b0945ce2c2 ("tools: hv: Add new fcopy application based on uio driver")
> Reported-by: Adrian Vladu <avladu@cloudbasesolutions.com>
> Closes: https://lore.kernel.org/linux-hyperv/Z1Y9ZkAt9GPjQsGi@liuwe-devbox-debian-v2/
> Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> ---
>  tools/hv/Makefile | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/hv/Makefile b/tools/hv/Makefile
> index 34ffcec264ab..d29e6be6309b 100644
> --- a/tools/hv/Makefile
> +++ b/tools/hv/Makefile
> @@ -2,7 +2,7 @@
>  # Makefile for Hyper-V tools
>  include ../scripts/Makefile.include
>  
> -ARCH := $(shell uname -m 2>/dev/null)
> +ARCH ?= $(shell uname -m 2>/dev/null)
>  sbindir ?= /usr/sbin
>  libexecdir ?= /usr/libexec
>  sharedstatedir ?= /var/lib
> @@ -20,18 +20,26 @@ override CFLAGS += -O2 -Wall -g -D_GNU_SOURCE -I$(OUTPUT)include
>  override CFLAGS += -Wno-address-of-packed-member
>  
>  ALL_TARGETS := hv_kvp_daemon hv_vss_daemon
> -ifneq ($(ARCH), aarch64)
> +ifeq ($(ARCH), x86_64)

Technically speaking, you can also build this for x86 (32bit). Whether
anybody uses it is another question.

>  ALL_TARGETS += hv_fcopy_uio_daemon
>  endif
>  ALL_PROGRAMS := $(patsubst %,$(OUTPUT)%,$(ALL_TARGETS))
>  
>  ALL_SCRIPTS := hv_get_dhcp_info.sh hv_get_dns_info.sh hv_set_ifconfig.sh
>  
> -all: $(ALL_PROGRAMS)
> +all: info $(ALL_PROGRAMS)
>  
>  export srctree OUTPUT CC LD CFLAGS
>  include $(srctree)/tools/build/Makefile.include
>  
> +info:
> +	@echo "---------------------"
> +	@echo "Building for:"
> +	@echo "CC $(CC)"
> +	@echo "LD $(LD)"
> +	@echo "ARCH $(ARCH)"
> +	@echo "---------------------"

I don't think this is needed. Anyone who's building the kernel source
should know what tool chain they are using and architecture they're
building for.

Thanks,
Wei.

> +
>  HV_KVP_DAEMON_IN := $(OUTPUT)hv_kvp_daemon-in.o
>  $(HV_KVP_DAEMON_IN): FORCE
>  	$(Q)$(MAKE) $(build)=hv_kvp_daemon
> -- 
> 2.43.0
>

Re: [PATCH] tools: hv: Fix cross-compilation

Posted by Saurabh Singh Sengar 1 year ago

On Thu, Dec 12, 2024 at 11:06:16PM +0000, Wei Liu wrote:
> On Thu, Dec 12, 2024 at 12:28:34AM -0800, Saurabh Sengar wrote:
> > Use the native ARCH only incase it is not set, this will allow
> > the cross complilation where ARCH is explicitly set. Add few
> > info prints as well to know what arch and toolchain is getting
> > used to build it.
> > 
> > Additionally, simplify the check for ARCH so that fcopy daemon
> > is build only for x86_64.
> > 
> > Fixes: 82b0945ce2c2 ("tools: hv: Add new fcopy application based on uio driver")
> > Reported-by: Adrian Vladu <avladu@cloudbasesolutions.com>
> > Closes: https://lore.kernel.org/linux-hyperv/Z1Y9ZkAt9GPjQsGi@liuwe-devbox-debian-v2/
> > Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> > ---
> >  tools/hv/Makefile | 14 +++++++++++---
> >  1 file changed, 11 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tools/hv/Makefile b/tools/hv/Makefile
> > index 34ffcec264ab..d29e6be6309b 100644
> > --- a/tools/hv/Makefile
> > +++ b/tools/hv/Makefile
> > @@ -2,7 +2,7 @@
> >  # Makefile for Hyper-V tools
> >  include ../scripts/Makefile.include
> >  
> > -ARCH := $(shell uname -m 2>/dev/null)
> > +ARCH ?= $(shell uname -m 2>/dev/null)
> >  sbindir ?= /usr/sbin
> >  libexecdir ?= /usr/libexec
> >  sharedstatedir ?= /var/lib
> > @@ -20,18 +20,26 @@ override CFLAGS += -O2 -Wall -g -D_GNU_SOURCE -I$(OUTPUT)include
> >  override CFLAGS += -Wno-address-of-packed-member
> >  
> >  ALL_TARGETS := hv_kvp_daemon hv_vss_daemon
> > -ifneq ($(ARCH), aarch64)
> > +ifeq ($(ARCH), x86_64)
> 
> Technically speaking, you can also build this for x86 (32bit). Whether
> anybody uses it is another question.

My intention is to allow fcopy daemon build only for the arch it has
been tested. IMO its better than restricting only for arm64/aarch64.

I tried with gcc '-m32' switch which I believe is for 32 bit x86 compilation
I see problems with it on other (kvp daemon) daemons too. I think we never
cared about 32 bit.

saurabh@Saurabh:/work/linux-next/tools/hv$ make ARCH=x86 CFLAGS=-m32
make[1]: Entering directory '/work/linux-next/tools/hv'
  CC      hv_kvp_daemon.o
hv_kvp_daemon.c:25:10: fatal error: sys/poll.h: No such file or directory
   25 | #include <sys/poll.h>
      |          ^~~~~~~~~~~~
compilation terminated.
make[1]: *** [/work/linux-next/tools/build/Makefile.build:106: hv_kvp_daemon.o] Error 1
make[1]: Leaving directory '/work/linux-next/tools/hv'
make: *** [Makefile:37: hv_kvp_daemon-in.o] Error 2


I don't have any strong opinion here, if you want I can allow x86 compilation
for fcopy daemon as well.

Please let me know what is your preference.

> 
> >  ALL_TARGETS += hv_fcopy_uio_daemon
> >  endif
> >  ALL_PROGRAMS := $(patsubst %,$(OUTPUT)%,$(ALL_TARGETS))
> >  
> >  ALL_SCRIPTS := hv_get_dhcp_info.sh hv_get_dns_info.sh hv_set_ifconfig.sh
> >  
> > -all: $(ALL_PROGRAMS)
> > +all: info $(ALL_PROGRAMS)
> >  
> >  export srctree OUTPUT CC LD CFLAGS
> >  include $(srctree)/tools/build/Makefile.include
> >  
> > +info:
> > +	@echo "---------------------"
> > +	@echo "Building for:"
> > +	@echo "CC $(CC)"
> > +	@echo "LD $(LD)"
> > +	@echo "ARCH $(ARCH)"
> > +	@echo "---------------------"
> 
> I don't think this is needed. Anyone who's building the kernel source
> should know what tool chain they are using and architecture they're
> building for.

I am fine removing it in V2.

- Saurabh

Re: [PATCH] tools: hv: Fix cross-compilation

Posted by Wei Liu 12 months ago

On Thu, Dec 12, 2024 at 08:01:02PM -0800, Saurabh Singh Sengar wrote:
> On Thu, Dec 12, 2024 at 11:06:16PM +0000, Wei Liu wrote:
> > On Thu, Dec 12, 2024 at 12:28:34AM -0800, Saurabh Sengar wrote:
> > > Use the native ARCH only incase it is not set, this will allow
> > > the cross complilation where ARCH is explicitly set. Add few
> > > info prints as well to know what arch and toolchain is getting
> > > used to build it.
> > > 
> > > Additionally, simplify the check for ARCH so that fcopy daemon
> > > is build only for x86_64.
> > > 
> > > Fixes: 82b0945ce2c2 ("tools: hv: Add new fcopy application based on uio driver")
> > > Reported-by: Adrian Vladu <avladu@cloudbasesolutions.com>
> > > Closes: https://lore.kernel.org/linux-hyperv/Z1Y9ZkAt9GPjQsGi@liuwe-devbox-debian-v2/
> > > Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> > > ---
> > >  tools/hv/Makefile | 14 +++++++++++---
> > >  1 file changed, 11 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/tools/hv/Makefile b/tools/hv/Makefile
> > > index 34ffcec264ab..d29e6be6309b 100644
> > > --- a/tools/hv/Makefile
> > > +++ b/tools/hv/Makefile
> > > @@ -2,7 +2,7 @@
> > >  # Makefile for Hyper-V tools
> > >  include ../scripts/Makefile.include
> > >  
> > > -ARCH := $(shell uname -m 2>/dev/null)
> > > +ARCH ?= $(shell uname -m 2>/dev/null)
> > >  sbindir ?= /usr/sbin
> > >  libexecdir ?= /usr/libexec
> > >  sharedstatedir ?= /var/lib
> > > @@ -20,18 +20,26 @@ override CFLAGS += -O2 -Wall -g -D_GNU_SOURCE -I$(OUTPUT)include
> > >  override CFLAGS += -Wno-address-of-packed-member
> > >  
> > >  ALL_TARGETS := hv_kvp_daemon hv_vss_daemon
> > > -ifneq ($(ARCH), aarch64)
> > > +ifeq ($(ARCH), x86_64)
> > 
> > Technically speaking, you can also build this for x86 (32bit). Whether
> > anybody uses it is another question.
> 
> My intention is to allow fcopy daemon build only for the arch it has
> been tested. IMO its better than restricting only for arm64/aarch64.
> 
> I tried with gcc '-m32' switch which I believe is for 32 bit x86 compilation
> I see problems with it on other (kvp daemon) daemons too. I think we never
> cared about 32 bit.
> 
> saurabh@Saurabh:/work/linux-next/tools/hv$ make ARCH=x86 CFLAGS=-m32
> make[1]: Entering directory '/work/linux-next/tools/hv'
>   CC      hv_kvp_daemon.o
> hv_kvp_daemon.c:25:10: fatal error: sys/poll.h: No such file or directory
>    25 | #include <sys/poll.h>
>       |          ^~~~~~~~~~~~
> compilation terminated.
> make[1]: *** [/work/linux-next/tools/build/Makefile.build:106: hv_kvp_daemon.o] Error 1
> make[1]: Leaving directory '/work/linux-next/tools/hv'
> make: *** [Makefile:37: hv_kvp_daemon-in.o] Error 2
> 
> 
> I don't have any strong opinion here, if you want I can allow x86 compilation
> for fcopy daemon as well.
> 
> Please let me know what is your preference.

We can leave the code as-is. If and when someone wants to build this for
32-bit x86, they can fix the code.

I think the number of people who want to build this for 32-bit x86 is
diminishing by the day -- it there was a large group in the first place.

Thanks,
Wei.