Re: [PATCHv6] 02/28] build: build Kconfig and config rules

["Jan Beulich" <JBeulich@suse.com>]

>>> On 24.11.15 at 18:51, <cardoe@cardoe.com> wrote:
> --- a/.gitignore
> +++ b/.gitignore
> @@ -217,6 +217,11 @@ tools/xentrace/tbctl
>  tools/xentrace/xenctx
>  tools/xentrace/xentrace
>  xen/.banner
> +xen/.config
> +xen/.config.old
> +xen/defconfig
> +xen/**/*.cmd
> +xen/**/modules.order

The last two seem rather dubious to me in our environment.

> @@ -239,6 +244,9 @@ xen/include/headers++.chk
>  xen/include/asm
>  xen/include/asm-*/asm-offsets.h
>  xen/include/compat/*
> +xen/include/config.h

Linux doesn't seem to generate a file like this. Is this perhaps a
stale entry you're copying over?

> --- /dev/null
> +++ b/xen/Kconfig
> @@ -0,0 +1,26 @@
> +#
> +# For a description of the syntax of this configuration file,
> +# see Documentation/kbuild/kconfig-language.txt from the Linux

This path needs fixing.

> +# kernel source tree.
> +#
> +mainmenu "Xen/$SRCARCH $XEN_FULLVERSION Configuration"
> +
> +config SRCARCH
> +	string
> +	option env="SRCARCH"
> +
> +config ARCH
> +	string
> +	option env="ARCH"
> +
> +source "arch/$SRCARCH/Kconfig"
> +
> +config XEN_FULLVERSION
> +	string
> +	option env="XEN_FULLVERSION"
> +
> +config DEFCONFIG_LIST
> +	string
> +	option defconfig_list
> +	default "$ARCH_DEFCONFIG"
> +	default "arch/$SRCARCH/defconfig"

Linux'es variant has just SRCARCH and the source directive. Why
do we need so much more here? In any even I again think you
should at least briefly explain any extensions you do in the commit
message.

> --- a/xen/Makefile
> +++ b/xen/Makefile
> @@ -17,6 +17,9 @@ export XEN_ROOT := $(BASEDIR)/..
>  
>  EFI_MOUNTPOINT ?= $(BOOT_DIR)/efi
>  
> +# Don't break if the build process wasn't called from the top level
> +XEN_TARGET_ARCH ?= $(shell uname -m)

I don't see the need for this. We already require setting this on the
command line when invoking a build process bypassing the top level
directory. Later in the series, when actually using the produced
xen/.config, I could see this becoming a nice enhancement (by
reading the value from the file).

> @@ -216,3 +220,16 @@ FORCE:
>  
>  %/: FORCE
>  	$(MAKE) -f $(BASEDIR)/Rules.mk -C $* built_in.o built_in_bin.o
> +
> +kconfig := silentoldconfig oldconfig config menuconfig defconfig \
> +	nconfig xconfig gconfig savedefconfig listnewconfig olddefconfig
> +.PHONY: $(kconfig)
> +$(kconfig):
> +	$(MAKE) -f $(BASEDIR)/scripts/kconfig/Makefile ARCH=$(XEN_TARGET_ARCH) $@
> +
> +$(BASEDIR)/include/config/%.conf: $(BASEDIR)/include/config/auto.conf.cmd
> +	$(Q)$(MAKE) -f $(BASEDIR)/scripts/kconfig/Makefile ARCH=$(XEN_TARGET_ARCH) silentoldconfig

Okay, I have found the Linux original to this and it's the same there,
but I'd like to ask anyway: How can this work without $* getting
passed on?

> +# Allow people to just run `make` as before and not force them to configure
> +$(BASEDIR)/.config $(BASEDIR)/include/config/auto.conf.cmd: ;
> +	$(Q)$(MAKE) -f $(BASEDIR)/scripts/kconfig/Makefile ARCH=$(XEN_TARGET_ARCH) defconfig

Is this correct? If I have xen/.config but no
xen/include/config/auto.conf.cmd I surely don't want "defconfig" to
be run (unless "defconfig" honors xen/.config, in which case the
goal's name would be questionable)?

Also do you really need all the explicit $(BASEDIR) references in the
rules above?

> --- /dev/null
> +++ b/xen/arch/arm/configs/arm32_defconfig
> @@ -0,0 +1 @@
> +CONFIG_64BIT=n
> diff --git a/xen/arch/arm/configs/arm64_defconfig 
> b/xen/arch/arm/configs/arm64_defconfig
> new file mode 100644
> index 0000000..e69de29

As said before I'm not really up to speed with defconfigs, but the
arm64 variant being empty but the arm32 one disabling 64BIT
seems backwards: You don't even have a choice on arm32.

> --- /dev/null
> +++ b/xen/arch/x86/Kconfig
> @@ -0,0 +1,18 @@
> +config X86_64
> +	def_bool y
> +
> +config X86
> +	def_bool y
> +	select HAS_GDBSX

Didn't you mean to convert HAS_* in subsequent patches?

Also - not 64BIT here, yet this - if added to ARM - should be added
here too so it can be used in common code.

> +config ARCH_DEFCONFIG
> +	string
> +	default "arch/x86/configs/x86_64_defconfig"

x86_defconfig perhaps?

> --- /dev/null
> +++ b/xen/scripts/kconfig/Makefile
> @@ -0,0 +1,77 @@
> +# xen/scripts/kconfig
> +
> +MAKEFLAGS += -rR

Why?

> +# default rule to do nothing
> +all:
> +
> +XEN_ROOT = $(CURDIR)/..
> +
> +# Xen doesn't have a silent build flag
> +quiet := silent_
> +Q := @
> +kecho := :
> +
> +# eventually you'll want to do out of tree builds
> +srctree = $(XEN_ROOT)/xen
> +objtree = $(srctree)
> +src := scripts/kconfig
> +obj := $(src)
> +KBUILD_SRC :=
> +
> +# handle functions (most of these lifted from different Linux makefiles
> +dot-target = $(dir $@).$(notdir $@)
> +depfile = $(subst $(comma),,$(dot-target).d)
> +basetarget = $(basename $(notdir $@))
> +cmd = $(cmd_$(1))
> +if_changed = $(if y, \
> +	@set -e; \
> +	$(cmd_$(1)); \
> +	)
> +
> +if_changed_dep = $(if y, \
> +	@set -e;	\
> +	$(cmd_$(1)); \
> +	)

Considering how much you stripped these fro their Linux original, why
did you leave the always true $(if ...) in here, and why did you not
consolidate things only single lines (helping readability).

> +define multi_depend
> +$(foreach m, $(notdir $1), \
> +	$(eval $(obj)/$m: \
> +	$(addprefix $(obj)/, $(foreach s, $3, $($(m:%$(strip $2)=%$(s)))))))
> +endef

Do you really need this?

> +# Set our default defconfig file
> +KBUILD_DEFCONFIG := $(ARCH)_defconfig
> +
> +# provide our shell
> +CONFIG_SHELL := $(SHELL)
> +
> +# provide the host compiler
> +HOSTCC := gcc
> +HOSTCXX := g++

Why is the C setting from ./Config.mk not good enough (the C++
one should then be added there if indeed needed, which I doubt)?

Jan