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

[Doug Goldstein <cardoe@xxxxxxxxxx>]

On 12/1/15 5:22 AM, Jan Beulich wrote:
>>>> 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.

So all these items are in Linux. And they seem to actually be necessary.
You'll just find them in different files (e.g. init/Kconfig) that I
didn't import because we really didn't need them for anything. Xen has
SRCARCH and ARCH. (In fact Xen had SRCARCH in the Makefiles and I
removed it in a patch about a month ago since it wasn't used and was
inconsistent with XEN_TARGET_ARCH/XEN_TARGET_SUBARCH. SRCARCH is the
general platform (e.g. x86/arm) while ARCH is the specific variant
(x86_64/arm64/arm32). Unfortunately a bunch of the machinery in kconfig
utilities these variables and when I tried to switch to
XEN_TARGET_ARCH/XEN_TARGET_SUBARCH it caused me to have to change the
pieces I copied from Linux (this is being discussed separately in this
patch series as undesirable by Ian).


> 
>>>> --- 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?

The ability to "git clone git://xen && cd xen/xen && make" because I
couldn't pick the correct defconfig to use. See the rules just below
this reply in xen/Makefile where they are used.

> 
>>>> @@ -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.

So maybe I'm not understanding your question. I don't understand why $*
would need to be passed on. The target "silentoldconfig" in the kconfig
Makefile takes care of building all of the files that the rule could
match. That's what I tried to say in my original response. I believe
your point is that I would have to pass the target file on like the
below implicit case?

.PHONY: all
all: something otherthing foo.c
        @echo "all = $*"

.PHONY: %thing
%thing:
        @echo "implicit = $*"

.PHONY: foo.c
foo.c:
        @echo "explicit = $*"

$ make
implicit = some
implicit = other
explicit = foo
all =


> 
>>>> +# 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.

I can drop it in as many places as possible if that's desirable.

> 
>>>> --- /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.

ok. Well the syntax is still "arch/SUBARCH/configs/ARCH_defconfig" so
the original is correct. There is no defconfig for the ambiguous x86
family. You're either building for x86_64 or x86_32 (which I referred to
as x86 in my original response).

This defconfig is for the 64-bit architecture of x86 (x86_64) and there
for its named correctly.

> 
>>>> --- /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.

I've sent this an its landed now so I'll remove that.

> 
>>>> +# 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.)

I've made a note to recreate the test that I had to write with makefiles
(unfortunately I've deleted it once I came up with the solution) and
attach it here.

> 
>>>> +# 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
> 

I'm not including Config.mk in those build files because it includes a
lot and many of them are redefinitions of what kconfig does. All I would
get out of it would be HOSTCC.

$ wc -l config/x86_64.mk config/Linux.mk config/StdGNU.mk Config.mk
   32 config/x86_64.mk
    3 config/Linux.mk
   50 config/StdGNU.mk
  287 Config.mk
  372 total

If there was a bit more granularity (config/host.mk) it wouldn't be so bad.

-- 
Doug Goldstein

Attachment: signature.asc
Description: OpenPGP digital signature

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