Re: [PATCH 1/2] Makefile: allow overriding CFLAGS on the command line

[Riku Voipio]

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 1/2] Makefile: allow overriding CFLAGS on the command line

[Riku Voipio]

On 4 November 2015 at 12:13, Andre Przywara <andre.przyw...@arm.com> wrote:
> Hi Riku,
>
> On 04/11/15 10:02, Riku Voipio wrote:
>> On 30 October 2015 at 19:20, Andre Przywara <andre.przyw...@arm.com> 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 <andre.przyw...@arm.com>
>>>
>>> [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 <will.dea...@arm.com>
>> 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_INCL

Re: [PATCH] kvmtool: don't rely on $HOME

[Riku Voipio]

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

[Riku Voipio]

On 10 September 2015 at 13:30, Will Deacon <will.dea...@arm.com> 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

[Riku Voipio]

On 4 September 2015 at 14:06, Andre Przywara <andre.przyw...@arm.com> wrote:
> Hi Riku,
>
> On 04/09/15 11:52, Riku Voipio wrote:
>> On 4 September 2015 at 13:10, Andre Przywara <andre.przyw...@arm.com> wrote:
>>> Hi Riku,
>>>
>>> On 03/09/15 12:20, riku.voi...@linaro.org wrote:
>>>> From: Riku Voipio <riku.voi...@linaro.org>
>>>>
>>>> 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 <riku.voi...@linaro.org>
>>>
>>> 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 <andre.przyw...@arm.com>

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

[Riku Voipio]

On 4 September 2015 at 13:10, Andre Przywara <andre.przyw...@arm.com> wrote:
> Hi Riku,
>
> On 03/09/15 12:20, riku.voi...@linaro.org wrote:
>> From: Riku Voipio <riku.voi...@linaro.org>
>>
>> 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 <riku.voi...@linaro.org>
>
> 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

[riku . voipio]

From: Riku Voipio <riku.voi...@linaro.org>

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 <riku.voi...@linaro.org>
---
 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