Re: [PATCH 1/2] Makefile: allow overriding CFLAGS on the command line
On 4 November 2015 at 12:13, Andre Przywara wrote: > Hi Riku, > > On 04/11/15 10:02, Riku Voipio wrote: >> On 30 October 2015 at 19:20, Andre Przywara wrote: >>> When a Makefile variable is set on the make command line, all >>> Makefile-internal assignments to that very variable are _ignored_. >>> Since we add quite some essential values to CFLAGS internally, >>> specifying some CFLAGS on the command line will usually break the >>> build (and not fix any include file problems you hoped to overcome >>> with that). >>> Somewhat against intuition GNU make provides the "override" directive >>> to change this behavior; with that assignments in the Makefile get >>> _appended_ to the value given on the command line. [1] >>> >>> Change any internal assignments to use that directive, so that a user >>> can use: >>> $ make CFLAGS=/path/to/my/include/dir >>> to teach kvmtool about non-standard header file locations (helpful >>> for cross-compilation) or to tweak other compiler options. >>> >>> Signed-off-by: Andre Przywara >>> >>> [1] >>> https://www.gnu.org/software/make/manual/html_node/Override-Directive.html >>> --- >>> Makefile | 15 +++ >>> 1 file changed, 7 insertions(+), 8 deletions(-) >>> >>> diff --git a/Makefile b/Makefile >>> index f8f7cc4..77a7c9f 100644 >>> --- a/Makefile >>> +++ b/Makefile >>> @@ -15,9 +15,7 @@ include config/utilities.mak >>> include config/feature-tests.mak >>> >>> CC := $(CROSS_COMPILE)gcc >>> -CFLAGS := >>> LD := $(CROSS_COMPILE)ld >>> -LDFLAGS:= >> >> This breaks builds of debian packages as dpkg-buildpackage sets LDFLAGS >> to something unsuitable for guest init. >> >> Looks like this has been an issue before: > >> >> commit 57fa349a9792a629e4ed2d89e1309cc96dcc39af >> Author: Will Deacon >> Date: Thu Jun 4 16:25:36 2015 +0100 >> >> Don't inherit CFLAGS and LDFLAGS from the environment >> >> kvmtool doesn't build with arbitrary flags, so let's clear CFLAGS and >> LDFLAGS by default at the top of the Makefile, allowing people to add >> additional options there if they really want to. >> >> Reported by Dave Jones, who ended up passing -std=gnu99 by mistake. > > Well, I fixed this issue later with making kvmtool compilation more > robust when using modern compiler standards. > That's why I wanted this kludge to go away. > >> I think it's better to have EXTRA_CFLAGS and EXTRA_LDFLAGS like the kernel >> has. > > Mmmh, I'd rather see guest_init creation use their own flags for it, > since it is so special and actually independent from the target userland. > Let me check this out and send out my guest_init Makefile fix I have > lying around here on the way. > > What LDFLAGS are actually causing your issues? LINK guest/init ld: unrecognized option '-Wl,-z,relro' That's another issue - kvmtool passes LDFLAGS to both LD and CC yet they actually take different command line options. > Cheers, > Andre. > >> >>> FIND := find >>> CSCOPE := cscope >>> @@ -162,7 +160,7 @@ ifeq ($(ARCH), arm) >>> OBJS+= arm/aarch32/kvm-cpu.o >>> ARCH_INCLUDE:= $(HDRS_ARM_COMMON) >>> ARCH_INCLUDE+= -Iarm/aarch32/include >>> - CFLAGS += -march=armv7-a >>> + override CFLAGS += -march=armv7-a >>> >>> ARCH_WANT_LIBFDT := y >>> endif >>> @@ -274,12 +272,12 @@ endif >>> ifeq ($(LTO),1) >>> FLAGS_LTO := -flto >>> ifeq ($(call try-build,$(SOURCE_HELLO),$(CFLAGS),$(FLAGS_LTO)),y) >>> - CFLAGS += $(FLAGS_LTO) >>> + override CFLAGS += $(FLAGS_LTO) >>> endif >>> endif >>> >>> ifeq ($(call try-build,$(SOURCE_STATIC),,-static),y) >>> - CFLAGS += -DCONFIG_GUEST_INIT >>> + override CFLAGS += -DCONFIG_GUEST_INIT >>> GUEST_INIT := guest/init >>> GUEST_OBJS = guest/guest_init.o >>> ifeq ($(ARCH_PRE_INIT),) >>> @@ -331,7 +329,8 @@ DEFINES += >>> -DKVMTOOLS_VERSION='"$(KVMTOOLS_VERSION)"' >>> DEFINES+= -DBUILD_ARCH='"$(ARCH)"' >>> >>> KVM_INCLUDE := include >>> -CFLAGS += $(CPPFLAGS) $(DEFINES)
Re: [PATCH 1/2] Makefile: allow overriding CFLAGS on the command line
On 30 October 2015 at 19:20, Andre Przywara wrote: > When a Makefile variable is set on the make command line, all > Makefile-internal assignments to that very variable are _ignored_. > Since we add quite some essential values to CFLAGS internally, > specifying some CFLAGS on the command line will usually break the > build (and not fix any include file problems you hoped to overcome > with that). > Somewhat against intuition GNU make provides the "override" directive > to change this behavior; with that assignments in the Makefile get > _appended_ to the value given on the command line. [1] > > Change any internal assignments to use that directive, so that a user > can use: > $ make CFLAGS=/path/to/my/include/dir > to teach kvmtool about non-standard header file locations (helpful > for cross-compilation) or to tweak other compiler options. > > Signed-off-by: Andre Przywara > > [1] https://www.gnu.org/software/make/manual/html_node/Override-Directive.html > --- > Makefile | 15 +++ > 1 file changed, 7 insertions(+), 8 deletions(-) > > diff --git a/Makefile b/Makefile > index f8f7cc4..77a7c9f 100644 > --- a/Makefile > +++ b/Makefile > @@ -15,9 +15,7 @@ include config/utilities.mak > include config/feature-tests.mak > > CC := $(CROSS_COMPILE)gcc > -CFLAGS := > LD := $(CROSS_COMPILE)ld > -LDFLAGS:= This breaks builds of debian packages as dpkg-buildpackage sets LDFLAGS to something unsuitable for guest init. Looks like this has been an issue before: commit 57fa349a9792a629e4ed2d89e1309cc96dcc39af Author: Will Deacon Date: Thu Jun 4 16:25:36 2015 +0100 Don't inherit CFLAGS and LDFLAGS from the environment kvmtool doesn't build with arbitrary flags, so let's clear CFLAGS and LDFLAGS by default at the top of the Makefile, allowing people to add additional options there if they really want to. Reported by Dave Jones, who ended up passing -std=gnu99 by mistake. I think it's better to have EXTRA_CFLAGS and EXTRA_LDFLAGS like the kernel has. > FIND := find > CSCOPE := cscope > @@ -162,7 +160,7 @@ ifeq ($(ARCH), arm) > OBJS+= arm/aarch32/kvm-cpu.o > ARCH_INCLUDE:= $(HDRS_ARM_COMMON) > ARCH_INCLUDE+= -Iarm/aarch32/include > - CFLAGS += -march=armv7-a > + override CFLAGS += -march=armv7-a > > ARCH_WANT_LIBFDT := y > endif > @@ -274,12 +272,12 @@ endif > ifeq ($(LTO),1) > FLAGS_LTO := -flto > ifeq ($(call try-build,$(SOURCE_HELLO),$(CFLAGS),$(FLAGS_LTO)),y) > - CFLAGS += $(FLAGS_LTO) > + override CFLAGS += $(FLAGS_LTO) > endif > endif > > ifeq ($(call try-build,$(SOURCE_STATIC),,-static),y) > - CFLAGS += -DCONFIG_GUEST_INIT > + override CFLAGS += -DCONFIG_GUEST_INIT > GUEST_INIT := guest/init > GUEST_OBJS = guest/guest_init.o > ifeq ($(ARCH_PRE_INIT),) > @@ -331,7 +329,8 @@ DEFINES += -DKVMTOOLS_VERSION='"$(KVMTOOLS_VERSION)"' > DEFINES+= -DBUILD_ARCH='"$(ARCH)"' > > KVM_INCLUDE := include > -CFLAGS += $(CPPFLAGS) $(DEFINES) -I$(KVM_INCLUDE) -I$(ARCH_INCLUDE) -O2 > -fno-strict-aliasing -g > +override CFLAGS+= $(CPPFLAGS) $(DEFINES) -I$(KVM_INCLUDE) > -I$(ARCH_INCLUDE) > +override CFLAGS+= -O2 -fno-strict-aliasing -g > > WARNINGS += -Wall > WARNINGS += -Wformat=2 > @@ -349,10 +348,10 @@ WARNINGS += -Wvolatile-register-var > WARNINGS += -Wwrite-strings > WARNINGS += -Wno-format-nonliteral > > -CFLAGS += $(WARNINGS) > +override CFLAGS+= $(WARNINGS) > > ifneq ($(WERROR),0) > - CFLAGS += -Werror > + override CFLAGS += -Werror > endif > > all: $(PROGRAM) $(PROGRAM_ALIAS) $(GUEST_INIT) $(GUEST_PRE_INIT) > -- > 2.5.1 > > -- > To unsubscribe from this list: send the line "unsubscribe kvm" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] kvmtool: don't rely on $HOME
On 17 September 2015 at 18:53, Will Deacon wrote: > On Thu, Sep 17, 2015 at 03:03:15PM +0100, Alban Crequy wrote: >> kvm__set_dir() called in main() and kvm__get_dir() rely on $HOME. But in >> some environments (such as starting lkvm through systemd-run), $HOME is >> undefined. This causes bind() to use a socket path containing "(null)" >> and this fails. The current code does not check errors returned by >> realpath(). >> >> Symptoms: >> >> | bind: No such file or directory >> | Error: Failed adding socket to epoll >> | Warning: Failed init: kvm_ipc__init >> | >> | Fatal: Initialisation failed >> >> This bug was first reported on https://github.com/coreos/rkt/issues/1393 >> >> Instead of using "$HOME/.lkvm/" (i.e. "/root/.lkvm/"), this patch uses >> "/var/lib/lkvm/". This also improve the error reporting by printing the >> socket filename. > > Hmm, but that requires lkvm to be run with sufficient privileges to > write to /var/lib, which I don't think is generally the case. I think we > have a few options: > > (1) Try /var/lib/lkvm if $HOME is NULL > (2) Use an alternative environment variable for the pid prefix > (3) Add a --pid command line option for the pidfile > (4) ??? > > Any preferences? What do other projects do? The right place to put a pid file would be $XDG_RUNTIME_DIR aka /run/user/$UID/. System services write their pidfiles and sockets to plain /run The place where to put the rootfs structure created in builtin-setup.c is more complicated. Is the created rootfs epheremal? Then sticking under /run(user/UID) is fine. Riku -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] kvmtool Makefile: relax arm test
On 10 September 2015 at 13:30, Will Deacon wrote: > On Thu, Sep 10, 2015 at 06:45:59AM +0100, Riku Voipio wrote: >> Ping? > Applied and pushed, thanks. thanks > In future, it's best to Cc me if you want to make sure stuff doesn't > get missed :) I just followed the CONTRIBUTING section from the README, maybe it needs to be appended with the CC hint? Riku -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] kvmtool Makefile: relax arm test
On 4 September 2015 at 14:06, Andre Przywara wrote: > Hi Riku, > > On 04/09/15 11:52, Riku Voipio wrote: >> On 4 September 2015 at 13:10, Andre Przywara wrote: >>> Hi Riku, >>> >>> On 03/09/15 12:20, riku.voi...@linaro.org wrote: >>>> From: Riku Voipio >>>> >>>> Currently Makefile accepts only armv7l.* When building kvmtool under 32bit >>>> personality on Aarch64 machines, uname -m reports "armv8l", so build fails. >>>> We expect doing 32bit arm builds in Aarch64 to become standard the same way >>>> people do i386 builds on x86_64 machines. >>>> >>>> Make the sed test a little more greedy so armv8l becomes acceptable. >>>> >>>> Signed-off-by: Riku Voipio >>> >>> The patch looks OK to me, I just wonder how you do the actual build >>> within the linux32 environment? >>> Do you have an arm cross compiler installed and set CROSS_COMPILE? Or is >>> there a magic compiler (driver) which uses uname -m as well? >>> And what would be the difference to setting ARCH=arm as well? Just >>> convenience? >> >> It's just an arm32 chroot, with an native arm32 compiler. The chroot >> is on an arm64 machine since these tend to be much faster than arm32 >> hardware. > > Oh right, a chroot, didn't think about the obvious ;-) > Also it applies to 64-bit kernels with 32-bit root filesystems, I think. > So: > > Acked-by: Andre Przywara Ping? >> >> It would of course be possible to set ARCH=arm, but that would mean >> some ifdefs in the Debian packaging, since the same build rule should >> work for all architectures. >> >> Riku >> -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] kvmtool Makefile: relax arm test
On 4 September 2015 at 13:10, Andre Przywara wrote: > Hi Riku, > > On 03/09/15 12:20, riku.voi...@linaro.org wrote: >> From: Riku Voipio >> >> Currently Makefile accepts only armv7l.* When building kvmtool under 32bit >> personality on Aarch64 machines, uname -m reports "armv8l", so build fails. >> We expect doing 32bit arm builds in Aarch64 to become standard the same way >> people do i386 builds on x86_64 machines. >> >> Make the sed test a little more greedy so armv8l becomes acceptable. >> >> Signed-off-by: Riku Voipio > > The patch looks OK to me, I just wonder how you do the actual build > within the linux32 environment? > Do you have an arm cross compiler installed and set CROSS_COMPILE? Or is > there a magic compiler (driver) which uses uname -m as well? > And what would be the difference to setting ARCH=arm as well? Just > convenience? It's just an arm32 chroot, with an native arm32 compiler. The chroot is on an arm64 machine since these tend to be much faster than arm32 hardware. It would of course be possible to set ARCH=arm, but that would mean some ifdefs in the Debian packaging, since the same build rule should work for all architectures. Riku -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] kvmtool Makefile: relax arm test
From: Riku Voipio Currently Makefile accepts only armv7l.* When building kvmtool under 32bit personality on Aarch64 machines, uname -m reports "armv8l", so build fails. We expect doing 32bit arm builds in Aarch64 to become standard the same way people do i386 builds on x86_64 machines. Make the sed test a little more greedy so armv8l becomes acceptable. Signed-off-by: Riku Voipio --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 1534e6f..7b17d52 100644 --- a/Makefile +++ b/Makefile @@ -103,7 +103,7 @@ OBJS+= hw/i8042.o # Translate uname -m into ARCH string ARCH ?= $(shell uname -m | sed -e s/i.86/i386/ -e s/ppc.*/powerpc/ \ - -e s/armv7.*/arm/ -e s/aarch64.*/arm64/ -e s/mips64/mips/) + -e s/armv.*/arm/ -e s/aarch64.*/arm64/ -e s/mips64/mips/) ifeq ($(ARCH),i386) ARCH := x86 -- 2.4.6 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html