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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 30.11.15 at 18:53, <cardoe@xxxxxxxxxx> wrote:
> On 11/30/15 8:36 AM, Jan Beulich wrote:
>>>>> On 24.11.15 at 18:51, <cardoe@xxxxxxxxxx> wrote:
>>> +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.
> 
> I'm not understanding the question here. Linux has 5 items in their
> list. The last two are identical to the 2 that I retained. Given other
> evolutions of the patchset I can drop the last one and keep it as just
> $ARCH_DEFCONFIG since I've always got that defined for arm and x86.

Oh, the context was ambiguous: The comment related to everything
quoted, not just the last option. I.e. I'm being irritated by there being
quite a few more config options here compared to Linux.

>>> --- 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).
> 
> One of the early on reviews told me it was not required (and I can
> confirm that I can run "git clean -xdf && cd xen && make" and it will
> build). This change makes that behavior still work.

Yes, non-cross builds of course have been working without setting
that variable. I was (not clearly enough) referring to cross builds,
including hypervisor builds on ix86 distros (which I consider at best
kind of cross, but would really deem to be native with there not
being any 32-bit hypervisor anymore).

So can you clarify what exactly breaks in which way without this
change?

>>> @@ -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?
> 
> silentoldconfig is the step that actually takes the .config and
> generates out all the files like auto.conf and generated/config.h It
> should be in xen/scripts/kconfig/Makefile.linux I believe.

Not sure what you're trying to tell me, but in any event I don't see
this to answer the question.

>>> +# 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?
> 
> I was asked to make the build work with out of tree builds and that's
> why I went this route. The rest of the Xen build system doesn't support
> out of tree builds so one can argue that's not needed.

That can't be right - $(BASEDIR) can't possibly refer to both the
source and object trees, yet the code above clearly references
both.

>>> --- /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.
> 
> I agree its squirrelly here. I believe I followed Linux and it only
> seems to come into play when you don't specify you're target arch and
> are running natively on the platform but want to build for the other
> arch. It likely can be dropped if its too confusing.

Unless you can explain this apparent backwardness, yes please.

>> Also - not 64BIT here, yet this - if added to ARM - should be added
>> here too so it can be used in common code.
> 
> That can fold into the above drop of 64BIT as configurable.

Iiuc on ARM you explicitly want this to be configurable. And even if
you make it a non-configurable setting, it should still be mirrored to
x86.

>>> +config ARCH_DEFCONFIG
>>> +   string
>>> +   default "arch/x86/configs/x86_64_defconfig"
>> 
>> x86_defconfig perhaps?
> 
> No. I was told to drop support for x86 entirely in an earlier review.
> Its not possible to configure for 32-bit x86 in v6.

x86 != 32-bit. I think you're mixing this up with ix86 or x86-32.
Here I consider x86 as to basic architecture without any
particular bit width in mind.

>>> --- /dev/null
>>> +++ b/xen/scripts/kconfig/Makefile
>>> @@ -0,0 +1,77 @@
>>> +# xen/scripts/kconfig
>>> +
>>> +MAKEFLAGS += -rR
>> 
>> Why?
> 
> kconfig in Linux makes the assumption that implicit rules and variables
> are turned off for the whole build process. Xen (oddly) does not do
> this.

Mind pointing out where this is being enforced? I've never run
across this.

> I sent another patch a while back that I planned on resending that
> disables the implicit rules and implicit variables.

That would be a prereq one then as it seems.

>>> +# 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).
> 
> Funny story about GNU Make but it clears up build problems between 3.81
> and 4.0 on machines that I tested this on.

Then you'd have to explain the issue in a comment, since otherwise
the first one to seriously look at this would be tempted to clean it up.
(But I guess a better solution could be found than making both of
these 4-line constructs.)

>>> +# 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)?
> 
> From an earlier review I dropped the usage of Config.mk because I was
> told the hypervisor (xen/) should build without ./configure being run.
> The C++ is needed for the kconfig frontends.

But Config.mk has nothing to do with top level configure, and is e.g.
being included from xen/Rules.mk.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel